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

Make CORS safer by not reflecting origin for * #10

Merged
merged 1 commit into from May 7, 2017

Conversation

Projects
None yet
2 participants
@ejcx
Copy link

commented May 7, 2017

This seems like it's small and nit-picky, but I think it
is pretty unexpected for someone using this to set a policy
of ACAO * and have the Origin reflected, and it has big security
implications as well.

Evan Johnson
Make CORS safer by not reflecting origin for *
This seems like it's small and nit-picky, but I think it
is pretty unexpected for someone using this to set a policy
of ACAO * and have the Origin reflected.

@ejcx ejcx force-pushed the ejcx:ejcx/SafeCors branch from fc079c8 to cc1cf75 May 7, 2017

@captncraig

This comment has been minimized.

Copy link
Owner

commented May 7, 2017

Hmm, I'll need to do some more research on this. It is a pattern I have seen in some other packages, which is why I did it initially. Not finding a ton of concrete recommendations though.

I have found this: http://stackoverflow.com/a/19744754/121660 which says it might not be possible yo use * origin in combination with the allow credentials flag. Recommendation seems to be * with no credentials allowed or concrete domain with credentials allowed.

As I write this out, I tend to agree with you. Converting * to concrete domains may allow scenarios to work that are less secure, and we are allowing users to shoot their own foots. Lemme think it over some more.

@ejcx

This comment has been minimized.

Copy link
Author

commented May 7, 2017

The other packages all do it wrong I'm sorry to say (and it's a huge security problem where people accidentally turning off security). You're right. "Access-Control-Allow-Origin: *" and Allow-Credentials: true, the browser will ignore the AllowCredentials. * means "credentials not allowed no matter what".

The problem with this package is that when people specify *, the CORS handler ends up reflecting the origin header instead of returning * like the user specified.

Here is some research I did on the topic: https://ejj.io/misconfigured-cors/. I occasionally look for new places to track CORS problems down and found this today.

@ejcx

This comment has been minimized.

Copy link
Author

commented May 7, 2017

To be a little more explicit about the problem case I envision.

I envision someone specifying origins "*" and allow_credentials true. Right now, that has the effect of turning off web security because * actually means "reflect whatever the origin header was".

After this change, people who specify "*" and allow_credentials true don't turn off security, and instead safely return the Access-Control-Allow-Origin: * header.

@captncraig

This comment has been minimized.

Copy link
Owner

commented May 7, 2017

I agree. Every other header is essentially passed through from user input. We should be consistent. And this doesn't hide a bad configuration from the user.

@captncraig captncraig merged commit 153f484 into captncraig:master May 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.