Start another round of code review

rmaucher · rmaucher · commit e053bfdb348c · 2026-06-19T23:25:54.000+02:00
Add a tweak to cors, needs review.
diff --git a/java/org/apache/catalina/AccessLog.java b/java/org/apache/catalina/AccessLog.java
@@ -69,11 +69,11 @@ public interface AccessLog {
      * Should this valve use request attributes for IP address, hostname, protocol and port used for the request? The
      * attributes used are:
      * <ul>
-     * <li>org.apache.catalina.RemoteAddr</li>
-     * <li>org.apache.catalina.RemoteHost</li>
-     * <li>org.apache.catalina.Protocol</li>
-     * <li>org.apache.catalina.ServerName</li>
-     * <li>org.apache.catalina.ServerPost</li>
+     * <li>org.apache.catalina.AccessLog.RemoteAddr</li>
+     * <li>org.apache.catalina.AccessLog.RemoteHost</li>
+     * <li>org.apache.catalina.AccessLog.Protocol</li>
+     * <li>org.apache.catalina.AccessLog.ServerName</li>
+     * <li>org.apache.catalina.AccessLog.ServerPort</li>
      * </ul>
      *
      * @param requestAttributesEnabled <code>true</code> causes the attributes to be used, <code>false</code> causes the
diff --git a/java/org/apache/catalina/Authenticator.java b/java/org/apache/catalina/Authenticator.java
@@ -37,7 +37,7 @@ public interface Authenticator {
      * @param request  Request we are processing
      * @param response Response we are populating
      *
-     * @return <code>true</code> if any specified constraints have been satisfied, or <code>false</code> if one more
+     * @return <code>true</code> if any specified constraints have been satisfied, or <code>false</code> if one or more
      *             constraints were not satisfied (in which case an authentication challenge will have been written to
      *             the response).
      *
diff --git a/java/org/apache/catalina/Context.java b/java/org/apache/catalina/Context.java
@@ -1618,7 +1618,7 @@ Set<String> addServletSecurity(ServletRegistration.Dynamic registration,
      * class - IllegalArgumentException will be thrown.
      *
      * @param clazz  Fully qualified class name
-     * @param method Post construct method name
+     * @param method Pre destroy method name
      *
      * @throws IllegalArgumentException if the fully qualified class name or method name are <code>NULL</code>; if there
      *                                      is already pre destroy method definition for the given class
diff --git a/java/org/apache/catalina/ant/AbstractCatalinaTask.java b/java/org/apache/catalina/ant/AbstractCatalinaTask.java
@@ -26,6 +26,7 @@
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.net.URLConnection;
+import java.nio.charset.StandardCharsets;
 
 import org.apache.catalina.util.IOTools;
 import org.apache.tomcat.util.http.Method;
@@ -46,13 +47,6 @@ public abstract class AbstractCatalinaTask extends BaseRedirectorHelperTask {
     protected AbstractCatalinaTask() {
     }
 
-    // ----------------------------------------------------- Instance Variables
-
-    /**
-     * manager webapp's encoding.
-     */
-    private static final String CHARSET = "utf-8";
-
 
     // ------------------------------------------------------------- Properties
 
@@ -270,7 +264,7 @@ public void execute(String command, InputStream istream, String contentType, lon
             }
 
             // Process the response message
-            reader = new InputStreamReader(hconn.getInputStream(), CHARSET);
+            reader = new InputStreamReader(hconn.getInputStream(), StandardCharsets.UTF_8);
             StringBuilder buff = new StringBuilder();
             String error = null;
             int msgPriority = Project.MSG_INFO;
diff --git a/java/org/apache/catalina/ant/JKStatusUpdateTask.java b/java/org/apache/catalina/ant/JKStatusUpdateTask.java
@@ -366,8 +366,9 @@ private StringBuilder createLink() {
                     sb.append("&ws=");
                     sb.append(workerStopped);
                 }
-                if ((workerRedirect != null)) { // other worker conrecte lb's
+                if ((workerRedirect != null)) { // redirect to other worker or load balancer
                     sb.append("&wr=");
+                    sb.append(URLEncoder.encode(workerRedirect, getCharset()));
                 }
                 if ((workerClusterDomain != null)) {
                     sb.append("&wc=");
@@ -400,10 +401,10 @@ protected void checkParameter() {
                 throw new BuildException(
                         "Must specify at a lb worker either" + "'lbStickySession' and 'lbForceSession' attribute");
             }
-            if (null != lbRecovertime && 60 < lbRecovertime.intValue()) {
+            if (null != lbRecovertime && lbRecovertime.intValue() < 60) {
                 throw new BuildException("The 'lbRecovertime' must be greater than 59");
             }
-            if (null != lbRetries && 1 < lbRetries.intValue()) {
+            if (null != lbRetries && lbRetries.intValue() < 2) {
                 throw new BuildException("The 'lbRetries' must be greater than 1");
             }
             isLBMode = true;
diff --git a/java/org/apache/catalina/authenticator/AuthenticatorBase.java b/java/org/apache/catalina/authenticator/AuthenticatorBase.java
@@ -671,8 +671,9 @@ protected boolean allowCorsPreflightBypass(Request request) {
             // This is a subset of the tests in CorsFilter.checkRequestType
             if (Method.OPTIONS.equals(request.getMethod())) {
                 String originHeader = request.getHeader(CorsFilter.REQUEST_HEADER_ORIGIN);
-                if (originHeader != null && !originHeader.isEmpty() && RequestUtil.isValidOrigin(originHeader) &&
-                        !RequestUtil.isSameOrigin(request, originHeader)) {
+                if (originHeader != null && !originHeader.isEmpty()
+                        && !"null".equals(originHeader) && RequestUtil.isValidOrigin(originHeader)
+                        && !RequestUtil.isSameOrigin(request, originHeader)) {
                     String accessControlRequestMethodHeader =
                             request.getHeader(CorsFilter.REQUEST_HEADER_ACCESS_CONTROL_REQUEST_METHOD);
                     if (accessControlRequestMethodHeader != null && !accessControlRequestMethodHeader.isEmpty()) {
@@ -1269,7 +1270,7 @@ public void login(String username, String password, Request request) throws Serv
      * @param username The user
      * @param password The password
      *
-     * @return The authenticated Principal
+     * @return The non null authenticated Principal
      *
      * @throws ServletException No principal was authenticated with the specified credentials
      */
diff --git a/java/org/apache/catalina/authenticator/BasicAuthenticator.java b/java/org/apache/catalina/authenticator/BasicAuthenticator.java
@@ -51,9 +51,8 @@ public BasicAuthenticator() {
 
     /**
      * Returns the character set used for encoding credentials, as set by the user.
-     * If the charset was null or empty, the effective charset will be ISO-8859-1.
      *
-     * @return the character set name
+     * @return the character set name, the default value is "UTF-8"
      */
     public String getCharset() {
         return charsetString;
@@ -65,6 +64,7 @@ public String getCharset() {
      * ISO-8859-1.
      *
      * @param charsetString the character set name
+     * @throws IllegalArgumentException if the charset is not supported
      */
     public void setCharset(String charsetString) {
         // Only acceptable options are null, "" or "UTF-8" (case-insensitive)
@@ -241,6 +241,7 @@ private void parseCredentials(byte[] decoded) throws IllegalArgumentException {
                 }
             }
 
+            // Tomcat allows a null password
             if (colon < 0) {
                 username = new String(decoded, charset);
                 // password will remain null!
diff --git a/java/org/apache/catalina/authenticator/LocalStrings.properties b/java/org/apache/catalina/authenticator/LocalStrings.properties
@@ -94,5 +94,6 @@ spnegoAuthenticator.ticketValidateFail=Failed to validate client supplied ticket
 
 sslAuthenticatorValve.authFailed=Authentication with the provided certificates failed
 sslAuthenticatorValve.http2=The context [{0}] in virtual host [{1}] is configured to use CLIENT-CERT authentication and [{2}] is configured to support HTTP/2. Use of CLIENT-CERT authentication is not compatible with the use of HTTP/2 unless certificateVerification is set to required.
+sslAuthenticatorValve.invalidContainer=The SSL authenticator valve must be associated with a Context container, with a parent Host and a parent Engine
 sslAuthenticatorValve.noCertificates=No certificates are included with this request
 sslAuthenticatorValve.tls13=The context [{0}] in virtual host [{1}] is configured to use CLIENT-CERT authentication and [{2}] is configured to support TLS 1.3 using JSSE. Use of CLIENT-CERT authentication is not compatible with the use of TLS 1.3 and JSSE unless certificateVerification is set to required.
diff --git a/java/org/apache/catalina/authenticator/SSLAuthenticator.java b/java/org/apache/catalina/authenticator/SSLAuthenticator.java
@@ -164,14 +164,17 @@ protected void startInternal() throws LifecycleException {
          */
         Container container = getContainer();
         if (!(container instanceof Context context)) {
+            log.warn(sm.getString("sslAuthenticatorValve.invalidContainer"));
             return;
         }
         container = context.getParent();
         if (!(container instanceof Host host)) {
+            log.warn(sm.getString("sslAuthenticatorValve.invalidContainer"));
             return;
         }
         container = host.getParent();
         if (!(container instanceof Engine engine)) {
+            log.warn(sm.getString("sslAuthenticatorValve.invalidContainer"));
             return;
         }
 
diff --git a/java/org/apache/catalina/authenticator/jaspic/SimpleServerAuthContext.java b/java/org/apache/catalina/authenticator/jaspic/SimpleServerAuthContext.java
@@ -49,7 +49,8 @@ public SimpleServerAuthContext(List<ServerAuthModule> modules) {
     /**
      * {@inheritDoc}
      * <p>
-     * Iterates through the configured modules and returns the first result that is not {@code SEND_FAILURE}.
+     * Iterates through the configured modules in order and returns the first result that is not
+     * {@code SEND_FAILURE}. If all modules return {@code SEND_FAILURE}, returns {@code SEND_FAILURE}.
      */
     @Override
     public AuthStatus validateRequest(MessageInfo messageInfo, Subject clientSubject, Subject serviceSubject)
diff --git a/java/org/apache/catalina/connector/Connector.java b/java/org/apache/catalina/connector/Connector.java
@@ -101,11 +101,13 @@ public Connector() {
     public Connector(String protocol) {
         configuredProtocol = protocol;
         ProtocolHandler p = null;
+        Exception ex = null;
         try {
             p = ProtocolHandler.create(protocol);
         } catch (Exception e) {
-            log.error(sm.getString("coyoteConnector.protocolHandlerInstantiationFailed"), e);
+            ex = e;
         }
+        protocolHandlerCreationException = ex;
         if (p != null) {
             protocolHandler = p;
             protocolHandlerClassName = protocolHandler.getClass().getName();
@@ -124,6 +126,7 @@ public Connector(String protocol) {
      * @param protocolHandler The protocol handler to use
      */
     public Connector(ProtocolHandler protocolHandler) {
+        protocolHandlerCreationException = null;
         protocolHandlerClassName = protocolHandler.getClass().getName();
         configuredProtocol = protocolHandlerClassName;
         this.protocolHandler = protocolHandler;
@@ -134,6 +137,13 @@ public Connector(ProtocolHandler protocolHandler) {
 
     // ----------------------------------------------------- Instance Variables
 
+
+    /**
+     * Exception creating the protocol handler.
+     */
+    protected final Throwable protocolHandlerCreationException;
+
+
     /**
      * The <code>Service</code> we are associated with (if any).
      */
@@ -1247,7 +1257,7 @@ protected void initInternal() throws LifecycleException {
         super.initInternal();
 
         if (protocolHandler == null) {
-            throw new LifecycleException(sm.getString("coyoteConnector.protocolHandlerInstantiationFailed"));
+            throw new LifecycleException(sm.getString("coyoteConnector.protocolHandlerInstantiationFailed"), protocolHandlerCreationException);
         }
 
         // Initialize adapter
diff --git a/java/org/apache/catalina/connector/CoyoteInputStream.java b/java/org/apache/catalina/connector/CoyoteInputStream.java
@@ -64,7 +64,7 @@ void clear() {
      * Prevent cloning the facade.
      */
     @Override
-    protected Object clone() throws CloneNotSupportedException {
+    public Object clone() throws CloneNotSupportedException {
         throw new CloneNotSupportedException();
     }
 
diff --git a/java/org/apache/catalina/connector/CoyoteOutputStream.java b/java/org/apache/catalina/connector/CoyoteOutputStream.java
@@ -64,7 +64,7 @@ protected CoyoteOutputStream(OutputBuffer ob) {
      * Prevent cloning the facade.
      */
     @Override
-    protected Object clone() throws CloneNotSupportedException {
+    public Object clone() throws CloneNotSupportedException {
         throw new CloneNotSupportedException();
     }
 
diff --git a/java/org/apache/catalina/connector/InputBuffer.java b/java/org/apache/catalina/connector/InputBuffer.java
@@ -175,7 +175,7 @@ public InputBuffer(int size, org.apache.coyote.Request coyoteRequest) {
     // --------------------------------------------------------- Public Methods
 
     /**
-     * Recycle the output buffer.
+     * Recycle the input buffer.
      */
     public void recycle() {
 
diff --git a/java/org/apache/catalina/connector/Request.java b/java/org/apache/catalina/connector/Request.java
@@ -1851,7 +1851,7 @@ public String getDecodedRequestURI() {
     /**
      * Get the decoded request URI.
      *
-     * @return the URL decoded request URI
+     * @return the URL decoded request URI as MessageBytes
      */
     public MessageBytes getDecodedRequestURIMB() {
         return coyoteRequest.decodedURI();
diff --git a/java/org/apache/catalina/connector/RequestFacade.java b/java/org/apache/catalina/connector/RequestFacade.java
@@ -80,7 +80,7 @@ public void clear() {
      * Prevent cloning the facade.
      */
     @Override
-    protected Object clone() throws CloneNotSupportedException {
+    public Object clone() throws CloneNotSupportedException {
         throw new CloneNotSupportedException();
     }
 
diff --git a/java/org/apache/catalina/connector/Response.java b/java/org/apache/catalina/connector/Response.java
@@ -445,9 +445,9 @@ public String getContentType() {
      * Return a PrintWriter that can be used to render error messages, regardless of whether a stream or writer has
      * already been acquired.
      *
-     * @return Writer which can be used for error reports. If the response is not an error report returned using
-     *             sendError or triggered by an unexpected exception thrown during the servlet processing (and only in
-     *             that case), null will be returned if the response stream has already been used.
+     * @return Writer which can be used for error reports. Returns null if the output
+     *             buffer is not in a new state (i.e., data has been written or headers
+     *             have been modified).
      *
      * @exception IOException if an input/output error occurs
      */
@@ -904,8 +904,12 @@ public void addCookie(final Cookie cookie) {
         if (header == null) {
             return;
         }
+        Context context = getContext();
+        if (context == null) {
+            return;
+        }
         // if we reached here, no exception, cookie is valid
-        addHeader("Set-Cookie", header, getContext().getCookieProcessor().getCharset());
+        addHeader("Set-Cookie", header, context.getCookieProcessor().getCharset());
     }
 
     /**
@@ -1088,10 +1092,12 @@ public Supplier<Map<String,String>> getTrailerFields() {
     @Override
     public String encodeRedirectURL(String url) {
         if (isEncodeable(toAbsolute(url))) {
-            return toEncoded(url, request.getSessionInternal().getIdInternal());
-        } else {
-            return url;
+            Session session = request.getSessionInternal();
+            if (session != null) {
+                return toEncoded(url, session.getIdInternal());
+            }
         }
+        return url;
     }
 
 
@@ -1102,7 +1108,7 @@ public String encodeURL(String url) {
         try {
             absolute = toAbsolute(url);
         } catch (IllegalArgumentException iae) {
-            // Relative URL
+            // URL construction failed
             return url;
         }
 
@@ -1414,7 +1420,7 @@ private static boolean doIsEncodeable(Context context, Request hreq, Session ses
      *
      * @param location URL to be (possibly) converted and then returned
      *
-     * @return the encoded URL
+     * @return the absolute URL
      *
      * @exception IllegalArgumentException if a MalformedURLException is thrown when converting the relative URL to an
      *                                         absolute one
diff --git a/java/org/apache/catalina/connector/ResponseFacade.java b/java/org/apache/catalina/connector/ResponseFacade.java
@@ -75,7 +75,7 @@ public void clear() {
      * Prevent cloning the facade.
      */
     @Override
-    protected Object clone() throws CloneNotSupportedException {
+    public Object clone() throws CloneNotSupportedException {
         throw new CloneNotSupportedException();
     }
 

Original file line number	Diff line number	Diff line change
`@@ -37,7 +37,7 @@ public interface Authenticator {`
`37`	`37`	`* @param request Request we are processing`
`38`	`38`	`* @param response Response we are populating`
`39`	`39`	`*`
`40`		`- * @return <code>true</code> if any specified constraints have been satisfied, or <code>false</code> if one more`
	`40`	`+ * @return <code>true</code> if any specified constraints have been satisfied, or <code>false</code> if one or more`
`41`	`41`	`* constraints were not satisfied (in which case an authentication challenge will have been written to`
`42`	`42`	`* the response).`
`43`	`43`	`*`
Original file line number	Diff line number	Diff line change
`@@ -1618,7 +1618,7 @@ Set<String> addServletSecurity(ServletRegistration.Dynamic registration,`
`1618`	`1618`	`* class - IllegalArgumentException will be thrown.`
`1619`	`1619`	`*`
`1620`	`1620`	`* @param clazz Fully qualified class name`
`1621`		`- * @param method Post construct method name`
	`1621`	`+ * @param method Pre destroy method name`
`1622`	`1622`	`*`
`1623`	`1623`	`* @throws IllegalArgumentException if the fully qualified class name or method name are <code>NULL</code>; if there`
`1624`	`1624`	`* is already pre destroy method definition for the given class`
Original file line number	Diff line number	Diff line change
`@@ -366,8 +366,9 @@ private StringBuilder createLink() {`
`366`	`366`	`sb.append("&ws=");`
`367`	`367`	`sb.append(workerStopped);`
`368`	`368`	`}`
`369`		`- if ((workerRedirect != null)) { // other worker conrecte lb's`
	`369`	`+ if ((workerRedirect != null)) { // redirect to other worker or load balancer`
`370`	`370`	`sb.append("&wr=");`
	`371`	`+ sb.append(URLEncoder.encode(workerRedirect, getCharset()));`
`371`	`372`	`}`
`372`	`373`	`if ((workerClusterDomain != null)) {`
`373`	`374`	`sb.append("&wc=");`
`@@ -400,10 +401,10 @@ protected void checkParameter() {`
`400`	`401`	`throw new BuildException(`
`401`	`402`	`"Must specify at a lb worker either" + "'lbStickySession' and 'lbForceSession' attribute");`
`402`	`403`	`}`
`403`		`- if (null != lbRecovertime && 60 < lbRecovertime.intValue()) {`
	`404`	`+ if (null != lbRecovertime && lbRecovertime.intValue() < 60) {`
`404`	`405`	`throw new BuildException("The 'lbRecovertime' must be greater than 59");`
`405`	`406`	`}`
`406`		`- if (null != lbRetries && 1 < lbRetries.intValue()) {`
	`407`	`+ if (null != lbRetries && lbRetries.intValue() < 2) {`
`407`	`408`	`throw new BuildException("The 'lbRetries' must be greater than 1");`
`408`	`409`	`}`
`409`	`410`	`isLBMode = true;`
Original file line number	Diff line number	Diff line change
`@@ -51,9 +51,8 @@ public BasicAuthenticator() {`
`51`	`51`
`52`	`52`	`/**`
`53`	`53`	`* Returns the character set used for encoding credentials, as set by the user.`
`54`		`- * If the charset was null or empty, the effective charset will be ISO-8859-1.`
`55`	`54`	`*`
`56`		`- * @return the character set name`
	`55`	`+ * @return the character set name, the default value is "UTF-8"`
`57`	`56`	`*/`
`58`	`57`	`public String getCharset() {`
`59`	`58`	`return charsetString;`
`@@ -65,6 +64,7 @@ public String getCharset() {`
`65`	`64`	`* ISO-8859-1.`
`66`	`65`	`*`
`67`	`66`	`* @param charsetString the character set name`
	`67`	`+ * @throws IllegalArgumentException if the charset is not supported`
`68`	`68`	`*/`
`69`	`69`	`public void setCharset(String charsetString) {`
`70`	`70`	`// Only acceptable options are null, "" or "UTF-8" (case-insensitive)`
`@@ -241,6 +241,7 @@ private void parseCredentials(byte[] decoded) throws IllegalArgumentException {`
`241`	`241`	`}`
`242`	`242`	`}`
`243`	`243`
	`244`	`+ // Tomcat allows a null password`
`244`	`245`	`if (colon < 0) {`
`245`	`246`	`username = new String(decoded, charset);`
`246`	`247`	`// password will remain null!`
Original file line number	Diff line number	Diff line change
`@@ -164,14 +164,17 @@ protected void startInternal() throws LifecycleException {`
`164`	`164`	`*/`
`165`	`165`	`Container container = getContainer();`
`166`	`166`	`if (!(container instanceof Context context)) {`
	`167`	`+ log.warn(sm.getString("sslAuthenticatorValve.invalidContainer"));`
`167`	`168`	`return;`
`168`	`169`	`}`
`169`	`170`	`container = context.getParent();`
`170`	`171`	`if (!(container instanceof Host host)) {`
	`172`	`+ log.warn(sm.getString("sslAuthenticatorValve.invalidContainer"));`
`171`	`173`	`return;`
`172`	`174`	`}`
`173`	`175`	`container = host.getParent();`
`174`	`176`	`if (!(container instanceof Engine engine)) {`
	`177`	`+ log.warn(sm.getString("sslAuthenticatorValve.invalidContainer"));`
`175`	`178`	`return;`
`176`	`179`	`}`
`177`	`180`