fix: added protocols to the url along with correct port range #519

allroundexperts · 2023-04-18T08:13:57Z

Fixed Issues

$ Expensify/App#17205

Tests

This is to be tested with Expensify/App#17564

QA

No QA

github-actions · 2023-04-18T08:14:16Z

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

allroundexperts · 2023-04-18T08:16:08Z

I have read the CLA Document and I hereby sign the CLA

allroundexperts · 2023-04-18T08:16:28Z

recheck

puneetlath · 2023-04-18T15:45:20Z

This makes sense to me. Let's add tests for it though.

thesahindia · 2023-04-18T18:00:06Z

@allroundexperts, is this ready for the review?

allroundexperts · 2023-04-18T18:01:19Z

@allroundexperts, is this ready for the review?

Sorry, forgot to make this ready for review. Although I have to write tests as suggested by @puneetlath.

allroundexperts · 2023-04-18T19:44:30Z

@puneetlath Added tests. Also exported URL_REGEX_WITH_REQUIRED_PROTOCOL that requires protocol to be present in the url as suggested by @MonilBhavsar

MonilBhavsar · 2023-04-19T09:56:10Z

lib/Url.js

 const addEscapedChar = reg => `(?:${reg}|&(?:amp|quot|#x27);)`;
 const URL_PATH_REGEX = `(?:${addEscapedChar('[.,=(+$!*]')}?\\/${addEscapedChar('[-\\w$@.+!*:(),=%~]')}*${addEscapedChar('[-\\w~@:%)]')}|\\/)*`;
 const URL_PARAM_REGEX = `(?:\\?${addEscapedChar('[-\\w$@.+!*()\\/,=%{}:;\\[\\]\\|_]')}*)?`;
 const URL_FRAGMENT_REGEX = `(?:#${addEscapedChar('[-\\w$@.+!*()[\\],=%;\\/:~]')}*)?`;
 const URL_REGEX = `(${URL_WEBSITE_REGEX}${URL_PATH_REGEX}(?:${URL_PARAM_REGEX}|${URL_FRAGMENT_REGEX})*)`;

+const URL_REGEX_WITH_REQUIRED_PROTOCOL = URL_REGEX.replace(`${URL_PROTOCOL_REGEX}?`, URL_PROTOCOL_REGEX);


We don't consider SMTP a valid protocol in bank account flow

@MonilBhavsar Updated.

Can we also add this to automated test

thesahindia

Looks good!

allroundexperts · 2023-04-20T06:42:23Z

@MonilBhavsar @puneetlath Kind bump for review.

__tests__/URL-test.js

MonilBhavsar · 2023-04-21T09:23:07Z

__tests__/URL-test.js

+        expect(regexToTest.test('https://google.com:02')).toBeFalsy();
+        expect(regexToTest.test('https://google.com:65536')).toBeFalsy();


Not sure, if we want to add under this test suite. I think this goes better under invalid URL's?

I'm not sure if I understand you here. All the failing cases are in its own it block. Do you want me to move these in there own describe block?

Hmm, I think in this it block, we want to test URL's that doesn't have a protocol.
These examples are falsy because of invalid ports but not because they have invalid protocol.

I think I've written this differently. In the first describe block, I'm checking for both invalid ports and protocol because the regex used there expects a mandatory protocol. For the second describe block, I'm not check the protocol because it becomes optional. Does that make sense?

MonilBhavsar · 2023-04-21T09:32:55Z

lib/Url.js

@@ -1,16 +1,22 @@
 import TLD_REGEX from './tlds';

-const URL_WEBSITE_REGEX = `(https?:\\/\\/)?((?:www\\.)?[-a-z0-9]+?\\.)+(?:${TLD_REGEX})(?:\\:\\d{2,4}|\\b|(?=_))`;
+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])';


Just curious, could you please explain the range of ports covered here?

The valid port range is from 1-65535

MonilBhavsar · 2023-04-21T09:37:21Z

lib/Url.js

 const addEscapedChar = reg => `(?:${reg}|&(?:amp|quot|#x27);)`;
 const URL_PATH_REGEX = `(?:${addEscapedChar('[.,=(+$!*]')}?\\/${addEscapedChar('[-\\w$@.+!*:(),=%~]')}*${addEscapedChar('[-\\w~@:%)]')}|\\/)*`;
 const URL_PARAM_REGEX = `(?:\\?${addEscapedChar('[-\\w$@.+!*()\\/,=%{}:;\\[\\]\\|_]')}*)?`;
 const URL_FRAGMENT_REGEX = `(?:#${addEscapedChar('[-\\w$@.+!*()[\\],=%;\\/:~]')}*)?`;
 const URL_REGEX = `(${URL_WEBSITE_REGEX}${URL_PATH_REGEX}(?:${URL_PARAM_REGEX}|${URL_FRAGMENT_REGEX})*)`;

+const URL_REGEX_WITH_REQUIRED_PROTOCOL = URL_REGEX.replace(`${URL_PROTOCOL_REGEX}?`, URL_PROTOCOL_REGEX);


Updated spaces

Removed more spaces

allroundexperts · 2023-04-24T12:38:33Z

@MonilBhavsar @puneetlath Can you please review this so that the ticket can move forward?

MonilBhavsar

Code looks good to me! I want to test somethings. But my app build is down, so I'll test and approve it before EOD

