fix(package-manager): gate the build on VPP package integrity (DOPE-539)

[marconetsf]

Pull request info

References

Link to Jira task

DOPE-539

Description of the changes proposed

VPP signature verification ran at import and at project open. Neither says anything about the package as it exists when a build starts: userData/packages/<id>/ is plain user-writable disk and the compiler reads it fresh every compile. The window was "project open → click Compile" — unbounded and entirely user-controlled.

What that window is worth, all confirmed on development:

Consumed at build time	Where	Effect of a mid-session edit
manifest.json (every read)	getInstalledPackageManifest → findVppDeviceByBoardName → BoardInfoResolver	any manifest field is attacker-chosen for the build
hal.source (arduino-cli)	compileProgram reads it into src/arduino.cpp	arbitrary C++ linked into the firmware
hal.pluginEntry (runtime-v4)	handleVendorPluginPackaging copies the dir into src/vpp_plugin/	arbitrary C compiled on a live PLC by the runtime's compile.sh
hal.licenseStore	injected into the Baremetal sketch	on-device licence storage backend is attacker-chosen
precompiledLibraryDir / localLibrariesDir	passed to arduino-cli as --library	prebuilt objects linked into the firmware

And it voids the licensing gate. capabilities.isLicensable is a manifest field (resolveTargetCapabilities → resolveLicensingTarget), so editing the installed manifest to isLicensable: false — or dropping hal.licenseStore — makes the whole flow skip: no licence FCs on connect, no activation call, no badge, and the build links the weak license_gate_weak.cpp / license_store_weak.cpp defaults. That is a paid-product gate bypassable by a text edit, by exactly the person who benefits from it.

Changes

• PackageManagerModule.verifyBoardPackageIntegrity(boardName) — resolves the VPP behind the board, re-runs verifyPackageSignature against TRUSTED_PACKAGE_KEYS, returns { ok: false, packageId, reason } on failure. No-op for built-in hals.json boards (the common case, so most builds pay nothing) and when REQUIRE_SIGNATURE is false.
• Three call sites: compileProgram before any package file is read, compileForDebugger, and handleVendorPluginPackaging. The last one re-checks rather than trusting compile entry — transpile, strucpp and the v4 bundle compose happen in between, which on a large project is minutes of wall clock, and that step is what copies vendor code into the bundle the PLC compiles. 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.
• formatPackageIntegrityError — one wording for all three refusals, naming the package, the reason and the remedy.
• 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, and stays: it warns the user before they spend a build. The build gate is the authoritative one.

Scope boundary — accepted residual risk, not a TODO

This gate hashes the package directory, then the pipeline reads that directory again. A process that watches for a build and swaps a file in that sub-second window can still land bytes in the build, and precompiledLibraryDir / localLibrariesDir are opened directly by arduino-cli so nothing verifies them between the gate and the link.

Closing that would mean verifying the bytes actually read against the signed hash map, which touches backend/shared/utils/vpp/verify-package-signature.ts (byte-identical with openplc-web, so a mirror PR). That work has been decided against — the residual attack needs local %APPDATA% write access plus winning a race on a build the attacker triggers, which sits inside the threat model DOPE-539 already accepts. DOPE-558 stays open only as the written record of what it would take; it is not queued work.

Reviewers: this PR is deliberately editor-only and needs no mirror PR.

DOD checklist

• The code is complete and according to developers’ standards.
• I have performed a self-review of my code.
• Meet the acceptance criteria.
• Unit tests are written and green.
• Test coverage: __ % — src/backend/editor carries no threshold; the new gate and its message formatter are fully covered by verify-board-package-integrity.test.ts (10 cases: manifest edited, plugin payload edited, file injected, unsigned, untrusted key, missing files, non-VPP board, empty store, plus the untouched-package negative control and "refusing is not removing").
• Integration tests are written and green — no integration harness for a real signed .vpp + toolchain; the refusal path in handleVendorPluginPackaging is covered at the unit level instead.
• Changes were communicated and updated in the ticket description.
• Reviewed and accepted by the Product Owner.
• End-to-end test are successful — needs manual verification with a real signed VPP: install it, open a project, edit manifest.json under %APPDATA%\open-plc-editor\packages\<id>\, compile → must refuse without an editor restart; then reinstall and confirm the untouched package builds and uploads unchanged.

