From 439e74d037e09c95b222488243bd7a2df91b3b84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Yusuf=20=C4=B0pek?= Date: Fri, 10 Apr 2026 20:38:22 +0300 Subject: [PATCH] feat(validation): implement validateAnnotationStrokes function for safe annotation data handling feat(rate-limit): add TRUSTED_PROXY_MODE for configurable proxy header trust feat(comments): validate annotation data structure in comment routes and components --- .env.example | 8 +++ app/api/comments/[commentId]/route.ts | 13 ++++- .../versions/[versionId]/comments/route.ts | 16 +++++- components/video-page-content.tsx | 7 ++- .../video-page/hooks/use-comment-actions.ts | 5 +- .../video-page/hooks/use-video-player.ts | 6 +- lib/rate-limit.ts | 57 ++++++++++++------- lib/validation.ts | 51 +++++++++++++++++ 8 files changed, 134 insertions(+), 29 deletions(-) diff --git a/.env.example b/.env.example index c0ac923..d4c9085 100644 --- a/.env.example +++ b/.env.example @@ -74,6 +74,14 @@ NODE_ENV="development" # Disable all app-level rate limiting for local testing. Leave unset to keep rate limiting enabled. # Accepted truthy values: "true", "1", "yes", "on" # DISABLE_RATE_LIMIT="true" + +# Trusted reverse proxy mode — controls which headers getClientIp() trusts for rate limiting. +# Set this only when you have confirmed that your proxy strips/overwrites client-supplied headers. +# cloudflare — trust cf-connecting-ip (Cloudflare edge in front of the origin) +# nginx — trust x-real-ip / last x-forwarded-for (Nginx real_ip_header with set_real_ip_from) +# Leave unset for local dev or when no trusted proxy is in place. +TRUSTED_PROXY_MODE="cloudflare" + # Admin emails for accessing the /admin panel (comma separated list) # e.g., "yusuf@example.com,admin@example.com" ADMIN_EMAILS="" diff --git a/app/api/comments/[commentId]/route.ts b/app/api/comments/[commentId]/route.ts index 8c85c27..c0e03f2 100644 --- a/app/api/comments/[commentId]/route.ts +++ b/app/api/comments/[commentId]/route.ts @@ -9,6 +9,7 @@ import { getShareSessionFromRequest } from '@/lib/share-session'; import { apiErrors, successResponse, withCacheControl } from '@/lib/api-response'; import { getGuestIdentityFromRequest } from '@/lib/guest-identity'; import { runWithConcurrency } from '@/lib/async-pool'; +import { validateAnnotationStrokes } from '@/lib/validation'; const CLEANUP_DELETE_CONCURRENCY = 5; @@ -172,7 +173,17 @@ export async function PATCH(request: NextRequest, { params }: RouteParams) { const updateData: Record = {}; if (content !== undefined && typeof content === 'string') updateData.content = content.trim(); if (tagId !== undefined) updateData.tagId = tagId; - if (annotationData !== undefined) updateData.annotationData = annotationData; + if (annotationData !== undefined) { + if (annotationData === null) { + updateData.annotationData = null; + } else { + const validStrokes = validateAnnotationStrokes(annotationData); + if (validStrokes === null) { + return apiErrors.badRequest('annotationData must be an array of valid stroke objects'); + } + updateData.annotationData = JSON.stringify(validStrokes); + } + } if (isResolved !== undefined) { updateData.isResolved = isResolved; updateData.resolvedAt = isResolved ? new Date() : null; diff --git a/app/api/versions/[versionId]/comments/route.ts b/app/api/versions/[versionId]/comments/route.ts index 7fa5ad8..db43e7e 100644 --- a/app/api/versions/[versionId]/comments/route.ts +++ b/app/api/versions/[versionId]/comments/route.ts @@ -10,6 +10,7 @@ import { HeadObjectCommand } from '@aws-sdk/client-s3'; import { r2Client, R2_BUCKET_NAME } from '@/lib/r2'; import { ensureGuestIdentityFromRequest, getGuestIdentityFromRequest, setGuestIdentityCookie } from '@/lib/guest-identity'; import { extractImageFileNameFromProxyUrl, sanitizeAssetDisplayName } from '@/lib/video-assets'; +import { validateAnnotationStrokes } from '@/lib/validation'; type RouteParams = { params: Promise<{ versionId: string }> }; const SAFE_IMAGE_PATH = /^\/api\/upload\/image\/[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\.[a-z0-9]+$/i; @@ -259,8 +260,17 @@ export async function POST(request: NextRequest, { params }: RouteParams) { if (guestName !== undefined && guestName !== null && String(guestName).length > 100) { return apiErrors.badRequest('Guest name must be 100 characters or fewer'); } - if (annotationData !== undefined && annotationData !== null && JSON.stringify(annotationData).length > 50_000) { - return apiErrors.badRequest('Annotation data is too large'); + + // Validate annotation data structure to prevent prototype pollution and stored XSS. + // Reject anything that is not a well-formed array of AnnotationStroke objects. + let serializedAnnotationData: string | null = null; + if (annotationData !== undefined && annotationData !== null) { + const validStrokes = validateAnnotationStrokes(annotationData); + if (validStrokes === null) { + return apiErrors.badRequest('annotationData must be an array of valid stroke objects'); + } + // Re-serialize to canonical JSON — strips any extra properties from the input. + serializedAnnotationData = JSON.stringify(validStrokes); } // If replying, verify parent exists in same version @@ -309,7 +319,7 @@ export async function POST(request: NextRequest, { params }: RouteParams) { voiceUrl: voiceUrl || null, voiceDuration: voiceDuration || null, imageUrl: imageUrl || null, - annotationData: annotationData || null, + annotationData: serializedAnnotationData, authorId: session?.user?.id || null, guestName: isGuest ? guestName : null, guestEmail: isGuest ? guestEmail : null, diff --git a/components/video-page-content.tsx b/components/video-page-content.tsx index 3b09bec..5898d11 100644 --- a/components/video-page-content.tsx +++ b/components/video-page-content.tsx @@ -13,6 +13,7 @@ import { VideoPageLoading } from '@/components/video-page/video-page-loading'; import { VideoPageError } from '@/components/video-page/video-page-error'; import { GuestNameGate } from '@/components/video-page/guest-name-gate'; import { useCommentMedia } from '@/components/video-page/hooks/use-comment-media'; +import { validateAnnotationStrokes } from '@/lib/validation'; import { useVersionActions } from '@/components/video-page/hooks/use-version-actions'; import { useWatchProgress } from '@/components/video-page/hooks/use-watch-progress'; import { useVideoPlayer } from '@/components/video-page/hooks/use-video-player'; @@ -498,7 +499,8 @@ export function VideoPageContent({ const editAnnotationInitialStrokes = useMemo(() => { if (editAnnotationData) { try { - return JSON.parse(editAnnotationData) as AnnotationStroke[]; + const parsed = JSON.parse(editAnnotationData); + return (validateAnnotationStrokes(parsed) as AnnotationStroke[] | null) ?? undefined; } catch { return undefined; } @@ -507,7 +509,8 @@ export function VideoPageContent({ const editingComment = comments.find((comment) => comment.id === editingCommentId); if (!editingComment?.annotationData) return undefined; try { - return JSON.parse(editingComment.annotationData) as AnnotationStroke[]; + const parsed = JSON.parse(editingComment.annotationData); + return (validateAnnotationStrokes(parsed) as AnnotationStroke[] | null) ?? undefined; } catch { return undefined; } diff --git a/components/video-page/hooks/use-comment-actions.ts b/components/video-page/hooks/use-comment-actions.ts index cf50d4c..b73a02d 100644 --- a/components/video-page/hooks/use-comment-actions.ts +++ b/components/video-page/hooks/use-comment-actions.ts @@ -16,6 +16,7 @@ import { toast } from 'sonner'; import type { AnnotationCanvasHandle, AnnotationStroke } from '@/components/annotation-canvas'; import type { Comment, CommentActionsConfig, CommentTag, Version, VideoData } from '@/components/video-page/types'; import { extractPastedImageFile, validateImageFile } from '@/components/video-page/image-upload-utils'; +import { validateAnnotationStrokes } from '@/lib/validation'; interface UseCommentActionsParams extends CommentActionsConfig { setVideo: Dispatch>; @@ -867,7 +868,9 @@ export function useCommentActions({ setIsEditingAnnotation(false); if (finalAnnotationData !== undefined && finalAnnotationData) { try { - setViewingAnnotation(JSON.parse(finalAnnotationData)); + const parsed = JSON.parse(finalAnnotationData); + const safe = validateAnnotationStrokes(parsed); + if (safe) setViewingAnnotation(safe as AnnotationStroke[]); } catch { // ignore parse errors } diff --git a/components/video-page/hooks/use-video-player.ts b/components/video-page/hooks/use-video-player.ts index 84feedd..69a314e 100644 --- a/components/video-page/hooks/use-video-player.ts +++ b/components/video-page/hooks/use-video-player.ts @@ -11,6 +11,7 @@ import type { PlayerAdapter, Version, } from '@/components/video-page/types'; +import { validateAnnotationStrokes } from '@/lib/validation'; interface UseVideoPlayerParams { activeVersion: Version | undefined; @@ -765,8 +766,9 @@ export function useVideoPlayer({ } if (annotation) { try { - const strokes = JSON.parse(annotation) as AnnotationStroke[]; - setViewingAnnotation(strokes); + const parsed = JSON.parse(annotation); + const safe = validateAnnotationStrokes(parsed); + setViewingAnnotation(safe as AnnotationStroke[] | null); } catch { setViewingAnnotation(null); } diff --git a/lib/rate-limit.ts b/lib/rate-limit.ts index b1b113a..c1fbcf1 100644 --- a/lib/rate-limit.ts +++ b/lib/rate-limit.ts @@ -150,33 +150,50 @@ function isPlausibleIp(value: string): boolean { /** * Get client IP from request headers. * - * Header priority: - * 1. cf-connecting-ip — set by Cloudflare (trusted proxy); cannot be spoofed by clients - * 2. x-forwarded-for — first entry, trusted only behind a proxy that overwrites it - * 3. x-real-ip — set by some reverse proxies (Nginx) - * 4. 127.0.0.1 — local development fallback + * Trusting proxy-injected headers is only safe when a known trusted proxy sits in front + * of this server and strips or overwrites those headers before forwarding requests. + * Set TRUSTED_PROXY_MODE to opt in: * - * Deployed behind Cloudflare, so cf-connecting-ip is the canonical source. + * TRUSTED_PROXY_MODE=cloudflare — trust cf-connecting-ip (Cloudflare edge) + * TRUSTED_PROXY_MODE=nginx — trust x-real-ip / x-forwarded-for (Nginx real_ip_header) + * + * Without TRUSTED_PROXY_MODE set, no proxy headers are trusted: all requests appear + * as 127.0.0.1, which means rate limits apply per-process rather than per-client IP. + * In that configuration, prefer session/user-keyed rate limits for authenticated endpoints. + * + * WARNING: Do not set TRUSTED_PROXY_MODE unless you have confirmed that your proxy + * strips or overwrites the corresponding headers on every inbound request. Failing to + * do so allows clients to spoof their IP and bypass rate limits. */ export function getClientIp(request: Request): string { - // Cloudflare always sets this to the true client IP - const cfIp = request.headers.get('cf-connecting-ip'); - if (cfIp && isPlausibleIp(cfIp)) { - return cfIp; + const mode = process.env.TRUSTED_PROXY_MODE?.trim().toLowerCase(); + + if (mode === 'cloudflare') { + // cf-connecting-ip is injected by Cloudflare and cannot be set by clients + // when origin access is restricted to Cloudflare's IP ranges. + const cfIp = request.headers.get('cf-connecting-ip'); + if (cfIp && isPlausibleIp(cfIp)) { + return cfIp; + } } - const forwardedFor = request.headers.get('x-forwarded-for'); - if (forwardedFor) { - const first = forwardedFor.split(',')[0].trim(); - if (isPlausibleIp(first)) return first; + if (mode === 'nginx') { + // x-real-ip is set by Nginx's real_ip_header directive (connection-level, not spoofable + // by clients when set_real_ip_from is configured for the upstream proxy). + const realIp = request.headers.get('x-real-ip'); + if (realIp && isPlausibleIp(realIp)) return realIp; + + // x-forwarded-for last entry added by Nginx when proxy_add_x_forwarded_for is used. + const forwardedFor = request.headers.get('x-forwarded-for'); + if (forwardedFor) { + const entries = forwardedFor.split(','); + const last = entries[entries.length - 1].trim(); + if (isPlausibleIp(last)) return last; + } } - const realIp = request.headers.get('x-real-ip'); - if (realIp && isPlausibleIp(realIp)) { - return realIp; - } - - // Fallback for local development + // No trusted proxy configured — fall back to a constant value. + // Rate limiting will apply per-process; use userId-keyed limits for authenticated endpoints. return '127.0.0.1'; } diff --git a/lib/validation.ts b/lib/validation.ts index 4871aed..6ab17a0 100644 --- a/lib/validation.ts +++ b/lib/validation.ts @@ -11,6 +11,57 @@ export function isValidHttpUrl(urlString: string): boolean { } } +// Matches exactly 6-digit hex colours produced by the annotation canvas (e.g. #FF3B30) +const ANNOTATION_COLOR_RE = /^#[0-9a-fA-F]{6}$/; +const MAX_STROKES = 500; +const MAX_POINTS_PER_STROKE = 2000; +const MIN_STROKE_WIDTH = 1; +const MAX_STROKE_WIDTH = 20; + +/** + * Validates and returns a safe copy of annotation stroke data. + * + * Accepts only an array of plain stroke objects with the exact shape created + * by AnnotationCanvas. Rejects anything that could trigger prototype pollution + * or carry unexpected properties into the renderer. + * + * Returns null when the input is absent or structurally invalid. + */ +export function validateAnnotationStrokes( + data: unknown +): { points: { x: number; y: number }[]; color: string; width: number }[] | null { + if (data === null || data === undefined) return null; + if (!Array.isArray(data)) return null; + if (data.length > MAX_STROKES) return null; + + const result: { points: { x: number; y: number }[]; color: string; width: number }[] = []; + + for (const stroke of data) { + if (stroke === null || typeof stroke !== 'object' || Array.isArray(stroke)) return null; + + const { points, color, width } = stroke as Record; + + if (!Array.isArray(points)) return null; + if (points.length > MAX_POINTS_PER_STROKE) return null; + + const safePoints: { x: number; y: number }[] = []; + for (const pt of points) { + if (pt === null || typeof pt !== 'object' || Array.isArray(pt)) return null; + const { x, y } = pt as Record; + if (typeof x !== 'number' || !isFinite(x)) return null; + if (typeof y !== 'number' || !isFinite(y)) return null; + safePoints.push({ x, y }); + } + + if (typeof color !== 'string' || !ANNOTATION_COLOR_RE.test(color)) return null; + if (typeof width !== 'number' || width < MIN_STROKE_WIDTH || width > MAX_STROKE_WIDTH) return null; + + result.push({ points: safePoints, color, width }); + } + + return result; +} + /** * Validates a URL and returns an error message if invalid */