Catalog push accepts a term that alias folding then makes invalid

conformance/licenseterm_canonical_disjoint_test.go

@@ -0,0 +1,185 @@
+package conformance
+
+// Drift guard: the post-fold disjointness rule the SDKs enforce IS a rule the
+// contract publishes.
+//
+// Disjointness is asserted twice, at two tiers, over two different readings of
+// the same field. The wire tier's half is a protovalidate rule and needs no
+// guard — it is IN the descriptor. The ingest tier's half is SDK code, and it
+// cannot be a CEL rule for the reason the neighbouring ingest-tier checks give:
+// the fold resolves aliases out of a generated vocabulary, and a CEL expression
+// that re-listed that vocabulary would drift from it.
+//
+// So the rule exists twice — as SDK code, which is what the reference
+// implementations run, and as prose in ramp.proto, which is what a third-party
+// implementor reads. An implementor who read only the contract would otherwise
+// build an Exchange that stores the term this rule exists to refuse, and every
+// suite in every language would stay green. Nothing made the two agree; this
+// file does.
+//
+// The halves work differently, as in regschema_rule_test.go. That the rule
+// FIRES is read out of the committed vectors — conformance imports nothing from
+// sdk/, so the ids arrive as data. That the contract STATES it is checked by
+// reading the .proto SOURCE, because a comment does not survive into the
+// descriptor and the comment is the thing an implementor actually reads.
+//
+// The phrases below are clauses only this rule's sentences can produce. A guard
+// that matched a bare word would pass on text that never states the rule: the
+// word "canonicalised" already occurs in several unrelated paragraphs of this
+// file, and "disjoint" occurs in the wire rule's own id.
+
+import (
+	"encoding/json"
+	"os"
+	"strings"
+	"testing"
+
+	"buf.build/go/protovalidate"
+	"google.golang.org/protobuf/reflect/protoreflect"
+)
+
+const (
+	rampProtoSource = "../proto/ramp/v1/ramp.proto"
+
+	// The two ids. They are deliberately different strings: one name per rule, which
+	// licenseterm_rule_ids_test.go enforces across the whole ingest-tier id set.
+	canonicalDisjointRuleID = "restriction.canonical_disjoint"
+	wireDisjointRuleID      = "restriction.permitted_prohibited_disjoint"
+)
+
+// canonicalDisjointFires reports whether the committed license-term vectors record
+// the ingest-tier rule at all. Read as data: this package is the guard tier BELOW
+// the SDKs and imports none of them.
+func canonicalDisjointFires(t *testing.T) bool {
+	t.Helper()
+	b, err := os.ReadFile(licenseTermVectors)
+	if err != nil {
+		t.Fatalf("read %s: %v", licenseTermVectors, err)
+	}
+	var doc struct {
+		Validate []struct {
+			Violation *struct {
+				Rule string `json:"rule"`
+			} `json:"violation"`
+		} `json:"validate"`
+	}
+	if err := json.Unmarshal(b, &doc); err != nil {
+		t.Fatalf("decode %s: %v", licenseTermVectors, err)
+	}
+	for _, v := range doc.Validate {
+		if v.Violation != nil && v.Violation.Rule == canonicalDisjointRuleID {
+			return true
+		}
+	}
+	return false
+}
+
+// messageBody returns the source text between a declaration's opening brace and the
+// first closing brace in column 1 — the whole body of a top-level message or service,
+// including the comments inside it.
+func messageBody(t *testing.T, src, decl string) string {
+	t.Helper()
+	start := strings.Index(src, decl)
+	if start < 0 {
+		t.Fatalf("%s: %q not found — the guard is reading the wrong file or the declaration was renamed", rampProtoSource, decl)
+	}
+	rest := src[start:]
+	end := strings.Index(rest, "\n}\n")
+	if end < 0 {
+		t.Fatalf("%s: %q is never closed in column 1", rampProtoSource, decl)
+	}
+	return rest[:end]
+}
+
+// docComment returns the run of // lines immediately above a declaration.
+func docComment(t *testing.T, src, decl string) string {
+	t.Helper()
+	at := strings.Index(src, decl)
+	if at < 0 {
+		t.Fatalf("%s: %q not found", rampProtoSource, decl)
+	}
+	lines := strings.Split(src[:at], "\n")
+	// The declaration starts a line, so the split ends with the empty string before it.
+	// Dropping it is what makes the walk below start on the last comment line rather
+	// than stopping instantly on a non-comment.
+	if len(lines) > 0 && strings.TrimSpace(lines[len(lines)-1]) == "" {
+		lines = lines[:len(lines)-1]
+	}
+	var out []string
+	for i := len(lines) - 1; i >= 0; i-- {
+		line := strings.TrimSpace(lines[i])
+		if !strings.HasPrefix(line, "//") {
+			break
+		}
+		out = append([]string{line}, out...)
+	}
+	return strings.Join(out, "\n")
+}
+
+func TestCanonicalDisjointRuleIsStatedByTheContract(t *testing.T) {
+	if !canonicalDisjointFires(t) {
+		t.Fatalf("no vector records %q — either the rule is gone or the corpus stopped covering it, "+
+			"and this guard would be holding the contract to a rule nothing runs", canonicalDisjointRuleID)
+	}
+
+	b, err := os.ReadFile(rampProtoSource)
+	if err != nil {
+		t.Fatalf("read %s: %v", rampProtoSource, err)
+	}
+	src := string(b)
+
+	// Where the rule is declared, the contract must say that the declared rule reads
+	// the tokens as written and that the property is asserted again after the fold.
+	// Both halves are load-bearing: an implementor who takes the CEL rule for the
+	// whole rule builds the gap this exists to close.
+	restriction := messageBody(t, src, "message Restriction {")
+	for _, want := range []string{
+		canonicalDisjointRuleID,
+		"compares the tokens AS WRITTEN",
+		"two spellings of ONE token pass this rule",
+	} {
+		if !strings.Contains(restriction, want) {
+			t.Errorf("message Restriction does not state %q — the SDK refuses a term the contract does not say it refuses", want)
+		}
+	}
+
+	// And the service comment, which is where a publisher reads what the Exchange
+	// will run, must carry the rule in its ingest-tier list.
+	catalog := docComment(t, src, "service CatalogService {")
+	for _, want := range []string{
+		canonicalDisjointRuleID,
+		"BOTH tiers assert",
+	} {
+		if !strings.Contains(catalog, want) {
+			t.Errorf("the CatalogService comment does not state %q — its two-tier description is incomplete", want)
+		}
+	}
+}
+
+// The two rules must keep two names. The ingest-tier id set is held to that as a
+// whole elsewhere; this pins the specific pair, because these two are the ones a
+// reader is most likely to collapse into one.
+func TestCanonicalDisjointDoesNotDisplaceTheWireRule(t *testing.T) {
+	if canonicalDisjointRuleID == wireDisjointRuleID {
+		t.Fatal("the ingest-tier and wire-tier ids are the same string — one name for two rules")
+	}
+	var found bool
+	EachMessage(func(md protoreflect.MessageDescriptor) {
+		if string(md.Name()) != "Restriction" {
+			return
+		}
+		mr, err := protovalidate.ResolveMessageRules(md)
+		if err != nil || mr == nil {
+			return
+		}
+		for _, r := range mr.GetCel() {
+			if r.GetId() == wireDisjointRuleID {
+				found = true
+			}
+		}
+	})
+	if !found {
+		t.Errorf("Restriction no longer declares %q — the ingest-tier rule reads the canonicalised tokens "+
+			"and was never a replacement for the boundary check over the tokens as received", wireDisjointRuleID)
+	}
+}

