fix(desktop): embedded UI auto-login against a joined brain + isolate the store in desktop tests - #134
Conversation
…olate the store in desktop tests operatorToken embedded a sid from the local session store, which a remote brain validates against its own store — always rejected, so the artist UI showed a login screen even with imported secrets. Mint a sid-less token in receiver-only mode (TTL-bounded, not revocable). Desktop tests now run against a temp APPSTASH_BASE_DIR (jest setupFiles) instead of the developer's real ~/.wavegrid. deleteProject also removes the project's secrets file. Co-authored-by: Dan Lynch <pyramation@gmail.com>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
Review complete. 🟡 2 medium 💬 Inline comments (2)
This PR adds a receiver-mode branch in the desktop operator-session so an operator embedded on a joined remote brain can obtain a Core finding 1: the sid-less receiver-mode admin token (
Reviewed commit: 7dc65c1 |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds receiver-only operator session mode with a sid-less admin JWT against joined brains, plus a secrets file purge on project deletion with associated test isolation and coverage.
Key findings
- 🟡 Non-revocable 12h admin operator token — operator-session.ts:46
- 🟡 Best-effort secret purge can silently leak keys — projects.ts:109
| if (store.getProjectConfig(project)?.receiver?.server) { | ||
| return signJwt(account.username, { | ||
| role: account.role, | ||
| ttlSec: Math.floor(TTL_MS / 1000) | ||
| }); | ||
| } |
There was a problem hiding this comment.
🟡 security · medium
Non-revocable 12h admin operator token
In receiver mode operatorToken mints a role: admin token with no sid against a joined brain (packages/desktop/src/main/operator-session.ts:46-50), and the brain's WebSocket upgrade accepts a sid-less JWT by signature alone with no live-session, role, or user-existence check (packages/server/src/server.ts:314-329).
Revoking the operator's sessions or demoting/removing the account does not kill an already-distributed token, which stays valid for the full 12h TTL_MS; a leaked token therefore grants admin show control that cannot be cut off server-side, unlike the session-bound local path.
📋 Prompt for AI Agents
In packages/desktop/src/main/operator-session.ts lines 46-51, the receiver-mode branch mints a sid-less role:admin JWT valid for TTL_MS (12h) that the remote brain's WebSocket upgrade accepts by signature alone (server.ts:314-329) with no session/role/user check. Reduce the blast radius by shortening ttlSec for this branch (e.g. re-mint on each embedded page load) and add a comment recording that this token cannot be revoked from Access→Sessions nor invalidated by demoting/removing the operator, so it is bounded only by expiry.
| fs.rmSync(projectSecretsFile(paths, name), { force: true }); | ||
| } catch { | ||
| /* best effort */ | ||
| } |
There was a problem hiding this comment.
🟡 security · medium
Best-effort secret purge can silently leak keys
The new fs.rmSync(projectSecretsFile(...)) runs inside the same catch {} that already swallowed the project-dir removal, and it executes after the registry entry is dropped — so deleteProject returns true even when the secret purge is skipped or fails (packages/settings/src/projects.ts:107-112).
Impact: a "deleted" project's jwtSecret/receiverKey can remain at config/secrets/<project>.json and be resurrected if the same-named project is re-created, since generateSecrets only back-fills missing values.
📋 Prompt for AI Agents
In packages/settings/src/projects.ts deleteProject() (lines 101-115), the secrets file purge (line 109) is inside the same try/catch as the project-dir removal and runs after the registry write, so a failed or short-circuited removal is silently reported as success. Move the secrets removal out of the best-effort block and before writeRegistry, and on failure log/rethrow so callers know the signing key was not purged.
1 participant
Summary
Upstreams the fixes found bringing up the Grace receiver laptop (receiver-only mode against
wss://hipzap.com), plus two small follow-ups the brief suggested.1. Auto-login never worked against a remote brain.
operatorToken()minted the embedded-UI JWT with asidfrom a session created in the laptop's store; the remote brain checkssidagainst its store, so the token was always rejected ("Session expired or revoked") and the operator got the login screen even with matchingjwtSecret. Fix: in receiver-only mode (receiver.serverset) mint a sid-less token and skip the phantom local session — both/api/meand the WS upgrade guard the session lookup withif (payload.sid), so it authenticates on signature alone.Trade-off: that token can't be revoked from Access → Sessions; only the 12h TTL bounds it.
2. Desktop tests wrote to the developer's real
~/.wavegrid.openStore()withoutAPPSTASH_BASE_DIRis the real store; a jest run createddesk-auth/no-users/joined-brainprojects and leftjoined-brainactive — repointing the show laptop at a 6-cannon layout with a key the brain rejects. Nowpackages/desktop/jest.setup.js(jestsetupFiles) pinsAPPSTASH_BASE_DIRto a temp dir for every desktop suite, and the two affected suites also pin it locally.3.
deleteProjectorphaned secrets. It removeddata/projects/<name>but leftconfig/secrets/<name>.json, so a later project with the same name silently inherited the oldjwtSecret/receiverKey. Now removed alongside (best-effort), with a test.Not verified: the embedded BrowserView rendering without a login prompt in the running Electron app — the brief's author probed the live brain's
/api/mewith a sid-less token (ok) vs. a local-sid token (rejected), and unit tests cover the token shape. Desktop: tsc clean, 99/99 jest; settings 85/85.Link to Devin session: https://app.devin.ai/sessions/c4872c2982734093bdee80d04a7d5ceb
Open in Devin Desktop: https://app.devin.ai/desktop/session/c4872c2982734093bdee80d04a7d5ceb?variant=devin
Requested by: @pyramation