aspnet / Mvc

[Archived] ASP.NET Core MVC is a model view controller framework for building dynamic web sites with clean separation of concerns, including the merged MVC, Web API, and Web Pages w/ Razor. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
5.62k stars 2.14k forks source link

parsing issue on asp.net when request quality factor is specified. #5150

Closed sk29110 closed 7 years ago

sk29110 commented 8 years ago

I am having issue when there is a quality factor specified in the HTTP header like the following.

Accept: text/html, image/gif, image/jpeg, ; q=.9, */; q=.1

and make an get request agent my asp.net API

As this error suggest there is some parsing issues if there is quality factor.

Is this the bug which mvc needs to take care or if there is some setting I need to make to make workaround.

System.ArgumentOutOfRangeException: Specified argument was out of the range of valid values.
Parameter name: length
   at Microsoft.Extensions.Primitives.StringSegment..ctor(String buffer, Int32 offset, Int32 length)
   at Microsoft.AspNetCore.Mvc.Formatters.MediaType.CreateMediaTypeSegmentWithQuality(String mediaType, Int32 start)
   at Microsoft.AspNetCore.Mvc.Formatters.Internal.AcceptHeaderParser.GetMediaTypeWithQualityLength(String input, Int32 start, MediaTypeSegmentWithQuality& result)
   at Microsoft.AspNetCore.Mvc.Formatters.Internal.AcceptHeaderParser.TryParseValue(String value, Int32& index, MediaTypeSegmentWithQuality& parsedValue)
   at Microsoft.AspNetCore.Mvc.Formatters.Internal.AcceptHeaderParser.ParseAcceptHeader(IList`1 acceptHeaders, IList`1 parsedValues)
   at Microsoft.AspNetCore.Mvc.Internal.ObjectResultExecutor.GetMediaTypes(MediaTypeCollection contentTypes, HttpRequest request)
dougbu commented 8 years ago

/cc @javiercn

javiercn commented 8 years ago

I believe the accept header is invalid text/html, image/gif, image/jpeg, ; q=.9, */; q=.1

That said, we should probably provide a better error message.

sk29110 commented 8 years ago

So current version should work with request quality factor?

I was asked to investigate about this because it's mention here. https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html

danroth27 commented 8 years ago

We should fail more gracefully.

zhoro commented 8 years ago

I am also having issue with this value Accept: text/html, image/gif, image/jpeg, *; q=.2, */*; q=.2

Eilon commented 8 years ago

BTW I'm wondering what's generating these apparently invalid headers? Is it custom code? Or some other component that should perhaps also be fixed?

Eilon commented 8 years ago

@javiercn assigning to you because I assigned the PR https://github.com/aspnet/Mvc/pull/5162 to you.

sk29110 commented 8 years ago

my customer found out when using tool from Dell called Boomi to generate http get call it attaches the request quality factor, but my customer said it gave them option to turn off the RQF.

Eilon commented 8 years ago

@sk29110 great, thanks for the info!

javiercn commented 8 years ago

@sk29110 @zhoro Are the tools you are using automatically generating those headers or are your customers producing that input.

All the headers provided are incorrect according to the HTTP specification. If you read the grammar, media-range = ( "*/*" | (type "/" "*") | (type "/" subtype)) * (";" parameter ) you can see that the media type can't be any of these values "", "", "/", "*/".

What I'm trying to say is that those headers are not valid accept headers and that the server is just returning an incorrect failure.

That said, it would be useful to know if the tool is generating the incorrect header or if its being provided by the customer.

Hope this helps clarify things.

sk29110 commented 8 years ago

Unfortunately I don't have access the tool and just found out by testing http header user sent me. So it might not generating correct header. At the end customer found out it has option to turn off the request quality factor so I didn't bother investigating.

zhoro commented 8 years ago

@javiercn I also don't have access to the information about clients software.

videege commented 8 years ago

This issue stumped us for a while where a client was reporting 500 errors from our API where it appeared to be functioning normally. If an invalid accept header is encountered, is it necessary to return a 500 response (and if so, can that response be made much more useful than a non-descript 500 response?).

javiercn commented 8 years ago

@videege, were you creating the header manually or was a tool creating it as part of the request, if it's the latter, can you tell us what tool it was?

videege commented 8 years ago

The requests were coming from some external COTS Java system - I don't have many details about it. But overall, the header value clients submit is outside of our control, right?

I think this also changed since RC1 - we have an RC1 app that will ignore the same Accept header and return successful responses.

saketml79 commented 8 years ago

We faced exactly the same issue with Accept: text/html, image/gif, image/jpeg, ; q=.2, */; q=.2 . Is there a way to handle the client's Accept parameter with q in the code so that we do not return 500??

