From 653913afcfe33e9d3731ccc15229930a6b3e38fe Mon Sep 17 00:00:00 2001 From: Chris Kehayias Date: Fri, 21 Aug 2026 07:39:30 -0400 Subject: [PATCH] fix(security): authenticate searchContacts and getCurrentUserProfile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two 'use server' actions were reachable as unauthenticated POST endpoints. Server actions compile to callable endpoints and src/proxy.ts:8 lets all /api paths through without a session, so neither had anything standing between an anonymous caller and Ministry Platform PII. searchContacts — no session check Searched Contacts across name, email, and mobile phone and returned up to 20 records including email address and mobile phone, to any caller. A one-character term was enough. Now calls auth.api.getSession and throws 'Authentication required' first. The check sits before the try/catch on purpose: inside it, the existing catch would have masked the auth failure as 'Failed to search contacts'. Placing it first also stops the empty-search-term early return from being an unauthenticated success path. getCurrentUserProfile — no session check and no ownership check 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. GUIDs are not usefully secret — they are in the client session and in /contactlookup/[guid] URLs. Rather than validate the parameter, the parameter is gone. The GUID now comes from the session, which makes the ownership question unaskable instead of merely answered: there is no argument left to tamper with. Also throws when an authenticated session carries no userGuid. The sole production caller (UserProvider) already passed the session's own GUID, so this is not a capability regression. It keeps userGuid as an effect dependency so switching users still re-fetches. Cross-user profile reads, if ever needed, belong in a separate role-gated function rather than a widening of this one. Tests: 427 passing (up from 421). New cases cover null session, session with no user.id, missing userGuid, an unauthenticated empty search term, and that a caller-supplied GUID cannot displace the session's. Audited the two remaining no-getSession action files and left them as-is: getMpTimezone returns a non-sensitive config string, and handleSignOut must work without a valid session. Closes the two TODOs; TestCoverage.md 5.2/5.3 updated to match. Co-Authored-By: Claude Opus 5 (1M context) --- ...-action-search-contacts-unauthenticated.md | 54 ----------- ...ver-action-user-profile-unauthenticated.md | 56 ----------- .claude/docs/TestCoverage.md | 32 +++++-- src/components/contact-lookup/actions.test.ts | 42 +++++++- src/components/contact-lookup/actions.ts | 14 ++- src/components/shared-actions/user.test.ts | 96 +++++++++++++++---- src/components/shared-actions/user.ts | 31 +++++- src/contexts/user-context.test.tsx | 4 +- src/contexts/user-context.tsx | 4 +- 9 files changed, 185 insertions(+), 148 deletions(-) delete mode 100644 .claude/TODO/server-action-search-contacts-unauthenticated.md delete mode 100644 .claude/TODO/server-action-user-profile-unauthenticated.md 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(() => {