fix(flue-review): cap tar entry size and honor pax linkpath overrides - #2388
Conversation
|
github-actions
Bot
added
size/M
review/needs-review
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the right follow-up: it closes the two security/correctness gaps in the flue-review untar parser (entry size cap and pax linkpath / GNU K long-link support) without touching published packages or the admin UI. The approach is sound for GitHub source tarballs, and the override precedence (pax > GNU longlink/longname > header) matches tar conventions.
I traced the changed paths, the escape validation applied to overridden symlink targets, and the new tests. The logic looks correct, but the PR claims to reject non-numeric and negative size fields in addition to oversized ones and only adds a test for the oversized case. AGENTS.md is explicit that bug fixes need reproducing tests. I also flagged the one-line noUncheckedIndexedAccess fix in the R2 instrumentation as a minor scope-discipline note; it is unrelated to the untar parser changes.
No query-count, i18n, RTL, SQL, API-envelope, authorization, or changeset issues apply here — the worker is private and has no user-facing UI.
| ), | ||
| ).rejects.toThrow(/size out of range/); | ||
| expect(r.files.size).toBe(0); | ||
| }); |
There was a problem hiding this comment.
[needs fixing] The PR description says entries with a non-numeric or negative size field are now rejected, but the only new test covers an oversized (128 MiB) field. AGENTS.md asks for a failing test before the fix and verification after it; adding coverage for malformed size fields protects the new guard from future regressions.
| }); | |
| }); | |
| it("rejects entries with a non-numeric or negative size field", async () => { | |
| async function expectRejected(sizeBytes: string) { | |
| const block = header({ name: "repo-abc/bad.bin", size: 0 }); | |
| const bytes = [...sizeBytes].map((c) => c.charCodeAt(0)); | |
| block.set(bytes, 124); | |
| block[124 + bytes.length] = 0; | |
| const r = recorder(); | |
| await expect( | |
| untarInto(r.target, tarball([block]), "/repo"), | |
| ).rejects.toThrow(/size out of range/); | |
| expect(r.files.size).toBe(0); | |
| } | |
| await expectRejected("bogus"); | |
| await expectRejected("-1"); | |
| }); |
| try { | ||
| // oxlint-disable-next-line typescript/no-unsafe-type-assertion | ||
| const result = await (bucket as unknown as Record<string, Function>)[method](...args); | ||
| const result = await (bucket as unknown as Record<string, Function>)[method]!(...args); |
There was a problem hiding this comment.
[suggestion] This one-line ! addition in the R2 instrumentation fixes a type error but is unrelated to the untar parser change. AGENTS.md discourages drive-by cleanups in unrelated files; if this was needed to keep pnpm typecheck green for the worker, note that in the test evidence, otherwise it belongs in its own PR.
github-actions
Bot
added
review/awaiting-author
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-cache | 5347166 | Aug 09 2026, 06:37 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-playground | 5347166 | Aug 09 2026, 06:37 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-do | 5347166 | Aug 09 2026, 06:39 AM |
There was a problem hiding this comment.
Pull request overview
This PR hardens the infra/flue-review tarball untar parser by adding defensive limits around entry sizes and improving correctness for symlink target decoding when tar headers exceed legacy ustar field sizes.
Changes:
- Add a 64 MiB per-entry declared-size cap (and reject negative / non-finite sizes) to avoid unbounded buffering in Durable Object memory.
- Honor pax
linkpathand GNUKlong-link records for symlink targets, applying the same escape validation as for normal link targets. - Fix a
noUncheckedIndexedAccesstype error in R2 operation instrumentation by asserting the wrapped method exists.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| infra/flue-review/.flue/lib/untar.ts | Adds entry size cap and support for pax/GNU symlink target overrides in the untar trust-boundary parser. |
| infra/flue-review/test/untar.test.ts | Adds regression tests for symlink target override handling and the entry-size cap. |
| infra/flue-review/.flue/sandboxes/cloudflare-shell.ts | Adjusts R2 instrumentation to satisfy noUncheckedIndexedAccess. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const rawName = readCString(header.subarray(0, 100)); | ||
| const prefix = readCString(header.subarray(345, 500)); | ||
| const size = parseInt(readCString(header.subarray(124, 136)).trim() || "0", 8); | ||
| // Mode bytes (100-108) are ignored: the workspace has no chmod and the | ||
| // reviewer never executes files. |
|
Both review rounds addressed in 5347166. The size field is now validated as strictly octal before parsing — Copilot's point was right that On the R2 instrumentation ~ 🤖 Claude Fable 5 |
There was a problem hiding this comment.
This is the right follow-up to the flue-review untar parser review. The PR closes the two stated gaps — an unbounded entry-size allocation risk and missing pax linkpath / GNU long-link (K) support — without touching published packages or the admin UI.
The previous re-review noted that the PR claimed to reject non-numeric and negative size fields but only tested the oversized case. That has now been addressed in infra/flue-review/test/untar.test.ts with the "rejects entries whose size field is not strictly octal" case, which includes "-0000001".
I re-read the untar parser, the new tests, and the one-line noUncheckedIndexedAccess fix in sandboxes/cloudflare-shell.ts. The size-cap placement (before need(padded)) prevents trying to buffer an oversized entry, the octal validation is strict and rejects non-numeric/negative fields, and the override precedence (pax > GNU longlink/longname > header) plus escape validation for overridden symlink targets is correct. No AGENTS.md issues apply: no i18n, RTL, SQL, API-envelope, auth, locale-filtering, or changeset concerns touch this private infrastructure worker; nothing here is on a logged-out hot path.
The R2 instrumentation change remains a minor scope note — it is a one-character type-only fix in a different file than the untar changes, though still within the same private worker package and needed to keep tsc --noEmit green. Given it is transparent in the description and does not change runtime behavior, it does not change my sign-off.
Approving with no blocking findings.
github-actions
Bot
added
review/approved
What does this PR do?
Closes the two remaining findings from the review of #2386 on the flue-review untar parser:
sizefield unconditionally, so a malformed or hostile header could trigger an unbounded allocation. Entries larger than 64 MiB (or with a non-numeric/negative size field) are now rejected.linkpathsupport. Only the paxpathkeyword was parsed, so a symlink whose target exceeds the 100-byte ustarlinknamefield would have been silently truncated. The parser now honors paxlinkpathand the GNUKlong-linkname record, and applies the same escape validation to overridden targets.Also fixes a one-line
noUncheckedIndexedAccessviolation in the R2 instrumentation that surfaced when runningtsc --noEmitagainst freshly generated worker types.Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.Changeset and i18n are n/a: this touches only the private
infra/flue-reviewworker, no published package or admin UI. Typecheck was run forinfra/flue-review(tsc --noEmit); tests are the flue-review vitest suite (49 passing, including new cases for the size cap, paxlinkpathoverride, escapinglinkpathtargets, and GNUKrecords).AI-generated code disclosure
Screenshots / test output
Try this PR
Open a fresh playground →
A full working EmDash site, deployed from this branch. Each visit gets its own session-scoped sandbox: no login needed and no shared state. Try the admin, edit content, hit the public site.
Tracks
fix/flue-review-untar-size-cap-linkpath. Updated automatically when the playground redeploys.