Add RequestSizeLimitAttribute #6453

jbagga · 2017-06-27T19:05:57Z

Addresses #6352

Tratcher · 2017-06-27T19:12:41Z

src/Microsoft.AspNetCore.Mvc.Core/RequestSizeLimitAttribute.cs

+        /// </summary>
+        /// <param name="bytes">The maximum request body size accepted by the application.</param>
+        /// <param name="disabled">Indicates whether this attribute is disabled.</param>
+        public RequestSizeLimitAttribute(long bytes, bool disabled)


This always requires both parameters, correct? I don't think you ever want to set both parameters for this attribute. Get rid of the constructor and only use Property setters?

e.g. [RequestSizeLimit(Bytes=10)], [RequestSizeLimit(Disabled=true)]. Not sure what happens for [RequestSizeLimit] though, I assume it would default to 0, false, which would be weird.

If the limit is not specified, [RequestSizeLimit], is it reasonable to set Disabled to true?

Sorry, what does Disabled do?

This sounds like a totally different attribute

@rynowak We discussed it here but now we just need one parameter so I think ctor can be used. @Tratcher?

rynowak · 2017-06-27T21:09:38Z

src/Microsoft.AspNetCore.Mvc.Core/RequestSizeLimitAttribute.cs

+
+            if (Disabled == false)
+            {
+                var requestLength = context.HttpContext.Request.Body.Length;


We should be setting the value on https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.AspNetCore.Http.Features/IHttpMaxRequestBodySizeFeature.cs

This isn't for us to implement

rynowak · 2017-06-27T21:15:26Z

src/Microsoft.AspNetCore.Mvc.Core/RequestSizeLimitAttribute.cs

+        /// the request body size is greater than the specified limit.
+        /// </summary>
+        /// <param name="context">The <see cref="ResourceExecutingContext"/>.</param>
+        public void OnResourceExecuting(ResourceExecutingContext context)


This should support scoping, meaning that the filter closest to the action is the one to act.

Look at:
https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/IAntiforgeryPolicy.cs
https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ValidateAntiforgeryTokenAuthorizationFilter.cs

The general pattern here is:

Define a policy interface (IRequestSizePolicy)

Define filter attributes that implement that policy interface (RequestSizeLimitAttribute, DisableRequestSizeLimitAttribute).

Each of these filter attributes checks if it's the last IRequestSizePolicy in context.Filters and skip itself if it isn't.

Example: https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNetCore.Mvc.ViewFeatures/Internal/ValidateAntiforgeryTokenAuthorizationFilter.cs#L61

Should there be two different classes, one for the filter and one for the attribute like your example? https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ValidateAntiForgeryTokenAttribute.cs.

I was following the pattern of ConsumesAttribute which implements Attribute and IResourceFilter.

I think there should be 3.

The two attributes, and then the policy interface.

When we have a separation between filter and attribute - it's usually because DI is involved

rynowak · 2017-06-27T21:16:32Z

test/Microsoft.AspNetCore.Mvc.FunctionalTests/FiltersTest.cs

@@ -554,5 +554,53 @@ public FiltersTest(MvcTestFixture<FiltersWebSite.Startup> fixture)
                $"CurrentCulture:{expected},CurrentUICulture:{expected}",
                await response.Content.ReadAsStringAsync());
        }
+
+        [Fact]
+        public async Task RequestSizeLimit_ThrowsForInvalidSize()


Please make a new class for these tests and don't put them in FiltersWebSite. That thing is a mess and we plan to just nuke it from orbit some day.

If all we do is set a feature, do we need functional tests for this? Wouldn't unit tests suffice?

It actually might be the case that we have to do unit tests. The test server may not implement this.

Yeah I tried and was unable to figure out how to initialize the feature for the functional tests. Updating the PR with just unit tests.

Do you have tests that use kestrel/httpsys? or just TestServer?

We only use testserver

We need to find a way to test this with a real server.

jbagga · 2017-06-28T00:57:18Z

@Tratcher @rynowak @pranavkm Updated

Tratcher · 2017-06-28T01:21:45Z

src/Microsoft.AspNetCore.Mvc.Core/DisableRequestSizeLimitAttribute.cs

+        public bool IsReusable => true;
+
+        /// <inheritdoc />
+        public int Order { get; set; } = 1000;


This seems high. This must apply before model binding.

Resource filters are always before model binding.

@jbagga - leave this at 0 (unset) unless you have a good reason to do otherwise.

Tratcher · 2017-06-28T01:22:25Z

src/Microsoft.AspNetCore.Mvc.Core/DisableRequestSizeLimitAttribute.cs

