Return generic error messages in production mode #1357

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

severussundar merged 44 commits into main from dev/shyamsundarj/genericize-error-messages-bad-requests

Apr 2, 2023

src/Service.Tests/Configuration/ConfigurationTests.cs

-Original file line number
+Diff line change
@@ Expand Up / @@ -51,6 +51,26 @@ public class ConfigurationTests @@
             private const int RETRY_COUNT = 5;
             private const int RETRY_WAIT_SECONDS = 1;
+            /// <summary>
+            /// A valid REST API request body with correct parameter types for all the fields.
+            /// </summary>
+            public const string REQUEST_BODY_WITH_CORRECT_PARAM_TYPES = @"
+                        {
+                            ""title"": ""New book"",
+                            ""publisher_id"": 1234
+                        }
+                    ";
+            /// <summary>
+            /// An invalid REST API request body with incorrect parameter type for publisher_id field.
+            /// </summary>
+            public const string REQUEST_BODY_WITH_INCORRECT_PARAM_TYPES = @"
+                        {
+                            ""title"": ""New book"",
+                            ""publisher_id"": ""one""
+                        }
+                    ";
             public TestContext TestContext { get; set; }
             [TestInitialize]
@@ Expand Down Expand Up / @@ -946,6 +966,65 @@ public async Task TestPathRewriteMiddlewareForGraphQL( @@
                 }
             }
+            /// <summary>
+            /// Validates the error message that is returned for REST requests with incorrect parameter type
+            /// when the engine is running in Production mode. The error messages in Production mode is
+            /// very generic to not reveal information about the underlying database objects backing the entity.
+            /// This test runs against a MsSql database. However, generic error messages will be returned in Production
+            /// mode when run against PostgreSql and MySql databases.
+            /// </summary>
+            /// <param name="requestType">Type of REST request</param>
+            /// <param name="requestPath">Endpoint for the REST request</param>
+            /// <param name="expectedErrorMessage">Right error message that should be shown to the end user</param>
+            [DataTestMethod]
+            [TestCategory(TestCategory.MSSQL)]
+            [DataRow(RestMethod.Get, "/api/Book/id/one", null, "Invalid value provided for field: id", DisplayName = "Validates the error message for a GET request with incorrect primary key parameter type on a table in production mode")]
+            [DataRow(RestMethod.Get, "/api/books_view_all/id/one", null, "Invalid value provided for field: id", DisplayName = "Validates the error message for a GET request with incorrect primary key parameter type on a view in production mode")]
+            [DataRow(RestMethod.Get, "/api/GetBook?id=one", REQUEST_BODY_WITH_CORRECT_PARAM_TYPES, "Invalid value provided for field: id", DisplayName = "Validates the error message for a GET request on a stored-procedure with incorrect parameter type in production mode")]
+            [DataRow(RestMethod.Get, "/api/GQLmappings/column1/one", null, "Invalid value provided for field: column1", DisplayName = "Validates the error message for a GET request with incorrect primary key parameter type with alias defined for primary key column on a table in production mode")]
+            [DataRow(RestMethod.Post, "/api/Book", REQUEST_BODY_WITH_INCORRECT_PARAM_TYPES, "Invalid value provided for field: publisher_id", DisplayName = "Validates the error message for a POST request with incorrect parameter type in the request body on a table in production mode")]
+            [DataRow(RestMethod.Put, "/api/Book/id/one", REQUEST_BODY_WITH_CORRECT_PARAM_TYPES, "Invalid value provided for field: id", DisplayName = "Validates the error message for a PUT request with incorrect primary key parameter type on a table in production mode")]
+            [DataRow(RestMethod.Put, "/api/Book/id/1", REQUEST_BODY_WITH_INCORRECT_PARAM_TYPES, "Invalid value provided for field: publisher_id", DisplayName = "Validates the error message for a bad PUT request with incorrect parameter type in the request body on a table in production mode")]
+            [DataRow(RestMethod.Patch, "/api/Book/id/one", REQUEST_BODY_WITH_CORRECT_PARAM_TYPES, "Invalid value provided for field: id", DisplayName = "Validates the error message for a PATCH request with incorrect primary key parameter type on a table in production mode")]
+            [DataRow(RestMethod.Patch, "/api/Book/id/1", REQUEST_BODY_WITH_INCORRECT_PARAM_TYPES, "Invalid value provided for field: publisher_id", DisplayName = "Validates the error message for a PATCH request with incorrect parameter type in the request body on a table in production mode")]
+            [DataRow(RestMethod.Delete, "/api/Book/id/one", REQUEST_BODY_WITH_CORRECT_PARAM_TYPES, "Invalid value provided for field: id", DisplayName = "Validates the error message for a DELETE request with incorrect primary key parameter type on a table in production mode")]
+            public async Task TestGenericErrorMessageForRestApiInProductionMode(
+                RestMethod requestType,
+                string requestPath,
+                string requestBody,
+                string expectedErrorMessage)
+            {
+                const string CUSTOM_CONFIG = "custom-config.json";
+                TestHelper.ConstructNewConfigWithSpecifiedHostMode(CUSTOM_CONFIG, HostModeType.Production, TestCategory.MSSQL);
+                string[] args = new[]
+                {
+                        $"--ConfigFileName={CUSTOM_CONFIG}"
+                };
+                using (TestServer server = new(Program.CreateWebHostBuilder(args)))
+                using (HttpClient client = server.CreateClient())
+                {
+                    HttpMethod httpMethod = SqlTestHelper.ConvertRestMethodToHttpMethod(requestType);
+                    HttpRequestMessage request;
+                    if (requestType is RestMethod.Get || requestType is RestMethod.Delete)
+                    {
+                        request = new(httpMethod, requestPath);
+                    }
+                    else
+                    {
+                        request = new(httpMethod, requestPath)
+                        {
+                            Content = JsonContent.Create(requestBody)
+                        };
+                    }
+                    HttpResponseMessage response = await client.SendAsync(request);
+                    string body = await response.Content.ReadAsStringAsync();
+                    Assert.AreEqual(HttpStatusCode.BadRequest, response.StatusCode);
+                    Assert.IsTrue(body.Contains(expectedErrorMessage));
+                }
+            }
             /// <summary>
             /// Tests that the when Rest or GraphQL is disabled Globally,
             /// any requests made will get a 404 response.
