mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
hook trust metadata and enforcement (#20321)
# Why We want shared hook trust that both the app and the TUI can build on, but the metadata is only useful if runtime behavior agrees with it. This PR adds a single backend trust model for hooks so unmanaged hooks cannot run until the current definition has been reviewed, while managed hooks remain runnable and non-configurable. # What - persist `trusted_hash` alongside hook state in `config.toml` - expose `currentHash` and derived `trustStatus` through `hooks/list` - derive trust from normalized hook definitions so equivalent hooks from `config.toml` and `hooks.json` share the same trust identity - gate unmanaged hooks on trust before they enter the runnable handler set # Reviewer Notes - key file to review is `codex-rs/hooks/src/engine/discovery.rs` - the only **core** change is schema related
This commit is contained in:
committed by
GitHub
Unverified
parent
78421face0
commit
0452dca986
@@ -22,6 +22,7 @@ use codex_protocol::ThreadId;
|
||||
use codex_protocol::protocol::HookOutputEntryKind;
|
||||
use codex_protocol::protocol::HookRunStatus;
|
||||
use codex_protocol::protocol::HookSource;
|
||||
use codex_protocol::protocol::HookTrustStatus;
|
||||
use pretty_assertions::assert_eq;
|
||||
use tempfile::tempdir;
|
||||
|
||||
@@ -121,7 +122,7 @@ with Path(r"{log_path}").open("a", encoding="utf-8") as handle:
|
||||
|
||||
assert!(engine.warnings().is_empty());
|
||||
assert_eq!(engine.handlers.len(), 1);
|
||||
assert!(engine.handlers[0].source.is_managed());
|
||||
assert_eq!(engine.handlers[0].source, HookSource::CloudRequirements);
|
||||
let listed = crate::list_hooks(crate::HooksConfig {
|
||||
legacy_notify_argv: None,
|
||||
feature_enabled: true,
|
||||
@@ -168,6 +169,68 @@ with Path(r"{log_path}").open("a", encoding="utf-8") as handle:
|
||||
assert!(log_contents.contains("\"hook_event_name\": \"PreToolUse\""));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn unknown_requirement_source_hooks_stay_managed() {
|
||||
let temp = tempdir().expect("create temp dir");
|
||||
let managed_dir =
|
||||
AbsolutePathBuf::try_from(temp.path().join("managed-hooks")).expect("absolute path");
|
||||
fs::create_dir_all(managed_dir.as_path()).expect("create managed hooks dir");
|
||||
let managed_hooks = managed_hooks_for_current_platform(
|
||||
managed_dir,
|
||||
HookEventsToml {
|
||||
pre_tool_use: vec![MatcherGroup {
|
||||
matcher: Some("^Bash$".to_string()),
|
||||
hooks: vec![HookHandlerConfig::Command {
|
||||
command: "python3 /tmp/managed.py".to_string(),
|
||||
timeout_sec: Some(10),
|
||||
r#async: false,
|
||||
status_message: Some("checking".to_string()),
|
||||
}],
|
||||
}],
|
||||
..Default::default()
|
||||
},
|
||||
);
|
||||
let config_layer_stack = ConfigLayerStack::new(
|
||||
Vec::new(),
|
||||
ConfigRequirements {
|
||||
managed_hooks: Some(ConstrainedWithSource::new(
|
||||
Constrained::allow_any(managed_hooks.clone()),
|
||||
Some(RequirementSource::Unknown),
|
||||
)),
|
||||
..ConfigRequirements::default()
|
||||
},
|
||||
ConfigRequirementsToml {
|
||||
hooks: Some(managed_hooks),
|
||||
..ConfigRequirementsToml::default()
|
||||
},
|
||||
)
|
||||
.expect("config layer stack");
|
||||
|
||||
let engine = ClaudeHooksEngine::new(
|
||||
/*enabled*/ true,
|
||||
Some(&config_layer_stack),
|
||||
Vec::new(),
|
||||
Vec::new(),
|
||||
CommandShell {
|
||||
program: String::new(),
|
||||
args: Vec::new(),
|
||||
},
|
||||
);
|
||||
|
||||
assert_eq!(engine.handlers.len(), 1);
|
||||
assert_eq!(engine.handlers[0].source, HookSource::Unknown);
|
||||
let discovered =
|
||||
super::discovery::discover_handlers(Some(&config_layer_stack), Vec::new(), Vec::new());
|
||||
assert_eq!(discovered.hook_entries.len(), 1);
|
||||
assert_eq!(discovered.hook_entries[0].source, HookSource::Unknown);
|
||||
assert_eq!(discovered.hook_entries[0].enabled, true);
|
||||
assert_eq!(discovered.hook_entries[0].is_managed, true);
|
||||
assert_eq!(
|
||||
discovered.hook_entries[0].trust_status,
|
||||
HookTrustStatus::Managed
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn user_disablement_filters_non_managed_hooks_but_not_managed_hooks() {
|
||||
let temp = tempdir().expect("create temp dir");
|
||||
@@ -228,13 +291,17 @@ fn user_disablement_filters_non_managed_hooks_but_not_managed_hooks() {
|
||||
);
|
||||
|
||||
assert_eq!(engine.handlers.len(), 1);
|
||||
assert!(engine.handlers[0].source.is_managed());
|
||||
assert_eq!(engine.handlers[0].source, HookSource::CloudRequirements);
|
||||
let discovered =
|
||||
super::discovery::discover_handlers(Some(&config_layer_stack), Vec::new(), Vec::new());
|
||||
assert_eq!(discovered.hook_entries.len(), 2);
|
||||
assert_eq!(discovered.hook_entries[0].key, managed_disabled_key);
|
||||
assert_eq!(discovered.hook_entries[0].enabled, true);
|
||||
assert!(discovered.hook_entries[0].is_managed);
|
||||
assert_eq!(
|
||||
discovered.hook_entries[0].trust_status,
|
||||
HookTrustStatus::Managed
|
||||
);
|
||||
assert_eq!(discovered.hook_entries[1].key, user_disabled_key);
|
||||
assert_eq!(discovered.hook_entries[1].enabled, false);
|
||||
assert!(!discovered.hook_entries[1].is_managed);
|
||||
@@ -281,13 +348,20 @@ fn user_disablement_does_not_filter_managed_layer_hooks() {
|
||||
);
|
||||
|
||||
assert_eq!(engine.handlers.len(), 1);
|
||||
assert!(engine.handlers[0].source.is_managed());
|
||||
assert_eq!(
|
||||
engine.handlers[0].source,
|
||||
HookSource::LegacyManagedConfigFile
|
||||
);
|
||||
let discovered =
|
||||
super::discovery::discover_handlers(Some(&config_layer_stack), Vec::new(), Vec::new());
|
||||
assert_eq!(discovered.hook_entries.len(), 1);
|
||||
assert_eq!(discovered.hook_entries[0].key, managed_key);
|
||||
assert_eq!(discovered.hook_entries[0].enabled, true);
|
||||
assert!(discovered.hook_entries[0].is_managed);
|
||||
assert_eq!(
|
||||
discovered.hook_entries[0].trust_status,
|
||||
HookTrustStatus::Managed
|
||||
);
|
||||
}
|
||||
|
||||
fn config_with_hook_state(key: &str, enabled: bool) -> TomlValue {
|
||||
@@ -339,6 +413,45 @@ fn config_with_pre_tool_use_hook(command: &str) -> TomlValue {
|
||||
.expect("config TOML should deserialize")
|
||||
}
|
||||
|
||||
fn trusted_plugin_hook_stack(
|
||||
config_path: AbsolutePathBuf,
|
||||
plugin_hook_sources: &[PluginHookSource],
|
||||
) -> ConfigLayerStack {
|
||||
let discovered = super::discovery::discover_handlers(
|
||||
/*config_layer_stack*/ None,
|
||||
plugin_hook_sources.to_vec(),
|
||||
Vec::new(),
|
||||
);
|
||||
let state = discovered
|
||||
.hook_entries
|
||||
.into_iter()
|
||||
.map(|entry| {
|
||||
(
|
||||
entry.key,
|
||||
serde_json::json!({
|
||||
"trusted_hash": entry.current_hash,
|
||||
}),
|
||||
)
|
||||
})
|
||||
.collect::<serde_json::Map<_, _>>();
|
||||
let config = serde_json::from_value(serde_json::json!({
|
||||
"hooks": {
|
||||
"state": state,
|
||||
},
|
||||
}))
|
||||
.expect("config TOML should deserialize");
|
||||
|
||||
ConfigLayerStack::new(
|
||||
vec![ConfigLayerEntry::new(
|
||||
ConfigLayerSource::User { file: config_path },
|
||||
config,
|
||||
)],
|
||||
ConfigRequirements::default(),
|
||||
ConfigRequirementsToml::default(),
|
||||
)
|
||||
.expect("config layer stack")
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn requirements_managed_hooks_warn_when_managed_dir_is_missing() {
|
||||
let temp = tempdir().expect("create temp dir");
|
||||
@@ -473,7 +586,7 @@ fn discovers_hooks_from_json_and_toml_in_the_same_layer() {
|
||||
config_table.insert("hooks".to_string(), hooks_table);
|
||||
let config_layer_stack = ConfigLayerStack::new(
|
||||
vec![ConfigLayerEntry::new(
|
||||
ConfigLayerSource::User {
|
||||
ConfigLayerSource::System {
|
||||
file: config_path.clone(),
|
||||
},
|
||||
config_toml,
|
||||
@@ -514,11 +627,13 @@ fn discovers_hooks_from_json_and_toml_in_the_same_layer() {
|
||||
tool_input: serde_json::json!({ "command": "echo hello" }),
|
||||
});
|
||||
assert_eq!(preview.len(), 2);
|
||||
assert!(
|
||||
assert_eq!(
|
||||
engine
|
||||
.handlers
|
||||
.iter()
|
||||
.all(|handler| !handler.source.is_managed())
|
||||
.map(|handler| handler.source)
|
||||
.collect::<Vec<_>>(),
|
||||
vec![HookSource::System, HookSource::System]
|
||||
);
|
||||
assert_eq!(preview[0].source_path, hooks_json_path);
|
||||
assert_eq!(preview[1].source_path, config_path);
|
||||
@@ -567,9 +682,13 @@ print(json.dumps({
|
||||
..Default::default()
|
||||
},
|
||||
}];
|
||||
let config_layer_stack = trusted_plugin_hook_stack(
|
||||
AbsolutePathBuf::try_from(temp.path().join("config.toml")).expect("absolute config path"),
|
||||
&plugin_hook_sources,
|
||||
);
|
||||
let engine = ClaudeHooksEngine::new(
|
||||
/*enabled*/ true,
|
||||
/*config_layer_stack*/ None,
|
||||
Some(&config_layer_stack),
|
||||
plugin_hook_sources.clone(),
|
||||
Vec::new(),
|
||||
CommandShell {
|
||||
@@ -671,9 +790,13 @@ fn plugin_hook_sources_expand_plugin_placeholders() {
|
||||
..Default::default()
|
||||
},
|
||||
}];
|
||||
let config_layer_stack = trusted_plugin_hook_stack(
|
||||
AbsolutePathBuf::try_from(temp.path().join("config.toml")).expect("absolute config path"),
|
||||
&plugin_hook_sources,
|
||||
);
|
||||
let engine = ClaudeHooksEngine::new(
|
||||
/*enabled*/ true,
|
||||
/*config_layer_stack*/ None,
|
||||
Some(&config_layer_stack),
|
||||
plugin_hook_sources,
|
||||
Vec::new(),
|
||||
CommandShell {
|
||||
|
||||
Reference in New Issue
Block a user