From 2787a56c8a62ce453335edab3aced6311d765024 Mon Sep 17 00:00:00 2001 From: Romeu Gouvea Date: Wed, 18 Oct 2023 17:45:59 -0700 Subject: [PATCH 1/4] Use `zone_name` for deployment of custom hostnames --- .changeset/rich-schools-fry.md | 11 ++ .../wrangler/src/__tests__/deploy.test.ts | 114 ++++++++++++++++++ packages/wrangler/src/dev.tsx | 10 +- packages/wrangler/src/zones.ts | 47 +++++++- 4 files changed, 176 insertions(+), 6 deletions(-) create mode 100644 .changeset/rich-schools-fry.md diff --git a/.changeset/rich-schools-fry.md b/.changeset/rich-schools-fry.md new file mode 100644 index 000000000000..4d9c81a4243c --- /dev/null +++ b/.changeset/rich-schools-fry.md @@ -0,0 +1,11 @@ +--- +"wrangler": minor +--- + +fix: use `zone_name` to determine a zone when the pattern is a custom hostname + +In Cloudflare for SaaS, custom hostnames of third party domain owners can be used in Cloudflare. +Workers are allowed to intercept these requests based on the routes configuration. +Before this change, the same logic used by `wrangler dev` was used in `wrangler deploy`, which caused wrangler to fail with: + +✘ [ERROR] Could not find zone for [partner-saas-domain.com] diff --git a/packages/wrangler/src/__tests__/deploy.test.ts b/packages/wrangler/src/__tests__/deploy.test.ts index e082b98e8cde..c7888b147561 100644 --- a/packages/wrangler/src/__tests__/deploy.test.ts +++ b/packages/wrangler/src/__tests__/deploy.test.ts @@ -713,6 +713,84 @@ describe("deploy", () => { `); }); + it("should deploy to a route with a SaaS domain", async () => { + writeWranglerToml({ + workers_dev: false, + routes: [ + { + pattern: "partner.com/*", + zone_name: "owned-zone.com", + }, + ], + }); + writeWorkerSource(); + mockUpdateWorkerRequest({ enabled: false }); + mockUploadWorkerRequest({ available_on_subdomain: false }); + mockGetZones("owned-zone.com", [{ id: "owned-zone-id-1" }]); + mockGetWorkerRoutes("owned-zone-id-1"); + mockPublishRoutesRequest({ + routes: [ + { + pattern: "partner.com/*", + zone_name: "owned-zone.com", + }, + ], + }); + await runWrangler("deploy ./index"); + expect(std).toMatchInlineSnapshot(` + Object { + "debug": "", + "err": "", + "info": "", + "out": "Total Upload: xx KiB / gzip: xx KiB + Uploaded test-name (TIMINGS) + Published test-name (TIMINGS) + partner.com/* (zone name: owned-zone.com) + Current Deployment ID: Galaxy-Class", + "warn": "", + } + `); + }); + + it("should deploy to a route with a SaaS subdomain", async () => { + writeWranglerToml({ + workers_dev: false, + routes: [ + { + pattern: "subdomain.partner.com/*", + zone_name: "owned-zone.com", + }, + ], + }); + writeWorkerSource(); + mockUpdateWorkerRequest({ enabled: false }); + mockUploadWorkerRequest({ available_on_subdomain: false }); + mockGetZones("owned-zone.com", [{ id: "owned-zone-id-1" }]); + mockGetWorkerRoutes("owned-zone-id-1"); + mockPublishRoutesRequest({ + routes: [ + { + pattern: "subdomain.partner.com/*", + zone_name: "owned-zone.com", + }, + ], + }); + await runWrangler("deploy ./index"); + expect(std).toMatchInlineSnapshot(` + Object { + "debug": "", + "err": "", + "info": "", + "out": "Total Upload: xx KiB / gzip: xx KiB + Uploaded test-name (TIMINGS) + Published test-name (TIMINGS) + subdomain.partner.com/* (zone name: owned-zone.com) + Current Deployment ID: Galaxy-Class", + "warn": "", + } + `); + }); + it("should deploy to a route with a pattern/{zone_id|zone_name} combo (service environments)", async () => { writeWranglerToml({ env: { @@ -8883,6 +8961,42 @@ function mockUnauthorizedPublishRoutesRequest({ ); } +function mockGetZones(domain: string, zones: { id: string }[] = []) { + msw.use( + rest.get("*/zones", (req, res, ctx) => { + expect([...req.url.searchParams.entries()]).toEqual([["name", domain]]); + + return res( + ctx.status(200), + ctx.json({ + success: true, + errors: [], + messages: [], + result: zones, + }) + ); + }) + ); +} + +function mockGetWorkerRoutes(zoneId: string) { + msw.use( + rest.get("*/zones/:zoneId/workers/routes", (req, res, ctx) => { + expect(req.params.zoneId).toEqual(zoneId); + + return res( + ctx.status(200), + ctx.json({ + success: true, + errors: [], + messages: [], + result: [], + }) + ); + }) + ); +} + function mockPublishRoutesFallbackRequest(route: { pattern: string; script: string; diff --git a/packages/wrangler/src/dev.tsx b/packages/wrangler/src/dev.tsx index ffc930179f3f..b8c4a05f0045 100644 --- a/packages/wrangler/src/dev.tsx +++ b/packages/wrangler/src/dev.tsx @@ -16,7 +16,11 @@ import * as metrics from "./metrics"; import { getAssetPaths, getSiteAssetPaths } from "./sites"; import { getAccountFromCache, loginOrRefreshIfRequired } from "./user"; import { collectKeyValues } from "./utils/collectKeyValues"; -import { getHostFromRoute, getZoneForRoute, getZoneIdFromHost } from "./zones"; +import { + getHostFromRoutePattern, + getZoneForRoute, + getZoneIdFromHost, +} from "./zones"; import { DEFAULT_INSPECTOR_PORT, DEFAULT_LOCAL_PORT, @@ -664,7 +668,7 @@ async function getZoneIdHostAndRoutes(args: StartDevOptions, config: Config) { } if (!zoneId && routes) { const firstRoute = routes[0]; - const zone = await getZoneForRoute(firstRoute); + const zone = await getZoneForRoute(firstRoute, true); if (zone) { zoneId = zone.id; host = zone.host; @@ -673,7 +677,7 @@ async function getZoneIdHostAndRoutes(args: StartDevOptions, config: Config) { } else if (!host) { if (routes) { const firstRoute = routes[0]; - host = getHostFromRoute(firstRoute); + host = getHostFromRoutePattern(firstRoute); // TODO(consider): do we need really need to do this? I've added the condition to throw to match the previous implicit behaviour of `new URL()` throwing upon invalid URLs, but could we just continue here without an inferred host? if (host === undefined) { diff --git a/packages/wrangler/src/zones.ts b/packages/wrangler/src/zones.ts index b2946f3ff5c4..b39dd155bd7d 100644 --- a/packages/wrangler/src/zones.ts +++ b/packages/wrangler/src/zones.ts @@ -28,6 +28,42 @@ export interface Zone { export function getHostFromRoute(route: Route): string | undefined { let host: string | undefined; + if (typeof route === "string") { + host = getHostFromUrl(route); + } else if (typeof route === "object") { + host = getHostFromUrl(route.pattern); + + if ("zone_name" in route) { + const hostFromZoneName = getHostFromUrl(route.zone_name); + if (hostFromZoneName && !host?.endsWith?.(`.${hostFromZoneName}`)) { + // Pattern is not a subdomain of zone. Use zone name as host instead. + host = hostFromZoneName; + } + } + } + + return host; +} + +/** + * Get the hostname on which to run a Worker. + * + * The most accurate place is usually + * `route.pattern`, as that includes any subdomains. For example: + * ```js + * { + * pattern: foo.example.com + * zone_name: example.com + * } + * ``` + * However, in the case of patterns that _can't_ be parsed as a hostname + * (primarily the pattern `*/ /*`), we fall back to the `zone_name` + * (and in the absence of that return undefined). + * @param route + */ +export function getHostFromRoutePattern(route: Route): string | undefined { + let host: string | undefined; + if (typeof route === "string") { host = getHostFromUrl(route); } else if (typeof route === "object") { @@ -42,14 +78,19 @@ export function getHostFromRoute(route: Route): string | undefined { } /** - * Try to compute the a zone ID and host name for one or more routes. + * Try to compute the a zone ID and host name for a route. * * When we're given a route, we do 2 things: * - We try to extract a host from it * - We try to get a zone id from the host */ -export async function getZoneForRoute(route: Route): Promise { - const host = getHostFromRoute(route); +export async function getZoneForRoute( + route: Route, + usePattern = false +): Promise { + const host = usePattern + ? getHostFromRoutePattern(route) + : getHostFromRoute(route); const id = typeof route === "object" && "zone_id" in route ? route.zone_id From 3378f991817b91fb4c51c5f385c765f73e442747 Mon Sep 17 00:00:00 2001 From: Romeu Gouvea Date: Tue, 24 Oct 2023 09:25:47 -0700 Subject: [PATCH 2/4] Simplify getting zone id from `zone_name` based on PR comments --- packages/wrangler/src/__tests__/dev.test.tsx | 16 ----- packages/wrangler/src/dev.tsx | 10 +--- packages/wrangler/src/zones.ts | 61 ++++---------------- 3 files changed, 15 insertions(+), 72 deletions(-) diff --git a/packages/wrangler/src/__tests__/dev.test.tsx b/packages/wrangler/src/__tests__/dev.test.tsx index 587c1c892e95..d8afcadd38d8 100644 --- a/packages/wrangler/src/__tests__/dev.test.tsx +++ b/packages/wrangler/src/__tests__/dev.test.tsx @@ -369,22 +369,6 @@ describe("wrangler dev", () => { expect(std.err).toMatchInlineSnapshot(`""`); }); - it("should fail for non-existing zones", async () => { - writeWranglerToml({ - main: "index.js", - routes: [ - { - pattern: "https://subdomain.does-not-exist.com/*", - zone_name: "exists.com", - }, - ], - }); - await fs.promises.writeFile("index.js", `export default {};`); - await expect(runWrangler("dev --remote")).rejects.toEqual( - new Error("Could not find zone for subdomain.does-not-exist.com") - ); - }); - it("should fail for non-existing zones, when falling back from */*", async () => { writeWranglerToml({ main: "index.js", diff --git a/packages/wrangler/src/dev.tsx b/packages/wrangler/src/dev.tsx index b8c4a05f0045..ffc930179f3f 100644 --- a/packages/wrangler/src/dev.tsx +++ b/packages/wrangler/src/dev.tsx @@ -16,11 +16,7 @@ import * as metrics from "./metrics"; import { getAssetPaths, getSiteAssetPaths } from "./sites"; import { getAccountFromCache, loginOrRefreshIfRequired } from "./user"; import { collectKeyValues } from "./utils/collectKeyValues"; -import { - getHostFromRoutePattern, - getZoneForRoute, - getZoneIdFromHost, -} from "./zones"; +import { getHostFromRoute, getZoneForRoute, getZoneIdFromHost } from "./zones"; import { DEFAULT_INSPECTOR_PORT, DEFAULT_LOCAL_PORT, @@ -668,7 +664,7 @@ async function getZoneIdHostAndRoutes(args: StartDevOptions, config: Config) { } if (!zoneId && routes) { const firstRoute = routes[0]; - const zone = await getZoneForRoute(firstRoute, true); + const zone = await getZoneForRoute(firstRoute); if (zone) { zoneId = zone.id; host = zone.host; @@ -677,7 +673,7 @@ async function getZoneIdHostAndRoutes(args: StartDevOptions, config: Config) { } else if (!host) { if (routes) { const firstRoute = routes[0]; - host = getHostFromRoutePattern(firstRoute); + host = getHostFromRoute(firstRoute); // TODO(consider): do we need really need to do this? I've added the condition to throw to match the previous implicit behaviour of `new URL()` throwing upon invalid URLs, but could we just continue here without an inferred host? if (host === undefined) { diff --git a/packages/wrangler/src/zones.ts b/packages/wrangler/src/zones.ts index b39dd155bd7d..94e56b2649e7 100644 --- a/packages/wrangler/src/zones.ts +++ b/packages/wrangler/src/zones.ts @@ -28,42 +28,6 @@ export interface Zone { export function getHostFromRoute(route: Route): string | undefined { let host: string | undefined; - if (typeof route === "string") { - host = getHostFromUrl(route); - } else if (typeof route === "object") { - host = getHostFromUrl(route.pattern); - - if ("zone_name" in route) { - const hostFromZoneName = getHostFromUrl(route.zone_name); - if (hostFromZoneName && !host?.endsWith?.(`.${hostFromZoneName}`)) { - // Pattern is not a subdomain of zone. Use zone name as host instead. - host = hostFromZoneName; - } - } - } - - return host; -} - -/** - * Get the hostname on which to run a Worker. - * - * The most accurate place is usually - * `route.pattern`, as that includes any subdomains. For example: - * ```js - * { - * pattern: foo.example.com - * zone_name: example.com - * } - * ``` - * However, in the case of patterns that _can't_ be parsed as a hostname - * (primarily the pattern `*/ /*`), we fall back to the `zone_name` - * (and in the absence of that return undefined). - * @param route - */ -export function getHostFromRoutePattern(route: Route): string | undefined { - let host: string | undefined; - if (typeof route === "string") { host = getHostFromUrl(route); } else if (typeof route === "object") { @@ -84,19 +48,18 @@ export function getHostFromRoutePattern(route: Route): string | undefined { * - We try to extract a host from it * - We try to get a zone id from the host */ -export async function getZoneForRoute( - route: Route, - usePattern = false -): Promise { - const host = usePattern - ? getHostFromRoutePattern(route) - : getHostFromRoute(route); - const id = - typeof route === "object" && "zone_id" in route - ? route.zone_id - : host - ? await getZoneIdFromHost(host) - : undefined; +export async function getZoneForRoute(route: Route): Promise { + const host = getHostFromRoute(route); + let id: string | undefined; + + if (typeof route === "object" && "zone_id" in route) { + id = route.zone_id; + } else if (typeof route === "object" && "zone_name" in route) { + id = await getZoneIdFromHost(route.zone_name); + } else if (host) { + id = await getZoneIdFromHost(host); + } + return id && host ? { id, host } : undefined; } From 0b0ca9760cdb2531267eeaa65a89669bdd1bd719 Mon Sep 17 00:00:00 2001 From: Samuel Macleod Date: Wed, 25 Oct 2023 17:07:21 +0100 Subject: [PATCH 3/4] Add some more tests --- packages/wrangler/src/__tests__/zones.test.ts | 101 +++++++++++++++++- 1 file changed, 100 insertions(+), 1 deletion(-) diff --git a/packages/wrangler/src/__tests__/zones.test.ts b/packages/wrangler/src/__tests__/zones.test.ts index 4749111edf9d..8c1d6e0e7d36 100644 --- a/packages/wrangler/src/__tests__/zones.test.ts +++ b/packages/wrangler/src/__tests__/zones.test.ts @@ -1,4 +1,23 @@ -import { getHostFromUrl } from "../zones"; +import { rest } from "msw"; +import { getHostFromUrl, getZoneForRoute } from "../zones"; +import { msw } from "./helpers/msw"; +function mockGetZones(domain: string, zones: { id: string }[] = []) { + msw.use( + rest.get("*/zones", (req, res, ctx) => { + expect([...req.url.searchParams.entries()]).toEqual([["name", domain]]); + + return res.once( + ctx.status(200), + ctx.json({ + success: true, + errors: [], + messages: [], + result: zones, + }) + ); + }) + ); +} describe("Zones", () => { describe("getHostFromUrl", () => { @@ -35,4 +54,84 @@ describe("Zones", () => { expect(getHostFromUrl(pattern)).toBe(host); }); }); + describe("getZoneForRoute", () => { + test("string route", async () => { + mockGetZones("example.com", [{ id: "example-id" }]); + expect(await getZoneForRoute("example.com/*")).toEqual({ + host: "example.com", + id: "example-id", + }); + }); + + test("string route (not a zone)", async () => { + mockGetZones("wrong.com", []); + await expect( + getZoneForRoute("wrong.com/*") + ).rejects.toMatchInlineSnapshot( + `[Error: Could not find zone for wrong.com]` + ); + }); + test("zone_id route", async () => { + // example-id and other-id intentionally different to show that the API is not called + // when a zone_id is provided in the route + mockGetZones("example.com", [{ id: "example-id" }]); + expect( + await getZoneForRoute({ pattern: "example.com/*", zone_id: "other-id" }) + ).toEqual({ + host: "example.com", + id: "other-id", + }); + }); + test("zone_id route (custom hostname)", async () => { + // example-id and other-id intentionally different to show that the API is not called + // when a zone_id is provided in the route + mockGetZones("example.com", [{ id: "example-id" }]); + expect( + await getZoneForRoute({ + pattern: "some.third-party.com/*", + zone_id: "other-id", + }) + ).toEqual({ + host: "some.third-party.com", + id: "other-id", + }); + }); + + test("zone_name route (apex)", async () => { + mockGetZones("example.com", [{ id: "example-id" }]); + expect( + await getZoneForRoute({ + pattern: "example.com/*", + zone_name: "example.com", + }) + ).toEqual({ + host: "example.com", + id: "example-id", + }); + }); + test("zone_name route (subdomain)", async () => { + mockGetZones("example.com", [{ id: "example-id" }]); + expect( + await getZoneForRoute({ + pattern: "subdomain.example.com/*", + zone_name: "example.com", + }) + ).toEqual({ + host: "subdomain.example.com", + id: "example-id", + }); + }); + test("zone_name route (custom hostname)", async () => { + mockGetZones("example.com", [{ id: "example-id" }]); + expect( + await getZoneForRoute({ + pattern: "some.third-party.com/*", + zone_name: "example.com", + }) + ).toEqual({ + host: "some.third-party.com", + id: "example-id", + }); + }); + }); }); From fe6c83cda4118604056592f0cdb549eccdddd43d Mon Sep 17 00:00:00 2001 From: Samuel Macleod Date: Wed, 25 Oct 2023 18:52:35 +0100 Subject: [PATCH 4/4] Add mock auth to tests --- packages/wrangler/src/__tests__/zones.test.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/wrangler/src/__tests__/zones.test.ts b/packages/wrangler/src/__tests__/zones.test.ts index 8c1d6e0e7d36..1df6ceb650d1 100644 --- a/packages/wrangler/src/__tests__/zones.test.ts +++ b/packages/wrangler/src/__tests__/zones.test.ts @@ -1,5 +1,6 @@ import { rest } from "msw"; import { getHostFromUrl, getZoneForRoute } from "../zones"; +import { mockAccountId, mockApiToken } from "./helpers/mock-account-id"; import { msw } from "./helpers/msw"; function mockGetZones(domain: string, zones: { id: string }[] = []) { msw.use( @@ -20,6 +21,8 @@ function mockGetZones(domain: string, zones: { id: string }[] = []) { } describe("Zones", () => { + mockAccountId(); + mockApiToken(); describe("getHostFromUrl", () => { test.each` pattern | host