Skip to content

Fix high-severity security vulnerabilities#4

Merged
ConsoleTVs merged 13 commits into
mainfrom
fix/high-security-vulnerabilities
Apr 10, 2026
Merged

Fix high-severity security vulnerabilities#4
ConsoleTVs merged 13 commits into
mainfrom
fix/high-security-vulnerabilities

Conversation

@ConsoleTVs

Copy link
Copy Markdown
Member

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-populates Detail from err.Error(), preventing raw database errors, file paths, and SQL fragments from reaching clients
  • Instance field now uses request.URL.Path instead of request.URL.String() to avoid leaking query parameters that may contain tokens

Error Handler Hook Bypass (framework)

  • handleError now receives the wrapped ResponseWriter instead of the original, ensuring BeforeWriteHeader hooks fire consistently for both success and error paths
  • Fixes broken logger middleware status tracking for error responses
  • writeHeaderCalled changed from plain bool to sync/atomic.Bool to prevent data races

Server Timeout Configuration (framework)

  • Added NewServer and NewServerWith helpers that create http.Server with secure timeout defaults (ReadHeaderTimeout: 10s, ReadTimeout: 30s, WriteTimeout: 60s, IdleTimeout: 120s)
  • Prevents Slowloris and connection-exhaustion attacks that are possible with http.ListenAndServe (zero timeouts)

Security Headers Middleware (framework/middleware)

  • New SecureHeaders() middleware sets X-Content-Type-Options: nosniff, X-Frame-Options: DENY, Referrer-Policy, X-XSS-Protection, and Strict-Transport-Security
  • Fully configurable via SecureHeadersWith() for application-specific needs

Rate Limiting Middleware (framework/middleware)

  • New RateLimit() middleware using per-key token bucket algorithm (golang.org/x/time/rate)
  • Defaults: 10 req/s per IP, burst of 20
  • Configurable key extraction, rates, and error response via RateLimitWith()

Open Redirect Prevention (contract/response)

  • Added SafeRedirect() that validates URLs are relative paths before redirecting
  • Returns ErrUnsafeRedirect for absolute URLs, protocol-relative URLs, javascript:, and data: URIs
  • Original Redirect() preserved with warning documentation

Panic Safety Improvements (contract/request)

  • Added TryHooks() as a non-panicking alternative to Hooks()
  • Added WARNING documentation to MustSession, MustSessionKeyed, and Hooks directing users to safe alternatives

Goroutine Exhaustion Prevention (framework/event)

  • MemoryBroker now bounds concurrent deliveries with a semaphore (default 1024)
  • Close() waits for in-flight deliveries via sync.WaitGroup instead of potentially leaking goroutines

Breaking Changes

Defaulted() no longer auto-populates Detail

What changed: problem.Defaulted() no longer sets Detail from the wrapped error's .Error() message.

Migration:

// Before: Detail was auto-populated from err.Error()
problem.NewProblem(err, 500).ServeHTTP(w, r)

// After: Set Detail explicitly if you want it in the response
problem.Problem{
    Detail: "A user-friendly message",
    Status: 500,
}.WithError(err).ServeHTTP(w, r)

Why: Internal error messages (database errors, file paths, SQL) were being exposed to HTTP clients.

New Dependencies

  • golang.org/x/time v0.15.0 (for rate limiting)

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.
@ConsoleTVs ConsoleTVs marked this pull request as ready for review April 10, 2026 23:17
@ConsoleTVs ConsoleTVs merged commit 337d578 into main Apr 10, 2026
2 checks passed
@ConsoleTVs ConsoleTVs deleted the fix/high-security-vulnerabilities branch April 10, 2026 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant