From 8b3f5731791ac1e9343265e7a8d6eb0fbe6159c5 Mon Sep 17 00:00:00 2001 From: FrameAutomata Date: Thu, 27 Aug 2026 11:43:06 -0500 Subject: [PATCH] fix: stop resolveHref/gotoHref throwing on bracketed paths resolve() from $app/paths takes a route id and populates its [param] segments by dereferencing a params argument. Both helpers handed it a concrete pathname and no params, so any path containing brackets threw `TypeError: Cannot read properties of undefined (reading '')`. safeLocalPath lets brackets through, so /login?returnTo=%2Fa%5Bb%5Dc reaches it. In login/+page.svelte the throw lands after the token is stored, and the form's own catch renders it as a credentials error -- the user ends up logged in, still on the login page, reading a TypeError. auth/callback has the same shape. These hrefs are already concrete, so they need the base path, not route resolution. That is also what goto() documents wanting for root-relative URLs. gotoHref now delegates to resolveHref rather than repeating it, and finish-setup routes its SSO returnTo through gotoHref instead of open-coding a bare goto() that drops the base path. Also widens the passthrough guard, which required '//' and so let scheme-only URIs through to resolve(). That is not merely a missing base prefix: resolve_route does route.slice(1) unconditionally, so it ate the first character -- 'mailto:x@y.z' came back as '/ailto:x@y.z'. Latent today; nothing in src/ passes one, but button.svelte and otel-setup-steps.svelte forward hrefs they do not control. The $app/paths test stub returned its argument unchanged, which is why no test ever saw this. It is replaced by SvelteKit's real client module so the new tests fail on the unfixed code. Closes #325 --- frontend/src/lib/utils/links.test.ts | 34 +++++++++++++++++++ frontend/src/lib/utils/links.ts | 11 +++--- frontend/src/lib/utils/navigation.test.ts | 14 +++++++- frontend/src/lib/utils/navigation.ts | 10 +++--- frontend/src/routes/finish-setup/+page.svelte | 4 +-- frontend/src/test/mocks/app-navigation.ts | 5 +++ frontend/src/test/mocks/app-paths.ts | 6 ---- frontend/vitest.config.ts | 14 +++++++- 8 files changed, 78 insertions(+), 20 deletions(-) create mode 100644 frontend/src/lib/utils/links.test.ts create mode 100644 frontend/src/test/mocks/app-navigation.ts delete mode 100644 frontend/src/test/mocks/app-paths.ts diff --git a/frontend/src/lib/utils/links.test.ts b/frontend/src/lib/utils/links.test.ts new file mode 100644 index 00000000..03123a4e --- /dev/null +++ b/frontend/src/lib/utils/links.test.ts @@ -0,0 +1,34 @@ +import { describe, expect, it } from 'vitest'; + +import { resolveHref } from './links'; + +describe('resolveHref', () => { + // resolve() from $app/paths reads these as route ids and dereferences a + // params argument resolveHref never had, throwing a TypeError. Any returnTo= + // reaches this, so a login could land the user logged in but stranded on the + // login page reading the TypeError as a credentials error. + it('passes bracketed paths through instead of throwing', () => { + expect(resolveHref('/a[b]c')).toBe('/a[b]c'); + expect(resolveHref('/tasks/[task]')).toBe('/tasks/[task]'); + expect(resolveHref('/issues/[hash]/events?preset=24h')).toBe( + '/issues/[hash]/events?preset=24h' + ); + }); + + it('keeps ordinary app paths and their suffixes intact', () => { + expect(resolveHref('/issues/abc123')).toBe('/issues/abc123'); + expect(resolveHref('/issues?projectId=one')).toBe('/issues?projectId=one'); + expect(resolveHref('/monitors#incidents')).toBe('/monitors#incidents'); + expect(resolveHref('')).toBe('/'); + }); + + it('leaves anything that is not an app path alone', () => { + expect(resolveHref('https://example.com/a')).toBe('https://example.com/a'); + expect(resolveHref('//example.com/a')).toBe('//example.com/a'); + expect(resolveHref('#section')).toBe('#section'); + // button.svelte and otel-setup-steps.svelte forward hrefs they do not + // control; a scheme-only URI must not have the base path glued onto it. + expect(resolveHref('mailto:support@example.com')).toBe('mailto:support@example.com'); + expect(resolveHref('tel:+15551234')).toBe('tel:+15551234'); + }); +}); diff --git a/frontend/src/lib/utils/links.ts b/frontend/src/lib/utils/links.ts index abd39414..d3126e9a 100644 --- a/frontend/src/lib/utils/links.ts +++ b/frontend/src/lib/utils/links.ts @@ -1,13 +1,16 @@ -import { resolve } from '$app/paths'; +import { base } from '$app/paths'; -export function splitHref(href: string): { pathname: string; suffix: string } { +function splitHref(href: string): { pathname: string; suffix: string } { const suffixIndex = href.search(/[?#]/); if (suffixIndex === -1) return { pathname: href, suffix: '' }; return { pathname: href.slice(0, suffixIndex), suffix: href.slice(suffixIndex) }; } +// Not resolve() from $app/paths: that reads its argument as a route id and +// throws on the literal brackets a concrete pathname can carry. Prefixing the +// base path is what a concrete href needs instead. export function resolveHref(href: string): string { - if (/^(?:[a-z][a-z\d+.-]*:)?\/\//i.test(href) || href.startsWith('#')) return href; + if (/^(?:[a-z][a-z\d+.-]*:|\/\/|#)/i.test(href)) return href; const { pathname, suffix } = splitHref(href); - return resolve((pathname || '/') as '/') + suffix; + return base + (pathname || '/') + suffix; } diff --git a/frontend/src/lib/utils/navigation.test.ts b/frontend/src/lib/utils/navigation.test.ts index 2dff2e8e..424b53a5 100644 --- a/frontend/src/lib/utils/navigation.test.ts +++ b/frontend/src/lib/utils/navigation.test.ts @@ -1,6 +1,18 @@ -import { describe, expect, it } from 'vitest'; +import { describe, expect, it, vi } from 'vitest'; + +const goto = vi.hoisted(() => vi.fn()); +vi.mock('$app/navigation', () => ({ goto })); import { authenticatedLandingPath, defaultAuthenticatedPath } from './landing'; +import { gotoHref } from './navigation'; + +describe('gotoHref', () => { + it('navigates to a bracketed path instead of throwing', async () => { + await gotoHref('/a[b]c?returnTo=1', { replaceState: true }); + + expect(goto).toHaveBeenCalledWith('/a[b]c?returnTo=1', { replaceState: true }); + }); +}); describe('defaultAuthenticatedPath', () => { it('opens the first organization when it has multiple projects', () => { diff --git a/frontend/src/lib/utils/navigation.ts b/frontend/src/lib/utils/navigation.ts index 6ac4eb85..7f685df4 100644 --- a/frontend/src/lib/utils/navigation.ts +++ b/frontend/src/lib/utils/navigation.ts @@ -1,15 +1,13 @@ import { goto } from '$app/navigation'; -import { resolve } from '$app/paths'; -import { splitHref } from './links'; +import { resolveHref } from './links'; export { authenticatedLandingPath, defaultAuthenticatedPath, safeLocalPath } from './landing'; export function gotoHref(href: string, options?: Parameters[1]) { - const { pathname, suffix } = splitHref(href); - let destination = resolve((pathname || '/') as '/'); - destination += suffix; - return goto(destination, options); + // The rule only recognises a literal resolve() at the call site. + // eslint-disable-next-line svelte/no-navigation-without-resolve + return goto(resolveHref(href), options); } // SSO logins bounce through the provider and land on /auth/callback, losing diff --git a/frontend/src/routes/finish-setup/+page.svelte b/frontend/src/routes/finish-setup/+page.svelte index d418b51f..76e03ed5 100644 --- a/frontend/src/routes/finish-setup/+page.svelte +++ b/frontend/src/routes/finish-setup/+page.svelte @@ -18,7 +18,7 @@ import { authState } from '$lib/state/auth.svelte'; import { projectsState } from '$lib/state/projects.svelte'; import { themeState } from '$lib/state/theme.svelte'; - import { consumeSsoReturnTo, safeLocalPath } from '$lib/utils/navigation'; + import { consumeSsoReturnTo, gotoHref, safeLocalPath } from '$lib/utils/navigation'; import SetupProjectsStep from '$lib/components/setup/setup-projects-step.svelte'; let phase = $state<'organization' | 'projects'>('organization'); @@ -63,7 +63,7 @@ returnTo = safeLocalPath(consumeSsoReturnTo()); if (newOrgId === null) { - goto(returnTo); + gotoHref(returnTo); return; } phase = 'projects'; diff --git a/frontend/src/test/mocks/app-navigation.ts b/frontend/src/test/mocks/app-navigation.ts new file mode 100644 index 00000000..8dd432d2 --- /dev/null +++ b/frontend/src/test/mocks/app-navigation.ts @@ -0,0 +1,5 @@ +// goto() needs a mounted router it cannot get in jsdom, so tests replace this. +// Throwing rather than no-opping keeps an unmocked navigation from passing. +export function goto(): Promise { + throw new Error('goto() reached the $app/navigation test stub; mock it in the test'); +} diff --git a/frontend/src/test/mocks/app-paths.ts b/frontend/src/test/mocks/app-paths.ts deleted file mode 100644 index 57529a83..00000000 --- a/frontend/src/test/mocks/app-paths.ts +++ /dev/null @@ -1,6 +0,0 @@ -export const base = ''; -export const assets = ''; - -export function resolve(path: string): string { - return path; -} diff --git a/frontend/vitest.config.ts b/frontend/vitest.config.ts index 850d43fb..7008a62c 100644 --- a/frontend/vitest.config.ts +++ b/frontend/vitest.config.ts @@ -4,11 +4,23 @@ import path from 'path'; export default defineConfig({ plugins: [svelte({ hot: false })], + // These tests run on plain svelte(), not sveltekit(), so nothing supplies the + // $app modules. $app/paths points at SvelteKit's real implementation rather + // than a stub because an identity stub is what kept the suite blind to #325, + // and would keep it blind to the next resolve() misuse. Nothing under test + // calls resolve() today. These are the globals that module reads. + define: { + __SVELTEKIT_PAYLOAD__: 'undefined', + __SVELTEKIT_PATHS_BASE__: '""', + __SVELTEKIT_APP_DIR__: '"_app"', + __SVELTEKIT_HASH_ROUTING__: 'false' + }, resolve: { conditions: ['browser'], alias: { $lib: path.resolve('./src/lib'), - '$app/paths': path.resolve('./src/test/mocks/app-paths.ts') + '$app/paths': path.resolve('./node_modules/@sveltejs/kit/src/runtime/app/paths/client.js'), + '$app/navigation': path.resolve('./src/test/mocks/app-navigation.ts') } }, test: {