Harden live-session security: returnUrl, WS origin, reconnect race, session cap - #34
Merged
Conversation
…ession cap
Verified findings from a full-solution review (false positives filtered out):
- SanitizeReturnUrl: reject backslash ("/\evil.com", "\evil.com") and control-char
return URLs that browsers normalise into protocol-relative redirects. Mirrors
ASP.NET Url.IsLocalUrl.
- LiveSession.AttachSocket: stop writing the dedup baselines (_lastSentBuffer/
_lastSentHtml) from a possibly-background thread. A reconnect now sets a volatile
_forceResend flag that RenderAndSendAsync consumes inside _renderLock, removing the
unsynchronised cross-thread field access.
- WebSocket endpoint: reject cross-origin handshakes (CSWSH) via the existing host-only
IsSameOrigin check used by the redeem endpoint.
- RaskLiveOptions.MaxSessions: opt-in cap on concurrent live sessions; an over-cap GET
returns 503 + Retry-After. Default 0 = unlimited (no behaviour change).
Docs: document the sessionId trust model, the WS origin check, and the session cap in
docs/authentication.md.
Tests: SanitizeReturnUrlTests, WebSocketOriginTests, SessionCapTests. Full non-E2E
suite green; auth + session-lifecycle E2E (13) green.
… re-auth on dispatch, returnUrl parity - H1: emit Cache-Control: no-store on the authenticated GET shell so the embedded session id (the de-facto WS/upload/download bearer) can't be cached by a shared proxy/bfcache and replayed by another principal. - H3: gate the upload and download endpoints (id-addressed, antiforgery-exempt) with IsSameOrigin + a session-owner check, so a leaked sessionId can't be used cross-origin or by a different signed-in user. Origin check runs before the one-shot download TryTake so a rejected attempt doesn't consume the entry. - M2: re-evaluate the route authorization guard for the current principal before invoking a WS event handler; a user whose access was revoked mid-session now gets a challenge redirect instead of one free handler invocation. - H4: introduce shared Rask.Core.Routing.LocalUrl.Sanitize (single source of truth for the IsLocalUrl rule); server SanitizeReturnUrl now delegates to it, and the WASM login flows (rask-wasm/rask-wasm-hosted templates + WasmCookie/ WasmJwt samples) sanitize returnUrl before Navigate, closing the open-redirect parity gap with the server. Tests: RootGetEndpointTests (no-store), UploadDownloadEndpointTests (cross-origin rejected, entry not consumed), RevokedAuthDispatchTests (handler skipped + redirect after revocation), Core LocalUrlTests. Server 155/155, Core 1436/1437.
…al route registrations - M8: a component property named with a C# keyword via a verbatim identifier (e.g. `public string? @event`) emitted invalid C# in the generated factory (`string? event = null`, `__c.event = event`) because ISymbol.Name strips the leading '@'. Add ComponentFactoryGenerator.EscapeIdentifier + PropInfo.Escaped and use it at every emitted identifier site (signature params, object initializer, property accesses, param refs); local names like `__old_<name>` keep the raw Name (a valid identifier even for keywords). No-op for ordinary names, so the common path is byte-identical. (The rare keyword-prop-on- [FactoryGeneric] combo is left as-is — no regression, no real-world instance.) - M9: a partial routed page with attributes on more than one declaration (e.g. [Route] on one part, [Obsolete]/a source-gen attribute on another) produced one Candidate per attributed declaration, emitting duplicate RouteRegistration entries (competing Route nodes for the same page) + duplicate [DynamicDependency]. Emit the registry from byFqn (deduped per FQN) instead of the raw candidate list. Tests: KeywordIdentifierAndPartialRouteTests — M8 verified by COMPILING the generated source (GeneratorRun.GeneratedCompileErrors), M9 by registration count. Generator suite 132/132; full solution build clean.
…/session ids - M1: the multi-fragment WS receive loop appended into an ArrayBufferWriter with no cap, so a client could stream an unbounded fragmented frame and force the server to buffer it whole before JsonDocument.Parse (per-socket memory DoS). Only the single-fragment path was bounded (16KB receive buffer). Add an 8MB MaxInboundFrameBytes cap (test-tunable) to the reassembly loop and ws.Abort() past it. Client→server frames are small; uploads use HTTP. - M5: redeem tickets (authority to set the auth cookie) and live-session ids (WS/upload/download bearer) were Guid.NewGuid() — only ~122 random bits and no contractual cryptographic-strength guarantee. New SecureToken.Create() uses RandomNumberGenerator.GetHexString (128 bits, same 32-lowercase-hex shape as Guid "N", a drop-in). Both sites call it. M4 (require Origin on the WS handshake) assessed and DECLINED: a cross-origin browser CSWSH attack always sends Origin (already rejected); the absent-Origin path can't be driven by a browser carrying the victim's cookie, the session is also gated by the unguessable sessionId, and ~38/40 existing WS connections (and non-browser tooling) connect without Origin by documented design. Marginal benefit, real breakage. Tests: WebSocketFrameSizeTests, SecureTokenTests. Server suite 159/159.
…download sink - M10: WasmLiveSession subscribed to IUserProvider.Changed in its ctor but never unsubscribed in Dispose, leaving an asymmetric teardown — a Changed raised after dispose would fire OnUserChanged on the disposed _lock. Not a live leak today (one session per page, Dispose uncalled in production, provider shares the session lifetime), but the implemented Dispose should be correct: store _userProvider and `-= OnUserChanged` first in Dispose. - M12: WasmDownloadSink only removed token entries on Pull, so an orphaned stage (a second Stage before the first is consumed, a coalesced render, or a token the browser never pulls after navigating away) leaked its byte[] for the whole page lifetime, unbounded. Add a FIFO cap (16) evicting the oldest on Stage; the normal one-stage-one-pull flow is untouched. M6 (EditContext timers/CTS) assessed and found to be a NON-ISSUE: the StickyTimer is one-shot (self-clears ~200ms) and the per-field CTS is disposed in every validation path (sync finally, async supersession, async-completion finally), so there is no persistent leak — only a bounded ~200ms post-unmount hold. A structural IDisposable change would add complexity for no benefit. M11 (IJSObjectReference map never released) is real but low-reachability and a shared Server+WASM design limitation; deferred to a dedicated unified change. Tests: DisposalTests (provider unsubscribe; download-sink bound + round-trip). WASM suite 46/46.
…UseAuthorization - M13: the rask-server and rask-wasm-hosted Host templates scaffolded a cookie with only Name/LoginPath set (default SecurePolicy=SameAsRequest) and no transport security, over an http-only launch profile — so the auth cookie went out without the Secure flag. Set Cookie.SecurePolicy=Always + SameSite=Lax, add UseHttpsRedirection() and UseHsts() (guarded out of Development, outside the //#if (auth) block so HTTPS applies regardless), and switch launchSettings to HTTPS-primary (added one for the Host, which had none) so the Secure cookie and redirect work in development. - M14: the hosted Host omitted app.UseAuthorization(), so a [Authorize] / RequireAuthorization() a consumer adds would be silently unenforced. Added it. The /api/login CSRF concern is mitigated by the now-SameSite=Lax auth cookie; a full antiforgery system is intentionally not added (it needs client-side token plumbing and is unusual for a JSON SPA login) — documented in a comment. Templates are not compiled in-repo (placeholder package ref + Compile Remove), so changes were verified by inspection, standard-API reasoning, JSON validation, and a full solution build. The cookie hardening stays inside //#if (auth); the no-auth scaffold is unaffected.
… race - Context.Has<T>() now marks the caller a context consumer (like Get<T>()), so a component that gates purely on Has bypasses the render cache and re-runs when the provider re-renders — previously it stayed cached and showed stale UI when the value appeared/changed. - Navigator.EnterHandler() resets the pending-navigation state (_dirty/_replace) on entry, so a handler that queued a Navigate(...) and then threw before TryConsumeHistory ran can't leak that navigation (and its replace flag) into the next dispatch and fire a nav the user never triggered. Reset on entry (not scope dispose) so the established consume-after-dispose pattern still works. - HandlerSyncContext.Post() now schedules and records the task under one lock, so DrainAsync can't snapshot _pending in the gap between Task.Run and the Add and return before a just-posted render completes. Tests: ContextTests.HasGate_MarksConsumer_*, NavigatorTests.UnconsumedNavigation_*. Core suite 1438/1439. Also triaged as NON-ISSUES (no change): the one-time Context.Get consumer latch (permanent latch is necessary — a skipped component can't be known to read context without executing it); Virtualize _activeFetch (WASM single-threaded, Server swap under the session lock, reference-field access atomic, CTS Dispose/IsCancellationRequested safe post-dispose); the WS navigate handler (self-navigation, no escalation, JSON-escaped); the coalesce-loop dropped render (already logged; auth/history sent on the first render); async lifecycle-hook faults after an await (the terminal ContinueWith checks IsFaulted BEFORE the PostFired short-circuit, and MoveNext captures the throw — faults ARE reported).
- Generator now URL-encodes the query-param KEY at generation time (the value was
already encoded at runtime), so an explicit [QueryParam("a b&c")] or a name with
'&'/'=' can't emit a malformed query string. No-op for property-name-derived
keys (valid identifiers).
- Rask.Example.Auth.WasmJwt.Host fails fast when Jwt:Key is unset outside
Development instead of silently falling back to the public, forgeable
JwtIssuer.DevKey. Dev convenience (DevKey) is preserved in Development.
Tests: RoutesGeneratorTests.QueryParam_NameWithSpecialChars_*; sample compiles.
Generator suite 133/133.
The fail-fast guard in WasmJwt.Host (no Jwt:Key outside Development) crashed the E2E host on startup, since the fixture runs every host with DOTNET_ENVIRONMENT=Production. Add an overridable ExtraEnvironment hook to ExampleAppFixture and have WasmJwtAuthAppFixture supply a test-only signing key, keeping the production hardening intact.
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.
Context
Output of a full-solution code review. A parallel agent sweep produced ~50 raw findings; a verification pass against the source filtered out the false positives (e.g. a claimed "Critical" keyed-diff bug that the code's own comment shows doesn't exist; an EditContext "CTS race" that's a non-issue under single-threaded dispatch; "XSS via
Raw" which is by-design BlazorMarkupStringparity). The Roslyn generators audited clean. This PR fixes the findings that survived verification.Changes
SanitizeReturnUrlopen-redirect — also reject backslash (/\evil.com,\evil.com) and control-char return URLs that browsers normalise into protocol-relative redirects (mirrors ASP.NETUrl.IsLocalUrl).AttachSocketdedup-baseline race — the dedup baselines (_lastSentBuffer/_lastSentHtml) were written from a possibly-background thread whileRenderAndSendAsyncread/swapped them under_renderLock. A reconnect now sets avolatile _forceResendflag consumed inside the render lock, removing the unsynchronised cross-thread access./rask/wsrejects cross-origin handshakes (403), reusing the existing host-onlyIsSameOriginhelper already used by the redeem endpoint.docs/authentication.mdnow spells out that the sessionId is a bearer secret and thehellohandler rebinds session identity (Blazor-circuit parity).RaskLiveOptions.MaxSessions; an over-cap GET returns503+Retry-After. Default0= unlimited, so no behaviour change unless configured.Two course-corrections during implementation: (a) #3 was refactored to reuse the existing
IsSameOriginhelper — its host-only semantics are deliberate (TLS-terminating proxies), so a stricter scheme+port check would have regressed legitimate handshakes; (b) the cap is a per-store instance value, not a global static, so concurrent hosts/tests don't interfere.Tests
SanitizeReturnUrlTests,WebSocketOriginTests,SessionCapTests.SocketLifecycleTestsreconnect test.