Allow connectors to avoid unused intervention log payload overhead - #3606
Open
upgle wants to merge 1 commit into
Open
Allow connectors to avoid unused intervention log payload overhead#3606upgle wants to merge 1 commit into
upgle wants to merge 1 commit into
Conversation
Allow C and C++ connectors to omit unused intervention log text while preserving disruptive actions and all other logging channels. Keep the default behavior and v3 object layout unchanged, and cover every current payload-producing path with regression tests.
📝 WalkthroughWalkthroughChangesThe change adds instance-level C++ and C APIs to control intervention log payload generation. Payload creation now uses shared logic across disruptive actions and body-limit rejections. Regression tests cover enabled, disabled, default, and response-preservation behavior. Intervention log payload control
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant APIClient
participant ModSecurity
participant Transaction
participant InterventionLog
APIClient->>ModSecurity: setInterventionLogPayloadEnabled(enabled)
ModSecurity->>ModSecurity: update internal payload flag
ModSecurity->>Transaction: apply setting to created transactions
Transaction->>InterventionLog: setLogPayload(rule message)
InterventionLog->>Transaction: clear or store intervention payload
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/regression/regression.cc (1)
166-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the disable-then-enable pattern.
For
enabled != 0, the code disables the payload first, then re-enables it with the actual value. This exercises the real enable transition instead of relying on the already-enabled default, but the reason isn't stated in a comment. A future maintainer could "simplify" this to a single call and silently weaken the regression coverage for the enable path.Add a short comment explaining why the disable call precedes the enable call.
📝 Proposed clarifying comment
if (t->intervention_log_payload_api == "c") { if (enabled != 0) { + // Force a real disabled->enabled transition instead of + // relying on the library's already-enabled default, so this + // test actually exercises the enable path. modsecurity::msc_set_intervention_log_payload_enabled( &context.m_modsec, 0); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/regression/regression.cc` around lines 166 - 183, Add a concise comment in the intervention log payload configuration block before the conditional disable calls, explaining that disabling first forces a genuine enable transition and preserves regression coverage; keep the existing C API and C++ API call sequence unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/regression/regression.cc`:
- Around line 166-183: Add a concise comment in the intervention log payload
configuration block before the conditional disable calls, explaining that
disabling first forces a genuine enable transition and preserves regression
coverage; keep the existing C API and C++ API call sequence unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5059da81-87d1-4402-88c5-9b1e1ca53ca7
📒 Files selected for processing (15)
headers/modsecurity/modsecurity.hsrc/Makefile.amsrc/actions/disruptive/deny.ccsrc/actions/disruptive/drop.ccsrc/actions/disruptive/redirect.ccsrc/intervention_log.hsrc/modsecurity.ccsrc/transaction.cctest/common/modsecurity_test_context.htest/common/modsecurity_test_results.htest/regression/regression.cctest/regression/regression_test.cctest/regression/regression_test.htest/test-cases/regression/intervention-log.jsontest/test-suite.in
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
1 participant
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.
I'm excited to make my first contribution to ModSecurity
thank you for taking the time to review it :)
what
ModSecurityIntervention::logpayload generationAPI
modsecurity.setInterventionLogPayloadEnabled(false);Payload generation remains enabled by default; the C API treats any nonzero value as enabled. The setting is scoped to a
ModSecurityinstance; configure it before publishing to worker threads. Changing it during active transactions is unsupported.The implementation reuses a reserved private log-property bit, adding no object field or virtual function. Existing callback setters preserve the bit.
why
The need surfaced while testing ModSecurity in an Envoy integration. The connector consumes intervention status, redirect URLs, and bounded structured rule events, but not the free-form
ModSecurityIntervention::logpayload; ModSecurity still formats, allocates, copies, and frees it on disruptive requests. This opt-out is intended for high-throughput or latency-sensitive connectors with the same usage pattern while preserving intervention behavior and other logging channels.An isolated Linux release microbenchmark used 12 alternating-order paired rounds with 50,000 transactions per sample. Enabled and disabled medians were 20.290 and 14.735 microseconds per transaction; the 5.541-microsecond median paired difference represents 27.3% lower phase-1 deny time, with all 12 pairs favoring omission.
Summary by CodeRabbit
New Features
Tests