Repository navigation
upgrade: bump expensify-common to 2.0.224 to enforce URL hostname length limits - #104791
Conversation
…rce URL hostname length limits
|
|
|
@linhvovan29546 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@linhvovan29546 I don't think Mobile-Expensify PR is needed here since all of the changes are JS changes, but still a second check from your side is appreciable :) TY! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a786382175
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
I'll look into the codex suggestions on tomorrow, thanks |
|
@Uzaifm127 Do we need to bump expensify-common in live-markdown? |
|
@linhvovan29546 IMO, we don't need to bump btw, looking into the suggestions... TY |
…102500-expensify-common
|
@linhvovan29546 Both codex suggestions above are real, we missed these cases during Since these cases have been caught before the final upgarde PR goes to production so these won't be considered as regressions. TY |
|
Update: @linhvovan29546 The follow up PR in expensify-common is WIP which is covering the missed cases mentioned above. I'm holding this PR untill that expensify-common follow up PR has been merged. Thanks. |
|
@linhvovan29546 The follow up PR in expensify-common covering the above cases is ready for review. Thanks. |
… hostname and email parsing fixes
|
@linhvovan29546 This PR is ready for review. Thanks. Note: There is one mistake I did, I accidentally pushed the last commit with commit message including the expensify-common version |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: mWeb Safari |
|
@Uzaifm127 are all of the gh action changes needed? |
|
@JS00001 Yes, IMO both GitHub Action changes are required for this dependency upgrade. I ran |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 JS00001 has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/JS00001 in version: 9.5.31-0 🚀
|
|
🚀 Deployed to staging by https://github.com/JS00001 in version: 9.5.31-0 🚀
|


Explanation of Change
This PR upgrades
expensify-commonfrom2.0.211to2.0.224to bring the URL hostname validation from expensify-common #951 and the follow-up changes from expensify-common #966 into App.Before this upgrade, ExpensiMark could turn an oversized hostname into a link. The URL then appeared in both the link destination and its visible text, so a message with fewer than 15,000 characters in the composer could exceed the backend's 15,000-character limit after parsing.
The updated parser checks both DNS hostname limits: 63 ASCII characters per label and 253 ASCII characters for the complete hostname. Hostnames exceeding either limit stay plain, including long dotted hostnames. The protocol, port, path, query, and fragment are excluded from the hostname length.
The follow-up PR prevents a valid-looking domain inside an oversized hostname from becoming a partial link. It also preserves the original email context during URL matching, so rejecting an oversized email domain doesn't expose the part before
@as a website link or leave the remainder styled as a mention. Separate valid URLs and normal email addresses continue to become links.Fixed Issues
$ #102500
PROPOSAL:
Tests
Test 1: Hostname label length
Open the App and open any chat.
Paste the following hostname, whose first label has 63 characters, into the composer and add a space:
Verify that the complete text becomes a link. Send it and verify that the sent message also shows the complete link.
Paste the following hostname, whose first label has 64 characters, into the composer and add a space:
Verify that the complete text stays plain, with no part converted into a link. Send it and verify that the sent message also stays plain.
Repeat steps 2 through 5 with
https://added before both hostnames and verify the same results.Paste the following URL into the composer and add a space:
Verify that the entire URL stays plain. In particular,
https://foo.commust not become a link on its own.Send it and verify that the sent message also stays completely plain.
Test 2: Complete hostname length
Open any chat.
Paste the following 253-character hostname into the composer and add a space:
Verify that the complete text becomes a link. Send it and verify that the sent message also shows the complete link.
Paste the following 254-character hostname into the composer and add a space:
Verify that the complete text stays plain, with no part converted into a link. Send it and verify that the sent message also stays plain.
Repeat steps 2 through 5 with
https://added before both hostnames and verify the same results.Test 3: Oversized hostname
Open the App on Web and open any chat.
Open Chrome DevTools and select the Console tab.
Run:
Paste the copied text into the composer and add a space after
.com.Verify that the text stays plain, with no part converted into a link.
Verify that the maximum comment length error is not displayed.
Test 4: Email with an oversized domain
Open any chat.
Paste the following text into the composer and add a space:
Verify that
valid.netandvalid.orgbecome links.Verify that the entire email address stays plain. Neither
foo.barnor the text after@should receive link or mention styling.Send the message and verify that the same parts remain linked or plain.
Send
[email protected]separately and verify that this normal email address becomes an email link.Offline tests
Same as Test
QA Steps
Same as Test
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android-native-1.mov
Android-native-2.mov
Android-native-3.mov
Android: mWeb Chrome
Android-mweb-1.mov
Android-mweb-2.mov
Android-mweb-3.mov
iOS: Native
iOS-native-1.mp4
iOS-native-2.mov
iOS-native-3.mov
iOS: mWeb Safari
iOS-safari-1.mov
iOS-safari-2.mov
iOS-safari-3.mov
MacOS: Chrome / Safari
macOS-1.mp4
macOS-2.mp4