Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(DO NOT MERGE) Testing CI checks III #1472

Open
wants to merge 3 commits into
base: fdc3-for-web-impl
Choose a base branch
from

Conversation

kriswest
Copy link
Contributor

Describe your change

Testing CI check when run on a PR from a repository fork (which are failing at the last step, posting a comment with test results)

@kriswest kriswest requested a review from a team as a code owner December 13, 2024 15:26
@kriswest
Copy link
Contributor Author

kriswest commented Dec 13, 2024

No dice @robmoffat, this PR doesn't touch the workflow file at all, it's got the pull request perms in it already, but it still fails.

This thread from the mocha team explains the issue well, we're getting a read-only token as the run is being triggered by a PR from a fork: mochajs/mocha#4553

There is more from Github (at https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/) that explains possible workarounds using the pull_request_target workflow trigger - but also that we probably shouldn't use it as it will mean 3rd parties can execute code in the context of the repository. I've only skim-read it, there may be more.

As the bit that is failing is the posting of the comment, a way forward would be to just remove that. Let the check pass or fail and we rely on the check's status indicator rather than a posted message. That seems sufficient. However, I f you wreally want to retain the comment you could do so by just inhibiting it for PRs from forks, which you can detect by comparing the head and base repos in the PR, as they do here: danger/danger-js#918 (comment)

Feel free to close this PR once you've had a read of this.

Copy link

424 passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant