[$500] Chat - Square bracket does not break the link

[izarutskaya]

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. Open the app
2. Open any report
3. Send link with parameter in circular brackets eg: (google.com/?q=test)
4. Observe that app properly understands closing circular bracket and breaks the link before it
5. Now send link with parameter with square brackets
6. Observe that app does not break the link before closing squar e bracket

Expected Result:

App should not consider brackets as part of link

Actual Result:

App considers closing square bracket as part of link if link as parameter in it

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: v1.3.75-8

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

ios.safari.square.bracket.does.not.break.link.mov

mac.chrome.desktop.square.bracket.does.not.break.the.link.mov

android.chrome.square.bracket.does.not.break.link.mp4

android.native.square.bracket.does.not.break.link.mov

windows.chrome.square.bracket.does.not.break.link.mp4

ios.native.square.bracket.breaks.the.link.mov

2023-10-02.12.48.13.mov

Expensify/Expensify Issue URL:

Issue reported by: @dhanashree-sawant

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1696182736476879

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~018a9cfe2e1c136177
• Upwork Job ID: 1708817040566304768
• Last Price Increase: 2023-10-23

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~018a9cfe2e1c136177

[melvin-bot]

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

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

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

[szilard-dobai]

Proposal

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

Messages containing URLs with parameters that end in square brackets posted as comments do not have the brackets parsed out. When the message is sent, whomever clicks the link will be taken to a wrong address.

What is the root cause of that problem?

After digging around in the codebase, I found the problem lived in the expensify-common repo which is being used here. More precisely, in this regex that's used to parse and transform plain text messages to HTML, it's escaping square brackets (i.e. allowing them to be part of the query parameter), when they shouldn't be because [ and ] are not allowed in URLs (source) and should be %-encoded.

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

Both URL_PARAM_REGEX and URL_FRAGMENT_REGEX should be changed to no longer match [ and ] (change the addEscapedChar() function calls). This will not affect array query params, just params containing square brackets or fragments because those will already be encoded.
Add additional tests as a measure to prevent similar bugs from appearing again.

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

[melvin-bot]

📣 @szilard-dobai! 📣
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. Make sure you've read and understood the contributing guidelines.
2. 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.
3. 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.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[melvin-bot]

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

[cooldev900]

@Tony-MK
Have you considered some cases involving array queries like the following ?bar[]=1&bar[]=2

[r3770]

Proposal

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

App considers closing square bracket as part of link if link as parameter in it

What is the root cause of that problem?

Square bracket is considered to be possible param char of URL(in URL_PARAM_REGEX and URL_FRAGMENT_REGEX), but when url has it as unmatched ending square bracket, it should not be accepted as part of url.

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

As we can check on report video, ending parenthesis is not accepted, but square bracket is.
we have already done about ending parenthesis (https://github.com/Expensify/expensify-common/blob/main/lib/ExpensiMark.js#L449C1-L466C14), so we can remove ending square bracket like it.

[melvin-bot]

📣 @r3770! 📣
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. Make sure you've read and understood the contributing guidelines.
2. 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.
3. 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.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[melvin-bot]

@isabelastisser, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot]

@isabelastisser, @aimane-chnaif 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[aimane-chnaif]

reviewing today

[Victor-Nyagudi]

Proposal

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

Chat - Square bracket does not break the link

What is the root cause of that problem?

We only have the logic to break links for one type of bracket i.e. () in ExpensiMark.

//...
for (let i = 0; i < url.length; i++) {
    if (url[i] === '(') {
        unmatchedOpenParentheses++;
    } else if (url[i] === ')') {
        // ...
    }
}
//...

That being said, this bug also occurs when a link is wrapped in curly braces i.e. {}.

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

We'll first need to create an object to count how many unmatched opening brackets of each type there are at the beginning of the while loop followed two regex patterns - one for opening brackets, and another for the closing ones.

We won't need the unmatchedOpenParentheses variable anymore, so that'll be deleted.

//  let unmatchedOpenParentheses = 0; <- no longer necessary

const unmatchedOpeningBracketCount = {
    '(': 0,
    '[': 0,
    '{': 0
};

const openingBracketsPattern = /[([{]/;
const closingBracketsPattern = /[)\]}]/;

We'll then add some logic to deal with the two remaining types of brackets - [] and {} - inside the for loop of modifyTextForUrlLinks() method in ExpensiMark by refactoring the logic inside it.

