Repository navigation
fix(guard): read quoted wrapper values and long-option prefixes in the hard rules (#464) - #467
Conversation
…e hard rules (#464) `env --chd /x sudo id` and `stdbuf --out L sudo id` were allowed: the command-position regex and guard._switch_width treated any long option they did not know exactly as a flag, so its value was read as the program. `env -iC "/a b" sudo id` was denied only through the walk's opaque-site fallback; the regex took a value as \S+ and the stage parser split the quoted value in two. - policy: a wrapper switch, its value and a VAR=value assignment are read as shell words made of pieces (quoted run, escape, plain char) that each start with a different character, so a word still matches one way only. timeout's duration may be quoted. - policy/guard: a long value-taking switch matches by unique prefix among the wrapper's value-taking switches, in the regex, _switch_width and env --chdir tracking; an ambiguous prefix stays a flag. - guard: the stage parser keeps a quoted run as one word. - policy: _cmd_position_cost bounds a chain through a quoted or escaped word by the end of its line. - tests: new shapes join the clusters table (and so the parity test), the backtracking bound test, and unit tests for prefixes and the regex. - chore(release): 10.2.27 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zj1PCfsSSJgzLxi32jvde
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (2)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (20)
🧰 Additional context used📓 Path-based instructions (2)Source excerpt: This file and the GitHub **[Issues tab](https://github.com/CryptoJones/omind/issues)** are two views of the same list and must stay in sync.📄 CodeRabbit inference engine (BACKLOG.md) Files:
Source excerpt: The version is declared in three places that must stay in lockstep — `pyproject.toml`, `src/omind/__init__.py`, and `uv.lock` — and a test asserts `__version__` matches `pyproject.toml`, so a release can't leave one behind.📄 CodeRabbit inference engine (CONTRIBUTING.md) Files:
🪛 ast-grep (0.45.3)src/omind/guard.py[warning] 3364-3364: XPath query is request-/variable-derived; use parameterized XPath to prevent injection. (xpath-injection-python) tests/test_guard.py[warning] 4595-4595: XPath query is request-/variable-derived; use parameterized XPath to prevent injection. (xpath-injection-python) [warning] 4596-4596: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling. (redos-non-literal-regex-python) src/omind/policy.py[warning] 151-151: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling. (redos-non-literal-regex-python) [warning] 152-152: Regex pattern passed to re is built from a non-literal (variable, call, concatenation, or f-string) value. If that value is attacker-controlled it can introduce a malicious pattern with catastrophic backtracking (ReDoS). Use a hardcoded literal pattern, or validate/escape untrusted input with re.escape() and bound the regex complexity before compiling. (redos-non-literal-regex-python) 🪛 markdownlint-cli2 (0.23.2)CHANGELOG.md[warning] 12-12: Headings should be surrounded by blank lines (MD022, blanks-around-headings) [warning] 45-45: Spaces inside code span elements (MD038, no-space-in-code) 🔇 Additional comments (6)
📝 SummarySummary by CodeRabbit
WalkthroughWrapper parsing now handles quoted and escaped values, unique-prefix long options, and additional value-taking switches. Guard stage parsing and command-position search-cost estimates use shared shell-word parsing. Regression tests and the 10.2.27 release records cover these changes. ChangesWrapper parsing
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The reported regex concern does not affect current callers, and release versions are in sync. No actionable merge-blocking risk remains after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change improves consistency of shell-command checks. No newly introduced bypass was established, but unsupported shell syntax and incomplete runtime validation limit confidence. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
EM review: changes requested (Flatline Roundtable: HAL9000, Cortana, TheDixieFlatline)The two shapes the issue names are fixed. I verified each finding below with Must fix
Should fix
Dismissed
Version: at push time, set the version one patch above origin/main. #466 is merging as 10.2.27, so this becomes 10.2.28. Proudly Made in Nebraska. Go Big Red! 🌽 https://xkcd.com/2347/ |
#464 review) - Stage parser, hard-rule regex and _cmd_position_cost use one word definition (policy.SHELL_WORD_RE), so an escaped quote no longer hides a bash -c stage and an escaped blank lands both parsers on the same program. - _cmd_position_cost reads whole quoted words: the benign quoted one-liner is judged again, and a quoted duration after a switch value is a link. - Long twins for timeout, nice, ionice, time, xargs and sudo value switches. - command -\v is a lookup; a quoted lookup switch stays judged (fail closed). - Line continuation inside a wrapper value pinned out of scope (xfail strict). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zj1PCfsSSJgzLxi32jvde
EM review: response (commit 6995e63)The branch sits on current Must fix
Should fix
Verification
Proudly Made in Nebraska. Go Big Red! 🌽 https://xkcd.com/2347/ |
|
EM merge (#464). Reviewed by the Flatline Roundtable (HAL9000, Cortana, TheDixieFlatline), with one fix round. Verified on the final head (
EM accepted two reasoned dissents from the fixer:
Out of scope: the line continuation All 23 CI jobs are green. Releasing as 10.2.27. Proudly Made in Nebraska. Go Big Red! 🌽 https://xkcd.com/2347/ |
1 participant
Summary
Two wrapper-argument shapes kept
sudoout of command position for the hard rules (#464).env --chd /x sudo idandstdbuf --out L sudo idwere allowed. A long value-taking wrapper switch now matches by any prefix that no other value-taking switch of that wrapper shares, as GNU getopt_long reads it. This applies inpolicy._wrapper_pattern(new_long_prefixes), inguard._switch_width(reusing_long_optionfrom fix(guard): more write tools for the repo-work classifier (#450) #456) and in_env_chdir_value. An ambiguous prefix stays a plain flag.env --chd sudo echo hiis no longer a false positive.env -iC "/a b" sudo idandxargs -0I '{} x' sudo idwere denied only by accident, through the walk's opaque-site fallback. The regex itself took a value as\S+, and_program_stagessplit" "in two and landed on"as the program. Now:VAR=valueassignment are read as shell words. A word is made of pieces: a quoted run, a backslash escape or a plain character. Each kind of piece starts with a different character, so a word matches only one way and cannot backtrack.timeout "5"is accepted as a duration._cmd_position_costnow bounds a chain through a quoted or escaped link word by the end of its line, because no word piece crosses a newline. It does the same for an assignment after a separator inside a word.Stays out of
decide, the deadline and the fact-lookup layer (#466).Test evidence
CLUSTERED_WRAPPER_HARD_COMMANDS, so the guard hard rules: bundled wrapper option clusters (env -iC dir sudo …) are not read getopt-style #451 parity test covers them. 21 of the new cases fail on main and pass here.test_command_position_reads_a_quoted_wrapper_value_as_one_wordchecks the rule regex directly, not the fallback._switch_widthprefix widths._env_chdir_valuereads--chd.env --chd sudo echo hiandenv -C "sudo x" echo hi.test_wrapper_runs_cannot_backtrack_the_hard_rulesgained six runs:'env -iC "/a b" ' * 400,"xargs -0I '{} x' " * 400,'env -C "-a -b" ' * 400,"env --chd /x " * 400,'X="a b" ' * 400and'timeout "5" ' * 400.ruff check .: clean.mypy src: no issues in 58 files.pytest -q: 3752 passed, 12 skipped.perf446.py: at most 0.02 s.probe459.py: at most 0.02 s, and the budget verdict is unchanged.probe453.py: same results as main, including the existingsource <(grep -v sudo .env)FP.probe465.py: same results, with the cubic case at most 0.02 s.Closes #464
Proudly Made in Nebraska. Go Big Red! 🌽 https://xkcd.com/2347/
🤖 Generated with Claude Code
https://claude.ai/code/session_011Zj1PCfsSSJgzLxi32jvde