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
commit 36020e7f936c37aab18caccaf0a0f266af054207 Author: opencode <[email protected]> AuthorDate: Fri Oct 9 10:03:54 2026 +0200 Fix storeconfig default-manager check always rejecting started managers ManagerBase.startInternal() always propagates the Engine's jvmRoute (null by default) to the session ID generator, overwriting its empty-string default. The isDefaultManager() check in ManagerSF compared the generator's jvmRoute against "", so it returned false for every started StandardManager and storeConfig wrote a redundant <Manager> element with a nested <SessionIdGenerator> for every context using the default configuration. Treat a generator jvmRoute that is empty or equal to the value the manager propagates as runtime state rather than configuration, and treat a not yet created generator as default. Since making the skip path reachable would otherwise silently drop configuration, require the exact manager and generator classes and check the persistable throwOnFailure property. --- .../org/apache/catalina/storeconfig/ManagerSF.java | 54 +++-- .../catalina/storeconfig/TestStoreConfig.java | 249 +++++++++++++++++++++ webapps/docs/changelog.xml | 14 ++ 3 files changed, 300 insertions(+), 17 deletions(-) diff --git a/java/org/apache/catalina/storeconfig/ManagerSF.java b/java/org/apache/catalina/storeconfig/ManagerSF.java index ee29281d94..ad38c89afd 100644 --- a/java/org/apache/catalina/storeconfig/ManagerSF.java +++ b/java/org/apache/catalina/storeconfig/ManagerSF.java @@ -68,11 +68,21 @@ public class ManagerSF extends StoreFactoryBase { */ protected boolean isDefaultManager(StandardManager smanager) { + // A custom sub-class is configured through its className attribute + if (!StandardManager.class.equals(smanager.getClass())) { + return false; + } + // StandardManager-specific property if (smanager.getPathname() != null) { return false; } + // LifecycleBase property + if (!smanager.getThrowOnFailure()) { + return false; + } + // ManagerBase properties if (smanager.getMaxActiveSessions() != -1) { return false; @@ -113,26 +123,36 @@ public class ManagerSF extends StoreFactoryBase { if (smanager.getSessionLastAccessAtStart() != Globals.STRICT_SERVLET_COMPLIANCE) { return false; } + // A generator that has not been created yet is the default: the manager + // creates one when it starts SessionIdGenerator sessionIdGenerator = smanager.getSessionIdGenerator(); - SessionIdGeneratorBase sigBase = null; - if (sessionIdGenerator == null || !StandardSessionIdGenerator.class.isInstance(sessionIdGenerator)) { + if (sessionIdGenerator != null && + !StandardSessionIdGenerator.class.equals(sessionIdGenerator.getClass())) { return false; } - sigBase = (SessionIdGeneratorBase) sessionIdGenerator; - if (!"".equals(sigBase.getJvmRoute())) { - return false; - } - if (sigBase.getSecureRandomClass() != null) { - return false; - } - if (!SessionIdGeneratorBase.DEFAULT_SECURE_RANDOM_ALGORITHM.equals(sigBase.getSecureRandomAlgorithm())) { - return false; - } - if (sigBase.getSecureRandomProvider() != null) { - return false; - } - if (sigBase.getSessionIdLength() != 16) { - return false; + if (sessionIdGenerator instanceof SessionIdGeneratorBase sigBase) { + // The manager propagates its jvmRoute to the generator when it starts, + // so a value matching the manager's is runtime state, not configuration + String sigJvmRoute = sigBase.getJvmRoute(); + if (sigJvmRoute != null && !sigJvmRoute.isEmpty() && + !sigJvmRoute.equals(smanager.getJvmRoute())) { + return false; + } + if (!sigBase.getThrowOnFailure()) { + return false; + } + if (sigBase.getSecureRandomClass() != null) { + return false; + } + if (!SessionIdGeneratorBase.DEFAULT_SECURE_RANDOM_ALGORITHM.equals(sigBase.getSecureRandomAlgorithm())) { + return false; + } + if (sigBase.getSecureRandomProvider() != null) { + return false; + } + if (sigBase.getSessionIdLength() != 16) { + return false; + } } return true; diff --git a/test/org/apache/catalina/storeconfig/TestStoreConfig.java b/test/org/apache/catalina/storeconfig/TestStoreConfig.java index b287846b7d..9d97398bed 100644 --- a/test/org/apache/catalina/storeconfig/TestStoreConfig.java +++ b/test/org/apache/catalina/storeconfig/TestStoreConfig.java @@ -34,11 +34,13 @@ import org.apache.catalina.Host; import org.apache.catalina.connector.Connector; import org.apache.catalina.core.StandardContext; import org.apache.catalina.realm.LockOutRealm; +import org.apache.catalina.session.StandardManager; import org.apache.catalina.startup.Catalina; import org.apache.catalina.startup.CatalinaBaseConfigurationSource; import org.apache.catalina.startup.Tomcat; import org.apache.catalina.startup.TomcatBaseTest; import org.apache.catalina.util.IOTools; +import org.apache.catalina.util.SessionIdGeneratorBase; import org.apache.catalina.valves.AccessLogValve; import org.apache.tomcat.util.net.SSLHostConfigPreSharedKey; import org.xml.sax.InputSource; @@ -330,6 +332,253 @@ public class TestStoreConfig extends TomcatBaseTest { .parse(new InputSource(new StringReader(contextXmlDump))); } + /** + * Verify that the <Manager> element is not stored for a running context using the default StandardManager + * configuration. The jvmRoute of the session ID generator is runtime state that the manager propagates from the + * Engine when it starts and must not defeat the default detection. + * + * @throws Exception if the test experiences an unexpected error + */ + @Test + public void testDefaultManagerNotStored() throws Exception { + Tomcat tomcat = getTomcatInstance(); + StoreConfigLifecycleListener storeConfigListener = new StoreConfigLifecycleListener(); + tomcat.getServer().addLifecycleListener(storeConfigListener); + + // Use a storable realm. The default embedded realm (Tomcat.SimpleRealm) is an inner class that the store + // path cannot instantiate a default instance of. + tomcat.getEngine().setRealm(new LockOutRealm()); + + File appDir = new File(getTemporaryDirectory(), "webapps/defmgr"); + if (!appDir.mkdirs()) { + Assert.fail("Unable to create the webapp directory"); + } + Context context = tomcat.addContext("/defmgr", appDir.getAbsolutePath()); + ((StandardContext) context).setDeployedFromServerXml(true); + + File conf = new File(getTemporaryDirectory(), "conf"); + if (!conf.isDirectory() && !conf.mkdirs()) { + Assert.fail("Unable to create conf directory"); + } + addDeleteOnTearDown(conf); + + tomcat.start(); + + // Undo the FastNonSecureRandom configuration the test base applies to every manager, so the started manager + // is in the same (default) state as it would be on a regular server + StandardManager manager = (StandardManager) ((StandardContext) context).getManager(); + manager.setSecureRandomClass(null); + ((SessionIdGeneratorBase) manager.getSessionIdGenerator()).setSecureRandomClass(null); + + IStoreConfig storeConfig = storeConfigListener.getStoreConfig(); + StoreDescription desc = storeConfig.getRegistry().findDescription(StandardContext.class); + Assert.assertNotNull(desc); + boolean oldSeparate = desc.isStoreSeparate(); + boolean oldExternalAllowed = desc.isExternalAllowed(); + boolean oldExternalOnly = desc.isExternalOnly(); + String serverXmlDump; + try { + desc.setStoreSeparate(true); + desc.setExternalAllowed(true); + desc.setExternalOnly(false); + StringWriter buffer = new StringWriter(); + storeConfig.store(new PrintWriter(buffer), -2, tomcat.getServer()); + serverXmlDump = buffer.toString(); + } finally { + desc.setStoreSeparate(oldSeparate); + desc.setExternalAllowed(oldExternalAllowed); + desc.setExternalOnly(oldExternalOnly); + } + + Assert.assertTrue(serverXmlDump, serverXmlDump.contains("path=\"/defmgr\"")); + // The manager of the started context is at its default configuration, so no <Manager> element may be stored + Assert.assertFalse(serverXmlDump, serverXmlDump.contains("<Manager")); + Assert.assertFalse(serverXmlDump, serverXmlDump.contains("StandardManager")); + Assert.assertFalse(serverXmlDump, serverXmlDump.contains("SessionIdGenerator")); + } + + /** + * Verify that a <Manager> element is still stored for a context whose StandardManager deviates from the + * default configuration. + * + * @throws Exception if the test experiences an unexpected error + */ + @Test + public void testNonDefaultManagerStored() throws Exception { + Tomcat tomcat = getTomcatInstance(); + StoreConfigLifecycleListener storeConfigListener = new StoreConfigLifecycleListener(); + tomcat.getServer().addLifecycleListener(storeConfigListener); + + // Use a storable realm. The default embedded realm (Tomcat.SimpleRealm) is an inner class that the store + // path cannot instantiate a default instance of. + tomcat.getEngine().setRealm(new LockOutRealm()); + + File appDir = new File(getTemporaryDirectory(), "webapps/nondflmgr"); + if (!appDir.mkdirs()) { + Assert.fail("Unable to create the webapp directory"); + } + Context context = tomcat.addContext("/nondflmgr", appDir.getAbsolutePath()); + ((StandardContext) context).setDeployedFromServerXml(true); + StandardManager manager = new StandardManager(); + manager.setMaxActiveSessions(100); + ((StandardContext) context).setManager(manager); + + File conf = new File(getTemporaryDirectory(), "conf"); + if (!conf.isDirectory() && !conf.mkdirs()) { + Assert.fail("Unable to create conf directory"); + } + addDeleteOnTearDown(conf); + + tomcat.start(); + + IStoreConfig storeConfig = storeConfigListener.getStoreConfig(); + StoreDescription desc = storeConfig.getRegistry().findDescription(StandardContext.class); + Assert.assertNotNull(desc); + boolean oldSeparate = desc.isStoreSeparate(); + boolean oldExternalAllowed = desc.isExternalAllowed(); + boolean oldExternalOnly = desc.isExternalOnly(); + String serverXmlDump; + try { + desc.setStoreSeparate(true); + desc.setExternalAllowed(true); + desc.setExternalOnly(false); + StringWriter buffer = new StringWriter(); + storeConfig.store(new PrintWriter(buffer), -2, tomcat.getServer()); + serverXmlDump = buffer.toString(); + } finally { + desc.setStoreSeparate(oldSeparate); + desc.setExternalAllowed(oldExternalAllowed); + desc.setExternalOnly(oldExternalOnly); + } + + Assert.assertTrue(serverXmlDump, serverXmlDump.contains("<Manager")); + Assert.assertTrue(serverXmlDump, serverXmlDump.contains("maxActiveSessions=\"100\"")); + } + + /** + * Verify that a <Manager> element with a StandardManager sub-class is still stored, since the sub-class is + * configured through its class name even when all inherited properties are at their defaults. + * + * @throws Exception if the test experiences an unexpected error + */ + @Test + public void testSubClassManagerStored() throws Exception { + Tomcat tomcat = getTomcatInstance(); + StoreConfigLifecycleListener storeConfigListener = new StoreConfigLifecycleListener(); + tomcat.getServer().addLifecycleListener(storeConfigListener); + + // Use a storable realm. The default embedded realm (Tomcat.SimpleRealm) is an inner class that the store + // path cannot instantiate a default instance of. + tomcat.getEngine().setRealm(new LockOutRealm()); + + File appDir = new File(getTemporaryDirectory(), "webapps/subcmgr"); + if (!appDir.mkdirs()) { + Assert.fail("Unable to create the webapp directory"); + } + Context context = tomcat.addContext("/subcmgr", appDir.getAbsolutePath()); + ((StandardContext) context).setDeployedFromServerXml(true); + ((StandardContext) context).setManager(new CustomTestManager()); + + File conf = new File(getTemporaryDirectory(), "conf"); + if (!conf.isDirectory() && !conf.mkdirs()) { + Assert.fail("Unable to create conf directory"); + } + addDeleteOnTearDown(conf); + + tomcat.start(); + + IStoreConfig storeConfig = storeConfigListener.getStoreConfig(); + StoreDescription desc = storeConfig.getRegistry().findDescription(StandardContext.class); + Assert.assertNotNull(desc); + boolean oldSeparate = desc.isStoreSeparate(); + boolean oldExternalAllowed = desc.isExternalAllowed(); + boolean oldExternalOnly = desc.isExternalOnly(); + String serverXmlDump; + try { + desc.setStoreSeparate(true); + desc.setExternalAllowed(true); + desc.setExternalOnly(false); + StringWriter buffer = new StringWriter(); + storeConfig.store(new PrintWriter(buffer), -2, tomcat.getServer()); + serverXmlDump = buffer.toString(); + } finally { + desc.setStoreSeparate(oldSeparate); + desc.setExternalAllowed(oldExternalAllowed); + desc.setExternalOnly(oldExternalOnly); + } + + Assert.assertTrue(serverXmlDump, serverXmlDump.contains("<Manager")); + Assert.assertTrue(serverXmlDump, serverXmlDump.contains(CustomTestManager.class.getName())); + } + + /** + * Verify that a <Manager> element is stored when the only deviation from the default configuration is a + * non-default <code>throwOnFailure</code> value, which is a persistable property that must not be lost. + * + * @throws Exception if the test experiences an unexpected error + */ + @Test + public void testManagerThrowOnFailureStored() throws Exception { + Tomcat tomcat = getTomcatInstance(); + StoreConfigLifecycleListener storeConfigListener = new StoreConfigLifecycleListener(); + tomcat.getServer().addLifecycleListener(storeConfigListener); + + // Use a storable realm. The default embedded realm (Tomcat.SimpleRealm) is an inner class that the store + // path cannot instantiate a default instance of. + tomcat.getEngine().setRealm(new LockOutRealm()); + + File appDir = new File(getTemporaryDirectory(), "webapps/tofmgr"); + if (!appDir.mkdirs()) { + Assert.fail("Unable to create the webapp directory"); + } + Context context = tomcat.addContext("/tofmgr", appDir.getAbsolutePath()); + ((StandardContext) context).setDeployedFromServerXml(true); + + File conf = new File(getTemporaryDirectory(), "conf"); + if (!conf.isDirectory() && !conf.mkdirs()) { + Assert.fail("Unable to create conf directory"); + } + addDeleteOnTearDown(conf); + + tomcat.start(); + + // Undo the FastNonSecureRandom configuration the test base applies to every manager, so throwOnFailure is the + // only deviation from the default manager configuration + StandardManager manager = (StandardManager) ((StandardContext) context).getManager(); + manager.setSecureRandomClass(null); + ((SessionIdGeneratorBase) manager.getSessionIdGenerator()).setSecureRandomClass(null); + manager.setThrowOnFailure(false); + + IStoreConfig storeConfig = storeConfigListener.getStoreConfig(); + StoreDescription desc = storeConfig.getRegistry().findDescription(StandardContext.class); + Assert.assertNotNull(desc); + boolean oldSeparate = desc.isStoreSeparate(); + boolean oldExternalAllowed = desc.isExternalAllowed(); + boolean oldExternalOnly = desc.isExternalOnly(); + String serverXmlDump; + try { + desc.setStoreSeparate(true); + desc.setExternalAllowed(true); + desc.setExternalOnly(false); + StringWriter buffer = new StringWriter(); + storeConfig.store(new PrintWriter(buffer), -2, tomcat.getServer()); + serverXmlDump = buffer.toString(); + } finally { + desc.setStoreSeparate(oldSeparate); + desc.setExternalAllowed(oldExternalAllowed); + desc.setExternalOnly(oldExternalOnly); + } + + Assert.assertTrue(serverXmlDump, serverXmlDump.contains("<Manager")); + Assert.assertTrue(serverXmlDump, serverXmlDump.contains("throwOnFailure=\"false\"")); + } + + public static class CustomTestManager extends StandardManager { + + public CustomTestManager() { + } + } + /** * Verify that a Context parsed from a Context element in server.xml is flagged as deployed from server.xml, so * storeconfig can detect it. The flag must also be set when server.xml is processed through the generated code diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 02656435bc..e9c553a1c7 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -333,6 +333,20 @@ a warning in the temporary location only in the rare cases where restoring it is not possible. (remm) </fix> + <fix> + Fix the <code>ManagerSF</code> store configuration factory always + treating a started <code>StandardManager</code> as non-default, since + the manager propagates its (possibly <code>null</code>) node identifier + to its session ID generator when it starts, which the default check + compared against an empty string. This caused a redundant + <code>Manager</code> element with a nested + <code>SessionIdGenerator</code> to be written for every context using + the default manager configuration when the running configuration was + stored. The default check was strengthened to require the exact + manager and session ID generator classes and to test + <code>throwOnFailure</code>, so the now reachable skip path cannot + silently drop a customised configuration. (remm) + </fix> </changelog> </subsection> <subsection name="Coyote"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
