Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
MIME-Version: 1.0
In-Reply-To: <CAFedD42_veJw3P-waLYb2DTGZtw_V16A0+vm+k61cOQsveh2-Q@mail.gmail.com>
References: <20160827101224.2B0063A0ED1@svn01-us-west.apache.org>
 <CAKQ1sVOt630mNsnPe=NN=rBw-QRuOmYktRqBebWYQZ4bLOCBTw@mail.gmail.com> <CAFedD42_veJw3P-waLYb2DTGZtw_V16A0+vm+k61cOQsveh2-Q@mail.gmail.com>
From: Yann Ylavic <ylavic.dev@gmail.com>
Date: Sat, 10 Sep 2016 13:58:25 +0200
Message-ID: <CAKQ1sVMLjU22UGXc2FoWfvgaR5k7Yu4wB-UF2v9hrSaRVXkvRw@mail.gmail.com>
Subject: Re: svn commit: r1757982 - /httpd/httpd/branches/2.4.x/STATUS
To: httpd-dev <dev@httpd.apache.org>
Content-Type: text/plain; charset=UTF-8
archived-at: Sat, 10 Sep 2016 11:58:38 -0000

Hi Luca,

On Sat, Sep 10, 2016 at 12:07 PM, Luca Toscano <toscano.luca@gmail.com> wrote:
>
> 2016-09-10 1:13 GMT+02:00 Yann Ylavic <ylavic.dev@gmail.com>:
>>
>> 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).

So we still may change the Last-Modified sent by a CGI...

>>
>> 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.

If we really didn't want to modify the (semantic of the )header, we'd
accept other timezones and convert it to GMT.
But dropping or interpreting it as GMT when it's not (i.e. s/<any
timezone>/GMT/ without changing the date) is a modification anyway.

So if we want to stick to the RFC (and it's very strict
interpretation, which I'm not sure it's that strict), either we ignore
or we error on non-GMT (the latter would be an unacceptable change in
behaviour IMHO).

>
>>
>>
>> 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.

Yes, I missed ap_set_last_modified() was using ~now when given zero.

> 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);

That'd change its behaviour, one may currently use it to set
Last-Modified to now, w/o calling ap_update_mtime(), and it used to
work...

Anyway, my personal preference would be to tolerate a different
timezone (than GMT), or at at worse ignore the header (if not GMT).

So I think the patch (against 2.4.x) would be something like :

Index: server/util_script.c
===================================================================
--- server/util_script.c    (revision 1760114)
+++ server/util_script.c    (working copy)
@@ -670,11 +670,26 @@ AP_DECLARE(int) ap_scan_script_header_err_core_ex(
         }
         /*
          * If the script gave us a Last-Modified header, we can't just
-         * pass it on blindly because of restrictions on future values.
+         * pass it on blindly (RFC mandates GMT).  Future values are
+         * handled by ap_set_last_modified() and changed to now.
          */
         else if (!strcasecmp(w, "Last-Modified")) {
-            ap_update_mtime(r, apr_date_parse_http(l));
-            ap_set_last_modified(r);
+            apr_time_t parsed_date = apr_date_parse_rfc(l);
+            if (parsed_date == APR_DATE_BAD) {
+               ap_log_rerror(SCRIPT_LOG_MARK, APLOG_DEBUG, 0, r,
+                             "Ignored Last-Modified header value: '%s'",
+                             l);
+            }
+            else if(parsed_date != apr_date_parse_http(l)) {
+                ap_log_rerror(SCRIPT_LOG_MARK, APLOG_DEBUG, 0, r,
+                              "Ignored Last-Modified header value (not "
+                              "within the GMT timezone, as required): '%s'",
+                              l);
+            }
+            else {
+                ap_update_mtime(r, parsed_date);
+                ap_set_last_modified(r);
+            }
         }
         else if (!strcasecmp(w, "Set-Cookie")) {
             apr_table_add(cookie_table, w, l);
_


Or the lenient mode :

Index: server/util_script.c
===================================================================
--- server/util_script.c    (revision 1760114)
+++ server/util_script.c    (working copy)
@@ -670,11 +670,21 @@ AP_DECLARE(int) ap_scan_script_header_err_core_ex(
         }
         /*
          * If the script gave us a Last-Modified header, we can't just
-         * pass it on blindly because of restrictions on future values.
+         * pass it on blindly (RFC mandates GMT, but be lenient and
+         * transform other timezones to GMT).  Future values are handled
+         * by ap_set_last_modified() and changed to now.
          */
         else if (!strcasecmp(w, "Last-Modified")) {
-            ap_update_mtime(r, apr_date_parse_http(l));
-            ap_set_last_modified(r);
+            apr_time_t parsed_date = apr_date_parse_rfc(l);
+            if (parsed_date == APR_DATE_BAD) {
+               ap_log_rerror(SCRIPT_LOG_MARK, APLOG_DEBUG, 0, r,
+                             "Ignored Last-Modified header value: '%s'",
+                             l);
+            }
+            else {
+                ap_update_mtime(r, parsed_date);
+                ap_set_last_modified(r);
+            }
         }
         else if (!strcasecmp(w, "Set-Cookie")) {
             apr_table_add(cookie_table, w, l);
_


Regards,
Yann.