Merge pull request #843 from tdurieux/codex/review-service-capabilities

feat: expose authenticated review-service capabilities

docs/review-service-capabilities.md

@@ -0,0 +1,84 @@
+# Review service capabilities
+
+`GET /service/v1/capabilities` is an optional server-to-server endpoint for the
+`4open.artifacts/1` contract. It returns the authenticated client ID, the supported
+contract version, and an empty `available` list. Linking, snapshots, protected
+files, retention, publication, entitlements and usage are not advertised. Clients
+must decline those workflows until their required capabilities are implemented.
+
+The endpoint is mounted before JSON parsing, browser sessions and passport. The
+entire `/service` namespace returns a typed 404 when configuration is absent.
+Only the exact GET path is accepted. Query strings, browser context headers,
+cookies, bodies, duplicate authentication headers and unknown paths are rejected.
+Rejections close the connection immediately, including requests whose declared
+body has not arrived. Responses contain no credentials, owner identity, repository data, cookies or CORS
+permission; they have `no-store` and `no-referrer` headers. No database, queue or
+provider call is involved.
+
+## Private configuration
+
+Leave `REVIEW_SERVICE_KEYS` unset until activation is approved. If configured,
+its value is a JSON object with exactly `version: 1` and `keys`. Each key has
+exactly these fields:
+
+- `clientId`: 32 lowercase hexadecimal characters.
+- `keyId`: 1–64 ASCII letters, digits, underscores or hyphens.
+- `tokenSHA256`: SHA-256 of the **ASCII 64-character lowercase hexadecimal bearer
+  token**, stored as 64 lowercase hexadecimal characters. Do not hash the decoded
+  token bytes and do not store a plaintext token in this registration.
+- `notBefore` and `notAfter`: UTC timestamps, `YYYY-MM-DDTHH:mm:ssZ`, in 2000–2099.
+  The start is inclusive and the end exclusive.
+
+Use private runtime configuration outside Git. The caller sends
+`Authorization: Bearer <token>`, `X-4open-Artifact-Client-Id` and
+`X-4open-Artifact-Service-Key-Id`. Keys grant only capabilities reads. Generate a
+separate token for this operation and environment; never reuse GitHub credentials,
+webhook secrets or future exchange credentials. The Go review client requires
+HTTPS with a trusted server certificate; a deployment must provide that transport.
+
+The registry is bounded to 64 KiB, 64 keys, 32 clients and 16 keys per client.
+Duplicate client/key pairs and reused digests are rejected. Unknown fields,
+plaintext secret fields, invalid timestamps and empty configuration fail startup
+with a generic diagnostic. Configuration uses ordinary JSON object parsing.
+Credentials are copied into a private registry and compared by constant-time
+SHA-256 digest equality. Overlapping validity windows permit rotation. Remove a
+key and restart to revoke it; this is not hot reloading.
+
+Authenticated clients share 60 reads per minute across their keys. Counters are
+bounded by registered clients, reset lazily, and have no timer. Limits are local
+to each server process; they are not a distributed quota. A 429 response supplies
+a typed failure and a bounded retry delay. This endpoint does not accept browser
+session or repository-owner authority.
+
+## Verification
+
+`mocha --no-config --no-package test/review-capabilities.test.js` passes eight
+HTTP/configuration scenarios, including 60 concurrent reads, cross-client and
+wrong-key rejection, raw duplicate headers, exact path checks, browser context,
+expiry, overlapping keys and restart revocation. Add
+`RUN_REVIEW_SERVICE_BENCHMARK=1` for a 150-request synthetic loopback measurement.
+The clean-install run averaged 2.035 ms, median 1.732 ms and p95 2.417 ms with fresh
+HTTP connections. These are local tooling measurements, not production latency.
+
+A local HTTPS interoperability rehearsal uses the existing Go
+`artifactclient.NewCapabilitiesReader` at review source `ee07d06`. It verifies
+certificate trust, accepts the empty capability response, rejects both unsupported
+access policies and rejects a wrong credential. No external provider is contacted.
+
+A clean `npm ci` install, the full TypeScript check and emitted build, UI build,
+and targeted ESLint pass. The full test suite reports 700 passing in 36 seconds
+and 49 pending opt-in cases; those skipped cases are not claimed as tested.
+The capability suite separately passes eight cases including its benchmark.
+
+An initial check used dependencies copied from another checkout and produced
+TypeScript diagnostics. Those installed versions differed from the lockfile;
+reinstalling the locked dependencies resolved every diagnostic without source
+changes. Use the lockfile when reproducing these results.
+
+The endpoint is implemented and tested locally. Upstream deployment, private key
+distribution, ownership consent and the remaining artifact workflows are pending.
+
+A raw-socket regression first reproduced a rejected request waiting for its unread
+10 MB body. Closing failed connections makes the same header-only request return
+400 and close immediately; the complete eight-case endpoint suite and full
+TypeScript checking pass. Full PR CI is repeated for this correction.