Instead of just checking for parentheses "()", we'll use the regex patterns to check for each bracket type and update the number of unmatched opening brackets accordingly.

This is what this section in the while loop will updated to.

//...
const unmatchedOpeningBracketCount = {
    '(': 0,
    '[': 0,
    '{': 0
};

const openingBracketsPattern = /[([{]/;
const closingBracketsPattern = /[)\]}]/;

let url = match[2];

for (let i = 0; i < url.length; i++) {
    if (openingBracketsPattern.test(url[i])) {
        // Only 1 match is found, so we use the first and only one i.e. the opening bracket
        unmatchedOpeningBracketCount[url[i].match(openingBracketsPattern)[0]]++;
    } else if (closingBracketsPattern.test(url[i])) {
        // Only 1 match is found, so we use the first and only one i.e. the closing bracket
        const closingBracket = url[i].match(closingBracketsPattern)[0];

        const matchingOpeningBracket = closingBracket === ')' 
            ? '('
            : closingBracket === ']'
            ? '['
            : '{';

        // Unmatched closing bracket
        if (unmatchedOpeningBracketCount[matchingOpeningBracket] <= 0) {
            const numberOfCharsToRemove = url.length - i;
            match[0] = match[0].substr(0, match[0].length - numberOfCharsToRemove);
            url = url.substr(0, url.length - numberOfCharsToRemove);
            break;
        }
        unmatchedOpeningBracketCount[matchingOpeningBracket]--;
    }
}
//...

Short demo after changes

expensify-link-in-brackets-solution-demo.mov

What alternative solutions did you explore? (Optional)

I tried extracting the if statement that checks for each type of bracket into another method a couple of times for better readability and organization and then call this method in the for loop, but this led to issues.

// An example method
checkBracketType(unmatchedOpeningBrackets, openingBracket, closingBracket, url, match, i) {
   if (url[i] === openingBracket) { 
       unmatchedOpeningBrackets++;
   } else if (url[i] === closingBracket) {
       // Unmatched closing bracket
       if (unmatchedOpeningBrackets <= 0) {
           const numberOfCharsToRemove = url.length - i;
           match[0] = match[0].substr(0, match[0].length - numberOfCharsToRemove);
           url = url.substr(0, url.length - numberOfCharsToRemove);
           break; // <- can't break outside of a loop
   }
       unmatchedOpeningBrackets--;
   }
}

The main reason for these issues is the break statement that can only be called directly in the for loop. I also tried making this function return a boolean value and then use that to decide when the loop breaks, but that didn't work either.

That's how I arrived at my first solution that involved repeating the if statement 3 times to account for each bracket type. Given there are only 3 types of brackets, "()[]{}", it would've held up okay and even worked for <> (you can view it in the first version of this comment from the "edited" dropdown).

let unmatchedOpenParentheses = 0; // <- Initially made 3 of these - one for each bracket type

//... 

