Skip to content

fix(core) disable command_might_be_dangerous when unsandboxed#15036

Merged
dylan-hurd-oai merged 4 commits intomainfrom
dh--sandbox--dangerous-commands
Mar 21, 2026
Merged

fix(core) disable command_might_be_dangerous when unsandboxed#15036
dylan-hurd-oai merged 4 commits intomainfrom
dh--sandbox--dangerous-commands

Conversation

@dylan-hurd-oai
Copy link
Copy Markdown
Collaborator

Summary

If we are in a mode that is already explicitly un-sandboxed, then ApprovalPolicy::Never should not block dangerous commands.

Testing

  • Existing unit test covers old behavior
  • Added a unit test for this new case

AskForApproval::Never => Decision::Forbidden,
AskForApproval::Never => {
if sandbox_is_explicitly_disabled {
// If the sandbox is explicitly disabled, we should allow the command to run
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could also be because no sandbox is available, like for Windows users who have not installed our sandbox?

Copy link
Copy Markdown
Collaborator Author

@dylan-hurd-oai dylan-hurd-oai Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I might be misunderstanding the intent of FileSystemSandboxKind. Should we instead use the following logic?

let sandbox_is_explicitly_disabled =  matches!(
    sandbox_policy,
    SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. },
)

I've updated to switch to this instead

@bolinfest
Copy link
Copy Markdown
Collaborator

Here's something that has been on my personal backlog:

Introduce a NoSandboxAvailable variant to SandboxMode and make sure ReadOnly means what we think it means everywhere it is used.

Currently, because we cannot distinguish between NoSandboxAvailable and FileSystemSandboxKind::Unrestricted, I don't think this change is safe.

Also, the point about prefix_rule() still stands, though I admit that if the agent is truly running unsandboxed and it is determined to do something, prefix_rule() is unlikely to stop it unless it's a setuid binary or something that can take an action as root that the agent has no other way of running...

@dylan-hurd-oai
Copy link
Copy Markdown
Collaborator Author

dylan-hurd-oai commented Mar 18, 2026

re: prefix_rule(), I don't think this change supersedes any ExecPolicy decisions, since this change is in render_decision_for_unmatched_command - I added an additional test to confirm this case.

);
}

struct ExecApprovalRequirementScenario {
Copy link
Copy Markdown
Collaborator

@bolinfest bolinfest Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this a bit confusing since most of the fields are "inputs" to the assertion whereas ExecApprovalRequirement is the only output.

What about dropping expected_requirement from the struct and doing:

async fn assert_exec_approval_requirement_for_command(
  scenario: ExecApprovalRequirementScenario,
  expected_requirement: ExecApprovalRequirement,
) {

I think that makes it clearer from the call site what is being asserted.

Or even have assert_exec_approval_requirement_for_command() return ExecApprovalRequirement and then have an assert_eq!() so you can pass a string as the third argument explaining the assertion.

@dylan-hurd-oai dylan-hurd-oai force-pushed the dh--sandbox--dangerous-commands branch from bbf12a8 to cd4a4e0 Compare March 21, 2026 01:02
@dylan-hurd-oai dylan-hurd-oai enabled auto-merge (squash) March 21, 2026 01:23
@dylan-hurd-oai dylan-hurd-oai merged commit 60c59a7 into main Mar 21, 2026
36 checks passed
@dylan-hurd-oai dylan-hurd-oai deleted the dh--sandbox--dangerous-commands branch March 21, 2026 01:28
@github-actions github-actions bot locked and limited conversation to collaborators Mar 21, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants