From de05bd9ccbeee468678d3e6acb6decdade33bb8d Mon Sep 17 00:00:00 2001 From: Rajat Date: Tue, 3 Nov 2026 13:42:34 +0530 Subject: [PATCH 1/3] fix(security): close the five high-severity audit findings - CSRF: cookie-session writes on the REST API now require the dashboard origin and the double-submit CSRF token; the dashboard proxy checks the origin and attaches the token itself. - OAuth tokens issued for the MCP resource no longer authenticate REST. - MCP ESP and Postmaster/SNDS tools enforce the same team-admin rule as REST (shared in team/authority.ts). - The dashboard /api/proxy sandboxes every response (CSP + nosniff) so tenant HTML cannot run on the dashboard origin. - Removing or demoting an organization member revokes their team access and the team API keys they created. Docs for authentication and MCP updated. Co-Authored-By: Claude Sonnet 5.5 --- apps/api/src/auth/csrf.ts | 87 ++++++++++++++++++ apps/api/src/auth/middleware.test.ts | 91 +++++++++++++++++++ apps/api/src/auth/middleware.ts | 31 ++++++- apps/api/src/billing/security.ts | 90 ++---------------- apps/api/src/mcp/policy.ts | 56 ++++++++++++ apps/api/src/mcp/tool.ts | 11 ++- apps/api/src/mcp/tools/esp.test.ts | 40 ++++++++ apps/api/src/mcp/tools/settings-tools.test.ts | 34 +++++++ apps/api/src/organization/queries.test.ts | 82 +++++++++++++++++ apps/api/src/organization/queries.ts | 42 +++++++++ apps/api/src/reputation/routes.ts | 8 +- apps/api/src/settings/esp/routes.ts | 11 +-- apps/api/src/team/authority.test.ts | 47 ++++++++++ apps/api/src/team/authority.ts | 29 ++++++ .../docs/developers/authentication.mdx | 10 ++ apps/docs/content/docs/developers/mcp.mdx | 10 ++ .../web/app/api/proxy/[...path]/route.test.ts | 70 +++++++++++++- apps/web/app/api/proxy/[...path]/route.ts | 35 ++++++- 18 files changed, 678 insertions(+), 106 deletions(-) create mode 100644 apps/api/src/auth/csrf.ts create mode 100644 apps/api/src/team/authority.test.ts create mode 100644 apps/api/src/team/authority.ts diff --git a/apps/api/src/auth/csrf.ts b/apps/api/src/auth/csrf.ts new file mode 100644 index 00000000..5f8e26b1 --- /dev/null +++ b/apps/api/src/auth/csrf.ts @@ -0,0 +1,87 @@ +import { randomBytes, timingSafeEqual } from "node:crypto"; + +export type SessionBoundaryFailure = { + status: 403; + body: { error: string }; +}; + +export function billingMutationOrigin(req: any): boolean { + if (!req.userId || req.authKind !== "session") return false; + const originHeader = + typeof req.headers.origin === "string" ? req.headers.origin : null; + const refererHeader = + typeof req.headers.referer === "string" ? req.headers.referer : null; + let origin = originHeader; + if (!origin && refererHeader) { + try { + origin = new URL(refererHeader).origin; + } catch { + origin = null; + } + } + if (!origin) return false; + const allowed = new Set(); + for (const value of [process.env.API_PUBLIC_URL, process.env.WEB_CLIENT]) { + if (!value) continue; + try { + allowed.add(new URL(value).origin); + } catch { + // Startup configuration validation reports malformed URLs. + } + } + return allowed.has(origin); +} + +function cookieValue(req: any, name: string): string | null { + const header = typeof req.headers?.cookie === "string" ? req.headers.cookie : ""; + for (const part of header.split(";")) { + const [key, ...value] = part.trim().split("="); + if (key === name) return value.join("=") || null; + } + return null; +} + +export function ensureCsrfCookie(req: any, res: any): string { + const existing = cookieValue(req, "sendlit_csrf"); + if (existing) return existing; + const token = randomBytes(32).toString("base64url"); + const secure = process.env.NODE_ENV === "production" ? "; Secure" : ""; + res.append("Set-Cookie", `sendlit_csrf=${token}; Path=/; SameSite=Lax${secure}`); + return token; +} + +function csrfTokenMatches(req: any, res: any): boolean { + const cookie = cookieValue(req, "sendlit_csrf"); + const header = + typeof req.headers?.["x-sendlit-csrf"] === "string" + ? req.headers["x-sendlit-csrf"] + : ""; + if (!cookie || !header) { + ensureCsrfCookie(req, res); + return false; + } + const left = Buffer.from(cookie); + const right = Buffer.from(header); + return left.length === right.length && timingSafeEqual(left, right); +} + +/** Session-only, same-origin and CSRF checks for sensitive dashboard writes + * (billing actions, account deletion). */ +export function dashboardSessionBoundary( + req: any, + res: any, +): SessionBoundaryFailure | null { + if (!req.userId || req.authKind !== "session") { + return { + status: 403, + body: { error: "billing_human_session_required" }, + }; + } + if (!billingMutationOrigin(req)) { + return { status: 403, body: { error: "csrf_origin_invalid" } }; + } + if (!csrfTokenMatches(req, res)) { + return { status: 403, body: { error: "csrf_token_invalid" } }; + } + return null; +} diff --git a/apps/api/src/auth/middleware.test.ts b/apps/api/src/auth/middleware.test.ts index 7b7af813..e1f4062a 100644 --- a/apps/api/src/auth/middleware.test.ts +++ b/apps/api/src/auth/middleware.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it, mock, vi } from "bun:test"; mock.module("./better-auth", () => ({ SESSION_COOKIE_NAME: "sendlit.session_token", + mcpResourceUrl: "https://sendlit.test/mcp", mcpProtectedResourceMetadataUrl: "https://sendlit.test/.well-known/oauth-protected-resource/mcp", })); @@ -87,3 +88,93 @@ describe("MCP auth middleware", () => { expect(JSON.stringify(authState)).not.toContain("sl_live_secret"); }); }); + +describe("REST auth middleware", () => { + const oauth = (audiences: string[]) => ({ + status: "authenticated" as const, + kind: "oauth" as const, + identity: { method: "oauth" as const, audiences }, + user: { id: "user-1" }, + userId: "user-1", + token: "t", + clientId: "client", + scopes: ["contacts:read"], + }); + const session = { + status: "authenticated" as const, + kind: "session" as const, + identity: { method: "session" as const }, + user: { id: "user-1" }, + userId: "user-1", + scopes: ["web"] as ["web"], + }; + + async function run(auth: unknown, req: Record) { + const middleware = createAuthMiddleware("rest", vi.fn(async () => auth) as any); + const request: any = { headers: {}, body: undefined, method: "GET", ...req }; + const res = responseDouble(); + const next = vi.fn(); + await middleware(request, res, next); + return { res, next, request }; + } + + it("refuses an OAuth token that is only valid for the MCP resource", async () => { + const { res, next } = await run(oauth(["https://sendlit.test/mcp"]), {}); + expect(res.statusCode).toBe(401); + expect(res.body).toMatchObject({ error: "invalid_token" }); + expect(next).not.toHaveBeenCalled(); + }); + + it("still accepts an OAuth token issued for the API itself", async () => { + const { next } = await run(oauth(["https://sendlit.test"]), {}); + expect(next).toHaveBeenCalledTimes(1); + }); + + it("refuses a cookie-session write that has no CSRF token", async () => { + // The attacker page is same-origin, so its Origin is allowed: only the + // token can stop it. + process.env.API_PUBLIC_URL = "https://sendlit.test"; + const { res, next } = await run(session, { + method: "POST", + headers: { origin: "https://sendlit.test", cookie: "sendlit_csrf=abc" }, + }); + expect(res.statusCode).toBe(403); + expect(res.body).toEqual({ error: "csrf_token_invalid" }); + expect(next).not.toHaveBeenCalled(); + }); + + it("refuses a cookie-session write from another origin", async () => { + process.env.API_PUBLIC_URL = "https://sendlit.test"; + const { res } = await run(session, { + method: "DELETE", + headers: { origin: "https://evil.test" }, + }); + expect(res.statusCode).toBe(403); + expect(res.body).toEqual({ error: "csrf_origin_invalid" }); + }); + + it("allows a cookie-session write with a trusted origin and matching token", async () => { + process.env.API_PUBLIC_URL = "https://sendlit.test"; + const { next } = await run(session, { + method: "POST", + headers: { + origin: "https://sendlit.test", + cookie: "sendlit_csrf=abc", + "x-sendlit-csrf": "abc", + }, + }); + expect(next).toHaveBeenCalledTimes(1); + }); + + it("does not require a token for reads or for API keys", async () => { + expect((await run(session, { method: "GET" })).next).toHaveBeenCalledTimes(1); + const key = { + status: "authenticated", + kind: "team_key", + user: null, + apiKeyId: "k", + teamId: "t", + }; + expect((await run(key, { method: "POST" })).next).toHaveBeenCalledTimes(1); + }); +}); diff --git a/apps/api/src/auth/middleware.ts b/apps/api/src/auth/middleware.ts index 3028ad34..9217e425 100644 --- a/apps/api/src/auth/middleware.ts +++ b/apps/api/src/auth/middleware.ts @@ -1,5 +1,6 @@ import type { NextFunction, Request, Response } from "express"; -import { mcpProtectedResourceMetadataUrl } from "./better-auth"; +import { dashboardSessionBoundary } from "./csrf"; +import { mcpProtectedResourceMetadataUrl, mcpResourceUrl } from "./better-auth"; import { type AuthInput, type AuthResult, @@ -9,6 +10,8 @@ import { type AuthResolver = (input: AuthInput) => Promise; +const SAFE_METHODS = new Set(["GET", "HEAD", "OPTIONS"]); + type AuthMiddlewareMode = "rest" | "mcp"; function applyAuthToRequest(req: any, auth: AuthResult, mode: AuthMiddlewareMode) { @@ -84,7 +87,33 @@ export function createAuthMiddleware( return; } + // OAuth tokens are issued for the MCP resource only. REST has no scope + // map, so honouring one there would turn a narrowly scoped MCP grant + // into the user's full authority (including minting team API keys). + if ( + mode === "rest" && + auth.kind === "oauth" && + !auth.identity.audiences?.some((audience) => audience !== mcpResourceUrl) + ) { + res.status(401).json({ + error: "invalid_token", + error_description: "This token is only valid for the MCP resource.", + }); + return; + } + applyAuthToRequest(req, auth, mode); + + // A session cookie is sent by the browser on any request to this origin, + // including a form on a tenant-published page. State changes therefore + // need the dashboard's Origin plus double-submit CSRF token. + if (mode === "rest" && auth.kind === "session" && !SAFE_METHODS.has(req.method)) { + const failure = dashboardSessionBoundary(req, res); + if (failure) { + res.status(failure.status).json(failure.body); + return; + } + } if (auth.kind === "oauth" || auth.kind === "session") { req.auth = auth.identity; } diff --git a/apps/api/src/billing/security.ts b/apps/api/src/billing/security.ts index 235c7640..5dcaed6d 100644 --- a/apps/api/src/billing/security.ts +++ b/apps/api/src/billing/security.ts @@ -1,4 +1,4 @@ -import { createHash, randomBytes, randomUUID, timingSafeEqual } from "node:crypto"; +import { createHash, randomBytes, randomUUID } from "node:crypto"; import { BillingWorkflowError } from "@codelitdev/billing/core"; import type { BillingActionGrant, @@ -8,6 +8,7 @@ import type { BillingGrantTarget } from "@codelitdev/platform/billing"; import { fromNodeHeaders } from "better-auth/node"; import { and, eq, gt, like, lte } from "drizzle-orm"; import { auth } from "../auth/better-auth"; +import { dashboardSessionBoundary } from "../auth/csrf"; import { db } from "../db/client"; import { organizationMembers, organizations, verification } from "../db/schema"; import { @@ -54,66 +55,6 @@ export const billingActions: readonly BillingAction[] = [ "pending_hide", ] as const; -export function billingMutationOrigin(req: any): boolean { - if (!req.userId || req.authKind !== "session") return false; - const originHeader = - typeof req.headers.origin === "string" ? req.headers.origin : null; - const refererHeader = - typeof req.headers.referer === "string" ? req.headers.referer : null; - let origin = originHeader; - if (!origin && refererHeader) { - try { - origin = new URL(refererHeader).origin; - } catch { - origin = null; - } - } - if (!origin) return false; - const allowed = new Set(); - for (const value of [process.env.API_PUBLIC_URL, process.env.WEB_CLIENT]) { - if (!value) continue; - try { - allowed.add(new URL(value).origin); - } catch { - // Startup configuration validation reports malformed URLs. - } - } - return allowed.has(origin); -} - -function cookieValue(req: any, name: string): string | null { - const header = typeof req.headers?.cookie === "string" ? req.headers.cookie : ""; - for (const part of header.split(";")) { - const [key, ...value] = part.trim().split("="); - if (key === name) return value.join("=") || null; - } - return null; -} - -export function ensureCsrfCookie(req: any, res: any): string { - const existing = cookieValue(req, "sendlit_csrf"); - if (existing) return existing; - const token = randomBytes(32).toString("base64url"); - const secure = process.env.NODE_ENV === "production" ? "; Secure" : ""; - res.append("Set-Cookie", `sendlit_csrf=${token}; Path=/; SameSite=Lax${secure}`); - return token; -} - -function csrfTokenMatches(req: any, res: any): boolean { - const cookie = cookieValue(req, "sendlit_csrf"); - const header = - typeof req.headers?.["x-sendlit-csrf"] === "string" - ? req.headers["x-sendlit-csrf"] - : ""; - if (!cookie || !header) { - ensureCsrfCookie(req, res); - return false; - } - const left = Buffer.from(cookie); - const right = Buffer.from(header); - return left.length === right.length && timingSafeEqual(left, right); -} - function tokenHash(token: string): string { return createHash("sha256").update(token, "utf8").digest("hex"); } @@ -130,27 +71,6 @@ async function sessionContext(req: any) { return current; } -/** Session-only, same-origin and CSRF checks for sensitive dashboard writes - * (billing actions, account deletion). */ -export function dashboardSessionBoundary( - req: any, - res: any, -): BillingSecurityFailure | null { - if (!req.userId || req.authKind !== "session") { - return { - status: 403, - body: { error: "billing_human_session_required" }, - }; - } - if (!billingMutationOrigin(req)) { - return { status: 403, body: { error: "csrf_origin_invalid" } }; - } - if (!csrfTokenMatches(req, res)) { - return { status: 403, body: { error: "csrf_token_invalid" } }; - } - return null; -} - function isLegacyAction(action: BillingAction): action is LegacyAction { return action === "organization_close" || action === "pending_hide"; } @@ -462,3 +382,9 @@ export async function requireBillingAction( return { status: 503, body: { error: "billing_security_unavailable" } }; } } + +export { + billingMutationOrigin, + dashboardSessionBoundary, + ensureCsrfCookie, +} from "../auth/csrf"; diff --git a/apps/api/src/mcp/policy.ts b/apps/api/src/mcp/policy.ts index 5ae7eaa5..876cec87 100644 --- a/apps/api/src/mcp/policy.ts +++ b/apps/api/src/mcp/policy.ts @@ -1,5 +1,6 @@ import { createPlatformError, type PlatformError } from "@codelitdev/platform"; import type { SendLitMcpContext } from "./context"; +import { canManageTeamEsp, isTeamAdminOrKey } from "../team/authority"; export const MCP_SCOPES = { contactsRead: "contacts:read", @@ -168,6 +169,61 @@ const teamKeyDeniedOperations: ReadonlySet = new Set([ "teams.removeKey", ]); +/** + * Operations REST reserves to team admins (or a team key), whatever scope the + * caller holds. A scope only says what a token may do for its user; it says + * nothing about the user's role in the team. Without this a plain member could + * consent to `esp:write` for their own client and repoint the team's ESP at a + * host they control. + */ +const teamAdminOperations: ReadonlySet = new Set([ + "reputation.connectGoogle", + "reputation.disconnectGoogle", + "reputation.connectSnds", + "reputation.disconnectSnds", + "settings.esps.create", + "settings.esps.update", + "settings.esps.remove", + "settings.esps.test", + "settings.esps.activate", + "settings.esps.retire", +]); + +/** Of those, the ones that also need the organization to allow team ESPs. */ +const teamEspOperations: ReadonlySet = new Set([ + "settings.esps.create", + "settings.esps.update", + "settings.esps.remove", + "settings.esps.test", + "settings.esps.activate", + "settings.esps.retire", +]); + +/** The role check REST applies, for the operations that have one. Run after + * `authorizeMcpOperation`; null when the caller may proceed. */ +export async function authorizeTeamAuthority( + operationId: string, + context: SendLitMcpContext, +): Promise { + if (!teamAdminOperations.has(operationId)) return null; + const caller = { + authKind: context.authKind, + teamId: context.teamId, + userId: context.user?.id ?? null, + }; + const allowed = teamEspOperations.has(operationId) + ? await canManageTeamEsp(caller) + : await isTeamAdminOrKey(caller); + if (allowed) return null; + return createPlatformError("forbidden", { + safeDetails: { + reason: teamEspOperations.has(operationId) + ? "team_esp_disabled" + : "team_admin_required", + }, + }); +} + export function isMcpOperation(operationId: string): operationId is McpOperationId { return Object.hasOwn(operationScopes, operationId); } diff --git a/apps/api/src/mcp/tool.ts b/apps/api/src/mcp/tool.ts index 389b45da..12340d00 100644 --- a/apps/api/src/mcp/tool.ts +++ b/apps/api/src/mcp/tool.ts @@ -3,7 +3,12 @@ import { z } from "zod"; import logger from "../services/log"; import { serializeDates } from "../utils/serialize"; import type { SendLitMcpContext, SendLitMcpTool } from "./context"; -import { authorizeMcpOperation, getRequiredScope, type McpOperationId } from "./policy"; +import { + authorizeMcpOperation, + authorizeTeamAuthority, + getRequiredScope, + type McpOperationId, +} from "./policy"; export type DedicatedToolDefinition = { name: string; @@ -50,7 +55,9 @@ export function dedicatedTool( async handler({ context, args }) { const startedAt = performance.now(); const log = { tool: tool.name, requiredScope, authKind: context.authKind }; - const denied = authorizeMcpOperation(operationId, context); + const denied = + authorizeMcpOperation(operationId, context) ?? + (await authorizeTeamAuthority(operationId, context)); if (denied) { logger.warn( { ...log, scopeDecision: "denied" }, diff --git a/apps/api/src/mcp/tools/esp.test.ts b/apps/api/src/mcp/tools/esp.test.ts index 55a9625b..f0def894 100644 --- a/apps/api/src/mcp/tools/esp.test.ts +++ b/apps/api/src/mcp/tools/esp.test.ts @@ -13,6 +13,8 @@ const mocks = { invalidateEspTransport: vi.fn(), invalidateTeamTransport: vi.fn(), testEspConfig: vi.fn(), + isTeamAdminOrKey: vi.fn(), + canManageTeamEsp: vi.fn(), }; // bun shares module mocks across test files in one process, so this mock @@ -32,6 +34,10 @@ mock.module("../../mail/transport", () => ({ invalidateEspTransport: mocks.invalidateEspTransport, invalidateTeamTransport: mocks.invalidateTeamTransport, })); +mock.module("../../team/authority", () => ({ + isTeamAdminOrKey: mocks.isTeamAdminOrKey, + canManageTeamEsp: mocks.canManageTeamEsp, +})); mock.module("../../settings/esp/test", () => ({ testEspConfig: mocks.testEspConfig })); const { espTools } = await import("./esp"); @@ -56,6 +62,8 @@ const call = (name: string, args: Record = {}, ctx: any = conte beforeEach(() => { for (const fn of Object.values(mocks)) fn.mockReset(); + mocks.isTeamAdminOrKey.mockResolvedValue(true); + mocks.canManageTeamEsp.mockResolvedValue(true); }); describe("MCP ESP tools", () => { @@ -285,4 +293,36 @@ describe("MCP ESP tools", () => { }); expect(mocks.createEspConfig).not.toHaveBeenCalled(); }); + + it("keeps ESP changes from a team member, as REST does", async () => { + mocks.canManageTeamEsp.mockResolvedValue(false); + mocks.isTeamAdminOrKey.mockResolvedValue(false); + for (const [name, args] of [ + ["update_esp", { espId: "esp-1", host: "attacker.example" }], + ["update_esp_config", { host: "attacker.example" }], + ["test_esp", { espId: "esp-1", to: "me@example.com" }], + ["create_esp", { name: "x" }], + ["delete_esp", { espId: "esp-1", confirm: true }], + ["activate_esp", { espId: "esp-1" }], + ["retire_esp", { espId: "esp-1", confirm: true }], + ] as const) { + await expect(call(name, { ...args }, oauth)).resolves.toMatchObject({ + error: { code: "forbidden", safeDetails: { reason: "team_esp_disabled" } }, + }); + } + expect(mocks.updateEspConfig).not.toHaveBeenCalled(); + expect(mocks.testEspConfig).not.toHaveBeenCalled(); + expect(mocks.createEspConfig).not.toHaveBeenCalled(); + expect(mocks.canManageTeamEsp).toHaveBeenCalledWith({ + authKind: "oauth", + teamId: "team-1", + userId: "user-1", + }); + }); + + it("leaves reading ESPs open to any team member", async () => { + mocks.canManageTeamEsp.mockResolvedValue(false); + mocks.listEspConfigs.mockResolvedValue([]); + await expect(call("list_esps", {}, oauth)).resolves.not.toHaveProperty("error"); + }); }); diff --git a/apps/api/src/mcp/tools/settings-tools.test.ts b/apps/api/src/mcp/tools/settings-tools.test.ts index 98607658..a292d164 100644 --- a/apps/api/src/mcp/tools/settings-tools.test.ts +++ b/apps/api/src/mcp/tools/settings-tools.test.ts @@ -10,6 +10,8 @@ const mocks = { connectSnds: vi.fn(), syncTeamNow: vi.fn(), consumeRateLimit: vi.fn(), + isTeamAdminOrKey: vi.fn(), + canManageTeamEsp: vi.fn(), }; class FakeSndsLinkError extends Error {} @@ -20,6 +22,10 @@ mock.module("../../settings/tracking-domain/queries", () => ({ verifyTrackingDomain: mocks.verifyTrackingDomain, toPublicTrackingDomain: (domain: unknown) => domain, })); +mock.module("../../team/authority", () => ({ + isTeamAdminOrKey: mocks.isTeamAdminOrKey, + canManageTeamEsp: mocks.canManageTeamEsp, +})); mock.module("../../reputation/google", () => ({ startGoogleConnect: mocks.startGoogleConnect, })); @@ -55,6 +61,8 @@ const call = (name: string, args: Record = {}, ctx: any = conte beforeEach(() => { for (const fn of Object.values(mocks)) fn.mockReset(); + mocks.isTeamAdminOrKey.mockResolvedValue(true); + mocks.canManageTeamEsp.mockResolvedValue(true); }); describe("MCP tracking domain tools", () => { @@ -102,6 +110,32 @@ describe("MCP tracking domain tools", () => { }); describe("MCP reputation tools", () => { + it("keeps connecting and disconnecting accounts from a team member, as REST does", async () => { + mocks.isTeamAdminOrKey.mockResolvedValue(false); + const member = { + ...context, + authKind: "oauth" as const, + user: { id: "user-1" }, + scopes: ["settings:read", "settings:write"], + }; + for (const [name, args] of [ + ["connect_google_postmaster", {}], + ["disconnect_google_postmaster", { confirm: true }], + [ + "connect_snds", + { dataUrl: "https://sendersupport.olc.protection.outlook.com/x" }, + ], + ["disconnect_snds", { confirm: true }], + ] as const) { + await expect(call(name, { ...args }, member)).resolves.toMatchObject({ + error: { code: "forbidden", safeDetails: { reason: "team_admin_required" } }, + }); + } + expect(mocks.startGoogleConnect).not.toHaveBeenCalled(); + expect(mocks.connectSnds).not.toHaveBeenCalled(); + expect(mocks.deleteConnection).not.toHaveBeenCalled(); + }); + it("returns the Google sign-in URL, or says Postmaster isn't configured", async () => { mocks.startGoogleConnect.mockReturnValue("https://accounts.google.com/x"); await expect(call("connect_google_postmaster")).resolves.toEqual({ diff --git a/apps/api/src/organization/queries.test.ts b/apps/api/src/organization/queries.test.ts index fc8dae49..21070c2d 100644 --- a/apps/api/src/organization/queries.test.ts +++ b/apps/api/src/organization/queries.test.ts @@ -19,6 +19,7 @@ const { organizationEspUsageBuckets, organizations, outboundMessages, + teamApiKeys, sequences, teams, user, @@ -49,7 +50,11 @@ const { createOrganization, getOrganizationMembership, listOrganizationsForUser, + removeOrganizationMember, + updateOrganizationMemberRole, } = await import("./queries"); +const { createApiKey } = await import("../apikey/queries"); +const { ensureTeamMembership, getTeamMembership } = await import("../team/queries"); const tdb = db as unknown as TestDb; @@ -596,4 +601,81 @@ describe("one owned Free organization", () => { name: "Second", }); }); + + describe("losing access to the organization", () => { + async function adminWithTeamAndKey() { + const [owner, admin] = await tdb + .insert(user) + .values( + ["Owner", "Admin"].map((name) => ({ + id: crypto.randomUUID(), + name, + email: `${name}-${crypto.randomUUID()}@example.com`.toLowerCase(), + emailVerified: true, + createdAt: new Date(), + updatedAt: new Date(), + })), + ) + .returning(); + const organization = await createOrganization(owner.id, "Acme"); + await addOrganizationMemberByEmail(organization.id, admin.email, "admin"); + const team = await createTeam({ + organizationId: organization.id, + creatorUserId: admin.id, + name: "Marketing", + }); + const key = await createApiKey(team.id, "ci", { + createdByType: "user", + createdById: admin.id, + }); + return { owner, admin, organization, team, key }; + } + + it("removes a member's team access and the team keys they minted", async () => { + const { admin, organization, team, key } = await adminWithTeamAndKey(); + expect(await getTeamMembership(team.id, admin.id)).not.toBeNull(); + + await expect(removeOrganizationMember(organization.id, admin.id)).resolves.toBe( + true, + ); + + expect(await getTeamMembership(team.id, admin.id)).toBeNull(); + const [row] = await tdb + .select() + .from(teamApiKeys) + .where(eq(teamApiKeys.teamApiKeyId, key.apiKey.teamApiKeyId)); + expect(row.revokedAt).not.toBeNull(); + }); + + it("leaves other people's team access and keys alone", async () => { + const { owner, admin, organization, team } = await adminWithTeamAndKey(); + await ensureTeamMembership({ teamId: team.id, userId: owner.id, role: "admin" }); + const ownerKey = await createApiKey(team.id, "owner key", { + createdByType: "user", + createdById: owner.id, + }); + + await removeOrganizationMember(organization.id, admin.id); + + expect(await getTeamMembership(team.id, owner.id)).not.toBeNull(); + const [row] = await tdb + .select() + .from(teamApiKeys) + .where(eq(teamApiKeys.teamApiKeyId, ownerKey.apiKey.teamApiKeyId)); + expect(row.revokedAt).toBeNull(); + }); + + it("takes team access away when an admin is demoted to member, but not when promoted", async () => { + const { admin, organization, team } = await adminWithTeamAndKey(); + + await updateOrganizationMemberRole(organization.id, admin.id, "owner"); + expect(await getTeamMembership(team.id, admin.id)).not.toBeNull(); + + await updateOrganizationMemberRole(organization.id, admin.id, "admin"); + expect(await getTeamMembership(team.id, admin.id)).not.toBeNull(); + + await updateOrganizationMemberRole(organization.id, admin.id, "member"); + expect(await getTeamMembership(team.id, admin.id)).toBeNull(); + }); + }); }); diff --git a/apps/api/src/organization/queries.ts b/apps/api/src/organization/queries.ts index 8792f269..494ee3ad 100644 --- a/apps/api/src/organization/queries.ts +++ b/apps/api/src/organization/queries.ts @@ -14,6 +14,7 @@ import { organizations, settings, teamDeliverySettings, + teamApiKeys, teamMembers, teams, user, @@ -607,6 +608,42 @@ async function assertBillingManagerRetained( } } +/** + * Team membership is its own table, so leaving an organization (or losing + * admin rights in it) does not touch it. Without this the person keeps full + * access to every team of the organization, and the team API keys they minted + * keep working. Runs inside the caller's transaction. + */ +async function revokeOrganizationTeamAccess( + tx: Parameters[0]>[0], + organizationId: string, + userId: string, +): Promise { + const organizationTeams = tx + .select({ id: teams.id }) + .from(teams) + .where(eq(teams.organizationId, organizationId)); + await tx + .delete(teamMembers) + .where( + and( + eq(teamMembers.userId, userId), + inArray(teamMembers.teamId, organizationTeams), + ), + ); + await tx + .update(teamApiKeys) + .set({ revokedAt: new Date() }) + .where( + and( + eq(teamApiKeys.createdByType, "user"), + eq(teamApiKeys.createdById, userId), + isNull(teamApiKeys.revokedAt), + inArray(teamApiKeys.teamId, organizationTeams), + ), + ); +} + export async function updateOrganizationMemberRole( organizationId: string, userId: string, @@ -656,6 +693,10 @@ export async function updateOrganizationMemberRole( .update(organizationMembers) .set({ role, updatedAt: new Date() }) .where(eq(organizationMembers.id, membership.id)); + // Team access came with admin rights; a plain member does not keep it. + if (role === "member" && membership.role !== "member") { + await revokeOrganizationTeamAccess(tx, organizationId, userId); + } }); return getOrganizationMemberView(organizationId, userId); } @@ -691,6 +732,7 @@ export async function removeOrganizationMember( await tx .delete(organizationMembers) .where(eq(organizationMembers.id, membership.id)); + await revokeOrganizationTeamAccess(tx, organizationId, userId); return true; }); } diff --git a/apps/api/src/reputation/routes.ts b/apps/api/src/reputation/routes.ts index 3ea80e7e..3665572a 100644 --- a/apps/api/src/reputation/routes.ts +++ b/apps/api/src/reputation/routes.ts @@ -6,7 +6,7 @@ import { requireAuth } from "../auth/middleware"; import { requireTeam } from "../auth/require-team"; import { captureError } from "../observability"; import { consumeRateLimit } from "../services/rate-counter"; -import { getTeamMembership } from "../team/queries"; +import { isTeamAdminOrKey } from "../team/authority"; import { verifyPixelToken } from "../utils/pixel-jwt"; import { exchangeGoogleCode, @@ -25,11 +25,7 @@ const s = initServer(); /** Connecting accounts changes what the team shares with SendLit: team * admins and team API keys only. */ -async function canManage(req: any): Promise { - if (req.authKind === "team_key") return true; - const membership = await getTeamMembership(req.teamId, req.userId); - return membership?.role === "admin"; -} +const canManage = (req: any) => isTeamAdminOrKey(req); const FORBIDDEN = { status: 403 as const, body: { error: "team_admin_required" } }; diff --git a/apps/api/src/settings/esp/routes.ts b/apps/api/src/settings/esp/routes.ts index 6dbcc1ad..5ce3aec1 100644 --- a/apps/api/src/settings/esp/routes.ts +++ b/apps/api/src/settings/esp/routes.ts @@ -3,12 +3,11 @@ import { createExpressEndpoints, initServer } from "@ts-rest/express"; import { Router } from "express"; import { requireAuth } from "../../auth/middleware"; import { requireTeam } from "../../auth/require-team"; -import { getTeamDeliverySetting } from "../../delivery/queries"; import { dailySendCount } from "../../mail/rate-limit"; import { invalidateEspTransport, invalidateTeamTransport } from "../../mail/transport"; import { effectiveDailySendLimit, warmupState } from "../../mail/warmup"; import { captureEvent } from "../../observability"; -import { getTeamMembership } from "../../team/queries"; +import { canManageTeamEsp as canManageTeamEspFor } from "../../team/authority"; import { isEspDeleteBlocker } from "./delete-blockers"; import { checkAndRememberDomainAuthentication, @@ -27,13 +26,7 @@ router.use(["/settings/esp", "/settings/esps"], requireAuth, requireTeam); const s = initServer(); -async function canManageTeamEsp(req: any): Promise { - const delivery = await getTeamDeliverySetting(req.teamId); - if (!delivery?.teamEspEnabled) return false; - if (req.authKind === "team_key") return true; - const membership = await getTeamMembership(req.teamId, req.userId); - return membership?.role === "admin"; -} +const canManageTeamEsp = (req: any) => canManageTeamEspFor(req); function toPublicShape(config: EspConfig) { return { diff --git a/apps/api/src/team/authority.test.ts b/apps/api/src/team/authority.test.ts new file mode 100644 index 00000000..2bc13cee --- /dev/null +++ b/apps/api/src/team/authority.test.ts @@ -0,0 +1,47 @@ +import { beforeEach, describe, expect, it, mock, vi } from "bun:test"; + +const mocks = { + getTeamMembership: vi.fn(), + getTeamDeliverySetting: vi.fn(), +}; + +mock.module("./queries", () => ({ getTeamMembership: mocks.getTeamMembership })); +mock.module("../delivery/queries", () => ({ + getTeamDeliverySetting: mocks.getTeamDeliverySetting, +})); + +const { canManageTeamEsp, isTeamAdminOrKey } = await import("./authority"); + +const user = { authKind: "oauth", teamId: "team-1", userId: "user-1" }; + +beforeEach(() => { + for (const fn of Object.values(mocks)) fn.mockReset(); + mocks.getTeamDeliverySetting.mockResolvedValue({ teamEspEnabled: true }); +}); + +describe("team authority", () => { + it("lets team admins and team keys through, but not members or anonymous callers", async () => { + mocks.getTeamMembership.mockResolvedValue({ role: "admin" }); + expect(await isTeamAdminOrKey(user)).toBe(true); + mocks.getTeamMembership.mockResolvedValue({ role: "member" }); + expect(await isTeamAdminOrKey(user)).toBe(false); + mocks.getTeamMembership.mockResolvedValue(null); + expect(await isTeamAdminOrKey(user)).toBe(false); + expect(await isTeamAdminOrKey({ authKind: "oauth", teamId: "team-1" })).toBe(false); + expect(await isTeamAdminOrKey({ authKind: "team_key", teamId: "team-1" })).toBe( + true, + ); + }); + + it("also needs the organization to allow team ESPs", async () => { + mocks.getTeamMembership.mockResolvedValue({ role: "admin" }); + expect(await canManageTeamEsp(user)).toBe(true); + mocks.getTeamDeliverySetting.mockResolvedValue({ teamEspEnabled: false }); + expect(await canManageTeamEsp(user)).toBe(false); + expect(await canManageTeamEsp({ authKind: "team_key", teamId: "team-1" })).toBe( + false, + ); + mocks.getTeamDeliverySetting.mockResolvedValue(null); + expect(await canManageTeamEsp(user)).toBe(false); + }); +}); diff --git a/apps/api/src/team/authority.ts b/apps/api/src/team/authority.ts new file mode 100644 index 00000000..9972bb2a --- /dev/null +++ b/apps/api/src/team/authority.ts @@ -0,0 +1,29 @@ +/** The caller as the team-authority checks see it. REST requests and MCP + * contexts both reduce to this, so one rule governs both surfaces. */ +export type TeamCaller = { + authKind?: string; + teamId: string; + userId?: string | null; +}; + +/** Team admins and team API keys (a key is already the team's own + * credential). A plain member is not enough. */ +export async function isTeamAdminOrKey(caller: TeamCaller): Promise { + if (caller.authKind === "team_key") return true; + if (!caller.userId) return false; + // Loaded here, not at the top: every MCP tool imports the policy, and the + // team and delivery queries pull in the database and billing, which most + // tool tests don't need. + const { getTeamMembership } = await import("./queries"); + const membership = await getTeamMembership(caller.teamId, caller.userId); + return membership?.role === "admin"; +} + +/** Changing the team's own ESP: a team admin or key, and only while the + * organization lets the team manage its own ESP. */ +export async function canManageTeamEsp(caller: TeamCaller): Promise { + const { getTeamDeliverySetting } = await import("../delivery/queries"); + const delivery = await getTeamDeliverySetting(caller.teamId); + if (!delivery?.teamEspEnabled) return false; + return isTeamAdminOrKey(caller); +} diff --git a/apps/docs/content/docs/developers/authentication.mdx b/apps/docs/content/docs/developers/authentication.mdx index 66b63191..4507163b 100644 --- a/apps/docs/content/docs/developers/authentication.mdx +++ b/apps/docs/content/docs/developers/authentication.mdx @@ -66,6 +66,16 @@ falls back to another credential when one is invalid: MCP accepts an OAuth access token or a team API key; a browser session cookie is not an MCP credential and is ignored there. +OAuth access tokens are issued for the MCP server only. The REST API refuses +them with `401 invalid_token`, so a narrowly scoped MCP grant can never act with +the user's full authority over REST. Use a team API key for REST integrations. + +A session cookie only works from the SendLit dashboard. Any write +(`POST`, `PUT`, `PATCH`, `DELETE`) made with one must come from the dashboard's +own origin and carry the `X-Sendlit-CSRF` token, otherwise it returns +`403 csrf_origin_invalid` or `403 csrf_token_invalid`. API keys and bearer +tokens are not affected. + ## Dashboard sign-in The web dashboard uses email OTP sign-in. The session is separate from team API keys: use the dashboard session for interactive administration and an API key for server-to-server requests. diff --git a/apps/docs/content/docs/developers/mcp.mdx b/apps/docs/content/docs/developers/mcp.mdx index b3f50f09..cc80192e 100644 --- a/apps/docs/content/docs/developers/mcp.mdx +++ b/apps/docs/content/docs/developers/mcp.mdx @@ -73,6 +73,16 @@ OAuth access is default-deny: every tool needs the scope of the REST operation i - `delivery_events:read` - `suppressions:read`, `suppressions:write` +A scope says what a token may do; it does not change what the person behind it +may do in their team. Changing the team's ESP (`create_esp`, `update_esp`, +`update_esp_config`, `delete_esp`, `delete_esp_config`, `activate_esp`, `retire_esp`, `test_esp`, +`send_test_email`) and connecting or disconnecting Google Postmaster Tools or SNDS need a team +admin, exactly as in the dashboard and REST API. ESP changes also need +the organization to allow team ESPs. A team member with `esp:write` or +`settings:write` gets `forbidden` with reason `team_admin_required` or +`team_esp_disabled`. Team API keys count as the team's own credential and are +allowed. + ## Available tools ### Contacts and segments diff --git a/apps/web/app/api/proxy/[...path]/route.test.ts b/apps/web/app/api/proxy/[...path]/route.test.ts index ac8bf2ea..32b04e84 100644 --- a/apps/web/app/api/proxy/[...path]/route.test.ts +++ b/apps/web/app/api/proxy/[...path]/route.test.ts @@ -164,7 +164,7 @@ describe("BFF proxy auth failures", () => { const response = await POST( request("sendlit_team_id=team_123", { method: "POST", - headers: { "x-forwarded-for": "203.0.113.7" }, + headers: { "x-forwarded-for": "203.0.113.7", origin: "http://localhost" }, body: JSON.stringify({ email: "reader@example.com" }), }), params(["contacts"]), @@ -189,7 +189,10 @@ describe("BFF proxy auth failures", () => { const { DELETE } = await import("./route"); const response = await DELETE( - request("sendlit_team_id=team_123", { method: "DELETE" }), + request("sendlit_team_id=team_123", { + method: "DELETE", + headers: { origin: "http://localhost" }, + }), params(["contacts", "cnt_123"]), ); @@ -197,4 +200,67 @@ describe("BFF proxy auth failures", () => { expect(response.headers.get("content-type")).toBeNull(); expect(await response.text()).toBe(""); }); + + it("refuses a write that did not come from the dashboard's own origin", async () => { + const { POST } = await import("./route"); + + for (const headers of [ + { origin: "https://api.example.com" }, + { referer: "https://api.example.com/w/team/p/issue" }, + {}, + ]) { + const response = await POST( + request("sendlit_team_id=team_123", { + method: "POST", + headers, + body: JSON.stringify({ email: "reader@example.com" }), + }), + params(["contacts"]), + ); + expect(response.status).toBe(403); + expect(await response.json()).toEqual({ error: "csrf_origin_invalid" }); + } + expect(mocks.fetch).not.toHaveBeenCalled(); + }); + + it("attaches the CSRF token to same-origin writes", async () => { + mocks.fetch.mockResolvedValue(new Response("{}", { status: 200 })); + const { POST } = await import("./route"); + + await POST( + request("sendlit_csrf=abc123", { + method: "POST", + headers: { origin: "http://localhost" }, + body: "{}", + }), + params(["contacts"]), + ); + + expect(mocks.fetch).toHaveBeenCalledWith( + expect.any(String), + expect.objectContaining({ + headers: expect.objectContaining({ "X-Sendlit-CSRF": "abc123" }), + }), + ); + }); + + it("sandboxes every response so tenant HTML cannot run on the dashboard origin", async () => { + mocks.fetch.mockResolvedValue( + new Response("", { + status: 200, + headers: { + "Content-Type": "text/html; charset=utf-8", + "Content-Security-Policy": "sandbox allow-popups", + }, + }), + ); + const { GET } = await import("./route"); + + const response = await GET(request(), params(["view", "token"])); + + expect(response.headers.get("content-security-policy")).toBe( + "default-src 'none'; sandbox", + ); + expect(response.headers.get("x-content-type-options")).toBe("nosniff"); + }); }); diff --git a/apps/web/app/api/proxy/[...path]/route.ts b/apps/web/app/api/proxy/[...path]/route.ts index fa82d49c..8aa2e766 100644 --- a/apps/web/app/api/proxy/[...path]/route.ts +++ b/apps/web/app/api/proxy/[...path]/route.ts @@ -3,6 +3,20 @@ import { type NextRequest, NextResponse } from "next/server"; import { API_URL } from "@/lib/config"; import { TEAM_ID_COOKIE } from "@/lib/tokens"; +/** True when the browser says the request came from this dashboard's own + * origin. Browsers always send `Origin` on a non-GET fetch or form POST. */ +function isSameOriginRequest(req: NextRequest): boolean { + const host = + req.headers.get("x-forwarded-host") ?? req.headers.get("host") ?? req.nextUrl.host; + const source = req.headers.get("origin") ?? req.headers.get("referer"); + if (!source) return false; + try { + return new URL(source).host === host; + } catch { + return false; + } +} + /** * Same-origin dashboard proxy. Browser requests carry only httpOnly session * cookies; the API resolves those cookies and enforces team membership. No @@ -13,10 +27,17 @@ async function forward( { params }: { params: Promise<{ path: string[] }> }, ) { const { path } = await params; - const cookieHeader = req.headers.get("cookie"); - const targetUrl = `${API_URL}/${path.join("/")}${req.nextUrl.search}`; const method = req.method; const isMutation = ["POST", "PUT", "PATCH", "DELETE"].includes(method); + // The API trusts the session cookie plus a double-submit token for writes. + // The proxy attaches that token itself, so it must first prove the request + // came from this dashboard and not from another origin (a sibling site, or + // a page a tenant published on the API host). + if (isMutation && !isSameOriginRequest(req)) { + return NextResponse.json({ error: "csrf_origin_invalid" }, { status: 403 }); + } + const cookieHeader = req.headers.get("cookie"); + const targetUrl = `${API_URL}/${path.join("/")}${req.nextUrl.search}`; // Establish the readable double-submit cookie on the first same-origin // request. It is not forwarded upstream on reads, preserving the proxy's // existing cookie/privacy behavior; mutation requests forward it. @@ -51,8 +72,8 @@ async function forward( ...(req.headers.get("origin") ? { Origin: req.headers.get("origin")! } : {}), ...(req.headers.get("referer") ? { Referer: req.headers.get("referer")! } : {}), ...(forwardedCookie ? { Cookie: forwardedCookie } : {}), - ...(req.headers.get("x-sendlit-csrf") - ? { "X-Sendlit-CSRF": req.headers.get("x-sendlit-csrf")! } + ...(isMutation + ? { "X-Sendlit-CSRF": req.headers.get("x-sendlit-csrf") || csrfToken } : {}), ...(req.headers.get("x-sendlit-billing-action-token") ? { @@ -78,6 +99,12 @@ async function forward( }, }); + // Nothing the API returns through here is a page to render. Tenant-authored + // HTML (web views, archives) must never run on the dashboard origin, so + // sandbox every response and stop the browser guessing a type. + res.headers.set("Content-Security-Policy", "default-src 'none'; sandbox"); + res.headers.set("X-Content-Type-Options", "nosniff"); + if (upstream.status === 401) { res.headers.set("X-Auth-Error", "session_expired"); } From afa416f44df6895b7fb8498d7cc5162efe1cb0ff Mon Sep 17 00:00:00 2001 From: Rajat Date: Tue, 3 Nov 2026 14:05:35 +0530 Subject: [PATCH 2/3] fix(web): type the proxy CSRF test headers Co-Authored-By: Claude Sonnet 5.5 --- apps/web/app/api/proxy/[...path]/route.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/apps/web/app/api/proxy/[...path]/route.test.ts b/apps/web/app/api/proxy/[...path]/route.test.ts index 32b04e84..e2561412 100644 --- a/apps/web/app/api/proxy/[...path]/route.test.ts +++ b/apps/web/app/api/proxy/[...path]/route.test.ts @@ -204,11 +204,12 @@ describe("BFF proxy auth failures", () => { it("refuses a write that did not come from the dashboard's own origin", async () => { const { POST } = await import("./route"); - for (const headers of [ + const bad: Record[] = [ { origin: "https://api.example.com" }, { referer: "https://api.example.com/w/team/p/issue" }, {}, - ]) { + ]; + for (const headers of bad) { const response = await POST( request("sendlit_team_id=team_123", { method: "POST", From a08ed77085ff165bc41c47e26d5e3d8a28014198 Mon Sep 17 00:00:00 2001 From: Rajat Date: Tue, 3 Nov 2026 16:41:03 +0530 Subject: [PATCH 3/3] style: sort imports (biome) Co-Authored-By: Claude Sonnet 5.5 --- apps/api/src/auth/middleware.ts | 2 +- apps/api/src/mcp/policy.ts | 2 +- apps/api/src/organization/queries.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/apps/api/src/auth/middleware.ts b/apps/api/src/auth/middleware.ts index 9217e425..8c83b42f 100644 --- a/apps/api/src/auth/middleware.ts +++ b/apps/api/src/auth/middleware.ts @@ -1,6 +1,6 @@ import type { NextFunction, Request, Response } from "express"; -import { dashboardSessionBoundary } from "./csrf"; import { mcpProtectedResourceMetadataUrl, mcpResourceUrl } from "./better-auth"; +import { dashboardSessionBoundary } from "./csrf"; import { type AuthInput, type AuthResult, diff --git a/apps/api/src/mcp/policy.ts b/apps/api/src/mcp/policy.ts index 876cec87..e2bc1cbb 100644 --- a/apps/api/src/mcp/policy.ts +++ b/apps/api/src/mcp/policy.ts @@ -1,6 +1,6 @@ import { createPlatformError, type PlatformError } from "@codelitdev/platform"; -import type { SendLitMcpContext } from "./context"; import { canManageTeamEsp, isTeamAdminOrKey } from "../team/authority"; +import type { SendLitMcpContext } from "./context"; export const MCP_SCOPES = { contactsRead: "contacts:read", diff --git a/apps/api/src/organization/queries.ts b/apps/api/src/organization/queries.ts index 494ee3ad..165e80ee 100644 --- a/apps/api/src/organization/queries.ts +++ b/apps/api/src/organization/queries.ts @@ -13,8 +13,8 @@ import { organizationMembers, organizations, settings, - teamDeliverySettings, teamApiKeys, + teamDeliverySettings, teamMembers, teams, user,