mirror of
https://github.com/nestriness/nestri.git
synced 2026-09-19 17:25:19 +03:00
fix(api): a misshapen id is bad input, not a server fault
Ids are stored in a fixed-width column, so an overlong one is refused by Postgres rather than simply matching nothing. That refusal is not a foreign-key violation, so it fell through to the global error boundary and reached the caller as a 500 — telling a host to retry something that can never succeed. Measured: a 44-character user id returned 500, where an absent but well-formed one correctly returned 404. `Identifier.schema` is the natural place for the check and had no callers yet, so it now asserts the exact width an id has as well as its prefix — including the separator, without which `usrsomething` reads as a user id. The enrolment schema uses it for both foreign keys, so the refusal happens where the input arrives and names the field. Also index `steam_enrolment.user_id`. The primary key begins with the machine, which answers what one host holds and nothing else, so neither of the two things that read by user alone can use it: the cascade behind deleting a user, and asking which hosts hold a token for one person. The table's migration has not been released, so this is folded into it rather than following it with a correction.
This commit is contained in:
@@ -149,12 +149,33 @@ describe('POST /machine/enrolment', () => {
|
|||||||
|
|
||||||
test('a user nobody has heard of is refused rather than crashing', async () => {
|
test('a user nobody has heard of is refused rather than crashing', async () => {
|
||||||
const host = await registeredHost('enrol-ghost');
|
const host = await registeredHost('enrol-ghost');
|
||||||
const res = await enrol(host, { userId: 'usr_nosuchuseratall', steamId: steamId(6) });
|
// Well-formed and simply absent, which is the case the foreign key
|
||||||
|
// catches. A malformed one never reaches the database at all.
|
||||||
|
const res = await enrol(host, {
|
||||||
|
userId: Identifier.ascending('user'),
|
||||||
|
steamId: steamId(6)
|
||||||
|
});
|
||||||
expect(res.status).toBe(404);
|
expect(res.status).toBe(404);
|
||||||
const body = (await res.json()) as any;
|
const body = (await res.json()) as any;
|
||||||
expect(body.type).toBe('not_found');
|
expect(body.type).toBe('not_found');
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test('a userId of the wrong shape is bad input, not a server fault', async () => {
|
||||||
|
// Ids live in a fixed-width column, so an overlong one is refused by
|
||||||
|
// the database rather than merely not found — and that refusal used to
|
||||||
|
// reach the host as a 500, which tells it to retry something that can
|
||||||
|
// never succeed. The width is checked where the input arrives.
|
||||||
|
const host = await registeredHost('enrol-misshapen');
|
||||||
|
const malformed = [`usr_${'a'.repeat(40)}`, 'usr_short', `mch_${'a'.repeat(26)}`, 'nonsense'];
|
||||||
|
for (const userId of malformed) {
|
||||||
|
// eslint-disable-next-line no-await-in-loop
|
||||||
|
const res = await enrol(host, { userId, steamId: steamId(15) });
|
||||||
|
expect(res.status).toBe(400);
|
||||||
|
// eslint-disable-next-line no-await-in-loop
|
||||||
|
expect(((await res.json()) as any).type).toBe('validation');
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
test('a Steam id has to look like one', async () => {
|
test('a Steam id has to look like one', async () => {
|
||||||
const host = await registeredHost('enrol-badsteam');
|
const host = await registeredHost('enrol-badsteam');
|
||||||
const res = await enrol(host, { userId: host.userId, steamId: 'not-a-steam-id' });
|
const res = await enrol(host, { userId: host.userId, steamId: 'not-a-steam-id' });
|
||||||
@@ -165,7 +186,7 @@ describe('POST /machine/enrolment', () => {
|
|||||||
const res = await app.request('/machine/enrolment', {
|
const res = await app.request('/machine/enrolment', {
|
||||||
method: 'POST',
|
method: 'POST',
|
||||||
headers: { 'x-nestri-admin-token': TEST_ADMIN_SECRET, 'content-type': 'application/json' },
|
headers: { 'x-nestri-admin-token': TEST_ADMIN_SECRET, 'content-type': 'application/json' },
|
||||||
body: JSON.stringify({ userId: 'usr_x', steamId: steamId(7) })
|
body: JSON.stringify({ userId: Identifier.ascending('user'), steamId: steamId(7) })
|
||||||
});
|
});
|
||||||
expect(res.status).toBe(403);
|
expect(res.status).toBe(403);
|
||||||
expect(((await res.json()) as any).message).toContain('Machine credentials');
|
expect(((await res.json()) as any).message).toContain('Machine credentials');
|
||||||
@@ -220,7 +241,7 @@ describe('POST /machine/enrolment/stale', () => {
|
|||||||
const res = await app.request('/machine/enrolment/stale', {
|
const res = await app.request('/machine/enrolment/stale', {
|
||||||
method: 'POST',
|
method: 'POST',
|
||||||
headers: { 'x-nestri-admin-token': TEST_ADMIN_SECRET, 'content-type': 'application/json' },
|
headers: { 'x-nestri-admin-token': TEST_ADMIN_SECRET, 'content-type': 'application/json' },
|
||||||
body: JSON.stringify({ userId: 'usr_x' })
|
body: JSON.stringify({ userId: Identifier.ascending('user') })
|
||||||
});
|
});
|
||||||
expect(res.status).toBe(403);
|
expect(res.status).toBe(403);
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -25,6 +25,12 @@
|
|||||||
-- itself only goes away when the machine or the user does, which the foreign
|
-- itself only goes away when the machine or the user does, which the foreign
|
||||||
-- keys already do.
|
-- keys already do.
|
||||||
--
|
--
|
||||||
|
-- The key begins with the machine, so it answers "what does this host hold" and
|
||||||
|
-- nothing else. `user_id` gets its own index because the two things that read
|
||||||
|
-- by user cannot use the key: deleting a user cascades into this table by that
|
||||||
|
-- column alone, and asking which hosts hold a token for one person is the
|
||||||
|
-- obvious next reader.
|
||||||
|
--
|
||||||
-- `last_ok_at` has no writer yet. A successful logon happens inside the
|
-- `last_ok_at` has no writer yet. A successful logon happens inside the
|
||||||
-- workload, which holds no control-plane credential, so the report has to come
|
-- workload, which holds no control-plane credential, so the report has to come
|
||||||
-- back out through the host and nothing carries it today. The column exists
|
-- back out through the host and nothing carries it today. The column exists
|
||||||
@@ -44,4 +50,5 @@ CREATE TABLE "steam_enrolment" (
|
|||||||
);
|
);
|
||||||
--> statement-breakpoint
|
--> statement-breakpoint
|
||||||
ALTER TABLE "steam_enrolment" ADD CONSTRAINT "steam_enrolment_machine_id_machine_id_fk" FOREIGN KEY ("machine_id") REFERENCES "public"."machine"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint
|
ALTER TABLE "steam_enrolment" ADD CONSTRAINT "steam_enrolment_machine_id_machine_id_fk" FOREIGN KEY ("machine_id") REFERENCES "public"."machine"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint
|
||||||
ALTER TABLE "steam_enrolment" ADD CONSTRAINT "steam_enrolment_user_id_user_id_fk" FOREIGN KEY ("user_id") REFERENCES "public"."user"("id") ON DELETE cascade ON UPDATE no action;
|
ALTER TABLE "steam_enrolment" ADD CONSTRAINT "steam_enrolment_user_id_user_id_fk" FOREIGN KEY ("user_id") REFERENCES "public"."user"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint
|
||||||
|
CREATE INDEX "steam_enrolment_user_idx" ON "steam_enrolment" USING btree ("user_id");
|
||||||
@@ -1,5 +1,5 @@
|
|||||||
{
|
{
|
||||||
"id": "5c6f5a6c-74ff-4b9d-95ac-bef2c2603273",
|
"id": "d519bc2f-25f8-46e1-b7df-67bf6b5a927a",
|
||||||
"prevId": "6fdda454-c7ee-42c1-931b-fa595385aac0",
|
"prevId": "6fdda454-c7ee-42c1-931b-fa595385aac0",
|
||||||
"version": "7",
|
"version": "7",
|
||||||
"dialect": "postgresql",
|
"dialect": "postgresql",
|
||||||
@@ -1860,7 +1860,23 @@
|
|||||||
"notNull": false
|
"notNull": false
|
||||||
}
|
}
|
||||||
},
|
},
|
||||||
"indexes": {},
|
"indexes": {
|
||||||
|
"steam_enrolment_user_idx": {
|
||||||
|
"name": "steam_enrolment_user_idx",
|
||||||
|
"columns": [
|
||||||
|
{
|
||||||
|
"expression": "user_id",
|
||||||
|
"isExpression": false,
|
||||||
|
"asc": true,
|
||||||
|
"nulls": "last"
|
||||||
|
}
|
||||||
|
],
|
||||||
|
"isUnique": false,
|
||||||
|
"concurrently": false,
|
||||||
|
"method": "btree",
|
||||||
|
"with": {}
|
||||||
|
}
|
||||||
|
},
|
||||||
"foreignKeys": {
|
"foreignKeys": {
|
||||||
"steam_enrolment_machine_id_machine_id_fk": {
|
"steam_enrolment_machine_id_machine_id_fk": {
|
||||||
"name": "steam_enrolment_machine_id_machine_id_fk",
|
"name": "steam_enrolment_machine_id_machine_id_fk",
|
||||||
|
|||||||
@@ -89,7 +89,7 @@
|
|||||||
{
|
{
|
||||||
"idx": 12,
|
"idx": 12,
|
||||||
"version": "7",
|
"version": "7",
|
||||||
"when": 1788690115352,
|
"when": 1788691753961,
|
||||||
"tag": "0012_steam_enrolment_without_a_token",
|
"tag": "0012_steam_enrolment_without_a_token",
|
||||||
"breakpoints": true
|
"breakpoints": true
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -27,8 +27,24 @@ export namespace Identifier {
|
|||||||
refreshToken: 'rft'
|
refreshToken: 'rft'
|
||||||
} as const;
|
} as const;
|
||||||
|
|
||||||
|
/**
|
||||||
|
* An id as this control plane issues them: the right prefix, and the exact
|
||||||
|
* width the column has.
|
||||||
|
*
|
||||||
|
* The width is the half that matters at an API boundary. Ids are stored in
|
||||||
|
* a fixed-width column, so an overlong string is *refused by the database*
|
||||||
|
* rather than simply not matching anything — which surfaces to the caller
|
||||||
|
* as a server fault instead of the validation error it actually is.
|
||||||
|
* Checking it where the input arrives is what keeps the two apart.
|
||||||
|
*
|
||||||
|
* The separator is part of the prefix check for the same reason: without
|
||||||
|
* it, `usrsomething` reads as a user id.
|
||||||
|
*/
|
||||||
export function schema(prefix: keyof typeof prefixes) {
|
export function schema(prefix: keyof typeof prefixes) {
|
||||||
return z.string().startsWith(prefixes[prefix]);
|
return z
|
||||||
|
.string()
|
||||||
|
.startsWith(`${prefixes[prefix]}_`)
|
||||||
|
.length(prefixes[prefix].length + 1 + LENGTH);
|
||||||
}
|
}
|
||||||
|
|
||||||
const LENGTH = 26;
|
const LENGTH = 26;
|
||||||
|
|||||||
@@ -1,4 +1,4 @@
|
|||||||
import { pgEnum, pgTable, primaryKey, text } from 'drizzle-orm/pg-core';
|
import { index, pgEnum, pgTable, primaryKey, text } from 'drizzle-orm/pg-core';
|
||||||
|
|
||||||
import { ulid, utc } from '../db/types.js';
|
import { ulid, utc } from '../db/types.js';
|
||||||
import { MachineTable } from '../machine/machine.sql.js';
|
import { MachineTable } from '../machine/machine.sql.js';
|
||||||
@@ -61,5 +61,12 @@ export const SteamEnrolmentTable = pgTable(
|
|||||||
lastOkAt: utc('last_ok_at'),
|
lastOkAt: utc('last_ok_at'),
|
||||||
revokedAt: utc('revoked_at')
|
revokedAt: utc('revoked_at')
|
||||||
},
|
},
|
||||||
(t) => [primaryKey({ columns: [t.machineId, t.userId] })]
|
(t) => [
|
||||||
|
primaryKey({ columns: [t.machineId, t.userId] }),
|
||||||
|
// The key starts with the machine, which answers "what does this host
|
||||||
|
// hold" and nothing else. Deleting a user cascades into this table by
|
||||||
|
// `user_id` alone, and asking which hosts hold a token for one person
|
||||||
|
// is the obvious next reader — neither can use the key.
|
||||||
|
index('steam_enrolment_user_idx').on(t.userId)
|
||||||
|
]
|
||||||
);
|
);
|
||||||
|
|||||||
@@ -2,6 +2,7 @@ import { afterAll, describe, expect, test } from 'bun:test';
|
|||||||
|
|
||||||
import { Fixtures } from '../db/fixtures.js';
|
import { Fixtures } from '../db/fixtures.js';
|
||||||
import { testDb } from '../db/test.js';
|
import { testDb } from '../db/test.js';
|
||||||
|
import { Identifier } from '../id.js';
|
||||||
import { Enrolment } from './enrolment.js';
|
import { Enrolment } from './enrolment.js';
|
||||||
|
|
||||||
const sql = testDb();
|
const sql = testDb();
|
||||||
@@ -121,6 +122,22 @@ describe('Enrolment.markStale', () => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe('the user foreign key has its own index', () => {
|
||||||
|
test('deleting a user, and asking by user, do not scan the table', async () => {
|
||||||
|
// The primary key starts with the machine, so neither of the two things
|
||||||
|
// that read by user alone can use it: the cascade behind a user
|
||||||
|
// deletion, and the question "which hosts hold a token for me".
|
||||||
|
const indexes = await sql<{ indexdef: string }[]>`
|
||||||
|
select indexdef from pg_indexes
|
||||||
|
where schemaname = 'public'
|
||||||
|
and tablename = 'steam_enrolment'
|
||||||
|
and indexname = 'steam_enrolment_user_idx'
|
||||||
|
`;
|
||||||
|
expect(indexes).toHaveLength(1);
|
||||||
|
expect(indexes[0]!.indexdef).toContain('user_id');
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
describe('Enrolment.listByMachine', () => {
|
describe('Enrolment.listByMachine', () => {
|
||||||
test('every enrolment for one host, oldest first', async () => {
|
test('every enrolment for one host, oldest first', async () => {
|
||||||
const h = await host('core-list');
|
const h = await host('core-list');
|
||||||
@@ -140,6 +157,6 @@ describe('Enrolment.listByMachine', () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
test('an unknown host has no enrolments rather than an error', async () => {
|
test('an unknown host has no enrolments rather than an error', async () => {
|
||||||
expect(await Enrolment.listByMachine('mch_nosuchmachine')).toEqual([]);
|
expect(await Enrolment.listByMachine(Identifier.ascending('machine'))).toEqual([]);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -5,6 +5,7 @@ import { Database } from '../db/index.js';
|
|||||||
import { ErrorCodes, VisibleError } from '../error.js';
|
import { ErrorCodes, VisibleError } from '../error.js';
|
||||||
import { Examples } from '../examples.js';
|
import { Examples } from '../examples.js';
|
||||||
import { fn } from '../fn.js';
|
import { fn } from '../fn.js';
|
||||||
|
import { Identifier } from '../id.js';
|
||||||
import { SteamEnrolmentState, SteamEnrolmentTable } from './enrolment.sql.js';
|
import { SteamEnrolmentState, SteamEnrolmentTable } from './enrolment.sql.js';
|
||||||
import { STEAM_ID_RE } from './index.js';
|
import { STEAM_ID_RE } from './index.js';
|
||||||
|
|
||||||
@@ -31,11 +32,15 @@ function isForeignKeyViolation(err: unknown): boolean {
|
|||||||
export namespace Enrolment {
|
export namespace Enrolment {
|
||||||
export const Info = z
|
export const Info = z
|
||||||
.object({
|
.object({
|
||||||
machineId: z.string().meta({
|
// Shaped, not merely non-empty. Both are foreign keys into
|
||||||
|
// fixed-width columns, so a string of the wrong width is rejected
|
||||||
|
// by the database itself — and a database refusal reaches a caller
|
||||||
|
// as a server fault rather than as the bad input it is.
|
||||||
|
machineId: Identifier.schema('machine').meta({
|
||||||
description: 'The host that holds a token for this user',
|
description: 'The host that holds a token for this user',
|
||||||
example: Examples.SteamEnrolment.machineId
|
example: Examples.SteamEnrolment.machineId
|
||||||
}),
|
}),
|
||||||
userId: z.string().meta({
|
userId: Identifier.schema('user').meta({
|
||||||
description: 'The person the host signed in as',
|
description: 'The person the host signed in as',
|
||||||
example: Examples.SteamEnrolment.userId
|
example: Examples.SteamEnrolment.userId
|
||||||
}),
|
}),
|
||||||
|
|||||||
Reference in New Issue
Block a user