diff --git a/web-admin/src/features/billing/plans/upgrade-to-plan.spec.ts b/web-admin/src/features/billing/plans/upgrade-to-plan.spec.ts new file mode 100644 index 000000000000..0294fe882bfc --- /dev/null +++ b/web-admin/src/features/billing/plans/upgrade-to-plan.spec.ts @@ -0,0 +1,219 @@ +import { describe, expect, it, vi } from "vitest"; +import type { CategorisedOrganizationBillingIssues } from "../selectors"; +import { + createUpgradeToPlanAction, + type UpgradeToPlanDependencies, +} from "./upgrade-to-plan"; + +const CURRENT_URL = "https://app.example/acme/settings"; + +function issues( + overrides: Partial = {}, +): CategorisedOrganizationBillingIssues { + return { + payment: [], + needsPaymentSetup: false, + ...overrides, + }; +} + +function dependencies( + overrides: Partial = {}, +): UpgradeToPlanDependencies { + return { + getPage: vi.fn( + () => + ({ url: new URL(CURRENT_URL) }) as ReturnType< + UpgradeToPlanDependencies["getPage"] + >, + ), + getCurrentUrl: vi.fn(() => CURRENT_URL), + getAllowedRedirectOrigins: vi.fn(() => ["https://developer.example"]), + fetchPaymentPortalUrl: vi + .fn() + .mockResolvedValue("https://payments.example/session"), + fetchPlan: vi.fn().mockResolvedValue({ displayName: "Growth" }), + renew: vi.fn().mockResolvedValue(undefined), + update: vi.fn().mockResolvedValue(undefined), + notifyRenewed: vi.fn(), + showWelcome: vi.fn(), + invalidate: vi.fn(), + open: vi.fn(), + ...overrides, + }; +} + +describe("upgradeToPlan", () => { + it("requests one short-lived portal URL when payment setup is required", async () => { + // Payment issues must take the portal path and skip plan mutations. + const deps = dependencies(); + const upgrade = createUpgradeToPlanAction(deps); + + await upgrade( + "acme", + "growth", + issues({ payment: [{}] as never[], needsPaymentSetup: true }), + null, + ); + + expect(deps.fetchPaymentPortalUrl).toHaveBeenCalledOnce(); + expect(deps.fetchPaymentPortalUrl).toHaveBeenCalledWith( + "acme", + "https://app.example/acme/-/upgrade-callback?plan=growth", + true, + ); + expect(deps.open).toHaveBeenCalledWith("https://payments.example/session"); + expect(deps.fetchPlan).not.toHaveBeenCalled(); + expect(deps.invalidate).not.toHaveBeenCalled(); + }); + + it("does not navigate when the payment portal request fails", async () => { + // A failed portal lookup must not trigger any post-request effects. + const deps = dependencies({ + fetchPaymentPortalUrl: vi.fn().mockRejectedValue(new Error("offline")), + }); + const upgrade = createUpgradeToPlanAction(deps); + + await expect( + upgrade("acme", "growth", issues({ payment: [{}] as never[] }), null), + ).rejects.toThrow("offline"); + + expect(deps.open).not.toHaveBeenCalled(); + expect(deps.invalidate).not.toHaveBeenCalled(); + }); + + it("renews a cancelled plan and reports success", async () => { + // Cancelled subscriptions renew instead of issuing an update. + const deps = dependencies(); + const upgrade = createUpgradeToPlanAction(deps); + + await upgrade("acme", "growth", issues({ cancelled: {} as never }), null); + + expect(deps.renew).toHaveBeenCalledOnce(); + expect(deps.renew).toHaveBeenCalledWith("acme", "growth"); + expect(deps.update).not.toHaveBeenCalled(); + expect(deps.notifyRenewed).toHaveBeenCalledWith("Growth"); + expect(deps.invalidate).toHaveBeenCalledWith("acme"); + }); + + it("leaves post-mutation effects untouched when renewal fails", async () => { + // Renewal errors must stop notification, invalidation, and navigation. + const deps = dependencies({ + renew: vi.fn().mockRejectedValue(new Error("renew failed")), + }); + const upgrade = createUpgradeToPlanAction(deps); + + await expect( + upgrade( + "acme", + "growth", + issues({ cancelled: {} as never }), + "https://app.example/return", + ), + ).rejects.toThrow("renew failed"); + + expect(deps.notifyRenewed).not.toHaveBeenCalled(); + expect(deps.invalidate).not.toHaveBeenCalled(); + expect(deps.open).not.toHaveBeenCalled(); + }); + + it("updates a valid plan and shows the welcome state", async () => { + // A standard upgrade updates the plan and shows the default success UI. + const deps = dependencies(); + const upgrade = createUpgradeToPlanAction(deps); + + await upgrade("acme", "growth", issues(), null); + + expect(deps.update).toHaveBeenCalledWith("acme", "growth"); + expect(deps.showWelcome).toHaveBeenCalledWith("growth"); + expect(deps.invalidate).toHaveBeenCalledWith("acme"); + }); + + it("surfaces update failure and permits a clean retry", async () => { + // Failed mutations release the in-flight guard for a later retry. + const update = vi + .fn() + .mockRejectedValueOnce(new Error("update failed")) + .mockResolvedValueOnce(undefined); + const deps = dependencies({ update }); + const upgrade = createUpgradeToPlanAction(deps); + + await expect( + upgrade("acme", "growth", issues(), "https://app.example/return"), + ).rejects.toThrow("update failed"); + expect(deps.invalidate).not.toHaveBeenCalled(); + expect(deps.open).not.toHaveBeenCalled(); + + await upgrade("acme", "growth", issues(), null); + expect(update).toHaveBeenCalledTimes(2); + expect(deps.invalidate).toHaveBeenCalledOnce(); + }); + + it("rejects an invalid plan before mutation", async () => { + // Missing plan metadata must fail before any billing write. + const deps = dependencies({ + fetchPlan: vi.fn().mockResolvedValue(undefined), + }); + const upgrade = createUpgradeToPlanAction(deps); + + await expect(upgrade("acme", "bogus", issues(), null)).rejects.toThrow( + "Plan bogus not found", + ); + + expect(deps.renew).not.toHaveBeenCalled(); + expect(deps.update).not.toHaveBeenCalled(); + expect(deps.invalidate).not.toHaveBeenCalled(); + }); + + it.each([ + ["same-origin", "https://app.example/return"], + ["configured origin", "https://developer.example/return"], + ])("opens an approved %s redirect", async (_name, redirect) => { + // Exact same-origin and configured-origin redirects are allowed. + const deps = dependencies(); + const upgrade = createUpgradeToPlanAction(deps); + + await upgrade("acme", "growth", issues(), redirect); + + expect(deps.open).toHaveBeenCalledWith(redirect); + expect(deps.showWelcome).not.toHaveBeenCalled(); + }); + + it("rejects a lookalike external redirect", async () => { + // Hostname lookalikes must fall back to the normal welcome state. + const deps = dependencies(); + const upgrade = createUpgradeToPlanAction(deps); + + await upgrade( + "acme", + "growth", + issues(), + "https://developer.example.evil.test/return", + ); + + expect(deps.open).not.toHaveBeenCalled(); + expect(deps.showWelcome).toHaveBeenCalledWith("growth"); + }); + + it("coalesces repeated clicks into one mutation", async () => { + // Concurrent clicks must share one request and one invalidation. + let resolveUpdate!: () => void; + const update = vi.fn( + () => + new Promise((resolve) => { + resolveUpdate = resolve; + }), + ); + const deps = dependencies({ update }); + const upgrade = createUpgradeToPlanAction(deps); + + const first = upgrade("acme", "growth", issues(), null); + const second = upgrade("acme", "growth", issues(), null); + await vi.waitFor(() => expect(update).toHaveBeenCalledOnce()); + + expect(second).toBe(first); + resolveUpdate(); + await Promise.all([first, second]); + expect(deps.invalidate).toHaveBeenCalledOnce(); + }); +}); diff --git a/web-admin/src/features/billing/plans/upgrade-to-plan.ts b/web-admin/src/features/billing/plans/upgrade-to-plan.ts index 41a9ab0a4545..a59264b76561 100644 --- a/web-admin/src/features/billing/plans/upgrade-to-plan.ts +++ b/web-admin/src/features/billing/plans/upgrade-to-plan.ts @@ -14,47 +14,129 @@ import { triggerWelcomeToRillDialog } from "@rilldata/web-admin/features/billing import { invalidateBillingInfo } from "@rilldata/web-admin/features/billing/invalidations.ts"; import { page } from "$app/stores"; import { get } from "svelte/store"; +import { + getConfiguredRedirectOrigins, + getSafeRedirectTarget, +} from "@rilldata/web-admin/lib/safe-redirect.ts"; + +type UpgradePlan = { + displayName?: string | null; +}; + +export type UpgradeToPlanDependencies = { + getPage: () => Parameters[0]; + getCurrentUrl: () => string; + getAllowedRedirectOrigins: () => readonly string[]; + fetchPaymentPortalUrl: ( + org: string, + returnUrl: string, + setup: boolean, + ) => Promise; + fetchPlan: (planName: string) => Promise; + renew: (org: string, planName: string) => Promise; + update: (org: string, planName: string) => Promise; + notifyRenewed: (displayName: string) => void; + showWelcome: (planName: string) => void; + invalidate: (org: string) => unknown; + open: (url: string) => void; +}; + +const defaultDependencies: UpgradeToPlanDependencies = { + getPage: () => get(page), + getCurrentUrl: () => window.location.href, + getAllowedRedirectOrigins: getConfiguredRedirectOrigins, + fetchPaymentPortalUrl: fetchPaymentsPortalURL, + fetchPlan: maybeFetchPublicPlanByName, + renew: (org, planName) => + adminServiceRenewBillingSubscription(org, { planName }), + update: (org, planName) => + adminServiceUpdateBillingSubscription(org, { planName }), + notifyRenewed: (displayName) => + eventBus.emit("notification", { + type: "success", + message: m.billing_plan_renewed({ planName: displayName }), + }), + showWelcome: triggerWelcomeToRillDialog, + invalidate: invalidateBillingInfo, + open: (url) => { + window.open(url, "_self"); + }, +}; -export async function upgradeToPlan( +async function performUpgradeToPlan( + dependencies: UpgradeToPlanDependencies, org: string, planName: string, categorisedIssues: CategorisedOrganizationBillingIssues, redirect: string | null, ) { if (categorisedIssues.payment.length > 0) { - // Payment setup is required first. Carry the chosen plan through the Stripe - // return URL so the upgrade-callback page can complete the upgrade. - window.open( - await fetchPaymentsPortalURL( - org, - getBillingUpgradeUrl(get(page), org, planName), - categorisedIssues.needsPaymentSetup, - ), - "_self", + // The server-issued portal URL is short-lived and signed, so only the + // winning in-flight request may fetch and consume it. + const portalUrl = await dependencies.fetchPaymentPortalUrl( + org, + getBillingUpgradeUrl(dependencies.getPage(), org, planName), + categorisedIssues.needsPaymentSetup, ); + if (!portalUrl) throw new Error("Payment portal URL is unavailable"); + dependencies.open(portalUrl); return; } - const plan = await maybeFetchPublicPlanByName(planName); - if (!plan) return; + const plan = await dependencies.fetchPlan(planName); + if (!plan) throw new Error(`Plan ${planName} not found`); + + const safeRedirect = getSafeRedirectTarget( + redirect, + dependencies.getCurrentUrl(), + { allowedOrigins: dependencies.getAllowedRedirectOrigins() }, + ); + if (categorisedIssues.cancelled) { - await adminServiceRenewBillingSubscription(org, { - planName, - }); - eventBus.emit("notification", { - type: "success", - message: m.billing_plan_renewed({ planName: plan.displayName ?? "" }), - }); + await dependencies.renew(org, planName); + dependencies.notifyRenewed(plan.displayName ?? ""); } else { - await adminServiceUpdateBillingSubscription(org, { + await dependencies.update(org, planName); + // A rejected redirect behaves like no redirect, so the successful upgrade + // still gets its normal welcome state. + if (!safeRedirect) dependencies.showWelcome(planName); + } + + // Mutation success is the commit point. Failed requests never invalidate + // cached billing data or navigate away from the error shown by the dialog. + void dependencies.invalidate(org); + if (safeRedirect) dependencies.open(safeRedirect); +} + +export function createUpgradeToPlanAction( + dependencies: UpgradeToPlanDependencies, +) { + const inFlight = new Map>(); + + return function runUpgradeToPlan( + org: string, + planName: string, + categorisedIssues: CategorisedOrganizationBillingIssues, + redirect: string | null, + ): Promise { + const key = JSON.stringify([org, planName]); + const activeRequest = inFlight.get(key); + if (activeRequest !== undefined) return activeRequest; + + const request = performUpgradeToPlan( + dependencies, + org, planName, + categorisedIssues, + redirect, + ).finally(() => { + // Release both successes and failures; a failed attempt can be retried, + // while rapid repeated clicks still share exactly one mutation. + if (inFlight.get(key) === request) inFlight.delete(key); }); - triggerWelcomeToRillDialog(planName); - } - void invalidateBillingInfo(org); - if (redirect) { - // redirect param could be on a different domain like the rill developer instance - // so using goto won't work - window.open(redirect, "_self"); - } + inFlight.set(key, request); + return request; + }; } + +export const upgradeToPlan = createUpgradeToPlanAction(defaultDependencies); diff --git a/web-admin/src/lib/safe-redirect.ts b/web-admin/src/lib/safe-redirect.ts new file mode 100644 index 000000000000..798174f2196e --- /dev/null +++ b/web-admin/src/lib/safe-redirect.ts @@ -0,0 +1,68 @@ +export type RedirectPolicy = { + allowedOrigins?: readonly string[]; + allowLoopback?: boolean; +}; + +const LOOPBACK_HOSTS = new Set(["localhost", "127.0.0.1", "[::1]"]); + +/** + * Resolves redirects against the current page and fails closed. A destination + * must be same-origin, explicitly configured, or (when requested) an HTTP + * loopback callback used by a local client. + */ +export function getSafeRedirectTarget( + redirect: string | null | undefined, + currentUrl: string, + policy: RedirectPolicy = {}, +): string | null { + if (!redirect?.trim()) return null; + + try { + const current = new URL(currentUrl); + const target = new URL(redirect, current); + + if ( + (target.protocol !== "http:" && target.protocol !== "https:") || + target.username || + target.password + ) { + return null; + } + + if (target.origin === current.origin) return target.toString(); + + const allowedOrigins = new Set( + (policy.allowedOrigins ?? []).flatMap((origin) => { + try { + return [new URL(origin).origin]; + } catch { + return []; + } + }), + ); + if (allowedOrigins.has(target.origin)) return target.toString(); + + if ( + policy.allowLoopback && + target.protocol === "http:" && + LOOPBACK_HOSTS.has(target.hostname) + ) { + return target.toString(); + } + } catch { + // Malformed input is an unapproved destination, not a navigation request. + } + + return null; +} + +/** Exact origins in this build-time allowlist may receive browser redirects. */ +export function getConfiguredRedirectOrigins( + value: string | undefined = import.meta.env + .RILL_UI_PUBLIC_ALLOWED_REDIRECT_ORIGINS as string | undefined, +): string[] { + return (value ?? "") + .split(",") + .map((origin) => origin.trim()) + .filter(Boolean); +} diff --git a/web-admin/src/routes/-/auth/device/+page.svelte b/web-admin/src/routes/-/auth/device/+page.svelte index 1478c9b0e285..836f95700dba 100644 --- a/web-admin/src/routes/-/auth/device/+page.svelte +++ b/web-admin/src/routes/-/auth/device/+page.svelte @@ -1,5 +1,7 @@ @@ -83,7 +62,7 @@ {m.auth_authorize_rill_cli()}

- {m.auth_authenticating_as({ email: user.email })}
{m.auth_confirm_code_displayed()}

{ - actionTaken = true; - confirmUserCode(); - }} - disabled={actionTaken}>{m.auth_confirm_code()} submit(true)} + disabled={inFlight || completed}>{m.auth_confirm_code()} { - actionTaken = true; - rejectUserCode(); - }} - disabled={actionTaken}>{m.common_cancel()} submit(false)} + disabled={inFlight || completed}>{m.common_cancel()}
@@ -115,7 +88,9 @@

