Fix security vulnerabilities and improve code quality #1020
Conversation
- Fix XSS vulnerability in embed page with DOMPurify sanitization - Add Pino-based logging infrastructure to replace console.log statements - Replace 877+ console.log/error/warn calls with proper logger - Fix TypeScript any types in auth module with ExtendedAdapterUser interface - Remove deprecated getConfiguredAuthPlugin function - Add environment variable validation function - Fix TypeScript implicit any types in prompts route - Resolve tsconfig.json type definition warnings
|
@yasuo72 is attempting to deploy a commit to the fkadev Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe changes introduce structured logging via pino across the application, improve type safety for authentication adapters, add HTML sanitization for security, implement environment validation at startup, reorganize type declarations, and remove a deprecated API. Overall, these are infrastructure and safety improvements without significant functional changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
package.json (1)
73-73:⚠️ Potential issue | 🟠 MajorAdd
pino-prettyas a dev dependency.The logger (
src/lib/logger.ts, line 10) referencespino-prettyas a transport target in development, but it's not listed inpackage.json. The dev logger will fail at startup without it.src/app/api/prompts/route.ts (1)
300-301:⚠️ Potential issue | 🟡 MinorInconsistent logging: POST error handler still uses
console.error.The GET handler (Line 452) was updated to use
logger.error, but the POST handler's catch block on Line 301 still usesconsole.error. This appears to be an oversight given the PR's goal of replacing console calls with the pino logger.Proposed fix
} catch (error) { - console.error("Create prompt error:", error); + logger.error({ error }, "Create prompt error"); return NextResponse.json(
🤖 Fix all issues with AI agents
In `@src/app/api/prompts/route.ts`:
- Around line 434-442: The map callback should not use the any type and should
not destructure a non-selected field; remove the ": any" on promptRaw so
TypeScript infers the correct Prisma type from promptsRaw, and drop the phantom
"embedding" from the destructuring (or instead include embedding in the Prisma
select if you actually need it). Update the mapper (promptsRaw.map(promptRaw =>
{ const { isPrivate, isUnlisted, unlistedAt, deletedAt, ...p } = promptRaw;
return { ...p, voteCount: p._count.votes, contributorCount:
p._count.contributors, contributors: p.contributors }; })) so promptRaw is
strongly typed and no undeclared fields like embedding are referenced.
In `@src/app/embed/page.tsx`:
- Line 7: The import statement import DOMPurify from "dompurify" in
src/app/embed/page.tsx will fail at runtime because dompurify is not listed in
package.json; add dompurify to dependencies and `@types/dompurify` to
devDependencies (or the equivalent with yarn/pnpm) by running e.g. npm install
dompurify && npm install -D `@types/dompurify` so the runtime package and
TypeScript types are present.
In `@src/lib/auth/index.ts`:
- Around line 49-52: The createUser override currently accepts
ExtendedAdapterUser which is narrower than the Adapter interface type and can
break strictFunctionTypes; change the method signature to accept the base
AdapterUser (e.g., async createUser(data: AdapterUser)) so it conforms to the
Adapter contract, then inside createUser narrow/validate for
ExtendedAdapterUser-specific fields (username, githubUsername) with runtime
checks or a type guard before using them; alternatively add an overload that
accepts AdapterUser and refines to ExtendedAdapterUser internally to ensure both
compile-time correctness and safe runtime behavior.
In `@src/lib/config/index.ts`:
- Around line 232-245: The validateEnvironment function currently treats
NEXTAUTH_URL as a hard required variable; remove NEXTAUTH_URL from the required
array in validateEnvironment (or move it into a new optional/recommended list)
so startup won't throw on platforms that infer it (e.g., Vercel); update the
error message to only list truly required keys (and, if you add an
optional/recommended list, include a comment or log indicating NEXTAUTH_URL is
recommended but may be inferred by some hosts).
🧹 Nitpick comments (4)
src/lib/config/index.ts (1)
254-258: Use the newloggerinstead ofconsole.warnfor consistency.The PR introduces a centralized
pinologger insrc/lib/logger.ts, yet this new function usesconsole.warn. Use the logger for consistent structured logging.♻️ Suggested fix
+import { logger } from "@/lib/logger"; + // Warn about optional but recommended variables const missingRecommended = recommended.filter(key => !process.env[key]); if (missingRecommended.length > 0) { - console.warn( - `Warning: Optional environment variables not set: ${missingRecommended.join(', ')}. ` + - `Some features may be limited.` + logger.warn( + { missing: missingRecommended }, + "Optional environment variables not set. Some features may be limited." ); }src/lib/logger.ts (1)
34-36:logQualityCheckErrorwon't serialize the error with stack trace as expected.Passing
{ error }as the first argument tologger.errorwill trigger the customerrorserializer, which is good. However, pino's convention is to pass the error object directly under theerrkey for best integration with pino-pretty and other transports. Consider using{ err: error }for consistency with pino conventions, or keep as-is since you've defined both serializer keys.src/app/api/prompts/route.ts (1)
151-151: Verbose inline type annotation duplicates Prisma's inferred type.The long inline type on the
.find()callback parameter is fragile — it must be kept in sync with theselectclause on Lines 139–145. Since Prisma already infers the element type from the query result, you can let TypeScript infer it or extract a named type.Proposed fix
- const similarPrompt = publicPrompts.find((p: { id: string; slug: string | null; title: string; content: string; author: { username: string } }) => isSimilarContent(content, p.content)); + const similarPrompt = publicPrompts.find((p) => isSimilarContent(content, p.content));src/lib/ai/quality-check.ts (1)
145-146: Original parse error is discarded.The
catchblock doesn't capture the thrown error, sologQualityCheckErrorreceives a genericnew Error(...)instead of the actualSyntaxErrorwith the parse failure details (e.g., position of the invalid token). Binding the error and forwarding it would improve debuggability.Proposed fix
- } catch { - logQualityCheckError(new Error("Failed to parse AI quality check response")); + } catch (parseError) { + logQualityCheckError(parseError);
|
Hi maintainers 👋 |
1 participant
This PR addresses critical security issues and improves overall code quality:
Security Fixes
<span>tags withclassattributes, preventing malicious HTML injectionCode Quality Improvements
TypeScript Improvements
anytypes in auth module with proper ExtendedAdapterUser interfaceanytypes in prompts route API handlersgetConfiguredAuthPlugin()functionNew Features
Changes
Testing
Summary by CodeRabbit
Bug Fixes
Chores