Pull request workflow to check for additional criteria for registry.json changes

[richardpark-msft]

This workflow runs whenever a pull request targets the registry.json in main. The goal, eventually, is to open up approvals for registry.json so extension developers can approve their own releases, while also making sure that big changes still run through the azd team.

The rules are pretty simple:

• If the PR is approved, or created, by an azd team member we don't do any additional checks.
• If the PR was created by anyone else then we check that it only adds a new version and doesn't change the providers, capabilities or namespace or other fields that are not purely cosmetic, like description. If it does then the check fails, and will require a core team member to approve to unblock it.

Fixes #8997

[github-actions]

📋 Prioritization Note

Thanks for the contribution! The linked issue isn't in the current milestone yet.
Thank you for logging this issue; our team is reviewing it. If you need urgent prioritization, tag @RickWinter and @kristenwomack to let us know.

[Copilot]

Pull request overview

This PR adds a new GitHub Actions workflow (ext-registry-check.yml) and a supporting Node.js script (ext-registry-check.js) that gates changes to cli/azd/extensions/registry.json. The goal (issue #8997) is to let extension developers self-approve simple version-bump releases while still routing riskier changes through the azd core team. The script bypasses all checks when the PR is authored or approved by a core-team member; otherwise it verifies the PR touches only registry.json and that new releases don't alter providers, capabilities, or namespaces relative to the latest existing release. It ships with a vitest test suite plus JS tooling config.

Changes:

• New ext-registry-check.yml workflow (triggers on registry.json PRs) invoking a github-script step.
• New ext-registry-check.js implementing core-team bypass, changed-file scoping, registry diffing, and a small semver comparator.
• New test/tooling scaffolding: test/ext-registry-check.test.js, package.json, package-lock.json, jsconfig.json.

Show a summary per file

File	Description
.github/workflows/ext-registry-check.yml	Adds the PR workflow; still lists a dev base branch and grants unused write scopes.
.github/scripts/ext-registry-check.js	Core logic; approval check doesn't bind to the head commit (bypass risk).
.github/scripts/test/ext-registry-check.test.js	Unit + opt-in live tests for the diff/approval logic.
.github/scripts/package.json	Test-runner package; main references an unrelated script; no CI runs test.
.github/scripts/package-lock.json	Locked dev dependencies for the vitest suite.
.github/scripts/jsconfig.json	Strict checkJs config for the scripts.

Key concerns: the approval bypass counts stale APPROVED reviews from earlier commits, which lets a contributor land provider/capability/namespace changes after a benign approval — the sibling approval gate already guards against this by filtering on the head SHA. Additional cleanup items: the leftover feature branch in the trigger, the vitest suite not being wired into any CI job, over-broad workflow permissions, and an incorrect main field.

Review details

Files not reviewed (1)

• .github/scripts/package-lock.json: Generated file

• Files reviewed: 5/6 changed files
• Comments generated: 5
• Review effort level: Medium

[danieljurek]

REST API Specs shows how we've developed JS logic to run in GH Actions previously. These include a minimal set of dependencies to keep the actions efficient.

GitHub Actions-specific scripts: https://github.com/Azure/azure-rest-api-specs/tree/main/.github/workflows/src
Minimal dependencies: https://github.com/Azure/azure-rest-api-specs/blob/main/.github/package.json

This is a guide for where to take things. I won't block here but fitting into this layout and design should make things easier going forward.

cc @mikeharder who is the keeper of the specs repo

[danieljurek]

Switching back to neutral pending permission question

[jongio]

Solid registry-gating workflow. The core logic in diffRegistry is well-structured: immutability enforcement on published releases, capability/provider diffing against the latest semver baseline, and namespace protection all look correct. The semver comparator handles prerelease labels properly, and the test suite (unit + opt-in live scenarios against real PRs) gives good coverage.

The SHA-binding fix for stale approvals and the permissions tightening both look correct in the latest commit.

A few items already flagged by earlier reviewers that are still open:

• The �ranches filter in �xt-registry-check.yml still includes �zd-auto-approve-simple-updates (the feature branch). Should be removed before merge so the workflow only gates PRs targeting main.
• package.json main field points to pr-approval-foundry-extensions-shared.js (copy-paste leftover). Should reference �xt-registry-check.js or be omitted.
• The vitest suite isn't wired into any CI workflow yet, so regressions in the check logic won't be caught automatically.
• �xt-registry-check.yml is missing a trailing newline.

None of these are blockers.

[jongio]

Solid registry-gating workflow. The core logic in diffRegistry is well-structured: immutability enforcement on published releases, capability/provider diffing against the latest semver baseline, and namespace protection all look correct. The semver comparator handles prerelease labels properly, and the test suite (unit + opt-in live scenarios against real PRs) gives good coverage.

The SHA-binding fix for stale approvals and the permissions tightening both look correct in the latest commit.

A few items already flagged by earlier reviewers that are still open:

• The branches filter in ext-registry-check.yml still includes azd-auto-approve-simple-updates (the feature branch). Should be removed before merge so the workflow only gates PRs targeting main.
• package.json main field points to pr-approval-foundry-extensions-shared.js (copy-paste leftover). Should reference ext-registry-check.js or be omitted.
• The vitest suite isn't wired into any CI workflow yet, so regressions in the check logic won't be caught automatically.
• ext-registry-check.yml is missing a trailing newline.

None of these are blockers.

[richardpark-msft]

Solid registry-gating workflow. The core logic in diffRegistry is well-structured: immutability enforcement on published releases, capability/provider diffing against the latest semver baseline, and namespace protection all look correct. The semver comparator handles prerelease labels properly, and the test suite (unit + opt-in live scenarios against real PRs) gives good coverage.

The SHA-binding fix for stale approvals and the permissions tightening both look correct in the latest commit.

A few items already flagged by earlier reviewers that are still open:

• The branches filter in ext-registry-check.yml still includes azd-auto-approve-simple-updates (the feature branch). Should be removed before merge so the workflow only gates PRs targeting main.
• package.json main field points to pr-approval-foundry-extensions-shared.js (copy-paste leftover). Should reference ext-registry-check.js or be omitted.
• The vitest suite isn't wired into any CI workflow yet, so regressions in the check logic won't be caught automatically.
• ext-registry-check.yml is missing a trailing newline.

None of these are blockers.

@jongio, tell me more about what's wrong with azd-auto-approve-simple-updates branch. I've got some other copilot feedback on it, but curious on your take. For me, it's nice as a debugging feature, but does it open up possible holes in other spots?

[jongio]

It's not a security hole. The branches filter on pull_request controls which target branches trigger the workflow. Here's why it's fine to leave for now but should be cleaned up before merge:

1. Once this PR merges and the branch is deleted, the entry is inert. No PR can target a branch that doesn't exist, so the filter never matches.
2. While the branch exists, PRs targeting it run the check, which is the correct behavior for testing.
3. If someone later recreated a branch with that name, the check would still run (not bypass), so there's no bypass vector.

The concern is purely cleanup: after merge, it's dead config. If someone reads the workflow later, they'll wonder what that branch is. Remove it before merge (or in a follow-up) so the intent is clear: this workflow gates PRs targeting main only.

Where it would matter is if you added conditional logic downstream that used the target branch as a trust signal (e.g., "skip checks for PRs targeting feature branches"). Then extra branches in the filter could widen the skip path. But nothing like that exists here today.

[richardpark-msft]

It's not a security hole. The branches filter on pull_request controls which target branches trigger the workflow. Here's why it's fine to leave for now but should be cleaned up before merge:

1. Once this PR merges and the branch is deleted, the entry is inert. No PR can target a branch that doesn't exist, so the filter never matches.
2. While the branch exists, PRs targeting it run the check, which is the correct behavior for testing.
3. If someone later recreated a branch with that name, the check would still run (not bypass), so there's no bypass vector.

The concern is purely cleanup: after merge, it's dead config. If someone reads the workflow later, they'll wonder what that branch is. Remove it before merge (or in a follow-up) so the intent is clear: this workflow gates PRs targeting main only.

Where it would matter is if you added conditional logic downstream that used the target branch as a trust signal (e.g., "skip checks for PRs targeting feature branches"). Then extra branches in the filter could widen the skip path. But nothing like that exists here today.

@jongio: There's an additional fun bit, which you could add to your reviewer-bot, that right now the workflow actually grabs the .js file from the PR branch. I'm going to modify it a bit, but tell me a bit about why that might be questionable.

[Copilot]

Review details

Files not reviewed (1)

• .github/scripts/package-lock.json: Generated file

• Files reviewed: 5/6 changed files
• Comments generated: 3
• Review effort level: Medium

[Copilot]

Review details

Files not reviewed (1)

• .github/scripts/package-lock.json: Generated file

• Files reviewed: 6/7 changed files
• Comments generated: 3
• Review effort level: Low

[jongio]

Incremental review of the changes since my last look (2a5e092 -> f3b3073).

The artifact URL check I flagged is now fixed. isAllowedArtifactURL parses with new URL() and validates the normalized origin and pathname instead of a raw prefix match, so a URL like https://github.com/Azure/azure-dev/releases/../../attacker/repo/releases/download/v1/x.zip now resolves to a path outside /Azure/azure-dev/releases/download/ and gets rejected. Host-spoofing variants (github.com.evil.com, github.com@evil.com) also fail the origin check. The new regression test covers the traversal case.

The remaining open item from my earlier comment (counting new releases per extension rather than across the whole registry) still stands as a design question, not a blocker.

[Copilot]

Review details

Files not reviewed (1)

• .github/scripts/package-lock.json: Generated file

• Files reviewed: 6/7 changed files
• Comments generated: 2
• Review effort level: Medium

[jongio]

The artifact URL hardening looks correct now. isAllowedArtifactURL rejects any percent-encoded input up front and constructs a URL so .. segments normalize before the origin and /Azure/azure-dev/releases/download/ path-prefix checks run. That closes both the ..-traversal and %2f bypasses I raised earlier, and the three new tests cover them. My earlier point about counting new releases across the whole registry is a reasonable product call to leave as-is.

One real correctness item still open before this gate runs against main: parseSemver uses withoutBuild.split('-')[1] as the prerelease, so any prerelease identifier that itself contains a hyphen gets truncated. 1.0.0-alpha-2 parses to alpha, identical to 1.0.0-alpha, which can make latestVersionBySemver select the wrong baseline and mis-order the forward-bump check. Per SemVer the prerelease is everything after the first hyphen, so coreAndPrerelease.slice(1).join('-') covers it.

[Copilot]

Review details

Files not reviewed (1)

• .github/scripts/package-lock.json: Generated file

• Files reviewed: 6/7 changed files
• Comments generated: 1
• Review effort level: Medium

[jongio]

Incremental review of the 6 commits since 2a5e092.

The artifact URL check is now correct. isAllowedArtifactURL parses the value with the URL class and compares url.origin plus the normalized url.pathname against the /Azure/azure-dev/releases/download/ prefix, and it rejects anything where decodeURIComponent changes the string. That closes both the .. path-traversal and the percent-encoded bypasses I flagged earlier. The new tests cover the resolves-outside case, the encoded dot-segment case, and the any-percent-encoding case.

The leftover feature branch was dropped from scripts-ci.yml, and node was bumped to 24 to match the runner.

One thing worth keeping in mind on the gate itself: for pull_request, ext-registry-check.yml runs from the PR head, so the YAML defining this check is PR-controlled even though the script is pinned to base.sha. That's mitigated today because /.github/ is core-team owned in CODEOWNERS, so neutering the gate in the same PR would require core-team review anyway. Only a risk if that ownership ever loosens.

Cosmetic leftover: ext-registry-check.yml still lists azd-auto-approve-simple-updates in its branches: filter. Harmless since registry PRs target main, but it can go along with the branch.

[Copilot]

Review details

Files not reviewed (1)

• .github/scripts/package-lock.json: Generated file

• Files reviewed: 6/7 changed files
• Comments generated: 1
• Review effort level: Medium

[jongio]

The artifact URL check now parses the URL and validates origin plus the /Azure/azure-dev/releases/download/ path, and rejects any percent-encoding, so the .. prefix-match bypass I flagged earlier is closed. Tests cover the traversal and encoded-traversal cases. The pull_request_target switch is safe here: checkout pins base.sha, the gate script runs from the base checkout, permissions are read-only, and PR content is fetched via the API rather than executed. Branch-filter cleanup looks good too.

[richardpark-msft]

/check-enforcer override

[jongio]

Incremental review of the 7 commits since 2a5e092.

The artifact URL check I flagged is fixed. isAllowedArtifactURL now rejects any percent-encoded input via the decodeURIComponent round-trip, then validates the normalized origin and /Azure/azure-dev/releases/download/ path through the URL class. That closes both the raw .. traversal and the %2f encoded traversal, since URL normalizes the dot segments and the decode guard blocks the encoded form. The three new tests cover the raw .., encoded ..%2f, and stray %41 cases.

The switch to pull_request_target is implemented safely. The workflow definition now comes from the base branch, so a PR can't rewrite the gate that governs its own approval. The checkout pins ref to pull_request.base.sha, there's no npm install of PR-controlled packages, github-script runs with its own bundled octokit, and permissions stay read-only (pull-requests: read, contents: read). registry.json is pulled as raw data through octokit and JSON.parsed, never executed. This also resolves the earlier concern about the workflow YAML being alterable in the PR branch.

The per-extension single-release limit is a deliberate call per your reply, so I'm not blocking on it. Approving.