fix(nano): stop trusting an expired free name while mojang is failing
When the premium-name lookup failed, isPremiumName fell back to any cached answer, however old. An expired "free" is exactly the answer that may have stopped being true: someone can buy the name after it was last seen free. For as long as api.mojang.com kept failing (429, 5xx, a timeout), a third-party player holding that name kept it on every reconnect, and the Velocity registry, keyed on the name, turned its new owner away as already connected. A hostile source could drive the host into Mojang's rate limit on purpose to hold names that way. A failed lookup now always counts as taken, so the player is renamed with the source's prefix. An expired "taken" already gave that answer, so only the stale "free" case changes. The cost is cosmetic: during an outage an ordinary third-party player may get a prefix they do not need, and their data follows the UUID, not the name. A new test gives the cache a free entry past its TTL and has Mojang answer 429. It fails on the old fallback. The two comments that described the fallback now describe the fail-closed rule.
This commit is contained in:
2 files changed
+16
-9
No files matched your search
@@ -230,7 +230,7 @@ var mojangProfileAPI = "https://api.mojang.com/users/profiles/minecraft/"
|
|||||||
// profileHTTPClient is deliberately more impatient than authHTTPClient: the name lookup is a
|
// profileHTTPClient is deliberately more impatient than authHTTPClient: the name lookup is a
|
||||||
// SECOND Mojang round-trip on a third-party login (the identity leg already spent one), and
|
// SECOND Mojang round-trip on a third-party login (the identity leg already spent one), and
|
||||||
// api.mojang.com is exactly what is unreliable from the networks these servers sit on. A
|
// api.mojang.com is exactly what is unreliable from the networks these servers sit on. A
|
||||||
// slow answer falls back to the cache instead of holding the login open.
|
// slow answer counts as taken instead of holding the login open.
|
||||||
var profileHTTPClient = &http.Client{Timeout: 2 * time.Second, Transport: upstreamTransport}
|
var profileHTTPClient = &http.Client{Timeout: 2 * time.Second, Transport: upstreamTransport}
|
||||||
|
|
||||||
// A name's premium status changes on human timescales, not per login, so it is cached — but
|
// A name's premium status changes on human timescales, not per login, so it is cached — but
|
||||||
@@ -256,11 +256,11 @@ var premiumNames = struct {
|
|||||||
}{m: make(map[string]premiumEntry)}
|
}{m: make(map[string]premiumEntry)}
|
||||||
|
|
||||||
// isPremiumName reports whether username belongs to a real Mojang account — which is what
|
// isPremiumName reports whether username belongs to a real Mojang account — which is what
|
||||||
// makes a third-party player holding it a squatter. On a lookup failure it prefers a stale
|
// makes a third-party player holding it a squatter. A lookup failure fails CLOSED (assume
|
||||||
// cached answer, and with nothing cached it fails CLOSED (assume premium → rename the
|
// premium → rename the third-party player), even over an expired "free": the name may have
|
||||||
// third-party player): a Mojang outage must not let a squatter keep a name the real owner is
|
// been bought since, and a Mojang outage must not let a squatter keep it. Being wrong that
|
||||||
// about to log in with. Being wrong that way costs a cosmetic prefix; being wrong the other
|
// way costs a cosmetic prefix; being wrong the other way bounces the name's actual owner off
|
||||||
// way bounces the name's actual owner off the proxy.
|
// the proxy.
|
||||||
func isPremiumName(ctx context.Context, username string) bool {
|
func isPremiumName(ctx context.Context, username string) bool {
|
||||||
key := strings.ToLower(username)
|
key := strings.ToLower(username)
|
||||||
|
|
||||||
@@ -273,9 +273,6 @@ func isPremiumName(ctx context.Context, username string) bool {
|
|||||||
|
|
||||||
taken, err := lookupPremiumName(ctx, username)
|
taken, err := lookupPremiumName(ctx, username)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
if hit {
|
|
||||||
return cached.taken
|
|
||||||
}
|
|
||||||
return true
|
return true
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -478,6 +478,16 @@ func TestPremiumNameCache(t *testing.T) {
|
|||||||
}
|
}
|
||||||
})
|
})
|
||||||
|
|
||||||
|
// The name may have been bought since it was last seen free, and its new owner is who a
|
||||||
|
// stale "free" would lock out for as long as the lookups keep failing.
|
||||||
|
t.Run("an expired free answer does not outlive an outage", func(t *testing.T) {
|
||||||
|
profileAPIAnswering(t, http.StatusTooManyRequests)
|
||||||
|
seedPremium("Steve0", false, premiumFreeTTL+time.Minute)
|
||||||
|
if !isPremiumName(ctx, "Steve0") {
|
||||||
|
t.Fatal("stale free answer trusted during an outage; a squatter keeps a just-bought name")
|
||||||
|
}
|
||||||
|
})
|
||||||
|
|
||||||
t.Run("the cache is cleared rather than grown past its bound", func(t *testing.T) {
|
t.Run("the cache is cleared rather than grown past its bound", func(t *testing.T) {
|
||||||
profileAPIAnswering(t, http.StatusNotFound)
|
profileAPIAnswering(t, http.StatusNotFound)
|
||||||
for i := range premiumCacheMax {
|
for i := range premiumCacheMax {
|
||||||
|
|||||||
Reference in new issue
Block a user