Security/p0 hardening#17
Merged
Merged
Conversation
…ons refs
Background:
The two December 2025 migrations 20251228000001_add_release_date_to_functions
and 20251228000002_fix_match_movies_signature both reference m.translations
in their match_movies / match_movies_by_profile / get_similar_movies
function bodies, but the translations column is only added later by
20260117000002_add_translations. The files were edited in place after
translations existed in production, so prod is fine, but `supabase db
reset` from scratch fails with `column m.translations does not exist`,
which blocked local Phase 1 testing.
Fix:
- Remove the premature translations references from the two December
migrations (returns table no longer lists translations jsonb; SELECT no
longer reads m.translations).
- Add a new migration 20260505000001_restore_match_translations that runs
AFTER 20260117000002_add_translations and re-adds translations to all
three function signatures via DROP + CREATE OR REPLACE.
Production impact: the new migration's CREATE OR REPLACE produces functions
identical to what's already in prod (translations column included), so it's
a no-op on prod. On a fresh dev DB the chain replays cleanly:
20251228000001 → match_* without translations
20260117000002 → translations column added
20260505000001 → match_* recreated with translations
Verified locally: `supabase db reset` completes successfully through all 14
migrations, and pg_get_function_result on the final match_movies returns:
TABLE(... release_date date, similarity double precision, translations jsonb)
…new improvement sections Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…in key in ops scripts - remove committed *.log files from git, ignore *.log and .env.* going forward - setup-production.sh: PROD_URL/ADMIN_API_KEY required from env, no hardcoded prod URL - all queue curl calls send X-Admin-Api-Key (guard lands in next commit) - delete unused apps/api/.env.redis Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t endpoints Fail-closed guard: X-Admin-Api-Key header must match ADMIN_API_KEY env var (timing-safe comparison). Applied controller-wide to /api/queues/* and /api/embeddings/*, and to the import POST routes of /api/tmdb/*. Also enables 'pnpm test' for the api package (was a stub). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- SupabaseAuthGuard validates Authorization: Bearer via supabaseAnon.auth.getUser - watchlist, chat, users fully guarded; recommendations guarded except /popular - user_id is taken from the verified JWT (CurrentUser decorator), never from query/body — closes cross-user data access via user_id substitution - chat routes: history/:userId -> history, clear/:userId -> clear - users routes: /:userId/language -> /me/language - web api client attaches the Supabase session token to every request Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Per-IP limit from RATE_LIMIT_WINDOW_MS / RATE_LIMIT_MAX_REQUESTS env (previously dead config), default 100 req/min. Stricter per-route limits: chat 10/min (LLM cost), movie/tv search 30/min (embedding cost). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ue scheduling whitelist+transform strips unknown body fields (clients can no longer smuggle user_id); class-validator DTOs cover chat message, watchlist add/watched and rotational import scheduling (5-field cron check, count 1..500). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ion checklist Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 2 minutes and 26 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThis PR implements a comprehensive P0 security hardening sprint dated 2026-06-11, adding authentication guards (admin API key and Supabase JWT), global request validation and rate limiting, protecting operational and user endpoints, updating the web client to send Bearer tokens, correcting database function signatures, and providing production deployment and operational monitoring documentation. ChangesP0 Security Hardening
Sequence DiagramsequenceDiagram
participant User
participant WebClient
participant API
participant AdminApiKeyGuard
participant SupabaseAuthGuard
participant QueuesController
User->>WebClient: log in via Supabase
WebClient->>WebClient: store Supabase session
User->>WebClient: request recommendations
WebClient->>API: GET /api/recommendations with Authorization Bearer token
API->>SupabaseAuthGuard: validate Bearer token
SupabaseAuthGuard->>API: attach AuthenticatedUser to request
API->>User: return personalized recommendations
Admin->>API: POST /api/queues/schedule with X-Admin-Api-Key
API->>AdminApiKeyGuard: validate admin key
AdminApiKeyGuard->>API: grant access
API->>QueuesController: schedule rotational import
API->>Admin: confirm rotation scheduled
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/api/src/queues/queues.controller.ts (2)
307-315:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn the effective default count in schedule responses.
Line 314 and Line 341 interpolate
body.count, which isundefinedwhen defaults are used (?? 100/?? 80). Return the resolved value instead.Suggested patch
async scheduleRotationalMovieImport( `@Body`() body: ScheduleRotationalImportDto ) { try { + const effectiveCount = body.count ?? 100; await this.queuesService.scheduleRotationalMovieImport( body.cronExpression, - body.count ?? 100 + effectiveCount ); return { success: true, - message: `Scheduled rotational movie import: ${body.cronExpression} (${body.count} per run)`, + message: `Scheduled rotational movie import: ${body.cronExpression} (${effectiveCount} per run)`, };async scheduleRotationalTvImport( `@Body`() body: ScheduleRotationalImportDto ) { try { + const effectiveCount = body.count ?? 80; await this.queuesService.scheduleRotationalTvImport( body.cronExpression, - body.count ?? 80 + effectiveCount ); return { success: true, - message: `Scheduled rotational TV import: ${body.cronExpression} (${body.count} per run)`, + message: `Scheduled rotational TV import: ${body.cronExpression} (${effectiveCount} per run)`, };Also applies to: 334-342
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/queues/queues.controller.ts` around lines 307 - 315, The response messages are interpolating body.count which can be undefined when defaults are applied; compute the resolved count locally (e.g., const resolvedCount = body.count ?? 100) before calling this.queuesService.scheduleRotationalMovieImport and use resolvedCount in the returned message; do the same for the rotational series endpoint (use the same pattern with the default 80 for the scheduleRotationalSeriesImport call) so the responses show the effective count rather than possibly "undefined".
19-20:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftComplete DTO coverage for request bodies in this controller.
These endpoints still use inline body object types, so global
ValidationPipecannot enforce class-based DTO rules here. In a P0 hardening PR, this leaves multiple admin write routes under-validated.As per coding guidelines,
apps/api/src/**/*.{ts,tsx}must “Define Data Transfer Objects (DTOs) for all endpoints in */dto/ directories to validate request payloads.” Based on learnings, apply the existing ValidationPipe+DTO security pattern consistently on new API endpoints.Also applies to: 48-49, 77-78, 105-106, 132-133, 192-193, 223-224, 253-254, 278-279, 380-387, 420-426
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/queues/queues.controller.ts` around lines 19 - 20, The controller is using inline body types which bypass class-based ValidationPipe; create proper DTO classes under a dto/ directory (e.g., CreateQueueDto, ListQueuesDto, UpdateQueueDto, BulkQueueActionDto) with class-validator decorators for required/optional fields (count:number, page?:number, year?:number, etc.), replace all inline `@Body`() parameter types in QueuesController methods with these DTO classes, update the method signatures and imports to use the new DTO types, and ensure any nested/array shapes are covered by corresponding DTOs so the global ValidationPipe enforces validation for the listed endpoints (lines referenced in the review).Sources: Coding guidelines, Learnings
apps/api/src/embeddings/embeddings.controller.ts (1)
17-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn HTTP 400 for invalid
idinstead of a 200 payload.Line 20 and Line 73 currently return
{ error: ... }, which still resolves as 200 OK. ThrowBadRequestExceptionto keep API semantics consistent for clients.Suggested patch
-import { Controller, Post, Param, Get, Logger, UseGuards } from '`@nestjs/common`'; +import { Controller, Post, Param, Get, Logger, UseGuards, BadRequestException } from '`@nestjs/common`'; ... const movieId = parseInt(id, 10); if (isNaN(movieId)) { - return { error: 'Invalid movie ID' }; + throw new BadRequestException('Invalid movie ID'); }Also applies to: 70-74
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/embeddings/embeddings.controller.ts` around lines 17 - 21, Replace the plain JSON error responses with HTTP 400 exceptions: in generateMovieEmbedding, instead of returning { error: 'Invalid movie ID' } when parseInt yields NaN, throw a BadRequestException with that message; do the same for the other endpoint that currently returns { error: ... } around lines 70-74 (the other embeddings handler) so both endpoints return proper HTTP 400 responses rather than 200 payloads.IMPROVEMENTS.md (1)
1680-1706:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLegacy priority section conflicts with the new P0–P4 source of truth.
Section
#14keeps an older active checklist that diverges from the revised prioritization (e.g., Line 1704). Mark it as archived or remove it to avoid planning drift.As per coding guidelines,
IMPROVEMENTS.mdmust remain the single source of truth and prioritize work via P0–P4.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@IMPROVEMENTS.md` around lines 1680 - 1706, Remove or archive the legacy priority block titled "## 14. 📝 Приоритетный список (обновлено 2026-01-24)" (the section referred to as `#14`) so the doc no longer contains a conflicting active checklist; either delete the entire section or prepend an "ARCHIVED" heading and a short note pointing readers to the current P0–P4 source of truth, and update any checklist items (e.g., the Language support and Node/Redis upgrade items) to be referenced or migrated into the P0–P4 list to avoid planning drift.Source: Coding guidelines
apps/web/lib/api/client.ts (1)
97-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop putting
user_idinto JWT-scoped URLs.The API controllers now derive identity from the Bearer token, but these methods still append
user_idto browser-visible URLs. That keeps leaking a stable identifier into history, logs, and analytics while leaving the web client out of sync with the hardened server contract. As per coding guidelines, "Use Supabase Auth for authentication in the web application. Extract user information from JWT tokens provided by Supabase Auth." Based on learnings, preserve and apply existing security patterns souser_idcomes from JWT-backed auth, not client-controlled request data.Suggested cleanup
- async getWatchlist(userId: string, status?: 'planned' | 'watched', language?: string): Promise<WatchlistItem[]> { - const statusParam = status ? `&status=${status}` : '' - const langParam = language ? `&language=${language}` : '' + async getWatchlist(status?: 'planned' | 'watched', language?: string): Promise<WatchlistItem[]> { + const params = new URLSearchParams() + if (status) params.set('status', status) + if (language) params.set('language', language) const response = await this.fetch<{ success: boolean; items: WatchlistItem[] }>( - `/api/watchlist?user_id=${userId}${statusParam}${langParam}` + `/api/watchlist${params.toString() ? `?${params.toString()}` : ''}` ) return response.items || [] } - async removeFromWatchlist(itemId: number, userId: string, content_type?: 'movie' | 'tv_show'): Promise<void> { - const contentTypeParam = content_type ? `&content_type=${content_type}` : '' - return this.fetch(`/api/watchlist/${itemId}?user_id=${userId}${contentTypeParam}`, { + async removeFromWatchlist(itemId: number): Promise<void> { + return this.fetch(`/api/watchlist/${itemId}`, { method: 'DELETE', }) } - async getRecommendations(userId: string, limit = 10, language?: string): Promise<Recommendation[]> { + async getRecommendations(limit = 10, language?: string): Promise<Recommendation[]> { const langParam = language ? `&language=${language}` : '' const response = await this.fetch<{ success: boolean; recommendations: Recommendation[] }>( - `/api/recommendations?user_id=${userId}&limit=${limit}${langParam}` + `/api/recommendations?limit=${limit}${langParam}` ) return response.recommendations || [] } - async getHybridRecommendations(userId: string, limit = 10, language?: string): Promise<Recommendation[]> { + async getHybridRecommendations(limit = 10, language?: string): Promise<Recommendation[]> { const langParam = language ? `&language=${language}` : '' const response = await this.fetch<{ success: boolean; recommendations: Recommendation[] }>( - `/api/recommendations/hybrid?user_id=${userId}&limit=${limit}${langParam}` + `/api/recommendations/hybrid?limit=${limit}${langParam}` ) return response.recommendations || [] }Also applies to: 122-126, 130-142
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/lib/api/client.ts` around lines 97 - 103, The getWatchlist method is leaking a client-controlled user_id in the URL; update getWatchlist to stop accepting or appending userId and call the JWT-scoped endpoint instead by removing `userId` from the signature/usages and calling this.fetch with `/api/watchlist${statusParam}${langParam}` (i.e., no `?user_id=...`), keep the existing status and language query params, and then update all callers of getWatchlist (and the other similar methods noted at 122-126 and 130-142) to stop passing a userId so the server derives identity from the Bearer token; keep references to the function name getWatchlist and the fetch call so you can locate and modify the code consistently.Sources: Coding guidelines, Learnings
🧹 Nitpick comments (6)
docs/PROJECT_REVIEW_2026-06-11.md (1)
17-20: ⚡ Quick winMark this document explicitly as a pre-hardening baseline snapshot.
These sections now conflict with the current security state in this PR (guards/throttling/ValidationPipe), so add a prominent “historical baseline” note and direct active prioritization to
IMPROVEMENTS.mdto avoid contradictory guidance.As per coding guidelines,
IMPROVEMENTS.mdis the single source of truth for project planning. Based on learnings,ROADMAP.mdandNEW_IMPROVEMENTS_SUMMARY.mdare archived and should not drive current prioritization.Also applies to: 74-77, 117-121
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/PROJECT_REVIEW_2026-06-11.md` around lines 17 - 20, Mark PROJECT_REVIEW_2026-06-11.md explicitly as a "pre-hardening baseline" by adding a prominent header/note at the top stating the document reflects a historical snapshot and is not the current security posture, and insert a clear directive that active prioritization and remediation decisions must be taken from IMPROVEMENTS.md (make IMPROVEMENTS.md the single source of truth). Update the sections in this same document that repeat these findings (the later review sections) to include the same baseline/archival note, and add an instruction linking tasks back to IMPROVEMENTS.md so triage and fixes (guards/throttling/ValidationPipe, key rotation, CI/tests, repo hygiene) are scheduled there; archive ROADMAP.md and NEW_IMPROVEMENTS_SUMMARY.md by adding an "Archived — do not use for prioritization" banner and remove them as drivers for current prioritization.Sources: Coding guidelines, Learnings
apps/web/lib/api/client.ts (1)
187-197: sendChatMessage still serializesuserId, but the backend strips/ignores it
apps/web/lib/api/types.ts:SendChatMessageParamsincludesuserId, andapps/web/lib/api/client.tspostsJSON.stringify(params)toPOST /api/chat(also passed fromapps/web/app/[locale]/chat/page.tsx).apps/api/src/chat/chat.controller.ts: identity is derived from@CurrentUser()and the request body usesSendChatMessageDto, which has nouserId;apps/api/src/main.tssetsValidationPipe({ whitelist: true }), so extra fields likeuserIdare stripped.- Recommend removing
userIdfrom the client-side payload/DTO to avoid sending unnecessary PII and prevent confusion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/lib/api/client.ts` around lines 187 - 197, sendChatMessage is serializing SendChatMessageParams (which currently includes userId) and POSTing it to /api/chat even though the server derives identity via `@CurrentUser`() and strips extra fields via ValidationPipe; remove userId from the client payload by updating the SendChatMessageParams type to exclude userId (or omit it when sending) and change sendChatMessage to JSON.stringify only the allowed fields (e.g., pick message, conversationId, etc.) before sending; also update callers (e.g., the chat page) to stop passing userId into sendChatMessage so no PII is included in the request.apps/api/src/auth/__tests__/supabase-auth.guard.spec.ts (1)
30-70: ⚡ Quick winAdd a test for rejected auth lookup to lock fail-closed behavior.
The suite does not cover
getUserpromise rejection (network/runtime failure), which is a distinct path from{ error }responses.Suggested test case
describe('SupabaseAuthGuard', () => { @@ it('rejects invalid or expired token', async () => { getUserMock.mockResolvedValue({ data: { user: null }, error: { message: 'invalid token' }, }); const { ctx } = contextWithAuth('Bearer expired-jwt'); await expect(guard.canActivate(ctx)).rejects.toThrow(UnauthorizedException); }); + + it('rejects when auth provider call rejects', async () => { + getUserMock.mockRejectedValue(new Error('network failure')); + const { ctx } = contextWithAuth('Bearer valid-jwt'); + await expect(guard.canActivate(ctx)).rejects.toThrow(UnauthorizedException); + }); });As per coding guidelines, TDD is mandatory for new API security behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/auth/__tests__/supabase-auth.guard.spec.ts` around lines 30 - 70, Add a new test in the SupabaseAuthGuard suite that simulates getUser throwing/rejecting (e.g., getUserMock.mockRejectedValue(new Error('network'))), calls canActivate with a valid Bearer token via contextWithAuth('Bearer some-jwt'), and asserts the guard rejects with UnauthorizedException and does not attach a user to the request; update the test file to reference SupabaseAuthGuard, getUserMock, contextWithAuth, and canActivate so the failure-to-resolve path (promise rejection) is covered and enforces fail-closed behavior.Source: Coding guidelines
apps/api/src/watchlist/dto/watchlist.dto.ts (2)
22-40: ⚡ Quick winSame identification validation applies here.
MarkAsWatchedDtohas the same all-optional identification fields asAddToWatchlistDto. If you implement cross-field validation forAddToWatchlistDto, apply the same pattern here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/watchlist/dto/watchlist.dto.ts` around lines 22 - 40, MarkAsWatchedDto currently has all-identification fields optional like AddToWatchlistDto; implement the same cross-field validation used for AddToWatchlistDto (e.g., a class-level validator or a custom decorator) to enforce that exactly one valid identifier is provided (movie_id OR content_type+content_id as required by your rules). Update MarkAsWatchedDto to use the same validator symbol or class-level constraint you added for AddToWatchlistDto so the same validation logic (including error messages) is applied consistently.
3-20: ⚡ Quick winConsider validating that at least one identification method is provided.
All fields are optional, allowing an empty payload to pass DTO validation. The service layer must handle this, but adding a custom validator ensuring either
movie_idOR (content_typeANDcontent_id) would catch invalid payloads earlier with clearer error messages.♻️ Optional: Add a custom validation constraint
+import { ValidateBy, ValidationOptions, buildMessage } from 'class-validator'; + +function HasIdentification(validationOptions?: ValidationOptions) { + return ValidateBy( + { + name: 'hasIdentification', + validator: { + validate(value, args): boolean { + const obj = args?.object as any; + return !!obj.movie_id || !!(obj.content_type && obj.content_id); + }, + defaultMessage: buildMessage( + () => 'Either movie_id or (content_type and content_id) must be provided', + validationOptions + ), + }, + }, + validationOptions + ); +} + +@HasIdentification() export class AddToWatchlistDto {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/watchlist/dto/watchlist.dto.ts` around lines 3 - 20, Add a class-level validation to AddToWatchlistDto so empty payloads are rejected: implement a custom validator (or class-validator `@ValidateIf-style` constraint) on AddToWatchlistDto that enforces that either movie_id is present OR both content_type and content_id are present, and return a clear message when neither identification method is provided; update the DTO to reference this validator so validation fails early for payloads missing movie_id and/or content_type+content_id.apps/api/src/queues/dto/schedule-rotational-import.dto.ts (1)
4-7: ⚡ Quick winConsider stricter cron validation for better error messages.
The current regex validates structure (5 space-separated tokens) but accepts semantically invalid expressions like
"foo bar baz qux quux". While BullMQ will catch this at scheduling time, stricter validation would provide earlier, clearer feedback.♻️ Optional: Use a cron validation library
-import { IsInt, IsOptional, Matches, Max, Min } from 'class-validator'; +import { IsInt, IsOptional, Max, Min } from 'class-validator'; +import { IsValidCron } from './validators/is-valid-cron.decorator'; -/** Standard 5-field cron: minute hour day-of-month month day-of-week */ -const CRON_5_FIELDS = /^(\S+\s+){4}\S+$/; export class ScheduleRotationalImportDto { - `@Matches`(CRON_5_FIELDS, { message: 'cronExpression must be a 5-field cron string' }) + `@IsValidCron`({ message: 'cronExpression must be a valid 5-field cron string' }) cronExpression!: string;Then create a custom validator using
cron-parseror similar library to validate actual cron semantics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/queues/dto/schedule-rotational-import.dto.ts` around lines 4 - 7, The current Matches validator on ScheduleRotationalImportDto uses CRON_5_FIELDS which only checks five space-separated tokens and allows semantically invalid crons; replace the simple regex check with a stricter validation by implementing a custom class-validator constraint (e.g., CronExpressionValidator) that uses a cron parsing library such as cron-parser to parse the cronExpression and return false on parse errors, then annotate the cron property with `@Validate`(CronExpressionValidator) (or similar) instead of Matches so invalid cron semantics are caught early and produce a clear error message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/app.module.ts`:
- Around line 27-31: ThrottlerModule.forRoot currently calls parseInt on
RATE_LIMIT_WINDOW_MS and RATE_LIMIT_MAX_REQUESTS directly which can yield NaN;
change this to parse and validate the values before passing them to
ThrottlerModule.forRoot: parse each env var (RATE_LIMIT_WINDOW_MS,
RATE_LIMIT_MAX_REQUESTS) into a number, check Number.isFinite() / isNaN() (or
fallback when the parsed value is not a valid positive integer), and supply safe
defaults (e.g., 60000 for ttl and 100 for limit) so ttl and limit are always
valid integers when passed into ThrottlerModule.forRoot.
In `@apps/api/src/auth/current-user.decorator.ts`:
- Around line 8-11: Replace the generic runtime Error thrown when request.user
is missing in the current-user.decorator (the guard-misconfiguration branch that
says "CurrentUser used without SupabaseAuthGuard") with Nest's
UnauthorizedException so the failure yields a 401 instead of a 500; update the
decorator file to import UnauthorizedException from `@nestjs/common` and throw new
UnauthorizedException(...) (including a short message) in the branch where
request.user is falsy and reference SupabaseAuthGuard in the message for
clarity.
In `@apps/api/src/auth/supabase-auth.guard.ts`:
- Around line 26-35: The guard currently slices the Authorization header into
token even when it's empty and calls supabaseAnon.auth.getUser; update the
Authorization handling in supabase-auth.guard.ts (SupabaseAuthGuard) to trim the
bearer token, validate that the resulting token is non-empty/non-whitespace
before calling supabaseAnon.auth.getUser, and throw the same
UnauthorizedException('Missing bearer token') if the token is blank—do the
change inline around the existing check (authorization.startsWith('Bearer '),
token extraction and the getUser call) without adding extra typing or
refactoring.
In `@apps/api/src/chat/chat.controller.ts`:
- Around line 37-38: Replace the hardcoded `@Throttle`({ default: { ttl: 60000,
limit: 10 } }) in chat.controller.ts with values driven from environment
variables (e.g., RATE_LIMIT_CHAT_TTL and RATE_LIMIT_CHAT_LIMIT), parsing them
into numbers and falling back to sensible defaults (ttl = 60 seconds, limit =
10). Update the `@Throttle` decorator on the ChatController / the method where it
appears to use Number(process.env.RATE_LIMIT_CHAT_TTL ?? 60) and
Number(process.env.RATE_LIMIT_CHAT_LIMIT ?? 10) so ops can tune chat rate limits
without rebuilding.
In `@apps/api/src/recommendations/recommendations.controller.ts`:
- Around line 157-167: Create a DTO class UpdateUserProfileDto in a dto/ folder
(e.g., UpdateUserProfileDto with min_rating?: number decorated with
class-validator: `@IsOptional`(), `@IsInt`(), `@Min`(0)/@Max(10) as appropriate) and
update the controller method updateUserProfile to accept `@Body`() dto:
UpdateUserProfileDto (and use dto.min_rating ?? 7 when calling
recommendationsService.updateUserProfile). Do the same for the me/language
endpoint: add a LanguageDto in dto/ and replace its inline body type with that
DTO. Ensure the module uses the ValidationPipe/transform settings so DTOs are
validated/transformed consistently.
In `@apps/api/src/scripts/setup-cron.ts`:
- Around line 83-85: Make ADMIN_API_KEY mandatory in setup mode and fail-closed:
in apps/api/src/scripts/setup-cron.ts ensure the header 'X-Admin-Api-Key' is
always included when running the --setup flow (remove the conditional spread
around process.env.ADMIN_API_KEY so the header is required), validate at startup
that process.env.ADMIN_API_KEY is present and exit non-zero with a clear error
if missing, and modify the schedule request logic (the function(s) performing
the fetch/POSTs for creating schedules—e.g., the code block that builds headers
and calls the schedule endpoints around lines 83-106 and 175-179) to check each
response and on any non-2xx response log the error and process.exit(1) so the
script reports failure if any schedule request fails.
In `@CURRENT_STATUS.md`:
- Around line 10-16: Update the "Security hardening" snapshot in
CURRENT_STATUS.md to reflect the changes introduced by this PR: change the
progress bar/percentage for "Security hardening" from 0% to the appropriate
completed percent, remove or revise the note "нет guards/throttler/validation;
ключи не ротированы", and replace the old "next steps" content on the referenced
blocks (lines around "Security hardening" and the sections noted 37–44 and
49–52) with accurate post-hardening status and remaining action items (e.g.,
completed guards/throttler/validation, key rotation status, and any follow-up
tasks).
- Around line 9-17: The fenced code block starting with triple backticks and the
lines beginning "MVP (Day 0-14): ..." is missing a language identifier (MD040);
update the opening fence from ``` to ```text (or another appropriate language
tag) so the block is properly annotated, e.g., change the line that reads ``` to
```text to satisfy the linter.
In `@PRODUCTION-QUICKSTART.md`:
- Around line 26-37: Update the unauthenticated curl examples for the queue
endpoints (/api/queues/schedule-rotational-movie-import,
/api/queues/schedule-rotational-tv-import, /api/queues/rotation-status) to
include the required X-Admin-Api-Key header; add a single reusable snippet
instruction at the top (e.g., set PROD_URL and ADMIN_KEY once) and reference
those variables in each curl example so all queue/embedding templates
consistently send X-Admin-Api-Key and avoid future drift.
In `@PRODUCTION-SETUP.md`:
- Around line 229-231: The fenced code block that currently contains "URL:
https://api-production-9141.up.railway.app/api/queues/stats" is missing a
language identifier; update the triple-backtick fence to include a language (for
example ```text or ```bash) so the markdown linter is satisfied and the block is
properly highlighted/validated.
- Around line 380-383: Documentation references POST /api/queues/schedule-clean
but the QueuesController only exposes POST /api/queues/clean; either add a new
handler in QueuesController named scheduleClean (or similar) that registers POST
/api/queues/schedule-clean and delegates to the existing cleaning logic, or
update PRODUCTION-SETUP.md to call the existing POST /api/queues/clean endpoint;
locate the controller methods (QueuesController and its clean handler) and
ensure route names match between code and docs.
In `@setup-production.sh`:
- Line 55: Change all interactive prompt reads to use the -r flag to prevent
backslash interpretation: replace occurrences like read -p "Select option (0-7):
" option with read -r -p "Select option (0-7): " option and do the same for the
other prompt lines referenced (the reads at the blocks around lines 115,
139-140, and 150-151); update each read invocation (e.g., the variable names
used there) so they consistently use read -r -p ... to safely capture raw input.
- Around line 25-31: The preflight curl check currently only verifies
connectivity and not the HTTP status, so 401/403 responses still count as
success; update the curl invocation (the if block using curl -s -H
"X-Admin-Api-Key: $ADMIN_API_KEY" --max-time 5 "$PROD_URL/api/queues/stats") to
capture the HTTP status (e.g., using -w "%{http_code}" and --output /dev/null)
and explicitly test that the returned status equals 200 before printing
"Production API is accessible" and continuing; on non-200 statuses include the
status code in the error message and exit nonzero.
---
Outside diff comments:
In `@apps/api/src/embeddings/embeddings.controller.ts`:
- Around line 17-21: Replace the plain JSON error responses with HTTP 400
exceptions: in generateMovieEmbedding, instead of returning { error: 'Invalid
movie ID' } when parseInt yields NaN, throw a BadRequestException with that
message; do the same for the other endpoint that currently returns { error: ...
} around lines 70-74 (the other embeddings handler) so both endpoints return
proper HTTP 400 responses rather than 200 payloads.
In `@apps/api/src/queues/queues.controller.ts`:
- Around line 307-315: The response messages are interpolating body.count which
can be undefined when defaults are applied; compute the resolved count locally
(e.g., const resolvedCount = body.count ?? 100) before calling
this.queuesService.scheduleRotationalMovieImport and use resolvedCount in the
returned message; do the same for the rotational series endpoint (use the same
pattern with the default 80 for the scheduleRotationalSeriesImport call) so the
responses show the effective count rather than possibly "undefined".
- Around line 19-20: The controller is using inline body types which bypass
class-based ValidationPipe; create proper DTO classes under a dto/ directory
(e.g., CreateQueueDto, ListQueuesDto, UpdateQueueDto, BulkQueueActionDto) with
class-validator decorators for required/optional fields (count:number,
page?:number, year?:number, etc.), replace all inline `@Body`() parameter types in
QueuesController methods with these DTO classes, update the method signatures
and imports to use the new DTO types, and ensure any nested/array shapes are
covered by corresponding DTOs so the global ValidationPipe enforces validation
for the listed endpoints (lines referenced in the review).
In `@apps/web/lib/api/client.ts`:
- Around line 97-103: The getWatchlist method is leaking a client-controlled
user_id in the URL; update getWatchlist to stop accepting or appending userId
and call the JWT-scoped endpoint instead by removing `userId` from the
signature/usages and calling this.fetch with
`/api/watchlist${statusParam}${langParam}` (i.e., no `?user_id=...`), keep the
existing status and language query params, and then update all callers of
getWatchlist (and the other similar methods noted at 122-126 and 130-142) to
stop passing a userId so the server derives identity from the Bearer token; keep
references to the function name getWatchlist and the fetch call so you can
locate and modify the code consistently.
In `@IMPROVEMENTS.md`:
- Around line 1680-1706: Remove or archive the legacy priority block titled "##
14. 📝 Приоритетный список (обновлено 2026-01-24)" (the section referred to as
`#14`) so the doc no longer contains a conflicting active checklist; either
delete the entire section or prepend an "ARCHIVED" heading and a short note
pointing readers to the current P0–P4 source of truth, and update any checklist
items (e.g., the Language support and Node/Redis upgrade items) to be referenced
or migrated into the P0–P4 list to avoid planning drift.
---
Nitpick comments:
In `@apps/api/src/auth/__tests__/supabase-auth.guard.spec.ts`:
- Around line 30-70: Add a new test in the SupabaseAuthGuard suite that
simulates getUser throwing/rejecting (e.g., getUserMock.mockRejectedValue(new
Error('network'))), calls canActivate with a valid Bearer token via
contextWithAuth('Bearer some-jwt'), and asserts the guard rejects with
UnauthorizedException and does not attach a user to the request; update the test
file to reference SupabaseAuthGuard, getUserMock, contextWithAuth, and
canActivate so the failure-to-resolve path (promise rejection) is covered and
enforces fail-closed behavior.
In `@apps/api/src/queues/dto/schedule-rotational-import.dto.ts`:
- Around line 4-7: The current Matches validator on ScheduleRotationalImportDto
uses CRON_5_FIELDS which only checks five space-separated tokens and allows
semantically invalid crons; replace the simple regex check with a stricter
validation by implementing a custom class-validator constraint (e.g.,
CronExpressionValidator) that uses a cron parsing library such as cron-parser to
parse the cronExpression and return false on parse errors, then annotate the
cron property with `@Validate`(CronExpressionValidator) (or similar) instead of
Matches so invalid cron semantics are caught early and produce a clear error
message.
In `@apps/api/src/watchlist/dto/watchlist.dto.ts`:
- Around line 22-40: MarkAsWatchedDto currently has all-identification fields
optional like AddToWatchlistDto; implement the same cross-field validation used
for AddToWatchlistDto (e.g., a class-level validator or a custom decorator) to
enforce that exactly one valid identifier is provided (movie_id OR
content_type+content_id as required by your rules). Update MarkAsWatchedDto to
use the same validator symbol or class-level constraint you added for
AddToWatchlistDto so the same validation logic (including error messages) is
applied consistently.
- Around line 3-20: Add a class-level validation to AddToWatchlistDto so empty
payloads are rejected: implement a custom validator (or class-validator
`@ValidateIf-style` constraint) on AddToWatchlistDto that enforces that either
movie_id is present OR both content_type and content_id are present, and return
a clear message when neither identification method is provided; update the DTO
to reference this validator so validation fails early for payloads missing
movie_id and/or content_type+content_id.
In `@apps/web/lib/api/client.ts`:
- Around line 187-197: sendChatMessage is serializing SendChatMessageParams
(which currently includes userId) and POSTing it to /api/chat even though the
server derives identity via `@CurrentUser`() and strips extra fields via
ValidationPipe; remove userId from the client payload by updating the
SendChatMessageParams type to exclude userId (or omit it when sending) and
change sendChatMessage to JSON.stringify only the allowed fields (e.g., pick
message, conversationId, etc.) before sending; also update callers (e.g., the
chat page) to stop passing userId into sendChatMessage so no PII is included in
the request.
In `@docs/PROJECT_REVIEW_2026-06-11.md`:
- Around line 17-20: Mark PROJECT_REVIEW_2026-06-11.md explicitly as a
"pre-hardening baseline" by adding a prominent header/note at the top stating
the document reflects a historical snapshot and is not the current security
posture, and insert a clear directive that active prioritization and remediation
decisions must be taken from IMPROVEMENTS.md (make IMPROVEMENTS.md the single
source of truth). Update the sections in this same document that repeat these
findings (the later review sections) to include the same baseline/archival note,
and add an instruction linking tasks back to IMPROVEMENTS.md so triage and fixes
(guards/throttling/ValidationPipe, key rotation, CI/tests, repo hygiene) are
scheduled there; archive ROADMAP.md and NEW_IMPROVEMENTS_SUMMARY.md by adding an
"Archived — do not use for prioritization" banner and remove them as drivers for
current prioritization.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b9334ba9-97c8-43c7-98bf-3da9f2a78ec7
⛔ Files ignored due to path filters (8)
api.logis excluded by!**/*.logapi_full.logis excluded by!**/*.logapi_local.logis excluded by!**/*.logapi_startup.logis excluded by!**/*.logapps/api/api.logis excluded by!**/*.logapps/web/web.logis excluded by!**/*.logpnpm-lock.yamlis excluded by!**/pnpm-lock.yamlweb.logis excluded by!**/*.log
📒 Files selected for processing (40)
.env.example.gitignoreCLAUDE.mdCURRENT_STATUS.mdIMPROVEMENTS.mdNEW_IMPROVEMENTS_SUMMARY.mdPRODUCTION-QUICKSTART.mdPRODUCTION-SETUP.mdROADMAP.mdSESSION_RESUME.mdapps/api/package.jsonapps/api/src/app.module.tsapps/api/src/auth/__tests__/admin-api-key.guard.spec.tsapps/api/src/auth/__tests__/supabase-auth.guard.spec.tsapps/api/src/auth/admin-api-key.guard.tsapps/api/src/auth/current-user.decorator.tsapps/api/src/auth/supabase-auth.guard.tsapps/api/src/chat/chat.controller.tsapps/api/src/chat/dto/send-chat-message.dto.tsapps/api/src/common/dto/__tests__/dto-validation.spec.tsapps/api/src/embeddings/embeddings.controller.tsapps/api/src/main.tsapps/api/src/movies/movies.controller.tsapps/api/src/queues/dto/schedule-rotational-import.dto.tsapps/api/src/queues/queues.controller.tsapps/api/src/recommendations/recommendations.controller.tsapps/api/src/scripts/setup-cron.tsapps/api/src/tmdb/tmdb.controller.tsapps/api/src/tv-shows/tv-shows.controller.tsapps/api/src/users/users.controller.tsapps/api/src/watchlist/dto/watchlist.dto.tsapps/api/src/watchlist/watchlist.controller.tsapps/web/lib/api/client.tsdocs/KEY_ROTATION_CHECKLIST.mddocs/PROJECT_REVIEW_2026-06-11.mdscripts/import-movies-bulk.shsetup-production.shsupabase/migrations/20251228000001_add_release_date_to_functions.sqlsupabase/migrations/20251228000002_fix_match_movies_signature.sqlsupabase/migrations/20260505000001_restore_match_translations.sql
- NaN-safe env parsing (parsePositiveIntEnv) for throttler config, TDD-covered - CurrentUser decorator: 401 instead of 500 when guard is missing (fail closed) - SupabaseAuthGuard: reject blank bearer token before calling Supabase - chat throttle tunable via RATE_LIMIT_CHAT_TTL_MS / RATE_LIMIT_CHAT_MAX_REQUESTS (ms semantics — reviewer's suggested default of 60 would be 60ms) - DTOs for recommendations update-profile and users me/language - setup-production.sh: preflight requires HTTP 200 (401/403 now fail), read -r - docs: admin header + $PROD_URL in all QUICKSTART curl examples, fenced block language, schedule-clean replaced with existing /api/queues/clean endpoint Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.