Load exec policy rules from requirements #10190
Conversation
gt-oai
force-pushed
the
gt/reqs-to-rules
branch
from
15e69f0 to
9ca5a59
Compare
bolinfest
left a comment
bolinfest
left a comment
There was a problem hiding this comment.
Please fill out the PR body.
| cmd.iter().map(std::string::ToString::to_string).collect() | ||
| } | ||
|
|
||
| fn allow_all(_: &[String]) -> Decision { |
| #[test] | ||
| fn deserialize_exec_policy_requirements() -> Result<()> { | ||
| let toml_str = r#" | ||
| [exec_policy] |
There was a problem hiding this comment.
We should not use exec_policy in any API since we have moved to "rules" as the public-facing name?
Though honestly, it may soon become permissions...
| let requirements_toml_file = if cfg!(windows) { | ||
| "C:\\etc\\codex\\requirements.toml" | ||
| } else { | ||
| "/etc/codex/requirements.toml" | ||
| }; |
There was a problem hiding this comment.
Should this be in a helper function or something?
There was a problem hiding this comment.
Wasn't material to the test
| #[error("exec policy prefix_rule at index {rule_index} has an empty justification")] | ||
| EmptyJustification { rule_index: usize }, | ||
|
|
||
| #[error("exec policy prefix_rule at index {rule_index} is missing a decision")] |
| "#; | ||
|
|
||
| let parsed: RequirementsExecPolicyToml = from_str(toml_str)?; | ||
| let err = parsed.to_policy().expect_err("allow decision not allowed"); |
There was a problem hiding this comment.
Why isn't the provider of requirements.toml allowed to declare a rule with decision = "allow"?
There was a problem hiding this comment.
Clarified the error message as to why this is the case
| #[test] | ||
| fn empty_prefix_rules_is_rejected() -> anyhow::Result<()> { | ||
| let toml_str = r#" | ||
| prefix_rules = [] |
There was a problem hiding this comment.
Couldn't this mean that an end-user is not allowed to define any rules?
| return Ok(policy); | ||
| }; | ||
|
|
||
| let mut combined_rules = policy.rules().clone(); |
There was a problem hiding this comment.
I don't think we should combine rules in this case, should we? Shouldn't requirements_policy be the only rules you can set?
There was a problem hiding this comment.
IMO users (and repo config) should be allowed to set rules that are more restrictive than the ones in requirements_policy. Rules already apply the most restrictive matching decision, so additional rules feels harmless to me.
I cannot think of a genuine use for "admins can require that command X is always allowed, regardless of what the user wants". Perhaps it would be good to e.g. always allow bazel build or something, but that feels more in the camp of sensible default that should be included in the repo, rather than a strict requirement that prevents a user's other rules from loading.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ 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". |
gt-oai
force-pushed
the
gt/reqs-to-rules
branch
3 times, most recently
from
c175b27 to
50bef97
Compare
gt-oai
force-pushed
the
gt/reqs-to-rules
branch
from
50bef97 to
6c2261a
Compare
requirements.tomlshould be able to specify rules which always run.My intention here was that these rules could only ever be restrictive, which means the decision can be "prompt" or "forbidden" but never "allow". A requirement of "you must always allow this command" didn't make sense to me, but happy to be gaveled otherwise.
Rules already applies the most restrictive decision, so we can safely merge these with rules found in other config folders.