MM-67542: stop leaking the server subpath in Boards Desktop navigation - #244
Conversation
Boards sent `${frontendBaseURL}${path}` (which includes the server subpath)
to the Desktop App on every navigation, unlike the core web app which sends
subpath-relative paths. The Desktop App added a workaround to strip that
subpath, and that workaround misfires for teams whose name collides with the
subpath, blocking a Desktop App fix.
Send a subpath-relative `/boards${path}` instead, matching the core web app
contract, so the Desktop App no longer needs to clean the path. Browser
routing keeps the subpath-aware history basename, so there is no change for
non-subpath deployments.
The desktop navigation history logic is extracted into a dedicated,
unit-tested `desktopHistory` module, with a regression test asserting the
subpath is never included in the path sent to the Desktop App.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change adds desktop-aware history handling for the Boards route. Navigation uses the Desktop API or a same-origin ChangesDesktop history integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant BrowserHistory
participant DesktopHistory
participant DesktopAPI
participant Window
BrowserHistory->>DesktopHistory: push(path)
DesktopHistory->>DesktopAPI: sendBrowserHistoryPush(path)
DesktopHistory->>Window: postMessage(path) fallback
Window->>DesktopHistory: receive same-origin path
DesktopHistory->>BrowserHistory: replace normalized path
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webapp/src/desktopHistory.ts`:
- Around line 47-52: Update the message listener around handleBrowserHistoryPush
to safely access event.data via optional chaining and invoke the handler only
when the extracted pathName is a string; otherwise ignore the message. Add a
fallback-message test covering a null event.data payload.
- Around line 31-37: Update handleBrowserHistoryPush to accept only the exact
boardsRouteBase path or paths beginning with boardsRouteBase followed by “/”,
rejecting routes such as `/boards-legacy/...`; remove the confirmed prefix using
slice rather than replace, and add a regression test covering the rejected
legacy route.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9b2f9fca-91a6-4e46-86be-d2aef7ffc1d4
📒 Files selected for processing (3)
webapp/src/desktopHistory.test.tswebapp/src/desktopHistory.tswebapp/src/index.tsx
- Require a `/boards` route boundary so paths like `/boards-legacy/...` are not treated as boards paths, and strip the prefix with `slice` (mapping the bare `/boards` to `/`). - Validate the fallback `message` payload with optional chaining and a string type guard before navigating, so a malformed same-origin message can't throw. - Add regression tests for the rejected legacy prefix, the bare boards route, and the message handler (valid, cross-origin, null payload, non-string path).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webapp/src/desktopHistory.ts`:
- Around line 31-40: Update handleBrowserHistoryPush to canonicalize pathName
before validating the boardsRouteBase boundary, preventing dot-segment inputs
such as `/boards/../admin` from reaching history.replace. Reject paths
containing traversal segments or whose canonical form falls outside `/boards`,
while preserving valid boards-root and nested routes; add a regression test
covering this input.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0c93255d-5597-49dd-9870-fc445d7340b5
📒 Files selected for processing (2)
webapp/src/desktopHistory.test.tswebapp/src/desktopHistory.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- webapp/src/desktopHistory.test.ts
…bbit review) Canonicalize the incoming path with the URL parser before re-checking the `/boards` boundary, so dot-segment inputs such as `/boards/../admin` (and their percent-encoded forms) resolve to a path outside boards and are rejected rather than reaching `history.replace`. The query string and hash are preserved. Adds regression tests for `/boards/../admin`, `/boards/%2e%2e/admin`, and query/hash preservation.
devinbinnie
left a comment
devinbinnie
left a comment
There was a problem hiding this comment.
LGTM. Just as a sanity check, have we manually tested the four cases highlighted in the PR description? I can provide a build with the cleaning removed if need be.
@devinbinnie - I have not, that would be great, thanks. |
@jgheithcock Here you go: https://github.com/mattermost/desktop/actions/runs/30936995306/artifacts/8903794993 |
avasconcelos114
left a comment
avasconcelos114
left a comment
There was a problem hiding this comment.
LGTM! nice to see this custom history separated into its own file with a dedicated test suite
Manual testing done(With a built version of the Desktop app with the fix in):
Remove subpath (this takes a fair bit of time to clear all the caches, restart servers etc) |
4 participants
Summary
Boards sent the Desktop App a browser-history path built from
`${frontendBaseURL}${path}`, andfrontendBaseURLis<serverSubpath>/boards— so every navigation leaked the server subpath.The core web app sends subpath-relative paths (it strips the basename itself);
to compensate for Boards, the Desktop App added a workaround that strips the
subpath from paths Boards pushes. That workaround misfires when a team's name
collides with the subpath, which is currently blocking a Desktop App fix.
This PR makes Boards send a subpath-relative
`/boards${path}`instead,matching the core web app contract, so the Desktop App no longer has to clean
the path.
What changed
/boards${path}(subpath-relative) to the Desktop App instead of${frontendBaseURL}${path}.index.tsxinto a dedicated, unit-testablewebapp/src/desktopHistory.tsmodule, and factored the repeated
/boardsliteral into a sharedboardsRouteBaseconstant used by both the send and receive sides.webapp/src/desktopHistory.test.tswith a regression test asserting thesubpath is never included in the path sent to Desktop (simulated
/company/boardssubpath deployment), plus receive-side andpostMessagefallback coverage.
Browser routing is unchanged: the history basename still includes the subpath,
so in-app URLs resolve exactly as before. No behavioral change for
non-subpath deployments — there, old and new produce byte-identical paths.
Compatibility & rollout
The only case where behavior differs is subpath deployments:
Safe sequencing: this Boards fix ships first; the Desktop App removes its
path-cleaning once no supported/ESR builds still ship an affected Boards. This
PR intentionally does not add a runtime capability signal — Desktop can gate
hack-removal on server/plugin version if it wants to decouple from the ESR tail.
Ticket Link
https://mattermost.atlassian.net/browse/MM-67542
Related Pull Requests
path-cleaning workaround once supported Boards versions include this fix.
(link the Desktop PR here once it exists)
Screenshots
N/A — no visual changes.
Test Plan
npm run test(882 tests / 487 snapshots pass; newdesktopHistorysuite 7/7),npm run check-types(tsc) clean,npm run check(eslint + stylelint) clean.switching, board/card navigation all work.
identical to today.
Release Note
Change Impact: 🟡 Medium
Regression Risk: Navigation behavior changes across web and desktop history handling. Tests cover subpaths, fallback handling, traversal rejection, and desktop-only paths. The flow is user-facing and spans multiple modules.
QA Recommendation: Run targeted manual QA for desktop and subpath navigation. No broad manual regression pass is required.
Generated by CodeRabbitAI