Closed jgrunzweig closed 3 years ago
Oh, man, my GitHub notification game is shit, yo. I didn't even see this till tonight. Thank you for reporting these issues, and for finding fixes for them all! That's baller.
I would love to merge these ricky-tick, considering I left you hanging for 12-16 days. I do have a couple requests, though:
Would you take out the version number bump? I find it's best to keep version changes separate, so other PRs/changes can be packaged in, too; or so reverts (if needed) are easier.
Would you also change those commit messages to include descriptions of what they change? It's nice to be able to read the git history without having to reference GitHub.
Perhaps something like these:
fix: properly stringify parsed port groupings (fixes #8)
fix: properly parse ungrouped port ranges (fixes #9)
fix: allow periods in option keywords (fixes #10)
I can do all those things directly on master if you'd rather not, but I'd really like to have yer face on the repo contributors ;) Looks like it's time for me to add a contributors section in the README (and jesus christ, a CHANGELOG).
Before I push any release up, I'm gonna add a few test cases to cover all these. In fact, I already wrote 'em to verify things :P
EXAMPLE_RULES_CORPUS = '''
alert ip [127.0.0.1, 127.0.0.2] any -> ![8.8.8.8/24, 1.1.1.1] any ( msg:"Test rule"; sid:12345678; rev:1; )
alert ip any 80:100 -> any any ( msg:"Fart"; )
alert ip any 80: -> any any ( msg:"Fart"; )
alert ip any :100 -> any any ( msg:"Fart"; )
alert ip any any -> any any ( msg:"Test rule"; tls.cert_subject; content:"CN=*.googleusercontent.com"; sid:12345678; rev:1; )
'''
EXAMPLE_RULES = EXAMPLE_RULES_CORPUS.strip().splitlines()
@pytest.mark.parametrize('source_rule', [
pytest.param(rule, id=rule)
for rule in EXAMPLE_RULES
])
def test_parse_to_string_to_parse(source_rule):
orig_parsed_rules = parse_rules(source_rule)
orig_parsed_rule = orig_parsed_rules[0]
stringified_rule = str(orig_parsed_rule)
twice_parsed_rules = parse_rules(stringified_rule)
twice_parsed_rule = twice_parsed_rules[0]
expected = orig_parsed_rule
actual = twice_parsed_rule
assert expected == actual
(There's gotta be better names for test_parse_to_string_to_parse
, orig_parsed_rule
, and twice_parsed_rule
, but brain no work right right now)
oh, hmm, I wonder if keywords are allowed to start with a period...
alert ip any [:100] -> any any ( msg:"end in group"; .stuff; sid: 8; )
3/6/2021 -- 00:53:11 - <Error> - [ERRCODE: SC_ERR_RULE_KEYWORD_UNKNOWN(102)] - unknown rule keyword '.stuff'.
Well, if that's the error, I guess they can technically start with a period.
I received an email today asking for support for option names with periods in them, so in the interest of expediency, I've gone and manually pushed these fixes to master. They've been published to PyPI as version 0.2.4.
Fixed for the following issues: