Skip to content

fix(policy): don't expose signer or key material through enforced handles#75

Open
AlonzoRicardo wants to merge 1 commit into
mainfrom
fix/policy-proxy-internal-reference-exposure
Open

fix(policy): don't expose signer or key material through enforced handles#75
AlonzoRicardo wants to merge 1 commit into
mainfrom
fix/policy-proxy-internal-reference-exposure

Conversation

@AlonzoRicardo

Copy link
Copy Markdown
Contributor

Summary

The policy engine's enforced account (wdk.getAccount()) and its protocol proxies only substituted the named members (the OPERATIONS write methods, protocol verbs, simulate, registerProtocol). Every other property read fell through to the raw account via Reflect.get, so the guarded handle still surfaced:

  • keyPair → the private key
  • _signer → the raw signer (account._signer.signTransaction(...))
  • _provider / _config
  • a protocol's _account back-reference (account.getSwapProtocol(x)._account.sendTransaction(...))

Any of these lets a caller holding an "enforced" account sign or move funds without policy evaluation, defeating the guardrail the proxy exists to provide.

Fix

Both get traps now hide key material and internal references through one shared rule:

function isProtectedMember (prop) {
  return prop === 'keyPair' || (typeof prop === 'string' && prop.startsWith('_'))
}
  • keyPair is blocked by name — it's the one public getter that returns key material.
  • The underscore rule structurally covers _signer, _provider, _config, the ERC-4337 _ownerAccount, and a protocol's _account across every chain, rather than a per-name denylist that would miss chain-specific fields.
  • The prototype-climb variant (Object.getPrototypeOf(account).sendTransaction.call(account, …)) is neutralized by the same rule: every signing path funnels through this._signer, now undefined.

Unchanged: enforced write methods, protocol getters, simulate, registerProtocol, all reads (getBalance, quotes, verify, …), and identity accessors (path, index). instanceof still holds (no getPrototypeOf trap). Ungoverned accounts (no policy registered) are untouched.

The wallet classes' public API is deliberately not changed — the boundary is enforced only at the proxy, so direct (non-governed) users keep keyPair/_signer (no # private fields introduced).

Scope / trust boundary

This contains a caller holding an enforced account. It is not a defense against code that also holds the WalletManager/WDK instance or constructs an account directly — that remains the existing, documented trust boundary.

Tests

New internal-reference containment block covering: keyPair hidden, _signer/_provider/_config hidden, reads + enforced ops still work, and the protocol _account reach-through blocked while protocol enforcement still holds. Full suite 135 passing, standard clean.

…dles

The policy-enforced account and protocol proxies substituted only the named
write methods; every other property access fell through to the raw account.
That left `keyPair` (the private key), `_signer`, `_provider`, and a
protocol's `_account` back-reference reachable through the guarded handle, so
a caller holding an enforced account could sign outside policy evaluation
(e.g. `account.keyPair.privateKey`, `account._signer.signTransaction(...)`,
`account.getSwapProtocol(x)._account.sendTransaction(...)`).

Both get-traps now hide key material (`keyPair`) and internal references
(underscore-prefixed by WDK convention) via a shared `isProtectedMember`
check. Enforced methods, protocol getters, `simulate`, `registerProtocol`,
reads, and identity accessors (`path`/`index`) are unchanged; the
prototype-climb variant is neutralized because every signing path funnels
through the now-hidden `_signer`.

Adds regression coverage for the account and protocol proxies.
@AlonzoRicardo
AlonzoRicardo force-pushed the fix/policy-proxy-internal-reference-exposure branch from e11471a to 0329f6e Compare July 8, 2026 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant