[TT-5070] Duplicate WriteHeader calls cause malformed responses from golang plugins

Makefile

@@ -53,6 +53,12 @@ build-linux:
 build-debug:
 	$(GOBUILD) -gcflags="all=-N -l" -tags "$(TAGS)" -o $(BINARY_NAME) .
 
+# build plugins for tests
+build-plugins:
+	@go build -tags goplugin -buildmode=plugin -gcflags "all=-N -l" -o ./test/goplugins/goplugins_debug.so ./test/goplugins/
+	@go build -tags goplugin -buildmode=plugin -race -o ./test/goplugins/goplugins_race.so ./test/goplugins/
+	@go build -tags goplugin -buildmode=plugin -o ./test/goplugins/goplugins.so ./test/goplugins/
+
 .PHONY: install
 install:
 	$(GOINSTALL) -tags "$(TAGS)"
@@ -95,5 +101,4 @@ docker:
 	docker build --platform ${BUILD_PLATFORM} --rm -t internal/tyk-gateway .
 
 docker-std: build
-	docker build --platform ${BUILD_PLATFORM} --no-cache -t internal/tyk-gateway:std -f ci/Dockerfile.std .
-
+	docker build --platform ${BUILD_PLATFORM} --no-cache -t internal/tyk-gateway:std -f ci/Dockerfile.std .

gateway/middleware.go

@@ -180,10 +180,17 @@
 
 			err, errCode := mw.ProcessRequest(w, r, mwConf)
 
