Skip to content

fix(flue-review): cap tar entry size and honor pax linkpath overrides #2388

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ascorbic merged 3 commits into from
Aug 9, 2026
Merged

fix(flue-review): cap tar entry size and honor pax linkpath overrides #2388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e526328
fix(flue-review): cap tar entry size and honor pax linkpath overrides
ascorbic Aug 8, 2026
56801a6
fix(flue-review): satisfy noUncheckedIndexedAccess in R2 instrumentation
ascorbic Aug 8, 2026
5347166
fix(flue-review): require a strictly octal tar size field
ascorbic Aug 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 29 additions & 8 deletions infra/flue-review/.flue/lib/untar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ export interface UntarTarget {
writeFileBytes(path: string, content: Uint8Array): Promise<void>;
}

/** Ceiling on a single entry's declared size; content is buffered in DO memory. */
const MAX_ENTRY_SIZE = 64 * 1024 * 1024;

const OCTAL_FIELD = /^[0-7]*$/;

function stripRoot(name: string): string | undefined {
const slash = name.indexOf("/");
if (slash === -1) return undefined;
Expand Down Expand Up @@ -38,9 +43,9 @@ function normalizeSegments(path: string): string[] | undefined {
/**
* Untar a ustar stream into `destDir`, stripping the archive's single
* top-level directory. Handles regular files, directories, symlinks, GNU
* longname ('L') and pax ('x') path overrides. One entry's content is
* buffered at a time. Rejects any entry path or symlink target that would
* resolve outside `destDir`.
* longname/longlink ('L'/'K') and pax ('x') path/linkpath overrides. One
* entry's content is buffered at a time, capped at MAX_ENTRY_SIZE. Rejects
* any entry path or symlink target that would resolve outside `destDir`.
*/
export async function untarInto(
target: UntarTarget,
Expand All @@ -52,7 +57,9 @@ export async function untarInto(
let files = 0;
let bytes = 0;
let pendingLongName: string | undefined;
let pendingLongLink: string | undefined;
let pendingPaxPath: string | undefined;
let pendingPaxLink: string | undefined;
const dirsMade = new Set<string>();

const append = (chunk: Uint8Array) => {
Expand Down Expand Up @@ -90,12 +97,18 @@ export async function untarInto(

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);
const sizeField = readCString(header.subarray(124, 136)).trim();
if (!OCTAL_FIELD.test(sizeField)) {
throw new Error(`tar entry size is not octal ("${sizeField}"): ${rawName}`);
}
const size = sizeField ? parseInt(sizeField, 8) : 0;
// Mode bytes (100-108) are ignored: the workspace has no chmod and the
// reviewer never executes files.
const type = String.fromCharCode(header[156] ?? 48);
const linkTarget = readCString(header.subarray(157, 257));

if (size > MAX_ENTRY_SIZE) {
throw new Error(`tar entry size out of range (${size} bytes): ${rawName}`);
}
const padded = Math.ceil(size / 512) * 512;
if (!(await need(padded)) && size > 0) {
throw new Error(`tar truncated: needed ${padded} bytes for entry ${rawName}`);
Expand All @@ -107,22 +120,30 @@ export async function untarInto(
pendingLongName = readCString(content);
continue;
}
if (type === "K") {
pendingLongLink = readCString(content);
continue;
}
if (type === "x" || type === "g") {
// pax records: "<len> key=value\n"
const text = decoder.decode(content);
for (const line of text.split("\n")) {
const eq = line.indexOf("=");
if (eq > 0 && line.slice(line.indexOf(" ") + 1, eq) === "path") {
pendingPaxPath = line.slice(eq + 1);
}
if (eq <= 0) continue;
const key = line.slice(line.indexOf(" ") + 1, eq);
if (key === "path") pendingPaxPath = line.slice(eq + 1);
else if (key === "linkpath") pendingPaxLink = line.slice(eq + 1);
}
continue;
}

const fullName =
pendingPaxPath ?? pendingLongName ?? (prefix ? `${prefix}/${rawName}` : rawName);
const linkTarget = pendingPaxLink ?? pendingLongLink ?? readCString(header.subarray(157, 257));
pendingLongName = undefined;
pendingLongLink = undefined;
pendingPaxPath = undefined;
pendingPaxLink = undefined;

if (fullName.startsWith("/")) {
throw new Error(`tar entry path is absolute: ${fullName}`);
Expand Down
2 changes: 1 addition & 1 deletion infra/flue-review/.flue/sandboxes/cloudflare-shell.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ function instrumentR2(bucket: R2Bucket): R2Bucket {
console.log(JSON.stringify({ message: "r2 op start", id, method, key }));
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);

@emdashbot emdashbot Bot Aug 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

console.log(JSON.stringify({ message: "r2 op end", id, method, key, ms: Date.now() - t0 }));
return result;
} catch (error) {
Expand Down
66 changes: 66 additions & 0 deletions infra/flue-review/test/untar.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,72 @@ describe("untarInto", () => {
expect(r.files.has("/repo/ignored-name.txt")).toBe(false);
});

it("applies pax linkpath and GNU longlink overrides to symlink targets", async () => {
const longTarget = `deep/${"d/".repeat(40)}target.txt`;
const paxBody = `linkpath=${longTarget}\n`;
const paxLen = String(paxBody.length + 3).length + 1 + paxBody.length;
const paxBytes = encoder.encode(`${paxLen} ${paxBody}`);
const longLinkBytes = encoder.encode(longTarget);
const r = recorder();
await untarInto(
r.target,
tarball([
header({ name: "repo-abc/PaxHeader", type: "x", size: paxBytes.length }),
contentBlocks(paxBytes),
header({ name: "repo-abc/pax-link", type: "2", linkTarget: "short" }),
header({ name: "repo-abc/@LongLink", type: "K", size: longLinkBytes.length }),
contentBlocks(longLinkBytes),
header({ name: "repo-abc/gnu-link", type: "2", linkTarget: "short" }),
header({ name: "repo-abc/plain-link", type: "2", linkTarget: "short" }),
]),
"/repo",
);
expect(r.symlinks.get("/repo/pax-link")).toBe(longTarget);
expect(r.symlinks.get("/repo/gnu-link")).toBe(longTarget);
expect(r.symlinks.get("/repo/plain-link")).toBe("short");
});

it("rejects pax linkpath targets that escape the destination", async () => {
const paxBody = "linkpath=../../etc/passwd\n";
const paxLen = String(paxBody.length + 3).length + 1 + paxBody.length;
const paxBytes = encoder.encode(`${paxLen} ${paxBody}`);
const r = recorder();
await expect(
untarInto(
r.target,
tarball([
header({ name: "repo-abc/PaxHeader", type: "x", size: paxBytes.length }),
contentBlocks(paxBytes),
header({ name: "repo-abc/evil", type: "2", linkTarget: "harmless" }),
]),
"/repo",
),
).rejects.toThrow(/escapes the destination/);
expect(r.symlinks.size).toBe(0);
});

it("rejects entries whose size field is not strictly octal", async () => {
for (const sizeField of ["10x", "size!", "-0000001"]) {
const block = header({ name: "repo-abc/bad.bin" });
block.set(encoder.encode(sizeField), 124);
const r = recorder();
await expect(untarInto(r.target, tarball([block]), "/repo")).rejects.toThrow(/not octal/);
expect(r.files.size).toBe(0);
}
});

it("rejects entries whose declared size exceeds the cap", async () => {
const r = recorder();
await expect(
untarInto(
r.target,
tarball([header({ name: "repo-abc/huge.bin", size: 128 * 1024 * 1024 })]),
"/repo",
),
).rejects.toThrow(/size out of range/);
expect(r.files.size).toBe(0);
});

@emdashbot emdashbot Bot Aug 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change
});
});
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");
});


it("joins the ustar prefix field with the name", async () => {
const r = recorder();
await untarInto(
Expand Down
Loading