Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Defining Access-Control-Expose-Headers parsing #827

Open
annevk opened this issue Nov 1, 2018 · 5 comments
Open

Defining Access-Control-Expose-Headers parsing #827

annevk opened this issue Nov 1, 2018 · 5 comments

Comments

@annevk
Copy link
Member

annevk commented Nov 1, 2018

This is for #814. There's basically two ways to do this:

  1. Get the value.
  2. If null, return.
  3. Check that value contains only token or comma. Return if it contains anything outside that range.
  4. Split on comma.
  5. Trim tab or space and ignore empty values. (Chrome doesn't do this, possibly due to https://bugs.chromium.org/p/chromium/issues/detail?id=896233 @MattMenke2. See Fetch: Access-Control-Expose-Headers parsing web-platform-tests/wpt#13849 for tests.)
  6. Now we have values.

Or say something about parsing per the ABNF:

  1. Get the value.
  2. Parse value per the ABNF for Access-Control-Expose-Headers. (Link it? Inline the ABNF? What's the return value of parsing per ABNF?)
  3. Now we have values.

(This doesn't always work since some stuff isn't really parsed per ABNF per se, but just literally compared or parsed differently due to compatibility reasons, but it could maybe work here, except it seems less clear to me and it's not entirely clear how ABNF maps to data structures and stuff.)

cc @mnot

@MattMenke2
Copy link
Contributor

MattMenke2 commented Nov 1, 2018

I can't find the code in Chrome to handle Access-Control-Expose-Headers - it may use string composition, or some other mechanism.

Looking at the tests, I don't think any inconsistencies in Chrome are too likely to be related to https://crbug.com/896233 - that would only basically affect handling of the ",bb-8" and "Access-Control-Expose-Headers: bb-8,,@#$#%%&^&^*()()11!" tests, and I think the buggy old behavior would actually have made the first one more likely to have Chrome pass the test, and have no effect on whether the second one does.

@yutakahirano

@MattMenke2
Copy link
Contributor

I did search for NamesStorage[8], but I wasn't creative enough in my searches through obfuscated code. :(

@annevk
Copy link
Member Author

annevk commented Nov 2, 2018

@MattMenke2 right, Chrome now fails the ,bb-8 test (it doesn't result in exposure of the bb-8 header, whereas before it would likely have exposed bb-8 as is required behavior for this header. So that's a regression of sorts in a way.

(Still looking for thoughts on ABNF versus fully-defined parsers. I'm somewhat inclined to go with the latter, but I'll do some more research.)

@MattMenke2
Copy link
Contributor

I find both the ABNF and text pseudo code a bit hard to follow, but the fully-defined parser option tends to allow for much less ambiguity, so that's what I'd prefer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants