Repository navigation
fix: require executor key for runtime command execution - #257
Merged
Merged
Conversation
POST /v1/runtimes/:runtimeId/commands was registered without the api group, so the OPR_EXECUTOR_SECRET init hook never ran and the route accepted unauthenticated docker-exec into any runtime. Co-authored-by: Jake Barnby <abnegate@users.noreply.github.com>
Co-authored-by: Jake Barnby <abnegate@users.noreply.github.com>
Co-authored-by: Jake Barnby <abnegate@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The route-auth test silently skips a route declared at end-of-file, weakening the security regression guard.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Secures runtime command execution with executor-key authentication.
Changes:
- Adds
apiandruntimesgroups to the commands route. - Tests unauthorized command requests.
- Adds a route-authentication regression test.
| File | Description |
|---|---|
app/controllers.php |
Protects command execution. |
tests/e2e/ExecutorTest.php |
Verifies unauthorized requests return 401. |
tests/unit/Http/RouteAuthTest.php |
Checks route group declarations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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
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.

Summary
POST /v1/runtimes/:runtimeId/commandswas registered without an HTTP group, so theOPR_EXECUTOR_SECRETinit hook — which is scoped to theapigroup — never ran. Every other sensitive executor route already declaresgroups(['api', ...]). The commands route therefore accepted unauthenticated requests and could execute commands inside any runtime container.This is a security fix for Help Scout report HS 1468629 (reporter Kikoichi).
Change
groups(['api', 'runtimes'])toPOST /v1/runtimes/:runtimeId/commands, matching sibling per-runtime routes (/logs,/executions, get/delete runtime).401 Missing executor key./v1/healthstays ungrouped so Docker healthchecks remain unauthenticated.A fail-closed rewrite of the init hook (auth unless a route opts out) was considered. Health was intentionally removed from the
apigroup so compose healthchecks work without a secret; making init global would have to special-case that route and risk changing unrelated 404/health behavior. The missing group was the sole bypass. A unit test now fails if any non-health route is declared withoutapi, so this class of omission cannot land again.Tests
testCommandsUnauthorized— empty executor key on/commandsreturns401withMissing executor key, same contract astestGetRuntimesUnauthorized.RouteAuthTest— every declared HTTP route except/v1/healthincludes theapigroup.Do not include reproduction payloads beyond the 401 assertion. After merge, rotate
OPR_EXECUTOR_SECRETon any deployment that exposed the executor on a shared runtimes network.