httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Luca Toscano <toscano.l...@gmail.com>
Subject Re: svn commit: r1757982 - /httpd/httpd/branches/2.4.x/STATUS
Date Sat, 10 Sep 2016 10:07:05 GMT
Hi Yann,

2016-09-10 1:13 GMT+02:00 Yann Ylavic <ylavic.dev@gmail.com>:

> On Sat, Aug 27, 2016 at 12:12 PM,  <elukey@apache.org> wrote:
> > Author: elukey
> > Date: Sat Aug 27 10:12:23 2016
> > New Revision: 1757982
> >
> > URL: http://svn.apache.org/viewvc?rev=1757982&view=rev
> > Log:
> > Updated backport proposal with the latest discussion on dev@
> > about Last-Modified header handling.
> > I removed jchampion's vote since it was related to a completely
> > different solution.
> >
> >
> > Modified:
> >     httpd/httpd/branches/2.4.x/STATUS
> >
> []
> >
> >    *) core: Drop an invalid Last-Modified header value coming
> > -     from a FCGI/CGI script instead of replacing it with Unix epoch.
> > -     Warn the users about Last-Modified header value replacements and
> > -     improved handling of non-GMT datestr.
> > +     from a (F)CGI script instead of replacing it with Unix epoch.
> > +     Warn the users about Last-Modified header value replacements
> > +     and violations of the RFC.
> >       trunk patch: http://svn.apache.org/r1748379
> >                    http://svn.apache.org/r1750747
> >                    http://svn.apache.org/r1750749
> > @@ -139,11 +139,13 @@ PATCHES PROPOSED TO BACKPORT FROM TRUNK:
> >                    http://svn.apache.org/r1751138
> >                    http://svn.apache.org/r1751139
> >                    http://svn.apache.org/r1751147
> > -     2.4.x: trunk patches works (final view http://apaste.info/9v3)
> > -     The last revision has been discussed in dev@ and submitted by
> Yann.
> > +                  http://svn.apache.org/r1757818
> > +     2.4.x: trunk patches works (final view http://apaste.info/FCs)
> > +     The last revision has been discussed extensively in dev@ and this
> seems to be
> > +     a good compromise for the moment.
> >       Tested the code with a simple PHP script returning different
> Last-Modified
> > -     headers (GMT now, GMT now Europe/Paris, GMT tomorrow, GMT
> yesterday).
> > -     +1: elukey, jchampion
> > +     headers (GMT now, GMT now Europe/Paris, GMT tomorrow, GMT
> yesterday, PST now).
> > +     +1: elukey
>
> As read it, this proposal (http://apaste.info/FCs) does not enforce
> GMT date and will treat any other timezone as if it were GMT (per
> apr_date_parse_http(), which does not look for "GMT" anywhere), right?
>

Yes, it leaves the current behavior unaltered (except for APR_DATE_BAD).


>
> Shouldn't we either ignore (i.e. not forward) non-GMT, or change it to
> GMT if it's a valid timezone?
>
> The only way to do this would be to use apr_date_parse_rfc() in the first
> place.
> If we'd want to ignore non-GMT, compare it to apr_date_parse_http().
> And only otherwise or if it matches, let it go through ap_update_mtime().
>

My decision to not use apr_date_parse_rfc came after the long discussion in
dev@ about whether or not a Last-Modified header coming from a CGI backend
should be considered HTTP input (so assuming GMT IIUC) or not. Since we
didn't reach a complete agreement on how to proceed I thought to propose a
light change as starter (to answer a question raised in users@ a while back
about warning admins on httpd modifications of the Last-Modified header
value) and think about more invasive changes in the future, like dropping a
non GMT header or converting it in GMT if on a valid timezone.


>
> I don't the difference between this proposal and the current code (but
> TRACEs), ap_update_mtime() is a noop with APR_DATE_BAD.
> Did I miss something?
>

This is a very good point, I didn't notice an important part. From what I
can see ap_update_mtime is indeed not updating anything, but r->mtime is
already 0 and the ap_set_last_modified picks it up and use it (ending up
with a Unix epoch datetime string). My check on APR_DATE_BAD avoids the
call to ap_set_last_modified, this is why the header is dropped. Maybe we
also need something like the following?

 AP_DECLARE(void) ap_set_last_modified(request_rec *r)
 {
-    if (!r->assbackwards) {
+    if (!r->assbackwards && r->mtime > 0) {
         apr_time_t mod_time = ap_rationalize_mtime(r, r->mtime);
         char *datestr = apr_palloc(r->pool, APR_RFC822_DATE_LEN);


Thanks for following up!

Regards,

Luca

Mime
View raw message