docs/design-history.md

@@ -1090,7 +1090,8 @@ A pushed entry passes two tiers at the Exchange. The wire tier is protovalidate:
 applied to the request exactly as received. The ingest tier runs afterwards, over the
 canonicalised terms: restriction tokens are folded and alias-resolved to their
 registered form, then a bare `Pricing.unit` or `Quota.metric` that is not a registered
-token is rejected, while an unregistered restriction token and an `OBLIGATION_KIND_OTHER`
+token is rejected, as is a restriction whose permitted and prohibited lists name one
+token once folded, while an unregistered restriction token and an `OBLIGATION_KIND_OTHER`
 obligation without detail are accepted and reported in `PushResourcesResponse.warnings`.
 The first tier was always in the contract. The second lived only inside the reference
 Exchange, so a publisher learned what it would say by pushing and reading the refusal.
@@ -1150,6 +1151,54 @@ what it does pin, at three different strengths. The two JSON ports walk every me
 reachable from an entry explicitly for the cross-field rules, because their composed
 schemas attach per message and a top-level parse would never reach `terms[1].pricing`.
 
+The consequence of that order took a while to surface, and it is the reason the tier
+boundary is not simply a matter of where a check is cheapest to run. A rule evaluated
+before the fold is a rule about SPELLINGS. `restriction.permitted_prohibited_disjoint`
+is `this.permitted.all(p, !(p in this.prohibited))` — a string comparison standing in
+for a statement about tokens — and the vocabulary it reads has ten aliases and folds
+ASCII case on every axis. So a term naming `scrape` under permitted and `crawl` under
+prohibited passed the rule, because as written the two lists shared nothing, and the
+fold then collapsed both into one token sitting in both lists. The wire tier had
+cleared the very violation the ingest tier went on to create.
+
+What made it expensive was where it landed. The stored term rides on offers and an
+Exchange validates its own responses, so the refusal arrived at a different party, on
+a different RPC, an unbounded time later: discovery of that resource failed with an
+internal error, while the push that caused it had been answered as accepted. A check
+that runs on the wrong values does not merely miss things; it can certify the thing it
+was written to prevent.
+
+The property is now asserted at both tiers, and that is the general rule this records:
+a rule that compares two token-valued fields is only correct on canonical values, so
+when an axis gains canonicalisation, every rule that reads it must be re-examined —
+either the rule moves to the tier that sees canonical values, or canonicalisation moves
+ahead of the tier that carries the rule. Here the rule moved, because the fold resolves
+aliases out of a generated vocabulary and a CEL expression that re-listed that
+vocabulary would drift from it, which is the same reason membership is not a descriptor
+rule.
+
+Three details of the second half are deliberate. It has its own id
+(`restriction.canonical_disjoint`) rather than reusing the first's, because one name for
+two rules is what the ingest-tier id guard exists to prevent, and these two really are
+different rules: they read different values and can disagree. It canonicalises what it
+compares instead of trusting its caller to have folded first — both callers do fold, but
+a rule written to close an ordering gap that opened its own would be the same mistake
+one tier down, and the fold is a fixed point, so the cost is a pass over tokens already
+canonical. And neither rule suppresses the other: a term that fails both is reported by
+both, matching the tier the contract already describes as collecting every violation
+rather than stopping at the first. Suppression was considered and rejected on a concrete
+ground — the server binding defaults to validation off, so on a mount that never asks
+for the wire tier the ingest check is the only one a pushed term meets, and a rule that
+stayed silent because "the boundary already caught it" would catch nothing there.
+
+One boundary of that argument is worth stating, because the contract's own sentence
+invites the stronger reading. Both disjointness rules are scoped to a single
+`Restriction`; what makes one restriction the whole of an axis is
+`license_term.one_restriction_per_kind`, which is a wire-tier rule. Split the two lists
+across two restrictions of one kind and neither disjointness rule sees a collision. So a
+mount without the wire tier gains the second READING of the rule, not the per-axis
+property — the property has always been held jointly, and this change does not move it.
+
 ## A catalog client is its own constructor
 
 The usage-report decision recorded above settled a rule that generalises: a client's

