fix(evr): require authoritative game server for server-profile updates#502
Conversation
There was a problem hiding this comment.
Pull request overview
Tightens EVR server-profile/stat update handling to prevent non-authoritative sessions from injecting player statistics by requiring the sender WS session to match the match’s authoritative game server session (label.GameServer.SessionID).
Changes:
- Enforce an authority check in
processUserServerProfileUpdate(senderSessionID == label.GameServer.SessionID) and emit a warn-level log on rejection. - Thread the sender session ID (and an injectable
NakamaModule) through the async update processing path. - Add SEC-3 regression tests covering both rejection of non-authoritative senders and acceptance of the authoritative broadcaster session.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
server/evr_pipeline_login.go |
Adds authoritative-sender verification for server-profile updates and updates the call signature to include sender session/NK module. |
server/evr_pipeline_sec3_test.go |
Introduces regression tests asserting unauthorized updates do not apply while legitimate broadcaster updates still apply. |
|
|
||
| func (p *EvrPipeline) processUserServerProfileUpdate(ctx context.Context, logger *zap.Logger, evrID evr.EvrId, label *MatchLabel, payload *evr.UpdatePayload) error { | ||
| func (p *EvrPipeline) processUserServerProfileUpdate(ctx context.Context, logger *zap.Logger, nk runtime.NakamaModule, senderSessionID uuid.UUID, evrID evr.EvrId, label *MatchLabel, payload *evr.UpdatePayload) error { | ||
| // SEC-3: A server profile update is only legitimate when it comes from the |
There was a problem hiding this comment.
until we have nevr-runtime going, the server profile updates come over a different connection. you will need to validate that the operatorID of the server is correct, not the sessionID.
There was a problem hiding this comment.
Fixed in 9c55813.
Authority is now the operator, not the session ID. The check compares the sender WS session's authenticated user id (session.UserID()) against label.GameServer.OperatorID — "the user id of the server", set to the registering user in NewGameServerPresence (server/evr_gameserver_presence.go:22,124). This matches the server-host identity used elsewhere (e.g. server/evr_runtime_rpc_match.go:236,310). A nil operator is treated as non-authoritative.
Old check (server/evr_pipeline_login.go:1543):
if label.GameServer == nil || senderSessionID != label.GameServer.SessionID {
New check:
if label.GameServer == nil || label.GameServer.OperatorID.IsNil() || senderOperatorID != label.GameServer.OperatorID {
The caller now passes session.UserID() instead of session.ID(). Warn-log on rejection retained.
SEC-3 tests (server/evr_pipeline_sec3_test.go): the fixture gives the label a GameServer whose SessionID is deliberately distinct from its OperatorID, so a lingering SessionID check would fail. RejectsNonAuthoritativeOperator asserts a mismatched operator gets zero events + a warn; AcceptsAuthoritativeOperator asserts the matching operator gets the update applied over that different connection. gofmt clean, go vet clean, go test -race ./server/ -run SEC3 passes.
|
Independently re-verified the operator-vs-session invariant via static analysis (commit 998a9da, file:line citations in the code comment) — the fix is correct, matches existing OperatorID-based precedent elsewhere in the codebase, and SEC-3 tests are green under One residual scope note for review, not a blocker: this check trusts any connection authenticated as the registering operator's account, not the specific physical server process bound to the match. If one operator runs multiple concurrent game server processes under the same Discord/native account, that operator's other connections could in principle inject stat updates into a sibling server's match. This is not new/widened by this fix — it matches the trust boundary every other OperatorID-based check in this codebase already uses (e.g. Marking ready for review. |
…C-3) userServerProfileUpdateRequest was dispatched with default auth (any authenticated session) and applied a stat update after only checking that the client-supplied EvrID was a player in the referenced match and a group member. It never verified the *sender* was the match's authoritative game server, so any authenticated client could inject server-profile stats for any player in a live non-arena match whose session ID it knew (integrity / IDOR). Require the sender's session to be the match's bound game server (label.GameServer.SessionID) before applying updates, reusing the broadcaster-session identity pattern already used in evr_match.go and set at registration in gameserverRegistrationRequest. Unauthorized senders are rejected with a warn-level log, no crash. processUserServerProfileUpdate now takes the sender session ID and an injected NakamaModule so the authority decision is unit-testable without live Postgres. Adds red/green regression tests. Co-Authored-By: Andrew Bates <a@sprock.io> Signed-off-by: Spritz Metis Sprock <spritz@sprock.io>
…(SEC-3) Pre-nevr-runtime, the server-profile update arrives over a different connection than the game server's registered session, so the sender's session ID never matches label.GameServer.SessionID. The SessionID-based authority check therefore rejected legitimate updates. Check the sender session's authenticated user id against label.GameServer.OperatorID (the user id that registered the server), matching the server-host identity used elsewhere (evr_runtime_rpc_match.go). A nil operator is treated as non-authoritative. Warn-log on rejection is retained. SEC-3 regression tests updated to assert non-authoritative operator is rejected and the authoritative operator is accepted over a distinct session. Co-Authored-By: Andrew Bates <a@sprock.io> Signed-off-by: Spritz Metis Sprock <spritz@sprock.io>
Independently re-verified (via static analysis of the session lifecycle, no live game server available) that the OperatorID-based authority check in processUserServerProfileUpdate is correct and the raw-session-ID assumption it replaced was wrong. Confirmed with concrete, mostly pre-SEC-3, human-authored evidence: - label.GameServer.SessionID is set from the *registration* RPC's own WS session (gameserverRegistrationRequest, evr_pipeline_gameserver.go:331). - UserServerProfileUpdateRequest is documented as arriving over the game server's separate login connection, not that registration session (evr_pipeline.go:574, predates SEC-3, from 7a693b1; and the handler's own doc comment, evr_pipeline_login.go:1478-1479, authored by Andrew in 8f76116, Aug 2024 -- well before this PR). - EVR game servers, like EVR clients, run multiple WS connections that derive identity from one login session rather than a single persistent socket (evr_session.go:21-22, :119 Secondary()/serverSession). - OperatorID is the established server-host identity pattern used elsewhere for the same kind of authority decision (evr_runtime_rpc_match.go:236,310; evr_discord_appbot.go:4744), so this fix matches precedent rather than diverging from it. No behavior change: this only expands the in-code proof comment on the existing (already correct) fix so a future reviewer doesn't have to re-derive the session-lifecycle argument from scratch. SEC-3 regression tests remain green. Co-Authored-By: Andrew Bates <a@sprock.io>
thesprockee
force-pushed
the
sec/profile-update-authz
branch
from
998a9da to
e6ce899
Compare
Summary
userServerProfileUpdateRequestapplied server-profile/stat updates after only checking that the target EvrID was a player in the referenced match — it never verified the sender was that match's authoritative game server. Any authenticated session could submit a crafted update for a player in a live non-arena match whose session ID it knew, injecting server-profile stats without being the game server. (Integrity/IDOR, no crash; arena is already excluded and it is gated byDisableStatisticsUpdates.)Fix
Require the sender session to be the match's authoritative game server before applying updates —
senderSessionID == label.GameServer.SessionID, the same identity the match join/detect paths already compare against. Unauthorized senders are rejected with a warn-level log, no crash.Tests
Red→green: a non-authoritative authenticated session's update is now rejected; the legitimate broadcaster session's update still applies.
gofmt/go vetclean; tests pass under-race.Note for reviewers
The guard assumes the game server sends the update over the same WS session that registered as the broadcaster (
label.GameServer.SessionID). Static evidence supports this. If a server used a separate connection, its updates would be rejected visibly (warn log), not silently — worth confirming against a live game server before relying on it in production.Deployment
None in this PR.
🤖 Generated with Claude Code