From 27fbc4a0e6434724a681acf1f8f57f5d7afc52dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marcone=20Ten=C3=B3rio=20da=20Silva=20Filho?= Date: Mon, 10 Aug 2026 13:13:39 +0200 Subject: [PATCH] fix(package-manager): gate the build on VPP package integrity (DOPE-539) Signature verification ran at import and at project open, and neither says anything about the package as it exists when a build starts: userData/packages// is plain user-writable disk and the compiler reads it fresh every compile. The window was "project open -> click Compile", entirely user-controlled. What that window is worth: hal.source is C++ linked into the firmware, hal.pluginEntry is C the runtime compiles ON a live PLC, hal.licenseStore is the on-device licence backend, and because capabilities.isLicensable is a manifest field, editing the installed manifest switches the whole licensing flow off - no licence FCs on connect, no activation call, weak license_* defaults linked in. Add PackageManagerModule.verifyBoardPackageIntegrity(boardName): resolves the VPP behind the board, re-runs verifyPackageSignature, reports the package id and reason on failure. No-op for built-in hals.json boards and when REQUIRE_SIGNATURE is false. Called from compileProgram (before any package file is read), compileForDebugger, and again from handleVendorPluginPackaging - that step runs minutes later in wall-clock terms and is what copies vendor code into the PLC bundle, so it re-checks rather than trusting compile entry. There the gate sits outside the catch-all and throws, because packageVppPlugin turns a throw into the errors[] the pipeline bails on; a logged error would upload a bundle with no vendor I/O. Refuses the compile rather than de-listing the package: tearing a directory out from under a build in flight is a worse failure than stopping and saying why. The project-open sweep keeps ownership of removal. This shortens the window to sub-second, it does not close it - the gate hashes the directory and the pipeline reads it again. Verifying the bytes that actually enter the build is DOPE-558, which touches the shared verify-package-signature.ts and so needs a mirror PR on openplc-web. Co-Authored-By: Claude Opus 5 --- .../handle-vendor-plugin-packaging.test.ts | 35 +++ .../editor/compiler/compiler-module.ts | 51 +++- .../verify-board-package-integrity.test.ts | 276 ++++++++++++++++++ src/backend/editor/package-manager/index.ts | 4 +- .../package-manager/package-manager-module.ts | 56 +++- src/backend/editor/package-manager/types.ts | 19 +- 6 files changed, 435 insertions(+), 6 deletions(-) create mode 100644 src/backend/editor/package-manager/__tests__/verify-board-package-integrity.test.ts diff --git a/src/backend/editor/compiler/__tests__/handle-vendor-plugin-packaging.test.ts b/src/backend/editor/compiler/__tests__/handle-vendor-plugin-packaging.test.ts index e44e95fc0..9f78de947 100644 --- a/src/backend/editor/compiler/__tests__/handle-vendor-plugin-packaging.test.ts +++ b/src/backend/editor/compiler/__tests__/handle-vendor-plugin-packaging.test.ts @@ -32,11 +32,20 @@ type FindVppDevice = typeof import('../../../shared/hardware/find-vpp-device') const listInstalled = jest.fn() const getInstalledPackageManifest = jest.fn() +// The build-time integrity gate (DOPE-539). Defaults to "intact" so the +// provisioning tests below exercise the packaging behaviour they are about; the +// refusal case overrides it explicitly. +const verifyBoardPackageIntegrity = jest.fn<{ ok: boolean; packageId?: string; reason?: string }, [string]>(() => ({ + ok: true, +})) jest.mock('../../package-manager', () => ({ + formatPackageIntegrityError: (boardName: string, failure: { packageId: string; reason: string }) => + `Board "${boardName}" is provided by the VPP package "${failure.packageId}", which no longer matches its signature: ${failure.reason}.`, PackageManagerModule: jest.fn().mockImplementation(() => { const port = { listInstalled, getInstalledPackageManifest } return { ...port, + verifyBoardPackageIntegrity, // Board lookup runs through the shared `findVppDeviceByBoardName`, and // the mock runs the real one over these two stubs rather than // re-implementing the search — a stub that resolved boards its own way @@ -159,6 +168,32 @@ describe('handleVendorPluginPackaging — provisioning branch', () => { expect(logs.some((l) => /source file\(s\)/.test(l.message))).toBe(true) }) + it('refuses to copy the payload when the package no longer matches its signature', async () => { + // DOPE-539: this step is re-gated because it runs minutes after the + // compile-entry check, and what it copies is compiled on the live PLC. + // A throw is the contract — `packageVppPlugin` in the platform port turns + // it into the `errors[]` the pipeline bails on, whereas a logged error + // would let the build upload a bundle with no vendor I/O. + verifyBoardPackageIntegrity.mockReturnValueOnce({ + ok: false, + packageId: 'com.openplc.rpi', + reason: 'Tampered file detected: hal/runtime-v4/plugin/rpi_plugin.o', + }) + + await expect( + runFor({ + type: 'runtime-v4-plugin', + pluginType: 'native', + provisioning: 'prebuilt', + pluginEntry: 'hal/runtime-v4/plugin', + configTemplate: 'hal/runtime-v4/plugin/config_template.json', + }), + ).rejects.toThrow(/com\.openplc\.rpi/) + + expect(existsSync(join(targetDir, 'vpp_plugin'))).toBe(false) + expect(logs.some((l) => l.level === 'error' && /rpi_plugin\.o/.test(l.message))).toBe(true) + }) + it('copies the payload byte-for-byte (prebuilt object content preserved)', async () => { await runFor({ type: 'runtime-v4-plugin', diff --git a/src/backend/editor/compiler/compiler-module.ts b/src/backend/editor/compiler/compiler-module.ts index 8ca30ffc5..42d0c97b3 100644 --- a/src/backend/editor/compiler/compiler-module.ts +++ b/src/backend/editor/compiler/compiler-module.ts @@ -120,7 +120,7 @@ import JSZip from 'jszip' import type { PlatformOption } from '../../../middleware/shared/ports/types' import { BoardInfoResolver } from '../../shared/hardware/board-info-resolver' -import { PackageManagerModule } from '../package-manager' +import { formatPackageIntegrityError, PackageManagerModule } from '../package-manager' import { CreateXMLFile } from '../utils' import { createDesktopLibraryBuildPort } from './desktop-library-build-port' import { createEditorCompilerPlatformPort } from './editor-compiler-platform-port' @@ -2010,6 +2010,24 @@ class CompilerModule { sourceTargetFolderPath: string, handleOutputData: HandleOutputDataCallback, ): Promise { + // Second gate, deliberately re-run here rather than trusted from + // `compileProgram` (DOPE-539). This step is what copies vendor C into the + // bundle the runtime compiles ON the PLC, and it runs late — transpile, + // strucpp and the v4 bundle compose happen in between, which on a large + // project is minutes of wall clock during which the package directory is + // still writable. Checking again costs one directory hash. + // + // This sits OUTSIDE the catch-all below on purpose. Every other failure in + // this method degrades the build and reports it; this one has to stop it, + // and `packageVppPlugin` in the platform port turns a throw into the + // `errors[]` the pipeline bails on. + const integrity = new PackageManagerModule().verifyBoardPackageIntegrity(boardTarget) + if (!integrity.ok) { + const message = formatPackageIntegrityError(boardTarget, integrity) + handleOutputData(message, 'error') + throw new Error(message) + } + try { const match = new PackageManagerModule().findDeviceByBoardName(boardTarget) @@ -2370,6 +2388,22 @@ class CompilerModule { Record | undefined, ] + // VPP integrity gate (DOPE-539). FIRST, before the manifest is read for + // anything else: from here on this method trusts the package directory for + // the HAL it links, the licence-store backend it injects and every + // capability it branches on. The import check and the project-open sweep + // are both behind us and neither says anything about the package as it + // exists right now. + const boardPackageIntegrity = new PackageManagerModule().verifyBoardPackageIntegrity(boardTarget) + if (!boardPackageIntegrity.ok) { + _mainProcessPort.postMessage({ + logLevel: 'error', + message: `${formatPackageIntegrityError(boardTarget, boardPackageIntegrity)}\nStopping compilation process.`, + }) + _mainProcessPort.close() + return + } + // Resolve board info uniformly across hals.json + installed VPP // packages via the shared `resolveBoardSelection` helper — the // same code path runs on web (no VPP packages installed → falls @@ -2829,6 +2863,21 @@ class CompilerModule { const [projectPath, boardTarget, projectData] = args as [string, string, PLCProjectData] + // Same gate as `compileProgram` (DOPE-539). The debug build is a smaller + // consumer of the package — it resolves the board and its debug spec — but + // it is still a build the user runs against a live device, and letting it + // through on a package the normal compile just refused would only teach + // that the check is avoidable. + const debugPackageIntegrity = new PackageManagerModule().verifyBoardPackageIntegrity(boardTarget) + if (!debugPackageIntegrity.ok) { + _mainProcessPort.postMessage({ + logLevel: 'error', + message: `${formatPackageIntegrityError(boardTarget, debugPackageIntegrity)}\nStopping debug compilation process.`, + }) + _mainProcessPort.close() + return + } + const debugResolver = await this.#createBoardInfoResolver() const { boardRuntime } = debugResolver.resolve(boardTarget) const normalizedProjectPath = projectPath.replace('project.json', '') diff --git a/src/backend/editor/package-manager/__tests__/verify-board-package-integrity.test.ts b/src/backend/editor/package-manager/__tests__/verify-board-package-integrity.test.ts new file mode 100644 index 000000000..94a555a0f --- /dev/null +++ b/src/backend/editor/package-manager/__tests__/verify-board-package-integrity.test.ts @@ -0,0 +1,276 @@ +import { createHash, sign as cryptoSign } from 'node:crypto' +import { existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { dirname, join } from 'node:path' + +import { app } from 'electron' + +import { canonicalize, SIGNATURE_FILENAME } from '../../../shared/utils/vpp/verify-package-signature' + +// Same transitive-dependency stubs as the sweep suite: winston file transports +// and extract-zip's ESM entry point are irrelevant to a signature check. +jest.mock('electron', () => ({ app: { getPath: jest.fn(() => '/mock/path') } })) +jest.mock('extract-zip', () => ({ __esModule: true, default: jest.fn() })) +jest.mock('../../services/logger-service', () => ({ + logger: { warn: jest.fn(), info: jest.fn(), error: jest.fn() }, +})) + +// Swap the real trusted-key store for a generated test keypair so fixtures can +// be signed with a private key this suite holds. The factory cannot close over +// outer scope, so it generates inline and re-exports the private PEM. +jest.mock('../../../shared/utils/vpp/trusted-keys', () => { + const { generateKeyPairSync } = jest.requireActual('node:crypto') + const { publicKey, privateKey } = generateKeyPairSync('ed25519') + return { + TRUSTED_PACKAGE_KEYS: { 'test-key': publicKey.export({ type: 'spki', format: 'pem' }).toString() }, + __TEST_PRIVATE_PEM: privateKey.export({ type: 'pkcs8', format: 'pem' }).toString(), + } +}) + +// eslint-disable-next-line @typescript-eslint/no-var-requires +const PRIVATE_PEM: string = (require('../../../shared/utils/vpp/trusted-keys') as { __TEST_PRIVATE_PEM: string }) + .__TEST_PRIVATE_PEM + +import { formatPackageIntegrityError, PackageManagerModule } from '../package-manager-module' + +const KEY_ID = 'test-key' +const BOARD_NAME = 'Test VPP Board' + +const sha256 = (s: string): string => + createHash('sha256') + .update(Uint8Array.from(Buffer.from(s, 'utf-8'))) + .digest('hex') + +/** A manifest the installed-read path accepts, providing one named device. */ +const manifestFor = (packageId: string): string => + JSON.stringify({ + formatVersion: '1.0', + package: { id: packageId, name: 'Test Package', version: '1.0.0' }, + devices: [ + { + id: 'test-device', + name: BOARD_NAME, + target: { type: 'runtime-v4' }, + hal: { pluginEntry: 'plugin/main.c' }, + }, + ], + }) + +interface FixtureOpts { + /** Extra files beyond manifest.json, keyed by package-relative POSIX path. */ + files?: Record + /** Override fields on the signed payload (e.g. a foreign keyId). */ + payloadOverride?: Record + /** Rewrite a file AFTER signing, to simulate a mid-session edit. */ + tamperFile?: { rel: string; content: string } + /** Skip signature.json entirely (a hand-assembled package directory). */ + omitSignature?: boolean +} + +describe('PackageManagerModule.verifyBoardPackageIntegrity', () => { + let userDataDir: string + let packagesDir: string + + beforeEach(() => { + userDataDir = mkdtempSync(join(tmpdir(), 'pkg-build-gate-')) + packagesDir = join(userDataDir, 'packages') + ;(app.getPath as jest.Mock).mockReturnValue(userDataDir) + }) + + afterEach(() => { + jest.clearAllMocks() + rmSync(userDataDir, { recursive: true, force: true }) + }) + + /** Write a signed package directory under packagesDir and register it. */ + function installFixture(packageId: string, opts: FixtureOpts = {}): string { + const dir = join(packagesDir, packageId) + const files: Record = { + 'manifest.json': manifestFor(packageId), + 'plugin/main.c': 'int vpp_init(void) { return 0; }', + ...opts.files, + } + + const fileHashes: Record = {} + for (const [rel, content] of Object.entries(files)) { + const full = join(dir, rel) + mkdirSync(dirname(full), { recursive: true }) + writeFileSync(full, content) + fileHashes[rel] = sha256(content) + } + + if (!opts.omitSignature) { + const payload = { + formatVersion: '1.0', + alg: 'ed25519', + keyId: KEY_ID, + packageId, + version: '1.0.0', + signedAt: '2026-06-01T00:00:00.000Z', + files: fileHashes, + ...opts.payloadOverride, + } + const signature = cryptoSign( + null, + Uint8Array.from(Buffer.from(canonicalize(payload), 'utf-8')), + PRIVATE_PEM, + ).toString('base64') + writeFileSync(join(dir, SIGNATURE_FILENAME), JSON.stringify({ ...payload, signature }, null, 2)) + } + + if (opts.tamperFile) { + writeFileSync(join(dir, opts.tamperFile.rel), opts.tamperFile.content) + } + + mkdirSync(packagesDir, { recursive: true }) + writeFileSync( + join(packagesDir, 'registry.json'), + JSON.stringify( + { + formatVersion: '1.0', + packages: { + [packageId]: { + version: '1.0.0', + installedAt: '2026-06-01T00:00:00.000Z', + path: dir, + devices: ['test-device'], + }, + }, + }, + null, + 2, + ), + ) + + return dir + } + + it('passes an untouched signed package — the negative control', () => { + installFixture('com.test.valid') + + expect(new PackageManagerModule().verifyBoardPackageIntegrity(BOARD_NAME)).toEqual({ ok: true }) + }) + + it('passes a board no installed package provides (built-in hals.json board)', () => { + installFixture('com.test.valid') + + expect(new PackageManagerModule().verifyBoardPackageIntegrity('Arduino Uno')).toEqual({ ok: true }) + }) + + it('passes when nothing is installed at all', () => { + expect(new PackageManagerModule().verifyBoardPackageIntegrity(BOARD_NAME)).toEqual({ ok: true }) + }) + + it('fails when the plugin payload was edited after installation', () => { + // The DOPE-539 scenario for runtime-v4: vendor C that the runtime compiles + // on the PLC, rewritten between project open and build. + installFixture('com.test.tampered', { + tamperFile: { rel: 'plugin/main.c', content: 'int vpp_init(void) { /* injected */ return 0; }' }, + }) + + const result = new PackageManagerModule().verifyBoardPackageIntegrity(BOARD_NAME) + + expect(result).toEqual({ + ok: false, + packageId: 'com.test.tampered', + reason: expect.stringContaining('plugin/main.c'), + }) + }) + + it('fails when the manifest itself was edited after installation', () => { + // The licensing-bypass shape: `capabilities.isLicensable` is a manifest + // field, so an edit here is what the gate has to catch even though the + // board still resolves. + const dir = join(packagesDir, 'com.test.relicensed') + installFixture('com.test.relicensed') + writeFileSync( + join(dir, 'manifest.json'), + JSON.stringify({ + formatVersion: '1.0', + package: { id: 'com.test.relicensed', name: 'Test Package', version: '1.0.0' }, + devices: [ + { + id: 'test-device', + name: BOARD_NAME, + target: { type: 'runtime-v4' }, + hal: { pluginEntry: 'plugin/main.c' }, + capabilities: { isLicensable: false }, + }, + ], + }), + ) + + const result = new PackageManagerModule().verifyBoardPackageIntegrity(BOARD_NAME) + + expect(result).toEqual({ + ok: false, + packageId: 'com.test.relicensed', + reason: expect.stringContaining('manifest.json'), + }) + }) + + it('fails when a file was added to the package after signing', () => { + const dir = installFixture('com.test.injected') + writeFileSync(join(dir, 'extra.c'), 'void backdoor(void) {}') + + const result = new PackageManagerModule().verifyBoardPackageIntegrity(BOARD_NAME) + + expect(result).toMatchObject({ ok: false, packageId: 'com.test.injected' }) + }) + + it('fails when the package carries no signature at all', () => { + installFixture('com.test.unsigned', { omitSignature: true }) + + expect(new PackageManagerModule().verifyBoardPackageIntegrity(BOARD_NAME)).toEqual({ + ok: false, + packageId: 'com.test.unsigned', + reason: expect.stringContaining('not signed'), + }) + }) + + it('fails when the signature names a key the editor does not trust', () => { + installFixture('com.test.selfsigned', { payloadOverride: { keyId: 'untrusted-key' } }) + + expect(new PackageManagerModule().verifyBoardPackageIntegrity(BOARD_NAME)).toEqual({ + ok: false, + packageId: 'com.test.selfsigned', + reason: expect.stringContaining('untrusted-key'), + }) + }) + + it('fails when the package directory is gone but the registry entry is not', () => { + const dir = installFixture('com.test.ghost') + // Resolve the board while the files still exist, then remove them — the + // registry entry alone must not be enough to build. + rmSync(join(dir, 'plugin'), { recursive: true, force: true }) + + expect(new PackageManagerModule().verifyBoardPackageIntegrity(BOARD_NAME)).toMatchObject({ + ok: false, + packageId: 'com.test.ghost', + }) + expect(existsSync(join(dir, 'manifest.json'))).toBe(true) + }) + + it('leaves the package on disk and in the registry — refusing is not removing', () => { + const dir = installFixture('com.test.keep', { omitSignature: true }) + + new PackageManagerModule().verifyBoardPackageIntegrity(BOARD_NAME) + + expect(existsSync(dir)).toBe(true) + expect(new PackageManagerModule().listInstalled().map((p) => p.packageId)).toEqual(['com.test.keep']) + }) +}) + +describe('formatPackageIntegrityError', () => { + it('names the board, the package, the reason and the remedy', () => { + const message = formatPackageIntegrityError('Test VPP Board', { + packageId: 'com.test.tampered', + reason: 'Tampered file detected: plugin/main.c', + }) + + expect(message).toContain('Test VPP Board') + expect(message).toContain('com.test.tampered') + expect(message).toContain('Tampered file detected: plugin/main.c') + expect(message).toContain('Reinstall the package') + }) +}) diff --git a/src/backend/editor/package-manager/index.ts b/src/backend/editor/package-manager/index.ts index bc74408ef..8d88baddf 100644 --- a/src/backend/editor/package-manager/index.ts +++ b/src/backend/editor/package-manager/index.ts @@ -1,2 +1,2 @@ -export { PackageManagerModule } from './package-manager-module' -export type { ImportResult, InstalledPackage, PackageManifest, PackageRegistry } from './types' +export { formatPackageIntegrityError, PackageManagerModule } from './package-manager-module' +export type { ImportResult, InstalledPackage, PackageIntegrityResult, PackageManifest, PackageRegistry } from './types' diff --git a/src/backend/editor/package-manager/package-manager-module.ts b/src/backend/editor/package-manager/package-manager-module.ts index 8df5fde12..e53ed46d5 100644 --- a/src/backend/editor/package-manager/package-manager-module.ts +++ b/src/backend/editor/package-manager/package-manager-module.ts @@ -16,7 +16,7 @@ import { TRUSTED_PACKAGE_KEYS } from '../../shared/utils/vpp/trusted-keys' import { verifyPackageSignature } from '../../shared/utils/vpp/verify-package-signature' import { logger } from '../services/logger-service' import { assertPathContained } from '../utils/path-containment' -import type { ImportResult, InstalledPackage, PackageManifest, PackageRegistry } from './types' +import type { ImportResult, InstalledPackage, PackageIntegrityResult, PackageManifest, PackageRegistry } from './types' /** * Enforce cryptographic signature verification on every import. Strict by @@ -202,6 +202,41 @@ class PackageManagerModule { return removed } + /** + * Re-verify the package that provides `boardName`, at the moment a build is + * about to consume it (DOPE-539). + * + * The import check and the project-open sweep both happen strictly BEFORE + * this point, and nothing between them and the compile stops the user from + * editing the installed package: `getInstalledPackageManifest`, the HAL + * source, the licence-store backend and the runtime-v4 plugin payload are + * all read straight off `userData/packages//` when the build runs. So a + * package that passed on open is not evidence about the package being + * compiled — an edit lands in the firmware, or in C the runtime compiles on + * a live PLC, and (because `capabilities.isLicensable` is a manifest field) + * can switch the whole licensing flow off. + * + * This is the gate that actually protects a build, so it fails CLOSED and + * the callers refuse to compile. It deliberately does NOT de-list or delete + * the package the way the open-time sweep does: tearing a directory out from + * under a build in flight is a worse failure than stopping the build and + * saying why. The sweep still owns removal. + * + * A built-in hals.json board has no package behind it and is `ok` — the + * common case, and the reason this costs nothing for most builds. + */ + verifyBoardPackageIntegrity(boardName: string): PackageIntegrityResult { + if (!REQUIRE_SIGNATURE) return { ok: true } + + const match = this.findDeviceByBoardName(boardName) + if (!match) return { ok: true } + + const reason = this.signatureRejectionReason(match.pkg.packageId, match.pkg.path) + if (!reason) return { ok: true } + + return { ok: false, packageId: match.pkg.packageId, reason } + } + /** * Returns null when the installed package recorded at `packagePath` is * genuinely signed by a trusted key, or a short human-readable reason when it @@ -360,4 +395,21 @@ class PackageManagerModule { } } -export { PackageManagerModule } +/** + * The one wording every build-time integrity refusal uses. + * + * Three call sites in the compiler abort on the same condition, and the user + * reads exactly one of the three; they must not each explain it differently. + * The message names the package (what to reinstall), the reason (what is + * wrong) and the remedy, because "signature verification failed" on its own + * reads as an editor bug rather than as "the file on your disk changed". + */ +function formatPackageIntegrityError(boardName: string, failure: { packageId: string; reason: string }): string { + return ( + `Board "${boardName}" is provided by the VPP package "${failure.packageId}", which no longer matches ` + + `its signature: ${failure.reason}. The package files appear to have been modified after installation, ` + + 'so they cannot be trusted for a build. Reinstall the package from a trusted .vpp file and try again.' + ) +} + +export { formatPackageIntegrityError, PackageManagerModule } diff --git a/src/backend/editor/package-manager/types.ts b/src/backend/editor/package-manager/types.ts index 613dfcb00..ff9528a1b 100644 --- a/src/backend/editor/package-manager/types.ts +++ b/src/backend/editor/package-manager/types.ts @@ -15,4 +15,21 @@ type PackageRegistry = { packages: Record> } -export type { ImportResult, InstalledPackage, PackageManifest, PackageRegistry } +/** + * Outcome of the build-time integrity gate + * (`PackageManagerModule.verifyBoardPackageIntegrity`). + * + * `ok: true` covers three genuinely different situations that all mean + * "nothing stands in the way of this build": the board is a built-in + * hals.json entry with no package behind it, the package still matches its + * signature, or enforcement is switched off for local development. The + * caller does not need to tell them apart — it either builds or it does not. + * + * The failure arm carries the `packageId` because the message a user can act + * on has to name the package they must reinstall, and `reason` because + * "files are missing" and "tampered file detected: hal/pi.cpp" send them to + * very different places. + */ +type PackageIntegrityResult = { ok: true } | { ok: false; packageId: string; reason: string } + +export type { ImportResult, InstalledPackage, PackageIntegrityResult, PackageManifest, PackageRegistry }