Re: svn commit: r1824297 - in /tomcat/trunk: java/org/apache/coyote/CompressionConfig.java webapps/docs/changelog.xml

2018-02-19 Thread Christopher Schultz
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

Konstantin,

On 2/15/18 8:14 AM, Konstantin Kolinko wrote:
> 2018-02-15 14:38 GMT+03:00  :
>> Author: markt Date: Thu Feb 15 11:38:11 2018 New Revision:
>> 1824297
>> 
>> URL: http://svn.apache.org/viewvc?rev=1824297=rev Log: 
>> Prevent Tomcat from applying gzip compression to content that is
>> already compressed with brotli compression. Patch provided by
>> burka. This closes #99
>> 
>> Modified: 
>> tomcat/trunk/java/org/apache/coyote/CompressionConfig.java 
>> tomcat/trunk/webapps/docs/changelog.xml
>> 
>> Modified:
>> tomcat/trunk/java/org/apache/coyote/CompressionConfig.java URL:
>> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/Comp
ressionConfig.java?rev=1824297=1824296=1824297=diff
>>
>> 

==
>> --- tomcat/trunk/java/org/apache/coyote/CompressionConfig.java
>> (original) +++
>> tomcat/trunk/java/org/apache/coyote/CompressionConfig.java Thu
>> Feb 15 11:38:11 2018 @@ -186,9 +186,10 @@ public class
>> CompressionConfig {
>> 
>> MimeHeaders responseHeaders = response.getMimeHeaders();
>> 
>> -// Check if content is not already gzipped +//
>> Check if content is not already compressed MessageBytes
>> contentEncodingMB =
>> responseHeaders.getValue("Content-Encoding"); -if
>> ((contentEncodingMB != null) &&
>> (contentEncodingMB.indexOf("gzip") != -1)) { +if
>> ((contentEncodingMB != null) &&
>> (contentEncodingMB.indexOf("gzip") != -1) && +
>> (contentEncodingMB.indexOf("br") != -1)) {
> 
> 1) Wrong. Causes regression. It should be ( != -1 || != -1).
> Either one is enough to skip compression.
> 
> 2) "indexOf(br)" sounds weak, there can be false positives. But
> not much of a problem: it would result in serving content as 
> non-compresses, it does not break the content.

There will be no false-positives if the value contains an RFC-valid
value. Only "br" contains "br".

I would argue that this should be an equality-check and not an
"indexOf" check, since the response should expect to have exactly one
encoding (as opposed to Accept-Encoding, which can list many
acceptable encodings).

> For "gzip" using indexOf() is OK:  It can be "x-gzip" (see
> 3.1.2.1. Content Codings in RFC7231.) [quote] The following
> content-coding values are defined by this specification:
> 
> compress (and x-compress): See Section 4.2.1 of [RFC7230]. deflate:
> See Section 4.2.2 of [RFC7230]. gzip (and x-gzip): See Section
> 4.2.3 of [RFC7230]. [/quote]

+1

- -chris
-BEGIN PGP SIGNATURE-
Comment: GPGTools - http://gpgtools.org
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQJRBAEBCAA7FiEEMmKgYcQvxMe7tcJcHPApP6U8pFgFAlqLAFAdHGNocmlzQGNo
cmlzdG9waGVyc2NodWx0ei5uZXQACgkQHPApP6U8pFinzA/8C6parkPID67fOx4i
V2R9Kq3+KxsS/XMPoPo2nyjAGbgJ2GfSJfRUmQbNQHGyxUrOcQS9n3FbG1fZqtMu
+I8eRTSeACEPxOID5W1J3bVWY+/KEfCFvRtw23ZUZuTs8kaVo8NbnCe7PsV15R2E
bGliecJbyIqvaeeXyUxbGBpDlUy9LV9E7xcW3Cro7WAFPid/vpSsXs4JtAZo2vqx
ZgyY5fGp3LWaWI8aeJZLEHk31aQEjS6xYhUbnxXwn3a2ALwJUrZW/AJ69jpVL7ge
fgDf5qU55g553EtQvKzGpBSogMTqnijlqgD1pEG6P4IBjEZZc8LwjJ+lY9AM+YIJ
Qbg1EPpiFuqxh/oF1csHOMdEztxVJHEEc8qELDkcP/tgUWHdHxoedIlPwYFRRyxV
zu26OQ9rDdqX7sAFngaic8ot178T4a0xvrFvy/8kV8Mp2gsgNgtspEf2ONxoLvav
x1pYGOuiYwb3tpllxfRsqqJZGXNvKO5+YYOiNSW21vCZpB32yVtZF5TPfx4Yvqie
N8HcSXc7JhY8N+HqusjVNlyWC0Y0Y8K+3mP36Ss4rUUI8XJgGpyOfFe9B4sioTXD
FaF5fpOE6pMBvLkBdnAt0X/MWVyMJLW7WYCNDo5mwuIusqM0mqwANJvJInDj2SMi
d4HEBDgTh6HcpHDFJ57I5ljZi5Q=
=hlZq
-END PGP SIGNATURE-

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



Re: svn commit: r1824297 - in /tomcat/trunk: java/org/apache/coyote/CompressionConfig.java webapps/docs/changelog.xml

2018-02-15 Thread Mark Thomas
On 15/02/18 13:14, Konstantin Kolinko wrote:
> 2018-02-15 14:38 GMT+03:00  :
>> Author: markt
>> Date: Thu Feb 15 11:38:11 2018
>> New Revision: 1824297
>>
>> URL: http://svn.apache.org/viewvc?rev=1824297=rev
>> Log:
>> Prevent Tomcat from applying gzip compression to content that is already 
>> compressed with brotli compression.
>> Patch provided by burka.
>> This closes #99
>>
>> Modified:
>> tomcat/trunk/java/org/apache/coyote/CompressionConfig.java
>> tomcat/trunk/webapps/docs/changelog.xml
>>
>> Modified: tomcat/trunk/java/org/apache/coyote/CompressionConfig.java
>> URL: 
>> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/CompressionConfig.java?rev=1824297=1824296=1824297=diff
>> ==
>> --- tomcat/trunk/java/org/apache/coyote/CompressionConfig.java (original)
>> +++ tomcat/trunk/java/org/apache/coyote/CompressionConfig.java Thu Feb 15 
>> 11:38:11 2018
>> @@ -186,9 +186,10 @@ public class CompressionConfig {
>>
>>  MimeHeaders responseHeaders = response.getMimeHeaders();
>>
>> -// Check if content is not already gzipped
>> +// Check if content is not already compressed
>>  MessageBytes contentEncodingMB = 
>> responseHeaders.getValue("Content-Encoding");
>> -if ((contentEncodingMB != null) && 
>> (contentEncodingMB.indexOf("gzip") != -1)) {
>> +if ((contentEncodingMB != null) && 
>> (contentEncodingMB.indexOf("gzip") != -1) &&
>> +(contentEncodingMB.indexOf("br") != -1)) {
> 
> 1) Wrong. Causes regression. It should be ( != -1 || != -1).  Either
> one is enough to skip compression.

Whoops. I'll fix that.

> 2) "indexOf(br)" sounds weak, there can be false positives. But not
> much of a problem: it would result in serving content as
> non-compresses, it does not break the content.

I did think about that. There aren't that many likely values in
Content-Encoding [1] and none of them conflict with the use of "br".

>> Modified: tomcat/trunk/webapps/docs/changelog.xml
>> URL: 
>> http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1824297=1824296=1824297=diff
>> ==
>> --- tomcat/trunk/webapps/docs/changelog.xml (original)
>> +++ tomcat/trunk/webapps/docs/changelog.xml Thu Feb 15 11:38:11 2018
>> @@ -60,6 +60,10 @@
>>  specific error codes and/or exception types with the
>>  ErrorReportValve. (markt)
>>
>> +  
>> +Prevent Tomcat from applying gzip compression to content that is 
>> already
>> +compressed with brotli compression. Patch provided by burka. (markt)
> 
> Based on patch provided...

Ack.

Mark

[1]
https://www.iana.org/assignments/http-parameters/http-parameters.xml#http-parameters-1


-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



Re: svn commit: r1824297 - in /tomcat/trunk: java/org/apache/coyote/CompressionConfig.java webapps/docs/changelog.xml

2018-02-15 Thread Konstantin Kolinko
2018-02-15 14:38 GMT+03:00  :
> Author: markt
> Date: Thu Feb 15 11:38:11 2018
> New Revision: 1824297
>
> URL: http://svn.apache.org/viewvc?rev=1824297=rev
> Log:
> Prevent Tomcat from applying gzip compression to content that is already 
> compressed with brotli compression.
> Patch provided by burka.
> This closes #99
>
> Modified:
> tomcat/trunk/java/org/apache/coyote/CompressionConfig.java
> tomcat/trunk/webapps/docs/changelog.xml
>
> Modified: tomcat/trunk/java/org/apache/coyote/CompressionConfig.java
> URL: 
> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/CompressionConfig.java?rev=1824297=1824296=1824297=diff
> ==
> --- tomcat/trunk/java/org/apache/coyote/CompressionConfig.java (original)
> +++ tomcat/trunk/java/org/apache/coyote/CompressionConfig.java Thu Feb 15 
> 11:38:11 2018
> @@ -186,9 +186,10 @@ public class CompressionConfig {
>
>  MimeHeaders responseHeaders = response.getMimeHeaders();
>
> -// Check if content is not already gzipped
> +// Check if content is not already compressed
>  MessageBytes contentEncodingMB = 
> responseHeaders.getValue("Content-Encoding");
> -if ((contentEncodingMB != null) && 
> (contentEncodingMB.indexOf("gzip") != -1)) {
> +if ((contentEncodingMB != null) && 
> (contentEncodingMB.indexOf("gzip") != -1) &&
> +(contentEncodingMB.indexOf("br") != -1)) {

1) Wrong. Causes regression. It should be ( != -1 || != -1).  Either
one is enough to skip compression.

2) "indexOf(br)" sounds weak, there can be false positives. But not
much of a problem: it would result in serving content as
non-compresses, it does not break the content.

For "gzip" using indexOf() is OK:  It can be "x-gzip" (see 3.1.2.1.
Content Codings in RFC7231.)
[quote]
   The following content-coding values are defined by this
   specification:

  compress (and x-compress): See Section 4.2.1 of [RFC7230].
  deflate: See Section 4.2.2 of [RFC7230].
  gzip (and x-gzip): See Section 4.2.3 of [RFC7230].
[/quote]

>  return false;
>  }
>
>
> Modified: tomcat/trunk/webapps/docs/changelog.xml
> URL: 
> http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1824297=1824296=1824297=diff
> ==
> --- tomcat/trunk/webapps/docs/changelog.xml (original)
> +++ tomcat/trunk/webapps/docs/changelog.xml Thu Feb 15 11:38:11 2018
> @@ -60,6 +60,10 @@
>  specific error codes and/or exception types with the
>  ErrorReportValve. (markt)
>
> +  
> +Prevent Tomcat from applying gzip compression to content that is 
> already
> +compressed with brotli compression. Patch provided by burka. (markt)

Based on patch provided...

> +  
>  
>
>
>
>
>
> -
> To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
> For additional commands, e-mail: dev-h...@tomcat.apache.org
>

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org