fix: deliver the security review findings, and correct its headline - #15
Merged
Conversation
A pool name becomes a config file path, a runner directory, a launch-agent label and a bare string in the status JSON, and none of those were checked. A name containing a double quote produced malformed JSON from 'status --json', a name containing a space broke the word splitting every 'for p in $(_rp_pool_names)' loop relies on, and '../evil' resolved outside the pool directory. Constrained to letters, digits, dot, underscore and hyphen, with '.' and '..' rejected by name because they pass a character-class test and are path hazards regardless. A case glob rather than a bash regex, since stock bash 3.2 treats a quoted right-hand side of =~ differently from an unquoted one. The comment in _rp_status_json claiming these values cannot need escaping was false for the pool name. This is what makes it true. Closes #10
curl was invoked without -f, so an HTTP error was written to the cache path and reported success. The 'already cached' test then trusted that file forever: every later tar failed and nothing said why. Verified against a nonexistent release asset, which returned exit 0, HTTP 404 and a 9-byte body. -f makes an HTTP error a failure. The download now lands under a temporary name in the same directory and is moved into place only once complete, so an interrupted fetch cannot leave a partial file behind either. Also verifies the sha256 the release publishes. The asset carries a digest field and shasum is stock on macOS, so this needs one more field in the existing filter and no new dependency. A release without a digest is skipped rather than refused. Closes #12
launchd starts the tick every 60 seconds whether or not the last one finished. A pool that is down is polled once per watched repository, so a pool watching a dozen repos against a slow GitHub can still be working when the next tick starts, and the two then interleave autoscale and sweep over the same activity timestamp. mkdir is the lock: atomic on every POSIX filesystem, and macOS has no flock. A lock older than 900s is treated as left behind by a killed process and broken, so a crash cannot wedge the scheduler permanently. Released via a trap so an error path cannot leak it. Closes #13
install.sh seeded the config with a plain cp, so the default umask of 022 left it readable by every user on the machine. That is the same file the example tells you to put RUNPOOL_WEBHOOK_TOKEN in. The example's header now says to keep it 0600 too, which covers the Homebrew path: the formula does not create the config, it tells you to copy it. Closes #11
…roups Two changes to who owns the public-repository check, because the two scopes are genuinely different. REPOSITORY scope is RunPool's, since GitHub has no per-repository equivalent. The check compared against PUBLIC and nothing else, so an empty answer from a failed API call fell straight through to registration: the one control between a stranger's pull request and a shell here was skipped exactly when GitHub was unreliable. It now treats PRIVATE and INTERNAL as allowed, PUBLIC as refused, and anything else as a refusal that says visibility could not be determined. The refusal also becomes a default rather than an absolute. --allow-public proceeds with a warning that states the risk. A refusal with no way past it just moves the decision somewhere less visible, like a forked copy of the tool or a hand-registered runner. ORGANISATION scope is GitHub's, and it already defaults safely. A runner group carries allows_public_repositories, it is false by default, and runners land in the default group because config.sh is never passed --runnergroup, so a public repo in the org does not get them. register --org now reads that setting and warns only when it has been turned on, says so plainly when it cannot be read, and never fails closed on it. It deliberately does not enumerate the org's public repositories to re-derive an answer GitHub already has. Closes #8 Closes #9
The README, AGENTS.md and the skill all said public repositories are refused, without qualifying that the check only exists at repository scope. An earlier draft of the security review read that as an org-scope hole; it is not. The control at org scope is GitHub's runner-group allows_public_repositories, which defaults to false. The docs were wrong about whose job it is, not about whether it is done. All three now say which layer owns which, and mention --allow-public. SECURITY.md states the two places RunPool knowingly differs from GitHub's hardening guidance: persistent runners, chosen for warm caches, and fork pull requests on private repositories, where the control is GitHub's per-repo approval setting and RunPool cannot enforce it. It also records why JIT tokens are not used, so the question does not get relitigated: JIT is for ephemeral runners, and RunPool registers persistently. Closes #14
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.
Delivers the findings from the v0.5.0 security review, and corrects the review's headline claim.
The correction, first
The review said organisation pools have no public-repository protection. That is wrong, and the error was not checking the runner group.
allows_public_repositoriesis GitHub's own control and it defaults tofalse. A public repository in the organisation is not handed those runners. The review overstated the exposure by looking only at RunPool's code and not at the layer beneath it.That settles the design question of whose job this is, which is what most of this PR turns on:
allows_public_repositoriesand warns only if it has been switched on. It deliberately does not enumerate an organisation's public repositories to re-derive an answer GitHub already has.The asymmetry between the two branches is intentional.
AGENTS.mdnow says so, so it does not get "evened up" later.Changes
035c3efregister44a57cad4df674ec2959b6e3852072172aeSECURITY.md, and docs that no longer overclaimThe refusal becomes a default rather than an absolute.
--allow-publicproceeds with a warning that states the risk. A wall with no gate just moves the decision somewhere less visible, like a forked copy of the tool or a hand-registered runner, and "refuses by default" is a claim the code can actually keep.One thing beyond the brief, flag it if unwanted. The fetch fix also verifies the SHA256. The release asset carries a
digestfield andshasumis stock on macOS, so it cost one extra field in the existing--jqfilter and no new dependency. A release without a digest is skipped rather than refused.Verification
Both checks CI runs, plus behaviour. Nothing here was asserted without being run.
--severity=warningvia the documented Docker image: both clean.RUNPOOL_BASEwith a stubbedgh, so no real API call and nothing could register: public refused with the escape hatch named; public plus--allow-publicwarns then proceeds; private and internal pass; unresolvable visibility refuses; orgtruewarns; orgfalsesilent; org unreadable says so and does not fail closed. Confirmed afterwards that no runner directory or pool config was created by any of it.../evil,two words,bad"name,.andhas/slashall refused; a valid name proceeds;status --jsonstill parses.Live pools were never touched. All testing used a scratch base.
marfaserved four jobs throughout and both pools are at their original counts.Not done, on purpose
JIT tokens. JIT config is for ephemeral runners that take one job and deregister. RunPool registers persistently, so adopting it would mean re-registering on every job: a different tool with a different lifecycle. The exposure it would reduce is a one-hour token that RunPool never writes to disk. Recorded in
SECURITY.mdso it does not come back around.Closes #8, closes #9, closes #10, closes #11, closes #12, closes #13, closes #14.