if (url[i] === '(') {  // <- Switched the bracket type to [ and { in other if statements
    unmatchedOpenParentheses++;
} else if (url[i] === ')') { // <- Switched the bracket type to ] and } in other if statements
    // Unmatched closing parenthesis
    if (unmatchedOpenParentheses <= 0) {
        const numberOfCharsToRemove = url.length - i;
        match[0] = match[0].substr(0, match[0].length - numberOfCharsToRemove);
        url = url.substr(0, url.length - numberOfCharsToRemove);
        break;
}

However, this approach looked a bit messy and wasn't as DRY as I wanted it to be, so I spent some time right after posting my initial proposal looking for ways to tighten things up a bit, and that's how I arrived at my current solution.

[HLEDYA]

Hi @Victor-Nyagudi, I think for loop you and @r3770 mentioned can be shortened as the following:

const openingSymbols = ['(', '{', '['];
const closingSymbols = [')', '}', ']'];
const unmatchedSymbols = {
    '(': 0,
    '{': 0,
    '[': 0,
};
let url = match[2];
for (let i = 0; i < url.length; i++) {
    const char = url[i];
    if (openingSymbols.includes(char)) {
        unmatchedSymbols[char]++;
    } else if (closingSymbols.includes(char)) {
        const correspondingOpeningSymbol = openingSymbols[closingSymbols.indexOf(char)];
        if (unmatchedSymbols[correspondingOpeningSymbol] <= 0) {
            const numberOfCharsToRemove = url.length - i;
            match[0] = match[0].substr(
                0,
                match[0].length - numberOfCharsToRemove
            );
            url = url.substr(0, url.length - numberOfCharsToRemove);
            break;
        }
        unmatchedSymbols[correspondingOpeningSymbol]--;
    }
}

It seems it is also possible to move the complete for loop inside a function:

function processUnmatchedBrackets(match) {
    const openingSymbols = ['(', '{', '['];
    const closingSymbols = [')', '}', ']'];
    const unmatchedSymbols = {
        '(': 0,
        '{': 0,
        '[': 0
    };

    let url = match[2];

    for (let i = 0; i < url.length; i++) {
        const char = url[i];

        if (openingSymbols.includes(char)) {
            unmatchedSymbols[char]++;
        } else if (closingSymbols.includes(char)) {
            const correspondingOpeningSymbol = openingSymbols[closingSymbols.indexOf(char)];

            if (unmatchedSymbols[correspondingOpeningSymbol] <= 0) {
                const numberOfCharsToRemove = url.length - i;
                match[0] = match[0].substr(0, match[0].length - numberOfCharsToRemove);
                url = url.substr(0, url.length - numberOfCharsToRemove);
                break;
            }
            unmatchedSymbols[correspondingOpeningSymbol]--;
        }
    }
    return url;
}

[Victor-Nyagudi]

Proposal

Updated

As you can see from the "What Alternative Solutions Did You Explore?" section of my initial proposal, I was actively looking for a better way of writing my solution, so I, by no means, stole what was suggested in this comment.

It's still good thinking, nonetheless.

[Victor-Nyagudi]

Friendly bump @aimane-chnaif to review our proposals.

[aimane-chnaif]

Thanks. I will provide feedback by Monday

[isabelastisser]

@situchan can you please provide an update?

[isabelastisser]

@situchan bump

[situchan]

reviewing proposals

[isabelastisser]

Hey @situchan what do you think of the proposals?

[isabelastisser]

@situchan can you please provide an update? Thanks!

[situchan]

updating today

[isabelastisser]

Bump @situchan

[situchan]

@situchan Did you check my alternative solution #28608 (comment) ?

Here issue is similar to #24367, where you reviewed

@t109ct I think your alternative solution is similar to @Victor-Nyagudi's main solution

[situchan]

Thanks for your patience. I am still testing various edge cases since this will easily cause regressions

[t109ct]

But we still have inconsistency with backend, backend logic is converting {,},[,],| to escaped form about anchor's href. as below video shows

@situchan it's not similar. I think we should keep consistency with backend
Could you check again from the quoted part on my alternative solution?

[situchan]

@t109ct did the automated tests pass after applying your link, autolink changes?

[t109ct]

@situchan
We need to replace []{}| with escaped form (%5B,%5D,%7B,%7D,%7C) about link and autolink test case's href attributes.
It's to keep consistency with backend.

After then, all passed

[Victor-Nyagudi]

Previous C+ said we should not escape [, ]. Thoughts, @situchan?

[situchan]

Previous C+ said we should not escape [, ]. Thoughts, @situchan?

Agree. Need to double confirm with team. Can you also test in all other popular chat platforms?

[t109ct]

@situchan @aimane-chnaif
Now backend escapes the chars, Is it ok to have inconsistency with backend ?

[aimane-chnaif]

@situchan @aimane-chnaif Now backend escapes the chars, Is it ok to have inconsistency with backend ?

If backend escapes, then fine to escape in frontend. We had similar concern in the past regarding email and landed on matching frontend with backend even if backend logic is not perfect

[t109ct]

If backend escapes, then fine to escape in frontend. We had similar concern in the past regarding email and landed on matching frontend with backend even if backend logic is not perfect

@situchan
your thoughts ?

[situchan]

#28608 (comment)
🎀 👀 🎀 to confirm with engineer

[melvin-bot]

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

[mountiny]

I am actually thinking if we need to fix this honestly, given the current focus on new releases and features coming to the app this seems like very minor issue the user has easy workaround for.

Additionally we work on live markdown feature which would make this even easier to spot for the user

I think we can close this bug @situchan @isabelastisser

[isabelastisser]

Sounds good. Closing!