From b57b25a4d7e2ea1caa7c4bd617a428d64aef0c4d Mon Sep 17 00:00:00 2001 From: John McLear Date: Wed, 8 Apr 2026 10:56:13 +0100 Subject: [PATCH] fix: setup-trusted-publishers.sh works with real npm trust CLI (#7491) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: setup-trusted-publishers.sh works with real npm trust CLI Two issues found when running the script for the first time after #7490: 1. `npm trust github --file` wants ONLY the workflow filename basename (e.g. `test-and-release.yml`), not the full `.github/workflows/test-and-release.yml` path. npm errors out with "GitHub Actions workflow must be just a file not a path" otherwise. Constants updated. 2. `npm trust github` requires 2FA on accounts that have it enabled, and there is no way to disable that requirement. Add a `--otp ` pass-through flag and forward it to every call so a maintainer can batch-process multiple packages within a single TOTP window. Documented the limitation in the script header. Also reword the call site so the npm command line is built without shell-string round-tripping (passing $CMD through `$( $CMD )` was unrelated to this bug but was bad practice). Co-Authored-By: Claude Opus 4.6 (1M context) * fix: setup-trusted-publishers.sh recognizes 409 as already-configured When --skip-existing is set, treat HTTP 409 Conflict from POST /-/package//trust as 'already configured' so re-runs of the bulk script don't fail on packages that were configured in a previous run. Co-Authored-By: Claude Opus 4.6 (1M context) * test: cover setup-trusted-publishers.sh, harden against set -e, document --otp Addresses qodo review on #7491: - Add backend regression test that shims `npm` on PATH and asserts `--file` is given the workflow basename (never a path), `--otp` is forwarded to every `npm trust github` call when supplied, and the loop survives a non-zero exit so `--skip-existing` can absorb 409 Conflict responses from the registry. - Wrap the `npm trust github` invocation in `set +e` / `set -e`. The `if configure_one` already shields the function from errexit in practice, but a future refactor moving the call site out of an `if` would silently reintroduce the bug — the explicit shim makes intent obvious and survives such refactors. - Document `--otp` and the 2FA / TOTP-expiry workflow in doc/npm-trusted-publishing.md so maintainers don't follow the docs and hit EOTP. Co-Authored-By: Claude Opus 4.6 (1M context) --------- Co-authored-by: Claude Opus 4.6 (1M context) --- bin/setup-trusted-publishers.sh | 47 ++++- doc/npm-trusted-publishing.md | 12 ++ .../backend/specs/setup-trusted-publishers.ts | 196 ++++++++++++++++++ 3 files changed, 246 insertions(+), 9 deletions(-) create mode 100644 src/tests/backend/specs/setup-trusted-publishers.ts diff --git a/bin/setup-trusted-publishers.sh b/bin/setup-trusted-publishers.sh index 60004a201..953e27cab 100755 --- a/bin/setup-trusted-publishers.sh +++ b/bin/setup-trusted-publishers.sh @@ -14,6 +14,12 @@ # bin/setup-trusted-publishers.sh --dry-run # print what would happen # bin/setup-trusted-publishers.sh --packages ep_align,ep_webrtc # bin/setup-trusted-publishers.sh --skip-existing # don't fail if already configured +# bin/setup-trusted-publishers.sh --otp 123456 # supply 2FA OTP up front +# +# Note: `npm trust github` requires 2FA. If your account has 2FA enabled +# (it should), pass --otp once and the same code will be reused for every +# package call inside the same minute. The TOTP code typically expires +# every 30s, so you may need to run the script in chunks via --packages. # # Each package gets a GitHub Actions trusted publisher pointing at the # canonical workflow file used by that package family: @@ -25,8 +31,10 @@ set -eu -PLUGIN_WORKFLOW=".github/workflows/test-and-release.yml" -CORE_WORKFLOW=".github/workflows/releaseEtherpad.yml" +# `npm trust github --file` wants ONLY the workflow filename (basename), +# not the full .github/workflows/ path. +PLUGIN_WORKFLOW="test-and-release.yml" +CORE_WORKFLOW="releaseEtherpad.yml" CORE_PACKAGE="ep_etherpad" CORE_REPO="etherpad-lite" ORG="ether" @@ -34,6 +42,7 @@ ORG="ether" DRY_RUN=0 SKIP_EXISTING=0 PACKAGES="" +OTP="" usage() { sed -n '2,/^$/p' "$0" | sed 's/^# \?//' @@ -46,6 +55,7 @@ while [ $# -gt 0 ]; do --dry-run) DRY_RUN=1; shift ;; --skip-existing) SKIP_EXISTING=1; shift ;; --packages) PACKAGES="$2"; shift 2 ;; + --otp) OTP="$2"; shift 2 ;; -h|--help) usage 0 ;; *) printf 'Unknown flag: %s\n' "$1" >&2; usage 1 ;; esac @@ -98,24 +108,43 @@ configure_one() { WORKFLOW="$PLUGIN_WORKFLOW" fi - CMD="npm trust github $PKG --repository $ORG/$REPO --file $WORKFLOW --yes" printf '%-40s -> %s/%s @ %s\n' "$PKG" "$ORG" "$REPO" "$WORKFLOW" if [ "$DRY_RUN" = "1" ]; then - printf ' (dry-run) would run: %s\n' "$CMD" + printf ' (dry-run) would run: npm trust github %s --repository %s/%s --file %s --yes\n' \ + "$PKG" "$ORG" "$REPO" "$WORKFLOW" return 0 fi - if OUTPUT=$($CMD 2>&1); then + # Disable -e around the npm call so a non-zero exit can never short-circuit + # the STATUS / --skip-existing handling below. In practice the wrapping + # `if configure_one` already suppresses errexit inside this function (POSIX + # errexit-in-conditional behaviour), but relying on that is fragile — anyone + # later refactoring the call site out of an `if` would silently reintroduce + # the bug. The explicit shim makes the intent obvious and survives such + # refactors. + set +e + if [ -n "$OTP" ]; then + OUTPUT=$(npm trust github "$PKG" --repository "$ORG/$REPO" --file "$WORKFLOW" --otp "$OTP" --yes 2>&1) + else + OUTPUT=$(npm trust github "$PKG" --repository "$ORG/$REPO" --file "$WORKFLOW" --yes 2>&1) + fi + STATUS=$? + set -e + if [ "$STATUS" -eq 0 ]; then printf ' ok\n' else + # The npm registry returns 409 Conflict when a trust relationship + # already exists (you can only have one per package today). Treat + # that as success when --skip-existing is set, alongside the older + # "already exists/configured" string match. if [ "$SKIP_EXISTING" = "1" ] && \ - echo "$OUTPUT" | grep -qiE "already (exists|configured)"; then + echo "$OUTPUT" | grep -qiE "409 Conflict|already (exists|configured)"; then printf ' already configured (skipped)\n' - else - printf ' FAILED:\n%s\n' "$OUTPUT" | sed 's/^/ /' - return 1 + return 0 fi + printf ' FAILED:\n%s\n' "$OUTPUT" | sed 's/^/ /' + return 1 fi } diff --git a/doc/npm-trusted-publishing.md b/doc/npm-trusted-publishing.md index 67812170e..2ba9a8841 100644 --- a/doc/npm-trusted-publishing.md +++ b/doc/npm-trusted-publishing.md @@ -45,8 +45,20 @@ bin/setup-trusted-publishers.sh --packages ep_align,ep_webrtc # Or ignore packages that are already configured (the registry only allows # one trust relationship per package today) bin/setup-trusted-publishers.sh --skip-existing + +# Supply a 2FA OTP up front (required if your npm account has 2FA enabled — +# it should). The same OTP is reused for every package call inside the same +# minute, so for large batches you may need to chunk via --packages. +bin/setup-trusted-publishers.sh --otp 123456 ``` +> **2FA / OTP note.** `npm trust github` requires an OTP whenever the +> account has 2FA enabled. Without `--otp`, npm will prompt interactively +> per package, which is unworkable in bulk. Pass `--otp ` once and the +> script will forward it to every `npm trust github` call. TOTP codes +> typically expire every 30 seconds, so for >30s runs split the work with +> `--packages ep_a,ep_b,...` and re-run with a fresh code. + The script discovers all non-archived `ether/ep_*` repos via `gh repo list` and runs `npm trust github --repository / --file --yes` for each one. `ep_etherpad` is mapped to the `etherpad-lite` repo and diff --git a/src/tests/backend/specs/setup-trusted-publishers.ts b/src/tests/backend/specs/setup-trusted-publishers.ts new file mode 100644 index 000000000..ea95d28e1 --- /dev/null +++ b/src/tests/backend/specs/setup-trusted-publishers.ts @@ -0,0 +1,196 @@ +'use strict'; + +// Regression tests for bin/setup-trusted-publishers.sh. +// +// We can't and don't want to call the real `npm trust github` registry from +// CI, so we shim `npm` with a fake binary placed earlier on $PATH. The fake +// binary records every invocation to a log file; the assertions below replay +// that log to verify: +// +// 1. `--file` is given the workflow *basename* (`test-and-release.yml`), +// never a full path. Real `npm trust github` rejects paths. +// 2. `--otp ` is forwarded to every `npm trust github` call when the +// script is invoked with `--otp`. +// 3. The script doesn't bail out under `set -eu` when `npm trust github` +// exits non-zero — it must keep going so `--skip-existing` can take +// effect on 409 Conflict responses. + +import {strict as assert} from 'assert'; +import {spawnSync} from 'child_process'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; + +const REPO_ROOT = path.resolve(__dirname, '..', '..', '..', '..'); +const SCRIPT = path.join(REPO_ROOT, 'bin', 'setup-trusted-publishers.sh'); + +type Invocation = string[]; + +const makeFakeNpm = ( + workdir: string, + opts: {trustExitCode?: number; trustStderr?: string} = {}, +): {logFile: string; binDir: string} => { + const binDir = path.join(workdir, 'bin'); + fs.mkdirSync(binDir, {recursive: true}); + const logFile = path.join(workdir, 'npm-calls.log'); + fs.writeFileSync(logFile, ''); + + // The fake npm records each invocation as one line of NUL-separated args + // followed by a record-separator (0x1e). That keeps quoting / spacing safe + // when we parse it back in JS. + const trustExit = opts.trustExitCode ?? 0; + const trustStderr = (opts.trustStderr ?? '').replace(/'/g, `'\\''`); + const script = `#!/bin/sh +# Fake npm used by setup-trusted-publishers regression tests. +case "$1" in + --version) + echo "11.5.1" + exit 0 + ;; + whoami) + echo "mockuser" + exit 0 + ;; + trust) + { + for arg in "$@"; do + printf '%s\\0' "$arg" >> "${logFile}" + done + printf '\\036' >> "${logFile}" + } + if [ -n '${trustStderr}' ]; then + printf '%s\\n' '${trustStderr}' >&2 + fi + exit ${trustExit} + ;; + *) + # Unknown subcommand — be loud so a regression in argv parsing surfaces. + echo "fake-npm: unexpected invocation: $*" >&2 + exit 99 + ;; +esac +`; + const npmPath = path.join(binDir, 'npm'); + fs.writeFileSync(npmPath, script); + fs.chmodSync(npmPath, 0o755); + return {logFile, binDir}; +}; + +const readInvocations = (logFile: string): Invocation[] => { + const raw = fs.readFileSync(logFile, 'utf8'); + if (raw.length === 0) return []; + return raw + .split('\x1e') + .filter((rec) => rec.length > 0) + .map((rec) => rec.split('\x00').filter((a) => a.length > 0)); +}; + +const runScript = ( + binDir: string, + args: string[], +): {status: number | null; stdout: string; stderr: string} => { + const env = {...process.env, PATH: `${binDir}:${process.env.PATH ?? ''}`}; + const result = spawnSync('sh', [SCRIPT, ...args], {env, encoding: 'utf8'}); + return {status: result.status, stdout: result.stdout, stderr: result.stderr}; +}; + +describe(__filename, function () { + let workdir: string; + + beforeEach(function () { + workdir = fs.mkdtempSync(path.join(os.tmpdir(), 'tp-test-')); + }); + + afterEach(function () { + fs.rmSync(workdir, {recursive: true, force: true}); + }); + + it('passes the workflow basename, not a path, to --file', function () { + const {logFile, binDir} = makeFakeNpm(workdir); + const {status, stderr} = runScript(binDir, [ + '--packages', 'ep_align,ep_webrtc', + '--skip-existing', + ]); + assert.equal(status, 0, `script exited ${status}: ${stderr}`); + + const calls = readInvocations(logFile); + assert.equal(calls.length, 2, `expected 2 npm trust calls, got ${calls.length}`); + + for (const call of calls) { + const fileIdx = call.indexOf('--file'); + assert.notEqual(fileIdx, -1, `--file missing in: ${call.join(' ')}`); + const fileArg = call[fileIdx + 1]; + assert.equal( + fileArg, 'test-and-release.yml', + `--file got "${fileArg}", expected the basename "test-and-release.yml"`, + ); + assert.ok( + !fileArg.includes('/'), + `--file value "${fileArg}" must not contain a path separator`, + ); + } + }); + + it('uses releaseEtherpad.yml (basename) for ep_etherpad', function () { + const {logFile, binDir} = makeFakeNpm(workdir); + const {status} = runScript(binDir, [ + '--packages', 'ep_etherpad', + '--skip-existing', + ]); + assert.equal(status, 0); + + const calls = readInvocations(logFile); + assert.equal(calls.length, 1); + const fileIdx = calls[0].indexOf('--file'); + assert.equal(calls[0][fileIdx + 1], 'releaseEtherpad.yml'); + }); + + it('forwards --otp to every npm trust github call', function () { + const {logFile, binDir} = makeFakeNpm(workdir); + const {status} = runScript(binDir, [ + '--otp', '654321', + '--packages', 'ep_align,ep_webrtc,ep_etherpad', + '--skip-existing', + ]); + assert.equal(status, 0); + + const calls = readInvocations(logFile); + assert.equal(calls.length, 3); + for (const call of calls) { + const otpIdx = call.indexOf('--otp'); + assert.notEqual(otpIdx, -1, `--otp missing in: ${call.join(' ')}`); + assert.equal(call[otpIdx + 1], '654321'); + } + }); + + it('omits --otp when the flag was not given', function () { + const {logFile, binDir} = makeFakeNpm(workdir); + runScript(binDir, ['--packages', 'ep_align', '--skip-existing']); + + const calls = readInvocations(logFile); + assert.equal(calls.length, 1); + assert.equal(calls[0].indexOf('--otp'), -1, `unexpected --otp in: ${calls[0].join(' ')}`); + }); + + it('keeps going under set -eu when npm trust github exits non-zero', function () { + // Simulate the registry's 409 Conflict response. Without the set +e + // shim around the npm call, the script would die on the first package + // and never reach the second one — so seeing TWO recorded calls proves + // the loop survived a non-zero exit. + const {logFile, binDir} = makeFakeNpm(workdir, { + trustExitCode: 1, + trustStderr: 'npm error code E409\nnpm error 409 Conflict - already configured', + }); + const {status} = runScript(binDir, [ + '--packages', 'ep_align,ep_webrtc', + '--skip-existing', + ]); + assert.equal(status, 0, 'script should exit 0 when --skip-existing absorbs 409s'); + + const calls = readInvocations(logFile); + assert.equal( + calls.length, 2, + `expected the loop to invoke npm twice despite the first failure, got ${calls.length}`, + ); + }); +});