Implementation review: PB-008, PR 455

Field	Value
Backlog item	PB-008 Keep the embedded Fabric view on Fabric hosts (feature FEAT-FAB-05; overlaps HD-027, SH-12)
Pull request	#259 (HD-027, merged as a24acb71) and #455 (three more sign-in hosts, merged as 036037d4)
Package	None
Reviewer	Engineering quality review thread. It didn't write #259 or #455.
Date	2026-10-27
Environment	Linux container, main 6159e507, Python venv for the engine, Playwright Chromium. No tenant, no Windows, no second PC.

Outcome#

Accepted with Documented Limitations (recommended).

The view keeps top-level pages and pop-ups on eight exact hosts and sends every other web link to the default browser. The host match is exact and holds up against look-alike hosts, userinfo, ports, case, trailing dots and percent-encoding. The Rust tests that cover this are in main.rs, and the CI shell job runs cargo test on every pull request and on main. The shell can't be built here, so I didn't run the tests in place. I copied the filter and its tests verbatim into a small crate and ran them there instead. Limits: images, scripts and frames inside a page aren't filtered, and I couldn't check the behavior on Windows or see a CI run result. FEAT-FAB-05 in the inventory still describes the old filter.

Recommended by: Engineering quality review. Decided by (product owner): pending.

Acceptance criteria#

#	Criterion (copied from the backlog item)	Result	Evidence
1	A link to another Microsoft site opens outside the app	Met (code and unit tests); in-app on Windows unable to verify	`fabric_nav` (app/src-tauri/src/main.rs:1291) returns `Stay` only for `https` on a host in `FABRIC_VIEW_HOSTS` (main.rs:1264) or the policy's `fabric_view_sign_in_hosts`, with no user name, password or explicit port. Other `http(s)` links get `Browser`: `fabric_nav_allows` (main.rs:1306) calls `open_in_browser` (main.rs:1320, `rundll32 url.dll,FileProtocolHandler` on Windows) and cancels the navigation. `http` to a listed host and every other scheme get `Refuse`, which only logs. The main and pop-out Fabric views (main.rs:1396) and every pop-up window (main.rs:1438) use the same filter. The test `look_alike_and_other_hosts_open_in_the_browser` covers `learn.microsoft.com`, `microsoft.com`, `fabric.microsoft.com`, `account.live.com` and others.
2	Covered by a Rust test run in CI	Met (not run here)	`mod fabric_nav_tests` (main.rs:2268–2443). .github/workflows/ci.yml runs on `pull_request` and on push to main (lines 3–6). Its `shell` job (windows-latest, line 253) runs `cargo test --release --locked` (line 281) before signing. That step was added in 1b85501f (HD-011, 2026-10-18), before #259 merged. The repo isn't reachable through `gh` from this session, so I couldn't see a CI run.

