Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 113 additions & 7 deletions .github/scripts/agents-guard.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ const path = require('path');
const DEFAULT_MARKER = '<!-- agents-guard-marker -->';

const DEFAULT_PROTECTED_PATHS = ['.github/workflows/agents-*.yml'];
const DEPENDENCY_UPDATE_BOT_LOGINS = new Set(['dependabot[bot]', 'renovate[bot]']);
const TRUSTED_DEPENDENCY_AUTHOR_ASSOCIATIONS = new Set(['OWNER', 'MEMBER', 'COLLABORATOR']);
const LEGACY_ALLOW_REMOVED_PATHS = [
// Keepalive consolidation retired the standalone keepalive sweeps.
'.github/workflows/agents-75-keepalive-on-gate.yml',
Expand Down Expand Up @@ -396,6 +398,83 @@ function extractLabelNames(labels) {
);
}

function parseActionReferenceLine(line) {
const match = String(line || '').match(/^\s*(?:-\s*)?uses:\s*["']?([^@\s#'"]+)@([^\s#'"]+)["']?(?:\s*(?:#.*)?)?$/i);
if (!match) {
return null;
}

return {
action: match[1].toLowerCase(),
ref: match[2],
};
Comment on lines +401 to +410

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reject expression-based refs in dependency-only parsing.

The current regex accepts non-literal refs (for example ${{...}}), which can be treated as dependency-only and bypass CODEOWNER even though that is workflow logic, not a static ref bump.

Suggested hardening
 function parseActionReferenceLine(line) {
   const match = String(line || '').match(/^\s*(?:-\s*)?uses:\s*["']?([^@\s#'"]+)@([^\s#'"]+)["']?(?:\s*(?:#.*)?)?$/i);
   if (!match) {
     return null;
   }
 
+  const action = match[1];
+  const ref = match[2];
+  if (ref.includes('${{') || ref.includes('}}')) {
+    return null;
+  }
+
   return {
-    action: match[1].toLowerCase(),
-    ref: match[2],
+    action: action.toLowerCase(),
+    ref,
   };
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
function parseActionReferenceLine(line) {
const match = String(line || '').match(/^\s*(?:-\s*)?uses:\s*["']?([^@\s#'"]+)@([^\s#'"]+)["']?(?:\s*(?:#.*)?)?$/i);
if (!match) {
return null;
}
return {
action: match[1].toLowerCase(),
ref: match[2],
};
function parseActionReferenceLine(line) {
const match = String(line || '').match(/^\s*(?:-\s*)?uses:\s*["']?([^@\s#'"]+)@([^\s#'"]+)["']?(?:\s*(?:#.*)?)?$/i);
if (!match) {
return null;
}
const action = match[1];
const ref = match[2];
if (ref.includes('${{') || ref.includes('}}')) {
return null;
}
return {
action: action.toLowerCase(),
ref,
};
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/scripts/agents-guard.js around lines 401 - 410, The regex pattern in
the parseActionReferenceLine function accepts expression-based refs like
${{...}} in the second capture group (the ref part), which should be rejected to
ensure only literal static refs are parsed. Modify the regex pattern to exclude
refs containing expression syntax, or add validation logic after the match to
reject any refs that contain $ or {{ patterns, ensuring that only literal
version tags and commit SHAs are treated as static dependency refs rather than
workflow logic expressions.

}

function sameActionSequence(left, right) {
if (left.length !== right.length) {
return false;
}

for (let index = 0; index < left.length; index += 1) {
if (left[index].action !== right[index].action) {
return false;
}
}

return true;
}

function sameActionRefSequence(left, right) {
if (left.length !== right.length) {
return false;
}

for (let index = 0; index < left.length; index += 1) {
if (left[index].action !== right[index].action || left[index].ref !== right[index].ref) {
return false;
}
}

return true;
}

function patchChangesOnlyActionReferences(patch) {
if (!patch || typeof patch !== 'string') {
return false;
}

const removedRefs = [];
const addedRefs = [];
for (const rawLine of patch.split(/\r?\n/)) {
if (!rawLine || rawLine.startsWith('+++') || rawLine.startsWith('---')) {
continue;
}

const marker = rawLine[0];
if (marker !== '+' && marker !== '-') {
continue;
}

const parsed = parseActionReferenceLine(rawLine.slice(1));
if (!parsed) {
return false;
}

if (marker === '-') {
removedRefs.push(parsed);
} else {
addedRefs.push(parsed);
}
}

if (removedRefs.length === 0 || addedRefs.length === 0) {
return false;
}

return sameActionSequence(removedRefs, addedRefs) &&
!sameActionRefSequence(removedRefs, addedRefs);
}

function evaluateGuard({
files = [],
labels = [],
Expand All @@ -404,6 +483,7 @@ function evaluateGuard({
protectedPaths = DEFAULT_PROTECTED_PATHS,
labelName = 'agents:allow-change',
authorLogin = '',
authorAssociation = '',
marker = DEFAULT_MARKER,
repository = process.env.GITHUB_REPOSITORY || '',
} = {}) {
Expand Down Expand Up @@ -524,16 +604,37 @@ function evaluateGuard({
}

const normalizedAuthor = authorLogin ? String(authorLogin).toLowerCase() : '';
const authorIsCodeowner = normalizedAuthor && codeownerLogins.has(normalizedAuthor);
const normalizedAuthorAssociation = authorAssociation
? String(authorAssociation).toUpperCase()
: '';
const authorIsCodeowner = Boolean(
normalizedAuthor && codeownerLogins.has(normalizedAuthor),
);
const hasExternalApproval = [...codeownerLogins].some((login) => approvedLogins.has(login));
const hasCodeownerApproval = hasExternalApproval || authorIsCodeowner;
const hasCodeownerApproval = Boolean(hasExternalApproval || authorIsCodeowner);

const hasProtectedChanges = modifiedProtectedPaths.size > 0;
// Security note: Allow `agents:allow-change` label to bypass CODEOWNER approval
// ONLY for automated dependency PRs from known bots (dependabot, renovate).
// Human PRs or other bot PRs still require CODEOWNER approval even with label.
const isAutomatedPR = normalizedAuthor && (normalizedAuthor === 'dependabot[bot]' || normalizedAuthor === 'renovate[bot]');
const needsApproval = hasProtectedChanges && !hasCodeownerApproval && !(hasAllowLabel && isAutomatedPR);
const protectedChangesAreDependencyOnly = hasProtectedChanges && relevantFiles
.filter((file) => file.status === 'modified' && matchProtectedPath(file.filename || ''))
.every((file) => patchChangesOnlyActionReferences(file.patch || ''));
const isDependencyUpdateBot = Boolean(
Comment on lines 616 to +620

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Treat added protected workflows as protected changes.

hasProtectedChanges only tracks status === 'modified', so adding a new .github/workflows/agents-*.yml file can bypass both label and CODEOWNER gating entirely.

Suggested fix
-  const hasProtectedChanges = modifiedProtectedPaths.size > 0;
-  const protectedChangesAreDependencyOnly = hasProtectedChanges && relevantFiles
-    .filter((file) => file.status === 'modified' && matchProtectedPath(file.filename || ''))
-    .every((file) => patchChangesOnlyActionReferences(file.patch || ''));
+  const changedProtectedFiles = relevantFiles.filter((file) => {
+    const current = file.filename || '';
+    const previous = file.previous_filename || '';
+    const isProtected = Boolean(
+      matchProtectedPath(current) || (previous ? matchProtectedPath(previous) : null),
+    );
+    return isProtected && (file.status === 'modified' || file.status === 'added');
+  });
+  const hasProtectedChanges = changedProtectedFiles.length > 0;
+  const protectedChangesAreDependencyOnly = hasProtectedChanges && changedProtectedFiles.every(
+    (file) => file.status === 'modified' && patchChangesOnlyActionReferences(file.patch || ''),
+  );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const hasProtectedChanges = modifiedProtectedPaths.size > 0;
// Security note: Allow `agents:allow-change` label to bypass CODEOWNER approval
// ONLY for automated dependency PRs from known bots (dependabot, renovate).
// Human PRs or other bot PRs still require CODEOWNER approval even with label.
const isAutomatedPR = normalizedAuthor && (normalizedAuthor === 'dependabot[bot]' || normalizedAuthor === 'renovate[bot]');
const needsApproval = hasProtectedChanges && !hasCodeownerApproval && !(hasAllowLabel && isAutomatedPR);
const protectedChangesAreDependencyOnly = hasProtectedChanges && relevantFiles
.filter((file) => file.status === 'modified' && matchProtectedPath(file.filename || ''))
.every((file) => patchChangesOnlyActionReferences(file.patch || ''));
const isDependencyUpdateBot = Boolean(
const changedProtectedFiles = relevantFiles.filter((file) => {
const current = file.filename || '';
const previous = file.previous_filename || '';
const isProtected = Boolean(
matchProtectedPath(current) || (previous ? matchProtectedPath(previous) : null),
);
return isProtected && (file.status === 'modified' || file.status === 'added');
});
const hasProtectedChanges = changedProtectedFiles.length > 0;
const protectedChangesAreDependencyOnly = hasProtectedChanges && changedProtectedFiles.every(
(file) => file.status === 'modified' && patchChangesOnlyActionReferences(file.patch || ''),
);
const isDependencyUpdateBot = Boolean(
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/scripts/agents-guard.js around lines 616 - 620, The
hasProtectedChanges variable only considers files with status 'modified',
allowing newly added protected workflow files to bypass protection checks.
Update the logic that sets hasProtectedChanges to also include files with status
'added' that match protected paths using matchProtectedPath. Additionally,
update the protectedChangesAreDependencyOnly filter condition to include both
'modified' and 'added' files when checking if protected changes only contain
dependency references via patchChangesOnlyActionReferences.

normalizedAuthor && DEPENDENCY_UPDATE_BOT_LOGINS.has(normalizedAuthor),
);
const authorCanUseDependencyBypass = Boolean(
isDependencyUpdateBot ||
(normalizedAuthorAssociation &&
TRUSTED_DEPENDENCY_AUTHOR_ASSOCIATIONS.has(normalizedAuthorAssociation)),
);
// Security note: the `agents:allow-change` label can bypass CODEOWNER approval
// only for protected workflow dependency reference updates. This keeps routine
// action version alignment moving while arbitrary workflow logic edits still
// require CODEOWNER review.
const hasDependencyUpgradeBypass = Boolean(
hasAllowLabel &&
authorCanUseDependencyBypass &&
protectedChangesAreDependencyOnly,
);
const needsApproval = hasProtectedChanges && !hasCodeownerApproval && !hasDependencyUpgradeBypass;
const needsLabel = hasProtectedChanges && !hasAllowLabel && !hasCodeownerApproval;

const failureReasons = [];
Expand Down Expand Up @@ -619,6 +720,10 @@ function evaluateGuard({
authorIsCodeowner,
needsLabel,
needsApproval,
hasDependencyUpgradeBypass,
protectedChangesAreDependencyOnly,
isDependencyUpdateBot,
authorCanUseDependencyBypass,
modifiedProtectedPaths: [...modifiedProtectedPaths],
touchedProtectedPaths: [...touchedProtectedPaths],
fatalViolations,
Expand All @@ -633,6 +738,7 @@ module.exports = {
evaluateGuard,
parseCodeowners,
globToRegExp,
patchChangesOnlyActionReferences,
validatePullRequestTargetSafety,
detectPullRequestTargetViolations,
};
3 changes: 3 additions & 0 deletions .github/workflows/agents-guard.yml
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,8 @@ jobs:
const authorLogin =
context.payload.pull_request.user &&
context.payload.pull_request.user.login;
const authorAssociation =
context.payload.pull_request.author_association || '';
const { owner, repo } = context.repo;
const baseRef = context.payload.pull_request.base.sha;
const labelName = 'agents:allow-change';
Expand Down Expand Up @@ -420,6 +422,7 @@ jobs:
protectedPaths,
labelName,
authorLogin,
authorAssociation,
marker,
});

Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/maint-76-claude-code-review.yml
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ jobs:
- name: Run Claude Code Review
id: claude
continue-on-error: true
uses: anthropics/claude-code-action@806af32823ef69c8ef357086c573a902af641307 # v1
uses: anthropics/claude-code-action@51705da45eecce209d4700538bf8377d5b5fc695 # v1
with:
claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }}
allowed_bots: '*'
Expand Down
4 changes: 2 additions & 2 deletions WORKFLOW_USER_GUIDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ Issue: "Add user authentication"
| `agent:needs-attention` | Human intervention required |
| `agents:auto-pilot-pause` | Auto-pilot paused |
| `agents:auto-pilot-failed` | Auto-pilot stopped due to errors |
| `agents:allow-change` | Permission signal for `agents-guard`; bypasses CODEOWNER approval only for automated dependency PRs from Dependabot/Renovate. Auto-applied by `maint-auto-label-dep-prs.yml`; manual application does not bypass guard enforcement. |
| `agents:allow-change` | Permission signal for `agents-guard`; bypasses guard-level CODEOWNER approval only for dependency-only `uses:` reference updates from Dependabot/Renovate or repository owner/member/collaborator PRs. Auto-applied to dependency-bot PRs by `maint-auto-label-dep-prs.yml`; arbitrary workflow logic edits still require review. |
| `needs-human` | Escalated to human |
| `follow-up` | Created as follow-up to another issue/PR |
| `duplicate` | Potential duplicate detected |
Expand Down Expand Up @@ -850,7 +850,7 @@ The Workflows repository includes maintenance workflows that handle sync, update
**Trigger:** When Dependabot or Renovate opens a PR

**Labels Applied:**
- `agents:allow-change` (so protected-workflow changes can be reviewed without manual label work)
- `agents:allow-change` (so dependency-bot protected-workflow version updates can use the guarded dependency-update lane)

**Use When:** Automatic, no action needed

Expand Down
Loading