Skip to content

fix(QTDI-2954): fix possible NPE in VaultClient #1230

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
undx wants to merge 5 commits into
base: master
Choose a base branch
from
Open

fix(QTDI-2954): fix possible NPE in VaultClient #1230

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b0b566a
fix(QTDI-2954): fix possible NPE in VaultClient
undx May 28, 2026
82c6e7a
fix(QTDI-2954): changes for copilot and sonar warnings
undx May 28, 2026
5843ca8
fix(QTDI-2954): changes requested by copilot
undx May 28, 2026
ddef9b8
Potential fix for pull request finding
undx May 28, 2026
9a3bf85
Merge branch 'master' into ouf/QTDI-2954-guard-npe
undx May 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 63 additions & 32 deletions vault-client/src/main/java/org/talend/sdk/components/vault/client/VaultClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,13 @@ public class VaultClient {
private final Predicate<Throwable> 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;
}
Comment thread
undx marked this conversation as resolved.
final int status = response.getStatus();
if (Status.NOT_FOUND.getStatusCode() == status || status >= 500) {
return false;
}
Expand Down Expand Up @@ -327,7 +333,7 @@ private CompletionStage<List<DecryptedValue>> doDecipher(final Collection<String
})
// oops, smtg went wrong
.exceptionally(e -> {
final Throwable cause = e.getCause();
final Throwable cause = unwrap(e);
String message = "";
int status = cantDecipherStatusCode;
if (WebApplicationException.class.isInstance(cause)) {
Expand Down Expand Up @@ -408,29 +414,34 @@ private CompletionStage<Authentication> 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()) {
Comment thread
undx marked this conversation as resolved.
Outdated
message = cause.getMessage();
}
throwError(response.getStatus(), message);
}
}
throwError(cause);
return null;
}

private <T> CompletableFuture<T> withRetries(final Supplier<CompletableFuture<T>> attempt,
Expand All @@ -449,9 +460,9 @@ private <T> CompletableFuture<T> retryFuture(final Supplier<CompletableFuture<T>
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)
Expand All @@ -469,13 +480,19 @@ 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();
status = response.getStatus();
if (response != null) {
status = response.getStatus();
Comment thread
undx marked this conversation as resolved.
if (ErrorPayload.class.isInstance(response.getEntity())) { // internal error
throw wae;
} else {
Expand All @@ -487,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) {
Expand Down
125 changes: 122 additions & 3 deletions vault-client/src/test/java/org/talend/sdk/components/vault/client/VaultClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -296,6 +299,124 @@ 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 vaultClient = new VaultClient();
vaultClient.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(vaultClient, 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());
}
}

@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<Throwable> shouldRetry =
(java.util.function.Predicate<Throwable>) 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));
}

@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
*
Expand All @@ -320,9 +441,7 @@ void executeWithBadEncrypted() {
@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_");
Expand Down
Loading