fix(server): owner-trust sweep — close dashboard privilege escalation + 4 cross-tenant holes - #291
Open
sebbsssss wants to merge 8 commits into
Open
fix(server): owner-trust sweep — close dashboard privilege escalation + 4 cross-tenant holes#291sebbsssss wants to merge 8 commits into
sebbsssss wants to merge 8 commits into
Conversation
The local requireOwner gate compared a CLIENT-SUPPLIED ?wallet= to the hardcoded (and publicly revealed — GET /auth + source constant) OWNER_WALLET with no proof of ownership. Any logged-in Privy user could append ?wallet=<OWNER_WALLET> and gain full bot-owner admin: agent-fleet CRUD AND remote task execution (executeTaskManually, which spends budget and drives autonomous agents). GET /agents separately scoped a private agent_keys read by the raw ?wallet=, leaking another tenant's registered agents. Fix: gate owner routes with requireOwnership (proves the caller owns the claimed wallet via Privy-linked wallets / email-only registered agent / Cortex clk_ key) then assert req.verifiedWallet === OWNER_WALLET. GET /agents now scopes to the caller's verified wallet, never a client param. A forged ?wallet= no longer satisfies any gate. Regression test (7 cases): a non-owner forging ?wallet=<OWNER_WALLET> gets 403 and never reaches executeTaskManually; a user who owns their own (non-owner) wallet is still denied; the proven owner passes; GET /agents cannot enumerate another tenant. Resolves project_dashboard_wallet_privesc.md / task_e888393f. apps/server tsc clean; dashboard route test 7/7. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ryption Owner-trust sweep (companion to the dashboard privilege-escalation fix). Four older routes scoped private data by a CLIENT-SUPPLIED wallet without proving the caller owns it: - chat (chatAuth): an authenticated Privy user passing ?wallet=<victim> got a chat session scoped to the victim's memories (read back via responses) while billing the victim. Now resolves the caller's Privy-linked wallets and rejects an unlinked claim (mirrors requireOwnership's Privy path). - topup (topupAuth): same pattern let a user read another wallet's balance and top-up history. Same ownership check added. - upload: had NO auth — only an ALLOWED_WALLETS allowlist checked against a raw ?wallet=. Since wallet addresses are public, anyone could read an allowlisted wallet's uploads and INJECT memories into its brain. Now requirePrivyAuth + requireOwnership; the allowlist is checked against req.verifiedWallet. - encryption (revoke/redelegate/revoke-all): read req.verifiedWallet but only mounted requirePrivyAuth (which never sets it) -> always 401: the revoke control plane was silently dead. Its tests were green only because they mocked requirePrivyAuth to set verifiedWallet (mock-hides-reality). Added requireOwnership so verifiedWallet is set and the handler's own mem.owner_wallet ownership check is reachable. The cortex clk_ API-key paths (chat/topup) were already correct and are untouched. encryption test updated to mock requireOwnership (pass-through). apps/server tsc clean; encryption 10/10; no previously-passing test regressed (2 topup + 5 chat-conversation failures are pre-existing, confirmed via stash). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The encryption control plane (revoke / redelegate / revoke-all) now mounts requireOwnership so req.verifiedWallet is actually set (before the fix it was only ever read, never set -> the routes were silently dead at 401). The prior test mocked requireOwnership as a pure no-op AND mocked requirePrivyAuth to set verifiedWallet itself, so it could not prove the route depended on requireOwnership at all — reverting the fix left it green. Rewrite to faithfully SIMULATE the ownership contract (mirrors dashboard.routes.test): requirePrivyAuth sets ONLY req.privyUser, and requireOwnership is the ONLY source of verifiedWallet, set ONLY to a wallet the caller provably owns (a forged ?wallet=/body.wallet is 403, no session is 401). Adds, per route: - forged ?wallet=<victim> (and body.wallet) is blocked at requireOwnership, before any DB read or revoke (the privilege-escalation attack) - a "depends on requireOwnership" case: with verifiedWallet unset the handler 401s and never revokes — proving no other source of the wallet - handler defense-in-depth: a proven owner still gets 403 on a memory owned by another wallet (mem.owner_wallet mismatch) - revoke-all is scoped to the verified wallet and can never enumerate or mass-revoke another tenant's rows Verified by reverting the fix (dropping requireOwnership from the chains): 13/19 go red, including the forged-wallet and legit-owner paths. With the fix in place 19/19 pass. apps/server tsc clean; dashboard.routes.test still 14/14. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pins the d317a84 fix to chatAuth's Privy path: a client-supplied ?wallet= must be proven-owned (via resolveWalletsForDid) before the session scopes to it. - CRITICAL: an authed Privy user forging ?wallet=<VICTIM> is rejected (403), and the victim's conversations are never queried (DB owner_wallet filter is captured and asserted empty). RED against the pre-fix trust-the-client code (returns 200 + victim scope); GREEN with the ownership check. - legit owner ?wallet=<WALLET_A> (caller provably owns it) → 200, scoped to A. - Cortex clk_ Bearer path still works → 200, scoped to the agent owner_wallet, Privy resolver untouched. - no-?wallet= Privy user falls back to DID agent lookup; malformed ?wallet= → 400 before any lookup/DB read; resolver-throws fails closed (500, never scoped to the claim); missing auth → 401. chatAuth is module-internal, so it's driven through the mounted chat router; resolveWalletsForDid is mocked for a controllable linked-wallet set (no network). Self-contained file; existing chat-routes-conversation suite unchanged (13 pass / 5 pre-existing fail). apps/server tsc clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds 7 cases to the privilege-escalation suite covering write routes and the body-wallet forgery vector, which the existing 7 cases (task-execution + GET /agents) did not exercise: - PUT /agents/:id and DELETE /agents/:id with a forged ?wallet=<OWNER> → 403 (pins the `ownerOnly` array on mutations, not just /tasks) - forged wallet smuggled via JSON body.wallet on POST /agents and /tasks/:id/execute → 403, executeTaskManually never called - a proven non-owner (owns their own wallet) cannot mutate the fleet - positive path: the proven owner passes the gate on PUT /agents/:id - the no-?wallet= resolved-owner path (clk_/DID) still reaches execution Faithful to the real requireOwnership contract: a claimed wallet the caller does not own is rejected before any DB scoping. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… victim billing Regression for the topupAuth Privy-path ownership check. An authenticated Privy user passing ?wallet=<victim> they do not own now gets 403 before any DB query, so GET /balance and GET /topup/history cannot leak another user's balance or top-up history. The legitimate owner (resolver-confirmed wallet) and the clk_ API-key path still return 200. Mocks @clude/brain/auth/privy-wallet-resolver (resolveWalletsForDid) with a controllable linked-wallet set — no network. Asserts fail-closed on resolver error (500, no leak) and 400 on malformed wallet before any ownership/DB work. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pins that the file-upload → memory-ingestion pipeline scopes strictly to req.verifiedWallet (proven ownership) and never to a client-supplied ?wallet= / multipart wallet field. Covers: - no auth → 401 on /batches, /process, /batch/:id, /check-access - authed non-owner forging ?wallet=<allowlisted-victim> → 403 on every route, and POST /process inserts NOTHING + never kicks the processor (no memory injection into a victim's brain) - proven owner path works (Privy-linked ?wallet= and Cortex clk_ key with no ?wallet=), with every inserted row owned by the verified wallet - allowlist still enforced on the verified wallet (proven-but-unlisted owner → 403) requireOwnership is mocked faithfully (a claimed wallet the caller does not own is rejected); verified RED against a vulnerable client-trust simulation (4 CRITICAL tests fail), GREEN against the fixed route. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…onfirmed safe) Backtest deep-audit produced these as proof the optionalOwnership-guarded routes have no client-wallet-trust hole. 22 cases, all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why this is its own PR
Lifted out of the marketplace branch so the board can fast-track the security fix independently of the larger Memory Pack Marketplace PR. One finding is a live, currently-exploitable CRITICAL.
The vulnerabilities (all in pre-existing routes, none in new marketplace code)
dashboard?wallet=<OWNER_WALLET>(a public address, also echoed byGET /auth) → full bot-owner admin: agent-fleet CRUD + remote task execution (executeTaskManually— spends budget, drives the autonomous agents)chat?wallet=<victim>→ chat session scoped to the victim's memories (read back via responses) + bills the victimtopupuploadALLOWED_WALLETSallowlist on a raw?wallet=. Wallet addresses are public ⇒ read others' uploads + inject memories into the bot's brainencryptionverifiedWalletbut only mountedrequirePrivyAuth(never sets it) → silently always-401 (control plane dead); green tests only via averifiedWallet-setting mockRoot cause & fix
requirePrivyAuthsetsreq.privyUseronly; onlyrequireOwnership/optionalOwnershipsetreq.verifiedWallet— and only to a wallet the caller provably owns (Privy-linked / Cortexclk_key). Every fix derives the owner fromverifiedWallet, never a client-supplied?wallet=/?owner=. The Cortex API-key paths were already correct and are untouched.Resolves the long-standing
dashboard_wallet_privescissue (was flagged unfixed).Verification
clk_path still works, proven per route.memory,graph,wiki-packs,memory-packs,pmp,pmp-packs) found no missed holes.stagingbase;apps/servertypecheck clean.🤖 Generated with Claude Code