fix(harmfulcommands): Corrected some edge cases #1128
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts the harmful command detector’s rm-pattern regex to more precisely match the rm command and expand coverage to additional dangerous path patterns (including relative paths like ./ and ..). File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughUpdated the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (2)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/tux/plugins/atl/harmfulcommands.py:25` </location>
<code_context>
r"(?:sudo\s+|doas\s+|run0\s+)?"
# rm command
- r"rm\s+"
+ r"\brm\b\s+"
# rm options
r"(?:-[frR]+|--force|--recursive|--no-preserve-root|\s+)*"
</code_context>
<issue_to_address>
**question:** Consider whether this should also catch `rm` immediately followed by flags (no space), e.g. `rm-rf /`.
The stricter `\brm\b\s+` pattern avoids substring matches but now only matches when `rm` is followed by whitespace. If you also need to catch forms like `rm-rf /` or `rm--no-preserve-root /`, you could instead allow optional whitespace or `-` after `rm`. If those forms are out of scope, the current pattern is fine.
</issue_to_address>
### Comment 2
<location> `src/tux/plugins/atl/harmfulcommands.py:29` </location>
<code_context>
r"(?:-[frR]+|--force|--recursive|--no-preserve-root|\s+)*"
# Root/home indicators
- r"(?:[/\∕~]\s*|\*|" # noqa: RUF001
+ r"(?:[/\∕~]\s*|\.(?:/|\.)\s*|\*|" # noqa: RUF001
# Critical system paths
r"/(?:bin|boot|etc|lib|proc|rooin|sys|tmp|usr|var(?:/log)?|network\.|system))"
</code_context>
<issue_to_address>
**issue (bug_risk):** Pattern for `./` and `..` misses `../` (dot-dot-slash) and may under-detect dangerous `rm -rf ../`.
`\.(?:/|\.)\s*` matches `./`, `./ `, `..`, and `.. `, but not `../` or `../ `, since the `/` after `..` doesn’t match `\s*`. As a result, `rm -rf ../` won’t be caught. Consider something like `\.(?:\.(?:/)?|/)\s*` so both `./`/`./ ` and `../`/`../ ` are treated as hazardous targets.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| r"(?:-[frR]+|--force|--recursive|--no-preserve-root|\s+)*" | ||
| # Root/home indicators | ||
| r"(?:[/\∕~]\s*|\*|" # noqa: RUF001 | ||
| r"(?:[/\∕~]\s*|\.(?:/|\.)\s*|\*|" # noqa: RUF001 |
There was a problem hiding this comment.
issue (bug_risk): Pattern for ./ and .. misses ../ (dot-dot-slash) and may under-detect dangerous rm -rf ../.
\.(?:/|\.)\s* matches ./, ./ , .., and .. , but not ../ or ../ , since the / after .. doesn’t match \s*. As a result, rm -rf ../ won’t be caught. Consider something like \.(?:\.(?:/)?|/)\s* so both .//./ and ..//../ are treated as hazardous targets.
📚 Documentation Preview
|
❌ 1 Tests Failed:
View the full list of 1 ❄️ flaky test(s)
To view more test analytics, go to the [Prevent Tests Dashboard](https://All Things Linux.sentry.io/prevent/tests/?preventPeriod=30d&integratedOrgName=allthingslinux&repository=tux&branch=fix%2Fharmfulcommands) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/tux/plugins/atl/harmfulcommands.py (1)
31-31: Pre-existing typo: "rooin" should likely be "root".The critical paths list includes "rooin", which appears to be a typo for "root" (the root user's home directory). This is pre-existing and not introduced by this PR, but worth correcting in a follow-up.
🔎 Proposed fix
- r"/(?:bin|boot|etc|lib|proc|rooin|sys|tmp|usr|var(?:/log)?|network\.|system))" + r"/(?:bin|boot|etc|lib|proc|root|sys|tmp|usr|var(?:/log)?|network\.|system))"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/tux/plugins/atl/harmfulcommands.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use strict type hints withType | Noneinstead ofOptional[Type]
Use NumPy docstrings for documenting functions and classes
Prefer absolute imports; relative imports allowed only within the same module
Organize imports in order: stdlib → third-party → local
Use 88 character line length
Use snake_case for functions and variables, PascalCase for classes, UPPER_CASE for constants
Always add imports to the top of the file unless absolutely necessary
Use async/await for I/O operations
Use custom exceptions for business logic with context logging and meaningful user messages
Use Pydantic for data validation
Keep files to a maximum of 1600 lines
Use one class or function per file when possible
Use descriptive filenames
Add appropriate logging to services and error handlers
Files:
src/tux/plugins/atl/harmfulcommands.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run All Tests (3.13.8)
- GitHub Check: Sourcery review
- GitHub Check: Seer Code Review
🔇 Additional comments (2)
src/tux/plugins/atl/harmfulcommands.py (2)
25-25: Word boundary improvement is solid.Adding
\baroundrmprevents false positives from words containing "rm" (e.g., "form", "harm", "firmware") while still correctly matching all valid rm command invocations. This improves the pattern's precision.
29-29: Relative path detection correctly addresses issue #1125.The added pattern
\.(?:/|\.)\s*successfully detects dangerous commands with relative paths like./and.., resolving the bypass where "sudo rm -rf ./*" was not being caught.Minor edge case: The pattern does not match a bare
.(single dot), sorm -rf .would not be detected. However, this is less critical since it attempts to remove the current directory itself, which typically fails, and is far less commonly used than./*or../*patterns.Given that this addresses the primary issue identified in #1125, this is acceptable.
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| r"(?:-[frR]+|--force|--recursive|--no-preserve-root|\s+)*" | ||
| # Root/home indicators | ||
| r"(?:[/\∕~]\s*|\*|" # noqa: RUF001 | ||
| r"(?:[/\∕~]\s*|\.(?:/|\.)\s*|\*|" # noqa: RUF001 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
… workflow" This reverts commit cf93d12.
Pull Request
Description
If your PR is related to an issue, please include the issue number below:
Related Issue: Closes #1125 #786
Type of Change:
Guidelines
My code follows the style guidelines of this project (formatted with Ruff)
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation if needed
My changes generate no new warnings
I have tested this change
Any dependent changes have been merged and published in downstream modules
I have added all appropriate labels to this PR
I have followed all of these guidelines.
How Has This Been Tested? (if applicable)
tested by sending various messages with the problems described in the issues (see screenshots)
Screenshots (if applicable)
Summary by Sourcery
Tighten detection of destructive rm commands in the harmfulcommands plugin to better catch edge cases and avoid false positives.
Bug Fixes: