Skip to content

Commit

Permalink
Allow option to disable handling of form data for POST requests (#64)
Browse files Browse the repository at this point in the history
  • Loading branch information
Shane32 authored Aug 19, 2024
1 parent c3cd223 commit c67df30
Show file tree
Hide file tree
Showing 5 changed files with 63 additions and 8 deletions.
22 changes: 22 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down Expand Up @@ -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
Expand Down
10 changes: 8 additions & 2 deletions src/GraphQL.AspNetCore3/GraphQLHttpMiddleware.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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.
/// </summary>
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}'.")
);

/// <summary>
/// Indicates that an unsupported HTTP method was requested.
Expand Down
13 changes: 13 additions & 0 deletions src/GraphQL.AspNetCore3/GraphQLHttpMiddlewareOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,19 @@ public class GraphQLHttpMiddlewareOptions : IAuthorizationOptions
/// </summary>
public bool ReadQueryStringOnPost { get; set; } = true;

/// <summary>
/// Enables parsing POST requests with the form content types such as <c>multipart-form/data</c>.
/// </summary>
/// <remarks>
/// There is a potential security vulnerability when employing cookie authentication
/// with the <c>multipart-form/data</c> 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.
/// </remarks>
public bool ReadFormOnPost { get; set; } = true;

/// <summary>
/// Enables reading variables from the query string.
/// Variables are interpreted as JSON and deserialized before being
Expand Down
1 change: 1 addition & 0 deletions src/Tests.ApiApprovals/GraphQL.AspNetCore3.approved.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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; }
Expand Down
25 changes: 19 additions & 6 deletions src/Tests/Middleware/PostTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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<string?, string?>("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]
Expand Down

0 comments on commit c67df30

Please sign in to comment.