From ed8b9170bfa7d25dd43df5f62d0cd8ca0563e65d Mon Sep 17 00:00:00 2001 From: henleda Date: Mon, 27 Jul 2026 09:30:40 -0500 Subject: [PATCH] fix(apply): stop normalization widening an exact-path DENY to the whole LB MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `normalize_service_policy_spec` nested-merges each matcher against `_RULE_DEFAULTS` so a partial value keeps its required sub-keys. For `path` that also injected the default `prefix_values: ["/"]` into any rule that named only `exact_values` / `regex_values` / `suffix_values` — and XC ORs the value lists, so a surgical one-endpoint DENY denied every path on the load balancer. Verified live on vpcopilot-lab (snapshot + verified rollback, policy deleted after): before normalized DENY path {"prefix_values": ["/"], "exact_values": ["/__widening-probe__"]} GET /__widening-probe__ -> 403 GET / -> 403 GET /api/pay -> 403 after normalized DENY path {"exact_values": ["/__widening-probe__"]} GET /__widening-probe__ -> 403 GET / -> 307 GET /api/pay -> 405 (both baseline) 12 of the 31 generated rules on disk are exact-only and were being widened. Not an outage — the safety spine catches it, because the legit probe is blocked too, so validation fails and rolls back. The cost is effectiveness: an exact-path band-aid could not pass validation on the first attempt, burning refine attempts or being reported as a failure when the policy was correct and normalization broke it. The nested merge exists to supply required STRUCTURAL sub-keys, not a matching value, so the fix drops only the inherited catch-all when the rule named a path itself. A rule naming NO path still gets `prefix_values: ["/"]` — that is what makes `generate`'s trailing allow-all work against XC's default-deny. Tests pin both directions, written before the fix: exact/regex/suffix-only lose the catch-all; an explicit prefix, prefix+exact together, a no-path rule, the trailing allow-all, the structural sub-keys and every other nested matcher are unchanged. Diffed old vs new normalization over all 15 service-policy artifacts on disk: 12 changed, 3 identical, and every changed rule differed ONLY in `path` with all other fields byte-identical. 200 pass. Co-Authored-By: Claude Opus 5 (1M context) --- src/vpcopilot/apply.py | 12 +++++++ tests/test_normalize.py | 70 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+) diff --git a/src/vpcopilot/apply.py b/src/vpcopilot/apply.py index eee2a32..ec22f5d 100644 --- a/src/vpcopilot/apply.py +++ b/src/vpcopilot/apply.py @@ -58,6 +58,9 @@ def _sp_block(spec: dict) -> dict: "jwt_claims": [], } _NESTED = ("path", "http_method", "body_matcher", "domain_matcher", "label_matcher", "user_identity_matcher") +# XC ORs these lists inside a path matcher — so a rule that names ANY of them must not also carry +# the catch-all `prefix_values: ["/"]` default. See normalize_service_policy_spec. +_PATH_VALUE_KEYS = ("prefix_values", "exact_values", "regex_values", "suffix_values") # A valid default Bot Defense policy (flag-only mitigation on all paths) — the exact shape XC # requires (protected endpoint + flow-label choice + mitigation), taken from a live LB config. @@ -93,6 +96,15 @@ def normalize_service_policy_spec(spec: dict) -> dict: for key in _NESTED: # nested-merge matchers so partial values keep required sub-keys if isinstance(rs.get(key), dict): merged[key] = {**_RULE_DEFAULTS[key], **rs[key]} + # …but the nested merge exists to supply required STRUCTURAL sub-keys, not a matching value. + # A rule naming an exact/regex/suffix path was also inheriting the default catch-all + # `prefix_values: ["/"]`, and XC ORs those lists — so a surgical one-endpoint DENY denied + # the whole LB. Verified live on vpcopilot-lab: exact-only DENY + injected prefix → 403 on + # every path; without it → 403 only on the named path. A rule naming NO path still gets the + # catch-all, which is what makes the trailing allow-all work. + src_path = rs.get("path") + if isinstance(src_path, dict) and any(src_path.get(k) for k in _PATH_VALUE_KEYS): + merged["path"] = {**merged["path"], "prefix_values": list(src_path.get("prefix_values") or [])} # XC wants a LIST for these matchers (query_params, headers, cookie_matchers, …); a weaker # model often emits a single matcher OBJECT — coerce it, else XC 400s "cannot unmarshal # object into []json.RawMessage". diff --git a/tests/test_normalize.py b/tests/test_normalize.py index 6eaac08..e17098a 100644 --- a/tests/test_normalize.py +++ b/tests/test_normalize.py @@ -38,3 +38,73 @@ def test_list_matcher_object_coerced_to_list(): # an empty object collapses to [] (a valid empty list), not [{}] empty = {"rule_list": {"rules": [{"spec": {"action": "DENY", "query_params": {}}}]}} assert normalize_service_policy_spec(empty)["rule_list"]["rules"][0]["spec"]["query_params"] == [] + + +# ---- path widening: a named path value must not inherit the catch-all prefix ---- +# Verified live on vpcopilot-lab 2026-07-27: XC ORs the path value lists, so a DENY carrying both +# exact_values ["/x"] and the default prefix_values ["/"] denies EVERY path. 12 of the 31 generated +# rules on disk are exact-only and were being widened this way. + +def _deny(path, **extra): + spec = {"rule_list": {"rules": [{"spec": {"action": "DENY", "http_method": {"methods": ["GET"]}, + **({"path": path} if path is not None else {}), **extra}}]}} + return normalize_service_policy_spec(spec)["rule_list"]["rules"][0]["spec"]["path"] + + +def test_exact_only_path_does_not_gain_the_catchall_prefix(): + p = _deny({"exact_values": ["/users/v1/_debug"]}) + assert p["exact_values"] == ["/users/v1/_debug"] + assert p["prefix_values"] == [] # the bug injected ["/"] here -> denied every path + + +def test_regex_only_path_does_not_gain_the_catchall_prefix(): + assert _deny({"regex_values": ["api/v1/x402/.*"]})["prefix_values"] == [] + + +def test_suffix_only_path_does_not_gain_the_catchall_prefix(): + assert _deny({"suffix_values": ["/admin"]})["prefix_values"] == [] + + +# ---- pins on behaviour that must NOT change ---- +def test_explicit_prefix_is_preserved_unchanged(): + p = _deny({"prefix_values": ["/api/pay"]}) + assert p["prefix_values"] == ["/api/pay"] and p["exact_values"] == [] + + +def test_prefix_and_exact_together_are_both_kept(): + p = _deny({"prefix_values": ["/api"], "exact_values": ["/health"]}) + assert p["prefix_values"] == ["/api"] and p["exact_values"] == ["/health"] + + +def test_rule_with_no_path_still_matches_everything(): + """A rule that names no path at all must still get the catch-all — this is what makes the + trailing allow-all work when a model omits its path.""" + assert _deny(None)["prefix_values"] == ["/"] + + +def test_empty_path_object_still_matches_everything(): + assert _deny({})["prefix_values"] == ["/"] + + +def test_structural_subkeys_are_still_filled_on_a_named_path(): + p = _deny({"exact_values": ["/x"]}) + assert p["transformers"] == [] and p["invert_matcher"] is False + assert set(p) == {"prefix_values", "exact_values", "regex_values", "suffix_values", + "transformers", "invert_matcher"} + + +def test_other_nested_matchers_are_untouched_by_the_path_rule(): + r = normalize_service_policy_spec({"rule_list": {"rules": [{"spec": { + "action": "DENY", "path": {"exact_values": ["/x"]}, + "http_method": {"methods": ["POST"]}, + "body_matcher": {"regex_values": ["amount[^0-9-]*-[0-9]"]}, + }}]}})["rule_list"]["rules"][0]["spec"] + assert r["http_method"]["methods"] == ["POST"] and "invert_matcher" in r["http_method"] + assert r["body_matcher"]["regex_values"] == ["amount[^0-9-]*-[0-9]"] + assert r["waf_action"] == {"none": {}} # unrelated required defaults still filled + + +def test_catchall_allow_rule_is_unaffected(): + """The trailing allow-all `generate` emits uses prefix_values ["/"] explicitly — it must keep + matching everything, or legit traffic 403s on XC's default-deny.""" + assert _deny({"prefix_values": ["/"]})["prefix_values"] == ["/"]