Skip to content

fix: enforce ownership and disable caching for AI endpoint#467

Open
Rial1608 wants to merge 249 commits into
nisshchayarathi:mainfrom
Rial1608:fix/ai-security
Open

fix: enforce ownership and disable caching for AI endpoint#467
Rial1608 wants to merge 249 commits into
nisshchayarathi:mainfrom
Rial1608:fix/ai-security

Conversation

@Rial1608

@Rial1608 Rial1608 commented May 22, 2026

Copy link
Copy Markdown
  • Enforced user ownership validation to prevent unauthorized access
  • Added Cache-Control: no-store to all responses to prevent caching of sensitive data
  • Standardized error handling (converted 403 to 404 to avoid resource exposure)
  • Improved input validation with strict type checks and allowed analysis types
  • Added structured logging for better debugging and observability
  • Replaced magic numbers with constants for maintainability
  • Improved type safety by replacing any with proper interfaces

Summary by CodeRabbit

  • Bug Fixes

    • Enforced repository ownership checks to prevent unauthorized analysis access
  • Improvements

    • Stronger request validation with clearer, structured failure logging
    • Unified success/error JSON responses with no-store cache headers
    • Improved error/status mapping, including distinct timeout (504) responses
    • Added timeout protection for analysis operations
    • More reliable authentication handling across API requests
  • New Features

    • Introduced a dedicated timeout error type for clearer timeout reporting

Review Change Stack

Tanisha-sharma7302 and others added 30 commits May 21, 2026 14:26
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
- chat: validate repositoryId as number, question as non-empty string, conversationHistory structure
- analyze-repository: validate repositoryId as number, type against allowed values
- analyze-code: validate code, language as non-empty strings, analysisType against allowed values
- explain-file: validate repositoryId as number, filePath as non-empty string
- suggest-commit: strict array and string type checks
- review-pr: remove unreachable fallthrough code
- Repository: add index on userId for listRepositories, getRepository, deleteRepository
- Repository: add composite index on [userId, url] for createRepository duplicate check
- AnalysisJob: add composite index on [repositoryId, userId, createdAt] for latest job lookup
- chat: validate repositoryId as number, question as non-empty string, conversationHistory structure
- analyze-repository: validate repositoryId as number, type against allowed values
- analyze-code: validate code, language as non-empty strings, analysisType against allowed values
- explain-file: validate repositoryId as number, filePath as non-empty string
- suggest-commit: strict array and string type checks
- review-pr: remove unreachable fallthrough code
- Repository: add index on userId for listRepositories, getRepository, deleteRepository
- Repository: add composite index on [userId, url] for createRepository duplicate check
- AnalysisJob: add composite index on [repositoryId, userId, createdAt] for latest job lookup
…0-feature/repository-skeleton-loader-189

Revert "feat(ui): add skeleton loader for repository readme (nisshchayarathi#189)"
Webhook handler queries gitHubRepo.findFirst({ where: { repoFullName, enabled: true } })
on every pull_request event. Adding a composite index on [repoFullName, enabled] avoids
a full table scan when matching incoming webhooks to enabled repos.
- chat: validate repositoryId as number, question as non-empty string, conversationHistory structure
- analyze-repository: validate repositoryId as number, type against allowed values
- analyze-code: validate code, language as non-empty strings, analysisType against allowed values
- explain-file: validate repositoryId as number, filePath as non-empty string
- suggest-commit: strict array and string type checks
- review-pr: remove unreachable fallthrough code
- Repository: add index on userId for listRepositories, getRepository, deleteRepository
- Repository: add composite index on [userId, url] for createRepository duplicate check
- AnalysisJob: add composite index on [repositoryId, userId, createdAt] for latest job lookup
Webhook handler queries gitHubRepo.findFirst({ where: { repoFullName, enabled: true } })
on every pull_request event. Adding a composite index on [repoFullName, enabled] avoids
a full table scan when matching incoming webhooks to enabled repos.
Account: add @@index([userId, provider]) to speed up count and deleteMany
queries filtering by user and provider (used in users/me, users/profile,
auth/signup, auth/login).

Session: add @@index([userId]) to speed up session lookups by user.
…ory queries

Branch, Contributor, and Language models are frequently queried by
repositoryId in stats and listing endpoints. Adding explicit
@@index([repositoryId]) on each model provides a dedicated non-unique
index optimized for count and list operations, separate from the
unique constraint indexes.
…d-indexes

fix(db): add indexes for common worker and API query paths
Webhook handler queries gitHubRepo.findFirst({ where: { repoFullName, enabled: true } })
on every pull_request event. Adding a composite index on [repoFullName, enabled] avoids
a full table scan when matching incoming webhooks to enabled repos.
Account: add @@index([userId, provider]) to speed up count and deleteMany
queries filtering by user and provider (used in users/me, users/profile,
auth/signup, auth/login).

Session: add @@index([userId]) to speed up session lookups by user.
…ory queries

Branch, Contributor, and Language models are frequently queried by
repositoryId in stats and listing endpoints. Adding explicit
@@index([repositoryId]) on each model provides a dedicated non-unique
index optimized for count and list operations, separate from the
unique constraint indexes.
AnalysisJob: add @@index([userId, status]) to speed up queries filtering
by user and job status (used in job listing and status filtering endpoints).
…sisjobs-db-indexes

