ring icon indicating copy to clipboard operation
ring copied to clipboard

Consequences of wrap-file's :index-files? default on wrap-content-type

Open wls opened this issue 4 years ago • 10 comments

Consider this simple static file delivery, but observe carefully its delivered Content-Type header:

(-> handler
    (wrap-file "public") ; :index-files? defaults to true, serving index.* files in directories
    (wrap-content-type)) ; set Content-Type based on the file extension in the URI

While this will always serve up index.html if it exists, how it gets delivered changes.

http://localhost/index.html — correctly serves with a mime type of text/html, due to the specified file extension in the URI. Good.

http://localhost/ — causes wrap-file to deliver index.html by default, but as the URI has no file extension wrap-content-type, unaware that wrap-file automatically selected this resource, delivers a response with a mime type of application/octet-stream. This is a problem, as it causes browsers, like Firefox, to download the content rather than display it. Bad.

Unfortunately, I'm too new to Ring to know if I'm doing this wrong, so apologies in advance should this turn into user education issue, but I didn't see in the documentation how to address it.

It seems that wrap-content-type should either be aware of the "effective URI" at best or allow its content-type-response to accept an override – though doing so isn't always reliable, as it may not always be an html index file delivered.

The behavior of wrap-file (or even wrap-resource) seems to be breaking a strong assumption made downstream by the loosely coupled wrap-content-type about the URI.


An aside: workarounds suggesting making a special route for that one off case don't take into consideration that other subdirectories suffer from the same problem; hacking routes to correct a middleware issue is obviously the wrong way to go.

wls avatar May 24 '20 16:05 wls

Yes, wrap-file and wrap-resource don't currently guess the media type from the file location, and they probably should do. PRs are welcome.

weavejester avatar May 24 '20 17:05 weavejester

What would be an acceptable design solution that stays within the philosophy of the library?

For instance, I can see where someone might actually want to get at the unmodified URI for some reason. So conveying an "effective URI" with the fully qualified filename along would be one way to do it. Is it acceptable, then, to add a new, optional, field to the request object (is that the right place) and let wrap-content-type use it, if present, otherwise fallback to the original URI?

I'm trying to think of minimal, efficient solutions that could correct the behavior, preserve backwards compatibility, and incur only the necessary overhead when needed. I'm typically not a fan of side-effects, so I'm a little hesitant how to pass this along otherwise.

wls avatar May 24 '20 17:05 wls

The wrap-file and wrap-resource middleware would add their own content-type header, using the functions in ring.util.mimetype.

weavejester avatar May 24 '20 18:05 weavejester

Today I ran into this as well. I had a look at the Ring code, and locally I fixed it, but in a different way. I modified the wrap-content-type middleware so that:

if the body of the request is a file, then use the mime type of that file instead of using the request uri.

@weavejester Does that seem like a good approach to you? If so, I'll make a PR asap.

svdo avatar Dec 07 '21 21:12 svdo

The request URI extension should take priority, but if there is no URI extension, then it can use the extension of the File object. This ensures that Ring remains backward compatible.

weavejester avatar Dec 07 '21 22:12 weavejester

Right, backwards compatibility is indeed important but that way it doesn't fix the issue.

I'll try if I can somehow add the header in file and resource middleware like you initially suggested.

svdo avatar Dec 08 '21 06:12 svdo

I could also add an option to content-type to retain backward compat, since I feel it is actually a bug...

Or maybe "if req uri points to DIRECTORY and response body is FILE then use mine type of file".

svdo avatar Dec 08 '21 06:12 svdo

Right, backwards compatibility is indeed important but that way it doesn't fix the issue.

Why doesn't it fix the issue?

weavejester avatar Dec 08 '21 06:12 weavejester

Oh wait it probably does, I think you're right. I'll have another go at it. (I misread your comment, the word extension being crucial.)

svdo avatar Dec 08 '21 07:12 svdo

@weavejester I created the pull request. Please let me know if you want me to change anything.

svdo avatar Dec 08 '21 08:12 svdo