Skip to content

Commit 3d9e41a

Browse files
dobby-coder[bot]dobby-coder[bot]
authored
fix: reject unknown fieldName in approveChangeRequest (#91)
The admin approveChangeRequest service guarded the org UPDATE on a fieldMap lookup but still flipped the change request to approved when the lookup missed. Unknown or stale fieldName values left the request showing approved while no column had changed. Validate fieldName against a typed allowlist before any mutation, throw InvalidChangeRequestFieldError when it misses, and surface that as a 400 fail in the page action. The org write also moves from sql.raw to a typed Drizzle update, removing the fragile dynamic-column pattern flagged in repos/postguard-business/security.md. Closes #87 Co-authored-by: dobby-yivi-agent[bot] <275734547+dobby-yivi-agent[bot]@users.noreply.github.com>
1 parent 823282f commit 3d9e41a

4 files changed

Lines changed: 207 additions & 15 deletions

File tree

‎src/lib/server/services/admin-change-requests.ts‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
export const APPROVABLE_FIELDS = ['name', 'domain', 'signingEmail', 'kvkNumber'] as const;
2+
export type ApprovableField = (typeof APPROVABLE_FIELDS)[number];
3+
4+
export function isApprovableField(value: string): value is ApprovableField {
5+
return (APPROVABLE_FIELDS as readonly string[]).includes(value);
6+
}
7+
8+
export class InvalidChangeRequestFieldError extends Error {
9+
readonly fieldName: string;
10+
constructor(fieldName: string) {
11+
super(`Unknown change-request fieldName: ${fieldName}`);
12+
this.name = 'InvalidChangeRequestFieldError';
13+
this.fieldName = fieldName;
14+
}
15+
}

‎src/lib/server/services/admin.ts‎

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,8 @@ import {
88
adminAccounts,
99
sessions
1010
} from '$lib/server/db/schema';
11-
import { eq, desc, and, isNull, or, count, sql } from 'drizzle-orm';
11+
import { eq, desc, and, isNull, or, count } from 'drizzle-orm';
12+
import { isApprovableField, InvalidChangeRequestFieldError } from './admin-change-requests';
1213

1314
export async function logAdminAction(
1415
adminId: string,
@@ -87,21 +88,15 @@ export async function approveChangeRequest(
8788
if (!reqs[0]) return null;
8889
const req = reqs[0];
8990

90-
// Apply the change to the organization
91-
const fieldMap: Record<string, string> = {
92-
name: 'name',
93-
domain: 'domain',
94-
signingEmail: 'signing_email',
95-
kvkNumber: 'kvk_number'
96-
};
97-
98-
if (fieldMap[req.fieldName]) {
99-
// Use raw SQL for dynamic column update
100-
await db.execute(
101-
sql`UPDATE organizations SET ${sql.raw(fieldMap[req.fieldName])} = ${req.newValue}, updated_at = NOW() WHERE id = ${req.orgId}`
102-
);
91+
if (!isApprovableField(req.fieldName)) {
92+
throw new InvalidChangeRequestFieldError(req.fieldName);
10393
}
10494

95+
await db
96+
.update(organizations)
97+
.set({ [req.fieldName]: req.newValue, updatedAt: new Date() })
98+
.where(eq(organizations.id, req.orgId));
99+
105100
await db
106101
.update(changeRequests)
107102
.set({

‎src/routes/(admin)/admin/organizations/[id]/+page.server.ts‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
addUserToOrganization,
1212
logAdminAction
1313
} from '$lib/server/services/admin';
14+
import { InvalidChangeRequestFieldError } from '$lib/server/services/admin-change-requests';
1415
import { setImpersonation } from '$lib/server/auth/session';
1516
import { isUniqueViolation } from '$lib/server/services/errors';
1617
import { isEnabled } from '$lib/feature-flags';
@@ -36,7 +37,15 @@ export const actions: Actions = {
3637

3738
if (!requestId) return fail(400, { error: 'Missing request ID' });
3839

39-
const req = await approveChangeRequest(requestId, adminId, reviewNotes);
40+
let req;
41+
try {
42+
req = await approveChangeRequest(requestId, adminId, reviewNotes);
43+
} catch (err) {
44+
if (err instanceof InvalidChangeRequestFieldError) {
45+
return fail(400, { error: 'invalid_field', fieldName: err.fieldName });
46+
}
47+
throw err;
48+
}
4049
if (req) {
4150
await logAdminAction(
4251
adminId,

‎tests/unit/admin-approve-change-request.test.ts‎

Lines changed: 173 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,173 @@
1+
import { describe, it, expect, vi, beforeEach } from 'vitest';
2+
import {
3+
isApprovableField,
4+
APPROVABLE_FIELDS,
5+
InvalidChangeRequestFieldError
6+
} from '../../src/lib/server/services/admin-change-requests';
7+
8+
const { approveChangeRequestMock, logAdminActionMock, isEnabledMock } = vi.hoisted(() => ({
9+
approveChangeRequestMock: vi.fn(),
10+
logAdminActionMock: vi.fn(),
11+
isEnabledMock: vi.fn()
12+
}));
13+
14+
vi.mock('$lib/server/services/admin', () => ({
15+
getOrganizationWithRequests: vi.fn(),
16+
getOrganizationById: vi.fn(),
17+
approveChangeRequest: approveChangeRequestMock,
18+
rejectChangeRequest: vi.fn(),
19+
deleteOrganization: vi.fn(),
20+
activateOrganization: vi.fn(),
21+
suspendOrganization: vi.fn(),
22+
addUserToOrganization: vi.fn(),
23+
logAdminAction: logAdminActionMock
24+
}));
25+
26+
vi.mock('$lib/server/auth/session', () => ({
27+
setImpersonation: vi.fn()
28+
}));
29+
30+
vi.mock('$lib/feature-flags', () => ({
31+
isEnabled: (...args: unknown[]) => isEnabledMock(...args)
32+
}));
33+
34+
import { actions } from '../../src/routes/(admin)/admin/organizations/[id]/+page.server';
35+
36+
type ApproveEvent = Parameters<NonNullable<typeof actions>['approve']>[0];
37+
38+
function buildEvent(overrides: {
39+
session?: Record<string, unknown> | null;
40+
requestId?: string | null;
41+
reviewNotes?: string;
42+
}): ApproveEvent {
43+
const form = new FormData();
44+
if (overrides.requestId !== null) form.set('requestId', overrides.requestId ?? 'req-id');
45+
if (overrides.reviewNotes !== undefined) form.set('reviewNotes', overrides.reviewNotes);
46+
47+
return {
48+
params: { id: 'org-id' },
49+
locals: { session: overrides.session ?? null },
50+
request: { formData: async () => form },
51+
getClientAddress: () => '127.0.0.1'
52+
} as unknown as ApproveEvent;
53+
}
54+
55+
describe('isApprovableField', () => {
56+
it.each([...APPROVABLE_FIELDS])('accepts the allowlist value %s', (field) => {
57+
expect(isApprovableField(field)).toBe(true);
58+
});
59+
60+
it.each(['status', 'contact_user_id', 'signing_email', 'kvk_number', '', 'NAME', 'id'])(
61+
'rejects unknown field %s',
62+
(field) => {
63+
expect(isApprovableField(field)).toBe(false);
64+
}
65+
);
66+
});
67+
68+
describe('InvalidChangeRequestFieldError', () => {
69+
it('carries the offending fieldName', () => {
70+
const err = new InvalidChangeRequestFieldError('mystery_column');
71+
expect(err).toBeInstanceOf(Error);
72+
expect(err.fieldName).toBe('mystery_column');
73+
expect(err.message).toContain('mystery_column');
74+
});
75+
});
76+
77+
describe('admin approve action — invalid fieldName handling', () => {
78+
beforeEach(() => {
79+
approveChangeRequestMock.mockReset();
80+
logAdminActionMock.mockReset();
81+
isEnabledMock.mockReset();
82+
isEnabledMock.mockReturnValue(true);
83+
});
84+
85+
it('rejects when no admin session is present and never calls the service', async () => {
86+
await expect(
87+
actions.approve(buildEvent({ session: null, requestId: 'req-1' }))
88+
).rejects.toMatchObject({ status: 401 });
89+
90+
expect(approveChangeRequestMock).not.toHaveBeenCalled();
91+
expect(logAdminActionMock).not.toHaveBeenCalled();
92+
});
93+
94+
it('returns 400 when the service throws InvalidChangeRequestFieldError and does not log an admin action', async () => {
95+
approveChangeRequestMock.mockRejectedValueOnce(
96+
new InvalidChangeRequestFieldError('mystery_column')
97+
);
98+
99+
const result = await actions.approve(
100+
buildEvent({
101+
session: { adminId: 'admin-1', userType: 'admin' },
102+
requestId: 'req-1',
103+
reviewNotes: 'looks good'
104+
})
105+
);
106+
107+
expect(result).toMatchObject({
108+
status: 400,
109+
data: { error: 'invalid_field', fieldName: 'mystery_column' }
110+
});
111+
expect(logAdminActionMock).not.toHaveBeenCalled();
112+
});
113+
114+
it('returns { approved: true } and logs when the service applies the change', async () => {
115+
approveChangeRequestMock.mockResolvedValueOnce({
116+
id: 'req-1',
117+
orgId: 'org-1',
118+
fieldName: 'name',
119+
newValue: 'New Name'
120+
});
121+
122+
const result = await actions.approve(
123+
buildEvent({
124+
session: { adminId: 'admin-1', userType: 'admin' },
125+
requestId: 'req-1',
126+
reviewNotes: 'ok'
127+
})
128+
);
129+
130+
expect(result).toEqual({ approved: true });
131+
expect(approveChangeRequestMock).toHaveBeenCalledWith('req-1', 'admin-1', 'ok');
132+
expect(logAdminActionMock).toHaveBeenCalledTimes(1);
133+
expect(logAdminActionMock).toHaveBeenCalledWith(
134+
'admin-1',
135+
'approve_change',
136+
'change_request',
137+
'req-1',
138+
{ fieldName: 'name', newValue: 'New Name', reviewNotes: 'ok' },
139+
'127.0.0.1'
140+
);
141+
});
142+
143+
it('returns { approved: true } without logging when the service reports no matching request', async () => {
144+
approveChangeRequestMock.mockResolvedValueOnce(null);
145+
146+
const result = await actions.approve(
147+
buildEvent({
148+
session: { adminId: 'admin-1', userType: 'admin' },
149+
requestId: 'missing-req',
150+
reviewNotes: ''
151+
})
152+
);
153+
154+
expect(result).toEqual({ approved: true });
155+
expect(logAdminActionMock).not.toHaveBeenCalled();
156+
});
157+
158+
it('re-throws unexpected errors so they surface as 500', async () => {
159+
const boom = new Error('db exploded');
160+
approveChangeRequestMock.mockRejectedValueOnce(boom);
161+
162+
await expect(
163+
actions.approve(
164+
buildEvent({
165+
session: { adminId: 'admin-1', userType: 'admin' },
166+
requestId: 'req-1'
167+
})
168+
)
169+
).rejects.toBe(boom);
170+
171+
expect(logAdminActionMock).not.toHaveBeenCalled();
172+
});
173+
});

0 commit comments

Comments
 (0)