From 0e38b0d11c264a801d72946018071d3905bac21f Mon Sep 17 00:00:00 2001 From: Valery Piashchynski Date: Fri, 29 May 2026 14:25:17 +0200 Subject: [PATCH 1/3] chore: simplify, modernize (Go 1.26), update deps - attributes: guard Get/Set type-assertion against non-attrs stored types (R-High) - handler/request: strings.NewReplacer for CRLF stripping; drop redundant ContainsRune pre-check (M) - handler/uploads: errors.Is(fs.ErrNotExist) instead of os.IsNotExist (M) - middleware/log: remove dead wrapper.data field; if/else over bool switch; reuse start time instead of two time.Now() calls; NewReplacer for CRLF (S+M) - middleware/maxRequest: assign r.Body directly instead of r.Clone (S) - plugin: Go 1.22 range-over-value for server loop; drop redundant r.Body.Close; call pool.Destroy via interface (S+M+R-Low) - servers/https/config: errors.Is(fs.ErrNotExist) in three stat checks (M) - tlsconf: remove dead TLS 1.3 cipher IDs from CipherSuites (silently ignored by Go) (R-Med+S) - acme: drop explicit zero-value struct fields (M) --- acme/acme.go | 32 ++++++-------------- attributes/attributes.go | 12 ++++++-- handler/request.go | 14 +++------ handler/uploads.go | 8 ++--- middleware/log.go | 62 +++++++++++++++++--------------------- middleware/maxRequest.go | 8 ++--- plugin.go | 22 +++----------- servers/https/config.go | 22 ++++++++------ tlsconf/default_tlsconf.go | 26 ++++------------ 9 files changed, 80 insertions(+), 126 deletions(-) diff --git a/acme/acme.go b/acme/acme.go index 98e0cd6..ececdc8 100644 --- a/acme/acme.go +++ b/acme/acme.go @@ -20,36 +20,24 @@ func IssueCertificates(cacheDir, email, challengeType string, domains []string, cache := certmagic.NewCache(certmagic.CacheOptions{ GetConfigForCert: func(_ certmagic.Certificate) (*certmagic.Config, error) { return &certmagic.Config{ - RenewalWindowRatio: 0, - MustStaple: false, - OCSP: certmagic.OCSPConfig{}, - Storage: &certmagic.FileStorage{Path: cacheDir}, + Storage: &certmagic.FileStorage{Path: cacheDir}, }, nil }, - OCSPCheckInterval: 0, - RenewCheckInterval: 0, - Capacity: 0, }) cfg := certmagic.New(cache, certmagic.Config{ - RenewalWindowRatio: 0, - MustStaple: false, - OCSP: certmagic.OCSPConfig{}, - Storage: &certmagic.FileStorage{Path: cacheDir}, + Storage: &certmagic.FileStorage{Path: cacheDir}, }) myAcme := certmagic.NewACMEIssuer(cfg, certmagic.ACMEIssuer{ - CA: certmagic.LetsEncryptProductionCA, - TestCA: certmagic.LetsEncryptStagingCA, - Email: email, - Agreed: true, - DisableHTTPChallenge: false, - DisableTLSALPNChallenge: false, - ListenHost: "0.0.0.0", - AltHTTPPort: altHTTPPort, - AltTLSALPNPort: altTLSAlpnPort, - CertObtainTimeout: time.Second * 240, - PreferredChains: certmagic.ChainPreference{}, + CA: certmagic.LetsEncryptProductionCA, + TestCA: certmagic.LetsEncryptStagingCA, + Email: email, + Agreed: true, + ListenHost: "0.0.0.0", + AltHTTPPort: altHTTPPort, + AltTLSALPNPort: altTLSAlpnPort, + CertObtainTimeout: time.Second * 240, }) if !useProduction { diff --git a/attributes/attributes.go b/attributes/attributes.go index 0e41ed7..9208d6a 100644 --- a/attributes/attributes.go +++ b/attributes/attributes.go @@ -72,7 +72,11 @@ func Get(r *http.Request, key string) any { return nil } - return v.(attrs).get(key) + a, ok := v.(attrs) + if !ok { + return nil + } + return a.get(key) } // Set sets the key to value. It replaces any existing @@ -83,7 +87,11 @@ func Set(r *http.Request, key string, value string) error { return errors.New("unable to find `psr:attributes` context key") } - v.(attrs).set(key, value) + a, ok := v.(attrs) + if !ok { + return errors.New("unable to find `psr:attributes` context key") + } + a.set(key, value) return nil } diff --git a/handler/request.go b/handler/request.go index c5e7f7a..8c16717 100644 --- a/handler/request.go +++ b/handler/request.go @@ -24,10 +24,6 @@ const ( // FetchIP extracts the client IP from net/http's RemoteAddr ("host:port" // or a bare IP). Returns the empty string for unparseable input. func FetchIP(pair string, log *slog.Logger) string { - if !strings.ContainsRune(pair, ':') { - return pair - } - addr, _, err := net.SplitHostPort(pair) if err == nil { return addr @@ -41,12 +37,12 @@ func FetchIP(pair string, log *slog.Logger) string { return ip.String() } +var crlfReplacer = strings.NewReplacer("\n", "", "\r", "") //nolint:gochecknoglobals + // URI returns the fully-qualified request URI, stripping CR/LF to prevent // header smuggling via the URL. func URI(r *http.Request) string { - uri := r.URL.String() - uri = strings.ReplaceAll(uri, "\n", "") - uri = strings.ReplaceAll(uri, "\r", "") + uri := crlfReplacer.Replace(r.URL.String()) if r.URL.Host != "" { return uri @@ -88,9 +84,7 @@ func extractCookies(r *http.Request) map[string]string { // cleanRawQuery strips CR/LF from the URL raw query before exposing it to PHP. func cleanRawQuery(q string) string { - q = strings.ReplaceAll(q, "\n", "") - q = strings.ReplaceAll(q, "\r", "") - return q + return crlfReplacer.Replace(q) } // populateBody fills req.Body / req.Parsed based on the request content-type. diff --git a/handler/uploads.go b/handler/uploads.go index 83c2dd5..46a3532 100644 --- a/handler/uploads.go +++ b/handler/uploads.go @@ -2,7 +2,9 @@ package handler import ( "encoding/json" + "errors" "io" + "io/fs" "log/slog" "mime/multipart" "os" @@ -166,8 +168,6 @@ func (f *FileUpload) Open(dir string, forbid, allow map[string]struct{}) error { // exists if file exists. func exists(path string) bool { // path is RR-generated TempFilename, not user-controlled. - if _, err := os.Stat(path); os.IsNotExist(err) { //nolint:gosec // G703 - return false - } - return true + _, err := os.Stat(path) //nolint:gosec // G703 + return !errors.Is(err, fs.ErrNotExist) } diff --git a/middleware/log.go b/middleware/log.go index d63c893..8d17896 100644 --- a/middleware/log.go +++ b/middleware/log.go @@ -16,6 +16,8 @@ import ( var _ io.ReadCloser = (*wrapper)(nil) var _ http.ResponseWriter = (*wrapper)(nil) +var crlfReplacer = strings.NewReplacer("\n", "", "\r", "") //nolint:gochecknoglobals + type wrapper struct { io.ReadCloser read int @@ -27,7 +29,6 @@ type wrapper struct { w http.ResponseWriter code int - data []byte } func (w *wrapper) Read(b []byte) (int, error) { @@ -85,7 +86,6 @@ func (w *wrapper) reset() { w.wc = false w.write = 0 w.w = nil - w.data = nil w.ReadCloser = nil } @@ -128,8 +128,7 @@ func (l *lm) Log(next http.Handler, accessLogs bool) http.Handler { } func (l *lm) writeLog(accessLog bool, r *http.Request, bw *wrapper, start time.Time) { - switch accessLog { - case false: + if !accessLog { l.log.Info("http log", "status", bw.code, "method", r.Method, @@ -140,38 +139,31 @@ func (l *lm) writeLog(accessLog bool, r *http.Request, bw *wrapper, start time.T "write_bytes", bw.write, "start", start, "elapsed", time.Since(start).Milliseconds()) - case true: - // external/cwe/cwe-117 - usrA := r.UserAgent() - usrA = strings.ReplaceAll(usrA, "\n", "") - usrA = strings.ReplaceAll(usrA, "\r", "") - - rfr := r.Referer() - rfr = strings.ReplaceAll(rfr, "\n", "") - rfr = strings.ReplaceAll(rfr, "\r", "") - - rq := r.URL.RawQuery - rq = strings.ReplaceAll(rq, "\n", "") - rq = strings.ReplaceAll(rq, "\r", "") - - l.log.Info("http access log", - "read_bytes", bw.read, - "write_bytes", bw.write, - "status", bw.code, - "method", r.Method, - "URI", r.RequestURI, - "URL", r.URL.String(), - "remote_address", r.RemoteAddr, - "query", rq, - "content_len", r.ContentLength, - "host", r.Host, - "user_agent", usrA, - "referer", rfr, - "time_local", time.Now().Format("02/Jan/06:15:04:05 -0700"), - "request_time", time.Now(), - "start", start, - "elapsed", time.Since(start).Milliseconds()) + return } + + // external/cwe/cwe-117 + usrA := crlfReplacer.Replace(r.UserAgent()) + rfr := crlfReplacer.Replace(r.Referer()) + rq := crlfReplacer.Replace(r.URL.RawQuery) + + l.log.Info("http access log", + "read_bytes", bw.read, + "write_bytes", bw.write, + "status", bw.code, + "method", r.Method, + "URI", r.RequestURI, + "URL", r.URL.String(), + "remote_address", r.RemoteAddr, + "query", rq, + "content_len", r.ContentLength, + "host", r.Host, + "user_agent", usrA, + "referer", rfr, + "time_local", start.Format("02/Jan/06:15:04:05 -0700"), + "request_time", start, + "start", start, + "elapsed", time.Since(start).Milliseconds()) } func (l *lm) getW(w http.ResponseWriter) *wrapper { diff --git a/middleware/maxRequest.go b/middleware/maxRequest.go index 6288430..ef0f838 100644 --- a/middleware/maxRequest.go +++ b/middleware/maxRequest.go @@ -6,12 +6,8 @@ import ( func MaxRequestSize(next http.Handler, maxReqSize uint64) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // validating request size - - r2 := r.Clone(r.Context()) - r2.Body = http.MaxBytesReader(w, r2.Body, int64(maxReqSize)) //nolint:gosec - // use max_request_size limit in megabytes - next.ServeHTTP(w, r2) + r.Body = http.MaxBytesReader(w, r.Body, int64(maxReqSize)) //nolint:gosec + next.ServeHTTP(w, r) }) } diff --git a/plugin.go b/plugin.go index 40fdea4..26d76f8 100644 --- a/plugin.go +++ b/plugin.go @@ -17,7 +17,6 @@ import ( "github.com/roadrunner-server/http/v6/handler" "github.com/roadrunner-server/http/v6/proxy" "github.com/roadrunner-server/http/v6/servers" - "github.com/roadrunner-server/pool/v2/pool/static_pool" "github.com/roadrunner-server/pool/v2/state/process" "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" jprop "go.opentelemetry.io/contrib/propagators/jaeger" @@ -161,14 +160,12 @@ func (p *Plugin) Serve() chan error { p.applyBundledMiddleware() // start all servers - for i := range p.servers { - go func(idx int) { - errSt := p.servers[idx].Serve(p.mdwr, p.cfg.Middleware) - if errSt != nil { + for _, srv := range p.servers { + go func(s servers.InternalServer[any]) { + if errSt := s.Serve(p.mdwr, p.cfg.Middleware); errSt != nil { errCh <- errSt - return } - }(i) + }(srv) } return errCh @@ -201,14 +198,7 @@ func (p *Plugin) Stop(ctx context.Context) error { } if p.pool != nil { - switch pp := p.pool.(type) { - case *static_pool.Pool: - if pp != nil { - pp.Destroy(ctx) - } - default: - // pool is nil, nothing to do - } + p.pool.Destroy(ctx) } doneCh <- struct{}{} @@ -242,8 +232,6 @@ func (p *Plugin) ServeHTTP(w http.ResponseWriter, r *http.Request) { p.handler.ServeHTTP(w, r) p.mu.RUnlock() - _ = r.Body.Close() - if span != nil { span.End() } diff --git a/servers/https/config.go b/servers/https/config.go index 739b96c..e20c429 100644 --- a/servers/https/config.go +++ b/servers/https/config.go @@ -1,11 +1,13 @@ package https import ( + "errors" + "io/fs" "net" "os" "strconv" - "github.com/roadrunner-server/errors" + rrerrors "github.com/roadrunner-server/errors" "github.com/roadrunner-server/http/v6/acme" ) @@ -95,11 +97,11 @@ func (s *SSL) EnableACME() bool { } func (s *SSL) Valid() error { - const op = errors.Op("ssl_valid") + const op = rrerrors.Op("ssl_valid") host, portStr, err := net.SplitHostPort(s.Address) if err != nil { - return errors.E(op, err) + return rrerrors.E(op, err) } if host == "" { s.host = "127.0.0.1" @@ -108,23 +110,23 @@ func (s *SSL) Valid() error { } port, err := strconv.ParseUint(portStr, 10, 16) if err != nil { - return errors.E(op, err) + return rrerrors.E(op, err) } s.Port = int(port) // the user use they own certificates if s.Acme == nil { if _, err := os.Stat(s.Key); err != nil { - if os.IsNotExist(err) { - return errors.E(op, errors.Errorf("key file '%s' does not exists", s.Key)) + if errors.Is(err, fs.ErrNotExist) { + return rrerrors.E(op, rrerrors.Errorf("key file '%s' does not exists", s.Key)) } return err } if _, err := os.Stat(s.Cert); err != nil { - if os.IsNotExist(err) { - return errors.E(op, errors.Errorf("cert file '%s' does not exists", s.Cert)) + if errors.Is(err, fs.ErrNotExist) { + return rrerrors.E(op, rrerrors.Errorf("cert file '%s' does not exists", s.Cert)) } return err @@ -134,8 +136,8 @@ func (s *SSL) Valid() error { // RootCA is optional, but if provided - check it if s.RootCA != "" { if _, err := os.Stat(s.RootCA); err != nil { - if os.IsNotExist(err) { - return errors.E(op, errors.Errorf("root ca path provided, but path '%s' does not exists", s.RootCA)) + if errors.Is(err, fs.ErrNotExist) { + return rrerrors.E(op, rrerrors.Errorf("root ca path provided, but path '%s' does not exists", s.RootCA)) } return err } diff --git a/tlsconf/default_tlsconf.go b/tlsconf/default_tlsconf.go index 3b5d8de..cecad6b 100644 --- a/tlsconf/default_tlsconf.go +++ b/tlsconf/default_tlsconf.go @@ -7,9 +7,6 @@ import ( ) func DefaultTLSConfig() *tls.Config { - var topCipherSuites []uint16 - var defaultCipherSuitesTLS13 []uint16 - hasGCMAsmAMD64 := cpu.X86.HasAES && cpu.X86.HasPCLMULQDQ hasGCMAsmARM64 := cpu.ARM64.HasAES && cpu.ARM64.HasPMULL // Keep in sync with crypto/aes/cipher_s390x.go. @@ -17,10 +14,13 @@ func DefaultTLSConfig() *tls.Config { hasGCMAsm := hasGCMAsmAMD64 || hasGCMAsmARM64 || hasGCMAsmS390X + // CipherSuites only controls TLS 1.0–1.2 suites; Go selects TLS 1.3 suites + // automatically (adding them here is silently ignored). + var cipherSuites []uint16 if hasGCMAsm { // If AES-GCM hardware is provided then priorities AES-GCM // cipher suites. - topCipherSuites = []uint16{ + cipherSuites = []uint16{ tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, @@ -28,15 +28,10 @@ func DefaultTLSConfig() *tls.Config { tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305, tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305, } - defaultCipherSuitesTLS13 = []uint16{ - tls.TLS_AES_128_GCM_SHA256, - tls.TLS_CHACHA20_POLY1305_SHA256, - tls.TLS_AES_256_GCM_SHA384, - } } else { // Without AES-GCM hardware, we put the ChaCha20-Poly1305 // cipher suites first. - topCipherSuites = []uint16{ + cipherSuites = []uint16{ tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305, tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305, tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, @@ -44,17 +39,8 @@ func DefaultTLSConfig() *tls.Config { tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, } - defaultCipherSuitesTLS13 = []uint16{ - tls.TLS_CHACHA20_POLY1305_SHA256, - tls.TLS_AES_128_GCM_SHA256, - tls.TLS_AES_256_GCM_SHA384, - } } - defaultCipherSuites := make([]uint16, 0, 22) - defaultCipherSuites = append(defaultCipherSuites, topCipherSuites...) - defaultCipherSuites = append(defaultCipherSuites, defaultCipherSuitesTLS13...) - return &tls.Config{ CurvePreferences: []tls.CurveID{ tls.X25519, @@ -62,7 +48,7 @@ func DefaultTLSConfig() *tls.Config { tls.CurveP384, tls.CurveP521, }, - CipherSuites: defaultCipherSuites, + CipherSuites: cipherSuites, MinVersion: tls.VersionTLS12, } } From ba9070b07053597230c4fbe80248603f82f5d3f6 Mon Sep 17 00:00:00 2001 From: Valery Piashchynski Date: Fri, 29 May 2026 23:00:29 +0200 Subject: [PATCH 2/3] fix(http): drop package-level crlfReplacer globals Replace the two package-level strings.Replacer vars (+nolint:gochecknoglobals) with inline strings.ReplaceAll (request.go) and a small stripCRLF helper (middleware/log.go). No global state. --- handler/request.go | 6 ++---- middleware/log.go | 11 +++++++---- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/handler/request.go b/handler/request.go index 8c16717..6ac5801 100644 --- a/handler/request.go +++ b/handler/request.go @@ -37,12 +37,10 @@ func FetchIP(pair string, log *slog.Logger) string { return ip.String() } -var crlfReplacer = strings.NewReplacer("\n", "", "\r", "") //nolint:gochecknoglobals - // URI returns the fully-qualified request URI, stripping CR/LF to prevent // header smuggling via the URL. func URI(r *http.Request) string { - uri := crlfReplacer.Replace(r.URL.String()) + uri := strings.ReplaceAll(strings.ReplaceAll(r.URL.String(), "\n", ""), "\r", "") if r.URL.Host != "" { return uri @@ -84,7 +82,7 @@ func extractCookies(r *http.Request) map[string]string { // cleanRawQuery strips CR/LF from the URL raw query before exposing it to PHP. func cleanRawQuery(q string) string { - return crlfReplacer.Replace(q) + return strings.ReplaceAll(strings.ReplaceAll(q, "\n", ""), "\r", "") } // populateBody fills req.Body / req.Parsed based on the request content-type. diff --git a/middleware/log.go b/middleware/log.go index 8d17896..b1a265e 100644 --- a/middleware/log.go +++ b/middleware/log.go @@ -16,7 +16,10 @@ import ( var _ io.ReadCloser = (*wrapper)(nil) var _ http.ResponseWriter = (*wrapper)(nil) -var crlfReplacer = strings.NewReplacer("\n", "", "\r", "") //nolint:gochecknoglobals +// stripCRLF removes CR/LF to prevent log injection (CWE-117). +func stripCRLF(s string) string { + return strings.ReplaceAll(strings.ReplaceAll(s, "\n", ""), "\r", "") +} type wrapper struct { io.ReadCloser @@ -143,9 +146,9 @@ func (l *lm) writeLog(accessLog bool, r *http.Request, bw *wrapper, start time.T } // external/cwe/cwe-117 - usrA := crlfReplacer.Replace(r.UserAgent()) - rfr := crlfReplacer.Replace(r.Referer()) - rq := crlfReplacer.Replace(r.URL.RawQuery) + usrA := stripCRLF(r.UserAgent()) + rfr := stripCRLF(r.Referer()) + rq := stripCRLF(r.URL.RawQuery) l.log.Info("http access log", "read_bytes", bw.read, From e336bd5d6a3693ccca28473dd9cf39da33f060fa Mon Sep 17 00:00:00 2001 From: Valery Piashchynski Date: Fri, 29 May 2026 23:43:07 +0200 Subject: [PATCH 3/3] fix(http): avoid typed-nil pool panic on Serve failure; clarify attr error NewPool returns a concrete *static_pool.Pool, so assigning its result directly to the api.Pool interface field on the error path boxed a typed nil into p.pool. The simplified Stop guard (`if p.pool != nil`) then sees a non-nil interface and calls Destroy on the nil receiver, which panics (Destroy dereferences sp.log immediately). Assign to p.pool only when the returned pointer is non-nil so the interface never holds a typed nil and the existing guard stays correct. Also distinguish the wrong-type case in attributes.Set from the missing-key case with its own error message for easier diagnosis. --- attributes/attributes.go | 2 +- plugin.go | 10 ++++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/attributes/attributes.go b/attributes/attributes.go index 9208d6a..c6d2c39 100644 --- a/attributes/attributes.go +++ b/attributes/attributes.go @@ -89,7 +89,7 @@ func Set(r *http.Request, key string, value string) error { a, ok := v.(attrs) if !ok { - return errors.New("unable to find `psr:attributes` context key") + return errors.New("unexpected type stored under `psr:attributes` context key") } a.set(key, value) return nil diff --git a/plugin.go b/plugin.go index 26d76f8..496fe32 100644 --- a/plugin.go +++ b/plugin.go @@ -127,12 +127,18 @@ func (p *Plugin) Serve() chan error { p.mu.Lock() defer p.mu.Unlock() - var err error - p.pool, err = p.server.NewPool(context.Background(), p.cfg.Pool, map[string]string{RrMode: RrModeHTTP}, p.log) + // NewPool returns a concrete *static_pool.Pool; assign it to the api.Pool + // interface field only when non-nil so p.pool never holds a typed nil + // (which would make the p.pool != nil guard in Stop spuriously true and + // panic inside Destroy on a nil receiver). + np, err := p.server.NewPool(context.Background(), p.cfg.Pool, map[string]string{RrMode: RrModeHTTP}, p.log) if err != nil { errCh <- err return errCh } + if np != nil { + p.pool = np + } // request queue + worker-facing ConnectRPC server p.queue = proxy.NewQueue(p.cfg.Proxy.InboxSize)