From ca9c33a63eb1e8d9fdceb70d89a28551893937a7 Mon Sep 17 00:00:00 2001 From: important-new Date: Wed, 22 Jul 2026 09:55:37 +0800 Subject: [PATCH 1/2] fix: harden agent report-access program (post-merge review of #258) Follow-up fixes from a review of the merged people/role-profiles + role-aware sending + agent unified-link program (#258): - SMS consent gate now keys on the per-recipient role, so a recipientKind=all rule can no longer text the client without recorded consent (TCPA) - primary-client add is an atomic insert-if-no-client (closes a TOCTOU race) - tenant /login excludes global-agent rows (no member lockout on a shared email) - resolveRecipients guards listPeople so a transient DB error can't silently drop every report delivery - report-token sign-in is emailed to the agent's own inbox instead of returned to the caller (closes a report-link -> full agent-session takeover) - automation editor warns when recipients = "everyone on the inspection" - analytics agent-name lookup is scoped to rendered rows and chunked under the D1 bind-parameter cap - removePerson is scoped to the URL inspection id - find-my-report redeem prefers a client session when the email holds a client-kind grant (agents still route to the dashboard) - misc: reactivate 409 mapping, cross-tenant template-id rejection, idempotent role-profile seed, fail-closed capability default Adds regression tests for the SMS-all, login-exclusion, and dual-identity paths. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_017YiCgWAmqyzpfs3pp1kjg3 --- .../portal/AgentReportActions.test.tsx | 21 ++++-- app/components/portal/AgentReportActions.tsx | 32 ++++++--- .../settings/AutomationEditorModal.tsx | 5 ++ app/routes/public/portal-inspection.tsx | 6 +- messages/en/reports.json | 9 +-- messages/en/settings-integrations.json | 1 + scripts/file-size-baseline.json | 6 +- scripts/tenant-scoping-baseline.json | 6 +- server/api/agent/magic-login.ts | 38 ++++++++-- server/api/inspections/people.ts | 2 +- server/api/portal.ts | 32 ++++++--- server/lib/people/capabilities.ts | 5 ++ .../validations/agent-magic-login.schema.ts | 2 +- server/services/agent/magic-login.service.ts | 18 +++-- server/services/auth.service.ts | 20 +++++- server/services/automation/sms.ts | 50 +++++-------- server/services/automation/trigger.ts | 19 ++++- .../inspection-analytics.service.ts | 11 ++- server/services/people.service.ts | 70 +++++++++++++++++-- server/services/portal.service.ts | 34 ++++++++- server/services/seed/seed-role-profiles.ts | 6 +- tests/unit/agent/magic-login.spec.ts | 39 +++++++---- tests/unit/auth/auth.service.spec.ts | 38 ++++++++++ .../automation-characterization.spec.ts | 2 +- .../automations/automation-flush-sms.spec.ts | 20 +++++- .../portal/find-my-report-agent-dest.spec.ts | 58 +++++++++++++++ 26 files changed, 438 insertions(+), 112 deletions(-) diff --git a/app/components/portal/AgentReportActions.test.tsx b/app/components/portal/AgentReportActions.test.tsx index 2d6a76ec..b7f51910 100644 --- a/app/components/portal/AgentReportActions.test.tsx +++ b/app/components/portal/AgentReportActions.test.tsx @@ -31,9 +31,9 @@ function renderActions( } describe("AgentReportActions — registered agent (hasAccount: true)", () => { - it('renders the "Go to my workspace" CTA', () => { + it('renders the "Email me a sign-in link" CTA', () => { const { getByTestId } = renderActions(true); - expect(getByTestId("agent-report-workspace-cta").textContent).toBe("Go to my workspace"); + expect(getByTestId("agent-report-workspace-cta").textContent).toBe("Email me a sign-in link"); }); it("submitting posts the agent-magic-login intent with tenant/inspectionId/token", async () => { @@ -41,7 +41,7 @@ describe("AgentReportActions — registered agent (hasAccount: true)", () => { const { getByTestId } = renderActions(true, async ({ request }) => { const formData = await request.formData(); submitted = Object.fromEntries(formData.entries()); - return { ok: true, intent: "agent-magic-login", loginUrl: null }; + return { ok: true, intent: "agent-magic-login", sent: true }; }); fireEvent.click(getByTestId("agent-report-workspace-cta")); @@ -56,6 +56,19 @@ describe("AgentReportActions — registered agent (hasAccount: true)", () => { }); }); + it("shows a check-your-email confirmation on success (never navigates)", async () => { + const { getByTestId, findByTestId } = renderActions(true, async () => ({ + ok: true, + intent: "agent-magic-login", + sent: true, + })); + + fireEvent.click(getByTestId("agent-report-workspace-cta")); + + const confirm = await findByTestId("agent-report-workspace-sent"); + expect(confirm.textContent).toMatch(/check your email/i); + }); + it("shows an error message when the action reports failure", async () => { const { getByTestId, findByText } = renderActions(true, async () => ({ ok: false, @@ -64,7 +77,7 @@ describe("AgentReportActions — registered agent (hasAccount: true)", () => { fireEvent.click(getByTestId("agent-report-workspace-cta")); - await findByText(/couldn't sign you in/i); + await findByText(/couldn't send your sign-in link/i); }); }); diff --git a/app/components/portal/AgentReportActions.tsx b/app/components/portal/AgentReportActions.tsx index 451a278b..b6414d03 100644 --- a/app/components/portal/AgentReportActions.tsx +++ b/app/components/portal/AgentReportActions.tsx @@ -7,9 +7,11 @@ * never sees this component. * * hasAccount branches the CTA: - * - true : "Go to my workspace" — posts the agent-magic-login intent and - * full-navigates to the single-use loginUrl on success (a 302 that mints - * a cookie, so it cannot be a client fetch + client-side redirect). + * - true : "Email me a sign-in link" — posts the agent-magic-login intent; + * the server EMAILS a single-use sign-in link to the agent's account inbox + * (never returns it), so on success we show a "check your email" + * confirmation rather than navigating. Emailing (vs. returning the link) + * closes the report-link → agent-session takeover vector (#258 review #5). * - false : "Create your free agent account" — a plain link into * /agent-signup with the recipient email + returnTo prefilled. * @@ -29,7 +31,8 @@ import { m } from "~/paraglide/messages"; interface AgentMagicLoginActionResult { ok: boolean; intent?: string; - loginUrl?: string | null; + /** True when the server accepted the request and (if an account exists) emailed the sign-in link. */ + sent?: boolean; } export interface AgentReportActionsProps { @@ -53,18 +56,19 @@ export function AgentReportActions({ }: AgentReportActionsProps) { const fetcher = useFetcher(); const [error, setError] = useState(false); + const [sent, setSent] = useState(false); const lastHandled = useRef(undefined); // Consume the action result exactly once per response (mirrors // WordExportButton's guard — fetcher.data keeps the same reference across - // renders, so without it this effect would re-fire and re-navigate). + // renders). The server emails the sign-in link and answers { sent: true } + // (anti-oracle — identical whether or not an account exists), so on success + // we show a "check your email" confirmation instead of navigating anywhere. useEffect(() => { if (fetcher.state !== "idle" || !fetcher.data || fetcher.data === lastHandled.current) return; lastHandled.current = fetcher.data; - if (fetcher.data.ok && fetcher.data.loginUrl) { - // Full navigation (not client-side) — the redeem endpoint is a - // cookie-setting 302, which a client-side router transition can't relay. - window.location.assign(fetcher.data.loginUrl); + if (fetcher.data.ok) { + setSent(true); } else { setError(true); } @@ -94,6 +98,16 @@ export function AgentReportActions({ const submitting = fetcher.state !== "idle"; + if (sent) { + return ( +
+ + {m.agent_report_actions_workspace_sent()} + +
+ ); + } + return (
+ {recipientKind === "all" && ( +

+ {m.settings_automations_all_recipients_warning()} +

+ )} {noChannel && (

{m.settings_automations_pick_channel()}

)} diff --git a/app/routes/public/portal-inspection.tsx b/app/routes/public/portal-inspection.tsx index 80d90d7e..883ce30a 100644 --- a/app/routes/public/portal-inspection.tsx +++ b/app/routes/public/portal-inspection.tsx @@ -212,7 +212,7 @@ export async function loader({ params, request, context }: Route.LoaderArgs) { /* ------------------------------------------------------------------ */ type AgentMagicLoginActionResult = - | { ok: true; intent: "agent-magic-login"; loginUrl: string | null } + | { ok: true; intent: "agent-magic-login"; sent: boolean } | { ok: false; intent: "agent-magic-login"; error?: string } | { ok: false; intent: string }; @@ -232,11 +232,11 @@ export async function action({ request, params, context }: Route.ActionArgs) { if (!res.ok) { return { ok: false, intent: "agent-magic-login" } satisfies AgentMagicLoginActionResult; } - const body = (await res.json()) as { data?: { loginUrl: string | null } }; + const body = (await res.json()) as { data?: { sent?: boolean } }; return { ok: true, intent: "agent-magic-login", - loginUrl: body.data?.loginUrl ?? null, + sent: body.data?.sent ?? true, } satisfies AgentMagicLoginActionResult; } catch { return { ok: false, intent: "agent-magic-login" } satisfies AgentMagicLoginActionResult; diff --git a/messages/en/reports.json b/messages/en/reports.json index f0a5cd2f..d3c3b535 100644 --- a/messages/en/reports.json +++ b/messages/en/reports.json @@ -202,10 +202,11 @@ "report_view_cert": "Certified Inspection Report", "report_view_repair_list_link": "View Repair List", "report_view_build_repair": "Build repair request", - "agent_report_actions_workspace_hint": "You already have an agent account for this email — sign in to track referrals and manage your profile.", - "agent_report_actions_workspace_cta": "Go to my workspace", - "agent_report_actions_workspace_pending": "Signing in…", - "agent_report_actions_workspace_error": "We couldn't sign you in right now. Please try again in a moment.", + "agent_report_actions_workspace_hint": "You already have an agent account for this email — we'll email you a secure link to sign in to your workspace.", + "agent_report_actions_workspace_cta": "Email me a sign-in link", + "agent_report_actions_workspace_pending": "Sending…", + "agent_report_actions_workspace_sent": "Check your email — we've sent a single-use sign-in link to your inbox. It expires in 15 minutes.", + "agent_report_actions_workspace_error": "We couldn't send your sign-in link right now. Please try again in a moment.", "agent_report_actions_signup_hint": "Create a free agent account to track referrals across every inspection you send.", "agent_report_actions_signup_cta": "Create your free agent account", "report_view_print": "Print", diff --git a/messages/en/settings-integrations.json b/messages/en/settings-integrations.json index a9095885..b855b874 100644 --- a/messages/en/settings-integrations.json +++ b/messages/en/settings-integrations.json @@ -257,6 +257,7 @@ "settings_automations_dothis_legend": "Do this", "settings_automations_delay_title": "Delay in minutes (for reminders: minutes BEFORE the inspection)", "settings_automations_pick_channel": "Pick at least one delivery channel.", + "settings_automations_all_recipients_warning": "\"Everyone on the inspection\" sends to every report-receiving person added to each inspection — including any listing (seller's) agent. Use a specific role if the buyer's report should not reach the seller's side.", "settings_automations_template_label": "Template", "settings_automations_select_template": "— Select a template —", "settings_automations_edit_new_template": "Edit / New template", diff --git a/scripts/file-size-baseline.json b/scripts/file-size-baseline.json index 4c5070dc..48a5977e 100644 --- a/scripts/file-size-baseline.json +++ b/scripts/file-size-baseline.json @@ -18,7 +18,7 @@ "server/api/inspections/report-delivery.ts": 667, "app/hooks/usePhotoOps.ts": 661, "server/lib/messaging/providers/telnyx-compliance.ts": 657, - "server/services/inspection/inspection-analytics.service.ts": 651, + "server/services/inspection/inspection-analytics.service.ts": 656, "app/components/editor/ItemEditor.tsx": 633, "server/api/admin/admin-settings.ts": 633, "app/components/inspection-edit/CompliancePanel.tsx": 623, @@ -28,10 +28,10 @@ "server/services/concierge.service.ts": 575, "server/api/auth.ts": 564, "app/lib/collab/results-binding.ts": 560, + "server/api/portal.ts": 551, "server/api/calendar.ts": 547, "app/components/NewInspectionWizard.tsx": 545, "server/api/inspections/core.ts": 541, - "server/api/portal.ts": 539, "server/services/inspection/inspection-publish.service.ts": 536, "server/services/agent/referral.ts": 533, "server/api/inspections/publish.ts": 532, @@ -59,8 +59,8 @@ "app/components/media-studio/VideoCapture.tsx": 433, "server/api/inspections/results.ts": 430, "server/api/inspections/hierarchy.ts": 428, - "server/lib/middleware/di.ts": 425, "app/hooks/useStructureEdit.ts": 424, + "server/lib/middleware/di.ts": 421, "app/routes/templates.tsx": 411, "app/routes/calendar.tsx": 402, "server/api/secrets.ts": 401 diff --git a/scripts/tenant-scoping-baseline.json b/scripts/tenant-scoping-baseline.json index f07ccf01..3f3ceed4 100644 --- a/scripts/tenant-scoping-baseline.json +++ b/scripts/tenant-scoping-baseline.json @@ -34,9 +34,9 @@ "server/services/agreement/signer-state.ts:53", "server/services/agreement/signer-state.ts:68", "server/services/ai.service.ts:91", - "server/services/auth.service.ts:133", - "server/services/auth.service.ts:189", - "server/services/auth.service.ts:218", + "server/services/auth.service.ts:149", + "server/services/auth.service.ts:205", + "server/services/auth.service.ts:234", "server/services/automation/core.ts:161", "server/services/automation/core.ts:189", "server/services/automation/delivery.ts:181", diff --git a/server/api/agent/magic-login.ts b/server/api/agent/magic-login.ts index 9eadf4be..ed3327c8 100644 --- a/server/api/agent/magic-login.ts +++ b/server/api/agent/magic-login.ts @@ -30,15 +30,15 @@ const requestRoute = createRoute(withMcpMetadata({ method: 'post', path: '/magic-login/request', tags: ['agents', 'public'], - summary: 'Exchange an agent report token for a login code', - description: 'Public, unauthenticated endpoint for the agent unified link. Exchanges a live, agent-kind report-link token for a single-use magic-login code (900 second TTL) that mints an agent session cookie on redeem. Returns loginUrl: null with 200 (not an error) when the token is valid but no global agent account exists yet for the recipient email, so probing tokens cannot be used to enumerate accounts.', + summary: 'Email an agent a single-use sign-in link for a report token', + description: 'Public, unauthenticated endpoint for the agent unified link. For a live, agent-kind report-link token whose recipient email has a global agent account, EMAILS a single-use magic-login link (900 second TTL) to that agent\'s account inbox — the link is never returned to the caller, so a leaked/forwarded report link cannot be replayed into a full agent session. Always returns { sent: true } with 200 (identical response and timing whether or not an account exists) so probing tokens cannot enumerate accounts.', request: { body: { content: { 'application/json': { schema: MagicLoginRequestSchema } } }, }, responses: { 200: { content: { 'application/json': { schema: MagicLoginRequestResponseSchema } }, - description: 'Login code issued, or loginUrl: null when no agent account exists for the recipient', + description: 'Always { sent: true } — a sign-in link is emailed to the agent when an account exists; nothing is sent otherwise. The link is never returned here.', }, 401: { description: 'Invalid, revoked, expired, inspection-mismatched, or non-agent report token' }, }, @@ -66,7 +66,7 @@ const redeemRoute = createRoute(withMcpMetadata({ export const agentMagicLoginRequestRoutes = createApiRouter() .openapi(requestRoute, async (c) => { const body = c.req.valid('json'); - const loginUrl = await requestMagicLogin({ + const result = await requestMagicLogin({ portalAccess: c.var.services.portalAccess, kv: c.env.TENANT_CACHE, db: c.env.DB, @@ -74,7 +74,35 @@ export const agentMagicLoginRequestRoutes = createApiRouter() reportToken: body.token, coreBaseUrl: getBaseUrl(c), }); - return c.json({ success: true as const, data: { loginUrl } }, 200); + // EMAIL the single-use sign-in link to the agent's own account inbox — + // never return it to the caller. The durable report link is a reusable + // bearer, so returning a full-session login URL to whoever presents it + // was an account-takeover vector (#258 review #5). Only the agent's inbox + // can now complete sign-in; report VIEWING via the token is unchanged. + if (result) { + // Defer the send to waitUntil — awaiting only on the account-exists + // path would make it measurably slower than the no-account path, a + // timing enumeration oracle. Mirrors server/api/agent/login.ts's + // loginLink and portal.ts's requestLink. + const sendPromise = (async () => { + try { + await c.var.services.email.sendAgentLoginLink(result.email, result.loginUrl); + } catch (err) { + logger.error('agent.magic_login.link_send_failed', {}, err instanceof Error ? err : undefined); + } + })(); + let execCtx: Pick | undefined; + try { + execCtx = c.executionCtx; + } catch { + execCtx = undefined; + } + if (execCtx) execCtx.waitUntil(sendPromise); + else await sendPromise; + } + // Anti-oracle: identical { sent: true } response (and timing) whether or + // not an agent account exists for the report link's recipient. + return c.json({ success: true as const, data: { sent: true as const } }, 200); }); /** GET /agent/magic-login — mounted at the TOP LEVEL (not under /api); see workers/app.ts. */ diff --git a/server/api/inspections/people.ts b/server/api/inspections/people.ts index 2afb9c89..862ce362 100644 --- a/server/api/inspections/people.ts +++ b/server/api/inspections/people.ts @@ -147,7 +147,7 @@ const peopleRoutes = createApiRouter() const tenantId = getTenantId(c); const { id, personId } = c.req.valid('param'); await assertInspectionOwned(getDrizzle(c), id, tenantId); - await c.var.services.people.removePerson(tenantId, personId); + await c.var.services.people.removePerson(tenantId, id, personId); return c.json({ success: true as const }, 200); }); diff --git a/server/api/portal.ts b/server/api/portal.ts index 096aa06f..26faa06e 100644 --- a/server/api/portal.ts +++ b/server/api/portal.ts @@ -390,16 +390,28 @@ const portalRoutes = portalRouter return c.json({ error: 'Invalid or expired link' }, 401); } - // SECURITY: a find-my-report magic link redeemed by an agent must mint - // an AGENT JWT (mirrors server/api/agent/login.ts's password mint - // EXACTLY — no tenantId) and NEVER the client __Host-portal_session - // cookie. Global agent accounts (findGlobalAgentByEmail — the single - // source of the "live global agent" predicate) are a SEPARATE identity - // plane from tenant-scoped client/co_client contacts; there is no - // token/grant object on this magic-link path (unlike exchangeRoute), - // only a verified email, so the global-agent-account lookup is the - // correct signal to branch on here. - const agent = await findGlobalAgentByEmail(c.env.DB, verified.email); + // Prefer a client session when this email holds a live CLIENT-KIND grant + // in this tenant, even if it ALSO has a global agent account — else a + // dual-identity recipient could never reach their client report (#258 + // review #9). hasLiveClientGrant is kind='client' specific, NOT + // listRecipientInspections (capability-based → also matches agent grants + // via the Spec 3 self-retrieve flip). Agent-role report tokens still can't + // unlock the client hub — that guard lives in exchangeRoute (role-based). + const tenantId = resolveTenantId(c); + let hasClientAccess = false; + if (tenantId) { + try { + hasClientAccess = await c.var.services.portal.hasLiveClientGrant(tenantId, verified.email); + } catch (err) { + logger.error('[portal] redeem client-access lookup failed', {}, err instanceof Error ? err : undefined); + } + } + + // SECURITY: an agent-only email (no client access) mints an AGENT JWT + // (mirrors server/api/agent/login.ts — no tenantId) and NEVER the client + // __Host-portal_session cookie. Global agents (findGlobalAgentByEmail) are + // a SEPARATE identity plane from tenant-scoped client/co_client contacts. + const agent = hasClientAccess ? null : await findGlobalAgentByEmail(c.env.DB, verified.email); if (agent) { const keyring = await c.var.keyringPromise!; const now = Math.floor(Date.now() / 1000); diff --git a/server/lib/people/capabilities.ts b/server/lib/people/capabilities.ts index a9b73a09..07b8b389 100644 --- a/server/lib/people/capabilities.ts +++ b/server/lib/people/capabilities.ts @@ -17,5 +17,10 @@ export function capabilitiesForKind(kind: RoleKind): RoleCapabilities { case 'client': return { receivesReport: true, selfRetrieveReport: true, canSign: true, canPay: true, canHaveAccount: false }; case 'agent': return { receivesReport: true, selfRetrieveReport: true, canSign: true, canPay: true, canHaveAccount: true }; case 'other': return { receivesReport: true, selfRetrieveReport: false, canSign: false, canPay: false, canHaveAccount: false }; + // Fail closed: callers cast DB `kind` strings through `as RoleKind`, so a + // corrupt/out-of-enum value would otherwise return undefined and throw on + // `[cap]` access. An unknown role gets NO capabilities (no report, no + // sign/pay/account), never an accidental grant. + default: return { receivesReport: false, selfRetrieveReport: false, canSign: false, canPay: false, canHaveAccount: false }; } } diff --git a/server/lib/validations/agent-magic-login.schema.ts b/server/lib/validations/agent-magic-login.schema.ts index a209a77e..306ebadd 100644 --- a/server/lib/validations/agent-magic-login.schema.ts +++ b/server/lib/validations/agent-magic-login.schema.ts @@ -17,6 +17,6 @@ export const MagicLoginRequestSchema = z.object({ export const MagicLoginRequestResponseSchema = createApiResponseSchema( z.object({ - loginUrl: z.string().nullable().describe('One-time magic-login URL (900s TTL), or null when no agent account exists for the report link recipient (anti-oracle — not an error).'), + sent: z.boolean().describe('Always true — a single-use sign-in link is EMAILED to the agent\'s account inbox (never returned here). Identical response and timing whether or not an agent account exists for the report link recipient (anti-oracle — the link is never returned to the caller, closing the report-link → session takeover vector).'), }), ).openapi('AgentMagicLoginRequestResponse'); diff --git a/server/services/agent/magic-login.service.ts b/server/services/agent/magic-login.service.ts index 91c81e5d..dae7154e 100644 --- a/server/services/agent/magic-login.service.ts +++ b/server/services/agent/magic-login.service.ts @@ -46,12 +46,20 @@ export interface RequestMagicLoginParams { * invalid/revoked/expired/inspection-mismatched, or when it resolves to a * non-agent-kind role (e.g. a client link) — those are hard auth failures. * + * Returns the login URL PLUS the agent's account email so the ROUTE can EMAIL + * the single-use link to that inbox — the link is NEVER returned to the HTTP + * caller. A durable agent report link is a reusable bearer (often no expiry) + * sent over email; handing a full-session login URL back to anyone who POSTs it + * turned a leaked/forwarded report link into full agent-account takeover (#258 + * review #5). Emailing to the account owner means only the agent's inbox can + * complete sign-in, while report VIEWING via the durable token is unchanged. + * * Returns `null` (NOT an error) when the token is valid and agent-kind but no - * global agent account exists yet for the recipient email — this is the - * anti-oracle case: the route returns the SAME `{ loginUrl: null }` 200 shape - * so a caller can't probe report tokens to learn which emails have accounts. + * global agent account exists yet for the recipient email — the route returns + * the SAME `{ sent: true }` 200 shape (and defers the send to waitUntil) so a + * caller can't probe report tokens to learn which emails have accounts. */ -export async function requestMagicLogin(params: RequestMagicLoginParams): Promise { +export async function requestMagicLogin(params: RequestMagicLoginParams): Promise<{ loginUrl: string; email: string } | null> { const { portalAccess, kv, db, inspectionId, reportToken, coreBaseUrl } = params; const grant = await resolvePortalAccess(portalAccess, reportToken, inspectionId); @@ -86,7 +94,7 @@ export async function requestMagicLogin(params: RequestMagicLoginParams): Promis if (!account) return null; - return mintLoginCode(kv, account.id, coreBaseUrl); + return { loginUrl: await mintLoginCode(kv, account.id, coreBaseUrl), email: account.email }; } export interface RequestMagicLoginByEmailParams { diff --git a/server/services/auth.service.ts b/server/services/auth.service.ts index f4f14420..d63cc97f 100644 --- a/server/services/auth.service.ts +++ b/server/services/auth.service.ts @@ -1,5 +1,5 @@ import { drizzle } from 'drizzle-orm/d1'; -import { eq, and, sql, isNull } from 'drizzle-orm'; +import { eq, ne, and, or, sql, isNull, isNotNull } from 'drizzle-orm'; import { users, tenantInvites, tenants } from '../lib/db/schema'; import { Errors } from '../lib/errors'; import { hashPassword, verifyPassword } from '../lib/password'; @@ -61,7 +61,23 @@ export class AuthService { // Soft-deleted (removed member / self-deleted account) rows are // excluded — a matching row that isn't NULL-deleted-at must never // authenticate, even if the caller somehow still knows the password. - const user = await db.select().from(users).where(and(eq(users.email, email), isNull(users.deletedAt))).get(); + // + // Global agents (role='agent', tenant_id IS NULL) are ALSO excluded: they + // authenticate exclusively through /agent-login (findGlobalAgentByEmail), + // never this tenant front door. Without this guard, `users.email` being + // unique only per (tenant_id, email) means an email held by BOTH a global + // agent and an invited tenant member returns a nondeterministic `.get()` + // row — the earlier-inserted agent row can shadow the member and lock the + // member out (and /agent-signup is self-serve, so the collision is + // attacker-seedable). See #258 review. + // De Morgan of NOT(role='agent' AND tenant_id IS NULL): keep every row + // that is either not an agent OR is tenant-scoped, i.e. exclude only the + // global-agent rows. + const user = await db.select().from(users).where(and( + eq(users.email, email), + isNull(users.deletedAt), + or(ne(users.role, 'agent'), isNotNull(users.tenantId)), + )).get(); if (!user) { // Perform a throwaway verification against a fixed hash so the response time diff --git a/server/services/automation/sms.ts b/server/services/automation/sms.ts index 385af64f..162f127e 100644 --- a/server/services/automation/sms.ts +++ b/server/services/automation/sms.ts @@ -1,6 +1,7 @@ import type { DrizzleD1Database } from 'drizzle-orm/d1'; import { eq, and } from 'drizzle-orm'; -import { automationLogs, automations, tenants, tenantConfigs, contactRoleProfiles } from '../../lib/db/schema'; +import type { automations, tenants} from '../../lib/db/schema'; +import { automationLogs, tenantConfigs } from '../../lib/db/schema'; import { PRIMARY_CLIENT_KEY } from '../../lib/people/default-role-profiles'; import { logger } from '../../lib/logger'; import { currentPeriodKey } from '../../lib/usage/period'; @@ -78,37 +79,22 @@ export function AutomationSms>(Base: T : null; if (!tpl || tpl.channel !== 'sms' || !tpl.body.trim()) return void (await skip('no sms template')); - // Consent gate — client only (agents/inspector implied; D5). Gate - // fires when the rule's recipient discriminator resolves to the - // PRIMARY_CLIENT_KEY role profile (replaces the old `recipient === - // 'client'` enum check; same consent-required-only-for-client behavior). - if (automation.recipientKind === 'role' && automation.recipientRoleProfileId) { - // try/catch (not `.get().catch()`) so this works under both the async - // D1 driver and the synchronous better-sqlite3 test driver. - let roleRow: { key: string } | null = null; - try { - roleRow = await db.select({ key: contactRoleProfiles.key }).from(contactRoleProfiles) - .where(and( - eq(contactRoleProfiles.tenantId, inspection.tenantId), - eq(contactRoleProfiles.id, automation.recipientRoleProfileId), - )).get() ?? null; - } catch (err) { - // DB error on consent-gate role lookup: fail closed (do not send). - // When we cannot prove the recipient is NOT a consent-requiring client, we must not send. - logger.error('sms consent-gate role lookup failed; failing closed (skipping send)', { - automationId: automation.id, - inspectionId: inspection.id, - tenantId: inspection.tenantId, - }, err instanceof Error ? err : undefined); - return void (await skip('consent-gate role lookup failed')); - } - if (roleRow?.key === PRIMARY_CLIENT_KEY) { - const { SmsConsentService } = await import('../sms-consent.service'); - const consentSvc = new SmsConsentService(this.db); - const contactId = inspection.clientContactId; - const latest = contactId ? await consentSvc.getLatest(inspection.tenantId, contactId) : null; - if (latest !== 'granted') return void (await skip('no sms consent')); - } + // Consent gate — client only (agents/inspector implied; D5). Keyed on + // the PER-RECIPIENT role stamped on the log (log.recipientRoleKey), + // NOT the rule's recipientKind. This fires for the primary-client + // recipient whether the rule targets the client directly + // (recipientKind='role') OR fans out to everyone (recipientKind='all'): + // the old gate keyed on `recipientKind === 'role'` and so never + // covered 'all', letting an 'all' rule text the client with no + // recorded TCPA consent. resolveRecipients stamps recipientRoleKey + // from the contact_role_profiles.key of each resolved person, so + // `=== PRIMARY_CLIENT_KEY` is an exact, lookup-free client match. + if (log.recipientRoleKey === PRIMARY_CLIENT_KEY) { + const { SmsConsentService } = await import('../sms-consent.service'); + const consentSvc = new SmsConsentService(this.db); + const contactId = inspection.clientContactId; + const latest = contactId ? await consentSvc.getLatest(inspection.tenantId, contactId) : null; + if (latest !== 'granted') return void (await skip('no sms consent')); } const resolved = await sms.resolveProvider(inspection.tenantId); diff --git a/server/services/automation/trigger.ts b/server/services/automation/trigger.ts index 1a77e090..5893bc59 100644 --- a/server/services/automation/trigger.ts +++ b/server/services/automation/trigger.ts @@ -4,7 +4,7 @@ import { automations, automationLogs, inspections } from '../../lib/db/schema'; import { nanoid } from 'nanoid'; import { logger } from '../../lib/logger'; import { createOiTemplateStore } from './template-store'; -import { type Constructor, type TriggerContext } from './shared'; +import type { Constructor, TriggerContext } from './shared'; import type { AutomationBase, HasEnsureSeeds, HasParseChannels } from './shared'; import { PRIMARY_CLIENT_KEY } from '../../lib/people/default-role-profiles'; import { PeopleService } from '../people.service'; @@ -303,7 +303,22 @@ export function AutomationTrigger>; + try { + people = await new PeopleService({ DB: this.db }).listPeople(inspection.tenantId, inspection.id); + } catch (err) { + logger.error('resolveRecipients: listPeople failed; skipping this rule\'s recipients', { + inspectionId: inspection.id, tenantId: inspection.tenantId, channel, + }, err instanceof Error ? err : undefined); + return []; + } const targets = rule.recipientKind === 'role' ? people.filter(p => p.roleProfileId === rule.recipientRoleProfileId) : people.filter(p => capabilitiesForKind(p.kind).receivesReport); diff --git a/server/services/inspection/inspection-analytics.service.ts b/server/services/inspection/inspection-analytics.service.ts index d232c4cc..1eb5c9f6 100644 --- a/server/services/inspection/inspection-analytics.service.ts +++ b/server/services/inspection/inspection-analytics.service.ts @@ -504,10 +504,15 @@ export class InspectionAnalyticsService extends InspectionSubService { // roles for every inspection in `all` — listing_agent = the selling // agent, buyer_agent = the referred/buyer's agent — mirroring the // priority the legacy code gave sellingAgentId over referredByAgentId. - const allIds = all.map(i => i.id as string); + // Scope to `uniqueIds` (rendered bucket ids), NOT every inspection in + // `all` — only bucket rows are decorate()'d — and chunk under D1's ~100 + // bound-param cap. The pre-fix code bound the full per-tenant inspection + // set and threw past ~95 inspections, breaking the dashboard (#258 review). const listingAgentNameByInspection = new Map(); const buyerAgentNameByInspection = new Map(); - if (allIds.length > 0) { + const AGENT_ROLE_ID_CHUNK = 90; // headroom for the fixed binds (tenantId ×2, role keys ×2) + for (let off = 0; off < uniqueIds.length; off += AGENT_ROLE_ID_CHUNK) { + const chunk = uniqueIds.slice(off, off + AGENT_ROLE_ID_CHUNK); const agentRoleRows = await db.select({ inspectionId: inspectionPeople.inspectionId, roleKey: contactRoleProfiles.key, @@ -526,7 +531,7 @@ export class InspectionAnalyticsService extends InspectionSubService { )) .where(and( eq(inspectionPeople.tenantId, tenantId), - inArray(inspectionPeople.inspectionId, allIds), + inArray(inspectionPeople.inspectionId, chunk), )); for (const r of agentRoleRows) { if (!r.contactName) continue; diff --git a/server/services/people.service.ts b/server/services/people.service.ts index 9def5d23..fe4283c6 100644 --- a/server/services/people.service.ts +++ b/server/services/people.service.ts @@ -1,6 +1,6 @@ import { drizzle } from 'drizzle-orm/d1'; -import { and, eq } from 'drizzle-orm'; -import { contacts, contactRoleProfiles, inspectionPeople } from '../lib/db/schema'; +import { and, eq, sql } from 'drizzle-orm'; +import { contacts, contactRoleProfiles, inspectionPeople, messageTemplates } from '../lib/db/schema'; import { capabilitiesForKind, type RoleCapabilities, type RoleKind } from '../lib/people/capabilities'; import { PRIMARY_CLIENT_KEY } from '../lib/people/default-role-profiles'; import { Errors } from '../lib/errors'; @@ -25,23 +25,53 @@ export class PeopleService { async addPerson(tenantId: string, inspectionId: string, contactId: string, roleProfileId: string): Promise { const prof = await this.profile(tenantId, roleProfileId); if (prof.key === PRIMARY_CLIENT_KEY) { - const existing = await this.db.select({ id: inspectionPeople.id }).from(inspectionPeople) + // Atomic insert-if-no-existing-client. A bare SELECT-then-INSERT leaves + // a TOCTOU race open: two concurrent adds of DIFFERENT client contacts + // both pass the existence check and both land, giving the inspection + // two primary clients (getPrimaryClient then returns a nondeterministic + // one). D1 serializes writes, so an INSERT ... SELECT ... WHERE NOT + // EXISTS evaluates the guard against committed rows and only one wins. + // ON CONFLICT keeps the same-contact idempotent dedup. See #258 review. + await this.db.run(sql` + INSERT INTO inspection_people (id, tenant_id, inspection_id, contact_id, role_profile_id, created_at) + SELECT ${crypto.randomUUID()}, ${tenantId}, ${inspectionId}, ${contactId}, ${roleProfileId}, ${Date.now()} + WHERE NOT EXISTS ( + SELECT 1 FROM inspection_people ip + JOIN contact_role_profiles crp ON ip.role_profile_id = crp.id + WHERE ip.tenant_id = ${tenantId} AND ip.inspection_id = ${inspectionId} AND crp.key = ${PRIMARY_CLIENT_KEY} + ) + ON CONFLICT (inspection_id, contact_id, role_profile_id) DO NOTHING + `); + // Verify our contact now holds the client role; if a different contact + // won the race (or was already the client), surface the friendly 409. + const winner = await this.db.select({ contactId: inspectionPeople.contactId }).from(inspectionPeople) .innerJoin(contactRoleProfiles, eq(inspectionPeople.roleProfileId, contactRoleProfiles.id)) .where(and( eq(inspectionPeople.tenantId, tenantId), eq(inspectionPeople.inspectionId, inspectionId), eq(contactRoleProfiles.key, PRIMARY_CLIENT_KEY), )).get(); - if (existing) throw Errors.Conflict('An inspection already has a primary client; use co_client for a second buyer.'); + if (!winner || winner.contactId !== contactId) { + throw Errors.Conflict('An inspection already has a primary client; use co_client for a second buyer.'); + } + return; } await this.db.insert(inspectionPeople).values({ id: crypto.randomUUID(), tenantId, inspectionId, contactId, roleProfileId, createdAt: new Date(), }).onConflictDoNothing(); } - async removePerson(tenantId: string, inspectionPersonId: string): Promise { + async removePerson(tenantId: string, inspectionId: string, inspectionPersonId: string): Promise { + // Scope the delete to the URL's inspection as well as the tenant — the + // personId path segment is asserted to belong to `inspectionId`, so a + // person row from a DIFFERENT inspection (same tenant) must not be + // deletable via /inspections/:id/people/:personId. See #258 review. await this.db.delete(inspectionPeople) - .where(and(eq(inspectionPeople.tenantId, tenantId), eq(inspectionPeople.id, inspectionPersonId))); + .where(and( + eq(inspectionPeople.tenantId, tenantId), + eq(inspectionPeople.inspectionId, inspectionId), + eq(inspectionPeople.id, inspectionPersonId), + )); } async listPeople(tenantId: string, inspectionId: string): Promise { @@ -148,8 +178,21 @@ export class PeopleService { .orderBy(contactRoleProfiles.sortOrder); } + /** Rejects any template id that is not an active row in THIS tenant's + * message_templates — a role profile must never reference another tenant's + * (or a bogus) template id. Null/undefined ids are allowed (no reference). */ + private async assertTemplatesOwned(tenantId: string, emailTemplateId?: string | null, smsTemplateId?: string | null) { + for (const id of [emailTemplateId, smsTemplateId]) { + if (!id) continue; + const row = await this.db.select({ id: messageTemplates.id }).from(messageTemplates) + .where(and(eq(messageTemplates.tenantId, tenantId), eq(messageTemplates.id, id))).get(); + if (!row) throw Errors.NotFound('Message template not found'); + } + } + /** Creates a tenant-defined (non-system) role profile with a unique, slugified key. */ async createProfile(tenantId: string, input: { label: string; kind: RoleKind; emailTemplateId?: string; smsTemplateId?: string }) { + await this.assertTemplatesOwned(tenantId, input.emailTemplateId, input.smsTemplateId); const key = await this.uniqueKey(tenantId, input.label); const now = new Date(); const row = { id: crypto.randomUUID(), tenantId, key, label: input.label, kind: input.kind, @@ -165,6 +208,21 @@ export class PeopleService { .where(and(eq(contactRoleProfiles.tenantId, tenantId), eq(contactRoleProfiles.id, id))).get(); if (!cur) throw Errors.NotFound('Role profile not found'); if (cur.isSystem && patch.active === false) throw Errors.Conflict('System role profiles cannot be deactivated'); + if (patch.emailTemplateId !== undefined || patch.smsTemplateId !== undefined) { + await this.assertTemplatesOwned(tenantId, patch.emailTemplateId, patch.smsTemplateId); + } + // Reactivating a profile whose key collides with an already-active profile + // would hit the partial unique index `uq_crp_tenant_key` (WHERE is_active=1) + // and surface a raw SQLite constraint error (500). Map it to a clean 409. + if (patch.active === true && !cur.active) { + const clash = await this.db.select({ id: contactRoleProfiles.id }).from(contactRoleProfiles) + .where(and( + eq(contactRoleProfiles.tenantId, tenantId), + eq(contactRoleProfiles.key, cur.key), + eq(contactRoleProfiles.active, true), + )).get(); + if (clash) throw Errors.Conflict('Another active role profile already uses this key'); + } await this.db.update(contactRoleProfiles).set({ ...patch, updatedAt: new Date() }) .where(and(eq(contactRoleProfiles.tenantId, tenantId), eq(contactRoleProfiles.id, id))); } diff --git a/server/services/portal.service.ts b/server/services/portal.service.ts index e3ae2917..0b2b1cca 100644 --- a/server/services/portal.service.ts +++ b/server/services/portal.service.ts @@ -15,7 +15,7 @@ */ import { drizzle } from 'drizzle-orm/d1'; import { and, eq, inArray, isNull, or, gt } from 'drizzle-orm'; -import { inspectionAccessTokens, inspections, agreementRequests, inspectionMessages } from '../lib/db/schema'; +import { inspectionAccessTokens, inspections, agreementRequests, inspectionMessages, contactRoleProfiles } from '../lib/db/schema'; import { isReportPublished } from '../lib/status/report-status'; import { PeopleService } from './people.service'; @@ -121,6 +121,38 @@ export class PortalService { })); } + /** + * True when this email holds a live (non-revoked, non-expired) access token + * whose role profile is CLIENT-kind (client/co_client) in this tenant. + * + * Deliberately NARROWER than listRecipientInspections: that method is + * capability-driven (selfRetrieveReport) and so ALSO matches AGENT-kind + * grants (Spec 3 opened selfRetrieveReport for agents). The find-my-report + * redeem branch must route only GENUINE clients to a client session and + * still send agents to the agent dashboard — so it keys on kind='client' + * specifically, not the shared self-retrieve capability (#258 review #9). + */ + async hasLiveClientGrant(tenantId: string, email: string): Promise { + const db = this.d(); + const now = new Date(); + const clientKeys = (await db.select({ key: contactRoleProfiles.key }).from(contactRoleProfiles) + .where(and( + eq(contactRoleProfiles.tenantId, tenantId), + eq(contactRoleProfiles.kind, 'client'), + eq(contactRoleProfiles.active, true), + ))).map((r) => r.key); + if (clientKeys.length === 0) return false; + const row = await db.select({ id: inspectionAccessTokens.id }).from(inspectionAccessTokens) + .where(and( + eq(inspectionAccessTokens.tenantId, tenantId), + eq(inspectionAccessTokens.recipientEmail, email), + inArray(inspectionAccessTokens.role, clientKeys), + isNull(inspectionAccessTokens.revokedAt), + or(isNull(inspectionAccessTokens.expiresAt), gt(inspectionAccessTokens.expiresAt, now)), + )).get(); + return row != null; + } + /** * 6-dimension status snapshot for one inspection. Returns null if the * inspection does not exist under this tenant. diff --git a/server/services/seed/seed-role-profiles.ts b/server/services/seed/seed-role-profiles.ts index 014274b5..ab731485 100644 --- a/server/services/seed/seed-role-profiles.ts +++ b/server/services/seed/seed-role-profiles.ts @@ -16,5 +16,9 @@ export async function seedRoleProfiles(db: DrizzleD1Database, tenantId: string, isSystem: p.isSystem, sortOrder: p.sortOrder, active: true, createdAt: now, updatedAt: now, })); - if (toInsert.length > 0) await db.insert(contactRoleProfiles).values(toInsert); + // onConflictDoNothing guards the check-then-insert race: two concurrent + // first-use seeds (e.g. two ensureSeeds in flight) would otherwise collide on + // the deterministic PK `crp__`. Deterministic ids make the + // insert naturally idempotent under the conflict. + if (toInsert.length > 0) await db.insert(contactRoleProfiles).values(toInsert).onConflictDoNothing(); } diff --git a/tests/unit/agent/magic-login.spec.ts b/tests/unit/agent/magic-login.spec.ts index eca366b9..9a451a12 100644 --- a/tests/unit/agent/magic-login.spec.ts +++ b/tests/unit/agent/magic-login.spec.ts @@ -98,14 +98,17 @@ describe('Agent magic-login primitive', () => { } function buildRequestApp(resolveToken: ReturnType, kv: ReturnType) { + // The request route now EMAILS the single-use link (never returns it), so + // stub the email service and expose the spy for assertions. + const sendAgentLoginLink = vi.fn().mockResolvedValue(undefined); const app = withErrorHandler(new OpenAPIHono()); app.use('*', async (c, next) => { c.env = { DB: {}, TENANT_CACHE: kv } as unknown as HonoConfig['Bindings']; - c.set('services', { portalAccess: { resolveToken } } as unknown as HonoConfig['Variables']['services']); + c.set('services', { portalAccess: { resolveToken }, email: { sendAgentLoginLink } } as unknown as HonoConfig['Variables']['services']); await next(); }); app.route('/api/agent', agentMagicLoginRequestRoutes); - return app; + return { app, sendAgentLoginLink }; } function buildRedeemApp(kv: ReturnType) { @@ -120,7 +123,7 @@ describe('Agent magic-login primitive', () => { } describe('POST /api/agent/magic-login/request', () => { - it('valid agent-kind token + email with an agent account → loginUrl with a code, KV seeded', async () => { + it('valid agent-kind token + email with an agent account → link EMAILED to the agent, KV seeded, { sent: true }', async () => { await seedRoleProfile('buyer_agent', 'agent'); await seedGlobalAgent(AGENT_USER_ID, AGENT_EMAIL); @@ -129,7 +132,7 @@ describe('Agent magic-login primitive', () => { recipientEmail: AGENT_EMAIL, revokedAt: null, expiresAt: null, }); const kv = makeKv(); - const app = buildRequestApp(resolveToken, kv); + const { app, sendAgentLoginLink } = buildRequestApp(resolveToken, kv); const res = await app.request('/api/agent/magic-login/request', { method: 'POST', @@ -138,17 +141,24 @@ describe('Agent magic-login primitive', () => { }); expect(res.status).toBe(200); - const body = await res.json() as { data: { loginUrl: string | null } }; - expect(body.data.loginUrl).toMatch(/\/agent\/magic-login\?code=[0-9a-f-]+$/); + const body = await res.json() as { data: { sent: boolean } }; + expect(body.data.sent).toBe(true); expect(resolveToken).toHaveBeenCalledWith('live-report-token'); - const code = new URL(body.data.loginUrl!, 'http://x').searchParams.get('code'); + // The single-use link is EMAILED to the agent's own inbox, never + // returned to the caller (closes the report-link → session takeover). + expect(sendAgentLoginLink).toHaveBeenCalledTimes(1); + const [toEmail, loginUrl] = sendAgentLoginLink.mock.calls[0] as [string, string]; + expect(toEmail).toBe(AGENT_EMAIL); + expect(loginUrl).toMatch(/\/agent\/magic-login\?code=[0-9a-f-]+$/); + + const code = new URL(loginUrl, 'http://x').searchParams.get('code'); const raw = await kv.get(`agent_ml:${code}`); expect(raw).not.toBeNull(); expect(JSON.parse(raw!)).toMatchObject({ userId: AGENT_USER_ID }); }); - it('no agent account for the recipient email → loginUrl: null, no KV write (anti-oracle)', async () => { + it('no agent account for the recipient email → { sent: true }, no email, no KV write (anti-oracle)', async () => { await seedRoleProfile('buyer_agent', 'agent'); // Deliberately no users row for this email. @@ -157,7 +167,7 @@ describe('Agent magic-login primitive', () => { recipientEmail: 'nobody@example.com', revokedAt: null, expiresAt: null, }); const kv = makeKv(); - const app = buildRequestApp(resolveToken, kv); + const { app, sendAgentLoginLink } = buildRequestApp(resolveToken, kv); const res = await app.request('/api/agent/magic-login/request', { method: 'POST', @@ -165,16 +175,19 @@ describe('Agent magic-login primitive', () => { body: JSON.stringify({ tenant: 'acme', inspectionId: INSP_ID, token: 'live-report-token' }), }); + // Anti-oracle: identical { sent: true } response, but no email + no KV + // write when no agent account exists for the recipient. expect(res.status).toBe(200); - const body = await res.json() as { data: { loginUrl: string | null } }; - expect(body.data.loginUrl).toBeNull(); + const body = await res.json() as { data: { sent: boolean } }; + expect(body.data.sent).toBe(true); + expect(sendAgentLoginLink).not.toHaveBeenCalled(); expect(kv.store.size).toBe(0); }); it('invalid/revoked/expired/inspection-mismatched report token → 401', async () => { const resolveToken = vi.fn().mockResolvedValue(null); const kv = makeKv(); - const app = buildRequestApp(resolveToken, kv); + const { app } = buildRequestApp(resolveToken, kv); const res = await app.request('/api/agent/magic-login/request', { method: 'POST', @@ -194,7 +207,7 @@ describe('Agent magic-login primitive', () => { recipientEmail: 'client@example.com', revokedAt: null, expiresAt: null, }); const kv = makeKv(); - const app = buildRequestApp(resolveToken, kv); + const { app } = buildRequestApp(resolveToken, kv); const res = await app.request('/api/agent/magic-login/request', { method: 'POST', diff --git a/tests/unit/auth/auth.service.spec.ts b/tests/unit/auth/auth.service.spec.ts index 40cbeba4..7ef0774f 100644 --- a/tests/unit/auth/auth.service.spec.ts +++ b/tests/unit/auth/auth.service.spec.ts @@ -103,6 +103,44 @@ describe('AuthService', () => { .rejects.toThrow('Invalid email or password'); }); + // #258 review #3 — the tenant /login front door must exclude global-agent + // rows (role='agent', tenant_id IS NULL). Agents authenticate only via + // /agent-login; without this a shared email locks the member out. + it('excludes global-agent rows so a member sharing the email still authenticates', async () => { + const email = 'dual@example.com'; + const memberPw = 'member-secret-123'; + const memberHash = await authService.hashPassword(memberPw); + const agentHash = await authService.hashPassword('agent-secret-999'); + + // Global agent row inserted FIRST (lower rowid) — the pre-fix `.get()` + // with no ORDER BY would return this row and shadow the member. + await testDb.insert(users).values({ + id: 'agent-1', tenantId: null, email, passwordHash: agentHash, + role: 'agent', createdAt: new Date(), + }); + await testDb.insert(users).values({ + id: 'member-1', tenantId: 't1', email, passwordHash: memberHash, + role: 'owner', createdAt: new Date(), + }); + + const result = await authService.validateCredentials(email, memberPw); + expect(result.id).toBe('member-1'); + expect(result.tenantId).toBe('t1'); + }); + + it('rejects a global-agent-only email at the tenant login front door', async () => { + const email = 'agent-only@example.com'; + const password = 'agent-secret-999'; + const hash = await authService.hashPassword(password); + await testDb.insert(users).values({ + id: 'agent-2', tenantId: null, email, passwordHash: hash, + role: 'agent', createdAt: new Date(), + }); + // Correct password, but the row is excluded → generic invalid-credentials. + await expect(authService.validateCredentials(email, password)) + .rejects.toThrow('Invalid email or password'); + }); + it('should allow joining a team with valid invitation', async () => { const token = 'invite-123'; const email = 'new@example.com'; diff --git a/tests/unit/automations/automation-characterization.spec.ts b/tests/unit/automations/automation-characterization.spec.ts index 1714df7d..68f43f2f 100644 --- a/tests/unit/automations/automation-characterization.spec.ts +++ b/tests/unit/automations/automation-characterization.spec.ts @@ -91,7 +91,7 @@ async function seedRuleAndLog(opts: { await db.insert(schema.automationLogs).values({ id: logId, tenantId: TENANT, automationId: ruleId, inspectionId: opts.inspectionId, recipient: opts.channel === 'sms' ? '+15551234567' : 'jane@example.com', - channel: opts.channel ?? 'email', + channel: opts.channel ?? 'email', recipientRoleKey: 'client', sendAt: new Date(Date.now() - 1000), status: 'pending', } as never); return logId; diff --git a/tests/unit/automations/automation-flush-sms.spec.ts b/tests/unit/automations/automation-flush-sms.spec.ts index c2bc4678..786027e8 100644 --- a/tests/unit/automations/automation-flush-sms.spec.ts +++ b/tests/unit/automations/automation-flush-sms.spec.ts @@ -51,7 +51,7 @@ beforeEach(async () => { // `over.contactId` (when set) seeds a contact + inspection_people 'client' row // under that SAME id, so tests that grant consent for `over.contactId` keep // working unchanged. -async function seedSmsLog(over: { contactId?: string | null; smsBody?: string } = {}) { +async function seedSmsLog(over: { contactId?: string | null; smsBody?: string; recipientKind?: 'role' | 'all' } = {}) { const inspId = crypto.randomUUID(); await db.insert(schema.inspections).values({ id: inspId, tenantId: TENANT, propertyAddress: '1 Main', @@ -67,15 +67,17 @@ async function seedSmsLog(over: { contactId?: string | null; smsBody?: string } } const ruleId = crypto.randomUUID(); const smsBody = over.smsBody ?? 'Hi {{client_name}} — {{company_name}}'; + const recipientKind = over.recipientKind ?? 'role'; await db.insert(schema.automations).values({ - id: ruleId, tenantId: TENANT, name: 'R', trigger: 'report.published', recipientKind: 'role', recipientRoleProfileId: roleProfileId('client'), + id: ruleId, tenantId: TENANT, name: 'R', trigger: 'report.published', + recipientKind, recipientRoleProfileId: recipientKind === 'role' ? roleProfileId('client') : null, delayMinutes: 0, subjectTemplate: 'S', bodyTemplate: 'B', smsBody, channels: '["sms"]', channel: 'sms', active: true, isDefault: false, createdAt: new Date(), } as never); const logId = crypto.randomUUID(); await db.insert(schema.automationLogs).values({ id: logId, tenantId: TENANT, automationId: ruleId, inspectionId: inspId, - recipient: '+15551234567', channel: 'sms', + recipient: '+15551234567', channel: 'sms', recipientRoleKey: 'client', sendAt: new Date(Date.now() - 1000), status: 'pending', } as never); // SP2 — give the seeded sms rule a referenced template (body == embedded smsBody), @@ -118,6 +120,18 @@ describe('flush() — SMS branch (Track L)', () => { expect(fakeSendMessage).not.toHaveBeenCalled(); }); + it('#1 recipientKind="all" does NOT bypass the client consent gate → skipped', async () => { + // Regression: the gate keys on the per-recipient log.recipientRoleKey, not + // the rule's recipientKind. An 'all' rule fanning out to the client must + // still be blocked without recorded consent (was a TCPA bypass). + const { logId } = await seedSmsLog({ contactId: 'c1', recipientKind: 'all' }); + await svc.flush(stubEmailFor, 'Acme', 'https://acme.example.com', smsRuntime); + const r = await statusOf(logId); + expect(r?.status).toBe('skipped'); + expect(r?.error).toMatch(/consent/); + expect(fakeSendMessage).not.toHaveBeenCalled(); + }); + it('client SMS with granted consent → sent via provider', async () => { const { logId } = await seedSmsLog({ contactId: 'c1' }); await new SmsConsentService({} as D1Database).record(TENANT, 'c1', 'granted', 'admin', {}); diff --git a/tests/unit/portal/find-my-report-agent-dest.spec.ts b/tests/unit/portal/find-my-report-agent-dest.spec.ts index 49638433..1719a94e 100644 --- a/tests/unit/portal/find-my-report-agent-dest.spec.ts +++ b/tests/unit/portal/find-my-report-agent-dest.spec.ts @@ -19,6 +19,8 @@ vi.mock('../../../server/lib/jwt-keyring', async (importOriginal) => { // eslint-disable-next-line import/order import portalRoutes from '../../../server/api/portal'; +import { PortalService } from '../../../server/services/portal.service'; +import { seedRoleProfiles } from '../../../server/services/seed/seed-role-profiles'; /** * Spec 3 Task 7 — GET /api/portal/:tenant/redeem (find-my-report magic-link @@ -41,11 +43,13 @@ describe('GET /api/portal/:tenant/redeem — find-my-report agent destination', let sqlite: ReturnType['sqlite']; function buildApp() { + const portalSvc = new PortalService({} as D1Database, { getObserveProgress: async () => { throw new Error('unused in this suite'); } }); const app = new OpenAPIHono(); app.use('*', async (c, next) => { c.set('tenantId', TENANT); c.set('requestedTenantSlug', 'acme-fmr'); c.set('keyringPromise', Promise.resolve({}) as unknown as HonoConfig['Variables']['keyringPromise']); + c.set('services', { portal: portalSvc } as unknown as HonoConfig['Variables']['services']); await next(); }); app.route('/api/portal', portalRoutes); @@ -59,6 +63,20 @@ describe('GET /api/portal/:tenant/redeem — find-my-report agent destination', } as never); } + async function seedInsp(id: string) { + await testDb.insert(schema.inspections).values({ + id, tenantId: TENANT, propertyAddress: `${id} Main St`, date: '2026-06-01', + status: 'requested', reportStatus: 'in_progress', paymentStatus: 'unpaid', createdAt: new Date(), + } as never); + } + + async function seedToken(inspectionId: string, email: string, role: string) { + await testDb.insert(schema.inspectionAccessTokens).values({ + id: crypto.randomUUID(), tenantId: TENANT, inspectionId, recipientEmail: email, role, + token: crypto.randomUUID(), createdAt: new Date(), expiresAt: null, revokedAt: null, + } as never); + } + beforeEach(async () => { const fix = createTestDb(); testDb = fix.db; @@ -70,6 +88,7 @@ describe('GET /api/portal/:tenant/redeem — find-my-report agent destination', id: TENANT, name: 'Acme', slug: 'acme-fmr', status: 'active', deploymentMode: 'shared', tier: 'free', createdAt: new Date(), } as never); + await seedRoleProfiles(testDb, TENANT, new Date(1)); }); afterEach(() => { @@ -126,6 +145,45 @@ describe('GET /api/portal/:tenant/redeem — find-my-report agent destination', expect(signJwtMock).not.toHaveBeenCalled(); }); + it('#9 dual identity: a global-agent email that ALSO holds a live client grant -> client __Host-portal_session, NOT agent', async () => { + const DUAL_EMAIL = 'dual@example.com'; + await seedGlobalAgent('00000000-0000-0000-0000-0000000000aa', DUAL_EMAIL); + await seedInsp('insp-dual'); + await seedToken('insp-dual', DUAL_EMAIL, 'client'); // client-KIND grant + const link = await signMagicLink(JWT_SECRET, DUAL_EMAIL); + + const res = await buildApp().request( + `/api/portal/acme-fmr/redeem?link=${encodeURIComponent(link)}`, {}, reqEnv()); + + expect(res.status).toBe(200); + const body = (await res.json()) as { data: { email: string; agent?: boolean } }; + expect(body.data.email).toBe(DUAL_EMAIL); + // Genuine client on this inspection — must reach the client portal even + // though the email also has a global agent account. + expect(body.data.agent).toBeUndefined(); + expect(res.headers.get('set-cookie') ?? '').toContain('__Host-portal_session='); + expect(signJwtMock).not.toHaveBeenCalled(); + }); + + it('#9 guard: a global agent holding only an AGENT-kind grant still routes to the agent dashboard (client-kind check must not match agents)', async () => { + const AGENT_EMAIL = 'buyeragent@example.com'; + await seedGlobalAgent('00000000-0000-0000-0000-0000000000ab', AGENT_EMAIL); + await seedInsp('insp-agent'); + await seedToken('insp-agent', AGENT_EMAIL, 'buyer_agent'); // agent-KIND grant (has selfRetrieveReport) + const link = await signMagicLink(JWT_SECRET, AGENT_EMAIL); + + const res = await buildApp().request( + `/api/portal/acme-fmr/redeem?link=${encodeURIComponent(link)}`, {}, reqEnv()); + + expect(res.status).toBe(200); + const body = (await res.json()) as { data: { email: string; agent?: boolean } }; + expect(body.data.agent).toBe(true); + const joined = (res.headers.getSetCookie ? res.headers.getSetCookie() : [res.headers.get('set-cookie') ?? '']).join('\n'); + expect(joined).toContain('__Host-inspector_token='); + expect(joined).not.toContain('__Host-portal_session='); + expect(signJwtMock).toHaveBeenCalledTimes(1); + }); + it('regression: an invalid/expired magic-link token -> unchanged 401, no cookie, no signJwt call', async () => { const res = await buildApp().request( '/api/portal/acme-fmr/redeem?link=not-a-real-token', {}, reqEnv()); From 79c66d189c4f91e80d6cede89c96886ba12842b5 Mon Sep 17 00:00:00 2001 From: important-new Date: Wed, 22 Jul 2026 10:23:22 +0800 Subject: [PATCH 2/2] fix(ci): regen OpenAPI snapshot + update agent-unified-link e2e for emailed link The magic-login/request response changed from { loginUrl } to { sent: true } (the single-use link is emailed to the agent now, not returned), which drifted the committed OpenAPI snapshot and broke the e2e that asserted the old return-and-navigate flow. - regenerate server/lib/mcp/openapi-snapshot.json - rewrite Scenario 1 + Scenario 4 to the emailed-link flow: request -> { sent } -> read the single-use link from the E2E email sink -> redeem -> /agent-dashboard (Scenario 4 waits for a code that differs from an earlier scenario's email) Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_017YiCgWAmqyzpfs3pp1kjg3 --- server/lib/mcp/openapi-snapshot.json | 4 +- tests/e2e/agent-unified-link.spec.ts | 80 ++++++++++++++++++++-------- 2 files changed, 60 insertions(+), 24 deletions(-) diff --git a/server/lib/mcp/openapi-snapshot.json b/server/lib/mcp/openapi-snapshot.json index 5f24c158..a1edbb3b 100644 --- a/server/lib/mcp/openapi-snapshot.json +++ b/server/lib/mcp/openapi-snapshot.json @@ -15065,8 +15065,8 @@ "$ref": "#/components/schemas/AgentMagicLoginRequest" } }, - "summary": "Exchange an agent report token for a login code", - "description": "Public, unauthenticated endpoint for the agent unified link. Exchanges a live, agent-kind report-link token for a single-use magic-login code (900 second TTL) that mints an agent session cookie on redeem. Returns loginUrl: null with 200 (not an error) when the token is valid but no global agent account exists yet for the recipient email, so probing tokens cannot be used to enumerate accounts." + "summary": "Email an agent a single-use sign-in link for a report token", + "description": "Public, unauthenticated endpoint for the agent unified link. For a live, agent-kind report-link token whose recipient email has a global agent account, EMAILS a single-use magic-login link (900 second TTL) to that agent's account inbox — the link is never returned to the caller, so a leaked/forwarded report link cannot be replayed into a full agent session. Always returns { sent: true } with 200 (identical response and timing whether or not an account exists) so probing tokens cannot enumerate accounts." }, { "operationId": "requestInvoicePayment", diff --git a/tests/e2e/agent-unified-link.spec.ts b/tests/e2e/agent-unified-link.spec.ts index 6de416dc..c1bfb73d 100644 --- a/tests/e2e/agent-unified-link.spec.ts +++ b/tests/e2e/agent-unified-link.spec.ts @@ -9,15 +9,16 @@ * renders the report directly instead of bouncing to the generic * "Sign in to your portal" page — Task 6's loader agent-branch * (app/lib/portal-exchange.ts's `isAgentToken` short-circuit). - * - Below the report, (Task 3) shows either "Go to my - * workspace" (a global agent account already exists for the recipient + * - Below the report, (Task 3) shows either "Email me a + * sign-in link" (a global agent account already exists for the recipient * email) or "Create your free agent account" (it doesn't). - * - "Go to my workspace" exchanges the durable report token for a single-use - * magic-login code (POST /api/agent/magic-login/request, Task 2) and - * redeems it (GET /agent/magic-login?code=) into an agent JWT — a - * SEPARATE token family from the report token itself (see the SECURITY - * note in server/api/agent/magic-login.ts). The report token alone can - * NEVER mint that session. + * - "Email me a sign-in link" asks the server (POST /api/agent/magic-login/ + * request, Task 2) to EMAIL a single-use magic-login link to the agent's + * own inbox — the link is never returned to the caller (#258 review #5), so + * a leaked/forwarded report link can't be replayed into a session. The agent + * opens the emailed link (GET /agent/magic-login?code=) to mint an agent JWT + * — a SEPARATE token family from the report token itself. The report token + * alone can NEVER mint that session. * - "Create your free agent account" prefills /agent-signup?email=... (Task * 4) and, on conversion from a report link, lands the new agent on * /agent-dashboard?welcome= (Task 4c). @@ -35,9 +36,10 @@ * not the actual :8789 wrangler dev port). Calling the SAME underlying public * API endpoints directly over a genuine top-level HTTP request (as this file * does via `request.post`/`request.get`) exercises the identical service code - * (server/services/agent/magic-login.service.ts) but gets a loginUrl built off - * the REAL request Host — the only difference from clicking the on-page CTA is - * which HTTP client fires the request, not which server code runs. + * (server/services/agent/magic-login.service.ts) and emails the sign-in link + * built off the REAL request Host — the only difference from clicking the + * on-page CTA is which HTTP client fires the request, not which server code runs. + * The emitted link is then read back from the E2E email sink. * * Run: npm run test:e2e -- agent-unified-link */ @@ -281,7 +283,7 @@ test.describe.serial('Agent unified link (Spec 3 Task 8)', () => { expect(publishBRes.status(), 'inspection B report must publish').toBe(200); }); - test('Scenario 1 — registered agent: report link renders + "Go to my workspace" -> authenticated /agent-dashboard', async ({ + test('Scenario 1 — registered agent: report link renders + "Email me a sign-in link" -> emailed code -> authenticated /agent-dashboard', async ({ page, request, }) => { @@ -309,12 +311,15 @@ test.describe.serial('Agent unified link (Spec 3 Task 8)', () => { const workspaceCta = freshPage.getByTestId('agent-report-workspace-cta'); await expect(workspaceCta).toBeVisible(); - await expect(workspaceCta).toHaveText('Go to my workspace'); + await expect(workspaceCta).toHaveText('Email me a sign-in link'); // Drive the SAME exchange the CTA's click posts through // (POST /api/agent/magic-login/request) directly over a real top-level // request — see the file header for why this, not a UI click, is the - // robust way to prove the redeem in this harness. + // robust way to prove the redeem in this harness. The endpoint EMAILS the + // single-use sign-in link to the agent's own inbox (never returns it), so + // it answers { sent: true } and we fetch the link from the email sink — + // this is the takeover-hardening from #258 review #5. const reportToken = new URL(portalUrl!).searchParams.get('token'); expect(reportToken, 'portal link must carry ?token=').toBeTruthy(); @@ -324,10 +329,21 @@ test.describe.serial('Agent unified link (Spec 3 Task 8)', () => { }); expect(magicRes.status(), 'magic-login/request for a registered agent').toBe(200); const magicBody = await magicRes.json(); - const loginUrl = magicBody.data?.loginUrl as string | null; - expect(loginUrl, 'a registered agent must get a non-null loginUrl').toBeTruthy(); - - await freshPage.goto(loginUrl!, { waitUntil: 'networkidle', timeout: NAV_TIMEOUT }); + expect(magicBody.data?.sent, 'request must report { sent: true } (link is emailed, never returned)').toBe(true); + + // The sign-in link email lands AFTER the report email already in the sink + // (deferred via waitUntil), so poll until the latest email carries a + // magic-login code rather than the report link. + let code: string | null = null; + for (let attempt = 0; attempt < 15; attempt++) { + const em = await pollLastEmail(request, REGISTERED_AGENT.email); + code = extractMagicLoginCode(String(em.html)); + if (code) break; + await new Promise((resolve) => setTimeout(resolve, 300)); + } + expect(code, 'the emailed sign-in link must contain a magic-login code').toBeTruthy(); + + await freshPage.goto(`${BASE_URL}/agent/magic-login?code=${code}`, { waitUntil: 'networkidle', timeout: NAV_TIMEOUT }); expect(freshPage.url()).toContain('/agent-dashboard'); await expect(freshPage.getByRole('heading', { name: 'Agent Dashboard' })).toBeVisible({ timeout: 10000 }); await expect(freshPage.getByTestId(`referral-row-${inspectionAId}`)).toBeVisible({ timeout: 10000 }); @@ -451,22 +467,42 @@ test.describe.serial('Agent unified link (Spec 3 Task 8)', () => { const reportToken = new URL(registeredPortalUrl).searchParams.get('token'); expect(reportToken, 'registeredPortalUrl must still carry ?token=').toBeTruthy(); + // Earlier scenarios already emailed this agent a sign-in code, and the sink + // returns the LATEST email — so capture that stale code first and then wait + // for THIS request's fresh code to supersede it (the link is emailed now, + // never returned — #258 review #5). + let prevCode: string | null = null; + try { + prevCode = extractMagicLoginCode(String((await pollLastEmail(request, REGISTERED_AGENT.email)).html)); + } catch { + prevCode = null; + } + const magicRes = await request.post(`${BASE_URL}/api/agent/magic-login/request`, { data: { tenant: 'e2e-agent-unified-link', inspectionId: inspectionAId, token: reportToken }, headers: { 'Content-Type': 'application/json' }, }); expect(magicRes.status()).toBe(200); - const loginUrl = (await magicRes.json()).data?.loginUrl as string | null; - expect(loginUrl, 'a fresh magic-login code must be issued').toBeTruthy(); + expect((await magicRes.json()).data?.sent, 'request must report { sent: true }').toBe(true); + + let code: string | null = null; + for (let attempt = 0; attempt < 20; attempt++) { + const em = await pollLastEmail(request, REGISTERED_AGENT.email); + const c = extractMagicLoginCode(String(em.html)); + if (c && c !== prevCode) { code = c; break; } + await new Promise((resolve) => setTimeout(resolve, 300)); + } + expect(code, 'a fresh magic-login code must be emailed').toBeTruthy(); + const loginUrl = `${BASE_URL}/agent/magic-login?code=${code}`; - const first = await request.get(loginUrl!, { maxRedirects: 0 }); + const first = await request.get(loginUrl, { maxRedirects: 0 }); expect(first.status(), 'first redeem must succeed').toBe(302); expect(first.headers()['location']).toContain('/agent-dashboard'); expect(first.headers()['set-cookie'] ?? '', 'first redeem must mint the session cookie').toContain( '__Host-inspector_token', ); - const second = await request.get(loginUrl!, { maxRedirects: 0 }); + const second = await request.get(loginUrl, { maxRedirects: 0 }); expect(second.status(), 'second redeem of the SAME code must fail').toBe(302); expect(second.headers()['location']).toContain('/agent-login'); expect(second.headers()['location']).toContain('error=expired_link');