fix(header): static file error gzip header handling and add tests (#123)

* fix: fix static file error gzip header handling and add tests

- Delay removal of gzip headers for error responses until Write is called, ensuring headers are set correctly for static files.
- Remove gzip headers for error responses after the handler finishes, improving behavior for static file serving.
- Add comprehensive tests covering static file compression, uncompressed responses, 404 error handling, and directory listing.
- Add a bug-repro test demonstrating that static file gzip headers may be incorrectly removed due to how status codes are handled.

fix #122

Signed-off-by: appleboy <[email protected]>

* test: improve test maintainability and security through refactoring

- Replace hardcoded "gzip" string with a named constant for improved maintainability in tests
- Change test file permissions in static file tests from octal 0644 to 0o600 for better security, except for one case using 0o644
- Minor formatting improvements to string concatenation in test content

Signed-off-by: appleboy <[email protected]>

* fix: set secure file permissions for generated test.js

- Change file permission from 644 to 600 when writing test.js in the static file test

Signed-off-by: appleboy <[email protected]>

---------

Signed-off-by: appleboy <[email protected]>

Author: appleboy

gzip.go

@@ -42,6 +42,7 @@ func (g *gzipWriter) Write(data []byte) (int, error) {
 	}
 
 	// For error responses (4xx, 5xx), don't compress
+	// Always check the current status, even if WriteHeader was called
 	if g.status >= 400 {
 		g.removeGzipHeaders()
 		return g.ResponseWriter.Write(data)
@@ -90,10 +91,9 @@ func (g *gzipWriter) WriteHeader(code int) {
 	g.status = code
 	g.statusWritten = true
 
-	// Remove gzip headers for error responses
-	if code >= 400 {
-		g.removeGzipHeaders()
-	}
+	// Don't remove gzip headers immediately for error responses in WriteHeader
+	// because some handlers (like static file server) may call WriteHeader multiple times
+	// We'll check the status in Write() method when content is actually written
 
 	g.Header().Del("Content-Length")
 	g.ResponseWriter.WriteHeader(code)

handler.go

@@ -89,6 +89,8 @@ func (g *gzipHandler) Handle(c *gin.Context) {
 	defer func() {
 		// Only close gzip writer if it was actually used (not for error responses)
 		if gw.status >= 400 {
+			// Remove gzip headers for error responses when handler is complete
+			gw.removeGzipHeaders()
 			gz.Reset(io.Discard)
 		} else if c.Writer.Size() < 0 {
 			// do not write gzip footer when nothing is written to the response body

handler_test.go

@@ -13,6 +13,8 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
+const gzipEncoding = "gzip"
+
 func TestHandleGzip(t *testing.T) {
 	gin.SetMode(gin.TestMode)
 
@@ -26,8 +28,8 @@ func TestHandleGzip(t *testing.T) {
 		{
 			name:                    "Gzip compression",
 			path:                    "/",
-			acceptEncoding:          "gzip",
-			expectedContentEncoding: "gzip",
+			acceptEncoding:          gzipEncoding,
+			expectedContentEncoding: gzipEncoding,
 			expectedBody:            "Gzip Test Response",
 		},
 		{
@@ -56,7 +58,7 @@ func TestHandleGzip(t *testing.T) {
 			assert.Equal(t, http.StatusOK, w.Code)
 			assert.Equal(t, tt.expectedContentEncoding, w.Header().Get("Content-Encoding"))
 
-			if tt.expectedContentEncoding == "gzip" {
+			if tt.expectedContentEncoding == gzipEncoding {
 				gr, err := gzip.NewReader(w.Body)
 				assert.NoError(t, err)
 				defer gr.Close()
@@ -91,7 +93,7 @@ func TestHandleDecompressGzip(t *testing.T) {
 	})
 
 	req, _ := http.NewRequestWithContext(context.Background(), "POST", "/", buf)
-	req.Header.Set("Content-Encoding", "gzip")
+	req.Header.Set("Content-Encoding", gzipEncoding)
 
 	w := httptest.NewRecorder()
 	router.ServeHTTP(w, req)
@@ -110,7 +112,7 @@ func TestHandle404NoCompression(t *testing.T) {
 	}{
 		{
 			name:           "404 with gzip accept-encoding should not compress",
-			acceptEncoding: "gzip",
+			acceptEncoding: gzipEncoding,
 			expectedGzip:   false,
 		},
 		{
@@ -142,7 +144,7 @@ func TestHandle404NoCompression(t *testing.T) {
 			// Check that Content-Encoding header is not set for 404 responses
 			contentEncoding := w.Header().Get("Content-Encoding")
 			if tt.expectedGzip {
-				assert.Equal(t, "gzip", contentEncoding)
+				assert.Equal(t, gzipEncoding, contentEncoding)
 			} else {
 				assert.Empty(t, contentEncoding, "404 responses should not have Content-Encoding: gzip")
 			}

static_test.go

@@ -0,0 +1,191 @@
+package gzip
+
+import (
+	"context"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/gin-gonic/gin"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestStaticFileWithGzip(t *testing.T) {
+	// Create a temporary directory and file for testing
+	tmpDir, err := os.MkdirTemp("", "gzip_static_test")
+	require.NoError(t, err)
+	defer os.RemoveAll(tmpDir)
+
+	// Create a test file
+	testFile := filepath.Join(tmpDir, "test.txt")
+	testContent := "This is a test file for static gzip compression testing. " +
+		"It should be long enough to trigger gzip compression."
+	err = os.WriteFile(testFile, []byte(testContent), 0o600)
+	require.NoError(t, err)
+
+	// Set up Gin router with gzip middleware and static file serving
+	gin.SetMode(gin.TestMode)
+	router := gin.New()
+	router.Use(Gzip(DefaultCompression))
+	router.Static("/static", tmpDir)
+
+	// Test static file request with gzip support
+	req, _ := http.NewRequestWithContext(context.Background(), "GET", "/static/test.txt", nil)
+	req.Header.Add(headerAcceptEncoding, "gzip")
+
+	w := httptest.NewRecorder()
+	router.ServeHTTP(w, req)
+
+	// The response should be successful and compressed
+	assert.Equal(t, http.StatusOK, w.Code)
+
+	// This is what should happen but currently fails due to the bug
+	// The static handler initially sets status to 404, causing gzip headers to be removed
+	assert.Equal(t, "gzip", w.Header().Get(headerContentEncoding), "Static file should be gzip compressed")
+	assert.Equal(t, headerAcceptEncoding, w.Header().Get(headerVary), "Vary header should be set")
+
+	// The compressed content should be smaller than original
+	assert.Less(t, w.Body.Len(), len(testContent), "Compressed content should be smaller")
+}
+
+func TestStaticFileWithoutGzip(t *testing.T) {
+	// Create a temporary directory and file for testing
+	tmpDir, err := os.MkdirTemp("", "gzip_static_test")
+	require.NoError(t, err)
+	defer os.RemoveAll(tmpDir)
+
+	// Create a test file
+	testFile := filepath.Join(tmpDir, "test.txt")
+	testContent := "This is a test file."
+	err = os.WriteFile(testFile, []byte(testContent), 0o600)
+	require.NoError(t, err)
+
+	// Set up Gin router with gzip middleware and static file serving
+	gin.SetMode(gin.TestMode)
+	router := gin.New()
+	router.Use(Gzip(DefaultCompression))
+	router.Static("/static", tmpDir)
+
+	// Test static file request without gzip support
+	req, _ := http.NewRequestWithContext(context.Background(), "GET", "/static/test.txt", nil)
+	// No Accept-Encoding header
+
+	w := httptest.NewRecorder()
+	router.ServeHTTP(w, req)
+
+	// The response should be successful and not compressed
+	assert.Equal(t, http.StatusOK, w.Code)
+	assert.Equal(t, "", w.Header().Get(headerContentEncoding), "Content should not be compressed")
+	assert.Equal(t, "", w.Header().Get(headerVary), "Vary header should not be set")
+	assert.Equal(t, testContent, w.Body.String(), "Content should match original")
+}
+
+func TestStaticFileNotFound(t *testing.T) {
+	// Create a temporary directory (but no files)
+	tmpDir, err := os.MkdirTemp("", "gzip_static_test")
+	require.NoError(t, err)
+	defer os.RemoveAll(tmpDir)
+
+	// Set up Gin router with gzip middleware and static file serving
+	gin.SetMode(gin.TestMode)
+	router := gin.New()
+	router.Use(Gzip(DefaultCompression))
+	router.Static("/static", tmpDir)
+
+	// Test request for non-existent file
+	req, _ := http.NewRequestWithContext(context.Background(), "GET", "/static/nonexistent.txt", nil)
+	req.Header.Add(headerAcceptEncoding, "gzip")
+
+	w := httptest.NewRecorder()
+	router.ServeHTTP(w, req)
+
+	// The response should be 404 and not compressed (this should work correctly)
+	assert.Equal(t, http.StatusNotFound, w.Code)
+	assert.Equal(t, "", w.Header().Get(headerContentEncoding), "404 response should not be compressed")
+	assert.Equal(t, "", w.Header().Get(headerVary), "Vary header should be removed for error responses")
+}
+
+func TestStaticDirectoryListing(t *testing.T) {
+	// Create a temporary directory with a file
+	tmpDir, err := os.MkdirTemp("", "gzip_static_test")
+	require.NoError(t, err)
+	defer os.RemoveAll(tmpDir)
+
+	// Create a test file
+	testFile := filepath.Join(tmpDir, "test.txt")
+	err = os.WriteFile(testFile, []byte("test content"), 0o600)
+	require.NoError(t, err)
+
+	// Set up Gin router with gzip middleware and static file serving
+	gin.SetMode(gin.TestMode)
+	router := gin.New()
+	router.Use(Gzip(DefaultCompression))
+	router.Static("/static", tmpDir)
+
+	// Test directory listing request
+	req, _ := http.NewRequestWithContext(context.Background(), "GET", "/static/", nil)
+	req.Header.Add(headerAcceptEncoding, "gzip")
+
+	w := httptest.NewRecorder()
+	router.ServeHTTP(w, req)
+
+	// Note: Gin's default static handler doesn't enable directory listing
+	// so this will return 404, which should NOT be compressed
+	assert.Equal(t, http.StatusNotFound, w.Code)
+	assert.Equal(t, "", w.Header().Get(headerContentEncoding), "404 response should not be compressed")
+	assert.Equal(t, "", w.Header().Get(headerVary), "Vary header should be removed for error responses")
+}
+
+// This test demonstrates the specific issue mentioned in #122
+func TestStaticFileGzipHeadersBug(t *testing.T) {
+	// Create a temporary directory and file for testing
+	tmpDir, err := os.MkdirTemp("", "gzip_static_test")
+	require.NoError(t, err)
+	defer os.RemoveAll(tmpDir)
+
+	// Create a test file
+	testFile := filepath.Join(tmpDir, "test.js")
+	testContent := "console.log('This is a JavaScript file that should be compressed when served as a static file');"
+	err = os.WriteFile(testFile, []byte(testContent), 0o600)
+	require.NoError(t, err)
+
+	// Set up Gin router with gzip middleware and static file serving
+	gin.SetMode(gin.TestMode)
+	router := gin.New()
+	router.Use(Gzip(DefaultCompression))
+	router.Static("/assets", tmpDir)
+
+	// Test static file request with gzip support
+	req, _ := http.NewRequestWithContext(context.Background(), "GET", "/assets/test.js", nil)
+	req.Header.Add(headerAcceptEncoding, "gzip")
+
+	w := httptest.NewRecorder()
+	router.ServeHTTP(w, req)
+
+	t.Logf("Response Status: %d", w.Code)
+	t.Logf("Content-Encoding: %s", w.Header().Get(headerContentEncoding))
+	t.Logf("Vary: %s", w.Header().Get(headerVary))
+	t.Logf("Content-Length: %s", w.Header().Get("Content-Length"))
+	t.Logf("Body Length: %d", w.Body.Len())
+
+	// This test will currently fail due to the bug described in issue #122
+	// The static handler sets status to 404 initially, causing gzip middleware to remove headers
+	assert.Equal(t, http.StatusOK, w.Code)
+
+	// These assertions will fail with the current bug:
+	// - Content-Encoding header will be empty instead of "gzip"
+	// - Vary header will be empty instead of "Accept-Encoding"
+	// - Content will not be compressed
+	if w.Header().Get(headerContentEncoding) != "gzip" {
+		t.Errorf("BUG REPRODUCED: Static file is not being gzip compressed. Content-Encoding: %q, expected: %q",
+			w.Header().Get(headerContentEncoding), "gzip")
+	}
+
+	if w.Header().Get(headerVary) != headerAcceptEncoding {
+		t.Errorf("BUG REPRODUCED: Vary header missing. Vary: %q, expected: %q",
+			w.Header().Get(headerVary), headerAcceptEncoding)
+	}
+}