Repository navigation
fix(orchestrator): refuse jobs whose run comes from a fork - #26
Merged
Merged
Conversation
A pull request runs its own copy of the workflow, so a fork can rewrite runs-on to ask for any gcrunner label, and gcrunner provisioned every queued job with one. An expression in runs-on that sends fork pull requests to GitHub-hosted runners only steered well-behaved ones. Before resolving the runner, the orchestrator now looks up the job's run and refuses it unless its head repository is the repository itself (a deleted fork leaves none, and is refused too). The refusal goes through the configuration-error path: a failed gcrunner check saying why, and the run cancelled. A failed lookup is retried like any other GitHub error. Tests reach the lookup through runSource, which TestMain points at the job's own repository, so the tests that are not about it need no GitHub API.
2 tasks
🔵 Tier A · Mergeable after minor fixes
The orchestrator checks each queued job’s workflow-run source and rejects runs originating from a different repository before runner resolution or VM provisioning. It caches run-source lookups and coalesces concurrent lookups, adds fake-GitHub tests for the protection and cache behavior, and documents the fork-run policy and fallback workflow expression. Latest changes: The newest commits add a six-hour run-source cache with singleflight coalescing, plus tests for cache expiry and retrying failed lookups.
Fix with agent prompt### Issue 1
orchestrator/run_source_test.go:87-90
**Exercise concurrent run-source lookups**
These calls are sequential, so they only verify cache hits; a regression that stops coalescing simultaneous jobs would still pass. The comparable config cache has a concurrent-miss test, so a blocked fake lookup with overlapping callers would verify this new guarantee.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.📂 Walkthrough · 5
Reviewed the commits since |
Production queues about 2,500 jobs an hour, many sharing a run. The head repository of a run never changes, so lookups are cached per run for six hours and concurrent jobs of one run share a single request. Failed lookups are not cached.
![hansi-codes[bot]](https://avatars.githubusercontent.com/in/5046327?s=60&v=4){kind=small}
Comment on lines
+87
to
+90
| for id := int64(1); id <= 3; id++ { | ||
| event := WorkflowJobEvent{WorkflowJob: WorkflowJob{ID: id, RunID: 7}, Repository: cloudRepository} | ||
| if err := checkRunSource(context.Background(), event); err != nil { | ||
| t.Fatalf("job %d refused: %v", id, err) |
There was a problem hiding this comment.
Exercise concurrent run-source lookups
These calls are sequential, so they only verify cache hits; a regression that stops coalescing simultaneous jobs would still pass. The comparable config cache has a concurrent-miss test, so a blocked fake lookup with overlapping callers would verify this new guarantee.
Prompt To Fix With AI
This is a comment left during a code review.
Path: orchestrator/run_source_test.go
Line: 87-90
Comment:
**Exercise concurrent run-source lookups**
These calls are sequential, so they only verify cache hits; a regression that stops coalescing simultaneous jobs would still pass. The comparable config cache has a concurrent-miss test, so a blocked fake lookup with overlapping callers would verify this new guarantee.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟡 Minor · testing · Reply if this doesn't apply.
1 of 2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A pull request runs its own copy of the workflow, so a fork can rewrite
runs-onto ask for any gcrunner label. The orchestrator checked the webhook signature and the label, then created a VM: on a public repository, anyone could run code on a VM in our project by opening a pull request. Theruns-onexpressionappwrite/appwriteuses to send fork pull requests toubuntu-24.04only steers well-behaved ones; it lives in the fork's copy of the workflow. Raised by hansi-codes on appwrite/helm-chart#18 (thread), which moves that repository to gcrunner shortly before it goes public.Before resolving the runner,
provisionJobnow looks up the job's run (GET /repos/{owner}/{repo}/actions/runs/{id}, already covered byactions: write) and refuses the job unlesshead_repositoryis the repository itself, compared case-insensitively. A run with no head repository (the fork was deleted) is refused too. The refusal goes through the existing configuration-error path: a failedgcrunnercheck on the commit saying why, and the run cancelled. A failed lookup is a normal GitHub error and is retried.website/content/docs/security.mdgets a Jobs From Forks section with the fallback expression.Testing
go vetandgo test -count=1 ./...pass.-raceon the new tests is clean.TestARunFromAnotherRepositoryGetsNoVMfails.runSource, whichTestMainpoints at the job's own repository.After deploying
comes from: a refused job logsJob N: configuration error: run R comes from <fork> …, failing the run. Onappwrite/appwrite, fork pull requests already ask forubuntu-24.04, so there should be none unless a fork changedruns-on./task/queuedlatency grows by one GitHub request for the first job of each run.