fix: bound authentication and skill subprocess output - #141
Closed
GautamSharma99 wants to merge 1 commit into
Closed
fix: bound authentication and skill subprocess output#141GautamSharma99 wants to merge 1 commit into
GautamSharma99 wants to merge 1 commit into
Conversation
Collaborator
|
Thanks for tackling unbounded authentication and skill subprocess output. Those limits are already in the current code, and this branch now conflicts with |
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
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.
Summary
Fixes #130.
Bound the output retained from authentication subprocesses and Codex skill JSONL streams so a verbose, malfunctioning, or compromised child process cannot grow the SDK/CLI process's memory usage without limit.
The change also avoids repeatedly reparsing the complete interactive-login transcript while looking for device-auth instructions.
Changes
Bounded authentication output
Authentication subprocesses now enforce an independent 64 KiB ceiling for stdout and stderr.
For interactive login:
For noninteractive authentication commands:
PluginBootstrapErrorcontaining a fixed, redacted message;Incremental login instruction parsing
CodexLoginHandleno longer reparses all output collected so far for every incoming chunk.Instead, each stream keeps a 4 KiB instruction-discovery tail. New chunks are parsed together with that bounded tail, and the first discovered URL and device code are retained. This bounds the parsing work while still recognizing instructions split across normal subprocess chunks.
Bounded skill JSONL parsing
The skill output reader now uses a byte-bounded incremental newline parser instead of
readline, which could retain an arbitrarily large unterminated line.The parser enforces:
When an oversized event is encountered, the remainder of that line is discarded and the stream continues to be drained. After draining, the command fails with a fixed
CodexSecurityErrorthat does not echo subprocess content. This prevents a child from being left blocked on a full output pipe while still reporting the boundary violation safely.The parser continues to support CRLF input, a final event without a trailing newline, chunks split at arbitrary byte boundaries, and split UTF-8 sequences.
Security impact
This prevents untrusted or unexpectedly verbose authentication and skill subprocesses from causing unbounded:
Overflow diagnostics are constant strings and do not include the offending child output, preserving the existing credential-redaction boundary.
Tests
Added regression coverage for:
Verification
pnpm run typespnpm run formatpnpm run buildpnpm run testFull test result: