Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Apr 26, 2023
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1c05677
fix: added protocols to the url along with correct port range
allroundexperts Apr 18, 2023
464b988
feat: add tests for url validation
allroundexperts Apr 18, 2023
83d301f
feat: remove SMTP from the allowed protocols
allroundexperts Apr 19, 2023
61f6c7f
Update URL-test.js
allroundexperts Apr 21, 2023
ed7cfd3
Update URL-test.js
allroundexperts Apr 21, 2023
498cd21
fix: update unit test to include smtp failure
allroundexperts Apr 21, 2023
9c24c3f
Merge branch 'main' into fix-17205
allroundexperts Apr 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions __tests__/URL-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import {URL_REGEX_WITH_REQUIRED_PROTOCOL, URL_REGEX} from '../lib/Url';


describe('Mandatory protocol for URL', () => {
it('correctly tests valid urls', () => {
const regexToTest = new RegExp(`^${URL_REGEX_WITH_REQUIRED_PROTOCOL}$`, 'i');
expect(regexToTest.test('https://google.com/')).toBeTruthy();
expect(regexToTest.test('http://google.com/')).toBeTruthy();
expect(regexToTest.test('ftp://google.com/')).toBeTruthy();

expect(regexToTest.test('https://we.are.expensify.com/how-we-got-here')).toBeTruthy();

expect(regexToTest.test('https://google.com:12')).toBeTruthy();
expect(regexToTest.test('https://google.com:65535')).toBeTruthy();

expect(regexToTest.test('https://google.com:65535/path/my')).toBeTruthy();
});

it('correctly tests invalid urls', () => {
const regexToTest = new RegExp(`^${URL_REGEX_WITH_REQUIRED_PROTOCOL}$`, 'i');
expect(regexToTest.test('google.com')).toBeFalsy();

expect(regexToTest.test('https://google.com:02')).toBeFalsy();
expect(regexToTest.test('https://google.com:65536')).toBeFalsy();
Comment on lines +17 to +18
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

});
});


describe('Optional protocol for URL', () => {
it('correctly tests valid urls', () => {
const regexToTest = new RegExp(`^${URL_REGEX}$`, 'i');
expect(regexToTest.test('google.com/')).toBeTruthy();
expect(regexToTest.test('https://google.com/')).toBeTruthy();
expect(regexToTest.test('ftp://google.com/')).toBeTruthy();

expect(regexToTest.test('we.are.expensify.com/how-we-got-here')).toBeTruthy();

expect(regexToTest.test('google.com:12')).toBeTruthy();
expect(regexToTest.test('google.com:65535')).toBeTruthy();

expect(regexToTest.test('google.com:65535/path/my')).toBeTruthy();
});

it('correctly tests invalid urls', () => {
const regexToTest = new RegExp(`^${URL_REGEX}$`, 'i');

expect(regexToTest.test('google.com:02')).toBeFalsy();
expect(regexToTest.test('google.com:65536')).toBeFalsy();
});
});
10 changes: 8 additions & 2 deletions lib/Url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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])';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The valid port range is from 1-65535

const URL_PROTOCOL_REGEX = '((ht|f)tps?:\\/\\/)';
const URL_WEBSITE_REGEX = `${URL_PROTOCOL_REGEX}?((?:www\\.)?[-a-z0-9]+?\\.)+(?:${TLD_REGEX})(?:\\:${ALLOWED_PORTS}|\\b|(?=_))`;
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MonilBhavsar Updated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add this to automated test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.


export {
URL_WEBSITE_REGEX,
URL_PATH_REGEX,
URL_PARAM_REGEX,
URL_FRAGMENT_REGEX,
URL_REGEX
URL_REGEX,
URL_REGEX_WITH_REQUIRED_PROTOCOL,
URL_PROTOCOL_REGEX,
};