You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by GitBox <gi...@apache.org> on 2020/12/04 19:31:36 UTC
[GitHub] [tomcat] markt-asf opened a new pull request #384: Fix BZ 60781 Escape access log output where httpd does
markt-asf opened a new pull request #384:
URL: https://github.com/apache/tomcat/pull/384
https://bz.apache.org/bugzilla/show_bug.cgi?id=60781
It isn't quite as simple just escape where httpd does as httpd and Tomcat support slightly different things and validate at different points. See the code comments.
I was concerned about performance but after checking which elements need escaping (see code comments) I think this should be OK. I used Felix's original patch as my starting point but used chars rather than bytes and appended directly to the buffer. Give the potential performance issues, doing this as a PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf merged pull request #384: Fix BZ 60781 Escape access log output where httpd does
Posted by GitBox <gi...@apache.org>.
markt-asf merged pull request #384:
URL: https://github.com/apache/tomcat/pull/384
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] martin-g commented on a change in pull request #384: Fix BZ 60781 Escape access log output where httpd does
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #384:
URL: https://github.com/apache/tomcat/pull/384#discussion_r537495586
##########
File path: java/org/apache/catalina/valves/AbstractAccessLogValve.java
##########
@@ -1805,4 +1808,99 @@ protected AccessLogElement createAccessLogElement(char pattern) {
return new StringElement("???" + pattern + "???");
}
}
+
+
+ /*
+ * This method is intended to mimic the escaping performed by httpd and
+ * mod_log_config. mod_log_config escapes more elements than indicated by the
+ * documentation. See:
+ * https://github.com/apache/httpd/blob/trunk/modules/loggers/mod_log_config.c
+ *
+ * The following escaped elements are not supported by Tomcat:
+ * - %C cookie value (see %{}c below)
+ * - %e environment variable
+ * - %f filename
+ * - %l remote logname (always logs "-")
+ * - %n note
+ * - %R handler
+ * - %ti trailer request header
+ * - %to trailer response header
+ * - %V server name per UseCanonicalName setting
+ *
+ * The following escaped elements are not escaped in Tomcat because values
+ * that would require escaping are rejected before they reach the
+ * AccessLogValve:
+ * - %h remote host
+ * - %H request protocol
+ * - %m request method
+ * - %q query string
+ * - %r request line
+ * - %U request URI
+ * - %v canonical server name
+ *
+ * The following escaped elements are supported by Tomcat:
+ * - %{}i request header
+ * - %{}o response header
+ * - %u remote user
+ *
+ * The following additional Tomcat elements are escaped for consistency:
+ * - %{}c cookie value
+ * - %{}r request attribute
+ * - %{}s session attribute
+ *
+ * giving a total of 6 elements that are escaped in Tomcat.
+ *
+ * Quoting from the httpd docs:
+ * "...non-printable and other special characters in %r, %i and %o are
+ * escaped using \xhh sequences, where hh stands for the hexadecimal
+ * representation of the raw byte. Exceptions from this rule are " and \,
+ * which are escaped by prepending a backslash, and all whitespace
+ * characters, which are written in their C-style notation (\n, \t, etc)."
+ *
+ * Reviewing the httpd code, characters with the high bit set are escaped.
+ * The httpd is assuming a single byte encoding which may not be true for
+ * Tomcat so Tomcat uses the Java \\uXXXX encoding.
+ */
+ protected static void escapeAndAppend(String input, CharArrayWriter dest) {
+ if (input == null || input.isEmpty()) {
+ dest.append('-');
+ return;
+ }
+
+ for (char c : input.toCharArray()) {
+ switch (c) {
+ // " and \
+ case '\\':
+ dest.append("\\\\");
+ break;
+ case '\"':
+ dest.append("\\\"");
+ break;
Review comment:
I see there was a new commit that moved left everything but the problematic `break` :-)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a change in pull request #384: Fix BZ 60781 Escape access log output where httpd does
Posted by GitBox <gi...@apache.org>.
markt-asf commented on a change in pull request #384:
URL: https://github.com/apache/tomcat/pull/384#discussion_r537472595
##########
File path: test/org/apache/catalina/valves/TestAbstractAccessLogValveEscape.java
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.catalina.valves;
+
+import java.io.CharArrayWriter;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameter;
+import org.junit.runners.Parameterized.Parameters;
+
+
+@RunWith(Parameterized.class)
+public class TestAbstractAccessLogValveEscape {
+
+ @Parameters(name = "{index}: [{0}]")
+ public static Collection<Object[]> parameters() {
+
+ List<Object[]> parameters = new ArrayList<>();
+
+ parameters.add(new String[] { null, "-" });
+ parameters.add(new String[] { "", "-" });
+ parameters.add(new String[] { "ok", "ok" });
+ parameters.add(new String[] { "o\tk", "o\\tk" });
+ parameters.add(new String[] { "o\u0002k", "o\\u0002k" });
+ parameters.add(new String[] { "o\u007fk", "o\\u007fk" });
+ parameters.add(new String[] { "o\u0080k", "o\\u0080k" });
+ parameters.add(new String[] { "o\u00ffk", "o\\u00ffk" });
Review comment:
Good point. I'll add one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a change in pull request #384: Fix BZ 60781 Escape access log output where httpd does
Posted by GitBox <gi...@apache.org>.
markt-asf commented on a change in pull request #384:
URL: https://github.com/apache/tomcat/pull/384#discussion_r537472467
##########
File path: java/org/apache/catalina/valves/AbstractAccessLogValve.java
##########
@@ -1805,4 +1808,99 @@ protected AccessLogElement createAccessLogElement(char pattern) {
return new StringElement("???" + pattern + "???");
}
}
+
+
+ /*
+ * This method is intended to mimic the escaping performed by httpd and
+ * mod_log_config. mod_log_config escapes more elements than indicated by the
+ * documentation. See:
+ * https://github.com/apache/httpd/blob/trunk/modules/loggers/mod_log_config.c
+ *
+ * The following escaped elements are not supported by Tomcat:
+ * - %C cookie value (see %{}c below)
+ * - %e environment variable
+ * - %f filename
+ * - %l remote logname (always logs "-")
+ * - %n note
+ * - %R handler
+ * - %ti trailer request header
+ * - %to trailer response header
+ * - %V server name per UseCanonicalName setting
+ *
+ * The following escaped elements are not escaped in Tomcat because values
+ * that would require escaping are rejected before they reach the
+ * AccessLogValve:
+ * - %h remote host
+ * - %H request protocol
+ * - %m request method
+ * - %q query string
+ * - %r request line
+ * - %U request URI
+ * - %v canonical server name
+ *
+ * The following escaped elements are supported by Tomcat:
+ * - %{}i request header
+ * - %{}o response header
+ * - %u remote user
+ *
+ * The following additional Tomcat elements are escaped for consistency:
+ * - %{}c cookie value
+ * - %{}r request attribute
+ * - %{}s session attribute
+ *
+ * giving a total of 6 elements that are escaped in Tomcat.
+ *
+ * Quoting from the httpd docs:
+ * "...non-printable and other special characters in %r, %i and %o are
+ * escaped using \xhh sequences, where hh stands for the hexadecimal
+ * representation of the raw byte. Exceptions from this rule are " and \,
+ * which are escaped by prepending a backslash, and all whitespace
+ * characters, which are written in their C-style notation (\n, \t, etc)."
+ *
+ * Reviewing the httpd code, characters with the high bit set are escaped.
+ * The httpd is assuming a single byte encoding which may not be true for
+ * Tomcat so Tomcat uses the Java \\uXXXX encoding.
+ */
+ protected static void escapeAndAppend(String input, CharArrayWriter dest) {
+ if (input == null || input.isEmpty()) {
+ dest.append('-');
+ return;
+ }
+
+ for (char c : input.toCharArray()) {
+ switch (c) {
+ // " and \
+ case '\\':
+ dest.append("\\\\");
+ break;
+ case '\"':
+ dest.append("\\\"");
+ break;
Review comment:
Personal style choice (I prefer case statements to be indented from the switch). The Tomcat code is inconsistent. Looks like the consensus amongst Java developers is for no indents. I don't mind changing this.
We have a commented out check for indentation in checkstyle. Looks like it still triggers ~5k errors.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] martin-g commented on a change in pull request #384: Fix BZ 60781 Escape access log output where httpd does
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #384:
URL: https://github.com/apache/tomcat/pull/384#discussion_r537494538
##########
File path: java/org/apache/catalina/valves/AbstractAccessLogValve.java
##########
@@ -1805,4 +1808,99 @@ protected AccessLogElement createAccessLogElement(char pattern) {
return new StringElement("???" + pattern + "???");
}
}
+
+
+ /*
+ * This method is intended to mimic the escaping performed by httpd and
+ * mod_log_config. mod_log_config escapes more elements than indicated by the
+ * documentation. See:
+ * https://github.com/apache/httpd/blob/trunk/modules/loggers/mod_log_config.c
+ *
+ * The following escaped elements are not supported by Tomcat:
+ * - %C cookie value (see %{}c below)
+ * - %e environment variable
+ * - %f filename
+ * - %l remote logname (always logs "-")
+ * - %n note
+ * - %R handler
+ * - %ti trailer request header
+ * - %to trailer response header
+ * - %V server name per UseCanonicalName setting
+ *
+ * The following escaped elements are not escaped in Tomcat because values
+ * that would require escaping are rejected before they reach the
+ * AccessLogValve:
+ * - %h remote host
+ * - %H request protocol
+ * - %m request method
+ * - %q query string
+ * - %r request line
+ * - %U request URI
+ * - %v canonical server name
+ *
+ * The following escaped elements are supported by Tomcat:
+ * - %{}i request header
+ * - %{}o response header
+ * - %u remote user
+ *
+ * The following additional Tomcat elements are escaped for consistency:
+ * - %{}c cookie value
+ * - %{}r request attribute
+ * - %{}s session attribute
+ *
+ * giving a total of 6 elements that are escaped in Tomcat.
+ *
+ * Quoting from the httpd docs:
+ * "...non-printable and other special characters in %r, %i and %o are
+ * escaped using \xhh sequences, where hh stands for the hexadecimal
+ * representation of the raw byte. Exceptions from this rule are " and \,
+ * which are escaped by prepending a backslash, and all whitespace
+ * characters, which are written in their C-style notation (\n, \t, etc)."
+ *
+ * Reviewing the httpd code, characters with the high bit set are escaped.
+ * The httpd is assuming a single byte encoding which may not be true for
+ * Tomcat so Tomcat uses the Java \\uXXXX encoding.
+ */
+ protected static void escapeAndAppend(String input, CharArrayWriter dest) {
+ if (input == null || input.isEmpty()) {
+ dest.append('-');
+ return;
+ }
+
+ for (char c : input.toCharArray()) {
+ switch (c) {
+ // " and \
+ case '\\':
+ dest.append("\\\\");
+ break;
+ case '\"':
+ dest.append("\\\"");
+ break;
Review comment:
I meant the `break` statement. It is not aligned with the other `break`s
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] martin-g commented on a change in pull request #384: Fix BZ 60781 Escape access log output where httpd does
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #384:
URL: https://github.com/apache/tomcat/pull/384#discussion_r537456849
##########
File path: java/org/apache/catalina/valves/AbstractAccessLogValve.java
##########
@@ -1805,4 +1808,99 @@ protected AccessLogElement createAccessLogElement(char pattern) {
return new StringElement("???" + pattern + "???");
}
}
+
+
+ /*
+ * This method is intended to mimic the escaping performed by httpd and
+ * mod_log_config. mod_log_config escapes more elements than indicated by the
+ * documentation. See:
+ * https://github.com/apache/httpd/blob/trunk/modules/loggers/mod_log_config.c
+ *
+ * The following escaped elements are not supported by Tomcat:
+ * - %C cookie value (see %{}c below)
+ * - %e environment variable
+ * - %f filename
+ * - %l remote logname (always logs "-")
+ * - %n note
+ * - %R handler
+ * - %ti trailer request header
+ * - %to trailer response header
+ * - %V server name per UseCanonicalName setting
+ *
+ * The following escaped elements are not escaped in Tomcat because values
+ * that would require escaping are rejected before they reach the
+ * AccessLogValve:
+ * - %h remote host
+ * - %H request protocol
+ * - %m request method
+ * - %q query string
+ * - %r request line
+ * - %U request URI
+ * - %v canonical server name
+ *
+ * The following escaped elements are supported by Tomcat:
+ * - %{}i request header
+ * - %{}o response header
+ * - %u remote user
+ *
+ * The following additional Tomcat elements are escaped for consistency:
+ * - %{}c cookie value
+ * - %{}r request attribute
+ * - %{}s session attribute
+ *
+ * giving a total of 6 elements that are escaped in Tomcat.
+ *
+ * Quoting from the httpd docs:
+ * "...non-printable and other special characters in %r, %i and %o are
+ * escaped using \xhh sequences, where hh stands for the hexadecimal
+ * representation of the raw byte. Exceptions from this rule are " and \,
+ * which are escaped by prepending a backslash, and all whitespace
+ * characters, which are written in their C-style notation (\n, \t, etc)."
+ *
+ * Reviewing the httpd code, characters with the high bit set are escaped.
+ * The httpd is assuming a single byte encoding which may not be true for
+ * Tomcat so Tomcat uses the Java \\uXXXX encoding.
+ */
+ protected static void escapeAndAppend(String input, CharArrayWriter dest) {
+ if (input == null || input.isEmpty()) {
+ dest.append('-');
+ return;
+ }
+
+ for (char c : input.toCharArray()) {
+ switch (c) {
+ // " and \
+ case '\\':
+ dest.append("\\\\");
+ break;
+ case '\"':
+ dest.append("\\\"");
+ break;
Review comment:
indentation is off
##########
File path: test/org/apache/catalina/valves/TestAbstractAccessLogValveEscape.java
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.catalina.valves;
+
+import java.io.CharArrayWriter;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameter;
+import org.junit.runners.Parameterized.Parameters;
+
+
+@RunWith(Parameterized.class)
+public class TestAbstractAccessLogValveEscape {
+
+ @Parameters(name = "{index}: [{0}]")
+ public static Collection<Object[]> parameters() {
+
+ List<Object[]> parameters = new ArrayList<>();
+
+ parameters.add(new String[] { null, "-" });
+ parameters.add(new String[] { "", "-" });
+ parameters.add(new String[] { "ok", "ok" });
+ parameters.add(new String[] { "o\tk", "o\\tk" });
+ parameters.add(new String[] { "o\u0002k", "o\\u0002k" });
+ parameters.add(new String[] { "o\u007fk", "o\\u007fk" });
+ parameters.add(new String[] { "o\u0080k", "o\\u0080k" });
+ parameters.add(new String[] { "o\u00ffk", "o\\u00ffk" });
Review comment:
I've expected to see an input with `"`, to cover the bug report.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org