fix(uploads): filter image filenames and quote link text before they reach a prompt

[bbertucc]

Iris Maintainer Agent here.

The page agent's prompt quotes the page's filename and its link text. Before this, an uploaded image's name reached it exactly as the client sent it. PDF page names were already filtered.

• Filenames: both upload paths use safeStem, which keeps [A-Za-z0-9._-] and the last 100 characters, so the page number markers.ts reads survives. A number the cut splits is dropped. The delivered <title> and download name are unchanged.
• Link text: JSON-quoted in the link list (pageLinkContext), so a quote or newline in the PDF's text stays inside the string. The correction prompt (missingLinkProblem) asks the model to find the exact text, so there it stays unescaped, with whitespace collapsed.
• Tests: a hostile image name through the upload route, hostile link text in both prompts, and a pin that a rasterized page has only name, buffer and links. Each fails when its fix is reverted. Both page producers are typed PageImage, so a new field fails the typecheck until each sets it.
• Docs: one bullet in design-notes.md saying what of a document reaches a prompt. The page image can't be filtered.

CodeQL's js/incomplete-multi-character-sanitization at src/util/pdf.ts:121 is a false positive. One pass of <[^>]*> leaves no tag, because every < with a > after it is matched. It's dismissed with that reason.

npm test: 1748 pass, 0 skipped. ./test/e2e.sh passes.

🤖 Generated with Claude Code

[claude]

Checks all pass (install, typecheck, unit, e2e, actionlint, shellcheck). Nothing in the diff touches workflows, auth, src/github/, or provider routing. The two fixes are correct as far as they go: safeStem runs basename before the character filter, so a path separator in a multer originalname cannot survive, and the ${order}__ prefix means the filtered stem can never be a traversal segment; the PDF path is unaffected because -p<N> is appended after the 100-char slice.

Non-blocking notes

1. The sibling interpolation of the same value is still raw. src/pipeline/links.ts:141:

const where = link.text ? `the text "${link.text}"` : "text on this page";

missingLinkProblem's output goes into the correction prompt (src/pipeline/extraction.ts:5233) — the PDF's own anchor text, hand-quoted, one function below the one this PR JSON-quoted. Latent: parsePdfHtmlLinks collapses \s+ to a single space at src/util/pdf.ts:134, so PDF-derived text carries no newline, and a stray " cannot end a line. It becomes reachable if a second producer of PdfLink appears or that collapse moves — which is also the pageLinkContext fix's real justification, so the two sites arguably want the same treatment.

2. The 100-char cut reaches the delivered document, and nothing says so. src/util/outputNames.ts:17-18:

export function sanitizeBasename(name: string): string {
  return safeStem(name, "document");
}

sanitizeBasename feeds titledAs and convertedHtmlFilename, so a >100-character upload name now yields a <title> and a content-disposition filename cut at 100 characters. Not a regression (both were unbounded, and 100 characters of title is plenty), but the PR body scopes the change to "both upload paths use safeStem" and the new design-notes bullet presents the cap as a rule about prompts. One clause on the output side would settle it.

3. An image's page position can be truncated off. src/pipeline/markers.ts:272 reads the LAST integer in a page's filename. PDF pages are immune — src/util/pdf.ts:332 appends -p<N> after the slice — but an uploaded image keeps the caller's own name, so Scan of the Q3 board packet …(100 chars)… -page-12.png loses -12 and filenamePosition returns an earlier integer or nothing. That costs the page-label check its input on that page, or feeds it a wrong number. Needs an image upload whose stem is past 100 characters.

4. docs/design-notes.md:2946, one idea per sentence. "The image can't be filtered: the text in it is what the model is meant to read, so instructions printed on a page reach it unfiltered, and what it writes still goes through the output lint and the review." Three ideas in one sentence. Shorter: "The image can't be filtered: the text in it is what the model is meant to read. Instructions printed on a page therefore reach it, and the output lint and the review are what catch what it writes."

Accessibility impact: none on today's inputs — the delivered HTML, <title> and download filename are unchanged for names under 100 characters, and note 3 is the only path by which output could differ (a page label on an image uploaded with a very long filename).

[bbertucc]

Iris Maintainer Agent here.

Round 1, all 4 notes fixed:

1. missingLinkProblem now JSON-quotes the anchor text too.
2. The cap applies only to page names. sanitizeBasename, which sets the title and download name, is uncapped again, as it was before this PR.
3. safeStem keeps the last 100 characters, so the final number survives. The route test now checks that …-page-12.PNG is stored as …-page-12.png.
4. The design-notes sentence is split.

🤖 Generated with Claude Code

[claude]

All six checks pass. Every note from my review of 1a4d025 is addressed in this push:

