You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Sander van Zoest <sa...@covalent.net> on 2000/12/06 23:55:36 UTC
[PATCH] Re: mod_log_config: Cookie logging patch 1.3.X
On Mon, 20 Nov 2000, Sander van Zoest wrote:
> On Fri, 17 Nov 2000, Sander van Zoest wrote:
> > in <20...@sung4.rd.bbc.co.uk>,
> > <http://archive.covalent.net/new-httpd/2000/08/0036.xml> Erik Meyer
> > posted a patch to mod_log_config that would use the %{cookie}c to log
> > specific cookies. PR#6418 has a similar patch.
> Below find the patch for HEAD + docs for apache 1.3 and 2.0 to add this
> functionality. Let me know what else you need to get this committed.
The new %{FOOBAR}C cookie functionality got added to 2.0, but most people
would want this functionality for 1.3.15 when it comes out.
I understand the reasoning why not to include a new "feature" in 1.3,
but meanwhile %c is also new for 1.3.15, so that pretty much kills the
argument for me. :-)
Cheers,
--
Sander van Zoest [sander@covalent.net]
Covalent Technologies, Inc. http://www.covalent.net/
(415) 536-5218 http://www.vanzoest.com/sander/
Index: apache-1.3/src/modules/standard/mod_log_config.c
===================================================================
RCS file: /work/cvs/root/asf/httpd/apache-1.3/src/modules/standard/mod_log_config.c,v
retrieving revision 1.85
diff -u -r1.85 mod_log_config.c
--- apache-1.3/src/modules/standard/mod_log_config.c 2000/11/14 09:57:22 1.85
+++ apache-1.3/src/modules/standard/mod_log_config.c 2000/11/18 02:59:48
@@ -124,6 +124,7 @@
* 'X' = connection aborted before the response completed.
* '+' = connection may be kept alive after the response is sent.
* '-' = connection will be closed after the response is sent.
+ * %...{FOOBAR}C: The contents of the HTTP cookie FOOBAR
* %...{FOOBAR}e: The contents of the environment variable FOOBAR
* %...f: filename
* %...h: remote host
@@ -412,6 +413,26 @@
return ap_table_get(r->subprocess_env, a);
}
+static const char *log_cookie(request_rec *r, char *a)
+{
+ const char *cookies;
+ char *start_cookie;
+
+ if((cookies = ap_table_get(r->headers_in, "Cookie")))
+ if(( start_cookie = strstr(cookies,a)))
+ {
+ char *cookie, *end_cookie;
+ start_cookie += strlen(a) + 1; /* cookie_name + '=' */
+ cookie = ap_pstrdup(r->pool, start_cookie);
+ /* kill everything in cookie after ';' */
+ end_cookie = strchr(cookie, ';');
+ if( end_cookie )
+ *end_cookie = '\0';
+ return cookie;
+ }
+ return NULL;
+}
+
static const char *log_request_time(request_rec *r, char *a)
{
int timz;
@@ -566,6 +587,9 @@
},
{
'c', log_connection_status, 0
+ },
+ {
+ 'C', log_cookie, 0
},
{
'\0'
Index: docs-1.3/htdocs/manual/mod/mod_log_config.html
===================================================================
RCS file: /work/cvs/root/asf/httpd/docs-1.3/htdocs/manual/mod/mod_log_config.html,v
retrieving revision 1.40
diff -u -r1.40 mod_log_config.html
--- docs-1.3/htdocs/manual/mod/mod_log_config.html 2000/11/13 02:01:35 1.40
+++ docs-1.3/htdocs/manual/mod/mod_log_config.html 2000/11/20 20:55:34
@@ -159,6 +159,8 @@
'X' = connection aborted before the response completed.
'+' = connection may be kept alive after the response is sent.
'-' = connection will be closed after the response is sent.
+%...{Foobar}C: The contents of cookie "Foobar" in the request sent to the
+ server.
%...{FOOBAR}e: The contents of the environment variable FOOBAR
%...f: Filename
%...h: Remote host
Re: [PATCH] Re: mod_log_config: Cookie logging patch 1.3.X
Posted by Martin Pool <mb...@linuxcare.com.au>.
> The parsing used in that patch works and is pretty similar to code
> used in mod_usertrack as you pointed out.
>
> The code is fine, but if we want to modularize it and create something
> similar to ap_get_token then thats fine with me also.
Well, I would suggest that actually it does not work properly. If the
cookie name is 'a' and there happens to be an 'a' character earlier in
the header, then I'm pretty sure that routine will return a bogus
result. It doesn't check that the match is actually a complete cookie
name. So you can either fix it now, or wait for somebody to encounter
the problem and submit it as a bug.
--
Martin Pool, Linuxcare, Inc.
+61 2 6262 8990
mbp@linuxcare.com, http://www.linuxcare.com/
Linuxcare. Support for the revolution.
Re: [PATCH] Re: mod_log_config: Cookie logging patch 1.3.X
Posted by Sander van Zoest <sa...@covalent.net>.
On Fri, 8 Dec 2000, Martin Pool wrote:
> On 7 Dec 2000, rbb@covalent.net wrote:
> > > Would it help if I made a parallel patch to 2.0? (Assuming this
> > > feature is not already there.)
> > Sander's original patch was added to 2.0 a few weeks ago.
> Right, but as Sander agreed yesterday, the parsing it uses is not
> really correct. So I was proposing to change it to use something
> similar to ap_get_token.
The parsing used in that patch works and is pretty similar to code
used in mod_usertrack as you pointed out.
The code is fine, but if we want to modularize it and create something
similar to ap_get_token then thats fine with me also.
Ultimately I just want the functionality to exist, in the best possible
manner, with the least amount of work on everyone. ;-)
--
Sander van Zoest [sander@covalent.net]
Covalent Technologies, Inc. http://www.covalent.net/
(415) 536-5218 http://www.vanzoest.com/sander/
Re: [PATCH] Re: mod_log_config: Cookie logging patch 1.3.X
Posted by Martin Pool <mb...@linuxcare.com.au>.
On 7 Dec 2000, rbb@covalent.net wrote:
>
> > Would it help if I made a parallel patch to 2.0? (Assuming this
> > feature is not already there.)
>
> Sander's original patch was added to 2.0 a few weeks ago.
Right, but as Sander agreed yesterday, the parsing it uses is not
really correct. So I was proposing to change it to use something
similar to ap_get_token.
--
Martin Pool, Linuxcare, Inc.
+61 2 6262 8990
mbp@linuxcare.com, http://www.linuxcare.com/
Linuxcare. Support for the revolution.
Re: [PATCH] Re: mod_log_config: Cookie logging patch 1.3.X
Posted by rb...@covalent.net.
> Would it help if I made a parallel patch to 2.0? (Assuming this
> feature is not already there.)
Sander's original patch was added to 2.0 a few weeks ago.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [PATCH] Re: mod_log_config: Cookie logging patch 1.3.X
Posted by Sander van Zoest <sa...@covalent.net>.
On Fri, 8 Dec 2000, Martin Pool wrote:
> On 7 Dec 2000, Greg Stein <gs...@lyra.org> wrote:
> Would it help if I made a parallel patch to 2.0? (Assuming this
> feature is not already there.)
As I stated at the beginning of this thread. The original patch that
was written has been commited to apache 2.0 already.
--
Sander van Zoest [sander@covalent.net]
Covalent Technologies, Inc. http://www.covalent.net/
(415) 536-5218 http://www.vanzoest.com/sander/
Re: [PATCH] Re: mod_log_config: Cookie logging patch 1.3.X
Posted by Martin Pool <mb...@linuxcare.com.au>.
On 7 Dec 2000, Greg Stein <gs...@lyra.org> wrote:
> I'm -0 on adding features to 1.3 with or without the token stuff.
Considering the large changes, my sense that it will be late in the
new year before the majority of sites have migrated to 2.0. I can
understand (at least some of) the engineering reasons to keep 1.3
stabilized, and to put effort into 2.0. But at the same time it seems
a little kind of cruel to not do anything new for people still using
1.3.
Would it help if I made a parallel patch to 2.0? (Assuming this
feature is not already there.)
--
Martin Pool, Linuxcare, Inc.
+61 2 6262 8990
mbp@linuxcare.com, http://www.linuxcare.com/
Linuxcare. Support for the revolution.
Re: [PATCH] Re: mod_log_config: Cookie logging patch 1.3.X
Posted by Greg Stein <gs...@lyra.org>.
On Fri, Dec 08, 2000 at 10:38:28AM +1100, Martin Pool wrote:
> On 7 Dec 2000, Greg Stein <gs...@lyra.org> wrote:
>...
> > -0 on this patch
>
> If I put ap_get_token back to the old behaviour, add a new function to
> really read tokens, and patch the ap_get_token documentation will that
> be OK? Or do you not want to add this feature in 1.3?
I'm -0 on adding features to 1.3 with or without the token stuff.
If ap_get_token() is put back and a new function introduced, then I'd remove
my veto on that *portion*, but overall still would state "-0".
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: [PATCH] Re: mod_log_config: Cookie logging patch 1.3.X
Posted by Martin Pool <mb...@linuxcare.com.au>.
On 7 Dec 2000, Greg Stein <gs...@lyra.org> wrote:
> -1 (veto) on the ap_get_token() changes. The modified code stops on
> characters other than "," and ";" (change of behavior).
That's very true -- sorry. The mod_negotiation code requires that
this function actually *not* retrieve tokens, according to the RFC
grammar for "token". Specifically "text/html" is two tokens with a
slash in between, but mod_negotiation thinks it is just one.
> -0 on this patch
If I put ap_get_token back to the old behaviour, add a new function to
really read tokens, and patch the ap_get_token documentation will that
be OK? Or do you not want to add this feature in 1.3?
--
Martin Pool, Linuxcare, Inc.
+61 2 6262 8990
mbp@linuxcare.com, http://www.linuxcare.com/
Linuxcare. Support for the revolution.
Re: [PATCH] Re: mod_log_config: Cookie logging patch 1.3.X
Posted by Greg Stein <gs...@lyra.org>.
-0 on this patch
-1 (veto) on the ap_get_token() changes. The modified code stops on
characters other than "," and ";" (change of behavior).
Cheers,
-g
On Thu, Dec 07, 2000 at 11:52:58AM +1100, Martin Pool wrote:
>...
> /* Retrieve a token, spacing over it and returning a pointer to
> * the first non-white byte afterwards. Note that these tokens
> * are delimited by semis and commas; and can also be delimited
> * by whitespace at the caller's option.
> - */
> -
> + *
> + * Strictly, HTTP tokens cannot be quoted or contain whitespace. But
> + * accepting this makes us more forgiving in what we accept, and
> + * allows this function also to be used to read quoted strings. */
> API_EXPORT(char *) ap_get_token(pool *p, const char **accept_line, int accept_
> white)
> {
> const char *ptr = *accept_line;
> @@ -1368,12 +1418,17 @@
> * (comments are already gone).
> */
>
> - while (*ptr && (accept_white || !ap_isspace(*ptr))
> - && *ptr != ';' && *ptr != ',') {
> - if (*ptr++ == '"')
> + while (*ptr) {
> + if (*ptr++ == '"') {
> while (*ptr)
> if (*ptr++ == '"')
> break;
> + } else if (accept_white && ap_isspace(*ptr)) {
> + /* not stricly part of a token, but the caller wants to accept it a
> nyhow */
> + ;
> + } else if (TEST_CHAR(*ptr, T_HTTP_TOKEN_STOP)) {
> + break;
> + }
> }
>...
--
Greg Stein, http://www.lyra.org/
Re: [PATCH] Re: mod_log_config: Cookie logging patch 1.3.X
Posted by Sander van Zoest <sa...@covalent.net>.
On Thu, 7 Dec 2000, Martin Pool wrote:
> On 7 Dec 2000, Martin Pool <mb...@linuxcare.com.au> wrote:
> > > I understand the reasoning why not to include a new "feature" in 1.3,
> > > but meanwhile %c is also new for 1.3.15, so that pretty much kills the
> > > argument for me. :-)
> Can we please use this patch instead? It's parsing of the header is
> much closer to the RFC.
> http://linuxcare.com.au/mbp/apache/apache-1.3.15dev-cookielog.patch
+1
--
Sander van Zoest [sander@covalent.net]
Covalent Technologies, Inc. http://www.covalent.net/
(415) 536-5218 http://www.vanzoest.com/sander/
Re: [PATCH] Re: mod_log_config: Cookie logging patch 1.3.X
Posted by Martin Pool <mb...@linuxcare.com.au>.
On 7 Dec 2000, Martin Pool <mb...@linuxcare.com.au> wrote:
> > I understand the reasoning why not to include a new "feature" in 1.3,
> > but meanwhile %c is also new for 1.3.15, so that pretty much kills the
> > argument for me. :-)
Can we please use this patch instead? It's parsing of the header is
much closer to the RFC.
http://linuxcare.com.au/mbp/apache/apache-1.3.15dev-cookielog.patch
--
Martin Pool, Linuxcare, Inc.
+61 2 6262 8990
mbp@linuxcare.com, http://www.linuxcare.com/
Linuxcare. Support for the revolution.
This patch adds a new log format %{FOO}C, which logs the received
cookie FOO, if any. It also adds a utility function to parse HTTP
attribute-value headers, and uses that code from mod_usertrack.c.
--
Martin Pool
Index: src/include/httpd.h
===================================================================
RCS file: /home/cvspublic/apache-1.3/src/include/httpd.h,v
retrieving revision 1.327
diff -u -r1.327 httpd.h
--- src/include/httpd.h 2000/11/14 09:57:01 1.327
+++ src/include/httpd.h 2000/12/07 00:42:41
@@ -1000,6 +1000,11 @@
API_EXPORT(char *) ap_get_list_item(pool *p, const char **field);
API_EXPORT(int) ap_find_list_item(pool *p, const char *line, const char *tok);
+API_EXPORT(int) ap_get_attr_value(pool *p, char const **line,
+ char const *wanted, char **valuep);
+API_EXPORT(int) ap_get_av_pair(pool *p, char const **lineptr,
+ char **attr, char **value);
+
API_EXPORT(char *) ap_get_token(pool *p, const char **accept_line, int accept_
white);
API_EXPORT(int) ap_find_token(pool *p, const char *line, const char *tok);
API_EXPORT(int) ap_find_last_token(pool *p, const char *line, const char *tok)
;
Index: src/main/util.c
===================================================================
RCS file: /home/cvspublic/apache-1.3/src/main/util.c,v
retrieving revision 1.192
diff -u -r1.192 util.c
--- src/main/util.c 2000/12/04 20:51:59 1.192
+++ src/main/util.c 2000/12/07 00:42:43
@@ -1344,12 +1344,62 @@
}
+/* Retrieve the value of a specified attribute from an av-list.
+ * Attribute names are case-insensitive.
+ *
+ * Returns true if the attribute was found. Even in this case VALUEP
+ * may still be null if the attribute was present with no value. */
+API_EXPORT(int) ap_get_attr_value(pool *p, char const **line,
+ char const *wanted,
+ char **valuep)
+{
+ char *name;
+
+ /* XXX: This will allocate copies of every item it scans, which is
+ * kind of inefficient. But the headers should never be huge, and
+ * you know what they say about premature optimization. */
+
+ while (ap_get_av_pair(p, line, &name, valuep)) {
+ if (!strcasecmp(name, wanted))
+ return 1;
+ }
+
+ *valuep = NULL;
+ return 0;
+}
+
+
+/* Retrieve an ATTRIBUTE=VALUE pair from an av-list. (See RFC2068 and
+ * RFC2109 for the definition.) The value may be missing, although
+ * most attributes require it. Returns true if anything was found. */
+API_EXPORT(int) ap_get_av_pair(pool *p, char const **lineptr,
+ char **attr, char **value)
+{
+ /* Read the first token, being the attribute. */
+ *attr = ap_get_token(p, lineptr, 0);
+ if (!**attr)
+ return 0;
+
+ /* We've already scanned trailing whitespace. Now try to scan the
+ * '=' character. */
+ if (**lineptr != '=')
+ return 1;
+
+ (*lineptr)++;
+
+ *value = ap_get_token(p, lineptr, 0);
+ return 1;
+}
+
+
/* Retrieve a token, spacing over it and returning a pointer to
* the first non-white byte afterwards. Note that these tokens
* are delimited by semis and commas; and can also be delimited
* by whitespace at the caller's option.
- */
-
+ *
+ * Strictly, HTTP tokens cannot be quoted or contain whitespace. But
+ * accepting this makes us more forgiving in what we accept, and
+ * allows this function also to be used to read quoted strings. */
API_EXPORT(char *) ap_get_token(pool *p, const char **accept_line, int accept_
white)
{
const char *ptr = *accept_line;
@@ -1368,12 +1418,17 @@
* (comments are already gone).
*/
- while (*ptr && (accept_white || !ap_isspace(*ptr))
- && *ptr != ';' && *ptr != ',') {
- if (*ptr++ == '"')
+ while (*ptr) {
+ if (*ptr++ == '"') {
while (*ptr)
if (*ptr++ == '"')
break;
+ } else if (accept_white && ap_isspace(*ptr)) {
+ /* not stricly part of a token, but the caller wants to accept it a
nyhow */
+ ;
+ } else if (TEST_CHAR(*ptr, T_HTTP_TOKEN_STOP)) {
+ break;
+ }
}
tok_len = ptr - tok_start;
Index: src/modules/standard/mod_log_config.c
===================================================================
RCS file: /home/cvspublic/apache-1.3/src/modules/standard/mod_log_config.c,v
retrieving revision 1.85
diff -u -r1.85 mod_log_config.c
--- src/modules/standard/mod_log_config.c 2000/11/14 09:57:22 1.85
+++ src/modules/standard/mod_log_config.c 2000/12/07 00:42:45
@@ -124,13 +124,14 @@
* 'X' = connection aborted before the response completed.
* '+' = connection may be kept alive after the response is sent.
* '-' = connection will be closed after the response is sent.
+ * %...{FU}C: Value of cookie FU received in request
* %...{FOOBAR}e: The contents of the environment variable FOOBAR
* %...f: filename
* %...h: remote host
* %...a: remote IP-address
* %...A: local IP-address
* %...{Foobar}i: The contents of Foobar: header line(s) in the request
- * sent to the client.
+ * sent by the client.
* %...l: remote logname (from identd, if supplied)
* %...{Foobar}n: The contents of note "Foobar" from another module.
* %...{Foobar}o: The contents of Foobar: header line(s) in the reply.
@@ -481,6 +482,29 @@
return "-";
}
+static const char *log_cookie(request_rec *r, char *wanted)
+{
+ char *cookie, *value;
+
+ /* Apache does not keep cookies parsed out, so we must scan
+ * through the request headers to find matches. Although it's
+ * possible that there might be multiple cookies of the same name
+ * for different domains that would be a pretty crazy design, so
+ * we don't handle it at the moment. */
+ char const *hdr = ap_table_get(r->headers_in, "Cookie");
+
+ if (!hdr)
+ return NULL;
+
+ if (ap_get_attr_value(r->pool, &hdr, wanted, &value))
+ return value;
+ else
+ return NULL;
+}
+
+
+
+
/*****************************************************************
*
* Parsing the log format string
@@ -563,6 +587,9 @@
},
{
'q', log_request_query, 0
+ },
+ {
+ 'C', log_cookie, 0
},
{
'c', log_connection_status, 0
Index: src/modules/standard/mod_usertrack.c
===================================================================
RCS file: /home/cvspublic/apache-1.3/src/modules/standard/mod_usertrack.c,v
retrieving revision 1.46
diff -u -r1.46 mod_usertrack.c
--- src/modules/standard/mod_usertrack.c 2000/11/14 09:57:28 1.46
+++ src/modules/standard/mod_usertrack.c 2000/12/07 00:42:45
@@ -230,20 +230,13 @@
}
if ((cookie = ap_table_get(r->headers_in, "Cookie")))
- if ((value = strstr(cookie, dcfg->cookie_name))) {
- char *cookiebuf, *cookieend;
-
- value += strlen(dcfg->cookie_name) + 1; /* Skip over the '=' */
- cookiebuf = ap_pstrdup(r->pool, value);
- cookieend = strchr(cookiebuf, ';');
- if (cookieend)
- *cookieend = '\0'; /* Ignore anything after a ; */
-
+ if (ap_get_attr_value(r->pool, &cookie, dcfg->cookie_name, &value)) {
/* Set the cookie in a note, for logging */
- ap_table_setn(r->notes, "cookie", cookiebuf);
+ ap_table_setn(r->notes, "cookie", value);
return DECLINED; /* There's already a cookie, no new one */
}
+
make_cookie(r);
return OK; /* We set our cookie */
}
Re: [PATCH] Re: mod_log_config: Cookie logging patch 1.3.X
Posted by Sander van Zoest <sa...@covalent.net>.
On Thu, 7 Dec 2000, Martin Pool wrote:
> > > Below find the patch for HEAD + docs for apache 1.3 and 2.0 to add this
> > > functionality. Let me know what else you need to get this committed.
> > The new %{FOOBAR}C cookie functionality got added to 2.0, but most people
> > would want this functionality for 1.3.15 when it comes out.
> I'd also like to see this go into 1.3, but the quoted code is a very
> dodgy way of parsing an attribute-value list. Almost all the code to
> do the parsing properly is already present in ap_get_token etc.
Ah, wasn't aware of that. Seems like the only code using ap_get_token
is mod_negotiation, so I'd never seen it before. I will work on a
new patch using ap_get_token and post it again.
Thanks for the pointer,
--
Sander van Zoest [sander@covalent.net]
Covalent Technologies, Inc. http://www.covalent.net/
(415) 536-5218 http://www.vanzoest.com/sander/
Re: [PATCH] Re: mod_log_config: Cookie logging patch 1.3.X
Posted by Martin Pool <mb...@linuxcare.com.au>.
On 6 Dec 2000, Sander van Zoest <sa...@covalent.net> wrote:
> On Mon, 20 Nov 2000, Sander van Zoest wrote:
> > On Fri, 17 Nov 2000, Sander van Zoest wrote:
> > > in <20...@sung4.rd.bbc.co.uk>,
> > > <http://archive.covalent.net/new-httpd/2000/08/0036.xml> Erik Meyer
> > > posted a patch to mod_log_config that would use the %{cookie}c to log
> > > specific cookies. PR#6418 has a similar patch.
> > Below find the patch for HEAD + docs for apache 1.3 and 2.0 to add this
> > functionality. Let me know what else you need to get this committed.
>
> The new %{FOOBAR}C cookie functionality got added to 2.0, but most people
> would want this functionality for 1.3.15 when it comes out.
>
> I understand the reasoning why not to include a new "feature" in 1.3,
> but meanwhile %c is also new for 1.3.15, so that pretty much kills the
> argument for me. :-)
I'd also like to see this go into 1.3, but the quoted code is a very
dodgy way of parsing an attribute-value list. Almost all the code to
do the parsing properly is already present in ap_get_token etc.
--
Martin Pool, Linuxcare, Inc.
+61 2 6262 8990
mbp@linuxcare.com, http://www.linuxcare.com/
Linuxcare. Support for the revolution.