From ad7b1623f4ac7befcc39b7332cfc83c8b489f5f7 Mon Sep 17 00:00:00 2001 From: Shane32 Date: Sun, 18 Aug 2024 21:34:52 -0400 Subject: [PATCH] Allow option to disable handling of form data for POST requests --- README.md | 22 ++++++++++++++++ .../GraphQLHttpMiddleware.cs | 10 ++++++-- .../GraphQLHttpMiddlewareOptions.cs | 13 ++++++++++ .../GraphQL.AspNetCore3.approved.txt | 1 + src/Tests/Middleware/PostTests.cs | 25 ++++++++++++++----- 5 files changed, 63 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 7f14bf3..74844fb 100644 --- a/README.md +++ b/README.md @@ -549,6 +549,7 @@ endpoint. | `MaximumFileSize` | Sets the maximum file size allowed for GraphQL multipart requests. | unlimited | | `MaximumFileCount` | Sets the maximum number of files allowed for GraphQL multipart requests. | unlimited | | `ReadExtensionsFromQueryString` | Enables reading extensions from the query string. | True | +| `ReadFormOnPost` | Enables parsing of form data for POST requests (may have security implications). | True | | `ReadQueryStringOnPost` | Enables parsing the query string on POST requests. | True | | `ReadVariablesFromQueryString` | Enables reading variables from the query string. | True | | `ValidationErrorsReturnBadRequest` | When enabled, GraphQL requests with validation errors have the HTTP status code set to 400 Bad Request. | True | @@ -758,6 +759,27 @@ are rejected over HTTP GET connections. Derive from `GraphQLHttpMiddleware` and As would be expected, subscription requests are only allowed over WebSocket channels. +### Handling form data for POST requests + +The GraphQL over HTTP specification does not outline a procedure for transmitting GraphQL requests via +HTTP POST connections using a `Content-Type` of `application/x-www-form-urlencoded` or `multipart/form-data`. +Allowing the processing of such requests could be advantageous in avoiding CORS preflight requests when +sending GraphQL queries from a web browser. Nevertheless, enabling this feature may give rise to security +risks when utilizing cookie authentication, since transmitting cookies with these requests does not trigger +a pre-flight CORS check. As a consequence, GraphQL.NET might execute a request and potentially modify data +even when the CORS policy prohibits it, regardless of whether the sender has access to the response. +This situation exposes the system to security vulnerabilities, which should be carefully evaluated and +mitigated to ensure the safe handling of GraphQL requests and maintain the integrity of the data. + +This functionality is activated by default to maintain backward compatibility, but it can be turned off by +setting the `ReadFormOnPost` value to `false`. The next major version of GraphQL.NET Server will have this +feature disabled by default, enhancing security measures. + +Keep in mind that CORS pre-flight requests are also not executed for GET requests, potentially presenting a +security risk. However, GraphQL query operations usually do not alter data, and mutations are refused. +Additionally, the response is not expected to be readable in the browser (unless CORS checks are successful), +which helps alleviate this concern. + ### Excessive `OperationCanceledException`s When hosting a WebSockets endpoint, it may be common for clients to simply disconnect rather diff --git a/src/GraphQL.AspNetCore3/GraphQLHttpMiddleware.cs b/src/GraphQL.AspNetCore3/GraphQLHttpMiddleware.cs index 90d1642..cf79386 100644 --- a/src/GraphQL.AspNetCore3/GraphQLHttpMiddleware.cs +++ b/src/GraphQL.AspNetCore3/GraphQLHttpMiddleware.cs @@ -223,7 +223,7 @@ public virtual async Task InvokeAsync(HttpContext context) return (await DeserializeFromGraphBodyAsync(httpRequest.Body, sourceEncoding), null); default: - if (httpRequest.HasFormContentType) { + if (httpRequest.HasFormContentType && _options.ReadFormOnPost) { try { var formCollection = await httpRequest.ReadFormAsync(context.RequestAborted); return ReadFormContent(formCollection); @@ -795,7 +795,13 @@ protected virtual Task HandleContentTypeCouldNotBeParsedErrorAsync(HttpContext c /// Writes a '415 Invalid Content-Type header: non-supported media type.' message to the output. /// protected virtual Task HandleInvalidContentTypeErrorAsync(HttpContext context, RequestDelegate next) - => WriteErrorResponseAsync(context, HttpStatusCode.UnsupportedMediaType, new InvalidContentTypeError($"non-supported media type '{context.Request.ContentType}'. Must be '{MEDIATYPE_JSON}', '{MEDIATYPE_GRAPHQL}' or a form body.")); + => WriteErrorResponseAsync( + context, + HttpStatusCode.UnsupportedMediaType, + _options.ReadFormOnPost + ? new InvalidContentTypeError($"non-supported media type '{context.Request.ContentType}'. Must be '{MEDIATYPE_JSON}', '{MEDIATYPE_GRAPHQL}' or a form body.") + : new InvalidContentTypeError($"non-supported media type '{context.Request.ContentType}'. Must be '{MEDIATYPE_JSON}' or '{MEDIATYPE_GRAPHQL}'.") + ); /// /// Indicates that an unsupported HTTP method was requested. diff --git a/src/GraphQL.AspNetCore3/GraphQLHttpMiddlewareOptions.cs b/src/GraphQL.AspNetCore3/GraphQLHttpMiddlewareOptions.cs index dc461c0..ab07f17 100644 --- a/src/GraphQL.AspNetCore3/GraphQLHttpMiddlewareOptions.cs +++ b/src/GraphQL.AspNetCore3/GraphQLHttpMiddlewareOptions.cs @@ -48,6 +48,19 @@ public class GraphQLHttpMiddlewareOptions : IAuthorizationOptions /// public bool ReadQueryStringOnPost { get; set; } = true; + /// + /// Enables parsing POST requests with the form content types such as multipart-form/data. + /// + /// + /// There is a potential security vulnerability when employing cookie authentication + /// with the multipart-form/data content type because sending cookies + /// alongside the request does not initiate a pre-flight CORS request. + /// As a result, GraphQL.NET carries out the request and potentially modifies data, + /// even if the CORS policy forbids it, irrespective of the sender's ability to access + /// the response. + /// + public bool ReadFormOnPost { get; set; } = true; + /// /// Enables reading variables from the query string. /// Variables are interpreted as JSON and deserialized before being diff --git a/src/Tests.ApiApprovals/GraphQL.AspNetCore3.approved.txt b/src/Tests.ApiApprovals/GraphQL.AspNetCore3.approved.txt index 68eee28..d009bee 100644 --- a/src/Tests.ApiApprovals/GraphQL.AspNetCore3.approved.txt +++ b/src/Tests.ApiApprovals/GraphQL.AspNetCore3.approved.txt @@ -166,6 +166,7 @@ namespace GraphQL.AspNetCore3 public int? MaximumFileCount { get; set; } public long? MaximumFileSize { get; set; } public bool ReadExtensionsFromQueryString { get; set; } + public bool ReadFormOnPost { get; set; } public bool ReadQueryStringOnPost { get; set; } public bool ReadVariablesFromQueryString { get; set; } public bool ValidationErrorsReturnBadRequest { get; set; } diff --git a/src/Tests/Middleware/PostTests.cs b/src/Tests/Middleware/PostTests.cs index 8c22307..d19fa8f 100644 --- a/src/Tests/Middleware/PostTests.cs +++ b/src/Tests/Middleware/PostTests.cs @@ -423,18 +423,31 @@ public async Task ContentType_GraphQLJson(string contentType) } [Theory] - [InlineData(false)] - [InlineData(true)] - public async Task UnknownContentType(bool badRequest) + [InlineData(false, true, "application/pdf")] + [InlineData(true, true, "application/pdf")] + [InlineData(false, false, "multipart-form/data")] + [InlineData(true, false, "multipart-form/data")] + [InlineData(false, false, "application/x-www-form-urlencoded")] + [InlineData(true, false, "application/x-www-form-urlencoded")] + public async Task UnknownContentType(bool badRequest, bool allowFormBody, string contentType) { _options.ValidationErrorsReturnBadRequest = badRequest; + _options.ReadFormOnPost = allowFormBody; var client = _server.CreateClient(); - var content = new StringContent("{count}"); - content.Headers.ContentType = new("application/pdf"); + HttpContent content = contentType switch { + "application/pdf" => new StringContent("{count}", null, contentType), + "multipart-form/data" => new MultipartFormDataContent { { new StringContent("{count}", null, "application/graphql"), "query" } }, + "application/x-www-form-urlencoded" => new FormUrlEncodedContent(new[] { new KeyValuePair("query", "{count}") }), + _ => throw new ArgumentOutOfRangeException(nameof(contentType)) + }; using var response = await client.PostAsync("/graphql", content); response.StatusCode.ShouldBe(HttpStatusCode.UnsupportedMediaType); var actual = await response.Content.ReadAsStringAsync(); - actual.ShouldBe(@"{""errors"":[{""message"":""Invalid \u0027Content-Type\u0027 header: non-supported media type \u0027application/pdf\u0027. Must be \u0027application/json\u0027, \u0027application/graphql\u0027 or a form body."",""extensions"":{""code"":""INVALID_CONTENT_TYPE"",""codes"":[""INVALID_CONTENT_TYPE""]}}]}"); + if (allowFormBody) { + actual.ShouldBe($@"{{""errors"":[{{""message"":""Invalid \u0027Content-Type\u0027 header: non-supported media type \u0027{content.Headers.ContentType?.ToString().Replace("\"", "\\u0022")}\u0027. Must be \u0027application/json\u0027, \u0027application/graphql\u0027 or a form body."",""extensions"":{{""code"":""INVALID_CONTENT_TYPE"",""codes"":[""INVALID_CONTENT_TYPE""]}}}}]}}"); + } else { + actual.ShouldBe($@"{{""errors"":[{{""message"":""Invalid \u0027Content-Type\u0027 header: non-supported media type \u0027{content.Headers.ContentType?.ToString().Replace("\"", "\\u0022")}\u0027. Must be \u0027application/json\u0027 or \u0027application/graphql\u0027."",""extensions"":{{""code"":""INVALID_CONTENT_TYPE"",""codes"":[""INVALID_CONTENT_TYPE""]}}}}]}}"); + } } [Theory]