prevent SQL injection by using parameterized queries.#709
Open
hitakshiA wants to merge 1 commit intotekdi:mainfrom
Open
prevent SQL injection by using parameterized queries.#709hitakshiA wants to merge 1 commit intotekdi:mainfrom
hitakshiA wants to merge 1 commit intotekdi:mainfrom
Conversation
…ing interpolation in SQL queries with parameterized queries (, , etc.) across fields.service.ts, tenant.service.ts, and user.service.ts to prevent SQL injection attacks (OWASP A03:2021). 13 vulnerable query instances fixed.
WalkthroughThis pull request converts SQL queries across three service files from vulnerable string interpolation to parameterized SQL queries using positional placeholders, preventing SQL injection attacks. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
hitakshiA
changed the title
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fields/fields.service.ts`:
- Around line 399-405: The SQL concatenates request-derived identifiers (e.g.
sourceDetails.table, tableName, whereClause, order) into raw queries used by
fieldsValuesRepository.query, leaving SQL injection risk; fix by validating and
whitelisting table and column identifiers (e.g. enforce sourceDetails.table
against an allow-list or enum and map where/order options to predefined clauses)
or switch to a safe query builder (e.g. knex/TypeORM QueryBuilder) instead of
string concatenation, then pass only literal values via $n parameters (e.g. the
sourceFieldName["value"]); apply the same validation/whitelisting approach to
the other occurrences you flagged (lines ~538-545, 700-724, 738-750, 1402-1403,
1501-1503) and replace any raw fragment interpolation with mapped safe fragments
or builder methods before calling fieldsValuesRepository.query.
- Around line 68-79: The whereClause currently appends an independent OR clause
for contextType which allows matching that contextType across any context;
instead, when both requiredData.context and requiredData.contextType are
provided, combine them into a single predicate so contextType is scoped to that
context (e.g. append OR (context = $i AND "contextType" = $j) or build
conditions via an array and join with proper parentheses). Update the logic
around whereClause/params in the same block (references: whereClause, params,
requiredData.context, requiredData.contextType, getFieldData) so that the
contextType clause is never emitted standalone if a context value exists. Ensure
parameter indices remain correct when pushing combined conditions before calling
this.getFieldData.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3a86a111-4696-4e2b-85c0-936888ee0cb9
📒 Files selected for processing (3)
src/fields/fields.service.tssrc/tenant/tenant.service.tssrc/user/user.service.ts
Comment on lines
+68
to
+79
| const params = []; | ||
| if (requiredData.context) { | ||
| whereClause += ` OR context = '${requiredData.context}' AND "contextType" IS NULL`; | ||
| params.push(requiredData.context); | ||
| whereClause += ` OR context = $${params.length} AND "contextType" IS NULL`; | ||
| } | ||
|
|
||
| if (requiredData.contextType) { | ||
| whereClause += ` OR "contextType" = '${requiredData.contextType}'`; | ||
| params.push(requiredData.contextType); | ||
| whereClause += ` OR "contextType" = $${params.length}`; | ||
| } | ||
|
|
||
| const data = await this.getFieldData(whereClause); | ||
| const data = await this.getFieldData(whereClause, params); |
There was a problem hiding this comment.
Keep contextType scoped to the requested context.
When both filters are present, Line 76 turns the predicate into a standalone OR "contextType" = $n, so this query can pull fields from other contexts that happen to share the same contextType. That broadens getFormCustomField() beyond the requested schema.
🛠️ Suggested fix
- const params = [];
- if (requiredData.context) {
- params.push(requiredData.context);
- whereClause += ` OR context = $${params.length} AND "contextType" IS NULL`;
- }
-
- if (requiredData.contextType) {
- params.push(requiredData.contextType);
- whereClause += ` OR "contextType" = $${params.length}`;
- }
-
- const data = await this.getFieldData(whereClause, params);
+ const params = [];
+ const clauses = ['(context IS NULL AND "contextType" IS NULL)'];
+
+ if (requiredData.context) {
+ params.push(requiredData.context);
+ clauses.push(`(context = $${params.length} AND "contextType" IS NULL)`);
+ }
+
+ if (requiredData.contextType) {
+ if (requiredData.context) {
+ params.push(requiredData.context, requiredData.contextType);
+ clauses.push(`(context = $${params.length - 1} AND "contextType" = $${params.length})`);
+ } else {
+ params.push(requiredData.contextType);
+ clauses.push(`("contextType" = $${params.length})`);
+ }
+ }
+
+ const data = await this.getFieldData(clauses.join(" OR "), params);📝 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 params = []; | |
| if (requiredData.context) { | |
| whereClause += ` OR context = '${requiredData.context}' AND "contextType" IS NULL`; | |
| params.push(requiredData.context); | |
| whereClause += ` OR context = $${params.length} AND "contextType" IS NULL`; | |
| } | |
| if (requiredData.contextType) { | |
| whereClause += ` OR "contextType" = '${requiredData.contextType}'`; | |
| params.push(requiredData.contextType); | |
| whereClause += ` OR "contextType" = $${params.length}`; | |
| } | |
| const data = await this.getFieldData(whereClause); | |
| const data = await this.getFieldData(whereClause, params); | |
| const params = []; | |
| const clauses = ['(context IS NULL AND "contextType" IS NULL)']; | |
| if (requiredData.context) { | |
| params.push(requiredData.context); | |
| clauses.push(`(context = $${params.length} AND "contextType" IS NULL)`); | |
| } | |
| if (requiredData.contextType) { | |
| if (requiredData.context) { | |
| params.push(requiredData.context, requiredData.contextType); | |
| clauses.push(`(context = $${params.length - 1} AND "contextType" = $${params.length})`); | |
| } else { | |
| params.push(requiredData.contextType); | |
| clauses.push(`("contextType" = $${params.length})`); | |
| } | |
| } | |
| const data = await this.getFieldData(clauses.join(" OR "), params); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fields/fields.service.ts` around lines 68 - 79, The whereClause currently
appends an independent OR clause for contextType which allows matching that
contextType across any context; instead, when both requiredData.context and
requiredData.contextType are provided, combine them into a single predicate so
contextType is scoped to that context (e.g. append OR (context = $i AND
"contextType" = $j) or build conditions via an array and join with proper
parentheses). Update the logic around whereClause/params in the same block
(references: whereClause, params, requiredData.context,
requiredData.contextType, getFieldData) so that the contextType clause is never
emitted standalone if a context value exists. Ensure parameter indices remain
correct when pushing combined conditions before calling this.getFieldData.
Comment on lines
+399
to
+405
| const query = `SELECT "name", "value" | ||
| FROM public.${fieldsData.sourceDetails.table} | ||
| WHERE value = $1 | ||
| GROUP BY "name", "value"`; | ||
|
|
||
| const checkSourceData = await this.fieldsValuesRepository.query( | ||
| query | ||
| query, [sourceFieldName["value"]] |
There was a problem hiding this comment.
These queries are still injectable through raw identifiers/fragments.
$n placeholders only protect literal values. The touched lines still splice sourceDetails.table / tableName / whereClause / order directly into the SQL text, and in this service those values are request-derived or persisted from earlier requests. The injection surface is still open even after parameterizing the value operands.
🔒 Hardening pattern
+ private readonly ALLOWED_SOURCE_TABLES = new Set([
+ "state",
+ "district",
+ "block",
+ "village",
+ // add every supported source table here
+ ]);
+
+ private getSafeSourceTable(tableName: string): string {
+ if (!this.ALLOWED_SOURCE_TABLES.has(tableName)) {
+ throw new Error(`Unsupported source table: ${tableName}`);
+ }
+ return `"${tableName}"`;
+ }Use that helper anywhere a table name is currently interpolated, and build WHERE / ORDER BY from validated enums instead of accepting raw SQL fragments.
Also applies to: 538-545, 700-724, 738-750, 1402-1403, 1501-1503
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fields/fields.service.ts` around lines 399 - 405, The SQL concatenates
request-derived identifiers (e.g. sourceDetails.table, tableName, whereClause,
order) into raw queries used by fieldsValuesRepository.query, leaving SQL
injection risk; fix by validating and whitelisting table and column identifiers
(e.g. enforce sourceDetails.table against an allow-list or enum and map
where/order options to predefined clauses) or switch to a safe query builder
(e.g. knex/TypeORM QueryBuilder) instead of string concatenation, then pass only
literal values via $n parameters (e.g. the sourceFieldName["value"]); apply the
same validation/whitelisting approach to the other occurrences you flagged
(lines ~538-545, 700-724, 738-750, 1402-1403, 1501-1503) and replace any raw
fragment interpolation with mapped safe fragments or builder methods before
calling fieldsValuesRepository.query.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaced raw string interpolation in SQL queries with parameterized queries (
$1,$2) across 3 files to prevent SQL injection (OWASP A03:2021).What changed
src/fields/fields.service.ts— 11 queries parameterized (SELECT, INSERT, UPDATE, DELETE)src/tenant/tenant.service.ts— 1 query parameterizedsrc/user/user.service.ts— 1 query parameterizedBefore
typescript
const query =
SELECT * FROM public."Roles" WHERE "tenantId" = '${tenantData.tenantId}';await this.tenantRepository.query(query);
After
const query =
SELECT * FROM public."Roles" WHERE "tenantId" = $1;await this.tenantRepository.query(query, [tenantData.tenantId]
Summary by CodeRabbit
Chores