Allow tilde (~) in url parameters by huzaifa-99 · Pull Request #533 · Expensify/expensify-common

huzaifa-99 · 2023-05-10T22:24:41Z

Fixed Issues

Tests

Verify if ~ works in query params

Send a valid url
Verify that the sent url appears as a valid url(blue and underlined)
Verify hovering over the url shows the tooltip in Web and Desktop
Verify that Right click (Web and Desktop) or long press (mWeb and native) shows Copy Url to Clipboard context menu
Edit the sent url
Verify steps 2-4

Examples:

https://example.com/search?q=foobar~
https://example.com/search?q=~foobar
https://example.com/?~parameter~
https://example.com/?parameter~
https://example.com/?~parameter

QA

Same as tests

Screenshots/Videos

Web

Safari

web.safari.mov

Chrome

web.chrome.mov

Mobile Web - Chrome

Mweb.Chrome.mp4

Mobile Web - Safari

ios.safari.mov

Desktop

desktop.mov

iOS

Screen.Recording.2023-05-12.at.9.39.02.PM.mov

Android

Android.Native.mp4

github-actions · 2023-05-10T22:25:01Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

sobitneupane

@huzaifa-99 Can you please add test video as well?

Along with the list of valid urls, please add following steps in Tests:

Send a valid url
Verify that the sent url appears as a valid url(blue and underlined)
Verify hovering over the url shows the tooltip in Web and Desktop
Verify that Right click (Web and Desktop) or long press (mWeb and native) shows Copy Url to Clipboard context menu
Edit the sent url
Verify steps 2-4

I don't think this PR at this moment will be sufficient to solve the case of \\.

sobitneupane · 2023-05-11T11:34:32Z


 const ALLOWED_PORTS = '([1-9][0-9]{0,3}|[1-5][0-9]{4}|6[0-4][0-9]{3}|65[0-4][0-9]{2}|655[0-2][0-9]|6553[0-5])';
-const URL_PROTOCOL_REGEX = '((ht|f)tps?:\\/\\/)';
+const URL_PROTOCOL_REGEX = '((ht|f)tps?:[\\/|\\\\]{2})';


Are you sure it is the right way to do it?

