Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Fix * behavior to be standards compliant.#57

Merged
rs merged 2 commits into
rs:masterrs/cors:masterfrom
ejcx:ej/fix-starejcx/cors-1:ej/fix-starCopy head branch name to clipboard
Jul 25, 2018
Merged

Fix * behavior to be standards compliant.#57
rs merged 2 commits into
rs:masterrs/cors:masterfrom
ejcx:ej/fix-starejcx/cors-1:ej/fix-starCopy head branch name to clipboard

Conversation

@ejcx

@ejcx ejcx commented Jul 25, 2018

Copy link
Copy Markdown
Contributor

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

ejcx commented Jul 25, 2018

Copy link
Copy Markdown
Contributor Author

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

ejcx commented Jul 25, 2018

Copy link
Copy Markdown
Contributor Author

That was fast! Cheers!

@c2h5oh

c2h5oh commented Jul 26, 2018

Copy link
Copy Markdown

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

@VojtechVitek

Copy link
Copy Markdown

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

@c2h5oh

c2h5oh commented Jul 26, 2018

Copy link
Copy Markdown

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

ejcx commented Jul 26, 2018

Copy link
Copy Markdown
Contributor Author

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

c2h5oh commented Jul 26, 2018

Copy link
Copy Markdown

@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

rs commented Jul 26, 2018

Copy link
Copy Markdown
Owner

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
Copy Markdown

@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

rs commented Jul 26, 2018

Copy link
Copy Markdown
Owner

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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.