Conversation
The dwsql (Azure Synapse) upsert builder emitted an UPDATE with no WHERE clause, so a single PUT upsert hitting the existing-row UPDATE branch overwrote every row in the target table and dropped the update DB policy (CWE-862).
Append WHERE {pkPredicates AND update DB policy} to the UPDATE branch, mirroring MsSqlQueryBuilder.Build(SqlUpsertQueryStructure) and the DWSql PATCH path. Add a regression test asserting the generated dwsql upsert UPDATE branch is scoped by the PK and the update database policy.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a critical security defect in the DWSQL (Azure Synapse) upsert query builder where the UPDATE branch could be emitted without a WHERE clause, potentially overwriting all rows and bypassing update DB policy scoping.
Changes:
- Scope the DWSQL upsert UPDATE branch using
WHERE {pkPredicates AND updateDbPolicy}(mirrors the MsSql upsert builder behavior). - Add a unit regression test asserting the DWSQL upsert UPDATE branch includes
WHERE, the PK predicate, and the update DB policy predicate.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Core/Resolvers/DWSqlQueryBuilder.cs | Adds WHERE scoping to the DWSQL upsert UPDATE branch using PK + update DB policy predicates. |
| src/Service.Tests/UnitTests/DwSqlQueryBuilderUpsertTests.cs | Adds a regression unit test to ensure DWSQL upsert UPDATE is always scoped by PK and update DB policy. |
Comment on lines
+4
to
+8
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Data; | ||
| using Azure.DataApiBuilder.Auth; | ||
| using Azure.DataApiBuilder.Config.DatabasePrimitives; |
Comment on lines
+130
to
+134
| string outColumn; | ||
| metadataProvider.Setup(x => x.TryGetBackingColumn(It.IsAny<string>(), It.IsAny<string>(), out outColumn)) | ||
| .Callback(new TryGetColumnCallback((string entity, string field, out string? column) | ||
| => _columnMapping.TryGetValue(field, out column))) | ||
| .Returns((string entity, string field, string column) => _columnMapping.ContainsKey(field)); |
Comment on lines
+136
to
+140
| string outExposed; | ||
| metadataProvider.Setup(x => x.TryGetExposedColumnName(It.IsAny<string>(), It.IsAny<string>(), out outExposed)) | ||
| .Callback(new TryGetColumnCallback((string entity, string field, out string? column) | ||
| => _columnMapping.TryGetValue(field, out column))) | ||
| .Returns((string entity, string field, string column) => _columnMapping.ContainsKey(field)); |
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
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.
The dwsql (Azure Synapse) upsert builder emitted an UPDATE with no WHERE clause, so a single PUT upsert hitting the existing-row UPDATE branch overwrote every row in the target table and dropped the update DB policy (CWE-862).
Append WHERE {pkPredicates AND update DB policy} to the UPDATE branch, mirroring MsSqlQueryBuilder.Build(SqlUpsertQueryStructure) and the DWSql PATCH path. Add a regression test asserting the generated dwsql upsert UPDATE branch is scoped by the PK and the update database policy.
Why make this change?
Fixes a security defect (CWE-862, missing row scoping / authorization) in the Azure Synapse Analytics (dwsql / Dedicated SQL Pool) upsert query builder.
The dwsql upsert builder computed pkPredicates but never appended a WHERE clause to the generated UPDATE. As a result, any authenticated user with entity-level update permission could issue a single [PUT /api//] that hit the existing-row UPDATE branch and overwrite every row in the target table, while also silently discarding the row-level update database policy. The API still returns a normal single-record 200, so the mass overwrite is indistinguishable from a legitimate single-row update in application logs.
Not affected: the MSSQL/PostgreSQL/MySQL upsert builders and DAB's own DWSql PATCH path ([Build(SqlUpdateStructure)]), which all correctly scope the UPDATE with WHERE.
What is this change?
Append WHERE {pkPredicates AND update DB policy} to the UPDATE branch, mirroring MsSqlQueryBuilder.Build(SqlUpsertQueryStructure) and the DWSql PATCH path. Add a regression test asserting the generated dwsql upsert UPDATE branch is scoped by the PK and the update database policy.
How was this tested?
Sample Request(s)
Given a dwsql entity Book (PK id) with update permission granted, the following existing-row upsert previously overwrote all rows:
PUT /api/Book/id/7
Content-Type: application/json
{
"title": "The Hobbit Returns to The Shire",
"publisher_id": 1234
}
Generated UPDATE branch before the fix (overwrites every row):
IF @ROWS_TO_UPDATE = 1 BEGIN UPDATE [dbo].[books] SET [title] = @param1, [publisher_id] = @Param2 END
Generated UPDATE branch after the fix (scoped to the target row and update policy):
IF @ROWS_TO_UPDATE = 1 BEGIN UPDATE [dbo].[books] SET [title] = @param1, [publisher_id] = @Param2 WHERE [dbo].[books].[id] = @param0 END