From 291adedf95305a8854fd93f2324613befe51f384 Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Fri, 7 Aug 2026 14:45:46 -0400 Subject: [PATCH 1/3] fix(compile): install vendor cores from the VPP's board manager URL A VPP that declares `target.boardManagerUrl` could not be compiled when its core lives outside arduino-cli's built-in index. Selecting an IndustrialShields board failed with: Invalid argument passed: Platform 'industrialshields:esp32' not found Arduino CLI process exited with code 7 The URL was resolved onto `BoardBuildInfo.boardManagerUrl` and then dropped: `resolveBoardSelection` did not copy it onto `boardEntry`, `InstallArduinoCoreArgs` had no field to carry it, and `handleCoreInstallation` spawned `core install` without `--additional-urls`. Vendor indexes shipped in VPPs were dead data. It went unnoticed because every other VPP targets a core that is either built into arduino-cli (arduino:avr, arduino:samd, arduino:mbed_edge) or hardcoded in the editor's `ARDUINO_DATA` (esp32, STM32, rp2040, FACTS). IndustrialShields is the first VPP core in neither list. Changes: - `InstallArduinoCoreArgs` gains `boardManagerUrl`, forwarded from `boardEntry` by the pipeline and populated by `resolveBoardSelection`. - `handleCoreInstallation` accepts the URL and passes `--additional-urls` to `core install`. - `handleCoreUpdateIndex` takes the URL too and is now actually called - it was dead code. `core install --additional-urls` alone is not enough, because the CLI resolves the platform against its cached index. The refresh is best-effort: a network failure there must not mask the real install error. - The arduino-cli config was written with `{ flag: 'wx' }` and skipped on EEXIST, so any URL added to `ARDUINO_DATA` never reached an existing install. Missing URLs are now merged into the existing file, preserving user-added entries and other settings. Vendors now ship their board index in the VPP instead of needing a patch to the editor's hardcoded list. Co-Authored-By: Claude Opus 5 (1M context) --- .../handle-core-installation.test.ts | 69 +++++++++++++++++++ .../editor/compiler/compiler-module.ts | 64 +++++++++++++++-- .../compiler/editor-compiler-platform-port.ts | 7 ++ .../editor/services/user-service/index.ts | 54 ++++++++++++--- .../shared/compile/__tests__/pipeline.test.ts | 36 ++++++++++ .../__tests__/resolve-board-selection.test.ts | 55 +++++++++++++++ src/backend/shared/compile/pipeline.ts | 11 +++ .../compile/steps/resolve-board-selection.ts | 5 ++ .../shared/ports/compiler-platform-port.ts | 10 +++ 9 files changed, 298 insertions(+), 13 deletions(-) diff --git a/src/backend/editor/compiler/__tests__/handle-core-installation.test.ts b/src/backend/editor/compiler/__tests__/handle-core-installation.test.ts index 393bd63d6..36415b989 100644 --- a/src/backend/editor/compiler/__tests__/handle-core-installation.test.ts +++ b/src/backend/editor/compiler/__tests__/handle-core-installation.test.ts @@ -126,3 +126,72 @@ describe('handleCoreInstallation (prebuilt core pin = exact manifest version)', expect(message).toMatch(/already installed/) }) }) + +/** + * Vendor board-manager index support. + * + * Regression cover for "Platform 'industrialshields:esp32' not found": a VPP + * declaring `target.boardManagerUrl` must reach arduino-cli as + * `--additional-urls`, on BOTH `core update-index` and `core install`. The + * index refresh is what makes the install resolvable — `core install` alone + * matches against the cached index and still fails. + */ +describe('handleCoreInstallation (vendor board manager URL)', () => { + const VENDOR_URL = 'https://apps.industrialshields.com/main/arduino/boards/package_industrialshields_index.json' + let compilerModule: CompilerModule + + beforeEach(() => { + compilerModule = new CompilerModule() + jest.mocked(spawn).mockReset() + jest.mocked(spawn).mockImplementation(() => fakeChild(0) as unknown as ReturnType) + jest.spyOn(compilerModule, 'getArduinoInstalledCores').mockResolvedValue({} as InstalledCores) + }) + + it('refreshes the index against the vendor URL BEFORE installing', async () => { + await compilerModule.handleCoreInstallation('industrialshields:esp32', jest.fn(), undefined, VENDOR_URL) + + expect(spawn).toHaveBeenCalledTimes(2) + const [, updateArgv] = jest.mocked(spawn).mock.calls[0] + const [, installArgv] = jest.mocked(spawn).mock.calls[1] + expect(updateArgv).toEqual(expect.arrayContaining(['core', 'update-index', '--additional-urls', VENDOR_URL])) + expect(installArgv).toEqual( + expect.arrayContaining(['core', 'install', 'industrialshields:esp32', '--additional-urls', VENDOR_URL]), + ) + }) + + it('passes --additional-urls alongside a pinned core version', async () => { + await compilerModule.handleCoreInstallation('industrialshields:esp32', jest.fn(), '2.7.1', VENDOR_URL) + + const [, installArgv] = jest.mocked(spawn).mock.calls[1] + expect(installArgv).toEqual( + expect.arrayContaining(['core', 'install', 'industrialshields:esp32@2.7.1', '--additional-urls', VENDOR_URL]), + ) + }) + + it('omits --additional-urls entirely when the board declares no vendor index', async () => { + await compilerModule.handleCoreInstallation('arduino:avr', jest.fn(), '1.8.6') + + expect(spawn).toHaveBeenCalledTimes(1) + const [, argv] = jest.mocked(spawn).mock.calls[0] + expect(argv).not.toContain('--additional-urls') + expect(argv).toEqual(expect.arrayContaining(['core', 'install', 'arduino:avr@1.8.6'])) + }) + + it('still attempts the install when the index refresh fails', async () => { + // First spawn (update-index) fails, second (install) succeeds. A flaky + // network on the refresh must not mask the install's own error. + let call = 0 + jest.mocked(spawn).mockImplementation(() => { + call += 1 + return fakeChild(call === 1 ? 1 : 0) as unknown as ReturnType + }) + + await expect( + compilerModule.handleCoreInstallation('industrialshields:esp32', jest.fn(), undefined, VENDOR_URL), + ).resolves.not.toThrow() + + expect(spawn).toHaveBeenCalledTimes(2) + const [, installArgv] = jest.mocked(spawn).mock.calls[1] + expect(installArgv).toEqual(expect.arrayContaining(['core', 'install', 'industrialshields:esp32'])) + }) +}) diff --git a/src/backend/editor/compiler/compiler-module.ts b/src/backend/editor/compiler/compiler-module.ts index 8c2309eb3..d5e50514c 100644 --- a/src/backend/editor/compiler/compiler-module.ts +++ b/src/backend/editor/compiler/compiler-module.ts @@ -938,9 +938,18 @@ class CompilerModule { // and the Arduino sketch walks them dynamically for I/O binding. // The debugger will be redesigned in Phase 4. - // TODO: This method is used to update the index of the Arduino core. - // We should validate if this is necessary and if it works correctly. - async handleCoreUpdateIndex(handleOutputData: HandleOutputDataCallback) { + /** + * `arduino-cli core update-index` — refetch the platform indexes. + * + * Required before installing a core that lives in a third-party index: + * passing `--additional-urls` to `core install` alone is not enough, + * because the CLI resolves the platform against its *cached* index and + * reports "Platform not found" until that cache has seen the vendor URL. + * + * `additionalUrls` is forwarded so the refresh covers the vendor index + * as well as the ones configured in `arduino-cli.yaml`. + */ + async handleCoreUpdateIndex(handleOutputData: HandleOutputDataCallback, additionalUrls?: string) { return new Promise>((resolve, reject) => { let binaryPath = this.arduinoCliBinaryPath const [flag, configFilePath] = this.arduinoCliBaseParameters @@ -949,7 +958,13 @@ class CompilerModule { // INFO: On Windows, we need to add the .exe extension to the binary path. binaryPath += '.exe' } - const executeCommand = spawn(binaryPath, ['core', 'update-index', flag, configFilePath]) + const executeCommand = spawn(binaryPath, [ + 'core', + 'update-index', + ...(additionalUrls ? ['--additional-urls', additionalUrls] : []), + flag, + configFilePath, + ]) let stderrData = '' @@ -971,10 +986,24 @@ class CompilerModule { }) } + /** + * Install the Arduino core a board needs, pulling it from a vendor + * board-manager index when the board declares one. + * + * `boardManagerUrl` comes from the VPP manifest (`target.boardManagerUrl`) + * or hals.json (`board_manager_url`). Cores outside arduino-cli's + * built-in index — `industrialshields:esp32`, for example — are + * unresolvable without it, and the install dies with + * "Platform '' not found" (exit 7). When one is supplied we refresh + * the index against that URL first, then install with the same + * `--additional-urls`; both steps are needed, since `core install` + * resolves against the cached index. + */ async handleCoreInstallation( boardCore: string | null, handleOutputData: (chunk: Buffer | string, logLevel?: 'info' | 'error') => void, coreVersion?: string, + boardManagerUrl?: string, ) { if (boardCore === null) return @@ -994,6 +1023,25 @@ class CompilerModule { handleOutputData(`Installing pinned core ${coreRef} (required by a prebuilt library)...`, 'info') } + // Refresh the platform index against the vendor URL before installing. + // Non-fatal: a transient network failure here should not mask the far + // more useful error that `core install` produces a moment later. + if (boardManagerUrl) { + handleOutputData(`Using vendor board index: ${boardManagerUrl}`, 'info') + try { + // `handleCoreUpdateIndex` logs at the wider 'info' | 'warning' | + // 'error' level set; this callback only accepts 'info' | 'error', + // so fold 'warning' down to 'info'. + await this.handleCoreUpdateIndex( + (chunk, level) => handleOutputData(chunk, level === 'error' ? 'error' : 'info'), + boardManagerUrl, + ) + } catch (error) { + const message = error instanceof Error ? error.message : String(error) + handleOutputData(`Warning: could not refresh the board index (${message}). Continuing.`, 'info') + } + } + let binaryPath = this.arduinoCliBinaryPath if (CompilerModule.HOST_PLATFORM === 'win32') { @@ -1001,7 +1049,13 @@ class CompilerModule { binaryPath += '.exe' } return new Promise>((resolve, reject) => { - const executeCommand = spawn(binaryPath, ['core', 'install', coreRef, ...this.arduinoCliBaseParameters]) + const executeCommand = spawn(binaryPath, [ + 'core', + 'install', + coreRef, + ...(boardManagerUrl ? ['--additional-urls', boardManagerUrl] : []), + ...this.arduinoCliBaseParameters, + ]) let stderrData = '' diff --git a/src/backend/editor/compiler/editor-compiler-platform-port.ts b/src/backend/editor/compiler/editor-compiler-platform-port.ts index 2a5e88beb..c85622d40 100644 --- a/src/backend/editor/compiler/editor-compiler-platform-port.ts +++ b/src/backend/editor/compiler/editor-compiler-platform-port.ts @@ -209,6 +209,12 @@ export function createEditorCompilerPlatformPort( * `handleCoreInstallation` already takes a core id and a log * callback — direct passthrough modulo the log-shape * translation. + * + * `args.boardManagerUrl` (the VPP's `target.boardManagerUrl`) is + * forwarded so vendor cores outside arduino-cli's built-in index + * install automatically rather than failing with "Platform not + * found". `handleCoreInstallation` refreshes the index against + * that URL before installing. */ async installArduinoCore(args: InstallArduinoCoreArgs, log: PlatformLog): Promise { try { @@ -219,6 +225,7 @@ export function createEditorCompilerPlatformPort( log(message, level ?? 'info') }, args.coreVersion, + args.boardManagerUrl, ) return { ok: true } } catch (error) { diff --git a/src/backend/editor/services/user-service/index.ts b/src/backend/editor/services/user-service/index.ts index 5e6aae3c3..c4ee7ebef 100644 --- a/src/backend/editor/services/user-service/index.ts +++ b/src/backend/editor/services/user-service/index.ts @@ -1,7 +1,7 @@ import { getErrorMessage } from '@root/frontend/utils/get-error-message' import { exec } from 'child_process' import { app } from 'electron' -import { access, constants, mkdir, rename, rm, writeFile } from 'fs/promises' +import { access, constants, mkdir, readFile, rename, rm, writeFile } from 'fs/promises' import { basename, join } from 'path' import { promisify } from 'util' @@ -144,22 +144,60 @@ class UserService { } /** - * Checks if the Arduino CLI configuration file exists and creates it if it doesn't. + * Ensure the Arduino CLI configuration file exists and carries every + * board-manager URL the editor ships with. + * + * This used to write with `{ flag: 'wx' }` and swallow `EEXIST`, which + * made the file effectively write-once: any URL added to `ARDUINO_DATA` + * after a user's first launch never reached them, and the only fix was + * deleting the file by hand. Now missing URLs are merged into the + * existing config on every start. + * + * Merge, never overwrite: users add their own indexes and change other + * settings in this file, and clobbering it would silently discard them. + * Anything already present is left untouched, including ordering. */ async #checkIfArduinoCliConfigExists(): Promise { const pathToArduinoCliConfig = join(app.getPath('userData'), 'User', 'arduino-cli.yaml') try { await writeFile(pathToArduinoCliConfig, UserService.ARDUINO_FILE_CONTENT, { flag: 'wx' }) + return } catch (err) { - // If the error is due to the file already existing, log a warning and continue. - if (err instanceof Error && err.message.includes('EEXIST')) { - console.warn(`File already exists at ${pathToArduinoCliConfig}.\nSkipping creation.`) - } else if (err instanceof Error) { - console.error(`Error creating Arduino CLI config at ${pathToArduinoCliConfig}: ${getErrorMessage(err)}`) - } else { + if (!(err instanceof Error && err.message.includes('EEXIST'))) { console.error(`Error creating Arduino CLI config at ${pathToArduinoCliConfig}: ${getErrorMessage(err)}`) + return } } + + // File already exists — reconcile its `additional_urls` with ours. + try { + const existing = await readFile(pathToArduinoCliConfig, 'utf-8') + const shipped = UserService.ARDUINO_FILE_CONTENT.match(/^\s*-\s*(https?:\/\/\S+)\s*$/gm) ?? [] + const missing = shipped + .map((line) => line.trim().replace(/^-\s*/, '')) + .filter((url) => !existing.includes(url)) + + if (missing.length === 0) return + + // Splice the missing entries in under the existing `additional_urls:` + // key, matching its indentation so the YAML stays valid. + const anchor = existing.match(/^(\s*)additional_urls:\s*$/m) + if (!anchor) { + console.warn( + `Arduino CLI config at ${pathToArduinoCliConfig} has no 'additional_urls' key. ` + + `Leaving it alone; missing board indexes: ${missing.join(', ')}`, + ) + return + } + const firstEntry = existing.match(/^(\s*)-\s*https?:\/\//m) + const indent = firstEntry ? firstEntry[1] : `${anchor[1]} ` + const updated = existing.replace(anchor[0], `${anchor[0]}\n${missing.map((u) => `${indent}- ${u}`).join('\n')}`) + + await writeFile(pathToArduinoCliConfig, updated, 'utf-8') + console.warn(`Added ${missing.length} missing board manager URL(s) to ${pathToArduinoCliConfig}.`) + } catch (err) { + console.error(`Error updating Arduino CLI config at ${pathToArduinoCliConfig}: ${getErrorMessage(err)}`) + } } async #executeArduinoCliCommand(command: string): Promise<{ stderr: string; stdout: string }> { diff --git a/src/backend/shared/compile/__tests__/pipeline.test.ts b/src/backend/shared/compile/__tests__/pipeline.test.ts index 169789e12..54c668441 100644 --- a/src/backend/shared/compile/__tests__/pipeline.test.ts +++ b/src/backend/shared/compile/__tests__/pipeline.test.ts @@ -199,6 +199,42 @@ describe('runCompilePipeline — simulator path', () => { expect(callArgs.argv).toEqual(['compile', '-b', 'arduino:avr:mega']) }) + // Regression: the board's `boardManagerUrl` (VPP `target.boardManagerUrl`) + // was resolved onto boardEntry but never forwarded to installArduinoCore, + // so vendor cores outside arduino-cli's built-in index could not be + // installed — "Platform 'industrialshields:esp32' not found". + it('forwards boardEntry.boardManagerUrl to installArduinoCore', async () => { + const port = makePort() + const { emit } = captureEvents() + const boardManagerUrl = + 'https://apps.industrialshields.com/main/arduino/boards/package_industrialshields_index.json' + await runCompilePipeline( + makeArgs({ + isSimulator: false, + boardRuntime: 'arduino-cli', + boardEntry: { + platform: 'industrialshields:esp32:esp32plc', + core: 'industrialshields:esp32', + boardManagerUrl, + }, + }), + port, + emit, + ) + expect(port.installArduinoCore).toHaveBeenCalledWith( + expect.objectContaining({ coreId: 'industrialshields:esp32', boardManagerUrl }), + expect.any(Function), + ) + }) + + it('omits boardManagerUrl for boards that do not declare one', async () => { + const port = makePort() + const { emit } = captureEvents() + await runCompilePipeline(makeArgs({ isSimulator: false, boardRuntime: 'arduino-cli' }), port, emit) + const [coreArgs] = port.installArduinoCore.mock.calls[0] + expect(coreArgs).not.toHaveProperty('boardManagerUrl') + }) + it('calls installArduinoCore + installArduinoLib before compileArduino (no-op semantics for web)', async () => { const port = makePort() const { emit } = captureEvents() diff --git a/src/backend/shared/compile/__tests__/resolve-board-selection.test.ts b/src/backend/shared/compile/__tests__/resolve-board-selection.test.ts index 1998eb024..41b2ea973 100644 --- a/src/backend/shared/compile/__tests__/resolve-board-selection.test.ts +++ b/src/backend/shared/compile/__tests__/resolve-board-selection.test.ts @@ -239,4 +239,59 @@ describe('resolveBoardSelection', () => { expect(result.boardEntry.extra_libraries).toEqual(['P1AM']) } }) + + it('carries target.boardManagerUrl through to boardEntry', () => { + // A VPP whose core is not in arduino-cli's built-in index must surface + // its vendor index here, or the pipeline has nothing to hand to + // `installArduinoCore` and the install fails with "Platform not found". + const boardManagerUrl = + 'https://apps.industrialshields.com/main/arduino/boards/package_industrialshields_index.json' + const pkg: InstalledPackage = { + packageId: 'com.openplc.industrialshields', + version: '1.0.1', + installedAt: '2026-01-01T00:00:00.000Z', + path: '/fake/packages/industrialshields', + devices: ['esp32-plc-14-0-10v'], + } + const manifest: PackageManifest = { + formatVersion: '1.0', + package: { + id: 'com.openplc.industrialshields', + name: 'IndustrialShields PLCs', + version: '1.0.1', + vendor: { name: 'Industrial Shields', logo: 'l.png' }, + description: 'd', + }, + devices: [ + { + id: 'esp32-plc-14-0-10v', + name: 'ESP32 PLC 14 0-10V', + preview: 'p.png', + target: { + type: 'arduino-cli', + core: 'industrialshields:esp32', + platform: 'industrialshields:esp32:plc14ios:cpu=plc14ios', + boardManagerUrl, + }, + hal: { + type: 'arduino-hal', + source: 'hal/arduino/esp32plc.cpp', + define: 'ISPLC_ESP32_PLC_14_0_10V', + }, + }, + ], + } as unknown as PackageManifest + const packageManager: PackageManagerPort = { + listInstalled: () => [pkg], + getInstalledPackageManifest: (id) => (id === pkg.packageId ? manifest : null), + } + const resolver = makeResolver({}, { packageManager }) + + const result = resolveBoardSelection(resolver, 'ESP32 PLC 14 0-10V') + expect(result.ok).toBe(true) + if (result.ok) { + expect(result.boardEntry.core).toBe('industrialshields:esp32') + expect(result.boardEntry.boardManagerUrl).toBe(boardManagerUrl) + } + }) }) diff --git a/src/backend/shared/compile/pipeline.ts b/src/backend/shared/compile/pipeline.ts index 1944d15bb..9100be5ec 100644 --- a/src/backend/shared/compile/pipeline.ts +++ b/src/backend/shared/compile/pipeline.ts @@ -118,6 +118,11 @@ export interface BoardHalsBuildEntry extends BoardHalsCompileEntry { /** Exact Arduino core version to install/verify before linking a prebuilt * arduino library (ABI-locked). From the VPP manifest `target.coreVersion`. */ coreVersion?: string + /** Vendor board-manager index (`package__index.json`). From the + * VPP manifest `target.boardManagerUrl` or hals.json `board_manager_url`. + * Forwarded to `installArduinoCore`, which passes it to arduino-cli as + * `--additional-urls` so cores outside the built-in index resolve. */ + boardManagerUrl?: string /** Compiler / runtime identifier (`'arduino-cli' | 'openplc-compiler' * | 'simulator'`). Used by `resolveTargetCapabilities`'s * preset lookup — without this the resolver can't pick the right @@ -719,6 +724,12 @@ async function runCompilePipelineInner( coreId: typeof boardEntry.platform === 'string' ? deriveArduinoCoreFromPlatform(boardEntry.platform) : '', // Pin the exact core version for prebuilt arduino libraries (ABI-locked). ...(boardEntry.coreVersion ? { coreVersion: boardEntry.coreVersion } : {}), + // Vendor board-manager index for cores outside arduino-cli's built-in + // list. Resolved from the VPP manifest's `target.boardManagerUrl`; the + // editor turns it into `--additional-urls` (and refreshes the index) + // so the core can be auto-installed instead of erroring out with + // "Platform not found". + ...(boardEntry.boardManagerUrl ? { boardManagerUrl: boardEntry.boardManagerUrl } : {}), }, makePlatformLog(emit, 'core-install'), ) diff --git a/src/backend/shared/compile/steps/resolve-board-selection.ts b/src/backend/shared/compile/steps/resolve-board-selection.ts index 492055486..11dc29c62 100644 --- a/src/backend/shared/compile/steps/resolve-board-selection.ts +++ b/src/backend/shared/compile/steps/resolve-board-selection.ts @@ -68,6 +68,11 @@ export function resolveBoardSelection(resolver: BoardInfoResolver, boardTarget: // come from the VPP manifest via BoardBuildInfo; absent for source boards. ...(boardInfo.precompiledLibraryDir ? { precompiledLibraryDir: boardInfo.precompiledLibraryDir } : {}), ...(boardInfo.coreVersion ? { coreVersion: boardInfo.coreVersion } : {}), + // Vendor board-manager index, so a core outside arduino-cli's built-in + // list can be auto-installed. The resolver fills this from the VPP + // manifest's `target.boardManagerUrl` (or hals.json `board_manager_url`); + // dropping it here is what made every VPP-declared index dead data. + ...(boardInfo.boardManagerUrl ? { boardManagerUrl: boardInfo.boardManagerUrl } : {}), // Capability resolution inputs. `resolveTargetCapabilities` // reads `compiler` + `vpp` + `capabilities` on whatever board // shape it's handed — without forwarding all three the diff --git a/src/middleware/shared/ports/compiler-platform-port.ts b/src/middleware/shared/ports/compiler-platform-port.ts index cc13ae2fa..349caa6e0 100644 --- a/src/middleware/shared/ports/compiler-platform-port.ts +++ b/src/middleware/shared/ports/compiler-platform-port.ts @@ -197,6 +197,16 @@ export interface InstallArduinoCoreArgs { * fails if it is unavailable — required for prebuilt arduino-hal boards * whose precompiled `.a` is ABI-locked to that core version. */ coreVersion?: string + /** Optional third-party board-manager index URL (e.g. a vendor's + * `package__index.json`). Sourced from the VPP manifest's + * `target.boardManagerUrl` (or `board_manager_url` in hals.json) and + * forwarded to arduino-cli as `--additional-urls`. + * + * Cores outside arduino-cli's built-in index are invisible without it: + * `core install industrialshields:esp32` fails with "Platform not found" + * unless the vendor index is supplied AND `core update-index` has been + * run against it. The editor does both; web ignores this field. */ + boardManagerUrl?: string } /** Arduino-CLI library install (editor-only. Same no-op From 326e97a3ae327f7fd428ddd897c58746d07e17e6 Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Mon, 10 Aug 2026 13:02:47 -0400 Subject: [PATCH 2/3] fix(review): format, and require https for boardManagerUrl MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the review on #1001. - **Format Check.** `prettier --write` on `user-service/index.ts`; the CI log named exactly that file and it was the one statement flagged. - **`boardManagerUrl` is now validated before it can reach the subprocess.** It rides through `PackageManifestSchema` on `.passthrough()` today, so it is typed in `types.ts` but entirely unchecked at runtime — and it becomes an `--additional-urls` argument to arduino-cli, which downloads a board package full of toolchain executables that later builds run. Signing a VPP vouches for the manifest, not for what the URL serves: arduino-cli's package checksums live INSIDE the index it fetches, so a plaintext index can be intercepted and the package that lands replaced. Requiring https closes that. Only the scheme is constrained — arduino-cli reads compressed indexes (.json.gz, .zip, .bz2) too, so pinning the path suffix would refuse valid vendors for no security gain. This follows the decision already recorded in this file for the version floors: a field a gate reads should not reach it as `unknown`. Strict where the artefact enters, tolerant where we only read what is already on disk — the same split the floors use, and for the same reason. A package installed before this constraint existed has its URL dropped with a warning rather than its whole manifest rejected, which would make every board it provides vanish from the board lookup on an upgrade the user never asked for. Dropping the field leaves it exactly as capable as it was before vendor indexes existed. openplc-packages carries the matching `"pattern": "^https://"`, so a package refused here cannot be built there either. - **Dropped an unnecessary `as unknown as PackageManifest`** in `resolve-board-selection.test.ts`. Verified genuinely redundant: the file typechecks without it. The remaining casts CodeRabbit flagged in `handle-core-installation.test.ts` are pre-existing jest scaffolding and that file's established fixture pattern; diverging from it there would make the file less consistent, not more. Tests: 13 new covering the accepted/refused URL shapes, the compressed-index case, and the drop-not-reject behaviour on the installed-read path. All 12 packages in openplc-packages still validate — every boardManagerUrl in the repo is already https. Co-Authored-By: Claude Opus 5 (1M context) --- .../editor/services/user-service/index.ts | 4 +- .../__tests__/resolve-board-selection.test.ts | 2 +- .../__tests__/package-manifest-schema.test.ts | 87 +++++++++++++++++++ .../shared/ports/package-manifest-schema.ts | 86 +++++++++++++++++- 4 files changed, 173 insertions(+), 6 deletions(-) diff --git a/src/backend/editor/services/user-service/index.ts b/src/backend/editor/services/user-service/index.ts index c4ee7ebef..3ef16c416 100644 --- a/src/backend/editor/services/user-service/index.ts +++ b/src/backend/editor/services/user-service/index.ts @@ -173,9 +173,7 @@ class UserService { try { const existing = await readFile(pathToArduinoCliConfig, 'utf-8') const shipped = UserService.ARDUINO_FILE_CONTENT.match(/^\s*-\s*(https?:\/\/\S+)\s*$/gm) ?? [] - const missing = shipped - .map((line) => line.trim().replace(/^-\s*/, '')) - .filter((url) => !existing.includes(url)) + const missing = shipped.map((line) => line.trim().replace(/^-\s*/, '')).filter((url) => !existing.includes(url)) if (missing.length === 0) return diff --git a/src/backend/shared/compile/__tests__/resolve-board-selection.test.ts b/src/backend/shared/compile/__tests__/resolve-board-selection.test.ts index 41b2ea973..146a6c04a 100644 --- a/src/backend/shared/compile/__tests__/resolve-board-selection.test.ts +++ b/src/backend/shared/compile/__tests__/resolve-board-selection.test.ts @@ -280,7 +280,7 @@ describe('resolveBoardSelection', () => { }, }, ], - } as unknown as PackageManifest + } const packageManager: PackageManagerPort = { listInstalled: () => [pkg], getInstalledPackageManifest: (id) => (id === pkg.packageId ? manifest : null), diff --git a/src/middleware/shared/ports/__tests__/package-manifest-schema.test.ts b/src/middleware/shared/ports/__tests__/package-manifest-schema.test.ts index e1ae083c3..5cbcb540d 100644 --- a/src/middleware/shared/ports/__tests__/package-manifest-schema.test.ts +++ b/src/middleware/shared/ports/__tests__/package-manifest-schema.test.ts @@ -138,3 +138,90 @@ describe('parseInstalledPackageManifest — the load path', () => { expect(parseInstalledPackageManifest(value)).toBeNull() }) }) + +// --------------------------------------------------------------------------- +// Board-manager URL +// +// The URL becomes an `--additional-urls` argument to arduino-cli, which then +// downloads a board package containing toolchain executables. The manifest +// signature vouches for the URL, not for what it serves — and arduino-cli's +// package checksums live inside the index it fetches, so a plaintext index is +// MITM-able. +// --------------------------------------------------------------------------- + +/** A manifest whose single device carries the given `target`. */ +const withTarget = (target: Record) => ({ + formatVersion: '1.0', + package: { id: 'vendor.board', name: 'Vendor Board', version: '1.0.0' }, + devices: [{ id: 'esp32-plc-21', name: 'ESP32 PLC 21', target }], +}) + +const VENDOR_INDEX = 'https://apps.industrialshields.com/main/arduino/boards/package_industrialshields_index.json' + +describe('PackageManifestSchema — board manager URL', () => { + it('accepts a device that declares no target at all', () => { + expect(PackageManifestSchema.safeParse(manifest()).success).toBe(true) + }) + + it('accepts a target with no board manager URL', () => { + expect(PackageManifestSchema.safeParse(withTarget({ type: 'arduino-cli', core: 'arduino:avr' })).success).toBe(true) + }) + + it('accepts the real https vendor index', () => { + expect(PackageManifestSchema.safeParse(withTarget({ boardManagerUrl: VENDOR_INDEX })).success).toBe(true) + }) + + it('accepts a compressed index — the scheme is constrained, not the suffix', () => { + // arduino-cli reads .json.gz / .zip / .bz2 indexes too. + expect(PackageManifestSchema.safeParse(withTarget({ boardManagerUrl: 'https://x.dev/i.json.gz' })).success).toBe( + true, + ) + }) + + it.each([ + ['plaintext http', 'http://x.dev/package_index.json'], + ['a local file', 'file:///etc/passwd'], + ['a bare path', '/tmp/package_index.json'], + ['junk', 'not a url'], + ['an empty string', ''], + ])('rejects %s', (_label, url) => { + const result = PackageManifestSchema.safeParse(withTarget({ boardManagerUrl: url })) + expect(result.success).toBe(false) + }) + + it('leaves the rest of target untouched', () => { + const parsed = PackageManifestSchema.safeParse( + withTarget({ type: 'arduino-cli', core: 'industrialshields:esp32', boardManagerUrl: VENDOR_INDEX }), + ) + expect(parsed.success).toBe(true) + if (parsed.success) { + expect(parsed.data.devices[0]).toMatchObject({ + target: { type: 'arduino-cli', core: 'industrialshields:esp32', boardManagerUrl: VENDOR_INDEX }, + }) + } + }) +}) + +describe('parseInstalledPackageManifest — board manager URL', () => { + it('keeps a usable URL', () => { + const parsed = parseInstalledPackageManifest(withTarget({ boardManagerUrl: VENDOR_INDEX })) + expect(parsed?.devices[0].target.boardManagerUrl).toBe(VENDOR_INDEX) + }) + + it('drops a non-https URL instead of hiding every board in the package', () => { + // A package installed before this constraint existed must keep loading; + // rejecting it would make its boards vanish on an upgrade the user did + // not ask for. + const parsed = parseInstalledPackageManifest( + withTarget({ type: 'arduino-cli', core: 'x:y', boardManagerUrl: 'http://x.dev/i.json' }), + ) + expect(parsed).not.toBeNull() + expect(parsed?.devices[0].target.boardManagerUrl).toBeUndefined() + // Everything else about the device survives. + expect(parsed?.devices[0]).toMatchObject({ id: 'esp32-plc-21', target: { type: 'arduino-cli', core: 'x:y' } }) + }) + + it('leaves a device with no target alone', () => { + expect(parseInstalledPackageManifest(manifest())).not.toBeNull() + }) +}) diff --git a/src/middleware/shared/ports/package-manifest-schema.ts b/src/middleware/shared/ports/package-manifest-schema.ts index fb89d961f..ae0ee9e87 100644 --- a/src/middleware/shared/ports/package-manifest-schema.ts +++ b/src/middleware/shared/ports/package-manifest-schema.ts @@ -61,6 +61,39 @@ const versionFloor = z .min(1) .refine(isValidVersion, { message: 'must be a version like "4.3.2", "4.3", "4" or "v4.3.2"' }) +/** + * A vendor board-manager index URL must be one we are willing to hand to + * arduino-cli as `--additional-urls`. + * + * Same reasoning as `versionFloor`: a field a gate reads should not reach it + * as `unknown`. This one goes further than a gate — it becomes an argument to + * a subprocess that downloads a board package, and board packages carry + * toolchain executables that later builds run. + * + * The signature on a VPP vouches for the manifest, not for what the URL + * serves. arduino-cli's package checksums live INSIDE the index it fetches, + * so a plaintext index is MITM-able and the board package that lands is + * whatever the intercepting index points at. Requiring https closes that. + * + * Only the scheme is constrained. arduino-cli accepts compressed indexes + * (`.json.gz`, `.zip`, `.bz2`) as well as plain `.json`, so pinning the path + * suffix would refuse valid vendors for no security gain. + * + * Authoring-side, openplc-packages' `schema/manifest.schema.json` carries the + * same constraint, so a package that would be refused here cannot be built + * there either. + */ +const boardManagerUrl = z.string().refine( + (value) => { + try { + return new URL(value).protocol === 'https:' + } catch { + return false + } + }, + { message: 'must be an https:// URL' }, +) + export const PackageManifestSchema = z .object({ formatVersion: z.string().min(1), @@ -84,7 +117,17 @@ export const PackageManifestSchema = z minRuntimeVersion: versionFloor.optional(), }) .passthrough(), - devices: z.array(z.object({}).passthrough()).min(1), + devices: z + .array( + z + .object({ + // Only `boardManagerUrl` is declared; everything else on a device + // — and on `target` itself — still flows through untouched. + target: z.object({ boardManagerUrl: boardManagerUrl.optional() }).passthrough().optional(), + }) + .passthrough(), + ) + .min(1), }) .passthrough() @@ -146,6 +189,45 @@ function withComparableFloorsOnly(value: unknown): unknown { return droppedAny ? { ...value, package: kept } : value } +/** True for a board-manager URL this codebase is willing to pass to arduino-cli. */ +function isUsableBoardManagerUrl(value: unknown): boolean { + return value === undefined || (typeof value === 'string' && boardManagerUrl.safeParse(value).success) +} + +/** + * Return `value` with any board-manager URL we would refuse removed, logging + * each one. + * + * Same strict-at-entry / tolerant-on-read split as the floors above, and for + * the same reason: a package installed before this constraint existed must not + * have all of its boards disappear from the board lookup on an upgrade where + * the user did nothing. Dropping the field leaves the package exactly as + * capable as it was before the vendor-index feature existed — the core simply + * is not auto-installed — while the log keeps the cause visible. + */ +function withUsableBoardManagerUrlsOnly(value: unknown): unknown { + if (!isRecord(value) || !Array.isArray(value.devices)) return value + + let droppedAny = false + // `Array.isArray` narrows `unknown` to `any[]`; keep the element type honest + // so the record checks below are doing real work. + const devices = (value.devices as unknown[]).map((device: unknown) => { + if (!isRecord(device) || !isRecord(device.target)) return device + if (isUsableBoardManagerUrl(device.target.boardManagerUrl)) return device + + console.warn( + `[package-manifest] installed device declares a board manager URL that is not https ` + + `(${JSON.stringify(device.target.boardManagerUrl)}); ignoring it — the vendor core it ` + + `points at will not be installed automatically`, + ) + droppedAny = true + const { boardManagerUrl: _dropped, ...target } = device.target + return { ...device, target } + }) + + return droppedAny ? { ...value, devices } : value +} + /** * Validate a manifest read back from a package that is ALREADY * INSTALLED, dropping a floor this codebase cannot compare rather than @@ -165,5 +247,5 @@ function withComparableFloorsOnly(value: unknown): unknown { * silently trading one invisible outcome for another. */ export function parseInstalledPackageManifest(value: unknown): PackageManifest | null { - return parsePackageManifest(withComparableFloorsOnly(value)) + return parsePackageManifest(withUsableBoardManagerUrlsOnly(withComparableFloorsOnly(value))) } From 94c28ebeeeefd5105d7b0df50c48f2c64e85d3b6 Mon Sep 17 00:00:00 2001 From: Thiago Alves Date: Mon, 10 Aug 2026 15:46:05 -0400 Subject: [PATCH 3/3] fix(review): drop the unnecessary devices type assertion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Array.isArray(value.devices)` already makes `.map` available, and annotating the callback parameter as `unknown` is what preserves the runtime element checking — the `as unknown[]` cast added nothing. tsc and eslint are both clean without it. Raised by CodeRabbit on #1001. Co-Authored-By: Claude Opus 5 (1M context) --- src/middleware/shared/ports/package-manifest-schema.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/middleware/shared/ports/package-manifest-schema.ts b/src/middleware/shared/ports/package-manifest-schema.ts index ae0ee9e87..48c438e6b 100644 --- a/src/middleware/shared/ports/package-manifest-schema.ts +++ b/src/middleware/shared/ports/package-manifest-schema.ts @@ -209,9 +209,7 @@ function withUsableBoardManagerUrlsOnly(value: unknown): unknown { if (!isRecord(value) || !Array.isArray(value.devices)) return value let droppedAny = false - // `Array.isArray` narrows `unknown` to `any[]`; keep the element type honest - // so the record checks below are doing real work. - const devices = (value.devices as unknown[]).map((device: unknown) => { + const devices = value.devices.map((device: unknown) => { if (!isRecord(device) || !isRecord(device.target)) return device if (isUsableBoardManagerUrl(device.target.boardManagerUrl)) return device