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
feat: CSRF plugin #1006
feat: CSRF plugin #1006
Changes from 1 commit
482d1a7
331a5e1
a76847c
ff83523
5f75844
579eac0
00cf251
00769c2
03ca032
d4afa38
19f96a2
1759abb
669b786
c4f101f
ba41af3
95c3526
6622d4c
4d27490
a20105b
b35ed95
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
One thing we need to be very careful with when doing this parsing is that we are parsing the correct value in the first place. HTTP allows you to specify a header more than once. For the purposes of the fetch spec, the string that the browser extracts and passes to the "parse a mime type" algorithm is "all values of the header, joined with comma-space".
Also note that mime types can have "params", like
text/plain; charset=utf8
. This is close enough totext/plain
for the sake of not preflighting!So let's say you tried to send a request with
The browser would combine this to
which is MIME type with essence
text/plain
and a parameterfoo
with value, application/json
. Which does not require preflight!If the browser actually sent this as two separate headers and then for some reason our server applied "just pay attention to the last one" then we'd think this was preflighted when it really wasn't.
In practice this works out OK because the browser will actually send this as a single joined header anyway... but it's still reasonable to try to be careful anyway. http::HeaderMap says it is a multi-map! Looks like
get
returns the first value associated. I'd suggest usingget_all
and joining on,
(comma space) to be as close as possible to the spec. (This is invisible in the Apollo Server 3 code because the CSRF prevention code receives aHeaders
object that works like the browser headers object and does the comma-space join automatically.) Worth testing too!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.
https://fetch.spec.whatwg.org/#cors-safelisted-request-header claims
This intentionally does not use [extract a MIME type](https://fetch.spec.whatwg.org/#concept-header-extract-mime-type) as that algorithm is rather forgiving and servers are not expected to implement it.
The example seems to concur (and explicitly show what would happen if a client sent text/plain and application/json), so I'm tempted to:
get_all
it also claims
If mimeType is failure, then return false.
but i wouldnt mind us being more strict in that regard tbh.Edit: it does use the parse a MIME type algorithm, but not the /extract/ a MIME type algorithm
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.
should be fixed in 331a5e1 but i ll add some tests to make sure it s the 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.
@o0Ignition0o I'll look at the code in a sec, but I don't think "parse and get the essence of each value" is what I'm asking for. The fetch spec wants you to parse just one content-type — it's just that that content-type should be formed by combining all content-type headers rather than by just arbitrarily picking one of them (which is what
get()
does)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.
can you please point me to the spec? I can't seem to find the "combine all content-type headers" part (and all i can find is how to parse one mime type https://mimesniff.spec.whatwg.org/#parse-a-mime-type)
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 think I found 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.
To match the fetch spec, this needs to use precisely this "parse a MIME type" algorithm. Looks like this is the implementation in the
mime
package, which based on the filename is RFC7231.Looking at the code, I see some things that look different. For example, this mime type parser considers leading whitespace to be an error instead of just stripping it. (So if you change line 235 to
" text/plain"
it thinks it's the error case which would have been preflighted.) In practice this is probably fine: the fetch algorithm is designed to be run on the client (ie with whatever garbage headers are in the JS) but when you're parsing a header that you're reading on the server presumably you drop all the leading whitespace.But worse is that it seems to only honor spaces, not tabs. So it chokes on
text/plain;\tfoo=bar
which seems wrong. Looking at issues somebody found this and in general people are asking if it's maintained. There's a relatively recent issue about a replacement. That one is brand new and doesn't have much usage yet but at least gets tabs correct.Since I can't find a MIME parser that claims to exactly implement the fetch/whatwg parser like I could in npm, maybe the logic of "if a content-type is provided but we can't parse it, assume it's safe" isn't a good idea here, and we should go with "we only accept requests that have a content-type that we successfully parsed and they didn't contain one of the three non-preflighted types". (ie, I trust that the npm
whatwg-mimetype
has accurate error handling but don't trust that the Rust implementations might incorrectly reject a string that should parse.) If you change the handling of the error case to rejecting (unless one of the other headers is provided of course) then it's probably fine to use the popular-if-undermaintained crate.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'm going to keep a close eye on the new crate, which seems to be going in the right direction.
I agree, let's:
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.
should be fixed in 331a5e1 but i ll add some tests to make sure it s the case