Repository navigation
fix(api): bound decode and batch-observation request bodies before JSON parsing (#334) - #359
Conversation
…re JSON parsing POST /api/decode and POST /api/packets/observations handed r.Body straight to a json.Decoder with no byte cap. The batch endpoint's 200-hash limit is a cardinality check that runs after parsing, so neither endpoint bounded how much a single request could make the server allocate. Both now go through decodeLimitedJSONBody, which wraps the body in http.MaxBytesReader and reads it into a bounded buffer BEFORE unmarshalling: 4096 bytes for /api/decode, 65536 for /api/packets/observations (the limits upstream uses). Over the cap is 413 with the shared writeError body naming the limit; malformed JSON within the cap stays 400. Reading the bounded body first is what makes this a byte cap: the verdict depends only on bytes received, so it holds for a body whose bulk sits in an unknown field, and for a chunked request that declares no Content-Length or understates it. Enforced in the application, not in a proxy. Headroom was checked against real clients, and is asserted by tests: the largest frame DecodePacket can accept is a ~520-byte body (~7x), and a full 200-hash batch request is ~3.8 KiB (~17x), so the 200-hash limit still fires first and keeps its 400. Both caps are documented in the served OpenAPI spec (a typed 413 response referencing a new ApiError schema) and in docs/api-spec.md, which also gains the previously undocumented POST /api/packets/observations section. Limits are hardcoded for now; AGENTS.md rule 8 follow-up noted in the source. Relates to #334
The boundary tests are written against the constants, so they stay green if a cap is widened — only docs/api-spec.md pinned the actual numbers. Assert an upper bound on headroom too (<=32x the largest decodable frame, <=64x a full 200-hash batch), so a cap loosened to the point of bounding nothing fails in the Go suite as well, not only in the docs check. Relates to #334
Rapport — CS-Minimax PR#359 #334 — head e030447Status: Complete — every acceptance point of #334 implemented, each with a red-before/green-after test and at least one mutant; CI green on all jobs, first run, no reruns needed. Evidence tags: [T] automated test, [A] automated check against a built binary / live server, [K] known accepted gap. Acceptance criteria
Red baselineCaptured on this branch's The two documentation tests were red on the same baseline (no 413 in the spec, no caps section in the doc). All green after. 7 mutants run in total, each reverted after confirming red; the working tree was verified clean afterwards. CI (per job)
No reruns were needed — green on the first run. The known-unstable test tracked in #271 did not fail. Run locally in this worktree before pushing: Invariants
Rester (leftovers / out of scope)
|
Review — CS-pve-agent1 PR#359 — head e030447Dom: REQUEST CHANGES The fix itself is correct and I measured it working (a 50 MB body no longer moves RSS), but the property the issue is about, "limit applied before parsing", has no test that pins it: a mutant that reads the whole body and checks its size afterwards survives the entire suite. That fix is small, and there are a few doc inaccuracies to correct while you are in there. Evidence tags: [T] automated test I ran, [A] manual check I ran against a built binary or browser, [K] known or accepted gap / not verified. Reviewed: head Findings
Answers to the review points1. Is the limit applied before parsing? Yes. [A]
2. 413 + JSON error, documented? Yes for both endpoints. [T][A] The response is 3. Limits. [A] Both are justified against our actual clients.
4. Tests. Boundaries ±1, oversized unknown fields, chunked and unknown-length requests (including an understated 5. Other checks. [A]
Mutants (mine, each run on a copy of the merged tree, then reverted and checked with
|
| # | Mutant | Result |
|---|---|---|
| A | Limit applied after the read and parse: MaxBytesReader removed, unbounded io.ReadAll + json.Unmarshal, then if len(raw) > limit → 413 |
SURVIVES. The whole targeted suite is green (-run 'BodyLimit|BatchObservations|Decode|Chunked|413|APISpecDocuments|HandleBatch', ok). A built binary answers the 50 MB body with the correct 413 while VmHWM goes 49.4 MB → 165.4 MB. → F1 |
| B | Streaming json.NewDecoder(MaxBytesReader(...)).Decode(v) (the path_inspect.go pattern) |
Killed, 7 subtests (all over-the-cap boundary, unknown-length and real-chunked cases). This confirms the PR's argument for reading into a bounded buffer first. |
| C | Only a Content-Length precheck (if r.ContentLength > limit → 413), unbounded read otherwise |
Killed, 5 subtests (…UnknownLength/*, TestBodyLimitsOverRealChunkedRequests/*_over_the_cap) |
Suggested fix for F1 (I wrote and ran it locally; it is not pushed). Feed each endpoint an endless body through a counting reader that errors after 8 MiB, then assert the 413 and that the handler pulled no more than limit + a small slack:
type countingEndless struct{ n, hardStop int64 }
func (c *countingEndless) Read(p []byte) (int, error) { /* '{' then spaces; c.n += len(p); error after hardStop */ }
// for each (path, limit): ServeHTTP with ContentLength=-1, then
// assert w.Code == 413 && body.n <= limit+64<<10It passes on the PR code. Against mutant A it fails on both endpoints: "handler read 8673600 bytes of an endless body; cap is 4096 / 65536".
Tests run
All on the merged tree (03f72e19, base ff9a501c):
cd cmd/server && go test ./... -count=1 -timeout 45m: ok (1282 s on this 4-core box; a first run with the default-timeout 10mhit the deadline inTest1690_BackgroundLoadHonestywhile the ingestor suite ran in parallel. No test failed, it was just slow) [T]cd cmd/ingestor && go test ./... -count=1 -timeout 45m: ok (2018 s; same note: the default-timeout parallel run hit the 10 m deadline inTestConcurrentWrites) [T] (untouched by this PR)sh test-all.sh: 233 passed, 0 failed (233 files) [T]node test-frontend-helpers.js: 709 passed, 0 failed [T]- E2E setup. A local Go server ran against
e2e-fixture.dbprepared as in CI: freshen, the seed SQL block,corescope-migrate, seeds 2073, 199 (and 245, as CI does). It was stopped through its port's pid. - E2E results.
test-issue-1487-byop-modal-layout-e2e.js4/4 [T]test-map-modal-fluid-e2e.js10/10 (BYOP modal) [T]test-issue-1648-m2-icons-e2e.js21/21 [T]test-e2e-playwright.js: 131/135 passed, 4 skipped [T]. It fails fast on "Version info lives on Perf dashboard, not in navbar" (the#navStatswait times out) here, identically against an origin/master binary, so it is environmental and not this PR. I re-ran with only that test skipped; the other 3 skips are pre-existing.- The PR adds no E2E of its own.
- CI on the head commit (run 37683987831, attempt 1): ✅ Go Build & Test, ✅ Playwright E2E, ✅ Docker. Release, Deploy and Badges were skipped as expected. No reruns; neither flaky: #226 Hash Stats adopters sort E2E — server sorts multiByteNodes on packets only, ties come out in random order #256 nor flaky: TestResolvedPathBackfill_WriteHoldUnderBudget_188 — wall-clock write-hold budget fails under CI load #267 appears.
Not verified
- [K] I did not measure behaviour behind a reverse proxy, or with HTTP/2 or keep-alive reuse after a 413.
MaxBytesReaderasks net/http to close the connection, and the client upload was indeed cut short, but I did not test that a follow-up request on the same connection behaves. - [K] The full Playwright job was not run locally: only the files above, and CI covers the rest.
- [K] The
public/packets.jsobs-sort caller can send more than 200 hashes (it already gets a 400, which is swallowed, and above ~3450 hashes it now gets a 413). I agree with the author that this is pre-existing and belongs in a separate client-side chunking change. I did not reproduce it in the browser; the fixture has a single multi-observation group. - [K] No staging or prod access was used.
Round 2 of PR #359 (#334). F1 (blocking): nothing in the suite distinguished "cap enforced while reading" from "unbounded read, size checked afterwards" — both answer the same 413, so no status assertion can tell them apart, and the second one is the memory regression #334 exists to prevent. Two new tests assert on the bytes the handler actually pulls off the body: an endless body (one '{' then spaces forever, with an 8 MiB hard stop) and a finite body 16x over the cap must both be cut off within limit+4096 bytes. F2: docs/api-spec.md claimed every body-taking endpoint answers 413. It does not. The caps table now names the real route (POST /api/paths/inspect, not /api/path-inspect), lists PUT /api/config/geo-filter with its 1 MiB cap, and gives the actual over-cap status and body of each row — 413 only for the two endpoints #334 touches, 400 for the three capped earlier. The one body-taking route with no cap at all (POST /api/admin/prune-geo-filter?confirm=true) is named as a known gap. info.description is scoped the same way. A new test pins every row against the constant the handler enforces, and fails if the non-existent /api/path-inspect reappears. F3: json.Unmarshal rejects trailing data where the old json.Decoder.Decode ignored it. That is intended (it matches decodeSingleJSONObject) and is now a decision: TestBodyLimitsRejectTrailingData covers junk and a second JSON object after the body on both endpoints, and trailing whitespace still passes. F4: hoisted the "responses" literal into a local variable, so emitting routeMeta.Errors no longer needs a map[string]interface{} type assertion. The non-test occurrence count in cmd/ + internal/ is back to master's 745. F5: the OpenAPI summary for POST /api/packets/observations said "Batch submit observations" next to a description saying it returns them; it is now "Get observations for several packets", pinned by a test. Relates to #334.
Measured while checking review finding F2: the limits that predate #334 wrap the body and hand it to a streaming json.Decoder, so the limit only bites when the JSON value itself runs past it. A 6019-byte POST /api/paths/inspect body whose object ends at byte 19 answers 200 despite the 4096-byte limit, while the same shape is 413 on POST /api/decode. Calling both of those "a byte cap" in docs/api-spec.md would still mislead a client, so the caps table now carries an "Enforced on" column, the two enforcement models are spelled out with that measurement as the example, and the weaker enforcement is listed as a known gap alongside the status-code mismatch. info.description says the same in one sentence. The table rows stay pinned to the constants the handlers enforce. Relates to #334.
…lead The caps section opened with "every endpoint that takes a request body bounds it", which the gaps list three paragraphs down immediately contradicts: POST /api/admin/prune-geo-filter?confirm=true has no bound. Point at the exception from the lead sentence instead of only from the list. Relates to #334.
Rapport — CS-Macmini PR#359 runde-2 — head 1e3eccfReview feedback addressed (commit Reviewed against "Review — CS-pve-agent1 PR#359" (REQUEST CHANGES, head Evidence tags: [T] automated test, [A] automated/manual check against a built binary or live server, [K] known accepted gap. Findings1. F1 (blocking) — no test distinguished "cap before parsing" from "size check after an unbounded read". Agreed, and the fix is exactly the property the reviewer named: assert on the bytes the handler pulls off the body, not on the status it answers. Two new tests in
The 4096-byte slack is justified in the test: Mutant A (the reviewer's) red → green [T]: Four subtests red on the mutant that survived the entire previous suite. Note the second assertion line: with no bound, the hard stop also turns the answer into a 400, so the mutant is wrong twice over. 2. F2 (should fix) —
The extra correction (commit So the table gained an "Enforced on" column (commit I did not align the three 3. F3 (nit) — trailing data. Agreed it should be a decision. 4. F4 (nit) — 5. F5 (nit) — numbers and summary. Agreed; 3812 B is right and my 4013 B was wrong. Corrected throughout the PR description: the headroom table ( MutantsEach applied to a clean tree, run, then reverted and the tree verified clean with
Mutant F also confirms the hoist did not quietly drop the 413 metadata it refactors. TestsLocal, in this worktree, on the merged tree:
E2E. A local Go server on port 13801 against
Live checks against the built binary [A], all on the same server: CI (per job)Run 37743709836, attempt 1, head
Neither known-unstable test (#271, #301) failed, so no job needed a second run. The intermediate run on Rester
|
Review — CS-pve-agent1 PR#359 — head 1e3eccfDom: APPROVE med nits Re-review after round 2. The blocking finding (F1) is fixed properly: the new tests measure the bytes the handler pulls off the body, not just the status code. My Mutant A is now red even in a variant that still answers the correct 413. F2–F5 are addressed. What is left is wording drift in the PR description and the docs, plus one doc test whose name promises more than it checks. None of it blocks a merge. Evidence tags: [T] automated test I ran, [A] manual check I ran against a built binary or local server, [K] known or accepted gap / not verified. Reviewed: head Findings
Answers to the review pointsF1 (was blocking): fixed. [T][A]
F2: docs and
F3: done. F4: done.
The type assertion is gone. The F5: mostly done. 3812 / 3831 / 284 / 17.2× are right [A] ( Scope. Round 2 only touches the five PR files and the findings' subject matter. The single change outside the two #334 handlers is the value-identical Standing checks. [A]
Tests and mutantsRed before / green after. I put both handlers back on master's unbounded
All are green on the merged tree. Every #334 acceptance criterion therefore has a test that is red before and green after:
The 200-hash limit is covered by round 1's My mutants. Each was run on a scratch copy of the merged tree with
Suites on the merged tree (
E2E. A local Go server ran against
CI on the head (run 37743709836, attempt 1,
There were no reruns, and neither #256 nor #267 appears. Not verified
|
3 participants
Relates to #334
Summary
POST /api/decodeandPOST /api/packets/observationshandedr.Bodystraight to ajson.Decoderwith no byte cap. The batch endpoint's 200-hash limit is a cardinality check that runs after parsing, so neither endpoint bounded how much a single request could make the server allocate.Both handlers now go through one new helper,
decodeLimitedJSONBody(cmd/server/request_body_limits.go):Reading the bounded body first, rather than letting a
json.Decoderstream it, is what makes this a byte cap: the verdict depends only on how many bytes arrived, never on where in the JSON the decoder happened to stop. A streaming decoder would let an oversized body through whenever the value it wants happens to complete inside the firstlimitbytes (trailing padding, ignored fields). Exactlylimitbytes is accepted;limit+1is 413.Over the cap answers 413 via the existing
writeErrorshape (#266) —{"error":"request body too large (max N bytes)"},application/json— so there is one error contract across endpoints. Malformed JSON within the cap keeps its existing 400. No newmap[string]interface{};cmd/serverstays read-only (no SQL touched).Why these limits (upstream's 4 KiB / 64 KiB, checked against our clients)
Upstream's numbers hold for us, and both are now asserted by tests rather than asserted in prose:
POST /api/decodePOST /api/packets/observationspublic/packets.js, which posts{"hex": <stripped hex>}. The largest frameDecodePacketcan accept at all is 1 header + 4 transport codes + 1 path-length byte +MAX_PATH_SIZE(64) path bytes +MAX_PACKET_PAYLOAD(184) payload bytes = 254 bytes → 508 hex chars → a ~520-byte body. Anything bigger is a frame the decoder rejects anyway. 4 KiB also matches the limit already onPOST /api/paths/inspect(inspectBodyLimit).{"hashes": [...]}of 16-hex content hashes. A full 200-hash request measures 3812 bytes on the wire ({"hashes":[+ 200x18 + 199 commas +]}; a 201-hash request is 3831 bytes). That is the reason 64 KiB is the right number and 4 KiB would not be: a 4 KiB cap would leave only 284 bytes of slack on a request the handler is supposed to accept, so the byte cap would start masking the 200-hash limit. At 64 KiB every request inside the 200-hash limit passes and a 201-hash request still gets the established400 too many hashes, not a 413.TestDecodeBodyLimitFitsLargestDecodableFrameandTestBatchObservationsHashLimitPreservedassert the headroom on both sides (≥4x so no legitimate client is cut off, ≤32x / ≤64x so a cap loosened to the point of bounding nothing fails CI).Deliberate behaviour change: trailing data is now rejected
decodeLimitedJSONBodyfinishes withjson.Unmarshal, which rejects anything but whitespace after the JSON value. Thejson.Decoder.Decodethese two handlers used before silently ignored it, so three bodies that were 200 on master are 400 on this branch:This is intentional, not a side effect: it matches
decodeSingleJSONObjectonPOST /api/channel-proposals, and our only clients build their bodies withJSON.stringify, which cannot produce trailing data. Trailing whitespace is valid JSON and is still accepted — the boundary tests pad with it.TestBodyLimitsRejectTrailingDatapins all four rejections plus the whitespace case.Preserved behaviour
TestBatchObservationsHashLimitPreserved, plus the pre-existingTestHandleBatchObservations).{"hex":"0200"}still 200 withdecoded.{header,path,payload}; empty hex still400 hex is required; undecodable hex still 400 (TestDecodeNormalRequestUnchanged).hashesstill{"results":{}}; malformed JSON still400 invalid JSON body.Not a proxy concern
The caps are enforced in the application.
TestBodyLimitsOverRealChunkedRequestsdrives a realhttptest.Serverover a real connection with a body of unknown length, andTestChunkedRequestHasNoContentLengthproves that request really arrives asTransfer-Encoding: chunkedwithContentLength == -1. A separate case feeds aContent-Lengththat understates the real body and still gets 413. Verified again with curl against a built binary (below).Perf
No hot path. One
io.ReadAllinto a buffer that is now bounded where it previously was not (thejson.Decoderallocated from the same unbounded stream), so peak per-request allocation on these two endpoints goes from unbounded to ≤4 KiB / ≤64 KiB. Strictly a reduction in worst case; no change in the normal case.Docs
cmd/server/openapi.go:routeMetagains a typedErrors []routeError, emitted as a real413response object (typedopenAPIResponse/openAPIMediaType, per the refactor: drive map[string]interface{} out of API response layer + internal data (694 occurrences in cmd/) — typed structs, codegen for frontend Kpa-clawbot/CoreScope#1383 no-new-untyped-map-literals convention) referencing a newApiErrorcomponent schema. Both route descriptions andinfo.descriptionstate the caps. Verified on the served/api/spec.docs/api-spec.md: new Request-body byte caps section under Error Responses (table of all four body-capped endpoints, and that the cap is on bytes received, not on the parsed value or onContent-Length),413added to the shared error list and toPOST /api/decode, and a newPOST /api/packets/observationssection — that endpoint was undocumented.TestAPISpecDocumentsBodyLimitspins the documented numbers to the constants, so the two cannot drift.Config follow-up (AGENTS.md rule 8)
Both limits are hardcoded constants for now, with a
TODO(#334, AGENTS.md rule 8)incmd/server/request_body_limits.go: they should become operator-configurable (config.json, and the customizer where it makes sense) in a later milestone. Not done here to keep this change to the security fix.Tests
New
cmd/server/request_body_limits_test.go(16 top-level tests, 31 cases):TestDecodeBodyLimitBoundaries,TestBatchObservationsBodyLimitBoundaries(limit−1 / limit / limit+1)TestDecodeBodyLimitOversizedUnknownField,TestBatchObservationsBodyLimitOversizedUnknownFieldTestBodyLimitsOverRealChunkedRequests(real connection),TestChunkedRequestHasNoContentLength,TestDecodeBodyLimitUnknownLength(incl. an understatedContent-Length),TestBatchObservationsBodyLimitUnknownLengthTestDecodeBodyLimitMalformedJSON,TestBatchObservationsBodyLimitMalformedJSON(under the cap → 400; over it → 413, bytes before syntax; empty body → 400)TestDecodeNormalRequestUnchanged,TestBatchObservationsNormalRequestUnchangedTestDecodeBodyLimitFitsLargestDecodableFrame,TestBatchObservationsHashLimitPreservedTestOpenAPIDocuments413OnBodyLimitedRoutes,TestAPISpecDocumentsBodyLimitsEvery 413 assertion also checks the body is the shared
{"error": ...}shape withContent-Type: application/json.Red before, green after
Captured on this branch's
origin/masterstarting point with only the two constants stubbed in (so the tests compile and fail on behaviour, not on a missing symbol) — 12 subtests red:TestOpenAPIDocuments413OnBodyLimitedRoutesandTestAPISpecDocumentsBodyLimitswere red on the same baseline (no 413 in the spec, no caps section in the doc). All green after.Mutants (7, each reverted after confirming red)
http.MaxBytesReaderwrapper dropped from the helperhttp.StatusBadRequestMaxBytesReader(..., limit+1)json.NewDecodermaxDecodeBodyBytesraised to 1 MiBTestDecodeBodyLimitFitsLargestDecodableFrame(>32x headroom),TestAPISpecDocumentsBodyLimitsmaxBatchObservationsBodyByteslowered to 1 KiBTestBatchObservationsHashLimitPreserved(both cases), the pre-existingTestHandleBatchObservations/too_many_hashes_returns_400,TestAPISpecDocumentsBodyLimitsTestOpenAPIDocuments413OnBodyLimitedRoutesMutant 6 is the one that matters for "preserve the 200-hash limit": a too-tight byte cap masks the cardinality limit, and the suite says so.
Live verification against a built binary
cmd/serverbuilt and started on port 13701 against a copy oftest-fixtures/e2e-fixture.dbfreshened, migrated and seeded exactly as the deploy workflow does.The chunked cases were confirmed to send no
Content-Lengthat all (curl -vrequest headers:Transfer-Encoding: chunked, no length header).Served spec on the same binary:
CI (per job, run locally in this worktree)
go build ./...,go vet ./...incmd/serverandcmd/ingestor— clean.gofmt -lon every touched/new file (request_body_limits.go,request_body_limits_test.go,routes.go,openapi.go) — no output. (Many pre-existing files incmd/serverare not gofmt-clean; none of them are touched here.)cd cmd/server && go test ./...— ok, 54.7s, no regressions.cd cmd/ingestor && go test ./...— ok, 112.6s. (Nointernal/*module touched.)sh test-all.sh— 233/233 files passed.node test-frontend-helpers.js— 709 passed, 0 failed.scripts/check-xss-sinks.sh --diff origin/master— clean (nopublic/**changes to scan).test-e2e-playwright.js132/135 passed, 3 skipped (the 3 skips are pre-existing and baked into the test),test-issue-1487-byop-modal-layout-e2e.js4/4,test-map-modal-fluid-e2e.js10/10 (includes the BYOP decode modal),test-issue-1648-m2-icons-e2e.js21/21,test-issue-147-packets-url-obs-e2e.js12/12,test-packet-detail-sender-hash-size-obs-e2e.js3/3,test-packets.js161/161,test-issue-1415-packets-layout.js26/26.deploy.yml9,release-fast-path.yml1 — neither file touched.GitHub Actions on this PR is checked after push; see the report comment.
Rester (leftovers / out of scope)
public/packets.jscan build a batch request larger than the handler accepts. The obs-sort path doespackets.filter(...).map(p => p.hash)with no cap, andPACKET_LIMITis 50000 on desktop, so a page with more than 200 multi-observation groups posts more than 200 hashes. That request already fails today with400 too many hashes(the response is swallowed byif (resp.ok)), so this PR does not introduce the problem — but above ~3400 hashes the answer changes from that 400 to a 413. Chunking the client into 200-hash batches is the real fix; it is new client logic with its own tests and belongs in its own issue, not in a server-side body-limit change.400.POST /api/paths/inspect(4096 B),POST /api/channel-proposals(1024 B) andPUT /api/config/geo-filter(1 MiB) wrap the body inhttp.MaxBytesReaderand hand it to a streaming decoder, so the limit only bites when the JSON value runs past it — a 6019-byte/api/paths/inspectbody whose object ends at byte 19 answers200[A], where the same shape is413on/api/decode.POST /api/admin/prune-geo-filter?confirm=true(API key) has no limit at all.docs/api-spec.mdnow states each endpoint's limit, how it is enforced, and its over-limit status, and lists all three as known gaps. Tightening them is API-visible, touches handlers this issue does not, and is deliberately not bundled here.