Conversation
Contributor
📝 WalkthroughWalkthroughAdds Google "Instant dev credentials" support: frontend UI toggle and conditional client creation, server config accessors and schema, validation in dash and runtime routes, model resolution loading dev creds for Google web clients, and corresponding unit/integration tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client as Web Client
participant API as Instant API
participant Config as Config/Credentials
participant Google as Google OAuth
User->>Client: Enable "Instant dev credentials"
Client->>API: POST /dash/apps/:id/oauth_clients (meta.useDevCredentials=true, no client_id/secret)
API->>API: Validate and store client (meta flag)
rect rgba(100,150,200,0.5)
Note over User,Google: OAuth Flow using managed dev credentials
User->>Client: Click "Sign in with Google"
Client->>API: GET /runtime/oauth/start (oauth_client_id, redirect_uri)
API->>Config: get-google-oauth-client-dev()
Config-->>API: Return dev client_id + client_secret
API->>API: Validate redirect_uri allowed for managed-dev
API->>Google: Redirect user to Google auth (using dev client_id)
Google-->>User: Consent screen
User->>Google: Approve -> Redirect to callback (code + state)
Client->>API: GET /runtime/oauth/callback (code, state)
API->>Google: POST token endpoint (code, dev client credentials)
Google-->>API: Return id_token + refresh_token
API-->>Client: Token response / user session
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Contributor
|
View Vercel preview at instant-www-js-test2-jsv.vercel.app. |
![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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/src/instant/config.clj`:
- Around line 237-247: The current get-google-oauth-client-dev can mix one env
value with the other fallback, producing mismatched credentials; update it to
resolve the pair atomically: read both env vars (GOOGLE_OAUTH_CLIENT_ID_DEV and
GOOGLE_OAUTH_CLIENT_SECRET_DEV) and normalize blank strings to nil (e.g. via
not-empty), then if both env values are present use them together; else if
neither env is present fall back to the EDN pair from `@config-map`
(:google-oauth-client-dev :client-id and :client-secret via
crypt-util/secret-value); otherwise return nil (no partial pair). Keep
crypt-util/obfuscate for the returned :client-secret and locate changes in
get-google-oauth-client-dev.
In `@server/src/instant/dash/routes.clj`:
- Around line 719-723: The route reads current-client but never asserts it
exists before calling app-oauth-client-model/update!, which lets a bad :id
create a new partial client; modify the handler that fetches current-client (the
binding named current-client where app-oauth-client-model/get-by-id is called)
to check for nil and return an appropriate error response (e.g. 404 or
validation error) immediately when current-client is missing instead of
proceeding to call app-oauth-client-model/update!; apply the same nil-existence
check and early return for the other occurrence around the code block at the
757-762 region to prevent update! from synthesizing a new record.
- Around line 687-703: The validation currently skips checking
discovery_endpoint for non-GitHub providers, which allows callers to register a
Google client with an arbitrary OIDC discovery URL when useDevCredentials is
enabled; update the validation around the ex/assert-valid! / discovery-endpoint
logic so that when (:provider_name provider) is "google" and the request
indicates useDevCredentials, discovery_endpoint is validated (or forced) to
equal Google’s canonical discovery document URL (the same literal used in
app_oauth_client.clj to detect managed-dev) — i.e., read useDevCredentials from
req/body, and if true and provider is "google" ensure discovery-endpoint is the
exact Google discovery URL (or fail validation via oauth-client-errors) so
managed-dev clients are pinned to the expected document.
In `@server/src/instant/runtime/routes.clj`:
- Around line 187-197: The localhost check in localhost-origin? only matches
"localhost" and "127.0.0.1", so IPv6 loopback addresses like "::1" are
incorrectly rejected by managed-dev-origin? (used in managed-dev auth flows);
update the host set in localhost-origin? to include "::1" and apply the same
change to the other identical occurrence of this logic (the second
implementation referenced in the diff) so IPv6 loopback redirects are accepted
for "generic" managed-dev service.
In `@server/test/instant/runtime/routes_test.clj`:
- Line 33: Remove the unused require for lambdaisland.uri by deleting the vector
entry `[lambdaisland.uri :as uri]` from the ns require list in the
routes_test.clj namespace (the unused alias `uri` is the symbol to target);
ensure the remaining requires remain properly comma/spacing separated and run
tests to confirm no references to `uri` remain.
🪄 Autofix (Beta)
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
Run ID: 8ac8ae3c-e18a-434c-b8a7-2897e22bb9ea
📒 Files selected for processing (8)
client/www/components/dash/auth/Google.tsxserver/src/instant/config.cljserver/src/instant/config_edn.cljserver/src/instant/dash/routes.cljserver/src/instant/model/app_oauth_client.cljserver/src/instant/runtime/routes.cljserver/test/instant/dash/routes_test.cljserver/test/instant/runtime/routes_test.clj
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/src/instant/dash/routes.clj`:
- Around line 754-758: Current condition blocks all meta updates when the
request includes :meta because it checks (or
(app-oauth-client-model/uses-dev-credentials? current-client) ...), preventing
any non-useDevCredentials changes on managed-dev clients; change the when to
only error when the client already uses dev credentials AND the request
explicitly tries to change or clear that flag: replace the existing (when (and
(body-contains? req :meta) (or (app-oauth-client-model/uses-dev-credentials?
current-client) (contains? (or meta {}) "useDevCredentials") (contains? (or meta
{}) :useDevCredentials) (nil? meta))) ...) with (when (and (body-contains? req
:meta) (app-oauth-client-model/uses-dev-credentials? current-client) (or
(contains? (or meta {}) "useDevCredentials") (contains? (or meta {})
:useDevCredentials) (nil? meta))) ...), so updates that leave useDevCredentials
unchanged (e.g., merged-meta preserving the flag) are allowed.
🪄 Autofix (Beta)
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
Run ID: e88ff626-5dd5-4c6a-98a9-2654fca96981
📒 Files selected for processing (5)
server/src/instant/config.cljserver/src/instant/dash/routes.cljserver/src/instant/runtime/routes.cljserver/test/instant/dash/routes_test.cljserver/test/instant/runtime/routes_test.clj
✅ Files skipped from review due to trivial changes (1)
- server/src/instant/runtime/routes.clj
🚧 Files skipped from review as they are similar to previous changes (1)
- server/test/instant/runtime/routes_test.clj
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/src/instant/dash/routes.clj`:
- Around line 745-747: The merged-meta computation uses (merge (:meta
current-client) meta) which preserves old values when the request explicitly
sends :meta nil; change the cond in the merged-meta binding to treat any
presence of :meta in the request as the new meta value itself: (cond
(body-contains? req :meta) meta :else (:meta current-client)), so that
downstream validation (e.g. oauth-client-errors) operates on the actual
post-update meta state (apply the same change for the other analogous branches
handling 757-769 and 770-774).
- Around line 677-678: The code calls app-oauth-service-provider-model/get-by-id
to set provider and then uses (:provider_name provider) later; add an explicit
nil-check right after obtaining provider and reject unknown provider_id with a
clear error response before any field validation or reading provider keys. In
practice, inside the same route handler where provider is set, if provider is
nil return the appropriate 4xx/validation error (e.g., "unknown provider_id")
and stop further processing; update the same logic paths that later read
:provider_name (and the non-GitHub credential/discovery handling) so they only
run when provider is non-nil.
🪄 Autofix (Beta)
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
Run ID: 7b2be008-8d65-4bf7-b173-6f44b6a52190
📒 Files selected for processing (2)
server/src/instant/dash/routes.cljserver/test/instant/dash/routes_test.clj
✅ Files skipped from review due to trivial changes (1)
- server/test/instant/dash/routes_test.clj
Comment on lines
+677
to
+678
| provider (app-oauth-service-provider-model/get-by-id {:app-id app-id | ||
| :id provider-id}) |
Contributor
There was a problem hiding this comment.
Reject unknown provider_ids before field validation.
If app-oauth-service-provider-model/get-by-id returns nil, this path falls into the generic non-GitHub branch on Line 684 and can report unrelated discovery_endpoint / credential errors instead of failing on the missing provider. Please assert the provider record before reading :provider_name.
Also applies to: 684-685
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/instant/dash/routes.clj` around lines 677 - 678, The code calls
app-oauth-service-provider-model/get-by-id to set provider and then uses
(:provider_name provider) later; add an explicit nil-check right after obtaining
provider and reject unknown provider_id with a clear error response before any
field validation or reading provider keys. In practice, inside the same route
handler where provider is set, if provider is nil return the appropriate
4xx/validation error (e.g., "unknown provider_id") and stop further processing;
update the same logic paths that later read :provider_name (and the non-GitHub
credential/discovery handling) so they only run when provider is non-nil.
Comment on lines
+745
to
+747
| merged-meta (cond | ||
| (body-contains? req :meta) (merge (:meta current-client) meta) | ||
| :else (:meta current-client)) |
Contributor
There was a problem hiding this comment.
Validate the same meta state that gets written.
When the request sends :meta nil, Line 757 already treats that as a real mutation, but Line 746 still leaves merged-meta at the old value while Line 773 writes nil. That lets oauth-client-errors validate stale appType / managed-dev flags instead of the post-update state.
🛠️ Suggested fix
- merged-meta (cond
- (body-contains? req :meta) (merge (:meta current-client) meta)
- :else (:meta current-client))
+ merged-meta (cond
+ (not (body-contains? req :meta)) (:meta current-client)
+ (nil? meta) nil
+ :else (merge (:meta current-client) meta))Also applies to: 757-769, 770-774
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/src/instant/dash/routes.clj` around lines 745 - 747, The merged-meta
computation uses (merge (:meta current-client) meta) which preserves old values
when the request explicitly sends :meta nil; change the cond in the merged-meta
binding to treat any presence of :meta in the request as the new meta value
itself: (cond (body-contains? req :meta) meta :else (:meta current-client)), so
that downstream validation (e.g. oauth-client-errors) operates on the actual
post-update meta state (apply the same change for the other analogous branches
handling 757-769 and 770-774).
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
1 participant
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
This implements a Clerk-style managed development credential mode for Google OAuth redirect clients.
The core UX is inside the existing Google client form rather than as a separate admin-page action:
Use Instant dev credentialsProduct / DX shape
The implementation is intentionally narrow.
What is supported:
webredirect clientsvercel,netlify)What is not supported:
ios,android)That scope is deliberate. Google redirect OAuth is a server-mediated flow where shared dev credentials are practical. Google Button / One Tap relies on Google client-side setup and origin registration, so trying to hide that behind shared backend credentials would be misleading and brittle.
Backend changes
Config
Added an optional managed-dev Google credential source in server config:
GOOGLE_OAUTH_CLIENT_ID_DEVGOOGLE_OAUTH_CLIENT_SECRET_DEV:google-oauth-client-devOAuth client resolution
When an OAuth client is:
webmeta.useDevCredentials = truewe now ignore stored app-specific credentials and instantiate the runtime OAuth client from the server-managed dev credentials plus Google's discovery document.
If the server is missing those managed credentials, we fail with a validation error rather than silently producing a broken flow.
Dashboard validation
Added explicit validation so that managed-dev mode:
webredirect clientsI also tightened the update path so JSON body handling is robust for string-vs-keyword body keys and so managed-dev mode cannot be toggled in place. The delete-and-recreate requirement is now enforced server-side, not just implied by the UI.
Runtime validation
At OAuth start time, managed-dev Google clients now reject production-style origins with a clear error. The runtime allows only:
vercel/netlifyorigin typesThis matters because even if someone manually creates a managed-dev client through the API, they still should not be able to use Instant's shared Google credentials against a production domain.
Frontend changes
The Google dashboard UI now:
webclients into managed-dev modeThis keeps the happy path simple for local development while still letting someone opt into their own production-ready Google credentials from the same form.
Tests
Added targeted end-to-end-ish server coverage in two places.
Runtime flow test
instant.runtime.routes-test/google-managed-dev-oauth-testCovers:
/runtime/oauth/start->/runtime/oauth/callback->/runtime/oauth/tokenflowThis test stubs Google network edges but exercises the real route flow and state/cookie plumbing.
Dashboard validation test
instant.dash.routes-test/google-managed-dev-oauth-client-validationCovers:
Manual verification
I did not complete a live browser-based Google sign-in against Google's real consent screen in this environment. That would require an interactive browser login. I did verify the full server flow under test coverage and validated the dashboard behavior and server-side guards around the new mode.
Why this shape
This is the smallest implementation that improves DX without creating an ambiguous production story.
A separate admin button would have made the model harder to reason about. Folding the behavior into the existing Google client form means the object the user creates is still just a Google OAuth client, with one explicit mode switch.
The runtime guard is also important. Without it, managed credentials would be too easy to accidentally carry into production setups.
Follow-ups I would consider separately