Skip to content

feat: add Phase 1 local transaction policy engine#55

Open
AlonzoRicardo wants to merge 14 commits into
mainfrom
feat/local-transaction-policies-phase-1
Open

feat: add Phase 1 local transaction policy engine#55
AlonzoRicardo wants to merge 14 commits into
mainfrom
feat/local-transaction-policies-phase-1

Conversation

@AlonzoRicardo
Copy link
Copy Markdown
Contributor

@AlonzoRicardo AlonzoRicardo commented May 4, 2026

Summary

Phase 1 of the Local Transaction Policies engine, per the PRD. Policies are plain JS objects registered against the WDK instance; the engine wraps every write-facing operation on a wallet account (and on protocol getters) so DENY rules throw PolicyViolationError before the underlying method runs and ALLOW rules pass through untouched.

What's in the engine

  • Three-group evaluation (account → wallet → project) with DENY-wins and account-level override_broader_scope to grant explicit exceptions for treasury wallets, etc.
  • Conditions are user-supplied functions, sync or async, with full access to a frozen context { operation, chain, account: <readOnly>, params, args }. Stateful policies are supported in Phase 1 via closures over user-owned state.
  • Per-condition timeout (default 30s, configurable via RegisterPolicyOptions.conditionTimeoutMs) so a stuck async condition cannot hang the wallet pipeline. On timeout the condition is treated the same as a throw.
  • Fail-closed DENY: a throwing or timing-out DENY condition is treated as matched, so an attacker cannot bypass a DENY by causing its backing service to fail. ALLOW rules retain fail-open-as-no-match.
  • account.simulate.<method>(...) mirror runs evaluation without execution and returns { decision, policy_id, matched_rule, reason, trace }.
  • Nested-call escape via AsyncLocalStorage (per-async-chain), so concurrent calls evaluate independently while nested calls within one chain skip re-evaluation. node:async_hooks is loaded lazily so importing @tetherto/wdk/bare does not fail at link time on runtimes that don't expose AsyncLocalStorage; the error only surfaces if a policy is actually registered.
  • Registration validation throws PolicyConfigurationError synchronously on unknown ops, missing fields, contradictory configuration, and unknown chain bindings — no soft-mode.
  • 17 supported operations + wildcard: sendTransaction, transfer, approve, signMessage, signHash, signTypedData, signAuthorization, delegate, revokeDelegation, swap, bridge, supply, withdraw, borrow, repay, buy, sell, *.

Public API additions

In index.js:

  • PolicyViolationError, PolicyConfigurationError (classes).
  • Typedefs: Policy, PolicyRule, PolicyCondition, PolicyContext, PolicyAction, PolicyScope, PolicyOperation, SimulationResult, SimulationTraceEntry, RegisterPolicyOptions.
  • Re-export of IWalletAccountReadOnly from @tetherto/wdk-wallet so condition functions can be typed without a direct parent dep.

In WDK:

  • New chainable method registerPolicy(chain?, policies, options?) (overloaded — chain string/array first, or policies first).
  • New private hook _applyPolicies slotted into getAccount and getAccountByPath between protocol registration and the return.
  • dispose([chains]) and dispose() clear policy bindings per-chain or globally.

Phase 2 / 3 forward-compat

  • state and onSuccess fields on PolicyRule and RegisterPolicyOptions are accepted at registration and ignored at runtime — reserved for Phase 2 (engine-managed state + post-execution hooks). state is deep-cloned at registration so callers cannot mutate engine state post-registration.
  • Conditions are functions in Phase 1; widening the validator + evaluator to also accept Phase 3 DSL objects is purely additive (no breaking change for existing function-form policies).
  • Phase 4 portal is external tooling that produces/consumes the same Policy shape — no SDK changes needed there.

A dedicated forward-compat audit confirmed no Phase 1 design choice forces a breaking change in any later phase.

Test plan

  • npm test — 95/95 passing (38 existing + 57 new in tests/wdk-manager-policy.test.js)
  • npm run lint — clean (Standard style)
  • npm run build:types — clean type generation
  • All new tests go through the public WDK API only (no internal-module imports per WDK testing standards)
  • End-to-end smoke pass through stub wallets: project DENY blocks; account override beats project DENY; simulate doesn't execute; concurrent calls evaluate independently; nested calls escape correctly; protocol getters wrap write methods only; multi-chain registration; dispose semantics; condition timeout fires; throwing DENY fails closed
  • All assertions use concrete values (no toBeInstanceOf/type checks); error catches unpack name, policyId, ruleName, reason, message; simulate results assert decision, policy_id, matched_rule, reason, trace

Implements the foundational engine for local transaction policies per the
PRD. Policies are plain JS objects registered against the WDK instance;
the engine wraps every write-facing operation on a wallet account (and on
protocol getters) so DENY rules throw PolicyViolationError before the
underlying method runs and ALLOW rules pass through untouched.

- Three-group evaluation (account → wallet → project) with DENY-wins and
  account-level override_broader_scope to grant explicit exceptions.
