diff --git a/.server-changes/reject-benchmarking-webhook-addresses.md b/.server-changes/reject-benchmarking-webhook-addresses.md new file mode 100644 index 00000000000..32d7792c746 --- /dev/null +++ b/.server-changes/reject-benchmarking-webhook-addresses.md @@ -0,0 +1,6 @@ +--- +area: webapp +type: fix +--- + +Reject alert webhook destinations in reserved benchmarking IP ranges. diff --git a/.server-changes/reject-prewhere-in-trql.md b/.server-changes/reject-prewhere-in-trql.md new file mode 100644 index 00000000000..23b55a5d2d1 --- /dev/null +++ b/.server-changes/reject-prewhere-in-trql.md @@ -0,0 +1,6 @@ +--- +area: webapp +type: fix +--- + +TRQL queries using the PREWHERE clause are now rejected with a clear error message. Use WHERE instead, which is filtered the same way but keeps your data isolation guarantees intact. diff --git a/apps/webapp/app/models/orgIntegration.server.ts b/apps/webapp/app/models/orgIntegration.server.ts index f2bd0feebce..16641f3a133 100644 --- a/apps/webapp/app/models/orgIntegration.server.ts +++ b/apps/webapp/app/models/orgIntegration.server.ts @@ -9,10 +9,15 @@ import { z } from "zod"; import { $transaction, prisma } from "~/db.server"; import { env } from "~/env.server"; import { logger } from "~/services/logger.server"; +import { redirectWithErrorMessage } from "./message.server"; import { slackSecretLogFields } from "./safeIntegrationLog"; import { slackAccessResultLogFields } from "./slackOAuthResultLog"; import { getSecretStore } from "~/services/secrets/secretStore.server"; -import { commitSession, getUserSession } from "~/services/sessionStorage.server"; +import { + clearSlackOAuthSessionBinding, + consumeSlackOAuthStateForSession, + createSlackOAuthStateForSession, +} from "~/models/slackOAuthState.server"; import { generateFriendlyId } from "~/v3/friendlyIdentifiers"; const SlackSecretSchema = z.object({ @@ -27,8 +32,6 @@ const SlackSecretSchema = z.object({ type SlackSecret = z.infer; -const REDIRECT_AFTER_AUTH_KEY = "redirect-back-after-auth"; - export type OrganizationIntegrationForService = Omit< AuthenticatableIntegration, "service" @@ -138,22 +141,26 @@ export class OrgIntegrationRepository { static async redirectToAuthService( service: IntegrationService, - state: string, + organizationId: string, + userId: string, request: Request, redirectTo: string ) { - const session = await getUserSession(request); - session.set(REDIRECT_AFTER_AUTH_KEY, redirectTo); - - const authUrl = service === "SLACK" ? this.slackAuthorizationUrl(state) : undefined; - - if (!authUrl) { + if (service !== "SLACK") { throw new Response("Unsupported service", { status: 400 }); } + const { nonce, sessionCookie } = await createSlackOAuthStateForSession(request, { + userId, + organizationId, + service: "slack", + redirectTo, + }); + + const authUrl = this.slackAuthorizationUrl(nonce); + logger.debug("Redirecting to auth service", { service, - authUrl, redirectTo, }); @@ -161,35 +168,33 @@ export class OrgIntegrationRepository { status: 302, headers: { location: authUrl, - "Set-Cookie": await commitSession(session), + "Set-Cookie": sessionCookie, }, }); } - static async redirectAfterAuth(request: Request) { - const session = await getUserSession(request); - - logger.debug("Redirecting back after auth", { - sessionData: session.data, - }); - - const redirectTo = session.get(REDIRECT_AFTER_AUTH_KEY); + static async redirectAfterAuth(request: Request, redirectTo: string, errorMessage?: string) { + const sessionCookie = await clearSlackOAuthSessionBinding(request); - if (!redirectTo) { - throw new Response("Invalid redirect", { status: 400 }); + if (errorMessage) { + const response = await redirectWithErrorMessage(redirectTo, request, errorMessage); + response.headers.append("Set-Cookie", sessionCookie); + return response; } - session.unset(REDIRECT_AFTER_AUTH_KEY); - return new Response(null, { status: 302, headers: { location: redirectTo, - "Set-Cookie": await commitSession(session), + "Set-Cookie": sessionCookie, }, }); } + static async consumeSlackOAuthState(request: Request, state: string, userId: string) { + return consumeSlackOAuthStateForSession(request, state, userId); + } + static async createOrgIntegration(serviceName: string, code: string, org: Organization) { switch (serviceName) { case "slack": { diff --git a/apps/webapp/app/models/slackOAuthState.server.ts b/apps/webapp/app/models/slackOAuthState.server.ts new file mode 100644 index 00000000000..93ad9ee5b57 --- /dev/null +++ b/apps/webapp/app/models/slackOAuthState.server.ts @@ -0,0 +1,139 @@ +import { randomBytes } from "node:crypto"; +import { z } from "zod"; +import { env } from "~/env.server"; +import { createRedisClient, type RedisClient } from "~/redis.server"; +import { commitSession, getUserSession } from "~/services/sessionStorage.server"; +import { singleton } from "~/utils/singleton"; + +const STATE_TTL_SECONDS = 10 * 60; +const CREATE_ATTEMPTS = 2; +const KEY_PREFIX = "oauth:slack:state:"; +const SLACK_OAUTH_SESSION_BINDING_KEY = "slack-oauth-session-binding"; + +const SlackOAuthStateSchema = z.object({ + userId: z.string(), + sessionBinding: z.string(), + organizationId: z.string(), + service: z.literal("slack"), + redirectTo: z.string().regex(/^\/(?!\/)/), +}); + +export type SlackOAuthState = z.infer; + +type CreateSlackOAuthState = SlackOAuthState; +type StartSlackOAuthState = Omit; +type ConsumeSlackOAuthState = Pick; + +const consumeScript = ` +local raw = redis.call("GET", KEYS[1]) +if not raw then return nil end +local decoded, state = pcall(cjson.decode, raw) +if not decoded or type(state) ~= "table" then return nil end +if state.userId ~= ARGV[1] or state.sessionBinding ~= ARGV[2] or state.service ~= ARGV[3] then + return nil +end +redis.call("DEL", KEYS[1]) +return raw +`; + +export class SlackOAuthStateStore { + constructor(private readonly redis: Pick) {} + + async create(state: CreateSlackOAuthState): Promise { + const parsedState = SlackOAuthStateSchema.parse(state); + + for (let attempt = 0; attempt < CREATE_ATTEMPTS; attempt++) { + const nonce = randomBytes(32).toString("base64url"); + const created = await this.redis.set( + this.#key(nonce), + JSON.stringify(parsedState), + "EX", + STATE_TTL_SECONDS, + "NX" + ); + if (created === "OK") return nonce; + } + + throw new Error("Failed to create a unique Slack OAuth state"); + } + + async consume( + nonce: string, + expected: ConsumeSlackOAuthState + ): Promise { + if (!/^[A-Za-z0-9_-]{43}$/.test(nonce)) return undefined; + + const raw = await this.redis.eval( + consumeScript, + 1, + this.#key(nonce), + expected.userId, + expected.sessionBinding, + expected.service + ); + if (typeof raw !== "string") return undefined; + + try { + return SlackOAuthStateSchema.safeParse(JSON.parse(raw)).data; + } catch { + return undefined; + } + } + + #key(nonce: string): string { + return `${KEY_PREFIX}{${nonce}}`; + } +} + +export async function createSlackOAuthStateForSession( + request: Request, + state: StartSlackOAuthState, + stateStore: SlackOAuthStateStore = getSlackOAuthStateStore() +): Promise<{ nonce: string; sessionCookie: string }> { + const session = await getUserSession(request); + const sessionBinding = randomBytes(32).toString("base64url"); + const nonce = await stateStore.create({ ...state, sessionBinding }); + session.set(SLACK_OAUTH_SESSION_BINDING_KEY, sessionBinding); + + return { nonce, sessionCookie: await commitSession(session) }; +} + +export async function consumeSlackOAuthStateForSession( + request: Request, + nonce: string, + userId: string, + stateStore: SlackOAuthStateStore = getSlackOAuthStateStore() +): Promise { + const session = await getUserSession(request); + const sessionBinding = session.get(SLACK_OAUTH_SESSION_BINDING_KEY); + if (typeof sessionBinding !== "string") return undefined; + + return stateStore.consume(nonce, { userId, sessionBinding, service: "slack" }); +} + +export async function clearSlackOAuthSessionBinding(request: Request): Promise { + const session = await getUserSession(request); + session.unset(SLACK_OAUTH_SESSION_BINDING_KEY); + return commitSession(session); +} + +function getSlackOAuthStateStore(): SlackOAuthStateStore { + if (!env.CACHE_REDIS_HOST) { + throw new Error("Cache Redis is required for Slack OAuth state"); + } + + return singleton( + "slackOAuthStateStore", + () => + new SlackOAuthStateStore( + createRedisClient("trigger:slack-oauth-state", { + host: env.CACHE_REDIS_HOST, + port: env.CACHE_REDIS_PORT, + username: env.CACHE_REDIS_USERNAME, + password: env.CACHE_REDIS_PASSWORD, + tlsDisabled: env.CACHE_REDIS_TLS_DISABLED === "true", + clusterMode: env.CACHE_REDIS_CLUSTER_MODE_ENABLED === "1", + }) + ) + ); +} diff --git a/apps/webapp/app/routes/_app.orgs.$organizationSlug.invite/route.tsx b/apps/webapp/app/routes/_app.orgs.$organizationSlug.invite/route.tsx index 5d016e96bc2..d8dcbcc017f 100644 --- a/apps/webapp/app/routes/_app.orgs.$organizationSlug.invite/route.tsx +++ b/apps/webapp/app/routes/_app.orgs.$organizationSlug.invite/route.tsx @@ -28,6 +28,7 @@ import { $replica } from "~/db.server"; import { env } from "~/env.server"; import { useOrganization } from "~/hooks/useOrganizations"; import { inviteMembers } from "~/models/member.server"; +import { checkInviteRateLimit, InviteRateLimitError } from "~/services/inviteRateLimiter.server"; import { redirectWithErrorMessage, redirectWithSuccessMessage } from "~/models/message.server"; import { resolveOrgIdFromSlug } from "~/models/organization.server"; import { TeamPresenter } from "~/presenters/TeamPresenter.server"; @@ -176,6 +177,23 @@ export const action = dashboardAction( } } + // Every invite emails the address, so cap per-org and per-inviter sends + // (same limiter as the invite-create API). With no org scope the + // slug didn't resolve and inviteMembers rejects anyway. + if (env.LOGIN_RATE_LIMITS_ENABLED && context.organizationId) { + try { + await checkInviteRateLimit(context.organizationId, userId, submission.value.emails.length); + } catch (error) { + if (error instanceof InviteRateLimitError) { + return json( + { errors: { body: "Too many invites sent. Please try again later." } }, + { status: 429 } + ); + } + throw error; + } + } + // Resolve the RBAC role choice. NO_RBAC_ROLE / undefined / unknown // role → don't pass one through; the runtime fallback handles it. // Validation: the chosen role must be in the org's assignable set diff --git a/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.alerts.new.connect-to-slack.ts b/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.alerts.new.connect-to-slack.ts index ddd1bf646b7..80529607340 100644 --- a/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.alerts.new.connect-to-slack.ts +++ b/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.alerts.new.connect-to-slack.ts @@ -47,6 +47,7 @@ export async function loader({ request, params }: LoaderFunctionArgs) { return await OrgIntegrationRepository.redirectToAuthService( "SLACK", project.organizationId, + userId, request, v3NewProjectAlertPathConnectToSlackPath({ slug: organizationSlug }, project, { slug: envParam, diff --git a/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.connect-to-slack.ts b/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.connect-to-slack.ts index b8bed6b631d..a47f8ec315a 100644 --- a/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.connect-to-slack.ts +++ b/apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.errors.connect-to-slack.ts @@ -42,6 +42,7 @@ export async function loader({ request, params }: LoaderFunctionArgs) { return await OrgIntegrationRepository.redirectToAuthService( "SLACK", project.organizationId, + userId, request, v3ErrorsConnectToSlackPath({ slug: organizationSlug }, project, { slug: envParam }) ); diff --git a/apps/webapp/app/routes/api.v1.orgs.$orgParam.invites.ts b/apps/webapp/app/routes/api.v1.orgs.$orgParam.invites.ts index 36e56e060eb..7a9139aa296 100644 --- a/apps/webapp/app/routes/api.v1.orgs.$orgParam.invites.ts +++ b/apps/webapp/app/routes/api.v1.orgs.$orgParam.invites.ts @@ -3,6 +3,7 @@ import { z } from "zod"; import { prisma } from "~/db.server"; import { env } from "~/env.server"; import { inviteMembers } from "~/models/member.server"; +import { checkInviteRateLimit, InviteRateLimitError } from "~/services/inviteRateLimiter.server"; import { logger } from "~/services/logger.server"; import { resolveOrganizationForApiUser } from "~/services/organizationApiAccess.server"; import { createActionPATApiRoute } from "~/services/routeBuilders/apiBuilder.server"; @@ -57,6 +58,26 @@ export const action = createActionPATApiRoute( return json({ error: "Membership is managed by Directory Sync" }, { status: 403 }); } + // Every invite emails the address, so cap per-org and per-inviter sends. + if (env.LOGIN_RATE_LIMITS_ENABLED) { + try { + await checkInviteRateLimit(organization.id, authentication.userId, body.emails.length); + } catch (error) { + if (error instanceof InviteRateLimitError) { + return json( + { error: "Too many invites sent. Please try again later." }, + { + status: 429, + headers: { + "Retry-After": Math.ceil(error.retryAfter / 1000).toString(), + }, + } + ); + } + throw error; + } + } + const { created, alreadyMembers, alreadyInvited } = await inviteMembers({ slug: organization.slug, emails: body.emails, diff --git a/apps/webapp/app/routes/integrations.$serviceName.callback.ts b/apps/webapp/app/routes/integrations.$serviceName.callback.ts index 8d2160287dc..fe4f0bd346f 100644 --- a/apps/webapp/app/routes/integrations.$serviceName.callback.ts +++ b/apps/webapp/app/routes/integrations.$serviceName.callback.ts @@ -1,6 +1,5 @@ import type { LoaderFunctionArgs } from "@remix-run/server-runtime"; import z from "zod"; -import { redirectBackWithErrorMessage } from "~/models/message.server"; import { OrgIntegrationRepository } from "~/models/orgIntegration.server"; import { requireUserId } from "~/services/session.server"; import { requestUrl } from "~/utils/requestUrl.server"; @@ -45,22 +44,35 @@ export async function loader({ request, params }: LoaderFunctionArgs) { const parsedParams = ParamsSchema.safeParse(params); - if (!parsedParams.success) { + if (!parsedParams.success || parsedParams.data.serviceName !== "slack") { throw new Response("Invalid params", { status: 400 }); } + const oauthState = await OrgIntegrationRepository.consumeSlackOAuthState( + request, + parsedSearchParams.data.state, + userId + ); + if (!oauthState) { + throw new Response("Invalid state", { status: 400 }); + } + const service = new CreateOrgIntegrationService(); const integration = await service.call( userId, - parsedSearchParams.data.state, - parsedParams.data.serviceName, + oauthState.organizationId, + oauthState.service, parsedSearchParams.data.code ); if (integration) { - return await OrgIntegrationRepository.redirectAfterAuth(request); + return await OrgIntegrationRepository.redirectAfterAuth(request, oauthState.redirectTo); } - return redirectBackWithErrorMessage(request, "Failed to connect to the service"); + return await OrgIntegrationRepository.redirectAfterAuth( + request, + oauthState.redirectTo, + "Failed to connect to the service" + ); } diff --git a/apps/webapp/app/routes/invite-resend.tsx b/apps/webapp/app/routes/invite-resend.tsx index 3cba88bd20a..132387d53e7 100644 --- a/apps/webapp/app/routes/invite-resend.tsx +++ b/apps/webapp/app/routes/invite-resend.tsx @@ -1,9 +1,10 @@ import { parseWithZod } from "@conform-to/zod"; import { json } from "@remix-run/server-runtime"; -import { env } from "process"; import { z } from "zod"; import { $replica } from "~/db.server"; import { resendInvite } from "~/models/member.server"; +import { env } from "~/env.server"; +import { checkInviteRateLimit, InviteRateLimitError } from "~/services/inviteRateLimiter.server"; import { redirectWithSuccessMessage } from "~/models/message.server"; import { scheduleEmail } from "~/services/scheduleEmail.server"; import { ssoController } from "~/services/sso.server"; @@ -50,6 +51,23 @@ export const action = dashboardAction( } } + // Every resend emails the invitee, so apply the same per-org / + // per-inviter cap as the invite-create API. With no org scope (the + // inviteId lookup above found nothing), resendInvite rejects anyway. + if (env.LOGIN_RATE_LIMITS_ENABLED && context.organizationId) { + try { + await checkInviteRateLimit(context.organizationId, user.id, 1); + } catch (error) { + if (error instanceof InviteRateLimitError) { + return json( + { errors: { body: "Too many invites sent. Please try again later." } }, + { status: 429 } + ); + } + throw error; + } + } + try { const invite = await resendInvite({ inviteId: submission.value.inviteId, diff --git a/apps/webapp/app/routes/login.magic/route.tsx b/apps/webapp/app/routes/login.magic/route.tsx index cb164bcc91e..1c90cff734f 100644 --- a/apps/webapp/app/routes/login.magic/route.tsx +++ b/apps/webapp/app/routes/login.magic/route.tsx @@ -29,6 +29,7 @@ import { checkMagicLinkEmailDailyRateLimit, MagicLinkRateLimitError, checkMagicLinkIpRateLimit, + canonicalizeEmailForRateLimit, } from "~/services/magicLinkRateLimiter.server"; import { ssoRedirectForEmail } from "~/services/ssoAutoDiscovery.server"; import { logger, tryCatch } from "@trigger.dev/core/v3"; @@ -167,11 +168,16 @@ export async function action({ request }: ActionFunctionArgs) { const xff = request.headers.get("x-forwarded-for"); const clientIp = extractClientIp(xff); + // Key the buckets on the canonical address so `+tag` aliases (and + // Gmail dot variants) of one inbox share it. Delivery still uses the + // raw submitted address below. + const rateLimitKey = canonicalizeEmailForRateLimit(email); + const [error] = await tryCatch( Promise.all([ clientIp ? checkMagicLinkIpRateLimit(clientIp) : Promise.resolve(), - checkMagicLinkEmailRateLimit(email), - checkMagicLinkEmailDailyRateLimit(email), + checkMagicLinkEmailRateLimit(rateLimitKey), + checkMagicLinkEmailDailyRateLimit(rateLimitKey), ]) ); diff --git a/apps/webapp/app/routes/resources.account.mfa.setup/route.tsx b/apps/webapp/app/routes/resources.account.mfa.setup/route.tsx index 983e81cc8ab..7c1f4893d7f 100644 --- a/apps/webapp/app/routes/resources.account.mfa.setup/route.tsx +++ b/apps/webapp/app/routes/resources.account.mfa.setup/route.tsx @@ -9,6 +9,7 @@ import { import { MultiFactorAuthenticationService } from "~/services/mfa/multiFactorAuthentication.server"; import { requireUserId } from "~/services/session.server"; import { ServiceValidationError } from "~/v3/services/baseService.server"; +import { MfaRateLimitError } from "~/services/mfa/mfaRateLimiterGlobal.server"; import { useMfaSetup } from "./useMfaSetup"; import { MfaToggle } from "./MfaToggle"; import { MfaSetupDialog } from "./MfaSetupDialog"; @@ -137,6 +138,14 @@ export async function action({ request }: ActionFunctionArgs) { return redirectWithErrorMessage("/account/security", request, error.message); } + if (error instanceof MfaRateLimitError) { + return redirectWithErrorMessage( + "/account/security", + request, + "Too many attempts. Please try again later." + ); + } + // Re-throw unexpected errors throw error; } diff --git a/apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.in.$.ts b/apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.in.$.ts index e42acbd94cb..8173aae5761 100644 --- a/apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.in.$.ts +++ b/apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.in.$.ts @@ -42,6 +42,9 @@ const FORWARDED_HEADERS = [ // The only turn metadata a browser may set: everything else the agent reads is injected // server-side. A whitelist — a new clientData field is server-owned until listed here on purpose. +// `repoSnapshot` is the dangerous one to smuggle past this: its `tarballUrl` is fetched and +// extracted on the agent worker, so a client-supplied one is SSRF from inside the worker +// network plus an attacker-controlled untar. const CLIENT_METADATA_KEYS = ["currentPage", "pageContext"] as const; export function pickAgentClientMetadata( diff --git a/apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.ts b/apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.ts index 2f8d6fb6166..89677bf43d5 100644 --- a/apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.ts +++ b/apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.ts @@ -600,8 +600,25 @@ export const action = async ({ request, params }: ActionFunctionArgs) => { } catch { /* invalid JSON — start without metadata */ } + const runtimeEnv = await findEnvironmentBySlug(project.id, envParam, userId); + if (!runtimeEnv) return json({ error: "Environment not found" }, { status: 404 }); + try { - const { publicAccessToken } = await startDashboardAgentSession({ chatId, clientData }); + // Whitelisted like `create` and the `in` proxy: this object lands in the resumed + // run's `basePayload.metadata` verbatim, so without the pick a client could inject + // any server-owned field into the agent's first turn (a `repoSnapshot.tarballUrl` + // is fetched and extracted on the worker). + const { publicAccessToken } = await startDashboardAgentSession({ + chatId, + clientData: { + ...pickAgentClientMetadata(clientData), + organizationId: project.organizationId, + userId, + projectId: project.id, + environmentId: runtimeEnv.id, + ...dashboardAgentEnvironmentAddress(runtimeEnv), + }, + }); return json({ publicAccessToken }); } catch (error) { logger.error("Failed to start dashboard agent session", { chatId, error }); diff --git a/apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.waitpoints.$waitpointFriendlyId.complete/route.tsx b/apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.waitpoints.$waitpointFriendlyId.complete/route.tsx index 4993e66f68f..8cf1b653b8e 100644 --- a/apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.waitpoints.$waitpointFriendlyId.complete/route.tsx +++ b/apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.waitpoints.$waitpointFriendlyId.complete/route.tsx @@ -79,27 +79,41 @@ export const action = async ({ request, params }: ActionFunctionArgs) => { throw new Error("Project not found"); } + const environment = await findEnvironmentBySlug(project.id, envParam, userId); + if (!environment) { + return redirectWithErrorMessage( + submission.value.failureRedirect, + request, + "No waitpoint found" + ); + } + const waitpointId = WaitpointId.toId(waitpointFriendlyId); let waitpoint = await runStore.findWaitpoint({ select: { - projectId: true, - environmentId: true, + id: true, }, where: { id: waitpointId, + projectId: project.id, + environmentId: environment.id, }, }); if (!waitpoint) { // Read-your-writes: a just-minted token may not have replicated. Re-read the owning primary // before the auth guard / "No waitpoint found" (mirrors the token complete/callback routes). waitpoint = await runStore.findWaitpointOnPrimary({ - select: { projectId: true, environmentId: true }, - where: { id: waitpointId }, + select: { id: true }, + where: { + id: waitpointId, + projectId: project.id, + environmentId: environment.id, + }, }); } - if (waitpoint?.projectId !== project.id) { + if (!waitpoint) { return redirectWithErrorMessage( submission.value.failureRedirect, request, @@ -157,23 +171,6 @@ export const action = async ({ request, params }: ActionFunctionArgs) => { ); } - const environment = await findEnvironmentBySlug(project.id, envParam, userId); - if (!environment) { - return redirectWithErrorMessage( - submission.value.failureRedirect, - request, - "Environment not found" - ); - } - - if (environment.id !== waitpoint.environmentId) { - return redirectWithErrorMessage( - submission.value.failureRedirect, - request, - "No waitpoint found" - ); - } - const data = submission.value.payload ? JSON.parse(submission.value.payload) : {}; const stringifiedData = await stringifyIO(data); const finalData = await processWaitpointCompletionPacket( diff --git a/apps/webapp/app/services/inviteRateLimiter.server.ts b/apps/webapp/app/services/inviteRateLimiter.server.ts new file mode 100644 index 00000000000..1b8ea65d675 --- /dev/null +++ b/apps/webapp/app/services/inviteRateLimiter.server.ts @@ -0,0 +1,98 @@ +import { Ratelimit } from "@upstash/ratelimit"; +import { env } from "~/env.server"; +import { createRedisRateLimitClient, RateLimiter } from "~/services/rateLimiter.server"; +import { singleton } from "~/utils/singleton"; + +/** + * Rate limiting for organization invite sends (API create + dashboard + * resend). Each sent invite is an email to an arbitrary address, so a + * member with manage:members could otherwise mass-mail Trigger.dev-branded + * invites. Limits are per-organization (the blast radius is the org's + * brand) and per-inviter (spreads a burst across collaborators sharing an + * org). + * + * Sized against the 50-emails-per-request body cap: the per-minute windows + * pass a few bulk imports but stop a scripted loop; the daily org cap + * (500 = 10 bulk imports) leaves headroom for onboarding batches while + * bounding a day of abuse. + */ +export class InviteRateLimitError extends Error { + public readonly retryAfter: number; + + constructor(retryAfter: number) { + super("Invite rate limit exceeded."); + this.retryAfter = retryAfter; + } +} + +function getRedisClient() { + return createRedisRateLimitClient({ + port: env.RATE_LIMIT_REDIS_PORT, + host: env.RATE_LIMIT_REDIS_HOST, + username: env.RATE_LIMIT_REDIS_USERNAME, + password: env.RATE_LIMIT_REDIS_PASSWORD, + tlsDisabled: env.RATE_LIMIT_REDIS_TLS_DISABLED === "true", + clusterMode: env.RATE_LIMIT_REDIS_CLUSTER_MODE_ENABLED === "1", + }); +} + +const inviteOrgPerMinuteRateLimiter = singleton( + "inviteOrgPerMinuteRateLimiter", + () => + new RateLimiter({ + redisClient: getRedisClient(), + keyPrefix: "invites:org", + limiter: Ratelimit.slidingWindow(100, "1 m"), // 100 invite emails / min / org + logSuccess: false, + logFailure: true, + }) +); + +const inviteOrgDailyRateLimiter = singleton( + "inviteOrgDailyRateLimiter", + () => + new RateLimiter({ + redisClient: getRedisClient(), + keyPrefix: "invites:org:daily", + limiter: Ratelimit.slidingWindow(500, "1 d"), // 500 invite emails / day / org + logSuccess: false, + logFailure: true, + }) +); + +const inviteInviterRateLimiter = singleton( + "inviteInviterRateLimiter", + () => + new RateLimiter({ + redisClient: getRedisClient(), + keyPrefix: "invites:inviter", + limiter: Ratelimit.slidingWindow(60, "1 m"), // 60 invite emails / min / inviter + logSuccess: false, + logFailure: true, + }) +); + +/** + * Check whether `count` invite emails can be sent on behalf of + * `organizationId` by `inviterId`. All windows are charged `count` so a + * single 50-email request counts as 50 sends, not 1 request. + * @throws {InviteRateLimitError} If any limit is exceeded + */ +export async function checkInviteRateLimit( + organizationId: string, + inviterId: string, + count: number +): Promise { + const results = await Promise.all([ + inviteOrgPerMinuteRateLimiter.limit(organizationId, count), + inviteOrgDailyRateLimiter.limit(organizationId, count), + inviteInviterRateLimiter.limit(inviterId, count), + ]); + + for (const result of results) { + if (!result.success) { + const retryAfter = Math.max(0, new Date(result.reset).getTime() - Date.now()); + throw new InviteRateLimitError(retryAfter); + } + } +} diff --git a/apps/webapp/app/services/magicLinkRateLimiter.server.ts b/apps/webapp/app/services/magicLinkRateLimiter.server.ts index 2944e467960..970ae3e5d45 100644 --- a/apps/webapp/app/services/magicLinkRateLimiter.server.ts +++ b/apps/webapp/app/services/magicLinkRateLimiter.server.ts @@ -68,6 +68,33 @@ function initializeMagicLinkIpRateLimiter() { }); } +/** + * Canonicalize an email address for rate-limit keying so address variants + * that resolve to the same inbox share one bucket: lowercase, strip any + * `+tag` subaddress, and (for Gmail domains, which ignore dots in the local + * part) remove `.` from the local part. Use only for the limiter key — + * delivery must always use the raw submitted address. + */ +export function canonicalizeEmailForRateLimit(email: string): string { + const atIndex = email.lastIndexOf("@"); + if (atIndex < 1) { + return email.toLowerCase(); + } + + const localPart = email.slice(0, atIndex).toLowerCase(); + const domain = email.slice(atIndex + 1).toLowerCase(); + + const withoutTag = localPart.split("+")[0]; + + // Gmail (and Google Workspace on gmail.com/googlemail.com) ignores dots in + // the local part; other providers treat them as distinct addresses. + if (domain === "gmail.com" || domain === "googlemail.com") { + return `${withoutTag.replaceAll(".", "")}@gmail.com`; + } + + return `${withoutTag}@${domain}`; +} + export async function checkMagicLinkEmailRateLimit(identifier: string): Promise { const result = await magicLinkEmailRateLimiter.limit(identifier); diff --git a/apps/webapp/app/services/mfa/multiFactorAuthentication.server.ts b/apps/webapp/app/services/mfa/multiFactorAuthentication.server.ts index 53304105333..6a41f161cff 100644 --- a/apps/webapp/app/services/mfa/multiFactorAuthentication.server.ts +++ b/apps/webapp/app/services/mfa/multiFactorAuthentication.server.ts @@ -8,6 +8,7 @@ import { createOTP } from "@better-auth/utils/otp"; import { base32 } from "@better-auth/utils/base32"; import { z } from "zod"; import { scheduleEmail } from "../scheduleEmail.server"; +import { checkMfaRateLimit } from "./mfaRateLimiterGlobal.server"; const generateRandomString = createRandomStringGenerator("A-Z", "0-9"); @@ -48,6 +49,11 @@ export class MultiFactorAuthenticationService { }; } + // Rate limit before checking the code: disabling MFA asks for the second + // factor, so unlimited attempts here would let a hijacked session + // brute-force it. Same limiter as the login verify path. + await checkMfaRateLimit(userId); + // validate the TOTP code const secretStore = getSecretStore(user.mfaSecretReference.provider); const secretResult = await secretStore.getSecret(SecretSchema, user.mfaSecretReference.key); @@ -171,6 +177,11 @@ export class MultiFactorAuthenticationService { throw new ServiceValidationError("User has not enabled MFA"); } + // Rate limit enrollment confirmation too: it verifies a TOTP code against + // a real secret, so it needs the same brute-force protection as the + // disable and login verify paths. + await checkMfaRateLimit(userId); + const secret = secretResult.secret; const otp = createOTP(secret, { diff --git a/apps/webapp/test/dashboardAgentClientMetadata.test.ts b/apps/webapp/test/dashboardAgentClientMetadata.test.ts index eb75621fc1b..da4e9c10a8b 100644 --- a/apps/webapp/test/dashboardAgentClientMetadata.test.ts +++ b/apps/webapp/test/dashboardAgentClientMetadata.test.ts @@ -3,9 +3,11 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; const mocks = vi.hoisted(() => ({ fetch: vi.fn(), findEnvironmentBySlug: vi.fn<(...args: any[]) => Promise>(), + startSession: vi.fn<(...args: any[]) => Promise>(), + chatExists: vi.fn<(...args: any[]) => Promise>(), })); -vi.mock("~/db.server", () => ({ $replica: {} })); +vi.mock("~/db.server", () => ({ prisma: {}, $replica: {} })); vi.mock("~/env.server", () => ({ env: { SESSION_SECRET: "test-session-secret" } })); vi.mock("~/services/session.server", () => ({ requireUser: async () => ({ id: "usr_real", admin: false, isImpersonating: false }), @@ -25,14 +27,32 @@ vi.mock("~/models/runtimeEnvironment.server", () => ({ })); vi.mock("~/services/dashboardAgent.server", () => ({ dashboardAgentApiOrigin: () => "https://api.trigger.dev", + isDashboardAgentConfigured: () => true, + mintDashboardAgentToken: async () => "pat_public", mintDashboardAgentUserActorToken: async () => "tr_uat_real", resolveDashboardAgentRepoSnapshot: async () => null, + startDashboardAgentSession: mocks.startSession, +})); +vi.mock("~/services/dashboardAgentHeadStart.server", () => ({ + startDashboardAgentHeadStart: vi.fn(), +})); +vi.mock("~/services/dashboardAgentDb.server", () => ({ dashboardAgentDb: {} })); +vi.mock("~/services/resolveTriggerUri.server", () => ({ resolveTriggerUri: () => null })); +// The chat route reaches the ClickHouse factory through the watch services, and the factory +// builds its client at import time from an env var no test sets. +vi.mock("~/services/clickhouse/clickhouseFactoryInstance.server", () => ({ + clickhouseFactory: { getClickhouseForOrganization: async () => ({}) }, +})); +vi.mock("@internal/dashboard-agent-db", async (importOriginal) => ({ + ...((await importOriginal()) as Record), + chatExists: mocks.chatExists, })); vi.mock("~/services/logger.server", () => ({ logger: { debug: vi.fn(), error: vi.fn(), warn: vi.fn(), info: vi.fn() }, })); import { action } from "~/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent.in.$"; +import { action as chatAction } from "~/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent"; async function appendTurn(metadata: Record): Promise> { const request = new Request( @@ -160,3 +180,100 @@ describe("dashboard agent `in` proxy — client metadata", () => { expect(metadata).not.toHaveProperty("somethingNew"); }); }); + +// `intent=start` resumes an owned chat; the client-supplied clientData is folded into the +// resumed run's payload metadata verbatim, so it goes through the same whitelist as the +// `in` proxy. Otherwise a client could inject `repoSnapshot.tarballUrl` and the agent +// worker would fetch and extract it. +describe("dashboard agent `start` intent — client metadata", () => { + beforeEach(() => { + mocks.chatExists.mockReset().mockResolvedValue(true); + mocks.startSession.mockReset().mockResolvedValue({ publicAccessToken: "pat_public" }); + mocks.findEnvironmentBySlug.mockReset().mockResolvedValue({ + id: "env_real", + type: "DEVELOPMENT", + branchName: null, + }); + }); + + async function startChat(clientData: Record) { + const form = new URLSearchParams({ + intent: "start", + chatId: "chat_real", + clientData: JSON.stringify(clientData), + }); + + const response = await chatAction({ + request: new Request( + "https://app.trigger.dev/resources/orgs/acme/projects/api/env/dev/dashboard-agent", + { + method: "POST", + headers: { "content-type": "application/x-www-form-urlencoded" }, + body: form.toString(), + } + ), + params: { organizationSlug: "acme", projectParam: "api", envParam: "dev" }, + context: {}, + } as any); + + expect(response.status).toBe(200); + expect(mocks.startSession).toHaveBeenCalledTimes(1); + return mocks.startSession.mock.calls[0][0].clientData as Record; + } + + it("keeps the whitelisted page context", async () => { + const clientData = await startChat({ currentPage: "/runs", pageContext: { kind: "runs" } }); + + expect(clientData).toMatchObject({ currentPage: "/runs", pageContext: { kind: "runs" } }); + }); + + it("drops every server-owned field a client sends", async () => { + const clientData = await startChat({ + currentPage: "/runs", + organizationId: "org_evil", + userId: "usr_evil", + projectId: "proj_evil", + projectRef: "proj_ref_evil", + environmentId: "env_evil", + environmentName: "prod", + environmentBranch: "evil-branch", + apiOrigin: "https://evil.example.com", + userActorToken: "tr_uat_evil", + repoSnapshot: { tarballUrl: "https://evil.example.com/x.tar.gz" }, + somethingNew: "smuggled", + }); + + expect(clientData).toMatchObject({ + currentPage: "/runs", + organizationId: "org_real", + userId: "usr_real", + projectId: "proj_real", + environmentId: "env_real", + environmentName: "dev", + }); + expect(clientData.projectRef).toBeUndefined(); + expect(clientData.environmentBranch).toBeUndefined(); + expect(clientData.apiOrigin).toBeUndefined(); + expect(clientData.userActorToken).toBeUndefined(); + expect(clientData.repoSnapshot).toBeUndefined(); + expect(clientData).not.toHaveProperty("somethingNew"); + }); + + it("re-injects the server-owned identity the resumed run boots with", async () => { + const clientData = await startChat({ + currentPage: "/runs", + organizationId: "org_evil", + userId: "usr_evil", + projectId: "proj_evil", + environmentId: "env_evil", + }); + + expect(clientData).toMatchObject({ + currentPage: "/runs", + organizationId: "org_real", + userId: "usr_real", + projectId: "proj_real", + environmentId: "env_real", + }); + }); +}); diff --git a/apps/webapp/test/dashboardAgentCreateChatOrdering.test.ts b/apps/webapp/test/dashboardAgentCreateChatOrdering.test.ts index a50fed55374..a30c0a9d945 100644 --- a/apps/webapp/test/dashboardAgentCreateChatOrdering.test.ts +++ b/apps/webapp/test/dashboardAgentCreateChatOrdering.test.ts @@ -63,10 +63,11 @@ vi.mock("~/services/logger.server", () => ({ logger: mocks.logger })); import { action } from "~/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboard-agent"; -function createChatRequest() { +function createChatRequest(clientData?: Record) { const form = new URLSearchParams({ intent: "create", message: JSON.stringify({ id: "m1", role: "user", parts: [{ type: "text", text: "hi" }] }), + ...(clientData ? { clientData: JSON.stringify(clientData) } : {}), }); return action({ @@ -129,6 +130,44 @@ describe("dashboard agent chat creation — nothing fallible after the row exist }); expect(mocks.softDeleteChat).not.toHaveBeenCalled(); }); + + // The head-start metadata is built from the client's clientData: a smuggled + // `repoSnapshot.tarballUrl` would be fetched and extracted on the agent worker, so only + // the whitelisted page context may survive the merge. + it("strips server-owned fields from the clientData before head-starting", async () => { + const response = await createChatRequest({ + currentPage: "/runs", + pageContext: { kind: "runs" }, + organizationId: "org_evil", + userId: "usr_evil", + projectId: "proj_evil", + environmentId: "env_evil", + userActorToken: "tr_uat_evil", + apiOrigin: "https://evil.example.com", + repoSnapshot: { tarballUrl: "https://evil.example.com/x.tar.gz" }, + somethingNew: "smuggled", + }); + + expect(response.status).toBe(200); + const metadata = mocks.headStart.mock.calls[0][0].metadata; + expect(metadata.currentPage).toBe("/runs"); + expect(metadata.pageContext).toEqual({ kind: "runs" }); + expect(metadata.organizationId).toBe("org_real"); + expect(metadata.userId).toBe("usr_real"); + expect(metadata.projectId).toBe("proj_real"); + expect(metadata.environmentId).toBe("env_real"); + expect(metadata.userActorToken).toBe("tr_uat_real"); + expect(metadata.apiOrigin).toBe("https://api.trigger.dev"); + expect(metadata.repoSnapshot).toBeUndefined(); + expect(metadata).not.toHaveProperty("somethingNew"); + + // The chat row's stored context is whitelisted too. createChat(db, params) takes + // the db as arg 0, so the params object (carrying metadata) is arg 1. + const chatMetadata = mocks.createChat.mock.calls[0][1].metadata; + expect(chatMetadata).toEqual({ + context: { currentPage: "/runs", pageContext: { kind: "runs" } }, + }); + }); }); // A failed start means no handover was dispatched and no message was sent, so any session it diff --git a/apps/webapp/test/slackOAuthCallback.test.ts b/apps/webapp/test/slackOAuthCallback.test.ts new file mode 100644 index 00000000000..28199dd17bd --- /dev/null +++ b/apps/webapp/test/slackOAuthCallback.test.ts @@ -0,0 +1,87 @@ +import { describe, expect, it, beforeEach, vi } from "vitest"; + +const mocks = vi.hoisted(() => ({ + requireUserId: vi.fn(), + consumeState: vi.fn(), + redirectAfterAuth: vi.fn(), + createIntegration: vi.fn(), +})); + +vi.mock("~/services/session.server", () => ({ requireUserId: mocks.requireUserId })); +vi.mock("~/models/orgIntegration.server", () => ({ + OrgIntegrationRepository: { + consumeSlackOAuthState: mocks.consumeState, + redirectAfterAuth: mocks.redirectAfterAuth, + }, +})); +vi.mock("~/v3/services/createOrgIntegration.server", () => ({ + CreateOrgIntegrationService: class { + call = mocks.createIntegration; + }, +})); +vi.mock("~/utils/requestUrl.server", () => ({ + requestUrl: (request: Request) => new URL(request.url), +})); +const { loader } = await import("../app/routes/integrations.$serviceName.callback.js"); + +const request = () => + new Request("https://example.com/integrations/slack/callback?code=code_123&state=state_123"); +const args = () => ({ request: request(), params: { serviceName: "slack" } }) as any; + +beforeEach(() => { + vi.resetAllMocks(); + mocks.requireUserId.mockResolvedValue("user_123"); +}); + +describe("Slack OAuth callback", () => { + it("uses the consumed state scope before exchanging the authorization code", async () => { + mocks.consumeState.mockResolvedValue({ + organizationId: "org_123", + service: "slack", + redirectTo: "/orgs/acme/projects/app/env/prod/alerts/new/connect-to-slack", + }); + const response = new Response(null, { status: 302 }); + mocks.createIntegration.mockResolvedValue({ id: "integration_123" }); + mocks.redirectAfterAuth.mockResolvedValue(response); + + await expect(loader(args())).resolves.toBe(response); + expect(mocks.consumeState).toHaveBeenCalledWith(expect.any(Request), "state_123", "user_123"); + expect(mocks.createIntegration).toHaveBeenCalledWith( + "user_123", + "org_123", + "slack", + "code_123" + ); + expect(mocks.redirectAfterAuth).toHaveBeenCalledWith( + expect.any(Request), + "/orgs/acme/projects/app/env/prod/alerts/new/connect-to-slack" + ); + }); + + it("rejects invalid state before the authorization code exchange or integration writes", async () => { + mocks.consumeState.mockResolvedValue(undefined); + + await expect(loader(args())).rejects.toMatchObject({ status: 400 }); + expect(mocks.createIntegration).not.toHaveBeenCalled(); + expect(mocks.redirectAfterAuth).not.toHaveBeenCalled(); + }); + + it("clears the session binding and returns to the stored path when integration fails", async () => { + const redirectTo = "/orgs/acme/projects/app/env/prod/alerts/new/connect-to-slack"; + mocks.consumeState.mockResolvedValue({ + organizationId: "org_123", + service: "slack", + redirectTo, + }); + mocks.createIntegration.mockResolvedValue(undefined); + const response = new Response(null, { status: 302 }); + mocks.redirectAfterAuth.mockResolvedValue(response); + + await expect(loader(args())).resolves.toBe(response); + expect(mocks.redirectAfterAuth).toHaveBeenCalledWith( + expect.any(Request), + redirectTo, + "Failed to connect to the service" + ); + }); +}); diff --git a/apps/webapp/test/slackOAuthState.test.ts b/apps/webapp/test/slackOAuthState.test.ts new file mode 100644 index 00000000000..02e293fd2e3 --- /dev/null +++ b/apps/webapp/test/slackOAuthState.test.ts @@ -0,0 +1,205 @@ +import { redisTest } from "@internal/testcontainers"; +import Redis from "ioredis"; +import { describe, expect, vi } from "vitest"; +import { + clearSlackOAuthSessionBinding, + consumeSlackOAuthStateForSession, + createSlackOAuthStateForSession, + SlackOAuthStateStore, +} from "../app/models/slackOAuthState.server.js"; + +vi.setConfig({ testTimeout: 30_000 }); + +const state = { + userId: "user_123", + sessionBinding: "session_123", + organizationId: "org_123", + service: "slack" as const, + redirectTo: "/orgs/acme/projects/app/env/prod/alerts/new/connect-to-slack", +}; + +const expectedState = { + userId: state.userId, + sessionBinding: state.sessionBinding, + service: state.service, +}; + +const keyForNonce = (nonce: string) => `oauth:slack:state:{${nonce}}`; + +describe("SlackOAuthStateStore", () => { + redisTest( + "creates a cryptographically random, expiring state value", + async ({ redisOptions }) => { + const redis = new Redis(redisOptions); + + try { + const store = new SlackOAuthStateStore(redis); + const nonce = await store.create(state); + + expect(nonce).toMatch(/^[A-Za-z0-9_-]{43}$/); + await expect(redis.get(keyForNonce(nonce))).resolves.toBe(JSON.stringify(state)); + const ttl = await redis.ttl(keyForNonce(nonce)); + expect(ttl).toBeGreaterThan(0); + expect(ttl).toBeLessThanOrEqual(600); + } finally { + redis.disconnect(); + } + } + ); + + redisTest( + "atomically accepts one matching callback and rejects concurrent replays", + async ({ redisOptions }) => { + const redis = new Redis(redisOptions); + + try { + const store = new SlackOAuthStateStore(redis); + const nonce = await store.create(state); + const results = await Promise.all([ + store.consume(nonce, expectedState), + store.consume(nonce, expectedState), + ]); + + expect(results.filter((result) => result !== undefined)).toEqual([state]); + expect(results.filter((result) => result === undefined)).toHaveLength(1); + } finally { + redis.disconnect(); + } + } + ); + + redisTest("rejects mismatched state without consuming it", async ({ redisOptions }) => { + const redis = new Redis(redisOptions); + const mismatches = [ + { userId: "user_456", sessionBinding: state.sessionBinding, service: "slack" }, + { userId: state.userId, sessionBinding: "session_456", service: "slack" }, + { userId: state.userId, sessionBinding: state.sessionBinding, service: "vercel" }, + ]; + + try { + const store = new SlackOAuthStateStore(redis); + + for (const mismatch of mismatches) { + const nonce = await store.create(state); + await expect(store.consume(nonce, mismatch as any)).resolves.toBeUndefined(); + await expect(store.consume(nonce, expectedState)).resolves.toEqual(state); + } + } finally { + redis.disconnect(); + } + }); + + redisTest("rejects missing state and malformed nonces", async ({ redisOptions }) => { + const redis = new Redis(redisOptions); + + try { + const store = new SlackOAuthStateStore(redis); + const nonce = await store.create(state); + await redis.del(keyForNonce(nonce)); + + await expect(store.consume(nonce, expectedState)).resolves.toBeUndefined(); + await expect(store.consume("malformed-state", expectedState)).resolves.toBeUndefined(); + } finally { + redis.disconnect(); + } + }); + + redisTest("rejects corrupt stored state", async ({ redisOptions }) => { + const redis = new Redis(redisOptions); + + try { + const store = new SlackOAuthStateStore(redis); + const nonce = "a".repeat(43); + await redis.set(keyForNonce(nonce), "{not-json", "EX", 600); + + await expect(store.consume(nonce, expectedState)).resolves.toBeUndefined(); + } finally { + redis.disconnect(); + } + }); + + redisTest("rejects protocol-relative return paths", async ({ redisOptions }) => { + const redis = new Redis(redisOptions); + + try { + const store = new SlackOAuthStateStore(redis); + await expect(store.create({ ...state, redirectTo: "//example.com" })).rejects.toThrow(); + } finally { + redis.disconnect(); + } + }); + + redisTest("binds callbacks to the initiating browser session", async ({ redisOptions }) => { + const redis = new Redis(redisOptions); + + try { + const store = new SlackOAuthStateStore(redis); + const stateWithoutBinding = { + userId: state.userId, + organizationId: state.organizationId, + service: state.service, + redirectTo: state.redirectTo, + }; + const first = await createSlackOAuthStateForSession( + new Request("https://example.com/connect"), + stateWithoutBinding, + store + ); + const second = await createSlackOAuthStateForSession( + new Request("https://example.com/connect"), + stateWithoutBinding, + store + ); + const firstCookie = first.sessionCookie.split(";", 1)[0]; + const secondCookie = second.sessionCookie.split(";", 1)[0]; + const requestWithCookie = (cookie?: string) => + new Request("https://example.com/integrations/slack/callback", { + headers: cookie ? { Cookie: cookie } : undefined, + }); + + await expect( + consumeSlackOAuthStateForSession(requestWithCookie(), first.nonce, state.userId, store) + ).resolves.toBeUndefined(); + await expect( + consumeSlackOAuthStateForSession( + requestWithCookie(secondCookie), + first.nonce, + state.userId, + store + ) + ).resolves.toBeUndefined(); + + const consumedState = await consumeSlackOAuthStateForSession( + requestWithCookie(firstCookie), + first.nonce, + state.userId, + store + ); + expect(consumedState).toMatchObject({ + userId: state.userId, + organizationId: state.organizationId, + service: state.service, + redirectTo: state.redirectTo, + sessionBinding: expect.any(String), + }); + + const clearedCookie = ( + await clearSlackOAuthSessionBinding(requestWithCookie(firstCookie)) + ).split(";", 1)[0]; + const staleNonce = await store.create({ + ...state, + sessionBinding: consumedState!.sessionBinding, + }); + await expect( + consumeSlackOAuthStateForSession( + requestWithCookie(clearedCookie), + staleNonce, + state.userId, + store + ) + ).resolves.toBeUndefined(); + } finally { + redis.disconnect(); + } + }); +}); diff --git a/apps/webapp/test/waitpointCompleteRouteAuthorization.test.ts b/apps/webapp/test/waitpointCompleteRouteAuthorization.test.ts new file mode 100644 index 00000000000..4c56d5ebdbe --- /dev/null +++ b/apps/webapp/test/waitpointCompleteRouteAuthorization.test.ts @@ -0,0 +1,183 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const auth = vi.hoisted(() => ({ + userId: "user_attacker", + projectId: "proj_shared", + environmentId: "env_attacker", +})); + +const waitpointHolder = vi.hoisted(() => ({ + waitpoint: undefined as { id: string; projectId: string; environmentId: string } | undefined, + replicaCalls: [] as Array<{ where: Record }>, + primaryCalls: [] as Array<{ where: Record }>, +})); + +const engineHolder = vi.hoisted(() => ({ calls: [] as Array> })); + +function matchesWaitpoint(where: Record) { + const waitpoint = waitpointHolder.waitpoint; + if (!waitpoint || where.id !== waitpoint.id) return undefined; + if (where.projectId !== undefined && where.projectId !== waitpoint.projectId) return undefined; + if (where.environmentId !== undefined && where.environmentId !== waitpoint.environmentId) { + return undefined; + } + return waitpoint; +} + +vi.mock("~/v3/runStore.server", () => ({ + runStore: { + findWaitpoint: async ({ where }: { where: Record }) => { + waitpointHolder.replicaCalls.push({ where }); + return matchesWaitpoint(where); + }, + findWaitpointOnPrimary: async ({ where }: { where: Record }) => { + waitpointHolder.primaryCalls.push({ where }); + return matchesWaitpoint(where); + }, + }, +})); + +vi.mock("~/db.server", () => ({ + $replica: { + project: { + findUnique: async () => ({ id: auth.projectId }), + }, + }, +})); + +vi.mock("~/models/runtimeEnvironment.server", () => ({ + findEnvironmentBySlug: async () => ({ id: auth.environmentId }), +})); + +vi.mock("~/v3/runEngine.server", () => ({ + engine: { + completeWaitpoint: async (args: Record) => { + engineHolder.calls.push(args); + return { id: args.id }; + }, + }, +})); + +vi.mock("~/services/session.server", () => ({ + requireUserId: async () => auth.userId, +})); + +vi.mock("~/env.server", () => ({ + env: { TASK_PAYLOAD_MAXIMUM_SIZE: 3_000_000 }, +})); + +vi.mock("~/services/logger.server", () => ({ + logger: { error: () => {}, info: () => {}, debug: () => {}, warn: () => {} }, +})); + +vi.mock("~/models/message.server", () => ({ + redirectWithErrorMessage: (redirect: string, _request: Request, message: string) => + new Response(null, { + status: 302, + headers: { location: redirect, "x-outcome": "error", "x-message": message }, + }), + redirectWithSuccessMessage: (redirect: string, _request: Request, message: string) => + new Response(null, { + status: 302, + headers: { location: redirect, "x-outcome": "success", "x-message": message }, + }), +})); + +vi.mock("~/runEngine/concerns/waitpointCompletionPacket.server", () => ({ + processWaitpointCompletionPacket: async () => ({ data: undefined, dataType: "application/json" }), +})); + +import { WaitpointId } from "@trigger.dev/core/v3/isomorphic"; +import { action } from "~/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.waitpoints.$waitpointFriendlyId.complete/route"; + +function completeRequest(type: "DATETIME" | "MANUAL", isTimeout = false) { + const body = new URLSearchParams({ + type, + successRedirect: "/success", + failureRedirect: "/failure", + }); + if (type === "MANUAL") body.set("payload", "{}"); + if (isTimeout) body.set("isTimeout", "1"); + + return new Request("http://localhost/complete", { + method: "POST", + headers: { "content-type": "application/x-www-form-urlencoded" }, + body, + }); +} + +async function complete(friendlyId: string, type: "DATETIME" | "MANUAL", isTimeout = false) { + return (await action({ + request: completeRequest(type, isTimeout), + params: { + organizationSlug: "org-slug", + projectParam: "project-slug", + envParam: "dev", + waitpointFriendlyId: friendlyId, + }, + context: {} as never, + })) as Response; +} + +beforeEach(() => { + waitpointHolder.waitpoint = undefined; + waitpointHolder.replicaCalls = []; + waitpointHolder.primaryCalls = []; + engineHolder.calls = []; +}); + +describe("dashboard waitpoint completion authorization", () => { + it.each([ + { type: "DATETIME" as const, isTimeout: false }, + { type: "MANUAL" as const, isTimeout: true }, + ])( + "rejects $type completion for another development environment", + async ({ type, isTimeout }) => { + const { id, friendlyId } = WaitpointId.generate(); + waitpointHolder.waitpoint = { + id, + projectId: auth.projectId, + environmentId: "env_victim", + }; + + const response = await complete(friendlyId, type, isTimeout); + + expect(response.headers.get("x-outcome")).toBe("error"); + expect(response.headers.get("x-message")).toBe("No waitpoint found"); + expect(engineHolder.calls).toHaveLength(0); + expect(waitpointHolder.replicaCalls[0]?.where).toEqual({ + id, + projectId: auth.projectId, + environmentId: auth.environmentId, + }); + expect(waitpointHolder.primaryCalls[0]?.where).toEqual({ + id, + projectId: auth.projectId, + environmentId: auth.environmentId, + }); + } + ); + + it.each([ + { type: "DATETIME" as const, isTimeout: false, message: "Waitpoint skipped" }, + { type: "MANUAL" as const, isTimeout: true, message: "Waitpoint timed out" }, + ])( + "allows $type completion in the authorized environment", + async ({ type, isTimeout, message }) => { + const { id, friendlyId } = WaitpointId.generate(); + waitpointHolder.waitpoint = { + id, + projectId: auth.projectId, + environmentId: auth.environmentId, + }; + + const response = await complete(friendlyId, type, isTimeout); + + expect(response.headers.get("x-outcome")).toBe("success"); + expect(response.headers.get("x-message")).toBe(message); + expect(engineHolder.calls).toHaveLength(1); + expect(engineHolder.calls[0]?.id).toBe(id); + expect(waitpointHolder.primaryCalls).toHaveLength(0); + } + ); +}); diff --git a/apps/webapp/test/waitpointCompleteRouteReplicaLag.guard.test.ts b/apps/webapp/test/waitpointCompleteRouteReplicaLag.guard.test.ts index 438d1a94dcc..0f9bda65b3d 100644 --- a/apps/webapp/test/waitpointCompleteRouteReplicaLag.guard.test.ts +++ b/apps/webapp/test/waitpointCompleteRouteReplicaLag.guard.test.ts @@ -1,7 +1,7 @@ // Property: under split replica lag the dashboard "complete waitpoint" route action still completes a -// just-minted token. It resolves the waitpoint by id via findWaitpoint (owning REPLICA), and on a null -// re-reads via findWaitpointOnPrimary before the projectId guard, so a token invisible on the lagging -// replica passes the guard and completion proceeds instead of failing with "No waitpoint found". +// just-minted token. It resolves the waitpoint by project and environment via findWaitpoint (owning +// REPLICA), and on a null re-reads via findWaitpointOnPrimary before returning "No waitpoint found". +// A token invisible on the lagging replica still completes after the primary fallback. // Drives the REAL exported action; only peripheral collaborators are mocked. The seam — runStore over a // split topology whose owning replica is frozen — is a REAL RoutingRunStore over real testcontainer // Postgres. @@ -64,8 +64,8 @@ vi.mock("~/models/message.server", () => ({ }), })); -// MANUAL-branch collaborators — the token completion path resolves the env then completes. Return an -// env whose id matches the seeded waitpoint's environmentId so the env guard passes. +// The completion path resolves the environment before loading the waitpoint. Return an environment +// whose id matches the seeded waitpoint so the scoped lookup can authorize it. const envHolder = vi.hoisted(() => ({ id: undefined as string | undefined })); vi.mock("~/models/runtimeEnvironment.server", () => ({ findEnvironmentBySlug: async () => (envHolder.id ? { id: envHolder.id } : null), @@ -147,8 +147,8 @@ const params = (friendlyId: string) => ({ describe("complete-waitpoint dashboard route reads-your-writes under split replica lag", () => { // LEGACY-resident (cuid) token minted on the control-plane writer; its replica lags. The action's - // findWaitpoint(id) misses, and the findWaitpointOnPrimary fallback must resolve it so the just- - // minted token passes the projectId guard and completes — NOT "No waitpoint found". + // environment-scoped findWaitpoint misses, and the findWaitpointOnPrimary fallback must resolve it + // so the just-minted token completes instead of returning "No waitpoint found". heteroRunOpsPostgresTest( "MANUAL token invisible on the lagging owning replica completes via the primary fallback", async ({ prisma14, prisma17 }) => { @@ -200,8 +200,8 @@ describe("complete-waitpoint dashboard route reads-your-writes under split repli } ); - // Same seam via the DATETIME "skip" branch (also gated by the shared projectId guard). Kept as a - // second, mock-light assertion of the fallback so the guard doesn't hinge on MANUAL-branch helpers. + // Same seam via the DATETIME "skip" branch. Kept as a second, mock-light assertion of the fallback + // so the authorization behavior doesn't hinge on MANUAL-branch helpers. heteroRunOpsPostgresTest( "DATETIME skip on a lag-invisible token resolves via the primary fallback", async ({ prisma14, prisma17 }) => { diff --git a/internal-packages/dashboard-agent/src/agent-runtime.ts b/internal-packages/dashboard-agent/src/agent-runtime.ts index 0058f04bb75..0f21235e373 100644 --- a/internal-packages/dashboard-agent/src/agent-runtime.ts +++ b/internal-packages/dashboard-agent/src/agent-runtime.ts @@ -271,6 +271,18 @@ export const dashboardAgentToolsKey = locals.create("dashboard-agent.to // within a recycle. type DashboardAgentMode = "assistant" | "code"; +// The snapshot is fetched and extracted on the agent worker, so its URL must be one the +// server would have minted: plain https. The host check lives in repo-tools' fetch. +// Guarded at the schema edge too because old workers (the deployed version is pinned) +// can still replay metadata that carries a snapshot. +const repoSnapshotTarballUrlSchema = z.string().refine((value) => { + try { + return new URL(value).protocol === "https:"; + } catch { + return false; + } +}, "tarballUrl must be an https URL"); + // A turn is in `code` mode when the project has a connected repo. Drives both the // tool set and the prompt. export function modeFor(clientData: { repoSnapshot?: unknown } | undefined): DashboardAgentMode { @@ -312,7 +324,7 @@ export const clientDataSchema = z.object({ // short-lived archive pointer the code-mode source tools read from. repoSnapshot: z .object({ - tarballUrl: z.string(), + tarballUrl: repoSnapshotTarballUrlSchema, owner: z.string(), repo: z.string(), sha: z.string(), diff --git a/internal-packages/dashboard-agent/src/dashboard-agent.test.ts b/internal-packages/dashboard-agent/src/dashboard-agent.test.ts index 0419a525b20..b8a9d508efb 100644 --- a/internal-packages/dashboard-agent/src/dashboard-agent.test.ts +++ b/internal-packages/dashboard-agent/src/dashboard-agent.test.ts @@ -1126,6 +1126,36 @@ describe("clientDataSchema", () => { }); expect(parsed.success).toBe(false); }); + + // The snapshot URL is fetched and extracted on the worker, so the schema refuses + // anything but plain https (the host allowlist is enforced at the fetch site). + it("rejects a repoSnapshot whose tarballUrl is not an https URL", () => { + for (const tarballUrl of [ + "http://codeload.github.com/acme/demo/tar.gz/abc", + "ftp://example.com/x.tar.gz", + "file:///etc/passwd", + "not a url", + ]) { + const parsed = clientDataSchema.safeParse({ + userId: "user_1", + organizationId: "org_1", + repoSnapshot: { tarballUrl, owner: "acme", repo: "demo", sha: "c".repeat(40) }, + }); + expect(parsed.success).toBe(false); + } + + const ok = clientDataSchema.safeParse({ + userId: "user_1", + organizationId: "org_1", + repoSnapshot: { + tarballUrl: "https://codeload.github.com/acme/demo/tar.gz/abc", + owner: "acme", + repo: "demo", + sha: "c".repeat(40), + }, + }); + expect(ok.success).toBe(true); + }); }); describe("buildDashboardAgentTools", () => { diff --git a/internal-packages/dashboard-agent/src/repo-tools.test.ts b/internal-packages/dashboard-agent/src/repo-tools.test.ts index 5d9579b40fe..6030ef72470 100644 --- a/internal-packages/dashboard-agent/src/repo-tools.test.ts +++ b/internal-packages/dashboard-agent/src/repo-tools.test.ts @@ -194,6 +194,27 @@ describe("repo-tools", () => { expect(res.error).toMatch(/Couldn't resolve the source/); }); + // The snapshot fetch runs on an internal worker, so only GitHub's archive host is + // allowed; anything else must fail before a request leaves the worker. + it("refuses to fetch a snapshot whose tarballUrl is not an allowed host", async () => { + for (const tarballUrl of [ + "http://codeload.github.com/acme/attacker/tar.gz/abc", + "https://attacker.example.com/x.tar.gz", + "https://github.com.evil.example/x.tar.gz", + "not a url", + ]) { + const bad: RepoSnapshot = { + tarballUrl, + owner: "acme", + repo: "attacker", + sha: "b".repeat(40), + }; + const res: any = await call(buildRepoTools(bad).read_file, { path: "README.md" }); + expect(res.error).toMatch(/Couldn't load the repository/); + expect(res.error).toMatch(/not a valid URL|not allowed/); + } + }); + it.runIf(hasRg)("search_code finds a match (and does not hang on stdin)", async () => { const res: any = await call(tools.search_code, { query: "const LIMIT" }); expect(res.error).toBeUndefined(); diff --git a/internal-packages/dashboard-agent/src/repo-tools.ts b/internal-packages/dashboard-agent/src/repo-tools.ts index e02c91dba05..e5645f0d8f3 100644 --- a/internal-packages/dashboard-agent/src/repo-tools.ts +++ b/internal-packages/dashboard-agent/src/repo-tools.ts @@ -39,6 +39,24 @@ export type RepoSnapshot = { }; const MAX_ARCHIVE_BYTES = 100 * 1024 * 1024; // 100MB ceiling on the download + +// The snapshot fetch is a network primitive on an internal worker, so the URL must be one +// the webapp would have minted: https to GitHub's own archive hosts (the signed redirect +// target of the `tarball` API is codeload). A validation failure is thrown before any +// request leaves the worker. +const ALLOWED_TARBALL_HOSTS = new Set(["codeload.github.com"]); + +function assertAllowedTarballUrl(tarballUrl: string): void { + let url: URL; + try { + url = new URL(tarballUrl); + } catch { + throw new Error("repo snapshot URL is not a valid URL"); + } + if (url.protocol !== "https:" || !ALLOWED_TARBALL_HOSTS.has(url.hostname)) { + throw new Error(`repo snapshot URL host is not allowed (${url.hostname || "unparseable"})`); + } +} // A tool result the model has to pay for on every later turn of the conversation: // 48KB is ~12k tokens, where the old 256KB ceiling was ~65k. export const MAX_READ_BYTES = 48 * 1024; @@ -101,7 +119,13 @@ async function ensureWorkspace(snapshot: RepoSnapshot): Promise { const timer = setTimeout(() => controller.abort(), FETCH_TIMEOUT_MS); let tarPath: string | undefined; try { - const res = await fetch(snapshot.tarballUrl, { signal: controller.signal }); + assertAllowedTarballUrl(snapshot.tarballUrl); + // No redirects: the server resolves the one hop to a signed codeload URL itself, so + // following one here would just reopen the host check to a redirect target. + const res = await fetch(snapshot.tarballUrl, { + signal: controller.signal, + redirect: "error", + }); if (!res.ok) throw new Error(`archive download failed (status ${res.status})`); const length = Number(res.headers.get("content-length") ?? 0); if (length > MAX_ARCHIVE_BYTES) throw new Error(`archive too large (${length} bytes)`); @@ -113,6 +137,29 @@ async function ensureWorkspace(snapshot: RepoSnapshot): Promise { tarPath = join(scratch, "repo.tar.gz"); await writeFile(tarPath, bytes); + // List the archive's top-level entries before extracting: anything that resolves + // outside the root after `--strip-components=1` (`..` segments, absolute paths) + // rejects the archive. Both bsdtar and GNU tar refuse `..` members outright; this + // rejects rather than relying on the extractor, and the read tools' realpath checks + // keep any surviving symlink member from pointing a read outside the root. + const { stdout: listing } = await execFileAsync("tar", ["-tzf", tarPath], { + // The listing is a line per member; a 100MB archive can't hold more members + // than this fits at any plausible path length. + maxBuffer: 64 * 1024 * 1024, + }); + const roots = new Set(); + for (const entry of listing.split("\n")) { + const name = entry.replace(/\/+$/, ""); + if (!name) continue; + if (isAbsolute(name) || name.split("/").includes("..")) { + throw new Error(`archive entry escapes the workspace (${name})`); + } + roots.add(name.split("/")[0]); + if (roots.size > 1) { + throw new Error("archive has more than one top-level entry; refusing to extract"); + } + } + await mkdir(workdir, { recursive: true }); await execFileAsync("tar", ["-xzf", tarPath, "-C", workdir, "--strip-components=1"]); await writeFile(join(workdir, ".ready"), snapshot.sha); diff --git a/internal-packages/tsql/src/query/printer.ts b/internal-packages/tsql/src/query/printer.ts index adfa62cedb0..ba05bdbb974 100644 --- a/internal-packages/tsql/src/query/printer.ts +++ b/internal-packages/tsql/src/query/printer.ts @@ -429,8 +429,15 @@ export class ClickHousePrinter { windowClause = windowDefs.join(", "); } + // PREWHERE runs before enforced tenant conditions in WHERE, so it cannot be + // exposed to customer-authored queries. + if (node.prewhere) { + throw new QueryError("PREWHERE is not supported. Use WHERE instead.", { + node: node.prewhere, + }); + } + // Process other clauses - const prewhere = node.prewhere ? this.visit(node.prewhere) : null; const whereStr = where ? this.visit(where) : null; // Process GROUP BY with context flags: @@ -502,7 +509,6 @@ export class ClickHousePrinter { `SELECT${space}${node.distinct ? "DISTINCT " : ""}${columns.join(comma)}`, joinedTables.length > 0 ? `FROM${space}${joinedTables.join(space)}` : null, arrayJoin || null, - prewhere ? `PREWHERE${space}${prewhere}` : null, whereStr ? `WHERE${space}${whereStr}` : null, groupBy && groupBy.length > 0 ? `GROUP BY${space}${groupBy.join(comma)}` : null, having ? `HAVING${space}${having}` : null, diff --git a/internal-packages/tsql/src/query/security.test.ts b/internal-packages/tsql/src/query/security.test.ts index 68853ec23fe..42aa2cc79cb 100644 --- a/internal-packages/tsql/src/query/security.test.ts +++ b/internal-packages/tsql/src/query/security.test.ts @@ -191,6 +191,30 @@ describe("Cross-Tenant Security", () => { }); }); + describe("PREWHERE", () => { + it.each([ + ["top-level query", "SELECT count(*) FROM task_runs PREWHERE toUInt8(task_identifier) = 1"], + [ + "case and whitespace variant", + "SELECT count(*) FROM task_runs\nPrEwHeRe\n toUInt8(task_identifier) = 1", + ], + [ + "nested query", + `SELECT id FROM task_runs WHERE id IN ( + SELECT run_id FROM task_events PREWHERE event_type = 'completed' + )`, + ], + [ + "set query", + `SELECT id FROM task_runs + UNION ALL + SELECT id FROM task_runs PREWHERE status = 'completed'`, + ], + ])("should reject PREWHERE in a %s", (_, query) => { + expect(() => compile(query)).toThrowError("PREWHERE is not supported. Use WHERE instead."); + }); + }); + describe("Table allowlisting", () => { it("should reject queries to unknown tables", () => { expect(() => {