Skip to content
Closed
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
327 changes: 327 additions & 0 deletions .github/workflows/validate-pr.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,327 @@
name: Validate PR

on:
pull_request_target:
types: [opened, reopened]

jobs:
validate-non-maintainer-pr:
name: Validate Non-Maintainer PR
runs-on: ubuntu-24.04
permissions:
pull-requests: write
contents: write
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unnecessary contents: write permission grants excessive access

Medium Severity

Both jobs declare contents: write permission, but no step in either job modifies repository file contents. The workflow only interacts with PRs (comments, labels, state changes, draft conversion) which are covered by pull-requests: write. Granting contents: write on a pull_request_target trigger — which runs in the context of the base branch — unnecessarily broadens the attack surface.

Additional Locations (1)
Fix in Cursor Fix in Web

outputs:
was-closed: ${{ steps.validate.outputs.was-closed }}
steps:
- name: Generate GitHub App token
id: app-token
uses: actions/create-github-app-token@f8d387b68d61c58ab83c6c016672934102569859 # v2
with:
app-id: ${{ vars.SDK_MAINTAINER_BOT_APP_ID }}
private-key: ${{ secrets.SDK_MAINTAINER_BOT_PRIVATE_KEY }}

- name: Validate PR
id: validate
uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1
with:
github-token: ${{ steps.app-token.outputs.token }}
script: |
const pullRequest = context.payload.pull_request;
const repo = context.repo;
const prAuthor = pullRequest.user.login;
const contributingUrl = `https://github.com/${repo.owner}/${repo.repo}/blob/${context.payload.repository.default_branch}/CONTRIBUTING.md`;

// --- Helper: check if a user has admin or maintain permission on a repo (cached) ---
const maintainerCache = new Map();
async function isMaintainer(owner, repoName, username) {
const key = `${owner}/${repoName}:${username}`;
if (maintainerCache.has(key)) return maintainerCache.get(key);
let result = false;
try {
const { data } = await github.rest.repos.getCollaboratorPermissionLevel({
owner,
repo: repoName,
username,
});
// permission field uses legacy values (admin/write/read/none) where
// maintain maps to write. Use role_name for the actual role.
result = ['admin', 'maintain'].includes(data.role_name);
} catch {
// noop — result stays false
}
maintainerCache.set(key, result);
return result;
}

// --- Step 1: Check if PR author is a maintainer (admin or maintain role) ---
const authorIsMaintainer = await isMaintainer(repo.owner, repo.repo, prAuthor);
if (authorIsMaintainer) {
core.info(`PR author ${prAuthor} has admin/maintain access. Skipping.`);
return;
}
core.info(`PR author ${prAuthor} is not a maintainer.`);

// --- Step 2: Parse issue references from PR body ---
const body = pullRequest.body || '';

// Match all issue reference formats:
// #123, Fixes #123, getsentry/repo#123, Fixes getsentry/repo#123
// https://github.com/getsentry/repo/issues/123
const issueRefs = [];
const seen = new Set();

// Pattern 1: Full GitHub URLs
const urlPattern = /https?:\/\/github\.com\/(getsentry)\/([\w.-]+)\/issues\/(\d+)/gi;
for (const match of body.matchAll(urlPattern)) {
const key = `${match[1]}/${match[2]}#${match[3]}`;
if (!seen.has(key)) {
seen.add(key);
issueRefs.push({ owner: match[1], repo: match[2], number: parseInt(match[3]) });
}
}

// Pattern 2: Cross-repo references (getsentry/repo#123)
const crossRepoPattern = /(?:(?:fix|fixes|fixed|close|closes|closed|resolve|resolves|resolved)\s+)?(getsentry)\/([\w.-]+)#(\d+)/gi;
for (const match of body.matchAll(crossRepoPattern)) {
const key = `${match[1]}/${match[2]}#${match[3]}`;
if (!seen.has(key)) {
seen.add(key);
issueRefs.push({ owner: match[1], repo: match[2], number: parseInt(match[3]) });
}
}

// Pattern 3: Same-repo references (#123)
// Negative lookbehind to avoid matching cross-repo refs or URLs already captured
const sameRepoPattern = /(?:(?:fix|fixes|fixed|close|closes|closed|resolve|resolves|resolved)\s+)?(?<![/\w])#(\d+)/gi;
for (const match of body.matchAll(sameRepoPattern)) {
const key = `${repo.owner}/${repo.repo}#${match[1]}`;
if (!seen.has(key)) {
seen.add(key);
issueRefs.push({ owner: repo.owner, repo: repo.repo, number: parseInt(match[1]) });
}
}

core.info(`Found ${issueRefs.length} issue reference(s): ${[...seen].join(', ')}`);

