Plan oauth-provider migration (#1)#380
Conversation
* Plan oauth-provider migration Co-authored-by: Steve Martocci <smart@users.noreply.github.com> * Migrate Convex auth to oauth provider Co-authored-by: Steve Martocci <smart@users.noreply.github.com> * Fix oauth provider validation issues Co-authored-by: Steve Martocci <smart@users.noreply.github.com> * Fix local backend e2e harness Co-authored-by: Steve Martocci <smart@users.noreply.github.com> * Address oauth provider PR review comments Co-authored-by: Steve Martocci <smart@users.noreply.github.com> * Add path OpenID discovery route Co-authored-by: Steve Martocci <smart@users.noreply.github.com> * Add generic protected resource metadata route Co-authored-by: Steve Martocci <smart@users.noreply.github.com> * Force Convex issuer in OAuth metadata Co-authored-by: Steve Martocci <smart@users.noreply.github.com> * Fix auth base path metadata handling Co-authored-by: Steve Martocci <smart@users.noreply.github.com> * Use Convex issuer in protected resource metadata Co-authored-by: Steve Martocci <smart@users.noreply.github.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Steve Martocci <smart@users.noreply.github.com>
|
@smart is attempting to deploy a commit to the Convex Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: get-convex/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR migrates the Convex Better Auth integration from the deprecated Sequence Diagram(s)sequenceDiagram
participant Dev
participant ConvexPlugin
participant OAuthProvider
participant JWTPlugin
participant ConvexHTTP
Dev->>ConvexPlugin: configure(opts.oauthProvider)
ConvexPlugin->>OAuthProvider: init(opts)
OAuthProvider->>ConvexPlugin: hooks, endpoints, schema contributions
ConvexPlugin->>JWTPlugin: read issuer/jwks via getPlugin("jwt")
ConvexPlugin->>ConvexHTTP: proxy well-known endpoints (rewritten base URL)
ConvexHTTP->>Client: serve discovery & protected-resource metadata (WWW-Authenticate exposed)
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/client/create-client.ts (1)
149-200: 💤 Low valueConsider validating
CONVEX_SITE_URLconsistently.
protectedResourceMetadataproperly throws whenCONVEX_SITE_URLis unset, but the redirect URL construction at lines 154, 162, 168, 176 does not validate. If undefined, these produce malformed URLs like"undefined/convex/...".In Convex production this is always set, but for consistency with the validation in
protectedResourceMetadata, consider extracting the validation:💡 Suggested approach
+const getConvexSiteUrl = () => { + const siteUrl = process.env.CONVEX_SITE_URL; + if (!siteUrl) { + throw new Error("CONVEX_SITE_URL is not set"); + } + return siteUrl; +}; + const protectedResourceMetadata = (resourcePath: string) => { - const siteUrl = process.env.CONVEX_SITE_URL; - if (!siteUrl) { - throw new Error("CONVEX_SITE_URL is not set"); - } + const siteUrl = getConvexSiteUrl(); // ... }; const registerWellKnownRoutes = (http: HttpRouter, path: string) => { + const siteUrl = getConvexSiteUrl(); // Use siteUrl instead of process.env.CONVEX_SITE_URL in redirect handlers };🤖 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/client/create-client.ts` around lines 149 - 200, The redirect URLs in registerWellKnownRoutes build strings using process.env.CONVEX_SITE_URL without validation (see registerWellKnownRoutes, routeIfMissing, Response.redirect), which can produce malformed "undefined/..." URLs; update registerWellKnownRoutes to use the same validated helper that protectedResourceMetadata uses (or add a small getValidatedConvexSiteUrl helper) to read and validate CONVEX_SITE_URL, then build redirect URLs from that validated base before calling Response.redirect or jsonResponse so all well-known routes consistently error or redirect only when a valid CONVEX_SITE_URL is present.
🤖 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/content/docs/framework-guides/sveltekit.mdx`:
- Around line 218-231: Update the install instruction so the documented package
versions match the oauthProvider configuration shown in the Convex plugin block
(the convex({ authConfig, oauthProvider: { ... loginPage, consentPage,
allowDynamicClientRegistration, pairwiseSecret }}) code). Replace the current
install command that pins better-auth@1.5.3 with a command that installs
better-auth@1.6.11 and `@better-auth/oauth-provider`@1.6.11 (save-exact) so the
oauth-provider features and pairwiseSecret support used in the oauthProvider
config are present.
In `@docs/content/docs/migrations/migrate-to-oauth-provider.mdx`:
- Around line 17-19: Update the npm install command shown as "npm install
better-auth@1.6.11 `@better-auth/oauth-provider`@1.6.11" to include the
--save-exact flag so the exact versions are persisted (i.e., change that install
line to include --save-exact).
In `@src/client/create-client.test.ts`:
- Around line 138-198: The test "registerRoutes exposes OAuth protected resource
metadata" mutates process.env.CONVEX_SITE_URL and doesn't restore it; capture
the original value of process.env.CONVEX_SITE_URL before setting it in that test
and restore it after the test completes (use a try/finally inside the it block
or an afterEach hook) so other tests aren't affected; ensure this cleanup
surrounds the code that calls createClient(component) and
client.registerRoutes(...) so the environment is reliably reset.
In `@src/plugins/convex/index.test.ts`:
- Around line 108-191: The tests modify process.env.CONVEX_SITE_URL without
restoring it; update the tests around convex(...) / getOAuthServerConfig to save
const _orig = process.env.CONVEX_SITE_URL before setting
process.env.CONVEX_SITE_URL, and restore it after the test (e.g., in a finally
block or an afterEach that sets process.env.CONVEX_SITE_URL = _orig or deletes
it if _orig === undefined) so the global env state is not leaked between tests;
apply this change to the two tests that call convex(...) and reference
getOAuthServerConfig.
In `@src/plugins/mcp/index.ts`:
- Around line 17-31: The exposeAuthenticateHeader function currently returns
early if Access-Control-Expose-Headers is present, which prevents adding
WWW-Authenticate; change it to read the existing Access-Control-Expose-Headers
value, append "WWW-Authenticate" if it's not already listed (preserving existing
values and avoiding duplicates), and set the merged string back onto the headers
before returning the new Response (keep using response.body, response.status,
response.statusText, and a new Headers instance); reference the
exposeAuthenticateHeader function and the header names "WWW-Authenticate" and
"Access-Control-Expose-Headers".
---
Nitpick comments:
In `@src/client/create-client.ts`:
- Around line 149-200: The redirect URLs in registerWellKnownRoutes build
strings using process.env.CONVEX_SITE_URL without validation (see
registerWellKnownRoutes, routeIfMissing, Response.redirect), which can produce
malformed "undefined/..." URLs; update registerWellKnownRoutes to use the same
validated helper that protectedResourceMetadata uses (or add a small
getValidatedConvexSiteUrl helper) to read and validate CONVEX_SITE_URL, then
build redirect URLs from that validated base before calling Response.redirect or
jsonResponse so all well-known routes consistently error or redirect only when a
valid CONVEX_SITE_URL is present.
🪄 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: get-convex/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2ababae6-7bd5-40d6-aa51-9c69ea5cb6c8
⛔ Files ignored due to path filters (5)
examples/next/package-lock.jsonis excluded by!**/package-lock.jsonexamples/react/package-lock.jsonis excluded by!**/package-lock.jsonexamples/tanstack/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.jsonsrc/component/_generated/component.tsis excluded by!**/_generated/**
📒 Files selected for processing (21)
CHANGELOG.mdMIGRATION_PLAN.mdREADME.mddocs/content/docs/framework-guides/sveltekit.mdxdocs/content/docs/migrations/meta.jsondocs/content/docs/migrations/migrate-to-oauth-provider.mdxe2e/backendHarness.jse2e/run-e2e-tests.shpackage.jsonsrc/auth-options.tssrc/client/adapter.tssrc/client/create-client.test.tssrc/client/create-client.tssrc/client/create-schema.test.tssrc/client/create-schema.tssrc/component/schema.tssrc/plugins/convex/index.test.tssrc/plugins/convex/index.tssrc/plugins/index.tssrc/plugins/mcp/index.test.tssrc/plugins/mcp/index.ts
💤 Files with no reviewable changes (1)
- src/auth-options.ts
|
working on these |
Co-authored-by: Steve Martocci <smart@users.noreply.github.com>
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
105-105:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid exact version pinning in
peerDependencies—use a semver range instead.Pinning
better-authto exactly1.6.11forces all consumers to use that precise version. npm's official documentation explicitly discourages exact patch pins in peerDependencies, stating: "make sure your plugin requirement is as broad as possible, and not to lock it down to specific patch versions." This approach prevents consumers from receiving patch fixes and causes peer dependency conflicts when they have any other version.Use a semver range to express compatibility while allowing patch updates:
Suggested fix
- "better-auth": "1.6.11", + "better-auth": "^1.6.11",The
^range allows any version from 1.6.11 up to (but not including) 2.0.0, which is appropriate for semver compatibility. Alternatively, use~1.6.11if you need stricter control (patch updates only: 1.6.12, 1.6.13, etc.).🤖 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 `@package.json` at line 105, The peerDependency entry for "better-auth" is pinned to an exact version ("1.6.11"); change it to a semver range to allow compatible patch/minor updates (for example replace "1.6.11" with "^1.6.11" or "~1.6.11") in the package.json peerDependencies section so consumers can use compatible versions without being forced to that exact patch; update the "better-auth" value accordingly.
🧹 Nitpick comments (1)
package.json (1)
23-23: ⚡ Quick winUse
prepublishOnlyinstead ofpreparefor clearer intent.The
preparescript runs during local development afternpm install, not during consumer installation from npm registry. However,prepareis unconventional for this use case. Sincepreversion(line 33) already builds before publishing, useprepublishOnlyto explicitly indicate the script should only run duringnpm publishpreparation:- "prepare": "npm run build", + "prepublishOnly": "npm run build",Alternatively, if
preversionfully covers your pre-publish needs (which it appears to, since it runsbuild:clean), the script may be redundant and can be removed entirely.🤖 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 `@package.json` at line 23, The package.json currently defines a "prepare" script which runs during local installs; replace or remove it to match publishing intent: either rename the "prepare" script to "prepublishOnly" so "npm publish" triggers the build, or remove the "prepare" entry entirely if the existing "preversion" script already runs the necessary build steps (e.g., "build:clean" and "build"); update the scripts section to use "prepublishOnly" or delete "prepare" accordingly to avoid unintended local-run builds.
🤖 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.
Outside diff comments:
In `@package.json`:
- Line 105: The peerDependency entry for "better-auth" is pinned to an exact
version ("1.6.11"); change it to a semver range to allow compatible patch/minor
updates (for example replace "1.6.11" with "^1.6.11" or "~1.6.11") in the
package.json peerDependencies section so consumers can use compatible versions
without being forced to that exact patch; update the "better-auth" value
accordingly.
---
Nitpick comments:
In `@package.json`:
- Line 23: The package.json currently defines a "prepare" script which runs
during local installs; replace or remove it to match publishing intent: either
rename the "prepare" script to "prepublishOnly" so "npm publish" triggers the
build, or remove the "prepare" entry entirely if the existing "preversion"
script already runs the necessary build steps (e.g., "build:clean" and "build");
update the scripts section to use "prepublishOnly" or delete "prepare"
accordingly to avoid unintended local-run builds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: get-convex/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 487b4b5c-b80d-4fb6-9a41-917c11b75da8
📒 Files selected for processing (1)
package.json
Co-authored-by: smartocci <smartocci@gmail.com>
2 participants
Summary
This PR migrates the Convex Better Auth component from Better Auth’s deprecated
oidc-providerplugin to the new@better-auth/oauth-providerpackage.The migration keeps Convex as the OAuth/OIDC issuer, preserves Convex-compatible JWT/JWKS behavior, and adds the route/discovery support needed for OAuth 2.1, OIDC, and MCP clients.
What changed
better-auth/plugins/oidc-providerwith@better-auth/oauth-providerbetter-authand@better-auth/oauth-providerto1.6.11oauthClientoauthRefreshTokenoauthAccessTokenoauthConsentconvex_jwtcookie/convex/token/convex/jwksCONVEX_SITE_URLas issuer even when appbaseURLdiffers/.well-known/openid-configuration/.well-known/oauth-authorization-server/.well-known/oauth-protected-resourceWWW-Authenticatethrough CORSwithMcpAuthoAuthDiscoveryMetadataoAuthProtectedResourceMetadataoauth_querycontinuation guidance--instance-secretand--instance-nameMigration notes
Existing OAuth/OIDC provider data requires migration.
Old
oauthApplication/ legacyoidcApplicationrecords should be migrated to the newoauthClienttable shape. Existing OAuth access and refresh tokens should be invalidated and clients should reauthorize because oauth-provider stores tokens differently and separates refresh tokens intooauthRefreshToken.Local-install users should regenerate their Better Auth schema after upgrading.
Testing
Passed locally:
npm testnpm run typechecknpm run lintnpm run buildnpm run test:e2enpx convex dev --once --typecheck disableinexamples/reactAlso verified with a throwaway Convex fixture using:
better-auth@1.6.11@better-auth/oauth-provider@1.6.11convex@1.39.1The fixture bundled successfully without Node built-in import failures.
Notes / remaining follow-up
This PR adds route-level and existing e2e coverage, but a full real-cloud OAuth provider acceptance test is still recommended before release:
/oauth2/userinfo/oauth2/end-sessionThat flow requires a real Convex deployment and deployment credentials, so it should be added as an opt-in cloud e2e suite.