Skip to content

feat: add AllowedRecipients and MaxTransactionValue policy rules#169

Open
Sertug17 wants to merge 7 commits intoopen-wallet-standard:mainfrom
Sertug17:feat/allowed-recipients-policy
Open

feat: add AllowedRecipients and MaxTransactionValue policy rules#169
Sertug17 wants to merge 7 commits intoopen-wallet-standard:mainfrom
Sertug17:feat/allowed-recipients-policy

Conversation

@Sertug17
Copy link
Copy Markdown
Contributor

@Sertug17 Sertug17 commented Apr 2, 2026

Closes #153

Summary

Adds two new declarative policy rules to the OWS policy engine:

AllowedRecipients

Deny transactions to addresses not in an explicit allowlist. Contract creation (to = None) is always denied when this rule is present — same strict stance as AllowedTypedDataContracts for missing verifyingContract.

{
  "type": "allowed_recipients",
  "addresses": ["0x742d35Cc6634C0532925a3b844Bc9e7595f2bD0C"]
}

MaxTransactionValue

Deny transactions whose value exceeds a per-tx wei cap. Message signing and non-EVM chains pass through when value = None.

{
  "type": "max_transaction_value",
  "max_wei": "100000000000000000"
}

Changes

  • ows-core/src/policy.rs — two new PolicyRule variants
  • ows-lib/src/policy_engine.rs — evaluation logic + 8 new unit tests
  • ows-cli/src/commands/policy.rs — CLI display for new rule types

Test plan

  • 8 new unit tests: address matching, case-insensitivity, contract creation denial, value under/at/over limit, None passthrough
  • All 228 existing tests pass
  • cargo fmt and cargo clippy -D warnings clean

Note

Medium Risk
Adds new declarative enforcement in the signing path (including EVM transaction field parsing), which can deny previously-allowed transactions if parsing or rule configuration is incorrect. Changes touch core policy types and agent signing context, so compatibility and edge cases around tx formats/values are the main risk.

Overview
Adds two new declarative policy rules: allowed_recipients (EVM sign_transaction recipient allowlist, denying contract creation) and max_transaction_value (per-tx wei cap).

Extends PolicyContext with SigningOperation and updates agent signing to populate operation plus parse EVM to/value from raw tx bytes for policy evaluation; non-EVM chains and message signing bypass these rules. Updates CLI policy show output and adds unit tests covering serde and rule behavior (case-insensitive matching, contract creation denial, value limits, and pass-through cases).

Reviewed by Cursor Bugbot for commit 0596edb. Bugbot is set up for automated code reviews on this repo. Configure here.

Closes open-wallet-standard#153

- AllowedRecipients: deny transactions to unlisted addresses; contract
  creation (to=None) is always denied when this rule is present
- MaxTransactionValue: deny transactions exceeding a per-tx wei cap;
  message signing and non-EVM chains pass through when value=None
- CLI policy display updated for both new rule types
- 8 new unit tests covering both rules, case-insensitivity, edge cases
@Sertug17 Sertug17 requested a review from njdawn as a code owner April 2, 2026 20:42
@vercel
Copy link
Copy Markdown

vercel bot commented Apr 2, 2026

@Sertug17 is attempting to deploy a commit to the MoonPay Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Copy Markdown
Contributor

@njdawn njdawn left a comment

Choose a reason for hiding this comment

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

PR #169 — feat: add AllowedRecipients and MaxTransactionValue policy rules

Status: correctness risk

Findings:

High — ows/crates/ows-lib/src/policy_engine.rs: AllowedRecipients denies when transaction.to is None, which is fine for contract creation. But ows/crates/ows-lib/src/key_ops.rs currently builds API-key policy contexts with transaction.to and transaction.value unset, so this rule appears to deny all API-key signing rather than only blocking contract creation or disallowed recipients.

Medium — ows/crates/ows-core/src/policy.rs: Docs say these rules are effectively EVM sign_transaction rules, but evaluation is not explicitly gated by chain or operation type. Current behavior depends on how PolicyContext happens to be populated.

Low — ows/crates/ows-core/src/policy.rs: New rule variants do not appear to have serde round-trip coverage comparable to the older policy rule variants.

Testing:

Ran cargo test -p ows-lib policy_engine.
New recipient/value tests passed
Two existing executable-policy tests timed out after 5 seconds; they look unrelated to this change but leave an integration gap.

- key_ops: parse to/value from EVM tx bytes before policy evaluation
  so AllowedRecipients and MaxTransactionValue rules work correctly
- policy_engine: gate AllowedRecipients and MaxTransactionValue to
  eip155 chains only — non-EVM chains pass through
- policy: add serde round-trip tests for both new rule variants
@Sertug17
Copy link
Copy Markdown
Contributor Author

Sertug17 commented Apr 6, 2026

Addressed all review findings:

