FileStreamResult writes to response body for HTTP HEAD when range processing is enabled

[dustinmoris]

Hi,

I was reviewing some code in order to see how ASP.NET has implemented streaming and I came across something which I believe is a bug, but I haven't tested it, so forgive me if I'm wrong.

I have noticed that if there is a HTTP HEAD request made where range processing is enabled that the FileResultExecutorBase calls the SetRangeHeaders method, which will return serveBody: true when the range header was valid.

The serveBody argument gets passed back into the ExecuteAsync method of FileStreamResultExecutor where it then continues to proceed and write the body of the response even though it was a HTTP HEAD request.

Can this be correct?

EDIT:

Also would you be interested in moving the basic functionality of streaming into the core ASP.NET project so it could be used from other frameworks such as Giraffe without having to copy the implementation?

I think it should be easily possible to create a method for HttpContext which could do the same as what a FileStreamResult does now, so it doesn't have to be an MVC specific thing. It could be wrapped by the FileStreamResult and then it could be re-used by other ASP.NET Core frameworks as well.

Any thoughts?

[dustinmoris]

Actually I think line 309 is wrong too, because the request might not even have a range header during a http HEAD request and yet serveBody is set to true.

[dustinmoris]

I think this bug has been introduced, because a HEAD should return the same metadata as a GET, except the HTTP body which should not be sent during a HEAD request according to the RFC:

9.4 HEAD
The HEAD method is identical to GET except that the server MUST NOT return a message-body in the response. The metainformation contained in the HTTP headers in response to a HEAD request SHOULD be identical to the information sent in response to a GET request. This method can be used for obtaining metainformation about the entity implied by the request without transferring the entity-body itself. This method is often used for testing hypertext links for validity, accessibility, and recent modification.

The response to a HEAD request MAY be cacheable in the sense that the information contained in the response MAY be used to update a previously cached entity from that resource. If the new field values indicate that the cached entity differs from the current entity (as would be indicated by a change in Content-Length, Content-MD5, ETag or Last-Modified), then the cache MUST treat the cache entry as stale.

[mkArtakMSFT]

@jbagga, can you please look into this? /cc: @Tratcher

[Tratcher]

Yeah, SetRangeHeaders needs its own Head check.

[jbagga]

@dustinmoris Thank you for bringing this to our attention. SetRangeHeaders indeed needs to check for the HTTP request method being Head.

@rynowak @Eilon for the second part of your issue about moving the core functionality for streaming

[rynowak]

@Tratcher - what at the deltas between what MVC does and https://github.com/aspnet/HttpAbstractions/blob/87cd79d6fc54bb4abf07c1e380cd7a9498a78612/src/Microsoft.AspNetCore.Http.Extensions/SendFileResponseExtensions.cs ?

Trying to help @dustinmoris question from the OP

Also would you be interested in moving the basic functionality of streaming into the core ASP.NET project so it could be used from other frameworks such as Giraffe without having to copy the implementation?

[Tratcher]

SendFileAsync's sole function is to copy the contents of the file to the network. It's a glorified Stream.WriteAsync(byte[]), and can even be called multiple times per request. it does not inspect request headers or set response headers.

MVC can check request headers like Range or If-xxx and alter the response accordingly. It will also set response headers like content-type, content-length, etag, last-modified, etc..

[dustinmoris]

Yes agreed, I think it would be hugely beneficial to have a range of HttpContext extensions which can do some of this common work. The MVC FileStreamResult is really not doing anything exotic or MVC specific, it is almost given that an application which needs to write a stream to the response will also have to check the Range header (and other mentioned headers) in order for a streaming endpoint to work as expected. This is so common that I think it really deserves a place in a lower level API (below MVC) from where it can be used in plain ASP.NET Core apps as well. Otherwise every framework on top of ASP.NET Core or even anyone who wants to write a plain ASP.NET Core app has to solve the same problem again, which is, as we can see, fairly cumbersome, so having this responsibility solved by the ASP.NET Core framework would be very nice.

Similar things already exist with other complex things like ChallengeAsync or AuthenticateAsync, etc. which are also extension methods which can be added to a plain ASP.NET Core app and then offer standard functionality which is really required by any app and not just MVC. MVC uses these methods to then wrap it into a nice Attribute driven API, but the core functionality is done in ASP.NET Core.

I think this is a good approach and I would love to see this type of architecture take place in other domains of ASP.NET Core as well.

[jbagga]

@Eilon The bug fix is in. Should the second part of the issue (the architecture discussion about moving FileStreamResult to a lower level API) be moved to a new issue in aspnet/Home?

[dustinmoris]

Hello, thanks for the prompt fix! Sorry for being annoying, but I am implementing something similar in a different application right now and reading all the RFCs and related docs and then comparing it to how MVC has done it as a point of reference and I think I just noticed another very minor bug.

The optional ETag gets matched against the If-Match and If-None-Match HTTP headers and for both headers MVC uses the same method called GetEtagMatchState. On line 264 the comparison method specifies a hard coded useStrongComparison: true but this is only correct for the If-Match header. During the validation of the If-None-Match header it is supposed to use weak comparison:

A recipient MUST use the weak comparison function when comparing
entity-tags for If-None-Match (Section 2.3.2), since weak entity-tags
can be used for cache validation even if there have been changes to
the representation data.

This one can be probably fixed quicker than me creating a new issue, but if you guys prefer to split it out into a new issue and the architectural question as well then I'm happy to do it - just let me know!

[jbagga]

Thanks @dustinmoris!

@Tratcher Looks like in StaticFiles the fix is simple https://github.com/aspnet/StaticFiles/blob/dev/src/Microsoft.AspNetCore.StaticFiles/StaticFileContext.cs#L193
In MVC I think we can have a bool passed in to GetEtagMatchState to specify strong or weak comparison. I'll work on the PR for MVC.

[davidfowl]

Similar to this #6260?

[dustinmoris]

Last question on the topic of FileStreamResult which I'll throw in for discussion. Looking at the GetPreconditionState I can see that if a web browser sends the If-Match (or other If-...) conditional headers then they get only validated on the server if a user provided an etag value. Is that correct? I would have expected that if a server sends a condition like If-Match that a server will send back a PreconditionFailed response if the provided ETags couldn't be matched, regardless if there was a different ETag or no ETag at all. What do you think?

[Tratcher]

Hmmm. For background this code was pulled over from StaticFiles where there's always an e-tag. The absence of an e-tag hasn't been thought through.

https://tools.ietf.org/html/rfc7232#section-3.1
Half of the If-Match comments are about uploads, which are out of scope for the code we're discussing. The developer should have processed those before calling these response APIs.

"It can also be used with safe
methods to abort a request if the selected representation does not
match one already stored (or partially stored) from a prior request."

It doesn't say what to do if the server can't provide an e-tag for the resource. However, e-tags are opaque tokens and are only provided by the server. For a client to have and send one it must have gotten it from the server on a prior request. In this regard I'd expect consistent behavior from the server, either always or never providing an e-tag for a given resource. That makes it extremely unlikely that we'd see this mismatch scenario in real life scenarios, so the existing behavior is probobly fine.

[jbagga]

Moved the discussion about streaming to #7259