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

Fix * behavior to be standards compliant. #57

Merged
merged 2 commits into from
Jul 25, 2018
Merged

Fix * behavior to be standards compliant. #57

merged 2 commits into from
Jul 25, 2018

Conversation

ejcx
Copy link
Contributor

@ejcx ejcx commented Jul 25, 2018

In section 6.1 of the CORS standard is talks about this exact situation.
Even though you have built in a guard-rail in to the library which will print
a nice warning, it is preferred to rely on the security already built in to
the standard.

When ACAO: * and ACAC: true are both specified the browser will refuse to
make the credentialed request.

Refer to the standard: https://www.w3.org/TR/cors/

In section 6.1 of the CORS standard is talks about this exact situation.
Even though you have built in a guard-rail in to the library which will print
a nice warning, it is preferred to rely on the security already built in to
the standard.

When ACAO: * and ACAC: true are both specified the browser will refuse to
make the request.

Refer to the standard: https://www.w3.org/TR/cors/
@ejcx
Copy link
Contributor Author

ejcx commented Jul 25, 2018

References: #55 which is the original conversation, which cites my blog post about this!

@rs rs merged commit e84ea22 into rs:master Jul 25, 2018
@ejcx
Copy link
Contributor Author

ejcx commented Jul 25, 2018

That was fast! Cheers!

@c2h5oh
Copy link

c2h5oh commented Jul 26, 2018

You might have jumped a gun on that one: go-chi/cors#6 (comment)

@VojtechVitek
Copy link

@c2h5oh are you suggesting we should revert this PR based on go-chi/cors#6 (comment) ?

@c2h5oh
Copy link

c2h5oh commented Jul 26, 2018

Yes I am or at least I'm recommending alternative solution is used.

  1. This package was compliant with the section of the standard mentioned in the pull request description

  2. With this pull request package is no longer compliant with the very section of the standard mentioned in pull request description

  3. This is a breaking change for those who were relying on this valid behavior and no setting is provided to revert to the standard compliant one

This pull request breaks a standard compliant behavior, because the standard includes something that can result in security vulnerability if misconfigured / used by someone who doesn't understand implications of that part of the standard.

I'm all for having secure defaults but completely breaking something because if misconfigured it might be a security issue? hell no.

@ejcx
Copy link
Contributor Author

ejcx commented Jul 26, 2018

Lol. I strongly recommend against this person's guidance. There is a conversation in the other thread that's worth reading, including other libraries that have had similar CORS vulnerabilities...

@c2h5oh
Copy link

c2h5oh commented Jul 26, 2018

@ejcx simple yes or no answer: is any of my 3 statements from the message above false?

and I agree - do read the other thread.

@rs
Copy link
Owner

rs commented Jul 26, 2018

Added a note about this change in the README, and the instruction on how to restore the previous behavior if necessary. Please see c084f7a.

@VojtechVitek
Copy link

@rs cool, thanks for this. Do you think we should release v1.5.0 or v2.0.0, since this is a breaking change?

@rs
Copy link
Owner

rs commented Jul 26, 2018

There is no break of API, and I still can't find a good reason to configure a service that way, so I think a v1.5.0 is enough.

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.

4 participants