From ee0add764e48928c47bb91ecc66627890bb44cf2 Mon Sep 17 00:00:00 2001 From: Artem Murashkin Date: Thu, 23 Jul 2026 11:58:17 +0200 Subject: [PATCH 1/2] fix: repair fork-only tests failing under TinyGo and -race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fork-only tests (both from cloudlinux/coraza 7e054cf) fail in CI but pass under plain `go test`, so the sync merge did not surface them. initcol_test.go: gate with `//go:build !tinygo`. The test lives in package actions_test and uses the `md` mock, which is only defined in setvar_persistence_test.go — a file already gated `!tinygo`. Under TinyGo that file drops out and `md` is undefined, breaking compilation. The test exercises the SESSION persistent collection, the same feature family the fork already excludes from TinyGo, so gating it matches existing intent. expirevar_test.go: give each parallel subtest its own action instance. The test got one expirevarFn via Get("expirevar") and shared it across eight t.Parallel() subtests, each calling Init(), which writes the receiver's collection/key/ttl fields — a data race caught by `go test -race`. It is a test-only defect: in production each rule gets its own action, Init runs once at compile time, and Evaluate only reads. Also renamed the three duplicate "missing ttl" subtests for readable output. Verified: `go vet -tags tinygo ./internal/...` clean; `go test -race` on internal/actions clean under both CI tag sets. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/actions/expirevar_test.go | 23 +++++++++++------------ internal/actions/initcol_test.go | 2 ++ 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/internal/actions/expirevar_test.go b/internal/actions/expirevar_test.go index ac72fff36..0d156f64c 100644 --- a/internal/actions/expirevar_test.go +++ b/internal/actions/expirevar_test.go @@ -11,14 +11,13 @@ import ( func TestExpirevar(t *testing.T) { t.Parallel() - a, err := Get("expirevar") - if err != nil { - t.Error("failed to get setvar action") + if _, err := Get("expirevar"); err != nil { + t.Error("failed to get expirevar action") } t.Run("no arguments", func(t *testing.T) { t.Parallel() - err := a.Init(nil, "") + err := expirevar().Init(nil, "") if !errors.Is(err, ErrMissingArguments) { t.Errorf("expected error ErrMissingArguments, got %v", err) } @@ -26,7 +25,7 @@ func TestExpirevar(t *testing.T) { t.Run("invalid collection", func(t *testing.T) { t.Parallel() - err := a.Init(nil, "INVALID.test=60") + err := expirevar().Init(nil, "INVALID.test=60") if !strings.Contains(err.Error(), "invalid collection, supported collections are: ") { t.Errorf("expected error 'invalid collection...', got %v", err) } @@ -34,7 +33,7 @@ func TestExpirevar(t *testing.T) { t.Run("missing variable name", func(t *testing.T) { t.Parallel() - err := a.Init(nil, "IP.=60") + err := expirevar().Init(nil, "IP.=60") if !errors.Is(err, ErrInvalidKVArguments) { t.Errorf("expected error ErrInvalidKVArguments, got %v", err) } @@ -42,23 +41,23 @@ func TestExpirevar(t *testing.T) { t.Run("missing ttl", func(t *testing.T) { t.Parallel() - err := a.Init(nil, "IP.test=") + err := expirevar().Init(nil, "IP.test=") if !strings.Contains(err.Error(), "invalid TTL, must be a positive integer") { t.Errorf("expected error 'missing TTL value', got %v", err) } }) - t.Run("missing ttl", func(t *testing.T) { + t.Run("negative ttl", func(t *testing.T) { t.Parallel() - err := a.Init(nil, "IP.test=-1") + err := expirevar().Init(nil, "IP.test=-1") if !strings.Contains(err.Error(), "invalid TTL, must be a positive integer") { t.Errorf("expected error 'missing TTL value', got %v", err) } }) - t.Run("missing ttl", func(t *testing.T) { + t.Run("zero ttl", func(t *testing.T) { t.Parallel() - err := a.Init(nil, "IP.test=0") + err := expirevar().Init(nil, "IP.test=0") if !strings.Contains(err.Error(), "invalid TTL, must be a positive integer") { t.Errorf("expected error 'missing TTL value', got %v", err) } @@ -66,7 +65,7 @@ func TestExpirevar(t *testing.T) { t.Run("valid input", func(t *testing.T) { t.Parallel() - err := a.Init(nil, "TX.testvar=60") + err := expirevar().Init(nil, "TX.testvar=60") if err != nil { t.Errorf("unexpected error for valid input, got %v", err) } diff --git a/internal/actions/initcol_test.go b/internal/actions/initcol_test.go index 01673e4b0..6fa2d72b9 100644 --- a/internal/actions/initcol_test.go +++ b/internal/actions/initcol_test.go @@ -1,6 +1,8 @@ // Copyright 2023 Juan Pablo Tosso and the OWASP Coraza contributors // SPDX-License-Identifier: Apache-2.0 +//go:build !tinygo + package actions_test import ( From a0d7e27446a7c1ae3991f10135f804ed5dc8ec42 Mon Sep 17 00:00:00 2001 From: Artem Murashkin Date: Wed, 22 Jul 2026 17:52:53 +0200 Subject: [PATCH 2/2] feat: TruncateRequestBody releases parked request body (DEF-51195) Connectors that keep a transaction open between the request and logging phases (waiting for the upstream response) pin the full buffered request body for the lifetime of the parked transaction, although those phases never re-read it. TruncateRequestBody(limit) releases the body right after request-phase analysis, keeping at most limit bytes as a preview for audit log part C. The in-memory branch copies the prefix into a fresh buffer instead of bytes.Buffer.Truncate, which retains the backing array (and transaction pooling would keep that capacity alive past Close). The file-backed branch truncates the temp file in place, or closes and removes it for limit=0. The REQUEST_BODY variable is cut to the same prefix via strings.Clone so the original backing string is released too; REQUEST_BODY_LENGTH keeps the original received length and parsed derivatives (ARGS_POST, JSON/XML matches) stay intact. Readers obtained via RequestBodyReader before truncation are invalidated and fail with types.ErrBodyTruncated: their positions may point beyond the shrunk buffer, and quietly serving the prefix or EOF would turn a contract violation (holding a reader across truncation) into silent data loss. Co-Authored-By: Claude Fable 5 --- internal/corazawaf/body_buffer.go | 61 ++++ internal/corazawaf/transaction.go | 24 ++ .../corazawaf/transaction_truncate_test.go | 308 ++++++++++++++++++ types/transaction.go | 26 ++ 4 files changed, 419 insertions(+) create mode 100644 internal/corazawaf/transaction_truncate_test.go diff --git a/internal/corazawaf/body_buffer.go b/internal/corazawaf/body_buffer.go index ba0de41bc..2b92d4d50 100644 --- a/internal/corazawaf/body_buffer.go +++ b/internal/corazawaf/body_buffer.go @@ -96,10 +96,16 @@ func (br *BodyBuffer) Write(data []byte) (n int, err error) { type bodyBufferReader struct { pos int + // err, when set, is returned by every Read; used to surface + // types.ErrBodyTruncated on readers invalidated by Truncate. + err error br *BodyBuffer } func (b *bodyBufferReader) Read(p []byte) (n int, err error) { + if b.err != nil { + return 0, b.err + } if b.br == nil { // reader has been closed and hence we don't attempt to do anymore read return 0, io.EOF @@ -146,6 +152,61 @@ func (br *BodyBuffer) Size() int64 { return br.length } +// Truncate discards all buffered content beyond limit bytes, releasing the +// backing memory. The in-memory buffer is replaced with a fresh one holding +// only the prefix (bytes.Buffer.Truncate retains the full backing array, +// which transaction pooling would keep alive past Close); the file-backed +// buffer is truncated in place, or closed and removed when limit is 0. +// A limit at or above the current size is a no-op. +func (br *BodyBuffer) Truncate(limit int64) error { + if limit >= br.length { + return nil + } + + if environment.HasAccessToFS && br.writer != nil { + if limit == 0 { + w := br.writer + br.writer = nil + br.length = 0 + br.invalidateReaders() + err := w.Close() + // remove even when Close fails: br.writer is already nil, so + // this is the last reference to the file and Reset() would no + // longer clean it up. + if rmErr := os.Remove(w.Name()); err == nil { + err = rmErr + } + return err + } + if err := br.writer.Truncate(limit); err != nil { + // nothing changed: length and readers stay valid + return err + } + br.length = limit + br.invalidateReaders() + return nil + } + fresh := &bytes.Buffer{} + fresh.Write(br.buffer.Bytes()[:limit]) + br.buffer = fresh + br.length = limit + br.invalidateReaders() + return nil +} + +// invalidateReaders marks all outstanding readers with ErrBodyTruncated: +// reading with a position beyond a shrunk buffer would panic, and quietly +// serving the prefix or EOF would hide the contract violation of holding a +// reader across truncation. New readers (e.g. audit log part C) see the +// kept prefix. +func (br *BodyBuffer) invalidateReaders() { + for _, r := range br.readers { + r.br = nil + r.err = types.ErrBodyTruncated + } + br.readers = nil +} + // Reset will reset buffers and delete temporary files func (br *BodyBuffer) Reset() error { br.buffer.Reset() diff --git a/internal/corazawaf/transaction.go b/internal/corazawaf/transaction.go index 64b85bb94..9cbcecd4e 100644 --- a/internal/corazawaf/transaction.go +++ b/internal/corazawaf/transaction.go @@ -153,6 +153,30 @@ func (tx *Transaction) SetScriptUsername(value string) { tx.variables.scriptUsername.Set(value) } +// TruncateRequestBody releases the buffered request body, keeping at most +// limit bytes as a preview for audit logging. See types.Transaction for the +// full contract. The REQUEST_BODY variable is truncated to the same prefix +// (copied, so the original backing string is released); REQUEST_BODY_LENGTH +// and parsed derivatives such as ARGS_POST are left intact. +func (tx *Transaction) TruncateRequestBody(limit int64) error { + if limit < 0 { + return errors.New("limit must be non-negative") + } + // lastPhase only advances when rules are evaluated, so it stays behind + // when the engine is off or the transaction was interrupted — states in + // which request-body analysis will never run and truncation is safe. + if tx.lastPhase < types.PhaseRequestBody && !tx.IsRuleEngineOff() && !tx.IsInterrupted() { + return errors.New("TruncateRequestBody must be called after ProcessRequestBody") + } + if err := tx.requestBodyBuffer.Truncate(limit); err != nil { + return err + } + if rb := tx.variables.requestBody.Get(); int64(len(rb)) > limit { + tx.variables.requestBody.Set(strings.Clone(rb[:limit])) + } + return nil +} + func (tx *Transaction) ID() string { return tx.id } diff --git a/internal/corazawaf/transaction_truncate_test.go b/internal/corazawaf/transaction_truncate_test.go new file mode 100644 index 000000000..6614106a5 --- /dev/null +++ b/internal/corazawaf/transaction_truncate_test.go @@ -0,0 +1,308 @@ +// Copyright 2022 Juan Pablo Tosso and the OWASP Coraza contributors +// SPDX-License-Identifier: Apache-2.0 + +package corazawaf + +import ( + "errors" + "io" + "os" + "strconv" + "strings" + "testing" + + "github.com/corazawaf/coraza/v3/internal/environment" + "github.com/corazawaf/coraza/v3/types" +) + +// makeTruncateTx builds a transaction with an urlencoded body already +// analyzed by ProcessRequestBody. memLimit <= 0 keeps the WAF default +// (in-memory buffering). +func makeTruncateTx(t *testing.T, memLimit int64, body string) *Transaction { + t.Helper() + waf := NewWAF() + if memLimit > 0 { + waf.SetRequestBodyInMemoryLimit(memLimit) + } + tx := waf.NewTransaction() + tx.RequestBodyAccess = true + tx.AuditLogParts = types.AuditLogParts("ABCFHZ") + tx.ProcessURI("/", "POST", "HTTP/1.1") + tx.AddRequestHeader("Content-Type", "application/x-www-form-urlencoded") + tx.ProcessRequestHeaders() + if _, _, err := tx.WriteRequestBody([]byte(body)); err != nil { + t.Fatal(err) + } + if _, err := tx.ProcessRequestBody(); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { tx.Close() }) + return tx +} + +func TestTruncateRequestBodyInMemory(t *testing.T) { + body := "key=" + strings.Repeat("v", 64*1024) + tx := makeTruncateTx(t, 0, body) + + if got := tx.requestBodyBuffer.buffer.Cap(); got < len(body) { + t.Fatalf("test setup: expected in-memory buffer capacity >= %d, got %d", len(body), got) + } + + const limit = 16 + if err := tx.TruncateRequestBody(limit); err != nil { + t.Fatal(err) + } + + if got := tx.requestBodyBuffer.Size(); got != limit { + t.Errorf("buffer size: want %d, got %d", limit, got) + } + if got := tx.requestBodyBuffer.buffer.Cap(); got > 1024 { + t.Errorf("backing array not released: capacity still %d", got) + } + if got := tx.variables.requestBody.Get(); got != body[:limit] { + t.Errorf("REQUEST_BODY: want %q, got %q", body[:limit], got) + } + if got := tx.variables.requestBodyLength.Get(); got != strconv.Itoa(len(body)) { + t.Errorf("REQUEST_BODY_LENGTH: want %d, got %s", len(body), got) + } + // parsed derivatives stay intact + if got := tx.variables.argsPost.Get("key"); len(got) != 1 || got[0] != body[4:] { + t.Error("ARGS_POST was affected by truncation") + } + // audit part C carries exactly the preview + if got := tx.AuditLog().Transaction_.Request_.Body_; got != body[:limit] { + t.Errorf("audit part C: want %q, got %q", body[:limit], got) + } + + // idempotency: repeated and larger-limit calls are no-ops + for _, l := range []int64{limit, limit + 100} { + if err := tx.TruncateRequestBody(l); err != nil { + t.Fatal(err) + } + if got := tx.variables.requestBody.Get(); got != body[:limit] { + t.Errorf("REQUEST_BODY changed after TruncateRequestBody(%d): %q", l, got) + } + } +} + +func TestTruncateRequestBodyInMemoryDiscard(t *testing.T) { + body := "key=" + strings.Repeat("v", 64*1024) + tx := makeTruncateTx(t, 0, body) + + if err := tx.TruncateRequestBody(0); err != nil { + t.Fatal(err) + } + if got := tx.requestBodyBuffer.Size(); got != 0 { + t.Errorf("buffer size: want 0, got %d", got) + } + if got := tx.requestBodyBuffer.buffer.Cap(); got != 0 { + t.Errorf("backing array not released: capacity still %d", got) + } + if got := tx.variables.requestBody.Get(); got != "" { + t.Errorf("REQUEST_BODY: want empty, got %q", got) + } + if got := tx.AuditLog().Transaction_.Request_.Body_; got != "" { + t.Errorf("audit part C: want empty, got %q", got) + } +} + +func TestTruncateRequestBodyFile(t *testing.T) { + if !environment.HasAccessToFS { + return // t.Skip doesn't work on TinyGo + } + body := "key=" + strings.Repeat("v", 1024) + tx := makeTruncateTx(t, 8, body) + + f := tx.requestBodyBuffer.writer + if f == nil { + t.Fatal("test setup: expected file-backed body buffer") + } + + const limit = 16 + if err := tx.TruncateRequestBody(limit); err != nil { + t.Fatal(err) + } + if st, err := os.Stat(f.Name()); err != nil { + t.Fatal(err) + } else if st.Size() != limit { + t.Errorf("tmp file size: want %d, got %d", limit, st.Size()) + } + if got := tx.requestBodyBuffer.Size(); got != limit { + t.Errorf("buffer size: want %d, got %d", limit, got) + } + if got := tx.AuditLog().Transaction_.Request_.Body_; got != body[:limit] { + t.Errorf("audit part C: want %q, got %q", body[:limit], got) + } +} + +func TestTruncateRequestBodyFileDiscard(t *testing.T) { + if !environment.HasAccessToFS { + return // t.Skip doesn't work on TinyGo + } + body := "key=" + strings.Repeat("v", 1024) + tx := makeTruncateTx(t, 8, body) + + f := tx.requestBodyBuffer.writer + if f == nil { + t.Fatal("test setup: expected file-backed body buffer") + } + if err := tx.TruncateRequestBody(0); err != nil { + t.Fatal(err) + } + if tx.requestBodyBuffer.writer != nil { + t.Error("tmp file writer not released") + } + if _, err := os.Stat(f.Name()); err == nil { + t.Error("tmp file was not deleted") + } + if got := tx.requestBodyBuffer.Size(); got != 0 { + t.Errorf("buffer size: want 0, got %d", got) + } +} + +func TestTruncateRequestBodyBeforeProcessing(t *testing.T) { + tx := NewWAF().NewTransaction() + defer tx.Close() + tx.RequestBodyAccess = true + tx.ProcessURI("/", "POST", "HTTP/1.1") + tx.ProcessRequestHeaders() + if _, _, err := tx.WriteRequestBody([]byte("key=value")); err != nil { + t.Fatal(err) + } + + if err := tx.TruncateRequestBody(0); err == nil { + t.Error("expected error when called before ProcessRequestBody") + } + if got := tx.requestBodyBuffer.Size(); got != int64(len("key=value")) { + t.Errorf("body must stay untouched on error, size: %d", got) + } +} + +// Readers obtained before truncation are invalidated: whether consumed or +// untouched, they must fail with ErrBodyTruncated on the next read instead +// of panicking or quietly serving the prefix/EOF. +func TestTruncateRequestBodyStaleReader(t *testing.T) { + tx := makeTruncateTx(t, 0, "key="+strings.Repeat("v", 1024)) + + consumed, err := tx.RequestBodyReader() + if err != nil { + t.Fatal(err) + } + if _, err := io.Copy(io.Discard, consumed); err != nil { + t.Fatal(err) + } + untouched, err := tx.RequestBodyReader() + if err != nil { + t.Fatal(err) + } + if err := tx.TruncateRequestBody(4); err != nil { + t.Fatal(err) + } + for name, r := range map[string]io.Reader{"consumed": consumed, "untouched": untouched} { + n, err := r.Read(make([]byte, 8)) + if n != 0 || !errors.Is(err, types.ErrBodyTruncated) { + t.Errorf("%s stale reader: want (0, ErrBodyTruncated), got (%d, %v)", name, n, err) + } + } +} + +func TestTruncateRequestBodyNegativeLimit(t *testing.T) { + tx := makeTruncateTx(t, 0, "key=value") + if err := tx.TruncateRequestBody(-1); err == nil { + t.Error("expected error for negative limit") + } +} + +// With the rule engine off, ProcessRequestBody skips rule evaluation and +// lastPhase never advances; truncation must still be accepted. +func TestTruncateRequestBodyEngineOff(t *testing.T) { + waf := NewWAF() + waf.RuleEngine = types.RuleEngineOff + tx := waf.NewTransaction() + defer tx.Close() + tx.RequestBodyAccess = true + tx.ProcessURI("/", "POST", "HTTP/1.1") + tx.ProcessRequestHeaders() + if _, _, err := tx.WriteRequestBody([]byte("key=value")); err != nil { + t.Fatal(err) + } + if _, err := tx.ProcessRequestBody(); err != nil { + t.Fatal(err) + } + if err := tx.TruncateRequestBody(0); err != nil { + t.Errorf("engine off: unexpected error: %v", err) + } +} + +// On an interrupted transaction ProcessRequestBody returns early without +// evaluating rules; truncation must still be accepted. +func TestTruncateRequestBodyInterrupted(t *testing.T) { + tx := NewWAF().NewTransaction() + defer tx.Close() + tx.RequestBodyAccess = true + tx.ProcessURI("/", "POST", "HTTP/1.1") + tx.ProcessRequestHeaders() + if _, _, err := tx.WriteRequestBody([]byte("key=value")); err != nil { + t.Fatal(err) + } + tx.interruption = &types.Interruption{Action: "deny"} + if _, err := tx.ProcessRequestBody(); err != nil { + t.Fatal(err) + } + if err := tx.TruncateRequestBody(0); err != nil { + t.Errorf("interrupted: unexpected error: %v", err) + } + if got := tx.requestBodyBuffer.Size(); got != 0 { + t.Errorf("buffer size: want 0, got %d", got) + } +} + +// Even when closing the temp file fails, the file must still be removed: +// br.writer is already nil at that point, so Reset() would never clean it up. +func TestTruncateRequestBodyCloseFailureStillRemovesFile(t *testing.T) { + if !environment.HasAccessToFS { + return // t.Skip doesn't work on TinyGo + } + tx := makeTruncateTx(t, 8, "key="+strings.Repeat("v", 1024)) + + f := tx.requestBodyBuffer.writer + if err := f.Close(); err != nil { // makes the buffer's own Close fail + t.Fatal(err) + } + if err := tx.TruncateRequestBody(0); err == nil { + t.Error("expected error from failing Close") + } + if tx.requestBodyBuffer.writer != nil { + t.Error("tmp file writer not released") + } + if _, err := os.Stat(f.Name()); err == nil { + t.Error("tmp file was not deleted") + } +} + +// A failing file truncation must leave the buffer untouched: size keeps the +// original value and outstanding readers are not invalidated. +func TestTruncateRequestBodyFileTruncateFailureKeepsState(t *testing.T) { + if !environment.HasAccessToFS { + return // t.Skip doesn't work on TinyGo + } + body := "key=" + strings.Repeat("v", 1024) + tx := makeTruncateTx(t, 8, body) + + r, err := tx.RequestBodyReader() + if err != nil { + t.Fatal(err) + } + if err := tx.requestBodyBuffer.writer.Close(); err != nil { // makes Truncate fail + t.Fatal(err) + } + if err := tx.TruncateRequestBody(16); err == nil { + t.Error("expected error from failing file truncation") + } + if got := tx.requestBodyBuffer.Size(); got != int64(len(body)) { + t.Errorf("buffer size: want %d, got %d", len(body), got) + } + if _, err := r.Read(make([]byte, 1)); errors.Is(err, types.ErrBodyTruncated) { + t.Error("reader invalidated although nothing was truncated") + } +} diff --git a/types/transaction.go b/types/transaction.go index 9edc604eb..6773fc45e 100644 --- a/types/transaction.go +++ b/types/transaction.go @@ -4,11 +4,19 @@ package types import ( + "errors" "io" "github.com/corazawaf/coraza/v3/debuglog" ) +// ErrBodyTruncated is returned by request body readers obtained before a +// TruncateRequestBody call: truncation invalidates them, and reading +// truncated data as if it were the full body would be silent data loss. +// Holding a reader across truncation means the caller's flow violates the +// TruncateRequestBody contract. +var ErrBodyTruncated = errors.New("request body released by TruncateRequestBody") + // Transaction is created from a WAF instance to handle web requests and responses, // it contains a copy of most WAF configurations that can be safely changed. // Transactions are used to store all data like URLs, request and response @@ -199,6 +207,24 @@ type Transaction interface { SetScriptFilename(string) SetScriptUsername(string) + // TruncateRequestBody releases the buffered request body once + // request-phase analysis is done, keeping at most limit bytes as a + // preview for audit logging. limit=0 discards the body entirely. + // Intended for connectors that keep a transaction open between the + // request and logging phases (waiting for the upstream response): those + // phases never re-read the request body, so holding it only inflates + // memory for the lifetime of the parked transaction. Call after + // ProcessRequestBody(); calling earlier returns an error and leaves the + // body untouched, unless the rule engine is off or the transaction is + // already interrupted — states in which request-body analysis will + // never run and truncation is accepted at any point. Afterwards audit log part C emits at most limit bytes + // (a raw prefix — it may cut mid-token) and the REQUEST_BODY variable + // holds the same prefix, while REQUEST_BODY_LENGTH keeps the original + // received length and parsed derivatives (ARGS_POST, JSON/XML matches) + // remain intact. Readers previously obtained via RequestBodyReader are + // invalidated and fail with ErrBodyTruncated. + TruncateRequestBody(limit int64) error + // Closer closes the transaction and releases any resources associated with it such as request/response bodies. io.Closer }