fix(rules): match FlagWithValue patterns against =-joined tokens#180
fix(rules): match FlagWithValue patterns against =-joined tokens#180
FlagWithValue patterns against =-joined tokens#180Conversation
`FlagWithValue` patterns like `[--sort *]` only matched space-separated forms (`--sort value`) but not `=`-joined forms (`--sort=value`). This caused commands like `git branch -a --sort=-committerdate` to fall through to `ask` instead of matching the allow rule. The `=`-joined splitting was already implemented for flag-only negation (`match_flag_token_with_equals`) but was missing from `FlagWithValue` matching and `optional_flags_absent` detection. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #180 +/- ##
==========================================
- Coverage 89.92% 89.90% -0.02%
==========================================
Files 38 38
Lines 7225 7273 +48
==========================================
+ Hits 6497 6539 +42
- Misses 728 734 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The `extract_placeholder_all` function (used for wrapper command placeholder extraction) had the same missing `=`-joined support as `match_tokens_core`. Wrapper patterns with `FlagWithValue` (e.g. `run -m|--mode debug <cmd>`) would fail to extract the placeholder when the flag was `=`-joined (e.g. `run -m=debug echo hello`). Co-Authored-By: Claude Opus 4.6 <[email protected]>
The `=`-joined matching behavior for `FlagWithValue` patterns was not documented. Users would not know that `--sort=-committerdate` is matched by a `[--sort *]` pattern without reading the source code. Co-Authored-By: Claude Opus 4.6 <[email protected]>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…test Per CLAUDE.md rules, tests that differ only in input should use `#[rstest]` with `#[case]` instead of separate functions. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the pattern matching system where Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where FlagWithValue patterns didn't match =-joined tokens. However, the current fix is incomplete, introducing a potential security bypass in wrapper command extraction when =-joined flag syntax is used, which violates established rules for security pattern matching and literal token parsing. Additionally, there's an opportunity to reduce code duplication in match_tokens_core to improve maintainability.
| // Case 2: `=`-joined flag and value (e.g. `--sort=value`) | ||
| if let Some(eq_pos) = cmd_tokens[i].find('=') { | ||
| let flag_part = &cmd_tokens[i][..eq_pos]; | ||
| let value_part = &cmd_tokens[i][eq_pos + 1..]; | ||
| if flag_part.starts_with('-') | ||
| && aliases.iter().any(|a| a.as_str() == flag_part) | ||
| && match_single_token(value, value_part, definitions) | ||
| { | ||
| let remaining = remove_indices(cmd_tokens, &[i]); | ||
| if let Some(ref mut caps) = captures { | ||
| let saved_len = caps.len(); | ||
| if matches!(value.as_ref(), PatternToken::Wildcard) { | ||
| caps.push(value_part); | ||
| } | ||
| if match_tokens_core(rest, &remaining, definitions, steps, Some(*caps)) | ||
| { | ||
| return true; | ||
| } | ||
| caps.truncate(saved_len); | ||
| } else if match_tokens_core(rest, &remaining, definitions, steps, None) { | ||
| return true; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The =-joined flag matching for FlagWithValue patterns is incomplete in extract_placeholder_all, leading to a security bypass. This allows wrapper patterns to be circumvented if flags are provided in =-joined form, potentially bypassing security rules. This violates the principle that pattern matching for security rules must check all occurrences of flags in the raw input, and that rules must match tokens literally, especially for flags with attached values. The same =-joined matching logic applied elsewhere should also be implemented in extract_placeholder_all for PatternToken::FlagWithValue. Additionally, consider refactoring the duplicated logic for space-separated and =-joined FlagWithValue matching in match_tokens_core to improve maintainability.
References
- For command parsing, pattern matching for security rules should be performed on the raw, tokenized input (
raw_tokens) to ensure all occurrences of flags are checked. Incomplete matching for=-joined flags can lead to security bypasses, violating this rule. - The command parser should follow a 'What You See Is How It Parses' principle. Rules must match tokens literally, especially for flags with attached values (e.g.,
-XPOSTor--flag=value), and incomplete matching for these forms violates this principle.
…into single match_engine Two nearly-identical recursive matching functions (match_tokens_core for boolean matching, extract_placeholder_all for placeholder extraction) duplicated all PatternToken variant handling. Bug fixes applied to one function were frequently missed in the other (PR #175, #179, #180). Merge both into a single `match_engine` function parameterized by an optional `extract` tuple. Each PatternToken variant's logic now exists in exactly one place. Also introduce a flag normalization layer (`split_flag_equals`, `flag_aliases_match_token`) to centralize `--flag=value` splitting, replacing the ad-hoc `=`-joined handling scattered across match_flag_token_with_equals, FlagWithValue, and optional_flags_absent. Co-Authored-By: Claude Opus 4.6 <[email protected]>
1 participant
Why
FlagWithValuepatterns (e.g.[--sort *]) did not match=-joined tokens (e.g.--sort=-committerdate)--sort value) matched, but=-joined form (--sort=value) failed because the flag was compared by exact match=-joined handling already existed for flag-only negation (match_flag_token_with_equals) but was not applied toFlagWithValueWhat
=-joined tokens into flag and value parts inFlagWithValuematching=-joined form inoptional_flags_absentto prevent false "flag absent" detection