-
Notifications
You must be signed in to change notification settings - Fork 51
fix(sign-plugin): prevent symlink escape check bypass and add test coverage #2721
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
tolzhabayev
wants to merge
8
commits into
main
Choose a base branch
from
test/sign-plugin-coverage
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
912bca1
test(sign-plugin): add manifest and sign command coverage
tolzhabayev 349037f
fix(sign-plugin): address review feedback on symlink guard and token …
tolzhabayev 2e98b59
test(sign-plugin): use tmp package for temp dirs in test fixtures
tolzhabayev c1a24da
fix(sign-plugin): preserve original error as cause in saveManifest
tolzhabayev 694516b
Merge branch 'main' into test/sign-plugin-coverage
tolzhabayev 03e683f
fix(sign-plugin): canonicalize base dir and harden server error forma…
tolzhabayev 7ebaeca
fix(sign-plugin): correct typo in signing success message
tolzhabayev 692f7f6
Merge branch 'main' into test/sign-plugin-coverage
tolzhabayev File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,194 @@ | ||
| 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<string, unknown> = {}): 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<typeof process.stdout.write>; | ||
| let exitSpy: MockInstance<typeof process.exit>; | ||
|
|
||
| function pluginDistDir(files: Record<string, string> = {}) { | ||
| 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 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'); | ||
| 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' }); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
41 changes: 41 additions & 0 deletions
41
packages/sign-plugin/src/utils/getCreatePluginVersion.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<string, string> = {}) { | ||
| 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(); | ||
| }); | ||
| }); |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.