[$1000] Inconsistent autolink parsing on parameter with styling characters (underscores and tildes)

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Open staging.new.expensify.com
2. Send this message to a chat : https://example.com/?_parameter_
3. Send this message to a chat : https://example.com/?~parameter~
4. Examine the difference of the parsing

Expected Result :

Since tilde is a valid URL character and treated as unreserved characters, just like underscore (https://datatracker.ietf.org/doc/html/rfc3986#section-2.3), parameters with tildes should be treated the same as parameter with underscores, hence tilde should be included in the URL

Actual Result :

While URL parameter with underscore parsed correctly and included in the autolink, URL parameter with tildes parsed separatedly and the parameter isn't included in the autolink

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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: 1.3.11.2
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

Tilde.Autolink.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01cb4a801e2a4e7612
• Upwork Job ID: 1655598377906573312
• Last Price Increase: 2023-05-08

[melvin-bot]

Triggered auto assignment to @NicMendonca (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

[huzaifa-99]

Proposal

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

We want to allow the tilde ~ symbol in the url regex just like we are allowing underscore _

What is the root cause of that problem?

We are not allowing tilde ~ in the url parameters regex in the expensify-common lib here

const URL_PARAM_REGEX = `(?:\\?${addEscapedChar('[-\\w$@.+!*()\\/,=%{}:;\\[\\]\\|_]')}*)?`;

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

We need to updated regex to allow ~, like this

const URL_PARAM_REGEX = `(?:\\?${addEscapedChar('[-\\w$@.+!*()\\/,=%{}:;\\[\\]\\|_|~]')}*)?`;

Optional:
At the moment, we are only allowing _ and ~ from the list of valid unreserved characters ([A-Za-z0-9_.\-~]) allowed in url (. and - also work in some cases).

I suggest we also allow \ so valid urls like these would work

https://example.com/search?q=foo\\bar
https://www.example.com/path/to/page.html
https://www.example.com\path\to\page.html
https:\\www.example.com/path/to/page.html

For this we need update the url param regex to allow \ with these changes

1. Allow \ in url path by updating website regex and path regex

const URL_PROTOCOL_REGEX = '((ht|f)tps?:[\\/|\\\\][\\/|\\\\])'; // [\\/|\\\\]{2} would also work in place of [\\/|\\\\][\\/|\\\\]
const URL_WEBSITE_REGEX = `${URL_PROTOCOL_REGEX}?((?:www\\.)?[a-z0-9](?:[-a-z0-9]*[a-z0-9])?\\.)+(?:${TLD_REGEX})(?:\\:${ALLOWED_PORTS}|\\b|(?=_))`;

const URL_PATH_REGEX = `(?:${addEscapedChar('[.,=(+$!*]')}?[\\/|\\\\]${addEscapedChar('[-\\w$@.+!*:(),=%~]')}*${addEscapedChar('[-\\w~@:%)]')}|\\/|\\\\)*`;

2. Allow \ in url params

const URL_PARAM_REGEX = `(?:\\?${addEscapedChar('[-\\w$@.+!*()\\/,=%{}:;\\[\\]\\|_|~|\\\\]')}*)?`;

What alternative solutions did you explore? (Optional)

N/A

[NicMendonca]

Since tilde is a valid URL character and treated as unreserved characters, just like underscore

Wow, TIL

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01cb4a801e2a4e7612

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[huzaifa-99]

Proposal above

[dummy-1111]

Proposal

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

Underscores and tildes are parsed differently though they are valid URI characters.

What is the root cause of that problem?

The main cause of this issue is in its query regex below.

const URL_PARAM_REGEX = `(?:\\?${addEscapedChar('[-\\w$@.+!*()\\/,=%{}:;\\[\\]\\|_]')}*)?`;

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

We need to update the param regex. Also \w includes _ and there is no needs to include it at the end.

const URL_PARAM_REGEX = `(?:\\?${addEscapedChar('[-\\w$@.+!*()\\/,=%{}:;\\[\\]\\|~]')}*)?`;

Result

mac_safari_18564.mp4

What alternative solutions did you explore? (Optional)

{}| are valid URI characters? I can see that this is the only difference between fragment and query regex. I've not found any documents describing these are valid URI characters. If I was wrong, please kindly include those links here. If not, we should remove them from path query.

[sobitneupane]

Thanks for the proposals.

Proposal from @huzaifa-99 looks good to me. I don't think any optional change will be required here.

🎀👀🎀 C+ reviewed

cc: @PauloGasparSv

[melvin-bot]

📣 @huzaifa-99 You have been assigned to this job by @PauloGasparSv!
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 📖

[PauloGasparSv]

LGTM to me too so I've assigned @huzaifa-99 here.

Btw, I think we should implement those optional changes + test to see if they cause any regressions.

[huzaifa-99]

Thanks for the proposal review @sobitneupane, @PauloGasparSv.

pull/533 up for review in expensify-common. I have also added the optional changes as @PauloGasparSv suggested

[PauloGasparSv]

The P.R. was merged succesfully. Since these changes were done to Expensify-common we still have to update that lib version in NewDot.

Someone else already created this P.R. which includes our changes. I'll ask them to add our Test and QA steps there

[PauloGasparSv]

Changes are in production! I think we are good to close this.

@NicMendonca can you help with payment and closing this issue?

[NicMendonca]

@PauloGasparSv weird, I am not sure why the hold for payment title didn't populate

[NicMendonca]

Going OOO until June 5th so assigning a buddy to issue payment

[melvin-bot]

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

[huzaifa-99]

@PauloGasparSv Is this eligible for a quick merge bonus?

Asking since I only created the expensify-common/pull/533 but the App/pull/18994 that introduced those changes in Expensify/App was done by someone else, tho their work was unrelated but it pulled the latest changes from expensify-common.

[PauloGasparSv]

@PauloGasparSv Is this eligible for a quick merge bonus?

Asking since I only created the Expensify/expensify-common#533 but the #18994 that introduced those changes in Expensify/App was done by someone else, tho their work was unrelated but it pulled the latest changes from expensify-common.

I think the compensation will only be based on when Expensify/expensify-common#533 was merged and not #18994 since that comes from another contributor : )
cc @kevinksullivan so you are aware of that too

[kevinksullivan]

@kerupuksambel @huzaifa-99 @sobitneupane offers sent. Let me know when you accept. Thanks!

[kerupuksambel]

@kevinksullivan offer accepted, thanks!

[huzaifa-99]

Accepted @kevinksullivan. Also is this eligible for a quick merge bonus, the pr got merged in 5 days including weekend (-2 days) ?

[kevinksullivan]

Correct @huzaifa-99

[kevinksullivan]

Paid out!

[kevinksullivan]

@huzaifa-99 upwork is being buggy and not letting my close the contract. Stay tuned on that.

[huzaifa-99]

Thanks for the heads up @kevinksullivan.

[kevinksullivan]

All set @huzaifa-99 !