docs/sdk-parity-matrix.md

@@ -12,7 +12,7 @@
 
 Go is the oracle (`sdk/go/{helpers,resolvers,core,connect,connectserver}`); Python and TS mirror it. This document is **generated** from the same two artifacts CI already enforces against the code, so it cannot drift from the real surface — a mismatch fails the API-surface gate or the corpus-completeness gate before it can reach this file.
 
-**At a glance:** 133 symbols at cross-language parity · 16 documented divergences · 183 Go-idiomatic exclusions · 35 conformance corpora, each tri-replayed.
+**At a glance:** 134 symbols at cross-language parity · 16 documented divergences · 183 Go-idiomatic exclusions · 35 conformance corpora, each tri-replayed.
 
 Layering (L1 pure trust core vs L2 I/O resolvers), the SSRF transport-wiring invariant, and naming conventions are recorded in [`design-history.md`](./design-history.md).
 
@@ -85,6 +85,7 @@ Legend: a name = the public face in that language · `—` = intentionally none
 | `RuleObligationOtherRequiresDetail` | `RULE_OBLIGATION_OTHER_REQUIRES_DETAIL` | `RULE_OBLIGATION_OTHER_REQUIRES_DETAIL` |
 | `RulePricingUnitRegistered` | `RULE_PRICING_UNIT_REGISTERED` | `RULE_PRICING_UNIT_REGISTERED` |
 | `RuleQuotaMetricRegistered` | `RULE_QUOTA_METRIC_REGISTERED` | `RULE_QUOTA_METRIC_REGISTERED` |
+| `RuleRestrictionCanonicalDisjoint` | `RULE_RESTRICTION_CANONICAL_DISJOINT` | `RULE_RESTRICTION_CANONICAL_DISJOINT` |
 | `RuleRestrictionTokenRegistered` | `RULE_RESTRICTION_TOKEN_REGISTERED` | `RULE_RESTRICTION_TOKEN_REGISTERED` |
 | `RuleViolation` | `RuleViolation` | `RuleViolation` |
 | `RuleWarning` | `RuleWarning` | `RuleWarning` |

proto/CHANGELOG.md

@@ -41,6 +41,30 @@ request-signing key already resolved. Holder binding for delegations still
 matches the wire signer, so a Broker may project a delegated request only when
 the agent has delegated to the Broker's key.
 
