Skip to content

Unexpected Exception Handling in NimbusReactiveJwtDecoder decode Method #14467

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.

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

Closed
Kardeen opened this issue Jan 17, 2024 · 3 comments
Closed
Assignees
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: bug A general bug

Comments

@Kardeen
Copy link
Contributor

Kardeen commented Jan 17, 2024

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:

@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:

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.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 17, 2024
@milaneuh
Copy link
Contributor

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
Copy link
Contributor

jzheaux commented Jan 18, 2024

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?

@jzheaux jzheaux self-assigned this Jan 18, 2024
@jzheaux jzheaux added type: bug A general bug in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 18, 2024
Kardeen added a commit to Kardeen/spring-security that referenced this issue Jan 18, 2024
Previously, the `decode` method threw a `JwtException` directly when encountering an unsupported algorithm or any exception during parsing. This commit introduces a more robust error handling mechanism. Now, instead of throwing exceptions directly, it returns a `Mono.error()` with a `BadJwtException` containing detailed error information. This approach provides more flexibility and allows the caller to handle errors in a more granular way, by being able to use project reactors onError functionality.

Closes spring-projectsgh-14467
@Kardeen
Copy link
Contributor Author

Kardeen commented Jan 18, 2024

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.

Kardeen added a commit to Kardeen/spring-security that referenced this issue Jan 22, 2024
Previously, the `decode` method threw a `JwtException` directly when encountering an unsupported algorithm or any exception during parsing. This commit introduces a more robust error handling mechanism. Now, instead of throwing exceptions directly, it returns a `Mono.error()` with a `BadJwtException` containing detailed error information. This approach provides more flexibility and allows the caller to handle errors in a more granular way, by being able to use project reactors onError functionality.

Closes spring-projectsgh-14467
Kardeen added a commit to Kardeen/spring-security that referenced this issue Jan 22, 2024
Previously, the `decode` method threw a `JwtException` directly when encountering an unsupported algorithm or any exception during parsing. This commit introduces a more robust error handling mechanism. Now, instead of throwing exceptions directly, it returns a `Mono.error()` with a `BadJwtException` containing detailed error information. This approach provides more flexibility and allows the caller to handle errors in a more granular way, by being able to use project reactors onError functionality.

Closes spring-projectsgh-14467
jzheaux added a commit that referenced this issue Jan 26, 2024
jzheaux added a commit that referenced this issue Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants