Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wls opened this issue May 24, 2020 · 10 comments
Open

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

wls opened this issue May 24, 2020 · 10 comments

Comments

@wls
Copy link

wls commented May 24, 2020

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.

@weavejester
Copy link
Member

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.

@wls
Copy link
Author

wls commented May 24, 2020

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.

@weavejester
Copy link
Member

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

@svdo
Copy link

svdo commented Dec 7, 2021

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.

@weavejester
Copy link
Member

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.

@svdo
Copy link

svdo commented Dec 8, 2021

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

svdo commented Dec 8, 2021

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".

@weavejester
Copy link
Member

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

Why doesn't it fix the issue?

@svdo
Copy link

svdo commented Dec 8, 2021

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

svdo commented Dec 8, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants