Set a Path on the CSRF cookie #944
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Without setting a Path, the UA will infer the Path from the URL path,
meaning that it will take the "directory name" of a URL path (so for a
response for a resource at `/index`, the path would be inferred as `/`,
and for a response for `/foo/bar` it would be inferred to be `/foo/`
etc.).
This is problematic with CSRF for multiple reasons:
- It causes a proliferation of CSRF cookies which pollute client-side
storage.
- It breaks CSRF when requests are sent across paths. For example, if a
resource at `/foo/bar` contains a form which submits to `/index`, they
would be theoretically using different CSRF states.
- Poem only processes a single cookie, which means that we have to rely
on the ordering specified in RFC 6265 Section 5.4. This is bad for two
reasons:
1. The ordering is only a SHOULD, not a MUST.
2. Poem does, in fact, not process cookies in the correct ordering
(this is subject of another commit to fix).
Note that this commit may break applications if they share a CSRF cookie
name with other applications hosted at different paths on the same
domain.
|
Thanks for merging and for the fast release! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Without setting a Path, the UA will infer the Path from the URL path, meaning that it will take the "directory name" of a URL path (so for a response for a resource at
/index, the path would be inferred as/, and for a response for/foo/barit would be inferred to be/foo/etc.).This is problematic with CSRF for multiple reasons:
It causes a proliferation of CSRF cookies which pollute client-side storage.
It breaks CSRF when requests are sent across paths. For example, if a resource at
/foo/barcontains a form which submits to/index, they would be theoretically using different CSRF states.Poem only processes a single cookie, which means that we have to rely on the ordering specified in RFC 6265 Section 5.4. This is bad for two reasons:
Note that this commit may break applications if they share a CSRF cookie name with other applications hosted at different paths on the same domain.