refactor(next-auth): modernize URL handling and remove legacy oauth dependency#28
Open
riceharvest wants to merge 62 commits intomainfrom
Open
refactor(next-auth): modernize URL handling and remove legacy oauth dependency#28riceharvest wants to merge 62 commits intomainfrom
riceharvest wants to merge 62 commits intomainfrom
Conversation
- Upgraded multiple packages to modern standards (Next.js, Next-auth, PWA, SEO). - Added new utility packages: critters, next-circuit-breaker, next-csrf, next-images, next-json-ld. - Integrated Changesets for versioning. - Updated CI/CD workflows and linting configurations. - Fixed numerous linting and type-checking issues across the monorepo.
- Remove legacy NextAuth adapters and resolve workspace version conflicts - Clean up test warning noise and fix tsconfig/jest setups for next-auth - Update Workbox/Terser dependencies in next-pwa to align with workspace - Synchronize root lockfile to reflect nested package resolutions
Fixes `JWT_AUTO_GENERATED_SIGNING_KEY` and `JWT_AUTO_GENERATED_ENCRYPTION_KEY` warnings properly by supplying JWKs directly in the test suite rather than mocking the logger.
Review Summary by QodoModernize OAuth, enhance CSRF protection, add MDX modules, and improve test coverage across packages
WalkthroughsDescription• **Modernized OAuth implementation**: Replaced legacy oauth package with native fetch-based implementation for OAuth 2.x, removing external dependencies (oauth and querystring) • **Enhanced CSRF protection**: Added verifyCsrfToken() for App Router support, improved token extraction from headers/body/query, changed httpOnly default to false for client-side access • **Refactored URL handling**: Updated parseUrl to return structured url object with origin, pathname, and href properties • **Provider configuration updates**: Renamed provider.protection to provider.checks supporting 'pkce', 'state', 'none'; added url property to AppProvider • **Session utilities enhancement**: Added parseTime() function supporting duration units (s, m, h, d), improved cookie serialization • **MDX module additions**: New modules for node retrieval (get-nodes.ts), configuration loading (get-config.ts), file discovery (get-files.ts), table of contents generation, and client-side hydration • **Next.js 13+ support**: Added native transpilePackages support for Next.js 13+ as alternative to webpack patching • **Test suite modernization**: Migrated multiple test suites from Jest/vitest to Node.js native testing, updated deprecated assertion methods, added comprehensive test coverage for new features • **Type safety improvements**: Enhanced TypeScript definitions for adapters, providers, and PWA plugin; improved type casting in session methods • **Bug fixes**: Fixed Critters runtime security issues with URL sanitization, improved container detection fallback handling • **Build configuration updates**: Updated tsup configs for TypeScript declaration generation and minification, simplified build setups • **New authentication examples**: Added mock database, API server, and client implementations for react-query-auth Vite example Diagramflowchart LR
A["Legacy oauth package"] -- "Replace with native fetch" --> B["OAuth 2.x client"]
C["provider.protection"] -- "Rename to checks" --> D["CSRF/PKCE validation"]
E["parseUrl function"] -- "Return structured object" --> F["url with origin/pathname/href"]
G["CSRF middleware"] -- "Add App Router support" --> H["verifyCsrfToken function"]
I["MDX modules"] -- "New implementations" --> J["get-nodes, get-config, get-files, TOC, client"]
K["Test suites"] -- "Modernize to Node.js native" --> L["Improved coverage"]
M["Next.js 13+"] -- "Add native support" --> N["transpilePackages config"]
File Changes1. packages/next-images/test/index.test.ts
|
Code Review by Qodo
1. parseUrl returns legacy fields
|
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
The PR includes a modernization of the OAuth client implementation in next-auth, replacing the legacy 'oauth' package with a native fetch-based solution. The Google provider remains unchanged and should continue to function correctly. The refactor also adds support for the new 'checks' property in the OAuth provider configuration, with backward compatibility for existing 'protection' property usage. Overall, the changes are well-implemented and should resolve the CI regressions mentioned in the PR description.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
23
to
+34
| const baseUrl = _host ? `${protocol}://${_host}` : defaultHost | ||
| const basePath = _path.length > 0 ? `/${_path.join('/')}` : defaultPath | ||
|
|
||
| return { baseUrl, basePath } | ||
| return { | ||
| baseUrl, | ||
| basePath, | ||
| url: { | ||
| origin: baseUrl, | ||
| pathname: basePath, | ||
| href: `${baseUrl}${basePath}` | ||
| } | ||
| } |
There was a problem hiding this comment.
1. parseurl returns legacy fields 📎 Requirement gap ✓ Correctness
parseUrl still returns baseUrl and basePath (and downstream code still destructures them), leaving legacy URL handling in place alongside the new url object. This fails the requirement to refactor away remaining baseUrl/basePath usage for consistent URL handling.
Agent Prompt
## Issue description
`baseUrl` and `basePath` are still returned from `parseUrl` and are still consumed by server logic, leaving legacy URL handling in place. The compliance requirement is to refactor all `baseUrl`/`basePath` references to a consistent modern URL approach.
## Issue Context
The PR introduces a structured `url` object (`origin`, `pathname`, `href`), but legacy fields remain part of the return value and are still destructured by `NextAuthHandler`.
## Fix Focus Areas
- packages/next-auth/src/lib/parse-url.js[23-34]
- packages/next-auth/src/server/index.js[71-106]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1
to
+27
| import * as React from 'react'; | ||
| import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; | ||
| import { ReactQueryDevtools } from '@tanstack/react-query-devtools'; | ||
| import { AuthScreen } from '@/components/auth-screen'; | ||
| import { UserInfo } from '@/components/user-info'; | ||
| import { AuthLoader } from '@/lib/auth'; | ||
| import { Container } from '@/components/ui'; | ||
|
|
||
| const SampleApp = () => { | ||
| const [queryClient] = React.useState(() => new QueryClient()); | ||
|
|
||
| return ( | ||
| <Container> | ||
| <QueryClientProvider client={queryClient}> | ||
| <ReactQueryDevtools /> | ||
| <AuthLoader | ||
| renderLoading={() => <div>Loading ...</div>} | ||
| renderUnauthenticated={() => <AuthScreen />} | ||
| > | ||
| <UserInfo /> | ||
| </AuthLoader> | ||
| </QueryClientProvider> | ||
| </Container> | ||
| ); | ||
| }; | ||
|
|
||
| export default SampleApp; |
There was a problem hiding this comment.
2. Example added under packages/ 📘 Rule violation ⛯ Reliability
New demo/example code was added under packages/react-query-auth/examples/vite, which is outside the allowed demo/example locations. This breaks the documented monorepo layout conventions for discoverability and consistency.
Agent Prompt
## Issue description
The PR adds an example app under `packages/react-query-auth/examples/vite`, which violates the monorepo layout rule for demos/examples.
## Issue Context
Demos/examples must live in `examples/`, `test-app/`, or `www/` to keep the repo consistent and discoverable.
## Fix Focus Areas
- packages/react-query-auth/examples/vite/src/App.tsx[1-27]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1
to
+5
| import glob from "fast-glob" | ||
| import path from "path" | ||
|
|
||
| import { getSourceConfig } from "./get-config" | ||
|
|
There was a problem hiding this comment.
3. get-files.ts not prettier-formatted 📘 Rule violation ⛯ Reliability
The newly added packages/next-mdx/src/get-files.ts uses double quotes and omits semicolons, conflicting with the repository's required Prettier formatting rules. This introduces inconsistent formatting into the codebase.
Agent Prompt
## Issue description
`packages/next-mdx/src/get-files.ts` does not match the required formatting rules (e.g., double quotes and missing semicolons).
## Issue Context
The monorepo treats Prettier as the source of truth; changes must follow the specified formatting rules.
## Fix Focus Areas
- packages/next-mdx/src/get-files.ts[1-32]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| @@ -0,0 +1,27 @@ | |||
| import { describe, it, expect, vi, beforeEach } from 'vitest'; | |||
There was a problem hiding this comment.
4. unit.test.js wrong extension 📘 Rule violation ⛯ Reliability
A new test file was added as packages/next-pwa/test/unit.test.js, but test files are required to be named *.test.ts or *.test.tsx. This can prevent standard tooling from consistently discovering/running the test suite.
Agent Prompt
## Issue description
A new test was added as `*.test.js`, but the repo requires test files to be named `*.test.ts` or `*.test.tsx`.
## Issue Context
Test discovery conventions rely on consistent naming patterns.
## Fix Focus Areas
- packages/next-pwa/test/unit.test.js[1-27]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+13
to
+16
| return { | ||
| getOAuthAccessToken: (code, codeVerifier) => getOAuth2AccessToken(code, provider, codeVerifier), | ||
| get: (accessToken, results) => getOAuth2(provider, accessToken, results) | ||
| } |
There was a problem hiding this comment.
5. Oauth2 authorize url crash 🐞 Bug ✓ Correctness
getAuthorizationUrl() calls client.getAuthorizeUrl() for OAuth 2.x providers, but oAuthClient() now returns an object without that function, causing a runtime TypeError and breaking all OAuth 2.x sign-ins.
Agent Prompt
### Issue description
OAuth 2.x sign-in flow calls `client.getAuthorizeUrl(...)`, but the refactored `oAuthClient()` no longer returns an object that implements `getAuthorizeUrl()`. This will throw at runtime and prevent any OAuth 2.x provider sign-in.
### Issue Context
The PR replaced the `oauth` package and now returns a plain object for OAuth 2.x with only `getOAuthAccessToken` and `get` methods.
### Fix Focus Areas
- packages/next-auth/src/server/lib/oauth/client.js[11-17]
- packages/next-auth/src/server/lib/signin/oauth.js[14-34]
### Notes
Either add a `getAuthorizeUrl(params)` implementation back onto the OAuth2 client (to keep existing call sites) or refactor the call site to build the authorization URL using `URL` and `URLSearchParams`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+225
to
+238
| async getOAuthRequestToken(params = {}) { | ||
| // Implement OAuth 1.0a request token logic | ||
| throw new Error("OAuth 1.0a is not yet fully implemented in the native client. Please use OAuth 2.0 or contact maintainers.") | ||
| } | ||
|
|
||
| async getOAuthAccessToken(oauth_token, oauth_token_secret, oauth_verifier) { | ||
| // Implement OAuth 1.0a access token logic | ||
| throw new Error("OAuth 1.0a is not yet fully implemented in the native client.") | ||
| } | ||
|
|
||
| async get(url, oauth_token, oauth_token_secret) { | ||
| // Implement OAuth 1.0a authenticated request | ||
| throw new Error("OAuth 1.0a is not yet fully implemented in the native client.") | ||
| } |
There was a problem hiding this comment.
6. Oauth1 sign-in always fails 🐞 Bug ✓ Correctness
The new OAuth1Client methods (getOAuthRequestToken, getOAuthAccessToken, get) unconditionally throw "not yet fully implemented" errors, so OAuth 1.x providers (e.g., Twitter version: "1.0A") cannot complete sign-in.
Agent Prompt
### Issue description
OAuth 1.x support is currently broken: `OAuth1Client` is a placeholder that throws for all required methods. OAuth 1.x providers (e.g., Twitter) will fail during sign-in when `getOAuthRequestToken` is called.
### Issue Context
The PR removed the legacy `oauth` dependency and introduced a minimal OAuth1 client, but it is not implemented.
### Fix Focus Areas
- packages/next-auth/src/server/lib/oauth/client.js[19-21]
- packages/next-auth/src/server/lib/oauth/client.js[215-239]
- packages/next-auth/src/server/lib/signin/oauth.js[39-52]
- packages/next-auth/src/providers/twitter.js[1-12]
### Notes
Preferred: keep OAuth2 native refactor, but preserve existing OAuth1 behavior by either reintroducing a working OAuth1 implementation (could still be internal) or continuing to use the prior OAuth library for OAuth1 only.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Owner
Author
|
|
1 similar comment
Owner
Author
|
|
added 3 commits
March 14, 2026 22:16
… .test.ts - Add missing getAuthorizeUrl to OAuth2 client to prevent crash - Reintroduce oauth package for OAuth1 support - Implement OAuth1Client using oauth library with promise wrappers - Rename next-pwa unit test to .test.ts to match repo conventions - Format changed files with Prettier Closes #... (addresses Qodo review issues)
Owner
Author
|
|
13 similar comments
Owner
Author
|
|
Owner
Author
|
|
Owner
Author
|
|
Owner
Author
|
|
Owner
Author
|
|
Owner
Author
|
|
Owner
Author
|
|
Owner
Author
|
|
Owner
Author
|
|
Owner
Author
|
|
Owner
Author
|
|
Owner
Author
|
|
Owner
Author
|
|
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.
Closes #25.
parseUrlto return a structuredurlobject (origin,pathname,href).NextAuthHandlerto use the newurlobject and updatedreq.options.provider.protectiontoprovider.checksand removedprovider.statein favor ofchecks(supporting 'pkce', 'state', 'none').oauthpackage with a nativefetch-based implementation for OAuth 2.x.oauthandquerystringfrom dependencies.OAuthConfigandAppProvider.