amadan
domains: revert SetClaimPaths validation ordering per review
Review round 1 caught that hoisting input validation into the CAS loop was
unforced: SetClaimCapabilities (same file) already validates
catchall/wildcard before its loop, and a validation error that depends only
on the caller's own input can never function as an existence or ownership
oracle, since the same input produces the same error regardless of who is
asking or whether the host exists. Restored validation to before the loop,
matching the brief and SetClaimCapabilities, and removed the comment that
asserted an unfounded security necessity.
The test that had appeared to force the reordering, the second case of
TestSetClaimPathsIsOwnerGated, used a placeholder target ("b") that happened
to fail NormalizeAliasHost's dot check — a fixture bug, not a deliberate
probe. Fixed the fixture to use a normalised target so the case tests
ownership rather than incidentally tripping validation, and added
TestSetClaimPathsRejectsUnnormalisedTarget to restore coverage of the
malformed-target error path against an owned, existing claim.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 files changed,
+39
−20
internal/domains/claims.go+14 −19internal/domains/paths_test.go+25 −1
diff --git a/internal/domains/claims.go b/internal/domains/claims.go| index 402eb99c4..e8fbda706 100644 |
| --- a/internal/domains/claims.go |
| +++ b/internal/domains/claims.go |
| @@ -389,6 +389,20 @@ func validPathPrefix(p string) bool { |
| // the claim gets the neutral ErrHostUnavailable, and a missing claim answers |
| // identically, so this door is not an existence oracle. |
| func SetClaimPaths(ctx context.Context, s release.Store, host, account, app string, paths map[string]string) (changed bool, err error) { |
| + if len(paths) > MaxPathsPerClaim { |
| + return false, ErrTooManyPaths |
| + } |
| + for prefix, target := range paths { |
| + if !validPathPrefix(prefix) { |
| + return false, ErrBadPathPrefix |
| + } |
| + // The target must be a plausible hostname. Whether it is a route this |
| + // account owns is decided by the reconcile pass, which — unlike this |
| + // door — can see the registry. |
| + if n, ok := NormalizeAliasHost(target); !ok || n != target { |
| + return false, fmt.Errorf("domains: path prefix %q: target %q is not a normalised hostname", prefix, target) |
| + } |
| + } |
| for attempt := 0; ; attempt++ { |
| cur, etag, err := getClaimETag(ctx, s, host) |
| if errors.Is(err, os.ErrNotExist) { |
| @@ -399,25 +413,6 @@ func SetClaimPaths(ctx context.Context, s release.Store, host, account, app stri |
| if cur.Account != account || cur.App != app { |
| return false, ErrHostUnavailable |
| } |
| - // Input validation happens only after existence and ownership are |
| - // confirmed: a malformed map submitted against a host the caller |
| - // doesn't hold must still surface the neutral ErrHostUnavailable, |
| - // never a validation error that would tell a non-owner their input |
| - // was even looked at. |
| - if len(paths) > MaxPathsPerClaim { |
| - return false, ErrTooManyPaths |
| - } |
| - for prefix, target := range paths { |
| - if !validPathPrefix(prefix) { |
| - return false, ErrBadPathPrefix |
| - } |
| - // The target must be a plausible hostname. Whether it is a route |
| - // this account owns is decided by the reconcile pass, which — |
| - // unlike this door — can see the registry. |
| - if n, ok := NormalizeAliasHost(target); !ok || n != target { |
| - return false, fmt.Errorf("domains: path prefix %q: target %q is not a normalised hostname", prefix, target) |
| - } |
| - } |
| if samePaths(cur.Paths, paths) { |
| return false, nil |
| } |
diff --git a/internal/domains/paths_test.go b/internal/domains/paths_test.go| index 54e46ce9b..4c738a4e4 100644 |
| --- a/internal/domains/paths_test.go |
| +++ b/internal/domains/paths_test.go |
| @@ -109,11 +109,35 @@ func TestSetClaimPathsIsOwnerGated(t *testing.T) { |
| t.Fatalf("err = %v, want ErrHostUnavailable for a non-owner", err) |
| } |
| if _, err := SetClaimPaths(ctx, s, "never-claimed.example", "tito", "classic", |
| - map[string]string{"a": "b"}); !errors.Is(err, ErrHostUnavailable) { |
| + map[string]string{"a": "b.example.com"}); !errors.Is(err, ErrHostUnavailable) { |
| t.Fatalf("err = %v, want the identical ErrHostUnavailable for an absent claim", err) |
| } |
| } |
| +// TestSetClaimPathsRejectsUnnormalisedTarget: a target that is not a |
| +// normalised hostname (no interior dot, wrong case, trailing dot, ...) is |
| +// refused with a validation error distinct from ErrHostUnavailable — this is |
| +// an owned, existing claim, so the failure can only be about the target |
| +// shape, not about who is asking or whether the host exists. |
| +func TestSetClaimPathsRejectsUnnormalisedTarget(t *testing.T) { |
| + ctx := context.Background() |
| + s := release.NewDirStore(t.TempDir()) |
| + if _, _, err := AttachClaim(ctx, s, Claim{ |
| + Schema: 1, Host: "ti.to", Account: "tito", App: "classic", |
| + Route: "ti.to", CreatedAt: "2026-09-03T00:00:00Z", |
| + }); err != nil { |
| + t.Fatal(err) |
| + } |
| + _, err := SetClaimPaths(ctx, s, "ti.to", "tito", "classic", |
| + map[string]string{"paulca": "notahostname"}) |
| + if err == nil { |
| + t.Fatal("err = nil, want a validation error for an unnormalised target") |
| + } |
| + if errors.Is(err, ErrHostUnavailable) { |
| + t.Fatalf("err = %v, want a validation error, not ErrHostUnavailable", err) |
| + } |
| +} |
| + |
| // TestSetClaimPathsUnchangedIsNoWrite: republishing an identical map must not |
| // burn a bucket write. Tito republishes wholesale as a repair, so the no-op |
| // case is the common one. |