Conversation
…nto wallet-auth Merge main
WalkthroughThis change introduces a full-stack implementation of wallet-based authentication using Stellar wallets (with Freighter support). It adds backend challenge generation, signature verification, JWT issuance, user synchronization with Supabase, and frontend integration including UI components, hooks, and utilities for wallet connection and authentication. Database migrations, validation, and rate limiting are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebApp
participant Freighter
participant Backend
participant Supabase
User->>WebApp: Clicks "Connect Wallet"
WebApp->>Freighter: Requests public key
Freighter-->>WebApp: Returns public key
WebApp->>Backend: POST /api/auth/challenge { publicKey }
Backend->>Supabase: Store challenge (wallet_challenges)
Backend-->>WebApp: Returns challenge
WebApp->>Freighter: Signs challenge (as Stellar tx memo)
Freighter-->>WebApp: Returns signed tx XDR
WebApp->>Backend: POST /api/auth/wallet { publicKey, signedTransaction, challenge }
Backend->>Supabase: Validate challenge, verify signature
Backend->>Supabase: Find or create wallet user/profile
Backend-->>WebApp: Returns JWT token, user data
WebApp->>User: Authenticated, redirects to dashboard
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 12
🔭 Outside diff range comments (1)
apps/backend/package.json (1)
15-15: Remove duplicate Stellar SDK dependency.You have both
@stellar/stellar-sdk(line 15) andstellar-sdk(line 26) in your dependencies. The@stellar/stellar-sdkis the official package name, whilestellar-sdkis likely the deprecated version. Having both can cause conflicts and increase bundle size.Remove the duplicate dependency:
- "stellar-sdk": "^13.3.0",Also applies to: 26-26
🧹 Nitpick comments (2)
apps/backend/src/index.ts (1)
35-35: Remove commented code.Clean up commented code if it's no longer needed, or add a TODO comment explaining why it's temporarily disabled.
-// app.use('/api/bookings', bookingRoutes);apps/backend/src/controllers/wallet-auth.controller.ts (1)
29-34: Consider adding domain and path attributes to the cookie.For better security and control, consider explicitly setting the domain and path attributes.
res.cookie('auth-token', authResponse.token, { httpOnly: true, secure: process.env.NODE_ENV === 'production', sameSite: 'strict', maxAge: 7 * 24 * 60 * 60 * 1000, + path: '/', + domain: process.env.COOKIE_DOMAIN || undefined, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/backend/package.json(1 hunks)apps/backend/src/config/supabase.ts(2 hunks)apps/backend/src/controllers/wallet-auth.controller.ts(1 hunks)apps/backend/src/index.ts(2 hunks)apps/backend/src/middleware/rateLimiter.ts(1 hunks)apps/backend/src/routes/wallet-auth.routes.ts(1 hunks)apps/backend/src/services/wallet-auth.services.ts(1 hunks)apps/backend/src/services/wallet-challenge.service.ts(1 hunks)apps/backend/src/types/wallet-auth.types.ts(1 hunks)apps/backend/src/validators/wallet-auth.validator.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
apps/backend/src/validators/wallet-auth.validator.ts (1)
apps/backend/src/types/wallet-auth.types.ts (2)
challengeRequestSchema(6-8)walletLoginSchema(13-17)
apps/backend/src/services/wallet-auth.services.ts (3)
apps/backend/src/config/supabase.ts (1)
supabase(205-217)apps/backend/src/types/wallet-auth.types.ts (2)
WalletLoginRequest(23-23)WalletAuthResponse(55-62)apps/backend/src/services/wallet-challenge.service.ts (2)
getValidChallenge(56-73)removeChallenge(78-84)
apps/backend/src/controllers/wallet-auth.controller.ts (1)
apps/backend/src/types/wallet-auth.types.ts (2)
ChallengeRequest(22-22)WalletLoginRequest(23-23)
apps/backend/src/routes/wallet-auth.routes.ts (2)
apps/backend/src/middleware/rateLimiter.ts (2)
challengeRateLimit(30-38)walletAuthRateLimit(17-25)apps/backend/src/validators/wallet-auth.validator.ts (2)
validateChallengeRequest(5-18)validateWalletLogin(20-33)
🔇 Additional comments (13)
apps/backend/src/routes/wallet-auth.routes.ts (1)
1-18: Well-structured wallet authentication routes.The route definitions follow good practices with proper middleware chaining (rate limiting → validation → controller). The separation of challenge generation and wallet authentication endpoints is logical and secure.
apps/backend/src/middleware/rateLimiter.ts (1)
18-19: Appropriate rate limiting values for wallet authentication.The rate limits seem well-calibrated for security:
- 10 wallet auth attempts per 15 minutes prevents brute force attacks
- 20 challenge requests per 5 minutes allows legitimate usage while preventing spam
These values strike a good balance between security and usability.
Also applies to: 31-32
apps/backend/src/validators/wallet-auth.validator.ts (2)
5-18: Well-implemented validation middleware.The validation logic is correctly structured with proper error handling. The middleware appropriately validates the request body against Zod schemas and provides clear error messages with HTTP 400 status for validation failures.
20-33: Consistent validation pattern maintained.The
validateWalletLoginmiddleware follows the same pattern asvalidateChallengeRequest, maintaining consistency in error handling and response structure throughout the validation layer.apps/backend/src/services/wallet-auth.services.ts (2)
10-27: LGTM! Robust signature verification implementation.The function properly handles errors and uses the Stellar SDK correctly for signature verification.
86-102: LGTM! Secure JWT generation with proper configuration checks.Good practice checking for the JWT_SECRET environment variable and using a reasonable expiration time.
apps/backend/src/controllers/wallet-auth.controller.ts (1)
6-18: LGTM! Clean challenge generation endpoint.Good error handling and appropriate status codes.
apps/backend/src/services/wallet-challenge.service.ts (4)
10-25: LGTM! Appropriate error handling for cleanup operation.Silent failure is acceptable for cleanup operations to avoid disrupting the main flow.
30-51: LGTM! Well-structured challenge generation.Good practice to clean up expired challenges before creating new ones. Proper error handling.
56-73: LGTM! Clear and secure challenge validation.Good use of timestamp comparison to ensure challenge validity.
89-91: LGTM! Useful for scheduled cleanup tasks.apps/backend/src/config/supabase.ts (1)
27-192: LGTM! Well-structured database schema for wallet authentication.The new tables provide a solid foundation for wallet-based authentication with appropriate fields for challenges, users, and profiles.
apps/backend/src/types/wallet-auth.types.ts (1)
33-46: Consider making some UserProfile fields required.The
verification_statusandlast_activefields are not optional in the interface but are stored as required in the database schema. This inconsistency could lead to runtime errors.
| "express": "^4.18.2", | ||
| "express-rate-limit": "^7.1.5", | ||
| "jest-mock-extended": "^4.0.0", | ||
| "joi": "^17.13.3", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Question the need for Joi alongside Zod.
You're adding joi validation library while already using zod (line 28). This introduces inconsistency in validation approaches across the codebase.
Consider removing Joi and using Zod consistently throughout the application:
- "joi": "^17.13.3",If Joi is truly needed for specific use cases, please document why both validation libraries are necessary.
📝 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.
| "joi": "^17.13.3", |
🤖 Prompt for AI Agents
In apps/backend/package.json at line 23, you have added the Joi validation
library while Zod is already used at line 28, causing inconsistency. Review the
codebase to determine if Joi is essential; if not, remove the Joi dependency to
maintain a single validation library (Zod). If Joi must remain for specific
reasons, add clear documentation explaining why both Joi and Zod are required.
| app.use('/auth', authRoutes); | ||
| app.use('/api/bookings', bookingRoutes); | ||
| app.use('/api/auth', walletAuthRoutes); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify the dual authentication route structure.
Having both /auth and /api/auth routes could confuse API consumers. Consider consolidating authentication routes under a single path prefix for consistency.
Consider one of these approaches:
Option 1: Consolidate under /api/auth
-app.use('/auth', authRoutes);
-app.use('/api/auth', walletAuthRoutes);
+app.use('/api/auth', authRoutes);
+app.use('/api/auth', walletAuthRoutes);Option 2: Use distinct paths
app.use('/auth', authRoutes);
-app.use('/api/auth', walletAuthRoutes);
+app.use('/auth/wallet', walletAuthRoutes);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/backend/src/index.ts around lines 33 to 34, the use of both '/auth' and
'/api/auth' routes for authentication can confuse API consumers. To fix this,
consolidate authentication routes under a single consistent path prefix by
either merging all auth routes under '/api/auth' or clearly separating them with
distinct and descriptive paths. Update the route registrations accordingly to
maintain clarity and consistency.
| export const challengeRateLimit = rateLimit({ | ||
| windowMs: 5 * 60 * 1000, | ||
| max: 20, | ||
| message: { | ||
| error: 'Too many challenge requests, please try again later.', | ||
| }, | ||
| standardHeaders: true, | ||
| legacyHeaders: false, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add keyGenerator for consistency with development environments.
Same issue as above - the challenge rate limiter needs the keyGenerator function for development environment compatibility.
export const challengeRateLimit = rateLimit({
windowMs: 5 * 60 * 1000,
max: 20,
message: {
error: 'Too many challenge requests, please try again later.',
},
standardHeaders: true,
legacyHeaders: false,
+ keyGenerator: (req): string => {
+ // Handle localhost and development environments
+ return req.ip || 'localhost';
+ },
});📝 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.
| export const challengeRateLimit = rateLimit({ | |
| windowMs: 5 * 60 * 1000, | |
| max: 20, | |
| message: { | |
| error: 'Too many challenge requests, please try again later.', | |
| }, | |
| standardHeaders: true, | |
| legacyHeaders: false, | |
| }); | |
| export const challengeRateLimit = rateLimit({ | |
| windowMs: 5 * 60 * 1000, | |
| max: 20, | |
| message: { | |
| error: 'Too many challenge requests, please try again later.', | |
| }, | |
| standardHeaders: true, | |
| legacyHeaders: false, | |
| keyGenerator: (req): string => { | |
| // Handle localhost and development environments | |
| return req.ip || 'localhost'; | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In apps/backend/src/middleware/rateLimiter.ts between lines 30 and 38, the
challengeRateLimit configuration is missing a keyGenerator function, which is
needed for consistent behavior across development environments. Add a
keyGenerator function to the rateLimit options that returns a unique key (such
as the user's IP address or another identifier) to ensure the rate limiter works
correctly in all environments.
| export const walletAuthRateLimit = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 10, | ||
| message: { | ||
| error: 'Too many wallet authentication attempts, please try again later.', | ||
| }, | ||
| standardHeaders: true, | ||
| legacyHeaders: false, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add keyGenerator for consistency with development environments.
The new rate limiters are missing the keyGenerator function present in the original rateLimiter (lines 8-11). This could cause issues in development environments where IP detection might not work properly.
Add the keyGenerator to both new rate limiters:
export const walletAuthRateLimit = rateLimit({
windowMs: 15 * 60 * 1000, // 15 minutes
max: 10,
message: {
error: 'Too many wallet authentication attempts, please try again later.',
},
standardHeaders: true,
legacyHeaders: false,
+ keyGenerator: (req): string => {
+ // Handle localhost and development environments
+ return req.ip || 'localhost';
+ },
});🤖 Prompt for AI Agents
In apps/backend/src/middleware/rateLimiter.ts around lines 17 to 25, the
walletAuthRateLimit configuration is missing the keyGenerator function that is
present in the original rateLimiter between lines 8 and 11. To fix this, add the
same keyGenerator function to walletAuthRateLimit to ensure consistent client
identification, especially in development environments where IP detection may
fail.
| await removeChallenge(storedChallenge.id); | ||
| const walletUser = await getOrCreateWalletUser(publicKey); | ||
| const token = generateJWT(walletUser.id, publicKey); | ||
| const profile = await getUserProfile(walletUser.id); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move challenge removal to after all operations succeed.
The challenge is removed immediately after signature verification, but if subsequent operations (user creation, JWT generation) fail, the user would need to request a new challenge even though their signature was valid.
const isValidSignature = await verifySignature(publicKey, challenge, signature);
if (!isValidSignature) {
throw new Error('Invalid signature');
}
- await removeChallenge(storedChallenge.id);
const walletUser = await getOrCreateWalletUser(publicKey);
const token = generateJWT(walletUser.id, publicKey);
const profile = await getUserProfile(walletUser.id);
+
+ // Remove challenge only after all operations succeed
+ await removeChallenge(storedChallenge.id);
return {
token,🤖 Prompt for AI Agents
In apps/backend/src/services/wallet-auth.services.ts around lines 121 to 125,
the removal of the challenge occurs immediately after signature verification,
which can cause issues if later operations fail. Move the call to
removeChallenge so that it happens only after all subsequent operations
(getOrCreateWalletUser, generateJWT, getUserProfile) complete successfully,
ensuring the challenge is only removed when the entire process succeeds.
| await supabase | ||
| .from('wallet_users') | ||
| .update({ updated_at: new Date().toISOString() }) | ||
| .eq('id', existingUser.id); |
There was a problem hiding this comment.
Add error handling for the update operation.
The update operation could fail, but the error is not being checked. This could lead to silent failures.
- await supabase
+ const { error: updateError } = await supabase
.from('wallet_users')
.update({ updated_at: new Date().toISOString() })
.eq('id', existingUser.id);
+
+ if (updateError) {
+ console.error('Error updating wallet user:', updateError);
+ throw new Error('Failed to update wallet user');
+ }📝 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.
| await supabase | |
| .from('wallet_users') | |
| .update({ updated_at: new Date().toISOString() }) | |
| .eq('id', existingUser.id); | |
| const { error: updateError } = await supabase | |
| .from('wallet_users') | |
| .update({ updated_at: new Date().toISOString() }) | |
| .eq('id', existingUser.id); | |
| if (updateError) { | |
| console.error('Error updating wallet user:', updateError); | |
| throw new Error('Failed to update wallet user'); | |
| } |
🤖 Prompt for AI Agents
In apps/backend/src/services/wallet-auth.services.ts around lines 40 to 43, the
update operation on the 'wallet_users' table does not handle potential errors.
Modify the code to capture the response from the update call, check if there is
an error, and handle it appropriately, such as by logging the error or throwing
an exception to prevent silent failures.
| async function getOrCreateWalletUser(publicKey: string) { | ||
| const { data: existingUser, error: fetchError } = await supabase | ||
| .from('wallet_users') | ||
| .select('*') | ||
| .eq('public_key', publicKey) | ||
| .single(); | ||
|
|
||
| if (existingUser && !fetchError) { | ||
| await supabase | ||
| .from('wallet_users') | ||
| .update({ updated_at: new Date().toISOString() }) | ||
| .eq('id', existingUser.id); | ||
|
|
||
| return existingUser; | ||
| } | ||
|
|
||
| const { data: newUser, error: createError } = await supabase | ||
| .from('wallet_users') | ||
| .insert({ | ||
| public_key: publicKey, | ||
| }) | ||
| .select() | ||
| .single(); | ||
|
|
||
| if (createError || !newUser) { | ||
| console.error('Error creating wallet user:', createError); | ||
| throw new Error('Failed to create wallet user'); | ||
| } | ||
|
|
||
| await supabase.from('profiles').insert({ | ||
| user_id: newUser.id, | ||
| verification_status: 'unverified', | ||
| last_active: new Date().toISOString(), | ||
| }); | ||
|
|
||
| return newUser; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using upsert to handle potential race conditions.
The current implementation has a race condition where two concurrent requests with the same public key could both try to create a new user. Consider using Supabase's upsert functionality.
async function getOrCreateWalletUser(publicKey: string) {
- const { data: existingUser, error: fetchError } = await supabase
- .from('wallet_users')
- .select('*')
- .eq('public_key', publicKey)
- .single();
-
- if (existingUser && !fetchError) {
- await supabase
- .from('wallet_users')
- .update({ updated_at: new Date().toISOString() })
- .eq('id', existingUser.id);
-
- return existingUser;
- }
-
- const { data: newUser, error: createError } = await supabase
+ const { data: user, error } = await supabase
.from('wallet_users')
- .insert({
+ .upsert({
public_key: publicKey,
+ updated_at: new Date().toISOString(),
})
.select()
.single();
- if (createError || !newUser) {
- console.error('Error creating wallet user:', createError);
- throw new Error('Failed to create wallet user');
+ if (error || !user) {
+ console.error('Error upserting wallet user:', error);
+ throw new Error('Failed to upsert wallet user');
}
- await supabase.from('profiles').insert({
- user_id: newUser.id,
- verification_status: 'unverified',
- last_active: new Date().toISOString(),
- });
+ // Check if profile exists
+ const { data: profile } = await supabase
+ .from('profiles')
+ .select('*')
+ .eq('user_id', user.id)
+ .single();
+
+ if (!profile) {
+ const { error: profileError } = await supabase.from('profiles').insert({
+ user_id: user.id,
+ verification_status: 'unverified',
+ last_active: new Date().toISOString(),
+ });
+
+ if (profileError) {
+ console.error('Error creating user profile:', profileError);
+ throw new Error('Failed to create user profile');
+ }
+ }
- return newUser;
+ return user;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/backend/src/services/wallet-auth.services.ts between lines 32 and 68,
the current getOrCreateWalletUser function has a race condition because it
separately checks for an existing user and then inserts a new one, which can
cause duplicate entries if two requests run concurrently. To fix this, replace
the separate select and insert calls with a single upsert operation on the
wallet_users table using the publicKey as the conflict target. This will
atomically insert or update the user record, preventing duplicates and race
conditions.
| res.status(200).json({ | ||
| user: authResponse.user, | ||
| token: authResponse.token, | ||
| }); |
There was a problem hiding this comment.
Security: Don't expose JWT token in response body when using HTTP-only cookies.
Returning the token in the response body defeats the security benefits of HTTP-only cookies, as it makes the token accessible to JavaScript and vulnerable to XSS attacks.
res.status(200).json({
user: authResponse.user,
- token: authResponse.token,
});📝 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.
| res.status(200).json({ | |
| user: authResponse.user, | |
| token: authResponse.token, | |
| }); | |
| res.status(200).json({ | |
| user: authResponse.user, | |
| }); |
🤖 Prompt for AI Agents
In apps/backend/src/controllers/wallet-auth.controller.ts around lines 36 to 39,
the JWT token is being sent in the JSON response body, which exposes it to
JavaScript and increases XSS risk. To fix this, remove the token from the JSON
response and instead set it as an HTTP-only cookie on the response object. Keep
only the user information in the JSON response body.
| export async function removeChallenge(challengeId: string): Promise<void> { | ||
| const { error } = await supabase.from('wallet_challenges').delete().eq('id', challengeId); | ||
|
|
||
| if (error) { | ||
| console.error('Error removing challenge:', error); | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider throwing an error when challenge removal fails.
Failed challenge removal could allow challenge reuse, which is a security concern.
export async function removeChallenge(challengeId: string): Promise<void> {
const { error } = await supabase.from('wallet_challenges').delete().eq('id', challengeId);
if (error) {
console.error('Error removing challenge:', error);
+ throw new Error('Failed to remove challenge');
}
}📝 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.
| export async function removeChallenge(challengeId: string): Promise<void> { | |
| const { error } = await supabase.from('wallet_challenges').delete().eq('id', challengeId); | |
| if (error) { | |
| console.error('Error removing challenge:', error); | |
| } | |
| } | |
| export async function removeChallenge(challengeId: string): Promise<void> { | |
| const { error } = await supabase.from('wallet_challenges').delete().eq('id', challengeId); | |
| if (error) { | |
| console.error('Error removing challenge:', error); | |
| throw new Error('Failed to remove challenge'); | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/backend/src/services/wallet-challenge.service.ts around lines 78 to 84,
the current code logs an error when challenge removal fails but does not stop
execution. To fix this, modify the function to throw an error if the deletion
operation returns an error, ensuring that failure to remove a challenge is
properly handled and prevents potential security issues.
| export const challengeRequestSchema = z.object({ | ||
| publicKey: z.string().min(1, 'Public key is required'), | ||
| }); | ||
|
|
||
| //=================== | ||
| // Wallet login schema | ||
| //=================== | ||
| export const walletLoginSchema = z.object({ | ||
| publicKey: z.string().min(1, 'Public key is required'), | ||
| signature: z.string().min(1, 'Signature is required'), | ||
| challenge: z.string().min(1, 'Challenge is required'), | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add Stellar public key format validation.
Consider adding specific validation for Stellar public keys to catch invalid formats early.
+const stellarPublicKeyRegex = /^G[A-Z2-7]{55}$/;
+
export const challengeRequestSchema = z.object({
- publicKey: z.string().min(1, 'Public key is required'),
+ publicKey: z.string().min(1, 'Public key is required').regex(stellarPublicKeyRegex, 'Invalid Stellar public key format'),
});
//===================
// Wallet login schema
//===================
export const walletLoginSchema = z.object({
- publicKey: z.string().min(1, 'Public key is required'),
+ publicKey: z.string().min(1, 'Public key is required').regex(stellarPublicKeyRegex, 'Invalid Stellar public key format'),
signature: z.string().min(1, 'Signature is required'),
challenge: z.string().min(1, 'Challenge is required'),
});🌐 Web query:
What is the correct regex pattern for validating Stellar public keys?
💡 Result:
The correct regex pattern for validating Stellar public keys corresponds to the key format described by the Stellar network specifications:
- Stellar public keys are encoded in a specific StrKey format.
- They are 56-character strings.
- They start with the uppercase letter "G".
- They consist of Base32 encoding (RFC4648) without padding, which means only characters A-Z (except I, L, O) and digits 2-7 are used.
A common, practical regex pattern to validate a Stellar public key is:
^G[A-Z2-7]{55}$
Explanation:
^asserts the start of the string.Gmatches the mandatory first character for public keys.[A-Z2-7]{55}matches the remaining 55 characters, all uppercase letters A-Z excluding invalid Base32 chars, and digits 2-7.$asserts the end of the string.
This pattern ensures the string is exactly 56 characters long and starts with "G", conforming to the Stellar public key format described by the official docs and supported libraries[2][8].
For more robust validation, including checksum verification and decoding, Stellar SDKs provide methods such as StrKey.isValidEd25519PublicKey(publicKey) in the JavaScript Stellar SDK, which internally handle base32 decoding and CRC16 checksum checks according to the StrKey specification[1][3][5].
In summary:
| Aspect | Detail |
|---|---|
| Length | 56 characters |
| First Character | G (uppercase) |
| Character Set | Base32 (A-Z and digits 2-7) |
| Regex Example | ^G[A-Z2-7]{55}$ |
| Recommended Validation | Use official SDK StrKey validation functions |
This regex is sufficient to filter valid-looking Stellar public keys but should be used alongside checksum verification for full correctness as implemented in SDKs.
Citations:
- 1: https://stellar.github.io/js-stellar-sdk/js-stellar-base_src_keypair.js.html
- 2: https://stellarx.freshdesk.com/support/solutions/articles/151000003747-what-is-a-public-key-where-can-i-find-it-
- 3: https://stellar.github.io/js-stellar-sdk/StrKey.html
- 4: https://itnext.io/how-i-helped-recover-5000xlm-from-ram-dumps-regexp-and-patience-42a8c8fc87cf
- 5: https://github.com/stellar/stellar-protocol/blob/master/ecosystem/sep-0023.md
- 6: https://dev.to/jajera/terraform-validation-rules-best-practices-examples-4abi
- 7: https://developers.stellar.org/docs/build/guides/basics/send-and-receive-payments
- 8: https://resources.stellar.org/hubfs/Asset_Issuance_on_Stellar.pdf
- 9: https://gist.github.com/paranoiq/1932126
- 10: Turning rawSecretKey() back into a Keypair? stellar/js-stellar-sdk#83
Enforce Stellar public key format in validation schemas
To ensure only valid Stellar public keys pass validation, update apps/backend/src/types/wallet-auth.types.ts as follows:
• Define the Stellar regex at the top of the file:
const stellarPublicKeyRegex = /^G[A-Z2-7]{55}$/;• In challengeRequestSchema, change:
- publicKey: z.string().min(1, 'Public key is required'),
+ publicKey: z.string()
+ .min(1, 'Public key is required')
+ .regex(stellarPublicKeyRegex, 'Invalid Stellar public key format'),• In walletLoginSchema, change:
- publicKey: z.string().min(1, 'Public key is required'),
+ publicKey: z.string()
+ .min(1, 'Public key is required')
+ .regex(stellarPublicKeyRegex, 'Invalid Stellar public key format'),For full checksum validation, consider using the Stellar SDK’s StrKey.isValidEd25519PublicKey() alongside this regex.
🤖 Prompt for AI Agents
In apps/backend/src/types/wallet-auth.types.ts between lines 6 and 17, the
current validation schemas only check for non-empty strings for public keys but
do not enforce the Stellar public key format. Define the Stellar public key
regex /^G[A-Z2-7]{55}$/ at the top of the file, then update the publicKey field
in both challengeRequestSchema and walletLoginSchema to use
z.string().regex(stellarPublicKeyRegex, 'Invalid Stellar public key format') to
enforce this pattern. Optionally, for stronger validation, integrate Stellar
SDK's StrKey.isValidEd25519PublicKey() in a custom validator.
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
apps/backend/src/controllers/wallet-auth.controller.ts (1)
49-52: Security: Don't expose JWT token in response body when using HTTP-only cookies.Returning the token in the response body defeats the security benefits of HTTP-only cookies, as it makes the token accessible to JavaScript and vulnerable to XSS attacks.
apps/backend/src/types/wallet-auth.types.ts (1)
4-13: Add Stellar public key format validation.Consider adding specific validation for Stellar public keys to catch invalid formats early.
🧹 Nitpick comments (6)
apps/web/src/components/auth/RegisterForm.tsx (1)
47-50: Complete the translation to English for password strength indicator.The password strength labels are still in Spanish while the rest of the UI has been translated to English.
- {strength <= 2 && 'Débil'} - {strength > 2 && strength <= 4 && 'Media'} - {strength > 4 && 'Fuerte'} + {strength <= 2 && 'Weak'} + {strength > 2 && strength <= 4 && 'Medium'} + {strength > 4 && 'Strong'}apps/backend/src/services/wallet-auth.service.ts (1)
46-48: Remove empty code blocks or add appropriate logging.Empty code blocks reduce code clarity. Either add appropriate logging or remove these blocks.
For signature validation (lines 46-48):
- if (!isValid) { - } else { - }For profile error handling (line 95):
if (profileError) { + console.error('Failed to create user profile:', profileError); }Also applies to: 95-95
apps/web/src/components/auth/WalletAuthButton.tsx (1)
71-75: Remove redundant conditional branch.Both branches of the conditional set the same state, making the check unnecessary.
} else if (isConnected && publicKey && !isLoading) { - if (hasUsedWalletAuth(publicKey)) { - setAuthState('connected'); - } else { - setAuthState('connected'); - } + setAuthState('connected'); }apps/web/src/services/api.ts (1)
131-154: Consider the implications of empty email for wallet users.The function sets
email: ''for wallet users. This might cause issues if other parts of the application expect a valid email address. Consider either:
- Using a placeholder email format like
${publicKey.slice(0, 8)}@wallet.stellar- Making email truly optional throughout the application
- Adding validation to handle empty emails in components that display user data
apps/web/src/hooks/useWallet.ts (1)
158-185: Consider optimizing the permission check in refreshConnection.The
isAllowedstate is set based on address existence, but you're not actually checking permissions withcheckFreighterPermissionlike ininitializeWallet.For consistency with initialization:
const refreshConnection = async () => { setWalletState((prev) => ({ ...prev, isLoading: true })); try { - const [connectionStatus, addressResult, networkResult] = await Promise.all([ + const [connectionStatus, permissionStatus, addressResult, networkResult] = await Promise.all([ checkFreighterConnection(), + checkFreighterPermission(), getFreighterAddress(), getFreighterNetwork(), ]); setWalletState({ isConnected: connectionStatus.isConnected && !!addressResult.address, publicKey: addressResult.address || null, network: networkResult.network || null, networkPassphrase: networkResult.networkPassphrase || null, isInstalled: connectionStatus.isInstalled, - isAllowed: !!addressResult.address, + isAllowed: permissionStatus.isAllowed, isLoading: false, error: null, });apps/web/src/lib/freighter-utils.ts (1)
233-246: Consider simplifying the signed message handling.The current logic is complex and the Buffer check might not work reliably in all environments.
Simplify the message handling:
let signedMessage: string | undefined; - if (typeof signResult.signedMessage === 'string') { - signedMessage = signResult.signedMessage; - } else if ( - signResult.signedMessage && - typeof Buffer !== 'undefined' && - typeof signResult.signedMessage.toString === 'function' - ) { - signedMessage = signResult.signedMessage.toString('utf8'); - } else { - signedMessage = undefined; - } + // Handle both string and Buffer-like responses + if (signResult.signedMessage) { + if (typeof signResult.signedMessage === 'string') { + signedMessage = signResult.signedMessage; + } else if (signResult.signedMessage instanceof Uint8Array) { + signedMessage = new TextDecoder().decode(signResult.signedMessage); + } else if (typeof signResult.signedMessage.toString === 'function') { + signedMessage = signResult.signedMessage.toString(); + } + }This approach uses standard web APIs (TextDecoder) instead of Node.js-specific Buffer and handles Uint8Array which is more common in browser environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
apps/backend/database/migrations/00004_create_wallet_auth_tables.sql(1 hunks)apps/backend/src/controllers/wallet-auth.controller.ts(1 hunks)apps/backend/src/routes/wallet-auth.routes.ts(1 hunks)apps/backend/src/services/wallet-auth.service.ts(1 hunks)apps/backend/src/services/wallet-challenge.service.ts(1 hunks)apps/backend/src/types/wallet-auth.types.ts(1 hunks)apps/backend/src/validators/wallet-auth.validator.ts(1 hunks)apps/web/package.json(1 hunks)apps/web/src/components/auth/RegisterForm.tsx(8 hunks)apps/web/src/components/auth/WalletAuthButton.tsx(1 hunks)apps/web/src/hooks/auth/use-auth.tsx(3 hunks)apps/web/src/hooks/useWallet.ts(1 hunks)apps/web/src/lib/freighter-utils.ts(1 hunks)apps/web/src/services/api.ts(9 hunks)apps/web/src/types/index.ts(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/web/package.json
- apps/web/src/types/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/backend/src/routes/wallet-auth.routes.ts
- apps/backend/src/validators/wallet-auth.validator.ts
- apps/backend/src/services/wallet-challenge.service.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
apps/web/src/components/auth/RegisterForm.tsx (1)
apps/web/src/components/auth/WalletAuthButton.tsx (1)
WalletAuthButton(25-258)
apps/web/src/services/api.ts (2)
apps/backend/src/types/wallet-auth.types.ts (2)
ChallengeResponse(45-48)WalletAuthResponse(27-43)apps/web/src/types/index.ts (1)
UserProfile(39-55)
apps/web/src/hooks/useWallet.ts (1)
apps/web/src/lib/freighter-utils.ts (5)
checkFreighterConnection(41-68)checkFreighterPermission(73-96)getFreighterAddress(127-148)getFreighterNetwork(153-176)connectFreighter(101-122)
🔇 Additional comments (9)
apps/web/src/components/auth/RegisterForm.tsx (1)
75-78: Consider the navigation flow after wallet authentication in registration context.The component navigates to
/dashboardafter successful wallet authentication, but this is a registration form. Consider whether new users should go through an onboarding flow or profile completion step first.apps/web/src/components/auth/WalletAuthButton.tsx (1)
41-61: Well-implemented auto-authentication flow.The auto-authentication logic is well-structured with proper error handling, state management, and user feedback. Good use of localStorage to track previous wallet authentication.
apps/web/src/services/api.ts (2)
49-71: LGTM! Well-structured interfaces for wallet authentication.The interfaces correctly mirror the backend response types and provide clear structure for wallet authentication responses.
435-472: Well-implemented wallet authentication methods.The new authentication methods follow the established patterns and properly handle errors. The integration with the wallet authentication flow is clean and consistent.
apps/web/src/hooks/useWallet.ts (2)
36-108: Excellent implementation of wallet initialization with proper cleanup.The use of a mounted flag to prevent state updates after unmount and comprehensive error handling makes this implementation robust. The state management is thorough and well-structured.
110-147: Well-implemented connection logic with proper validation.The connect function properly validates the returned address and maintains loading states throughout the operation. Error handling is comprehensive.
apps/web/src/hooks/auth/use-auth.tsx (2)
42-64: Good implementation of auth state persistence with error handling.The error handling for localStorage parsing prevents the app from crashing on corrupted data, and the cleanup ensures a fresh start on error.
125-183: Well-implemented transaction building and signing flow.The transaction construction is correct for authentication purposes, with proper error handling throughout the signing process. Using sequence number '0' is appropriate for this non-submittable auth transaction.
apps/web/src/lib/freighter-utils.ts (1)
41-176: Excellent error handling and API abstraction.The consistent error handling pattern across all functions provides a clean abstraction over the Freighter API. The separation of connection checking, permission verification, and wallet operations is well-designed.
| if (!walletLoginData.publicKey) { | ||
| return res.status(400).json({ error: 'Missing publicKey' }); | ||
| } | ||
| if (!walletLoginData.signedTransaction) { | ||
| return res.status(400).json({ error: 'Missing signedTransaction' }); | ||
| } | ||
| if (!walletLoginData.challenge) { | ||
| return res.status(400).json({ error: 'Missing challenge' }); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Remove redundant manual validation checks.
These manual validation checks should be handled by the validation middleware. If the middleware is properly configured, these checks are redundant.
🏁 Script executed:
#!/bin/bash
# Description: Verify validation middleware is applied to the wallet auth routes
# Search for wallet auth routes configuration
rg -A 10 "authenticateWallet|/wallet" apps/backend/src/routes/
# Check if validation middleware is imported and used
ast-grep --pattern 'walletLoginSchema'Length of output: 2068
Remove redundant manual validation in controller
The validateWalletLogin middleware already enforces presence and type checks for publicKey, signedTransaction, and challenge. You can safely delete the manual if (!…){ return res.status(400)… } blocks in the controller.
- File to update:
apps/backend/src/controllers/wallet-auth.controller.ts - Remove lines 28–36 (the three manual
ifchecks forpublicKey,signedTransaction, andchallenge)
Suggested diff:
--- a/apps/backend/src/controllers/wallet-auth.controller.ts
+++ b/apps/backend/src/controllers/wallet-auth.controller.ts
@@ -28,12 +28,6 @@ export const authenticateWalletController = async (req, res) => {
const walletLoginData = req.body;
-
- if (!walletLoginData.publicKey) {
- return res.status(400).json({ error: 'Missing publicKey' });
- }
- if (!walletLoginData.signedTransaction) {
- return res.status(400).json({ error: 'Missing signedTransaction' });
- }
- if (!walletLoginData.challenge) {
- return res.status(400).json({ error: 'Missing challenge' });
- }
// …rest of authentication logic…📝 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 (!walletLoginData.publicKey) { | |
| return res.status(400).json({ error: 'Missing publicKey' }); | |
| } | |
| if (!walletLoginData.signedTransaction) { | |
| return res.status(400).json({ error: 'Missing signedTransaction' }); | |
| } | |
| if (!walletLoginData.challenge) { | |
| return res.status(400).json({ error: 'Missing challenge' }); | |
| } | |
| export const authenticateWalletController = async (req, res) => { | |
| const walletLoginData = req.body; | |
| // …rest of authentication logic… | |
| } |
🤖 Prompt for AI Agents
In apps/backend/src/controllers/wallet-auth.controller.ts between lines 28 and
36, remove the manual validation blocks that check for the presence of
publicKey, signedTransaction, and challenge, as these are already validated by
the validateWalletLogin middleware. Simply delete these if statements and their
corresponding return responses to avoid redundant validation.
| console.log(`🌐 API Call: ${options.method || 'GET'} ${API_BASE_URL}${endpoint}`); | ||
| if (options.body) { | ||
| console.log('📤 Request body:', options.body); | ||
| } | ||
|
|
There was a problem hiding this comment.
Remove request body logging to prevent sensitive data exposure.
Logging request bodies can expose sensitive information like passwords, authentication tokens, or private keys in the console. This is a security risk, especially in production.
Remove the request body logging or make it conditional:
- console.log(`🌐 API Call: ${options.method || 'GET'} ${API_BASE_URL}${endpoint}`);
- if (options.body) {
- console.log('📤 Request body:', options.body);
- }
+ if (process.env.NODE_ENV === 'development') {
+ console.log(`🌐 API Call: ${options.method || 'GET'} ${API_BASE_URL}${endpoint}`);
+ // Never log sensitive endpoints
+ const sensitiveEndpoints = ['/auth/', '/wallet/'];
+ const isSensitive = sensitiveEndpoints.some(e => endpoint.includes(e));
+ if (options.body && !isSensitive) {
+ console.log('📤 Request body:', options.body);
+ }
+ }📝 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.
| console.log(`🌐 API Call: ${options.method || 'GET'} ${API_BASE_URL}${endpoint}`); | |
| if (options.body) { | |
| console.log('📤 Request body:', options.body); | |
| } | |
| if (process.env.NODE_ENV === 'development') { | |
| console.log(`🌐 API Call: ${options.method || 'GET'} ${API_BASE_URL}${endpoint}`); | |
| // Never log sensitive endpoints | |
| const sensitiveEndpoints = ['/auth/', '/wallet/']; | |
| const isSensitive = sensitiveEndpoints.some(e => endpoint.includes(e)); | |
| if (options.body && !isSensitive) { | |
| console.log('📤 Request body:', options.body); | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/web/src/services/api.ts around lines 175 to 179, the code logs the
request body which may expose sensitive data. Remove the console.log statement
that outputs the request body or modify it to only log in non-production
environments to prevent sensitive data exposure in production.
| CREATE OR REPLACE FUNCTION public.cleanup_expired_challenges() | ||
| RETURNS void AS $$ | ||
| BEGIN | ||
| DELETE FROM public.wallet_challenges | ||
| WHERE expires_at < NOW(); | ||
| END; | ||
| $$ LANGUAGE plpgsql; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Schedule automatic cleanup of expired challenges.
The cleanup function is defined but not automatically scheduled. Expired challenges will accumulate without periodic cleanup.
Add a scheduled job using pg_cron or a similar mechanism:
-- After creating the function, schedule it to run every hour
-- This requires pg_cron extension
CREATE EXTENSION IF NOT EXISTS pg_cron;
SELECT cron.schedule(
'cleanup-expired-wallet-challenges',
'0 * * * *', -- Run every hour
'SELECT public.cleanup_expired_challenges();'
);Alternatively, trigger cleanup on each new challenge creation to maintain table hygiene.
🤖 Prompt for AI Agents
In apps/backend/database/migrations/00004_create_wallet_auth_tables.sql around
lines 26 to 33, the cleanup_expired_challenges function is defined but not
scheduled to run automatically. To fix this, add SQL commands after the function
definition to create the pg_cron extension if not present and schedule the
cleanup_expired_challenges function to run every hour using cron.schedule with
the appropriate cron expression. This will ensure expired challenges are
regularly deleted without manual intervention.
| CREATE TABLE IF NOT EXISTS public.wallet_challenges ( | ||
| id UUID DEFAULT gen_random_uuid() PRIMARY KEY, | ||
| public_key TEXT NOT NULL, | ||
| challenge TEXT NOT NULL, | ||
| expires_at TIMESTAMP WITH TIME ZONE NOT NULL, | ||
| created_at TIMESTAMP WITH TIME ZONE DEFAULT NOW() | ||
| ); | ||
|
|
||
| -- Create indexes for efficient lookups | ||
| CREATE INDEX IF NOT EXISTS idx_wallet_challenges_public_key ON public.wallet_challenges(public_key); | ||
| CREATE INDEX IF NOT EXISTS idx_wallet_challenges_expires_at ON public.wallet_challenges(expires_at); | ||
| CREATE INDEX IF NOT EXISTS idx_wallet_challenges_challenge ON public.wallet_challenges(challenge); | ||
|
|
There was a problem hiding this comment.
Add unique constraint to challenge field for security.
The challenge field should have a unique constraint to prevent duplicate challenges from being generated, which could be a security vulnerability.
Add a unique constraint:
CREATE TABLE IF NOT EXISTS public.wallet_challenges (
id UUID DEFAULT gen_random_uuid() PRIMARY KEY,
public_key TEXT NOT NULL,
- challenge TEXT NOT NULL,
+ challenge TEXT NOT NULL UNIQUE,
expires_at TIMESTAMP WITH TIME ZONE NOT NULL,
created_at TIMESTAMP WITH TIME ZONE DEFAULT NOW()
);📝 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.
| CREATE TABLE IF NOT EXISTS public.wallet_challenges ( | |
| id UUID DEFAULT gen_random_uuid() PRIMARY KEY, | |
| public_key TEXT NOT NULL, | |
| challenge TEXT NOT NULL, | |
| expires_at TIMESTAMP WITH TIME ZONE NOT NULL, | |
| created_at TIMESTAMP WITH TIME ZONE DEFAULT NOW() | |
| ); | |
| -- Create indexes for efficient lookups | |
| CREATE INDEX IF NOT EXISTS idx_wallet_challenges_public_key ON public.wallet_challenges(public_key); | |
| CREATE INDEX IF NOT EXISTS idx_wallet_challenges_expires_at ON public.wallet_challenges(expires_at); | |
| CREATE INDEX IF NOT EXISTS idx_wallet_challenges_challenge ON public.wallet_challenges(challenge); | |
| CREATE TABLE IF NOT EXISTS public.wallet_challenges ( | |
| id UUID DEFAULT gen_random_uuid() PRIMARY KEY, | |
| public_key TEXT NOT NULL, | |
| challenge TEXT NOT NULL UNIQUE, | |
| expires_at TIMESTAMP WITH TIME ZONE NOT NULL, | |
| created_at TIMESTAMP WITH TIME ZONE DEFAULT NOW() | |
| ); | |
| -- Create indexes for efficient lookups | |
| CREATE INDEX IF NOT EXISTS idx_wallet_challenges_public_key ON public.wallet_challenges(public_key); | |
| CREATE INDEX IF NOT EXISTS idx_wallet_challenges_expires_at ON public.wallet_challenges(expires_at); | |
| CREATE INDEX IF NOT EXISTS idx_wallet_challenges_challenge ON public.wallet_challenges(challenge); |
🤖 Prompt for AI Agents
In apps/backend/database/migrations/00004_create_wallet_auth_tables.sql lines 1
to 13, the challenge column in the wallet_challenges table lacks a unique
constraint, which can lead to duplicate challenges and security issues. Add a
unique constraint on the challenge column by modifying the table definition to
include UNIQUE(challenge) or by adding a separate ALTER TABLE statement to
enforce uniqueness on the challenge field.
| -- Record this migration as completed | ||
| INSERT INTO migrations (name) VALUES ('001_create_wallet_auth_tables') ON CONFLICT (name) DO NOTHING; | ||
|
|
There was a problem hiding this comment.
Fix migration name to match file name.
The migration name in the INSERT statement doesn't match the actual file name, which could cause migration tracking issues.
--- Record this migration as completed
-INSERT INTO migrations (name) VALUES ('001_create_wallet_auth_tables') ON CONFLICT (name) DO NOTHING;
+INSERT INTO migrations (name) VALUES ('00004_create_wallet_auth_tables') ON CONFLICT (name) DO NOTHING;🤖 Prompt for AI Agents
In apps/backend/database/migrations/00004_create_wallet_auth_tables.sql around
lines 50 to 52, the migration name in the INSERT statement is
'001_create_wallet_auth_tables' but it should match the file name
'00004_create_wallet_auth_tables'. Update the migration name in the INSERT INTO
migrations statement to '00004_create_wallet_auth_tables' to ensure proper
migration tracking.
apps/web/src/hooks/auth/use-auth.tsx
Outdated
| const challengeText = challengeResponse.challenge; | ||
| if (challengeText.length > 28) { | ||
| throw new Error('Challenge too long for transaction memo'); | ||
| } | ||
| const account = new Account(walletPublicKey, '0'); |
There was a problem hiding this comment.
Fix challenge length validation to check UTF-8 byte length.
Stellar text memos support up to 28 bytes in UTF-8, not 28 characters. Multi-byte characters could cause issues.
const challengeText = challengeResponse.challenge;
- if (challengeText.length > 28) {
+ const challengeBytes = new TextEncoder().encode(challengeText);
+ if (challengeBytes.length > 28) {
throw new Error('Challenge too long for transaction memo');
}📝 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.
| const challengeText = challengeResponse.challenge; | |
| if (challengeText.length > 28) { | |
| throw new Error('Challenge too long for transaction memo'); | |
| } | |
| const account = new Account(walletPublicKey, '0'); | |
| const challengeText = challengeResponse.challenge; | |
| const challengeBytes = new TextEncoder().encode(challengeText); | |
| if (challengeBytes.length > 28) { | |
| throw new Error('Challenge too long for transaction memo'); | |
| } | |
| const account = new Account(walletPublicKey, '0'); |
🤖 Prompt for AI Agents
In apps/web/src/hooks/auth/use-auth.tsx around lines 120 to 124, the current
validation checks the challenge length by character count, but Stellar memos
limit to 28 bytes in UTF-8 encoding. Update the validation to calculate the
UTF-8 byte length of challengeResponse.challenge and throw the error if it
exceeds 28 bytes, ensuring multi-byte characters are correctly accounted for.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
apps/web/src/hooks/auth/procted-route.tsx (1)
1-58: Fix the filename typo to match the expected import path.The filename
procted-route.tsxshould beprotected-route.tsx(missing 'e') to match the import in the dashboard layout and follow proper naming conventions.
♻️ Duplicate comments (2)
apps/backend/src/services/wallet-auth.service.ts (2)
16-16: Critical: Fix inverted network selection logic.The network selection is inverted - production should use PUBLIC network, not TESTNET.
56-59: Add error logging in signature verification.Swallowing errors makes debugging difficult. Log the error before returning false.
🧹 Nitpick comments (5)
apps/web/src/hooks/auth/procted-route.tsx (2)
18-18: Optimize the default prop to prevent unnecessary re-renders.The
allowedAuthTypesdefault value creates a new array on each render, potentially causing unnecessary re-executions of the useEffect.+const DEFAULT_ALLOWED_AUTH_TYPES: ('email' | 'wallet')[] = ['email', 'wallet']; + export default function ProtectedRoute({ children, redirectTo = '/login', - allowedAuthTypes = ['email', 'wallet'], + allowedAuthTypes = DEFAULT_ALLOWED_AUTH_TYPES, }: ProtectedRouteProps) {
34-34: Remove or replace the debug console.log statement.Console logging should be avoided in production code. Consider using a proper logging service or removing this debug statement.
- console.log('🚫 Auth type not allowed:', authType, 'Allowed:', allowedAuthTypes);apps/backend/src/services/wallet-auth.service.ts (2)
43-44: Optimize import placement for better performance.The dynamic import of
Keypairinside the function adds unnecessary overhead. Move this to the top-level imports.-import { Networks, StrKey, Transaction } from '@stellar/stellar-sdk'; +import { Networks, StrKey, Transaction, Keypair } from '@stellar/stellar-sdk';And update the function:
- const { Keypair } = await import('@stellar/stellar-sdk'); - const keypair = Keypair.fromPublicKey(publicKey); + const keypair = Keypair.fromPublicKey(publicKey);
151-200: Consider adding request timeout and improved error messages.The main authentication function lacks timeout handling and could benefit from more specific error messages for different failure scenarios.
Consider adding:
- Timeout handling for signature verification
- More specific error messages (e.g., "Challenge expired" vs "Invalid challenge")
- Structured error responses for better frontend handling
apps/backend/database/migrations/00005_create_profile_table.sql (1)
26-27: Consider more granular RLS policies.The current policy grants full access to service_role, but more granular policies might be beneficial for security.
Consider adding policies for different access patterns:
-- Allow users to read their own profiles CREATE POLICY "Users can read own profile" ON public.profiles FOR SELECT USING (auth.uid()::text = user_id::text); -- Allow users to update their own profiles CREATE POLICY "Users can update own profile" ON public.profiles FOR UPDATE USING (auth.uid()::text = user_id::text);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/backend/database/migrations/00005_create_profile_table.sql(1 hunks)apps/backend/src/services/wallet-auth.service.ts(1 hunks)apps/web/src/app/dashboard/layout.tsx(1 hunks)apps/web/src/app/dashboard/page.tsx(1 hunks)apps/web/src/hooks/auth/procted-route.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/src/app/dashboard/page.tsx
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/web/src/app/dashboard/layout.tsx (1)
apps/web/src/hooks/auth/procted-route.tsx (1)
ProtectedRoute(15-57)
apps/web/src/hooks/auth/procted-route.tsx (2)
apps/web/src/hooks/auth/use-auth.tsx (1)
useAuth(220-220)apps/web/src/services/authService.ts (1)
isAuthenticated(89-108)
🔇 Additional comments (7)
apps/web/src/app/dashboard/layout.tsx (1)
8-14: Clean and appropriate layout implementation.The dashboard layout properly wraps children in the ProtectedRoute component, providing the necessary authentication protection for dashboard pages.
apps/web/src/hooks/auth/procted-route.tsx (3)
24-39: Well-implemented authentication logic with proper flow control.The authentication checking logic properly handles the loading states, authentication status, and auth type validation with appropriate redirects.
41-50: Clean loading state implementation.The loading spinner with proper styling and messaging provides good user feedback during authentication checks.
52-57: Proper guard clause implementation for unauthorized access.The final authentication check correctly returns null for unauthorized users and renders children for authorized ones.
apps/backend/src/services/wallet-auth.service.ts (1)
98-109: Handle profile creation failure more gracefully.Profile creation failure only logs a warning but doesn't affect the authentication flow. Consider if this should be a critical error or if the warning approach is intentional.
Should profile creation be mandatory for wallet authentication? The current implementation allows users to authenticate even if profile creation fails, which might lead to inconsistent user experience.
apps/backend/database/migrations/00005_create_profile_table.sql (2)
19-20: Add unique constraint on user_id.The current schema allows multiple profiles per user, which may not be intended. Consider adding a unique constraint.
Should there be a unique constraint on
user_idto ensure one profile per user? The current schema allows multiple profiles per user.-- If one profile per user is intended: CREATE UNIQUE INDEX IF NOT EXISTS idx_profiles_user_id_unique ON public.profiles(user_id);
34-46: LGTM: Well-implemented automatic timestamp update.The trigger function and trigger setup for automatic
updated_attimestamp updates is correctly implemented and follows PostgreSQL best practices.
| @@ -0,0 +1,16 @@ | |||
| import type React from 'react'; | |||
| import ProtectedRoute from '~/hooks/auth/procted-route'; | |||
There was a problem hiding this comment.
Fix the typo in the import path.
The import path has a typo: procted-route should be protected-route (missing 'e'). This will cause a module not found error.
-import ProtectedRoute from '~/hooks/auth/procted-route';
+import ProtectedRoute from '~/hooks/auth/protected-route';📝 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.
| import ProtectedRoute from '~/hooks/auth/procted-route'; | |
| import ProtectedRoute from '~/hooks/auth/protected-route'; |
🤖 Prompt for AI Agents
In apps/web/src/app/dashboard/layout.tsx at line 2, fix the typo in the import
path by changing 'procted-route' to 'protected-route' to correctly reference the
module and avoid the module not found error.
| const { data: profile } = await supabase | ||
| .from('profiles') | ||
| .select('*') | ||
| .eq('user_id', userId) | ||
| .single(); | ||
|
|
||
| return profile || undefined; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for database operations.
The getUserProfile function doesn't handle potential database errors, which could cause the authentication flow to fail unexpectedly.
async function getUserProfile(userId: string) {
- const { data: profile } = await supabase
+ const { data: profile, error } = await supabase
.from('profiles')
.select('*')
.eq('user_id', userId)
.single();
+ if (error && error.code !== 'PGRST116') { // PGRST116 is "not found"
+ console.error('Error fetching user profile:', error);
+ throw new Error('Failed to fetch user profile');
+ }
+
return profile || undefined;
}📝 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.
| const { data: profile } = await supabase | |
| .from('profiles') | |
| .select('*') | |
| .eq('user_id', userId) | |
| .single(); | |
| return profile || undefined; | |
| async function getUserProfile(userId: string) { | |
| const { data: profile, error } = await supabase | |
| .from('profiles') | |
| .select('*') | |
| .eq('user_id', userId) | |
| .single(); | |
| if (error && error.code !== 'PGRST116') { // PGRST116 is "not found" | |
| console.error('Error fetching user profile:', error); | |
| throw new Error('Failed to fetch user profile'); | |
| } | |
| return profile || undefined; | |
| } |
🤖 Prompt for AI Agents
In apps/backend/src/services/wallet-auth.service.ts around lines 118 to 124, the
database query to fetch the user profile lacks error handling, which may cause
unhandled exceptions. Modify the code to check for errors returned by the
Supabase query and handle them appropriately, such as logging the error or
throwing a controlled exception, before returning the profile data or undefined.
| CREATE TABLE IF NOT EXISTS public.profiles ( | ||
| id UUID DEFAULT gen_random_uuid() PRIMARY KEY, | ||
| user_id UUID NOT NULL, | ||
| name TEXT, | ||
| avatar_url TEXT, | ||
| phone TEXT, | ||
| address TEXT, | ||
| preferences JSONB DEFAULT '{}', | ||
| social_links JSONB DEFAULT '{}', | ||
| verification_status TEXT DEFAULT 'unverified' CHECK (verification_status IN ('verified', 'unverified', 'pending')), | ||
| last_active TIMESTAMP WITH TIME ZONE DEFAULT NOW(), | ||
| created_at TIMESTAMP WITH TIME ZONE DEFAULT NOW(), | ||
| updated_at TIMESTAMP WITH TIME ZONE DEFAULT NOW() | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding foreign key constraint for user_id.
The user_id column lacks a foreign key constraint, which could lead to orphaned profiles if the referenced user is deleted.
CREATE TABLE IF NOT EXISTS public.profiles (
id UUID DEFAULT gen_random_uuid() PRIMARY KEY,
- user_id UUID NOT NULL,
+ user_id UUID NOT NULL REFERENCES wallet_users(id) ON DELETE CASCADE,
name TEXT,
avatar_url TEXT,
phone TEXT,
address TEXT,
preferences JSONB DEFAULT '{}',
social_links JSONB DEFAULT '{}',
verification_status TEXT DEFAULT 'unverified' CHECK (verification_status IN ('verified', 'unverified', 'pending')),
last_active TIMESTAMP WITH TIME ZONE DEFAULT NOW(),
created_at TIMESTAMP WITH TIME ZONE DEFAULT NOW(),
updated_at TIMESTAMP WITH TIME ZONE DEFAULT NOW()
);📝 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.
| CREATE TABLE IF NOT EXISTS public.profiles ( | |
| id UUID DEFAULT gen_random_uuid() PRIMARY KEY, | |
| user_id UUID NOT NULL, | |
| name TEXT, | |
| avatar_url TEXT, | |
| phone TEXT, | |
| address TEXT, | |
| preferences JSONB DEFAULT '{}', | |
| social_links JSONB DEFAULT '{}', | |
| verification_status TEXT DEFAULT 'unverified' CHECK (verification_status IN ('verified', 'unverified', 'pending')), | |
| last_active TIMESTAMP WITH TIME ZONE DEFAULT NOW(), | |
| created_at TIMESTAMP WITH TIME ZONE DEFAULT NOW(), | |
| updated_at TIMESTAMP WITH TIME ZONE DEFAULT NOW() | |
| ); | |
| CREATE TABLE IF NOT EXISTS public.profiles ( | |
| id UUID DEFAULT gen_random_uuid() PRIMARY KEY, | |
| user_id UUID NOT NULL REFERENCES wallet_users(id) ON DELETE CASCADE, | |
| name TEXT, | |
| avatar_url TEXT, | |
| phone TEXT, | |
| address TEXT, | |
| preferences JSONB DEFAULT '{}', | |
| social_links JSONB DEFAULT '{}', | |
| verification_status TEXT DEFAULT 'unverified' CHECK (verification_status IN ('verified', 'unverified', 'pending')), | |
| last_active TIMESTAMP WITH TIME ZONE DEFAULT NOW(), | |
| created_at TIMESTAMP WITH TIME ZONE DEFAULT NOW(), | |
| updated_at TIMESTAMP WITH TIME ZONE DEFAULT NOW() | |
| ); |
🤖 Prompt for AI Agents
In apps/backend/database/migrations/00005_create_profile_table.sql around lines
3 to 16, the user_id column is missing a foreign key constraint. Add a foreign
key constraint referencing the users table (or the appropriate users table) on
user_id to ensure referential integrity and prevent orphaned profiles when a
user is deleted.
respp
left a comment
respp
left a comment
There was a problem hiding this comment.
The issue requires tests. Once you’ve written them, please place them where they belong inside this folder, in apps/backend/tests
There was a problem hiding this comment.
@Benjtalkshow there are a few things that need improvement. Initially, the backend endpoint and its validation are fine, but there's no 5-minute expiration for the challenge, no timestamp to prevent reuse, and no integration with Redis/Supabase for temporary storage.
respp
left a comment
respp
left a comment
There was a problem hiding this comment.
I've left most of the feedback directly in the files. Please remember to run the linter and pull the latest changes from the repository before working on the required updates.
There was a problem hiding this comment.
On the frontend side, new functionality was added that already existed, resulting in duplicated logic. The existing useWallet hook is not used, there is no integration with window.freighterApi.signTransaction, and the signing logic is redundantly reimplemented. Session management is completely missing: there's no secure cookie configuration, no integration with useAuth, no visual feedback like toasts, and no handling of authentication state. Error handling is minimal, cases like "WalletNotFoundError", connection rejections, and expired challenges are not covered, and there are no error messages displayed in the UI. Additionally, no testing has been implemented here either. I suggest revisiting the issue to better address these problems: #82
There was a problem hiding this comment.
On the frontend side, new functionality was added that already existed, resulting in duplicated logic. The existing useWallet hook is not used, there is no integration with window.freighterApi.signTransaction, and the signing logic is redundantly reimplemented. Session management is completely missing: there's no secure cookie configuration, no integration with useAuth, no visual feedback like toasts, and no handling of authentication state. Error handling is minimal, cases like "WalletNotFoundError", connection rejections, and expired challenges are not covered, and there are no error messages displayed in the UI. Additionally, no testing has been implemented here either. I suggest revisiting the issue to better address these problems: #82
@respp
I used the useWallet hook. I chose to use @stellr/frighter-api because window.frighterApi.signTransaction often fails to detect the wallet, I mentioned this in my PR. I also used useAuth and added enough toast messages to show errors.
I’ll fix the issue where expired challenges are not being stored properly. I’ll also write the necessary tests and take care of the other issues you pointed out. Thanks
@respp |
|
@respp I even created protected routes for the dashboard since the dashboard wasn’t protected before. The challenge generation and message signing are working perfectly, and I also added appropriate toast messages for every user action. The only issues I noticed are:
Please kindly go through my pull request message, watch the video evidence I attached, or even test it locally. Everything I’ve explained here was already included in my PR description. Thanks. I’ll write the test and fix the challenge generation issue. |
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
apps/backend/package.json (1)
15-15: Critical: Remove duplicate stellar-sdk dependency.You have two stellar-sdk entries in your dependencies:
@stellar/stellar-sdkat line 15stellar-sdkat line 27This duplication can cause version conflicts and installation issues.
"@stellar/stellar-sdk": "^13.3.0", "@supabase/supabase-js": "^2.39.0", "bcrypt": "^5.1.1", "cors": "^2.8.5", "dotenv": "^16.3.1", "express": "^4.18.2", "express-rate-limit": "^7.1.5", "jest-mock-extended": "^4.0.0", "joi": "^17.13.3", "jsonwebtoken": "^9.0.2", "node-cron": "^4.2.0", "soroban-client": "^1.0.1", -"stellar-sdk": "^13.3.0", "supertest": "^7.1.2", "uuid": "^11.1.0", "zod": "^3.22.4"Also applies to: 27-27
♻️ Duplicate comments (4)
apps/backend/package.json (1)
23-23: Question the need for Joi alongside Zod.You're adding
joivalidation library while already usingzod(line 30). This introduces inconsistency in validation approaches across the codebase.Consider removing Joi and using Zod consistently throughout the application:
- "joi": "^17.13.3",If Joi is truly needed for specific use cases, please document why both validation libraries are necessary.
apps/backend/src/services/wallet-challenge.service.ts (1)
75-81: Consider throwing an error when challenge removal fails.Failed challenge removal could allow challenge reuse, which is a security concern.
export async function removeChallenge(challengeId: string): Promise<void> { const { error } = await supabase.from('wallet_challenges').delete().eq('id', challengeId); if (error) { console.error('Error removing challenge:', error); + throw new Error('Failed to remove challenge'); } }apps/backend/src/services/wallet-auth.service.ts (1)
131-137: Add error handling for database operations.The
getUserProfilefunction doesn't handle potential database errors, which could cause the authentication flow to fail unexpectedly.async function getUserProfile(userId: string) { - const { data: profile } = await supabase + const { data: profile, error } = await supabase .from('profiles') .select('*') .eq('user_id', userId) .single(); + if (error && error.code !== 'PGRST116') { // PGRST116 is "not found" + console.error('Error fetching user profile:', error); + throw new Error('Failed to fetch user profile'); + } + return profile || undefined; }apps/web/src/services/api.ts (1)
173-177: Remove request body logging to prevent sensitive data exposure.This security issue was already identified in a previous review. Request body logging can expose sensitive information like passwords, authentication tokens, or private keys.
🧹 Nitpick comments (1)
apps/web/src/components/NetworkStatus.tsx (1)
6-68: Well-structured component with good UX patterns.The component implements proper conditional rendering, comprehensive network mapping, and semantic color coding. The early returns for loading and disconnected states provide good UX.
Consider memoizing the network info calculation for better performance:
+import { useMemo } from 'react'; import { AlertTriangle, CheckCircle, Globe } from 'lucide-react'; import { useWallet } from '../hooks/useWallet'; export default function NetworkStatus() { const { network, isConnected, isLoading } = useWallet(); if (isLoading || !isConnected) { return null; } - const getNetworkInfo = () => { + const networkInfo = useMemo(() => { switch (network?.toUpperCase()) { // ... existing cases } - }; + }, [network]); - const networkInfo = getNetworkInfo(); const IconComponent = networkInfo.icon;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
apps/backend/package.json(1 hunks)apps/backend/src/controllers/wallet-auth.controller.ts(1 hunks)apps/backend/src/index.ts(3 hunks)apps/backend/src/services/cleanup-schedular.ts(1 hunks)apps/backend/src/services/wallet-auth.service.ts(1 hunks)apps/backend/src/services/wallet-challenge.service.ts(1 hunks)apps/backend/src/types/wallet-error-types.ts(1 hunks)apps/backend/tests/integration/wallet-auth.test.ts(1 hunks)apps/web/src/components/NetworkStatus.tsx(1 hunks)apps/web/src/components/auth/WalletAuthButton.tsx(1 hunks)apps/web/src/hooks/auth/use-auth.tsx(3 hunks)apps/web/src/hooks/useWallet.ts(1 hunks)apps/web/src/lib/network-utils.ts(1 hunks)apps/web/src/services/api.ts(13 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/backend/src/types/wallet-error-types.ts
- apps/web/src/lib/network-utils.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/backend/src/index.ts
- apps/backend/src/controllers/wallet-auth.controller.ts
- apps/web/src/hooks/auth/use-auth.tsx
- apps/web/src/hooks/useWallet.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
apps/backend/src/services/cleanup-schedular.ts (1)
apps/backend/src/services/wallet-challenge.service.ts (1)
cleanupAllExpiredChallenges(109-139)
apps/web/src/components/NetworkStatus.tsx (1)
apps/web/src/hooks/useWallet.ts (1)
useWallet(23-211)
apps/backend/tests/integration/wallet-auth.test.ts (6)
apps/backend/src/config/supabase.ts (1)
supabase(205-217)apps/backend/src/controllers/wallet-auth.controller.ts (1)
generateChallenge(16-27)apps/backend/src/services/wallet-challenge.service.ts (4)
generateChallenge(8-48)getValidChallenge(53-70)removeChallenge(75-81)cleanupAllExpiredChallenges(109-139)apps/backend/src/services/wallet-auth.service.ts (1)
authenticateWallet(164-211)apps/backend/src/types/wallet-error-types.ts (5)
InvalidPublicKeyError(29-34)InvalidChallengeError(15-20)SignatureVerificationError(22-27)WalletNotFoundError(1-6)ConnectionRejectedError(8-13)apps/backend/src/index.ts (1)
app(24-24)
🔇 Additional comments (6)
apps/backend/package.json (1)
25-25: Dependencies serve clear purposes for wallet authentication.The added dependencies are well-justified:
node-cronfor scheduled cleanup taskssupertestfor API testinguuidfor unique identifier generationThese align with the wallet authentication features being implemented.
Also applies to: 28-29
apps/backend/src/services/wallet-challenge.service.ts (1)
1-48: Solid implementation with good security practices.The service properly handles challenge lifecycle with appropriate expiry times, validation, and cleanup. The reuse of valid challenges is a good optimization, and the logging provides good operational visibility.
Also applies to: 53-70, 109-139
apps/backend/src/services/wallet-auth.service.ts (1)
15-74: Comprehensive wallet authentication implementation.The service implements a robust authentication flow with proper transaction verification, user management, and JWT generation. The signature verification logic is solid, and the error handling and logging throughout most of the service is excellent.
Also applies to: 78-125, 143-211
apps/web/src/components/auth/WalletAuthButton.tsx (1)
54-74: Well-structured error handling!The error message mapping provides clear, user-friendly messages for different error scenarios. Good defensive programming with the fallback for unknown error types.
apps/backend/tests/integration/wallet-auth.test.ts (1)
1-447: Excellent test coverage!This is a comprehensive test suite that covers all aspects of the wallet authentication system:
- Challenge generation and lifecycle
- Authentication flow with valid/invalid inputs
- API endpoints with proper error handling
- Custom error types
The console suppression pattern (lines 35-46) is a nice touch to keep test output clean.
apps/web/src/services/api.ts (1)
424-461: Well-implemented wallet authentication methods!The new wallet authentication methods follow the existing API patterns and properly handle errors. The challenge-response flow is correctly implemented with appropriate error handling.
| import cron, { type ScheduledTask } from 'node-cron'; | ||
| import { cleanupAllExpiredChallenges } from './wallet-challenge.service'; | ||
|
|
||
| let cleanupJob: ScheduledTask | null = null; | ||
| export function startCleanupScheduler() { | ||
| // Run cleanup every 2 minutes | ||
| cleanupJob = cron.schedule('*/2 * * * *', async () => { | ||
| try { | ||
| console.log('Running expired challenges cleanup...'); | ||
| await cleanupAllExpiredChallenges(); | ||
| console.log('Expired challenges cleanup completed'); | ||
| } catch (error) { | ||
| console.error('Error during challenges cleanup:', error); | ||
| } | ||
| }); | ||
|
|
||
| console.log('Challenge cleanup scheduler started (runs every 2 minutes)'); | ||
| } | ||
|
|
||
| export function stopCleanupScheduler() { | ||
| if (cleanupJob) { | ||
| cleanupJob.stop(); | ||
| cleanupJob = null; | ||
| console.log('Challenge cleanup scheduler stopped'); | ||
| } | ||
| } | ||
|
|
||
| // Run initial cleanup on startup | ||
| export async function runInitialCleanup() { | ||
| try { | ||
| console.log('Running initial expired challenges cleanup...'); | ||
| await cleanupAllExpiredChallenges(); | ||
| console.log('Initial cleanup completed'); | ||
| } catch (error) { | ||
| console.error('Error during initial cleanup:', error); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix filename typo and consider configurable cleanup interval.
The implementation logic is solid with proper error handling and logging. However, there's a typo in the filename: "schedular" should be "scheduler".
-// File: apps/backend/src/services/cleanup-schedular.ts
+// File: apps/backend/src/services/cleanup-scheduler.tsAlso consider making the cleanup interval configurable via environment variables:
export function startCleanupScheduler() {
+ const cleanupInterval = process.env.CLEANUP_INTERVAL || '*/2 * * * *';
- cleanupJob = cron.schedule('*/2 * * * *', async () => {
+ cleanupJob = cron.schedule(cleanupInterval, async () => {
// ... rest of the function
});
- console.log('Challenge cleanup scheduler started (runs every 2 minutes)');
+ console.log(`Challenge cleanup scheduler started (runs every ${cleanupInterval})`);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/backend/src/services/cleanup-schedular.ts lines 1 to 37, rename the file
to cleanup-scheduler.ts to fix the typo. Additionally, modify the cron schedule
expression to be configurable by reading the interval from an environment
variable, with a default fallback to the current 2-minute interval. This allows
flexible adjustment of the cleanup frequency without code changes.
| const handleAuthenticate = async () => { | ||
| setAuthState('authenticating'); | ||
| try { | ||
| await loginWithWallet(); | ||
| setAuthState('success'); | ||
| onSuccess?.(); | ||
| } catch (error) { | ||
| console.error('Authentication failed:', error); | ||
| const errorMessage = getErrorMessage(error); | ||
| onError?.(errorMessage); | ||
| setAuthState('error'); | ||
|
|
||
| // Reset to idle after showing error briefly | ||
| setTimeout(() => setAuthState('idle'), 3000); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add cleanup for setTimeout to prevent memory leaks.
The setTimeout on line 46 could cause a memory leak if the component unmounts before the timeout completes. Additionally, logging the full error object might expose sensitive information in production.
Apply this diff to fix the issues:
+import { useCallback, useEffect, useRef } from 'react';
const handleAuthenticate = async () => {
+ const timeoutRef = useRef<NodeJS.Timeout>();
setAuthState('authenticating');
try {
await loginWithWallet();
setAuthState('success');
onSuccess?.();
} catch (error) {
- console.error('Authentication failed:', error);
+ if (process.env.NODE_ENV === 'development') {
+ console.error('Authentication failed:', error);
+ }
const errorMessage = getErrorMessage(error);
onError?.(errorMessage);
setAuthState('error');
// Reset to idle after showing error briefly
- setTimeout(() => setAuthState('idle'), 3000);
+ timeoutRef.current = setTimeout(() => setAuthState('idle'), 3000);
}
};
+
+// Add cleanup effect
+useEffect(() => {
+ return () => {
+ if (timeoutRef.current) {
+ clearTimeout(timeoutRef.current);
+ }
+ };
+}, []);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/web/src/components/auth/WalletAuthButton.tsx around lines 33 to 48, the
setTimeout used to reset auth state after an error can cause a memory leak if
the component unmounts before it fires. To fix this, store the timeout ID in a
ref and clear it in a useEffect cleanup function. Also, replace console.error
with a safer logging method that avoids exposing sensitive error details in
production, such as logging only the error message or a sanitized version.
| // =========================== | ||
| // Transform wallet user to UserProfile | ||
| // =========================== | ||
| function transformWalletUser(walletUser: WalletAuthResponse['user']): UserProfile { | ||
| return { | ||
| id: walletUser.id, | ||
| name: walletUser.profile?.name || 'Wallet User', | ||
| email: '', | ||
| phone: walletUser.profile?.phone || '', | ||
| avatar: | ||
| walletUser.profile?.avatar_url || | ||
| 'https://images.unsplash.com/photo-1494790108755-2616b612b786?w=100', | ||
| memberSince: walletUser.profile?.last_active | ||
| ? new Date(walletUser.profile.last_active).getFullYear().toString() | ||
| : new Date().getFullYear().toString(), | ||
| verified: walletUser.profile?.verification_status === 'verified', | ||
| location: walletUser.profile?.address, | ||
| bio: '', | ||
| preferences: { | ||
| currency: 'USD', | ||
| language: 'English', | ||
| notifications: true, | ||
| }, | ||
| publicKey: walletUser.publicKey, | ||
| }; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider handling the empty email field for wallet users.
The transformWalletUser function sets an empty string for the email field (line 134). This might cause issues in parts of the application that expect a valid email address.
Consider either:
- Using a placeholder email format like
wallet-${publicKey.substring(0, 8)}@stellar.local - Making the email field optional in the UserProfile type
- Documenting this behavior for wallet-authenticated users
🏁 Script executed:
#!/bin/bash
# Description: Check if any code expects non-empty email values from UserProfile
# Search for email field usage in the codebase
rg -A 3 -B 3 "\.email\b" --type ts --type tsx | grep -v "test\." | head -20
# Check for email validation or required email checks
ast-grep --pattern 'if ($_.email) { $$$ }'Length of output: 150
🏁 Script executed:
#!/bin/bash
# Search for any usage of the email property on UserProfile across the repo
rg -n "\.email\b"
# Locate the definition of UserProfile to inspect whether email is required or optional
rg -n "interface UserProfile" -A3 -B1 || echo "No interface UserProfile found"
rg -n "type UserProfile" -A3 -B1 || echo "No type alias UserProfile found"Length of output: 2259
Ensure wallet users have a valid or optional email
Components across the app (e.g. profile‐management, HostContactCard, booking mailto links) assume UserProfile.email is nonempty and valid. Returning '' will lead to broken mailto URLs, blank UI fields, or regex validation failures.
Changes required:
- In apps/web/src/services/api.ts (transformWalletUser), replace the empty string with either a placeholder or
undefined - In apps/web/src/types/index.ts, update
UserProfile.emailtoemail?: stringif you choose to allow missing emails - Guard UI code (tenant‐dashboard, HostContactCard, etc.) to skip or disable email‐based flows when there is no email
Suggested diff for placeholder email approach:
--- a/apps/web/src/services/api.ts
@@ function transformWalletUser(walletUser: WalletAuthResponse['user']): UserProfile {
- email: '',
+ email:
+ walletUser.profile?.email ||
+ `wallet-${walletUser.publicKey.substring(0, 8)}@stellar.local`,And in apps/web/src/types/index.ts:
export interface UserProfile {
id: string;
name: string;
- email: string;
+ email?: string;
// …
}📝 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.
| // =========================== | |
| // Transform wallet user to UserProfile | |
| // =========================== | |
| function transformWalletUser(walletUser: WalletAuthResponse['user']): UserProfile { | |
| return { | |
| id: walletUser.id, | |
| name: walletUser.profile?.name || 'Wallet User', | |
| email: '', | |
| phone: walletUser.profile?.phone || '', | |
| avatar: | |
| walletUser.profile?.avatar_url || | |
| 'https://images.unsplash.com/photo-1494790108755-2616b612b786?w=100', | |
| memberSince: walletUser.profile?.last_active | |
| ? new Date(walletUser.profile.last_active).getFullYear().toString() | |
| : new Date().getFullYear().toString(), | |
| verified: walletUser.profile?.verification_status === 'verified', | |
| location: walletUser.profile?.address, | |
| bio: '', | |
| preferences: { | |
| currency: 'USD', | |
| language: 'English', | |
| notifications: true, | |
| }, | |
| publicKey: walletUser.publicKey, | |
| }; | |
| } | |
| // =========================== | |
| // Transform wallet user to UserProfile | |
| // =========================== | |
| function transformWalletUser(walletUser: WalletAuthResponse['user']): UserProfile { | |
| return { | |
| id: walletUser.id, | |
| name: walletUser.profile?.name || 'Wallet User', | |
| email: | |
| walletUser.profile?.email || | |
| `wallet-${walletUser.publicKey.substring(0, 8)}@stellar.local`, | |
| phone: walletUser.profile?.phone || '', | |
| avatar: | |
| walletUser.profile?.avatar_url || | |
| 'https://images.unsplash.com/photo-1494790108755-2616b612b786?w=100', | |
| memberSince: walletUser.profile?.last_active | |
| ? new Date(walletUser.profile.last_active).getFullYear().toString() | |
| : new Date().getFullYear().toString(), | |
| verified: walletUser.profile?.verification_status === 'verified', | |
| location: walletUser.profile?.address, | |
| bio: '', | |
| preferences: { | |
| currency: 'USD', | |
| language: 'English', | |
| notifications: true, | |
| }, | |
| publicKey: walletUser.publicKey, | |
| }; | |
| } |
🤖 Prompt for AI Agents
In apps/web/src/services/api.ts between lines 127 and 152, the
transformWalletUser function returns an empty string for the email field, which
causes issues in components expecting a valid nonempty email. To fix this,
replace the empty string with either a placeholder email or undefined.
Additionally, update the UserProfile.email type in apps/web/src/types/index.ts
to be optional (email?: string) if allowing missing emails. Finally, review and
update UI components like tenant-dashboard and HostContactCard to handle cases
where email is missing by skipping or disabling email-dependent features.
|
@respp
|
2 participants
Pull Request | StellarRent
Closes #82
📝 Summary
Implemented secure wallet-based authentication for Stellar wallets using challenge-response mechanism. Users can now authenticate with Freighter wallet as an alternative to email/password.
Files Added/Modified
Backend
🔄 Changes Made
POST /auth/challengeendpoint that generates cryptographically secure challengesPOST /auth/walletendpoint for wallet authenticationwallet_challenges,wallet_users,profilesKey Features
@stellar/freighter-api(notwindow.freighterApi) for reliable wallet detectionFrontend
Authentication Flow
✅ Testing Checklist
Proof
Watch Video
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Chores