security: harden plugin auth surface and frontend SDKs - #40
Open
juicycleff wants to merge 12 commits into
Open
Conversation
Plugins register their own routes, so each is responsible for gating itself. Five route groups shipped without any authentication: the apikey, subscription, oauth2provider admin, social admin, and sso admin groups. None of their handlers carried an authorization check either, and plugin routes mount on a bare router.Group(basePath), so nothing upstream compensated. The worst was apikey: handleCreate took user_id from the request body and returned the secret, so a single unauthenticated POST minted a credential authenticating as any user in the system. Add plugin.SessionGuard / PermissionGuard / AdminGuard so the chain the core API already uses for /v1/admin is one call rather than five hand-rolled copies. No import cycle — middleware and authprovider do not import plugin. The guards return nil rather than failing closed when an engine exposes no registry or RBAC, so bare test wiring still registers routes. - oauth2provider/social/sso admin groups: session + manage permission on oauth2_client / social_provider / sso_connection respectively. - apikey: session on the group, plus ownership enforcement — user_id is now optional and defaults to the caller, naming another user needs manage/apikey, app-wide listing is admin-only, and revoking a key you do not own returns 404 so ids cannot be probed. - subscription: session on all six groups; the 12 mutating routes also require manage/billing. Reads stay session-only so end users can list plans and check entitlements. Two existing tests asserted the old behaviour and were updated: the SDK test called an admin endpoint as a plain user and expected success (now asserts 403), and the apikey validation test expected missing user_id to be a 400 (the field is optional now). Note forge infers `required` from the absence of omitempty, so that needed the struct tag changed, not just the handler. Tenant-scoped billing reads still trust the caller's tenant_id param; closing that is a tenancy redesign, not a guard, and is left as follow-up.
SignIn and SignUp read ?redirect= and navigated to it unvalidated, so ?redirect=https://evil.com sent a user to a hostile page immediately after authenticating — the most convincing possible moment to present a fake "confirm your password" screen. A javascript: value in the same position is an XSS vector. Add isSafeRedirect / safeRedirectTarget in ui-core and use them at both navigation sites. The check resolves the candidate against the current origin and compares origins rather than inspecting the string. The usual fix, candidate.startsWith("/"), is not sufficient: //evil.com, /\evil.com, \\evil.com, /\/evil.com and https:/\evil.com all pass it and all resolve to a different host. URL parsing also normalises the control characters used to disguise schemes, so java\nscript: is caught as javascript: by the protocol allowlist. allowedOrigins supports cross-host deployments and is matched exactly — no wildcards or subdomain patterns, since that is a well-known account-takeover footgun. Entries are normalised through the URL parser so a trailing slash or capitalised host cannot cause a silent miss, and an unparseable entry stays inert rather than matching everything. Add vitest to ui-core and pin the behaviour with 53 cases covering each bypass class. Verified the suite fails (11 tests) when the implementation is swapped for the naive prefix check, so it catches the regression it exists to prevent. The middleware that sets ?redirect= only ever writes a bare pathname and was already safe; the vulnerability was entirely on the consuming side, reachable by crafting the URL directly.
The proxy relayed only Authorization and Cookie, so every proxied request reached the backend wearing the Next.js server's IP. That collapses all users into one rate-limit bucket and silently disables the backend's brute-force protection, account lockout, geo/anomaly scoring, and audit trail — including the MFA challenge limiter, whose own comment names rate limiting as its sole defence. Relay user-agent, accept-language and the x-forwarded-*/x-real-ip family. These are passed through as received, which is correct behind a load balancer or CDN that overwrites them; the header documents that a directly-exposed Next.js app can have X-Forwarded-For forged and needs the backend to trust only its peer address instead. Also: - Set X-Content-Type-Options: nosniff on all three response paths. This route is mounted on the app's own origin, so anything the backend returns as HTML executes with access to this origin's cookies and storage; nosniff stops the browser upgrading an unlabelled body into script on its own. A body labelled JSON that fails to parse is now served as inert text/plain rather than sniffed. - Stop forcing Content-Type: application/json on forwarded requests, which corrupted multipart uploads and form posts. The caller's own type is passed through, and GET/HEAD send none at all. - Add a 30s upstream timeout (configurable) so a hung backend cannot tie up the server indefinitely. Verified against a live echo server: header forwarding, content-type passthrough and nosniff on each path.
Two unauthenticated paths to a full session.
Magic link: GetVerification matches on token alone and every
verification type shares one table, but /v1/magic-link/verify never
checked what it was handed. Email-verification records carry a 6-digit
OTP in the same Token column, so posting {"token":"123456"} walked a
10^6 keyspace and minted a session on a hit — bypassing the 5-attempt
cap the real email-verify path enforces. Confirmed by test: before the
fix, guessing the live OTP returns 200 with a session_token.
Reject any verification whose Type is not magic_link, and rate limit
both routes (send as resend-verification, verify as verify-email).
Phone: the challenge was only deleted on success, with no attempt
counter and no rate limiting, so a 6-digit code could be walked over
its 5-minute TTL and the winning guess returned a session. Add a
5-attempt cap on the challenge itself, discarding it once reached.
The counter is the primary defence rather than the route limit,
because PluginRateLimit returns no middleware at all when rate limiting
is disabled by config — a limiter that can be switched off cannot be
the only thing standing in the way. The rewritten challenge keeps its
original deadline; re-Setting a full CodeTTL would have let an attacker
hold a challenge open indefinitely by guessing, extending the very
window the counter exists to bound.
Also:
- Extract authsome.PluginRateLimit so plugins cap code-submission
endpoints uniformly, and collapse mfa's private copy into it (three
near-identical helpers become one).
- phone.New now applies Config.SMSSender immediately. It was only
applied in OnInit, so a caller passing a sender to New got a nil
sender and a 500 from /start.
- Add phone.SetCeremonyStore alongside the existing SetStore/SetAppID
test affordances.
Both fixes are pinned by tests verified to fail when the guard is
removed: magic-link 2 tests, phone 2 of 4.
Not addressed: ConsumeVerification ignores rows-affected, so concurrent
redemption of one magic link can still yield two sessions for the same
user. Fixing it means returning a count from the Store interface across
all four backends; impact is low since the token is the user's own.
The authorization server issued codes that were barely constrained by the request that created them, and redeemed them with most of the RFC 6749 checks missing. Token exchange (handleAuthorizationCodeGrant): - redirect_uri was never compared to the code's own (§4.1.3), so the binding between the leg that obtained a code and the leg that redeems it did not exist. - client_id was never compared either, so a code minted for one client could be redeemed by another. - PKCE was enforced only when a challenge happened to be present, which let a public client opt out of its own protection by omitting it. - Consumed was read and then written as two statements, so concurrent exchanges could both pass the replay check. Authorization (handleAuthorize): - Requested scopes were stored verbatim, so a client registered for "profile" could name any scope and have it honoured at exchange. Now intersected with the registered set, rejecting anything outside it. - GrantTypes was recorded at registration but never enforced, so any client could run any flow. - Public clients must now send a code_challenge, and must use S256 — plain leaves the verifier recoverable anywhere the challenge is observable, and the authorize URL reaches history, logs and Referer. - The redirect was built by string concatenation, which corrupted a registered URI already carrying a query and let an unescaped state inject extra parameters. Now built with net/url. ConsumeAuthCode becomes a compare-and-set returning whether the caller was the one that consumed the code, across all four backends: postgres and sqlite gain a consumed=false predicate and check RowsAffected, mongo filters on consumed:false and checks ModifiedCount, memory tests the flag under its existing lock. Two adjacent defects found while testing: - RegisterRoutes nil-dereferenced p.engine when called before OnInit, which also made the plugin untestable without a full engine. Now uses plugin.SessionGuard, which nil-checks. - AuthorizeRequest.RedirectURI was marked required at the binder, so a request omitting it was rejected before the handler could apply the single-registered-URI rule the code already implemented. RFC 6749 §4.1.1 makes it optional; the tag now matches. 15 tests added. Each guard was verified load-bearing by removing it and confirming the suite fails. Note the concurrency test alone cannot pin the compare-and-set — the earlier Consumed read usually rejects losers first — so a stub store drives the lost-race outcome directly.
…et attempts Three gaps that each defeated MFA from a different direction. Disabling needed no second factor. handleDisable removed the enrollment on session auth alone, so a stolen or hijacked session could strip the protection MFA exists to provide and keep long-term access. It now requires a current TOTP code or an unused recovery code, with the same replay protection used at sign-in — a code spent on one sensitive action cannot authorise another inside its window. Recovery codes did not work where they are needed. handleRecoveryVerify required an already-authenticated session, but the sign-in gate only accepted TOTP, so a user locked out of their authenticator could never redeem one. ChallengeRequest.Code already documented "or recovery code"; the challenge handler now honours it and issues a real session. The ticket had no attempt bound. A ticket is partial authentication — the password leg already passed — so the only thing left is a 6-digit code, and the handler's own comment named route rate limiting as the sole defence. PluginRateLimit returns no middleware at all when rate limiting is disabled by config, which made that defence optional. The ticket now counts wrong codes and is discarded after MaxMFATicketAttempts (5). As with the phone challenge, the rewritten ticket keeps its original deadline: re-Setting a full MFATicketTTL would let a caller hold a ticket open indefinitely by guessing, extending the very window the counter bounds. A ticket whose attempt count cannot be persisted is dropped rather than left standing uncounted. Also: - Extract consumeRecoveryCode, shared by the challenge and step-up paths. It compares every unused code rather than stopping at the first match, so a wrong code is not measurably faster than a right one under bcrypt. - Add GenerateTOTPCodeAt so tests can obtain codes from two different time-steps; a step-up check cannot reuse the code that completed an earlier step. - DisableRequest.Code is omitempty so an unauthenticated caller still gets 401 rather than a binder 400 that reports a missing field before authentication was checked. 11 tests added. Each guard verified load-bearing by removing it: step-up 4 failures, ticket cap 1, recovery-at-challenge 1.
…harden IP and single-use Clears the remaining backend findings from the review. Plugin init failures were swallowed. EmitOnInit logged a warning and carried on, so a plugin that could not resolve its store, keys or bridges left the engine serving traffic with that control silently absent — an MFA plugin that never wired its store enforces nothing, and a startup warning is not a reliable way to notice. EmitOnInit now returns the first failure named by plugin, InitPlugins propagates it, and Start and the extension abort. pluginsInitialized stays false on failure so a recovering caller can retry. Session-TTL settings were structurally dead. magiclink, sso and social each declared and registered TTL settings that nothing ever read: IssueSession resolves session config centrally via sessionConfigForApp, and the plugin values were only consulted in a test-only fallback branch, so every one of those dashboard controls did nothing in production. IssueSessionRequest now carries SessionTTL/RefreshTTL and the three plugins resolve their settings into it, delivering the per-auth-method lifetimes the settings advertise — a magic-link session need not last as long as an interactive login. The override may only shorten. The app-level config is the operator's ceiling; a per-method setting that could lengthen would be a way around it, so a longer or zero request leaves the configured value in place. Client IP was spoofable in consent and waitlist records. Both had their own helper that returned X-Forwarded-For unconditionally, so any caller could state its own IP — worth little in a record whose purpose is to evidence who consented and from where. Both now use middleware.ClientIP, which honours the header only when the peer is a trusted proxy. ConsumeVerification is single-use across all backends. postgres and sqlite already filtered on consumed=false but discarded RowsAffected, and memory did not check at all, so two concurrent redemptions of one token could both succeed. All four now report ErrNotFound when nothing was consumed — the contract mongo already had — and both callers treat that as a replay rather than an internal error. Passkey LoginBeginRequest.Email is removed. Nothing read it — resolveUser resolves from the auth context — so supplying a value selected the step-up branch while the value itself was discarded. The ceremony is now chosen by whether the caller is authenticated, which is what the branch always meant. Email-identified login stays unimplemented; it would disclose whether an address has passkeys registered, which the discoverable flow avoids by design. 16 tests added. Each guard verified load-bearing by removing it: OnInit propagation 2 failures, TTL shorten-only 1, single-use consume 1.
.claude/worktrees holds full checkouts of this repo, so a stray `git add -A` would commit a copy of the tree into itself. Uses .claude/* rather than .claude/ because git does not descend into an excluded directory — with the bare directory form the negations for shared config (settings.json, agents/, commands/, skills/) would never be reachable.
…ad stored sessions
Two frontend defects, both cases of code that looked right and silently
did nothing.
Session expiry was fabricated. Three of the four places that build a
Session hardcoded "now + 1 hour" and discarded the server's own
expires_at, which AuthResponse has always carried. The refresh timer is
scheduled from that value, so any server TTL other than an hour was
wrong in the dangerous direction: a 15-minute token left the client
sitting on a dead session until minute 59. The MFA-challenge path
already did this correctly; the other three were oversights against an
established pattern. All four now go through one toSession helper, so
there is a single place that knows how expiry is derived.
createCookieStorage was invisible to the server. It persists the whole
Session as JSON under the ui-core storage key, so the cookie is named
authsome%3Asession and holds an object — while middleware and
getServerSession read authsome_session_token expecting a bare token.
Nothing bridged them, so the helper never delivered what its own doc
promised ("so that Next.js middleware and server components can read
it"), and forcing the name through cookieName would have sent
`Bearer <json>` upstream and redirect-looped a signed-in user.
Add readSessionToken, which accepts either shape and prefers the
backend's httpOnly cookie — that is the one JavaScript cannot have
tampered with. A malformed stored cookie reads as "no session" rather
than yielding a value that would be sent upstream as a bearer token.
SESSION_STORAGE_KEY is exported from ui-core so the cookie name is
derived rather than duplicated as a literal.
Also documents that a JS-written cookie cannot be httpOnly, so the
refresh token it carries is readable by any script on the page — the
backend cookie is preferable wherever it is available.
Adds vitest to ui-nextjs. 15 tests; both fixes verified load-bearing by
reverting them (expiry 2 failures, cookie fallback 1).
Both the middleware and AuthManager.initialize treated "could not verify" as "verified", from opposite directions. Middleware only redirected on an explicit 401. A 403, a 404 from a misconfigured baseURL, a 500, or any network error fell through and served the protected route — and since a caller can set an arbitrary cookie value, that meant anyone could reach protected pages whenever the auth API was unhealthy or misaddressed. Validation now returns an explicit verdict: 2xx is valid, 401/403 is rejected, everything else is "unavailable", meaning no verdict was obtained. Unavailable fails closed. onUnavailable: "allow" restores the old behaviour for operators who want availability over authentication and have a second enforcement point behind this one. Failing closed exposed a latent redirect loop: if publicPaths omits the sign-in page, redirecting an unverifiable session there sends it straight back. The sign-in page is now unconditionally reachable — it is the one page a user with an unusable session must be able to load. The old fail-open hid this, and a test caught it. Also adds a 5s timeout. Middleware runs on every matched request, so an auth API that hangs without one stalls the site rather than failing a check. initialize reported status:"authenticated" with `user: null as unknown as User` whenever hydration threw. The intent — don't sign someone out over a transient blip — was right, but the execution told every consumer gating on isAuthenticated to render protected UI for a session nothing had validated, and made user.email a crash. The type assertion existed specifically to defeat the check that would have caught it. Split the two failure modes instead. A 401/403 is the server rejecting the credential: that signs out. Anything else yields a new status:"unknown" — not authenticated, so isAuthenticated is false and protected UI stays hidden with no consumer changes, but the session is retained and a refresh stays scheduled so an outage resolves itself. 25 middleware tests and 65 core tests; both fixes verified load-bearing by restoring the old behaviour (7 failures each).
Two remaining exposures from the security review. Token storage defaulted to localStorage, so every app that did not configure it opted in silently to keeping the refresh token — good for weeks — somewhere any injected script can read. That turns one XSS from a page-lifetime problem into long-term account access. The default is now in-memory: tokens survive the page, not a reload, and no other script can reach them. Persistence stays available as a deliberate choice via createLocalStorage(), whose doc states the exposure. Surviving reloads is better solved by letting the backend set httpOnly session cookies, which JavaScript cannot read at all — that path already exists through the Next.js proxy and readSessionToken. This is a behaviour change: apps relying on the implicit default now lose their session on reload until they pass createLocalStorage() or move to cookies. publicPaths wildcards matched by plain string containment, so "/api/public*" also covered "/api/publicadmin" and "/api/public-internal". One entry meant to expose a single subtree quietly exposed every sibling route sharing its prefix, with nothing in the config to show it had happened. Wildcards now match only at a path-segment boundary: "/api/public*" covers "/api/public" and "/api/public/keys" and nothing else. Trailing slashes are normalized so "/health" and "/health/" are one route, and a root wildcard still covers everything. Docs updated — react.mdx and vue.mdx both listed localStorage as the default, and overview.mdx's example passed it explicitly. 10 tests added. Both verified load-bearing by restoring the old behaviour: the storage default writes to localStorage, and the prefix match leaks into sibling routes.
…t found pnpm lint failed across the workspace because eslint was never installed and no config existed, so the UI packages — which now carry real test suites and the session-handling code from the security review — were unlinted in CI. Adds eslint 9 with typescript-eslint and eslint-plugin-react-hooks, one flat config at the root rather than per-package, and a lint script on each package. Generated clients (src/generated) are ignored: linting a generator's output produces findings nobody here can act on. Type-aware linting is deliberately off — it needs a per-file program and pnpm typecheck already runs tsc everywhere. The cost is noted in the config: no-floating-promises is unavailable, and it is a rule this codebase would benefit from. It found a real bug on the first run. refreshSession accepted an `attempt` parameter, never read it, and the retry path called back in without it — so the counter never advanced. A refresh token the server had revoked was retried every 30 seconds for as long as the tab stayed open, the user was never signed out, and nothing bounded it. Fixed along the same lines as initialize: a 401/403 is the server rejecting the token, which signs out, since retrying cannot help. Anything else is no verdict, retried with exponential backoff up to MAX_REFRESH_ATTEMPTS, after which the state becomes "unknown" — session retained, but not claiming authenticated for something nothing has validated. Also fixed the two other errors it surfaced: nine dead imports across storybook stories (the react-jsx transform makes the bare React imports unnecessary), and an empty InputProps interface, which is a second name for its supertype and open to declaration merging widening it. 26 warnings remain, all no-explicit-any in the hand-written client wrapper. Left as warnings deliberately: they are honest boundary types, and turning them into errors would either block the build or invite blanket suppression. 4 refresh tests added, verified load-bearing by restoring the unbounded retry (3 failures).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+79
to
+81
| window.location.href = safeRedirectTarget(params.get("redirect"), "/", { | ||
| currentOrigin: window.location.origin, | ||
| }); |
Comment on lines
+75
to
+77
| window.location.href = safeRedirectTarget(params.get("redirect"), "/", { | ||
| currentOrigin: window.location.origin, | ||
| }); |
|
|
||
| /** Strips trailing slashes, preserving the root path as "/". */ | ||
| function normalizePath(p: string): string { | ||
| const trimmed = p.replace(/\/+$/, ""); |
Comment on lines
+79
to
+81
| window.location.href = safeRedirectTarget(params.get("redirect"), "/", { | ||
| currentOrigin: window.location.origin, | ||
| }); |
Comment on lines
+75
to
+77
| window.location.href = safeRedirectTarget(params.get("redirect"), "/", { | ||
| currentOrigin: window.location.origin, | ||
| }); |
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
2 participants
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.
Continues the security-hardening work already on this branch with a full review of the plugin system and the frontend SDKs, plus fixes for everything it found.
The earlier commits (through
4cb1e35) cover store conformance, dependency patches, and the first hardening pass. The 12 commits after that are the new work described below.Unauthenticated endpoints
Five plugin route groups shipped with no authentication. Plugins register their own routes, so each is responsible for gating itself, and nothing upstream compensated — plugin routes mount on a bare
router.Group(basePath).user_idwith any scopesThe worst was apikey:
handleCreatetookuser_idfrom the request body and returned the secret, so one unauthenticated POST minted a credential authenticating as any user.Adds
plugin.SessionGuard/PermissionGuard/AdminGuardso the chain the core API already uses for/v1/adminis one call rather than five hand-rolled copies.Paths to a session from an unauthenticated start
GetVerificationmatches on token alone and every verification type shares one table. Email-verification records carry a 6-digit OTP in the same column, so{"token":"123456"}walked a 10⁶ keyspace and minted a session on a hit. Confirmed by test: before the fix, guessing the live OTP returned 200 with asession_token.OAuth2 authorization server
redirect_uriwas never compared to the code's own (RFC 6749 §4.1.3), nor wasclient_id. PKCE was enforced only when a challenge happened to be present, letting a public client opt out by omitting it. Requested scopes were stored verbatim, andGrantTypeswas recorded at registration but never enforced.Consumedwas read and written as two statements, so concurrent exchanges could both pass.ConsumeAuthCodeis now a compare-and-set across all four backends.MFA
Disabling needed no second factor — a stolen session could strip MFA silently. Recovery codes required an already-authenticated session while the sign-in gate only accepted TOTP, so they could never be redeemed when actually needed. The ticket had no attempt bound, and the handler's own comment named route rate limiting as its sole defence — which returns no middleware at all when disabled by config.
Frontend SDKs
SignIn/SignUp—?redirect=was followed unvalidated, at the moment a phishing page is most convincing. The usualstartsWith("/")guard is insufficient://evil.comand/\evil.comboth pass it and both resolve to another host.initialize()reportedstatus: "authenticated"withuser: null as unknown as User. Both now distinguish "rejected" from "no verdict".localStorage, putting a multi-week refresh token where any injected script can read it. Now in-memory by default, withcreateLocalStorage()as an explicit opt-in.publicPathswildcards matched by string containment, so/api/public*also covered/api/publicadmin.Tooling
pnpm lintfailed workspace-wide because eslint was never installed and no config existed. Adding it surfaced a real bug on the first run:refreshSessionaccepted anattemptparameter, never read it, and the retry re-entered without it — so a revoked refresh token retried every 30s for as long as the tab stayed open, and the user was never signed out..claude/is now gitignored; it holds full worktree checkouts of this repo.Verification
Every fix is pinned by a test verified to fail when the guard is removed — not just to pass with it. That caught two tests that looked like coverage and were not: an OAuth2 concurrency test that couldn't isolate the compare-and-set (the earlier
Consumedread wins the race in practice), and two mutations that reported nothing because removing the line broke the build rather than failing a test.Breaking changes
Reviewers should know these before merge; each is documented in its own commit:
DELETE /v1/mfa/enrollmentnow requires a JSON body withcodecode_challenge; token exchange now requires a matchingredirect_uriInitPluginsreturnserror, and a plugin failingOnInitaborts startup instead of being skippedAuthStategained anunknownmember (exhaustive switches need an arm;=== "authenticated"checks are already safe)createLocalStorage()or move to httpOnly cookiesLoginBeginRequest.Emailremoved (it was never read)Not addressed
tenant_idparam. Closing that is a tenancy redesign, not a guard.forge.Unauthorizedreturned from middleware yields500 "nope\n"instead of a JSON 401 — this codebase works around it with 19 hand-writtenctx.JSONcalls and zero typed error returns. Alsorequired-by-default binder inference with four ways to opt out, which caused three separate bugs here.main, surfaced by the push. Unrelated to this branch's first-party code.🤖 Generated with Claude Code