redirectURI containing ? without actual query parameters

[Ádám Siklósi]

The toURI() function class com.nimbusds.oauth2.sdk.AuthorizationResponse contains the following snippet: (checked version 6.14 and 8.4)

if (StringUtils.isBlank(getRedirectionURI().getRawQuery())) {
    sb.append('?');
} else {
    // The original redirect_uri may contain query params,
    // see <http://tools.ietf.org/html/rfc6749#section-3.1.2>
    sb.append('&');
}

According to https://tools.ietf.org/html/rfc3986#section-3.4

The characters slash ("/") and question mark ("?") may represent data within the query component.

To my understanding this means that if "?" is present in the URI it doesn't necessary indicate that there are actual query parameters. Given what the code’s purpose is, in my opinion it is incorrectly assuming that it has to append a “?”.

‌

Certain Relying Party implementations require the ? to be always present. Even if there are no query parameters. Example: "https://host/path?"
This might be questionable practice, but doesn't violate the above mentioned RFC.

‌

The problem raises when the AuthorizationResponse.toURI() generates "https://host/path??code=..." in this case. Which breaks the authentication flow in some cases after the redirect. Although this behaviour might be a bug on the client side as the RFC also states:

The query component is indicated by the first question mark ("?")

Telling that there might be more than one“?” in the URI.

‌

Proposal

Do not append “?” to the URI when there are no query parameters and there is a “?” already. (this must be a 3rd case: not adding “?” nor “&”)

[Yavor Vasilev]

Thanks for the report, will add a test check that

[Ádám Siklósi]

Thanks for the quick response!

Just an idea: at some point you can also evaluate to use URIBuilder from Apache or something like that. To avoid dealing with these characters yourself.

(I don’t have experience with it myself, but seems like a good idea: https://stackoverflow.com/questions/26614721/how-can-i-modify-update-a-java-net-uri-object)

[Yavor Vasilev]

Thanks. We have a policy to avoid ext deps.

[Yavor Vasilev]

Fixed: 13ab24d1

[Yavor Vasilev]

Check out v 8.4.1 in a few minutes.

Thanks for reporting!

[Ádám Siklósi]

Seems like you fixed a different problem, maybe my description was not clear enough.

I try to demonstrate with variables values in the code:

• StringBuilder sb = new StringBuilder(getRedirectionURI().toString()); is “http://host/path?“
• String serializedParameters = URLUtils.serializeParameters(toParameters()); is “code=xxxx”
• Therefore new URI(sb.toString()) will become “http://host/path??code=xxxx“

This happens, because if (StringUtils.isBlank(getRedirectionURI().getRawQuery())) { is just checking for query parameters for adding the “?”. However the “?” can be already present in the URI even if there are no query parameters.

[Yavor Vasilev]

Check out v8.4.2

Commit: cb0b043fd009b2f5fb80937a6cd1be4958b78cf3

[Ádám Siklósi]

Great! Thanks a lot!