Skip to content

fix(policy): require all brace-expanded allow paths#39

Merged
StressTestor merged 2 commits into
mainfrom
codex/fix-allow-list-bypass-due-to-brace-expansion
Jun 23, 2026
Merged

fix(policy): require all brace-expanded allow paths#39
StressTestor merged 2 commits into
mainfrom
codex/fix-allow-list-bypass-due-to-brace-expansion

Conversation

@StressTestor

Copy link
Copy Markdown
Owner

Motivation

  • Brace expansion was applied existentially in the shared path matcher so an allow rule could succeed if any single expansion matched, letting mixed brace expressions (e.g. {/repo/src/main.rs,/tmp/secret}) bypass a default = "block" allow-list; allow rules must instead require that every runtime-expanded path is allowed.

Description

  • Make matches_allow_path call a new matches_path_env_all_expansions helper so allow semantics require universal coverage across brace expansions instead of existential matching of any expansion. (matches_allow_path, matches_path_env_all_expansions, matches_path_resolved_all_expansions, path_expansions_match_any in src/policy/matcher.rs).
  • Preserve the prior deny behavior (existential match) by keeping matches_path_resolved semantics unchanged for deny rules.
  • Update policy enforcement to brace-expand each extracted path token and require every expanded runtime path to be covered by at least one allow rule before deferring to the default action (changes in src/policy/mod.rs).
  • Add regression tests covering: deny-vs-allow brace semantics, the reported allow-list bypass shape, and multi-rule allowed expansions (tests in src/policy/matcher.rs and src/policy/mod.rs).

Testing

  • Ran the full test suite with cargo test and all tests passed (255 passed; 0 failed).
  • Ran the new/targeted tests individually with cargo test allow_list_checks_each_brace_expanded_runtime_path, cargo test allow_list_accepts_brace_expansions_covered_by_different_rules, and cargo test deny_brace_expansion_matches_any_target_but_allow_requires_all, and each passed.
  • Ran git diff --check (no issues) and noted cargo fmt --check reported repository-wide formatting differences unrelated to this minimal security fix, so formatting changes were not applied to avoid noisy unrelated edits.

Codex Task

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e1583a1480

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

Comment thread src/policy/mod.rs Outdated
reason: Some(format!("path {path} not in allow list")),
matched_rule: Some("allow.paths (miss)".into()),
};
for expanded_path in crate::common::shell::brace_expand(path, 64) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Treat truncated brace expansions as an allow-list miss

In lockdown policies, this only checks the first 64 strings returned by brace_expand(path, 64), and src/common/shell.rs shows that helper silently stops once the cap is reached. A crafted brace list with 64 allowed alternatives followed by /tmp/secret is still expanded by the shell at runtime, but the new allow-list loop never sees the 65th path and can return Allow, leaving the same bypass shape available with a larger brace list. The check should fail closed when expansion is truncated or otherwise ensure every runtime expansion is accounted for.

Useful? React with 👍 / 👎.

@StressTestor StressTestor force-pushed the codex/fix-allow-list-bypass-due-to-brace-expansion branch from e1583a1 to b99e3c1 Compare June 23, 2026 08:52
@StressTestor StressTestor merged commit 47f029c into main Jun 23, 2026
4 checks passed
@StressTestor StressTestor deleted the codex/fix-allow-list-bypass-due-to-brace-expansion branch June 23, 2026 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant