Stop kcap's stderr being read as hook output, and stop it carrying the raw server URL - #444
Merged
Merged
Conversation
Gemini's hook runner selects the text it parses as `stdout.trim() ||
stderr.trim()`, in a `child.on("close")` handler that never consults the
event name. Only SessionStart wrote to stdout, so on SessionEnd and
Notification — and on any event we do not route — kcap's stderr became
the hook's result. That is live today: a rejected lifecycle POST makes
`AgentHookPoster` write `[kcap] gemini-hook session-end/gemini: HTTP 400`,
and Gemini consumes it as hook-sourced content.
The invariant is now structural rather than a discipline each early
return has to remember, which is how it came to hold for one event out of
four: a write-once sink is created as soon as a `hook_event_name` is
recognised and flushed from a `finally`. A path with a real payload (the
SessionStart memory envelope) claims the single write first; everything
else degrades to an explicit `{"continue":true}`. Input with no parseable
`hook_event_name` still stays silent — we cannot know a hook fired.
Also documents the second half of the contract, which the issue's
analysis had backwards. Gemini's plain-text fallback is not
unconditionally non-blocking: it maps exit 0 and 1 to `decision: "allow"`
but any other code to `decision: "deny"`, which `isBlockingDecision()`
honours. kcap stays out of that band only because `hook` is in
`CrashReporter.FailOpenCommands`, which turns `EnsureAbsolute`'s
`Environment.Exit(2)` into a throw and makes the top-level catch return
0 — a load-bearing coupling across three files that nothing stated. It is
now pinned by a test.
Tests assert what Gemini would actually do with the bytes, not merely
that some were written, and the failed-POST case asserts the diagnostic
really was written before checking that stdout shadows it — without that
positive control the test passes vacuously the day the diagnostic moves.
`WriteSessionStartOutput` is dropped: the refactor left it with no
production call site, and its throwing-writer contract now belongs to the
sink. Behaviour measured on gemini 0.53.0.
`WriteUnreachableError` interpolated the base URL raw:
Console.Error.WriteLine($"{UnreachableHint} {baseUrl} {ex.Message}");
A `server_url` may carry userinfo credentials, and `UnusableUrlDiagnostic`
— same assembly, same stream, same hazard — has sanitized for exactly
that since it was written. This call bypassed it.
It is reachable from the HOOK path, not just interactive commands:
`AgentHookPoster.PostAsync` calls it on any `HttpRequestException`, so an
unreachable or misconfigured server printed the credential-bearing URL on
every lifecycle POST, for every vendor. Seventeen other call sites across
Recap, Review, Projects, ValidatePlan, Curate, Errors, OAuthLoginFlow and
Program.cs share it.
Control characters are now stripped from BOTH variable components, not
just the URL. Guarding only the URL would have left the other half of the
interpolation open, and this line goes to a stream harnesses parse — on
Gemini, kcap's stderr is read as the hook's own result when stdout is
empty, so an embedded newline was a line-injection vector into content
the model reads.
The fixed hint's own `\r` is deliberately left alone: it is not
attacker-reachable, and changing it would alter the output every existing
call site produces. The tests assert "the payload never begins a line"
rather than "the output holds no control character" for that reason — the
blanket form would have been testing the fixed prefix and failing for a
reason unrelated to injection.
Rendering is split into a pure `RenderUnreachableError` so the guard is
testable without capturing Console.Error. The host and the failure cause
both survive sanitization, which is what keeps the line actionable; a
test pins that, so this cannot be "fixed" by sanitizing to nothing.
Also corrects the doc comment, which claimed the method writes a
structured JSON error. It writes an interpolated line.
PR Summary by QodoEnsure Gemini hooks always emit stdout JSON; sanitize unreachable-server stderr
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Code review caught that the stated invariant is stronger than the code holds, and that my own tests disprove it. `HookResultWriter.Write` consumes the claim BEFORE attempting the write, so a throwing stdout leaves the output empty (throw before any byte) or truncated (throw mid-payload), with the backstop no-opping in both cases — and the stderr fallback back in play. `A_throwing_write_is_swallowed_and_still_consumes_the_single_claim` pins exactly those two shapes with its 0 and 5 arguments, while the class remarks claimed every recognised firing writes exactly one JSON object and that stdout always wins. The behaviour is unchanged and remains the right trade: nothing here can distinguish a zero-byte failure from a partial write, and appending a second object onto a partial one is unparseable — which is the same stderr fallback with extra steps. A stdout we cannot write to has no recovery from inside this process. So the prose moves to "exactly one write ATTEMPT", with the residue named rather than papered over, at all four sites that overstated it: the class remarks, the invariant comment in Handle, the sink method doc, and the test-class remarks. AllowPayload similarly now says its cannot-fail property is about RENDERING, not delivery. Stating an invariant more absolutely than the code holds is what invites the next person to "restore" it — here, by adding a retry that manufactures the concatenated-object case.
The previous commit lumped both throwing-writer outcomes together as "falls back to stderr". Only one of them does. The selection is `stdout.trim() || stderr.trim()`, so a TRUNCATED stdout is truthy and stays selected — stderr is never reached. The invalid JSON fails to parse and degrades to plain text, which at the exit codes this command returns is an allow carrying our own partial payload as junk context. Only the zero-byte throw leaves stdout falsy and selects stderr, which is the original bug surviving for that one case. That also sharpens why a retry is wrong: it does not re-expose stderr, it guarantees the truncated case — invalid, still-selected, model-visible stdout. The rationale now lives in exactly one place, on HookResultWriter, with the class remarks, the Handle comment and the tests referencing it in a line each. Repeating it at four sites is how the claim drifted twice — first overstating the invariant, then mis-describing the residues while correcting it — and it ran against this repo's "do not get too verbose in comments" convention. No behaviour change; no test assertion changed.
Qodo rule violation: the new tests checked JsonElement.ValueKind
directly, against this repo's "DO use JsonElementExtensions instead of
checking JSON value kind" convention.
The two reads now go through the helpers, and both say more than the
ValueKind check did:
- The backstop test reads Obj("hookSpecificOutput") back. Obj() yields
null for any non-object root, so this proves the root shape AND that
the first payload survived intact, in one read.
- AssertNonBlockingJsonObject reads Str("decision"). Str() returns null
unless the root is an object AND the property is a string — the same
two conditions Gemini applies before honouring a decision — so one
read covers what the ValueKind check and TryGetProperty did together.
An equality assertion against AllowPayload follows it, which is
strictly stronger than a shape check and pins the object-ness that a
null read cannot distinguish from a missing key.
The integration test's TryGetProperty("decision") is aligned to Str() for
the same reason.
Verified the new read is not vacuously null: temporarily setting
AllowPayload to {"decision":"deny"} fails on the decision assertion
itself, which precedes the equality assertion.
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.
Linear: AI-1618, AI-1729
Two findings that share one root: kcap's stderr is a channel something else reads. Both were found by auditing what kcap actually writes there on Gemini hook paths.
AI-1618 — Gemini consumes kcap's stderr as hook output
Gemini's hook runner selects the text it parses as
stdout.trim() || stderr.trim(), in achild.on("close")handler that never consults the event name. OnlySessionStartwrote to stdout, so onSessionEnd,Notification, and any event we do not route, kcap's stderr became the hook's result.This is live, not theoretical. The new test's RED output was the bug in production form:
One correction to the issue's analysis
The issue records this as safely benign because "the plain-text fallback never synthesises a block." It does:
decision: "deny"is whatisBlockingDecision()returns true for — a blocked session, not junk context.kcap stays out of that band only because
"hook"is inCrashReporter.FailOpenCommands, which turnsHttpClientExtensions.EnsureAbsolute'sEnvironment.Exit(2)into a throw and makes Program.cs's top-level catch return 0. That is a load-bearing coupling across three files that nothing stated. It is now written down next to the command and pinned by a test, so removing"hook"from that set fails loudly instead of turning a malformedserver_urlinto a blocked Gemini session.The fix
The invariant is now structural instead of per-path discipline — which is precisely why it held for one event out of four. A write-once sink is created as soon as a
hook_event_nameis recognised and flushed from afinally. A path with a real payload (the SessionStart memory envelope) claims the single write first; everything else degrades to an explicit{"continue":true}. Five scattered emit calls collapse into one guarantee, and a future early return cannot reopen the hole.Input with no parseable
hook_event_namestill stays silent — we cannot know a hook fired at all, and emitting a decision object into some other consumer's stdout is its own hazard.eventNameis deliberately not filtered to the events we route, since Gemini's close handler does not filter either.WriteSessionStartOutputis removed: the refactor left it with no production call site, and its throwing-writer contract now belongs to the sink.AI-1729 — the unreachable-API line printed the raw server URL
A
server_urlmay carry userinfo credentials.UnusableUrlDiagnostic— same assembly, same stream, same hazard — has sanitized for exactly this since it was written; this call bypassed it.Reachable from the hook path, not just interactive commands:
AgentHookPoster.PostAsynccalls it on anyHttpRequestException, so an unreachable or misconfigured server printed the credential-bearing URL on every lifecycle POST, for every vendor. Seventeen other call sites share it.Control characters are now stripped from both variable components. Guarding only the URL would leave the other half of the interpolation open — and before AI-1618's fix, an embedded newline here was a line-injection vector into content the model reads.
The fixed hint's own
\ris left alone: not attacker-reachable, and changing it would alter output every existing call site produces. The tests assert "the payload never begins a line" rather than "the output holds no control character" for that reason — the blanket form would have been testing the fixed prefix and failing for a reason unrelated to injection.Verification
isBlockingDecision— not merely that some bytes were written.HttpClientExtensions/AgentHookPoster(AgentHookPosterTests, the threeHttpClientExtensions*classes,UnusableUrlGuardTests,UnusableUrlDiagnosticTests,SetupCommandTests, bothSpawnBeforePostclasses): all green.CS8604inGeminiHookCommandis inherited — proved against a detachedorigin/mainworktree (same warning, line 272 → 333, shifted by added lines), not assumed.Gemini behaviour measured on
gemini 0.53.0, read from the installed bundle. Treated as a version-specific observation: the mitigation does not depend on it, since emitting a valid non-blocking object is correct under any of these selection rules.No README change — no user-facing CLI surface changes (no new command, flag, default, or prerequisite).
🤖 Generated with Claude Code