Feat/improve project #847
Feat/improve project #847
Conversation
WalkthroughAdds a frontend env var for API base URL. Introduces runtime env validation for SUPABASE and JWT in backend startup and Supabase client module. Updates rate limiter to skip health-check requests. Reworks email registration to use a mock wallet address derived from user ID, updating persisted and returned fields. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as Backend API
participant RL as RateLimiter
participant HC as HealthCheck Skip
Client->>API: Request (GET /, /health, /api/health)
API->>RL: evaluate(request)
RL->>HC: skipPredicate(user-agent, path)
alt Health-check path/UA
HC-->>RL: true
note over RL,API: Health-check bypasses rate limiting
RL-->>API: allow (no count)
else Normal traffic
HC-->>RL: false
RL-->>API: apply limit (count/enforce)
end
API-->>Client: Response
sequenceDiagram
autonumber
actor User
participant Auth as AuthService.registerWithEmail
participant DB as Users Table
User->>Auth: registerWithEmail(email, password)
Auth->>DB: create user
DB-->>Auth: newUser(id)
note right of Auth: Generate mock wallet\naddress from newUser.id
Auth->>DB: update user.wallet_address = mockAddress
alt Update success
DB-->>Auth: updated user
Auth-->>User: { user, wallet: { address: mockAddress, type: "invisible" } }
else Update fails
DB-->>Auth: error
Auth-->>User: throw AppError
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Frontend CI/CD Pipeline Completed Successfully!
Ready for review and deployment! 🚀 |
![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
🧹 Nitpick comments (2)
backend/src/lib/supabase/supabase.ts (1)
6-12: Track temporary debug code for removal.The comment indicates this debug logging should be removed after fixing. Consider creating a tracked issue to ensure this temporary code is removed once the environment configuration is stable.
Do you want me to open a new issue to track the removal of this debug code?
backend/src/services/auth.service.ts (1)
142-157: Temporary mock wallet approach is appropriate for development.The commented-out wallet generation code is clearly marked with TODO comments indicating it will be re-enabled when Stellar integration is ready. This is a reasonable approach for continuing development while blockchain integration is being worked on.
Do you want me to open a tracking issue for re-enabling Stellar wallet generation and blockchain registration?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.env.local(1 hunks)backend/src/index.ts(1 hunks)backend/src/lib/supabase/supabase.ts(1 hunks)backend/src/middlewares/ratelimit.middleware.ts(1 hunks)backend/src/services/auth.service.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
backend/src/middlewares/ratelimit.middleware.ts (1)
backend/src/types/express.d.ts (1)
Request(5-8)
backend/src/services/auth.service.ts (3)
backend/src/lib/supabase/supabase.ts (1)
supabase(14-14)backend/src/utils/AppError.ts (1)
AppError(6-25)backend/src/utils/sanitizeUser.ts (1)
sanitizeUser(3-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (20.x)
- GitHub Check: build (18.x)
🔇 Additional comments (2)
.env.local (1)
1-1: LGTM! Frontend API URL configured correctly.The
NEXT_PUBLIC_API_URLvariable follows Next.js conventions and appropriately targets the local backend API endpoint for development.backend/src/services/auth.service.ts (1)
234-241: Mock wallet data consistently applied in response.The sanitized user object and wallet response correctly use the mock wallet address. The wallet type 'invisible' appropriately indicates this is a generated (mock) wallet rather than an external wallet.
| // Validate critical environment variables | ||
| console.log('🔍 Checking environment variables...'); | ||
| const requiredEnvVars = [ | ||
| 'SUPABASE_URL', | ||
| 'SUPABASE_SERVICE_ROLE_KEY', | ||
| 'JWT_SECRET' | ||
| ]; | ||
|
|
||
| const missingEnvVars = requiredEnvVars.filter(varName => !process.env[varName]); | ||
| if (missingEnvVars.length > 0) { | ||
| console.error('❌ Missing required environment variables:', missingEnvVars); | ||
| console.error('📋 Available SUPABASE env vars:', Object.keys(process.env).filter(k => k.includes('SUPABASE'))); | ||
| } else { | ||
| console.log('✅ All required environment variables are set'); | ||
| } |
There was a problem hiding this comment.
Halt execution on missing critical environment variables.
The validation block logs missing environment variables but allows the application to continue startup. This can lead to confusing runtime errors later when these variables are accessed. Additionally, similar validation logic exists in backend/src/lib/supabase/supabase.ts, creating duplication.
Apply this diff to exit on missing variables:
const missingEnvVars = requiredEnvVars.filter(varName => !process.env[varName]);
if (missingEnvVars.length > 0) {
console.error('❌ Missing required environment variables:', missingEnvVars);
console.error('📋 Available SUPABASE env vars:', Object.keys(process.env).filter(k => k.includes('SUPABASE')));
+ process.exit(1);
} else {
console.log('✅ All required environment variables are set');
}Consider consolidating this validation with the similar logic in backend/src/lib/supabase/supabase.ts to eliminate duplication.
📝 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.
| // Validate critical environment variables | |
| console.log('🔍 Checking environment variables...'); | |
| const requiredEnvVars = [ | |
| 'SUPABASE_URL', | |
| 'SUPABASE_SERVICE_ROLE_KEY', | |
| 'JWT_SECRET' | |
| ]; | |
| const missingEnvVars = requiredEnvVars.filter(varName => !process.env[varName]); | |
| if (missingEnvVars.length > 0) { | |
| console.error('❌ Missing required environment variables:', missingEnvVars); | |
| console.error('📋 Available SUPABASE env vars:', Object.keys(process.env).filter(k => k.includes('SUPABASE'))); | |
| } else { | |
| console.log('✅ All required environment variables are set'); | |
| } | |
| // Validate critical environment variables | |
| console.log('🔍 Checking environment variables...'); | |
| const requiredEnvVars = [ | |
| 'SUPABASE_URL', | |
| 'SUPABASE_SERVICE_ROLE_KEY', | |
| 'JWT_SECRET' | |
| ]; | |
| const missingEnvVars = requiredEnvVars.filter(varName => !process.env[varName]); | |
| if (missingEnvVars.length > 0) { | |
| console.error('❌ Missing required environment variables:', missingEnvVars); | |
| console.error( | |
| '📋 Available SUPABASE env vars:', | |
| Object.keys(process.env).filter(k => k.includes('SUPABASE')) | |
| ); | |
| process.exit(1); | |
| } else { | |
| console.log('✅ All required environment variables are set'); | |
| } |
🤖 Prompt for AI Agents
In backend/src/index.ts around lines 4 to 18, the env-var check currently only
logs missing variables; modify it to exit the process with a non-zero code when
any required env var is missing (e.g., call process.exit(1) after logging the
missing vars) so startup halts instead of continuing into runtime errors, and
then remove or consolidate duplicate validation by centralizing this check into
backend/src/lib/supabase/supabase.ts (or export a small validation helper from
that module and call it here) to avoid duplicated logic.
| const supabaseUrl = process.env.SUPABASE_URL! | ||
| const supabaseKey = process.env.SUPABASE_SERVICE_ROLE_KEY! |
There was a problem hiding this comment.
Move validation before using the variables.
The non-null assertions (!) and subsequent usage of supabaseUrl and supabaseKey occur before the validation check on lines 7-12. If these environment variables are missing, createClient will be called with undefined values, causing a runtime error before the validation logging executes.
Apply this diff to validate before usage:
import { createClient } from '@supabase/supabase-js'
-const supabaseUrl = process.env.SUPABASE_URL!
-const supabaseKey = process.env.SUPABASE_SERVICE_ROLE_KEY!
-
// Log for debugging (will remove after fixing)
-if (!supabaseUrl || !supabaseKey) {
+const supabaseUrl = process.env.SUPABASE_URL
+const supabaseKey = process.env.SUPABASE_SERVICE_ROLE_KEY
+
+if (!supabaseUrl || !supabaseKey) {
console.error('❌ SUPABASE CONFIGURATION ERROR:');
console.error('SUPABASE_URL:', supabaseUrl ? 'SET' : 'MISSING');
console.error('SUPABASE_SERVICE_ROLE_KEY:', supabaseKey ? 'SET' : 'MISSING');
console.error('All env vars:', Object.keys(process.env).filter(k => k.includes('SUPABASE')));
+ throw new Error('Missing required SUPABASE environment variables');
}
export const supabase = createClient(supabaseUrl, supabaseKey)📝 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 supabaseUrl = process.env.SUPABASE_URL! | |
| const supabaseKey = process.env.SUPABASE_SERVICE_ROLE_KEY! | |
| import { createClient } from '@supabase/supabase-js' | |
| const supabaseUrl = process.env.SUPABASE_URL | |
| const supabaseKey = process.env.SUPABASE_SERVICE_ROLE_KEY | |
| // Log for debugging (will remove after fixing) | |
| if (!supabaseUrl || !supabaseKey) { | |
| console.error('❌ SUPABASE CONFIGURATION ERROR:'); | |
| console.error('SUPABASE_URL:', supabaseUrl ? 'SET' : 'MISSING'); | |
| console.error('SUPABASE_SERVICE_ROLE_KEY:', supabaseKey ? 'SET' : 'MISSING'); | |
| console.error('All env vars:', Object.keys(process.env).filter(k => k.includes('SUPABASE'))); | |
| throw new Error('Missing required SUPABASE environment variables'); | |
| } | |
| export const supabase = createClient(supabaseUrl, supabaseKey) |
🤖 Prompt for AI Agents
In backend/src/lib/supabase/supabase.ts around lines 3 to 4, the code currently
uses non-null assertions when reading SUPABASE_URL and SUPABASE_SERVICE_ROLE_KEY
before the presence check, which can cause a runtime error; change to read the
env vars without "!" into local constants, perform the existing validation check
first (log and throw or process.exit if missing), and only after validation call
createClient using the validated variables (remove the non-null assertions and
ensure createClient receives defined strings).
| // Skip rate limiting for health checks from Render, Vercel, etc. | ||
| skip: (req: Request) => { | ||
| const userAgent = req.get('User-Agent') || ''; | ||
| const isHealthCheck = userAgent.includes('Render') || | ||
| userAgent.includes('Vercel') || | ||
| userAgent.includes('health-check') || | ||
| req.path === '/' || | ||
| req.path === '/health' || | ||
| req.path === '/api/health'; | ||
| return isHealthCheck; | ||
| }, |
There was a problem hiding this comment.
User-Agent bypass creates security vulnerability.
The skip predicate bypasses rate limiting for any request containing 'Render', 'Vercel', or 'health-check' in the User-Agent header. Since User-Agent headers are trivially spoofed, this creates a security bypass that attackers can exploit to circumvent rate limiting on all endpoints.
Consider these more secure alternatives:
Option 1 (Recommended): Path-based bypass only
skip: (req: Request) => {
- const userAgent = req.get('User-Agent') || '';
- const isHealthCheck = userAgent.includes('Render') ||
- userAgent.includes('Vercel') ||
- userAgent.includes('health-check') ||
- req.path === '/' ||
- req.path === '/health' ||
- req.path === '/api/health';
+ const isHealthCheck = req.path === '/health' || req.path === '/api/health';
return isHealthCheck;
},Option 2: Add IP allowlist for monitoring services
skip: (req: Request) => {
const MONITORING_IPS = process.env.MONITORING_IPS?.split(',') || [];
const clientIp = req.ip || '';
const isHealthCheckPath = req.path === '/health' || req.path === '/api/health';
const isTrustedMonitor = MONITORING_IPS.includes(clientIp);
return isHealthCheckPath || isTrustedMonitor;
},The root path (/) should likely retain rate limiting unless there's a specific requirement to bypass it.
🤖 Prompt for AI Agents
In backend/src/middlewares/ratelimit.middleware.ts around lines 31 to 41, the
current skip predicate bypasses rate limiting based on User-Agent which is
trivially spoofable; remove the User-Agent checks and only allow bypass for
explicit health-check paths (e.g., '/health' and '/api/health'), and if needed
add an optional IP allowlist (read from an env var like MONITORING_IPS) to
additionally permit trusted monitoring IPs; do not bypass for the root path '/'
and ensure the client IP is taken from a reliable source (req.ip or a trusted
proxy header) when checking the allowlist.
| // Mock wallet address for development | ||
| const mockWalletAddress = `MOCK_${newUser.id.substring(0, 8).toUpperCase()}`; | ||
|
|
||
| // Update user with mock wallet address | ||
| const { error: updateError } = await supabase | ||
| .from("users") | ||
| .update({ wallet_address: mockWalletAddress }) | ||
| .eq("id", newUser.id); | ||
|
|
||
| if (updateError) { | ||
| throw new AppError(`Failed to link mock wallet to user: ${updateError.message}`, 500); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify production deployment guards for mock wallet flow.
The mock wallet implementation uses a deterministic pattern (MOCK_ + user ID substring) which is appropriate for development. However, ensure that this code path is protected by environment checks or feature flags to prevent mock wallets from being created in production.
Run the following script to check for production environment guards:
🏁 Script executed:
#!/bin/bash
# Description: Search for environment or feature flag checks around wallet generation
# Check if there are any environment-based guards in auth routes or service
rg -n -C5 "NODE_ENV.*production|FEATURE.*WALLET|ENABLE.*STELLAR" --type=tsLength of output: 11058
🏁 Script executed:
#!/bin/bash
rg -n -C5 "mockWalletAddress" backend/src/services/auth.service.tsLength of output: 1063
Wrap mock wallet flow in a production guard
The mock wallet logic in backend/src/services/auth.service.ts (lines 159–170, 234–240) runs unconditionally—enclose it in a process.env.NODE_ENV !== 'production' check or a feature flag to prevent mock wallets in production.
🤖 Prompt for AI Agents
In backend/src/services/auth.service.ts around lines 159–170 and 234–240 the
code unconditionally creates and links a mock wallet address; wrap both
mock-wallet creation and update flows with a production guard (e.g. if
(process.env.NODE_ENV !== 'production') { ... }) or a configurable feature flag
check so the mock logic never runs in production; ensure the guard surrounds
both the generation of mockWalletAddress and the Supabase update,
return/continue normally when skipped, and keep error handling intact inside the
guarded block.
|
✅ Frontend CI/CD Pipeline Completed Successfully!
Ready for review and deployment! 🚀 |
1 participant
📝 Pull Request Title
🛠️ Issue
📚 Description
✅ Changes applied
🔍 Evidence/Media (screenshots/videos)
Summary by CodeRabbit
New Features
Chores