From 350b696984685c7652c9f10c6fd9fb269d3ec04b Mon Sep 17 00:00:00 2001 From: Matteo Pace Date: Thu, 31 Oct 2024 17:30:15 +0100 Subject: [PATCH] refactor process body related logs and doc --- examples/http-server/go.sum | 1 + http/middleware.go | 21 ++------------ internal/corazawaf/transaction.go | 40 +++++++++++++++----------- internal/corazawaf/transaction_test.go | 4 +-- testing/coreruleset/go.sum | 2 -- types/transaction.go | 16 +++++++---- 6 files changed, 39 insertions(+), 45 deletions(-) diff --git a/examples/http-server/go.sum b/examples/http-server/go.sum index 4457419b9..b60148c40 100644 --- a/examples/http-server/go.sum +++ b/examples/http-server/go.sum @@ -1,6 +1,7 @@ github.com/corazawaf/coraza/v3 v3.2.1 h1:zBIji4ut9FtFe8lXdqFwXMAkUoDJZ7HsOlEUYWERLI8= github.com/corazawaf/coraza/v3 v3.2.1/go.mod h1:fVndCGdUHJWl9c26VZPcORQRzUYwMPnRkC6TyTkhbUg= github.com/corazawaf/libinjection-go v0.2.2 h1:Chzodvb6+NXh6wew5/yhD0Ggioif9ACrQGR4qjTCs1g= +github.com/corazawaf/libinjection-go v0.2.2/go.mod h1:OP4TM7xdJ2skyXqNX1AN1wN5nNZEmJNuWbNPOItn7aw= github.com/foxcpp/go-mockdns v1.1.0 h1:jI0rD8M0wuYAxL7r/ynTrCQQq0BVqfB99Vgk7DlmewI= github.com/foxcpp/go-mockdns v1.1.0/go.mod h1:IhLeSFGed3mJIAXPH2aiRQB+kqz7oqu8ld2qVbOu7Wk= github.com/magefile/mage v1.15.0 h1:BvGheCMAsG3bWUDbZ8AyXXpCNwU9u5CB6sM+HNb9HYg= diff --git a/http/middleware.go b/http/middleware.go index acb72e16d..c1e3f50bb 100644 --- a/http/middleware.go +++ b/http/middleware.go @@ -86,27 +86,10 @@ func processRequest(tx types.Transaction, req *http.Request) (*types.Interruptio // Adds all remaining bytes beyond the coraza limit to its buffer // It happens when the partial body has been processed and it did not trigger an interruption - body := io.MultiReader(rbr, req.Body) + bodyReader := io.MultiReader(rbr, req.Body) // req.Body is transparently reinizialied with a new io.ReadCloser. // The http handler will be able to read it. - // Prior to Go 1.19 NopCloser does not implement WriterTo if the reader implements it. - // - https://github.com/golang/go/issues/51566 - // - https://tip.golang.org/doc/go1.19#minor_library_changes - // This avoid errors like "failed to process request: malformed chunked encoding" when - // using io.Copy. - // In Go 1.19 we just do `req.Body = io.NopCloser(reader)` - if rwt, ok := body.(io.WriterTo); ok { - req.Body = struct { - io.Reader - io.WriterTo - io.Closer - }{body, rwt, req.Body} - } else { - req.Body = struct { - io.Reader - io.Closer - }{body, req.Body} - } + req.Body = io.NopCloser(bodyReader) } } diff --git a/internal/corazawaf/transaction.go b/internal/corazawaf/transaction.go index c05128271..7fce408f3 100644 --- a/internal/corazawaf/transaction.go +++ b/internal/corazawaf/transaction.go @@ -969,9 +969,9 @@ func (tx *Transaction) ReadRequestBodyFrom(r io.Reader) (*types.Interruption, in // ProcessRequestBody Performs the analysis of the request body (if any) // -// This method perform the analysis on the request body. It is optional to -// call that function. If this API consumer already knows that there isn't a -// body for inspect it is recommended to skip this step. +// It is recommended to call this method even if it is not expected to have a body. +// It permits to execute rules belonging to request body phase, but not necesarily +// processing the request body. // // Remember to check for a possible intervention. func (tx *Transaction) ProcessRequestBody() (*types.Interruption, error) { @@ -985,11 +985,15 @@ func (tx *Transaction) ProcessRequestBody() (*types.Interruption, error) { } if tx.lastPhase != types.PhaseRequestHeaders { - if tx.lastPhase >= types.PhaseRequestBody { - // Phase already evaluated or skipped - tx.debugLogger.Warn().Msg("ProcessRequestBody should have already been called") - } else { - tx.debugLogger.Debug().Msg("Skipping request body processing, anomalous call before request headers evaluation") + switch { + case tx.lastPhase == types.PhaseRequestBody: + // This condition can happen quite often when ProcessPartial is used as the write body functions call ProcessRequestBody when + // the limit is reached + tx.debugLogger.Debug().Msg("Request body processing has been already performed") + case tx.lastPhase > types.PhaseRequestBody: + tx.debugLogger.Warn().Msg("Skipping anomalous call to ProcessRequestBody. It should have already been called") + default: + tx.debugLogger.Warn().Msg("Skipping anomalous call to ProcessRequestBody. It has been called before request headers evaluation") } return nil, nil } @@ -1215,9 +1219,9 @@ func (tx *Transaction) ReadResponseBodyFrom(r io.Reader) (*types.Interruption, i // ProcessResponseBody Perform the analysis of the the response body (if any) // -// This method perform the analysis on the response body. It is optional to -// call that method. If this API consumer already knows that there isn't a -// body for inspect it is recommended to skip this step. +// It is recommended to call this method even if it is not expected to have a body. +// It permits to execute rules belonging to request body phase, but not necesarily +// processing the response body. // // note Remember to check for a possible intervention. func (tx *Transaction) ProcessResponseBody() (*types.Interruption, error) { @@ -1231,14 +1235,18 @@ func (tx *Transaction) ProcessResponseBody() (*types.Interruption, error) { } if tx.lastPhase != types.PhaseResponseHeaders { - if tx.lastPhase >= types.PhaseResponseBody { - // Phase already evaluated or skipped - tx.debugLogger.Warn().Msg("ProcessResponseBody should have already been called") - } else { + switch { + case tx.lastPhase == types.PhaseResponseBody: + // This condition can happen quite often when ProcessPartial is used as the write body functions call ProcessResponseBody when + // the limit is reached + tx.debugLogger.Debug().Msg("Response body processing has been already performed") + case tx.lastPhase > types.PhaseResponseBody: + tx.debugLogger.Warn().Msg("Skipping anomalous call to ProcessResponseBody. It should have already been called") + default: // Prevents evaluating response body rules if last phase has not been response headers. It may happen // when a server returns an error prior to evaluating WAF rules, but ResponseBody is still called at // the end of http stream - tx.debugLogger.Debug().Msg("Skipping response body processing, anomalous call before response headers evaluation") + tx.debugLogger.Warn().Msg("Skipping anomalous call to ProcessResponseBody. It has been called before response headers evaluation") } return nil, nil } diff --git a/internal/corazawaf/transaction_test.go b/internal/corazawaf/transaction_test.go index bfff8821a..0bb0c4c65 100644 --- a/internal/corazawaf/transaction_test.go +++ b/internal/corazawaf/transaction_test.go @@ -1018,10 +1018,10 @@ func TestProcessBodiesSkippedIfHeadersPhasesNotReached(t *testing.T) { if want, have := 3, len(logEntries); want != have { t.Fatalf("unexpected number of log entries, want %d, have %d", want, have) } - if want, have := "anomalous call before request headers evaluation", logEntries[1]; !strings.Contains(have, want) { + if want, have := "has been called before request headers evaluation", logEntries[1]; !strings.Contains(have, want) { t.Fatalf("unexpected message, want %q, have %q", want, have) } - if want, have := "anomalous call before response headers evaluation", logEntries[2]; !strings.Contains(have, want) { + if want, have := "has been called before response headers evaluation", logEntries[2]; !strings.Contains(have, want) { t.Fatalf("unexpected message, want %q, have %q", want, have) } if err := tx.Close(); err != nil { diff --git a/testing/coreruleset/go.sum b/testing/coreruleset/go.sum index 7a692094c..c978a841b 100644 --- a/testing/coreruleset/go.sum +++ b/testing/coreruleset/go.sum @@ -4,8 +4,6 @@ github.com/Masterminds/semver v1.5.0 h1:H65muMkzWKEuNDnfl9d70GUjFniHKHRbFPGBuZ3Q github.com/Masterminds/semver v1.5.0/go.mod h1:MB6lktGJrhw8PrUyiEoblNEGEQ+RzHPF078ddwwvV3Y= github.com/Masterminds/sprig v2.22.0+incompatible h1:z4yfnGrZ7netVz+0EDJ0Wi+5VZCSYp4Z0m2dk6cEM60= github.com/Masterminds/sprig v2.22.0+incompatible/go.mod h1:y6hNFY5UBTIWBxnzTeuNhlNS5hqE0NB0E6fgfo2Br3o= -github.com/bmatcuk/doublestar/v4 v4.6.1 h1:FH9SifrbvJhnlQpztAx++wlkk70QBf0iBWDwNy7PA4I= -github.com/bmatcuk/doublestar/v4 v4.6.1/go.mod h1:xBQ8jztBU6kakFMg+8WGxn0c6z1fTSPVIjEY1Wr7jzc= github.com/bmatcuk/doublestar/v4 v4.7.1 h1:fdDeAqgT47acgwd9bd9HxJRDmc9UAmPpc+2m0CXv75Q= github.com/bmatcuk/doublestar/v4 v4.7.1/go.mod h1:xBQ8jztBU6kakFMg+8WGxn0c6z1fTSPVIjEY1Wr7jzc= github.com/corazawaf/coraza-coreruleset/v4 v4.6.0 h1:VGlMw3QMuKaV7XgifPgcqCm66K+HRSdM4d9PRh1nD50= diff --git a/types/transaction.go b/types/transaction.go index dfbb3c66e..97c7d7ce0 100644 --- a/types/transaction.go +++ b/types/transaction.go @@ -77,9 +77,9 @@ type Transaction interface { // ProcessRequestBody Performs the analysis of the request body (if any) // - // This method perform the analysis on the request body. It is optional to - // call that function. If this API consumer already knows that there isn't a - // body for inspect it is recommended to skip this step. + // It is recommended to call this method even if it is not expected to have a body. + // It permits to execute rules belonging to request body phase, but not necesarily + // processing the request body. // // Remember to check for a possible intervention. ProcessRequestBody() (*Interruption, error) @@ -88,6 +88,8 @@ type Transaction interface { // returns an interruption if the body is bigger than the limit and the action is to // reject. This is specially convenient to resolve an interruption before copying // the body into the request body buffer. + // ProcessRequestBody is called automatically when the action is to process partially + // the body (up to the limit) if the limit is reached. // // It returns the corresponding interruption, the number of bytes written an error if any. WriteRequestBody(b []byte) (*Interruption, int, error) @@ -96,6 +98,8 @@ type Transaction interface { // returns an interruption if the body is bigger than the limit and the action is to // reject. This is specially convenient to resolve an interruption before copying // the body into the request body buffer. + // ProcessRequestBody is called automatically when the action is to process partially + // the body (up to the limit) if the limit is reached. // // It returns the corresponding interruption, the number of bytes written an error if any. ReadRequestBodyFrom(io.Reader) (*Interruption, int, error) @@ -120,9 +124,9 @@ type Transaction interface { // ProcessResponseBody Perform the analysis of the response body (if any) // - // This method perform the analysis on the response body. It is optional to - // call that method. If this API consumer already knows that there isn't a - // body for inspect it is recommended to skip this step. + // It is recommended to call this method even if it is not expected to have a body. + // It permits to execute rules belonging to request body phase, but not necesarily + // processing the response body. // // note Remember to check for a possible intervention. ProcessResponseBody() (*Interruption, error)