On Wed, Jun 13, 2018 at 08:07:11PM +0200, Reyk Floeter wrote: > On Sat, Jan 13, 2018 at 05:23:35PM +0100, Sebastian Benoit wrote: > > Hiltjo Posthuma(hil...@codemadness.org) on 2018.01.13 13:08:38 +0100: > > > On Sat, Jan 13, 2018 at 09:39:44AM +0100, Anton Lindqvist wrote: > > > > On Tue, Jan 09, 2018 at 05:38:57PM +0100, Hidv?gi G?bor wrote: > > > > > >Synopsis: httpd reports wrong mimetype when item is in the browser > > > > > >cache > > > > > >Category: httpd > > > > > >Environment: > > > > > System : OpenBSD 6.2 > > > > > Details : OpenBSD 6.2 (GENERIC) #91: Wed Oct 4 00:35:21 > > > > > MDT > > > > > 2017 > > > > > > > > > > dera...@armv7.openbsd.org:/usr/src/sys/arch/armv7/compile/GENERIC > > > > > > > > > > Architecture: OpenBSD.armv7 > > > > > Machine : armv7 > > > > > >Description: > > > > > > > > > > httpd serves static files (eg. images) with Last-Modified http > > > > > header. When > > > > > a browser next time asks whether this file changed (sends > > > > > If-Modified-Since > > > > > http header) httpd responds with wrong mimetype, 'text/html' when the > > > > > resource is in the browser cache (304 Not Modified status code). > > > > > > > > > > >How-To-Repeat: > > > > > > > > > > This bug is common, not arm only. When for example you open this > > > > > image: > > > > > https://man.openbsd.org/openbsd.gif > > > > > > > > > > in a browser with developer tools (F12) open, on the network tab you > > > > > can > > > > > take a look at the response headers, mimetype is correct (image/gif). > > > > > After > > > > > opening press refresh (F5) and look at the response headers again, > > > > > and you > > > > > get the incorrect mimetype, 'text/html'. > > > > > > > > > > >Fix: > > > > > > > > > > check httpd source > > > > > > > > Please try out this diff, it makes sure to set the correct MIME-type and > > > > not respond with a body if the resource has not changed. Sending this to > > > > tech@ as well. > > > > > > > > Index: server_file.c > > > > =================================================================== > > > > RCS file: /cvs/src/usr.sbin/httpd/server_file.c,v > > > > retrieving revision 1.65 > > > > diff -u -p -r1.65 server_file.c > > > > --- server_file.c 2 Feb 2017 22:19:59 -0000 1.65 > > > > +++ server_file.c 12 Jan 2018 19:10:20 -0000 > > > > @@ -230,8 +230,15 @@ server_file_request(struct httpd *env, s > > > > goto abort; > > > > } > > > > > > > > - if ((ret = server_file_modified_since(clt->clt_descreq, st)) != > > > > -1) > > > > - return (ret); > > > > + if (server_file_modified_since(clt->clt_descreq, st) == 0) { > > > > + media = media_find_config(env, srv_conf, path); > > > > + ret = server_response_http(clt, 304, media, 0, > > > > + st->st_mtim.tv_sec); > > > > + if (ret != -1) > > > > + goto done; > > > > + else > > > > + goto fail; > > > > + } > > > > > > > > /* Now open the file, should be readable or we have another > > > > problem */ > > > > if ((fd = open(path, O_RDONLY)) == -1) > > > > @@ -663,10 +670,10 @@ server_file_modified_since(struct http_d > > > > if (strptime(since->kv_value, > > > > "%a, %d %h %Y %T %Z", &tm) != NULL && > > > > timegm(&tm) >= st->st_mtim.tv_sec) > > > > - return (304); > > > > + return (0); > > > > } > > > > > > > > - return (-1); > > > > + return (1); > > > > } > > > > > > > > int > > > > > > > > > > Hey, > > > > > > I've tested your patch. > > > > > > When requesting a non-modified CSS file: > > > > > > #!/bin/sh > > > host="127.0.0.1" > > > port="6970" > > > printf 'GET /style.css HTTP/1.1\r\nHost: %s:%s\r\nIf-Modified-Since: > > > Sat, 16 Dec 2017 13:07:53 GMT\r\nConnection: close\r\n\r\n' "$host" > > > "$port" | \ > > > nc "$host" "$port" > > > > > > Full HTTP response: > > > > > > HTTP/1.1 304 Not Modified > > > Connection: close > > > Content-Length: 0 > > > Content-Type: text/css > > > Date: Sat, 13 Jan 2018 11:54:13 GMT > > > Last-Modified: Sun, 05 Mar 2017 12:22:05 GMT > > > Server: OpenBSD httpd > > > > > > I wonder if httpd should just omit the response header Content-Length and > > > Content-Type entirely for this statuscode. Some httpd such as Nginx just > > > omit them aswell. > > > > rfc7230 HTTP/1.1 Message Syntax and Routing page 29f. > > > > A server MAY send a Content-Length header field in a 304 (Not > > Modified) response to a conditional GET request (Section 4.1 of > > [RFC7232]); a server MUST NOT send Content-Length in such a response > > *unless its field-value equals the decimal number of octets that would* > > have been sent in the payload body of a 200 (OK) response to the same > > request. > > > > So I think it is better to omit the Content-Length header. I'm afraid > that a (potentially broken) client will wait for a body even with the > 304 response. > > The diff below is based on Anton's fix but without changing the code's > style to a boolean 0/1. It also avoids an mtime in the future (same > as the other server_response_http() further below), and omits the > Content-Length by accepting a -1 size. > > Reyk > > Index: usr.sbin/httpd/server_file.c > =================================================================== > RCS file: /cvs/src/usr.sbin/httpd/server_file.c,v > retrieving revision 1.65 > diff -u -p -u -p -r1.65 server_file.c > --- usr.sbin/httpd/server_file.c 2 Feb 2017 22:19:59 -0000 1.65 > +++ usr.sbin/httpd/server_file.c 13 Jun 2018 18:02:45 -0000 > @@ -230,8 +230,14 @@ server_file_request(struct httpd *env, s > goto abort; > } > > - if ((ret = server_file_modified_since(clt->clt_descreq, st)) != -1) > - return (ret); > + if ((ret = server_file_modified_since(clt->clt_descreq, st)) != -1) { > + /* send the header without a body */ > + media = media_find_config(env, srv_conf, path); > + if ((ret = server_response_http(clt, ret, media, -1, > + MINIMUM(time(NULL), st->st_mtim.tv_sec))) == -1) > + goto fail; > + goto done; > + } > > /* Now open the file, should be readable or we have another problem */ > if ((fd = open(path, O_RDONLY)) == -1) > Index: usr.sbin/httpd/server_http.c > =================================================================== > RCS file: /cvs/src/usr.sbin/httpd/server_http.c,v > retrieving revision 1.120 > diff -u -p -u -p -r1.120 server_http.c > --- usr.sbin/httpd/server_http.c 11 Jun 2018 12:12:51 -0000 1.120 > +++ usr.sbin/httpd/server_http.c 13 Jun 2018 18:02:46 -0000 > @@ -1362,7 +1362,7 @@ server_response_http(struct client *clt, > (error = server_httperror_byid(code)) == NULL) > return (-1); > > - if (server_log_http(clt, code, size) == -1) > + if (server_log_http(clt, code, size >= 0 ? size : 0) == -1) > return (-1); > > /* Add error codes */ > @@ -1388,9 +1388,9 @@ server_response_http(struct client *clt, > return (-1); > > /* Set content length, if specified */ > - if ((cl = > + if (size >= 0 && ((cl = > kv_add(&resp->http_headers, "Content-Length", NULL)) == NULL || > - kv_set(cl, "%lld", (long long)size) == -1) > + kv_set(cl, "%lld", (long long)size) == -1)) > return (-1); > > /* Set last modification time */ > @@ -1423,7 +1423,7 @@ server_response_http(struct client *clt, > server_bufferevent_print(clt, "\r\n") == -1) > return (-1); > > - if (size == 0 || resp->http_method == HTTP_METHOD_HEAD) { > + if (size <= 0 || resp->http_method == HTTP_METHOD_HEAD) { > bufferevent_enable(clt->clt_bev, EV_READ|EV_WRITE); > if (clt->clt_persist) > clt->clt_toread = TOREAD_HTTP_HEADER;
Hey Reyk, I've tested the patch and it works fine. Thanks for looking into it :) I wonder, maybe the media/Content-Type can be optional too when the data is not modified. Updated diff below, what do you think? diff --git usr.sbin/httpd/server_file.c usr.sbin/httpd/server_file.c index 4255119e659..1207736548e 100644 --- usr.sbin/httpd/server_file.c +++ usr.sbin/httpd/server_file.c @@ -230,8 +230,13 @@ server_file_request(struct httpd *env, struct client *clt, char *path, goto abort; } - if ((ret = server_file_modified_since(clt->clt_descreq, st)) != -1) - return (ret); + if ((ret = server_file_modified_since(clt->clt_descreq, st)) != -1) { + /* send the header without a body */ + if ((ret = server_response_http(clt, ret, NULL, -1, + MINIMUM(time(NULL), st->st_mtim.tv_sec))) == -1) + goto fail; + goto done; + } /* Now open the file, should be readable or we have another problem */ if ((fd = open(path, O_RDONLY)) == -1) diff --git usr.sbin/httpd/server_http.c usr.sbin/httpd/server_http.c index b8146c3a115..a906c74803f 100644 --- usr.sbin/httpd/server_http.c +++ usr.sbin/httpd/server_http.c @@ -1358,11 +1358,10 @@ server_response_http(struct client *clt, unsigned int code, struct kv *ct, *cl; char tmbuf[32]; - if (desc == NULL || media == NULL || - (error = server_httperror_byid(code)) == NULL) + if (desc == NULL || (error = server_httperror_byid(code)) == NULL) return (-1); - if (server_log_http(clt, code, size) == -1) + if (server_log_http(clt, code, size >= 0 ? size : 0) == -1) return (-1); /* Add error codes */ @@ -1383,14 +1382,14 @@ server_response_http(struct client *clt, unsigned int code, return (-1); /* Set media type */ - if ((ct = kv_add(&resp->http_headers, "Content-Type", NULL)) == NULL || - kv_set(ct, "%s/%s", media->media_type, media->media_subtype) == -1) + if (media && ((ct = kv_add(&resp->http_headers, "Content-Type", NULL)) == NULL || + kv_set(ct, "%s/%s", media->media_type, media->media_subtype) == -1)) return (-1); /* Set content length, if specified */ - if ((cl = + if (size >= 0 && ((cl = kv_add(&resp->http_headers, "Content-Length", NULL)) == NULL || - kv_set(cl, "%lld", (long long)size) == -1) + kv_set(cl, "%lld", (long long)size) == -1)) return (-1); /* Set last modification time */ @@ -1423,7 +1422,7 @@ server_response_http(struct client *clt, unsigned int code, server_bufferevent_print(clt, "\r\n") == -1) return (-1); - if (size == 0 || resp->http_method == HTTP_METHOD_HEAD) { + if (size <= 0 || resp->http_method == HTTP_METHOD_HEAD) { bufferevent_enable(clt->clt_bev, EV_READ|EV_WRITE); if (clt->clt_persist) clt->clt_toread = TOREAD_HTTP_HEADER; -- Kind regards, Hiltjo