Copilot commented on code in PR #843:
URL: https://github.com/apache/unomi/pull/843#discussion_r3740234644
##########
graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/servlet/GraphQLServlet.java:
##########
@@ -124,7 +125,10 @@ public void configure(WebSocketServletFactory factory) {
protected void service(HttpServletRequest request, HttpServletResponse
response) throws ServletException, IOException {
LOGGER.debug("GraphQLServlet service called with request: {}",
request.getRequestURI());
if (factory.isUpgradeRequest(request, response)) {
- try {
+ if (!validator.validateWebSocketUpgrade(request, response)) {
Review Comment:
The `try` that paired with the existing `catch (URISyntaxException)` was
removed, so this method no longer parses and the module cannot compile. Restore
the `try` before the validation block.
##########
graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/servlet/websocket/SubscriptionWebSocket.java:
##########
@@ -106,33 +125,50 @@ private void unsubscribe(GraphQLMessage message) {
private void subscribe(GraphQLMessage message) {
final Map<String, Object> payload = message.getPayload();
- ExecutionInput executionInput = ExecutionInput.newExecutionInput()
- .query((String) payload.get("query"))
- .variables((Map<String, Object>) payload.get("variables"))
- .operationName((String) payload.get("operationName"))
- .context(serviceManager)
- .build();
-
- ExecutionResult executionResult = this.graphQL.execute(executionInput);
- if (executionResult.getErrors() != null &&
!executionResult.getErrors().isEmpty()) {
- sendMessage(GraphQLMessage.create(message.getId())
- .errors(executionResult.getErrors())
- .build());
- closeConnection(message, "Error executing graphQL query");
- return;
- } else if (!(executionResult.getData() instanceof Publisher)) {
- final String error = "Fetched value should be instance of
Publisher, was: " + executionResult.getClass().getName();
- sendMessage(GraphQLMessage.create(message.getId())
- .errors(Collections.singletonList(error))
- .build());
- closeConnection(message, error);
- return;
+ try {
+ securityService.setCurrentSubject(subject);
+ executionContextManager.setCurrentContext(executionContext);
+
+ Map<String, Object> variables = (Map<String, Object>)
payload.get("variables");
+ if (variables == null) {
+ variables = new HashMap<>();
+ }
+
+ ExecutionInput executionInput = ExecutionInput.newExecutionInput()
+ .query((String) payload.get("query"))
+ .variables(variables)
+ .operationName((String) payload.get("operationName"))
+ .context(serviceManager)
+ .build();
+
+ ExecutionResult executionResult =
this.graphQL.execute(executionInput);
+ if (executionResult.getErrors() != null &&
!executionResult.getErrors().isEmpty()) {
+ sendMessage(GraphQLMessage.create(message.getId())
+ .errors(executionResult.getErrors())
+ .build());
+ closeConnection(message, "Error executing graphQL query");
+ return;
+ } else if (!(executionResult.getData() instanceof Publisher)) {
+ Object data = executionResult.getData();
+ final String error = "Fetched value should be instance of
Publisher, was: " + (data == null ? "null" : data.getClass().getName());
+ .errors(Collections.singletonList(error))
+ .build());
Review Comment:
This branch lost the `sendMessage(GraphQLMessage.create(...` call, leaving
method calls with no receiver; the class will not compile. Recreate the error
message before closing the connection.
##########
graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/servlet/GraphQLServlet.java:
##########
@@ -124,7 +125,10 @@ public void configure(WebSocketServletFactory factory) {
protected void service(HttpServletRequest request, HttpServletResponse
response) throws ServletException, IOException {
LOGGER.debug("GraphQLServlet service called with request: {}",
request.getRequestURI());
if (factory.isUpgradeRequest(request, response)) {
- try {
+ if (!validator.validateWebSocketUpgrade(request, response)) {
+ cleanupSecurityContext();
+ return;
+ }
Review Comment:
Successful validation binds the subject and execution context to the servlet
thread, but cleanup is only guaranteed if the WebSocket creator runs. If
`super.service` rejects the handshake or throws before invoking the creator,
those credentials remain on a pooled thread and can leak into a later request.
Enclose the authenticated upgrade processing, including `super.service`, in a
`finally` that always calls `cleanupSecurityContext()`.
--
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]
