Reserve missing preserved paths in Linux sandbox policy#18446
Reserve missing preserved paths in Linux sandbox policy#18446evawong-oai wants to merge 5 commits intomainfrom
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
be03010 to
0cbf816
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be03010efb
ℹ️ 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".
68debc4 to
260e53d
Compare
13dff2c to
16eeec0
Compare
06ad689 to
d4502ef
Compare
d4502ef to
7681996
Compare
Represent missing .agents and .codex preserved path masks as read-only empty directories instead of empty files so Git ignores them while cleanup still removes the host mount targets after bwrap exits. Co-authored-by: Codex <noreply@openai.com>
374d917 to
10fa02e
Compare
Co-authored-by: Codex <noreply@openai.com>
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.
| "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?
…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.
Summary
This PR protects preserved workspace metadata paths under workspace write sandboxes without breaking normal Git discovery. The preserved paths 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 handled the same way as.codexand.agents. 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, and creation must be blocked before the sandboxed command runs.Design
.git,.codex, and.agentspaths stay read only under default writable roots..codexand.agentsare reserved under writable roots so first creation is blocked..gitis reserved only when doing so does not break Git discovery..gitunder a valid parent repo or worktree stays absent so normalgit statusandgit rev parsekeep discovering the parent.codex sandboxand normal Codex shell tool execution.git init, common file commands, and literal shell redirection targets./dev/null, treats transient empty preserved files as synthetic when another active sandbox owns them, and cleans synthetic placeholders after bwrap exits.Reviewer Collaboration
.gitmaterialization broke parent repo discovery. The PR now leaves child.gitabsent under a valid ancestor repo or worktree and blocks creation through the command guard..git,.codex, and.agentsplaceholders. The PR now treats empty preserved paths with active synthetic ownership as synthetic and removes them when the last owner exits, while preserving real prior empty files.codex sandboxentrypoint had too much preserved path policy logic. The latest head moves that logic into shared shell command code and keeps the debug sandbox entrypoint as a caller of the shared guard.Validation
Current PR head:
f8f8584923d3e1b9d9e19ffb1d67cb60de96a876.git initandmkdir .codexare blocked, normal JSONL viewer coding works, and no.git,.codex, or.agentsplaceholders remain afterward.codex sandboxand into shared shell command code, then wires the CLI and core shell paths through the same guard.