fix(web): normalize PDF attachment filenames safely

[vincentkoc]

Additional instructions

MUST: Keep Allow edits from maintainers enabled for this PR so maintainers
can help update the branch when needed.

What Problem This Solves

Fixes a double-escaping path in the bundled PDF worker reported by CodeQL alert 8. The path came from pdfjs-dist@6.2.108, including its current upstream source.

Why This Change Was Made

There is no newer pdfjs-dist release carrying a fix. A pinned pnpm patch replaces the dependency's ordered replacement chain with one explicit filename-normalization scan, preserving the upstream contract while removing the vulnerable pattern. Both modern and legacy worker builds are patched, and checked-in web assets are regenerated from that dependency.

User Impact

PDF attachment filenames keep the same slash-normalization behavior without passing through a double-escaping replacement chain.

Evidence

• Linux node v26.7.0 with pnpm 11.20.0
• pnpm install --frozen-lockfile
• TypeScript formatting and lint
• FakeCo AWS tests
• Root and workspace typechecks
• Two consecutive pnpm build runs with byte-identical embedded web assets
• node --check apps/api/internal/webassets/dist/_app/immutable/assets/pdf.worker.D3ahraH8.mjs
• Full multi-stage docker build after copying the pnpm patch into the dependency-install stage
• Docs site build
• Exhaustive equivalence check over 9,841 input strings composed of slash, backslash, and ordinary characters through length 8
• Production/dependency artifact LOC: +92/-16, including Docker patch availability, the pinned dependency patch, lock metadata, and 99%-similar generated asset renames. The positive delta records the dependency contract and removes the security finding without suppressing it.

Coordination

PR #169's web source changes remain separate and retain their contributor ownership. This PR regenerates assets from current main; it does not copy or supersede that source work.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

[clawsweeper]

Codex review: needs maintainer review before merge. Reviewed August 28, 2026, 5:07 PM ET / 21:07 UTC.

ClawSweeper review

What this changes

The PR replaces chained PDF.js worker slash substitutions with a single filename-normalization scan, adds the pnpm patch to Docker dependency setup, and regenerates embedded web assets.

Merge readiness

⚠️ Ready for maintainer review - 4 items remain

Current main still has the vulnerable PDF.js filename-normalization chain. This MEMBER-authored PR remains relevant, but its draft head is dirty after the web-toolchain refresh and needs a rebased, regenerated artifact plus maintainer approval of the local dependency patch strategy.

Priority: P1
Reviewed head: 546754c38f30c25d8e441cf91181a5930f27bf6a
Owner decision: Required. See Decision needed.

Review scores

Measure	Result	What it means
Overall readiness	🦐 gold shrimp (3/6)	The repair is focused and documented, but the dirty head must be refreshed and security-checked against the current web toolchain.
Proof confidence	🐚 platinum hermit (4/6)	Not applicable: This MEMBER-authored PR is exempt from the external-contributor proof gate. Its body records build, Docker, CodeQL, and equivalence checks, but the exact rebased worker still needs verification because this head is not mergeable.
Patch quality	🦐 gold shrimp (3/6)	Security review found an item that needs attention.

Verification

Check	Result	Evidence
Real behavior	Not applicable	Not applicable: This MEMBER-authored PR is exempt from the external-contributor proof gate. Its body records build, Docker, CodeQL, and equivalence checks, but the exact rebased worker still needs verification because this head is not mergeable.
Evidence reviewed	5 items	Production dependency boundary: The artifact viewer imports pdfjs-dist and assigns its bundled build/pdf.worker.mjs URL as the PDF worker, establishing that the patch affects PDF viewing in production. Introduced remediation: The PR’s locked pnpm patch replaces the three ordered filename replacements in both PDF.js worker variants with one explicit scan; the lockfile records the patched dependency. Current-main behavior: Current main still resolves unpatched pdfjs-dist 6.2.108 and its embedded worker retains the chained replacement expression, so the requested remediation is not already implemented.
Findings	None	None.
Security	Needs attention	Revalidate the local PDF.js patch after rebase: The patch changes handling of untrusted FileSpec filename metadata, while the current PR head is dirty after main regenerated the web bundle; verify the exact rebased artifact and CodeQL result before merging.

How this fits together

ClickClack’s web artifact viewer loads PDF.js to read PDF attachments, then the built web bundle is embedded in the Go API and served to browser clients. The changed worker converts PDF file-spec metadata into attachment names used by the viewer.

flowchart LR
  PDF[PDF attachment] --> Worker[PDF.js worker]
  Worker --> Normalize[Filename normalization]
  Normalize --> Viewer[Web artifact viewer]
  Viewer --> Bundle[Embedded web assets]
  Bundle --> API[Go API]
  API --> Browser[Browser client]

Loading

Decision needed

Question	Recommendation
Should ClickClack carry this temporary local PDF.js patch for the CodeQL-reported filename path after it is rebased onto current main?	Accept a rebased temporary patch: Rebase, regenerate assets, and verify CodeQL and the container build on the resulting head while retaining the patch until an upstream release resolves it.

Why: Choosing to maintain a local override for an upstream dependency that parses untrusted PDF metadata is a repository security-policy decision.

Before merge

• Resolve security concern: Revalidate the local PDF.js patch after rebase - The patch changes handling of untrusted FileSpec filename metadata, while the current PR head is dirty after main regenerated the web bundle; verify the exact rebased artifact and CodeQL result before merging.
• Resolve merge risk (P1) - The draft head is dirty against current main, so it cannot land until the patch is rebased and assets are regenerated from the refreshed toolchain.
• Resolve merge risk (P1) - The local patch changes parsing of untrusted PDF metadata; CodeQL and container-build verification should be repeated on the exact mergeable head.