Local verification

• npx jest src/backend/editor src/middleware/adapters/editor → 41 suites, 796 tests, all green
• npx tsc --noEmit, eslint, prettier --check → clean

🤖 Generated with Claude Code

[coderabbitai]

Walkthrough

The package manager now verifies VPP board-package integrity and returns typed results. Packaging, normal compilation, and debugger compilation stop on invalid packages. Tests cover signed packages, tampering, missing content, trust failures, registry consistency, and formatted errors.

Changes

VPP package integrity

Layer / File(s)	Summary
Integrity contract and package verification src/backend/editor/package-manager/types.ts, src/backend/editor/package-manager/package-manager-module.ts, src/backend/editor/package-manager/index.ts, src/backend/editor/package-manager/__tests__/verify-board-package-integrity.test.ts	Adds PackageIntegrityResult, package verification, formatted integrity errors, public exports, signed-package fixtures, and coverage for valid and invalid package states.
Compiler integrity gates src/backend/editor/compiler/compiler-module.ts, src/backend/editor/compiler/__tests__/handle-vendor-plugin-packaging.test.ts	Checks package integrity before packaging, normal compilation, and debugger compilation. Invalid packages produce errors and stop the operation. Packaging regression coverage confirms no destination directory is created.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

• Autonomy-Logic/openplc-editor#886: Both changes modify CompilerModule.handleVendorPluginPackaging.
• Autonomy-Logic/openplc-editor#997: Both changes add VPP build-time validation in compiler-module.ts.

Suggested labels: enhancement

Suggested reviewers: thiagoralves

Poem

I checked each package, leaf by leaf,
And stopped the build when proofs were brief.
Signed files stayed safe and sound,
Tampered bundles stayed unbound.
Hop, hop—integrity now leads the way! 🐇

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the main change: enforcing VPP package integrity during builds.
Description check	✅ Passed	The description is detailed, follows the template, documents changes and risks, and reports test results and remaining manual verification.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch bugfix/DOPE-539-vpp-integrity-gate-at-build

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (2)

src/backend/editor/compiler/__tests__/handle-vendor-plugin-packaging.test.ts (1)

38-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use PackageIntegrityResult for the integrity mock.

The current type permits { ok: false } without a package ID or a reason. Use the production discriminated union so test fixtures preserve the compiler contract.

Proposed fix

+import type { PackageIntegrityResult } from '../../package-manager'
+
-const verifyBoardPackageIntegrity = jest.fn<{ ok: boolean; packageId?: string; reason?: string }, [string]>(() => ({
-  ok: true,
-}))
+const verifyBoardPackageIntegrity = jest.fn(
+  (_boardName: string): PackageIntegrityResult => ({ ok: true }),
+)

