Conversation
GoFurry
requested review from
ReneWerner87,
efectn,
gaby and
sixcolors
and removed request for
a team
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the Coraza Web Application Firewall (WAF) middleware for Fiber v3. The implementation includes a core Engine for managing WAF instances, support for manual reloads, and built-in metrics collection. Review feedback identifies a potential issue where an error from the request body reader is ignored, which could lead to downstream handlers receiving an empty request body. Additionally, a redundant call to the request body processing function was noted for removal to improve code clarity.
| req.Body = io.NopCloser(io.MultiReader(rbr, req.Body)) | ||
| } | ||
|
|
||
| return tx.ProcessRequestBody() |
There was a problem hiding this comment.
The tx.ReadRequestBodyFrom function, called earlier for accessible request bodies, already processes the body internally by calling tx.ProcessRequestBody. This final call to tx.ProcessRequestBody() is therefore redundant and could be confusing. It should be removed, and the function should return nil, nil to indicate that processing is complete without any interruptions.
| return tx.ProcessRequestBody() | |
| return nil, nil |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new Coraza WAF middleware for Fiber v3: documentation and CI badge, a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Fiber as Fiber Middleware
participant Engine as Coraza Engine
participant WAF as Coraza WAF / Transaction
participant Handler as Block/Error Handler
Client->>Fiber: HTTP request
Fiber->>Engine: Check Next() (bypass?)
alt Bypass
Fiber->>Fiber: Record request metrics
Fiber-->>Client: Forward request
else Inspect
Fiber->>Engine: Snapshot WAF / create transaction
Engine->>WAF: Create transaction (with/without options)
Fiber->>WAF: Process connection & headers
alt Header interruption
WAF-->>Fiber: Interruption
Fiber->>Handler: Invoke BlockHandler/ErrorHandler
Handler-->>Client: Block/Error response
else Headers OK
Fiber->>WAF: Optionally read & process body
alt Body interruption
WAF-->>Fiber: Interruption
Fiber->>Handler: Invoke BlockHandler
Handler-->>Client: Block response
else No interruption
Fiber->>Fiber: Record latency/metrics
Fiber-->>Client: Forward request
end
end
end
Fiber->>Engine: Close transaction / update metrics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v3/coraza/coraza.go`:
- Around line 56-62: The code currently enables matched-rule logging by default
via ConfigDefault (EnableErrorLog: true) and writes attacker-controlled data
using rule.URI() and rule.ErrorLog(); change this so matched-rule details are
either opt-in or redacted: add a distinct config flag (e.g.,
EnableMatchedRuleLogging or keep EnableErrorLog but default it to false in
ConfigDefault) and update the logging site(s) that call rule.URI() and
rule.ErrorLog() to check that flag before emitting raw values; if the flag is
disabled, log only a safe summary/identifier (e.g., rule.ID or masked text) or
redact/escape sensitive fragments prior to logging. Ensure references to
ConfigDefault, EnableErrorLog (or new EnableMatchedRuleLogging), and the logging
call sites that use rule.URI()/rule.ErrorLog() are updated accordingly.
- Around line 186-188: The code currently only applies cfg.BlockMessage when
non-empty so callers cannot reset to the package default; update both places
that set e.blockMessage (the Init function and the config update path that
references cfg.BlockMessage) to assign e.blockMessage = cfg.BlockMessage when
cfg.BlockMessage != "" else set e.blockMessage = defaultBlockMessage so an
explicit empty BlockMessage restores the defaultBlockMessage instead of
preserving the previous custom text. Ensure you update the assignments that
reference cfg.BlockMessage and e.blockMessage (including the Init method and the
config update logic) accordingly.
- Around line 386-395: The log reads e.reloadCount after releasing e.mu which
can race with another Reload(); fix by reading and storing e.reloadCount (and
any other fields needed for the log) while holding the lock in the Reload
implementation (use e.mu, e.reloadCount), then release e.mu and pass the local
variable into slog.Info (instead of accessing e.reloadCount after unlock) so the
logged value is consistent and race-free.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 226936d6-c7b3-415c-a90b-8ee19d4fcd2d
⛔ Files ignored due to path filters (4)
.github/workflows/test-coraza.ymlis excluded by!**/*.ymlgo.workis excluded by!**/*.work,!**/*.workv3/coraza/go.modis excluded by!**/*.modv3/coraza/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (5)
v3/README.mdv3/coraza/README.mdv3/coraza/coraza.gov3/coraza/coraza_test.gov3/coraza/metrics.go
📜 Review details
🧰 Additional context used
🪛 LanguageTool
v3/coraza/README.md
[grammar] ~5-~5: Ensure spelling is correct
Context: --- id: coraza --- # Coraza 
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (6)
v3/coraza/metrics.go (1)
81-120: LGTM: the metrics collector keeps the hot path lean.Atomics for request/block counters and a mutex only around latency aggregation are a good fit for middleware metrics, and the snapshot/report helpers return copy-style data instead of leaking mutable state.
Also applies to: 132-147
v3/coraza/coraza.go (2)
277-363: LGTM: the transaction wrapper fails closed.Recovering around both request inspection and transaction cleanup, then always calling
ProcessLogging()andClose(), makes the middleware much safer under panic/error paths.
541-548: No issue found —fiber.Ctx.Port()correctly returns the remote client port.In Fiber v3.1.0,
fiber.Ctx.Port()returns the remote (client) port, not the server port. The code at line 547 correctly populatesRemoteAddrwith the client IP and client port, which is whattx.ProcessConnectionexpects.v3/coraza/coraza_test.go (1)
331-431: Nice coverage on lifecycle and fail-closed behavior.The reload, failed re-init, and WAF panic tests hit the sharpest edges in the new
EngineAPI and give good protection against regressions there.v3/README.md (1)
24-24: LGTM: the index entry matches the new middleware and workflow.v3/coraza/README.md (1)
33-123: Nice README coverage for the public surface.The doc covers the config fields,
NewvsNewEngine, and the startup behavior around missing rule files, which are the key integration details for callers.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
v3/coraza/coraza.go (2)
361-362: Consider loggingtx.Close()errors.The error from
tx.Close()is silently discarded. While this avoids masking the primary error, failed cleanup could indicate resource leaks worth observing.♻️ Optional: log close errors
tx.ProcessLogging() - _ = tx.Close() + if err := tx.Close(); err != nil { + slog.Debug("[Coraza] transaction close failed", "error", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/coraza/coraza.go` around lines 361 - 362, The code currently discards the error from tx.Close() after calling tx.ProcessLogging(); change it to capture and log the Close error (e.g., err := tx.Close(); if err != nil { /* log at warn/debug level */ }) so failed cleanup is observable without masking any primary error; locate the calls to tx.ProcessLogging() and tx.Close() in coraza.go and add a non-fatal log (using the existing logger in scope) when tx.Close() returns an error.
514-528: Body reassignment may be unnecessary in Fiber context.Line 527 reconstructs
req.Bodyfor potential re-reading, following standard Coraza connector patterns. However, downstream Fiber handlers use the originalfiber.Ctx, not this converted*http.Request. The body reassignment has no effect on Fiber's request handling.If Fiber's body is needed downstream, consider whether this code path serves a purpose or can be simplified.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/coraza/coraza.go` around lines 514 - 528, The reassignment of req.Body after calling tx.ReadRequestBodyFrom and tx.RequestBodyReader (in coraza.go within the code handling tx.IsRequestBodyAccessible()) is unnecessary for Fiber because downstream handlers use fiber.Ctx rather than the converted *http.Request; remove or guard the io.NopCloser(io.MultiReader(rbr, req.Body)) assignment to avoid a no-op and potential confusion. Specifically, in the block that calls tx.ReadRequestBodyFrom(...) and tx.RequestBodyReader(...), either delete the req.Body = ... line or add a runtime check for Fiber context (or exported flag) and only reassign req.Body when running under a standard net/http handler, leaving Fiber’s body untouched. Ensure references to tx.ReadRequestBodyFrom and tx.RequestBodyReader remain and keep error handling unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@v3/coraza/coraza.go`:
- Around line 361-362: The code currently discards the error from tx.Close()
after calling tx.ProcessLogging(); change it to capture and log the Close error
(e.g., err := tx.Close(); if err != nil { /* log at warn/debug level */ }) so
failed cleanup is observable without masking any primary error; locate the calls
to tx.ProcessLogging() and tx.Close() in coraza.go and add a non-fatal log
(using the existing logger in scope) when tx.Close() returns an error.
- Around line 514-528: The reassignment of req.Body after calling
tx.ReadRequestBodyFrom and tx.RequestBodyReader (in coraza.go within the code
handling tx.IsRequestBodyAccessible()) is unnecessary for Fiber because
downstream handlers use fiber.Ctx rather than the converted *http.Request;
remove or guard the io.NopCloser(io.MultiReader(rbr, req.Body)) assignment to
avoid a no-op and potential confusion. Specifically, in the block that calls
tx.ReadRequestBodyFrom(...) and tx.RequestBodyReader(...), either delete the
req.Body = ... line or add a runtime check for Fiber context (or exported flag)
and only reassign req.Body when running under a standard net/http handler,
leaving Fiber’s body untouched. Ensure references to tx.ReadRequestBodyFrom and
tx.RequestBodyReader remain and keep error handling unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7e54b268-92bb-4a95-b9de-b3b977fdc21f
📒 Files selected for processing (1)
v3/coraza/coraza.go
📜 Review details
🔇 Additional comments (14)
v3/coraza/coraza.go (14)
1-23: LGTM!Package declaration and imports are appropriate for the WAF middleware implementation.
56-62: Matched-rule logging enabled by default may expose sensitive data.
EnableErrorLog: truecauseslogError()(lines 478-486) to emitrule.URI()andrule.ErrorLog()which can contain attacker-controlled request paths, query strings, and body fragments. Consider defaulting tofalseor redacting sensitive portions before logging.
[duplicate_comment, raise_major_issue]
64-104: LGTM!The middleware configuration types and handler signatures are well-structured with clear documentation.
106-127: LGTM!Engine struct is well-organized with clear separation of WAF state, configuration, and observability fields under mutex protection.
129-158: LGTM!The fail-fast panic in
New()is documented and appropriate for startup configuration errors.NewEngine()provides an error-returning alternative for advanced use cases.
186-188: No path to restore the default block message.When
cfg.BlockMessageis empty, the previouse.blockMessagevalue persists. Once a custom message is set, callers cannot reset todefaultBlockMessageby passing an empty string.Consider always assigning the value:
if cfg.BlockMessage != "" { e.blockMessage = cfg.BlockMessage } else { e.blockMessage = defaultBlockMessage }[duplicate_comment, raise_minor_issue]
194-203: Same limitation as Init: empty string doesn't restore default.Consider changing the guard to restore
defaultBlockMessagewhenmsgis empty instead of silently returning.
[duplicate_comment, raise_minor_issue]
205-275: LGTM!The
Middleware()implementation has proper lifecycle management, correct metrics recording calls, and comprehensive error handling with appropriate fallback to default handlers.
277-339: LGTM!Solid defensive implementation with panic recovery, proper transaction lifecycle management via deferred cleanup, and clear error propagation paths.
386-397: Data race:e.reloadCountread after mutex release.Line 395 accesses
e.reloadCountafter the mutex is unlocked on line 393. A concurrentReload()can modify this field, causing a race condition.🔒 Fix: capture value before unlocking
e.mu.Lock() e.waf = newWAF e.initErr = nil e.setWAFOptionsStateLocked(newWAF) e.reloadCount++ e.reloadSuccessCount++ e.lastLoadedAt = time.Now() + reloadCount := e.reloadCount e.mu.Unlock() - slog.Info("[Coraza] reload completed successfully", "reload_count", e.reloadCount) + slog.Info("[Coraza] reload completed successfully", "reload_count", reloadCount) return nil[duplicate_comment, raise_critical_issue]
399-440: LGTM!Helper methods have proper locking semantics, and
setWAFOptionsStateLocked()clearly indicates its lock precondition through naming convention.
442-467: LGTM!Proper defensive slice copying and consistent read lock usage for the observability snapshot.
469-476: LGTM!Default handlers are straightforward with appropriate header signaling for blocked requests.
533-630: LGTM!Helper functions are well-implemented with appropriate error handling, graceful fallbacks for edge cases (e.g., invalid remote addresses), and proper defensive copying in
cloneConfig().
There was a problem hiding this comment.
🧹 Nitpick comments (2)
v3/coraza/coraza_test.go (1)
254-258: Consider checking errors fromio.ReadAllin test assertions.Several test cases discard the error from
io.ReadAllwith a blank identifier. While this is common in tests where the response body is expected to be valid, explicitly checking for errors would make tests more robust and easier to debug if something unexpected happens.Example fix for one occurrence
firstResp := performRequest(t, firstApp, httptest.NewRequest(http.MethodGet, "/?attack=1", nil)) defer firstResp.Body.Close() - firstBody, _ := io.ReadAll(firstResp.Body) + firstBody, err := io.ReadAll(firstResp.Body) + if err != nil { + t.Fatalf("failed to read first response body: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/coraza/coraza_test.go` around lines 254 - 258, The test currently ignores errors returned by io.ReadAll when reading firstResp.Body and secondResp.Body; update the reads to capture and assert no error (e.g., err := io.ReadAll(...); require.NoError(t, err) or if err != nil { t.Fatalf(...) }) for both firstBody and secondBody so failures are reported clearly; apply this change where io.ReadAll is called for firstResp.Body and secondResp.Body in the test that uses performRequest, and ensure you close bodies before/after asserting errors as needed.v3/coraza/coraza.go (1)
567-584: Wildcard paths skip validation, relying on Coraza to handle errors.The
validateDirectivesFilefunction skips validation for paths containing*(glob patterns). This is reasonable since the actual file resolution happens inside Coraza'sWithDirectivesFromFile. However, if the glob matches no files, Coraza may silently load no rules rather than returning an error, which could lead to a WAF running with incomplete protection.Consider documenting this behavior or logging a warning when glob patterns are used.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/coraza/coraza.go` around lines 567 - 584, The function validateDirectivesFile currently returns early when path contains "*" which skips validation; update validateDirectivesFile to emit a clear warning whenever a glob pattern is used (e.g., "directives path contains wildcard, Coraza will resolve files at runtime; no files matched could leave WAF unprotected") so operators are notified — use the package's logger (the same logging facility used elsewhere in this package) to log at warning level and keep the early return behavior; reference the relationship to Coraza's WithDirectivesFromFile in the warning text to make intent clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@v3/coraza/coraza_test.go`:
- Around line 254-258: The test currently ignores errors returned by io.ReadAll
when reading firstResp.Body and secondResp.Body; update the reads to capture and
assert no error (e.g., err := io.ReadAll(...); require.NoError(t, err) or if err
!= nil { t.Fatalf(...) }) for both firstBody and secondBody so failures are
reported clearly; apply this change where io.ReadAll is called for
firstResp.Body and secondResp.Body in the test that uses performRequest, and
ensure you close bodies before/after asserting errors as needed.
In `@v3/coraza/coraza.go`:
- Around line 567-584: The function validateDirectivesFile currently returns
early when path contains "*" which skips validation; update
validateDirectivesFile to emit a clear warning whenever a glob pattern is used
(e.g., "directives path contains wildcard, Coraza will resolve files at runtime;
no files matched could leave WAF unprotected") so operators are notified — use
the package's logger (the same logging facility used elsewhere in this package)
to log at warning level and keep the early return behavior; reference the
relationship to Coraza's WithDirectivesFromFile in the warning text to make
intent clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 28e16fed-78cf-47a5-867d-350d95657e15
📒 Files selected for processing (3)
v3/coraza/README.mdv3/coraza/coraza.gov3/coraza/coraza_test.go
📜 Review details
🧰 Additional context used
🪛 LanguageTool
v3/coraza/README.md
[grammar] ~5-~5: Ensure spelling is correct
Context: --- id: coraza --- # Coraza
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (7)
v3/coraza/README.md (1)
1-125: LGTM!The documentation is comprehensive, well-structured, and accurately reflects the middleware implementation. The config table, usage examples, and behavioral notes provide clear guidance for users.
v3/coraza/coraza_test.go (1)
1-551: Test suite provides comprehensive coverage.The test suite thoroughly exercises the middleware's functionality including:
- Initialization paths (with/without config, local files, RootFS)
- Request blocking (headers, body inspection)
- Custom handlers (Next, BlockHandler, ErrorHandler)
- Metrics tracking
- Lifecycle management (Reload, Snapshot, Report)
- Panic recovery with fail-closed behavior
- Instance isolation
The use of
t.Helper(), proper cleanup withdefer, and the fake WAF implementation for panic testing demonstrate good testing practices.v3/coraza/coraza.go (5)
179-184: LGTM - Past review concerns addressed.The
resolveBlockMessage()function correctly handles empty block message inputs by returningdefaultBlockMessage, which addresses the past review concern about allowing callers to restore the default block message.
386-392: LGTM - Race condition fix verified.The
reloadCountvalue is now captured while holding the lock (line 388) and used for logging after the lock is released (line 392). This correctly addresses the past review concern about the data race one.reloadCount.
253-257: LGTM - Logging does not expose attacker-controlled data.The interruption logging only includes structured identifiers (
rule_id,action,status) rather than raw request data likerule.URI()orrule.ErrorLog(). This addresses the past review concern about matched-rule logging potentially exposing attacker-controlled data.
1-650: Overall implementation is well-designed and addresses past review feedback.The middleware implementation demonstrates:
- Proper mutex usage for thread-safe access to engine state
- Fail-closed behavior with panic recovery
- Clean separation between simple (
New) and advanced (NewEngine) APIs- Graceful degradation when reinitialization fails
- Comprehensive observability via metrics and snapshots
All previously raised concerns from past reviews have been addressed.
501-516: This concern is not applicable—the request body handling is intentional and correct. Thereqat line 292 is created viac.Request.Clone(c.Context()), which produces an independent copy of the request. Whentx.ReadRequestBodyFrom(bodyReader)consumes the body at line 506, it only affects the cloned request, not the original Fiber context's request. Downstream handlers receive the original contextc(viac.Next()at line 316), soc.Body()continues to work correctly and is unaffected by WAF processing.
|
Updated in the latest commit. Changes made:
Also updated the README and tests accordingly. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
v3/coraza/coraza_test.go (1)
252-257: Handle response-body read errors instead of discarding them.
io.ReadAllerrors are ignored at Line 252, Line 256, Line 309, Line 335, Line 397, and Line 423. In tests, this can hide real failures.♻️ Minimal fix pattern
- firstBody, _ := io.ReadAll(firstResp.Body) + firstBody, err := io.ReadAll(firstResp.Body) + if err != nil { + t.Fatalf("failed to read response body: %v", err) + }Also applies to: 309-309, 335-335, 397-397, 423-423
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/coraza/coraza_test.go` around lines 252 - 257, The test currently ignores errors returned by io.ReadAll when reading response bodies (e.g., the calls assigning firstBody, secondBody in coraza_test.go after performRequest), which can hide failures; update each io.ReadAll usage in this file to check the returned error and fail the test on error (use t.Fatalf or t.Fatalf-like assertion) before using the body, and ensure bodies are still closed with defer resp.Body.Close() where applicable; search for io.ReadAll occurrences around variables firstBody, secondBody and any other similar reads (lines noted in the review) and add proper error handling for each.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@v3/coraza/coraza_test.go`:
- Around line 252-257: The test currently ignores errors returned by io.ReadAll
when reading response bodies (e.g., the calls assigning firstBody, secondBody in
coraza_test.go after performRequest), which can hide failures; update each
io.ReadAll usage in this file to check the returned error and fail the test on
error (use t.Fatalf or t.Fatalf-like assertion) before using the body, and
ensure bodies are still closed with defer resp.Body.Close() where applicable;
search for io.ReadAll occurrences around variables firstBody, secondBody and any
other similar reads (lines noted in the review) and add proper error handling
for each.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f8f1772-d0bb-4afe-a0f6-b058c9cc441e
📒 Files selected for processing (4)
v3/coraza/README.mdv3/coraza/coraza.gov3/coraza/coraza_test.gov3/coraza/metrics.go
🚧 Files skipped from review as they are similar to previous changes (1)
- v3/coraza/coraza.go
📜 Review details
🧰 Additional context used
🪛 LanguageTool
v3/coraza/README.md
[grammar] ~5-~5: Ensure spelling is correct
Context: --- id: coraza --- # Coraza
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (3)
v3/coraza/metrics.go (1)
67-158: Thread-safe metrics/reporting implementation looks solid.The collector synchronization model and the
Enginesnapshot/report methods are clean and consistent with the middleware observability behavior.v3/coraza/README.md (1)
26-121: API and behavior documentation is aligned with implementation.The documented signatures/config fields and lifecycle/observability notes match the code paths in
v3/coraza/coraza.goandv3/coraza/metrics.go.v3/coraza/coraza_test.go (1)
24-457: Great coverage across middleware behavior, lifecycle, and observability paths.The suite exercises key happy-path and failure-path behavior, including panic recovery and snapshot/report semantics.
There was a problem hiding this comment.
Pull request overview
Adds a new Fiber v3 middleware module under v3/coraza that integrates the Coraza WAF engine, including engine lifecycle management (init/reload), request inspection, and lightweight observability (metrics + snapshots), along with CI and documentation wiring consistent with other contrib/v3 middlewares.
Changes:
- Introduces
coraza.EnginewithNew(...)middleware entrypoint,NewEngine(...),Engine.Init(...), andEngine.Reload(). - Adds in-memory request metrics + engine lifecycle snapshots (
MetricsSnapshot,EngineSnapshot,Report()). - Adds a dedicated test workflow, module docs, and updates workspace/index to include the new module.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| v3/coraza/coraza.go | Core middleware + engine lifecycle/inspection logic. |
| v3/coraza/metrics.go | Default metrics collector + snapshot/report DTOs. |
| v3/coraza/coraza_test.go | End-to-end tests for init, blocking, body inspection, reload/snapshots, and error paths. |
| v3/coraza/README.md | Module documentation and usage examples. |
| v3/coraza/go.mod | New module definition and dependencies. |
| v3/coraza/go.sum | Dependency checksums for the new module. |
| .github/workflows/test-coraza.yml | CI workflow to run tests for the new module. |
| v3/README.md | Adds coraza to the v3 middleware index list. |
| go.work | Adds ./v3/coraza to the workspace. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
v3/coraza/coraza.go
Outdated
| func newEngine(collector MetricsCollector) *Engine { | ||
| if collector == nil { | ||
| collector = NewDefaultMetricsCollector() | ||
| } | ||
|
|
There was a problem hiding this comment.
newEngine only checks collector == nil, which doesn’t catch a typed-nil MetricsCollector (e.g. (*defaultMetricsCollector)(nil) stored in the interface). In that case Engine.Middleware() will panic on metrics.RecordRequest() and Engine.MetricsSnapshot() can panic on collector.GetMetrics(). Consider treating typed-nil collectors as nil (similar to how other middleware handles typed-nil interfaces) and falling back to NewDefaultMetricsCollector() (or defensively nil-check in callers).
| type defaultMetricsCollector struct { | ||
| totalRequests uint64 | ||
| blockedRequests uint64 | ||
|
|
There was a problem hiding this comment.
defaultMetricsCollector uses atomic.AddUint64/LoadUint64 on uint64 struct fields. On 32-bit architectures this can trigger sync/atomic: misaligned 64-bit atomic operation unless the fields are 64-bit aligned. Using atomic.Uint64 (or otherwise ensuring 64-bit alignment) for these counters would avoid that portability/concurrency hazard.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
v3/coraza/coraza_test.go (1)
478-498: Add reload coverage for no-directive engine mode.Current reload tests only cover directive-backed configs. Add a case where the engine is initialized without
DirectivesFileandReload()is expected to succeed, so default mode doesn’t regress.🧪 Suggested test
+func TestReloadWithoutDirectivesSucceeds(t *testing.T) { + engine, err := NewEngine(Config{ + RequestBodyAccess: true, + }) + if err != nil { + t.Fatalf("failed to initialize engine without directives: %v", err) + } + + if err := engine.Reload(); err != nil { + t.Fatalf("expected reload without directives to succeed, got: %v", err) + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/coraza/coraza_test.go` around lines 478 - 498, Add a test case that initializes the engine in "no-directive" mode (i.e., create the engine without setting DirectivesFile or any directive-backed config), call engine.Reload(), then take engine.Snapshot() and assert ReloadSuccessTotal, InitSuccessTotal and ReloadCount are incremented as in TestEngineSnapshotTracksLifecycleCounters; use the same methods engine.Reload() and engine.Snapshot() and the same snapshot fields ReloadSuccessTotal, InitSuccessTotal, ReloadCount to validate the behavior so the default/no-directive reload path is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v3/coraza/coraza.go`:
- Around line 364-366: The Reload() method incorrectly returns an error when
cfg.DirectivesFile is empty even though no-directive mode is valid; remove the
early return that checks if len(cfg.DirectivesFile) == 0 so Reload() will
proceed in no-directive configurations (i.e., delete or bypass the conditional
that returns fmt.Errorf("no configuration available for reload")). Ensure
Reload() still validates other necessary reload sources if any, but do not treat
an empty cfg.DirectivesFile as an error condition.
- Around line 580-588: In validateDirectivesFile, when path contains a wildcard
currently you only warn and return nil; instead resolve the wildcard against the
provided root fs.FS (e.g., use fs.Glob or walk root and match the pattern) to
ensure at least one file matches, and if no matches are found return a non-nil
error (and keep the warning log) so a mistyped wildcard doesn't silently open
the WAF; reference validateDirectivesFile, root (fs.FS) and path to locate where
to implement the glob/match check and error return.
In `@v3/coraza/metrics.go`:
- Around line 89-95: RecordLatency may silently overflow m.latencySum (type
time.Duration) when adding duration; update
defaultMetricsCollector.RecordLatency to detect saturation before adding (for
example compute newSum := m.latencySum + duration and if newSum < m.latencySum
or newSum < 0 treat as overflow) and clamp m.latencySum to a safe maximum (e.g.,
time.Duration(math.MaxInt64) or a defined MaxLatency) instead of wrapping; keep
the latencyCount increment but ensure the guard uses the same mutex and
references the method name RecordLatency and fields latencySum and latencyCount
so callers and metrics consumers are not corrupted by wrapped durations.
---
Nitpick comments:
In `@v3/coraza/coraza_test.go`:
- Around line 478-498: Add a test case that initializes the engine in
"no-directive" mode (i.e., create the engine without setting DirectivesFile or
any directive-backed config), call engine.Reload(), then take engine.Snapshot()
and assert ReloadSuccessTotal, InitSuccessTotal and ReloadCount are incremented
as in TestEngineSnapshotTracksLifecycleCounters; use the same methods
engine.Reload() and engine.Snapshot() and the same snapshot fields
ReloadSuccessTotal, InitSuccessTotal, ReloadCount to validate the behavior so
the default/no-directive reload path is covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3532a14-46c0-4a4e-a17b-c8282dafb376
📒 Files selected for processing (3)
v3/coraza/coraza.gov3/coraza/coraza_test.gov3/coraza/metrics.go
|
Added |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func New(config ...Config) fiber.Handler { | ||
| cfg := ConfigDefault | ||
| if len(config) > 0 { | ||
| cfg = config[0] | ||
| } | ||
|
|
There was a problem hiding this comment.
New(config ...Config) currently discards ConfigDefault whenever a config is provided (cfg = config[0]). This means omitted fields fall back to their zero values (e.g., RequestBodyAccess becomes false, and LogLevel can become LevelTrace), which contradicts the documented defaults and can unintentionally disable request body inspection.
| // NewEngine creates and initializes an Engine with the provided configuration. | ||
| func NewEngine(cfg Config) (*Engine, error) { | ||
| engine := newEngine(cfg.MetricsCollector) | ||
| if err := engine.Init(cfg); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return engine, nil | ||
| } |
There was a problem hiding this comment.
MetricsCollector is part of Config, but it is only applied once in NewEngine via newEngine(cfg.MetricsCollector). Subsequent calls to Engine.Init(cfg) ignore cfg.MetricsCollector, so reinitializing an engine cannot change the collector even though the API suggests it can. Consider either applying cfg.MetricsCollector inside Init (and updating stored config accordingly) or documenting/removing it from the Init-level configuration to avoid misleading behavior.
v3/coraza/coraza.go
Outdated
| fiberlog.Warnw( | ||
| "Coraza directives path uses glob matching and is expanded before initialization", | ||
| "path", path, | ||
| "note", "all matching directives files will be loaded in sorted order", | ||
| ) | ||
|
|
There was a problem hiding this comment.
resolveDirectivesFiles logs glob-related warnings using fiberlog.Warnw(...) directly, which bypasses the middleware/engine LogLevel configuration used elsewhere (via logWithLevel). If LogLevel is intended to control lifecycle logging, consider routing this warning through the same log-level filtering (e.g., pass the configured level into resolveDirectivesFiles or log from the caller).
| fiberlog.Warnw( | |
| "Coraza directives path uses glob matching and is expanded before initialization", | |
| "path", path, | |
| "note", "all matching directives files will be loaded in sorted order", | |
| ) |
v3/coraza/README.md
Outdated
| If you want the defaults, start from `coraza.ConfigDefault` and override the fields you need. | ||
| By default, the middleware starts without external rule files. Set `DirectivesFile` to load your Coraza or CRS ruleset. | ||
| Request body size follows the Fiber app `BodyLimit`. | ||
| Wildcard entries in `DirectivesFile` are resolved by Coraza at runtime. If a wildcard matches no files, the WAF may start without the expected rules loaded. |
There was a problem hiding this comment.
The README states that wildcard entries in DirectivesFile are resolved by Coraza at runtime and may start with no rules if there are no matches, but the implementation expands globs before initialization and returns an error when no files match. Please update this section to reflect the actual behavior (pre-expansion + fail on zero matches), or adjust the code to match the documented behavior.
| Wildcard entries in `DirectivesFile` are resolved by Coraza at runtime. If a wildcard matches no files, the WAF may start without the expected rules loaded. | |
| Wildcard entries in `DirectivesFile` are expanded before Coraza initializes. If a wildcard matches no files, initialization fails with an error and the middleware does not start. |
This PR adds a new
corazamiddleware module for Fiber v3 underv3/coraza.It integrates the Coraza WAF engine as a Fiber middleware and is designed to stay aligned with the existing
gofiber/contribmiddleware style:coraza.New(config ...Config) fiber.Handlercoraza.NewEngine(Config)Features
Engine.Reload()Notes
coraza.New()starts without external rule files by default; rules are loaded whenDirectivesFileis configuredcoraza.New(...)to panic, which matches fail-fast startup expectations for middleware initializationValidation
Tested with: