Merge pull request 'fix: hand npm the release artifact's absolute path, so publishing finds it' (#83) from fix/publish-absolute-artifact-path into main

Reviewed-on: https://git.riverofglorymn.org/River_of_Glory_Church/Ekklesia/pulls/83
Reviewed-by: The_River <[email protected]>

docs/development/releases.md

@@ -243,9 +243,10 @@ never builds, packs, or edits a package, and it never deletes, unpublishes, or o
    name, size, SHA-256, integrity, `SHA256SUMS` line, and the `package.json` inside it must match.
 2. **Publishes in dependency order.** Each package comes after the train packages it depends on,
    then alphabetically. For each package:
-   - **absent:** `npm publish <file.tgz>` under the staging dist-tag `ekklesia-staging`. npm sends the
+   - **absent:** `npm publish <file.tgz>` under the staging dist-tag `ekklesia-staging`, given the
-     file's own bytes and runs no script for a file; the registry checks them against their
+     artifact's absolute path (npm runs in a temporary directory of its own), after the file is
-     integrity and stores them as they are;
+     checked once more against the manifest. npm sends the file's own bytes and runs no script for a
+     file; the registry checks them against their integrity and stores them as they are;
    - **present with the same bytes:** accepted as already published, and verified;
    - **present with other bytes:** a hard stop. Nothing is deleted or overwritten, nothing after it
      is published, and the version is burned ([Immutability](#immutability)).

scripts/publish-package-train.ts

@@ -109,6 +109,7 @@ export function tarballPackageJson(tarball: Uint8Array): Record<string, unknown>
 
 export interface LocalPackage {
   readonly entry: TrainPackage;
+  /** The artifact's absolute path: the exact file validated against the release manifest. */
   readonly path: string;
   readonly data: Buffer;
   readonly packageJson: Readonly<Record<string, unknown>>;
@@ -129,9 +130,12 @@ export async function loadReleaseTrain(
   releaseDirectory: string,
   expected: { readonly version: string; readonly revision?: string },
 ): Promise<LocalTrain> {
+  // Resolved once, here: every artifact path from now on is absolute, so the file npm is handed
+  // is the file validated below, whatever directory npm itself runs in.
+  const directory = path.resolve(releaseDirectory);
   const files = new Map<string, Buffer>();
-  for (const file of await readdir(releaseDirectory)) {
+  for (const file of await readdir(directory)) {
-    files.set(file, await readFile(path.join(releaseDirectory, file)));
+    files.set(file, await readFile(path.join(directory, file)));
   }
   const manifestBytes = files.get(PACKAGE_TRAIN_MANIFEST);
   if (manifestBytes === undefined) {
@@ -156,7 +160,7 @@ export async function loadReleaseTrain(
         data,
         entry,
         packageJson: tarballPackageJson(data),
-        path: path.join(releaseDirectory, entry.file),
+        path: path.join(directory, entry.file),
       };
     }),
   };
@@ -525,9 +529,9 @@ async function npmSession(registry: string, token: string, npm: string): Promise
       const stderr = String((error as { stderr?: unknown }).stderr ?? "");
       const summary = stderr
         .split("\n")
-        .filter((line) => /^npm error (?:code|\d{3}|[A-Z])/u.test(line))
+        .filter((line) => /^npm error (?:code|path|syscall|errno|enoent|\d{3}|[A-Z])/u.test(line))
         .filter((line) => !line.includes("complete log"))
-        .slice(0, 3)
+        .slice(0, 5)
         .join(" / ")
         .replaceAll(token, "***")
         .replaceAll(home, "<npm home>");
@@ -541,13 +545,22 @@ async function npmSession(registry: string, token: string, npm: string): Promise
     addTag: (name, version, tag) =>
       run(["dist-tag", "add", `${name}@${version}`, tag], `Moving ${tag} to ${name}@${version}`),
     dispose: () => rm(home, { force: true, recursive: true }),
-    publish: (file, tag) =>
+    publish: async (file, tag) => {
-      run(
+      // npm runs in its own temporary home, so a relative path would name a file there, not the
+      // release artifact. Only the artifact's absolute path is ever passed.
+      if (!path.isAbsolute(file)) {
+        throw new PublicationError(
+          "PUBLISH_FAILED",
+          `Refusing to publish ${file}: npm is only ever given an artifact's absolute path.`,
+        );
+      }
+      await run(
         // A file spec runs no lifecycle script and sends the file's own bytes; --ignore-scripts
         // says so explicitly. --access public is what a scoped package needs on a public registry.
         ["publish", file, "--tag", tag, "--access", "public", "--ignore-scripts"],
-        `Publishing ${path.basename(file)}`,
+        `Publishing ${path.basename(file)} from ${file}`,
-      ),
+      );
+    },
   };
 }
 
@@ -677,6 +690,15 @@ export async function publishPackageTrain(options: PublishOptions): Promise<Publ
       let existing = versionOf(await reader.packument(name), version);
       if (existing === undefined) {
         await options.hooks?.beforePublish?.(name, index);
+        // The file npm reads must still be the validated artifact, byte for byte.
+        const current = await readFile(local.path).catch(() => undefined);
+        if (current === undefined || !current.equals(local.data)) {
+          throw new PublicationError(
+            "INVALID_RELEASE",
+            `${local.path} is missing or no longer the artifact validated against the release manifest.`,
+          );
+        }
+        log(`${name}@${version}: publishing ${local.path} (sha256 ${local.entry.sha256})`);
         try {
           await npm.publish(local.path, stagingTag);
           outcome = "published";

scripts/test-package-publication.sh

@@ -110,7 +110,7 @@ for user in publisher outsider; do
   password_variable="${user}_password"
   api POST /admin/users "{\"username\":\"$user\",\"password\":\"${!password_variable}\",\"email\":\"$user@example.invalid\",\"must_change_password\":false}" >/dev/null
 done
-owners=(River_of_Glory_Church ekklesia-burned-version ekklesia-tag-retry)
+owners=(River_of_Glory_Church ekklesia-burned-version ekklesia-tag-retry ekklesia-artifact-layout)
 for owner in "${owners[@]}"; do
   api POST /orgs "{\"username\":\"$owner\",\"visibility\":\"public\"}" >/dev/null
   team="$(api POST "/orgs/$owner/teams" '{"name":"package-publishers","includes_all_repositories":false,"can_create_org_repo":false,"units":["repo.packages"],"units_map":{"repo.packages":"write"}}' |

tests/package-publication.integration.ts

@@ -1,5 +1,5 @@
 import { execFile } from "node:child_process";
-import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises";
+import { cp, mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises";
 import { tmpdir } from "node:os";
 import path from "node:path";
 import { promisify } from "node:util";
@@ -28,8 +28,8 @@ const origin = setting("EKKLESIA_TEST_REGISTRY_ORIGIN");
 const releaseDirectory = setting("EKKLESIA_TEST_RELEASE");
 const publishToken = setting("EKKLESIA_TEST_PUBLISH_TOKEN");
 const outsiderToken = setting("EKKLESIA_TEST_OUTSIDER_TOKEN");
-const [mainOwner, burnedOwner, tagOwner] = setting("EKKLESIA_TEST_OWNERS").split(" ");
+const [mainOwner, burnedOwner, tagOwner, layoutOwner] = setting("EKKLESIA_TEST_OWNERS").split(" ");
-if (!origin || !releaseDirectory || !publishToken || !outsiderToken || !tagOwner) {
+if (!origin || !releaseDirectory || !publishToken || !outsiderToken || !layoutOwner) {
   throw new Error("Run this suite with scripts/test-package-publication.sh.");
 }
 const registryOf = (owner: string | undefined) =>
@@ -303,3 +303,57 @@ describe("moving the final tag, interrupted and resumed", () => {
     }
   });
 });
+
+describe("the publish workflow's own invocation", () => {
+  it("publishes from the Actions artifact layout, given a relative release path from another directory", async () => {
+    // Exactly as .github/workflows/publish-packages.yml runs it: the artifact unpacked under
+    // `package-train/` in the job's workspace, and `--release "$(find package-train …)"`.
+    const workspace = await mkdtemp(path.join(tmpdir(), "ekklesia-publish-workspace-"));
+    scratch.push(workspace);
+    const releaseName = path.basename(releaseDirectory);
+    await mkdir(path.join(workspace, "package-train"), { recursive: true });
+    await cp(releaseDirectory, path.join(workspace, "package-train", releaseName), {
+      recursive: true,
+    });
+    const record = path.join(workspace, "publication-record.json");
+    const { stdout } = await execFileAsync(
+      process.execPath,
+      [
+        path.resolve("scripts/publish-package-train.ts"),
+        "--release",
+        `package-train/${releaseName}`,
+        "--registry",
+        registryOf(layoutOwner),
+        "--version",
+        train.manifest.version,
+        "--revision",
+        train.manifest.source.revision,
+        "--manifest-sha256",
+        train.manifestSha256,
+        "--record",
+        record,
+        "--require-public-read",
+        "--allow-http",
+      ],
+      {
+        cwd: workspace,
+        env: { [PUBLISH_TOKEN_VARIABLE]: publishToken, PATH: process.env.PATH ?? "" },
+      },
+    );
+    expect(stdout).toContain(
+      `publishing ${path.join(workspace, "package-train", releaseName, local("@ekklesia/contracts").entry.file)}`,
+    );
+    expect(stdout).not.toContain(publishToken);
+    const published = JSON.parse(await readFile(record, "utf8")) as PublicationRecord;
+    expect(published.packages.map((entry) => entry.outcome)).toEqual(Array(9).fill("published"));
+    for (const name of order) {
+      const version = (await anonymousPackument(layoutOwner, name))?.versions[
+        train.manifest.version
+      ];
+      expect(
+        (await anonymousBytes(version?.dist.tarball ?? "")).equals(local(name).data),
+        name,
+      ).toBe(true);
+    }
+  });
+});

tests/package-publication.test.ts

@@ -1,4 +1,4 @@
-import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises";
+import { chmod, cp, mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises";
 import { tmpdir } from "node:os";
 import path from "node:path";
 import { fileURLToPath } from "node:url";
@@ -462,6 +462,119 @@ describe("the registry and its credentials", () => {
   });
 });
 
+describe("the artifact npm is given", () => {
+  /**
+   * The release where the Actions artifact puts it, `<workspace>/package-train/<release>`, named
+   * relatively from the current directory, which is not the release directory: exactly how the
+   * publish workflow called the tool when npm was handed a path relative to its own temporary home.
+   */
+  async function nestedRelease(): Promise<{ relative: string; absolute: string }> {
+    const workspace = await mkdtemp(path.join(tmpdir(), "ekklesia-workspace-"));
+    scratch.push(workspace);
+    const absolute = path.join(workspace, "package-train", `ekklesia-packages-${VERSION}`);
+    await mkdir(path.dirname(absolute), { recursive: true });
+    await cp(await releaseDirectory(), absolute, { recursive: true });
+    const relative = path.relative(process.cwd(), absolute);
+    expect(path.isAbsolute(relative)).toBe(false);
+    expect(process.cwd()).not.toBe(absolute);
+    return { absolute, relative };
+  }
+
+  /** An npm that records what it was given, and where it ran, then fails as a registry would. */
+  async function recordingNpm(): Promise<{
+    npm: string;
+    calls: () => Promise<{ argv: string[]; cwd: string; exists: boolean }[]>;
+  }> {
+    const directory = await mkdtemp(path.join(tmpdir(), "ekklesia-fake-npm-"));
+    scratch.push(directory);
+    const log = path.join(directory, "calls.jsonl");
+    const npm = path.join(directory, "npm");
+    await writeFile(
+      npm,
+      `#!/usr/bin/env node
+const { appendFileSync, existsSync } = require("node:fs");
+const argv = process.argv.slice(2);
+appendFileSync(${JSON.stringify(log)}, JSON.stringify({ argv, cwd: process.cwd(), exists: existsSync(argv[1] ?? "") }) + "\\n");
+process.stderr.write("npm error code E503\\nnpm error 503 Service Unavailable\\n");
+process.exit(1);
+`,
+    );
+    await chmod(npm, 0o755);
+    return {
+      calls: async () =>
+        (await readFile(log, "utf8").catch(() => ""))
+          .split("\n")
+          .filter(Boolean)
+          .map((line) => JSON.parse(line)),
+      npm,
+    };
+  }
+
+  const notFound = (async () => new Response("{}", { status: 404 })) as typeof fetch;
+
+  it("resolves a relative, nested release to absolute artifact paths when it loads the train", async () => {
+    const { absolute, relative } = await nestedRelease();
+    const train = await loadReleaseTrain(relative, { revision: REVISION, version: VERSION });
+    for (const local of train.packages) {
+      expect(local.path).toBe(path.join(absolute, local.entry.file));
+      expect(path.isAbsolute(local.path)).toBe(true);
+      expect((await readFile(local.path)).equals(local.data)).toBe(true);
+    }
+  });
+
+  it("hands npm the exact validated tarball by its absolute path, from npm's own directory, and says which", async () => {
+    const { absolute, relative } = await nestedRelease();
+    const { calls, npm } = await recordingNpm();
+    const error = await publishPackageTrain({
+      fetch: notFound,
+      npm,
+      registry: REGISTRY,
+      releaseDirectory: relative,
+      revision: REVISION,
+      token: "publish-token-value",
+      version: VERSION,
+    }).catch((caught: unknown) => caught);
+    expect(error).toMatchObject({ code: "PUBLISH_FAILED" });
+    const first = tarballName("@ekklesia/contracts", VERSION);
+    const expected = path.join(absolute, first);
+    // The failure names the exact file npm was given, and never the token.
+    expect((error as Error).message).toContain(expected);
+    expect((error as Error).message).toContain("E503");
+    expect((error as Error).message).not.toContain("publish-token-value");
+
+    const [call] = await calls();
+    expect(call?.argv.slice(0, 2)).toEqual(["publish", expected]);
+    expect(call?.exists).toBe(true);
+    // npm ran in a directory of its own, where a relative path would have named nothing.
+    expect(call?.cwd).not.toBe(absolute);
+    expect(path.isAbsolute(call?.cwd ?? "")).toBe(true);
+    expect(JSON.stringify(call)).not.toContain("publish-token-value");
+  });
+
+  it("refuses to publish an artifact that changed after it was validated, before npm runs", async () => {
+    const { absolute, relative } = await nestedRelease();
+    const { calls, npm } = await recordingNpm();
+    const file = path.join(absolute, tarballName("@ekklesia/contracts", VERSION));
+    await expect(
+      publishPackageTrain({
+        fetch: notFound,
+        hooks: {
+          beforePublish: async (_name, index) => {
+            if (index === 0) await writeFile(file, "changed after validation");
+          },
+        },
+        npm,
+        registry: REGISTRY,
+        releaseDirectory: relative,
+        revision: REVISION,
+        token: "publish-token-value",
+        version: VERSION,
+      }),
+    ).rejects.toMatchObject({ code: "INVALID_RELEASE" });
+    expect(await calls()).toEqual([]);
+  });
+});
+
 describe("what the publisher may do to a registry", () => {
   it("never deletes, unpublishes, or overwrites: it only publishes new versions and moves a tag", async () => {
     // Code only: the tool's comments may say what it never does.