Highkey_ops.rs now parses to and value from EVM transaction bytes before policy evaluation using a lightweight RLP decoder. Non-EVM chains return (None, None) and pass through.

MediumAllowedRecipients and MaxTransactionValue are now explicitly gated to eip155: chains. Non-EVM chains pass through both rules unconditionally.

Low — Added serde round-trip tests for both AllowedRecipients and MaxTransactionValue in policy.rs, matching the coverage of existing rule variants.

@Sertug17
Copy link
Copy Markdown
Contributor Author

Sertug17 commented Apr 6, 2026

Also addressed the Cursor Bugbot finding: moved eval_allowed_recipients
and eval_max_transaction_value before the test module, consistent with
the placement of eval_allowed_chains and eval_expires_at.

- key_ops: handle all EIP-2718 typed tx types (0x00-0x7f) in
  parse_evm_tx_fields, not just 0x01 and 0x02
- policy: add SigningOperation enum to PolicyContext so rules can
  distinguish sign_transaction from sign_message/sign_hash
- policy_engine: gate AllowedRecipients and MaxTransactionValue to
  SignTransaction only — message signing always passes through
- tests: add message_signing pass-through tests for both rules
@Sertug17
Copy link
Copy Markdown
Contributor Author

Sertug17 commented Apr 6, 2026

Addressed both Cursor Bugbot findings:

EIP-2718 typed tx bypass — parse_evm_tx_fields now strips the type
byte for any first byte in [0x00, 0x7f] per EIP-2718, covering 0x03
(EIP-4844 blob txs) and future types like 0x04 (EIP-7702). Previously
only 0x01 and 0x02 were handled.

Message signing blocked by AllowedRecipients — Added a
SigningOperation enum to PolicyContext (SignTransaction, SignMessage,
SignHash, SignTypedData). AllowedRecipients and MaxTransactionValue now
gate on operation == SignTransaction, so message signing always passes
through unconditionally. Added 2 new tests covering this behavior.

…ard#169

- enforce_policy_and_decrypt_key: parse EVM to/value from tx bytes
  and set operation=SignTransaction (was SignMessage + None fields)
- parse_evm_tx_fields: handle value fields > 16 bytes safely:
  values > u128::MAX treated as infinite, values > 32 bytes (invalid
  uint256) return sentinel string to trigger MaxTransactionValue denial
@Sertug17
Copy link
Copy Markdown
Contributor Author

Sertug17 commented Apr 6, 2026

Addressed the two remaining Cursor Bugbot findings:

enforce_policy_and_decrypt_key bypass — This function (used by
sign_and_send) now also calls parse_evm_tx_fields and sets
operation=SignTransaction, matching sign_with_api_key. Previously it
hardcoded to=None/value=None and SignMessage, bypassing both rules.

Value truncation — parse_evm_tx_fields now handles large value
fields safely: values > 16 bytes (> u128::MAX) are treated as
effectively infinite so MaxTransactionValue always denies them. Values

32 bytes (invalid uint256) return a sentinel string that also
triggers denial.

…ions

EIP-4844 (0x03) and EIP-7702 (0x04) use EIP-1559 field layout
where to=index 5 and value=index 6. Previously they fell through
to legacy indices (3,4) which reads max_fee_per_gas as to and
gas_limit as value, enabling a MaxTransactionValue policy bypass.
@Sertug17
Copy link
Copy Markdown
Contributor Author

Sertug17 commented Apr 6, 2026

Fixed the remaining medium finding:

EIP-4844/7702 field indices — parse_evm_tx_fields now correctly
uses EIP-1559 layout (to=5, value=6) for all typed transactions except
EIP-2930 (0x01). EIP-4844 (0x03) and EIP-7702 (0x04) share the
EIP-1559 field layout, so the previous fallthrough to legacy indices
(3,4) was reading max_fee_per_gas as to and gas_limit as value,
enabling a MaxTransactionValue bypass for those tx types.

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit cdf3470. Configure here.

u128::MAX.to_string() is a valid parseable u128, so a max_wei set
to u128::MAX would allow oversized values through. Replace with
'U128_OVERFLOW' sentinel which fails u128::parse() and triggers
denial in eval_max_transaction_value.
@Sertug17
Copy link
Copy Markdown
Contributor Author

Sertug17 commented Apr 6, 2026

Fixed the final Cursor Bugbot finding:

u128::MAX.to_string() was replaced with "U128_OVERFLOW" sentinel for
values with 17-32 bytes. Since "U128_OVERFLOW" fails u128::parse(),
eval_max_transaction_value now correctly denies transactions with
values > u128::MAX rather than silently allowing them through when
max_wei happens to equal u128::MAX.

@Sertug17
Copy link
Copy Markdown
Contributor Author

Sertug17 commented Apr 6, 2026

@njdawn All review findings and Cursor Bugbot issues have been addressed
across the follow-up commits. Ready for re-review when you get a chance.

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.

Policy engine lacks content-level gating for transaction and message signing

2 participants