Re: svn commit: r1837056 - in /httpd/httpd/trunk: ./ include/ modules/filters/ modules/http/ modules/http2/ modules/proxy/ modules/test/ server/
Evgeny Kotkov writes: >> +1 for the patch, I missed the separate 304 handling in >> mod_brotli/deflate, thanks for catching this! > > Thanks, I will commit it at the earliest opportunity. Committed and proposed for a backport to 2.4.x: https://svn.apache.org/r1843242 https://svn.apache.org/r1843245 Thanks, Evgeny Kotkov
Re: svn commit: r1837056 - in /httpd/httpd/trunk: ./ include/ modules/filters/ modules/http/ modules/http2/ modules/proxy/ modules/test/ server/
On Thu, Oct 4, 2018 at 7:20 PM William A Rowe Jr wrote: > > On Thu, Oct 4, 2018 at 12:09 PM Evgeny Kotkov > wrote: >> >> >> However, a more important question is whether there is an actual problem to >> solve. I see that ap_http_header_filter() features a whitelist of headers >> that are sent for 304 responses (http_filters.c:1428), and all headers such >> as Content-Encoding are filtered anyway. > > > AIUI Transfer-* headers should be filtered. Content-* headers must match > the specific ETag as if the response was 200, from my reading. I'm reading the below as a "SHOULD NOT" for anything other than: Cache-Control, Content-Location, Date, ETag, Expires, and Vary. https://tools.ietf.org/html/rfc7232#section-4.1 : Since the goal of a 304 response is to minimize information transfer when the recipient already has one or more cached representations, a sender SHOULD NOT generate representation metadata other than the above listed fields unless said metadata exists for the purpose of guiding cache updates (e.g., Last-Modified might be useful if the response does not have an ETag field). I may be missing something but it seems to me that Content-Encoding shouldn't be set, "Vary: Accept-Encoding" is how we tell that content encoders (deflate, brotli...) are (or could be) in the place.
Re: svn commit: r1837056 - in /httpd/httpd/trunk: ./ include/ modules/filters/ modules/http/ modules/http2/ modules/proxy/ modules/test/ server/
On Thu, Oct 4, 2018 at 12:09 PM Evgeny Kotkov wrote: > > However, a more important question is whether there is an actual problem to > solve. I see that ap_http_header_filter() features a whitelist of headers > that are sent for 304 responses (http_filters.c:1428), and all headers such > as Content-Encoding are filtered anyway. > AIUI Transfer-* headers should be filtered. Content-* headers must match the specific ETag as if the response was 200, from my reading.
Re: svn commit: r1837056 - in /httpd/httpd/trunk: ./ include/ modules/filters/ modules/http/ modules/http2/ modules/proxy/ modules/test/ server/
Yann Ylavic writes: >> I am thinking about fixing this with the attached patch and proposing it for >> backport to 2.4.x. >> >> Would there be any objections to that? > > +1 for the patch, I missed the separate 304 handling in > mod_brotli/deflate, thanks for catching this! Thanks, I will commit it at the earliest opportunity. > It seems that the 304 "shortcut" happens too late though, after > entity/content-* headers are added to the response, which does not > look right. Don't we need something like the attached patch too? I haven't checked it in details, but at first glance I think that this patch could break a few cases. One example would be a case with several filters working in a chain for a 304 response. The first of them sees Content-Encoding identity, performs some fix-ups, such as the one in deflate_check_etag() and removes itself from the chain without altering the r->content-encoding or the Content- Encoding header value. The next filter then sees the C-E identity again, decides to do another fix-up before bailing out, and thus results in an incorrect ETag value or something similar. (An interesting observation is that https://svn.apache.org/r743814 does an opposite of this patch by ensuring that C-E is actually set prior to bailing out and removing itself when dealing with 304's.) However, a more important question is whether there is an actual problem to solve. I see that ap_http_header_filter() features a whitelist of headers that are sent for 304 responses (http_filters.c:1428), and all headers such as Content-Encoding are filtered anyway. So maybe the current state doesn't require fixing at all — assuming that neither mod_deflate nor mod_brotli can actually begin streaming the 304 response with the (unexpected) set of headers — which I don't think could be happening. Thanks, Evgeny Kotkov
Re: svn commit: r1837056 - in /httpd/httpd/trunk: ./ include/ modules/filters/ modules/http/ modules/http2/ modules/proxy/ modules/test/ server/
On Wed, Oct 3, 2018 at 5:14 PM Evgeny Kotkov
wrote:
>
> I am thinking about fixing this with the attached patch and proposing it for
> backport to 2.4.x.
>
> Would there be any objections to that?
+1 for the patch, I missed the separate 304 handling in
mod_brotli/deflate, thanks for catching this!
It seems that the 304 "shortcut" happens too late though, after
entity/content-* headers are added to the response, which does not
look right. Don't we need something like the attached patch too?
Regards,
Yann.
Index: modules/filters/mod_brotli.c
===
--- modules/filters/mod_brotli.c (revision 1840709)
+++ modules/filters/mod_brotli.c (working copy)
@@ -414,21 +414,6 @@ static apr_status_t compress_filter(ap_filter_t *f
return ap_pass_brigade(f->next, bb);
}
-/* If the entire Content-Encoding is "identity", we can replace it. */
-if (!encoding || ap_cstr_casecmp(encoding, "identity") == 0) {
-apr_table_setn(r->headers_out, "Content-Encoding", "br");
-} else {
-apr_table_mergen(r->headers_out, "Content-Encoding", "br");
-}
-
-if (r->content_encoding) {
-r->content_encoding = apr_table_get(r->headers_out,
-"Content-Encoding");
-}
-
-apr_table_unset(r->headers_out, "Content-Length");
-apr_table_unset(r->headers_out, "Content-MD5");
-
/* https://bz.apache.org/bugzilla/show_bug.cgi?id=39727
* https://bz.apache.org/bugzilla/show_bug.cgi?id=45023
*
@@ -461,6 +446,21 @@ static apr_status_t compress_filter(ap_filter_t *f
return ap_pass_brigade(f->next, bb);
}
+/* If the entire Content-Encoding is "identity", we can replace it. */
+if (!encoding || ap_cstr_casecmp(encoding, "identity") == 0) {
+apr_table_setn(r->headers_out, "Content-Encoding", "br");
+} else {
+apr_table_mergen(r->headers_out, "Content-Encoding", "br");
+}
+
+if (r->content_encoding) {
+r->content_encoding = apr_table_get(r->headers_out,
+"Content-Encoding");
+}
+
+apr_table_unset(r->headers_out, "Content-Length");
+apr_table_unset(r->headers_out, "Content-MD5");
+
ctx = create_ctx(conf->quality, conf->lgwin, conf->lgblock,
f->c->bucket_alloc, r->pool);
f->ctx = ctx;
Index: modules/filters/mod_deflate.c
===
--- modules/filters/mod_deflate.c (revision 1840709)
+++ modules/filters/mod_deflate.c (working copy)
@@ -772,12 +772,22 @@ static apr_status_t deflate_out_filter(ap_filter_t
"Forcing compression (force-gzip set)");
}
+if (c->etag_opt != AP_DEFLATE_ETAG_NOCHANGE) {
+deflate_check_etag(r, "gzip", c->etag_opt);
+}
+
+/* For a 304 response, only change the headers */
+if (r->status == HTTP_NOT_MODIFIED) {
+ap_remove_output_filter(f);
+return ap_pass_brigade(f->next, bb);
+}
+
/* At this point we have decided to filter the content. Let's try to
* to initialize zlib (except for 304 responses, where we will only
* send out the headers).
*/
-if (r->status != HTTP_NOT_MODIFIED) {
+{
ctx->bb = apr_brigade_create(r->pool, f->c->bucket_alloc);
ctx->buffer = apr_palloc(r->pool, c->bufferSize);
ctx->libz_end_func = deflateEnd;
@@ -832,16 +842,7 @@ static apr_status_t deflate_out_filter(ap_filter_t
}
apr_table_unset(r->headers_out, "Content-Length");
apr_table_unset(r->headers_out, "Content-MD5");
-if (c->etag_opt != AP_DEFLATE_ETAG_NOCHANGE) {
-deflate_check_etag(r, "gzip", c->etag_opt);
-}
-/* For a 304 response, only change the headers */
-if (r->status == HTTP_NOT_MODIFIED) {
-ap_remove_output_filter(f);
-return ap_pass_brigade(f->next, bb);
-}
-
/* add immortal gzip header */
e = apr_bucket_immortal_create(gzip_header, sizeof gzip_header,
f->c->bucket_alloc);
Re: svn commit: r1837056 - in /httpd/httpd/trunk: ./ include/ modules/filters/ modules/http/ modules/http2/ modules/proxy/ modules/test/ server/
Yann Ylavic writes:
> Log:
> http: Enforce consistently no response body with both 204 and 304 statuses.
[...]
> --- httpd/httpd/trunk/modules/filters/mod_deflate.c (original)
> +++ httpd/httpd/trunk/modules/filters/mod_deflate.c Mon Jul 30 13:08:23 2018
> @@ -642,18 +642,19 @@ static apr_status_t deflate_out_filter(a
>
> /*
> * Only work on main request, not subrequests,
> - * that are not a 204 response with no content
> + * that are not responses with no content (204/304),
> * and are not tagged with the no-gzip env variable
> * and not a partial response to a Range request.
> */
> -if ((r->main != NULL) || (r->status == HTTP_NO_CONTENT) ||
> +if ((r->main != NULL) ||
> +AP_STATUS_IS_HEADER_ONLY(r->status) ||
I think that this change affects how mod_deflate and mod_brotli handle 304's.
Previously, they handled HTTP_NO_CONTENT and HTTP_NOT_MODIFIED
separately. This allowed them to fixup headers such as ETag for 304 responses,
following RFC7232, 4.1 (saying that a 304 response must include appropriate
ETag, Vary and etc.). However, with this change both of them would do nothing
for 304, and potentially violate the spec for ETag and maybe some other header
values.
I am thinking about fixing this with the attached patch and proposing it for
backport to 2.4.x.
Would there be any objections to that?
Thanks,
Evgeny Kotkov
Index: modules/filters/mod_brotli.c
===
--- modules/filters/mod_brotli.c(revision 1842726)
+++ modules/filters/mod_brotli.c(working copy)
@@ -346,12 +346,14 @@ static apr_status_t compress_filter(ap_filter_t *f
const char *accepts;
/* Only work on main request, not subrequests, that are not
- * responses with no content (204/304), and are not tagged with the
+ * a 204 response with no content, and are not tagged with the
* no-brotli env variable, and are not a partial response to
* a Range request.
+ *
+ * Note that responding to 304 is handled separately to set
+ * the required headers (such as ETag) per RFC7232, 4.1.
*/
-if (r->main
-|| AP_STATUS_IS_HEADER_ONLY(r->status)
+if (r->main || r->status == HTTP_NO_CONTENT
|| apr_table_get(r->subprocess_env, "no-brotli")
|| apr_table_get(r->headers_out, "Content-Range")) {
ap_remove_output_filter(f);
Index: modules/filters/mod_deflate.c
===
--- modules/filters/mod_deflate.c (revision 1842726)
+++ modules/filters/mod_deflate.c (working copy)
@@ -642,12 +642,14 @@ static apr_status_t deflate_out_filter(ap_filter_t
/*
* Only work on main request, not subrequests,
- * that are not responses with no content (204/304),
+ * that are not a 204 response with no content
* and are not tagged with the no-gzip env variable
* and not a partial response to a Range request.
+ *
+ * Note that responding to 304 is handled separately to
+ * set the required headers (such as ETag) per RFC7232, 4.1.
*/
-if ((r->main != NULL) ||
-AP_STATUS_IS_HEADER_ONLY(r->status) ||
+if ((r->main != NULL) || (r->status == HTTP_NO_CONTENT) ||
apr_table_get(r->subprocess_env, "no-gzip") ||
apr_table_get(r->headers_out, "Content-Range")
) {
@@ -654,7 +656,7 @@ static apr_status_t deflate_out_filter(ap_filter_t
if (APLOG_R_IS_LEVEL(r, APLOG_TRACE1)) {
const char *reason =
(r->main != NULL) ? "subrequest"
:
-AP_STATUS_IS_HEADER_ONLY(r->status) ? "no content"
:
+(r->status == HTTP_NO_CONTENT) ? "no content"
:
apr_table_get(r->subprocess_env, "no-gzip") ? "no-gzip" :
"content-range";
ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r,
@@ -1533,12 +1535,14 @@ static apr_status_t inflate_out_filter(ap_filter_t
/*
* Only work on main request, not subrequests,
- * that are not responses with no content (204/304),
+ * that are not a 204 response with no content
* and not a partial response to a Range request,
* and only when Content-Encoding ends in gzip.
+ *
+ * Note that responding to 304 is handled separately to
+ * set the required headers (such as ETag) per RFC7232, 4.1.
*/
-if (!ap_is_initial_req(r) ||
-AP_STATUS_IS_HEADER_ONLY(r->status) ||
+if (!ap_is_initial_req(r) || (r->status == HTTP_NO_CONTENT) ||
(apr_table_get(r->headers_out, "Content-Range") != NULL) ||
(check_gzip(r,
