fix(vault): restore this-binding in guardVaultAccess for privileged-capable methods

[cammyjee]

Summary

guardVaultAccess (services/vault/guard.ts) wraps every PRIVILEGED_CAPABLE method (getPublicKey, encrypt, decrypt, createHmac, verifyHmac, createSignature, verifySignature, revealCounterpartyKeyLinkage, revealSpecificKeyLinkage, acquireCertificate, proveCertificate, listCertificates) with a privilege check before passing the call through to the real wallet. The pass-through was a bare invocation — value(args, originator) — which drops this. The sibling branch two lines above (for methods not in PRIVILEGED_CAPABLE) already does this correctly via value.bind(target); this branch never got the same treatment.

Real wallet implementations (e.g. @bsv/wallet-toolbox-mobile's SimpleWalletManager) read this internally:

async getPublicKey(args, originator) {
    this.ensureCanCall(originator);
    return await this.underlying.getPublicKey(args, originator);
}

With this unbound, this.ensureCanCall resolves to undefined, and calling it throws undefined is not a function — with no property name in the message, since it's a computed/member call inside a dynamically-evaluated WebView script, which made this very hard to trace from the app side.

Impact: this broke every ordinary, non-privileged getPublicKey call from the in-tab CWI bridge, not just the privileged ones the guard was meant to block — and getPublicKey is the first call a BRC-100 connect makes. That means any website opened in BSV Browser's own in-app browser, using the injected window.CWI substrate (and the same guardVaultAccess call site backing the desktop-pairing WalletClient), silently fails to connect. Found this by chasing a real "wallet won't connect" report through the whole stack, down to a bare err.message with no context, before finding this line.

Why the existing tests didn't catch it

__tests__/vault/guard.test.ts's fakeWallet() stubs are plain closures that ignore this entirely — they can't distinguish a bound call from an unbound one. Added a regression test using a real class instance method that reads this (matching how SimpleWalletManager actually behaves), confirmed it fails against the unfixed code and passes against the fix.

Test plan

• All 16 existing + 1 new test in __tests__/vault/guard.test.ts pass
• New regression test verified load-bearing (fails on unfixed code with the exact this-loss symptom, passes with the fix)
• tsc --noEmit clean (two pre-existing, unrelated errors in __tests__/manual/ predate this change)
• Manually reproduced and confirmed fixed on-device: BRC-100 connect via injected window.CWI now succeeds against a real website

🤖 Generated with Claude Code

[raspi-user]

I reviewed this PR. I think this is a correct and important fix, and I do not see a blocker.

The bug is real: guardVaultAccess already binds non-privileged-capable methods with value.bind(target), but the privileged-capable branch was invoking the underlying wallet method as a bare function:

value(args, originator)

That loses the wallet instance as this. For wallet implementations that use instance state internally, such as methods calling this.ensureCanCall(...) or this.underlying..., that turns ordinary non-privileged calls into runtime failures. Since getPublicKey is in the privileged-capable set but is also used for ordinary BRC-100 connect flows, the impact described in the PR makes sense: the guard intended to block only unauthorized privileged: true calls, but instead broke normal wallet connection paths too.

The fix:

value.call(target, args, originator)

is the right shape. It preserves the existing privilege gate, still rejects non-admin privileged: true requests, and restores the original receiver for allowed calls.

The added regression test is also directionally right. The earlier fake wallet methods were closures and could not detect lost this; the new class-style test does catch the actual production failure mode. That is the right kind of test for this bug.

I would approve this. The only minor follow-up I would consider, not a merge blocker, is adding one more regression case showing that an admin-originated privileged-capable method also preserves this, not just an ordinary non-privileged getPublicKey call. The same .call(target, ...) path covers both, so this is not a correctness concern with the current patch, just a slightly stronger test around the security-sensitive branch.

A second minor point: the explanatory comment in guard.ts is accurate, but quite long for a one-line receiver-binding fix. I would not block on it, because the previous failure was subtle and security-adjacent, but it could be shortened later if maintainers prefer less commentary in the hot path.

Overall: this is a small, well-scoped fix for a real wallet-connect regression, with a load-bearing regression test. I would merge it.