Copilot commented on code in PR #2718:
URL: https://github.com/apache/shiro/pull/2718#discussion_r3301176660
##########
web/src/main/java/org/apache/shiro/web/servlet/AbstractShiroFilter.java:
##########
@@ -375,6 +388,7 @@ protected void doFilterInternal(ServletRequest
servletRequest, ServletResponse s
subject.execute((Callable<Void>) () -> {
updateSessionLastAccessTime(request, response);
executeChain(request, response, chain);
+ incrementSessionVersion();
Review Comment:
`incrementSessionVersion()` is invoked after `executeChain(...)` inside the
same callable. If `executeChain` throws, the version won't be incremented even
though `updateSessionLastAccessTime` (and potentially other session mutations)
may already have occurred, leaving the session updated without a corresponding
version bump. If the version is meant to represent a request-level update
boundary, consider ensuring it runs in a `finally` block around `executeChain`.
##########
core/src/main/java/org/apache/shiro/session/mgt/SimpleSession.java:
##########
@@ -60,7 +64,7 @@ public class SimpleSession implements ValidatingSession,
Serializable {
// changes do not require a change to this number. If you need to generate
// a new number in this case, use the JDK's 'serialver' program to
generate it.
@Serial
- private static final long serialVersionUID = -7125642695178165650L;
+ private static final long serialVersionUID = -7125642695178165651L;
Review Comment:
The `serialVersionUID` was changed even though this class already uses
custom `writeObject/readObject` with a bitmask specifically to preserve
serialization compatibility across field changes. Bumping the UID will cause
`InvalidClassException` when deserializing existing serialized sessions (e.g.,
in distributed caches), which is a significant backward-compat break. If the
intent is to remain compatible, keep the prior UID and handle missing/new
fields in `readObject`; only bump the UID if breaking deserialization is
explicitly intended and documented.
##########
core/src/main/java/org/apache/shiro/session/mgt/SimpleSessionFactory.java:
##########
@@ -36,10 +36,7 @@ public class SimpleSessionFactory implements SessionFactory {
*/
public Session createSession(SessionContext initData) {
if (initData != null) {
- String host = initData.getHost();
- if (host != null) {
- return new SimpleSession(host);
- }
+ return new SimpleSession(initData.getHost(),
initData.isVersioned());
}
return new SimpleSession();
}
Review Comment:
`SimpleSessionFactory` only creates a versioned `SimpleSession` when a
non-null `SessionContext` is provided. However, `SessionManager.start(null)` is
used in multiple tests and is part of the public API; if versioning is enabled
at the `SessionManager` level, starting with a null context will silently
create a non-versioned session and bypass the stale-write protection. Consider
ensuring a default `SessionContext` (with versioning flag set when appropriate)
is used when `initData` is null, or otherwise ensuring sessions are versioned
whenever `SessionManager.isVersioned()` is true.
##########
core/src/main/java/org/apache/shiro/session/mgt/SimpleSession.java:
##########
@@ -433,6 +472,10 @@ public String toString() {
return sb.toString();
}
+ void setStartTimestamp(Date startTimestamp) {
Review Comment:
`SimpleSession#setStartTimestamp` was changed from a public setter to
package-private. This is an API-breaking change for any downstream users that
construct/adjust `SimpleSession` instances directly (similar for the removed
`setHost`/`setStopTimestamp` setters). If this restriction is intentional for
thread-safety, consider documenting the change and/or providing an alternative
supported way to set these values (or keep the setters and enforce
thread-safety another way).
##########
core/src/main/java/org/apache/shiro/session/mgt/eis/CachingSessionDAO.java:
##########
@@ -244,7 +246,21 @@ protected void cache(Session session, Serializable
sessionId) {
* @param cache the cache to store the session
*/
protected void cache(Session session, Serializable sessionId,
Cache<Serializable, Session> cache) {
- cache.put(sessionId, session);
+ if (session instanceof VersionedSession versionedSession &&
versionedSession.isVersioned()) {
+ var previous = (VersionedSession)
cache.get(versionedSession.getId());
+ if (previous == null || previous.getVersion() <=
versionedSession.getVersion()) {
Review Comment:
`cache.get(...)` is cast to `VersionedSession` and then
`previous.getVersion()` is called without verifying the cached value is
versioned. If the cache already contains a non-versioned `VersionedSession`
instance (e.g., `SimpleSession` with `isVersioned()==false`), `getVersion()`
will throw due to the internal null version. Consider checking `previous
instanceof VersionedSession && previous.isVersioned()` before calling
`getVersion()`, and fall back to unconditional `put` (or treat previous version
as 0) when the cached session isn't versioned.
##########
web/src/main/java/org/apache/shiro/web/servlet/AbstractShiroFilter.java:
##########
@@ -336,6 +338,17 @@ protected void updateSessionLastAccessTime(ServletRequest
request, ServletRespon
}
}
+ protected void incrementSessionVersion() {
+ if (!isHttpSessions()) {
+ var session = SecurityUtils.getSubject().getSession(false);
+ if (session != null) {
+ NativeSessionManager sm = (NativeSessionManager) SecurityUtils
+
.getSecurityManager(DefaultWebSecurityManager.class).getSessionManager();
+ sm.incrementVersion(new DefaultSessionKey(session.getId()));
Review Comment:
`incrementSessionVersion()` unconditionally casts `getSessionManager()` to
`NativeSessionManager`. If a custom `WebSecurityManager` is configured with
non-HTTP session mode but a non-`NativeSessionManager` implementation, this
will throw `ClassCastException` and fail the request after the chain runs.
Prefer an `instanceof NativeSessionManager` guard (or query capability via a
method) before calling `incrementVersion`.
##########
core/src/main/java/org/apache/shiro/session/mgt/eis/CachingSessionDAO.java:
##########
@@ -244,7 +246,21 @@ protected void cache(Session session, Serializable
sessionId) {
* @param cache the cache to store the session
*/
protected void cache(Session session, Serializable sessionId,
Cache<Serializable, Session> cache) {
- cache.put(sessionId, session);
+ if (session instanceof VersionedSession versionedSession &&
versionedSession.isVersioned()) {
+ var previous = (VersionedSession)
cache.get(versionedSession.getId());
Review Comment:
This uses `cache.get(versionedSession.getId())` even though the cache key
for this method is the `sessionId` parameter. If these ever differ (e.g.,
caller passes a different key type/representation), the version comparison will
be against the wrong entry and the stale-write protection won't work. Use the
`sessionId` argument consistently when reading from the cache.
##########
core/src/main/java/org/apache/shiro/session/mgt/AbstractNativeSessionManager.java:
##########
@@ -310,4 +310,13 @@ public void checkValid(SessionKey key) throws
InvalidSessionException {
protected void onChange(Session s) {
}
+
+ @Override
+ public long incrementVersion(SessionKey key) {
+ Session session = lookupRequiredSession(key);
+ if (session instanceof VersionedSession versionedSession &&
versionedSession.isVersioned()) {
+ return versionedSession.incrementVersion();
Review Comment:
`incrementVersion` mutates the underlying `Session` but does not call
`onChange(session)`. For the default `DefaultSessionManager` implementation,
`onChange` is what persists updates via `SessionDAO.update`, so version
increments will not be written back to the DAO/cache and won't provide ordering
protection across requests/JVMs. Consider invoking `onChange(session)` after
incrementing (and ensure it only happens when versioning is enabled).
##########
core/src/main/java/org/apache/shiro/session/mgt/SimpleSession.java:
##########
@@ -491,22 +552,33 @@ private void readObject(ObjectInputStream in) throws
IOException, ClassNotFoundE
this.startTimestamp = (Date) in.readObject();
}
if (isFieldPresent(bitMask, STOP_TIMESTAMP_BIT_MASK)) {
- this.stopTimestamp = (Date) in.readObject();
+ this.stopTimestamp = new AtomicReference<>((Date) in.readObject());
+ } else {
+ this.stopTimestamp = new AtomicReference<>();
}
if (isFieldPresent(bitMask, LAST_ACCESS_TIME_BIT_MASK)) {
- this.lastAccessTime = (Date) in.readObject();
+ this.lastAccessTime = new AtomicReference<>((Date)
in.readObject());
+ } else {
+ this.lastAccessTime = new AtomicReference<>();
}
if (isFieldPresent(bitMask, TIMEOUT_BIT_MASK)) {
- this.timeout = in.readLong();
+ this.timeout = new AtomicLong(in.readLong());
+ } else {
+ this.timeout = new AtomicLong();
}
if (isFieldPresent(bitMask, EXPIRED_BIT_MASK)) {
- this.expired = in.readBoolean();
+ this.expired = new AtomicBoolean(in.readBoolean());
+ } else {
+ this.expired = new AtomicBoolean();
}
if (isFieldPresent(bitMask, HOST_BIT_MASK)) {
this.host = in.readUTF();
}
if (isFieldPresent(bitMask, ATTRIBUTES_BIT_MASK)) {
- this.attributes = (Map<Object, Object>) in.readObject();
+ this.attributes = (ConcurrentHashMap<Object, Object>)
in.readObject();
+ }
+ if (isFieldPresent(bitMask, VERSION_BIT_MASK)) {
+ this.version = new AtomicLong(in.readLong());
}
Review Comment:
`readObject` casts the deserialized attributes map to `ConcurrentHashMap`.
If older serialized sessions (or user code) stored a different `Map`
implementation (e.g., `HashMap`), this will throw `ClassCastException` during
session deserialization. Prefer deserializing to `Map` and then
copying/wrapping into a `ConcurrentHashMap` (similar to `setAttributes`) to
preserve compatibility.
##########
core/src/main/java/org/apache/shiro/session/mgt/NativeSessionManager.java:
##########
@@ -178,4 +178,5 @@ public interface NativeSessionManager extends
SessionManager {
*/
Object removeAttribute(SessionKey sessionKey, Object attributeKey) throws
InvalidSessionException;
Review Comment:
`incrementVersion(...)` was added to `NativeSessionManager` without JavaDoc.
Since this is a public API and affects session consistency semantics, it should
document when it should be called (per mutation vs per request), whether it
persists the change, and what the return value represents when versioning is
disabled.
##########
core/src/main/java/org/apache/shiro/session/mgt/SessionManager.java:
##########
@@ -58,4 +58,6 @@ public interface SessionManager {
* @since 1.0
*/
Session getSession(SessionKey key) throws SessionException;
+
Review Comment:
`SessionManager` is a public API and its methods are documented, but
`isVersioned()` was added without any JavaDoc/@since. Please document what
“versioned” means in this context (e.g., whether it controls session creation,
cache ordering semantics, or persistence behavior) and what default
implementations should return.
##########
core/src/main/java/org/apache/shiro/session/mgt/SimpleSession.java:
##########
@@ -144,19 +160,32 @@ public void setStartTimestamp(Date startTimestamp) {
* active.
*/
public Date getStopTimestamp() {
- return stopTimestamp;
- }
-
- public void setStopTimestamp(Date stopTimestamp) {
- this.stopTimestamp = stopTimestamp;
+ return stopTimestamp.get();
}
+ @Override
public Date getLastAccessTime() {
- return lastAccessTime;
+ return lastAccessTime.get();
}
public void setLastAccessTime(Date lastAccessTime) {
- this.lastAccessTime = lastAccessTime;
+ this.lastAccessTime.set(lastAccessTime);
+ }
+
+ @Override
+ public long getVersion() {
+ Objects.requireNonNull(version, "versioned session is required");
+ return version.get();
+ }
+
+ @Override
+ public boolean isVersioned() {
+ return version != null;
+ }
+
+ public long incrementVersion() {
+ Objects.requireNonNull(version, "versioned session is required");
+ return version.incrementAndGet();
}
Review Comment:
`incrementVersion()` implements `VersionedSession.incrementVersion()` but is
missing an `@Override` annotation, unlike the other `VersionedSession` methods
in this class. Adding `@Override` helps catch signature drift and is consistent
with the rest of the implementation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
