Repository navigation
fix(codeql): resolve 11 py/path-injection alerts via os.path.realpath+startswith - #492
Merged
Merged
Conversation
…19,23,28-31,45-47) Resolves the remaining 11 py/path-injection alerts by switching _safe_path (agent-api.py), _resolve_note_path (dashboard.py), and the /answer inline handler to the CodeQL-recognized sanitizer pair: Path::PathNormalization : os.path.realpath Path::SafeAccessCheck : str.startswith(base + os.sep) Path.resolve / Path.is_relative_to are NOT in codeql/python-all's PathInjection customizations (Stdlib.qll only models os.path.normpath, abspath, realpath and str.startswith), which is why prior refactors #487/#488 didn't close the alerts. Verified locally with CodeQL CLI 2.25.2 + codeql/python-queries 1.8.0: before: 11 path-injection alerts (current main at dc7bfbe) after: 0 path-injection alerts Behavioral sanity checks unchanged: traversal, absolute, empty, and dotfile inputs are all stripped by the existing regex whitelist and then confined by the realpath + startswith guard. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 of 4 tasks
liususan091219
added a commit
that referenced
this pull request
Apr 21, 2026
…-logging) (#493) ## #43 js/resource-exhaustion (src/web-client.ts:2549) #489's `Math.min(ttl, MAX_TTL_MS)` is an `isNumericFlowStep` (numeric passthrough) in `codeql/javascript-all@2.6.27` — it narrows the value but doesn't register as a sanitizer. The `js/resource-exhaustion` config only blocks taint via `UpperBoundsCheckSanitizerGuard`, which matches `RelationalComparison` AST nodes (`<`, `<=`, `>`, `>=`). Rewrote the clamp as `ttl <= MAX_TTL_MS ? ttl : MAX_TTL_MS` — semantically identical, but CodeQL now sees `ttl` as the lesser operand in a `<=` comparison and blocks taint in the true branch. ## #44 js/clear-text-logging (src/voice-agent.ts:71) The `${value.length}` and `${prefixOk}` interpolations in `console.error` both read properties of `value`. The env var `GEMINI_API_KEY` triggers `HeuristicNames` → tainted source; `allowImplicitRead` in the cleartext config pulls any property of a tainted value into the logger sink. #486 stopped echoing substrings but the length + ternary-prefix diagnostic still flow the taint. Dropped those two interpolations from the log; kept the name, expected format ("AIza..." 35-60 chars), and remediation URL. Diagnostic loss is minor — a malformed key always needs to be regenerated anyway. ## Verification Local CodeQL CLI 2.25.2 + codeql/javascript-queries@2.3.7: - main (dc7bfbe): 2 alerts (resource-exhaustion @ web-client.ts:2549, clear-text-logging @ voice-agent.ts:71) - this branch: 0 alerts `npx tsc --noEmit` clean. Closes #43, #44. Together with #492 (merged earlier this session), this zeroes the CodeQL security-code-scanning queue. Co-authored-by: Xueqing Liu <xueqingliu@Xueqings-Mac-Studio.local> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 of 3 tasks
sonichi
added a commit
that referenced
this pull request
Apr 21, 2026
…494) * fix(discord-bridge): allowlist-gate [file:/send:/attach:] sends The result-text `[file: PATH]` / `[send: PATH]` / `[attach: PATH]` markers previously passed a bare `os.path.isfile()` check before attaching. Any path that existed on disk would be forwarded to Discord — trusted today because only agent output produces those markers, but brittle against future ingress changes or a tooling drift that starts preserving structured file items. This change: - Adds `_is_path_sendable(path)` using os.path.realpath + prefix match (matches the sanitizer shape CodeQL recognized in #492). - Allows paths under `results/`, `notes/`, `~/Desktop/iclr-backups/`, or with `/tmp/sutando-` / `/private/tmp/sutando-` prefix. - Fails closed with `(file not allowed: PATH)` visible inline. - Patches both sinks: normal reply path (line 757) + proactive DM path (line 857). Tested via 6-case helper unit test (repo-local allowed, /tmp/sutando- allowed, /etc/passwd rejected, non-existent rejected, random /tmp rejected, notes/ allowed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(discord-bridge): widen allowlist per MacBook review MacBook cold-review flagged 3 legitimate agent-generated paths that would have been rejected under the initial narrow scope: - docs/ — repo-local; historical send of stand-avatar-candidate-1.png found in results/ archive. - /tmp/echo-intro/*.mp4 — Echo Act IV launch video assets. - ~/Documents/sutando-launch-assets/* — persistent copy of launch assets Chi saved yesterday. All 3 are known agent-generated roots (not attacker-controlled), so the design intent of "trust agent output under known roots" covers them. Widening preserves the fail-closed guarantee for unknown paths. 6/3 new-case sanity pass (docs/, /tmp/echo-, /etc/passwd). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Chi <wangchi@Chis-Mac-mini.hsd1.wa.comcast.net> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
liususan091219
added a commit
to liususan091219/sutando-fork
that referenced
this pull request
May 28, 2026
…chi#17-19,23,28-31,45-47) (sonichi#492) Resolves the remaining 11 py/path-injection alerts by switching _safe_path (agent-api.py), _resolve_note_path (dashboard.py), and the /answer inline handler to the CodeQL-recognized sanitizer pair: Path::PathNormalization : os.path.realpath Path::SafeAccessCheck : str.startswith(base + os.sep) Path.resolve / Path.is_relative_to are NOT in codeql/python-all's PathInjection customizations (Stdlib.qll only models os.path.normpath, abspath, realpath and str.startswith), which is why prior refactors sonichi#487/sonichi#488 didn't close the alerts. Verified locally with CodeQL CLI 2.25.2 + codeql/python-queries 1.8.0: before: 11 path-injection alerts (current main at dc7bfbe) after: 0 path-injection alerts Behavioral sanity checks unchanged: traversal, absolute, empty, and dotfile inputs are all stripped by the existing regex whitelist and then confined by the realpath + startswith guard. Co-authored-by: Xueqing Liu <xueqingliu@Xueqings-Mac-Studio.local> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
liususan091219
added a commit
to liususan091219/sutando-fork
that referenced
this pull request
May 28, 2026
… (clear-text-logging) (sonichi#493) ## sonichi#43 js/resource-exhaustion (src/web-client.ts:2549) sonichi#489's `Math.min(ttl, MAX_TTL_MS)` is an `isNumericFlowStep` (numeric passthrough) in `codeql/javascript-all@2.6.27` — it narrows the value but doesn't register as a sanitizer. The `js/resource-exhaustion` config only blocks taint via `UpperBoundsCheckSanitizerGuard`, which matches `RelationalComparison` AST nodes (`<`, `<=`, `>`, `>=`). Rewrote the clamp as `ttl <= MAX_TTL_MS ? ttl : MAX_TTL_MS` — semantically identical, but CodeQL now sees `ttl` as the lesser operand in a `<=` comparison and blocks taint in the true branch. ## sonichi#44 js/clear-text-logging (src/voice-agent.ts:71) The `${value.length}` and `${prefixOk}` interpolations in `console.error` both read properties of `value`. The env var `GEMINI_API_KEY` triggers `HeuristicNames` → tainted source; `allowImplicitRead` in the cleartext config pulls any property of a tainted value into the logger sink. sonichi#486 stopped echoing substrings but the length + ternary-prefix diagnostic still flow the taint. Dropped those two interpolations from the log; kept the name, expected format ("AIza..." 35-60 chars), and remediation URL. Diagnostic loss is minor — a malformed key always needs to be regenerated anyway. ## Verification Local CodeQL CLI 2.25.2 + codeql/javascript-queries@2.3.7: - main (dc7bfbe): 2 alerts (resource-exhaustion @ web-client.ts:2549, clear-text-logging @ voice-agent.ts:71) - this branch: 0 alerts `npx tsc --noEmit` clean. Closes sonichi#43, sonichi#44. Together with sonichi#492 (merged earlier this session), this zeroes the CodeQL security-code-scanning queue. Co-authored-by: Xueqing Liu <xueqingliu@Xueqings-Mac-Studio.local> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
liususan091219
pushed a commit
to liususan091219/sutando-fork
that referenced
this pull request
May 28, 2026
…onichi#494) * fix(discord-bridge): allowlist-gate [file:/send:/attach:] sends The result-text `[file: PATH]` / `[send: PATH]` / `[attach: PATH]` markers previously passed a bare `os.path.isfile()` check before attaching. Any path that existed on disk would be forwarded to Discord — trusted today because only agent output produces those markers, but brittle against future ingress changes or a tooling drift that starts preserving structured file items. This change: - Adds `_is_path_sendable(path)` using os.path.realpath + prefix match (matches the sanitizer shape CodeQL recognized in sonichi#492). - Allows paths under `results/`, `notes/`, `~/Desktop/iclr-backups/`, or with `/tmp/sutando-` / `/private/tmp/sutando-` prefix. - Fails closed with `(file not allowed: PATH)` visible inline. - Patches both sinks: normal reply path (line 757) + proactive DM path (line 857). Tested via 6-case helper unit test (repo-local allowed, /tmp/sutando- allowed, /etc/passwd rejected, non-existent rejected, random /tmp rejected, notes/ allowed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(discord-bridge): widen allowlist per MacBook review MacBook cold-review flagged 3 legitimate agent-generated paths that would have been rejected under the initial narrow scope: - docs/ — repo-local; historical send of stand-avatar-candidate-1.png found in results/ archive. - /tmp/echo-intro/*.mp4 — Echo Act IV launch video assets. - ~/Documents/sutando-launch-assets/* — persistent copy of launch assets Chi saved yesterday. All 3 are known agent-generated roots (not attacker-controlled), so the design intent of "trust agent output under known roots" covers them. Widening preserves the fail-closed guarantee for unknown paths. 6/3 new-case sanity pass (docs/, /tmp/echo-, /etc/passwd). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Chi <wangchi@Chis-Mac-mini.hsd1.wa.comcast.net> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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.
Summary
Closes the remaining 11
py/path-injectionalerts by switching three flagged helpers to the CodeQL-recognized sanitizer pair.main_safe_path)/answerhandler)_safe_path)_resolve_note_path)_resolve_note_path)Root cause
Prior PRs (#487, #488) added
Path(x).nameand inline comments calling it a "CodeQL-recognized sanitizer". That turns out not to be true for thepy/path-injectionquery. Incodeql/python-all@7.0.4:Path::PathNormalization::Rangeis only extended byos.path.normpath,os.path.abspath, andos.path.realpath(Stdlib.qll:1108-1132).Path::SafeAccessCheck::Rangeis only extended bystr.startswith(Stdlib.qll:5153).Path.resolve(),Path.is_relative_to(), andPath.nameare not modeled, so the helper's internal.name+.is_relative_toguards look like untracked pass-through flows to CodeQL — taint continues fromraw_slug/task_idstraight through to the file-system sinks.Change
Three helpers switch to
os.path.realpath(...)+str.startswith(base + os.sep):src/agent-api.py::_safe_pathsrc/agent-api.pyinline/answertask-writersrc/dashboard.py::_resolve_note_pathThe regex whitelist at the front is preserved (defence-in-depth), so behaviour is identical for every input. Diff is
+31 / -38.Verification
Local CodeQL CLI 2.25.2 +
codeql/python-queries@1.8.0:mainatdc7bfbe): 11 path-injection alerts.Test plan
python3 -c "import ast; ast.parse(open('src/agent-api.py').read())"— syntax OKpython3 -c "import ast; ast.parse(open('src/dashboard.py').read())"— syntax OK_safe_pathcorrectly strips../etc/passwd,/etc/passwd,.., and empty input — all remain insidebase_diror returnNoneSupersedes #491 (comments-only; closed).
— Lucy (Mac Studio bot)