As per coding guidelines, “Model variant states as discriminated unions.”

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/backend/editor/compiler/__tests__/handle-vendor-plugin-packaging.test.ts`
around lines 38 - 40, Update the verifyBoardPackageIntegrity mock’s generic
return type to use the production PackageIntegrityResult discriminated union
instead of the inline object type, ensuring test fixtures require the
appropriate packageId and reason fields for failed results while preserving the
existing successful fixture.

Source: Coding guidelines

src/backend/editor/package-manager/__tests__/verify-board-package-integrity.test.ts (1)

31-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove prohibited type assertions from the test setup.

Use unknown and narrow the mocked trusted-key module before reading __TEST_PRIVATE_PEM. Configure or narrow the mocked app.getPath function without as jest.Mock.

src/backend/editor/package-manager/__tests__/verify-board-package-integrity.test.ts#31-L32, src/backend/editor/package-manager/__tests__/verify-board-package-integrity.test.ts#77

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/backend/editor/package-manager/__tests__/verify-board-package-integrity.test.ts`
around lines 31 - 32, In
src/backend/editor/package-manager/__tests__/verify-board-package-integrity.test.ts
lines 31-32, replace the trusted-key module type assertion with an unknown value
and narrow it before reading __TEST_PRIVATE_PEM; at line 77, configure or narrow
app.getPath through its existing typed mock without using as jest.Mock. Update
both sites while preserving the test setup behavior.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In
`@src/backend/editor/compiler/__tests__/handle-vendor-plugin-packaging.test.ts`:
- Around line 38-40: Update the verifyBoardPackageIntegrity mock’s generic
return type to use the production PackageIntegrityResult discriminated union
instead of the inline object type, ensuring test fixtures require the
appropriate packageId and reason fields for failed results while preserving the
existing successful fixture.

In
`@src/backend/editor/package-manager/__tests__/verify-board-package-integrity.test.ts`:
- Around line 31-32: In
src/backend/editor/package-manager/__tests__/verify-board-package-integrity.test.ts
lines 31-32, replace the trusted-key module type assertion with an unknown value
and narrow it before reading __TEST_PRIVATE_PEM; at line 77, configure or narrow
app.getPath through its existing typed mock without using as jest.Mock. Update
both sites while preserving the test setup behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6d6f8f9d-b380-488f-9111-4268178fd122

📥 Commits

Reviewing files that changed from the base of the PR and between e5fbc33 and 27fbc4a.

📒 Files selected for processing (6)

• src/backend/editor/compiler/__tests__/handle-vendor-plugin-packaging.test.ts
• src/backend/editor/compiler/compiler-module.ts
• src/backend/editor/package-manager/__tests__/verify-board-package-integrity.test.ts
• src/backend/editor/package-manager/index.ts
• src/backend/editor/package-manager/package-manager-module.ts
• src/backend/editor/package-manager/types.ts

[JulioSergioFS]

PR Review — openplc-editor #1000

Verdict: Approve with comments

The threat model is real and precisely stated, the gate fails closed, and the call-site placement is thought through rather than sprayed. Nothing here blocks merge. The comments below are about (a) two of the three gates having no test at their call site, (b) the licensing-bypass claim in the description being broader than what the code actually closes, and (c) the arduino/baremetal path getting half the protection the runtime-v4 path gets, for the same window.

What I verified against the code

• The consumption table in the description is accurate. BoardInfoResolver.#fromVppDevice maps hal.source → halSourceFile, hal.pluginEntry, hal.licenseStore → licenseStoreFiles, hal.libraries → localLibrariesDir, hal.precompiledLibrary → precompiledLibraryDir and device.capabilities → capabilities, all resolved against pkg.path — i.e. read off userData/packages/<id>/ at build time, exactly as claimed.
• The handleVendorPluginPackaging throw really does bail the pipeline. packageVppPlugin in editor-compiler-platform-port.ts:601 converts the throw into errors[], and backend/shared/compile/pipeline.ts:572 turns a non-empty errors[] into bailError. The "a logged error would upload a bundle with no vendor I/O" reasoning holds.
• The abort pattern matches the file's existing convention. postMessage({logLevel:'error'}) → close() → return is what every other early-exit in compileProgram / compileForDebugger does. No new control-flow shape introduced.
• "Editor-only, no mirror PR" is correct. .github/workflows/ci-sync.yml triggers the surface comparison only on src/frontend/, src/middleware/shared/, src/backend/shared/, src/__architecture__/ and resources/sources/(arduino|Baremetal|hal)/. All six changed files are under src/backend/editor/. The Shared Surface Sync job passing confirms it.
• Refusing is genuinely not removing. verifyBoardPackageIntegrity calls signatureRejectionReason and returns; verifyInstalledSignatures (the open-time sweep) is the only thing that still rmSynces and mutates the registry. The test pins this.

