Skip to content

Fix/413 payload too large - #60

Closed
PrathamAsrani wants to merge 2 commits into
SolidGeeks:mainfrom
PrathamAsrani:fix/413-payload-too-large
Closed

PrathamAsrani wants to merge 2 commits into
SolidGeeks:mainfrom
PrathamAsrani:fix/413-payload-too-large

Conversation

@PrathamAsrani

Copy link
Copy Markdown

Thanks for the rep.

What this changes

How to check it

The Solid checklist

  • Tests cover the change and pass in CI
  • Docs are updated (README, comments, changelog entry)
  • No secrets, keys or personal data in the diff
  • Every commit is signed off (git commit -s, see DCO)
  • New files carry the repo's SPDX license header

@0b3l1X-SolidGeeks 0b3l1X-SolidGeeks 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.

Thanks for this, and welcome! Rejecting an oversized body from its Content-Length before reading it is a good idea, and the new tests (announced size, chunked body, a small per-route limit, invalid JSON) are exactly the cases worth covering. A few things before it can go in:

1. The branch starts from an old main. Its base is 52d1695, 32 commits behind, so it conflicts on server/server.js and tests/api.test.mjs. Since then, 0.7.0 (#38) already fixed the 413 itself: the server now answers 413 with its JSON error instead of cutting the connection, and the import test checks it. Current readBody reads an oversized body to its end without keeping it, then answers 413, and cuts the connection only past DISCARD_MAX (64 MB over the limit). Your version would replace that with the older logic and lose the cap.

2. What main still lacks is your early check. Today a client that announces 2 MB and sends nothing keeps the request open until the timeout. Could you rebase on main and keep only that part? Something like this at the top of the current readBody:

if (Number(req.headers['content-length']) > limit) { req.resume(); return reject(httpError(413, 'Payload too large')); }

plus Connection: close on 413 answers, as you did. Leave the in-flight path (over / DISCARD_MAX) as it is. Answering while the client is still sending is what used to give clients a network error instead of the 413.

3. Please drop the 20 → 30 MB default, or explain why in the description and update the README table and .env.example, which both say 20. Note also that Math.max(1, parseInt(...) || 30) turns MAX_IMPORT_MB=0 into 30.

4. Housekeeping:

  • Sign off the commits (git commit -s): the DCO check is failing.
  • Fill in the PR template (what changes, how to check it).
  • Use commit messages that say what changes. "fix" won't help whoever reads the history next.
  • Add a changelog line under Unreleased.

Your tests should carry over almost as they are once rebased: the announced-size and chunked cases are the useful ones. Happy to look again as soon as you push.


Generated by Claude Code

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