diff --git a/app/src-tauri/src/cdp/session.rs b/app/src-tauri/src/cdp/session.rs index 20af77266..75e52cf9c 100644 --- a/app/src-tauri/src/cdp/session.rs +++ b/app/src-tauri/src/cdp/session.rs @@ -309,6 +309,28 @@ async fn run_session_cycle( ]); } + // Slack Huddles need the same media-capture set as Meet: + // - audioCapture / videoCapture: getUserMedia for huddle voice + + // optional camera tile. Without these, the huddle pre-flight + // enumerateDevices returns empty and the join button silently + // no-ops. + // - displayCapture: getDisplayMedia for in-huddle screen share. + // - clipboardReadWrite: huddle invite-link copy + slash-command + // paste flows. + // Mirrors the gmeet pattern from #1054. The huddle popup paint + // lifecycle bug is tracked separately under #1074 / the CEF + // tracking issue — granting these perms now means once the paint + // bug clears, the huddle is functional immediately rather than + // requiring a follow-up perms wire-up. + if origin_host_is(&origin, "app.slack.com") { + perms.extend_from_slice(&[ + "audioCapture", + "videoCapture", + "displayCapture", + "clipboardReadWrite", + ]); + } + if let Err(e) = cdp .call( "Browser.grantPermissions", @@ -466,6 +488,29 @@ mod tests { assert!(!origin_host_is("file:///etc/hosts", "meet.google.com")); } + /// The slack-huddle media-perm grant is host-gated by + /// `origin_host_is(origin, "app.slack.com")`. Lock the matcher so a + /// future refactor can't silently widen / narrow the set of origins + /// that get `audioCapture`/`videoCapture`/`displayCapture` etc. + #[test] + fn origin_host_is_matches_app_slack_com_for_huddle_grant() { + // canonical slack web origin + assert!(origin_host_is("https://app.slack.com", "app.slack.com")); + // case-insensitive (matches Url-normalised input + raw header) + assert!(origin_host_is("https://APP.SLACK.COM", "app.slack.com")); + // explicit port tolerated + assert!(origin_host_is("https://app.slack.com:443", "app.slack.com")); + + // marketing site / files CDN must NOT receive media perms — only + // the huddle-bearing app origin + assert!(!origin_host_is("https://slack.com", "app.slack.com")); + assert!(!origin_host_is("https://files.slack.com", "app.slack.com")); + // unrelated provider + assert!(!origin_host_is("https://meet.google.com", "app.slack.com")); + // non-http schemes never match (e.g. about:blank popup placeholder) + assert!(!origin_host_is("about:blank", "app.slack.com")); + } + #[test] fn target_match_accepts_placeholder_and_real_provider_fragments_only_for_same_account() { assert!(target_matches_account_url( diff --git a/app/src-tauri/src/webview_accounts/mod.rs b/app/src-tauri/src/webview_accounts/mod.rs index 746b29439..7953c8be3 100644 --- a/app/src-tauri/src/webview_accounts/mod.rs +++ b/app/src-tauri/src/webview_accounts/mod.rs @@ -225,6 +225,30 @@ fn popup_should_stay_in_app(provider: &str, url: &Url) -> bool { } } +/// `true` if `scheme` is a known provider native-desktop-app deep-link +/// scheme. We suppress these instead of routing them to the system +/// browser because macOS hands them to the native provider app +/// (e.g. `slack://magic-login/` signs the native Slack app into +/// the workspace, breaking embedded-webview isolation: the workspace's +/// session ends up inside the native client even though the user only +/// signed in via OpenHuman's embedded webview). +/// +/// The HTTPS fallback in each provider's web flow handles sign-in +/// without the deep link, so suppression is safe — the page just +/// continues on the next link in the sequence. +/// +/// Caller contract: only suppress when [`rewrite_provider_deep_link`] +/// has already returned `None` for the URL. Schemes we DO know how to +/// rewrite into a web-client URL (e.g. `zoomus://`) must take the +/// rewrite path first; those flows expect to stay in-app, not be +/// silently dropped. +fn is_provider_native_deep_link_scheme(scheme: &str) -> bool { + matches!( + scheme, + "slack" | "discord" | "tg" | "msteams" | "zoomus" | "zoommtg" + ) +} + /// `true` if a popup request should be denied AND the parent webview /// should be navigated to the popup URL instead. /// @@ -301,6 +325,10 @@ fn redact_navigation_url(url: &Url) -> String { safe.set_fragment(None); safe.to_string() } + +fn redact_native_deep_link_url(url: &Url) -> String { + format!("{}://", url.scheme()) +} /// Unwrap provider-side "link safety" redirects so the system browser /// lands on the real destination. /// @@ -1554,6 +1582,20 @@ pub async fn webview_account_open( if url_is_internal(&nav_provider, url) { true } else { + // Suppress provider native-desktop-app deep-link schemes that + // we don't know how to rewrite. macOS would otherwise hand + // these to the native provider app — `slack://magic-login/…` + // signs the native Slack app into the workspace, breaking + // embedded-webview isolation (#1074). The web flow's HTTPS + // fallback handles sign-in without the deep link. + if is_provider_native_deep_link_scheme(url.scheme()) { + log::warn!( + "[webview-accounts] suppressing native-app deep-link scheme={} url={} (would breach workspace isolation)", + url.scheme(), + redact_native_deep_link_url(url) + ); + return false; + } let target = unwrap_provider_redirect(url) .map(|u| u.to_string()) .unwrap_or_else(|| url.to_string()); @@ -1638,6 +1680,19 @@ pub async fn webview_account_open( ); NewWindowResponse::Allow } else { + // Suppress provider native-desktop-app deep-link schemes that + // we don't know how to rewrite (matches the on_navigation + // fallback). Without this, a `slack://...` popup would land + // in the native Slack app via macOS's URL handler and + // breach embedded-webview workspace isolation (#1074). + if is_provider_native_deep_link_scheme(url.scheme()) { + log::warn!( + "[webview-accounts] suppressing native-app deep-link scheme={} url={} (would breach workspace isolation)", + url.scheme(), + redact_native_deep_link_url(&url) + ); + return NewWindowResponse::Deny; + } let target = unwrap_provider_redirect(&url) .map(|u| u.to_string()) .unwrap_or_else(|| url.to_string()); @@ -2732,6 +2787,87 @@ mod tests { .is_none()); } + // ── is_provider_native_deep_link_scheme: native-app suppression ─── + // + // These guard the workspace-isolation contract from #1074: provider + // native-desktop-app deep-link schemes must NEVER reach the system + // browser, because macOS hands them off to the native provider app + // which then signs the user into the workspace using session tokens + // intended only for the embedded webview (see slack://magic-login + // smoking gun in the #1074 trace). + + #[test] + fn deep_link_scheme_matches_known_provider_native_apps() { + // Slack desktop ("slack://T01.../magic-login/") + assert!(is_provider_native_deep_link_scheme("slack")); + // Discord desktop + assert!(is_provider_native_deep_link_scheme("discord")); + // Telegram desktop ("tg://join?invite=…") + assert!(is_provider_native_deep_link_scheme("tg")); + // Microsoft Teams + assert!(is_provider_native_deep_link_scheme("msteams")); + // Zoom client (both variants registered by the installer) + assert!(is_provider_native_deep_link_scheme("zoomus")); + assert!(is_provider_native_deep_link_scheme("zoommtg")); + } + + #[test] + fn deep_link_scheme_rejects_legitimate_external_schemes() { + // HTTP(S) — the bread-and-butter external link. + assert!(!is_provider_native_deep_link_scheme("https")); + assert!(!is_provider_native_deep_link_scheme("http")); + // Mail clients are legit external — must NOT be suppressed. + assert!(!is_provider_native_deep_link_scheme("mailto")); + // Telephone / sms are legit external too. + assert!(!is_provider_native_deep_link_scheme("tel")); + assert!(!is_provider_native_deep_link_scheme("sms")); + // about: / data: / blob: handled elsewhere; never deep-link. + assert!(!is_provider_native_deep_link_scheme("about")); + assert!(!is_provider_native_deep_link_scheme("data")); + assert!(!is_provider_native_deep_link_scheme("blob")); + // Empty / unrelated string. + assert!(!is_provider_native_deep_link_scheme("")); + assert!(!is_provider_native_deep_link_scheme("file")); + } + + #[test] + fn deep_link_scheme_matches_real_world_slack_magic_login_url() { + // Real slack://-flavoured magic-login URL recorded in the + // #1074 CDP trace. The handler must catch it before + // open_in_system_browser is reached. + let parsed = url("slack://T01CWHNCJ9Z/magic-login/11035712490054-abc"); + assert!(is_provider_native_deep_link_scheme(parsed.scheme())); + } + + #[test] + fn deep_link_scheme_does_not_match_https_app_slack_com() { + // The web-flow URL stays untouched — only the slack:// scheme is + // suppressed; ordinary HTTPS slack navigations route normally. + let parsed = url("https://app.slack.com/client/T01CWHNCJ9Z"); + assert!(!is_provider_native_deep_link_scheme(parsed.scheme())); + } + + /// Locks the contract that zoomus:// stays on the rewrite path + /// (handled by `rewrite_provider_deep_link` for the "zoom" provider) + /// rather than being silently suppressed. + /// + /// The wiring in on_navigation / on_new_window calls + /// `rewrite_provider_deep_link` BEFORE the suppress check, so a + /// rewriteable scheme is rewritten and never reaches the suppress + /// branch. This test pins both halves of that contract: the rewrite + /// still succeeds for zoom, AND the scheme is recognised as a + /// native-app deep-link (so if a future provider config dropped the + /// rewrite, suppression would be the safe fallback rather than + /// leaking to the system browser). + #[test] + fn zoomus_join_still_rewrites_and_is_recognized_as_native_scheme() { + let zoom_url = url("zoomus://zoom.us/join?action=join&confno=9819254358"); + assert!(is_provider_native_deep_link_scheme(zoom_url.scheme())); + let rewritten = rewrite_provider_deep_link("zoom", &zoom_url) + .expect("zoom rewrite should still succeed before suppress branch"); + assert_eq!(rewritten.as_str(), "https://app.zoom.us/wc/join/9819254358"); + } + #[test] fn rewrite_percent_encodes_reserved_chars_in_pwd() { // Zoom tokens commonly contain `&` / `=` / `%` / `#` / `+` which diff --git a/docs/qa/SLACK-PARITY.md b/docs/qa/SLACK-PARITY.md index 70876dec0..04de8a812 100644 --- a/docs/qa/SLACK-PARITY.md +++ b/docs/qa/SLACK-PARITY.md @@ -23,7 +23,7 @@ | 2 | **Messaging** — channels, DMs, group DMs; threads | ✅ pass | User confirmed send-receive works in DMs/channels. Threads navigable. UI parity matches web Slack. | Memory-side extraction blocked by #7 (scanner doesn't run); will re-verify post-fix. | | 3 | **Reactions & emoji** — picker opens; reactions post correctly | ✅ pass | User confirmed reactions + threads work end-to-end. UI render confirmed in screenshot (`🙏 1`, `👍 1`). | Pre-audit "extraction missing" claim DEFERRED — unverifiable until #7 fix lands and scanner actually runs. Will re-recall memories post-fix; if reactions show in memory docs, the static audit was wrong (like #4 was). Only if missing post-#7-fix do we touch `extract.rs`. | | 4 | **File sharing** — upload, download; image previews | ✅ pass | User confirmed upload/download/preview all work. | Pre-audit "allowed_hosts mismatch" was a **false alarm**: `slackb.com` IS the working CDN host. Spec's `slack-imgs.com` + `slack-files.com` are stale — DO NOT change `webview_accounts/mod.rs:101`. Drop that fix from the plan. | -| 5 | **Huddles** — popup spawn (about:blank → huddle URL whitelisted); audio/video; popup cleanup on end | ❌ **fail** | Huddle clicked → popup spawned (CEF child window opens) → window stays **blank white forever**. Log: `[webview-accounts] new-window request about:blank → in-app popup (provider=slack)` then `on_context_created browser_id=4 origin=about:blank#openhuman-acct-...` and **no further navigation**. Slack's `popup.location = huddleURL` cross-window write is not propagating to CEF child. | **CHILD ISSUE TO FILE**: title `[Bug] webview/slack: huddle popup stays blank — about:blank→huddle URL navigation lost`. Reproduces 100% on main `b11b8f33`. Hypothesis: CEF about:blank popups don't honor parent-set `location` writes; need to intercept `window.open` early and pre-navigate to huddle URL, or rewrite about:blank → huddle URL in `webview_accounts/mod.rs:popup_should_stay_in_app` arm. | +| 5 | **Huddles** — popup spawn (about:blank → huddle URL whitelisted); audio/video; popup cleanup on end | ❌ **fail** (paint lifecycle bug; deferred to CEF tracking issue) | `OPENHUMAN_SLACK_HUDDLE_PROBE=1` debug instrumentation captured the popup lifecycle 12:28:29–12:28:37: `windowOpen url=about:blank`, `target_created`, `target_info_changed url=about:blank` (and stays at about:blank for 8s), `target_destroyed`. **No `Page.frameNavigated` ever fires on the popup target** — URL never leaves `about:blank`. Combined with user-observed behaviour ("name + voice + camera tile briefly render then go white"), the working hypothesis (CEF doesn't honor cross-window `popup.location = huddleURL`) was **invalidated**. Real root cause: Slack uses `popup.document.write(html)` to inject huddle UI into the same `about:blank` document — there is no URL navigation to intercept; the bug is CEF's paint/compositor lifecycle on same-document popups. | Tracked in **[#1074](https://github.com/tinyhumansai/openhuman/issues/1074)** (popup paint) → root-cause refined to **[#1079](https://github.com/tinyhumansai/openhuman/issues/1079)** (CEF same-document popup paint lifecycle). The #1074 PR ships partial fixes (media perms grant + slack:// deep-link isolation, see "Fixes shipped in this PR" below); full huddle paint resolution gated on the CEF tracking issue. Joins the runtime-gap family in `feedback_cef_runtime_gaps.md` (Web Push, BrowserChannel, MediaPipe segmentation, **same-document popup paint**). | | 6 | **Notifications** — native OS notifications; per-channel mute; DND; `notification_settings` toggle | ❌ **fail** | User received Slack DM from Shanu while NOT focused on OpenHuman. macOS Notifications perm granted for OpenHuman.app. Result: **no native toast fired**. Log shows zero `forward_native_notification` / `webview_notification` events at message-arrival time. CEF shim `installed shims browser_id=3 origin=https://app.slack.com/client` was registered, but page-side `new Notification(...)` never reached Rust handler. | **CHILD ISSUE TO FILE**: title `[Bug] webview/slack: native OS notifications never fire — page→Rust bridge broken`. Repro: signed-in Slack workspace, OpenHuman backgrounded, peer sends DM → no toast. Hypothesis: (a) Slack web suppresses Notification when its own permission state for the origin isn't `"granted"`, or (b) CEF Notification shim wrap doesn't actually call `forward_native_notification`. Need to verify CEF `Notification.permission` value at slack.com origin and confirm the constructor wrap path. Pre-audit gaps (per-channel mute missing; default=true) are still valid but moot until basic toast path works. | | 7 | **Memory ingestion** — IDB scanner; `memory_doc_ingest` posts; current behavior groups by `channel_id` (per-day grouping deferred) | ❌ **fail (scanner never spawns)** | RPC call `openhuman.memory_recall_memories {"namespace":"slack-web:29da7de6...","limit":20}` returns `{memories:[]}`. Log shows: `[webview-accounts] slack ScannerRegistry not in app state` immediately after Slack account opens. Zero `memory_doc_ingest` events ever fired. **Root cause**: `app/src-tauri/src/lib.rs:998` does `manage(std::sync::Arc::new(slack_scanner::ScannerRegistry::new()))` — but `slack_scanner::ScannerRegistry::new()` already returns `Arc` (`mod.rs:744-746`). Result: managed state is `Arc>`. Lookup at `webview_accounts/mod.rs:1751` is `try_state::>()` → returns None → scanner never spawns. WhatsApp/Discord/Telegram lines (997/999/1000) correctly use the bare `ScannerRegistry::new()` (no extra Arc wrap). Slack alone is double-wrapped. | **ONE-LINE FIX**. Change `lib.rs:998` from `.manage(std::sync::Arc::new(slack_scanner::ScannerRegistry::new()))` to `.manage(slack_scanner::ScannerRegistry::new())`. Post-fix: confirmed scanner ingests one doc per channel (`emit_and_persist` at `mod.rs:230` explicitly groups by `channel_id` only — no per-day split); the `(channel_id, day)` shape from the original spec remains a deferred follow-up rather than a regression. Reactions/threads extraction gaps still apply but blocked by getting scanner running first. | | 8 | **DOM snapshot** — fast-tick captures unread badges + channel list | ✅ pass | Sidebar matches web Slack: channel list (general, random, team-backend/frontend/product, notify-frontend-gi/se, External connections), DMs (Sanil 1, Alan, Aniketh, Cyrus, Mega Mind, Shanu, Steven), DMs+Activity nav badges (1 each), per-DM unread badge on Sanil (1). | DOM extraction working as designed. | @@ -88,10 +88,12 @@ These were confirmed by static read of `main` before smoke. The smoke run will v | A | `lib.rs:998` double-Arc-wrapped `slack_scanner::ScannerRegistry::new()` (which already returns `Arc`). Tauri lookup at `webview_accounts/mod.rs:1751` for `Arc` missed the `Arc>` shape. Scanner never spawned. | Drop the redundant outer `Arc::new(...)` so the managed type matches the lookup. Mirrors the pattern already used for whatsapp/discord/telegram (lines 997/999/1000). | `app/src-tauri/src/lib.rs:998` | ✅ post-fix log shows `[sl] scanner up account=… interval=30s` and no `slack ScannerRegistry not in app state` warning | | B | Native Slack notifications never fired. CEF Notification permission for `slack.com` origin remained `default`; the existing JS shim only masked `Notification.permission === "granted"` for the page check, but the real CEF Notification path silently no-op'd at the C++ level when no actual grant existed. | Issue a browser-scoped `Browser.grantPermissions(["notifications"])` CDP call against the provider's origin right after attach. Adds an `origin_of()` helper to extract `scheme://host[:port]` from `real_url`. | `app/src-tauri/src/cdp/session.rs` (new helper + grant call between shim injection and `Page.enable`) | ✅ post-fix log shows `[cdp-session][…] granted notifications for origin=https://app.slack.com`; macOS toasts now fire when OpenHuman is unfocused | | D | (Surfaced once Bug A let the scanner run.) Slack's client router does `pushState('/client//')` shortly after first load, stripping the `#openhuman-account-` fragment from the page-target URL. Scanner's `find` matcher `starts_with(prefix) && ends_with(fragment)` failed every tick after pushState. Memory ingest stayed empty. | Relax the matcher: try strict (prefix + fragment) first, fall back to any same-origin Slack page target. Per-account `data_directory` isolation guarantees one Slack page-target per origin per account, so the broader match is safe. Same fix in both `scan_once` and `dom_scan_once`. | `app/src-tauri/src/slack_scanner/mod.rs:114-135` (`scan_once`) and `:740-755` (`dom_scan_once`) | ✅ post-fix log shows multiple `[sl][…] memory upsert ok namespace=slack-web:… key= msgs=` lines (general/random/team-product channels + alan/sanil/shanu/elvin516/nikhil DMs); RPC `openhuman.memory_recall_memories {namespace:"slack-web:"}` returns 7 docs | +| C | Slack `slack://T.../magic-login/` deep-links from inside the embedded Slack webview were being routed to `open_in_system_browser`. macOS handed them to the native Slack desktop app, which consumed the magic-login token and signed the workspace into the native client — breaking embedded-webview workspace isolation. Same risk for `discord://`, `tg://`, `msteams://`. Discovered while instrumenting #1074. | Introduce `is_provider_native_deep_link_scheme()` helper. In both the on_navigation external-fallback and on_new_window external-fallback paths, suppress these schemes BEFORE the `open_in_system_browser` call — the page's HTTPS fallback completes sign-in without the deep link. `zoomus://` / `zoommtg://` continue to take the existing `rewrite_provider_deep_link` path (locked by `zoomus_join_still_rewrites_and_is_recognized_as_native_scheme`). | `app/src-tauri/src/webview_accounts/mod.rs` (new helper near `popup_should_stay_in_app`; wired into both navigation paths) | ✅ unit tests in `webview_accounts::tests::deep_link_scheme_*`; manual smoke deferred to merge of [#1074](https://github.com/tinyhumansai/openhuman/issues/1074) PR | +| E | Slack Huddles' getUserMedia / getDisplayMedia surface had no browser-level permission grants — `enumerateDevices()` returned empty inside the huddle popup, the join button silently no-op'd. Same class as Bug B (notifications) but for media. Mirrors the gmeet pattern landed in PR #1054. | Extend the per-origin grant list in `cdp/session.rs::run_session_cycle` to add `audioCapture` / `videoCapture` / `displayCapture` / `clipboardReadWrite` for `app.slack.com` alongside `notifications`. | `app/src-tauri/src/cdp/session.rs` (slack arm in the `Browser.grantPermissions` block) | ✅ unit test `origin_host_is_matches_app_slack_com_for_huddle_grant`; functional verification deferred to the CEF popup-paint fix in [#1079](https://github.com/tinyhumansai/openhuman/issues/1079) (popup currently can't render long enough to use the perms; once it can, mic/cam/screenshare are wired) | ## Out of scope (separate child issues recommended) -- **Huddle popup blank** — `popup = window.open('about:blank'); popup.location = huddleURL` pattern; CEF child popup doesn't honor cross-window location write. Fix shape: CDP listener on parent main-frame nav matching `app.slack.com/huddle/`, force-navigate child via `Page.navigate`. Unbounded debug; deferred. +- **Huddle popup blank** — root cause refined after [#1074 Phase 0 CDP probe](https://github.com/tinyhumansai/openhuman/issues/1074). Slack uses `popup.document.write(html)` to inject huddle UI into the same `about:blank` document, NOT `popup.location = url` — so there is no URL signal to intercept. The bug is CEF's paint/compositor lifecycle on same-document popups (popup paints first frame correctly, then surface goes white). Tracked in **[#1079](https://github.com/tinyhumansai/openhuman/issues/1079)**. Pre-conditions for huddle audio/video are now in place (`Browser.grantPermissions` for `audioCapture`/`videoCapture`/`displayCapture` on `app.slack.com`, see Fixes shipped in this PR row E); huddle becomes functional immediately once the CEF paint bug clears. - **CEF parent webview blanks after huddle interaction + tab switch** — likely shares root cause with the huddle popup; verify after that lands. - **`pnpm dev:app` `restart_app` regression on PR #1007** — orthogonal bug, broader than Slack; affects all dev:app sessions. Requires instrumenting `getActiveUserIdFromCore()` to confirm whether it returns `null` in dev mode and triggers the seed/identity mismatch. - **In-app Slack logout removes the OpenHuman sidebar tile but cookies persist** — UX inconsistency (re-add = already signed in). Decide between (a) keep the tile after in-app logout, or (b) purge the per-account CEF profile on the logout signal. @@ -100,6 +102,6 @@ These were confirmed by static read of `main` before smoke. The smoke run will v ## Sign-off - Tester: oxoxDev -- Result: ✅ Three confirmed bugs (#7 scanner spawn / #6 notifications / Bug-D scanner target match) fixed. Memory ingest end-to-end working for 7 channels/DMs. Five other criteria pass; one skipped (no second workspace); two deferred to separate child issues (huddle, CEF blank-after-huddle). -- Date: 2026-04-29 -- Action items: file follow-up issues for huddle popup + CEF blank-after-huddle if not already tracked. dev:app restart-loop regression also worth a separate ticket. +- Result: ✅ Three confirmed bugs (#7 scanner spawn / #6 notifications / Bug-D scanner target match) fixed. Memory ingest end-to-end working for 7 channels/DMs. Five other criteria pass; one skipped (no second workspace); huddle popup paint deferred to CEF tracking issue [#1079](https://github.com/tinyhumansai/openhuman/issues/1079) (root cause refined from the original "popup.location lost" hypothesis to "Slack uses popup.document.write into about:blank → CEF same-document popup paint dies after first frame"); related slack:// deep-link workspace-isolation leak fixed as Bug C; Slack Huddles media perms grant landed as Bug E (functional once #1079 clears). +- Date: 2026-04-29 (initial smoke); 2026-05-01 (huddle root-cause + Bug C/E follow-ups via #1074 instrumentation) +- Action items: [#1079](https://github.com/tinyhumansai/openhuman/issues/1079) tracks the CEF same-document popup paint lifecycle (the new path for "huddle popup blank"). The legacy "CEF blank-after-huddle on tab switch" entry above likely shares root cause with #1079 — revisit once that lands. dev:app restart-loop regression remains a separate ticket.