Skip to content

fix(sql): keep DB connection, userid in spoof check, retry connects - #53

Open
Rushaway wants to merge 1 commit into
masterfrom
fix/sql-audit
Open

Rushaway wants to merge 1 commit into
masterfrom
fix/sql-audit

Conversation

@Rushaway

Copy link
Copy Markdown
Member

Summary

  • Wrong player could be kicked: the spoof-check DataPack carried the raw client index; when the async SELECT came back after the player left, KickClient(client) could hit whoever took the slot. The pack now carries GetClientUserId(client) and Query_SelectConnection resolves it with GetClientOfUserId (+ 0 check) (PlayerManager.sp).
  • Spoof check skipped every map change: OnMapEnd deleted the connection and OnConfigsExecuted reconnected, so every player authorizing before the new connection landed hit the !g_hDatabase early return and was never checked. The connection is now kept across maps (SourceMod keeps it; the MySQL driver auto-reconnects), and once a connection is ready, DB_CheckPendingClients re-runs the check for connected, authorized players that missed it (tracked with g_bConnectionChecked, reset in OnClientConnected). This also covers late-load players, who were checked before the DB connected.
  • Failed connect never retried: GotDatabase returned on failure with the state stuck at Connecting, so DB_Connect never tried again. It now schedules a retry (DB_ScheduleReconnect, same 15 s delay as a lost connection). The retry timer no longer uses TIMER_FLAG_NO_MAPCHANGE: a map change killing it would leave the state stuck in Wait.
  • Any SQL error dropped the connection: DB_Conn_Lost deleted the handle on any results == null. It now only does so when the error text is a real connection error ("Lost connection", "gone away", "Can't connect", "Broken pipe").
  • Charset: db.SetCharset("utf8mb4") is called once per new MySQL connection before any query is queued (survives the driver's auto-reconnect); the async SET NAMES is only sent as fallback if SetCharset fails.
  • Dead retry code removed: DB_SetNames / DB_CreateTable had retry branches that could never run (DB_Connect() is always true right after connecting) and would have passed a possibly-deleted raw handle to a timer. Removed along with the sm_manager_query_retry convar, which only fed those branches.
  • Queries: the INSERT / SELECT now use g_hDatabase.Format instead of plain Format (escapes the auth/address strings). The MySQL upsert now also sets timestamp=CURRENT_TIMESTAMP on duplicate key: the column had no ON UPDATE, so it kept the first-seen time while the SQLite INSERT OR REPLACE path refreshes it.
  • Version bump 2.3.6 -> 2.3.7.

Not included

  • LogPotentialSpoofer passes the raw client index to an async HTTP request (OnPlayerSummaryReceived / APIWebResponse); same class of bug but not SQL, left for a separate change.

Test plan

  • Compiles with spcomp 1.12.0.7259 (same deps as CI)
  • CI build passes
  • Change map several times: only one "Connected to database." in the logs, no reconnect per map
  • Restart the server with players reconnecting immediately: players authorized before "Connected to database." get their connection row inserted/updated after the connect
  • Stop MySQL at startup: "Connecting to database ... failed" logged, retry every 15 s, connects once MySQL is back
  • NoSteam client with a known Steam ID and different IP still gets kicked; with the same IP it is allowed
  • MySQL: timestamp in connection updates on every rejoin
  • sm_manager_query_retry can be removed from cfg/sourcemod/PlayerManager.cfg

🤖 Generated with Claude Code

- Pass the userid (not the client index) through the spoof-check query.
- Keep the connection across maps; re-check players authorized while
  the DB was not connected.
- Retry failed connects; only treat real connection errors as lost.
- SetCharset once per connection (SET NAMES as fallback), drop dead
  retry branches, db.Format for queries, refresh MySQL timestamp.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@WareZ57 WareZ57 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed against master (merges cleanly, CI green). It compiles with spcomp 1.12.0.7259 and the same deps as CI (MultiColors, connect.inc, ripext, ProxyKiller, utilshelper), and _connect_included is defined so the SQL code is actually compiled. No warnings.

What I checked:

  • The spoof-check pack now carries the userid, and Query_SelectConnection bails out on !client (PlayerManager.sp:865-875) before it can reach the KickClient at :902. The insert path just skips that cell. That's correct.
  • Reconnect state machine: DB_ScheduleReconnect is only reached from GotDatabase failure (only while Connecting and only for the current lock) and from DB_Conn_Lost (only when not already Wait/Connecting). So there's at most one TimerDB_Reconnect in flight, and without TIMER_FLAG_NO_MAPCHANGE the Wait state can't get stuck across a map change anymore. OnConfigsExecuted -> DB_Connect() is a no-op while connected, so keeping the handle across maps doesn't open new connections.
  • SetCharset runs before the handle is published and before DB_SetNames/DB_CreateTable queue anything (:662-671).
  • Ordering in GotDatabase: the table creation is queued at normal priority and DB_CheckPendingClients queues its SELECTs at DBPrio_Low, so on a fresh DB the table exists first.
  • DB_CheckPendingClients uses AuthId_Engine, which is what the OnClientAuthorized forward receives, so SteamClientGotValidateAuthTicketResponse matches the same key. Late-load players are picked up too, since their flag starts at false.
  • g_hDatabase.Format escaping and the timestamp=CURRENT_TIMESTAMP on the MySQL upsert both look fine.

One small gap, not blocking: a check that was sent but failed on a lost connection isn't re-run. OnClientAuthorized sets g_bConnectionChecked[client] = true (:212) before the SELECT. If that SELECT comes back with "gone away"/"Lost connection", Query_SelectConnection logs and returns (:877-882), the handle is dropped and a reconnect is scheduled. But DB_CheckPendingClients skips that player after the reconnect because the flag is already true. So a NoSteam spoofer who joins right as MySQL restarts isn't checked until the next map. Setting g_bConnectionChecked[client] = false; in that error branch (you already have client there) would let the reconnect pick them up.

LGTM otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants