search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

[HOLD for payment 2023-04-17] [$1000] Can’t parse the url if the letter ‘h’ of https is uppercase #16633

Closed kavimuru closed 1 year ago

kavimuru commented 1 year ago

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:

  1. Go to any report, try to send the url that the letter ‘h’ in http or https is uppercase. (For example: Https://google.com).
  2. Try to to click on that url.
  3. Notice that the app opens the wrong url and browser can’t load it.

Expected Result:

The url should be parsed correctly.

Actual Result:

The the app opens the wrong url and browser can’t load it.

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Version Number: 1.2.90-7 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/43996225/228312995-3766be08-65a0-4470-aa6b-3f96a3296322.mp4

https://user-images.githubusercontent.com/43996225/228313019-39d684b8-558f-48d6-b5b3-3fa151e94384.MP4

Expensify/Expensify Issue URL: Issue reported by: @hungvu193 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1679990736704189

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01557f6c93ea21a4f4
  • Upwork Job ID: 1640811631278223360
  • Last Price Increase: 2023-03-28
MelvinBot commented 1 year ago

Triggered auto assignment to @NicMendonca (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

MelvinBot commented 1 year ago

Bug0 Triage Checklist (Main S/O)

NicMendonca commented 1 year ago

wow, super interesting bug @hungvu193. Just tested in slack, and I can open Https://www.google.com/ and https://www.google.com/

MelvinBot commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01557f6c93ea21a4f4

MelvinBot commented 1 year ago

Current assignee @NicMendonca is eligible for the External assigner, not assigning anyone new.

MelvinBot commented 1 year ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @thesahindia (External)

MelvinBot commented 1 year ago

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

ShogunFire commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Opening the url starting with Http with "H" in uppercase doesn't work

What is the root cause of that problem?

"http" and "https" with some letters in uppercase are not well handled by apps and browsers

What changes do you think we should make in order to solve the problem?

We can replace any version of "http" and "https" by their lowercase version in openUrl() in react native here

It has been done for example in this repo

What alternative solutions did you explore? (Optional)

jayeshmangwani commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

URL is broken if we use capital H in Https in a message e.g. Https://test.com

What is the root cause of that problem?

When generating the message from input, we develop the wrong link for capital letters in Http/Https. In Expensify-common/lib/ExpensiMark.js right now, we check if the link includes http, then do not add https:// before the link; otherwise, add https before the link, so when any capital letter added in http/https, then we will see the duplicate Https in link,we can see this by copying the link or clicking the link, and due to this duplicate https link will not work because https://Https//Google.com is not the correct link

What changes do you think we should make in order to solve the problem?

replacement: (match, g1, g2, g3) => {
    if (!g1.trim()) {
        return match;
    }

    return `<a href="${g3 && g3.includes('http') ? '' : 'https://'}${g2}" target="_blank" rel="noreferrer noopener">${g1.trim()}</a>`;
},

here when we compare the g3.includes('http') , we should convert the link to lowercase before comparing with https g3.toLowerCase().includes('http')

note: we have add this logic to autolink to when checking for the http in the link

What alternative solutions did you explore? (Optional)

N/A

ihalton commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

url link can not work when the prefix Https:// or Http:// including the H, for example.

Https://google.com or Http://google.com

What is the root cause of that problem?

the root casue is when our link prefix is Https:// or Http://, it identified as common text, so added the a repeat prefix in autolink rule in Expensify-common/lib/ExpensiMark.js, like as https://Http://google.com.

notes: in the link rule, it also has the same issue, which is in Expensify-common/lib/ExpensiMark.js link rule part

What changes do you think we should make in order to solve the problem?

I think we can update the code modifyTextForUrlLinks to fix the issue

from

replacedText = replacedText.concat(replacement(match[0], linkText, match[2], match[3]));

to 

replacedText = replacedText.concat(replacement(match[0], linkText, match[2], match[3] && match[3].toLowerCase()));

What alternative solutions did you explore? (Optional)

update the callback function replacement in link rule part and auto link part, add toLowerCase.

            {
                name: 'link',
                process: (textToProcess, replacement) => {
                    const regex = new RegExp(
                        `\\[([^\\][]*(?:\\[[^\\][]*][^\\][]*)*)]\\(${URL_REGEX}\\)(?![^<]*(<\\/pre>|<\\/code>))`,
                        'gi',
                    );
                    return this.modifyTextForUrlLinks(regex, textToProcess, replacement);
                },

                // We use a function here to check if there is already a https:// on the link.
                // If there is not, we force the link to be absolute by prepending '//' to the target.
                replacement: (match, g1, g2, g3) => {
                    if (!g1.trim()) {
                        return match;
                    }

                    return `<a href="${g3 && g3.includes('http') ? '' : 'https://'}${g2}" target="_blank" rel="noreferrer noopener">${g1.trim()}</a>`;
                },
            },
            {
                name: 'autolink',

                process: (textToProcess, replacement) => {
                    const regex = new RegExp(
                        `(?![^<]*>|[^<>]*<\\/)([_*~]*?)${URL_REGEX}\\1(?![^<]*(<\\/pre>|<\\/code>))`,
                        'gi',
                    );
                    return this.modifyTextForUrlLinks(regex, textToProcess, replacement);
                },

                // We use a function here to check if there is already a https:// on the link.
                // If there is not, we force the link to be absolute by prepending '//' to the target.
                replacement: (match, g1, g2, g3) => (
                    `${g1}<a href="${g3 && g3.includes('http') ? '' : 'https://'}${g2}" target="_blank" rel="noreferrer noopener">${g2}</a>${g1}`
                ),
            },
MelvinBot commented 1 year ago

📣 @ihalton! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Screen Shot 2022-11-16 at 4 42 54 PM

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
ihalton commented 1 year ago

Contributor details Your Expensify account email: halton.xu@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~0151a1fe2baff18781

MelvinBot commented 1 year ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

eh2077 commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Send following link markdown to a chat, like this

[Https://www.regex101.com](Https://www.regex101.com)

Click the link will open https://Https//www.regex101.com which is a invalid link.

Same issue for sending a link

Https://www.regex101.com

What is the root cause of that problem?

We send the following link markdown text as an example to dig the root cause.

[Https://www.regex101.com](Https://www.regex101.com)

The link will be translated into below invalid anchor html and send backend to save

<a href=\"https://Https://www.regex101.com\" target=\"_blank\" rel=\"noreferrer noopener\">Https://www.regex101.com</a>

and backend will correct the invalid link a little bit by removing the : after Https, like

<a href=\"https://Https//www.regex101.com\" target=\"_blank\" rel=\"noreferrer noopener\">Https://www.regex101.com</a>

We can verify this by checking the link markdown in the editing mode

[Https://www.regex101.com](https://Https//www.regex101.com)

Now let's deep dive into from frontend why this link markdown text

[Https://www.regex101.com](Https://www.regex101.com)

is translated into invalid anchor html with extra https:// prefix of property href

<a href=\"https://Https://www.regex101.com\" target=\"_blank\" rel=\"noreferrer noopener\">Https://www.regex101.com</a>

We convert link markdown into html here using this translation rule.

We use this regex to match link markdown

const regex = new RegExp(
    `\\[([^\\][]*(?:\\[[^\\][]*][^\\][]*)*)]\\(${URL_REGEX}\\)(?![^<]*(<\\/pre>|<\\/code>))`,
    'gi',
);

The regex above is look like(only keep two TLD regex, COM and ORG)

Screenshot 2023-03-29 at 6 40 16 PM

From the picture above we can find that the root cause is that when g3 is Https:// , we'll add an extra https:// before g2 Https://www.regex101.com, see https://github.com/Expensify/expensify-common/blob/9cf047b9741d3c02820d515dccb9e0a45b6595f5/lib/ExpensiMark.js#L90

That's why link markdown 

[Https://www.regex101.com](Https://www.regex101.com)

is translated into anchor html

<a href=\"https://Https://www.regex101.com\" target=\"_blank\" rel=\"noreferrer noopener\">Https://www.regex101.com</a>

And similar RCA for auto link case.

What changes do you think we should make in order to solve the problem?

From the RCA part we know that regex of g3 is (https?:\/\/)? which is original from here, so g3 value is one of

  1. empty string
  2. http://
  3. https://

And regex is case insensitive, so to fix this issue, we only need to add https:// prefix to the link if g3 is empty. We can improve this line 

return `<a href="${g3 ? '' : 'https://'}${g2}" target="_blank" rel="noreferrer noopener">${g1.trim()}</a>`;

We can do same fix for auto link

What alternative solutions did you explore? (Optional)

N/A

praveen-ix commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

When we write a message, a kind Https://google.com Capital 'H' in link. it gets converted into https://http//google.com When users click on it, it does not work. image

What is the root cause of that problem?

In ExpensiMark.js, <a href="${g3 && g3.includes('http') ? '' : 'https://'}${g2}" target="_blank" rel="noreferrer noopener">${g1.trim()}</a> Value of g3 is 'Https://' We check g3.includes('http') that return false and add extra http:// and URL become http://http//google.com and it does not work when app open this is in browser.

What changes do you think we should make in order to solve the problem?

We had tested that g3 values are 

empty string
http://
https://

If g3 is empty we have to add 'https//' otherwise no need to add anything. Changes needs to do here

return <a href="${g3 ? '' : 'https://'}${g2}" target="_blank" rel="noreferrer noopener">${g1.trim()}</a>;

What alternative solutions did you explore? (Optional)

N/A

MelvinBot commented 1 year ago

📣 @praveen-ix! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Screen Shot 2022-11-16 at 4 42 54 PM

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
praveen-ix commented 1 year ago

Contributor details Your Expensify account email: praveenb@incubxperts.com Upwork Profile Link: https://www.upwork.com/freelancers/~01b83079378002e03d

MelvinBot commented 1 year ago

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

praveen-ix commented 1 year ago

Contributor details Your Expensify account email: praveenb@incubxperts.com Upwork Profile Link: https://www.upwork.com/freelancers/~01b83079378002e03d

MelvinBot commented 1 year ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

kaushiktd commented 1 year ago

Please re-state the problem that we are trying to solve in this issue.

Can’t parse the URL if the letter ‘h’ of https is uppercase

What is the root cause of that problem?

In your custom ExpensiMark.jslibrary, you are comparing HTTP string in the same format as input by the user when new message is generated from the composer, so this is not matched with the given condition and it generates a broken URL like https://https//google.com in below mentioned file: https://github.com/Expensify/expensify-common/blob/9cf047b9741d3c02820d515dccb9e0a45b6595f5/lib/ExpensiMark.js#L85-L91

What changes do you think we should make in order to solve the problem?

You need to adjust the condition in the below-mentioned file which is comparing HTTP or HTTPS in ExpensiMark.js file to the below URL https://github.com/Expensify/expensify-common/blob/9cf047b9741d3c02820d515dccb9e0a45b6595f5/lib/ExpensiMark.js#L85-L91 Convert the text into lowercase and compare it with the condition to get the exact match. Add below lines to line# 89 or line# 125

if (!_.isEmpty(g1)) {
  g1 = g1.toLowerCase();
}

if (!_.isEmpty(g2)) {
   g2 = g2.toLowerCase();
}

if (!_.isEmpty(g3)) {
   g3 = g3.toLowerCase();
}
thesahindia commented 1 year ago

@eh2077's proposal looks good to me!

C+ reviewed 🎀👀🎀

cc: @aldo-expensify

MelvinBot commented 1 year ago

@NicMendonca, @thesahindia, @aldo-expensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

MelvinBot commented 1 year ago

📣 @eh2077 You have been assigned to this job by @aldo-expensify! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

aldo-expensify commented 1 year ago

Thanks @thesahindia , I tried the proposal and seems to work fine. Hired @eh2077 !

While @jayeshmangwani 's proposal also worked, I think @eh2077 's proposed changed made more sense considering the regex is case insensitive and the possible values that g3 could take.

eh2077 commented 1 year ago

📣 @eh2077 You have been assigned to this job by @aldo-expensify! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

Applied the job and PR for expensify-common is ready for review https://github.com/Expensify/expensify-common/pull/514

cc @thesahindia @aldo-expensify

MelvinBot commented 1 year ago

Reviewing label has been removed, please complete the "BugZero Checklist".

MelvinBot commented 1 year ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.2.97-2 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-04-17. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

MelvinBot commented 1 year ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

NicMendonca commented 1 year ago

@thesahindia @eh2077 - sent you the offer

@hungvu193 can you apply please, or let me know your name in Upwork if you have already applied? https://www.upwork.com/jobs/~01557f6c93ea21a4f4

Thanks!

eh2077 commented 1 year ago

@NicMendonca Thanks and accepted the offer

hungvu193 commented 1 year ago

@NicMendonca thank you, I've just applied

thesahindia commented 1 year ago

This was an improvement and not a regression.

Regression test proposal

  1. Go to any chat
  2. Send a link with h in uppercase for https (e.g. Https://www.google.com)
  3. Click the on the link
  4. Verify the link can be opened correctly in browser
thesahindia commented 1 year ago

@thesahindia @eh2077 - sent you the offer

Accepted, thanks!

NicMendonca commented 1 year ago

@eh2077 paid + bonus

@thesahindia paid + bonus

@hungvu193 just need you to accept the offer 😊

hungvu193 commented 1 year ago

@NicMendonca Thank you, just accepted 😁

NicMendonca commented 1 year ago

@hungvu193 paid!

NicMendonca commented 1 year ago

@thesahindia so just to make sure - you think this should be the regression test?

thesahindia commented 1 year ago

Correct!

NicMendonca commented 1 year ago

Fab! We're all set here!