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

fix: preview-web can't be run on forks #84

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

pfeerick
Copy link
Member

GitHub repo SECRETS are not exposed to forks, thus the preview-web step can't be run on pull requests from forks.

It may be possible to use a different event trigger... pull_request_target ... in order to get around this, but this then also changes the context of the base of the pull request, from it being the merge commit, to that of the base of the pull request, and am not sure if this will have unexpected consequences, hence this more immediate quick fix.

GitHub repo SECRETS are not exposed to forks, thus the preview-web step can't be run on pull requests from forks.
Copy link

codecov bot commented Jun 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 39.54%. Comparing base (21b749c) to head (37c1dbb).

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #84   +/-   ##
=======================================
  Coverage   39.54%   39.54%           
=======================================
  Files         103      103           
  Lines       12006    12006           
  Branches      645      645           
=======================================
  Hits         4748     4748           
  Misses       7251     7251           
  Partials        7        7           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@freshollie
Copy link
Collaborator

@pfeerick this makes sense, you could also try implement something like this https://michaelheap.com/access-secrets-from-forks/

@pfeerick
Copy link
Member Author

Oh... thanks for that ... will look at that tomorrow :)

@pfeerick pfeerick marked this pull request as draft June 17, 2024 09:46
@pfeerick
Copy link
Member Author

pfeerick commented Jun 20, 2024

Ok, either something has changed in github, or that method only works for checkout... so it looks like the only real way to do this is how github themselves recommended it... by using a split workflow... one being the "insecure", and the second the "secure" one. Thankfully the current structure of the CI build seems to be suited to being split right down the middle where it's needed, so it shouldn't be that hard to simply split it in two.

https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

@pfeerick
Copy link
Member Author

pfeerick commented Jun 20, 2024

Ok, I'm done for today 🤕 Both stage 1 and stage 2 appear to be working as intended.

stage 1 - test-and-compile - https://github.com/pfeerick/buddy/actions/runs/9591978099
stage 2 - build-app-and-preview - https://github.com/pfeerick/buddy/actions/runs/9592007656
stage 3 - release - https://github.com/pfeerick/buddy/actions/runs/9592184521

stage 2 was expected to fail where it did as while I gave it a valid cloudflare API and re-ran just that failed step, I don't have a mirror of the buddy project setup, so it merely progressed further than the first run.

stage 3 appears to have correctly skipped due to the failure of the prior stage.

Now sort of wondering if I should split it down a bit further... i.e. five workflows ... test, compile, app, web, release ... or maybe rename... test, build, release? 🤔

@pfeerick pfeerick marked this pull request as ready for review July 9, 2024 01:19
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.

2 participants