fix(sqlite): upsert syntax error on addvip, keep name/lastvisit on setvip by Rushaway · Pull Request #27 · srcdslab/sm-plugin-VIP-Core

Rushaway · 2026-09-20T09:23:46Z

Summary

SQL_UpdateVIP(): the pack written by UTIL_SET_VIP_PLAYER() never included the target's name, and iLastVisit was computed from iTarget before iTarget was actually read from the pack — so sm_setvip-path VIPs were stored with an empty name and lastvisit = 0. Fixed by deriving both from iTarget after it's read, mirroring UTIL_ADD_VIP_PLAYER().
SQL_UpdateVIP() builds its query with plain FormatEx() (no auto-escaping), so Database.Escape() on name/group there is genuinely needed and correct.
Correction (see comment on SQL injection via unescaped player name in vip_users INSERT/UPDATE queries #25): my original fix also added Database.Escape() calls in DB_UpdateClient() and UTIL_ADD_VIP_PLAYER(), both of which build their query with g_hDatabase.Format() — which already escapes %s arguments by default. Those calls were redundant and caused double-escaping (corrupting names containing quotes/backslashes). Reverted just those two spots back to passing the raw strings directly; g_hDatabase.Format() still escapes them correctly on its own.

Fixes #26. Partially addresses #25 (only the SQL_UpdateVIP() part of it was a real gap).

Test plan

Compiled cleanly with the local SourcePawn compiler (no errors/warnings).

🤖 Generated with Claude Code

- DB_UpdateClient(), UTIL_ADD_VIP_PLAYER() and SQL_UpdateVIP() formatted the player's display name (fully attacker-controlled) and VIP group straight into vip_users queries with no escaping. Escape both before formatting. - UTIL_SET_VIP_PLAYER()/SQL_UpdateVIP() never populated the name written to the DB (declared but unassigned local), storing an empty name for every VIP added/updated via this path. Also fixed iLastVisit, which was computed from iTarget before iTarget was actually read from the DataPack, so it was always stored as 0. Both now mirror UTIL_ADD_VIP_PLAYER()'s pattern of deriving name/lastvisit from iTarget once it's known. Fixes #25 Fixes #26 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Copilot

Copilot review overview

🟡 Changes recommended

SQL query buffers can truncate escaped values, and existing VIP rows do not receive updated names or visit timestamps.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 4

Open (4)

Increase query buffer for escaped maximum-length names · New
Resize query buffer for escaped name and group values · New
Prevent query truncation for repeated escaped group value · New
Update name and lastvisit in duplicate-key clause · New

What changed in this PR

This PR hardens VIP database writes against SQL injection and fixes missing player names and visit timestamps in sm_setvip.

Changes:

Escapes player names and VIP groups before SQL formatting.
Correctly derives target names and lastvisit timestamps.
Remaining issues: SQL buffers may truncate escaped values, and duplicate-key updates omit name and lastvisit.

File	Summary
`addons/sourcemod/scripting/vip/UTIL.sp`	Updates VIP insert logic and target metadata handling; query buffers need resizing, and duplicate-key updates must include `name` and `lastvisit`.
`addons/sourcemod/scripting/vip/Database.sp`	Escapes names during client updates; the query buffer must accommodate escaped maximum-length names.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+		char szName[MNL], szNameEsc[MNL * 2 + 1];
 		GetClientName(iClient, SZF(szName));
-		g_hDatabase.Format(SZF(szQuery), "UPDATE `vip_users` SET `name` = '%s', `lastvisit` = %d WHERE `account_id` = %d%s;", szName, GetTime(), iClientID, g_szSID);
+		g_hDatabase.Escape(szName, SZF(szNameEsc));


+	char szNameEsc[MNL * 2 + 1], szGroupEsc[64 * 2 + 1];
+	g_hDatabase.Escape(szName, SZF(szNameEsc));
+	g_hDatabase.Escape(szGroup, SZF(szGroupEsc));


-	int iTarget, iExpires, iAccountID, iLastVisit = iTarget ? GetTime() : 0;
-	char szQuery[PMP*2], szName[MNL*2+1], szAdmin[PMP], szTargetInfo[PMP], szGroup[64];
+	int iTarget, iExpires, iAccountID, iLastVisit;
+	char szQuery[PMP*2], szName[MNL], szAdmin[PMP], szTargetInfo[PMP], szGroup[64];


 	{
 		FormatEx(SZF(szQuery), "INSERT INTO `vip_users` (`account_id`, `sid`, `expires`, `group`, `name`, `lastvisit`) VALUES (%d, %d, %d, '%s', '%s', %d) \
-		ON DUPLICATE KEY UPDATE `expires` = %d, `group` = '%s';", iAccountID, g_CVAR_iServerID, iExpires, szGroup, szName, iLastVisit, iExpires, szGroup);
+		ON DUPLICATE KEY UPDATE `expires` = %d, `group` = '%s';", iAccountID, g_CVAR_iServerID, iExpires, szGroupEsc, szNameEsc, iLastVisit, iExpires, szGroupEsc);


…() calls DB_UpdateClient() and UTIL_ADD_VIP_PLAYER() both build their queries with g_hDatabase.Format(), which already escapes %s arguments by default (see the Database.Format doc comment in SourceMod's dbi.inc: "All format specifiers are escaped (see SQL_EscapeString) unless the '!' flag is used"). The g_hDatabase.Escape() calls I added in front of those double-escape every string - e.g. an apostrophe in a name would come back with extra backslashes in the database, corrupting stored names instead of fixing anything. Reverted those two spots back to passing the raw strings directly. SQL_UpdateVIP() is unaffected and unchanged: both of its branches build the query with plain FormatEx(), which does not auto-escape, so the g_hDatabase.Escape() calls there are genuinely needed and correct - as is the unrelated empty-name/lastvisit-always-0 fix in the same function. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

- UTIL_ADD_VIP_PLAYER(): the SQLite ON CONFLICT DO UPDATE SET clause was missing a comma between the `group` and `expires` assignments, making the whole query fail to parse on every call (insert or conflict) when running on the SQLite backend. Added the missing comma. - SQL_UpdateVIP(): the SQLite branch used INSERT OR REPLACE, which rewrites the entire row on an account_id conflict. Combined with the new offline fallback (name='unknown', lastvisit=0) added in the previous commit, running sm_setvip on an offline account that already has a real name/lastvisit stored would silently overwrite that data. Switched to a partial "INSERT ... ON CONFLICT DO UPDATE SET expires, group" upsert, matching the MySQL branch's ON DUPLICATE KEY UPDATE semantics (only expires/group change on conflict). - SQL_UpdateVIP(): `if (iTarget)` treated GET_CID()'s -1 sentinel (returned when a stored userid no longer maps to a connected client) as truthy, which would call GetClientName(-1, ...) instead of falling back to the offline branch. Changed to `if (iTarget > 0)`. - SQL_UpdateVIP(): switched both branches from FormatEx()+manual Database.Escape() scratch buffers to g_hDatabase.Format(), matching the auto-escaping pattern already used by UTIL_ADD_VIP_PLAYER(), UTIL_SET_VIP_PLAYER() and DB_UpdateClient() in this same file, and removing the now-unneeded szNameEsc/szGroupEsc buffers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

cmer81

Verified locally on SourcePawn 1.13.0.7427: base (master @ 8eccd2f) and head (928d20d) both compile, no new warnings.

LGTM. The escaping approach is right: Database.Format() escapes every %s argument (dbi.inc: "All format specifiers are escaped unless the '!' flag is used"), so switching SQL_UpdateVIP() from FormatEx() to g_hDatabase.Format() closes the injection, and dropping the manual Escape() calls elsewhere avoids double-escaping.

Worth calling out: the missing comma after `group` = excluded.`group` in the SQLite branch of UTIL_ADD_VIP_PLAYER() (introduced in 2f2f418) made that statement a syntax error, so on SQLite every add through that path was failing. This PR fixes it — probably worth a release.

Non-blocking:

The PR description still says SQL_UpdateVIP() uses FormatEx() + Database.Escape(); the code now uses g_hDatabase.Format(). Worth updating so the history stays accurate.
VIP_VERSION is still 3.1.2 R; a patch bump would help tell fixed servers apart.
Two small remarks inline (query buffer size, name/lastvisit on existing rows).

… fixes #28 already moved the sm_setvip query inline with g_hDatabase.Format (escaping + name/lastvisit), superseding the SQL_UpdateVIP() rewrite. What this PR still brings on top of master: - missing comma in UTIL_ADD_VIP_PLAYER's SQLite upsert (syntax error) - partial upsert instead of INSERT OR REPLACE for sm_setvip on SQLite, so an offline target keeps its known name/lastvisit

cmer81 · 2026-09-27T12:08:15Z

Merged master into this branch to resolve the conflict with #28 (351fdfd).

#28 already moved the sm_setvip query inline in UTIL_SET_VIP_PLAYER() with g_hDatabase.Format(), so the injection and the empty name/lastvisit are fixed on master and the SQL_UpdateVIP() rewrite here is superseded. I kept master's version and only retained what this PR still adds:

the missing comma in UTIL_ADD_VIP_PLAYER()'s SQLite upsert (`group` = excluded.`group`,), which made sm_addvip on an existing account a syntax error on SQLite
a partial upsert instead of INSERT OR REPLACE for sm_setvip on SQLite, so an offline target keeps its known name/lastvisit (account_id is the PRIMARY KEY there)

Net diff vs master: +6/-2 in UTIL.sp. Compiles on SM 1.13.0.7427 with no warnings. The title could be updated to something like "fix(sqlite): upsert syntax error on addvip, keep name/lastvisit on setvip".

Copilot AI lite review requested due to automatic review settings September 20, 2026 09:23

Copilot started reviewing on behalf of Rushaway September 20, 2026 09:24 View session

Copilot AI reviewed Sep 20, 2026

View reviewed changes

Rushaway and others added 2 commits September 20, 2026 15:33

cmer81 approved these changes Sep 27, 2026

View reviewed changes

Comment thread addons/sourcemod/scripting/vip/UTIL.sp Outdated

Comment thread addons/sourcemod/scripting/vip/UTIL.sp Outdated

cmer81 changed the title ~~Fix SQL injection via player name and empty name/lastvisit on sm_setvip~~ fix(sqlite): upsert syntax error on addvip, keep name/lastvisit on setvip Sep 27, 2026

cmer81 merged commit abb2aad into master Sep 27, 2026
6 checks passed

Navigation Menu

fix(sqlite): upsert syntax error on addvip, keep name/lastvisit on setvip - #27

fix(sqlite): upsert syntax error on addvip, keep name/lastvisit on setvip#27
cmer81 merged 4 commits into
masterfrom
fix/sql-injection-and-empty-name-on-set-vip

Rushaway commented Sep 20, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

cmer81 left a comment

Uh oh!

Uh oh!

Uh oh!

cmer81 commented Sep 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Rushaway commented Sep 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

cmer81 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cmer81 commented Sep 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Rushaway commented Sep 20, 2026 •

edited

Loading