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.

Sign up for GitHub

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

Jump to bottom

Split validation workflow #10

Open
joyeecheung opened this issue Dec 3, 2024 · 4 comments · Fixed by #31
Open

Split validation workflow #10

joyeecheung opened this issue Dec 3, 2024 · 4 comments · Fixed by #31
Labels
good first issue Good for newcomers

Comments

@joyeecheung
Copy link
Member

joyeecheung commented Dec 3, 2024 •
edited
Loading

As mentioned in #5 (comment)

...apparently the secrets are not passed into workflow runs triggered by PRs from forks. So I guess that means we should split the validation into two parts:

  1. If it's triggered by a PR from a branch in this repo (which means it must be from someone who already has write access here), it can invoke additional validation to check the URLs are reachable and valid (which requires access to bluesky secrets).
  2. Otherwise, only local validations are run on the JSON files.

This means:

  1. Split everything below

    bluesky/actions/login-and-validate.js

    Line 21 in dbdab89

    const agent = await login(account);
    to a separate file
  2. Splitting https://github.com/nodejs/bluesky/blob/main/.github/workflows/validate.yml into two workflows, one local and one remote. PRs started from a branch in this repo get both run for them, and PRs from forks just run the one that's without the login.
@joyeecheung joyeecheung added the good first issue Good for newcomers label Dec 3, 2024
@Ishan-Jadhav
Copy link

Hi joyeecheung,
Can I work on this issue?

@joyeecheung
Copy link
Member Author

Sure, go ahead please!

@joyeecheung joyeecheung mentioned this issue Dec 11, 2024
Merged
@aduh95 aduh95 linked a pull request Dec 18, 2024 that will close this issue
fix: validation of PRs from forks #31
Merged
@joyeecheung joyeecheung reopened this Jan 9, 2025
@joyeecheung
Copy link
Member Author

Reopened because of #55 - I think for maximum safety, we should still split the validation workflows and only invoke validation that needs secrets for PRs opened from this repo.

joyeecheung added a commit that referenced this issue Jan 9, 2025
@joyeecheung
Use pull_request in validation workflow
a63f85a 
See #55 - using pull_request_target would allow the workflow to run without authorization. While there are some code in this workflow to defend against naive attacks e.g. adding scripts to the actions, there could be other attack vectors e.g. via specially crafted branch names or file names that evade GitHub's escape rules. It would be too hard to wrap our head around this, so the easiest way to defend against it would be to restrict full validation that require access to secrets to PRs opened from branches in this repo.

For PRs opened from forks, I think we should go back to what I proposed in #10 - that is, only run local validations that require no access to secrets, and use `pull_request` for it.

cc @aduh95 @nodejs/tsc
@joyeecheung joyeecheung mentioned this issue Jan 9, 2025
Merged
joyeecheung added a commit that referenced this issue Jan 9, 2025
@joyeecheung
Use pull_request in validation workflow (#56)
f8e9222 
See #55 - using pull_request_target would allow the workflow to run without authorization. While there are some code in this workflow to defend against naive attacks e.g. adding scripts to the actions, there could be other attack vectors e.g. via specially crafted branch names or file names that evade GitHub's escape rules. It would be too hard to wrap our head around this, so the easiest way to defend against it would be to restrict full validation that require access to secrets to PRs opened from branches in this repo.

For PRs opened from forks, I think we should go back to what I proposed in #10 - that is, only run local validations that require no access to secrets, and use `pull_request` for it.

cc @aduh95 @nodejs/tsc
@aduh95
Copy link
Contributor

aduh95 commented Jan 9, 2025

Another idea would be to use a Merge Queue; the downside is we can't know there's a failure until we attempt to land the PR – basically, the process would be if a PR looks good enough to a reviewer, they add it to the merge queue (the usual green merge button on GH UI), and either the validation succeeds and the PR is merged, either it fails and the PR stays open.

Ideally we would not handle just the validation from the queue but also the actual processing, I'm not sure if that would work, I guess I can try and see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: validation of PRs from forks aduh95/bluesky
3 participants
@joyeecheung @aduh95 @Ishan-Jadhav