Address PR review comments - security and code quality fixes

mchestr · mchestr · commit 317f84610663 · 2025-12-27T23:48:40.000-08:00
- Add Zod validation for Jellyfin credentials (Critical #2) - Sanitize and validate email inputs for account linking (Critical #3) - Replace console.log with centralized logger (Important #5) - Replace type assertions with Zod runtime validation (Minor #9) - Enhance error handling in Jellyfin sign-in (Important #4) All critical and important issues from code review have been addressed. Tests passing, linting clean.
diff --git a/actions/onboarding.ts b/actions/onboarding.ts
@@ -5,9 +5,19 @@ import { prisma } from "@/lib/prisma"
 import { createLogger } from "@/lib/utils/logger"
 import type { AuthService } from "@/types/onboarding"
 import { getServerSession } from "next-auth"
+import { z } from "zod"
 
 const logger = createLogger("ONBOARDING")
 
+/**
+ * Zod schema for onboarding status
+ * Validates the onboardingStatus JSON field from the database
+ */
+const OnboardingStatusSchema = z.object({
+  plex: z.boolean(),
+  jellyfin: z.boolean(),
+})
+
 interface OnboardingStatusRecord {
   plex: boolean
   jellyfin: boolean
@@ -32,10 +42,10 @@ export async function getOnboardingStatus(service?: AuthService) {
       },
     })
 
-    const status = (user?.onboardingStatus as unknown as OnboardingStatusRecord) || {
-      plex: false,
-      jellyfin: false,
-    }
+    const validation = OnboardingStatusSchema.safeParse(user?.onboardingStatus)
+    const status: OnboardingStatusRecord = validation.success
+      ? validation.data
+      : { plex: false, jellyfin: false }
 
     // If no service specified, use primary auth service
     const targetService: AuthService = service || (user?.primaryAuthService as AuthService) || "plex"
@@ -68,10 +78,10 @@ export async function completeOnboarding(service: AuthService) {
       select: { onboardingStatus: true },
     })
 