// --- Helper: close PR with comment and labels ---
async function closePR(message, reasonLabel) {
await github.rest.issues.addLabels({
...repo,
issue_number: pullRequest.number,
labels: ['violating-contribution-guidelines', reasonLabel],
});

await github.rest.issues.createComment({
...repo,
issue_number: pullRequest.number,
body: message,
});

await github.rest.pulls.update({
...repo,
pull_number: pullRequest.number,
state: 'closed',
});

core.setOutput('was-closed', 'true');
}

// --- Step 3: No issue references ---
if (issueRefs.length === 0) {
core.info('No issue references found. Closing PR.');
await closePR([
'This PR has been automatically closed. All non-maintainer contributions must reference an existing GitHub issue.',
'',
'**Next steps:**',
'1. Find or open an issue describing the problem or feature',
'2. Discuss the approach with a maintainer in the issue',
'3. Once a maintainer has acknowledged your proposed approach, open a new PR referencing the issue',
'',
`Please review our [contributing guidelines](${contributingUrl}) for more details.`,
].join('\n'), 'missing-issue-reference');
return;
}

// --- Step 4: Validate each referenced issue ---
// A PR is valid if ANY referenced issue passes all checks.
let hasAssigneeConflict = false;
let hasNoDiscussion = false;

for (const ref of issueRefs) {
core.info(`Checking issue ${ref.owner}/${ref.repo}#${ref.number}...`);

let issue;
try {
const { data } = await github.rest.issues.get({
owner: ref.owner,
repo: ref.repo,
issue_number: ref.number,
});
issue = data;
} catch (e) {
core.warning(`Could not fetch issue ${ref.owner}/${ref.repo}#${ref.number}: ${e.message}`);
continue;
}

// Check assignee: if assigned to someone other than PR author, flag it
if (issue.assignees && issue.assignees.length > 0) {
const assignedToAuthor = issue.assignees.some(a => a.login === prAuthor);
if (!assignedToAuthor) {
core.info(`Issue ${ref.owner}/${ref.repo}#${ref.number} is assigned to someone else.`);
hasAssigneeConflict = true;
continue;
}
}

// Check discussion: both PR author and a maintainer must have commented
const comments = await github.paginate(github.rest.issues.listComments, {
owner: ref.owner,
repo: ref.repo,
issue_number: ref.number,
per_page: 100,
});

// Also consider the issue author as a participant (opening the issue is a form of discussion)
// Guard against null user (deleted/suspended GitHub accounts)
const prAuthorParticipated =
issue.user?.login === prAuthor ||
comments.some(c => c.user?.login === prAuthor);

let maintainerParticipated = false;
if (prAuthorParticipated) {
// Check each commenter (and issue author) for admin/maintain access on the issue's repo
const usersToCheck = new Set();
if (issue.user?.login) usersToCheck.add(issue.user.login);
for (const comment of comments) {
if (comment.user?.login && comment.user.login !== prAuthor) {
usersToCheck.add(comment.user.login);
}
}

for (const user of usersToCheck) {
if (user === prAuthor) continue;
if (await isMaintainer(repo.owner, repo.repo, user)) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The maintainer check incorrectly validates against the PR's repository instead of the referenced issue's repository, breaking cross-repo logic.
Severity: CRITICAL

Suggested Fix

Change the call to isMaintainer to use the properties of the referenced issue's repository. Replace isMaintainer(repo.owner, repo.repo, user) with isMaintainer(ref.owner, ref.repo, user).

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: .github/workflows/validate-pr.yml#L204

Potential issue: When checking for maintainer participation on a linked issue, the logic
incorrectly checks for maintainer status on the pull request's repository instead of the
issue's repository. The code at line 204 calls `isMaintainer(repo.owner, repo.repo,
user)`, but for cross-repository issue references, it should check against the issue's
repository using `ref.owner` and `ref.repo`. This bug will cause the workflow to
incorrectly close valid pull requests that reference issues in other repositories where
a maintainer of that other repository has participated.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maintainer check uses PR repo instead of issue repo

Low Severity

The isMaintainer call passes repo.owner, repo.repo (the PR's repository) but the comment on line 193 says it checks "admin/maintain access on the issue's repo." For cross-repo references (e.g., an issue in getsentry/sentry referenced from a PR on getsentry/sentry-native), this checks maintainer status on the wrong repo. If intentional, the comment is misleading; if the comment reflects intent, ref.owner and ref.repo are needed instead.

Fix in Cursor Fix in Web

maintainerParticipated = true;
core.info(`Maintainer ${user} participated in ${ref.owner}/${ref.repo}#${ref.number}.`);
break;
}
}
}

if (prAuthorParticipated && maintainerParticipated) {
core.info(`Issue ${ref.owner}/${ref.repo}#${ref.number} has valid discussion. PR is allowed.`);
return; // PR is valid — at least one issue passes all checks
}

