Skip to content

Commit

Permalink
refactor process body related logs and doc
Browse files Browse the repository at this point in the history
  • Loading branch information
M4tteoP committed Oct 31, 2024
1 parent 7205791 commit 350b696
Show file tree
Hide file tree
Showing 6 changed files with 39 additions and 45 deletions.
1 change: 1 addition & 0 deletions examples/http-server/go.sum
Original file line number Diff line number Diff line change
@@ -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=
Expand Down
21 changes: 2 additions & 19 deletions http/middleware.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}

Expand Down
40 changes: 24 additions & 16 deletions internal/corazawaf/transaction.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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
}
Expand Down Expand Up @@ -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) {
Expand All @@ -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
}
Expand Down
4 changes: 2 additions & 2 deletions internal/corazawaf/transaction_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 0 additions & 2 deletions testing/coreruleset/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -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=
Expand Down
16 changes: 10 additions & 6 deletions types/transaction.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand Down

0 comments on commit 350b696

Please sign in to comment.