gnodet-bot commented on code in PR #28512:
URL: https://github.com/apache/camel/pull/28512#discussion_r4376274894
##########
components/camel-oauth/src/main/java/org/apache/camel/oauth/OAuth.java:
##########
@@ -98,4 +98,8 @@ public OAuthSession getOrCreateSession(Exchange exchange) {
public OAuthSession createSession(Exchange exchange) {
return getSessionStore().createSession(exchange);
}
+
+ public OAuthSession rotateSession(Exchange exchange, OAuthSession session)
{
Review Comment:
📝 **Missing Javadoc:** The new `rotateSession()` public method lacks
Javadoc. The interface method in `OAuthSessionStore` has full documentation —
this delegating method in `OAuth` should carry at minimum `/** {@inheritDoc}
*/`.
##########
components/camel-oauth/src/main/java/org/apache/camel/oauth/InMemorySession.java:
##########
@@ -34,6 +34,16 @@ public InMemorySession(String sessionId) {
this.sessionId = sessionId;
}
+ public Map<String, Object> getValues() {
+ return new HashMap<>(values);
+ }
+
+ public void putAll(Map<String, Object> newValues) {
Review Comment:
💡 **Encapsulation:** `getValues()` and `putAll()` are only called from
`InMemorySessionStore.rotateSession()` in the same package. Making them
package-private reduces the public API surface of `InMemorySession` and
prevents unintended external access to session internals:
```suggestion
void putAll(Map<String, Object> newValues) {
```
Same for `getValues()` at line 37 — remove the `public` modifier there as
well.
##########
components/camel-oauth/src/main/java/org/apache/camel/oauth/InMemorySessionStore.java:
##########
@@ -62,21 +62,46 @@ public Optional<OAuthSession> getSession(Exchange exchange)
{
return maybeSession;
}
+ @Override
public OAuthSession createSession(Exchange exchange) {
var sessionId = UUID.randomUUID().toString();
var session = new InMemorySession(sessionId);
sessions.put(sessionId, session);
var msg = exchange.getMessage();
- log.info("New OAuthSession: {}", sessionId);
+ log.debug("New OAuthSession: {}", sessionId);
setSessionCookie(msg, session);
msg.setHeader(OAuth.CAMEL_OAUTH_SESSION_ID, sessionId);
return session;
}
+ @Override
+ public OAuthSession rotateSession(Exchange exchange, OAuthSession session)
{
+ if (session == null) {
+ return createSession(exchange);
+ }
+ var oldSessionId = session.getSessionId();
+ sessions.remove(oldSessionId);
+
+ var newSessionId = UUID.randomUUID().toString();
+ var newSession = new InMemorySession(newSessionId);
+ if (session instanceof InMemorySession inMemorySession) {
+ newSession.putAll(inMemorySession.getValues());
+ }
+ sessions.put(newSessionId, newSession);
+
+ var msg = exchange.getMessage();
+ log.debug("Rotated OAuthSession from {} to {}", oldSessionId,
newSessionId);
Review Comment:
🔒 **Security — session identifiers in logs:** The PR title states the goal
of not logging session identifiers, but this debug line logs both the old and
new session IDs. Even at `DEBUG` level, this creates a credential-exposure risk
in environments where debug logs are collected (log aggregators, SIEM).
Consider logging only a non-sensitive signal:
```suggestion
log.debug("Rotated OAuthSession");
```
##########
components/camel-oauth/src/main/java/org/apache/camel/oauth/OAuthSessionStore.java:
##########
@@ -25,4 +25,16 @@ public interface OAuthSessionStore {
Optional<OAuthSession> getSession(Exchange exchange);
OAuthSession createSession(Exchange exchange);
+
+ /**
+ * Rotates the session, replacing the current session identifier with a
newly generated one and transferring any
+ * session state.
+ *
+ * @param exchange the current exchange
+ * @param session the existing session to rotate
+ * @return the rotated session with a new session identifier
+ */
+ default OAuthSession rotateSession(Exchange exchange, OAuthSession
session) {
Review Comment:
🔒 **Security — session fixation risk for custom implementations:** The
default no-op silently returns the existing session unchanged, meaning any
custom `OAuthSessionStore` that doesn't override this method will be silently
vulnerable to session fixation. The existing Javadoc doesn't warn about this.
Consider adding an `@implNote` to the Javadoc above:
```
* @implNote The default implementation is a no-op that returns the existing
session unchanged.
* Implementors MUST override this method to prevent session
fixation — failing to do so
* leaves the session identifier unchanged across privilege
escalation (login).
```
--
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]
