Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 57 additions & 3 deletions scripts/release/manifest-lib.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { join, dirname } from "node:path";
import { fileURLToPath } from "node:url";
import { collectAssets, collectModules, stableStringify } from "./hash-lib.ts";
import {
generateManifest, readDeployablePackages, readDeployInputs,
generateManifest, readDeployablePackages, readDeployInputs, releaseShortName,
} from "./manifest-lib.ts";

const RELEASE = dirname(fileURLToPath(import.meta.url));
Expand All @@ -25,8 +25,8 @@ const GOLDEN_PATH = join(TESTDATA, "golden-manifest.json");
const PLACEHOLDER_RE =
/^\$(ACCOUNT_ID|PUBLIC_BASE_URL|KV_[A-Z0-9_]+_ID|R2_[A-Z0-9_]+_NAME|WORKER_NAME\([a-z0-9-]+\)|SECRET\([A-Z0-9_]+\))/;

function buildTestManifest() {
const workers = readDeployablePackages(join(ROOT, "packages")).map((pkg) => {
function readTestWorkerBuilds() {
return readDeployablePackages(join(ROOT, "packages")).map((pkg) => {
const bundleDir = join(TESTDATA, "fixture-bundles", pkg.name);
assert.ok(existsSync(bundleDir),
`missing fixture bundle for new deployable package: add scripts/release/testdata/` +
Expand All @@ -40,7 +40,9 @@ function buildTestManifest() {
deployInputs: readDeployInputs(pkg.dir),
};
});
}

function buildTestManifest(workers = readTestWorkerBuilds()) {
return generateManifest({
releaseId: "r000000-fixture",
commit: "0000000000000000000000000000000000000000",
Expand Down Expand Up @@ -186,6 +188,58 @@ test("worker entries carry the deploy contract", () => {
}
});

// The deploy wizard sends a gatekeeper's manifest shortName as the install slug verbatim, and
// the slug becomes a GATEKEEPER_<SLUG> binding name, so the deploy service rejects anything
// outside this charset (packages/deploy/src/naming.ts). A shortName that fails here reaches
// customers as a redacted 500 on install with no workflow logs — fail the release build instead.
const SLUG_RE = /^[a-z][a-z0-9]*$/;
const MAX_SLUG_LEN = 20;

test("every gatekeeper shortName is a legal deploy slug", () => {
const { workers } = buildTestManifest();
for (const [name, entry] of Object.entries(workers)) {
if (entry.kind !== "gatekeeper") continue;
assert.match(entry.shortName ?? "", SLUG_RE,
`${name}: shortName ${entry.shortName} is not a legal install slug; it must be ` +
`lowercase letters and digits starting with a letter (it becomes GATEKEEPER_<SLUG>)`);
assert.ok((entry.shortName ?? "").length <= MAX_SLUG_LEN,
`${name}: shortName ${entry.shortName} exceeds ${MAX_SLUG_LEN} chars`);
assert.equal(entry.vars.BASE_URL, `$PUBLIC_BASE_URL/gatekeeper/${entry.shortName}`,
`${name}: BASE_URL path must match shortName`);
}
});

test("releaseShortName folds package names into the slug charset", () => {
assert.equal(releaseShortName("gatekeeper-mcp-portal"), "mcpportal");
// No-op for names that already conform.
assert.equal(releaseShortName("gatekeeper-google"), "google");
});

test("every gatekeeper shortName is unique", () => {
const owners = new Map<string, string>();
for (const [name, entry] of Object.entries(buildTestManifest().workers)) {
if (entry.shortName === undefined) continue;
const owner = owners.get(entry.shortName);
assert.equal(owner, undefined,
`${owner} and ${name} both emit shortName ${entry.shortName}`);
owners.set(entry.shortName, name);
}
});

// The fold is lossy — gatekeeper-foo-bar and gatekeeper-foobar both emit `foobar` — and two
// gatekeepers with the same slug would contend for one GATEKEEPER_<SLUG> binding and one
// /gatekeeper/<slug> route on every customer instance. Per-entry validity can't catch that.
test("generateManifest rejects gatekeepers whose folded shortNames collide", () => {
const builds = readTestWorkerBuilds();
const google = builds.find((w) => w.pkgName === "gatekeeper-google");
assert.ok(google, "expected gatekeeper-google among the deployable packages");
// Folds to "google" too, so it collides with gatekeeper-google.
const collider = { ...google, pkgName: "gatekeeper-goo-gle" };

assert.throws(() => buildTestManifest([...builds, collider]),
/gatekeeper-google and gatekeeper-goo-gle both emit shortName "google"/);
});

test("per-package deploy-inputs.json files are well-formed when present", () => {
const KINDS = new Set(["secret", "var", "workerName"]);
for (const pkg of readDeployablePackages(join(ROOT, "packages"))) {
Expand Down
34 changes: 31 additions & 3 deletions scripts/release/manifest-lib.ts
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,18 @@ export function gatekeeperShortName(pkgName: string): string {
return pkgName.slice(GATEKEEPER_PREFIX.length);
}

/**
* The release manifest's shortName. Deployed instances bind gatekeepers as GATEKEEPER_<SLUG> and
* the router recovers the path from that binding name, so a slug must survive `toUpperCase()` and
* back — the deploy wizard restricts it to /^[a-z][a-z0-9]*$/ and sends the manifest shortName as
* the install slug verbatim. Package names are not so restricted (gatekeeper-mcp-portal), so fold
* here. Distinct from gatekeeperShortName(), which staging/preview use with GATEKEEPER_<PKG_NAME>
* bindings (underscores, router maps _ -> -) where a hyphen does round-trip.
*/
export function releaseShortName(pkgName: string): string {
return gatekeeperShortName(pkgName).replace(/[^a-z0-9]/g, "");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Guard against normalized short-name collisions

This transformation is lossy: for example, gatekeeper-foo-bar and gatekeeper-foobar both emit foobar. The deploy service then gives both installs the same GATEKEEPER_FOOBAR binding and the router can expose only one at /gatekeeper/foobar. The new manifest test checks validity per entry but not uniqueness, so such a release would pass. Please reject duplicate emitted shortNames (or use an explicit exceptional mapping) when assembling the manifest.

}

/** Read a package's `deploy-inputs.json`, or undefined if it declares none. */
export function readDeployInputs(pkgDir: string): DeployInput[] | undefined {
const path = join(pkgDir, "deploy-inputs.json");
Expand Down Expand Up @@ -439,7 +451,7 @@ export function buildWorkerEntry(
// (default entrypoint — it forwards whole HTTP requests, not vendor RPC).
gatekeeperBindingExpansion = { propsByPackage: {} };
} else {
vars.BASE_URL = `$PUBLIC_BASE_URL/gatekeeper/${gatekeeperShortName(pkgName)}`;
vars.BASE_URL = `$PUBLIC_BASE_URL/gatekeeper/${releaseShortName(pkgName)}`;
installable = !NOT_INSTALLABLE.has(pkgName);
if (installable) {
inputs = deployInputs ??
Expand All @@ -461,7 +473,7 @@ export function buildWorkerEntry(

return {
kind,
...(kind === "gatekeeper" ? { shortName: gatekeeperShortName(pkgName) } : {}),
...(kind === "gatekeeper" ? { shortName: releaseShortName(pkgName) } : {}),
installable,
...(PREINSTALL.has(pkgName) ? { preinstall: true } : {}),
...(SINGLETON.has(pkgName) ? { singleton: true } : {}),
Expand Down Expand Up @@ -510,8 +522,24 @@ export function generateManifest({
assetVariants?: Record<string, CollectedAssets>;
}): ReleaseManifest {
const workerEntries: Record<string, WorkerEntry> = {};
// releaseShortName() folds the package name into the install-slug charset, and that fold is
// lossy: gatekeeper-foo-bar and gatekeeper-foobar both emit `foobar`. Two gatekeepers sharing a
// slug would want the same GATEKEEPER_FOOBAR binding and the same /gatekeeper/foobar route, so
// one would silently shadow the other on every customer instance. Fail the release build here —
// the per-entry slug check can't see the collision, only the assembled set can.
const shortNameOwner = new Map<string, string>();
for (const w of workers) {
workerEntries[w.pkgName] = buildWorkerEntry(w);
const entry = buildWorkerEntry(w);
if (entry.shortName !== undefined) {
const owner = shortNameOwner.get(entry.shortName);
if (owner !== undefined) {
throw new Error(`${owner} and ${w.pkgName} both emit shortName "${entry.shortName}"; ` +
`install slugs must be unique (each becomes a GATEKEEPER_<SLUG> binding and a ` +
`/gatekeeper/<slug> route). Rename one package so the slugs differ.`);
}
shortNameOwner.set(entry.shortName, w.pkgName);
}
workerEntries[w.pkgName] = entry;
}

const assets: ReleaseManifest["assets"] = {};
Expand Down
4 changes: 2 additions & 2 deletions scripts/release/testdata/golden-manifest.json
Original file line number Diff line number Diff line change
Expand Up @@ -599,9 +599,9 @@
"invocation_logs": false
}
},
"shortName": "mcp-portal",
"shortName": "mcpportal",
"vars": {
"BASE_URL": "$PUBLIC_BASE_URL/gatekeeper/mcp-portal",
"BASE_URL": "$PUBLIC_BASE_URL/gatekeeper/mcpportal",
"MCP_ALLOW_INSECURE": "false"
}
},
Expand Down
Loading