diff --git a/.claude/TODO/server-action-search-contacts-unauthenticated.md b/.claude/TODO/server-action-search-contacts-unauthenticated.md deleted file mode 100644 index bd9f7a0d..00000000 --- a/.claude/TODO/server-action-search-contacts-unauthenticated.md +++ /dev/null @@ -1,54 +0,0 @@ -# TODO: `searchContacts` server action has no session check - -**Created:** 2026-08-21 -**Severity:** High — unauthenticated PII disclosure. -**Status:** Open. Documented during the test-coverage push; not fixed, because adding auth here is a -behavior change on a user-facing path. - -## Symptom - -`src/components/contact-lookup/actions.ts` is a `'use server'` action with **zero** `getSession` -calls. It searches `Contacts` across `First_Name`, `Last_Name`, `Nickname`, `Email_Address`, and -`Mobile_Phone`, returning up to 20 matching records **including email address and mobile phone**. - -```ts -export async function searchContacts(searchTerm: string): Promise { - if (!searchTerm || searchTerm.trim().length === 0) return []; - const contactService = await ContactService.getInstance(); - return await contactService.contactSearch(searchTerm.trim()); -} -``` - -## Why this is reachable - -Server actions compile to callable POST endpoints. `src/proxy.ts:8` explicitly allows all `/api` -paths through without a session. Every sibling action file (`contact-logs/actions.ts`, -`contact-lookup-details/actions.ts`) does check the session — so this reads as an oversight, not a -deliberate design decision. - -A one-character search term returns 20 church members with contact details, to any caller. - -## Proposed fix - -Match the sibling pattern exactly: - -```ts -const session = await auth.api.getSession({ headers: await headers() }); -if (!session?.user?.id) { - throw new Error('Authentication required'); -} -``` - -Then add a test asserting an unauthenticated caller is rejected. - -## Why the existing tests missed it - -`contact-lookup/actions.ts` is at 100% statement and branch coverage with 5 passing tests (empty -input, whitespace, trimming, service errors, pass-through). None of them asks the authorization -question, because nothing in the code answers it. Coverage cannot flag a check that was never -written. - -## Related - -- `.claude/TODO/server-action-user-profile-unauthenticated.md` — same class of defect -- `.claude/docs/TestCoverage.md` §7.3 diff --git a/.claude/TODO/server-action-user-profile-unauthenticated.md b/.claude/TODO/server-action-user-profile-unauthenticated.md deleted file mode 100644 index 6c2851c8..00000000 --- a/.claude/TODO/server-action-user-profile-unauthenticated.md +++ /dev/null @@ -1,56 +0,0 @@ -# TODO: `getCurrentUserProfile` has neither authentication nor an ownership check - -**Created:** 2026-08-21 -**Severity:** High — unauthenticated disclosure of arbitrary users' profiles, roles, and groups. -**Status:** Open. Documented during the test-coverage push; not fixed. - -## Symptom - -`src/components/shared-actions/user.ts`: - -```ts -export async function getCurrentUserProfile(id: string): Promise { - const userService = await UserService.getInstance(); - return await userService.getUserProfile(id); -} -``` - -Two problems, not one: - -1. **No session check.** Like `searchContacts`, this is a `'use server'` action reachable as a POST - endpoint with no authentication. -2. **No ownership check.** The name says "current user" but the function takes an arbitrary - `User_GUID` and returns whatever profile that GUID names. `src/services/userService.ts:72-89` - also loads that user's **roles and user groups** — i.e. it discloses the authorization model for - any user whose GUID is known. - -GUIDs are not usefully secret: `session.user.userGuid` is present in the client-side session, and MP -GUIDs appear in URLs elsewhere in the app (`/contactlookup/[guid]`). - -## Proposed fix - -```ts -const session = await auth.api.getSession({ headers: await headers() }); -if (!session?.user?.id) throw new Error('Authentication required'); - -const requested = id ?? session.user.userGuid; -if (requested !== session.user.userGuid) { - // Either reject, or gate on an explicit "may read other users" role. - throw new Error('Forbidden'); -} -``` - -If cross-user reads are genuinely needed by some feature, that is a separate authorized path and -should be a separate, role-gated function — not an unauthenticated one named `getCurrentUserProfile`. -Consider dropping the parameter entirely and reading the GUID from the session, which makes the -ownership question unaskable. - -## Why the existing tests missed it - -Both existing tests pass `'guid-123'` and assert pass-through. 100% coverage, 4/4 statements. The -authorization question is never asked. - -## Related - -- `.claude/TODO/server-action-search-contacts-unauthenticated.md` -- `.claude/docs/TestCoverage.md` §7.4 diff --git a/.claude/docs/TestCoverage.md b/.claude/docs/TestCoverage.md index 81c0c721..00eda80d 100644 --- a/.claude/docs/TestCoverage.md +++ b/.claude/docs/TestCoverage.md @@ -165,21 +165,33 @@ searchContactLogs("5; DROP") → filter: "Contact_ID = 5; DROP" `contactLogService.ts` is at **100% statements and 100% branches**. No test passes a non-numeric value, which is exactly why full coverage did not catch it. -### 5.2 `searchContacts` — no authentication 🔴 +### 5.2 `searchContacts` — no authentication ✅ FIXED -→ `.claude/TODO/server-action-search-contacts-unauthenticated.md` +Was: a `'use server'` action with zero `getSession` calls, returning up to 20 contacts including +email and mobile phone. `proxy.ts:8` allows all `/api` paths without a session, and every sibling +action file did check. 100% statements, 100% branches, 5 passing tests, none of which asked the +authorization question — because nothing in the code answered it. -A `'use server'` action with zero `getSession` calls, returning up to 20 contacts including email and -mobile phone. `proxy.ts:8` allows all `/api` paths without a session, and every sibling action file -does check. 100% statements, 100% branches, 5 passing tests, none of which asks the authorization -question — because nothing in the code answers it. +Now: `searchContacts` calls `auth.api.getSession` and throws `Authentication required` before any +other work. The check sits **before** the try/catch, so the auth failure surfaces as itself rather +than being masked as `Failed to search contacts`, and the empty-search-term early return cannot +become an unauthenticated success path. Three tests cover it: null session, session with no +`user.id`, and rejection of an empty term while unauthenticated. -### 5.3 `getCurrentUserProfile` — no authentication, no ownership check 🔴 +### 5.3 `getCurrentUserProfile` — no authentication, no ownership check ✅ FIXED -→ `.claude/TODO/server-action-user-profile-unauthenticated.md` +Was: took an arbitrary `User_GUID` and returned that user's profile **plus their roles and user +groups** — disclosing the authorization model for any user whose GUID was known. 100% covered; both +tests asserted pass-through. -Takes an arbitrary `User_GUID` and returns that user's profile **plus their roles and user groups**. -100% covered. Both tests assert pass-through. +Now: the parameter is **gone**. The action reads `userGuid` from the session, which makes the +ownership question unaskable rather than merely answered — there is no longer an argument for a +caller to tamper with. It throws `Authentication required` with no session and `User GUID not found +in session` when an authenticated session carries no GUID. `UserProvider` calls it with no argument +and keeps `userGuid` only as an effect dependency so switching users still re-fetches. + +If a feature ever needs to read another user's profile, that is a separate, explicitly role-gated +function — not a widening of this one. ### 5.4 Contact-log actions authenticate but never authorize 🟠 diff --git a/src/components/contact-lookup/actions.test.ts b/src/components/contact-lookup/actions.test.ts index 7f1f5dd8..46e77244 100644 --- a/src/components/contact-lookup/actions.test.ts +++ b/src/components/contact-lookup/actions.test.ts @@ -1,7 +1,20 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -const { mockContactSearch } = vi.hoisted(() => ({ +const { mockContactSearch, mockGetSession } = vi.hoisted(() => ({ mockContactSearch: vi.fn(), + mockGetSession: vi.fn(), +})); + +vi.mock('@/lib/auth', () => ({ + auth: { + api: { + getSession: mockGetSession, + }, + }, +})); + +vi.mock('next/headers', () => ({ + headers: vi.fn().mockResolvedValue(new Headers()), })); vi.mock('@/services/contactService', () => ({ @@ -14,9 +27,36 @@ vi.mock('@/services/contactService', () => ({ import { searchContacts } from './actions'; +const mockAuthSession = { + user: { id: 'internal-id', userGuid: 'user-guid-123' }, +}; + describe('searchContacts', () => { beforeEach(() => { vi.clearAllMocks(); + mockGetSession.mockResolvedValue(mockAuthSession); + }); + + it('should require authentication', async () => { + mockGetSession.mockResolvedValue(null); + + await expect(searchContacts('John')).rejects.toThrow('Authentication required'); + expect(mockContactSearch).not.toHaveBeenCalled(); + }); + + it('should reject a session with no user id', async () => { + mockGetSession.mockResolvedValue({ user: { userGuid: 'user-guid-123' } }); + + await expect(searchContacts('John')).rejects.toThrow('Authentication required'); + expect(mockContactSearch).not.toHaveBeenCalled(); + }); + + it('should reject an unauthenticated caller before validating the search term', async () => { + mockGetSession.mockResolvedValue(null); + + // The empty-term early return must not become an unauthenticated success path. + await expect(searchContacts('')).rejects.toThrow('Authentication required'); + expect(mockContactSearch).not.toHaveBeenCalled(); }); it('should return results for valid search term', async () => { diff --git a/src/components/contact-lookup/actions.ts b/src/components/contact-lookup/actions.ts index a4dc17eb..6532052e 100644 --- a/src/components/contact-lookup/actions.ts +++ b/src/components/contact-lookup/actions.ts @@ -2,8 +2,18 @@ import { ContactService } from '@/services/contactService'; import { ContactSearch } from '@/lib/dto'; +import { auth } from '@/lib/auth'; +import { headers } from 'next/headers'; export async function searchContacts(searchTerm: string): Promise { + // Server actions compile to callable POST endpoints and src/proxy.ts lets all + // /api paths through without a session, so this check is the only thing + // standing between an anonymous caller and 20 contacts' emails and phones. + const session = await auth.api.getSession({ headers: await headers() }); + if (!session?.user?.id) { + throw new Error('Authentication required'); + } + try { if (!searchTerm || searchTerm.trim().length === 0) { return []; @@ -11,10 +21,10 @@ export async function searchContacts(searchTerm: string): Promise ({ +const { mockGetUserProfile, mockGetSession } = vi.hoisted(() => ({ mockGetUserProfile: vi.fn(), + mockGetSession: vi.fn(), +})); + +vi.mock('@/lib/auth', () => ({ + auth: { + api: { + getSession: mockGetSession, + }, + }, +})); + +vi.mock('next/headers', () => ({ + headers: vi.fn().mockResolvedValue(new Headers()), })); vi.mock('@/services/userService', () => ({ @@ -14,36 +27,85 @@ vi.mock('@/services/userService', () => ({ import { getCurrentUserProfile } from './user'; +const mockAuthSession = { + user: { id: 'internal-id', userGuid: 'guid-123' }, +}; + +const mockProfile = { + User_ID: 1, + User_GUID: 'guid-123', + Contact_ID: 100, + First_Name: 'John', + Nickname: 'Johnny', + Last_Name: 'Doe', + Email_Address: 'john@example.com', + Mobile_Phone: null, + Image_GUID: null, + roles: ['Admin'], + userGroups: ['Staff'], +}; + describe('getCurrentUserProfile', () => { beforeEach(() => { vi.clearAllMocks(); }); - it('should call UserService with correct ID', async () => { - const mockProfile = { - User_ID: 1, - User_GUID: 'guid-123', - Contact_ID: 100, - First_Name: 'John', - Nickname: 'Johnny', - Last_Name: 'Doe', - Email_Address: 'john@example.com', - Mobile_Phone: null, - Image_GUID: null, - roles: ['Admin'], - userGroups: ['Staff'], - }; + it('should require authentication', async () => { + mockGetSession.mockResolvedValueOnce(null); + + await expect(getCurrentUserProfile()).rejects.toThrow('Authentication required'); + expect(mockGetUserProfile).not.toHaveBeenCalled(); + }); + + it('should reject a session with no user id', async () => { + mockGetSession.mockResolvedValueOnce({ user: { userGuid: 'guid-123' } }); + + await expect(getCurrentUserProfile()).rejects.toThrow('Authentication required'); + expect(mockGetUserProfile).not.toHaveBeenCalled(); + }); + + it('should reject an authenticated session that carries no userGuid', async () => { + mockGetSession.mockResolvedValueOnce({ user: { id: 'internal-id' } }); + + await expect(getCurrentUserProfile()).rejects.toThrow('User GUID not found in session'); + expect(mockGetUserProfile).not.toHaveBeenCalled(); + }); + + it("should look up the profile using the session's own User_GUID", async () => { + mockGetSession.mockResolvedValueOnce(mockAuthSession); mockGetUserProfile.mockResolvedValueOnce(mockProfile); - const result = await getCurrentUserProfile('guid-123'); + const result = await getCurrentUserProfile(); expect(mockGetUserProfile).toHaveBeenCalledWith('guid-123'); expect(result).toEqual(mockProfile); }); + it('should ignore any caller-supplied GUID and use the session GUID', async () => { + mockGetSession.mockResolvedValueOnce(mockAuthSession); + mockGetUserProfile.mockResolvedValueOnce(mockProfile); + + // A hostile caller can still POST an argument at the compiled endpoint; the + // action takes no parameters, so the victim's GUID must never be used. + await (getCurrentUserProfile as unknown as (id: string) => Promise)( + 'someone-elses-guid' + ); + + expect(mockGetUserProfile).toHaveBeenCalledWith('guid-123'); + expect(mockGetUserProfile).not.toHaveBeenCalledWith('someone-elses-guid'); + }); + + it('should return undefined when MP has no matching user', async () => { + mockGetSession.mockResolvedValueOnce(mockAuthSession); + mockGetUserProfile.mockResolvedValueOnce(undefined); + + await expect(getCurrentUserProfile()).resolves.toBeUndefined(); + }); + it('should propagate errors', async () => { + mockGetSession.mockResolvedValueOnce(mockAuthSession); mockGetUserProfile.mockRejectedValueOnce(new Error('Service error')); - await expect(getCurrentUserProfile('guid-123')).rejects.toThrow('Service error'); + await expect(getCurrentUserProfile()).rejects.toThrow('Service error'); }); }); diff --git a/src/components/shared-actions/user.ts b/src/components/shared-actions/user.ts index 1e65feda..84f2b844 100644 --- a/src/components/shared-actions/user.ts +++ b/src/components/shared-actions/user.ts @@ -2,14 +2,35 @@ import { MPUserProfile } from "@/lib/providers/ministry-platform/types"; import { UserService } from '@/services/userService'; +import { auth } from "@/lib/auth"; +import { headers } from "next/headers"; /** - * Fetches the current user's profile from Ministry Platform - * @param id - The user's contact ID - * @returns The user's profile data + * Fetches the signed-in user's own profile from Ministry Platform. + * + * Takes no parameters by design. The User_GUID is read from the session rather + * than accepted from the caller, because this action also discloses the user's + * roles and user groups — an arbitrary-GUID parameter would let any caller read + * the authorization model for any user whose GUID they knew, and GUIDs are not + * usefully secret (they appear in the client session and in /contactlookup URLs). + * + * If a feature ever needs to read another user's profile, add a separate, + * explicitly role-gated function rather than widening this one. + * + * @returns The signed-in user's profile data, or undefined if MP has no match */ -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('Authentication required'); + } + + const userGuid = (session.user as { userGuid?: string }).userGuid; + if (!userGuid) { + throw new Error('User GUID not found in session'); + } + const userService = await UserService.getInstance(); - const userProfile = await userService.getUserProfile(id); + const userProfile = await userService.getUserProfile(userGuid); return userProfile; } diff --git a/src/contexts/user-context.test.tsx b/src/contexts/user-context.test.tsx index a3169653..45e12196 100644 --- a/src/contexts/user-context.test.tsx +++ b/src/contexts/user-context.test.tsx @@ -98,7 +98,7 @@ describe('UserContext', () => { await waitFor(() => { expect(screen.getByTestId('name')).toHaveTextContent('John'); }); - expect(mockGetCurrentUserProfile).toHaveBeenCalledWith('guid-123'); + expect(mockGetCurrentUserProfile).toHaveBeenCalledWith(); }); it('should resolve to null profile when no session', async () => { @@ -213,6 +213,6 @@ describe('UserContext', () => { await waitFor(() => { expect(screen.getByTestId('name')).toHaveTextContent('none'); }); - expect(mockGetCurrentUserProfile).toHaveBeenCalledWith('guid-123'); + expect(mockGetCurrentUserProfile).toHaveBeenCalledWith(); }); }); diff --git a/src/contexts/user-context.tsx b/src/contexts/user-context.tsx index b961d2d4..e52dee9d 100644 --- a/src/contexts/user-context.tsx +++ b/src/contexts/user-context.tsx @@ -49,7 +49,9 @@ export function UserProvider({ children }: UserProviderProps) { setUserProfilePromise(RESOLVED_NULL); return; } - setUserProfilePromise(getCurrentUserProfile(userGuid).then((p) => p ?? null)); + // No argument: the action reads the User_GUID from the session server-side. + // userGuid stays a dependency so switching users re-fetches. + setUserProfilePromise(getCurrentUserProfile().then((p) => p ?? null)); }, [userGuid, isPending, refreshKey]); const refreshUserProfile = useCallback(() => {