sjawhar-legion-105: Per-role GitHub App credential isolation for workers#111
sjawhar-legion-105: Per-role GitHub App credential isolation for workers#111
Conversation
sjawhar
force-pushed
the
sjawhar-legion-105
branch
2 times, most recently
from
6963363 to
3174bc2
Compare
Behavioral Test ResultsSummaryFAIL — Spec compliance check failed. Environment skipped (no point smoke-testing code that doesn't implement the spec). The implementation provides process-level separation (separate Spec Compliance Results
Critical Finding: Isolation Model Does Not Meet SpecThe direction change comment states:
The implementation violates this in three ways: 1. Credential endpoint is unauthenticated ( 2. Private keys on shared filesystem ( 3. Same OS user, shared filesystem ( The docs ( Bug: CLI --env Never Sent to Daemon
Test Quality Critiquegithub-apps.test.ts (248 lines) — Solid
multi-serve.test.ts (179 lines) — Shallow
server.test.ts (new additions) — Good but gaps
cli/index.test.ts (new additions) — Missed the bug
Documentation Feedback
Observations
|
sjawhar
force-pushed
the
sjawhar-legion-105
branch
from
3174bc2 to
85abcd2
Compare
Behavioral Test Results (Re-test after fixes)SummaryPASS — 8/8 acceptance criteria verified. All three previous failures fixed. 859 tests pass, tsc clean, biome clean. Previous Failures — Now Fixed
Spec Compliance Results
Build Results
Live Smoke Tests
Test Quality Critique (Updated)Improvements since last review:
Remaining gaps (P2):
Documentation Feedback
Observations
|
sjawhar
left a comment
sjawhar
left a comment
There was a problem hiding this comment.
Review Summary
CRITICAL (P1): 2 issues
IMPORTANT (P2): 5 issues
MINOR (P3): 2 suggestions
Verdict: Changes requested — 2 critical issues must be addressed before merge.
Architecture Assessment
The overall design is solid. Process-level isolation via per-role serves is a meaningful improvement over the status quo. Token management with RS256 JWT, lazy-refresh caching, and concurrent request deduplication is well-implemented. Backward compatibility (fallback to shared serve) is handled cleanly. Test coverage is thorough — good tests for env scrubbing, token caching, deduplication, env leak prevention on API endpoints.
CI is green (lint, test, typecheck all pass).
Critical Issues
-
Setup guide references a removed API endpoint —
GET /credentials/{role}was intentionally removed (test verifies 404), butdocs/setup/github-apps.mdStep 5 still tells users to curl it for verification. -
GET /workers/{id}/envundermines credential isolation — This unauthenticated endpoint returns any worker's credentials (including auto-injectedGH_TOKEN) to any local caller. A worker on theimplserve can read thereviewworker's token, bypassing process-level isolation. The direction change on #105 explicitly states: "a misbehaving worker process must not be able to escalate to another role's credentials."
Additional Notes (P3, not in inline comments)
- Token expiry for long-running serves: Role serve processes keep the token from startup in their env. Tokens expire after 1 hour. The health loop restarts unhealthy serves but doesn't proactively refresh tokens before expiry. Long-running workers (>55 min) could hit expired tokens.
- No test for token refresh near expiry: TokenManager tests verify caching and deduplication but don't test the 5-minute refresh window behavior (token within REFRESH_WINDOW_MS of expiry should trigger regeneration).
docs/setup/github-apps.md
Outdated
| DAEMON_PORT=13370 # or your configured port | ||
|
|
||
| # Test impl credentials | ||
| curl -s http://127.0.0.1:$DAEMON_PORT/credentials/impl | jq . |
There was a problem hiding this comment.
P1/CRITICAL: This endpoint (GET /credentials/{role}) was removed — the test GET /credentials/{role} endpoint is removed in server.test.ts explicitly expects 404. Users following this setup guide will hit a dead end at Step 5.
Fix: Replace with a verification method that actually works, e.g.:
- Dispatch a test worker and check its env:
POST /workersthenGET /workers/{id}/env - Add a lightweight status endpoint that reports configured roles without exposing tokens (e.g.,
GET /github-apps/status→{"impl": true, "review": true, "ops": false})
packages/daemon/src/daemon/server.ts
Outdated
| if (!entry) { | ||
| return notFound(); | ||
| } | ||
| return jsonResponse({ env: entry.env ?? {} }); |
There was a problem hiding this comment.
P1/CRITICAL: This endpoint returns any worker's env (including auto-injected GH_TOKEN) to any local caller without authentication. A worker on the impl serve can call:
curl http://127.0.0.1:$DAEMON_PORT/workers/{review-worker-id}/envand get the review role's token, bypassing process-level isolation.
Fix options (in order of simplicity):
- Filter credentials from response — strip
GH_TOKEN,GIT_AUTHOR_*,GIT_COMMITTER_*from the response. Workers on multi-serve already get tokens from the serve process environment; the/envendpoint is only needed for manually-passed--envvars. - Don't store auto-injected credentials in the worker entry — only pass them to the serve process env.
- Scope the endpoint — verify the caller is the worker itself (requires identity proof).
Option 1 is simplest and preserves backward compat for the --env flag.
packages/daemon/src/daemon/server.ts
Outdated
| GIT_COMMITTER_EMAIL: cred.gitIdentity.email, | ||
| }; | ||
| } | ||
| } catch { |
There was a problem hiding this comment.
P2/IMPORTANT: Token generation failures are silently swallowed. If getToken() fails (bad key file, GitHub API outage, expired installation), the worker silently falls back to ambient credentials and runs under the wrong identity — with no indication in logs.
Fix: Add console.error("Failed to inject role credentials for ${mode}: ${error}") in this catch block.
| opts.getWorkerAdapter?.(mode as WorkerModeLiteral) ?? opts.adapter; | ||
|
|
||
| // Auto-inject role credentials when GitHub Apps configured | ||
| let workerEnv: Record<string, string> | undefined = isRecord(envPayload) |
There was a problem hiding this comment.
P2/IMPORTANT: isRecord(envPayload) validates it's a non-null object but doesn't check that all values are strings. The CLI validates via parseEnvJson, but direct API calls could pass {"KEY": 123} — stored and returned as-is.
Fix: Extract the string-value validation from parseEnvJson into a shared helper and use it here, or return badRequest("env values must be strings") on violation.
| # If no CRITICAL issues — mark ready for merge (approved): | ||
| gh pr ready "$LEGION_ISSUE_ID" | ||
| # Check if we have a separate identity (GH_TOKEN set by daemon) | ||
| if [ -n "$GH_TOKEN" ]; then |
There was a problem hiding this comment.
P2/IMPORTANT: [ -n "$GH_TOKEN" ] doesn't distinguish App tokens from personal tokens. If a user has GH_TOKEN set for other purposes AND Apps are NOT configured, the reviewer will attempt gh pr review --approve/--request-changes with the personal token — which fails because GitHub blocks self-review.
Fix options:
- Check for a daemon-injected marker env var (e.g.,
LEGION_APP_ROLE=review) — set during credential injection - Check if the token starts with
ghs_(installation tokens use this prefix) - Fall through to legacy path if
gh pr reviewreturns an error
| * Environment variables to strip from role serves to prevent credential leakage. | ||
| * Workers should only have access to their role's GH_TOKEN. | ||
| */ | ||
| const SCRUBBED_ENV_KEYS = ["GH_TOKEN", "GITHUB_TOKEN", "GH_HOST", "GH_CONFIG_DIR", "HOME"]; |
There was a problem hiding this comment.
P2/IMPORTANT: Scrubbing HOME entirely could break tools that depend on it (npm cache, pip, jj config, Node.js module resolution). Many Unix tools fall back to /etc/passwd when HOME is unset, which gives the real user's home — the same location you're trying to protect.
Fix: Set HOME to a safe, role-specific directory (e.g., /tmp/legion-${role}) instead of removing it. This preserves tool compatibility while preventing ~/.config/gh/ credential access.
|
|
||
| // Check role serves health | ||
| if (roleServeManager?.hasRoleServes()) { | ||
| const unhealthyRoles = await roleServeManager.checkHealth(); |
There was a problem hiding this comment.
P2/IMPORTANT: After restarting a role serve, active worker sessions are lost — but unlike the shared serve restart path (which re-creates sessions from the state file), there's no session re-creation step here.
Workers on the restarted serve will fail on their next operation. The controller will eventually mark them dead and re-dispatch, but this causes unnecessary delay.
Fix: After restartRole(), read the state file, find workers whose mode maps to this role (via modeToRole()), and call adapter.createSession() on the restarted serve.
Add per-role GitHub App authentication with process-level isolation:
- JWT generation (RS256, Web Crypto) and installation token management
- Per-role serve instances with scrubbed environments (GH_TOKEN, GH_CONFIG_DIR)
- RoleServeManager routes workers to role-specific serves by mode
- GET /credentials/{role} and GET /workers/{id}/env daemon endpoints
- Native gh pr review --approve/--request-changes for reviewer identity
- Controller skill docs for credential dispatch with --env flag
- CLI --env JSON flag for dispatch command
- Setup guide for creating and configuring GitHub Apps
Closes #105
sjawhar
force-pushed
the
sjawhar-legion-105
branch
from
85abcd2 to
e5cdc0f
Compare
sjawhar
left a comment
sjawhar
left a comment
There was a problem hiding this comment.
Re-Review Summary
All previous findings have been addressed.
CRITICAL (P1): 0 remaining (2 fixed)
IMPORTANT (P2): 0 remaining (5 fixed)
MINOR (P3): 1 new observation
Verdict: Approved ✅
Previous Findings — Verification
| # | Severity | Finding | Status |
|---|---|---|---|
| 1 | P1 | Setup guide references removed /credentials/{role} endpoint |
✅ Fixed — Step 5 now uses POST /workers + GET /workers/{id}/env + daemon log inspection |
| 2 | P1 | GET /workers/{id}/env leaks credentials cross-role |
✅ Fixed — endpoint now strips GH_TOKEN, GIT_AUTHOR_*, GIT_COMMITTER_*, LEGION_APP_ROLE. Only manually-passed env vars returned. Test coverage added. |
| 3 | P2 | Silent credential injection failure | ✅ Fixed — console.error("Failed to inject role credentials for ${mode}: ...") added |
| 4 | P2 | Missing env value validation in daemon API | ✅ Fixed — string-value validation with badRequest('env values must be strings (key "${k}")'). Test added. |
| 5 | P2 | Fragile $GH_TOKEN identity check in review workflow |
✅ Fixed — now checks $LEGION_APP_ROLE = "review" (daemon-injected marker). LEGION_APP_ROLE also injected in credential flow (server.ts). |
| 6 | P2 | HOME scrubbed entirely from role serve env | ✅ Fixed — HOME = /tmp/legion-${role}. Test verifies HOME = "/tmp/legion-impl". |
| 7 | P2 | Missing session re-creation after role serve restart | ✅ Fixed — health loop now reads state file and re-creates sessions for workers mapped to the restarted role via modeToRole(). |
New Observation (P3)
Session re-creation in index.ts extracts mode via workerEntry.id.split("-").pop(), while server.ts has a more robust extractModeFromWorkerId() helper that iterates all modes and checks endsWith("-${mode}"). Both work for the common case, but the existing helper handles edge cases better. Not blocking — the try/catch handles any mismatches.
CI Status
All checks passing: lint ✅, test ✅, typecheck ✅
| const state = await resolvedDeps.readStateFile(config.stateFilePath); | ||
| for (const workerEntry of Object.values(state.workers)) { | ||
| try { | ||
| const workerMode = workerEntry.id.split("-").pop(); |
There was a problem hiding this comment.
P3/MINOR: Mode extraction here uses workerEntry.id.split("-").pop(), while server.ts has an existing extractModeFromWorkerId() helper that iterates all valid modes and checks endsWith("-${mode}"). The existing helper is slightly more robust for edge cases (e.g., issue IDs containing mode-like suffixes).
Not blocking — the try/catch handles any mismatches gracefully. Consider using the existing helper in a follow-up for consistency.
|
Superseded by updated PR with smoke-test fixes: removed HOME override (broke mise/tool managers), consolidated ops role into review (2 Apps instead of 3). |
1 participant
Closes #105
Summary
Per-role GitHub App authentication with process-level credential isolation — each worker role (impl, review, ops) runs in its own
opencode serveprocess with role-specificGH_TOKENand scrubbed environment.Key changes:
github-apps.ts): RS256 JWT generation via Web Crypto, installation token exchange,TokenManagerwith lazy-refresh caching (5-min window),modeToRole()mapping,buildRoleEnv()environment scrubbingmulti-serve.ts):RoleServeManagercreates per-roleopencode serveinstances with isolated credentials. Workers are routed to the appropriate serve by mode→role mapping. Falls back to shared serve when Apps aren't configured (backward compatible)GET /credentials/{role}endpoint for token retrieval,GET /workers/{id}/envfor per-worker env, health loop monitors and restarts unhealthy role servesgh pr review --approve/--request-changeswhen running as separate identity--envflag for dispatch with credential injection--env '{"KEY":"VALUE"}'JSON flag for dispatch commanddocs/setup/github-apps.md)Isolation model:
Private keys never leave the daemon process. Workers only receive short-lived installation tokens via env vars.
GH_CONFIG_DIR=/dev/nullprevents access to personalghcredentials.Test results: