app: 0001: Verify external plugin manifests

[illume]

Summary

• allow product builds to select an external app plugin manifest with HEADLAMP_BUILD_MANIFEST
• package the selected manifest under the fixed runtime name app-build-manifest.json
• require and verify SHA-256 digests for archive and file sources from external manifests
• confine local plugin files to the manifest directory and copy them privately before verification and extraction
• limit downloads by size and time, clean partial files, and restrict HTTPS redirects by status and host
• reject malformed redirect locations through the normal download error and cleanup path
• cap extracted bytes and entries, reject links and unsupported tar entries, and copy only confined regular files
• prepare the complete plugin set in an adjacent staging directory and preserve the previous set after any failure
• validate selected manifest, plugins, and proxy-urls fields before Electron Builder packages them
• constrain proxy URL globs to the backend-compatible * wildcard syntax
• compose external manifest selection with manifest-declared platform metadata
• run the typed setup entrypoint on its required Node.js 22.6 or newer runtime
• convert the manifest config and setup entrypoints to documented, strictly typed TypeScript
• preserve the bundled default manifest behavior

Test coverage

The unit and e2e commits intentionally precede the implementation commit.

• app unit coverage: 39 focused tests; 85.9% branch coverage across the changed TypeScript scripts (84.48% for build-manifest.ts, 86.81% for setup-plugins.ts)
• app e2e: runs the real setup-plugins.ts Node process, verifies a failed digest preserves the previous plugin set, then installs the verified selected plugin and removes stale entries
• full app unit suite: 14 files, 214 tests passed
• Electron Builder TypeScript config loading and platform metadata composition passed
• app TypeScript check, lint, and formatting passed

Origin

This upstreams the plugin-manifest work from the original downstream review, Azure/aks-desktop__UNCLEAN#228, authored by Oleksandr Dubenko. The resulting 0001-headlamp-upstream-external-plugin-manifest.patch is carried into Azure/aks-desktop#823. The current Azure source-package commits that retain and consume the patch are Azure/aks-desktop@68e024d and Azure/aks-desktop@c3f501e.

Assisted by copilot

[kubernetes-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: illume

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [illume]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Copilot]

Pull request overview

This PR adds support for selecting an external Electron app plugin manifest at build time via HEADLAMP_BUILD_MANIFEST, ensures the selected manifest is packaged under Headlamp’s runtime manifest name, and hardens plugin archive handling by enforcing/validating SHA-256 digests (for remote archives from external manifests) plus safer download redirect behavior.

Changes:

• Add HEADLAMP_BUILD_MANIFEST resolution + manifest loading helper (app/scripts/build-manifest.js) and wire it into plugin setup.
• Enforce/verify SHA-256 digests for plugin archives and harden https download handling (URL validation, HTTPS-only, redirect limits).
• Package the chosen manifest through an Electron Builder config wrapper, and add unit + e2e coverage for the new behavior.

Note: CI status and PR commit history (e.g., merge commits / coherence) were not available in the provided context, so they could not be independently verified here.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
app/scripts/setup-plugins.js	Loads selected build manifest; verifies SHA-256 digests; tightens archive download redirect/URL handling; exports helpers for tests.
app/scripts/build-manifest.js	Adds manifest path resolution and JSON loading (supports HEADLAMP_BUILD_MANIFEST).
app/package.json	Updates Electron Builder invocations to use the new electron-builder.config.js.
app/electron/build-manifest.test.ts	Adds unit tests for manifest selection/packaging and archive integrity/download behavior.
app/electron-builder.config.js	Wraps Electron Builder config to package the selected manifest under app-build-manifest.json.
app/e2e-tests/tests/externalPluginManifest.spec.ts	Adds e2e test that runs setup-plugins.js with an external manifest and a verified local plugin archive.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Suppressed comments (3)

package.json:74

• Same issue as app:build: invoking a .ts script with plain node is expected to fail. Include the TypeScript runner flag when starting the app so plugin setup runs reliably.

    "app:start": "cd app && node ./scripts/setup-plugins.ts && npm run start",

Makefile:335

• setup-plugins.ts is executed with plain node, which is expected to fail for TypeScript sources unless a loader/flag is used. Add the TypeScript runner flag here so make run-app works after this change.

	cd app && npm install && node ./scripts/setup-plugins.ts && npm run start

Makefile:338

• setup-plugins.ts is executed with plain node, which is expected to fail for TypeScript sources unless a loader/flag is used. Add the TypeScript runner flag here so make run-only-app works after this change.

	cd app && npm install && node ./scripts/setup-plugins.ts && npm run dev-only-app

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

Suppressed comments (3)

app/scripts/setup-plugins.ts:204