+**A term is now checked for permitted/prohibited disjointness a second time, over
+the canonicalised tokens (`restriction.canonical_disjoint`; SDK behaviour change
+plus a comment clarification, no wire change).** `restriction.permitted_prohibited_disjoint`
+runs at the wire tier, over the request exactly as received, so it compares token
+SPELLINGS. Ten registered aliases resolve to eight distinct restriction tokens —
+`scrape` is a registered alias of `crawl`, `adapt` and `derivative` both mean
+`modify`, `personal` means `individual` — and every axis also folds ASCII case. A term naming
+one spelling under `permitted` and another under `prohibited` therefore passed the
+boundary check and became, once the ingest tier folded it, a stored term with the
+same token in both lists.
+
+Nothing looked at it again, and the failure surfaced elsewhere: the term rides on
+offers, an Exchange validates its own responses, and so every discovery request
+returning that resource answered with an internal error while the push had looked
+clean. The ingest tier now asserts the same property over the canonical values,
+under its own rule id, in `ValidateLicenseTerm` and its Python and TypeScript twins.
+The boundary rule is unchanged.
+
+Disjointness is now the one property both tiers assert, deliberately: they read
+different values, neither suppresses the other, and a deployment that does not mount
+the wire tier still gets the second. The `Restriction` and `CatalogService` comments
+say so, and a conformance guard holds the contract's statement to the rule the SDKs
+run.
+
 **Signed delivery URLs are documented as Ed25519 signed by the Exchange and
 verified with its published public key, not HMAC-SHA256 over a shared secret
 (documentation correction; no wire change).** Since the initial public snapshot

proto/ramp/v1/ramp.proto

@@ -1231,6 +1231,24 @@ message License {
 //   USER_TYPE — RAMP user/organization categories
 message Restriction {
   // A token cannot be both permitted and prohibited on the same axis.
+  //
+  // Read that as a property of the AXIS, held by two rules together: this one is
+  // scoped to a single Restriction, and one restriction per kind — the rule on
+  // LicenseTerm below — is what makes one Restriction the whole of an axis. Split
+  // the two lists across two restrictions of the same kind and neither disjointness
+  // rule sees a collision; that shape is refused by the one-per-kind rule instead,
+  // at the wire tier.
+  //
+  // This rule compares the tokens AS WRITTEN, because it runs at the wire tier,
+  // over the request exactly as received. Several tokens have more than one
+  // accepted spelling — a registered alias beside its canonical form, and either
+  // one in any ASCII case — so two spellings of ONE token pass this rule and
+  // become the same token when the ingest tier folds them. The ingest tier
+  // therefore asserts the same property a second time, over the canonicalised
+  // tokens, under the SDK rule id restriction.canonical_disjoint; see
+  // CatalogService. Both refuse the term, and a term that fails both is reported
+  // by both — the second is not a fallback for the first, it reads different
+  // values.
   option (buf.validate.message).cel = {
     id: "restriction.permitted_prohibited_disjoint"
     message: "a token cannot appear in both permitted and prohibited"
@@ -2281,16 +2299,26 @@ enum IngestionSource {
 // GEOGRAPHY; a non-ASCII byte is never folded) and alias-resolved to their
 // registered form — the aliases are authored beside the tokens, on the
 // RestrictionKind values — after which a bare (non-namespaced) Pricing.unit or
-// Quota.metric that is not a registered token is rejected, while an
-// unregistered restriction token and an OBLIGATION_KIND_OTHER obligation
-// without detail are accepted and reported in PushResourcesResponse.warnings.
+// Quota.metric that is not a registered token is rejected, as is a restriction
+// whose permitted and prohibited lists name one token once folded
+// (restriction.canonical_disjoint), while an unregistered restriction token and
+// an OBLIGATION_KIND_OTHER obligation without detail are accepted and reported
+// in PushResourcesResponse.warnings.
+//
+// Disjointness is the one property BOTH tiers assert, and deliberately so: the
+// wire tier reads the tokens as written and the ingest tier reads what the fold
+// produced, so a term the boundary clears can still be refused here. A deployment
+// that does not mount the wire tier still gets the second of those two readings —
+// it does not thereby get the one-per-kind rule that makes a restriction the whole
+// of its axis, which stays a wire-tier rule.
+//
 // The SDK ships both tiers as a publisher-side pre-check; the Exchange's own
 // run of them is the deciding one.
 //
 // A push is ALL-OR-NOTHING, at both tiers. A hard rejection anywhere in the
 // submission — an envelope or term rule at the wire tier, an unregistered bare
-// unit or metric at the ingest tier — refuses the entire submission and persists
-// nothing; the publisher fixes and resubmits the whole set. The refusal names
+// unit or metric or a restriction that is not disjoint once folded at the ingest
+// tier — refuses the entire submission and persists nothing; the publisher fixes and resubmits the whole set. The refusal names
 // the entries that failed and why, and that per-entry detail is REPORTING, never
 // partial acceptance: no entry of a refused submission is stored. Warnings do
 // not refuse anything.