Strengths

• The gate reuses the sweep's own rejection helper rather than re-deriving what "bad" means. signatureRejectionReason already covers bad id shape, path escaping packagesDir, missing files and every verifyPackageSignature failure, so the build gate and the startup sweep can't drift on the definition of a valid package.
• ok: true for a built-in hals.json board is the right default and is cheap. findDeviceByBoardName returns null for every non-VPP board, so the common build pays one registry read and no hashing.
• The second gate's placement argument is correct and non-obvious. It sits outside the method's catch-all because every other failure there degrades the build and reports it, while this one has to stop it. That distinction is stated in the comment, which is the right place for it.
• The test suite is adversarial rather than confirmatory. Manifest edited (the licensing shape, with isLicensable: false in the tampered manifest), payload edited, file injected, unsigned, untrusted key, plus two negative controls and an explicit "refusing is not removing" assertion. The trusted-key store is swapped for a generated keypair so fixtures are genuinely signed — not stubbed past the crypto.
• The scope-boundary section is honest. It names the residual race, names what closing it would cost (the byte-identical verify-package-signature.ts, hence a mirror PR), and states that it has been decided against rather than dressing it as a TODO. That is the right way to write this section.

Findings

1. (Medium) Two of the three gates have no test at their call site

handle-vendor-plugin-packaging.test.ts covers one gate. compileProgram and compileForDebugger are covered only transitively, by the module-level unit tests for verifyBoardPackageIntegrity itself.

What is untested is precisely the part a refactor would break silently:

• that the gate runs before resolveBoardSelection and before any package file is read — someone moving board resolution above it would still pass every existing test;
• that the abort is close() + return and not a throw that the worker turns into a generic failure message.

Both are cheap to pin: the existing suites for these methods already stub _mainProcessPort, so an assertion that the port received the integrity message and was closed, with no board resolution attempted, is a handful of lines.

2. (Medium) The licensing-bypass claim is broader than what this closes

The description says editing isLicensable: false means "no licence FCs on connect, no activation call, no badge" — and then presents this PR as closing that. It closes the build half only.

resolveLicensingTarget / resolveTargetCapabilities are consumed in the renderer, off boardInfo derived from the installed manifest: src/frontend/hooks/use-device-license.ts:71, .../device/configuration/board.tsx:718, and the manifest read at src/main/modules/ipc/main.ts:1669. None of those go through a compile. A tampered manifest still suppresses the licence FCs, the activation call and the badge without the user ever pressing Compile.

That is not commercially serious on its own — the device-side gate is what enforces, and this PR does stop the firmware from being built against the weak license_gate_weak.cpp / license_store_weak.cpp defaults, which is the part that matters. But the description reads as if the vector is closed. Either extend the check to the connect path (main.ts:1669 is the natural chokepoint — it is the one place the renderer gets a manifest) or narrow the wording so the next reader doesn't assume the connect path is covered.

3. (Medium) The arduino/baremetal path gets one gate for the same multi-minute window

The reasoning for the second gate — "transpile, strucpp and the v4 bundle compose happen in between, which on a large project is minutes of wall clock" — applies verbatim to the arduino path, which gets only the entry check. Between the gate at compileProgram and the actual reads:

• handleGenerateArduinoCppFile copies hal.source into src/arduino.cpp;
• licenseStoreFiles are injected into the Baremetal sketch;
• precompiledLibraryDir / localLibrariesDir are handed to arduino-cli as --library and opened by arduino-cli itself at link time.

So the path that links arbitrary C++ into the firmware and chooses the on-device licence store has the weaker coverage of the two. If the second hash is worth its cost for runtime-v4, the same call before handleCompileArduinoProgram (or installAsArduinoLibrary) is worth it here. The "accepted residual risk" paragraph covers the sub-second race, not this one.

