From f0f126e2d14b3f0ce1c895336578c2edb1676c3e Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Sat, 1 Aug 2026 14:16:26 +0000 Subject: [PATCH 1/3] fix(ci): validate untrusted workflow identifiers --- scripts/ci/test_validate_coverage_artifact.py | 48 ++++++++-- scripts/ci/test_validate_workflow_run.cjs | 94 +++++++++++-------- scripts/ci/validate-coverage-artifact.py | 4 +- scripts/ci/validate-workflow-run.cjs | 38 +++++--- 4 files changed, 124 insertions(+), 60 deletions(-) diff --git a/scripts/ci/test_validate_coverage_artifact.py b/scripts/ci/test_validate_coverage_artifact.py index e162cd6..4a460c1 100644 --- a/scripts/ci/test_validate_coverage_artifact.py +++ b/scripts/ci/test_validate_coverage_artifact.py @@ -24,12 +24,13 @@ def setUp(self): (self.root / "src").mkdir() (self.root / "internal").mkdir() (self.root / "src/a.ts").write_text("export const a = 1;\n") + (self.root / "-tracked.ts").write_text("export const tracked = true;\n") (self.root / "internal/a.go").write_text("package internal\n") (self.root / "go.mod").write_text("module github.com/RandomCodeSpace/kb\n\ngo 1.24\n") subprocess.run(["git", "init", "-q", self.root], check=True) subprocess.run(["git", "-C", self.root, "config", "user.email", "ci@example.invalid"], check=True) subprocess.run(["git", "-C", self.root, "config", "user.name", "CI"], check=True) - subprocess.run(["git", "-C", self.root, "add", "src/a.ts", "internal/a.go", "go.mod"], check=True) + subprocess.run(["git", "-C", self.root, "add", "--", "src/a.ts", "-tracked.ts", "internal/a.go", "go.mod"], check=True) subprocess.run(["git", "-C", self.root, "commit", "-qm", "fixture"], check=True) self.base = subprocess.check_output(["git", "-C", self.root, "rev-parse", "HEAD"], text=True).strip() self.lcov = b"TN:\nSF:src/a.ts\nDA:1,1\nend_of_record\n" @@ -84,17 +85,18 @@ def archive(self, *, lcov=None, go=None, manifest_bytes=None, symlink=False): archive.writestr("manifest.json", manifest_bytes) return path - def run_validator(self, archive, output=None): + def run_validator(self, archive, output=None, *, candidate_sha=SHA, base_sha=None, event="pull_request", pull_request=7): output = output or self.root / f"out-{os.urandom(4).hex()}" + base_sha = self.base if base_sha is None else base_sha args = [ "python3", str(SCRIPT), "--archive", str(archive), "--output", str(output), "--repository", "RandomCodeSpace/kb", "--workflow", "Regression and candidate coverage", "--workflow-ref", "RandomCodeSpace/kb/.github/workflows/quality.yml@refs/pull/7/merge", "--workflow-sha", WORKFLOW_SHA, "--test-revision-sha", WORKFLOW_SHA, - "--run-id", "10", "--run-attempt", "1", "--event", "pull_request", - "--candidate-repository", "RandomCodeSpace/kb", "--candidate-sha", SHA, - "--candidate-tree", TREE, "--candidate-ref", "feature", "--pull-request", "7", - "--base-sha", self.base, "--security-base-sha", self.base, "--base-ref", "main", "--repository-root", str(self.root), + "--run-id", "10", "--run-attempt", "1", "--event", event, + "--candidate-repository", "RandomCodeSpace/kb", f"--candidate-sha={candidate_sha}", + "--candidate-tree", TREE, "--candidate-ref", "feature", "--pull-request", str(pull_request), + f"--base-sha={base_sha}", "--security-base-sha", self.base, "--base-ref", "main", "--repository-root", str(self.root), "--maintenance", "false", ] return subprocess.run(args, text=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) @@ -102,6 +104,40 @@ def run_validator(self, archive, output=None): def test_accepts_exact_bundle(self): self.assertEqual(self.run_validator(self.archive()).returncode, 0) + def test_accepts_tracked_source_with_option_like_name(self): + lcov = b"TN:\nSF:-tracked.ts\nDA:1,1\nend_of_record\n" + self.assertEqual(self.run_validator(self.archive(lcov=lcov)).returncode, 0) + + def test_accepts_valid_push_revision_boundary(self): + manifest = self.manifest() + manifest["producer"]["event"] = "push" + manifest["candidate"]["sha"] = self.base + manifest["candidate"]["pull_request"] = 0 + result = self.run_validator( + self.archive(manifest_bytes=json.dumps(manifest).encode()), + candidate_sha=self.base, + event="push", + pull_request=0, + ) + self.assertEqual(result.returncode, 0, result.stderr) + + def test_rejects_option_like_traversal_and_control_git_revisions(self): + for field, value in ( + ("sha", "--help"), + ("base_sha", "../HEAD"), + ("sha", "1" * 39 + "\n"), + ): + with self.subTest(field=field, value=value): + manifest = self.manifest() + manifest["candidate"][field] = value + result = self.run_validator( + self.archive(manifest_bytes=json.dumps(manifest).encode()), + candidate_sha=value if field == "sha" else SHA, + base_sha=value if field == "base_sha" else self.base, + ) + self.assertNotEqual(result.returncode, 0) + self.assertIn("must be a lowercase 40-character Git SHA", result.stderr) + def test_rejects_traversal_and_url_sources(self): for source in ("../src/a.ts", "/src/a.ts", "https://evil.invalid/a.ts", "src//a.ts"): with self.subTest(source=source): diff --git a/scripts/ci/test_validate_workflow_run.cjs b/scripts/ci/test_validate_workflow_run.cjs index 5c02609..15c2cf4 100644 --- a/scripts/ci/test_validate_workflow_run.cjs +++ b/scripts/ci/test_validate_workflow_run.cjs @@ -68,7 +68,7 @@ const server = createServer((request, response) => { response.end(JSON.stringify(fixture || { message: 'not found' })); }); -server.listen(0, '127.0.0.1', () => { +function runValidator(envOverrides = {}) { const child = spawn(process.execPath, [join(__dirname, 'validate-workflow-run.cjs')], { env: { ...process.env, @@ -82,50 +82,64 @@ server.listen(0, '127.0.0.1', () => { TRIGGER_RUN_ATTEMPT: '1', TRIGGER_HEAD_REPOSITORY_ID: '99', TRIGGER_HEAD_SHA: head, + ...envOverrides, }, stdio: ['ignore', 'pipe', 'pipe'], }); let stderr = ''; child.stderr.on('data', (chunk) => { stderr += chunk; }); - child.on('close', (code) => { - assert.equal(code, 0, stderr); - const output = readFileSync(outputPath, 'utf8'); - assert.match(output, new RegExp(`candidate_sha=${head}`)); - assert.match(output, /workflow_ref=RandomCodeSpace\/kb\/\.github\/workflows\/quality\.yml@refs\/pull\/7\/merge/); - assert.match(output, new RegExp(`workflow_sha=${merge}`)); - assert.match(output, /pull_request=7/); - assert.match(output, new RegExp(`base_sha=${base}`)); - assert.match(output, /candidate_repository=ExampleContributor\/kb/); - const requestCount = requests.length; - event.workflow_run.id = runId + 1; - writeFileSync(eventPath, JSON.stringify(event)); - const hostile = spawn(process.execPath, [join(__dirname, 'validate-workflow-run.cjs')], { - env: { - ...process.env, - GITHUB_API_URL: `http://127.0.0.1:${server.address().port}`, - GITHUB_EVENT_PATH: eventPath, - GITHUB_OUTPUT: outputPath, - GITHUB_REPOSITORY: repository, - GITHUB_TOKEN: 'test-token', - TRIGGER_RUN_ID: String(runId), - TRIGGER_WORKFLOW_ID: '5', - TRIGGER_RUN_ATTEMPT: '1', - TRIGGER_HEAD_REPOSITORY_ID: '99', - TRIGGER_HEAD_SHA: head, - }, - stdio: ['ignore', 'pipe', 'pipe'], - }); - let hostileStderr = ''; - hostile.stderr.on('data', (chunk) => { hostileStderr += chunk; }); - hostile.on('close', async (hostileCode) => { - assert.notEqual(hostileCode, 0); - assert.match(hostileStderr, /trusted workflow run id mismatch/); - assert.equal(requests.length, requestCount, 'mismatched event id must not reach the network'); - await testManifestDescriptorReads(); - await testSonarOriginPinning(); - server.close(() => console.log('CI JavaScript hostile fixtures passed')); - }); - }); + return new Promise((resolve) => child.on('close', (code) => resolve({ code, stderr }))); +} + +async function runWorkflowTests() { + const valid = await runValidator(); + assert.equal(valid.code, 0, valid.stderr); + const output = readFileSync(outputPath, 'utf8'); + assert.match(output, new RegExp(`candidate_sha=${head}`)); + assert.match(output, /workflow_ref=RandomCodeSpace\/kb\/\.github\/workflows\/quality\.yml@refs\/pull\/7\/merge/); + assert.match(output, new RegExp(`workflow_sha=${merge}`)); + assert.match(output, /pull_request=7/); + assert.match(output, new RegExp(`base_sha=${base}`)); + assert.match(output, /candidate_repository=ExampleContributor\/kb/); + + const requestCount = requests.length; + const hostileEnvironmentCases = [ + [{ TRIGGER_RUN_ID: `${runId}/../admin` }, /TRIGGER_RUN_ID must be a positive integer/], + [{ TRIGGER_RUN_ID: '9007199254740992' }, /TRIGGER_RUN_ID must be a positive safe integer/], + [{ GITHUB_REPOSITORY: 'RandomCodeSpace/kb/../admin' }, /GITHUB_REPOSITORY is invalid/], + ]; + for (const [envOverrides, expectedError] of hostileEnvironmentCases) { + const hostile = await runValidator(envOverrides); + assert.notEqual(hostile.code, 0); + assert.match(hostile.stderr, expectedError); + assert.equal(requests.length, requestCount, 'invalid API path identifier must not reach the network'); + } + + event.workflow_run.head_repository.full_name = 'ExampleContributor/kb/../../admin'; + writeFileSync(eventPath, JSON.stringify(event)); + const injectedRepository = await runValidator(); + assert.notEqual(injectedRepository.code, 0); + assert.match(injectedRepository.stderr, /candidate repository is invalid/); + assert.equal(requests.length, requestCount, 'injected repository path must not reach the network'); + + event.workflow_run.head_repository.full_name = candidateRepository; + event.workflow_run.id = runId + 1; + writeFileSync(eventPath, JSON.stringify(event)); + const mismatchedRun = await runValidator(); + assert.notEqual(mismatchedRun.code, 0); + assert.match(mismatchedRun.stderr, /trusted workflow run id mismatch/); + assert.equal(requests.length, requestCount, 'mismatched event id must not reach the network'); +} + +server.listen(0, '127.0.0.1', () => { + runWorkflowTests() + .then(testManifestDescriptorReads) + .then(testSonarOriginPinning) + .then(() => server.close(() => console.log('CI JavaScript hostile fixtures passed'))) + .catch((error) => server.close(() => { + console.error(error); + process.exitCode = 1; + })); }); async function testManifestDescriptorReads() { diff --git a/scripts/ci/validate-coverage-artifact.py b/scripts/ci/validate-coverage-artifact.py index f0b080f..f4af115 100644 --- a/scripts/ci/validate-coverage-artifact.py +++ b/scripts/ci/validate-coverage-artifact.py @@ -255,14 +255,14 @@ def unique_object(pairs): manifest_base = manifest["candidate"]["base_sha"] if manifest_base != empty_tree: base_exists = subprocess.run( - ["git", "-C", str(args.repository_root), "cat-file", "-e", f"{manifest_base}^{{commit}}"], + ["git", "-C", str(args.repository_root), "cat-file", "-e", "--end-of-options", f"{manifest_base}^{{commit}}"], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, check=False, ) if base_exists.returncode != 0: fail("candidate event base is not an available commit") if args.event != "pull_request": ancestor = subprocess.run( - ["git", "-C", str(args.repository_root), "merge-base", "--is-ancestor", manifest_base, args.candidate_sha], + ["git", "-C", str(args.repository_root), "merge-base", "--is-ancestor", "--end-of-options", manifest_base, args.candidate_sha], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, check=False, ) if ancestor.returncode != 0: diff --git a/scripts/ci/validate-workflow-run.cjs b/scripts/ci/validate-workflow-run.cjs index e4c0cae..86542ff 100644 --- a/scripts/ci/validate-workflow-run.cjs +++ b/scripts/ci/validate-workflow-run.cjs @@ -37,9 +37,24 @@ function repositoryName(value, label) { return value; } -async function github(path) { - const apiUrl = process.env.GITHUB_API_URL || 'https://api.github.com'; - const response = await fetch(`${apiUrl}${path}`, { +function githubPathIdentifier(value, label) { + if (typeof value === 'number') return String(safeInteger(value, label)); + if (typeof value !== 'string' || !value || value.length > 100 || !/^[A-Za-z0-9_.-]+$/.test(value)) { + throw new Error(`${label} is invalid`); + } + return encodeURIComponent(value); +} + +async function github(segments, query = {}) { + if (!Array.isArray(segments) || !segments.length) throw new Error('GitHub API path is invalid'); + const apiUrl = new URL(process.env.GITHUB_API_URL || 'https://api.github.com'); + if (!['http:', 'https:'].includes(apiUrl.protocol) || apiUrl.username || apiUrl.password || apiUrl.search || apiUrl.hash) { + throw new Error('GITHUB_API_URL is invalid'); + } + const path = segments.map((segment, index) => githubPathIdentifier(segment, `GitHub API path segment ${index + 1}`)).join('/'); + apiUrl.pathname = `${apiUrl.pathname.replace(/\/+$/, '')}/${path}`; + for (const [key, value] of Object.entries(query)) apiUrl.searchParams.set(key, String(value)); + const response = await fetch(apiUrl, { headers: { Accept: 'application/vnd.github+json', Authorization: `Bearer ${required('GITHUB_TOKEN')}`, @@ -60,7 +75,7 @@ function output(values) { async function main() { const event = JSON.parse(readFileSync(required('GITHUB_EVENT_PATH'), 'utf8')); - const repository = required('GITHUB_REPOSITORY'); + const repository = repositoryName(required('GITHUB_REPOSITORY'), 'GITHUB_REPOSITORY'); const triggerRunId = environmentInteger('TRIGGER_RUN_ID'); const triggerWorkflowId = environmentInteger('TRIGGER_WORKFLOW_ID'); const triggerRunAttempt = environmentInteger('TRIGGER_RUN_ATTEMPT'); @@ -83,8 +98,8 @@ async function main() { equal(safeInteger(trigger.head_repository.id, 'head repository id'), triggerHeadRepositoryId, 'trusted head repository id'); equal(trigger.head_sha, triggerHeadSha, 'trusted head SHA'); - const encodedRepo = repository.split('/').map(encodeURIComponent).join('/'); - const run = await github(`/repos/${encodedRepo}/actions/runs/${triggerRunId}`); + const repositorySegments = repository.split('/'); + const run = await github(['repos', ...repositorySegments, 'actions', 'runs', triggerRunId]); equal(run.id, triggerRunId, 'API run id'); equal(run.workflow_id, triggerWorkflowId, 'API workflow id'); equal(run.name, WORKFLOW_NAME, 'API workflow name'); @@ -96,12 +111,12 @@ async function main() { equal(run.head_sha, triggerHeadSha, 'API head SHA'); equal(run.run_attempt, triggerRunAttempt, 'API run attempt'); const workflowFile = WORKFLOW_PATH.split('/').at(-1); - const workflow = await github(`/repos/${encodedRepo}/actions/workflows/${encodeURIComponent(workflowFile)}`); + const workflow = await github(['repos', ...repositorySegments, 'actions', 'workflows', workflowFile]); equal(workflow.id, triggerWorkflowId, 'workflow-by-path id'); equal(workflow.path, WORKFLOW_PATH, 'workflow-by-path path'); equal(workflow.name, WORKFLOW_NAME, 'workflow-by-path name'); - const jobs = await github(`/repos/${encodedRepo}/actions/runs/${triggerRunId}/jobs?filter=latest&per_page=100`); + const jobs = await github(['repos', ...repositorySegments, 'actions', 'runs', triggerRunId, 'jobs'], { filter: 'latest', per_page: 100 }); const jobMatches = jobs.jobs.filter((job) => job.name === JOB_NAME && job.run_attempt === triggerRunAttempt); if (jobMatches.length !== 1) throw new Error(`expected exactly one ${JOB_NAME} job, found ${jobMatches.length}`); const job = jobMatches[0]; @@ -111,7 +126,7 @@ async function main() { equal(job.conclusion, 'success', 'producer job conclusion'); equal(job.head_sha, triggerHeadSha, 'producer job head SHA'); - const artifacts = await github(`/repos/${encodedRepo}/actions/runs/${triggerRunId}/artifacts?per_page=100`); + const artifacts = await github(['repos', ...repositorySegments, 'actions', 'runs', triggerRunId, 'artifacts'], { per_page: 100 }); equal(artifacts.total_count, 1, 'artifact count'); equal(artifacts.artifacts.length, 1, 'returned artifact count'); const artifact = artifacts.artifacts[0]; @@ -125,8 +140,7 @@ async function main() { equal(artifact.workflow_run?.head_sha, triggerHeadSha, 'artifact head SHA'); equal(artifact.workflow_run?.head_repository_id, triggerHeadRepositoryId, 'artifact head repository id'); - const encodedCandidateRepo = candidateRepository.split('/').map(encodeURIComponent).join('/'); - const commit = await github(`/repos/${encodedCandidateRepo}/git/commits/${triggerHeadSha}`); + const commit = await github(['repos', ...candidateRepository.split('/'), 'git', 'commits', triggerHeadSha]); equal(commit.sha, triggerHeadSha, 'candidate commit SHA'); const candidateTree = commit.tree?.sha; if (!/^[0-9a-f]{40}$/.test(candidateTree || '')) throw new Error('candidate tree is invalid'); @@ -149,7 +163,7 @@ async function main() { baseSha = pull.base?.sha; baseRef = pull.base?.ref; candidateRef = pull.head?.ref; - const pullDetails = await github(`/repos/${encodedRepo}/pulls/${pull.number}`); + const pullDetails = await github(['repos', ...repositorySegments, 'pulls', pull.number]); equal(pullDetails.head?.sha, triggerHeadSha, 'pull request API head SHA'); equal(pullDetails.head?.repo?.full_name, candidateRepository, 'pull request API head repository'); equal(pullDetails.base?.repo?.full_name, repository, 'pull request API base repository'); From 1624743f20235af45c3250017b451a41f4333527 Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Sat, 1 Aug 2026 14:47:52 +0000 Subject: [PATCH 2/3] fix(ci): verify Sonar compute task revision --- .../protected-sonar/verify-sonar-task.cjs | 62 ++++++-- .../verify-sonar-task.test.cjs | 144 ++++++++++++++++++ .github/workflows/quality.yml | 1 + scripts/ci/test_validate_workflow_run.cjs | 12 +- scripts/ci/test_workflow_structure.py | 6 + 5 files changed, 209 insertions(+), 16 deletions(-) create mode 100644 .github/actions/protected-sonar/verify-sonar-task.test.cjs diff --git a/.github/actions/protected-sonar/verify-sonar-task.cjs b/.github/actions/protected-sonar/verify-sonar-task.cjs index b79b230..6077e08 100644 --- a/.github/actions/protected-sonar/verify-sonar-task.cjs +++ b/.github/actions/protected-sonar/verify-sonar-task.cjs @@ -30,6 +30,25 @@ async function sonar(url) { return response.json(); } +function scannerRevision(scannerContext) { + if (typeof scannerContext !== 'string' || /[\0\r]/.test(scannerContext)) { + throw new Error('compute-engine scanner context is missing or malformed'); + } + const property = /^(?: - )?sonar\.scm\.revision=(.*)$/; + const revisions = scannerContext + .split('\n') + .map((line) => line.match(property)) + .filter(Boolean) + .map((match) => match[1]); + if (revisions.length !== 1) { + throw new Error(`compute-engine scanner context contains ${revisions.length} revision properties`); + } + if (!/^[0-9a-f]{40}$/.test(revisions[0])) { + throw new Error('compute-engine scanner revision is malformed'); + } + return revisions[0]; +} + async function main() { const candidateSha = required('CANDIDATE_SHA'); if (!/^[0-9a-f]{40}$/.test(candidateSha)) throw new Error('candidate SHA is invalid'); @@ -40,7 +59,14 @@ async function main() { } const ceTaskId = report.get('ceTaskId'); if (!/^[A-Za-z0-9_-]+$/.test(ceTaskId || '')) throw new Error('invalid compute-engine task id'); - const task = await sonar(new URL(`/api/ce/task?id=${encodeURIComponent(ceTaskId)}`, SONAR_ORIGIN)); + const analysisMode = required('ANALYSIS_MODE'); + if (!['pull_request', 'branch', 'main'].includes(analysisMode)) { + throw new Error(`unsupported analysis mode ${analysisMode}`); + } + const taskQuery = new URL('/api/ce/task', SONAR_ORIGIN); + taskQuery.searchParams.set('id', ceTaskId); + taskQuery.searchParams.set('additionalFields', 'scannerContext'); + const task = await sonar(taskQuery); if (task.task?.status !== 'SUCCESS' || !task.task.analysisId) { throw new Error(`compute-engine task is not successful: ${task.task?.status || 'missing'}`); } @@ -48,16 +74,28 @@ async function main() { throw new Error(`compute-engine component ${task.task.componentKey} != ${required('SONAR_PROJECT_KEY')}`); } - const query = new URL('/api/project_analyses/search', SONAR_ORIGIN); - query.searchParams.set('project', required('SONAR_PROJECT_KEY')); - query.searchParams.set('pageSize', '100'); - if (required('ANALYSIS_MODE') === 'pull_request') query.searchParams.set('pullRequest', required('PULL_REQUEST')); - if (required('ANALYSIS_MODE') === 'branch') query.searchParams.set('branch', required('CANDIDATE_REF')); - const analyses = await sonar(query); - const analysis = analyses.analyses?.find((item) => item.key === task.task.analysisId); - if (!analysis) throw new Error(`analysis ${task.task.analysisId} not returned by project analysis API`); - if (analysis.revision !== candidateSha) throw new Error(`analysis revision ${analysis.revision} != ${candidateSha}`); - console.log(`verified Sonar task ${ceTaskId}, analysis ${analysis.key}, revision ${analysis.revision}`); + if (analysisMode === 'pull_request') { + const pullRequest = required('PULL_REQUEST'); + if (!/^[1-9][0-9]*$/.test(pullRequest)) throw new Error('pull request number is invalid'); + if (String(task.task.pullRequest) !== pullRequest) { + throw new Error(`compute-engine pull request ${task.task.pullRequest || 'missing'} != ${pullRequest}`); + } + } else { + if (task.task.pullRequest !== undefined && task.task.pullRequest !== null) { + throw new Error(`unexpected compute-engine pull request ${task.task.pullRequest}`); + } + const candidateRef = required('CANDIDATE_REF'); + if (analysisMode === 'branch' && task.task.branch !== candidateRef) { + throw new Error(`compute-engine branch ${task.task.branch || 'missing'} != ${candidateRef}`); + } + if (analysisMode === 'main' && task.task.branch !== undefined && task.task.branch !== null && task.task.branch !== candidateRef) { + throw new Error(`compute-engine branch ${task.task.branch} != ${candidateRef}`); + } + } + + const revision = scannerRevision(task.task.scannerContext); + if (revision !== candidateSha) throw new Error(`compute-engine scanner revision ${revision} != ${candidateSha}`); + console.log(`verified Sonar task ${ceTaskId}, analysis ${task.task.analysisId}, revision ${revision}`); } if (require.main === module) { @@ -67,4 +105,4 @@ if (require.main === module) { }); } -module.exports = { main }; +module.exports = { main, scannerRevision }; diff --git a/.github/actions/protected-sonar/verify-sonar-task.test.cjs b/.github/actions/protected-sonar/verify-sonar-task.test.cjs new file mode 100644 index 0000000..7fe51ca --- /dev/null +++ b/.github/actions/protected-sonar/verify-sonar-task.test.cjs @@ -0,0 +1,144 @@ +#!/usr/bin/env node + +const assert = require('node:assert/strict'); +const { mkdirSync, mkdtempSync, writeFileSync } = require('node:fs'); +const { tmpdir } = require('node:os'); +const { join } = require('node:path'); + +const verifier = require('./verify-sonar-task.cjs'); + +const candidateSha = '0123456789abcdef0123456789abcdef01234567'; +const projectKey = 'RandomCodeSpace_kb'; +const project = mkdtempSync(join(tmpdir(), 'verify-sonar-task-')); +mkdirSync(join(project, '.scannerwork')); +writeFileSync( + join(project, '.scannerwork/report-task.txt'), + 'serverUrl=https://sonarcloud.io\nceTaskId=task-1\n', +); + +Object.assign(process.env, { + ANALYSIS_MODE: 'pull_request', + CANDIDATE_REF: 'feature', + CANDIDATE_SHA: candidateSha, + PROJECT_BASE_DIR: project, + PULL_REQUEST: '18', + SONAR_PROJECT_KEY: projectKey, + SONAR_TOKEN: 'test-token', +}); + +function response(body) { + return { ok: true, json: async () => body }; +} + +function ceTask(overrides = {}) { + return { + task: { + analysisId: 'analysis-1', + componentKey: projectKey, + pullRequest: '18', + scannerContext: [ + 'Scanner properties:', + ` - sonar.projectKey=${projectKey}`, + ` - sonar.scm.revision=${candidateSha}`, + ' - sonar.pullrequest.key=18', + '', + ].join('\n'), + status: 'SUCCESS', + ...overrides, + }, + }; +} + +async function runTask(task) { + const seen = []; + global.fetch = async (url) => { + seen.push(new URL(url)); + return response(task); + }; + await verifier.main(); + assert.equal(seen.length, 1); + assert.equal(seen[0].pathname, '/api/ce/task'); + assert.equal(seen[0].searchParams.get('id'), 'task-1'); + assert.equal(seen[0].searchParams.get('additionalFields'), 'scannerContext'); +} + +async function rejects(task, pattern) { + global.fetch = async () => response(task); + await assert.rejects(verifier.main(), pattern); +} + +async function main() { + const originalFetch = global.fetch; + const originalLog = console.log; + try { + const messages = []; + console.log = (message) => messages.push(message); + + await runTask(ceTask()); + assert.equal(messages.length, 1); + assert.doesNotMatch(messages[0], /sonar\.projectKey|scannerContext/); + + await rejects(ceTask({ pullRequest: '19' }), /compute-engine pull request 19 != 18/); + await rejects( + ceTask({ scannerContext: 'sonar.scm.revision=ffffffffffffffffffffffffffffffffffffffff\n' }), + /scanner revision .* !=/, + ); + await rejects( + ceTask({ scannerContext: `sonar.scm.revision=${candidateSha}\n - sonar.scm.revision=${candidateSha}\n` }), + /contains 2 revision properties/, + ); + await rejects( + ceTask({ scannerContext: `sonar.projectKey=${projectKey}\n` }), + /contains 0 revision properties/, + ); + await rejects( + ceTask({ scannerContext: `sonar.scm.revision=${candidateSha}%0Asonar.scm.revision=${candidateSha}\n` }), + /scanner revision is malformed/, + ); + await rejects( + ceTask({ scannerContext: `sonar.scm.revision=${candidateSha}\r\n` }), + /scanner context is missing or malformed/, + ); + for (const prefix of ['- ', ' - ', '\t-\t', ' -- ', ' -', ' + ', ' * ', ' - - ', ' ']) { + await rejects( + ceTask({ scannerContext: `${prefix}sonar.scm.revision=${candidateSha}\n` }), + /contains 0 revision properties/, + ); + } + await rejects( + ceTask({ scannerContext: ` - attacker.sonar.scm.revision=${candidateSha}\n` }), + /contains 0 revision properties/, + ); + await rejects(ceTask({ status: 'FAILED' }), /compute-engine task is not successful: FAILED/); + + process.env.ANALYSIS_MODE = 'branch'; + process.env.CANDIDATE_REF = 'fix/sonar-security-batch-1'; + const seen = []; + global.fetch = async (url) => { + const parsed = new URL(url); + seen.push(parsed); + return response(ceTask({ branch: 'fix/sonar-security-batch-1', pullRequest: undefined })); + }; + await verifier.main(); + assert.equal(seen.length, 1); + assert.equal(seen[0].searchParams.get('additionalFields'), 'scannerContext'); + await rejects( + ceTask({ branch: 'fix/other-branch', pullRequest: undefined }), + /compute-engine branch fix\/other-branch != fix\/sonar-security-batch-1/, + ); + await rejects( + ceTask({ branch: undefined, pullRequest: undefined }), + /compute-engine branch missing != fix\/sonar-security-batch-1/, + ); + + originalLog('verify-sonar-task hostile tests passed'); + } finally { + global.fetch = originalFetch; + console.log = originalLog; + } +} + +main().catch((error) => { + console.error(error); + process.exitCode = 1; +}); diff --git a/.github/workflows/quality.yml b/.github/workflows/quality.yml index d5f2f61..518fc5f 100644 --- a/.github/workflows/quality.yml +++ b/.github/workflows/quality.yml @@ -125,6 +125,7 @@ jobs: python3 scripts/ci/test_workflow_structure.py node scripts/ci/test_validate_workflow_run.cjs node scripts/ci/test_ci_monitor.cjs + node .github/actions/protected-sonar/verify-sonar-task.test.cjs candidate_coverage: name: Candidate head coverage diff --git a/scripts/ci/test_validate_workflow_run.cjs b/scripts/ci/test_validate_workflow_run.cjs index 15c2cf4..7520d0a 100644 --- a/scripts/ci/test_validate_workflow_run.cjs +++ b/scripts/ci/test_validate_workflow_run.cjs @@ -202,15 +202,19 @@ async function testSonarOriginPinning() { global.fetch = async (url) => { const parsed = new URL(url); seen.push(parsed.href); - const body = parsed.pathname === '/api/ce/task' - ? { task: { status: 'SUCCESS', analysisId: 'analysis-1', componentKey: process.env.SONAR_PROJECT_KEY } } - : { analyses: [{ key: 'analysis-1', revision: head }] }; + const body = { task: { + status: 'SUCCESS', + analysisId: 'analysis-1', + branch: process.env.CANDIDATE_REF, + componentKey: process.env.SONAR_PROJECT_KEY, + scannerContext: `sonar.scm.revision=${head}\n`, + } }; return { ok: true, json: async () => body }; }; const { main: verifySonarTask } = require('../../.github/actions/protected-sonar/verify-sonar-task.cjs'); writeFileSync(reportPath, 'serverUrl=https://sonarcloud.io/untrusted/path\nceTaskId=task-1\n'); await verifySonarTask(); - assert.equal(seen.length, 2); + assert.equal(seen.length, 1); assert.ok(seen.every((url) => new URL(url).origin === 'https://sonarcloud.io')); const requestCount = seen.length; diff --git a/scripts/ci/test_workflow_structure.py b/scripts/ci/test_workflow_structure.py index 189c646..c471dcc 100644 --- a/scripts/ci/test_workflow_structure.py +++ b/scripts/ci/test_workflow_structure.py @@ -67,6 +67,12 @@ def test_candidate_coverage_has_no_secret_context(self): self.assertIn("repository: ${{ github.event.pull_request.head.repo.full_name || github.repository }}", job) self.assertIn("ref: ${{ github.event.pull_request.head.sha || github.sha }}", job) + def test_sonar_verifier_hostile_fixture_runs_in_ci(self): + self.assertIn( + "node .github/actions/protected-sonar/verify-sonar-task.test.cjs", + QUALITY, + ) + def test_branch_and_pr_control_plane_classification_match(self): branch = SONAR.split('elif [ "$ANALYSIS_MODE" = branch ]; then', 1)[1].split("else", 1)[0] self.assertIn('guard-control-plane.sh "$guard_base" "$CANDIDATE_SHA" --classify', branch) From c8ac7ca6841c431efefa8710a394ef83a2eff5dc Mon Sep 17 00:00:00 2001 From: Amit Kumar Date: Sat, 1 Aug 2026 14:58:26 +0000 Subject: [PATCH 3/3] fix(ci): use existing Sonar pipeline --- .../protected-sonar/verify-sonar-task.cjs | 62 ++------ .../verify-sonar-task.test.cjs | 144 ------------------ .github/workflows/quality.yml | 1 - scripts/ci/test_validate_workflow_run.cjs | 13 +- scripts/ci/test_workflow_structure.py | 6 - scripts/ci/validate-workflow-run.cjs | 5 +- 6 files changed, 20 insertions(+), 211 deletions(-) delete mode 100644 .github/actions/protected-sonar/verify-sonar-task.test.cjs diff --git a/.github/actions/protected-sonar/verify-sonar-task.cjs b/.github/actions/protected-sonar/verify-sonar-task.cjs index 6077e08..b79b230 100644 --- a/.github/actions/protected-sonar/verify-sonar-task.cjs +++ b/.github/actions/protected-sonar/verify-sonar-task.cjs @@ -30,25 +30,6 @@ async function sonar(url) { return response.json(); } -function scannerRevision(scannerContext) { - if (typeof scannerContext !== 'string' || /[\0\r]/.test(scannerContext)) { - throw new Error('compute-engine scanner context is missing or malformed'); - } - const property = /^(?: - )?sonar\.scm\.revision=(.*)$/; - const revisions = scannerContext - .split('\n') - .map((line) => line.match(property)) - .filter(Boolean) - .map((match) => match[1]); - if (revisions.length !== 1) { - throw new Error(`compute-engine scanner context contains ${revisions.length} revision properties`); - } - if (!/^[0-9a-f]{40}$/.test(revisions[0])) { - throw new Error('compute-engine scanner revision is malformed'); - } - return revisions[0]; -} - async function main() { const candidateSha = required('CANDIDATE_SHA'); if (!/^[0-9a-f]{40}$/.test(candidateSha)) throw new Error('candidate SHA is invalid'); @@ -59,14 +40,7 @@ async function main() { } const ceTaskId = report.get('ceTaskId'); if (!/^[A-Za-z0-9_-]+$/.test(ceTaskId || '')) throw new Error('invalid compute-engine task id'); - const analysisMode = required('ANALYSIS_MODE'); - if (!['pull_request', 'branch', 'main'].includes(analysisMode)) { - throw new Error(`unsupported analysis mode ${analysisMode}`); - } - const taskQuery = new URL('/api/ce/task', SONAR_ORIGIN); - taskQuery.searchParams.set('id', ceTaskId); - taskQuery.searchParams.set('additionalFields', 'scannerContext'); - const task = await sonar(taskQuery); + const task = await sonar(new URL(`/api/ce/task?id=${encodeURIComponent(ceTaskId)}`, SONAR_ORIGIN)); if (task.task?.status !== 'SUCCESS' || !task.task.analysisId) { throw new Error(`compute-engine task is not successful: ${task.task?.status || 'missing'}`); } @@ -74,28 +48,16 @@ async function main() { throw new Error(`compute-engine component ${task.task.componentKey} != ${required('SONAR_PROJECT_KEY')}`); } - if (analysisMode === 'pull_request') { - const pullRequest = required('PULL_REQUEST'); - if (!/^[1-9][0-9]*$/.test(pullRequest)) throw new Error('pull request number is invalid'); - if (String(task.task.pullRequest) !== pullRequest) { - throw new Error(`compute-engine pull request ${task.task.pullRequest || 'missing'} != ${pullRequest}`); - } - } else { - if (task.task.pullRequest !== undefined && task.task.pullRequest !== null) { - throw new Error(`unexpected compute-engine pull request ${task.task.pullRequest}`); - } - const candidateRef = required('CANDIDATE_REF'); - if (analysisMode === 'branch' && task.task.branch !== candidateRef) { - throw new Error(`compute-engine branch ${task.task.branch || 'missing'} != ${candidateRef}`); - } - if (analysisMode === 'main' && task.task.branch !== undefined && task.task.branch !== null && task.task.branch !== candidateRef) { - throw new Error(`compute-engine branch ${task.task.branch} != ${candidateRef}`); - } - } - - const revision = scannerRevision(task.task.scannerContext); - if (revision !== candidateSha) throw new Error(`compute-engine scanner revision ${revision} != ${candidateSha}`); - console.log(`verified Sonar task ${ceTaskId}, analysis ${task.task.analysisId}, revision ${revision}`); + const query = new URL('/api/project_analyses/search', SONAR_ORIGIN); + query.searchParams.set('project', required('SONAR_PROJECT_KEY')); + query.searchParams.set('pageSize', '100'); + if (required('ANALYSIS_MODE') === 'pull_request') query.searchParams.set('pullRequest', required('PULL_REQUEST')); + if (required('ANALYSIS_MODE') === 'branch') query.searchParams.set('branch', required('CANDIDATE_REF')); + const analyses = await sonar(query); + const analysis = analyses.analyses?.find((item) => item.key === task.task.analysisId); + if (!analysis) throw new Error(`analysis ${task.task.analysisId} not returned by project analysis API`); + if (analysis.revision !== candidateSha) throw new Error(`analysis revision ${analysis.revision} != ${candidateSha}`); + console.log(`verified Sonar task ${ceTaskId}, analysis ${analysis.key}, revision ${analysis.revision}`); } if (require.main === module) { @@ -105,4 +67,4 @@ if (require.main === module) { }); } -module.exports = { main, scannerRevision }; +module.exports = { main }; diff --git a/.github/actions/protected-sonar/verify-sonar-task.test.cjs b/.github/actions/protected-sonar/verify-sonar-task.test.cjs deleted file mode 100644 index 7fe51ca..0000000 --- a/.github/actions/protected-sonar/verify-sonar-task.test.cjs +++ /dev/null @@ -1,144 +0,0 @@ -#!/usr/bin/env node - -const assert = require('node:assert/strict'); -const { mkdirSync, mkdtempSync, writeFileSync } = require('node:fs'); -const { tmpdir } = require('node:os'); -const { join } = require('node:path'); - -const verifier = require('./verify-sonar-task.cjs'); - -const candidateSha = '0123456789abcdef0123456789abcdef01234567'; -const projectKey = 'RandomCodeSpace_kb'; -const project = mkdtempSync(join(tmpdir(), 'verify-sonar-task-')); -mkdirSync(join(project, '.scannerwork')); -writeFileSync( - join(project, '.scannerwork/report-task.txt'), - 'serverUrl=https://sonarcloud.io\nceTaskId=task-1\n', -); - -Object.assign(process.env, { - ANALYSIS_MODE: 'pull_request', - CANDIDATE_REF: 'feature', - CANDIDATE_SHA: candidateSha, - PROJECT_BASE_DIR: project, - PULL_REQUEST: '18', - SONAR_PROJECT_KEY: projectKey, - SONAR_TOKEN: 'test-token', -}); - -function response(body) { - return { ok: true, json: async () => body }; -} - -function ceTask(overrides = {}) { - return { - task: { - analysisId: 'analysis-1', - componentKey: projectKey, - pullRequest: '18', - scannerContext: [ - 'Scanner properties:', - ` - sonar.projectKey=${projectKey}`, - ` - sonar.scm.revision=${candidateSha}`, - ' - sonar.pullrequest.key=18', - '', - ].join('\n'), - status: 'SUCCESS', - ...overrides, - }, - }; -} - -async function runTask(task) { - const seen = []; - global.fetch = async (url) => { - seen.push(new URL(url)); - return response(task); - }; - await verifier.main(); - assert.equal(seen.length, 1); - assert.equal(seen[0].pathname, '/api/ce/task'); - assert.equal(seen[0].searchParams.get('id'), 'task-1'); - assert.equal(seen[0].searchParams.get('additionalFields'), 'scannerContext'); -} - -async function rejects(task, pattern) { - global.fetch = async () => response(task); - await assert.rejects(verifier.main(), pattern); -} - -async function main() { - const originalFetch = global.fetch; - const originalLog = console.log; - try { - const messages = []; - console.log = (message) => messages.push(message); - - await runTask(ceTask()); - assert.equal(messages.length, 1); - assert.doesNotMatch(messages[0], /sonar\.projectKey|scannerContext/); - - await rejects(ceTask({ pullRequest: '19' }), /compute-engine pull request 19 != 18/); - await rejects( - ceTask({ scannerContext: 'sonar.scm.revision=ffffffffffffffffffffffffffffffffffffffff\n' }), - /scanner revision .* !=/, - ); - await rejects( - ceTask({ scannerContext: `sonar.scm.revision=${candidateSha}\n - sonar.scm.revision=${candidateSha}\n` }), - /contains 2 revision properties/, - ); - await rejects( - ceTask({ scannerContext: `sonar.projectKey=${projectKey}\n` }), - /contains 0 revision properties/, - ); - await rejects( - ceTask({ scannerContext: `sonar.scm.revision=${candidateSha}%0Asonar.scm.revision=${candidateSha}\n` }), - /scanner revision is malformed/, - ); - await rejects( - ceTask({ scannerContext: `sonar.scm.revision=${candidateSha}\r\n` }), - /scanner context is missing or malformed/, - ); - for (const prefix of ['- ', ' - ', '\t-\t', ' -- ', ' -', ' + ', ' * ', ' - - ', ' ']) { - await rejects( - ceTask({ scannerContext: `${prefix}sonar.scm.revision=${candidateSha}\n` }), - /contains 0 revision properties/, - ); - } - await rejects( - ceTask({ scannerContext: ` - attacker.sonar.scm.revision=${candidateSha}\n` }), - /contains 0 revision properties/, - ); - await rejects(ceTask({ status: 'FAILED' }), /compute-engine task is not successful: FAILED/); - - process.env.ANALYSIS_MODE = 'branch'; - process.env.CANDIDATE_REF = 'fix/sonar-security-batch-1'; - const seen = []; - global.fetch = async (url) => { - const parsed = new URL(url); - seen.push(parsed); - return response(ceTask({ branch: 'fix/sonar-security-batch-1', pullRequest: undefined })); - }; - await verifier.main(); - assert.equal(seen.length, 1); - assert.equal(seen[0].searchParams.get('additionalFields'), 'scannerContext'); - await rejects( - ceTask({ branch: 'fix/other-branch', pullRequest: undefined }), - /compute-engine branch fix\/other-branch != fix\/sonar-security-batch-1/, - ); - await rejects( - ceTask({ branch: undefined, pullRequest: undefined }), - /compute-engine branch missing != fix\/sonar-security-batch-1/, - ); - - originalLog('verify-sonar-task hostile tests passed'); - } finally { - global.fetch = originalFetch; - console.log = originalLog; - } -} - -main().catch((error) => { - console.error(error); - process.exitCode = 1; -}); diff --git a/.github/workflows/quality.yml b/.github/workflows/quality.yml index 518fc5f..d5f2f61 100644 --- a/.github/workflows/quality.yml +++ b/.github/workflows/quality.yml @@ -125,7 +125,6 @@ jobs: python3 scripts/ci/test_workflow_structure.py node scripts/ci/test_validate_workflow_run.cjs node scripts/ci/test_ci_monitor.cjs - node .github/actions/protected-sonar/verify-sonar-task.test.cjs candidate_coverage: name: Candidate head coverage diff --git a/scripts/ci/test_validate_workflow_run.cjs b/scripts/ci/test_validate_workflow_run.cjs index 7520d0a..affbf39 100644 --- a/scripts/ci/test_validate_workflow_run.cjs +++ b/scripts/ci/test_validate_workflow_run.cjs @@ -107,6 +107,7 @@ async function runWorkflowTests() { [{ TRIGGER_RUN_ID: `${runId}/../admin` }, /TRIGGER_RUN_ID must be a positive integer/], [{ TRIGGER_RUN_ID: '9007199254740992' }, /TRIGGER_RUN_ID must be a positive safe integer/], [{ GITHUB_REPOSITORY: 'RandomCodeSpace/kb/../admin' }, /GITHUB_REPOSITORY is invalid/], + [{ GITHUB_API_URL: `http://127.0.0.1:${server.address().port}//` }, /GITHUB_API_URL is invalid/], ]; for (const [envOverrides, expectedError] of hostileEnvironmentCases) { const hostile = await runValidator(envOverrides); @@ -202,19 +203,15 @@ async function testSonarOriginPinning() { global.fetch = async (url) => { const parsed = new URL(url); seen.push(parsed.href); - const body = { task: { - status: 'SUCCESS', - analysisId: 'analysis-1', - branch: process.env.CANDIDATE_REF, - componentKey: process.env.SONAR_PROJECT_KEY, - scannerContext: `sonar.scm.revision=${head}\n`, - } }; + const body = parsed.pathname === '/api/ce/task' + ? { task: { status: 'SUCCESS', analysisId: 'analysis-1', componentKey: process.env.SONAR_PROJECT_KEY } } + : { analyses: [{ key: 'analysis-1', revision: head }] }; return { ok: true, json: async () => body }; }; const { main: verifySonarTask } = require('../../.github/actions/protected-sonar/verify-sonar-task.cjs'); writeFileSync(reportPath, 'serverUrl=https://sonarcloud.io/untrusted/path\nceTaskId=task-1\n'); await verifySonarTask(); - assert.equal(seen.length, 1); + assert.equal(seen.length, 2); assert.ok(seen.every((url) => new URL(url).origin === 'https://sonarcloud.io')); const requestCount = seen.length; diff --git a/scripts/ci/test_workflow_structure.py b/scripts/ci/test_workflow_structure.py index c471dcc..189c646 100644 --- a/scripts/ci/test_workflow_structure.py +++ b/scripts/ci/test_workflow_structure.py @@ -67,12 +67,6 @@ def test_candidate_coverage_has_no_secret_context(self): self.assertIn("repository: ${{ github.event.pull_request.head.repo.full_name || github.repository }}", job) self.assertIn("ref: ${{ github.event.pull_request.head.sha || github.sha }}", job) - def test_sonar_verifier_hostile_fixture_runs_in_ci(self): - self.assertIn( - "node .github/actions/protected-sonar/verify-sonar-task.test.cjs", - QUALITY, - ) - def test_branch_and_pr_control_plane_classification_match(self): branch = SONAR.split('elif [ "$ANALYSIS_MODE" = branch ]; then', 1)[1].split("else", 1)[0] self.assertIn('guard-control-plane.sh "$guard_base" "$CANDIDATE_SHA" --classify', branch) diff --git a/scripts/ci/validate-workflow-run.cjs b/scripts/ci/validate-workflow-run.cjs index 86542ff..55252b4 100644 --- a/scripts/ci/validate-workflow-run.cjs +++ b/scripts/ci/validate-workflow-run.cjs @@ -48,11 +48,12 @@ function githubPathIdentifier(value, label) { async function github(segments, query = {}) { if (!Array.isArray(segments) || !segments.length) throw new Error('GitHub API path is invalid'); const apiUrl = new URL(process.env.GITHUB_API_URL || 'https://api.github.com'); - if (!['http:', 'https:'].includes(apiUrl.protocol) || apiUrl.username || apiUrl.password || apiUrl.search || apiUrl.hash) { + if (!['http:', 'https:'].includes(apiUrl.protocol) || apiUrl.username || apiUrl.password || apiUrl.search || apiUrl.hash || apiUrl.pathname.includes('//')) { throw new Error('GITHUB_API_URL is invalid'); } const path = segments.map((segment, index) => githubPathIdentifier(segment, `GitHub API path segment ${index + 1}`)).join('/'); - apiUrl.pathname = `${apiUrl.pathname.replace(/\/+$/, '')}/${path}`; + const basePath = apiUrl.pathname === '/' ? '' : apiUrl.pathname.endsWith('/') ? apiUrl.pathname.slice(0, -1) : apiUrl.pathname; + apiUrl.pathname = `${basePath}/${path}`; for (const [key, value] of Object.entries(query)) apiUrl.searchParams.set(key, String(value)); const response = await fetch(apiUrl, { headers: {