[parked] Pending whitelist entries: foundation for bulk import

[Pixnop]

This is the foundation piece for #104, bulk whitelist import. It builds the pending entry the import will lean on, and nothing of the import surface itself, which is a separate later PR.

A whitelist entry is matched at the gate by PlayerUid, so an operator has no way to list a player who has never connected: there is no uid to type, and nimctl whitelist add --player only resolves a name against a live session. A pending entry closes that. It is an entry stored with an empty PlayerUid and a set PlayerName. The gate matches it by name, and on the first match it binds to that player's authenticated uid, becoming an ordinary uid-keyed entry from then on. Matching by name at the door is sound because by the time the gate runs the captured name and uid are the authenticated identity from the validated Identification frame, and account names are globally unique, so a name match admits exactly the one authenticated account carrying that name.

Pending is whitelist-only. It lives in a name-keyed store behind WhitelistStore, with its own state file, so the generic RegistryEntryStore shared with the ban list and the existing uid path are left byte for byte unchanged and the ban path gains no pending concept. WhitelistStore.Add routes by shape, a name-only entry to the pending list and a uid entry to the old path. Bind adds the settled uid row before dropping the pending one, so a join landing in the gap between the two is covered by one or the other and never by neither, and the bound form is what persists. Both lists are listed and swept together, and because a pending entry carries an empty uid the uid gate can never match it by accident.

POST /api/whitelist now accepts a name-only body and stores it pending alongside today's uid entry, and a new POST /api/whitelist/bind settles a pending entry onto the uid its name turned out to carry. The bind is idempotent: binding a name that is already bound or was never pending is a no-op that still answers ok, because the proxy fires it best-effort off a join and must never be handed an error for a race it cannot do anything about. The gate binds without blocking the join. On a uid-coverage miss it checks for a pending entry by the authenticated name, admits the player the moment it matches, and fires the bind on a background task, so the join is never waiting on the control plane. A registry that is unreachable at that instant still admits the player and leaves the entry pending for next time, with no exception reaching the pump. Every bind is logged with the name and uid. The transfer gate reads pending the same way, since entering a backend is exactly what a scoped entry is for. nimctl whitelist add for an offline player stores pending instead of failing, a live player still resolves to their uid immediately, and the response says which of the two happened.

Eager name-to-uid resolution, resolving offline names against the account services at import time, is deliberately not here. It waits on confirmation that Anego welcomes the auth-server traffic, and pending entries are the design precisely because they push that resolution to the moment the player arrives and reduce it to almost nothing. This PR references #104 and does not close it.

Tests cover the refusals as hard as the admits: a pending entry admits a matching name and binds, a pending entry for one name does not admit another, a scope covers its own backend and no other, the bind is idempotent and both the pending and the bound forms survive a registry restart, and a bind that fails or throws still admits the player and leaves the entry pending. The existing whitelist and ban suites are green and unchanged except where a test encoded the very behaviour this replaces, a name-only add being refused, which is now a name-only add being stored pending.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
92.4% Coverage on New Code
2.2% Duplication on New Code

See analysis details on SonarQube Cloud

[Pixnop]

Holding this one, and the reason is a flaw in the design I wrote rather than in the implementation: my brief asserted that the name and uid at the gate are "the authenticated identity from the validated Identification frame". That is not true, and the code comments faithfully repeat the claim.

The proxy parses Identification but never validates it. ProxySession.cs:55-59 says so plainly: the mp token inside those bytes is checked with the auth server by the first BACKEND that receives them, which is the whole basis of the #57 single-use-token handling. At the moment CheckLandingWhitelist runs, capturedPlayerName and capturedPlayerUid are values the client asserted and nobody has verified.

The uid path has always had that property and it is tolerable there: passing the gate with a borrowed uid gets you to a backend that then rejects your token, and a uid is not something an attacker typically holds. Matching on NAME changes both halves of that.

