fix: Use the correct regular expression to parse Cmnd_Alias and other aliases #68
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideUpdate alias parsing regex in scan_sudoers.py to use eBNF‐compliant NAME tokens and handle optional spacing around “=”, and add a new test suite to verify alias definitions with and without spaces. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @richm - I've reviewed your changes - here's some feedback:
- The alias regex patterns are almost identical—consider refactoring the common pattern generation into a helper function or template to avoid duplication and simplify future maintenance.
- You can improve readability and performance by converting your capturing groups into non-capturing groups (?:…) where you don’t need to retain matches, and by removing redundant quantifiers.
- It would be valuable to add a negative test case verifying that alias names starting with non-uppercase letters are rejected, to ensure the regex strictly enforces the upper-case NAME rule.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The alias regex patterns are almost identical—consider refactoring the common pattern generation into a helper function or template to avoid duplication and simplify future maintenance.
- You can improve readability and performance by converting your capturing groups into non-capturing groups (?:…) where you don’t need to retain matches, and by removing redundant quantifiers.
- It would be valuable to add a negative test case verifying that alias names starting with non-uppercase letters are rejected, to ensure the regex strictly enforces the upper-case NAME rule.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #68 +/- ##
=======================================
Coverage ? 52.31%
=======================================
Files ? 1
Lines ? 346
Branches ? 0
=======================================
Hits ? 181
Misses ? 165
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| # I'm assuming these are ASCII - so the pattern used for NAME is ([A-Z][A-Z0-9_]*) | ||
| cmnd_alias_re = re.compile( | ||
| r"(^Cmnd_Alias)+\s+(\S+)+\s*\={1}\s*((\S+,{1}\s*)+\S+|\S+)\s*(\:)*(.*)*$" | ||
| r"(^Cmnd_Alias)+\s+([A-Z][A-Z0-9_]*)\s*\=\s*((\S+,\s*)+\S+|\S+)\s*(\:)*(.*)*$" |
Check failure
Code scanning / CodeQL
Inefficient regular expression High
| # I'm assuming these are ASCII - so the pattern used for NAME is ([A-Z][A-Z0-9_]*) | ||
| cmnd_alias_re = re.compile( | ||
| r"(^Cmnd_Alias)+\s+(\S+)+\s*\={1}\s*((\S+,{1}\s*)+\S+|\S+)\s*(\:)*(.*)*$" | ||
| r"(^Cmnd_Alias)+\s+([A-Z][A-Z0-9_]*)\s*\=\s*((\S+,\s*)+\S+|\S+)\s*(\:)*(.*)*$" |
Check failure
Code scanning / CodeQL
Inefficient regular expression High
| ) | ||
| host_alias_re = re.compile( | ||
| r"(^Host_Alias)+\s+(\S+)+\s*\={1}\s*((\S+,{1}\s*)+\S+|\S+)\s*(\:)*(.*)*$" | ||
| r"(^Host_Alias)+\s+([A-Z][A-Z0-9_]*)\s*\=\s*((\S+,\s*)+\S+|\S+)\s*(\:)*(.*)*$" |
Check failure
Code scanning / CodeQL
Inefficient regular expression High
| ) | ||
| host_alias_re = re.compile( | ||
| r"(^Host_Alias)+\s+(\S+)+\s*\={1}\s*((\S+,{1}\s*)+\S+|\S+)\s*(\:)*(.*)*$" | ||
| r"(^Host_Alias)+\s+([A-Z][A-Z0-9_]*)\s*\=\s*((\S+,\s*)+\S+|\S+)\s*(\:)*(.*)*$" |
Check failure
Code scanning / CodeQL
Inefficient regular expression High
| ) | ||
| runas_alias_re = re.compile( | ||
| r"(^Runas_Alias)+\s+(\S+)+\s*\={1}\s*((\S+,{1}\s*)+\S+|\S+)\s*(\:)*(.*)*$" | ||
| r"(^Runas_Alias)+\s+([A-Z][A-Z0-9_]*)\s*\=\s*((\S+,\s*)+\S+|\S+)\s*(\:)*(.*)*$" |
Check failure
Code scanning / CodeQL
Inefficient regular expression High
| ) | ||
| runas_alias_re = re.compile( | ||
| r"(^Runas_Alias)+\s+(\S+)+\s*\={1}\s*((\S+,{1}\s*)+\S+|\S+)\s*(\:)*(.*)*$" | ||
| r"(^Runas_Alias)+\s+([A-Z][A-Z0-9_]*)\s*\=\s*((\S+,\s*)+\S+|\S+)\s*(\:)*(.*)*$" |
Check failure
Code scanning / CodeQL
Inefficient regular expression High
| ) | ||
| user_alias_re = re.compile( | ||
| r"(^User_Alias)+\s+(\S+)+\s*\={1}\s*((\S+,{1}\s*)+\S+|\S+)\s*(\:)*(.*)*$" | ||
| r"(^User_Alias)+\s+([A-Z][A-Z0-9_]*)\s*\=\s*((\S+,\s*)+\S+|\S+)\s*(\:)*(.*)*$" |
Check failure
Code scanning / CodeQL
Inefficient regular expression High
| ) | ||
| user_alias_re = re.compile( | ||
| r"(^User_Alias)+\s+(\S+)+\s*\={1}\s*((\S+,{1}\s*)+\S+|\S+)\s*(\:)*(.*)*$" | ||
| r"(^User_Alias)+\s+([A-Z][A-Z0-9_]*)\s*\=\s*((\S+,\s*)+\S+|\S+)\s*(\:)*(.*)*$" |
Check failure
Code scanning / CodeQL
Inefficient regular expression High
… aliases. Cause: The regex was not taking into consideration that the Cmnd_Alias value does not have to have spaces on either side of the `=`. The same is true for other Alias values. Consequence: The regex would never terminate, and the role would appear to hang. Fix: Ensure the regex complies with the eBNF definition of the field from the sudoers file specification. Result: The Alias values are parsed correctly. https://issues.redhat.com/browse/RHEL-106261 Signed-off-by: Rich Megginson <rmeggins@redhat.com>
richm
changed the title
Contributor
Author
|
[citest] |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Cause: The regex was not taking into consideration that the Cmnd_Alias value
does not have to have spaces on either side of the
=. The same is true forother Alias values.
Consequence: The regex would never terminate, and the role would appear to hang.
Fix: Ensure the regex complies with the eBNF definition of the field from the
sudoers file specification.
Result: The Alias values are parsed correctly.
https://issues.redhat.com/browse/RHEL-106261
Signed-off-by: Rich Megginson rmeggins@redhat.com
Summary by Sourcery
Fix alias parsing regex to align with sudoers specification and add tests to validate parsing with and without spaces around '='.
Bug Fixes:
Tests: