fix(annotate): block symlink escape in HTML asset serving + cleanups#927
Merged
Conversation
The /api/html-assets route and the share-payload inliner checked path containment lexically, so an in-directory symlink pointing outside the HTML's folder (e.g. evil.css -> ~/.ssh/id_rsa) passed the check and was served or base64-inlined into the share payload. In remote mode the inliner auto-fires at startup, so this could upload symlinked local files to the paste service with no user action. Resolve symlinks with realpathSync on both the asset and the root before the relative-path check, in all three runtime copies (Bun route handler, shared node inliner, Pi route handler). Non-existent assets fall back to the lexical path and 404 on read. Adds regression tests for both sinks.
Expand the Pi sandbox harness to create multiple commits, a feature branch, and a rename+delete+modify commit so /plannotator-review can exercise every diff mode (uncommitted, staged, branch, merge-base).
The READMEs pointed at a non-existent anchor (installation/#verifying-your-install); the verification guide is a standalone reference page. Point to /docs/reference/verifying-your-install/ (and split the hook README's link so version pinning -> installation, verification -> the reference page).
backnotprop
added a commit
that referenced
this pull request
Jun 17, 2026
… sinks The symlink-containment check existed as four byte-identical copies (Bun html-assets route, share inliner, annotate /api/share-html, Pi server). That duplication is exactly why the escape was missed in one sink before — #927 hardened three and missed the fourth, and the #929 fix added a fifth-in-waiting. Export the single canonical isWithinDirectory from the shared (Pi-vendored) html-assets-node module and have every sink import it; delete the three duplicate bodies and their now-dead realpathSync/relative/isAbsolute imports. A new sink can no longer silently diverge. Regression tests + 690 server/shared tests pass; build:pi clean.
backnotprop
added a commit
that referenced
this pull request
Jun 17, 2026
) * fix(server): always show remote URL + close share-html symlink gap Two pre-release fixes found by the QA sweep. 1. Remote URL stranding: handleServerReady only opened a browser (silent on a headless box) and wrote a ready-file; the user-visible URL came solely from writeRemoteShareLink, gated on sharingEnabled. So a remote/SSH user with sharing disabled (PLANNOTATOR_SHARE=disabled or config.json {share:"disabled"}, widened by #921) saw no URL and the agent hung on waitForDecision() forever. Now handleServerReady prints the reachable localhost URL whenever the session is remote, independent of sharing; the share link stays an extra. Pi/OpenCode already printed the URL unconditionally and are unaffected. 2. share-html symlink gap: the #927 symlink-containment fix hardened the /api/html-assets asset sinks but missed packages/server/annotate.ts's /api/share-html containment, which stayed lexical. A symlinked *.html inside the doc directory pointing outside it leaked the target's contents into the share payload. Now realpath-resolves both root and target like the other sinks. Bun-only — Pi's copy was already hardened. Adds regression tests for both (handleServerReady remote stderr; share-html returns 403 on symlink escape). * refactor(server): single shared isWithinDirectory for all asset/share sinks The symlink-containment check existed as four byte-identical copies (Bun html-assets route, share inliner, annotate /api/share-html, Pi server). That duplication is exactly why the escape was missed in one sink before — #927 hardened three and missed the fourth, and the #929 fix added a fifth-in-waiting. Export the single canonical isWithinDirectory from the shared (Pi-vendored) html-assets-node module and have every sink import it; delete the three duplicate bodies and their now-dead realpathSync/relative/isAbsolute imports. A new sink can no longer silently diverge. Regression tests + 690 server/shared tests pass; build:pi clean. * fix(pi): surface the session URL in the in-turn 'opened' notice for remote Remote Pi users never saw a review/annotate/last URL: it was emitted from openBrowserForServer AFTER the command's turn ended (fire-and-forget), and Pi only renders a notify during an active turn — so it silently dropped. The 'X opened. You can keep chatting' notice fires in-turn and DOES show, so fold the URL into it for remote sessions (single-line, matching the notify convention). Local sessions are unchanged (browser auto-opens).
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.
Security fix (the main one)
The raw-HTML annotate feature (#924) serves local support assets via
/api/html-assets/<token>/<path>and inlines them into share payloads. Both checked path containment lexically, so a symlink inside the HTML's directory pointing outside it (e.g.evil.css -> ~/.ssh/id_rsa) passed the check and was served / base64-inlined. In remote mode the inliner auto-fires at server startup, so a malicious HTML bundle could exfiltrate symlinked local files to the paste service with no user action.Fix: resolve symlinks with
realpathSyncon both the asset and the root before the relative-path containment check, in all three runtime copies:packages/server/html-assets.ts(Bun route handler)packages/shared/html-assets-node.ts(shared share-inliner; Pi gets it viavendor.sh)apps/pi-extension/server/serverAnnotate.ts(Pi route handler)Non-existent assets fall back to the lexical path and 404 on read; the root is realpath'd too so macOS
/tmp→/private/tmpsymlinking doesn't cause false rejections. Adds regression tests for both sinks (route returns 403, inliner doesn't embed the secret).Found via an adversarial multi-agent QA pass over the two unreleased PRs (#890, #924); confirmed and reproduced by two independent reviewers.
Also in this PR (unrelated, bundled to keep it moving)
tests/manual/local/sandbox-pi.shto build rich git state (multiple commits, a feature branch, rename+delete+modify) so/plannotator-reviewcan exercise every diff mode.README.mdandapps/hook/README.md— they pointed at a non-existentinstallation/#verifying-your-installanchor; now point to the standalone/docs/reference/verifying-your-install/page.Test
bun test packages/server/annotate-html-assets.test.ts packages/shared/html-assets.test.ts— 11 pass (incl. 2 new symlink regressions)bun run typecheck— clean