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.
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.