fix(sandbox): cap the Go daemon's /tools/sync request body - #6480
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The other three obvious daemon body-size gaps in this area (
/read,/edit, config-update) are already covered by open PRs #6215, #6039, and #6252 — this fixes a fourth, uncovered one:ToolsSync(POST /_sandbox/tools/sync) decodes the request body with a barejson.NewDecoder(r.Body), with no size limit at all (not even the daemon's general 500MB transfer cap that the fs routes get viadecodeBody).Why it matters: a misbehaving or malicious caller can stream an unbounded body at this route and the daemon buffers it into memory while decoding, which can OOM the process — the daemon's health probe treats a single miss as dead and Studio tears the sandbox pod down mid-session.
Fix: wrap
r.Bodyinhttp.MaxBytesReadercapped at 1MB before decoding — the payload is just a URL plus a handful of headers, never a file transfer, so 1MB is generous headroom, matching the same pattern already used for/dispatch(maxDispatchBodyBytes) and theConfigUpdate/OrgFsConfigroutes in the open #6252 PR.Regression test:
TestToolsSyncRejectsOversizedBodysends a body over the cap and asserts a 400 instead of the decoder buffering it unbounded;TestToolsSyncRejectsMissingURLis an existing-behavior sanity check alongside it.How to verify:
cd packages/sandbox/daemon-go && go test ./internal/routes/... -run TestToolsSync -vLocally ran:
go build ./...,go vet ./internal/routes/...,gofmt -l(clean), and the fullgo test ./internal/routes/...package (all green). CI runs the rest.Summary by cubic
Caps the Go daemon’s ToolsSync (POST /_sandbox/tools/sync) request body at 1MB to prevent unbounded memory use. Previously the handler decoded the body without a limit; oversized bodies could be buffered into memory and OOM the process.
http.MaxBytesReader(1MB) beforejson.Decoder, aligning with caps used by dispatch/config routes.Written for commit 2050053. Summary will update on new commits.