Concretely, with a pending entry for Bob on an event backend, an attacker sends Identification carrying the name Bob and their OWN uid and their own valid token. The uid lookup misses, the pending lookup hits on the name, the gate admits, and the proxy mints a reservation for the asserted uid, which is genuinely theirs. The backend validates the token, gets that same real uid, finds the matching reservation, and lets them in. Nothing downstream re-checks the whitelist: CheckForwardingAsync consumes a reservation by player.PlayerUID and checks nothing else. So knowing one name on the list is enough to reach a whitelisted backend, and event lists are public by nature.

The bind makes it worse rather than being the only issue. FirePendingBind writes the asserted uid into Bob's entry, so the attacker also takes the slot permanently and Bob is refused when he actually arrives.

What I think this needs, and I would like @Zaldaryon's read before anyone rebuilds it. The admission decision has to stay where it is, because the gate must choose a backend before any backend sees a byte. So the verification has to come from the side that holds the validated identity: the ServerMod sees the real player.PlayerUID on join and could re-check whitelist coverage there and kick on a miss, exactly the shape ReservationRequired already has. The bind belongs on that side too, keyed on the validated uid, which removes the theft entirely. The cost is new plumbing, since the ServerMod does not consult the whitelist today.

The alternative is to drop name matching altogether and keep only eager name-to-uid resolution at import time, so entries are always uid-keyed and the gate never trusts a name. That is the path waiting on whether Anego welcomes the auth-server traffic, and if that answer is yes it may simply be the better design.

Either way this branch should not land as it stands. The store seam, the bind ordering and the test spread are all sound and survive whichever direction we take.

[Zaldaryon]

The bind flow, idempotency, race safety, scope matching, and gate ordering all check out, traced through PendingWhitelistStore, WhitelistStore, ProxySession, and the endpoint layer. One gap found that I'd want fixed before merge rather than as a follow-up.

Pending whitelist entries (name-only, no uid) have no removal path anywhere. whitelist-remove/POST /api/whitelist/remove both take PlayerUid (WhitelistRemoveRequest has no PlayerName field) and WhitelistStore.Remove only forwards to the uid-keyed RegistryEntryStore, never touching PendingWhitelistStore. The only code path that clears a pending row is WhitelistStore.Bind, triggered by a matching join.

Concretely: an operator runs whitelist-add --player Bob (typo, or a name they later want to revoke before the player ever connects) with no duration, creating a permanent pending entry. There is no way to undo it. It sits there until "Bob" actually joins and binds. For a moderation tool this is a real gap, not just an inconvenience: a mistaken or malicious pending entry can't be revoked before the player who'd benefit from it ever shows up.

Since this PR already ships whitelist-add's offline-pending path (nimctl and POST /api/whitelist both create pending entries now), I'd want at least a name-keyed removal path landing in the same PR, or a clear note that revocation is explicitly out of scope and tracked separately before this merges. Everything else here looks solid.

[Pixnop]

Confirmed, and it is worse than an inconvenience for the reason you name: WhitelistRemoveRequest carries only PlayerUid, WhitelistStore.Remove forwards to the uid-keyed store alone, and Bind is the only thing that clears a pending row. So a permanent pending entry created by a typo, or by someone who should not have, cannot be revoked before the named player shows up to claim it. For a moderation tool that is the wrong shape.

Both of your points and mine now sit upstream of the same question, so I want to settle the direction before anyone writes more code here.

I posted an analysis earlier on this PR that you may have read past, since your review came at it from the code rather than the thread: the name match at the gate is not safe as designed, and that is my error in the brief, not the implementation's. The proxy parses Identification but never validates it, the backend does, so the name the gate matches on is client-asserted. An attacker who knows one name on a pending list asserts it with their own uid and their own valid token, passes the gate, and the backend then validates that same real uid against the reservation the proxy minted and admits them. Event lists are public, so the precondition is free.

Which means there are two live options and your removal gap only matters under one of them.

