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

fileserver: Better handling of HTTP status override #4132

Merged
merged 2 commits into from
Apr 29, 2021

Conversation

francislavoie
Copy link
Member

@francislavoie francislavoie commented Apr 24, 2021

Just something I noticed when reading through the code; I think the io.Copy was a remnant of an older version of that code. I don't see a reason why we don't just let it fall through and serve the file with http.ServeContent normally. More consistent that way.

Also, I hoisted the status_code override logic above the error handler detection bit, because then it allows for overriding the status code during error routes. Not necessarily that likely to be useful, but it's more correct. (Reminder that the first w.WriteHeader() wins, subsequent ones have no effect). Marking this as a bug just for this reason, but it's barely worth discussing as a bug 😅.

Edit: So calling w.WriteHeader() ourselves makes the w.Header().Set() operations in http.ServeContent not actually work correctly, which isn't ideal.

After a bit of googling, I found a great solution, by wrapping the ResponseWriter to intercept the w.WriteHeader() call that http.ServeContent does to write the header we want instead.

Just something I noticed when reading through the code; I think the `io.Copy` was a remnant of an older version of that code. I don't see a reason why we don't just let it fall through and serve the file with `http.ServeContent` normally. More consistent that way.

Also, I hoisted the `status_code` override logic above the error handler detection bit, because then it allows for overriding the status code during error routes. Not necessarily that likely to be useful, but it's more correct. (Reminder that the first `w.WriteHeader()` wins, subsequent ones have no effect)
@francislavoie francislavoie added the bug 🐞 Something isn't working label Apr 24, 2021
@francislavoie francislavoie added this to the v2.4.0 milestone Apr 24, 2021
This makes sure additional header fields can be set by `http.ServeContent`, by only intercepting the `WriteHeader` call when `http.ServeContent` actually calls it.
@francislavoie francislavoie changed the title fileserver: Don't need io.Copy in error handling fileserver: Better handling of HTTP status override Apr 28, 2021
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this LGTM.

You did test this, right? 😄

@francislavoie
Copy link
Member Author

francislavoie commented Apr 29, 2021

Yep, with this Caddyfile, and a 404.html file in $PWD

{
        admin off
        debug
}

:8883 {
        root * .

        handle_errors {
                rewrite * /404.html
                file_server
        }

        file_server
}

Request to http://localhost:8883/ serves /404.html with status 404, and Last-Modified header is set (wasn't before)

Before:

image

After:

image

@francislavoie francislavoie merged commit 3a1e81d into caddyserver:master Apr 29, 2021
@francislavoie francislavoie deleted the fileserver-cleanup branch April 29, 2021 06:01
@mholt
Copy link
Member

mholt commented Apr 29, 2021

Perfect, thanks so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants