Re: svn commit: r645160 - in /httpd/httpd/trunk: CHANGES docs/manual/mod/mod_session_dbd.xml modules/session/config.m4 modules/session/mod_session_dbd.c

[Graham Leggett]

Ruediger Pluem wrote:

+    /* load anonymous sessions */
+    if (conf->name_set || conf->name2_set) {
+
+        /* load RFC2109 compliant cookie */
+        if (conf->name_set) {
+            ap_cookie_read(r, conf->name, &key, conf->remove);
+        }
+
+        /* load RFC2965 compliant cookie */
+        if (!key && conf->name2_set) {
+            ap_cookie_read(r, conf->name2, &key, conf->remove);
+        }

and ap_cookie_read looks for both cookie types?

Why this? name is already set to the correct value (either conf->name or conf->name2)

Oops - this is code that should be have collapsed after a bugfix. Will fix.

+        /* don't cache pages with a session */
+        apr_table_addn(r->headers_out, "Cache-Control", "no-cache");

Why don't we cache these pages? Because of the cookie? We can avoid saving

give the user the chance to cache it.

the Set-Cookie header in the the cache by CacheIgnoreHeaders. IMHO we should

We cannot assume the Apache cache is the only cache in the chain. If a cache accidentally returns a page with the user's session cookie inside it, the new user will take over the old user's session and potentially their identity (if the session was used to contain an identity, which it probably will be). I understand that Vary potentially helps us here, I need to look deeper at this when my brain is clearer and I have a bit more time to look at this in more detail. This is why SessionInclude and SessionExclude were included, so there was an efficient mechanism to exclude image directories and other entities which you want cached, without needing to create complex <Directory> and <Location> sections, which may not be convenient or practical.

+    char *last;
+    char *line = apr_pstrdup(cmd->pool, args);
+    session_dbd_dir_conf *conf = (session_dbd_dir_conf *) config;
+    char *cookie = apr_strtok(line, " \t", &last);
+    conf->name = cookie;
+    conf->name_set = 1;
+    while (apr_isspace(*last)) {
+        last++;
+    }
+    conf->name_attrs = last;

So name_attrs may contain \t's. Is this intended?

Regards,
Graham
--

It is intended as a safety measure - if a user puts in a \t as a separator between the cookie and the attrs, that user will be in a world of pain trying to figure out why their cookie has a weird funny name, or why the server won't start because "the cookie contains the ';' character".

smime.p7s
Description: S/MIME Cryptographic Signature