src/server/index.ts

@@ -41,6 +41,8 @@ import { monitorRequests } from "../core/request-monitoring";
 import { startPerformanceMonitoring } from "../core/performance-monitoring";
 import { getAnonymizationPoolStats } from "../core/anonymization-pool";
 
+import { createReviewCapabilities } from "./service/review-capabilities";
+
 const logger = createLogger("server");
 
 // Lazily build the templated index.html on first request so the server
@@ -106,6 +108,7 @@ export default async function start() {
   app.use(monitorRequests("api"));
   startPerformanceMonitoring("api", getAnonymizationPoolStats);
   app.set("query parser", "extended");
+  app.use("/service", createReviewCapabilities(process.env.REVIEW_SERVICE_KEYS));
   app.use("/github/app/webhook", githubAppWebhook);
   app.use(express.json());
   // Preserve the empty body used by API validation when no JSON was parsed.

src/server/service/review-capabilities.ts

@@ -0,0 +1,200 @@
+import { createHash, randomBytes, timingSafeEqual } from "crypto";
+import { RequestHandler } from "express";
+
+const contract = "4open.artifacts/1";
+const clientPattern = /^[a-f0-9]{32}$/;
+const keyPattern = /^[A-Za-z0-9_-]{1,64}$/;
+const secretPattern = /^[a-f0-9]{64}$/;
+type Key = { clientId: string; digest: Buffer; from: number; until: number };
+
+function invalid(): never {
+  // Never include private configuration or request values in diagnostic errors.
+  throw new Error("Invalid REVIEW_SERVICE_KEYS configuration");
+}
+function record(value: unknown, fields: string[]): Record<string, unknown> {
+  if (!value || typeof value !== "object" || Array.isArray(value)) invalid();
+  const data = value as Record<string, unknown>;
+  if (
+    Object.keys(data).length !== fields.length ||
+    fields.some((k) => !Object.prototype.hasOwnProperty.call(data, k))
+  )
+    invalid();
+  return data;
+}
+function timestamp(value: unknown): number {
+  if (
+    typeof value !== "string" ||
+    !/^20[0-9]{2}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}Z$/.test(value)
+  )
+    invalid();
+  const time = Date.parse(value);
+  if (
+    !Number.isFinite(time) ||
+    new Date(time).toISOString() !== value.replace("Z", ".000Z")
+  )
+    invalid();
+  return time;
+}
+function registry(raw: string): Map<string, Key> {
+  if (Buffer.byteLength(raw, "utf8") > 65536) invalid();
+  let parsed: unknown;
+  try {
+    parsed = JSON.parse(raw);
+  } catch {
+    invalid();
+  }
+  const config = record(parsed, ["version", "keys"]);
+  if (
+    config.version !== 1 ||
+    !Array.isArray(config.keys) ||
+    config.keys.length < 1 ||
+    config.keys.length > 64
+  )
+    invalid();
+  const keys = new Map<string, Key>(),
+    digests = new Set<string>(),
+    clients = new Map<string, number>();
+  for (const entry of config.keys) {
+    const k = record(entry, [
+      "clientId",
+      "keyId",
+      "tokenSHA256",
+      "notBefore",
+      "notAfter",
+    ]);
+    if (
+      typeof k.clientId !== "string" ||
+      !clientPattern.test(k.clientId) ||
+      typeof k.keyId !== "string" ||
+      !keyPattern.test(k.keyId) ||
+      typeof k.tokenSHA256 !== "string" ||
+      !secretPattern.test(k.tokenSHA256)
+    )
+      invalid();
+    const from = timestamp(k.notBefore),
+      until = timestamp(k.notAfter);
+    const id = k.clientId + ":" + k.keyId;
+    const count = (clients.get(k.clientId) || 0) + 1;
+    if (
+      until <= from ||
+      keys.has(id) ||
+      digests.has(k.tokenSHA256) ||
+      count > 16
+    )
+      invalid();
+    clients.set(k.clientId, count);
+    if (clients.size > 32) invalid();
+    digests.add(k.tokenSHA256);
+    keys.set(id, {
+      clientId: k.clientId,
+      digest: Buffer.from(k.tokenSHA256, "hex"),
+      from,
+      until,
+    });
+  }
+  return keys;
+}
+
+/** Optional server-to-server read-only endpoint. No browser, owner or repository authority is inferred. */
+export function createReviewCapabilities(
+  raw?: string,
+  now: () => number = Date.now,
+): RequestHandler {
+  const keys = raw === undefined ? null : registry(raw);
+  // One bounded counter per registered client, shared by its rotation keys.
+  const rates = new Map<string, { minute: number; count: number }>();
+  const dummyDigest = Buffer.alloc(32);
+  return (req, res) => {
+    res.setHeader("Cache-Control", "no-store");
+    res.setHeader("Referrer-Policy", "no-referrer");
+    res.setHeader("X-Content-Type-Options", "nosniff");
+    const fail = (status: number, code: string, retry?: number) => {
+      // Do not keep a rejected request alive while Node drains an unread body.
+      res.setHeader("Connection", "close");
+      if (retry) res.setHeader("Retry-After", String(retry));
+      res.status(status).json({
+        contract,
+        code,
+        requestId: randomBytes(16).toString("hex"),
+        ...(retry ? { retryAfterSeconds: retry } : {}),
+      });
+    };
+    if (!keys || req.originalUrl !== "/service/v1/capabilities")
+      return fail(404, "not-found");
+    if (req.method !== "GET") return fail(405, "invalid-request");
+    // Reject browser context and bodies before authentication; never read or log them.
+    if (
+      [
+        "cookie",
+        "origin",
+        "referer",
+        "sec-fetch-site",
+        "sec-fetch-mode",
+        "sec-fetch-dest",
+        "sec-fetch-user",
+      ].some((h) => req.headers[h] !== undefined)
+    )
+      return fail(403, "forbidden");
+    if (
+      req.headers["transfer-encoding"] !== undefined ||
+      req.headers["content-type"] !== undefined ||
+      (req.headers["content-length"] !== undefined &&
+        req.headers["content-length"] !== "0")
+    )
+      return fail(400, "invalid-request");
+    const one = (name: string): string | undefined => {
+      let count = 0;
+      for (let i = 0; i < req.rawHeaders.length; i += 2)
+        if (req.rawHeaders[i].toLowerCase() === name) count++;
+      const value = req.headers[name];
+      return count === 1 && typeof value === "string" ? value : undefined;
+    };
+    const client = one("x-4open-artifact-client-id"),
+      keyId = one("x-4open-artifact-service-key-id"),
+      authorization = one("authorization");
+    if (
+      !client ||
+      !clientPattern.test(client) ||
+      !keyId ||
+      !keyPattern.test(keyId) ||
+      !authorization ||
+      !/^Bearer [a-f0-9]{64}$/i.test(authorization) ||
+      !secretPattern.test(authorization.slice(7))
+    )
+      return fail(401, "unauthorized");
+    const key = keys.get(client + ":" + keyId);
+    const digest = createHash("sha256")
+      .update(authorization.slice(7), "ascii")
+      .digest();
+    const equal = timingSafeEqual(digest, key ? key.digest : dummyDigest);
+    const time = now();
+    if (
+      !key ||
+      !equal ||
+      !Number.isFinite(time) ||
+      time < key.from ||
+      time >= key.until
+    )
+      return fail(401, "unauthorized");
+    const minute = Math.floor(time / 60000);
+    let rate = rates.get(client);
+    if (!rate || rate.minute !== minute) {
+      rate = { minute, count: 0 };
+      rates.set(client, rate);
+    }
+    if (rate.count >= 60)
+      return fail(
+        429,
+        "rate-limited",
+        Math.max(1, Math.ceil((60000 - (time % 60000)) / 1000)),
+      );
+    rate.count++;
+    // No link, snapshot, protected-delivery or retention support exists yet.
+    res.status(200).json({
+      contract,
+      clientId: client,
+      contracts: [contract],
+      available: [],
+    });
+  };
+}

