Stop negative amounts bypassing spending ceilings and approvals

Found while auditing the risk engine. Verified against the exact policy the
shipped pitch demo uses (max_amount=5000, require_approval_over=500):

    amount= 9500 -> DENY              (correct)
    amount=-9500 -> ALLOW             (no ceiling, no human, no risk score)

Both bounds in _check_constraints are `amount > limit`, so any negative value
cleared the hard ceiling AND the human-review threshold simultaneously. risk.py
independently gated its score on `amount > 0`, so the same request also scored
zero risk — the two systems that should have caught it failed the same way for
the same reason. Nothing validates the sign anywhere, including the request
schema. Some payment APIs treat a negative refund as a charge, which turns this
from a nonsense value into a transfer in the opposite direction, unbounded.

The fix is what the engine's own comment already claimed it did. That comment
reads "a policy that constrains amount must not be bypassable by simply
omitting (or malforming) the amount — route such actions to a human", but only
the `amount is None` case was implemented; a negative parses fine through
_coerce_number and sailed past. Negative amounts now take that same
already-prescribed route rather than inventing sign semantics — notably NOT by
silently comparing abs(), which would reinterpret the caller's intent and
execute a transfer they did not ask for.

risk.py now scores by magnitude (abs) rather than signed value, so -9500 and
+9500 carry identical exposure weight and the risk signal agrees with the
policy decision instead of calling the negative case harmless.

11 regression tests covering the bypass values, that positive amounts are
completely unchanged (250 allow / 501 approval / 5001 deny), that the
pre-existing missing-amount rule still holds, that 0 is NOT swept up as
negative, and that a policy with no amount condition doesn't suddenly start
demanding approval.

450 passed, 2 skipped; ruff- and mypy-clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Author: madhusudanpatnaik

CHANGELOG.md

@@ -42,6 +42,15 @@ have a chain worth retaining.
 
 ### Fixed
 
+- **Negative amounts bypassed spending ceilings and approval thresholds**
+  (high) — every amount bound was `amount > limit`, so against a policy with
+  `max_amount: 5000` and `require_approval_over: 500`, an `amount` of `-9500`
+  was ALLOWED outright while `+9500` was correctly denied: no ceiling, no
+  human, and no risk score (which separately gated on `amount > 0`). Payment
+  APIs that read a negative refund as a charge turn this into a transfer the
+  ceiling exists to prevent. Negative amounts now take the same route the
+  engine already prescribed for a malformed amount — human review — and the
+  risk score measures exposure by magnitude.
 - **Cross-tenant IDOR in SCIM provisioning** (critical) — every SCIM endpoint
   operated on the user table by raw primary key with no `org_id` scoping, behind
   a single deployment-wide bearer token. One tenant's IdP token could list,

agentguard/policy/engine.py

@@ -321,6 +321,24 @@ def _check_constraints(
                 "amount — routed to human review.",
             )
 
+        # A NEGATIVE amount is malformed for this purpose too, and used to sail
+        # straight through: both bounds below are `amount > limit`, so -9500
+        # cleared a max_amount of 5000 AND a require_approval_over of 500 with
+        # no ceiling and no human, while +9500 was correctly denied. Some
+        # payment APIs also treat a negative refund as a charge, which turns
+        # the bypass into a transfer in the opposite direction. There is no
+        # legitimate negative value for a spend ceiling to bound, so this takes
+        # the same route the comment above already prescribes for a malformed
+        # amount rather than inventing sign semantics (e.g. silently comparing
+        # abs(), which would reinterpret the caller's intent).
+        if amount_constrained and amount is not None and amount < 0:
+            return (
+                Decision.REQUIRE_APPROVAL,
+                f"Amount-constrained action submitted with a negative amount "
+                f"({amount:g}) — cannot be bounded by a spending limit, routed "
+                "to human review.",
+            )
+
         # All operator-supplied numeric conditions are parsed with the fail-safe
         # coercer and fail CLOSED on a malformed value — a bad condition must
         # never raise on the authorize hot path (that would 500 the decision and

agentguard/risk.py