• downloadFile currently allows 6 redirects because it rejects only when redirectCount > 5. With redirectCount documented as “redirects followed so far”, the usual intent is to allow at most 5 redirects and reject on the 6th hop (i.e. when redirectCount >= 5 before issuing the next request). This also means the “excessive redirects” test is not exercising the actual limit boundary.

    if (parsedUrl.protocol !== 'https:') {
      reject(new Error(`Plugin archive URL must use HTTPS: ${url}`));
      return;
    }
    if (redirectCount > 5) {

app/scripts/setup-plugins.ts:268

• fetchArchive creates a temporary directory but only deletes the downloaded archive file. The extracted contents and the temporary directory itself are never removed, so repeated builds can accumulate headlamp-plugins* directories under the system temp folder.

  const temporaryFolder = fs.mkdtempSync(path.join(os.tmpdir(), 'headlamp-plugins'));
  const archivePath = path.join(temporaryFolder, archiveName);
  await downloadFile(url, archivePath);
  verifyArchiveDigest(archivePath, sha256);
  await extractArchive(name, archivePath, temporaryFolder);

app/scripts/setup-plugins.ts:289

• manifest is parsed from JSON and then cast to BuildManifest, but main() assumes every plugins entry is a well-formed object and destructures it immediately. A malformed manifest like {"plugins":[null]} or an entry missing name will throw a non-actionable runtime error (or produce undefined paths). Adding minimal runtime validation here makes the “verify external manifests” behavior more robust and yields clearer failures.

  for (const plugin of manifest.plugins ?? []) {
    const { name, archive, file, sha256 } = plugin;
    validatePluginSource(plugin);

[Copilot]

Pull request overview

Copilot reviewed 11 out of 13 changed files in this pull request and generated 1 comment.

Files not reviewed (1)

• app/package-lock.json: Generated file

Suppressed comments (4)

app/scripts/setup-plugins.ts:249

• The download promise only observes the destination stream. If the HTTP response aborts or errors mid-body, response.pipe(file) can leave a partial file and the promise pending or surface an unhandled response error; the digest check is then never reached reliably. Use stream.pipeline(response, file) and resolve only after that promise completes so premature closes reject and clean up.

          const file = fs.createWriteStream(destinationPath);
          response.pipe(file);
          file.on('error', reject);

app/scripts/setup-plugins.ts:98

• This still accepts Windows-invalid directory names such as foo:bar, NUL, control characters, or names ending in a dot/space. Those values pass validation but fail or alias when fs.mkdirSync(path.join(PLUGIN_FOLDER, name)) runs in the Windows build. Validate plugin names as portable path segments, including reserved characters/device names and trailing dots/spaces, before clearing the staging directory.

    /[\\/]/.test(plugin.name)

app/scripts/setup-plugins.ts:263

• This treats every non-2xx response carrying Location as a redirect, so a failed 404/500 response can be followed instead of rejected as promised. Also, a malformed Location makes new URL throw inside the asynchronous callback, bypassing reject. Restrict this branch to actual redirect status codes and convert malformed locations into a rejected download.

        } else if (response.headers.location) {
          const redirectUrl = new URL(response.headers.location, parsedUrl).toString();

app/scripts/setup-plugins.ts:1

• The PR history introduces the JavaScript implementation in e18bf2d, replaces it with a 669-line TypeScript conversion in 158281d, then applies a 110-line hardening correction in 662c3d4 and runtime-entrypoint corrections in 0e57f13. This significantly rewrites and fixes the same feature across later commits, making the commit-by-commit flow difficult to review. Please squash/reorder these commits so the implementation lands in its final validated form and each remaining commit is coherent.

import crypto from 'node:crypto';

[Copilot]

Pull request overview

Copilot reviewed 11 out of 13 changed files in this pull request and generated no new comments.

Files not reviewed (1)

• app/package-lock.json: Generated file

Suppressed comments (6)

app/scripts/setup-plugins.ts:285

• This remote-download temporary directory is also left behind; deleting only archivePath retains the extracted archive contents, and failures retain both contents and the downloaded file. Wrap download, verification, and extraction in try/finally and recursively remove the temporary directory.

  const temporaryFolder = fs.mkdtempSync(path.join(os.tmpdir(), 'headlamp-plugins'));

app/scripts/setup-plugins.ts:242

• The download only observes the destination stream. If the HTTP response aborts or errors mid-body, response.pipe(file) can leave this promise pending or surface an unhandled response error, so setup does not reliably reject failed downloads. Use the already imported pipeline so source errors and premature closes reject the promise.

          const file = fs.createWriteStream(destinationPath);
          response.pipe(file);
          file.on('error', reject);

app/scripts/setup-plugins.ts:248

• This follows any non-2xx response carrying Location, including 4xx/5xx failures, contradicting the promised rejection of unsuccessful downloads. Also, new URL() can throw inside this asynchronous callback, bypassing reject; restrict redirects to redirect status codes and convert malformed locations into a rejected promise.

        } else if (response.headers.location) {
          const redirectUrl = new URL(response.headers.location, parsedUrl).toString();

app/scripts/setup-plugins.ts:100

• This validation still accepts names that cannot be created reliably by the Windows build, such as foo:bar, NUL, control characters, or names ending in a dot/space. They pass validation but fail or alias at mkdirSync, after the staging directory has been cleared. Validate name as a portable path segment.

    /[\\/]/.test(plugin.name)

app/scripts/setup-plugins.ts:167

• The default extraction directory is never removed. Every local file entry leaves a complete extracted plugin tree under the system temp directory, including on extraction/copy failures. Track whether extractArchive created the directory and remove owned temporary directories in a finally block.

This issue also appears on line 285 of the same file.

  temporaryFolder: string = fs.mkdtempSync(path.join(os.tmpdir(), 'headlamp-plugins'))

app/scripts/setup-plugins.ts:1

• The PR history introduces the JavaScript implementation in e18bf2d, replaces it with a 669-line TypeScript conversion in 158281d, then applies a 177-line hardening correction in 9a96050 and runtime-entrypoint corrections in e0df468. This preserves known-broken intermediate states and makes the security-sensitive change difficult to review commit by commit. Please squash/reorder these commits so the implementation lands in its final validated form and each remaining commit is coherent.

import crypto from 'node:crypto';

[unlikelyzero]

Overall this is a solid start on archive verification, and the redirect re-validation and digest-format checks are done carefully. One thing outside the changed lines: app/app-build-manifest.json (the manifest Headlamp actually ships with) doesn't declare a sha256 for any of its three plugins, and requireDigest only applies when externalManifest is true. So as it stands, this PR's own verification feature never runs against our official build - it only kicks in for someone else's manifest. Might be worth pinning digests on our own manifest as part of this PR, or at least filing a fast follow, otherwise this reads as verification for third parties but not for us.

A few more things inline.

[illume]

Overall this is a solid start on archive verification, and the redirect re-validation and digest-format checks are done carefully. One thing outside the changed lines: app/app-build-manifest.json (the manifest Headlamp actually ships with) doesn't declare a sha256 for any of its three plugins, and requireDigest only applies when externalManifest is true. So as it stands, this PR's own verification feature never runs against our official build - it only kicks in for someone else's manifest. Might be worth pinning digests on our own manifest as part of this PR, or at least filing a fast follow, otherwise this reads as verification for third parties but not for us.

This is taken care in this following PR now:

• 0003: app: verify packaged plugin identities #6829

Good catch!

[Copilot]

Pull request overview

Copilot reviewed 11 out of 13 changed files in this pull request and generated 2 comments.

Files not reviewed (1)

• app/package-lock.json: Generated file

[Copilot]

Pull request overview

Copilot reviewed 12 out of 14 changed files in this pull request and generated 1 comment.

Files not reviewed (1)

• app/package-lock.json: Generated file

Suppressed comments (3)

package.json:13

• Node 22.6 does not reliably run this ESM-style .ts entrypoint in this package: .ts follows .js, the nearest package has no type: module, and syntax detection only became enabled by default in Node 22.7. The new floor therefore accepts 22.6.x even though the build/start commands fail on the entrypoint's import/import.meta syntax. Raise all engine/lock metadata to at least 22.7.0, or make the entrypoints explicitly ESM (for example, .mts) while retaining 22.6 support.

    "node": ">=22.6.0"

app/scripts/setup-plugins.ts:493

• If the staging rename succeeds but deleting backupFolder throws, this catch sees that PLUGIN_FOLDER already exists, skips restoration, and rethrows. Setup then reports failure after replacing the previous plugin set and leaves the backup behind, contradicting the failure-preservation guarantee. Separate the swap rollback from best-effort backup cleanup, or remove the new folder and restore the backup before propagating the cleanup error.

    fs.rmSync(backupFolder, { recursive: true, force: true });
  } catch (error) {
    if (hadExistingFolder && !fs.existsSync(PLUGIN_FOLDER) && fs.existsSync(backupFolder)) {
      fs.renameSync(backupFolder, PLUGIN_FOLDER);

app/scripts/setup-plugins.ts:295

• The PR history significantly rewrites the same setup implementation across commits: the initial archive implementation is converted in 84b8d166, then changed by 470 lines in fddb586f, and the runtime invocations are corrected in 782d622f. Please squash or reorder these corrective commits into a coherent implementation sequence so each commit is reviewable and runnable.

export function downloadFile(