diff --git a/apps/api/src/people/people-invite.service.spec.ts b/apps/api/src/people/people-invite.service.spec.ts index f8fc9ce542..82473b62ae 100644 --- a/apps/api/src/people/people-invite.service.spec.ts +++ b/apps/api/src/people/people-invite.service.spec.ts @@ -19,6 +19,7 @@ jest.mock('@db', () => ({ user: { findFirst: jest.fn(), create: jest.fn(), + update: jest.fn(), }, member: { findFirst: jest.fn(), @@ -324,6 +325,125 @@ describe('PeopleInviteService', () => { ); }); + it('creates invited employees with a verified email so trusted SSO providers can link', async () => { + (mockDb.organization.findUnique as jest.Mock).mockResolvedValue({ + name: 'Test Org', + }); + (mockDb.user.findFirst as jest.Mock).mockResolvedValue(null); + (mockDb.user.create as jest.Mock).mockResolvedValue({ + id: 'user_new', + email: 'emp@example.com', + }); + (mockDb.member.findFirst as jest.Mock).mockResolvedValue(null); + (mockDb.member.create as jest.Mock).mockResolvedValue({ + id: 'member_new', + }); + ( + mockDb.employeeTrainingVideoCompletion.createMany as jest.Mock + ).mockResolvedValue({ + count: 5, + }); + + const results = await service.inviteMembers({ + ...baseParams, + invites: [{ email: 'emp@example.com', roles: ['employee'] }], + }); + + expect(results[0].success).toBe(true); + // An unverified local user makes better-auth refuse to link trusted + // OAuth providers (account_not_linked), stranding invited employees at + // the portal sign-in page, so new users must be created verified. + expect(mockDb.user.create).toHaveBeenCalledWith({ + data: expect.objectContaining({ emailVerified: true }), + }); + }); + + it('upgrades a legacy unverified user to verified when re-invited as an employee', async () => { + (mockDb.organization.findUnique as jest.Mock).mockResolvedValue({ + name: 'Test Org', + }); + // Legacy row: created before employees were created verified, and no + // longer a member anywhere, so the one-time member backfill missed it. + (mockDb.user.findFirst as jest.Mock).mockResolvedValue({ + id: 'user_legacy', + email: 'emp@example.com', + emailVerified: false, + }); + (mockDb.member.findFirst as jest.Mock).mockResolvedValue(null); + (mockDb.member.create as jest.Mock).mockResolvedValue({ + id: 'member_new', + }); + ( + mockDb.employeeTrainingVideoCompletion.createMany as jest.Mock + ).mockResolvedValue({ count: 5 }); + + const results = await service.inviteMembers({ + ...baseParams, + invites: [{ email: 'emp@example.com', roles: ['employee'] }], + }); + + expect(results[0].success).toBe(true); + expect(mockDb.user.update).toHaveBeenCalledWith({ + where: { id: 'user_legacy' }, + data: { emailVerified: true }, + }); + }); + + it('does not touch an already-verified user when re-invited', async () => { + (mockDb.organization.findUnique as jest.Mock).mockResolvedValue({ + name: 'Test Org', + }); + (mockDb.user.findFirst as jest.Mock).mockResolvedValue({ + id: 'user_verified', + email: 'emp@example.com', + emailVerified: true, + }); + (mockDb.member.findFirst as jest.Mock).mockResolvedValue(null); + (mockDb.member.create as jest.Mock).mockResolvedValue({ + id: 'member_new', + }); + ( + mockDb.employeeTrainingVideoCompletion.createMany as jest.Mock + ).mockResolvedValue({ count: 5 }); + + const results = await service.inviteMembers({ + ...baseParams, + invites: [{ email: 'emp@example.com', roles: ['employee'] }], + }); + + expect(results[0].success).toBe(true); + expect(mockDb.user.update).not.toHaveBeenCalled(); + }); + + it('upgrades a legacy unverified user to verified when invited to an admin role', async () => { + (mockDb.user.findFirst as jest.Mock).mockResolvedValue({ + id: 'user_legacy', + email: 'admin@example.com', + emailVerified: false, + }); + (mockDb.member.findFirst as jest.Mock).mockResolvedValue(null); + (mockDb.organization.findUnique as jest.Mock).mockResolvedValue({ + name: 'Test Org', + }); + (mockDb.invitation.create as jest.Mock).mockResolvedValue({ + id: 'inv_new', + }); + + const results = await service.inviteMembers({ + ...baseParams, + invites: [{ email: 'admin@example.com', roles: ['admin'] }], + }); + + expect(results[0].success).toBe(true); + expect(mockDb.invitation.create).toHaveBeenCalled(); + // The invitee signs in to accept; an unverified row would block linking + // a Google/Microsoft sign-in to it (account_not_linked). + expect(mockDb.user.update).toHaveBeenCalledWith({ + where: { id: 'user_legacy' }, + data: { emailVerified: true }, + }); + }); + it('should reactivate deactivated members', async () => { (mockDb.organization.findUnique as jest.Mock).mockResolvedValue({ name: 'Test Org', diff --git a/apps/api/src/people/people-invite.service.ts b/apps/api/src/people/people-invite.service.ts index ef5feab296..37ce027843 100644 --- a/apps/api/src/people/people-invite.service.ts +++ b/apps/api/src/people/people-invite.service.ts @@ -170,10 +170,20 @@ export class PeopleInviteService { }); if (!existingUser) { + // Mark the email verified up front: the address is admin-provided, and + // every sign-in method (OTP, magic link, trusted OAuth) proves mailbox + // ownership anyway. An unverified user row makes better-auth refuse to + // link Google/Microsoft sign-ins to it (account_not_linked), which + // strands invited employees at the portal sign-in page. const newUser = await db.user.create({ - data: { emailVerified: false, email, name: email.split('@')[0] }, + data: { emailVerified: true, email, name: email.split('@')[0] }, }); userId = newUser.id; + } else { + // Legacy rows may predate the created-verified behavior (and the member + // backfill only covered users who were members at the time), so upgrade + // on re-invite too. + await this.ensureEmailVerified(existingUser); } const finalUserId = existingUser?.id ?? userId; @@ -250,6 +260,29 @@ export class PeopleInviteService { return { emailSent }; } + /** + * Upgrade a legacy unverified user row to verified when (re-)inviting them. + * + * Invited addresses are admin-provided, and every sign-in method (email OTP, + * magic link, trusted OAuth) proves mailbox ownership before a session is + * issued, so the flag grants nothing to anyone who cannot already receive + * mail at the address. Without it, better-auth refuses to link a + * Google/Microsoft sign-in to the existing row (account_not_linked). No-op + * when already verified. + */ + private async ensureEmailVerified(user: { + id: string; + emailVerified: boolean; + }): Promise { + if (user.emailVerified) { + return; + } + await db.user.update({ + where: { id: user.id }, + data: { emailVerified: true }, + }); + } + private async inviteWithCheck(params: { email: string; roles: string[]; @@ -272,6 +305,11 @@ export class PeopleInviteService { }); if (existingUser) { + // Same rationale as the employee path: an unverified legacy row would + // keep blocking Google/Microsoft sign-in linking (account_not_linked) + // when this invitee goes to accept. + await this.ensureEmailVerified(existingUser); + const existingMember = await db.member.findFirst({ where: { userId: existingUser.id, organizationId }, }); diff --git a/apps/app/src/actions/people/create-employee-action.ts b/apps/app/src/actions/people/create-employee-action.ts deleted file mode 100644 index 234a4709c6..0000000000 --- a/apps/app/src/actions/people/create-employee-action.ts +++ /dev/null @@ -1,57 +0,0 @@ -'use server'; - -import { completeEmployeeCreation } from '@/lib/db/employee'; -import { Prisma } from '@db/server'; -import { authActionClient } from '../safe-action'; -import { createEmployeeSchema } from '../schema'; -import type { ActionResponse } from '../types'; - -export const createEmployeeAction = authActionClient - .inputSchema(createEmployeeSchema) - .metadata({ - name: 'create-employee', - track: { - event: 'create-employee', - channel: 'server', - }, - }) - .action(async ({ parsedInput, ctx }): Promise => { - const { name, email, department, externalEmployeeId } = parsedInput; - const { user, session } = ctx; - - if (!session.activeOrganizationId) { - return { - success: false, - error: 'Not authorized - no organization found', - }; - } - - try { - const employee = await completeEmployeeCreation({ - name, - email, - department, - organizationId: session.activeOrganizationId, - externalEmployeeId, - }); - - return { - success: true, - data: employee, - }; - } catch (error) { - console.error('Error creating employee:', error); - - if (error instanceof Prisma.PrismaClientKnownRequestError && error.code === 'P2002') { - return { - success: false, - error: 'An employee with this email already exists in your organization', - }; - } - - return { - success: false, - error: error instanceof Error ? error.message : 'Failed to create employee', - }; - } - }); diff --git a/apps/app/src/actions/schema.ts b/apps/app/src/actions/schema.ts index 5cc2ab706d..46a7f86427 100644 --- a/apps/app/src/actions/schema.ts +++ b/apps/app/src/actions/schema.ts @@ -1,6 +1,5 @@ import { CommentEntityType, - Departments, Frequency, Impact, Likelihood, @@ -283,14 +282,6 @@ export const assistantSettingsSchema = z.object({ enabled: z.boolean().optional(), }); -export const createEmployeeSchema = z.object({ - name: z.string().min(1, 'Name is required'), - email: z.string().email('Invalid email address'), - department: z.nativeEnum(Departments, { error: 'Department is required' }), - externalEmployeeId: z.string().optional(), - isActive: z.boolean().default(true), -}); - export const updatePolicyOverviewSchema = z.object({ id: z.string(), title: z.string(), diff --git a/apps/app/src/lib/db/employee.ts b/apps/app/src/lib/db/employee.ts index a548ac2897..5ea2b319b7 100644 --- a/apps/app/src/lib/db/employee.ts +++ b/apps/app/src/lib/db/employee.ts @@ -1,67 +1,5 @@ -import { env } from '@/env.mjs'; import { trainingVideos } from '@/lib/data/training-videos'; -import { db, type Departments, type Member, type Role } from '@db/server'; -import { revalidatePath } from 'next/cache'; - -if (!env.NEXT_PUBLIC_PORTAL_URL) { - throw new Error('NEXT_PUBLIC_PORTAL_URL is not set'); -} - -/** - * Complete employee creation by handling all steps: - * 1. Create/reactivate the employee - * 2. Create/update portal user - * 3. Create training video entries - */ -export async function completeEmployeeCreation(params: { - name: string; - email: string; - department: Departments; - organizationId: string; - externalEmployeeId?: string; -}): Promise { - const { name, email, department, organizationId } = params; - - // Check if the user already exists - const existingUser = await db.user.findFirst({ - where: { - email: { - equals: email, - mode: 'insensitive', - }, - }, - }); - - let employee: Member; - let userId: string; - - if (existingUser) { - // Handle existing user flow - employee = await handleExistingUser({ - userId: existingUser.id, - organizationId, - department, - }); - userId = existingUser.id; - } else { - // Handle new user flow - employee = await createNewUser({ - name, - email, - organizationId, - department, - }); - userId = employee.userId; - } - - // Create training video entries for the employee - await createTrainingVideoEntries(employee.id); - - // Revalidate relevant paths to update UI - revalidatePath(`/${organizationId}/people`); - - return employee; -} +import { db } from '@db/server'; /** * Creates training video tracking entries for a new member @@ -79,121 +17,3 @@ export async function createTrainingVideoEntries(memberId: string) { return result; } - -/** - * Handle the flow for an existing user - */ -async function handleExistingUser({ - userId, - organizationId, - department, -}: { - userId: string; - organizationId: string; - department: Departments; -}): Promise { - // Check if user is already a member of the organization - const existingMember = await db.member.findFirst({ - where: { - userId, - organizationId, - }, - }); - - if (!existingMember) { - // Create a new member record if they're not already in the organization - const newMember = await db.member.create({ - data: { - userId, - organizationId, - role: 'employee', - department, - isActive: true, - }, - }); - return newMember; - } - - // User is already a member, check if they have any role - const existingMemberRoles = existingMember.role.split(',') as ( - | 'admin' - | 'auditor' - | 'employee' - | 'owner' - )[]; - - // If they already have any role, we can't add them as an employee - if (existingMemberRoles.length > 0) { - throw new Error( - `User already has role(s): ${existingMemberRoles.join(', ')}. Each person can only have one role.`, - ); - } - - // If they have no role (this shouldn't happen but just in case), assign employee role - // Instead of using auth.api, update the member record directly - try { - const updatedMember = await db.member.update({ - where: { - id: existingMember.id, - }, - data: { - role: 'employee' as Role, - department, - isActive: true, - }, - }); - - if (!updatedMember) { - console.error('[EXISTING_USER] Failed to update member role - no member returned'); - throw new Error('Failed to update member role'); - } - - return updatedMember; - } catch (dbError) { - console.error('[EXISTING_USER] Database error when updating member role:', dbError); - throw new Error( - `Failed to update member role: ${dbError instanceof Error ? dbError.message : String(dbError)}`, - ); - } -} - -/** - * Create a new user and add them as a member - */ -async function createNewUser({ - name, - email, - organizationId, - department, -}: { - name: string; - email: string; - organizationId: string; - department: Departments; -}): Promise { - // Create a skeleton user - const user = await db.user.create({ - data: { - name, - email, - emailVerified: false, - }, - }); - - // Add them as a member to the organization with the employee role - const newMember = await db.member.create({ - data: { - userId: user.id, - organizationId, - role: 'employee', - department, - isActive: true, - }, - }); - - if (!newMember) { - throw new Error('Failed to add employee to organization'); - } - - return newMember; -} diff --git a/apps/portal/package.json b/apps/portal/package.json index 0152ad730a..00a20c4cf6 100644 --- a/apps/portal/package.json +++ b/apps/portal/package.json @@ -44,9 +44,11 @@ "@types/node": "^24.0.3", "eslint": "^9.18.0", "eslint-config-next": "15.5.2", + "jsdom": "^26.1.0", "postcss": "^8.5.4", "tailwindcss": "^4.1.8", - "typescript": "^5.8.3" + "typescript": "^5.8.3", + "vitest": "^3.2.7" }, "peerDependencies": { "react": "^19.1.1", @@ -62,6 +64,7 @@ "dev": "next dev --turbopack -p 3002", "lint": "eslint . && prettier --check .", "prebuild": "bun run db:generate", - "start": "next start" + "start": "next start", + "test": "vitest run" } } diff --git a/apps/portal/src/app/components/google-sign-in.tsx b/apps/portal/src/app/components/google-sign-in.tsx index 9434c17ee4..5018f7696b 100644 --- a/apps/portal/src/app/components/google-sign-in.tsx +++ b/apps/portal/src/app/components/google-sign-in.tsx @@ -1,6 +1,7 @@ 'use client'; import { authClient } from '@/app/lib/auth-client'; +import { buildSignInCallbackUrls } from '@/app/lib/auth-callback'; import { Button } from '@trycompai/ui/button'; import { Icons } from '@trycompai/ui/icons'; import { Spinner } from '@trycompai/design-system'; @@ -18,26 +19,18 @@ export function GoogleSignIn({ const handleSignIn = async () => { setLoading(true); - // Build the callback URL with search params - const baseURL = window.location.origin; - const isDeviceAuth = searchParams?.get('device_auth') === 'true'; - const path = isDeviceAuth - ? '/auth/device-callback' - : inviteCode - ? `/invite/${inviteCode}` - : '/'; - const redirectTo = new URL(path, baseURL); - - // Append all search params if they exist - if (searchParams) { - searchParams.forEach((value, key) => { - redirectTo.searchParams.append(key, value); - }); - } + const { callbackURL, errorCallbackURL } = buildSignInCallbackUrls({ + origin: window.location.origin, + inviteCode, + searchParams, + }); await authClient.signIn.social({ provider: 'google', - callbackURL: redirectTo.toString(), + callbackURL, + // Without this, an OAuth callback error redirects to the API root + // (Swagger docs) instead of back to the portal. See CS-760. + errorCallbackURL, }); }; diff --git a/apps/portal/src/app/components/microsoft-sign-in.tsx b/apps/portal/src/app/components/microsoft-sign-in.tsx index 24339ef06d..49b524589a 100644 --- a/apps/portal/src/app/components/microsoft-sign-in.tsx +++ b/apps/portal/src/app/components/microsoft-sign-in.tsx @@ -1,6 +1,7 @@ 'use client'; import { authClient } from '@/app/lib/auth-client'; +import { buildSignInCallbackUrls } from '@/app/lib/auth-callback'; import { Button } from '@trycompai/ui/button'; import { Icons } from '@trycompai/ui/icons'; import { Spinner } from '@trycompai/design-system'; @@ -20,26 +21,18 @@ export function MicrosoftSignIn({ setLoading(true); try { - // Build the callback URL with search params - const baseURL = window.location.origin; - const isDeviceAuth = searchParams?.get('device_auth') === 'true'; - const path = isDeviceAuth - ? '/auth/device-callback' - : inviteCode - ? `/invite/${inviteCode}` - : '/'; - const redirectTo = new URL(path, baseURL); - - // Append all search params if they exist - if (searchParams) { - searchParams.forEach((value, key) => { - redirectTo.searchParams.append(key, value); - }); - } + const { callbackURL, errorCallbackURL } = buildSignInCallbackUrls({ + origin: window.location.origin, + inviteCode, + searchParams, + }); await authClient.signIn.social({ provider: 'microsoft', - callbackURL: redirectTo.toString(), + callbackURL, + // Without this, an OAuth callback error redirects to the API root + // (Swagger docs) instead of back to the portal. See CS-760. + errorCallbackURL, }); } catch (error) { setLoading(false); diff --git a/apps/portal/src/app/lib/auth-callback.test.ts b/apps/portal/src/app/lib/auth-callback.test.ts new file mode 100644 index 0000000000..65d7391cde --- /dev/null +++ b/apps/portal/src/app/lib/auth-callback.test.ts @@ -0,0 +1,66 @@ +import { describe, expect, it } from 'vitest'; +import { buildSignInCallbackUrls } from './auth-callback'; + +const PORTAL_ORIGIN = 'https://portal.trycomp.ai'; +const API_ORIGIN = 'https://api.trycomp.ai'; + +describe('buildSignInCallbackUrls', () => { + // CS-760: a new Microsoft/Entra user accepts a portal invite; the OAuth + // callback errors and better-auth redirects to the errorCallbackURL. If that + // URL is missing, better-auth defaults to the API baseURL and the user lands + // on the Swagger docs. The error URL MUST be rooted at the portal, not the API. + it('returns an errorCallbackURL rooted at the portal, not the API', () => { + const { errorCallbackURL } = buildSignInCallbackUrls({ + origin: PORTAL_ORIGIN, + inviteCode: 'invite_test123', + }); + + const url = new URL(errorCallbackURL); + expect(url.origin).toBe(PORTAL_ORIGIN); + expect(url.origin).not.toBe(API_ORIGIN); + expect(url.pathname).toBe('/auth'); + }); + + it('sends the success path to the invite page when an invite code is present', () => { + const { callbackURL, errorCallbackURL } = buildSignInCallbackUrls({ + origin: PORTAL_ORIGIN, + inviteCode: 'org_abc123', + }); + + expect(callbackURL).toBe(`${PORTAL_ORIGIN}/invite/org_abc123`); + // Success goes to the invite; error still returns to the portal sign-in. + expect(errorCallbackURL).toBe(`${PORTAL_ORIGIN}/auth`); + }); + + it('routes device-auth sign-ins to the device callback and preserves params on both URLs', () => { + const searchParams = new URLSearchParams({ + device_auth: 'true', + callback_port: '54321', + state: 'xyz', + }); + + const { callbackURL, errorCallbackURL } = buildSignInCallbackUrls({ + origin: PORTAL_ORIGIN, + searchParams, + }); + + const success = new URL(callbackURL); + expect(success.pathname).toBe('/auth/device-callback'); + expect(success.searchParams.get('callback_port')).toBe('54321'); + expect(success.searchParams.get('state')).toBe('xyz'); + + const error = new URL(errorCallbackURL); + expect(error.origin).toBe(PORTAL_ORIGIN); + expect(error.pathname).toBe('/auth'); + expect(error.searchParams.get('callback_port')).toBe('54321'); + }); + + it('defaults the success path to root when there is no invite or device auth', () => { + const { callbackURL, errorCallbackURL } = buildSignInCallbackUrls({ + origin: PORTAL_ORIGIN, + }); + + expect(callbackURL).toBe(`${PORTAL_ORIGIN}/`); + expect(errorCallbackURL).toBe(`${PORTAL_ORIGIN}/auth`); + }); +}); diff --git a/apps/portal/src/app/lib/auth-callback.ts b/apps/portal/src/app/lib/auth-callback.ts new file mode 100644 index 0000000000..aa4ce7441f --- /dev/null +++ b/apps/portal/src/app/lib/auth-callback.ts @@ -0,0 +1,49 @@ +export interface SignInCallbackUrls { + /** Where better-auth sends the user after a successful OAuth sign-in. */ + callbackURL: string; + /** Where better-auth sends the user when the OAuth callback errors. */ + errorCallbackURL: string; +} + +/** + * Build the success + error callback URLs for a portal OAuth (Google/Microsoft) + * sign-in. + * + * Both URLs are absolute and rooted at the PORTAL origin. Passing an + * `errorCallbackURL` is essential: without it, better-auth falls back to a + * default error URL rooted at the API's baseURL (api.trycomp.ai) when the OAuth + * callback errors, which lands the user on the Swagger API docs page instead of + * the portal (see redirectOnError/parseState in better-auth and CS-760). This is + * most visible for brand-new Microsoft/Entra work accounts, whose ID token can + * miss claims and trip the error path. Sending the error path back to the + * portal's /auth page lets the user retry or fall back to the magic-link login. + */ +export function buildSignInCallbackUrls({ + origin, + inviteCode, + searchParams, +}: { + origin: string; + inviteCode?: string; + searchParams?: URLSearchParams; +}): SignInCallbackUrls { + const isDeviceAuth = searchParams?.get('device_auth') === 'true'; + const successPath = isDeviceAuth + ? '/auth/device-callback' + : inviteCode + ? `/invite/${inviteCode}` + : '/'; + + return { + callbackURL: withSearchParams(new URL(successPath, origin), searchParams).toString(), + // On error, return the user to the portal sign-in page (never the API). + errorCallbackURL: withSearchParams(new URL('/auth', origin), searchParams).toString(), + }; +} + +function withSearchParams(url: URL, searchParams?: URLSearchParams): URL { + searchParams?.forEach((value, key) => { + url.searchParams.append(key, value); + }); + return url; +} diff --git a/apps/portal/vitest.config.ts b/apps/portal/vitest.config.ts new file mode 100644 index 0000000000..1102a058a0 --- /dev/null +++ b/apps/portal/vitest.config.ts @@ -0,0 +1,24 @@ +import { resolve } from 'path'; +import { defineConfig } from 'vitest/config'; + +// No vite plugins on purpose: esbuild already transforms TSX with the automatic +// JSX runtime, and plugin types (e.g. @vitejs/plugin-react, vite-tsconfig-paths) +// can clash with vitest's bundled vite during Next's build-time typecheck on +// Vercel. Aliases are mirrored by hand from tsconfig instead — see +// apps/framework-editor/vitest.config.ts for the same approach. +export default defineConfig({ + test: { + // jsdom, not node: the include glob covers component tests (.jsx/.tsx) that + // need DOM APIs (e.g. testing-library's render). Matches apps/app and + // apps/framework-editor. Node-only tests run fine under jsdom too. + environment: 'jsdom', + include: ['src/**/*.{test,spec}.{js,mjs,cjs,ts,mts,cts,jsx,tsx}'], + exclude: ['node_modules', 'dist', '.next'], + }, + resolve: { + // Mirror the tsconfig `@/*` path alias so tests can import via `@/...`. + alias: { + '@': resolve(__dirname, './src'), + }, + }, +}); diff --git a/bun.lock b/bun.lock index 9428056d21..e95936b8d4 100644 --- a/bun.lock +++ b/bun.lock @@ -487,9 +487,11 @@ "@types/node": "^24.0.3", "eslint": "^9.18.0", "eslint-config-next": "15.5.2", + "jsdom": "^26.1.0", "postcss": "^8.5.4", "tailwindcss": "^4.1.8", "typescript": "^5.8.3", + "vitest": "^3.2.7", }, "peerDependencies": { "react": "^19.1.1", diff --git a/packages/db/prisma/migrations/20260720193500_backfill_member_email_verified/migration.sql b/packages/db/prisma/migrations/20260720193500_backfill_member_email_verified/migration.sql new file mode 100644 index 0000000000..116c745c1c --- /dev/null +++ b/packages/db/prisma/migrations/20260720193500_backfill_member_email_verified/migration.sql @@ -0,0 +1,18 @@ +-- Invited employees (people-invite / add-employee flows) were created with +-- "emailVerified" = false. better-auth refuses to link a trusted OAuth provider +-- (Google/Microsoft) sign-in to an existing unverified user (account_not_linked), +-- so those employees could not sign in with SSO. New employee rows are now +-- created with "emailVerified" = true; this backfills existing org members so +-- they can sign in too. +-- +-- Safe because password auth is disabled: every sign-in method (email OTP, +-- magic link, trusted OAuth) proves control of the mailbox before a session is +-- issued, so the flag grants nothing to anyone who cannot already receive mail +-- at the address. +UPDATE "User" +SET "emailVerified" = true, + "updatedAt" = NOW() +WHERE "emailVerified" = false + AND EXISTS ( + SELECT 1 FROM "Member" m WHERE m."userId" = "User".id + ); diff --git a/packages/integration-platform/src/manifests/github/checks/__tests__/code-scanning.test.ts b/packages/integration-platform/src/manifests/github/checks/__tests__/code-scanning.test.ts index 9eb7be46cf..f265de9545 100644 --- a/packages/integration-platform/src/manifests/github/checks/__tests__/code-scanning.test.ts +++ b/packages/integration-platform/src/manifests/github/checks/__tests__/code-scanning.test.ts @@ -3,16 +3,33 @@ import type { CheckContext } from '../../../../types'; import type { GitHubRepo } from '../../types'; import { codeScanningCheck } from '../code-scanning'; +// GitHub's real 403 bodies for the code-scanning API. +const FEATURE_OFF_BODY = + '{"message":"Code Security must be enabled for this repository to use code scanning.","status":"403"}'; +const NOT_ACCESSIBLE_BODY = '{"message":"Resource not accessible by integration","status":"403"}'; + +type SecurityStatus = 'enabled' | 'disabled'; + interface RepoConfig { private: boolean; /** - * `advanced_security.status`. `undefined` means GitHub omitted the - * `security_and_analysis` block entirely — which is what happens over an OAuth - * connection whose user lacks repo-admin visibility. + * `advanced_security.status` (the legacy field, still sent by GitHub Enterprise + * Server / older payloads). `undefined` means GitHub omitted the whole + * `security_and_analysis` block — what happens over a connection without + * repo-admin visibility. + */ + ghas?: SecurityStatus; + /** + * `code_security.status` — GitHub's 2026 Code Security GA renamed the + * code-scanning entitlement to this. Newer payloads carry it instead of `ghas`. */ - ghas?: 'enabled' | 'disabled'; - /** Whether the code-scanning default-setup API 403s (permission/GHAS gate). */ - defaultSetup403?: boolean; + codeSecurity?: SecurityStatus; + /** When set, the code-scanning default-setup API 403s with this body. */ + defaultSetup403Body?: string; + /** default-setup returns `state: 'configured'` (code scanning is on). */ + defaultSetupConfigured?: boolean; + /** Workflow files present in the repo tree: path → file content. */ + workflowFiles?: Record; } const makeRepo = (fullName: string, config: RepoConfig): GitHubRepo => { @@ -26,15 +43,30 @@ const makeRepo = (fullName: string, config: RepoConfig): GitHubRepo => { default_branch: 'main', owner: { login: owner!, type: 'Organization' }, }; - if (config.ghas) { - repo.security_and_analysis = { advanced_security: { status: config.ghas } }; + if (config.ghas || config.codeSecurity) { + repo.security_and_analysis = { + ...(config.codeSecurity ? { code_security: { status: config.codeSecurity } } : {}), + ...(config.ghas ? { advanced_security: { status: config.ghas } } : {}), + }; } return repo; }; +// Mirrors how ctx.fetch surfaces an HTTP error: the response body is appended to +// the message, so the check can read GitHub's own explanation from `String(error)`. +const httpError = (status: number, body: string): Error => + new Error(`HTTP ${status}: Forbidden - ${body}`); + +interface FailResult { + resourceId: string; + title: string; + description: string; + remediation?: string; +} + interface RunResult { passed: Array<{ resourceId: string; title: string }>; - failed: Array<{ resourceId: string; title: string }>; + failed: FailResult[]; } async function runCheck(repos: Record): Promise { @@ -50,11 +82,17 @@ async function runCheck(repos: Record): Promise { metadata: {}, log: () => {}, warn: () => {}, + error: () => {}, pass: (result) => { passed.push({ resourceId: result.resourceId ?? '', title: result.title }); }, fail: (result) => { - failed.push({ resourceId: result.resourceId ?? '', title: result.title }); + failed.push({ + resourceId: result.resourceId ?? '', + title: result.title, + description: result.description, + remediation: result.remediation, + }); }, fetch: (async (path: string): Promise => { // /repos// @@ -62,7 +100,7 @@ async function runCheck(repos: Record): Promise { if (repoMatch) { const fullName = repoMatch[1]!; const config = repos[fullName]; - if (!config) throw new Error(`404 ${path}`); + if (!config) throw httpError(404, 'repo'); return makeRepo(fullName, config) as unknown as T; } @@ -70,17 +108,30 @@ async function runCheck(repos: Record): Promise { const setupMatch = path.match(/^\/repos\/([^/]+\/[^/]+)\/code-scanning\/default-setup$/); if (setupMatch) { const config = repos[setupMatch[1]!]!; - // GitHub 403s this endpoint for private repos when the token lacks the - // repo-admin visibility the code-scanning API requires (and, separately, - // when GHAS is off). Both surface identically to us. - if (config.defaultSetup403) throw new Error(`403 Forbidden: ${path}`); + if (config.defaultSetup403Body) throw httpError(403, config.defaultSetup403Body); + if (config.defaultSetupConfigured) { + return { state: 'configured', languages: ['javascript'] } as unknown as T; + } return { state: 'not-configured' } as unknown as T; } - // /repos///git/trees/?recursive=1 — no workflow files - // (default-setup repos carry no CodeQL .yml). - if (/^\/repos\/[^/]+\/[^/]+\/git\/trees\/.+/.test(path)) { - return { sha: 'x', url: '', truncated: false, tree: [] } as unknown as T; + // /repos///git/trees/?recursive=1 + const treeMatch = path.match(/^\/repos\/([^/]+\/[^/]+)\/git\/trees\/.+/); + if (treeMatch) { + const config = repos[treeMatch[1]!]!; + const tree = Object.keys(config.workflowFiles ?? {}).map((p) => ({ + path: p, + type: 'blob' as const, + })); + return { sha: 'x', url: '', truncated: false, tree } as unknown as T; + } + + // /repos///contents/ + const contentsMatch = path.match(/^\/repos\/([^/]+\/[^/]+)\/contents\/(.+)$/); + if (contentsMatch) { + const content = repos[contentsMatch[1]!]?.workflowFiles?.[contentsMatch[2]!]; + if (content == null) throw httpError(404, 'contents'); + return { path: contentsMatch[2]!, encoding: 'utf-8', content } as unknown as T; } throw new Error(`Unexpected fetch: ${path}`); @@ -98,38 +149,114 @@ async function runCheck(repos: Record): Promise { } describe('codeScanningCheck GHAS visibility (regression: CS-756)', () => { - it('does NOT report GHAS required when GHAS status is unknown (OAuth non-admin: 403 + private + no security_and_analysis block)', async () => { - // Saltbox repro: OAuth token without repo-admin visibility on a private repo - // that actually HAS GHAS + CodeQL default setup enabled. GitHub omits the - // `security_and_analysis` block AND 403s the code-scanning API. We cannot - // confirm GHAS is off, so we must not falsely claim it is. + it('does NOT report GHAS required when the feature is visible-but-unknown and the API 403s for permission (OAuth non-admin)', async () => { + // OAuth token without repo-admin visibility on a private repo that actually + // HAS code scanning enabled. GitHub omits the `security_and_analysis` block + // AND 403s the code-scanning API with a permission ("not accessible") body — + // NOT a "must be enabled" body. We cannot confirm the feature is off, so we + // must not falsely claim it is. const { passed, failed } = await runCheck({ - 'saltbox/s1-app-monitor': { private: true, defaultSetup403: true }, // ghas block omitted + 'saltbox/s1-app-monitor': { private: true, defaultSetup403Body: NOT_ACCESSIBLE_BODY }, }); expect(passed).toHaveLength(0); expect(failed).toHaveLength(1); - // Before the fix this was: - // "Code scanning requires GitHub Advanced Security for s1-app-monitor" expect(failed[0]!.title).toBe('Cannot access code scanning configuration for s1-app-monitor'); - expect(failed[0]!.title).not.toContain('requires GitHub Advanced Security'); + expect(failed[0]!.title).not.toContain('requires GitHub'); }); - it('still reports GHAS required when GHAS is positively disabled on a private repo', async () => { + it('still reports Code Security required when it is positively disabled on a private repo', async () => { const { failed } = await runCheck({ - 'acme/no-ghas': { private: true, ghas: 'disabled', defaultSetup403: true }, + 'acme/no-ghas': { private: true, ghas: 'disabled', defaultSetup403Body: NOT_ACCESSIBLE_BODY }, }); expect(failed).toHaveLength(1); - expect(failed[0]!.title).toBe('Code scanning requires GitHub Advanced Security for no-ghas'); + expect(failed[0]!.title).toBe('Code scanning requires GitHub Code Security for no-ghas'); }); - it('reports permission-denied (not GHAS required) when GHAS is enabled but the API still 403s', async () => { + it('reports permission-denied (not Code Security required) when GHAS is enabled but the API still 403s for permission', async () => { const { failed } = await runCheck({ - 'acme/ghas-on': { private: true, ghas: 'enabled', defaultSetup403: true }, + 'acme/ghas-on': { private: true, ghas: 'enabled', defaultSetup403Body: NOT_ACCESSIBLE_BODY }, }); expect(failed).toHaveLength(1); expect(failed[0]!.title).toBe('Cannot access code scanning configuration for ghas-on'); }); }); + +describe('codeScanningCheck feature-off vs permission (regression: CS-762)', () => { + it('private repo with Code Security disabled (new code_security field) → "requires Code Security", NOT a permission error', async () => { + // The exact CS-762 case: GitHub's 2026 payload carries `code_security` + // (not `advanced_security`) and the 403 body says the feature must be enabled. + const { passed, failed } = await runCheck({ + 'acme/webapp': { + private: true, + codeSecurity: 'disabled', + defaultSetup403Body: FEATURE_OFF_BODY, + }, + }); + + expect(passed).toEqual([]); + expect(failed).toHaveLength(1); + const finding = failed[0]!; + expect(finding.title).toBe('Code scanning requires GitHub Code Security for webapp'); + // Must NOT surface the misleading permission message or the wrong remediation. + expect(finding.description).not.toContain('does not have permission'); + expect(finding.remediation).not.toContain('Code scanning alerts: Read'); + }); + + it('403 "must be enabled" body wins even when security_and_analysis is omitted (unknown)', async () => { + const { failed } = await runCheck({ + 'acme/api': { private: true, defaultSetup403Body: FEATURE_OFF_BODY }, + }); + + expect(failed.map((f) => f.title)).toEqual([ + 'Code scanning requires GitHub Code Security for api', + ]); + }); + + it('public repo with a "must be enabled" 403 → not-configured (never a permission error)', async () => { + const { failed } = await runCheck({ + 'acme/public': { private: false, defaultSetup403Body: FEATURE_OFF_BODY }, + }); + + expect(failed.map((f) => f.title)).toEqual(['Code scanning not enabled for public']); + }); + + it('genuine "Resource not accessible" 403 → permission-denied with an accurate remediation', async () => { + const { failed } = await runCheck({ + 'acme/secret': { private: true, defaultSetup403Body: NOT_ACCESSIBLE_BODY }, + }); + + expect(failed).toHaveLength(1); + const finding = failed[0]!; + expect(finding.title).toBe('Cannot access code scanning configuration for secret'); + expect(finding.description).toContain('Administration: read'); + expect(finding.remediation).not.toContain('Code scanning alerts: Read'); + }); + + it('default setup configured → pass', async () => { + const { passed, failed } = await runCheck({ + 'acme/scanned': { private: true, codeSecurity: 'enabled', defaultSetupConfigured: true }, + }); + + expect(failed).toEqual([]); + expect(passed.map((p) => p.title)).toEqual(['CodeQL scanning configured for scanned']); + }); + + it('third-party SAST workflow satisfies the check even when the API 403s', async () => { + const { passed, failed } = await runCheck({ + 'acme/sast': { + private: true, + defaultSetup403Body: FEATURE_OFF_BODY, + workflowFiles: { + '.github/workflows/security.yml': + 'jobs:\n scan:\n steps:\n - uses: github/codeql-action/upload-sarif@v3', + }, + }, + }); + + expect(failed).toEqual([]); + expect(passed.map((p) => p.title)).toEqual(['CodeQL scanning configured for sast']); + }); +}); diff --git a/packages/integration-platform/src/manifests/github/checks/code-scanning.ts b/packages/integration-platform/src/manifests/github/checks/code-scanning.ts index b33c4f5c54..60f2caa426 100644 --- a/packages/integration-platform/src/manifests/github/checks/code-scanning.ts +++ b/packages/integration-platform/src/manifests/github/checks/code-scanning.ts @@ -47,10 +47,11 @@ type CodeScanningStatus = | { status: 'ghas-required' }; /** - * Whether GitHub Advanced Security is enabled for a repo. `unknown` covers the - * case where GitHub omits the repo's `security_and_analysis` block because the - * token lacks repo-admin visibility (common over OAuth connections) — we must - * NOT treat that as `disabled`. + * Whether the repo's code-scanning entitlement (GitHub Code Security, formerly + * Advanced Security) is enabled. `unknown` covers the case where GitHub omits the + * repo's `security_and_analysis` block because the token lacks repo-admin + * visibility (common over OAuth connections) — we must NOT treat that as + * `disabled`. */ type GhasStatus = 'enabled' | 'disabled' | 'unknown'; @@ -165,6 +166,13 @@ export const codeScanningCheck: IntegrationCheck = { ghasStatus: GhasStatus; }): Promise => { let apiGot403 = false; + // GitHub returns 403 for two very different reasons, and only one is our + // fault. When the code-scanning feature is simply not turned on for the + // repo, the 403 body says "…must be enabled…" (Code Security / Advanced + // Security). When our token genuinely lacks access it says "Resource not + // accessible by integration". This flag records the former so we don't + // report a repo-configuration gap as a missing integration permission. + let featureDisabled = false; // First, try the default setup API try { @@ -182,11 +190,18 @@ export const codeScanningCheck: IntegrationCheck = { const errorStr = String(error); if (errorStr.includes('403') || errorStr.includes('Forbidden')) { - // The code-scanning API requires GHAS for private repos, but reading - // workflow file contents only requires contents:read. A 403 here does - // not mean we can't check for code scanning workflows. + // The code-scanning API requires Code Security for private repos, but + // reading workflow file contents only requires contents:read. A 403 + // here does not mean we can't check for code scanning workflows. + // + // `ctx.fetch` puts GitHub's response body in the error message, so we + // can read GitHub's own explanation. "…must be enabled…" means the + // feature is off on the repo, not that we lack permission. + if (/must be enabled|advanced security|code security/i.test(errorStr)) { + featureDisabled = true; + } ctx.log( - `Code scanning API returned 403 for ${repoName} (private: ${isPrivate}, ghas: ${ghasStatus}). Falling back to workflow file scanning.`, + `Code scanning API returned 403 for ${repoName} (private: ${isPrivate}, ghas: ${ghasStatus}, featureDisabled: ${featureDisabled}). Falling back to workflow file scanning.`, ); apiGot403 = true; } else { @@ -208,13 +223,19 @@ export const codeScanningCheck: IntegrationCheck = { } if (apiGot403) { - // A 403 from the code-scanning API on a private repo can mean either GHAS - // is off (CodeQL genuinely can't be configured) OR the token lacks the - // repo-admin visibility the API requires. Only claim "GHAS required" when - // we can positively confirm GHAS is disabled. Over an OAuth connection - // without repo admin, GitHub omits the repo's `security_and_analysis` - // block, so ghasStatus is 'unknown' — treat that (and 'enabled') as - // permission-denied rather than falsely reporting GHAS is not enabled. + // Prefer GitHub's own explanation. If it told us the feature must be + // enabled, this is a repo-configuration gap, not a permission problem: + // a private repo needs GitHub Code Security (paid), while a public repo + // just has not set code scanning up. + if (featureDisabled) { + return isPrivate ? { status: 'ghas-required' } : { status: 'not-configured' }; + } + // No feature-off signal in the body. Fall back to the repo's + // `security_and_analysis` block: only claim "Code Security required" when + // we can positively confirm it is disabled. Over a connection without + // repo-admin visibility GitHub omits that block, so ghasStatus is + // 'unknown' — treat that (and 'enabled') as permission-denied rather than + // falsely reporting the feature is off. if (isPrivate && ghasStatus === 'disabled') { return { status: 'ghas-required' }; } @@ -229,11 +250,17 @@ export const codeScanningCheck: IntegrationCheck = { if (!repo) continue; const tree = await fetchRepoTree(repo.full_name, repo.default_branch); - // `security_and_analysis` is only returned to repo admins, so over an OAuth - // connection without admin visibility this is undefined — 'unknown', NOT - // 'disabled'. Conflating the two is what made us falsely report GHAS off. + // Read the code-scanning entitlement. GitHub's 2026 Code Security GA renamed + // this from `advanced_security` to `code_security`; check the new key first + // and fall back to the old one for GitHub Enterprise Server / older payloads. + // `security_and_analysis` is only returned to repo admins, so over a + // connection without admin visibility the whole block is undefined — + // 'unknown', NOT 'disabled'. Conflating the two is what made us falsely + // report the feature off (or, after the OAuth fix, falsely report a + // permission problem). + const sa = repo.security_and_analysis; const ghasStatus: GhasStatus = - repo.security_and_analysis?.advanced_security?.status ?? 'unknown'; + sa?.code_security?.status ?? sa?.advanced_security?.status ?? 'unknown'; const codeScanningStatus = await getCodeScanningStatus({ repoName: repo.full_name, @@ -271,14 +298,14 @@ export const codeScanningCheck: IntegrationCheck = { case 'ghas-required': ctx.fail({ - title: `Code scanning requires GitHub Advanced Security for ${repo.name}`, + title: `Code scanning requires GitHub Code Security for ${repo.name}`, description: - 'This is a private repository. GitHub Advanced Security (GHAS) must be enabled before CodeQL can be configured. GHAS is a paid feature for private repositories.', + 'This private repository has no code scanning configured. GitHub CodeQL requires GitHub Code Security (formerly GitHub Advanced Security), a paid feature for private repositories. A third-party SAST tool (Semgrep, Snyk, Trivy, etc.) that uploads SARIF results also satisfies this check.', resourceType: 'repository', resourceId: repo.full_name, severity: 'medium', remediation: - 'Enable GitHub Advanced Security in the repository settings (Settings → Code security and analysis → GitHub Advanced Security), then enable CodeQL.', + 'Enable GitHub Code Security for this repository (Settings → Code security → GitHub Advanced Security / Code Security) and turn on CodeQL default setup, or add a workflow under .github/workflows that runs a SAST tool and uploads SARIF (github/codeql-action/upload-sarif).', evidence: { [repo.full_name]: { code_scanning: { @@ -294,12 +321,12 @@ export const codeScanningCheck: IntegrationCheck = { ctx.fail({ title: `Cannot access code scanning configuration for ${repo.name}`, description: - 'The GitHub integration does not have permission to read code scanning configuration. This may be due to missing permissions or organization policies.', + "GitHub returned 403 when reading this repository's code scanning configuration. GitHub requires admin (Administration: read) access to the repository to read this setting, and the connected account or GitHub App installation does not currently have it for this repo.", resourceType: 'repository', resourceId: repo.full_name, severity: 'medium', remediation: - 'Ensure the GitHub App has "Code scanning alerts: Read" permission. If this is an organization repository, check that organization policies allow access.', + 'Reconnect with an account that has admin access to this repository, or grant the GitHub App installation admin (Administration: read) access to it. If this is an organization repository, also confirm organization policies allow the app to access it.', evidence: { [repo.full_name]: { code_scanning: { diff --git a/packages/integration-platform/src/manifests/github/types.ts b/packages/integration-platform/src/manifests/github/types.ts index 990c17386a..bd0c935a23 100644 --- a/packages/integration-platform/src/manifests/github/types.ts +++ b/packages/integration-platform/src/manifests/github/types.ts @@ -17,6 +17,11 @@ export interface GitHubRepo { default_branch: string; owner: { login: string; type?: 'User' | 'Organization' }; security_and_analysis?: { + // GitHub's 2026 "Code Security" GA renamed the code-scanning entitlement from + // `advanced_security` to `code_security`. Newer API responses only carry + // `code_security`; `advanced_security` is kept for GitHub Enterprise Server + // and older responses. + code_security?: { status: 'enabled' | 'disabled' }; advanced_security?: { status: 'enabled' | 'disabled' }; dependabot_security_updates?: { status: 'enabled' | 'disabled' }; secret_scanning?: { status: 'enabled' | 'disabled' };