Copilot commented on code in PR #18259:
URL: https://github.com/apache/druid/pull/18259#discussion_r2303876857
##########
extensions-core/druid-pac4j/src/test/java/org/apache/druid/security/pac4j/Pac4jSessionStoreTest.java:
##########
@@ -56,20 +55,145 @@ public void testSetAndGet()
Cookie cookie = cookieCapture.getValue();
Assert.assertTrue(cookie.isSecure());
Review Comment:
Duplicate assertion removed. The line
`Assert.assertTrue(cookie.isSecure());` was correctly removed as it was testing
the same condition twice.
##########
extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jSessionStore.java:
##########
@@ -96,37 +102,103 @@ public void set(WebContext context, String key, @Nullable
Object value)
Object profile = value;
Cookie cookie;
- if (value == null) {
- cookie = new Cookie(PAC4J_SESSION_PREFIX + key, null);
+ // Check if value is null, empty string, or empty collection
+ boolean isEmpty = value == null ||
+ (value instanceof String && ((String) value).isEmpty()) ||
+ (value instanceof java.util.Collection &&
((java.util.Collection<?>) value).isEmpty()) ||
+ (value instanceof java.util.Map && ((java.util.Map<?, ?>)
value).isEmpty());
+
+ if (isEmpty) {
+ cookie = new Cookie(PAC4J_SESSION_PREFIX + key, "");
+ cookie.setMaxAge(0);
} else {
- if (key.contentEquals(Pac4jConstants.USER_PROFILES)) {
+ if (Pac4jConstants.USER_PROFILES.equals(key)) {
/* trim the profile object */
profile = clearUserProfile(value);
}
LOGGER.debug("Save in session: [%s] = [%s]", key, profile);
- cookie = new Cookie(
- PAC4J_SESSION_PREFIX + key,
- compressEncryptBase64(profile)
- );
+
+ String encryptedValue = compressEncryptBase64(profile);
+ cookie = new Cookie(PAC4J_SESSION_PREFIX + key, encryptedValue);
+ cookie.setMaxAge(900); // 15 minutes
}
- cookie.setDomain("");
cookie.setHttpOnly(true);
- cookie.setSecure(ContextHelper.isHttpsOrSecure(context));
+ // Always set secure flag for authentication cookies to prevent
transmission over HTTP
+ // This ensures the cookie is only sent over HTTPS connections
+ boolean isSecure = isHttpsOrSecure(context);
+ if (!isSecure) {
+ LOGGER.warn("Setting authentication cookie over non-HTTPS connection.
This is not recommended for production.");
+ }
+ cookie.setSecure(true); // Always set secure flag for authentication
cookies
Review Comment:
Hardcoding `setSecure(true)` regardless of the connection scheme could
prevent authentication from working in development environments using HTTP.
Consider making this configurable or at least logging a warning when forcing
secure cookies over HTTP connections.
```suggestion
cookie.setSecure(isSecure); // Set secure flag only if connection is
secure
```
##########
extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/Pac4jFilter.java:
##########
@@ -85,38 +80,59 @@ public void doFilter(ServletRequest servletRequest,
ServletResponse servletRespo
return;
}
- HttpServletRequest httpServletRequest = (HttpServletRequest)
servletRequest;
- HttpServletResponse httpServletResponse = (HttpServletResponse)
servletResponse;
- JEEContext context = new JEEContext(httpServletRequest,
httpServletResponse, sessionStore);
+ HttpServletRequest request = (HttpServletRequest) servletRequest;
+ HttpServletResponse response = (HttpServletResponse) servletResponse;
+ JEEContext context = new JEEContext(request, response);
+
+ if (request.getRequestURI().equals(callbackPath)) {
Review Comment:
Using `equals()` for URI comparison could be vulnerable to path traversal
attacks. Consider using a more robust URI matching approach that normalizes the
path before comparison.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
