feat: CSRF plugin #1006

o0Ignition0o · 2022-05-10T14:52:19Z

This Pull request does a couple of things:

Add a CSRF protection plugin, that is now enabled by default.
Changes the default CORS allow_headers behavior from allow only Content-Type, apollographql-client-name and apollographql-client-version to mirror the received access-control-request-headers

Co-authored-by: Simon Sapin [email protected]

Co-authored-by: Simon Sapin <[email protected]>

netlify · 2022-05-10T14:52:26Z

✅ Deploy Preview for apollo-router-docs canceled.

Name	Link
🔨 Latest commit	`b35ed95`
🔍 Latest deploy log	https://app.netlify.com/sites/apollo-router-docs/deploys/627e35b255d9010009a25e93

glasser

I didn't expect you to implement this within hours of my incoherent "omg I just found an issue but also have to pick up my daughter at school in 7 minutes" ramblings — wow, thanks!

apollo-router-core/src/plugins/csrf.rs

apollo-router/src/configuration/mod.rs

glasser · 2022-05-11T00:36:11Z

apollo-router-core/src/plugins/csrf.rs

+
+fn content_type_requires_preflight(headers: &HeaderMap) -> bool {
+    headers
+        .get(header::CONTENT_TYPE)


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 to text/plain for the sake of not preflighting!

So let's say you tried to send a request with

content-type: text/plain; foo= content-type: application/json

The browser would combine this to

content-type: text/plain; foo=, application/json

which is MIME type with essence text/plain and a parameter foo 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 using get_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 a Headers object that works like the browser headers object and does the comma-space join automatically.) Worth testing too!

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:

use get_all

parse and get the essence of each value

check if any essence isnt part of the non preflight

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

should be fixed in 331a5e1 but i ll add some tests to make sure it s the case

@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)

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)

I think I found it!

glasser · 2022-05-11T00:37:22Z

apollo-router-core/src/plugins/csrf.rs

