You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by re...@apache.org on 2026/10/07 16:48:45 UTC
(tomcat) branch main updated (9e035e8d06 -> 2840b562eb)
This is an automated email from the ASF dual-hosted git repository.
rmaucher pushed a change to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
from 9e035e8d06 Follow-on to "Allow re-adding a host that was previously removed..."
new df14492478 Fix nested role search DN computation in JNDIRealm
new 98652e3818 Track StartTlsResponse per connection in JNDIRealm
new 2afbb8fc88 Fix DataSourceRealm role lookup when no role store is configured
new 74e803426d Report malformed JNDIRealm user patterns as a clear configuration error
new 21c7717d6e Make JNDIRealm URL failover decision per connection attempt
new 8538b3c87a Fix HTTP DIGEST authentication via JAASRealm
new 0dbb3feb01 Report missing user credentials as an authentication failure in JNDIRealm
new 44561a80c5 Trim whitespace when parsing the checkedOsUsers list in SecurityListener
new 3518d1045b Reject non-positive check periods in TLSCertificateReloadListener
new 449e4ebd18 Ignore malformed Accept-Encoding q-values in DefaultServlet precompressed handling
new 3866869e05 Store namespace-less WebDAV properties in DataSourcePropertyStore
new e44f30dc6e Remove sessions from the manager map by instance identity
new a1b3b73c55 Use long arithmetic when calculating session rates in ManagerBase
new f892bf5099 Discard invalid persisted sessions on load in StandardManager
new 52dfcd3f4b Validate the principal object graph before persisting it in StandardSession
new 2840b562eb Merge branch 'main' of git@github.com:apache/tomcat.git into main
The 16 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../org/apache/catalina/realm/DataSourceRealm.java | 31 +--
.../apache/catalina/realm/JAASCallbackHandler.java | 18 +-
java/org/apache/catalina/realm/JAASRealm.java | 8 +-
java/org/apache/catalina/realm/JNDIRealm.java | 113 +++++++----
.../apache/catalina/realm/LocalStrings.properties | 2 +
.../catalina/security/LocalStrings.properties | 1 +
.../apache/catalina/security/SecurityListener.java | 1 +
.../security/TLSCertificateReloadListener.java | 9 +-
.../catalina/servlets/DataSourcePropertyStore.java | 13 +-
.../apache/catalina/servlets/DefaultServlet.java | 7 +-
java/org/apache/catalina/session/ManagerBase.java | 12 +-
.../apache/catalina/session/StandardManager.java | 16 +-
.../apache/catalina/session/StandardSession.java | 22 ++-
.../apache/catalina/realm/TestDataSourceRealm.java | 49 +++++
test/org/apache/catalina/realm/TestJAASRealm.java | 66 +++++++
test/org/apache/catalina/realm/TestJNDIRealm.java | 38 ++++
.../catalina/realm/TestJNDIRealmIntegration.java | 17 +-
.../catalina/realm/TestJNDIRealmSearchAsUser.java | 99 ++++++++++
.../catalina/security/TestSecurityListener.java | 52 +++++
.../TestTLSCertificateReloadListener.java} | 22 ++-
.../catalina/servlets/TestDefaultServlet.java | 65 +++++++
.../catalina/servlets/TestWebdavPropertyStore.java | 33 ++++
.../apache/catalina/session/TestManagerBase.java | 63 ++++++
.../session/TestPersistentManagerBase.java | 105 ++++++++++
.../catalina/session/TestStandardManager.java | 211 +++++++++++++++++++++
webapps/docs/changelog.xml | 32 ++++
webapps/docs/realm-howto.xml | 8 +-
27 files changed, 1007 insertions(+), 106 deletions(-)
create mode 100644 test/org/apache/catalina/realm/TestJNDIRealmSearchAsUser.java
create mode 100644 test/org/apache/catalina/security/TestSecurityListener.java
copy test/{jakarta/servlet/jsp/TestPageContext.java => org/apache/catalina/security/TestTLSCertificateReloadListener.java} (61%)
create mode 100644 test/org/apache/catalina/session/TestManagerBase.java
create mode 100644 test/org/apache/catalina/session/TestPersistentManagerBase.java
create mode 100644 test/org/apache/catalina/session/TestStandardManager.java
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
(tomcat) 04/16: Report malformed JNDIRealm user patterns as a clear configuration error
Posted by re...@apache.org.
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 74e803426d34fffefae666ca62890e91e829a3c3
Author: opencode <re...@apache.org>
AuthorDate: Wed Oct 7 15:34:15 2026 +0200
Report malformed JNDIRealm user patterns as a clear configuration error
parseUserPatternString() could walk off the end of the pattern string
when parentheses were unbalanced: a trailing open paren made
charAt(startParenLoc + 1) throw StringIndexOutOfBoundsException, a
missing close paren made charAt(endParenLoc - 1) throw with index -2,
and a pattern whose only open paren was escaped made the weeding loop
read charAt(-2) after indexOf had returned -1. Certain inputs such as
(|x) oscillated the weeding loop forever. The obscure crash surfaced
while setting the userPattern property, e.g. during server.xml
parsing.
Guard the paren searches, throw an IllegalArgumentException naming the
offending pattern when parentheses are unbalanced, and treat a pattern
in which every paren is escaped as a single pattern rather than
crashing, consistent with the no-parens case. setUserPattern now
parses before assigning, so a rejected pattern leaves the previous
configuration intact. Add unit tests covering the malformed and
fallback cases.
---
java/org/apache/catalina/realm/JNDIRealm.java | 33 ++++++++++++++++---
.../apache/catalina/realm/LocalStrings.properties | 1 +
test/org/apache/catalina/realm/TestJNDIRealm.java | 38 ++++++++++++++++++++++
3 files changed, 67 insertions(+), 5 deletions(-)
diff --git a/java/org/apache/catalina/realm/JNDIRealm.java b/java/org/apache/catalina/realm/JNDIRealm.java
index 4c1e4dde3e..091ede4816 100644
--- a/java/org/apache/catalina/realm/JNDIRealm.java
+++ b/java/org/apache/catalina/realm/JNDIRealm.java
@@ -894,11 +894,13 @@ public class JNDIRealm extends RealmBase {
* @param userPattern The new user pattern
*/
public void setUserPattern(String userPattern) {
- this.userPattern = userPattern;
if (userPattern == null) {
+ this.userPattern = null;
userPatternArray = null;
} else {
- userPatternArray = parseUserPatternString(userPattern);
+ String[] parsed = parseUserPatternString(userPattern);
+ this.userPattern = userPattern;
+ userPatternArray = parsed;
singleConnection = create();
}
}
@@ -2951,6 +2953,8 @@ public class JNDIRealm extends RealmBase {
* @param userPatternString - a string LDAP search paths surrounded by parentheses
*
* @return a parsed string array
+ *
+ * @throws IllegalArgumentException if the pattern contains unbalanced parentheses
*/
protected String[] parseUserPatternString(String userPatternString) {
@@ -2965,19 +2969,38 @@ public class JNDIRealm extends RealmBase {
// weed out escaped open parens and parens enclosing the
// whole statement (in the case of valid LDAP search
// strings: (|(something)(somethingelse))
- while ((userPatternString.charAt(startParenLoc + 1) == '|') ||
- (startParenLoc != 0 && userPatternString.charAt(startParenLoc - 1) == '\\')) {
+ while (startParenLoc > -1 && startParenLoc < userPatternString.length() - 1 &&
+ ((userPatternString.charAt(startParenLoc + 1) == '|') ||
+ (startParenLoc != 0 && userPatternString.charAt(startParenLoc - 1) == '\\'))) {
startParenLoc = userPatternString.indexOf('(', startParenLoc + 1);
}
+ if (startParenLoc == -1) {
+ break;
+ }
+ if (startParenLoc == userPatternString.length() - 1) {
+ // an open paren at the end cannot start a pattern
+ throw new IllegalArgumentException(sm.getString("jndiRealm.invalidUserPattern",
+ userPatternString));
+ }
int endParenLoc = userPatternString.indexOf(')', startParenLoc + 1);
// weed out escaped end-parens
- while (userPatternString.charAt(endParenLoc - 1) == '\\') {
+ while (endParenLoc > 0 && userPatternString.charAt(endParenLoc - 1) == '\\') {
endParenLoc = userPatternString.indexOf(')', endParenLoc + 1);
}
+ if (endParenLoc == -1) {
+ // no matching close paren for this pattern
+ throw new IllegalArgumentException(sm.getString("jndiRealm.invalidUserPattern",
+ userPatternString));
+ }
String nextPathPart = userPatternString.substring(startParenLoc + 1, endParenLoc);
pathList.add(nextPathPart);
startParenLoc = userPatternString.indexOf('(', endParenLoc + 1);
}
+ if (pathList.isEmpty()) {
+ // every paren was escaped, nothing to separate: treat the
+ // whole string as the pattern
+ return new String[] { userPatternString };
+ }
return pathList.toArray(new String[0]);
}
return null;
diff --git a/java/org/apache/catalina/realm/LocalStrings.properties b/java/org/apache/catalina/realm/LocalStrings.properties
index f84e69b447..42aec0e181 100644
--- a/java/org/apache/catalina/realm/LocalStrings.properties
+++ b/java/org/apache/catalina/realm/LocalStrings.properties
@@ -85,6 +85,7 @@ jndiRealm.invalidHostnameVerifier=[{0}] not a valid class name for a HostnameVer
jndiRealm.invalidName=Search returned unparsable absolute name: [{0}]
jndiRealm.invalidSslProtocol=Given protocol [{0}] is invalid. It has to be one of [{1}]
jndiRealm.invalidSslSocketFactory=[{0}] not a valid class name for an SSLSocketFactory
+jndiRealm.invalidUserPattern=The user pattern [{0}] is not valid: it contains unbalanced parentheses
jndiRealm.multipleEntries=User name [{0}] has multiple entries
jndiRealm.negotiatedTls=Negotiated tls connection using protocol [{0}]
jndiRealm.open=Exception opening directory server connection
diff --git a/test/org/apache/catalina/realm/TestJNDIRealm.java b/test/org/apache/catalina/realm/TestJNDIRealm.java
index 0a7f3f8186..c8a870dc95 100644
--- a/test/org/apache/catalina/realm/TestJNDIRealm.java
+++ b/test/org/apache/catalina/realm/TestJNDIRealm.java
@@ -136,6 +136,44 @@ public class TestJNDIRealm {
Assert.assertTrue(latch.await(30, TimeUnit.SECONDS));
}
+ @Test
+ public void testParseUserPatternStringValid() throws Exception {
+ JNDIRealm realm = new JNDIRealm();
+
+ Assert.assertArrayEquals(new String[] { "cn={0}" },
+ realm.parseUserPatternString("cn={0}"));
+ Assert.assertArrayEquals(new String[] { "cn={0}", "cn={0},o=myorg" },
+ realm.parseUserPatternString("(cn={0})(cn={0},o=myorg)"));
+ Assert.assertArrayEquals(new String[] { "cn={0}", "cn={0},o=myorg" },
+ realm.parseUserPatternString("(|(cn={0})(cn={0},o=myorg))"));
+ Assert.assertArrayEquals(new String[] { "cn=\\(x" },
+ realm.parseUserPatternString("cn=\\(x"));
+ }
+
+ @Test(expected = IllegalArgumentException.class)
+ public void testParseUserPatternStringTrailingOpenParen() throws Exception {
+ JNDIRealm realm = new JNDIRealm();
+ realm.setUserPattern("cn={0}(");
+ }
+
+ @Test(expected = IllegalArgumentException.class)
+ public void testParseUserPatternStringMissingCloseParen() throws Exception {
+ JNDIRealm realm = new JNDIRealm();
+ realm.setUserPattern("(cn={0}");
+ }
+
+ @Test(expected = IllegalArgumentException.class)
+ public void testParseUserPatternStringOnlyOpenParen() throws Exception {
+ JNDIRealm realm = new JNDIRealm();
+ realm.setUserPattern("(");
+ }
+
+ @Test(expected = IllegalArgumentException.class)
+ public void testParseUserPatternStringEscapedOpenParenOnly() throws Exception {
+ JNDIRealm realm = new JNDIRealm();
+ realm.setUserPattern("cn=\\(");
+ }
+
private JNDIRealm buildRealm(String password) throws NamingException,
NoSuchFieldException, IllegalAccessException, LifecycleException {
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: (tomcat) 12/16: Remove sessions from the manager map by instance identity
Posted by Mark Thomas <ma...@apache.org>.
On 13/10/2026 18:05, Rémy Maucherat wrote:
<snip/>
> - Proposed fix: Make the decision and the removal atomic per session
> ID, e.g. in `StoreBase.processExpires()` acquire
> `manager.getStore().getSessionStoreLock(key).writeLock()` around the
> `isLoaded` check/`expire()`/`remove(key)` block, and in
> `ManagerBase.remove(Session, boolean)` only remove when the mapped
> object is the same instance (`sessions.remove(id, session)`), so a
> concurrently swapped-in session can never be evicted by a stale copy.
>
> But since it is so unlikely, I am ok with reverting and seeing what
> the next review says. So I'll do that. It would need to come up with a
> better testcase.
That is amusing. I was looking at the code and reached the conclusion
that adding the lock was the right fix.
I'll add the lock shortly. With the lock, the object check should be
unnecessary.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: (tomcat) 12/16: Remove sessions from the manager map by instance identity
Posted by Rémy Maucherat <re...@apache.org>.
On Tue, Oct 13, 2026 at 6:56 PM Mark Thomas <ma...@apache.org> wrote:
>
> I think this needs to be reverted.
>
> I think the problem this commit is trying to solve can only occur if two
> separate sessions end up with the same ID. If that happens we have much
> bigger issues.
Yes, I pushed back using this argument (session id collisions are
impossible), but the scenario is actually not really about that [the
testcase is though].
> Also, the PersistentManager has to remove by ID. It can't remove by
> identity since as a session restored from the store will be a different
> instance every time it is restored.
### [MEDIUM] TOCTOU race: background store expiration can evict a live
session that is swapped in concurrently, losing the session
- File: /home/opencode/code-review/tomcat/java/org/apache/catalina/session/StoreBase.java:156-189
(unlocked `isLoaded` check + `expire()`), interacting with
/home/opencode/code-review/tomcat/java/org/apache/catalina/session/PersistentManagerBase.java:576-643
(`swapIn`/`reactivateLoadedSession`) and
/home/opencode/code-review/tomcat/java/org/apache/catalina/session/ManagerBase.java:774-792
(`remove` deletes by session ID, not by object identity).
- Description: Sequence: (1) background thread in
`StoreBase.processExpires()` loads a swapped-out session copy C from
the Store, determines it is expired, and checks `isLoaded(key)`
(StoreBase.java:170-181) - a plain, lock-free `sessions.get(key)` -
which returns `false`; (2) concurrently, the owner's next request
calls `PersistentManagerBase.findSession()` -> `swapIn()`, which takes
the per-ID store write lock, loads the session L from the
still-present store entry and adds it to the `sessions` map
(PersistentManagerBase.java:628-643); (3) the background thread then
calls `session.expire()` on copy C; `expire()` first runs the
application's `sessionDestroyed` listeners
(StandardSession.java:610-640 - an arbitrarily long window), then
`manager.remove(C, true)` executes
`sessions.remove(C.getIdInternal())` (ManagerBase.java:789-791) which
removes whatever object is currently mapped to that ID - i.e. the live
session L - and `PersistentManagerBase.remove()`
(PersistentManagerBase.java:449-456) additionally deletes the store
entry. Result: L is orphaned (still referenced by the in-flight
request, now invisible to the Manager) and the store backup is gone,
so the next request for that ID cannot recover the session - the user
is logged out with state lost, and the app received a
`sessionDestroyed` for a session that was still active.
- Verification: Confirmed `sessions.remove(id)` is key-based
(ManagerBase.java:789-791), `PersistentManagerBase.remove` removes the
store entry by ID (PersistentManagerBase.java:449-456), and `isLoaded`
(PersistentManagerBase.java:220-229) performs no locking; `swapIn`
holds the per-ID store write lock only around its own load/add, and
`processExpires()` takes no store lock around the isLoaded decision,
so no mutual exclusion exists between the two paths. The window spans
the whole `sessionDestroyed` listener invocation, so it is not
negligible.
- Proposed fix: Make the decision and the removal atomic per session
ID, e.g. in `StoreBase.processExpires()` acquire
`manager.getStore().getSessionStoreLock(key).writeLock()` around the
`isLoaded` check/`expire()`/`remove(key)` block, and in
`ManagerBase.remove(Session, boolean)` only remove when the mapped
object is the same instance (`sessions.remove(id, session)`), so a
concurrently swapped-in session can never be evicted by a stale copy.
But since it is so unlikely, I am ok with reverting and seeing what
the next review says. So I'll do that. It would need to come up with a
better testcase.
Rémy
> Mark
>
>
> On 07/10/2026 17:48, remm@apache.org wrote:
> > 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 e44f30dc6eb435254cea39acfc1f0333481634e1
> > Author: opencode <re...@apache.org>
> > AuthorDate: Wed Oct 7 17:30:51 2026 +0200
> >
> > Remove sessions from the manager map by instance identity
> >
> > ManagerBase.remove(Session, boolean) removed the map entry keyed by the
> > session ID regardless of which instance the map currently held. A stale
> > copy of a session, such as one deserialized from a Store, could there-
> > fore evict a different, live instance that had the same ID, losing it
> > without any of its own state being expired.
> >
> > Use the two argument ConcurrentHashMap removal so the entry is only
> > removed when the map still holds the session instance passed in.
> > ---
> > java/org/apache/catalina/session/ManagerBase.java | 7 +-
> > .../session/TestPersistentManagerBase.java | 105 +++++++++++++++++++++
> > 2 files changed, 111 insertions(+), 1 deletion(-)
> >
> > diff --git a/java/org/apache/catalina/session/ManagerBase.java b/java/org/apache/catalina/session/ManagerBase.java
> > index 16bb91b071..6ba65d4873 100644
> > --- a/java/org/apache/catalina/session/ManagerBase.java
> > +++ b/java/org/apache/catalina/session/ManagerBase.java
> > @@ -787,7 +787,12 @@ public abstract class ManagerBase extends LifecycleMBeanBase implements Manager
> > }
> >
> > if (session.getIdInternal() != null) {
> > - sessions.remove(session.getIdInternal());
> > + // Only remove the entry if the map still holds this exact session
> > + // instance. This prevents a stale copy of a session (for example,
> > + // an expired copy loaded from a store) from removing a live
> > + // session with the same ID that was loaded into the map in the
> > + // meantime.
> > + sessions.remove(session.getIdInternal(), session);
> > }
> > }
> >
> > diff --git a/test/org/apache/catalina/session/TestPersistentManagerBase.java b/test/org/apache/catalina/session/TestPersistentManagerBase.java
> > new file mode 100644
> > index 0000000000..1d266f8d1e
> > --- /dev/null
> > +++ b/test/org/apache/catalina/session/TestPersistentManagerBase.java
> > @@ -0,0 +1,105 @@
> > +/*
> > + * 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.session;
> > +
> > +import java.io.File;
> > +
> > +import org.junit.Assert;
> > +import org.junit.Test;
> > +
> > +import org.apache.catalina.Session;
> > +import org.apache.catalina.startup.ExpandWar;
> > +import org.apache.tomcat.unittest.TesterContext;
> > +import org.apache.tomcat.unittest.TesterHost;
> > +import org.apache.tomcat.unittest.TesterServletContext;
> > +
> > +public class TestPersistentManagerBase {
> > +
> > + private static final String SESSION_ID = "testerSessionId";
> > +
> > + /*
> > + * Removing a stale copy of a session must not evict a different session
> > + * instance with the same ID from the manager.
> > + */
> > + @Test
> > + public void testRemoveStaleCopyDoesNotEvictLiveSession() throws Exception {
> > + File dir = new File("SESS_TEMP_STALE_REMOVE");
> > + ExpandWar.delete(dir);
> > + try {
> > + PersistentManager manager = new PersistentManager();
> > +
> > + FileStore store = new FileStore();
> > + store.setDirectory(dir.getAbsolutePath());
> > + manager.setStore(store);
> > +
> > + TesterContext context = new TesterContext();
> > + context.setServletContext(new TesterServletContext());
> > + context.setParent(new TesterHost());
> > + manager.setContext(context);
> > +
> > + manager.start();
> > +
> > + Session live = manager.createSession(SESSION_ID);
> > +
> > + // A different instance carrying the same session ID, not
> > + // registered with the manager, as produced by loading a copy from
> > + // the store before it is swapped in
> > + StandardSession stale = new StandardSession(manager);
> > + stale.id = SESSION_ID;
> > +
> > + manager.remove(stale, false);
> > +
> > + // Before the fix, the removal was based on the session ID alone
> > + // and removed the live session from the manager
> > + Session current = manager.findSession(SESSION_ID);
> > + Assert.assertSame(live, current);
> > + } finally {
> > + ExpandWar.delete(dir);
> > + }
> > + }
> > +
> > + /*
> > + * Control for the test above: removing the instance that is in the map
> > + * does remove the session.
> > + */
> > + @Test
> > + public void testRemoveLiveInstanceRemovesSession() throws Exception {
> > + File dir = new File("SESS_TEMP_LIVE_REMOVE");
> > + ExpandWar.delete(dir);
> > + try {
> > + PersistentManager manager = new PersistentManager();
> > +
> > + FileStore store = new FileStore();
> > + store.setDirectory(dir.getAbsolutePath());
> > + manager.setStore(store);
> > +
> > + TesterContext context = new TesterContext();
> > + context.setServletContext(new TesterServletContext());
> > + context.setParent(new TesterHost());
> > + manager.setContext(context);
> > +
> > + manager.start();
> > +
> > + Session session = manager.createSession(SESSION_ID);
> > + manager.remove(session, false);
> > +
> > + Assert.assertNull(manager.findSession(SESSION_ID));
> > + } finally {
> > + ExpandWar.delete(dir);
> > + }
> > + }
> > +}
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: (tomcat) 12/16: Remove sessions from the manager map by instance identity
Posted by Mark Thomas <ma...@apache.org>.
I think this needs to be reverted.
I think the problem this commit is trying to solve can only occur if two
separate sessions end up with the same ID. If that happens we have much
bigger issues.
Also, the PersistentManager has to remove by ID. It can't remove by
identity since as a session restored from the store will be a different
instance every time it is restored.
Mark
On 07/10/2026 17:48, remm@apache.org wrote:
> 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 e44f30dc6eb435254cea39acfc1f0333481634e1
> Author: opencode <re...@apache.org>
> AuthorDate: Wed Oct 7 17:30:51 2026 +0200
>
> Remove sessions from the manager map by instance identity
>
> ManagerBase.remove(Session, boolean) removed the map entry keyed by the
> session ID regardless of which instance the map currently held. A stale
> copy of a session, such as one deserialized from a Store, could there-
> fore evict a different, live instance that had the same ID, losing it
> without any of its own state being expired.
>
> Use the two argument ConcurrentHashMap removal so the entry is only
> removed when the map still holds the session instance passed in.
> ---
> java/org/apache/catalina/session/ManagerBase.java | 7 +-
> .../session/TestPersistentManagerBase.java | 105 +++++++++++++++++++++
> 2 files changed, 111 insertions(+), 1 deletion(-)
>
> diff --git a/java/org/apache/catalina/session/ManagerBase.java b/java/org/apache/catalina/session/ManagerBase.java
> index 16bb91b071..6ba65d4873 100644
> --- a/java/org/apache/catalina/session/ManagerBase.java
> +++ b/java/org/apache/catalina/session/ManagerBase.java
> @@ -787,7 +787,12 @@ public abstract class ManagerBase extends LifecycleMBeanBase implements Manager
> }
>
> if (session.getIdInternal() != null) {
> - sessions.remove(session.getIdInternal());
> + // Only remove the entry if the map still holds this exact session
> + // instance. This prevents a stale copy of a session (for example,
> + // an expired copy loaded from a store) from removing a live
> + // session with the same ID that was loaded into the map in the
> + // meantime.
> + sessions.remove(session.getIdInternal(), session);
> }
> }
>
> diff --git a/test/org/apache/catalina/session/TestPersistentManagerBase.java b/test/org/apache/catalina/session/TestPersistentManagerBase.java
> new file mode 100644
> index 0000000000..1d266f8d1e
> --- /dev/null
> +++ b/test/org/apache/catalina/session/TestPersistentManagerBase.java
> @@ -0,0 +1,105 @@
> +/*
> + * 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.session;
> +
> +import java.io.File;
> +
> +import org.junit.Assert;
> +import org.junit.Test;
> +
> +import org.apache.catalina.Session;
> +import org.apache.catalina.startup.ExpandWar;
> +import org.apache.tomcat.unittest.TesterContext;
> +import org.apache.tomcat.unittest.TesterHost;
> +import org.apache.tomcat.unittest.TesterServletContext;
> +
> +public class TestPersistentManagerBase {
> +
> + private static final String SESSION_ID = "testerSessionId";
> +
> + /*
> + * Removing a stale copy of a session must not evict a different session
> + * instance with the same ID from the manager.
> + */
> + @Test
> + public void testRemoveStaleCopyDoesNotEvictLiveSession() throws Exception {
> + File dir = new File("SESS_TEMP_STALE_REMOVE");
> + ExpandWar.delete(dir);
> + try {
> + PersistentManager manager = new PersistentManager();
> +
> + FileStore store = new FileStore();
> + store.setDirectory(dir.getAbsolutePath());
> + manager.setStore(store);
> +
> + TesterContext context = new TesterContext();
> + context.setServletContext(new TesterServletContext());
> + context.setParent(new TesterHost());
> + manager.setContext(context);
> +
> + manager.start();
> +
> + Session live = manager.createSession(SESSION_ID);
> +
> + // A different instance carrying the same session ID, not
> + // registered with the manager, as produced by loading a copy from
> + // the store before it is swapped in
> + StandardSession stale = new StandardSession(manager);
> + stale.id = SESSION_ID;
> +
> + manager.remove(stale, false);
> +
> + // Before the fix, the removal was based on the session ID alone
> + // and removed the live session from the manager
> + Session current = manager.findSession(SESSION_ID);
> + Assert.assertSame(live, current);
> + } finally {
> + ExpandWar.delete(dir);
> + }
> + }
> +
> + /*
> + * Control for the test above: removing the instance that is in the map
> + * does remove the session.
> + */
> + @Test
> + public void testRemoveLiveInstanceRemovesSession() throws Exception {
> + File dir = new File("SESS_TEMP_LIVE_REMOVE");
> + ExpandWar.delete(dir);
> + try {
> + PersistentManager manager = new PersistentManager();
> +
> + FileStore store = new FileStore();
> + store.setDirectory(dir.getAbsolutePath());
> + manager.setStore(store);
> +
> + TesterContext context = new TesterContext();
> + context.setServletContext(new TesterServletContext());
> + context.setParent(new TesterHost());
> + manager.setContext(context);
> +
> + manager.start();
> +
> + Session session = manager.createSession(SESSION_ID);
> + manager.remove(session, false);
> +
> + Assert.assertNull(manager.findSession(SESSION_ID));
> + } finally {
> + ExpandWar.delete(dir);
> + }
> + }
> +}
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
(tomcat) 12/16: Remove sessions from the manager map by instance identity
Posted by re...@apache.org.
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 e44f30dc6eb435254cea39acfc1f0333481634e1
Author: opencode <re...@apache.org>
AuthorDate: Wed Oct 7 17:30:51 2026 +0200
Remove sessions from the manager map by instance identity
ManagerBase.remove(Session, boolean) removed the map entry keyed by the
session ID regardless of which instance the map currently held. A stale
copy of a session, such as one deserialized from a Store, could there-
fore evict a different, live instance that had the same ID, losing it
without any of its own state being expired.
Use the two argument ConcurrentHashMap removal so the entry is only
removed when the map still holds the session instance passed in.
---
java/org/apache/catalina/session/ManagerBase.java | 7 +-
.../session/TestPersistentManagerBase.java | 105 +++++++++++++++++++++
2 files changed, 111 insertions(+), 1 deletion(-)
diff --git a/java/org/apache/catalina/session/ManagerBase.java b/java/org/apache/catalina/session/ManagerBase.java
index 16bb91b071..6ba65d4873 100644
--- a/java/org/apache/catalina/session/ManagerBase.java
+++ b/java/org/apache/catalina/session/ManagerBase.java
@@ -787,7 +787,12 @@ public abstract class ManagerBase extends LifecycleMBeanBase implements Manager
}
if (session.getIdInternal() != null) {
- sessions.remove(session.getIdInternal());
+ // Only remove the entry if the map still holds this exact session
+ // instance. This prevents a stale copy of a session (for example,
+ // an expired copy loaded from a store) from removing a live
+ // session with the same ID that was loaded into the map in the
+ // meantime.
+ sessions.remove(session.getIdInternal(), session);
}
}
diff --git a/test/org/apache/catalina/session/TestPersistentManagerBase.java b/test/org/apache/catalina/session/TestPersistentManagerBase.java
new file mode 100644
index 0000000000..1d266f8d1e
--- /dev/null
+++ b/test/org/apache/catalina/session/TestPersistentManagerBase.java
@@ -0,0 +1,105 @@
+/*
+ * 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.session;
+
+import java.io.File;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import org.apache.catalina.Session;
+import org.apache.catalina.startup.ExpandWar;
+import org.apache.tomcat.unittest.TesterContext;
+import org.apache.tomcat.unittest.TesterHost;
+import org.apache.tomcat.unittest.TesterServletContext;
+
+public class TestPersistentManagerBase {
+
+ private static final String SESSION_ID = "testerSessionId";
+
+ /*
+ * Removing a stale copy of a session must not evict a different session
+ * instance with the same ID from the manager.
+ */
+ @Test
+ public void testRemoveStaleCopyDoesNotEvictLiveSession() throws Exception {
+ File dir = new File("SESS_TEMP_STALE_REMOVE");
+ ExpandWar.delete(dir);
+ try {
+ PersistentManager manager = new PersistentManager();
+
+ FileStore store = new FileStore();
+ store.setDirectory(dir.getAbsolutePath());
+ manager.setStore(store);
+
+ TesterContext context = new TesterContext();
+ context.setServletContext(new TesterServletContext());
+ context.setParent(new TesterHost());
+ manager.setContext(context);
+
+ manager.start();
+
+ Session live = manager.createSession(SESSION_ID);
+
+ // A different instance carrying the same session ID, not
+ // registered with the manager, as produced by loading a copy from
+ // the store before it is swapped in
+ StandardSession stale = new StandardSession(manager);
+ stale.id = SESSION_ID;
+
+ manager.remove(stale, false);
+
+ // Before the fix, the removal was based on the session ID alone
+ // and removed the live session from the manager
+ Session current = manager.findSession(SESSION_ID);
+ Assert.assertSame(live, current);
+ } finally {
+ ExpandWar.delete(dir);
+ }
+ }
+
+ /*
+ * Control for the test above: removing the instance that is in the map
+ * does remove the session.
+ */
+ @Test
+ public void testRemoveLiveInstanceRemovesSession() throws Exception {
+ File dir = new File("SESS_TEMP_LIVE_REMOVE");
+ ExpandWar.delete(dir);
+ try {
+ PersistentManager manager = new PersistentManager();
+
+ FileStore store = new FileStore();
+ store.setDirectory(dir.getAbsolutePath());
+ manager.setStore(store);
+
+ TesterContext context = new TesterContext();
+ context.setServletContext(new TesterServletContext());
+ context.setParent(new TesterHost());
+ manager.setContext(context);
+
+ manager.start();
+
+ Session session = manager.createSession(SESSION_ID);
+ manager.remove(session, false);
+
+ Assert.assertNull(manager.findSession(SESSION_ID));
+ } finally {
+ ExpandWar.delete(dir);
+ }
+ }
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
(tomcat) 03/16: Fix DataSourceRealm role lookup when no role store is configured
Posted by re...@apache.org.
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 2afbb8fc88874d6021d2dd1b5707724ea186df4c
Author: opencode <re...@apache.org>
AuthorDate: Wed Oct 7 15:23:36 2026 +0200
Fix DataSourceRealm role lookup when no role store is configured
startInternal() endorses a realm with no role store (both userRoleTable
and roleNameCol unset) but then unconditionally built the roles prepared
statement by concatenating the unset role column and role table,
producing the literal invalid statement SELECT null FROM null ... .
With the default allRolesMode of strict the early guard in getRoles()
did not short-circuit, so every role lookup ran the invalid statement,
logged a swallowed SQLException and returned null, which failed
authentication of valid credentials and made getPrincipal return null.
The realm started successfully but failed 100% of authentications.
Skip the role lookup whenever no role store is defined, regardless of
allRolesMode, matching JNDIRealm and MemoryRealm which return an empty
role set for a valid user when no role source is configured. The
allRolesMode is interpreted centrally in RealmBase. The deliberate
hardening of failing authentication when a configured role lookup fails
is unchanged. The invalid statement is no longer constructed at all
when no role store is defined.
Add a Derby backed regression test for the strict mode with no role
store configuration. It fails before the fix.
---
.../org/apache/catalina/realm/DataSourceRealm.java | 31 ++++++++------
.../apache/catalina/realm/TestDataSourceRealm.java | 49 ++++++++++++++++++++++
2 files changed, 67 insertions(+), 13 deletions(-)
diff --git a/java/org/apache/catalina/realm/DataSourceRealm.java b/java/org/apache/catalina/realm/DataSourceRealm.java
index 8d574a9b6a..28e7b2cfdc 100644
--- a/java/org/apache/catalina/realm/DataSourceRealm.java
+++ b/java/org/apache/catalina/realm/DataSourceRealm.java
@@ -537,9 +537,9 @@ public class DataSourceRealm extends RealmBase {
*/
protected ArrayList<String> getRoles(Connection dbConnection, String username) {
- if (allRolesMode != AllRolesMode.STRICT_MODE && !isRoleStoreDefined()) {
- // Using an authentication only configuration and no role store has
- // been defined so don't spend cycles looking
+ if (!isRoleStoreDefined()) {
+ // No role store has been defined so there is nothing to look up.
+ // The all roles mode is interpreted centrally in RealmBase.
return new ArrayList<>(0);
}
@@ -591,18 +591,23 @@ public class DataSourceRealm extends RealmBase {
throw new LifecycleException(sm.getString("dataSourceRealm.roleConfigMismatch"));
}
- // Create the roles PreparedStatement string
- StringBuilder temp = new StringBuilder("SELECT ");
- temp.append(roleNameCol);
- temp.append(" FROM ");
- temp.append(userRoleTable);
- temp.append(" WHERE ");
- temp.append(userNameCol);
- temp.append(" = ?");
- preparedRoles = temp.toString();
+ // Create the roles PreparedStatement string. When no role store is
+ // defined there is nothing to query and no statement to build.
+ if (isRoleStoreDefined()) {
+ StringBuilder temp = new StringBuilder("SELECT ");
+ temp.append(roleNameCol);
+ temp.append(" FROM ");
+ temp.append(userRoleTable);
+ temp.append(" WHERE ");
+ temp.append(userNameCol);
+ temp.append(" = ?");
+ preparedRoles = temp.toString();
+ } else {
+ preparedRoles = null;
+ }
// Create the credentials PreparedStatement string
- temp = new StringBuilder("SELECT ");
+ StringBuilder temp = new StringBuilder("SELECT ");
temp.append(userCredCol);
temp.append(" FROM ");
temp.append(userTable);
diff --git a/test/org/apache/catalina/realm/TestDataSourceRealm.java b/test/org/apache/catalina/realm/TestDataSourceRealm.java
index b897f1553b..1fbe75ca5b 100644
--- a/test/org/apache/catalina/realm/TestDataSourceRealm.java
+++ b/test/org/apache/catalina/realm/TestDataSourceRealm.java
@@ -158,4 +158,53 @@ public class TestDataSourceRealm extends LoggingBaseTest {
db.stop();
}
+
+ @Test
+ public void testRealmWithoutRoleStore() throws Exception {
+
+ db = new DerbyDataSourceRealm("dsRealmNoRoles");
+ db.setUserTable("users");
+ db.setUserNameCol("user_name");
+ db.setUserCredCol("user_pass");
+
+ // Create only the users table, no role store is configured
+ Connection connection = db.open();
+ for (String sql: SIMPLE_SCHEMA.split(";")) {
+ if (sql.contains("user_roles")) {
+ continue;
+ }
+ try (Statement statement = connection.createStatement()) {
+ statement.execute(sql);
+ }
+ }
+
+ try (PreparedStatement stmt = connection.prepareStatement(USERS_INSERT)) {
+ stmt.setString(1, "tomcat");
+ stmt.setString(2, "password");
+ stmt.executeUpdate();
+ }
+
+ db.start();
+
+ // Default strict mode with no role store. Authentication of a valid
+ // user must succeed with an empty role list rather than fail.
+ Principal p = db.authenticate("tomcat", "bar");
+ Assert.assertNull(p);
+
+ p = db.authenticate("tomcat", "password");
+ Assert.assertTrue(p instanceof GenericPrincipal);
+ GenericPrincipal gp = (GenericPrincipal) p;
+ Assert.assertEquals(0, gp.getRoles().length);
+
+ p = db.getPrincipal("tomcat");
+ Assert.assertTrue(p instanceof GenericPrincipal);
+ gp = (GenericPrincipal) p;
+ Assert.assertEquals(0, gp.getRoles().length);
+
+ List<String> roles = db.getRoles("tomcat");
+ Assert.assertNotNull(roles);
+ Assert.assertEquals(0, roles.size());
+
+ db.stop();
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
(tomcat) 06/16: Fix HTTP DIGEST authentication via JAASRealm
Posted by re...@apache.org.
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 8538b3c87aa500a40e5b85d4d2b5f15e78e106eb
Author: opencode <re...@apache.org>
AuthorDate: Wed Oct 7 16:04:43 2026 +0200
Fix HTTP DIGEST authentication via JAASRealm
For DIGEST, JAASRealm.authenticate() passes the raw client digest to
JAASCallbackHandler as the password. The handler constructor
pre-mutated that value via CredentialHandler.mutate() whenever the
realm's MessageDigestCredentialHandler used the same algorithm as the
digest authentication. JAASMemoryLoginModule then handed the mutated
value back as the clientDigest of the RealmBase digest comparison,
where it was compared against the deterministic digest computed from
the stored credentials. A salted or re-digested value can never equal
that server digest, so DIGEST authentication always failed in this
configuration. The pre-mutation was a leftover of the pre-Credential
Handler realm.digest() design and the CredentialHandler API does not
expect pre-mutated input anywhere else.
Pass the password or client digest to the LoginModule unchanged. The
branch could only trigger for DIGEST with a matching algorithm, so
BASIC, FORM and CLIENT-CERT flows are unaffected. Update the class and
constructor documentation, the JAASRealm javadoc and the realm howto
accordingly, and add a regression test for the DIGEST flow. Include a
changelog entry.
---
.../apache/catalina/realm/JAASCallbackHandler.java | 18 +++---
java/org/apache/catalina/realm/JAASRealm.java | 8 +--
test/org/apache/catalina/realm/TestJAASRealm.java | 66 ++++++++++++++++++++++
webapps/docs/changelog.xml | 10 ++++
webapps/docs/realm-howto.xml | 8 ++-
5 files changed, 92 insertions(+), 18 deletions(-)
diff --git a/java/org/apache/catalina/realm/JAASCallbackHandler.java b/java/org/apache/catalina/realm/JAASCallbackHandler.java
index 3c640dd0bd..9ef1e0890d 100644
--- a/java/org/apache/catalina/realm/JAASCallbackHandler.java
+++ b/java/org/apache/catalina/realm/JAASCallbackHandler.java
@@ -32,8 +32,8 @@ import org.apache.tomcat.util.res.StringManager;
* Implementation of the JAAS <code>CallbackHandler</code> interface, used to negotiate delivery of the username and
* credentials that were specified to our constructor. No interaction with the user is required (or possible).
* <p>
- * This <code>CallbackHandler</code> will pre-digest the supplied password, if required by the
- * <code><Realm></code> element in <code>server.xml</code>.
+ * The supplied password or client digest is passed to the <code>LoginModule</code> unchanged. Any digest comparison is
+ * expected to be performed by the <code>LoginModule</code> when it calls back into the <code>Realm</code>.
* <p>
* At present, <code>JAASCallbackHandler</code> knows how to handle callbacks of type
* <code>javax.security.auth.callback.NameCallback</code>,
@@ -46,8 +46,7 @@ public class JAASCallbackHandler implements CallbackHandler {
/**
- * Construct a callback handler configured with the specified values. Note that if the <code>JAASRealm</code>
- * instance specifies digested passwords, the <code>password</code> parameter will be pre-digested here.
+ * Construct a callback handler configured with the specified values.
*
* @param realm Our associated JAASRealm instance
* @param username Username to be authenticated with
@@ -64,7 +63,9 @@ public class JAASCallbackHandler implements CallbackHandler {
*
* @param realm Our associated JAASRealm instance
* @param username Username to be authenticated with
- * @param password Password to be authenticated with
+ * @param password Password to be authenticated with. For DIGEST authentication this is the client digest, which
+ * is passed through unchanged since the digest comparison is performed by the LoginModule
+ * when it calls back into the Realm
* @param nonce Server generated nonce
* @param nc Nonce count
* @param cnonce Client generated nonce
@@ -78,12 +79,7 @@ public class JAASCallbackHandler implements CallbackHandler {
String cnonce, String qop, String realmName, String digestA2, String algorithm, String authMethod) {
this.realm = realm;
this.username = username;
-
- if (password != null && realm.hasMessageDigest(algorithm)) {
- this.password = realm.getCredentialHandler().mutate(password);
- } else {
- this.password = password;
- }
+ this.password = password;
this.nonce = nonce;
this.nc = nc;
this.cnonce = cnonce;
diff --git a/java/org/apache/catalina/realm/JAASRealm.java b/java/org/apache/catalina/realm/JAASRealm.java
index 28a0258ba0..40f8d0c8e1 100644
--- a/java/org/apache/catalina/realm/JAASRealm.java
+++ b/java/org/apache/catalina/realm/JAASRealm.java
@@ -110,10 +110,10 @@ import org.apache.tomcat.util.file.ConfigurationSource;
* <li>As part of the login process, JAASRealm registers its own <code>CallbackHandler</code>, called (unsurprisingly)
* <code>JAASCallbackHandler</code>. This handler supplies the HTTP requests' username and credentials to the
* user-supplied <code>LoginModule</code></li>
- * <li>As with other <code>Realm</code> implementations, digested passwords are supported if the
- * <code><Realm></code> element in <code>server.xml</code> contains a <code>digest</code> attribute;
- * <code>JAASCallbackHandler</code> will digest the password prior to passing it back to the
- * <code>LoginModule</code></li>
+ * <li>As with other <code>Realm</code> implementations, digested passwords are supported via the configured
+ * <code>CredentialHandler</code>. The credentials are passed to the <code>LoginModule</code> unchanged; any digest
+ * comparison is expected to be performed by the <code>LoginModule</code> when it calls back into the
+ * <code>Realm</code></li>
* </ul>
*/
public class JAASRealm extends RealmBase {
diff --git a/test/org/apache/catalina/realm/TestJAASRealm.java b/test/org/apache/catalina/realm/TestJAASRealm.java
index 49ab665ff9..2ce87e8289 100644
--- a/test/org/apache/catalina/realm/TestJAASRealm.java
+++ b/test/org/apache/catalina/realm/TestJAASRealm.java
@@ -18,7 +18,10 @@ package org.apache.catalina.realm;
import java.io.File;
import java.io.PrintWriter;
+import java.nio.charset.StandardCharsets;
+import java.security.MessageDigest;
import java.security.Principal;
+import java.util.Locale;
import org.junit.Assert;
import org.junit.Test;
@@ -26,9 +29,12 @@ import org.junit.Test;
import org.apache.catalina.Context;
import org.apache.catalina.startup.Tomcat;
import org.apache.catalina.startup.TomcatBaseTest;
+import org.apache.tomcat.util.buf.HexUtils;
public class TestJAASRealm extends TomcatBaseTest {
+ private static final String ALGORITHM = "MD5";
+
private static final String CONFIG =
"CustomLogin {\n" +
" org.apache.catalina.realm.TesterLoginModule\n" +
@@ -115,4 +121,64 @@ public class TestJAASRealm extends TomcatBaseTest {
gp.logout();
}
+ @Test
+ public void testMemoryLoginModuleDigestAuth() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ File tomcatUsersXml = new File(getTemporaryDirectory(), "tomcat-users-lm-digest.xml");
+ try (PrintWriter writer = new PrintWriter(tomcatUsersXml)) {
+ writer.write(TestMemoryRealm.CONFIG);
+ }
+ addDeleteOnTearDown(tomcatUsersXml);
+
+ // Write login config to the temp path
+ File loginConfFile = new File(getTemporaryDirectory(), "memoryLoginConfigDigest.conf");
+ try (PrintWriter writer = new PrintWriter(loginConfFile)) {
+ String path = tomcatUsersXml.getAbsolutePath();
+ if (File.separatorChar == '\\') {
+ path = path.replace("\\", "\\\\");
+ }
+ writer.write(CONFIG_MEMORY.replace("tomcat-users-lm.xml", path));
+ }
+ addDeleteOnTearDown(loginConfFile);
+
+ JAASRealm jaasRealm = new JAASRealm();
+ jaasRealm.setAppName("MemoryLogin");
+ // Use the same algorithm for the realm credential handler as used for
+ // the digest authentication
+ MessageDigestCredentialHandler credentialHandler = new MessageDigestCredentialHandler();
+ credentialHandler.setAlgorithm(ALGORITHM);
+ jaasRealm.setCredentialHandler(credentialHandler);
+ jaasRealm.setUserClassNames(GenericPrincipal.class.getName());
+ jaasRealm.setRoleClassNames(GenericPrincipal.class.getName());
+ jaasRealm.setConfigFile(loginConfFile.getAbsolutePath());
+ Context context = tomcat.addContext("/jaastest", null);
+ context.setRealm(jaasRealm);
+
+ tomcat.start();
+
+ String nonce = "test-nonce";
+ String nc = "00000001";
+ String cnonce = "test-cnonce";
+ String qop = "auth";
+ String realmName = "test-realm";
+
+ MessageDigest md5 = MessageDigest.getInstance(ALGORITHM);
+ String digestA2 = HexUtils.toHexString(md5.digest("GET:/jaastest".getBytes(StandardCharsets.UTF_8)));
+ // digestA1 as the client computes it from the stored (clear text in
+ // the login module's user file) password
+ String digestA1 = HexUtils.toHexString(
+ md5.digest(("admin:" + realmName + ":sekr3t").getBytes(StandardCharsets.UTF_8)));
+ String clientDigest = HexUtils.toHexString(md5.digest(
+ (digestA1 + ":" + nonce + ":" + nc + ":" + cnonce + ":" + qop + ":" + digestA2)
+ .getBytes(StandardCharsets.UTF_8)));
+
+ Principal p = jaasRealm.authenticate("admin", clientDigest.toLowerCase(Locale.ENGLISH), nonce, nc, cnonce,
+ qop, realmName, digestA2, ALGORITHM);
+ Assert.assertNotNull(p);
+ Assert.assertTrue(p instanceof GenericPrincipal);
+ GenericPrincipal gp = (GenericPrincipal) p;
+ Assert.assertTrue(gp.hasRole("testrole"));
+ gp.logout();
+ }
}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 0fcc249322..373afef76f 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -274,6 +274,16 @@
and changes the signature of the protected
<code>getDirectoryContextEnvironment()</code> method. (remm)
</fix>
+ <fix>
+ Stop pre-digesting the HTTP DIGEST client digest in
+ <code>JAASCallbackHandler</code>. The pre-digest, a leftover from the
+ pre-<code>CredentialHandler</code> design, could never match the
+ deterministic server digest and so made DIGEST authentication via
+ <code>JAASRealm</code> always fail whenever the realm's credential
+ handler algorithm matched the digest algorithm. The client digest is
+ now passed to the <code>LoginModule</code> unchanged, consistent with
+ the non-JAAS realms. (remm)
+ </fix>
</changelog>
</subsection>
<subsection name="Coyote">
diff --git a/webapps/docs/realm-howto.xml b/webapps/docs/realm-howto.xml
index e1959eafaa..8a73db5870 100644
--- a/webapps/docs/realm-howto.xml
+++ b/webapps/docs/realm-howto.xml
@@ -1021,9 +1021,11 @@ and restarting the server, without any code changes to your application.</li>
security information for an already authenticated user will <strong>not</strong>
be reflected until the next time that user logs on again.</li>
<li>As with other <code>Realm</code> implementations, digested passwords
- are supported if the <code><Realm></code> element in <code>server.xml</code>
- contains a <code>digest</code> attribute; JAASRealm's <code>CallbackHandler</code>
- will digest the password prior to passing it back to the <code>LoginModule</code></li>
+ are supported via the configured <code>CredentialHandler</code>.
+ JAASRealm's <code>CallbackHandler</code> passes the password or
+ client digest to the <code>LoginModule</code> unchanged; any digest
+ comparison is expected to be performed by the <code>LoginModule</code>
+ when it calls back into the <code>Realm</code></li>
</ul>
</subsection>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
(tomcat) 02/16: Track StartTlsResponse per connection in JNDIRealm
Posted by re...@apache.org.
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 98652e381832062f6fcc89d05ca9c689c30c43b8
Author: opencode <re...@apache.org>
AuthorDate: Wed Oct 7 15:10:56 2026 +0200
Track StartTlsResponse per connection in JNDIRealm
JNDIRealm stored the StartTlsResponse of the most recently negotiated
TLS session in a single realm-wide, non-volatile field. With
connectionPoolSize greater than one and useStartTls enabled, every
open() overwrote the reference and close() of any connection closed
whatever session the field happened to reference. Closing one pooled
connection - pool overflow in release(), failover paths, or stop -
therefore terminated the TLS session of a different, still active
pooled connection, causing spurious authentication failures, while
superseded sessions leaked. Concurrent open() calls also raced on the
non-volatile field.
Store the StartTlsResponse on the JNDIConnection that negotiated it
and close only the connection's own response when that connection is
closed. The response is now recorded only after negotiate() succeeds,
so a failed negotiation cannot leave a stale reference behind.
Include a changelog entry.
---
java/org/apache/catalina/realm/JNDIRealm.java | 45 ++++++++++++++++-----------
webapps/docs/changelog.xml | 7 +++++
2 files changed, 34 insertions(+), 18 deletions(-)
diff --git a/java/org/apache/catalina/realm/JNDIRealm.java b/java/org/apache/catalina/realm/JNDIRealm.java
index cf016af936..4c1e4dde3e 100644
--- a/java/org/apache/catalina/realm/JNDIRealm.java
+++ b/java/org/apache/catalina/realm/JNDIRealm.java
@@ -345,8 +345,6 @@ public class JNDIRealm extends RealmBase {
*/
private boolean useStartTls = false;
- private StartTlsResponse tls = null;
-
/**
* The list of enabled cipher suites used for establishing tls connections. <code>null</code> means to use the
* default cipher suites.
@@ -2388,11 +2386,13 @@ public class JNDIRealm extends RealmBase {
}
// Close tls startResponse if used
- if (tls != null) {
+ if (connection.tls != null) {
try {
- tls.close();
+ connection.tls.close();
} catch (IOException ioe) {
containerLog.error(sm.getString("jndiRealm.tlsClose"), ioe);
+ } finally {
+ connection.tls = null;
}
}
// Close our opened connection
@@ -2692,7 +2692,7 @@ public class JNDIRealm extends RealmBase {
protected void open(JNDIConnection connection) throws NamingException {
try {
// Ensure that we have a directory context available
- connection.context = createDirContext(getDirectoryContextEnvironment());
+ connection.context = createDirContext(connection, getDirectoryContextEnvironment());
} catch (Exception e) {
if (alternateURL == null || alternateURL.isEmpty()) {
// No alternate URL. Re-throw the exception.
@@ -2702,7 +2702,7 @@ public class JNDIRealm extends RealmBase {
// log the first exception.
containerLog.info(sm.getString("jndiRealm.exception.retry"), e);
// Try connecting to the alternate url.
- connection.context = createDirContext(getDirectoryContextEnvironment());
+ connection.context = createDirContext(connection, getDirectoryContextEnvironment());
} finally {
// reset it in case the connection times out.
// the primary may come back.
@@ -2718,9 +2718,10 @@ public class JNDIRealm extends RealmBase {
}
- private DirContext createDirContext(Hashtable<String,String> env) throws NamingException {
+ private DirContext createDirContext(JNDIConnection connection, Hashtable<String,String> env)
+ throws NamingException {
if (useStartTls) {
- return createTlsDirContext(env);
+ return createTlsDirContext(connection, env);
} else {
return new InitialDirContext(env);
}
@@ -2775,15 +2776,17 @@ public class JNDIRealm extends RealmBase {
/**
- * Create a tls enabled LdapContext and set the StartTlsResponse tls instance variable.
+ * Create a tls enabled LdapContext and set the StartTlsResponse tls instance field of the connection.
*
- * @param env Environment to use for context creation
+ * @param connection The directory server connection wrapper
+ * @param env Environment to use for context creation
*
* @return configured {@link LdapContext}
*
* @throws NamingException when something goes wrong while negotiating the connection
*/
- private DirContext createTlsDirContext(Hashtable<String,String> env) throws NamingException {
+ private DirContext createTlsDirContext(JNDIConnection connection, Hashtable<String,String> env)
+ throws NamingException {
Map<String,Object> savedEnv = new HashMap<>();
for (String key : Arrays.asList(Context.SECURITY_AUTHENTICATION, Context.SECURITY_CREDENTIALS,
Context.SECURITY_PRINCIPAL, Context.SECURITY_PROTOCOL)) {
@@ -2795,16 +2798,17 @@ public class JNDIRealm extends RealmBase {
LdapContext result = null;
try {
result = new InitialLdapContext(env, null);
- tls = (StartTlsResponse) result.extendedOperation(new StartTlsRequest());
- if (getHostnameVerifier() != null) {
- tls.setHostnameVerifier(getHostnameVerifier());
- }
- if (getCipherSuitesArray() != null) {
- tls.setEnabledCipherSuites(getCipherSuitesArray());
- }
+ StartTlsResponse tls = (StartTlsResponse) result.extendedOperation(new StartTlsRequest());
try {
+ if (getHostnameVerifier() != null) {
+ tls.setHostnameVerifier(getHostnameVerifier());
+ }
+ if (getCipherSuitesArray() != null) {
+ tls.setEnabledCipherSuites(getCipherSuitesArray());
+ }
SSLSession negotiate = tls.negotiate(getSSLSocketFactory());
containerLog.debug(sm.getString("jndiRealm.negotiatedTls", negotiate.getProtocol()));
+ connection.tls = tls;
} catch (IOException ioe) {
NamingException ne = new NamingException(ioe.getMessage());
ne.initCause(ioe);
@@ -3352,6 +3356,11 @@ public class JNDIRealm extends RealmBase {
*/
public volatile DirContext context = null;
+ /**
+ * The {@link StartTlsResponse} negotiated for this connection, if any.
+ */
+ public volatile StartTlsResponse tls = null;
+
/**
* Constructs a new JNDIConnection.
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index e72f796cd5..d115e5e3bd 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -258,6 +258,13 @@
<code>JNDIRealm</code>, so that groups nested more than one level deep
are found when <code>roleBase</code> uses pattern replacements. (remm)
</fix>
+ <fix>
+ Track the <code>StartTlsResponse</code> used by <code>JNDIRealm</code>
+ per connection rather than in a single realm-wide field, so that
+ closing one pooled connection when <code>useStartTls</code> is enabled
+ no longer terminates the TLS session of another, still active pooled
+ connection. (remm)
+ </fix>
</changelog>
</subsection>
<subsection name="Coyote">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
(tomcat) 09/16: Reject non-positive check periods in TLSCertificateReloadListener
Posted by re...@apache.org.
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 3518d1045bdce41e7c00aeef2006c7b2de7401b7
Author: opencode <re...@apache.org>
AuthorDate: Wed Oct 7 16:37:44 2026 +0200
Reject non-positive check periods in TLSCertificateReloadListener
A check period of zero or less left the next scheduled check in the
past, so the certificate renewal check - including keystore re-reads
and SSLContext recreation for any SSLHostConfig with a certificate
inside the renewal window - ran on every periodic event (every ten
seconds by default) instead of once per check period. Warn and retain
the current value when a non-positive period is configured.
---
.../catalina/security/LocalStrings.properties | 1 +
.../security/TLSCertificateReloadListener.java | 9 ++++-
.../security/TestTLSCertificateReloadListener.java | 38 ++++++++++++++++++++++
3 files changed, 47 insertions(+), 1 deletion(-)
diff --git a/java/org/apache/catalina/security/LocalStrings.properties b/java/org/apache/catalina/security/LocalStrings.properties
index f22897c3ca..aafe327c90 100644
--- a/java/org/apache/catalina/security/LocalStrings.properties
+++ b/java/org/apache/catalina/security/LocalStrings.properties
@@ -24,6 +24,7 @@ SecurityListener.checkUserWarning=Start attempted while running as user [{0}]. R
listener.notServer=This listener must only be nested within Server elements, but is in [{0}].
+tlsCertRenewalListener.invalidCheckPeriod=The check period [{0}] is invalid. It must be greater than zero. The current value will be retained.
tlsCertRenewalListener.notRenewed=[{0}], TLS virtual host [{1}] certificate with subject [{2}] that expires on [{3}] is overdue for renewal
tlsCertRenewalListener.reloadFailed=[{0}], TLS virtual host [{1}] reload of TLS configuration failed
tlsCertRenewalListener.reloadSuccess=[{0}], TLS virtual host [{1}] attempted to reload TLS configuration
diff --git a/java/org/apache/catalina/security/TLSCertificateReloadListener.java b/java/org/apache/catalina/security/TLSCertificateReloadListener.java
index e48d56276f..dd31132653 100644
--- a/java/org/apache/catalina/security/TLSCertificateReloadListener.java
+++ b/java/org/apache/catalina/security/TLSCertificateReloadListener.java
@@ -78,11 +78,18 @@ public class TLSCertificateReloadListener implements LifecycleListener {
/**
- * Set the time, in seconds, between reloading checks.
+ * Set the time, in seconds, between reloading checks. Values less than or equal to zero are rejected and the
+ * current value is retained.
*
* @param checkPeriod The new time, in seconds, between reloading checks
*/
public void setCheckPeriod(int checkPeriod) {
+ if (checkPeriod <= 0) {
+ // A non-positive period would leave the next check time in the
+ // past, causing the reload check to run on every periodic event.
+ log.warn(sm.getString("tlsCertRenewalListener.invalidCheckPeriod", checkPeriod));
+ return;
+ }
this.checkPeriod = checkPeriod;
}
diff --git a/test/org/apache/catalina/security/TestTLSCertificateReloadListener.java b/test/org/apache/catalina/security/TestTLSCertificateReloadListener.java
new file mode 100644
index 0000000000..3a86e5506d
--- /dev/null
+++ b/test/org/apache/catalina/security/TestTLSCertificateReloadListener.java
@@ -0,0 +1,38 @@
+/*
+ * 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.security;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+public class TestTLSCertificateReloadListener {
+
+ @Test
+ public void testSetCheckPeriodRejectsNonPositive() {
+ TLSCertificateReloadListener listener = new TLSCertificateReloadListener();
+ int defaultPeriod = listener.getCheckPeriod();
+
+ listener.setCheckPeriod(0);
+ Assert.assertEquals(defaultPeriod, listener.getCheckPeriod());
+
+ listener.setCheckPeriod(-1);
+ Assert.assertEquals(defaultPeriod, listener.getCheckPeriod());
+
+ listener.setCheckPeriod(60);
+ Assert.assertEquals(60, listener.getCheckPeriod());
+ }
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: (tomcat) 15/16: Validate the principal object graph before persisting it in StandardSession
Posted by Rémy Maucherat <re...@apache.org>.
On Fri, Oct 16, 2026 at 9:29 AM Mark Thomas <ma...@apache.org> wrote:
>
> On 14/10/2026 14:14, Mark Thomas wrote:
> > On 14/10/2026 11:56, Rémy Maucherat wrote:
> >> On Wed, Oct 14, 2026 at 12:31 PM Mark Thomas <ma...@apache.org> wrote:
> >>>
> >>> I am tempted to refactor this. Assuming that Principals are normally
> >>> serializable, the serializing and throwing away seems wasteful.
> >>>
> >>> I am leaning towards trying to write the Principal and catching the
> >>> NotSerializableException. I'll see what that looks like.
> >>
> >> Yes, so the idea is to not corrupt everything which seemed like a good
> >> plan. As for the solution it is of course relatively wasteful since it
> >> serializes to a nested byte stream. It didn't seem too bad to me so I
> >> validated it.
> >
> > It is a trade-off. This approach is more robust as it handles custom
> > serailization safely whereas catching NotSerializableException may fail
> > if the Principal uses custom serialization.
> >
> > That said, attributes are currently using the catch
> > NotSerializableException approach and switching to this more robust
> > approach would be expensive for every attribute.
> >
> > Principals might be more out of the application's control so I can see
> > an argument for more robust checks compared to attributes.
> >
> > I think you have convinced me to keep this code. I'll extend the same
> > fix to Delta session.
>
> I had a few ideas for alternative solutions overnight but unfortunately
> none of them were workable.
>
> I have also been thinking more about this trade-off and looking at where
> else we test p instanceof Serializable and I have changed my mind on
> this change.
>
> I think the right approach is to catch and handle the
> NotSerializableException.
>
> My reasoning is as follows:
>
> - Principals returned by Tomcat Realms already handle this correctly.
> GenericPrinipcal has custom serialization that drops any
> non-serializable nested Principal
> - Therefore the problem only applies to Principals provided by 3rd-party
> authentication mechanisms
> - There is already a instanceof Serializable test
> - If a 3rd-party Principal passes that test but fails to serialize that
> is a bug in the 3rd-party Principal. To quote the Javadoc:
> "It is an error to declare a class Serializable if this is not the
> case."
> - This defence mechanism adds costs to all serialization events
> - The change effectively penalises everybody to mitigate the risk of a
> bug in 3rd-party code the vast majority will not be using
>
> Therefore, I intend to re-work this code to use the same approach as is
> currently used for session attributes. I have most of this already in a
> stash from when I looked at this earlier this week. I need to update the
> comments to include some of the above and check the tests.
+1
Ultimately it's indeed an unlikely situation, so this is very
reasonable. The most important is then to add a comment on the
decision so that the explanation is remembered.
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: (tomcat) 15/16: Validate the principal object graph before persisting it in StandardSession
Posted by Mark Thomas <ma...@apache.org>.
On 14/10/2026 14:14, Mark Thomas wrote:
> On 14/10/2026 11:56, Rémy Maucherat wrote:
>> On Wed, Oct 14, 2026 at 12:31 PM Mark Thomas <ma...@apache.org> wrote:
>>>
>>> I am tempted to refactor this. Assuming that Principals are normally
>>> serializable, the serializing and throwing away seems wasteful.
>>>
>>> I am leaning towards trying to write the Principal and catching the
>>> NotSerializableException. I'll see what that looks like.
>>
>> Yes, so the idea is to not corrupt everything which seemed like a good
>> plan. As for the solution it is of course relatively wasteful since it
>> serializes to a nested byte stream. It didn't seem too bad to me so I
>> validated it.
>
> It is a trade-off. This approach is more robust as it handles custom
> serailization safely whereas catching NotSerializableException may fail
> if the Principal uses custom serialization.
>
> That said, attributes are currently using the catch
> NotSerializableException approach and switching to this more robust
> approach would be expensive for every attribute.
>
> Principals might be more out of the application's control so I can see
> an argument for more robust checks compared to attributes.
>
> I think you have convinced me to keep this code. I'll extend the same
> fix to Delta session.
I had a few ideas for alternative solutions overnight but unfortunately
none of them were workable.
I have also been thinking more about this trade-off and looking at where
else we test p instanceof Serializable and I have changed my mind on
this change.
I think the right approach is to catch and handle the
NotSerializableException.
My reasoning is as follows:
- Principals returned by Tomcat Realms already handle this correctly.
GenericPrinipcal has custom serialization that drops any
non-serializable nested Principal
- Therefore the problem only applies to Principals provided by 3rd-party
authentication mechanisms
- There is already a instanceof Serializable test
- If a 3rd-party Principal passes that test but fails to serialize that
is a bug in the 3rd-party Principal. To quote the Javadoc:
"It is an error to declare a class Serializable if this is not the
case."
- This defence mechanism adds costs to all serialization events
- The change effectively penalises everybody to mitigate the risk of a
bug in 3rd-party code the vast majority will not be using
Therefore, I intend to re-work this code to use the same approach as is
currently used for session attributes. I have most of this already in a
stash from when I looked at this earlier this week. I need to update the
comments to include some of the above and check the tests.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: (tomcat) 15/16: Validate the principal object graph before persisting it in StandardSession
Posted by Mark Thomas <ma...@apache.org>.
On 14/10/2026 11:56, Rémy Maucherat wrote:
> On Wed, Oct 14, 2026 at 12:31 PM Mark Thomas <ma...@apache.org> wrote:
>>
>> I am tempted to refactor this. Assuming that Principals are normally
>> serializable, the serializing and throwing away seems wasteful.
>>
>> I am leaning towards trying to write the Principal and catching the
>> NotSerializableException. I'll see what that looks like.
>
> Yes, so the idea is to not corrupt everything which seemed like a good
> plan. As for the solution it is of course relatively wasteful since it
> serializes to a nested byte stream. It didn't seem too bad to me so I
> validated it.
It is a trade-off. This approach is more robust as it handles custom
serailization safely whereas catching NotSerializableException may fail
if the Principal uses custom serialization.
That said, attributes are currently using the catch
NotSerializableException approach and switching to this more robust
approach would be expensive for every attribute.
Principals might be more out of the application's control so I can see
an argument for more robust checks compared to attributes.
I think you have convinced me to keep this code. I'll extend the same
fix to Delta session.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: (tomcat) 15/16: Validate the principal object graph before persisting it in StandardSession
Posted by Rémy Maucherat <re...@apache.org>.
On Wed, Oct 14, 2026 at 12:31 PM Mark Thomas <ma...@apache.org> wrote:
>
> I am tempted to refactor this. Assuming that Principals are normally
> serializable, the serializing and throwing away seems wasteful.
>
> I am leaning towards trying to write the Principal and catching the
> NotSerializableException. I'll see what that looks like.
Yes, so the idea is to not corrupt everything which seemed like a good
plan. As for the solution it is of course relatively wasteful since it
serializes to a nested byte stream. It didn't seem too bad to me so I
validated it.
Rémy
> Mark
>
>
> On 07/10/2026 17:49, remm@apache.org wrote:
> > 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 52dfcd3f4b56f83feae4f60eb40b3f899c6a43a9
> > Author: opencode <re...@apache.org>
> > AuthorDate: Wed Oct 7 18:05:37 2026 +0200
> >
> > Validate the principal object graph before persisting it in StandardSession
> >
> > With persistAuthentication enabled, only the top-level Principal was
> > checked for Serializable. A principal whose object graph contained a
> > non-serializable element threw NotSerializableException mid-write from
> > the swallowing try/catch around stream.writeObject(principal), leaving
> > partially written object bytes in the stream. The later load then
> > failed with a WriteAbortedException or ClassCastException, corrupting
> > the whole persisted session record while the write side reported
> > success.
> >
> > Serialize the graph into a throwaway buffer first, dropping the
> > principal and warning when the graph is not serializable, matching the
> > behaviour of the existing top-level check.
> > ---
> > .../apache/catalina/session/StandardSession.java | 22 +++++--
> > .../catalina/session/TestStandardManager.java | 67 ++++++++++++++++++++++
> > 2 files changed, 85 insertions(+), 4 deletions(-)
> >
> > diff --git a/java/org/apache/catalina/session/StandardSession.java b/java/org/apache/catalina/session/StandardSession.java
> > index 55c2bd6080..6f53bb1b93 100644
> > --- a/java/org/apache/catalina/session/StandardSession.java
> > +++ b/java/org/apache/catalina/session/StandardSession.java
> > @@ -17,6 +17,7 @@
> > package org.apache.catalina.session;
> >
> > import java.beans.PropertyChangeSupport;
> > +import java.io.ByteArrayOutputStream;
> > import java.io.IOException;
> > import java.io.NotSerializableException;
> > import java.io.ObjectInputStream;
> > @@ -1300,11 +1301,24 @@ public class StandardSession implements HttpSession, Session, Serializable {
> >
> > // Write authentication information (may be null values)
> > stream.writeObject(sessionAuthType);
> > - try {
> > - stream.writeObject(sessionPrincipal);
> > - } catch (NotSerializableException e) {
> > - manager.getContext().getLogger().warn(sm.getString("standardSession.principalNotSerializable", id), e);
> > + if (sessionPrincipal != null) {
> > + /*
> > + * The instanceof check above only covers the principal itself.
> > + * Verify that the complete object graph is serializable before
> > + * writing to the session stream, since a NotSerializableException
> > + * thrown for a nested element mid-write would leave the session
> > + * record corrupted and unreadable. Discard the principal in the
> > + * same way as above if the graph is not serializable.
> > + */
> > + try (ByteArrayOutputStream baos = new ByteArrayOutputStream();
> > + ObjectOutputStream principalStream = new ObjectOutputStream(baos)) {
> > + principalStream.writeObject(sessionPrincipal);
> > + } catch (NotSerializableException e) {
> > + sessionPrincipal = null;
> > + manager.getContext().getLogger().warn(sm.getString("standardSession.principalNotSerializable", id), e);
> > + }
> > }
> > + stream.writeObject(sessionPrincipal);
> > stream.writeObject(expectedSessionId);
> > stream.writeObject(savedRequest);
> >
> > diff --git a/test/org/apache/catalina/session/TestStandardManager.java b/test/org/apache/catalina/session/TestStandardManager.java
> > index 2a9edeba26..4f42f526aa 100644
> > --- a/test/org/apache/catalina/session/TestStandardManager.java
> > +++ b/test/org/apache/catalina/session/TestStandardManager.java
> > @@ -20,6 +20,7 @@ import java.io.File;
> > import java.io.FileOutputStream;
> > import java.io.ObjectOutputStream;
> > import java.io.Serializable;
> > +import java.security.Principal;
> > import java.util.concurrent.atomic.AtomicInteger;
> >
> > import jakarta.servlet.http.HttpSessionActivationListener;
> > @@ -29,6 +30,7 @@ import jakarta.servlet.http.HttpSessionListener;
> > import org.junit.Assert;
> > import org.junit.Test;
> >
> > +import org.apache.catalina.Session;
> > import org.apache.catalina.startup.ExpandWar;
> > import org.apache.tomcat.unittest.TesterContext;
> > import org.apache.tomcat.unittest.TesterHost;
> > @@ -60,6 +62,26 @@ public class TestStandardManager {
> > }
> > }
> >
> > + public static class TesterNonSerializableGraphPrincipal implements Principal, Serializable {
> > +
> > + private static final long serialVersionUID = 1L;
> > +
> > + private final String name;
> > +
> > + @SuppressWarnings("unused")
> > + private final Object notSerializable = new Object();
> > +
> > + public TesterNonSerializableGraphPrincipal(String name) {
> > + this.name = name;
> > + }
> > +
> > + @Override
> > + public String getName() {
> > + return name;
> > + }
> > + }
> > +
> > +
> > /*
> > * A session that was persisted as invalid (which can happen when a
> > * session expires concurrently with unload()) must be discarded on load
> > @@ -141,4 +163,49 @@ public class TestStandardManager {
> > ExpandWar.delete(dir);
> > }
> > }
> > +
> > + /*
> > + * A principal whose own class is Serializable but whose object graph
> > + * contains a non-serializable element must not corrupt the persisted
> > + * session record. The principal is dropped instead, keeping the rest of
> > + * the session loadable.
> > + */
> > + @Test
> > + public void testUnloadDropsPrincipalWithNonSerializableGraph() throws Exception {
> > + File dir = new File("SESS_TEMP_STANDARD_PRINCIPAL");
> > + ExpandWar.delete(dir);
> > + Assert.assertTrue(dir.mkdirs());
> > + File file = new File(dir, "TEST_SESSIONS.ser");
> > + try {
> > + StandardManager manager = new StandardManager();
> > +
> > + TesterContext context = new TesterContext();
> > + context.setServletContext(new TesterServletContext());
> > + context.setParent(new TesterHost());
> > + manager.setContext(context);
> > +
> > + manager.setPathname(file.getAbsolutePath());
> > + manager.setPersistAuthentication(true);
> > + manager.start();
> > +
> > + Session session = manager.createSession(VALID_ID);
> > + session.setPrincipal(new TesterNonSerializableGraphPrincipal("testUser"));
> > +
> > + manager.unload();
> > +
> > + try {
> > + manager.load();
> > + } catch (Throwable t) {
> > + // Before the fix, the NotSerializableException was swallowed
> > + // mid-write, corrupting the stream, so loading the file failed
> > + Assert.fail("Loading the persisted session failed: " + t);
> > + }
> > +
> > + Session loaded = manager.findSession(VALID_ID);
> > + Assert.assertNotNull(loaded);
> > + Assert.assertNull(loaded.getPrincipal());
> > + } finally {
> > + ExpandWar.delete(dir);
> > + }
> > + }
> > }
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: (tomcat) 15/16: Validate the principal object graph before persisting it in StandardSession
Posted by Mark Thomas <ma...@apache.org>.
I am tempted to refactor this. Assuming that Principals are normally
serializable, the serializing and throwing away seems wasteful.
I am leaning towards trying to write the Principal and catching the
NotSerializableException. I'll see what that looks like.
Mark
On 07/10/2026 17:49, remm@apache.org wrote:
> 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 52dfcd3f4b56f83feae4f60eb40b3f899c6a43a9
> Author: opencode <re...@apache.org>
> AuthorDate: Wed Oct 7 18:05:37 2026 +0200
>
> Validate the principal object graph before persisting it in StandardSession
>
> With persistAuthentication enabled, only the top-level Principal was
> checked for Serializable. A principal whose object graph contained a
> non-serializable element threw NotSerializableException mid-write from
> the swallowing try/catch around stream.writeObject(principal), leaving
> partially written object bytes in the stream. The later load then
> failed with a WriteAbortedException or ClassCastException, corrupting
> the whole persisted session record while the write side reported
> success.
>
> Serialize the graph into a throwaway buffer first, dropping the
> principal and warning when the graph is not serializable, matching the
> behaviour of the existing top-level check.
> ---
> .../apache/catalina/session/StandardSession.java | 22 +++++--
> .../catalina/session/TestStandardManager.java | 67 ++++++++++++++++++++++
> 2 files changed, 85 insertions(+), 4 deletions(-)
>
> diff --git a/java/org/apache/catalina/session/StandardSession.java b/java/org/apache/catalina/session/StandardSession.java
> index 55c2bd6080..6f53bb1b93 100644
> --- a/java/org/apache/catalina/session/StandardSession.java
> +++ b/java/org/apache/catalina/session/StandardSession.java
> @@ -17,6 +17,7 @@
> package org.apache.catalina.session;
>
> import java.beans.PropertyChangeSupport;
> +import java.io.ByteArrayOutputStream;
> import java.io.IOException;
> import java.io.NotSerializableException;
> import java.io.ObjectInputStream;
> @@ -1300,11 +1301,24 @@ public class StandardSession implements HttpSession, Session, Serializable {
>
> // Write authentication information (may be null values)
> stream.writeObject(sessionAuthType);
> - try {
> - stream.writeObject(sessionPrincipal);
> - } catch (NotSerializableException e) {
> - manager.getContext().getLogger().warn(sm.getString("standardSession.principalNotSerializable", id), e);
> + if (sessionPrincipal != null) {
> + /*
> + * The instanceof check above only covers the principal itself.
> + * Verify that the complete object graph is serializable before
> + * writing to the session stream, since a NotSerializableException
> + * thrown for a nested element mid-write would leave the session
> + * record corrupted and unreadable. Discard the principal in the
> + * same way as above if the graph is not serializable.
> + */
> + try (ByteArrayOutputStream baos = new ByteArrayOutputStream();
> + ObjectOutputStream principalStream = new ObjectOutputStream(baos)) {
> + principalStream.writeObject(sessionPrincipal);
> + } catch (NotSerializableException e) {
> + sessionPrincipal = null;
> + manager.getContext().getLogger().warn(sm.getString("standardSession.principalNotSerializable", id), e);
> + }
> }
> + stream.writeObject(sessionPrincipal);
> stream.writeObject(expectedSessionId);
> stream.writeObject(savedRequest);
>
> diff --git a/test/org/apache/catalina/session/TestStandardManager.java b/test/org/apache/catalina/session/TestStandardManager.java
> index 2a9edeba26..4f42f526aa 100644
> --- a/test/org/apache/catalina/session/TestStandardManager.java
> +++ b/test/org/apache/catalina/session/TestStandardManager.java
> @@ -20,6 +20,7 @@ import java.io.File;
> import java.io.FileOutputStream;
> import java.io.ObjectOutputStream;
> import java.io.Serializable;
> +import java.security.Principal;
> import java.util.concurrent.atomic.AtomicInteger;
>
> import jakarta.servlet.http.HttpSessionActivationListener;
> @@ -29,6 +30,7 @@ import jakarta.servlet.http.HttpSessionListener;
> import org.junit.Assert;
> import org.junit.Test;
>
> +import org.apache.catalina.Session;
> import org.apache.catalina.startup.ExpandWar;
> import org.apache.tomcat.unittest.TesterContext;
> import org.apache.tomcat.unittest.TesterHost;
> @@ -60,6 +62,26 @@ public class TestStandardManager {
> }
> }
>
> + public static class TesterNonSerializableGraphPrincipal implements Principal, Serializable {
> +
> + private static final long serialVersionUID = 1L;
> +
> + private final String name;
> +
> + @SuppressWarnings("unused")
> + private final Object notSerializable = new Object();
> +
> + public TesterNonSerializableGraphPrincipal(String name) {
> + this.name = name;
> + }
> +
> + @Override
> + public String getName() {
> + return name;
> + }
> + }
> +
> +
> /*
> * A session that was persisted as invalid (which can happen when a
> * session expires concurrently with unload()) must be discarded on load
> @@ -141,4 +163,49 @@ public class TestStandardManager {
> ExpandWar.delete(dir);
> }
> }
> +
> + /*
> + * A principal whose own class is Serializable but whose object graph
> + * contains a non-serializable element must not corrupt the persisted
> + * session record. The principal is dropped instead, keeping the rest of
> + * the session loadable.
> + */
> + @Test
> + public void testUnloadDropsPrincipalWithNonSerializableGraph() throws Exception {
> + File dir = new File("SESS_TEMP_STANDARD_PRINCIPAL");
> + ExpandWar.delete(dir);
> + Assert.assertTrue(dir.mkdirs());
> + File file = new File(dir, "TEST_SESSIONS.ser");
> + try {
> + StandardManager manager = new StandardManager();
> +
> + TesterContext context = new TesterContext();
> + context.setServletContext(new TesterServletContext());
> + context.setParent(new TesterHost());
> + manager.setContext(context);
> +
> + manager.setPathname(file.getAbsolutePath());
> + manager.setPersistAuthentication(true);
> + manager.start();
> +
> + Session session = manager.createSession(VALID_ID);
> + session.setPrincipal(new TesterNonSerializableGraphPrincipal("testUser"));
> +
> + manager.unload();
> +
> + try {
> + manager.load();
> + } catch (Throwable t) {
> + // Before the fix, the NotSerializableException was swallowed
> + // mid-write, corrupting the stream, so loading the file failed
> + Assert.fail("Loading the persisted session failed: " + t);
> + }
> +
> + Session loaded = manager.findSession(VALID_ID);
> + Assert.assertNotNull(loaded);
> + Assert.assertNull(loaded.getPrincipal());
> + } finally {
> + ExpandWar.delete(dir);
> + }
> + }
> }
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
(tomcat) 15/16: Validate the principal object graph before persisting it in StandardSession
Posted by re...@apache.org.
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 52dfcd3f4b56f83feae4f60eb40b3f899c6a43a9
Author: opencode <re...@apache.org>
AuthorDate: Wed Oct 7 18:05:37 2026 +0200
Validate the principal object graph before persisting it in StandardSession
With persistAuthentication enabled, only the top-level Principal was
checked for Serializable. A principal whose object graph contained a
non-serializable element threw NotSerializableException mid-write from
the swallowing try/catch around stream.writeObject(principal), leaving
partially written object bytes in the stream. The later load then
failed with a WriteAbortedException or ClassCastException, corrupting
the whole persisted session record while the write side reported
success.
Serialize the graph into a throwaway buffer first, dropping the
principal and warning when the graph is not serializable, matching the
behaviour of the existing top-level check.
---
.../apache/catalina/session/StandardSession.java | 22 +++++--
.../catalina/session/TestStandardManager.java | 67 ++++++++++++++++++++++
2 files changed, 85 insertions(+), 4 deletions(-)
diff --git a/java/org/apache/catalina/session/StandardSession.java b/java/org/apache/catalina/session/StandardSession.java
index 55c2bd6080..6f53bb1b93 100644
--- a/java/org/apache/catalina/session/StandardSession.java
+++ b/java/org/apache/catalina/session/StandardSession.java
@@ -17,6 +17,7 @@
package org.apache.catalina.session;
import java.beans.PropertyChangeSupport;
+import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.NotSerializableException;
import java.io.ObjectInputStream;
@@ -1300,11 +1301,24 @@ public class StandardSession implements HttpSession, Session, Serializable {
// Write authentication information (may be null values)
stream.writeObject(sessionAuthType);
- try {
- stream.writeObject(sessionPrincipal);
- } catch (NotSerializableException e) {
- manager.getContext().getLogger().warn(sm.getString("standardSession.principalNotSerializable", id), e);
+ if (sessionPrincipal != null) {
+ /*
+ * The instanceof check above only covers the principal itself.
+ * Verify that the complete object graph is serializable before
+ * writing to the session stream, since a NotSerializableException
+ * thrown for a nested element mid-write would leave the session
+ * record corrupted and unreadable. Discard the principal in the
+ * same way as above if the graph is not serializable.
+ */
+ try (ByteArrayOutputStream baos = new ByteArrayOutputStream();
+ ObjectOutputStream principalStream = new ObjectOutputStream(baos)) {
+ principalStream.writeObject(sessionPrincipal);
+ } catch (NotSerializableException e) {
+ sessionPrincipal = null;
+ manager.getContext().getLogger().warn(sm.getString("standardSession.principalNotSerializable", id), e);
+ }
}
+ stream.writeObject(sessionPrincipal);
stream.writeObject(expectedSessionId);
stream.writeObject(savedRequest);
diff --git a/test/org/apache/catalina/session/TestStandardManager.java b/test/org/apache/catalina/session/TestStandardManager.java
index 2a9edeba26..4f42f526aa 100644
--- a/test/org/apache/catalina/session/TestStandardManager.java
+++ b/test/org/apache/catalina/session/TestStandardManager.java
@@ -20,6 +20,7 @@ import java.io.File;
import java.io.FileOutputStream;
import java.io.ObjectOutputStream;
import java.io.Serializable;
+import java.security.Principal;
import java.util.concurrent.atomic.AtomicInteger;
import jakarta.servlet.http.HttpSessionActivationListener;
@@ -29,6 +30,7 @@ import jakarta.servlet.http.HttpSessionListener;
import org.junit.Assert;
import org.junit.Test;
+import org.apache.catalina.Session;
import org.apache.catalina.startup.ExpandWar;
import org.apache.tomcat.unittest.TesterContext;
import org.apache.tomcat.unittest.TesterHost;
@@ -60,6 +62,26 @@ public class TestStandardManager {
}
}
+ public static class TesterNonSerializableGraphPrincipal implements Principal, Serializable {
+
+ private static final long serialVersionUID = 1L;
+
+ private final String name;
+
+ @SuppressWarnings("unused")
+ private final Object notSerializable = new Object();
+
+ public TesterNonSerializableGraphPrincipal(String name) {
+ this.name = name;
+ }
+
+ @Override
+ public String getName() {
+ return name;
+ }
+ }
+
+
/*
* A session that was persisted as invalid (which can happen when a
* session expires concurrently with unload()) must be discarded on load
@@ -141,4 +163,49 @@ public class TestStandardManager {
ExpandWar.delete(dir);
}
}
+
+ /*
+ * A principal whose own class is Serializable but whose object graph
+ * contains a non-serializable element must not corrupt the persisted
+ * session record. The principal is dropped instead, keeping the rest of
+ * the session loadable.
+ */
+ @Test
+ public void testUnloadDropsPrincipalWithNonSerializableGraph() throws Exception {
+ File dir = new File("SESS_TEMP_STANDARD_PRINCIPAL");
+ ExpandWar.delete(dir);
+ Assert.assertTrue(dir.mkdirs());
+ File file = new File(dir, "TEST_SESSIONS.ser");
+ try {
+ StandardManager manager = new StandardManager();
+
+ TesterContext context = new TesterContext();
+ context.setServletContext(new TesterServletContext());
+ context.setParent(new TesterHost());
+ manager.setContext(context);
+
+ manager.setPathname(file.getAbsolutePath());
+ manager.setPersistAuthentication(true);
+ manager.start();
+
+ Session session = manager.createSession(VALID_ID);
+ session.setPrincipal(new TesterNonSerializableGraphPrincipal("testUser"));
+
+ manager.unload();
+
+ try {
+ manager.load();
+ } catch (Throwable t) {
+ // Before the fix, the NotSerializableException was swallowed
+ // mid-write, corrupting the stream, so loading the file failed
+ Assert.fail("Loading the persisted session failed: " + t);
+ }
+
+ Session loaded = manager.findSession(VALID_ID);
+ Assert.assertNotNull(loaded);
+ Assert.assertNull(loaded.getPrincipal());
+ } finally {
+ ExpandWar.delete(dir);
+ }
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
(tomcat) 13/16: Use long arithmetic when calculating session rates in ManagerBase
Posted by re...@apache.org.
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 a1b3b73c556eb0670f914b3a403f0a83cca6ba4c
Author: opencode <re...@apache.org>
AuthorDate: Wed Oct 7 17:32:24 2026 +0200
Use long arithmetic when calculating session rates in ManagerBase
calculateRate() cast the millisecond span covered by the timing cache
to int. On applications sparse enough for the cache to span more than
2^31 milliseconds, the cast overflowed and the JMX rate attributes were
reported as negative values or, when the truncated span was zero, the
division threw an ArithmeticException.
Perform the calculation in long arithmetic.
---
java/org/apache/catalina/session/ManagerBase.java | 5 +-
.../apache/catalina/session/TestManagerBase.java | 63 ++++++++++++++++++++++
2 files changed, 66 insertions(+), 2 deletions(-)
diff --git a/java/org/apache/catalina/session/ManagerBase.java b/java/org/apache/catalina/session/ManagerBase.java
index 6ba65d4873..f12be206ee 100644
--- a/java/org/apache/catalina/session/ManagerBase.java
+++ b/java/org/apache/catalina/session/ManagerBase.java
@@ -1099,8 +1099,9 @@ public abstract class ManagerBase extends LifecycleMBeanBase implements Manager
}
}
if (counter > 0) {
- if (oldest < now) {
- result = (1000 * 60 * counter) / (int) (now - oldest);
+ long span = now - oldest;
+ if (span > 0) {
+ result = (int) ((1000L * 60 * counter) / span);
} else {
// Better than reporting zero
result = Integer.MAX_VALUE;
diff --git a/test/org/apache/catalina/session/TestManagerBase.java b/test/org/apache/catalina/session/TestManagerBase.java
new file mode 100644
index 0000000000..82f59ba48c
--- /dev/null
+++ b/test/org/apache/catalina/session/TestManagerBase.java
@@ -0,0 +1,63 @@
+/*
+ * 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.session;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import org.apache.catalina.session.ManagerBase.SessionTiming;
+
+public class TestManagerBase {
+
+ /*
+ * Timing entries more than 2^31 milliseconds old must produce a sensible
+ * rate rather than the result of an overflowing cast to int, which could
+ * be negative or lead to a division by zero.
+ */
+ @Test
+ public void testSessionRateWithSpanOverIntMax() {
+ StandardManager manager = new StandardManager();
+
+ long now = System.currentTimeMillis();
+
+ manager.sessionCreationTiming.clear();
+ manager.sessionCreationTiming.add(new SessionTiming(now - (1L << 32), 100));
+
+ // Before the fix the delta was cast to int, yielding zero (division by
+ // zero) or a small garbage value for spans that are a multiple of
+ // 2^32
+ Assert.assertEquals(0, manager.getSessionCreateRate());
+ }
+
+ @Test
+ public void testSessionRateWithSpanOverflowingToNegative() {
+ StandardManager manager = new StandardManager();
+
+ long now = System.currentTimeMillis();
+
+ // A span just under 2^32 casts to a negative int
+ long span = (1L << 32) - 3000;
+
+ manager.sessionCreationTiming.clear();
+ for (int i = 0; i < 100; i++) {
+ manager.sessionCreationTiming.add(new SessionTiming(now - span, 100));
+ }
+
+ // Before the fix this returned a negative rate
+ Assert.assertEquals(0, manager.getSessionCreateRate());
+ }
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
(tomcat) 07/16: Report missing user credentials as an authentication failure in JNDIRealm
Posted by re...@apache.org.
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 0dbb3feb010ef145bcb365bee061ef13c7e2e1f9
Author: opencode <re...@apache.org>
AuthorDate: Wed Oct 7 16:15:23 2026 +0200
Report missing user credentials as an authentication failure in JNDIRealm
userCredentialsAdd() put the user credentials directly into the JNDI
environment, which is a Hashtable that rejects null values. When an
as-user search was reached without credentials, for example with
userSearchAsUser or roleSearchAsUser enabled and a GSS or client
certificate principal lookup, the realm failed with a raw
NullPointerException instead of a clean authentication failure.
Throw an AuthenticationException with a clear message when no
credentials are available for the user. The check runs before the
environment is modified, so pooled connections are never left with
partial security settings.
---
java/org/apache/catalina/realm/JNDIRealm.java | 8 +-
.../apache/catalina/realm/LocalStrings.properties | 1 +
.../catalina/realm/TestJNDIRealmSearchAsUser.java | 99 ++++++++++++++++++++++
3 files changed, 107 insertions(+), 1 deletion(-)
diff --git a/java/org/apache/catalina/realm/JNDIRealm.java b/java/org/apache/catalina/realm/JNDIRealm.java
index 4c8601488b..b8396dc5df 100644
--- a/java/org/apache/catalina/realm/JNDIRealm.java
+++ b/java/org/apache/catalina/realm/JNDIRealm.java
@@ -2003,9 +2003,15 @@ public class JNDIRealm extends RealmBase {
* @param dn Distinguished name of user
* @param credentials Credentials of user
*
- * @exception NamingException if a directory server error occurs
+ * @exception NamingException if a directory server error occurs or no credentials are available for the user
*/
private void userCredentialsAdd(DirContext context, String dn, String credentials) throws NamingException {
+ if (credentials == null) {
+ // The JNDI environment is a Hashtable that does not accept null
+ // values. Without credentials, searching or binding as the user is
+ // not possible.
+ throw new AuthenticationException(sm.getString("jndiRealm.noUserCredentials", dn));
+ }
// Set up security environment to bind as the user
context.addToEnvironment(Context.SECURITY_PRINCIPAL, dn);
context.addToEnvironment(Context.SECURITY_CREDENTIALS, credentials);
diff --git a/java/org/apache/catalina/realm/LocalStrings.properties b/java/org/apache/catalina/realm/LocalStrings.properties
index 42aec0e181..99630ed3be 100644
--- a/java/org/apache/catalina/realm/LocalStrings.properties
+++ b/java/org/apache/catalina/realm/LocalStrings.properties
@@ -88,6 +88,7 @@ jndiRealm.invalidSslSocketFactory=[{0}] not a valid class name for an SSLSocketF
jndiRealm.invalidUserPattern=The user pattern [{0}] is not valid: it contains unbalanced parentheses
jndiRealm.multipleEntries=User name [{0}] has multiple entries
jndiRealm.negotiatedTls=Negotiated tls connection using protocol [{0}]
+jndiRealm.noUserCredentials=Cannot perform the search or bind as user [{0}]: no password is available for the user
jndiRealm.open=Exception opening directory server connection
jndiRealm.tlsClose=Exception closing tls response
diff --git a/test/org/apache/catalina/realm/TestJNDIRealmSearchAsUser.java b/test/org/apache/catalina/realm/TestJNDIRealmSearchAsUser.java
new file mode 100644
index 0000000000..e691423d35
--- /dev/null
+++ b/test/org/apache/catalina/realm/TestJNDIRealmSearchAsUser.java
@@ -0,0 +1,99 @@
+/*
+ * 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.realm;
+
+import java.net.InetAddress;
+
+import javax.naming.AuthenticationException;
+
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import org.apache.juli.logging.LogFactory;
+
+import com.unboundid.ldap.listener.InMemoryDirectoryServer;
+import com.unboundid.ldap.listener.InMemoryDirectoryServerConfig;
+import com.unboundid.ldap.listener.InMemoryListenerConfig;
+import com.unboundid.ldap.sdk.AddRequest;
+import com.unboundid.ldap.sdk.LDAPConnection;
+import com.unboundid.ldap.sdk.LDAPResult;
+import com.unboundid.ldap.sdk.ResultCode;
+
+public class TestJNDIRealmSearchAsUser {
+
+ private static InMemoryDirectoryServer ldapServer;
+
+ @BeforeClass
+ public static void createLDAP() throws Exception {
+ InMemoryDirectoryServerConfig config = new InMemoryDirectoryServerConfig("dc=example,dc=com");
+ InetAddress localhost = InetAddress.getByName("localhost");
+ InMemoryListenerConfig listenerConfig =
+ new InMemoryListenerConfig("localListener", localhost, 0, null, null, null);
+ config.setListenerConfigs(listenerConfig);
+ ldapServer = new InMemoryDirectoryServer(config);
+
+ ldapServer.startListening();
+
+ try (LDAPConnection conn = ldapServer.getConnection()) {
+ AddRequest addBase = new AddRequest(
+ "dn: dc=example,dc=com",
+ "objectClass: top",
+ "objectClass: domain",
+ "dc: example");
+ LDAPResult result = conn.processOperation(addBase);
+ Assert.assertEquals(ResultCode.SUCCESS, result.getResultCode());
+
+ AddRequest addUserTest = new AddRequest(
+ "dn: cn=test,dc=example,dc=com",
+ "objectClass: top",
+ "objectClass: person",
+ "objectClass: organizationalPerson",
+ "cn: test",
+ "sn: Test");
+ result = conn.processOperation(addUserTest);
+ Assert.assertEquals(ResultCode.SUCCESS, result.getResultCode());
+ }
+ }
+
+ @AfterClass
+ public static void destroyLDAP() {
+ ldapServer.shutDown(true);
+ }
+
+ @Test
+ public void testUserSearchAsUserWithoutCredentials() throws Exception {
+ JNDIRealm realm = new JNDIRealm();
+ realm.containerLog = LogFactory.getLog(TestJNDIRealmSearchAsUser.class);
+
+ realm.setConnectionURL("ldap://localhost:" + ldapServer.getListenPort());
+ realm.setUserSearch("cn={0}");
+ realm.setUserSearchAsUser(true);
+
+ JNDIRealm.JNDIConnection connection = realm.get();
+ try {
+ realm.getUser(connection, "test");
+ Assert.fail("Searching as the user was expected to fail since no " +
+ "password is available for the user");
+ } catch (AuthenticationException e) {
+ // Expected. Before the fix this was a NullPointerException.
+ } finally {
+ realm.release(connection);
+ }
+ }
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
(tomcat) 01/16: Fix nested role search DN computation in JNDIRealm
Posted by re...@apache.org.
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 df144924786a36dbec41bac3ae3af66fa31259b9
Author: opencode <re...@apache.org>
AuthorDate: Wed Oct 7 15:04:55 2026 +0200
Fix nested role search DN computation in JNDIRealm
In getRoles(), the direct role search computed result DNs using the
per-user formatted roleBase (the base actually searched) while the
nested group search round still used the raw roleBase field. When
roleBase contained {0}..{n} pattern replacements, the computed DNs
embedded the literal placeholder text so the member filter of the
next search round could never match and groups nested more than one
level deep were silently missed. With roleBase set to null, the raw
field also caused an NPE when composing relative result names.
This completes the fix applied in 81f16b0a71 which addressed the same
issue in the direct search path.
Add regression coverage: a group nested one level below the group
directly containing the user in TestJNDIRealmIntegration, exercised
with a patterned roleBase. The new parameter sets fail without the
fix. Include a changelog entry.
---
java/org/apache/catalina/realm/JNDIRealm.java | 5 ++---
.../apache/catalina/realm/TestJNDIRealmIntegration.java | 17 +++++++++++++++--
webapps/docs/changelog.xml | 6 ++++++
3 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/java/org/apache/catalina/realm/JNDIRealm.java b/java/org/apache/catalina/realm/JNDIRealm.java
index 77dd34dc84..cf016af936 100644
--- a/java/org/apache/catalina/realm/JNDIRealm.java
+++ b/java/org/apache/catalina/realm/JNDIRealm.java
@@ -2206,8 +2206,7 @@ public class JNDIRealm extends RealmBase {
doFilterEscaping(doAttributeValueEscaping(group.getValue())) });
if (containerLog.isTraceEnabled()) {
- containerLog
- .trace("Perform a nested group search with base " + roleBase + " and filter " + filter);
+ containerLog.trace("Perform a nested group search with base " + base + " and filter " + filter);
}
results = searchAsUser(connection.context, user, base, filter, controls, isRoleSearchAsUser());
@@ -2219,7 +2218,7 @@ public class JNDIRealm extends RealmBase {
if (attrs == null) {
continue;
}
- String dname = getDistinguishedName(connection.context, roleBase, result);
+ String dname = getDistinguishedName(connection.context, base, result);
String name = getAttributeValue(roleName, attrs);
if (name != null && dname != null && !groupMap.containsKey(dname)) {
groupMap.put(dname, name);
diff --git a/test/org/apache/catalina/realm/TestJNDIRealmIntegration.java b/test/org/apache/catalina/realm/TestJNDIRealmIntegration.java
index 060a95fe52..d16418d6e1 100644
--- a/test/org/apache/catalina/realm/TestJNDIRealmIntegration.java
+++ b/test/org/apache/catalina/realm/TestJNDIRealmIntegration.java
@@ -75,10 +75,12 @@ public class TestJNDIRealmIntegration {
}
}
parameterSets.add(new Object[] { "cn={0},ou=s\\;ub,ou=people,dc=example,dc=com", null, null, ROLE_SEARCH_A,
- "{3},ou=people,dc=example,dc=com", "testsub", "test", new String[] { "TestGroup4" },
+ "{3},ou=people,dc=example,dc=com", "testsub", "test",
+ new String[] { "TestGroup4", "TestGroup5" },
userRoleAttribute, Integer.valueOf(1) });
parameterSets.add(new Object[] { "cn={0},ou=s\\;ub,ou=people,dc=example,dc=com", null, null, ROLE_SEARCH_A,
- "{3},ou=people,dc=example,dc=com", "testsub", "test", new String[] { "TestGroup4" },
+ "{3},ou=people,dc=example,dc=com", "testsub", "test",
+ new String[] { "TestGroup4", "TestGroup5" },
userRoleAttribute, Integer.valueOf(4) });
}
/*
@@ -319,6 +321,17 @@ public class TestJNDIRealmIntegration {
result = conn.processOperation(addGroupTest4);
Assert.assertEquals(ResultCode.SUCCESS, result.getResultCode());
+ // Nested one level below TestGroup4 so the patterned roleBase must be resolved correctly for the
+ // member filter of the second nested search round to match
+ AddRequest addGroupTest5 = new AddRequest(
+ "dn: cn=TestGroup5,ou=s\\;ub,ou=people,dc=example,dc=com",
+ "objectClass: top",
+ "objectClass: groupOfNames",
+ "cn: TestGroup5",
+ "member: cn=TestGroup4,ou=s\\;ub,ou=people,dc=example,dc=com");
+ result = conn.processOperation(addGroupTest5);
+ Assert.assertEquals(ResultCode.SUCCESS, result.getResultCode());
+
// Bug 65373
AddRequest addUserBug65373 = new AddRequest(
"dn: cn=\\3C\\3E\\2B=\\22#\\3B\\2Crrr,ou=people,dc=example,dc=com",
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 07d3ff76b1..e72f796cd5 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -252,6 +252,12 @@
interface must implement this method. (markt)
</fix>
<!-- Entries for backport and removal before 12.0.0-M1 below this line -->
+ <fix>
+ Use the resolved <code>roleBase</code> value when computing the
+ distinguished name of a group found during a nested role search in
+ <code>JNDIRealm</code>, so that groups nested more than one level deep
+ are found when <code>roleBase</code> uses pattern replacements. (remm)
+ </fix>
</changelog>
</subsection>
<subsection name="Coyote">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
(tomcat) 16/16: Merge branch 'main' of git@github.com:apache/tomcat.git into main
Posted by re...@apache.org.
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 2840b562eb2acf85445f51ebdd693357b4d7a11a
Merge: 52dfcd3f4b 9e035e8d06
Author: remm <re...@apache.org>
AuthorDate: Wed Oct 7 18:20:56 2026 +0200
Merge branch 'main' of git@github.com:apache/tomcat.git into main
java/org/apache/catalina/Session.java | 4 ++--
.../apache/catalina/ha/session/DeltaManager.java | 2 +-
.../apache/catalina/ha/session/DeltaSession.java | 2 +-
.../apache/catalina/manager/ManagerServlet.java | 4 ++--
.../catalina/manager/host/HostManagerServlet.java | 24 +++++++++++++++++++++-
.../apache/catalina/session/StandardSession.java | 2 +-
.../catalina/ha/session/TestDeltaSession.java | 23 +++++++++++++++++++++
.../apache/catalina/manager/TestManagerWebapp.java | 7 ++++++-
webapps/docs/changelog.xml | 6 +++++-
9 files changed, 64 insertions(+), 10 deletions(-)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
(tomcat) 08/16: Trim whitespace when parsing the checkedOsUsers list in SecurityListener
Posted by re...@apache.org.
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 44561a80c54782e0582dfc01e2e1f4bffbc8befb
Author: opencode <re...@apache.org>
AuthorDate: Wed Oct 7 16:20:31 2026 +0200
Trim whitespace when parsing the checkedOsUsers list in SecurityListener
Entries in a comma separated checkedOsUsers configuration such as
"root, admin" were stored without trimming, so an entry containing
whitespace never matched the actual value of user.name and the
prohibition check was silently disabled for that user. Trim each
entry before adding it to the set of prohibited users.
---
.../apache/catalina/security/SecurityListener.java | 1 +
.../catalina/security/TestSecurityListener.java | 52 ++++++++++++++++++++++
2 files changed, 53 insertions(+)
diff --git a/java/org/apache/catalina/security/SecurityListener.java b/java/org/apache/catalina/security/SecurityListener.java
index 5acf512e30..c14405bad7 100644
--- a/java/org/apache/catalina/security/SecurityListener.java
+++ b/java/org/apache/catalina/security/SecurityListener.java
@@ -101,6 +101,7 @@ public class SecurityListener implements LifecycleListener {
} else {
String[] userNames = userNameList.split(",");
for (String userName : userNames) {
+ userName = userName.trim();
if (!userName.isEmpty()) {
checkedOsUsers.add(userName.toLowerCase(Locale.getDefault()));
}
diff --git a/test/org/apache/catalina/security/TestSecurityListener.java b/test/org/apache/catalina/security/TestSecurityListener.java
new file mode 100644
index 0000000000..14c7a3c5fb
--- /dev/null
+++ b/test/org/apache/catalina/security/TestSecurityListener.java
@@ -0,0 +1,52 @@
+/*
+ * 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.security;
+
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class TestSecurityListener {
+
+ private final String originalUserName = System.getProperty("user.name");
+
+ @After
+ public void restoreUserName() {
+ if (originalUserName != null) {
+ System.setProperty("user.name", originalUserName);
+ } else {
+ System.clearProperty("user.name");
+ }
+ }
+
+ @Test
+ public void testCheckOsUserWithWhitespaceInList() {
+ SecurityListener listener = new SecurityListener();
+ listener.setCheckedOsUsers("root, admin");
+
+ System.setProperty("user.name", "admin");
+ try {
+ listener.checkOsUser();
+ } catch (Error e) {
+ // Expected. Before the fix the whitespace was not trimmed so the
+ // entry never matched and no Error was thrown.
+ return;
+ }
+ Assert.fail("User [admin] was expected to be prohibited by the " +
+ "configured list [root, admin]");
+ }
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
(tomcat) 11/16: Store namespace-less WebDAV properties in DataSourcePropertyStore
Posted by re...@apache.org.
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 3866869e0552197e0aba5459993474fbc17b7a63
Author: opencode <re...@apache.org>
AuthorDate: Wed Oct 7 16:53:20 2026 +0200
Store namespace-less WebDAV properties in DataSourcePropertyStore
Node.getNamespaceURI() returns null for a property element without a
namespace, which was passed directly into the namespace column of the
queries. With the documented NOT NULL schema every SET, REMOVE and
single-property PROPFIND of a namespace-less property such as a plain
<displayname> failed with a SQLException and a 500 response, while the
memory store handled these properties without issue.
Normalise a null namespace to the empty string, which the SQL
statements and the XMLWriter output handle consistently, making the two
PropertyStore implementations agree.
---
.../catalina/servlets/DataSourcePropertyStore.java | 13 +++++----
.../catalina/servlets/TestWebdavPropertyStore.java | 33 ++++++++++++++++++++++
2 files changed, 40 insertions(+), 6 deletions(-)
diff --git a/java/org/apache/catalina/servlets/DataSourcePropertyStore.java b/java/org/apache/catalina/servlets/DataSourcePropertyStore.java
index ee900662a6..b2031a6463 100644
--- a/java/org/apache/catalina/servlets/DataSourcePropertyStore.java
+++ b/java/org/apache/catalina/servlets/DataSourcePropertyStore.java
@@ -22,6 +22,7 @@ import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.util.ArrayList;
+import java.util.Objects;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantReadWriteLock;
@@ -46,7 +47,7 @@ import org.w3c.dom.Node;
* A single properties table with four columns is used:
* <ul>
* <li>path: the resource path</li>
- * <li>namespace: the node namespace</li>
+ * <li>namespace: the node namespace, or the empty string when the property has no namespace</li>
* <li>name: the local name in the namespace</li>
* <li>node: the full serialized XML node including the name</li>
* </ul>
@@ -266,7 +267,7 @@ public class DataSourcePropertyStore implements WebdavServlet.PropertyStore {
try (Connection connection = dataSource.getConnection();
PreparedStatement statement = connection.prepareStatement(getPropertyStatement)) {
statement.setString(1, resource);
- statement.setString(2, property.getNamespaceURI());
+ statement.setString(2, Objects.requireNonNullElse(property.getNamespaceURI(), ""));
statement.setString(3, property.getLocalName());
if (statement.execute()) {
ResultSet rs = statement.getResultSet();
@@ -341,7 +342,7 @@ public class DataSourcePropertyStore implements WebdavServlet.PropertyStore {
try {
try (PreparedStatement statement = connection.prepareStatement(getPropertyStatement)) {
statement.setString(1, resource);
- statement.setString(2, node.getNamespaceURI());
+ statement.setString(2, Objects.requireNonNullElse(node.getNamespaceURI(), ""));
statement.setString(3, node.getLocalName());
if (statement.execute()) {
ResultSet rs = statement.getResultSet();
@@ -354,14 +355,14 @@ public class DataSourcePropertyStore implements WebdavServlet.PropertyStore {
try (PreparedStatement statement = connection.prepareStatement(setPropertyStatement)) {
statement.setString(1, serializedNode);
statement.setString(2, resource);
- statement.setString(3, node.getNamespaceURI());
+ statement.setString(3, Objects.requireNonNullElse(node.getNamespaceURI(), ""));
statement.setString(4, node.getLocalName());
statement.execute();
}
} else {
try (PreparedStatement statement = connection.prepareStatement(addPropertyStatement)) {
statement.setString(1, resource);
- statement.setString(2, node.getNamespaceURI());
+ statement.setString(2, Objects.requireNonNullElse(node.getNamespaceURI(), ""));
statement.setString(3, node.getLocalName());
statement.setString(4, serializedNode);
statement.execute();
@@ -377,7 +378,7 @@ public class DataSourcePropertyStore implements WebdavServlet.PropertyStore {
Node node = operation.getPropertyNode();
try (PreparedStatement statement = connection.prepareStatement(removePropertyStatement)) {
statement.setString(1, resource);
- statement.setString(2, node.getNamespaceURI());
+ statement.setString(2, Objects.requireNonNullElse(node.getNamespaceURI(), ""));
statement.setString(3, node.getLocalName());
statement.execute();
} catch (SQLException e) {
diff --git a/test/org/apache/catalina/servlets/TestWebdavPropertyStore.java b/test/org/apache/catalina/servlets/TestWebdavPropertyStore.java
index 8f1bd2d6d2..f8743f79a9 100644
--- a/test/org/apache/catalina/servlets/TestWebdavPropertyStore.java
+++ b/test/org/apache/catalina/servlets/TestWebdavPropertyStore.java
@@ -33,6 +33,8 @@ import javax.sql.DataSource;
import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;
+import jakarta.servlet.http.HttpServletResponse;
+
import org.junit.Assert;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -69,6 +71,10 @@ public class TestWebdavPropertyStore extends LoggingBaseTest {
" <V:othervalue>foooooooo</V:othervalue>\n" +
"</V:someprop>";
+ private static final String PROPERTY4 =
+ "<?xml version=\"1.0\" encoding=\"utf-8\" ?>\n" +
+ "<displayname>test value</displayname>";
+
public static final String SIMPLE_SCHEMA =
"CREATE TABLE webdavproperties (\n" +
" path VARCHAR(1024) NOT NULL,\n" +
@@ -244,6 +250,33 @@ public class TestWebdavPropertyStore extends LoggingBaseTest {
Assert.assertFalse(propertyStore.propfind("/other/path2", node1, false, xmlWriter9));
Assert.assertTrue(xmlWriter9.toString().isEmpty());
+ // Set a property without a namespace
+ Document document4 = documentBuilder.parse(new InputSource(new ByteArrayInputStream(PROPERTY4.getBytes(StandardCharsets.UTF_8))));
+ Node node4 = document4.getDocumentElement();
+
+ operations = new ArrayList<>();
+ operations.add(new ProppatchOperation(PropertyUpdateType.SET, node4));
+ propertyStore.proppatch("/some/path1", operations);
+ Assert.assertEquals(HttpServletResponse.SC_OK, operations.get(0).getStatusCode());
+
+ XMLWriter xmlWriter10 = new XMLWriter();
+ Assert.assertTrue(propertyStore.propfind("/some/path1", node4, false, xmlWriter10));
+ Assert.assertTrue(xmlWriter10.toString().contains(">test value<"));
+
+ XMLWriter xmlWriter11 = new XMLWriter();
+ Assert.assertFalse(propertyStore.propfind("/some/path1", null, true, xmlWriter11));
+ Assert.assertTrue(xmlWriter11.toString().contains("<displayname"));
+
+ // Remove the property without a namespace
+ operations = new ArrayList<>();
+ operations.add(new ProppatchOperation(PropertyUpdateType.REMOVE, node4));
+ propertyStore.proppatch("/some/path1", operations);
+ Assert.assertEquals(HttpServletResponse.SC_OK, operations.get(0).getStatusCode());
+
+ XMLWriter xmlWriter12 = new XMLWriter();
+ Assert.assertFalse(propertyStore.propfind("/some/path1", null, true, xmlWriter12));
+ Assert.assertFalse(xmlWriter12.toString().contains("<displayname"));
+
propertyStore.destroy();
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
(tomcat) 10/16: Ignore malformed Accept-Encoding q-values in DefaultServlet precompressed handling
Posted by re...@apache.org.
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 449e4ebd189334fee3d15b0becd404b9775eac53
Author: opencode <re...@apache.org>
AuthorDate: Wed Oct 7 16:48:49 2026 +0200
Ignore malformed Accept-Encoding q-values in DefaultServlet precompressed handling
Double.parseDouble() of the q-value in getBestPrecompressedResource()
was unprotected, so a client header such as Accept-Encoding: br;q=abc
triggered a NumberFormatException and the request failed with a 500
whenever a precompressed variant of the requested resource existed.
Skip a preference with an unparsable q-value instead, consistent with
treating it as not expressed.
---
.../apache/catalina/servlets/DefaultServlet.java | 7 ++-
.../catalina/servlets/TestDefaultServlet.java | 65 ++++++++++++++++++++++
2 files changed, 71 insertions(+), 1 deletion(-)
diff --git a/java/org/apache/catalina/servlets/DefaultServlet.java b/java/org/apache/catalina/servlets/DefaultServlet.java
index 6f2afb6b48..988bd894e6 100644
--- a/java/org/apache/catalina/servlets/DefaultServlet.java
+++ b/java/org/apache/catalina/servlets/DefaultServlet.java
@@ -1487,7 +1487,12 @@ public class DefaultServlet extends HttpServlet {
if (equalsIdx == -1) {
continue;
}
- quality = Double.parseDouble(preference.substring(equalsIdx + 1).trim());
+ try {
+ quality = Double.parseDouble(preference.substring(equalsIdx + 1).trim());
+ } catch (NumberFormatException nfe) {
+ // Invalid q-value: ignore this preference
+ continue;
+ }
}
if (quality >= bestResourceQuality) {
String encoding = preference;
diff --git a/test/org/apache/catalina/servlets/TestDefaultServlet.java b/test/org/apache/catalina/servlets/TestDefaultServlet.java
index 13a4dd09ce..ca7ef94873 100644
--- a/test/org/apache/catalina/servlets/TestDefaultServlet.java
+++ b/test/org/apache/catalina/servlets/TestDefaultServlet.java
@@ -381,6 +381,71 @@ public class TestDefaultServlet extends TomcatBaseTest {
Assert.assertTrue(responseHeaders.contains("vary: accept-encoding"));
}
+ /*
+ * Verify that a malformed q-value in Accept-Encoding does not fail the
+ * request and only causes the affected preference to be ignored.
+ */
+ @Test
+ public void testMalformedPrecompressedQValue() throws Exception {
+
+ Tomcat tomcat = getTomcatInstance();
+
+ File appDir = new File("test/webapp");
+
+ long gzSize = new File(appDir, "index.html.gz").length();
+ long indexSize = new File(appDir, "index.html").length();
+
+ // app dir is relative to server home
+ Context ctxt = tomcat.addContext("", appDir.getAbsolutePath());
+ Wrapper defaultServlet = Tomcat.addServlet(ctxt, "default",
+ DefaultServlet.class.getName());
+ defaultServlet.addInitParameter("precompressed", "br=.br,gzip=.gz");
+ defaultServlet.addInitParameter("fileEncoding", "ISO-8859-1");
+
+ ctxt.addServletMapping("/", "default");
+ ctxt.addMimeMapping("html", "text/html");
+
+ tomcat.start();
+
+ TestCompressedClient client = new TestCompressedClient(getPort());
+
+ client.reset();
+ // @formatter:off
+ client.setRequest(new String[] {
+ "GET /index.html HTTP/1.1" + CRLF +
+ "Host: localhost" + CRLF +
+ "Connection: Close" + CRLF +
+ "Accept-Encoding: br;q=abc" + CRLF +
+ CRLF
+ });
+ // @formatter:on
+ client.connect();
+ client.processRequest();
+ Assert.assertTrue(client.isResponse200());
+ List<String> responseHeaders = client.getResponseHeaders();
+ Assert.assertFalse(responseHeaders.contains("Content-Encoding"));
+ Assert.assertTrue(responseHeaders.contains("Content-Length: " + indexSize));
+ Assert.assertTrue(responseHeaders.contains("vary: accept-encoding"));
+
+ client.reset();
+ // @formatter:off
+ client.setRequest(new String[] {
+ "GET /index.html HTTP/1.1" + CRLF +
+ "Host: localhost" + CRLF +
+ "Connection: Close" + CRLF +
+ "Accept-Encoding: br;q=abc,gzip" + CRLF +
+ CRLF
+ });
+ // @formatter:on
+ client.connect();
+ client.processRequest();
+ Assert.assertTrue(client.isResponse200());
+ responseHeaders = client.getResponseHeaders();
+ Assert.assertTrue(responseHeaders.contains("Content-Encoding: gzip"));
+ Assert.assertTrue(responseHeaders.contains("Content-Length: " + gzSize));
+ Assert.assertTrue(responseHeaders.contains("vary: accept-encoding"));
+ }
+
/*
* Verify preferring of brotli in default configuration for actual Firefox and Chrome requests.
*/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
(tomcat) 14/16: Discard invalid persisted sessions on load in StandardManager
Posted by re...@apache.org.
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 f892bf50997c83f72bba662007f00e5cf313cf84
Author: opencode <re...@apache.org>
AuthorDate: Wed Oct 7 17:48:58 2026 +0200
Discard invalid persisted sessions on load in StandardManager
Sessions persisted as invalid - possible when a session expires while
unload() is writing the session file - were previously inserted into
the manager map, activated and then expired again. That emitted a
sessionDestroyed event (and attribute valueUnbound notifications) for
a session that never fired sessionCreated in this JVM, fired
sessionDidActivate for a dead session and skewed the expired session
statistics. The earlier expiration only existed to remove the map
entry that the load loop itself had just added.
Skip sessions that are invalid after deserialization instead, without
adding them to the map.
---
.../apache/catalina/session/StandardManager.java | 16 ++-
.../catalina/session/TestStandardManager.java | 144 +++++++++++++++++++++
2 files changed, 154 insertions(+), 6 deletions(-)
diff --git a/java/org/apache/catalina/session/StandardManager.java b/java/org/apache/catalina/session/StandardManager.java
index f9b23bfa3d..251a9d602b 100644
--- a/java/org/apache/catalina/session/StandardManager.java
+++ b/java/org/apache/catalina/session/StandardManager.java
@@ -154,14 +154,18 @@ public class StandardManager extends ManagerBase {
StandardSession session = getNewSession();
session.readObjectData(ois);
session.setManager(this);
- sessions.put(session.getIdInternal(), session);
- session.activate();
if (!session.isValidInternal()) {
- // If session is already invalid,
- // expire session to prevent memory leak.
- session.setValid(true);
- session.expire();
+ // The session was already invalid when it was
+ // persisted, typically because it expired while
+ // the sessions were being written. It has already
+ // left the map of the writing JVM and the
+ // associated events have already been fired.
+ // Discard it without activating or expiring it
+ // again.
+ continue;
}
+ sessions.put(session.getIdInternal(), session);
+ session.activate();
}
} finally {
// Delete the persistent storage file in all cases, since retrying after an exception
diff --git a/test/org/apache/catalina/session/TestStandardManager.java b/test/org/apache/catalina/session/TestStandardManager.java
new file mode 100644
index 0000000000..2a9edeba26
--- /dev/null
+++ b/test/org/apache/catalina/session/TestStandardManager.java
@@ -0,0 +1,144 @@
+/*
+ * 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.session;
+
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.ObjectOutputStream;
+import java.io.Serializable;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import jakarta.servlet.http.HttpSessionActivationListener;
+import jakarta.servlet.http.HttpSessionEvent;
+import jakarta.servlet.http.HttpSessionListener;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import org.apache.catalina.startup.ExpandWar;
+import org.apache.tomcat.unittest.TesterContext;
+import org.apache.tomcat.unittest.TesterHost;
+import org.apache.tomcat.unittest.TesterServletContext;
+
+public class TestStandardManager {
+
+ private static final String VALID_ID = "validSessionId";
+ private static final String INVALID_ID = "invalidSessionId";
+
+ public static class TesterActivationAttribute implements HttpSessionActivationListener, Serializable {
+
+ private static final long serialVersionUID = 1L;
+
+ private boolean activated;
+
+ @Override
+ public void sessionDidActivate(HttpSessionEvent se) {
+ activated = true;
+ }
+
+ @Override
+ public void sessionWillPassivate(HttpSessionEvent se) {
+ // NO-OP
+ }
+
+ public boolean wasActivated() {
+ return activated;
+ }
+ }
+
+ /*
+ * A session that was persisted as invalid (which can happen when a
+ * session expires concurrently with unload()) must be discarded on load
+ * without being activated and expired again, since that would emit a
+ * sessionDestroyed event for a session that never emitted sessionCreated
+ * in this JVM.
+ */
+ @Test
+ public void testLoadDiscardsInvalidPersistedSession() throws Exception {
+ File dir = new File("SESS_TEMP_STANDARD_LOAD");
+ ExpandWar.delete(dir);
+ Assert.assertTrue(dir.mkdirs());
+ File file = new File(dir, "TEST_SESSIONS.ser");
+ try {
+ StandardManager manager = new StandardManager();
+
+ AtomicInteger created = new AtomicInteger();
+ AtomicInteger destroyed = new AtomicInteger();
+ HttpSessionListener listener = new HttpSessionListener() {
+ @Override
+ public void sessionCreated(HttpSessionEvent se) {
+ created.incrementAndGet();
+ }
+
+ @Override
+ public void sessionDestroyed(HttpSessionEvent se) {
+ destroyed.incrementAndGet();
+ }
+ };
+
+ TesterContext context = new TesterContext() {
+ @Override
+ public Object[] getApplicationLifecycleListeners() {
+ return new Object[] { listener };
+ }
+ };
+ context.setServletContext(new TesterServletContext());
+ context.setParent(new TesterHost());
+ manager.setContext(context);
+
+ manager.setPathname(file.getAbsolutePath());
+ manager.start();
+
+ StandardSession valid = (StandardSession) manager.createSession(VALID_ID);
+ TesterActivationAttribute activation = new TesterActivationAttribute();
+ valid.setAttribute("activation", activation);
+
+ // A session that was already invalid when it was persisted
+ StandardSession invalid = manager.getNewSession();
+ invalid.setManager(manager);
+ invalid.id = INVALID_ID;
+
+ try (ObjectOutputStream oos = new ObjectOutputStream(new FileOutputStream(file))) {
+ oos.writeObject(Integer.valueOf(2));
+ valid.writeObjectData(oos);
+ invalid.writeObjectData(oos);
+ }
+
+ // Session creation already reported the session created during
+ // setup above. Reset the counters so only events emitted by load()
+ // are observed.
+ created.set(0);
+ destroyed.set(0);
+
+ manager.load();
+
+ // Before the fix, the invalid session was activated and then
+ // expired, emitting an unbalanced sessionDestroyed event
+ Assert.assertEquals(0, destroyed.get());
+ Assert.assertEquals(0, created.get());
+ Assert.assertNull(manager.findSession(INVALID_ID));
+
+ // Control: the valid session is loaded and activated as usual
+ Assert.assertNotNull(manager.findSession(VALID_ID));
+ TesterActivationAttribute loadedActivation = (TesterActivationAttribute) ((StandardSession) manager
+ .findSession(VALID_ID)).getAttribute("activation");
+ Assert.assertTrue(loadedActivation.wasActivated());
+ } finally {
+ ExpandWar.delete(dir);
+ }
+ }
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
(tomcat) 05/16: Make JNDIRealm URL failover decision per connection attempt
Posted by re...@apache.org.
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 21c7717d6ec7b56fa667916eee05278155e5f6d2
Author: opencode <re...@apache.org>
AuthorDate: Wed Oct 7 15:52:15 2026 +0200
Make JNDIRealm URL failover decision per connection attempt
open() signalled the failover retry by setting the realm-wide,
non-volatile connectionAttempt field, which
getDirectoryContextEnvironment() read to select between connectionURL
and alternateURL. With connection pooling enabled, open() runs
concurrently on different connections without any realm-level
serialization: while one thread was failing over, another thread
building its first-attempt environment could read the flag and connect
to the alternate URL, and the reset in the finally block could send a
concurrent thread's failover retry back to the primary that had just
failed. Visibility of the plain int was not guaranteed either.
The flag's meaningful lifetime is a single open() call, so remove the
shared field entirely and pass an explicit useAlternateURL parameter to
getDirectoryContextEnvironment(). This removes a protected field and
changes a protected method signature. Neither was referenced outside
JNDIRealm. Include a changelog entry.
---
java/org/apache/catalina/realm/JNDIRealm.java | 26 +++++++++-----------------
webapps/docs/changelog.xml | 9 +++++++++
2 files changed, 18 insertions(+), 17 deletions(-)
diff --git a/java/org/apache/catalina/realm/JNDIRealm.java b/java/org/apache/catalina/realm/JNDIRealm.java
index 091ede4816..4c8601488b 100644
--- a/java/org/apache/catalina/realm/JNDIRealm.java
+++ b/java/org/apache/catalina/realm/JNDIRealm.java
@@ -296,11 +296,6 @@ public class JNDIRealm extends RealmBase {
*/
protected String alternateURL;
- /**
- * The number of connection attempts. If greater than zero we use the alternate url.
- */
- protected int connectionAttempt = 0;
-
/**
* Add this role to every authenticated user
*/
@@ -2694,21 +2689,16 @@ public class JNDIRealm extends RealmBase {
protected void open(JNDIConnection connection) throws NamingException {
try {
// Ensure that we have a directory context available
- connection.context = createDirContext(connection, getDirectoryContextEnvironment());
+ connection.context = createDirContext(connection, getDirectoryContextEnvironment(false));
} catch (Exception e) {
if (alternateURL == null || alternateURL.isEmpty()) {
// No alternate URL. Re-throw the exception.
throw e;
}
- connectionAttempt = 1;
// log the first exception.
containerLog.info(sm.getString("jndiRealm.exception.retry"), e);
// Try connecting to the alternate url.
- connection.context = createDirContext(connection, getDirectoryContextEnvironment());
- } finally {
- // reset it in case the connection times out.
- // the primary may come back.
- connectionAttempt = 0;
+ connection.context = createDirContext(connection, getDirectoryContextEnvironment(true));
}
}
@@ -2830,16 +2820,18 @@ public class JNDIRealm extends RealmBase {
/**
* Create our directory context configuration.
*
+ * @param useAlternateURL Whether the alternate URL should be used in place of the connection URL
+ *
* @return java.util.Hashtable the configuration for the directory context.
*/
- protected Hashtable<String,String> getDirectoryContextEnvironment() {
+ protected Hashtable<String,String> getDirectoryContextEnvironment(boolean useAlternateURL) {
Hashtable<String,String> env = new Hashtable<>();
// Configure our directory context environment.
- if (containerLog.isTraceEnabled() && connectionAttempt == 0) {
+ if (containerLog.isTraceEnabled() && !useAlternateURL) {
containerLog.trace("Connecting to URL " + connectionURL);
- } else if (containerLog.isTraceEnabled() && connectionAttempt > 0) {
+ } else if (containerLog.isTraceEnabled() && useAlternateURL) {
containerLog.trace("Connecting to URL " + alternateURL);
}
env.put(Context.INITIAL_CONTEXT_FACTORY, contextFactory);
@@ -2849,9 +2841,9 @@ public class JNDIRealm extends RealmBase {
if (connectionPassword != null) {
env.put(Context.SECURITY_CREDENTIALS, connectionPassword);
}
- if (connectionURL != null && connectionAttempt == 0) {
+ if (connectionURL != null && !useAlternateURL) {
env.put(Context.PROVIDER_URL, connectionURL);
- } else if (alternateURL != null && connectionAttempt > 0) {
+ } else if (alternateURL != null && useAlternateURL) {
env.put(Context.PROVIDER_URL, alternateURL);
}
if (authentication != null) {
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index d115e5e3bd..0fcc249322 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -265,6 +265,15 @@
no longer terminates the TLS session of another, still active pooled
connection. (remm)
</fix>
+ <fix>
+ Select the primary or alternate directory server URL for each
+ <code>JNDIRealm</code> connection attempt individually rather than via
+ a realm-wide mutable flag that concurrent pooled connections raced on,
+ which could transiently cause a connection to use the wrong URL. Note
+ that this removes the <code>connectionAttempt</code> protected field
+ and changes the signature of the protected
+ <code>getDirectoryContextEnvironment()</code> method. (remm)
+ </fix>
</changelog>
</subsection>
<subsection name="Coyote">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org