-
-
Notifications
You must be signed in to change notification settings - Fork 677
Description
The effect of this is that a similar symptom to #3303 is still live on iOS. But the cause is unrelated, so making this a separate issue.
Originally described at #4089 (comment) and #4089 (comment) , just before I merged #4089 fixing #3303 .
To reproduce:
-
Went to message list, found a message with an upload that wasn't an image. (To do that: in the webapp searched for
has:attachment
, and scrolled through history to find one that wasn't an image.) Specifically the file happened to be a PDF, with a.pdf
extension on its name. -
Hit the link. Got a browser view. But after a few seconds of loading, the result was an error:
That appears to be from S3 directly, because that's in the location bar at the top. The error message begins:
SignatureDoesNotMatch The request signature we calculated does not match the signature you provided. Check your key and signing method. [... then a bunch of details ...]
-
Ditto on a second try. I watched the timing closely this time, and it was only about a second between me tapping the link and the error message appearing. So it's not an expiration issue -- there's something else wrong.
On further investigation, I have a partial diagnosis:
-
From that error page, I hit the "share" icon and chose "Copy", to get the URL onto the clipboard. Then went and pasted it elsewhere (the compose box, as the handiest place.) Here it is:
https://zulip-uploads.s3.amazonaws.com/1230/-Opc2L055IYelpraPb1oRDeU/sagas.pdf?Signature=2mpNGb0ysKFpVN8bkkMVsulQSVE%253D&Expires=1591317724&AWSAccessKeyId=AKIAIEVMBCAT2WD3M5KQ
-
I went and pulled up the same upload in the webapp, to compare. (It works fine there.) Here's the URL I find in the location bar there:
https://zulip-uploads.s3.amazonaws.com/1230/-Opc2L055IYelpraPb1oRDeU/sagas.pdf?Signature=xipp%2FD69nk89xmkKha3cx6K%2FSSg%3D&Expires=1591317779&AWSAccessKeyId=AKIAIEVMBCAT2WD3M5KQ
-
I think the problem is that
%253D
. That's the percent-encoding of%3D
, which is itself the percent-encoding of=
. Note there's a%3D
at the end of theSignature
query-parameter in the successful URL. Both signatures look like base64, which very often ends with a=
as padding. -
So it seems like we're double-encoding the URL, and as a result the decoding of it has a signature ending in
%3D
instead of in=
and the signature doesn't validate.
Looking at the code, it's clear where that's happening -- in src/utils/openLink.js
, just in the iOS branch, we call encodeURI
on the URL. That sure will turn a %3D
into a %253D
.
Unfortunately it's going to be a bit trickier than just removing that call, because it was put there to fix another bug: 66a9e9d (#3507) fixed #3315.
So it seems like we need to, in openLink
on iOS before passing the URL to SafariView.show
:
- %-encode non-ASCII characters -- that's Exception in SafariView on non-ASCII URLs ("unsupported scheme") #3315;
- but not %-encode
%
itself, instead leave it alone; - and presumably also leave alone all the other characters that
encodeURI
leaves alone, likea
andZ
and/
; - and it's not clear if we should %-encode the remaining characters that
encodeURI
affects, like"
and a bunch of other punctuation.
A key step in fixing this is going to be just end-to-end testing: make a URL filled with a ton of these characters, post it in a Zulip message, try following that link, and see what URL actually comes through.