Expand test coverage

Cleanup.
apache · Mar 7, 2025 · 163004d · 163004d
1 parent 6aa4a7b
commit 163004d
Show file tree

Hide file tree

Showing 4 changed files with 86 additions and 54 deletions.
diff --git a/java/org/apache/catalina/valves/AbstractAccessLogValve.java b/java/org/apache/catalina/valves/AbstractAccessLogValve.java
@@ -1816,6 +1816,10 @@ protected AccessLogElement createAccessLogElement(char pattern) {
      * encoding which may not be true for Tomcat so Tomcat uses the Java \\uXXXX encoding.
      */
     protected static void escapeAndAppend(String input, CharArrayWriter dest) {
+        escapeAndAppend(input, dest, false);
+    }
+
+    protected static void escapeAndAppend(String input, CharArrayWriter dest, boolean escapeQuoteAsDouble) {
         if (input == null || input.isEmpty()) {
             dest.append('-');
             return;
@@ -1851,7 +1855,11 @@ protected static void escapeAndAppend(String input, CharArrayWriter dest) {
                             dest.write(input, next, current - next);
                         }
                         next = current + 1;
-                        dest.append("\\\"");
+                        if (escapeQuoteAsDouble) {
+                            dest.append("\"\"");
+                        } else {
+                            dest.append("\\\"");
+                        }
                         break;
                     // Don't output individual unchanged chars,
                     // write the sub string only when the first char to encode

diff --git a/java/org/apache/catalina/valves/ExtendedAccessLogValve.java b/java/org/apache/catalina/valves/ExtendedAccessLogValve.java
@@ -80,26 +80,6 @@
  * <li><code>x-H(scheme)</code>: getScheme</li>
  * <li><code>x-H(secure)</code>: isSecure</li>
  * </ul>
- * <p>
- * Log rotation can be on or off. This is dictated by the <code>rotatable</code> property.
- * </p>
- * <p>
- * For UNIX users, another field called <code>checkExists</code> is also available. If set to true, the log file's
- * existence will be checked before each logging. This way an external log rotator can move the file somewhere and
- * Tomcat will start with a new file.
- * </p>
- * <p>
- * For JMX junkies, a public method called <code>rotate</code> has been made available to allow you to tell this
- * instance to move the existing log file to somewhere else and start writing a new log file.
- * </p>
- * <p>
- * Conditional logging is also supported. This can be done with the <code>condition</code> property. If the value
- * returned from ServletRequest.getAttribute(condition) yields a non-null value, the logging will be skipped.
- * </p>
- * <p>
- * For extended attributes coming from a getAttribute() call, it is you responsibility to ensure there are no newline or
- * control characters.
- * </p>
  *
  * @author Peter Rossbach
  */
@@ -114,27 +94,33 @@ public class ExtendedAccessLogValve extends AccessLogValve {
      * double quotes ("").
      *
      * @param value - The value to wrap
+     * @param buf the buffer to write to
      *
      * @return '-' if null. Otherwise, toString() will be called on the object and the value will be wrapped in quotes
      *             and any quotes will be escaped with 2 sets of quotes.
      */
-    static String wrap(Object value) {
+    static void wrap(Object value, CharArrayWriter buf) {
         String svalue;
         // Does the value contain a " ? If so must encode it
         if (value == null || "-".equals(value)) {
-            return "-";
+            buf.append('-');
+            return;
         }
 
         try {
             svalue = value.toString();
         } catch (Throwable e) {
             ExceptionUtils.handleThrowable(e);
             /* Log error */
-            return "-";
+            buf.append('-');
+            return;
         }
 
-        /* Wrap all values in double quotes. */
-        return "\"" + svalue.replace("\"", "\"\"") + "\"";
+        buf.append('\"');
+        if (!svalue.isEmpty()) {
+            escapeAndAppend(svalue, buf, true);
+        }
+        buf.append('\"');
     }
 
     @Override
@@ -198,7 +184,7 @@ public RequestHeaderElement(String header) {
 
         @Override
         public void addElement(CharArrayWriter buf, Date date, Request request, Response response, long time) {
-            buf.append(wrap(request.getHeader(header)));
+            wrap(request.getHeader(header), buf);
         }
     }
 
@@ -211,7 +197,7 @@ public ResponseHeaderElement(String header) {
 
         @Override
         public void addElement(CharArrayWriter buf, Date date, Request request, Response response, long time) {
-            buf.append(wrap(response.getHeader(header)));
+            wrap(response.getHeader(header), buf);
         }
     }
 
@@ -224,7 +210,7 @@ public ServletContextElement(String attribute) {
 
         @Override
         public void addElement(CharArrayWriter buf, Date date, Request request, Response response, long time) {
-            buf.append(wrap(request.getContext().getServletContext().getAttribute(attribute)));
+            wrap(request.getContext().getServletContext().getAttribute(attribute), buf);
         }
     }
 
@@ -253,7 +239,7 @@ public void addElement(CharArrayWriter buf, Date date, Request request, Response
             if (value.length() == 0) {
                 buf.append('-');
             } else {
-                buf.append(wrap(value.toString()));
+                wrap(value, buf);
             }
         }
     }
@@ -283,7 +269,9 @@ public void addElement(CharArrayWriter buf, Date date, Request request, Response
                         }
                         buffer.append(iter.next());
                     }
-                    buf.append(wrap(buffer.toString()));
+                    wrap(buffer, buf);
+                } else {
+                    buf.append('-');
                 }
                 return;
             }
@@ -300,7 +288,7 @@ public RequestAttributeElement(String attribute) {
 
         @Override
         public void addElement(CharArrayWriter buf, Date date, Request request, Response response, long time) {
-            buf.append(wrap(request.getAttribute(attribute)));
+            wrap(request.getAttribute(attribute), buf);
         }
     }
 
@@ -317,7 +305,7 @@ public void addElement(CharArrayWriter buf, Date date, Request request, Response
             if (request != null) {
                 session = request.getSession(false);
                 if (session != null) {
-                    buf.append(wrap(session.getAttribute(attribute)));
+                    wrap(session.getAttribute(attribute), buf);
                 }
             }
         }
@@ -342,7 +330,13 @@ private String urlEncode(String value) {
 
         @Override
         public void addElement(CharArrayWriter buf, Date date, Request request, Response response, long time) {
-            buf.append(wrap(urlEncode(request.getParameter(parameter))));
+            String parameterValue;
+            try {
+                parameterValue = request.getParameter(parameter);
+            } catch (IllegalStateException ise) {
+                parameterValue = null;
+            }
+            wrap(urlEncode(parameterValue), buf);
         }
     }
 
@@ -695,63 +689,63 @@ protected AccessLogElement getServletRequestElement(String parameter) {
             return new AccessLogElement() {
                 @Override
                 public void addElement(CharArrayWriter buf, Date date, Request request, Response response, long time) {
-                    buf.append(wrap(request.getAuthType()));
+                    wrap(request.getAuthType(), buf);
                 }
             };
         } else if ("remoteUser".equals(parameter)) {
             return new AccessLogElement() {
                 @Override
                 public void addElement(CharArrayWriter buf, Date date, Request request, Response response, long time) {
-                    buf.append(wrap(request.getRemoteUser()));
+                    wrap(request.getRemoteUser(), buf);
                 }
             };
         } else if ("requestedSessionId".equals(parameter)) {
             return new AccessLogElement() {
                 @Override
                 public void addElement(CharArrayWriter buf, Date date, Request request, Response response, long time) {
-                    buf.append(wrap(request.getRequestedSessionId()));
+                    wrap(request.getRequestedSessionId(), buf);
                 }
             };
         } else if ("requestedSessionIdFromCookie".equals(parameter)) {
             return new AccessLogElement() {
                 @Override
                 public void addElement(CharArrayWriter buf, Date date, Request request, Response response, long time) {
-                    buf.append(wrap("" + request.isRequestedSessionIdFromCookie()));
+                    wrap(String.valueOf(request.isRequestedSessionIdFromCookie()), buf);
                 }
             };
         } else if ("requestedSessionIdValid".equals(parameter)) {
             return new AccessLogElement() {
                 @Override
                 public void addElement(CharArrayWriter buf, Date date, Request request, Response response, long time) {
-                    buf.append(wrap("" + request.isRequestedSessionIdValid()));
+                    wrap(String.valueOf(request.isRequestedSessionIdValid()), buf);
                 }
             };
         } else if ("contentLength".equals(parameter)) {
             return new AccessLogElement() {
                 @Override
                 public void addElement(CharArrayWriter buf, Date date, Request request, Response response, long time) {
-                    buf.append(wrap("" + request.getContentLengthLong()));
+                    wrap(String.valueOf(request.getContentLengthLong()), buf);
                 }
             };
         } else if ("characterEncoding".equals(parameter)) {
             return new AccessLogElement() {
                 @Override
                 public void addElement(CharArrayWriter buf, Date date, Request request, Response response, long time) {
-                    buf.append(wrap(request.getCharacterEncoding()));
+                    wrap(request.getCharacterEncoding(), buf);
                 }
             };
         } else if ("locale".equals(parameter)) {
             return new AccessLogElement() {
                 @Override
                 public void addElement(CharArrayWriter buf, Date date, Request request, Response response, long time) {
-                    buf.append(wrap(request.getLocale()));
+                    wrap(request.getLocale(), buf);
                 }
             };
         } else if ("protocol".equals(parameter)) {
             return new AccessLogElement() {
                 @Override
                 public void addElement(CharArrayWriter buf, Date date, Request request, Response response, long time) {
-                    buf.append(wrap(request.getProtocol()));
+                    wrap(request.getProtocol(), buf);
                 }
             };
         } else if ("scheme".equals(parameter)) {
@@ -765,7 +759,7 @@ public void addElement(CharArrayWriter buf, Date date, Request request, Response
             return new AccessLogElement() {
                 @Override
                 public void addElement(CharArrayWriter buf, Date date, Request request, Response response, long time) {
-                    buf.append(wrap("" + request.isSecure()));
+                    wrap(Boolean.valueOf(request.isSecure()), buf);
                 }
             };
         }

diff --git a/test/org/apache/catalina/valves/TestExtendedAccessLogValve.java b/test/org/apache/catalina/valves/TestExtendedAccessLogValve.java
@@ -52,7 +52,12 @@ public static Collection<Object[]> data() {
         patterns.add(new Object[]{"basic", "time cs-method cs-uri-stem cs-uri-query"});
         patterns.add(new Object[]{"ip", "time cs-method sc-status c-ip s-ip s-dns c-dns"});
         patterns.add(new Object[]{"headers", "time cs-method cs(Referer) cs(Cookie) sc(Content-Type)"});
-        patterns.add(new Object[]{"bytes", "date time cs-method cs-uri-stem bytes time-taken cached"});
+        patterns.add(new Object[]{"bytes", "date time cs-method cs-uri bytes time-taken cached"});
+        patterns.add(new Object[]{"tomcat1", "x-threadname x-A(testSCAttr) x-C(COOKIE-1_3) x-O(Custom)"});
+        patterns.add(new Object[]{"tomcat2", "x-R(testRAttr) x-S(sessionAttr) x-P(testParam)"});
+        patterns.add(new Object[]{"tomcat3", "x-H(authType) x-H(characterEncoding) x-H(contentLength)"});
+        patterns.add(new Object[]{"tomcat4", "x-H(locale) x-H(protocol) x-H(remoteUser) x-H(requestedSessionId)"});
+        patterns.add(new Object[]{"tomcat5", "x-H(requestedSessionIdFromCookie) x-H(requestedSessionIdValid) x-H(scheme) x-H(secure)"});
         return patterns;
     }
 
@@ -85,14 +90,19 @@ public void testLogFormat() throws Exception {
             private static final long serialVersionUID = 1L;
             @Override
             protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException {
+                req.getServletContext().setAttribute("testSCAttr", "testSCAttrValue");
+                req.getSession(true).setAttribute("sessionAttr", "sessionAttrValue");
+                req.setAttribute("testRAttr", "testRValue");
+                resp.addHeader("Custom", "value1");
+                resp.addHeader("Custom", "value2");
                 resp.getWriter().write("Test response");
             }
         });
         ctx.addServletMappingDecoded("/test", "testServlet");
 
         tomcat.start();
 
-        String url = "http://localhost:" + getPort() + "/test";
+        String url = "http://localhost:" + getPort() + "/test?testParam=testValue";
         ByteChunk out = new ByteChunk();
         Map<String, List<String>> reqHead = new HashMap<>();
         List<String> cookieHeaders = new ArrayList<>();
@@ -139,6 +149,8 @@ private void processLogContent(String content) {
         Assert.assertTrue("No data entries found", !dataLines.isEmpty());
 
         String entryLine = dataLines.get(0);
+        System.out.println(name + ": " + entryLine);
+
         String[] parts = entryLine.split("\\s+");
 
         String[] expectedFields = logPattern.split("\\s+");

diff --git a/test/org/apache/catalina/valves/TestExtendedAccessLogValveWrap.java b/test/org/apache/catalina/valves/TestExtendedAccessLogValveWrap.java
@@ -16,48 +16,66 @@
  */
 package org.apache.catalina.valves;
 
+import java.io.CharArrayWriter;
+
 import org.junit.Assert;
 import org.junit.Test;
 
 public class TestExtendedAccessLogValveWrap {
 
     @Test
     public void alpha() {
-        Assert.assertEquals("\"foo\"", ExtendedAccessLogValve.wrap("foo"));
+        CharArrayWriter buf = new CharArrayWriter();
+        ExtendedAccessLogValve.wrap("foo", buf);
+        Assert.assertEquals("\"foo\"", buf.toString());
     }
 
     @Test
     public void testNull() {
-        Assert.assertEquals("-", ExtendedAccessLogValve.wrap(null));
+        CharArrayWriter buf = new CharArrayWriter();
+        ExtendedAccessLogValve.wrap(null, buf);
+        Assert.assertEquals("-", buf.toString());
     }
 
     @Test
     public void empty() {
-        Assert.assertEquals("\"\"", ExtendedAccessLogValve.wrap(""));
+        CharArrayWriter buf = new CharArrayWriter();
+        ExtendedAccessLogValve.wrap("", buf);
+        Assert.assertEquals("\"\"", buf.toString());
     }
 
     @Test
     public void singleQuoteMiddle() {
-        Assert.assertEquals("\"foo'bar\"", ExtendedAccessLogValve.wrap("foo'bar"));
+        CharArrayWriter buf = new CharArrayWriter();
+        ExtendedAccessLogValve.wrap("foo'bar", buf);
+        Assert.assertEquals("\"foo'bar\"", buf.toString());
     }
 
     @Test
     public void doubleQuoteMiddle() {
-        Assert.assertEquals("\"foo\"\"bar\"", ExtendedAccessLogValve.wrap("foo\"bar"));
+        CharArrayWriter buf = new CharArrayWriter();
+        ExtendedAccessLogValve.wrap("foo\"bar", buf);
+        Assert.assertEquals("\"foo\"\"bar\"", buf.toString());
     }
 
     @Test
     public void doubleQuoteStart() {
-        Assert.assertEquals("\"\"\"foobar\"", ExtendedAccessLogValve.wrap("\"foobar"));
+        CharArrayWriter buf = new CharArrayWriter();
+        ExtendedAccessLogValve.wrap("\"foobar", buf);
+        Assert.assertEquals("\"\"\"foobar\"", buf.toString());
     }
 
     @Test
     public void doubleQuoteEnd() {
-        Assert.assertEquals("\"foobar\"\"\"", ExtendedAccessLogValve.wrap("foobar\""));
+        CharArrayWriter buf = new CharArrayWriter();
+        ExtendedAccessLogValve.wrap("foobar\"", buf);
+        Assert.assertEquals("\"foobar\"\"\"", buf.toString());
     }
 
     @Test
     public void doubleQuote() {
-        Assert.assertEquals("\"\"\"\"", ExtendedAccessLogValve.wrap("\""));
+        CharArrayWriter buf = new CharArrayWriter();
+        ExtendedAccessLogValve.wrap("\"", buf);
+        Assert.assertEquals("\"\"\"\"", buf.toString());
     }
 }