From eaa84e3af58256ed2622e9ef6178b0afaa57a080 Mon Sep 17 00:00:00 2001 From: Thomas Durieux <5577568+tdurieux@users.noreply.github.com> Date: Sun, 27 Sep 2026 12:51:28 +0000 Subject: [PATCH] Add optional authenticated review-service capabilities endpoint --- docs/review-service-capabilities.md | 74 ++++++ src/server/index.ts | 3 + src/server/service/review-capabilities.ts | 202 ++++++++++++++++ test/review-capabilities.test.js | 281 ++++++++++++++++++++++ 4 files changed, 560 insertions(+) create mode 100644 docs/review-service-capabilities.md create mode 100644 src/server/service/review-capabilities.ts create mode 100644 test/review-capabilities.test.js diff --git a/docs/review-service-capabilities.md b/docs/review-service-capabilities.md new file mode 100644 index 0000000..97bbc3d --- /dev/null +++ b/docs/review-service-capabilities.md @@ -0,0 +1,74 @@ +# 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. +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 `, `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 seven +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 recorded run averaged 1.708 ms, median 1.657 ms and p95 1.974 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. + +The isolated new module passes strict TypeScript checking. The repository-wide +check fails with the same diagnostics on unchanged base `083544b`: ES6 does not +support existing named regex groups, an existing Octokit response type is missing +`retryCount`, and the existing library target lacks `String.at`. These are retained +as baseline limitations; this change does not claim a passing full upstream build. + +The endpoint is implemented and tested locally. Upstream deployment, private key +distribution, ownership consent and the remaining artifact workflows are pending. diff --git a/src/server/index.ts b/src/server/index.ts index eac73e1..158b0e4 100644 --- a/src/server/index.ts +++ b/src/server/index.ts @@ -38,6 +38,8 @@ import config from "../config"; import { resolveTrustProxy, isCloudflareIP } from "./trustProxy"; import { createLogger, serializeError } from "../core/logger"; +import { createReviewCapabilities } from "./service/review-capabilities"; + const logger = createLogger("server"); // Lazily build the templated index.html on first request so the server @@ -101,6 +103,7 @@ function indexResponse(req: express.Request, res: express.Response) { export default async function start() { const app = express(); 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. diff --git a/src/server/service/review-capabilities.ts b/src/server/service/review-capabilities.ts new file mode 100644 index 0000000..477a021 --- /dev/null +++ b/src/server/service/review-capabilities.ts @@ -0,0 +1,202 @@ +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 { + if (!value || typeof value !== "object" || Array.isArray(value)) invalid(); + const data = value as Record; + 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 { + 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(), + digests = new Set(), + clients = new Map(); + 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(); + 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) => { + 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: [], + }); + }; +} diff --git a/test/review-capabilities.test.js b/test/review-capabilities.test.js new file mode 100644 index 0000000..2c4e44d --- /dev/null +++ b/test/review-capabilities.test.js @@ -0,0 +1,281 @@ +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", + ); + }); + 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", + ); + }); +});