@@ -87,8 +87,14 @@ def assess(db: Session, agent: Agent, req: ActionRequest, decision: PolicyDecisi
         factors.append("egress")
 
     amount = _coerce_amount((req.metadata or {}).get("amount"))
-    if amount is not None and amount > 0:
-        bump = min(int(amount / 1000 * 5), 20)
+    # Magnitude, not signed value: `amount > 0` meant a -9500 refund scored the
+    # same zero risk as no amount at all, even though its exposure is identical
+    # to +9500 (and some payment APIs read a negative refund as a charge). The
+    # policy engine routes negative amounts to human review; the risk score
+    # should agree with that rather than call them harmless.
+    if amount is not None and amount != 0:
+        exposure = abs(amount)
+        bump = min(int(exposure / 1000 * 5), 20)
         if bump:
             score += bump
             factors.append(f"amount:{amount:g}")

tests/test_negative_amount.py

@@ -0,0 +1,83 @@
+"""Negative amounts must not bypass spending ceilings or approval thresholds.
+
+Verified bypass before this fix, against the exact policy the shipped pitch
+demo uses (max_amount=5000, require_approval_over=500):
+
+    amount= 9500 -> DENY              (correct)
+    amount=-9500 -> ALLOW             (no ceiling, no human, no risk score)
+
+Both bounds in _check_constraints are `amount > limit`, so any negative value
+cleared both. risk.py additionally gated its score on `amount > 0`, so the same
+request also scored zero risk. Some payment APIs treat a negative refund as a
+charge, which turns this from a nonsense value into a transfer the ceiling was
+meant to prevent.
+"""
+
+from __future__ import annotations
+
+import pytest
+
+from agentguard.dlp.scanner import DLPResult
+from agentguard.models import Decision, Effect, Policy
+from agentguard.policy.engine import ActionRequest, PolicyEngine
+
+CEILING, REVIEW_OVER = 5000, 500
+
+
+def _policy() -> Policy:
+    return Policy(
+        id=1, name="refunds", effect=Effect.ALLOW, resource="payment:**",
+        actions=["payment.refund"], enabled=True, priority=0,
+        conditions={"max_amount": CEILING, "require_approval_over": REVIEW_OVER},
+    )
+
+
+def _decide(amount):
+    req = ActionRequest(action_type="payment.refund",
+                        resource="payment:stripe:refund",
+                        metadata={} if amount is None else {"amount": amount})
+    return PolicyEngine().evaluate(req, [_policy()],
+                                   dlp=DLPResult(findings=[], redacted=None))
+
+
+@pytest.mark.parametrize("amount", [-1, -0.01, -500, -9500, -1_000_000, "-9500"])
+def test_negative_amounts_never_pass_silently(amount):
+    """The bypass: any of these previously returned ALLOW."""
+    assert _decide(amount).decision == Decision.REQUIRE_APPROVAL
+
+
+def test_positive_amounts_are_unchanged():
+    """The fix must not alter correct existing behaviour."""
+    assert _decide(250).decision == Decision.ALLOW
+    assert _decide(REVIEW_OVER + 1).decision == Decision.REQUIRE_APPROVAL
+    assert _decide(CEILING + 1).decision == Decision.DENY
+
+
+def test_missing_amount_still_routes_to_human():
+    """Pre-existing fail-closed rule this fix is modelled on."""
+    assert _decide(None).decision == Decision.REQUIRE_APPROVAL
+
+
+def test_zero_is_allowed_not_treated_as_negative():
+    """0 is a bounded, verifiable amount — it must not be swept up by the fix."""
+    assert _decide(0).decision == Decision.ALLOW
+
+
+def test_unconstrained_policy_ignores_amount_sign():
+    """A policy with no amount condition must not suddenly demand approval."""
+    p = Policy(id=2, name="open", effect=Effect.ALLOW, resource="payment:**",
+               actions=["payment.refund"], enabled=True, priority=0, conditions={})
+    req = ActionRequest(action_type="payment.refund", resource="payment:x",
+                        metadata={"amount": -9500})
+    assert PolicyEngine().evaluate(
+        req, [p], dlp=DLPResult(findings=[], redacted=None)).decision == Decision.ALLOW
+
+
+def test_risk_scores_negative_amount_by_magnitude():
+    """risk.py gated on `amount > 0`, so -9500 scored as if there were no amount."""
+    from agentguard.risk import _coerce_amount
+
+    assert _coerce_amount(-9500) == -9500.0
+    # The scoring path is exercised via assess(); here we assert the property
+    # that motivated the change: exposure is magnitude, not signed value.
+    assert abs(_coerce_amount(-9500)) == abs(_coerce_amount(9500))