Skip to content

fix(files): open a remote PC's PDF from Android's Open button - #448

Merged
devswha merged 7 commits into
devswha:mainfrom
nahwan-kim:fix/remote-pdf-open
Oct 5, 2026
Merged

devswha merged 7 commits into
devswha:mainfrom
nahwan-kim:fix/remote-pdf-open

Conversation

@nahwan-kim

@nahwan-kim nahwan-kim commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Change

On Android, the file viewer's <iframe> cannot draw a PDF, so Chrome shows an Open button in its place. That button's navigation is sent with Sec-Fetch-Site: cross-site (Chrome treats the fallback UI as another origin, see https://issues.chromium.org/issues/374094200).

For a file on the app's own PC this works: GET /api/fs/file has no origin check. For a file on a remote PC the URL is /api/machines/<id>/fs/file, and handleMachineRequest refused every request that fails sameOrigin(), so the phone showed {"error":{"code":"invalid_origin","message":"Use PC controls from this app"}} instead of the PDF.

handleMachineRequest now lets GET/HEAD of /api/machines/<id>/fs/file through without the origin check, the same rule as the local /api/fs/file. Nothing else changes:

  • authentication (decideAccess) and the read-only device refusal for file reads in index.ts still run first;
  • every other PC route, and any other method on fs/file, still answers invalid_origin from another origin;
  • the proxy still forwards only its bridge bearer, never browser cookies or Authorization;
  • //, .. and encoded slashes do not match the exemption ([^/]+ for the id, exact fs/file tail), and // paths still 404 as before.

Validation

  • Reproduced on a running v0.3.49 server: /api/machines/<remote>/fs/stat and fs/file answer 403 invalid_origin with sec-fetch-site: cross-site or same-site, 200 with same-origin, none or no header; the local /api/fs/* answers 200 for all of them.
  • server/machine-proxy.test.ts: a cross-site and a same-site GET of a PC's fs/file stream the file, and a cross-site HEAD answers 200; a cross-site fs/stat, pane/conversation, POST fs/file (with x-herdr-machine: 1) and a foreign-origin PUT still answer 403 invalid_origin. The new test fails on main (403) and passes with the change.
  • bun run generate:types --check, bun run typecheck, bun run build pass. bun run test:unit on macOS has the same 14 failures with and without this change (Codex resume selection and pi store path tests, platform-specific); none touch this code.

@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration
  • Configuration used: Repository: devswha/herdr-web-ui/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: d1ec2005-cadc-43d0-9fa4-d04ebec44fed
📥 Commits

Reviewing files that changed from the base of the PR and between 484dfda and 15f94e8.

📒 Files selected for processing (2)
  • CHANGELOG.md
  • server/device-access.contract.test.ts
 ______________________________________________________________
< Ad Astra Per Codicem Fixis. To the stars through code fixes. >
 --------------------------------------------------------------
  \
   \   \
        \ /\
        ( )
      .( o ).

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: devswha/herdr-web-ui/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 1bccf274-22c4-4b28-befe-49bde9af5610
📥 Commits

Reviewing files that changed from the base of the PR and between e67c7cc and 484dfda.

📒 Files selected for processing (2)
  • CHANGELOG.md
  • server/machine-proxy.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • CHANGELOG.md

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


📝 Summary

Summary by CodeRabbit

  • Bug Fixes
    • Fixed an issue on Android where opening a PDF stored on a remote PC from the file viewer displayed an Open button but then failed with an invalid_origin error. Remote-PC PDFs now open as expected, while PDFs stored on the app’s own PC continue to work. Other file-viewer behavior is unchanged.

Walkthrough

The machine API allows GET and HEAD requests to matching machine file paths to bypass the same-origin check. Tests cover successful PDF reads and requests that still return invalid_origin.

Changes

Remote machine PDF access

Layer / File(s) Summary
File-read origin check
server/machine-api.ts, server/machine-proxy.test.ts, CHANGELOG.md
Matching file GET and HEAD requests bypass the same-origin check. Tests cover successful PDF reads and rejected cross-origin requests. The changelog records the Android remote-PC PDF issue.

Priority: ⬇️ Low

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

Change: Bug fix · Severity of issue fixed: Low

Suggested reviewers: devswha

Merge Risk: ⚪ Minimal · up to 484df

The change enables the intended remote-file navigation while preserving authentication and read-only restrictions. No material merge-blocking issue is established.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 484df

The exception is limited to file reads, and authentication, read-only device restrictions, and bridge credential isolation remain enforced. It covers all readable remote files, not only PDFs. No unauthorized disclosure was established, but practical exposure depends on deployment authentication and reverse-proxy configuration that was not available.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The maximum newly origin-exempt scope is file retrieval from each connected remote machine addressable through the connection server, bounded by that remote process's filesystem permissions rather than by a PDF or workspace restriction. Reaching this scope still requires outer full access; watch-role devices are denied. No write or terminal-control route receives the exception.

Security Findings and Attack Paths

  • inferred — An attacker-controlled origin can now initiate a remote-file request that the base origin gate rejected if the request independently satisfies outer access. Navigation or embedding may render or download the response, but attacker JavaScript reading protected contents was not established: the proxy does not pass CORS grants, and browser-origin isolation remains relevant. The canonical security assessment contains no retained findings; deployed proxy behavior remains unverified.

Trust Boundaries and Controls

  • observed — Authentication and watch-role filesystem refusal precede machine dispatch. A bridge bearer alone is not an alternate authentication path for the machine-management route, and the local-machine alias retains its separate origin restriction.
  • observed — Token and device cookies use HttpOnly and SameSite=Strict, limiting ordinary cross-site credential delivery. However, unchanged local, Tailscale-owner, and open access decisions can grant full authority without cookies. The new exception therefore relies on those access decisions and browser response isolation rather than solely on cookie policy.

Hardening Proposals

  • proposed — Add dispatcher-level regression coverage proving that cross-origin file requests still reject unauthenticated and watch-role callers, and validate that supported reverse proxies do not add permissive credentialed CORS. The added handler-level tests do not exercise those outer guarantees.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main change: opening a remote PC’s PDF from Android.
Description check ✅ Passed The description explains the problem and resulting behavior under Change. Under Validation, it lists contract-test coverage and relevant check results, including the reported unit-test failures.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@nahwan-kim

Copy link
Copy Markdown
Contributor Author

Merged the latest main (CHANGELOG: both entries kept under [Unreleased]) and added a cross-site HEAD assertion to the proxy test, since the exemption covers HEAD too. No behavior change.

devswha and others added 3 commits October 5, 2026 15:38
…rigin exemption

Review follow-up on devswha#448 (Codex adversarial review): the PR's test calls
handleMachineRequest directly, so nothing showed that the read-only refusal in
index.ts still runs ahead of the new origin exemption for GET and HEAD of
/api/machines/<id>/fs/file. The device-access contract test now asserts it for
both methods with a cross-site header: 403 read_only.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@devswha
devswha merged commit f731585 into devswha:main Oct 5, 2026
11 of 12 checks passed
@devswha

devswha commented Oct 5, 2026

Copy link
Copy Markdown
Owner

Merged, thank you. Before merging I had a Codex security pass look at the exemption: it confirmed the watch-device refusal and the // rule still run first (I added a contract test for that), and noted one low-severity point: the exemption keys on method and path only, so with a cookie-less identity (tokenless loopback or Tailscale owner) a third-party page could also embed the file URL as an image and learn whether it decodes. That is the same exposure the local /api/fs/file already has, so it did not block the merge. If you still have the device handy: what Sec-Fetch-Mode and Sec-Fetch-Dest does Chrome send on the Open button's request? If it is navigate, a follow-up could narrow the exemption to navigations.

@nahwan-kim

Copy link
Copy Markdown
Contributor Author

Measured on the device: Chrome 154 on Android (Galaxy Z Fold), on a fresh HTTPS origin over Tailscale Serve, with a test page holding the same <iframe src=…pdf> as the file viewer and the same response headers as fileResponse (inline, nosniff, private, no-store, no CSP):

Request Sec-Fetch-Site Sec-Fetch-Mode Sec-Fetch-Dest Sec-Fetch-User Referer
the iframe's own load (Chrome then shows the Open button) same-origin navigate iframe – the page
tapping Open cross-site navigate document ?1 none

So the request #448 lets through is a top-level, user-activated navigation. Narrowing the exemption to Sec-Fetch-Mode: navigate (or Sec-Fetch-Dest: document) would still open it while refusing a cross-site <img>/fetch embed (no-cors/cors, dest image/empty). A request without Sec-Fetch headers already passes sameOrigin() when it has no Origin, so that case would not change. Happy to send that follow-up if you want it.

(A first run on a port where the phone still had an old service worker registered showed Sec-Fetch-Dest: empty for these navigations; the table above is from a clean origin.)

devswha added a commit that referenced this pull request Oct 5, 2026
Bumps package.json and herdr-plugin.toml to 0.3.50 and moves the
Unreleased notes under [0.3.50] - 2026-10-06, with the compare link.
Adds the PR link and credit the #471 entry was missing; every other
entry already carries them. Raises the remote PC bundle to 17: #470
changes the prompt answer route on the bridge, so a remote PC on bundle
16 keeps the old behaviour until its bridge is updated. Push the
`remote-v17` tag and let the bundle workflow publish before running
release.yml.

Known limits carried into the notes: Japanese and Chinese text uses the
device's fonts beside Pretendard; #469 stays open for what #470 does not
cover (answers after the first committing key, identical twin prompts,
screens no reader knows).

Carries #422 #423 #424 #425 #426 #435 #437 #439 #446 #447 #448 #452 #454
#455 #456 #457 #458 #459 #460 #461 #462 #463 #465 #466 #467 #468 #471
#473 #474 #475

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants