diff --git a/application/single_app/config.py b/application/single_app/config.py index efe40558c..638e73afe 100644 --- a/application/single_app/config.py +++ b/application/single_app/config.py @@ -101,7 +101,7 @@ EXECUTOR_TYPE = 'thread' EXECUTOR_MAX_WORKERS = 30 SESSION_TYPE = 'filesystem' -VERSION = "0.261.289" +VERSION = "0.261.290" IS_DEVELOPMENT = is_development_env_enabled() # Opt-out for deployments where App Service Easy Auth is active but the platform diff --git a/application/v2_ui/src/components/layout/Sidebar.tsx b/application/v2_ui/src/components/layout/Sidebar.tsx index deb1be0cc..2c970061d 100644 --- a/application/v2_ui/src/components/layout/Sidebar.tsx +++ b/application/v2_ui/src/components/layout/Sidebar.tsx @@ -39,7 +39,6 @@ import { Users, } from 'lucide-react'; import { useUiStore } from '../../stores/uiStore'; -import { safeSameOriginUrl } from '../../lib/adminOperations'; import { useBootstrapStore } from '../../stores/bootstrapStore'; import { useChatStore } from '../../stores/chatStore'; import { classicChatHref } from '../../lib/conversationUrl'; @@ -84,6 +83,17 @@ const NAV_ITEMS: NavItem[] = [ { to: '/content-review', label: 'Content review', icon: ShieldCheck, hint: 'Review screened knowledge, including existing holds when new scanning is disabled' }, ]; +/** + * A rail destination, kept as a path relative to the router's `/v2` base. + * + * It must never become a full URL. React Router treats an absolute URL outside the `/v2` + * basename as an external link and renders a plain anchor, so every rail link would reload + * the classic page at that path instead of routing within V2 (#1698). + */ +function safeNavHref(value: string): string { + return NAV_ITEMS.some((item) => item.to === value) ? value : '/chat'; +} + function BrandMark({ collapsed }: { collapsed: boolean }) { const branding = useBootstrapStore((state) => state.data?.branding); const theme = useUiStore((state) => state.theme); @@ -491,7 +501,7 @@ export function Sidebar({ mobile = false }: { mobile?: boolean }) { const label = item.to === '/public' ? publicLabels.plural : item.label; return ( diff --git a/docs/explanation/fixes/V2_SIDEBAR_NAV_LINKS_FIX.md b/docs/explanation/fixes/V2_SIDEBAR_NAV_LINKS_FIX.md new file mode 100644 index 000000000..b5c02663a --- /dev/null +++ b/docs/explanation/fixes/V2_SIDEBAR_NAV_LINKS_FIX.md @@ -0,0 +1,102 @@ +# V2 Sidebar Navigation Links Fix (v0.261.290) + +## Issue + +Every primary link in the V2 left rail opened the classic interface instead of its V2 +page. The affected links were Chats, Agents, My Workspace, Group Workspaces, Public +Workspaces, Approval requests and Content review. For example, clicking **My Workspace** on +`/v2/chat` did a full page load to `/workspace`, which is the classic workspace page. The +link's highlight in the rail also never matched the open page. Tracked in +[#1698](https://github.com/microsoft/simplechat/issues/1698). + +Fixed in version: **0.261.290**, tracked in `application/single_app/config.py`. + +## Root cause + +`Sidebar.tsx` built each rail link with: + +```tsx +to={safeSameOriginUrl(item.to, window.location.origin) ?? '/'} +``` + +`safeSameOriginUrl` in `lib/adminOperations.ts` returns a full URL, such as +`https://host/workspace`. It was written for the admin endpoint links, which really need a +full address. React Router's `Link` routes an absolute URL inside the application only when +its path is under the router's basename. The V2 basename is `/v2`, and `/workspace` is not +under it. React Router therefore treated each rail link as external. It rendered a plain +`` without its click handler, so the browser loaded the +classic Flask route at that path. `NavLink` also never marked the link as current. + +Two parallel changes had each fixed an XSS sink check finding on the same line, +`to={item.to}`. One used a relative allowlist helper, `safeNavHref`, in #1687. The other +used `safeSameOriginUrl`. A later merge kept the second change. Both helper names match the +checker's approved `safe*Href` and `safe*Url` patterns, so CI passed. The Playwright test +that clicks **Chats** and waits for `/v2/chat` would have failed, but UI tests skip when no +deployment is configured. + +The same name made this easy to get wrong. `safeSameOriginUrl` in `lib/apiClient.ts` returns +a relative path, while the `lib/adminOperations.ts` function of the same name returns a full +URL. + +## Technical details + +### Files modified + +- `application/v2_ui/src/components/layout/Sidebar.tsx`: brings back `safeNavHref(value)`. + It returns the path unchanged when the path is one of the rail's own `NAV_ITEMS` + destinations, and `/chat` otherwise. The links now use `to={safeNavHref(item.to)}`, so + they stay relative to `/v2` and still pass the XSS sink check. The unused + `lib/adminOperations` import is removed. +- `application/single_app/config.py`: version `0.261.290`. + +Custom Pages, External Links, Latest Features and **Back to classic UI** are deliberately +plain anchors to server-rendered or third-party pages, and they are unchanged. The +workspace section rail is also unchanged. It already passes its links through +`normalizeWorkspaceUrl`, which returns only a pathname. + +### Tests + +`functional_tests/test_v2_sidebar_primary_nav_stays_in_v2.py` has 5 cases. It checks that: + +- every rail destination is a root-relative path with a matching route in `App.tsx`; +- the rail's `NavLink` uses `safeNavHref(item.to)`, does not use the page origin, and does + not import from `lib/adminOperations`; +- `safeNavHref` returns the allowlisted path unchanged and never builds a URL; +- no `to=` prop in any V2 `.tsx` file is built from `location.origin` or + `safeSameOriginUrl`. The only exception is `normalizeWorkspaceUrl`, and the test confirms + it still returns only `target.pathname`; +- the application version is at least `0.261.290`. + +Three of the five cases fail against the code before the fix. + +`ui_tests/test_v2_sidebar_primary_nav_links.py` has 7 cases, one per rail link. Starting +from the V2 home page, each case checks three things after clicking the link: + +- The link's `href` is `/v2/`. +- The page stays in the single-page application. A marker set on `window` survives, which a + document load would wipe. +- The address lands on the V2 route and the link gets `aria-current="page"`. + +All seven fail before the fix. Like the other V2 UI tests, it needs `SIMPLECHAT_UI_BASE_URL` +and an authenticated storage state. + +## Impact + +V2 users can move around the interface from the rail again without being sent to the +classic interface. The open page is highlighted in the rail again. + +## Validation + +- Before: each rail link's `href` was a full URL outside `/v2`, such as + `http://127.0.0.1:5174/workspace`. Clicking it reloaded the document at the classic path, + and no link was marked current. +- After: each `href` is `/v2/`. Clicking a link routes inside the SPA without a + reload, from both the home page and the chat page, and highlights the link. Approval + requests then settles on its default category, `/v2/approvals/all`. +- To check this locally, the Vite dev server ran against a stub `/api/v2/bootstrap`. The UI + test passed 7 of 7 with the fix and failed 7 of 7 with the previous `Sidebar.tsx`. + +## Related + +- [V2 Terms of Use and Approval Requests](../features/V2_TERMS_AND_APPROVALS.md) +- [Workspace Notification Links Fix](WORKSPACE_NOTIFICATION_LINKS_FIX.md) diff --git a/docs/explanation/release_notes.md b/docs/explanation/release_notes.md index 7e69e1ecc..1502ad3b5 100644 --- a/docs/explanation/release_notes.md +++ b/docs/explanation/release_notes.md @@ -2,6 +2,16 @@ For feature-focused and fix-focused drill-downs by version, see [Features by Version](https://github.com/microsoft/simplechat/tree/main/docs/explanation/features) and [Fixes by Version](https://github.com/microsoft/simplechat/tree/main/docs/explanation/fixes). +### **(v0.261.290)** + +#### Bug Fixes + +* **V2 Sidebar Links Opened the Classic Interface** + * Every primary link in the V2 left rail opened the classic page at the same path instead of the V2 page. This covered Chats, Agents, My Workspace, Group Workspaces, Public Workspaces, Approval requests and Content review. The rail also never highlighted the open page. + * Rail links now stay relative to `/v2`, so each one opens its V2 page without a reload and is highlighted. + * A functional test now flags any router link built from the page origin in the V2 source, and a UI test clicks each rail link. + * (Ref: `Sidebar.tsx` `safeNavHref`, [#1698](https://github.com/microsoft/simplechat/issues/1698), [V2 Sidebar Navigation Links Fix](fixes/V2_SIDEBAR_NAV_LINKS_FIX.md)) + ### **(v0.261.288)** #### New Features diff --git a/functional_tests/test_v2_sidebar_primary_nav_stays_in_v2.py b/functional_tests/test_v2_sidebar_primary_nav_stays_in_v2.py new file mode 100644 index 000000000..ed9660ed6 --- /dev/null +++ b/functional_tests/test_v2_sidebar_primary_nav_stays_in_v2.py @@ -0,0 +1,253 @@ +#!/usr/bin/env python3 +""" +Functional test for the V2 rail's primary navigation staying inside the V2 interface. +Version: 0.261.290 +Implemented in: 0.261.290 + +Every primary link in the V2 rail -- Chats, Agents, My Workspace, Group Workspaces, Public +Workspaces, Approval requests and Content review -- opened the classic interface instead of +its V2 page (#1698). + +The rail passed each destination through `safeSameOriginUrl(item.to, window.location.origin)` +from lib/adminOperations.ts, which returns a full URL such as `https://host/workspace`. +React Router only routes an absolute URL in-app when its path sits under the router's +basename. `/workspace` is outside `/v2`, so `` rendered a plain external anchor and the +browser loaded the classic Flask page at that path. The active highlight never matched +either. Two parallel fixes for the same XSS sink check had edited the line, and the merge +kept the one with the absolute URL. Both helper names satisfy the checker, so nothing failed. + +This test ensures that: + - every rail destination is a root-relative path with a matching V2 route, + - the rail links are built by the relative `safeNavHref` allowlist rather than from the + page origin, + - no router link anywhere in the V2 source is built from the page origin, apart from the + workspace section rail's `normalizeWorkspaceUrl`, which is checked to return a bare + pathname. + +These are source-level assertions, so they run without a deployment. +ui_tests/test_v2_sidebar_primary_nav_links.py clicks the rendered links against a live tenant. +""" + +import os +import re +import sys +from pathlib import Path + +sys.path.append(os.path.dirname(os.path.abspath(__file__))) + +from test_support.versioning import assert_app_version_at_least + +REPO_ROOT = Path(__file__).resolve().parents[1] +V2_SRC = REPO_ROOT / "application" / "v2_ui" / "src" +SIDEBAR_PATH = V2_SRC / "components" / "layout" / "Sidebar.tsx" + +# The one helper that may receive the page origin in a router `to=` prop, because it +# validates against that origin and then hands back only the URL's pathname. +ORIGIN_SAFE_ROUTE_HELPERS = ("normalizeWorkspaceUrl(",) + + +def _read(path): + return path.read_text(encoding="utf-8") + + +def _nav_items_block(sidebar_source): + """Return the NAV_ITEMS literal.""" + match = re.search(r"const NAV_ITEMS: NavItem\[\] = \[(.*?)\n\];", sidebar_source, re.DOTALL) + assert match, "NAV_ITEMS could not be located in Sidebar.tsx" + return match.group(1) + + +def _nav_item_paths(sidebar_source): + paths = re.findall(r"\bto:\s*'([^']*)'", _nav_items_block(sidebar_source)) + assert paths, "NAV_ITEMS declares no destinations" + return paths + + +def _balanced_expression(source, open_index): + """Return the text between the brace at ``open_index`` and its matching close brace.""" + depth = 0 + for index in range(open_index, len(source)): + character = source[index] + if character == "{": + depth += 1 + elif character == "}": + depth -= 1 + if depth == 0: + return source[open_index + 1:index] + raise AssertionError("Unbalanced braces after a to= attribute") + + +def _to_expressions(source): + """Yield ``(line_number, expression)`` for every ``to={...}`` attribute in a TSX file.""" + for match in re.finditer(r"(? (") + assert start != -1, "The rail no longer renders NAV_ITEMS with NAV_ITEMS.map" + region = sidebar_source[start:] + link = region.find(" item.to === value)" in body, ( + "safeNavHref must only accept destinations the rail itself declares" + ) + assert "? value :" in body, ( + "safeNavHref must return the allowlisted path unchanged, so it stays relative" + ) + fallback = re.search(r":\s*'([^']*)'\s*;", body) + assert fallback and fallback.group(1) in _nav_item_paths(sidebar), ( + "safeNavHref's fallback must itself be a rail destination" + ) + for forbidden in ("new URL", "origin", ".href", "location"): + assert forbidden not in body, ( + f"safeNavHref must not use {forbidden!r}; turning the path into a full URL is " + "the defect this guards against" + ) + + print("safeNavHref test passed!") + return True + + +def test_router_links_are_not_built_from_the_page_origin(): + """No V2 router link builds its target from the page origin.""" + print("Testing router link targets across the V2 source...") + + offenders = [] + checked = 0 + for path in sorted(V2_SRC.rglob("*.tsx")): + source = _read(path) + for line_number, expression in _to_expressions(source): + checked += 1 + uses_origin = re.search(r"\blocation\s*\.\s*origin\b|\bsafeSameOriginUrl\s*\(", expression) + if not uses_origin: + continue + if any(helper in expression for helper in ORIGIN_SAFE_ROUTE_HELPERS) and ( + "safeSameOriginUrl" not in expression + ): + continue + relative = path.relative_to(REPO_ROOT).as_posix() + offenders.append(f"{relative}:{line_number}: to={{{' '.join(expression.split())}}}") + + assert checked, "No to={...} attributes were found in the V2 source" + assert not offenders, ( + "Router links must take a path relative to the /v2 base. A target built from the page " + "origin becomes an absolute URL, which React Router renders as an external link that " + "reloads the classic page (#1698):\n " + "\n ".join(offenders) + ) + + # The exemption above is only sound while the helper still returns a bare pathname. + workspace_context = _read(V2_SRC / "lib" / "workspaceContext.ts") + helper = _function_body(workspace_context, "normalizeWorkspaceUrl") + returns = re.findall(r"\breturn\s+([^;]+);", helper) + assert returns == ["target.pathname"], ( + "normalizeWorkspaceUrl is exempt from the origin check because it returns only " + f"target.pathname; it now returns {returns!r}" + ) + + print(f"Router link targets test passed ({checked} to= attributes checked)!") + return True + + +def test_version_is_at_least_the_fix_version(): + """The application carries at least the version this fix arrived in.""" + print("Testing version...") + assert_app_version_at_least( + "0.261.290", + reason="The V2 rail's primary links stay inside V2 from 0.261.290 (#1698).", + ) + print("Version test passed!") + return True + + +if __name__ == "__main__": + tests = [ + test_primary_nav_paths_are_relative_v2_routes, + test_primary_nav_links_are_never_absolute_urls, + test_safe_nav_href_returns_the_relative_path, + test_router_links_are_not_built_from_the_page_origin, + test_version_is_at_least_the_fix_version, + ] + + results = [] + for test in tests: + try: + results.append(test()) + except Exception as exc: + print(f"Test failed: {exc}") + import traceback + + traceback.print_exc() + results.append(False) + + print(f"\nResults: {sum(1 for result in results if result)}/{len(results)} tests passed") + sys.exit(0 if all(results) else 1) diff --git a/ui_tests/test_v2_sidebar_primary_nav_links.py b/ui_tests/test_v2_sidebar_primary_nav_links.py new file mode 100644 index 000000000..43b2d4521 --- /dev/null +++ b/ui_tests/test_v2_sidebar_primary_nav_links.py @@ -0,0 +1,133 @@ +# test_v2_sidebar_primary_nav_links.py +""" +UI test for the V2 rail's primary navigation staying inside the V2 interface. + +Version: 0.261.290 +Implemented in: 0.261.290 + +Every primary link in the V2 rail opened the classic interface instead of its V2 page +(#1698). The links had been given full URLs such as https://host/workspace. React Router +treats an absolute URL outside its /v2 basename as an external link, so it rendered a plain +anchor and the browser loaded the classic page at that path. + +For each primary rail link this test checks that the href is under /v2, and that clicking it +routes inside the single-page application. A marker set on window survives, which a document +load would wipe. The address must land on the V2 route, and the link must then be highlighted +as the current page. +""" + +import os +import re +from pathlib import Path + +import pytest +from playwright.sync_api import expect + + +BASE_URL = os.getenv("SIMPLECHAT_UI_BASE_URL", "").rstrip("/") +STORAGE_STATE = os.getenv("SIMPLECHAT_UI_STORAGE_STATE", "") or os.getenv( + "SIMPLECHAT_UI_ADMIN_STORAGE_STATE", + "", +) + +# (accessible name, V2 route). Public Workspaces is named by the administrator-configurable +# display name, so its label is read from bootstrap instead. +PRIMARY_LINKS = ( + ("Chats", "/chat"), + ("Agents", "/agents"), + ("My Workspace", "/workspace"), + ("Group Workspaces", "/groups"), + (None, "/public"), + ("Approval requests", "/approvals"), + ("Content review", "/content-review"), +) +DEFAULT_PUBLIC_PLURAL = "Public Workspaces" + + +def _require_environment(): + if not BASE_URL: + pytest.skip("Set SIMPLECHAT_UI_BASE_URL to run this UI test.") + if not STORAGE_STATE or not Path(STORAGE_STATE).exists(): + pytest.skip( + "Set SIMPLECHAT_UI_STORAGE_STATE or SIMPLECHAT_UI_ADMIN_STORAGE_STATE to a " + "valid authenticated Playwright storage state file." + ) + + +@pytest.fixture +def v2_context(playwright): + """An authenticated browser context pointed at the V2 interface.""" + _require_environment() + + browser = playwright.chromium.launch() + context = browser.new_context( + storage_state=STORAGE_STATE, + viewport={"width": 1440, "height": 900}, + ) + try: + yield context + finally: + context.close() + browser.close() + + +def _open_v2(context, path="/v2"): + """Load a V2 route and return ``(page, bootstrap_payload)``.""" + page = context.new_page() + response = page.goto(f"{BASE_URL}{path}", wait_until="domcontentloaded") + + assert response is not None, f"Expected a navigation response for {path}." + if response.status in {401, 403}: + pytest.skip("The configured session is not authorised for the V2 interface.") + if response.status == 503: + pytest.skip("The V2 bundle is not built in the environment under test.") + assert response.ok, f"Expected {path} to load, got HTTP {response.status}." + + bootstrap = page.request.get(f"{BASE_URL}/api/v2/bootstrap") + assert bootstrap.ok, f"Expected bootstrap to load, got HTTP {bootstrap.status}." + + # The rail only exists once bootstrap has resolved in the browser too. + expect(page.get_by_role("navigation", name="Primary")).to_be_visible() + + return page, bootstrap.json() + + +def _public_plural(bootstrap): + labels = (bootstrap.get("settings") or {}).get("public_workspace_labels") or {} + plural = labels.get("plural") + return plural if isinstance(plural, str) and plural.strip() else DEFAULT_PUBLIC_PLURAL + + +@pytest.mark.ui +@pytest.mark.parametrize( + "label, route", + PRIMARY_LINKS, + ids=[route.strip("/") for _label, route in PRIMARY_LINKS], +) +def test_primary_rail_link_routes_within_v2(v2_context, label, route): + """A rail link must open its V2 page in place, not reload into the classic interface.""" + # Starting from the home page means every link, Chats included, changes the route. + page, bootstrap = _open_v2(v2_context) + name = label or _public_plural(bootstrap) + + rail = page.get_by_role("navigation", name="Primary") + link = rail.get_by_role("link", name=name, exact=True).first + expect(link).to_be_visible() + + # The rendered href is what the browser follows. A full URL here is the defect: it lands + # on the classic page at the same path. + expect(link).to_have_attribute("href", f"/v2{route}") + + page.evaluate("window.__v2RailNavigationMarker = 'kept'") + link.click() + + # Approval requests settles on its default category, so a deeper V2 path is acceptable. + page.wait_for_url(re.compile(rf"^{re.escape(BASE_URL)}/v2{re.escape(route)}(?:[/?#]|$)")) + + assert page.evaluate("window.__v2RailNavigationMarker") == "kept", ( + f"{name} reloaded the document instead of routing inside the V2 application." + ) + expect(page.get_by_role("navigation", name="Primary")).to_be_visible() + expect(rail.get_by_role("link", name=name, exact=True).first).to_have_attribute( + "aria-current", "page" + )