- Conditions are user-supplied functions, sync or async, with full access
  to a frozen context { operation, chain, account: <readOnly>, params, args }.
- account.simulate.<method>(...) mirror runs evaluation without execution
  and returns { decision, policy_id, matched_rule, reason, trace }.
- Nested-call escape via Symbol-keyed flag so approve()/bridge() that
  internally call sendTransaction() are not double-evaluated.
- Registration validation throws PolicyConfigurationError synchronously on
  unknown ops, missing fields, and contradictory configuration; no
  soft-mode.
- 17 supported operations + wildcard (sendTransaction, transfer, approve,
  signMessage, signHash, signTypedData, signAuthorization, delegate,
  revokeDelegation, swap, bridge, supply, withdraw, borrow, repay, buy,
  sell).
- Phase 2/3 hooks reserved: state and onSuccess fields accepted on rules
  and registration options, ignored at runtime.

Public exports added to index.js: PolicyViolationError,
PolicyConfigurationError, plus the Policy / PolicyRule / PolicyCondition /
PolicyContext / PolicyAction / PolicyScope / PolicyOperation /
SimulationResult / SimulationTraceEntry / RegisterPolicyOptions typedefs,
and a re-export of IWalletAccountReadOnly so consumers can type their
condition functions without depending on the parent SDK directly.

Test coverage: 49 new tests in tests/wdk-manager-policy.test.js, all
through the public WDK API; 80 total passing across the suite.
Concurrency (blocker)
- Replace per-account POLICY_CTX flag with AsyncLocalStorage. The flag
  approach silently bypassed evaluation for the second of any two
  concurrent calls on the same account: call A's in-flight marker was
  visible to call B during A's await, and B took the nested-escape
  branch. AsyncLocalStorage scopes the marker to the async chain that
  set it, so concurrent calls evaluate independently while nested calls
  within one chain still skip re-evaluation.
- Add regression tests: concurrent ALLOW (both evaluate, both run) and
  concurrent DENY (both throw, neither executes).

TOCTOU on params
- buildContext now structuredClones each cloneable arg. Conditions see
  a snapshot taken at evaluation time; mutating the original tx object
  during an async condition's await no longer changes what conditions
  evaluated. The original args still flow through to original(...args)
  untouched.
- Tests for both directions: caller-side mutation after the call starts
  doesn't affect what conditions saw, and condition-side mutation
  doesn't propagate to the underlying call.

PolicyViolationError ergonomics
- Add optional rule.reason field. When set on a DENY rule, propagates
  to err.reason, simulate-result.reason, and the error message.
- When err.reason equals err.ruleName (the default fallback), drop the
  redundant trailing ": <reason>" from the message.

Trace shape consistency
- SimulationTraceEntry now uses snake_case (policy_id, rule_name) to
  match the snake_case top-level SimulationResult. Previously
  inconsistent (top-level snake_case, nested camelCase).

Integration diagnostic
- The wrapper checks for IWalletAccount.toReadOnlyAccount() and throws
  PolicyConfigurationError with a clear message if missing, instead of
  surfacing a TypeError from inside engine internals.

Defensive policy clone
- PolicyRegistry.add now stores a shallow clone of the policy and its
  rules so callers cannot mutate engine state by editing the original
  object after registration.

Typedef docs
- PolicyRule.override_broader_scope: documented as account-scope
  ALLOW only, registration-order matching, short-circuits both wallet
  and project evaluation.
- PolicyRule.reason: documented.
- Policy.accounts: documented as exact-string match only in Phase 1.
- PolicyContext.params and .args: documented as structured clones.

Test count: 87 passing (was 80), all through the public WDK API.
- README: add 'Transaction Policies' to Key Capabilities and a focused
  usage section showing registerPolicy(), DENY-with-PolicyViolationError,
  and account.simulate.<method>(). Mentions the three scopes,
  override_broader_scope, and the upcoming templates package.
- AGENTS.md: extend Repository Specifics to include policies as a
  third orchestrated lifecycle alongside wallets and protocols, and
  add a Policy Engine section covering source layout, public surface,
  AsyncLocalStorage marker, Phase 2 reservations, and the public-API
  test discipline.
Copy link
Copy Markdown
Contributor

@claudiovb claudiovb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Policy Engine Review — Phase 1

Solid architecture. Three-group evaluation semantics, DENY-wins, AsyncLocalStorage concurrency isolation, TOCTOU protection, and simulate mirror are all correct. 87/87 tests pass, lint clean, types clean. A 20-scenario E2E dogfood suite against real WalletManager/WalletAccount/SwapProtocol/BridgeProtocol subclasses passes end-to-end.

Findings

ID Severity Title
C-1 Critical bare-async-hooks missing AsyncLocalStorage it breaks WDK import on Bare
H-1 High No timeout on async conditions a never-resolving Promise hangs forever
H-2 High Shallow policy clone the rule.state shared by reference after registration
H-3 High Throwing DENY condition bypassed when a matching ALLOW exists

All four are tested with reproduction gists in the inline comments.