+			// Workaround
+			// ProcessRequest signature is too narrow it has to be extended to handle cases like this
+			// Abstraction should not know anything about implementation
+			if errors.Is(err, ErrResponseSucceed) {
+				err = nil
+			}
+
 			if err != nil {
-				writeResponse := true
 				// Prevent double error write
-				if goPlugin, isGoPlugin := actualMW.(*GoPluginMiddleware); isGoPlugin && goPlugin.handler != nil {
+				writeResponse := true
+				if goPlugin, isGoPlugin := actualMW.(*GoPluginMiddleware); isGoPlugin && goPlugin.handler != nil || errors.Is(err, ErrResponseErrorSent) {
 					writeResponse = false
 				}
 

gateway/mw_go_plugin.go

@@ -15,9 +15,23 @@ import (
 	"github.com/TykTechnologies/tyk/goplugin"
 	"github.com/TykTechnologies/tyk/internal/httpctx"
 	"github.com/TykTechnologies/tyk/internal/middleware"
+	"github.com/TykTechnologies/tyk/pkg/errpack"
 	"github.com/TykTechnologies/tyk/request"
 )
 
+var (
+	ErrResponseSucceed = errpack.New(
+		"response succeed",
+		errpack.WithType(errpack.TypeDomain),
+		errpack.WithLogLevel(logrus.TraceLevel),
+	)
+	ErrResponseErrorSent = errpack.New(
+		"plugin error",
+		errpack.WithType(errpack.TypeDomain),
+		errpack.WithLogLevel(logrus.DebugLevel),
+	)
+)
+
 // customResponseWriter is a wrapper around standard http.ResponseWriter
 // plus it tracks if response was sent and what status code was sent
 type customResponseWriter struct {
@@ -302,7 +316,7 @@ func (m *GoPluginMiddleware) handlePluginResponse(
 	}, rw.statusCodeSent, rw.getHttpResponse(r), false)
 
 	// no need to continue passing this request down to reverse proxy
-	return nil, middleware.StatusRespond
+	return ErrResponseSucceed, middleware.StatusRespond
 }
 
 func (m *GoPluginMiddleware) handleErrorResponse(
@@ -327,5 +341,9 @@ func (m *GoPluginMiddleware) handleErrorResponse(
 	err := fmt.Errorf("plugin function sent error response code: %d", rw.statusCodeSent)
 	logger.WithError(err).Error("Failed to process request with Go-plugin middleware func")
 
+	if rw.responseSent {
+		err = fmt.Errorf("%w: %w", ErrResponseErrorSent, err)
+	}
+
 	return err, rw.statusCodeSent
 }

goplugin/mw_go_plugin_test.go

@@ -745,3 +745,143 @@
 	assert.True(t, found)
 	assert.Equal(t, float64(114), session.Rate)
 }
+
+func TestGoPlugin_DontWriteBodyInCaseIfPluginRespondsWith4xxOrHigher(t *testing.T) {
+	t.Run("api level plugin", func(t *testing.T) {
+		t.Run("writes body", func(t *testing.T) {
+			ts := gateway.StartTest(nil)
+			t.Cleanup(ts.Close)
+
+			ts.Gw.BuildAndLoadAPI(func(spec *gateway.APISpec) {
+				spec.Proxy.ListenPath = "/test-api/"
+				spec.UseKeylessAccess = true
+				spec.UseStandardAuth = false
+				spec.CustomMiddleware = apidef.MiddlewareSection{
+					Driver: apidef.GoPluginDriver,
+					Pre: []apidef.MiddlewareDefinition{
+						{
+							Name: "RejectWithBody",
+							Path: goPluginFilename(),
+						},
+					},
+				}
+			})
+
+			ts.Run(t, []test.TestCase{
+				{
+					Path: "/test-api/get",
+					Code: http.StatusForbidden,
+					BodyMatchFunc: func(bytes []byte) bool {
+						return assert.Equal(t, "hello", string(bytes))
+					},
+				},
+			}...)
+		})
+
+		t.Run("does not write body if plugin does not write one", func(t *testing.T) {
+			ts := gateway.StartTest(nil)
+			t.Cleanup(ts.Close)
+
+			ts.Gw.BuildAndLoadAPI(func(spec *gateway.APISpec) {
+				spec.Proxy.ListenPath = "/test-api-without/"
+				spec.UseKeylessAccess = true
+				spec.UseStandardAuth = false
+				spec.CustomMiddleware = apidef.MiddlewareSection{
+					Driver: apidef.GoPluginDriver,
+					Pre: []apidef.MiddlewareDefinition{
+						{
+							Name: "RejectWithoutBody",
+							Path: goPluginFilename(),
+						},
+					},
+				}
+			})
+
+			ts.Run(t, []test.TestCase{
+				{
+					Path: "/test-api-without/get",
+					Code: http.StatusForbidden,
+					BodyMatchFunc: func(bytes []byte) bool {
+						return assert.Equal(t, "", string(bytes))
+					},
+				},
+			}...)
+		})
+	})
+
+	t.Run("endpoint level plugin", func(t *testing.T) {
+		t.Run("writes body if plugin writes one", func(t *testing.T) {
+			ts := gateway.StartTest(nil)
+			t.Cleanup(ts.Close)
+
+			ts.Gw.BuildAndLoadAPI(func(spec *gateway.APISpec) {
+				spec.Proxy.ListenPath = "/test-api/"
+				spec.UseKeylessAccess = true
+				spec.UseStandardAuth = false
+
+				// Configure endpoint-level Go plugin
+				v := spec.VersionData.Versions["Default"]
+				v.UseExtendedPaths = true
+				v.ExtendedPaths = apidef.ExtendedPathsSet{
+					GoPlugin: []apidef.GoPluginMeta{
+						{
+							Path:       "/get",
+							Method:     "GET",
+							PluginPath: goPluginFilename(),
+							SymbolName: "RejectWithBody",
+						},
+					},
+				}
+				spec.VersionData.Versions["Default"] = v
+			})
+
+			ts.Run(t, []test.TestCase{
+				{
+					Path: "/test-api/get",
+					Code: http.StatusForbidden,
+					BodyMatchFunc: func(bytes []byte) bool {
+						return assert.Equal(t, "hello", string(bytes))
+					},
+				},
+			}...)
+		})
+
+		t.Run("does not write body if plugin does not write one", func(t *testing.T) {
+			ts := gateway.StartTest(nil)
+			t.Cleanup(ts.Close)
+
+			gateway.BuildOASAPI()
+
+			ts.Gw.BuildAndLoadAPI(func(spec *gateway.APISpec) {
+				spec.Proxy.ListenPath = "/test-api-without/"
+				spec.UseKeylessAccess = true
+				spec.UseStandardAuth = false
+
+				// Configure endpoint-level Go plugin
+				v := spec.VersionData.Versions["Default"]
+				v.UseExtendedPaths = true
+				v.ExtendedPaths = apidef.ExtendedPathsSet{
+					GoPlugin: []apidef.GoPluginMeta{
+						{
+							Path:       "/get",
+							Method:     "GET",
+							PluginPath: goPluginFilename(),
+							SymbolName: "RejectWithoutBody",
+						},
+					},
+				}
+				spec.VersionData.Versions["Default"] = v
+			})
+
+			ts.Run(t, []test.TestCase{
+				{
+					Path: "/test-api-without/get",
+					Code: http.StatusForbidden,
+					BodyMatchFunc: func(bytes []byte) bool {
+						return assert.Equal(t, "", string(bytes))
+					},
+				},
+			}...)
+		})
+	})
+}

test/goplugins/test_goplugin.go

@@ -227,3 +227,20 @@ func MyPluginApplyingPolicy(rw http.ResponseWriter, r *http.Request) {
 
 	ctx.SetSession(r, session, true)
 }
+
+func RejectWithBody(
+	rw http.ResponseWriter,
+	_ *http.Request,
+) {
+
+	rw.WriteHeader(403)
+	_, _ = rw.Write([]byte(`hello`)) // nolint:errcheck
+}
+
+func RejectWithoutBody(
+	rw http.ResponseWriter,
+	_ *http.Request,
+) {
+
+	rw.WriteHeader(403)
+}