fix(security): env leak, timer TDZ, symlink escape, xray injection - #85
Merged
Conversation
…ection Addresses the security/correctness findings from the external review of PR #81. - **CRITICAL (#51)** cross-review no longer forwards the whole process.env to the reviewer subprocess (it held API tokens visible in the invocation and any log). buildInvocation now copies a strict allowlist (PATH/HOME/locale…) plus the recursion guard and auth home; `plan` prints only env KEY NAMES, never values. - **#51** runner: a synchronous spawn failure hit a timer temporal-dead-zone and threw ReferenceError instead of the typed fallback — the timer binding is now declared before `done`. A non-zero reviewer exit is a fallback, not trusted output. - **#68** store: `writeKind` resolves the real path and rejects a `registry` dir that symlinks outside the repo, so an approved write can never land elsewhere. - **#68** xray: baseUrl is parsed and restricted to a clean https origin+path (no userinfo/query/fragment); `since` and the record's `updated` must be ISO instants; a record key must be PROJECT-NNN — closing the JQL-injection and malformed-record surfaces. - Regression tests for each (env allowlist, sync-spawn fallback, non-zero exit, symlink escape, URL/since/record validation). Relates to #51 #68. Resolves the PR #81 env-leak, spawn-failure, symlink, and xray review threads. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KwyFyRC6pepaK6DuMK7YeT
5 tasks
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/correctness fixes from the external review of PR #81 — real bugs the unit tests missed.
process.envto the reviewer subprocess (API tokens visible in the invocation + logs). Now a strict allowlist + guard + auth home only;planprints env key names, never values.ReferenceErrorinstead of the typed fallback. Fixed. Non-zero exit is now a fallback.writeKindrejects aregistrydir that symlinks outside the repo (an approved write could land elsewhere — reproduced).since/updatedmust be ISO; record key must bePROJECT-NNN— closes the JQL-injection surface.Regression tests for every case; full suite + generated-drift green. Base
3.0. Resolves the PR #81 env-leak, spawn-failure, symlink, and xray threads.Relates to #51 #68