Hi Yann, 2016-09-10 1:13 GMT+02:00 Yann Ylavic <[email protected]>:
> On Sat, Aug 27, 2016 at 12:12 PM, <[email protected]> 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
