From 4ddad862df2e758d4b0663707381a69468612330 Mon Sep 17 00:00:00 2001 From: Bretton Date: Wed, 26 Aug 2026 21:40:14 -0700 Subject: [PATCH] fix(auth): enforce instance lock server-side, drop stale profile on unauthenticated sync - POST /api/auth/login now returns 403 for any origin other than PUBLIC_INSTANCE_URL when PUBLIC_LOCK_TO_INSTANCE is on (the default). The flag previously only hid the instance field in the login UI, so the endpoint could still start OAuth against an arbitrary host. Policy lives in a pure lockedInstanceOrigin() helper shared by the browser (LINKED_INSTANCE_URL) and server. - Profile.syncFromServer() now resets to guest when the server reports no session, instead of leaving a previously persisted authenticated profile in localStorage. A shared device no longer keeps showing the prior user's handle/avatar after their cookie expires. Co-Authored-By: Claude Fable 5 --- docs/ENVIRONMENT.md | 2 +- src/lib/app/state/auth.svelte.test.ts | 63 ++++++++++++++++++++++ src/lib/app/state/auth.svelte.ts | 12 ++++- src/lib/app/state/instance.svelte.ts | 8 +-- src/lib/app/state/instance/resolve.test.ts | 38 +++++++++++++ src/lib/app/state/instance/resolve.ts | 19 +++++++ src/lib/server/instance.ts | 10 ++++ src/routes/api/auth/auth.test.ts | 50 +++++++++++++++++ src/routes/api/auth/login/+server.ts | 18 ++++++- 9 files changed, 213 insertions(+), 7 deletions(-) diff --git a/docs/ENVIRONMENT.md b/docs/ENVIRONMENT.md index a4b39603..d98eae52 100644 --- a/docs/ENVIRONMENT.md +++ b/docs/ENVIRONMENT.md @@ -17,7 +17,7 @@ without the prefix is server-only. All are read at **runtime** (via | `PUBLIC_INSTANCE_URL` | browser, server | **yes in production** | The Coves backend as reachable from the browser (e.g. `https://coves.social`). The server refuses to boot in production without it, even if `PUBLIC_INTERNAL_INSTANCE` is set, because the browser can only ever see this value. In dev the OAuth cookie is scoped to this host, so `hooks.server.ts` redirects any other hostname to it (RFC 8252 requires `127.0.0.1`, not `localhost`). | | `PUBLIC_INTERNAL_INSTANCE` | server | no | Server-only shortcut to the backend for `hooks.server.ts` (`/api/me` validation) and the `/api/proxy` upstream — e.g. `http://appview:8080` on a Docker network, or `http://127.0.0.1:8081` in dev to skip the Caddy loop. Falls back to `PUBLIC_INSTANCE_URL`. | | `ALLOW_HTTP_INTERNAL_INSTANCE` | server | no | `"true"` to let the production proxy talk plaintext `http://` **only** to the origin of `PUBLIC_INTERNAL_INSTANCE` (which must then carry an explicit `http://` scheme). Any other `http://` target is still rejected with 400. | -| `PUBLIC_LOCK_TO_INSTANCE` | browser | no (default `true`) | When `true`, the login UI is pinned to `PUBLIC_INSTANCE_URL` and users cannot type a different instance. Set `false` to allow arbitrary instances. | +| `PUBLIC_LOCK_TO_INSTANCE` | browser, server | no (default `true`) | When `true`, login is pinned to `PUBLIC_INSTANCE_URL`: the login UI hides the instance field and `POST /api/auth/login` rejects any other origin with 403. Set `false` to allow arbitrary instances. | Resolution precedence: diff --git a/src/lib/app/state/auth.svelte.test.ts b/src/lib/app/state/auth.svelte.test.ts index 2e358c9d..43067f40 100644 --- a/src/lib/app/state/auth.svelte.test.ts +++ b/src/lib/app/state/auth.svelte.test.ts @@ -17,6 +17,7 @@ vi.mock('$lib/server/session', () => ({ import { isAuthenticated, isGuest, + profile, type ProfileInfo, type GuestProfile, type AuthenticatedProfile, @@ -163,3 +164,65 @@ describe('LogoutResult interface', () => { expect(result.remoteLogoutError).toBe('Token revocation failed') }) }) + +describe('Profile.syncFromServer', () => { + const authed: AuthenticatedProfile = { + type: 'authenticated', + id: 'did:plc:abc123', + instance: 'https://coves.social' as any, + jwt: 'authenticated', + did: 'did:plc:abc123' as any, + handle: 'test.user' as any, + } + + const seedAuthenticated = () => { + profile.meta.profiles = [authed] + profile.meta.profile = authed.id + } + + it('adopts the server account when authenticated', () => { + profile.syncFromServer({ + authenticated: true, + activeAccountId: 'did:plc:xyz', + account: { + id: 'did:plc:xyz', + did: 'did:plc:xyz', + handle: 'other.user', + instance: 'https://coves.social', + }, + } as any) + + expect(profile.meta.profile).toBe('did:plc:xyz') + expect(profile.meta.profiles).toHaveLength(1) + expect(isAuthenticated(profile.meta.profiles[0])).toBe(true) + }) + + it('drops a persisted authenticated profile when the server has no session', () => { + seedAuthenticated() + + profile.syncFromServer({ authenticated: false } as any) + + expect(profile.meta.profile).toBe('guest') + expect(profile.meta.profiles).toHaveLength(1) + expect(isGuest(profile.meta.profiles[0])).toBe(true) + }) + + it('drops a persisted authenticated profile when session data is missing', () => { + seedAuthenticated() + + profile.syncFromServer(undefined) + + expect(profile.meta.profile).toBe('guest') + expect(isGuest(profile.meta.profiles[0])).toBe(true) + }) + + it('leaves an existing guest profile untouched when unauthenticated', () => { + profile.syncFromServer(undefined) + const before = profile.meta.profiles[0] + + profile.syncFromServer({ authenticated: false } as any) + + expect(profile.meta.profiles[0]).toBe(before) + expect(profile.meta.profile).toBe('guest') + }) +}) diff --git a/src/lib/app/state/auth.svelte.ts b/src/lib/app/state/auth.svelte.ts index 7bb199f7..77085f12 100644 --- a/src/lib/app/state/auth.svelte.ts +++ b/src/lib/app/state/auth.svelte.ts @@ -266,7 +266,17 @@ class Profile { * @param serverSession - The session data from the server (passed via page data) */ syncFromServer(serverSession: ServerSession | undefined): void { - if (!serverSession || !serverSession.authenticated) return + if (!serverSession || !serverSession.authenticated) { + // The server is the source of truth. If it reports no session (cookie + // expired, revoked, or cleared) drop any persisted authenticated + // profile so a shared device doesn't keep showing the previous user's + // handle and avatar as if they were still signed in. + if (this.meta.profiles.some(isAuthenticated)) { + this.meta.profiles = [createGuestProfile()] + this.meta.profile = 'guest' + } + return + } // Convert server account to client ProfileInfo format const serverProfile: AuthenticatedProfile = { diff --git a/src/lib/app/state/instance.svelte.ts b/src/lib/app/state/instance.svelte.ts index 884d3886..629b4887 100644 --- a/src/lib/app/state/instance.svelte.ts +++ b/src/lib/app/state/instance.svelte.ts @@ -3,6 +3,7 @@ import { env } from '$env/dynamic/public' import { profile } from './auth.svelte' import { hasRequiredInstanceConfig, + isLockedToInstance, MISSING_INSTANCE_MESSAGE, resolveInstanceUrl, } from './instance/resolve' @@ -17,10 +18,9 @@ class InstanceData { export const instance = new InstanceData() -export const LINKED_INSTANCE_URL = - (env.PUBLIC_LOCK_TO_INSTANCE ?? 'true').toLowerCase() == 'true' - ? env.PUBLIC_INSTANCE_URL - : undefined +export const LINKED_INSTANCE_URL = isLockedToInstance(env) + ? env.PUBLIC_INSTANCE_URL + : undefined const getDefaultInstance = (): string => { // The instance URL must never default to a third-party host. In production diff --git a/src/lib/app/state/instance/resolve.test.ts b/src/lib/app/state/instance/resolve.test.ts index 39b425d9..cfafe7c2 100644 --- a/src/lib/app/state/instance/resolve.test.ts +++ b/src/lib/app/state/instance/resolve.test.ts @@ -4,7 +4,9 @@ import { canonicalPublicHost, hasRequiredInstanceConfig, instanceOrigin, + isLockedToInstance, isUpstreamSchemeAllowed, + lockedInstanceOrigin, normalizeInstanceUrl, resolveInstanceUrl, } from './resolve' @@ -155,3 +157,39 @@ describe('addressHeaderWarning', () => { } }) }) + +describe('isLockedToInstance', () => { + it('defaults to locked when unset', () => { + expect(isLockedToInstance({})).toBe(true) + }) + + it('only the literal "false" unlocks, case-insensitively', () => { + expect(isLockedToInstance({ PUBLIC_LOCK_TO_INSTANCE: 'false' })).toBe(false) + expect(isLockedToInstance({ PUBLIC_LOCK_TO_INSTANCE: 'FALSE' })).toBe(false) + expect(isLockedToInstance({ PUBLIC_LOCK_TO_INSTANCE: 'true' })).toBe(true) + expect(isLockedToInstance({ PUBLIC_LOCK_TO_INSTANCE: 'no' })).toBe(false) + }) +}) + +describe('lockedInstanceOrigin', () => { + it('returns the public origin when locked', () => { + expect(lockedInstanceOrigin(BOTH)).toBe('https://coves.social') + }) + + it('normalises a bare host and drops any path', () => { + expect( + lockedInstanceOrigin({ PUBLIC_INSTANCE_URL: 'coves.social/app' }), + ).toBe('https://coves.social') + }) + + it('returns null when unlocked', () => { + expect( + lockedInstanceOrigin({ ...BOTH, PUBLIC_LOCK_TO_INSTANCE: 'false' }), + ).toBeNull() + }) + + it('returns null when there is nothing to pin to', () => { + expect(lockedInstanceOrigin({})).toBeNull() + expect(lockedInstanceOrigin({ PUBLIC_INSTANCE_URL: '::' })).toBeNull() + }) +}) diff --git a/src/lib/app/state/instance/resolve.ts b/src/lib/app/state/instance/resolve.ts index 3be9a1df..695db6c9 100644 --- a/src/lib/app/state/instance/resolve.ts +++ b/src/lib/app/state/instance/resolve.ts @@ -25,6 +25,7 @@ export interface InstanceEnv { readonly PUBLIC_INSTANCE_URL?: string readonly PUBLIC_INTERNAL_INSTANCE?: string + readonly PUBLIC_LOCK_TO_INSTANCE?: string } export type InstanceSide = 'browser' | 'server' @@ -85,6 +86,24 @@ export function instanceOrigin(raw: string | undefined): string | null { return new URL(normalized).origin } +/** + * Whether this deployment pins login to `PUBLIC_INSTANCE_URL`. + * Defaults to locked; only the literal `"false"` (any case) opens it up. + */ +export function isLockedToInstance(env: InstanceEnv): boolean { + return (env.PUBLIC_LOCK_TO_INSTANCE ?? 'true').toLowerCase() === 'true' +} + +/** + * The only origin login may target when the deployment is locked, or null + * when unlocked or when `PUBLIC_INSTANCE_URL` is unset/invalid (nothing to + * pin to, so callers must not enforce). + */ +export function lockedInstanceOrigin(env: InstanceEnv): string | null { + if (!isLockedToInstance(env)) return null + return instanceOrigin(env.PUBLIC_INSTANCE_URL) +} + /** * Host (with port) of `PUBLIC_INSTANCE_URL`, used in development to redirect * `localhost` to the `127.0.0.1` origin the OAuth cookie is scoped to. diff --git a/src/lib/server/instance.ts b/src/lib/server/instance.ts index ff0fd790..f0f58e82 100644 --- a/src/lib/server/instance.ts +++ b/src/lib/server/instance.ts @@ -10,6 +10,7 @@ import { addressHeaderWarning, canonicalPublicHost, isUpstreamSchemeAllowed, + lockedInstanceOrigin, MISSING_INSTANCE_MESSAGE, resolveInstanceUrl, } from '$lib/app/state/instance/resolve' @@ -55,6 +56,15 @@ export function addressHeaderConfigWarning(): string | null { ) } +/** + * When `PUBLIC_LOCK_TO_INSTANCE` is on, the sole origin login may target; + * null when unlocked or unconfigured. The login UI hides the instance field + * under the same flag — this is the server-side half of that policy. + */ +export function loginLockedOrigin(): string | null { + return lockedInstanceOrigin(publicEnv) +} + /** Applies the `ALLOW_HTTP_INTERNAL_INSTANCE` plaintext policy to a target URL. */ export function upstreamSchemeAllowed(target: string): boolean { return isUpstreamSchemeAllowed(target, { diff --git a/src/routes/api/auth/auth.test.ts b/src/routes/api/auth/auth.test.ts index ed7e88d2..ab50e004 100644 --- a/src/routes/api/auth/auth.test.ts +++ b/src/routes/api/auth/auth.test.ts @@ -14,6 +14,10 @@ vi.mock('$env/dynamic/private', () => ({ env: {}, })) +// Mutable so individual tests can flip the instance-lock policy. +const publicEnv = vi.hoisted((): Record => ({})) +vi.mock('$env/dynamic/public', () => ({ env: publicEnv })) + /** * Helper to create authenticated App.Locals with the new shape. */ @@ -46,6 +50,52 @@ global.fetch = mockFetch describe('POST /api/auth/login', () => { beforeEach(() => { vi.clearAllMocks() + for (const key of Object.keys(publicEnv)) delete publicEnv[key] + }) + + describe('PUBLIC_LOCK_TO_INSTANCE enforcement', () => { + const login = (instance: string) => + loginHandler( + createMockEvent({ + method: 'POST', + body: { handle: 'user.example.com', instance }, + cookies: createMockCookies(), + url: 'http://localhost:5173/api/auth/login', + }), + ) + + it('rejects a foreign instance with 403 when locked (the default)', async () => { + publicEnv.PUBLIC_INSTANCE_URL = 'https://coves.social' + + const response = await login('https://attacker.example') + + expect(response.status).toBe(403) + expect((await response.json()).error).toMatch(/own instance/) + }) + + it('compares origins, so a bare host or trailing path still matches', async () => { + publicEnv.PUBLIC_INSTANCE_URL = 'https://coves.social' + + expect((await login('coves.social')).status).toBe(200) + expect((await login('https://coves.social/')).status).toBe(200) + }) + + it('rejects a scheme downgrade of the locked instance', async () => { + publicEnv.PUBLIC_INSTANCE_URL = 'https://coves.social' + + expect((await login('http://coves.social')).status).toBe(403) + }) + + it('allows any instance when PUBLIC_LOCK_TO_INSTANCE=false', async () => { + publicEnv.PUBLIC_INSTANCE_URL = 'https://coves.social' + publicEnv.PUBLIC_LOCK_TO_INSTANCE = 'false' + + expect((await login('https://other.example')).status).toBe(200) + }) + + it('does not enforce when PUBLIC_INSTANCE_URL is unset', async () => { + expect((await login('https://other.example')).status).toBe(200) + }) }) it('returns OAuth redirect URL for valid handle/instance', async () => { diff --git a/src/routes/api/auth/login/+server.ts b/src/routes/api/auth/login/+server.ts index 3d290666..5cc0fb9a 100644 --- a/src/routes/api/auth/login/+server.ts +++ b/src/routes/api/auth/login/+server.ts @@ -4,6 +4,7 @@ import type { RequestHandler } from './$types' import { PENDING_AUTH_COOKIE_OPTIONS } from '$lib/server/cookies' import { generateOAuthState } from '$lib/server/csrf' import { normalizeInstanceUrl } from '$lib/app/state/instance/resolve' +import { loginLockedOrigin } from '$lib/server/instance' interface LoginRequest { handle: string @@ -51,13 +52,28 @@ export const POST: RequestHandler = async ({ } const instanceUrl = new URL(normalizedInstance) - // Built once: all four logged rejections below share this request context. + // Built once: every logged rejection below shares this request context. const logContext = { requestId: locals.requestId, method: request.method, path: url.pathname, } + // PUBLIC_LOCK_TO_INSTANCE is a deployment policy, not a UI preference: the + // login form hides the instance field, but this endpoint is reachable + // directly, so refuse to start OAuth against any other origin. + const lockedOrigin = loginLockedOrigin() + if (lockedOrigin !== null && instanceUrl.origin !== lockedOrigin) { + log.warn( + `[auth/login] Rejected login to non-locked instance: ${instanceUrl.origin}`, + logContext, + ) + return json( + { error: 'This deployment only allows login to its own instance' }, + { status: 403 }, + ) + } + // Validate redirect URL to prevent open redirect attacks // Only allow relative URLs (starting with /) or same-origin URLs let safeRedirect = '/' -- 2.51.2