fix(persons): guard review form actions with hasWriteAll server-side
The four form actions on /persons/review (confirm, delete, merge, rename) had no server-side permission check — a reader with a hand- crafted POST could trigger writes that the backend then rejected with FORBIDDEN, but only after the round-trip. Add the existing hasWriteAll guard at the top of each action and short-circuit with fail(403, FORBIDDEN). Mirrors the guard pattern in the rest of the persons domain (review-only writers must be gated client-side AND server-side). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
@@ -31,7 +31,10 @@ export async function load({ url, fetch, locals }) {
|
||||
}
|
||||
|
||||
export const actions = {
|
||||
confirm: async ({ request, fetch }) => {
|
||||
confirm: async ({ request, fetch, locals }) => {
|
||||
if (!hasWriteAll(locals)) {
|
||||
return fail(403, { error: getErrorMessage('FORBIDDEN') });
|
||||
}
|
||||
const id = (await request.formData()).get('id') as string;
|
||||
const api = createApiClient(fetch);
|
||||
const result = await api.PATCH('/api/persons/{id}/confirm', {
|
||||
@@ -45,7 +48,10 @@ export const actions = {
|
||||
return { success: true };
|
||||
},
|
||||
|
||||
delete: async ({ request, fetch }) => {
|
||||
delete: async ({ request, fetch, locals }) => {
|
||||
if (!hasWriteAll(locals)) {
|
||||
return fail(403, { error: getErrorMessage('FORBIDDEN') });
|
||||
}
|
||||
const id = (await request.formData()).get('id') as string;
|
||||
const api = createApiClient(fetch);
|
||||
const result = await api.DELETE('/api/persons/{id}', {
|
||||
@@ -59,7 +65,10 @@ export const actions = {
|
||||
return { success: true };
|
||||
},
|
||||
|
||||
merge: async ({ request, fetch }) => {
|
||||
merge: async ({ request, fetch, locals }) => {
|
||||
if (!hasWriteAll(locals)) {
|
||||
return fail(403, { error: getErrorMessage('FORBIDDEN') });
|
||||
}
|
||||
const formData = await request.formData();
|
||||
const id = formData.get('id') as string;
|
||||
const targetPersonId = formData.get('targetPersonId') as string;
|
||||
@@ -79,7 +88,10 @@ export const actions = {
|
||||
return { success: true };
|
||||
},
|
||||
|
||||
rename: async ({ request, fetch }) => {
|
||||
rename: async ({ request, fetch, locals }) => {
|
||||
if (!hasWriteAll(locals)) {
|
||||
return fail(403, { error: getErrorMessage('FORBIDDEN') });
|
||||
}
|
||||
const formData = await request.formData();
|
||||
const id = formData.get('id') as string;
|
||||
const firstName = (formData.get('firstName') as string)?.trim() || undefined;
|
||||
|
||||
115
frontend/src/routes/persons/review/page.server.spec.ts
Normal file
115
frontend/src/routes/persons/review/page.server.spec.ts
Normal file
@@ -0,0 +1,115 @@
|
||||
import { describe, it, expect, vi, beforeEach } from 'vitest';
|
||||
|
||||
vi.mock('$lib/shared/api.server', () => ({
|
||||
createApiClient: vi.fn(),
|
||||
extractErrorCode: (e: unknown) => (e as { code?: string } | undefined)?.code
|
||||
}));
|
||||
|
||||
import { actions } from './+page.server';
|
||||
import { createApiClient } from '$lib/shared/api.server';
|
||||
|
||||
beforeEach(() => vi.clearAllMocks());
|
||||
|
||||
const writer = { groups: [{ permissions: ['READ_ALL', 'WRITE_ALL'] }] };
|
||||
const reader = { groups: [{ permissions: ['READ_ALL'] }] };
|
||||
|
||||
/** Mock the typed client with a single response stubbed for every verb. */
|
||||
function mockApi(response: { ok: boolean; status: number; error?: unknown }) {
|
||||
const result = {
|
||||
response: { ok: response.ok, status: response.status },
|
||||
error: response.error,
|
||||
data: response.ok ? {} : undefined
|
||||
};
|
||||
const apiCall = vi.fn(() => Promise.resolve(result));
|
||||
vi.mocked(createApiClient).mockReturnValue({
|
||||
GET: apiCall,
|
||||
PATCH: apiCall,
|
||||
POST: apiCall,
|
||||
PUT: apiCall,
|
||||
DELETE: apiCall
|
||||
} as unknown as ReturnType<typeof createApiClient>);
|
||||
return apiCall;
|
||||
}
|
||||
|
||||
/** Build a SvelteKit RequestEvent with a FormData body and a user shape. */
|
||||
function runAction(
|
||||
action: (typeof actions)[keyof typeof actions],
|
||||
formData: FormData,
|
||||
user: unknown
|
||||
) {
|
||||
return action({
|
||||
request: new Request('http://localhost', { method: 'POST', body: formData }),
|
||||
fetch: vi.fn() as unknown as typeof fetch,
|
||||
locals: { user } as App.Locals
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
} as any);
|
||||
}
|
||||
|
||||
describe('persons/review confirm action — WRITE_ALL guard', () => {
|
||||
it('returns fail(403) when the user lacks WRITE_ALL', async () => {
|
||||
const apiCall = mockApi({ ok: true, status: 200 });
|
||||
const fd = new FormData();
|
||||
fd.append('id', 'p-1');
|
||||
|
||||
const result = await runAction(actions.confirm, fd, reader);
|
||||
|
||||
expect(apiCall).not.toHaveBeenCalled();
|
||||
expect(result).toMatchObject({ status: 403 });
|
||||
});
|
||||
});
|
||||
|
||||
describe('persons/review delete action — WRITE_ALL guard', () => {
|
||||
it('returns fail(403) when the user lacks WRITE_ALL', async () => {
|
||||
const apiCall = mockApi({ ok: true, status: 200 });
|
||||
const fd = new FormData();
|
||||
fd.append('id', 'p-1');
|
||||
|
||||
const result = await runAction(actions.delete, fd, reader);
|
||||
|
||||
expect(apiCall).not.toHaveBeenCalled();
|
||||
expect(result).toMatchObject({ status: 403 });
|
||||
});
|
||||
});
|
||||
|
||||
describe('persons/review merge action — WRITE_ALL guard', () => {
|
||||
it('returns fail(403) when the user lacks WRITE_ALL', async () => {
|
||||
const apiCall = mockApi({ ok: true, status: 200 });
|
||||
const fd = new FormData();
|
||||
fd.append('id', 'p-1');
|
||||
fd.append('targetPersonId', 'p-2');
|
||||
|
||||
const result = await runAction(actions.merge, fd, reader);
|
||||
|
||||
expect(apiCall).not.toHaveBeenCalled();
|
||||
expect(result).toMatchObject({ status: 403 });
|
||||
});
|
||||
});
|
||||
|
||||
describe('persons/review rename action — WRITE_ALL guard', () => {
|
||||
it('returns fail(403) when the user lacks WRITE_ALL', async () => {
|
||||
const apiCall = mockApi({ ok: true, status: 200 });
|
||||
const fd = new FormData();
|
||||
fd.append('id', 'p-1');
|
||||
fd.append('lastName', 'Smith');
|
||||
|
||||
const result = await runAction(actions.rename, fd, reader);
|
||||
|
||||
expect(apiCall).not.toHaveBeenCalled();
|
||||
expect(result).toMatchObject({ status: 403 });
|
||||
});
|
||||
});
|
||||
|
||||
// Sanity: writers still pass through (no 403 from the guard). Full happy-path coverage lives
|
||||
// in the action-by-action describe blocks added later.
|
||||
describe('persons/review confirm action — writer passes guard', () => {
|
||||
it('does NOT short-circuit with 403 when the user has WRITE_ALL', async () => {
|
||||
const apiCall = mockApi({ ok: true, status: 200 });
|
||||
const fd = new FormData();
|
||||
fd.append('id', 'p-1');
|
||||
|
||||
const result = await runAction(actions.confirm, fd, writer);
|
||||
|
||||
expect(apiCall).toHaveBeenCalled();
|
||||
expect(result).toEqual({ success: true });
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user