fix: Fixes importing url encoded recipe deeplinks #3446
Conversation
jsibbison-square
changed the title
DOsinga
left a comment
DOsinga
left a comment
There was a problem hiding this comment.
So this is probably better than what we had before and if you don't have time, let's get something like this in, but if you could, I would prefer a more thorough overhaul (we ran into an issue like this before and then we decided on a quick fix that didn't quite).
Deeplinks should just be base64 encoded, but base64 web safe. We should do this in the server and the client - or possibly even better add this to the recipe routes and only do it on the server in one central place.
If that fails, we can do the url decode/encode to see if we can get that to work.
What do you think?
| throw new Error('No recipe configuration found in deeplink'); | ||
| } | ||
| const configJson = Buffer.from(configBase64, 'base64').toString('utf-8'); | ||
| const urlDecoded = decodeURIComponent(configBase64); |
There was a problem hiding this comment.
I think if the configBase64 was not url encoded and it does contain a +, this will cause issues
There was a problem hiding this comment.
Yeah I think we should officially document the format of recipe deeplinks so that external integrations know how to use them. I expect we should use the url_safe version and then url-encode. Unless we are going the route of url_safe_no_pad.
These changes are backwards incompatible which is why I'm hoping we can make an official/decision statement so we have a position to operate from.
There was a problem hiding this comment.
They are backwards compatible, but since we have both used url-encoded and not url-encoded versions, it doesn't quite fix it.
is there a point in url-encoding the url_safe version?
There was a problem hiding this comment.
Seems like even with base64::engine::general_purpose::URL_SAFE we still want to encode it for maximum interoperability. Eg. %3D unencoded is = which would break if this was part of a query url or form submit.
There was a problem hiding this comment.
I am not sure I follow - the whole point of url safe base64 is that you can use it in, eh, urls. so url safe base64 with the = stripped seems like the most compact and portable way that we can do this
for decoding we'd have to put a bunch of safety measures in place probably, so still try to url-decode it if the base64 decode doesn't work etc
There was a problem hiding this comment.
👍 yep we can use URL_SAFE_NO_PAD so the = are never added into the strings (so no need to url encode). It just means that standard base64 decoding won't work unless you add the NO_PAD option. https://docs.rs/base64/latest/base64/#padding-characters
We will still have to have backwards compatible decoding for the old ones we generated, so ultimately won't simplify any code but atleast there will be a standard going forward.
There was a problem hiding this comment.
I'm working on having the encoding/decoding only done on the server.
There was a problem hiding this comment.
great - do you want to do that in this, or merge this first
Signed-off-by: Soroosh <[email protected]>
Signed-off-by: Kyle Santiago <[email protected]>
Signed-off-by: Kyle Santiago <[email protected]>
Signed-off-by: Adam Tarantino <[email protected]>
Looks like in #2845 we starting url encoding deeplinks in the cli and the ui was updated for opening deeplinks in that pr. However since then there is an Import recipe dialogue and that code has a different execution path that was not urldecoding.
This PR fixes that for #3373 . Just exploring if there is a way to unify some of the code a little.
We should have a separate discussion around the expected recipe deeplink format: url-encoded vs base64 no padding etc.
Also unifying this import code with the existing code in main.ts#processProtocolUrl is a deeper investigation and needs to wait for the new UI to land.