amadan
edge: match a prefix against the normalised path, and name what a hit attributes
firstSegment("//paulca/foo") returned "", so the request fell through to the
un-migrated backend — while Rails and most other backends normalise it to
/paulca/foo and serve it. During a reversible dual-run migration that is a
one-character way to make the pre-migration app answer for a flipped account,
silently. TestFirstSegment pinned {"//", ""}, which read as though the case
had been considered and settled.
The rule is now: match the segment the BACKEND will serve. path.Clean, then
split. Every shape is pinned with a test row and spelled out in firstSegment's
doc comment so nobody later "fixes" it in the opposite direction:
//paulca/foo -> paulca (leading empty segments collapse)
/./paulca -> paulca
/paulca/../bob -> bob (the backend serves bob's page)
/paulca/.. -> none (resolves to the host's own root)
/paulca/ -> paulca
/paulcarson -> paulcarson (segment equality, never HasPrefix)
Cleaning only ever moves a request toward the account the backend will serve;
the dangerous direction — a flipped account served by the app it was flipped
away from — is the one it closes. One residue, documented: r.URL.Path is
already percent-decoded, so an escaped dot-dot cleans away and the target
receives the raw escaped path and will most likely 404 it. A 404 on a
pathological URL, not a flip bypass.
MATCHING normalises; PROXYING does not. The upstream still receives the
original uncleaned path, pinned in the same test.
Also on lookupUpstreamPath, the attribution paragraph: a path hit returns the
TARGET's upstream entry wholesale, so metering bills the target's route, the
version header names the target's release, purge tenancy is the target's, and
the activator wakes the target. Three are correct. The fourth is a gap — the
cache keys on the REQUESTED host while purgeModifier is built from the
target's route, so a path-routed app's Carlos-Purge drops nothing and the
Carlos-Purge-Host escape hatch is refused for crossing an app boundary, which
is the one thing path routing exists to do. Not fixed here: the purge tenancy
model is not this feature's to redesign. Bounded and recorded in spec §11.
TestPathRoutedRequestMetersAgainstTheTarget pins the metering half so a later
"fix" in the wrong direction is caught. Three smaller doc corrections while in
here: lookupIn is the HOST half (ServeHTTP calls lookupUpstreamPath); the
target is resolved exactly while the host half also accepts a wildcard key,
and why that asymmetry is safe; and one hop only, no chaining, do not add
recursion.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 files changed,
+228
−5
internal/edge/edge.go+93 −5internal/edge/path_test.go+135 −0
diff --git a/internal/edge/edge.go b/internal/edge/edge.go| index 6ee146919..c29a0f31b 100644 |
| --- a/internal/edge/edge.go |
| +++ b/internal/edge/edge.go |
| @@ -36,6 +36,7 @@ import ( |
| "net/textproto" |
| "os" |
| "os/signal" |
| + "path" |
| "path/filepath" |
| "slices" |
| "strconv" |
| @@ -889,8 +890,10 @@ func (e *Edge) deploymentHost(host string) bool { |
| } |
| // lookupIn resolves host to its upstream entry within a given map — the |
| -// request-routing lookup ServeHTTP uses (NOT Allowed's autocert HostPolicy — |
| -// see the comment there). An exact match (a real route, or a plain #151 |
| +// HOST half of the request-routing lookup (NOT Allowed's autocert HostPolicy |
| +// — see the comment there). ServeHTTP does not call it directly: it calls |
| +// lookupUpstreamPath, which wraps this and then resolves a leading path |
| +// segment for the hostnames that carry a prefix map. An exact match (a real route, or a plain #151 |
| // alias landed in the same map) always wins; on a miss, a one-label wildcard |
| // key folds in: "*.<parent>" is the shape Task 3's reconcile pass derives |
| // from a claim's catch-all row, and it lands in the SAME map as every other |
| @@ -934,13 +937,50 @@ func (e *Edge) lookupUpstream(host string) (upstream, bool) { |
| // Every miss falls through to the hostname's own route rather than failing: |
| // an unlisted prefix, a dangling target, a request for "/". A prefix map is a |
| // migration in progress, and the un-migrated answer is always the safe one. |
| -func (e *Edge) lookupUpstreamPath(host, path string) (upstream, bool) { |
| +// |
| +// ATTRIBUTION: a path hit returns the TARGET's upstream entry wholesale, so |
| +// everything downstream that reads up.route reads the target's row, not the |
| +// requested hostname's. Four consequences, named here because this is where a |
| +// reader will look for them: |
| +// |
| +// 1. Usage metering bills the TARGET's route, account and app (the |
| +// UsageMeter wrap in ServeHTTP keys on up.route.Host). Correct: the |
| +// target's backend did the work and burned the bytes. Pinned by |
| +// TestPathRoutedRequestMetersAgainstTheTarget — a later "fix" that |
| +// attributes to the requested hostname would double-count nothing and |
| +// bill the wrong app. |
| +// 2. The X-Carlos-Version response header reports the TARGET's release. |
| +// Correct: it names the code that produced the response. |
| +// 3. Purge tenancy is the TARGET's app and account (purgeModifier is built |
| +// from up.route). Correct as tenancy — but it is also a real GAP, see |
| +// below. |
| +// 4. Activator keying and hibernation are the TARGET's (up.hibernating and |
| +// up.route come from the same entry), so a request for a flipped prefix |
| +// wakes the target instance, not the requested hostname's. Correct: the |
| +// target is the process that has to be awake to answer. |
| +// |
| +// The gap in (3): the response CACHE keys on the REQUESTED host |
| +// (CacheKey{Host: host} in ServeHTTP), while purgeModifier resolves the purge |
| +// host from the target's route. So a path-routed app emitting Carlos-Purge |
| +// purges under go.tito.io while its entry is stored under ti.to, and nothing |
| +// is dropped; the documented Carlos-Purge-Host: ti.to escape hatch does not |
| +// help either, because resolvePurgeHost requires the override to name a host |
| +// in the SAME app — and path routing exists precisely to cross an app |
| +// boundary. Aliases never hit this: an alias key maps to the primary's own |
| +// route, so the app comparison passes trivially. |
| +// |
| +// The bound on that gap: a path-routed response can go stale for at most the |
| +// cache's max TTL, and only for responses that opted into caching with a |
| +// positive max-age. Uncached responses are unaffected. Widening the purge |
| +// tenancy model to follow a prefix map is a design change this feature did |
| +// not make; it is recorded as a known gap in design spec §11. |
| +func (e *Edge) lookupUpstreamPath(host, urlPath string) (upstream, bool) { |
| m := e.current() |
| up, ok := lookupIn(m, host) |
| if !ok || up.paths == nil { |
| return up, ok |
| } |
| - seg := firstSegment(path) |
| + seg := firstSegment(urlPath) |
| if seg == "" { |
| return up, true |
| } |
| @@ -948,10 +988,25 @@ func (e *Edge) lookupUpstreamPath(host, path string) (upstream, bool) { |
| if !ok { |
| return up, true |
| } |
| + // m[target], not lookupIn(m, target): the TARGET is resolved exactly, |
| + // while the host half above also accepts a one-label wildcard key. The |
| + // asymmetry is deliberate and it is documented rather than removed — |
| + // route_paths targets are hostnames the reconcile pass verified against a |
| + // real local route (aliases.go), and a real route is always an exact key, |
| + // so a target reachable ONLY through "*.parent" cannot be produced by any |
| + // supported writer. If one ever were, it would miss here and fall through |
| + // to the host's own route, which is this function's safe direction |
| + // anyway. |
| tu, ok := m[target] |
| if !ok { |
| return up, true |
| } |
| + // ONE HOP, deliberately: if tu itself carries a prefix map, it is NOT |
| + // re-resolved against this request's path. A prefix selects a route, and |
| + // the selected route's own map belongs to ITS hostname's traffic, not to |
| + // this one's. Do not add recursion here — chaining buys nothing the |
| + // publisher cannot express by pointing the prefix at the final target |
| + // directly, and it would introduce a cycle to detect. |
| return tu, true |
| } |
| @@ -963,11 +1018,44 @@ func (e *Edge) lookupUpstreamPath(host, path string) (upstream, bool) { |
| // matching segment-bounded: "/paulcarson" yields "paulcarson", which is not |
| // equal to "paulca", so a longer account name can never be dragged across a |
| // migration by a shorter one. |
| +// |
| +// The path is path.Clean'd FIRST, and that is a deliberate choice with a |
| +// direction — do not "simplify" it back to a raw split. The rule is: match |
| +// the segment the BACKEND will serve, because that is the account whose |
| +// traffic the flip is about. Rails, and every other server that normalises, |
| +// serves "//paulca/foo" as "/paulca/foo"; a raw split yields "" there and |
| +// falls the request through to the UN-MIGRATED app, which then answers for a |
| +// migrated account. During a reversible dual-run migration that is a |
| +// one-character way to bypass a flip, silently. The shapes, and where each |
| +// one routes: |
| +// |
| +// "//paulca/foo" -> "paulca" (leading empty segments collapse) |
| +// "/./paulca" -> "paulca" (dot segment collapses) |
| +// "/paulca/../bob" -> "bob" (the backend serves bob's page, so bob |
| +// is the account this request is about) |
| +// "/paulca/.." -> "" (resolves to "/", the host's own root) |
| +// "/paulca/" -> "paulca" (trailing slash is not a second segment) |
| +// "/paulcarson" -> "paulcarson" (segment equality, never HasPrefix) |
| +// "paulca" -> "" (not an absolute path; matches nothing) |
| +// |
| +// Only the MATCHING decision uses the cleaned form. The upstream still |
| +// receives the request's original, uncleaned path — no prefix stripping and |
| +// no rewriting, per design spec §6, so the flipped and unflipped URL spaces |
| +// stay byte-identical. |
| +// |
| +// One residue, accepted: r.URL.Path is already percent-DECODED, so |
| +// "/%2e%2e/bob" cleans to "/bob" and routes to bob's route, which then |
| +// receives the raw escaped path and will most likely 404 it. That is a 404 |
| +// on a pathological URL, not a flip bypass — the direction that matters here |
| +// is that a flipped account can never be served by the app it was flipped |
| +// away from. |
| func firstSegment(p string) string { |
| if p == "" || p[0] != '/' { |
| return "" |
| } |
| - p = p[1:] |
| + // Clean of a rooted path is rooted, and is "/" when everything resolves |
| + // away — so the slice below is always safe and yields "" for the root. |
| + p = path.Clean(p)[1:] |
| if i := strings.IndexByte(p, '/'); i >= 0 { |
| p = p[:i] |
| } |
diff --git a/internal/edge/path_test.go b/internal/edge/path_test.go| index 8c3ff8ae9..d29bd5265 100644 |
| --- a/internal/edge/path_test.go |
| +++ b/internal/edge/path_test.go |
| @@ -165,6 +165,69 @@ func TestPathRouteDoesNotStripThePrefix(t *testing.T) { |
| } |
| } |
| +// TestPathMatchNormalisesBeforeMatching is the flip-bypass pin. Rails (and |
| +// most backends) serve "//paulca/foo" as "/paulca/foo", so a matcher that |
| +// splits the raw path yields "" there and falls the request through to the |
| +// app the account was flipped AWAY from — a one-character way to make the |
| +// pre-migration app answer for a migrated account, silently, during a |
| +// reversible dual-run. The match is made on the path.Clean'd form; the |
| +// upstream still receives the original bytes (pinned below). |
| +func TestPathMatchNormalisesBeforeMatching(t *testing.T) { |
| + classic, _ := hostEchoBackend(t, "classic") |
| + seen := make(chan string, 8) |
| + golive := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| + seen <- r.URL.Path |
| + w.Write([]byte("go")) |
| + })) |
| + t.Cleanup(golive.Close) |
| + |
| + reg := openReg(t) |
| + for _, rt := range []registry.Route{ |
| + {Host: "ti.to", Kind: "service", Addr: classic.Listener.Addr().String(), Status: "ready"}, |
| + {Host: "go.tito.io", Kind: "service", Addr: golive.Listener.Addr().String(), Status: "ready"}, |
| + } { |
| + if err := reg.Upsert(rt); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + upsertPath(t, reg, "ti.to", "flipped", "go.tito.io") |
| + |
| + e, err := New(reg, nil) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + srv := httptest.NewServer(e) |
| + t.Cleanup(srv.Close) |
| + |
| + for _, tc := range []struct{ path, want string }{ |
| + {"//flipped", "go"}, |
| + {"//flipped/event", "go"}, |
| + {"///flipped/event", "go"}, |
| + {"/./flipped", "go"}, |
| + // Resolves to another account's subtree, so it is that account's |
| + // request — and that account is not flipped. |
| + {"/flipped/../notflipped", "classic"}, |
| + // Resolves to the root, which is the host's own route. |
| + {"/flipped/..", "classic"}, |
| + } { |
| + code, body := getHostPath(t, srv.URL, "ti.to", tc.path) |
| + if code != http.StatusOK { |
| + t.Fatalf("%s: status = %d, want 200", tc.path, code) |
| + } |
| + if body != tc.want { |
| + t.Fatalf("%s: body = %q, want %q", tc.path, body, tc.want) |
| + } |
| + } |
| + |
| + // Matching normalises; PROXYING does not. The upstream must still receive |
| + // the request's original, uncleaned path — no stripping, no rewriting, so |
| + // the flipped and unflipped URL spaces stay byte-identical (spec §6). |
| + got := <-seen |
| + if got != "//flipped" { |
| + t.Fatalf("upstream saw %q for //flipped, want the original uncleaned path", got) |
| + } |
| +} |
| + |
| // TestPathPreservesInboundHost: the upstream sees the hostname the browser |
| // asked for, matching the alias contract. Tito Go must be able to build |
| // absolute URLs that keep the visitor on ti.to. |
| @@ -252,6 +315,59 @@ func TestHostsWithoutPathsAreUnaffected(t *testing.T) { |
| } |
| } |
| +// TestPathRoutedRequestMetersAgainstTheTarget pins the attribution ruling |
| +// documented on lookupUpstreamPath: a path hit returns the TARGET's upstream |
| +// entry wholesale, so the usage meter bills the target's route, account and |
| +// app — not the hostname the browser asked for. That is correct (the target's |
| +// backend burned the bytes) and it is easy to "fix" the wrong way later, so |
| +// it is pinned here. The same entry is what makes the version header, the |
| +// purge tenancy and the activator keying the target's; see that doc comment. |
| +func TestPathRoutedRequestMetersAgainstTheTarget(t *testing.T) { |
| + body := "go" |
| + classic, _ := hostEchoBackend(t, "classic") |
| + golive, _ := hostEchoBackend(t, body) |
| + reg := openReg(t) |
| + for _, rt := range []registry.Route{ |
| + {Host: "ti.to", Kind: "service", Addr: classic.Listener.Addr().String(), |
| + Status: "ready", Account: "tito", App: "classic"}, |
| + {Host: "go.tito.io", Kind: "service", Addr: golive.Listener.Addr().String(), |
| + Status: "ready", Account: "tito", App: "titogo"}, |
| + } { |
| + if err := reg.Upsert(rt); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + upsertPath(t, reg, "ti.to", "flipped", "go.tito.io") |
| + |
| + e, err := New(reg, nil) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + meter := NewUsageMeter() |
| + e.EnableUsage(meter) |
| + srv := httptest.NewServer(e) |
| + t.Cleanup(srv.Close) |
| + |
| + if code, got := getHostPath(t, srv.URL, "ti.to", "/flipped/event"); code != http.StatusOK || got != body { |
| + t.Fatalf("flipped request = %d %q, want 200 %q", code, got, body) |
| + } |
| + |
| + rows := meter.Drain() |
| + row, ok := rows["go.tito.io"] |
| + if !ok { |
| + t.Fatalf("usage rows = %+v, want the TARGET route metered", rows) |
| + } |
| + if row.Requests != 1 || row.BytesOut != int64(len(body)) { |
| + t.Fatalf("go.tito.io row = %+v, want Requests=1 BytesOut=%d", row, len(body)) |
| + } |
| + if row.App != "titogo" { |
| + t.Fatalf("go.tito.io row = %+v, want the TARGET's app, not the requested hostname's", row) |
| + } |
| + if _, ok := rows["ti.to"]; ok { |
| + t.Fatalf("usage rows = %+v, want nothing billed to the requested hostname for a path-routed request", rows) |
| + } |
| +} |
| + |
| // TestFirstSegment is the matcher itself, tested directly because every |
| // routing decision in this feature rests on it. |
| func TestFirstSegment(t *testing.T) { |
| @@ -264,6 +380,25 @@ func TestFirstSegment(t *testing.T) { |
| {"", ""}, |
| {"paulca", ""}, // not an absolute path; never matches |
| {"//", ""}, |
| + // Normalised shapes. The rule is "match the segment the BACKEND will |
| + // serve": Rails and friends serve //paulca/foo as /paulca/foo, so a |
| + // raw split would fall a flipped account through to the app it was |
| + // flipped away from — a one-character flip bypass. Each row here is |
| + // a decision, not an observation; see firstSegment's doc comment |
| + // before changing one. |
| + {"//paulca", "paulca"}, |
| + {"//paulca/foo", "paulca"}, |
| + {"///paulca/foo", "paulca"}, |
| + {"/./paulca", "paulca"}, |
| + {"/paulca/./awesomeconf", "paulca"}, |
| + {"/paulca/../bob", "bob"}, // the backend serves bob's page |
| + {"/paulca/..", ""}, // resolves to the host's own root |
| + {"/paulca/../", ""}, // |
| + {"/../bob", "bob"}, // above the root is still the root |
| + {"/..", ""}, // |
| + {"/.", ""}, // |
| + {"/paulca//awesomeconf", "paulca"}, |
| + {"/paulca/awesomeconf/", "paulca"}, |
| } { |
| if got := firstSegment(tc.in); got != tc.want { |
| t.Fatalf("firstSegment(%q) = %q, want %q", tc.in, got, tc.want) |