Reject unknown policy condition keys instead of silently ignoring them

The worst failure mode found in this codebase so far, because it is one
keystroke away and completely invisible. Proved against a policy an operator
would believe caps spending at $5,000:

    {"max_amount":  5000}        $999,999 -> DENY    (correct)
    {"max_ammount": 5000}        $999,999 -> ALLOW
    {"maxAmount":   5000}        $999,999 -> ALLOW
    {"require_approval_ovr": 5}  $999,999 -> ALLOW

Unrecognised keys are simply never read by _check_constraints, so for a
security control they fail OPEN. The console authors `conditions` as a raw JSON
textarea with no schema, validation or autocomplete, the policy saves with a
201, and the console renders it as an active constraint. Nothing anywhere
reveals the ceiling is inert.

It also removes a real inconsistency in the engine's own philosophy: a
malformed VALUE already fails closed (`{"max_amount": "abc"}` routes to human
review), while an unknown KEY failed open. Both are the same operator error.

Validation lives on PolicyIn rather than in the route handlers, so every write
path — create, update, preview and rollback — is covered by construction rather
than by remembering to call it. Errors name the offending key, suggest the
intended one via difflib, and list all six valid keys, because the person
reading this is mid-typo and needs the fix, not a schema reference.

Chose write-time rejection over a console form (which would leave
Terraform/script/API authors unprotected — how production policies actually get
written) and over warn-only (weaker than the fail-closed precedent the engine
already sets for malformed values).

19 tests: every valid key accepted, a pinned assertion that the allowlist
matches what the engine reads so the two can't drift, eight realistic typos
rejected, the error message asserted to be actionable, a valid+invalid mix
still rejected, and an end-to-end API check that the write is refused 422 while
the correctly-spelled policy still creates. Existing seeds and demos needed no
changes — they only ever used valid keys.

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

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

Author: madhusudanpatnaik

CHANGELOG.md

@@ -42,6 +42,20 @@ have a chain worth retaining.
 
 ### Fixed
 
