Skip to content

Address Copilot review feedback for phase‑4 handling - #12

Merged
Easton97-Jens merged 5 commits into
master-phase4from
codex/fetch-and-evaluate-copilot-comments-for-pr-#377
May 14, 2026
Merged

Address Copilot review feedback for phase‑4 handling#12
Easton97-Jens merged 5 commits into
master-phase4from
codex/fetch-and-evaluate-copilot-comments-for-pr-#377

Conversation

@Easton97-Jens

Copy link
Copy Markdown
Owner

Motivation

  • Improve correctness and operational observability of the new phase‑4 handling and its JSON logging as flagged by Copilot review comments.
  • Avoid unnecessary allocation/copy overhead and add safer error reporting for config file loading and log writes.
  • Make phase‑4 Test::Nginx tests stricter and meaningful so they actually validate the new behaviour.

Description

  • Only cache intervention.log into the request context when a phase‑4 JSON log file is configured to avoid unnecessary allocation and failure paths (src/ngx_http_modsecurity_module.c).
  • Report open() failures (with ngx_errno) when loading modsecurity_phase4_content_types_file to aid diagnosis of permission/race errors (src/ngx_http_modsecurity_module.c).
  • Remove unused phase4_content_type field from the request context to simplify the phase‑4 flow (src/ngx_http_modsecurity_common.h).
  • Ensure phase‑4 event wanted_action reflects redirects vs denies by emitting redirect for 3xx interventions and deny otherwise (src/ngx_http_modsecurity_body_filter.c).
  • Check the return value of ngx_write_fd() when writing phase‑4 JSON logs and log a warning / return error on short/failed writes to avoid silent data loss (src/ngx_http_modsecurity_body_filter.c).
  • Tighten and fix tests to reflect correct behaviour: convert the invalid‑config placeholder into a real startup‑failure assertion (tests/modsecurity-phase4-invalid-config.t), require mode_safe specifically (tests/modsecurity-phase4-modes.t), remove a duplicated regex branch (tests/modsecurity-phase4-content-types.t), and make the HTTP/2 proxy test daemon return a phase‑4 trigger response with an in‑scope Content-Type: text/html so strict mode testing is valid (tests/modsecurity-proxy-h2.t).
  • Minor housekeeping: removed duplicate phase‑4 macro definitions that are provided in the shared header (src/ngx_http_modsecurity_module.c).

Testing

  • Copilot review data for PR Implement Phase 4 handling for ModSecurity-nginx owasp-modsecurity/ModSecurity-nginx#377 was retrieved via the GitHub REST API and all Copilot review comments were evaluated before changes were applied.
  • Attempted to run the requested Test::Nginx suites with prove for the phase‑4 tests (tests/modsecurity-phase4-*.t, plus related tests/modsecurity*.t), but the environment lacks the Test::Nginx Perl module, so the test run failed with "Can't locate Test/Nginx.pm" and no automated Test::Nginx tests executed.
  • Static verification performed: inspected and adjusted C allocations, string lifetimes, logging calls, and test expectations; changes were committed as Address Copilot review feedback for phase4 handling.

Codex Task

@sonarqubecloud

Copy link
Copy Markdown

@Easton97-Jens
Easton97-Jens merged commit beb3e87 into master-phase4 May 14, 2026
25 checks passed
@Easton97-Jens
Easton97-Jens deleted the codex/fetch-and-evaluate-copilot-comments-for-pr-#377 branch May 14, 2026 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant