chore: added console.errro - #30
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the backend's error handling and code readability. It introduces explicit Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Render
Destroyed
📝 WalkthroughWalkthroughEnhanced error logging and code quality in backend controllers. Added console.error() calls in catch blocks across data and schema controllers, introduced a null-check guard for project retrieval, and removed stray comments. No changes to function signatures or core validation logic. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces console.error logging within catch blocks across several controllers, enhancing error visibility. However, a security audit identified significant NoSQL injection vulnerabilities in the data handling logic. Specifically, the getAllData function lacks sanitization for query parameters, and the updateSingleData function has incomplete validation for 'Date' fields, potentially allowing attackers to manipulate database queries. Implementing robust input validation and sanitization, ideally with a dedicated library for MongoDB operator stripping, is strongly recommended to mitigate these risks. Additionally, for improved logging practices, consider adopting a structured logger for all new console.error calls. Please also address a minor typo in a comment and a small typo in the pull request title.
| console.timeEnd("getall") | ||
| res.json(data); | ||
| } catch (err) { | ||
| console.error(err); |
There was a problem hiding this comment.
The getAllData function is vulnerable to NoSQL injection because it passes the unsanitized req.query object to the QueryEngine. The QueryEngine.filter method directly assigns query parameters to the MongoDB query object, allowing an attacker to use MongoDB operators (e.g., ?field[$ne]=value) to bypass filters or extract sensitive data. Although this line adds error logging, the underlying function remains insecure.
| for (const key in incomingData) { | ||
| const fieldRule = schemaRules.find(f => f.key === key); | ||
| if (!fieldRule) continue; // Unknown fields ko ignore karo | ||
| if (!fieldRule) continue; |
There was a problem hiding this comment.
The updateSingleData function is vulnerable to NoSQL injection, particularly for fields defined as 'Date' in the schema. The validation loop modified here lacks a type check for 'Date' fields, allowing an attacker to provide an object containing MongoDB operators (e.g., {"myDateField": {"$gt": ""}}). Since the sanitize function only checks top-level keys for the $ prefix, these nested operators will bypass sanitization and be passed directly to Model.findByIdAndUpdate, potentially leading to unauthorized data manipulation.
| console.timeEnd("insert data") | ||
| res.status(201).json(result); | ||
| } catch (err) { | ||
| console.error(err); |
There was a problem hiding this comment.
While console.error is useful, for production environments it's recommended to use a structured logger (e.g., Winston, Pino). Structured logging provides more context with log levels, can output in machine-readable formats like JSON for easier analysis with log management tools, and offers more flexible configuration.
| const Model = getCompiledModel(connection, collectionConfig, project._id, project.resources.db.isExternal); | ||
|
|
||
| // Strict Schema Validation | ||
| // Schma Validation |
There was a problem hiding this comment.
There's a typo in this comment. It should be 'Schema'.
| // Schma Validation | |
| // Schema Validation |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/controllers/schema.controller.js (1)
30-30: Useconstfor non-reassignedproject.Line 30 does not reassign
project, soconstcommunicates intent better.Proposed small cleanup
- let project = req.project; + const project = req.project;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/controllers/schema.controller.js` at line 30, Change the variable declaration from mutable to immutable: replace the "let project = req.project" binding in schema.controller.js with a const declaration (i.e., use "const project") since the variable is not reassigned; update any surrounding comments or usages if needed but keep the identifier "project" unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/controllers/data.controller.js`:
- Line 140: Replace the typo in the inline comment reading "Schma Validation"
with the correct "Schema Validation" so the validation section comment is clear
and accurate (search for the exact comment string "Schma Validation" in
backend/controllers/data.controller.js and update it to "Schema Validation").
In `@backend/controllers/schema.controller.js`:
- Line 57: Add a null-safety guard before calling deleteProjectByApiKeyCache:
check that fullProject && fullProject.apiKey is truthy and only then call
deleteProjectByApiKeyCache(fullProject.apiKey). Mirror the existing pattern used
for req.hashedApiKey (lines handling req.hashedApiKey) so you avoid constructing
malformed cache keys when fullProject.apiKey is falsy.
---
Nitpick comments:
In `@backend/controllers/schema.controller.js`:
- Line 30: Change the variable declaration from mutable to immutable: replace
the "let project = req.project" binding in schema.controller.js with a const
declaration (i.e., use "const project") since the variable is not reassigned;
update any surrounding comments or usages if needed but keep the identifier
"project" unchanged.
| const Model = getCompiledModel(connection, collectionConfig, project._id, project.resources.db.isExternal); | ||
|
|
||
| // Strict Schema Validation | ||
| // Schma Validation |
There was a problem hiding this comment.
Fix typo in validation comment.
Line 140 says Schma Validation; please rename it to Schema Validation for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/controllers/data.controller.js` at line 140, Replace the typo in the
inline comment reading "Schma Validation" with the correct "Schema Validation"
so the validation section comment is clear and accurate (search for the exact
comment string "Schma Validation" in backend/controllers/data.controller.js and
update it to "Schema Validation").
| await setProjectById(projectId.toString(), fullProject); | ||
| await deleteProjectByApiKeyCache(fullProject.apiKey); // In case it was cached by hashed key | ||
| // NOTE: req.hashedApiKey is available from middleware if we strictly want that. | ||
| await deleteProjectByApiKeyCache(fullProject.apiKey); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check Project model for apiKey requirement/default =="
fd 'Project\.(js|ts)$' | while read -r f; do
echo "--- $f ---"
rg -n -C3 'apiKey|required|default' "$f" || true
done
echo
echo "== Inspect deleteProjectByApiKeyCache null/falsy handling =="
fd 'redisCaching\.(js|ts)$' | while read -r f; do
echo "--- $f ---"
rg -n -C4 'deleteProjectByApiKeyCache|apiKey|toString\(' "$f" || true
doneRepository: yash-pouranik/urBackend
Length of output: 2373
🏁 Script executed:
# Find schema.controller.js and read context around line 57
fd 'schema\.controller\.js$' | head -1 | xargs cat -n | sed -n '40,75p'Repository: yash-pouranik/urBackend
Length of output: 1535
Add a guard before invalidating API-key cache.
Line 57 passes fullProject.apiKey directly to deleteProjectByApiKeyCache, which lacks null-safety and would create a malformed cache key (e.g., project:apikey:null) if the value is ever falsy. Though the schema requires apiKey, defensive guarding aligns with the pattern already used at lines 58–60 for req.hashedApiKey.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/controllers/schema.controller.js` at line 57, Add a null-safety guard
before calling deleteProjectByApiKeyCache: check that fullProject &&
fullProject.apiKey is truthy and only then call
deleteProjectByApiKeyCache(fullProject.apiKey). Mirror the existing pattern used
for req.hashedApiKey (lines handling req.hashedApiKey) so you avoid constructing
malformed cache keys when fullProject.apiKey is falsy.
1 participant
Summary by CodeRabbit
Release Notes