From 912bca1dd0001b3a83ef9ff1dda89a886ba717b2 Mon Sep 17 00:00:00 2001 From: Timur Olzhabayev Date: Thu, 11 Jun 2026 11:17:14 +0200 Subject: [PATCH 1/4] test(sign-plugin): add manifest and sign command coverage 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. --- .../src/commands/sign.command.test.ts | 172 +++++++++++++++++ .../sign-plugin/src/commands/sign.command.ts | 28 ++- .../src/utils/getCreatePluginVersion.test.ts | 41 ++++ .../sign-plugin/src/utils/manifest.test.ts | 175 ++++++++++++++++++ packages/sign-plugin/src/utils/manifest.ts | 70 ++----- .../src/utils/pluginValidation.test.ts | 40 +++- .../sign-plugin/src/utils/tests/fixtures.ts | 34 ++++ 7 files changed, 507 insertions(+), 53 deletions(-) create mode 100644 packages/sign-plugin/src/commands/sign.command.test.ts create mode 100644 packages/sign-plugin/src/utils/getCreatePluginVersion.test.ts create mode 100644 packages/sign-plugin/src/utils/manifest.test.ts create mode 100644 packages/sign-plugin/src/utils/tests/fixtures.ts diff --git a/packages/sign-plugin/src/commands/sign.command.test.ts b/packages/sign-plugin/src/commands/sign.command.test.ts new file mode 100644 index 0000000000..117d2b51de --- /dev/null +++ b/packages/sign-plugin/src/commands/sign.command.test.ts @@ -0,0 +1,172 @@ +import minimist from 'minimist'; +import { existsSync, readFileSync, rmSync } from 'node:fs'; +import path from 'node:path'; +import { MockInstance, vi } from 'vitest'; +import { sign } from './sign.command.js'; +import { ManifestInfo } from '../utils/manifest.js'; +import { createPluginDistDir, createTempDir, writeFiles } from '../utils/tests/fixtures.js'; + +const mocks = vi.hoisted(() => { + return { + postData: vi.fn(), + }; +}); + +vi.mock('../utils/request.js', () => { + return { postData: mocks.postData }; +}); + +const PROCESS_EXIT_MESSAGE = 'process.exit called'; + +function argvFor(args: Record = {}): minimist.ParsedArgs { + return { _: [], ...args } as minimist.ParsedArgs; +} + +function getPostedManifest(): ManifestInfo { + return mocks.postData.mock.calls[0][1] as ManifestInfo; +} + +describe('sign command', () => { + const tempDirs: string[] = []; + let stdoutSpy: MockInstance; + let exitSpy: MockInstance; + + function pluginDistDir(files: Record = {}) { + const dir = createPluginDistDir(files); + tempDirs.push(dir); + return dir; + } + + beforeEach(() => { + exitSpy = vi.spyOn(process, 'exit').mockImplementation(() => { + throw new Error(PROCESS_EXIT_MESSAGE); + }); + stdoutSpy = vi.spyOn(process.stdout, 'write').mockImplementation(() => true); + vi.stubEnv('GRAFANA_ACCESS_POLICY_TOKEN', 'test-token'); + vi.stubEnv('GRAFANA_API_KEY', undefined); + vi.stubEnv('GRAFANA_COM_URL', undefined); + mocks.postData.mockReset().mockResolvedValue({ status: 200, data: 'SIGNED-MANIFEST' }); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + vi.restoreAllMocks(); + }); + + afterAll(() => { + tempDirs.forEach((dir) => rmSync(dir, { recursive: true, force: true })); + }); + + it('should exit when the plugin dist directory does not exist', async () => { + await expect(sign(argvFor({ distDir: '/path/that/does/not/exist' }))).rejects.toThrow(PROCESS_EXIT_MESSAGE); + + expect(exitSpy).toHaveBeenCalledWith(1); + expect(mocks.postData).not.toHaveBeenCalled(); + }); + + it('should post the manifest to the Grafana API and save the signed response', async () => { + const dir = pluginDistDir({ 'module.js': 'export {};' }); + + await sign(argvFor({ distDir: dir })); + + const [url, manifest, headers] = mocks.postData.mock.calls[0]; + expect(url).toBe('https://grafana.com/api/plugins/ci/sign'); + expect(headers).toEqual({ Authorization: 'Bearer test-token' }); + expect(manifest).toMatchObject({ + plugin: 'grafana-test-app', + version: '1.0.0', + signPlugin: { version: expect.any(String) }, + }); + expect(readFileSync(path.join(dir, 'MANIFEST.txt'), 'utf-8')).toBe('SIGNED-MANIFEST'); + }); + + it('should respect the GRAFANA_COM_URL environment variable', async () => { + vi.stubEnv('GRAFANA_COM_URL', 'https://grafana-dev.com/api'); + const dir = pluginDistDir(); + + await sign(argvFor({ distDir: dir })); + + expect(mocks.postData.mock.calls[0][0]).toBe('https://grafana-dev.com/api/plugins/ci/sign'); + }); + + it('should pass signatureType and rootUrls to the manifest', async () => { + const dir = pluginDistDir(); + + await sign( + argvFor({ + distDir: dir, + signatureType: 'private', + rootUrls: 'https://example.com/grafana,https://grafana.example.com', + }) + ); + + expect(getPostedManifest()).toMatchObject({ + signatureType: 'private', + rootUrls: ['https://example.com/grafana', 'https://grafana.example.com'], + }); + }); + + it('should exit without calling the API when a root URL is invalid', async () => { + const dir = pluginDistDir(); + + await expect(sign(argvFor({ distDir: dir, rootUrls: 'not-a-valid-url' }))).rejects.toThrow(PROCESS_EXIT_MESSAGE); + + expect(exitSpy).toHaveBeenCalledWith(1); + expect(mocks.postData).not.toHaveBeenCalled(); + expect(existsSync(path.join(dir, 'MANIFEST.txt'))).toBe(false); + }); + + it('should exit without calling the API when no token is configured', async () => { + vi.stubEnv('GRAFANA_ACCESS_POLICY_TOKEN', undefined); + const dir = pluginDistDir(); + + await expect(sign(argvFor({ distDir: dir }))).rejects.toThrow(PROCESS_EXIT_MESSAGE); + + expect(exitSpy).toHaveBeenCalledWith(1); + expect(mocks.postData).not.toHaveBeenCalled(); + }); + + it('should warn about deprecation but proceed when only GRAFANA_API_KEY is set', async () => { + vi.stubEnv('GRAFANA_ACCESS_POLICY_TOKEN', undefined); + vi.stubEnv('GRAFANA_API_KEY', 'legacy-api-key'); + const dir = pluginDistDir(); + + await sign(argvFor({ distDir: dir })); + + const stdout = stdoutSpy.mock.calls.map((call) => String(call[0])).join(''); + expect(stdout).toContain('deprecated'); + expect(mocks.postData.mock.calls[0][2]).toEqual({ Authorization: 'Bearer legacy-api-key' }); + }); + + it('should exit without saving the manifest when the API responds with an error', async () => { + mocks.postData.mockResolvedValue({ status: 400, data: JSON.stringify({ message: 'invalid plugin' }) }); + const dir = pluginDistDir(); + + await expect(sign(argvFor({ distDir: dir }))).rejects.toThrow(PROCESS_EXIT_MESSAGE); + + expect(exitSpy).toHaveBeenCalledWith(1); + expect(existsSync(path.join(dir, 'MANIFEST.txt'))).toBe(false); + }); + + it('should exit when the API request fails', async () => { + mocks.postData.mockRejectedValue(new Error('socket hang up')); + const dir = pluginDistDir(); + + await expect(sign(argvFor({ distDir: dir }))).rejects.toThrow(PROCESS_EXIT_MESSAGE); + + expect(exitSpy).toHaveBeenCalledWith(1); + expect(existsSync(path.join(dir, 'MANIFEST.txt'))).toBe(false); + }); + + it('should add the create-plugin version from .config/.cprc.json to the manifest', async () => { + const cwdDir = createTempDir(); + tempDirs.push(cwdDir); + writeFiles(cwdDir, { '.config/.cprc.json': JSON.stringify({ version: '5.0.0' }) }); + vi.spyOn(process, 'cwd').mockReturnValue(cwdDir); + const dir = pluginDistDir(); + + await sign(argvFor({ distDir: dir })); + + expect(getPostedManifest().createPlugin).toEqual({ version: '5.0.0' }); + }); +}); diff --git a/packages/sign-plugin/src/commands/sign.command.ts b/packages/sign-plugin/src/commands/sign.command.ts index 0a7044f1d4..d867ab636e 100644 --- a/packages/sign-plugin/src/commands/sign.command.ts +++ b/packages/sign-plugin/src/commands/sign.command.ts @@ -25,6 +25,8 @@ export const sign = async (argv: minimist.ParsedArgs) => { process.exit(1); } + const token = getSigningToken(); + try { output.log({ title: 'Signing plugin.', @@ -46,7 +48,7 @@ export const sign = async (argv: minimist.ParsedArgs) => { manifest.createPlugin = { version: createPluginVersion }; } - const signedManifest = await signManifest(manifest); + const signedManifest = await signManifest(manifest, token); saveManifest(pluginDistDir, signedManifest); output.success({ @@ -63,3 +65,27 @@ export const sign = async (argv: minimist.ParsedArgs) => { process.exit(1); } }; + +function getSigningToken(): string { + const GRAFANA_API_KEY = process.env.GRAFANA_API_KEY; + const GRAFANA_ACCESS_POLICY_TOKEN = process.env.GRAFANA_ACCESS_POLICY_TOKEN; + const token = GRAFANA_ACCESS_POLICY_TOKEN ? GRAFANA_ACCESS_POLICY_TOKEN : GRAFANA_API_KEY; + + if (!token) { + output.error({ + title: 'Missing GRAFANA_ACCESS_POLICY_TOKEN.', + body: ['You must create a GRAFANA_ACCESS_POLICY_TOKEN env variable to sign plugins.'], + link: 'https://grafana.com/developers/plugin-tools/publish-a-plugin/sign-a-plugin#generate-an-access-policy-token', + }); + process.exit(1); + } + if (GRAFANA_API_KEY) { + output.warning({ + title: 'Usage of GRAFANA_API_KEY is deprecated.', + body: ['Please migrate to using a GRAFANA_ACCESS_POLICY_TOKEN instead.'], + link: 'https://grafana.com/developers/plugin-tools/publish-a-plugin/sign-a-plugin', + }); + } + + return token; +} diff --git a/packages/sign-plugin/src/utils/getCreatePluginVersion.test.ts b/packages/sign-plugin/src/utils/getCreatePluginVersion.test.ts new file mode 100644 index 0000000000..5e620d06b6 --- /dev/null +++ b/packages/sign-plugin/src/utils/getCreatePluginVersion.test.ts @@ -0,0 +1,41 @@ +import { rmSync } from 'node:fs'; +import { vi } from 'vitest'; +import { getCreatePluginVersion } from './getCreatePluginVersion.js'; +import { createTempDir, writeFiles } from './tests/fixtures.js'; + +describe('getCreatePluginVersion', () => { + const tempDirs: string[] = []; + + function mockCwd(files: Record = {}) { + const dir = createTempDir(); + tempDirs.push(dir); + writeFiles(dir, files); + vi.spyOn(process, 'cwd').mockReturnValue(dir); + } + + afterEach(() => { + vi.restoreAllMocks(); + }); + + afterAll(() => { + tempDirs.forEach((dir) => rmSync(dir, { recursive: true, force: true })); + }); + + it('should return the version from .config/.cprc.json', () => { + mockCwd({ '.config/.cprc.json': JSON.stringify({ version: '5.0.0' }) }); + + expect(getCreatePluginVersion()).toBe('5.0.0'); + }); + + it('should return null when .config/.cprc.json does not exist', () => { + mockCwd(); + + expect(getCreatePluginVersion()).toBeNull(); + }); + + it('should return null when .config/.cprc.json is malformed', () => { + mockCwd({ '.config/.cprc.json': 'not-valid-json' }); + + expect(getCreatePluginVersion()).toBeNull(); + }); +}); diff --git a/packages/sign-plugin/src/utils/manifest.test.ts b/packages/sign-plugin/src/utils/manifest.test.ts new file mode 100644 index 0000000000..eaaad6c8da --- /dev/null +++ b/packages/sign-plugin/src/utils/manifest.test.ts @@ -0,0 +1,175 @@ +import crypto from 'node:crypto'; +import { readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; +import path from 'node:path'; +import { vi } from 'vitest'; +import { buildManifest, ManifestInfo, saveManifest, signManifest } from './manifest.js'; +import { createPluginDistDir, createTempDir, DEFAULT_PLUGIN_JSON, writeFiles } from './tests/fixtures.js'; + +const mocks = vi.hoisted(() => { + return { + postData: vi.fn(), + }; +}); + +vi.mock('./request.js', () => { + return { postData: mocks.postData }; +}); + +function sha256(content: string) { + return crypto.createHash('sha256').update(content).digest('hex'); +} + +describe('buildManifest', () => { + const tempDirs: string[] = []; + + function pluginDistDir(files: Record = {}, pluginJson: object = DEFAULT_PLUGIN_JSON) { + const dir = createPluginDistDir(files, pluginJson); + tempDirs.push(dir); + return dir; + } + + afterAll(() => { + tempDirs.forEach((dir) => rmSync(dir, { recursive: true, force: true })); + }); + + it('should build a manifest with the plugin id, version and a sha256 hash per file', async () => { + const dir = pluginDistDir({ + 'module.js': 'console.log("hello");', + 'README.md': '# readme', + }); + + const manifest = await buildManifest(dir); + + expect(manifest.plugin).toBe('grafana-test-app'); + expect(manifest.version).toBe('1.0.0'); + expect(manifest.files).toEqual({ + 'plugin.json': sha256(JSON.stringify(DEFAULT_PLUGIN_JSON)), + 'module.js': sha256('console.log("hello");'), + 'README.md': sha256('# readme'), + }); + }); + + it('should include files in nested directories using posix paths', async () => { + const dir = pluginDistDir({ + 'img/logo.svg': '', + 'nested/deeply/file.txt': 'nested content', + }); + + const manifest = await buildManifest(dir); + + expect(manifest.files['img/logo.svg']).toBe(sha256('')); + expect(manifest.files['nested/deeply/file.txt']).toBe(sha256('nested content')); + expect(Object.keys(manifest.files).every((filePath) => !filePath.includes('\\'))).toBe(true); + }); + + it('should exclude an existing MANIFEST.txt from the manifest', async () => { + const dir = pluginDistDir({ + 'module.js': 'export {};', + 'MANIFEST.txt': 'a previously signed manifest', + }); + + const manifest = await buildManifest(dir); + + expect(manifest.files['MANIFEST.txt']).toBeUndefined(); + expect(manifest.files['module.js']).toBeDefined(); + }); + + it('should include symbolic links that resolve to files inside the plugin directory', async () => { + const dir = pluginDistDir({ + 'module.js': 'export {};', + }); + symlinkSync(path.join(dir, 'module.js'), path.join(dir, 'module-link.js')); + + const manifest = await buildManifest(dir); + + expect(manifest.files['module.js']).toBe(sha256('export {};')); + expect(manifest.files['module-link.js']).toBe(sha256('export {};')); + }); + + it('should throw when a symbolic link targets a file outside the plugin directory', async () => { + const outsideDir = createTempDir(); + tempDirs.push(outsideDir); + writeFiles(outsideDir, { 'secret.txt': 'secret' }); + + const dir = pluginDistDir(); + symlinkSync(path.join(outsideDir, 'secret.txt'), path.join(dir, 'secret-link.txt')); + + await expect(buildManifest(dir)).rejects.toThrow(/targets a file outside of the base directory/); + }); + + it('should throw when plugin.json is missing', async () => { + const dir = createTempDir(); + tempDirs.push(dir); + writeFileSync(path.join(dir, 'module.js'), 'export {};'); + + await expect(buildManifest(dir)).rejects.toThrow(); + }); +}); + +describe('signManifest', () => { + const manifest: ManifestInfo = { + plugin: 'grafana-test-app', + version: '1.0.0', + files: { 'plugin.json': 'hash' }, + }; + + beforeEach(() => { + vi.stubEnv('GRAFANA_COM_URL', undefined); + mocks.postData.mockReset(); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + }); + + it('should return the signed manifest when the API responds with status 200', async () => { + mocks.postData.mockResolvedValue({ status: 200, data: 'SIGNED-MANIFEST' }); + + const signedManifest = await signManifest(manifest, 'test-token'); + + expect(signedManifest).toBe('SIGNED-MANIFEST'); + expect(mocks.postData).toHaveBeenCalledWith('https://grafana.com/api/plugins/ci/sign', manifest, { + Authorization: 'Bearer test-token', + }); + }); + + it('should throw with the status code and server details when the API responds with an error', async () => { + mocks.postData.mockResolvedValue({ status: 400, data: JSON.stringify({ message: 'invalid plugin' }) }); + + await expect(signManifest(manifest, 'test-token')).rejects.toThrow( + 'Server responded with status code 400 along with: message: invalid plugin' + ); + }); + + it('should throw with the raw response body when the error response is not JSON', async () => { + mocks.postData.mockResolvedValue({ status: 500, data: 'Internal Server Error' }); + + await expect(signManifest(manifest, 'test-token')).rejects.toThrow( + 'Server responded with status code 500 along with: Internal Server Error' + ); + }); + + it('should propagate network errors', async () => { + mocks.postData.mockRejectedValue(new Error('socket hang up')); + + await expect(signManifest(manifest, 'test-token')).rejects.toThrow('socket hang up'); + }); +}); + +describe('saveManifest', () => { + it('should write the signed manifest to MANIFEST.txt', () => { + const dir = createTempDir(); + + const result = saveManifest(dir, 'SIGNED-MANIFEST'); + + expect(result).toBe(true); + expect(readFileSync(path.join(dir, 'MANIFEST.txt'), 'utf-8')).toBe('SIGNED-MANIFEST'); + rmSync(dir, { recursive: true, force: true }); + }); + + it('should throw when the directory is not writable', () => { + expect(() => saveManifest('/path/that/does/not/exist', 'SIGNED-MANIFEST')).toThrow( + 'Failed to write signed manifest to /path/that/does/not/exist.' + ); + }); +}); diff --git a/packages/sign-plugin/src/utils/manifest.ts b/packages/sign-plugin/src/utils/manifest.ts index 99e8de38af..a1243787cb 100644 --- a/packages/sign-plugin/src/utils/manifest.ts +++ b/packages/sign-plugin/src/utils/manifest.ts @@ -3,8 +3,6 @@ import { readFileSync, writeFileSync } from 'node:fs'; import fs from 'node:fs/promises'; import path from 'node:path'; import { postData } from './request.js'; -import { output } from './utils.output.js'; -import { styleText } from 'node:util'; const MANIFEST_FILE = 'MANIFEST.txt'; @@ -79,54 +77,28 @@ export async function buildManifest(dir: string): Promise { return manifest; } -export async function signManifest(manifest: ManifestInfo): Promise { - const GRAFANA_API_KEY = process.env.GRAFANA_API_KEY; - const GRAFANA_ACCESS_POLICY_TOKEN = process.env.GRAFANA_ACCESS_POLICY_TOKEN; - - if (!GRAFANA_ACCESS_POLICY_TOKEN && !GRAFANA_API_KEY) { - output.error({ - title: 'Missing GRAFANA_ACCESS_POLICY_TOKEN.', - body: ['You must create a GRAFANA_ACCESS_POLICY_TOKEN env variable to sign plugins.'], - link: 'https://grafana.com/developers/plugin-tools/publish-a-plugin/sign-a-plugin#generate-an-access-policy-token', - }); - process.exit(1); - } - if (GRAFANA_API_KEY) { - output.warning({ - title: 'Usage of GRAFANA_API_KEY is deprecated.', - body: ['Please migrate to using a GRAFANA_ACCESS_POLICY_TOKEN instead.'], - link: 'https://grafana.com/developers/plugin-tools/publish-a-plugin/sign-a-plugin', - }); - } - +export async function signManifest(manifest: ManifestInfo, token: string): Promise { const GRAFANA_COM_URL = process.env.GRAFANA_COM_URL || 'https://grafana.com/api'; const url = GRAFANA_COM_URL + '/plugins/ci/sign'; - const token = GRAFANA_ACCESS_POLICY_TOKEN ? GRAFANA_ACCESS_POLICY_TOKEN : GRAFANA_API_KEY; - try { - const info = await postData(url, manifest, { - Authorization: 'Bearer ' + token, - }); - if (info.status !== 200) { - const dataAsArray = Object.entries(JSON.parse(info.data)).map(([key, value]) => `${key}: ${value}`); - output.error({ - title: 'Error signing manifest.', - body: [ - `Server responded with status code ${styleText(['yellow'], info.status.toString())} along with:`, - ...output.bulletList(dataAsArray), - ], - }); - process.exit(1); - } + const info = await postData(url, manifest, { + Authorization: 'Bearer ' + token, + }); - return info.data; - } catch (err: any) { - const body = err.response?.data?.message ? [err.response.data.message] : [err.message]; - output.error({ - title: 'Error signing manifest.', - body, - }); - process.exit(1); + if (info.status !== 200) { + throw new Error(`Server responded with status code ${info.status} along with: ${formatServerError(info.data)}`); + } + + return info.data; +} + +function formatServerError(data: string): string { + try { + return Object.entries(JSON.parse(data)) + .map(([key, value]) => `${key}: ${value}`) + .join(', '); + } catch (err) { + return data; } } @@ -135,10 +107,6 @@ export function saveManifest(dir: string, signedManifest: string) { 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}.`); } } diff --git a/packages/sign-plugin/src/utils/pluginValidation.test.ts b/packages/sign-plugin/src/utils/pluginValidation.test.ts index e6fea66126..eeabbe4aea 100644 --- a/packages/sign-plugin/src/utils/pluginValidation.test.ts +++ b/packages/sign-plugin/src/utils/pluginValidation.test.ts @@ -1,4 +1,4 @@ -import { getPluginJson, validatePluginJson } from './pluginValidation.js'; +import { assertRootUrlIsValid, getPluginJson, validatePluginJson } from './pluginValidation.js'; describe('pluginValidation', () => { describe('plugin.json', () => { @@ -11,5 +11,43 @@ describe('pluginValidation', () => { test('missing "id" field in the plugin.json file', () => { expect(() => validatePluginJson({})).toThrow('Plugin id is missing in plugin.json'); }); + + test('missing "info" node in the plugin.json file', () => { + expect(() => validatePluginJson({ id: 'grafana-test-app' })).toThrow( + 'Plugin info node is missing in plugin.json' + ); + }); + + test('missing "info.version" field in the plugin.json file', () => { + expect(() => validatePluginJson({ id: 'grafana-test-app', info: {} })).toThrow( + 'Plugin info.version is missing in plugin.json' + ); + }); + + test('invalid plugin type', () => { + expect(() => + validatePluginJson({ id: 'grafana-test-app', info: { version: '1.0.0' }, type: 'renderer' }) + ).toThrow('Invalid plugin type in plugin.json: renderer'); + }); + + test('plugin id not ending with the plugin type', () => { + expect(() => + validatePluginJson({ id: 'grafana-test-datasource', info: { version: '1.0.0' }, type: 'app' }) + ).toThrow('[plugin.json] id should end with: -app'); + }); + + test.each(['app', 'datasource', 'panel'])('valid plugin.json with type %s', (type) => { + expect(() => validatePluginJson({ id: `grafana-test-${type}`, info: { version: '1.0.0' }, type })).not.toThrow(); + }); + }); + + describe('assertRootUrlIsValid', () => { + test('valid root URL', () => { + expect(() => assertRootUrlIsValid('https://example.com/grafana')).not.toThrow(); + }); + + test('invalid root URL', () => { + expect(() => assertRootUrlIsValid('not-a-valid-url')).toThrow('not-a-valid-url is not a valid URL'); + }); }); }); diff --git a/packages/sign-plugin/src/utils/tests/fixtures.ts b/packages/sign-plugin/src/utils/tests/fixtures.ts new file mode 100644 index 0000000000..5e462e46dd --- /dev/null +++ b/packages/sign-plugin/src/utils/tests/fixtures.ts @@ -0,0 +1,34 @@ +import { mkdirSync, mkdtempSync, realpathSync, writeFileSync } from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; + +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 os.tmpdir() +// returns /var/... which resolves to /private/var/... +export function createTempDir(): string { + return realpathSync(mkdtempSync(path.join(os.tmpdir(), 'sign-plugin-test-'))); +} + +export function writeFiles(dir: string, files: Record): void { + for (const [relativePath, content] of Object.entries(files)) { + const filePath = path.join(dir, relativePath); + mkdirSync(path.dirname(filePath), { recursive: true }); + writeFileSync(filePath, content); + } +} + +export function createPluginDistDir(files: Record = {}, pluginJson: object = DEFAULT_PLUGIN_JSON) { + const dir = createTempDir(); + writeFiles(dir, { + 'plugin.json': JSON.stringify(pluginJson), + ...files, + }); + return dir; +} From 349037fd7bccd7557f8ba9e16a3c26e63b7ba305 Mon Sep 17 00:00:00 2001 From: Timur Olzhabayev Date: Thu, 11 Jun 2026 11:39:40 +0200 Subject: [PATCH 2/4] fix(sign-plugin): address review feedback on symlink guard and token precedence The symlink escape check used a string prefix comparison, so a target in a sibling directory like -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. --- .../src/commands/sign.command.test.ts | 22 +++++++++++++++++++ .../sign-plugin/src/commands/sign.command.ts | 3 +-- .../sign-plugin/src/utils/manifest.test.ts | 12 ++++++++++ packages/sign-plugin/src/utils/manifest.ts | 6 ++++- 4 files changed, 40 insertions(+), 3 deletions(-) diff --git a/packages/sign-plugin/src/commands/sign.command.test.ts b/packages/sign-plugin/src/commands/sign.command.test.ts index 117d2b51de..d8e0631d86 100644 --- a/packages/sign-plugin/src/commands/sign.command.test.ts +++ b/packages/sign-plugin/src/commands/sign.command.test.ts @@ -126,6 +126,28 @@ describe('sign command', () => { expect(mocks.postData).not.toHaveBeenCalled(); }); + it('should report an invalid root URL before a missing token', async () => { + vi.stubEnv('GRAFANA_ACCESS_POLICY_TOKEN', undefined); + const dir = pluginDistDir(); + + await expect(sign(argvFor({ distDir: dir, rootUrls: 'not-a-valid-url' }))).rejects.toThrow(PROCESS_EXIT_MESSAGE); + + const stdout = stdoutSpy.mock.calls.map((call) => String(call[0])).join(''); + expect(stdout).toContain('not-a-valid-url is not a valid URL'); + expect(stdout).not.toContain('Missing GRAFANA_ACCESS_POLICY_TOKEN'); + }); + + it('should prefer GRAFANA_ACCESS_POLICY_TOKEN over GRAFANA_API_KEY but still warn about deprecation', async () => { + vi.stubEnv('GRAFANA_API_KEY', 'legacy-api-key'); + const dir = pluginDistDir(); + + await sign(argvFor({ distDir: dir })); + + const stdout = stdoutSpy.mock.calls.map((call) => String(call[0])).join(''); + expect(stdout).toContain('deprecated'); + expect(mocks.postData.mock.calls[0][2]).toEqual({ Authorization: 'Bearer test-token' }); + }); + it('should warn about deprecation but proceed when only GRAFANA_API_KEY is set', async () => { vi.stubEnv('GRAFANA_ACCESS_POLICY_TOKEN', undefined); vi.stubEnv('GRAFANA_API_KEY', 'legacy-api-key'); diff --git a/packages/sign-plugin/src/commands/sign.command.ts b/packages/sign-plugin/src/commands/sign.command.ts index d867ab636e..3db2cc1d6f 100644 --- a/packages/sign-plugin/src/commands/sign.command.ts +++ b/packages/sign-plugin/src/commands/sign.command.ts @@ -25,8 +25,6 @@ export const sign = async (argv: minimist.ParsedArgs) => { process.exit(1); } - const token = getSigningToken(); - try { output.log({ title: 'Signing plugin.', @@ -48,6 +46,7 @@ export const sign = async (argv: minimist.ParsedArgs) => { manifest.createPlugin = { version: createPluginVersion }; } + const token = getSigningToken(); const signedManifest = await signManifest(manifest, token); saveManifest(pluginDistDir, signedManifest); diff --git a/packages/sign-plugin/src/utils/manifest.test.ts b/packages/sign-plugin/src/utils/manifest.test.ts index eaaad6c8da..12f8d8a39f 100644 --- a/packages/sign-plugin/src/utils/manifest.test.ts +++ b/packages/sign-plugin/src/utils/manifest.test.ts @@ -97,6 +97,18 @@ describe('buildManifest', () => { await expect(buildManifest(dir)).rejects.toThrow(/targets a file outside of the base directory/); }); + it('should throw when a symbolic link targets a sibling directory sharing the plugin directory prefix', async () => { + const containerDir = createTempDir(); + tempDirs.push(containerDir); + const dir = path.join(containerDir, 'plugin'); + const siblingDir = path.join(containerDir, 'plugin-evil'); + writeFiles(dir, { 'plugin.json': JSON.stringify(DEFAULT_PLUGIN_JSON) }); + writeFiles(siblingDir, { 'secret.txt': 'secret' }); + symlinkSync(path.join(siblingDir, 'secret.txt'), path.join(dir, 'secret-link.txt')); + + await expect(buildManifest(dir)).rejects.toThrow(/targets a file outside of the base directory/); + }); + it('should throw when plugin.json is missing', async () => { const dir = createTempDir(); tempDirs.push(dir); diff --git a/packages/sign-plugin/src/utils/manifest.ts b/packages/sign-plugin/src/utils/manifest.ts index a1243787cb..38cf162f2c 100644 --- a/packages/sign-plugin/src/utils/manifest.ts +++ b/packages/sign-plugin/src/utils/manifest.ts @@ -35,7 +35,11 @@ async function* walk(dir: string, baseDir: string): RecursiveWalk { yield path.relative(baseDir, entry); } else if (d.isSymbolicLink()) { const realPath = await fs.realpath(entry); - if (!realPath.startsWith(baseDir)) { + // A prefix check would treat sibling paths like -evil as inside the base directory. + const relativeToBase = path.relative(baseDir, realPath); + const isOutsideBaseDir = + relativeToBase === '..' || relativeToBase.startsWith('..' + path.sep) || path.isAbsolute(relativeToBase); + if (isOutsideBaseDir) { throw new Error( `symbolic link ${path.relative(baseDir, entry)} targets a file outside of the base directory: ${baseDir}` ); From 2e98b5961d536983c296bfe3f748a446af7709a0 Mon Sep 17 00:00:00 2001 From: Timur Olzhabayev Date: Thu, 11 Jun 2026 15:35:44 +0200 Subject: [PATCH 3/4] test(sign-plugin): use tmp package for temp dirs in test fixtures --- package-lock.json | 14 +++++++++++++- packages/sign-plugin/package.json | 4 +++- packages/sign-plugin/src/utils/tests/fixtures.ts | 10 +++++----- 3 files changed, 21 insertions(+), 7 deletions(-) diff --git a/package-lock.json b/package-lock.json index d1a69b3d56..5e9ec2cec7 100644 --- a/package-lock.json +++ b/package-lock.json @@ -36913,12 +36913,24 @@ "@libs/find-up": "^1.0.0", "@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" }, "engines": { "node": ">=20" } }, + "packages/sign-plugin/node_modules/tmp": { + "version": "0.2.7", + "resolved": "https://registry.npmjs.org/tmp/-/tmp-0.2.7.tgz", + "integrity": "sha512-e0votIpp4Uo2AJYSzVHV6xCcawuiez3DzqDAbrTc3YxBkplN6e+dM13ZeIcZnDg/QpSuU2zfZ3rzwY8ukEnaXw==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">=14.14" + } + }, "packages/tsconfig": { "name": "@grafana/tsconfig", "version": "2.2.0", diff --git a/packages/sign-plugin/package.json b/packages/sign-plugin/package.json index 88613f871f..6754c30f59 100644 --- a/packages/sign-plugin/package.json +++ b/packages/sign-plugin/package.json @@ -33,7 +33,9 @@ "@libs/find-up": "^1.0.0", "@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" }, "engines": { "node": ">=20" diff --git a/packages/sign-plugin/src/utils/tests/fixtures.ts b/packages/sign-plugin/src/utils/tests/fixtures.ts index 5e462e46dd..d681a11a47 100644 --- a/packages/sign-plugin/src/utils/tests/fixtures.ts +++ b/packages/sign-plugin/src/utils/tests/fixtures.ts @@ -1,6 +1,6 @@ -import { mkdirSync, mkdtempSync, realpathSync, writeFileSync } from 'node:fs'; -import os from 'node:os'; +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', @@ -10,10 +10,10 @@ export const DEFAULT_PLUGIN_JSON = { }, }; -// realpathSync prevents false symlink-escape errors on macOS where os.tmpdir() -// returns /var/... which resolves to /private/var/... +// 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(mkdtempSync(path.join(os.tmpdir(), 'sign-plugin-test-'))); + return realpathSync(dirSync({ prefix: 'sign-plugin-test-', unsafeCleanup: true }).name); } export function writeFiles(dir: string, files: Record): void { From c1a24dab47f6077b856b9d21ebc92eca8e9d625e Mon Sep 17 00:00:00 2001 From: Timur Olzhabayev Date: Fri, 26 Jun 2026 12:20:16 +0200 Subject: [PATCH 4/4] fix(sign-plugin): preserve original error as cause in saveManifest 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. --- packages/sign-plugin/src/utils/manifest.test.ts | 8 +++++++- packages/sign-plugin/src/utils/manifest.ts | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/sign-plugin/src/utils/manifest.test.ts b/packages/sign-plugin/src/utils/manifest.test.ts index 12f8d8a39f..ea78332edd 100644 --- a/packages/sign-plugin/src/utils/manifest.test.ts +++ b/packages/sign-plugin/src/utils/manifest.test.ts @@ -169,14 +169,20 @@ describe('signManifest', () => { }); describe('saveManifest', () => { + const tempDirs: string[] = []; + + afterAll(() => { + tempDirs.forEach((dir) => rmSync(dir, { recursive: true, force: true })); + }); + it('should write the signed manifest to MANIFEST.txt', () => { const dir = createTempDir(); + tempDirs.push(dir); const result = saveManifest(dir, 'SIGNED-MANIFEST'); expect(result).toBe(true); expect(readFileSync(path.join(dir, 'MANIFEST.txt'), 'utf-8')).toBe('SIGNED-MANIFEST'); - rmSync(dir, { recursive: true, force: true }); }); it('should throw when the directory is not writable', () => { diff --git a/packages/sign-plugin/src/utils/manifest.ts b/packages/sign-plugin/src/utils/manifest.ts index 38cf162f2c..87185ef73f 100644 --- a/packages/sign-plugin/src/utils/manifest.ts +++ b/packages/sign-plugin/src/utils/manifest.ts @@ -111,6 +111,6 @@ export function saveManifest(dir: string, signedManifest: string) { writeFileSync(path.join(dir, MANIFEST_FILE), signedManifest); return true; } catch (error) { - throw new Error(`Failed to write signed manifest to ${dir}.`); + throw new Error(`Failed to write signed manifest to ${dir}.`, { cause: error }); } }