Skip to content

Commit 1496397

Browse files
fzipiclaude
andcommitted
fix: address Copilot review findings on PR owasp-modsecurity#3594
- 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>
1 parent 2c7af26 commit 1496397

25 files changed

Lines changed: 120 additions & 36 deletions

tests/apache_server.py

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -73,9 +73,10 @@ def __init__(self, log_matcher, port: int = 8088, server_name: str = "localhost"
7373
self._per_test_conf: Optional[Path] = None
7474

7575
def _run_control(self, extra_conf: Optional[str], action: str) -> subprocess.CompletedProcess:
76-
cmd = [
77-
self.httpd_path,
78-
"-d", self.apache_server_root,
76+
cmd = [self.httpd_path]
77+
if self.apache_server_root:
78+
cmd += ["-d", self.apache_server_root]
79+
cmd += [
7980
"-f", str(self.base_conf),
8081
"-c", f"Listen {self.server_name}:{self.port}",
8182
]

tests/dump_regression_fixtures.pl

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,13 @@
5151

5252
# Same %ENV keys as run-regression-tests.pl, so conf/request strings that
5353
# interpolate $ENV{...} evaluate identically here and at real test-run time.
54+
# These are real, absolute, this-machine paths - needed as-is so that
55+
# conf=>sub{} coderefs (e.g. config/00-load-modsec.t opens a real file via
56+
# $ENV{DIST_ROOT}) work correctly during eval. Every one of them gets
57+
# replaced by a portable placeholder token in the final JSON output (see
58+
# %PLACEHOLDER_FOR below) so the checked-in fixtures don't hard-code this
59+
# machine's paths - the Python harness substitutes its own local paths back
60+
# in before starting Apache.
5461
%ENV = (
5562
%ENV,
5663
SERVER_ROOT => $server_root,
@@ -239,4 +246,27 @@ sub serialize_request {
239246
push @out, \%entry;
240247
}
241248

242-
print JSON::PP->new->canonical->utf8->encode(\@out);
249+
my $json = JSON::PP->new->canonical->utf8->encode(\@out);
250+
251+
# Swap this machine's real absolute paths for portable placeholder tokens
252+
# (regression_fixtures.py substitutes its own local paths back in before
253+
# starting Apache). Longest values first, so a shorter path that's a prefix
254+
# of a longer one (e.g. SCRIPT_DIR is a prefix of DEBUG_LOG) doesn't get
255+
# substituted first and break the longer match.
256+
my @path_keys = qw(
257+
DEBUG_LOG AUDIT_LOG ERROR_LOG HTTPD_CONF
258+
DATA_DIR TEMP_DIR UPLOAD_DIR CONF_DIR LOGS_DIR HTDOCS
259+
TEST_SERVER_ROOT REGRESSION_DIR SCRIPT_DIR DIST_ROOT
260+
SERVER_ROOT MODULES_DIR RUNASUSER
261+
);
262+
for my $key (sort { length($ENV{$b}) <=> length($ENV{$a}) } @path_keys) {
263+
my $value = $ENV{$key};
264+
next unless defined $value and length $value;
265+
# Plain printable text, not a control character: JSON forbids raw control
266+
# bytes inside string literals, and this way the substitution can operate
267+
# on the already-serialized JSON text without worrying about escaping.
268+
my $placeholder = "##${key}##";
269+
$json =~ s/\Q$value\E/$placeholder/g;
270+
}
271+
272+
print $json;

tests/pytest.ini

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
[tool:pytest]
1+
[pytest]
22
minversion = 7.0
33
testpaths = .
44
python_files = test_*.py

tests/regression/fixtures/action/00-disruptive-actions.json

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
[{"comment":"append content","conf":"\n\t\tSecRuleEngine On\n SecContentInjection On\n\t\tSecDebugLog \"/Users/fzipitria/Workspace/OWASP/ModSecurity/tests/regression/server_root/logs/modsec_debug.log\"\n\t\tSecDebugLogLevel 9\n\t\tSecAction \"phase:1,setvar:tx.test=test,id:500002\"\n\t\tSecAction \"phase:2,append:'APPEND: %{tx.test}',id:500003\"\n\t","match_log":{"debug":[{"__regex__":1,"flags":"","pattern":"Added content to bottom: APPEND: test"},1]},"match_response":{"content":{"__regex__":1,"flags":"","pattern":"APPEND: test$"},"status":{"__regex__":1,"flags":"","pattern":"^200$"}},"request":{"__type__":"http_request","content":"","headers":[],"method":"GET","uri":"http://localhost:8088/test.txt"},"type":"action"},{"comment":"prepend content","conf":"\n\t\tSecRuleEngine On\n SecContentInjection On\n\t\tSecDebugLog \"/Users/fzipitria/Workspace/OWASP/ModSecurity/tests/regression/server_root/logs/modsec_debug.log\"\n\t\tSecDebugLogLevel 9\n\t\tSecAction \"phase:1,setvar:tx.test=test,id:500004\"\n\t\tSecAction \"phase:2,prepend:'PREPEND: %{tx.test}',id:500005\"\n\t","match_log":{"debug":[{"__regex__":1,"flags":"","pattern":"Added content to top: PREPEND: test"},1]},"match_response":{"content":{"__regex__":1,"flags":"","pattern":"^PREPEND: test"},"status":{"__regex__":1,"flags":"","pattern":"^200$"}},"request":{"__type__":"http_request","content":"","headers":[],"method":"GET","uri":"http://localhost:8088/test.txt"},"type":"action"}]
1+
[{"comment":"append content","conf":"\n\t\tSecRuleEngine On\n SecContentInjection On\n\t\tSecDebugLog \"##DEBUG_LOG##\"\n\t\tSecDebugLogLevel 9\n\t\tSecAction \"phase:1,setvar:tx.test=test,id:500002\"\n\t\tSecAction \"phase:2,append:'APPEND: %{tx.test}',id:500003\"\n\t","match_log":{"debug":[{"__regex__":1,"flags":"","pattern":"Added content to bottom: APPEND: test"},1]},"match_response":{"content":{"__regex__":1,"flags":"","pattern":"APPEND: test$"},"status":{"__regex__":1,"flags":"","pattern":"^200$"}},"request":{"__type__":"http_request","content":"","headers":[],"method":"GET","uri":"http://localhost:8088/test.txt"},"type":"action"},{"comment":"prepend content","conf":"\n\t\tSecRuleEngine On\n SecContentInjection On\n\t\tSecDebugLog \"##DEBUG_LOG##\"\n\t\tSecDebugLogLevel 9\n\t\tSecAction \"phase:1,setvar:tx.test=test,id:500004\"\n\t\tSecAction \"phase:2,prepend:'PREPEND: %{tx.test}',id:500005\"\n\t","match_log":{"debug":[{"__regex__":1,"flags":"","pattern":"Added content to top: PREPEND: test"},1]},"match_response":{"content":{"__regex__":1,"flags":"","pattern":"^PREPEND: test"},"status":{"__regex__":1,"flags":"","pattern":"^200$"}},"request":{"__type__":"http_request","content":"","headers":[],"method":"GET","uri":"http://localhost:8088/test.txt"},"type":"action"}]

tests/regression/fixtures/action/10-detectiononly-actions.json

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.

tests/regression/fixtures/action/10-logging.json

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
[{"comment":"SecAuditEngine On","conf":"\n\t\tSecAuditEngine On\n\t\tSecAuditLog /Users/fzipitria/Workspace/OWASP/ModSecurity/tests/regression/server_root/logs/modsec_audit.log\n\t","match_log":{"audit":[{"__regex__":1,"flags":"","pattern":"."},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":"SecAuditEngine Off","conf":"\n\t\tSecAuditEngine Off\n\t\tSecAuditLog /Users/fzipitria/Workspace/OWASP/ModSecurity/tests/regression/server_root/logs/modsec_audit.log\n\t","match_log":{"-audit":[{"__regex__":1,"flags":"","pattern":"."},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":"SecAuditEngine RelevantOnly (pos)","conf":"\n\t\tSecRuleEngine On\n\t\tSecAuditEngine RelevantOnly\n\t\tSecAuditLog /Users/fzipitria/Workspace/OWASP/ModSecurity/tests/regression/server_root/logs/modsec_audit.log\n\t\tSecDebugLog /Users/fzipitria/Workspace/OWASP/ModSecurity/tests/regression/server_root/logs/modsec_debug.log\n\t\tSecDebugLogLevel 9\n\t\tSecResponseBodyAccess On\n\t\tSecDefaultAction \"phase:2,log,auditlog,pass\"\n\t\tSecRule REQUEST_URI \".\" \"phase:4,deny,id:500251\"\n\t","match_log":{"audit":[{"__regex__":1,"flags":"","pattern":"."},1]},"match_response":{"status":{"__regex__":1,"flags":"","pattern":"^403$"}},"request":{"__type__":"http_request","content":"","headers":[],"method":"GET","uri":"http://localhost:8088/test.txt"},"type":"config"},{"comment":"SecAuditEngine RelevantOnly (neg)","conf":"\n\t\tSecAuditEngine RelevantOnly\n\t\tSecAuditLog /Users/fzipitria/Workspace/OWASP/ModSecurity/tests/regression/server_root/logs/modsec_audit.log\n\t\tSecResponseBodyAccess On\n\t\tSecDefaultAction \"phase:2,log,auditlog,pass\"\n\t","match_log":{"-audit":[{"__regex__":1,"flags":"","pattern":"."},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":"SecAuditLogType Serial","conf":"\n\t\tSecAuditEngine On\n\t\tSecAuditLog /Users/fzipitria/Workspace/OWASP/ModSecurity/tests/regression/server_root/logs/modsec_audit.log\n\t\tSecAuditLogType Serial\n\t","match_log":{"audit":[{"__regex__":1,"flags":"","pattern":"."},1]},"match_response":{"status":{"__regex__":1,"flags":"","pattern":"^404$"}},"request":{"__type__":"http_request","content":"","headers":[],"method":"GET","uri":"http://localhost:8088/bogus"},"type":"config"},{"comment":"SecAuditLogRelevantStatus (pos)","conf":"\n\t\tSecAuditEngine RelevantOnly\n\t\tSecAuditLog /Users/fzipitria/Workspace/OWASP/ModSecurity/tests/regression/server_root/logs/modsec_audit.log\n\t\tSecAuditLogRelevantStatus \"^4\"\n\t","match_log":{"audit":[{"__regex__":1,"flags":"","pattern":"."},1]},"match_response":{"status":{"__regex__":1,"flags":"","pattern":"^404$"}},"request":{"__type__":"http_request","content":"","headers":[],"method":"GET","uri":"http://localhost:8088/bogus"},"type":"config"},{"comment":"SecAuditLogRelevantStatus (neg)","conf":"\n\t\tSecAuditEngine RelevantOnly\n\t\tSecAuditLog /Users/fzipitria/Workspace/OWASP/ModSecurity/tests/regression/server_root/logs/modsec_audit.log\n\t\tSecAuditLogRelevantStatus \"^4\"\n\t","match_log":{"-audit":[{"__regex__":1,"flags":"","pattern":"."},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":"SecAuditLogParts (minimal)","conf":"\n\t\tSecAuditEngine On\n\t\tSecAuditLog /Users/fzipitria/Workspace/OWASP/ModSecurity/tests/regression/server_root/logs/modsec_audit.log\n\t\tSecRequestBodyAccess On\n\t\tSecResponseBodyAccess On\n\t\tSecAuditLogParts \"AZ\"\n\t","match_log":{"-audit":[{"__regex__":1,"flags":"","pattern":"-[B-Y]--"},1],"audit":[{"__regex__":1,"flags":"s","pattern":"-A--.*-Z--"},1]},"match_response":{"status":{"__regex__":1,"flags":"","pattern":"^200$"}},"request":{"__type__":"http_request","content":"YT0xciY9Mg==","headers":[["Content-Type","application/x-www-form-urlencoded"]],"method":"POST","uri":"http://localhost:8088/test.txt"},"type":"config"},{"comment":"SecAuditLogParts (default)","conf":"\n\t\tSecAuditEngine On\n\t\tSecAuditLog /Users/fzipitria/Workspace/OWASP/ModSecurity/tests/regression/server_root/logs/modsec_audit.log\n\t\tSecRequestBodyAccess On\n\t\tSecResponseBodyAccess On\n\t","match_log":{"-audit":[{"__regex__":1,"flags":"","pattern":"-[DEGIJK]--"},1],"audit":[{"__regex__":1,"flags":"s","pattern":"-A--.*-B--.*-F--.*-H--.*-Z--"},1]},"match_response":{"status":{"__regex__":1,"flags":"","pattern":"^200$"}},"request":{"__type__":"http_request","content":"YT0xciY9Mg==","headers":[["Content-Type","application/x-www-form-urlencoded"]],"method":"POST","uri":"http://localhost:8088/test.txt"},"type":"config"},{"comment":"SecAuditLogParts (all)","conf":"\n\t\tSecRuleEngine On\n\t\tSecAuditEngine On\n\t\tSecAuditLog /Users/fzipitria/Workspace/OWASP/ModSecurity/tests/regression/server_root/logs/modsec_audit.log\n\t\tSecRequestBodyAccess On\n\t\tSecResponseBodyAccess On\n\t\tSecAuditLogParts \"ABCDEFGHIJKZ\"\n\t\tSecAction \"phase:4,log,auditlog,allow,id:500086\"\n\t","match_log":{"audit":[{"__regex__":1,"flags":"s","pattern":"-A--.*-B--.*-C--.*-F--.*-E--.*-H--.*-K--.*-Z--"},1]},"match_response":{"status":{"__regex__":1,"flags":"","pattern":"^200$"}},"request":{"__type__":"http_request","content":"YT0xciY9Mg==","headers":[["Content-Type","application/x-www-form-urlencoded"]],"method":"POST","uri":"http://localhost:8088/test.txt"},"type":"config"}]
1+
[{"comment":"SecAuditEngine On","conf":"\n\t\tSecAuditEngine On\n\t\tSecAuditLog ##AUDIT_LOG##\n\t","match_log":{"audit":[{"__regex__":1,"flags":"","pattern":"."},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":"SecAuditEngine Off","conf":"\n\t\tSecAuditEngine Off\n\t\tSecAuditLog ##AUDIT_LOG##\n\t","match_log":{"-audit":[{"__regex__":1,"flags":"","pattern":"."},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":"SecAuditEngine RelevantOnly (pos)","conf":"\n\t\tSecRuleEngine On\n\t\tSecAuditEngine RelevantOnly\n\t\tSecAuditLog ##AUDIT_LOG##\n\t\tSecDebugLog ##DEBUG_LOG##\n\t\tSecDebugLogLevel 9\n\t\tSecResponseBodyAccess On\n\t\tSecDefaultAction \"phase:2,log,auditlog,pass\"\n\t\tSecRule REQUEST_URI \".\" \"phase:4,deny,id:500251\"\n\t","match_log":{"audit":[{"__regex__":1,"flags":"","pattern":"."},1]},"match_response":{"status":{"__regex__":1,"flags":"","pattern":"^403$"}},"request":{"__type__":"http_request","content":"","headers":[],"method":"GET","uri":"http://localhost:8088/test.txt"},"type":"config"},{"comment":"SecAuditEngine RelevantOnly (neg)","conf":"\n\t\tSecAuditEngine RelevantOnly\n\t\tSecAuditLog ##AUDIT_LOG##\n\t\tSecResponseBodyAccess On\n\t\tSecDefaultAction \"phase:2,log,auditlog,pass\"\n\t","match_log":{"-audit":[{"__regex__":1,"flags":"","pattern":"."},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":"SecAuditLogType Serial","conf":"\n\t\tSecAuditEngine On\n\t\tSecAuditLog ##AUDIT_LOG##\n\t\tSecAuditLogType Serial\n\t","match_log":{"audit":[{"__regex__":1,"flags":"","pattern":"."},1]},"match_response":{"status":{"__regex__":1,"flags":"","pattern":"^404$"}},"request":{"__type__":"http_request","content":"","headers":[],"method":"GET","uri":"http://localhost:8088/bogus"},"type":"config"},{"comment":"SecAuditLogRelevantStatus (pos)","conf":"\n\t\tSecAuditEngine RelevantOnly\n\t\tSecAuditLog ##AUDIT_LOG##\n\t\tSecAuditLogRelevantStatus \"^4\"\n\t","match_log":{"audit":[{"__regex__":1,"flags":"","pattern":"."},1]},"match_response":{"status":{"__regex__":1,"flags":"","pattern":"^404$"}},"request":{"__type__":"http_request","content":"","headers":[],"method":"GET","uri":"http://localhost:8088/bogus"},"type":"config"},{"comment":"SecAuditLogRelevantStatus (neg)","conf":"\n\t\tSecAuditEngine RelevantOnly\n\t\tSecAuditLog ##AUDIT_LOG##\n\t\tSecAuditLogRelevantStatus \"^4\"\n\t","match_log":{"-audit":[{"__regex__":1,"flags":"","pattern":"."},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":"SecAuditLogParts (minimal)","conf":"\n\t\tSecAuditEngine On\n\t\tSecAuditLog ##AUDIT_LOG##\n\t\tSecRequestBodyAccess On\n\t\tSecResponseBodyAccess On\n\t\tSecAuditLogParts \"AZ\"\n\t","match_log":{"-audit":[{"__regex__":1,"flags":"","pattern":"-[B-Y]--"},1],"audit":[{"__regex__":1,"flags":"s","pattern":"-A--.*-Z--"},1]},"match_response":{"status":{"__regex__":1,"flags":"","pattern":"^200$"}},"request":{"__type__":"http_request","content":"YT0xciY9Mg==","headers":[["Content-Type","application/x-www-form-urlencoded"]],"method":"POST","uri":"http://localhost:8088/test.txt"},"type":"config"},{"comment":"SecAuditLogParts (default)","conf":"\n\t\tSecAuditEngine On\n\t\tSecAuditLog ##AUDIT_LOG##\n\t\tSecRequestBodyAccess On\n\t\tSecResponseBodyAccess On\n\t","match_log":{"-audit":[{"__regex__":1,"flags":"","pattern":"-[DEGIJK]--"},1],"audit":[{"__regex__":1,"flags":"s","pattern":"-A--.*-B--.*-F--.*-H--.*-Z--"},1]},"match_response":{"status":{"__regex__":1,"flags":"","pattern":"^200$"}},"request":{"__type__":"http_request","content":"YT0xciY9Mg==","headers":[["Content-Type","application/x-www-form-urlencoded"]],"method":"POST","uri":"http://localhost:8088/test.txt"},"type":"config"},{"comment":"SecAuditLogParts (all)","conf":"\n\t\tSecRuleEngine On\n\t\tSecAuditEngine On\n\t\tSecAuditLog ##AUDIT_LOG##\n\t\tSecRequestBodyAccess On\n\t\tSecResponseBodyAccess On\n\t\tSecAuditLogParts \"ABCDEFGHIJKZ\"\n\t\tSecAction \"phase:4,log,auditlog,allow,id:500086\"\n\t","match_log":{"audit":[{"__regex__":1,"flags":"s","pattern":"-A--.*-B--.*-C--.*-F--.*-E--.*-H--.*-K--.*-Z--"},1]},"match_response":{"status":{"__regex__":1,"flags":"","pattern":"^200$"}},"request":{"__type__":"http_request","content":"YT0xciY9Mg==","headers":[["Content-Type","application/x-www-form-urlencoded"]],"method":"POST","uri":"http://localhost:8088/test.txt"},"type":"config"}]

0 commit comments

Comments
 (0)