[GitHub] [tomcat] martin-g commented on a change in pull request #384: Fix BZ 60781 Escape access log output where httpd does

[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