Re: [PR] Require authentication for GraphQL WebSocket upgrade (unomi-3.0.x) [unomi]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #844:
URL: https://github.com/apache/unomi/pull/844#discussion_r3935504183


##########
graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/servlet/auth/GraphQLServletSecurityValidator.java:
##########
@@ -109,13 +110,55 @@ private boolean isPublicOperation(String query) {
         return true;
     }
 
+    /**
+     * Authenticates a WebSocket upgrade that carries an {@code Authorization} header. Subscriptions are
+     * never public, so only Basic JAAS credentials are accepted.
+     *
+     * @return true when the caller is authenticated; false after a 401 has been sent
+     */
+    public boolean validateWebSocketUpgrade(HttpServletRequest req, HttpServletResponse res) throws IOException {
+        if (req.getHeader("Authorization") == null) {
+            res.addHeader("WWW-Authenticate", "Basic realm=\"karaf\"");
+            res.sendError(HttpServletResponse.SC_UNAUTHORIZED);
+            return false;
+        }
+        if (isAuthenticatedUser(req)) {
+            return true;
+        }
+        res.sendError(HttpServletResponse.SC_UNAUTHORIZED);

Review Comment:
   A 401 response must include a `WWW-Authenticate` challenge, but invalid or malformed credentials reach this branch without one. Add the same Basic challenge used by the missing-header branch so upgrade clients receive a standards-compliant response.



##########
graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/servlet/websocket/SubscriptionWebSocket.java:
##########
@@ -82,9 +169,85 @@ public void onWebSocketText(String textMessage) {
         }
     }
 
+    /**
+     * Authenticates the socket from the {@code connection_init} payload, which is how a browser client
+     * presents credentials (it cannot set request headers on the handshake). A socket that already
+     * authenticated on the upgrade is left as it is.
+     *
+     * @return true when the socket may proceed, false when it has been closed
+     */
+    private boolean handleConnectionInit(GraphQLMessage message) {
+        if (authenticated) {
+            return true;
+        }
+        if (isDeadlineExpired()) {
+            return refuseAfterDeadline(message);
+        }
+
+        final String credential = basicCredentialFrom(message.getPayload());
+        if (credential == null || validator == null || !validator.authenticateBasicCredential(credential)) {
+            LOGGER.warn("Refusing GraphQL WebSocket connection_init without a valid credential");
+            sendMessage(GraphQLMessage.create(message.getId())
+                    .type(GraphQLMessage.TYPE_CONNECTION_ERROR)
+                    .errors(Collections.singletonList("Not authenticated"))
+                    .build());
+            closeConnection(message, CLOSE_POLICY_VIOLATION, "Not authenticated");
+            return false;
+        }
+
+        synchronized (authenticationLock) {
+            // The deadline may have fired while the credential was being checked.
+            if (deadlineExpired) {
+                return refuseAfterDeadline(message);
+            }
+            this.authenticated = true;
+        }
+        cancelAuthenticationDeadline();
+        final Session session = getSession();
+        if (session != null) {
+            // Authenticated: drop the short unauthenticated deadline.
+            session.setIdleTimeout(0);

Review Comment:
   After browser-side `connection_init` authentication, Jetty interprets an idle timeout of `0` as disabled. This makes these sockets bypass the factory's normal configured idle limit indefinitely, unlike sockets authenticated during the upgrade. The scheduled task already enforces the authentication deadline, so preserve and restore the session's original timeout (or avoid changing it at all).



##########
graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/servlet/auth/GraphQLServletSecurityValidator.java:
##########
@@ -109,13 +110,55 @@ private boolean isPublicOperation(String query) {
         return true;
     }
 
+    /**
+     * Authenticates a WebSocket upgrade that carries an {@code Authorization} header. Subscriptions are
+     * never public, so only Basic JAAS credentials are accepted.
+     *
+     * @return true when the caller is authenticated; false after a 401 has been sent
+     */
+    public boolean validateWebSocketUpgrade(HttpServletRequest req, HttpServletResponse res) throws IOException {
+        if (req.getHeader("Authorization") == null) {
+            res.addHeader("WWW-Authenticate", "Basic realm=\"karaf\"");
+            res.sendError(HttpServletResponse.SC_UNAUTHORIZED);
+            return false;
+        }
+        if (isAuthenticatedUser(req)) {
+            return true;
+        }
+        res.sendError(HttpServletResponse.SC_UNAUTHORIZED);
+        return false;
+    }
+
+    /**
+     * Authenticates a Basic credential that did not arrive on an HTTP request, such as the one a browser
+     * client sends in the WebSocket {@code connection_init} payload. Same JAAS check as the HTTP path.
+     */
+    public boolean authenticateBasicCredential(String authorizationValue) {
+        return authenticateBasic(authorizationValue, null);
+    }
+
     private boolean isAuthenticatedUser(HttpServletRequest req) {
         req.setAttribute(AUTHENTICATION_TYPE, HttpServletRequest.BASIC_AUTH);
+        return authenticateBasic(req.getHeader("Authorization"), req);
+    }
 
-        String authHeader = req.getHeader("Authorization");
-
-        String usernameAndPassword = new String(Base64.getDecoder().decode(authHeader.substring(6).getBytes()));
+    /**
+     * @param req the originating request, or {@code null} when the credential did not arrive on one
+     */
+    private boolean authenticateBasic(String authHeader, HttpServletRequest req) {
+        if (authHeader == null || !authHeader.startsWith("Basic ")) {

Review Comment:
   HTTP authentication scheme names are case-insensitive, so this exact-case check rejects valid headers such as `basic <credentials>` or `BASIC <credentials>` on both HTTP and WebSocket paths. Use a case-insensitive prefix comparison while retaining the existing credential decoding.



-- 
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: dev-unsubscribe@unomi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org