diff --git a/.claude/TODO/2026-10-10-dev-panel-resolvecontactids-caller-identifiers.md b/.claude/TODO/2026-10-10-dev-panel-resolvecontactids-caller-identifiers.md deleted file mode 100644 index a78b57b..0000000 --- a/.claude/TODO/2026-10-10-dev-panel-resolvecontactids-caller-identifiers.md +++ /dev/null @@ -1,36 +0,0 @@ ---- -title: "ToolService.resolveContactIds still takes table/column names from its caller (dev panel)" -severity: low -tags: [security] -area: services -files: [src/services/toolService.ts, src/components/dev-panel/panels/contact-records-actions.ts] -discovered: 2026-09-30 -discovered_by: step2-authorization-and-input-validation (deferred observation, carried over when that TODO was closed) -status: open ---- - -## Problem -Step 2 stopped `FamilyService.resolveContactIdFromPage` from trusting -caller-supplied table and column names. It now takes `{ pageId, recordId }` and -looks up the page's metadata on the server through `ToolService.getPageData`. -`ToolService.resolveContactIds`, which the dev panel's `ContactRecordsPanel` -uses, still accepts table/column identifiers from its caller. They are -identifier-validated (`validateColumnName`), and the path runs only in dev -(`requireDevSession`), so it is not exploitable today. It is still the one -remaining place where a caller picks which table the service account reads. - -## Evidence -- `src/services/toolService.ts` `resolveContactIds`: table/column arguments - come from the dev-panel action. -- `src/components/dev-panel/panels/contact-records-actions.ts`: gated by - `requireDevSession()`. - -## Proposed fix -Give it the same treatment as `resolveContactIdFromPage`. The action passes -`pageId` and the selection; the service looks up `Table_Name`, primary key and -contact column from `getPageData(pageId)` and validates them with -`validateFkPath`. Add a hostile-identifier test. - -## Impact if not fixed -Defence in depth only. A future non-dev caller could copy the pattern and read -arbitrary numeric columns through the service account. diff --git a/.claude/TODO/INDEX.md b/.claude/TODO/INDEX.md index 743bdfc..4ecde9d 100644 --- a/.claude/TODO/INDEX.md +++ b/.claude/TODO/INDEX.md @@ -18,7 +18,7 @@ Severity tiers: - **medium**: doc drift, missing test, refactor with real cost - **low**: nits, minor doc fixes, stylistic improvements -Total: **6 open TODOs**. +Total: **5 open TODOs**. > **`step6`** and **`step8`** are the remaining steps from the 2026-09-30 MPNext > playbook audit (one PR each). `held-*` items wait on an owner decision. @@ -39,23 +39,21 @@ _none open_ | routing | security, drift | Step 6 — Headers, next.config, server-only, prerender + build in CI | [→](2026-09-30-step6-headers-csp-build-guards.md) | | routing | security, doc | Held — CSP notes for customer-deployed clones | [→](2026-09-30-held-csp-customer-deployments.md) | -### Low (4) +### Low (3) | Area | Tags | Title | File | |---|---|---|---| | components | security, bug | Step 8 — Address-label caps, error boundaries, tool-params decode, codegen escaping | [→](2026-09-30-step8-fork-specific-low-risk-items.md) | | components | bug | Add/Edit Family search: "No contacts found" empty state never renders | [→](2026-09-13-search-empty-state-never-renders.md) | | components | refactor, missing-test | Unreachable "empty STEP_FIELDS" branch in `GroupWizard.handleNext` | [→](2026-09-13-dead-empty-fields-branch-handlenext.md) | -| services | security | `ToolService.resolveContactIds` takes table/column names from its caller (dev panel) | [→](2026-10-10-dev-panel-resolvecontactids-caller-identifiers.md) | --- ## By tag -### security (4) +### security (3) - step6-headers-csp-build-guards — medium - held-csp-customer-deployments — medium - step8-fork-specific-low-risk-items — low -- dev-panel-resolvecontactids-caller-identifiers — low ### bug (2) - step8-fork-specific-low-risk-items — low @@ -84,7 +82,7 @@ _none open_ |---|---| | components | 3 | | routing | 2 | -| services | 1 | +| services | 0 | | auth | 0 | | commands | 0 | | mp-provider | 0 | diff --git a/.claude/references/components/dev-panel.md b/.claude/references/components/dev-panel.md index 13279da..7f90812 100644 --- a/.claude/references/components/dev-panel.md +++ b/.claude/references/components/dev-panel.md @@ -50,7 +50,7 @@ Localhost-only developer overlay rendered by `ToolContainer` when `params` is pr | `src/components/dev-panel/panels/selection-actions.ts` | `resolveSelection(selectionId, pageId)` → `{ recordIds, count }` (auth-gated) | | `src/components/dev-panel/panels/selection-actions.test.ts` | Tests for auth/userGuid guards and delegation | | `src/components/dev-panel/panels/contact-records-panel.tsx` | Maps record IDs → Contact IDs via `resolveContactRecords`, driven by `params.recordID` or upstream selection | -| `src/components/dev-panel/panels/contact-records-actions.ts` | `resolveContactRecords(table, pk, contactIdField, recordIds)` → `ContactRecordResult` | +| `src/components/dev-panel/panels/contact-records-actions.ts` | `resolveContactRecords(pageId, recordIds)` → `ContactRecordResult \| null` (page metadata resolved server-side) | | `src/components/dev-panel/panels/user-tools-panel.tsx` | Lists the user's authorized tool paths, computes `isAuthorized` against `NEXT_PUBLIC_PROD_URL + pathname`, reports status up via `onAuthorizationChange` | | `src/components/dev-panel/panels/user-tools-actions.ts` | `getUserTools()` → `string[]` (paths, auth-gated, resolves `userGuid` → `User_ID`) | | `src/components/dev-panel/panels/user-tools-actions.test.ts` | Tests for auth/userGuid guards and delegation | @@ -101,7 +101,7 @@ When missing, the form disables the submit button and surfaces an amber notice n | `api_dev_DeployTool` | `ToolService.deployTool` | `src/services/toolService.ts:268` (via `deploy-tool-actions.ts`) | | `api_MPNextTools_GetPages` | `ToolService.listPages` | `src/services/toolService.ts:207` (via `deploy-tool-actions.ts`) | -`ContactRecordsPanel` does **not** call a stored procedure — it uses `getTableRecords` in `ToolService.resolveContactIds` (`src/services/toolService.ts:335`). +`ContactRecordsPanel` does **not** call a stored procedure — it uses `getTableRecords` in `ToolService.resolveContactIds` (`src/services/toolService.ts`). ### Glossary candidates - **DevPanel** — localhost-only overlay defined at `src/components/dev-panel/dev-panel.tsx:44` @@ -134,11 +134,9 @@ export async function resolveSelection( // src/components/dev-panel/panels/contact-records-actions.ts export async function resolveContactRecords( - tableName: string, - primaryKey: string, - contactIdField: string, + pageId: number, recordIds: number[] -): Promise; +): Promise; // src/components/dev-panel/panels/user-tools-actions.ts export async function getUserTools(): Promise; diff --git a/.claude/references/services/query-patterns.md b/.claude/references/services/query-patterns.md index adfdc4b..1196a26 100644 --- a/.claude/references/services/query-patterns.md +++ b/.claude/references/services/query-patterns.md @@ -141,7 +141,7 @@ export function validateColumnName(value: string): string { /* throws if ≠ /^[ | Where | Call | Purpose | |-------|------|---------| | `userService.ts:72, 84` | `validateGuid(guid)` | Guard `User_GUID = '...'` filter | -| `toolService.ts:307-334` | `validateColumnName`, `validatePositiveInt` | Guard `resolveContactIds` — rejects bad table/PK/contact-id-field/record-ID values | +| `toolService.ts:307-334` | `validateColumnName`, `validateFkPath`, `validatePositiveInt` | Guard `resolveContactIds` — rejects bad page-metadata table/PK/contact-id-field and record-ID values | | `addressLabelService.ts:76` | `batch.forEach(validatePositiveInt)` | Guard batched `Contact_ID IN (...)` | | `groupService.ts:176` | `validatePositiveInt(groupId)` | Guard `Group_ID = ${id}` in `getGroup` | diff --git a/.claude/references/services/tool-service.md b/.claude/references/services/tool-service.md index 205efe0..28f61aa 100644 --- a/.claude/references/services/tool-service.md +++ b/.claude/references/services/tool-service.md @@ -40,7 +40,7 @@ export class ToolService { | `getPageData(pageID: number)` | `PageData \| null` | SP `api_Tools_GetPageData` | | `getSelectionRecordIds(selectionId, userId, pageId)` | `number[]` | SP `api_Common_GetSelection` | | `getUserTools(userId: number)` | `string[]` (tool paths) | SP `api_Tools_GetUserTools` | -| `resolveContactIds(tableName, primaryKey, contactIdField, recordIds)` | `ContactRecordResult` | `getTableRecords` on `{tableName}` in batches of 100 | +| `resolveContactIds(pageId, recordIds)` | `ContactRecordResult \| null` | `getPageData(pageId)`, then `getTableRecords` on the page's table in batches of 100 | | `listPages(search?: string)` | `PageLookup[]` (max 100) | SP `api_MPNextTools_GetPages`, filtered in-memory | | `listRoles(search?: string)` | `RoleLookup[]` (max 100) | `getTableRecords('dp_Roles')` | | `deployTool(input: DeployToolInput)` | `DeployToolResult` | SP `api_dev_DeployTool` (dev credentials) | @@ -101,61 +101,33 @@ return []; ## resolveContactIds -Resolves record IDs (from a selection) to their associated Contact IDs. Works for direct Contact_ID columns AND FK traversal paths like `Participant_ID_Table.Contact_ID`. +Resolves record IDs on an MP page (a single record or a selection) to their associated Contact IDs. Works for direct Contact_ID columns AND FK traversal paths like `Participant_ID_Table.Contact_ID`. + +Takes a **page ID, not table/column names**. The page's `Table_Name`, `Primary_Key` and `Contact_ID_Field` are looked up server-side through `getPageData(pageId)` and validated before interpolation, so no caller chooses what the service account reads. `FamilyService.resolveContactIdFromPage` gets the same treatment from its action. ```typescript -// src/services/toolService.ts:299-350 +// src/services/toolService.ts — resolveContactIds public async resolveContactIds( - tableName: string, primaryKey: string, contactIdField: string, recordIds: number[] -): Promise { - const envelope = { tableName, primaryKey, contactIdField }; - - validateColumnName(primaryKey); - validateColumnName(tableName); - if (contactIdField.includes('.')) { - contactIdField.split('.').forEach(validateColumnName); - } else { - validateColumnName(contactIdField); - } - - if (recordIds.length === 0) return { ...envelope, records: [] }; - - if (contactIdField === primaryKey) { - // e.g., Contacts table — PK IS the contact ID - return { - ...envelope, - records: recordIds.map(id => ({ recordId: id, contactId: id })), - }; - } - - const contactIdResponseKey = contactIdField.includes('.') - ? contactIdField.split('.').pop()! - : contactIdField; - - const allRecords: ContactRecord[] = []; - for (let i = 0; i < recordIds.length; i += ToolService.BATCH_SIZE) { - const batch = recordIds.slice(i, i + ToolService.BATCH_SIZE); - batch.forEach(validatePositiveInt); - const rows = await this.mp!.getTableRecords>({ - table: tableName, - select: `${primaryKey}, ${contactIdField}`, - filter: `${primaryKey} IN (${batch.join(',')})`, - }); - for (const row of rows) { - allRecords.push({ - recordId: row[primaryKey], - contactId: row[contactIdResponseKey], - }); - } - } - - return { ...envelope, records: allRecords }; + pageId: number, recordIds: number[] +): Promise { + validatePositiveInt(pageId, 'pageId'); + await AuthorizationService.getInstance().requireSecurityRole({ table: 'Contacts', operation: 'read' }); + + const page = await this.getPageData(pageId); + if (!page?.Table_Name || !page.Primary_Key || !page.Contact_ID_Field?.trim()) return null; + + const tableName = validateColumnName(page.Table_Name); + const primaryKey = validateColumnName(page.Primary_Key); + const contactIdField = validateFkPath(page.Contact_ID_Field.trim(), 'Contact_ID_Field'); + // ...short-circuit when contactIdField === primaryKey, else + // getTableRecords in batches of MP_FETCH_BATCH_SIZE (100) } ``` +- **Returns `null`** when the page is unknown or lacks `Table_Name` / `Primary_Key` / `Contact_ID_Field`. - **Short-circuits** when the table's PK *is* Contact_ID (e.g., `Contacts` table). - **Batches** at 100 IDs per request. -- **Validates** `tableName`, `primaryKey`, every segment of `contactIdField`, and every record ID. +- **Validates** `pageId`, `Table_Name` and `Primary_Key` (`validateColumnName`), `Contact_ID_Field` (`validateFkPath` — no extra columns, aliases, or non-`_TABLE` segments), and every record ID. **Return type:** ```typescript diff --git a/src/components/dev-panel/panels/contact-records-actions.test.ts b/src/components/dev-panel/panels/contact-records-actions.test.ts index 30d9651..75c2eaa 100644 --- a/src/components/dev-panel/panels/contact-records-actions.test.ts +++ b/src/components/dev-panel/panels/contact-records-actions.test.ts @@ -34,10 +34,10 @@ describe('resolveContactRecords', () => { }; mockResolveContactIds.mockResolvedValueOnce(expected); - const result = await resolveContactRecords('Contacts', 'Contact_ID', 'Contact_ID', [1, 2, 3]); + const result = await resolveContactRecords(7, [1, 2, 3]); expect(mockRequireDevSession).toHaveBeenCalledWith('Dev panel'); - expect(mockResolveContactIds).toHaveBeenCalledWith('Contacts', 'Contact_ID', 'Contact_ID', [1, 2, 3]); + expect(mockResolveContactIds).toHaveBeenCalledWith(7, [1, 2, 3]); expect(result).toEqual(expected); }); @@ -45,7 +45,7 @@ describe('resolveContactRecords', () => { mockRequireDevSession.mockRejectedValueOnce(new Error('Unauthorized - Missing user session data')); await expect( - resolveContactRecords('Contacts', 'Contact_ID', 'Contact_ID', [1]) + resolveContactRecords(7, [1]) ).rejects.toThrow('Unauthorized - Missing user session data'); expect(mockResolveContactIds).not.toHaveBeenCalled(); }); @@ -54,7 +54,7 @@ describe('resolveContactRecords', () => { mockResolveContactIds.mockRejectedValueOnce(new Error('Not authorized')); await expect( - resolveContactRecords('Contacts', 'Contact_ID', 'Contact_ID', [1]) + resolveContactRecords(7, [1]) ).rejects.toThrow('Not authorized'); }); }); diff --git a/src/components/dev-panel/panels/contact-records-actions.ts b/src/components/dev-panel/panels/contact-records-actions.ts index e7a9697..56cd6de 100644 --- a/src/components/dev-panel/panels/contact-records-actions.ts +++ b/src/components/dev-panel/panels/contact-records-actions.ts @@ -4,14 +4,17 @@ import { requireDevSession } from './require-dev-session'; import { ToolService } from '@/services/toolService'; import type { ContactRecordResult } from '@/services/toolService'; +/** + * Takes the page ID, not table/column names: the service resolves the page's + * metadata server-side, so the caller never chooses what the service account + * reads. + */ export async function resolveContactRecords( - tableName: string, - primaryKey: string, - contactIdField: string, + pageId: number, recordIds: number[] -): Promise { +): Promise { await requireDevSession('Dev panel'); const toolService = await ToolService.getInstance(); - return toolService.resolveContactIds(tableName, primaryKey, contactIdField, recordIds); + return toolService.resolveContactIds(pageId, recordIds); } diff --git a/src/components/dev-panel/panels/contact-records-panel.test.tsx b/src/components/dev-panel/panels/contact-records-panel.test.tsx index 43472fd..a3a437c 100644 --- a/src/components/dev-panel/panels/contact-records-panel.test.tsx +++ b/src/components/dev-panel/panels/contact-records-panel.test.tsx @@ -37,13 +37,30 @@ describe("ContactRecordsPanel", () => { }); it("renders nothing when there is neither a single record nor a selection", () => { - const params: ToolParams = { pageData }; + const params: ToolParams = { pageID: 292, pageData }; const { container } = render(); expect(container.firstChild).toBeNull(); }); + it("does not resolve when there is no page ID", () => { + const params: ToolParams = { pageData, recordID: 1 }; + render(); + expect(mockResolveContactRecords).not.toHaveBeenCalled(); + }); + + it("renders nothing when the service returns null (page has no contact metadata)", async () => { + mockResolveContactRecords.mockResolvedValueOnce(null); + + const { container } = render(); + + await waitFor(() => { + expect(screen.queryByText("Development Mode - Loading Contact Records...")).not.toBeInTheDocument(); + }); + expect(container.firstChild).toBeNull(); + }); + it("renders nothing when recordID is 0 (falsy for hasSingleRecord) and no selection", () => { - const params: ToolParams = { pageData, recordID: 0 }; + const params: ToolParams = { pageID: 292, pageData, recordID: 0 }; const { container } = render(); expect(container.firstChild).toBeNull(); }); @@ -56,7 +73,7 @@ describe("ContactRecordsPanel", () => { records: [{ recordId: 1, contactId: 100 }], }); - const params: ToolParams = { pageData, recordID: 1 }; + const params: ToolParams = { pageID: 292, pageData, recordID: 1 }; render(); expect( @@ -68,7 +85,7 @@ describe("ContactRecordsPanel", () => { expect(screen.getByText("Development Mode - Contact Records")).toBeInTheDocument(); }); expect(screen.getByText("1 contact resolved")).toBeInTheDocument(); - expect(mockResolveContactRecords).toHaveBeenCalledWith("Contacts", "Contact_ID", "Contact_ID", [1]); + expect(mockResolveContactRecords).toHaveBeenCalledWith(292, [1]); }); it("resolves a selection of record IDs and pluralizes the count", async () => { @@ -82,7 +99,7 @@ describe("ContactRecordsPanel", () => { ], }); - const params: ToolParams = { pageData }; + const params: ToolParams = { pageID: 292, pageData }; render(); expect(screen.getByText(/Resolving Contact IDs from selection/)).toBeInTheDocument(); @@ -90,13 +107,13 @@ describe("ContactRecordsPanel", () => { await waitFor(() => { expect(screen.getByText("2 contacts resolved")).toBeInTheDocument(); }); - expect(mockResolveContactRecords).toHaveBeenCalledWith("Contacts", "Contact_ID", "Contact_ID", [1, 2]); + expect(mockResolveContactRecords).toHaveBeenCalledWith(292, [1, 2]); }); it("shows an error state when resolution fails", async () => { mockResolveContactRecords.mockRejectedValueOnce(new Error("resolve failed")); - render(); + render(); await waitFor(() => { expect(screen.getByText("Development Mode - Contact Records Error")).toBeInTheDocument(); @@ -107,7 +124,7 @@ describe("ContactRecordsPanel", () => { it("shows a generic error message for a non-Error rejection", async () => { mockResolveContactRecords.mockRejectedValueOnce("nope"); - render(); + render(); await waitFor(() => { expect(screen.getByText("Failed to resolve contact records")).toBeInTheDocument(); @@ -122,7 +139,7 @@ describe("ContactRecordsPanel", () => { records: Array.from({ length: 7 }, (_, i) => ({ recordId: i + 1, contactId: 100 + i })), }); - render(); + render(); await waitFor(() => { expect(screen.getByText("+2 more")).toBeInTheDocument(); @@ -137,7 +154,7 @@ describe("ContactRecordsPanel", () => { records: [{ recordId: 1, contactId: 100 }], }); - render(); + render(); await waitFor(() => { expect(screen.getByText("View Raw JSON")).toBeInTheDocument(); diff --git a/src/components/dev-panel/panels/contact-records-panel.tsx b/src/components/dev-panel/panels/contact-records-panel.tsx index 62ed852..b6960b7 100644 --- a/src/components/dev-panel/panels/contact-records-panel.tsx +++ b/src/components/dev-panel/panels/contact-records-panel.tsx @@ -17,12 +17,11 @@ export function ContactRecordsPanel({ params, selectionRecordIds }: ContactRecor const [error, setError] = useState(null); const contactIdField = params.pageData?.Contact_ID_Field; - const tableName = params.pageData?.Table_Name; - const primaryKey = params.pageData?.Primary_Key; + const pageId = params.pageID; const hasSingleRecord = params.recordID !== undefined && params.recordID > 0; const hasSelection = selectionRecordIds !== undefined && selectionRecordIds.length > 0; - const canResolve = contactIdField && tableName && primaryKey && (hasSingleRecord || hasSelection); + const canResolve = contactIdField && pageId && (hasSingleRecord || hasSelection); useEffect(() => { if (!canResolve) return; @@ -37,7 +36,7 @@ export function ContactRecordsPanel({ params, selectionRecordIds }: ContactRecor async function fetchContacts() { try { - const data = await resolveContactRecords(tableName!, primaryKey!, contactIdField!, recordIds); + const data = await resolveContactRecords(pageId!, recordIds); setResult(data); } catch (err) { setError(err instanceof Error ? err.message : "Failed to resolve contact records"); @@ -47,7 +46,7 @@ export function ContactRecordsPanel({ params, selectionRecordIds }: ContactRecor } fetchContacts(); - }, [canResolve, hasSingleRecord, params.recordID, selectionRecordIds, tableName, primaryKey, contactIdField]); + }, [canResolve, hasSingleRecord, params.recordID, selectionRecordIds, pageId]); if (!contactIdField) return null; if (!hasSingleRecord && !hasSelection) return null; diff --git a/src/services/toolService.test.ts b/src/services/toolService.test.ts index 69a093a..ced62af 100644 --- a/src/services/toolService.test.ts +++ b/src/services/toolService.test.ts @@ -179,12 +179,32 @@ describe('ToolService', () => { }); describe('resolveContactIds', () => { + const page = (overrides: Record = {}) => [[{ + Page_ID: 7, + Display_Name: 'Page', + Singular_Name: 'Page', + Table_Name: 'Group_Participants', + Primary_Key: 'Group_Participant_ID', + Contact_ID_Field: 'Participant_ID_Table.Contact_ID', + ...overrides, + }]]; + + it('looks up the page metadata server-side from the page ID', async () => { + mockExecuteProcedureWithBody.mockResolvedValueOnce(page()); + mockGetTableRecords.mockResolvedValueOnce([]); + + const service = await ToolService.getInstance(); + await service.resolveContactIds(7, [1]); + + expect(mockExecuteProcedureWithBody).toHaveBeenCalledWith('api_Tools_GetPageData', { '@PageID': 7 }); + expect(mockRequireSecurityRole).toHaveBeenCalledWith({ table: 'Contacts', operation: 'read' }); + }); + it('should return empty records when recordIds is empty', async () => { + mockExecuteProcedureWithBody.mockResolvedValueOnce(page()); + const service = await ToolService.getInstance(); - const result = await service.resolveContactIds( - 'Group_Participants', 'Group_Participant_ID', - 'Participant_ID_Table.Contact_ID', [] - ); + const result = await service.resolveContactIds(7, []); expect(mockGetTableRecords).not.toHaveBeenCalled(); expect(result).toEqual({ @@ -195,12 +215,35 @@ describe('ToolService', () => { }); }); - it('should short-circuit when contactIdField equals primaryKey', async () => { + it.each([ + ['the page is unknown', [[]]], + ['the page has no Contact_ID_Field', page({ Contact_ID_Field: undefined })], + ['the Contact_ID_Field is blank', page({ Contact_ID_Field: ' ' })], + ['the page has no Table_Name', page({ Table_Name: '' })], + ['the page has no Primary_Key', page({ Primary_Key: '' })], + ])('returns null when %s', async (_label, pageResult) => { + mockExecuteProcedureWithBody.mockResolvedValueOnce(pageResult); + const service = await ToolService.getInstance(); - const result = await service.resolveContactIds( - 'Contacts', 'Contact_ID', 'Contact_ID', [10, 20, 30] + expect(await service.resolveContactIds(7, [1])).toBeNull(); + expect(mockGetTableRecords).not.toHaveBeenCalled(); + }); + + it.each([0, -1, 1.5, NaN])('rejects invalid page ID %s before any I/O', async (bad) => { + const service = await ToolService.getInstance(); + await expect(service.resolveContactIds(bad, [1])).rejects.toThrow('Expected positive integer for pageId'); + expect(mockRequireSecurityRole).not.toHaveBeenCalled(); + expect(mockExecuteProcedureWithBody).not.toHaveBeenCalled(); + }); + + it('should short-circuit when contactIdField equals primaryKey', async () => { + mockExecuteProcedureWithBody.mockResolvedValueOnce( + page({ Table_Name: 'Contacts', Primary_Key: 'Contact_ID', Contact_ID_Field: 'Contact_ID' }) ); + const service = await ToolService.getInstance(); + const result = await service.resolveContactIds(7, [10, 20, 30]); + expect(mockGetTableRecords).not.toHaveBeenCalled(); expect(result).toEqual({ tableName: 'Contacts', @@ -214,52 +257,77 @@ describe('ToolService', () => { }); }); + it('rejects a non-positive record ID on the short-circuit path', async () => { + mockExecuteProcedureWithBody.mockResolvedValueOnce( + page({ Table_Name: 'Contacts', Primary_Key: 'Contact_ID', Contact_ID_Field: 'Contact_ID' }) + ); + + const service = await ToolService.getInstance(); + await expect(service.resolveContactIds(7, [1, 0])).rejects.toThrow('Expected positive integer'); + }); + it('should resolve contact IDs with a direct column', async () => { + mockExecuteProcedureWithBody.mockResolvedValueOnce( + page({ Table_Name: 'Donations', Primary_Key: 'Donation_ID', Contact_ID_Field: 'Contact_ID' }) + ); mockGetTableRecords.mockResolvedValueOnce([ { Donation_ID: 1, Contact_ID: 100 }, { Donation_ID: 2, Contact_ID: 200 }, ]); const service = await ToolService.getInstance(); - const result = await service.resolveContactIds( - 'Donations', 'Donation_ID', 'Contact_ID', [1, 2] - ); + const result = await service.resolveContactIds(7, [1, 2]); expect(mockGetTableRecords).toHaveBeenCalledWith({ table: 'Donations', select: 'Donation_ID, Contact_ID', filter: 'Donation_ID IN (1,2)', }); - expect(result.records).toEqual([ + expect(result!.records).toEqual([ { recordId: 1, contactId: 100 }, { recordId: 2, contactId: 200 }, ]); }); it('should resolve contact IDs with FK traversal path', async () => { + mockExecuteProcedureWithBody.mockResolvedValueOnce(page()); mockGetTableRecords.mockResolvedValueOnce([ { Group_Participant_ID: 10, Contact_ID: 500 }, { Group_Participant_ID: 11, Contact_ID: 501 }, ]); const service = await ToolService.getInstance(); - const result = await service.resolveContactIds( - 'Group_Participants', 'Group_Participant_ID', - 'Participant_ID_Table.Contact_ID', [10, 11] - ); + const result = await service.resolveContactIds(7, [10, 11]); expect(mockGetTableRecords).toHaveBeenCalledWith({ table: 'Group_Participants', select: 'Group_Participant_ID, Participant_ID_Table.Contact_ID', filter: 'Group_Participant_ID IN (10,11)', }); - expect(result.records).toEqual([ + expect(result!.records).toEqual([ { recordId: 10, contactId: 500 }, { recordId: 11, contactId: 501 }, ]); }); + it.each([ + ['Table_Name', { Table_Name: 'Contacts; DROP TABLE Contacts' }, 'Invalid column name'], + ['Primary_Key', { Primary_Key: 'Contact_ID) OR (1=1' }, 'Invalid column name'], + ['Contact_ID_Field (extra column)', { Contact_ID_Field: 'Contact_ID, Social_Security_Number' }, 'Invalid Contact_ID_Field'], + ['Contact_ID_Field (alias)', { Contact_ID_Field: 'Donor_ID AS Contact_ID' }, 'Invalid Contact_ID_Field'], + ['Contact_ID_Field (non-FK segment)', { Contact_ID_Field: 'dp_Users.Password' }, 'Invalid Contact_ID_Field'], + ])('rejects a hostile %s from page metadata without querying', async (_label, overrides, message) => { + mockExecuteProcedureWithBody.mockResolvedValueOnce(page(overrides)); + + const service = await ToolService.getInstance(); + await expect(service.resolveContactIds(7, [1])).rejects.toThrow(message); + expect(mockGetTableRecords).not.toHaveBeenCalled(); + }); + it('should batch record IDs in groups of 100', async () => { + mockExecuteProcedureWithBody.mockResolvedValueOnce( + page({ Table_Name: 'Events', Primary_Key: 'Event_ID', Contact_ID_Field: 'Contact_ID' }) + ); const ids = Array.from({ length: 150 }, (_, i) => i + 1); mockGetTableRecords.mockResolvedValueOnce( ids.slice(0, 100).map(id => ({ Event_ID: id, Contact_ID: id + 1000 })) @@ -269,23 +337,28 @@ describe('ToolService', () => { ); const service = await ToolService.getInstance(); - const result = await service.resolveContactIds( - 'Events', 'Event_ID', 'Contact_ID', ids - ); + const result = await service.resolveContactIds(7, ids); expect(mockGetTableRecords).toHaveBeenCalledTimes(2); - expect(result.records).toHaveLength(150); - expect(result.records[0]).toEqual({ recordId: 1, contactId: 1001 }); - expect(result.records[149]).toEqual({ recordId: 150, contactId: 1150 }); + expect(result!.records).toHaveLength(150); + expect(result!.records[0]).toEqual({ recordId: 1, contactId: 1001 }); + expect(result!.records[149]).toEqual({ recordId: 150, contactId: 1150 }); }); it('should propagate errors', async () => { + mockExecuteProcedureWithBody.mockResolvedValueOnce(page()); mockGetTableRecords.mockRejectedValueOnce(new Error('Table not found')); const service = await ToolService.getInstance(); - await expect( - service.resolveContactIds('Bad_Table', 'ID', 'Contact_ID', [1]) - ).rejects.toThrow('Table not found'); + await expect(service.resolveContactIds(7, [1])).rejects.toThrow('Table not found'); + }); + + it('propagates an authorization refusal before reading page metadata', async () => { + mockRequireSecurityRole.mockRejectedValueOnce(new Error('Access denied')); + + const service = await ToolService.getInstance(); + await expect(service.resolveContactIds(7, [1])).rejects.toThrow('Access denied'); + expect(mockExecuteProcedureWithBody).not.toHaveBeenCalled(); }); }); diff --git a/src/services/toolService.ts b/src/services/toolService.ts index 55b60e2..09def91 100644 --- a/src/services/toolService.ts +++ b/src/services/toolService.ts @@ -1,7 +1,7 @@ import { MPHelper } from "@/lib/providers/ministry-platform"; import { PageData } from "@/lib/tool-params"; import { AuthorizationService } from "@/services/authorizationService"; -import { validatePositiveInt, validateColumnName, escapeFilterString } from "@/lib/validation"; +import { validatePositiveInt, validateColumnName, validateFkPath, escapeFilterString } from "@/lib/validation"; import { MP_FETCH_BATCH_SIZE } from "@/lib/constants"; export interface ContactRecord { @@ -309,44 +309,50 @@ export class ToolService { } /** - * Resolves record IDs to their associated Contact IDs by querying the - * specified table. Supports direct columns and FK traversal paths + * Resolves record IDs on an MP page to their associated Contact IDs. + * Supports direct columns and FK traversal paths * (e.g. `Participant_ID_Table.Contact_ID`). * + * Takes the page ID, NOT table/column names: the page's `Table_Name`, + * `Primary_Key` and `Contact_ID_Field` are looked up server-side from MP + * (`getPageData`) and validated before they are interpolated, so no caller + * can choose which table or column the service account reads. Same + * treatment as `FamilyService.resolveContactIdFromPage`. + * * Short-circuits when the contact ID field IS the primary key (e.g. Contacts table). * Batches queries in groups of 100 to avoid oversized IN clauses. * - * @param tableName - The Ministry Platform table to query - * @param primaryKey - The primary key column of the table - * @param contactIdField - Column or FK path that resolves to a Contact_ID + * @param pageId - The Ministry Platform Page ID the records belong to * @param recordIds - Array of primary key values to look up - * @returns Promise - Mapped record/contact ID pairs + * @returns Promise - Mapped record/contact ID + * pairs, or null when the page is unknown or has no Contact_ID_Field */ public async resolveContactIds( - tableName: string, - primaryKey: string, - contactIdField: string, + pageId: number, recordIds: number[] - ): Promise { + ): Promise { + validatePositiveInt(pageId, 'pageId'); await AuthorizationService.getInstance().requireSecurityRole({ table: 'Contacts', operation: 'read', }); - const envelope = { tableName, primaryKey, contactIdField }; - validateColumnName(primaryKey); - validateColumnName(tableName); - if (contactIdField.includes('.')) { - contactIdField.split('.').forEach(validateColumnName); - } else { - validateColumnName(contactIdField); + const page = await this.getPageData(pageId); + if (!page?.Table_Name || !page.Primary_Key || !page.Contact_ID_Field?.trim()) { + return null; } + const tableName = validateColumnName(page.Table_Name); + const primaryKey = validateColumnName(page.Primary_Key); + const contactIdField = validateFkPath(page.Contact_ID_Field.trim(), 'Contact_ID_Field'); + const envelope = { tableName, primaryKey, contactIdField }; + if (recordIds.length === 0) { return { ...envelope, records: [] }; } if (contactIdField === primaryKey) { + recordIds.forEach((id) => validatePositiveInt(id)); return { ...envelope, records: recordIds.map(id => ({ recordId: id, contactId: id })),