+- **A typo in a policy condition key silently disabled that control** (high) —
+  keys the engine doesn't recognise are never read, so for a security control
+  they fail *open*: a policy written `{"max_ammount": 5000}` (one transposed
+  letter, authored in the console's raw-JSON textarea) saved successfully,
+  showed as active, and allowed a $999,999 action through what the operator
+  believed was a $5,000 ceiling. Unknown condition keys are now rejected at
+  write time with a "did you mean" suggestion and the list of valid keys. This
+  also removes an inconsistency — a malformed *value* already failed closed
+  while an unknown *key* failed open.
+- **An unusable vault key surfaced only on the first governed call** — seeding
+  before setting `AGENTGUARD_SECRET_KEY` (the order the README implies) left
+  connector credentials encrypted under the dev default; the server booted
+  clean and failed later with "key rotated?" despite no rotation. Now checked
+  at startup with an actionable message.
 - **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`

agentguard/schemas.py

@@ -5,7 +5,7 @@
 from datetime import datetime
 from typing import Any
 
-from pydantic import BaseModel, ConfigDict, Field
+from pydantic import BaseModel, ConfigDict, Field, field_validator
 
 
 # --- Auth -------------------------------------------------------------------
@@ -67,6 +67,36 @@ class OrgUserIn(BaseModel):
 
 # --- Roles & policies -------------------------------------------------------
 
+# Every key the policy engine actually reads out of `conditions`. A key outside
+# this set is silently ignored at evaluation time, which for a security control
+# means it FAILS OPEN: `{"max_ammount": 5000}` (one typo) reads as "no ceiling",
+# and a $999,999 action sails through a policy the operator believes caps it at
+# $5,000. The console authors this field as a raw JSON textarea, so a typo is a
+# keystroke away and nothing downstream would ever surface it.
+#
+# Note the inconsistency this removes: a malformed VALUE (`{"max_amount":
+# "abc"}`) already fails closed to human review in engine._check_constraints,
+# while an unknown KEY failed open. Both are operator error; both should now be
+# caught, and catching this one at write time is strictly better than at
+# evaluation time because the operator is still looking at the policy.
+_VALID_POLICY_CONDITIONS = frozenset({
+    "max_amount",           # hard spending ceiling -> deny above
+    "require_approval_over",  # spending threshold -> human review above
+    "rate_limit",           # {"count": N, "per_seconds": S}
+    "time_window",          # {"start": "09:00", "end": "17:00"} UTC
+    "attributes",           # ABAC predicates on subject.* / resource.* / env.*
+    "risk_step_up",         # per-policy override of the global step-up threshold
+})
+
+
+def _suggest_condition_key(unknown: str) -> str | None:
+    """Best-effort 'did you mean' for a mistyped condition key."""
+    import difflib
+
+    matches = difflib.get_close_matches(unknown, _VALID_POLICY_CONDITIONS, n=1, cutoff=0.6)
+    return matches[0] if matches else None
+
+
 class PolicyIn(BaseModel):
     name: str = ""
     effect: str = Field(default="allow", pattern="^(allow|deny)$")
@@ -77,6 +107,23 @@ class PolicyIn(BaseModel):
     priority: int = 0
     enabled: bool = True
 
+    @field_validator("conditions")
+    @classmethod
+    def _reject_unknown_conditions(cls, v: dict[str, Any]) -> dict[str, Any]:
+        unknown = sorted(set(v) - _VALID_POLICY_CONDITIONS)
+        if not unknown:
+            return v
+        details = []
+        for key in unknown:
+            suggestion = _suggest_condition_key(key)
+            details.append(f"{key!r}" + (f" (did you mean {suggestion!r}?)" if suggestion else ""))
+        raise ValueError(
+            "unknown policy condition key(s): " + ", ".join(details)
+            + ". An unrecognised key is ignored when the policy is evaluated, so the "
+            "constraint you intended would not be enforced. Valid keys: "
+            + ", ".join(sorted(_VALID_POLICY_CONDITIONS))
+        )
+
 
 class PolicyOut(PolicyIn):
     model_config = ConfigDict(from_attributes=True)

tests/test_policy_condition_keys.py

@@ -0,0 +1,93 @@
+"""Unknown policy condition keys must be rejected at write time.
+
+An unrecognised key in `conditions` is simply never read by the policy engine,
+so for a security control it FAILS OPEN. Proved before this validation existed,
+against a policy the operator believed capped spending at $5,000:
+
+    {"max_amount":  5000}  ->  $999,999  DENY    (correct)
+    {"max_ammount": 5000}  ->  $999,999  ALLOW   (one typo, ceiling gone)
+    {"maxAmount":   5000}  ->  $999,999  ALLOW
+    {"require_approval_ovr": 5} -> $999,999 ALLOW
+
+The console authors this field as a raw JSON textarea, so the typo is one
+keystroke away, the policy saves successfully, and nothing downstream ever
+reveals that the constraint is inert.
+
+This also removes an inconsistency: a malformed VALUE already failed closed
+(`{"max_amount": "abc"}` routes to human review in _check_constraints), while
+an unknown KEY failed open.
+"""
+
+from __future__ import annotations
+
+import pytest
+from pydantic import ValidationError
+
+from agentguard.schemas import _VALID_POLICY_CONDITIONS, PolicyIn
+
+
+@pytest.mark.parametrize("key", sorted(_VALID_POLICY_CONDITIONS))
+def test_every_documented_key_is_accepted(key):
+    """The allowlist must not reject a key the engine actually honours."""
+    assert PolicyIn(conditions={key: {}}).conditions == {key: {}}
+
+
+def test_allowlist_matches_what_the_engine_reads():
+    """Guard against the allowlist and the engine drifting apart.
+
+    If someone teaches the engine a new condition key without adding it here,
+    that key becomes un-writable through the API; if they remove one, a dead
+    key stays writable. Both are caught by pinning the set explicitly.
+    """
+    assert _VALID_POLICY_CONDITIONS == {
+        "max_amount", "require_approval_over", "rate_limit",
+        "time_window", "attributes", "risk_step_up",
+    }
+
+
+@pytest.mark.parametrize("bad", [
+    "max_ammount", "maxAmount", "max_amount_usd", "require_approval_ovr",
+    "MAX_AMOUNT", "rate_limits", "timewindow", "wibble",
+])
+def test_unknown_keys_are_rejected(bad):
+    with pytest.raises(ValidationError):
+        PolicyIn(conditions={bad: 5000})
+
+
+def test_error_names_the_key_the_valid_set_and_a_suggestion():
+    """The message has to be actionable — this is an operator typo, at 2am."""
+    with pytest.raises(ValidationError) as exc:
+        PolicyIn(conditions={"max_ammount": 5000})
+    msg = str(exc.value)
+    assert "max_ammount" in msg, "must name the offending key"
+    assert "did you mean 'max_amount'" in msg, "must suggest the intended key"
+    assert "max_amount" in msg and "rate_limit" in msg, "must list valid keys"
+
+
+def test_a_valid_key_alongside_a_typo_still_rejects():
+    """Partial correctness must not mask the broken half."""
+    with pytest.raises(ValidationError):
+        PolicyIn(conditions={"max_amount": 5000, "require_approval_ovr": 500})
+
+
+def test_empty_conditions_are_fine():
+    assert PolicyIn(conditions={}).conditions == {}
+
+
+def test_rejected_over_the_api(client, admin_headers):
+    """End-to-end: the API refuses the write rather than storing a dead policy."""
+    role = client.post("/api/roles", json={"name": "typo-role"},
+                       headers=admin_headers).json()
+    r = client.post(f"/api/roles/{role['id']}/policies", headers=admin_headers,
+                    json={"effect": "allow", "resource": "payment:**",
+                          "actions": ["payment.refund"],
+                          "conditions": {"max_ammount": 5000}})
+    assert r.status_code == 422, r.text
+    assert "max_ammount" in r.text
+
+    # And the correctly-spelled policy is still accepted.
+    ok = client.post(f"/api/roles/{role['id']}/policies", headers=admin_headers,
+                     json={"effect": "allow", "resource": "payment:**",
+                           "actions": ["payment.refund"],
+                           "conditions": {"max_amount": 5000}})
+    assert ok.status_code == 201, ok.text