From 0ecd935528a4eda1947d237c4dc965c38adc35cd Mon Sep 17 00:00:00 2001 From: cluade-reviewer-andresmgsl Date: Wed, 22 Jul 2026 23:17:20 +0000 Subject: [PATCH] =?UTF-8?q?install-apt:=20address=20review=20nits=20?= =?UTF-8?q?=E2=80=94=20apt-readable=20file=20modes,=20test=20cleanup,=20cl?= =?UTF-8?q?earer=20refusal=20error?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - chmod 0644 every keyring and sources.list entry after writing: tee inherits the caller's umask, and under e.g. umask 077 apt's unprivileged _apt user could not read the keyring - Tests now run the script under umask 077 and assert the 0644 modes (mutation-checked: dropping the chmod fails a test), and remove their temp directories on exit - Refusal error now states that metadata was already refreshed before concluding the existing nodesource.list is unsuitable Verified on fresh debian:13 under umask 077: all four files 0644, install succeeds. npm test 32/32. Co-Authored-By: Claude Opus 4.8 (1M context) --- scripts/install-apt.sh | 12 ++++++++++-- test/install-apt.test.js | 15 ++++++++++++++- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/scripts/install-apt.sh b/scripts/install-apt.sh index 4ac6efe..9e68c5d 100755 --- a/scripts/install-apt.sh +++ b/scripts/install-apt.sh @@ -63,14 +63,18 @@ ensure_nodejs_source() { local ns_keyring="$APT_ETC/keyrings/nodesource.asc" local ns_list="$APT_ETC/sources.list.d/nodesource.list" if [ -e "$ns_list" ]; then - echo "error: nodejs >= $NODE_MIN is unavailable and $ns_list already exists;" >&2 - echo "refusing to overwrite it. Point it at a Node >= 22 release and re-run." >&2 + echo "error: even after refreshing apt metadata, no source provides nodejs >= $NODE_MIN," >&2 + echo "and $ns_list already exists; refusing to overwrite it." >&2 + echo "Point it at a Node >= 22 release (e.g. node_22.x) and re-run." >&2 exit 1 fi echo "Adding NodeSource (Node 22) ..." curl -fsSL https://deb.nodesource.com/gpgkey/nodesource-repo.gpg.key | $SUDO tee "$ns_keyring" >/dev/null echo "deb [signed-by=$ns_keyring] https://deb.nodesource.com/node_22.x nodistro main" \ | $SUDO tee "$ns_list" >/dev/null + # tee inherits our umask; apt's unprivileged _apt user must be able to + # read these. + $SUDO chmod 0644 "$ns_keyring" "$ns_list" update_only_source "$ns_list" node_candidate_ok || { echo "error: still no nodejs >= $NODE_MIN available after adding NodeSource" >&2; exit 1; } } @@ -80,6 +84,9 @@ $SUDO install -d -m 0755 "$APT_ETC/keyrings" curl -fsSL "$FORGE_URL/api/packages/$OWNER/debian/repository.key" | $SUDO tee "$KEYRING" >/dev/null echo "deb [signed-by=$KEYRING] $FORGE_URL/api/packages/$OWNER/debian $DISTRIBUTION $COMPONENT" \ | $SUDO tee "$LIST" >/dev/null +# tee inherits our umask; apt's unprivileged _apt user must be able to +# read these. +$SUDO chmod 0644 "$KEYRING" "$LIST" # Newer apt verifies with sqv (Sequoia), which rejects the signature Forgejo # currently produces for its Debian registry (malformed Ed25519 MPI encoding @@ -94,6 +101,7 @@ if ! update_only_source "$LIST"; then echo echo "deb [trusted=yes] $FORGE_URL/api/packages/$OWNER/debian $DISTRIBUTION $COMPONENT" \ | $SUDO tee "$LIST" >/dev/null + $SUDO chmod 0644 "$LIST" update_only_source "$LIST" fi diff --git a/test/install-apt.test.js b/test/install-apt.test.js index c78b7dc..51dd362 100644 --- a/test/install-apt.test.js +++ b/test/install-apt.test.js @@ -14,8 +14,12 @@ const SCRIPT = path.join(__dirname, '..', 'scripts', 'install-apt.sh'); // candAfterNodesource Candidate after an update once nodesource.list exists // The apt-cache stub localizes the "Candidate:" label unless LC_ALL=C is set, // so every scenario doubles as a regression test for locale-safe parsing. +const cleanups = []; +process.on('exit', () => { for (const dir of cleanups) fs.rmSync(dir, { recursive: true, force: true }); }); + function runScenario({ candInitial, candAfterUpdate, candAfterNodesource, preexistingNodesourceList }) { const root = fs.mkdtempSync(path.join(os.tmpdir(), 'stoke-apt-test-')); + cleanups.push(root); const bin = path.join(root, 'bin'); const state = path.join(root, 'state'); const aptEtc = path.join(root, 'etc', 'apt'); @@ -58,7 +62,9 @@ function runScenario({ candInitial, candAfterUpdate, candAfterNodesource, preexi 'exit 0', ].join('\n')); - const res = spawnSync('bash', [SCRIPT], { + // Restrictive umask: apt-readable 0644 files must come from the script's + // explicit chmod, not from a lucky default. + const res = spawnSync('bash', ['-c', 'umask 077 && exec bash "$1"', 'bash', SCRIPT], { encoding: 'utf8', env: { ...process.env, @@ -72,11 +78,15 @@ function runScenario({ candInitial, candAfterUpdate, candAfterNodesource, preexi }); const read = (p) => (fs.existsSync(p) ? fs.readFileSync(p, 'utf8') : null); + const mode = (p) => (fs.existsSync(p) ? fs.statSync(p).mode & 0o777 : null); return { res, aptEtc, nodesourceList: read(path.join(aptEtc, 'sources.list.d', 'nodesource.list')), + nodesourceListMode: mode(path.join(aptEtc, 'sources.list.d', 'nodesource.list')), nodesourceKey: read(path.join(aptEtc, 'keyrings', 'nodesource.asc')), + nodesourceKeyMode: mode(path.join(aptEtc, 'keyrings', 'nodesource.asc')), + forgeKeyMode: mode(path.join(aptEtc, 'keyrings', 'forgejo-heavy-duty.asc')), aptGetLog: read(path.join(state, 'apt-get.log')) || '', }; } @@ -87,6 +97,7 @@ test('suitable nodejs candidate already available: installs without touching Nod assert.equal(s.res.status, 0, s.res.stderr); assert.equal(s.nodesourceList, null); assert.match(s.aptGetLog, /install -y stoke/); + assert.equal(s.forgeKeyMode, 0o644, 'forge keyring must be readable by _apt'); }); test('no cached metadata: refreshes apt lists before deciding, no NodeSource needed', () => { @@ -105,6 +116,8 @@ test('distro nodejs too old: bootstraps NodeSource and installs', () => { assert.equal(s.res.status, 0, s.res.stderr); assert.match(s.nodesourceList, /deb \[signed-by=.*nodesource\.asc\] https:\/\/deb\.nodesource\.com\/node_22\.x nodistro main/); assert.equal(s.nodesourceKey, 'FAKE-KEY\n'); + assert.equal(s.nodesourceKeyMode, 0o644, 'NodeSource keyring must be readable by _apt'); + assert.equal(s.nodesourceListMode, 0o644, 'NodeSource list must be readable by _apt'); assert.match(s.aptGetLog, /install -y stoke/); });