Status-react: Unsafe characters aren't excluded or encoded in URL

[churik]

As a user, I want to tap any link on chat or type any char in webview and don't see any crashes or app hanging.

The app doesn't crash anymore, only parsing URLs in UI is wrong!

Description

_Type_: Bug
_Summary_: some of chars are unsafe and couldn't be the part of link; for such types of links other chats (Telegram, Slack) are excluded them as part of link.
In Status these chars are resolved as part of link;
If you tap on some of them, it leads to app crash or endless loop on Android.

Expected behavior

_Scenario 1:_
chars are not resolved as part of the link;
_Scenario 2_
no crashes or endless spinner

Actual behavior

_Scenario 1:_
chars are resolved as part of the link
(Status VS Telegram):

_Scenario 2_
crashes or endless spinner in Android

Reproduction

_Scenario 1:_

• Open Status
• Sign in
• Join any 1-1 or public chat
• Send one link according to table below:

|Char | Where is located | Link Examle | Link handling (Slack, Tetlgram) | Link handling (Status chat)|
|----|----|----|----|----|
|!, $, %, &, *, +, ,, ., :, ";", ?, @, \, ], ^, grave accent, {, \\\\|, }, ~, ‚, „, †, ‡, ‰, ‹, ‘, ’, “, ”, •, ›, ¡, ¢, £, ¤, ¥, ¦, §, ¨, ©, «, ¬, ®, ¯, °, ±, ², ³, ´, ¶, ·, ¸, ¹, º, », ¼, ½, ¾, ¿ , =| At the beginning, At the end, At the middle | https://example.com!; !https://example.com; https://exam!ple.com | If char is placed in the end : it is not resolved as part of link;If char is placed in the middle: it splits link in 2 parts and it is not resolved as part of link. | chars are resolved as part of link|
|%, :| At the middle | https://exam%ple.com | Slack: resolved as link;Telegram: link is split; | chars are resolved as part of link|

• pay attention how links are handled in Status

_Scenario 2:_

• Open Status on Android
• Sign in
• Join any 1-1 or public chat
• Send one link from each category (according to table below):

