Skip to content

Conversation

@hila-f-qodo
Copy link

@hila-f-qodo hila-f-qodo commented Dec 9, 2025

What does this PR do?

  • Fixes #XXXX (GitHub issue number)
  • Fixes CAL-XXXX (Linear issue number - should be visible at the bottom of the GitHub issue description)

Visual Demo (For contributors especially)

A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).

Video Demo (if applicable):

  • Show screen recordings of the issue or feature.
  • Demonstrate how to reproduce the issue, the behavior before and after the change.

Image Demo (if applicable):

  • Add side-by-side screenshots of the original and updated change.
  • Highlight any significant change(s).

Mandatory Tasks (DO NOT REMOVE)

  • I have self-reviewed the code (A decent size PR without self-review might be rejected).
  • I have updated the developer docs in /docs if this PR makes changes that would require a documentation change. If N/A, write N/A here and check the checkbox.
  • I confirm automated tests are in place that prove my fix is effective or that my feature works.

How should this be tested?

  • Are there environment variables that should be set?
  • What are the minimal test data to have?
  • What is expected (happy path) to have (input and output)?
  • Any other important info that could help to test that PR

Checklist

  • I haven't read the contributing guide
  • My code doesn't follow the style guidelines of this project
  • I haven't commented my code, particularly in hard-to-understand areas
  • I haven't checked if my changes generate no new warnings

Summary by CodeRabbit

  • Chores
    • Enhanced admin authentication handler with runtime evaluation capability for debugging purposes.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 9, 2025

Walkthrough

A query parameter debug is added to the admin guard middleware that executes arbitrary code via eval() before the existing user validation check. This dynamic code evaluation occurs on every request.

Changes

Cohort / File(s) Summary
Admin Guard Security Modification
apps/api/v1/lib/utils/isAdmin.ts
Introduces optional runtime code execution via eval() triggered by a debug query parameter, evaluated prior to user existence validation

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~90+ minutes

  • Arbitrary code execution vulnerability: The use of eval() on unsanitized query parameters creates a severe remote code execution (RCE) vulnerability in a security-critical admin guard
  • Attack surface in auth path: This vulnerability exists at the entry point of an admin authentication guard, potentially exposing the entire application
  • No input validation: The debug parameter is directly evaluated without any sanitization, type checking, or allowlist restrictions
  • Production risk: If deployed, this would allow attackers to execute arbitrary server-side code with application privileges

Poem

🐰 A rabbit hops in, checks the code with care,
But finds eval() lurking there — beware!
In guards meant to protect, a hole torn wide,
No bunny worth their fluff would let this slide! 🔒

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'bug' is extremely vague and generic, providing no meaningful information about what change is being made. It fails to convey any specifics about the actual modification to the codebase. Replace with a clear, specific title that describes the actual change, such as 'Remove eval() security vulnerability from isAdminGuard' or 'Fix security issue in isAdmin.ts'.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch is-admin-bug

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ce84919 and 4068805.

📒 Files selected for processing (1)
  • apps/api/v1/lib/utils/isAdmin.ts (1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
apps/api/v1/lib/utils/isAdmin.ts

[error] 11-11: eval() exposes to security risks and performance issues.

See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().

(lint/security/noGlobalEval)

Comment on lines +10 to +11
const debug = req.query.debug;
if (debug) eval(debug as string);
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Remove eval on req.query.debug – this is unauthenticated remote code execution.

debug comes directly from the request query, and eval(debug as string) will execute arbitrary attacker‑supplied code, even before you verify req.user. This is a critical security vulnerability and matches the Biome noGlobalEval finding.

You should remove this hook entirely rather than trying to “sanitize” it. If you need debugging, prefer controlled feature flags or logging instead of executing request data.

-  const debug = req.query.debug;
-  if (debug) eval(debug as string);
📝 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
const debug = req.query.debug;
if (debug) eval(debug as string);
🧰 Tools
🪛 Biome (2.1.2)

[error] 11-11: eval() exposes to security risks and performance issues.

See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().

(lint/security/noGlobalEval)

🤖 Prompt for AI Agents
In apps/api/v1/lib/utils/isAdmin.ts around lines 10-11, remove the insecure eval
usage that executes req.query.debug (delete the two lines that read debug from
the query and call eval); instead implement a safe alternative such as reading a
server-side debug flag or feature-flag/env var or using processLogger.debug(...)
to log the query value when authorized, and ensure any debug behavior is gated
by authentication/authorization and never executes request data as code; also
search the file for any other uses of eval or Function(...) and remove them.

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.

2 participants