From 0d69ea77d1e303b73206cd991850616df6ad0022 Mon Sep 17 00:00:00 2001 From: Sean Leonard Date: Tue, 27 Sep 2022 11:47:20 -0700 Subject: [PATCH 1/9] Adds ArgumentException handling for SqlDeleteQueryStructure code path. --- .../SqlDeleteQueryStructure.cs | 50 ++++++++++++------- 1 file changed, 31 insertions(+), 19 deletions(-) diff --git a/src/Service/Resolvers/Sql Query Structures/SqlDeleteQueryStructure.cs b/src/Service/Resolvers/Sql Query Structures/SqlDeleteQueryStructure.cs index 353b6152fd..d1460b700a 100644 --- a/src/Service/Resolvers/Sql Query Structures/SqlDeleteQueryStructure.cs +++ b/src/Service/Resolvers/Sql Query Structures/SqlDeleteQueryStructure.cs @@ -1,3 +1,4 @@ +using System; using System.Collections.Generic; using System.Net; using Azure.DataApiBuilder.Config; @@ -20,28 +21,39 @@ public SqlDeleteStructure( { TableDefinition tableDefinition = GetUnderlyingTableDefinition(); - List primaryKeys = tableDefinition.PrimaryKey; - foreach (KeyValuePair param in mutationParams) + try { - if (param.Value is null) + List primaryKeys = tableDefinition.PrimaryKey; + foreach (KeyValuePair param in mutationParams) { - // Should never happen since delete mutations expect non nullable pk params - throw new DataApiBuilderException( - message: $"Unexpected {param.Key} null argument.", - statusCode: HttpStatusCode.BadRequest, - subStatusCode: DataApiBuilderException.SubStatusCodes.BadRequest); - } + if (param.Value is null) + { + // Should never happen since delete mutations expect non nullable pk params + throw new DataApiBuilderException( + message: $"Unexpected {param.Key} null argument.", + statusCode: HttpStatusCode.BadRequest, + subStatusCode: DataApiBuilderException.SubStatusCodes.BadRequest); + } - // primary keys used as predicates - SqlMetadataProvider.TryGetBackingColumn(EntityName, param.Key, out string? backingColumn); - if (primaryKeys.Contains(backingColumn!)) - { - Predicates.Add(new Predicate( - new PredicateOperand(new Column(DatabaseObject.SchemaName, DatabaseObject.Name, backingColumn!)), - PredicateOperation.Equal, - new PredicateOperand($"@{MakeParamWithValue(GetParamAsColumnSystemType(param.Value.ToString()!, backingColumn!))}") - )); - } + // primary keys used as predicates + SqlMetadataProvider.TryGetBackingColumn(EntityName, param.Key, out string? backingColumn); + if (primaryKeys.Contains(backingColumn!)) + { + Predicates.Add(new Predicate( + new PredicateOperand(new Column(DatabaseObject.SchemaName, DatabaseObject.Name, backingColumn!)), + PredicateOperation.Equal, + new PredicateOperand($"@{MakeParamWithValue(GetParamAsColumnSystemType(param.Value.ToString()!, backingColumn!))}") + )); + } + } + } + catch (ArgumentException ex) + { + // ArgumentException thrown from GetParamAsColumnSystemType() + throw new DataApiBuilderException( + message: ex.Message, + statusCode: HttpStatusCode.BadRequest, + subStatusCode: DataApiBuilderException.SubStatusCodes.BadRequest); } } } From 0562825247a038283b53bf81af076c9e0759c3c9 Mon Sep 17 00:00:00 2001 From: Sean Leonard Date: Tue, 27 Sep 2022 12:11:01 -0700 Subject: [PATCH 2/9] syntactic sugar for Exception Filter to reduce amount of code used by if/else and throw; --- .../Sql Query Structures/BaseSqlQueryStructure.cs | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/src/Service/Resolvers/Sql Query Structures/BaseSqlQueryStructure.cs b/src/Service/Resolvers/Sql Query Structures/BaseSqlQueryStructure.cs index 41dc825a9f..4851140f4c 100644 --- a/src/Service/Resolvers/Sql Query Structures/BaseSqlQueryStructure.cs +++ b/src/Service/Resolvers/Sql Query Structures/BaseSqlQueryStructure.cs @@ -202,17 +202,10 @@ protected object GetParamAsColumnSystemType(string param, string columnName) { return ParseParamAsSystemType(param, systemType); } - catch (Exception e) + catch (Exception e) when (e is FormatException || e is ArgumentNullException || e is OverflowException) { - if (e is FormatException || - e is ArgumentNullException || - e is OverflowException) - { - throw new ArgumentException($"Parameter \"{param}\" cannot be resolved as column \"{columnName}\" " + + throw new ArgumentException($"Parameter \"{param}\" cannot be resolved as column \"{columnName}\" " + $"with type \"{systemType.Name}\"."); - } - - throw; } } From 0efa127c838660127f016a6ae5b9a5dd863825ef Mon Sep 17 00:00:00 2001 From: Sean Leonard Date: Tue, 27 Sep 2022 12:25:11 -0700 Subject: [PATCH 3/9] preserve stack trace. --- .../Resolvers/Sql Query Structures/BaseSqlQueryStructure.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Service/Resolvers/Sql Query Structures/BaseSqlQueryStructure.cs b/src/Service/Resolvers/Sql Query Structures/BaseSqlQueryStructure.cs index 4851140f4c..d2fb8045fd 100644 --- a/src/Service/Resolvers/Sql Query Structures/BaseSqlQueryStructure.cs +++ b/src/Service/Resolvers/Sql Query Structures/BaseSqlQueryStructure.cs @@ -204,8 +204,10 @@ protected object GetParamAsColumnSystemType(string param, string columnName) } catch (Exception e) when (e is FormatException || e is ArgumentNullException || e is OverflowException) { - throw new ArgumentException($"Parameter \"{param}\" cannot be resolved as column \"{columnName}\" " + - $"with type \"{systemType.Name}\"."); + throw new ArgumentException( + message: $"Parameter \"{param}\" cannot be resolved as column \"{columnName}\" " + + $"with type \"{systemType.Name}\".", + innerException: e); } } From 5531486defe49633081b6aac75507da31eb447a9 Mon Sep 17 00:00:00 2001 From: Sean Leonard Date: Tue, 11 Oct 2022 12:54:52 -0700 Subject: [PATCH 4/9] remove extra whitespace --- .../Resolvers/Sql Query Structures/SqlDeleteQueryStructure.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Service/Resolvers/Sql Query Structures/SqlDeleteQueryStructure.cs b/src/Service/Resolvers/Sql Query Structures/SqlDeleteQueryStructure.cs index d1460b700a..58c21d5e0e 100644 --- a/src/Service/Resolvers/Sql Query Structures/SqlDeleteQueryStructure.cs +++ b/src/Service/Resolvers/Sql Query Structures/SqlDeleteQueryStructure.cs @@ -45,7 +45,7 @@ public SqlDeleteStructure( new PredicateOperand($"@{MakeParamWithValue(GetParamAsColumnSystemType(param.Value.ToString()!, backingColumn!))}") )); } - } + } } catch (ArgumentException ex) { From d1a0eec2b6bb42ec99e887d4a4a226ee27cdbc35 Mon Sep 17 00:00:00 2001 From: Sean Leonard Date: Tue, 11 Oct 2022 13:39:30 -0700 Subject: [PATCH 5/9] Added integration test coverage for all REST operations to validate invalid PK Value cast error handling. --- .../RestApiTests/Delete/DeleteApiTestBase.cs | 20 ++++++++++++++ .../RestApiTests/Find/FindApiTestBase.cs | 18 +++++++++++++ .../RestApiTests/Insert/InsertApiTestBase.cs | 26 +++++++++++++++++++ .../RestApiTests/Patch/PatchApiTestBase.cs | 25 ++++++++++++++++++ .../RestApiTests/Put/PutApiTestBase.cs | 26 +++++++++++++++++++ 5 files changed, 115 insertions(+) diff --git a/src/Service.Tests/SqlTests/RestApiTests/Delete/DeleteApiTestBase.cs b/src/Service.Tests/SqlTests/RestApiTests/Delete/DeleteApiTestBase.cs index f03b519689..5aff0080b1 100644 --- a/src/Service.Tests/SqlTests/RestApiTests/Delete/DeleteApiTestBase.cs +++ b/src/Service.Tests/SqlTests/RestApiTests/Delete/DeleteApiTestBase.cs @@ -159,6 +159,26 @@ await SetupAndRunRestApiTest( ); } + /// + /// Tests that a cast failure of primary key value type results in HTTP 400 Bad Request. + /// e.g. Attempt to cast a string '{}' to the 'id' column type of int will fail. + /// + [TestMethod] + public async Task DeleteWithUncastablePKValue() + { + await SetupAndRunRestApiTest( + primaryKeyRoute: "id/{}", + queryString: string.Empty, + entityNameOrPath: _integrationEntityName, + sqlQuery: string.Empty, + operationType: Operation.Delete, + requestBody: string.Empty, + exceptionExpected: true, + expectedErrorMessage: "Parameter \"{}\" cannot be resolved as column \"id\" with type \"Int32\".", + expectedStatusCode: HttpStatusCode.BadRequest, + expectedSubStatusCode: DataApiBuilderException.SubStatusCodes.BadRequest.ToString() + ); + } /// /// DeleteWithSqlInjectionTest attempts to inject a SQL statement /// through the primary key route of a delete operation. diff --git a/src/Service.Tests/SqlTests/RestApiTests/Find/FindApiTestBase.cs b/src/Service.Tests/SqlTests/RestApiTests/Find/FindApiTestBase.cs index b30b3cde4a..01571b3b26 100644 --- a/src/Service.Tests/SqlTests/RestApiTests/Find/FindApiTestBase.cs +++ b/src/Service.Tests/SqlTests/RestApiTests/Find/FindApiTestBase.cs @@ -1451,6 +1451,24 @@ await SetupAndRunRestApiTest( ); } + /// + /// Tests that a cast failure of primary key value type results in HTTP 400 Bad Request. + /// e.g. Attempt to cast a string '{}' to the 'id' column type of int will fail. + /// + [TestMethod] + public async Task FindWithUncastablePKValue() + { + await SetupAndRunRestApiTest( + primaryKeyRoute: "id/{}", + queryString: string.Empty, + entityNameOrPath: _integrationEntityName, + sqlQuery: null, + exceptionExpected: true, + expectedErrorMessage: "Parameter \"{}\" cannot be resolved as column \"id\" with type \"Int32\".", + expectedStatusCode: HttpStatusCode.BadRequest + ); + } + /// /// Tests the REST Api for FindById operation with attempts at /// Sql Injection in the primary key route. diff --git a/src/Service.Tests/SqlTests/RestApiTests/Insert/InsertApiTestBase.cs b/src/Service.Tests/SqlTests/RestApiTests/Insert/InsertApiTestBase.cs index 74ff7830b0..fea23c18d9 100644 --- a/src/Service.Tests/SqlTests/RestApiTests/Insert/InsertApiTestBase.cs +++ b/src/Service.Tests/SqlTests/RestApiTests/Insert/InsertApiTestBase.cs @@ -362,6 +362,32 @@ await SetupAndRunRestApiTest( ); } + /// + /// Tests that a cast failure of primary key value type results in HTTP 400 Bad Request. + /// e.g. Attempt to cast a string '{}' to the 'publisher_id' column type of int will fail. + /// + [TestMethod] + public async Task InsertWithUncastablePKValue() + { + string requestBody = @" + { + ""title"": ""BookTitle"", + ""publisher_id"": ""StringFailsToCastToInt"" + }"; + + await SetupAndRunRestApiTest( + primaryKeyRoute: string.Empty, + queryString: string.Empty, + entityNameOrPath: _integrationEntityName, + sqlQuery: null, + operationType: Operation.Insert, + requestBody: requestBody, + exceptionExpected: true, + expectedErrorMessage: "Parameter \"StringFailsToCastToInt\" cannot be resolved as column \"publisher_id\" with type \"Int32\".", + expectedStatusCode: HttpStatusCode.BadRequest + ); + } + /// /// Tests the InsertOne functionality with a missing field in the request body: /// A non-nullable field in the Json Body is missing. diff --git a/src/Service.Tests/SqlTests/RestApiTests/Patch/PatchApiTestBase.cs b/src/Service.Tests/SqlTests/RestApiTests/Patch/PatchApiTestBase.cs index fa36113aa6..a9ccc82c51 100644 --- a/src/Service.Tests/SqlTests/RestApiTests/Patch/PatchApiTestBase.cs +++ b/src/Service.Tests/SqlTests/RestApiTests/Patch/PatchApiTestBase.cs @@ -473,6 +473,31 @@ await SetupAndRunRestApiTest( ); } + /// + /// Tests that a cast failure of primary key value type results in HTTP 400 Bad Request. + /// e.g. Attempt to cast a string '{}' to the 'publisher_id' column type of int will fail. + /// + [TestMethod] + public async Task PatchWithUncastablePKValue() + { + string requestBody = @" + { + ""publisher_id"": ""StringFailsToCastToInt"" + }"; + + await SetupAndRunRestApiTest( + primaryKeyRoute: "id/1", + queryString: string.Empty, + entityNameOrPath: _integrationEntityName, + sqlQuery: null, + operationType: Operation.UpsertIncremental, + requestBody: requestBody, + exceptionExpected: true, + expectedErrorMessage: "Parameter \"StringFailsToCastToInt\" cannot be resolved as column \"publisher_id\" with type \"Int32\".", + expectedStatusCode: HttpStatusCode.BadRequest + ); + } + /// /// Tests the Patch functionality with a REST PATCH request /// without a primary key route. We expect a failure and so diff --git a/src/Service.Tests/SqlTests/RestApiTests/Put/PutApiTestBase.cs b/src/Service.Tests/SqlTests/RestApiTests/Put/PutApiTestBase.cs index 1f556d2d0a..bc5988da8e 100644 --- a/src/Service.Tests/SqlTests/RestApiTests/Put/PutApiTestBase.cs +++ b/src/Service.Tests/SqlTests/RestApiTests/Put/PutApiTestBase.cs @@ -643,6 +643,32 @@ await SetupAndRunRestApiTest( ); } + /// + /// Tests that a cast failure of primary key value type results in HTTP 400 Bad Request. + /// e.g. Attempt to cast a string '{}' to the 'publisher_id' column type of int will fail. + /// + [TestMethod] + public async Task PutWithUncastablePKValue() + { + string requestBody = @" + { + ""title"": ""BookTitle"", + ""publisher_id"": ""StringFailsToCastToInt"" + }"; + + await SetupAndRunRestApiTest( + primaryKeyRoute: "id/1", + queryString: string.Empty, + entityNameOrPath: _integrationEntityName, + sqlQuery: null, + operationType: Operation.Upsert, + requestBody: requestBody, + exceptionExpected: true, + expectedErrorMessage: "Parameter \"StringFailsToCastToInt\" cannot be resolved as column \"publisher_id\" with type \"Int32\".", + expectedStatusCode: HttpStatusCode.BadRequest + ); + } + /// /// Tests the Put functionality with a REST PUT request /// with the request body having null value for non-nullable column From 39471123fef7e97fc4369e4ba109a672663bb28a Mon Sep 17 00:00:00 2001 From: Sean Leonard Date: Thu, 13 Oct 2022 10:00:26 -0700 Subject: [PATCH 6/9] Add innerException tracking when raising DAB exception. --- .../Resolvers/Sql Query Structures/SqlDeleteQueryStructure.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Service/Resolvers/Sql Query Structures/SqlDeleteQueryStructure.cs b/src/Service/Resolvers/Sql Query Structures/SqlDeleteQueryStructure.cs index 58c21d5e0e..1503fdfb8d 100644 --- a/src/Service/Resolvers/Sql Query Structures/SqlDeleteQueryStructure.cs +++ b/src/Service/Resolvers/Sql Query Structures/SqlDeleteQueryStructure.cs @@ -53,7 +53,8 @@ public SqlDeleteStructure( throw new DataApiBuilderException( message: ex.Message, statusCode: HttpStatusCode.BadRequest, - subStatusCode: DataApiBuilderException.SubStatusCodes.BadRequest); + subStatusCode: DataApiBuilderException.SubStatusCodes.BadRequest, + innerException: ex); } } } From 3b4e4e63e47730a192caea5c46ecdab8ac17a129 Mon Sep 17 00:00:00 2001 From: Sean Leonard Date: Thu, 13 Oct 2022 10:01:03 -0700 Subject: [PATCH 7/9] remove outdated comment. --- .../Resolvers/Sql Query Structures/SqlDeleteQueryStructure.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Service/Resolvers/Sql Query Structures/SqlDeleteQueryStructure.cs b/src/Service/Resolvers/Sql Query Structures/SqlDeleteQueryStructure.cs index 1503fdfb8d..594f29b266 100644 --- a/src/Service/Resolvers/Sql Query Structures/SqlDeleteQueryStructure.cs +++ b/src/Service/Resolvers/Sql Query Structures/SqlDeleteQueryStructure.cs @@ -49,7 +49,6 @@ public SqlDeleteStructure( } catch (ArgumentException ex) { - // ArgumentException thrown from GetParamAsColumnSystemType() throw new DataApiBuilderException( message: ex.Message, statusCode: HttpStatusCode.BadRequest, From a088239cf9bfa766cca710ef59d6ea36602492b0 Mon Sep 17 00:00:00 2001 From: Sean Leonard Date: Thu, 13 Oct 2022 10:02:13 -0700 Subject: [PATCH 8/9] Raise DAB exception instead of argument exception --- .../Resolvers/Sql Query Structures/BaseSqlQueryStructure.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Service/Resolvers/Sql Query Structures/BaseSqlQueryStructure.cs b/src/Service/Resolvers/Sql Query Structures/BaseSqlQueryStructure.cs index 217d61e369..a3576ad5e1 100644 --- a/src/Service/Resolvers/Sql Query Structures/BaseSqlQueryStructure.cs +++ b/src/Service/Resolvers/Sql Query Structures/BaseSqlQueryStructure.cs @@ -204,9 +204,11 @@ protected object GetParamAsColumnSystemType(string param, string columnName) } catch (Exception e) when (e is FormatException || e is ArgumentNullException || e is OverflowException) { - throw new ArgumentException( + throw new DataApiBuilderException( message: $"Parameter \"{param}\" cannot be resolved as column \"{columnName}\" " + $"with type \"{systemType.Name}\".", + statusCode: HttpStatusCode.BadRequest, + subStatusCode: DataApiBuilderException.SubStatusCodes.BadRequest, innerException: e); } } From fabadb16e14a17f64100c9cde78513d9bbf58dfb Mon Sep 17 00:00:00 2001 From: Sean Leonard Date: Wed, 19 Oct 2022 11:40:56 -0700 Subject: [PATCH 9/9] revised try/catch structure. --- .../RestApiTests/Delete/DeleteApiTestBase.cs | 3 +- .../BaseSqlQueryStructure.cs | 6 ++- .../SqlDeleteQueryStructure.cs | 48 +++++++------------ 3 files changed, 25 insertions(+), 32 deletions(-) diff --git a/src/Service.Tests/SqlTests/RestApiTests/Delete/DeleteApiTestBase.cs b/src/Service.Tests/SqlTests/RestApiTests/Delete/DeleteApiTestBase.cs index 5aff0080b1..87cffa252c 100644 --- a/src/Service.Tests/SqlTests/RestApiTests/Delete/DeleteApiTestBase.cs +++ b/src/Service.Tests/SqlTests/RestApiTests/Delete/DeleteApiTestBase.cs @@ -177,8 +177,9 @@ await SetupAndRunRestApiTest( expectedErrorMessage: "Parameter \"{}\" cannot be resolved as column \"id\" with type \"Int32\".", expectedStatusCode: HttpStatusCode.BadRequest, expectedSubStatusCode: DataApiBuilderException.SubStatusCodes.BadRequest.ToString() - ); + ); } + /// /// DeleteWithSqlInjectionTest attempts to inject a SQL statement /// through the primary key route of a delete operation. diff --git a/src/Service/Resolvers/Sql Query Structures/BaseSqlQueryStructure.cs b/src/Service/Resolvers/Sql Query Structures/BaseSqlQueryStructure.cs index a3576ad5e1..6edced68f7 100644 --- a/src/Service/Resolvers/Sql Query Structures/BaseSqlQueryStructure.cs +++ b/src/Service/Resolvers/Sql Query Structures/BaseSqlQueryStructure.cs @@ -122,7 +122,11 @@ public Type GetColumnSystemType(string columnName) } else { - throw new ArgumentException($"{columnName} is not a valid column of {DatabaseObject.Name}"); + throw new DataApiBuilderException( + message: $"{columnName} is not a valid column of {DatabaseObject.Name}", + statusCode: HttpStatusCode.BadRequest, + subStatusCode: DataApiBuilderException.SubStatusCodes.BadRequest + ); } } diff --git a/src/Service/Resolvers/Sql Query Structures/SqlDeleteQueryStructure.cs b/src/Service/Resolvers/Sql Query Structures/SqlDeleteQueryStructure.cs index 594f29b266..353b6152fd 100644 --- a/src/Service/Resolvers/Sql Query Structures/SqlDeleteQueryStructure.cs +++ b/src/Service/Resolvers/Sql Query Structures/SqlDeleteQueryStructure.cs @@ -1,4 +1,3 @@ -using System; using System.Collections.Generic; using System.Net; using Azure.DataApiBuilder.Config; @@ -21,40 +20,29 @@ public SqlDeleteStructure( { TableDefinition tableDefinition = GetUnderlyingTableDefinition(); - try + List primaryKeys = tableDefinition.PrimaryKey; + foreach (KeyValuePair param in mutationParams) { - List primaryKeys = tableDefinition.PrimaryKey; - foreach (KeyValuePair param in mutationParams) + if (param.Value is null) { - if (param.Value is null) - { - // Should never happen since delete mutations expect non nullable pk params - throw new DataApiBuilderException( - message: $"Unexpected {param.Key} null argument.", - statusCode: HttpStatusCode.BadRequest, - subStatusCode: DataApiBuilderException.SubStatusCodes.BadRequest); - } + // Should never happen since delete mutations expect non nullable pk params + throw new DataApiBuilderException( + message: $"Unexpected {param.Key} null argument.", + statusCode: HttpStatusCode.BadRequest, + subStatusCode: DataApiBuilderException.SubStatusCodes.BadRequest); + } - // primary keys used as predicates - SqlMetadataProvider.TryGetBackingColumn(EntityName, param.Key, out string? backingColumn); - if (primaryKeys.Contains(backingColumn!)) - { - Predicates.Add(new Predicate( - new PredicateOperand(new Column(DatabaseObject.SchemaName, DatabaseObject.Name, backingColumn!)), - PredicateOperation.Equal, - new PredicateOperand($"@{MakeParamWithValue(GetParamAsColumnSystemType(param.Value.ToString()!, backingColumn!))}") - )); - } + // primary keys used as predicates + SqlMetadataProvider.TryGetBackingColumn(EntityName, param.Key, out string? backingColumn); + if (primaryKeys.Contains(backingColumn!)) + { + Predicates.Add(new Predicate( + new PredicateOperand(new Column(DatabaseObject.SchemaName, DatabaseObject.Name, backingColumn!)), + PredicateOperation.Equal, + new PredicateOperand($"@{MakeParamWithValue(GetParamAsColumnSystemType(param.Value.ToString()!, backingColumn!))}") + )); } } - catch (ArgumentException ex) - { - throw new DataApiBuilderException( - message: ex.Message, - statusCode: HttpStatusCode.BadRequest, - subStatusCode: DataApiBuilderException.SubStatusCodes.BadRequest, - innerException: ex); - } } } }