From 5626d6c9562a5da600568acadc740b1427f043ed Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jos=C3=A9=20Carlos=20Ch=C3=A1vez?= <jcchavezs@gmail.com>
Date: Mon, 10 Jul 2023 09:40:32 +0200
Subject: [PATCH] chore: keep the handler error instead of trying to hijack it
 as it breaks the user experience. (#85)

* chore: keep the handler error instead of trying to hijack it as it breaks the user experience.

Currently when a later in the chain middleware returned an error with a certain status code, we attempted to wrap that error with an own error which lead to misleading behaviours e.g. https://github.com/corazawaf/coraza-caddy/issues/83.

* chore: returns error to interrupt flow.
---
 coraza.go      | 28 +++++++++++-----------------
 interceptor.go | 32 ++++++++++++++++++++++++++------
 2 files changed, 37 insertions(+), 23 deletions(-)

diff --git a/coraza.go b/coraza.go
index a400f4d..2cc1077 100644
--- a/coraza.go
+++ b/coraza.go
@@ -4,6 +4,7 @@
 package coraza
 
 import (
+	"errors"
 	"net/http"
 	"path/filepath"
 	"strings"
@@ -91,6 +92,8 @@ func (m *corazaModule) Validate() error {
 	return nil
 }
 
+var errInterruptionTriggered = errors.New("interruption triggered")
+
 // ServeHTTP implements caddyhttp.MiddlewareHandler.
 func (m corazaModule) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyhttp.Handler) error {
 	id := randomString(16)
@@ -115,35 +118,26 @@ func (m corazaModule) ServeHTTP(w http.ResponseWriter, r *http.Request, next cad
 	// It fails if any of these functions returns an error and it stops on interruption.
 	if it, err := processRequest(tx, r); err != nil {
 		return caddyhttp.HandlerError{
-			StatusCode: 500,
+			StatusCode: http.StatusInternalServerError,
 			ID:         tx.ID(),
 			Err:        err,
 		}
 	} else if it != nil {
-		w.WriteHeader(obtainStatusCodeFromInterruptionOrDefault(it, http.StatusOK))
-		return nil
+		return caddyhttp.HandlerError{
+			StatusCode: obtainStatusCodeFromInterruptionOrDefault(it, http.StatusOK),
+			ID:         tx.ID(),
+			Err:        errInterruptionTriggered,
+		}
 	}
 
 	ww, processResponse := wrap(w, r, tx)
 
 	// We continue with the other middlewares by catching the response
 	if err := next.ServeHTTP(ww, r); err != nil {
-		return caddyhttp.HandlerError{
-			StatusCode: 500,
-			ID:         tx.ID(),
-			Err:        err,
-		}
+		return err
 	}
 
-	if err := processResponse(tx, r); err != nil {
-		return caddyhttp.HandlerError{
-			StatusCode: 500,
-			ID:         tx.ID(),
-			Err:        err,
-		}
-	}
-
-	return nil
+	return processResponse(tx, r)
 }
 
 // Unmarshal Caddyfile implements caddyfile.Unmarshaler.
diff --git a/interceptor.go b/interceptor.go
index be76443..b0cc5d0 100644
--- a/interceptor.go
+++ b/interceptor.go
@@ -9,6 +9,7 @@ import (
 	"log"
 	"net/http"
 
+	"github.com/caddyserver/caddy/v2/modules/caddyhttp"
 	"github.com/corazawaf/coraza/v3/types"
 )
 
@@ -128,15 +129,25 @@ func wrap(w http.ResponseWriter, r *http.Request, tx types.Transaction) (
 		}
 
 		if tx.IsResponseBodyAccessible() && tx.IsResponseBodyProcessable() {
-
 			if it, err := tx.ProcessResponseBody(); err != nil {
 				i.overrideWriteHeader(http.StatusInternalServerError)
 				i.flushWriteHeader()
-				return err
+
+				return caddyhttp.HandlerError{
+					ID:         tx.ID(),
+					StatusCode: http.StatusInternalServerError,
+					Err:        err,
+				}
 			} else if it != nil {
-				i.overrideWriteHeader(obtainStatusCodeFromInterruptionOrDefault(it, i.statusCode))
+				code := obtainStatusCodeFromInterruptionOrDefault(it, i.statusCode)
+				i.overrideWriteHeader(code)
 				i.flushWriteHeader()
-				return nil
+
+				return caddyhttp.HandlerError{
+					ID:         tx.ID(),
+					StatusCode: code,
+					Err:        errInterruptionTriggered,
+				}
 			}
 
 			// we release the buffer
@@ -144,7 +155,12 @@ func wrap(w http.ResponseWriter, r *http.Request, tx types.Transaction) (
 			if err != nil {
 				i.overrideWriteHeader(http.StatusInternalServerError)
 				i.flushWriteHeader()
-				return fmt.Errorf("failed to release the response body reader: %v", err)
+
+				return caddyhttp.HandlerError{
+					ID:         tx.ID(),
+					StatusCode: http.StatusInternalServerError,
+					Err:        fmt.Errorf("failed to release the response body reader: %v", err),
+				}
 			}
 
 			// this is the last opportunity we have to report the resolved status code
@@ -152,7 +168,11 @@ func wrap(w http.ResponseWriter, r *http.Request, tx types.Transaction) (
 			// response status code.)
 			i.flushWriteHeader()
 			if _, err := io.Copy(w, reader); err != nil {
-				return fmt.Errorf("failed to copy the response body: %v", err)
+				return caddyhttp.HandlerError{
+					ID:         tx.ID(),
+					StatusCode: http.StatusInternalServerError,
+					Err:        fmt.Errorf("failed to copy the response body: %v", err),
+				}
 			}
 		} else {
 			i.flushWriteHeader()