Re: [PATCH] Bug 18388: Set-Cookie header not honored on 304 (Not modified) status

[Ryan Eberhard <ry...@entegrity.com>]

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 
>>>> <ry...@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);
>>  
>>
>