assafvayner HF Staff commited on
Commit
857fbc2
·
1 Parent(s): c24efc8

fix(auth): gate admin routes on the decoded path and in the actions themselves

Browse files

The hooks gate tested the raw `event.url.pathname`, but SvelteKit matches
routes on the decoded path, so a logged-in member could reach /admin (and
reset the admin's password) via /%61dmin. Decode the pathname the same way
before every check, and stop relying on the route gate alone: each admin
load and action now calls requireAdmin, and the mutating actions require a
member target so the admin account can never be renamed, reset, or
deactivated from /admin.

src/hooks.server.ts CHANGED
@@ -1,4 +1,5 @@
1
  import { dev } from '$app/environment';
 
2
  import { SAFE_NEXT } from '$lib/server/auth/safe-next';
3
  import { SESSION_COOKIE, verifySessionToken } from '$lib/server/auth/session';
4
  import { getApp } from '$lib/server/app';
@@ -23,7 +24,14 @@ export const handle: Handle = async ({ event, resolve }) => {
23
  }
24
  }
25
 
26
- const path = event.url.pathname;
 
 
 
 
 
 
 
27
  const user = event.locals.user;
28
 
29
  if (!user) {
 
1
  import { dev } from '$app/environment';
2
+ import { decodePathname } from '$lib/server/auth/decode-path';
3
  import { SAFE_NEXT } from '$lib/server/auth/safe-next';
4
  import { SESSION_COOKIE, verifySessionToken } from '$lib/server/auth/session';
5
  import { getApp } from '$lib/server/app';
 
24
  }
25
  }
26
 
27
+ // SvelteKit matches routes on the decoded pathname, so every check below must use it too:
28
+ // gating on the raw pathname lets `/%61dmin` slip past the admin check and still hit /admin.
29
+ let path: string;
30
+ try {
31
+ path = decodePathname(event.url.pathname);
32
+ } catch {
33
+ error(400, 'Malformed URL');
34
+ }
35
  const user = event.locals.user;
36
 
