Yann Ylavic <yla...@apache.org> 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, r->headers_out, r->err_headers_out) == 0)
) {
