From 79738cc7003dadbc22794c819c07dd902523a80d Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Mon, 13 Mar 2023 18:47:37 -0400 Subject: [PATCH] fix(credentials): threadlocal credentials are cleared before request processing Signed-off-by: Andrew Azores --- .../http/AbstractAuthenticatedRequestHandler.java | 15 +++++++++++++-- .../web/http/api/v2/AbstractV2RequestHandler.java | 5 ++++- .../v2/graph/AbstractPermissionedDataFetcher.java | 1 + .../api/v1/TargetRecordingDeleteHandlerTest.java | 1 + 4 files changed, 19 insertions(+), 3 deletions(-) diff --git a/src/main/java/io/cryostat/net/web/http/AbstractAuthenticatedRequestHandler.java b/src/main/java/io/cryostat/net/web/http/AbstractAuthenticatedRequestHandler.java index 016746fce3..8fce5e2ea8 100644 --- a/src/main/java/io/cryostat/net/web/http/AbstractAuthenticatedRequestHandler.java +++ b/src/main/java/io/cryostat/net/web/http/AbstractAuthenticatedRequestHandler.java @@ -67,6 +67,7 @@ import io.vertx.core.http.HttpServerRequest; import io.vertx.ext.web.RoutingContext; import io.vertx.ext.web.handler.HttpException; +import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.exception.ExceptionUtils; public abstract class AbstractAuthenticatedRequestHandler implements RequestHandler { @@ -120,6 +121,10 @@ protected Future validateRequestAuthorization(HttpServerRequest req) th protected ConnectionDescriptor getConnectionDescriptorFromContext(RoutingContext ctx) { String targetId = ctx.pathParam("targetId"); + if (StringUtils.isBlank(targetId)) { + throw new HttpException(404); + } + credentialsManager.setSessionCredentials(targetId, null); try { Credentials credentials; if (ctx.request().headers().contains(JMX_AUTHORIZATION_HEADER)) { @@ -157,8 +162,14 @@ protected ConnectionDescriptor getConnectionDescriptorFromContext(RoutingContext } credentials = new Credentials(parts[0], parts[1]); credentialsManager.setSessionCredentials(targetId, credentials); - ctx.addEndHandler( - unused -> credentialsManager.setSessionCredentials(targetId, null)); + // use the headers end handler to clear the ThreadLocal session credentials since + // this handler will run synchronously in the context thread, ensuring the + // ThreadLocal is cleared on time and not left dangling until an async cleanup + ctx.response() + .headersEndHandler( + unused -> credentialsManager.setSessionCredentials(targetId, null)); + // ctx.addEndHandler( + // unused -> credentialsManager.setSessionCredentials(targetId, null)); } else { credentials = credentialsManager.getCredentialsByTargetId(targetId); } diff --git a/src/main/java/io/cryostat/net/web/http/api/v2/AbstractV2RequestHandler.java b/src/main/java/io/cryostat/net/web/http/api/v2/AbstractV2RequestHandler.java index 07a852397d..ccda5e0383 100644 --- a/src/main/java/io/cryostat/net/web/http/api/v2/AbstractV2RequestHandler.java +++ b/src/main/java/io/cryostat/net/web/http/api/v2/AbstractV2RequestHandler.java @@ -96,7 +96,7 @@ public final void handle(RoutingContext ctx) { RequestParameters requestParams = RequestParameters.from(ctx); String targetId = requestParams.getPathParams().get("targetId"); if (targetId != null) { - ctx.addEndHandler(unused -> credentialsManager.setSessionCredentials(targetId, null)); + credentialsManager.setSessionCredentials(targetId, null); } try { if (requiresAuthentication()) { @@ -110,6 +110,9 @@ public final void handle(RoutingContext ctx) { } } writeResponse(ctx, handle(requestParams)); + if (targetId != null) { + credentialsManager.setSessionCredentials(targetId, null); + } } catch (ApiException | HttpException e) { throw e; } catch (Exception e) { diff --git a/src/main/java/io/cryostat/net/web/http/api/v2/graph/AbstractPermissionedDataFetcher.java b/src/main/java/io/cryostat/net/web/http/api/v2/graph/AbstractPermissionedDataFetcher.java index 196e72ae6b..24b0d73f3e 100644 --- a/src/main/java/io/cryostat/net/web/http/api/v2/graph/AbstractPermissionedDataFetcher.java +++ b/src/main/java/io/cryostat/net/web/http/api/v2/graph/AbstractPermissionedDataFetcher.java @@ -98,6 +98,7 @@ public final T get(DataFetchingEnvironment environment) throws Exception { // targetId to credentials protected Optional getSessionCredentials( DataFetchingEnvironment environment, String targetId) { + credentialsManager.setSessionCredentials(targetId, null); RoutingContext ctx = GraphQLHandler.getRoutingContext(environment.getGraphQlContext()); if (!ctx.request() .headers() diff --git a/src/test/java/io/cryostat/net/web/http/api/v1/TargetRecordingDeleteHandlerTest.java b/src/test/java/io/cryostat/net/web/http/api/v1/TargetRecordingDeleteHandlerTest.java index b721fce69c..440b51ec09 100644 --- a/src/test/java/io/cryostat/net/web/http/api/v1/TargetRecordingDeleteHandlerTest.java +++ b/src/test/java/io/cryostat/net/web/http/api/v1/TargetRecordingDeleteHandlerTest.java @@ -130,6 +130,7 @@ void shouldHandleDeletedRecording() throws Exception { @Test void shouldHandleRecordingNotFound() throws Exception { Mockito.when(ctx.pathParam("recordingName")).thenReturn("someRecording"); + Mockito.when(ctx.pathParam("targetId")).thenReturn("someTarget"); Mockito.when(ctx.request()).thenReturn(req); Mockito.when(ctx.request().headers()).thenReturn(MultiMap.caseInsensitiveMultiMap());