mirror of
https://github.com/openai/codex.git
synced 2026-10-27 18:48:37 +00:00
Invalidate cached Guardian approvals for unscored permission widening (#44617)
## Why An unscored `exec_command` requesting additional permissions could reuse an earlier Guardian v2 approval score, even though that score did not cover the expanded permissions. ## What changed When `UnscoredAction::AgeScore` applies, invalidate the cached score for default-namespace `exec_command` calls with `sandbox_permissions` set to `with_additional_permissions` by marking the call as a scoring failure. ## Testing Extend the review-scope regression test to verify that an ordinary sandboxed command preserves cached approval, while a command requesting additional network permissions clears it and causes review to fail closed with `scoring_failure`. GitOrigin-RevId: d6e3e1ec7d618dff2ef03ffcb92869249f0e547a
This commit is contained in:
@@ -32,6 +32,7 @@ use codex_extension_api::ThreadStartInput;
|
||||
use codex_extension_api::ToolFinishInput;
|
||||
use codex_extension_api::ToolLifecycleContributor;
|
||||
use codex_extension_api::ToolLifecycleFuture;
|
||||
use codex_extension_api::ToolPayload;
|
||||
use codex_extension_api::ToolStartInput;
|
||||
use codex_features::Feature;
|
||||
use codex_guardian_context::ContextTarget;
|
||||
@@ -277,6 +278,20 @@ impl GuardianV2Extension {
|
||||
.fetch_add(/*val*/ 1, Ordering::Relaxed)
|
||||
.saturating_add(/*rhs*/ 1);
|
||||
score_progress.wrapper_lag.record(&input, index);
|
||||
// Unscored permission widening must not reuse an earlier approval score.
|
||||
if input.tool_name.is_default_namespace()
|
||||
&& input.tool_name.name == "exec_command"
|
||||
&& let ToolPayload::Function { arguments } = input.payload
|
||||
&& let Ok(arguments) = serde_json::from_str::<serde_json::Value>(arguments)
|
||||
&& arguments
|
||||
.get("sandbox_permissions")
|
||||
.and_then(serde_json::Value::as_str)
|
||||
== Some("with_additional_permissions")
|
||||
{
|
||||
score_progress
|
||||
.latest_failed_tool_call
|
||||
.fetch_max(index, Ordering::Release);
|
||||
}
|
||||
}
|
||||
UnscoredAction::InvalidateScore => {
|
||||
let index = score_progress
|
||||
|
||||
@@ -490,7 +490,7 @@ async fn sandboxed_shell_classification_respects_review_scope() -> Result<()> {
|
||||
arguments: r#"{"cmd":"pwd"}"#.to_owned(),
|
||||
};
|
||||
let additional_permissions = ToolPayload::Function {
|
||||
arguments: r#"{"cmd":"pwd","sandbox_permissions":"with_additional_permissions"}"#
|
||||
arguments: r#"{"cmd":"pwd","sandbox_permissions":"with_additional_permissions","additional_permissions":{"network":{"enabled":true}}}"#
|
||||
.to_owned(),
|
||||
};
|
||||
let unsandboxed = ToolPayload::Function {
|
||||
@@ -539,6 +539,13 @@ async fn sandboxed_shell_classification_respects_review_scope() -> Result<()> {
|
||||
|
||||
let fixture = GuardianFailureFixture::new().await?;
|
||||
let thread_store = fixture.test.codex.thread_extension_data();
|
||||
let mut score = thread_store
|
||||
.get::<SecurityRiskScore>()
|
||||
.expect("fixture should publish a score")
|
||||
.as_ref()
|
||||
.clone();
|
||||
score.scores.insert("action_risk".to_owned(), 0.0);
|
||||
thread_store.insert(score);
|
||||
let score_progress = thread_store
|
||||
.get::<GuardianV2ScoreProgress>()
|
||||
.expect("Guardian v2 should track score progress per thread");
|
||||
@@ -547,30 +554,42 @@ async fn sandboxed_shell_classification_respects_review_scope() -> Result<()> {
|
||||
.load(Ordering::Acquire);
|
||||
let turn_store = ExtensionData::new("turn-1");
|
||||
let tool_name = ToolName::plain("exec_command");
|
||||
let payload = ToolPayload::Function {
|
||||
arguments: r#"{"cmd":"pwd"}"#.to_owned(),
|
||||
};
|
||||
for (call_id, payload, expected_decision) in [
|
||||
("call-2", sandboxed, Some(ReviewDecision::Approved)),
|
||||
("call-3", additional_permissions, None),
|
||||
] {
|
||||
fixture.registry.tool_lifecycle_contributors()[0]
|
||||
.on_tool_start(ToolStartInput {
|
||||
session_store: &fixture.session_store,
|
||||
thread_store,
|
||||
turn_store: &turn_store,
|
||||
turn_id: "turn-1",
|
||||
root_turn_id: None,
|
||||
call_id,
|
||||
originating_item_id: None,
|
||||
tool_name: &tool_name,
|
||||
mcp_tool: None,
|
||||
payload: &payload,
|
||||
conversation_history: Arc::new(TestConversationHistory(Vec::new())),
|
||||
source: ToolCallSource::Direct,
|
||||
})
|
||||
.await;
|
||||
|
||||
fixture.registry.tool_lifecycle_contributors()[0]
|
||||
.on_tool_start(ToolStartInput {
|
||||
session_store: &fixture.session_store,
|
||||
thread_store,
|
||||
turn_store: &turn_store,
|
||||
turn_id: "turn-1",
|
||||
root_turn_id: None,
|
||||
call_id: "call-2",
|
||||
originating_item_id: None,
|
||||
tool_name: &tool_name,
|
||||
mcp_tool: None,
|
||||
payload: &payload,
|
||||
conversation_history: Arc::new(TestConversationHistory(Vec::new())),
|
||||
source: ToolCallSource::Direct,
|
||||
})
|
||||
.await;
|
||||
assert_eq!(
|
||||
cached_approval(
|
||||
&fixture.registry,
|
||||
thread_store,
|
||||
r#"{"tool":"exec_command","cmd":"pwd"}"#,
|
||||
/*metrics*/ None,
|
||||
)
|
||||
.await,
|
||||
expected_decision,
|
||||
);
|
||||
}
|
||||
|
||||
assert_eq!(
|
||||
score_progress.latest_tool_call.load(Ordering::Acquire),
|
||||
latest_scored_tool_call + 1
|
||||
latest_scored_tool_call + 2
|
||||
);
|
||||
assert_eq!(
|
||||
score_progress
|
||||
@@ -578,6 +597,7 @@ async fn sandboxed_shell_classification_respects_review_scope() -> Result<()> {
|
||||
.load(Ordering::Acquire),
|
||||
latest_scored_tool_call
|
||||
);
|
||||
fixture.assert_fails_closed("scoring_failure").await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user