fix(oauth): recover missing client registrations - #135
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe change adds configurable OAuth client registration keys, signed recoverable client IDs, SQLite restoration for missing registrations, provider integration, and tests and documentation for recovery and legacy compatibility. ChangesOAuth client recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OAuthProvider
participant SqliteOAuthClientsStore
participant recoverClientRegistration
participant SqliteOAuthStore
OAuthProvider->>SqliteOAuthClientsStore: authorize with client ID
SqliteOAuthClientsStore->>recoverClientRegistration: recover client ID with key
recoverClientRegistration-->>SqliteOAuthClientsStore: validated client registration
SqliteOAuthClientsStore->>SqliteOAuthStore: restore client after redirect validation
SqliteOAuthStore-->>OAuthProvider: available client registration
OAuthProvider-->>OAuthProvider: issue authorization code
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR adds HMAC-authenticated, self-contained identifiers for public OAuth clients so missing SQLite registrations can be recovered after owner approval.
Confidence Score: 5/5The PR appears safe to merge, with no concrete blocking or independently actionable non-blocking defects identified. Recoverable registrations are authenticated, schema-validated, restricted to public clients, checked against the current redirect allowlist, and persisted only after the existing owner-approval gate succeeds.
|
| Filename | Overview |
|---|---|
| src/oauth-client-registration.ts | Implements bounded HMAC signing, constant-time signature verification, schema validation, and reconstruction for public-client registrations. |
| src/oauth-store.ts | Issues recoverable client IDs, reconstructs missing clients after signature and redirect-host checks, and adds idempotent persistence. |
| src/oauth-provider.ts | Supplies the registration key to the client store and persists a recovered registration only after owner approval succeeds. |
| src/config.ts | Resolves and validates the dedicated registration key, falling back to a deterministic key derived from the owner token. |
| src/cli.ts | Extends initialization to preserve, derive, or generate the client-registration key as appropriate. |
| src/user-config.ts | Adds secure random key generation and memory-hard compatibility derivation. |
| src/oauth-store.test.ts | Covers transient recovery, failed and successful approval, persistence timing, and redirect-policy rejection. |
| src/oauth-client-registration.test.ts | Covers signed-ID round trips, payload and signature tampering, wrong keys, length limits, and rejection of confidential clients. |
Sequence Diagram
sequenceDiagram
participant Client as OAuth Client
participant Router as OAuth Router
participant Store as Client Store
participant Provider as OAuth Provider
participant DB as SQLite
Client->>Router: Authorization request with cached client_id
Router->>Store: getClient(client_id)
Store->>DB: Look up registration
DB-->>Store: Missing
Store->>Store: Verify HMAC and decode metadata
Store->>Store: Check redirect hosts against allowlist
Store-->>Router: Transient recovered registration
Router->>Provider: authorize(client, parameters)
Provider-->>Client: Owner approval form
Client->>Provider: Submit owner password
Provider->>Provider: Verify owner password
Provider->>DB: Restore client registration
Provider-->>Client: Redirect with authorization code
Reviews (1): Last reviewed commit: "fix(oauth): harden registration key deri..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/oauth-store.test.ts (1)
226-234: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the rejection status on the wrong-owner-token path.
authorizationResponsediscards the status code, so this call proves only that no client row was written. Any early return or unrelated failure insideauthorizeproduces the same result. Capture the status in the test double and assert401, so the negative case pins the intended behavior.💚 Suggested change
+ const rejected = authorizationResponse("wrong-owner-token"); await recoveredProvider.authorize( recovered, params, - authorizationResponse("wrong-owner-token"), + rejected, ); + assert.equal(statusOf(rejected), 401);Record the value inside the helper:
function authorizationResponse( ownerToken: string, onRedirect?: (location: string) => void, ): Response { const response = { req: { method: "POST", body: { owner_token: ownerToken } }, - status() { + statusCode: 0, + status(code: number) { + response.statusCode = code; return response; },and expose it with a small
statusOfaccessor, or assert on(rejected as unknown as { statusCode: number }).statusCode.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/oauth-store.test.ts` around lines 226 - 234, Update the wrong-owner-token test around recoveredProvider.authorize to retain the authorization response status and assert it is 401, using a statusOf accessor or direct statusCode inspection on the captured response. Keep the existing assertion that no client row is written.src/oauth-client-registration.test.ts (1)
28-39: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd the legacy-ID and extra-segment negative cases.
The documented legacy path depends on
recoverClientRegistrationrejecting pre-change random client IDs. No assertion covers that. Theextra !== undefinedbranch is also uncovered.💚 Suggested additional assertions
assert.equal(recoverClientRegistration(clientId, `${signingKey}-wrong`), undefined); assert.equal(recoverClientRegistration(`devspace-v1.${"x".repeat(5000)}.signature`, signingKey), undefined); + +// Legacy random client IDs are not recoverable. +assert.equal( + recoverClientRegistration("devspace-0b3f9c1e-2d4a-4f77-9c0e-1a2b3c4d5e6f", signingKey), + undefined, +); +// A trailing segment invalidates the client ID even when the signature matches. +assert.equal(recoverClientRegistration(`${clientId}.extra`, signingKey), undefined); +// A different prefix is rejected. +assert.equal( + recoverClientRegistration(`devspace-v2.${parts[1]}.${parts[2]}`, signingKey), + undefined, +);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/oauth-client-registration.test.ts` around lines 28 - 39, Add negative assertions in the registration recovery tests around recoverClientRegistration: verify a documented pre-change random/legacy client ID is rejected, and verify an ID containing an additional segment beyond the expected three segments returns undefined. Keep the existing malformed-part, wrong-signing-key, and oversized-payload cases unchanged.src/oauth-client-registration.ts (1)
27-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the oversize rejection observable.
If the encoded client ID exceeds
MAX_CLIENT_ID_LENGTH,createRecoverableClientIdreturnsundefinedandSqliteOAuthStore.registerClientfalls back to a random UUID atsrc/oauth-store.tslines 79-81. Recovery is then permanently unavailable for that client, and nothing records why.Return a discriminated result or log at debug level so the reason is inspectable. A registration with many or long
redirect_urisreaches this path.As per coding guidelines: "Represent important behavior through schemas, types, checks, or explicit tool results rather than hidden prompt conventions."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/oauth-client-registration.ts` at line 27, The oversize branch in createRecoverableClientId currently returns undefined without exposing why, while registerClient silently falls back to a UUID. Make the rejection observable by returning a discriminated result or emitting a debug-level log that identifies the MAX_CLIENT_ID_LENGTH overflow, and update SqliteOAuthStore.registerClient to handle the chosen result without losing the existing fallback behavior.Source: Coding guidelines
src/user-config.ts (1)
107-113: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMake the client-registration derivation cost explicit and document password rotation.
Adding
{ N: 16_384, r: 8, p: 1 }keeps the current scrypt work factor but makes the cost inspectable and versioned withdevspace-oauth-client-registration-v1. Add a short note at the derivation site that the derived registration key is bound to theownerToken; if the owner password changes without a storedclientRegistrationKey, previously issued recoverable client IDs stop validating and affected clients must re-register.♻️ Suggested change
export function deriveClientRegistrationKey(ownerToken: string): string { + // Owner-token-bound: rotating the owner password without a stored + // clientRegistrationKey invalidates all previously issued recoverable client IDs. return scryptSync( ownerToken, "devspace-oauth-client-registration-v1", 32, + { N: 16_384, r: 8, p: 1 }, ).toString("base64url"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/user-config.ts` around lines 107 - 113, Update deriveClientRegistrationKey to pass the explicit scrypt cost parameters N: 16_384, r: 8, and p: 1 while preserving the existing salt and output encoding. Add a brief comment at the derivation site documenting that the key is bound to ownerToken and that password changes without a stored clientRegistrationKey invalidate previously issued recoverable client IDs, requiring client re-registration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/security.md`:
- Around line 46-52: Revise the recovery claim in the security documentation to
state that repository verification is limited to the in-process
testClientRegistrationRecovery flow using SingleUserOAuthProvider and a mock
Express Response, without claiming validation of the SDK authorization router,
real redirect URI and PKCE checks, or a real ChatGPT/MCP-host reconnect. If a
real packaged npm/npx host reconnect was verified, document its scope and
restart, checkout, platform, tool, widget, and rendered-artifact coverage
instead.
In `@src/oauth-client-registration.ts`:
- Around line 22-27: Update recoverableClientId and
SqliteOAuthClientsStore.registerClient so the exact pre-parse registration is
used when creating the signed payload and persisted for recovery, rather than
OAuthClientInformationFullSchema.parse output. Preserve host-supplied RFC 7591
fields and extra properties, and ensure registration data remains restorable
even when SDK validation would normalize, reject, or drop fields.
In `@src/oauth-store.ts`:
- Around line 89-91: Update SqliteOAuthStore.restoreClient to validate the
restored client's redirect URIs against the current allowedRedirectHosts policy
before calling saveClient. Reject the client when any redirect URI has a
disallowed host, preserving the existing save behavior only for clients that
pass validation.
---
Nitpick comments:
In `@src/oauth-client-registration.test.ts`:
- Around line 28-39: Add negative assertions in the registration recovery tests
around recoverClientRegistration: verify a documented pre-change random/legacy
client ID is rejected, and verify an ID containing an additional segment beyond
the expected three segments returns undefined. Keep the existing malformed-part,
wrong-signing-key, and oversized-payload cases unchanged.
In `@src/oauth-client-registration.ts`:
- Line 27: The oversize branch in createRecoverableClientId currently returns
undefined without exposing why, while registerClient silently falls back to a
UUID. Make the rejection observable by returning a discriminated result or
emitting a debug-level log that identifies the MAX_CLIENT_ID_LENGTH overflow,
and update SqliteOAuthStore.registerClient to handle the chosen result without
losing the existing fallback behavior.
In `@src/oauth-store.test.ts`:
- Around line 226-234: Update the wrong-owner-token test around
recoveredProvider.authorize to retain the authorization response status and
assert it is 401, using a statusOf accessor or direct statusCode inspection on
the captured response. Keep the existing assertion that no client row is
written.
In `@src/user-config.ts`:
- Around line 107-113: Update deriveClientRegistrationKey to pass the explicit
scrypt cost parameters N: 16_384, r: 8, and p: 1 while preserving the existing
salt and output encoding. Add a brief comment at the derivation site documenting
that the key is bound to ownerToken and that password changes without a stored
clientRegistrationKey invalidate previously issued recoverable client IDs,
requiring client re-registration.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a8d21b0a-7d1c-46e5-8afa-6524204dd1f0
📒 Files selected for processing (12)
docs/configuration.mddocs/security.mdpackage.jsonsrc/cli.tssrc/config.test.tssrc/config.tssrc/oauth-client-registration.test.tssrc/oauth-client-registration.tssrc/oauth-provider.tssrc/oauth-store.test.tssrc/oauth-store.tssrc/user-config.ts
[GPT-5.6-THINKING] RESPONDING ON BEHALF OF WAISHNAVThe additional review findings are covered in |
1 participant
ChatGPT reconnect can restart OAuth with its cached client ID. If DevSpace loses the corresponding SQLite client row during local development, authorization stops at
invalid_client, forcing the connector to be deleted and configured again.New public client registrations now use an authenticated, self-contained client ID backed by a private signing key. A missing registration is reconstructed transiently, checked against the current redirect allowlist, and persisted only after successful Owner password approval. Access and refresh tokens remain opaque server-side state. Registrations created before this change need one normal re-registration before this recovery path applies.
Summary by CodeRabbit
New Features
Security
Documentation