fix: validate private state before access - #481
Conversation
|
@codex review Please review commit |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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. |
![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: 8f4c05113d
ℹ️ 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".
mldangelo-oai
changed the title
|
Codex Review: Didn't find any major issues. Keep them coming! 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. |
zcrab-oai
left a comment
zcrab-oai
left a comment
There was a problem hiding this comment.
Found a deployment gap that prevents the new workbench security checks from reaching existing plugin installations.
| requested = requested_state_dir() | ||
| root = Path(requested) | ||
| try: | ||
| require_secure_state_ancestry(requested) |
There was a problem hiding this comment.
[P1] Refresh installed plugins when shipping this workbench security fix
Could we bump both bundled-plugin version declarations, or explicitly land this before a coordinated plugin-version bump? bootstrapPlugin reuses an already staged marketplace when the version is unchanged, and the installed MCP server loads workbench_db.py from its own installed root. Existing 0.1.20 installations therefore keep running the previous workbench without this new private-state and ancestry validation after an SDK upgrade.
|
@codex review Please review the current head |
|
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: df0ad83fb1
ℹ️ 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".
| await validateCodexSecurityStateDirectory(stateDirectory, (canonical) => | ||
| requireOutputOutsideRepository(protectedRoot, canonical), | ||
| ); |
There was a problem hiding this comment.
When CODEX_SECURITY_STATE_DIR traverses a repository-controlled symlink, this call validates the resolved destination but discards the returned canonical path; #localScanInputs subsequently returns the original alias, which is passed to scanRuntimeCodexConfig as a writable filesystem root and exported to the workbench. The repository can retarget that alias after preflight, causing later workbench operations to resolve and write a different state location. Fresh evidence despite the earlier addressed comment is that the current head still propagates the original stateDirectory at lines 1805, 549, 823, and 986. Store and propagate the canonical value returned here instead.
AGENTS.md reference: sdk/typescript/AGENTS.md:L19-L20
Useful? React with 👍 / 👎.
2 participants
Summary
Make unsafe output-parent errors actionable and validate configured state paths before state-backed operations access data.
The existing ancestry restriction is intentional: a private child can still be renamed or replaced through a writable, non-sticky parent. This change keeps that protection in place.
Changes
0.1.24so existing cached installations refresh the workbench.This complements the overlap checks in #418. The structured-error work in #463 addresses a separate concern.
Testing
Current metadata-only follow-up
df0ad83f:git diff --check: passed.Historical published head
5254d5fc:bun test --randomize --seed 12345: 1,323 passed, 11 skipped, 0 failed.pnpm run test: 1,323 passed, 11 skipped, 0 failed.pnpm run types,pnpm run format, Ruff lint on the changed Python script, andgit diff --check: passed.An unchanged login-cleanup test timed out in an earlier full run. It passed in isolated checks on the earlier revisions and in both final full runs. No authentication cleanup code was changed.
Risk and rollout
Existing state roots with group or other permissions, including older
0755directories, now fail state-backed operations. Use a dedicated private state directory, or correct its permissions only when it is yours and safe to change. New workbench state is created privately. The existing global output-ancestry rules are unchanged, and POSIX ownership and mode checks remain platform-gated. The new validation does not change existing state or parent permissions, silently relocate explicit paths, or migrate history or credentials. No dependency or npm-package release change is included. The metadata-only follow-up changes no application behavior. Current mainb4fe2d87still needs an additive conflict-resolution merge and validation before this PR can merge; keep both bundled-plugin versions synchronized with the final integration order.Public disclosure review
Newly authored material was reviewed for public disclosure. Existing automated review comments contain access-restricted report references; maintainer cleanup remains outstanding.