search

hapijs / crumb

CSRF crumb generation and validation for hapi
Other
171 stars 50 forks source link

crumb fails when there are multiple cookies with the same name #118

Closed bodawei closed 5 years ago

bodawei commented 6 years ago

We ran into a case, today, (using the Postman utility) where we inadvertently ended up with two csrf tokens in our cookies (e.g. crumb=12345; ... crumb=5678;). When crumb retrieves the cookies:

        let crumb = request.state[settings.key];

it returns an array of the cookie values, which of course don't match header !== request.plugins.crumb. While it is completely reasonable to fail in this case (it is, after all, not a match!), it would probably help developers identify what is going wrong faster if this were checked.

My suggestion is to add another check in onPostAuth something like:

if (Array.isArray(request.plugins.crumb)) {
    request.log(['crumb', 'unauthorized', 'duplicate-cookies'], 'validation failed');
}

If this seems acceptable to you, I can put together a pull request with a chance like this.

spanditcaa commented 5 years ago

I've seen a similar issue with authentication cookies where our resolution was to log that multiple were present, but then proceed to validate using the first cookie in the array. Yes - please PR @bodawei.

hueniverse commented 5 years ago

@spanditcaa A PR was submitted two months ago per your request. Let me know if you can no longer maintain this module.

spanditcaa commented 5 years ago

@hueniverse I had some idea that I'd figure out why crumb might set multiple cookies - suspecting there must be a different path or subdomain in play, but never got to that. @bodawei this was merged as crumb@7.2.3 - thanks for the PR and sorry for the delay.

hueniverse commented 5 years ago

@spanditcaa make sure you create and assign milestones. The current open one is outdated and this issue was not assigned to any.

bodawei commented 5 years ago

@spanditcaa Thanks for the merge!

lock[bot] commented 4 years ago

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.