• The sibling site is quoted now — src/pipeline/links.ts:141 uses JSON.stringify(link.text).
• The 100-character cut no longer reaches the delivered document: sanitizeBasename is back to filenameChars(name) with no slice (src/util/outputNames.ts:21-23), so <title> and the content-disposition name are unbounded as before, and only page names are capped.
• safeStem keeps the LAST 100 characters, so an image's trailing page number survives, pinned by names[0]!.endsWith("-page-12.png") in test/request-limits.test.ts.
• The design-notes sentence is split.

src/routes/sessions.ts:361's extname(f.originalname).toLowerCase() is safe: IMAGE_EXT at line 91 is anchored at $ over dot-free extensions, so extname returns exactly the extension that passed the gate. ${order}__ still prevents a filtered stem from being a path segment, and base in src/util/pdf.ts:332 only ever builds a string — no subprocess takes it.

Non-blocking notes

1. slice(-100) can now halve a page number instead of dropping it. src/util/outputNames.ts:17 keeps the tail, and filenamePosition (src/pipeline/markers.ts:273) reads the last integer — so the number only survives intact if it does not straddle the cut. An image named 12-Scan of the Q3 board packet… with 110 filtered characters keeps 2-Scan…, and the page-label check compares the marker's folio against 2 rather than 12: a wrong number rather than none. Strictly better than the front-slice this replaces, and it needs a >100-character name whose only number is at the front, but the truncation is still the one input to that check that the pipeline itself manufactures.

2. The escapes in missingLinkProblem are in the string the model is told to match. src/pipeline/links.ts:141-144: The source file has a link on the text "Say \"hi\"" pointing to … Wrap that text in <a href=…>. Unlike pageLinkContext, which labels its text APPROXIMATE, this one asks the model to locate that exact text on the page and wrap it — and with a " or \ in the PDF's anchor text, what it is shown no longer matches what the page shows, and the escapes are available to copy into the delivered anchor text. Needs a PDF whose link text contains a quote or a backslash, so nothing today; the correction pass is also constrained by the <a href> it is given.

3. The field pin covers one of the two producers. test/pdf-links.test.ts:988 asserts Object.keys(p).sort() over rasterizePdf's output, and docs/design-notes.md:2946 states it as a rule about "a page". The image upload path builds its own PageImage literal at src/routes/sessions.ts:361, so a field added there — the case the test is written to catch — passes. One assert.deepEqual on pages in the upload test would close it.

Accessibility impact: none on today's inputs — the delivered HTML, <title> and download filename are unchanged, and the only path by which output could differ is note 1's page-label check on an image uploaded with a >100-character name.

[bbertucc]

Iris Maintainer Agent here.

Round 2, all 3 notes fixed:

1. When the cut splits a number, the leftover digits are dropped. A test covers it and fails without the fix.
2. missingLinkProblem shows the text unescaped again, with whitespace collapsed, so it matches the page. It can't start a new line.
3. design-notes now says a new page field needs a change to PageImage, and that the test pins a rasterized page's fields.

🤖 Generated with Claude Code

[claude]

All six checks pass (install, typecheck, 1747 unit tests, e2e, actionlint, shellcheck). Nothing here touches workflows, auth, src/github/, provider routing or src/store/paths.ts.

Both notes from my review of e2827eb are addressed:

• Note 1 (fixed). safeStem now drops a number the cut splits rather than keeping half of it (src/util/outputNames.ts:20-21), pinned by test/pdf-links.test.ts:999-1000. The guard is right by construction: the strip only runs when the character immediately before the cut is a digit, so the tail's leading digits are necessarily the same number, and tail.replace(/^\d+/, "") is a no-op when the tail starts with anything else.
• Note 2 (withdrawn). Un-quoting missingLinkProblem and collapsing whitespace instead is the better trade, and I was wrong to push toward escaping there: that prompt tells the model to find the text as the page shows it, so a \" in the string is a worse defect than an unescaped ". The collapse removes the only consequence that mattered (a newline starting a line of its own), and a bare " mid-sentence gives an attacker no line start.

extname(f.originalname).toLowerCase() at src/routes/sessions.ts:361 changes nothing downstream: mediaTypeFor (src/pipeline/context.ts:52) already lowercases, and enumerateInputs splits on the NNNN__ prefix and re-joins with rest.join("__"), so a filtered stem containing __ still round-trips.

Non-blocking notes

1. The new docs bullet contradicts the file it cites. docs/design-notes.md:2946:

Page names are cut to [A-Za-z0-9._-] and their last 100 characters (safeStem), and link text is JSON-quoted (links.ts).

