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"] == ["/"]