Findings

• [medium] Revalidate the local PDF.js patch after rebase — patches/pdfjs-dist@6.2.108.patch:1

Agent review details

Security

Needs attention: This is a targeted security remediation, but the locally patched PDF.js worker must be reviewed and verified on a rebased mergeable head.

Review metrics

Metric	Value	Why it matters
Patch surface	13 files, +92/-16	The behavioral change is a 52-line dependency patch; the remaining changes primarily update generated web-asset references and patch installation metadata.

Merge-risk options

Maintainer options:

1. Rebase and verify the patched dependency (recommended)
   Resolve the dirty merge, regenerate embedded assets from current main, and rerun CodeQL and the Docker build on the exact updated head.
2. Pause for remediation direction
   Do not merge if maintainers prefer an upstream release or another mitigation to a locally maintained PDF.js patch.

Technical review

Best possible solution:

Carry a narrowly documented temporary PDF.js patch only after rebasing it onto current main, regenerating the embedded assets, and verifying the exact artifact and Docker build.

Do we have a high-confidence way to reproduce the issue?

Yes, at source level: current main’s production PDF worker retains the three-stage replacement chain and the web viewer imports that worker. The reviewed snapshot does not include a malicious-PDF runtime trace.

Is this the best way to solve the issue?

Unclear pending maintainer direction: this is a narrow remediation, but permanently carrying a local security patch to PDF.js requires explicit repository approval and a current-main rebase.

AGENTS.md: found, but no applicable review policy affected this item.

Codex review notes: model internal, reasoning high; reviewed against 486fd23545af.

Labels

Label changes:

• add merge-risk: 🚨 compatibility: The PR changes bundled PDF.js filename normalization and must preserve existing attachment-name behavior after rebase.
• add merge-risk: 🚨 security-boundary: The patched worker consumes metadata from untrusted PDF files and needs verification on the exact rebased artifact.
• add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
• add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: This MEMBER-authored PR is exempt from the external-contributor proof gate. Its body records build, Docker, CodeQL, and equivalence checks, but the exact rebased worker still needs verification because this head is not mergeable.
• remove rating: 🐚 platinum hermit: Current PR rating is rating: 🦐 gold shrimp, so this older rating label is no longer current.
• remove status: 👀 ready for maintainer look: Current PR status label is status: ⏳ waiting on author.

Label justifications:

• P1: This is an open remediation for a CodeQL-reported path that processes untrusted PDF attachment metadata.
• merge-risk: 🚨 compatibility: The PR changes bundled PDF.js filename normalization and must preserve existing attachment-name behavior after rebase.
• merge-risk: 🚨 security-boundary: The patched worker consumes metadata from untrusted PDF files and needs verification on the exact rebased artifact.
• rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
• status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: This MEMBER-authored PR is exempt from the external-contributor proof gate. Its body records build, Docker, CodeQL, and equivalence checks, but the exact rebased worker still needs verification because this head is not mergeable.

Evidence

Security concerns:

• [medium] Revalidate the local PDF.js patch after rebase — patches/pdfjs-dist@6.2.108.patch:1
  The patch changes handling of untrusted FileSpec filename metadata, while the current PR head is dirty after main regenerated the web bundle; verify the exact rebased artifact and CodeQL result before merging.
  Confidence: 0.96

What I checked:

• Production dependency boundary: The artifact viewer imports pdfjs-dist and assigns its bundled build/pdf.worker.mjs URL as the PDF worker, establishing that the patch affects PDF viewing in production. (apps/web/src/components/artifacts/ArtifactViewer.svelte:34, 546754c38f30)
• Introduced remediation: The PR’s locked pnpm patch replaces the three ordered filename replacements in both PDF.js worker variants with one explicit scan; the lockfile records the patched dependency. (patches/pdfjs-dist@6.2.108.patch:1, 546754c38f30)
• Current-main behavior: Current main still resolves unpatched pdfjs-dist 6.2.108 and its embedded worker retains the chained replacement expression, so the requested remediation is not already implemented. (apps/api/internal/webassets/dist/_app/immutable/assets/pdf.worker.CLesOks4.mjs:38742, 486fd23545af)
• Main-branch refresh: The current-main dependency refresh regenerated the embedded web assets and changed the Docker/web toolchain after this PR head; GitHub reports this head dirty and no pinned test merge is available. (Dockerfile:5, 486fd23545af)
• Area provenance: The PDF worker entrypoint appears to date to the split frontend/API implementation, and Peter Steinberger also authored the current main toolchain refresh relevant to the rebase. (apps/web/src/components/artifacts/ArtifactViewer.svelte:34, 04402c823df1)

Likely related people:

• Peter Steinberger: He introduced the current PDF viewer integration and authored the current-main dependency/toolchain refresh that regenerated the assets this patch must now integrate with. (role: feature and recent area contributor; confidence: high; commits: 04402c823df1, 486fd23545af; files: apps/web/src/components/artifacts/ArtifactViewer.svelte, Dockerfile, apps/api/internal/webassets/dist)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Rebase onto current main and regenerate the embedded web assets.
• Record CodeQL and Docker build results for the exact rebased worker artifact, redacting private endpoints and credentials.

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (2 earlier review cycles)

• reviewed 2026-08-21T11:13:23.950Z sha ab830ae :: needs maintainer review before merge. :: none
• reviewed 2026-08-21T15:23:12.710Z sha 546754c :: needs maintainer review before merge. :: none