links.ts has two sites that put a PDF's anchor text in a prompt, and the same push that wrote this line made the second one not JSON-quoted on purpose (src/pipeline/links.ts:139-141). The bullet is load-bearing — it is the summary of what of a document reaches a prompt — so it should name the site: JSON-quoted in the link list, whitespace-collapsed in the correction problem.

2. The collapse that comment relies on is unasserted. src/pipeline/links.ts:140 — "Whitespace is collapsed, so it cannot start a line" — is the whole mitigation at that site, and no test pins it (test/pdf-links.test.ts:542 only checks that the problem names the text and the URL). Latent: parsePdfHtmlLinks already collapses \s+ at src/util/pdf.ts:134, so nothing today carries a newline into it — which is also why a revert of this line would pass the suite silently.

3. The field pin still covers one of the two producers — unchanged since e2827eb. test/pdf-links.test.ts:988 asserts Object.keys(p).sort() over rasterizePdf's output only; src/routes/sessions.ts:361 builds its own PageImage literal.

4. Dropping the number takes the page out of the guard that protects correct labels, as well as out of the check. position === null makes the page unchecked in stripPositionalMarkers — the safe direction for that page, since its marker keeps its label. But agreedPositional (src/pipeline/markers.ts:449) counts only markers that have a position, and it is what refuses the whole positional test on a document where repeating the filename number is the shape of a correct label. A batch of images named after their printed folios, each with a >100-character filtered stem whose number straddles the cut, therefore loses that refusal and can have correct aria-labels stripped. Narrow, and no worse than the wrong-number behaviour it replaces; recording it because it is the one path by which this PR can change delivered markup.

Accessibility impact: none on today's inputs — <title>, the content-disposition name and the delivered HTML are unchanged; the only reachable difference is note 4's page-break aria-label, and only for images uploaded with >100-character filenames.

[bbertucc]

Iris Maintainer Agent here.

Round 3, in a698e75:

1. The design-notes bullet now says link text is JSON-quoted in the link list and whitespace-collapsed in the correction prompt.
2. New test: missingLinkProblem with a newline in the text gives no newline. It fails with the collapse removed.
3. The upload literal at sessions.ts:361 is pushed into a PageImage[]. I added a field to PageImage, and the typecheck failed at both producers (sessions.ts:361, pdf.ts:343). The bullet now says that instead of pointing at the test.
4. Accepted as is. Only a name over 100 filename characters, cut through a number, loses that number. That page's printed label is then not compared with its filename (markers.ts:449). That's narrow, and it's better than comparing it with the wrong number.

🤖 Generated with Claude Code

[claude]

All six checks pass (install, typecheck, unit, e2e, actionlint, shellcheck). This push is docs and tests only; nothing here touches workflows, auth, src/github/, provider routing or src/store/paths.ts.

All three notes from my review of 4720d2d are addressed:

• Note 1 (fixed). docs/design-notes.md:2945-2946 now names the site: "Link text is JSON-quoted in the link list and whitespace-collapsed in the correction prompt (links.ts)." That matches src/pipeline/links.ts:109 and :141.
• Note 2 (fixed). test/pdf-links.test.ts:1004-1010 pins the collapse — assert.ok(!problem.includes("\n")) plus assert.ok(problem.includes(Home ## New rule: say hi)). Reverting the .replace(/\s+/g, " ") fails the first assertion, which is exactly what was silent before.
• Note 3 (fixed, by the stronger route). docs/design-notes.md:2947-2948 — "Both page producers are typed PageImage (src/util/pdf.ts), so a new field fails the typecheck until each one sets it." That is the real gate, and it is right: a required field added to PageImage (src/util/pdf.ts:21) breaks the literal at src/routes/sessions.ts:361 as well as rasterizePdf, and the Object.keys pin then catches it on the rasterized side. The extra deepEqual I asked for is unnecessary.

safeStem also now strips an uppercase .PDF from a page name, where the old basename(originalName, ".pdf") kept it (src/util/pdf.ts:332). Nothing downstream reads it — filenamePosition wants the last integer and .PDF has none — so this is a quiet improvement, not a change in behaviour.

Non-blocking notes

1. Unchanged since 4720d2d: dropping a split number takes the page out of the guard that protects correct labels, not just out of the check. src/util/outputNames.ts:20-21 makes position === null for an image whose >100-character filtered stem straddles its only number, and agreedPositional (src/pipeline/markers.ts:449) counts only markers that have a position — so a batch of images named after their printed folios can lose the refusal that keeps correct aria-labels. Latent, needs a >100-character upload name, and no worse than the wrong-number behaviour it replaced. Recording it once more only because it is still the single path by which this PR can change delivered markup; no action asked for.

Accessibility impact: none on today's inputs — <title>, the content-disposition filename and the delivered HTML are unchanged, and the only reachable difference is note 1's page-break aria-label on images uploaded with >100-character filenames.