37
  if (!user) {
src/lib/server/auth/decode-path.ts ADDED
@@ -0,0 +1,8 @@
 
 
 
 
 
 
 
 
 
1
+ /**
2
+ * Decode a URL pathname the way SvelteKit does before matching routes: every
3
+ * segment between literal `%25` sequences is decoded, so `/%61dmin` becomes
4
+ * `/admin`. Throws `URIError` on a malformed escape sequence.
5
+ */
6
+ export function decodePathname(pathname: string): string {
7
+ return pathname.split('%25').map(decodeURIComponent).join('%25');
8
+ }
src/lib/server/auth/guards.ts ADDED
@@ -0,0 +1,17 @@
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
1
+ import { error } from '@sveltejs/kit';
2
+ import type { User } from '../../domain/types';
3
+ import { UsersRepo, UserValidationError } from '../users/repository';
4
+
5
+ /** Every admin load/action calls this; the route gate in hooks is defence in depth, not the check. */
6
+ export function requireAdmin(locals: App.Locals): User {
7
+ const user = locals.user;
8
+ if (!user || user.role !== 'admin') error(403, 'Admins only');
9
+ return user;
10
+ }
11
+
12
+ /** Admin mutations may only target members: the admin's own account is managed via /me. */
13
+ export function requireMemberTarget(users: UsersRepo, id: string): User {
14
+ const user = users.byId(id);
15
+ if (!user || user.role !== 'member') throw new UserValidationError('Unknown user', 'id');
16
+ return user;
17
+ }
src/routes/admin/+page.server.ts CHANGED
@@ -1,11 +1,13 @@
1
  import { toPublicUser } from '$lib/domain/types';
 
2
  import { UserValidationError } from '$lib/server/users/repository';
3
  import { fail } from '@sveltejs/kit';
4
  import type { Actions, PageServerLoad } from './$types';
5
 
6
- export const load: PageServerLoad = ({ locals }) => ({
7
- members: locals.app.users.members().map(toPublicUser)
8
- });
 
9
 
10
  function handle<T, E extends Record<string, unknown>>(fn: () => Promise<T>, extra: E) {
11
  return fn().catch((e) => {
@@ -17,6 +19,7 @@ function handle<T, E extends Record<string, unknown>>(fn: () => Promise<T>, extr
17
 
18
  export const actions: Actions = {
19
  create: async ({ request, locals }) => {
 
20
  const form = await request.formData();
21
  const username = String(form.get('username') ?? '');
22
  const displayName = String(form.get('displayName') ?? '');
@@ -30,11 +33,13 @@ export const actions: Actions = {
30
  );
31
  },
32
  rename: async ({ request, locals }) => {
 
33
  const form = await request.formData();
34
  const id = String(form.get('id') ?? '');
35
  const displayName = String(form.get('displayName') ?? '');
36
  return handle(
37
  async () => {
 
38
  await locals.app.users.rename(id, displayName);
39
  return { renamed: id };
40
  },
@@ -42,11 +47,13 @@ export const actions: Actions = {
42
  );
43
  },
44
  resetPassword: async ({ request, locals }) => {
 
45
  const form = await request.formData();
46
  const id = String(form.get('id') ?? '');
47
  const password = String(form.get('password') ?? '');
48
  return handle(
49
  async () => {
 
50
  await locals.app.users.setPassword(id, password, { mustChange: true });
51
  return { reset: id };
52
  },
@@ -54,11 +61,16 @@ export const actions: Actions = {
54
  );
55
  },
56
  setActive: async ({ request, locals }) => {
 
57
  const form = await request.formData();
58
  const id = String(form.get('id') ?? '');
59
- const active = form.get('active') === 'true';
 
 
 
60
  return handle(
61
  async () => {
 
62
  await locals.app.users.setActive(id, active);
63
  return { toggled: id };
64
  },
 
1
  import { toPublicUser } from '$lib/domain/types';
2
+ import { requireAdmin, requireMemberTarget } from '$lib/server/auth/guards';
3
  import { UserValidationError } from '$lib/server/users/repository';
4
  import { fail } from '@sveltejs/kit';
5
  import type { Actions, PageServerLoad } from './$types';
6
 
7
+ export const load: PageServerLoad = ({ locals }) => {
8
+ requireAdmin(locals);
9
+ return { members: locals.app.users.members().map(toPublicUser) };
10
+ };
11
 
12
  function handle<T, E extends Record<string, unknown>>(fn: () => Promise<T>, extra: E) {
13
  return fn().catch((e) => {
 
19
 
20
  export const actions: Actions = {
21
  create: async ({ request, locals }) => {
22
+ requireAdmin(locals);
23
  const form = await request.formData();
24
  const username = String(form.get('username') ?? '');
25
  const displayName = String(form.get('displayName') ?? '');
 
33
  );
34
  },
35
  rename: async ({ request, locals }) => {
36
+ requireAdmin(locals);
37
  const form = await request.formData();
38
  const id = String(form.get('id') ?? '');
39
  const displayName = String(form.get('displayName') ?? '');
40
  return handle(
41
  async () => {
42
+ requireMemberTarget(locals.app.users, id);
43
  await locals.app.users.rename(id, displayName);
44
  return { renamed: id };
45
  },
 
47
  );
48
  },
49
  resetPassword: async ({ request, locals }) => {
50
+ requireAdmin(locals);
51
  const form = await request.formData();
52
  const id = String(form.get('id') ?? '');
53
  const password = String(form.get('password') ?? '');
54
  return handle(
55
  async () => {
56
+ requireMemberTarget(locals.app.users, id);
57
  await locals.app.users.setPassword(id, password, { mustChange: true });
58
  return { reset: id };
59
  },
 
61
  );
62
  },
63
  setActive: async ({ request, locals }) => {
64
+ requireAdmin(locals);
65
  const form = await request.formData();
66
  const id = String(form.get('id') ?? '');
67
+ const raw = form.get('active');
68
+ if (raw !== 'true' && raw !== 'false')
69
+ return fail(400, { action: 'setActive', id, error: 'Invalid request', field: 'active' });
70
+ const active = raw === 'true';
71
  return handle(
72
  async () => {
73
+ requireMemberTarget(locals.app.users, id);
74
  await locals.app.users.setActive(id, active);
75
  return { toggled: id };
76
  },
src/routes/admin/+page.svelte CHANGED
@@ -6,6 +6,15 @@
6
  let { data, form }: PageProps = $props();
7
  const errorFor = (action: string, id?: string) =>
8
  form?.action === action && (id === undefined || form?.id === id) ? form?.error : undefined;
 
 
 
 
 
 
 
 
 
9
  </script>
10
 
11
  <main class="space-y-6 p-4">
@@ -14,6 +23,8 @@
14
  <a href="/admin/storage" class="text-sm text-green-700 underline">Storage</a>
15
  </header>
16
 
 
 
17
  <section class="rounded-xl bg-white p-4 shadow-sm">
18
  <h2 class="mb-3 font-semibold">Add user</h2>
19
  <form method="POST" action="?/create" use:enhance class="space-y-3">
 
6
  let { data, form }: PageProps = $props();
7
  const errorFor = (action: string, id?: string) =>
8
  form?.action === action && (id === undefined || form?.id === id) ? form?.error : undefined;
9
+ // An error whose id matches no listed member has no inline slot to appear in; show it here
10
+ // rather than letting the failure pass silently.
11
+ const orphanError = $derived(
12
+ form?.error &&
13
+ form?.action !== 'create' &&
14
+ !data.members.some((m) => m.id === form?.id)
15
+ ? form.error
16
+ : undefined
17
+ );
18
  </script>
19
 
20
  <main class="space-y-6 p-4">
 
23
  <a href="/admin/storage" class="text-sm text-green-700 underline">Storage</a>
24
  </header>
25
 
26
+ <FormError message={orphanError} />
27
+
28
  <section class="rounded-xl bg-white p-4 shadow-sm">
29
  <h2 class="mb-3 font-semibold">Add user</h2>
30
  <form method="POST" action="?/create" use:enhance class="space-y-3">
src/routes/admin/storage/+page.server.ts CHANGED
@@ -1,7 +1,9 @@
 
1
  import { fail } from '@sveltejs/kit';
2
  import type { Actions, PageServerLoad } from './$types';
3
 
4
  export const load: PageServerLoad = ({ locals }) => {
 
5
  const { ledger, compaction, config, guard } = locals.app;
6
  return {
7
  stats: ledger.stats(),
@@ -17,11 +19,13 @@ export const load: PageServerLoad = ({ locals }) => {
17
 
18
  export const actions: Actions = {
19
  compact: async ({ locals }) => {
 
20
  try {
21
  const result = await locals.app.runCompaction();
22
  return { result };
23
  } catch (e) {
24
- return fail(500, { error: e instanceof Error ? e.message : String(e) });
 
25
  }
26
  }
27
  };
 
1
+ import { requireAdmin } from '$lib/server/auth/guards';
2
  import { fail } from '@sveltejs/kit';
3
  import type { Actions, PageServerLoad } from './$types';
4
 
5
  export const load: PageServerLoad = ({ locals }) => {
6
+ requireAdmin(locals);
7
  const { ledger, compaction, config, guard } = locals.app;
8
  return {
9
  stats: ledger.stats(),
 
19
 
20
  export const actions: Actions = {
21
  compact: async ({ locals }) => {
22
+ requireAdmin(locals);
23
  try {
24
  const result = await locals.app.runCompaction();
25
  return { result };
26
  } catch (e) {
27
+ console.error('compaction failed', e);
28
+ return fail(500, { error: 'Compaction failed; see server logs' });
29
  }
30
  }
31
  };
tests/e2e/smoke.spec.ts CHANGED
@@ -138,6 +138,16 @@ test('settle up records a payment and clears the debt; delete works', async ({ p
138
  await login(page, 'bob', 'temppass1');
139
  await changePassword(page, 'temppass1', 'bobpass123');
140
 
 
 
 
 
 
 
 
 
 
 
141
  await page.goto('/balances');
142
  await expect(page.getByText('You owe', { exact: true })).toBeVisible();
143
  await page.getByRole('link', { name: 'Settle up' }).click();
 
138
  await login(page, 'bob', 'temppass1');
139
  await changePassword(page, 'temppass1', 'bobpass123');
140
 
141
+ // A member cannot reach the admin routes, not even through a percent-encoded path that
142
+ // SvelteKit decodes back to /admin before matching.
143
+ expect((await page.request.get('/%61dmin')).status()).toBe(403);
144
+ expect((await page.request.get('/%61dmin/storage')).status()).toBe(403);
145
+ const stolen = await page.request.post('/%61dmin?/resetPassword', {
146
+ form: { id: 'u_admin', password: 'ownedadmin1' },
147
+ headers: { origin: 'http://localhost:4173' }
148
+ });
149
+ expect(stolen.status()).toBe(403);
150
+
151
  await page.goto('/balances');
152
  await expect(page.getByText('You owe', { exact: true })).toBeVisible();
153
  await page.getByRole('link', { name: 'Settle up' }).click();
tests/unit/auth/guards.test.ts ADDED
@@ -0,0 +1,75 @@
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
1
+ import { mkdtemp, rm } from 'node:fs/promises';
2
+ import { tmpdir } from 'node:os';
3
+ import { join } from 'node:path';
4
+ import { afterEach, beforeEach, describe, expect, it } from 'vitest';
5
+ import { decodePathname } from '../../../src/lib/server/auth/decode-path';
6
+ import { requireAdmin, requireMemberTarget } from '../../../src/lib/server/auth/guards';
7
+ import { LocalFsStore } from '../../../src/lib/server/storage/local-fs-store';
8
+ import { UsersRepo, UserValidationError } from '../../../src/lib/server/users/repository';
9
+ import type { User } from '../../../src/lib/domain/types';
10
+
11
+ const localsFor = (user: User | null) => ({ user }) as unknown as App.Locals;
12
+
13
+ describe('requireAdmin', () => {
14
+ const admin = { id: 'u_admin', role: 'admin' } as User;
15
+ const member = { id: 'u_bob', role: 'member' } as User;
16
+
17
+ it('returns the admin', () => {
18
+ expect(requireAdmin(localsFor(admin))).toBe(admin);
19
+ });
20
+
21
+ it('rejects members and anonymous requests with 403', () => {
22
+ for (const user of [member, null]) {
23
+ expect(() => requireAdmin(localsFor(user))).toThrowError(
24
+ expect.objectContaining({ status: 403 })
25
+ );
26
+ }
27
+ });
28
+ });
29
+
30
+ describe('requireMemberTarget', () => {
31
+ let dir: string;
32
+ let repo: UsersRepo;
33
+ let bobId: string;
34
+ const clock = () => new Date('2026-08-29T12:00:00.000Z');
35
+
36
+ beforeEach(async () => {
37
+ dir = await mkdtemp(join(tmpdir(), 'splitwise-guards-'));
38
+ repo = await UsersRepo.load(new LocalFsStore(dir), { clock });
39
+ bobId = (await repo.create({ username: 'bob', displayName: 'Bob', password: 'temppass1' })).id;
40
+ });
41
+ afterEach(() => rm(dir, { recursive: true, force: true }));
42
+
43
+ it('returns the member', () => {
44
+ expect(requireMemberTarget(repo, bobId).username).toBe('bob');
45
+ });
46
+
47
+ it('rejects unknown ids and the admin account', () => {
48
+ for (const id of ['', 'u_nope', 'u_admin']) {
49
+ expect(() => requireMemberTarget(repo, id)).toThrowError(UserValidationError);
50
+ try {
51
+ requireMemberTarget(repo, id);
52
+ } catch (e) {
53
+ expect((e as UserValidationError).message).toBe('Unknown user');
54
+ expect((e as UserValidationError).field).toBe('id');
55
+ }
56
+ }
57
+ });
58
+ });
59
+
60
+ describe('decodePathname', () => {
61
+ it('decodes escapes the way SvelteKit matches routes', () => {
62
+ expect(decodePathname('/%61dmin')).toBe('/admin');
63
+ expect(decodePathname('/%61dmin/storage')).toBe('/admin/storage');
64
+ expect(decodePathname('/a%2Fb')).toBe('/a/b');
65
+ expect(decodePathname('/admin')).toBe('/admin');
66
+ });
67
+
68
+ it('leaves an encoded percent sign intact', () => {
69
+ expect(decodePathname('/x%25y')).toBe('/x%25y');
70
+ });
71
+
72
+ it('throws on a malformed escape', () => {
73
+ expect(() => decodePathname('/%zz')).toThrowError(URIError);
74
+ });
75
+ });