Skip to content

Bad way for supporting long ins - #54

Open
iced wants to merge 3 commits into
treeder:mainfrom
iced:main
Open

Bad way for supporting long ins#54
iced wants to merge 3 commits into
treeder:mainfrom
iced:main

Conversation

@iced

@iced iced commented Apr 14, 2026

Copy link
Copy Markdown
Collaborator

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.

@iced
iced requested a review from treeder April 14, 2026 16:02

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread d1.js
Comment on lines +365 to +368
const rawIn = this.toValues(q2[2])
.map((v) => `'${v.replace("'", '')}'`) // prevents injections
.join(',')
w.push(` ${q0} IN (${rawIn})`)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

security-critical critical

This implementation introduces a critical SQL injection vulnerability and potential runtime errors:

  1. 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.
  2. Runtime Error: this.toValues() can return non-string types (e.g., numbers from booleans, or null). Calling .replace() on a non-string will throw a TypeError.
  3. Data Corruption: Removing single quotes from values changes the actual data being queried (e.g., O'Reilly becomes OReilly).

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.

Suggested change
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 + ')')

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.

1 participant