fix: binder required semantics - #36
Merged
Merged
Conversation
v1.1.2 adds ownValues tracking to the pooled http.Ctx: ShareValues now records that the values map was borrowed, and Cleanup hands the context a private map before returning it to the pool. Without that, a context returned to the pool still aliasing another context's values map lets the next unrelated request clear and overwrite values belonging to a request still in flight. That map carries request identity and tenant scope, so the failure mode is cross-request data corruption rather than a stale read.
CORS wildcard subdomains were matched with strings.HasSuffix, so "*.example.com" accepted any origin merely ending in "example.com" — evil-example.com and notexample.com among them. Since getAllowOrigin reflects the request origin back and this path is reachable with AllowCredentials, an attacker could register a matching domain and read authenticated responses. Origins are now parsed and matched on label boundaries. The rate limiter keyed on RemoteAddr, which Go sets to "IP:ephemeral-port". A new port per connection meant a new bucket per connection: 100 of 100 requests from one IP were allowed against a burst of 3, and the bucket map grew without bound under client control. Keying is now per client IP via ClientIP, the map is capped, and RateLimitWithKey covers proxy and user-ID keying. NewRateLimiter's cleanup goroutine also had no way to stop; added Stop(). Timeout buffered the entire response with no cap and implemented neither Flusher nor Hijacker, so streaming stalled until the handler returned, WebSocket upgrades failed through it, and any large response was held in memory. Buffering is now bounded and abandoned on flush or hijack, and the timeout path no longer writes a second header over a committed response. Recovery called logger.Error unguarded — a nil logger panicked inside the deferred recover, killing the connection instead of returning the 500 it exists to produce. It now guards the logger, logs debug.Stack() as its comment always claimed, re-panics http.ErrAbortHandler, and skips writing over an already-committed response. Logging's SensitiveHeaders and IncludeHeaders were never read, so the default redaction list read as protection that did not exist; the measured duration was discarded too. GetRequestID always returned "" because nothing ever stored the key it reads, and the inbound X-Request-ID was echoed and logged unbounded.
A 5xx response body built from a wrapped error embeds whatever the error carried — driver output, SQL fragments, file paths, internal hostnames — and anyone able to trigger the error could read it. Both paths leaked. The router's fallback put err.Error() straight into the 500 body, and InternalError produces an IHTTPError whose ResponseBody embeds the wrapped text, which DefaultErrorHandler (the path real apps take, since the app always installs one) serialized verbatim. The redaction therefore lives in internal/shared, where both paths go through it: 5xx bodies are replaced with a minimal envelope unless exposure is enabled, 4xx bodies pass through untouched because validation messages are exactly what the client needs to correct the request. The app enables exposure outside production so local debugging is unaffected; detail always reaches the logs either way.
Three defects that silently crossed request boundaries. Group interceptor inheritance appended onto the group's own slice. When that slice had spare capacity — three separate WithGroupInterceptor options give len 3, cap 4 — each route wrote its interceptor into the group's backing array, and cfg is shallow-copied into route.config and closed over by applyMiddlewareAndInterceptors, so the aliasing outlived registration. A route registered earlier silently inherited a later sibling's interceptor: a route guarded by requireAuth ran publicOnly instead. Both the interceptor and middleware slices are now cloned before appending. shareContextValues aliases the lender's values map into the borrower for O(1) propagation, but the borrower was then returned to the shared pool still holding that alias. The next unrelated request drew it out and NewContext's clear() wiped, then overwrote, the lender's map mid-flight — and that map holds tenant scope (see forge.SetScope), making this a tenant-isolation failure. The borrow is now released before pooling, using the context's own ReleaseSharedValues where available (go-utils >= v1.1.2) and falling back otherwise. Parallel and ParallelAny handed the same context to N goroutines. The context has no internal synchronization — Set is a bare map write and Scope() an unsynchronized lazy init — so interceptors that touch it raced, and an unsynchronized map write is a fatal runtime error recover() cannot catch: it takes the process down rather than failing the request. The documented Enrich pattern triggers it. Interceptors now receive a syncContext sharing one guard. The guard is detachable, which matters more than the mutex: ParallelAny returns on first success and leaves goroutines running, and a lock makes concurrent access safe without stopping a straggler from writing to a context the handler already returned and the pool may have reassigned. Each regression test was confirmed to fail against the pre-fix code.
WebSocket upgrades performed no Origin check. Browsers do not apply CORS to upgrades but do send cookies with them, so any site a logged-in user visited could open an authenticated socket as that user. WebTransport already had a checker, but an empty allow-list meant "allow everything", leaving its default open too. Both now share one checker that defaults to same-origin. Requests with no Origin are still allowed, since non-browser clients do not send one and browsers always do; cross-origin clients are permitted explicitly via WithWebSocketOrigins. Wildcard entries match on label boundaries, so "*.example.com" does not accept evil-example.com. SSE wrote "event: %s" and "data: %s" with no validation. A newline in the event name or a multi-line payload terminates the field and lets the rest parse as further SSE fields — event forgery wherever any part of the value is user-influenced, and silent corruption of multi-line data regardless. Event names and comments are now rejected if they contain a newline, and data is encoded one "data: " line per line as the grammar requires. Connection IDs came from time.Now().UnixNano(), which collides for upgrades landing in the same clock tick; two live connections sharing an ID means cross-talk wherever connections are tracked by ID, and the value was trivially guessable. Now UUIDs. WebSocket reads were unbounded, letting a peer request an arbitrarily large allocation via the advertised frame length, and concurrent readers could interleave partial frames. Reads are now capped and serialized. SSE, WebSocket and WebTransport handlers never called Cleanup, the only place scope.End() and MultipartForm.RemoveAll() run — so every long-lived connection leaked its DI scope, and any that parsed a multipart form leaked temp files, for the process lifetime. These are the longest-lived contexts in the system, so this was the most consequential leak. Also dropped the wildcard Access-Control-Allow-Origin on the AsyncAPI spec endpoint, which made the channel and message map readable by any origin.
Nothing capped request bodies. Binding decoded straight from r.Body, so a single unauthenticated request could drive allocation as large as the client chose to send. MaxRequestBodySize now defaults to 10 MiB and is enforced via MaxBytesReader before any handler, binder or middleware reads the body, with WithMaxBodySize to raise it on upload routes or drop the limit. WebSocketOrigins exposes the upgrade allow-list; unset means same-origin. pprof had no authorization seam. Once enabled the endpoints dump heap and goroutine state, let a caller pin a CPU for an arbitrary duration, and /cmdline discloses the process argv — which commonly carries credentials passed as flags. WithPprofGuard authorizes each request and 404s on denial so the endpoints stay undiscoverable; enabling pprof without a guard now logs a warning. Internal error exposure is wired to Environment, on everywhere but production.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Conventional Commits ValidationPR Title: valid |
Contributor
Conventional Commits ValidationPR Title: valid |
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
1 participant
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.
No description provided.