fix: prevent private workflows from selecting hosted runners #1325
There are no files selected for viewing
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
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,71 @@ | ||
| import fs from 'node:fs/promises'; | ||
| import path from 'node:path'; | ||
|
|
||
| import * as yaml from 'js-yaml'; | ||
| import { z } from 'zod'; | ||
|
|
||
| import type { PackageConfig } from '../packageConfig.js'; | ||
|
|
||
| const runnerSchema = z.union([z.string(), z.array(z.string())]); | ||
| const workflowSchema = z.object({ | ||
| jobs: z.record( | ||
| z.string(), | ||
| z | ||
| .object({ | ||
| 'runs-on': runnerSchema.optional(), | ||
| strategy: z.object({ matrix: z.record(z.string(), z.unknown()) }).optional(), | ||
| }) | ||
| .nullable() | ||
| ), | ||
| }); | ||
|
|
||
| export async function assertPrivateWorkflowRunners(config: PackageConfig, workflowsPath: string): Promise<void> { | ||
| if (config.isPublicRepo || !config.isRepoVisibilityKnown) return; | ||
| const entries = await fs.readdir(workflowsPath, { withFileTypes: true }).catch((error: NodeJS.ErrnoException) => { | ||
| if (error.code === 'ENOENT') return []; | ||
| throw error; | ||
| }); | ||
| for (const { name: fileName } of entries.filter((entry) => entry.isFile() && /\.ya?ml$/u.test(entry.name))) { | ||
| const workflow = workflowSchema.parse(yaml.load(await fs.readFile(path.join(workflowsPath, fileName), 'utf8'))); | ||
| for (const [jobName, job] of Object.entries(workflow.jobs)) { | ||
| if (!job?.['runs-on']) continue; | ||
| const runner = job['runs-on']; | ||
| if (isSelfHosted(runner)) continue; | ||
| const match = typeof runner === 'string' && /^\$\{\{\s*matrix\.(\w+)\s*\}\}$/u.exec(runner); | ||
| const candidates: unknown[] = []; | ||
| if (match && job.strategy) { | ||
| const key = match[1]!; | ||
| const matrix = job.strategy.matrix; | ||
| const values = z.array(z.unknown()).safeParse(matrix[key]); | ||
| if (values.success) candidates.push(...values.data); | ||
| const include = z.array(z.record(z.string(), z.unknown())).safeParse(matrix.include); | ||
| if (include.success) | ||
| candidates.push(...include.data.map((row) => row[key]).filter((value) => value !== undefined)); | ||
| } else { | ||
| candidates.push(runner); | ||
| } | ||
| const allowed = | ||
| candidates.length > 0 && | ||
| candidates.every( | ||
| (candidate) => | ||
| isSelfHosted(candidate) || | ||
| (config.repository?.toLowerCase() === 'github:willbooster/cheerlings' && | ||
| fileName === 'build-desktop-apps.yml' && | ||
| jobName === 'build' && | ||
| candidate === 'windows-latest') | ||
| ); | ||
| if (!allowed) { | ||
| throw new Error( | ||
| `${fileName}: jobs.${jobName}.runs-on must select self-hosted runners in a private repository. Fix the workflow before running wbfy.` | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+28
to
+63
Contributor
There was a problem hiding this comment. There are several robustness and correctness improvements that can be made here:
for (const { name: fileName } of entries.filter((entry) => entry.isFile() && /\.ya?ml$/u.test(entry.name))) {
let parsedYaml: unknown;
try {
parsedYaml = yaml.load(await fs.readFile(path.join(workflowsPath, fileName), 'utf8'));
} catch {
continue;
}
const parsed = workflowSchema.safeParse(parsedYaml);
if (!parsed.success) continue;
const workflow = parsed.data;
for (const [jobName, job] of Object.entries(workflow.jobs)) {
if (!job?.['runs-on']) continue;
const runner = job['runs-on'];
if (isSelfHosted(runner)) continue;
const match = typeof runner === 'string' && /^\$\{\{\s*matrix\.([\w-]+)\s*\}\}$/u.exec(runner);
const candidates: unknown[] = [];
if (match && job.strategy?.matrix) {
const key = match[1]!;
const matrix = job.strategy.matrix;
const matrixValue = matrix[key];
if (matrixValue !== undefined) {
const values = Array.isArray(matrixValue) ? matrixValue : [matrixValue];
candidates.push(...values);
}
const include = z.array(z.record(z.string(), z.unknown())).safeParse(matrix.include);
if (include.success) {
candidates.push(...include.data.map((row) => row[key]).filter((value) => value !== undefined));
}
} else {
candidates.push(runner);
}
const allowed =
candidates.length > 0 &&
candidates.every(
(candidate) =>
isSelfHosted(candidate) ||
(config.repository?.toLowerCase() === 'github:willbooster/cheerlings' &&
fileName === 'build-desktop-apps.yml' &&
jobName === 'build' &&
candidate === 'windows-latest')
);
if (!allowed) {
throw new Error(
`${fileName}: jobs.${jobName}.runs-on must select self-hosted runners in a private repository. Fix the workflow before running wbfy.`
);
}
}
}
Member
Author
There was a problem hiding this comment. Skipping invalid YAML or schema failures would bypass the requested fail-fast runner check, so those errors must propagate. The 52-repository audit has no hyphenated runner matrix keys; unsupported custom selections must be fixed explicitly under the repository's canonical-format rule. GitHub documents matrix axes as arrays, not scalar values: https://docs.github.com/en/actions/how-tos/write-workflows/choose-what-workflows-do/run-job-variations . No observed caller requires the proposed scalar fallback. |
||
| } | ||
|
|
||
| function isSelfHosted(runner: unknown): boolean { | ||
| const labels = runnerSchema.safeParse(runner); | ||
| if (!labels.success) return false; | ||
| if (Array.isArray(labels.data)) return labels.data.includes('self-hosted'); | ||
| return labels.data === 'self-hosted'; | ||
| } | ||
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,66 @@ | ||
| import assert from 'node:assert/strict'; | ||
| import fs from 'node:fs'; | ||
| import path from 'node:path'; | ||
|
|
||
| import { YAML } from 'bun'; | ||
| import { expect, test } from 'bun:test'; | ||
| import { z } from 'zod'; | ||
|
|
||
| import { generateWorkflows } from '../../src/generators/workflow.js'; | ||
| import { promisePool } from '../../src/utils/promisePool.js'; | ||
| import { withTempWorkflowsRepo } from '../helpers/callerWorkflow.js'; | ||
| import { createConfig } from '../helpers/testConfig.js'; | ||
|
|
||
| const callerSchema = z.object({ | ||
| jobs: z.record(z.string(), z.object({ with: z.record(z.string(), z.unknown()).optional() })), | ||
| }); | ||
|
|
||
| test('rewriting private callers removes hosted overrides without losing custom runner constraints', async () => { | ||
| await withTempWorkflowsRepo('wbfy-private-runners-', async (dirPath, workflowsPath) => { | ||
| const labels = ['self-hosted', 'macOS', 'large']; | ||
| const original = { | ||
| jobs: { | ||
| test: { | ||
| uses: 'WillBooster/reusable-workflows/.github/workflows/test.yml@main', | ||
| with: { | ||
| github_hosted_runner: true, | ||
| runs_on: JSON.stringify('ubuntu-22.04'), | ||
| custom_test_command: 'bun run test/ci', | ||
| }, | ||
| }, | ||
| custom: { | ||
| uses: 'WillBooster/reusable-workflows/.github/workflows/run-script.yml@main', | ||
| with: { github_hosted_runner: true, runs_on: JSON.stringify(labels) }, | ||
| }, | ||
| }, | ||
| }; | ||
| const filePath = path.join(workflowsPath, 'custom.yml'); | ||
| fs.writeFileSync(filePath, YAML.stringify(original)); | ||
| const config = createConfig({ dirPath, isRoot: true, isPublicRepo: false }); | ||
| await generateWorkflows(config); | ||
| await promisePool.promiseAll(); | ||
| const written = fs.readFileSync(filePath, 'utf8'); | ||
| const { jobs } = callerSchema.parse(YAML.parse(written)); | ||
| for (const job of Object.values(jobs)) expect(job.with?.github_hosted_runner).toBeUndefined(); | ||
| expect(jobs.test?.with?.runs_on).toBeUndefined(); | ||
| expect(jobs.test?.with?.custom_test_command).toBe(original.jobs.test.with.custom_test_command); | ||
| expect(JSON.parse(String(jobs.custom?.with?.runs_on))).toEqual(labels); | ||
| await generateWorkflows(config); | ||
| await promisePool.promiseAll(); | ||
| expect(fs.readFileSync(filePath, 'utf8')).toBe(written); | ||
| }); | ||
| }); | ||
|
|
||
| test('private custom runner violations stop generation before any workflow is rewritten', async () => { | ||
| await withTempWorkflowsRepo('wbfy-private-custom-', async (dirPath, workflowsPath) => { | ||
| const filePath = path.join(workflowsPath, 'custom.yaml'); | ||
| const content = `jobs:\n build:\n strategy:\n matrix:\n runner: [ubuntu-latest, macos-latest]\n runs-on: \${{ matrix.runner }}\n steps:\n - run: echo build\n`; | ||
| fs.writeFileSync(filePath, content); | ||
| await assert.rejects( | ||
| generateWorkflows(createConfig({ dirPath, isRoot: true, isPublicRepo: false })), | ||
| /jobs\.build\.runs-on/u | ||
| ); | ||
| expect(fs.readFileSync(filePath, 'utf8')).toBe(content); | ||
| expect(fs.readdirSync(workflowsPath)).toEqual(['custom.yaml']); | ||
| }); | ||
| }); |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In GitHub Actions, a
strategyblock can be defined without amatrix(for example, to configurefail-fastormax-paralleloptions). Makingmatrixrequired in the Zod schema will causeworkflowSchema.parseto throw a validation error and crashwbfyon such valid workflows. Makingmatrixoptional prevents these unexpected crashes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 52-repository private workflow audit contains 17 strategy blocks, all with matrix; none needs this alternative shape. The repository instructions require handling only observed organization cases and fixing unsupported custom workflows rather than adding compatibility branches. This validator intentionally fails before writes when a custom workflow is outside the supported shape.