|Char | Where is located | Link Examle | Link handling (Status chat) | Link handling (Status webview)|
|----|----|----|----|----|
|" , {, }, grave accent, ˜ ,¨, ¯, ´, ¸, | At the beginning, At the end, At the middle | exam{ple.com | chars are resolved as part of link | no encoding;crash on Android.|
|space| At the beginning, At the end, At the middle | exam ple.com | space is not resolved as part of URL | no encoding;crash on Android (known, #4205).|
|;, [, ], ^| At the end,At the middle | https://example.com;https://examp]le.com | chars are resolved as part of link | endless spinner on Android|
|@, \| At the end | example.com@ | chars are resolved as part of link | endless spinner on Android|
|The ASCII control characters %00-%1F| At the middle | https://examp%11le.com | chars are resolved as part of link | control characters are replaced by space on Android; endless spinner is shown|

• tap on links or just paste them in Status browser

Solution

_Summary_:

• [ ] don't resolve chars !, $, %, &, *, +, ,, ., :, ";", ?, @, \, ], ^, grave accent, {, \\|, }, ~, ‚, „, †, ‡, ‰, ‹, ‘, ’, “, ”, •, ›, ¡, ¢, £, ¤, ¥, ¦, §, ¨, ©, «, ¬, ®, ¯, °, ±, ², ³, ´, ¶, ·, ¸, ¹, º, », ¼, ½, ¾, ¿ , =, " , {, }, grave accent, ˜ ,¨, ¯, ´, ¸ as part of links (according to provided examples);
• [x] encode " , {, }, grave accent, ˜ ,¨, ¯, ´, ¸ to prevent app crash;
• [ ] handle error with chars ;, [, ], ^ and The ASCII control characters %00-%1F on Android

Additional Information

• Status version: nightly 19/07/18
• Operating System: Android, iOS

Logs

• TF session for crash: https://app.testfairy.com/projects/4803622-status/builds/8526949/sessions/4396502918/?accessToken=ZsO/sUjPZjB2OhT/Z5Z1S1QehnA
• TF session for endless spinner: https://app.testfairy.com/projects/4803622-status/builds/8530385/sessions/4396751579/?accessToken=li1UMhqLmGZCf-Es0Km-iE69cgk
• Note: issue with whitespaces in URL is already created (https://github.com/status-im/status-react/issues/4205)

[status-github-bot[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[status-github-bot[bot]]

This issue has been automatically closed. Please re-open if this issue is important to you.

[churik]

Still reproducible

[asemiankevich]

some examples from desktop

[status-github-bot[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[status-github-bot[bot]]

This issue has been automatically closed. Please re-open if this issue is important to you.

[churik]

Isn't stale

[mandrigin]

@churik can you check https://github.com/status-im/status-react/pull/7592 branch? it shouldn't crash Android anymore at least...

[churik]

https://github.com/status-im/status-react/pull/7592 fixes scenario2
Scenario 1 is still relevant

[mandrigin]

So, what is left, is we need to parse URLs in the UI correctly, so if any of the invalid characters is used, it shouldn't be recognized. Let's put a bounty on it.

[bitsikka]

seems related to
https://github.com/status-im/status-react/issues/7863

[StatusSceptre]

@mandrigin adding bounty now.

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

__This issue now has a funding of 80.0 DAI (80.0 USD @ $1.0/DAI) attached to it.__

• If you would like to work on this issue you can 'start work' on the Gitcoin Issue Details page.
  
  
  
  • Want to chip in? Add your own contribution here.
  
  
  • Questions? Checkout Gitcoin Help or the Gitcoin Slack
  
  
  • $56,484.18 more funded OSS Work available on the Gitcoin Issue Explorer
  
  

[StatusSceptre]

Giving @dragosroua this one. Will switch if it becomes stale.

[dragosroua]

On it.

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

@dragosroua due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

• [x] reminder (3 days)
• [x] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

[StatusSceptre]

@dragosroua what is the status of this bounty. Please provide an update or I'll have to stop work with you and open it back up. Thank you.

[dragosroua]

Hi

Sorry for the misunderstanding, I stopped working on this a few days ago, I
thought you got the notification already. I got a notification myself
saying something about the issue being escalated to somebody (not a very
clear message, IMHO) so I inferred I have to step down from it.

Best,

Dragos Roua

Meet me in one of these places:
http://dragosroua.com
http://21.co/dragosroua

On Tue, Apr 30, 2019 at 9:53 PM StatusSceptre [e-mail]
wrote:

@dragosroua https://github.com/dragosroua what is the status of this
bounty. Please provide an update or I'll have to stop work with you and
open it back up. Thank you.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/status-im/status-react/issues/5242#issuecomment-488091621,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAMSOP5G3FIGCEU6J7OC7H3PTCPSHANCNFSM4FK7KCTA
.

[StatusSceptre]

Thanks for the update! Hope to see you on around with this and other bounties!

[StatusSceptre]

@skeptycal are you still wanting to work on this?

[skeptycal]

Hi, Sorry for the delay ... I can be working on this if you want. I didn't get a notice ...

[rachelhamlin]

@skeptycal it's been a good and long time since you showed interest on this, but I'm going through and cleaning up all our old bounties now. Would you still care to take a shot at it? Sorry to have left you hanging there.

[StatusSceptre]

Hi @scydev - slowly getting around to approving bounties. You're officially assigned, welcome to the project! Let us know if you need any help.

[ScyDev]

Thanks! I'll work on it in the coming weeks.

[gitcoinbot]

@scydev Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

• [x] reminder (3 days)
• [ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

[ScyDev]

@gitcoinbot Yes. I'll work on it in the coming weeks.

[gitcoinbot]

@scydev Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

• [x] reminder (3 days)
• [ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

[ScyDev]

@gitcoinbot Yes. I'll work on it in the coming weeks. Can this reminder be set to 1 week or so?

[gitcoinbot]

@scydev Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

• [x] reminder (3 days)
• [ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

[gitcoinbot]

@scydev Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

• [x] reminder (3 days)
• [ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

@scydev due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

• [x] reminder (3 days)
• [x] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

[ScyDev]

Yes. I'll work on it in the coming weeks. I have reported back to the dev team that I'm progressing. Can this reminder be set to 1 week or so?

[StatusSceptre]

I don't think we can customize the Gitcoin system messages. But glad you are still working!

[ScyDev]

The @gitcoinbot provides links to snooze the reminders. Check his comment above :)

[StatusSceptre]

The last place I thought to look :) I clicked on '5 days' and it took me back to the issue portal. No message or notification confirming the change, and no visible option to do so from the page:

So I guess we'll see if it worked..!

[gitcoinbot]

@scydev Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

• [x] reminder (3 days)
• [ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

[ScyDev]

@gitcoinbot Yes, working on it.

[gitcoinbot]

@scydev Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

• [x] reminder (3 days)
• [ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

[ScyDev]

@gitcoinbot Yes, working on it.

[ScyDev]

After a lot of messing around with the Nix dev environment, I'm now finally working on the issue itself. Future issues will process faster now that I've got everything up and running.

[flexsurfer]

@churik @cammellos is this issue still relevant?

[cammellos]

This is still relevant, I have tried and most of those still resolve as link.

The thing is that technically https://test!.com is a valid link, as I don't think there's a limitation on which character can be in a URL.
But http://test!.com it should though be URL encoded at some point (likely when clicking), in order to be handled correctly. so maybe that's what we should do instead of completely disabling those links.

[cammellos]

Also the parsing is not done in status-react anymore , it's here https://github.com/status-im/markdown , but if we only decide to url-encode those links when the user clicks on them, then the change is only in status-react.

[flexsurfer]

@churik could please confirm that it doesn't crash anymore? in that case we could just escape urls opened from chat

[rachelhamlin]

No crash when I try to open https://exam•ple.com on Galaxy S8 from 9/12/19 nightly. Just a page can not be loaded error, in both Status & Chrome.

[ScyDev]

I'm sorry, I have to abandon this issue. It's too much currently to learn Clojure and Go at the same time.

[churik]

@flexsurfer - yes, no crash anymore.
Sp only resolving in chat is currently the issue.

[Epicalert]

@cammellos I can't seem to find anything in this repo that links to https://github.com/status-im/markdown. Where is this library being used here?

[churik]

Unsafe characters are still not encoded in URL.
At least should be encoded for sure - after updating webview in https://github.com/status-im/status-react/pull/10090 it is not replaced by %20

cc @siphiuel

[hesterbruikman]

@churik can you confirm this is still relevant. I see the crash was fixed. The encoding not? Here's what I found on Android 1.7

" is replaced by %22 - https://exam"ple.com in browser shows as https://exam%22ple.com in browser
is not parsed as url in chat (results in https://exam ple.com)

I ask as the bounty was still open and we have an applicant on Gitcoin

[churik]

Should be rechecked again after https://github.com/status-im/status-react/issues/11469

[flexsurfer]

@churik could we recheck it ?