| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Second half of the security pass. All of these are reachable by a user who imports a .src.md from an untrusted source, which is a normal thing to do — the CLI's import command exists to fetch notebooks off the internet. - DELETE /api/srcbooks/:id built a path with Path.join and handed it to fs.rm(recursive), and express URL-decodes params, so `..%2F..%2FDocuments` escaped SRCBOOKS_DIR. Paths from user input now go through containedPath, which resolves and asserts containment. removeSrcbook returns its promise so a failure is reported rather than becoming an unhandled rejection. - POST /api/file read any path on disk. Now confined to SRCBOOKS_DIR, which still covers what go-to-definition needs: a srcbook's own sources and the type declarations under its node_modules. - formatCode interpolated a filename into a shell command string. Filenames are validated on rename but not when they arrive from a decoded .src.md, so `###### $(id).mjs` in an imported notebook reached a shell. Now execFile with an argument array, run in the srcbook's own directory so npx resolves that copy of prettier. Prettier warns on stderr while still succeeding, so only a non-zero exit counts as failure now. - Same treatment for the depcheck invocation in deps.mts. - Filenames are validated at the decoder, which is where untrusted input enters. Both the h6 heading and the link target of an external-form cell, since the latter carries the real filename on disk. - POST /api/settings passed req.body straight to db.update().set(), so any column was writable — including aiBaseUrl, which decides where AI requests and the user's API key get sent. Now an explicit zod allowlist. 21 new tests: path containment (including the sibling-prefix case, where /tmp/srcbooks-evil starts with /tmp/srcbooks as a string but is not inside it) and decoder filename rejection. Suite is 38 tests, up from 11.
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
packages/api/ path-utils.mts +42/-0 containedPath / requireContainedPath schemas/ config.mts +31/-0 zod allowlist for POST /api/settings server/ http.mts +58/-9 contain /api/file + delete route, validate settings srcmd/ decoding.mts +38/-3 validate filenames at the trust boundary srcbook/ path.mts +13/-0 isSrcbookCellPath index.mts +3/-1 removeSrcbook returns its promise session.mts +21/-8 execFile for prettier, stderr is not failure deps.mts +23/-16 execFile for depcheck, reject instead of throw test/ path-utils.test.mts +55/-0 9 tests incl. sibling-prefix case srcmd.test.mts +49/-0 12 tests for filename rejection tasks/0002-...md +22/-1 close, record findings .changeset/ +19/-0Stacked on #507. Closes task 0002.
The second half of the security pass. Everything here is reachable by importing a .src.md from an untrusted source — which is a normal thing to do, since srcbook import exists specifically to fetch notebooks off the internet.
Unlike #507, these were found by reading rather than testing: both are destructive, so I didn't exercise them against a live instance.
Fixes
Path traversal into fs.rm(recursive: true). DELETE /api/srcbooks/:id built the path with Path.join, and express URL-decodes route params, so ..%2F..%2FDocuments escaped SRCBOOKS_DIR. Paths from user input now go through a containedPath helper that resolves and asserts containment. removeSrcbook returns its promise so failures are reported instead of becoming unhandled rejections.
Arbitrary file read. POST /api/file read any path on disk. Now confined to SRCBOOKS_DIR. Not deleted, because go-to-definition genuinely needs it — tsserver reports paths into a srcbook's own src/ and into type declarations under its node_modules, and that root covers both.
Shell injection via cell filename. formatCode interpolated a filename into a command string run through exec. Filenames are validated on rename but not when they arrive from a decoded .src.md, so ###### $(id).mjs in an imported notebook reached a shell. Now execFile with an argument array, run in the srcbook's own directory so npx resolves that copy of prettier rather than whatever is near the server's cwd. Same treatment for the depcheck call in deps.mts.
Validation moved to the boundary. Filenames are now checked when a .src.md is decoded, which is where untrusted input enters. This covers the link target as well as the h6 heading — the external form is ###### foo.ts followed by [foo.ts](./src/foo.ts), and it's the link that carries the real filename, so validating only the heading would have left the path open.
Mass assignment on settings. POST /api/settings handed req.body straight to db.update(configs).set(...), so any column was writable — including aiBaseUrl, which decides where AI requests, and the user's API key, get sent. Now an explicit zod allowlist.
Incidental
formatCode treated any stderr output as failure, but prettier writes warnings to stderr while still exiting 0. Only a non-zero exit counts now.
Tests
21 new, suite is 38 (from 11 at the start of the stack). The containment tests include the case that makes this kind of check go wrong in practice: /tmp/srcbooks-evil passes a naive startsWith('/tmp/srcbooks') while being an entirely different directory, so the check compares against root + path.sep.
Build, lint, and format all pass.
Noticed, not fixed
Two format bugs from REVIEW.md §2.2 that belong in their own change rather than a security PR: decode() still throws instead of returning {error: true} when metadata is missing, and encode() still assumes cells[0]/cells[1] are the title and package.json.
🤖 Generated with Claude Code