fix: ignore pre-fetch requests on auth routes #251
Conversation
DanielRivers
requested review from
DaveOrDead and
peterphanouvong
as code owners
WalkthroughThe pull request introduces several modifications across multiple handler functions in the codebase. Key changes include the addition of type definitions for the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
src/utils/isPreFetch.ts (2)
4-6: Consider using a constant for the 'prefetch' string.The implementation is correct, but we can improve maintainability by extracting the repeated string.
+const PREFETCH_HEADER_VALUE = 'prefetch'; + export function isPreFetch(req: NextRequest): boolean { - const isPrefetch = req?.headers['purpose'] === 'prefetch' || - req?.headers['x-purpose'] === 'prefetch' || - req?.headers['x-moz'] === 'prefetch'; + const isPrefetch = req?.headers['purpose'] === PREFETCH_HEADER_VALUE || + req?.headers['x-purpose'] === PREFETCH_HEADER_VALUE || + req?.headers['x-moz'] === PREFETCH_HEADER_VALUE;
3-9: Add JSDoc documentation for the utility function.Please add documentation explaining the purpose of this function and its role in handling pre-fetch requests.
+/** + * Determines if the incoming request is a pre-fetch request by examining various headers. + * This is used to prevent unnecessary processing of pre-fetch requests on auth routes. + * + * @param req - The Next.js request object + * @returns boolean indicating if the request is a pre-fetch request + */ export function isPreFetch(req: NextRequest): boolean {src/handlers/createOrg.ts (2)
Line range hint
6-6: Add type safety for the routerClient parameter.For consistency with other handlers and improved type safety, add explicit typing for the routerClient parameter.
-export const createOrg = async (routerClient) => { +export const createOrg = async (routerClient: RouterClient) => {
Add pre-fetch check for consistency with other auth handlers
The verification shows that all other auth handlers (
login,logout, andregister) implement pre-fetch checking using theisPreFetchutility, butcreateOrghandler doesn't. Add the following at the start of the handler:if (isPreFetch(routerClient.req)) { return null; }
src/handlers/createOrg.ts: Add import statement forisPreFetchfrom "../utils/isPreFetch"🔗 Analysis chain
Line range hint
1-19: Consider adding pre-fetch check for consistency.Other auth handlers now implement pre-fetch checking. Should this handler also ignore pre-fetch requests for consistency?
Let's check if this handler is used in similar contexts:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage patterns of createOrg compared to other handlers echo "Checking createOrg usage patterns:" rg -A 2 "createOrg|login|logout|register" --type ts # Check if this is exposed via similar routes echo "Checking route definitions:" fd -e ts -e js --full-path "*/api/*" --exec rg -l "createOrg|login|logout|register" {}Length of output: 5568
Script:
#!/bin/bash # Let's check for the isPreFetch function implementation and its usage echo "Checking isPreFetch implementation and usage:" rg -A 2 "isPreFetch" --type ts # Let's also check the createOrg route definition echo "Checking createOrg route definition:" rg -A 2 "createOrg.*route" --type tsLength of output: 1540
src/handlers/logout.ts (1)
Line range hint
18-28: Consider additional URL sanitizationThe post-logout redirect URL handling looks good, but consider adding additional validation to prevent open redirect vulnerabilities.
Consider:
- Validating against an allowlist of domains
- Adding URL pattern validation
- Implementing strict URL parsing checks
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/handlers/createOrg.ts(1 hunks)src/handlers/login.ts(2 hunks)src/handlers/logout.ts(2 hunks)src/handlers/register.ts(2 hunks)src/utils/isPreFetch.ts(1 hunks)
🔇 Additional comments (6)
src/handlers/login.ts (2)
2-2: Implementation looks good!
The changes effectively address the pre-fetch handling requirement while maintaining existing functionality:
- Type safety improvements ✓
- Pre-fetch detection ✓
- Existing redirect logic preserved ✓
Also applies to: 8-31
8-11: Verify type safety of null return.
The implementation looks good, but let's ensure the return type allows null.
✅ Verification successful
Let me gather more information about the return type of the login handler and related handlers.
Let me check one more thing to verify the return types of these handlers.
Return type of null is safe in this context
Based on the codebase analysis:
- Multiple handlers (
register,logout,login) follow the same pattern of returningnullfor pre-fetch checks - The handlers don't have explicit return type annotations, suggesting flexible return types
- Other utility functions in the codebase (
getUser,getOrganization, etc.) commonly returnnullas a valid response - The pattern is consistently used across the authentication flow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if RouterClient methods or related types define return types
echo "Checking RouterClient type definitions:"
rg -A 5 "interface RouterClient|class RouterClient" --type ts
# Look for similar patterns in other handlers
echo "Checking return types in other handlers:"
rg -B 2 "return null" --type ts
Length of output: 2754
Script:
#!/bin/bash
# Check the type definition or implementation of login handler
ast-grep --pattern 'export const login = async $_: $_) => $_'
# Check other handler implementations for return type patterns
ast-grep --pattern 'export const $_ = async $_: RouterClient) => $_'
# Look for RouterClient usage in type definitions
rg "RouterClient" --type ts -B 2 -A 2
Length of output: 2714
Script:
#!/bin/bash
# Look for type definitions or interfaces that might define handler return types
rg "type|interface.*Handler" --type ts -B 2 -A 2
# Check if there are any type annotations in the handlers
ast-grep --pattern 'export const $_ = async ($_: RouterClient): $_ => $_'
Length of output: 13714
src/handlers/register.ts (3)
8-8: Type safety improvement looks good!
The explicit RouterClient type annotation enhances code reliability and IDE support.
31-31: Clean return statement refactor
Good simplification of the return statement by removing unnecessary void.
9-11: Verify pre-fetch handling across all auth routes
The pre-fetch check implementation looks correct. However, let's verify consistent implementation across other auth routes.
✅ Verification successful
Pre-fetch handling is consistent across auth routes with minor logging difference
The verification shows that isPreFetch check is consistently implemented in all auth handlers (login, logout, and register), with the same pattern:
- All handlers import
isPreFetchfrom "../utils/isPreFetch" - All handlers check
isPreFetch(routerClient.req)at the start - All handlers return
nullon pre-fetch
Minor note: The logout handler includes an extra console.log("isPreFetch") statement, but this doesn't affect the functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if isPreFetch is consistently implemented across auth handlers
# Expected: All auth handlers should implement the pre-fetch check similarly
# Search for auth handler files and check for isPreFetch implementation
rg -l "RouterClient" src/handlers/ | xargs rg "isPreFetch" -C 2
Length of output: 1538
src/handlers/logout.ts (1)
29-29: Clean return statement refactor
Good simplification of the return statement, consistent with other handlers.
DanielRivers
force-pushed
the
feat/ignore-prefetch
branch
from
93c855e to
79e91fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/utils/isPreFetch.test.ts (2)
7-11: Consider enhancing mock implementation type safetyWhile the mock function works, it could benefit from improved type safety and clarity.
Consider this enhanced implementation:
- const mockNextRequest = (headers: Record<string, string>) => { - return { - headers: new Headers(headers) - } as NextRequest; + const mockNextRequest = (headers: Record<string, string>): Pick<NextRequest, 'headers'> => { + return { + headers: new Headers(headers) + };This change:
- Uses
Picktype to be explicit about partial implementation- Removes type assertion in favor of explicit typing
- Makes it clear which parts of NextRequest are being mocked
13-49: Consider adding additional test cases for completenessThe test suite is well-structured with good coverage of basic scenarios and edge cases. However, consider adding these additional test cases for more comprehensive coverage:
Add these test cases:
it('handles case-insensitive header values', () => { const req = mockNextRequest({ purpose: 'PREFETCH', 'x-purpose': 'Prefetch' }); expect(isPreFetch(req)).toBe(true); }); it('returns true when multiple prefetch headers are present', () => { const req = mockNextRequest({ purpose: 'prefetch', 'x-purpose': 'navigation' }); expect(isPreFetch(req)).toBe(true); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/handlers/logout.ts(2 hunks)src/utils/isPreFetch.test.ts(1 hunks)src/utils/isPreFetch.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/handlers/logout.ts
- src/utils/isPreFetch.ts
🔇 Additional comments (2)
src/utils/isPreFetch.test.ts (2)
1-5: LGTM! Well-structured test file setup
The import statements are clean and follow TypeScript best practices. The test file naming convention follows the standard pattern.
1-50: Verify integration with auth route handlers
The test coverage for the isPreFetch utility is thorough. However, let's verify its integration with auth routes to ensure it fulfills the PR's objective.
✅ Verification successful
Integration with auth routes verified successfully
The isPreFetch utility is properly integrated across all auth route handlers (register, login, and logout). Each handler correctly:
- Imports the utility function
- Checks for prefetch requests early in the handler
- Returns null for prefetch requests, preventing unnecessary processing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that auth route handlers properly utilize isPreFetch
# Check for isPreFetch usage in auth-related files
echo "Checking auth route handlers for isPreFetch implementation:"
rg -l "isPreFetch" src/handlers/
Length of output: 231
Script:
#!/bin/bash
# Check implementation in auth handlers to verify proper usage
echo "=== Checking register.ts ==="
rg "isPreFetch" src/handlers/register.ts -B 2 -A 2
echo -e "\n=== Checking login.ts ==="
rg "isPreFetch" src/handlers/login.ts -B 2 -A 2
echo -e "\n=== Checking logout.ts ==="
rg "isPreFetch" src/handlers/logout.ts -B 2 -A 2
Length of output: 1114
Explain your changes
Issue: When the auth routes are pre-fetched, this could cause unexpected behaviour.
Fix: When a prefetch is noticed, ignore them.
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.