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

More wildcards in CORS when used without credentials #298

Merged
merged 6 commits into from
May 24, 2016
Merged

Conversation

annevk
Copy link
Member

@annevk annevk commented May 5, 2016

Fixes #251 and fixes #252.

@annevk
Copy link
Member Author

annevk commented May 5, 2016

@tyoshino review appreciated. @roryhewitt, @craigfrancis, @mozfreddyb, you might be interested in reviewing too. @sicking, I included your suggestion around Authorization.

@@ -1856,12 +1858,14 @@ <h4 id="http-new-header-syntax"><span class="secno">4.2.4 </span>HTTP new-header
<pre>Access-Control-Request-Method = <a class="external" data-anolis-spec="http" href="https://tools.ietf.org/html/rfc7230#section-3.1.1">method</a>
Access-Control-Request-Headers = #<a class="external" data-anolis-spec="http" href="https://tools.ietf.org/html/rfc7230#section-3.2">field-name</a>

Access-Control-Allow-Origin = origin-or-null / "*"
wilrdcard = "*"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I highly suspect this to be a typo and it should read "wildcard"?

@annevk
Copy link
Member Author

annevk commented May 5, 2016

I thought a bit about this and I think one thing we should probably change is to make the CORS-preflight cache support wildcards directly. That could have a pretty dramatic impact on reducing chattiness.

@roryhewitt
Copy link

@annevk that would be nice. +1 from me.

Access-Control-Allow-Credentials = %x74.72.75.65 ; "true", case-sensitive
Access-Control-Expose-Headers = #<a class="external" data-anolis-spec="http" href="https://tools.ietf.org/html/rfc7230#section-3.2">field-name</a>
Access-Control-Expose-Headers = field-name-or-wildcard
Copy link

@roryhewitt roryhewitt May 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be #field-name-or-wildcard (with a leading #?), and also contain the link to the IETF?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should have the leading #, but since this is not an IETF production this should not link.

@annevk
Copy link
Member Author

annevk commented May 9, 2016

I now changed it so that the CORS-preflight cache can store wildcards which should reduce chattiness a whole lot for non-credentialed requests (at the point user agents implement this, of course).

@annevk
Copy link
Member Author

annevk commented May 12, 2016

@roryhewitt @tyoshino would you be so kind to look over the latest changes? I think this is ready, but I just want to be sure.

@roryhewitt
Copy link

@annevk I think the second example in section 4.2.6 should have the last line changed to something like this:

This is because bar.invalid needs to explicitly share each header by listing their names in the Access-Control-Expose-Headers response header. Alternatively, bar.invalid could have returned Access-Control-Expose-Headers: * which would share all response headers.

Basically showing developers that they can specify Access-Control-Expose-Headers: *.

@annevk
Copy link
Member Author

annevk commented May 13, 2016

Ended up writing an extra paragraph.

@roryhewitt
Copy link

@annevk Perfect!

<li><p>Let <var>headerNames</var> be the result of
<span title=concept-header-parse>parsing</span>
`<code title=http-access-control-expose-headers>Access-Control-Expose-Headers</code>` in
<var>response</var>'s <span title=concept-response-header-list>header list</span>.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

paragraph not enclosed

@tyoshino
Copy link
Member

tyoshino commented May 24, 2016

Sorry for delay.

lgtm but I want to make sure that it's fine that

  • a user-agent may end up sending no credential even when credentials mode is set to include
  • in such case, the server cannot know that the client is using "credentials mode == include omit" only by inspecting the received request, and therefore it's not safe to use the wildcard.

This means that developers are basically required to coordinate use of the wildcard between client code and server. Right?

[Edited] include -> omit

@annevk
Copy link
Member Author

annevk commented May 24, 2016

@tyoshino yeah, I think that coordination has effectively always been required (it also applies to the wildcard case for origin). It might help if we start communicating that more clearly, but that would be a new request header and a separate discussion.

@tyoshino
Copy link
Member

ok. lgtm

@annevk
Copy link
Member Author

annevk commented May 24, 2016

Thank you (and the others)!

@annevk annevk merged commit cdbb13c into master May 24, 2016
@annevk annevk deleted the cors-wildcard branch May 24, 2016 09:42
@roryhewitt
Copy link

@annevk forgive my ignorance, but I'm not sure how this all goes together...

If these new wildcard values are now in the master branch (and are what I see when I go to https://fetch.spec.whatwg.org/#http-new-header-syntax), does that mean that they are officially supported by all (major) browsers? Or is this just a new version of the Fetch spec, and as browsers add support for them, they will publicize this fact?

@annevk
Copy link
Member Author

annevk commented May 24, 2016

@roryhewitt the latter. I wish it were that easy!

@tonyjin
Copy link

tonyjin commented Sep 15, 2016

Also not sure how this all goes together, but I noticed the following two don't match:
https://www.w3.org/TR/cors/#access-control-allow-headers-response-header
https://fetch.spec.whatwg.org/#http-new-header-syntax

Specifically:
Access-Control-Allow-Headers: "Access-Control-Allow-Headers" ":" #field-name (in W3)
Access-Control-Allow-Headers = #field-name-or-wildcard (in Fetch Spec)

As I was writing this I realized the W3 recommendation was published in Jan 2014, so I guess we'll have to wait for a newer version to be published?

Does the latest fetch spec's definition of Access-Control-Allow-Headers supersede the W3 recommendation?

@annevk
Copy link
Member Author

annevk commented Sep 15, 2016

@tonyjin yes it does. W3C's copy of CORS is obsolete and should not be used. Unfortunately they have so far refused to clearly mark it as such.

@roryhewitt
Copy link

Hey Tony,

Yes, the fetch spec on WHATWG (including the CORS headers) supersedes the
W3C spec.

Unfortunately, it appears to be a low priority to get this 'publicized' by,
for instance, updating the W3C spec to make it clear.

Note also that, although the latest fetch spec includes wildcards for the
ACAM, ACAH and ACEH headers, I'm not sure whether these special values are
actually supported by any browsers yet...

Perhaps @annevk can comment?

Rory

Sent from my phone - please excuse spelling & brevity...

On Sep 15, 2016 12:34 PM, "Tony Jin" notifications@github.com wrote:

Also not sure how this all goes together, but I noticed the following two
don't match:
https://www.w3.org/TR/cors/#access-control-allow-headers-response-header
https://fetch.spec.whatwg.org/#http-new-header-syntax

Specifically:
Access-Control-Allow-Headers: "Access-Control-Allow-Headers" ":"
#field-name (in W3)
Access-Control-Allow-Headers = #field-name-or-wildcard (in Fetch Spec)

As I was writing this I realized the W3 recommendation was published in
Jan 2014, so I guess we'll have to wait for a newer version to be published?

Does the latest fetch spec's definition of Access-Control-Allow-Headers
supersede the W3 recommendation?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#298 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ALBf60j_0OIhrc8xQunqDE1LaTQOZ8v3ks5qqZ2-gaJpZM4IYBHx
.

@tonyjin
Copy link

tonyjin commented Sep 16, 2016

Thanks for clarifying and working on this @annevk & @roryhewitt - I ran into this PR after getting frustrated at adding Access-Control-Allow-Headers: [a bajillion headers] in nginx 🙄

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