You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2019/10/23 09:09:31 UTC
[tomcat] branch 8.5.x updated: Fix
https://bz.apache.org/bugzilla/show_bug.cgi?id=63829
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 8.5.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/8.5.x by this push:
new 9054e10 Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=63829
9054e10 is described below
commit 9054e10d53170afcd7dd85bd22335238625958dc
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Wed Oct 23 08:50:11 2019 +0200
Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=63829
Improve the check of the Content-Encoding header when looking to see if
Tomcat is serving pre-compressed content. Ensure that only a full token
is matched and that the match is case insensitive.
---
java/org/apache/coyote/CompressionConfig.java | 28 ++++++++++++++++++----
java/org/apache/coyote/LocalStrings.properties | 2 ++
java/org/apache/coyote/http11/Http11Processor.java | 18 ++------------
.../apache/tomcat/util/http/parser/TokenList.java | 24 ++++++++++++++++++-
webapps/docs/changelog.xml | 6 +++++
5 files changed, 57 insertions(+), 21 deletions(-)
diff --git a/java/org/apache/coyote/CompressionConfig.java b/java/org/apache/coyote/CompressionConfig.java
index fd0652e..520ed2e 100644
--- a/java/org/apache/coyote/CompressionConfig.java
+++ b/java/org/apache/coyote/CompressionConfig.java
@@ -20,17 +20,26 @@ import java.io.IOException;
import java.io.StringReader;
import java.util.ArrayList;
import java.util.Enumeration;
+import java.util.HashSet;
import java.util.List;
+import java.util.Set;
import java.util.StringTokenizer;
import java.util.regex.Pattern;
+import org.apache.juli.logging.Log;
+import org.apache.juli.logging.LogFactory;
import org.apache.tomcat.util.buf.MessageBytes;
import org.apache.tomcat.util.http.MimeHeaders;
import org.apache.tomcat.util.http.ResponseUtil;
import org.apache.tomcat.util.http.parser.AcceptEncoding;
+import org.apache.tomcat.util.http.parser.TokenList;
+import org.apache.tomcat.util.res.StringManager;
public class CompressionConfig {
+ private static final Log log = LogFactory.getLog(CompressionConfig.class);
+ private static final StringManager sm = StringManager.getManager(CompressionConfig.class);
+
private int compressionLevel = 0;
private Pattern noCompressionUserAgents = null;
private String compressibleMimeType = "text/html,text/xml,text/plain,text/css," +
@@ -193,10 +202,21 @@ public class CompressionConfig {
// Check if content is not already compressed
MessageBytes contentEncodingMB = responseHeaders.getValue("Content-Encoding");
- if (contentEncodingMB != null &&
- (contentEncodingMB.indexOf("gzip") != -1 ||
- contentEncodingMB.indexOf("br") != -1)) {
- return false;
+ if (contentEncodingMB != null) {
+ // Content-Encoding values are ordered but order is not important
+ // for this check so use a Set rather than a List
+ Set<String> tokens = new HashSet<>();
+ try {
+ TokenList.parseTokenList(responseHeaders.values("Content-Encoding"), tokens);
+ } catch (IOException e) {
+ // Because we are using StringReader, any exception here is a
+ // Tomcat bug.
+ log.warn(sm.getString("compressionConfig.ContentEncodingParseFail"), e);
+ return false;
+ }
+ if (tokens.contains("gzip") || tokens.contains("br")) {
+ return false;
+ }
}
// If force mode, the length and MIME type checks are skipped
diff --git a/java/org/apache/coyote/LocalStrings.properties b/java/org/apache/coyote/LocalStrings.properties
index 03dec37..3f0fbdf 100644
--- a/java/org/apache/coyote/LocalStrings.properties
+++ b/java/org/apache/coyote/LocalStrings.properties
@@ -48,6 +48,8 @@ abstractProtocolHandler.stopError=Failed to stop end point associated with Proto
asyncStateMachine.invalidAsyncState=Calling [{0}] is not valid for a request with Async state [{1}]
+compressionConfig.ContentEncodingParseFail=Failed to parse Content-Encoding header when chekcing to see if compression was already in use
+
request.notAsync=It is only valid to switch to non-blocking IO within async processing or HTTP upgrade processing
request.nullReadListener=The listener passed to setReadListener() may not be null
request.readListenerSet=The non-blocking read listener has already been set
diff --git a/java/org/apache/coyote/http11/Http11Processor.java b/java/org/apache/coyote/http11/Http11Processor.java
index 05e595a..f6de5f1 100644
--- a/java/org/apache/coyote/http11/Http11Processor.java
+++ b/java/org/apache/coyote/http11/Http11Processor.java
@@ -18,10 +18,7 @@ package org.apache.coyote.http11;
import java.io.IOException;
import java.io.InterruptedIOException;
-import java.io.StringReader;
import java.nio.ByteBuffer;
-import java.util.Collection;
-import java.util.Enumeration;
import java.util.HashSet;
import java.util.Locale;
import java.util.Map;
@@ -956,7 +953,7 @@ public class Http11Processor extends AbstractProcessor {
MessageBytes connectionValueMB = headers.getValue(Constants.CONNECTION);
if (connectionValueMB != null && !connectionValueMB.isNull()) {
Set<String> tokens = new HashSet<>();
- parseConnectionTokens(headers, tokens);
+ TokenList.parseTokenList(headers.values(Constants.CONNECTION), tokens);
if (tokens.contains(Constants.CLOSE)) {
keepAlive = false;
} else if (tokens.contains(Constants.KEEPALIVE)) {
@@ -1369,22 +1366,11 @@ public class Http11Processor extends AbstractProcessor {
}
Set<String> tokens = new HashSet<>();
- parseConnectionTokens(headers, tokens);
+ TokenList.parseTokenList(headers.values(Constants.CONNECTION), tokens);
return tokens.contains(token);
}
- private static void parseConnectionTokens(MimeHeaders headers, Collection<String> tokens) throws IOException {
- Enumeration<String> values = headers.values(Constants.CONNECTION);
- while (values.hasMoreElements()) {
- String nextHeaderValue = values.nextElement();
- if (nextHeaderValue != null) {
- TokenList.parseTokenList(new StringReader(nextHeaderValue), tokens);
- }
- }
- }
-
-
private void prepareSendfile(OutputFilter[] outputFilters) {
String fileName = (String) request.getAttribute(
org.apache.coyote.Constants.SENDFILE_FILENAME_ATTR);
diff --git a/java/org/apache/tomcat/util/http/parser/TokenList.java b/java/org/apache/tomcat/util/http/parser/TokenList.java
index ca5e153..db40877 100644
--- a/java/org/apache/tomcat/util/http/parser/TokenList.java
+++ b/java/org/apache/tomcat/util/http/parser/TokenList.java
@@ -18,7 +18,9 @@ package org.apache.tomcat.util.http.parser;
import java.io.IOException;
import java.io.Reader;
+import java.io.StringReader;
import java.util.Collection;
+import java.util.Enumeration;
import java.util.Locale;
public class TokenList {
@@ -29,12 +31,32 @@ public class TokenList {
/**
+ * Parses an enumeration of header values of the form 1#token, forcing all
+ * parsed values to lower case.
+ *
+ * @param inputs The headers to parse
+ * @param result The Collection (usually a list of a set) to which the
+ * parsed tokens should be added
+ *
+ * @throws IOException If an I/O error occurs reading the header
+ */
+ public static void parseTokenList(Enumeration<String> inputs, Collection<String> result) throws IOException {
+ while (inputs.hasMoreElements()) {
+ String nextHeaderValue = inputs.nextElement();
+ if (nextHeaderValue != null) {
+ TokenList.parseTokenList(new StringReader(nextHeaderValue), result);
+ }
+ }
+ }
+
+
+ /**
* Parses a header of the form 1#token, forcing all parsed values to lower
* case. This is typically used when header values are case-insensitive.
*
* @param input The header to parse
* @param result The Collection (usually a list of a set) to which the
- * parsed token should be added
+ * parsed tokens should be added
*
* @throws IOException If an I/O error occurs reading the header
*/
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 0c42ec0..a96f393 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -73,6 +73,12 @@
<code>Connection</code> HTTP headers looking for a specific token, be
stricter in ensuring that the exact token is present. (markt)
</fix>
+ <fix>
+ <bug>63829</bug>: Improve the check of the <code>Content-Encoding</code>
+ header when looking to see if Tomcat is serving pre-compressed content.
+ Ensure that only a full token is matched and that the match is case
+ insensitive. (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="Other">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch 8.5.x updated: Fix
https://bz.apache.org/bugzilla/show_bug.cgi?id=63829
Posted by Mark Thomas <ma...@apache.org>.
On 23/10/2019 17:56, Christopher Schultz wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
>
> Mark,
>
> On 10/23/19 07:38, Mark Thomas wrote:
>> On 23/10/2019 13:28, Mark Thomas wrote:
>>> On 23/10/2019 11:09, markt@apache.org wrote:
>>>> This is an automated email from the ASF dual-hosted git
>>>> repository.
>>>>
>>>> markt pushed a commit to branch 8.5.x in repository
>>>> https://gitbox.apache.org/repos/asf/tomcat.git
>>>>
>>>>
>>>> The following commit(s) were added to refs/heads/8.5.x by this
>>>> push: new 9054e10 Fix
>>>> https://bz.apache.org/bugzilla/show_bug.cgi?id=63829 9054e10 is
>>>> described below
>>>>
>>>> commit 9054e10d53170afcd7dd85bd22335238625958dc Author: Mark
>>>> Thomas <ma...@apache.org> AuthorDate: Wed Oct 23 08:50:11 2019
>>>> +0200
>>>>
>>>> Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=63829
>>>>
>>>> Improve the check of the Content-Encoding header when looking
>>>> to see if Tomcat is serving pre-compressed content. Ensure that
>>>> only a full token is matched and that the match is case
>>>> insensitive.
>>>
>>> This isn't complete for 8.5.x because only HTTP/2 uses
>>> CompressionConfig to make the compress / don't compress
>>> decision.
>>>
>>> I could apply a similar change to the relevant parts of
>>> Http11Processor but I was wondering about the possibility of a
>>> more extensive back-port that aligned the 8.5.x implementation
>>> with 9.0.x. This would involve API changes including: - retain a
>>> reference to the Protocol - change to constructor signature -
>>> remove unnecessary getters and setters
>>>
>>> While this is tempting from both a simplification PoV and from
>>> an aligning 8.5.x and 9.0.x PoV I do wonder what the risk of
>>> breakage is if users are extending Http11Processor. It is an
>>> internal API but I suspect it is still used by some.
>>>
>>> I think I am going to look at see if there is some sort of middle
>>> ground to be found. Meanwhile, what do people think about API
>>> changes along the lines of the above.
>>
>> Looking at the history, we have changed the API for the constructor
>> in the past so I think it would be safe to do that again. My plan
>> is to replace most of the parameters with the Protocol. I think the
>> getters and setters will need to stay. They can/should probably be
>> deprecated as they are removed in 9.0.x.
>
> Maybe just add another constructor instead of replacing? I guess that
> would make the code uglier in other places where you'd prefer to use
> Protocol instead of Parts/Of/Protocol and have to check to see what
> you've got.
Exactly. On the grounds we have changed this constructor a few times
during the life of 8.5.x, I think this change is safe. It should also
reduce the need for future changes.
The next step (that wires in the CompressionConfig) might be a little
more controversial. I need to finish that work and see what it looks like.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch 8.5.x updated: Fix
https://bz.apache.org/bugzilla/show_bug.cgi?id=63829
Posted by Christopher Schultz <ch...@christopherschultz.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Mark,
On 10/23/19 07:38, Mark Thomas wrote:
> On 23/10/2019 13:28, Mark Thomas wrote:
>> On 23/10/2019 11:09, markt@apache.org wrote:
>>> This is an automated email from the ASF dual-hosted git
>>> repository.
>>>
>>> markt pushed a commit to branch 8.5.x in repository
>>> https://gitbox.apache.org/repos/asf/tomcat.git
>>>
>>>
>>> The following commit(s) were added to refs/heads/8.5.x by this
>>> push: new 9054e10 Fix
>>> https://bz.apache.org/bugzilla/show_bug.cgi?id=63829 9054e10 is
>>> described below
>>>
>>> commit 9054e10d53170afcd7dd85bd22335238625958dc Author: Mark
>>> Thomas <ma...@apache.org> AuthorDate: Wed Oct 23 08:50:11 2019
>>> +0200
>>>
>>> Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=63829
>>>
>>> Improve the check of the Content-Encoding header when looking
>>> to see if Tomcat is serving pre-compressed content. Ensure that
>>> only a full token is matched and that the match is case
>>> insensitive.
>>
>> This isn't complete for 8.5.x because only HTTP/2 uses
>> CompressionConfig to make the compress / don't compress
>> decision.
>>
>> I could apply a similar change to the relevant parts of
>> Http11Processor but I was wondering about the possibility of a
>> more extensive back-port that aligned the 8.5.x implementation
>> with 9.0.x. This would involve API changes including: - retain a
>> reference to the Protocol - change to constructor signature -
>> remove unnecessary getters and setters
>>
>> While this is tempting from both a simplification PoV and from
>> an aligning 8.5.x and 9.0.x PoV I do wonder what the risk of
>> breakage is if users are extending Http11Processor. It is an
>> internal API but I suspect it is still used by some.
>>
>> I think I am going to look at see if there is some sort of middle
>> ground to be found. Meanwhile, what do people think about API
>> changes along the lines of the above.
>
> Looking at the history, we have changed the API for the constructor
> in the past so I think it would be safe to do that again. My plan
> is to replace most of the parameters with the Protocol. I think the
> getters and setters will need to stay. They can/should probably be
> deprecated as they are removed in 9.0.x.
Maybe just add another constructor instead of replacing? I guess that
would make the code uglier in other places where you'd prefer to use
Protocol instead of Parts/Of/Protocol and have to check to see what
you've got.
- -chris
-----BEGIN PGP SIGNATURE-----
Comment: Using GnuPG with Thunderbird - https://www.enigmail.net/
iQIzBAEBCAAdFiEEMmKgYcQvxMe7tcJcHPApP6U8pFgFAl2weCQACgkQHPApP6U8
pFiRbBAAiK81Tr/i+gBQox9/9FoBBh98609HqkCn2F3KtqXKH5XqVa1zRw6LFG9P
vo4PUP5NQni3g/QIOyUGGp7TZ5yOXSs/h4awKzMrezRGRHOwvSGw7LsLU6KI1cIZ
rOWdEk/j17luLuw6KcxKKcgXHFgG7e0UH2OINF8NuNG4hLP9ZDYEbQBYltY4wKVq
Tr0ppqQcGZogsrY/qzCwq4CAwoAgfw2hpoUygJYXT0/5Xwa3EMnYT6afbzhkuAcM
8DOlN71G+qPsJ0rablsbHZ0KE4JQ6hWioU7o+w+JyOAcJEiYPz2UBzezPDBsr6Id
E7+txlQ5tWXsPOvYpE7aDvdjF7z+vQ+C3RpYk7mCFB4UOcYJasK4uQZhieLl6aKt
FElqFIl/UkZOiEY0z3H6MU9yJ2fOFyFqjhcvzTpptUTDmpKj4Ks1D+nwxkr25om/
spnKX/cRF1mYjgFww/LgTP7qOn/BSVUtj44H8OJiGDVzK1giTrpF++uUYMdaMF33
HVk80lIhkyxlbQzTH+3C2HQhfiHn1t4TbZSvSRaTMFJvKHV6LB2NGfbqEigIMVvk
RtOFeK4jGvhnVS91eRNpQW/hu+Vc14ISOQmt/s6gpKel+Sq5XRQJLPO7svOOc2Cz
rm/NxWCGdMr7EEi4g2p0s/a1FK2BXnr/nalKYMBUOHWXdU/6Yes=
=9+Lb
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch 8.5.x updated: Fix
https://bz.apache.org/bugzilla/show_bug.cgi?id=63829
Posted by Mark Thomas <ma...@apache.org>.
On 23/10/2019 13:28, Mark Thomas wrote:
> On 23/10/2019 11:09, markt@apache.org wrote:
>> This is an automated email from the ASF dual-hosted git repository.
>>
>> markt pushed a commit to branch 8.5.x
>> in repository https://gitbox.apache.org/repos/asf/tomcat.git
>>
>>
>> The following commit(s) were added to refs/heads/8.5.x by this push:
>> new 9054e10 Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=63829
>> 9054e10 is described below
>>
>> commit 9054e10d53170afcd7dd85bd22335238625958dc
>> Author: Mark Thomas <ma...@apache.org>
>> AuthorDate: Wed Oct 23 08:50:11 2019 +0200
>>
>> Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=63829
>>
>> Improve the check of the Content-Encoding header when looking to see if
>> Tomcat is serving pre-compressed content. Ensure that only a full token
>> is matched and that the match is case insensitive.
>
> This isn't complete for 8.5.x because only HTTP/2 uses CompressionConfig
> to make the compress / don't compress decision.
>
> I could apply a similar change to the relevant parts of Http11Processor
> but I was wondering about the possibility of a more extensive back-port
> that aligned the 8.5.x implementation with 9.0.x. This would involve API
> changes including:
> - retain a reference to the Protocol
> - change to constructor signature
> - remove unnecessary getters and setters
>
> While this is tempting from both a simplification PoV and from an
> aligning 8.5.x and 9.0.x PoV I do wonder what the risk of breakage is if
> users are extending Http11Processor. It is an internal API but I suspect
> it is still used by some.
>
> I think I am going to look at see if there is some sort of middle ground
> to be found. Meanwhile, what do people think about API changes along the
> lines of the above.
Looking at the history, we have changed the API for the constructor in
the past so I think it would be safe to do that again. My plan is to
replace most of the parameters with the Protocol. I think the getters
and setters will need to stay. They can/should probably be deprecated as
they are removed in 9.0.x.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch 8.5.x updated: Fix
https://bz.apache.org/bugzilla/show_bug.cgi?id=63829
Posted by Mark Thomas <ma...@apache.org>.
On 23/10/2019 11:09, markt@apache.org wrote:
> This is an automated email from the ASF dual-hosted git repository.
>
> markt pushed a commit to branch 8.5.x
> in repository https://gitbox.apache.org/repos/asf/tomcat.git
>
>
> The following commit(s) were added to refs/heads/8.5.x by this push:
> new 9054e10 Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=63829
> 9054e10 is described below
>
> commit 9054e10d53170afcd7dd85bd22335238625958dc
> Author: Mark Thomas <ma...@apache.org>
> AuthorDate: Wed Oct 23 08:50:11 2019 +0200
>
> Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=63829
>
> Improve the check of the Content-Encoding header when looking to see if
> Tomcat is serving pre-compressed content. Ensure that only a full token
> is matched and that the match is case insensitive.
This isn't complete for 8.5.x because only HTTP/2 uses CompressionConfig
to make the compress / don't compress decision.
I could apply a similar change to the relevant parts of Http11Processor
but I was wondering about the possibility of a more extensive back-port
that aligned the 8.5.x implementation with 9.0.x. This would involve API
changes including:
- retain a reference to the Protocol
- change to constructor signature
- remove unnecessary getters and setters
While this is tempting from both a simplification PoV and from an
aligning 8.5.x and 9.0.x PoV I do wonder what the risk of breakage is if
users are extending Http11Processor. It is an internal API but I suspect
it is still used by some.
I think I am going to look at see if there is some sort of middle ground
to be found. Meanwhile, what do people think about API changes along the
lines of the above.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org