[No QA] Normalize path in getPathFromState to fix AI Features Promo SecurityError - #99040
Conversation
This reverts the changes from #97849 so the SecurityError fix can be reimplemented as a single normalization step at the end of getPathFromState. Co-authored-by: Cong Pham <suneox@users.noreply.github.com>
Instead of hand-normalizing slashes at each concatenation site, build the raw path (dynamic or RN) and collapse any repeated slashes to a single leading slash in getPathFromState. This guarantees exactly one leading slash and no internal '//', so the browser never parses a path segment as a host and history.pushState no longer throws a SecurityError. A single malformed-path guard logs an alert if a doubled slash is ever produced upstream. Co-authored-by: Cong Pham <suneox@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
|
🤖 Web testing could not be completed — the automated session lost authentication before the test could run. I set out to verify this PR's fix on Web: open a report ( I couldn't get that far. The pre-started web session was handed over on the unauthenticated "Welcome! … Join" sign-up/validate screen instead of Home. Reaching Home from there requires an email magic code that this automated environment can't retrieve, and reloading/re-opening the session is disallowed (it drops client auth), so there was no safe path back to an authenticated Home in this run. The code itself is on the fixed branch ( Evidence & details
Unauthenticated screen observed: 
Suggested next step: re-run the web test once the session start macro runs after the PR branch is checked out and the dev server has stabilized, so it hands off an already-authenticated session on Home. Then re-trigger me (or use the |
|
🤖 Android: PASS — no regression from the The
AI Features Promo modal open on the report: Report restored & interactive after dismissing: Flow & notes
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppNot available Android: mWeb ChromeNot available iOS: HybridAppNot available iOS: mWeb SafariNot available MacOS: Chrome / SafariCleanShot.2026-08-20.at.15.43.24.1.mp4Update 08-31 CleanShot.2026-08-31.at.16.14.27.1.mp4 |
|
@MelvinBot This issue should be [NO QA]. Check all items in the PR Author Checklist and mark the PR as ready for review |
MelvinBot
changed the title
|
🤖 Done,
|
![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: 50c23deaa2
ℹ️ 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".
| // Exactly one leading slash and no internal `//`, so the browser never parses a segment as a host. | ||
| const normalizedPath = `/${pathOnly}`.replaceAll(/\/{2,}/g, '/'); |
There was a problem hiding this comment.
When an eligible user directly opens /r/:reportID/ai-features-promo, useAIFeaturesPromoModal now calls createDynamicRoute with that current URL as its base, producing /r/:reportID/ai-features-promo/ai-features-promo. This normalization only collapses adjacent slash characters, so it does not address the duplicated route suffix; keep the active AI_FEATURES_PROMO_MODAL_NAVIGATOR check before navigating.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The issue cannot be reproduced
CleanShot.2026-08-20.at.16.09.11.1.mp4
|
@WojtekBoman can you please review? @suneox I feel like we could include test steps on this one |
|
@MelvinBot update the Test section from the previous PR |
|
Done — I copied the Tests and QA Steps from the previous PR #97849 into this PR's body, replacing the TODO placeholders: Tests
QA Steps: Same as tests. These steps are behavior-level (deep-link → modal appears, no |
|
Hey, I took a look at this PR and noticed some things that should be adjusted. The centralized approach is right and the fix works, the notes below are refinements to the snippet I posted in the issue:
|
There was a problem hiding this comment.
Could we add a test that a well-formed path does not alert? With the === '/' join restored, the root-base case becomes a happy path, and that's the regression that would otherwise ship silently.
it('does not log an alert for a well-formed path', () => {
mockRNGetPathFromState.mockReturnValue('/');
const state = buildState([{name: 'StandardScreen'}, {name: 'TestDynamicScreen'}]);
expect(getPathFromState(state as PartialState<NavigationState>)).toBe('/test-dynamic');
expect(mockLogAlert).not.toHaveBeenCalled();
});For that to be meaningful the spy needs clearing, since jest.spyOn is at module scope with no beforeEach - otherwise a call leaked from an earlier test fails it (and the existing assertion can pass for the same reason):
beforeEach(() => {
mockLogAlert.mockClear();
});There was a problem hiding this comment.
We also already have jest.clearAllMocks() on beforeEach, so mockLogAlert.mockClear() isn't needed.
|
FYI: I’ll be OOO next Monday and Tuesday 🌴 If further assistance is needed with this PR, @collectioneur will help :) |
|
Confirmed: RN handles removing multiple as well as trailing slashes at path = path.replace(/\/+/g, '/');
path = path.length > 1 ? path.replace(/\/$/, '') : path;and logging rawPath can leak the query so @MelvinBot let apply #99040 (comment)
Actually, I'm not sure the simulator setup is straightforward for QA, so I still think this should be marked as [No QA] and drop the steps from the QA Steps body instead. And @MelvinBot please add more tests for this case |
…d tests Co-authored-by: Cong Pham <suneox@users.noreply.github.com>
|
Applied all the refinements from Code —
Tests —
Body — marked as |
|
No code fix is needed — every automated check on It fails because the approval count is zero: Your earlier approval was invalidated when I pushed Full check statusWhen you pinged me, the run for
|
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Thanks, looks good to me
|
🚧 mountiny 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/mountiny in version: 9.4.65-0 🚀
|
|
No help site changes are required, so I did not create a draft PR. This PR is an internal navigation fix. It normalizes the path built by Why the help site is unaffectedI checked the full diff — three files, all internal:
Against the help site criteria:
Since there is no help site PR to review, there is nothing to mark @suneox, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.4.65-3 🚀
Bundle Size Analysis (Sentry): |
6 participants
Explanation of Change
This reimplements the fix for the
SecurityError: Failed to execute 'pushState' on 'History'crash on/r/:reportID/ai-features-promo(Dynamic_AIFeaturesPromoModal_Root) using a centralized path-normalization safety net, as suggested here.1. Reverts PR #97849. The first commit reverts the merged fix (#97849), which hand-normalized the slash at each concatenation site inside
getPathFromStateWithDynamicRouteand added a re-entrancy guard inuseAIFeaturesPromoModal. That approach only patched the specific dynamic-route construction that produced the crash.2. Normalizes the path in the hand-built dynamic-route branch. Instead of fixing individual concatenation sites, the fix keeps the correct root-base join (mirroring
createDynamicRoute.ts) and adds a single normalization safety net right where the dynamic path is assembled, so every consumer of the path is covered:The root-base join means normal paths are well-formed, and the normalization is a pure safety net on top: any future dynamic-route path bug is collapsed to a valid path (so
history.pushStatecan't throw aSecurityError) and reported viaLog.alert. Normalization lives in the dynamic branch rather than at the top ofgetPathFromStatebecause React Navigation's owngetPathFromStatealready collapses slashes on the standard-screen branch. The alert logs onlyscreenName— never the raw path — so query params aren't leaked.Added regression unit tests in
getPathFromStateTests.ts: a well-formed root (/) base produces/test-dynamicand does not alert, and a malformed doubled-slash base is normalized to a single slash while alerting with the screen name only.Fixed Issues
$ #97470
PROPOSAL: #97470 (comment)
Tests
page.routedelay)./r/<id>/ai-features-promo/using a non-existent reportID.SecurityError: Failed to execute 'pushState', and the URL is not doubled (it stays/r/<id>/ai-features-promo/, not…/ai-features-promo/ai-features-promo/).Offline tests
QA Steps
[No QA] — reproducing the crash requires simulating OpenReport API latency, which isn't a practical staging setup for QA. The change is covered by unit tests in
getPathFromStateTests.tsand the localTestssteps above.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari