Unexpected Exception Handling in NimbusReactiveJwtDecoder decode Method

[Kardeen]

Description

We are experiencing an issue where exceptions thrown by the decode method are not being caught as expected when used in a reactive stream. This is causing problems when we attempt to handle these exceptions using doOnError or onErrorReturn.

Steps to reproduce:

1. Call the decode method with a token string that would fail the parsing of JWTParser.parse(String s) method e.g. eyyyyy.
2. Use the returned Mono<Jwt> in a reactive stream.
3. Attempt to catch any exceptions thrown by the decode method using doOnError or onErrorReturn.

Failing code

reactiveJwtDecoder
                .decode("tokenString")
                .doOnError(err -> log.error("Error decoding token", err))
                .flatMap(result -> Mono.just("decoded token successfully"))
                .onErrorReturn("could not decode token");

Observed Result:

The exceptions thrown by the decode method are not being caught by doOnError or onErrorReturn. Instead, they cause the reactive stream to terminate prematurely.

Expected Result:

The exceptions thrown by the decode method should be caught by doOnError or onErrorReturn and allow the reactive stream to continue processing regardless of whether the token string was provided wrapped in a Mono or directly to the method.

Additional Information:

The decode method is defined as follows:

spring-security/oauth2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/NimbusReactiveJwtDecoder.java

Lines 146 to 153 in 06f829e

	@Override
	public Mono<Jwt> decode(String token) throws JwtException {
	JWT jwt = parse(token);
	if (jwt instanceof PlainJWT) {
	throw new BadJwtException("Unsupported algorithm of " + jwt.getHeader().getAlgorithm());
	}
	return this.decode(jwt);
	}

And the parse method is defined as follows:

spring-security/oauth2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/NimbusReactiveJwtDecoder.java

Lines 155 to 162 in 06f829e

	private JWT parse(String token) {
	try {
	return JWTParser.parse(token);
	}
	catch (Exception ex) {
	throw new BadJwtException("An error occurred while attempting to decode the Jwt: " + ex.getMessage(), ex);
	}
	}

As a workaround, we have found that wrapping the token into a Mono and then using flatMap causes the exceptions to be caught correctly. However, this is not ideal as it requires modifying the code that calls the decode method.

Working workaround:

Mono.just("tokenString")
                .flatMap(reactiveJwtDecoder::decode)
                .doOnError(err -> log.error("Error decoding token", err))
                .flatMap(result -> Mono.just("decoded token successfully"))
                .onErrorReturn("could not decode token");

We believe the issue lies in the fact that the BadJwtException thrown by the parse method is not being wrapped into a Mono.error(). Similarly, the exception thrown when the jwt is an instance of PlainJWT should also be wrapped into a Mono.error().

We propose that the decode method be modified to wrap these exceptions into a Mono.error() so that they can be caught by doOnError or onErrorReturn.

Possible solution:

@Override
public Mono<Jwt> decode(String token) throws JwtException {
    try {
        JWT jwt = JWTParser.parse(token);
        if (jwt instanceof PlainJWT) {
            return Mono.error(new BadJwtException(
                "Unsupported algorithm of " + jwt.getHeader().getAlgorithm()));
        }
        return this.decode(jwt);
    } catch (Exception ex) {
        return Mono.error(new BadJwtException(
            "An error occurred while attempting to decode the Jwt: " + ex.getMessage(), ex));
    }
}

Thank you for considering this bug report. We appreciate your assistance in resolving this issue.

[milaneuh]

Wouldn't it be better for the try catch to catch the specific exception "BadJwtException" since it's the one causing the bug ?(We maybe don't need to catch other exceptions ?) Like that :

@Override
public Mono<Jwt> decode(String token) throws JwtException {
    try {
        JWT jwt = JWTParser.parse(token);
        if (jwt instanceof PlainJWT) {
            return Mono.error(new BadJwtException(
                "Unsupported algorithm of " + jwt.getHeader().getAlgorithm()));
        }
        return this.decode(jwt);
    } catch (BadJwtException ex) {
        return Mono.error(new BadJwtException(
            "An error occurred while attempting to decode the Jwt: " + ex.getMessage(), ex));
    }
}

I'm still new to working on opensource so feel free to correct me ! 😄

[jzheaux]

Hi, @Kardeen, I agree that the implementation should return Mono.error instead of throwing an exception. Are you able to submit a PR that makes the changes you described?

If so, would you please base if off of the 5.8.x branch and I'll take care of forward-porting it to other releases?

[Kardeen]

Hi @jzheaux,
I just submitted a PR implementing the changes mentioned.

@milaneuh the JWTParser.parse(token) call does return different exceptions, like ParseException, which would not be caught by the catch part. My solution reuses the previous exception handling but makes it more concise and returns it correctly using Mono.error() instead.