Informational

  • I-1: structuredClone fallback to raw value for non-cloneable args (policy-context.js:52) which is the only way to go I think, worth documenting though.
  • I-2: No policy introspection API for listing registered policies or inspecting governed operations at runtime this would be really valuable for agentic tooling, worth considering for Phase 2.
  • I-3: PR description says "80/80 passing (31 existing + 49 new)" the actual count is 87 (38 + 49).
  • I-4: PR description says "Symbol-keyed flag" for nested-call escape but code uses AsyncLocalStorage since commit 7567876.

JSDoc, style, and types review to follow.

Comment thread src/policy/policy-account-wrapper.js Outdated
Comment thread src/policy/policy-evaluator.js Outdated
Comment thread src/policy/policy-registry.js Outdated
Comment thread src/policy/policy-evaluator.js
- C-1: Lazy-load node:async_hooks in policy-account-wrapper so importing
  WDK on Bare doesn't fail at link time. AsyncLocalStorage is only
  resolved when an account is actually wrapped; if the runtime can't
  provide it, applyPoliciesToAccount throws a clear PolicyConfigurationError.

- H-1: Race each condition against a configurable conditionTimeoutMs
  (default 30s). On timeout, the condition is surfaced as a throw and
  follows the same fail-mode as any other condition error. Configurable
  via RegisterPolicyOptions, validated at registration.

- H-2: Deep-clone rule.state with structuredClone in the registry so
  Phase 2's engine-managed state can't be mutated by callers post-
  registration.

- H-3: A throwing or timing-out DENY condition is now treated as
  matched (fail-closed) rather than skipped. An attacker can no longer
  bypass a DENY by causing its backing service to throw while a sibling
  ALLOW would have matched. ALLOW rules retain fail-open-as-no-match.

Tests: +8 covering timeouts (allow/deny paths, validation, no-op when
condition resolves quickly), throwing DENY fail-closed, throwing ALLOW
falls through, rule.reason precedence, and defensive deep-clone.
…cies

Both changes per PM feedback (Lokesh).

Wallet scope removed:
- The 'wallet' scope was mechanically redundant once project-scope policies
  respect their chain binding. With DENY-wins evaluation, the precedence
  layer (account → wallet → project) collapsed to (account → project)
  because the project layer never won over wallet anyway.
- Project-scope policies now narrow by chain when registered with a chain
  argument. registerPolicy('ethereum', policy) applies to ethereum only;
  registerPolicy(['ethereum','ton'], policy) applies to both;
  registerPolicy(policy) applies globally.
- disposeChain now narrows project policies bound to the disposed chain
  (and removes them entirely if the chain list becomes empty), so the
  wallet-scope dispose semantics are preserved.

Account-scope policies accept integer indexes:
- The accounts field accepts a mixed array of derivation paths and
  non-negative integer indexes — accounts: [0, "44'/60'/0'/0/0"].
- Index entries match the index passed to wdk.getAccount(chain, index).
- Index entries do not match accounts retrieved via getAccountByPath
  (no synchronous path → index resolver on the wallet manager).
  Path entries match either retrieval style. Documented.

Tests: 7 new (chain-bound project narrowing, mixed accounts array,
index-vs-path retrieval semantics, validator rejects non-integer /
negative entries). 101/101 pass. Smoke 51/51. Lint + types clean.

Docs (README, AGENTS.md) updated to reflect the two-scope model.
Adds src/policy/policy-context-store.js. Picks the implementation at
first use:

- On Node (and any runtime exposing node:async_hooks.AsyncLocalStorage):
  wraps the native primitive directly. Zero overhead, full nested-call
  escape, concurrent-call isolation as before.

- On Bare (and any runtime missing AsyncLocalStorage): falls back to a
  no-op store. getStore() always returns undefined, run() just calls fn().
  Effect: every wrapped method call evaluates policies independently,
  including nested calls.

Why a no-op rather than a Promise-prototype-patch shim: modern V8 (since
2018) inlines `await` to skip user-visible Promise.prototype.then, so
the Zone.js technique no longer captures context across awaits in pure
user-space. The only correct propagation paths are AsyncLocalStorage or
the upcoming TC39 AsyncContext, both of which require runtime support.
Bare is waiting on AsyncContext to reach Stage 3 + V8 implementation
before adding AsyncLocalStorage upstream (per Holepunch); when that
lands, this fallback is automatically replaced by the native path with
no code changes needed here.

Behavioral consequence on Bare: nested wrapped-method calls re-evaluate
(e.g. a project DENY on sendTransaction will block a bridge() that
internally sends a transaction). Documented in README and AGENTS.md
under "Runtime support". Every other engine behavior — basic ALLOW/DENY,
async conditions, concurrent-call isolation, account.simulate, fail-
closed DENY — works identically on both runtimes.

Tests: 7 new in tests/wdk-manager-policy-bare.test.js, mocking
node:async_hooks via jest.unstable_mockModule to force the fallback
path through the public WDK API. 108/108 pass total. Lint + types
clean. Smoke 51/51.
…es on