+        .get(header::CONTENT_TYPE)
+        .map(|content_type| {
+            if let Ok(as_str) = content_type.to_str() {
+                if let Ok(mime_type) = as_str.parse::<mime::Mime>() {


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.

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:

treat failure to turn the HeaderValue into a valid utf8 string as a condition that would have triggered preflight

trim and replace \t with ' ' before we try to parse

treat failure to parse mime type as a non sufficient condition to trigger a preflight

should be fixed in 331a5e1 but i ll add some tests to make sure it s the case

todo: add more documentation + actually test the yml + update the cors docs tomorrow with simon

garypen

I don't know enough about how CSRF prevention should be implemented to approve, but I've asked a couple of questions...

apollo-router-core/src/plugins/csrf.rs

…it comes to content type parsing

apollo-router-core/src/plugins/csrf.rs

abernix · 2022-05-13T07:59:01Z

CONTRIBUTING.md


 To build and run the documentation site locally, you'll have to install the relevant packages by doing the following
 from the root of the `router` repository:

 ```sh
 cd docs
 npm i
-npm start
+npm start -->


the three steps defined above:

cd docs npm i npm start``` are out of date; We will turn this //TODO comment into an issue, which @SimonSapin wants to tackle as a followup

I’ve submitted #1036. Maybe undo this chunk here to avoid a merge conflict?

abernix · 2022-05-13T08:01:17Z

docs/source/configuration/cors.mdx

@@ -1,10 +1,210 @@
 ---


Let's separate out documentation to another PR?

apollo-router-core/src/plugins/csrf.rs

Two breaking sections: - one that explains the CORS default behavior changed - one that explains the CSRF plugin will reject simple requests by default. One feature section that explains a new CSRF plugin now exists.

…ests+ add an failed csrf check integration test

Fixes #1039 The plugin was added in #1006 Docs are imported from Apollo Server’s own docs and adapted.

Fixes #1038 This commit updates the CORS documentation to reflect the recent [CORS and CSRF](#1006) updates.

wip

482d1a7

Co-authored-by: Simon Sapin <[email protected]>

This comment has been minimized.

Sign in to view

o0Ignition0o changed the title ~~wip~~ feat: CSRF plugin May 10, 2022

o0Ignition0o assigned o0Ignition0o and SimonSapin May 10, 2022

glasser requested changes May 11, 2022

View reviewed changes

o0Ignition0o added 3 commits May 11, 2022 18:32

wip: address review issues + add a bit of documentation.

331a5e1

todo: add more documentation + actually test the yml + update the cors docs tomorrow with simon

compliance

a76847c

remove noise from the custom-headers.router.yml file

ff83523

garypen reviewed May 11, 2022

View reviewed changes

apollo-router-core/src/plugins/csrf.rs Outdated Show resolved Hide resolved

apollo-router-core/src/plugins/csrf.rs Outdated Show resolved Hide resolved

glasser requested changes May 11, 2022

View reviewed changes

recommended -> required + get closer to the fetch specification when …

5f75844

…it comes to content type parsing

glasser approved these changes May 11, 2022

View reviewed changes

apollo-router-core/src/plugins/csrf.rs Outdated Show resolved Hide resolved

o0Ignition0o and others added 8 commits May 11, 2022 21:39

wip, add a bunch of comments

579eac0

more comments

00cf251

update documentation

00769c2

csrf tests: add helper functions for accepted/rejected requests

03ca032

csrf: add a test for MIME type parsing

d4afa38

wip, try to update docs

19f96a2

rename csrf disabled option to unsafe_disabled + docs pass 1/n

1759abb

cors documentation part 2/n

669b786

abernix reviewed May 13, 2022

View reviewed changes

o0Ignition0o added 3 commits May 13, 2022 10:26

update documentation snapshot

c4f101f

revert cors documentation, as we will polish it in a followup pr

ba41af3

Merge branch 'main' into igni/csrf

95c3526

bnjjj approved these changes May 13, 2022

View reviewed changes

apollo-router-core/src/plugins/csrf.rs Outdated Show resolved Hide resolved

o0Ignition0o added 2 commits May 13, 2022 11:12

Add three changelog entries:

6622d4c

Two breaking sections: - one that explains the CORS default behavior changed - one that explains the CSRF plugin will reject simple requests by default. One feature section that explains a new CSRF plugin now exists.

order breaking section items by impact

4d27490

o0Ignition0o marked this pull request as ready for review May 13, 2022 09:14

o0Ignition0o added 2 commits May 13, 2022 12:38

hook the csrf plugin + send preflighted requests in our integration t…

a20105b

…ests+ add an failed csrf check integration test

revert CONTRIBUTING.md as it will be followed up on by @SimonSapin

b35ed95

o0Ignition0o requested review from BrynCooke, abernix and garypen May 13, 2022 10:43

This was referenced May 13, 2022

CORS plugin: Update documentation #1038

Closed

CSRF plugin: Update documentation #1039

Closed

Geal approved these changes May 13, 2022

View reviewed changes

o0Ignition0o merged commit 253e2f0 into main May 13, 2022

o0Ignition0o deleted the igni/csrf branch May 13, 2022 13:07

Geal added this to the v0.9.0 milestone May 13, 2022

o0Ignition0o mentioned this pull request May 16, 2022

error in local setup #1023

Closed

SimonSapin mentioned this pull request May 16, 2022

Add docs for the CSRF prevention plugin #1060

Merged

o0Ignition0o mentioned this pull request May 17, 2022

Document the new CORS behavior #1044

Merged

SimonSapin added a commit that referenced this pull request May 17, 2022

Add docs for the CSRF prevention plugin (#1060)

c6dce05

Fixes #1039 The plugin was added in #1006 Docs are imported from Apollo Server’s own docs and adapted.

o0Ignition0o added a commit that referenced this pull request May 19, 2022

Document the new CORS behavior (#1044)

0684d1f

Fixes #1038 This commit updates the CORS documentation to reflect the recent [CORS and CSRF](#1006) updates.

BrynCooke mentioned this pull request May 20, 2022

release: v0.9.2 #1115

Merged

abernix mentioned this pull request Sep 21, 2022

reintroduce health liveliness check #1861

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: CSRF plugin #1006

feat: CSRF plugin #1006

o0Ignition0o commented May 10, 2022 •

edited

Loading

netlify bot commented May 10, 2022 •

edited

Loading

This comment has been minimized.

glasser left a comment

glasser May 11, 2022

o0Ignition0o May 11, 2022 •

edited

Loading

o0Ignition0o May 11, 2022

glasser May 11, 2022

o0Ignition0o May 11, 2022 •

edited

Loading

o0Ignition0o May 11, 2022

glasser May 11, 2022

o0Ignition0o May 11, 2022

o0Ignition0o May 11, 2022

garypen left a comment

abernix May 13, 2022

o0Ignition0o May 13, 2022

SimonSapin May 13, 2022

abernix May 13, 2022

feat: CSRF plugin #1006

feat: CSRF plugin #1006

Conversation

o0Ignition0o commented May 10, 2022 • edited Loading

netlify bot commented May 10, 2022 • edited Loading

✅ Deploy Preview for apollo-router-docs canceled.

This comment has been minimized.

glasser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

o0Ignition0o May 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

o0Ignition0o May 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garypen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

o0Ignition0o commented May 10, 2022 •

edited

Loading

netlify bot commented May 10, 2022 •

edited

Loading

o0Ignition0o May 11, 2022 •

edited

Loading

o0Ignition0o May 11, 2022 •

edited

Loading