Skip to content

feature: add gzip support for static files #143

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
siamskoi wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

feature: add gzip support for static files #143

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3297bdf
Add gzip support for static files
siamskoi Jan 28, 2025
86baaaf
Enable configurable gzip compression with file size limits
siamskoi Jan 30, 2025
c9999dc
Enable configurable gzip compression with file size limits
siamskoi Jan 30, 2025
37d9eb0
Refactor gzip handling to use klauspost/compress library
siamskoi Feb 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ type Config struct {

// Response headers to add to every static.
Response map[string]string `mapstructure:"response"`

Gzip bool `mapstructure:"gzip"`
}

// Valid returns nil if config is valid.
Expand Down
76 changes: 76 additions & 0 deletions plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
package static

import (
"compress/gzip"
"fmt"
"io"
"net/http"
"os"
"path"
"strings"
"sync"
"unsafe"

rrcontext "github.com/roadrunner-server/context"
Expand All @@ -23,6 +27,8 @@ const (
RootPluginName = "http"
)

var cacheMutex sync.Mutex
Copy link

@coderabbitai coderabbitai bot Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Move cacheMutex to Plugin struct.

The global mutex could cause issues with testing and multiple plugin instances. Consider moving it to the Plugin struct.

-var cacheMutex sync.Mutex

 type Plugin struct {
     // ... other fields ...
+    cacheMutex sync.Mutex
     gzip bool
 }

Committable suggestion skipped: line range outside the PR's diff.


Copy link
Member

@rustatian rustatian Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No global variables (this is a bad practice in Go) allowed

type Configurer interface {
// UnmarshalKey takes a single key and unmarshal it into a Struct.
UnmarshalKey(name string, out any) error
Expand All @@ -48,6 +54,8 @@ type Plugin struct {
forbiddenExtensions map[string]struct{}
// opentelemetry
prop propagation.TextMapPropagator

gzip bool
Copy link
Member

@rustatian rustatian Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

option without explanation comment. Also I think this is a bad name for the option, because what a gzip=true should do in the gzip middleware?

}

// Init must return configure service and return true if the service hasStatus enabled. Must return error in case of
Expand Down Expand Up @@ -79,6 +87,7 @@ func (s *Plugin) Init(cfg Configurer, log Logger) error {

s.log = log.NamedLogger(PluginName)
s.root = http.Dir(s.cfg.Dir)
s.gzip = s.cfg.Gzip

// init forbidden
for i := 0; i < len(s.cfg.Forbid); i++ {
Expand Down Expand Up @@ -225,11 +234,78 @@ func (s *Plugin) Middleware(next http.Handler) http.Handler { //nolint:gocognit,
}
}

if s.gzip && r.Header.Get("Accept-Encoding") == "gzip" {
s.log.Debug("gzip compression requested")
cf, err := s.compressContent(fp, f)
if err != nil {
s.log.Error("failed to compress content", zap.Error(err))
http.Error(w, "Internal Server Error", http.StatusInternalServerError)
return
}
err = f.Close()
if err != nil {
s.log.Error("failed to close file", zap.Error(err))
}
f = cf
Copy link
Member

@rustatian rustatian Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what is that. You're closing a file and then reassign another file to the closed one to be closed by the initial defer?

// No new defer to close needed for cf. f is already closed, previous defer exist
w.Header().Set("Content-Encoding", "gzip")
}
Copy link

@coderabbitai coderabbitai bot Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve gzip compression handling.

Two issues need to be addressed:

  1. The Accept-Encoding check is too strict. Browsers can send multiple encodings.
  2. Missing check for already compressed content types (e.g., images, videos).

Apply this diff to fix the issues:

-        if s.gzip && r.Header.Get("Accept-Encoding") == "gzip" {
+        if s.gzip && strings.Contains(r.Header.Get("Accept-Encoding"), "gzip") {
+            // Skip compression for already compressed formats
+            contentType := http.DetectContentType(make([]byte, 512))
+            if strings.Contains(contentType, "image/") ||
+               strings.Contains(contentType, "video/") ||
+               strings.Contains(contentType, "audio/") {
+                s.log.Debug("skipping compression for already compressed content", 
+                    zap.String("type", contentType))
+                http.ServeContent(w, r, finfo.Name(), finfo.ModTime(), f)
+                return
+            }

Committable suggestion skipped: line range outside the PR's diff.


// we passed all checks - serve the file
http.ServeContent(w, r, finfo.Name(), finfo.ModTime(), f)
})
}

func (s *Plugin) createCompressedFile(originalFile http.File, compressedPath string) error {
cacheMutex.Lock()
Copy link
Member

@rustatian rustatian Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No globals

defer cacheMutex.Unlock()

// We check again to avoid a race between threads.
if _, err := os.Stat(compressedPath); err == nil {
return nil
}
compressedFile, err := os.Create(s.cfg.Dir + compressedPath)
if err != nil {
return err
}
defer func() {
Copy link
Member

@rustatian rustatian Jan 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too many unnecessarry defers. There're no errors returning by another function calls, to overload a stack with 2 defers in a row.

err := compressedFile.Close()
if err != nil {
s.log.Error("failed to close file", zap.Error(err))
}
}()

gz := gzip.NewWriter(compressedFile)
defer func() {
Copy link
Member

@rustatian rustatian Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

second unnecessary defer

err := gz.Close()
if err != nil {
s.log.Error("failed to close file", zap.Error(err))
}
}()

_, err = io.Copy(gz, originalFile)
return err
}
Copy link

@coderabbitai coderabbitai bot Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Address security and resource management concerns.

Several issues need attention:

  1. No cleanup of partially written files on error
  2. Missing maximum file size check
  3. Potential path traversal vulnerability

Apply these improvements:

 func (s *Plugin) createCompressedFile(originalFile http.File, compressedPath string) error {
+    // Prevent path traversal
+    if !strings.HasPrefix(compressedPath, "/") {
+        compressedPath = "/" + compressedPath
+    }
+    cleanPath := path.Clean(compressedPath)
+    if !strings.HasPrefix(cleanPath, "/") {
+        return fmt.Errorf("invalid path")
+    }
+
     cacheMutex.Lock()
     defer cacheMutex.Unlock()
 
-    compressedFile, err := os.Create(s.cfg.Dir + compressedPath)
+    tempFile, err := os.CreateTemp(s.cfg.Dir, "gzip-*.tmp")
     if err != nil {
         return err
     }
+    tempName := tempFile.Name()
     defer func() {
-        err := compressedFile.Close()
+        err := tempFile.Close()
         if err != nil {
             s.log.Error("failed to close file", zap.Error(err))
         }
+        if err != nil {
+            _ = os.Remove(tempName)
+        }
     }()
 
-    gz := gzip.NewWriter(compressedFile)
+    // Limit the size of compressed files
+    maxSize := int64(10 * 1024 * 1024) // 10MB
+    limitedReader := io.LimitReader(originalFile, maxSize)
+
+    gz := gzip.NewWriter(tempFile)
     defer func() {
         err := gz.Close()
         if err != nil {
             s.log.Error("failed to close file", zap.Error(err))
         }
     }()
 
-    _, err = io.Copy(gz, originalFile)
-    return err
+    _, err = io.Copy(gz, limitedReader)
+    if err != nil {
+        return err
+    }
+
+    // Atomically rename the temp file to the final compressed file
+    return os.Rename(tempName, s.cfg.Dir+cleanPath)
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (s *Plugin) createCompressedFile(originalFile http.File, compressedPath string) error {
cacheMutex.Lock()
defer cacheMutex.Unlock()
// We check again to avoid a race between threads.
if _, err := os.Stat(compressedPath); err == nil {
return nil
}
compressedFile, err := os.Create(s.cfg.Dir + compressedPath)
if err != nil {
return err
}
defer func() {
err := compressedFile.Close()
if err != nil {
s.log.Error("failed to close file", zap.Error(err))
}
}()
gz := gzip.NewWriter(compressedFile)
defer func() {
err := gz.Close()
if err != nil {
s.log.Error("failed to close file", zap.Error(err))
}
}()
_, err = io.Copy(gz, originalFile)
return err
}
func (s *Plugin) createCompressedFile(originalFile http.File, compressedPath string) error {
// Prevent path traversal
if !strings.HasPrefix(compressedPath, "/") {
compressedPath = "/" + compressedPath
}
cleanPath := path.Clean(compressedPath)
if !strings.HasPrefix(cleanPath, "/") {
return fmt.Errorf("invalid path")
}
cacheMutex.Lock()
defer cacheMutex.Unlock()
// We check again to avoid a race between threads.
if _, err := os.Stat(compressedPath); err == nil {
return nil
}
tempFile, err := os.CreateTemp(s.cfg.Dir, "gzip-*.tmp")
if err != nil {
return err
}
tempName := tempFile.Name()
defer func() {
err := tempFile.Close()
if err != nil {
s.log.Error("failed to close file", zap.Error(err))
}
if err != nil {
_ = os.Remove(tempName)
}
}()
// Limit the size of compressed files
maxSize := int64(10 * 1024 * 1024) // 10MB
limitedReader := io.LimitReader(originalFile, maxSize)
gz := gzip.NewWriter(tempFile)
defer func() {
err := gz.Close()
if err != nil {
s.log.Error("failed to close file", zap.Error(err))
}
}()
_, err = io.Copy(gz, limitedReader)
if err != nil {
return err
}
// Atomically rename the temp file to the final compressed file
return os.Rename(tempName, s.cfg.Dir+cleanPath)
}


func (s *Plugin) compressContent(originalFilePath string, originalFile http.File) (http.File, error) {
compressedFilePath := originalFilePath + ".gz"

if _, err := os.Stat(compressedFilePath); os.IsNotExist(err) {
if err := s.createCompressedFile(originalFile, compressedFilePath); err != nil {
s.log.Debug("Error creating compressed file: ", zap.Error(err))
return nil, err
}
}

compressedFile, err := s.root.Open(compressedFilePath)
if err != nil {
s.log.Debug("Unable to open compressed file: ", zap.Error(err))
return nil, err
}

return compressedFile, nil
}

func bytesToStr(data []byte) string {
if len(data) == 0 {
return ""
Expand Down