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

Default CSRF tokens to the root path "/" #1248

Merged
merged 1 commit into from Aug 16, 2016

Conversation

Projects
None yet
3 participants
@MaxGabriel
Copy link
Member

MaxGabriel commented Jul 11, 2016

  • The default path of cookies is the current path making the request
    • e.g. an AJAX request made from http://example.com/foo/bar would be /foo
    • This causes multiple CSRF tokens to build up as you navigate a site
    • This will cause errors if the CSRF tokens have different values, and an invalid token is sent.
  • Closes #1247

@MaxGabriel MaxGabriel force-pushed the fixCsrfPath branch from e2f206f to 269f232 Jul 11, 2016

@MaxGabriel

This comment has been minimized.

Copy link
Member Author

MaxGabriel commented Jul 11, 2016

@bitemyapp

This comment has been minimized.

Copy link
Contributor

bitemyapp commented Jul 11, 2016

screenshot from 2016-07-11 00-27-18

been a web dev for 8 years, working dev for 10 and today-I-learned

@bitemyapp

This comment has been minimized.

Copy link
Contributor

bitemyapp commented Jul 11, 2016

No objections here since this looks like a cleaner solution than my hack.

I do have one question, how is uniqueness preserved in the cookie store? is it keyed by key+path? As long as we believe this'll be guaranteed-unique and not duplicate then 👍 from me.

@MaxGabriel

This comment has been minimized.

Copy link
Member Author

MaxGabriel commented Jul 11, 2016

I do have one question, how is uniqueness preserved in the cookie store? is it keyed by key+path? As long as we believe this'll be guaranteed-unique and not duplicate then 👍 from me.

At least for Yesod (not sure if Wai has something different), it looks like the cookies are parsed directly from the HTTP text, so you can get multiple cookies with the same name:

req <- getRequest
traceShowM "Cookies are: "
traceShowM $ reqCookies req
"Cookies are: "
[("XSRF-TOKEN","yoxecmpgmd"),("_SESSION","1q/bVJn7lnz+oigJTv2b+NosvuXa1EJXtM7Yfg8kp7uWRVXiZeoobGLM0iKZl/KCO3bi5bmwrAapH8P5hrcfpscvoh7aUtPQ7AqCJNXMmuKMLHmGDOjjnpm42A+CTE89u8UQIAJ+f/Y="),("XSRF-TOKEN","s6fpSkuJqv")]

So there's nothing guaranteeing uniqueness based on name alone, but that's OK because this PR gives the CSRF tokens all the same name. The CSRF cookies only persist for a session, so the old CSRF tokens with the wrong path will go away when the browser's closed.

I don't think this really answers your question though, because I think we need to find the code that sends cookies back to the browser to make sure it accepts cookies with the same name, as long as they have a different path. I'm a little out of it this morning, hope that was coherent.

@bitemyapp

This comment has been minimized.

Copy link
Contributor

bitemyapp commented Jul 11, 2016

I think the tension here is between making everything that is valid HTTP expressible vs. what is sensible.

I'm not really sure what duplicate cookies of the same name are supposed to mean (multiplicity? dubious.) and most web stacks I've worked with haven't supported it.

I'd like to clear up why duplicate keys are expressible and what they mean for this particular feature if possible. Changing the default path doesn't fix this problem by itself, does this change make duplicate values of the same key going out the door impossible? Is that desirable?

@MaxGabriel

This comment has been minimized.

Copy link
Member Author

MaxGabriel commented Jul 11, 2016

Ok, I was working off the idea that multiple cookies of the same name are HTTP expressible, so Yesod should support them.

My understanding of cookies:

When a client makes a request to a server, the server can send back one or more cookies using Set-Cookie header(s) (e.g. Set-Cookie: lang=en-US; Expires=Wed, 09 Jun 2021 10:18:14 GMT). When the client makes a request to the server, it sends just the cookie key/value pair (e.g. Cookie: lang=en-US).

The cookies persist on the client until the browser deletes them, based on their expiration settings/the user deleting them manually. The server can delete cookies early by sending back a new cookie with the same name and an expires date in the past.

Stuff I read:

This issue:

Clients can build up multiple cookies with the same name when they have different paths. The client "SHOULD sort the cookie list... with longer paths before shorter paths", but "servers SHOULD NOT rely upon the serialization order" (SO). This can happen as the user browses your site and different paths store different cookies. It can also happen when the server sends out multiple cookies with different paths in a single request, like so:

setCookie def { setCookieName = "name", setCookieValue = "value1", setCookiePath = Just "/foo" }
setCookie def { setCookieName = "name", setCookieValue = "value2", setCookiePath = Just "/" }

I'm pretty sure the issue we ran into here was that as users navigated between different paths, different cookies built up, since the CSRF code only sets the cookie once per request.

As far as preventing multiple duplicate cookies from going out, RFC 6265 does have this to say:

Servers SHOULD NOT include more than one Set-Cookie header field in
the same response with the same cookie-name. (See Section 5.2 for
how user agents handle this case.)

I can't find where in Section 5.2 they expand on that. It sort of surprises me the RFC didn't say "with the same cookie-name [and path and domain]". Anyway, do you think there's any value in preventing multiple cookies from going out if they have the same name? It seems like an edge case, and one only triggered by a user actively trying to do it.

Finally, it seems wrong to me to remove duplicate incoming cookies from APIs like reqCookies. The server can't prevent this happening, the best it can do is choose one of them. But that just makes it harder to tell there's an issue going on causing multiple cookies to be sent from the client. The current way (a list of key-value pairs in tuples) is fine for lookups, too.

@bitemyapp

This comment has been minimized.

Copy link
Contributor

bitemyapp commented Jul 13, 2016

Anyway, do you think there's any value in preventing multiple cookies from going out if they have the same name? It seems like an edge case, and one only triggered by a user actively trying to do it.

I think the server should only send one value out of a particular name, preferring the "newest" possible value where a tiebreaker is needed. That seems more correct based on what the standard says.

I don't think I'd do anything about duplicates when it comes to accepting the data though. The author of the server needs to decide how, if at all, they want to handle that but most users are relying on middleware that will silently fail in weird ways (like CSRF middleware did for us) when they encounter this. YMMV.

Default CSRF tokens to the root path "/"
* The default path of cookies is the current path making the request
  * e.g. an AJAX request made from http://example.com/foo/bar would be /foo
  * This causes multiple CSRF tokens to build up as you navigate a site
  * This will cause errors if the CSRF tokens have different values, and an invalid token is sent.
* Closes #1247

@MaxGabriel MaxGabriel force-pushed the fixCsrfPath branch from 269f232 to e628736 Aug 16, 2016

@MaxGabriel MaxGabriel merged commit 2ec1336 into master Aug 16, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@snoyberg snoyberg removed the in progress label Aug 16, 2016

@snoyberg snoyberg deleted the fixCsrfPath branch Oct 7, 2016

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.