Add REST API v1 for external PAT clients - #19
Conversation
External bots and website-next need PAT-authenticated REST instead of the panel RPC. Slice 0 covers admins, groups, and rehash.
Review — REST API v1Read the whole surface: The shape is right. A declarative route table locked by Two things I would want fixed before merge, then a handful of smaller ones. 1. The panel cookie is still read — and written — on every REST request
$userbank = new CUserManager(Auth::verify());
Consequences:
This contradicts the PR description's own invariant ("REST never reads the panel cookie"). The PR already establishes the right pattern — 2. REST admin create and update write no audit logThe mutations that go through
So the two highest-value writes in the API — granting in-game admin, and changing an existing admin's Steam ID, web group, immunity or server access — are the only ones invisible in the audit log. That is the inverse of the priority you would want, and it is precisely the flow the staff-hub integration is built around ( Worth fixing3. 4. Rate-limit files are never garbage collected. Two related notes while you are in there: the read-modify-write is not atomic, so concurrent requests undercount (fine for a limiter, worth a comment so the next reader does not file it as a bug); and anonymous keying on 5. Minting a PAT needs no re-authentication, and a password change does not revoke tokens. Smaller things
Items 1 and 2 are the ones I would hold the merge on — both are cases where the implementation quietly falls short of an invariant the PR description states, which is the kind of gap that gets trusted later. Everything below that is comfortably follow-up material. |
Second review pass
Two things I want to correct or firm up from the first pass before the new material. Verified clean, so ignore any worry here: the token UI is not an XSS vector. Also verified, and it makes the earlier suggestion cheaper than I implied: 6.
|
| RPC | Returns its insert id? | What the service does |
|---|---|---|
bans.add |
yes, bid |
uses it |
notes.add |
yes, nid |
uses it |
bans.add_comment |
yes — added by this PR ('cid' => $cid) |
uses it |
mods.add |
no | WHERE modfolder = :folder OR name = :name ORDER BY mid DESC |
comms.add |
no | MAX(bid) before, then bid > :before AND authid AND aid after |
The PR author clearly identified the problem and solved it properly once — the one-line addition of $cid to api_bans_add_comment() is exactly right. It just was not carried to the other two. Two of five write paths are left inferring their own result from a SELECT, in an API whose stated audience is bots that retry.
ModsService::create() is the worse of the two: OR across two columns plus ORDER BY mid DESC means that if mods.add ever succeeds in a way that does not produce the highest matching mid, or if an unrelated older mod matches on name where the new one matched on folder, the 201 describes the wrong resource. CommsService::create() is better fenced by the authid + aid scoping, but two concurrent identical calls from the same token still cross-attribute.
Adding a return value to mods.add and comms.add the way bans.add_comment just got one removes both SELECTs and both races. It also lets CommentsService::create() drop its latestCid() fallback, which is now unreachable in practice and is itself racy — ORDER BY cid DESC on the parent can pick up a concurrent comment from a different admin.
7. The protest and submission queues can be destroyed but not archived
GET /protests and GET /submissions both accept ?archived=true and read the archive. Nothing in the route table can write that flag. The only mutation exposed is DELETE, which both services hard-delete with archiv => '0':
Api::invoke('protests.remove', ['pid' => $pid, 'archiv' => '0']);So a bot triaging the queue has exactly one option, and it is the irreversible one. The panel's normal action for a handled report is to archive it — reversible, auditable, and the thing the archived=true filter exists to read back. The API can read the result of that action and can destroy rows, but cannot perform it.
Given submissions.remove and protests.remove already take archiv as a parameter, this looks like one route each (POST /{pid}/archive, or archived on a PATCH) rather than new handler work. Worth doing before external clients start reaching for DELETE because it is the only verb on offer.
8. The nginx snippet in the docs will 404 for most people who follow it
docs/.../rest-api.mdx gives this as the rewrite for non-Docker installs:
location /api/v1 {
rewrite ^/api/v1$ /api/v1.php last;
rewrite ^/api/v1/(.*)$ /api/v1.php/$1 last;
}That produces /api/v1.php/me, which is correct — and then a stock location ~ \.php$ block hands PHP-FPM SCRIPT_FILENAME=…/api/v1.php/me and gets "Primary script unknown", or never gets there at all because try_files $uri =404 rejects the path first. PATH_INFO on nginx needs the companion directives, which the page does not mention:
fastcgi_split_path_info ^(.+\.php)(/.*)$;
fastcgi_param PATH_INFO $fastcgi_path_info;This matters more than a normal docs nit because it is the only documented route for the non-Docker install base, and the page currently oversells it twice — line 47 calls the PATH_INFO form "always works, including tarball installs without rewrite", and line 206 repeats it. FrontController::requestPath() does have a REQUEST_URI regex fallback that saves the pretty-URL case, but it cannot save a request PHP-FPM refused to route. The Authorization half of the same section is fine — PatAuthenticator::authorizationHeader() falling back to getallheaders() genuinely does cover apache2handler.
9. The data-exposure inventory omits the two most identifying tables
The PR description and the docs both carry a careful list of what GET never returns: admin password / validate / attempts / lockout_until / srv_password, servers.rcon, settings.smtp.pass, settings.telemetry.instance_id. Neither mentions that GET /submissions returns email, ip and submitter_ip (sip), and GET /protests returns email and ip (pip).
To be fair to the implementation: I checked the panel, and page_admin_bans_submissions.tpl and page_admin_bans_protests.tpl show sip and pip to admins holding the same flags. This is not a permission bug — the fields match what the flag already grants.
What changes is the shape of the access. The panel shows one queue page to a signed-in human; the API hands a bot token 100 reporter email addresses and IPs per request, with ?archived=true to sweep the historical set as well. That is a bulk-egress surface that did not previously exist, sitting behind a token that (per finding 5 in the first pass) can be minted without re-authentication and never expires. It does not need a code change to be defensible, but it does need to be stated — the inventory that carefully lists smtp.pass should not be silent about the reporters' personal data.
10. Smaller things this pass turned up
GroupsService::list()is the only unpaginated list endpoint. Every other one capsper_pageat 100; this returns every web group and every SourceMod group in one response, withflagsfor both.SubmissionsService::toResource()emits raw unvalidated text assteam. The fallback is$steam2 ?? ($rawSteam !== '' ? $rawSteam : null), so a malformedSteamIdin the DB is passed through verbatim into a field the spec documents as a Steam2 id.BansServiceandAdminsServiceboth returnnullin that position.- The boolean-coercion split is now settled, and
wantsKick()is the outlier.ServersService(enabled),SubmissionsServiceandProtestsService(archived) all accepttrue | 1 | "1" | "true". OnlyBansService::wantsKick()narrows totrue | 1, so{"kick": "true"}silently drops the kick fan-out while{"archived": "true"}on the next endpoint works. One shared helper, andwantsKickis the one to change. - Two docs rows are silent exactly where they are most permissive. The routes table documents authorization for its neighbours —
DELETE /notes/{nid}says "Author or Owner",DELETE /comments/{cid}says "Owner only" — butPATCH /comments/{cid}says only "body", and that is the row where any web admin can edit any other admin's comment (first pass, last bullet). Same pattern one row up:GET /comms/{cid}/commentsis listed as plain "Public" with no note that it skips theconfig.enablecommsgate the/commsrows document so carefully — which is the behaviour I flagged as finding 3. Envelope::statusForCode()defaults to 400. The known codes are mapped well, but an unmapped server-fault code coming out of a legacy handler surfaces to the client as a 4xx, telling a bot to fix its request when the panel is the thing that broke. Adefault => 500for codes ending in_failed, or an explicit allowlist, would fail in the safer direction.- Two rough edges in the token card. Creating a token does not insert a row into the table — the secret appears, but the list only catches up on reload, and if it was the admin's first token the "No tokens yet." message stays on screen next to the secret they just minted. Revoking the last token removes its row but does not restore that empty state.
Where this leaves the PR
Nothing in this second pass displaces the first two items — the Auth::verify() cookie write and the missing audit log on admin create/update are still the ones I would gate on, and both are cases where the code falls short of an invariant the PR itself asserts.
Of the new material, 6 (return the insert ids) is the one I would most like to see land with this PR rather than after it, because the fix is already in the diff for the third case and the pattern is at its most obvious right now. 8 is a fast docs fix with an outsized blast radius for self-hosters. 7 and 9 are design and disclosure calls that are yours to make, not defects.
The overall judgement from the first pass is unchanged and if anything firmer after reading the rest: the architecture is sound, the permission matrix is the right shape, the AGENTS.md and ARCHITECTURE.md updates actually honour the repo's own documentation contract instead of skipping it, and the OpenAPI spec is genuinely in sync on day one. This is good work with a short, concrete list between it and merge.
2 participants
Description
This ships a versioned HTTP API at
/api/v1for external clients: bots, website backends, and scripts. It is a separate product fromPOST /api.php. Panel JavaScript stays on the JSON RPC (cookie JWT + CSRF). REST never reads the panel cookie.How operators use it
sbpp_pat_plus 64 hex characters and is shown once. The panel stores only a SHA-256 hash.Authorization: Bearer sbpp_pat_…on each request./api/v1/…(Apache rewrite in the prod image and the rebuilt dev image). PATH_INFO fallback always works:/api/v1.php/….Tokens inherit that admin's web flags. There are no extra scopes. A read-only bot is an admin with list flags, not a trimmed token. Soft-retired admins (
enabled = 0) cannot use a token. Password lockout does not apply. Revoke is the kill switch.The intended staff-hub flow (website-next): mint an Owner (or Add/Edit/Delete Admins) PAT in the backend,
PUT /admins/{steam64}to grant in-game admin,POST /admins/{steam64}/deactivateto demote. Discord stays in the bot. SourceBans does not know about Discord.Request shape
{ "data": …, "meta": … }with real HTTP status (200, 201, 400, 401, 403, 404, 409, 429, 500).{ "error": { "code", "message", "field"? } }.{id}on/admins/{id}is a numeric aid or a 17-digit Steam64 starting with 7. Steam2/Steam3 in the path is 400./bansand/commslengthis minutes (0 = permanent). GETlengthis seconds (what is stored).Api::invoke()where those already exist (ban, unban, comms, comments, servers add/remove/rcon, notes, mods, protests, submissions, admin deactivate/reactivate/remove, rehash). List/get, Steam64 upsert, PATCH/servers, and GET/PATCH/settingsare dedicated REST queries.sm_rehashserver-side whenconfig.enableadminrehashingis on and puts the result inmeta.rehash. If the caller lacks rehash permission, the mutate still succeeds andmeta.rehash.attemptedisfalse(the write is not rolled back with a 403).Retry-After.SB_REST_CORS_ORIGINSinconfig.phpfor a browser origin. Backend-to-panel calls do not need it.Auth and isolation (baked into the design)
Authorization: Bearer sbpp_pat_…. Cookie JWT must not authenticate REST (that would be a CSRF trap in browsers, and would leak IPs on public GET).api/v1.phpdefinesSBPP_RESTbeforeinit.php;CSRF::init()no-ops. PanelPOST /api.phpstill requires CSRF.Log::initis rebound to the PAT (or anonymous) userbank so audit rows are attributed to the token admin, not a leftover cookie session.password/validate/attempts/lockout_until/srv_password,servers.rcon,settings.smtp.pass,settings.telemetry.instance_id.EDIT_ADMINSalone cannot change an Owner.cid, not a later audit-log insert id./serversmatches?p=servers: enabled hosts only, nogroup_ids, ignoresenabled=, 404 on a disabled{sid}. A PAT may filterenabled=and seesgroup_ids.rconis never returned./commsis 404 whenconfig.enablecommsis off. A PAT still reads./bansand/commsapply the samebanlist.hideplayerips/banlist.hideadminnamegates as the panel lists.config.enablepubliccommentsis off (admins still see them). DELETE/comments/{id}is Owner.Schema, rewrite, docs
:prefix_api_tokens(fresh install instruc.sql, upgrade via updater812.php). Hash + prefix only.account.tokens_create/list/revoke(panel UI, not REST)./api/v1in prod + dev conf. Docs nginx snippet +HTTP_AUTHORIZATIONnote.docs/src/content/docs/configuring/rest-api.mdx. OpenAPI:web/api/openapi-v1.yaml, also served atGET /api/v1/openapi.yaml.Sbpp\Rest\Routes::all(). Adding a write route requires a row inRestPermissionMatrixTest.Endpoints
Base:
/api/v1or/api/v1.php. All authenticated routes need a PAT unless marked public.Meta
/openapi.yamlCaller
/meAdmins and groups
{id}is aid or Steam64./adminspage,per_page, cap 100)/admins/{id}/admins/{id}/admins/{id}/admins/{id}/deactivateenabled=0). Ban history keeps the name/admins/{id}/reactivate/admins/{id}reason/groupsgidmapping). No group write in this version/system/rehash{ "sids": [1,2] }. Admin mutate already rehashes when the setting is onBans
/bans/bans/{bid}/banslengthminutes. Optionalkick: truefans RCON (meta.kick)/bans/{bid}/unbanureasonComm blocks
/commskindis mute or gag. Silence is two rows. *404 for anonymous when Comm blocks are off/comms/{cid}/commskind: mute, gag, or silence/comms/{cid}/unblockureason/comms/{cid}Servers
/serversquery, neverrcon. Anonymous omitsgroup_idsand ignoresenabled=. PAT may filter/servers/{sid}/serversip/address,port,mod.enableddefaults true/servers/{sid}rconto keep the stored password/servers/{sid}/servers/{sid}/rconNotes, mods, queues
/notes?steam=/notessteam+body/notes/{nid}/mods,/mods/{mid}/modsname+folder/mods/{mid}ureason/protests,/protests/{pid}archived=truefor the archive/protests/{pid}archiv=0)/submissions,/submissions/{sid}archived=truefor archive/submissions/{sid}Comments and settings
/bans/{bid}/comments,/comms/{cid}/comments/bans/{bid}/comments,/comms/{cid}/commentsbody. Responseidis the comment cid/comments/{cid}body/comments/{cid}/settingssmtp.passortelemetry.instance_id/settingsMotivation and Context
External clients (website-next staff hub, Discord bots, scripts) need a real HTTP API with Bearer tokens, real status codes, and a stable resource model.
POST /api.phpis the panel RPC: cookie + CSRF, chrome envelopes, no PAT. Mixing those clients onto RPC would either leak session auth into browsers or force bots through CSRF. This API is the supported path for that work.Not in this version (by design): generated password on PUT create, a designed-from-scratch rate limiter beyond the file limiter, comment PATCH author check, OpenAPI completeness polish,
kick: truevsenablekickit, blockit fan-out on POST/comms. Group create/edit stays in the panel.How Has This Been Tested?
RestAuth,RestAdmins,RestBans,RestComms,RestServers,RestNotes,RestMods,RestProtests,RestSubmissions,RestComments,RestSettings,RestSession,RestPermissionMatrix) plus account token RPC snapshots.web/tests/e2e/specs/flows/rest-api.spec.ts(mint PAT on Your account, call/meand a write).Screenshots (if appropriate):
N/A (API + Your account token card). Operator docs at Configuring → REST API.
Types of changes
Checklist: