Re: svn commit: r1837056 - in /httpd/httpd/trunk: ./ include/ modules/filters/ modules/http/ modules/http2/ modules/proxy/ modules/test/ server/

[Evgeny Kotkov <ev...@visualsvn.com>]

Yann Ylavic <yl...@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

Re: svn commit: r1837056 - in /httpd/httpd/trunk: ./ include/ modules/filters/ modules/http/ modules/http2/ modules/proxy/ modules/test/ server/

[Yann Ylavic <yl...@gmail.com>]

On Thu, Oct 4, 2018 at 7:20 PM William A Rowe Jr <wr...@rowe-clan.net> wrote:
>
> On Thu, Oct 4, 2018 at 12:09 PM Evgeny Kotkov <ev...@visualsvn.com> 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/

[William A Rowe Jr <wr...@rowe-clan.net>]

On Thu, Oct 4, 2018 at 12:09 PM Evgeny Kotkov <ev...@visualsvn.com>
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/

[Evgeny Kotkov <ev...@visualsvn.com>]

Evgeny Kotkov <ev...@visualsvn.com> 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/

[Evgeny Kotkov <ev...@visualsvn.com>]

Yann Ylavic <yl...@gmail.com> 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/

[Yann Ylavic <yl...@gmail.com>]

On Wed, Oct 3, 2018 at 5:14 PM Evgeny Kotkov
<ev...@visualsvn.com> 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.