Fixed security issues and improved code quality #1021
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
📝 WalkthroughWalkthroughThis PR introduces structured logging infrastructure using pino, replaces console logging with standardized logger calls across multiple service files, adds XSS protection via DOMPurify sanitization, improves TypeScript type safety with enhanced configurations and annotations, introduces startup-time environment validation, and removes deprecated code. 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 (4)
package.json (1)
74-74:⚠️ Potential issue | 🟠 Major
pino-prettyis used as a transport but not listed as a dependency.
src/lib/logger.tsreferencespino-prettyas a transport target in development mode. If it's not installed, the logger will fail at runtime in dev. Add it todevDependencies.Suggested fix
"devDependencies": { "@clack/prompts": "^0.11.0", + "pino-pretty": "^13.0.0",src/lib/ai/improve-prompt.ts (1)
45-57:⚠️ Potential issue | 🟡 Minor
cosineSimilarityreturnsNaNfor zero-magnitude vectors.If either vector is all zeros,
Math.sqrt(normA) * Math.sqrt(normB)is 0, producingNaN. This would pass the>= SIMILARITY_THRESHOLDfilter unpredictably. A guard returning0for zero-norm vectors would be safer.🛡️ Proposed fix
return dotProduct / (Math.sqrt(normA) * Math.sqrt(normB)); + const denom = Math.sqrt(normA) * Math.sqrt(normB); + return denom === 0 ? 0 : dotProduct / denom;(Replace the existing return with the guarded version.)
src/lib/ai/quality-check.ts (1)
139-144:⚠️ Potential issue | 🟡 MinorUnsafe type assertion on untrusted AI response.
result.reason as DelistReason | nullblindly trusts the AI output. If the model returns an unexpected string (e.g.,"SPAM"), it will be stored in the database as adelistReasonthat doesn't match the enum, potentially causing downstream issues. Validate against the knownDelistReasonvalues.🛡️ Proposed fix
+ const VALID_DELIST_REASONS: Set<string> = new Set(["TOO_SHORT", "NOT_ENGLISH", "LOW_QUALITY", "NOT_LLM_INSTRUCTION", "MANUAL"]); + return { shouldDelist: !!result.shouldDelist, - reason: result.reason as DelistReason | null, + reason: VALID_DELIST_REASONS.has(result.reason) ? (result.reason as DelistReason) : null, confidence: result.confidence || 0, details: result.details || "Quality check completed", };src/app/api/prompts/route.ts (1)
300-301:⚠️ Potential issue | 🟡 MinorMissed
console.error→logger.errorreplacement in the POST catch block.The GET handler (Line 452) was migrated to
logger.error, but the POST handler's catch block on Line 301 still usesconsole.error. This is inconsistent with the PR's goal of replacing all console calls.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/lib/auth/index.ts`:
- Around line 8-12: The ExtendedAdapterUser interface is declared but not used;
replace the loose any casts by updating the createUser signature and any adapter
user parameters to use ExtendedAdapterUser so you can access username and
githubUsername without (data as any). Concretely, change the createUser
parameter type from AdapterUser to ExtendedAdapterUser and update any local
variables/parameters (e.g., where you currently do (data as any).username or
(data as any).githubUsername) to use the typed ExtendedAdapterUser, leaving
runtime checks if needed.
In `@src/lib/config/index.ts`:
- Around line 254-258: Replace the console.warn call that logs
missingRecommended with the new logger: import and use the exported logger
instance and call logger.warn(...) instead of console.warn(...), keeping the
same interpolated message using missingRecommended.join(', ') so the warning
content is unchanged; update the top of the file to add the logger import (from
the project's logger module) and remove the console usage.
- Around line 232-244: Exported validateEnvironment() is never invoked so
required env vars may go unvalidated; call validateEnvironment() from your
application's earliest startup entry point (before any initialization that
depends on env vars) — for example, import { validateEnvironment } from
'src/lib/config' into the top of the startup/bootstrap module (the file that
runs first) and invoke validateEnvironment() synchronously at module load time
so DATABASE_URL and NEXTAUTH_SECRET are validated before the app initializes.
In `@src/rohit.MD`:
- Around line 1-32: Remove the rohit.MD document from the commit and PR: delete
the file from the branch (or revert the addition) so it is not committed to src,
and instead move any valid changelog content into the PR description or a
top-level CHANGELOG.md; scrub the text to remove any absolute/local paths (e.g.,
Windows file:///C:/... references) and correct the environment-variable
statement to reflect that NEXTAUTH_URL is optional/recommended (not required) to
match validateEnvironment() behavior.
🧹 Nitpick comments (5)
src/lib/auth/index.ts (1)
162-163: Remaininganytypes in JWT and session callbacks.The
jwtandsessioncallbacks useanyfortoken,user, andsessionparameters. NextAuth provides proper types for these — consider typing them with the module-augmented types declared at the bottom of this file.Also applies to: 205-206
src/lib/logger.ts (1)
34-36: Error helpers passerroras a data field — consider usingerrfor Pino convention.While the custom
errorserializer handles this, Pino's ecosystem and tooling conventionally expect theerrkey. Using{ err: error }instead of{ error }would align with Pino conventions and ensure compatibility with log aggregation tools that look forerr.src/app/api/prompts/route.ts (2)
151-151: Verbose inline type annotation on.find()callback parameter.The type is already inferred from the Prisma query's
selectclause. The explicit annotation is redundant and will break if the select shape changes.Simplified version
- 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));
434-442: DestructuredisPrivate,isUnlisted,unlistedAt,deletedAtare unused after stripping.This is intentional (stripping internal fields from the API response), which is fine. However,
delistReasonand other potentially sensitive fields from the Prismaincludeare not stripped. Consider whether fields likedelistReasonshould also be excluded from the public API response.src/lib/ai/improve-prompt.ts (1)
11-25: Extract duplicatedgetOpenAIClient()singleton pattern into a shared module.This exact pattern is duplicated across five files:
improve-prompt.ts,quality-check.ts,generation.ts,embeddings.ts, andslug.ts. Extract it into@/lib/ai/openai-client.ts.Note:
slug.tsreturnsOpenAI | nulland returnsnullif the API key is missing, while the other four files throw an error. The extraction should either support both patterns or updateslug.tsto match the error-throwing behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/instrumentation.ts`:
- Around line 4-7: The call to validateEnvironment() inside register() runs
before Sentry is initialized so thrown errors aren't captured; move the
validateEnvironment() invocation to after the dynamic imports/Sentry
initialization block in register() (i.e., call validateEnvironment() only once
Sentry has been set up) so startup validation errors can be reported, keeping
the validateEnvironment() function and its synchronous behavior unchanged.
1 participant
Hey! I've been working on making the codebase more secure and maintainable. Here's what I did:
Security Fixes
Code Quality Improvements
TypeScript Improvements
anytypes by using proper Prisma type inferencegetConfiguredAuthPlugin()functionBetter Configuration
What Changed
Testing
This should make the codebase more reliable and easier to maintain going forward!
Summary by CodeRabbit
Bug Fixes
Dependencies
Improvements