ENV BODY_SIZE_LIMIT=0 throws error "Content-length of 337 exceeds limit of 0 bytes."

[eunukasiko]

Describe the bug

Since this change https://github.com/sveltejs/kit/pull/11289/files#diff-4fd596e614892c76109d32e8f58ad17b3e874414d798603c12f63669db7399d3R40
we cannot set the limit to unlimited.

The docs say to set it to 0 https://kit.svelte.dev/docs/adapter-node#environment-variables-bodysizelimit
but 0 is not undefined and as such it's throwing the error.

Reproduction

1. Set BODY_SIZE_LIMIT=0
2. await request_event.request.text();

Logs

No response

System Info

System:
    OS: macOS 13.6.3
    CPU: (12) x64 Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
    Memory: 35.71 MB / 32.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.10.0 - ~/n/bin/node
    npm: 10.2.3 - ~/n/bin/npm
  Browsers:
    Chrome: 120.0.6099.109
    Safari: 17.2
  npmPackages:
    @sveltejs/adapter-node: ^2.0.0 => 2.0.0 
    @sveltejs/kit: ^2.0.2 => 2.0.2 
    @sveltejs/vite-plugin-svelte: ^3.0.0 => 3.0.1 
    svelte: ^4.2.8 => 4.2.8 
    vite: ^5.0.0 => 5.0.9

Severity

blocking an upgrade

Additional Information

No response

[Rykuno]

I've also experienced this bug. We have a need to upload images to the backend and after upgrading to sveltekit v2; settings the BODY_SIZE_LIMIT=0 does not disable the check, but rather tries to enforce a 0kb body limit.

[omaralmgerbie]

Are there any workarounds to fix this issue or do I have to set the BODY_SIZE_LIMIT to a large value to ignore this issue?

[Rykuno]

Are there any workarounds to fix this issue or do I have to set the BODY_SIZE_LIMIT to a large value to ignore this issue?

Looking at the code, pretty much. Just set it to an arbitrarily high value and continue to let your custom check in handle to the work.

[omaralmgerbie]

BODY_SIZE_LIMIT

its strange that even when I set BODY_SIZE_LIMIT= to a very large number Its still sees it as 0

[Conduitry]

There appear to be two ways to address this, and I don't have a strong opinion about which is better. One is to update the getRequest() function in packages/kit/src/exports/node/index.js to interpret a bodySizeLimit of 0 to mean no limit. And the other is to update the Node adapter adapter so that is passes bodySizeLimit: undefined again instead of bodySizeLimit: 0 when run with BODY_SIZE_LIMIT=0.

@benmccann Do you have thoughts about that? I'm not sure to what extent @sveltejs/kit/node is considered a public API. Other official adapters do use the getRequest() function, but it looks like only the Node one passes a bodySizeLimit value.

[benmccann]

While BODY_SIZE_LIMIT=0 seems pretty unlikely I guess someone could make some weird server that they only want to handle GET/POST requests without bodies. undefined seems more appropriate here as 0 does have a defined meaning and is actually more sensible that someone may use it than BODY_SIZE_LIMIT=1.

[benmccann]

If it's easier to make it an int than undefined for some reason we could also use BODY_SIZE_LIMIT=-1 to indicate disabled

[Conduitry]

Untested, but I think just a

diff --git a/packages/adapter-node/src/handler.js b/packages/adapter-node/src/handler.js
index dcb478f5e..4812ec9ff 100644
--- a/packages/adapter-node/src/handler.js
+++ b/packages/adapter-node/src/handler.js
@@ -19,7 +19,7 @@ const address_header = env('ADDRESS_HEADER', '').toLowerCase();
 const protocol_header = env('PROTOCOL_HEADER', '').toLowerCase();
 const host_header = env('HOST_HEADER', 'host').toLowerCase();
 const port_header = env('PORT_HEADER', '').toLowerCase();
-const body_size_limit = parseInt(env('BODY_SIZE_LIMIT', '524288'));
+const body_size_limit = parseInt(env('BODY_SIZE_LIMIT', '524288')) || undefined;
 
 const dir = path.dirname(fileURLToPath(import.meta.url));
 

would suffice then.

[Conduitry]

Oh, unless what you were proposing was that someone run the adapted app with BODY_SIZE_LIMIT=undefined?

I think it makes sense to keep the same runtime API for adapted apps as we had before - we arguably could change it now because this particular feature was already not working since 2.0, but it seems nicer to fix it and return to the old API.

[benmccann]

It looks like it currently disables it if you set any non-numeric value like BODY_SIZE_LIMIT=disabled. I'm personally of the mind that we just update the docs and migration guide recommending that and say it's a breaking change in 2.0. While no one is asking to use BODY_SIZE_LIMIT=0 it seems like it should be just as valid as any other number

[Conduitry]

Hm. I think BODY_SIZE_LIMIT=disabled works because we parseInt and then end up passing bodySizeLimit: NaN, and then content_length > NaN is false for all numbers. That feels brittle to me, though. If we ever ended up rewriting getRequest so that it checked content_length <= body_size_limit instead, that would break again.

[benmccann]

Yeah, probably better to have the adapter explicitly convert from NaN to undefined in that case

[Rich-Harris]

If we're making breaking changes then I'd sooner we use parseFloat instead of parseInt so that we can do all these...

BODY_SIZE_LIMIT=Infinity
BODY_SIZE_LIMIT=0x80000
BODY_SIZE_LIMIT=1e5

...and error on anything that results in NaN.

That seems like the only sensible alternative to reinstating the status quo ante, to me

[Conduitry]

I may be biased as the author of #11574, but I think that for now what would be best is to merge that in so that we have a version that removes the unintended breaking change between v1 and v2. And then after that we can decide whether we want to switch the env var handling, when we would want to release that breaking change, what we might want to bundle it with, etc.

[benmccann]

BODY_SIZE_LIMIT=Infinity is nice, but BODY_SIZE_LIMIT=0x800 seems like a very strange API. I wonder if it'd be better to just support Infinity vs using parseFloat if we want to do that

Does it really matter if a breaking change was intended or not? I think what matters is that it's documented

[Rich-Harris]

I'm inclined to agree with @Conduitry — let's just fix the regression for now so that people's apps work again. We can then figure out what breaking changes we actually want at leisure.

Not sure what's so strange about allowing arbitrary number expressions — using hex or (especially) exponential notation in this context seems reasonable to me