fix(doc): deny symlink escapes on /api/doc reads - #1437
Conversation
|
Heads up while you are actively on this: the branch is conflicting with main, which moved substantially today (a large header rework in packages/editor and packages/review-editor, plus new code-nav endpoints in both server runtimes), and the conflict is blocking CI from running on your latest commits. A rebase onto current main will unblock checks. The doc-read path you are hardening was not restructured, so the rebase should be mostly mechanical. Once it is green we will get you a full review promptly; this fix is wanted. |
|
No problem will rebase now! |
6cc3dcd to
1996133
Compare
|
Owe you an apology and one more rebase, and this one is entirely on us. Yesterday a fix commit on main was made with a blanket Your PR still renders correctly, but merging it as-is would reintroduce the pre-cleanup commits into main's history. One more Sorry for asking twice in two days. The review itself is next on our list once the branch is rebased — the containment work looks solid. |
`/api/doc` checked containment lexically only, so a symlink planted inside an allowed root resolved to an in-root path and was then read through to its target. All six content vectors leaked, plus the existence oracle on `/api/doc/exists`. Authorization was spread across 11 call sites in four branches of `handleDoc`, each interleaving resolve, authorize and read, and `apps/pi-extension/server/reference.ts` is a hand-maintained clone carrying the same sites and the same hole. Split resolve from read so authorization sits on one seam, in a shared module both runtimes consume. Resolution may stat but never reads, and the four render branches collapse to a document reader and a code reader, which also erases the missing size cap on the HTML branch rather than fixing it twice. Deliberate behavior changes: escaping symlinks are denied on every branch in both runtimes; the HTML branch enforces the 2MB cap; roots are realpath-normalized so a root reachable through a symlink resolves under either spelling.
Drop prose that restates the adjacent code and keep the facts it carried: that resolveDocTarget's result is unauthorized, and why the annotate version endpoints derive their history slug from the contained path.
1996133 to
72acf7d
Compare
|
Not a problem, updated! |
|
CI status on the rebased branch, with some triage to save you time:
What we've ruled out:
So it's Linux-runner-specific and introduced by the branch. Given your own note about having to realpath-normalize both sides because macOS Happy to dig further if you get stuck - and thanks again for the quick rebase turnaround. |
|
Makes sense, I'll test on Docker locally this round, either directly or with act |
storage.ts froze PLANNOTATOR_DATA_DIR at module import. Any consumer that first loaded it through a dynamic import inside a test body captured that test's temp dir for the rest of the process, so storage silently disagreed with draft.ts, config.ts, feedback-archive.ts and the annotate server, all of which resolve the dir live. That is what made four annotate.test.ts cases fail on CI but not locally: apps/opencode-plugin/embedded.ts reaches @plannotator/server through an await import(), and Bun's filesystem-dependent file order put that file before annotate.test.ts on CI only. getPlannotatorDataDir() already reads the environment on every call, so each use site now calls it directly. No signature changes.
|
The four failures reproduce locally, and the diagnosis needs correcting on three counts. This is not Linux-specific, not introduced by this branch, and not the realpath/slug interaction. No Docker needed. This fails the same four tests on macOS: That pair fails the same way on The cause is const DATA_DIR = getPlannotatorDataDir();The data dir is frozen at module import. Every other consumer resolves the data dir live: Only CI sees it because Bun's test-file discovery order is filesystem-dependent, and it is not controllable by argument order. Both earlier hypotheses were tested directly and passed: realpath/slug drift on a non-symlinked tmp, and XDG drift with The commit pushed here resolves the data dir per call in Four other modules freeze the data dir the same way: |
|
You are right on all three counts, and thank you for testing our hypothesis directly instead of just asserting against it - the realpath/slug guess was wrong, and the import-order mechanics you traced (dynamic import inside the lifecycle test's override window, filesystem-dependent discovery order deciding poisoner vs victim, the vendored Pi copy freezing at a different moment) explain every observation we had, including why the rerun 'broke the flake pattern.' This has been our #1464 mystery for five occurrences; consider it root-caused by you. One coordination note: #1473 (from another contributor, opened yesterday) fixes the same storage.ts freeze the same way, as part of a test-isolation PR closing #1455. So whichever of the two lands first, the other will get a small conflict on storage.ts that should resolve trivially since the changes converge. We'll sequence that on our side during review - no action needed from you. Please do file the follow-up for the other four frozen-at-import modules (improvement-hooks, codex-review, tour-review, guide-review); keeping this PR scoped was the right call. CI runs on your new head are approved and running. |
|
Deep review complete: the containment held against eight adversarial probes written independently of your suite (traversal, nested symlink chains, missing-leaf reattach, absolute paths, exists-endpoint escapes), legitimate symlink setups all still serve, Bun/Pi parity is line-for-line, no client surface mishandles the new status codes, and neutering the realpath gate fails 18 tests - the guards are real. Full runs clean on both runtimes, typecheck green. We'll carry two notes forward: a release-note line for the intended behavior change (in-root symlinks pointing elsewhere on the user's own disk now 403), and a small post-merge cleanup deduplicating the two storage regression tests that now coexist. Merging. Thank you for the containment work, the patience through two rebases we caused, and for root-causing our longest-standing CI flake along the way - this PR ended up improving the project well beyond its own scope. |
2 participants
Consolidates file handling for the review UIs into one shared module. Fixes symlinks breaking project folder containment and reading arbitrary paths on disk.
Approach
Resolving a path and reading it are now separate steps, so the permission check can run between them.
Resolving may
stata path to see whether it exists. It now checks the realpath and requires that to be inside the project in addition to the input path.realpathfails on a path that does not exist, and the check still has to judge those, since an outside path must be refused whether or not the file is there. So it resolves the deepest part of the path that does exist and re-attaches the rest.That catches a missing file under a symlinked folder without turning an ordinary missing file into a denial. Anything unexpected back from
realpathdenies the request.Behavior Changes
base.tmpdir()is itself a symlink and normalizing one side breaks every test on a Mac.basedirectory that reaches outside the project through a symlink is also refused.Tests
Each way a symlink can escape has its own test on both servers, since a single test covering one of them passes against a fix that missed a branch. Also covers a project root named through a symlink, an outside path that does not exist, and the HTML size limit.