On Wed, Oct 14, 2026 at 12:31 PM Mark Thomas <[email protected]> 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, [email protected] 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 <[email protected]> > > 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: [email protected] > > For additional commands, e-mail: [email protected] > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
