fix: refactor bash validation#1400
Conversation
|
Some tests are added suggested in #1398 (comment) |
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
d878372 to
57b5f18
Compare
AngeloDanducci
left a comment
There was a problem hiding this comment.
One additional comment not inlined, looking at the issue it seems we want to allow single semicolon terminators.
However this sample snippet seems to fail
pattern = ShellOperatorPattern()
is_dangerous, _ = pattern.check(["find", "logs", "-exec", "cat", "{}", ";"])
assert is_dangerous is False
is working now. Thanks! |
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
AngeloDanducci
left a comment
There was a problem hiding this comment.
A few more nits on second read through.
| return False, "" | ||
|
|
||
| # Check for dangerous flags: -f (force), -r (recursive) | ||
| dangerous_flags = {"-f", "--force", "-r", "--recursive", "--force-all"} |
There was a problem hiding this comment.
| dangerous_flags = {"-f", "--force", "-r", "--recursive", "--force-all"} | |
| dangerous_flags = {"-rf", "-f", "--force", "-r", "--recursive", "--force-all"} |
There was a problem hiding this comment.
The "-rf" and tests for it added.
| pytest.skip("MelleaTool not available") | ||
|
|
||
|
|
||
| class TestBashValidationBugFixes: |
There was a problem hiding this comment.
One coverage gap I think - there's no test that a nested-command rejection records a violation with pattern_name="NestedDangerousCommandPattern". The audit-trail tests all go through check_all_patterns, not _check_nested_dangerous_commands.
test/stdlib/tools/test_shell.py (append to TestBashValidationBugFixes)
Suggestion deletes the class line but it's just meant as a test to be added under this class.
| class TestBashValidationBugFixes: | |
| def test_nested_dangerous_command_records_violation(self) -> None: | |
| """Nested-command rejections (e.g. env sudo) record an audit violation.""" | |
| from mellea.stdlib.tools._bash_audit import get_bash_violations | |
| initial_count = len(get_bash_violations()) | |
| env = StaticBashEnvironment() | |
| result = env.execute("env sudo whoami") | |
| assert result.skipped is True | |
| assert result.success is False | |
| violations = get_bash_violations() | |
| assert len(violations) > initial_count | |
| assert violations[-1].pattern == "NestedDangerousCommandPattern" | |
| assert violations[-1].category == "nested_command" |
There was a problem hiding this comment.
This test is added.
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
AngeloDanducci
left a comment
There was a problem hiding this comment.
Everything I spotted looks good, thanks for the followups Aki.
8ab6bf1
Pull Request
Issue
Fix #1398
Description
Testing
Attribution
Adding a new component, requirement, sampling strategy, or tool?
If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.
NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.