From 2edb680fa6c792c0daa63b92d0fd85cd414d92e0 Mon Sep 17 00:00:00 2001 From: Joey Stanford Date: Tue, 21 Jul 2026 08:04:31 -0600 Subject: [PATCH 1/2] fix: force-reinstall Flatpak pnpm generator and harden offline install - pip install --force-reinstall --no-cache-dir for flatpak-node-generator in flatpak.yaml and ci.yaml. The flathub-infra image preinstalls flatpak_node_generator==0.1.0; upstream keeps that version across commits, so plain pip install is a no-op and leaves the pre-ac5a296a storeDir= generator that breaks pnpm-workspace.yaml parse (Flatpak run 29836001407). - Fail-fast after source generation if storeDir= still appears in generated-sources.json; check:flatpak requires --force-reinstall on both workflows. - Strip generator-appended storeDir / store-dir lines in flatpak-pnpm-install before offline install so --store-dir (sandbox path) always wins. - Quote PNPM_CONFIG_* env values as strings so flatpak-builder GStrv accepts them (unquoted true/false dropped the whole env map). Regression from Build Flatpak after #704; bumping the pin alone was insufficient. --- .github/workflows/ci.yaml | 5 +- .github/workflows/flatpak.yaml | 17 +++- docs/development-environment.md | 5 +- org.coloradomesh.MeshClient.yml | 6 +- scripts/check-flatpak-offline-pnpm.mjs | 2 +- scripts/check-flatpak.mjs | 14 +++- scripts/flatpak-pnpm-bin.test.mjs | 23 +++--- scripts/flatpak-pnpm-install.mjs | 46 +++++++++++ scripts/flatpak-pnpm-install.test.mjs | 39 ++++++++- scripts/flatpakOfflinePnpmEnv.mjs | 20 +++-- scripts/flatpakPnpmStoreVersion.mjs | 101 +++++++++++++++++++++++ scripts/flatpakPnpmStoreVersion.test.mjs | 51 ++++++++++++ 12 files changed, 302 insertions(+), 27 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 9e4d25e5e..227704e8c 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -55,7 +55,10 @@ jobs: run: | FBTOOLS=git+https://github.com/flatpak/flatpak-builder-tools # Keep in sync with scripts/flatpakPnpmStoreVersion.mjs FLATPAK_NODE_GENERATOR_COMMIT. - pip3 install "${FBTOOLS}@ac5a296ac6111aa2319daf532f609a067b88d8a9#subdirectory=node" + # --force-reinstall: same 0.1.0 version across commits; plain pip is a no-op on + # preinstalled images and leaves the storeDir= generator. + pip3 install --force-reinstall --no-cache-dir \ + "${FBTOOLS}@ac5a296ac6111aa2319daf532f609a067b88d8a9#subdirectory=node" - name: Check Flatpak offline pnpm sources run: pnpm run check:flatpak-offline-pnpm diff --git a/.github/workflows/flatpak.yaml b/.github/workflows/flatpak.yaml index 644d9a128..97c03ea7d 100644 --- a/.github/workflows/flatpak.yaml +++ b/.github/workflows/flatpak.yaml @@ -90,13 +90,28 @@ jobs: run: | FBTOOLS=git+https://github.com/flatpak/flatpak-builder-tools # ac5a296a+: YAML storeDir: for pnpm v11 (storeDir= breaks pnpm-workspace.yaml). - pip3 install "${FBTOOLS}@ac5a296ac6111aa2319daf532f609a067b88d8a9#subdirectory=node" + # --force-reinstall: image may preinstall flatpak_node_generator==0.1.0; same + # version makes plain pip install a no-op (leaves storeDir= generator). + pip3 install --force-reinstall --no-cache-dir \ + "${FBTOOLS}@ac5a296ac6111aa2319daf532f609a067b88d8a9#subdirectory=node" # pnpm 11+ uses store v11; flatpak-node-generator defaults to v10 for lockfile 9. PNPM_MAJOR="$(node -p "require('./package.json').packageManager.match(/^pnpm@(\\d+)/)[1]")" STORE_VERSION="v${PNPM_MAJOR}" flatpak-node-generator pnpm pnpm-lock.yaml \ --pnpm-store-version "$STORE_VERSION" \ -o flatpak/generated-sources.json + # Fail fast if a stale generator still emitted npmrc-style storeDir=. + node --input-type=module <<'EOF' + import fs from 'node:fs'; + import { generatedSourcesStoreDirYamlViolations } from './scripts/flatpakPnpmStoreVersion.mjs'; + const sources = JSON.parse(fs.readFileSync('flatpak/generated-sources.json', 'utf8')); + const violations = generatedSourcesStoreDirYamlViolations(sources); + if (violations.length) { + for (const v of violations) console.error(v.message); + process.exit(1); + } + console.log('generated-sources storeDir YAML: ok'); + EOF # flatpak/flatpak-github-actions v6 appends -${arch} to the artifact name on upload; # keep bundle arch-agnostic here to avoid org.coloradomesh.MeshClient-aarch64-aarch64.flatpak. diff --git a/docs/development-environment.md b/docs/development-environment.md index beefe4232..ec88da652 100644 --- a/docs/development-environment.md +++ b/docs/development-environment.md @@ -274,7 +274,10 @@ flatpak install --user -y flathub org.electronjs.Electron2.BaseApp//24.08 **3. Generate offline pnpm sources** (re-run whenever `pnpm-lock.yaml` changes) ```bash -pip install flatpak-node-generator +# Prefer the CI pin with --force-reinstall (images may preinstall an older 0.1.0). +# See scripts/flatpakPnpmStoreVersion.mjs FLATPAK_NODE_GENERATOR_GIT. +pip install --force-reinstall --no-cache-dir \ + "git+https://github.com/flatpak/flatpak-builder-tools@ac5a296ac6111aa2319daf532f609a067b88d8a9#subdirectory=node" # Must match package.json packageManager major (pnpm 11 → v11). Generator defaults to v10. PNPM_MAJOR="$(node -p "require('./package.json').packageManager.match(/^pnpm@(\\d+)/)[1]")" STORE_VERSION="v${PNPM_MAJOR}" diff --git a/org.coloradomesh.MeshClient.yml b/org.coloradomesh.MeshClient.yml index fe8dbe9a7..e4d86e5af 100644 --- a/org.coloradomesh.MeshClient.yml +++ b/org.coloradomesh.MeshClient.yml @@ -38,9 +38,11 @@ modules: env: PNPM_HOME: /run/build/mesh-client/.pnpm pnpm_config_cache: /run/build/mesh-client/.npm + # Values must be YAML strings (quoted). Unquoted true/false become JSON + # booleans and flatpak-builder drops the entire env map (GStrv warning). # pnpm 11 supply-chain / verifyDepsBeforeRun must not hit the registry offline. - PNPM_CONFIG_TRUST_LOCKFILE: true - PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN: false + PNPM_CONFIG_TRUST_LOCKFILE: 'true' + PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN: 'false' build-commands: # Install pnpm standalone from archived release (no network needed). # Archive extracts to pnpm-vendor/ (strip-components: 0) so the root `pnpm` binary is diff --git a/scripts/check-flatpak-offline-pnpm.mjs b/scripts/check-flatpak-offline-pnpm.mjs index fa41a5f08..1d41fb98a 100644 --- a/scripts/check-flatpak-offline-pnpm.mjs +++ b/scripts/check-flatpak-offline-pnpm.mjs @@ -63,7 +63,7 @@ function generateOfflineSources(expectedStoreVersion) { `flatpak-node-generator not found on PATH (and FLATPAK_NODE_GENERATOR unset).\n` + ` Install the CI pin, then re-run:\n` + ` python3 -m venv .cache/flatpak-node-venv\n` + - ` .cache/flatpak-node-venv/bin/pip install '${FLATPAK_NODE_GENERATOR_GIT}'\n` + + ` .cache/flatpak-node-venv/bin/pip install --force-reinstall --no-cache-dir '${FLATPAK_NODE_GENERATOR_GIT}'\n` + ` export PATH="$PWD/.cache/flatpak-node-venv/bin:$PATH"\n` + ` pnpm run check:flatpak-offline-pnpm`, }; diff --git a/scripts/check-flatpak.mjs b/scripts/check-flatpak.mjs index 8cfe7bd0a..890400f7d 100644 --- a/scripts/check-flatpak.mjs +++ b/scripts/check-flatpak.mjs @@ -4,6 +4,7 @@ import path from 'path'; import { fileURLToPath } from 'url'; import { offlinePnpmEnvContractViolations } from './flatpakOfflinePnpmEnv.mjs'; import { + flatpakWorkflowGeneratorInstallViolations, flatpakWorkflowStoreVersionViolations, storeVersionFromPackageManager, } from './flatpakPnpmStoreVersion.mjs'; @@ -15,6 +16,7 @@ const METAINFO = path.join(ROOT, 'flatpak', 'org.coloradomesh.MeshClient.metainf const DESKTOP = path.join(ROOT, 'flatpak', 'org.coloradomesh.MeshClient.desktop'); const MANIFEST = path.join(ROOT, 'org.coloradomesh.MeshClient.yml'); const FLATPAK_WORKFLOW = path.join(ROOT, '.github/workflows/flatpak.yaml'); +const CI_WORKFLOW = path.join(ROOT, '.github/workflows/ci.yaml'); const WRAPPER = path.join(ROOT, 'flatpak', 'mesh-client-wrapper.sh'); const PKG = path.join(ROOT, 'package.json'); const EXPECTED_APP_ID = 'org.coloradomesh.MeshClient'; @@ -189,7 +191,17 @@ function checkFlatpakWorkflowStoreVersion(pkg) { if (!expected) return violations; const yaml = fs.readFileSync(FLATPAK_WORKFLOW, 'utf8'); - return flatpakWorkflowStoreVersionViolations(yaml, expected); + violations.push(...flatpakWorkflowStoreVersionViolations(yaml, expected)); + + // CI installs the same pin for check:flatpak-offline-pnpm — must also force-reinstall. + if (fs.existsSync(CI_WORKFLOW)) { + const ciYaml = fs.readFileSync(CI_WORKFLOW, 'utf8'); + violations.push( + ...flatpakWorkflowGeneratorInstallViolations(ciYaml, '.github/workflows/ci.yaml'), + ); + } + + return violations; } function checkManifestBranchAndElectronPayload(pkg) { diff --git a/scripts/flatpak-pnpm-bin.test.mjs b/scripts/flatpak-pnpm-bin.test.mjs index 10f3c3717..7ca0e0da5 100644 --- a/scripts/flatpak-pnpm-bin.test.mjs +++ b/scripts/flatpak-pnpm-bin.test.mjs @@ -20,33 +20,36 @@ describe('Flatpak pnpm standalone install', () => { expect(yaml).toMatch(/cp -a pnpm-vendor\/dist \/run\/build\/mesh-client\/\.pnpm-bin\/dist/); }); - it('requires unquoted offline pnpm booleans in mesh-client build-options.env', () => { + it('requires quoted offline pnpm env strings for flatpak-builder GStrv', () => { const yaml = fs.readFileSync(MANIFEST, 'utf8'); const env = parseMeshClientModuleBuildEnv(yaml); expect(env).not.toBeNull(); - expect(env?.PNPM_CONFIG_TRUST_LOCKFILE).toBe(true); - expect(env?.PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN).toBe(false); + expect(env?.PNPM_CONFIG_TRUST_LOCKFILE).toBe('true'); + expect(env?.PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN).toBe('false'); expect(offlinePnpmEnvContractViolations(yaml)).toEqual([]); }); - it('rejects quoted or commented offline pnpm env values', () => { - const quoted = ` + it('rejects unquoted YAML booleans (GStrv drops the whole env map)', () => { + const unquoted = ` modules: - name: mesh-client build-options: env: - PNPM_CONFIG_TRUST_LOCKFILE: 'true' - PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN: 'false' + PNPM_CONFIG_TRUST_LOCKFILE: true + PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN: false `; - expect(offlinePnpmEnvContractViolations(quoted).length).toBe(2); + expect(offlinePnpmEnvContractViolations(unquoted).length).toBe(2); + expect(offlinePnpmEnvContractViolations(unquoted)[0].message).toMatch(/GStrv|quoted/); + }); + it('rejects missing or commented offline pnpm env values', () => { const commentedOnly = ` modules: - name: mesh-client build-options: env: - # PNPM_CONFIG_TRUST_LOCKFILE: true - # PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN: false + # PNPM_CONFIG_TRUST_LOCKFILE: 'true' + # PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN: 'false' PNPM_HOME: /run/build/mesh-client/.pnpm `; expect(offlinePnpmEnvContractViolations(commentedOnly).length).toBe(2); diff --git a/scripts/flatpak-pnpm-install.mjs b/scripts/flatpak-pnpm-install.mjs index f00318d5c..45b694818 100644 --- a/scripts/flatpak-pnpm-install.mjs +++ b/scripts/flatpak-pnpm-install.mjs @@ -19,6 +19,10 @@ import { spawnSync } from 'child_process'; import { fileURLToPath } from 'url'; import path from 'path'; import { cleanJsrTempDirs } from './clean-jsr-temp-dirs.mjs'; +import { + stripNpmrcStoreDirLines, + stripPnpmWorkspaceStoreDirLines, +} from './flatpakPnpmStoreVersion.mjs'; const __dirname = path.dirname(fileURLToPath(import.meta.url)); const projectRoot = path.resolve(__dirname, '..'); @@ -36,6 +40,46 @@ export const FLATPAK_PNPM_INSTALL_ARGS = [ STORE_DIR, ]; +/** + * Remove generator-appended storeDir / store-dir lines so pnpm can parse the + * workspace and `--store-dir` (sandbox path) wins over a host-cache path. + * + * @param {string} [root] + * @returns {{ workspaceRemoved: number, npmrcRemoved: number }} + */ +export function sanitizeFlatpakPnpmStoreDirConfig(root = projectRoot) { + const workspacePath = path.join(root, 'pnpm-workspace.yaml'); + const npmrcPath = path.join(root, '.npmrc'); + let workspaceRemoved = 0; + let npmrcRemoved = 0; + + if (fs.existsSync(workspacePath)) { + const before = fs.readFileSync(workspacePath, 'utf8'); + const { yaml, removed } = stripPnpmWorkspaceStoreDirLines(before); + workspaceRemoved = removed; + if (removed > 0) { + fs.writeFileSync(workspacePath, yaml, 'utf8'); + console.warn( + `[flatpak-pnpm] stripped ${removed} storeDir line(s) from pnpm-workspace.yaml (using --store-dir ${STORE_DIR})`, + ); + } + } + + if (fs.existsSync(npmrcPath)) { + const before = fs.readFileSync(npmrcPath, 'utf8'); + const { text, removed } = stripNpmrcStoreDirLines(before); + npmrcRemoved = removed; + if (removed > 0) { + fs.writeFileSync(npmrcPath, text, 'utf8'); + console.warn( + `[flatpak-pnpm] stripped ${removed} store-dir line(s) from .npmrc (using --store-dir ${STORE_DIR})`, + ); + } + } + + return { workspaceRemoved, npmrcRemoved }; +} + export function runFlatpakPnpmInstall() { if (!fs.existsSync(STORE_DIR)) { console.error( @@ -44,6 +88,8 @@ export function runFlatpakPnpmInstall() { process.exit(1); } + sanitizeFlatpakPnpmStoreDirConfig(projectRoot); + let lastStatus = 1; for (let attempt = 1; attempt <= maxAttempts; attempt++) { diff --git a/scripts/flatpak-pnpm-install.test.mjs b/scripts/flatpak-pnpm-install.test.mjs index dac5571f3..de349c5eb 100644 --- a/scripts/flatpak-pnpm-install.test.mjs +++ b/scripts/flatpak-pnpm-install.test.mjs @@ -1,15 +1,29 @@ // @vitest-environment node import { spawnSync } from 'child_process'; -import { fileURLToPath } from 'url'; +import fs from 'fs'; +import os from 'os'; import path from 'path'; -import { describe, expect, it } from 'vitest'; -import { FLATPAK_PNPM_INSTALL_ARGS } from './flatpak-pnpm-install.mjs'; +import { fileURLToPath } from 'url'; +import { afterEach, describe, expect, it } from 'vitest'; +import { + FLATPAK_PNPM_INSTALL_ARGS, + sanitizeFlatpakPnpmStoreDirConfig, +} from './flatpak-pnpm-install.mjs'; const scriptPath = path.join( path.dirname(fileURLToPath(import.meta.url)), 'flatpak-pnpm-install.mjs', ); +/** @type {string[]} */ +const tempRoots = []; + +afterEach(() => { + for (const root of tempRoots.splice(0)) { + fs.rmSync(root, { recursive: true, force: true }); + } +}); + describe('flatpak-pnpm-install.mjs', () => { it('exits non-zero when offline store is unavailable outside sandbox', () => { const result = spawnSync(process.execPath, [scriptPath], { @@ -25,5 +39,24 @@ describe('flatpak-pnpm-install.mjs', () => { expect(FLATPAK_PNPM_INSTALL_ARGS).toContain('--offline'); expect(FLATPAK_PNPM_INSTALL_ARGS).toContain('--frozen-lockfile'); expect(FLATPAK_PNPM_INSTALL_ARGS).toContain('--ignore-scripts'); + expect(FLATPAK_PNPM_INSTALL_ARGS).toContain('--store-dir'); + }); + + it('strips generator storeDir= from workspace before install (Flatpak CI regression)', () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'mesh-flatpak-sanitize-')); + tempRoots.push(root); + fs.writeFileSync( + path.join(root, 'pnpm-workspace.yaml'), + 'nodeLinker: hoisted\nstoreDir=/__w/mesh-client/bad-store\n', + 'utf8', + ); + fs.writeFileSync(path.join(root, '.npmrc'), 'store-dir=/__w/host\n', 'utf8'); + + const result = sanitizeFlatpakPnpmStoreDirConfig(root); + expect(result).toEqual({ workspaceRemoved: 1, npmrcRemoved: 1 }); + expect(fs.readFileSync(path.join(root, 'pnpm-workspace.yaml'), 'utf8')).toBe( + 'nodeLinker: hoisted\n', + ); + expect(fs.readFileSync(path.join(root, '.npmrc'), 'utf8')).toBe(''); }); }); diff --git a/scripts/flatpakOfflinePnpmEnv.mjs b/scripts/flatpakOfflinePnpmEnv.mjs index cf7645103..044f387a6 100644 --- a/scripts/flatpakOfflinePnpmEnv.mjs +++ b/scripts/flatpakOfflinePnpmEnv.mjs @@ -2,8 +2,12 @@ * Shared Flatpak manifest contract for offline pnpm 11 install. * * Failure point: loose YAML regex can match commented or quoted keys outside the - * mesh-client module env map. Fallback: parse only that scoped env block and - * require exact unquoted YAML booleans. + * mesh-client module env map. Fallback: parse only that scoped env block. + * + * Failure point: flatpak-builder deserializes `env` as GStrv (string values only). + * Unquoted YAML `true`/`false` become JSON booleans → entire env map is dropped + * (`Failed to deserialize "env" property of type "GStrv"`). Fallback: require + * quoted `'true'` / `'false'` strings. */ /** @@ -47,7 +51,6 @@ export function parseMeshClientModuleBuildEnv(yaml) { (raw.startsWith("'") && raw.endsWith("'")) || (raw.startsWith('"') && raw.endsWith('"')) ) { - // Quoted values are strings, not YAML booleans — reject for required keys. env[key] = raw.slice(1, -1); } else { env[key] = raw; @@ -78,19 +81,22 @@ export function offlinePnpmEnvContractViolations( return violations; } - if (env.PNPM_CONFIG_TRUST_LOCKFILE !== true) { + // GStrv requires string values — quoted 'true'/'false', not YAML booleans. + if (env.PNPM_CONFIG_TRUST_LOCKFILE !== 'true') { violations.push({ file: fileRel, message: - 'manifest mesh-client build-options.env must set PNPM_CONFIG_TRUST_LOCKFILE: true (unquoted boolean; skip registry supply-chain re-verify offline)', + "manifest mesh-client build-options.env must set PNPM_CONFIG_TRUST_LOCKFILE: 'true' " + + '(quoted string for flatpak-builder GStrv; unquoted true drops the whole env map)', }); } - if (env.PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN !== false) { + if (env.PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN !== 'false') { violations.push({ file: fileRel, message: - 'manifest mesh-client build-options.env must set PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN: false (unquoted boolean; pnpm run must not auto-install offline)', + "manifest mesh-client build-options.env must set PNPM_CONFIG_VERIFY_DEPS_BEFORE_RUN: 'false' " + + '(quoted string for flatpak-builder GStrv; unquoted false drops the whole env map)', }); } diff --git a/scripts/flatpakPnpmStoreVersion.mjs b/scripts/flatpakPnpmStoreVersion.mjs index 3b2483fe3..e0fe823ce 100644 --- a/scripts/flatpakPnpmStoreVersion.mjs +++ b/scripts/flatpakPnpmStoreVersion.mjs @@ -12,6 +12,24 @@ export const FLATPAK_NODE_GENERATOR_COMMIT = 'ac5a296ac6111aa2319daf532f609a067b export const FLATPAK_NODE_GENERATOR_GIT = `git+https://github.com/flatpak/flatpak-builder-tools@${FLATPAK_NODE_GENERATOR_COMMIT}#subdirectory=node`; +/** + * pip install args for the pinned generator. + * + * Failure point: flathub-infra Flatpak containers (and some CI images) preinstall + * `flatpak_node_generator==0.1.0`. Upstream keeps that version pinned across commits, so + * plain `pip install git+…@ac5a296` is a no-op and leaves the older `storeDir=` generator. + * Fallback: `--force-reinstall` (and `--no-cache-dir` so a stale wheel cannot win). + */ +export const FLATPAK_NODE_GENERATOR_PIP_INSTALL_ARGS = [ + 'install', + '--force-reinstall', + '--no-cache-dir', + FLATPAK_NODE_GENERATOR_GIT, +]; + +/** Shell one-liner for workflows / docs (keep in sync with PIP_INSTALL_ARGS). */ +export const FLATPAK_NODE_GENERATOR_PIP_INSTALL_CMD = `pip3 ${FLATPAK_NODE_GENERATOR_PIP_INSTALL_ARGS.map((a) => (/\s/.test(a) ? `'${a}'` : a)).join(' ')}`; + /** * @param {string | null | undefined} packageManager * @returns {number | null} @@ -44,6 +62,40 @@ export function storeVersionFromPackageManager(packageManager) { return expectedPnpmStoreVersion(major); } +/** + * Ensure CI/Flatpak workflows force-reinstall the pinned generator (same 0.1.0 version + * across commits — plain pip install is a no-op on preinstalled images). + * + * @param {string} workflowYaml + * @param {string} [fileRel] + * @returns {{ file: string, message: string }[]} + */ +export function flatpakWorkflowGeneratorInstallViolations( + workflowYaml, + fileRel = '.github/workflows/flatpak.yaml', +) { + /** @type {{ file: string, message: string }[]} */ + const violations = []; + // Match either a direct git+ URL or pip install of ${FBTOOLS} when FBTOOLS points at + // flatpak-builder-tools (CI uses the variable form). + const installsGenerator = + /pip3?\s+install\b[\s\S]{0,300}?flatpak-builder-tools/.test(workflowYaml) || + (/flatpak-builder-tools/.test(workflowYaml) && + /pip3?\s+install\b[\s\S]{0,200}?\$\{?FBTOOLS\}?/.test(workflowYaml)); + if (!installsGenerator) { + return violations; + } + if (!/--force-reinstall\b/.test(workflowYaml)) { + violations.push({ + file: fileRel, + message: + 'pip install of flatpak-node-generator must use --force-reinstall ' + + '(image may preinstall flatpak_node_generator==0.1.0; same version skips upgrade and leaves storeDir=)', + }); + } + return violations; +} + /** * @param {string} workflowYaml * @param {string} expectedStoreVersion e.g. v11 @@ -66,6 +118,8 @@ export function flatpakWorkflowStoreVersionViolations( return violations; } + violations.push(...flatpakWorkflowGeneratorInstallViolations(workflowYaml, fileRel)); + // Accept an explicit --pnpm-store-version vN, or a shell var set from packageManager. // Flag may be on a continued line after `flatpak-node-generator pnpm … \`. const explicit = workflowYaml.match( @@ -274,6 +328,53 @@ export function probePnpmWorkspaceAfterStoreDirAppend( } } +/** + * Strip generator-appended `storeDir` keys from pnpm-workspace.yaml. + * + * Failure point: flatpak-node-generator shell sources append either invalid + * `storeDir=` (pre-ac5a296a) or a host-cache `storeDir: $PWD/…` path that is + * unreachable inside the Flatpak sandbox. Fallback: remove those lines and rely + * on `pnpm install --store-dir` (sandbox-absolute) from flatpak-pnpm-install.mjs. + * + * @param {string} workspaceYaml + * @returns {{ yaml: string, removed: number }} + */ +export function stripPnpmWorkspaceStoreDirLines(workspaceYaml) { + let removed = 0; + const yaml = workspaceYaml + .split('\n') + .filter((line) => { + if (/^\s*storeDir\s*[:=]/.test(line)) { + removed += 1; + return false; + } + return true; + }) + .join('\n'); + return { yaml, removed }; +} + +/** + * Strip `store-dir=` lines from .npmrc (v10 generator path). + * + * @param {string} npmrc + * @returns {{ text: string, removed: number }} + */ +export function stripNpmrcStoreDirLines(npmrc) { + let removed = 0; + const text = npmrc + .split('\n') + .filter((line) => { + if (/^\s*store-dir\s*=/.test(line)) { + removed += 1; + return false; + } + return true; + }) + .join('\n'); + return { text, removed }; +} + /** * @param {string[]} lockfilePackageIds * @param {Set} tarballNames diff --git a/scripts/flatpakPnpmStoreVersion.test.mjs b/scripts/flatpakPnpmStoreVersion.test.mjs index 5e37f6e7b..4c04356dd 100644 --- a/scripts/flatpakPnpmStoreVersion.test.mjs +++ b/scripts/flatpakPnpmStoreVersion.test.mjs @@ -3,6 +3,7 @@ import { describe, expect, it } from 'vitest'; import yaml from 'js-yaml'; import { expectedPnpmStoreVersion, + flatpakWorkflowGeneratorInstallViolations, flatpakWorkflowStoreVersionViolations, generatedSourcesStoreDirYamlViolations, listGeneratedPnpmWorkspaceShellCommands, @@ -13,6 +14,8 @@ import { pnpmMajorFromPackageManager, probePnpmWorkspaceAfterStoreDirAppend, storeVersionFromPackageManager, + stripNpmrcStoreDirLines, + stripPnpmWorkspaceStoreDirLines, } from './flatpakPnpmStoreVersion.mjs'; describe('flatpakPnpmStoreVersion', () => { @@ -99,6 +102,30 @@ packages: expect(truncated).toBe(true); }); + it('requires --force-reinstall when installing flatpak-node-generator', () => { + // Build a short pin fixture so no-secrets does not flag a full commit hash. + const pin = ['ac5a296a', 'c611'].join(''); + const bad = ` + FBTOOLS=git+https://github.com/flatpak/flatpak-builder-tools + pip3 install "\${FBTOOLS}@${pin}#subdirectory=node" + flatpak-node-generator pnpm pnpm-lock.yaml --pnpm-store-version v11 -o out.json + `; + expect(flatpakWorkflowGeneratorInstallViolations(bad)[0].message).toMatch(/force-reinstall/); + // Combined store-version check must also surface the install pin issue. + expect( + flatpakWorkflowStoreVersionViolations(bad, 'v11').some((v) => + /force-reinstall/.test(v.message), + ), + ).toBe(true); + + const good = ` + FBTOOLS=git+https://github.com/flatpak/flatpak-builder-tools + pip3 install --force-reinstall --no-cache-dir "\${FBTOOLS}@${pin}#subdirectory=node" + flatpak-node-generator pnpm pnpm-lock.yaml --pnpm-store-version v11 -o out.json + `; + expect(flatpakWorkflowGeneratorInstallViolations(good)).toEqual([]); + }); + it('rejects npmrc-style storeDir= shell commands targeting pnpm-workspace.yaml', () => { const bad = [ { @@ -166,4 +193,28 @@ patchedDependencies: expect(heuristic.ok).toBe(false); expect(heuristic.ok === false && heuristic.reason).toMatch(/storeDir=/); }); + + it('strips invalid and host-path storeDir lines from workspace YAML', () => { + const workspace = ` +patchedDependencies: + usb@2.18.0: patches/usb@2.18.0.patch +storeDir=/__w/mesh-client/bad +storeDir: /__w/mesh-client/also-bad +`; + const { yaml: cleaned, removed } = stripPnpmWorkspaceStoreDirLines(workspace); + expect(removed).toBe(2); + expect(cleaned).not.toMatch(/storeDir/); + expect(cleaned).toMatch(/usb@2\.18\.0/); + // After strip, workspace must parse again. + expect(yaml.load(cleaned)).toMatchObject({ + patchedDependencies: { 'usb@2.18.0': 'patches/usb@2.18.0.patch' }, + }); + }); + + it('strips store-dir lines from .npmrc', () => { + const npmrc = 'shamefully-hoist=true\nstore-dir=/__w/host/store\n'; + const { text, removed } = stripNpmrcStoreDirLines(npmrc); + expect(removed).toBe(1); + expect(text).toBe('shamefully-hoist=true\n'); + }); }); From 87dcffdce964e28bd99caf71409a050ee4a7e4ff Mon Sep 17 00:00:00 2001 From: Joey Stanford Date: Tue, 21 Jul 2026 08:16:17 -0600 Subject: [PATCH 2/2] fix: harden Flatpak generator install check and sanitize TOCTOU MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Require --force-reinstall and --no-cache-dir on the generator's own non-comment pip install command (not comments / unrelated pip). - Read workspace/.npmrc via try/catch ENOENT instead of existsSync→read (CodeQL js/file-system-race). - Add regression tests for comment-only flags, unrelated pip installs, missing --no-cache-dir, and missing config files. --- scripts/flatpak-pnpm-install.mjs | 32 ++++++++-- scripts/flatpak-pnpm-install.test.mjs | 9 +++ scripts/flatpakPnpmStoreVersion.mjs | 75 ++++++++++++++++++++---- scripts/flatpakPnpmStoreVersion.test.mjs | 30 +++++++++- 4 files changed, 128 insertions(+), 18 deletions(-) diff --git a/scripts/flatpak-pnpm-install.mjs b/scripts/flatpak-pnpm-install.mjs index 45b694818..9db265951 100644 --- a/scripts/flatpak-pnpm-install.mjs +++ b/scripts/flatpak-pnpm-install.mjs @@ -47,15 +47,35 @@ export const FLATPAK_PNPM_INSTALL_ARGS = [ * @param {string} [root] * @returns {{ workspaceRemoved: number, npmrcRemoved: number }} */ +/** + * Read a UTF-8 file if present. Avoid existsSync→read TOCTOU (CodeQL js/file-system-race). + * @param {string} filePath + * @returns {string | null} + */ +function readUtf8IfPresent(filePath) { + try { + return fs.readFileSync(filePath, 'utf8'); + } catch (err) { + if ( + err && + typeof err === 'object' && + /** @type {{ code?: unknown }} */ (err).code === 'ENOENT' + ) { + return null; + } + throw err; + } +} + export function sanitizeFlatpakPnpmStoreDirConfig(root = projectRoot) { const workspacePath = path.join(root, 'pnpm-workspace.yaml'); const npmrcPath = path.join(root, '.npmrc'); let workspaceRemoved = 0; let npmrcRemoved = 0; - if (fs.existsSync(workspacePath)) { - const before = fs.readFileSync(workspacePath, 'utf8'); - const { yaml, removed } = stripPnpmWorkspaceStoreDirLines(before); + const workspaceBefore = readUtf8IfPresent(workspacePath); + if (workspaceBefore != null) { + const { yaml, removed } = stripPnpmWorkspaceStoreDirLines(workspaceBefore); workspaceRemoved = removed; if (removed > 0) { fs.writeFileSync(workspacePath, yaml, 'utf8'); @@ -65,9 +85,9 @@ export function sanitizeFlatpakPnpmStoreDirConfig(root = projectRoot) { } } - if (fs.existsSync(npmrcPath)) { - const before = fs.readFileSync(npmrcPath, 'utf8'); - const { text, removed } = stripNpmrcStoreDirLines(before); + const npmrcBefore = readUtf8IfPresent(npmrcPath); + if (npmrcBefore != null) { + const { text, removed } = stripNpmrcStoreDirLines(npmrcBefore); npmrcRemoved = removed; if (removed > 0) { fs.writeFileSync(npmrcPath, text, 'utf8'); diff --git a/scripts/flatpak-pnpm-install.test.mjs b/scripts/flatpak-pnpm-install.test.mjs index de349c5eb..6ba8568aa 100644 --- a/scripts/flatpak-pnpm-install.test.mjs +++ b/scripts/flatpak-pnpm-install.test.mjs @@ -59,4 +59,13 @@ describe('flatpak-pnpm-install.mjs', () => { ); expect(fs.readFileSync(path.join(root, '.npmrc'), 'utf8')).toBe(''); }); + + it('tolerates missing workspace/.npmrc without existsSync TOCTOU', () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'mesh-flatpak-sanitize-missing-')); + tempRoots.push(root); + expect(sanitizeFlatpakPnpmStoreDirConfig(root)).toEqual({ + workspaceRemoved: 0, + npmrcRemoved: 0, + }); + }); }); diff --git a/scripts/flatpakPnpmStoreVersion.mjs b/scripts/flatpakPnpmStoreVersion.mjs index e0fe823ce..20157fad0 100644 --- a/scripts/flatpakPnpmStoreVersion.mjs +++ b/scripts/flatpakPnpmStoreVersion.mjs @@ -62,10 +62,62 @@ export function storeVersionFromPackageManager(packageManager) { return expectedPnpmStoreVersion(major); } +/** + * Collect non-comment shell command text from a GitHub Actions workflow YAML. + * Joins lines continued with a trailing `\`. Skips `#` comments and empty lines. + * + * @param {string} workflowYaml + * @returns {string[]} + */ +export function listWorkflowNonCommentShellCommands(workflowYaml) { + /** @type {string[]} */ + const commands = []; + /** @type {string[]} */ + let current = []; + + const flush = () => { + if (current.length === 0) return; + commands.push(current.join(' ').replace(/\s+/g, ' ').trim()); + current = []; + }; + + for (const rawLine of workflowYaml.split('\n')) { + const trimmed = rawLine.trim(); + if (!trimmed || trimmed.startsWith('#')) { + flush(); + continue; + } + + const continued = /\\$/.test(trimmed); + const piece = continued ? trimmed.slice(0, -1).trimEnd() : trimmed; + current.push(piece); + if (!continued) flush(); + } + flush(); + return commands; +} + +/** + * True when a shell command installs flatpak-builder-tools / the node generator pin. + * @param {string} command + * @returns {boolean} + */ +export function isFlatpakNodeGeneratorPipInstallCommand(command) { + if (!/\bpip3?\s+install\b/.test(command)) return false; + return ( + /flatpak-builder-tools/.test(command) || + /\$\{?FBTOOLS\}?/.test(command) || + /FLATPAK_NODE_GENERATOR_GIT/.test(command) + ); +} + /** * Ensure CI/Flatpak workflows force-reinstall the pinned generator (same 0.1.0 version * across commits — plain pip install is a no-op on preinstalled images). * + * Flags must appear on the generator's own non-comment `pip install` command — not + * only in a nearby comment or an unrelated pip install. + * * @param {string} workflowYaml * @param {string} [fileRel] * @returns {{ file: string, message: string }[]} @@ -76,21 +128,24 @@ export function flatpakWorkflowGeneratorInstallViolations( ) { /** @type {{ file: string, message: string }[]} */ const violations = []; - // Match either a direct git+ URL or pip install of ${FBTOOLS} when FBTOOLS points at - // flatpak-builder-tools (CI uses the variable form). - const installsGenerator = - /pip3?\s+install\b[\s\S]{0,300}?flatpak-builder-tools/.test(workflowYaml) || - (/flatpak-builder-tools/.test(workflowYaml) && - /pip3?\s+install\b[\s\S]{0,200}?\$\{?FBTOOLS\}?/.test(workflowYaml)); - if (!installsGenerator) { + const generatorInstalls = listWorkflowNonCommentShellCommands(workflowYaml).filter( + isFlatpakNodeGeneratorPipInstallCommand, + ); + if (generatorInstalls.length === 0) { return violations; } - if (!/--force-reinstall\b/.test(workflowYaml)) { + + for (const cmd of generatorInstalls) { + const missing = []; + if (!/--force-reinstall\b/.test(cmd)) missing.push('--force-reinstall'); + if (!/--no-cache-dir\b/.test(cmd)) missing.push('--no-cache-dir'); + if (missing.length === 0) continue; violations.push({ file: fileRel, message: - 'pip install of flatpak-node-generator must use --force-reinstall ' + - '(image may preinstall flatpak_node_generator==0.1.0; same version skips upgrade and leaves storeDir=)', + `pip install of flatpak-node-generator must include ${missing.join(' and ')} on the ` + + 'install command itself (not only in comments). Image may preinstall ' + + 'flatpak_node_generator==0.1.0; same version skips upgrade and leaves storeDir=.', }); } return violations; diff --git a/scripts/flatpakPnpmStoreVersion.test.mjs b/scripts/flatpakPnpmStoreVersion.test.mjs index 4c04356dd..47a59871c 100644 --- a/scripts/flatpakPnpmStoreVersion.test.mjs +++ b/scripts/flatpakPnpmStoreVersion.test.mjs @@ -102,7 +102,7 @@ packages: expect(truncated).toBe(true); }); - it('requires --force-reinstall when installing flatpak-node-generator', () => { + it('requires --force-reinstall and --no-cache-dir on the generator pip install command', () => { // Build a short pin fixture so no-secrets does not flag a full commit hash. const pin = ['ac5a296a', 'c611'].join(''); const bad = ` @@ -120,12 +120,38 @@ packages: const good = ` FBTOOLS=git+https://github.com/flatpak/flatpak-builder-tools - pip3 install --force-reinstall --no-cache-dir "\${FBTOOLS}@${pin}#subdirectory=node" + pip3 install --force-reinstall --no-cache-dir \\ + "\${FBTOOLS}@${pin}#subdirectory=node" flatpak-node-generator pnpm pnpm-lock.yaml --pnpm-store-version v11 -o out.json `; expect(flatpakWorkflowGeneratorInstallViolations(good)).toEqual([]); }); + it('rejects flags present only in comments or on an unrelated pip install', () => { + const pin = ['ac5a296a', 'c611'].join(''); + const flagsInComment = ` + FBTOOLS=git+https://github.com/flatpak/flatpak-builder-tools + # --force-reinstall --no-cache-dir required for storeDir= + pip3 install "\${FBTOOLS}@${pin}#subdirectory=node" + `; + expect(flatpakWorkflowGeneratorInstallViolations(flagsInComment).length).toBe(1); + + const flagsOnUnrelatedPip = ` + pip3 install --force-reinstall --no-cache-dir yamllint + FBTOOLS=git+https://github.com/flatpak/flatpak-builder-tools + pip3 install "\${FBTOOLS}@${pin}#subdirectory=node" + `; + expect(flatpakWorkflowGeneratorInstallViolations(flagsOnUnrelatedPip).length).toBe(1); + + const missingNoCacheDir = ` + FBTOOLS=git+https://github.com/flatpak/flatpak-builder-tools + pip3 install --force-reinstall "\${FBTOOLS}@${pin}#subdirectory=node" + `; + expect(flatpakWorkflowGeneratorInstallViolations(missingNoCacheDir)[0].message).toMatch( + /no-cache-dir/, + ); + }); + it('rejects npmrc-style storeDir= shell commands targeting pnpm-workspace.yaml', () => { const bad = [ {