fix: consolidate contributor runtime-security hardening - #152
Conversation
A failed turn was only honored when its error payload was an object with a string message. Every other shape fell through the event loop, and because a turn.completed had already been observed the scan was returned to the caller as successful and recorded with complete-scan. Treat any turn.failed as terminal. Only error.message is reused, because that is the single shape the previous code already surfaced; no other shape is forwarded or stringified, since this message reaches fail-scan --message and is stored in scans.failure_message without redaction.
The scan finally block awaited knowledge-base cleanup, target-paths removal, and the credential home release without guarding any of them. A rejection in a finally block replaces the pending exception, so a cleanup failure discarded the outcome the try and catch blocks had already produced and the caller was handed an unrelated filesystem error instead of the reason the scan failed. The catch block had already recorded the real message through fail-scan, so the persisted scan history and the reported error disagreed. Removing the temporary inputs is best effort and is now reported through onWarning instead of deciding the result. Releasing the credential home lock is not: it only marks itself done once the lock directory is gone, so a failed release leaves an owner.json naming a live process that stale-lock recovery will not reclaim. That failure still propagates, and is downgraded to a warning only when the scan already failed and its error is the one worth keeping. The failed-scan marker is recorded on entry to the catch block because requireOpen and throwIfAborted raise a different error for the same failed scan, the release keeps its own finally so reporting the earlier failures cannot skip it, and the warning formatter is total so no hostile reason can throw in place of the result.
Co-authored-by: mangeshraut712 <mbr63@drexel.edu>
![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: 0409a6e7d6
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27924f5f5a
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
codex-security/sdk/typescript/src/api.ts
Lines 1914 to 1915 in 418fd91
Preserve trusted projects with credential-like path names
When the active project's absolute path contains a component such as token, key, env, or set—for example /workspace/token-service—this credential-word filter makes safeString reject the path. The projects projection consequently drops its trust entry even when it is the active project and well within the 256-entry limit, so config_preflight.py gets no exact trust match and refuses to load the project's .codex/config.toml; project paths need structural validation without applying the credential-name filter used for model-facing strings.
ℹ️ 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".
classifyConnectionFailure matches bare words anywhere in a message, and the unauthorized, forbidden and rate-limited branches of scanFailureMessage replace the message entirely with fixed advice. A local failure whose text merely contained one of those words was therefore reported as a credential problem, and its own text reached neither stderr nor the JSON error field. The reported trigger is "permission denied", the standard strerror for EACCES: a read-only TMPDIR surfaced as "The stored ChatGPT credentials cannot access the configured model." Skip classification for failures that cannot originate from the model transport -- InvalidTargetError, OutputDirectoryError, ConfigurationError, PluginPythonUnavailableError, and filesystem syscall errno codes -- so they keep their own message. Network errno codes are deliberately excluded from that set because they are genuine transport failures. Advice still replaces the underlying text for real transport failures. Upstream authentication and authorization errors can name the organization or project, and cli-authentication.test.ts pins that they must not reach stderr. Fixes #36
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Reject non-sticky or untrusted parents for durable auth state, re-check on every credential-home use path, harden auth.json, and pin identity for the lock session so shared-volume rename races cannot steal auth. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89e53213a5
ℹ️ 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".
|
Security review completed. No security issues were found in this pull request. Reviewed commit: ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b6d8ba845
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: 5b6d8ba845
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: a020832eaa
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5537f2357a
ℹ️ 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".
|
Security review completed. No security issues were found in this pull request. Reviewed commit: ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce61cfc407
ℹ️ 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".
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
# Conflicts: # sdk/typescript/tests-ts/runtime.test.ts
ianw-oai
left a comment
ianw-oai
left a comment
There was a problem hiding this comment.
Verified consolidated credential redaction, private credential storage, failed-turn handling, subprocess limits, and compatibility with the merged private-output protections; focused suites and typecheck pass.
ianw-oai
left a comment
ianw-oai
left a comment
There was a problem hiding this comment.
Confirmed custom authorization key-value credentials are fully redacted, including persisted multiscan receipts. Focused regression suites: 168 passing; TypeScript and formatting checks pass.
7 participants
Preserves the original contributor-authored commits from PRs #119, #126, #127, #141, #144, #146, #150, and #151, plus explicitly credited cross-platform login hardening from #142. Stored scan failures and multiscan diagnostics redact credentials; every turn.failed shape fails closed; scan cleanup preserves the original result; authentication/skill subprocess output and shutdown are bounded; configured plugin discovery and cost-session event parsing enforce limits; and active preflight profiles/projects survive projection bounds. Follow-up regressions prove both interactive and noninteractive oversized children that ignore SIGTERM are escalated and settle deterministically, inherited login pipes settle on every platform, and macOS login fixtures exit cleanly.
Validation: bun test --timeout 30000 ./tests-ts (487 pass, 5 platform/integration skips, 0 failures); TypeScript typecheck; Prettier checks. Original Git authorship is preserved; no external contributor forks were modified.