core.info(`Issue ${ref.owner}/${ref.repo}#${ref.number} lacks discussion between author and maintainer.`);
hasNoDiscussion = true;
}

// --- Step 5: No valid issue found — close with the most relevant reason ---
if (hasAssigneeConflict) {
core.info('Closing PR: referenced issue is assigned to someone else.');
await closePR([
'This PR has been automatically closed. The referenced issue is already assigned to someone else.',
'',
'If you believe this assignment is outdated, please comment on the issue to discuss before opening a new PR.',
'',
`Please review our [contributing guidelines](${contributingUrl}) for more details.`,
].join('\n'), 'issue-already-assigned');
return;
}

if (hasNoDiscussion) {
core.info('Closing PR: no discussion between PR author and a maintainer in the referenced issue.');
await closePR([
'This PR has been automatically closed. The referenced issue does not show a discussion between you and a maintainer.',
'',
'To avoid wasted effort on both sides, please discuss your proposed approach in the issue first and wait for a maintainer to respond before opening a PR.',
'',
`Please review our [contributing guidelines](${contributingUrl}) for more details.`,
].join('\n'), 'missing-maintainer-discussion');
return;
}

// If we get here, all issue refs were unfetchable
core.info('Could not validate any referenced issues. Closing PR.');
await closePR([
'This PR has been automatically closed. The referenced issue(s) could not be found.',
'',
'**Next steps:**',
'1. Ensure the issue exists and is in a `getsentry` repository',
'2. Discuss the approach with a maintainer in the issue',
'3. Once a maintainer has acknowledged your proposed approach, open a new PR referencing the issue',
'',
`Please review our [contributing guidelines](${contributingUrl}) for more details.`,
].join('\n'), 'missing-issue-reference');

enforce-draft:
name: Enforce Draft PR
needs: [validate-non-maintainer-pr]
if: |
always()
&& github.event.pull_request.draft == false
&& needs.validate-non-maintainer-pr.outputs.was-closed != 'true'
runs-on: ubuntu-24.04
permissions:
pull-requests: write
contents: write
steps:
- name: Generate GitHub App token
id: app-token
uses: actions/create-github-app-token@f8d387b68d61c58ab83c6c016672934102569859 # v2
with:
app-id: ${{ vars.SDK_MAINTAINER_BOT_APP_ID }}
private-key: ${{ secrets.SDK_MAINTAINER_BOT_PRIVATE_KEY }}

- name: Convert PR to draft
env:
GH_TOKEN: ${{github.token}}
PR_URL: ${{ github.event.pull_request.html_url }}
run: |
gh pr ready "$PR_URL" --undo
Comment on lines +279 to +283
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The workflow uses the default github.token to convert a PR to a draft, which lacks the required permissions, causing the step to fail.
Severity: HIGH

Suggested Fix

In the "Convert PR to draft" step, change the GH_TOKEN environment variable to use the generated app token: GH_TOKEN: ${{ steps.app-token.outputs.token }}.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: .github/workflows/validate-pr.yml#L279-L283

Potential issue: The "Convert PR to draft" step in the `enforce-draft` job uses the
default `GH_TOKEN: ${{github.token}}`. This token lacks the necessary permissions to
change a pull request's draft status via the GraphQL API, which the `gh` CLI uses
internally. This will cause the step to fail with a "Resource not accessible by
integration" error whenever a non-draft PR is submitted. The workflow correctly
generates a more privileged GitHub App token in the preceding step
(`steps.app-token.outputs.token`) but fails to use it for this operation.


- name: Label and comment
uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1
with:
github-token: ${{ steps.app-token.outputs.token }}
script: |
const pullRequest = context.payload.pull_request;
const repo = context.repo;

// Label the PR so maintainers can filter/track violations
await github.rest.issues.addLabels({
...repo,
issue_number: pullRequest.number,
labels: ['converted-to-draft'],
});

// Check for existing bot comment to avoid duplicates on reopen
const comments = await github.rest.issues.listComments({
...repo,
issue_number: pullRequest.number,
});
const botComment = comments.data.find(c =>
c.user.type === 'Bot' &&
c.body.includes('automatically converted to draft')
);
if (botComment) {
core.info('Bot comment already exists, skipping.');
return;
}

const contributingUrl = `https://github.com/${repo.owner}/${repo.repo}/blob/${context.payload.repository.default_branch}/CONTRIBUTING.md`;

await github.rest.issues.createComment({
...repo,
issue_number: pullRequest.number,
body: [
`This PR has been automatically converted to draft. All PRs must start as drafts per our [contributing guidelines](${contributingUrl}).`,
'',
'**Next steps:**',
'1. Ensure CI passes',
'2. Fill in the PR description completely',
'3. Mark as "Ready for review" when you\'re done'
].join('\n')
});
Loading