Limit size of post requests

[benmccann]

Describe the problem

I believe a malicious user could bring down the server by issuing a very large post request. I think requests get buffered into memory, so you could exhaust the server memory.

Describe the proposed solution

Next.js has a default 1mb limit: https://nextjs.org/docs/api-routes/request-helpers#custom-config

Express has a default 100kb limit: https://github.com/expressjs/body-parser#limit-3

Play Framework has an in memory limit as well as a disk limit:
https://www.playframework.com/documentation/2.8.x/JavaBodyParsers

I'm not sure if a server-wide limit is enough or if we also need to be able to set it on a per-endpoint basis or some other granularity.

It should be configurable

Alternatives considered

No response

Importance

nice to have

Additional Information

No response

[Rich-Harris]

What would that mean beyond doing this?

export function handle({ event, resolve }) {
  const length = +(event.request.headers.get('content-length') ?? '0');
  if (length > MAX_SIZE) {
    throw error(413);
  }

  return resolve(event);
}

Could also reject POST requests without a content-length, but you get the idea. (I'm assuming — haven't tested this — that it's not possible for the requester to lie about the content-length, i.e. declare a short content-length but then have a huge payload. I assume request.formData() or request.body.getReader() would blow up in that case.)

One wrinkle is that if you wanted to do this on a per-route basis, we would need to change the proposed actions API:

export const actions = {
  /** @type {import('./$types').Action} */
-  uploadPhoto: ({ fields, files }) => {
+  uploadPhoto: ({ request }) => {
+    const length = +event.request.headers.get('content-length');
+    if (length > MAX_SIZE) throw error(413);
+    const { fields, files } = await formData(request);
    const title = fields.get('title');
    const image = files.get('image');

    // ...
  }
}

Perhaps that's a good thing though. It would mean we could specify handleFile behaviour per-action, if necessary, and maybe is a better way to provide type safety than generic arguments to Action. In the common case where no files are involved, a developer would just do await request.formData().

[Mlocik97]

I find it weird to solve this on framework level... anyway if we are going more far, we actually don't need to deny bigger request, than do streaming, and if there is too many request from other users to handle, we could send header (timeout). that will make client wait for specific amount of time, until it resume in sending more data. And if we go even more far, we can do load balancing on level of endpoints and users, somewhat like what K8s does, but not with making more instancies of server and distributing request between them, but instead distributing request between endpoints in "streaming" way. Still this doesn't actually solve memory problem, but it solves unresponsivity of server, as if in case of some specific endpoints taking too much memory, we could terminate instance of it and send the "timeout" or some error to client. Anyway those stuff are more typically done on level of proxy, runtime, container or systems build on top of containers like K8s.

[benmccann]

@Mlocik97 my concern here isn't too much usage, but malicious users. If users discover they can make a 10GB post request to crash any SvelteKit server then that could be abused

@Rich-Harris yeah, it's possible you could solve this in handle assuming content-length is always present and can't be forged with a low value to cause issues (I don't know the answers to those questions). But I feel that at the very least we'd want it in the default template rather than having apps be vulnerable by default

[Rich-Harris]

Did some tests and confirmed that a) content-length is always present on non-malicious POST requests and b) if the payload exceeds content-length, it will be truncated (and if the body is still being transferred once enough of the stream has been consumed, the request will be terminated). So I'm satisfied that a simple content-length check is sufficient.

Which means that other than the addendum to the forms proposal, there's no breaking change required here, so I'll remove that label

[mc-0bit]

Simply checking the Content-Length header might not be enough.

1. Content-Length may not be present on a POST request. The latest rfc states that a POST request should provide Content-Length.

   A user agent SHOULD send Content-Length in a request when the method defines a meaning for enclosed content and it is not sending Transfer-Encoding.
   rfc9110

   From my understanding this does not mean that a request must provide Content-Length or Transfer-Encoding. (Transfer-Encoding is part of HTTP/1.1 and only applies to HTTP/1.1)

   But on the other hand it seems as for HTTP/1.1 it is a must for a client to send either Content-Length or Transfer-Encoding.

   A user agent that sends a request that contains a message body MUST send either a valid Content-Length header field or use the chunked transfer coding. rfc9112.

   I might be missing something but it feels like these two statements conflict each other so feel free to make what you want out of this. A request with Transfer-Encoding is a valid request and would be blocked if only the existence/value of Content-Length is checked. See 3. for more info.

2. You might not want to trust the Content-Length header since it seems like it can provide a false value.

   A request or response that includes message content can include a content-length header field. A request or response is also malformed if the value of a content-length header field does not equal the sum of the DATA frame payload lengths that form the content, unless the message is defined as having no content. rfc9113

   This does not seem to be an issue considering @Rich-Harris comment.

3. A server MAY reject a request that contains both Content-Length and Transfer-Encoding or process such a request in accordance with the Transfer-Encoding alone. Regardless, the server MUST close the connection after responding to such a request to avoid the potential attacks. rfc9112

   and

   If a message is received with both a Transfer-Encoding and a Content-Length header field, the Transfer-Encoding overrides the Content-Length. Such a message might indicate an attempt to perform request smuggling or response splitting and ought to be handled as an error. An intermediary that chooses to forward the message MUST first remove the received Content-Length field and process the Transfer-Encoding (as described below) prior to forwarding the message downstream. rfc9112

   This means that Transfer-Encoding takes precedence over Content-Length if the request is not rejected and causes Content-Length to be removed. A simple Content-Length check can be an issue if SvelteKit runs behind a proxy that might remove the Content-Length header. This could lead to a desync between the proxy and sveltekit where the former thinks of the request as valid while the latter does not.

It is important to know that a server is allowed to reject requests without a Content-Length header and a message body. See below.

A server MAY reject a request that contains a message body but not a Content-Length by responding with 411 (Length Required). rfc9112

Unless a transfer coding other than chunked has been applied, a client that sends a request containing a message body SHOULD use a valid Content-Length header field if the message body length is known in advance, rather than the chunked transfer coding, since some existing services respond to chunked with a 411 (Length Required) status code even though they understand the chunked transfer coding. This is typically because such services are implemented via a gateway that requires a content length in advance of being called, and the server is unable or unwilling to buffer the entire request before processing. rfc9112

I guess all of this boils down to what features are wanted in SvelteKit. If you choose to offer a way to limit the size of POST requests, then my suggestion is a server wide request size limit with a sane default that is configurable alongside the ability to adjust this limit depending on the endpoint.

As it currently stands with @Rich-Harris's proposal one would have to manually add some code in every POST endpoint to protect said endpoint. This just does not go well with me. This would probably make generating api documentation that includes the request size limit of each endpoint harder.

tldr:

1. Should SvelteKit support HTTP/1.1's Transfer-Encoding?
2. I believe that there needs to be a better way handling request size limits.

[Mlocik97]

Can't we have per endpoint something like:

export const POST = {
    event: (event) => {
         // logic
    },
    config: (config) => { ...config, limit: 1024 } // size in kb, 1024 means 1MB limit for upload size
}

I know it's ugly (as API), I was more thinking about approach, aka being able to configure limit right in HTTP methods (or resp. actions in case of +page.server file)

[Rich-Harris]

Browser behaviour is a subset of what RFC 9110 allows - I'm pretty sure all browsers will always include content-length with all POST requests (at least, all form submissions). If that's not the case I would be grateful for a repro

[mc-0bit]

I too believe that browsers always include Content-Length, but what about other clients that might behave diffrently? I doubt that there are many tools that still utilize Transfer-Encoding. IMO not supporting Transfer-Encoding is perfectly fine as long as it is noted somewhere.

[benmccann]

If we're worried about malicious users, then considering browser behavior alone is not sufficient. A malicious user could use any client they wish including one that is not spec compliant.

[Rich-Harris]

If an app chooses to accept a request with transfer-encoding, then they would be responsible for terminating the request if it exceeded some threshold. It's not possible for SvelteKit to intercede here without consuming the request body (which would prevent the app from doing so). So any default behaviour that prevents excessive requests would presumably have to prevent these requests altogether.

considering browser behavior alone is not sufficient. A malicious user could use any client they wish including one that is not spec compliant

If the request size exceeds the stated content-length, the request gets terminated automatically. Unless I'm mistaken, this isn't something we need to worry about at a framework level (as long as requests with excess content-length are automatically blocked).

[benmccann]

I was worrying what might happen if a malicious client sent no content-length, but it looks like that's fine:

kit/packages/kit/src/exports/node/index.js

Line 15 in 1418c94

if (isNaN(length) && h['transfer-encoding'] == null) {

[benmccann]

Actually, we shouldn't be erroring on undefined content-length because that's how you stream content with HTTP/2. But then we have an issue that a malicious client could simply leave off the content-length. We could provide our own Request that checks the body size when you buffer by calling .json(), .text(), etc. That would have to be provided by the framework and not by the user. The issue is that I'm not quite sure how we'd handle more advanced cases like a higher limit for certain paths

If we include this in the framework, for the option name, I'd propose bodySizeLimit with a default limit of 512k

If we want the user to do this they basically have to disallow undefined content-length except for whitelisted URLs which they know will be streaming. That might end up being the cleanest solution given that we can't have per-endpoint options. I really do wish we could do something like export const bodySizeLimit = '1mb' or export const bodySizeLimit = null for an endpoint, but that's hard with mutiple handlers in a single file. This feels like something that should probably at least be in the default template or highlighted in a security section in the docs along with things like csrf, csp, etc.

For reference, here's the Next.js implementation: https://github.com/vercel/next.js/blob/6f352357fb432b954930d6aeae890b5e0fd2e6aa/packages/next/server/api-utils/node.ts#L141

[Rich-Harris]

We could provide our own Request that checks the body size when you buffer by calling .json(), .text(), etc

surely the purpose is to not buffer excessive requests in the first place?

The issue is that I'm not quite sure how we'd handle more advanced cases like a higher limit for certain paths

same way we handle CSRF — we have a sensible default, and if you want custom handling, you can easily implement it yourself in handle or in individual endpoints

[benmccann]

surely the purpose is to not buffer excessive requests in the first place?

I was thinking that if you call these methods we know you're not streaming, which means content-length should be defined, so we can check it without forcing the content-length to be defined in when you're consuming the body in a streaming fashion

same way we handle CSRF — we have a sensible default, and if you want custom handling, you can easily implement it yourself in handle or in individual endpoints

Okay, so streaming will be disabled by default. Sounds like a reasonable enough solution for now. I think longer-term we might be able to use the other proposed solution, but I'm not going to be able to implement that before tomorrow

[benmccann]

Hmmm, well I probably should have realized this earlier, but it's the adapter that calls getRequest and all the serverless platforms already have size limits for request bodies. So maybe this makes more sense as an option to adapter-node? The difficulty with that is that it'd only get evaluated in prod and not dev. But perhaps that's not the worst since it'd align with all the other adapters. We should probably have a better answer for that longer-term, but that's a much bigger question

[repsac-by]

As a workaround.
You can interrupt the request in something like this:

/**
 * @param {number} length
 */
function limitStream(length) {
	let size = 0;
	return new TransformStream({
		transform(chunk, controller) {
			size += chunk.byteLength;
			if (size > length) {
				controller.error(new Error(`Received body size exceeded allowed: ${length} bytes`));
				controller.terminate();
				return;
			}
			controller.enqueue(chunk);
		}
	});
}

/**
 * @param {Request} request
 * @param {number} body_size_limit
 */
export function limit(request, body_size_limit) {
	const content_length = Number(request.headers.get('content-length'));

	if (content_length && content_length > body_size_limit) {
		throw new Error(
			`Received content-length of ${content_length}, but only accept up to ${body_size_limit} bytes.`
		);
	}

	return new Request(request.url, {
		method: request.method,
		headers: request.headers,
		body: request.body?.pipeThrough(limitStream(body_size_limit)),
	});
}

/** @type {import('./$types').Actions} */
export  const actions = {
	async default({ request }) {
		const data = await limit(request, 512 * 1024).formData();

		//...
	}
}