Fix high-severity security vulnerabilities#4
Merged
Conversation
Defaulted() was auto-populating the Detail field from err.Error(), exposing raw database errors, file paths, and SQL fragments in HTTP responses. Removed the auto-population so only explicitly set Detail values reach clients. Also changed Instance from request.URL.String() (which includes query params that may contain tokens) to request.URL.Path. BREAKING CHANGE: Defaulted() no longer sets Detail from the wrapped error. Applications that relied on automatic Detail population must now set problem.Detail explicitly when creating Problem values.
handleError was called with the original ResponseWriter, bypassing hooks (BeforeWriteHeader, BeforeWrite) for error responses. This broke logger middleware status tracking and could produce corrupted responses when a handler partially wrote then returned an error. Now passes the wrapped writer to handleError. Also changed writeHeaderCalled from plain bool to sync/atomic.Bool to prevent data races under concurrent access patterns.
The framework provided no way to configure HTTP server timeouts. When used with http.ListenAndServe, all timeouts default to zero, enabling Slowloris and connection-exhaustion attacks. NewServer and NewServerWith create properly configured http.Server instances with secure defaults (ReadHeaderTimeout 10s, ReadTimeout 30s, WriteTimeout 60s, IdleTimeout 120s).
Applications had no built-in security headers, leaving them vulnerable to MIME sniffing, clickjacking, and protocol downgrade attacks. SecureHeaders sets X-Content-Type-Options, X-Frame-Options, Referrer-Policy, X-XSS-Protection, and Strict-Transport-Security with safe defaults. All headers are configurable via SecureHeadersWith.
No rate limiting existed, leaving auth endpoints vulnerable to brute force and the session layer to creation floods. RateLimit uses a per-key token bucket (golang.org/x/time/rate) with configurable request rate, burst size, and key extraction. Defaults to 10 req/s per IP with burst of 20.
Redirect() sets the Location header from a raw parameter with no validation, enabling open redirects via javascript:, data:, or external domain URLs when the value comes from user input. Added SafeRedirect that validates the URL is a relative path and added ErrUnsafeRedirect sentinel error. The original Redirect is preserved with a warning in its documentation.
Hooks() panics when the hooks middleware is missing, and MustSession and MustSessionKeyed panic without sessions. Since Recover middleware is opt-in, these panics crash the goroutine when used outside the framework handler chain. Added TryHooks as a non-panicking alternative and added WARNING docs to all Must*/panic functions directing users to safe alternatives.
Publish spawned a new goroutine per handler with no concurrency limit, allowing goroutine exhaustion under high throughput with slow handlers. Added a semaphore (default 1024 concurrent deliveries) and a sync.WaitGroup so Close waits for in-flight deliveries to complete instead of potentially leaking goroutines.
Default option structs (ServerOptions, SecureHeadersOptions, RateLimitOptions) are now package-level vars instead of functions, matching the stdlib pattern (http.DefaultClient, etc.). RateLimitWith is refactored to extract rateLimitRegistry and withDefaults methods, reducing cyclomatic complexity. NewServerWith uses the same withDefaults pattern for consistency.
Receivers use full descriptive names (options, registry, writer). Boolean fields and methods use is/has prefix (IsWriteHeaderCalled, isWriteHeaderCalled). Boolean functions use is prefix (isMatchingEvent, isMatchingEventParts).
Remove is/has prefix from boolean fields and methods to match Go stdlib style. Rename expiration to expiresAt for date fields. Use err instead of e for named error returns.
Remove is/has prefix from boolean fields (isClosed, isFirst). Replace single-letter variables with descriptive names: q to stmt in database methods, s to session in middleware, k/v to cacheKey/value in cache driver, k/d to name/fallback in request helpers, r to recovered in panic recovery, b to body in request body helpers. Restructure errors.As calls with if-scoped target variables.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses 9 high-severity security vulnerabilities identified during the comprehensive security audit (findings #4-#12 from the audit report). This PR builds on top of the critical fixes in #3.
Information Leakage Prevention (problem)
Defaulted()no longer auto-populatesDetailfromerr.Error(), preventing raw database errors, file paths, and SQL fragments from reaching clientsInstancefield now usesrequest.URL.Pathinstead ofrequest.URL.String()to avoid leaking query parameters that may contain tokensError Handler Hook Bypass (framework)
handleErrornow receives the wrappedResponseWriterinstead of the original, ensuringBeforeWriteHeaderhooks fire consistently for both success and error pathswriteHeaderCalledchanged from plainbooltosync/atomic.Boolto prevent data racesServer Timeout Configuration (framework)
NewServerandNewServerWithhelpers that createhttp.Serverwith secure timeout defaults (ReadHeaderTimeout: 10s, ReadTimeout: 30s, WriteTimeout: 60s, IdleTimeout: 120s)http.ListenAndServe(zero timeouts)Security Headers Middleware (framework/middleware)
SecureHeaders()middleware setsX-Content-Type-Options: nosniff,X-Frame-Options: DENY,Referrer-Policy,X-XSS-Protection, andStrict-Transport-SecuritySecureHeadersWith()for application-specific needsRate Limiting Middleware (framework/middleware)
RateLimit()middleware using per-key token bucket algorithm (golang.org/x/time/rate)RateLimitWith()Open Redirect Prevention (contract/response)
SafeRedirect()that validates URLs are relative paths before redirectingErrUnsafeRedirectfor absolute URLs, protocol-relative URLs,javascript:, anddata:URIsRedirect()preserved with warning documentationPanic Safety Improvements (contract/request)
TryHooks()as a non-panicking alternative toHooks()MustSession,MustSessionKeyed, andHooksdirecting users to safe alternativesGoroutine Exhaustion Prevention (framework/event)
Close()waits for in-flight deliveries viasync.WaitGroupinstead of potentially leaking goroutinesBreaking Changes
Defaulted()no longer auto-populates DetailWhat changed:
problem.Defaulted()no longer setsDetailfrom the wrapped error's.Error()message.Migration:
Why: Internal error messages (database errors, file paths, SQL) were being exposed to HTTP clients.
New Dependencies
golang.org/x/timev0.15.0 (for rate limiting)