roadmap icon indicating copy to clipboard operation
roadmap copied to clipboard

rfc: CSRF protection

Open ematipico opened this issue 2 years ago • 11 comments

Summary

CSRF - Cross-site Request forgery - protection

Links

ematipico avatar Apr 02 '24 15:04 ematipico

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"]
  }
})

florian-lefebvre avatar Apr 02 '24 15:04 florian-lefebvre

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

natemoo-re avatar Apr 02 '24 15:04 natemoo-re

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

florian-lefebvre avatar Apr 02 '24 15:04 florian-lefebvre

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

ematipico avatar Apr 02 '24 15:04 ematipico

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.

lilnasy avatar Apr 02 '24 16:04 lilnasy

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

ematipico avatar Apr 05 '24 13:04 ematipico

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 https://github.com/withastro/astro/issues/10678#issuecomment-2051402692

septatrix avatar Apr 12 '24 10:04 septatrix

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

ematipico avatar Apr 12 '24 10:04 ematipico

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

matthewp avatar Apr 18 '24 17:04 matthewp

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 avatar Apr 19 '24 14:04 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

septatrix avatar Apr 19 '24 14:04 septatrix

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.

ematipico avatar May 14 '24 07:05 ematipico

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

florian-lefebvre avatar May 20 '24 05:05 florian-lefebvre

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?

ematipico avatar May 20 '24 06:05 ematipico

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

florian-lefebvre avatar May 20 '24 06:05 florian-lefebvre

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

ematipico avatar May 20 '24 07:05 ematipico

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

florian-lefebvre avatar May 20 '24 13:05 florian-lefebvre

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

matthewp avatar May 21 '24 12:05 matthewp