4. (Low) The gate does redundant work, on the main thread

Each call site does new PackageManagerModule() — whose constructor mkdirSyncs — then verifyBoardPackageIntegrity re-reads registry.json and parses every installed manifest via findVppDeviceByBoardName, then verifyPackageSignature walks and readFileSync+sha256s every file in the package, all synchronously. In handleVendorPluginPackaging the board is then resolved a second time on the very next line (compiler-module.ts:2024 then :2032).

compileProgram / compileForDebugger take a MessagePortMain, i.e. they run in the Electron main process, so a VPP shipping a sizeable precompiledLibrary blocks the UI thread for the duration of the hash — now twice per runtime-v4 build. Two cheap improvements:

• have verifyBoardPackageIntegrity return the resolved VppDeviceMatch on success so handleVendorPluginPackaging doesn't resolve it again;
• if any shipped VPP is large, make the hash async (or cache on (path, mtime, size) between the two gates of a single build).

Not a blocker — the startup sweep already pays a comparable cost across all installed packages.

5. (Low) A tampered manifest that breaks board resolution produces the wrong diagnostic

findVppDeviceByBoardName skips packages whose manifest cannot be parsed and matches on device.name. So an edit that renames the device, or that leaves the JSON invalid, makes the gate return ok: true (no match) and the build then dies downstream in resolveBoardSelection with a "board not found" message. Not a hole — such a build cannot succeed either way — but the user is told the wrong thing about a tampered package.

Cheap improvement: when no device matches and the registry lists at least one package, run signatureRejectionReason over the listed packages and report a tamper message if any fails, before falling through to ok: true.

6. (Low) REQUIRE_SIGNATURE now controls three behaviours, and its comment describes one

The constant's doc says "Enforce cryptographic signature verification on every import… Flip to false ONLY for local/offline development with unsigned packages." It now also disables the startup sweep and this build gate. A developer flipping it for an unsigned local .vpp silently turns off the build gate too, which is a materially different thing from relaxing an import check. One added sentence.

7. (Nit) Test name doesn't match what the test does

'fails when the package directory is gone but the registry entry is not' removes only plugin/, then asserts manifest.json still exists. The !existsSync(packagePath) branch of signatureRejectionReason — the "package files are missing" reason — is never exercised. Either rename the case (it is really "files removed from the package") or add one that removes the whole directory.

8. (Nit) The re-implemented error formatter in the vendor-plugin mock

handle-vendor-plugin-packaging.test.ts mocks formatPackageIntegrityError with its own, differently-worded string, while the assertion only checks that the package id appears. The duplication buys nothing and can drift from the real wording. Passing the real function through (or asserting on a fragment the mock doesn't own) is simpler.

CodeRabbit's note on the same file is also worth taking: type the mock as PackageIntegrityResult. The hand-written mock type permits { ok: false } with no packageId/reason, which the real discriminated union forbids — so the mock can express a state the production type cannot.

Test assessment

Strong where it counts. Ten cases against real signed fixtures, with the trusted-key store replaced by a generated Ed25519 keypair rather than the crypto being stubbed out, so the tests exercise verifyPackageSignature for real. Both negative controls (untouched package passes; non-VPP board passes) are present, which is what stops a gate like this from being trivially "always fails". The "leaves the package on disk and in the registry" test pins the deliberate divergence from the sweep, which is the design decision most likely to be "corrected" by a future contributor.

Gaps, in priority order:

1. call-site tests for the two compiler gates (finding 1);
2. no test that a second, untampered package is unaffected when a different installed package is tampered — findVppDeviceByBoardName is "first match wins" over listInstalled() order, and it would be good to pin that the gate follows the same resolution the build does;
3. the manual E2E in the DOD checklist is correctly still open, and it is the only thing that exercises a real signed .vpp end to end. Worth doing before merge rather than after, since it is also the only check on the message a user actually sees.