Repository navigation
fix(browser): keep in-app browser logins when a WSL or managed SSH host re-pairs - #29610
Conversation
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🔵 Low · up to
|
7d0b11a to
e3b42f8
Compare
|
SSH coverage: this PR changes the browser identity for managed SSH (orcad) hosts, but the PR's own run skipped the SSH Docker lane because of its path filter. I dispatched the e2e workflow on this head (e3b42f8): https://github.com/stablyai/orca/actions/runs/38889312626. Both |
e3b42f8 to
0ad75ff
Compare
a27588f to
b77de09
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
650a3f85-fc20-45d5-a8d2-a29a147f5782
📥 Commits
Reviewing files that changed from the base of the PR and between 7d0b11a7b5fee68b2e0913c9fb7265aa8ba0a2fb and b77de0947e874e01d83a044e403190e39c8ac7ed.
📒 Files selected for processing: 31
config/scripts/pr-e2e-source-routing.mjssrc/main/browser/browser-client-page-command-executor-dependencies.tssrc/main/browser/browser-client-page-command-executor-fencing.test.tssrc/main/browser/browser-client-page-command-executor-test-harness.tssrc/main/browser/browser-client-page-command-executor.test.tssrc/main/browser/browser-client-page-command-executor.tssrc/main/browser/browser-client-page-command-integration.test.tssrc/main/browser/browser-client-page-creation.tssrc/main/browser/browser-client-page-execution-host-supersession.test.tssrc/main/browser/browser-client-page-reconciliation-adapters.test.tssrc/main/browser/browser-client-page-unavailability.test.tssrc/main/browser/browser-client-route-cookie-import-partition-parity.test.tssrc/main/browser/browser-client-route-cookie-import.test.tssrc/main/browser/browser-client-route-cookie-import.tssrc/main/browser/browser-route-partition-binding-file.tssrc/main/browser/browser-route-partition-binding-store-fake.tssrc/main/browser/browser-route-partition-binding-store.test.tssrc/main/browser/browser-route-partition-binding-store.tssrc/main/browser/browser-route-partition-migration.test.tssrc/main/browser/browser-route-partition-migration.tssrc/main/browser/browser-route-partition-storage-retirement.test.tssrc/main/browser/browser-route-session-partition-binding.tssrc/main/browser/browser-route-session-registry-contract.tssrc/main/browser/browser-route-session-registry.tssrc/main/browser/browser-route-session-runtime.tssrc/main/browser/browser-route-session-state.tssrc/main/browser/paired-runtime-browser-authority-identity.tssrc/main/browser/paired-runtime-browser-client-host-composition.test.tssrc/main/browser/paired-runtime-browser-client-host-route-identity.test.tssrc/main/browser/paired-runtime-browser-client-host-runtime.tstests/e2e/ssh-orcad-browser-re-pair-cookies.spec.ts
🚧 Files skipped from review as they are similar to previous changes: 1
- src/main/browser/browser-route-partition-migration.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.
| specs: ['tests/e2e/ssh-orcad-browser-re-pair-cookies.spec.ts'], | ||
| matches: (file) => | ||
| isProductSource(file) && | ||
| /^src\/main\/browser\/(?:paired-runtime-browser-(?:authority-identity|client-host-runtime)|browser-route-partition-(?:migration|binding-store|binding-file)|browser-client-page-creation|browser-client-route-cookie-import)\.ts$/.test( |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the session-registry and session-binding modules to the re-pair route.
The ssh.orcad-browser-re-pair-cookies route does not match src/main/browser/browser-route-session-registry.ts or src/main/browser/browser-route-session-partition-binding.ts. Both files are on the path that this spec exercises:
assertReusableinbrowser-route-session-registry.tsnow decides whether a live partition is reused after a re-pair. Before this change, the same case threwbrowser_route_partition_binding_conflict.persistBrowserRouteSessionBindingandresolveBrowserRouteSessionPartitioninbrowser-route-session-partition-binding.tsforwardcompatibleIdentityand persistaliasFingerprint.
A later PR can break one of these files and the Docker re-pair lane will not run. The author already reported that the path filter skipped the SSH Docker lane for this PR. Add both modules to the pattern.
Proposed fix
--- "a/config/scripts/pr-e2e-source-routing.mjs"
+++ "b/config/scripts/pr-e2e-source-routing.mjs"
@@ -182,7 +182,7 @@
specs: ['tests/e2e/ssh-orcad-browser-re-pair-cookies.spec.ts'],
matches: (file) =>
isProductSource(file) &&
- /^src\/main\/browser\/(?:paired-runtime-browser-(?:authority-identity|client-host-runtime)|browser-route-partition-(?:migration|binding-store|binding-file)|browser-client-page-creation|browser-client-route-cookie-import)\.ts$/.test(
+ /^src\/main\/browser\/(?:paired-runtime-browser-(?:authority-identity|client-host-runtime)|browser-route-partition-(?:migration|binding-store|binding-file)|browser-route-session-(?:registry|partition-binding)|browser-client-page-creation|browser-client-route-cookie-import)\.ts$/.test(
file
)
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /^src\/main\/browser\/(?:paired-runtime-browser-(?:authority-identity|client-host-runtime)|browser-route-partition-(?:migration|binding-store|binding-file)|browser-client-page-creation|browser-client-route-cookie-import)\.ts$/.test( | |
| /^src\/main\/browser\/(?:paired-runtime-browser-(?:authority-identity|client-host-runtime)|browser-route-partition-(?:migration|binding-store|binding-file)|browser-route-session-(?:registry|partition-binding)|browser-client-page-creation|browser-client-route-cookie-import)\.ts$/.test( |
…st re-pairs Client-hosted browser pages for a server Orca deployed (WSL hosts today; managed SSH desktop tabs use the SSH route) now derive their partition from the environment and the server's public key, not the pairing revision and device id that rotate on every re-pair. Existing jars are adopted, and each binding keeps the shipping build's fingerprint (with the stable one as an alias) so a downgraded build still opens the same jar.
b77de09 to
57fe0cd
Compare
ELI5
When Orca runs its own server inside a WSL distro, the in-app browser pages for that host are shown on this computer and keep their own cookie jar here. That jar was named after the current "pairing" between this desktop and the server. Orca pairs again on its own whenever the host's pairing changes or the server drops this desktop, and every re-pair silently switched to a new, empty jar: the user was signed out of every site they had logged into for that host.
Now the jar is named after the host itself (which saved server it is, plus the server's own public key), so a re-pair keeps the same jar. A server with a different key is a different server and still gets its own, empty jar.
What Changed
Mechanism:
paired-runtime-browser-authority-identity.ts(moved out of the client-host runtime): for an environment with anorcadDeployment, the authority identity is a digest of the Orca profile, the environment id and the server's public key. The pairing revision and paired device id, which rotate on each re-pair, are left out. Other environments keep exactly today's identity.browser-route-partition-migration.ts: when the identity differs from what the shipping build derives (the "compatible" identity, today's pairing identity), a binding'sfingerprintalways follows the compatible identity, and the stable fingerprint is stored beside it in a new optionalaliasFingerprintslist. Existing jars are adopted from the current pairing, or from the older per-process scheme, as before.browser-route-partition-binding-store.ts/-file.ts: lookups match the fingerprint or an alias;rebindandsettake an optional alias. Aliases are capped at two and parsed leniently.Why
The jar must outlive a re-pair but never be reachable by a different server. The environment id plus the pinned server key meet both: a re-pair keeps them, a reinstalled or foreign server has a different key.
Keeping the shipping build's fingerprint as the record's primary key, with the stable one as an alias, keeps mixed desktop versions working. An older build reads only
fingerprint, so after a downgrade it still finds the same jar at whatever pairing it is on, instead of opening an empty one or failing withbrowser_route_partition_binding_conflict. Rewriting the fingerprint outright (the existing runtimeId migration's approach) would break a downgrade; a separate alias file would need its own cleanup on removal and eviction.This replaces the earlier version of this PR (pairing history in the environment records, WSL registration GUIDs in the identity, alias-only adoption). Keying on the pinned key alone already separates a re-imported distro whose orcad generated a new key, and it removes the record-schema changes and the registry read that the follow-up #31289 needed.
ref-oss check: other remote editors key per-remote local storage by the durable remote address and never by session credentials; that matches this approach, which did not change.
Linked Issue
N/A (maintainer).
Visual Proof
N/A: no UI change. The behavior is covered by partition and session-registry tests.
Testing
Unit (
paired-runtime-browser-client-host-route-identity.test.ts, binding store and migration tests):fingerprint) reopens it both before and after the re-pair. An older build's rewrite that drops aliases is re-adopted without a binding conflict.End to end: not run live. The managed SSH Docker lane cannot exercise this path (managed SSH tabs never become desktop-hosted pages, confirmed by a spec I wrote and then removed), and the WSL lane drives host RPC only, with no browser. The Docker managed-orcad lane (
ssh-orcad-pairing-repair,ssh-orcad-browser-routing,paired-client-hosted-browser-cookie-survival) passed on this change.Local checks: the full static analysis and typecheck job steps, and the
src/main/browserandsrc/main/ipc/runtime-environmentsunit folders single-worker (the only local failures, inbrowser-manager-tab-identity,browser-manager-viewport-ownership, the GC retention tests andruntime-environment-session-tabs-delta, fail identically on main locally).Known limitations (nothing is deleted in any of them):
AI Disclosure
Review
Agent skill upstream boundary
Notes
No wire change. The only persisted change is the optional
aliasFingerprintsfield in the desktop-local partition binding file; older builds ignore it.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)