Re: svn commit: r1884280 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy_ftp.c modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c

[Roy T. Fielding]

> On Dec 16, 2020, at 2:52 AM, Graham Leggett  wrote:
> 
> On 12 Dec 2020, at 01:59, Roy T. Fielding  > wrote:
> 
>> That is too many questions. The purpose of the cache requirement is so that 
>> the cache
>> does not deliver a non-validated entry after receiving a failed validation. 
>> It doesn't really
>> matter what code is returned so long as it is 5xx, so the specs are 
>> over-constraining here.
>> 
>> The CoAdvisor test suite is overly pedantic.
>> 
>> And, as stated, this only applies when the request contains max-revalidate 
>> and the
>> action being done is a cache revalidation. No other code should behave this 
>> way.
> 
> To clarify, the change under discussion covers behaviour when a proxy of any 
> kind (including an ftp proxy) suffers a low level network error of any kind, 
> from DNS errors to low level network errors.

Yes, and as I said, that is wrong.

> Whether ultimately correct or not, CoAdvisor was very specific as to the 
> codes to be returned in these cases.

I am very happy for their financial success. However, they are frequently
wrong, even though they are supposed to be based on the standard.

>> Reverting the change is the correct call, regardless, but it is also the 
>> right choice.
>> I have filed a bug on the Cache spec to change that MUST send 504 to a MUST 
>> send 5xx.
>> 
>>https://github.com/httpwg/http-core/issues/608 
>> 
>> 
>> If you think we need to change other things in the specs, please file bugs 
>> now.
> 
> The above issue relates to must-revalidate only, while the wider issue is for 
> which 5xx error to be returned in which situation. Can you clarify?
> 
> The patch in this thread covers each case, and basically boils down to a 
> choice between “Bad Gateway” or “Timed Out”.

The patch changed BAD_GATEWAY to Timed Out for no reason whatsoever.

The spec text that you pointed to is about cache and cache control when
the cache is disconnected from the network. The spec says that the cache
must generate a 504 when it is disconnected, meaning it has NO response
other than the entry in its cache. I fully understand why CoAdvisor is
confused and unable to properly test for that condition, given that it isn't
an internal cache testing suite, but this has nothing to do with a gateway
receiving or generating a 5xx code upon gateway fail. And, in any case,
the requirement in the spec is overly constraining when it is supposed to
be minimally preventing the cache from reusing its cached response.

IOW, gateway != cache.

> Given the long list here http://coad.measurement-factory.com/clients.html 
>  I would be keen to make 
> sure httpd worked the same way as all those other servers.

That is not an interop concern.

Roy

Re: svn commit: r1884505 - in /httpd/httpd/trunk: changes-entries/pr64339.txt modules/filters/mod_xml2enc.c

[Nick Kew]

> On 16 Dec 2020, at 17:47, Yann Ylavic  wrote:
> 
> On Wed, Dec 16, 2020 at 6:36 PM Yann Ylavic  wrote:
>> 
>> On Wed, Dec 16, 2020 at 5:23 PM  wrote:
>>> 
>>> -/* only act if starts-with "text/" or contains "xml" */
>>> -if (strncmp(ctype, "text/", 5) && !strstr(ctype, "xml"))  {
>>> +/* only act if starts-with "text/" or contains "+xml" */
>>> +if (strncmp(ctype, "text/", 5) && !strstr(ctype, "+xml"))  {
>>> ap_remove_output_filter(f);
>>> return ap_pass_brigade(f->next, bb) ;
>>> }
>> 
>> Wouldn't this stop matching "application/xml" for instance?
>> 
>> Possibly this test instead:
>>if (strncmp(ctype, "text/", 5)
>>&& (!(x = strstr(ctype, "xml"))
>>|| x == ctype || !strchr("/+", x[-1]))) {
>> ?
> 
> I would even remove the "text/" check (why act on "text/plain" for
> instance), so maybe:
>if (!(x = strstr(ctype, "xml"))
>|| x == ctype || !strchr("/+", x[-1])
>|| apr_isalnum(x[3])) {
> ?

Be liberal in what you accept.  You can limit it further in configuration,
but you can't override a hardwired check.

It certainly needs to operate on text/html for mod_proxy_html, and users might
find reasons for running it on other text types as an alternative to an iconv
filter like mod_charset(_lite).

-- 
Nick Kew

Re: svn commit: r1884505 - in /httpd/httpd/trunk: changes-entries/pr64339.txt modules/filters/mod_xml2enc.c

[Yann Ylavic]

On Wed, Dec 16, 2020 at 6:36 PM Yann Ylavic  wrote:
>
> On Wed, Dec 16, 2020 at 5:23 PM  wrote:
> >
> > -/* only act if starts-with "text/" or contains "xml" */
> > -if (strncmp(ctype, "text/", 5) && !strstr(ctype, "xml"))  {
> > +/* only act if starts-with "text/" or contains "+xml" */
> > +if (strncmp(ctype, "text/", 5) && !strstr(ctype, "+xml"))  {
> >  ap_remove_output_filter(f);
> >  return ap_pass_brigade(f->next, bb) ;
> >  }
>
> Wouldn't this stop matching "application/xml" for instance?
>
> Possibly this test instead:
> if (strncmp(ctype, "text/", 5)
> && (!(x = strstr(ctype, "xml"))
> || x == ctype || !strchr("/+", x[-1]))) {
> ?

I would even remove the "text/" check (why act on "text/plain" for
instance), so maybe:
if (!(x = strstr(ctype, "xml"))
|| x == ctype || !strchr("/+", x[-1])
|| apr_isalnum(x[3])) {
?

Re: svn commit: r1884505 - in /httpd/httpd/trunk: changes-entries/pr64339.txt modules/filters/mod_xml2enc.c

[Yann Ylavic]

On Wed, Dec 16, 2020 at 5:23 PM  wrote:
>
> -/* only act if starts-with "text/" or contains "xml" */
> -if (strncmp(ctype, "text/", 5) && !strstr(ctype, "xml"))  {
> +/* only act if starts-with "text/" or contains "+xml" */
> +if (strncmp(ctype, "text/", 5) && !strstr(ctype, "+xml"))  {
>  ap_remove_output_filter(f);
>  return ap_pass_brigade(f->next, bb) ;
>  }

Wouldn't this stop matching "application/xml" for instance?

Possibly this test instead:
if (strncmp(ctype, "text/", 5)
&& (!(x = strstr(ctype, "xml"))
|| x == ctype || !strchr("/+", x[-1]))) {
?

Re: svn commit: r1884280 - in /httpd/httpd/trunk: CHANGES modules/proxy/mod_proxy_ftp.c modules/proxy/mod_proxy_http.c modules/proxy/proxy_util.c

[Graham Leggett]

On 12 Dec 2020, at 01:59, Roy T. Fielding  wrote:

> That is too many questions. The purpose of the cache requirement is so that 
> the cache
> does not deliver a non-validated entry after receiving a failed validation. 
> It doesn't really
> matter what code is returned so long as it is 5xx, so the specs are 
> over-constraining here.
> 
> The CoAdvisor test suite is overly pedantic.
> 
> And, as stated, this only applies when the request contains max-revalidate 
> and the
> action being done is a cache revalidation. No other code should behave this 
> way.

To clarify, the change under discussion covers behaviour when a proxy of any 
kind (including an ftp proxy) suffers a low level network error of any kind, 
from DNS errors to low level network errors.

Whether ultimately correct or not, CoAdvisor was very specific as to the codes 
to be returned in these cases.

> Reverting the change is the correct call, regardless, but it is also the 
> right choice.
> I have filed a bug on the Cache spec to change that MUST send 504 to a MUST 
> send 5xx.
> 
>https://github.com/httpwg/http-core/issues/608 
> 
> 
> If you think we need to change other things in the specs, please file bugs 
> now.

The above issue relates to must-revalidate only, while the wider issue is for 
which 5xx error to be returned in which situation. Can you clarify?

The patch in this thread covers each case, and basically boils down to a choice 
between “Bad Gateway” or “Timed Out”.

Given the long list here http://coad.measurement-factory.com/clients.html I 
would be keen to make sure httpd worked the same way as all those other servers.

Regards,
Graham
—

Re: svn commit: r1884456 - /httpd/httpd/trunk/CHANGES

[Yann Ylavic]

On Tue, Dec 15, 2020 at 9:36 PM Ruediger Pluem  wrote:
>
> On 12/15/20 4:38 PM, Yann Ylavic wrote:
> >
> > When should we do that? I suppose anytime on trunk and when merging on
> > 2.4.x (or at worst when releasing)?
> Correct. Any time on trunk or if you don't want to have it on the trunk 
> CHANGES remove
> the file just after committing it. It is still there for the backport.
> See also README.CHANGES.
> For 2.4.x you can execute it after backporting, but since 
> http://svn.apache.org/viewvc?view=revision&revision=1881738
> the tag.sh script will do it before tagging a release.

That's great tooling, thanks Rüdiger!
Now MMN :)

Regards;
Yann.