Elevated Sandbox 2 #7792

iceweasel-oai · 2025-12-09T23:01:19Z

DPAPI helpers for storing Sandbox user passwords securely
creation of Offline/Online sandbox users
ACL setup for sandbox users
firewall rule setup

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ 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-rs/windows-sandbox-rs/src/setup_main_win.rs

iceweasel-oai · 2025-12-10T03:58:06Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ 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-rs/windows-sandbox-rs/src/setup_main_win.rs

iceweasel-oai · 2025-12-10T04:52:43Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ 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-rs/windows-sandbox-rs/src/setup_main_win.rs

etraut-openai · 2025-12-10T19:06:46Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ 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".

chatgpt-codex-connector · 2025-12-10T19:12:29Z

codex-rs/windows-sandbox-rs/src/setup_main_win.rs

+    let offline_blob = dpapi_protect(offline_pwd.as_bytes())?;
+    let online_blob = dpapi_protect(online_pwd.as_bytes())?;


DPAPI blobs unusable for non-admin caller

Sandbox passwords are encrypted in the elevated helper with dpapi_protect (CryptProtectData default scope) and written to sandbox_users.json, but later are decrypted in identity::decode_password using dpapi::unprotect under the invoking user’s profile. If the setup runs via UAC with alternate admin credentials (a common case for non-admin users), the DPAPI blobs are bound to that admin account and cannot be decrypted by the original caller, so sandbox credential loading will consistently fail even though setup succeeded. Consider encrypting with machine scope or otherwise ensuring the same user context is used for both protect/unprotect.

Useful? React with 👍 / 👎.

@iceweasel-oai, this looks like a legit bug.

etraut-openai

I think this looks reasonable overall. I left a few comments and questions to consider. Before this functionality ships:

I think the "marker file" mechanism should be removed. It seems unnecessary and will result in a long tail of support issues if it's left in.
Like the current sandbox mechanism, this implementation "bleeds" access rights across instances of the CLI and extension. For example, if you start two instances of the CLI in two different directories, they will have write access to each other's workspace directory, but they shouldn't. At a minimum, this should be documented. Ideally, it should be fixed.
We're currently adding DACLs and allocating users, but we never clean up these artifacts. Since there's no uninstaller, these are left on the user's machine, contributing to system rot. We should think about ways to mitigate this.
Since the new mechanism involves escalation, I think it needs to go through a thorough security review and preferably include a formal threat model.

etraut-openai · 2025-12-10T19:06:10Z

codex-rs/exec-server/tests/suite/accept_elicitation.rs

 /// command should be run privileged outside the sandbox.
+// Flaky: failing on some runners due to codex-linux-sandbox not being present.
+#[ignore]
 #[tokio::test(flavor = "current_thread")]


bolinfest has been working to fix for this, so this can be removed.

etraut-openai · 2025-12-10T19:39:33Z

codex-rs/windows-sandbox-rs/src/setup_main_win.rs

+    let offline_blob = dpapi_protect(offline_pwd.as_bytes())?;
+    let online_blob = dpapi_protect(online_pwd.as_bytes())?;


@iceweasel-oai, this looks like a legit bug.

etraut-openai · 2025-12-10T20:48:50Z

codex-rs/windows-sandbox-rs/src/identity.rs

+    pub password: String,
+}
+
+fn load_marker(codex_home: &Path) -> Result<Option<SetupMarker>> {


This "marker file" approach strikes me as overly complicated and fragile. I think we can eliminate this entirely. Can't we just run checks at startup that validate that the appropriate ACLs are in place and elevate if we need to? This seems like a much more robust and self-healing approach.

etraut-openai · 2025-12-11T01:17:09Z

codex-rs/windows-sandbox-rs/src/setup_main_win.rs

+    let users = SandboxUsersFile {
+        version: SETUP_VERSION,
+        offline: SandboxUserRecord {
+            username: offline_user.to_string(),


It looks like the usernames for the two accounts are constants. Is there a reason we're storing them to the file?

etraut-openai · 2025-12-11T01:18:23Z

codex-rs/windows-sandbox-rs/src/setup_main_win.rs

+    let ret = real_main();
+    if let Err(e) = &ret {
+        // Best-effort: log unexpected top-level errors.
+        if let Ok(codex_home) = std::env::var("CODEX_HOME") {


How will these errors be relayed to the end user in a way that they can understand the problem and take action?

etraut-openai · 2025-12-11T01:19:11Z

codex-rs/windows-sandbox-rs/src/setup_main_win.rs

+    ret
+}
+
+fn real_main() -> Result<()> {


Is there perhaps a better name for this function?

chatgpt-codex-connector bot reviewed Dec 9, 2025

View reviewed changes

codex-rs/windows-sandbox-rs/src/setup_main_win.rs Show resolved Hide resolved

codex-rs/windows-sandbox-rs/src/setup_main_win.rs Show resolved Hide resolved

iceweasel-oai force-pushed the dev/iceweasel/elevated-sandbox-2 branch from c483107 to a7e1dd6 Compare December 10, 2025 03:08

chatgpt-codex-connector bot reviewed Dec 10, 2025

View reviewed changes

codex-rs/windows-sandbox-rs/src/setup_main_win.rs Show resolved Hide resolved

chatgpt-codex-connector bot reviewed Dec 10, 2025

View reviewed changes

codex-rs/windows-sandbox-rs/src/setup_main_win.rs Show resolved Hide resolved

iceweasel-oai assigned dylan-hurd-oai and etraut-openai Dec 10, 2025

chatgpt-codex-connector bot reviewed Dec 10, 2025

View reviewed changes

etraut-openai approved these changes Dec 11, 2025

View reviewed changes

iceweasel-oai added 17 commits December 10, 2025 20:50

Windows sandbox groundwork prep

75c1455

clippy fixes

2049008

Windows sandbox setup assets

918bb06

Wire setup modules and dpapi exports

01db1fc

Group Windows exports under a single cfg

34fec61

clippy cleanup

2b08d36

merge mask-checking functions

232eb05

rename setup modules, more clippy fixes

335fa01

gate setup_main for windows only

e112aa4

cargo fmt prefers this I guess

4080e23

remove unused code

ce85529

remove unused code, proper windows gating for setup main

a00178e

forgot to add a file

2645376

fix windows gating

ba09b5d

fix gating, skip flaky test

5cc1f45

include delete bits in ACL mask

ecc89d6

un-ignore test

6aa57a5

iceweasel-oai force-pushed the dev/iceweasel/elevated-sandbox-2 branch from dcc0ef9 to 6aa57a5 Compare December 11, 2025 04:55

revert lockfile changes

3a05930

iceweasel-oai merged commit 13c0919 into main Dec 11, 2025
26 checks passed

iceweasel-oai deleted the dev/iceweasel/elevated-sandbox-2 branch December 11, 2025 05:23

github-actions bot locked and limited conversation to collaborators Dec 11, 2025

		let offline_blob = dpapi_protect(offline_pwd.as_bytes())?;
		let online_blob = dpapi_protect(online_pwd.as_bytes())?;

Elevated Sandbox 2 #7792

Elevated Sandbox 2 #7792

Conversation

iceweasel-oai commented Dec 9, 2025

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

iceweasel-oai commented Dec 10, 2025

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

iceweasel-oai commented Dec 10, 2025

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

etraut-openai commented Dec 10, 2025

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

etraut-openai Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

etraut-openai left a comment

Choose a reason for hiding this comment

Uh oh!

etraut-openai Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

etraut-openai Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

etraut-openai Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

etraut-openai Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

etraut-openai Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

etraut-openai Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants