fix(plugins): read artifact manifest with the same tar parser as the unpack (#1622)

The consent check read the first manifest.json with a hand-written tar
reader, while the unpack keeps the last one and also follows pax and
long-name headers. An archive with two manifests could pass the check
with one capability list and get granted another. Read the manifest
with node-tar, count only plain files, and refuse archives that hold
more than one manifest.json.

src/backend/plugins/artifact-manifest.ts

@@ -1,50 +1,59 @@
+import path from "node:path";
 import zlib from "node:zlib";
+import * as tar from "tar";
 
 const MAX_UNPACKED_BYTES = 512 * 1024 * 1024;
 
 /**
- * Reads one file out of a .tmxplug (a gzipped tar) without unpacking it to
+ * Reads every copy of one file out of a .tmxplug (a gzipped tar) without
- * disk. Returns null when the archive has no such file.
+ * unpacking it to disk. Uses the same tar parser as the loader's unpack, so
+ * pax and long-name headers resolve the same way, and only plain files count
+ * because they are the only kind the unpack writes. Names compare
+ * case-insensitively since a case-insensitive disk folds them together.
  */
-export function readArtifactFile(
+export function readArtifactFiles(buffer: Buffer, wanted: string): Buffer[] {
-  buffer: Buffer,
+  const archive = zlib.gunzipSync(buffer, {
-  wanted: string,
+    maxOutputLength: MAX_UNPACKED_BYTES,
-): Buffer | null {
+  });
-  const tar = zlib.gunzipSync(buffer, { maxOutputLength: MAX_UNPACKED_BYTES });
+  const key = wanted.toLowerCase();
-  let offset = 0;
+  const found: Buffer[] = [];
-  while (offset + 512 <= tar.length) {
+  const parser = new tar.Parser({
-    const header = tar.subarray(offset, offset + 512);
+    strict: true,
-    if (header.every((byte) => byte === 0)) break;
+    onReadEntry: (entry) => {
-    const field = (start: number, length: number) =>
+      const name = path.posix.normalize(entry.path.replace(/\\/g, "/"));
-      header
+      if (entry.type !== "File" || name.toLowerCase() !== key) {
-        .subarray(start, start + length)
+        entry.resume();
-        .toString("utf8")
+        return;
-        .replace(/\0.*$/s, "");
+      }
-    const prefix = field(345, 155);
+      const chunks: Buffer[] = [];
-    const name = (
+      entry.on("data", (chunk: Buffer) => chunks.push(chunk));
-      prefix ? `${prefix}/${field(0, 100)}` : field(0, 100)
+      entry.on("end", () => found.push(Buffer.concat(chunks)));
-    ).replace(/^\.\//, "");
+    },
-    const size = Number.parseInt(field(124, 12).trim() || "0", 8);
+  });
-    if (!Number.isFinite(size) || size < 0) break;
+  // The parser runs synchronously on an in-memory buffer.
-    const start = offset + 512;
+  parser.end(archive);
-    if (name === wanted) return tar.subarray(start, start + size);
+  return found;
-    offset = start + Math.ceil(size / 512) * 512;
-  }
-  return null;
 }
 
-/** The manifest.json packed in a .tmxplug, parsed but not validated. */
+/**
+ * The manifest.json packed in a .tmxplug, parsed but not validated. An
+ * archive with more than one is refused: the unpack keeps the last copy, so
+ * whichever one this read the other could carry different capabilities.
+ */
 export function readArtifactManifest(buffer: Buffer): Record<string, unknown> {
-  let raw: Buffer | null;
+  let copies: Buffer[];
   try {
-    raw = readArtifactFile(buffer, "manifest.json");
+    copies = readArtifactFiles(buffer, "manifest.json");
   } catch {
     throw new Error("the file is not a valid .tmxplug archive");
   }
-  if (!raw) throw new Error("the archive has no manifest.json");
+  if (copies.length === 0) throw new Error("the archive has no manifest.json");
+  if (copies.length > 1) {
+    throw new Error("the archive has more than one manifest.json");
+  }
   let parsed: unknown;
   try {
-    parsed = JSON.parse(raw.toString("utf8"));
+    parsed = JSON.parse(copies[0].toString("utf8"));
   } catch {
     throw new Error("manifest.json is not valid JSON");
   }

src/backend/tests/plugins/artifact-manifest.test.ts

@@ -2,9 +2,10 @@ import { describe, expect, it } from "vitest";
 import fs from "node:fs";
 import os from "node:os";
 import path from "node:path";
+import zlib from "node:zlib";
 import * as tar from "tar";
 import {
-  readArtifactFile,
+  readArtifactFiles,
   readArtifactManifest,
   sameCapabilities,
 } from "../../plugins/artifact-manifest.js";
@@ -27,20 +28,42 @@ function pack(files: Record<string, string>, prefix = ""): Buffer {
   }
 }
 
-describe("readArtifactFile", () => {
+/** Two archives' entries back to back, so a name can appear twice. */
+function concat(first: Buffer, second: Buffer): Buffer {
+  const entries = (buffer: Buffer) => {
+    const raw = zlib.gunzipSync(buffer);
+    // Drop the two zero blocks that end every tar.
+    return raw.subarray(0, raw.length - 1024);
+  };
+  return zlib.gzipSync(
+    Buffer.concat([entries(first), entries(second), Buffer.alloc(1024)]),
+  );
+}
+
+describe("readArtifactFiles", () => {
   it("finds a file by name, with or without a ./ prefix", () => {
     const plain = pack({ "manifest.json": "{}", "dist/a.js": "x" });
-    expect(readArtifactFile(plain, "dist/a.js")?.toString()).toBe("x");
+    expect(
+      readArtifactFiles(plain, "dist/a.js").map((b) => b.toString()),
+    ).toEqual(["x"]);
     const dotted = pack({ "manifest.json": '{"id":"a"}' }, "./");
-    expect(readArtifactFile(dotted, "manifest.json")?.toString()).toBe(
+    expect(
-      '{"id":"a"}',
+      readArtifactFiles(dotted, "manifest.json").map((b) => b.toString()),
+    ).toEqual(['{"id":"a"}']);
+  });
+
+  it("returns nothing for a missing file", () => {
+    expect(readArtifactFiles(pack({ "a.txt": "a" }), "manifest.json")).toEqual(
+      [],
     );
   });
 
-  it("returns null for a missing file", () => {
+  it("resolves names that need a pax header", () => {
-    expect(readArtifactFile(pack({ "a.txt": "a" }), "manifest.json")).toBe(
+    const long = `dist/${"x".repeat(150)}.js`;
-      null,
+    const archive = pack({ "manifest.json": "{}", [long]: "x" });
-    );
+    expect(readArtifactFiles(archive, long).map((b) => b.toString())).toEqual([
+      "x",
+    ]);
   });
 });
 
@@ -68,6 +91,18 @@ describe("readArtifactManifest", () => {
       readArtifactManifest(pack({ "manifest.json": "[1]" })),
     ).toThrow(/not an object/);
   });
+
+  it("refuses an archive with two manifests", () => {
+    const first = pack({ "manifest.json": '{"capabilities":[]}' });
+    const second = pack({ "manifest.json": '{"capabilities":["ssh"]}' });
+    expect(() => readArtifactManifest(concat(first, second))).toThrow(
+      /more than one manifest.json/,
+    );
+    const upper = pack({ "MANIFEST.JSON": '{"capabilities":["ssh"]}' });
+    expect(() => readArtifactManifest(concat(first, upper))).toThrow(
+      /more than one manifest.json/,
+    );
+  });
 });
 
 describe("sameCapabilities", () => {