Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

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

Closed
sk29110 opened this issue Aug 16, 2016 · 33 comments
Closed

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

sk29110 opened this issue Aug 16, 2016 · 33 comments
Assignees
Labels
3 - Done bug up-for-grabs Members of our awesome commnity can handle this issue
Milestone

Comments

@sk29110
Copy link

sk29110 commented Aug 16, 2016

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
Copy link
Member

dougbu commented Aug 16, 2016

/cc @javiercn

@javiercn
Copy link
Member

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
Copy link
Author

sk29110 commented Aug 17, 2016

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 danroth27 added this to the Backlog milestone Aug 17, 2016
@danroth27 danroth27 added the up-for-grabs Members of our awesome commnity can handle this issue label Aug 17, 2016
@danroth27
Copy link
Member

We should fail more gracefully.

@zhoro
Copy link

zhoro commented Aug 19, 2016

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

@Eilon
Copy link
Member

Eilon commented Aug 19, 2016

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 Eilon modified the milestones: 1.1.0, Backlog Aug 19, 2016
@Eilon
Copy link
Member

Eilon commented Aug 19, 2016

@javiercn assigning to you because I assigned the PR #5162 to you.

@sk29110
Copy link
Author

sk29110 commented Aug 20, 2016

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
Copy link
Member

Eilon commented Aug 20, 2016

@sk29110 great, thanks for the info!

@javiercn
Copy link
Member

@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
Copy link
Author

sk29110 commented Aug 26, 2016

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
Copy link

zhoro commented Aug 26, 2016

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

@videege
Copy link

videege commented Sep 15, 2016

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
Copy link
Member

@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
Copy link

videege commented Sep 15, 2016

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
Copy link

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
Copy link
Member

rynowak commented Sep 16, 2016

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
Copy link

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
Copy link
Member

@NSchoemaker That is correct

@nkassis
Copy link

nkassis commented Sep 29, 2016

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
Copy link
Member

Eilon commented Sep 29, 2016

@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
Copy link
Member

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
Copy link
Member

Eilon commented Sep 30, 2016

@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
Copy link
Member

rynowak commented Sep 30, 2016

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
Copy link
Member

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
Copy link
Member

Eilon commented Sep 30, 2016

@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
Copy link
Member

Eilon commented Sep 30, 2016

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

@javiercn
Copy link
Member

javiercn commented Sep 30, 2016

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
Copy link
Member

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
Copy link
Member

Eilon commented Oct 2, 2016

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

@Eilon
Copy link
Member

Eilon commented Oct 2, 2016

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

@dazinator
Copy link

dazinator commented Dec 5, 2016

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
Copy link
Member

javiercn commented Dec 6, 2016

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?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
3 - Done bug up-for-grabs Members of our awesome commnity can handle this issue
Projects
None yet
Development

No branches or pull requests