Skip to content

Fail closed when the wallet screen cannot confirm an approval - #124

Merged
vvillait88 merged 1 commit into
mainfrom
fix/as-003-fail-closed
Aug 28, 2026
Merged

Fail closed when the wallet screen cannot confirm an approval#124
vvillait88 merged 1 commit into
mainfrom
fix/as-003-fail-closed

Conversation

@vvillait88

Copy link
Copy Markdown
Contributor

Summary

Both wallet-OFAC paths branched on if decision == "deny", so everything that was not literally the string "deny" was permitted by structure: None from an unreadable response, and any decision value the API adds later. Both paths already deny on an API outage and describe themselves as strict-liability, so letting an unreadable answer through contradicted their own posture. On the compute-first path it would have settled the payment.

Sites: agentscore_commerce/checkout.py (_run_wallet_sanctions_only) and agentscore_commerce/checkout_compute_first.py.

Both now deny unless the decision is exactly "allow". A missing or null decision is an unreadable response rather than a verdict about the buyer, so it maps to api_error, matching how an outage is already reported on these paths; a decision that is present but not an approval keeps wallet_not_trusted.

Found by the Q3 2026 penetration test, worked with Varun. The node-commerce half is PR #118 there, and the API half has already landed in core.

Type of change

  • Bug fix (no breaking change)
  • New feature (no breaking change)
  • Breaking change (existing callers must update)
  • Docs, tests, or internal maintenance only

No exported surface changes. The only behavior difference is that a response which could not be read as an approval is now refused; a merchant relying on that was relying on the defect.

Public API

None. No signature, model or response shape changes.

Behavior changes for two response shapes that should not occur against a current API: an assess response with no decision, and one carrying an unrecognised decision value. Both now deny where they previously allowed, returning api_error and wallet_not_trusted respectively.

Test plan

uv run ruff check, uv run ruff format --check and uv run ty check all clean. uv run pytest: 1868 passed, 4 skipped, coverage 95.37% (above the 95% gate).

Five new tests across the two paths: missing decision, explicit null decision, and an unrecognised decision value, asserting on both the denial code and, for compute-first, that verify_payment was never called so no settle occurred.

Both guards were drilled rather than assumed: restoring the original decision == "deny" check in both files fails 5 of the new tests, and every one passes again once reverted.

Deliberately not added: a policy_result check. Neither path sends a merchant policy, so requiring one would deny every call, since policy_result is correctly null when no policy was submitted. That half of the finding lives in the node gate, which does send policies.

Checklist

  • Tests cover the new behavior, and the suite passes locally
  • Lint, format, and type checks pass
  • Docs and README examples updated if the public surface changed
  • No secrets, credentials, or personal data in the diff or the tests

Both wallet-OFAC paths branched on `if decision == "deny"`, so everything that
was not literally the string "deny" was permitted by structure: None from an
unreadable response, and any decision value the API adds later. Both paths
already deny on an API outage, describing themselves as strict-liability, so
letting an unreadable answer through contradicted their own posture. On the
compute-first path it would have settled the payment.

Both now deny unless the decision is exactly "allow". A missing or null decision
is an unreadable response rather than a verdict about the buyer, so it maps to
api_error, matching how an outage is already reported on these paths; a decision
that is present but not an approval keeps wallet_not_trusted.

Neither path sends a merchant policy, so neither gained a policy_result check.
Requiring one here would deny every call, since policy_result is correctly null
when no policy was submitted.

Found by the Q3 2026 penetration test. The node-commerce half is a separate
change, and the API half has already landed.
@vvillait88
vvillait88 merged commit 6b6a746 into main Aug 28, 2026
7 checks passed
@vvillait88
vvillait88 deleted the fix/as-003-fail-closed branch August 28, 2026 00:25
vvillait88 added a commit that referenced this pull request Aug 28, 2026
…an approval (#125)

## Summary

Version bump for the fail-closed change merged in #124.

Minor rather than patch, because runtime behavior changes. Both
wallet-OFAC paths branched on `decision == "deny"`, so everything that
was not literally that string was permitted **by structure**, including
a null decision from an unreadable response. On the compute-first path
that would have settled the payment.

## Type of change

- [ ] Bug fix (no breaking change)
- [ ] New feature (no breaking change)
- [ ] Breaking change (existing callers must update)
- [x] Docs, tests, or internal maintenance only

The bump itself is maintenance; the behavior it releases landed in #124
and is classified there.

## Public API

No signature, model or response shape changes.

Behavior changes for two response shapes that should not occur against a
current API: an assess response with no `decision`, and one carrying an
unrecognised decision value. Both now deny where they previously
allowed, returning `api_error` and `wallet_not_trusted` respectively.
Nothing to migrate.

## Test plan

On the bumped tree: `uv run ruff check`, `uv run ruff format --check`
and `uv run ty check` all clean; `uv run pytest` gives 1868 passed, 4
skipped, coverage 95.37% against the 95% gate.

The behavior itself is verified in #124, including a drill that restores
the original `decision == "deny"` check in both files and confirms five
tests fail.

## Checklist

- [x] Tests cover the new behavior, and the suite passes locally
- [x] Lint, format, and type checks pass
- [x] Docs and README examples updated if the public surface changed
- [x] No secrets, credentials, or personal data in the diff or the tests
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