Skip to content

Commit 78a587b

Browse files
pranavkmpranavkm
authored
Guard against client disconnect exceptions that appear when reading body (#25146)
* Guard against client disconnect exceptions that appear when performing ReadFormAsync Reading the request body may throw an exception. This change adds some extra guards for this and presents this as a 4xx response rather than a 5xx response. * Add some tests * Fixup test
1 parent a9b596e commit 78a587b

14 files changed

+493
-19
lines changed

‎src/Antiforgery/src/Internal/DefaultAntiforgeryTokenStore.cs

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
using System;
55
using System.Diagnostics;
6+
using System.IO;
67
using System.Threading.Tasks;
78
using Microsoft.AspNetCore.Http;
89
using Microsoft.Extensions.Options;
@@ -57,7 +58,24 @@ public async Task<AntiforgeryTokenSet> GetRequestTokensAsync(HttpContext httpCon
5758
{
5859
// Check the content-type before accessing the form collection to make sure
5960
// we report errors gracefully.
60-
var form = await httpContext.Request.ReadFormAsync();
61+
IFormCollection form;
62+
try
63+
{
64+
form = await httpContext.Request.ReadFormAsync();
65+
}
66+
catch (InvalidDataException ex)
67+
{
68+
// ReadFormAsync can throw InvalidDataException if the form content is malformed.
69+
// Wrap it in an AntiforgeryValidationException and allow the caller to handle it as just another antiforgery failure.
70+
throw new AntiforgeryValidationException(Resources.AntiforgeryToken_UnableToReadRequest, ex);
71+
}
72+
catch (IOException ex)
73+
{
74+
// Reading the request body (which happens as part of ReadFromAsync) may throw an exception if a client disconnects.
75+
// Wrap it in an AntiforgeryValidationException and allow the caller to handle it as just another antiforgery failure.
76+
throw new AntiforgeryValidationException(Resources.AntiforgeryToken_UnableToReadRequest, ex);
77+
}
78+
6179
requestToken = form[_options.FormFieldName];
6280
}
6381

‎src/Antiforgery/src/Resources.resx

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,9 @@
136136
<data name="AntiforgeryToken_TokensSwapped" xml:space="preserve">
137137
<value>Validation of the provided antiforgery token failed. The cookie token and the request token were swapped.</value>
138138
</data>
139+
<data name="AntiforgeryToken_UnableToReadRequest" xml:space="preserve">
140+
<value>Unable to read the antiforgery request token from the posted form.</value>
141+
</data>
139142
<data name="AntiforgeryToken_UsernameMismatch" xml:space="preserve">
140143
<value>The provided antiforgery token was meant for user "{0}", but the current user is "{1}".</value>
141144
</data>

‎src/Antiforgery/test/DefaultAntiforgeryTokenStoreTest.cs

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33

44
using System;
55
using System.Collections.Generic;
6+
using System.IO;
7+
using System.Threading;
68
using System.Threading.Tasks;
79
using Microsoft.AspNetCore.Http;
810
using Microsoft.Extensions.Primitives;
@@ -235,6 +237,56 @@ public async Task GetRequestTokens_BothHeaderValueAndFormFieldsEmpty_ReturnsNull
235237
Assert.Null(tokenSet.RequestToken);
236238
}
237239

240+
[Fact]
241+
public async Task GetRequestTokens_ReadFormAsyncThrowsIOException_ThrowsAntiforgeryValidationException()
242+
{
243+
// Arrange
244+
var ioException = new IOException();
245+
var httpContext = new Mock<HttpContext>();
246+
247+
httpContext.Setup(r => r.Request.Cookies).Returns(Mock.Of<IRequestCookieCollection>());
248+
httpContext.SetupGet(r => r.Request.HasFormContentType).Returns(true);
249+
httpContext.Setup(r => r.Request.ReadFormAsync(It.IsAny<CancellationToken>())).Throws(ioException);
250+
251+
var options = new AntiforgeryOptions
252+
{
253+
Cookie = { Name = "cookie-name" },
254+
FormFieldName = "form-field-name",
255+
HeaderName = null,
256+
};
257+
258+
var tokenStore = new DefaultAntiforgeryTokenStore(new TestOptionsManager(options));
259+
260+
// Act & Assert
261+
var ex = await Assert.ThrowsAsync<AntiforgeryValidationException>(() => tokenStore.GetRequestTokensAsync(httpContext.Object));
262+
Assert.Same(ioException, ex.InnerException);
263+
}
264+
265+
[Fact]
266+
public async Task GetRequestTokens_ReadFormAsyncThrowsInvalidDataException_ThrowsAntiforgeryValidationException()
267+
{
268+
// Arrange
269+
var exception = new InvalidDataException();
270+
var httpContext = new Mock<HttpContext>();
271+
272+
httpContext.Setup(r => r.Request.Cookies).Returns(Mock.Of<IRequestCookieCollection>());
273+
httpContext.SetupGet(r => r.Request.HasFormContentType).Returns(true);
274+
httpContext.Setup(r => r.Request.ReadFormAsync(It.IsAny<CancellationToken>())).Throws(exception);
275+
276+
var options = new AntiforgeryOptions
277+
{
278+
Cookie = { Name = "cookie-name" },
279+
FormFieldName = "form-field-name",
280+
HeaderName = null,
281+
};
282+
283+
var tokenStore = new DefaultAntiforgeryTokenStore(new TestOptionsManager(options));
284+
285+
// Act & Assert
286+
var ex = await Assert.ThrowsAsync<AntiforgeryValidationException>(() => tokenStore.GetRequestTokensAsync(httpContext.Object));
287+
Assert.Same(exception, ex.InnerException);
288+
}
289+
238290
[Theory]
239291
[InlineData(false, CookieSecurePolicy.SameAsRequest, null)]
240292
[InlineData(true, CookieSecurePolicy.SameAsRequest, true)]

‎src/Mvc/Mvc.Core/src/ModelBinding/FormFileValueProviderFactory.cs

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Copyright (c) .NET Foundation. All rights reserved.
1+
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

44
using System;
@@ -42,6 +42,14 @@ private static async Task AddValueProviderAsync(ValueProviderFactoryContext cont
4242
}
4343
catch (InvalidDataException ex)
4444
{
45+
// ReadFormAsync can throw InvalidDataException if the form content is malformed.
46+
// Wrap it in a ValueProviderException that the CompositeValueProvider special cases.
47+
throw new ValueProviderException(Resources.FormatFailedToReadRequestForm(ex.Message), ex);
48+
}
49+
catch (IOException ex)
50+
{
51+
// ReadFormAsync can throw IOException if the client disconnects.
52+
// Wrap it in a ValueProviderException that the CompositeValueProvider special cases.
4553
throw new ValueProviderException(Resources.FormatFailedToReadRequestForm(ex.Message), ex);
4654
}
4755

‎src/Mvc/Mvc.Core/src/ModelBinding/FormValueProviderFactory.cs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,14 @@ private static async Task AddValueProviderAsync(ValueProviderFactoryContext cont
4444
}
4545
catch (InvalidDataException ex)
4646
{
47+
// ReadFormAsync can throw InvalidDataException if the form content is malformed.
48+
// Wrap it in a ValueProviderException that the CompositeValueProvider special cases.
49+
throw new ValueProviderException(Resources.FormatFailedToReadRequestForm(ex.Message), ex);
50+
}
51+
catch (IOException ex)
52+
{
53+
// ReadFormAsync can throw IOException if the client disconnects.
54+
// Wrap it in a ValueProviderException that the CompositeValueProvider special cases.
4755
throw new ValueProviderException(Resources.FormatFailedToReadRequestForm(ex.Message), ex);
4856
}
4957

‎src/Mvc/Mvc.Core/src/ModelBinding/JQueryFormValueProviderFactory.cs

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,10 @@
33

44
using System;
55
using System.Globalization;
6+
using System.IO;
67
using System.Threading.Tasks;
8+
using Microsoft.AspNetCore.Http;
9+
using Microsoft.AspNetCore.Mvc.Core;
710

811
namespace Microsoft.AspNetCore.Mvc.ModelBinding
912
{
@@ -34,7 +37,23 @@ private static async Task AddValueProviderAsync(ValueProviderFactoryContext cont
3437
{
3538
var request = context.ActionContext.HttpContext.Request;
3639

37-
var formCollection = await request.ReadFormAsync();
40+
IFormCollection formCollection;
41+
try
42+
{
43+
formCollection = await request.ReadFormAsync();
44+
}
45+
catch (InvalidDataException ex)
46+
{
47+
// ReadFormAsync can throw InvalidDataException if the form content is malformed.
48+
// Wrap it in a ValueProviderException that the CompositeValueProvider special cases.
49+
throw new ValueProviderException(Resources.FormatFailedToReadRequestForm(ex.Message), ex);
50+
}
51+
catch (IOException ex)
52+
{
53+
// ReadFormAsync can throw IOException if the client disconnects.
54+
// Wrap it in a ValueProviderException that the CompositeValueProvider special cases.
55+
throw new ValueProviderException(Resources.FormatFailedToReadRequestForm(ex.Message), ex);
56+
}
3857

3958
var valueProvider = new JQueryFormValueProvider(
4059
BindingSource.Form,

‎src/Mvc/Mvc.Core/test/ModelBinding/CompositeValueProviderTest.cs

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,10 @@
55
using System.Collections.Generic;
66
using System.Globalization;
77
using System.Linq;
8+
using System.Threading.Tasks;
89
using Microsoft.AspNetCore.Http;
10+
using Microsoft.AspNetCore.Mvc.Abstractions;
11+
using Microsoft.AspNetCore.Routing;
912
using Microsoft.Extensions.Primitives;
1013
using Moq;
1114
using Xunit;
@@ -45,6 +48,25 @@ protected override IEnumerableValueProvider GetEnumerableValueProvider(
4548
return new CompositeValueProvider() { emptyValueProvider, valueProvider };
4649
}
4750

51+
[Fact]
52+
public async Task TryCreateAsync_AddsModelStateError_WhenValueProviderFactoryThrowsValueProviderException()
53+
{
54+
// Arrange
55+
var factory = new Mock<IValueProviderFactory>();
56+
factory.Setup(f => f.CreateValueProviderAsync(It.IsAny<ValueProviderFactoryContext>())).ThrowsAsync(new ValueProviderException("Some error"));
57+
var actionContext = new ActionContext(new DefaultHttpContext(), new RouteData(), new ActionDescriptor(), new ModelStateDictionary());
58+
59+
// Act
60+
var (success, result) = await CompositeValueProvider.TryCreateAsync(actionContext, new[] { factory.Object });
61+
62+
// Assert
63+
Assert.False(success);
64+
var modelState = actionContext.ModelState;
65+
Assert.False(modelState.IsValid);
66+
var entry = Assert.Single(modelState);
67+
Assert.Empty(entry.Key);
68+
}
69+
4870
[Fact]
4971
public void GetKeysFromPrefixAsync_ReturnsResultFromFirstValueProviderThatReturnsValues()
5072
{

‎src/Mvc/Mvc.Core/test/ModelBinding/FormFileValueProviderFactoryTest.cs

Lines changed: 57 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,16 @@
1-
// Copyright (c) .NET Foundation. All rights reserved.
1+
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

4+
using System;
45
using System.Collections.Generic;
56
using System.IO;
7+
using System.Threading;
68
using System.Threading.Tasks;
79
using Microsoft.AspNetCore.Http;
810
using Microsoft.AspNetCore.Mvc.Abstractions;
911
using Microsoft.AspNetCore.Routing;
1012
using Microsoft.Extensions.Primitives;
13+
using Moq;
1114
using Xunit;
1215

1316
namespace Microsoft.AspNetCore.Mvc.ModelBinding
@@ -60,6 +63,59 @@ public async Task CreateValueProviderAsync_AddsValueProvider()
6063
v => Assert.IsType<FormFileValueProvider>(v));
6164
}
6265

66+
[Fact]
67+
public async Task GetValueProviderAsync_ThrowsValueProviderException_IfReadingFormThrowsInvalidDataException()
68+
{
69+
// Arrange
70+
var exception = new InvalidDataException();
71+
var valueProviderContext = CreateThrowingContext(exception);
72+
73+
var factory = new FormFileValueProviderFactory();
74+
75+
// Act & Assert
76+
var ex = await Assert.ThrowsAsync<ValueProviderException>(() => factory.CreateValueProviderAsync(valueProviderContext));
77+
Assert.Same(exception, ex.InnerException);
78+
}
79+
80+
[Fact]
81+
public async Task GetValueProviderAsync_ThrowsValueProviderException_IfReadingFormThrowsInvalidOperationException()
82+
{
83+
// Arrange
84+
var exception = new IOException();
85+
var valueProviderContext = CreateThrowingContext(exception);
86+
87+
var factory = new FormFileValueProviderFactory();
88+
89+
// Act & Assert
90+
var ex = await Assert.ThrowsAsync<ValueProviderException>(() => factory.CreateValueProviderAsync(valueProviderContext));
91+
Assert.Same(exception, ex.InnerException);
92+
}
93+
94+
[Fact]
95+
public async Task GetValueProviderAsync_ThrowsOriginalException_IfReadingFormThrows()
96+
{
97+
// Arrange
98+
var exception = new TimeZoneNotFoundException();
99+
var valueProviderContext = CreateThrowingContext(exception);
100+
101+
var factory = new FormFileValueProviderFactory();
102+
103+
// Act & Assert
104+
var ex = await Assert.ThrowsAsync<TimeZoneNotFoundException>(() => factory.CreateValueProviderAsync(valueProviderContext));
105+
Assert.Same(exception, ex);
106+
}
107+
108+
private static ValueProviderFactoryContext CreateThrowingContext(Exception exception)
109+
{
110+
var context = new Mock<HttpContext>();
111+
context.Setup(c => c.Request.ContentType).Returns("application/x-www-form-urlencoded");
112+
context.Setup(c => c.Request.HasFormContentType).Returns(true);
113+
context.Setup(c => c.Request.ReadFormAsync(It.IsAny<CancellationToken>())).ThrowsAsync(exception);
114+
var actionContext = new ActionContext(context.Object, new RouteData(), new ActionDescriptor());
115+
var valueProviderContext = new ValueProviderFactoryContext(actionContext);
116+
return valueProviderContext;
117+
}
118+
63119
private static ValueProviderFactoryContext CreateContext(string contentType)
64120
{
65121
var context = new DefaultHttpContext();

‎src/Mvc/Mvc.Core/test/ModelBinding/FormValueProviderFactoryTest.cs

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,17 @@
11
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

4+
using System;
45
using System.Collections.Generic;
56
using System.Globalization;
7+
using System.IO;
8+
using System.Threading;
69
using System.Threading.Tasks;
710
using Microsoft.AspNetCore.Http;
811
using Microsoft.AspNetCore.Mvc.Abstractions;
912
using Microsoft.AspNetCore.Routing;
1013
using Microsoft.Extensions.Primitives;
14+
using Moq;
1115
using Xunit;
1216

1317
namespace Microsoft.AspNetCore.Mvc.ModelBinding.Test
@@ -47,6 +51,59 @@ public async Task GetValueProviderAsync_ReturnsValueProvider_WithCurrentCulture(
4751
Assert.Equal(CultureInfo.CurrentCulture, valueProvider.Culture);
4852
}
4953

54+
[Fact]
55+
public async Task GetValueProviderAsync_ThrowsValueProviderException_IfReadingFormThrowsInvalidDataException()
56+
{
57+
// Arrange
58+
var exception = new InvalidDataException();
59+
var valueProviderContext = CreateThrowingContext(exception);
60+
61+
var factory = new FormValueProviderFactory();
62+
63+
// Act & Assert
64+
var ex = await Assert.ThrowsAsync<ValueProviderException>(() => factory.CreateValueProviderAsync(valueProviderContext));
65+
Assert.Same(exception, ex.InnerException);
66+
}
67+
68+
[Fact]
69+
public async Task GetValueProviderAsync_ThrowsValueProviderException_IfReadingFormThrowsInvalidOperationException()
70+
{
71+
// Arrange
72+
var exception = new IOException();
73+
var valueProviderContext = CreateThrowingContext(exception);
74+
75+
var factory = new FormValueProviderFactory();
76+
77+
// Act & Assert
78+
var ex = await Assert.ThrowsAsync<ValueProviderException>(() => factory.CreateValueProviderAsync(valueProviderContext));
79+
Assert.Same(exception, ex.InnerException);
80+
}
81+
82+
[Fact]
83+
public async Task GetValueProviderAsync_ThrowsOriginalException_IfReadingFormThrows()
84+
{
85+
// Arrange
86+
var exception = new TimeZoneNotFoundException();
87+
var valueProviderContext = CreateThrowingContext(exception);
88+
89+
var factory = new FormValueProviderFactory();
90+
91+
// Act & Assert
92+
var ex = await Assert.ThrowsAsync<TimeZoneNotFoundException>(() => factory.CreateValueProviderAsync(valueProviderContext));
93+
Assert.Same(exception, ex);
94+
}
95+
96+
private static ValueProviderFactoryContext CreateThrowingContext(Exception exception)
97+
{
98+
var context = new Mock<HttpContext>();
99+
context.Setup(c => c.Request.ContentType).Returns("application/x-www-form-urlencoded");
100+
context.Setup(c => c.Request.HasFormContentType).Returns(true);
101+
context.Setup(c => c.Request.ReadFormAsync(It.IsAny<CancellationToken>())).ThrowsAsync(exception);
102+
var actionContext = new ActionContext(context.Object, new RouteData(), new ActionDescriptor());
103+
var valueProviderContext = new ValueProviderFactoryContext(actionContext);
104+
return valueProviderContext;
105+
}
106+
50107
private static ValueProviderFactoryContext CreateContext(string contentType)
51108
{
52109
var context = new DefaultHttpContext();

0 commit comments

Comments
 (0)