If pending entries survive, they need verification to move to the ServerMod, which is the only place holding the validated identity, with the bind moving there too and keyed on the real uid. Your name-keyed removal path lands in that same work, and it wants a name-keyed whitelist-remove, a PlayerName on the remove request, and a listing that shows pending rows so an operator can see what they are revoking.

If instead we resolve names to uids eagerly at import time, pending entries disappear entirely. Every entry is uid-keyed from creation, the gate never trusts a name, today's removal path already covers everything, and both problems evaporate. That option depends on whether Anego welcomes bulk traffic to their resolve endpoint, which Tsu is asking about, and the room's read so far (rate-limit it, attribute it clearly) suggests it may well be fine.

My inclination is to wait for that answer rather than build the ServerMod verification path, because if eager resolution is available it is the better design outright and not the fallback I originally framed it as. If the answer comes back no, I will rebuild this with verification and revocation on the backend side and your finding folded in.

Parking the branch rather than merging either way. Nothing in the store seam or the tests is wasted under either outcome.

[Zaldaryon]

Agree with the read and with parking this. The gate cannot validate what it matches on: ProxySession.cs:55-58 documents that the mp token inside Identification is checked by the first backend to receive it, not by the proxy, so name and uid at the gate are client-asserted rather than the validated identity the comments here describe. ProxySession.cs:643-674 goes further and shows the assumption already has teeth in production: an operator can run a backend with auth verification off, which is exactly the condition a matched pending entry needs upstream of it to be exploitable the way you describe.

Two things for the eager-resolution against ServerMod-verification tradeoff that are not in the thread yet. The ServerMod-side check does not need pending entries to be worth building on its own. NimbusServerModSystem.CheckForwardingAsync (line 334) already runs post-join with the validated player.PlayerUID, already kicks through KickForReservation, and already has ReservationRequired and FailClosedWhenRegistryUnreachable as config knobs. A whitelist re-check there is the same shape as the reservation check sitting next to it, and it closes the bug class for the uid path too, not only pending: today a spoofed uid at the gate is already a full bypass on any backend running with auth verification off, since nothing downstream re-checks membership. That part does not wait on Anego at all.

Eager name-to-uid resolution at import is not free either. VS account names can change, so a mapping resolved once at import time can go stale, and the resolver still has to decide what to do with a name that does not resolve or resolves to an account nobody meant to whitelist. Worth weighing against the ServerMod path rather than treating it as the simpler option once Anego answers.

My preference: build the ServerMod re-check as its own PR, independent of pending entries. It holds under either answer from Anego, and it means the pending-entries question only decides whether pending entries exist, not whether whitelist enforcement is sound. Happy to write it if that direction works for you.

[Pixnop]

Both points land, and the first one is worse than I wrote it. A backend running with auth verification off is not a hypothetical operator mistake, it is the workaround we ourselves documented for #57 and told people to use. So the uid path is already a full bypass on those backends today, and my "a borrowed uid dies at the backend's token check" reasoning only holds where that check is on. That makes the ServerMod re-check a fix for a live bug rather than groundwork for a feature.

The name-churn point is the one I had not weighed at all. A uid resolved at import goes stale when the account renames, and that failure is silent: the entry still looks valid and covers nobody. That is a real cost against eager resolution rather than the free win I was treating it as.

So yes, your direction, and please write it. Confirmed the shape is there to slot into: CheckForwardingAsync already kicks through KickForReservation at lines 392 and 406 with ReservationRequired and FailClosedWhenRegistryUnreachable deciding the policy, so a membership re-check sits next to the reservation check rather than inventing a new path. Worth deciding explicitly what an unreachable registry does there, since a whitelist that fails open on an outage and a reservation check that fails closed would be an odd pair on the same backend.

Closing this PR rather than leaving it parked. Pending entries stop being a prerequisite for anything once enforcement is sound on the backend, so they go back to being one option for #104 to be decided when Anego answers, and the branch stays on origin if that answer makes them worth rebuilding.