Per PR feedback: the policy engine doesn't actually know about
blockchains. It routes policies based on whatever string was passed to
`registerWallet` — that string can be a chain name, but it could equally
be `"treasury-cold"`, `"my-hot-wallet"`, or any consumer-chosen label.
Calling the parameter `chain` was misleading.

API rename:
- `wdk.registerPolicy(chain, policy)` → `wdk.registerPolicy(wallet, policy)`
- `PolicyContext.chain` → `PolicyContext.wallet` (passed to conditions)
- All error messages mentioning "chain" → "wallet"

Internal rename:
- `PolicyRegistry._accountByChain` → `_accountByWallet`
- `policy._chains` → `policy._wallets`
- `disposeChain` → `disposeWallet`
- `normaliseChainArg` → `normaliseWalletArg`
- All param names follow

Out of scope: WDK manager keeps `blockchain` for `registerWallet` /
`getAccount` / `getAccountByPath` since that's existing API; the rename
is local to the policy engine surface.

Future-proof: if real chain-level awareness is ever needed (e.g. "deny
on Polygon regardless of which manager"), it would be additive — a
`getChainId()` getter on wallet packages so the engine can ask the
provider what chain it's connected to. Not required for Phase 1.

Tests + smoke pass: 108/108, 51/51. Lint + types clean.
…e and Bare

Replaces the AsyncLocalStorage-based wrapper (and its no-op Bare fallback)
with a single Proxy-based mechanism that works identically on every
runtime that supports ES Proxy.

How it works
- `_applyPolicies` returns a Proxy wrapping the underlying account
  instead of mutating it. WDK manager returns this proxy from
  `getAccount` / `getAccountByPath`.
- The proxy's `get` trap exposes enforced versions of write methods.
- For every non-enforced property, the trap returns the underlying value;
  if it's a function, it's bound to the underlying account.
- Internal SDK code that calls `this.someMethod()` resolves on the
  underlying account, not the proxy — bypassing enforcement naturally.
- Protocol packages that hold a direct reference to the account
  (constructed before the proxy exists) likewise bypass enforcement.
- Only access through the proxy goes through policy evaluation.

Why this is better than what we had
- Identical code path on Node and Bare. No `AsyncLocalStorage`, no
  `node:async_hooks` dependency, no runtime detection, no
  Promise-prototype patching, no fallback.
- Eliminates the divergence we shipped previously (Node had
  nested-call escape, Bare didn't). Both now work the same way.
- Doesn't depend on V8 internals or TC39 AsyncContext landing.
- The original account stays clean — easier to reason about and
  matches the "policy proxy is a view, not a mutation" mental model
  the dev portal will need in later phases.

Files
- Added: `src/policy/policy-account-proxy.js`
- Removed: `src/policy/policy-account-wrapper.js`,
  `src/policy/policy-context-store.js`,
  `tests/wdk-manager-policy-bare.test.js` (the Bare-divergence tests
  no longer apply — coverage for nested escape is in the main test
  file and works the same on every runtime).
- Updated: `src/policy/policy-engine.js` (calls the new factory),
  `src/wdk-manager.js` (returns the proxy), README + AGENTS.md
  (drop the "Bare-specific limitation" section).

Tests + types + lint + smoke all clean: 101/101 tests, 51/51 smoke.
Per Lokesh + Jonathan: a policy should be self-describing, so the wallet
binding lives inside the policy object instead of as a positional arg to
registerPolicy. `wallet` accepts `string | string[]`, optional for project
scope (omitting = all wallets), required for account scope. The dual
overload of registerPolicy is gone.

Also gitignore .scripts/.agents (per wdk-local-files convention).
Comment thread src/policy/policy-account-proxy.js Outdated
Comment on lines +107 to +122
return new Proxy(account, {
get (target, prop) {
if (enforcedMethods.has(prop)) return enforcedMethods.get(prop)
if (enforcedGetters.has(prop)) return enforcedGetters.get(prop)
if (prop === 'simulate') return simulate

const value = Reflect.get(target, prop, target)

// Bind functions to the underlying target so internal `this.method()`
// calls resolve on the original account, bypassing the proxy. This is
// how nested-call escape works without any async-context tracking.
if (typeof value === 'function') return value.bind(target)

return value
}
})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Policy bypass in the proxy's get trap leaks the raw account via registerProtocol

The account-level get trap intercepts enforcedMethods, enforcedGetters, and simulate, then falls through to Reflect.get + .bind(target) for everything else. That's correct for ordinary reads, but account.registerProtocol is special: pre-existing code in wdk-manager.js (_registerProtocols) installs it directly on the raw account and the closure returns that raw account reference. So proxy.registerProtocol(...) hands the caller back the raw account, and every subsequent write on it skips policy evaluation.

The chained form is the dangerous one: account.registerProtocol(...).getXProtocol(...).x(...) is the protocol-attachment pattern this PR's own AGENTS.md and README suggest.

Reproduction

Save as repro.mjs at repo root, run with node repro.mjs:

import WDK from './index.js'
import WalletManager from '@tetherto/wdk-wallet'
import { BridgeProtocol } from '@tetherto/wdk-wallet/protocols'

class M extends WalletManager {
  async getAccount () {
    return {
      path: "0'/0/0",
      keyPair: { publicKey: new Uint8Array(32), privateKey: new Uint8Array(32) },
      sendTransaction: async () => ({ hash: '0xH' }),
      toReadOnlyAccount: async () => ({ getAddress: async () => '0x', getBalance: async () => 0n }),
      dispose () {}
    }
  }
}

const wdk = new WDK('abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about')
wdk.registerWallet('eth', M)
wdk.registerPolicy({
  id: 'deny', name: 'deny', scope: 'project',
  rules: [{ name: 'r', operation: 'sendTransaction', action: 'DENY', conditions: [] }]
})

const proxy = await wdk.getAccount('eth', 0)

class B extends BridgeProtocol {}
const returned = proxy.registerProtocol('br', B, {})

console.log('returned === proxy?', returned === proxy)
await proxy.sendTransaction({}).then(() => console.log('via proxy:    NO THROW'))
  .catch(e => console.log('via proxy:   ', e.constructor.name))
await returned.sendTransaction({}).then(() => console.log('via returned: NO THROW the policy is bypassed'))
  .catch(e => console.log('via returned:', e.constructor.name))

Output:

returned === proxy? false
via proxy:    PolicyViolationError
via returned: NO THROW the policy is bypassed

returned === proxy -> false is the structural bypass, the caller is no longer holding the policy-enforced proxy. The NO THROW line is the consequence: the rule blocks sendTransaction through the proxy but not through the leaked reference.

Suggested fix (lands in this get trap)

let proxyHandle
const handler = {
  get (target, prop) {
    if (enforcedMethods.has(prop)) return enforcedMethods.get(prop)
    if (enforcedGetters.has(prop)) return enforcedGetters.get(prop)
    if (prop === 'simulate') return simulate

    if (prop === 'registerProtocol' && typeof target.registerProtocol === 'function') {
      const fn = target.registerProtocol.bind(target)
      return (...args) => { fn(...args); return proxyHandle }
    }

    const value = Reflect.get(target, prop, target)
    if (typeof value === 'function') return value.bind(target)
    return value
  }
}
proxyHandle = new Proxy(account, handler)
return proxyHandle

I didn't test extensively the fix but in basic cases seems like a functional starting point.

Optional doc nit: the JSDoc above this trap (and AGENTS.md "Policy Engine") says "The original account is never mutated." That's true of the policy engine itself, but _registerProtocols does install registerProtocol / getXProtocol on the raw account before the proxy is built. Worth softening to "The policy engine itself does not mutate the account" once the bypass above is closed.

Comment on lines +157 to +167
return new Proxy(protocol, {
get (target, prop) {
if (enforcedMethods.has(prop)) return enforcedMethods.get(prop)

const value = Reflect.get(target, prop, target)

if (typeof value === 'function') return value.bind(target)

return value
}
})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Membrane gap: the protocol._account leaks the raw account ( up for discussion)

wrapProtocolInProxy's get trap returns whatever Reflect.get produces for non-method properties. Every upstream protocol in @tetherto/wdk-wallet stores the raw account on this._account:

node_modules/@tetherto/wdk-wallet/src/protocols/swap-protocol.js:108:    this._account = account
node_modules/@tetherto/wdk-wallet/src/protocols/bridge-protocol.js:92:    this._account = account
node_modules/@tetherto/wdk-wallet/src/protocols/fiat-protocol.js:230:    this._account = account
node_modules/@tetherto/wdk-wallet/src/protocols/lending-protocol.js:182:    this._account = account

So proxy.getBridgeProtocol(label)._account reaches the underlying account through the policy-enforced proxy, and the engine never sees subsequent calls on it. The _ prefix says "private," but JavaScript doesn't enforce it.

Reproduction

Save as repro.mjs at repo root, run with node repro.mjs:

import WDK from './index.js'
import WalletManager from '@tetherto/wdk-wallet'
import { BridgeProtocol } from '@tetherto/wdk-wallet/protocols'

class M extends WalletManager {
  async getAccount () {
    return {
      path: "0'/0/0",
      keyPair: { publicKey: new Uint8Array(32), privateKey: new Uint8Array(32) },
      sendTransaction: async () => ({ hash: '0xH' }),
      toReadOnlyAccount: async () => ({ getAddress: async () => '0x', getBalance: async () => 0n }),
      dispose () {}
    }
  }
}
class B extends BridgeProtocol {}

const wdk = new WDK('abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about')
wdk.registerWallet('eth', M)
wdk.registerProtocol('eth', 'br', B, {})
wdk.registerPolicy({
  id: 'deny', name: 'deny', scope: 'project',
  rules: [{ name: 'r', operation: 'sendTransaction', action: 'DENY', conditions: [] }]
})

const proxy = await wdk.getAccount('eth', 0)
const bridge = proxy.getBridgeProtocol('br')

console.log('bridge._account === proxy?', bridge._account === proxy)
await proxy.sendTransaction({}).then(() => console.log('via proxy:           NO THROW'))
  .catch(e => console.log('via proxy:          ', e.constructor.name))
await bridge._account.sendTransaction({}).then(() => console.log('via bridge._account: NO THROW the policy is bypassed'))
  .catch(e => console.log('via bridge._account:', e.constructor.name))

Output:

bridge._account === proxy? false
via proxy:           PolicyViolationError
via bridge._account: NO THROW the policy is bypassed

Two options

  1. Accept + document. Add a security note to AGENTS.md / README.md: "Policies enforce the surface of the proxy returned by getAccount / getAccountByPath. Reaching for protocol._account or other underscore-prefixed fields bypasses enforcement; treat them as private. The same applies to account-level operations invoked from inside a protocol's own methods (e.g. bridge.bridge(...) internally calling this._account.sendTransaction(...)), which is the documented nested-call escape."

  2. Implement a membrane. A WeakMap<rawAccount, proxyAccount> consulted inside this get trap before returning:

    const value = Reflect.get(target, prop, target)
    if (value === ctx.account) return ctx.accountProxy
    if (typeof value === 'function') return value.bind(target)
    return value

    For the swap to fire on every protocol getter we also need to drop the if (opsToWrap.length === 0) continue guard at lines 91-94 so protocols are always wrapped, otherwise proxy.getBridgeProtocol(label) falls through and returns the raw protocol when no protocol-method policy exists. With both changes in place the base reproduction above flips to PolicyViolationError (verified locally, full suite still 133/133).

    Two caveats came up while testing this combination:

    • Cost. Every proxy.getXProtocol(label) call now allocates a Proxy, even when no policy targets that protocol's methods. Cheap individually but it's a hot path we weren't paying for before.
    • Same class of gap remains via nested calls. If a user defines a B extends BridgeProtocol whose bridge(opts) does this._account.sendTransaction({ ... }), calling bridge.bridge(opts) still bypasses the sendTransaction policy even with option 2 applied. Reason: inside bridge.bridge, this is bound to the raw protocol, this._account is the raw account, no proxy is in the call chain. This is the same nested escape the JSDoc on createPolicyEnforcedAccount already documents as intentional. Closing it would mean replacing this._account at protocol construction time with the proxy account, which crosses the @tetherto/wdk-wallet boundary. But as I understand it this bridge call should not be affected by sendTransaction policies so behaviour is correct, just flagging it now because if that assumption is wrong it will be an issue in the future.

Recommendation

I'd lean toward option 1 for Phase 1. Closing the _account direct read alone gives a false sense of safety if sendTransaction protection is expected to apply to bridge.bridge() calls too. And even if it isn't, _account isn't supposed to be used by consumers anyway (hence the _); JS doesn't enforce that convention, but a # private field upstream would, which is a wdk-wallet change and a separate discussion.

Accept + document for Phase 1, track the WeakMap membrane plus the nested-call story as Phase 2 if it proves necessary. The current design's biggest win is Bare/JSC portability; WeakMap works on Bare's runtime (I verified locally), I haven't tested it on JSC-backed Bare specifically.

Comment thread src/policy/policy-engine.js Outdated
* @property {string} [reason] - Optional human-readable explanation. When set on a DENY rule that matches, propagates to PolicyViolationError.reason and to the matching simulate-result. Defaults to the rule's name.
* @property {PolicyOperation | PolicyOperation[]} operation
* @property {PolicyAction} action
* @property {boolean} [override_broader_scope] - When true on an account-scope ALLOW rule that matches, the rule's verdict short-circuits both wallet- and project-scope evaluation. Account-scope rules are evaluated in registration order; the first matching override-flag rule wins. Only valid on account-scope ALLOW rules.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale JSDoc reference to wallet scope

Wallet scope was removed in 7889bc3. This JSDoc still mentions it:

"…short-circuits both wallet- and project-scope evaluation."

Should read:

"…short-circuits project-scope evaluation."

Comment thread src/policy/policy-account-proxy.js Outdated

const writeMethods = PROTOCOL_METHODS[type]

simulate[getterName] = () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cosmetic: simulate[getterName] = () => { ... } takes zero args, but the real account.getXProtocol(label) takes one. JS silently swallows the label, so proxy.simulate.getBridgeProtocol('myLabel') and proxy.simulate.getBridgeProtocol() behave identically. Not a bug today since simulation is label-agnostic, but accepting the label arg for surface parity would avoid future confusion if Phase 2 ever makes simulation label-aware.

C-2 (critical): proxy.registerProtocol(...) was returning the raw account
(per _registerProtocols closure), letting callers escape enforcement via
the chained form `proxy.registerProtocol(...).sendTransaction(...)`. The
proxy's get trap now intercepts registerProtocol and rewrites the return
value to the proxy itself. Regression test added.

H-4 (doc): added security/portability note to README + AGENTS clarifying
that enforcement applies to the proxy surface only, and underscore-prefixed
fields like `protocol._account` bypass by design (private by convention,
matches the documented nested-call escape behavior).

Doc-1: JSDoc on `override_broader_scope` still mentioned wallet scope
which was removed in 7889bc3.

Cosmetic: simulate.getXProtocol(label) now accepts the label arg for
parity with the real account.getXProtocol(label) — silently swallowed
today, reserved for Phase 2.
Comment thread src/policy/constants.js Outdated
Comment on lines +18 to +19
* The complete set of write-facing operations the policy engine wraps in Phase 1.
* The wildcard `*` matches any of them.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not really necessary to document internal properties. Also, avoid terms that make sense only in the scope of our company and team e.g., phase 1.

Comment thread src/policy/constants.js Outdated
*
* @internal
*/
export const OPERATIONS = Object.freeze([
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no real reason to freeze these objects since they are not accessible by external code.

Comment thread src/policy/constants.js Outdated
Comment on lines +23 to +44
export const OPERATIONS = Object.freeze([
'sendTransaction',
'transfer',
'approve',
'signMessage',
'signHash',
'signTypedData',
'signAuthorization',
'delegate',
'revokeDelegation',
'swap',
'bridge',
'supply',
'withdraw',
'borrow',
'repay',
'buy',
'sell'
])

/** @internal */
export const OPERATIONS_SET = new Set(OPERATIONS)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can replace all references to OPERATIONS_SET with OPERATIONS and get rid of this constant. We never really need the operation list to be a set.

Comment thread src/policy/policy-validators.js Outdated
Comment on lines +45 to +54
/**
* Normalises the wallet field of a policy into an array of non-empty strings
* or `undefined` (meaning "apply to every registered wallet").
*
* @internal
* @param {string | string[] | undefined} wallet
* @param {string} policyId - The owning policy's id, used to build error messages.
* @returns {string[] | undefined}
*/
export function normalisePolicyWallet (wallet, policyId) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to document internal components, but if you choose to do it you should do it properly i.e., document all internal components and not only some of them, use proper types and provide meaningful descriptions for all functions, arguments and return values.

Comment thread src/policy/policy-validators.js Outdated
* Validates a registerPolicy options bag (currently only `state`, reserved for Phase 2).
*
* @internal
* @param {object | undefined} options
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never use object, implement a proper type definition instead. Use brackets to mark optional arguments instead of * | undefined, and provide a meaningful description of the argument.

Comment on lines +17 to +22
/**
* Thrown by a wrapped wallet account method when a registered policy blocks
* the attempted operation. Carries the policy id, rule name, and a
* human-readable reason so callers (developers, agent runtimes) can react.
*/
export default class PolicyViolationError extends Error {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error class description should not state when the error is thrown. Also, listing available fields in the description is quite useless since their documentation is already part of the specification of the class.

Comment thread src/policy/policy-error.js Outdated
Comment on lines +23 to +24
/**
* @param {string} policyId - The id of the policy that produced the verdict.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing description for the constructor.

Comment thread src/policy/policy-error.js Outdated
Comment on lines +23 to +28
/**
* @param {string} policyId - The id of the policy that produced the verdict.
* @param {string} ruleName - The name of the matching rule.
* @param {string} reason - A human-readable explanation.
*/
constructor (policyId, ruleName, reason) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using three positional arguments, use a single object:

Suggested change
/**
* @param {string} policyId - The id of the policy that produced the verdict.
* @param {string} ruleName - The name of the matching rule.
* @param {string} reason - A human-readable explanation.
*/
constructor (policyId, ruleName, reason) {
/**
* @param {Object} error - The error.
* @param {string} error.policyId - The id of the policy that produced the verdict.
* @param {string} error.ruleName - The name of the matching rule.
* @param {string} error.reason - A human-readable explanation.
*/
constructor ({ policyId, ruleName, reason }) {

Comment thread src/policy/policy-error.js Outdated
Comment on lines +35 to +42
/** @type {string} */
this.policyId = policyId

/** @type {string} */
this.ruleName = ruleName

/** @type {string} */
this.reason = reason
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These properties should have private visibility and be accessible through a public getter method. Make sure to include a meaningful description in their documentation.

Comment on lines +30 to +71
* @typedef {'ALLOW' | 'DENY'} PolicyAction
*/

/**
* @typedef {'project' | 'account'} PolicyScope
*/

/**
* @typedef {'sendTransaction' | 'transfer' | 'approve' | 'signMessage' | 'signHash'
* | 'signTypedData' | 'signAuthorization' | 'delegate' | 'revokeDelegation'
* | 'swap' | 'bridge' | 'supply' | 'withdraw' | 'borrow' | 'repay' | 'buy' | 'sell'
* | '*'} PolicyOperation
*/

/**
* @typedef {object} PolicyContext
* @property {PolicyOperation} operation - The intercepted operation name.
* @property {string} wallet - The wallet identifier (the same string passed to `wdk.registerWallet`). Despite the name, this is an opaque key chosen by the consumer — it might be a chain name like `"ethereum"`, but it could equally be `"treasury-cold"` or any other label.
* @property {IWalletAccountReadOnly} account - A read-only view of the wallet account.
* @property {unknown} params - The first argument to the wrapped method.
* @property {readonly unknown[]} args - The full argument array.
*/

/**
* @typedef {(context: PolicyContext) => boolean | Promise<boolean>} PolicyCondition
*/

/**
* @typedef {object} PolicyRule
* @property {string} name
* @property {string} [reason] - Optional human-readable explanation. When set on a DENY rule that matches, propagates to PolicyViolationError.reason and to the matching simulate-result. Defaults to the rule's name.
* @property {PolicyOperation | PolicyOperation[]} operation
* @property {PolicyAction} action
* @property {boolean} [override_broader_scope] - When true on an account-scope ALLOW rule that matches, the rule's verdict short-circuits project-scope evaluation. Account-scope rules are evaluated in registration order; the first matching override-flag rule wins. Only valid on account-scope ALLOW rules.
* @property {PolicyCondition[]} conditions
* @property {object} [state] Reserved for Phase 2; ignored at runtime.
* @property {(c: PolicyContext) => void | Promise<void>} [onSuccess] Reserved for Phase 2; ignored at runtime.
*/

/**
* @typedef {string | number} AccountIdentifier
*/
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By using zod, you could infer all these types from the JSON or zod schema. This is extremely useful since it centralizes typing, validation and normalization in a single source file.
https://zod.dev/basics?id=inferring-types#inferring-types

…ation)

D-1..D-6, D-8..D-12: style/JSDoc/structure cleanups across constants,
policy-engine, policy-error, policy-registry, policy-validators.

D-7 + D-13: zod adoption for policy + register-options validation.
Schemas live in src/policy/policy-schemas.js. policy-validators.js
becomes a thin wrapper that runs the schemas and maps ZodError →
PolicyConfigurationError via a formatter that preserves the existing
error message contract. Sets us up for one-liner JSON-schema export for
the dev portal (z.toJSONSchema(policySchema)).

Verified compatible with Bare runtime: zod imports cleanly, schemas
parse, refinements fire — full 11/11 Bare end-to-end suite passes.

Other notable changes:
 - PolicyViolationError uses single-object constructor arg with private
   fields and documented getters (D-11, D-12).
 - PolicyRegistry uses Map instead of Object.create(null) (D-8).
 - collectReferencedOperations early-returns on wildcard hit (D-6).
 - Cross-field policy rules (account-scope requirements,
   override_broader_scope constraint) consolidated in one superRefine.

Verification: lint clean, types clean, 105/105 Jest, 51/51 smoke,
11/11 Bare, 8/8 EVM integration, 6/6 ERC-4337 integration.
Drops the bespoke message formatter (~80 lines) in favor of a 20-line
helper that just prefixes zod's default issue message with the policy
(and rule, where applicable) context. superRefine messages are now
short and field-scoped; the formatter unifies the prefix.

Test assertions on validator error messages switched from exact
toBe(...) to toMatch(/regex/) capturing the policy id and field — same
intent, less brittle to message wording.

policy-validators.js + policy-schemas.js: 326 → 252 lines, now 15 lines
smaller than the pre-zod imperative version.

All checks still green: 105/105 jest, 51/51 smoke, 11/11 Bare,
8/8 EVM integration, 6/6 ERC-4337 integration, lint+types clean.
@socket-security
Copy link
Copy Markdown

socket-security Bot commented May 21, 2026

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedzod@​4.4.310010010095100

View full report

Sweeps across the policy engine source + wdk-manager.registerPolicy:

R1  — class JSDoc on PolicyEngine + PolicyRegistry now starts with the
      description, @internal at the end.
R2  — @typedef declarations use {Object} (capitalized) — 6 sites in
      policy-engine.js.
R3  — every @param/@Property tag has a dash before the description.
R11 — replaced inline `import('zod').ZodError` with a top-of-file typedef
      alias in policy-schemas.js.
R13 — bare `object` / `object[]` / `Promise<object>` types replaced with
      named typedefs (Policy, PolicyRule, PolicyContext, IWalletAccount,
      PolicyGroups, PolicyVerdict, Verdict, ZodError, PolicyEngine).
R28 — every documented field now carries a meaningful description; the
      public typedefs (Policy, PolicyRule, SimulationResult, etc.) gained
      one-line descriptions per property that consumers see in IDE hover.

Public-API surface (Policy + PolicyRule + SimulationResult typedefs,
PolicyViolationError, PolicyConfigurationError, registerPolicy) gets
the strictest treatment since these ship via the generated .d.ts.
Internal helpers updated to the same standard for consistency.

PolicyGroups typedef moved from policy-evaluator.js to policy-registry.js
(the module that produces it); evaluator imports it.

Verified: lint clean, types regenerated cleanly, 105/105 jest,
51/51 smoke, 11/11 Bare e2e, 8/8 EVM integration, 6/6 ERC-4337 integration.
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.

3 participants