Skip to content

fix: prevent SQL injection via SelectQueryBuilder.distinctOn (CVE-2026-76848) - #12804

Open
Amine-H wants to merge 5 commits into
typeorm:masterfrom
Amine-H:fix/CVE-2026-76848
Open

fix: prevent SQL injection via SelectQueryBuilder.distinctOn (CVE-2026-76848)#12804
Amine-H wants to merge 5 commits into
typeorm:masterfrom
Amine-H:fix/CVE-2026-76848

Conversation

@Amine-H

@Amine-H Amine-H commented Aug 27, 2026 •
edited
Loading

Copy link
Copy Markdown

Description of change

Fixes CVE-2026-76848: SQL injection via SelectQueryBuilder.distinctOn.

Current behavior

SelectQueryBuilder.distinctOn accepts an array of strings and stores them on the expression map without validation. For PostgreSQL-family drivers, createSelectDistinctExpression joins that array and interpolates the result into the generated statement as SELECT DISTINCT ON (values), with no escaping, quoting, identifier validation, or allowlist, and without routing the values through replacePropertyNames or the driver's escape helper.

Because the interpolation point is a parenthesized SQL expression list rather than an identifier-only position, a supplied element could carry arbitrary expressions — including correlated subqueries. An application that forwards a client-controlled value into distinctOn (for example, to let a caller choose a deduplication column) allowed that client to read data anywhere the application's database role can reach through boolean or time-based inference, independently of the entity being queried.

New behavior

  1. distinctOn rejects semicolons, using the same guard as the group-by / order-by query-builder methods, to prevent statement stacking.

  2. createSelectDistinctExpression escapes every value through buildDistinctOnExpression:

    • property paths such as post.author are resolved to the actual database column via entity metadata, and
    • every identifier segment is quoted with the driver's escape() helper.

    Unknown property paths, metadata-less aliases (for example, subqueries), and arbitrary expressions are also escaped segment by segment, so no value is ever interpolated verbatim and nothing can execute as raw SQL.

How it was verified

  • Added regression tests in test/functional/query-builder/sql-injection/sql-injection.test.ts covering escaping of valid property paths, aliases without metadata (including hyphens), rejection of semicolon-based statement stacking, and escaping of correlated-subquery / DROP TABLE / UNION payloads.
  • Existing distinct-on functional tests pass (3/3).
  • Full query-builder test suite passes (436 passing, 0 failures).
  • tsc --noEmit and ESLint are clean.