@@ Expand Down @@

src/Service.Tests/SqlTests/SqlTestHelper.cs

-Original file line number
+Diff line change
@@ Expand Up @@
             /// </summary>
             /// <param name="restMethod"></param>
             /// <returns>HttpMethod corresponding the RestMethod provided as input.</returns>
-            private static HttpMethod ConvertRestMethodToHttpMethod(RestMethod? restMethod)
+            public static HttpMethod ConvertRestMethodToHttpMethod(RestMethod? restMethod)
             {
                 switch (restMethod)
                 {
@@ Expand Down @@

src/Service.Tests/TestHelper.cs

-Original file line number
+Diff line change
@@ Expand Up @@
                 },
                 ""entities"": {}" +
               "}";
+            /// <summary>
+            /// Utility method that reads the config file for a given database type and constructs a
+            /// new config file with changes just in the host mode section.
+            /// </summary>
+            /// <param name="configFileName">Name of the new config file to be constructed</param>
+            /// <param name="hostModeType">HostMode for the engine</param>
+            /// <param name="databaseType">Database type</param>
+            public static void ConstructNewConfigWithSpecifiedHostMode(string configFileName, HostModeType hostModeType, string databaseType)
+            {
+                RuntimeConfigProvider configProvider = TestHelper.GetRuntimeConfigProvider(databaseType);
+                RuntimeConfig config = configProvider.GetRuntimeConfiguration();
+                HostGlobalSettings customHostGlobalSettings = config.HostGlobalSettings with { Mode = hostModeType };
+                JsonElement serializedCustomHostGlobalSettings =
+                    JsonSerializer.SerializeToElement(customHostGlobalSettings, RuntimeConfig.SerializerOptions);
+                Dictionary<GlobalSettingsType, object> customRuntimeSettings = new(config.RuntimeSettings);
+                customRuntimeSettings.Remove(GlobalSettingsType.Host);
+                customRuntimeSettings.Add(GlobalSettingsType.Host, serializedCustomHostGlobalSettings);
+                RuntimeConfig configWithCustomHostMode =
+                    config with { RuntimeSettings = customRuntimeSettings };
+                File.WriteAllText(
+                    configFileName,
+                    JsonSerializer.Serialize(configWithCustomHostMode, RuntimeConfig.SerializerOptions));
+            }
         }
     }

src/Service/Resolvers/Sql Query Structures/BaseSqlQueryStructure.cs

-Original file line number
+Diff line change
@@ Expand Up / @@ -330,30 +330,6 @@ protected List<LabelledColumn> GenerateOutputColumns() @@
                 return outputColumns;
             }
