Conversation
|
@mimijuwonlo-commits is attempting to deploy a commit to the olufunbiik's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe PR normalizes authenticated request context across guards and controllers by introducing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/verification/verification.controller.ts (1)
67-74:⚠️ Potential issue | 🔴 CriticalUse
ForbiddenExceptioninstead of genericErrorfor access control.Throwing a generic
Errorreturns HTTP 500 instead of 403. The global exception filter only remapsHttpExceptionsubclasses; plainErrorinstances default toINTERNAL_SERVER_ERRORstatus code.Suggested fix
import { Controller, Get, Post, Patch, Delete, Body, Param, Query, UseGuards, UseInterceptors, UploadedFiles, ParseUUIDPipe, HttpCode, HttpStatus, + ForbiddenException, } from '@nestjs/common'; @@ - if (!user.isArtist) { - throw new Error('Only artists can request verification'); - } + if (!user.isArtist) { + throw new ForbiddenException('Only artists can request verification'); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/verification/verification.controller.ts` around lines 67 - 74, In createVerificationRequest change the thrown generic Error to Nest's ForbiddenException so access denial returns HTTP 403: replace the throw new Error('Only artists can request verification') in the createVerificationRequest method with throw new ForbiddenException('Only artists can request verification') (import ForbiddenException from `@nestjs/common` if not already imported).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/admin/guards/admin-role.guard.ts`:
- Around line 47-49: Remove logging of raw user.userId in the AdminRoleGuard
logger.warn calls and replace with a privacy-safe identifier: either use the
existing request/correlation id from the execution context (if available) or log
a redacted/hashed form of the principal (e.g., SHA-256 or truncated HMAC of
user.userId) so the original stable identifier is not persisted; update both the
warn at the `Denied admin access: no admin role for userId=${user.userId}` site
and the similar warn/error calls around the other occurrence (lines referenced
in the review) to include the correlation id or the hashed/redacted user id
instead of user.userId, and ensure the hashing uses a constant salt/config so
the resulting token can be correlated for troubleshooting without exposing the
raw value.
In `@backend/src/auth/guards/roles.guard.ts`:
- Around line 9-12: RolesGuard is injectable and injects User repository but
isn't provided by any module, causing runtime resolution failures where
controllers use `@UseGuards`(RolesGuard); add RolesGuard to the AuthModule
providers and exports (alongside TypeOrmModule.forFeature([User]) and existing
providers like AuthService and JwtAuthGuard) so it can be injected, then ensure
modules using `@UseGuards`(RolesGuard) import AuthModule (or export RolesGuard
from a shared guards module) so RolesGuard can be resolved at runtime.
---
Outside diff comments:
In `@backend/src/verification/verification.controller.ts`:
- Around line 67-74: In createVerificationRequest change the thrown generic
Error to Nest's ForbiddenException so access denial returns HTTP 403: replace
the throw new Error('Only artists can request verification') in the
createVerificationRequest method with throw new ForbiddenException('Only artists
can request verification') (import ForbiddenException from `@nestjs/common` if not
already imported).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a66ef2b2-113c-4f8c-a9f5-7f10194e71f3
📒 Files selected for processing (11)
backend/src/admin/admin.controller.tsbackend/src/admin/guards/admin-role.guard.tsbackend/src/app.module.tsbackend/src/artiste-payout/payouts.controller.tsbackend/src/artists/artists.controller.tsbackend/src/auth/guards/roles.guard.tsbackend/src/recommendations/recommendations.controller.tsbackend/src/social-sharing/referral.controller.tsbackend/src/subscription-tiers/subscriptions.controller.tsbackend/src/verification/verification.controller.tsbackend/src/waveform/waveform.controller.ts
| logger.warn( | ||
| `Denied admin access: no admin role for userId=${user.userId}`, | ||
| ); |
There was a problem hiding this comment.
Avoid logging raw userId values on authorization failures.
These warning paths persist stable user identifiers in auth logs. That is unnecessary for denial auditing and creates a privacy/compliance risk. Prefer a request/correlation ID, or redact/hash the principal before logging.
🛠️ Minimal fix
- logger.warn(
- `Denied admin access: no admin role for userId=${user.userId}`,
- );
+ logger.warn(
+ 'Denied admin access: no admin role for authenticated principal',
+ );
...
- logger.warn(
- `Denied admin access: missing permissions for userId=${user.userId} required=${requiredPermissions.join(',')}`,
- );
+ logger.warn(
+ `Denied admin access: missing permissions required=${requiredPermissions.join(',')}`,
+ );Also applies to: 58-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/admin/guards/admin-role.guard.ts` around lines 47 - 49, Remove
logging of raw user.userId in the AdminRoleGuard logger.warn calls and replace
with a privacy-safe identifier: either use the existing request/correlation id
from the execution context (if available) or log a redacted/hashed form of the
principal (e.g., SHA-256 or truncated HMAC of user.userId) so the original
stable identifier is not persisted; update both the warn at the `Denied admin
access: no admin role for userId=${user.userId}` site and the similar warn/error
calls around the other occurrence (lines referenced in the review) to include
the correlation id or the hashed/redacted user id instead of user.userId, and
ensure the hashing uses a constant salt/config so the resulting token can be
correlated for troubleshooting without exposing the raw value.
| constructor( | ||
| private reflector: Reflector, | ||
| @InjectRepository(User) private readonly userRepository: Repository<User>, | ||
| ) {} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "RolesGuard references in modules:"
rg -n -C2 '\bRolesGuard\b' backend/src --glob '**/*.module.ts' || true
echo
echo "RolesGuard use sites:"
rg -n -C2 '@UseGuards\([^)]*\bRolesGuard\b' backend/src --type=ts || true
echo
echo "AuthModule wiring:"
sed -n '1,200p' backend/src/auth/auth.module.tsRepository: OlufunbiIK/tip-tune
Length of output: 6982
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Finding RolesGuard file ==="
fd -type f -name '*roles.guard*' backend/src
echo
echo "=== RolesGuard implementation ==="
cat -n backend/src/auth/guards/roles.guard.ts
echo
echo "=== Checking if RolesGuard is exported from any index/barrel file ==="
rg -n 'RolesGuard' backend/src --glob '**/*index*' || true
echo
echo "=== Checking if RolesGuard appears in any .module.ts ==="
rg -n 'RolesGuard' backend/src --glob '**/*.module.ts' || true
echo
echo "=== Checking RolesGuard in auth.module.ts (full file) ==="
wc -l backend/src/auth/auth.module.tsRepository: OlufunbiIK/tip-tune
Length of output: 282
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Finding RolesGuard file ==="
find backend/src -name '*roles.guard*'
echo
echo "=== RolesGuard implementation ==="
cat -n backend/src/auth/guards/roles.guard.ts
echo
echo "=== Checking all .module.ts files for RolesGuard ==="
rg 'RolesGuard' backend/src --type ts -l
echo
echo "=== Full AuthModule file ==="
cat -n backend/src/auth/auth.module.tsRepository: OlufunbiIK/tip-tune
Length of output: 3269
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Checking app.module.ts ==="
find backend/src -name 'app.module.ts' -exec cat -n {} \;
echo
echo "=== Searching for RolesGuard in any import/provider context ==="
rg -B3 -A3 'RolesGuard' backend/src --type ts --glob '**/*.ts' | grep -E '(providers|imports|@Module)' || trueRepository: OlufunbiIK/tip-tune
Length of output: 6788
RolesGuard must be added to AuthModule's providers and exports.
RolesGuard is @Injectable() and depends on @InjectRepository(User), but is not registered in any module's providers array. It is currently used via @UseGuards(RolesGuard) across 15+ routes in ReportsModule, ModerationModule, FeesModule, platinum-fee, and VerificationModule, but these controllers cannot resolve it at runtime.
To fix, add RolesGuard to AuthModule.providers and AuthModule.exports:
`@Module`({
imports: [TypeOrmModule.forFeature([User]), ...],
providers: [AuthService, WalletStrategy, JwtAuthGuard, RolesGuard],
exports: [AuthService, JwtAuthGuard, WalletStrategy, RolesGuard],
})
export class AuthModule {}Then import AuthModule into any module that uses @UseGuards(RolesGuard), or export RolesGuard from a shared guards module.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/auth/guards/roles.guard.ts` around lines 9 - 12, RolesGuard is
injectable and injects User repository but isn't provided by any module, causing
runtime resolution failures where controllers use `@UseGuards`(RolesGuard); add
RolesGuard to the AuthModule providers and exports (alongside
TypeOrmModule.forFeature([User]) and existing providers like AuthService and
JwtAuthGuard) so it can be injected, then ensure modules using
`@UseGuards`(RolesGuard) import AuthModule (or export RolesGuard from a shared
guards module) so RolesGuard can be resolved at runtime.
| async createTier(@Body() dto: CreateSubscriptionTierDto) { | ||
| // Enforce that the artistId in the DTO matches the requesting artist | ||
| return this.subscriptionsService.createTier(dto); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "DTO fields:"
rg -n -C2 'class CreateSubscriptionTierDto|class UpdateSubscriptionTierDto|artistId' \
backend/src/subscription-tiers/subscriptions.dto.ts
echo
echo "Tier mutation controller/service flow:"
rg -n -C3 'createTier\(|updateTier\(|deleteTier\(' \
backend/src/subscription-tiers/subscriptions.controller.ts \
backend/src/subscription-tiers/subscriptions.service.tsRepository: OlufunbiIK/tip-tune
Length of output: 5051
🏁 Script executed:
cat -n backend/src/subscription-tiers/subscriptions.dto.tsRepository: OlufunbiIK/tip-tune
Length of output: 2859
🏁 Script executed:
cat -n backend/src/auth/decorators/current-user.decorator.tsRepository: OlufunbiIK/tip-tune
Length of output: 946
🏁 Script executed:
rg -n 'artistId' backend/src/subscription-tiers/subscriptions.dto.tsRepository: OlufunbiIK/tip-tune
Length of output: 85
🏁 Script executed:
cat -n backend/src/subscription-tiers/subscriptions.service.ts | head -100Repository: OlufunbiIK/tip-tune
Length of output: 4035
🏁 Script executed:
rg -n "artistId|userId" backend/src/subscription-tiers/subscriptions.service.ts | head -30Repository: OlufunbiIK/tip-tune
Length of output: 1055
🏁 Script executed:
fd -t f "user.*\.(ts|entity)" backend/src/ | head -10Repository: OlufunbiIK/tip-tune
Length of output: 553
🏁 Script executed:
rg -l "entity|Entity" backend/src/ | grep -i user | head -5Repository: OlufunbiIK/tip-tune
Length of output: 251
🏁 Script executed:
cat -n backend/src/users/entities/user.entity.tsRepository: OlufunbiIK/tip-tune
Length of output: 1892
🏁 Script executed:
rg -n "artistId|isArtist" backend/src/users/entities/user.entity.tsRepository: OlufunbiIK/tip-tune
Length of output: 86
🏁 Script executed:
fd -t f "artist.*entity" backend/src/ -o fd -t f "profile.*entity" backend/src/ | head -10Repository: OlufunbiIK/tip-tune
Length of output: 186
🏁 Script executed:
rg -l "artistId|ArtistId" backend/src/ --type ts | grep -i "entity\|model" | head -10Repository: OlufunbiIK/tip-tune
Length of output: 607
🏁 Script executed:
cat -n backend/src/subscription-tiers/subscription-tier.entity.tsRepository: OlufunbiIK/tip-tune
Length of output: 1775
🏁 Script executed:
rg -n "userId|artistId" backend/src/subscription-tiers/subscription-tier.entity.tsRepository: OlufunbiIK/tip-tune
Length of output: 85
Don't take artistId from the request body for tier mutations.
These endpoints are artist-owned actions, but artistId is entirely client-controlled via the request. createTier spreads a DTO with artistId: string directly into the tier, updateTier calls service.updateTier(tierId, dto.artistId, ...) even though UpdateSubscriptionTierDto has no artistId field (causing a TypeScript TS2339 error), and deleteTier accepts artistId from the request body. Because the service authorizes by comparing the supplied artistId to tier.artistId, callers can spoof any artist's tier.
Resolve ownership from @CurrentUser("userId") instead. Since artists are identified by their user ID (the isArtist boolean flag exists), pass userId to the service methods and perform the ownership check against tier.artistId === userId.
🔐 Suggested fix
`@Post`("tiers")
- async createTier(`@Body`() dto: CreateSubscriptionTierDto) {
- // Enforce that the artistId in the DTO matches the requesting artist
- return this.subscriptionsService.createTier(dto);
+ async createTier(
+ `@CurrentUser`("userId") userId: string,
+ `@Body`() dto: CreateSubscriptionTierDto,
+ ) {
+ return this.subscriptionsService.createTier(userId, dto);
}
`@Patch`("tiers/:tierId")
async updateTier(
`@Param`("tierId", ParseUUIDPipe) tierId: string,
+ `@CurrentUser`("userId") userId: string,
`@Body`() dto: UpdateSubscriptionTierDto,
) {
- // Note: artist ownership should be enforced in service using user context if needed
- return this.subscriptionsService.updateTier(tierId, dto.artistId, dto);
+ return this.subscriptionsService.updateTier(tierId, userId, dto);
}
`@Delete`("tiers/:tierId")
`@HttpCode`(HttpStatus.NO_CONTENT)
- async deleteTier(`@Param`("tierId", ParseUUIDPipe) tierId: string, `@Body`() body: { artistId: string }) {
- await this.subscriptionsService.deleteTier(tierId, body.artistId);
+ async deleteTier(
+ `@Param`("tierId", ParseUUIDPipe) tierId: string,
+ `@CurrentUser`("userId") userId: string,
+ ) {
+ await this.subscriptionsService.deleteTier(tierId, userId);
+ }Also applies to: 52–53, 62–63
2 participants
close #322
close #323
close #324
close #325
Summary by CodeRabbit
New Features
Refactor
/apiprefix from subscription, payout, referral, and waveform endpoints.