feat(admin): add raw SELECT query execution API with pagination#841
feat(admin): add raw SELECT query execution API with pagination#841nevil-mathew merged 12 commits intodevelopfrom
Conversation
nevil-mathew
changed the title
WalkthroughAdds an admin-only raw SQL execution flow: controller exposes Admin.executeRawQuery which delegates to AdminHelper.executeRawQuery; new rawQueryUtils validates and paginates queries; service runs paginated SELECT + COUNT and logs the action; authenticator short-circuits admin path when no override headers; i18n keys added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor AdminUser as Admin User
participant Controller as Admin Controller (v1)
participant Service as Admin Service / AdminHelper
participant Utils as rawQueryUtils
participant DB as Database
AdminUser->>Controller: POST /v1/admin/raw-query { query, pageNo, pageSize }
Controller->>Controller: verify admin context / set req.decodedToken
alt not admin
Controller-->>AdminUser: 4xx (USER_IS_NOT_A_ADMIN)
else admin
Controller->>Service: executeRawQuery(query, adminId, pageNo, pageSize)
Service->>Utils: validateQuerySecurity(query)
Utils-->>Service: OK / throws (invalid/forbidden)
Service->>Utils: getPaginationParams(pageNo,pageSize)
Utils-->>Service: { limit, offset }
Service->>DB: Execute SELECT ... LIMIT {limit} OFFSET {offset}
DB-->>Service: rows[]
Service->>DB: SELECT COUNT(*) FROM (original_query) AS sub
DB-->>Service: totalCount
Service-->>Controller: { success, data: rows, count: totalCount }
Controller-->>AdminUser: 200 QUERY_EXECUTED_SUCCESSFULLY
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 7
🧹 Nitpick comments (1)
src/services/admin.js (1)
849-849: Avoid logging full raw SQL to stdoutUse structured logging and consider masking/shortening queries. Console logs can leak sensitive data into logs.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/controllers/v1/admin.js(1 hunks)src/locales/en.json(1 hunks)src/services/admin.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/controllers/**
⚙️ CodeRabbit configuration file
These are API controllers. Focus on request/response handling, security (auth middleware usage), and proper status codes.
Files:
src/controllers/v1/admin.js
src/services/**
⚙️ CodeRabbit configuration file
This is core business logic. Please check for correctness, efficiency, and potential edge cases.
Files:
src/services/admin.js
🧬 Code graph analysis (2)
src/controllers/v1/admin.js (2)
src/services/admin.js (3)
adminService(30-30)query(318-323)query(469-474)src/generics/materializedViews.js (2)
adminService(422-427)query(402-402)
src/services/admin.js (2)
src/controllers/v1/admin.js (4)
pageNo(222-222)pageSize(223-223)responses(13-13)httpStatusCode(11-11)src/helpers/responses.js (1)
error(28-28)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/services/admin.js (1)
877-886: Don't expose raw DB error messages; use locale keys for validation errors.Line 880 returns
error.messagewhich may expose:
- Database schema details
- Internal error stack traces
- SQL syntax in error messages
Additionally, lines 881-884 use fragile string matching (
error.message.includes('QUERY_')) to distinguish client vs server errors. This breaks if error messages change.Apply this pattern instead:
} catch (error) { console.error('Error executing raw query:', error) + + // Map known validation errors to appropriate status codes + const knownErrors = new Set([ + 'INVALID_QUERY_INPUT', + 'ONLY_SELECT_QUERIES_ALLOWED', + 'QUERY_INVALID_OR_UNBALANCED_QUOTES', + 'QUERY_FORBIDDEN_INJECTION_PATTERNS', + 'QUERY_FORBIDDEN_PATTERNS' + ]) + + const isClientError = knownErrors.has(error.message) + return responses.failureResponse({ - message: error.message || 'QUERY_EXECUTION_FAILED', - statusCode: error.message.includes('QUERY_') - ? httpStatusCode.bad_request - : httpStatusCode.internal_server_error, - responseCode: error.message.includes('QUERY_') ? 'CLIENT_ERROR' : 'SERVER_ERROR', + message: isClientError ? error.message : 'QUERY_EXECUTION_FAILED', + statusCode: isClientError + ? httpStatusCode.bad_request + : httpStatusCode.internal_server_error, + responseCode: isClientError ? 'CLIENT_ERROR' : 'SERVER_ERROR', }) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/locales/en.json(1 hunks)src/services/admin.js(2 hunks)src/utils/rawQueryUtils.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/services/**
⚙️ CodeRabbit configuration file
This is core business logic. Please check for correctness, efficiency, and potential edge cases.
Files:
src/services/admin.js
🧠 Learnings (4)
📚 Learning: 2025-08-06T07:27:08.445Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#783
File: src/database/queries/fileUpload.js:40-59
Timestamp: 2025-08-06T07:27:08.445Z
Learning: In the ELEVATE-Project/user repository, pagination is handled by src/middlewares/pagination.js which validates page and limit parameters and sets req.pageNo and req.pageSize. Service methods like getBulkInvitesFilesList use these pre-validated values when calling database query functions like listUploads. Additional parameter validation in database query functions is redundant since the middleware handles validation upstream.
Applied to files:
src/services/admin.js
📚 Learning: 2025-08-06T07:27:08.445Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#783
File: src/database/queries/fileUpload.js:40-59
Timestamp: 2025-08-06T07:27:08.445Z
Learning: In the ELEVATE-Project/user repository, pagination parameters are validated by src/middlewares/pagination.js middleware which sets req.pageNo and req.pageSize. Service methods like getBulkInvitesFilesList use these pre-validated values when calling database query functions. Database query functions like listUploads receive already-validated pagination parameters, so additional validation in the query layer is redundant.
Applied to files:
src/services/admin.js
📚 Learning: 2025-08-06T07:28:13.285Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#783
File: src/database/queries/fileUpload.js:40-59
Timestamp: 2025-08-06T07:28:13.285Z
Learning: In the ELEVATE-Project/user repository, pagination parameters (page and limit) are validated in src/middlewares/pagination.js middleware which is applied globally to all routes. The middleware sets req.pageNo and req.pageSize with proper validation, so database query functions like listUploads don't need additional parameter validation.
Applied to files:
src/services/admin.js
📚 Learning: 2025-08-06T07:27:08.445Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#783
File: src/database/queries/fileUpload.js:40-59
Timestamp: 2025-08-06T07:27:08.445Z
Learning: The ELEVATE-Project/user repository uses a pagination middleware at src/middlewares/pagination.js that handles comprehensive validation of page and limit parameters, including type conversion, default values, and bounds checking. Database query functions like listUploads don't need additional parameter validation since the middleware handles this upstream.
Applied to files:
src/services/admin.js
🧬 Code graph analysis (1)
src/utils/rawQueryUtils.js (1)
src/controllers/v1/admin.js (2)
pageNo(222-222)pageSize(223-223)
🔇 Additional comments (12)
src/services/admin.js (6)
820-839: LGTM! Input validation is appropriate.The method correctly validates that the query parameter is a non-empty string before processing.
841-845: LGTM! Query sanitization and security validation are correctly delegated.Stripping trailing semicolons before validation prevents simple SQL injection attempts, and delegating to
rawQueryUtils.validateQuerySecurityensures comprehensive security checks.
848-848: LGTM! Query logging provides audit trail.Logging admin ID and query details supports security auditing and troubleshooting.
850-859: LGTM! Query execution uses parameterized bindings and timeout.Using
replacementsfor LIMIT/OFFSET prevents SQL injection, and the 30-second timeout prevents long-running queries from blocking resources.
861-867: LGTM! Count query correctly uses sanitized subquery.Wrapping the sanitized query in a subquery for COUNT ensures accurate pagination metadata without re-executing the full result set.
869-876: Verified locale key exists
QUERY_EXECUTED_SUCCESSFULLYis defined in src/locales/en.json (line 186).src/utils/rawQueryUtils.js (6)
1-34: LGTM! Security validation is comprehensive and defense-in-depth.The multi-layered approach validates:
- Query starts with SELECT or WITH
- Balanced quotes
- Critical injection patterns (semicolons, comments)
- Forbidden SQL operations (after normalization)
This defense-in-depth strategy provides robust protection against SQL injection.
42-48: LGTM! Pagination bounds prevent resource exhaustion.Capping page size at 1000 and ensuring positive values prevents unbounded queries. While the pagination middleware already validates these parameters (based on learnings), this defensive validation is acceptable.
Based on learnings
55-58: Note: Restrictive by design—blocks semicolons even in string literals.This function intentionally rejects queries containing semicolons, comments, or backslashes anywhere in the query—including inside string literals like
'O\'Brien'or'test;value'.While overly restrictive, this is a defensible security posture for admin tooling where the trade-off favors safety over flexibility. If legitimate use cases require these characters in literals, consider checking against the normalized query instead.
If you need to support these characters in string literals, apply this change:
function hasCriticalInjectionPatterns(lowerQuery) { - const criticalPatterns = [';', '--', '/*', '*/', '#', '\\'] - return criticalPatterns.some((pattern) => lowerQuery.includes(pattern)) + const criticalPatterns = [';', '--', '/*', '*/', '#'] + return criticalPatterns.some((pattern) => lowerQuery.includes(pattern)) }And move backslash detection to
normalizeQuerycontext, or document that backslashes in literals are intentionally blocked.
65-75: LGTM! Query normalization correctly strips comments and literals.The regex patterns correctly handle:
- Multi-line
/* */comments- Single-line
--comments- Hash
#comments- Escaped quotes (
''and"")This allows
hasForbiddenPatternsto detect DDL/DML keywords without false positives from comments or string literals.
82-85: LGTM! Forbidden pattern detection is appropriately scoped.Blocking DROP, ALTER, TRUNCATE, INSERT, UPDATE, and DELETE prevents destructive operations while allowing read-only SELECT and WITH queries.
92-116: LGTM! Quote validation correctly handles SQL escaping.The character-by-character scan properly handles escaped quotes (
''and""), ensuring balanced quote detection works even with complex string literals.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/controllers/v1/admin.js (1)
199-229: Consider consistency and validation improvements.Two observations:
Inconsistent error handling pattern (line 213): This method returns the failure response directly, while other admin methods in this controller (e.g.,
deleteUserline 27,addOrgAdminline 102,deactivateOrgline 148) throw it. For consistency, consider usingthrowhere as well.Missing query validation (line 220): The method extracts
queryfromreq.bodywithout verifying it exists. While the service layer validates the query type, adding a basic check here would provide clearer error messaging at the controller boundary.Apply this diff for consistency and clarity:
try { // Validate admin role if (!utilsHelper.validateRoleAccess(req.decodedToken.roles, common.ADMIN_ROLE)) { - return responses.failureResponse({ + throw responses.failureResponse({ message: 'USER_IS_NOT_A_ADMIN', statusCode: httpStatusCode.bad_request, responseCode: 'CLIENT_ERROR', }) } const { query } = req.body + + if (!query) { + throw responses.failureResponse({ + message: 'INVALID_QUERY_INPUT', + statusCode: httpStatusCode.bad_request, + responseCode: 'CLIENT_ERROR', + }) + } const pageNo = req.pageNo const pageSize = req.pageSizesrc/utils/rawQueryUtils.js (2)
55-58: Backslash pattern check may not work as intended.Line 56 includes
'\\'in thecriticalPatternsarray to check for backslashes. However,'\\'is a string containing a single backslash character (JavaScript escape sequence). When checking iflowerQuery.includes('\\'), this searches for the literal two-character sequence\+\in the lowercased query string, not a single backslash.For clarity and correctness:
- If you intend to block single backslashes in queries, the pattern is already correct (
'\\'represents one backslash)- If you intend to block the two-character sequence
\\, you'd need'\\\\'(four backslashes in source → two backslashes in string → matches\\in query)The current implementation is likely correct for blocking single backslashes (which could be used for escaping), but consider adding a comment for clarity.
Add a clarifying comment:
function hasCriticalInjectionPatterns(lowerQuery) { - const criticalPatterns = [';', '--', '/*', '*/', '#', '\\'] + const criticalPatterns = [ + ';', // Statement separator + '--', // SQL comment + '/*', // Multi-line comment start + '*/', // Multi-line comment end + '#', // MySQL comment + '\\', // Backslash (potential escape character) + ] return criticalPatterns.some((pattern) => lowerQuery.includes(pattern)) }
82-85: Overly restrictive forbidden patterns block legitimate SELECT queries.Line 83 blocks several operations that are valid and common in read-only SELECT queries:
- UNION: Standard SQL for combining result sets (e.g.,
SELECT * FROM users WHERE role='admin' UNION SELECT * FROM users WHERE role='manager')- CONCAT: String concatenation function (e.g.,
SELECT CONCAT(first_name, ' ', last_name) AS full_name FROM users)- || (double pipe): SQL standard string concatenation operator, also logical OR in some dialects (e.g.,
SELECT first_name || ' ' || last_name FROM users)While these can be used in injections, they're also essential for many legitimate analytical queries that admins might need to run. Blocking them significantly limits the utility of this feature.
Consider one of these approaches:
Option 1: Remove UNION, CONCAT, and || from the forbidden list, relying on other security layers (admin-only access, query logging, timeouts, read-only queries):
function hasForbiddenPatterns(normalizedQuery) { - const forbiddenPatterns = /\b(drop|alter|truncate|insert|update|delete|union|concat|\|\|)\b/i + const forbiddenPatterns = /\b(drop|alter|truncate|insert|update|delete)\b/i return forbiddenPatterns.test(normalizedQuery) }Option 2: Keep restrictive validation but document the limitation and consider future enhancement for allowlisting specific patterns:
+// Note: This blocks UNION, CONCAT, and || (pipe) to minimize injection risk. +// Consider allowlisting specific patterns if legitimate use cases require them. function hasForbiddenPatterns(normalizedQuery) { const forbiddenPatterns = /\b(drop|alter|truncate|insert|update|delete|union|concat|\|\|)\b/i return forbiddenPatterns.test(normalizedQuery) }I'd recommend Option 1 for a better admin experience, given the existing security measures (admin role validation, audit logging on line 852 of admin.js, and 30-second timeout on line 862).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/controllers/v1/admin.js(1 hunks)src/locales/en.json(1 hunks)src/services/admin.js(2 hunks)src/utils/rawQueryUtils.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/controllers/**
⚙️ CodeRabbit configuration file
These are API controllers. Focus on request/response handling, security (auth middleware usage), and proper status codes.
Files:
src/controllers/v1/admin.js
src/services/**
⚙️ CodeRabbit configuration file
This is core business logic. Please check for correctness, efficiency, and potential edge cases.
Files:
src/services/admin.js
🧠 Learnings (4)
📚 Learning: 2025-08-06T07:27:08.445Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#783
File: src/database/queries/fileUpload.js:40-59
Timestamp: 2025-08-06T07:27:08.445Z
Learning: In the ELEVATE-Project/user repository, pagination is handled by src/middlewares/pagination.js which validates page and limit parameters and sets req.pageNo and req.pageSize. Service methods like getBulkInvitesFilesList use these pre-validated values when calling database query functions like listUploads. Additional parameter validation in database query functions is redundant since the middleware handles validation upstream.
Applied to files:
src/controllers/v1/admin.jssrc/services/admin.js
📚 Learning: 2025-08-06T07:27:08.445Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#783
File: src/database/queries/fileUpload.js:40-59
Timestamp: 2025-08-06T07:27:08.445Z
Learning: In the ELEVATE-Project/user repository, pagination parameters are validated by src/middlewares/pagination.js middleware which sets req.pageNo and req.pageSize. Service methods like getBulkInvitesFilesList use these pre-validated values when calling database query functions. Database query functions like listUploads receive already-validated pagination parameters, so additional validation in the query layer is redundant.
Applied to files:
src/controllers/v1/admin.jssrc/services/admin.js
📚 Learning: 2025-08-06T07:28:13.285Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#783
File: src/database/queries/fileUpload.js:40-59
Timestamp: 2025-08-06T07:28:13.285Z
Learning: In the ELEVATE-Project/user repository, pagination parameters (page and limit) are validated in src/middlewares/pagination.js middleware which is applied globally to all routes. The middleware sets req.pageNo and req.pageSize with proper validation, so database query functions like listUploads don't need additional parameter validation.
Applied to files:
src/controllers/v1/admin.jssrc/services/admin.js
📚 Learning: 2025-08-06T07:27:08.445Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#783
File: src/database/queries/fileUpload.js:40-59
Timestamp: 2025-08-06T07:27:08.445Z
Learning: The ELEVATE-Project/user repository uses a pagination middleware at src/middlewares/pagination.js that handles comprehensive validation of page and limit parameters, including type conversion, default values, and bounds checking. Database query functions like listUploads don't need additional parameter validation since the middleware handles this upstream.
Applied to files:
src/controllers/v1/admin.jssrc/services/admin.js
🧬 Code graph analysis (3)
src/controllers/v1/admin.js (3)
src/services/admin.js (4)
common(15-15)responses(39-39)httpStatusCode(16-16)adminService(31-31)src/generics/materializedViews.js (1)
adminService(422-427)src/helpers/responses.js (1)
error(28-28)
src/utils/rawQueryUtils.js (1)
src/controllers/v1/admin.js (2)
pageNo(222-222)pageSize(223-223)
src/services/admin.js (1)
src/controllers/v1/admin.js (4)
pageNo(222-222)pageSize(223-223)responses(13-13)httpStatusCode(11-11)
🔇 Additional comments (1)
src/locales/en.json (1)
186-192: LGTM! All required locale keys are present.The seven new localization keys comprehensively cover all error scenarios and success cases for the raw query execution feature. The keys align with the validation logic in
rawQueryUtils.jsand the response messages inadmin.js.
|
@coderabbitai, do a full review |
|
@aks30, I'll perform a comprehensive review of this PR. Given that this feature introduces raw SQL query execution capabilities, I'll pay special attention to:
I'll provide detailed feedback shortly. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/middlewares/authenticator.js (1)
224-262: Fix missing req.decodedToken assignment in override header path.When an admin provides override headers,
decodedToken.datais modified (lines 256-258) but never assigned toreq.decodedToken. This causes downstream controllers to fail with undefined errors when accessingreq.decodedToken.Apply this diff to assign
req.decodedTokenafter overriding:// Override the values from the token with sanitized header values and fetched orgId decodedToken.data.tenant_code = tenantCode decodedToken.data.organization_id = org.id // Use the ID from the database decodedToken.data.organization_code = orgCode - } else { - req.decodedToken = decodedToken.data } + req.decodedToken = decodedToken.data return next() }
🧹 Nitpick comments (1)
src/controllers/v1/admin.js (1)
211-211: Add input validation for query parameter.Validate that
queryis non-empty before proceeding to prevent unnecessary service calls and provide clearer error messages.Apply this diff:
const { query } = req.body + + if (!query || typeof query !== 'string' || !query.trim()) { + throw responses.failureResponse({ + message: 'INVALID_QUERY_INPUT', + statusCode: httpStatusCode.bad_request, + responseCode: 'CLIENT_ERROR', + }) + } const pageNo = req.pageNo
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/controllers/v1/admin.js(1 hunks)src/middlewares/authenticator.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/controllers/**
⚙️ CodeRabbit configuration file
These are API controllers. Focus on request/response handling, security (auth middleware usage), and proper status codes.
Files:
src/controllers/v1/admin.js
src/middlewares/**
⚙️ CodeRabbit configuration file
This is security-sensitive middleware. Scrutinize for potential vulnerabilities.
Files:
src/middlewares/authenticator.js
🧠 Learnings (5)
📚 Learning: 2025-08-06T07:27:08.445Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#783
File: src/database/queries/fileUpload.js:40-59
Timestamp: 2025-08-06T07:27:08.445Z
Learning: In the ELEVATE-Project/user repository, pagination is handled by src/middlewares/pagination.js which validates page and limit parameters and sets req.pageNo and req.pageSize. Service methods like getBulkInvitesFilesList use these pre-validated values when calling database query functions like listUploads. Additional parameter validation in database query functions is redundant since the middleware handles validation upstream.
Applied to files:
src/controllers/v1/admin.js
📚 Learning: 2025-08-06T07:27:08.445Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#783
File: src/database/queries/fileUpload.js:40-59
Timestamp: 2025-08-06T07:27:08.445Z
Learning: In the ELEVATE-Project/user repository, pagination parameters are validated by src/middlewares/pagination.js middleware which sets req.pageNo and req.pageSize. Service methods like getBulkInvitesFilesList use these pre-validated values when calling database query functions. Database query functions like listUploads receive already-validated pagination parameters, so additional validation in the query layer is redundant.
Applied to files:
src/controllers/v1/admin.js
📚 Learning: 2025-08-06T07:28:13.285Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#783
File: src/database/queries/fileUpload.js:40-59
Timestamp: 2025-08-06T07:28:13.285Z
Learning: In the ELEVATE-Project/user repository, pagination parameters (page and limit) are validated in src/middlewares/pagination.js middleware which is applied globally to all routes. The middleware sets req.pageNo and req.pageSize with proper validation, so database query functions like listUploads don't need additional parameter validation.
Applied to files:
src/controllers/v1/admin.js
📚 Learning: 2025-08-06T07:27:08.445Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#783
File: src/database/queries/fileUpload.js:40-59
Timestamp: 2025-08-06T07:27:08.445Z
Learning: The ELEVATE-Project/user repository uses a pagination middleware at src/middlewares/pagination.js that handles comprehensive validation of page and limit parameters, including type conversion, default values, and bounds checking. Database query functions like listUploads don't need additional parameter validation since the middleware handles this upstream.
Applied to files:
src/controllers/v1/admin.js
📚 Learning: 2025-07-31T08:44:36.982Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#776
File: src/services/entities.js:18-23
Timestamp: 2025-07-31T08:44:36.982Z
Learning: In the ELEVATE-Project/user codebase, organizationCode and tenantCode parameters passed to service methods always come from req.decodedToken.organization_code and req.decodedToken.tenant_code, which are guaranteed to be present after token validation. Additional validation for these parameters in service methods is unnecessary as the token validation process ensures they are always available.
Applied to files:
src/middlewares/authenticator.js
🧬 Code graph analysis (2)
src/controllers/v1/admin.js (2)
src/services/admin.js (3)
adminService(31-31)query(319-324)query(470-475)src/generics/materializedViews.js (2)
adminService(422-427)query(402-402)
src/middlewares/authenticator.js (2)
src/services/account.js (1)
decodedToken(888-888)src/services/user-sessions.js (1)
decodedToken(264-264)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
src/services/admin.js (3)
11-11: Import QueryTypes to prevent runtime error.The code at lines 861 and 868 uses
sequelize.QueryTypes.SELECT, butQueryTypesis not imported at line 11 (onlyOpis imported). This will cause a runtime error when the method is called.Apply this diff:
-const { Op } = require('sequelize') +const { Op, QueryTypes } = require('sequelize')
859-870: Use imported QueryTypes constant.Once
QueryTypesis imported, replacesequelize.QueryTypes.SELECTwith the imported constant for consistency.Apply this diff:
const data = await sequelize.query(paginatedQuery, { replacements: { limit, offset }, - type: sequelize.QueryTypes.SELECT, + type: QueryTypes.SELECT, timeout: 30000, }) const countQuery = `SELECT COUNT(*) AS count FROM (${sanitizedQuery}) AS subquery` const [countResult] = await sequelize.query(countQuery, { - type: sequelize.QueryTypes.SELECT, + type: QueryTypes.SELECT, timeout: 30000, })
881-890: Don't expose database error details in API responses.Line 884 returns
error.messagein the API response, which can leak internal database schema, table names, or query syntax details to clients. Additionally, lines 885-888 use fragile string matching (error.message.includes('QUERY_')) to categorize errors.Apply this diff to log errors server-side and return generic messages:
} catch (error) { console.error('Error executing raw query:', error) + + // Validation errors from rawQueryUtils start with these patterns + const validationErrors = [ + 'INVALID_QUERY_INPUT', + 'ONLY_SELECT_QUERIES_ALLOWED', + 'QUERY_INVALID_OR_UNBALANCED_QUOTES', + 'QUERY_FORBIDDEN_INJECTION_PATTERNS', + 'QUERY_FORBIDDEN_PATTERNS', + ] + const isValidationError = validationErrors.includes(error.message) + return responses.failureResponse({ - message: error.message || 'QUERY_EXECUTION_FAILED', - statusCode: error.message.includes('QUERY_') - ? httpStatusCode.bad_request - : httpStatusCode.internal_server_error, - responseCode: error.message.includes('QUERY_') ? 'CLIENT_ERROR' : 'SERVER_ERROR', + message: isValidationError ? error.message : 'QUERY_EXECUTION_FAILED', + statusCode: isValidationError ? httpStatusCode.bad_request : httpStatusCode.internal_server_error, + responseCode: isValidationError ? 'CLIENT_ERROR' : 'SERVER_ERROR', }) }src/middlewares/authenticator.js (1)
259-262: Early return still bypasses permission checks for all admin users.The refactored else block at lines 259-261 now explicitly assigns
req.decodedToken = decodedToken.databefore returning, but both paths (with or without override headers) still skip role validation (lines 265-281) and permission checks (lines 283-294) for all admin users. The past review comment flagged this as a potential security concern. If admin users are intentionally exempt from these validations, add a comment explaining the design decision; otherwise, ensure admins also undergo the standard permission verification flow.src/controllers/v1/admin.js (1)
209-220: Add admin role validation before executing raw queries.This method lacks the admin role validation present in all other admin methods in this file (e.g., lines 26-32, 101-107, 147-153, 175-181). Raw query execution is a highly privileged operation that must be restricted to admin users only. This is a critical security gap.
Apply this diff:
async executeRawQuery(req) { try { + if (!utilsHelper.validateRoleAccess(req.decodedToken.roles, common.ADMIN_ROLE)) { + throw responses.failureResponse({ + message: 'USER_IS_NOT_A_ADMIN', + statusCode: httpStatusCode.bad_request, + responseCode: 'CLIENT_ERROR', + }) + } + const { query } = req.body const pageNo = req.pageNo const pageSize = req.pageSize return await adminService.executeRawQuery(query, req.decodedToken.id, pageNo, pageSize) } catch (error) { return error } }src/utils/rawQueryUtils.js (1)
42-48: Align pagination cap with middleware limit.Line 44 caps
validPageSizeat 1000, but the pagination middleware atsrc/middlewares/pagination.jscaps the limit at 100. This inconsistency allows the service layer to accept page sizes that bypass the middleware's intended constraint.Apply this diff to align with the middleware:
function getPaginationParams(pageNo, pageSize) { const validPageNo = Math.max(1, parseInt(pageNo) || 1) - const validPageSize = Math.min(Math.max(1, parseInt(pageSize) || 100), 1000) // Cap at 1000 + const validPageSize = Math.min(Math.max(1, parseInt(pageSize) || 100), 100) // Cap at 100 (aligned with pagination middleware) const offset = (validPageNo - 1) * validPageSize return { limit: validPageSize, offset }Based on learnings.
🧹 Nitpick comments (1)
src/utils/rawQueryUtils.js (1)
55-58: Consider adding test coverage for edge cases.The critical injection patterns check includes semicolons, comments, and backslashes. While comprehensive, consider adding unit tests to verify:
- Escaped quotes within string literals don't trigger false positives
- Multi-line queries with legitimate CTEs (WITH clauses) pass validation
- Various comment styles (`, /*, #) are properly detected
This would increase confidence in the validation logic for production use.
Do you want me to help generate unit test cases for these edge scenarios?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/controllers/v1/admin.js(1 hunks)src/locales/en.json(1 hunks)src/middlewares/authenticator.js(1 hunks)src/services/admin.js(2 hunks)src/utils/rawQueryUtils.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/services/**
⚙️ CodeRabbit configuration file
This is core business logic. Please check for correctness, efficiency, and potential edge cases.
Files:
src/services/admin.js
src/controllers/**
⚙️ CodeRabbit configuration file
These are API controllers. Focus on request/response handling, security (auth middleware usage), and proper status codes.
Files:
src/controllers/v1/admin.js
src/middlewares/**
⚙️ CodeRabbit configuration file
This is security-sensitive middleware. Scrutinize for potential vulnerabilities.
Files:
src/middlewares/authenticator.js
🧠 Learnings (4)
📚 Learning: 2025-08-06T07:27:08.445Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#783
File: src/database/queries/fileUpload.js:40-59
Timestamp: 2025-08-06T07:27:08.445Z
Learning: In the ELEVATE-Project/user repository, pagination is handled by src/middlewares/pagination.js which validates page and limit parameters and sets req.pageNo and req.pageSize. Service methods like getBulkInvitesFilesList use these pre-validated values when calling database query functions like listUploads. Additional parameter validation in database query functions is redundant since the middleware handles validation upstream.
Applied to files:
src/services/admin.jssrc/controllers/v1/admin.jssrc/utils/rawQueryUtils.js
📚 Learning: 2025-08-06T07:27:08.445Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#783
File: src/database/queries/fileUpload.js:40-59
Timestamp: 2025-08-06T07:27:08.445Z
Learning: In the ELEVATE-Project/user repository, pagination parameters are validated by src/middlewares/pagination.js middleware which sets req.pageNo and req.pageSize. Service methods like getBulkInvitesFilesList use these pre-validated values when calling database query functions. Database query functions like listUploads receive already-validated pagination parameters, so additional validation in the query layer is redundant.
Applied to files:
src/services/admin.jssrc/controllers/v1/admin.js
📚 Learning: 2025-08-06T07:28:13.285Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#783
File: src/database/queries/fileUpload.js:40-59
Timestamp: 2025-08-06T07:28:13.285Z
Learning: In the ELEVATE-Project/user repository, pagination parameters (page and limit) are validated in src/middlewares/pagination.js middleware which is applied globally to all routes. The middleware sets req.pageNo and req.pageSize with proper validation, so database query functions like listUploads don't need additional parameter validation.
Applied to files:
src/services/admin.jssrc/controllers/v1/admin.js
📚 Learning: 2025-08-06T07:27:08.445Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#783
File: src/database/queries/fileUpload.js:40-59
Timestamp: 2025-08-06T07:27:08.445Z
Learning: The ELEVATE-Project/user repository uses a pagination middleware at src/middlewares/pagination.js that handles comprehensive validation of page and limit parameters, including type conversion, default values, and bounds checking. Database query functions like listUploads don't need additional parameter validation since the middleware handles this upstream.
Applied to files:
src/services/admin.jssrc/controllers/v1/admin.js
🔇 Additional comments (3)
src/controllers/v1/admin.js (1)
213-214: Pagination parameters are correctly sourced from middleware.The code correctly uses
req.pageNoandreq.pageSize, which are pre-validated by the pagination middleware (src/middlewares/pagination.js) that runs globally before this controller method. No additional validation is needed here.Based on learnings.
src/locales/en.json (1)
186-192: All required locale keys are present and properly defined.The seven new locale keys support the raw query execution feature:
QUERY_EXECUTED_SUCCESSFULLY- used in the service success responseINVALID_QUERY_INPUT- validation error for malformed inputONLY_SELECT_QUERIES_ALLOWED- security validation in rawQueryUtilsQUERY_EXECUTION_FAILED- generic failure messageQUERY_INVALID_OR_UNBALANCED_QUOTES- quote validation in rawQueryUtilsQUERY_FORBIDDEN_INJECTION_PATTERNS- injection detection in rawQueryUtilsQUERY_FORBIDDEN_PATTERNS- forbidden operation detection in rawQueryUtilsAll keys align with the error codes thrown by
src/utils/rawQueryUtils.jsand messages returned bysrc/services/admin.js.src/utils/rawQueryUtils.js (1)
11-34: Validation logic is comprehensive and well-structured.The
validateQuerySecurityfunction implements defense-in-depth:
- Line 15: Ensures queries start with SELECT or WITH
- Line 20: Validates balanced quotes (detects injection via quote breaking)
- Line 25: Detects critical patterns (semicolons, comments, backslashes)
- Lines 30-32: Normalizes and checks for forbidden SQL operations
This layered approach effectively prevents common SQL injection vectors while allowing legitimate SELECT queries.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/services/admin.js (1)
872-879: Use the correct locale key.Line 874 uses
QUERY_EXECUTED_SUCCESSFULLY, but according to the past review comments and the locale keys added insrc/locales/en.json, the correct key should beRAW_QUERY_EXECUTED_SUCCESSFULLY.Apply this diff:
return responses.successResponse({ statusCode: httpStatusCode.ok, - message: 'QUERY_EXECUTED_SUCCESSFULLY', + message: 'RAW_QUERY_EXECUTED_SUCCESSFULLY', result: { data, count, }, })
🧹 Nitpick comments (1)
src/services/admin.js (1)
880-891: Refine error categorization for maintainability.The current error categorization logic (lines 882-886) is brittle:
startsWith('QUERY_')could incorrectly classify database errors that happen to start with "QUERY_".- Hardcoded string checks (
=== 'INVALID_QUERY_INPUT',=== 'ONLY_SELECT_QUERIES_ALLOWED') are fragile and require updates when new validation errors are added.Consider a more robust approach that explicitly checks against known validation errors or uses a custom error class.
Apply this diff for more maintainable error handling:
} catch (error) { console.error('Error executing raw query:', error) - const isValidationError = - error.message && - (error.message.startsWith('QUERY_') || - error.message === 'INVALID_QUERY_INPUT' || - error.message === 'ONLY_SELECT_QUERIES_ALLOWED') + // Known validation error messages from rawQueryUtils + const validationErrors = new Set([ + 'INVALID_QUERY_INPUT', + 'ONLY_SELECT_QUERIES_ALLOWED', + 'QUERY_INVALID_OR_UNBALANCED_QUOTES', + 'QUERY_FORBIDDEN_INJECTION_PATTERNS', + 'QUERY_FORBIDDEN_PATTERNS', + ]) + const isValidationError = error.message && validationErrors.has(error.message) + return responses.failureResponse({ message: isValidationError ? error.message : 'QUERY_EXECUTION_FAILED', statusCode: isValidationError ? httpStatusCode.bad_request : httpStatusCode.internal_server_error, responseCode: isValidationError ? 'CLIENT_ERROR' : 'SERVER_ERROR', }) }This approach:
- Explicitly lists all validation errors in one place
- Uses a Set for O(1) lookup
- Won't misclassify database errors
- Makes it clear which errors are user-facing vs. server errors
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/services/admin.js(2 hunks)src/utils/rawQueryUtils.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/services/**
⚙️ CodeRabbit configuration file
This is core business logic. Please check for correctness, efficiency, and potential edge cases.
Files:
src/services/admin.js
🧠 Learnings (4)
📚 Learning: 2025-08-06T07:27:08.445Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#783
File: src/database/queries/fileUpload.js:40-59
Timestamp: 2025-08-06T07:27:08.445Z
Learning: In the ELEVATE-Project/user repository, pagination is handled by src/middlewares/pagination.js which validates page and limit parameters and sets req.pageNo and req.pageSize. Service methods like getBulkInvitesFilesList use these pre-validated values when calling database query functions like listUploads. Additional parameter validation in database query functions is redundant since the middleware handles validation upstream.
Applied to files:
src/utils/rawQueryUtils.jssrc/services/admin.js
📚 Learning: 2025-08-06T07:27:08.445Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#783
File: src/database/queries/fileUpload.js:40-59
Timestamp: 2025-08-06T07:27:08.445Z
Learning: In the ELEVATE-Project/user repository, pagination parameters are validated by src/middlewares/pagination.js middleware which sets req.pageNo and req.pageSize. Service methods like getBulkInvitesFilesList use these pre-validated values when calling database query functions. Database query functions like listUploads receive already-validated pagination parameters, so additional validation in the query layer is redundant.
Applied to files:
src/services/admin.js
📚 Learning: 2025-08-06T07:28:13.285Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#783
File: src/database/queries/fileUpload.js:40-59
Timestamp: 2025-08-06T07:28:13.285Z
Learning: In the ELEVATE-Project/user repository, pagination parameters (page and limit) are validated in src/middlewares/pagination.js middleware which is applied globally to all routes. The middleware sets req.pageNo and req.pageSize with proper validation, so database query functions like listUploads don't need additional parameter validation.
Applied to files:
src/services/admin.js
📚 Learning: 2025-08-06T07:27:08.445Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#783
File: src/database/queries/fileUpload.js:40-59
Timestamp: 2025-08-06T07:27:08.445Z
Learning: The ELEVATE-Project/user repository uses a pagination middleware at src/middlewares/pagination.js that handles comprehensive validation of page and limit parameters, including type conversion, default values, and bounds checking. Database query functions like listUploads don't need additional parameter validation since the middleware handles this upstream.
Applied to files:
src/services/admin.js
🧬 Code graph analysis (1)
src/services/admin.js (1)
src/controllers/v1/admin.js (4)
pageNo(213-213)pageSize(214-214)responses(13-13)httpStatusCode(11-11)
🔇 Additional comments (8)
src/services/admin.js (2)
18-18: LGTM!The import of
rawQueryUtilsis correctly structured and used appropriately in theexecuteRawQuerymethod.
824-870: LGTM! Solid security and execution logic.The implementation demonstrates strong security practices:
- Input validation: Checks query is a string before processing.
- Query sanitization: Strips trailing semicolons to prevent query chaining.
- Security validation: Delegates to
rawQueryUtils.validateQuerySecurityfor comprehensive checks.- SQL injection prevention: Uses replacements for
limitandoffsetparameters.- Long-running query protection: Applies 30-second timeout to both queries.
- Pagination: Leverages middleware-validated
pageNoandpageSizevalues.- Count coercion: Line 870 ensures count is a number, preventing NaN issues.
The separation of concerns (security validation in utils, execution in service) is well-designed.
Based on learnings.
src/utils/rawQueryUtils.js (6)
11-34: LGTM! Comprehensive security validation.The
validateQuerySecurityfunction implements defense-in-depth with multiple layers:
- Query type validation: Ensures only SELECT or WITH queries (line 15).
- Quote validation: Prevents unbalanced quotes that could break parsing (line 20).
- Critical injection patterns: Blocks semicolons, comments, and backslashes (line 25).
- Forbidden operations: Normalizes query and checks for DDL/DML verbs (lines 30-32).
Each validation layer throws a distinct error message, making it easy to diagnose issues. The approach is thorough and well-designed.
42-48: LGTM! Pagination cap aligns with middleware.The pagination parameters are properly validated:
- pageNo: Clamped to minimum 1, defaults to 1 if invalid.
- pageSize: Clamped between 1 and 100, defaults to 100 if invalid.
- Offset calculation: Correctly computed as
(pageNo - 1) * pageSize.The cap of 100 at line 44 aligns with the middleware limit in
src/middlewares/pagination.js, ensuring consistent behavior across the application.Based on learnings.
55-58: LGTM! Effective injection pattern detection.The function detects critical SQL injection vectors:
- Semicolon: Prevents multi-statement execution.
- Comment syntax (
--,/*,*/,#): Prevents comment-based attacks.- Backslash: Prevents escape-based manipulation.
The simple includes-based approach is efficient and catches common attack patterns before more expensive normalization.
65-75: LGTM! Effective query normalization.The function removes content that should be excluded from pattern analysis:
- Block comments:
/* ... */(line 68)- Line comments:
--and#(lines 69-70)- String literals: Single and double quoted strings with escape handling (lines 71-72)
- Whitespace normalization: Collapses multiple spaces (line 73)
This ensures that forbidden patterns are detected in the actual SQL structure, not within comments or string literals. The regex patterns correctly handle escaped quotes (
''and"").
82-85: LGTM! Comprehensive forbidden pattern detection.The regex at line 83 blocks dangerous SQL operations:
- DDL operations:
drop,alter,truncate(prevents schema changes)- DML operations:
insert,update,delete(prevents data modification)- Exfiltration techniques:
union,concat,||(prevents UNION-based injection and concatenation tricks)The use of word boundaries (
\b) ensures the pattern matches complete keywords only, avoiding false positives (e.g., won't flag column names likedrop_date). The case-insensitive flag provides robust matching.
92-116: LGTM! Robust quote validation.The function validates balanced quotes with proper escape handling:
- State tracking: Maintains separate flags for single and double quotes (lines 93-94).
- Escape handling: Correctly handles SQL standard escaping (
''for single quotes,""for double quotes) by skipping the next character (lines 101-103, 107-109).- Quote nesting: Prevents quote type switching when inside another quote type (lines 100, 106).
- Balance check: Returns true only if both quote types are closed (line 115).
This prevents unbalanced quote injection attacks where an attacker could break out of a string context. The logic correctly handles the SQL standard escape convention (doubled quotes) rather than backslash escaping.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/controllers/v1/admin.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/controllers/**
⚙️ CodeRabbit configuration file
These are API controllers. Focus on request/response handling, security (auth middleware usage), and proper status codes.
Files:
src/controllers/v1/admin.js
🧠 Learnings (4)
📚 Learning: 2025-08-06T07:27:08.445Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#783
File: src/database/queries/fileUpload.js:40-59
Timestamp: 2025-08-06T07:27:08.445Z
Learning: In the ELEVATE-Project/user repository, pagination is handled by src/middlewares/pagination.js which validates page and limit parameters and sets req.pageNo and req.pageSize. Service methods like getBulkInvitesFilesList use these pre-validated values when calling database query functions like listUploads. Additional parameter validation in database query functions is redundant since the middleware handles validation upstream.
Applied to files:
src/controllers/v1/admin.js
📚 Learning: 2025-08-06T07:27:08.445Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#783
File: src/database/queries/fileUpload.js:40-59
Timestamp: 2025-08-06T07:27:08.445Z
Learning: In the ELEVATE-Project/user repository, pagination parameters are validated by src/middlewares/pagination.js middleware which sets req.pageNo and req.pageSize. Service methods like getBulkInvitesFilesList use these pre-validated values when calling database query functions. Database query functions like listUploads receive already-validated pagination parameters, so additional validation in the query layer is redundant.
Applied to files:
src/controllers/v1/admin.js
📚 Learning: 2025-08-06T07:28:13.285Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#783
File: src/database/queries/fileUpload.js:40-59
Timestamp: 2025-08-06T07:28:13.285Z
Learning: In the ELEVATE-Project/user repository, pagination parameters (page and limit) are validated in src/middlewares/pagination.js middleware which is applied globally to all routes. The middleware sets req.pageNo and req.pageSize with proper validation, so database query functions like listUploads don't need additional parameter validation.
Applied to files:
src/controllers/v1/admin.js
📚 Learning: 2025-08-06T07:27:08.445Z
Learnt from: nevil-mathew
PR: ELEVATE-Project/user#783
File: src/database/queries/fileUpload.js:40-59
Timestamp: 2025-08-06T07:27:08.445Z
Learning: The ELEVATE-Project/user repository uses a pagination middleware at src/middlewares/pagination.js that handles comprehensive validation of page and limit parameters, including type conversion, default values, and bounds checking. Database query functions like listUploads don't need additional parameter validation since the middleware handles this upstream.
Applied to files:
src/controllers/v1/admin.js
🧬 Code graph analysis (1)
src/controllers/v1/admin.js (3)
src/services/admin.js (6)
common(15-15)responses(39-39)httpStatusCode(16-16)adminService(31-31)query(319-324)query(470-475)src/generics/materializedViews.js (2)
adminService(422-427)query(402-402)src/helpers/responses.js (1)
error(28-28)
🔇 Additional comments (1)
src/controllers/v1/admin.js (1)
211-217: Admin role validation is correctly implemented.The method properly validates admin access using
utilsHelper.validateRoleAccess, consistent with other admin methods in this controller. This addresses the security concern from previous reviews.Based on learnings from the pagination middleware implementation and the authenticator flow.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/validators/v1/admin.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/validators/**
⚙️ CodeRabbit configuration file
Validate all incoming data thoroughly. Check for missing or incomplete validation rules.
Files:
src/validators/v1/admin.js
🧬 Code graph analysis (1)
src/validators/v1/admin.js (1)
src/controllers/v1/admin.js (1)
req(219-219)
2 participants
Summary by CodeRabbit
New Features
Chores