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([email protected]) 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
> > > > >
> > > > > [email protected]:/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
