Any links pasted into e.cash that includes a colon `:` break causing only the portion before the colon to be hyperlinked #4105

isagoico · 2021-07-16T12:25:51Z

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

Log in to e.cash and navigate to a conversation
Paste and send these links that contain colons:
A. https://www.google.com/maps/place/%E9%9D%92%E5%B3%B6%E9%80%A3%E7%B5%A1%E8%88%B9%E4%B9%97%E5%A0%B4/@33.7363156,132.4877213,17.78z/data=!4m5!3m4!1s0x3545615c8c65bf7f:0xb89272c1a705a33f!8m2!3d33.7366776!4d132.4878843
B. https://www.google.com/maps/place/The+Flying+Saucer/@42.4043314,-86.2742418,15z/data=!4m5!3m4!1s0x0:0xe28f6108670216bc!8m2!3d42.4043316!4d-86.2742121
Notice that the links are cut off after the colon and therefore don't hyperlink correctly.

Expected Result:

Links including colons should be correctly hyperlinked. In this example when clicked, should redirect to:
A. The cat island in Japan (and yes, it's an island full of cats)
B. The Flying Saucer in South Haven, MI

Actual Result:

Both links are not hyperlinked correctly and cut off after the colon.

Workaround:

User has to manually select the full link and paste it in the URL bar.

Platform:

Where is this issue occurring?

Web ✔️
iOS ✔️
Android ✔️
Desktop App ✔️
Mobile Web ✔️

Version Number: 1.0.79-0

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos:
Most Google Maps links are affected by this because they contain colons in the URL.
Please check the screenshot in Actual result.
I will try different links from other sites and, if I find more affected by this, will post the ones that have the same issue in comments.

Upwork Post: https://www.upwork.com/jobs/~017aff792b5b4f4a79

View all open jobs on Upwork

From @mallenexpensify https://expensify.slack.com/archives/C01GTK53T8Q/p1626278479245000

Links pasted into chats sometimes get cut off and the full link isn't properly hyperlinked in a message, especially for Google Maps links.

The text was updated successfully, but these errors were encountered:

MelvinBot · 2021-07-16T12:25:54Z

Triggered auto assignment to @Gonals (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

MelvinBot · 2021-07-19T12:48:04Z

Triggered auto assignment to @trjExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

Gonals · 2021-07-19T12:49:10Z

We'll probably have to tweak the markdown a bit to get this to work correctly.
Sending to the pool!

trjExpensify · 2021-07-19T18:33:51Z

Looking at the screenshot, the common theme seems to be the colon : in the URL. Anything after that gets cut off. I tested that theory and it seems to hold up:

With that, I've updated the OP of the issue.

MelvinBot · 2021-07-19T18:39:28Z

Triggered auto assignment to @marcaaron (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

parasharrajat · 2021-07-19T19:04:39Z

Proposal

I just adjusted the Path Regex to this

                                     Added this V 
const URL_PATH_REGEX = '(?:\\/[-\\w$@.&+!*"\'(),:=%~]*[-\\w~@:%)]|\\/)*';

And it is working fine and the whole test suite passed. It fixes the main issue.

There is another issue here #4105 (comment). which I will fix in the upcoming steps.

Explanation

We escape matching strings before applying scanning rules. Escaping converts a few chars as follows:

 var escapeMap = {
    '&': '&amp;',
    '<': '&lt;',
    '>': '&gt;',
    '"': '&quot;',
    "'": '&#x27;',
    '`': '&#x60;'
  };

Our Url regex didn't encounter escaped Symbols thus parsing is failing for '.

Fix

We have to update the URL regex to match the escaped Symbols as well.

For example, Path regex would be converted to

const URL_PATH_REGEX = '(?:\\/(?:[-\\w$@.&+!*"\'(),:=%~]|&amp;|&quot;|&#x27;)*[-\\w~@:%)]|\\/)*';

This fixes the other issue.

trjExpensify · 2021-07-19T19:34:22Z

Interesting, what's the difference with what looks like a : in the list already? (right after the ?)

@mallenexpensify seems to have found this one as well, but the single quotation mark (') also seems included in the Path regex already? 🤔

parasharrajat · 2021-07-19T21:03:06Z

Ok, I found the issue with '. We are escaping the string before matching the URLs using Str.safeEscape. It escapes

  var escapeMap = {
    '&': '&amp;',
    '<': '&lt;',
    '>': '&gt;',
    '"': '&quot;',
    "'": '&#x27;',
    '`': '&#x60;'
  };

So at the time of matching ' will be ' that's why the match fails.

rdjuric · 2021-07-19T21:05:59Z

Interesting, what's the difference with what looks like a : in the list already? (right after the ?)

The : in ?: is part of the regex syntax, it's used to define a non-capturing group.

parasharrajat · 2021-07-19T21:08:07Z

Again, I think escaping the URL with htmlentities will make it incorrect. so it's is better to unescape it before matching the URLs.

Edit:
We can't unescape it at this step, It will become impossible to escape it back after scanning or URLs. Escaping is needed for other selectors. If we escape it back then our converted markup will be escaped as well which is wrong.

So I have found the fix for that as well and we may have to update a few of regex to match the escaped version of

  var escapeMap = {
    '&': '&amp;',
    '<': '&lt;',
    '>': '&gt;',
    '"': '&quot;',
    "'": '&#x27;',
    '`': '&#x60;'
  };

which will fix future issues. Updating proposal...

parasharrajat · 2021-07-20T16:56:44Z

PR is up Expensify/expensify-common#396.

MelvinBot · 2021-07-23T19:02:49Z

@trjExpensify, @marcaaron Whoops! This issue is 2 days overdue. Let's get this updated quick!

trjExpensify · 2021-07-26T09:20:07Z

This is done, closing it out. Reopen if I missed something, Gents!

isagoico · 2021-11-04T01:28:42Z

Reopening this issue since @mallenexpensify was able to reproduce here https://expensify.slack.com/archives/C01GTK53T8Q/p1635954172411200

mallenexpensify · 2021-11-04T15:22:47Z

I'm thinking, for issues like this that are likely to be recurring as we find variations, we shouldn't continue to reopen this one but open a new one each time. In the new issue, we can link back to this, gather examples then decide if/when we want a contributor to work on.

@marcaaron @trjExpensify @parasharrajat , y'all agree?

trjExpensify · 2021-11-04T16:05:28Z

Agreed!

…

On Thu, Nov 4, 2021 at 3:22 PM Matt Allen ***@***.***> wrote: I'm thinking, for issues like this that are likely to be recurring as we find variations, we shouldn't continue to reopen this one but open a new one each time. In the new issue, we can link back to this, gather examples then decide if/when we want a contributor to work on. @marcaaron <https://github.com/marcaaron> @trjExpensify <https://github.com/trjExpensify> @parasharrajat <https://github.com/parasharrajat> , y'all agree? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4105 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD3246ICBYI4PVAACJFQO4TUKKXOFANCNFSM5APPJT4Q> .

-- *Tom Rhys Jones * *Expensify*

parasharrajat · 2021-11-04T16:25:45Z

I think parsing links is really giving us a hard time and we are hitting limitations of js regex. Should we reconsider md parser for links?

No preference @mallenexpensify

mallenexpensify · 2021-11-04T17:21:38Z

Also... here's the link
https://www.google.com/maps/place/Poster+Auctions+International,+Inc./@40.7384572,-73.9942151,20.17z/data=!4m13!1m7!3m6!1s0x89c259a2c5fdee77:0x1439488db9133bae!2s26+W+17th+St,+New+York,+NY+10011!3b1!8m2!3d40.7383679!4d-73.993907!3m4!1s0x89c259a2c438eb63:0x115ba65a3675338b!8m2!3d40.7384793!4d-73.9937764

isagoico · 2021-11-04T17:32:01Z

I agree with @mallenexpensify opening new tickets for other scenarios are easier to follow than reopening old ones. Let me know if we should start with this variation and create another ticket.

Also, able to reproduce with Matt's link

marcaaron · 2021-11-04T18:10:01Z

I'm thinking, for issues like this that are likely to be recurring as we find variations, we shouldn't continue to reopen this one but open a new one each time. In the new issue, we can link back to this, gather examples then decide if/when we want a contributor to work on.

Sure. This link maybe looks like it's broken for another reason so we can close this.

mallenexpensify · 2021-11-04T18:31:22Z

@isagoico can you create a brand new issue and include a link back to this? Thanks

isagoico · 2021-11-04T18:50:31Z

Issue created here 🔝

isagoico added Engineering Daily KSv2 labels Jul 16, 2021

MelvinBot assigned Gonals Jul 16, 2021

Gonals removed their assignment Jul 19, 2021

Gonals added Improvement Item broken or needs improvement. Weekly KSv2 External Added to denote the issue can be worked on by a contributor labels Jul 19, 2021

MelvinBot assigned trjExpensify Jul 19, 2021

MelvinBot removed the Weekly KSv2 label Jul 19, 2021

Gonals added Weekly KSv2 and removed External Added to denote the issue can be worked on by a contributor Daily KSv2 labels Jul 19, 2021

Gonals added the External Added to denote the issue can be worked on by a contributor label Jul 19, 2021

MelvinBot added Daily KSv2 and removed Weekly KSv2 labels Jul 19, 2021

trjExpensify changed the title ~~Some links pasted into e.cash are broken and not properly hyperlinked~~ Any links pasted into e.cash that includes a colon : break causing only the portion before the colon to be hyperlinked Jul 19, 2021

trjExpensify added the Exported label Jul 19, 2021

MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 19, 2021

MelvinBot assigned marcaaron Jul 19, 2021

trjExpensify assigned parasharrajat Jul 20, 2021

trjExpensify removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 20, 2021

parasharrajat mentioned this issue Jul 20, 2021

fixed url parsing for escaped chars Expensify/expensify-common#396

Merged

parasharrajat mentioned this issue Jul 20, 2021

[No QA] Update e-common #4142

Merged

MelvinBot added the Overdue label Jul 23, 2021

trjExpensify closed this as completed Jul 26, 2021

MelvinBot removed the Overdue label Jul 26, 2021

isagoico reopened this Nov 4, 2021

MelvinBot added the Overdue label Nov 4, 2021

marcaaron added the Weekly KSv2 label Nov 4, 2021

MelvinBot removed the Overdue label Nov 4, 2021

marcaaron removed the Daily KSv2 label Nov 4, 2021

marcaaron closed this as completed Nov 4, 2021

isagoico mentioned this issue Nov 4, 2021

Google location link is being cut off and placed in a new line #6216

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Any links pasted into e.cash that includes a colon `:` break causing only the portion before the colon to be hyperlinked #4105

Any links pasted into e.cash that includes a colon `:` break causing only the portion before the colon to be hyperlinked #4105

isagoico commented Jul 16, 2021 •

edited by trjExpensify

Loading

MelvinBot commented Jul 16, 2021

MelvinBot commented Jul 19, 2021

Gonals commented Jul 19, 2021

trjExpensify commented Jul 19, 2021

MelvinBot commented Jul 19, 2021

parasharrajat commented Jul 19, 2021 •

edited

Loading

trjExpensify commented Jul 19, 2021

parasharrajat commented Jul 19, 2021 •

edited

Loading

rdjuric commented Jul 19, 2021

parasharrajat commented Jul 19, 2021 •

edited

Loading

parasharrajat commented Jul 20, 2021

MelvinBot commented Jul 23, 2021

trjExpensify commented Jul 26, 2021

isagoico commented Nov 4, 2021

mallenexpensify commented Nov 4, 2021

trjExpensify commented Nov 4, 2021 via email

parasharrajat commented Nov 4, 2021 •

edited

Loading

mallenexpensify commented Nov 4, 2021

isagoico commented Nov 4, 2021

marcaaron commented Nov 4, 2021

mallenexpensify commented Nov 4, 2021

isagoico commented Nov 4, 2021

Any links pasted into e.cash that includes a colon : break causing only the portion before the colon to be hyperlinked #4105

Any links pasted into e.cash that includes a colon : break causing only the portion before the colon to be hyperlinked #4105

Comments

isagoico commented Jul 16, 2021 • edited by trjExpensify Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platform:

MelvinBot commented Jul 16, 2021

MelvinBot commented Jul 19, 2021

Gonals commented Jul 19, 2021

trjExpensify commented Jul 19, 2021

MelvinBot commented Jul 19, 2021

parasharrajat commented Jul 19, 2021 • edited Loading

Proposal

Explanation

Fix

trjExpensify commented Jul 19, 2021

parasharrajat commented Jul 19, 2021 • edited Loading

rdjuric commented Jul 19, 2021

parasharrajat commented Jul 19, 2021 • edited Loading

parasharrajat commented Jul 20, 2021

MelvinBot commented Jul 23, 2021

trjExpensify commented Jul 26, 2021

isagoico commented Nov 4, 2021

mallenexpensify commented Nov 4, 2021

trjExpensify commented Nov 4, 2021 via email

parasharrajat commented Nov 4, 2021 • edited Loading

mallenexpensify commented Nov 4, 2021

isagoico commented Nov 4, 2021

marcaaron commented Nov 4, 2021

mallenexpensify commented Nov 4, 2021

isagoico commented Nov 4, 2021

Any links pasted into e.cash that includes a colon `:` break causing only the portion before the colon to be hyperlinked #4105

Any links pasted into e.cash that includes a colon `:` break causing only the portion before the colon to be hyperlinked #4105

isagoico commented Jul 16, 2021 •

edited by trjExpensify

Loading

parasharrajat commented Jul 19, 2021 •

edited

Loading

parasharrajat commented Jul 19, 2021 •

edited

Loading

parasharrajat commented Jul 19, 2021 •

edited

Loading

parasharrajat commented Nov 4, 2021 •

edited

Loading