Fix high-severity security and bug findings - #2244
Conversation
kojiwakayama
added
bug
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b6f8cd552
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Confine read-file/edit-file to the project directory so traversal like ../../etc/passwd is rejected before any filesystem access. Closes #2226.
Validate every ctx.fs path against the project root before calling a Deno API. In compiled-binary mode the worker has full read permission, so this adapter is the only boundary stopping user route handlers from reading arbitrary host files. Closes #2227.
Return a generic 500 body; the full error is already logged server-side (SEC-009). Closes #2228.
Pad out-of-range positions with 0xff instead of 0x00 so a padded slot can never coincide with a real 0x00 byte and read as a match. Closes #2229.
Run child graphs on a scoped DAGExecutor instead of mutating the shared maxConcurrency field, which concurrent map nodes could race on and leave permanently corrupted. Closes #2230.
Wrap the SSE/AG-UI read loops in try/finally so a read error or a throwing event handler still releases the reader, instead of leaving the ReadableStream locked and the response body leaked. Closes #2231.
Reject with a proper JsonRpcError instead of a plain object, matching every other dispatch method. Closes #2232.
Default the rewritten-import cache buster to a content hash instead of Date.now(), which produced a unique token per millisecond and defeated all downstream module caching. Closes #2233.
Drop the entry when stat succeeds but the path is no longer a regular file, so we don't re-stat and fall through to a full fetch on every lookup. Closes #2238.
The security/bug-fix PR changes coding-agent template files and needs a Veryfront Code release bump after the 0.1.738 module-fetcher release landed on main. Regenerate the embedded template manifest so scaffolded coding-agent tools contain the path traversal guard, and keep the runtime version constant in sync with deno.json. Constraint: veryfront-code PRs that create a release must patch deno.json and VERSION. Constraint: Template source changes must keep cli/templates/manifest.json current. Rejected: Keep deno.lock churn from verification | no dependency changed and the lockfile edits were Deno cache resolution noise. Confidence: high Scope-risk: narrow Tested: deno task verify:quick Tested: deno run -A scripts/build/generate-templates-manifest.ts --check Tested: git diff --check
b38403c to
28f7c67
Compare
Deriving the cache buster from the importing module's source left a child module stamped with the parent's hash, so a changed child kept serving stale until the parent changed. Restore the prior per-request token; a correct optimization needs per-target-module versioning stamped by the module server. Reverts the #2233 change.
Canonicalize paths with Deno.realPath so a symlink inside the project that points outside it is rejected rather than followed, and use relative() for containment so valid child paths are accepted on Windows. Adds regression tests for traversal and symlink escape. (#2227)
read:true lets compiled-binary route handlers bypass ctx.fs via direct Deno.* calls. Point to the blocking follow-up (#2245) rather than implying the ctx.fs guard closes it.
|
Thanks for the review — addressed below. Codex P1 — template manifest not regenerated: Fixed. The guarded Codex P2 / cache-buster transitive staleness: Agreed, this was a real regression — a child stamped with the parent's content hash kept serving stale when only the child changed. Reverted the content-hash cache buster back to the prior per-request token. A correct optimization needs per-target-module versioning stamped by the module server (which reads the actual file); tracked separately. Review #2 + Codex P3 — symlink + Windows guard weakness: Fixed in Review #1 (HIGH/BLOCK) — compiled-binary If the reduced scope (defense-in-depth in compiled-binary mode, full containment elsewhere, with #2245 blocking) isn't acceptable, I'm happy to keep this blocked until #2245 lands with binary-CI evidence. |
kojiwakayama
changed the title
SerializedError isn't assignable to Record<string, unknown> directly; go via unknown for the absence-of-cause assertion.
Cross-runtime (Deno/Node/Bun) canonicalizing path resolver, needed so template tools can defeat symlink escapes without runtime-specific APIs.
read-file, edit-file and list-files now canonicalize the target via realPath and reject anything outside the project root — closing both ../ traversal (list-files had no guard at all) and symlink escapes (read/edit had only a lexical check). Containment is separator-normalized for Windows. Manifest regenerated so scaffolded projects get the guards. (#2226)
Keep the runtime export-surface guard in sync with the new realPath export.
|
Round 2 blockers — all addressed. 1. Typecheck fail on worker-script.test.ts:140 (HIGH): Fixed. The 2. list-files.ts path traversal (HIGH): Fixed. It had no containment at all — added the same canonicalized guard as read/edit. 3. read/edit/list symlink escape (HIGH): Fixed. The template tools no longer use a lexical check. All three now canonicalize via a new portable Supporting changes:
Verification: Unchanged by design: compiled-binary |
The second code-review batch bounds cross-project module fetches at 5MB. The fallback check needs to compare UTF-8 bytes rather than UTF-16 code units so sources without Content-Length cannot exceed the intended byte cap with non-ASCII text. This also prepares the rebased batch for release 0.1.740.\n\nConstraint: PR #2247 was rebased onto release 0.1.739 after #2244 merged.\nConstraint: veryfront-code release PRs must keep deno.json and VERSION in sync.\nRejected: Keeping deno.lock churn from local Deno verification | dependency-resolution metadata changed without a dependency update.\nConfidence: high\nScope-risk: narrow\nTested: deno test --frozen --allow-all src/modules/react-loader/ssr-module-loader/cross-project-import-loader.test.ts src/mcp/session.test.ts src/provider/runtime-loader.test.ts\nTested: deno check --frozen focused touched module set\nTested: deno task verify:quick\nTested: git diff --check
1 participant
Addresses the high-severity findings from the deep code review that are ready to ship now. This PR is release-prepared as
0.1.739and includes the regenerated coding-agent template manifest.Security
read-fileandedit-fileto the project dir and regeneratescli/templates/manifest.json.ctx.fsadapter unscoped — validates documentedctx.fspaths against the project root, including symlink and Windows-style escape coverage. Non-compiled mode is fully contained byread=[projectDir]. Compiled-binary directDeno.*reads remain a separate blocking follow-up in Narrow compiled-binary worker read permission (currently read:true) #2245.constantTimeEqualpadding — pads with0xffto match the CSRF comparison.Bugs
maxConcurrency.setLogLevel— throwsJsonRpcErrorinstead of rejecting with a plain object.Explicit non-closure
Deno.*filesystem reads.Verification
deno task verify:quickdeno run -A scripts/build/generate-templates-manifest.ts --checkgit diff --check2023 passed, 0 failed.