fix(agent): clear CodeQL py/path-injection on the #1083 result-callback sink#1275
Open
AndriiPasternak31 wants to merge 1 commit into
Open
fix(agent): clear CodeQL py/path-injection on the #1083 result-callback sink#1275AndriiPasternak31 wants to merge 1 commit into
AndriiPasternak31 wants to merge 1 commit into
Conversation
#1083, CodeQL) The #950 normpath+startswith guard cleared the construction site but CodeQL re-flagged the _persist/_delete sinks: it treats _pending_path's arg→return as tainted and does not propagate a CWE-022 barrier across a callee return. Inline the guard co-located with each write/replace/unlink sink (exactly how #950's guard sits with its rmtree/exists sink), and drop the now-unused _pending_path. Refs #1083 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if dest != base and not dest.startswith(base + os.sep): | ||
| raise ValueError(f"pending-result path escapes {base}: {execution_id!r}") | ||
| tmp = f"{dest}.tmp" | ||
| Path(tmp).write_text(json.dumps(record)) |
| raise ValueError(f"pending-result path escapes {base}: {execution_id!r}") | ||
| tmp = f"{dest}.tmp" | ||
| Path(tmp).write_text(json.dumps(record)) | ||
| Path(tmp).replace(dest) |
| raise ValueError(f"pending-result path escapes {base}: {execution_id!r}") | ||
| tmp = f"{dest}.tmp" | ||
| Path(tmp).write_text(json.dumps(record)) | ||
| Path(tmp).replace(dest) |
| dest = os.path.normpath(os.path.join(base, f"{execution_id}.json")) | ||
| if dest != base and not dest.startswith(base + os.sep): | ||
| return | ||
| Path(dest).unlink(missing_ok=True) |
|
Resolve by running |
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
2 participants
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.
Why
PR #1273 (#1083 fire-and-forget dispatch) squash-merged into
devwhile CodeQL was still failing — CodeQL is not a required check, so auto-merge landed it. The merged code indocker/base-image/agent_server/services/result_callback.pytripspy/path-injection(3 high) at the pending-result write/unlink sinks: the agent-suppliedexecution_idflows into~/.trinity/pending-results/<id>.json.This is a follow-up that lands the correct, CodeQL-clean fix on
dev.What
Inline the
#950path-containment guard (os.path.normpath+startswithprefix-check) co-located with each write/replace/unlink sink in_persist/_delete, and drop the now-unused_pending_pathhelper.Why this shape (the journey)
The guard had to be at the sink, in the same function:
try_spawn_async(caller)resolve()+is_relative_to()in_pending_pathos.path.basename()in_pending_pathnormpath+startswithin_pending_path_persist/_deletesinks (barrier not propagated across a callee return)normpath+startswithinlined at the sink (this PR)Defense-in-depth is layered: the
try_spawn_asyncregex belt still rejects non-token/UUID ids upstream (→ sync fallback); the inlined guard is the suspenders. The value is backend-generated (secrets.token_urlsafe/UUID) and never attacker-controlled in normal operation — these were defense-in-depth alerts, not an exploitable path.Tests
tests/unit/test_1083_result_callback.py:test_persist_delete_reject_traversal(traversal id is best-effort rejected — nothing written outside_PENDING_DIR, no raise) + existing persist/resend/delivery coverage. 27 passed locally; ruff clean.Refs #1083
🤖 Generated with Claude Code