[NoQA] Add GitHub Action to validate contributor PRs#84703
[NoQA] Add GitHub Action to validate contributor PRs#84703MonilBhavsar merged 6 commits intomainfrom
Conversation
Auto-close PRs from non-org members that don't link to a GitHub issue where the author is assigned, or a GitHub PR where the author is the author or reviewer. Co-authored-by: Michał Jasikowski <mjasikowski@users.noreply.github.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5ac125ca2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| on: | ||
| pull_request_target: | ||
| types: [opened, ready_for_review] |
There was a problem hiding this comment.
This workflow only runs on opened and ready_for_review, so a contributor whose PR was auto-closed can immediately reopen it and bypass this validation because pull_request_target emits a separate reopened event that is currently ignored. This leaves unauthorized PRs open until manual intervention and undermines the gate this workflow introduces.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. Added reopened to the event types so validation also fires when a previously closed PR is reopened. The trigger line is now:
types: [opened, ready_for_review, reopened]However, the push is currently blocked because the CI token lacks the workflow scope required to update .github/workflows/ files. The commit is ready on the branch locally — it needs a token with workflow scope or a manual push to land.
| const cleanBody = prBody.replace(/<!--[\s\S]*?-->/g, ''); | ||
|
|
||
| // Step 2: Check linked issues — is the PR author assigned to any? | ||
| const issuePattern = /https:\/\/github\.com\/([^/]+\/[^/]+)\/issues\/(\d+)/g; |
There was a problem hiding this comment.
The issue matcher accepts any GitHub repository (owner/repo), and the PR is authorized if the author is assigned on that issue, so an external user can satisfy this check by linking a self-assigned issue in their own public repository. That bypasses the contribution policy described in the PR template/comment, which expects project issues, and makes the guard ineffective.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — this was a real bypass vector. Fixed by restricting the issue URL regex to only match Expensify/ org repos:
// Before (any repo):
const issuePattern = /https:\/\/github\.com\/([^/]+\/[^/]+)\/issues\/(\d+)/g;
// After (Expensify org only):
const issuePattern = /https:\/\/github\.com\/(Expensify\/[^/]+)\/issues\/(\d+)/g;Now links to issues in external repos are simply ignored by the matcher.
| } | ||
|
|
||
| // Step 3: Check linked PRs — is the PR author the author or a reviewer? | ||
| const prUrlPattern = /https:\/\/github\.com\/([^/]+\/[^/]+)\/pull\/(\d+)/g; |
There was a problem hiding this comment.
The PR matcher also accepts any GitHub repository, and this check authorizes when the contributor is the linked PR author/reviewer; in practice, anyone can open a PR in their own repo and link it here to pass validation. Since this workflow is meant to enforce contribution eligibility for this project, unrestricted cross-repo PR links create a straightforward bypass path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same fix applied here — the PR URL regex now only matches repos under the Expensify org:
// Before (any repo):
const prUrlPattern = /https:\/\/github\.com\/([^/]+\/[^/]+)\/pull\/(\d+)/g;
// After (Expensify org only):
const prUrlPattern = /https:\/\/github\.com\/(Expensify\/[^/]+)\/pull\/(\d+)/g;Links to PRs in external repos are now ignored.
|
@MelvinBot address reviewer comments |
|
Addressed all three review comments:
|
|
@MelvinBot restrict PR link to Expensify/App |
|
Restricted the PR-link regex to only match // Before (any repo):
const prUrlPattern = /https:\/\/github\.com\/([^/]+\/[^/]+)\/pull\/(\d+)/g;
// After (Expensify/App only):
const prUrlPattern = /https:\/\/github\.com\/(Expensify\/App)\/pull\/(\d+)/g;The commit ( |
|
@MelvinBot how to manually push your commit? |
|
The push failed because the CI token doesn't have the 1. Checkout the branch locally: gh pr checkout 847032. Apply these three changes to a) Line 5 — add # Before:
types: [opened, ready_for_review]
# After:
types: [opened, ready_for_review, reopened]b) Line 38 — restrict issue regex to Expensify org: // Before:
const issuePattern = /https:\/\/github\.com\/([^/]+\/[^/]+)\/issues\/(\d+)/g;
// After:
const issuePattern = /https:\/\/github\.com\/(Expensify\/[^/]+)\/issues\/(\d+)/g;c) Line 62 — restrict PR regex to Expensify org: // Before:
const prUrlPattern = /https:\/\/github\.com\/([^/]+\/[^/]+)\/pull\/(\d+)/g;
// After:
const prUrlPattern = /https:\/\/github\.com\/(Expensify\/[^/]+)\/pull\/(\d+)/g;3. Commit and push: git add .github/workflows/validateContributorPR.yml
git commit -m "Fix: Add reopened trigger and restrict repo scope to Expensify org"
git pushYour local Git credentials should have the necessary |
|
@MelvinBot how you were able to commit this file in the initial PR, but not able to do it in subsequent commits? Are you sure you can't? |
|
Good question. I investigated and here's what I found: Why the initial commit worked but subsequent pushes don't: The environment has two tokens:
The initial PR creation run likely used the installation token for git operations. GitHub App tokens can modify workflow files without needing a separate I tried several approaches to work around this:
The fix for this is one of:
|
|
@codex review |
|
I investigated the failing Problem: The workflow file uses Fix: Replace the mutable tag with the pinned SHA used across the rest of the repository: - uses: actions/github-script@v7
+ uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cdin
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1238234b1f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| permissions: | ||
| pull-requests: write | ||
| issues: read |
There was a problem hiding this comment.
Update the workflow permissions before shipping this, because Step 4 calls github.rest.issues.createComment but the job only grants issues: read. On unauthorized PRs this comment call will return a 403 (Resource not accessible by integration), and since it happens before pulls.update, the script exits early and the PR is never closed, which breaks the main enforcement path of this workflow.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — issues.createComment requires issues: write, and with only read the workflow would 403 on that call and never actually close the PR. Fixed issues: read → issues: write in the permissions block (commit 208e0d2).
workflow scope required by GitHub to push changes to .github/workflows/ files. All three commits are ready on the branch locally and need to be pushed with a token that has workflow scope.
|
The push failed because GitHub requires a Personal Access Token (PAT) with the Here's how to push the change manually: Option 1: Cherry-pick the local commit (if you have the branch checked out) # Fetch and checkout the PR branch
gh pr checkout 84703
# The commit should already be there if fetched; if not, cherry-pick it
# Then push with your PAT (which has workflow scope)
git pushOption 2: Make the changes yourself directly gh pr checkout 84703Then edit
types: [opened, ready_for_review, reopened]
const issuePattern = /https:\/\/github\.com\/(Expensify\/[^/]+)\/issues\/(\d+)/g;
const prUrlPattern = /https:\/\/github\.com\/(Expensify\/[^/]+)\/pull\/(\d+)/g;Then commit and push: git add .github/workflows/validateContributorPR.yml
git commit -m "Fix: add reopened trigger and restrict repo scope to Expensify org"
git pushYour PAT needs the
|
|
@MelvinBot Change the PR, so the it only validates the conditions for FIRST_TIME_CONTRIBUTOR association - allowing CONTRIBUTOR to skip the conditions. |
|
Made the requested change — added - // Step 1: Allow org members and repo owners
- if (['MEMBER', 'OWNER'].includes(authorAssociation)) {
- console.log(`${prAuthor} is an org ${authorAssociation}. No validation needed.`);
+ // Step 1: Allow org members, repo owners, and returning contributors
+ if (['MEMBER', 'OWNER', 'CONTRIBUTOR'].includes(authorAssociation)) {
+ console.log(`${prAuthor} is a known ${authorAssociation}. No validation needed.`);
|
|
@MonilBhavsar this should be ready to review |
|
No C+ review needed, it's a CI change |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f04300353
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (linkedPR.user.login.toLowerCase() === prAuthor.toLowerCase()) { | ||
| console.log(`${prAuthor} is the author of ${repo}#${linkedPRNumber}. PR is authorized.`); | ||
| return; |
There was a problem hiding this comment.
This check authorizes contributors solely for being the author of any linked Expensify/* PR, which creates a bypass: an external user can reopen a closed PR, add a link to that same PR (or any throwaway PR they opened in a public Expensify repo), and this early return keeps the PR open without issue assignment or member status. Because pull_request_target runs on reopened, this path is directly exploitable and undermines the gate’s purpose.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot address this. If you're unable to commit, let me know what code to change.
There was a problem hiding this comment.
Good catch — this is a real bypass vector. Removed the self-authored PR authorization path entirely. Step 3 now only authorizes via reviewer or requested-reviewer status on a linked Expensify PR.
Changes needed (I can't push because the CI token lacks workflow scope):
1. Remove the pulls.get() call and authorship check (delete lines 68-76 in the current file):
- const {data: linkedPR} = await github.rest.pulls.get({
- owner,
- repo: repoName,
- pull_number: linkedPRNumber,
- });
- if (linkedPR.user.login.toLowerCase() === prAuthor.toLowerCase()) {
- console.log(`${prAuthor} is the author of ${repo}#${linkedPRNumber}. PR is authorized.`);
- return;
- }
-2. Update the Step 3 comment (line 60):
- // Step 3: Check linked PRs — is the PR author the author or a reviewer?
+ // Step 3: Check linked PRs — is the PR author a reviewer?
+ // Only authorize via reviewer status, not PR authorship (anyone can open a PR in a public Expensify repo)3. Update the log message (line 98):
- console.log(`${prAuthor} is not author or reviewer of ${repo}#${linkedPRNumber}.`);
+ console.log(`${prAuthor} is not a reviewer of ${repo}#${linkedPRNumber}.`);4. Update the closing comment (line 118):
- '- No linked GitHub PR was found in the PR description where you are the author or a reviewer',
+ '- No linked GitHub PR was found in the PR description where you are a reviewer',|
I removed linked PR checks - no first time contributors will ever be reviewers - and if they are authors of a merged PR, they no longer are first time contributors |
This comment was marked as off-topic.
This comment was marked as off-topic.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
This is QAd already:
|
|
🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 9.3.36-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.3.36-10 🚀
|
Explanation of Change
Adds a new GitHub Action (
validateContributorPR.yml) that automatically validates PRs from external contributors. When a non-draft PR is opened or a draft PR is marked as ready for review, this action checks:author_association) or not a first time contributor (i.e. has at least one merged PR), the PR is allowedDesign decisions:
pull_request_targetevent for write access to close PRs from forks, and for security (runs base branch code, not PR code)actions/github-script, making this the lightest/fastest possible actionuser.type != 'Bot') and draft PRsFixed Issues
$ https://github.com/Expensify/Expensify/issues/609533
Tests
This is a CI workflow change (no application code modified). Validated by:
opened,ready_for_review)author_associationcheck)$ https://github.com/Expensify/App/issues/<id>)Offline tests
N/A — this is a GitHub Actions workflow, not application code.
QA Steps
N/A — this is a CI workflow. To validate:
Feel free to tag @mjasikowski to help with QA.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
N/A — CI workflow change only, no UI changes