Repository navigation
fix(security): redact GCP metadata authentication error bodies - #2399
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edffd26f5f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
[P2] Confirmed the existing exact-head concern without duplicating its inline: #2399 (comment). The changed GCP metadata HTTP-error branch throws without consuming, canceling, or aborting response.body, and then clears the only abort timeout. An independent local streaming-HTTP reproduction confirmed that the previous path closed an indefinitely streaming 503 response while the new path rejected authentication and left its network connection open; repeated failures can accumulate sockets/resources. Preserve the new status-only redaction while disposing of the stream through Shims.CancelReadableStream(response.body). Suppress cancellation errors and abort the request if cleanup fails, because a custom stream can reject cancellation with a secret-bearing error that must not replace or contaminate the sanitized status-only error/cause. Add streaming-body cancellation and secret-bearing cancellation-rejection regressions.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27d1fbd883
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
[P2] The previous response-body leak is fixed, but the new direct await of Shims.CancelReadableStream(response.body) can permanently hang on a supported custom Fetch ReadableStream whose cancel() never settles. The configured timeout aborts the controller but does not settle that independent cancellation promise, so getToken() never throws its sanitized status error and the shared workload-identity refresh promise blocks every caller. Existing current inline already reports it: #2399 (comment). Race cleanup against the abort signal, dispose the temporary listener, suppress secret-bearing cleanup failures, and retain the original sanitized error. Add a cancellation-never-settles regression.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Independent two-pass re-review confirmed both prior issues are resolved. Non-OK metadata response streams are canceled without reading sensitive bodies, cancellation races the configured abort timeout, abort listeners are removed, late or secret-bearing cancellation failures cannot leak or hang shared workload authentication, and focused regressions cover detached never-settling custom streams. No remaining findings.
Summary
Validation