Conversation
Subrequests share the parent's pool, so the cleanup-list fallback in ngx_http_modsecurity_get_module_ctx() returns the main request's transaction for them. With auth_request the auth subrequest's response headers were fed to phase 3 as the "response headers" of the main request and ctx->processed was set, so the real response headers were never inspected. With log_subrequest on, the subrequest also runs the LOG phase, so msc_process_logging() was called on the main request's transaction once for the subrequest and once for the main request: phase 5 and the audit log ran twice for a single transaction. Skip subrequests in the header filter, in the body filter and in the log handler; the main request's own headers, body and logging phase are still processed. The body filter cannot currently see a subrequest, because the module is ordered after the postpone filter and that one forwards subrequest output as r->main, but the check is kept so the module stays correct if the filter order ever changes. No check is needed in the access handler: ngx_http_core_access_phase() already skips the whole ACCESS phase for subrequests. Behaviour change: subrequests are no longer inspected at all, even when the parent location has modsecurity off and the subrequest's location has it on. A subrequest never produces a response of its own, so there is nothing for the WAF to protect there.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 4 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughSubrequests now bypass ModSecurity body filtering, header inspection, and audit logging. Main requests retain the existing processing path. A new ChangesSubrequest bypass
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
1 participant
what
r != r->main). No change to the access handler: nginx core already skips the ACCESS phase for subrequests.tests/modsecurity-subrequest.t(auth_request+log_subrequest on): the main response's headers are inspected in phase 3, the auth subrequest's response is not, and exactly one audit record is written per request.why
ngx_http_modsecurity_get_module_ctx()falls back to walking the pool cleanup list when a request has no context of its own. A subrequest therefore gets the main request's transaction.auth_request, the auth subrequest's response headers were fed to phase 3 as the main request's response headers andctx->processedwas set, so the real response headers were never inspected (the new test fails on master with the rule for the main response header absent and the rule for the subrequest's status present).log_subrequest on, the LOG phase also runs for the subrequest, somsc_process_logging()ran twice on one transaction: phase 5 and the audit log executed twice (the new test counts two audit records on master, one after the fix).modsecurity onwhile the parent has it off. A subrequest produces no response of its own, so there is nothing for the WAF to protect there. The body-filter check is unreachable with the module's enforced filter order (the postpone filter forwards subrequest output asr->main) and is kept as a cheap safeguard should that order ever change.references
Origin: this change comes from a performance review of the connector done with Claude Fable 5.1 (Anthropic). The patch and its test were verified by building the module against nginx master with libmodsecurity 3.0.14 (PCRE2) and, with upstream CI's flags (
--without-pcre2 --with-http_v2_module --with-http_auth_request_module), against libmodsecurity 3.0.9 (PCRE1), then running the fulltests/modsecurity*.tsuite in both builds (16 files, 256 tests, all passing).Summary by CodeRabbit
Bug Fixes
Tests