Block cross-site form POSTs by default

[Rich-Harris]

Closes #72. Adds a config.kit.csrf.checkOrigin option that defaults to true

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

[changeset-bot]

🦋 Changeset detected

Latest commit: 65508b0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[Conduitry]

I'm wondering whether we should only do this for POSTs, because those are the only ones that can be done with old-fashioned form submits. The other ones would already be blocked anyway, right, unless you specifically took action to deal with the preflight CORS stuff? And if there were a way to only do this check for old-fashioned form submits and not other cross-origin fetch requests, that also might be nice, but I don't know that that's something we can reliably determine.

If this is something people are going to have to turn off before they can even start to do the legwork to allow CORS requests, that seems like it's inviting someone to then forget to close the form submit hole.

[Rich-Harris]

Good point. Changed it to just POST. We could reliably determine native form submissions if Safari and IE11 supported sec-fetch-dest, but... they don't

[Conduitry]

Could we block it only if it's a POST and the Origin doesn't match and it has a form Content-Type? If someone wanted to expose a POST endpoint for legitimate use by other servers, they would likely write it to accept JSON, so that wouldn't get in their way. And I think it still provides the same amount of protection.

[Rich-Harris]

Oh you know what? That should definitely work. D'oh

[Prinzhorn]

Could we block it only if it's a POST and the Origin doesn't match and it has a form Content-Type? If someone wanted to expose a POST endpoint for legitimate use by other servers, they would likely write it to accept JSON, so that wouldn't get in their way. And I think it still provides the same amount of protection.

Until ~6 years ago the following would've bypassed the current implementation in Chrome and Firefox:

<script>
  navigator.sendBeacon(
    'http://localhost:5173/todos',
    new Blob([JSON.stringify({ text: 'pwned' })], { type: 'application/json' })
  );
</script>

Spice in some _method and you got yourself full CSRF not just limited to POST.

sendBeacon was using no-cors mode by default. I'm not up-to-date if that's still possible in semi-recent other browsers.

[ChrisOgden]

This broke my implementation of Auth0 as an openid-connect module (rough draft but working - starbasehq/sveltekit-openid-connect#11). In order to use the POST callback from Auth0 I have to set this option to false to disable CSRF checks. Would it be possible to whitelist certain domains/urls and still have the rest of the CSRF protection?

[Conduitry]

Perhaps eventually. But for now, until we figure out what that API looks like, it should be pretty simple to disable that setting in the configuration and then re-create the adjusted check in your handle hook.

[ChrisOgden]

Perhaps eventually. But for now, until we figure out what that API looks like, it should be pretty simple to disable that setting in the configuration and then re-create the adjusted check in your handle hook.

That solution works for now. Here is how I did it:

top of hooks.js

// Where CSRF_ALLOWED is pulled from environment (process.env) and is a comma separated list of allowed hostname
// Where AUTH0_DOMAIN is endpoint from auth0 for tenant
const csrfAllowed = [`https://${AUTH0_DOMAIN}`, ...(CSRF_ALLOWED || '').split(',').filter(Boolean)]

Inside handle

export async function handle ({ event, resolve }) {
	const { forbidden: forbidCSRF, response: responseCSRF } = checkCSRF(event.request)
	if (forbidCSRF) return responseCSRF
	// ... rest of handle function
}

function at bottom of hooks.js

function checkCSRF (request) {

	const url = new URL(request.url)
	const type = request.headers.get('content-type')?.split(';')[0]
	const forbidden =
		request.method === 'POST' &&
		!_.includes([url.origin, ...csrfAllowed], request.headers.get('origin')) &&
		(type === 'application/x-www-form-urlencoded' || type === 'multipart/form-data')

	if (forbidden) {
		const response = new Response(`Cross-site ${request.method} form submissions are forbidden`, {
			status: 403
		})
		return { forbidden, response }
	} else {
		return { forbidden, response: null }
	}
}