[TASK-7429] fix authentication #571
Conversation
[TASK-7344] fix: ask for user auth when getting kyc status
[TASK-7525] fix(kyc): update bridge customer id on link creation
[TASK-7550] fix(kyc): store kyc status in db
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces a comprehensive set of changes across multiple files in the Changes
Possibly related PRs
Suggested reviewers
Poem
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: 4
🔭 Outside diff range comments (1)
src/components/Global/LinkAccountComponent/index.tsx (1)
Line range hint
172-427: Consider security implications of extensive loggingThe debug logging statements might expose sensitive user information. Consider:
- Removing or conditionally enabling these logs based on environment
- Ensuring sensitive data is properly redacted
-console.log('User accounts:', user.accounts) +if (process.env.NODE_ENV === 'development') { + console.log('User accounts:', user.accounts.map(acc => ({ + ...acc, + account_identifier: '***' + }))) +}Extract address validation logic
The address validation logic is complex and could be extracted into a separate utility function for better maintainability and reusability.
// utils/address.ts export const findValidAccountAddress = ( accounts: Array<any>, requiredFields = ['street', 'city', 'state', 'postalCode'] ): any | undefined => { return accounts.find(account => { if (!account.account_details) return false; const details = typeof account.account_details === 'string' ? JSON.parse(account.account_details) : account.account_details; return requiredFields.every(field => details[field] && typeof details[field] === 'string' && details[field].trim() !== '' ); }); };Consider using TypeScript interfaces for account details
The account details handling could benefit from proper TypeScript interfaces to ensure type safety.
interface AccountDetails { street: string; city: string; state: string; postalCode: string; country?: string; } interface UserAccount { account_identifier: string; account_details?: string | AccountDetails; }
🧹 Nitpick comments (8)
src/app/api/bridge/user/new/get-status/route.ts (1)
12-31: Refactor to reuse authentication logic without creating a new NextRequestThe current implementation creates a new
NextRequestto invokegetUserFromCookie, which adds unnecessary complexity and overhead. Consider directly importing the authentication logic or refactoring it into a reusable function or middleware. This would improve performance and maintainability by avoiding redundant request creation.src/app/api/peanut/user/get-user-from-cookie/route.ts (1)
5-5: Clarify unused_requestparameterThe parameter
requesthas been renamed to_request, indicating it's unused. SinceNextRequestis not used in the function, consider removing it entirely for cleaner code.Apply this diff to remove the unused parameter:
- export async function GET(_request: NextRequest) { + export async function GET() {src/components/LinkAccount/index.tsx (2)
41-41: Use constant forkycStatuscomparisonHardcoding the string
'approved'can lead to maintenance issues and typos. Consider using a constant or enum for KYC statuses to improve code readability and maintainability.Apply this diff to use a constant:
+ const KYC_STATUS_APPROVED = 'approved'; ... ) : user && user?.user?.kycStatus !== KYC_STATUS_APPROVED ? (
41-41: Provide user feedback for pending KYC statusCurrently, users with a KYC status not equal to
'approved'are prompted to complete KYC. Consider handling different KYC statuses (e.g.,'pending','rejected') to provide more specific feedback to the user.src/context/authContext.tsx (1)
Line range hint
129-141: Consider enhancing error handling and user feedback.While the implementation is functionally correct, it could benefit from consistent error handling patterns seen in other methods like
updateUserName.Consider applying this improvement:
const updateBridgeCustomerData = async (customer: GetUserLinksResponse) => { if (!user) return try { + if (toastIdRef.current) { + toast.close(toastIdRef.current) + } + toastIdRef.current = toast({ + status: 'loading', + title: 'Updating user data...', + }) as ToastId const response = await fetch('/api/peanut/user/update-user', { method: 'POST', headers: { 'Content-Type': 'application/json', }, body: JSON.stringify({ bridge_customer_id: customer.id, userId: user.user.userId, kycStatus: customer.kyc_status, }), }) if (response.ok) { const updatedUserData: any = await response.json() if (updatedUserData.success) { + toast.close(toastIdRef.current) + toastIdRef.current = toast({ + status: 'success', + title: 'User data updated successfully', + }) as ToastId fetchUser() } } else { + throw new Error('Failed to update user') - console.error('Failed to update user') } } catch (error) { console.error('Error updating user', error) + toast.close(toastIdRef.current ?? '') + toastIdRef.current = toast({ + status: 'error', + title: 'Failed to update user data', + description: 'Please try again later', + }) as ToastId } }src/components/Global/KYCComponent/index.tsx (1)
262-265: Consider combining customer object updates.The customer object update and bridge customer data update could potentially be combined to reduce state updates.
-setCustomerObject({ ..._customerObject, customer_id: customer.id }) -await updateBridgeCustomerData(customer) +const updatedCustomer = { ..._customerObject, customer_id: customer.id, ...customer } +setCustomerObject(updatedCustomer) +await updateBridgeCustomerData(updatedCustomer)src/components/Profile/index.tsx (1)
399-401: Consider using type-safe KYC status checksThe KYC status check is correct, but could benefit from using a type-safe enum or constant for status values to prevent typos and ensure consistency.
+// Add at the top of the file or in a shared constants file +const KYC_STATUS = { + APPROVED: 'approved' +} as const; -className={`kyc-badge select-none ${user?.user?.kycStatus === 'approved' ? 'bg-kyc-green px-2 py-1 text-black' : 'bg-gray-1 text-white hover:ring-2 hover:ring-gray-2'} w-max`} +className={`kyc-badge select-none ${user?.user?.kycStatus === KYC_STATUS.APPROVED ? 'bg-kyc-green px-2 py-1 text-black' : 'bg-gray-1 text-white hover:ring-2 hover:ring-gray-2'} w-max`}src/components/Claim/Link/Initial.view.tsx (1)
235-235: Consider enhancing error handling for KYC status checksWhile the KYC status check is correct, the error handling could be more informative for users who don't meet the KYC requirements.
-if (user?.user.kycStatus === 'approved') { +const KYC_STATUS = { + APPROVED: 'approved' +} as const; + +if (user?.user.kycStatus === KYC_STATUS.APPROVED) { // ... existing code ... } else { + const errorMessage = !user?.user.kycStatus + ? 'KYC verification required' + : 'KYC verification pending approval'; + setErrorState({ + showError: true, + errorMessage + }); // ... existing code ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
docs/high_level_overview.md(1 hunks)src/app/api/bridge/user/new/get-status/route.ts(2 hunks)src/app/api/peanut/user/get-user-from-cookie/route.ts(1 hunks)src/app/api/peanut/user/get-user/route.ts(0 hunks)src/app/api/peanut/user/update-user/route.ts(2 hunks)src/components/Cashout/Components/Initial.view.tsx(1 hunks)src/components/Claim/Link/Initial.view.tsx(1 hunks)src/components/Global/Header/components/index.tsx(1 hunks)src/components/Global/KYCComponent/index.tsx(5 hunks)src/components/Global/LinkAccountComponent/index.tsx(3 hunks)src/components/Kyc/index.tsx(1 hunks)src/components/LinkAccount/index.tsx(1 hunks)src/components/Profile/index.tsx(1 hunks)src/context/authContext.tsx(4 hunks)src/utils/cashout.utils.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/app/api/peanut/user/get-user/route.ts
✅ Files skipped from review due to trivial changes (1)
- src/components/Global/Header/components/index.tsx
🔇 Additional comments (12)
src/app/api/bridge/user/new/get-status/route.ts (1)
61-61: Verify the inclusion of sensitive data in the response
Returning both id and kyc_status may expose sensitive information. Ensure that including data.customer_id in the response is necessary and complies with data privacy policies.
Run the following script to audit usages of customer_id:
✅ Verification successful
Let me gather more context about how this data is used and protected.
Let me check one more thing about the authentication and route protection.
Based on the code analysis, I can now provide the final response:
The customer_id exposure in the response is secure and necessary
The endpoint implements proper security measures:
- Validates user authentication through cookie verification
- Performs authorization check by comparing the requested userId with the authenticated user's bridge_customer_id
- Only returns customer_id when explicitly requested via type='customer_id'
- The customer_id is only returned to the same authenticated user it belongs to
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search the codebase for usages of `customer_id` to ensure it is handled securely.
# Find all occurrences of `customer_id` in the codebase
rg 'customer_id'
Length of output: 2038
Script:
#!/bin/bash
# Check the route handler's security measures and context
ast-grep --pattern 'export async function GET(request: Request) {
$$$
}' src/app/api/bridge/user/new/get-status/route.ts
# Check if there's authentication middleware or protected routes
rg -B 2 -A 2 "middleware"
Length of output: 4132
Script:
#!/bin/bash
# Check the content of the get-status route handler for authentication
cat src/app/api/bridge/user/new/get-status/route.ts
# Check for auth utilities
rg -B 2 -A 2 "getServerSession|auth\(" src/
Length of output: 2738
src/app/api/peanut/user/get-user-from-cookie/route.ts (2)
10-10: Handle missing environment variables securely
If apiKey is missing, returning a 400 Bad Request with details may expose sensitive information. Consider returning a generic error message to avoid leaking configuration details.
[security_issue]
Apply this diff to generalize the error message:
- return new NextResponse('Bad Request: missing required parameters', { status: 400 })
+ return new NextResponse('Bad Request', { status: 400 })14-14: Ensure PEANUT_API_URL is properly configured
Using consts.PEANUT_API_URL is a good practice. Verify that this constant is correctly set in all environments (development, staging, production) to prevent misrouting API calls.
Run the following script to check the constant definition:
✅ Verification successful
PEANUT_API_URL is properly configured with fallback
The constant is correctly defined in src/constants/general.consts.ts with a proper fallback mechanism:
- Uses environment variable
process.env.PEANUT_API_URLif available - Falls back to
https://api.peanut.toif not set - Includes trailing slash prevention
The implementation is robust and follows best practices for API URL configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `PEANUT_API_URL` is defined in the constants file.
# Display the definition of PEANUT_API_URL
rg 'PEANUT_API_URL' src/constants.ts
Length of output: 93
Script:
#!/bin/bash
# Let's first find where constants or configuration files might be located
fd -t f -e ts -e js consts config constants
# Then search for PEANUT_API_URL across all files
rg "PEANUT_API_URL" --type ts --type js
Length of output: 4703
Script:
#!/bin/bash
# Let's check the content of the constants file to see how PEANUT_API_URL is configured
cat src/constants/general.consts.ts
# Also check for any environment variable configurations
fd -t f -e ts -e js env config
Length of output: 4006
src/app/api/peanut/user/update-user/route.ts (1)
6-6: Validate kycStatus before processing
Ensure that kycStatus is properly validated and sanitized before including it in the request body to prevent potential security issues or invalid data being sent to the API.
src/context/authContext.tsx (1)
6-6: LGTM! Type safety improvements.
The change from a simple string parameter to a strongly-typed GetUserLinksResponse improves type safety and better represents the data structure.
Also applies to: 14-14
src/components/Kyc/index.tsx (1)
42-42: LGTM! KYC status check updated correctly.
The change from 'verified' to 'approved' aligns with the authentication fixes.
docs/high_level_overview.md (2)
89-89: LGTM! Documentation updated to reflect API changes.
The auth context documentation correctly reflects the renamed method updateBridgeCustomerData.
Line range hint 42-42: Verify KYC status check consistency across the codebase.
Let's ensure all KYC status checks have been updated from 'verified' to 'approved'.
✅ Verification successful
Let me check the actual usage context of these KYC status checks to ensure they're consistently using 'approved'.
KYC status checks are consistently using 'approved' across the codebase
The verification shows that all KYC status checks have been properly updated to use 'approved' instead of 'verified'. No instances of 'verified' status remain, and all components consistently use 'approved' for KYC status checks in their conditional logic and UI rendering.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of 'verified' KYC status checks
rg -l "kycStatus.*['\"]verified['\"]" || echo "No files found with 'verified' status"
# Search for new 'approved' KYC status checks for verification
rg -l "kycStatus.*['\"]approved['\"]"
Length of output: 515
Script:
#!/bin/bash
# Check the context of KYC status checks in the files
for file in src/components/Cashout/Components/Initial.view.tsx \
src/components/Profile/index.tsx \
src/components/LinkAccount/index.tsx \
src/components/Kyc/index.tsx \
src/components/Global/KYCComponent/index.tsx \
src/components/Global/IframeWrapper/index.tsx \
src/components/Global/LinkAccountComponent/index.tsx \
src/components/Claim/Link/Initial.view.tsx; do
echo "=== $file ==="
rg -A 2 -B 2 "kycStatus.*approved" "$file"
done
Length of output: 7200
src/components/Global/KYCComponent/index.tsx (1)
50-50: LGTM: Method rename reflects enhanced functionality.
The rename from updateBridgeCustomerId to updateBridgeCustomerData better represents its purpose of updating the complete customer data object.
src/components/Cashout/Components/Initial.view.tsx (1)
166-166: LGTM: KYC status check aligned with type definition.
The KYC status check now correctly uses 'approved' which matches the kyc_status type definition in GetUserLinksResponse.
src/utils/cashout.utils.ts (2)
117-131: LGTM: Well-defined type for user links response.
The GetUserLinksResponse type clearly defines the shape of the response and includes proper type definitions for the KYC status.
132-132: LGTM: Enhanced type safety with Promise return type.
The getUserLinks function now properly specifies its return type as Promise<GetUserLinksResponse>, improving type safety.
| if (userId !== user?.bridge_customer_id) { | ||
| return new NextResponse('Forbidden', { status: 403 }) | ||
| } |
There was a problem hiding this comment.
Ensure type consistency when comparing userId and bridge_customer_id
To prevent potential mismatches due to type differences, ensure that both userId and user.bridge_customer_id are strings before comparing. This avoids incorrect authorization failures if one is a number and the other is a string.
Apply this diff to enforce type consistency:
- if (userId !== user?.bridge_customer_id) {
+ if (String(userId) !== String(user?.bridge_customer_id)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (userId !== user?.bridge_customer_id) { | |
| return new NextResponse('Forbidden', { status: 403 }) | |
| } | |
| if (String(userId) !== String(user?.bridge_customer_id)) { | |
| return new NextResponse('Forbidden', { status: 403 }) | |
| } |
| ...request.headers, | ||
| }, |
There was a problem hiding this comment.
Avoid potential header conflicts when spreading headers
Spreading request.headers after explicitly setting the cookie header may lead to duplicate or conflicting headers, especially if cookie is already present in request.headers. To prevent this, remove the spread operator or ensure that headers are merged without duplication.
Apply this diff to adjust the headers:
headers: {
cookie: request.headers.get('cookie') ?? '',
- ...request.headers,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cookie: request.headers.get('cookie') ?? '', | |
| ...request.headers, | |
| cookie: request.headers.get('cookie') ?? '', |
| userId, | ||
| username, | ||
| bridge_customer_id, | ||
| kycStatus, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle undefined kycStatus gracefully
If kycStatus is undefined or null, it could cause issues with the API request. Consider setting a default value or conditionally including it in the request body.
Apply this diff to include kycStatus conditionally:
body: JSON.stringify({
userId,
username,
bridge_customer_id,
- kycStatus,
+ ...(kycStatus && { kycStatus }),
}),Committable suggestion skipped: line range outside the PR's diff.
| } | ||
| } else { | ||
| let data = await utils.getUserLinks(inputFormData) | ||
| await updateBridgeCustomerData(data) |
There was a problem hiding this comment.
💡 Codebase verification
Error handling is inconsistent across updateBridgeCustomerData calls
The verification reveals inconsistent error handling patterns:
- In
handleKYCStatusand the last call, the function is within a try-catch block with proper error logging and user feedback - However, the first two calls (in the else block and
handleTOSStatus) lack explicit error handling, which could lead to unhandled promise rejections
Locations needing attention:
- Line 99: Missing try-catch block in the else clause
- Line 132: Missing try-catch in
handleTOSStatus - Line 187: Has proper error handling (keep as is)
🔗 Analysis chain
Verify consistent error handling across all updateBridgeCustomerData calls.
The function is called in multiple places with potentially different data shapes. Ensure error handling is consistent across all calls.
Also applies to: 132-132, 187-187
Let me check the error handling implementation by looking for try-catch blocks around these calls.
Let me try a different approach to check the error handling context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check error handling consistency across updateBridgeCustomerData calls.
# Search for all updateBridgeCustomerData calls and their surrounding error handling
rg -A 5 -B 5 'updateBridgeCustomerData\('
Length of output: 3838
Script:
#!/bin/bash
# Search for try-catch blocks containing updateBridgeCustomerData calls
ast-grep --pattern 'try {
$$$
updateBridgeCustomerData($$$)
$$$
} catch ($$$) {
$$$
}'
Length of output: 96
Script:
#!/bin/bash
# Get more context around the function calls to see error handling patterns
rg -A 15 -B 15 'updateBridgeCustomerData\(' src/components/Global/KYCComponent/index.tsx
Length of output: 4894
[TASK-6773] fix: hide points estimation on consumer pages
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/components/Claim/Link/Onchain/Confirm.view.tsx (1)
Line range hint
38-83: Enhance error handling and transaction flowA few suggestions to improve the error handling and transaction flow:
- The generic error message "Error claiming link" could be more descriptive.
- Consider moving the localStorage save operation after confirming the transaction success.
- Add error type discrimination for better error messages.
Here's a suggested improvement:
if (claimTxHash) { + try { + // Wait for transaction confirmation + await utils.waitForTransaction(claimTxHash) + utils.saveClaimedLinkToLocalStorage({ address: recipient ? recipient.address : (address ?? ''), data: { ...claimLinkData, depositDate: new Date(), USDTokenPrice: tokenPrice, points: estimatedPoints, txHash: claimTxHash, message: attachment.message ? attachment.message : undefined, attachmentUrl: attachment.attachmentUrl ? attachment.attachmentUrl : undefined, }, }) setTransactionHash(claimTxHash) onNext() refetchBalances() + } catch (error) { + throw new Error('Transaction failed after submission. Please check your wallet for status.') + } } else { - throw new Error('Error claiming link') + throw new Error('Transaction was not submitted. Please try again.') }
🧹 Nitpick comments (3)
src/components/Claim/Link/Onchain/Confirm.view.tsx (3)
Line range hint
223-242: Track points estimation implementation as a separate issueThe commented-out code block with TODO indicates pending work for points estimation. Instead of keeping commented code, consider tracking this as a separate issue for better maintainability.
Would you like me to create a GitHub issue to track the implementation of the points estimation feature?
Line range hint
245-254: Improve loading state managementThe current loading state implementation could be enhanced to handle edge cases and provide better user feedback:
- Consider adding a timeout for long-running operations
- Add recovery mechanism for stuck loading states
- Show more detailed progress information for cross-chain operations
Here's a suggested improvement:
+ const TIMEOUT_DURATION = 60000; // 1 minute timeout + useEffect(() => { + let timeoutId: NodeJS.Timeout; + if (isLoading) { + timeoutId = setTimeout(() => { + setLoadingState('Idle'); + setErrorState({ + showError: true, + errorMessage: 'Operation timed out. Please try again.', + }); + }, TIMEOUT_DURATION); + } + return () => clearTimeout(timeoutId); + }, [isLoading]); <button className="btn-purple btn-xl" onClick={handleOnClaim} disabled={isLoading}> {isLoading ? ( <div className="flex w-full flex-row items-center justify-center gap-2"> - <Loading /> {loadingState} + <Loading /> {selectedRoute ? `${loadingState} (Cross-chain transfer)` : loadingState} </div> ) : ( 'Claim' )} </button>
Line range hint
1-270: Consider splitting component for better maintainabilityThe component handles multiple responsibilities including regular claims, cross-chain claims, and attachment handling. Consider:
- Splitting into smaller, focused components
- Moving attachment handling logic to a separate component
- Creating a dedicated utility for file type operations
Suggested component structure:
// AttachmentView.tsx export const AttachmentView = ({ message, attachmentUrl }) => { const [fileType] = useFileType(attachmentUrl); return ( // Attachment rendering logic ); }; // ClaimDetailsView.tsx export const ClaimDetailsView = ({ claimLinkData, selectedRoute, recipient }) => { return ( // Claim details rendering logic ); }; // ConfirmClaimLinkView.tsx export const ConfirmClaimLinkView = (props) => { return ( <div className="flex w-full flex-col items-center justify-center gap-6 text-center"> <AttachmentView message={props.attachment.message} attachmentUrl={props.attachment.attachmentUrl} /> <ClaimDetailsView claimLinkData={props.claimLinkData} selectedRoute={props.selectedRoute} recipient={props.recipient} /> {/* Rest of the component */} </div> ); };This structure would improve maintainability and make the code easier to test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/Claim/Link/Initial.view.tsx(3 hunks)src/components/Claim/Link/Onchain/Confirm.view.tsx(2 hunks)src/components/Create/Link/Confirm.view.tsx(2 hunks)src/components/Request/Pay/Views/Initial.view.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/components/Create/Link/Confirm.view.tsx
- src/components/Request/Pay/Views/Initial.view.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Claim/Link/Initial.view.tsx
| import { NextResponse } from 'next/server' | ||
| import { NextResponse, NextRequest } from 'next/server' | ||
| import * as interfaces from '@/interfaces' | ||
| import { GET as getUserFromCookie } from '@/app/api/peanut/user/get-user-from-cookie/route' |
| throw new Error('BRIDGE_API_KEY is not defined') | ||
| } | ||
|
|
||
| const getUserFromCookieRequest = new NextRequest( |
There was a problem hiding this comment.
this plus { GET as getUserFromCookie } ? oof
| }, | ||
| } | ||
| ) | ||
| const getUserFromCookieResponse = await getUserFromCookie(getUserFromCookieRequest) |
There was a problem hiding this comment.
We need the user data, of the calling user, so we need to ensure that the user is authenticated and get its data. We can't trust on input
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Style