🔐 feat: Recover Code API Machine Credentials After Outages - #260
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b4feb9aaf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const allowed = await this.redis.eval( | ||
| LIMIT_RECOVERY_ATTEMPTS_SCRIPT, | ||
| 1, | ||
| recoveryRateKey(workerId, 'complete'), | ||
| String(this.recovery.maxAttemptsPerMinute ?? DEFAULT_RECOVERY_ATTEMPTS_PER_MINUTE), | ||
| ); |
There was a problem hiding this comment.
Because the recovery endpoint is intentionally unauthenticated, this worker-wide counter is incremented before confirming that the supplied challenge exists or belongs to the enrollment. Anyone who knows a worker ID can continuously submit well-formed fabricated proofs (30 per minute by default), causing every legitimate signed recovery attempt to receive HTTP 429 precisely while the worker is offline. Validate and rate-limit against the stored challenge, or otherwise partition untrusted traffic so invalid requests cannot consume the enrolled worker's entire recovery budget.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed at 84608a27da8fbac9952803339bd019e626333b40. Challenge creation now requires the enrolled key to sign the server, worker, operation, timestamp, and fresh nonce; only verified requests consume the shared challenge budget. Fabricated completions do not consume the worker budget: invalid signatures are limited per unpredictable challenge, and the worker counter is charged inside the atomic Redis completion only after proof verification and the single-use check. Budgets are scoped to the enrollment generation. Tests cover repeated fabricated proofs, replay, concurrent replicas, revocation, and key replacement.
| /** Enrollment and lease credential shared only with the configured worker. */ | ||
| BRIDGE_TOKEN: process.env.CODEAPI_BRIDGE_TOKEN ?? '', | ||
| /** Opt-in stable HTTPS origin of this Code API deployment (shared by all replicas). */ | ||
| BRIDGE_RECOVERY_SERVER_ID: process.env.CODEAPI_BRIDGE_RECOVERY_SERVER_ID ?? '', |
There was a problem hiding this comment.
The documented local deployment uses .env with docker-compose up, but docker-compose.yaml explicitly forwards only the existing bridge variables in its environment sections and does not pass this new server ID or any of the associated recovery settings. Consequently, setting the documented variables in .env leaves BRIDGE_RECOVERY_SERVER_ID empty inside the API container, so recovery remains disabled and its routes return 404. Add the recovery variables to the Compose service environment and its configuration test.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed at 84608a27da8fbac9952803339bd019e626333b40. docker-compose.yaml now forwards the recovery origin, enrollment TTL, challenge TTL, and both bounded rate settings to api and service-worker, with default-off origin and tested default/override values. The primary Compose Redis now has AOF plus a named /data volume; the rollout documentation warns operators to migrate existing in-memory Redis state before recreating a running container. node tests/compose-bridge-config.cjs passes and verifies the settings do not leak to the sandbox.
|
Exact-head review handoff: Both Codex findings on the previous head have fixes and verified inline replies. The follow-up signs recovery starts before charging a worker's quota, isolates invalid completion attempts, and forwards all recovery settings in Docker Compose with persistent Redis. Focused bridge tests (62), package identity tests (2), Compose config tests, and targeted TypeScript checks passed. Full service TypeScript still reports the existing unrelated baseline diagnostics. CI is running for this head. A maintainer can trigger a new Codex review for this exact head if desired; the Lia GitHub App cannot trigger that review. I will monitor new findings and CI results. |
|
@codex review the latest head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84608a27da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| !/^[A-Za-z0-9_-]{43}$/.test(request.nonce) || | ||
| !Number.isFinite(proofTime) || | ||
| Math.abs(Date.now() - proofTime) > PROOF_CLOCK_SKEW_MS || | ||
| !verifyBridgeRecoveryStart(enrollment.publicKey, request, signature) |
There was a problem hiding this comment.
In the latest head, any internet client that knows an enrolled worker ID can submit unlimited syntactically valid requests with random signatures; each request performs Redis reads and an Ed25519 verification here, but the only start-rate counter is incremented later, after successful verification. Because the bridge router is mounted before API authentication and has no outer limiter, invalid proofs never consume any bounded bucket, allowing sustained traffic to exhaust API/Redis capacity and prevent legitimate recovery or other requests. Add a separate untrusted-traffic limit that applies before the signature work without consuming the enrolled worker's authenticated recovery budget.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed at f14811531236d49c98a246ce9795a0215a85832b. Both recovery endpoints now apply a separate Redis-backed per-connection-peer and per-operation limit before reading enrollment or verifying an Ed25519 signature. The peer comes from req.socket.remoteAddress, never X-Forwarded-For or a caller-supplied worker ID, so forged requests cannot evade the limit by changing headers or IDs. This untrusted bucket is independent of the signed worker budgets. HTTP 429 includes Retry-After; the Compose default is 240 requests/minute per peer per endpoint. Tests prove forged starts are throttled across two replicas before signature work, bogus completions are limited separately, and valid machine recovery is unaffected by the worker's signed budget. Reverse-proxy peer sharing and ingress-level client-IP/global limits are documented. 64 focused bridge tests, scoped TypeScript checks, and Compose configuration tests passed.
|
@codex review the latest head |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Summary
Codex findings addressed
Rollout and verification
Deploy compatible Code API replicas first, preserve and migrate Redis state before mounting a new volume, then enroll workers again. Browser device approval and automatic CLI reconnect are follow-up PRs; this PR does not implement user-facing login or broaden execution permissions.
packages/codeand touched service TypeScript typechecks: passed.