MonilBhavsar

I just did a quick testing. We should have identified the whole URL including protocol, no?

allroundexperts · 2023-04-25T19:51:20Z

I just did a quick testing. We should have identified the whole URL including protocol, no?

@MonilBhavsar Are you sure that you tested this correctly? Here is how it looks on my side:

Screen.Recording.2023-04-26.at.12.50.24.AM.mov

puneetlath

It looks good to me but would like @MonilBhavsar to confirm as well. Thanks!

MonilBhavsar · 2023-04-26T04:53:45Z

Hmm, Looks the lib is linked to project correctly but let me double check.

MonilBhavsar · 2023-04-26T05:01:18Z

Sorry, i think I had issue linking because of conflicting node versions
Looks good and works well. Thanks for working on this!

fix: added protocols to the url along with correct port range

1c05677

allroundexperts mentioned this pull request Apr 18, 2023

fix: standardise the usage of url validator Expensify/App#17564

Merged

56 tasks

puneetlath self-requested a review April 18, 2023 15:45

allroundexperts marked this pull request as ready for review April 18, 2023 18:00

allroundexperts requested a review from a team as a code owner April 18, 2023 18:00

melvin-bot bot requested review from MonilBhavsar and removed request for a team April 18, 2023 18:01

feat: add tests for url validation

464b988

MonilBhavsar reviewed Apr 19, 2023

View reviewed changes

feat: remove SMTP from the allowed protocols

83d301f

allroundexperts requested a review from MonilBhavsar April 19, 2023 10:23

thesahindia previously approved these changes Apr 19, 2023

View reviewed changes

MonilBhavsar reviewed Apr 21, 2023

View reviewed changes

Update URL-test.js

61f6c7f

Updated spaces

allroundexperts dismissed thesahindia’s stale review via 61f6c7f April 21, 2023 09:47

allroundexperts added 2 commits April 21, 2023 15:02

Update URL-test.js

ed7cfd3

Removed more spaces

fix: update unit test to include smtp failure

498cd21

allroundexperts requested a review from MonilBhavsar April 24, 2023 09:48

MonilBhavsar reviewed Apr 24, 2023

View reviewed changes

puneetlath previously approved these changes Apr 24, 2023

View reviewed changes

MonilBhavsar reviewed Apr 25, 2023

View reviewed changes

Merge branch 'main' into fix-17205

9c24c3f

allroundexperts dismissed puneetlath’s stale review via 9c24c3f April 25, 2023 19:59

puneetlath approved these changes Apr 25, 2023

View reviewed changes

MonilBhavsar approved these changes Apr 26, 2023

View reviewed changes

MonilBhavsar merged commit 3cdaa94 into Expensify:main Apr 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: added protocols to the url along with correct port range #519

fix: added protocols to the url along with correct port range #519

allroundexperts commented Apr 18, 2023 •

edited

Loading

github-actions bot commented Apr 18, 2023 •

edited

Loading

allroundexperts commented Apr 18, 2023

allroundexperts commented Apr 18, 2023

puneetlath commented Apr 18, 2023

thesahindia commented Apr 18, 2023

allroundexperts commented Apr 18, 2023

allroundexperts commented Apr 18, 2023

MonilBhavsar Apr 19, 2023

allroundexperts Apr 19, 2023

MonilBhavsar Apr 21, 2023

MonilBhavsar Apr 21, 2023

allroundexperts Apr 21, 2023

thesahindia left a comment

allroundexperts commented Apr 20, 2023

MonilBhavsar Apr 21, 2023

allroundexperts Apr 21, 2023

MonilBhavsar Apr 21, 2023

allroundexperts Apr 21, 2023

MonilBhavsar Apr 21, 2023

allroundexperts Apr 21, 2023

MonilBhavsar Apr 21, 2023

allroundexperts commented Apr 24, 2023

MonilBhavsar left a comment

MonilBhavsar left a comment

allroundexperts commented Apr 25, 2023

puneetlath left a comment

MonilBhavsar commented Apr 26, 2023

MonilBhavsar commented Apr 26, 2023

		expect(regexToTest.test('https://google.com:02')).toBeFalsy();
		expect(regexToTest.test('https://google.com:65536')).toBeFalsy();

fix: added protocols to the url along with correct port range #519

fix: added protocols to the url along with correct port range #519

Conversation

allroundexperts commented Apr 18, 2023 • edited Loading

Fixed Issues

Tests

QA

github-actions bot commented Apr 18, 2023 • edited Loading

allroundexperts commented Apr 18, 2023

allroundexperts commented Apr 18, 2023

puneetlath commented Apr 18, 2023

thesahindia commented Apr 18, 2023

allroundexperts commented Apr 18, 2023

allroundexperts commented Apr 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thesahindia left a comment

Choose a reason for hiding this comment

allroundexperts commented Apr 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allroundexperts commented Apr 24, 2023

MonilBhavsar left a comment

Choose a reason for hiding this comment

MonilBhavsar left a comment

Choose a reason for hiding this comment

allroundexperts commented Apr 25, 2023

puneetlath left a comment

Choose a reason for hiding this comment

MonilBhavsar commented Apr 26, 2023

MonilBhavsar commented Apr 26, 2023

allroundexperts commented Apr 18, 2023 •

edited

Loading

github-actions bot commented Apr 18, 2023 •

edited

Loading