-            ///<summary>
-            /// Gets the value of the parameter cast as the system type
-            /// of the column this parameter is associated with
-            ///</summary>
-            /// <exception cref="ArgumentException">columnName is not a valid column of table or param
-            /// does not have a valid value type</exception>
-            protected object GetParamAsColumnSystemType(string param, string columnName)
-            {
-                Type systemType = GetColumnSystemType(columnName);
-                try
-                {
-                    return ParseParamAsSystemType(param, systemType);
-                }
-                catch (Exception e) when (e is FormatException || e is ArgumentNullException || e is OverflowException)
-                {
-                    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);
-                }
-            }
             /// <summary>
             /// Tries to parse the string parameter to the given system type
             /// Useful for inferring parameter types for columns or procedure parameters
@@ Expand Down Expand Up / @@ -506,5 +482,62 @@ public void ProcessOdataClause(FilterClause odataClause) @@
             {
                 return filterClause.Expression.Accept<string>(visitor);
             }
+            /// <summary>
+            /// Gets the value of the parameter cast as the system type
+            /// </summary>
+            /// <param name="fieldValue">Field value as a string</param>
+            /// <param name="fieldName">Field name whose value is being converted to the specified system type. This is used only for constructing the error messages incase of conversion failures</param>
+            /// <param name="systemType">System type to which the parameter value is parsed to</param>
+            /// <returns>The parameter value parsed to the specified system type</returns>
+            /// <exception cref="DataApiBuilderException">Raised when the conversion of parameter value to the specified system type fails. The error message returned will be different in development
+            /// and production modes. In production mode, the error message returned will be generic so as to not reveal information about the database object backing the entity</exception>
+            protected object GetParamAsSystemType(string fieldValue, string fieldName, Type systemType)
+            {
+                try
+                {
+                    return ParseParamAsSystemType(fieldValue, systemType);
+                }
+                catch (Exception e) when (e is FormatException || e is ArgumentNullException || e is OverflowException)
+                {
+                    string errorMessage;
+                    SourceType sourceTypeOfDbObject = MetadataProvider.EntityToDatabaseObject[EntityName].SourceType;
+                    if (MetadataProvider.IsDevelopmentMode())
+                    {
+                        if (sourceTypeOfDbObject is SourceType.StoredProcedure)
+                        {
+                            errorMessage = $@"Parameter ""{fieldValue}"" cannot be resolved as stored procedure parameter ""{fieldName}"" " +
+                                    $@"with type ""{systemType.Name}"".";
+                        }
+                        else
+                        {
+                            errorMessage = $"Parameter \"{fieldValue}\" cannot be resolved as column \"{fieldName}\" " +
+                                    $"with type \"{systemType.Name}\".";
+                        }
+                    }
+                    else
+                    {
+                        string fieldNameToBeDisplayedInErrorMessage = fieldName;
+                        if (sourceTypeOfDbObject is SourceType.Table || sourceTypeOfDbObject is SourceType.View)
+                        {
+                            if (MetadataProvider.TryGetExposedColumnName(EntityName, fieldName, out string? exposedName))
+                            {
+                                fieldNameToBeDisplayedInErrorMessage = exposedName!;
+                            }
+                        }
+                        errorMessage = $"Invalid value provided for field: {fieldNameToBeDisplayedInErrorMessage}";
+                    }
+                    throw new DataApiBuilderException(
+                        message: errorMessage,
+                        statusCode: HttpStatusCode.BadRequest,
+                        subStatusCode: DataApiBuilderException.SubStatusCodes.BadRequest,
+                        innerException: e);
+                }
+            }
         }
     }

src/Service/Resolvers/Sql Query Structures/SqlDeleteQueryStructure.cs

            
                      Original file line number
                      Diff line number
                      Diff line change
                  
    @@ -53,7 +53,7 @@ public SqlDeleteStructure(
  
                        Predicates.Add(new Predicate(

                            new PredicateOperand(new Column(DatabaseObject.SchemaName, DatabaseObject.Name, backingColumn!)),

                            PredicateOperation.Equal,

                            new PredicateOperand($"{MakeParamWithValue(GetParamAsColumnSystemType(param.Value.ToString()!, backingColumn!))}")

                            new PredicateOperand($"{MakeParamWithValue(GetParamAsSystemType(param.Value.ToString()!, backingColumn!, GetColumnSystemType(backingColumn!)))}")

                        ));

                    }

                }

