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

give simpleHeaders to simpleCorsResourcePolicy #26

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

2mol
Copy link

@2mol 2mol commented Aug 30, 2019

It makes more sense to give simpleHeaders explicitely to the simpleCorsResourcePolicy, and by extension to simpleCors.

The reason for this is that the preflight request chrome and firefox make vie OPTIONS includes content-type, resulting in a 400. As the documentation states, all simpleHeaders except content-type are implicit when passing the empty list to corsRequestHeaders.

I hope this is ok, let me know if I misunderstood the intention regarding simpleCors.

It makes more sense to give `simpleHeaders` explicitely to the `simpleCorsResourcePolicy`, and by extension to `simpleCors`.

The reason for this is that the preflight request chrome and firefox make vie OPTIONS includes `content-type`, resulting in a 400. As the documentation states, all `simpleHeaders` _except_ `content-type` are implicit when passing the empty list to `corsRequestHeaders`.

I hope this is ok, let me know if I misunderstood the intention regarding `simpleCors`.
@2mol 2mol mentioned this pull request Oct 13, 2019
@guaraqe
Copy link

guaraqe commented Mar 29, 2020

I had the same problems, and the fix above solved it. In my opinions this gives a better out-of-the-box experience.

@larskuhtz
Copy link
Owner

I hesitate to take this change without carefully reading the related sections in the CORS standard. The behavior of the server guides the browser to reject unsafe scenarios. Thus, a more liberal policy is always more convenient, but, generally, not safer.

I am happy to merge this when I am convinced that it is in accordance with the standard, doesn't affect security for other users of the library, and doesn't break existing uses. In case of doubt, I prefer some inconvenience for some use cases.

@larskuhtz
Copy link
Owner

Unfortunately, I am not working with CORS on a daily basis and re-familiarizing myself with the standard always takes some time, which is the reason why this package is maintained rather conservatively.

@larskuhtz
Copy link
Owner

I appreciate that people use this package and file issues and submit PRs. And am sorry, that I am slow in keeping up with responding.

@larskuhtz
Copy link
Owner

In concrete, I am concerned that the change in the PR may conflict with the following paragraph from section 6.2 of the CORS standard:

  1. If each of the header field-names is a simple header and none is Content-Type, this step may be skipped.

    Add one or more Access-Control-Allow-Headers headers consisting of (a subset of) the list of headers.

    Note: If a header field name is a simple header and is not Content-Type, it is not required to be listed. Content-Type is to be listed as only a subset of its values makes it qualify as simple header.

    Note: Since the list of headers can be unbounded, simply returning supported headers from Access-Control-Allow-Headers can be enough.

@2mol
Copy link
Author

2mol commented Mar 29, 2020

I appreciate that people use this package and file issues and submit PRs. And am sorry, that I am slow in keeping up with responding.

No need to apologise! I for one appreciate that you are careful and methodical about these changes, I agree with your points about being conservative instead of convenient.

I have to admit I have to spend more time parsing that section 6.2 to understand it :)

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

Successfully merging this pull request may close these issues.

3 participants