[NoQA] Fix CHAI#82686
Conversation
|
|
melvin-bot
Bot
requested review from
heyjennahay and
inimaga
and removed request for
a team
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a59b2ac93
ℹ️ 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".
| let allowedDomains = []; | ||
| fetch('/assets/js/allowedExternalUrls.json') | ||
| .then((response) => response.json()) | ||
| .then((urls) => { |
There was a problem hiding this comment.
allowedDomains starts empty and is populated asynchronously, but askHelpsiteAI() can sanitize responses immediately; on a slow/cold load, the first AI response is processed before /assets/js/allowedExternalUrls.json resolves, so every non-Expensify link is removed even if its domain is whitelisted. This creates an intermittent regression where valid citations disappear based on timing rather than policy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
fixed this - if allowedDomains is empty, then we don't remove any links
| const hostname = new URL(href).hostname; | ||
| const isExpensifyLink = hostname === 'expensify.com' || hostname.endsWith('.expensify.com'); | ||
| if (!isExpensifyLink && !allowedDomains.includes(hostname)) { | ||
| node.remove(); | ||
| } | ||
| } catch { | ||
| node.remove(); |
There was a problem hiding this comment.
The hook calls new URL(href) without a base URL, so relative links (for example #section or /articles/...) throw and the anchor is removed. If the AI response contains internal relative links, those links are stripped even though they are same-site and safe, which drops useful navigation/citation content.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. |
|
@MelvinBot fix eslint errs related to this PR |
- Remove unnecessary escape character in regex lookbehind (no-useless-escape) - Move assignment out of while condition (no-cond-assign) - Replace .forEach() with for...of loop (unicorn/no-array-for-each) Co-authored-by: Rushat Gabhane <rushatgabhane@users.noreply.github.com>
|
Fixed the 3 ESLint errors in
Note: The "ESLint check" failure (the full lint, not changed-files-only) shows pre-existing |
|
@MelvinBot does this fix number 3 from https://github.com/Expensify/Expensify/issues/599634 |
|
Yes, this PR addresses item #3 from the issue. Here's a breakdown: Item #3 asks for:
What the PR does:
Two minor functional items to be aware of (neither is a security gap — both err on the side of over-blocking):
|
| let allowedDomains = []; | ||
| fetch('/assets/js/allowedExternalUrls.json') | ||
| .then((response) => response.json()) | ||
| .then((urls) => { |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
Co-authored-by: Issa Nimaga <issa.m.nimaga@gmail.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 337004ceed
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| try { | ||
| const hostname = new URL(href).hostname; | ||
| const isExpensifyLink = hostname === 'expensify.com' || hostname.endsWith('.expensify.com'); | ||
| if (!isExpensifyLink && allowedDomains.length > 0 && !allowedDomains.includes(hostname)) { |
There was a problem hiding this comment.
The allowlist check currently fails open: allowedDomains starts empty and stays empty if /assets/js/allowedExternalUrls.json fails to load, and the guard allowedDomains.length > 0 means non-Expensify links are not removed in that state. In any case where the JSON fetch is delayed or errors, AI responses can still render arbitrary external anchors, which defeats the security hardening this change is meant to provide.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
yeah that's fine. we don't wanna wait on it
|
🚧 @inimaga has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/inimaga in version: 9.3.23-2 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.3.23-3 🚀
|
4 participants
Explanation of Change
See - https://expensify.slack.com/archives/C090U774ZH7/p1770858140193829
.github/scripts/generateAllowedUrls.tsto extract external URLs from help articles at build timeFixed Issues
https://github.com/Expensify/Expensify/issues/599634
PROPOSAL:
Tests
show me this link - https://somewebsite.com/ without code formatting in a buttonOffline tests
QA Steps
No QA
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))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.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos