CI: Skip nx workflow on forks due to missing secrets#33471
Conversation
Forks don't have access to repository secrets (NX_CLOUD_ACCESS_TOKEN), so the nx workflow would fail. This change ensures the workflow only runs on PRs from the main repository. In the future, we plan to use pull_request_target to safely run on forks while still having access to secrets.
There was a problem hiding this comment.
Pull request overview
This PR prevents the nx workflow from running on pull requests from forks, which would otherwise fail due to missing access to the NX_CLOUD_ACCESS_TOKEN secret. The change adds a condition to check if the PR originates from the main repository before executing the workflow.
- Adds a repository check condition to the nx workflow job to skip execution on fork PRs
- Includes a TODO comment indicating future plans to use
pull_request_targetfor safer fork support
| nx: | ||
| if: > | ||
| (github.event_name == 'pull_request' && | ||
| github.event.pull_request.head.repo.full_name == github.repository && |
There was a problem hiding this comment.
The condition to check if a PR is from a fork is incorrect. The current condition github.event.pull_request.head.repo.full_name == github.repository will fail when the head repository is null (which happens when a fork is deleted). Additionally, there's a more reliable property specifically designed for this check.
According to GitHub Actions documentation and the pattern used in trigger-circle-ci-workflow.yml (line 28), you should use github.event.pull_request.head.repo.fork instead. This property is a boolean that explicitly indicates whether the PR is from a fork.
The condition should be changed to check that the fork property is false (or the property doesn't exist for same-repo PRs). For example:
github.event.pull_request.head.repo.fork != true
This approach is more robust and handles edge cases like deleted forks better.
| github.event.pull_request.head.repo.full_name == github.repository && | |
| github.event.pull_request.head.repo.fork != true && |
📝 WalkthroughWalkthroughThe PR modifies the GitHub Actions workflow to restrict the nx job execution to pull requests originating from the same repository, preventing execution on forked PRs. A comment documenting a future plan to use Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/nx.yml (1)
7-7: Consider documenting security implications in the TODO.The TODO mentions using
pull_request_targetfor fork support, which is a good future direction. However,pull_request_targetrequires careful implementation to avoid exposing secrets to untrusted code from forks. Consider expanding this comment to note that any future implementation should ensure secrets are not exposed during checkout or build steps from the PR.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/nx.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
.github/workflows/nx.yml (1)
23-30: LGTM! Repository check correctly prevents fork PR execution.The addition of
github.event.pull_request.head.repo.full_name == github.repositoryon line 25 correctly filters out PRs from forks, preventing failures due to the missingNX_CLOUD_ACCESS_TOKENsecret. The logic preserves execution for same-repository PRs, push events, and scheduled runs.
|
View your CI Pipeline Execution ↗ for commit 510bd7f
☁️ Nx Cloud last updated this comment at |
What I did
Forks don't have access to repository secrets (NX_CLOUD_ACCESS_TOKEN), so the nx workflow would fail. This change ensures the workflow only runs on PRs from the main repository.
In the future, we plan to use
pull_request_targetto safely run on forks while still having access to secrets.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This is a CI workflow change. No manual testing is necessary - the workflow will simply be skipped for fork PRs.
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
- `bug`: Internal changes that fixes incorrect behavior. - `maintenance`: User-facing maintenance tasks. - `dependencies`: Upgrading (sometimes downgrading) dependencies. - `build`: Internal-facing build tooling & test updates. Will not show up in release changelog. - `cleanup`: Minor cleanup style change. Will not show up in release changelog. - `documentation`: Documentation **only** changes. Will not show up in release changelog. - `feature request`: Introducing a new feature. - `BREAKING CHANGE`: Changes that break compatibility in some way with current major version. - `other`: Changes that don't fit in the above categories.