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

Ignore CORS #61

Closed
hueniverse opened this issue Oct 16, 2015 · 11 comments
Closed

Ignore CORS #61

hueniverse opened this issue Oct 16, 2015 · 11 comments

Comments

@hueniverse
Copy link
Member

@hueniverse hueniverse commented Oct 16, 2015

I don't know why the change was made to skip crumb protection when CORS is enabled but please revert that change. While theoretically, a properly configured CORS deployment does not need much of this extra protection, this is a violation of security layering principle.

Also, almost no one configures CORS correctly.

@stongo

This comment has been minimized.

Copy link
Contributor

@stongo stongo commented Oct 19, 2015

Crumb protection is not being skipped when CORS is turned on. The crumb cookie will only be set for domains listed in the allowOrigins setting, but crumb protection will still be enabled for all requests. The whole purpose of the security fix was to not leak the crumb token to unauthorized Cross Origin requests.
If crumb is not currently behaving like this, it is a bug.
Will verify today.

@hueniverse

This comment has been minimized.

Copy link
Member Author

@hueniverse hueniverse commented Oct 19, 2015

We might need to expose the Origin match utility in hapi then to do this properly.

@hueniverse

This comment has been minimized.

Copy link
Member Author

@hueniverse hueniverse commented Oct 26, 2015

Does it just not set the cookie or also doesn't validate the form if no cookie is set?

@stongo

This comment has been minimized.

Copy link
Contributor

@stongo stongo commented Nov 9, 2015

@hueniverse if a CORS request is from an unknown Origin, the generate function isn't called and a cookie isn't returned.
That is the only code affected by CORS
Where is the Origin match utility in HAPI? I would be happy to integrate that

@hueniverse

This comment has been minimized.

Copy link
Member Author

@hueniverse hueniverse commented Jan 2, 2016

Can you explain this to me: https://github.com/hapijs/crumb/blob/master/lib/index.js#L203-L206

It makes no sense. You are comparing the incoming host header to the connection host. The way you are testing it is based on the old shot module behavior where it didn't set the host header properly. I upgraded to hapi 11 and half the tests broke.

@hueniverse

This comment has been minimized.

Copy link
Member Author

@hueniverse hueniverse commented Jan 2, 2016

In hapi v12 you should use request.info.cors.isOriginMatch exclusively without using any of the custom config stuff you have in crumb right now. IOW, we need a new major version of crumb for hapi v12.

@stongo

This comment has been minimized.

Copy link
Contributor

@stongo stongo commented Jan 2, 2016

Ok will do. Thanks for exposing that
On Jan 2, 2016 2:23 PM, "Eran Hammer" notifications@github.com wrote:

In hapi v12 you should use request.info.cors.isOriginMatch exclusively
without using any of the custom config stuff you have in crumb right now.
IOW, we need a new major version of crumb for hapi v12.


Reply to this email directly or view it on GitHub
#61 (comment).

@stongo

This comment has been minimized.

Copy link
Contributor

@stongo stongo commented Jan 12, 2016

Using request.info.cors.isOriginMatch alone won't allow a route to set the crumb for a CORS client and same-origin client (ie server rendered page)
That was what the extra logic had been for. Set crumb if it's a same-origin request OR an allowed CORS origin.
Maybe we don't support that, but it had been a request that came in an issue.

@hueniverse

This comment has been minimized.

Copy link
Member Author

@hueniverse hueniverse commented Jan 12, 2016

What does it mean "same origin request"? This line: https://github.com/hapijs/crumb/blob/master/lib/index.js#L203-L206 makes no sense as described above. Upgrade the tests to hapi v12 and see what happens.

@stongo

This comment has been minimized.

Copy link
Contributor

@stongo stongo commented Jan 12, 2016

I have already upgraded and tested locally (hapi12) branch.
Sorry for not explaining well. What I'm trying to describe is an edge case and probably not worth supporting.
Should be able to PR the update later today

@stongo

This comment has been minimized.

Copy link
Contributor

@stongo stongo commented Jan 15, 2016

6.0.0 release addresses this

@stongo stongo closed this Jan 15, 2016
@stongo stongo added this to the 6.0.0 milestone Jan 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.