ashibhardwaj commented on code in PR #18259:
URL: https://github.com/apache/druid/pull/18259#discussion_r2304260791
##########
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:
The current code already logs a warning when the connection isn't HTTPS, so
operators are alerted to the security risk.
This prevents accidental exposure in production environments where proxy
configurations might not set the secure flag correctly, and it encourages
better security practices throughout the development lifecycle.
Authentication systems should be secure by default rather than convenient by
default.
--
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]
