Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
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
4 changes: 2 additions & 2 deletions sei-cosmos/server/api/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,12 +116,12 @@ func (s *Server) Start(cfg config.Config, apiMetrics *telemetry.Metrics) error {
if cfg.API.EnableUnsafeCORS {
allowAllCORS := handlers.CORS(handlers.AllowedHeaders([]string{"Content-Type"}))
s.mtx.Unlock()
return tmrpcserver.Serve(context.Background(), s.listener, allowAllCORS(h), tmCfg)
return tmrpcserver.Serve(context.Background(), s.listener, tmrpcserver.NewGzipHandler(allowAllCORS(h)), tmCfg)
}

logger.Info("starting API server...")
s.mtx.Unlock()
return tmrpcserver.Serve(context.Background(), s.listener, s.Router, tmCfg)
return tmrpcserver.Serve(context.Background(), s.listener, tmrpcserver.NewGzipHandler(s.Router), tmCfg)
}

// Close closes the API server.
Expand Down
2 changes: 1 addition & 1 deletion sei-tendermint/internal/inspect/rpc/rpc.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ func Handler(rpcConfig *config.RPCConfig, routes core.RoutesMap) http.Handler {
if rpcConfig.IsCorsEnabled() {
rootHandler = addCORSHandler(rpcConfig, mux)
}
return rootHandler
return server.NewGzipHandler(rootHandler)
}

