Conversation
parseUrlEncodedParameters() splits on & and then on the first =, with no notion of a scheme or a ?, so handing it a whole URI folds the scheme into the first key. openid4vp://?request_uri=... yielded the single key openid4vp://?request_uri and the lookup threw "No request_uri". The call only worked when some other parameter preceded it, and even then client_id itself was unreadable for the same reason -- it just went unused. OpenID4VP 1.0 puts client_id inside the signed request object and section 5.10 lets the Authorization Request reference it by request_uri, so a verifier that signs its request has no obligation to repeat client_id in the outer query. request_uri on its own is a request a conforming verifier may send. Takes the substring after the first ? rather than going through Url, whose handling of a custom scheme with an empty authority is less obvious here. A URI with no ? yields no parameters, which reaches the same "No request_uri". Test: added UriSchemeQueryParsingTest, five cases covering request_uri alone, request_uri after client_id, the key names, request_uri_method, and a URI with no query. Confirmed three of them fail against the previous implementation. Ran detektMetadataCommonMain. Signed-off-by: Tatsuya Katsuhara <kthrtty@gmail.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2032
parseUrlEncodedParameters()splits on&and then on the first=, with no notion of a scheme or a?, so handing it a whole URI folds the scheme into the first key.openid4vp://?request_uri=...yielded the single keyopenid4vp://?request_uriand the lookup threwNo request_uri.The call only worked when some other parameter preceded
request_uri. Even thenclient_iditself was unreadable for the same reason, its key beingopenid4vp://?client_id; it simply went unused, which is why this stayed hidden.OpenID4VP 1.0 puts
client_idinside the signed request object and section 5.10 lets the Authorization Request reference it byrequest_uri, so a verifier that signs its request has no obligation to repeatclient_idin the outer query.request_urion its own is a request a conforming verifier may send.Approach
Takes the substring after the first
?rather than going throughUrl, whose handling of a custom scheme with an empty authority is less obvious in this context. A URI with no?yields no parameters, which reaches the sameNo request_urias before.The helper is
internalso the parsing can be tested on its own;uriSchemePresentment()itself needs an HTTP client and aPresentmentSource.Verification
UriSchemeQueryParsingTestcovers five cases:request_urialone,request_uriafterclient_id, the resulting key names,request_uri_method, and a URI with no query.Three of them fail against the previous implementation, so they hold the regression rather than merely passing:
Ran
:multipaz:jvmTestanddetektMetadataCommonMain.Found while wiring the iOS wallet's custom URL schemes (openwallet-foundation/multipaz-wallet#35).
Not included
No CHANGELOG entry: the 0.102.0 section lists feature-sized items and this is a small parsing fix. Happy to add one if you consider it notable.