fix(web): normalize PDF attachment filenames safely - #175
Draft
vincentkoc wants to merge 1 commit into
Draft
Conversation
Contributor
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
P1
Contributor
|
Codex review: needs maintainer review before merge. Reviewed August 21, 2026, 11:23 AM ET / 15:23 UTC. ClawSweeper reviewWhat this changesThe PR applies a locked pdfjs-dist patch to normalize PDF attachment filenames in one scan, updates Docker dependency installation, and regenerates embedded web assets. Merge readinessKeep open: current main still has the reported replacement chain, while this member-authored draft needs explicit maintainer review after Playwright E2E completes. Priority: P1 Review scores
Verification
Live VerificationCommand: Result: PASS (completed) Assertions:
How this fits togetherClickClack’s artifact viewer gives the browser a pdfjs-dist worker to parse PDF attachments. The web build copies that worker into Go-embedded web assets, so the dependency patch must work in both local and Docker installs. flowchart LR
A[PDF attachment] --> B[Artifact viewer]
B --> C[PDF worker]
C --> D[Filename normalization]
D --> E[Web build]
E --> F[Embedded web assets]
F --> G[Go web server]
Decision needed
Why: Repository policy keeps member-authored PRs open for explicit maintainer handling, and this changes a bundled third-party PDF worker used for attachment previews. Before merge
Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Land the narrow locked dependency patch once a maintainer confirms the alert remediation and the remaining Playwright E2E check completes, retaining regenerated worker assets as the deployable output. Do we have a high-confidence way to reproduce the issue? No live exploit reproduction was supplied; current main contains the reported ordered replacement chain, so the path is source-reproducible but was not independently replayed. Is this the best way to solve the issue? Yes. A version-locked pnpm patch plus regenerated worker assets is the narrowest maintainable remedy while no newer upstream release is identified in the PR. AGENTS.md: found, but no applicable review policy affected this item. Codex review notes: model internal, reasoning high; reviewed against e617cb5f1842. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
vincentkoc
force-pushed
the
fix/clickclack-pdfjs-filename-normalization
branch
2 times, most recently
from
August 21, 2026 15:03
54da493 to
e64f5a2
Compare
vincentkoc
force-pushed
the
fix/clickclack-pdfjs-filename-normalization
branch
from
August 21, 2026 15:16
e64f5a2 to
546754c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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-distrelease 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
node v26.7.0withpnpm 11.20.0pnpm install --frozen-lockfilepnpm buildruns with byte-identical embedded web assetsnode --check apps/api/internal/webassets/dist/_app/immutable/assets/pdf.worker.D3ahraH8.mjsdocker buildafter copying the pnpm patch into the dependency-install stageCoordination
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.