-
Notifications
You must be signed in to change notification settings - Fork 75
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
fix: use url-safe b64 encoding in deep links #580
fix: use url-safe b64 encoding in deep links #580
Conversation
Signed-off-by: Ariel Gentile <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>
8ab0d7a
to
14f9ac7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes good call. Wasn't failing for the BC gov cases (on local dev or our deployed environments) but I think should definitely be the url-safe format for all uses.
Tried locally with these changes and it's good for me. Note about string format flake failure in code comment below.
oidc-controller/api/routers/oidc.py
Outdated
if settings.USE_URL_DEEP_LINK: | ||
suffix = ( | ||
f'_url={base64.b64encode(url_to_message.encode("utf-8")).decode("utf-8")}' | ||
f'_url={base64.urlsafe_b64encode( | ||
url_to_message.encode("utf-8")).decode("utf-8")}' | ||
) | ||
else: | ||
suffix = f'c_i={base64.b64encode(formated_msg.encode("utf-8")).decode("utf-8")}' | ||
suffix = ( | ||
f'c_i={base64.urlsafe_b64encode( | ||
formated_msg.encode("utf-8")).decode("utf-8")}' | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The flake8 unterminated string literal
error here also crops as a runtime on my docker setup
I'm kind of confused as I thought parenthesis with multi-line like this should work... maybe it's the {
group breaking across multiple lines.
I made it work locally with triple quote
# BC Wallet deep link
if settings.USE_URL_DEEP_LINK:
suffix = f"""_url={base64.urlsafe_b64encode
(url_to_message.encode("utf-8")).decode("utf-8")}"""
suffix = f"""c_i={base64.urlsafe_b64encode(
formated_msg.encode("utf-8")).decode("utf-8")}"""
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah I think it's this: https://discuss.python.org/t/multiple-lines-in-f-string-replacement-field/24214
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tip! I think it is OK now.
Signed-off-by: Ariel Gentile <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
I'm not sure if it already works well in your BC Wallet, but in our app we are using Credo to parse the DIDComm out-of-band invitation and it was failing, since it expects to have only base64url characters. This comes from Aries RFC 0268, which talks about "base64url-encoded invitation". This Base 64 encoding with an URL and filename safe alphabet is defined in https://datatracker.ietf.org/doc/html/rfc4648#section-5.
So I fixed it by simply using
urlsafe_b64encode
instead of regularb64encode
for _url or c_i parameter.I didn't test it in BC Wallet, but if it is using Credo as well I don't see why it wouldn't work 🤔 .