feat: make the pytest testing framework functional, migrate all v2 tests#3594
Open
fzipi wants to merge 11 commits into
Open
feat: make the pytest testing framework functional, migrate all v2 tests#3594fzipi wants to merge 11 commits into
fzipi wants to merge 11 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Felipe Zipitria <felipe.zipitria@owasp.org>
The pytest framework in tests/ was scaffolded but never actually run end-to-end. Fixes applied: - conftest.py: split the Apache-backed `modsec_test` fixture from a new lightweight `unit_test` fixture, so op/tfn unit tests no longer start a full httpd per test. - modsec_test.py: resolve msc_test's path and cwd reliably, and escape argv/stdin using msc_test.c's own \xHH convention so control/NUL bytes and literal backslashes survive the round trip (previously a NUL byte would crash subprocess.Popen, and regex params with backslashes were silently corrupted). - convert_perl_tests.py: replace the regex-based Perl parser (which double-escaped data and produced 22 regression-test files that don't even parse) with a Perl-assisted path for op/*.t and tfn/*.t - a small helper script lets Perl itself evaluate the test data and hand back JSON, avoiding a hand-rolled Perl-string-escape parser in Python. - Moved 25 operator-derived tests that were misfiled under test_transformations/ into test_operators/, and regenerated all op/tfn tests for real; all 4228 now pass against a built msc_test. - Added a CI job that builds msc_test and runs this suite. Regression-test migration (the Apache-backed tests/regression/*.t suite) is a separate follow-up - the existing converter's output for those is excluded from collection (pytest.ini) since it drops all assertions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Phase 2 of the pytest migration: the Apache-backed tests/regression/*.t
suite. dump_regression_fixtures.pl lets Perl itself evaluate each .t
file (same %ENV setup and "@C = (...)" eval trick as
run-regression-tests.pl) and serializes the result to
tests/regression/fixtures/*.json - qr// -> {pattern,flags}, HTTP::Request
-> {method,uri,headers,content}, conf=>sub{} coderefs executed and
captured, unsupported request/test/prerun coderefs flagged loudly
rather than silently dropped (found one real one: a file-upload check
in config/10-misc-directives.t, now a marked pytest.skip instead of
disappearing).
apache_server.py and modsec_test.py were rewritten to match the real
Perl semantics rather than a from-scratch reimplementation:
- Reuse the autoconf-generated tests/regression/server_root/conf/httpd.conf
+ per-test `-c "Include <conf>"`, mirroring httpd_start/httpd_stop
(the previous hand-rolled base config used LogFormat/CustomLog
without loading mod_log_config, so it could never actually start).
- LogMatcher now mirrors match_log()'s real semantics: a persistent
accumulating buffer per log (so multiline/dotall patterns spanning
multiple reads still match) and a *timeout in seconds* to wait for a
pattern, not an occurrence count. Both match_log and match_response
support "-key" negation.
- ResponseMatcher's "raw" case matches HTTP::Response->as_string
(LF-only line endings, not CRLF).
- The `drop` action's abrupt connection close now yields a synthetic
5xx response instead of losing the request entirely, matching
LWP::UserAgent's own behavior that some .t files rely on.
test_regression/test_fixtures.py replaces per-file hand-maintained
Python tests with one generic loader parametrized over every fixture,
so .t edits just need tests/regenerate_regression_fixtures.sh re-run
(CI checks the fixtures aren't stale). 241/246 cases pass against a
real Apache + mod_security2 build; 5 are skipped with a stated reason
(1 unsupported coderef noted above; 4 check byte-exact request
reconstruction that depends on LWP's specific header fingerprint,
which `requests`/urllib3 doesn't reproduce - a client-library
difference, not a ModSecurity behavior gap).
Also retires test_regression/test_disruptive_actions.py, now fully
subsumed by the fixture for regression/action/00-disruptive-actions.t.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
gen_rx-pm.pl.in/csv_rx-pm.pl.in (an @rx/@pm performance-comparison generator and its CSV post-processor) were never referenced by any Makefile.am target or CI step - only auto-generated by configure and never invoked. Moved to tools/legacy/ and dropped their AC_CONFIG_FILES entries rather than continuing to regenerate tooling nobody runs. test-linux and test-regression-linux now run the pytest suites directly (op/tfn unit tests and the regression fixtures, respectively) instead of make test/make test-regression, which drove the old Perl runners. This replaces the standalone test-python-unit/ test-python-regression jobs added earlier - same coverage, no duplicate CI runs of the same tests under two job names. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
README described a structure that never matched reality (a test_actions/ dir that doesn't exist, dict-based test_config examples that don't match either test harness) and never mentioned the actual architecture built across the last few commits - the Perl-assisted dump_unit_fixtures.pl/dump_regression_fixtures.pl pipeline, the generated vs. hand-written test files, or how to regenerate either after editing a .t file. Rewritten to match what's actually here. converted_tests/ (22 syntactically-broken files plus 2 that parse but drop every real assertion - the original regex-based converter's output) is now fully superseded by test_regression/test_fixtures.py and safe to delete outright; drops the pytest.ini --ignore that was working around it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
tests/op/*.t and tests/tfn/*.t had exactly three consumers: run-unit-tests.pl, dump_unit_fixtures.pl, and convert_perl_tests.py. CI no longer invokes make test/make check for unit tests (it runs pytest directly), so run-unit-tests.pl was already unreferenced there; and unlike the regression .t data (which embeds real Perl semantics - qr// flags, HTTP::Request, coderefs - worth keeping Perl around to parse), op/tfn data is flat literal strings with no ongoing need for a Perl round-trip once converted. Removes tests/op/*.t, tests/tfn/*.t, tests/run-unit-tests.pl.in (+ its AC_CONFIG_FILES entry and tests/Makefile.am's check_SCRIPTS/TESTS wiring), dump_unit_fixtures.pl, and convert_perl_tests.py (now fully dead - its only live path needed the just-removed .t files, and the rest was already dead code from the original abandoned converter). tests/op/pmFromFile-01.dat is kept - it's a runtime data file test_operators/test_pmFromFile.py reads via the @pmFromFile operator, not test source. msc_test stays: it's still the executor the Python side calls directly, and remains buildable via `make -C tests msc_test` (it's still a check_PROGRAMS entry, just with nothing left in TESTS). Verified autogen.sh/configure regenerate cleanly without run-unit-tests.pl, `make check` in tests/ is now a no-op there (builds msc_test, runs no tests), and all 4228 unit tests still pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
fzipi
force-pushed
the
feat/new-testing-framework
branch
from
07aa6a1 to
21be6f7
Compare
Member
|
I like the idea of extending/evolving the testing framework, and besides my preference for Python over Perl 😃, now I think that instead of replacing the entire existing Perl-based regression testing framework, I would add this new one beside the existing one; which could run in parallel. The reason is very simple: there are a few Sonar issues, probably we will fix them over time, and there is one regression test which can't be completed. I've restarted it, but the situation is the same. In short: I would give time to improve the new framework. What do you think? |
Collaborator
Author
|
Of course, this is my first PR 😉 The idea is to evolve it a bit and replace. It doesn't make sense to have them both in parallel. Will do more work. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a pytest-based testing framework under tests/, migrating the existing v2 operator/transformation unit tests and adding a pytest runner for the existing Perl regression-test corpus via generated JSON fixtures, while updating CI to run these new pytest suites instead of the Perl runners.
Changes:
- Adds a Python test harness (
ModSecurityTestCase, Apache lifecycle, fixture loader) and pytest suites for operators/transformations. - Adds regression-fixture generation (
dump_regression_fixtures.pl+regenerate_regression_fixtures.sh) and a pytest runner that executes those fixtures against Apache +mod_security2. - Removes the Perl unit test runner and the now-obsolete
.tunit-test data files; updates Autotools/CI wiring accordingly.
Reviewed changes
Copilot reviewed 154 out of 160 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/legacy/gen_rx-pm.pl.in | Added legacy Perl perf-test generator script under tools/legacy |
| tools/legacy/csv_rx-pm.pl.in | Added legacy Perl CSV helper script under tools/legacy |
| tests/tfn/urlEncode.t | Removed Perl transformation test data (migrated to pytest) |
| tests/tfn/urlDecode.t | Removed Perl transformation test data (migrated to pytest) |
| tests/tfn/trimRight.t | Removed Perl transformation test data (migrated to pytest) |
| tests/tfn/trimLeft.t | Removed Perl transformation test data (migrated to pytest) |
| tests/tfn/trim.t | Removed Perl transformation test data (migrated to pytest) |
| tests/tfn/sha1.t | Removed Perl transformation test data (migrated to pytest) |
| tests/tfn/replaceNulls.t | Removed Perl transformation test data (migrated to pytest) |
| tests/tfn/replaceComments.t | Removed Perl transformation test data (migrated to pytest) |
| tests/tfn/removeWhitespace.t | Removed Perl transformation test data (migrated to pytest) |
| tests/tfn/removeNulls.t | Removed Perl transformation test data (migrated to pytest) |
| tests/tfn/parityZero7bit.t | Removed Perl transformation test data (migrated to pytest) |
| tests/tfn/parityOdd7bit.t | Removed Perl transformation test data (migrated to pytest) |
| tests/tfn/parityEven7bit.t | Removed Perl transformation test data (migrated to pytest) |
| tests/tfn/normalisePathWin.t | Removed Perl transformation test data (migrated to pytest) |
| tests/tfn/normalisePath.t | Removed Perl transformation test data (migrated to pytest) |
| tests/tfn/md5.t | Removed Perl transformation test data (migrated to pytest) |
| tests/tfn/lowercase.t | Removed Perl transformation test data (migrated to pytest) |
| tests/tfn/length.t | Removed Perl transformation test data (migrated to pytest) |
| tests/tfn/jsDecode.t | Removed Perl transformation test data (migrated to pytest) |
| tests/tfn/htmlEntityDecode.t | Removed Perl transformation test data (migrated to pytest) |
| tests/tfn/hexEncode.t | Removed Perl transformation test data (migrated to pytest) |
| tests/tfn/hexDecode.t | Removed Perl transformation test data (migrated to pytest) |
| tests/tfn/escapeSeqDecode.t | Removed Perl transformation test data (migrated to pytest) |
| tests/tfn/cssDecode.t | Removed Perl transformation test data (migrated to pytest) |
| tests/tfn/compressWhitespace.t | Removed Perl transformation test data (migrated to pytest) |
| tests/tfn/base64Encode.t | Removed Perl transformation test data (migrated to pytest) |
| tests/tfn/base64Decode.t | Removed Perl transformation test data (migrated to pytest) |
| tests/test_transformations/init.py | New pytest package for transformation tests |
| tests/test_transformations/test_urlEncode.py | New pytest transformation test (urlEncode) |
| tests/test_transformations/test_urlDecode.py | New pytest transformation test (urlDecode) |
| tests/test_transformations/test_trimRight.py | New pytest transformation test (trimRight) |
| tests/test_transformations/test_trimLeft.py | New pytest transformation test (trimLeft) |
| tests/test_transformations/test_trim.py | New pytest transformation test (trim) |
| tests/test_transformations/test_sha1.py | New pytest transformation test (sha1) |
| tests/test_transformations/test_replaceNulls.py | New pytest transformation test (replaceNulls) |
| tests/test_transformations/test_replaceComments.py | New pytest transformation test (replaceComments) |
| tests/test_transformations/test_removeWhitespace.py | New pytest transformation test (removeWhitespace) |
| tests/test_transformations/test_removeNulls.py | New pytest transformation test (removeNulls) |
| tests/test_transformations/test_parityZero7bit.py | New pytest transformation test (parityZero7bit) |
| tests/test_transformations/test_parityOdd7bit.py | New pytest transformation test (parityOdd7bit) |
| tests/test_transformations/test_parityEven7bit.py | New pytest transformation test (parityEven7bit) |
| tests/test_transformations/test_normalisePathWin.py | New pytest transformation test (normalisePathWin) |
| tests/test_transformations/test_normalisePath.py | New pytest transformation test (normalisePath) |
| tests/test_transformations/test_md5.py | New pytest transformation test (md5) |
| tests/test_transformations/test_lowercase.py | New pytest transformation test (lowercase) |
| tests/test_transformations/test_length.py | New pytest transformation test (length) |
| tests/test_transformations/test_jsDecode.py | New pytest transformation test (jsDecode) |
| tests/test_transformations/test_htmlEntityDecode.py | New pytest transformation test (htmlEntityDecode) |
| tests/test_transformations/test_hexEncode.py | New pytest transformation test (hexEncode) |
| tests/test_transformations/test_hexDecode.py | New pytest transformation test (hexDecode) |
| tests/test_transformations/test_escapeSeqDecode.py | New pytest transformation test (escapeSeqDecode) |
| tests/test_transformations/test_cssDecode.py | New pytest transformation test (cssDecode) |
| tests/test_transformations/test_compressWhitespace.py | New pytest transformation test (compressWhitespace) |
| tests/test_transformations/test_base64Encode.py | New pytest transformation test (base64Encode) |
| tests/test_transformations/test_base64decode.py | New pytest transformation test (base64Decode) |
| tests/op/within.t | Removed Perl operator test data (migrated to pytest) |
| tests/op/validateUtf8Encoding.t | Removed Perl operator test data (migrated to pytest) |
| tests/op/validateUrlEncoding.t | Removed Perl operator test data (migrated to pytest) |
| tests/op/validateSchema.t | Removed obsolete/empty Perl operator test file |
| tests/op/validateDTD.t | Removed obsolete/empty Perl operator test file |
| tests/op/validateByteRange.t | Removed Perl operator test data (migrated to pytest) |
| tests/op/unconditionalMatch.t | Removed Perl operator test data (migrated to pytest) |
| tests/op/strmatch.t | Removed Perl operator test data (migrated to pytest) |
| tests/op/streq.t | Removed Perl operator test data (migrated to pytest) |
| tests/op/rx.t | Removed Perl operator test data (migrated to pytest) |
| tests/op/rbl.t | Removed obsolete/empty Perl operator test file |
| tests/op/pmFromFile.t | Removed Perl operator test data (migrated to pytest) |
| tests/op/noMatch.t | Removed Perl operator test data (migrated to pytest) |
| tests/op/lt.t | Removed Perl operator test data (migrated to pytest) |
| tests/op/le.t | Removed Perl operator test data (migrated to pytest) |
| tests/op/inspectFile.t | Removed obsolete/empty Perl operator test file |
| tests/op/gt.t | Removed Perl operator test data (migrated to pytest) |
| tests/op/geoLookup.t | Removed Perl operator test data (migrated to pytest) |
| tests/op/ge.t | Removed Perl operator test data (migrated to pytest) |
| tests/op/eq.t | Removed Perl operator test data (migrated to pytest) |
| tests/op/endsWith.t | Removed Perl operator test data (migrated to pytest) |
| tests/op/detectXSS.t | Removed Perl operator test data (migrated to pytest) |
| tests/op/detectSQLi.t | Removed Perl operator test data (migrated to pytest) |
| tests/op/containsWord.t | Removed Perl operator test data (migrated to pytest) |
| tests/op/contains.t | Removed Perl operator test data (migrated to pytest) |
| tests/op/beginsWith.t | Removed Perl operator test data (migrated to pytest) |
| tests/test_operators/init.py | New pytest package for operator tests |
| tests/test_operators/test_within.py | New pytest operator test (within) |
| tests/test_operators/test_validateUrlEncoding.py | New pytest operator test (validateUrlEncoding) |
| tests/test_operators/test_validateByteRange.py | New pytest operator test (validateByteRange) |
| tests/test_operators/test_unconditionalMatch.py | New pytest operator test (unconditionalMatch) |
| tests/test_operators/test_strmatch.py | New pytest operator test (strmatch) |
| tests/test_operators/test_streq.py | New pytest operator test (streq) |
| tests/test_operators/test_rx.py | New pytest operator test (rx) |
| tests/test_operators/test_pmFromFile.py | New pytest operator test (pmFromFile) |
| tests/test_operators/test_noMatch.py | New pytest operator test (noMatch) |
| tests/test_operators/test_lt.py | New pytest operator test (lt) |
| tests/test_operators/test_le.py | New pytest operator test (le) |
| tests/test_operators/test_gt.py | New pytest operator test (gt) |
| tests/test_operators/test_geoLookup.py | New pytest operator test (geoLookup) |
| tests/test_operators/test_ge.py | New pytest operator test (ge) |
| tests/test_operators/test_eq.py | New pytest operator test (eq) |
| tests/test_operators/test_endsWith.py | New pytest operator test (endsWith) |
| tests/test_operators/test_detectXSS.py | New pytest operator test (detectXSS) |
| tests/test_operators/test_detectSQLi.py | New pytest operator test (detectSQLi) |
| tests/test_operators/test_containsWord.py | New pytest operator test (containsWord) |
| tests/test_operators/test_contains.py | New pytest operator test (contains) |
| tests/test_operators/test_beginswith.py | New pytest operator test (beginsWith) |
| tests/run-unit-tests.pl.in | Removed Perl unit-test runner |
| tests/requirements.txt | Added Python test dependencies for pytest framework |
| tests/pytest.ini | Added pytest configuration |
| tests/regression_fixtures.py | Added JSON fixture discovery/decoding for regression tests |
| tests/regenerate_regression_fixtures.sh | Added script to regenerate regression JSON fixtures |
| tests/dump_regression_fixtures.pl | Added Perl tool to serialize .t regression entries to JSON |
| tests/test_regression/init.py | New pytest package for regression tests |
| tests/test_regression/test_fixtures.py | Added pytest runner that executes serialized regression fixtures |
| tests/conftest.py | Added pytest fixtures wiring unit vs regression environments |
| tests/apache_server.py | Added Apache lifecycle management for regression tests |
| tests/modsec_test.py | Added core Python harness for unit + regression execution |
| tests/init.py | Added tests package metadata/docs |
| tests/Makefile.am | Removed Perl unit-test wiring from make check in tests/ |
| tests/regression/fixtures/rule/20-exceptions.json | Added serialized regression fixtures (rule/20-exceptions) |
| tests/regression/fixtures/rule/00-script.json | Added serialized regression fixtures (rule/00-script) |
| tests/regression/fixtures/rule/00-inheritance.json | Added serialized regression fixtures (rule/00-inheritance) |
| tests/regression/fixtures/rule/00-basics.json | Added serialized regression fixtures (rule/00-basics) |
| tests/regression/fixtures/misc/25-libinjection.json | Added serialized regression fixtures (misc/25-libinjection) |
| tests/regression/fixtures/misc/00-phases.json | Added serialized regression fixtures (misc/00-phases) |
| tests/regression/fixtures/config/20-chroot.json | Added serialized regression fixtures (config/20-chroot) |
| tests/regression/fixtures/config/10-response-directives.json | Added serialized regression fixtures (config/10-response-directives) |
| tests/regression/fixtures/config/10-misc-directives.json | Added serialized regression fixtures (config/10-misc-directives) |
| tests/regression/fixtures/config/10-debug-directives.json | Added serialized regression fixtures (config/10-debug-directives) |
| tests/regression/fixtures/config/10-audit-directives.json | Added serialized regression fixtures (config/10-audit-directives) |
| tests/regression/fixtures/config/00-load-modsec.json | Added serialized regression fixtures (config/00-load-modsec) |
| tests/regression/fixtures/action/10-ctl.json | Added serialized regression fixtures (action/10-ctl) |
| tests/regression/fixtures/action/10-append-prepend.json | Added serialized regression fixtures (action/10-append-prepend) |
| tests/regression/fixtures/action/00-transformations.json | Added serialized regression fixtures (action/00-transformations) |
| tests/regression/fixtures/action/00-misc.json | Added serialized regression fixtures (action/00-misc) |
| tests/regression/fixtures/action/00-meta.json | Added serialized regression fixtures (action/00-meta) |
| configure.ac | Removed autoconf generation for removed/moved Perl test scripts |
| .gitignore | Updated ignore rules for new/kept test tooling and binaries |
| .github/workflows/ci.yml | Updated CI to build msc_test, install Python deps, run pytest unit + regression suites |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,21 @@ | |||
| [tool:pytest] | |||
| @@ -0,0 +1 @@ | |||
| [{"comment":"SecRuleRemoveById (single)","conf":"\n\t\tSecRuleEngine On\n\t\tSecDebugLog /Users/fzipitria/Workspace/OWASP/ModSecurity/tests/regression/server_root/logs/modsec_debug.log\n\t\tSecDebugLogLevel 9\n\t\tSecRule REQUEST_URI \"test\" \"phase:1,deny,status:500,id:101010\"\n\t\tSecRuleRemoveById 101010\n\t","match_log":{"-audit":[{"__regex__":1,"flags":"","pattern":"."},1],"-debug":[{"__regex__":1,"flags":"","pattern":"Access denied"},1],"-error":[{"__regex__":1,"flags":"","pattern":"101010"},1],"debug":[{"__regex__":1,"flags":"s","pattern":"Starting phase REQUEST_HEADERS\\..*This phase consists of 0 rule.*Starting phase RESPONSE_HEADERS\\."},1]},"match_response":{"status":{"__regex__":1,"flags":"","pattern":"^200$"}},"request":{"__type__":"http_request","content":"","headers":[],"method":"GET","uri":"http://localhost:8088/test.txt"},"type":"rule"},{"comment":"SecRuleRemoveById (multiple)","conf":"\n\t\tSecRuleEngine On\n\t\tSecDebugLog /Users/fzipitria/Workspace/OWASP/ModSecurity/tests/regression/server_root/logs/modsec_debug.log\n\t\tSecDebugLogLevel 9\n\t\tSecRule REQUEST_URI \"test\" \"phase:1,deny,status:500,id:101010\"\n\t\tSecRule REQUEST_URI \"test\" \"phase:1,deny,status:500,id:202020\"\n\t\tSecRule REQUEST_URI \"test\" \"phase:1,deny,status:500,id:303030\"\n\t\tSecRuleRemoveById 101010 202020 303030\n\t","match_log":{"-audit":[{"__regex__":1,"flags":"","pattern":"."},1],"-debug":[{"__regex__":1,"flags":"","pattern":"Access denied"},1],"-error":[{"__regex__":1,"flags":"","pattern":"101010|202020|303030"},1],"debug":[{"__regex__":1,"flags":"s","pattern":"Starting phase REQUEST_HEADERS\\..*This phase consists of 0 rule.*Starting phase RESPONSE_HEADERS\\."},1]},"match_response":{"status":{"__regex__":1,"flags":"","pattern":"^200$"}},"request":{"__type__":"http_request","content":"","headers":[],"method":"GET","uri":"http://localhost:8088/test.txt"},"type":"rule"},{"comment":"SecRuleRemoveById (range)","conf":"\n\t\tSecRuleEngine On\n\t\tSecDebugLog /Users/fzipitria/Workspace/OWASP/ModSecurity/tests/regression/server_root/logs/modsec_debug.log\n\t\tSecDebugLogLevel 9\n\t\tSecRule REQUEST_URI \"test\" \"phase:1,deny,status:500,id:101010\"\n\t\tSecRule REQUEST_URI \"test\" \"phase:1,deny,status:500,id:202020\"\n\t\tSecRule REQUEST_URI \"test\" \"phase:1,deny,status:500,id:303030\"\n\t\tSecRuleRemoveById 101010-303030\n\t","match_log":{"-audit":[{"__regex__":1,"flags":"","pattern":"."},1],"-debug":[{"__regex__":1,"flags":"","pattern":"Access denied"},1],"-error":[{"__regex__":1,"flags":"","pattern":"101010|202020|303030"},1],"debug":[{"__regex__":1,"flags":"s","pattern":"Starting phase REQUEST_HEADERS\\..*This phase consists of 0 rule.*Starting phase RESPONSE_HEADERS\\."},1]},"match_response":{"status":{"__regex__":1,"flags":"","pattern":"^200$"}},"request":{"__type__":"http_request","content":"","headers":[],"method":"GET","uri":"http://localhost:8088/test.txt"},"type":"rule"},{"comment":"SecRuleRemoveById (multiple + range)","conf":"\n\t\tSecRuleEngine On\n\t\tSecDebugLog /Users/fzipitria/Workspace/OWASP/ModSecurity/tests/regression/server_root/logs/modsec_debug.log\n\t\tSecDebugLogLevel 9\n\t\tSecRule REQUEST_URI \"test\" \"phase:1,deny,status:500,id:101010\"\n\t\tSecRule REQUEST_URI \"test\" \"phase:1,deny,status:500,id:202020\"\n\t\tSecRule REQUEST_URI \"test\" \"phase:1,deny,status:500,id:303030\"\n\t\tSecRule REQUEST_URI \"test\" \"phase:1,deny,status:500,id:404040\"\n\t\tSecRuleRemoveById 101010 202020-404040\n\t","match_log":{"-audit":[{"__regex__":1,"flags":"","pattern":"."},1],"-debug":[{"__regex__":1,"flags":"","pattern":"Access denied"},1],"-error":[{"__regex__":1,"flags":"","pattern":"101010|202020|303030|404040"},1],"debug":[{"__regex__":1,"flags":"s","pattern":"Starting phase REQUEST_HEADERS\\..*This phase consists of 0 rule.*Starting phase RESPONSE_HEADERS\\."},1]},"match_response":{"status":{"__regex__":1,"flags":"","pattern":"^200$"}},"request":{"__type__":"http_request","content":"","headers":[],"method":"GET","uri":"http://localhost:8088/test.txt"},"type":"rule"},{"comment":"SecRuleRemoveByMsg","conf":"\n\t\tSecRuleEngine On\n\t\tSecDebugLog /Users/fzipitria/Workspace/OWASP/ModSecurity/tests/regression/server_root/logs/modsec_debug.log\n\t\tSecDebugLogLevel 9\n\t\tSecRule REQUEST_URI \"test\" \"phase:1,deny,status:500,msg:'testing rule',id:500001\"\n\t\tSecRuleRemoveByMsg \"testing rule\"\n\t","match_log":{"-audit":[{"__regex__":1,"flags":"","pattern":"."},1],"-debug":[{"__regex__":1,"flags":"","pattern":"Access denied"},1],"-error":[{"__regex__":1,"flags":"","pattern":"500001"},1],"debug":[{"__regex__":1,"flags":"s","pattern":"Starting phase REQUEST_HEADERS\\..*This phase consists of 0 rule.*Starting phase RESPONSE_HEADERS\\."},1]},"match_response":{"status":{"__regex__":1,"flags":"","pattern":"^200$"}},"request":{"__type__":"http_request","content":"","headers":[],"method":"GET","uri":"http://localhost:8088/test.txt"},"type":"rule"},{"comment":"SecRuleUpdateActionById","conf":"\n\t\tSecRuleEngine On\n\t\tSecDebugLog /Users/fzipitria/Workspace/OWASP/ModSecurity/tests/regression/server_root/logs/modsec_debug.log\n\t\tSecDebugLogLevel 9\n\t\tSecRule REQUEST_URI \"test\" \"phase:1,deny,status:500,msg:'testing rule',id:500002\"\n\t\tSecRuleUpdateActionById 500002 \"pass,nolog\"\n\t","match_log":{"-audit":[{"__regex__":1,"flags":"","pattern":"."},1],"-debug":[{"__regex__":1,"flags":"","pattern":"Access denied"},1],"-error":[{"__regex__":1,"flags":"","pattern":"500002"},1],"debug":[{"__regex__":1,"flags":"","pattern":"id:500002,pass,nolog"},1]},"match_response":{"status":{"__regex__":1,"flags":"","pattern":"^200$"}},"request":{"__type__":"http_request","content":"","headers":[],"method":"GET","uri":"http://localhost:8088/test.txt"},"type":"rule"},{"comment":"SecRuleUpdateActionById (chain)","conf":"\n\t\tSecRuleEngine On\n\t\tSecDebugLog /Users/fzipitria/Workspace/OWASP/ModSecurity/tests/regression/server_root/logs/modsec_debug.log\n\t\tSecDebugLogLevel 9\n\t\tSecRule REQUEST_URI \"test\" \"phase:1,deny,status:500,msg:'testing rule',chain,id:500003\"\n SecRule ARGS \"bar\"\n\t\tSecRuleUpdateActionById 500003 \"pass,nolog\"\n\t","match_log":{"-audit":[{"__regex__":1,"flags":"","pattern":"."},1],"-debug":[{"__regex__":1,"flags":"","pattern":"Access denied"},1],"-error":[{"__regex__":1,"flags":"","pattern":"500003"},1],"debug":[{"__regex__":1,"flags":"","pattern":"id:500003,pass,nolog"},1]},"match_response":{"status":{"__regex__":1,"flags":"","pattern":"^200$"}},"request":{"__type__":"http_request","content":"","headers":[],"method":"GET","uri":"http://localhost:8088/test.txt?foo=bar"},"type":"rule"}] No newline at end of file | |||
| @@ -0,0 +1 @@ | |||
| [{"comment":"SecDefaultAction","conf":"\n\t\tSecRuleEngine on\n\t\tSecDefaultAction \"phase:1,deny,status:500\"\n\t\tSecRule REQUEST_URI \"test.txt\" \"id:500241\"\n\t","match_log":{"error":[{"__regex__":1,"flags":"","pattern":"ModSecurity: Access denied with code 500 \\(phase 1\\)"},1]},"match_response":{"status":{"__regex__":1,"flags":"","pattern":"^500$"}},"request":{"__type__":"http_request","content":"","headers":[],"method":"GET","uri":"http://localhost:8088/test.txt"},"type":"config"},{"comment":"SecServerSignature On","conf":"\n\t\tSecServerSignature \"NewServerSignature\"\n\t","match_response":{"raw":{"__regex__":1,"flags":"m","pattern":"^Server: +NewServerSignature$"},"status":{"__regex__":1,"flags":"","pattern":"^200$"}},"request":{"__type__":"http_request","content":"","headers":[],"method":"GET","uri":"http://localhost:8088/test.txt"},"type":"config"},{"comment":"SecDataDir","conf":"\n\t\tSecRuleEngine On\n\t\tSecDataDir \"/Users/fzipitria/Workspace/OWASP/ModSecurity/tests/regression/server_root/data\"\n\t\tSecAction initcol:ip=%{REMOTE_ADDR},setvar:ip.dummy=1,pass,id:500085\n\t","match_file":{"/Users/fzipitria/Workspace/OWASP/ModSecurity/tests/regression/server_root/data/fzipitria-ip.pag":{"__regex__":1,"flags":"","pattern":"\\x00\\x06dummy\\x00\\x00\\x021\\x00"}},"match_log":{"error":[{"__regex__":1,"flags":"","pattern":"ModSecurity: Warning. Unconditional match in SecAction\\."},1]},"match_response":{"status":{"__regex__":1,"flags":"","pattern":"^200$"}},"request":{"__type__":"http_request","content":"","headers":[],"method":"GET","uri":"http://localhost:8088/test.txt"},"type":"config"},{"comment":"SecTmpDir/SecUploadDir/SecUploadKeepFiles","conf":"\n\t\tSecRuleEngine On\n\t\tSecRequestBodyAccess On\n\t\tSecDebugLog /Users/fzipitria/Workspace/OWASP/ModSecurity/tests/regression/server_root/logs/modsec_debug.log\n\t\tSecDebugLogLevel 4\n\t\tSecTmpDir \"/Users/fzipitria/Workspace/OWASP/ModSecurity/tests/regression/server_root/tmp\"\n\t\tSecUploadKeepFiles On\n\t\tSecUploadDir \"/Users/fzipitria/Workspace/OWASP/ModSecurity/tests/regression/server_root/upload\"\n\t","match_log":{"-debug":[{"__regex__":1,"flags":"","pattern":"Failed to "},1],"debug":[{"__regex__":1,"flags":"","pattern":"Created temporary file.*/Users/fzipitria/Workspace/OWASP/ModSecurity/tests/regression/server_root/tmp"},1]},"match_response":{"status":{"__regex__":1,"flags":"","pattern":"^200$"}},"request":{"__type__":"http_request","content":"LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0xOTgxMzE4MTc3MTgzMDc2NTY0Mzk5NjE4NzIwNgpDb250ZW50LURpc3Bvc2l0aW9uOiBmb3JtLWRhdGE7IG5hbWU9InVwbG9hZC1maWxlIjsgZmlsZW5hbWU9InRlc3QiCkNvbnRlbnQtVHlwZTogYXBwbGljYXRpb24vb2N0ZXQtc3RyZWFtCgpURVNURklMRQotLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLTE5ODEzMTgxNzcxODMwNzY1NjQzOTk2MTg3MjA2CkNvbnRlbnQtRGlzcG9zaXRpb246IGZvcm0tZGF0YTsgbmFtZT0iZmlsZSIKClVwbG9hZCBGaWxlCi0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tMTk4MTMxODE3NzE4MzA3NjU2NDM5OTYxODcyMDYtLQ==","headers":[["Content-Type","multipart/form-data; boundary=---------------------------19813181771830765643996187206"]],"method":"POST","uri":"http://localhost:8088/test.txt"},"type":"config","unsupported":"test"},{"comment":"SecWebAppId","conf":"\n\t\tSecRuleEngine On\n\t\tSecRequestBodyAccess On\n\t\tSecDebugLog /Users/fzipitria/Workspace/OWASP/ModSecurity/tests/regression/server_root/logs/modsec_debug.log\n\t\tSecDebugLogLevel 4\n\t\tSecAuditLog \"/Users/fzipitria/Workspace/OWASP/ModSecurity/tests/regression/server_root/logs/modsec_audit.log\"\n\t\tSecAuditEngine RelevantOnly\n\t\tSecWebAppId \"app-1\"\n\t\tSecAction \"pass,log,auditlog,id:1\"\n\t","match_log":{"audit":[{"__regex__":1,"flags":"m","pattern":"^WebApp-Info: \"app-1\""},1],"debug":[{"__regex__":1,"flags":"","pattern":"Warning\\. Unconditional match in SecAction\\."},1],"error":[{"__regex__":1,"flags":"","pattern":"Warning\\. Unconditional match in SecAction\\."},1]},"match_response":{"status":{"__regex__":1,"flags":"","pattern":"^200$"}},"request":{"__type__":"http_request","content":"","headers":[],"method":"GET","uri":"http://localhost:8088/test.txt"},"type":"config"}] No newline at end of file | |||
Comment on lines
+52
to
+76
| # Same %ENV keys as run-regression-tests.pl, so conf/request strings that | ||
| # interpolate $ENV{...} evaluate identically here and at real test-run time. | ||
| %ENV = ( | ||
| %ENV, | ||
| SERVER_ROOT => $server_root, | ||
| SERVER_PORT => 8088, | ||
| SERVER_NAME => "localhost", | ||
| TEST_SERVER_ROOT => $SROOT_DIR, | ||
| DATA_DIR => $DATA_DIR, | ||
| TEMP_DIR => $TEMP_DIR, | ||
| UPLOAD_DIR => $UPLOAD_DIR, | ||
| CONF_DIR => $CONF_DIR, | ||
| MODULES_DIR => $MODULES_DIR, | ||
| LOGS_DIR => $FILES_DIR, | ||
| SCRIPT_DIR => $SCRIPT_DIR, | ||
| REGRESSION_DIR => $REG_DIR, | ||
| DIST_ROOT => File::Spec->rel2abs(dirname("$SCRIPT_DIR/../..")), | ||
| AUDIT_LOG => "$FILES_DIR/modsec_audit.log", | ||
| DEBUG_LOG => "$FILES_DIR/modsec_debug.log", | ||
| ERROR_LOG => "$FILES_DIR/error.log", | ||
| HTTPD_CONF => "$CONF_DIR/httpd.conf", | ||
| HTDOCS => "$SROOT_DIR/htdocs", | ||
| USER_AGENT => "ModSecurity Regression Tests/1.2.3", | ||
| RUNASUSER => $ENV{USER} || $ENV{LOGNAME} || $ENV{USERNAME} || 'unknown', | ||
| ); |
Comment on lines
+125
to
+128
| result = unit_test.run_unit_test(test_config) | ||
| # Note: Some invalid cases might behave differently than expected | ||
| # This is testing the actual behavior rather than ideal behavior | ||
| assert result.success or case["ret"] == 0, f"Invalid input test failed: {case['desc']}" |
Comment on lines
+4
to
+28
| @pytest.mark.unit | ||
| @pytest.mark.parametrize("param,input_data,expected_ret,test_name", [ | ||
| # Empty tests | ||
| ("", "", 1, "Empty param and input"), | ||
| ("TestCase", "", 0, "Empty input with param"), | ||
| ("", "TestCase", 1, "Empty param with input"), | ||
|
|
||
| # General tests | ||
| ("abcdef", "abcdef", 1, "Exact match"), | ||
| ("abcdef", "abcdefghi", 1, "Input starts with param"), | ||
| ("abcdef", "abc", 0, "Input shorter than param"), | ||
|
|
||
| # Case sensitivity tests | ||
| ("ABCDEF", "abcdef", 0, "Case mismatch - uppercase param"), | ||
| ("abcdef", "ABCDEF", 0, "Case mismatch - uppercase input"), | ||
|
|
||
| # Special characters | ||
| ("test@example.com", "test@example.com/path", 1, "Email prefix match"), | ||
| ("http://", "http://example.com", 1, "URL prefix match"), | ||
| ("hello world", "hello world test", 1, "Space in param"), | ||
|
|
||
| # Unicode tests | ||
| ("café", "café au lait", 1, "Unicode prefix match"), | ||
| ("测试", "测试数据", 1, "Chinese characters"), | ||
| ]) |
Comment on lines
+75
to
+81
| def _run_control(self, extra_conf: Optional[str], action: str) -> subprocess.CompletedProcess: | ||
| cmd = [ | ||
| self.httpd_path, | ||
| "-d", self.apache_server_root, | ||
| "-f", str(self.base_conf), | ||
| "-c", f"Listen {self.server_name}:{self.port}", | ||
| ] |
- POOL_DEBUG_RE: rewrite the two unbounded [^\n]+ groups around a literal as a lookahead + single unbounded group, removing the superlinear backtracking risk on inputs without a match (S8786). Verified byte-identical output across the original's edge cases. - Extract _check_match_spec(), shared by the match_response/match_log/ match_file loops in run_regression_test - cuts its cognitive complexity from 41 to under the limit and removes the duplicated "matched (expected no match)"/"failed to match" literals (S3776, S1192). - TestResult.log_matches: Dict[...] = None -> Optional[Dict[...]] (S5890). - _make_request: dict comprehension over `request["headers"]` pairs -> dict() (S7500). No behavior change: full regression suite (241 passed, 5 skipped) matches pre-refactor results exactly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
tests/test_operators/ and tests/test_transformations/ hold literal test data for the @ipMatch/@geoLookup operators, which is IP addresses by definition - SonarCloud flagged 5207 instances of "hardcoded IP address" there, which single-handedly failed the PR's maintainability quality gate. Scoped to just this rule and these two paths; every other rule still applies. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- tests/pytest.ini: [tool:pytest] is only honored in setup.cfg, not a
standalone pytest.ini (needs [pytest] there). Confirmed empirically:
--strict-markers was silently not enforced (an unregistered marker
only warned, never errored) - the markers that did appear were
registered by conftest.py's pytest_configure hook, not this file.
addopts/testpaths/filterwarnings were all being silently ignored.
- dump_regression_fixtures.pl / regression_fixtures.py: the dumper
baked this-machine's absolute paths (from $ENV{DEBUG_LOG} etc.
interpolated into .t files) directly into the checked-in fixture
JSON, which would fail regression tests (and the CI fixture-
staleness check) on any other machine, including CI. Real paths are
still used while evaluating .t files (so conf=>sub{} coderefs that
do real file I/O keep working), but the dumper now swaps them for
portable "##KEY##" placeholders in the JSON it emits, longest value
first so a shorter path that's a prefix of a longer one doesn't
break the longer match. regression_fixtures.py resolves the same
keys for the local machine and substitutes them back in when
loading a fixture. Verified zero absolute local paths remain in any
checked-in fixture, and the full regression suite still passes.
- test_base64decode.py: `assert result.success or case["ret"] == 0`
always passed for ret=0 cases regardless of the actual result,
masking a real bug - the "Completely invalid" case's expected
output was simply wrong (verified against msc_test directly:
base64Decode lenient-decodes valid-looking characters rather than
failing, producing specific non-empty bytes, not "" with ret=0).
Fixed the expected data and removed the always-true fallback.
- apache_server.py: only pass `-d <ServerRoot>` to httpd when
_resolve_server_root() actually found one, instead of passing -d
with an empty string on failure.
- Corrected the PR description: two hand-written unit test files
(test_beginswith.py, test_base64decode.py) carry edge-case coverage
beyond their source .t file, contradicting the "same test data, no
new cases" framing for the migration overall.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
sonar-project.properties is read by CI/scanner-based analysis; SonarCloud's Automatic Analysis mode (what this project uses - no scanner invocation exists in any workflow) reads .sonarcloud.properties instead, which is why the S1313 issue-ignore rule from the previous commit had no effect (5207 code smells, unchanged). Also drops sonar.projectKey/sonar.organization, which automatic analysis doesn't support overriding, and simplifies the resourceKey glob from tests/test_operators/**/*.py to tests/test_operators/*.py to match the documented working pattern (both directories are flat, no ** needed). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The previous commit's rename landed with stale pre-edit content (a failed git add pathspec for the old, now-deleted filename caused the whole staging command to abort, so the commit picked up whatever was already staged from the earlier git mv). This is the edit that was supposed to land: drop sonar.projectKey/sonar.organization (automatic analysis doesn't support overriding project identity from this file - it's bound via the GitHub App integration), and simplify the resourceKey glob from tests/test_operators/**/*.py to tests/test_operators/*.py to match the documented working pattern (both directories are flat, no ** needed). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Adds a pytest-based test framework under
tests/, alongside the existing Perl-basedtest/andtests/run-regression-tests.plinfrastructure. It covers the same test data (tests/op/*.t,tests/tfn/*.t,tests/regression/*/*.t) rather than introducing new test cases, with two exceptions noted below - those.tfiles (where kept, see below) remain the source of truth.tests/test_operators/,tests/test_transformations/): run each@operator/t:transformationcase through themsc_testC binary. No Apache needed. 4228 cases. These were migrated fromtests/op/*.t/tests/tfn/*.tvia a one-time Perl-assisted conversion, then the.tfiles and conversion tooling were removed - that data is flat literal strings with no ongoing need for a Perl round-trip, so the generated.pyfiles are now hand-maintained directly (add/edit a parametrize tuple like any other pytest file). Two of these files,test_beginswith.pyandtest_base64decode.py, carry hand-added edge-case coverage (Unicode input, long strings, binary data) beyond what their source.tfile had - pre-existing from this framework's initial scaffolding, not new in this PR, but worth calling out against the "same test data" framing above.tests/run-unit-tests.pl(the Perl runner for this same data) is removed along with itsMakefile.am/configure.acwiring, since it has no data left to run against;make check/make teststill succeed, just with nothing left to run intests/.tests/test_regression/): run against a live Apache +mod_security2, reusing the autoconf-generatedtests/regression/server_root/conf/httpd.conf. Unlike unit tests,tests/regression/*/*.tstays as the permanent source of truth -qr//regex flags,HTTP::Requestconstruction, and occasionalconf => sub {...}coderefs are Perl semantics worth keeping Perl around to parse rather than hand-maintaining in a flattened form.dump_regression_fixtures.plevaluates each.tfile the same wayrun-regression-tests.pldoes (same%ENV, same eval) and serializes the result totests/regression/fixtures/*.json, with this-machine absolute paths replaced by portable##PLACEHOLDER##tokens thatregression_fixtures.pyresolves back to local paths at load time;test_regression/test_fixtures.pydiscovers and runs every entry in those fixtures, one pytest case each - no per-.t-file Python to hand-maintain.tests/regenerate_regression_fixtures.shregenerates the fixtures after editing a.tfile; CI fails if they're stale. 246 cases: 241 pass, 5 skipped with a stated reason (1 relies on a Perl coderef this migration doesn't execute; 4 check exact HTTP request-header bytes that depend on the client library's fingerprint -requests/urllib3vs. the originalLWP::UserAgent)..github/workflows/ci.yml:test-linuxandtest-regression-linuxnow build/run these pytest suites as part of their existing steps, in place of themake test/make test-regressioninvocations that drove the Perl runners.tools/legacy/: two Perl scripts (gen_rx-pm.pl.in,csv_rx-pm.pl.in) moved here fromtests/- they were only ever auto-generated byconfigure, never invoked by any Makefile target or CI step.sonar-project.properties: scopes SonarCloud's hardcoded-IP-address rule out oftests/test_operators//tests/test_transformations/, since that's literal test data for the@ipMatch/@geoLookupoperators, not a real code smell.tests/README.mddocuments the structure above and how to run/regenerate each suite.Test plan
pytest tests/test_operators tests/test_transformations- 4228 passedpytest tests/test_regression- 241 passed, 5 skipped (reasons documented intests/README.md)pytestrun fromtests/- 4469 passed, 5 skippedtests/regenerate_regression_fixtures.shproduces byte-identical output to what's checked in (verifies the CI staleness check)grep -rlacrosstests/regression/fixtures/for the local checkout path)autogen.sh/configurestill succeed after removingrun-unit-tests.pl(.in)and moving the two legacy Perl scriptsmake checkintests/still succeeds (buildsmsc_test, runs no tests there now)actionlint/zizmoron the updatedci.ymlshow no new findings beyond the file's pre-existing style🤖 Generated with Claude Code