From ca7a55e53061a40ae409f283daf0a2bf533a9b5d Mon Sep 17 00:00:00 2001 From: Dotta Date: Tue, 19 May 2026 15:24:54 -0500 Subject: [PATCH] Harden cloud upstream auth URLs --- cli/src/__tests__/cloud.test.ts | 10 +++++++ cli/src/commands/client/cloud.ts | 18 ++++++++++++ .../src/migrations/0087_cloud_upstreams.sql | 5 ++++ packages/db/src/schema/cloud_upstreams.ts | 4 +-- server/src/__tests__/cloud-upstreams.test.ts | 11 +++++++- server/src/services/cloud-upstreams.ts | 28 +++++++++++++++---- 6 files changed, 67 insertions(+), 9 deletions(-) diff --git a/cli/src/__tests__/cloud.test.ts b/cli/src/__tests__/cloud.test.ts index b2707598c1b..326e46ca4a5 100644 --- a/cli/src/__tests__/cloud.test.ts +++ b/cli/src/__tests__/cloud.test.ts @@ -8,6 +8,7 @@ import { buildBundleFromLocalCompany, cloudCommandExitCodes, connectCloud, + discoverUpstream, } from "../commands/client/cloud.js"; import { LocalUpstreamPushCoordinator, @@ -88,6 +89,15 @@ describe("cloud CLI helpers", () => { expect(cloudCommandExitCodes.schemaMismatch).toBe(3); }); + it("requires HTTPS before fetching cloud discovery except for localhost development", async () => { + globalThis.fetch = vi.fn(async () => { + throw new Error("discovery fetch should not run for insecure remote URLs"); + }) as typeof fetch; + + await expect(discoverUpstream("http://cloud.example.test")).rejects.toThrow(/must use HTTPS/i); + expect(globalThis.fetch).not.toHaveBeenCalled(); + }); + it("builds deterministic chunks with validated payload hashes", async () => { const bundle = await buildTestBundle(); diff --git a/cli/src/commands/client/cloud.ts b/cli/src/commands/client/cloud.ts index 56da8493160..46539a17778 100644 --- a/cli/src/commands/client/cloud.ts +++ b/cli/src/commands/client/cloud.ts @@ -239,6 +239,7 @@ export async function pushCloud(opts: CloudPushOptions): Promise { export async function discoverUpstream(remoteUrl: string): Promise { const base = new URL(remoteUrl); + assertSecureCloudUpstreamUrl(base, "Cloud upstream remote URL"); const discoveryUrl = new URL("/.well-known/paperclip-upstream", base); return requestCloudJson(discoveryUrl.toString(), { method: "GET" }); } @@ -347,6 +348,8 @@ async function authorizeWithBrowser( ): Promise { const pkce = discovery.auth.pkce; if (!pkce) throw new Error("Remote did not advertise PKCE authorization."); + assertSecureCloudUpstreamUrl(pkce.authorizeUrl, "Cloud upstream authorize URL"); + assertSecureCloudUpstreamUrl(pkce.tokenUrl, "Cloud upstream token URL"); const callback = await startPkceCallbackServer(); const verifier = randomBytes(32).toString("base64url"); const challenge = createHash("sha256").update(verifier).digest("base64url"); @@ -388,6 +391,8 @@ async function authorizeWithDeviceCode( ): Promise { const device = discovery.auth.deviceCode; if (!device) throw new Error("Remote did not advertise device-code authorization."); + assertSecureCloudUpstreamUrl(device.deviceCodeUrl, "Cloud upstream device-code URL"); + assertSecureCloudUpstreamUrl(device.tokenUrl, "Cloud upstream token URL"); const response = await requestCloudJson<{ deviceCode: string; userCode: string; @@ -407,6 +412,7 @@ async function authorizeWithDeviceCode( console.error(pc.bold("Cloud device authorization required")); console.error(`Open: ${response.verificationUri}`); console.error(`Code: ${response.userCode}`); + assertSecureCloudUpstreamUrl(response.verificationUri, "Cloud upstream verification URL"); if (opts.openBrowser) openUrl(response.verificationUri); const expiresAt = Date.parse(response.expiresAt); @@ -435,6 +441,18 @@ async function authorizeWithDeviceCode( throw new Error("Device-code authorization expired before it was approved."); } +function assertSecureCloudUpstreamUrl(url: string | URL, label: string): void { + const parsed = typeof url === "string" ? new URL(url) : url; + if (parsed.protocol === "https:" || isLocalhostHost(parsed.hostname)) { + return; + } + throw new Error(`${label} must use HTTPS except for localhost development.`); +} + +function isLocalhostHost(hostname: string): boolean { + return hostname === "localhost" || hostname === "127.0.0.1" || hostname === "::1" || hostname === "[::1]"; +} + function buildEntitiesFromPortableExport( localCompanyId: string, sourceInstanceId: string, diff --git a/packages/db/src/migrations/0087_cloud_upstreams.sql b/packages/db/src/migrations/0087_cloud_upstreams.sql index 4eb06a21374..305d8984779 100644 --- a/packages/db/src/migrations/0087_cloud_upstreams.sql +++ b/packages/db/src/migrations/0087_cloud_upstreams.sql @@ -66,6 +66,11 @@ DO $$ BEGIN ALTER TABLE "cloud_upstream_runs" ADD CONSTRAINT "cloud_upstream_runs_company_id_companies_id_fk" FOREIGN KEY ("company_id") REFERENCES "public"."companies"("id") ON DELETE cascade ON UPDATE no action; END IF; END $$;--> statement-breakpoint +DO $$ BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_constraint WHERE conname = 'cloud_upstream_connections_last_run_id_cloud_upstream_runs_id_fk') THEN + ALTER TABLE "cloud_upstream_connections" ADD CONSTRAINT "cloud_upstream_connections_last_run_id_cloud_upstream_runs_id_fk" FOREIGN KEY ("last_run_id") REFERENCES "public"."cloud_upstream_runs"("id") ON DELETE set null ON UPDATE no action; + END IF; +END $$;--> statement-breakpoint CREATE INDEX IF NOT EXISTS "cloud_upstream_connections_company_idx" ON "cloud_upstream_connections" USING btree ("company_id");--> statement-breakpoint CREATE INDEX IF NOT EXISTS "cloud_upstream_runs_company_created_idx" ON "cloud_upstream_runs" USING btree ("company_id","created_at");--> statement-breakpoint CREATE INDEX IF NOT EXISTS "cloud_upstream_runs_connection_idx" ON "cloud_upstream_runs" USING btree ("connection_id"); diff --git a/packages/db/src/schema/cloud_upstreams.ts b/packages/db/src/schema/cloud_upstreams.ts index 636e8b52c31..d2f4ccf0b1e 100644 --- a/packages/db/src/schema/cloud_upstreams.ts +++ b/packages/db/src/schema/cloud_upstreams.ts @@ -1,4 +1,4 @@ -import { boolean, index, integer, jsonb, pgTable, text, timestamp, uuid } from "drizzle-orm/pg-core"; +import { type AnyPgColumn, boolean, index, integer, jsonb, pgTable, text, timestamp, uuid } from "drizzle-orm/pg-core"; import { companies } from "./companies.js"; export const cloudUpstreamConnections = pgTable( @@ -33,7 +33,7 @@ export const cloudUpstreamConnections = pgTable( pendingRedirectUri: text("pending_redirect_uri"), pendingTokenUrl: text("pending_token_url"), - lastRunId: uuid("last_run_id"), + lastRunId: uuid("last_run_id").references((): AnyPgColumn => cloudUpstreamRuns.id, { onDelete: "set null" }), createdAt: timestamp("created_at", { withTimezone: true }).notNull().defaultNow(), updatedAt: timestamp("updated_at", { withTimezone: true }).notNull().defaultNow(), }, diff --git a/server/src/__tests__/cloud-upstreams.test.ts b/server/src/__tests__/cloud-upstreams.test.ts index 95b347f6798..c7ca25f147f 100644 --- a/server/src/__tests__/cloud-upstreams.test.ts +++ b/server/src/__tests__/cloud-upstreams.test.ts @@ -1,7 +1,7 @@ import { describe, expect, it } from "vitest"; import { HttpError } from "../errors.js"; -import { cloudUpstreamRemoteFailureReport } from "../services/cloud-upstreams.js"; +import { cloudUpstreamRemoteFailureReport, parseSecureCloudUpstreamUrl } from "../services/cloud-upstreams.js"; describe("cloud upstream remote failures", () => { it("preserves the cloud response body and message on run reports", () => { @@ -49,4 +49,13 @@ describe("cloud upstream remote failures", () => { error: "network failed", }); }); + + it("requires HTTPS for credential-bearing cloud URLs except localhost development", () => { + expect(parseSecureCloudUpstreamUrl("https://cloud.example.test/token", "Cloud upstream token URL").origin) + .toBe("https://cloud.example.test"); + expect(parseSecureCloudUpstreamUrl("http://localhost:3100/token", "Cloud upstream token URL").origin) + .toBe("http://localhost:3100"); + expect(() => parseSecureCloudUpstreamUrl("http://cloud.example.test/token", "Cloud upstream token URL")) + .toThrow(/must use HTTPS/i); + }); }); diff --git a/server/src/services/cloud-upstreams.ts b/server/src/services/cloud-upstreams.ts index c4787684cc0..35fe196bedd 100644 --- a/server/src/services/cloud-upstreams.ts +++ b/server/src/services/cloud-upstreams.ts @@ -558,7 +558,7 @@ export function cloudUpstreamService(db: Db, options: { instanceId?: string } = } : {}), events: [ ...input.initialEvents, - event(failedAt.toISOString(), "push", "failed", failure.errorMessage ?? failure.error), + event(failedAt.toISOString(), currentStep, "failed", failure.errorMessage ?? failure.error), ], report: { runId: input.runId, @@ -695,10 +695,7 @@ export function cloudUpstreamService(db: Db, options: { instanceId?: string } = } async function fetchDiscovery(remoteUrl: string): Promise> { - const parsed = new URL(remoteUrl); - if (parsed.protocol !== "https:" && parsed.hostname !== "localhost" && parsed.hostname !== "127.0.0.1") { - throw badRequest("Cloud upstream targets require HTTPS except localhost development"); - } + const parsed = parseSecureCloudUpstreamUrl(remoteUrl, "Cloud upstream target URL"); const stackId = firstPathSegment(parsed.pathname); const discoveryUrl = new URL("/.well-known/paperclip-upstream", parsed.origin); if (stackId) { @@ -809,7 +806,26 @@ function consentUrlFromDiscovery(discovery: Record): string { } function tokenUrlFromDiscovery(discovery: Record): string { - return stringField(objectField(objectField(discovery, "auth"), "pkce"), "tokenUrl"); + const url = stringField(objectField(objectField(discovery, "auth"), "pkce"), "tokenUrl"); + parseSecureCloudUpstreamUrl(url, "Cloud upstream token URL"); + return url; +} + +export function parseSecureCloudUpstreamUrl(url: string, label: string): URL { + let parsed: URL; + try { + parsed = new URL(url); + } catch { + throw badRequest(`${label} must be a valid URL`); + } + if (parsed.protocol === "https:" || isLocalhostHost(parsed.hostname)) { + return parsed; + } + throw badRequest(`${label} must use HTTPS except localhost development`); +} + +function isLocalhostHost(hostname: string): boolean { + return hostname === "localhost" || hostname === "127.0.0.1" || hostname === "::1" || hostname === "[::1]"; } function buildWarnings(schemaMajor: number): CloudUpstreamWarning[] {