Paul J. Reder wrote:
> I just have a quick comment based on some work I did recently.
> You should check for the ETag value in both headers_out locations.
> It is actually a bit more likely that the ETag will be in
> r->err_headers_out
> rather than r->headers_out, but it could be in either.
hmm, I had thought about that. ap_meets_conditions only checks headers_out,
so I figured it was safe.
but you bring up an interesting point. outside of this patch, if somebody
manually populates an ETag header in err_headers_out then it won't be caught
by the "no-etag" stuff we already depend on. I'll account for that as well.
> Your patch should
> look in both locations and stick the altered value back where you found it.
yes, good idea. and it brings up another good point. the weaken API only
worked for tags generated with ap_make_etag - manual ETag entries could pass
through to the client unaltered if they happen after the new ap_weaken_etag
call. so, some additional logic is required in the header filter to take
care of those cases.
>
> The other comment I have is that in the ap_weaken_etag function you call
> etag = apr_table_get(r->headers_out, "ETag") twice (once in the decl/init
> and once in the conditional).
drat. refactoring leakage :)
>
> Other than that, I'm not enough of an expert on tags and weakness...
> from what
> I do know this looks good, but some doc for the function might be useful to
> help folks know when they might be legally able to weaken a tag.
sure. I placed a brief bit and a pointer to the docs in httpd.h, in
addition to expanding the comments in the weaken function more. is there
someplace else that's appropriate?
thanks for your input - new patch attached.
--Geoff
Index: include/http_protocol.h
===================================================================
RCS file: /home/cvspublic/httpd-2.0/include/http_protocol.h,v
retrieving revision 1.84
diff -u -r1.84 http_protocol.h
--- include/http_protocol.h 3 Feb 2003 17:52:53 -0000 1.84
+++ include/http_protocol.h 12 Dec 2003 19:03:00 -0000
@@ -196,11 +196,28 @@
AP_DECLARE(char *) ap_make_etag(request_rec *r, int force_weak);
/**
- * Set the E-tag outgoing header
+ * Set the ETag outgoing header
* @param The current request
* @deffunc void ap_set_etag(request_rec *r)
*/
AP_DECLARE(void) ap_set_etag(request_rec *r);
+
+/**
+ * Supress the ETag outgoing header
+ * @param The current request
+ * @deffunc void ap_supress_etag(request_rec *r)
+ */
+AP_DECLARE(void) ap_supress_etag(request_rec *r);
+
+/**
+ * Force generation of a weak ETag header.
+ * weak tags are allowed when the resource is semantically
+ * equivalent to a prior version, even though the content
+ * bits are different. see RFC-2616 13.3.3
+ * @param The current request
+ * @deffunc void ap_weaken_etag(request_rec *r)
+ */
+AP_DECLARE(void) ap_weaken_etag(request_rec *r);
/**
* Set the last modified time for the file being sent
Index: modules/filters/mod_include.c
===================================================================
RCS file: /home/cvspublic/httpd-2.0/modules/filters/mod_include.c,v
retrieving revision 1.292
diff -u -r1.292 mod_include.c
--- modules/filters/mod_include.c 10 Dec 2003 02:30:20 -0000 1.292
+++ modules/filters/mod_include.c 12 Dec 2003 19:03:14 -0000
@@ -3584,7 +3584,7 @@
* We don't know if we are going to be including a file or executing
* a program - in either case a strong ETag header will likely be invalid.
*/
- apr_table_setn(f->r->notes, "no-etag", "");
+ ap_supress_etag(f->r);
return OK;
}
Index: modules/http/http_protocol.c
===================================================================
RCS file: /home/cvspublic/httpd-2.0/modules/http/http_protocol.c,v
retrieving revision 1.473
diff -u -r1.473 http_protocol.c
--- modules/http/http_protocol.c 16 Nov 2003 02:09:13 -0000 1.473
+++ modules/http/http_protocol.c 12 Dec 2003 19:03:18 -0000
@@ -1631,9 +1631,22 @@
/*
* Now remove any ETag response header field if earlier processing
* says so (such as a 'FileETag None' directive).
+ *
+ * be sure to clear err_headers_out, in case some non-core module
+ * populates it manually.
*/
if (apr_table_get(r->notes, "no-etag") != NULL) {
apr_table_unset(r->headers_out, "ETag");
+ apr_table_unset(r->err_headers_out, "ETag");
+ }
+
+ /* speaking of err_headers_out, if an ETag is added to either
+ * table manually, the ap_make_etag/ap_weaken_etag link is
+ * broken. do one final sweep to make sure we weaken the ETag
+ * no matter where it originated
+ */
+ if (apr_table_get(r->notes, "weak-etag") != NULL) {
+ ap_weaken_etag(r);
}
/* determine the protocol and whether we should use keepalives. */
@@ -2693,7 +2706,7 @@
* note it for the header-sender to ignore.
*/
if (etag_bits & ETAG_NONE) {
- apr_table_setn(r->notes, "no-etag", "omit");
+ ap_supress_etag(r);
return "";
}
@@ -2711,7 +2724,13 @@
* we send a weak tag instead of a strong one, since it could
* be modified again later in the second, and the validation
* would be incorrect.
+ *
+ * additional criteria for a weak tag is if force_weak is true
+ * or ap_weaken_etag() was called
*/
+
+ force_weak = (force_weak || apr_table_get(r->notes, "weak-etag"));
+
if ((r->request_time - r->mtime > (1 * APR_USEC_PER_SEC)) &&
!force_weak) {
weak = NULL;
@@ -2831,6 +2850,52 @@
}
apr_table_setn(r->headers_out, "ETag", etag);
+}
+
+/*
+ * Take steps to ensure that current and future-generated ETag
+ * headers are marked as weak.
+ * strong tags need to be unique for every version of a resource
+ * that has ever existed. weak tags are acceptable whenever the
+ * the resource is has altered but it's meaning has not, when
+ * the bits have changed but the content is semantically equivalent.
+ */
+AP_DECLARE(void) ap_weaken_etag(request_rec *r)
+{
+ const char *etag;
+
+ /* if the ETag header exists in either the headers_out or
+ * err_headers_out table and is not already marked
+ * as weak, mark it now
+ */
+ if ((etag = apr_table_get(r->headers_out, "ETag")) &&
+ strncmp(etag, ETAG_WEAK, strlen(ETAG_WEAK))) {
+
+ etag = apr_pstrcat(r->pool, ETAG_WEAK, etag, NULL);
+ apr_table_setn(r->headers_out, "ETag", etag);
+ }
+
+ if ((etag = apr_table_get(r->err_headers_out, "ETag")) &&
+ strncmp(etag, ETAG_WEAK, strlen(ETAG_WEAK))) {
+
+ etag = apr_pstrcat(r->pool, ETAG_WEAK, etag, NULL);
+ apr_table_setn(r->err_headers_out, "ETag", etag);
+ }
+
+ /* make sure future calls to ap_make_etag() produce weak tags
+ * note that this does not prevent people from manually adding
+ * their own strong ETag headers. this is taken care of later
+ * in the header filter
+ */
+ apr_table_setn(r->notes, "weak-etag", "yes");
+}
+
+/*
+ * Formalize ETag header supression.
+ */
+AP_DECLARE(void) ap_supress_etag(request_rec *r)
+{
+ apr_table_setn(r->notes, "no-etag", "omit");
}
static int parse_byterange(char *range, apr_off_t clength,
