From b0b566a3701b4f51b3c6d78b54df4ebc067509e4 Mon Sep 17 00:00:00 2001 From: Emmanuel GALLOIS Date: Thu, 28 May 2026 11:43:19 +0200 Subject: [PATCH 1/4] fix(QTDI-2954): fix possible NPE in VaultClient --- .../components/vault/client/VaultClient.java | 2 +- .../vault/client/VaultClientTest.java | 34 +++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/vault-client/src/main/java/org/talend/sdk/components/vault/client/VaultClient.java b/vault-client/src/main/java/org/talend/sdk/components/vault/client/VaultClient.java index 2d7ab3937f2ed..5a097b74d5e91 100644 --- a/vault-client/src/main/java/org/talend/sdk/components/vault/client/VaultClient.java +++ b/vault-client/src/main/java/org/talend/sdk/components/vault/client/VaultClient.java @@ -474,8 +474,8 @@ private void throwError(final Throwable cause) { if (WebApplicationException.class.isInstance(cause)) { final WebApplicationException wae = WebApplicationException.class.cast(cause); final Response response = wae.getResponse(); - status = response.getStatus(); if (response != null) { + status = response.getStatus(); if (ErrorPayload.class.isInstance(response.getEntity())) { // internal error throw wae; } else { diff --git a/vault-client/src/test/java/org/talend/sdk/components/vault/client/VaultClientTest.java b/vault-client/src/test/java/org/talend/sdk/components/vault/client/VaultClientTest.java index 0e7ad456027c5..6f47021fbd299 100644 --- a/vault-client/src/test/java/org/talend/sdk/components/vault/client/VaultClientTest.java +++ b/vault-client/src/test/java/org/talend/sdk/components/vault/client/VaultClientTest.java @@ -19,12 +19,15 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; import java.util.HashMap; import java.util.Map; import javax.enterprise.inject.se.SeContainer; import javax.enterprise.inject.se.SeContainerInitializer; import javax.inject.Inject; +import javax.ws.rs.WebApplicationException; import javax.ws.rs.client.Client; import javax.ws.rs.client.Entity; import javax.ws.rs.client.WebTarget; @@ -296,6 +299,37 @@ void executeWithBadEncrypted() { response.readEntity(ErrorPayload.class).getDescription()); } + @Test + void throwErrorWithNullResponseDoesNotNpe() throws Exception { + // A WebApplicationException whose getResponse() returns null must not trigger an NPE + // in throwError(Throwable); the default cantDecipherStatusCode should be used instead. + final int defaultStatus = 422; + // Use a fresh VaultClient instance (not the CDI proxy) so reflective field access works. + final VaultClient client = new VaultClient(); + client.setCantDecipherStatusCode(defaultStatus); + final WebApplicationException waeWithoutResponse = new WebApplicationException("boom") { + + @Override + public Response getResponse() { + return null; + } + }; + + final Method throwError = VaultClient.class.getDeclaredMethod("throwError", Throwable.class); + throwError.setAccessible(true); + try { + throwError.invoke(client, waeWithoutResponse); + org.junit.jupiter.api.Assertions.fail("Expected a WebApplicationException to be thrown"); + } catch (final InvocationTargetException ite) { + final Throwable target = ite.getTargetException(); + assertTrue(target instanceof WebApplicationException, + "Expected WebApplicationException, got " + target); + final WebApplicationException thrown = (WebApplicationException) target; + assertEquals(defaultStatus, thrown.getResponse().getStatus()); + assertEquals("boom", thrown.getMessage()); + } + } + /** * Demonstration purpose: with a "real" vault server instance * From 82c6e7adeffa0c0eee7208343bc151c0b8dc4b52 Mon Sep 17 00:00:00 2001 From: Emmanuel GALLOIS Date: Thu, 28 May 2026 13:32:06 +0200 Subject: [PATCH 2/4] fix(QTDI-2954): changes for copilot and sonar warnings --- .../components/vault/client/VaultClient.java | 8 ++++- .../vault/client/VaultClientTest.java | 33 +++++++++++++++---- 2 files changed, 34 insertions(+), 7 deletions(-) diff --git a/vault-client/src/main/java/org/talend/sdk/components/vault/client/VaultClient.java b/vault-client/src/main/java/org/talend/sdk/components/vault/client/VaultClient.java index 5a097b74d5e91..c50847a0efd31 100644 --- a/vault-client/src/main/java/org/talend/sdk/components/vault/client/VaultClient.java +++ b/vault-client/src/main/java/org/talend/sdk/components/vault/client/VaultClient.java @@ -155,7 +155,13 @@ public class VaultClient { private final Predicate shouldRetry = cause -> { if (WebApplicationException.class.isInstance(cause)) { final WebApplicationException wae = WebApplicationException.class.cast(cause); - final int status = wae.getResponse().getStatus(); + final Response response = wae.getResponse(); + if (response == null) { + // no response information: don't make the retry loop NPE, keep retrying as a + // recoverable error (the underlying cause is logged by retryFuture). + return true; + } + final int status = response.getStatus(); if (Status.NOT_FOUND.getStatusCode() == status || status >= 500) { return false; } diff --git a/vault-client/src/test/java/org/talend/sdk/components/vault/client/VaultClientTest.java b/vault-client/src/test/java/org/talend/sdk/components/vault/client/VaultClientTest.java index 6f47021fbd299..d89dd64817832 100644 --- a/vault-client/src/test/java/org/talend/sdk/components/vault/client/VaultClientTest.java +++ b/vault-client/src/test/java/org/talend/sdk/components/vault/client/VaultClientTest.java @@ -305,8 +305,8 @@ void throwErrorWithNullResponseDoesNotNpe() throws Exception { // in throwError(Throwable); the default cantDecipherStatusCode should be used instead. final int defaultStatus = 422; // Use a fresh VaultClient instance (not the CDI proxy) so reflective field access works. - final VaultClient client = new VaultClient(); - client.setCantDecipherStatusCode(defaultStatus); + final VaultClient vaultClient = new VaultClient(); + vaultClient.setCantDecipherStatusCode(defaultStatus); final WebApplicationException waeWithoutResponse = new WebApplicationException("boom") { @Override @@ -318,7 +318,7 @@ public Response getResponse() { final Method throwError = VaultClient.class.getDeclaredMethod("throwError", Throwable.class); throwError.setAccessible(true); try { - throwError.invoke(client, waeWithoutResponse); + throwError.invoke(vaultClient, waeWithoutResponse); org.junit.jupiter.api.Assertions.fail("Expected a WebApplicationException to be thrown"); } catch (final InvocationTargetException ite) { final Throwable target = ite.getTargetException(); @@ -330,6 +330,29 @@ public Response getResponse() { } } + @Test + @SuppressWarnings("unchecked") + void shouldRetryWithNullResponseDoesNotNpe() throws Exception { + // Regression: shouldRetry must not call wae.getResponse().getStatus() when the response is null; + // otherwise retryFuture() raises an NPE before throwError(...) can build the fallback payload. + final VaultClient vaultClient = new VaultClient(); + final java.lang.reflect.Field f = VaultClient.class.getDeclaredField("shouldRetry"); + f.setAccessible(true); + final java.util.function.Predicate shouldRetry = + (java.util.function.Predicate) f.get(vaultClient); + + final WebApplicationException waeWithoutResponse = new WebApplicationException("boom") { + + @Override + public Response getResponse() { + return null; + } + }; + + // Must not NPE, and must allow the retry loop to keep running (true = retry). + assertTrue(shouldRetry.test(waeWithoutResponse)); + } + /** * Demonstration purpose: with a "real" vault server instance * @@ -354,9 +377,7 @@ public Response getResponse() { @Test @Disabled void executeWithRealVault() { - // setup.setVaultUrl("http://localhost:8200"); - final Client realClt = setup.vaultClient(setup.vaultExecutorService()); - // vault.setVault(setup.vaultTarget(realClt)); + setup.vaultClient(setup.vaultExecutorService()); vault.setAuthEndpoint("/v1/auth/approle/login"); vault.setDecryptEndpoint("/v1/transit/decrypt/{x-talend-tenant-id}"); vault.setRole(() -> "_role-id_"); From 5843ca814b0c1baa2bf2c81e4c5c7c2032252422 Mon Sep 17 00:00:00 2001 From: Emmanuel GALLOIS Date: Thu, 28 May 2026 16:00:20 +0200 Subject: [PATCH 3/4] fix(QTDI-2954): changes requested by copilot --- .../components/vault/client/VaultClient.java | 85 ++++++++++++------- .../vault/client/VaultClientTest.java | 64 ++++++++++++++ 2 files changed, 119 insertions(+), 30 deletions(-) diff --git a/vault-client/src/main/java/org/talend/sdk/components/vault/client/VaultClient.java b/vault-client/src/main/java/org/talend/sdk/components/vault/client/VaultClient.java index c50847a0efd31..229dd5942bfa6 100644 --- a/vault-client/src/main/java/org/talend/sdk/components/vault/client/VaultClient.java +++ b/vault-client/src/main/java/org/talend/sdk/components/vault/client/VaultClient.java @@ -333,7 +333,7 @@ private CompletionStage> doDecipher(final Collection { - final Throwable cause = e.getCause(); + final Throwable cause = unwrap(e); String message = ""; int status = cantDecipherStatusCode; if (WebApplicationException.class.isInstance(cause)) { @@ -414,29 +414,34 @@ private CompletionStage doAuth(final String role, final String s return authentication; }) // - .exceptionally(e -> { - final Throwable cause = e.getCause(); - if (WebApplicationException.class.isInstance(cause)) { - final WebApplicationException wae = WebApplicationException.class.cast(cause); - final Response response = wae.getResponse(); - String message = ""; - if (ErrorPayload.class.isInstance(wae.getResponse().getEntity())) { - throw wae; // already logged and setup broken so just rethrow - } else { - try { - message = response.readEntity(String.class); - } catch (final Exception ignored) { - // no-op - } - if (message.isEmpty()) { - message = cause.getMessage(); - } - throwError(response.getStatus(), message); - } - } - throwError(cause); - return null; - }); + .exceptionally(this::handleAuthException); + } + + private Authentication handleAuthException(final Throwable e) { + final Throwable cause = unwrap(e); + if (WebApplicationException.class.isInstance(cause)) { + final WebApplicationException wae = WebApplicationException.class.cast(cause); + final Response response = wae.getResponse(); + // if the response is null we cannot extract status/entity, fall back to the + // null-safe throwError(Throwable) below (avoids an NPE that would mask the cause). + if (response != null) { + String message = ""; + if (ErrorPayload.class.isInstance(response.getEntity())) { + throw wae; // already logged and setup broken so just rethrow + } + try { + message = response.readEntity(String.class); + } catch (final Exception ignored) { + // no-op + } + if (message.isEmpty()) { + message = cause.getMessage(); + } + throwError(response.getStatus(), message); + } + } + throwError(cause); + return null; } private CompletableFuture withRetries(final Supplier> attempt, @@ -455,9 +460,9 @@ private CompletableFuture retryFuture(final Supplier log .info("[retryFuture] Retry failed operation ({}/{}). Reason: {}.", attemptsSoFar, numberOfRetryOnFailure, throwable.getMessage()); - if (nextAttempt > numberOfRetryOnFailure || !shouldRetry.test(throwable.getCause())) { + if (nextAttempt > numberOfRetryOnFailure || !shouldRetry.test(unwrap(throwable))) { log.info("[retryFuture] Stop retry failed operation (condition triggered)."); - throwError(throwable.getCause()); + throwError(unwrap(throwable)); } return flatten(flatten(CompletableFuture.supplyAsync(attempter, scheduler)) .thenApply(CompletableFuture::completedFuture) @@ -475,10 +480,16 @@ private void throwError(final int status, final String message) { } private void throwError(final Throwable cause) { + // CompletableFuture#exceptionally hands the exception "as is". If the previous stage threw + // the exception directly (not wrapped in a CompletionException), Throwable#getCause() can + // return null. Be defensive so we never trigger an NPE that would mask the original error. + final Throwable safeCause = cause != null + ? cause + : new IllegalStateException("Unknown error (null cause)"); String message = ""; int status = cantDecipherStatusCode; - if (WebApplicationException.class.isInstance(cause)) { - final WebApplicationException wae = WebApplicationException.class.cast(cause); + if (WebApplicationException.class.isInstance(safeCause)) { + final WebApplicationException wae = WebApplicationException.class.cast(safeCause); final Response response = wae.getResponse(); if (response != null) { status = response.getStatus(); @@ -493,13 +504,27 @@ private void throwError(final Throwable cause) { } } } - if (message.isEmpty()) { - message = cause.getMessage(); + if (message == null || message.isEmpty()) { + message = safeCause.getMessage(); } throw new WebApplicationException(message, Response.status(status).entity(new ErrorPayload(ErrorDictionary.UNEXPECTED, message)).build()); } + /** + * Returns a non-null root cause for a {@link Throwable} received by a + * {@link java.util.concurrent.CompletableFuture#exceptionally(java.util.function.Function)} lambda. + * Falls back to the exception itself when {@link Throwable#getCause()} is {@code null} (the + * upstream stage threw the exception directly instead of wrapping it in a {@code CompletionException}). + */ + private static Throwable unwrap(final Throwable e) { + if (e == null) { + return new IllegalStateException("Unknown error (null exception)"); + } + final Throwable cause = e.getCause(); + return cause != null ? cause : e; + } + // workaround while geronimo-config does not support generics of generics // (1.2.1 in org.apache.geronimo.config.cdi.ConfigInjectionBean.create) private boolean isReloadableConfigSet(final String value) { diff --git a/vault-client/src/test/java/org/talend/sdk/components/vault/client/VaultClientTest.java b/vault-client/src/test/java/org/talend/sdk/components/vault/client/VaultClientTest.java index d89dd64817832..8149fda525d8b 100644 --- a/vault-client/src/test/java/org/talend/sdk/components/vault/client/VaultClientTest.java +++ b/vault-client/src/test/java/org/talend/sdk/components/vault/client/VaultClientTest.java @@ -353,6 +353,70 @@ public Response getResponse() { assertTrue(shouldRetry.test(waeWithoutResponse)); } + @Test + void handleAuthExceptionWithNullResponseDoesNotNpe() throws Exception { + // Regression: doAuth().exceptionally() used to dereference wae.getResponse().getEntity() + // and response.getStatus() without a null check; with a null response that NPE masked the + // original cause. The handler now falls through to throwError(Throwable) which is null-safe. + final int defaultStatus = 422; + final VaultClient client = new VaultClient(); + client.setCantDecipherStatusCode(defaultStatus); + + final WebApplicationException waeWithoutResponse = new WebApplicationException("boom") { + + @Override + public Response getResponse() { + return null; + } + }; + // Mimics what CompletableFuture#exceptionally receives: a CompletionException wrapping cause. + final java.util.concurrent.CompletionException wrapped = + new java.util.concurrent.CompletionException(waeWithoutResponse); + + final java.lang.reflect.Method handler = + VaultClient.class.getDeclaredMethod("handleAuthException", Throwable.class); + handler.setAccessible(true); + try { + handler.invoke(client, wrapped); + org.junit.jupiter.api.Assertions.fail("Expected a WebApplicationException to be thrown"); + } catch (final java.lang.reflect.InvocationTargetException ite) { + final Throwable target = ite.getTargetException(); + assertTrue(target instanceof WebApplicationException, + "Expected WebApplicationException, got " + target); + final WebApplicationException thrown = (WebApplicationException) target; + assertEquals(defaultStatus, thrown.getResponse().getStatus()); + assertEquals("boom", thrown.getMessage()); + } + } + + @Test + void handleAuthExceptionWithNullCauseDoesNotNpe() throws Exception { + // Regression: CompletableFuture#exceptionally can receive an exception thrown directly + // (not wrapped in CompletionException), so e.getCause() may be null. The handler must + // not NPE on cause.getMessage() / shouldRetry / throwError. + final int defaultStatus = 422; + final VaultClient client = new VaultClient(); + client.setCantDecipherStatusCode(defaultStatus); + + // RuntimeException with no cause => e.getCause() returns null. + final RuntimeException directThrow = new RuntimeException("direct"); + + final java.lang.reflect.Method handler = + VaultClient.class.getDeclaredMethod("handleAuthException", Throwable.class); + handler.setAccessible(true); + try { + handler.invoke(client, directThrow); + org.junit.jupiter.api.Assertions.fail("Expected a WebApplicationException to be thrown"); + } catch (final java.lang.reflect.InvocationTargetException ite) { + final Throwable target = ite.getTargetException(); + assertTrue(target instanceof WebApplicationException, + "Expected WebApplicationException, got " + target); + final WebApplicationException thrown = (WebApplicationException) target; + assertEquals(defaultStatus, thrown.getResponse().getStatus()); + assertEquals("direct", thrown.getMessage()); + } + } + /** * Demonstration purpose: with a "real" vault server instance * From ddef9b8dbf9710cb4d42b0d4cdef1227da84f8e4 Mon Sep 17 00:00:00 2001 From: Emmanuel GALLOIS Date: Thu, 28 May 2026 16:10:47 +0200 Subject: [PATCH 4/4] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../org/talend/sdk/components/vault/client/VaultClient.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vault-client/src/main/java/org/talend/sdk/components/vault/client/VaultClient.java b/vault-client/src/main/java/org/talend/sdk/components/vault/client/VaultClient.java index 229dd5942bfa6..2b3b2526cb6fe 100644 --- a/vault-client/src/main/java/org/talend/sdk/components/vault/client/VaultClient.java +++ b/vault-client/src/main/java/org/talend/sdk/components/vault/client/VaultClient.java @@ -434,7 +434,7 @@ private Authentication handleAuthException(final Throwable e) { } catch (final Exception ignored) { // no-op } - if (message.isEmpty()) { + if (message == null || message.isEmpty()) { message = cause.getMessage(); } throwError(response.getStatus(), message);