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.
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
Add a page on CSRF #38151
base: main
Are you sure you want to change the base?
Add a page on CSRF #38151
Changes from all commits
a38d63b
d201e2b
bc46410
e2a8269
493e8aa
9459dde
2f7d82e
3dff110
82e13f2
5e01e97
7d9fb7d
162c6e3
2b4c134
5a6d295
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just shorthand? The URL is an HTTP get
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, yes you are right, this ought to be fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> 2b4c134
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might not be worth saying here but it should be said in this doc (I'm working down, so apologies if you already covered it), but as above, GET should never be used for state changing requests for exactly this reason - too easy to hack, and you can't use CRSF tokens in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I feel like we should say this but I'm not sure where to put it exactly. I mentioned it in the bit about SameSite since it's very relevant to Lax.
I struggle a bit with this. How practically is this really easier than the form method? No JS needed but so what?
And yes, at least for Django you need to avoid
GET
if you want to use CSRF tokens, and we could mention it there too, but that's really just a functional point about implementing that defense, and it's already covered in the Django docs for that (https://docs.djangoproject.com/en/5.1/ref/csrf/), so it doesn't feel like an "extra defense".Mentioning it on its own section under "defenses" seems wrong too, since it's not a defense on its own.
I'll think some more about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this. In particular the potential issues with Lax have never been explained to me before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I feel this is vague and wish I had a concrete example of how an attacker could circumvent
Lax
. I just copied this bit from the spec but don't really understand it. Don't tell anyone that though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a big vague. I think only top level navigations with GET include cookies, so you'd be mostly fine if you don't do something dumb like make state-changing requests using URL params.
Here is some reading https://portswigger.net/web-security/csrf/bypassing-samesite-restrictions#bypassing-samesite-lax-restrictions-using-get-requests
There are a few cases worth highlighting there - samesite isn't sameorigin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I.e. I don't see how the popup case is a bypass or in some way an avenue for a same-site attack unless it gets to you maybe to open a subdomain - i.e. same-origin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like it's not just GET, but is only safe methods: https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis-02#section-5.5. And yes, the other bit of the spec does say SameSite gives you good protection if you enforce the use of unsafe methods like POST. So perhaps we can use this as a place to say don't issue state-changing requests using unsafe methods, and here's a reason why.
And if you do only use unsafe methods, then SameSite is a reasonable defense, except for the samesite != sameorigin thing.
Ah, that's a helpful link, yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, - safe methods are this list:
So the methods you might use for changing something is GET. There is another note in the spect that safe method implementations should be kept idempotent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed 5a6d295 to go into some more detail on SameSite issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a "Defense summary checklist" like that one.