This is an automated email from the ASF dual-hosted git repository.
rmaucher pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new 6f3ecfb6fa Code review fixes
6f3ecfb6fa is described below
commit 6f3ecfb6faddc894fd9662af542b88ba6fac9c0d
Author: remm <[email protected]>
AuthorDate: Thu Jun 11 23:40:04 2026 +0200
Code review fixes
---
.../catalina/manager/HTMLManagerServlet.java | 4 +--
java/org/apache/catalina/manager/JspHelper.java | 14 --------
.../apache/catalina/manager/ManagerServlet.java | 40 +++++++++++++---------
.../catalina/manager/host/HostManagerServlet.java | 18 +++++-----
.../catalina/mbeans/ContextResourceLinkMBean.java | 6 +++-
.../catalina/mbeans/ContextResourceMBean.java | 6 +++-
.../mbeans/DataSourceUserDatabaseMBean.java | 6 ++--
.../apache/catalina/mbeans/LocalStrings.properties | 1 +
java/org/apache/catalina/mbeans/MBeanFactory.java | 9 +++--
java/org/apache/catalina/mbeans/MBeanUtils.java | 5 +--
.../catalina/mbeans/SparseUserDatabaseMBean.java | 6 ++--
.../realm/DigestCredentialHandlerBase.java | 8 ++++-
java/org/apache/catalina/realm/JAASRealm.java | 3 ++
java/org/apache/catalina/realm/JNDIRealm.java | 12 +++++--
java/org/apache/catalina/realm/LockOutRealm.java | 2 +-
15 files changed, 81 insertions(+), 59 deletions(-)
diff --git a/java/org/apache/catalina/manager/HTMLManagerServlet.java
b/java/org/apache/catalina/manager/HTMLManagerServlet.java
index a72d174f54..f624475280 100644
--- a/java/org/apache/catalina/manager/HTMLManagerServlet.java
+++ b/java/org/apache/catalina/manager/HTMLManagerServlet.java
@@ -327,7 +327,7 @@ public class HTMLManagerServlet extends ManagerServlet {
args[0] = getServletContext().getContextPath();
args[1] = smClient.getString("htmlManagerServlet.title");
if (htmlSubTitle != null) {
- args[1] += "</font><br/><font size=\"+1\">" + htmlSubTitle;
+ args[1] += "</font><br/><font size=\"+1\">" +
Escape.htmlElementContent(htmlSubTitle);
}
// HTML Header Section
@@ -1080,7 +1080,7 @@ public class HTMLManagerServlet extends ManagerServlet {
httpSession.removeAttribute(attributeName);
} catch (IllegalStateException ise) {
if (debug >= 1) {
- log("Cannot remote attribute '" + attributeName + "' for
invalidated session id " + sessionId);
+ log("Cannot remove attribute '" + attributeName + "' for
invalidated session id " + sessionId);
}
}
return wasPresent;
diff --git a/java/org/apache/catalina/manager/JspHelper.java
b/java/org/apache/catalina/manager/JspHelper.java
index 067f8a0ef0..7c661d75c9 100644
--- a/java/org/apache/catalina/manager/JspHelper.java
+++ b/java/org/apache/catalina/manager/JspHelper.java
@@ -202,20 +202,6 @@ public class JspHelper {
}
- /*
- * Following copied from org.apache.taglibs.standard.tag.common.core.Util
v1.1.2
- */
-
- private static final int HIGHEST_SPECIAL = '>';
- private static final char[][] specialCharactersRepresentation = new
char[HIGHEST_SPECIAL + 1][];
- static {
- specialCharactersRepresentation['&'] = "&".toCharArray();
- specialCharactersRepresentation['<'] = "<".toCharArray();
- specialCharactersRepresentation['>'] = ">".toCharArray();
- specialCharactersRepresentation['"'] = """.toCharArray();
- specialCharactersRepresentation['\''] = "'".toCharArray();
- }
-
/**
* Escapes XML special characters in the given object's string
representation.
*
diff --git a/java/org/apache/catalina/manager/ManagerServlet.java
b/java/org/apache/catalina/manager/ManagerServlet.java
index a87d6b0b0d..4f596ce916 100644
--- a/java/org/apache/catalina/manager/ManagerServlet.java
+++ b/java/org/apache/catalina/manager/ManagerServlet.java
@@ -28,6 +28,7 @@ import java.util.ArrayList;
import java.util.Arrays;
import java.util.Enumeration;
import java.util.HashMap;
+import java.util.Iterator;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Locale;
@@ -413,12 +414,14 @@ public class ManagerServlet extends HttpServlet
implements ContainerServlet {
}
// Set our properties from the initialization parameters
- String value;
- try {
- value = getServletConfig().getInitParameter("debug");
- debug = Integer.parseInt(value);
- } catch (Throwable t) {
- ExceptionUtils.handleThrowable(t);
+ String value = getServletConfig().getInitParameter("debug");
+ if (value != null) {
+ try {
+ debug = Integer.parseInt(value);
+ } catch (NumberFormatException e) {
+ // Ignore
+ debug = 0;
+ }
}
// Acquire global JNDI resources if available
@@ -1702,18 +1705,21 @@ public class ManagerServlet extends HttpServlet
implements ContainerServlet {
for (SSLHostConfig sslHostConfig : sslHostConfigs) {
String name = connector.toString() + "-" +
sslHostConfig.getHostName();
List<String> certList = new ArrayList<>();
- SSLContext sslContext =
sslHostConfig.getCertificates().iterator().next().getSslContext();
- if (sslContext == null) {
-
certList.add(smClient.getString("managerServlet.certsNotLoaded"));
- } else {
- X509Certificate[] certs =
sslContext.getAcceptedIssuers();
- if (certs == null) {
-
certList.add(smClient.getString("managerServlet.certsNotAvailable"));
- } else if (certs.length == 0) {
-
certList.add(smClient.getString("managerServlet.trustedCertsNotConfigured"));
+ Iterator<SSLHostConfigCertificate> certificates =
sslHostConfig.getCertificates().iterator();
+ while (certificates.hasNext()) {
+ SSLContext sslContext =
certificates.next().getSslContext();
+ if (sslContext == null) {
+
certList.add(smClient.getString("managerServlet.certsNotLoaded"));
} else {
- for (Certificate cert : certs) {
- certList.add(cert.toString());
+ X509Certificate[] certs =
sslContext.getAcceptedIssuers();
+ if (certs == null) {
+
certList.add(smClient.getString("managerServlet.certsNotAvailable"));
+ } else if (certs.length == 0) {
+
certList.add(smClient.getString("managerServlet.trustedCertsNotConfigured"));
+ } else {
+ for (Certificate cert : certs) {
+ certList.add(cert.toString());
+ }
}
}
}
diff --git a/java/org/apache/catalina/manager/host/HostManagerServlet.java
b/java/org/apache/catalina/manager/host/HostManagerServlet.java
index 00b45e1277..8a528cf0ac 100644
--- a/java/org/apache/catalina/manager/host/HostManagerServlet.java
+++ b/java/org/apache/catalina/manager/host/HostManagerServlet.java
@@ -45,7 +45,6 @@ import org.apache.catalina.Wrapper;
import org.apache.catalina.core.ContainerBase;
import org.apache.catalina.core.StandardHost;
import org.apache.catalina.startup.HostConfig;
-import org.apache.tomcat.util.ExceptionUtils;
import org.apache.tomcat.util.buf.StringUtils;
import org.apache.tomcat.util.res.StringManager;
@@ -278,12 +277,14 @@ public class HostManagerServlet extends HttpServlet
implements ContainerServlet
}
// Set our properties from the initialization parameters
- String value;
- try {
- value = getServletConfig().getInitParameter("debug");
- debug = Integer.parseInt(value);
- } catch (Throwable t) {
- ExceptionUtils.handleThrowable(t);
+ String value = getServletConfig().getInitParameter("debug");
+ if (value != null) {
+ try {
+ debug = Integer.parseInt(value);
+ } catch (NumberFormatException e) {
+ // Ignore
+ debug = 0;
+ }
}
}
@@ -606,13 +607,12 @@ public class HostManagerServlet extends HttpServlet
implements ContainerServlet
writer.println(smClient.getString("hostManagerServlet.persisted"));
} catch (Exception e) {
getServletContext().log(sm.getString("hostManagerServlet.persistFailed"), e);
-
writer.println(smClient.getString("hostManagerServlet.persistFailed"));
// catch InstanceNotFoundException when StoreConfig is not enabled
instead of printing
// the failure message
if (e instanceof InstanceNotFoundException) {
writer.println(smClient.getString("hostManagerServlet.noStoreConfig"));
} else {
-
writer.println(smClient.getString("hostManagerServlet.exception",
e.toString()));
+
writer.println(smClient.getString("hostManagerServlet.persistFailed"));
}
}
}
diff --git a/java/org/apache/catalina/mbeans/ContextResourceLinkMBean.java
b/java/org/apache/catalina/mbeans/ContextResourceLinkMBean.java
index 6e3159eec0..0ee4ef4aab 100644
--- a/java/org/apache/catalina/mbeans/ContextResourceLinkMBean.java
+++ b/java/org/apache/catalina/mbeans/ContextResourceLinkMBean.java
@@ -22,6 +22,8 @@ import javax.management.MBeanException;
import javax.management.ReflectionException;
import javax.management.RuntimeOperationsException;
+import org.apache.juli.logging.Log;
+import org.apache.juli.logging.LogFactory;
import org.apache.tomcat.util.descriptor.web.ContextResourceLink;
import org.apache.tomcat.util.descriptor.web.NamingResources;
import org.apache.tomcat.util.res.StringManager;
@@ -38,6 +40,7 @@ public class ContextResourceLinkMBean extends
BaseCatalinaMBean<ContextResourceL
public ContextResourceLinkMBean() {
}
+ protected static final Log log =
LogFactory.getLog(ContextResourceLinkMBean.class);
private static final StringManager sm =
StringManager.getManager(ContextResourceLinkMBean.class);
@Override
@@ -95,7 +98,8 @@ public class ContextResourceLinkMBean extends
BaseCatalinaMBean<ContextResourceL
switch (name) {
case "global" -> crl.setGlobal((String) value);
case "description" -> crl.setDescription((String) value);
- case "name" -> crl.setName((String) value);
+ // Updating the name actually needs removing and adding back the
component under the new name
+ case "name" -> log.info(sm.getString("mBean.nameChange"));
case "type" -> crl.setType((String) value);
default -> crl.setProperty(name, "" + value);
}
diff --git a/java/org/apache/catalina/mbeans/ContextResourceMBean.java
b/java/org/apache/catalina/mbeans/ContextResourceMBean.java
index 6daa3f2000..6f2e15359a 100644
--- a/java/org/apache/catalina/mbeans/ContextResourceMBean.java
+++ b/java/org/apache/catalina/mbeans/ContextResourceMBean.java
@@ -22,6 +22,8 @@ import javax.management.MBeanException;
import javax.management.ReflectionException;
import javax.management.RuntimeOperationsException;
+import org.apache.juli.logging.Log;
+import org.apache.juli.logging.LogFactory;
import org.apache.tomcat.util.descriptor.web.ContextResource;
import org.apache.tomcat.util.descriptor.web.NamingResources;
import org.apache.tomcat.util.res.StringManager;
@@ -38,6 +40,7 @@ public class ContextResourceMBean extends
BaseCatalinaMBean<ContextResource> {
public ContextResourceMBean() {
}
+ protected static final Log log =
LogFactory.getLog(ContextResourceMBean.class);
private static final StringManager sm =
StringManager.getManager(ContextResourceMBean.class);
@Override
@@ -96,7 +99,8 @@ public class ContextResourceMBean extends
BaseCatalinaMBean<ContextResource> {
switch (name) {
case "auth" -> cr.setAuth((String) value);
case "description" -> cr.setDescription((String) value);
- case "name" -> cr.setName((String) value);
+ // Updating the name actually needs removing and adding back the
component under the new name
+ case "name" -> log.info(sm.getString("mBean.nameChange"));
case "scope" -> cr.setScope((String) value);
case "type" -> cr.setType((String) value);
default -> cr.setProperty(name, "" + value);
diff --git a/java/org/apache/catalina/mbeans/DataSourceUserDatabaseMBean.java
b/java/org/apache/catalina/mbeans/DataSourceUserDatabaseMBean.java
index 866d3917e8..d6e76365db 100644
--- a/java/org/apache/catalina/mbeans/DataSourceUserDatabaseMBean.java
+++ b/java/org/apache/catalina/mbeans/DataSourceUserDatabaseMBean.java
@@ -115,7 +115,7 @@ public class DataSourceUserDatabaseMBean extends
BaseModelMBean {
/**
* Create a new Group and return the corresponding name.
*
- * @param groupname Group name of the new group
+ * @param groupname Group name of the new group
* @param description Description of the new group
*
* @return the new group name
@@ -130,8 +130,8 @@ public class DataSourceUserDatabaseMBean extends
BaseModelMBean {
/**
* Create a new Role and return the corresponding name.
*
- * @param rolename Group name of the new group
- * @param description Description of the new group
+ * @param rolename Role name of the new role
+ * @param description Description of the new role
*
* @return the new role name
*/
diff --git a/java/org/apache/catalina/mbeans/LocalStrings.properties
b/java/org/apache/catalina/mbeans/LocalStrings.properties
index 138b3a36d1..c53fac189c 100644
--- a/java/org/apache/catalina/mbeans/LocalStrings.properties
+++ b/java/org/apache/catalina/mbeans/LocalStrings.properties
@@ -27,6 +27,7 @@ globalResources.userDatabaseCreateError=Exception creating
UserDatabase MBeans f
listener.notServer=This listener must only be nested within Server elements,
but is in [{0}].
mBean.attributeNotFound=Cannot find attribute [{0}]
+mBean.nameChange=Cannot update name property
mBean.nullAttribute=Attribute is null
mBean.nullName=Attribute name is null
diff --git a/java/org/apache/catalina/mbeans/MBeanFactory.java
b/java/org/apache/catalina/mbeans/MBeanFactory.java
index f77689ad34..a782328fcc 100644
--- a/java/org/apache/catalina/mbeans/MBeanFactory.java
+++ b/java/org/apache/catalina/mbeans/MBeanFactory.java
@@ -497,7 +497,7 @@ public class MBeanFactory {
*
* @param domain Domain name for the container instance
* @param defaultHost Name of the default host to be used in the Engine
- * @param baseDir Base directory value for Engine
+ * @param baseDir Base directory value for Engine, not used anymore
*
* @return the object name of the created service
*
@@ -805,7 +805,9 @@ public class MBeanFactory {
ObjectName oname = new ObjectName(name);
// Acquire a reference to the component to be removed
Container container = getParentContainerFromChild(oname);
- container.setRealm(null);
+ if (container != null) {
+ container.setRealm(null);
+ }
}
@@ -841,6 +843,9 @@ public class MBeanFactory {
// Acquire a reference to the component to be removed
ObjectName oname = new ObjectName(name);
Container container = getParentContainerFromChild(oname);
+ if (container == null) {
+ return;
+ }
Valve[] valves = container.getPipeline().getValves();
for (Valve valve : valves) {
if (valve instanceof JmxEnabled) {
diff --git a/java/org/apache/catalina/mbeans/MBeanUtils.java
b/java/org/apache/catalina/mbeans/MBeanUtils.java
index 979c72937f..4b20476a3d 100644
--- a/java/org/apache/catalina/mbeans/MBeanUtils.java
+++ b/java/org/apache/catalina/mbeans/MBeanUtils.java
@@ -361,15 +361,16 @@ public class MBeanUtils {
throws MalformedObjectNameException {
ObjectName name = null;
+ String quotedEnvironmentName = ObjectName.quote(environment.getName());
Object container = environment.getNamingResources().getContainer();
if (container instanceof Server) {
- name = new ObjectName(domain + ":type=Environment" +
",resourcetype=Global,name=" + environment.getName());
+ name = new ObjectName(domain + ":type=Environment" +
",resourcetype=Global,name=" + quotedEnvironmentName);
} else if (container instanceof Context) {
Context context = ((Context) container);
ContextName cn = new ContextName(context.getName(), false);
Container host = context.getParent();
name = new ObjectName(domain + ":type=Environment" +
",resourcetype=Context,host=" + host.getName() +
- ",context=" + cn.getDisplayName() + ",name=" +
environment.getName());
+ ",context=" + cn.getDisplayName() + ",name=" +
quotedEnvironmentName);
}
return name;
diff --git a/java/org/apache/catalina/mbeans/SparseUserDatabaseMBean.java
b/java/org/apache/catalina/mbeans/SparseUserDatabaseMBean.java
index 26ccdb4565..50bff322b2 100644
--- a/java/org/apache/catalina/mbeans/SparseUserDatabaseMBean.java
+++ b/java/org/apache/catalina/mbeans/SparseUserDatabaseMBean.java
@@ -79,7 +79,7 @@ public class SparseUserDatabaseMBean extends BaseModelMBean {
/**
- * The <code>ManagedBean</code> information describing Group MBeans.
+ * The <code>ManagedBean</code> information describing Role MBeans.
*/
protected final ManagedBean managedRole = registry.findManagedBean("Role");
@@ -171,8 +171,8 @@ public class SparseUserDatabaseMBean extends BaseModelMBean
{
/**
* Create a new Role and return the corresponding MBean Name.
*
- * @param rolename Group name of the new group
- * @param description Description of the new group
+ * @param rolename Role name of the new role
+ * @param description Description of the new role
*
* @return the new role object name
*/
diff --git a/java/org/apache/catalina/realm/DigestCredentialHandlerBase.java
b/java/org/apache/catalina/realm/DigestCredentialHandlerBase.java
index e249016a43..1d626d2cad 100644
--- a/java/org/apache/catalina/realm/DigestCredentialHandlerBase.java
+++ b/java/org/apache/catalina/realm/DigestCredentialHandlerBase.java
@@ -189,7 +189,13 @@ public abstract class DigestCredentialHandlerBase
implements CredentialHandler {
String hexSalt = storedCredentials.substring(0, sep1);
- int iterations = Integer.parseInt(storedCredentials.substring(sep1 +
1, sep2));
+ int iterations;
+ try {
+ iterations = Integer.parseInt(storedCredentials.substring(sep1 +
1, sep2));
+ } catch (NumberFormatException e) {
+ logInvalidStoredCredentials(storedCredentials);
+ return false;
+ }
String storedHexEncoded = storedCredentials.substring(sep2 + 1);
byte[] salt;
diff --git a/java/org/apache/catalina/realm/JAASRealm.java
b/java/org/apache/catalina/realm/JAASRealm.java
index ca2efb833e..afd3521c72 100644
--- a/java/org/apache/catalina/realm/JAASRealm.java
+++ b/java/org/apache/catalina/realm/JAASRealm.java
@@ -620,6 +620,9 @@ public class JAASRealm extends RealmBase {
jaasConfigurationLoaded = true;
return null;
}
+ if (jaasConfigurationLoaded) {
+ return jaasConfiguration;
+ }
@SuppressWarnings("unchecked")
Class<Configuration> sunConfigFile =
(Class<Configuration>)
Class.forName("com.sun.security.auth.login.ConfigFile");
diff --git a/java/org/apache/catalina/realm/JNDIRealm.java
b/java/org/apache/catalina/realm/JNDIRealm.java
index 0c3b3a031d..874d2258f0 100644
--- a/java/org/apache/catalina/realm/JNDIRealm.java
+++ b/java/org/apache/catalina/realm/JNDIRealm.java
@@ -2323,8 +2323,14 @@ public class JNDIRealm extends RealmBase {
NamingEnumeration<?> e = attr.getAll();
try {
while (e.hasMore()) {
- String value = (String) e.next();
- values.add(value);
+ Object value = e.next();
+ String valueString;
+ if (value instanceof byte[]) {
+ valueString = new String((byte[]) value);
+ } else {
+ valueString = value.toString();
+ }
+ values.add(valueString);
}
} catch (PartialResultException ex) {
if (!adCompat) {
@@ -2679,7 +2685,7 @@ public class JNDIRealm extends RealmBase {
@Override
public boolean isAvailable() {
// Simple best effort check
- return (connectionPool != null || singleConnection.context != null);
+ return (connectionPool != null || (singleConnection != null &&
singleConnection.context != null));
}
diff --git a/java/org/apache/catalina/realm/LockOutRealm.java
b/java/org/apache/catalina/realm/LockOutRealm.java
index e019a9c643..db602b0d71 100644
--- a/java/org/apache/catalina/realm/LockOutRealm.java
+++ b/java/org/apache/catalina/realm/LockOutRealm.java
@@ -395,7 +395,7 @@ public class LockOutRealm extends CombinedRealm {
/**
* The time of the last failure.
*/
- private long lastFailureTime = 0;
+ private volatile long lastFailureTime = 0;
/**
* Default constructor.
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