-    const currentStatus = (user?.onboardingStatus as unknown as OnboardingStatusRecord) || {
-      plex: false,
-      jellyfin: false,
-    }
+    const validation = OnboardingStatusSchema.safeParse(user?.onboardingStatus)
+    const currentStatus: OnboardingStatusRecord = validation.success
+      ? validation.data
+      : { plex: false, jellyfin: false }
 
     // Update the status for the specified service
     const updatedStatus = {
diff --git a/app/(app)/page.tsx b/app/(app)/page.tsx
@@ -8,9 +8,15 @@ import { prisma } from "@/lib/prisma";
 import type { AuthService } from "@/types/onboarding";
 import { getServerSession } from "next-auth";
 import { redirect } from "next/navigation";
+import { z } from "zod";
 
 export const dynamic = 'force-dynamic'
 
+const OnboardingStatusSchema = z.object({
+  plex: z.boolean(),
+  jellyfin: z.boolean(),
+})
+
 interface OnboardingStatusRecord {
   plex: boolean
   jellyfin: boolean
@@ -57,7 +63,10 @@ export default async function Home() {
 
     // Check service-specific onboarding completion
     if (user) {
-      const status = (user.onboardingStatus as unknown as OnboardingStatusRecord) || { plex: false, jellyfin: false };
+      const validation = OnboardingStatusSchema.safeParse(user.onboardingStatus)
+      const status: OnboardingStatusRecord = validation.success
+        ? validation.data
+        : { plex: false, jellyfin: false };
       const primaryService: AuthService = (user.primaryAuthService as AuthService) || "plex";
 
       // Redirect to onboarding if primary service onboarding not complete
diff --git a/components/auth/jellyfin-sign-in-button.tsx b/components/auth/jellyfin-sign-in-button.tsx
@@ -93,7 +93,20 @@ export function JellyfinSignInButton({
         setIsLoading(false)
       }
     } catch (err) {
-      const errorMsg = "Failed to sign in. Please try again."
+      console.error('Sign-in error:', err)
+      let errorMsg = "Failed to sign in. Please try again."
+
+      // Provide more specific error messages based on error type
+      if (err instanceof Error) {
+        if (err.message.includes('fetch')) {
+          errorMsg = "Unable to reach the server. Please check your connection."
+        } else if (err.message.includes('credentials') || err.message.includes('unauthorized')) {
+          errorMsg = "Invalid username or password."
+        } else if (err.message) {
+          errorMsg = `Failed to sign in: ${err.message}`
+        }
+      }
+
       setError(errorMsg)
       onError?.(errorMsg)
       setIsLoading(false)
diff --git a/lib/auth.ts b/lib/auth.ts
@@ -29,73 +29,69 @@ export const authOptions: NextAuthOptions = {
           process.env.ENABLE_TEST_AUTH === 'true'
 
         if (isTestMode && credentials?.authToken) {
-          console.log('[AUTH] Test mode active, checking test token:', credentials.authToken)
-          console.log('[AUTH] NODE_ENV:', process.env.NODE_ENV)
-          console.log('[AUTH] NEXT_PUBLIC_ENABLE_TEST_AUTH:', process.env.NEXT_PUBLIC_ENABLE_TEST_AUTH)
-          console.log('[AUTH] ENABLE_TEST_AUTH:', process.env.ENABLE_TEST_AUTH)
+          logger.debug('Test mode active', {
+            hasToken: !!credentials.authToken,
+            nodeEnv: process.env.NODE_ENV,
+          })
 
           if (credentials.authToken === 'TEST_ADMIN_TOKEN') {
              // Return the seeded admin user
-             console.log('[AUTH] Looking up admin test user')
+             logger.debug('Looking up admin test user')
              try {
                const adminUser = await prisma.user.findUnique({
                  where: { email: 'admin@example.com' }
                })
-               console.log('[AUTH] Admin user lookup result:', adminUser ? { id: adminUser.id, email: adminUser.email, isAdmin: adminUser.isAdmin } : 'not found')
 
                if (adminUser && adminUser.isAdmin) {
-                 console.log('[AUTH] Admin test user found:', adminUser.email)
+                 logger.debug('Admin test user found', { email: adminUser.email })
                  const userData = {
                    id: adminUser.id,
                    email: adminUser.email,
                    name: adminUser.name,
                    image: adminUser.image,
                    isAdmin: true,
                  }
-                 console.log('[AUTH] Returning user data:', userData)
                  return userData
                } else {
-                 console.error('[AUTH] Admin test user not found or not admin. User:', adminUser)
+                 logger.error('Admin test user not found or not admin', { hasUser: !!adminUser })
                  return null
                }
              } catch (error) {
-               console.error('[AUTH] Error looking up admin user:', error)
+               logger.error('Error looking up admin user', error)
                return null
              }
           }
 
           if (credentials.authToken === 'TEST_REGULAR_TOKEN') {
              // Return the seeded regular user
-             console.log('[AUTH] Looking up regular test user')
+             logger.debug('Looking up regular test user')
              try {
                const regularUser = await prisma.user.findUnique({
                  where: { email: 'regular@example.com' }
                })
-               console.log('[AUTH] Regular user lookup result:', regularUser ? { id: regularUser.id, email: regularUser.email, isAdmin: regularUser.isAdmin } : 'not found')
 
                if (regularUser) {
-                 console.log('[AUTH] Regular test user found:', regularUser.email)
+                 logger.debug('Regular test user found', { email: regularUser.email })
                  const userData = {
                    id: regularUser.id,
                    email: regularUser.email,
                    name: regularUser.name,
                    image: regularUser.image,
                    isAdmin: regularUser.isAdmin,
                  }
-                 console.log('[AUTH] Returning user data:', userData)
                  return userData
                } else {
-                 console.error('[AUTH] Regular test user not found')
+                 logger.error('Regular test user not found')
                  return null
                }
              } catch (error) {
-               console.error('[AUTH] Error looking up regular user:', error)
+               logger.error('Error looking up regular user', error)
                return null
              }
           }
 
           // If test mode but unrecognized token, fail
-          console.error('[AUTH] Test mode active but unrecognized test token:', credentials.authToken)
+          logger.error('Test mode active but unrecognized test token')
           return null
         }
 
@@ -280,9 +276,17 @@ export const authOptions: NextAuthOptions = {
 
           if (!dbUser) {
             // Check if user with same username/email exists (for account linking)
-            // Jellyfin usernames may or may not be emails
-            const userByEmail = jellyfinUser.username.includes('@')
-              ? await prisma.user.findUnique({ where: { email: jellyfinUser.username } })
+            // Validate if Jellyfin username is a valid email format
+            const emailSchema = z.string().email()
+            const emailValidation = emailSchema.safeParse(jellyfinUser.username)
+            const normalizedEmail = emailValidation.success
+              ? jellyfinUser.username.toLowerCase().trim()
+              : null
+
+            const userByEmail = normalizedEmail
+              ? await prisma.user.findUnique({
+                  where: { email: normalizedEmail }
+                })
               : null
 
             if (userByEmail) {
@@ -307,7 +311,7 @@ export const authOptions: NextAuthOptions = {
                 data: {
                   jellyfinUserId: jellyfinUser.id,
                   name: jellyfinUser.username,
-                  email: jellyfinUser.username.includes('@') ? jellyfinUser.username : null,
+                  email: normalizedEmail,
                   isAdmin,
                   primaryAuthService: "jellyfin",
                   onboardingStatus: { plex: false, jellyfin: false },
@@ -395,20 +399,26 @@ export const authOptions: NextAuthOptions = {
         session.user.image = token.picture as string
         session.user.isAdmin = token.isAdmin as boolean
       } else {
-        console.warn(`[AUTH] Session callback - missing token.sub or session.user: hasTokenSub=${!!token.sub} hasSessionUser=${!!session.user}`)
+        logger.warn('Session callback - missing token.sub or session.user', {
+          hasTokenSub: !!token.sub,
+          hasSessionUser: !!session.user
+        })
       }
       return session
     },
     async jwt({ token, user }) {
       if (user) {
-        console.log('[AUTH] JWT callback - user:', { id: user.id, email: user.email, isAdmin: (user as any).isAdmin })
+        logger.debug('JWT callback - user signed in', {
+          userId: user.id,
+          email: user.email,
+          isAdmin: (user as any).isAdmin
+        })
         // Store user info in JWT when user first signs in
         token.sub = user.id
         token.name = user.name
         token.email = user.email
         token.picture = user.image
         token.isAdmin = (user as any).isAdmin || false
-        console.log('[AUTH] JWT callback - token updated:', { sub: token.sub, email: token.email, isAdmin: token.isAdmin })
       }
       return token
     },
diff --git a/lib/jellyfin-auth.ts b/lib/jellyfin-auth.ts
@@ -6,6 +6,7 @@
 
 import { authenticateJellyfinUser, getJellyfinUserById } from "@/lib/connections/jellyfin-users"
 import { createLogger } from "@/lib/utils/logger"
+import { z } from "zod"
 
 const logger = createLogger("JELLYFIN_AUTH")
 
@@ -24,6 +25,22 @@ interface JellyfinAuthResult {
   error?: string
 }
 
+/**
+ * Zod schema for Jellyfin credentials
+ * Validates username and password inputs before authentication
+ */
+const JellyfinCredentialsSchema = z.object({
+  username: z
+    .string()
+    .min(1, "Username is required")
+    .max(100, "Username is too long")
+    .trim(),
+  password: z
+    .string()
+    .min(1, "Password is required")
+    .max(1000, "Password is too long"),
+})
+
 /**
  * Authenticate a Jellyfin user with username and password
  *
@@ -37,18 +54,31 @@ export async function authenticateJellyfin(
   username: string,
   password: string
 ): Promise<JellyfinAuthResult> {
-  logger.debug("Authenticating Jellyfin user", { username })
+  // Validate credentials
+  const validation = JellyfinCredentialsSchema.safeParse({ username, password })
+  if (!validation.success) {
+    const errors = validation.error.errors.map(e => e.message).join(", ")
+    logger.warn("Invalid Jellyfin credentials format", { errors })
+    return {
+      success: false,
+      error: errors,
+    }
+  }
+
+  const { username: validatedUsername, password: validatedPassword } = validation.data
+
+  logger.debug("Authenticating Jellyfin user", { username: validatedUsername })
 
   try {
     const authResult = await authenticateJellyfinUser(
       jellyfinConfig,
-      username,
-      password
+      validatedUsername,
+      validatedPassword
     )
 
     if (!authResult.success || !authResult.data) {
       logger.warn("Jellyfin authentication failed", {
-        username,
+        username: validatedUsername,
         error: authResult.error,
       })
       return {
@@ -73,7 +103,7 @@ export async function authenticateJellyfin(
       },
     }
   } catch (error) {
-    logger.error("Error during Jellyfin authentication", error, { username })
+    logger.error("Error during Jellyfin authentication", error, { username: validatedUsername })
     return {
       success: false,
       error: "An unexpected error occurred during authentication",