+            if (IsClosestRequestSizePolicy(context.Filters))
+            {
+                var maxRequestBodySizeFeature = context.HttpContext.Features.Get<IHttpMaxRequestBodySizeFeature>();
+                maxRequestBodySizeFeature.MaxRequestBodySize = null;


Check for null feature

You also need to check IsReadOnly and not try to change the value. Possibly log that the setting could not be applied. This would happen if the request body had already started reading, such as being buffered.
It would also make sense to log if the feature was missing and the attribute couldn't be applied.

Tratcher · 2017-06-28T01:23:04Z

src/Microsoft.AspNetCore.Mvc.Core/DisableRequestSizeLimitAttribute.cs

+
+        private bool IsClosestRequestSizePolicy(IList<IFilterMetadata> filters)
+        {
+            // Determine if this instance is the 'effective' antiforgery policy.


antiforgery?

yeah update this 👍

rynowak · 2017-06-28T01:41:01Z

src/Microsoft.AspNetCore.Mvc.Core/DisableRequestSizeLimitAttribute.cs

+using Microsoft.AspNetCore.Http.Features;
+using Microsoft.AspNetCore.Mvc.Filters;
+
+namespace Microsoft.AspNetCore.Mvc.Core


Microsoft.AspNetCore.Mvc

The general rule of thumb is that anything I user will have to type in a controller belongs in Microsoft.AspNetCore.Mvc

rynowak · 2017-06-28T01:41:26Z

src/Microsoft.AspNetCore.Mvc.Core/DisableRequestSizeLimitAttribute.cs

+    public class DisableRequestSizeLimitAttribute : Attribute, IRequestSizePolicy, IOrderedFilter, IResourceFilter
+    {
+        /// <inheritdoc />
+        public bool IsReusable => true;


Remove this, it's not needed

rynowak · 2017-06-28T01:41:31Z

src/Microsoft.AspNetCore.Mvc.Core/DisableRequestSizeLimitAttribute.cs

+
+namespace Microsoft.AspNetCore.Mvc.Core
+{
+    public class DisableRequestSizeLimitAttribute : Attribute, IRequestSizePolicy, IOrderedFilter, IResourceFilter


rynowak · 2017-06-28T01:42:20Z

src/Microsoft.AspNetCore.Mvc.Core/IRequestSizePolicy.cs

+    /// <summary>
+    /// A marker interface for filters which define a policy for maximum size for the request body.
+    /// </summary>
+    public interface IRequestSizePolicy : IFilterMetadata


rynowak · 2017-06-28T01:43:35Z

src/Microsoft.AspNetCore.Mvc.Core/IRequestSizePolicy.cs

+
+using Microsoft.AspNetCore.Mvc.Filters;
+
+namespace Microsoft.AspNetCore.Mvc.Core


This should probably be .Mvc as well

rynowak · 2017-06-28T01:43:43Z

src/Microsoft.AspNetCore.Mvc.Core/RequestSizeLimitAttribute.cs

+using Microsoft.AspNetCore.Http.Features;
+using Microsoft.AspNetCore.Mvc.Filters;
+
+namespace Microsoft.AspNetCore.Mvc.Core


rynowak · 2017-06-28T01:44:03Z

src/Microsoft.AspNetCore.Mvc.Core/RequestSizeLimitAttribute.cs

+{
+    /// <summary>
+    /// A filter that specifies the request body size limit.
+    /// </summary>


It might help here to put some links to in the docs to the http feature

The feature interfaces aren't really meant for direct user consumption. putting it in the docs here would be more confusing than the current description.

rynowak · 2017-06-28T01:44:19Z

src/Microsoft.AspNetCore.Mvc.Core/RequestSizeLimitAttribute.cs

+    [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)]
+    public class RequestSizeLimitAttribute : Attribute, IRequestSizePolicy, IOrderedFilter, IResourceFilter
+    {
+        private long _bytes;


rynowak · 2017-06-28T01:44:27Z

src/Microsoft.AspNetCore.Mvc.Core/RequestSizeLimitAttribute.cs

+        }
+
+        /// <inheritdoc />
+        public bool IsReusable => true;


rynowak · 2017-06-28T01:44:32Z

src/Microsoft.AspNetCore.Mvc.Core/RequestSizeLimitAttribute.cs

+        public bool IsReusable => true;
+
+        /// <inheritdoc />
+        public int Order { get; set; } = 1000;


rynowak · 2017-06-28T01:46:21Z

src/Microsoft.AspNetCore.Mvc.Core/RequestSizeLimitAttribute.cs

+            if (IsClosestRequestSizePolicy(context.Filters))
+            {
+                var maxRequestBodySizeFeature = context.HttpContext.Features.Get<IHttpMaxRequestBodySizeFeature>();
+                maxRequestBodySizeFeature.MaxRequestBodySize = _bytes;


Same comments apply here, we need to check for null.

@Tratcher - any ideas on what a reasonable behavior is when a server doesn't support this feature?

I could see throwing, but the problem will be that we're going to encourage users to use things like TestServer.

We might want to document that this will no op when the server doesn't support it

rynowak · 2017-06-28T01:46:52Z

test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ResponseCacheFilterTest.cs

@@ -3,7 +3,6 @@

 using System;
 using System.Collections.Generic;
-using System.Linq;


rynowak · 2017-06-28T01:47:12Z

test/Microsoft.AspNetCore.Mvc.Core.Test/RequestSizeLimitAttributeTests.cs

+using Microsoft.AspNetCore.Routing;
+using Xunit;
+
+namespace Microsoft.AspNetCore.Mvc.Core


update the namespace of these tests as well

rynowak · 2017-06-28T01:53:01Z

test/Microsoft.AspNetCore.Mvc.Core.Test/RequestSizeLimitAttributeTests.cs

+
+namespace Microsoft.AspNetCore.Mvc.Core
+{
+    public class RequestSizeLimitAttributeTests


Test not Tests ( yeah, I went there. 😲

rynowak · 2017-06-28T01:53:41Z

test/WebSites/FiltersWebSite/Controllers/ResourceFilterController.cs

 using System.Linq;
 using Microsoft.AspNetCore.Mvc;
+using Microsoft.AspNetCore.Mvc.Core;


rynowak

Design of this is looking overall right. 👍

halter73 · 2017-06-28T02:04:22Z

src/Microsoft.AspNetCore.Mvc.Core/RequestSizeLimitAttribute.cs

+                throw new ArgumentNullException(nameof(context));
+            }
+
+            if (IsClosestRequestSizePolicy(context.Filters))


It's pretty inexpensive to set IHttpMaxRequestBodySizeFeature.MaxRequestBodySize on both of our servers. Wouldn't it be simpler to just greedily set the property instead of checking IsClosestRequestSizePolicy each time?

This is assuming that the "closest" filter has its OnResourceExecuting method called last which I'm unsure about.

They use this pattern elsewhere. it's better not to run the same functionality twice, especially if it starts logging.

It's better to just run a single policy. These things tend to sprout more features over time so the clean semantics are important

halter73 · 2017-06-28T02:05:30Z

src/Microsoft.AspNetCore.Mvc.Core/DisableRequestSizeLimitAttribute.cs

+
+        /// <summary>
+        /// As an <see cref="IResourceFilter"/>, this filter looks at the request and rejects it before going ahead if
+        /// the request body size is greater than the specified limit.


Nit: there is no specified limit in this case.

Don't worry about my IsClosestRequestSizePolicy comment

rynowak · 2017-06-29T03:02:10Z

test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/RequestSizeLimitResourceFilterTest.cs

+        }
+
+        [Fact]
+        public void LogsFeatureNotFound()


We haven't normally written unit tests for the logging when it's simple like this

Can I remove all logging unit tests?

You've already written them, what's the harm in keeping them? Unless they need to churn do to other API changes.

dougbu · 2017-06-29T03:07:00Z

test/WebSites/FiltersWebSite/Controllers/MiddlewareFilterTestController.cs

 using Microsoft.AspNetCore.Mvc;
-using Microsoft.AspNetCore.Mvc.Filters;


@rynowak is being stingy with 😺 and this is the money stop‼️

jbagga · 2017-06-29T15:49:42Z

@rynowak @dougbu Updated

rynowak · 2017-06-29T16:57:07Z

src/Microsoft.AspNetCore.Mvc.Core/RequestSizeLimitAttribute.cs

+        /// <summary>
+        /// The request body size limit.
+        /// </summary>
+        public long Bytes { get; set; }


Did y'all discuss making this a constructor parameter vs property earlier? This seems odd to me as a property because [RequestSizeLimit] will set it to 0.

Yes, now that there's a single parameter then making it part of the constructor would be fine.

OK please make that change. Other than that this looks good to me.

Have you tested this out manually (given that we can't functional test it)?

Yes I did some sandbox testing. Updated the PR with the constructor

jbagga · 2017-06-29T18:51:33Z

@rynowak @dougbu Updated

dougbu

I'll just look at added documentation next time through

dougbu · 2017-06-29T18:48:22Z

test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DisableRequestSizeLimitResourceFilterTest.cs

+        }
+
+        [Fact]
+        public void DisableRequestSizeLimitAttribute_LogsMaxRequestBodySizeSetToNull()


Has nothing to do w/ DisableRequestSizeLimitAttribute; just call this LogsMaxRequestBodySizeSetToNull

dougbu · 2017-06-29T18:48:54Z

test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/RequestSizeLimitResourceFilterTest.cs

+        }
+
+        [Fact]
+        public void RequestSizeLimitAttribute_LogsMaxRequestBodySizeSet()


Has nothing to do w/ RequestSizeLimitAttribute; just call this LogsMaxRequestBodySizeSet

dougbu · 2017-06-29T18:50:46Z

src/Microsoft.AspNetCore.Mvc.Core/RequestSizeLimitAttribute.cs

+    {
+        private readonly long _bytes;
+
+        public RequestSizeLimitAttribute(long bytes)


+ doc comments (e.g. parameter description similar to what you had for the Bytes property)

dougbu · 2017-06-29T18:56:49Z

src/Microsoft.AspNetCore.Mvc.Core/DisableRequestSizeLimitAttribute.cs

+namespace Microsoft.AspNetCore.Mvc
+{
+    /// <summary>
+    /// Sets the <see cref="IHttpMaxRequestBodySizeFeature.MaxRequestBodySize"/> to <c>null</c>.


I hadn't seen @Tratcher's comment on the feature when I made my earlier common on the inconsistent <see/> versus descriptive text. Best to consistently describe what's going on here in the public classes. But, leave what you've got in the .Internal classes alone.

The feature interfaces aren't really meant for direct user consumption. putting it in the docs here would be more confusing than the current description.

jbagga · 2017-06-29T19:07:03Z

@dougbu @rynowak Updated

rynowak · 2017-06-29T19:20:46Z

Looks great!

Tratcher · 2017-06-29T19:24:54Z

src/Microsoft.AspNetCore.Mvc.Core/DisableRequestSizeLimitAttribute.cs

+namespace Microsoft.AspNetCore.Mvc
+{
+    /// <summary>
+    /// Sets the request body size limit to <c>null</c>.


Weren't you going to change this to say it disabled the size limit?

jbagga requested review from rynowak and Tratcher June 27, 2017 19:05

dnfclas added the cla-already-signed label Jun 27, 2017

Tratcher reviewed Jun 27, 2017

View reviewed changes

rynowak reviewed Jun 27, 2017

View reviewed changes

Tratcher reviewed Jun 28, 2017

View reviewed changes

rynowak reviewed Jun 28, 2017

View reviewed changes

rynowak suggested changes Jun 28, 2017

View reviewed changes

halter73 previously requested changes Jun 28, 2017

View reviewed changes

halter73 reviewed Jun 28, 2017

View reviewed changes

rynowak reviewed Jun 29, 2017

View reviewed changes

dougbu reviewed Jun 29, 2017

View reviewed changes

jbagga force-pushed the jbagga/RequestSizeLimit6352 branch from 0340a6c to e56b4e0 Compare June 29, 2017 15:45

rynowak reviewed Jun 29, 2017

View reviewed changes

dougbu suggested changes Jun 29, 2017

View reviewed changes

dougbu reviewed Jun 29, 2017

View reviewed changes

dougbu approved these changes Jun 29, 2017

View reviewed changes

rynowak approved these changes Jun 29, 2017

View reviewed changes

Tratcher reviewed Jun 29, 2017

View reviewed changes

jbagga added 12 commits June 29, 2017 12:26

[Design] Add RequestSizeLimitAttribute

04e022e

Change design

08703ad

Remove functional tests

4e0ce7c

Feedback addressed

575e17b

Add filters

78c8d1c

Move filters to internal ns

1d9a41a

Add tests; modify log messages

4c3297d

modify doc

6bfbcb0

Add ctor

dee20a3

Add doc to ctor

1a019c2

Fix docs

2b5bfa5

Add period

f3e413d

Tratcher approved these changes Jun 29, 2017

View reviewed changes

Change doc

d977ad4

jbagga force-pushed the jbagga/RequestSizeLimit6352 branch from 2225bcd to d977ad4 Compare June 29, 2017 19:38

jbagga merged commit 17f6b17 into dev Jun 29, 2017

jbagga deleted the jbagga/RequestSizeLimit6352 branch June 29, 2017 20:04


		using Microsoft.AspNetCore.Mvc.Filters;

		namespace Microsoft.AspNetCore.Mvc.Core

		using Microsoft.AspNetCore.Mvc;
		using Microsoft.AspNetCore.Mvc.Filters;

Add RequestSizeLimitAttribute #6453

Add RequestSizeLimitAttribute #6453

Conversation

jbagga commented Jun 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbagga commented Jun 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynowak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbagga commented Jun 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbagga commented Jun 29, 2017 • edited Loading

dougbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbagga commented Jun 29, 2017

rynowak commented Jun 29, 2017

Choose a reason for hiding this comment

jbagga commented Jun 29, 2017 •

edited

Loading