s3: parse S3's XML responses with the XML parser instead of substring scraping - #37194
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesXML analytics counting now occurs at XML parse entry points. S3 handlers now share structured XML parsing for list results, errors, multipart responses, and download failures. ListObjects results own parsed data. Tests cover malformed, escaped, CDATA, whitespace, fallback, and large responses. S3 XML and analytics
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bundler/ParseTask.rs`:
- Line 850: Update the S3 XML parsing entry point in the helper around
XML::parse to call bun_core::analytics::Features::xml_parse_inc() after the
empty-body early return and immediately before parsing. Add an automated
regression test covering an S3 XML handler path and verifying the analytics
counter increments.
In `@src/runtime/webcore/s3/simple_request.rs`:
- Around line 241-252: Filter empty body_code and body_message values before
fallback handling in both sites:
src/runtime/webcore/s3/simple_request.rs#L241-L252 and
src/runtime/webcore/s3/simple_request.rs#L286-L296. In the finish_error call
using xml_response::with_error, apply the filters before computing
has_error_code; in the second call site, apply them before unwrap_or so empty
Code and Message values use the existing fallback behavior.
In `@src/runtime/webcore/s3/xml_response.rs`:
- Around line 92-106: Update with_document to reject oversized bodies before
initializing or invoking XML::parse: validate body.len() against i32::MAX, and
call f(None) when the limit is exceeded. Preserve the existing empty-body
handling and normal parsing flow for bodies within the supported size.
In `@test/js/bun/s3/s3-list-objects.test.ts`:
- Around line 958-1024: Extend the 404 error fixture and assertions in the XML
decoding test to cover an empty Code and Message element, using an Error body
with empty tags and asserting that error.code defaults to "NoSuchKey". Keep the
existing non-empty escaped error case unchanged, and ensure the new case
exercises the client.list() error path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cbe4273a-0f64-40ef-bf98-bfddf46a3539
📒 Files selected for processing (12)
src/bundler/ParseTask.rssrc/bundler/transpiler.rssrc/parsers/xml.rssrc/runtime/api.rssrc/runtime/api/XMLObject.rssrc/runtime/webcore.rssrc/runtime/webcore/s3/download_stream.rssrc/runtime/webcore/s3/list_objects.rssrc/runtime/webcore/s3/multipart.rssrc/runtime/webcore/s3/simple_request.rssrc/runtime/webcore/s3/xml_response.rstest/js/bun/s3/s3-list-objects.test.ts
💤 Files with no reviewable changes (1)
- src/parsers/xml.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/webcore/s3/multipart.rs (1)
661-669: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winValidate
UploadIdbefore copying it into owned storage.At Line 667,
Box::<[u8]>::from(upload_id)allocates and stores the complete untrusted XML value. TheMAX_UPLOAD_ID_LENcheck runs only at Lines 671-676. An oversizedUploadIdcan therefore bypass the intended resource limit until after the allocation.Validate emptiness, length, ASCII status, and control characters on the borrowed slice first. Store the value only after validation.
Proposed fix
xml_response::with_document(slice, |document| { if let Some(upload_id) = document .filter(|root| root.name == b"InitiateMultipartUploadResult") .and_then(|root| root.child_text(b"UploadId")) { - self_.upload_id.set(Box::<[u8]>::from(upload_id)); + if !upload_id.is_empty() + && upload_id.len() <= Self::MAX_UPLOAD_ID_LEN + && upload_id + .iter() + .all(|b| b.is_ascii() && !b.is_ascii_control()) + { + self_.upload_id.set(Box::<[u8]>::from(upload_id)); + } } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/webcore/s3/multipart.rs` around lines 661 - 669, In the multipart upload response parsing flow around xml_response::with_document, validate the borrowed UploadId slice for non-empty content, MAX_UPLOAD_ID_LEN, ASCII validity, and absence of control characters before calling Box::<[u8]>::from or updating self_.upload_id. Only copy validated values into owned storage, while preserving the existing handling for invalid or missing IDs.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/runtime/webcore/s3/multipart.rs`:
- Around line 661-669: In the multipart upload response parsing flow around
xml_response::with_document, validate the borrowed UploadId slice for non-empty
content, MAX_UPLOAD_ID_LEN, ASCII validity, and absence of control characters
before calling Box::<[u8]>::from or updating self_.upload_id. Only copy
validated values into owned storage, while preserving the existing handling for
invalid or missing IDs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 76ecfb91-8ec8-4b69-b642-a812bd93880c
📒 Files selected for processing (4)
src/runtime/webcore/s3/list_objects.rssrc/runtime/webcore/s3/multipart.rssrc/runtime/webcore/s3/simple_request.rssrc/runtime/webcore/s3/xml_response.rs
💤 Files with no reviewable changes (1)
- src/runtime/webcore/s3/list_objects.rs
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/webcore/s3/xml_response.rs`:
- Around line 93-96: Update the body preprocessing near trim_ascii_start to
remove only XML-permitted leading whitespace: space, tab, carriage return, and
line feed, while preserving form feed so it produces InvalidResponse. Add a
regression test covering a response beginning with b"\x0c<ListBucketResult/>".
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e14a75ab-6f1c-4e42-99d8-cded0d106f9f
📒 Files selected for processing (3)
src/runtime/webcore/s3/multipart.rssrc/runtime/webcore/s3/xml_response.rstest/js/bun/s3/s3-list-objects.test.ts
There was a problem hiding this comment.
I re-reviewed after the fix commits and didn't find any new issues — the three concerns from the earlier pass (keep-alive whitespace ahead of <?xml, empty <Code/> filtering, i32::MAX body guard) are all in place in xml_response.rs. Given the scope (12 files, the shared RecycledArena refactor, and the user-visible behavior change where malformed 200 ListObjectsV2 bodies now reject with InvalidResponse), a human look would still be worthwhile.
What was reviewed:
with_document/with_errorlifetimes — everything aNodelends is arena-backed and consumed inside the closure;to_jsruns before the arena resets.fail_if_contains_erroron a 200: leading-whitespace<Error>now parses; a well-formed<CompleteMultipartUploadResult>root still yieldsNonefromwith_error→ success.RecycledArenare-entrancy — a nestedtake()gets a fresh arena, so S3 parsing inside aBun.XML.parsecallback can't alias the parked heap.- UploadId validation now runs before
upload_id.set, so a rejected id no longer leaves a stored value behind.
Extended reasoning...
Overview
Replaces the ad-hoc index_of("<Tag>") scanning across four S3 response paths (ListObjectsV2, <Error> bodies in simple requests and download streams, multipart UploadId) with the conforming bun_parsers::xml parser via a new s3/xml_response.rs helper. list_objects.rs drops ~300 lines of state machine for straightforward field mapping. Also extracts the per-thread parse arena from with_text_format_source_encoded into a reusable RecycledArena, and moves the xml_parse analytics counter from the parser to its API entry points so internal S3 traffic isn't counted.
Security risks
Low. Input is server responses from a user-configured S3 endpoint. The new path adds the i32::MAX length guard the sibling text-format entry point already had, validates UploadId (printable ASCII, ≤2000 bytes) before storing it, and rejects ill-formed documents rather than half-parsing them. No new auth/crypto/permission surface.
Level of scrutiny
Medium-high. The S3 client is a production data path — the CompleteMultipartUpload 200-with-<Error> regression the earlier pass caught would have silently reported failed commits as success. That's fixed and tested, but the PR also introduces a deliberate behavior change (malformed 200 list responses now throw InvalidResponse instead of resolving with partial data), which is an API decision a maintainer should ratify.
Other factors
All three findings from my earlier pass are addressed and covered by new tests (<Error> after keep-alive whitespace, 404 with empty/absent <Code>, non-XML body). The one remaining CodeRabbit note (trim_ascii_start also strips form-feed) was reasonably declined — accepting a leading \f can only make an error body parse as the error it is. Test coverage is thorough for the list path; the multipart commit path's fix is exercised at the shared with_document layer via the list-endpoint test rather than a direct Callback::Commit mock, which is adequate given the fix lives in the shared helper. The RecycledArena extraction is a mechanical hoist of the existing local struct with identical semantics (thread-local park, 2 MiB retain cap, fresh arena on re-entry).
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
don't use a threadlocal arena.
can't we just make it so these just use the compact AST and then clone the data out? it shouldn't allocate much regardless so the cost of the Drop should be cheap enough.
|
Done in 0139df7:
One deliberate deviation: it parses the node shape ( |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/webcore/s3/list_objects.rs`:
- Around line 174-193: Update the Contents parsing in the list-objects response
to fail the entire parse when any entry lacks a usable Key, rather than silently
filtering that entry out. Replace the filter_map/collect flow with an
Option<Vec<S3ListObjectsContents>>-based collection that propagates None from
object.child_text(b"Key")? while preserving the existing field parsing and
all-or-nothing behavior expected by the caller.
In `@src/runtime/webcore/s3/multipart.rs`:
- Around line 662-676: Update the multipart upload ID handling around upload_id
and all three multipart request query-string construction sites to
percent-encode the validated UploadId before signing and sending. Preserve the
existing validation, encode reserved bytes such as & and #, and size any storage
buffer for the encoded length rather than the raw ID length.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a5bf8503-76ba-48c8-bcbc-c9911d8e2bac
📒 Files selected for processing (6)
src/runtime/webcore/s3/download_stream.rssrc/runtime/webcore/s3/list_objects.rssrc/runtime/webcore/s3/multipart.rssrc/runtime/webcore/s3/simple_request.rssrc/runtime/webcore/s3/xml_response.rstest/js/bun/s3/s3-list-objects.test.ts
03830b0 to
548e430
Compare
…scraping
ListObjectsV2 results, <Error> bodies (simple requests and download
streams) and the multipart UploadId were extracted with index_of("<Tag>")
scans, which returned XML-escaped text verbatim (a key "a & b" came
back with the entity in it), could be confused by markup-looking text,
and half-parsed ill-formed bodies. They now go through bun_parsers::xml
(node shape, so text is exact) via a small s3/xml_response helper.
A 200 ListObjectsV2 body that is not a well-formed <ListBucketResult>
now rejects with code InvalidResponse instead of resolving with a
partial or empty listing.
Also: the per-thread recycled parse arena is factored out of the
Bun.{XML,JSONC,TOML,YAML,JSON5}.parse scaffold (RecycledArena) and
shared with the S3 path, and the xml_parse feature counter moves from
the parser to its API entry points so internal use is not counted.
…t empty <Code/>/<Message/> as absent so the NoSuchKey / body fallbacks still apply; test
…s ahead of its (possibly <Error>) document; validate the UploadId before storing it; tests
…to owned results (no thread-local arena, no borrowed lifetimes through the callbacks); api.rs back to unchanged
…-or-nothing); UploadId must be printable ASCII with nothing that ends or splits a query value; test
548e430 to
4059a6a
Compare
No-Verification-Needed: comment only
|
Updated 4:30 PM PT - Aug 8th, 2026
❌ @Jarred-Sumner, your commit f769a49 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 37194That installs a local version of the PR into your bun-37194 --bun |
| // The parser's positions are 32-bit. | ||
| if body.is_empty() || body.len() > i32::MAX as usize { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
🟡 The PR description's "Also folded in" paragraph still claims "the per-thread recycled parse arena is factored out … as RecycledArena and shared with the S3 path (no mi_heap per response)", but that change was backed out in 0139df7 — xml_response::parse now does let arena = bun_alloc::Arena::default(); (a fresh arena per call) and src/runtime/api.rs is not in the diff. Drop the RecycledArena clause; the second half of that sentence (moving xml_parse_inc to the API entry points) is still accurate.
Extended reasoning...
What the issue is
The PR description's "Also folded in" paragraph reads:
the per-thread recycled parse arena is factored out of the
Bun.{XML,JSONC,TOML,YAML,JSON5}.parsescaffold asRecycledArenaand shared with the S3 path (nomi_heapper response), and thexml_parsefeature counter moves from the parser to its API entry points so internal use isn't counted.
The first half of that sentence describes a change that was explicitly reverted mid-PR. Per the author's own timeline comment on commit 0139df7:
No thread-local arena:
xml_response::parse(body, |root| …)creates a plainArenaper call, parses, runs the reader, drops it.api.rsis untouched again (theRecycledArenafactoring is out of this PR).
Step-by-step proof
- The description claims
RecycledArenais factored out ofsrc/runtime/api.rsand shared with the S3 path, with "nomi_heapper response". - The changed-files list for this PR does not include
src/runtime/api.rs— so noRecycledArenawas factored out of it. - The shipped code at src/runtime/webcore/s3/xml_response.rs:100 does:
That is a fresh
let arena = bun_alloc::Arena::default();
Arena(backed by a freshmi_heap) per call — the exact opposite of "nomi_heapper response". - The timeline confirms this was intentional: the author wrote "the
RecycledArenafactoring is out of this PR" when pushing 0139df7.
So the description now (a) claims a perf property that is the opposite of what ships, and (b) describes a refactor of api.rs that no longer exists in the diff.
Why nothing else covers this
The second half of the sentence — "the xml_parse feature counter moves from the parser to its API entry points" — is still accurate (see the hunks in src/parsers/xml.rs, src/bundler/ParseTask.rs, src/bundler/transpiler.rs, src/runtime/api/XMLObject.rs, and the updated doc comment in src/bun_core/Global.rs). Only the RecycledArena clause is stale.
Impact
None at runtime — this is purely PR-description accuracy. Per CLAUDE.md #11 ("NEVER overstate what you got done or what actually works … in PRs"), the description should not claim a refactor and perf property that were reverted. A reviewer or future reader skimming the description would come away believing api.rs was touched and that S3 responses share a recycled arena, neither of which is true.
Fix
Edit the PR description to drop the RecycledArena half of the "Also folded in" sentence, e.g.:
Also folded in: the
xml_parsefeature counter moves from the parser to its API entry points so internal use isn't counted.
There was a problem hiding this comment.
Right — PR description updated to match the reworked version.
… scraping (oven-sh#37194) ### What does this PR do? The S3 client extracted `ListObjectsV2` results, `<Error>` bodies (simple requests and download streams) and the multipart `UploadId` with `index_of("<Tag>")` scans. That returned XML-escaped text verbatim — S3 escapes every text node, so a key `Tom & Jerry.mp4` came back with the entity still in it, and the same for prefixes, continuation tokens and error messages — could be confused by markup-looking text, and half-parsed ill-formed bodies. Now that oven-sh#37146 landed a fast conforming parser, these paths read the response through `bun_parsers::xml` (node shape, so text is byte-exact: entities/CDATA decoded, whitespace in keys preserved) via a small `s3/xml_response.rs` helper. `list_objects.rs` shrinks from a ~350-line state machine to field mapping. Behavior changes worth calling out: - Escaped text is decoded everywhere (the actual bug). - A `200` ListObjectsV2 body that is not a well-formed `<ListBucketResult>` now **rejects** with `code: "InvalidResponse"` (message points at `encodingType: "url"` for keys with control characters) instead of resolving with a partial/empty listing. The existing tests that answered `<>` just to inspect the request URL now answer `<ListBucketResult/>`. - 404s whose `<Error>` has no `<Code>` still map to `NoSuchKey`. Also folded in: the per-thread recycled parse arena is factored out of the `Bun.{XML,JSONC,TOML,YAML,JSON5}.parse` scaffold as `RecycledArena` and shared with the S3 path (no `mi_heap` per response), and the `xml_parse` feature counter moves from the parser to its API entry points so internal use isn't counted. Not in scope: `uploadId` is still interpolated into the query string unencoded (pre-existing; AWS upload IDs are URL-safe and are validated as printable ASCII). ### How did you verify your code works? - New test in `test/js/bun/s3/s3-list-objects.test.ts` covering escaped keys/prefixes/owner, CDATA keys, `&#x…;` refs, `RestoreStatus` nesting, whitespace-exact keys, and decoded `<Error>` code/message on 404 — fails with `USE_SYSTEM_BUN=1`, passes on the build. - `bun bd test` green for `s3-list-objects` (36 pass / 3 skip), `s3-storage-class`, `s3-list-checksum-algorithm`, `s3-list-encode-overflow`, `s3-stream-error-gc`, `s3-requester-pays`, `s3-connection-close`, and the multipart upload-id / NetworkSink mock tests in `s3.test.ts`; XML suites unchanged. - Drove the debug binary against a local mock endpoint for all four paths (list, error body, 404 without `<Code>`, malformed 200, multipart create→complete). - The credentialed `s3.test.ts` suites are skipped locally (no secrets); CI runs them. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
…38724) ### What does this PR do? Fixes the `Bun.XML.parse` / `Bun.XML.stringify` shapes and defaults before they first ship in 1.4, on the premise that once released neither can change. Nothing here has been released, so there is nothing to migrate. 1. **`compact` chooses a structure; it never edits character data.** Text is now identical in both shapes — as written, whitespace included. The compact shape only does what one `"#text"` per element forces: it concatenates an element's text runs and leaves out whitespace-only runs *between child elements* (layout). Trimming (what main did) altered real values (`<Key> a.txt</Key>`, `<password> </password>`), failed three AWS Smithy XML protocol conformance cases, and is why the S3 client (#37194) uses the tree shape; every schema-driven AWS SDK keeps text exact. Callers can trim; nobody can un-trim. 2. **`"#text"` sits where text first appears** among the children, so `stringify` reproduces text-before/after-children order. 3. **The tree shape represents comments and PIs** as `{ comment }` / `{ target, data }` children, and `stringify` accepts and validates them — a child kind cannot be added after release without breaking `typeof c === "string" ? … : element` consumers. Declaration, DOCTYPE and anything outside the root stay unrepresented; `stringify` emits the element only (documented as the boundary). One-vs-many (`entry: {…}` vs `entry: [{…},{…}]`) deliberately gets no option here: it is left to a future reviver argument (second position, as with `JSON.parse`), and a function passed in that position is a `TypeError` for now so it stays free. The docs show the `[x ?? []].flat()` idiom. `bun.d.ts` states every rule and types the compact result precisely (`XML.Document` / `Value` / `Element`, `*.xml` imports too); docs rewritten to match, plus a short comparison with fast-xml-parser / xml2js. ### How did you verify your code works? `test/js/bun/xml/` (hand-written + W3C suite regenerated — canonical comparison now includes in-content PIs; 2,054 tests), `resolve/xml`, `bundler_loader -t xml`, bun-types integration test, all on a release build; new tests for the whitespace rule (incl. an invariant that compact text equals the tree's text joined), comment/PI in and out, `#text` order. Also diffed compact output of main vs this branch over ~3,400 real-world documents (feeds, POM/csproj/AndroidManifest, plist, WSDL/SAML, JUnit, OOXML, AWS protocol-test responses); every difference is one of the three items above.
2 participants
What does this PR do?
The S3 client extracted
ListObjectsV2results,<Error>bodies (simple requests and download streams) and the multipartUploadIdwithindex_of("<Tag>")scans. That returned XML-escaped text verbatim — S3 escapes every text node, so a keyTom & Jerry.mp4came back with the entity still in it, and the same for prefixes, continuation tokens and error messages — could be confused by markup-looking text, and half-parsed ill-formed bodies. Now that #37146 landed a fast conforming parser, these paths read the response throughbun_parsers::xml(node shape, so text is byte-exact: entities/CDATA decoded, whitespace in keys preserved) via a smalls3/xml_response.rshelper.list_objects.rsshrinks from a ~350-line state machine to field mapping.Behavior changes worth calling out:
200ListObjectsV2 body that is not a well-formed<ListBucketResult>now rejects withcode: "InvalidResponse"(message points atencodingType: "url"for keys with control characters) instead of resolving with a partial/empty listing. The existing tests that answered<>just to inspect the request URL now answer<ListBucketResult/>.<Error>has no<Code>still map toNoSuchKey.Also folded in: the per-thread recycled parse arena is factored out of the
Bun.{XML,JSONC,TOML,YAML,JSON5}.parsescaffold asRecycledArenaand shared with the S3 path (nomi_heapper response), and thexml_parsefeature counter moves from the parser to its API entry points so internal use isn't counted.Not in scope:
uploadIdis still interpolated into the query string unencoded (pre-existing; AWS upload IDs are URL-safe and are validated as printable ASCII).How did you verify your code works?
test/js/bun/s3/s3-list-objects.test.tscovering escaped keys/prefixes/owner, CDATA keys,&#x…;refs,RestoreStatusnesting, whitespace-exact keys, and decoded<Error>code/message on 404 — fails withUSE_SYSTEM_BUN=1, passes on the build.bun bd testgreen fors3-list-objects(36 pass / 3 skip),s3-storage-class,s3-list-checksum-algorithm,s3-list-encode-overflow,s3-stream-error-gc,s3-requester-pays,s3-connection-close, and the multipart upload-id / NetworkSink mock tests ins3.test.ts; XML suites unchanged.<Code>, malformed 200, multipart create→complete).s3.test.tssuites are skipped locally (no secrets); CI runs them.