Skip to content

fix(server): signed PDF links open only the linked file - #18239

Open
only21mil wants to merge 1 commit into
pingdotgg:mainfrom
only21mil:fix/pdf-asset-links-exact-file
Open

only21mil wants to merge 1 commit into
pingdotgg:mainfrom
only21mil:fix/pdf-asset-links-exact-file

Conversation

@only21mil

Copy link
Copy Markdown
Contributor

Problem

Signed asset links for workspace file previews in apps/server/src/assets/AssetAccess.ts gave PDFs the folder-wide scope meant for HTML previews. A link opened or shared for one PDF was not limited to that file. As reported in #9228, its scope included other preview files beside it and in nested folders. A PDF link should grant access to the linked file and nothing else.

Change

The minting rule now gives folder scope only to HTML previews. PDFs and other supported non-HTML previews receive exact-file claims. HTML keeps folder scope because pages need their relative styles, scripts and images. The existing image-dimension inspection still runs before claim creation.

New tests in apps/server/src/assets/AssetAccess.test.ts check that a PDF link opens only its own file. They also check that an HTML page ending in .html, .htm, .HTML or .HTM still loads its relative styles, scripts and images.

Links minted before this change keep their old scope until they expire; new non-HTML links get the exact-file scope.

Scope and approval

Fixes #9228. This rebuilds closed #9260 on current main, following @shivamhwp's direction to preserve image-dimension inspection and keep folder scope only for HTML. Credit to @404khai for the same approach in #9260.

This qualifies for the small, focused obvious-bug exception. It corrects the scope of an existing file-preview grant with one claim-selection change and regression tests. PDF links still open their own file, and HTML and image previews behave as before.

Verification

  • vp test run apps/server/src/assets/AssetAccess.test.ts: 43 tests passed. With the old source and new tests, 2 PDF tests failed and 41 passed; restoring the fix made all 43 pass.
  • Mutation checks: the old image-only rule makes both PDF tests fail; making every preview exact makes the HTML tests fail. Dropping .htm and matching extensions case-sensitively each make 2 of the 4 HTML cases fail.
  • vp test run apps/server/src/assets apps/server/src/http.test.ts apps/server/src/auth/RpcAuthorization.test.ts: 110 tests passed across 6 files.
  • Server vp run typecheck: exit 0, no errors.
  • Focused lint: no errors, one pre-existing warning. Format check passed. Both knip checks for exports and files/dependencies passed with no findings.

An independent reviewer ran the server asset, HTTP and auth suites plus related web, mobile, client-runtime and shared suites: 29 test files and 509 tests passed. That run used the same AssetAccess.ts, before the HTML test was split into four cases. Manual browser and mobile checks were not run.

The screenshot shows the rule before and after the fix and the red/green test results.

The minting rule and the new test results, before and after this change

Made with Claude Opus 5.5 (code, tests and PR text review, Claude Code harness) and GPT-6.1 Sol (code reviews, PR text and screenshot, Codex harness), all in T3 Code.

A signed URL for a workspace PDF carried a folder-scoped claim, so the
same token also served other preview files in the PDF's folder and its
subfolders until it expired.

Only HTML previews keep the folder claim, since a page loads its
relative styles, scripts and images. PDFs and every other workspace
preview now get an exact-file claim.

Refs pingdotgg#9228

Co-authored-by: 404khai <160283456+404khai@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:XS 0-9 changed lines (additions + deletions). labels Oct 11, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This focused fix changes the access scope of signed workspace-file URLs, narrowing new PDF and non-HTML links from folder-wide access to the linked file while preserving HTML relative assets. Because this is an access-control boundary change with security implications, it requires human review.

Notes:

  • Diff unchanged. Approvability was decided on eligibility alone.

You can add or adjust custom eligibility rules. Learn more.

@juliusmarminge juliusmarminge added the macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews label Oct 11, 2026 — with ChatGPT Codex Connector
@coderabbitai

coderabbitai Bot commented Oct 11, 2026

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: c75ecc64-d3ce-44de-93f0-ac22efd5aca4

📥 Commits

Reviewing files that changed from the base of the PR and between ed0f7b9 and 4cf2d93.


📒 Files selected for processing (2)
  • apps/server/src/assets/AssetAccess.test.ts
  • apps/server/src/assets/AssetAccess.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.



📝 Walkthrough

Walkthrough

Workspace preview claims now use folder scope for HTML previews and exact-file scope for all non-HTML previews. Tests cover PDF access at the workspace root and in nested directories, plus HTML entry files and relative resources.

Changes

Workspace preview asset scope

Layer / File(s) Summary
Set and verify workspace preview claim scope
apps/server/src/assets/AssetAccess.ts, apps/server/src/assets/AssetAccess.test.ts
finalizeWorkspaceFileAsset grants folder-scoped claims only to HTML previews. Tests check that PDF links do not resolve sibling or nested PDFs, and that HTML entry files resolve relative assets while rejecting traversal and dot-segment paths.

Priority: ⬆️ High

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix · Severity of issue fixed: High

Suggested reviewers: juliusmarminge


Merge Risk | ⚪ Minimal · up to 4cf2d

Merge Risk: ⚪ Minimal · up to 4cf2d

Signed PDF links now open only the linked file, while HTML previews still load their relative assets. No merge-blocking risk was identified. Manual browser and mobile checks were not run.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 4cf2d

New non-HTML links have narrower permissions, while HTML pages retain access to relative assets. Existing links retain their previous permissions until expiry. Exact-file links authorize a workspace path, not an immutable snapshot of its contents.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • observed — A holder of a newly issued non-HTML bearer link can request only the claimed workspace pathname through the exact resolver. Added tests assert that PDF siblings and nested PDFs are denied for both root-level and nested linked PDFs. HTML retains folder-relative access to supported assets, bounded by canonical workspace-root checks.

Trust Boundaries and Controls

  • observed — The request suffix does not confer authority: signature and expiry checks establish the bearer capability, then exact resolution requires the claimed basename. Folder resolution rejects dot, parent, hidden, NUL-containing, and unsupported-extension paths; canonical resolution rejects targets outside the workspace root or targets that are not regular files.

Resilience and Maintainability Implications

  • inferred — Repeated resolution reuses the same stateless signed authority and checks expiry on each request. Path replacement and the interval between canonical validation and HTTP file opening remain inherited limitations, not new exposure introduced by the narrower minting rule. Stable-content binding and concurrent replacement behavior are not established by the available contract or added tests.

Pre-merge checks | Passed 6
✅ Passed checks (6 passed)
Check name Status Explanation
Description check Passed The description includes all required sections, explains the problem and change, links the issue and approval context, and documents focused verification results and limitations.
Linked Issues check Passed The pull request links issue #9228, explains the matching scope problem, and documents related approval context from pull request #9260.
Out of Scope Changes check Passed The changes are focused on claim selection in AssetAccess.ts and targeted regression tests. The reported scope matches the stated bug fix.
Title check Passed The title clearly and concisely identifies the main change: signed PDF links now open only the linked file.
Linked Issues check Passed The pull request addresses the coding requirements in open issue #9228. AssetAccess.ts now sets exactFile when filePreviewKind({ name: resolved.relativePath }) !== "html", so PDFs and other non-…
Out of Scope Changes check Passed The changed source code and tests directly support issue #9228. The source change is limited to preview claim selection. The added tests cover the PDF regression and preserve the required HTML behavio…

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews size:XS 0-9 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Signed asset links for PDFs are scoped to the parent directory, exposing every sibling PDF

2 participants