Allowed hosts (exact match, lower-cased by the url crate): app.fabric.microsoft.com, app.powerbi.com, login.microsoftonline.com, login.microsoft.com, login.windows.net, plus device.login.microsoftonline.com, autologon.microsoftazuread-sso.com and login.live.com (added by #455). An organization can add exact DNS names with fabric_view_sign_in_hosts in %ProgramData%\FabricDesktop\policy.json (policy_sign_in_hosts and dns_name, main.rs:129–147). Wildcards, ports, schemes, userinfo, trailing dots and IP addresses are dropped there. The view also starts on about:blank, which isn't allowed as a pop-up.

Matching: the code compares url.host_str() for exact equality, not by suffix. In the harness, these went to Browser: evil-fabric.microsoft.com.attacker.com, app.fabric.microsoft.com.attacker.com, app.fabric.microsoft.com@evil.com, login.microsoftonline.com%2F@evil.com, app.fabric.microsoft.com%2eevil.com (decodes to .evil.com), app.fabric.microsoft.com. (trailing dot), :8443 and :444 ports, Cyrillic а, 127.0.0.1 and [::1]. Uppercase hosts and an explicit :443 stay, because the url crate normalizes both. The url crate parses app.fabric.microsoft.com\@evil.com with host app.fabric.microsoft.com and path /@evil.com/, the same way WHATWG browsers do, so it stays on Fabric.

Checks#

Check	Result	Evidence or note
Scope matches the item	Met	#259 replaced `stays_in_view` (any `.microsoft.com`, `.microsoftonline.com`, `.powerbi.com`, `.live.com`, `.msauth.net`, `.msftauth.net`; any scheme) with the exact list and also filters pop-ups. #455 adds the device step, Seamless SSO and `login.live.com`. The broad `FABRIC_HOSTS` list (main.rs:1254) now only decides which cookies are saved.
New tests fail without the change, pass with it	Met (harness, not the real crate)	`cargo test` on the real crate fails here: `gdk-3.0` is missing for `gdk-sys` (Tauri on Linux). Instead I built a crate under the scratchpad (`pb008-harness`) with `url` 2.5.8 (from Cargo.lock) and `serde_json`. It holds main.rs lines 128–147, 1249–1250, 1260–1304 and 1337–1354, plus the six filter tests from 2271–2359 and 2417–2443, all copied verbatim. On main: 6 passed. With #455's three hosts removed: `fabric_pages_and_sign_in_stay_in_the_view` fails on `device.login.microsoftonline.com`, the other 5 pass. With `fabric_nav` swapped for the pre-#259 suffix rule: 6 of 6 fail. My extra probe test (the matching list above): passes on main, 7 of 7.
Full suites pass (pytest, UI, cargo)	Not run	cargo isn't buildable in this container (above). No engine or UI change in #455. #259's engine change (`app_settings.py` policy key, `test_policy_fail_closed.py`) wasn't rerun for this item.
Reachable where docs say	Met (code)	Fabric and Monitor pages, and the Monitoring hub pop-out. All three views are built through `fabric_view_show` with the filter. A test (main.rs:2361) checks that `NewWindowResponse::Allow`, WebView2's own unfiltered window, appears nowhere in main.rs.
OneLake data safety	n/a	No data writes.
Authentication and distribution	Met	The filter protects the saved Fabric sign-in cookies. Only the machine-wide policy file can add sign-in hosts; the user settings can't. On federated tenants, an identity-provider page that isn't part of sign-in (MFA registration, for example) opens in the browser. docs/security.md:55 says so.
No unproven claim; Preview tags	Partly	hardening/backlog.md:132 says a manual Windows check was recorded in the PR. I couldn't repeat it, and the claim isn't verified here.
Docs match the code	Met for security.md; not for the inventory	docs/security.md:55 lists the same eight hosts and rules. FEAT-FAB-05 (FEATURE_INVENTORY.md:606–618 and line 170) still says the allow-list covers any `.microsoft.com` and `.live.com`, "Rust signin tests never run by CI", and next action "Restrict in-view navigation … and add cargo test to CI". Line 93 still names FEAT-FAB-05 among features whose tests never run.
Accessibility of new controls	n/a	No new controls.
Inventory and backlog updated	Not met	See the docs row. PB-008 is "In review".

Findings#

#	Severity (blocking / should fix / note)	Finding	Where	Status
1	should fix	FEAT-FAB-05 still describes the pre-#259 filter and says CI never runs the Rust tests. Its limitation, evidence and next action are stale, and the feature is still "Unverified".	docs/product/FEATURE_INVENTORY.md:93, 170, 606–618 (and the JSON)	Open: product data owner
2	note	Only top-level navigations and pop-ups are filtered. Images, scripts and iframes inside an allowed page can load from anywhere. Frame navigation is left to Fabric's own pages. The code comment says this (main.rs:1261–1262), but security.md:55 doesn't.	main.rs:1261; docs/security.md:55	Open: add one line to security.md
3	note	A refused navigation (`http` to a listed host, `file:`, `javascript:` and so on) only writes to stderr; the user sees nothing happen. A failed browser launch is ignored too (`let _ = open_in_browser`).	main.rs:1306–1317	Open: low risk
4	note	I couldn't confirm the CI result or check the Windows behavior: there's no Windows here and no GitHub API access to the repo. The inventory says CI run 2235 at 524e6a3 passed its Rust jobs (FEATURE_INVENTORY.md:1428, for another feature). 524e6a3 includes #259 and #455, but main.rs has changed since (291+/22−). I treated that line as a claim and didn't check it.	ci.yml:281	Needs a look at the latest main CI run's `shell` job

Documented limitations (only for Accepted with Documented Limitations)#

Limitation	Where users are told	Backlog item that tracks it
Pages on an identity provider that aren't part of sign-in (MFA registration, managing a personal account) open in the default browser.	docs/security.md:55	PB-008 / HD-027
Sub-resources and frames inside allowed pages aren't filtered.	Code comment only (finding 2)	PB-008
Not exercised in the running app by this review.	hardening/backlog.md:132 (manual check recorded in the PR)	Tenant smoke test

Inventory and backlog changes#

Features changed: FEAT-FAB-05: replace the limitation with the exact-host rule, add fabric_nav_tests and ci.yml shell job as evidence, and update the next action (finding 1).
Backlog item state: PB-008 In review → Accepted with Documented Limitations, on the product owner's decision.