test/review-capabilities.test.js

@@ -0,0 +1,309 @@
+require("ts-node/register/transpile-only");
+const { expect } = require("chai");
+const { createHash } = require("crypto");
+const http = require("http");
+const process = require("process");
+const express = require("express");
+const {
+  createReviewCapabilities,
+} = require("../src/server/service/review-capabilities");
+const client = "a".repeat(32),
+  token = "b".repeat(64);
+const digest = (value) =>
+  createHash("sha256").update(value, "ascii").digest("hex");
+const key = (overrides = {}) => ({
+  clientId: client,
+  keyId: "rotation-1",
+  tokenSHA256: digest(token),
+  notBefore: "2026-01-01T00:00:00Z",
+  notAfter: "2027-01-01T00:00:00Z",
+  ...overrides,
+});
+const config = (keys) => JSON.stringify({ version: 1, keys });
+const headers = {
+  "X-4open-Artifact-Client-Id": client,
+  "X-4open-Artifact-Service-Key-Id": "rotation-1",
+  Authorization: "Bearer " + token,
+};
+describe("review service capabilities", function () {
+  let server, port, time, downstream;
+  async function start(raw = config([key()])) {
+    time = Date.parse("2026-06-01T00:00:00Z");
+    downstream = 0;
+    const app = express();
+    app.use(
+      "/service",
+      createReviewCapabilities(raw, () => time),
+    );
+    app.use((_req, res) => {
+      downstream++;
+      res.setHeader("Set-Cookie", "browser=1");
+      res.send("browser");
+    });
+    server = http.createServer(app);
+    await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve));
+    port = server.address().port;
+  }
+  function request(options = {}) {
+    return new Promise((resolve, reject) => {
+      const req = http.request(
+        {
+          hostname: "127.0.0.1",
+          port,
+          path: "/service/v1/capabilities",
+          method: "GET",
+          headers,
+          ...options,
+        },
+        (res) => {
+          const parts = [];
+          res.on("data", (data) => parts.push(data));
+          res.on("end", () =>
+            resolve({
+              status: res.statusCode,
+              headers: res.headers,
+              text: Buffer.concat(parts).toString(),
+            }),
+          );
+        },
+      );
+      req.on("error", reject);
+      req.end(options.body);
+    });
+  }
+  afterEach(async () => {
+    if (server) {
+      await new Promise((resolve) => server.close(resolve));
+      server = undefined;
+    }
+  });
+  it("is disabled without a registry and bypasses browser middleware", async () => {
+    // Explicitly omit configuration, rather than start()'s test default.
+    const app = express();
+    app.use("/service", createReviewCapabilities());
+    app.use((_q, r) => r.send("browser"));
+    server = app.listen(0, "127.0.0.1");
+    await new Promise((resolve) => server.once("listening", resolve));
+    port = server.address().port;
+    const result = await request();
+    expect(result.status).to.equal(404);
+    expect(result.headers["set-cookie"]).to.equal(undefined);
+  });
+  it("advertises only the supported contract, bound to the authenticated client", async () => {
+    await start();
+    const result = await request();
+    expect(result.status).to.equal(200);
+    expect(JSON.parse(result.text)).to.deep.equal({
+      contract: "4open.artifacts/1",
+      clientId: client,
+      contracts: ["4open.artifacts/1"],
+      available: [],
+    });
+    expect(result.headers["cache-control"]).to.equal("no-store");
+    expect(result.headers["referrer-policy"]).to.equal("no-referrer");
+    expect(result.headers["set-cookie"]).to.equal(undefined);
+    expect(result.headers["access-control-allow-origin"]).to.equal(undefined);
+    expect(downstream).to.equal(0);
+  });
+  it("rejects missing, wrong, duplicated and cross-client credentials without reflecting secrets", async () => {
+    await start();
+    for (const input of [
+      {},
+      { ...headers, Authorization: "Bearer " + "c".repeat(64) },
+      { ...headers, "X-4open-Artifact-Client-Id": "d".repeat(32) },
+      { ...headers, "X-4open-Artifact-Service-Key-Id": "unknown" },
+      {
+        ...headers,
+        Authorization: [headers.Authorization, headers.Authorization],
+      },
+      { ...headers, "X-4open-Artifact-Client-Id": [client, client] },
+      {
+        ...headers,
+        "X-4open-Artifact-Service-Key-Id": ["rotation-1", "rotation-1"],
+      },
+    ]) {
+      const r = await request({ headers: input });
+      expect(r.status).to.equal(401);
+      const failure = JSON.parse(r.text);
+      expect(failure.code).to.equal("unauthorized");
+      expect(failure.requestId).to.match(/^[a-f0-9]{32}$/);
+      expect(r.text).not.to.contain(token);
+      expect(r.text).not.to.contain(client);
+    }
+    expect(downstream).to.equal(0);
+  });
+  it("has exact paths and rejects browser context, methods, queries and bodies", async () => {
+    await start();
+    for (const path of [
+      "/service/v1/capabilities/",
+      "/service/v1/capabilities?code=secret",
+      "/service/v1/%63apabilities",
+      "/service/v1/bindings",
+      "/service/V1/capabilities",
+    ])
+      expect((await request({ path })).status, path).to.equal(404);
+    for (const method of ["POST", "PUT", "OPTIONS", "HEAD"])
+      expect((await request({ method })).status).to.equal(405);
+    for (const name of [
+      "Cookie",
+      "Origin",
+      "Referer",
+      "Sec-Fetch-Site",
+      "Sec-Fetch-Mode",
+      "Sec-Fetch-Dest",
+      "Sec-Fetch-User",
+    ])
+      expect(
+        (await request({ headers: { ...headers, [name]: "browser" } })).status,
+      ).to.equal(403);
+    expect(
+      (
+        await request({
+          headers: { ...headers, "Content-Type": "application/json" },
+        })
+      ).status,
+    ).to.equal(400);
+    expect(
+      (
+        await request({
+          headers: { ...headers, "Content-Length": "2" },
+          body: "{}",
+        })
+      ).status,
+    ).to.equal(400);
+    expect(downstream).to.equal(0);
+  });
+  it("supports overlapping keys with inclusive start, exclusive expiry and restart revocation", async () => {
+    const other = "c".repeat(64);
+    await start(
+      config([key(), key({ keyId: "rotation-2", tokenSHA256: digest(other) })]),
+    );
+    time = Date.parse(key().notBefore) - 1;
+    expect((await request()).status).to.equal(401);
+    time++;
+    expect((await request()).status).to.equal(200);
+    expect(
+      (
+        await request({
+          headers: {
+            ...headers,
+            "X-4open-Artifact-Service-Key-Id": "rotation-2",
+            Authorization: "Bearer " + other,
+          },
+        })
+      ).status,
+    ).to.equal(200);
+    time = Date.parse(key().notAfter);
+    expect((await request()).status).to.equal(401);
+    await new Promise((resolve) => server.close(resolve));
+    server = undefined;
+    await start(
+      config([key({ keyId: "rotation-2", tokenSHA256: digest(other) })]),
+    );
+    expect((await request()).status).to.equal(401);
+  });
+  it("limits reads per client across keys and resets without allocating anonymous counters", async () => {
+    const other = "c".repeat(64);
+    await start(
+      config([key(), key({ keyId: "rotation-2", tokenSHA256: digest(other) })]),
+    );
+    const results = await Promise.all(
+      Array.from({ length: 60 }, () => request()),
+    );
+    expect(results.every((r) => r.status === 200)).to.equal(true);
+    const r = await request({
+      headers: {
+        ...headers,
+        "X-4open-Artifact-Service-Key-Id": "rotation-2",
+        Authorization: "Bearer " + other,
+      },
+    });
+    expect(r.status).to.equal(429);
+    expect(r.headers["retry-after"]).to.equal("60");
+    expect(JSON.parse(r.text).retryAfterSeconds).to.equal(60);
+    time += 60000;
+    expect((await request()).status).to.equal(200);
+  });
+  it("validates bounded digest-only closed configuration without leaking it", () => {
+    const bad = [
+      "",
+      "{",
+      "x".repeat(65537),
+      "null",
+      "[]",
+      config([]),
+      JSON.stringify({ version: 2, keys: [key()] }),
+      config([key({ secret: token })]),
+      config([key({ tokenSHA256: token.toUpperCase() })]),
+      config([key(), key()]),
+      config([key(), key({ keyId: "other" })]),
+      config([key({ notAfter: key().notBefore })]),
+      config([key({ notBefore: "2026-02-30T00:00:00Z" })]),
+      config([key({ notBefore: "2026-01-01T01:00:00+01:00" })]),
+      config([key({ clientId: "../client" })]),
+      config([key({ keyId: "a:b" })]),
+      config(
+        Array.from({ length: 17 }, (_, i) =>
+          key({ keyId: "k" + i, tokenSHA256: digest(String(i)) }),
+        ),
+      ),
+    ];
+    for (const raw of bad)
+      expect(() => createReviewCapabilities(raw)).to.throw(
+        "Invalid REVIEW_SERVICE_KEYS configuration",
+      );
+  });
+  it("closes rejected incomplete bodies without waiting for upload completion", async function () {
+    this.timeout(5000);
+    await start();
+    const result = await new Promise((resolve, reject) => {
+      const socket = require("net").createConnection({
+        host: "127.0.0.1",
+        port,
+      });
+      let received = "";
+      socket.setTimeout(1500, () => {
+        socket.destroy();
+        reject(new Error("rejected request still waiting for its unread body"));
+      });
+      socket.on("error", reject);
+      socket.on("data", (data) => {
+        received += data.toString();
+      });
+      socket.on("close", () => resolve(received));
+      socket.on("connect", () =>
+        socket.write(
+          "GET /service/v1/capabilities HTTP/1.1\r\nHost: localhost\r\nContent-Length: 10000000\r\nConnection: keep-alive\r\n\r\n",
+        ),
+      );
+    });
+    expect(result).to.contain("HTTP/1.1 400");
+    expect(result.toLowerCase()).to.contain("connection: close");
+    expect(downstream).to.equal(0);
+  });
+  if (process.env.RUN_REVIEW_SERVICE_BENCHMARK === "1")
+    it("records bounded loopback read performance", async () => {
+      await start();
+      const samples = [];
+      for (let i = 0; i < 150; i++) {
+        if (i % 50 === 0) time += 60000;
+        const begin = process.hrtime.bigint();
+        const r = await request();
+        expect(r.status).to.equal(200);
+        samples.push(Number(process.hrtime.bigint() - begin) / 1e6);
+      }
+      samples.sort((a, b) => a - b);
+      const report = {
+        requests: samples.length,
+        transport: "loopback HTTP, fresh connections, synthetic credentials",
+        meanMs: samples.reduce((a, b) => a + b, 0) / samples.length,
+        medianMs: samples[75],
+        p95Ms: samples[142],
+        providerCalls: false,
+      };
+      require("fs").writeFileSync(
+        "/tmp/upstream-capabilities-perf.json",
+        JSON.stringify(report, null, 2) + "\n",
+      );
+    });
+});