rynowak commented 8 years ago

I'd suggest for now that you can write a quick middleware to handle the accept header before it gets to MVC. You can either change the header and remove the invalid comma, or return a 400 if you'd prefer that.

// in Startup.Configure

app.Use(next => context => 
{
    var accept = context.Request.Headers["Accept"];
    if (accept.Contains(", ;")
    {
         // Update the header
         context.Request.Headers["Accept"] = accept.Replace(", ;");

         // OR return a 400
        context.Response.StatusCode = 400;
        return Task.FromResult(0);
    }

    return next(context)
}
NSchoemaker commented 8 years ago

Hi,

I also have the problem that the server returns a 500 when an invalid Accept header is sent to our API. Fixed it temporarily so it returns a 400 instead.

If i understand correctly the behaviour is going to be a 400 response in the future ?

TIA

javiercn commented 8 years ago

@NSchoemaker That is correct

nkassis commented 7 years ago

Just had to deal with this issue with one of our clients. Apparently the default accept header set by java.net.HTTPUrlConnection in java 6 was:

text/html, image/gif, image/jpeg, *; q=.2, */*; q=.2

I'm not sure that returning a 400 would be a good idea here, while this value might be invalid it may be good to follow the principle of "be liberal in what you accept from others." and just ignore the value sent in if invalid and use a proper default. That would be more inline with what other web servers seem to do for this header.

Eilon commented 7 years ago

@nkassis hmm interesting. Do you know if that Java bug has since been fixed? That header doesn't even make sense, does it?

One problem with being liberal about what we accept is that it's not clear how to interpret these values. I do agree with the principal in general, but in ambiguous cases we just need to be careful to not "make things worse."

danroth27 commented 7 years ago

With accept headers the server can always do whatever it wants, so I think it's ok to very liberal with invalid accept headers.

Eilon commented 7 years ago

@danroth27 ah, yes, of course. I guess the question is which "wrong" format do we optimize our liberal parser for? E.g. trying to interpret Java's "format" correctly might make the Boomi "format" worse.

rynowak commented 7 years ago

The other nice thing here is that we've got middleware. The 'server' in this case doesn't enforce anything. If we really wanted to we could write a strict parser in MVC, and recommend using rewrite or another middleware to normalize.

Obviously if we can write a parser that we're happy with in one place then that's simplest for everyone, but we've got some options if that's not possible.

danroth27 commented 7 years ago

I think we should optimize for the standard. If the header doesn't parse according to the standard then we consider it nonsensical and ignore it. Thoughts?

Eilon commented 7 years ago

@danroth27 would we / should we write a middleware that is optional and attempts to normalize requests? And if we do, but it's not enabled by default, is it worth it?

Eilon commented 7 years ago

(By default meaning: by default in templates, not in the runtime.)

javiercn commented 7 years ago

Ignoring errors I believe is a breaking change as you are serving some request that you previously wouldn't and I'm scared that might have some unintended consequences in the system or produce other errors harder to diagnose later on the pipeline.

I think this at best should be opt-in and at that point, if you have to deal with this scenario, I think it's just easier to put a small piece of middleware in front of UseMvc to modify the accept header and remove/filter any invalid value. That way the user is in control of what he wants to do and its not us making a decision (ignoring the value) for them.

I think there is enough ambiguity for us not to make a choice on behalf of the user, for example text/html, ;q=0.2 does this mean text/html;q=0.2 or does this mean text/html, */*;q=0.2

danroth27 commented 7 years ago

Ignoring errors I believe is a breaking change as you are serving some request that you previously wouldn't and I'm scared that might have some unintended consequences in the system or produce other errors harder to diagnose later on the pipeline.

I really don't think that concern is warranted. In generally the philosophy of the web is to be forgiving. If you can give a reasonable response then you should. The server is under no obligation to do anything with the accept header that the client sent. If the client sent an invalid accept header I think ignoring the header and sending a response with the default formatter is reasonable and would avoid customers getting blocked unnecessarily. If the customer wants stricter validation they can add middleware that does that.

Eilon commented 7 years ago

I think I'm ok with @danroth27 's proposed behavior.

Eilon commented 7 years ago

And I also agree that being more permissive is not a concerning breaking change.

dazinator commented 7 years ago

Is there a way to be notified when an invalid header has been "allowed" - that would be a useful event to intercept in terms of logging it as a warning for investigation. Perhaps this can be surfaced from MVC in someway?

If MVC is logging something when this happens then I think that would suffice, as when using something like serilog (and seq) it's easy to filter for such things.

javiercn commented 7 years ago

I don't think so, given that this logic is deeply buried into a low level component. We could definitely do something, but it would not be trivial. @rynowak any thought on this?