Reserve missing preserved paths in Linux sandbox policy#18446
Conversation
30deabb to
e4249a5
Compare
e4249a5 to
f85a396
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ 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". |
30d621d to
042c0ba
Compare
|
Reviewer evidence update. I extended the same preserved path rule to missing I also reran linked worktree validation on a Linux devbox with system I fixed the devbox validation gap where blocked missing The rerun passed on the Linux devbox with system I also locally passed sandboxing crate tests, protocol crate tests, format check, and whitespace diff check. The branch remains one commit, now |
042c0ba to
18885a0
Compare
2daf148 to
bd1520f
Compare
fb6fcdf to
e76236e
Compare
10fa02e to
fa603bb
Compare
| handle_exit_status(status); | ||
| } | ||
|
|
||
| fn preserved_path_write_forbidden_reason( |
There was a problem hiding this comment.
Something feels off: this the logic that powers codex sandbox. This file should be small, as it should be calling into existing library code. This seems off.
There was a problem hiding this comment.
Agreed. I moved this out of the CLI sandbox entry point in the latest commit.
The CLI sandbox path now calls the shared preserved path write guard from the shell command crate, and the agent shell path calls the same helper. The debug sandbox file is back to being an entry point instead of owning the policy logic.
| "git" => git_init_preserved_path_write(command, cwd, file_system_sandbox_policy), | ||
| "touch" | "mkdir" | "rm" | "rmdir" | "ln" | "mv" | "cp" | "install" => command |
There was a problem hiding this comment.
Why are we special-casing things?
There was a problem hiding this comment.
This is for resolving parent/child .git folders security concern.
There was a problem hiding this comment.
I have since then moved this list into a policy configuration rather than having policy defined in code for separation of concerns.
…factors F-1: SANDBOX PATCH send_add_credits_nudge_email_inner -> early auth-required error. Upstream openai#18446 added a new /backend-api/add_credits_nudge_email call; copilot-api does not use it. backend_credit_type kept #[allow(dead_code)]. F-3: Reconcile type-level drift against new codex-model-provider crate: - codex-api/src/auth.rs: add on_unauthorized(&self) default no-op method on AuthProvider trait so Copilot retry path in core/src/client.rs can trigger invalidation through dyn AuthProvider. - codex-api/src/api_bridge.rs: switch CoreAuthProvider to impl crate::auth::AuthProvider (the upstream trait), and move on_unauthorized into the trait impl. - codex-api/src/lib.rs: pub use CoreAuthProvider + AuthProvider as ApiAuthProvider (alias for pre-refactor call sites). - core/src/client.rs: - ModelClientState.provider: ModelProviderInfo -> SharedModelProvider so .auth(), .api_provider(), .api_auth(), .auth_manager() trait methods resolve through create_model_provider(...). - ModelClient::new stores the wrapped provider (fixes stray 'provider,' shorthand). - is_copilot() uses .info().is_copilot(). - current_client_setup: Copilot branch re-inlined — build CopilotHeaderSource from the session-scoped OnceLock<Arc<CopilotAuth>> and wrap in CoreAuthProvider.with_copilot. Non-Copilot branch unchanged. - core/src/copilot_transport.rs: build_copilot_client now takes SharedAuthProvider and returns ResponsesClient<T> (upstream collapsed the auth generic on ResponsesClient). - core/src/compact_remote.rs: turn_context.provider now SharedModelProvider; use .info().is_copilot(). - cli/src/responses_cmd.rs: wrap CoreAuthProvider in Arc<dyn AuthProvider> via SharedAuthProvider alias to match the new build_copilot_client signature.
3145058 to
3ae2e24
Compare
3ae2e24 to
0293af6
Compare
05f4eb2
into
codex/bugb15632-runtime-permissions
|
Split this into a manual stack so each review surface is smaller.
PR 18446 is now the Linux child at the top of the stack. The final tree is unchanged from the rebased full branch, but the review surfaces are separated by policy, macOS, preflight, runtime propagation, and Linux. |
|
Follow up: the old auto merge setting carried over when I changed this PR base, so GitHub marked this old PR merged into the temporary parent branch. I restored the parent branch to its scoped runtime permission commit and opened #19852 as the current Linux child for review. Use #19852 for the active top of the stack. |
Summary
This PR protects preserved workspace metadata paths under workspace write sandboxes without breaking normal Git discovery. The preserved names are
.git,.codex, and.agents.BUGB 15632 tracks the Bugcrowd submission.
Root Cause
The previous policy only treated
.gitand.agentsas protected after they already existed. That left a first run gap where sandboxed code in a new workspace could create.git/config.The report used
git initfollowed by a Git config value that could persist on the host. A later host side Git command could then load attacker controlled repo metadata.The hard part is that missing
.gitcannot always be materialized as a read only placeholder. If Codex runs inside a subdirectory of an existing Git repo or worktree, materializing a child.gitchanges Git discovery and hides the valid parent repo. In that case the child.gitmust stay absent while creation is still denied.Design
preserved_path_names..git,.codex, or.agentsunless an explicit user write policy covers that preserved path..git,.codex, and.agentspaths stay read only under default writable roots..codexand.agentsare reserved under writable roots so first creation is blocked..gitunder a valid parent repo or worktree stays absent so normalgit statusandgit rev parsekeep discovering the parent./dev/null.protected_create_targets, runs bwrap with a parent helper only when cleanup or protected target checks are needed, and removes any forbidden preserved target that a sandboxed process managed to create before returning failure.touch,mkdir,rm,rmdir,ln,mv,cp,install, orgit init. It only catches literal shell redirection targets early for user feedback. The sandbox policy is the security boundary.Validation
Current PR head:
3ae2e24e72eb879d66de9e28b6f32226f52d5eae.Current base main:
bb83eec825b74aaf06f74650d2c004b0629dd19a.just fmt.just cfor each case.git initandmkdir .codexare denied, normal coding can create the requested JSONL viewer, and no child.git,.codex, or.agentsdirectory is left behind.