fix(db): add index on AnalysisJob for user status queries
Session: add @@index([expires]) to speed up session cleanup queries that
filter by expiry date. NextAuth and custom auth flows periodically delete
expired sessions, and this index avoids a full table scan.
Session: add @@index([expires]) to speed up session cleanup queries that
filter by expiry date. NextAuth and custom auth flows periodically delete
expired sessions, and this index avoids a full table scan.
Add UUID format validation to both analysis job routes to reject
invalid job IDs early with a 400 response instead of returning a
misleading 404.

- app/api/analysis-jobs/[id]/route.ts: validate jobId is valid UUID
- app/api/analysis/[jobId]/route.ts: validate jobId is valid UUID
Rial1608 and others added 2 commits May 22, 2026 19:20
…validation-400-89

fix: Add input validation to auth routes and return 400 with actionable messages (nisshchayarathi#89)

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@app/api/ai/analyze-repository/route.ts`:
- Around line 2-7: Remove the unresolved Git conflict markers and restore a
single clean import block: import isHttpError and requireAuth from
"`@/lib/api-auth`" (as intended) and retain the existing AnalysisTimeoutError
import from "`@/lib/errors/AnalysisTimeoutError`"; ensure there are no duplicate
or conflicting imports for isHttpError/requireAuth and that the import list in
route.ts references the symbols exactly as shown so the file parses correctly.
🪄 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 Plus

Run ID: dff1e4f6-338c-4327-b8ae-124505d6f462

📥 Commits

Reviewing files that changed from the base of the PR and between 466316c and 9067815.

📒 Files selected for processing (1)
  • app/api/ai/analyze-repository/route.ts

Comment on lines +2 to +7
<<<<<<< HEAD
import { isHttpError, requireAuth } from "@/lib/middleware";
import { AnalysisTimeoutError } from "@/lib/errors/AnalysisTimeoutError";
=======
import { isHttpError, requireAuth } from "@/lib/api-auth";
>>>>>>> upstream/main

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Unresolved merge conflict markers will break the build.

The file contains Git conflict markers that prevent parsing. Based on the PR objectives (removing x-resource-owner-id header-based checks from middleware and moving ownership enforcement to route handlers), the @/lib/api-auth import path appears to be the intended resolution.

Proposed fix
 import { NextRequest, NextResponse } from "next/server";
-<<<<<<< HEAD
-import { isHttpError, requireAuth } from "`@/lib/middleware`";
-import { AnalysisTimeoutError } from "`@/lib/errors/AnalysisTimeoutError`";
-=======
 import { isHttpError, requireAuth } from "`@/lib/api-auth`";
->>>>>>> upstream/main
+import { AnalysisTimeoutError } from "`@/lib/errors/AnalysisTimeoutError`";
 import { getGeminiService } from "`@/lib/services/geminiService`";
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<<<<<<< HEAD
import { isHttpError, requireAuth } from "@/lib/middleware";
import { AnalysisTimeoutError } from "@/lib/errors/AnalysisTimeoutError";
=======
import { isHttpError, requireAuth } from "@/lib/api-auth";
>>>>>>> upstream/main
import { NextRequest, NextResponse } from "next/server";
import { isHttpError, requireAuth } from "`@/lib/api-auth`";
import { AnalysisTimeoutError } from "`@/lib/errors/AnalysisTimeoutError`";
import { getGeminiService } from "`@/lib/services/geminiService`";
🧰 Tools
🪛 Biome (2.4.15)

[error] 2-2: Expected a statement but instead found '<<<<<<< HEAD'.

(parse)


[error] 5-5: Expected a statement but instead found '======='.

(parse)


[error] 7-7: Expected a statement but instead found '>>>>>>> upstream/main'.

(parse)

🤖 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 `@app/api/ai/analyze-repository/route.ts` around lines 2 - 7, Remove the
unresolved Git conflict markers and restore a single clean import block: import
isHttpError and requireAuth from "`@/lib/api-auth`" (as intended) and retain the
existing AnalysisTimeoutError import from "`@/lib/errors/AnalysisTimeoutError`";
ensure there are no duplicate or conflicting imports for isHttpError/requireAuth
and that the import list in route.ts references the symbols exactly as shown so
the file parses correctly.

@Rial1608

Copy link
Copy Markdown
Author

Hi @nisshchayarathi

All requested changes have been addressed and conflicts resolved. The remaining Vercel check appears to require authorization—please let me know if anything is needed from my side.

Looking forward to your feedback.

pithva007 and others added 16 commits May 22, 2026 19:36
…-delete-cascade-285

fix: remove related analysis jobs during repository deletion
…oks-security-no-store-183

feat(webhooks): enforce ownership checks and no-store caching (nisshchayarathi#183)
…progress-messages-295

feat: improve analysis job progress reporting and percentages
…itory-id-validation-63

feat(security): add server-side validation for repository identifiers (nisshchayarathi#63)
…-time-budget-312

feat: add configurable time budget to analysis cron runner
…pp-permissions-309

docs: document GitHub App permissions and environment setup
…ct-secrets-auth-checks-340

security: redact sensitive logs and strengthen authorization checks
…-failed-status-handling-301

fix: set repository status to failed on analysis errors
…-time-budget-followup-342

feat: improve cron runner time budget handling and observability
…tore-user-profile-routes-262

fix: prevent caching of user profile API responses
@tmdeveloper007

Copy link
Copy Markdown
Contributor

This issue has been fixed in PR #2594: #2594

The fix includes:

  • Targeted code changes addressing the root cause
  • TypeScript compilation passes
  • All CI checks passing

Please review and merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.