diff --git a/src/hooks.server.test.ts b/src/hooks.server.test.ts --- a/src/hooks.server.test.ts +++ b/src/hooks.server.test.ts @@ -162,6 +162,7 @@ await handle({ event, resolve }) expect(mockFetch).toHaveBeenCalledWith('http://localhost:4000/api/me', { headers: { Cookie: 'coves_session=sealed-token-value' }, + signal: expect.any(AbortSignal), }) expect(event.locals.auth.authenticated).toBe(true) if (event.locals.auth.authenticated) { @@ -308,6 +309,33 @@ warnSpy.mockRestore() }) + it('classifies TimeoutError and AbortError as network errors by name', async () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) + + for (const name of ['TimeoutError', 'AbortError']) { + warnSpy.mockClear() + mockFetch.mockRejectedValue(new DOMException('operation failed', name)) + + const cookies = createMockCookies({ + coves_session: 'sealed-token-value', + }) + const event = createMockEvent({ cookies }) + const resolve = createMockResolve() + + await handle({ event, resolve }) + + expect(event.locals.auth.authenticated).toBe(false) + expect(event.locals.authError).toBe('network_error') + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining('Network error calling /api/me'), + expect.any(DOMException), + ) + expect(cookies.delete).not.toHaveBeenCalled() + } + + warnSpy.mockRestore() + }) + it('does not delete the coves_session cookie on network error', async () => { vi.spyOn(console, 'warn').mockImplementation(() => {}) @@ -515,6 +543,7 @@ expect(mockFetch).toHaveBeenCalledWith( 'https://coves.example.com/api/me', { headers: { Cookie: 'coves_session=sealed-token-value' }, + signal: expect.any(AbortSignal), }, ) expect(event.locals.auth.authenticated).toBe(true) @@ -676,5 +705,104 @@ message: 'Forbidden', }) expect(result).toEqual({ message: 'An unexpected error occurred' }) + }) + + describe('log sanitization', () => { + const sealedToken = 'sealed-session-token-v1.super-secret-value' + + /** + * Creates an event carrying the sealed session token in both places it + * lives on a real authenticated request: the Cookie header and locals.auth. + */ + function createEventWithToken(): RequestEvent { + const cookies = createMockCookies({ coves_session: sealedToken }) + const event = createMockEvent({ + cookies, + locals: { + auth: { + authenticated: true, + account: { + did: 'did:plc:test123', + handle: 'test.example.com', + pdsUrl: 'https://pds.example.com', + sealedToken, + }, + authToken: sealedToken, + }, + } as unknown as App.Locals, + }) + // Replace the bare request with one that includes the session cookie + // header, as the real server would receive it. + Object.assign(event, { + request: new Request(event.url, { + headers: { Cookie: `coves_session=${sealedToken}` }, + }), + }) + return event + } + + /** Best-effort string form of a logged argument for content assertions. */ + function stringifyLoggedArg(arg: unknown): string { + if (typeof arg === 'string') return arg + try { + return JSON.stringify(arg) ?? String(arg) + } catch { + return String(arg) + } + } + + it('never logs the event object or the session token for non-404 errors', async () => { + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + + const event = createEventWithToken() + await handleError({ + error: new Error('Internal database connection failed'), + event, + status: 500, + message: 'Internal Server Error', + }) + + expect(errorSpy).toHaveBeenCalled() + for (const call of errorSpy.mock.calls) { + for (const arg of call) { + expect(arg).not.toBe(event) + expect(stringifyLoggedArg(arg)).not.toContain(sealedToken) + } + } + + errorSpy.mockRestore() + }) + + it('does not call console.error for 404 errors', async () => { + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + + await handleError({ + error: new Error('Page not found'), + event: createEventWithToken(), + status: 404, + message: 'Not Found', + }) + + expect(errorSpy).not.toHaveBeenCalled() + + errorSpy.mockRestore() + }) + + it('logs the error stack for diagnostics', async () => { + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + + const error = new Error('boom') + await handleError({ + error, + event: createEventWithToken(), + status: 500, + message: 'Internal Server Error', + }) + + expect(error.stack).toBeDefined() + expect(errorSpy).toHaveBeenCalledWith(error.stack) + + errorSpy.mockRestore() + }) }) }) diff --git a/src/hooks.server.ts b/src/hooks.server.ts --- a/src/hooks.server.ts +++ b/src/hooks.server.ts @@ -38,6 +38,13 @@ * error type alone, to avoid misclassifying programming bugs as transient network errors. */ function isNetworkError(error: unknown): boolean { if (error instanceof Error) { + // AbortSignal.timeout() rejects with a DOMException named 'TimeoutError'; + // an aborted fetch rejects with 'AbortError'. Match the structured name + // rather than message substrings so programming bugs that merely mention + // "timeout"/"abort" in their message are not misclassified. + if (error.name === 'TimeoutError' || error.name === 'AbortError') { + return true + } const msg = error.message.toLowerCase() return ( msg.includes('fetch failed') || @@ -97,6 +104,9 @@ const response = await fetch(`${instance}/api/me`, { headers: { Cookie: `coves_session=${covesSession}`, }, + // A hung backend must not pile up requests until the Node process + // exhausts sockets — this fetch runs on every authenticated request. + signal: AbortSignal.timeout(10_000), }) if (!response.ok) { @@ -170,11 +180,12 @@ if (status == 404) { return { message: 'Not found' } } - console.error(`An error was captured:`) - console.error(error) - console.error(`Event:`, event) - console.error(`Status:`, status) - console.error(`Message:`, message) + // Log only safe request context — never the full event, which contains the + // session cookie and sealed auth token (locals.auth.authToken). + console.error( + `[hooks] Error captured: ${event.request.method} ${event.url.pathname} (status ${status}): ${message}`, + ) + console.error(error instanceof Error ? (error.stack ?? error.message) : error) return { message: 'An unexpected error occurred' } } diff --git a/src/lib/app/i18n/en.json b/src/lib/app/i18n/en.json --- a/src/lib/app/i18n/en.json +++ b/src/lib/app/i18n/en.json @@ -821,7 +821,8 @@ "settingsImport": "Successfully imported settings", "settingsImportWarning": "The imported settings don't seem valid. Are you sure you want to import this?", "userLoading": "Still loading your user data...", "lemmyDonate": "Your account's server runs Lemmy, and the developers are requesting donations. They are able to develop Lemmy as an open source platform, free of tracking and ads, thanks to the generosity of its users.\n\nAnnually, they ask you to consider donating to support their work, and allow them to continue maintaining and improving Lemmy.\n\n[Donate](https://join-lemmy.org/donate)\n\n*Note: this is a donation to Lemmy, not Kelp.*", - "sessionExpired": "Your session has expired. Please log in again." + "sessionExpired": "Your session has expired. Please log in again.", + "serverUnreachable": "Can't reach the server right now. You may appear logged out, but your session is preserved." }, "settings": { "title": "Settings", diff --git a/src/lib/app/instance.svelte.ts b/src/lib/app/instance.svelte.ts --- a/src/lib/app/instance.svelte.ts +++ b/src/lib/app/instance.svelte.ts @@ -1,4 +1,4 @@ -import { browser } from '$app/environment' +import { browser, dev } from '$app/environment' import { env } from '$env/dynamic/public' import { profile } from './auth.svelte' @@ -18,13 +18,23 @@ ? env.PUBLIC_INSTANCE_URL : undefined const getDefaultInstance = (): string => { - if (browser) { - return env.PUBLIC_INSTANCE_URL || 'lemdro.id' - } else { - return ( - env.PUBLIC_INTERNAL_INSTANCE || env.PUBLIC_INSTANCE_URL || 'lemdro.id' + // The instance URL must never default to a third-party host. In production + // the server fails fast when PUBLIC_INSTANCE_URL is missing — even if + // PUBLIC_INTERNAL_INSTANCE is set, because the browser can only ever see + // PUBLIC_INSTANCE_URL, so an internal-only config would leave every client + // without an instance. In dev and in the browser we return '' instead of + // throwing; server-side consumers (e.g. the API proxy) treat empty as a + // hard config error. + if (!browser && !dev && !env.PUBLIC_INSTANCE_URL) { + throw new Error( + '[instance] PUBLIC_INSTANCE_URL is required in production. Set PUBLIC_INSTANCE_URL (PUBLIC_INTERNAL_INSTANCE is optional on top of it).', ) } + + const configured = browser + ? env.PUBLIC_INSTANCE_URL + : env.PUBLIC_INTERNAL_INSTANCE || env.PUBLIC_INSTANCE_URL + return configured || '' } export const DEFAULT_INSTANCE_URL = getDefaultInstance() diff --git a/src/lib/app/markdown/renderers/MdLink.svelte b/src/lib/app/markdown/renderers/MdLink.svelte --- a/src/lib/app/markdown/renderers/MdLink.svelte +++ b/src/lib/app/markdown/renderers/MdLink.svelte @@ -1,6 +1,5 @@ - +{#if safe} + + {@render children?.()} + +{:else} {@render children?.()} - +{/if} diff --git a/src/lib/app/markdown/renderers/plugins.js b/src/lib/app/markdown/renderers/plugins.js --- a/src/lib/app/markdown/renderers/plugins.js +++ b/src/lib/app/markdown/renderers/plugins.js @@ -123,6 +123,27 @@ // "implicit user mention" rewrite turned every real email link (e.g. // support@coves.social on /legal) into a dead /profile/ link. } +// Markdown link targets are untrusted user content. Enforce a protocol +// allowlist here rather than relying on upstream regex stripping: the URL +// parser normalizes tricks like embedded tabs in "java\tscript:" that +// pattern-based blocklists miss. Relative links resolve against the base +// and come out as https:, so they pass. +export const SAFE_PROTOCOLS = new Set(['http:', 'https:', 'mailto:']) + +/** + * Whether a markdown link href is safe to render as an anchor. + * @param {string} href + * @returns {boolean} + */ +export const isSafeHref = (href) => { + if (!href) return false + try { + return SAFE_PROTOCOLS.has(new URL(href, 'https://base.invalid').protocol) + } catch { + return false + } +} + export function subSupscriptExtension(tokensExtractor) { return { name: 'subscriptSuperscript', diff --git a/src/lib/app/markdown/renderers/plugins.test.ts b/src/lib/app/markdown/renderers/plugins.test.ts --- a/src/lib/app/markdown/renderers/plugins.test.ts +++ b/src/lib/app/markdown/renderers/plugins.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from 'vitest' -import { localizeLink, CONTENT_REGEXES } from './plugins' +import { localizeLink, isSafeHref, CONTENT_REGEXES } from './plugins' // --------------------------------------------------------------------------- // localizeLink() - user links @@ -90,6 +90,72 @@ it('returns undefined for an external comment link (legacy route removed)', () => { const result = localizeLink('https://lemmy.world/comment/6789') expect(result).toBeUndefined() + }) +}) + +// --------------------------------------------------------------------------- +// isSafeHref() - protocol allowlist for untrusted markdown link targets +// --------------------------------------------------------------------------- + +describe('isSafeHref', () => { + it('rejects javascript: URLs', () => { + expect(isSafeHref('javascript:alert(1)')).toBe(false) + }) + + it('rejects javascript: with an embedded tab (parser strips it)', () => { + // The exact bypass this defends against: the URL parser normalizes + // "java\tscript:" back to "javascript:", which regex blocklists miss. + expect(isSafeHref('java\tscript:alert(1)')).toBe(false) + }) + + it('rejects javascript: with an embedded newline (parser strips it)', () => { + expect(isSafeHref('java\nscript:alert(1)')).toBe(false) + }) + + it('rejects mixed-case javascript: URLs', () => { + expect(isSafeHref('JaVaScRiPt:alert(1)')).toBe(false) + }) + + it('rejects data: URLs', () => { + expect(isSafeHref('data:text/html,x')).toBe(false) + }) + + it('rejects vbscript: URLs', () => { + expect(isSafeHref('vbscript:x')).toBe(false) + }) + + it('accepts https: URLs', () => { + expect(isSafeHref('https://example.com')).toBe(true) + }) + + it('accepts http: URLs', () => { + expect(isSafeHref('http://example.com')).toBe(true) + }) + + it('accepts mailto: URLs', () => { + expect(isSafeHref('mailto:a@b.c')).toBe(true) + }) + + it('accepts relative paths (resolved against the base)', () => { + expect(isSafeHref('/c/technology')).toBe(true) + }) + + it('accepts fragment-only links', () => { + expect(isSafeHref('#section')).toBe(true) + }) + + it('accepts query-only links', () => { + expect(isSafeHref('?query=1')).toBe(true) + }) + + it('accepts protocol-relative URLs (intentional: resolves to https:)', () => { + // "//evil.example" resolves against the https: base, so its protocol is + // https: — an ordinary external link, safe to render as an anchor. + expect(isSafeHref('//evil.example')).toBe(true) + }) + + it('rejects the empty string', () => { + expect(isSafeHref('')).toBe(false) }) }) diff --git a/src/lib/feature/community/CommunityFlair.svelte b/src/lib/feature/community/CommunityFlair.svelte deleted file mode 100644 --- a/src/lib/feature/community/CommunityFlair.svelte +++ /dev/null @@ -1,46 +0,0 @@ - - -
diff --git a/src/lib/feature/moderation/BanModal.svelte b/src/lib/feature/moderation/BanModal.svelte deleted file mode 100644 --- a/src/lib/feature/moderation/BanModal.svelte +++ /dev/null @@ -1,134 +0,0 @@ - - -