rfc: CSRF protection

[ematipico]

Summary

CSRF - Cross-site Request forgery - protection

Links

• Rendered RFC
• Stage 2 Proposal
• Stage 1 Proposal

[florian-lefebvre]

I wonder if it would be better to make the option available under security, in case we want to add more security features (eg. honeypot) to core. So

// astro.config.mjs
export default defineConfig({
  security: {
    csrfProtection: ["origin", "cookie"]
  }
})

[natemoo-re]

@florian-lefebvre that's a good suggestion. We do try to avoid nesting too early since we don't necessarily know if we'll ever add something like a security.honeypot option, but if we already see lots of potential for expanding the APIs, adding a top-level category is a good suggestion.

[florian-lefebvre]

I think CORS could be supported in the future as well but i can't see anything else rn

[ematipico]

@lilnasy do you envision some option for the CSP support? If so, we could add a top-level security option.

[lilnasy]

Not necessarily. It's too soon to say because it's only stage 1, but it's possible a solution could just work. I only envision a flag for backwards compatibility, and that's only if the picked solution can't be retrofitted.

[natemoo-re]

I assume static support would be a non-goal here? We can only add CSRF protection to the routes that are handled by the Astro server, right?

[lilnasy]

Yes, and the threat of CSRF is only relevant to data mutations, so I would think that this is okay.

[natemoo-re]

Would love to discuss this! Obviously we should, but can we get more specific about what level of security we are able to provide and what must be left up to users? This RFC seems like a good reason to have that larger conversation.

[ematipico]

I am not good at writing, so if you have a better suggestion, I would really appreciate it

[natemoo-re]

I would expect a bit more detail here!

Here are some other potential goals to consider that may (not) be relevant:

• Automatically apply CSRF protection to all routes
• Allow users to configure CSRF protection per-route
• Enable CSRF protection by default (eventually)
• Only pages/, or API endpoints too?
• Only server, or hybrid too?

[ematipico]

I pushed two commits:

• d25271b (#879) that clarifies the goals
• e299435 (#879) that clarifies use cases

I wouldn't want to enable CSRF by default now because it could break users's applications. For the future, if we want to make it a default, I think it should require a separate discussion, outside of this RFC.

[natemoo-re]

This seems more like background than implementation. Can we automatically match the origin or does it require a user to configure the site? How would the cookie be generated? Is the cookie static or refreshed for each build?

[ematipico]

In this commit 4d37d51 (#879) I changed the shape of the configuration due to some requirements that are needed for the cookie/token strategy.

[matthewp]

What is the **known requests**? Which requests are these that it's checked?

[ematipico]

They are mentioned in this paragraph, third sentence.

[lilnasy]

I'm not sure how angular's approach works.

• From the sound of it, it seems stateful. Who writes the state, who reads the state, where does it go?
• The angular docs make mention of setting a custom header, implying they manage the client-side code that makes the request. Is astro going to offer a client-side fetch() wrapper?

A few implementation details may help communicate the approach.

[lilnasy]

It comes across as odd that the token "name" is configurable because I don't think the token was introduced as a key-value pair.

Astro will create a token that will be saved inside a cookie named Astro-csrf-token.

If "Astro-csrf-token" is the cookie name, the token is the value of that cookie, what's a token name?

[septatrix]

This will break form submissions when using curl for example or pretty much any other tool which is not a modern browser such as http clients in most programming languages. Another example would be load-testing tools. While some like k6 support setting custom headers this is cumbersome and not supported by everything.

A cookie based implementation would at least work with everything which has a sense of session. So not curl but at least the other stuff I listed.

Originally posted by @septatrix in withastro/astro#10678 (comment)

[ematipico]

@septatrix: have you read the RFC? The RFC proposes a cookie solution, it would be great if you could share your thoughts about that.

[matthewp]

@septatrix can you explain why this will break curl usage? Does curl not allow you to sit the origin header?

[septatrix]

Curl does allow setting headers but this has to be done manually, existing automation might fail and - as said above - this is not the case for everything. E.g. many load testing tools are very simplistic and do not allow setting headers.

The cookie solution would still require changes to existing curl scripts and require one additional initial request to fetch the cookie. However, cookies are automatically handled by many other tools, especially client libraries in many programming languages establish session which store cookies between requests.

[septatrix]

One might also consider adding a configuration to the origin checking of how to treat requests with missing origin headers. To my knowledge this would apply to old browsers and most of non-browser tools. This would allow tools like curl et al to keep working while omitting the security checks for the fraction of users running very old browsers

[ematipico]

We decided to reduce the scope of the protection to the origin check only, hence the proposed configuration is also reduced. Changed applied in bc87ea7 (#879)

Also, this is a call for consensus! The RFC is ready to be shipped.

[florian-lefebvre]

Is it possible to opt-out of CSRF on a route basis? For example for webhooks like stripe

[ematipico]

Is it possible to opt-out of CSRF on a route basis? For example for webhooks like stripe

No, it isn't possible at the moment. Do you have webhooks that send posts like they were forms?

[florian-lefebvre]

Not sure I understand what you mean. In the case of stripe, a webhook is a post route that accepts some data (through request.text()). Stripe docs recommend disabling cors for the webhook specifically

[ematipico]

I understand now. I think it's a legitimate request, however, It can be implemented after this RFC lands

[florian-lefebvre]

I created a RFC so we don't don't forget #923

[matthewp]

The final comment period has ended and this RFC is accepted.