func addCORSHandler(rpcConfig *config.RPCConfig, h http.Handler) http.Handler {
Expand Down
1 change: 1 addition & 0 deletions sei-tendermint/internal/rpc/core/env.go
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,7 @@ func (env *Environment) StartService(ctx context.Context, conf *config.Config) (
})
rootHandler = corsMiddleware.Handler(mux)
}
rootHandler = rpcserver.NewGzipHandler(rootHandler)
if conf.RPC.IsTLSEnabled() {
go func() {
if err := rpcserver.ServeTLS(
Expand Down
235 changes: 235 additions & 0 deletions sei-tendermint/rpc/jsonrpc/server/gzip.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,235 @@
package server

import (
"bufio"
"compress/gzip"
"fmt"
"io"
"net"
"net/http"
"strconv"
"strings"
"sync"
)

const minCompressBytes = 1024

var gzPool = sync.Pool{
New: func() any {
w, _ := gzip.NewWriterLevel(io.Discard, gzip.BestSpeed)
return w
Comment thread
cursor[bot] marked this conversation as resolved.
},
}

type gzipResponseWriter struct {
resp http.ResponseWriter

gz *gzip.Writer
contentLength uint64
hasLength bool
inited bool
}

func (w *gzipResponseWriter) init() {
if w.inited {
return
}
w.inited = true

hdr := w.resp.Header()
length := hdr.Get("content-length")
if len(length) > 0 {
if n, err := strconv.ParseUint(length, 10, 64); err == nil {
w.hasLength = true
w.contentLength = n
}
}

// Skip compression if the inner handler already encoded the response, opted
// out via Transfer-Encoding: identity, or emitted a byte range whose
// Content-Range offsets describe the uncompressed body.
if hdr.Get("content-encoding") != "" || hdr.Get("transfer-encoding") == "identity" ||
hdr.Get("content-range") != "" {
return
}

// Skip compression for small responses with a known Content-Length; below
// the threshold the gzip overhead outweighs the savings and the CPU cost
// per byte is not worth it for unauthenticated callers.
if w.hasLength && w.contentLength < minCompressBytes {
return
}

w.gz = gzPool.Get().(*gzip.Writer)

Check warning on line 63 in sei-tendermint/rpc/jsonrpc/server/gzip.go

View check run for this annotation

Claude / Claude Code Review

minCompressBytes threshold never triggers for the main RPC handlers

The `minCompressBytes` size-skip safeguard in `gzipResponseWriter.init()` never triggers for the main Tendermint JSON-RPC handlers, because `writeHTTPResponse`/`writeRPCResponse` (sei-tendermint/rpc/jsonrpc/server/http_server.go) call `WriteHeader` then `Write` without ever setting `Content-Length` — so `init()` runs while `hasLength` is still false and every response, including tiny ones, gets gzip-compressed. This makes the added CPU-amplification guard effectively dead code for the primary RP
Comment on lines +41 to +63

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 The minCompressBytes size-skip safeguard in gzipResponseWriter.init() never triggers for the main Tendermint JSON-RPC handlers, because writeHTTPResponse/writeRPCResponse (sei-tendermint/rpc/jsonrpc/server/http_server.go) call WriteHeader then Write without ever setting Content-Length — so init() runs while hasLength is still false and every response, including tiny ones, gets gzip-compressed. This makes the added CPU-amplification guard effectively dead code for the primary RPC traffic it was meant to protect (it does still work for handlers that explicitly set Content-Length, e.g. the swagger http.FileServer path).

Extended reasoning...

What the bug is. gzipResponseWriter.init() (gzip.go:41-63) is supposed to skip compression for small responses via: if w.hasLength && w.contentLength < minCompressBytes { return }. hasLength/contentLength are only populated a few lines earlier from hdr.Get(\"content-length\"). If that header is empty at the time init() runs, hasLength stays false and the size-skip branch can never be taken — the writer falls through and always compresses.

The code path that triggers it. The two real call sites for the Tendermint JSON-RPC server are writeHTTPResponse (http_server.go:144-165) and writeRPCResponse (http_server.go:173-196). Both do exactly:

w.Header().Set(\"Content-Type\", \"application/json\")
w.WriteHeader(statusCode)
w.Write(body)

Neither sets Content-Length anywhere. gzipResponseWriter.WriteHeader invokes init() immediately (for any non-bodyless status), and at that point w.resp.Header() genuinely has no content-length key — net/http's own auto-computed Content-Length (when the whole body fits in one buffered write) is only applied by the standard library at a layer below this wrapper, and only after the handler returns, so this wrapper's init() never observes it. I verified this directly against the current http_server.go in the diff/tree: both functions set only Content-Type, call WriteHeader, then Write, with no Content-Length anywhere in between.

Why nothing else prevents it. The size guard is genuinely conditioned on hasLength, and there's no fallback that infers size from the body slice length that's about to be written in Write() — by the time Write() is called, init() (and the compress/no-compress decision) has already run and been locked in via WriteHeader. So for this specific call pattern (WriteHeader before Write, no explicit Content-Length), the threshold is structurally unreachable.

Concrete proof. Consider a request to /status with Accept-Encoding: gzip, returning a body like {\"jsonrpc\":\"2.0\",\"id\":1,\"result\":{...}} that happens to be 40 bytes:

  1. NewGzipHandler sees acceptsGzip(r) == true and no websocket upgrade, so it wraps w in a gzipResponseWriter.
  2. writeHTTPResponse runs: w.Header().Set(\"Content-Type\", ...), then w.WriteHeader(200).
  3. Inside WriteHeader, status 200 isn't in the bodyless set, so w.init() runs. hdr.Get(\"content-length\") is \"\" (never set) → hasLength = false. The size-skip branch if w.hasLength && w.contentLength < minCompressBytes evaluates to if false && ... → false → not taken. w.gz is allocated from the pool and Content-Encoding: gzip is set.
  4. w.Write(body) (40 bytes) goes through w.gz.Write(...), gzip-compressing a 40-byte payload (which nets out larger on the wire than the original once gzip's ~20-byte header/footer overhead is added), purely for CPU cost with no size benefit.

This holds for every JSON-RPC response through this path, regardless of size — the design intent (skip compressing small bodies) never activates for the traffic it was presumably added to protect; it only works for handlers elsewhere (e.g. the Cosmos API's swagger http.FileServer) that do set Content-Length before/without an explicit early WriteHeader.

Impact and severity. This is not a correctness bug — responses remain valid, complete gzip streams and clients decode them fine. The practical cost is wasted CPU per tiny request and ~20 bytes of gzip framing overhead on payloads that gain nothing from compression. Also, the writer pool uses gzip.NewWriterLevel(io.Discard, gzip.BestSpeed) (level 1), not the default level-6 that an earlier review comment assumed, so the actual CPU cost per byte is low — this meaningfully caps the amplification concern. Given that, this doesn't warrant blocking merge; it's a nit worth fixing so the safeguard actually does what its name implies.

How to fix. Determine hasLength/contentLength from the byte slice passed to the first Write() call when Content-Length hasn't been set explicitly (i.e., defer the compress/no-compress decision until the first Write, using len(b) as a stand-in for total size when the handler is known to write once), or have writeHTTPResponse/writeRPCResponse set Content-Length explicitly before calling WriteHeader (safe here since amir-deris already confirmed on the PR thread that every handler in this server marshals to a full byte slice and writes once, never streaming).

w.gz.Reset(w.resp)
hdr.Del("content-length")
hdr.Set("content-encoding", "gzip")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[nit] Setting Content-Encoding: gzip before the first write disables net/http's automatic Content-Type sniffing: with no explicit Content-Type, the server sniffs the compressed bytes and emits Content-Type: application/x-gzip instead of the real (uncompressed) type. Impact is low here because the Tendermint/Cosmos JSON-RPC handlers set Content-Type explicitly, but any handler relying on sniffing would be affected. Consider capturing/deriving Content-Type from the uncompressed bytes before enabling gzip. (Raised by Codex.)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[suggestion] Content-Type is not preserved before compression. Once Content-Encoding: gzip is set here and Write sends gzip bytes to the underlying ResponseWriter, net/http's content sniffing (triggered when the inner handler didn't set Content-Type explicitly) inspects the gzip magic bytes and emits Content-Type: application/x-gzip. Standard gzip middleware captures/sets the content type from the first pre-compression write (or calls http.DetectContentType on the uncompressed bytes) to avoid this. The wired JSON-RPC and gRPC-gateway handlers set application/json explicitly so impact is limited, but any handler relying on auto-detection will now serve a wrong Content-Type. (Raised by Codex P2.)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[suggestion] Codex P2: content-type sniffing. If the inner handler writes a body without setting Content-Type, net/http sniffs the first bytes written to determine it — which will now be the gzip header, yielding Content-Type: application/x-gzip instead of the real type. Consider sniffing/preserving the uncompressed Content-Type (e.g. via http.DetectContentType on the first chunk) before the first compressed write. In practice the handlers wired up here (JSON-RPC writeHTTPResponse, gRPC-gateway, FileServer) all set Content-Type explicitly, so this is defensive rather than an active bug — hence a suggestion, but worth hardening since the middleware is generic.

}
Comment thread
cursor[bot] marked this conversation as resolved.

func (w *gzipResponseWriter) Header() http.Header {
return w.resp.Header()
}

func (w *gzipResponseWriter) WriteHeader(status int) {
// Bodyless responses must not be compressed — gzip would write a stream
// terminator into what must be an empty body (RFC 7230 §3.3). 206 responses
// carry Content-Range offsets that describe the uncompressed body.
if status == http.StatusNoContent || status == http.StatusNotModified ||
Comment thread
amir-deris marked this conversation as resolved.
status == http.StatusPartialContent || (status >= 100 && status <= 199) {
w.inited = true // skip gzip setup
w.resp.WriteHeader(status)
return
}
w.init()
w.resp.WriteHeader(status)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

1xx responses disable later compression

Low Severity

WriteHeader treats 1xx statuses like final bodyless responses and sets inited permanently. Interim 1xx writes must not lock out compression, but a later final WriteHeader/Write then skips init, so an otherwise compressible response stays identity-encoded.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit cb74faa. Configure here.


func (w *gzipResponseWriter) Write(b []byte) (int, error) {
w.init()

if w.gz == nil {
return w.resp.Write(b)
}

return w.gz.Write(b)
}

func (w *gzipResponseWriter) Flush() {
Comment thread
amir-deris marked this conversation as resolved.
// Decide the encoding before headers are committed: a bare Flush() before
// any Write() would otherwise commit identity-encoded headers, after which a
// later Write() sets Content-Encoding: gzip too late and emits gzip bytes
// under an identity encoding.
w.init()
if w.gz != nil {
_ = w.gz.Flush()
}
if f, ok := w.resp.(http.Flusher); ok {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[suggestion] Codex P1: gzipResponseWriter statically implements http.Flusher, so any wrapped handler sees w.(http.Flusher) == true and believes a flush reached the network — but Flush() only forwards when the underlying writer is a Flusher. In both wired-in paths the underlying writer is sei-tendermint's statusWriter (from recoverAndLogHandler), which embeds only http.ResponseWriter+http.Hijacker and is NOT a Flusher, so gz.Flush() runs but the network flush is a silent no-op. Note this is not a regression: statusWriter already masked net/http's Flusher before this PR, so streaming through this server never flushed incrementally, and the final response is still a complete/valid gzip stream. The only new wrinkle is falsely advertising flush capability. Fine to merge as-is; flagging so it's a conscious choice.

f.Flush()
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Flush blocked by statusWriter wrapper

Low Severity

gzipResponseWriter.Flush type-asserts w.resp as http.Flusher, but in every Serve/ServeTLS path resp is statusWriter, which embeds the http.ResponseWriter interface and does not implement Flusher. Compressed bytes therefore sit in the server buffer until the handler returns, despite the streaming-flush test and comments claiming incremental delivery works.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit affed4d. Configure here.


// Hijack implements http.Hijacker by forwarding to the inner ResponseWriter.
// The gzip writer is closed first so the hijacked connection starts clean.
func (w *gzipResponseWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) {
h, ok := w.resp.(http.Hijacker)
if !ok {
return nil, nil, fmt.Errorf("gzip middleware: underlying ResponseWriter does not implement http.Hijacker")
}
w.close()
return h.Hijack()
}

func (w *gzipResponseWriter) close() {
if w.gz == nil {
return
}
_ = w.gz.Close()
gzPool.Put(w.gz)
w.gz = nil
}

// abort returns the writer to the pool without emitting the gzip footer, used
// when a panic unwinds mid-response so we don't frame a valid gzip stream
// ahead of the plaintext 500 body appended by the recovery handler.
func (w *gzipResponseWriter) abort() {
if w.gz == nil {
return
}
gzPool.Put(w.gz)
w.gz = nil
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Panic corrupts gzip-encoded responses

Medium Severity

abort returns the gzip writer to the pool without clearing Content-Encoding, then re-panics into recoverAndLogHandler, which writes a plaintext 500 directly to the inner ResponseWriter. After init has run, clients still see Content-Encoding: gzip with a non-gzip body, so decompression fails.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit affed4d. Configure here.


// acceptsGzip reports whether the request advertises gzip encoding support,
// respecting q-values and the "*" wildcard per RFC 7231 §5.3.4.
func acceptsGzip(r *http.Request) bool {
gzipQ := -1.0
starQ := -1.0
for _, field := range r.Header["Accept-Encoding"] {
for part := range strings.SplitSeq(field, ",") {
part = strings.TrimSpace(part)
coding, params, _ := strings.Cut(part, ";")
coding = strings.ToLower(strings.TrimSpace(coding))
q := 1.0
for p := range strings.SplitSeq(params, ";") {
p = strings.TrimSpace(p)
if k, v, ok := strings.Cut(p, "="); ok && strings.ToLower(strings.TrimSpace(k)) == "q" {
if f, err := strconv.ParseFloat(strings.TrimSpace(v), 64); err == nil {
q = f
}
}
}
switch coding {
case "gzip":
gzipQ = q
case "*":
starQ = q
}
}
}
if gzipQ >= 0 {
return gzipQ > 0
}
if starQ >= 0 {
return starQ > 0
}
return false
}

// ensureVaryAcceptEncoding appends Accept-Encoding to the Vary header exactly
// once, deduplicating against any value already set by the inner handler or
// CORS middleware. Vary: * already implies Accept-Encoding, so it is left as-is.
func ensureVaryAcceptEncoding(h http.Header) {
existing := h.Get("Vary")
for part := range strings.SplitSeq(existing, ",") {
v := strings.TrimSpace(part)
if strings.EqualFold(v, "Accept-Encoding") || v == "*" {
return
}
}
if existing == "" {
h.Set("Vary", "Accept-Encoding")
} else {
h.Set("Vary", existing+", Accept-Encoding")
}
}

// hasUpgradeToken reports whether the Upgrade header contains token (RFC 7230
// §6.7 permits a comma-separated list; each token is matched case-insensitively).
func hasUpgradeToken(r *http.Request, token string) bool {
for _, field := range r.Header["Upgrade"] {
for part := range strings.SplitSeq(field, ",") {
if strings.EqualFold(strings.TrimSpace(part), token) {
return true
}
}
}
return false
}

// NewGzipHandler wraps next with gzip response compression. Compression is
// skipped for clients that do not advertise gzip support via Accept-Encoding.
// WebSocket upgrades are passed through unmodified; gzipResponseWriter also
// implements http.Hijacker as defense-in-depth for non-canonical Upgrade values.
func NewGzipHandler(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// Vary must be set on every response — compressed or not — so that CDN
// caches key on Accept-Encoding and never serve a wrong variant.
ensureVaryAcceptEncoding(w.Header())

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[suggestion] Vary: Accept-Encoding is set here before the inner handler runs. If a downstream handler or CORS middleware later calls w.Header().Set("Vary", ...) (rather than Add), it overwrites this value and drops Accept-Encoding. A CDN keyed only on the remaining Vary token could then serve a gzip-encoded variant to a client that never sent Accept-Encoding: gzip. Of the two wirings, rs/cors (env.go) uses Header().Add("Vary", ...) and is safe, but gorilla handlers.CORS (sei-cosmos server.go) and any future handler using Set are not guaranteed to be. Safer to (re)ensure the Vary token at header-commit time (in the wrapper's init()/WriteHeader) as well as up front. (Raised by Codex.)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[nit] Vary: Accept-Encoding is set at handler entry, before the inner handler runs. An inner handler that does Header().Set("Vary", ...) would clobber it, letting a cache serve a compressed variant to a non-gzip client. The two CORS layers actually wired here (rs/cors and gorilla handlers.CORS) use Header().Add("Vary", ...), so the current wiring is safe — but this is fragile. Consider re-asserting Vary in init()/WriteHeader (when headers are committed) instead of at entry. (Raised by Codex P1; low practical impact given current wiring.)


if !acceptsGzip(r) || hasUpgradeToken(r, "websocket") {
next.ServeHTTP(w, r)
return
}

wrapper := &gzipResponseWriter{resp: w}
defer func() {
if p := recover(); p != nil {
wrapper.abort()
panic(p)
}
wrapper.close()
}()

next.ServeHTTP(wrapper, r)
Comment thread
cursor[bot] marked this conversation as resolved.
})
}
Loading
Loading