feat(api): return 1 on disruption, add bulk headers, add coraza_free_string#94
feat(api): return 1 on disruption, add bulk headers, add coraza_free_string#94fzipi merged 9 commits intocorazawaf:mainfrom
Conversation
|
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:
📝 WalkthroughWalkthroughStandardizes C API result codes (error/ok/interruption), changes processing functions to return them, adds bulk header-import functions that decode length-prefixed packed buffers with bounds checks, and exposes a string-free helper. Example tests updated to align with new return semantics. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
🧹 Nitpick comments (1)
libcoraza/coraza.go (1)
357-389: Duplicate decode loop — extract a shared helper.
coraza_add_request_headersandcoraza_add_response_headersdiffer only in theAddRequestHeadervsAddResponseHeadercall. Extract the decode/iteration into a small helper that yields(name, value)(or takes a callback), so any fix (e.g., theuint32overflow above) only needs to be made once.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libcoraza/coraza.go` around lines 357 - 389, Both coraza_add_request_headers and coraza_add_response_headers share the same decode loop; extract that logic into a single helper (e.g., iteratePackedHeaders or decodePackedHeaders) that accepts the packed buffer and count and either returns a slice of (name,value) pairs or accepts a callback func(name, value string). Replace the duplicate loops in coraza_add_request_headers and coraza_add_response_headers with calls to this helper and call tx.AddRequestHeader / tx.AddResponseHeader inside the callback; ensure the helper centralizes all bounds checks and uses safe unsigned-to-int conversions (avoid uint32 overflow by validating lengths before shifting/combining) so future fixes apply once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libcoraza/coraza.go`:
- Around line 288-290: Add defensive validation at the top of
coraza_add_request_headers and coraza_add_response_headers: check if packed_len
< 0 or count < 0 and immediately return C.int(-1); additionally if packed == nil
&& packed_len > 0 return C.int(-1) to avoid dereferencing a null pointer; only
call C.GoBytes(unsafe.Pointer(packed), packed_len) after these validations pass
so you never pass a negative length or a nil pointer to C.GoBytes.
- Around line 288-316: The value length decoding in coraza_add_request_headers
(and similarly in coraza_add_response_headers) uses int shifts which can
overflow on 32-bit platforms; change the decode to read valueLen as a uint32
(e.g. valueLenU := uint32(buf[off])<<24 | uint32(buf[off+1])<<16 |
uint32(buf[off+2])<<8 | uint32(buf[off+3])), perform bounds checks using that
uint32 (ensure off and nameLen are converted and compared safely), verify off +
int(valueLenU) does not exceed len(buf) and only then convert valueLenU to int
for slicing; update all corresponding checks and conversions in both
coraza_add_request_headers and coraza_add_response_headers to avoid signed
overflow panics.
- Around line 500-507: Add extern declarations for the three new exported
functions so SWIG knows about them: add extern prototypes for
coraza_add_request_headers, coraza_add_response_headers, and coraza_free_string
to the coraza.i function declarations section (matching the //export lines in
coraza.go). After adding them, run the sync check (make check-swig-sync) to
verify. Optional: in the coraza_free_string implementation consider guarding
against nil (mirror the internal freeString helper) before calling C.free to be
consistent.
---
Nitpick comments:
In `@libcoraza/coraza.go`:
- Around line 357-389: Both coraza_add_request_headers and
coraza_add_response_headers share the same decode loop; extract that logic into
a single helper (e.g., iteratePackedHeaders or decodePackedHeaders) that accepts
the packed buffer and count and either returns a slice of (name,value) pairs or
accepts a callback func(name, value string). Replace the duplicate loops in
coraza_add_request_headers and coraza_add_response_headers with calls to this
helper and call tx.AddRequestHeader / tx.AddResponseHeader inside the callback;
ensure the helper centralizes all bounds checks and uses safe unsigned-to-int
conversions (avoid uint32 overflow by validating lengths before
shifting/combining) so future fixes apply once.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
Tested locally with the full CRS 4.x regression suite using the coraza_httpd server from PR #92: all 4721 tests pass (920 excluded, same as caddy). |
|
Can we use an |
|
Done — added |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
libcoraza/coraza.go (1)
294-321:⚠️ Potential issue | 🟠 MajorFinish hardening the packed-buffer bounds checks.
Two crash paths remain:
packed == nil && packed_len > 0still reachesC.GoBytes, andint(uint32(...))can wrap negative on 32-bit builds before the bounds check. This is the remaining unresolved part of the earlier bulk-header validation finding.Proposed fix for both parsers
func coraza_add_request_headers(t C.coraza_transaction_t, packed *C.char, packed_len C.int, count C.int) C.int { if packed_len < 0 || count < 0 { return -1 } + if packed == nil && packed_len > 0 { + return -1 + } tx := fromRaw[types.Transaction](t) buf := C.GoBytes(unsafe.Pointer(packed), packed_len) @@ - valueLen := int(uint32(buf[off])<<24 | uint32(buf[off+1])<<16 | uint32(buf[off+2])<<8 | uint32(buf[off+3])) + valueLenU := uint32(buf[off])<<24 | uint32(buf[off+1])<<16 | uint32(buf[off+2])<<8 | uint32(buf[off+3]) off += 4 - if off+valueLen > len(buf) { + if uint64(valueLenU) > uint64(len(buf)-off) { return -1 } + valueLen := int(valueLenU) value := string(buf[off : off+valueLen]) @@ func coraza_add_response_headers(t C.coraza_transaction_t, packed *C.char, packed_len C.int, count C.int) C.int { if packed_len < 0 || count < 0 { return -1 } + if packed == nil && packed_len > 0 { + return -1 + } tx := fromRaw[types.Transaction](t) buf := C.GoBytes(unsafe.Pointer(packed), packed_len) @@ - valueLen := int(uint32(buf[off])<<24 | uint32(buf[off+1])<<16 | uint32(buf[off+2])<<8 | uint32(buf[off+3])) + valueLenU := uint32(buf[off])<<24 | uint32(buf[off+1])<<16 | uint32(buf[off+2])<<8 | uint32(buf[off+3]) off += 4 - if off+valueLen > len(buf) { + if uint64(valueLenU) > uint64(len(buf)-off) { return -1 } + valueLen := int(valueLenU) value := string(buf[off : off+valueLen])Verify both parsers have the nil guard and avoid converting
uint32lengths tointbefore range checks:#!/bin/bash # Description: Inspect the bulk header parsers for nil guards and safe uint32 length checks. set -euo pipefail rg -n -C4 'func coraza_add_(request|response)_headers|packed == nil|C\.GoBytes\(unsafe\.Pointer\(packed\), packed_len\)|valueLenU|int\(uint32\(buf\[off\]\)<<24' libcoraza/coraza.goAlso applies to: 370-397
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libcoraza/coraza.go` around lines 294 - 321, The request parser coraza_add_request_headers (and the sibling coraza_add_response_headers) must add a nil check for packed before calling C.GoBytes and must stop converting big-endian uint32 length fields to int before validating bounds; read the 4-byte length into a uint32 (e.g. valueLenU), check that off+int(valueLenU) (or compare remaining bytes as uint32) is within buf length, then convert to int for slicing; likewise handle the 2-byte nameLen as uint16-first if not already, and ensure the initial guard covers the case packed == nil && packed_len > 0 to return -1 before C.GoBytes is invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@coraza.i`:
- Line 416: The extern deallocator coraza_free_string(char *s) must not be
exposed to SWIG; add a SWIG ignore directive for coraza_free_string so bindings
are not generated for that function (e.g., add "%ignore coraza_free_string;" in
the interface before or above the extern declaration) to prevent managed
languages from calling the raw C deallocator and causing double-free/memory
corruption; ensure no other wrapper or %newobject is applied to
coraza_free_string.
In `@examples/java/SimpleGet.java`:
- Around line 105-107: The current check uses check(ret >= 0, ...) which allows
a returned 0 to pass and masks regressions; instead assert the explicit
interruption return code from coraza_process_request_headers (it returns 1 when
interrupted by a deny rule). Replace the loose check with a precise assertion
that ret == 1 (or otherwise explicitly handle the non-interruption case) for the
coraza.coraza_process_request_headers(tx) call, updating the error message to
mention the unexpected interruption return value; reference the symbols
coraza_process_request_headers and tx when locating the code to change.
In `@examples/python/simple_get.py`:
- Around line 111-113: The current check after calling
coraza_process_request_headers(tx) uses a loose assertion (ret >= 0) which would
miss regressions where the API returns 0 instead of the expected interruption
code; change the assertion to explicitly verify the expected interruption return
(use ret == 1) and update the error message in the _check call to include both
the function name coraza_process_request_headers, the tx identifier, and the
actual returned value to aid debugging.
---
Duplicate comments:
In `@libcoraza/coraza.go`:
- Around line 294-321: The request parser coraza_add_request_headers (and the
sibling coraza_add_response_headers) must add a nil check for packed before
calling C.GoBytes and must stop converting big-endian uint32 length fields to
int before validating bounds; read the 4-byte length into a uint32 (e.g.
valueLenU), check that off+int(valueLenU) (or compare remaining bytes as uint32)
is within buf length, then convert to int for slicing; likewise handle the
2-byte nameLen as uint16-first if not already, and ensure the initial guard
covers the case packed == nil && packed_len > 0 to return -1 before C.GoBytes is
invoked.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66cc7f0f-c22b-4b4f-895d-74242ae859ed
📒 Files selected for processing (4)
coraza.iexamples/java/SimpleGet.javaexamples/python/simple_get.pylibcoraza/coraza.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
examples/python/simple_get.py (2)
201-203: Consistency nit:test_request_body_from_filestill asserts strict== 0.These three phase calls on the pass-rule path legitimately return
CORAZA_OKtoday, but for consistency with the relaxed checks intest_lifecycle(Lines 122/126/142) and to avoid brittle failures if a future non-deny interruption becomes possible on this path, consider loosening to>= 0(or!= CORAZA_ERROR). Not a functional bug.Proposed tightening
- _check(_c.coraza_process_request_body(tx) == 0, "coraza_process_request_body failed") - _check(_c.coraza_process_response_headers(tx, 200, "HTTP/1.1") == 0, "coraza_process_response_headers failed") - _check(_c.coraza_process_response_body(tx) == 0, "coraza_process_response_body failed") + _check(_c.coraza_process_request_body(tx) >= 0, "coraza_process_request_body failed") + _check(_c.coraza_process_response_headers(tx, 200, "HTTP/1.1") >= 0, "coraza_process_response_headers failed") + _check(_c.coraza_process_response_body(tx) >= 0, "coraza_process_response_body failed")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/python/simple_get.py` around lines 201 - 203, The three strict equality checks using _check against 0 for coraza_process_request_body, coraza_process_response_headers, and coraza_process_response_body are brittle; change the assertions to accept any non-error return (e.g., use _check(_c.coraza_process_request_body(tx) >= 0, "...") or _check(_c.coraza_process_request_body(tx) != CORAZA_ERROR, "...")) for all three calls (coraza_process_request_body, coraza_process_response_headers, coraza_process_response_body) so the test matches the relaxed checks used in test_lifecycle and avoids false failures if non-deny non-zero codes are returned.
111-142: Optional: use the exposed enum constants instead of numeric literals.Since
coraza.iexposesCORAZA_ERROR,CORAZA_OK, andCORAZA_INTERRUPTIONvia SWIG, the assertions could reference_c.CORAZA_INTERRUPTIONand_c.CORAZA_OKinstead of hardcoded1and0. This improves readability and makes regressions (e.g., an accidental return of0from a phase expected to interrupt) more self-documenting. The current literals are functionally correct.Proposed refinement
- _check(ret == 1, f"coraza_process_request_headers: expected CORAZA_INTERRUPTION (1), got {ret}") + _check(ret == _c.CORAZA_INTERRUPTION, f"coraza_process_request_headers: expected CORAZA_INTERRUPTION, got {ret}") - _check(ret >= 0, f"coraza_process_request_body failed: {ret}") + _check(ret >= _c.CORAZA_OK, f"coraza_process_request_body failed: {ret}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/python/simple_get.py` around lines 111 - 142, Replace hardcoded numeric return checks with the SWIG-exposed enum constants to improve readability: use _c.CORAZA_INTERRUPTION for the check on coraza_process_request_headers instead of 1, use _c.CORAZA_OK for checks that expect success (coraza_append_request_body, coraza_add_response_header, coraza_append_response_body) instead of 0, and continue to use comparison operators (>= 0) for processing functions (coraza_process_request_body, coraza_process_response_headers, coraza_process_response_body) as before; update the _check assertions accordingly referencing the functions coraza_process_request_headers, coraza_append_request_body, coraza_add_response_header, coraza_append_response_body, and coraza_process_response_body.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@examples/python/simple_get.py`:
- Around line 201-203: The three strict equality checks using _check against 0
for coraza_process_request_body, coraza_process_response_headers, and
coraza_process_response_body are brittle; change the assertions to accept any
non-error return (e.g., use _check(_c.coraza_process_request_body(tx) >= 0,
"...") or _check(_c.coraza_process_request_body(tx) != CORAZA_ERROR, "...")) for
all three calls (coraza_process_request_body, coraza_process_response_headers,
coraza_process_response_body) so the test matches the relaxed checks used in
test_lifecycle and avoids false failures if non-deny non-zero codes are
returned.
- Around line 111-142: Replace hardcoded numeric return checks with the
SWIG-exposed enum constants to improve readability: use _c.CORAZA_INTERRUPTION
for the check on coraza_process_request_headers instead of 1, use _c.CORAZA_OK
for checks that expect success (coraza_append_request_body,
coraza_add_response_header, coraza_append_response_body) instead of 0, and
continue to use comparison operators (>= 0) for processing functions
(coraza_process_request_body, coraza_process_response_headers,
coraza_process_response_body) as before; update the _check assertions
accordingly referencing the functions coraza_process_request_headers,
coraza_append_request_body, coraza_add_response_header,
coraza_append_response_body, and coraza_process_response_body.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ffdd4072-93da-41e4-b4fd-9f9a9f507018
📒 Files selected for processing (3)
coraza.iexamples/java/SimpleGet.javaexamples/python/simple_get.py
🚧 Files skipped from review as they are similar to previous changes (2)
- coraza.i
- examples/java/SimpleGet.java
- Phase functions now return 1 when interrupted, -1 on error, 0 on success - Add coraza_add_request_headers/coraza_add_response_headers for bulk packed headers - Add coraza_free_string for safe deallocation of strings returned by libcoraza Ref corazawaf#93
…_free_string in SWIG
fzipi
force-pushed
the
feat/api-ergonomics
branch
from
c6d9e15 to
ea4fb4c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
libcoraza/coraza.go (1)
290-401: Use the newCORAZA_ERROR/CORAZA_OKconstants and deduplicate the two decoders.Two small follow-ups for the bulk-header functions:
Consistency. The phase functions now return
C.CORAZA_ERROR/C.CORAZA_OK/C.CORAZA_INTERRUPTION, butcoraza_add_request_headersandcoraza_add_response_headersstill hardcode-1and0. Since the whole point of the enum is to give every C-exported function a common vocabulary, swap the literals for the named constants.DRY. The two functions are byte-for-byte identical except for the terminal
tx.AddRequestHeader/tx.AddResponseHeadercall. Extracting a small parser avoids having to fix a decoder bug in two places next time.♻️ Proposed refactor
+// decodePackedHeaders walks the [name_len u16][name][value_len u32][value] × count +// format and invokes addFn for each decoded header. Returns CORAZA_OK on success +// or CORAZA_ERROR on any bounds violation. +func decodePackedHeaders(packed *C.char, packed_len C.int, count C.int, addFn func(name, value string)) C.int { + if packed_len < 0 || count < 0 { + return C.CORAZA_ERROR + } + buf := C.GoBytes(unsafe.Pointer(packed), packed_len) + off := 0 + for i := 0; i < int(count); i++ { + if off+2 > len(buf) { + return C.CORAZA_ERROR + } + nameLen := int(uint16(buf[off])<<8 | uint16(buf[off+1])) + off += 2 + if off+nameLen > len(buf) { + return C.CORAZA_ERROR + } + name := string(buf[off : off+nameLen]) + off += nameLen + if off+4 > len(buf) { + return C.CORAZA_ERROR + } + vl := uint32(buf[off])<<24 | uint32(buf[off+1])<<16 | uint32(buf[off+2])<<8 | uint32(buf[off+3]) + off += 4 + if uint64(vl) > uint64(len(buf)-off) { + return C.CORAZA_ERROR + } + valueLen := int(vl) + value := string(buf[off : off+valueLen]) + off += valueLen + addFn(name, value) + } + return C.CORAZA_OK +} + //export coraza_add_request_headers func coraza_add_request_headers(t C.coraza_transaction_t, packed *C.char, packed_len C.int, count C.int) C.int { - if packed_len < 0 || count < 0 { - return -1 - } tx := fromRaw[types.Transaction](t) - buf := C.GoBytes(unsafe.Pointer(packed), packed_len) - off := 0 - for i := 0; i < int(count); i++ { - ... duplicated decode loop ... - } - return 0 + return decodePackedHeaders(packed, packed_len, count, tx.AddRequestHeader) }(analogous change for
coraza_add_response_headers)Note the refactor also bounds-checks
vlasuint64before converting toint, which closes a narrow 32-bit residual: on 32-bit builds today,int(uint32(...))at lines 315 / 391 wraps any value ≥ 2 GiB to a negativeint, so the subsequentoff+valueLen > len(buf)guard passes spuriously and the next slice expression panics. On 64-bit builds this is harmless becauseintis 64 bits, but doing the range check before the conversion makes the code portable and obviously correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libcoraza/coraza.go` around lines 290 - 401, Replace the literal return codes and deduplicate the duplicate parsers: change coraza_add_request_headers and coraza_add_response_headers to return C.CORAZA_ERROR / C.CORAZA_OK (and C.CORAZA_INTERRUPTION where appropriate) instead of -1/0, and extract the shared byte-decoding logic into a helper (e.g., parsePackedHeaders) that accepts the packed buffer, packed_len, count and a callback to invoke either tx.AddRequestHeader or tx.AddResponseHeader; in the helper do robust bounds checks including validating valueLen as an unsigned width (check uint64 valueLen <= uint64(len(buf)) before converting to int) to avoid 32-bit int wraparound, and call that helper from coraza_add_request_headers and coraza_add_response_headers.coraza.i (1)
386-400: Consider returningcoraza_result_tfor the phase functions in the extern declarations.The phase functions
coraza_process_request_headers,coraza_process_request_body,coraza_process_response_headers, andcoraza_process_response_bodyshould be declared as returningcoraza_result_tinstead ofint. This improves SWIG-generated bindings: Java code gets typed enum constants to compare against, and Python users see symbolic constant names in context. The values remain ABI-compatible withint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coraza.i` around lines 386 - 400, Change the return type of the phase functions from int to coraza_result_t to improve bindings: update the extern declarations for coraza_process_request_headers, coraza_process_request_body, coraza_process_response_headers, and coraza_process_response_body to return coraza_result_t instead of int (leave argument lists unchanged); ensure you update every matching forward declaration/prototype for these function names so SWIG/Python/Java see the enum-typed result while preserving ABI compatibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@coraza.i`:
- Around line 386-400: Change the return type of the phase functions from int to
coraza_result_t to improve bindings: update the extern declarations for
coraza_process_request_headers, coraza_process_request_body,
coraza_process_response_headers, and coraza_process_response_body to return
coraza_result_t instead of int (leave argument lists unchanged); ensure you
update every matching forward declaration/prototype for these function names so
SWIG/Python/Java see the enum-typed result while preserving ABI compatibility.
In `@libcoraza/coraza.go`:
- Around line 290-401: Replace the literal return codes and deduplicate the
duplicate parsers: change coraza_add_request_headers and
coraza_add_response_headers to return C.CORAZA_ERROR / C.CORAZA_OK (and
C.CORAZA_INTERRUPTION where appropriate) instead of -1/0, and extract the shared
byte-decoding logic into a helper (e.g., parsePackedHeaders) that accepts the
packed buffer, packed_len, count and a callback to invoke either
tx.AddRequestHeader or tx.AddResponseHeader; in the helper do robust bounds
checks including validating valueLen as an unsigned width (check uint64 valueLen
<= uint64(len(buf)) before converting to int) to avoid 32-bit int wraparound,
and call that helper from coraza_add_request_headers and
coraza_add_response_headers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17d11efb-fbbb-4d4b-a1bd-a5296b213a70
📒 Files selected for processing (4)
coraza.iexamples/java/SimpleGet.javaexamples/python/simple_get.pylibcoraza/coraza.go
✅ Files skipped from review due to trivial changes (2)
- examples/java/SimpleGet.java
- examples/python/simple_get.py
|
I like both nitpicks from the rabbit 😄 |
…tions in examples
There was a problem hiding this comment.
♻️ Duplicate comments (1)
libcoraza/coraza.go (1)
315-321:⚠️ Potential issue | 🟠 MajorSigned-overflow in
valueLendecode is still reachable on 32-bit builds.The previous fix moved the decode to
uint32but then narrows tointbefore the bounds check:valueLen := int(uint32(buf[off])<<24 | uint32(buf[off+1])<<16 | uint32(buf[off+2])<<8 | uint32(buf[off+3])) off += 4 if off+valueLen > len(buf) { ... }On platforms where
intis 32-bit (e.g.GOARCH=386,arm,mips), any value with the high bit set (≥ 2 GiB, legal for au32) becomes a negativeintduring theint(...)conversion. The checkoff+valueLen > len(buf)then passes with a negative sum, andbuf[off : off+valueLen]panics — a crafted packed buffer can crash the host process.coraza_add_response_headershas the same issue at Line 391.Keep the length as
uint32and range-check against the remaining buffer space before converting toint:🛡️ Proposed fix
- if off+4 > len(buf) { - return C.CORAZA_ERROR - } - valueLen := int(uint32(buf[off])<<24 | uint32(buf[off+1])<<16 | uint32(buf[off+2])<<8 | uint32(buf[off+3])) - off += 4 - if off+valueLen > len(buf) { - return C.CORAZA_ERROR - } - value := string(buf[off : off+valueLen]) - off += valueLen + if off+4 > len(buf) { + return C.CORAZA_ERROR + } + vl := uint32(buf[off])<<24 | uint32(buf[off+1])<<16 | uint32(buf[off+2])<<8 | uint32(buf[off+3]) + off += 4 + if uint64(vl) > uint64(len(buf)-off) { + return C.CORAZA_ERROR + } + valueLen := int(vl) + value := string(buf[off : off+valueLen]) + off += valueLenApply the equivalent change in
coraza_add_response_headers.Also applies to: 391-397
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libcoraza/coraza.go` around lines 315 - 321, The current decode converts the 4-byte big-endian length into int, allowing negative values on 32-bit platforms; instead decode into a uint32 (e.g., valueLen uint32 = uint32(buf[off])<<24 | ...), advance off by 4, then check that uint64(off)+uint64(valueLen) <= uint64(len(buf)) (or compare valueLen against remaining uint32(len(buf)-off)) to prevent overflow before converting; only after the bounds check convert valueLen to int for slicing and use it to build value := string(buf[off:off+int(valueLen)]). Apply the identical change in coraza_add_response_headers for its length decode and bounds check.
🧹 Nitpick comments (2)
libcoraza/coraza.go (2)
327-334: Header-processing functions: note the missingCORAZA_ERRORpath.These phase functions can only return
CORAZA_OKorCORAZA_INTERRUPTIONbecausetx.ProcessRequestHeaders/tx.ProcessResponseHeadersdo not return an error. That matches the current Coraza v3 API and aligns with the test expectations (ret >= 0is acceptable), so no change needed — worth calling out in the doc comment for the public header so callers understand that-1is not a possible result from these two entry points but is possible from the body-processing ones.Also applies to: 425-432
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libcoraza/coraza.go` around lines 327 - 334, The header-processing exported functions (coraza_process_request_headers and coraza_process_response_headers) currently only return CORAZA_OK or CORAZA_INTERRUPTION because tx.ProcessRequestHeaders / tx.ProcessResponseHeaders do not return errors; update the public doc comment above these exported functions to explicitly state that CORAZA_ERROR (−1) is not possible for these entry points whereas body-processing functions can return CORAZA_ERROR, so callers and tests know that valid return values are only CORAZA_OK or CORAZA_INTERRUPTION for these two functions.
294-324: Consider pre-computing capacity when adding headers.The loop calls
tx.AddRequestHeaderper iteration without any pre-allocation hint, butcountis known up-front. Iftypes.Transactionexposes a bulk/append-with-capacity variant (or if the underlying implementation grows a map/slice), passingcountonce would avoid repeated reallocations on large header sets. If no such API exists, this is safe to ignore — keeping as a non-blocking note.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libcoraza/coraza.go` around lines 294 - 324, The loop in coraza_add_request_headers calls tx.AddRequestHeader repeatedly while count is known up-front; to avoid repeated reallocations, pre-allocate capacity before the loop by either calling a capacity-reserving method on types.Transaction (e.g., tx.ReserveRequestHeaders(int(count)) if available) or by collecting headers into a slice with pre-allocated capacity (make([]Header, 0, int(count)) or similar) and then calling a bulk add method (e.g., tx.AddRequestHeaders(headers)) after the loop; update code references coraza_add_request_headers, types.Transaction, and tx.AddRequestHeader accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@libcoraza/coraza.go`:
- Around line 315-321: The current decode converts the 4-byte big-endian length
into int, allowing negative values on 32-bit platforms; instead decode into a
uint32 (e.g., valueLen uint32 = uint32(buf[off])<<24 | ...), advance off by 4,
then check that uint64(off)+uint64(valueLen) <= uint64(len(buf)) (or compare
valueLen against remaining uint32(len(buf)-off)) to prevent overflow before
converting; only after the bounds check convert valueLen to int for slicing and
use it to build value := string(buf[off:off+int(valueLen)]). Apply the identical
change in coraza_add_response_headers for its length decode and bounds check.
---
Nitpick comments:
In `@libcoraza/coraza.go`:
- Around line 327-334: The header-processing exported functions
(coraza_process_request_headers and coraza_process_response_headers) currently
only return CORAZA_OK or CORAZA_INTERRUPTION because tx.ProcessRequestHeaders /
tx.ProcessResponseHeaders do not return errors; update the public doc comment
above these exported functions to explicitly state that CORAZA_ERROR (−1) is not
possible for these entry points whereas body-processing functions can return
CORAZA_ERROR, so callers and tests know that valid return values are only
CORAZA_OK or CORAZA_INTERRUPTION for these two functions.
- Around line 294-324: The loop in coraza_add_request_headers calls
tx.AddRequestHeader repeatedly while count is known up-front; to avoid repeated
reallocations, pre-allocate capacity before the loop by either calling a
capacity-reserving method on types.Transaction (e.g.,
tx.ReserveRequestHeaders(int(count)) if available) or by collecting headers into
a slice with pre-allocated capacity (make([]Header, 0, int(count)) or similar)
and then calling a bulk add method (e.g., tx.AddRequestHeaders(headers)) after
the loop; update code references coraza_add_request_headers, types.Transaction,
and tx.AddRequestHeader accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19d63a5c-ccd4-42e9-bee3-e97159993010
📒 Files selected for processing (2)
examples/python/simple_get.pylibcoraza/coraza.go
|
Done — bulk header functions now use |
|
Fixed the 32-bit overflow: |
fzipi
changed the title
2 participants
Summary
Implements the three API improvements from #93:
Interruption signaling —
coraza_process_request_headers,coraza_process_request_body,coraza_process_response_headers,coraza_process_response_bodynow return 1 when interrupted, -1 on error, 0 on success. Callers can skip subsequent phases without pollingcoraza_intervention()on every phase.Bulk header API —
coraza_add_request_headers()andcoraza_add_response_headers()accept a packed buffer ([name_len u16][name_bytes][value_len u32][value_bytes] × count) for single-call header injection.coraza_free_string()— safe deallocation for strings returned by libcoraza (e.g.coraza_matched_rule_get_error_log), avoiding allocator mismatch on Windows.Ref #93
cc @jptosso @fzipi
Summary by CodeRabbit
New Features
Improvements
Tests