I am not an expert on regex. But I think you might have to use ( instead of [ with disjunction. I think something like
(?:\\/{2}|\\\\{2}) should do the job.

I accidently removed the second \\/, mb

Apologies, maybe i didn't understand your comment properly. The \\/ was intentional. I will test (?:\\/{2}|\\\\{2}) with some urls and update.

Your change will accept https:/\google.com and https:||google.com as valid url not only https:\\google.com

Ah yes, Thank you @sobitneupane!. I have updated with (?:\\/{2}|\\\\{2}) as you suggested.

There was another place in my changes that used \\/|\\\\, fixed here

huzaifa-99 · 2023-05-11T14:14:34Z

I don't think this PR will be sufficient to solve the case of \.

@sobitneupane just to confirm on next steps here. Should I exclude this changes for the PR? and add test for ~ only?

sobitneupane · 2023-05-11T14:28:44Z

Nope. I don't mean that. I mean some other changes might be required within this repo (probably here) or in Expensify/App repo.

Did you find any issue in the PR? At the present state, I don't think PR will pass these tests.

huzaifa-99 · 2023-05-11T23:19:24Z

Didn't knew about the tooltip hover, thanks @sobitneupane!.
I tested and ~ in urls does work with tooltip but \\ has some issues, essentially \ works in query params ex: https://example.com/search?q=foo\\bar but not in path ex: https:\\example.com\search?q=foo\\bar.

Will check the required changes and post here soon.

huzaifa-99 · 2023-05-12T12:56:54Z

@sobitneupane this looks like an issue with the api. Api will recognise backslashes in some areas of the url as valid while not matching others.

In my tests, I found these urls (with backslashes) are recognised as valid by the api (and these work with tooltips/pressables)

# In query params
https://www.example.com/path/to/page.html?q=foo\\bar 
https://www.example.com/path/to/page.html?q=foo\\bar&y=john\\doe

# In path
https://www.example.com/path/to/page.html\
https://www.example.com/path/to\page.html\
https://www.example.com/path/to\page.html/
https://www.example.com/path\to\page.html/
https://www.example.com/path\to/page.html/

while these ones fail

# Trailing backslash after the TLD part
https://www.example.com/

# Backslashes after the protocol
https:\\www.example.com/

So summarising from this, API recognises backslashes when in query params or the path but not in the domain part.

Why it fails

With backslashes after the protocol or the trailing backslash after the TLD, the api responds like this. Notice that the anchor tag in the html key has no href prop.

For other urls the api will add the href prop

I also tried turning off internet. The failing urls (with backslash in domain part and trailing backslash after TLD) work when not connected to the internet, when connection is restored the api updates and it breaks the links for urls that have backslash after protocol part or the trailing backslash after TLD

Offline test

Screen.Recording.2023-05-12.at.5.51.47.PM.mov

Demo to prove above statements

Demo.URL.Backslashes-compressed.mp4

Also just for clarification, I used the same changes in this PR.

@PauloGasparSv @sobitneupane Can you please guide me on the next steps? Should we wait/hold on a change in API or just proceed with the changes in this PR?

huzaifa-99 · 2023-05-12T13:00:12Z

I will add tests soon after your response @sobitneupane

sobitneupane · 2023-05-12T13:52:11Z

# Backslashes after the protocol
https:\\www.example.com/

With backslashes after the protocol or the trailing backslash after the TLD, the api responds like this. Notice that the anchor tag in the html key has no href prop.

I also tried turning off internet. The failing urls (with backslash in domain part and trailing backslash after TLD) work when not connected to the internet, when connection is restored the api updates and it breaks the links for urls that have backslash after protocol part

Oh yes. Looks like backend is returning anchor without href.

cc: @PauloGasparSv

Update: I just noticed that https:\\google.com is not considered as url in github. It will show it as https:\google.com.
Even slack doesn't consider it as url. It matches our current behavior.
Discord also doesn't consider it as url.

huzaifa-99 · 2023-05-12T14:13:25Z

Update: I just noticed that https:\google.com is not considered as url in github. It will show it as https:\google.com.
Even slack doesn't consider it as url. It matches our current behavior.

@sobitneupane I found another case

Trailing backslash after TLD works in github but not in slack ex: https://google.com\

huzaifa-99 · 2023-05-12T14:14:14Z

It matches our current behavior.

@sobitneupane just to confirm if i should add tests with \?

sobitneupane · 2023-05-12T14:15:46Z

Let's wait for @PauloGasparSv's opinion on #533 (comment)

huzaifa-99 · 2023-05-12T14:39:06Z

Sure thanks!.

Just to add to the convo, i noticed that slack doesn't allow \ anywhere in the url while Github allows it after the TLD. So https://google.com\?q=foo\\bar works in github but not in slack.

If we want to follow slack then we can remove the backslash changes
else if we want to follow Github then we need to make a small change in API to allow the trailing \ after the TLD ex: https://google.com\ and in the pr changes we need to revert the \ allowed in protocol part of the url here

PauloGasparSv · 2023-05-12T14:52:41Z

I'm still trying to find time today to take a look at the API and into this.

If we want to follow slack then we can remove the backslash changes

That's really good to know! Whatsapp does allow \ in the URLS though.

Taking in consideration that:

The original issue's intent which isn't specifically to make \ and \\ work but instead only ~ (this was an optional change we wanted to add)
Slack and GH don't recognize that / as part of the URL so IMO it's ok to match that behavior (despite Whatsapp allowing it).
The API error. I don't have a lot of time to look into this : /

I think we shouldn't bother with making \ work and only fix ~ here.

Does that sound good @sobitneupane @huzaifa-99?
That means we'll not add tests for / and we'll remove it from the regex changes here right?

PauloGasparSv · 2023-05-12T14:53:12Z

Btw, thks for all the work here so far!

huzaifa-99 · 2023-05-12T14:56:30Z

Thank you for the input @PauloGasparSv

I think we shouldn't bother with making \ work and only fix ~ here.
Does that sound good @sobitneupane @huzaifa-99?
That means we'll not add tests for / and we'll remove it from the regex changes here right?

Sounds good to me!
Yes we will have to remove the regex changes for \. I will add tests for ~ soon.

sobitneupane · 2023-05-12T16:12:10Z

@huzaifa-99 Please let me know when it is ready for review. Looks like you need to remove \\ related stuffs and update your Test steps to include test steps mentioned here.

huzaifa-99 · 2023-05-12T16:44:53Z

@sobitneupane tests are added for all platforms, PR is ready.

Please let me know if any issues.

huzaifa-99 · 2023-05-15T07:19:17Z

@sobitneupane bump ^

sobitneupane

Changes look good and test well.

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

huzaifa-99 · 2023-05-15T11:06:06Z

Thank you @sobitneupane 🎉

PauloGasparSv

LGTM, tested changes on Web locally!

Will test and evidences to each platform on the App P.R.

PauloGasparSv · 2023-05-15T14:39:26Z

Oh the first commit here isn't signed so I can't merge this! Checking what to do here

huzaifa-99 · 2023-05-15T16:23:26Z

Ah apologies @PauloGasparSv.

Should I create a new PR and close this one out. I could also force push a fresh commit and remove existing ones? These are not good enough options but not sure on what's the next step.

Update: I just found out there is an option to sign previous commits but never done it. Will try and update here soon

PauloGasparSv · 2023-05-15T16:34:51Z

Don't worry @huzaifa-99!

Update: I just found out there is an option to sign previous commits but never done it. Will try and update here soon

Thks for that!!!! I also searched a bit and asked for help internally, do you mind trying to:

Run git -c core.editor=true rebase --exec 'git commit --amend --no-edit -n -S' -i $COMMIT_HASH replacing $COMMIT_HASH with your commit hash. (Found many similar solutions online but the team recommended this one)
Force push (Thks for worrying about this but it's fine do to that this time)

Can you please do that and ping us @huzaifa-99?

huzaifa-99 · 2023-05-15T17:35:05Z

@PauloGasparSv Thank you (and the team) for the solution.

I tried the command, it worked but I had to use the hash of previous commit (the commit before my unsigned commit). Anyways a lesson learned and got to know an existing commit can be signed as well. Thanks again!

huzaifa-99 · 2023-05-16T08:24:50Z

@sobitneupane @PauloGasparSv I am not sure if i should make a PR in Expensify/App as there is already one Expensify/App#18994 that applies my commits as well.

Please let me know on the next steps. Thanks!

sobitneupane · 2023-05-16T08:29:42Z

I don't think you should create another PR if there is already one which includes commit latest than yours.

cc: @PauloGasparSv

huzaifa-99 · 2023-05-16T10:06:33Z

sure, Thank you!

huzaifa-99 requested a review from a team as a code owner May 10, 2023 22:24

melvin-bot Bot requested review from PauloGasparSv and removed request for a team May 10, 2023 22:25

huzaifa-99 mentioned this pull request May 10, 2023

[$1000] Inconsistent autolink parsing on parameter with styling characters (underscores and tildes) Expensify/App#18564

Closed

6 tasks

This comment was marked as off-topic.

Sign in to view

sobitneupane reviewed May 11, 2023

View reviewed changes

huzaifa-99 requested a review from sobitneupane May 12, 2023 12:59

PauloGasparSv assigned huzaifa-99 May 12, 2023

huzaifa-99 changed the title ~~Allow tilde (~) and double-backslash (\\) in urls~~ Allow tilde (~) in url parameters May 12, 2023

sobitneupane approved these changes May 15, 2023

View reviewed changes

PauloGasparSv approved these changes May 15, 2023

View reviewed changes

huzaifa-99 force-pushed the eApp/18565-allow-tilde-and-backslash-in-urls branch from a683fca to 5318124 Compare May 15, 2023 17:30

huzaifa-99 and others added 4 commits May 15, 2023 22:30

Allow tilde (~) and double-backslash (\\) in urls

2c9aba5

Updated url protocol regex

dab4fb5

Fix url path regex

6a32b85

Dont allow backslash (\\) in url

2f762d9

huzaifa-99 force-pushed the eApp/18565-allow-tilde-and-backslash-in-urls branch from 5318124 to 2f762d9 Compare May 15, 2023 17:31

PauloGasparSv merged commit 0685262 into Expensify:main May 15, 2023

PauloGasparSv mentioned this pull request May 16, 2023

fix: update version expensify common repo Expensify/App#18994

Merged

57 tasks

MelvinBot mentioned this pull request Jun 20, 2023

[Snyk] Fix for 1 vulnerabilities #552

Closed

Uh oh!

Conversation

huzaifa-99 commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixed Issues

Tests

QA

Screenshots/Videos

Safari

Chrome

Uh oh!

github-actions Bot commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as off-topic.

This comment was marked as off-topic.

sobitneupane left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sobitneupane May 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huzaifa-99 May 11, 2023

Choose a reason for hiding this comment

Uh oh!

huzaifa-99 May 12, 2023

Choose a reason for hiding this comment

Uh oh!

sobitneupane May 12, 2023

Choose a reason for hiding this comment

Uh oh!

huzaifa-99 May 12, 2023

Choose a reason for hiding this comment

Uh oh!

huzaifa-99 May 12, 2023

Choose a reason for hiding this comment

Uh oh!

huzaifa-99 commented May 11, 2023

Uh oh!

sobitneupane commented May 11, 2023

Uh oh!

huzaifa-99 commented May 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

huzaifa-99 commented May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why it fails

Uh oh!

huzaifa-99 commented May 12, 2023

Uh oh!

sobitneupane commented May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

huzaifa-99 commented May 12, 2023

Uh oh!

huzaifa-99 commented May 12, 2023

Uh oh!

sobitneupane commented May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

huzaifa-99 commented May 12, 2023

Uh oh!

PauloGasparSv commented May 12, 2023

Uh oh!

PauloGasparSv commented May 12, 2023

Uh oh!

huzaifa-99 commented May 12, 2023

Uh oh!

sobitneupane commented May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

huzaifa-99 commented May 12, 2023

Uh oh!

huzaifa-99 commented May 15, 2023

Uh oh!

sobitneupane left a comment

Choose a reason for hiding this comment

Screenshots/Videos

huzaifa-99 commented May 10, 2023 •

edited

Loading

github-actions Bot commented May 10, 2023 •

edited

Loading

sobitneupane left a comment •

edited

Loading

sobitneupane May 11, 2023 •

edited

Loading

huzaifa-99 commented May 11, 2023 •

edited

Loading

huzaifa-99 commented May 12, 2023 •

edited

Loading

sobitneupane commented May 12, 2023 •

edited

Loading

sobitneupane commented May 12, 2023 •

edited

Loading

sobitneupane commented May 12, 2023 •

edited

Loading

huzaifa-99 commented May 15, 2023 •

edited

Loading