Skip to content

[SDK-732] Fix --next uploads over 10 MB (#14) and bump axios to 1.20.0 (#23) - #33

Open
devtools-agent[bot] wants to merge 6 commits into
masterfrom
sdk-732-fix-14-23
Open

devtools-agent[bot] wants to merge 6 commits into
masterfrom
sdk-732-fix-14-23

Conversation

@devtools-agent

@devtools-agent devtools-agent Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Linear: SDK-732

Fixes #14. Fixes #23.

What changed

  • Request body larger than maxBodyLength limit #14, --next uploads over 10 MB (src/sourcemaps/signed-url-uploader.js): the signed-URL PUT now passes maxBodyLength: Infinity and maxContentLength: Infinity, the same settings RollbarAPI already uses in src/common/rollbar-api.js. (maxBodyLength is the one that matters for the upload; maxContentLength caps the response and is only there to match.) Before this, the bare axios.put fell back to axios's default limit.
  • Axios Cross-Site Request Forgery Vulnerability #23, axios advisory (package.json): axios 1.15.0 → 1.20.0, still pinned to an exact version.
  • AGENTS.md, plus CLAUDE.md as a symlink to it. It holds one rule, from review: don't change the version in package.json in a PR that makes other changes; version bumps go in their own PR. So this PR no longer bumps the version. Users only get these fixes once a release is published, because 0.2.1 on npm still ships axios ^0.24.0, so the 0.2.2 bump needs its own PR.
  • Test (test/sourcemaps/signed-url-uploader.test.js): the new test PUTs an 11 MB zip to a local HTTP server. It checks that the server receives every byte and that maxBodyLength is Infinity. The existing upload test left its axios.put stub in place for later tests. It now restores it in a finally, so the stub doesn't leak even if its assertion fails.

Why

  • Request body larger than maxBodyLength limit #14: I reproduced it on the 0.2.1 release (axios 0.24.0). An 11 MB PUT fails with Request body larger than maxBodyLength limit, and the server receives 0 bytes. It doesn't reproduce on master (axios 1.15.0) or with 1.20.0, because axios v1 changed the default limit to -1, meaning unlimited. Setting the option explicitly means the upload size no longer depends on an axios default.
  • Axios Cross-Site Request Forgery Vulnerability #23: GHSA-wf5p-g6vw-rhxx (fixed in 0.28.0) was already cleared on master by the axios v1 upgrade (Upgrade Axios to v1 #25), but it is still present in the released 0.2.1. axios 1.15.0 still has advisories fixed in 1.15.1, 1.15.2, 1.16.0 and 1.18.0. With 1.20.0, npm audit --omit=dev reports no axios advisories.

Validation

  • npm test: 37 passing, 1 pending (a skip that was already there). Before this change: 36 passing.
  • npm run lint: clean.
  • The existing upload test was made to fail on purpose. The 11 MB test after it still passed, which shows the stub is restored.
  • With the source change reverted, the new test fails.
  • A manual repro script (11 MB PUT to a local server) fails on axios 0.24.0 and succeeds on this branch.
  • Tested on Node 20.19.2 / npm 9.2.0 only.

Overlap with #31 (SDK-731)

#31 also bumps axios to 1.20.0 and rewrites signed-url-uploader.test.js. Whichever PR merges second will have small conflicts in package.json (the adjacent adm-zip line) and in that test file. npm audit still reports one high-severity adm-zip advisory, which #31 fixes. It's out of scope here.

🤖 Generated with Claude Code

AI Agent and others added 3 commits September 29, 2026 20:54
The `--next` upload PUTs the zip with a bare axios.put, so it inherited
axios's default body limit. On axios 0.x (the 0.2.1 release) that is
10 MB, and larger bundles failed with "Request body larger than
maxBodyLength limit". Set maxBodyLength/maxContentLength to Infinity,
as RollbarAPI already does, so the size doesn't depend on axios
defaults.

Also restore the axios.put stub the existing upload test leaked.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1.15.0 already clears GHSA-wf5p-g6vw-rhxx from #23, but still has open
advisories fixed in 1.15.1, 1.15.2, 1.16.0 and 1.18.0. 1.20.0 is the
current release; npm audit reports no axios advisories.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@linear-code

linear-code Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

SDK-732

SDK-731

@rollbar-circleci-machine

Copy link
Copy Markdown

AI Agent Review LGTM (openai, openai-astra)

LGTM. No blocking findings were found.

LGTM — no findings.

Fix: src/sourcemaps/signed-url-uploader.js:66-68 adds maxContentLength: Infinity and maxBodyLength: Infinity to the --next signed-URL axios.put. The main API client already does exactly this (src/common/rollbar-api.js:29-31), so the two upload paths now behave the same. The signed URL comes from the Rollbar API response (src/sourcemaps/command.js:74), so leaving the size limit to the server is reasonable.

Tests: The new test test/sourcemaps/signed-url-uploader.test.js:77-99 starts a real local HTTP server, replaces zipFiles with an 11 MB buffer and goes through the real upload() path. It checks both the option passed to axios and the number of bytes the server received. received is updated before the server answers (lines 80-81), so when axios.put resolves the count is final. The test runs inside the .upload() block, whose beforeEach sets global.output, which the catch at signed-url-uploader.js:77 needs. The added stub.restore() at line 74 is required; without it, sinon.spy(axios, 'put') at line 88 would fail on an already-wrapped method. One small note: restore() runs after the expect at line 73, so if that assertion fails, the stub leaks into the next test.

Dependency: axios goes from 1.15.0 to 1.20.0 (package.json:14), still pinned to an exact version. The code uses no new axios API.

Not verified: The checkout has no node_modules or lockfile, so I could not check whether axios 1.15.0's http adapter already lifted follow-redirects' 10 MB default. So I can't confirm the new test fails on the base code. The explicit option is harmless either way. I also did not check that axios 1.20.0 exists on npm. I have not run the tests.

Existing problems outside this diff (not findings):

  • .github/workflows/node.js.yml:35 passes ${{ matrix.node-version }}, but the matrix defines node (lines 19-27). The Node 18/20/22/24 matrix is probably never applied, so every job runs on the runner's default Node.
  • src/sourcemaps/signed-url-uploader.js:5 creates one AdmZip instance for the whole module. Entries pile up across SignedUrlUploader instances and zipFiles() calls within one process.

Comment thread package.json Outdated
@rollbar-circleci-machine

Copy link
Copy Markdown

AI Agent Review LGTM (openai, openai-astra)

LGTM. No blocking findings were found.

LGTM, no findings.

  • Fix (src/sourcemaps/signed-url-uploader.js:66-68): The --next path (src/sourcemaps/command.js:74) now sends maxContentLength: Infinity and maxBodyLength: Infinity on the signed-URL axios.put. That matches what src/common/rollbar-api.js:29-31 already does for the other upload paths. One small point: maxContentLength limits the response size, not the upload size. So the new comment ("Don't cap the zip size client side") really only describes maxBodyLength. The extra setting does no harm.
  • Test (test/sourcemaps/signed-url-uploader.test.js:77-99): The test sends a real 11 MB body to a local http server and checks both the config passed to axios and the number of bytes the server received. Because upload() catches its own errors (signed-url-uploader.js:76-78), the received === size check is what would expose a failed upload. The new stub.restore() at line 74 is required, or the later sinon.spy(axios, 'put') would try to wrap an already-wrapped function. One minor gap: that restore only runs if the first test passes. A sinon.restore() in afterEach would stop one failure from causing the next test to fail too.
  • Couldn't check here: The checkout has no lockfile and no node_modules. So I can't confirm how axios 1.20.0's http adapter handles the default maxBodyLength (for example, whether it already passes Infinity to follow-redirects). That means I can't say whether the byte-count check would fail without the fix. The spy check on maxBodyLength does pin down the new setting. I also can't confirm from here that axios 1.20.0 is published or what else changed between 1.15.0 and 1.20.0. Nothing in this diff shows that tests were run.
  • Existing issues outside the changed lines (not findings): signed-url-uploader.js:5 creates one module-level AdmZip instance, so entries would pile up if zipFiles() ran more than once in the same process. signed-url-uploader.js:12 sets zipBuffer to the Buffer constructor as a placeholder. In .github/workflows/node.js.yml:35, the step reads matrix.node-version, but the matrix defines node (lines 19-27), so the Node version matrix probably isn't applied.

AI Agent and others added 3 commits September 30, 2026 05:33
Version bumps go in their own PR. This reverts commit 06f7a4f.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
maxContentLength caps the response, not the upload, so say what each option
does. Restore the axios.put stub in a finally so a failed assertion doesn't
leak it into the next test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rollbar-circleci-machine

Copy link
Copy Markdown

AI Agent Review LGTM (openai, openai-astra)

LGTM. No blocking findings were found.

LGTM: no issues in the changed lines.

  • Fix (Request body larger than maxBodyLength limit #14): src/sourcemaps/signed-url-uploader.js:66-69 passes maxContentLength: Infinity and maxBodyLength: Infinity to the signed-URL axios.put. These are the same limits RollbarAPI already sets on its axios instance (src/common/rollbar-api.js:29-31), so the --next path (src/sourcemaps/command.js:61-75) now behaves like the regular upload path. The new comment is accurate.
  • Tests: The first .upload() test used to leave its axios.put stub in place. The new try/finally restore (test/sourcemaps/signed-url-uploader.test.js:72-77) is required, because otherwise sinon.spy(axios, 'put') at line 91 would fail with an already-wrapped error. The new 11 MB test sends to a real local HTTP server, checks the byte count the server received, and checks the config passed to axios.put. Any upload failure is swallowed by upload() (signed-url-uploader.js:77-79) and leaves received === 0, so the test fails clearly. Whether the 11 MB round trip alone would have failed before the fix depends on axios's default maxBodyLength handling. I can't check that here because there is no node_modules or lockfile. The args[2].maxBodyLength assertion pins the config either way. I have not run the tests, and this diff doesn't show them passing.
  • Dependency (Axios Cross-Site Request Forgery Vulnerability #23): axios is pinned to 1.20.0 (package.json:14). There is no lockfile, so the transitive tree and the behaviour of that release can't be checked in this review.
  • Docs: AGENTS.md adds a rule to keep version bumps in their own PR, and this PR follows it (package.json:3 is unchanged at 0.2.1). CLAUDE.md is a symlink to AGENTS.md (reading it returns the AGENTS.md content). This is outside the ticket's scope but harmless.

Existing problems outside the diff (not findings, noted for follow-up):

  • .github/workflows/node.js.yml:17-35 defines the matrix key node but the step passes ${{ matrix.node-version }}, which doesn't exist. So setup-node never gets a version and the 18/20/22/24/latest matrix probably isn't testing distinct Node versions.
  • src/sourcemaps/signed-url-uploader.js:5 creates one module-level AdmZip instance that every SignedUrlUploader shares. Entries pile up across zipFiles() calls in the same process. This only matters for tests or programmatic reuse, not for a single CLI run.

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.

Axios Cross-Site Request Forgery Vulnerability Request body larger than maxBodyLength limit

2 participants