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

Some adjustments #2

Merged
merged 3 commits into from
Apr 26, 2018
Merged

Some adjustments #2

merged 3 commits into from
Apr 26, 2018

Conversation

henri-hulski
Copy link
Member

@henri-hulski henri-hulski commented Feb 10, 2018

This PR do 3 things:

  • Omit the Access-Control-Allow-Credentials header when allow_credentials is not True.
    See documentation.
  • Set Vary header to 'Origin' when allowed_origin specifies a URI.
    See documentation.
  • Set allow_credentials by default to False.
    Specifying "*" as a wildcard for the Access-Control-Allow-Origin header is only allowed for
    requests without credentials.
    So the default 'allowed_origin': '*' and 'allow_credentials': True are in conflict which each other.

@henri-hulski
Copy link
Member Author

henri-hulski commented Feb 10, 2018

@kagesenshi Could you have a look if I'm not missing something. I make the adjustments after reading the Mozilla documentation. Maybe in practice it looks different.

@kagesenshi
Copy link
Collaborator

hmm .. while the Vary header is ok, allow-credentials header i think i need some time to validate,

as not returning true for that might cause browser to ignore the resource if the resource have credentials (authorization header, cookie, tls cert). Probably should return true if user is authorized, but i'll look at that later.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Credentials#Directives

@henri-hulski
Copy link
Member Author

henri-hulski commented Feb 12, 2018

Not sure if I follow. After the PR, when allow_credentials setting is True the Access-Control-Allow-Credentials header will be set to 'true'. When allow_credentials setting is False the Access-Control-Allow-Credentials header will be omitted.

The third change is setting the allow_credentials setting by default to False as if it would be True it would be in conflict with the default setting for allowed_origin: '*', which is only allowed for requests without credentials.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Origin#Directives

@henri-hulski
Copy link
Member Author

@kagesenshi Are you able to review the PR like this or should I split it in 3 PRs for each commit?

@henri-hulski
Copy link
Member Author

henri-hulski commented Apr 5, 2018

@kagesenshi @faassen @href could someone of you review this PR?
I really would like to release more.cors as I need it for morepath-realworld-example-app which I would like to puplish.
Together with the frontend cerebral-realworld-example-app which is also nearly ready now.

@href
Copy link
Member

href commented Apr 5, 2018

As someone without any CORS knowledge I think I should not weigh in on this.

@henri-hulski
Copy link
Member Author

@blaflamme Maybe you can review? I remember you also worked on CORS integration.

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