Re: httpd response mimetype bug

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: httpd response mimetype bug

Anton Lindqvist-2
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
>
> [hidden email]:/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

Reply | Threaded
Open this post in threaded view
|

Re: httpd response mimetype bug

Anton Lindqvist-2
On Sat, Jan 13, 2018 at 01:08:38PM +0100, Hiltjo Posthuma wrote:

> 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
> > >
> > > [hidden email]:/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.
>
> At the moment responses with redirects (statuscode 301 and 302) also
> output a body response. Maybe it is better to handle it in server_http.c
> in the function server_abort_http() and not output the body there for some
> response status codes? I'm not sure.

Quoting the RFC[1]:

> A server MAY send a Content-Length header field in a 304 (Not
> Modified) response to a conditional GET request ...; 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 my previous diff was obviously wrong since the actual size must be
used if the Content-Length header is included. Including it seems to
result in a less intrusive diff. As for the Content-Type header, I can't
find any guidance on whetever it can or cannot be included.

[1] https://tools.ietf.org/html/rfc7230#section-3.3.2

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 13 Jan 2018 15:01:33 -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, st->st_size,
+    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

Reply | Threaded
Open this post in threaded view
|

Re: httpd response mimetype bug

Sebastian Benoit-3
In reply to this post by Anton Lindqvist-2
Hiltjo Posthuma([hidden email]) 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
> > >
> > > [hidden email]:/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.

> At the moment responses with redirects (statuscode 301 and 302) also
> output a body response. Maybe it is better to handle it in server_http.c
> in the function server_abort_http() and not output the body there for some
> response status codes? I'm not sure.
>
> --
> Kind regards,
> Hiltjo
>