fix(harmfulcommands): Corrected some edge cases #1127
Closed
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
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideTightens the harmful shell command detector for destructive rm invocations by making the rm token match more precise and expanding the set of root-like path prefixes that are treated as dangerous. Sequence diagram for harmful command detection during message handlingsequenceDiagram
actor User
participant ChatClient
participant Server
participant HarmfulCommandDetector
User->>ChatClient: Send message
ChatClient->>Server: Submit message content
Server->>HarmfulCommandDetector: check_message(message_text)
HarmfulCommandDetector->>HarmfulCommandDetector: apply_regex_for_rm_detection
alt matches_dangerous_rm
HarmfulCommandDetector-->>Server: result(harmful=True)
Server-->>ChatClient: block_or_warn_user
else no_dangerous_rm
HarmfulCommandDetector-->>Server: result(harmful=False)
Server-->>ChatClient: accept_and_display_message
end
Flow diagram for updated harmful rm command regex logicflowchart TD
A[Start harmful rm detection] --> B[Input shell_like text]
B --> C{Match privilege prefix?}
C -->|sudo/doas/run0 or none| D[Match rm token with word boundaries: \brm\b]
D --> E{Match rm options?}
E -->|yes or none| F{Match dangerous path prefix?}
F -->|/ or ∕ or ~ or /. or *| G[Dangerous path detected]
F -->|/bin,/boot,/etc,/lib,/proc,/rooin,/sys,/tmp,/usr,/var,/var/log,/network.,/system| G
G --> H[Flag message as harmful]
F -->|no match| I[No dangerous rm detected]
H --> J[End]
I --> J
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. 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. WalkthroughRequire whole-word matching for "rm" and extend the dangerous-rm pattern to recognize dot-prefixed relative paths (e.g., Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Including
.in the[/\∕~/.]character class makes anyrmon a dot-prefixed path (e.g.,rm .bashrc) match as “harmful”; if the intent is to catchrm -rf ././specifically, consider a more targeted pattern like(?:\./|\s+\.)instead of a bare.in the character class to avoid false positives. - Since the behavior of
rm -rf .is nuanced across systems and shells, it would be helpful to encode the assumption about treating.as dangerous directly in a short code comment near the regex, so future changes don’t accidentally widen or narrow that behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Including `.` in the `[/\∕~/.]` character class makes any `rm` on a dot-prefixed path (e.g., `rm .bashrc`) match as “harmful”; if the intent is to catch `rm -rf .` / `./` specifically, consider a more targeted pattern like `(?:\./|\s+\.)` instead of a bare `.` in the character class to avoid false positives.
- Since the behavior of `rm -rf .` is nuanced across systems and shells, it would be helpful to encode the assumption about treating `.` as dangerous directly in a short code comment near the regex, so future changes don’t accidentally widen or narrow that behavior.
## Individual Comments
### Comment 1
<location> `src/tux/plugins/atl/harmfulcommands.py:28-29` </location>
<code_context>
r"(?:-[frR]+|--force|--recursive|--no-preserve-root|\s+)*"
# Root/home indicators
- r"(?:[/\∕~]\s*|\*|" # noqa: RUF001
+ r"(?:[/\∕~/.]\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>
**suggestion (bug_risk):** The added `.` in the character class makes the rule very broad and will flag benign commands like `rm .foo` or `rm .`.
Including `.` in this class makes any `rm` of hidden files or `.` itself count as harmful, which is much broader than the prior `/ ∕ ~` behavior. If you mainly want to catch `./` and `../`, consider matching those explicitly (e.g., `r"(?:[/\∕~]|\.(?:/|\.))."`) instead of treating any leading `.` as a root indicator, to avoid false positives like `rm .gitignore` or `rm ./*.log`.
```suggestion
# Root/home indicators
r"(?:[/\∕~]\s*|\.(?:/|\.)\s*|\*|" # noqa: RUF001
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
❌ 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=meatsnails%2Ftux%3Amain) |
![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)
29-29: Fix successfully addresses issue #1125, but consider removing duplicate character.The addition of
.to the character class correctly fixes the bypass issue by now matching patterns like./,./*, etc. However, the character class[/\∕~/.]contains the forward slash/twice (positions 1 and 4), which is redundant.🔎 Proposed cleanup to remove duplicate
- r"(?:[/\∕~/.]\s*|\*|" # noqa: RUF001 + r"(?:[/\∕~.]\s*|\*|" # noqa: RUF001
📜 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 (1)
src/tux/plugins/atl/harmfulcommands.py (1)
25-25: Excellent improvement to reduce false positives.Adding word boundaries
\baround "rm" prevents the regex from matching substrings within unrelated words (e.g., "confirm", "alarm"), significantly reducing false positives while maintaining correct detection of the actualrmcommand.
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
Comment on lines
29
to
31
| r"(?:[/\∕~/.]\s*|\*|" # noqa: RUF001 | ||
| # Critical system paths | ||
| r"/(?:bin|boot|etc|lib|proc|rooin|sys|tmp|usr|var(?:/log)?|network\.|system))" |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| 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.
Bug: The regex \.(?:/|\.)\s* fails to detect the harmful command rm -rf . because it doesn't account for a single dot followed by a space, leaving a gap in the safety check.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The updated regex pattern \.(?:/|\.)\s* in the is_harmful() function is designed to detect harmful commands targeting dot-prefixed paths. However, this pattern requires the dot to be followed by either a forward slash (/) or another dot (.). As a result, it fails to match a command like rm -rf ., where the dot is followed by a space. The underlying assumption that rm -rf . is not a dangerous command is incorrect; it recursively deletes all contents of the current directory. This gap in detection means the bot will not warn users about this destructive command, defeating a core safety feature.
💡 Suggested Fix
Modify the regex pattern to correctly identify a single dot (.) as a target for commands like rm. The pattern should be updated to match a bare dot followed by whitespace, in addition to the existing ./ and .. cases. This will ensure commands like rm -rf . are properly flagged.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/tux/plugins/atl/harmfulcommands.py#L29
Potential issue: The updated regex pattern `\.(?:/|\.)\s*` in the `is_harmful()`
function is designed to detect harmful commands targeting dot-prefixed paths. However,
this pattern requires the dot to be followed by either a forward slash (`/`) or another
dot (`.`). As a result, it fails to match a command like `rm -rf .`, where the dot is
followed by a space. The underlying assumption that `rm -rf .` is not a dangerous
command is incorrect; it recursively deletes all contents of the current directory. This
gap in detection means the bot will not warn users about this destructive command,
defeating a core safety feature.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8012009
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
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)
Additional Information
"rm -rf ." wouldn't work on pretty much any linux and or unix-like system but i left it in just incase (and it keeps the code cleaner)
Summary by Sourcery
Tighten harmful command detection for dangerous rm usages to cover additional edge cases.
Bug Fixes: