Return-Path: Delivered-To: apmail-httpd-dev-archive@httpd.apache.org Received: (qmail 37286 invoked by uid 500); 3 Jul 2003 14:39:09 -0000 Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm Precedence: bulk Reply-To: dev@httpd.apache.org list-help: list-unsubscribe: list-post: Delivered-To: mailing list dev@httpd.apache.org Received: (qmail 37201 invoked from network); 3 Jul 2003 14:39:07 -0000 Message-ID: <3F043FAC.9010701@entegrity.com> Date: Thu, 03 Jul 2003 10:37:32 -0400 From: Ryan Eberhard User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0.2) Gecko/20021120 Netscape/7.01 X-Accept-Language: en-us, en MIME-Version: 1.0 To: dev@httpd.apache.org CC: Ryan Eberhard Subject: Re: [PATCH] Bug 18388: Set-Cookie header not honored on 304 (Not modified) status References: <3EDCB64E.1090301@entegrity.com> <3EDE113B.5070106@entegrity.com> <3EDFAD91.2060908@entegrity.com> <3EE0CE81.4090608@entegrity.com> <3EE4B2FD.8050005@entegrity.com> Content-Type: multipart/alternative; boundary="------------000208020409070408070200" X-MW-BTID: 090525000020031845274700014 X-MW-CTIME: 1057243147 X-MW-SENDING-MTA: 192.92.110.50 HOP-COUNT: 1 X-MAILWATCH-INSTANCEID: 01020009edb5b04e-41cf-4f1b-a6a0-94847ea1da8f X-OriginalArrivalTime: 03 Jul 2003 14:39:08.0087 (UTC) FILETIME=[DC12D470:01C34170] X-Spam-Rating: daedalus.apache.org 1.6.2 0/1000/N --------------000208020409070408070200 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit No one seems to have bitten on this patch... I have to admit that I don't really like modifying server_rec and adding another configuration directive either. Would the group be more amenable to a patch that prevented the server from returning a 304 (Not Modified) code when a Set-Cookie header is present? This would reduce (hopefully slightly) the effectiveness of the browser cache, but only when the server has a Set-Cookie header to send. This should address the concern raised that cached page contents and cookie values remain in-synch. Ryan Eberhard Ryan Eberhard wrote: > I noticed one minor correction. In the comment line for the change to > httpd.h I transposed "304" with "403". The comment line should read: > >"/** Allow SetCookie header on HTTP Not Modified (304) status? */" > > > Ryan Eberhard wrote: > >> Attached is a patch to add a configuration directive to control >> whether the server is allowed to issue Set-Cookie headers when the >> HTTP status is 304 (Not Modified). >> >> Files changed: >> http-2.0/include/httpd.h -- Added allow_setcookie_on_not_modfied >> member to server_rec >> http-2.0/server/config.c -- Initialization of new member to 0 to >> preserve current behavior >> http-2.0/modules/http/http_core.c -- Define directive and set...() >> method >> http-2.0/modules/http/http_protocol.c -- Emit Set-Cookie header if >> status is 304 and directive allows >> >> Tests (performed with sniffer): >> Status 200, directive missing -> Set-Cookie processed >> Status 304, directive missing -> Set-Cookie ignored >> Status 200, directive set to "Off" -> Set-Cookie processed >> Status 304, directive set to "Off" -> Set-Cookie ignored >> Status 200, directive set to "On" -> Set-Cookie processed >> Status 304, directive set to "On" -> Set-Cookie processed >> >> I didn't see the source for the online documentation, e.g. "Directive >> Index" and "Apache Core Features" (with the list of configuration >> directives). If someone would please point me to that source base, I >> will gladly submit a patch for the documentation too. >> >> Ryan Eberhard wrote: >> >>>> --On Wednesday, June 4, 2003 11:33 AM -0400 Ryan Eberhard >>>> wrote: >>>> >>>> > I would appreciate the compromise where this behavior could be >>>> configured, >>>> > particularly if there is a way for a module to update the behavior >>>> > programmatically, e.g. without having to edit the configuration >>>> file. >>>> >>>> You are free to submit a patch that does this. -- justin >>> >>> >>> >>> Thanks. I will take this on. My initial thought is that this would >>> be configured at server level and there probably should be a >>> configuration directive, e.g. AllowSetCookieOnNotModified On | Off. >>> >>> I searched the site and did not see a document describing naming >>> conventions for directives. If there is one and someone could send >>> me the link, I would appreciate it. >>> >>> Ryan >>> >>> >> >>------------------------------------------------------------------------ >> >>--- httpd.h.old 2003-06-06 13:04:18.000000000 -0400 >>+++ httpd.h 2003-06-06 11:00:57.000000000 -0400 >>@@ -1111,6 +1111,9 @@ >> int limit_req_fieldsize; >> /** limit on number of request header fields */ >> int limit_req_fields; >>+ >>+ /** Allow SetCookie header on HTTP Not Modified (403) status? */ >>+ int allow_setcookie_on_not_modified; >> }; >> >> typedef struct core_output_filter_ctx { >> >> >>------------------------------------------------------------------------ >> >>--- config.c.old 2003-06-06 13:01:52.000000000 -0400 >>+++ config.c 2003-06-06 11:01:55.000000000 -0400 >>@@ -1722,7 +1722,9 @@ >> s->limit_req_line = main_server->limit_req_line; >> s->limit_req_fieldsize = main_server->limit_req_fieldsize; >> s->limit_req_fields = main_server->limit_req_fields; >>- >>+ >>+ s->allow_setcookie_on_not_modified = 0; >>+ >> *ps = s; >> >> return ap_parse_vhost_addrs(p, hostname, s); >> >> >>------------------------------------------------------------------------ >> >>--- http_core.c.old 2003-06-06 13:05:38.000000000 -0400 >>+++ http_core.c 2003-06-06 11:08:04.000000000 -0400 >>@@ -127,6 +127,18 @@ >> return NULL; >> } >> >>+static const char *set_allow_setcookie_on_not_modified(cmd_parms *cmd, >>+ void *dummy, int arg) >>+{ >>+ const char *err = ap_check_cmd_context(cmd, NOT_IN_DIR_LOC_FILE|NOT_IN_LIMIT); >>+ if (err != NULL) { >>+ return err; >>+ } >>+ >>+ cmd->server->allow_setcookie_on_not_modified = (arg != 0); >>+ return NULL; >>+} >>+ >> static const command_rec http_cmds[] = { >> AP_INIT_TAKE1("KeepAliveTimeout", set_keep_alive_timeout, NULL, RSRC_CONF, >> "Keep-Alive timeout duration (sec)"), >>@@ -134,6 +146,11 @@ >> "Maximum number of Keep-Alive requests per connection, or 0 for infinite"), >> AP_INIT_TAKE1("KeepAlive", set_keep_alive, NULL, RSRC_CONF, >> "Whether persistent connections should be On or Off"), >>+ AP_INIT_FLAG("AllowSetCookieOnNotModified", >>+ set_allow_setcookie_on_not_modified, >>+ NULL, RSRC_CONF, >>+ "Whether allowing Set-Cookie headers on HTTP Not \ >>+ Modified (304) status should be On or Off"), >> { NULL } >> }; >> >> >> >>------------------------------------------------------------------------ >> >>--- http_protocol.c.old 2003-06-06 13:05:39.000000000 -0400 >>+++ http_protocol.c 2003-06-06 13:08:38.000000000 -0400 >>@@ -1683,6 +1683,12 @@ >> "WWW-Authenticate", >> "Proxy-Authenticate", >> NULL); >>+ if (r->server->allow_setcookie_on_not_modified) { >>+ const char *sch = apr_table_get(r->headers_out, "Set-Cookie"); >>+ if (sch != NULL) { >>+ form_header_field(&h, "Set-Cookie", sch); >>+ } >>+ } >> } >> else { >> send_all_header_fields(&h, r); >> >> > --------------000208020409070408070200 Content-Type: text/html; charset=us-ascii Content-Transfer-Encoding: 7bit No one seems to have bitten on this patch...  I have to admit that I don't really like modifying server_rec and adding another configuration directive either.

Would the group be more amenable to a patch that prevented the server from returning a 304 (Not Modified) code when a Set-Cookie header is present?  This would reduce (hopefully slightly) the effectiveness of the browser cache, but only when the server has a Set-Cookie header to send.  This should address the concern raised that cached page contents and cookie values remain in-synch.

Ryan Eberhard

Ryan Eberhard wrote:
I noticed one minor correction.  In the comment line for the change to httpd.h I transposed "304" with "403".  The comment line should read: 
"/** Allow SetCookie header on HTTP Not Modified (304) status? */"

Ryan Eberhard wrote:
Attached is a patch to add a configuration directive to control whether the server is allowed to issue Set-Cookie headers when the HTTP status is 304 (Not Modified).

Files changed:
http-2.0/include/httpd.h -- Added allow_setcookie_on_not_modfied member to server_rec
http-2.0/server/config.c -- Initialization of new member to 0 to preserve current behavior
http-2.0/modules/http/http_core.c -- Define directive and set...() method
http-2.0/modules/http/http_protocol.c -- Emit Set-Cookie header if status is 304 and directive allows

Tests (performed with sniffer):
Status 200, directive missing -> Set-Cookie processed
Status 304, directive missing -> Set-Cookie ignored
Status 200, directive set to "Off" -> Set-Cookie processed
Status 304, directive set to "Off" -> Set-Cookie ignored
Status 200, directive set to "On" -> Set-Cookie processed
Status 304, directive set to "On" -> Set-Cookie processed

I didn't see the source for the online documentation, e.g. "Directive Index" and "Apache Core Features" (with the list of configuration directives).  If someone would please point me to that source base, I will gladly submit a patch for the documentation too.

Ryan Eberhard wrote:

--On Wednesday, June 4, 2003 11:33 AM -0400 Ryan Eberhard <ryan.eberhard@entegrity.com> wrote:

> I would appreciate the compromise where this behavior could be configured,
> particularly if there is a way for a module to update the behavior
> programmatically, e.g. without having to edit the configuration file.

You are free to submit a patch that does this.  -- justin


Thanks.  I will take this on.  My initial thought is that this would be configured at server level and there probably should be a configuration directive, e.g. AllowSetCookieOnNotModified On | Off.

I searched the site and did not see a document describing naming conventions for directives.  If there is one and someone could send me the link, I would appreciate it.

Ryan 




--- httpd.h.old	2003-06-06 13:04:18.000000000 -0400
+++ httpd.h	2003-06-06 11:00:57.000000000 -0400
@@ -1111,6 +1111,9 @@
     int limit_req_fieldsize;
     /** limit on number of request header fields  */
     int limit_req_fields; 
+    
+    /** Allow SetCookie header on HTTP Not Modified (403) status? */
+    int allow_setcookie_on_not_modified;
 };
 
 typedef struct core_output_filter_ctx {
  

--- config.c.old	2003-06-06 13:01:52.000000000 -0400
+++ config.c	2003-06-06 11:01:55.000000000 -0400
@@ -1722,7 +1722,9 @@
     s->limit_req_line = main_server->limit_req_line;
     s->limit_req_fieldsize = main_server->limit_req_fieldsize;
     s->limit_req_fields = main_server->limit_req_fields;
-
+	
+    s->allow_setcookie_on_not_modified = 0;
+    
     *ps = s;
 
     return ap_parse_vhost_addrs(p, hostname, s);
  

--- http_core.c.old	2003-06-06 13:05:38.000000000 -0400
+++ http_core.c	2003-06-06 11:08:04.000000000 -0400
@@ -127,6 +127,18 @@
     return NULL;
 }
 
+static const char *set_allow_setcookie_on_not_modified(cmd_parms *cmd, 
+                                                       void *dummy, int arg)
+{
+    const char *err = ap_check_cmd_context(cmd, NOT_IN_DIR_LOC_FILE|NOT_IN_LIMIT);
+    if (err != NULL) {
+        return err;
+    }
+
+    cmd->server->allow_setcookie_on_not_modified = (arg != 0);
+    return NULL;
+}
+
 static const command_rec http_cmds[] = {
     AP_INIT_TAKE1("KeepAliveTimeout", set_keep_alive_timeout, NULL, RSRC_CONF,
                   "Keep-Alive timeout duration (sec)"),
@@ -134,6 +146,11 @@
      "Maximum number of Keep-Alive requests per connection, or 0 for infinite"),
     AP_INIT_TAKE1("KeepAlive", set_keep_alive, NULL, RSRC_CONF,
                   "Whether persistent connections should be On or Off"),
+    AP_INIT_FLAG("AllowSetCookieOnNotModified", 
+                 set_allow_setcookie_on_not_modified, 
+                 NULL, RSRC_CONF,
+                 "Whether allowing Set-Cookie headers on HTTP Not \
+                 Modified (304) status should be On or Off"),
     { NULL }
 };
 
  

--- http_protocol.c.old	2003-06-06 13:05:39.000000000 -0400
+++ http_protocol.c	2003-06-06 13:08:38.000000000 -0400
@@ -1683,6 +1683,12 @@
                      "WWW-Authenticate",
                      "Proxy-Authenticate",
                      NULL);
+        if (r->server->allow_setcookie_on_not_modified) {
+            const char *sch = apr_table_get(r->headers_out, "Set-Cookie");
+            if (sch != NULL) {
+                form_header_field(&h, "Set-Cookie", sch);
+            }
+        }
     }
     else {
         send_all_header_fields(&h, r);


--------------000208020409070408070200--