{successMsg}

{/if} {#if errorMsg} -

{errorMsg}

+ {/if} diff --git a/web-admin/src/routes/-/auth/device/device-authorization-action.ts b/web-admin/src/routes/-/auth/device/device-authorization-action.ts new file mode 100644 index 000000000000..89d5d0854c6d --- /dev/null +++ b/web-admin/src/routes/-/auth/device/device-authorization-action.ts @@ -0,0 +1,186 @@ +import { getSafeRedirectTarget } from "@rilldata/web-admin/lib/safe-redirect.ts"; + +export type DeviceAuthorizationInput = { + userCode: string | null; + confirmed: boolean; + redirect: string | null; + currentUrl: string; +}; + +export type DeviceAuthorizationState = { + inFlight: boolean; + completed: boolean; + successMessage: string; + errorMessage: string; +}; + +export type DeviceAuthorizationMessages = { + confirmed: string; + rejected: string; + confirmationFailed: string; + rejectionFailed: string; + invalidCode: string; + unsafeRedirect: string; +}; + +type DeviceAuthorizationResponse = { + ok: boolean; + text: () => Promise; +}; + +export type DeviceAuthorizationDependencies = { + adminUrl: string; + fetch: ( + input: string, + init: RequestInit, + ) => Promise; + navigate: (url: string) => void; + messages: DeviceAuthorizationMessages; + onState: (state: DeviceAuthorizationState) => void; +}; + +export function isValidDeviceUserCode(code: string | null): code is string { + return !!code && code.length <= 128 && /^[A-Za-z0-9_-]+$/.test(code); +} + +export function buildDeviceAuthorizationRequestUrl( + adminUrl: string, + userCode: string, + confirmed: boolean, +): string { + const requestUrl = new URL(adminUrl); + const basePath = requestUrl.pathname.replace(/\/+$/, ""); + requestUrl.pathname = `${basePath}/auth/oauth/device`; + requestUrl.search = ""; + requestUrl.hash = ""; + + // URLSearchParams owns encoding, so user input can never add or override a + // request parameter through string concatenation. + requestUrl.searchParams.set("user_code", userCode); + requestUrl.searchParams.set("code_confirmed", String(confirmed)); + return requestUrl.toString(); +} + +function withDetail(message: string, detail: string) { + const trimmedDetail = detail.trim(); + return trimmedDetail ? `${message}: ${trimmedDetail}` : message; +} + +async function performDeviceAuthorization( + input: DeviceAuthorizationInput, + dependencies: DeviceAuthorizationDependencies, +): Promise> { + const failureMessage = input.confirmed + ? dependencies.messages.confirmationFailed + : dependencies.messages.rejectionFailed; + + if (!isValidDeviceUserCode(input.userCode)) { + throw new Error(dependencies.messages.invalidCode); + } + + let response: DeviceAuthorizationResponse; + try { + response = await dependencies.fetch( + buildDeviceAuthorizationRequestUrl( + dependencies.adminUrl, + input.userCode, + input.confirmed, + ), + { + method: "POST", + credentials: "include", + }, + ); + } catch (error) { + const detail = error instanceof Error ? error.message : String(error); + throw new Error(withDetail(failureMessage, detail)); + } + + if (!response.ok) { + let detail = ""; + try { + detail = await response.text(); + } catch { + // The status still provides a useful failure when the body is unreadable. + } + throw new Error(withDetail(failureMessage, detail)); + } + + if (!input.confirmed) { + return { + successMessage: dependencies.messages.rejected, + errorMessage: "", + }; + } + + if (input.redirect) { + const safeRedirect = getSafeRedirectTarget( + input.redirect, + input.currentUrl, + { allowLoopback: true }, + ); + if (safeRedirect) { + dependencies.navigate(safeRedirect); + } else { + return { + successMessage: dependencies.messages.confirmed, + errorMessage: dependencies.messages.unsafeRedirect, + }; + } + } + + return { + successMessage: dependencies.messages.confirmed, + errorMessage: "", + }; +} + +export function createDeviceAuthorizationAction( + dependencies: DeviceAuthorizationDependencies, +) { + let inFlight: Promise | null = null; + let completed = false; + + return function authorizeDevice( + input: DeviceAuthorizationInput, + ): Promise { + // A pending or completed decision owns the device code. Repeated clicks + // must not send a second confirm/reject request. + if (inFlight !== null) return inFlight; + if (completed) return Promise.resolve(); + + dependencies.onState({ + inFlight: true, + completed: false, + successMessage: "", + errorMessage: "", + }); + + const request = performDeviceAuthorization(input, dependencies) + .then(({ successMessage, errorMessage }) => { + completed = true; + dependencies.onState({ + inFlight: false, + completed: true, + successMessage, + errorMessage, + }); + }) + .catch((error) => { + dependencies.onState({ + inFlight: false, + completed: false, + successMessage: "", + errorMessage: error instanceof Error ? error.message : String(error), + }); + }) + .finally(() => { + // Request failures unlock the buttons for a deliberate retry; success + // remains completed and cannot be submitted again. + if (inFlight === request) inFlight = null; + }); + + inFlight = request; + return request; + }; +} diff --git a/web-admin/src/routes/-/auth/device/page.spec.ts b/web-admin/src/routes/-/auth/device/page.spec.ts new file mode 100644 index 000000000000..fdfa041ac01b --- /dev/null +++ b/web-admin/src/routes/-/auth/device/page.spec.ts @@ -0,0 +1,238 @@ +import { describe, expect, it, vi } from "vitest"; +import { + buildDeviceAuthorizationRequestUrl, + createDeviceAuthorizationAction, + type DeviceAuthorizationDependencies, + type DeviceAuthorizationInput, + type DeviceAuthorizationState, +} from "./device-authorization-action"; + +const messages = { + confirmed: "confirmed", + rejected: "rejected", + confirmationFailed: "confirmation failed", + rejectionFailed: "rejection failed", + invalidCode: "invalid code", + unsafeRedirect: "unsafe redirect", +}; + +function input( + overrides: Partial = {}, +): DeviceAuthorizationInput { + return { + userCode: "ABCD-1234", + confirmed: true, + redirect: null, + currentUrl: "https://app.example/-/auth/device", + ...overrides, + }; +} + +function response(ok: boolean, body = "") { + return { + ok, + text: vi.fn().mockResolvedValue(body), + }; +} + +function setup(overrides: Partial = {}) { + const states: DeviceAuthorizationState[] = []; + const dependencies: DeviceAuthorizationDependencies = { + adminUrl: "https://admin.example/api", + fetch: vi.fn().mockResolvedValue(response(true)), + navigate: vi.fn(), + messages, + onState: (state) => states.push(state), + ...overrides, + }; + return { + dependencies, + states, + authorize: createDeviceAuthorizationAction(dependencies), + }; +} + +describe("device authorization action", () => { + it("confirms a code with one credentialed POST", async () => { + // Confirmation sends one encoded request and records completed state. + const { authorize, dependencies, states } = setup(); + + await authorize(input()); + + expect(dependencies.fetch).toHaveBeenCalledOnce(); + const [requestUrl, init] = vi.mocked(dependencies.fetch).mock.calls[0]; + const parsed = new URL(requestUrl); + expect(parsed.pathname).toBe("/api/auth/oauth/device"); + expect(parsed.searchParams.get("user_code")).toBe("ABCD-1234"); + expect(parsed.searchParams.get("code_confirmed")).toBe("true"); + expect(init).toEqual({ method: "POST", credentials: "include" }); + expect(states.at(-1)).toEqual({ + inFlight: false, + completed: true, + successMessage: "confirmed", + errorMessage: "", + }); + }); + + it("rejects a code successfully without following a redirect", async () => { + // Rejection records success but never follows the supplied callback. + const { authorize, dependencies, states } = setup(); + + await authorize( + input({ + confirmed: false, + redirect: "https://app.example/should-not-open", + }), + ); + + const [requestUrl] = vi.mocked(dependencies.fetch).mock.calls[0]; + expect(new URL(requestUrl).searchParams.get("code_confirmed")).toBe( + "false", + ); + expect(dependencies.navigate).not.toHaveBeenCalled(); + expect(states.at(-1)?.successMessage).toBe("rejected"); + }); + + it.each([ + ["confirmation", true, "confirmation failed"], + ["rejection", false, "rejection failed"], + ])( + "displays a failed %s response and enables retry", + async (_name, confirmed, text) => { + // HTTP failures show decision-specific detail and unlock a retry. + const fetch = vi + .fn() + .mockResolvedValueOnce(response(false, "expired code")) + .mockResolvedValueOnce(response(true)); + const { authorize, dependencies, states } = setup({ fetch }); + const request = input({ confirmed }); + + await authorize(request); + + expect(states.at(-1)).toEqual({ + inFlight: false, + completed: false, + successMessage: "", + errorMessage: `${text}: expired code`, + }); + expect(dependencies.navigate).not.toHaveBeenCalled(); + + await authorize(request); + expect(fetch).toHaveBeenCalledTimes(2); + expect(states.at(-1)?.completed).toBe(true); + }, + ); + + it("displays a network error without completing the action", async () => { + // Transport failures leave the code incomplete and surface their detail. + const { authorize, states } = setup({ + fetch: vi.fn().mockRejectedValue(new Error("offline")), + }); + + await authorize(input()); + + expect(states.at(-1)).toMatchObject({ + completed: false, + errorMessage: "confirmation failed: offline", + }); + }); + + it("rejects a malformed code before issuing a request", async () => { + // Invalid user codes are rejected locally before URL construction. + const { authorize, dependencies, states } = setup(); + + await authorize(input({ userCode: "ABC&code_confirmed=false" })); + + expect(dependencies.fetch).not.toHaveBeenCalled(); + expect(states.at(-1)).toMatchObject({ + inFlight: false, + completed: false, + errorMessage: "invalid code", + }); + }); + + it("encodes parameters without allowing user input to replace them", () => { + // URLSearchParams must preserve attacker-like text as one code value. + const requestUrl = buildDeviceAuthorizationRequestUrl( + "https://admin.example/api?old=value", + "ABC&code_confirmed=false", + true, + ); + const parsed = new URL(requestUrl); + + expect(parsed.searchParams.get("user_code")).toBe( + "ABC&code_confirmed=false", + ); + expect(parsed.searchParams.getAll("code_confirmed")).toEqual(["true"]); + expect(parsed.searchParams.has("old")).toBe(false); + }); + + it("keeps both buttons disabled while the request is in flight", async () => { + // Concurrent decisions share the active request until it settles. + let resolveRequest!: (value: ReturnType) => void; + const fetch = vi.fn( + () => + new Promise>((resolve) => { + resolveRequest = resolve; + }), + ); + const { authorize, states } = setup({ fetch }); + const request = input(); + + const first = authorize(request); + const second = authorize(request); + + expect(states.at(-1)).toMatchObject({ + inFlight: true, + completed: false, + }); + expect(second).toBe(first); + expect(fetch).toHaveBeenCalledOnce(); + + resolveRequest(response(true)); + await Promise.all([first, second]); + expect(states.at(-1)).toMatchObject({ + inFlight: false, + completed: true, + }); + }); + + it.each([ + "https://app.example/complete", + "http://localhost:43123/callback", + "http://127.0.0.1:43123/callback?code=a%20b", + "http://[::1]:43123/callback", + ])( + "allows an approved same-origin or loopback redirect: %s", + async (redirect) => { + // Browser-local callbacks are allowed only by the explicit policy. + const { authorize, dependencies } = setup(); + + await authorize(input({ redirect })); + + expect(dependencies.navigate).toHaveBeenCalledWith( + new URL(redirect).toString(), + ); + }, + ); + + it.each([ + "https://attacker.example/callback", + "http://localhost.attacker.example/callback", + "javascript:alert(1)", + ])("rejects an external redirect after confirming: %s", async (redirect) => { + // Unsafe callbacks do not undo confirmation, but they never navigate. + const { authorize, dependencies, states } = setup(); + + await authorize(input({ redirect })); + + expect(dependencies.fetch).toHaveBeenCalledOnce(); + expect(dependencies.navigate).not.toHaveBeenCalled(); + expect(states.at(-1)).toEqual({ + inFlight: false, + completed: true, + successMessage: "confirmed", + errorMessage: "unsafe redirect", + }); + }); +}); diff --git a/web-admin/src/routes/[organization]/-/upgrade-callback/+page.svelte b/web-admin/src/routes/[organization]/-/upgrade-callback/+page.svelte index 3b5e8794a7d7..e38dfdd81cbe 100644 --- a/web-admin/src/routes/[organization]/-/upgrade-callback/+page.svelte +++ b/web-admin/src/routes/[organization]/-/upgrade-callback/+page.svelte @@ -4,6 +4,7 @@ import { createAdminServiceRenewBillingSubscription, createAdminServiceUpdateBillingSubscription, + type V1BillingIssue, } from "@rilldata/web-admin/client"; import { invalidateBillingInfo } from "@rilldata/web-admin/features/billing/invalidations"; import { @@ -29,6 +30,9 @@ import { onMount } from "svelte"; import type { PageData } from "./$types"; import { m } from "@rilldata/web-common/lib/i18n/gen/messages"; + import { extractErrorMessage } from "@rilldata/web-common/lib/errors"; + import { getConfiguredRedirectOrigins } from "@rilldata/web-admin/lib/safe-redirect"; + import { createUpgradeCallbackAction } from "./upgrade-callback-action"; export let data: PageData; $: ({ cancelled, paymentIssues } = data); @@ -47,71 +51,78 @@ const planUpdater = createAdminServiceUpdateBillingSubscription(); const planRenewer = createAdminServiceRenewBillingSubscription(); + let inFlight = false; + let errorMsg = ""; - async function upgrade() { - // if there are still payment issues then do not upgrade - if (paymentIssues.length) { + const runUpgradeCallback = createUpgradeCallbackAction({ + getPaymentPortalUrl: (input) => + fetchPaymentsPortalURL( + input.organization, + getBillingUpgradeUrl($page, input.organization), + needsPaymentSetup(input.paymentIssues), + ), + notifyPaymentIssue: (input, portalUrl) => { eventBus.emit("notification", { type: "error", message: m.billing_fix_payment_issues({ - details: getPaymentIssueErrorText(paymentIssues), + details: getPaymentIssueErrorText(input.paymentIssues), }), link: { text: m.billing_update_payment(), - href: await fetchPaymentsPortalURL( - organization, - getBillingUpgradeUrl($page, organization), - needsPaymentSetup(paymentIssues), - ), + href: portalUrl, }, options: { persisted: true, }, }); - return goto(`/${organization}/-/settings/billing`); - } - const paidPlan = await maybeFetchPublicPlanByName(planName); - if (!paidPlan) return goto(`/${organization}/-/settings/billing`); - try { - if (cancelled) { - await $planRenewer.mutateAsync({ - org: organization, - data: { planName }, - }); - eventBus.emit("notification", { - type: "success", - message: m.billing_plan_renewed({ planName: paidPlan.displayName }), - }); - } else { - await $planUpdater.mutateAsync({ - org: organization, - data: { planName }, - }); - // if redirect is set then this page won't be active. - // so this will lead to pop-in of the modal before navigating away - if (!redirect) { - triggerWelcomeToRillDialog(planName); - } - } - void invalidateBillingInfo(organization); - } catch { - // TODO - } - if (redirect) { - // redirect param could be on a different domain like the rill developer instance - // so using goto won't work - window.open(redirect, "_self"); - } else { - return goto(`/${organization}`); - } - } + }, + fetchPlan: maybeFetchPublicPlanByName, + renew: (org, selectedPlan) => + $planRenewer.mutateAsync({ + org, + data: { planName: selectedPlan }, + }), + update: (org, selectedPlan) => + $planUpdater.mutateAsync({ + org, + data: { planName: selectedPlan }, + }), + notifyRenewed: (displayName) => + eventBus.emit("notification", { + type: "success", + message: m.billing_plan_renewed({ planName: displayName }), + }), + showWelcome: triggerWelcomeToRillDialog, + invalidate: invalidateBillingInfo, + navigate: (url) => { + void goto(url); + }, + open: (url) => { + window.open(url, "_self"); + }, + formatError: extractErrorMessage, + onState: (state) => { + inFlight = state.inFlight; + errorMsg = state.errorMessage; + }, + }); - onMount(() => upgrade()); + onMount(() => { + void runUpgradeCallback({ + organization, + planName, + cancelled, + paymentIssues, + redirect, + currentUrl: $page.url.toString(), + allowedRedirectOrigins: getConfiguredRedirectOrigins(), + }); + }); - + {#if inFlight}{/if} {#if cancelled} {m.billing_renewing_plan({ plan: planDisplayName })} @@ -119,6 +130,11 @@ {m.billing_upgrading_plan({ plan: planDisplayName })} {/if} + {#if errorMsg} + + {/if} diff --git a/web-admin/src/routes/[organization]/-/upgrade-callback/page.spec.ts b/web-admin/src/routes/[organization]/-/upgrade-callback/page.spec.ts new file mode 100644 index 000000000000..0aee06ba5cd3 --- /dev/null +++ b/web-admin/src/routes/[organization]/-/upgrade-callback/page.spec.ts @@ -0,0 +1,219 @@ +import { describe, expect, it, vi } from "vitest"; +import { + createUpgradeCallbackAction, + type UpgradeCallbackDependencies, + type UpgradeCallbackInput, + type UpgradeCallbackState, +} from "./upgrade-callback-action"; + +type PaymentIssue = { reason: string }; + +function input( + overrides: Partial> = {}, +): UpgradeCallbackInput { + return { + organization: "acme", + planName: "growth", + cancelled: false, + paymentIssues: [], + redirect: null, + currentUrl: "https://app.example/acme/-/upgrade-callback", + allowedRedirectOrigins: ["https://developer.example"], + ...overrides, + }; +} + +function setup( + overrides: Partial> = {}, +) { + const states: UpgradeCallbackState[] = []; + const dependencies: UpgradeCallbackDependencies = { + getPaymentPortalUrl: vi + .fn() + .mockResolvedValue("https://payments.example/session"), + notifyPaymentIssue: vi.fn(), + fetchPlan: vi.fn().mockResolvedValue({ displayName: "Growth" }), + renew: vi.fn().mockResolvedValue(undefined), + update: vi.fn().mockResolvedValue(undefined), + notifyRenewed: vi.fn(), + showWelcome: vi.fn(), + invalidate: vi.fn(), + navigate: vi.fn(), + open: vi.fn(), + formatError: (error) => + error instanceof Error ? error.message : "Unknown error", + onState: (state) => states.push(state), + ...overrides, + }; + return { + dependencies, + states, + run: createUpgradeCallbackAction(dependencies), + }; +} + +describe("upgrade callback action", () => { + it("returns payment issues to billing without mutating a plan", async () => { + // Payment issues return to billing through a fresh portal URL. + const { dependencies, run, states } = setup(); + const request = input({ + paymentIssues: [{ reason: "payment-required" }], + }); + + await run(request); + + expect(dependencies.getPaymentPortalUrl).toHaveBeenCalledWith(request); + expect(dependencies.notifyPaymentIssue).toHaveBeenCalledWith( + request, + "https://payments.example/session", + ); + expect(dependencies.navigate).toHaveBeenCalledWith( + "/acme/-/settings/billing", + ); + expect(dependencies.update).not.toHaveBeenCalled(); + expect(dependencies.invalidate).not.toHaveBeenCalled(); + expect(states.at(-1)).toMatchObject({ completed: true, errorMessage: "" }); + }); + + it("displays a portal request failure and stays on the callback", async () => { + // Portal failures remain retryable and do not navigate away. + const { dependencies, run, states } = setup({ + getPaymentPortalUrl: vi + .fn() + .mockRejectedValue(new Error("portal unavailable")), + }); + + await run(input({ paymentIssues: [{ reason: "payment-required" }] })); + + expect(states.at(-1)).toEqual({ + inFlight: false, + completed: false, + errorMessage: "portal unavailable", + }); + expect(dependencies.notifyPaymentIssue).not.toHaveBeenCalled(); + expect(dependencies.navigate).not.toHaveBeenCalled(); + }); + + it("updates, invalidates, and navigates internally after success", async () => { + // A successful update commits cache and default navigation effects. + const { dependencies, run, states } = setup(); + + await run(input()); + + expect(dependencies.update).toHaveBeenCalledWith("acme", "growth"); + expect(dependencies.showWelcome).toHaveBeenCalledWith("growth"); + expect(dependencies.invalidate).toHaveBeenCalledWith("acme"); + expect(dependencies.navigate).toHaveBeenCalledWith("/acme"); + expect(states[0]).toMatchObject({ inFlight: true }); + expect(states.at(-1)).toMatchObject({ inFlight: false, completed: true }); + }); + + it("renews and opens an approved configured redirect", async () => { + // Cancelled subscriptions renew before following an approved redirect. + const { dependencies, run } = setup(); + + await run( + input({ + cancelled: true, + redirect: "https://developer.example/project", + }), + ); + + expect(dependencies.renew).toHaveBeenCalledWith("acme", "growth"); + expect(dependencies.update).not.toHaveBeenCalled(); + expect(dependencies.notifyRenewed).toHaveBeenCalledWith("Growth"); + expect(dependencies.open).toHaveBeenCalledWith( + "https://developer.example/project", + ); + expect(dependencies.navigate).not.toHaveBeenCalled(); + }); + + it.each([ + ["update", false], + ["renew", true], + ] as const)( + "handles a failed %s without invalidating or navigating", + async (method, cancelled) => { + // Either billing mutation may fail without committing later effects. + const failure = vi.fn().mockRejectedValue(new Error(`${method} failed`)); + const { dependencies, run, states } = setup({ [method]: failure }); + + await run( + input({ + cancelled, + redirect: "https://app.example/return", + }), + ); + + expect(states.at(-1)).toMatchObject({ + completed: false, + errorMessage: `${method} failed`, + }); + expect(dependencies.invalidate).not.toHaveBeenCalled(); + expect(dependencies.open).not.toHaveBeenCalled(); + expect(dependencies.navigate).not.toHaveBeenCalled(); + }, + ); + + it("displays an invalid-plan error before any mutation", async () => { + // Invalid plan input must fail before renew or update is attempted. + const { dependencies, run, states } = setup({ + fetchPlan: vi.fn().mockResolvedValue(null), + }); + + await run(input({ planName: "bogus" })); + + expect(states.at(-1)?.errorMessage).toBe("Plan bogus not found"); + expect(dependencies.update).not.toHaveBeenCalled(); + expect(dependencies.renew).not.toHaveBeenCalled(); + expect(dependencies.navigate).not.toHaveBeenCalled(); + }); + + it.each(["https://app.example/return", "https://developer.example/return"])( + "opens an approved redirect: %s", + async (redirect) => { + // Exact same-origin and configured-origin destinations may open. + const { dependencies, run } = setup(); + + await run(input({ redirect })); + + expect(dependencies.open).toHaveBeenCalledWith(redirect); + expect(dependencies.navigate).not.toHaveBeenCalled(); + }, + ); + + it("rejects an external lookalike and falls back to the organization", async () => { + // A lookalike hostname falls back to the organization landing page. + const { dependencies, run } = setup(); + + await run( + input({ redirect: "https://developer.example.attacker.test/return" }), + ); + + expect(dependencies.open).not.toHaveBeenCalled(); + expect(dependencies.navigate).toHaveBeenCalledWith("/acme"); + expect(dependencies.showWelcome).toHaveBeenCalledWith("growth"); + }); + + it("coalesces repeated callback execution into one mutation", async () => { + // Repeated lifecycle execution must share one billing mutation. + let resolveUpdate!: () => void; + const update = vi.fn( + () => + new Promise((resolve) => { + resolveUpdate = resolve; + }), + ); + const { dependencies, run } = setup({ update }); + const request = input(); + + const first = run(request); + const second = run(request); + await vi.waitFor(() => expect(update).toHaveBeenCalledOnce()); + + expect(second).toBe(first); + resolveUpdate(); + await Promise.all([first, second]); + expect(dependencies.invalidate).toHaveBeenCalledOnce(); + }); +}); diff --git a/web-admin/src/routes/[organization]/-/upgrade-callback/upgrade-callback-action.ts b/web-admin/src/routes/[organization]/-/upgrade-callback/upgrade-callback-action.ts new file mode 100644 index 000000000000..09757b8e8535 --- /dev/null +++ b/web-admin/src/routes/[organization]/-/upgrade-callback/upgrade-callback-action.ts @@ -0,0 +1,125 @@ +import { getSafeRedirectTarget } from "@rilldata/web-admin/lib/safe-redirect.ts"; + +type UpgradePlan = { + displayName?: string | null; +}; + +export type UpgradeCallbackInput = { + organization: string; + planName: string; + cancelled: boolean; + paymentIssues: PaymentIssue[]; + redirect: string | null; + currentUrl: string; + allowedRedirectOrigins: readonly string[]; +}; + +export type UpgradeCallbackState = { + inFlight: boolean; + completed: boolean; + errorMessage: string; +}; + +export type UpgradeCallbackDependencies = { + getPaymentPortalUrl: ( + input: UpgradeCallbackInput, + ) => Promise; + notifyPaymentIssue: ( + input: UpgradeCallbackInput, + portalUrl: string, + ) => void; + fetchPlan: (planName: string) => Promise; + renew: (organization: string, planName: string) => Promise; + update: (organization: string, planName: string) => Promise; + notifyRenewed: (displayName: string) => void; + showWelcome: (planName: string) => void; + invalidate: (organization: string) => unknown; + navigate: (url: string) => void; + open: (url: string) => void; + formatError: (error: unknown) => string; + onState: (state: UpgradeCallbackState) => void; +}; + +async function performUpgradeCallback( + input: UpgradeCallbackInput, + dependencies: UpgradeCallbackDependencies, +) { + if (input.paymentIssues.length) { + const portalUrl = await dependencies.getPaymentPortalUrl(input); + if (!portalUrl) throw new Error("Payment portal URL is unavailable"); + dependencies.notifyPaymentIssue(input, portalUrl); + dependencies.navigate(`/${input.organization}/-/settings/billing`); + return; + } + + const plan = await dependencies.fetchPlan(input.planName); + if (!plan) throw new Error(`Plan ${input.planName} not found`); + + const safeRedirect = getSafeRedirectTarget(input.redirect, input.currentUrl, { + allowedOrigins: input.allowedRedirectOrigins, + }); + + if (input.cancelled) { + await dependencies.renew(input.organization, input.planName); + dependencies.notifyRenewed(plan.displayName ?? ""); + } else { + await dependencies.update(input.organization, input.planName); + if (!safeRedirect) dependencies.showWelcome(input.planName); + } + + // Invalidation and navigation are post-mutation effects. They must never run + // when the billing request rejects. + void dependencies.invalidate(input.organization); + if (safeRedirect) { + dependencies.open(safeRedirect); + } else { + dependencies.navigate(`/${input.organization}`); + } +} + +export function createUpgradeCallbackAction( + dependencies: UpgradeCallbackDependencies, +) { + let inFlight: Promise | null = null; + let completed = false; + + return function runUpgradeCallback( + input: UpgradeCallbackInput, + ): Promise { + // onMount or repeated UI events may race; only the active request owns the + // mutation and its redirect. + if (inFlight !== null) return inFlight; + if (completed) return Promise.resolve(); + + dependencies.onState({ + inFlight: true, + completed: false, + errorMessage: "", + }); + + const request = performUpgradeCallback(input, dependencies) + .then(() => { + completed = true; + dependencies.onState({ + inFlight: false, + completed: true, + errorMessage: "", + }); + }) + .catch((error) => { + dependencies.onState({ + inFlight: false, + completed: false, + errorMessage: dependencies.formatError(error), + }); + }) + .finally(() => { + // Failures clear the guard so an explicit retry can issue a fresh + // request; successful callbacks remain completed and cannot duplicate. + if (inFlight === request) inFlight = null; + }); + + inFlight = request; + return request; + }; +}