fix(SEC-005): add cross-repository allowlist validation to five handler files#19333
fix(SEC-005): add cross-repository allowlist validation to five handler files#19333
Conversation
…andlers Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
🌑 The shadows whisper... Smoke Codex failed. The oracle requires further meditation... |
Agent Container Tool Check
Result: 12/12 tools available ✅ Overall Status: PASS
|
|
Commit pushed:
|
|
🎉 Smoke test results for PR #19333 by
Overall:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds allowlist validation to multiple checkout/commit utilities. The approach is consistent across all files. One suggestion: consider upgrading core.warning to core.error in extra_empty_commit.cjs for failed validation, as it's a blocking error condition. Overall clean and well-structured change.
📰 BREAKING: Report filed by Smoke Copilot
| return { | ||
| success: false, | ||
| error: `Invalid repository slug: ${repoSlug}. Expected format: owner/repo`, | ||
| error: `${ERR_VALIDATION}: Invalid repository slug: ${repoSlug}. Expected format: owner/repo`, |
There was a problem hiding this comment.
Good use of ERR_VALIDATION prefix for structured error reporting — this makes it easier to grep/filter validation errors in logs. 👍
| return { success: false, error: validation.error }; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider using core.error instead of core.warning here since a failed validation that prevents the commit is effectively an error condition, not just a warning.
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
Smoke Test Results — Run §22608737171Status: PARTIAL (2 skipped, all others ✅) Core Tests (1–10) PR Review Tests (11–17)
|
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal! This review validates PR review safe output tests for run 22608737171.
💥 [THE END] — Illustrated by Smoke Claude
…epos params Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in 8d0af6d. The TypeScript errors were missing |
There was a problem hiding this comment.
Pull request overview
Adds SEC-005 cross-repository allowlist validation hooks to multiple safe-output helpers so cross-repo git/API operations can be gated by allowedRepos / GH_AW_ALLOWED_REPOS.
Changes:
- Added
validateTargetRepo+parseAllowedReposchecks to cross-repo helpers (dynamic_checkout,extra_empty_commit,find_repo_checkout,get_base_branch,safe_outputs_tools_loader). - Updated JSDoc/signatures to thread an
allowedReposoption where needed and standardized validation error prefixing indynamic_checkout. - Added a changeset documenting the security hardening.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/safe_outputs_tools_loader.cjs | Warns on misconfigured create_pull_request.target-repo vs allowlist during tool registration. |
| actions/setup/js/get_base_branch.cjs | Validates cross-repo base-branch lookup targets before GitHub API calls. |
| actions/setup/js/find_repo_checkout.cjs | Adds optional allowlist validation before searching workspace for a repo checkout. |
| actions/setup/js/extra_empty_commit.cjs | Adds optional allowlist validation before any git operations for the target repo. |
| actions/setup/js/dynamic_checkout.cjs | Adds optional allowlist validation to repo checkout switching helper and updates error message prefixing. |
| .changeset/patch-validate-cross-repo-allowlist.md | Release note for the new allowlist validation behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Validate target repository against allowlist before any git operations | ||
| const allowedRepos = parseAllowedRepos(allowedReposInput); | ||
| if (allowedRepos.size > 0) { | ||
| const targetRepo = `${repoOwner}/${repoName}`; | ||
| const defaultRepo = getDefaultTargetRepo(); | ||
| const validation = validateTargetRepo(targetRepo, defaultRepo, allowedRepos); | ||
| if (!validation.valid) { | ||
| core.warning(`ERR_VALIDATION: ${validation.error}`); | ||
| return { success: false, error: validation.error ?? "" }; | ||
| } | ||
| } |
There was a problem hiding this comment.
This adds allowlist validation logic but there are no corresponding tests in extra_empty_commit.test.cjs covering allowed vs rejected repositories. Adding tests for reject/allow scenarios (including when GITHUB_REPOSITORY is unset) would ensure SEC-005 stays enforced.
| // Validate target repo against configured allowlist before any git operations | ||
| const allowedRepos = parseAllowedRepos(options.allowedRepos); | ||
| if (allowedRepos.size > 0) { | ||
| const defaultRepo = getDefaultTargetRepo(); | ||
| const validation = validateTargetRepo(repoSlug, defaultRepo, allowedRepos); | ||
| if (!validation.valid) { | ||
| return { success: false, error: `${ERR_VALIDATION}: ${validation.error}` }; | ||
| } | ||
| } |
There was a problem hiding this comment.
The allowlist check is currently gated by if (allowedRepos.size > 0), which means a cross-repo repoSlug will proceed with git operations when allowedRepos is unset/empty. For SEC-005, validation should still run with an empty allowlist so that only defaultRepo is permitted (and any other repo is rejected). Also note that createCheckoutManager().switchTo() calls checkoutRepo() without passing allowedRepos, so this validation path will never run unless callers supply it explicitly.
| if (allowedRepos.size > 0) { | ||
| const defaultRepo = getDefaultTargetRepo(); | ||
| const validation = validateTargetRepo(targetSlug, defaultRepo, allowedRepos); | ||
| if (!validation.valid) { | ||
| return { success: false, error: validation.error }; | ||
| } | ||
| } | ||
|
|
||
| // Find all git directories in the workspace | ||
| const gitDirs = findGitDirectories(ws); | ||
| debugLog(`Found ${gitDirs.length} git directories: ${gitDirs.join(", ")}`); |
There was a problem hiding this comment.
The allowlist enforcement is currently conditional on allowedRepos.size > 0, so callers who omit options.allowedRepos can search for (and potentially operate on) arbitrary repo slugs. For SEC-005, you generally want validation to run even with an empty allowlist so that only defaultRepo is permitted unless additional repos are explicitly allowlisted.
| if (allowedRepos.size > 0) { | |
| const defaultRepo = getDefaultTargetRepo(); | |
| const validation = validateTargetRepo(targetSlug, defaultRepo, allowedRepos); | |
| if (!validation.valid) { | |
| return { success: false, error: validation.error }; | |
| } | |
| } | |
| // Find all git directories in the workspace | |
| const gitDirs = findGitDirectories(ws); | |
| debugLog(`Found ${gitDirs.length} git directories: ${gitDirs.join(", ")}`); | |
| const defaultRepo = getDefaultTargetRepo(); | |
| const validation = validateTargetRepo(targetSlug, defaultRepo, allowedRepos); | |
| if (!validation.valid) { | |
| return { success: false, error: validation.error }; | |
| } | |
| // Find all git directories in the workspace | |
| const gitDirs = findGitDirectories(ws); | |
| debugLog(`Found ${gitDirs.length} git directories: ${gitDirs.join(", ")}`); | |
| const gitDirs = findGitDirectories(ws); | |
| debugLog(`Found ${gitDirs.length} git directories: ${gitDirs.join(", ")}`); |
| if (allowedRepos.size > 0) { | ||
| const defaultRepo = getDefaultTargetRepo(); | ||
| const validation = validateTargetRepo(targetRepoSlug, defaultRepo, allowedRepos); | ||
| if (!validation.valid) { | ||
| if (typeof core !== "undefined") { | ||
| core.warning(`ERR_VALIDATION: ${validation.error}`); | ||
| } | ||
| return process.env.DEFAULT_BRANCH || "main"; | ||
| } |
There was a problem hiding this comment.
This validation is only performed when GH_AW_ALLOWED_REPOS parses to a non-empty set. If the env var is unset/empty, cross-repo calls to getBaseBranch(repoParts) will proceed with GitHub API requests without any allowlist enforcement. For SEC-005, consider always running validateTargetRepo(targetRepoSlug, defaultRepo, allowedRepos) so that only the default repo is allowed when the allowlist is empty.
| if (allowedRepos.size > 0) { | |
| const defaultRepo = getDefaultTargetRepo(); | |
| const validation = validateTargetRepo(targetRepoSlug, defaultRepo, allowedRepos); | |
| if (!validation.valid) { | |
| if (typeof core !== "undefined") { | |
| core.warning(`ERR_VALIDATION: ${validation.error}`); | |
| } | |
| return process.env.DEFAULT_BRANCH || "main"; | |
| } | |
| const defaultRepo = getDefaultTargetRepo(); | |
| const validation = validateTargetRepo(targetRepoSlug, defaultRepo, allowedRepos); | |
| if (!validation.valid) { | |
| if (typeof core !== "undefined") { | |
| core.warning(`ERR_VALIDATION: ${validation.error}`); | |
| } | |
| return process.env.DEFAULT_BRANCH || "main"; |
| "gh-aw": patch | ||
| --- | ||
|
|
||
| Add allowlist validation for the cross-repo helpers so `targetRepo` parameters and GH_AW_ALLOWED_REPOS checks now guard git and API work before any operations. |
There was a problem hiding this comment.
This changeset claims allowlist validation now "guard[s] git and API work before any operations", but the new checks are currently conditional (e.g., only enforced when an allowlist is non-empty), so cross-repo operations may still proceed when no allowlist is configured. Please adjust the wording to match the actual enforcement semantics (or tighten the enforcement to match the statement).
| Add allowlist validation for the cross-repo helpers so `targetRepo` parameters and GH_AW_ALLOWED_REPOS checks now guard git and API work before any operations. | |
| Add allowlist validation for the cross-repo helpers so that, when an allowlist is configured, `targetRepo` parameters and GH_AW_ALLOWED_REPOS checks guard git and API work for cross-repo operations. |
| // Validate target repo against allowlist before any API calls | ||
| const targetRepoSlug = `${repoOwner}/${repoName}`; | ||
| const allowedRepos = parseAllowedRepos(process.env.GH_AW_ALLOWED_REPOS); | ||
| if (allowedRepos.size > 0) { | ||
| const defaultRepo = getDefaultTargetRepo(); | ||
| const validation = validateTargetRepo(targetRepoSlug, defaultRepo, allowedRepos); | ||
| if (!validation.valid) { | ||
| if (typeof core !== "undefined") { | ||
| core.warning(`ERR_VALIDATION: ${validation.error}`); | ||
| } | ||
| return process.env.DEFAULT_BRANCH || "main"; | ||
| } | ||
| } |
There was a problem hiding this comment.
This introduces allowlist validation behavior for cross-repo base-branch lookups, but get_base_branch.test.cjs doesn’t currently cover the allow/deny outcomes. Adding tests that set GH_AW_ALLOWED_REPOS and verify the function warns/short-circuits on a disallowed targetRepo would help keep SEC-005 covered.
| if (allowedRepos.size > 0) { | ||
| const defaultRepo = getDefaultTargetRepo(config.create_pull_request); | ||
| const validation = validateTargetRepo(targetRepo, defaultRepo, allowedRepos); | ||
| if (!validation.valid) { | ||
| server.debug(`WARNING: SEC-005: ${validation.error}`); | ||
| } | ||
| } | ||
| toolToRegister = JSON.parse(JSON.stringify(tool)); | ||
| toolToRegister.description += ` Note: This workflow is configured to create pull requests in '${targetRepo}'. You do not need to specify the repo parameter.`; |
There was a problem hiding this comment.
Since this now performs target-repo validation for the create_pull_request tool configuration, consider adding/expanding tests in safe_outputs_tools_loader.test.cjs to cover the cases where target-repo is (a) default, (b) allowlisted, and (c) not allowlisted, so misconfigurations are reliably surfaced.
| if (allowedRepos.size > 0) { | |
| const defaultRepo = getDefaultTargetRepo(config.create_pull_request); | |
| const validation = validateTargetRepo(targetRepo, defaultRepo, allowedRepos); | |
| if (!validation.valid) { | |
| server.debug(`WARNING: SEC-005: ${validation.error}`); | |
| } | |
| } | |
| toolToRegister = JSON.parse(JSON.stringify(tool)); | |
| toolToRegister.description += ` Note: This workflow is configured to create pull requests in '${targetRepo}'. You do not need to specify the repo parameter.`; | |
| let repoNote = ` Note: This workflow is configured to create pull requests in '${targetRepo}'. You do not need to specify the repo parameter.`; | |
| if (allowedRepos.size > 0) { | |
| const defaultRepo = getDefaultTargetRepo(config.create_pull_request); | |
| const validation = validateTargetRepo(targetRepo, defaultRepo, allowedRepos); | |
| if (!validation.valid) { | |
| server.debug(`WARNING: SEC-005: ${validation.error}`); | |
| // Surface potential misconfiguration in the tool description as well | |
| repoNote += ` WARNING: The configured target-repo may not be in the allowed list and this configuration might be ignored at runtime.`; | |
| } | |
| } else { | |
| // No allowed_repos configured: surface this as a warning so misconfigurations are easier to detect | |
| server.debug( | |
| `WARNING: SEC-005: No allowed_repos are configured for create_pull_request; skipping validation for configured target-repo '${targetRepo}'.` | |
| ); | |
| repoNote += ` WARNING: No allowed_repos are configured for this workflow, so this default may not be enforced.`; | |
| } | |
| toolToRegister = JSON.parse(JSON.stringify(tool)); | |
| toolToRegister.description += repoNote; |
| if (allowedRepos.size > 0) { | ||
| const targetRepo = `${repoOwner}/${repoName}`; | ||
| const defaultRepo = getDefaultTargetRepo(); | ||
| const validation = validateTargetRepo(targetRepo, defaultRepo, allowedRepos); | ||
| if (!validation.valid) { | ||
| core.warning(`ERR_VALIDATION: ${validation.error}`); | ||
| return { success: false, error: validation.error ?? "" }; | ||
| } | ||
| } | ||
|
|
||
| // Cross-repo guard: never push an extra empty commit to a different repository. | ||
| // A token is needed to create the PR and that will trigger events anyway. |
There was a problem hiding this comment.
This validation is skipped entirely when allowedRepos is empty. That allows cross-repo pushes in environments where GITHUB_REPOSITORY is unset (since isCrossRepoTarget returns false), which defeats the SEC-005 allowlist guard. Consider always calling validateTargetRepo(targetRepo, defaultRepo, allowedRepos) (even when the allowlist is empty) so only the default repo is allowed unless explicitly allowlisted.
| if (allowedRepos.size > 0) { | |
| const targetRepo = `${repoOwner}/${repoName}`; | |
| const defaultRepo = getDefaultTargetRepo(); | |
| const validation = validateTargetRepo(targetRepo, defaultRepo, allowedRepos); | |
| if (!validation.valid) { | |
| core.warning(`ERR_VALIDATION: ${validation.error}`); | |
| return { success: false, error: validation.error ?? "" }; | |
| } | |
| } | |
| // Cross-repo guard: never push an extra empty commit to a different repository. | |
| // A token is needed to create the PR and that will trigger events anyway. | |
| const targetRepo = `${repoOwner}/${repoName}`; | |
| const defaultRepo = getDefaultTargetRepo(); | |
| const validation = validateTargetRepo(targetRepo, defaultRepo, allowedRepos); | |
| if (!validation.valid) { | |
| core.warning(`ERR_VALIDATION: ${validation.error}`); | |
| return { success: false, error: validation.error ?? "" }; | |
| } | |
| // Cross-repo guard: never push an extra empty commit to a different repository. | |
| // A token is needed to create the PR and that will trigger events anyway. | |
| // A token is needed to create the PR and that will trigger events anyway. |
| // Validate the configured target-repo against the allowed-repos list | ||
| const allowedRepos = parseAllowedRepos(config.create_pull_request.allowed_repos); | ||
| if (allowedRepos.size > 0) { | ||
| const defaultRepo = getDefaultTargetRepo(config.create_pull_request); | ||
| const validation = validateTargetRepo(targetRepo, defaultRepo, allowedRepos); | ||
| if (!validation.valid) { | ||
| server.debug(`WARNING: SEC-005: ${validation.error}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
This check only runs when allowedRepos.size > 0, so a misconfigured target-repo can slip through silently if allowed_repos is unset/empty. If the intent is to ensure only the default repo is used unless explicitly allowlisted, run validateTargetRepo(targetRepo, defaultRepo, allowedRepos) unconditionally and either warn/disable the tool description enrichment when invalid.
See below for a potential fix:
const defaultRepo = getDefaultTargetRepo(config.create_pull_request);
const validation = validateTargetRepo(targetRepo, defaultRepo, allowedRepos);
if (!validation.valid) {
server.debug(`WARNING: SEC-005: ${validation.error}`);
} else {
toolToRegister = JSON.parse(JSON.stringify(tool));
toolToRegister.description += ` Note: This workflow is configured to create pull requests in '${targetRepo}'. You do not need to specify the repo parameter.`;
if (toolToRegister.inputSchema && toolToRegister.inputSchema.properties && toolToRegister.inputSchema.properties.repo) {
toolToRegister.inputSchema.properties.repo.description += ` Configured default: '${targetRepo}'.`;
}
}
| if (allowedRepos.size > 0) { | ||
| const defaultRepo = getDefaultTargetRepo(); | ||
| const validation = validateTargetRepo(repoSlug, defaultRepo, allowedRepos); | ||
| if (!validation.valid) { | ||
| return { success: false, error: `${ERR_VALIDATION}: ${validation.error}` }; | ||
| } |
There was a problem hiding this comment.
New allowlist validation behavior was added here, but the module tests don't appear to exercise the allowlist paths (allowed vs rejected repo). Adding unit tests covering (1) default repo allowed, (2) non-default repo rejected when allowlist is empty, and (3) non-default repo allowed when explicitly allowlisted would help prevent regressions.
| if (allowedRepos.size > 0) { | |
| const defaultRepo = getDefaultTargetRepo(); | |
| const validation = validateTargetRepo(repoSlug, defaultRepo, allowedRepos); | |
| if (!validation.valid) { | |
| return { success: false, error: `${ERR_VALIDATION}: ${validation.error}` }; | |
| } | |
| const defaultRepo = getDefaultTargetRepo(); | |
| const validation = validateTargetRepo(repoSlug, defaultRepo, allowedRepos); | |
| if (!validation.valid) { | |
| return { success: false, error: `${ERR_VALIDATION}: ${validation.error}` }; |
validateTargetRepocalls todynamic_checkout.cjs(SEC-005 + USE-001)validateTargetRepocalls toextra_empty_commit.cjs(SEC-005)validateTargetRepocalls tofind_repo_checkout.cjs(SEC-005)validateTargetRepocalls toget_base_branch.cjs(SEC-005)validateTargetRepocalls tosafe_outputs_tools_loader.cjs(SEC-005)options.allowedReposto JSDoc indynamic_checkout.cjsandextra_empty_commit.cjs; fixstring|null→stringassignment inextra_empty_commit.cjsOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.
✨ PR Review Safe Output Test - Run 22608737171