Bad way for supporting long ins - #54
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the package version to 3.7.8 and modifies the SQL generation logic in d1.js to manually construct IN clauses. A critical security vulnerability was identified in this change, as the manual string manipulation used to prevent SQL injection is insufficient and only handles the first occurrence of a single quote. Additionally, the implementation lacks type checking for non-string values, which could lead to runtime errors. A code suggestion has been provided to properly escape single quotes and handle various data types correctly.
| const rawIn = this.toValues(q2[2]) | ||
| .map((v) => `'${v.replace("'", '')}'`) // prevents injections | ||
| .join(',') | ||
| w.push(` ${q0} IN (${rawIn})`) |
There was a problem hiding this comment.
This implementation introduces a critical SQL injection vulnerability and potential runtime errors:
- SQL Injection: The use of
.replace("'", '')only removes the first occurrence of a single quote. An attacker can exploit this by providing a string with multiple quotes (e.g.,val''); --) to break out of the string literal and execute arbitrary SQL. - Runtime Error:
this.toValues()can return non-string types (e.g., numbers from booleans, or null). Calling.replace()on a non-string will throw aTypeError. - Data Corruption: Removing single quotes from values changes the actual data being queried (e.g.,
O'ReillybecomesOReilly).
While bypassing parameterized queries is discouraged, if it is necessary to handle D1's parameter limits, you must properly escape strings by doubling single quotes ('') and handle non-string types correctly.
| const rawIn = this.toValues(q2[2]) | |
| .map((v) => `'${v.replace("'", '')}'`) // prevents injections | |
| .join(',') | |
| w.push(` ${q0} IN (${rawIn})`) | |
| const rawIn = this.toValues(q2[2]) | |
| .map((v) => typeof v === 'string' ? "'" + v.replace(/'/g, "''") + "'" : (v === null ? 'NULL' : v)) | |
| .join(',') | |
| w.push(' ' + q0 + ' IN (' + rawIn + ')') |
cloudflare/workers-sdk#2922 - not supported directly :(
Building query manually and removing ' from all in params so it'll not escape from str. Yes, it's really bad but will improve performance significantly.