Pull-Request Checklist

  • Code is up-to-date with the master branch
  • This pull request links a relevant issue using a closing keyword:
    Closes #12805
  • There are new or updated tests validating the change (tests/**.test.ts)
  • Documentation has been updated to reflect this change (docs/docs/**.md)

@github-actions github-actions Bot added the needs-triage PR needs issue link triage label Aug 27, 2026
@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Aug 27, 2026 •
edited
Loading

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (2) 📜 Skill insights (0)

Grey Divider


Action required

1. payload comment repeats assertions 📘 Rule violation ⚙ Maintainability ⭐ New
Description 
The added comment merely narrates the two immediately following assertions and adds no context that
the test code does not already express. This is extra commentary prohibited by the checklist's
AI-noise rule.
Code

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

+                // The payload may only appear inside an escaped identifier.
Evidence 
PR Compliance ID 4 prohibits extra comments and AI-generated noise. The comment at line 707 directly
paraphrases the containment assertions at lines 708-709 instead of providing otherwise unavailable
context.

Rule 4: Remove AI-generated noise
test/functional/query-builder/sql-injection/sql-injection.test.ts[707-709]

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

## Issue description
Remove the redundant comment that restates what the following assertions verify.

## Issue Context
The test assertions already show that the payload must occur only in an escaped identifier and not as raw `DISTINCT ON` SQL. The compliance checklist prohibits extra AI-like comments.

## Fix Focus Areas
- test/functional/query-builder/sql-injection/sql-injection.test.ts[707-707]

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

2. Composite relation paths break 🐞 Bug ≡ Correctness ⭐ New
Description 
For a relation backed by multiple join columns, distinctOn(["post.category"]) now falls through
and emits "post"."category", which is not a database column and causes the query to fail.
Previously, whole-query property replacement mapped an exact relation path to its first join column,
including composite relations.
Code

src/query-builder/SelectQueryBuilder.ts[R2341-2344]

+                const column = this.resolvePropertyPathColumn(
+                    alias,
+                    propertyPath,
+                )
Evidence 
The new builder uses resolvePropertyPathColumn and falls back to escaping the unresolved property
as a physical column. Repository metadata resolution intentionally returns an exact relation path
only for a single-column relation, whereas the prior replacement path explicitly mapped any relation
with join columns to the first join column.

src/query-builder/SelectQueryBuilder.ts[2339-2359]
src/query-builder/SelectQueryBuilder.ts[2405-2410]
src/metadata/EntityMetadata.ts[747-762]
src/query-builder/QueryBuilder.ts[755-760]
src/query-builder/SelectQueryBuilder.ts[85-98]

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

## Issue description
Exact relation property paths with composite join columns no longer resolve to a database column in `DISTINCT ON`.

## Issue Context
`EntityMetadata.findColumnWithPropertyPath` only resolves an exact relation path when that relation has one join column, while the previous whole-query replacement selected the first join column for any relation with join columns. Preserve that behavior for exact owning relation paths without incorrectly qualifying junction-table columns.

## Fix Focus Areas
- src/query-builder/SelectQueryBuilder.ts[2388-2410]
- src/query-builder/QueryBuilder.ts[755-760]
- src/metadata/EntityMetadata.ts[747-762]

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

3. Verbose helper comments add noise ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description 
buildDistinctOnExpression adds multiple inline comments that merely narrate straightforward
branches and repeat security claims already covered by its JSDoc. This is inconsistent with nearby
expression builders and violates the explicit requirement to avoid extra AI-like comments.
Code

src/query-builder/SelectQueryBuilder.ts[R2334-2336]

+        // Resolve an exact registered alias prefix first. Alias names are
+        // unrestricted strings and are safely escaped, so they are not
+        // subject to any character restrictions.
Evidence 
Rule 4 explicitly disallows extra comments and style inconsistent with the file. The cited added
lines narrate the immediately following alias lookup, while additional added comments similarly
narrate fallback branches; neighboring expression builders use concise method documentation without
this running commentary.

Rule 4: Remove AI-generated noise
src/query-builder/SelectQueryBuilder.ts[2334-2336]
src/query-builder/SelectQueryBuilder.ts[2356-2358]
src/query-builder/SelectQueryBuilder.ts[2369-2379]

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 DISTINCT ON helpers contain redundant inline commentary that narrates the implementation and repeats the method-level JSDoc.
## Issue Context
Compliance rule 4 requires avoiding extra AI-generated comments and style inconsistent with the file. Preserve comments only where they explain non-obvious behavior that the code and concise JSDoc cannot convey.
## Fix Focus Areas
- src/query-builder/SelectQueryBuilder.ts[2334-2336]
- src/query-builder/SelectQueryBuilder.ts[2356-2358]
- src/query-builder/SelectQueryBuilder.ts[2369-2379]

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

View action required (3)
4. distinctOn docs misstate errors ✓ Resolved 📘 Rule violation ≡ Correctness
Description 
The documentation says every arbitrary SQL expression throws, but alias-prefixed payloads bypass
validation and are escaped instead, as the new regression test explicitly expects. This leaves the
documented public behavior inconsistent with the implemented API behavior.
Code

docs/docs/query-builder/1-select-query-builder.md[512]

+Arbitrary SQL expressions are not allowed and throw an error.
Evidence 
Rule 2 requires documentation to reflect public API changes. The new documentation promises an error
for arbitrary SQL expressions, while the registered-alias branch returns an escaped value without
invoking validation and the test at lines 632-639 verifies that behavior for `post.id); DROP TABLE
post; --`.

Rule 2: Docs updated for user-facing changes
docs/docs/query-builder/1-select-query-builder.md[511-512]
src/query-builder/SelectQueryBuilder.ts[2347-2374]
test/functional/query-builder/sql-injection/sql-injection.test.ts[632-639]

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 documented `distinctOn` rejection behavior does not match the implementation for values beginning with a registered alias.
## Issue Context
`buildDistinctOnExpression` returns from the registered-alias branch before calling `validateDistinctOnExpression`, and the added test expects an alias-prefixed injection payload to be escaped rather than rejected. Either validate the property-path portion so arbitrary expressions throw, or document the escaping exception accurately.
## Fix Focus Areas
- docs/docs/query-builder/1-select-query-builder.md[511-512]
- src/query-builder/SelectQueryBuilder.ts[2347-2374]
- test/functional/query-builder/sql-injection/sql-injection.test.ts[626-639]

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

5. Relation column paths break ✓ Resolved 🐞 Bug ≡ Correctness
Description 
buildDistinctOnExpression cannot resolve relation-plus-referenced-column paths such as
post.category.id, so it emits "post"."category"."id" instead of the mapped join column
"post"."categoryId". These valid paths were mapped by the existing whole-query replacement before
this change and now produce invalid SQL.
Code

src/query-builder/SelectQueryBuilder.ts[R2354-2355]

+                const column =
+                    alias.metadata.findColumnWithPropertyPath(propertyPath)
Evidence 
The query previously underwent the general property replacement pass, which explicitly maps a
relation path plus its referenced column to the join-column database name. The new resolver uses a
metadata method that only matches direct columns or an exact relation path, then falls back to
emitting each unmatched path segment as an identifier; the repository's Post/category fixture
demonstrates that category.id must map to categoryId.

src/query-builder/SelectQueryBuilder.ts[84-98]
src/query-builder/QueryBuilder.ts[762-773]
src/metadata/EntityMetadata.ts[747-762]
src/query-builder/SelectQueryBuilder.ts[2351-2374]
test/github-issues/8459/entity/Post.ts[18-23]

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

## Issue description
`buildDistinctOnExpression` resolves alias-qualified values only through `findColumnWithPropertyPath`. That API handles direct columns and exact single-column relation paths, but not relation-plus-referenced-column paths such as `category.id`; the fallback then quotes each component as separate SQL identifiers and generates a nonexistent column reference.
## Issue Context
Before this change, raw `DISTINCT ON` expressions passed through `replacePropertyNamesForTheWholeQuery`, whose replacement map explicitly translates `relation.propertyPath + referencedColumn.propertyPath` to the relation join column's database name. Preserve that supported property-path behavior while continuing to escape every emitted identifier.
## Fix Focus Areas
- src/query-builder/SelectQueryBuilder.ts[2351-2374]
- src/query-builder/QueryBuilder.ts[762-773]
- src/metadata/EntityMetadata.ts[747-762]
- test/functional/query-builder/distinct-on/query-builder-distinct-on.test.ts[111-173]

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

6. Document distinctOn restrictions ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description 
distinctOn now rejects arbitrary SQL expressions and only accepts identifier/property paths,
changing the behavior of a public query-builder API. The existing documentation was not updated and
still says DISTINCT ON expressions follow the same rules as orderBy.
Code

src/query-builder/SelectQueryBuilder.ts[R2331-2334]

+    protected validateDistinctOnExpression(columnName: string): void {
+        if (!/^[A-Za-z0-9_$]+(\.[A-Za-z0-9_$]+)*$/.test(columnName))
+            throw new TypeORMError(
+                `Invalid DISTINCT ON expression "${columnName}". Only column names and property paths are allowed.`,
Evidence 
Compliance rule 2 requires documentation for public API or usage changes. The new validator enforces
an externally visible input restriction, while the current DISTINCT ON documentation says its
expressions are interpreted using the same rules as order-by and does not disclose the new
restriction.

Rule 2: Docs updated for user-facing changes
src/query-builder/SelectQueryBuilder.ts[2331-2335]
docs/docs/query-builder/1-select-query-builder.md[508-523]

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

## Issue description
Document the new `distinctOn` input restriction introduced by this security fix.
## Issue Context
`distinctOn` now accepts only column names or dot-separated property paths and rejects arbitrary SQL expressions. The current query-builder documentation still states that DISTINCT ON expressions use the same rules as order-by, which no longer accurately describes the public API.
## Fix Focus Areas
- docs/docs/query-builder/1-select-query-builder.md[508-523]
- src/query-builder/SelectQueryBuilder.ts[2331-2335]

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


Remediation recommended

7. Dotted column names split ✓ Resolved 🐞 Bug ≡ Correctness
Description 
For an entity column whose physical database name contains a dot,
distinctOn(["post.profile.name"]) now emits "post"."profile"."name" instead of the valid
"post"."profile.name". The metadata lookup only recognizes entity property paths, so the fallback
changes which column PostgreSQL addresses and can cause an undefined-column error.
Code

src/query-builder/SelectQueryBuilder.ts[R2362-2365]

+                propertyPath
+                    .split(".")
+                    .map((part) => this.escape(part))
+                    .join(".")
Evidence 
Custom names are retained as ColumnMetadata.databaseName, while the existing whole-query
replacement explicitly maps a complete database name and escapes it as one identifier. The new
resolver only calls findColumnWithPropertyPath; when that fails, these changed lines split the
physical name at every dot.

src/metadata/ColumnMetadata.ts[932-998]
src/metadata/EntityMetadata.ts[747-760]
src/query-builder/QueryBuilder.ts[776-789]
src/query-builder/QueryBuilder.ts[819-823]
src/query-builder/SelectQueryBuilder.ts[2341-2365]
src/query-builder/SelectQueryBuilder.ts[2395-2413]

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

## Issue description
`buildDistinctOnExpression` splits unresolved dotted values into separate identifier segments, even when the suffix is a physical database column name that contains a dot. This changes valid `DISTINCT ON` SQL from a single quoted column identifier into a multi-part path.
## Issue Context
Column metadata preserves custom database names, and the prior whole-query replacement map matched and escaped each complete database name. When an alias has metadata, look up the complete suffix against column `databaseName` before applying the segment-by-segment fallback; apply equivalent handling for a bare value against the main alias.
## Fix Focus Areas
- src/query-builder/SelectQueryBuilder.ts[2337-2382]
- src/query-builder/SelectQueryBuilder.ts[2395-2413]

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

8. assertNoSemicolon comment misstates usage 📘 Rule violation ⚙ Maintainability
Description 
The added JSDoc says this helper validates sort/group expressions, but the PR only calls it from
distinctOn; neither groupBy nor orderBy uses it. This inaccurate, unnecessary commentary is
inconsistent with the implementation and violates the requirement to avoid AI-generated noise.
Code

src/query-builder/QueryBuilder.ts[R1760-1762]

+    /**
+     * Rejects a `;` in sort/group expressions to prevent SQL injection.
+     *
Evidence 
Rule 4 prohibits extra AI-like comments and style noise. The added JSDoc at
QueryBuilder.ts[1760-1762] claims sort/group coverage, while the PR branch's sole call is from
SelectQueryBuilder.distinctOn at SelectQueryBuilder.ts[265-266].

Rule 4: Remove AI-generated noise
src/query-builder/QueryBuilder.ts[1760-1762]
src/query-builder/SelectQueryBuilder.ts[265-266]

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 `assertNoSemicolon` JSDoc claims the helper applies to sort/group expressions, although its only caller is `distinctOn`.
## Issue Context
Keep comments narrowly aligned with actual behavior and avoid commentary that inaccurately generalizes the helper's scope.
## Fix Focus Areas
- src/query-builder/QueryBuilder.ts[1760-1765]

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

9. Valid aliases are rejected ✓ Resolved 🐞 Bug ≡ Correctness
Description 
validateDistinctOnExpression rejects safe paths such as post-item.id even when post-item is
the query's registered alias, so previously valid PostgreSQL DISTINCT ON queries now throw before
SQL generation. Query aliases are unrestricted strings elsewhere and are safely identifier-escaped
by the existing property replacement machinery.
Code

src/query-builder/SelectQueryBuilder.ts[R2331-2334]

+    protected validateDistinctOnExpression(columnName: string): void {
+        if (!/^[A-Za-z0-9_$]+(\.[A-Za-z0-9_$]+)*$/.test(columnName))
+            throw new TypeORMError(
+                `Invalid DISTINCT ON expression "${columnName}". Only column names and property paths are allowed.`,
Evidence 
createAlias accepts and stores any supplied string without identifier-character restrictions,
while the whole-query replacement code historically recognizes that exact alias prefix and emits it
through this.escape. The new validator instead permits only [A-Za-z0-9_$] segments before alias
resolution, making safe registered aliases containing other characters unusable specifically in
distinctOn.

src/query-builder/QueryExpressionMap.ts[406-424]
src/query-builder/QueryBuilder.ts[732-753]
src/query-builder/QueryBuilder.ts[798-833]
src/query-builder/SelectQueryBuilder.ts[2331-2335]
src/query-builder/SelectQueryBuilder.ts[2362-2379]

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

## Issue description
DISTINCT ON rejects valid property paths when a registered alias contains characters such as hyphens, despite aliases being unrestricted and safely escapable.
## Issue Context
Resolve an exact registered alias prefix before applying strict validation to the remaining property path, and always escape the resolved alias. Preserve rejection of unknown raw SQL expressions.
## Fix Focus Areas
- src/query-builder/SelectQueryBuilder.ts[2331-2379]
- test/functional/query-builder/sql-injection/sql-injection.test.ts[552-625]

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

  • Author self-review: I have reviewed the code review findings, and addressed the relevant ones.

Grey Divider

Tip of the day 
💡 Did you know, you can ask Qodo to dismiss a finding you disagree with, with your reason on record

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@Amine-H
fix: resolve registered aliases before validating distinctOn expressions
ebac237 
Address review feedback: resolve an exact registered alias prefix before
applying strict identifier validation, so aliases containing characters
such as hyphens remain usable. Document the new distinctOn restriction.
@Amine-H

Amine-H commented Aug 27, 2026

Copy link
Copy Markdown
Author

Thanks for the review. Both findings are addressed in commit ebac2372c:

1. distinctOn restrictions now documented

Updated docs/docs/query-builder/1-select-query-builder.md: DISTINCT ON expressions must be column names or entity property paths (e.g. user.id), and arbitrary SQL expressions throw an error.

2. Registered aliases are no longer rejected

buildDistinctOnExpression now resolves an exact registered alias prefix before applying strict identifier validation, so aliases containing characters such as hyphens (post-item.id) remain usable and are safely escaped. The strict validation now applies only to values that do not match a registered alias (the injection vector). Unknown raw SQL expressions are still rejected.

Tests added for both behaviors; the full query-builder suite passes (435 passing, 0 failures), and tsc --noEmit + ESLint are clean.

@Amine-H
Amine-H marked this pull request as draft August 27, 2026 16:20
@Amine-H
fix: reject statement terminators and escape distinctOn values instea…
95b569a 
…d of allowlisting

Follow the query-builder's existing pattern: reject semicolons (statement
stacking) rather than arbitrary expressions, and escape every value through
the driver so correlated subqueries and other expressions cannot execute.
@Amine-H

Amine-H commented Aug 27, 2026

Copy link
Copy Markdown
Author

Updated the approach in commit 95b569a7b to match how the rest of the query builder guards these inputs, instead of allowlisting arbitrary expressions:

  • Reject statement terminators, not expressions. distinctOn now uses the same semicolon guard as the group-by / order-by paths (assertNoSemicolon, added to the base QueryBuilder), preventing statement stacking.
  • Escape everything else. buildDistinctOnExpression resolves known property paths to their database columns and quotes every identifier segment with the driver's escape(). Unknown values — including correlated subqueries, DROP TABLE, UNION, and OR 1=1 payloads — are escaped into inert quoted identifiers rather than rejected, so they can never execute as raw SQL while legitimate aliases (e.g. post-item) keep working.

The strict allowlist from the earlier revision is removed. Tests and docs updated accordingly; full query-builder suite passes (436 passing, 0 failures), tsc --noEmit and ESLint clean.

@Amine-H
Amine-H marked this pull request as ready for review August 27, 2026 16:34
@Amine-H Amine-H mentioned this pull request Aug 27, 2026
Open
@github-actions github-actions Bot added linked-issue PR references an issue and removed needs-triage PR needs issue link triage labels Aug 27, 2026
@Amine-H
fix: resolve relation property paths and correct assertNoSemicolon JSDoc
9e33b0b 
distinctOn now resolves relation-plus-referenced-column paths such as
'post.category.id' to the relation's join column (e.g. 'post.categoryId'),
preserving the mapping the whole-query property replacement previously
performed. The assertNoSemicolon JSDoc no longer claims sort/group usage.
@Amine-H

Amine-H commented Aug 27, 2026

Copy link
Copy Markdown
Author

Addressed the two remaining findings in commit 9e33b0bf0:

2. Relation column paths break — fixed. buildDistinctOnExpression now resolves relation-plus-referenced-column paths via a new resolvePropertyPathColumn helper, which mirrors the mapping the whole-query property replacement performs. post.category.id now emits the join column "post"."categoryId" instead of "post"."category"."id". Added a regression test with a ManyToOne fixture covering this.

4. assertNoSemicolon JSDoc misstates usage — fixed. The comment no longer claims sort/group coverage; it now accurately describes the helper as a generic statement-stacking guard for raw string inputs.

Full query-builder suite passes (437 passing, 0 failures); tsc --noEmit and ESLint clean.

@Amine-H
fix: keep dotted column names intact and drop inline commentary
7b2f7bf 
distinctOn now resolves physical database column names that contain dots
(e.g. 'post.profile.name') as a single identifier instead of splitting them
into separate path segments, and the helper no longer carries redundant
inline comments.
@Amine-H

Amine-H commented Aug 27, 2026

Copy link
Copy Markdown
Author

Addressed the remaining findings in commit 7b2f7bff4:

1. Verbose helper comments — removed. buildDistinctOnExpression no longer carries inline narrative comments; the concise JSDoc covers the behavior, consistent with neighboring expression builders.

5. Dotted column names split — fixed. resolvePropertyPathColumn now matches the complete suffix against ColumnMetadata.databaseName before the segment-by-segment fallback. A physical column named profile.name produces "post"."profile.name" as a single identifier instead of "post"."profile"."name". Added a regression test with a dotted database column name.

6. assertNoSemicolon JSDoc was corrected in the previous commit (9e33b0bf0) — it no longer claims sort/group coverage and reads "Rejects a ; in raw string inputs to prevent SQL statement stacking."

Full query-builder suite passes (438 passing, 0 failures); tsc --noEmit and ESLint clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Compliance violation linked-issue PR references an issue

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@Amine-H