Allow edits to OAuth providers on the dashboard - #2856
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughOAuth provider settings now support editing credentials and provider-specific configuration in the dashboard. Apple, Clerk, and Firebase use dedicated editors. GitHub and LinkedIn use a shared editor. OAuth requests now use SSRF-protected HTTP helpers with bounded response bodies. ChangesOAuth client editing
Guarded OAuth discovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ProviderEditor
participant updateClient
User->>ProviderEditor: Edit provider configuration
ProviderEditor->>updateClient: Submit updated client
updateClient-->>ProviderEditor: Return updated client
sequenceDiagram
participant OAuthFlow
participant webhook_sender
participant GuardedHTTPClient
OAuthFlow->>webhook_sender: Request OAuth or JWKS endpoint
webhook_sender->>GuardedHTTPClient: Validate URL and execute request
GuardedHTTPClient-->>webhook_sender: Return status, headers, and body
webhook_sender-->>OAuthFlow: Return normalized response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
client/www/components/dash/auth/Firebase.tsx (1)
122-135: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShow the editor entry point when no project ID is parsed.
If
client.discovery_endpointdoes not match the regex,currentProjectIdis''and theCopyableis hidden. The "Update project ID" button still renders, so the user can recover. Consider adding a short message that the project ID could not be read, so the empty state is not silent.🤖 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 `@client/www/components/dash/auth/Firebase.tsx` around lines 122 - 135, Update the non-editing branch of the Firebase component to show a concise message when currentProjectId is empty, indicating that the Firebase project ID could not be read; keep the existing Copyable conditional and “Update project ID” button unchanged so users can recover.client/www/components/dash/auth/shared.tsx (1)
454-516: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConfirm that a blank secret is accepted by the API for a client that has no secret yet.
handleSaveomitsclient_secretwhen the field is blank. For a client created without a secret, the user can save a client ID and never be told that the secret is still missing. Consider requiring the secret whenclient.client_idwas previously empty, or state the current secret status in the read view.🤖 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 `@client/www/components/dash/auth/shared.tsx` around lines 454 - 516, Update OAuthCredentialsEditor.handleSave to explicitly handle a blank clientSecret when the client previously had no client.client_id, either requiring the secret before saving or clearly surfacing the missing-secret status in the read view; preserve optional secret updates for clients that already have credentials.
🤖 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 `@client/www/components/dash/auth/Apple.tsx`:
- Around line 60-100: Normalize cleared teamId and keyId values as absent in the
Apple credential save flow around validationError and handleSave. Remove these
metadata keys or omit them when their values are empty before updateClient
persists the credentials, so jwt/apple-client-secret receives no empty Apple
metadata.
In `@client/www/components/dash/auth/Firebase.tsx`:
- Around line 93-107: Update handleSave to validate projectId against the
Firebase project ID format of lowercase letters, digits, and hyphens before
constructing discovery_endpoint. Keep the existing missing-ID toast, but show an
appropriate validation error and return without calling updateClient when the
format is invalid.
In `@client/www/components/dash/auth/shared.tsx`:
- Around line 559-566: Credential-form Cancel buttons currently default to
submit and can trigger form submission. Add type="button" to each listed Cancel
Button in client/www/components/dash/auth/shared.tsx (559-566),
client/www/components/dash/auth/Apple.tsx (198-205),
client/www/components/dash/auth/Clerk.tsx (239-246), and
client/www/components/dash/auth/Firebase.tsx (166-173), including the add-client
forms; do not change the shared Button default.
---
Nitpick comments:
In `@client/www/components/dash/auth/Firebase.tsx`:
- Around line 122-135: Update the non-editing branch of the Firebase component
to show a concise message when currentProjectId is empty, indicating that the
Firebase project ID could not be read; keep the existing Copyable conditional
and “Update project ID” button unchanged so users can recover.
In `@client/www/components/dash/auth/shared.tsx`:
- Around line 454-516: Update OAuthCredentialsEditor.handleSave to explicitly
handle a blank clientSecret when the client previously had no client.client_id,
either requiring the secret before saving or clearly surfacing the
missing-secret status in the read view; preserve optional secret updates for
clients that already have credentials.
🪄 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: c709b4eb-1b7e-4c6c-b6a8-7bd7f65e6dde
📒 Files selected for processing (7)
client/www/components/dash/AppAuth.tsxclient/www/components/dash/auth/Apple.tsxclient/www/components/dash/auth/Clerk.tsxclient/www/components/dash/auth/Firebase.tsxclient/www/components/dash/auth/GitHub.tsxclient/www/components/dash/auth/LinkedIn.tsxclient/www/components/dash/auth/shared.tsx
Contributor
|
View Vercel preview at instant-www-js-update-apple-secret-jsv.vercel.app. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@server/src/instant/auth/oauth.clj`:
- Around line 303-305: Validate both token_endpoint and userinfo_endpoint from
the parsed discovery document before accepting them, using the same URL-safety
guard as endpoint. Update GenericOAuthClient.get-user-info and token exchange
requests to use the guarded client for these discovered URLs, and add a test
proving a discovery document with a safe endpoint but unsafe token_endpoint is
rejected.
🪄 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: bf2aa791-64b7-4eb0-b915-7acc5838c821
📒 Files selected for processing (4)
client/www/components/dash/auth/Apple.tsxserver/src/instant/auth/oauth.cljserver/src/instant/webhook_sender.cljserver/test/instant/auth/oauth_test.clj
🚧 Files skipped from review as they are similar to previous changes (1)
- client/www/components/dash/auth/Apple.tsx
dwwoelfel
changed the title
dwwoelfel
marked this pull request as ready for review
August 11, 2026 20:35
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@server/src/instant/auth/oauth.clj`:
- Around line 151-165: Update the response handling around safe-post-form so
:success? is evaluated before json/<-json parses (:body resp), or catch parsing
failures and return the controlled "Error exchanging code for token." result.
Preserve the existing successful-response parsing and error_description behavior
for valid OAuth error JSON.
In `@server/src/instant/webhook_sender.clj`:
- Around line 203-215: Update execute-response to read the response body through
a capped stream with the configured byte limit, rejecting responses that exceed
it even without a Content-Length header. Replace the unbounded (.string)
conversion while preserving the existing success, status, headers, and body
result shape for both safe-get and safe-post-form.
🪄 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: f1a48db0-163b-4239-bca0-90c5bbbfe4af
📒 Files selected for processing (4)
server/src/instant/auth/jwt.cljserver/src/instant/auth/oauth.cljserver/src/instant/webhook_sender.cljserver/test/instant/auth/oauth_test.clj
🚧 Files skipped from review as they are similar to previous changes (1)
- server/test/instant/auth/oauth_test.clj
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/test/instant/webhook_sender_test.clj (1)
216-240: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd coverage for
safe-post-form.This test covers normal and oversized responses through
safe-getonly.safe-post-formis another guarded entry point that uses the shared response path. Add an oversized chunked-response case throughsafe-post-formto protect the OAuth token-request path from future regressions.Suggested test extension
+ (.enqueue server (.. (MockResponse$Builder.) + (chunkedBody ^String (apply str (repeat 100 \a)) 8) + (build))) + (let [url (str "http://127.0.0.1.nip.io:" (.getPort server) "/post")] + (is (thrown-with-msg? clojure.lang.ExceptionInfo + #"exceeds size limit" + (webhook-sender/safe-post-form + url {"code" "test"})) + "a form POST body over the cap is rejected"))🤖 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 `@server/test/instant/webhook_sender_test.clj` around lines 216 - 240, Add coverage to the safe-get-caps-response-body test for webhook-sender/safe-post-form using a chunked response larger than webhook-sender/max-response-bytes, and assert it throws ExceptionInfo with an “exceeds size limit” message. Reuse the existing MockWebServer setup and cleanup while exercising the OAuth token-request entry point.
🤖 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.
Nitpick comments:
In `@server/test/instant/webhook_sender_test.clj`:
- Around line 216-240: Add coverage to the safe-get-caps-response-body test for
webhook-sender/safe-post-form using a chunked response larger than
webhook-sender/max-response-bytes, and assert it throws ExceptionInfo with an
“exceeds size limit” message. Reuse the existing MockWebServer setup and cleanup
while exercising the OAuth token-request entry point.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: afd7063b-7d98-4d02-ab6e-3cfb4e77afd9
📒 Files selected for processing (3)
server/src/instant/auth/oauth.cljserver/src/instant/webhook_sender.cljserver/test/instant/webhook_sender_test.clj
🚧 Files skipped from review as they are similar to previous changes (1)
- server/src/instant/auth/oauth.clj
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
2 participants
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.
We had the ability to edit the google oauth client, now you can edit credentials for the rest of the clients.