From 9dad61f5087e56a708d41340187f7ddac8f9c0f5 Mon Sep 17 00:00:00 2001 From: Lemon-miaow Date: Wed, 23 Sep 2026 21:21:21 +0800 Subject: [PATCH] fix(nano): screen unusable profiles per source instead of stopping the ladder (#55) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A configured Yggdrasil root answering 200 with a name outside the Minecraft charset (or an identity UUID that does not parse) was rejected one layer up in the handler: a silent 204 with no log line, and because the rejection returned instead of continuing, every source behind the broken one was unreachable for that login. The resolver already treats the same class (200 without a usable profile, non-200, unreachable) as skip + log + failed; the name/UUID screens lived above it and silently stopped the ladder instead. Live on the audit box, a single sloppy root produced 204s with no trace anywhere, and [bad root, valid root] answered 204 where the valid root would have admitted the login; nothing else in the nano matrix (60 checks across input validation, canonical rewrite, premium rename, failure modes, failover, log discipline, properties relay) was red. Screen both shapes inside resolveHasJoined, before a 200 can win: identity ids must parse, third-party names must match the charset. A bad answer is logged ('unusable profile name' / 'unparseable profile id'), skipped, and counted as failed — 503 when nothing else validates, and later sources get their turn. The handler's guards stay as the last line before anything leaves (comments updated). Gates: gofmt, go vet, go test ./..., deploy/bootstrap_test.sh all clean. Green live (v0.0.0+fix55): the five bad-name cases and the two failover cases all pass; matrix rerun 60/60. --- internal/api/handlers_hasjoined.go | 32 +++++++++--- internal/api/handlers_hasjoined_test.go | 67 +++++++++++++++++++++---- 2 files changed, 84 insertions(+), 15 deletions(-) diff --git a/internal/api/handlers_hasjoined.go b/internal/api/handlers_hasjoined.go index 762a9be..093e561 100644 --- a/internal/api/handlers_hasjoined.go +++ b/internal/api/handlers_hasjoined.go @@ -142,7 +142,9 @@ func (a *API) handleHasJoined(w http.ResponseWriter, r *http.Request) { // Canonicalize identity. A trusted (Mojang) source keeps its UUID; a self-asserted // source is rewritten into felisAuthNS so it can never land in Mojang's UUID space - // nor onto another source's. An unparseable identity UUID is not trustworthy → reject. + // nor onto another source's. resolveHasJoined has already screened both shapes and + // skipped unusable ones as failed; the two guards below are the last line before + // anything leaves, kept even though nothing reaches them. var canonical uuid.UUID if src.Identity { id, err := uuid.Parse(prof.ID) @@ -154,8 +156,8 @@ func (a *API) handleHasJoined(w http.ResponseWriter, r *http.Request) { } else { // A third-party source is untrusted input, its name included: nothing stops a // hostile or sloppy root from answering with "§4admin", an empty string, or 200 - // characters, all of which this handler would otherwise relay straight into the - // proxy's player list. + // characters, all of which must not be relayed straight into the proxy's player + // list. Screened in resolveHasJoined; this is the last-line guard. if !mcUsernameRe.MatchString(prof.Name) { w.WriteHeader(http.StatusNoContent) return @@ -320,9 +322,10 @@ func lookupPremiumName(ctx context.Context, username string) (bool, error) { // resolveHasJoined queries each configured source in priority order and returns the // first that validates the session (200 with a profile). 204 is "not my player". Any other -// outcome (unreachable, another status, a body that is not a profile) skips the source too, -// but is logged with its tag and reported as failed: otherwise a dead or mistyped source -// looks exactly like a player it does not know, and nobody finds out. +// outcome (unreachable, another status, a body that is not a profile, or a profile this +// multiplexer will not emit) skips the source too, but is logged with its tag and reported +// as failed: otherwise a dead or mistyped source looks exactly like a player it does not +// know, and nobody finds out. func (a *API) resolveHasJoined(ctx context.Context, username, serverID, ip string) (prof *sessionProfile, src AuthSource, failed bool) { for _, src := range a.AuthSources { u := src.URL + "?username=" + url.QueryEscape(username) + "&serverId=" + url.QueryEscape(serverID) @@ -362,6 +365,23 @@ func (a *API) resolveHasJoined(ctx context.Context, username, serverID, ip strin failed = true continue } + // Screen what a 200 is allowed to mean BEFORE it can win. A profile this + // multiplexer will not emit — an identity UUID that does not parse, a third-party + // name outside the Minecraft charset — is the same class as a body that is not a + // profile: skip, log, count as failed. Letting it win would stop the ladder on one + // sloppy root (every source behind it silently unreachable) and read as "nobody + // knows this player" to Velocity while a source had actually answered. + if src.Identity { + if _, perr := uuid.Parse(p.ID); perr != nil { + log.Printf("hasJoined: identity source %q answered 200 with an unparseable profile id %q", src.Tag, p.ID) + failed = true + continue + } + } else if !mcUsernameRe.MatchString(p.Name) { + log.Printf("hasJoined: source %q answered 200 with an unusable profile name %q", src.Tag, p.Name) + failed = true + continue + } return &p, src, failed } return nil, AuthSource{}, failed diff --git a/internal/api/handlers_hasjoined_test.go b/internal/api/handlers_hasjoined_test.go index 8a9a585..aef75a0 100644 --- a/internal/api/handlers_hasjoined_test.go +++ b/internal/api/handlers_hasjoined_test.go @@ -2,12 +2,14 @@ package api import ( "bufio" + "bytes" "context" "encoding/hex" "encoding/json" "errors" "fmt" "io" + "log" "net" "net/http" "net/http/httptest" @@ -337,14 +339,26 @@ func TestHasJoined(t *testing.T) { } }) - // Mojang is trusted for its UUIDs, which is exactly why one that does not parse must not - // be emitted as some default: every such login would share the nil UUID. - t.Run("identity source with an unparseable id -> 204", func(t *testing.T) { + // Mojang is trusted for its UUIDs, which is exactly why one that does not parse must + // not be emitted as some default: every such login would share the nil UUID. The + // unusable 200 is surfaced like every other bad answer — skipped, logged, and 503 when + // nothing else validates — instead of reading as "no such session". + t.Run("identity source with an unparseable id -> skipped and surfaced", func(t *testing.T) { mojang := fakeYgg(t, "not-a-uuid", "Notch") api := newTestAPI(newFakeRepo(), newFakeCluster()) api.AuthSources = []AuthSource{{Tag: "mojang", URL: mojang.URL, Identity: true}} - if w := getHasJoined(api.InternalHandler(), "Notch", "abc"); w.Code != http.StatusNoContent { - t.Fatalf("code = %d, want 204 (%q)", w.Code, w.Body.String()) + + var buf bytes.Buffer + prev := log.Writer() + log.SetOutput(&buf) + w := getHasJoined(api.InternalHandler(), "Notch", "abc") + log.SetOutput(prev) + + if w.Code != http.StatusServiceUnavailable { + t.Fatalf("code = %d, want 503 (%q)", w.Code, w.Body.String()) + } + if !strings.Contains(buf.String(), "unparseable profile id") { + t.Fatalf("the skip was not logged: %q", buf.String()) } }) @@ -559,16 +573,51 @@ func TestHasJoinedPremiumNameRename(t *testing.T) { } }) - // A third-party root is untrusted input, its name field included. - t.Run("hostile upstream name -> 204", func(t *testing.T) { + // A third-party root is untrusted input, its name field included. An unusable name is + // refused AND surfaced: skipped like every other bad answer (logged, counted as + // failed), so it can never be relayed and can never be mistaken for "no such player". + t.Run("hostile upstream name is skipped, logged and surfaced", func(t *testing.T) { stubMojangNames(t) for _, bad := range []string{"§4admin", "not a name", "ab", strings.Repeat("x", 17), ""} { third := fakeYgg(t, notchMojangID, bad) api := newTestAPI(newFakeRepo(), newFakeCluster()) api.AuthSources = []AuthSource{{Tag: "littleskin", Prefix: "LS", URL: third.URL, Identity: false}} - if w := getHasJoined(api.InternalHandler(), "Notch", "abc"); w.Code != http.StatusNoContent { - t.Fatalf("upstream name %q: code = %d, want 204 (%q)", bad, w.Code, w.Body.String()) + + var buf bytes.Buffer + prev := log.Writer() + log.SetOutput(&buf) + w := getHasJoined(api.InternalHandler(), "Notch", "abc") + log.SetOutput(prev) + + if w.Code != http.StatusServiceUnavailable { + t.Fatalf("upstream name %q: code = %d, want 503 (%q)", bad, w.Code, w.Body.String()) + } + if !strings.Contains(buf.String(), "unusable profile name") { + t.Fatalf("upstream name %q: the skip was not logged: %q", bad, buf.String()) } } }) + + // The reason the skip must CONTINUE the ladder rather than stop it: a broken root early + // in the list would otherwise silently block a login its later source would have + // validated (live on the audit box, the bad name produced a silent 204 and the valid + // source was never asked). + t.Run("hostile name cannot block a later source", func(t *testing.T) { + stubMojangNames(t) + bad := fakeYgg(t, notchMojangID, "§4admin") + good := fakeYgg(t, notchMojangID, "Notch") + api := newTestAPI(newFakeRepo(), newFakeCluster()) + api.AuthSources = []AuthSource{ + {Tag: "broken", Prefix: "BR", URL: bad.URL, Identity: false}, + {Tag: "littleskin", Prefix: "LS", URL: good.URL, Identity: false}, + } + w := getHasJoined(api.InternalHandler(), "Notch", "abc") + if w.Code != http.StatusOK { + t.Fatalf("code = %d, want 200 from the second source (%q)", w.Code, w.Body.String()) + } + p := profileOf(t, w) + if want := undashed(uuid.NewMD5(felisAuthNS, []byte("littleskin:"+notchMojangID))); p.ID != want { + t.Fatalf("id = %q, want the second source's canonical %q", p.ID, want) + } + }) }