fix(sign-plugin): prevent symlink escape check bypass and add test coverage by tolzhabayev · Pull Request #2721 · grafana/plugin-tools

tolzhabayev · 2026-06-11T09:17:40Z

What this PR does / why we need it:

@grafana/sign-plugin had almost no test coverage (a single file with two assertions). This PR brings it to 36 tests across 4 files:

buildManifest — sha256 per file, nested directories, MANIFEST.txt exclusion, posix path keys (regression guard for the Windows \ → / sanitisation), internal symlinks included, symlinks escaping the base directory rejected, missing plugin.json. Runs against real temp dirs created with the tmp package (the repo convention for test temp dirs, see Create Plugin: Update tests to use tmp package #2010 — keeps CodeQL from flagging os.tmpdir() usage).
sign command integration tests — real fixture dist dirs with only postData mocked: happy path writes the signed MANIFEST.txt, GRAFANA_COM_URL override, --signatureType/--rootUrls passthrough, invalid root URL exits before any network call, missing token exits, GRAFANA_API_KEY deprecation warning still proceeds, non-200/network errors exit without writing a manifest, createPlugin.version picked up from .config/.cprc.json.
signManifest/saveManifest units — success, non-200 with JSON and non-JSON bodies, network error propagation, write failure.
pluginValidation + getCreatePluginVersion — all validation branches, assertRootUrlIsValid, present/absent/malformed .cprc.json.

To make error paths testable, signManifest now takes the token as a parameter and throws on failure instead of calling process.exit(1); saveManifest likewise throws. Token resolution (including the GRAFANA_API_KEY deprecation warning) moved into the sign command, which owns all output and exits. The integration tests were written before this refactor and pass unchanged after it, so user-facing behavior is preserved.

Which issue(s) this PR fixes:

n/a

Special notes for your reviewer:

The fixture helper realpaths temp dirs because on macOS os.tmpdir() returns /var/... which resolves to /private/var/..., which would otherwise falsely trip the symlink-escape check in walk.
getPluginJson in pluginValidation.ts uses require() in an ESM package and has no callers — it looks like dead code, but removing it felt out of scope for this PR.

pkg-pr-new · 2026-06-11T09:32:09Z

Open in StackBlitz

@grafana/create-plugin

npm i https://pkg.pr.new/grafana/plugin-tools/@grafana/create-plugin@c1a24da -D

@grafana/eslint-config

npm i https://pkg.pr.new/grafana/plugin-tools/@grafana/eslint-config@c1a24da -D

@grafana/eslint-plugin-plugins

npm i https://pkg.pr.new/grafana/plugin-tools/@grafana/eslint-plugin-plugins@c1a24da -D

@grafana/plugin-docs-cli

npm i https://pkg.pr.new/grafana/plugin-tools/@grafana/plugin-docs-cli@c1a24da -D

@grafana/plugin-docs-parser

npm i https://pkg.pr.new/grafana/plugin-tools/@grafana/plugin-docs-parser@c1a24da -D

@grafana/plugin-e2e

npm i https://pkg.pr.new/grafana/plugin-tools/@grafana/plugin-e2e@c1a24da -D

@grafana/plugin-meta-extractor

npm i https://pkg.pr.new/grafana/plugin-tools/@grafana/plugin-meta-extractor@c1a24da -D

@grafana/plugin-types-bundler

npm i https://pkg.pr.new/grafana/plugin-tools/@grafana/plugin-types-bundler@c1a24da -D

@grafana/react-detect

npm i https://pkg.pr.new/grafana/plugin-tools/@grafana/react-detect@c1a24da -D

@grafana/sign-plugin

npm i https://pkg.pr.new/grafana/plugin-tools/@grafana/sign-plugin@c1a24da -D

@grafana/tsconfig

npm i https://pkg.pr.new/grafana/plugin-tools/@grafana/tsconfig@c1a24da -D

commit: c1a24da

jackw

Nice!

Took it for a spin locally and changes look good.

Left a comment about temp file creation. I'm not sure if codeql is broken or the rules changed.

jackw · 2026-06-11T09:42:56Z

@@ -0,0 +1,34 @@
+import { mkdirSync, mkdtempSync, realpathSync, writeFileSync } from 'node:fs';


We use https://github.com/raszi/node-tmp in this repo to handle creation and deletion of temp directories and files as recommended by codeql which was creating noise about os.tmpdir usage in tests.

codeql: https://github.com/grafana/plugin-tools/security/code-scanning/24

fix pr: #2010

Good call - switched the fixtures over to tmp in 797162c.

tolzhabayev · 2026-06-18T13:55:28Z

@jackw need another 👍

jackw

LGTM! 🚀

Copilot

Pull request overview

This PR hardens @grafana/sign-plugin’s manifest generation against symlink-escape bypasses, refactors signing/saving to throw errors (so the command owns exiting/output), and adds substantial unit + integration test coverage across the signing pipeline.

Changes:

Strengthens symlink handling in buildManifest() to prevent base-directory escape via crafted symlink targets.
Refactors signManifest() / saveManifest() to throw on failure and moves token resolution + exit behavior into the sign command.
Adds comprehensive Vitest coverage for manifest creation, signing error paths, CLI behavior, and create-plugin version detection.

Reviewed changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
packages/sign-plugin/src/utils/tests/fixtures.ts	Adds shared temp-dir + file-writing helpers for tests.
packages/sign-plugin/src/utils/pluginValidation.test.ts	Expands coverage for plugin.json validation and root URL validation.
packages/sign-plugin/src/utils/manifest.ts	Fixes symlink escape checks; makes signing/saving throw errors instead of exiting.
packages/sign-plugin/src/utils/manifest.test.ts	Adds unit tests for manifest building, symlinks, signing, and saving.
packages/sign-plugin/src/utils/getCreatePluginVersion.test.ts	Adds tests for reading `.config/.cprc.json` version behavior.
packages/sign-plugin/src/commands/sign.command.ts	Moves token resolution into the command; passes token to `signManifest()`.
packages/sign-plugin/src/commands/sign.command.test.ts	Adds CLI integration tests ensuring correct behavior and failure handling.
packages/sign-plugin/package.json	Adds devDependencies to support new test fixtures.
package-lock.json	Lockfile updates for the new devDependencies.

+import { mkdirSync, realpathSync, writeFileSync } from 'node:fs';
+import path from 'node:path';
+import { dirSync } from 'tmp';
+
+export const DEFAULT_PLUGIN_JSON = {
+  id: 'grafana-test-app',
+  type: 'app',
+  info: {
+    version: '1.0.0',
+  },
+};
+
+// realpathSync prevents false symlink-escape errors on macOS where the temp dir
+// lives under /var/... which resolves to /private/var/...
+export function createTempDir(): string {
+  return realpathSync(dirSync({ prefix: 'sign-plugin-test-', unsafeCleanup: true }).name);
+}


  "devDependencies": {
    "@libs/output": "^1.0.3",
    "@libs/version": "^1.0.2",
-    "@types/minimist": "^1.2.5"
+    "@types/minimist": "^1.2.5",
+    "@types/tmp": "0.2.6",
+    "tmp": "0.2.7"
  },


 export function saveManifest(dir: string, signedManifest: string) {
  try {
    writeFileSync(path.join(dir, MANIFEST_FILE), signedManifest);
    return true;
  } catch (error) {
-    output.error({
-      title: 'Error saving manifest',
-      body: [`Failed to write signed manifest to ${dir}.`],
-    });
-    process.exit(1);
+    throw new Error(`Failed to write signed manifest to ${dir}.`);
  }


mckn

LGTM! Great work on this. Something that we might want to open an issue about:

symlink-to-directory within base dir: walk classifies entries as either isFile() or isSymbolicLink() (via lstat). A symlink pointing to a directory inside the base dir would pass the escape check and be yielded, then hashFile would fail trying to read a directory as a file. This isn't introduced here and is out of scope, but worth a follow-up issue.

mckn · 2026-06-23T11:08:57Z

@@ -0,0 +1,34 @@
+import { mkdirSync, realpathSync, writeFileSync } from 'node:fs';
+import path from 'node:path';
+import { dirSync } from 'tmp';


I think it might be worth looking into dropping the dependency on 'tmp' (if possible).

It might be possible to replace it with: mkdtempSync + realpathSync from node:fs

Yeah, I left it because of #2721 (comment)

mckn · 2026-06-23T11:10:43Z

+  });
+});
+
+describe('saveManifest', () => {


Should we also update this test to use the "tempDirs cleanup pattern" that you have in the other tests? If it is possible.

Should we also update this test to use the "tempDirs cleanup pattern" that you have in the other tests? If it is possible.

good idea, here it is c1a24da#diff-51c38a2838733feeb986bfeafc9e94a52f57c8be8014e2bf0a9b92f267b8b201R175

Adds unit tests for buildManifest, signManifest, saveManifest, pluginValidation and getCreatePluginVersion, plus integration tests for the sign command against real fixture dirs with only the network mocked. To make the error paths testable, signManifest now takes the token as a parameter and throws on failure instead of calling process.exit; saveManifest likewise throws. Token resolution (including the GRAFANA_API_KEY deprecation warning) moved into the sign command, which owns all output and exits. User-facing behavior is unchanged.

…precedence The symlink escape check used a string prefix comparison, so a target in a sibling directory like <baseDir>-evil passed as inside the base directory. Replace it with a path.relative() check and add a regression test for the sibling-prefix bypass. Token resolution moved back inside the signing flow, right before signManifest, so an invalid root URL or malformed manifest is reported before a missing token, matching the original error precedence. Add tests for that precedence and for GRAFANA_ACCESS_POLICY_TOKEN winning over GRAFANA_API_KEY while the deprecation warning is preserved.

Address review feedback: keep the original write error (EACCES/ENOENT) as the cause when saveManifest re-throws, and apply the shared tempDirs cleanup pattern to the saveManifest test block.

Copilot

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.

      const realPath = await fs.realpath(entry);
-      if (!realPath.startsWith(baseDir)) {
+      // A prefix check would treat sibling paths like <baseDir>-evil as inside the base directory.
+      const relativeToBase = path.relative(baseDir, realPath);
+      const isOutsideBaseDir =
+        relativeToBase === '..' || relativeToBase.startsWith('..' + path.sep) || path.isAbsolute(relativeToBase);


+    const signedManifest = await signManifest(manifest, token);

    saveManifest(pluginDistDir, signedManifest);
    output.success({


+function formatServerError(data: string): string {
+  try {
+    return Object.entries(JSON.parse(data))
+      .map(([key, value]) => `${key}: ${value}`)
+      .join(', ');
+  } catch (err) {
+    return data;
  }
 }


github-project-automation Bot added this to Grafana Catalog Team Jun 11, 2026

github-project-automation Bot moved this to 📬 Triage in Grafana Catalog Team Jun 11, 2026

tolzhabayev self-assigned this Jun 11, 2026

grafana-catalog-project-bot Bot moved this from 📬 Triage to 🧑‍💻 In development in Grafana Catalog Team Jun 11, 2026

tolzhabayev marked this pull request as ready for review June 11, 2026 09:26

tolzhabayev requested a review from a team as a code owner June 11, 2026 09:26

tolzhabayev requested review from Ukochka, academo and andresmgot June 11, 2026 09:26

jackw added the preview Opts the PR into pkg.pr.new preview publishing label Jun 11, 2026

tolzhabayev requested a review from jackw June 11, 2026 09:33

tolzhabayev moved this from 🧑‍💻 In development to 🔬 In review in Grafana Catalog Team Jun 11, 2026

tolzhabayev changed the title ~~test(sign-plugin): add manifest and sign command coverage~~ fix(sign-plugin): prevent symlink escape check bypass and add test coverage Jun 11, 2026

jackw previously approved these changes Jun 11, 2026

View reviewed changes

tolzhabayev dismissed jackw’s stale review via 797162c June 11, 2026 13:36

tolzhabayev requested a review from a team as a code owner June 11, 2026 13:36

tolzhabayev requested review from eledobleefe and jackw June 11, 2026 13:36

jackw previously approved these changes Jun 18, 2026

View reviewed changes

tolzhabayev requested a review from mckn June 22, 2026 10:17

mckn requested a review from Copilot June 23, 2026 06:41

Copilot started reviewing on behalf of mckn June 23, 2026 06:41 View session

Copilot AI reviewed Jun 23, 2026

View reviewed changes

mckn previously approved these changes Jun 23, 2026

View reviewed changes

tolzhabayev added 4 commits June 26, 2026 12:16

test(sign-plugin): use tmp package for temp dirs in test fixtures

2e98b59

fix(sign-plugin): preserve original error as cause in saveManifest

c1a24da

Address review feedback: keep the original write error (EACCES/ENOENT) as the cause when saveManifest re-throws, and apply the shared tempDirs cleanup pattern to the saveManifest test block.

tolzhabayev dismissed stale reviews from mckn and jackw via c1a24da June 26, 2026 10:20

tolzhabayev force-pushed the test/sign-plugin-coverage branch from 208b7a5 to c1a24da Compare June 26, 2026 10:20

tolzhabayev requested review from Copilot, jackw and mckn June 26, 2026 10:24

Copilot started reviewing on behalf of tolzhabayev June 26, 2026 10:25 View session

Copilot AI reviewed Jun 26, 2026

View reviewed changes

		@@ -0,0 +1,34 @@
		import { mkdirSync, mkdtempSync, realpathSync, writeFileSync } from 'node:fs';

Uh oh!

Conversation

tolzhabayev commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pkg-pr-new Bot commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jackw left a comment

Choose a reason for hiding this comment

Uh oh!

jackw Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

tolzhabayev Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

tolzhabayev commented Jun 18, 2026

Uh oh!

jackw left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

mckn left a comment

Choose a reason for hiding this comment

Uh oh!

mckn Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

tolzhabayev Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

mckn Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

tolzhabayev Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tolzhabayev commented Jun 11, 2026 •

edited

Loading

pkg-pr-new Bot commented Jun 11, 2026 •

edited

Loading