amadan
edge: match a prefix on both spellings of the first segment
The path.Clean-only matcher I shipped closed one flip bypass and opened another. With paulca flipped and bob not, "/paulca/../bob" cleans to "bob", misses the map, and falls through to CLASSIC carrying the original path — so a backend that does not collapse ".." (or that routes "/:account/*rest") serves paulca's page from the app paulca was flipped away from. That is the exact direction the comment I wrote claimed could never happen. The trade was arguable; the absolute in the comment was not. Neither spelling is safe alone, and they fail in opposite directions: raw alone misses "//paulca/foo" -> classic answers for a flipped acct cleaned alone misses "/paulca/../bob" -> classic answers for a flipped acct So the map is now consulted for BOTH: the raw split and the path.Clean'd one, accepting a hit from either, cleaned winning when both hit and disagree. A flipped account reaches its new app whatever the backend does with the odd shape. The cost is the benign direction — "/paulca/../bob" reaches the new app carrying "/paulca/../bob", which a normalising target resolves to "/bob" and 404s, an account it was not given. firstSegment goes back to the raw split; firstSegmentClean is the cleaned half. The comment loses the "can never" and gains what each half buys, what it misses, what the pair costs, and the residue measured rather than assumed: "/%2e%2e/bob" matches bob while the upstream gets the escaped path (likely 404, harmless); "/paulca%2ffoo" matches paulca while a strict backend sees one segment (mis-flip toward the NEW app, the safe direction); "." and ".." can never be prefixes because validPathPrefix refuses them. TestFirstSegment pins both columns for every row, including the rows where they disagree. TestPathMatchTriesBothFormsOfTheFirstSegment routes 15 shapes through a live edge and still pins that the upstream receives the original uncleaned bytes. All three neighbouring rules were applied to the source and observed to fail it: clean-only on "/flipped/../notflipped", raw-only on "//flipped", and a reversed tie-break on "/flipped/../onbeta".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 files changed,
+175
−94
internal/edge/edge.go+76 −39internal/edge/path_test.go+99 −55
diff --git a/internal/edge/edge.go b/internal/edge/edge.go| index c29a0f31b..d1ac4885f 100644 |
| --- a/internal/edge/edge.go |
| +++ b/internal/edge/edge.go |
| @@ -980,11 +980,58 @@ func (e *Edge) lookupUpstreamPath(host, urlPath string) (upstream, bool) { |
| if !ok || up.paths == nil { |
| return up, ok |
| } |
| - seg := firstSegment(urlPath) |
| - if seg == "" { |
| - return up, true |
| - } |
| + // BOTH FORMS, cleaned first. The prefix map is consulted for the |
| + // path.Clean'd first segment and then, if that missed and the two differ, |
| + // for the raw one. When both name a prefix, the cleaned one wins. |
| + // |
| + // Neither form alone is safe, and they fail in OPPOSITE directions. The |
| + // invariant being protected is one-way: during a reversible dual-run |
| + // migration, the app an account was flipped AWAY from must never answer |
| + // for that account. Serving a not-yet-flipped account from the NEW app is |
| + // the benign direction — at worst a 404 while the migration runs. |
| + // |
| + // - RAW alone misses "//paulca/foo". Rails and most other backends |
| + // collapse that to "/paulca/foo" and serve it, so the request falls |
| + // through to the legacy app, which then answers for a flipped |
| + // account. One character, silent. |
| + // - CLEANED alone misses "/paulca/../bob", which resolves to "bob". A |
| + // backend that does NOT collapse ".." — or that routes on the first |
| + // segment with a glob like "/:account/*rest" — then serves paulca's |
| + // page from the legacy app. The same bypass, reached the other way. |
| + // |
| + // Taking a hit from either form closes both: "//paulca/foo" hits on the |
| + // cleaned form, "/paulca/../bob" hits on the raw one, and a flipped |
| + // account reaches its new app whatever the backend does with the odd |
| + // shape. The cost is that "/paulca/../bob" reaches the NEW app carrying |
| + // "/paulca/../bob", which a normalising target resolves to "/bob" and |
| + // 404s — an account it was not given. Benign direction, and deliberate. |
| + // |
| + // Residue, measured rather than assumed. None of it crosses the |
| + // invariant above, and it is written down so nobody has to re-derive it: |
| + // |
| + // - "/%2e%2e/bob": r.URL.Path is already percent-DECODED, so this |
| + // cleans to "/bob" and matches "bob". The upstream receives the raw |
| + // "/%2e%2e/bob" and will most likely 404 it. Harmless. |
| + // - "/paulca%2ffoo": decodes to "/paulca/foo", so both forms match |
| + // "paulca" and the request reaches paulca's new app — while a strict |
| + // backend treats the upstream path as the SINGLE segment |
| + // "paulca/foo". A mis-flip toward the NEW app: the safe direction. |
| + // - A prefix literally named "." or ".." cannot exist: validPathPrefix |
| + // (internal/domains/claims.go) refuses both, so the raw form of |
| + // "/../bob" ("..") can never match anything. |
| + // |
| + // Do not collapse this back to a single form. Whichever one you keep, |
| + // you reopen the bypass the other one closes. |
| + // |
| + // No guard is needed for the empty segment ("/", "//", ""): validPathPrefix |
| + // refuses an empty prefix, so no row can be keyed on one and both lookups |
| + // below simply miss, falling through to the host's own route. |
| + seg := firstSegmentClean(urlPath) |
| + raw := firstSegment(urlPath) |
| target, ok := up.paths[seg] |
| + if !ok && raw != seg { |
| + target, ok = up.paths[raw] |
| + } |
| if !ok { |
| return up, true |
| } |
| @@ -1011,57 +1058,47 @@ func (e *Edge) lookupUpstreamPath(host, urlPath string) (upstream, bool) { |
| } |
| // firstSegment returns the first path segment of an absolute URL path, |
| -// without slashes: "/paulca/awesomeconf" -> "paulca". "" for "/", for the |
| -// empty string, and for anything not starting with "/". |
| +// without slashes, splitting the path EXACTLY as it arrived: |
| +// "/paulca/awesomeconf" -> "paulca". "" for "/", for the empty string, and |
| +// for anything not starting with "/". |
| // |
| // Returning the segment rather than testing a string prefix is what makes |
| // 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. |
| +// This is the RAW half of the matcher. See lookupUpstreamPath's match block |
| +// for why both halves exist and why neither is safe alone. |
| func firstSegment(p string) string { |
| if p == "" || p[0] != '/' { |
| return "" |
| } |
| - // 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:] |
| + p = p[1:] |
| if i := strings.IndexByte(p, '/'); i >= 0 { |
| p = p[:i] |
| } |
| return p |
| } |
| +// firstSegmentClean is firstSegment over the path.Clean'd path, so leading |
| +// empty segments and dot segments resolve before the split: |
| +// |
| +// "//paulca/foo" -> "paulca" (leading empty segments collapse) |
| +// "/./paulca" -> "paulca" (dot segment collapses) |
| +// "/paulca/../bob" -> "bob" (".." resolves) |
| +// "/paulca/.." -> "" (resolves to "/", the host's own root) |
| +// "/paulca/" -> "paulca" (a trailing slash is not a second segment) |
| +// |
| +// This is the CLEANED half of the matcher; the raw half is firstSegment. |
| +func firstSegmentClean(p string) string { |
| + if p == "" || p[0] != '/' { |
| + return "" |
| + } |
| + // 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. |
| + return firstSegment(path.Clean(p)) |
| +} |
| + |
| func (e *Edge) ServeHTTP(w http.ResponseWriter, r *http.Request) { |
| host := r.Host |
| if h, _, err := net.SplitHostPort(host); err == nil { |
diff --git a/internal/edge/path_test.go b/internal/edge/path_test.go| index d29bd5265..b6c48e3ba 100644 |
| --- a/internal/edge/path_test.go |
| +++ b/internal/edge/path_test.go |
| @@ -165,32 +165,45 @@ 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) { |
| +// TestPathMatchTriesBothFormsOfTheFirstSegment is the flip-bypass pin, and it |
| +// pins BOTH directions of it, because the matcher's two halves fail opposite |
| +// ways and each one alone reopens what the other closes. |
| +// |
| +// The invariant: during a reversible dual-run migration, the app an account |
| +// was flipped AWAY from must never answer for that account. |
| +// |
| +// - Raw-split alone yields "" for "//flipped/foo", so the request falls |
| +// through to classic — which most backends then serve as |
| +// "/flipped/foo". One character, silent. (The original I2 finding.) |
| +// - Clean-first alone yields "notflipped" for "/flipped/../notflipped", so |
| +// THAT falls through to classic — and a backend which does not collapse |
| +// ".." (or which routes "/:account/*rest") serves flipped's page from |
| +// classic. Same bypass, reached from the other side. |
| +// |
| +// Consulting both closes both. The cost is the benign direction: a |
| +// normalising target gets asked for an account it was not given and 404s. |
| +func TestPathMatchTriesBothFormsOfTheFirstSegment(t *testing.T) { |
| classic, _ := hostEchoBackend(t, "classic") |
| - seen := make(chan string, 8) |
| + seen := make(chan string, 16) |
| golive := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| seen <- r.URL.Path |
| w.Write([]byte("go")) |
| })) |
| t.Cleanup(golive.Close) |
| + beta, _ := hostEchoBackend(t, "beta") |
| 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"}, |
| + {Host: "beta.tito.io", Kind: "service", Addr: beta.Listener.Addr().String(), Status: "ready"}, |
| } { |
| if err := reg.Upsert(rt); err != nil { |
| t.Fatal(err) |
| } |
| } |
| upsertPath(t, reg, "ti.to", "flipped", "go.tito.io") |
| + upsertPath(t, reg, "ti.to", "onbeta", "beta.tito.io") |
| e, err := New(reg, nil) |
| if err != nil { |
| @@ -199,32 +212,51 @@ func TestPathMatchNormalisesBeforeMatching(t *testing.T) { |
| 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"}, |
| + for _, tc := range []struct{ path, want, why string }{ |
| + // Only the CLEANED form finds the account. |
| + {"//flipped", "go", "leading empty segment"}, |
| + {"//flipped/event", "go", "leading empty segment"}, |
| + {"///flipped/event", "go", "leading empty segments"}, |
| + {"/./flipped", "go", "dot segment"}, |
| + // Only the RAW form finds it. These are the rows that would go to |
| + // classic under a clean-only matcher — the regression this test |
| + // exists to prevent. |
| + {"/flipped/../notflipped", "go", "raw first segment is still flipped"}, |
| + {"/flipped/..", "go", "raw first segment is still flipped"}, |
| + {"/flipped/../", "go", "raw first segment is still flipped"}, |
| + // BOTH forms hit, and they name different prefixes: the CLEANED one |
| + // wins, because that is the account a normalising backend will serve. |
| + {"/flipped/../onbeta", "beta", "cleaned wins when both hit"}, |
| + // Neither form names a flipped account: the origin still answers. |
| + {"/notflipped", "classic", "no prefix matches"}, |
| + {"/notflipped/../alsonot", "classic", "no prefix matches either way"}, |
| + {"/", "classic", "root"}, |
| + {"//", "classic", "root, however spelled"}, |
| + // Segment-bounded as ever; normalisation must not have loosened it. |
| + {"/flippedxyz", "classic", "segment equality, never HasPrefix"}, |
| + {"//flippedxyz/event", "classic", "segment equality after cleaning too"}, |
| } { |
| 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) |
| + t.Fatalf("%s: body = %q, want %q (%s)", tc.path, body, tc.want, tc.why) |
| } |
| } |
| // 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) |
| + for _, want := range []string{"//flipped", "//flipped/event", "///flipped/event", "/./flipped"} { |
| + if got := <-seen; got != want { |
| + t.Fatalf("upstream saw %q, want the original uncleaned path %q", got, want) |
| + } |
| + } |
| + for _, want := range []string{"/flipped/../notflipped", "/flipped/..", "/flipped/../"} { |
| + if got := <-seen; got != want { |
| + t.Fatalf("upstream saw %q, want the original uncleaned path %q", got, want) |
| + } |
| } |
| } |
| @@ -368,40 +400,52 @@ func TestPathRoutedRequestMetersAgainstTheTarget(t *testing.T) { |
| } |
| } |
| -// TestFirstSegment is the matcher itself, tested directly because every |
| -// routing decision in this feature rests on it. |
| +// TestFirstSegment is the matcher's two halves, tested directly because every |
| +// routing decision in this feature rests on them. Both columns are pinned for |
| +// every row, INCLUDING the rows where they disagree — the disagreements are |
| +// the whole reason lookupUpstreamPath consults both (see its match block). |
| func TestFirstSegment(t *testing.T) { |
| - for _, tc := range []struct{ in, want string }{ |
| - {"/paulca", "paulca"}, |
| - {"/paulca/", "paulca"}, |
| - {"/paulca/awesomeconf", "paulca"}, |
| - {"/paulca/awesomeconf/admin", "paulca"}, |
| - {"/", ""}, |
| - {"", ""}, |
| - {"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"}, |
| + for _, tc := range []struct{ in, wantRaw, wantClean string }{ |
| + // Ordinary shapes: the two halves agree, and this is every real |
| + // request on the platform. |
| + {"/paulca", "paulca", "paulca"}, |
| + {"/paulca/", "paulca", "paulca"}, |
| + {"/paulca/awesomeconf", "paulca", "paulca"}, |
| + {"/paulca/awesomeconf/admin", "paulca", "paulca"}, |
| + {"/paulca/awesomeconf/", "paulca", "paulca"}, |
| + {"/paulcarson", "paulcarson", "paulcarson"}, // segment equality, never HasPrefix |
| + {"/", "", ""}, |
| + {"", "", ""}, |
| + {"paulca", "", ""}, // not an absolute path; never matches |
| + {"//", "", ""}, |
| + |
| + // Leading empty segments and dot segments: only the CLEANED half |
| + // finds the account. Rails and friends serve //paulca/foo as |
| + // /paulca/foo, so without this half a flipped account falls through |
| + // to the app it was flipped away from — the original I2 bypass. |
| + {"//paulca", "", "paulca"}, |
| + {"//paulca/foo", "", "paulca"}, |
| + {"///paulca/foo", "", "paulca"}, |
| + {"/./paulca", ".", "paulca"}, |
| + {"/paulca/./awesomeconf", "paulca", "paulca"}, |
| + {"/paulca//awesomeconf", "paulca", "paulca"}, |
| + |
| + // ".." shapes: the two halves DISAGREE, and only the RAW half still |
| + // names paulca. A backend that does not collapse ".." would serve |
| + // paulca's page, so dropping the raw half here reopens the same |
| + // bypass from the other side. Consulting both is what closes it. |
| + {"/paulca/../bob", "paulca", "bob"}, |
| + {"/paulca/..", "paulca", ""}, |
| + {"/paulca/../", "paulca", ""}, |
| + {"/../bob", "..", "bob"}, // ".." can never be a prefix (validPathPrefix) |
| + {"/..", "..", ""}, |
| + {"/.", ".", ""}, |
| } { |
| - if got := firstSegment(tc.in); got != tc.want { |
| - t.Fatalf("firstSegment(%q) = %q, want %q", tc.in, got, tc.want) |
| + if got := firstSegment(tc.in); got != tc.wantRaw { |
| + t.Fatalf("firstSegment(%q) = %q, want %q", tc.in, got, tc.wantRaw) |
| + } |
| + if got := firstSegmentClean(tc.in); got != tc.wantClean { |
| + t.Fatalf("firstSegmentClean(%q) = %q, want %q", tc.in, got, tc.wantClean) |
| } |
| } |
| } |