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: {