fix(googlechat): cancel unread fetchOk bodies before release - #111290
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 19, 2026, 3:55 AM ET / 07:55 UTC. Summary PR surface: Source +5, Tests +113. Total +118 across 2 files. Reproducibility: yes. from source: Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the caller-local unread-body cancellation after a rebase confirms the focused transport test and required checks still pass; keep the general SSRF helper contract unchanged unless a separately reviewed cross-caller refactor establishes that every caller can safely delegate cleanup to it. Do we have a high-confidence way to reproduce the issue? Yes, from source: Is this the best way to solve the issue? Yes. Cancelling only at the Google Chat status-only caller is the narrowest maintainable fix: it protects the unread AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 58452de71188. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +5, Tests +113. Total +118 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
Yigtwxx
left a comment
There was a problem hiding this comment.
The !response.bodyUsed guard is the right shape here — it covers the status-only fetchOk path without double-consuming the JSON/buffer handlers that already read the stream, and putting it in finally means it also runs when handleResponse throws.
I checked this against the repo-wide convention and it matches: the same !bodyUsed → cancel().catch(() => undefined) pattern is well established across the guard consumers, so this isn't a local invention.
Two notes: one site in this extension that I believe the sweep missed, and one about what the new test actually establishes. Both inline.
| } finally { | ||
| // fetchOk (status-only) leaves the body unread. JSON/buffer handlers set | ||
| // bodyUsed first. release() does not cancel streams. | ||
| if (!response.bodyUsed) { |
There was a problem hiding this comment.
This comment states the invariant precisely — and there's a third guard site in this same extension where it's violated, which I think is a genuine miss rather than out of scope.
extensions/googlechat/src/google-auth.runtime.ts:426-436 (not in this diff):
try {
const body = await readGoogleAuthResponseBytes(response);
...
} finally {
await release(); // :435 — no cancel
}readGoogleAuthResponseBytes can throw before it ever touches the stream — the size check at :444-446 runs against the content-length header, while response.body?.getReader() isn't reached until :449:
if (contentLength !== null && contentLength > MAX_GOOGLE_AUTH_RESPONSE_BYTES) {
throw new Error(`Google auth response exceeds ${MAX_GOOGLE_AUTH_RESPONSE_BYTES} bytes.`);
}So a Google auth endpoint advertising an oversized Content-Length unwinds to that finally with bodyUsed === false and response.body !== null — exactly the state your comment says release() doesn't handle.
What convinced me this is an oversight and not a deliberate boundary: the extension's third guard site already does it correctly. extensions/googlechat/src/auth.ts:123:
if (!response.ok) {
await response.body?.cancel().catch(() => undefined);
throw new Error(`Failed to fetch Chat certs (${response.status})`);
}So googlechat had three sites — one already compliant, one fixed here, one left. Same shape as this hunk would close it:
} finally {
if (!response.bodyUsed) {
await response.body?.cancel().catch(() => undefined);
}
await release();
}| return { | ||
| response, | ||
| finalUrl: url, | ||
| release: async () => {}, |
There was a problem hiding this comment.
Smaller point, about what this proves rather than whether it passes.
Using a real loopback server and a real undici Response is the right call — the socket-close assertion is meaningful, and it's much better than a fabricated response object. But release is stubbed to a no-op here, and the thesis in the production comment is specifically "release() does not cancel streams". The real release() reaches closeDispatcher → waitForDispatcherClose (src/infra/net/ssrf.ts:734-761), which calls dispatcher.close() and escalates to destroyDispatcher() on timeout — i.e. it does tear the socket down, just not promptly or via the stream.
So the test currently establishes "cancel() closes the socket", which would hold even if release() were sufficient. It doesn't establish that the leak exists on the real path.
Letting the real release run — return the guard's own release rather than async () => {} — would close that gap, or alternatively one extra case asserting the socket stays open when only release() runs. The sibling PR #111275 (tlon) ends up with this property for free by driving the real fetchWithSsrFGuard with ssrfPolicy: { allowPrivateNetwork: true } plus a loopback lookupFn instead of mocking the module, if you want a shape to copy.
|
Second pass, since the test surface grew from +113 to +299 after my earlier review and nothing had re-read it. The stubbed- The tests are load-bearing. Removing just the new guard from turns 2 of 6 red, and restoring it returns 6/6. So the file pins this fix specifically, not incidental behaviour. My other point is still open, and I can now show it instead of asserting it. try {
const body = await readGoogleAuthResponseBytes(response);
...
} finally {
await release(); // no cancel
}
I measured it with the same harness style this PR uses — a real loopback server, the real
The second row is exactly the state Worth noting the trigger is reachable without a hostile server: any auth response advertising more than 1 MiB gets there, and the size guard exists precisely because that is considered possible. Not asking to widen this PR — the |
|
Merged via squash.
|
The size guard in readGoogleAuthResponseBytes inspects content-length and throws before it reaches response.body.getReader(), so an oversized auth response leaves the stream untouched and the finally block releases the dispatcher without cancelling it. This is the same shape openclaw#111290 just fixed in api.ts, applied to the auth transport's own guard site.
The size guard in readGoogleAuthResponseBytes inspects content-length and throws before it reaches response.body.getReader(), so an oversized auth response leaves the stream untouched and the finally block releases the dispatcher without cancelling it. This is the same shape openclaw#111290 just fixed in api.ts, applied to the auth transport's own guard site.
…w#111290) Co-authored-by: Peter Steinberger <steipete@gmail.com>
…115873) The size guard in readGoogleAuthResponseBytes inspects content-length and throws before it reaches response.body.getReader(), so an oversized auth response leaves the stream untouched and the finally block releases the dispatcher without cancelling it. This is the same shape #111290 just fixed in api.ts, applied to the auth transport's own guard site.
…penclaw#115873) The size guard in readGoogleAuthResponseBytes inspects content-length and throws before it reaches response.body.getReader(), so an oversized auth response leaves the stream untouched and the finally block releases the dispatcher without cancelling it. This is the same shape openclaw#111290 just fixed in api.ts, applied to the auth transport's own guard site.
What Problem This Solves
Google Chat status-only API calls (
fetchOk, used bydeleteGoogleChatMessage)go through
withGoogleChatResponse→fetchWithSsrFGuard, then only need theHTTP status. The
finallypath calledrelease()without cancelling an unreadresponse body.
release()tears down the SSRF dispatcher but does notcancel streams, so undici can keep the TCP connection pinned after a successful
DELETE (or any other
fetchOkcaller).JSON/buffer helpers already consume the body (
bodyUsed), so only status-onlysuccess paths leaked.
Why This Change Was Made
In
withGoogleChatResponse'sfinally, cancel the unread body when!response.bodyUsedbeforerelease(). That coversfetchOkwithout changingfetchJson/fetchBuffer(they setbodyUsedfirst). Error responses alreadyconsume the body via
readGoogleChatErrorResponse.User Impact
Before: A Chat API endpoint that streams or holds a successful status-only
body could leave the request socket open after delete (or other
fetchOk)returned.
After: Status-only successes cancel the unread body and release the socket
promptly. JSON/media reads and error parsing are unchanged.
Evidence
extensions/googlechat/src/api.ts,extensions/googlechat/src/api.fetchok.transport.test.tsdeleteGoogleChatMessage→fetchOk→withGoogleChatResponse→fetchWithSsrFGuard(
auditContext: "googlechat.api.ok") → cancel unread →release()Real behavior proof
Behavior or issue addressed
Successful Google Chat
fetchOk(DELETE) must cancel unread bodies. Withoutcancel, a streaming 200 body that never ends keeps
serverObservedSocketClose: false250ms after the call returns.Canonical reachability path
deleteGoogleChatMessage→fetchOk→withGoogleChatResponse→fetchWithSsrFGuardDELETE → status OK →finallycancel unread →release()Shared helper / provider constraint check
Uses existing
fetchWithSsrFGuard. Same undici consume-or-cancel contract ascron preflight / Discord voice CDN status-only paths
(#111226 / #111269).
Real environment tested
Production
deleteGoogleChatMessagetransport test (committed): loopbackopen-body DELETE; SSRF guard proxied to undici
fetchand rewritten ontoloopback so the Chat hostname can be exercised locally; production
cancel/
releasestill run on a realResponse. Auth is stubbed(
getGoogleChatAccessToken).Cancel+release contract before/after (same open-stream DELETE shape):
Negative control (no cancel):
After cancel:
macOS, Node v22.23.1.
Evidence after fix
Premise: Undici requires every response body to be consumed or canceled.
AI-assisted: Yes.