src/Service/Resolvers/Sql Query Structures/SqlExecuteQueryStructure.cs

-Original file line number
+Diff line change
@@ Expand Up / @@ -44,22 +44,18 @@ public SqlExecuteStructure( @@
                     if (requestParams.TryGetValue(paramKey, out object? requestParamValue))
                     {
                         // Parameterize, then add referencing parameter to ProcedureParameters dictionary
-                        try
+                        string? parametrizedName = null;
+                        if (requestParamValue is not null)
                         {
-                            string parameterizedName = MakeParamWithValue(requestParamValue is null ? null :
-                                GetParamAsProcedureParameterType(requestParamValue.ToString()!, paramKey));
-                            ProcedureParameters.Add(paramKey, $"{parameterizedName}");
+                            Type systemType = GetUnderlyingStoredProcedureDefinition().Parameters[paramKey].SystemType!;
+                            parametrizedName = MakeParamWithValue(GetParamAsSystemType(requestParamValue.ToString()!, paramKey, systemType));
                         }
-                        catch (ArgumentException ex)
+                        else
                         {
-                            // In the case GetParamAsProcedureParameterType fails to parse as SystemType from database metadata
-                            // Keep message being returned to the client more generalized to not expose schema info
-                            throw new DataApiBuilderException(
-                                message: $"Invalid value supplied for field: {paramKey}",
-                                statusCode: HttpStatusCode.BadRequest,
-                                subStatusCode: DataApiBuilderException.SubStatusCodes.BadRequest,
-                                innerException: ex);
+                            parametrizedName = MakeParamWithValue(value: null);
                         }
+                        ProcedureParameters.Add(paramKey, $"{parametrizedName}");
                     }
                     else
                     {
@@ Expand All / @@ -80,30 +76,5 @@ public SqlExecuteStructure( @@
                     }
                 }
             }
-            /// <summary>
-            /// Gets the value of the parameter cast as the system type
-            /// of the stored procedure parameter this parameter is associated with
-            /// </summary>
-            private object GetParamAsProcedureParameterType(string param, string procParamName)
-            {
-                Type systemType = GetUnderlyingStoredProcedureDefinition().Parameters[procParamName].SystemType!;
-                try
-                {
-                    return ParseParamAsSystemType(param, systemType);
-                }
-                catch (Exception e)
-                {
-                    if (e is FormatException ||
-                        e is ArgumentNullException ||
-                        e is OverflowException)
-                    {
-                        throw new ArgumentException($@"Parameter ""{param}"" cannot be resolved as stored procedure parameter ""{procParamName}"" " +
-                            $@"with type ""{systemType.Name}"".", innerException: e);
-                    }
-                    throw;
-                }
-            }
         }
     }

src/Service/Resolvers/Sql Query Structures/SqlInsertQueryStructure.cs

            
                      Original file line number
                      Diff line number
                      Diff line change
                  
    @@ -1,12 +1,9 @@
  
    // Copyright (c) Microsoft Corporation.

    // Licensed under the MIT License.

    using System;

    using System.Collections.Generic;

    using System.Net;

    using Azure.DataApiBuilder.Auth;

    using Azure.DataApiBuilder.Config;

    using Azure.DataApiBuilder.Service.Exceptions;

    using Azure.DataApiBuilder.Service.GraphQLBuilder.Mutations;

    using Azure.DataApiBuilder.Service.Models;

    using Azure.DataApiBuilder.Service.Services;

    @@ -90,28 +87,17 @@ private void PopulateColumnsAndParams(string columnName, object? value)
  
                InsertColumns.Add(columnName);

                string paramName;

                try

                if (value is not null)

                {

                    if (value != null)

                    {

                        paramName = MakeParamWithValue(

                            GetParamAsColumnSystemType(value.ToString()!, columnName));

                    }

                    else

                    {

                        paramName = MakeParamWithValue(null);

                    }

                    Values.Add($"{paramName}");

                    paramName = MakeParamWithValue(

                        GetParamAsSystemType(value.ToString()!, columnName, GetColumnSystemType(columnName)));

                }

                catch (ArgumentException ex)

                else

                {

                    throw new DataApiBuilderException(

                        message: ex.Message,

                        statusCode: HttpStatusCode.BadRequest,

                        subStatusCode: DataApiBuilderException.SubStatusCodes.BadRequest,

                        innerException: ex);

                    paramName = MakeParamWithValue(value: null);

                }

                Values.Add($"{paramName}");

            }

            /// <summary>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return generic error messages in production mode #1357

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!