Skip to content

fix: request.length overflows on Content-Length > 2GB - #1961

Merged
fengmk2 merged 2 commits into
koajs:masterfrom
tejgokani:fix/request-length-overflow
May 21, 2026
Merged

fengmk2 merged 2 commits into
koajs:masterfrom
tejgokani:fix/request-length-overflow

Conversation

@tejgokani

@tejgokani tejgokani commented May 19, 2026 •

Copy link
Copy Markdown
Contributor

Problem

ctx.request.length uses ~~len (double bitwise NOT) to parse the Content-Length header, which truncates values to a signed 32-bit integer. For Content-Length values exceeding 2,147,483,647 bytes (~2GB), the result silently wraps to incorrect values:

~~'2147483648'  → -2147483648  (sign flip)
~~'4294967296'  → 0
~~'10000000000' → 1410065408   (garbage)

This affects any middleware using ctx.request.length for upload size enforcement, rate limiting, or request body pre-allocation. In the era of large file uploads and video processing APIs, this is a realistic scenario.

Solution

Replace ~~len with Number.parseInt(len, 10):

  • Replace ~~len (32-bit truncation) with Number.parseInt(len, 10)
  • Handles arbitrarily large Content-Length values up to Number.MAX_SAFE_INTEGER
  • Preserves existing behavior for values under 2GB, including zero
  • Add comprehensive test cases for overflow boundaries and edge cases

Fixes integer overflow where ~~'2147483648' returned -2147483648 instead of the correct value. Now correctly parses large file uploads and returns undefined for unparseable values, consistent with existing patterns.

Test Coverage

Added 6 comprehensive test cases covering:

  • Zero value (edge case)
  • Content-Length > 2GB (2147483648, 10000000000) — new capability
  • Floating-point strings ('10.5') — truncation behavior preserved
  • Non-numeric strings ('invalid') — returns undefined
  • Empty string — already guarded by if check

All existing tests pass (439/439). No breaking changes.

Checklist

  • I have ensured my pull request is not behind the main or master branch of the original repository.
  • I have rebased all commits where necessary so that reviewing this pull request can be done without having to merge it first.
  • I have written a commit message that passes commitlint linting.
  • I have ensured that my code changes pass linting tests.
  • I have ensured that my code changes pass unit tests.
  • I have described my pull request and the reasons for code changes along with context if necessary.

tejgokani added 2 commits May 19, 2026 22:46
- Replace ~~len (32-bit truncation) with Number.parseInt(len, 10)
- Handles arbitrarily large Content-Length values up to Number.MAX_SAFE_INTEGER
- Preserves existing behavior for values under 2GB, including zero
- Add comprehensive test cases for overflow boundaries and edge cases

Fixes integer overflow where ~~'2147483648' returned -2147483648 instead of
the correct value. Now correctly parses large file uploads and returns
undefined for unparseable values, consistent with existing patterns.
- Replace ~~len (32-bit truncation) with Number.parseInt(len, 10)
- Handles arbitrarily large Content-Length values up to Number.MAX_SAFE_INTEGER
- Preserves existing behavior for values under 2GB, including zero
- Add comprehensive test cases for overflow boundaries and edge cases

Fixes integer overflow where ~~'2147483648' returned -2147483648 instead of
the correct value. Now correctly parses large file uploads and returns
undefined for unparseable values, consistent with existing patterns.
@codecov

codecov Bot commented May 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.90%. Comparing base (e0ba8ef) to head (6903bc8).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1961   +/-   ##
=======================================
  Coverage   99.90%   99.90%           
=======================================
  Files           9        9           
  Lines        2105     2106    +1     
=======================================
+ Hits         2103     2104    +1     
  Misses          2        2           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tejgokani

Copy link
Copy Markdown
Contributor Author

@olleolleolle
Please take a look at this PR

@olleolleolle olleolleolle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a non-maintainer, and having read this patch only, I think it looks good. I recommend its merging.

@tejgokani

Copy link
Copy Markdown
Contributor Author

Thank you so much for the approval. Could please tell me who approves for the merge, so that I may address the issue to them specifically.

@fengmk2 fengmk2 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tejgokani Thanks!

@fengmk2
fengmk2 merged commit 3f3ac48 into koajs:master May 21, 2026
6 checks passed
@tejgokani

Copy link
Copy Markdown
Contributor Author

Thank you so much to all the maintainers and contributors for the opportunity.

Regards,
tejgokani

fengmk2 added a commit to eggjs/egg that referenced this pull request May 21, 2026
Mirror the Request.length fix on the Response sibling getter to keep
the fork aligned with upstream koa. Preserves the existing `|| 0`
fallback so the behavior for non-numeric headers is unchanged.

Ref: koajs/koa#1961
killagu pushed a commit to eggjs/egg that referenced this pull request Jun 28, 2026
## Summary

- Port [koajs/koa#1961](koajs/koa#1961) into our
maintained `@eggjs/koa`.
- Add explicit radix `10` to `Number.parseInt` so `Content-Length` is
always parsed as base 10.
- Return `undefined` (instead of `NaN`) for non-numeric `Content-Length`
values, matching the documented `number | undefined` contract.

## Why

The upstream koa bug was `~~len` truncating to a signed 32-bit integer,
silently producing wrong sizes for payloads > 2GB. Our fork already
moved off `~~` to `Number.parseInt`, but two gaps remained:

1. No explicit radix — surprising parsing for leading `0`/`0x` inputs.
2. Non-numeric headers leaked `NaN` to callers instead of `undefined`.

## Test plan

- [x] `npx vitest run packages/koa/test/request/length.test.ts` — 8/8
passing
- [ ] Ported all six regression cases from upstream: zero, 2GB+, 10GB,
decimal truncation, non-numeric, empty string.

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Bug Fixes**
* Content-Length parsing now returns undefined for invalid or
non-numeric values instead of NaN, and consistently parses numeric
values using a fixed decimal radix for more predictable behavior.
* **Tests**
* Expanded length parsing tests to cover zero, very large values,
decimal-like truncation, empty and non-numeric inputs.

<!-- review_stack_entry_start -->

[![Review Change
Stack](https://storage.googleapis.com/coderabbit_public_assets/review-stack-in-coderabbit-ui.svg)](https://app.coderabbit.ai/change-stack/eggjs/egg/pull/5952?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack)

<!-- review_stack_entry_end -->
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants