From 0e135f674037d048c32d1e8cf384f9ef9eb44ce5 Mon Sep 17 00:00:00 2001 From: Chris Kehayias Date: Sun, 13 Sep 2026 06:40:35 -0400 Subject: [PATCH 1/3] fix(security): close IDOR in getCurrentUserProfile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `getCurrentUserProfile(id)` took a caller-supplied MP `User_GUID` and passed it to `UserService.getUserProfile` behind only a "a session exists" check. A server action is a callable POST endpoint whose payload the caller shapes, so the parameter was reachable regardless of what the sole legitimate caller passed. Any authenticated MP user could therefore read any other user's First_Name, Last_Name, Email_Address, Mobile_Phone, photo GUID, and their full role and user-group list — the last of which is also a reconnaissance aid for locating a high-privilege account. This is the F1 pattern from the downstream hardening playbook ("a session proves only that *some* MP user signed in") surviving inside the one file CLAUDE.md rule 12 blesses as a carve-out. The carve-out is justified as "the user's own profile"; nothing enforced "own". The parameter is REMOVED rather than validated against the session. A value that must equal a server-derived one has no reason to cross the wire, and deleting it makes the carve-out's justification enforceable by the type signature instead of by reviewer vigilance. The guard also moves from `session?.user?.id` to a non-empty `userGuid` check. `user.id` is Better Auth's internal ID; its presence does not prove an MP identity exists. `userGuid` is `required: true` in `src/lib/auth.ts`, so keying on it fails closed. Tests drive the adversarial shape, not the type: one case calls the action through a cast that forges an argument and asserts the session GUID is still what reaches the service. Verified protective by negative control — restoring the vulnerable body fails 4 of 6 cases, the IDOR case reporting `expected [SESSION_GUID], received ["attacker-supplied-guid"]`. Co-Authored-By: Claude Opus 5 (1M context) --- src/components/shared-actions/user.test.ts | 85 +++++++++------------- src/components/shared-actions/user.ts | 42 +++++------ src/contexts/user-context.test.tsx | 2 +- src/contexts/user-context.tsx | 2 +- 4 files changed, 54 insertions(+), 77 deletions(-) diff --git a/src/components/shared-actions/user.test.ts b/src/components/shared-actions/user.test.ts index 259d026..28ccd74 100644 --- a/src/components/shared-actions/user.test.ts +++ b/src/components/shared-actions/user.test.ts @@ -1,16 +1,14 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -const { mockGetUserProfile, mockGetSession, mockGetUserIdByGuid } = vi.hoisted(() => ({ +const { mockGetUserProfile, mockGetSession } = vi.hoisted(() => ({ mockGetUserProfile: vi.fn(), mockGetSession: vi.fn(), - mockGetUserIdByGuid: vi.fn(), })); vi.mock('@/services/userService', () => ({ UserService: { getInstance: vi.fn().mockResolvedValue({ getUserProfile: mockGetUserProfile, - getUserIdByGuid: mockGetUserIdByGuid, }), }, })); @@ -23,20 +21,22 @@ vi.mock('next/headers', () => ({ headers: vi.fn().mockResolvedValue(new Headers()), })); -import { getCurrentUserProfile, getCurrentUserIdFromSession } from './user'; +import { getCurrentUserProfile } from './user'; + +const SESSION_GUID = '550e8400-e29b-41d4-a716-446655440000'; describe('getCurrentUserProfile', () => { beforeEach(() => { vi.clearAllMocks(); mockGetSession.mockResolvedValue({ - user: { id: 'internal-id', userGuid: '550e8400-e29b-41d4-a716-446655440000' }, + user: { id: 'internal-id', userGuid: SESSION_GUID }, }); }); - it('should call UserService with correct ID', async () => { + it("should look up the profile using the session's userGuid", async () => { const mockProfile = { User_ID: 1, - User_GUID: '550e8400-e29b-41d4-a716-446655440000', + User_GUID: SESSION_GUID, Contact_ID: 100, First_Name: 'John', Nickname: 'Johnny', @@ -49,67 +49,48 @@ describe('getCurrentUserProfile', () => { }; mockGetUserProfile.mockResolvedValueOnce(mockProfile); - const result = await getCurrentUserProfile('550e8400-e29b-41d4-a716-446655440000'); + const result = await getCurrentUserProfile(); - expect(mockGetUserProfile).toHaveBeenCalledWith('550e8400-e29b-41d4-a716-446655440000'); + expect(mockGetUserProfile).toHaveBeenCalledWith(SESSION_GUID); expect(result).toEqual(mockProfile); }); - it('should throw when not authenticated', async () => { - mockGetSession.mockResolvedValueOnce(null); + it('should ignore any caller-supplied argument and use the session GUID', async () => { + mockGetUserProfile.mockResolvedValueOnce(undefined); - await expect(getCurrentUserProfile('550e8400-e29b-41d4-a716-446655440000')) - .rejects.toThrow('Unauthorized'); - }); + // The action takes no parameters; a caller forging one must not influence the lookup. + await (getCurrentUserProfile as unknown as (id: string) => Promise)( + 'attacker-supplied-guid' + ); - it('should propagate errors', async () => { - mockGetUserProfile.mockRejectedValueOnce(new Error('Service error')); - - await expect(getCurrentUserProfile('550e8400-e29b-41d4-a716-446655440000')) - .rejects.toThrow('Service error'); + expect(mockGetUserProfile).toHaveBeenCalledWith(SESSION_GUID); + expect(mockGetUserProfile).not.toHaveBeenCalledWith('attacker-supplied-guid'); }); -}); -describe('getCurrentUserIdFromSession', () => { - beforeEach(() => { - vi.clearAllMocks(); - }); + it('should throw when the session has no userGuid', async () => { + mockGetSession.mockResolvedValueOnce({ user: { id: 'internal-id' } }); - it('resolves the MP User_ID via getUserIdByGuid', async () => { - mockGetUserIdByGuid.mockResolvedValueOnce(42); + await expect(getCurrentUserProfile()).rejects.toThrow('Unauthorized'); + expect(mockGetUserProfile).not.toHaveBeenCalled(); + }); - const result = await getCurrentUserIdFromSession({ - user: { id: 'internal-id', userGuid: '550e8400-e29b-41d4-a716-446655440000' }, - } as unknown as Awaited>); + it('should throw when the session userGuid is an empty string', async () => { + mockGetSession.mockResolvedValueOnce({ user: { id: 'internal-id', userGuid: '' } }); - expect(mockGetUserIdByGuid).toHaveBeenCalledWith('550e8400-e29b-41d4-a716-446655440000'); - expect(result).toBe(42); + await expect(getCurrentUserProfile()).rejects.toThrow('Unauthorized'); + expect(mockGetUserProfile).not.toHaveBeenCalled(); }); - it('throws when session is null', async () => { - await expect( - getCurrentUserIdFromSession( - null as unknown as Awaited> - ) - ).rejects.toThrow('User GUID not found in session'); - }); + it('should throw when not authenticated', async () => { + mockGetSession.mockResolvedValueOnce(null); - it('throws when session.user has no userGuid', async () => { - await expect( - getCurrentUserIdFromSession({ - user: { id: 'internal-id' }, - } as unknown as Awaited>) - ).rejects.toThrow('User GUID not found in session'); - expect(mockGetUserIdByGuid).not.toHaveBeenCalled(); + await expect(getCurrentUserProfile()).rejects.toThrow('Unauthorized'); + expect(mockGetUserProfile).not.toHaveBeenCalled(); }); - it('propagates errors from getUserIdByGuid', async () => { - mockGetUserIdByGuid.mockRejectedValueOnce(new Error('User not found')); + it('should propagate errors', async () => { + mockGetUserProfile.mockRejectedValueOnce(new Error('Service error')); - await expect( - getCurrentUserIdFromSession({ - user: { id: 'internal-id', userGuid: 'bogus' }, - } as unknown as Awaited>) - ).rejects.toThrow('User not found'); + await expect(getCurrentUserProfile()).rejects.toThrow('Service error'); }); }); diff --git a/src/components/shared-actions/user.ts b/src/components/shared-actions/user.ts index 6e3c822..7875442 100644 --- a/src/components/shared-actions/user.ts +++ b/src/components/shared-actions/user.ts @@ -5,32 +5,28 @@ import { MPUserProfile } from "@/lib/providers/ministry-platform/types"; import { UserService } from '@/services/userService'; import { headers } from 'next/headers'; -type BetterAuthSession = Awaited>; - -export async function getCurrentUserProfile(id: string): Promise { - const session = await auth.api.getSession({ headers: await headers() }); - if (!session?.user?.id) throw new Error('Unauthorized'); - - const userService = await UserService.getInstance(); - const userProfile = await userService.getUserProfile(id); - return userProfile; -} - /** - * Resolves the numeric MP `User_ID` for the given Better Auth session by looking up - * `session.user.userGuid` in `dp_Users`. Throws if the session has no user GUID. + * Returns the profile of the *calling* user only. + * + * Carve-out from the `AuthorizationService` gate (CLAUDE.md rule 12): this action + * exposes no MP data other than the caller's own profile, so there is no role to + * check beyond "is this a real MP identity". * - * This is the canonical way for server actions to obtain the `$userId` value that - * should be threaded through to MP write operations for audit attribution. + * The GUID is derived from the session and MUST NOT become a parameter again. + * Server actions are caller-shaped POST endpoints, so a `id`/`userGuid` argument + * here was a live IDOR: any authenticated MP user could read another user's + * contact details, roles, and user groups. Keeping it session-only makes the + * carve-out's "own profile" justification enforceable by the type signature. + * + * The guard keys on `userGuid` (declared `required: true` in `src/lib/auth.ts`), + * not `session.user.id` — the latter is Better Auth's internal ID and its presence + * does not prove an MP identity exists. */ -export async function getCurrentUserIdFromSession( - session: BetterAuthSession -): Promise { - const userGuid = (session?.user as Record | undefined)?.userGuid as - | string - | undefined; - if (!userGuid) throw new Error('User GUID not found in session'); +export async function getCurrentUserProfile(): Promise { + const session = await auth.api.getSession({ headers: await headers() }); + const userGuid = (session?.user as Record | undefined)?.userGuid; + if (typeof userGuid !== 'string' || userGuid.length === 0) throw new Error('Unauthorized'); const userService = await UserService.getInstance(); - return userService.getUserIdByGuid(userGuid); + return userService.getUserProfile(userGuid); } diff --git a/src/contexts/user-context.test.tsx b/src/contexts/user-context.test.tsx index c19785a..e799b11 100644 --- a/src/contexts/user-context.test.tsx +++ b/src/contexts/user-context.test.tsx @@ -66,7 +66,7 @@ describe('UserContext', () => { expect(result.current.userProfile).toEqual(mockProfile); expect(result.current.error).toBeNull(); - expect(mockGetCurrentUserProfile).toHaveBeenCalledWith('guid-123'); + expect(mockGetCurrentUserProfile).toHaveBeenCalledWith(); }); it('should set null profile when no session', async () => { diff --git a/src/contexts/user-context.tsx b/src/contexts/user-context.tsx index cd88cc7..967cae2 100644 --- a/src/contexts/user-context.tsx +++ b/src/contexts/user-context.tsx @@ -38,7 +38,7 @@ export function UserProvider({ children }: UserProviderProps) { try { setIsLoading(true); setError(null); - const profile = await getCurrentUserProfile(userGuid); + const profile = await getCurrentUserProfile(); setUserProfile(profile ?? null); } catch (err) { setError(err instanceof Error ? err : new Error("Failed to load user profile")); From 9322bae528324de8e73c597af1754fe0bd130f9c Mon Sep 17 00:00:00 2001 From: Chris Kehayias Date: Sun, 13 Sep 2026 06:40:47 -0400 Subject: [PATCH 2/3] refactor(auth): gate sample-template, drop dead identity code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ons from closing the getCurrentUserProfile IDOR. **`generateSampleTemplate` now authorizes.** It was a `'use server'` action with no gate and no in-file justification — an undocumented fifth carve-out, where CLAUDE.md rule 12 allows four and requires each to be argued in writing. It reads no MP data (it builds a static .docx from constants), so it leaked nothing; it is gated rather than documented because it is a capability of the address-label tool and can simply take that tool's gate. The carve-out list stays at four. **`UserService.getUserIdByGuid` is deleted.** Deleting the dead server action `getCurrentUserIdFromSession` removed its last production caller. That action was itself worth removing on its own terms: it was an exported endpoint accepting a *session object* as a parameter, and its docstring claimed to be "the canonical way for server actions to obtain the `$userId`" — contradicting CLAUDE.md rule 13, `authorizationService.ts`, and the security reference, which all state `$userId` comes only from the gate's return value. Stale pre-AuthorizationService guidance that would have led the next reader astray. The four action test files mocking `getUserIdByGuid` turned out to have been mocking `UserService` wholesale despite their production code not importing it at all, so the entire vi.mock blocks went rather than one entry each. `src/lib/auth.ts` is a comment-only change: `extractUserGuid`'s JSDoc cited the deleted method as the downstream `validateGuid` caller; retargeted to `getUserProfile`, which is the surviving one. 805 tests pass (809 less the 4 deleted), eslint clean, build green. Co-Authored-By: Claude Opus 5 (1M context) --- src/components/address-labels/actions.test.ts | 9 ----- .../address-labels/sample-template.ts | 12 ++++++ .../panels/deploy-tool-actions.test.ts | 12 ------ .../field-management/actions.test.ts | 12 ------ src/components/group-wizard/actions.test.ts | 17 -------- src/lib/auth.ts | 2 +- src/services/userService.test.ts | 40 ------------------- src/services/userService.ts | 20 ---------- 8 files changed, 13 insertions(+), 111 deletions(-) diff --git a/src/components/address-labels/actions.test.ts b/src/components/address-labels/actions.test.ts index f29276d..c88ea10 100644 --- a/src/components/address-labels/actions.test.ts +++ b/src/components/address-labels/actions.test.ts @@ -5,7 +5,6 @@ const mockGetSelectionRecordIds = vi.hoisted(() => vi.fn()); const mockGetAddressesForContacts = vi.hoisted(() => vi.fn()); const mockGetAddressForContact = vi.hoisted(() => vi.fn()); const mockToBlob = vi.hoisted(() => vi.fn()); -const mockGetUserIdByGuid = vi.hoisted(() => vi.fn()); const mockDocxtemplaterRender = vi.hoisted(() => vi.fn()); const mockDocxtemplaterGetZip = vi.hoisted(() => vi.fn()); @@ -56,14 +55,6 @@ vi.mock('@/services/toolService', () => ({ }, })); -vi.mock('@/services/userService', () => ({ - UserService: { - getInstance: vi.fn().mockResolvedValue({ - getUserIdByGuid: mockGetUserIdByGuid, - }), - }, -})); - vi.mock('@/services/addressLabelService', () => ({ AddressLabelService: { getInstance: vi.fn().mockResolvedValue({ diff --git a/src/components/address-labels/sample-template.ts b/src/components/address-labels/sample-template.ts index dfa1e2d..d888371 100644 --- a/src/components/address-labels/sample-template.ts +++ b/src/components/address-labels/sample-template.ts @@ -1,6 +1,7 @@ 'use server'; import { Document, Paragraph, TextRun, Packer, PageBreak } from 'docx'; +import { AuthorizationService } from '@/services/authorizationService'; /** * Generates a sample .docx template with merge tokens pre-placed @@ -12,6 +13,17 @@ import { Document, Paragraph, TextRun, Packer, PageBreak } from 'docx'; * Returns base64-encoded .docx content. */ export async function generateSampleTemplate(): Promise { + // Authorize, don't just authenticate (CLAUDE.md rule 12). This action reads no + // MP data — it builds a static .docx from constants — so it leaks nothing by + // itself. It is gated anyway rather than documented as a carve-out: it is a + // capability of the address-label tool, so it takes that tool's gate, and the + // carve-out list stays at four entries instead of growing a fifth that a + // reader would have to re-reason about later. + await AuthorizationService.getInstance().requireSecurityRole({ + table: 'Contacts', + operation: 'read', + }); + const doc = new Document({ sections: [{ properties: { diff --git a/src/components/dev-panel/panels/deploy-tool-actions.test.ts b/src/components/dev-panel/panels/deploy-tool-actions.test.ts index 9511d60..1be75e6 100644 --- a/src/components/dev-panel/panels/deploy-tool-actions.test.ts +++ b/src/components/dev-panel/panels/deploy-tool-actions.test.ts @@ -5,13 +5,11 @@ const { mockListPages, mockListRoles, mockDeployTool, - mockGetUserIdByGuid, } = vi.hoisted(() => ({ mockGetSession: vi.fn(), mockListPages: vi.fn(), mockListRoles: vi.fn(), mockDeployTool: vi.fn(), - mockGetUserIdByGuid: vi.fn(), })); vi.mock('@/lib/auth', () => ({ @@ -36,14 +34,6 @@ vi.mock('@/services/toolService', () => ({ }, })); -vi.mock('@/services/userService', () => ({ - UserService: { - getInstance: vi.fn().mockResolvedValue({ - getUserIdByGuid: mockGetUserIdByGuid, - }), - }, -})); - import { deployToolAction, getDeployToolEnvStatusAction, @@ -92,8 +82,6 @@ describe('deploy-tool-actions', () => { mockListPages.mockReset(); mockListRoles.mockReset(); mockDeployTool.mockReset(); - mockGetUserIdByGuid.mockReset(); - mockGetUserIdByGuid.mockResolvedValue(42); vi.stubEnv('NODE_ENV', 'development'); }); diff --git a/src/components/field-management/actions.test.ts b/src/components/field-management/actions.test.ts index 30dbf7b..f345f9a 100644 --- a/src/components/field-management/actions.test.ts +++ b/src/components/field-management/actions.test.ts @@ -7,8 +7,6 @@ const { mockGetTableMetadata, mockUpdatePageFieldOrder, mockGetInstance, - mockGetUserIdByGuid, - mockUserGetInstance, } = vi.hoisted(() => ({ mockGetSession: vi.fn(), mockGetPages: vi.fn(), @@ -16,8 +14,6 @@ const { mockGetTableMetadata: vi.fn(), mockUpdatePageFieldOrder: vi.fn(), mockGetInstance: vi.fn(), - mockGetUserIdByGuid: vi.fn(), - mockUserGetInstance: vi.fn(), })); vi.mock('@/lib/auth', () => ({ @@ -70,10 +66,6 @@ vi.mock('@/services/fieldManagementService', () => ({ }, })); -vi.mock('@/services/userService', () => ({ - UserService: { getInstance: mockUserGetInstance }, -})); - import { fetchPages, fetchPageFieldData, savePageFieldOrder } from './actions'; const authedSession = { @@ -94,10 +86,6 @@ describe('field-management actions', () => { getTableMetadata: mockGetTableMetadata, updatePageFieldOrder: mockUpdatePageFieldOrder, }); - mockUserGetInstance.mockResolvedValue({ - getUserIdByGuid: mockGetUserIdByGuid, - }); - mockGetUserIdByGuid.mockResolvedValue(42); }); describe('fetchPages', () => { diff --git a/src/components/group-wizard/actions.test.ts b/src/components/group-wizard/actions.test.ts index 2efcc9a..5e3b46c 100644 --- a/src/components/group-wizard/actions.test.ts +++ b/src/components/group-wizard/actions.test.ts @@ -8,9 +8,7 @@ const { mockGetGroup, mockCreateGroup, mockUpdateGroup, - mockGetUserIdByGuid, mockGroupGetInstance, - mockUserGetInstance, } = vi.hoisted(() => ({ mockGetSession: vi.fn(), mockFetchAllLookups: vi.fn(), @@ -19,9 +17,7 @@ const { mockGetGroup: vi.fn(), mockCreateGroup: vi.fn(), mockUpdateGroup: vi.fn(), - mockGetUserIdByGuid: vi.fn(), mockGroupGetInstance: vi.fn(), - mockUserGetInstance: vi.fn(), })); vi.mock('@/lib/auth', () => ({ @@ -72,10 +68,6 @@ vi.mock('@/services/groupService', () => ({ GroupService: { getInstance: mockGroupGetInstance }, })); -vi.mock('@/services/userService', () => ({ - UserService: { getInstance: mockUserGetInstance }, -})); - import { fetchGroupWizardLookups, searchContacts, @@ -144,9 +136,6 @@ mockRequireSecurityRole.mockImplementation(async () => { createGroup: mockCreateGroup, updateGroup: mockUpdateGroup, }); - mockUserGetInstance.mockResolvedValue({ - getUserIdByGuid: mockGetUserIdByGuid, - }); }); describe('fetchGroupWizardLookups', () => { @@ -318,7 +307,6 @@ describe('createGroup', () => { mockGetSession.mockResolvedValueOnce({ user: { id: 'user-1', userGuid: '550e8400-e29b-41d4-a716-446655440000' }, }); - mockGetUserIdByGuid.mockResolvedValueOnce(42); mockCreateGroup.mockResolvedValueOnce({ Group_ID: 200, Group_Name: 'Test Group' }); const result = await createGroup(BASE_FORM); @@ -331,7 +319,6 @@ describe('createGroup', () => { mockGetSession.mockResolvedValueOnce({ user: { id: 'user-1', userGuid: '550e8400-e29b-41d4-a716-446655440000' }, }); - mockGetUserIdByGuid.mockResolvedValueOnce(42); mockCreateGroup.mockRejectedValueOnce(new Error('MP create failed')); const result = await createGroup(BASE_FORM); @@ -343,7 +330,6 @@ describe('createGroup', () => { mockGetSession.mockResolvedValueOnce({ user: { id: 'user-1', userGuid: '550e8400-e29b-41d4-a716-446655440000' }, }); - mockGetUserIdByGuid.mockResolvedValueOnce(42); mockCreateGroup.mockRejectedValueOnce('boom'); const result = await createGroup(BASE_FORM); @@ -381,7 +367,6 @@ describe('updateGroup', () => { mockGetSession.mockResolvedValueOnce({ user: { id: 'user-1', userGuid: '550e8400-e29b-41d4-a716-446655440000' }, }); - mockGetUserIdByGuid.mockResolvedValueOnce(42); mockUpdateGroup.mockResolvedValueOnce({ Group_ID: 100, Group_Name: 'Updated' }); const result = await updateGroup(100, BASE_FORM); @@ -394,7 +379,6 @@ describe('updateGroup', () => { mockGetSession.mockResolvedValueOnce({ user: { id: 'user-1', userGuid: '550e8400-e29b-41d4-a716-446655440000' }, }); - mockGetUserIdByGuid.mockResolvedValueOnce(42); mockUpdateGroup.mockRejectedValueOnce(new Error('MP update failed')); const result = await updateGroup(100, BASE_FORM); @@ -406,7 +390,6 @@ describe('updateGroup', () => { mockGetSession.mockResolvedValueOnce({ user: { id: 'user-1', userGuid: '550e8400-e29b-41d4-a716-446655440000' }, }); - mockGetUserIdByGuid.mockResolvedValueOnce(42); mockUpdateGroup.mockRejectedValueOnce('boom'); const result = await updateGroup(100, BASE_FORM); diff --git a/src/lib/auth.ts b/src/lib/auth.ts index 58f9483..cbaeb4b 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -126,7 +126,7 @@ export const userAdditionalFields = { * Extracts a usable MP `User_GUID` from an OIDC userinfo payload, or `null`. * * Exported for testing. Validates the shape because `sub` is the value every - * downstream MP lookup keys on — `UserService.getUserIdByGuid` already runs it + * downstream MP lookup keys on — `UserService.getUserProfile` already runs it * through `validateGuid`, so an unparseable `sub` would otherwise surface as a * mystery failure on the first MP call instead of a clean refusal at sign-in. */ diff --git a/src/services/userService.test.ts b/src/services/userService.test.ts index e140cf6..205b1e6 100644 --- a/src/services/userService.test.ts +++ b/src/services/userService.test.ts @@ -116,44 +116,4 @@ describe('UserService', () => { await expect(service.getUserProfile('b0000000-0000-0000-0000-000000000000')).rejects.toThrow('API error'); }); }); - - describe('getUserIdByGuid', () => { - it('should return User_ID for valid GUID', async () => { - mockGetTableRecords.mockResolvedValueOnce([{ User_ID: 42 }]); - - const service = await UserService.getInstance(); - const result = await service.getUserIdByGuid('550e8400-e29b-41d4-a716-446655440000'); - - expect(result).toBe(42); - expect(mockGetTableRecords).toHaveBeenCalledWith({ - table: 'dp_Users', - select: 'User_ID', - filter: "User_GUID = '550e8400-e29b-41d4-a716-446655440000'", - top: 1, - }); - }); - - it('should throw when user not found (empty array)', async () => { - mockGetTableRecords.mockResolvedValueOnce([]); - - const service = await UserService.getInstance(); - await expect( - service.getUserIdByGuid('550e8400-e29b-41d4-a716-446655440000') - ).rejects.toThrow('User not found'); - }); - - it('should throw when result is null', async () => { - mockGetTableRecords.mockResolvedValueOnce(null as unknown as []); - - const service = await UserService.getInstance(); - await expect( - service.getUserIdByGuid('550e8400-e29b-41d4-a716-446655440000') - ).rejects.toThrow('User not found'); - }); - - it('should throw for malformed GUID (via validateGuid)', async () => { - const service = await UserService.getInstance(); - await expect(service.getUserIdByGuid('not-a-guid')).rejects.toThrow('Invalid GUID format'); - }); - }); }); diff --git a/src/services/userService.ts b/src/services/userService.ts index dd5bd33..3c3e974 100644 --- a/src/services/userService.ts +++ b/src/services/userService.ts @@ -58,26 +58,6 @@ export class UserService { * @returns Promise - The user profile data from Ministry Platform * @throws Will throw an error if the Ministry Platform query fails */ - /** - * Looks up the Ministry Platform User_ID for a given User GUID - * - * @param guid - The User GUID to resolve - * @returns Promise - The numeric User_ID - * @throws Will throw an error if the user is not found - */ - public async getUserIdByGuid(guid: string): Promise { - const records = await this.mp!.getTableRecords<{ User_ID: number }>({ - table: 'dp_Users', - select: 'User_ID', - filter: `User_GUID = '${validateGuid(guid)}'`, - top: 1, - }); - if (!records || records.length === 0) { - throw new Error('User not found'); - } - return records[0].User_ID; - } - public async getUserProfile(id: string): Promise { const records = await this.mp!.getTableRecords({ table: "dp_Users", From 64d9db7553a4e5ff09bb1aee9a44a1d574fbc6ab Mon Sep 17 00:00:00 2001 From: Chris Kehayias Date: Sun, 13 Sep 2026 06:40:59 -0400 Subject: [PATCH 3/3] docs(references): sync auth references to the no-parameter profile action MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Updates the ~25 reference sites that documented `getCurrentUserProfile(id)` and the old `session.user.id` guard, across components/layout, contexts/user-provider, data-flow/call-graphs, data-flow/error-catalog, auth/{user-identity,sessions,oauth-flow}, DECISIONS, testing/inventory, security/README, and both READMEs. The security carve-out entry gains one clause noting the "own profile" justification is now enforced by the signature rather than asserted. Also repairs pre-existing drift found while in these files — this doc set carries line-number citations, so stale ones are not cosmetic: - `Error('User GUID not found in session')` no longer exists anywhere in `src/`; the catalog row citing four action files is removed. - `Error('Unauthorized')` had eight cited sites; seven no longer throw it. Rewritten to the single surviving site, with a row added for the `UnauthorizedError` the gate now raises in their place. - `auth-wrapper.tsx` line count, test count, and verbatim implementation block were all stale — the block omitted the `userGuid`-missing redirect to `/session-error` entirely. `last_verified` is bumped only on files verified in full; call-graphs.md and error-catalog.md are left unbumped because only their auth sections were checked, and claiming otherwise would overstate coverage. KNOWN GAP, deliberately not fixed here: ~20 reference mentions of `UserService.getUserIdByGuid` survive, several instructing readers to resolve `$userId` through it. That method no longer exists, and the instruction contradicted rule 13 even before it was deleted. Wider than this change and scoped to a follow-up sweep. Co-Authored-By: Claude Opus 5 (1M context) --- .../port-better-auth-1.6-userguid.md | 10 +++- .claude/references/DECISIONS.md | 2 +- .claude/references/auth/oauth-flow.md | 3 +- .claude/references/auth/sessions.md | 2 +- .claude/references/auth/user-identity.md | 3 +- .claude/references/components/layout.md | 54 ++++++++++++------- .claude/references/contexts/user-provider.md | 30 +++++------ .claude/references/data-flow/call-graphs.md | 12 ++--- .claude/references/data-flow/error-catalog.md | 12 ++--- .claude/references/security/README.md | 3 +- .claude/references/testing/inventory.md | 2 +- README.md | 4 +- src/components/shared-actions/README.md | 7 ++- 13 files changed, 88 insertions(+), 56 deletions(-) diff --git a/.claude/playbooks/port-better-auth-1.6-userguid.md b/.claude/playbooks/port-better-auth-1.6-userguid.md index a7c8703..f3747db 100644 --- a/.claude/playbooks/port-better-auth-1.6-userguid.md +++ b/.claude/playbooks/port-better-auth-1.6-userguid.md @@ -81,9 +81,15 @@ plugin. Better Auth generates its own internal `user.id`; the MP `User_GUID` (th OAuth `sub`) is carried on the session as a **custom user `additionalField`** named `userGuid`, populated server-side from the OAuth profile via `mapProfileToUser`. **Everything MP-related keys off `userGuid`** — the client -`UserProvider` calls `getCurrentUserProfile(userGuid)` to load the profile +`UserProvider` calls `getCurrentUserProfile()` to load the profile (avatar, name). No `userGuid` → no profile → dead avatar/menu. +> If your fork still declares that action as `getCurrentUserProfile(userGuid)`, +> fix it separately: a server action is a caller-shaped POST endpoint, so the +> parameter is an IDOR — any authenticated MP user can read another user's +> profile. Derive the GUID from the session inside the action. Unrelated to the +> 1.6 upgrade, but you will be looking right at the code. + **The breaking change:** As of Better Auth **1.6**, the function that pulls additional fields off an OAuth provider profile stopped letting a field declared `input: false` through when a value is supplied. Two things changed versus the @@ -643,7 +649,7 @@ unusual route-group layout — stop and ask the user before improvising. because `genericOAuth`'s `additionalFields` aren't inferred — e.g. `(session?.user as { userGuid?: string })?.userGuid`. - **The avatar/menu chain end-to-end:** `useSession()` → `session.user.userGuid` - → `UserProvider` → `getCurrentUserProfile(userGuid)` → `MPUserProfile` + → `UserProvider` → `getCurrentUserProfile()` → `MPUserProfile` (`Image_GUID`, names) → `Header` renders the photo + `UserMenu`. Any break in `userGuid` collapses the whole chain to a non-interactive fallback (a generic `UserCircleIcon`, not text initials). diff --git a/.claude/references/DECISIONS.md b/.claude/references/DECISIONS.md index 72cd75c..e98eb01 100644 --- a/.claude/references/DECISIONS.md +++ b/.claude/references/DECISIONS.md @@ -80,7 +80,7 @@ Architectural decisions captured by the context-engineering review at SHA `971c4 **Date:** 2026-04-17 **Status:** Accepted **Context:** A tempting design is to enrich the session object inside `customSession` with the user's full MP profile (roles, user groups, Contact_ID, Image_GUID). Every consumer would then read a single object. The downside is that `customSession` runs on every cache miss, and MP profile lookups require extra MP API calls (`dp_Users` + `dp_User_Roles` + `dp_User_User_Groups`). -**Decision:** `customSession` in `src/lib/auth.ts:97-112` does only `firstName` / `lastName` splitting from `user.name` — no API calls. MP profile loading moves to the client, behind `UserProvider` (`src/contexts/user-context.tsx`), which calls the `getCurrentUserProfile(userGuid)` server action on mount and exposes `useUser()`. +**Decision:** `customSession` in `src/lib/auth.ts:398-413` does only `firstName` / `lastName` splitting from `user.name` — no API calls. MP profile loading moves to the client, behind `UserProvider` (`src/contexts/user-context.tsx`), which calls the parameterless `getCurrentUserProfile()` server action on mount (the action re-derives the GUID from the session — a GUID parameter would be an IDOR) and exposes `useUser()`. **Consequences:** `getSession()` stays cheap. Sign-in does not break when MP is down. Client components that need roles/groups must mount under `UserProvider`; every page load incurs one extra round-trip. `UserService.getUserProfile()` issues three queries (profile + roles + groups). **Alternatives considered:** - **Enrich in `customSession`** — would hit MP API on every JWT refresh and couple sign-in availability to MP uptime. diff --git a/.claude/references/auth/oauth-flow.md b/.claude/references/auth/oauth-flow.md index 7bff4a6..e7af464 100644 --- a/.claude/references/auth/oauth-flow.md +++ b/.claude/references/auth/oauth-flow.md @@ -119,7 +119,8 @@ mapProfileToUser: (profile) => { e. Creates account (accountId=sub, tokens) — storeAccountCookie: true f. Creates session → sets JWT cookie (cookieCache) 8. Browser lands on callbackURL (app page) -9. Client-side UserProvider reads session.user.userGuid → getCurrentUserProfile(userGuid) +9. Client-side UserProvider reads session.user.userGuid (to decide whether to load) + → getCurrentUserProfile() [server action re-derives the GUID from the session] ``` ## Sign-in entry (verbatim from `src/app/signin/page.tsx`) diff --git a/.claude/references/auth/sessions.md b/.claude/references/auth/sessions.md index 633d39c..d1364d5 100644 --- a/.claude/references/auth/sessions.md +++ b/.claude/references/auth/sessions.md @@ -50,7 +50,7 @@ user: { ## `customSession` (verbatim) ```typescript -// src/lib/auth.ts:97-112 +// src/lib/auth.ts:398-413 customSession( async ({ user, session }) => { // No API calls here — profile loading is handled by UserProvider diff --git a/.claude/references/auth/user-identity.md b/.claude/references/auth/user-identity.md index bb69320..32469d4 100644 --- a/.claude/references/auth/user-identity.md +++ b/.claude/references/auth/user-identity.md @@ -123,12 +123,13 @@ const userGuid = (session?.user as { userGuid?: string } | undefined)?.userGuid; ```typescript // src/contexts/user-context.tsx:29-49 (excerpt) +// userGuid gates whether the load fires; it is NOT passed to the action. const userGuid = (session?.user as { userGuid?: string } | undefined)?.userGuid; const loadUserProfile = useCallback(async () => { if (!userGuid) { /* ... */ return; } // ... - const profile = await getCurrentUserProfile(userGuid); + const profile = await getCurrentUserProfile(); // no argument: the action re-derives the GUID server-side setUserProfile(profile ?? null); }, [userGuid]); ``` diff --git a/.claude/references/components/layout.md b/.claude/references/components/layout.md index e19d458..8618984 100644 --- a/.claude/references/components/layout.md +++ b/.claude/references/components/layout.md @@ -15,14 +15,14 @@ related: - ../routing/README.md - ../services/README.md - tool-framework.md -last_verified: 2026-04-17 +last_verified: 2026-09-13 --- ## Purpose `AuthWrapper` is the server-side session gate used at the app-shell level; it redirects unauthenticated requests to `/signin` while preserving the original path+query as `callbackUrl`. `shared-actions/` holds server actions used across multiple features (currently just `getCurrentUserProfile`). ## Files -- `src/components/layout/auth-wrapper.tsx` — server component, 20 lines +- `src/components/layout/auth-wrapper.tsx` — server component, 31 lines - `src/components/layout/auth-wrapper.test.tsx` — redirect + callback preservation tests - `src/components/layout/index.ts` — barrel: `AuthWrapper` - `src/components/shared-actions/user.ts` — `getCurrentUserProfile` server action @@ -36,6 +36,8 @@ last_verified: 2026-04-17 - The `x-pathname` header is set upstream by the proxy (`src/proxy.ts`) so the server component can see the original requested URL; it falls back to `/` when absent. - `shared-actions/user.ts` is marked `'use server'` at the top of the file — all exports are server actions. - Shared actions re-validate auth inside each action (`auth.api.getSession(...)`) — they do not trust the caller. +- `getCurrentUserProfile` takes **no parameters**. The MP `User_GUID` is derived from the session inside the action. A caller-supplied GUID would be a live IDOR: server actions are caller-shaped POST endpoints, so any authenticated MP user could have read another user's contact details, roles, and user groups. +- The guard keys on a non-empty-string `session.user.userGuid` (declared `required: true` in `src/lib/auth.ts`), not `session.user.id` — the latter is Better Auth's internal ID and its presence does not prove an MP identity exists. ## API / Interface @@ -65,19 +67,28 @@ export async function AuthWrapper({ children }: { children: React.ReactNode }) { redirect(`${signinUrl.pathname}${signinUrl.search}`); } + // A session without a userGuid is unusable: every MP lookup keys off userGuid, + // and without it the header avatar/menu never renders — which leaves the user + // with no way to even sign out (the trap behind the better-auth 1.6 regression). + // Route these broken sessions to a recovery page that CAN sign them out, + // rather than rendering a dead app. /session-error lives outside the (web) + // route group, so it is not wrapped by AuthWrapper and cannot redirect-loop. + const userGuid = (session.user as { userGuid?: string | null }).userGuid; + if (!userGuid) { + redirect("/session-error"); + } + return <>{children}; } ``` ### `getCurrentUserProfile` -Source: `src/components/shared-actions/user.ts:8` +Source: `src/components/shared-actions/user.ts:25` ```typescript -export async function getCurrentUserProfile( - id: string -): Promise +export async function getCurrentUserProfile(): Promise ``` -Implementation: +Implementation (docstring elided — see source for the IDOR rationale): ```typescript 'use server'; @@ -86,13 +97,13 @@ import { MPUserProfile } from "@/lib/providers/ministry-platform/types"; import { UserService } from '@/services/userService'; import { headers } from 'next/headers'; -export async function getCurrentUserProfile(id: string): Promise { +export async function getCurrentUserProfile(): Promise { const session = await auth.api.getSession({ headers: await headers() }); - if (!session?.user?.id) throw new Error('Unauthorized'); + const userGuid = (session?.user as Record | undefined)?.userGuid; + if (typeof userGuid !== 'string' || userGuid.length === 0) throw new Error('Unauthorized'); const userService = await UserService.getInstance(); - const userProfile = await userService.getUserProfile(id); - return userProfile; + return userService.getUserProfile(userGuid); } ``` @@ -101,30 +112,36 @@ export async function getCurrentUserProfile(id: string): Promise`, call `next/navigation` `redirect()` (which throws internally to abort rendering) - 4. On valid session, render `<>{children}` + 4. On a session with no `userGuid`, `redirect("/session-error")` — that route sits outside the `(web)` group, so it is not itself wrapped and cannot loop + 5. On valid session, render `<>{children}` - **`getCurrentUserProfile` flow** - 1. Re-validate session via `auth.api.getSession()` — if no `session.user.id`, throw `Unauthorized` + 1. Re-validate session via `auth.api.getSession()` — if `session.user.userGuid` is not a non-empty string, throw `Unauthorized` 2. Await `UserService.getInstance()` (async singleton) - 3. Delegate to `userService.getUserProfile(id)` and return the `MPUserProfile` (or `undefined`) + 3. Delegate to `userService.getUserProfile(userGuid)` and return the `MPUserProfile` (or `undefined`) ## Shared Actions catalog | Export | File | Purpose | |---|---|---| -| `getCurrentUserProfile(id)` | `src/components/shared-actions/user.ts:8` | Fetch the current user's MP profile (`MPUserProfile`) by `User_GUID`; throws `Unauthorized` if no session. Backed by `UserService.getUserProfile`. | +| `getCurrentUserProfile()` | `src/components/shared-actions/user.ts:25` | Fetch the **calling** user's MP profile (`MPUserProfile`); `User_GUID` comes from the session, never from a parameter. Throws `Unauthorized` if the session has no `userGuid`. Backed by `UserService.getUserProfile`. | Guidelines (verbatim from `src/components/shared-actions/README.md`): - Place actions here when they are **used by multiple components across different features**, provide **shared utility**, or handle **cross-cutting concerns**. - Keep actions **co-located** when they are feature-specific or tightly coupled to a single feature's logic. ## Tests -- `src/components/layout/auth-wrapper.test.tsx` — 4 cases: +- `src/components/layout/auth-wrapper.test.tsx` — 6 cases: - redirects with `callbackUrl` from `x-pathname` - falls back to `/` when `x-pathname` is missing - preserves URL-encoded query params through the redirect + - redirects to `/session-error` when `userGuid` is absent + - redirects to `/session-error` when `userGuid` is `null` - returns children when authenticated -- `src/components/shared-actions/user.test.ts` — 3 cases: - - passes `id` through to `UserService.getUserProfile` and returns the profile +- `src/components/shared-actions/user.test.ts` — 6 cases: + - looks the profile up with the session's `userGuid` and returns it + - ignores a caller-forged argument (cast through `unknown`) and still uses the session GUID + - throws `Unauthorized` when the session has no `userGuid` + - throws `Unauthorized` when `userGuid` is an empty string - throws `Unauthorized` when `auth.api.getSession()` returns `null` - propagates service-layer errors @@ -135,6 +152,7 @@ Both test files use `vi.hoisted()` to share mock references (required pattern - **`redirect()` throws.** `next/navigation` `redirect()` aborts rendering by throwing a magic error. Do not wrap in try/catch; do not add code after the redirect call expecting it to run on the unauthenticated branch. - **`callbackUrl` relies on `x-pathname`.** If a route bypasses the proxy (or a future proxy matcher excludes it), `x-pathname` will be missing and unauthenticated users land on `/` after sign-in. Verify proxy matcher coverage in `src/proxy.ts` when adding new protected routes. - **Shared actions must re-validate auth.** `getCurrentUserProfile` calls `auth.api.getSession()` itself rather than trusting caller context. Any new action added here must do the same (see `../auth/README.md` for session access patterns). +- **Never re-add an identity parameter.** `getCurrentUserProfile` is a CLAUDE.md rule-12 carve-out from the `AuthorizationService` gate on the grounds that it returns only the caller's own profile. That justification holds only because there is no GUID argument to forge; adding one re-opens the IDOR. ## Related docs - `../auth/README.md` — Better Auth session shape, `session.user.userGuid` vs `session.user.id` diff --git a/.claude/references/contexts/user-provider.md b/.claude/references/contexts/user-provider.md index 6e00444..204692d 100644 --- a/.claude/references/contexts/user-provider.md +++ b/.claude/references/contexts/user-provider.md @@ -5,26 +5,26 @@ type: reference applies_to: [src/contexts/user-context.tsx, src/contexts/user-context.test.tsx, src/app/providers.tsx] symbols: [UserProvider, useUser, MPUserProfile] related: [session.md, ../auth/README.md, ../services/README.md] -last_verified: 2026-04-17 +last_verified: 2026-09-13 --- ## Purpose -Client context that watches the Better Auth session, extracts `userGuid` (OIDC `sub` → MP `User_GUID`), and loads the enriched `MPUserProfile` (roles + groups) via the `getCurrentUserProfile` server action. +Client context that watches the Better Auth session, extracts `userGuid` (OIDC `sub` → MP `User_GUID`), and loads the enriched `MPUserProfile` (roles + groups) via the `getCurrentUserProfile` server action. `userGuid` gates *whether* the load fires; the action re-derives it server-side and takes no argument. ## Files - `src/contexts/user-context.tsx` — provider + hook + error handling - `src/contexts/user-context.test.tsx` — 6 test cases covering load/error/refresh/missing-guid - `src/app/providers.tsx` — mounts `` at the app shell -- `src/components/shared-actions/user.ts` — `getCurrentUserProfile(id)` server action -- `src/services/userService.ts:81` — `UserService.getUserProfile(id)` downstream lookup +- `src/components/shared-actions/user.ts` — `getCurrentUserProfile()` server action (no parameters; derives the GUID from the session) +- `src/services/userService.ts:81` — `UserService.getUserProfile(guid)` downstream lookup - `src/lib/providers/ministry-platform/types/user-profile.types.ts` — `MPUserProfile` shape ## Key concepts - **Client-side profile load** — MP profile is fetched after mount, not injected by the server. Page renders before `userProfile` is available (`isLoading` starts `true`). - **`userGuid` is the key** — `session.user.id` is Better Auth's internal ID; `session.user.userGuid` is the MP `User_GUID`. Only `userGuid` is used for MP lookups (`user-context.tsx:27-29`). -- **Effect gating** — the effect only fires load when `!isPending && userGuid`; the else-branch (`!isPending && !session`) clears state. If the session exists but has no `userGuid`, **neither branch runs** (`user-context.tsx:51-58`; test at `user-context.test.tsx:88-99`). +- **Effect gating** — the effect only fires load when `!isPending && userGuid`; the else-branch (`!isPending && !session`) clears state. If the session exists but has no `userGuid`, **neither branch runs** (`user-context.tsx:51-59`; test at `user-context.test.tsx:88-99`). - **Error state is local** — `getCurrentUserProfile` rejections are caught; `error` is exposed on context, `userProfile` is reset to `null`, `isLoading` ends `false` (`user-context.tsx:43-48`). -- **`useUser` throws outside a provider** — guard at `user-context.tsx:78-80`. +- **`useUser` throws outside a provider** — guard at `user-context.tsx:79-81`. ## API / Interface @@ -64,7 +64,7 @@ export interface MPUserProfile { - `useEffect` fires `loadUserProfile()` when `!isPending && userGuid` truthy. - `loadUserProfile`: - Sets `isLoading=true`, `error=null`. - - Calls `getCurrentUserProfile(userGuid)`. + - Calls `getCurrentUserProfile()` — no argument; the action re-derives the GUID from the server-side session. - On success: `setUserProfile(profile ?? null)`. - On failure: `setError(Error)`, `setUserProfile(null)`. - `finally` block sets `isLoading=false`. @@ -92,7 +92,7 @@ export function Providers({ children }: ProvidersProps) { } ``` -Consumer hook (from `src/contexts/user-context.tsx:76-82`): +Consumer hook (from `src/contexts/user-context.tsx:77-83`): ```typescript export function useUser() { @@ -112,10 +112,10 @@ UserProvider mount └─ userGuid = session.user.userGuid └─ useEffect [!isPending && userGuid] └─ loadUserProfile() - └─ getCurrentUserProfile(userGuid) (server action: src/components/shared-actions/user.ts) - └─ auth.api.getSession({ headers }) (re-verifies session server-side) + └─ getCurrentUserProfile() (server action: src/components/shared-actions/user.ts:25) + └─ auth.api.getSession({ headers }) (re-verifies session; throws Unauthorized unless user.userGuid is a non-empty string) └─ UserService.getInstance() - └─ UserService.getUserProfile(id) (src/services/userService.ts:81) + └─ UserService.getUserProfile(userGuid) (src/services/userService.ts:81 — GUID from the session, never the caller) ├─ MP getTableRecords dp_Users (filter: User_GUID = '...') ├─ MP getTableRecords dp_User_Roles (filter: User_ID = ...) └─ MP getTableRecords dp_User_User_Groups (filter: User_ID = ...) @@ -128,16 +128,16 @@ UserProvider mount | Test | Line | Asserts | |---|---|---| | throws outside provider | 34 | `useUser()` without `` throws | -| loads profile when session has userGuid | 47 | `getCurrentUserProfile` called with guid, profile set, `error` null | +| loads profile when session has userGuid | 47 | `getCurrentUserProfile` called with **no arguments**, profile set, `error` null | | null profile when no session | 72 | `data: null` → `userProfile=null`, action not called | | no fetch when session lacks userGuid | 88 | `data: { user: { id } }` (no `userGuid`) → action not called | | handles profile load error | 101 | rejected promise → `error` set, `userProfile=null` | | `refreshUserProfile` re-fetches | 119 | second call returns updated profile; action called twice | ## Gotchas -- Reading `useUser().userProfile` before load returns `null` — gate UI on `isLoading` (inline; `user-context.tsx:24,51-58`). -- Consumer must be a client component and wrapped by `` — the hook throws otherwise (`user-context.tsx:78-80`). -- Session without `userGuid` leaves `isLoading` at its previous value — if the session switches to "missing guid" mid-lifecycle, neither branch of the effect clears it (`user-context.tsx:51-58`). Prefer `useAppSession` + explicit checks in components that need to distinguish "loading" vs "guid missing". +- Reading `useUser().userProfile` before load returns `null` — gate UI on `isLoading` (inline; `user-context.tsx:24,51-59`). +- Consumer must be a client component and wrapped by `` — the hook throws otherwise (`user-context.tsx:79-81`). +- Session without `userGuid` leaves `isLoading` at its previous value — if the session switches to "missing guid" mid-lifecycle, neither branch of the effect clears it (`user-context.tsx:51-59`). Prefer `useAppSession` + explicit checks in components that need to distinguish "loading" vs "guid missing". - Direct import from `@/lib/auth-client` bypasses the `useAppSession` wrapper — `UserProvider` itself does this (`user-context.tsx:22`) because it needs `isPending` which `useAppSession` drops. ## Related docs diff --git a/.claude/references/data-flow/call-graphs.md b/.claude/references/data-flow/call-graphs.md index a64c5de..630f3ea 100644 --- a/.claude/references/data-flow/call-graphs.md +++ b/.claude/references/data-flow/call-graphs.md @@ -160,12 +160,12 @@ last_verified: 2026-04-17 2. `src/contexts/user-context.tsx:22` — `authClient.useSession()` (reactive subscription to JWT cookie cache). 3. `src/contexts/user-context.tsx:29` — derive `userGuid = (session?.user as { userGuid?: string } | undefined)?.userGuid`. 4. `src/contexts/user-context.tsx:51-58` — `useEffect` fires when `!isPending && userGuid`: calls `loadUserProfile()` (line 53). -5. `src/contexts/user-context.tsx:31-49` — `loadUserProfile` sets `isLoading=true`, calls `getCurrentUserProfile(userGuid)` (line 41). -6. `src/components/shared-actions/user.ts:8` — server action `getCurrentUserProfile(id)`. -7. `src/components/shared-actions/user.ts:9-10` — `auth.api.getSession({ headers: await headers() })`; throws `'Unauthorized'` if no `session.user.id`. -8. `src/components/shared-actions/user.ts:12-13` — `UserService.getInstance()` → `userService.getUserProfile(id)`. +5. `src/contexts/user-context.tsx:31-49` — `loadUserProfile` sets `isLoading=true`, calls `getCurrentUserProfile()` (line 41) with no arguments. +6. `src/components/shared-actions/user.ts:25` — server action `getCurrentUserProfile()`; takes no parameters, so a caller cannot name another user (IDOR). +7. `src/components/shared-actions/user.ts:26-28` — `auth.api.getSession({ headers: await headers() })`, then derive `userGuid` from `session.user`; throws `'Unauthorized'` unless it is a non-empty string. +8. `src/components/shared-actions/user.ts:30-31` — `UserService.getInstance()` → `userService.getUserProfile(userGuid)`. 9. `src/services/userService.ts:81-110` — runs 3 MP queries: - - `mp.getTableRecords('dp_Users', { filter: "User_GUID = ''", select: "User_ID, User_GUID, Contact_ID_TABLE.First_Name, ..., Contact_ID_TABLE.dp_fileUniqueId AS Image_GUID", top: 1 })` at lines 82-87. + - `mp.getTableRecords('dp_Users', { filter: "User_GUID = ''", select: "User_ID, User_GUID, Contact_ID_TABLE.First_Name, ..., Contact_ID_TABLE.dp_fileUniqueId AS Image_GUID", top: 1 })` at lines 86-91. - `Promise.all([dp_User_Roles fetch, dp_User_User_Groups fetch])` at lines 92-103, keyed by the numeric `User_ID` from the first query. 10. `src/services/userService.ts:105-109` — returns `{ ...profile, roles: string[], userGroups: string[] }`. 11. `src/contexts/user-context.tsx:42` — `setUserProfile(profile ?? null)`. @@ -176,7 +176,7 @@ last_verified: 2026-04-17 **Error paths:** - Server action throws → caught at `src/contexts/user-context.tsx:43-46` → sets `error` state, `userProfile = null`. -- No `session.user.id` → `Error('Unauthorized')` at `src/components/shared-actions/user.ts:10`. +- Session `userGuid` missing or empty → `Error('Unauthorized')` at `src/components/shared-actions/user.ts:28`. (Not keyed on `session.user.id` — that is Better Auth's internal ID and does not prove an MP identity.) - Profile row missing → `userProfile = undefined` returned at `src/services/userService.ts:90`; normalized to `null` in context (line 42). **Return shape:** `MPUserProfile | null` in context state (with `roles: string[]`, `userGroups: string[]`). diff --git a/.claude/references/data-flow/error-catalog.md b/.claude/references/data-flow/error-catalog.md index 9f55dfb..04719fe 100644 --- a/.claude/references/data-flow/error-catalog.md +++ b/.claude/references/data-flow/error-catalog.md @@ -53,10 +53,10 @@ All MP REST errors originate in `src/lib/providers/ministry-platform/utils/http- | `Error('Invalid GUID format: ${value}')` | `src/lib/validation.ts:6` (`validateGuid`) | server-action catch (e.g., `userService` caller) | component catches → `setError` | no | | `Error('Expected positive integer, got: ${value}')` | `src/lib/validation.ts:13` (`validatePositiveInt`) | service callsite try/catch propagates to action | component catches → `setError` | no | | `Error('Invalid column name: ${value}')` | `src/lib/validation.ts:20` (`validateColumnName`) | `toolService.resolveContactIds` callsite → action | component catches → `setError` | no | -| `Error('Unauthorized')` | `src/components/address-labels/actions.ts:30`, `src/components/dev-panel/panels/selection-actions.ts:18`, `src/components/dev-panel/panels/contact-records-actions.ts:15`, `src/components/dev-panel/panels/user-tools-actions.ts:12`, `src/components/field-management/actions.ts:10`, `src/components/group-wizard/actions.ts:19`, `src/components/shared-actions/user.ts:10`, `src/components/template-editor/actions.ts:9` | component `try/catch` → `setError` (or `ActionError` shape for group-wizard) | toast / inline error | no | -| `Error('Unauthorized - Missing user session data')` (deploy-tool, user-tools variants) | `src/components/dev-panel/panels/deploy-tool-actions.ts:19`, `src/components/dev-panel/panels/user-tools-actions.ts:12` | component catch → `setError` | as above | no | -| `Error('User GUID not found in session')` | `src/components/address-labels/actions.ts:36`, `src/components/dev-panel/panels/selection-actions.ts:21`, `src/components/dev-panel/panels/user-tools-actions.ts:17`, `src/components/group-wizard/actions.ts:25` | component catch → `setError` | toast / inline | no | -| `Error('Deploy Tool is not available in production.')` | `src/components/dev-panel/panels/deploy-tool-actions.ts:15` | deploy-tool panel catch → `setError` | "production" warning to user | no | +| `Error('Unauthorized')` | `src/components/shared-actions/user.ts:28` — session has no non-empty `userGuid` | `src/contexts/user-context.tsx:43-46` try/catch → `error` state | silent (consumers of `useUser()` inspect `error`) | no | +| `UnauthorizedError('Not authorized')` (`code: "UNAUTHORIZED"`) | `src/services/authorizationService.ts:186` (`requireSecurityRole`) — the gate every feature action and service method calls | component `try/catch` → `setError` (or `ActionError` shape for group-wizard) | toast / inline error | yes — `console.warn` of `{table, operation, reason, userId}` only (identifiers/shape, never record content) | +| `Error('Unauthorized - Missing user session data')` | `src/components/dev-panel/panels/require-dev-session.ts:34` — single guard shared by every dev-panel action | component catch → `setError` | as above | no | +| ``Error(`${featureLabel} is not available in production.`)`` | `src/components/dev-panel/panels/require-dev-session.ts:30` (`featureLabel` defaults to `"Dev panel"`; deploy-tool passes `"Deploy Tool"`) | dev-panel / deploy-tool panel catch → `setError` | "production" warning to user | no | | `Error('MJML source must be between 1 and 512000 characters')` | `src/components/template-editor/actions.ts:19` | editor dialogs (`editor-code-dialog.tsx`, `editor-export-dialog.tsx`) catch | inline error state | no | | Zod `ZodError` via `zodResolver` (group wizard) | `src/components/group-wizard/schema.ts` (`groupWizardSchema`) | RHF `form.trigger(...)` in `src/app/(web)/tools/groupwizard/group-wizard.tsx:104` | in-form field error messages | no | | `Error('Input must contain only digits')` (IMb) | `src/lib/imb-encoder.ts:323` | `src/lib/barcode-helpers.ts:48` **silent catch — falls through to POSTNET** | no user-facing error; label prints without IMb | no (silent) | @@ -70,7 +70,7 @@ All MP REST errors originate in `src/lib/providers/ministry-platform/utils/http- | Type | Thrown at | Caught at | User-facing | Logged | |---|---|---|---|---| -| `Error('User not found')` | `src/services/userService.ts:76` (`getUserIdByGuid`) | server action (e.g., `user-tools-actions.ts`, `address-labels/actions.ts:36`) propagates → component catch | inline error / toast | no | +| `Error('User not found')` | `src/services/userService.ts:76` (`getUserIdByGuid`) | caller propagates → component catch | inline error / toast | no | | `Error('Tool Name is required')` | `src/services/toolService.ts:248` (`deployTool` guard) | deploy-tool UI catch → `setError` | inline error | no | | `Error('Launch Page is required')` | `src/services/toolService.ts:249` | deploy-tool UI | inline | no | | `Error('Tool Name must be 30 characters or fewer')` | `src/services/toolService.ts:250` | deploy-tool UI | inline | no | @@ -92,7 +92,7 @@ These do NOT throw to the component; server action returns `{ success: false, er | PDF render failure (`@react-pdf/renderer` `toBlob`) | throws inside try at `src/components/address-labels/actions.ts:157` | `:162-168` wraps as `{success:false, error: message}` | inline error | yes, `console.error('generateLabelPdf error:', error)` | | Docx render failure (`Packer.toBuffer`) | throws inside try at `src/components/address-labels/actions.ts:190` | `:194-200` wraps as envelope | inline error | yes, `console.error('generateLabelDocx error:', error)` | | Docxtemplater render error (tag mismatch) | throws inside `doc.render` at `src/components/address-labels/actions.ts:273` | `:279-286` wraps; if message includes `'tag'` returns prettier error | inline error | yes, `console.error('mergeTemplate error:', error)` | -| Profile fetch failure in `UserProvider` | `src/components/shared-actions/user.ts:10` (`Unauthorized`) or downstream MP error from `UserService.getUserProfile` | `src/contexts/user-context.tsx:43-46` try/catch sets `error` state | silent (consumers of `useUser()` inspect `error`) | no | +| Profile fetch failure in `UserProvider` | `src/components/shared-actions/user.ts:28` (`Unauthorized` — session has no `userGuid`) or downstream MP error from `UserService.getUserProfile` | `src/contexts/user-context.tsx:43-46` try/catch sets `error` state | silent (consumers of `useUser()` inspect `error`) | no | | `Error("useUser must be used within a UserProvider")` | `src/contexts/user-context.tsx:79` | uncaught — surfaces to nearest React error boundary | white-screen error unless a boundary is present | no | | `Error("useFormField should be used within ")` | `src/components/ui/form.tsx:53` | uncaught — React error boundary | same as above | no | | "Invalid JSON data…" (template-editor import) | `JSON.parse` throws at `src/components/template-editor/editor-import-dialog.tsx:34` | `:36` silent catch → `setError('Invalid JSON data…')` | inline error | no | diff --git a/.claude/references/security/README.md b/.claude/references/security/README.md index 9dbea02..0b4e1bf 100644 --- a/.claude/references/security/README.md +++ b/.claude/references/security/README.md @@ -82,7 +82,8 @@ data. Each documents why **in-file**. Adding a fifth needs the same justification, in the file, in writing. - `components/layout/auth-wrapper.tsx` — it *is* the session gate -- `components/shared-actions/user.ts` — the user's own profile +- `components/shared-actions/user.ts` — the user's own profile; enforced by the + signature (no parameter to forge), not just asserted - `components/shared-actions/domain.ts` — the domain-wide time zone (one string) - `components/dev-panel/panels/require-dev-session.ts` — dev-only (`NODE_ENV !== "production"`), and the services it calls gate anyway diff --git a/.claude/references/testing/inventory.md b/.claude/references/testing/inventory.md index c90c613..148caf5 100644 --- a/.claude/references/testing/inventory.md +++ b/.claude/references/testing/inventory.md @@ -51,7 +51,7 @@ All 37 test files grouped by area. Totals from facts snapshot: **37 files / 507 | `src/components/dev-panel/panels/user-tools-actions.test.ts` | Authorization checks, session validation | | `src/components/layout/auth-wrapper.test.tsx` | `AuthWrapper` render gating based on session | | `src/components/user-menu/actions.test.ts` | Sign-out action, OIDC logout redirect | -| `src/components/shared-actions/user.test.ts` | `getCurrentUserProfile` delegation | +| `src/components/shared-actions/user.test.ts` | `getCurrentUserProfile` session-derived GUID lookup, `Unauthorized` on missing/empty `userGuid`, IDOR regression (caller-forged argument ignored), error propagation | ## Core lib (8 files) diff --git a/README.md b/README.md index e705e2f..04f079f 100644 --- a/README.md +++ b/README.md @@ -613,7 +613,7 @@ Alert, Alert Dialog, Avatar, Badge, Breadcrumb, Button, Card, Checkbox, Command, - **template-editor/** — 12 components for visual template editing with GrapesJS - **user-menu/** — User dropdown with profile display and OIDC sign-out action - **dev-panel/** — Unified developer overlay (localhost-only) showing parsed URL params, MP selection data, contact records, and authorized tools -- **shared-actions/** — Cross-feature server actions (`getCurrentUserProfile`) +- **shared-actions/** — Cross-feature server actions (`getCurrentUserProfile()` — returns the *calling* user's MP profile; the `User_GUID` comes from the session, never from a parameter) All components use kebab-case file naming, PascalCase component names, and named exports with barrel index files. @@ -670,7 +670,7 @@ npm run test:coverage # With coverage report | User Service | `userService.test.ts` | Profile with roles/groups, parallel queries | | User Tools Panel | `user-tools-actions.test.ts` | Authorization checks, session validation | | User Menu | `actions.test.ts` | Sign-out action, OIDC logout redirect | -| Shared Actions | `user.test.ts` | getCurrentUserProfile delegation | +| Shared Actions | `user.test.ts` | getCurrentUserProfile session-derived GUID, unauthorized paths, IDOR regression | | Session Context | `session-context.test.tsx` | useAppSession hook wrapper | | IMb Encoder | `imb-encoder.test.ts` | USPS Intelligent Mail barcode encoding | | POSTNET Encoder | `postnet-encoder.test.ts` | POSTNET barcode encoding | diff --git a/src/components/shared-actions/README.md b/src/components/shared-actions/README.md index e9c7c58..d0ba1d1 100644 --- a/src/components/shared-actions/README.md +++ b/src/components/shared-actions/README.md @@ -18,7 +18,7 @@ Keep actions with their component folder when: ## Examples **Shared Actions (place here):** -- `user.ts` - User profile operations used by contexts and components +- `user.ts` - Current-user profile lookup used by contexts and components - `auth.ts` - Authentication actions used across the app - `analytics.ts` - Analytics tracking used by multiple features - `notifications.ts` - Notification system used app-wide @@ -32,6 +32,11 @@ Keep actions with their component folder when: // Importing shared actions import { getCurrentUserProfile } from '@/components/shared-actions/user'; +// Returns the CALLING user's profile. It takes no arguments by design: a server +// action is a caller-shaped POST endpoint, so a user-id/GUID parameter would let +// any authenticated MP user read anyone else's profile (IDOR). Do not re-add one. +const profile = await getCurrentUserProfile(); + // Importing feature-specific actions import { myAction } from './actions'; // Within same folder ```