fix(mysql)!: use index identifiers instead of raw SQL in QB.useIndex()

[eddieran]

Summary

Fixes #12333. Replaces #12336 which was accidentally closed when the fork was deleted.

User-provided identifiers passed to useIndex() and setLock() lockTables were interpolated directly into SQL strings without escaping, allowing SQL injection.

Changes

• useIndex(): Wrap the index name with this.escape() so it is quoted with the driver's identifier escape character (e.g. backticks for MySQL).
• setLock() lockTables: Wrap each table name with this.escape() before joining into the OF clause (Postgres).

Both changes use the existing QueryBuilder.escape() method which delegates to the driver's escape implementation, consistent with how all other identifiers are handled throughout the query builder.

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (2) 📎 Requirement gaps (0)

1. useIndex() arrays undocumented ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

useIndex() now accepts string[], which is a user-facing API enhancement, but the public
QueryBuilder docs still only show a single string usage. This can lead to users missing the new
supported usage and relying on outdated documentation.

Code

src/query-builder/SelectQueryBuilder.ts[R1529-1536]

+     * Set certain index(es) to be used by the query.
*
-     * @param index Name of index to be used.
+     * @param indexes Name(s) of index(es) to be used.
*/
-    useIndex(index: string): this {
-        this.expressionMap.useIndex = index
+    useIndex(indexes: string | string[]): this {
+        this.expressionMap.useIndex = Array.isArray(indexes)
+            ? indexes
+            : [indexes]

Evidence

PR code changes extend useIndex() to accept string | string[], which is a user-facing API/usage
change, while the QueryBuilder documentation section for custom index usage shows only a
single-string example and does not describe the array form.

Rule 2: Docs updated for user-facing changes
src/query-builder/SelectQueryBuilder.ts[1529-1536]
docs/docs/query-builder/1-select-query-builder.md[1022-1032]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`useIndex()` now accepts an array of index names (`string[]`), but the public documentation doesn't mention the array form.
## Issue Context
The QueryBuilder docs currently show only `.useIndex("my_index")`, which is still valid but omits the newly supported multi-index usage.
## Fix Focus Areas
- src/query-builder/SelectQueryBuilder.ts[1529-1536]
- docs/docs/query-builder/1-select-query-builder.md[1022-1032]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. return skips driver loop ✓ Resolved 📘 Rule violation ☼ Reliability

Description

The new SQL-injection tests return from the test body when a non-target driver is encountered,
which can prematurely exit the loop and skip running assertions for later eligible drivers. This
reduces functional test coverage and can let regressions slip through depending on datasource
ordering.

Code

test/functional/query-builder/sql-injection/sql-injection.test.ts[R299-301]

+                if (!DriverUtils.isMySQLFamily(dataSource.driver)) {
+                    return
+                }

Evidence

PR Compliance ID 4 requires avoiding abnormal defensive checks/inconsistent style; using return
inside a for (const dataSource of dataSources) loop aborts the whole test rather than skipping
only the current datasource, making the new tests unreliable.

Rule 4: Remove AI-generated noise
test/functional/query-builder/sql-injection/sql-injection.test.ts[299-301]
test/functional/query-builder/sql-injection/sql-injection.test.ts[317-319]
test/functional/query-builder/sql-injection/sql-injection.test.ts[332-334]
test/functional/query-builder/sql-injection/sql-injection.test.ts[352-354]
test/functional/query-builder/sql-injection/sql-injection.test.ts[370-372]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new tests iterate `for (const dataSource of dataSources)` but use `return` when a datasource is not the expected driver family, which exits the entire test early and may skip assertions for later matching datasources.
## Issue Context
This can make the added security regression tests pass without actually testing the relevant drivers, depending on datasource ordering.
## Fix Focus Areas
- test/functional/query-builder/sql-injection/sql-injection.test.ts[299-301]
- test/functional/query-builder/sql-injection/sql-injection.test.ts[317-319]
- test/functional/query-builder/sql-injection/sql-injection.test.ts[332-334]
- test/functional/query-builder/sql-injection/sql-injection.test.ts[352-354]
- test/functional/query-builder/sql-injection/sql-injection.test.ts[370-372]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Double-escaped lockTables ✓ Resolved 🐞 Bug ≡ Correctness

Description

createLockExpression() now escapes every expressionMap.lockTables entry, but the find-options
path pre-escapes lock table aliases before storing them, so the same identifier gets escaped twice
and Postgres will error that the relation in the FOR UPDATE OF clause is not in the FROM clause.
This breaks Repository.findOne({ lock: { ..., tables: [...] } }) on Postgres-family drivers.

Code

src/query-builder/SelectQueryBuilder.ts[R2802-2806]

+            lockTablesClause =
+                " OF " +
+                this.expressionMap.lockTables
+                    .map((table) => this.escape(table))
+                    .join(", ")

Evidence

In createLockExpression(), the PR change maps this.expressionMap.lockTables through
this.escape() before joining into the OF clause, so the stored strings are always escaped at
render time. Separately, when applying findOptions.lock.tables, the builder already calls
this.escape(tableAlias.name) and passes those *escaped* strings into setLock(..., lockTables),
meaning expressionMap.lockTables contains quoted identifiers like "post". Since
QueryBuilder.escape() delegates to driver.escape() (which wraps in quotes and doubles embedded
quotes), escaping an already-quoted string produces a different identifier (e.g., "post" →
""post"" inside an outer quote), which will not match the actual FROM-clause alias. The repository
functional test suite uses findOne with lock.tables: ["post"], so this regression is exercised
in-repo.

src/query-builder/SelectQueryBuilder.ts[2789-2807]
src/query-builder/SelectQueryBuilder.ts[3359-3387]
src/query-builder/QueryBuilder.ts[570-583]
src/driver/postgres/PostgresDriver.ts[1059-1066]
test/functional/repository/find-options-locking/find-options-locking.test.ts[644-688]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`createLockExpression()` now escapes every `expressionMap.lockTables` entry. However, the `findOptions.lock.tables` path already escapes aliases before calling `setLock()`, so identifiers get escaped twice (e.g. `"post"` becomes `"""post"""`), breaking Postgres `FOR UPDATE OF` resolution.
### Issue Context
- `createLockExpression()` now does: `lockTables.map((t) => this.escape(t))`
- `applyFindOptions` builds `tableNames` via `return this.escape(tableAlias.name)` and passes them to `setLock()`
### Fix Focus Areas
- src/query-builder/SelectQueryBuilder.ts[3359-3387]
- src/query-builder/SelectQueryBuilder.ts[2789-2807]
### Proposed fix
Make `expressionMap.lockTables` consistently store *raw* alias names (unescaped). Concretely:
1. In the `findOptions.lock.tables` mapping, return `tableAlias.name` (not `this.escape(tableAlias.name)`).
2. Keep escaping centralized in `createLockExpression()` (the new behavior), so both `setLock(..., ["post"])` and `findOptions.lock.tables` produce the same correctly-escaped SQL.
### Validation
Ensure `test/functional/repository/find-options-locking/find-options-locking.test.ts` still passes (especially the cases using `lock: { ..., tables: ["post"] }`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (1)

4. useIndex() functional test outdated ✓ Resolved 📘 Rule violation ☼ Reliability

Description

The useIndex() SQL output is now identifier-escaped, but the existing functional test still
asserts the unescaped SQL string. This leaves the security fix without an updated functional
regression assertion and will likely break/undermine the test coverage.

Code

src/query-builder/SelectQueryBuilder.ts[2304]

+                useIndex = ` USE INDEX (${this.escape(this.expressionMap.useIndex)})`

Evidence

PR Compliance ID 3 requires issue fixes to be covered/updated in the functional test suite. The PR
changes useIndex to emit an escaped identifier, while the functional test still expects `USE INDEX
(my_index)` without escaping.

Rule 3: Prefer functional tests over per-issue tests
src/query-builder/SelectQueryBuilder.ts[2300-2306]
test/functional/query-builder/select/query-builder-select.test.ts[626-640]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`useIndex()` now escapes the index identifier via `this.escape(...)`, but the functional test still expects the prior unescaped SQL. Update the functional test to assert the escaped identifier so the fix is covered.
## Issue Context
The PR changes SQL generation for MySQL-family drivers to prevent SQL injection via user-provided identifiers.
## Fix Focus Areas
- test/functional/query-builder/select/query-builder-select.test.ts[626-640]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Breaking useIndex type change 🐞 Bug ⚙ Maintainability

Description

QueryExpressionMap.useIndex was changed from string to string[], which changes the public type
of QueryBuilder.expressionMap.useIndex and can break TypeScript consumer code that reads/assigns
it as a string. This is observable because QueryBuilder (and its expressionMap property) is
exported from the package entrypoint.

Code

src/query-builder/QueryExpressionMap.ts[175]

+    useIndex?: string[]

Evidence

The diff changes useIndex to an array, and expressionMap is a publicly accessible property on
the exported QueryBuilder, making this type change observable to library consumers.

src/query-builder/QueryExpressionMap.ts[168-176]
src/query-builder/QueryBuilder.ts[70-74]
src/index.ts[137-146]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`QueryExpressionMap.useIndex` is part of the public API surface via `QueryBuilder.expressionMap`. Changing it from `string` to `string[]` breaks downstream TypeScript code that treats it as a string.
### Issue Context
The PR needs multiple indexes and escaping, but this can be done without changing the exposed type of `expressionMap.useIndex`.
### Fix Focus Areas
- src/query-builder/QueryExpressionMap.ts[168-176]
- src/query-builder/SelectQueryBuilder.ts[1533-1537]
- src/query-builder/SelectQueryBuilder.ts[2237-2245]
### Implementation sketch
- Keep `QueryExpressionMap.useIndex?: string` (legacy) for compatibility.
- Add a new internal/expression-map field, e.g. `useIndexes?: string[]`, to store the normalized list.
- Update `SelectQueryBuilder.useIndex()` to populate `useIndexes` (copying the array defensively) and optionally set legacy `useIndex` to a comma-separated string for debugging/back-compat.
- Update SQL generation to prefer `useIndexes` when present, otherwise fall back to parsing legacy `useIndex`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. Empty USE INDEX entries ✓ Resolved 🐞 Bug ≡ Correctness

Description

createSelectExpression() now splits useIndex by commas and escapes each trimmed segment, but
does not remove empty segments, so inputs with trailing/duplicate commas generate USE INDEX with
an empty escaped identifier (e.g., ``). MySQL will reject such SQL at execution time.

Code

src/query-builder/SelectQueryBuilder.ts[R2291-2294]

+                useIndex = ` USE INDEX (${this.expressionMap.useIndex
+                    .split(",")
+                    .map((i) => this.escape(i.trim()))
+                    .join(", ")})`

Evidence

The new logic escapes every .split(",") segment, including empty strings after trimming; MySQL’s
identifier escape wraps any string (including empty) in backticks, yielding an empty identifier
token.

src/query-builder/SelectQueryBuilder.ts[2287-2296]
src/driver/mysql/MysqlDriver.ts[529-536]
src/query-builder/QueryExpressionMap.ts[168-175]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`useIndex` SQL generation escapes comma-separated segments but doesn’t filter out empty segments. If the user passes a string with a trailing comma or duplicate commas, SQL becomes invalid (e.g., `USE INDEX (``)`).
### Issue Context
The new code does `split(',') -> trim -> escape -> join`, but should drop empty trimmed values (and optionally throw if the final list is empty).
### Fix Focus Areas
- src/query-builder/SelectQueryBuilder.ts[2287-2296]
### Suggested change
Parse into an array first:
- `const indexes = this.expressionMap.useIndex.split(',').map(s => s.trim()).filter(Boolean)`
- If `indexes.length > 0`, join `indexes.map(i => this.escape(i))`
- Otherwise, omit the USE INDEX clause (or throw a TypeORMError if you want strict validation).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

7. disableEscaping bypasses escaping ✓ Resolved 🐞 Bug ⛨ Security

Description

The new useIndex() and lockTables escaping uses QueryBuilder.escape(), which returns raw input
after .disableEscaping(), re-enabling SQL injection via untrusted identifier inputs. This
contradicts the PR’s stated goal unless disableEscaping() is considered an explicit opt-out from
injection protection for identifiers.

Code

src/query-builder/SelectQueryBuilder.ts[2304]

+                useIndex = ` USE INDEX (${this.expressionMap.useIndex.split(",").map((i) => this.escape(i.trim())).join(", ")})`

Evidence

Both modified call sites call this.escape(...), but disableEscaping() flips a flag such that
escape() returns the input unchanged. Therefore, if a consumer disables escaping globally and
passes untrusted strings into useIndex() / lockTables, the values will again be interpolated
into SQL without escaping.

src/query-builder/SelectQueryBuilder.ts[2300-2306]
src/query-builder/SelectQueryBuilder.ts[2787-2800]
src/query-builder/QueryBuilder.ts[567-583]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The injection fix for `useIndex()` and `setLock(..., lockTables)` is currently bypassable when callers use `.disableEscaping()`, because `QueryBuilder.escape()` becomes a no-op and returns raw identifier strings.
### Issue Context
If `.disableEscaping()` is intended as a formatting/quoting toggle, it likely should not remove injection protection for user-provided identifiers in these specific APIs. At minimum, this behavior should be made explicit (docs) or the escaping should bypass the flag for these inputs.
### Fix Focus Areas
- src/query-builder/SelectQueryBuilder.ts[2300-2306]
- src/query-builder/SelectQueryBuilder.ts[2787-2800]
- src/query-builder/QueryBuilder.ts[567-583]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (3)

8. Missing #12333 test reference 📘 Rule violation ⚙ Maintainability

Description

New functional tests were added for an issue fix but do not include an issue reference
comment/annotation. This reduces traceability to the reported issue and does not meet the test
convention described in the compliance checklist.

Code

test/functional/query-builder/sql-injection/sql-injection.test.ts[R535-640]

+    describe("useIndex", () => {
+        it("should escape a malicious index name", () =>
+            Promise.all(
+                dataSources.map(async (dataSource) => {
+                    if (!DriverUtils.isMySQLFamily(dataSource.driver)) {
+                        return
+                    }
+
+                    const sql = dataSource
+                        .createQueryBuilder(Post, "post")
+                        .useIndex("my_index; DROP TABLE post")
+                        .getSql()
+
+                    // The malicious payload should be wrapped in backticks,
+                    // not interpreted as a raw SQL statement
+                    expect(sql).to.contain("USE INDEX")
+                    expect(sql).to.contain("`my_index; DROP TABLE post`")
+                }),
+            ))
+
+        it("should escape each index name when comma-separated", () =>
+            Promise.all(
+                dataSources.map(async (dataSource) => {
+                    if (!DriverUtils.isMySQLFamily(dataSource.driver)) {
+                        return
+                    }
+
+                    const sql = dataSource
+                        .createQueryBuilder(Post, "post")
+                        .useIndex("idx_one, idx_two")
+                        .getSql()
+
+                    expect(sql).to.contain("`idx_one`, `idx_two`")
+                }),
+            ))
+
+        it("should escape a malicious comma-separated index name", () =>
+            Promise.all(
+                dataSources.map(async (dataSource) => {
+                    if (!DriverUtils.isMySQLFamily(dataSource.driver)) {
+                        return
+                    }
+
+                    const sql = dataSource
+                        .createQueryBuilder(Post, "post")
+                        .useIndex("good_index, bad`; DROP TABLE post")
+                        .getSql()
+
+                    expect(sql).to.not.match(
+                        /DROP TABLE(?! post`)/, // should only appear inside escaped identifier
+                    )
+                    expect(sql).to.contain("USE INDEX")
+                }),
+            ))
+    })
+
+    describe("setLock lockTables", () => {
+        it("should escape a malicious table name in lockTables", () =>
+            Promise.all(
+                dataSources.map(async (dataSource) => {
+                    if (
+                        !DriverUtils.isPostgresFamily(dataSource.driver)
+                    ) {
+                        return
+                    }
+
+                    const sql = dataSource
+                        .createQueryBuilder(Post, "post")
+                        .setLock("pessimistic_write", undefined, [
+                            "post; DROP TABLE post",
+                        ])
+                        .getSql()
+
+                    expect(sql).to.not.match(
+                        /OF post; DROP TABLE/,
+                    )
+                    expect(sql).to.contain(
+                        '"post; DROP TABLE post"',
+                    )
+                }),
+            ))
+
+        it("should escape multiple malicious table names in lockTables", () =>
+            Promise.all(
+                dataSources.map(async (dataSource) => {
+                    if (
+                        !DriverUtils.isPostgresFamily(dataSource.driver)
+                    ) {
+                        return
+                    }
+
+                    const sql = dataSource
+                        .createQueryBuilder(Post, "post")
+                        .setLock("pessimistic_write", undefined, [
+                            "post",
+                            "user; DROP TABLE user",
+                        ])
+                        .getSql()
+
+                    expect(sql).to.contain('"post"')
+                    expect(sql).to.contain(
+                        '"user; DROP TABLE user"',
+                    )
+                }),
+            ))
+    })

Evidence

PR Compliance ID 3 requires issue-fix tests in test/functional to include an issue reference
comment when applicable. The added useIndex and setLock lockTables test blocks do not include
any reference to #12333 or similar issue ID in the added lines.

Rule 3: Prefer functional tests over per-issue tests
test/functional/query-builder/sql-injection/sql-injection.test.ts[535-640]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New functional tests for this issue fix do not include an issue reference (e.g., `#12333`) as requested by the testing compliance rule, reducing traceability.
## Issue Context
The PR description states this change fixes `#12333`, and the added tests in the functional suite appear to be directly validating that fix.
## Fix Focus Areas
- test/functional/query-builder/sql-injection/sql-injection.test.ts[535-640]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

9. lockTables escaping lacks test 📘 Rule violation ☼ Reliability

Description

lockTables entries are now escaped before being interpolated into the Postgres OF clause, but
there is no functional test assertion validating the generated SQL is properly quoted. This leaves
the injection fix unverified by a regression test.

Code

src/query-builder/SelectQueryBuilder.ts[2799]

+            lockTablesClause = " OF " + this.expressionMap.lockTables.map((table) => this.escape(table)).join(", ")

Evidence

PR Compliance ID 3 expects issue fixes to be represented in functional tests. The PR changes lock
SQL generation to escape lockTables, while existing functional coverage uses `setLock(...,
lockTables)` without asserting on the resulting SQL quoting/escaping behavior.

Rule 3: Prefer functional tests over per-issue tests
src/query-builder/SelectQueryBuilder.ts[2788-2800]
test/functional/query-builder/locking/query-builder-locking.test.ts[707-733]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Add/extend a functional test to assert that Postgres `lockTables` values are properly escaped/quoted in the generated `... FOR <lock> OF <tables>` clause.
## Issue Context
The PR fixes SQL injection risk by applying `this.escape(table)` when building the `OF` clause. A regression test should validate that potentially malicious identifiers are rendered as quoted identifiers (not raw SQL).
## Fix Focus Areas
- test/functional/query-builder/locking/query-builder-locking.test.ts[707-733]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

10. useIndex list mis-escaped ✓ Resolved 🐞 Bug ≡ Correctness

Description

useIndex() now escapes the entire useIndex string, so a comma-separated list becomes a single
quoted identifier (e.g., USE INDEX (a, b)), which is invalid MySQL syntax if callers pass multiple
index names in one string. This conflicts with the internal example showing multiple indexes in USE
INDEX().

Code

src/query-builder/SelectQueryBuilder.ts[2304]

+                useIndex = ` USE INDEX (${this.escape(this.expressionMap.useIndex)})`

Evidence

SelectQueryBuilder escapes the full useIndex string, while MysqlDriver.escape() wraps the whole
string in backticks without parsing; the QueryExpressionMap example shows a multi-index `USE INDEX
(col1_index, col2_index)` form, which would become a single backticked identifier under the new
logic.

src/query-builder/SelectQueryBuilder.ts[2300-2306]
src/query-builder/QueryExpressionMap.ts[168-175]
src/driver/mysql/MysqlDriver.ts[529-536]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`useIndex` is escaped as a single identifier string. If the input contains multiple index names separated by commas, MySQL will receive an invalid `USE INDEX (`a, b`)` clause.
### Issue Context
The internal comment/example for `useIndex` shows `USE INDEX (col1_index, col2_index)`, implying a multi-index list is plausible.
### Fix Focus Areas
- src/query-builder/SelectQueryBuilder.ts[2300-2306]
- src/query-builder/QueryExpressionMap.ts[168-175]
### Suggested fix
Parse `this.expressionMap.useIndex` as a comma-separated list (split on `,`, trim whitespace), escape each token individually with `this.escape()`, then join with `, `. Alternatively, if only a single index is intended to be supported, update the internal example/comment to avoid implying comma-separated lists are valid input.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

11. useIndex array shared by clone 🐞 Bug ☼ Reliability

Description

SelectQueryBuilder.useIndex() stores a caller-provided array by reference, and
QueryExpressionMap.clone() copies the useIndex reference as-is; mutating that array after
cloning can change multiple builders’ generated SQL unexpectedly. This new risk is introduced
because useIndex is now an array type.

Code

src/query-builder/SelectQueryBuilder.ts[R1533-1536]

+    useIndex(indexes: string | string[]): this {
+        this.expressionMap.useIndex = Array.isArray(indexes)
+            ? indexes
+            : [indexes]

Evidence

The builder assigns the input array directly, and the expression map clone assigns useIndex
directly, so both objects can share a single string[] instance after cloning or after passing in a
caller-owned array.

src/query-builder/SelectQueryBuilder.ts[1529-1537]
src/query-builder/QueryExpressionMap.ts[506-516]
src/query-builder/QueryExpressionMap.ts[168-176]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Now that `useIndex` is a `string[]`, both `useIndex()` and `QueryExpressionMap.clone()` should avoid sharing array references to prevent cross-builder coupling through mutation.
### Issue Context
Even if TypeORM code doesn’t mutate `useIndex` in-place, consumers can hold and later mutate the array they passed to `useIndex([...])`, or mutate `expressionMap.useIndex` directly. Because clone() currently assigns the same reference, clones can be affected too.
### Fix Focus Areas
- src/query-builder/SelectQueryBuilder.ts[1533-1537]
- src/query-builder/QueryExpressionMap.ts[483-516]
### Implementation sketch
- In `SelectQueryBuilder.useIndex()`:
- if `indexes` is an array, assign a copy: `this.expressionMap.useIndex = [...indexes]`.
- In `QueryExpressionMap.clone()`:
- copy: `map.useIndex = this.useIndex ? [...this.useIndex] : this.useIndex`.
(If you adopt a separate `useIndexes` field for back-compat, apply the same defensive copies there too.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[smith-xyz]

changes look good - could add a test around this behavior in test/functional/query-builder/sql-injection/

[eddieran]

@smith-xyz Thanks for the review! I've added SQL injection tests in test/functional/query-builder/sql-injection/sql-injection.test.ts covering:

• useIndex() — verifies malicious index names like my_index; DROP TABLE post get properly escaped with backticks (MySQL)
• useIndex() with comma-separated names — verifies that idx_one, idx_two is split and each index is individually escaped, and that malicious payloads in multi-index strings are contained
• setLock() lockTables — verifies malicious table names in lockTables get properly escaped with double quotes (Postgres), both single and multiple tables

I also addressed the Qodo review feedback: useIndex now splits on commas and escapes each index name individually to correctly handle multiple index hints like USE INDEX (idx_one, idx_two). The existing useIndex test assertion was updated to reflect the escaped output.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[qodo-free-for-open-source-projects]

Persistent review updated to latest commit 9984aaa

[smith-xyz]

lgtm! thank you for your contribution

[pkg-pr-new]

• typeorm-sql-js-example

  npm i https://pkg.pr.new/typeorm@12344
  

commit: 3a4efeb

[gioboa]

Thanks @eddieran for your great help 🙏

[alumni]

Possible edge case: using table paths (and similar for indices). Both tables and indices (basically any database object) can be referenced by a path: "SCHEMA"."MY_TABLE".

IMO this is not a blocker to merge, both of these functionalities (useIndex/lockTables) are almost never used.

[qodo-free-for-open-source-projects]

Persistent review updated to latest commit 9500618

[qodo-free-for-open-source-projects]

Persistent review updated to latest commit 3a154e6

[alumni]

One thing: we should find a way to not split by ,. Maybe expressionMap.useIndex should be string[] and we should define SelectQueryBuilder.useIndex(...indexes: string[])/SelectQueryBuilder.useIndex(indexes: string|string[]) 🤷🏻‍♂️

[smith-xyz]

@eddieran can you check on the tests - I think it could be related qodo's no.2 finding. Might need to update the applyFindOptions path to pass tableAlias.name instead of this.escape(tableAlias.name) to setLock()?

e.g. its triple quoting in there FOR UPDATE OF """Post"""

[alumni]

It's double escaped because in Repository APIs we escape things, but in QueryBuilder we don't always (on purpose, for raw SQL).

In this case, I think both should be quoted in the QueryBuilder because we do not expect the users to provide raw SQL in those arguments (it's always an index name or a table name, you can't provide e.g. a subquery).

The current escaping behavior in setLock is actually the reason why I didn't try to extend the implementation the SAP driver for this sub-feature when I added locking support (it's slightly different than Postgres, you lock columns, not tables) :)

Right now I'm more interested in changing the signature of useIndex since it's a breaking change and we can add it to the 1.0 release - it will be difficult to release it after.

Ideally we should not accept issues that list more than 1 point (the linked one has 4 points and e.g. point #3 is irrelevant, we need to handle these things independently from each other) nor PRs that solve more than 1 issue (this one has 2, would have been easier to handle them independently).

[eddieran]

Re: comma-split — agree that's the right fix. Switching useIndex to string | string[] (with rest-param overload) avoids the brittle .split(",") and removes a class of edge cases (whitespace, quoted commas in identifiers). Happy to land that here as a follow-up commit since it'd be a breaking change worth landing for 1.0, or split it into a separate PR — your call.

Re: schema-qualified paths ("SCHEMA"."MY_TABLE") — yeah, naive escaping would mangle those. The cleanest path is probably split-on-dot then escape each component, mirroring how escapePath already works elsewhere in the codebase. I can extend the tests to cover that.

Re: smith-xyz's FOR UPDATE OF """Post""" — I see what alumni's saying about the QueryBuilder/Repository contract. If the intent is that QueryBuilder accepts raw fragments and Repository sanitizes, then the double-escape in applyFindOptions is the bug (Repository is passing already-escaped names to a QueryBuilder method that re-escapes). I'd rather not touch that path in this PR — it's a separate concern from the input validation here. Want me to file it as its own issue?

[michaelbromley]

@eddieran after internal discussion here's how we want to handle this:

• Keep the useIndex change but shift it to accept string[] instead of string
• The lockTables escaping fix can come separately with more time to understand the implementation (ie leave it as-is before your change)
• The double-escaping bug (Qodo finding) still needs fixing either way

Let us know if you'll have time to do that as we'd like to get the useIndex signature change in before we release v1.0 - so in the next day or so would be best. If not we can take over and finish that up.

[michaelbromley]

@eddieran I went ahead and pushed some changes as described. Hope that's ok and thanks for your work so far.

[alumni]

the upgrade guide for MySQL needs a small update (only place where you can use useIndex) - will quickly create a commit