feat(playtest): add playtest portal backend #242

Merged
sionzee merged 3 commits from feat/playtest-portal into main 2026-08-11 00:24:58 +02:00
Owner

Adds the API surface behind playtest.evobug.com: per-product branded
signup funnels, age gating, NDA collection, waitlist selection and Steam
key distribution.

Two classes of secret are stored and neither is ever readable from a
database dump alone:

  • Steam keys, sealed with AES-256-GCM under an HKDF-derived,
    domain-separated subkey. List endpoints return the masked form only;
    plaintext requires a separate revealKey call with a stated reason,
    and every reveal is audited. Assignment uses SELECT ... FOR UPDATE
    SKIP LOCKED plus a unique index on assignedApplicationId, so two
    admins acting at once cannot issue the same key twice.

  • NDA legal name and postal address, encrypted the same way. Only the
    country code and a hash of the exact document text stay readable, so
    the agreement can be proven without a second copy of the personal
    data.

The date of birth is deliberately not stored at all. The age gate checks
it in memory, discards it, and persists only the bracket that was met
("18+") - the funnel promises this to the player, so it is enforced in
the schema rather than by convention.

Admin identity is kept entirely separate from the public users table: a
compromised playtester account must not be one role flip away from the
key vault. Admins are invite-only with argon2id passwords, mandatory
TOTP (RFC 6238, implemented in-repo and verified against the spec's test
vectors, with a persisted counter to block replay), sessions stored only
as SHA-256 hashes with idle and absolute expiry, per-account lockout,
and an append-only audit log.

Providers whose Evobug application is not configured on a deployment are
no longer advertised on the signup screen, so a player cannot be sent to
a dead end; requesting one returns 503 rather than a generic 500.
Required connections are intentionally not filtered - dropping one would
silently disable a gate an admin turned on.

Also extends users with steamId/mickleId and the display handles the
account screen needs. The two touched spec files are existing
toStrictEqual assertions on the full user shape.

676 tests pass.

Adds the API surface behind playtest.evobug.com: per-product branded signup funnels, age gating, NDA collection, waitlist selection and Steam key distribution. Two classes of secret are stored and neither is ever readable from a database dump alone: - Steam keys, sealed with AES-256-GCM under an HKDF-derived, domain-separated subkey. List endpoints return the masked form only; plaintext requires a separate revealKey call with a stated reason, and every reveal is audited. Assignment uses SELECT ... FOR UPDATE SKIP LOCKED plus a unique index on assignedApplicationId, so two admins acting at once cannot issue the same key twice. - NDA legal name and postal address, encrypted the same way. Only the country code and a hash of the exact document text stay readable, so the agreement can be proven without a second copy of the personal data. The date of birth is deliberately not stored at all. The age gate checks it in memory, discards it, and persists only the bracket that was met ("18+") - the funnel promises this to the player, so it is enforced in the schema rather than by convention. Admin identity is kept entirely separate from the public users table: a compromised playtester account must not be one role flip away from the key vault. Admins are invite-only with argon2id passwords, mandatory TOTP (RFC 6238, implemented in-repo and verified against the spec's test vectors, with a persisted counter to block replay), sessions stored only as SHA-256 hashes with idle and absolute expiry, per-account lockout, and an append-only audit log. Providers whose Evobug application is not configured on a deployment are no longer advertised on the signup screen, so a player cannot be sent to a dead end; requesting one returns 503 rather than a generic 500. Required connections are intentionally not filtered - dropping one would silently disable a gate an admin turned on. Also extends users with steamId/mickleId and the display handles the account screen needs. The two touched spec files are existing toStrictEqual assertions on the full user shape. 676 tests pass.
feat(playtest): add playtest portal backend
Some checks failed
OpenCodeReview / review (deepseek) (pull_request) Failing after 5s
Tests / test (pull_request) Failing after 50s
OpenCodeReview / review (luna) (pull_request) Successful in 20m32s
61f937726a
Adds the API surface behind playtest.evobug.com: per-product branded
signup funnels, age gating, NDA collection, waitlist selection and Steam
key distribution.

Two classes of secret are stored and neither is ever readable from a
database dump alone:

  - Steam keys, sealed with AES-256-GCM under an HKDF-derived,
    domain-separated subkey. List endpoints return the masked form only;
    plaintext requires a separate revealKey call with a stated reason,
    and every reveal is audited. Assignment uses SELECT ... FOR UPDATE
    SKIP LOCKED plus a unique index on assignedApplicationId, so two
    admins acting at once cannot issue the same key twice.

  - NDA legal name and postal address, encrypted the same way. Only the
    country code and a hash of the exact document text stay readable, so
    the agreement can be proven without a second copy of the personal
    data.

The date of birth is deliberately not stored at all. The age gate checks
it in memory, discards it, and persists only the bracket that was met
("18+") - the funnel promises this to the player, so it is enforced in
the schema rather than by convention.

Admin identity is kept entirely separate from the public users table: a
compromised playtester account must not be one role flip away from the
key vault. Admins are invite-only with argon2id passwords, mandatory
TOTP (RFC 6238, implemented in-repo and verified against the spec's test
vectors, with a persisted counter to block replay), sessions stored only
as SHA-256 hashes with idle and absolute expiry, per-account lockout,
and an append-only audit log.

Providers whose Evobug application is not configured on a deployment are
no longer advertised on the signup screen, so a player cannot be sent to
a dead end; requesting one returns 503 rather than a generic 500.
Required connections are intentionally not filtered - dropping one would
silently disable a gate an admin turned on.

Also extends users with steamId/mickleId and the display handles the
account screen needs. The two touched spec files are existing
toStrictEqual assertions on the full user shape.

676 tests pass.
fix(playtest): stop the test helper clobbering JWT_SECRET
All checks were successful
Tests / test (pull_request) Successful in 51s
208d9ac4b2
The suite passed locally and failed 41 tests in CI, including the
pre-existing Reviews specs which this branch never touched.

Cause: playtest/test-helpers.ts assigned process.env.JWT_SECRET in its
own module body. Contracts here disagree on when they read that variable
- auth/index.ts snapshots it at import, reviews/index.ts and shop
read it per call - so an assignment from a helper module lands after
some readers and before others. Reviews then signed tokens via auth with
one secret and verified them with another, and the playtest specs failed
wherever a seeded user token had to survive requireUser.

It only reproduced without a .env file: with one present, the ??= was a
no-op and every reader agreed. That is precisely the difference between
a developer machine and CI, which is why local runs stayed green.

Set the test defaults in src/test-preload.ts instead. bunfig.toml
preloads it before any module is evaluated, so there is a single
consistent value no matter when a module reads it.

Also removed the module-load capture from the playtest contracts in
favour of a shared getJwtSecret(), memoised on the value, so these
modules cannot drift from the per-call readers again. That also
de-duplicates the constant across three files.

676 pass with and without .env.
ocr-bot left a comment

OpenCodeReview (openai/gpt-5.6-luna) — 40 findings

40 posted inline.


model openai/gpt-5.6-luna · 19 file(s) reviewed · 6,681,950 tokens (5,739,157 in / 942,793 out / 2,534,333 cached — 44% of input) · 20m27s

<!-- open-code-review model="openai/gpt-5.6-luna" --> ### OpenCodeReview (`openai/gpt-5.6-luna`) — 40 findings 40 posted inline. --- <sub>model `openai/gpt-5.6-luna` · 19 file(s) reviewed · 6,681,950 tokens (5,739,157 in / 942,793 out / 2,534,333 cached — 44% of input) · 20m27s</sub>
Lines 60-63
@ -0,0 +57,7 @@
"drifter", "phantom", "airlock", "salvage", "beacon", "nebula", "gantry", "thruster",
];
const picked = Array.from(
crypto.getRandomValues(new Uint8Array(4)),
(byte) => words[byte % words.length],
);
Member

[security · medium] The generated password has only 4 words from a 16-word dictionary (2^16 possibilities) plus two random bytes (2^16), for at most 32 bits of entropy—not the “strong” password promised by the option documentation. If the hash is exposed or the online controls are bypassed, this is readily brute-forceable. Generate the credential from substantially more random bytes or use a much larger unbiased word list.

return Buffer.from(crypto.getRandomValues(new Uint8Array(24))).toString("base64url");
**[security · medium]** The generated password has only 4 words from a 16-word dictionary (2^16 possibilities) plus two random bytes (2^16), for at most 32 bits of entropy—not the “strong” password promised by the option documentation. If the hash is exposed or the online controls are bypassed, this is readily brute-forceable. Generate the credential from substantially more random bytes or use a much larger unbiased word list. ```suggestion return Buffer.from(crypto.getRandomValues(new Uint8Array(24))).toString("base64url"); ```
sionzee marked this conversation as resolved
Lines 70-73
@ -0,0 +67,7 @@
}
const args = parseArgs(process.argv.slice(2));
const email = args.email?.trim().toLowerCase();
const displayName = args.name?.trim();
if (!email || !displayName) {
Member

[other · low] The parser treats a missing option value as the literal string "true" and accepts unknown flags. In addition, the email is only trimmed/lowercased and the display name is not checked for control characters. For example, --email --name Ops creates an account with email true, which the login contract rejects as an invalid email, and untrusted display-name characters are interpolated into terminal output. Reject unknown/missing flags and validate the email/name before any insert.

const email = adminEmailSchema.parse(args.email);
const displayName = safeDisplayNameSchema.parse(args.name);
**[other · low]** The parser treats a missing option value as the literal string `"true"` and accepts unknown flags. In addition, the email is only trimmed/lowercased and the display name is not checked for control characters. For example, `--email --name Ops` creates an account with email `true`, which the login contract rejects as an invalid email, and untrusted display-name characters are interpolated into terminal output. Reject unknown/missing flags and validate the email/name before any insert. ```suggestion const email = adminEmailSchema.parse(args.email); const displayName = safeDisplayNameSchema.parse(args.name); ```
sionzee marked this conversation as resolved
Lines 90-91
@ -0,0 +87,5 @@
process.exit(1);
}
const password = args.password ?? process.env.PLAYTEST_ADMIN_PASSWORD ?? generatePassword();
const generatedPassword = !args.password && !process.env.PLAYTEST_ADMIN_PASSWORD;
Member

[security · medium] Reading --password from process.argv exposes the administrator password through shell history and local process-list inspection. This is especially problematic because the script is intended to bootstrap credentials and subsequently prints them. Reject the CLI password option and obtain it from a hidden interactive prompt or a protected secret manager instead.

if (args.password) throw new Error("Do not pass administrator passwords on the command line");
const password = process.env.PLAYTEST_ADMIN_PASSWORD ?? (await readPasswordFromProtectedStdin());
**[security · medium]** Reading `--password` from `process.argv` exposes the administrator password through shell history and local process-list inspection. This is especially problematic because the script is intended to bootstrap credentials and subsequently prints them. Reject the CLI password option and obtain it from a hidden interactive prompt or a protected secret manager instead. ```suggestion if (args.password) throw new Error("Do not pass administrator passwords on the command line"); const password = process.env.PLAYTEST_ADMIN_PASSWORD ?? (await readPasswordFromProtectedStdin()); ```
sionzee marked this conversation as resolved
Lines 98-99
@ -0,0 +95,5 @@
process.exit(1);
}
const client = new Bun.SQL(process.env.DATABASE_URL);
const db = drizzle({ client, relations });
Member

[maintainability · medium] All database, hashing, encryption, and insert operations are top-level awaits, while client.end() is only reached on success or the existing-admin branch. A connection or database error therefore produces an uncaught exception and skips cleanup, leaving operators without a controlled failure result and potentially leaving the SQL client open. Put the database work in try/finally and report expected failures explicitly.

const client = new Bun.SQL(process.env.DATABASE_URL);
try {
	// perform the select/insert and credential setup here
} finally {
	await client.end();
}
**[maintainability · medium]** All database, hashing, encryption, and insert operations are top-level awaits, while `client.end()` is only reached on success or the existing-admin branch. A connection or database error therefore produces an uncaught exception and skips cleanup, leaving operators without a controlled failure result and potentially leaving the SQL client open. Put the database work in `try/finally` and report expected failures explicitly. ```suggestion const client = new Bun.SQL(process.env.DATABASE_URL); try { // perform the select/insert and credential setup here } finally { await client.end(); } ```
sionzee marked this conversation as resolved
Lines 101-105
@ -0,0 +98,8 @@
const client = new Bun.SQL(process.env.DATABASE_URL);
const db = drizzle({ client, relations });
const [existing] = await db
.select({ id: playtestAdminsTable.id })
.from(playtestAdminsTable)
.where(eq(playtestAdminsTable.email, email))
.limit(1);
Member

[maintainability · medium] The duplicate check and insert are separate operations. Two invocations can both observe no existing row; the unique email constraint prevents duplicate identities, but the losing insert then raises an uncaught database error instead of the intended duplicate-admin message. Use an atomic insert with onConflictDoNothing (or catch and classify the unique-constraint error) and handle the no-row result.

const [admin] = await db
	.insert(playtestAdminsTable)
	.values(/* ... */)
	.onConflictDoNothing({ target: playtestAdminsTable.email })
	.returning({ id: playtestAdminsTable.id });
**[maintainability · medium]** The duplicate check and insert are separate operations. Two invocations can both observe no existing row; the unique email constraint prevents duplicate identities, but the losing insert then raises an uncaught database error instead of the intended duplicate-admin message. Use an atomic insert with `onConflictDoNothing` (or catch and classify the unique-constraint error) and handle the no-row result. ```suggestion const [admin] = await db .insert(playtestAdminsTable) .values(/* ... */) .onConflictDoNothing({ target: playtestAdminsTable.email }) .returning({ id: playtestAdminsTable.id }); ```
sionzee marked this conversation as resolved
Lines 136-138
@ -0,0 +133,6 @@
const line = "─".repeat(66);
console.log(`
${line}
Playtest admin created - id ${admin?.id}
Member

[security · high] This stdout block emits the generated password (when no password was supplied), the TOTP secret/URI, and every recovery code. Stdout can be captured by CI/deployment logs, shell redirection, terminal recording, or a supervisor; possession of the password plus the TOTP secret (or a recovery code) is sufficient to complete the admin login. Require a private, operator-confirmed secret-delivery channel before creating the account and refuse non-interactive output rather than writing all authentication factors to stdout.

if (!process.stdout.isTTY || !process.stdin.isTTY) {
	throw new Error("Refusing to print bootstrap credentials without a private interactive TTY");
}
**[security · high]** This stdout block emits the generated password (when no password was supplied), the TOTP secret/URI, and every recovery code. Stdout can be captured by CI/deployment logs, shell redirection, terminal recording, or a supervisor; possession of the password plus the TOTP secret (or a recovery code) is sufficient to complete the admin login. Require a private, operator-confirmed secret-delivery channel before creating the account and refuse non-interactive output rather than writing all authentication factors to stdout. ```suggestion if (!process.stdout.isTTY || !process.stdin.isTTY) { throw new Error("Refusing to print bootstrap credentials without a private interactive TTY"); } ```
sionzee marked this conversation as resolved
Lines 159-169
@ -0,0 +156,14 @@
const inviteToken = generateToken(32);
const expiresAt = new Date(Date.now() + ADMIN_INVITE_TTL_MS);
const [created] = await context.db
.insert(playtestAdminsTable)
.values({
email: input.email,
displayName: input.displayName,
status: "invited",
invitedByAdminId: actor.id,
inviteTokenHash: await sha256Hex(inviteToken),
inviteExpiresAt: expiresAt,
})
.returning();
Member

[bug · medium] The preflight findAdminByEmail check does not serialize with this insert. The schema's unique email constraint prevents duplicate rows, but concurrent invitations can still raise an unhandled unique-constraint exception instead of the declared ADMIN_EXISTS error. Use an atomic conflict-safe insert and map an empty returning() result to ADMIN_EXISTS.

			.onConflictDoNothing({ target: playtestAdminsTable.email })
			.returning();

		// If no row is returned, throw errors.ADMIN_EXISTS().
**[bug · medium]** The preflight `findAdminByEmail` check does not serialize with this insert. The schema's unique email constraint prevents duplicate rows, but concurrent invitations can still raise an unhandled unique-constraint exception instead of the declared `ADMIN_EXISTS` error. Use an atomic conflict-safe insert and map an empty `returning()` result to `ADMIN_EXISTS`. ```suggestion .onConflictDoNothing({ target: playtestAdminsTable.email }) .returning(); // If no row is returned, throw errors.ADMIN_EXISTS(). ```
sionzee marked this conversation as resolved
Lines 205-206
@ -0,0 +202,5 @@
export const acceptInvite = base
.input(
z.object({
token: z.string().min(1),
password: adminPasswordSchema,
Member

[maintainability · low] Generated invite tokens are about 43 characters, but this unauthenticated input has no upper bound. A caller can submit arbitrarily large strings to hashing and database lookup, creating avoidable CPU/memory work if request-body limits are not enforced elsewhere. Bound the token to a reasonable maximum (for example 128 characters).

			token: z.string().min(1).max(128),
			password: adminPasswordSchema,
**[maintainability · low]** Generated invite tokens are about 43 characters, but this unauthenticated input has no upper bound. A caller can submit arbitrarily large strings to hashing and database lookup, creating avoidable CPU/memory work if request-body limits are not enforced elsewhere. Bound the token to a reasonable maximum (for example 128 characters). ```suggestion token: z.string().min(1).max(128), password: adminPasswordSchema, ```
sionzee marked this conversation as resolved
Lines 244-255
@ -0,0 +241,15 @@
const totpSecret = generateTotpSecret();
await context.db
.update(playtestAdminsTable)
.set({
passwordHash: await Bun.password.hash(input.password, ADMIN_PASSWORD_HASH_OPTIONS),
totpSecretCiphertext: await encryptSecret(totpSecret, CRYPTO_DOMAIN.totpSecret),
totpEnabled: false,
// Burn the invite so the link is single-use.
inviteTokenHash: null,
inviteExpiresAt: null,
updatedAt: new Date(),
})
.where(eq(playtestAdminsTable.id, admin.id));
Member

[security · medium] The invitation is checked in a separate SELECT, then consumed with an UPDATE that only matches the administrator ID. Two concurrent requests with the same token can both pass the SELECT and overwrite the password/TOTP secret, so the documented single-use invitation is not atomic. Consume it with a conditional update that also matches the token hash, invited status, and expiry, use returning() to detect a lost race, and reject the loser.

		// Make this an atomic conditional UPDATE (token hash + invited status + expiry)
		// and throw INVALID_INVITE when returning() reports that no row was updated.
**[security · medium]** The invitation is checked in a separate SELECT, then consumed with an UPDATE that only matches the administrator ID. Two concurrent requests with the same token can both pass the SELECT and overwrite the password/TOTP secret, so the documented single-use invitation is not atomic. Consume it with a conditional update that also matches the token hash, `invited` status, and expiry, use `returning()` to detect a lost race, and reject the loser. ```suggestion // Make this an atomic conditional UPDATE (token hash + invited status + expiry) // and throw INVALID_INVITE when returning() reports that no row was updated. ```
sionzee marked this conversation as resolved
Lines 305-309
@ -0,0 +302,8 @@
.handler(async ({ input, context, errors }) => {
const meta = getRequestMeta(context.headers);
const limit = checkRateLimit(
`playtest:admin-login:${meta.ip ?? "unknown"}`,
LOGIN_RATE_LIMIT_MAX,
LOGIN_RATE_LIMIT_WINDOW_MS,
);
Member

[security · medium] This limiter is keyed solely by meta.ip, which is request metadata derived from forwarded-IP headers. If the application is reachable without a proxy that rewrites those headers, a caller can send a different forwarded address on every request and obtain a fresh bucket. Derive the limiter key from the server/trusted-proxy connection address (and use the forwarded value only for audit attribution).

		// Use a trusted connection/proxy-derived client IP for this key, not the
		// user-supplied forwarded address stored in `meta.ip`.
**[security · medium]** This limiter is keyed solely by `meta.ip`, which is request metadata derived from forwarded-IP headers. If the application is reachable without a proxy that rewrites those headers, a caller can send a different forwarded address on every request and obtain a fresh bucket. Derive the limiter key from the server/trusted-proxy connection address (and use the forwarded value only for audit attribution). ```suggestion // Use a trusted connection/proxy-derived client IP for this key, not the // user-supplied forwarded address stored in `meta.ip`. ```
sionzee marked this conversation as resolved
Lines 311-313
@ -0,0 +308,6 @@
LOGIN_RATE_LIMIT_WINDOW_MS,
);
if (!limit.allowed) {
throw errors.RATE_LIMITED({ data: { retryAfter: limit.retryAfter } });
}
Member

[security · medium] The threat-model header says every outcome, including failures, is audited, but the rate-limit branch returns before any writeAudit call. This hides repeated brute-force activity from the audit trail; record a rate-limited login outcome before throwing (and apply the same treatment to early MFA error paths).

		if (!limit.allowed) {
			await writeAudit(context.db, meta, {
				action: "admin.login.rate_limited",
				metadata: { retryAfter: limit.retryAfter },
			});
			throw errors.RATE_LIMITED({ data: { retryAfter: limit.retryAfter } });
		}
**[security · medium]** The threat-model header says every outcome, including failures, is audited, but the rate-limit branch returns before any `writeAudit` call. This hides repeated brute-force activity from the audit trail; record a rate-limited login outcome before throwing (and apply the same treatment to early MFA error paths). ```suggestion if (!limit.allowed) { await writeAudit(context.db, meta, { action: "admin.login.rate_limited", metadata: { retryAfter: limit.retryAfter }, }); throw errors.RATE_LIMITED({ data: { retryAfter: limit.retryAfter } }); } ```
sionzee marked this conversation as resolved
Lines 361-366
@ -0,0 +358,9 @@
// Password accepted - reset the failure counter but do NOT mark the session
// authenticated. That only happens after the second factor.
await context.db
.update(playtestAdminsTable)
.set({ failedLoginCount: 0, lockedUntil: null, updatedAt: new Date() })
.where(eq(playtestAdminsTable.id, admin.id));
clearRateLimit(`playtest:admin-login:${meta.ip ?? "unknown"}`);
Member

[security · high] The password step clears both the durable account lockout and the per-IP bucket before the second factor succeeds. An attacker with a valid password can submit one wrong TOTP code, call adminLogin again to reset the counter, and repeat indefinitely; confirmTotpEnrollment also does not count failed codes or apply an independent limit. Keep the failure/rate-limit state until MFA or enrollment has successfully promoted the session, and enforce the same lockout/throttling policy on both partial endpoints.

		// Do not reset the account lockout or clear the login bucket here; only
		// reset them after verifyTotpChallenge/confirmTotpEnrollment succeeds.
**[security · high]** The password step clears both the durable account lockout and the per-IP bucket before the second factor succeeds. An attacker with a valid password can submit one wrong TOTP code, call `adminLogin` again to reset the counter, and repeat indefinitely; `confirmTotpEnrollment` also does not count failed codes or apply an independent limit. Keep the failure/rate-limit state until MFA or enrollment has successfully promoted the session, and enforce the same lockout/throttling policy on both partial endpoints. ```suggestion // Do not reset the account lockout or clear the login bucket here; only // reset them after verifyTotpChallenge/confirmTotpEnrollment succeeds. ```
sionzee marked this conversation as resolved
Lines 444-454
@ -0,0 +441,14 @@
throw errors.INVALID_CODE();
}
await context.db
.update(playtestAdminsTable)
.set({
// Spending the counter makes this exact code unusable again.
totpLastUsedCounter: result.counter,
failedLoginCount: 0,
lastLoginAt: new Date(),
lastLoginIp: meta.ip,
updatedAt: new Date(),
})
.where(eq(playtestAdminsTable.id, admin.id));
Member

[security · medium] totpLastUsedCounter is validated from the administrator snapshot loaded by the middleware, then written without a compare-and-set predicate. Concurrent requests can both accept the same code before either write is visible, defeating the single-use/replay guarantee; concurrent failure updates can likewise lose increments. Consume the counter atomically (or lock and re-read the row) and reject the request when another update has already advanced it.

		// Use a row lock or a conditional update requiring lastUsedCounter to still
		// be lower than result.counter, and reject when the update affects no rows.
**[security · medium]** `totpLastUsedCounter` is validated from the administrator snapshot loaded by the middleware, then written without a compare-and-set predicate. Concurrent requests can both accept the same code before either write is visible, defeating the single-use/replay guarantee; concurrent failure updates can likewise lose increments. Consume the counter atomically (or lock and re-read the row) and reject the request when another update has already advanced it. ```suggestion // Use a row lock or a conditional update requiring lastUsedCounter to still // be lower than result.counter, and reject when the update affects no rows. ```
sionzee marked this conversation as resolved
Lines 541-544
@ -0,0 +538,7 @@
.errors({
INVALID_CODE: { message: "That recovery code is not valid" },
})
.handler(async ({ input, context, errors }) => {
const { admin, session, meta } = context as unknown as AdminSessionContext;
const candidate = await fingerprint(
Member

[security · medium] Unlike verifyTotpChallenge, this recovery path never checks isLockedOut(admin). After repeated bad recovery-code attempts (or a lockout caused by bad TOTP), a valid recovery code can still promote the session while the account is locked. Apply the lockout check before accepting any second factor, and only clear the failure state after successful authentication.

	.handler(async ({ input, context, errors }) => {
		const { admin, session, meta } = context as unknown as AdminSessionContext;

		if (isLockedOut(admin)) {
			throw new ORPCError("FORBIDDEN", { message: "Too many failed attempts. Try again later." });
		}

		const candidate = await fingerprint(
**[security · medium]** Unlike `verifyTotpChallenge`, this recovery path never checks `isLockedOut(admin)`. After repeated bad recovery-code attempts (or a lockout caused by bad TOTP), a valid recovery code can still promote the session while the account is locked. Apply the lockout check before accepting any second factor, and only clear the failure state after successful authentication. ```suggestion .handler(async ({ input, context, errors }) => { const { admin, session, meta } = context as unknown as AdminSessionContext; if (isLockedOut(admin)) { throw new ORPCError("FORBIDDEN", { message: "Too many failed attempts. Try again later." }); } const candidate = await fingerprint( ```
sionzee marked this conversation as resolved
Lines 561-572
@ -0,0 +558,15 @@
throw errors.INVALID_CODE();
}
const remaining = stored.filter((hash) => hash !== match);
await context.db
.update(playtestAdminsTable)
.set({
recoveryCodeHashes: remaining,
failedLoginCount: 0,
lastLoginAt: new Date(),
lastLoginIp: meta.ip,
updatedAt: new Date(),
})
.where(eq(playtestAdminsTable.id, admin.id));
Member

[security · medium] The recovery-code array is also read and consumed in separate operations. Two concurrent requests can both find the same hash in stored, both compute the same remaining array, and both return success even though the code is meant to be single-use. Re-read under a row lock or use a compare-and-swap update on the stored array and reject a request whose update affects no rows.

		// Consume under a row lock or compare-and-swap the original `stored` value;
		// return INVALID_CODE if another request has already removed this hash.
**[security · medium]** The recovery-code array is also read and consumed in separate operations. Two concurrent requests can both find the same hash in `stored`, both compute the same `remaining` array, and both return success even though the code is meant to be single-use. Re-read under a row lock or use a compare-and-swap update on the stored array and reject a request whose update affects no rows. ```suggestion // Consume under a row lock or compare-and-swap the original `stored` value; // return INVALID_CODE if another request has already removed this hash. ```
sionzee marked this conversation as resolved
Lines 660-661
@ -0,0 +657,5 @@
})
.where(eq(playtestAdminsTable.id, admin.id));
// Force every other device to re-authenticate with the new credential.
await revokeAllSessions(context.db, admin.id);
Member

[bug · low] revokeAllSessions revokes every live session for the administrator, including the session making this request. That contradicts the comment that only other devices should re-authenticate and causes the caller's next request to fail as logged out. Pass the current session ID through an exclusion-aware helper, or revoke other sessions directly.

		// Revoke all sessions except the current session (the helper must accept an exclusion ID).
		await revokeAllSessionsExcept(context.db, admin.id, session.id);
**[bug · low]** `revokeAllSessions` revokes every live session for the administrator, including the session making this request. That contradicts the comment that only other devices should re-authenticate and causes the caller's next request to fail as logged out. Pass the current session ID through an exclusion-aware helper, or revoke other sessions directly. ```suggestion // Revoke all sessions except the current session (the helper must accept an exclusion ID). await revokeAllSessionsExcept(context.db, admin.id, session.id); ```
sionzee marked this conversation as resolved
Lines 768-776
@ -0,0 +765,12 @@
await context.db
.update(playtestAdminsTable)
.set({
totpEnabled: false,
totpSecretCiphertext: null,
totpConfirmedAt: null,
totpLastUsedCounter: null,
recoveryCodeHashes: [],
status: "invited",
updatedAt: new Date(),
})
Member

[security · medium] Resetting TOTP unconditionally changes the target to invited, even when the target was disabled. Because the existing password hash is retained, that disabled account can log in through the enrollment branch and confirmTotpEnrollment changes it to active, unintentionally re-enabling it. Preserve disabled until a separate explicit status change enables the account.

				status: target.status === "disabled" ? "disabled" : "invited", // select target.status above
**[security · medium]** Resetting TOTP unconditionally changes the target to `invited`, even when the target was `disabled`. Because the existing password hash is retained, that disabled account can log in through the enrollment branch and `confirmTotpEnrollment` changes it to `active`, unintentionally re-enabling it. Preserve `disabled` until a separate explicit status change enables the account. ```suggestion status: target.status === "disabled" ? "disabled" : "invited", // select target.status above ```
sionzee marked this conversation as resolved
Lines 862-865
@ -0,0 +859,7 @@
.input(z.object({}).optional())
.output(z.object({ removed: z.number() }))
.handler(async ({ context }) => {
const removed = await context.db
.delete(playtestAdminSessionsTable)
.where(and(lt(playtestAdminSessionsTable.expiresAt, new Date()), isNull(playtestAdminSessionsTable.revokedAt)))
.returning({ id: playtestAdminSessionsTable.id });
Member

[bug · medium] The cleanup description says expired sessions are dropped, but this predicate excludes every revoked session. Sessions revoked by logout, password changes, disabling an account, or TOTP reset therefore remain forever and the table can grow without bound. Unless revoked rows have an explicit retention requirement, prune all sessions past expiresAt (or add a separate retention cleanup).

			.where(lt(playtestAdminSessionsTable.expiresAt, new Date()))
**[bug · medium]** The cleanup description says expired sessions are dropped, but this predicate excludes every revoked session. Sessions revoked by logout, password changes, disabling an account, or TOTP reset therefore remain forever and the table can grow without bound. Unless revoked rows have an explicit retention requirement, prune all sessions past `expiresAt` (or add a separate retention cleanup). ```suggestion .where(lt(playtestAdminSessionsTable.expiresAt, new Date())) ```
sionzee marked this conversation as resolved
Lines 29-32
@ -0,0 +26,7 @@
* Only http(s) URLs. Blocks `javascript:` and `data:` from reaching an href or
* a background-image on the branded page.
*/
const httpUrlSchema = z
.string()
.max(500)
.refine((value) => /^https?:\/\//i.test(value), "Must be an http(s) URL");
Member

[security · medium] This only checks the string prefix, so malformed values such as https:// and strings containing characters significant to an HTML/CSS embedding context are accepted. Parse the URL and require a non-empty hostname here, and still serialize it safely for the eventual href or CSS context.

const httpUrlSchema = z.string().max(500).refine((value) => {
	try {
		const url = new URL(value);
		return (url.protocol === "http:" || url.protocol === "https:") && url.hostname.length > 0;
	} catch {
		return false;
	}
}, "Must be a valid http(s) URL");
**[security · medium]** This only checks the string prefix, so malformed values such as `https://` and strings containing characters significant to an HTML/CSS embedding context are accepted. Parse the URL and require a non-empty hostname here, and still serialize it safely for the eventual `href` or CSS context. ```suggestion const httpUrlSchema = z.string().max(500).refine((value) => { try { const url = new URL(value); return (url.protocol === "http:" || url.protocol === "https:") && url.hostname.length > 0; } catch { return false; } }, "Must be a valid http(s) URL"); ```
sionzee marked this conversation as resolved
@ -0,0 +41,4 @@
const productShapeSchema = z.object({
slug: slugSchema,
name: z.string().min(1).max(255).trim(),
Member

[bug · low] Because .min(1) runs before .trim(), a whitespace-only name passes validation and is then transformed into an empty string. Validate the trimmed value so products cannot be created or updated with a blank display name.

	name: z.string().trim().min(1).max(255),
**[bug · low]** Because `.min(1)` runs before `.trim()`, a whitespace-only name passes validation and is then transformed into an empty string. Validate the trimmed value so products cannot be created or updated with a blank display name. ```suggestion name: z.string().trim().min(1).max(255), ```
sionzee marked this conversation as resolved
@ -0,0 +62,4 @@
ndaIntroText: z.string().max(20_000).nullish(),
disclaimerText: z.string().max(20_000).nullish(),
waitlistText: z.string().max(20_000).nullish(),
privacyPolicyUrl: httpUrlSchema.default("https://evobug.com/privacy"),
Member

[maintainability · low] This reusable product-admin contract embeds one deployment's business domain in the default policy URL. In another branded deployment or environment, an omitted value silently points players to the wrong privacy policy. Read the default from deployment configuration (or require the admin to provide it).

	privacyPolicyUrl: httpUrlSchema.default(config.playtestPrivacyPolicyUrl),
**[maintainability · low]** This reusable product-admin contract embeds one deployment's business domain in the default policy URL. In another branded deployment or environment, an omitted value silently points players to the wrong privacy policy. Read the default from deployment configuration (or require the admin to provide it). ```suggestion privacyPolicyUrl: httpUrlSchema.default(config.playtestPrivacyPolicyUrl), ```
sionzee marked this conversation as resolved
Lines 135-141
@ -0,0 +132,10 @@
assertConsistentProviders(input.allowedAuthProviders, input.requiredConnections);
const [existing] = await context.db
.select({ id: playtestProductsTable.id })
.from(playtestProductsTable)
.where(eq(playtestProductsTable.slug, input.slug))
.limit(1);
if (existing) throw errors.SLUG_TAKEN();
Member

[bug · medium] The preflight SELECT does not make slug creation concurrency-safe. Two administrators can both pass it; the database unique constraint then rejects one insert with a raw uniqueness error rather than the declared SLUG_TAKEN error. Use a conflict-safe insert (and map the update path's unique violation as well) instead of relying on this check.

		const [created] = await context.db
			.insert(playtestProductsTable)
			.values(productValues)
			.onConflictDoNothing({ target: playtestProductsTable.slug })
			.returning();

		if (!created) throw errors.SLUG_TAKEN();
**[bug · medium]** The preflight `SELECT` does not make slug creation concurrency-safe. Two administrators can both pass it; the database unique constraint then rejects one insert with a raw uniqueness error rather than the declared `SLUG_TAKEN` error. Use a conflict-safe insert (and map the update path's unique violation as well) instead of relying on this check. ```suggestion const [created] = await context.db .insert(playtestProductsTable) .values(productValues) .onConflictDoNothing({ target: playtestProductsTable.slug }) .returning(); if (!created) throw errors.SLUG_TAKEN(); ```
sionzee marked this conversation as resolved
Lines 161-167
@ -0,0 +158,10 @@
if (!created) throw new ORPCError("INTERNAL_ERROR");
await writeAudit(context.db, meta, {
action: "product.create",
actorAdminId: admin.id,
entityType: "playtest_product",
entityId: created.id,
metadata: { slug: created.slug, name: created.name },
});
Member

[maintainability · medium] The mutation is committed before writeAudit runs. writeAudit deliberately swallows insert failures, so an audit database failure can return a successful product change with no corresponding audit record. If product mutations require complete audit coverage, execute the mutation and audit insert through one transaction or make audit failure visible/alertable to the caller.

		// Perform the product mutation and its audit INSERT through the same
		// transaction, or explicitly surface a failed audit instead of returning success.
**[maintainability · medium]** The mutation is committed before `writeAudit` runs. `writeAudit` deliberately swallows insert failures, so an audit database failure can return a successful product change with no corresponding audit record. If product mutations require complete audit coverage, execute the mutation and audit insert through one transaction or make audit failure visible/alertable to the caller. ```suggestion // Perform the product mutation and its audit INSERT through the same // transaction, or explicitly surface a failed audit instead of returning success. ```
sionzee marked this conversation as resolved
Lines 209-213
@ -0,0 +206,8 @@
if (clash) throw errors.SLUG_TAKEN();
}
const [updated] = await context.db
.update(playtestProductsTable)
.set({ ...patch, updatedAt: new Date() })
.where(eq(playtestProductsTable.id, id))
.returning();
Member

[bug · high] status is part of the generic patch, so updateProduct({ id, status: "archived" }) bypasses the assigned-key guard in archiveProduct. This can hide a product while testers still have assigned keys, violating the archiving invariant. Reject this transition here or route it through the same guarded archive path.

		if (patch.status === "archived" && before.status !== "archived") {
			throw new ORPCError("BAD_REQUEST", {
				message: "Use archiveProduct to archive a playtest",
			});
		}

		const [updated] = await context.db
			.update(playtestProductsTable)
			.set({ ...patch, updatedAt: new Date() })
			.where(eq(playtestProductsTable.id, id))
			.returning();
**[bug · high]** `status` is part of the generic patch, so `updateProduct({ id, status: "archived" })` bypasses the assigned-key guard in `archiveProduct`. This can hide a product while testers still have assigned keys, violating the archiving invariant. Reject this transition here or route it through the same guarded archive path. ```suggestion if (patch.status === "archived" && before.status !== "archived") { throw new ORPCError("BAD_REQUEST", { message: "Use archiveProduct to archive a playtest", }); } const [updated] = await context.db .update(playtestProductsTable) .set({ ...patch, updatedAt: new Date() }) .where(eq(playtestProductsTable.id, id)) .returning(); ```
sionzee marked this conversation as resolved
Lines 265-274
@ -0,0 +262,13 @@
.orderBy(playtestProductsTable.id);
// Two grouped aggregates rather than N+1 per product.
const applicationCounts = await context.db
.select({
productId: playtestApplicationsTable.productId,
total: count(),
waitlisted: sql<number>`count(*) filter (where ${playtestApplicationsTable.status} = 'waitlisted')`.mapWith(
Number,
),
})
.from(playtestApplicationsTable)
.groupBy(playtestApplicationsTable.productId);
Member

[performance · medium] These aggregates scan and group every application and key row even when the listing excludes archived products, and the two independent queries run sequentially. As historical applications and key inventory grow, the admin list does unnecessary full-table work and adds avoidable latency. Restrict both aggregates to the returned product IDs and execute the independent queries in parallel.

		const productIds = products.map((product) => product.id);
		const [applicationCounts, keyCounts] = await Promise.all([
			// Aggregate applications with `where(inArray(productId, productIds))`.
			getApplicationCounts(productIds),
			// Aggregate keys with `where(inArray(productId, productIds))`.
			getKeyCounts(productIds),
		]);
**[performance · medium]** These aggregates scan and group every application and key row even when the listing excludes archived products, and the two independent queries run sequentially. As historical applications and key inventory grow, the admin list does unnecessary full-table work and adds avoidable latency. Restrict both aggregates to the returned product IDs and execute the independent queries in parallel. ```suggestion const productIds = products.map((product) => product.id); const [applicationCounts, keyCounts] = await Promise.all([ // Aggregate applications with `where(inArray(productId, productIds))`. getApplicationCounts(productIds), // Aggregate keys with `where(inArray(productId, productIds))`. getKeyCounts(productIds), ]); ```
sionzee marked this conversation as resolved
Lines 333-343
@ -0,0 +330,14 @@
.handler(async ({ input, context, errors }) => {
const { admin, meta } = context as unknown as AdminSessionContext;
const [assigned] = await context.db
.select({ value: count() })
.from(playtestKeysTable)
.where(and(eq(playtestKeysTable.productId, input.id), eq(playtestKeysTable.status, "assigned")));
if ((assigned?.value ?? 0) > 0) throw errors.KEYS_STILL_ASSIGNED();
await context.db
.update(playtestProductsTable)
.set({ status: "archived", updatedAt: new Date() })
.where(eq(playtestProductsTable.id, input.id));
Member

[bug · medium] The assigned-key check and the archive update are separate operations, so a concurrent assignKey can assign a key after this count but before the product is archived. The update result is also ignored, so a nonexistent product ID returns success and creates an audit entry anyway. Perform the existence check, a lock protocol shared with key assignment, the guard, and the update atomically; verify the update returned a row.

		const archived = await context.db.transaction(async (tx) => {
			// `assignKey` must acquire this same product lock before assigning.
			const [product] = await tx
				.select({ id: playtestProductsTable.id })
				.from(playtestProductsTable)
				.where(eq(playtestProductsTable.id, input.id))
				.for("update");

			if (!product) throw new ORPCError("NOT_FOUND", { message: "Playtest product not found" });

			const [assigned] = await tx
				.select({ value: count() })
				.from(playtestKeysTable)
				.where(and(eq(playtestKeysTable.productId, input.id), eq(playtestKeysTable.status, "assigned")));

			if ((assigned?.value ?? 0) > 0) throw errors.KEYS_STILL_ASSIGNED();

			const [updated] = await tx
				.update(playtestProductsTable)
				.set({ status: "archived", updatedAt: new Date() })
				.where(eq(playtestProductsTable.id, input.id))
				.returning({ id: playtestProductsTable.id });

			return updated;
		});
**[bug · medium]** The assigned-key check and the archive update are separate operations, so a concurrent `assignKey` can assign a key after this count but before the product is archived. The update result is also ignored, so a nonexistent product ID returns success and creates an audit entry anyway. Perform the existence check, a lock protocol shared with key assignment, the guard, and the update atomically; verify the update returned a row. ```suggestion const archived = await context.db.transaction(async (tx) => { // `assignKey` must acquire this same product lock before assigning. const [product] = await tx .select({ id: playtestProductsTable.id }) .from(playtestProductsTable) .where(eq(playtestProductsTable.id, input.id)) .for("update"); if (!product) throw new ORPCError("NOT_FOUND", { message: "Playtest product not found" }); const [assigned] = await tx .select({ value: count() }) .from(playtestKeysTable) .where(and(eq(playtestKeysTable.productId, input.id), eq(playtestKeysTable.status, "assigned"))); if ((assigned?.value ?? 0) > 0) throw errors.KEYS_STILL_ASSIGNED(); const [updated] = await tx .update(playtestProductsTable) .set({ status: "archived", updatedAt: new Date() }) .where(eq(playtestProductsTable.id, input.id)) .returning({ id: playtestProductsTable.id }); return updated; }); ```
sionzee marked this conversation as resolved
@ -0,0 +42,4 @@
Thanks for signing up. Places in each round are limited, so we review applications in batches and pick a spread of players and hardware that suits whatever we're testing at the time.
If you're selected, you'll get an email with the NDA. Sign it, and your key appears on this page.
Member

[other · medium] This default confirmation is returned for every playtest, including products with requireNda: false. Those applications move directly to selected and do not receive or sign an NDA, so this tells those players to complete a step that does not exist. Use wording that branches on the product's NDA requirement or stays generic.

If you're selected, you'll get an email with the next steps. Once any required paperwork is complete, your key appears on this page.
**[other · medium]** This default confirmation is returned for every playtest, including products with `requireNda: false`. Those applications move directly to `selected` and do not receive or sign an NDA, so this tells those players to complete a step that does not exist. Use wording that branches on the product's NDA requirement or stays generic. ```suggestion If you're selected, you'll get an email with the next steps. Once any required paperwork is complete, your key appears on this page. ```
sionzee marked this conversation as resolved
@ -0,0 +56,4 @@
fallback: string,
gameName: string,
): string {
return (template ?? fallback).replaceAll("{{GAME}}", gameName);
Member

[other · medium] An empty (or whitespace-only) admin override is accepted by productShapeSchema and stored as-is. Because this uses only a nullish check, saving an empty disclaimer/NDA/waitlist field suppresses the safety-critical default instead of falling back, which can leave the funnel without its required disclosure text. Treat blank overrides as missing (or reject them at validation).

const source = template?.trim() ? template : fallback;
	return source.replaceAll("{{GAME}}", gameName);
**[other · medium]** An empty (or whitespace-only) admin override is accepted by `productShapeSchema` and stored as-is. Because this uses only a nullish check, saving an empty disclaimer/NDA/waitlist field suppresses the safety-critical default instead of falling back, which can leave the funnel without its required disclosure text. Treat blank overrides as missing (or reject them at validation). ```suggestion const source = template?.trim() ? template : fallback; return source.replaceAll("{{GAME}}", gameName); ```
Member

[other · low] gameName is used as a replacement string, so JavaScript interprets $&, $`` and $'sequences in a product name as replacement patterns. Since product names are not restricted from containing$, a name such as A $& Bchanges the rendered copy. Use the callback form ofreplaceAll` to insert the name literally.

return (template ?? fallback).replaceAll("{{GAME}}", () => gameName);
**[other · low]** `gameName` is used as a replacement string, so JavaScript interprets `$&`, `$`` and `$'` sequences in a product name as replacement patterns. Since product names are not restricted from containing `$`, a name such as `A $& B` changes the rendered copy. Use the callback form of `replaceAll` to insert the name literally. ```suggestion return (template ?? fallback).replaceAll("{{GAME}}", () => gameName); ```
sionzee marked this conversation as resolved
Lines 345-348
@ -316,0 +342,7 @@
from: r.playtestApplicationsTable.productId,
to: r.playtestProductsTable.id,
}),
user: r.one.usersTable({
from: r.playtestApplicationsTable.userId,
to: r.usersTable.id,
}),
Member

[maintainability · low] The schema defines two additional foreign keys from applications to playtestAdminsTable (selectedByAdminId and rejectedByAdminId), but neither is represented here. Relational queries therefore cannot navigate from an application to the admin who selected or rejected it, and the relation model is incomplete for these audit fields. Add two distinctly aliased r.one.playtestAdminsTable relations (and inverse collections if those are queried from the admin side).

		user: r.one.usersTable({
			from: r.playtestApplicationsTable.userId,
			to: r.usersTable.id,
		}),
		selectedBy: r.one.playtestAdminsTable({
			from: r.playtestApplicationsTable.selectedByAdminId,
			to: r.playtestAdminsTable.id,
			alias: "playtest_application_selector_relation",
		}),
		rejectedBy: r.one.playtestAdminsTable({
			from: r.playtestApplicationsTable.rejectedByAdminId,
			to: r.playtestAdminsTable.id,
			alias: "playtest_application_rejector_relation",
		}),
**[maintainability · low]** The schema defines two additional foreign keys from applications to `playtestAdminsTable` (`selectedByAdminId` and `rejectedByAdminId`), but neither is represented here. Relational queries therefore cannot navigate from an application to the admin who selected or rejected it, and the relation model is incomplete for these audit fields. Add two distinctly aliased `r.one.playtestAdminsTable` relations (and inverse collections if those are queried from the admin side). ```suggestion user: r.one.usersTable({ from: r.playtestApplicationsTable.userId, to: r.usersTable.id, }), selectedBy: r.one.playtestAdminsTable({ from: r.playtestApplicationsTable.selectedByAdminId, to: r.playtestAdminsTable.id, alias: "playtest_application_selector_relation", }), rejectedBy: r.one.playtestAdminsTable({ from: r.playtestApplicationsTable.rejectedByAdminId, to: r.playtestAdminsTable.id, alias: "playtest_application_rejector_relation", }), ```
sionzee marked this conversation as resolved
Lines 373-376
@ -316,0 +370,7 @@
from: r.playtestKeysTable.assignedUserId,
to: r.usersTable.id,
}),
application: r.one.playtestApplicationsTable({
from: r.playtestKeysTable.assignedApplicationId,
to: r.playtestApplicationsTable.id,
}),
Member

[maintainability · low] playtestKeysTable has an uploadedByAdminId foreign key to playtestAdminsTable, but no corresponding relation is exposed. This prevents relational queries from retrieving the administrator responsible for importing a key and leaves one of the key-vault audit links unmapped. Add a named r.one.playtestAdminsTable relation (and an inverse admin collection if needed).

		application: r.one.playtestApplicationsTable({
			from: r.playtestKeysTable.assignedApplicationId,
			to: r.playtestApplicationsTable.id,
		}),
		uploadedBy: r.one.playtestAdminsTable({
			from: r.playtestKeysTable.uploadedByAdminId,
			to: r.playtestAdminsTable.id,
		}),
**[maintainability · low]** `playtestKeysTable` has an `uploadedByAdminId` foreign key to `playtestAdminsTable`, but no corresponding relation is exposed. This prevents relational queries from retrieving the administrator responsible for importing a key and leaves one of the key-vault audit links unmapped. Add a named `r.one.playtestAdminsTable` relation (and an inverse admin collection if needed). ```suggestion application: r.one.playtestApplicationsTable({ from: r.playtestKeysTable.assignedApplicationId, to: r.playtestApplicationsTable.id, }), uploadedBy: r.one.playtestAdminsTable({ from: r.playtestKeysTable.uploadedByAdminId, to: r.playtestAdminsTable.id, }), ```
sionzee marked this conversation as resolved
@ -0,0 +1,73 @@
/**
* Fixed-window in-memory rate limiter.
Member

[other · low] The implementation removes each timestamp independently and calculates retry time from the oldest hit, which is sliding-window behavior rather than fixed-window behavior. This mismatch can cause callers or tests to assume that all hits reset at one common boundary. Rename the description (or change the algorithm) to make the contract accurate.

 * Sliding-window in-memory rate limiter.
**[other · low]** The implementation removes each timestamp independently and calculates retry time from the oldest hit, which is sliding-window behavior rather than fixed-window behavior. This mismatch can cause callers or tests to assume that all hits reset at one common boundary. Rename the description (or change the algorithm) to make the contract accurate. ```suggestion * Sliding-window in-memory rate limiter. ```
sionzee marked this conversation as resolved
@ -0,0 +12,4 @@
hits: number[];
}
const buckets = new Map<string, Bucket>();
Member

[security · high] The process-global map retains one entry for every distinct caller-supplied key until the sweeper runs, with no bound on key count or on the number of hits stored for a key. A public caller that derives keys from request data can force many unique buckets and make both memory use and the periodic scan grow without limit. Add a bounded cache/insertion policy (and bound per-bucket hits), and ensure public callers use a normalized, trusted client identity rather than arbitrary request values.

const MAX_BUCKETS = 10_000;
const buckets = new Map<string, Bucket>();
**[security · high]** The process-global map retains one entry for every distinct caller-supplied key until the sweeper runs, with no bound on key count or on the number of hits stored for a key. A public caller that derives keys from request data can force many unique buckets and make both memory use and the periodic scan grow without limit. Add a bounded cache/insertion policy (and bound per-bucket hits), and ensure public callers use a normalized, trusted client identity rather than arbitrary request values. ```suggestion const MAX_BUCKETS = 10_000; const buckets = new Map<string, Bucket>(); ```
sionzee marked this conversation as resolved
@ -0,0 +24,4 @@
/**
* Record an attempt against `key` and report whether it may proceed.
*/
export function checkRateLimit(key: string, max: number, windowMs: number): RateLimitResult {
Member

[security · low] Because this exported helper accepts runtime max and windowMs values without validation, a non-positive windowMs makes every previous hit expire immediately and effectively disables throttling. Invalid max values can likewise produce permanent denial or nonsensical remaining values. Validate positive finite parameters and fail closed before mutating the bucket.

export function checkRateLimit(key: string, max: number, windowMs: number): RateLimitResult {
	if (!Number.isSafeInteger(max) || max < 1) throw new RangeError("max must be a positive integer");
	if (!Number.isFinite(windowMs) || windowMs <= 0) throw new RangeError("windowMs must be positive");
**[security · low]** Because this exported helper accepts runtime `max` and `windowMs` values without validation, a non-positive `windowMs` makes every previous hit expire immediately and effectively disables throttling. Invalid `max` values can likewise produce permanent denial or nonsensical `remaining` values. Validate positive finite parameters and fail closed before mutating the bucket. ```suggestion export function checkRateLimit(key: string, max: number, windowMs: number): RateLimitResult { if (!Number.isSafeInteger(max) || max < 1) throw new RangeError("max must be a positive integer"); if (!Number.isFinite(windowMs) || windowMs <= 0) throw new RangeError("windowMs must be positive"); ```
sionzee marked this conversation as resolved
@ -0,0 +64,4 @@
const now = Date.now();
for (const [key, bucket] of buckets) {
// One hour is longer than any window used here, so anything older is dead.
if (bucket.hits.every((t) => now - t > 60 * 60 * 1000)) {
Member

[maintainability · medium] This cleanup threshold is hard-coded independently of the windowMs supplied to checkRateLimit. Any caller using a window longer than one hour will have its still-active bucket removed early and can exceed its configured limit; shorter windows also retain stale keys unnecessarily. Store the effective window/expiry with each bucket and use that value when sweeping.

		if (bucket.hits.every((t) => now - t > bucket.windowMs)) {
**[maintainability · medium]** This cleanup threshold is hard-coded independently of the `windowMs` supplied to `checkRateLimit`. Any caller using a window longer than one hour will have its still-active bucket removed early and can exceed its configured limit; shorter windows also retain stale keys unnecessarily. Store the effective window/expiry with each bucket and use that value when sweeping. ```suggestion if (bucket.hits.every((t) => now - t > bucket.windowMs)) { ```
sionzee marked this conversation as resolved
@ -0,0 +50,4 @@
export function base32Decode(input: string): Uint8Array {
// Users retyping a secret tend to include spaces and padding; both are noise.
const normalized = input.replace(/[\s=]/g, "").toUpperCase();
Member

[security · medium] Removing every = accepts padding in the middle of a secret, and the decoder silently discards non-zero trailing bits. Thus malformed input can decode as a different HMAC key instead of being rejected. The decoder also allows an empty or very short result, which the TOTP path should reject as a weak/malformed RFC 4226 secret and handle without allowing crypto.subtle.importKey errors to escape.

	const normalized = input.replace(/\s/g, "").toUpperCase();
	if (!/^[A-Z2-7]*={0,6}$/.test(normalized)) throw new Error("Invalid base32 padding");
**[security · medium]** Removing every `=` accepts padding in the middle of a secret, and the decoder silently discards non-zero trailing bits. Thus malformed input can decode as a different HMAC key instead of being rejected. The decoder also allows an empty or very short result, which the TOTP path should reject as a weak/malformed RFC 4226 secret and handle without allowing `crypto.subtle.importKey` errors to escape. ```suggestion const normalized = input.replace(/\s/g, "").toUpperCase(); if (!/^[A-Z2-7]*={0,6}$/.test(normalized)) throw new Error("Invalid base32 padding"); ```
sionzee marked this conversation as resolved
Lines 156-160
@ -0,0 +153,8 @@
/**
* Check a submitted code, scanning the drift window and rejecting replays.
*/
export async function verifyTotp(
secret: string,
token: string,
options: TotpVerifyOptions = {},
): Promise<TotpVerifyResult> {
Member

[security · high] This exported verifier performs no attempt throttling. Any network-facing caller that invokes it without both per-account and per-IP limits can search the six-digit token space online; the same protection is needed for enrollment and recovery attempts. Keep this low-level function pure if desired, but enforce rate limiting in every network-facing wrapper before calling it.

	// Network-facing callers must apply per-account and per-IP rate limiting before invoking this verifier.
**[security · high]** This exported verifier performs no attempt throttling. Any network-facing caller that invokes it without both per-account and per-IP limits can search the six-digit token space online; the same protection is needed for enrollment and recovery attempts. Keep this low-level function pure if desired, but enforce rate limiting in every network-facing wrapper before calling it. ```suggestion // Network-facing callers must apply per-account and per-IP rate limiting before invoking this verifier. ```
sionzee marked this conversation as resolved
Lines 161-167
@ -0,0 +158,10 @@
token: string,
options: TotpVerifyOptions = {},
): Promise<TotpVerifyResult> {
const {
digits = DEFAULT_DIGITS,
period = DEFAULT_PERIOD_SECONDS,
window = DEFAULT_WINDOW,
atMs = Date.now(),
lastUsedCounter = null,
} = options;
Member

[security · medium] digits, period, and window are accepted without runtime validation. A zero/non-finite period can make totpCounter produce a value that BigInt cannot convert, malformed digits can make the dynamic RegExp throw, and an unbounded window can force an unbounded number of HMAC operations. Validate integer ranges (and cap the window) before using these values in both generation and verification.

	if (!Number.isSafeInteger(digits) || digits < 1 || digits > 10) throw new RangeError("digits out of range");
	if (!Number.isSafeInteger(period) || period <= 0) throw new RangeError("period out of range");
	if (!Number.isSafeInteger(window) || window < 0 || window > 10) throw new RangeError("window out of range");
**[security · medium]** `digits`, `period`, and `window` are accepted without runtime validation. A zero/non-finite period can make `totpCounter` produce a value that `BigInt` cannot convert, malformed `digits` can make the dynamic `RegExp` throw, and an unbounded window can force an unbounded number of HMAC operations. Validate integer ranges (and cap the window) before using these values in both generation and verification. ```suggestion if (!Number.isSafeInteger(digits) || digits < 1 || digits > 10) throw new RangeError("digits out of range"); if (!Number.isSafeInteger(period) || period <= 0) throw new RangeError("period out of range"); if (!Number.isSafeInteger(window) || window < 0 || window > 10) throw new RangeError("window out of range"); ```
sionzee marked this conversation as resolved
Lines 187-188
@ -0,0 +184,5 @@
for (let offset = -window; offset <= window; offset++) {
const counter = current + offset;
// Replay guard - never accept a step at or below one already spent.
if (lastUsedCounter !== null && counter <= lastUsedCounter) continue;
Member

[security · high] This replay check only compares against a caller-provided snapshot and returns the matched counter; it never atomically consumes that counter. Two concurrent calls with the same lastUsedCounter can both return valid: true before either caller persists the new value, so the documented one-use guarantee is not enforced. Require a compare-and-swap/transactional update at the persistence boundary and authenticate only when that update succeeds, or make counter consumption part of this API.

		// The persistence layer must atomically advance its stored counter to `counter`
		// before treating this match as authenticated.
**[security · high]** This replay check only compares against a caller-provided snapshot and returns the matched counter; it never atomically consumes that counter. Two concurrent calls with the same `lastUsedCounter` can both return `valid: true` before either caller persists the new value, so the documented one-use guarantee is not enforced. Require a compare-and-swap/transactional update at the persistence boundary and authenticate only when that update succeeds, or make counter consumption part of this API. ```suggestion // The persistence layer must atomically advance its stored counter to `counter` // before treating this match as authenticated. ```
sionzee marked this conversation as resolved
Lines 241-242
@ -0,0 +238,5 @@
* One-time recovery codes for when an admin loses their authenticator.
*
* Formatted `XXXX-XXXX-XXXX` from an unambiguous alphabet so they survive being
* written on paper. ~62 bits of entropy each; they are stored only as keyed
* fingerprints, never in clear.
Member

[documentation · low] The recovery-code alphabet has 32 symbols and each code contains 12 symbols, so the entropy is 32^12 = 2^60, not approximately 62 bits. The inaccurate claim can mislead security sizing and operational documentation.

 * written on paper. ~60 bits of entropy each; they are stored only as keyed
 * fingerprints, never in clear.
**[documentation · low]** The recovery-code alphabet has 32 symbols and each code contains 12 symbols, so the entropy is 32^12 = 2^60, not approximately 62 bits. The inaccurate claim can mislead security sizing and operational documentation. ```suggestion * written on paper. ~60 bits of entropy each; they are stored only as keyed * fingerprints, never in clear. ```
sionzee marked this conversation as resolved
fix(playtest): address PR review findings
Some checks failed
Tests / test (pull_request) Failing after 53s
5defbba396
All 40 findings were checked against the code; 21 were reproduced by
running it. Fixes below, with regression tests in review-fixes.spec.ts.

Security:

  - The second factor was brute-forceable. adminLogin cleared both the
    persisted lockout and the rate-limit buckets after the password
    check, so anyone holding a password could loop login -> guess ->
    login and reset both counters indefinitely. Verified: 12 consecutive
    wrong codes with lockedUntil still null. Those are now cleared only
    once MFA has actually promoted the session, enrolment counts its
    failures too, and login charges a per-account bucket alongside the
    spoofable per-IP one.
  - resetAdminTotp moved a disabled admin to "invited", letting it log
    in via the enrolment branch, which ends by setting "active" -
    silently undoing a disable. Verified end to end. Disabled now stays
    disabled.
  - verifyRecoveryCode never checked the lockout, so a locked account
    could still be promoted with a recovery code.
  - The TOTP counter and the recovery-code array were read from a
    middleware snapshot and written unconditionally. Both are now
    compare-and-set, so a concurrent request cannot spend the same code
    twice.
  - Invitations are consumed with a conditional update on token hash and
    status, so racing the same link cannot set two passwords.
  - The bootstrap script's generated password had ~32 bits, not the
    "strong" it advertised; it is now 20 random bytes. It also refuses
    to print credentials to a non-interactive stream, rejects a
    password passed as an argv flag, and validates its input.

Correctness:

  - updateProduct accepted status:"archived", routing straight past
    archiveProduct's assigned-key guard.
  - archiveProduct reported success for a nonexistent id and raced key
    assignment; it now locks the product row and verifies the update.
  - changeAdminPassword revoked the caller's own session, contradicting
    its comment. It now excludes the current session.
  - pruneAdminSessions skipped revoked rows, so logout/disable/reset
    left sessions behind permanently.
  - A blank admin override erased safety-critical copy, because "" beat
    the ?? fallback. Blank now falls back.
  - renderCopy used a replacement string, so a product name containing
    $& re-inserted the {{GAME}} placeholder. Now uses the callback form.
  - The waitlist default promised an NDA even when requireNda is false.
  - .min(1) ran before .trim(), so a whitespace-only product name
    became "".
  - Slug and admin-email creation now let the unique constraint decide
    instead of a preflight SELECT that cannot serialise with the insert.
  - The URL check matched a prefix, accepting "https://" with no host.
  - base32 stripped "=" anywhere and ignored non-zero trailing bits, so
    malformed input could decode to a different HMAC key.
  - TOTP options and rate-limiter parameters are validated; a
    non-positive window silently disabled throttling.
  - The limiter is documented as sliding-window, which is what it is,
    bounds its key count, and sweeps each bucket against its own window
    rather than a fixed hour.
  - Added the three missing admin relations, scoped the product-list
    aggregates to the listed ids and ran them concurrently, and made the
    default privacy policy URL configurable.

Not changed, deliberately: audit writes stay outside the mutation
transaction. writeAudit cannot fail a security action that already
succeeded, and making product changes roll back on an audit-insert
failure trades a worse property for a better-looking one.

696 pass with and without .env.
sionzee left a comment

Went through all 40 findings and verified each against the code - 21 reproduced by running it, none were false positives. Fixes are in 5defbba, with regression tests in review-fixes.spec.ts. Replies inline; two are answered as deliberate trade-offs rather than changes.

Went through all 40 findings and verified each against the code - 21 reproduced by running it, none were false positives. Fixes are in `5defbba`, with regression tests in `review-fixes.spec.ts`. Replies inline; two are answered as deliberate trade-offs rather than changes.
@ -0,0 +57,4 @@
"drifter", "phantom", "airlock", "salvage", "beacon", "nebula", "gantry", "thruster",
];
const picked = Array.from(
Author
Owner

Confirmed and fixed - I calculated it as exactly 32 bits, so the "strong" in the docstring was simply wrong. Replaced with 20 random bytes (~132 bits) rendered base64url.

Confirmed and fixed - I calculated it as exactly 32 bits, so the "strong" in the docstring was simply wrong. Replaced with 20 random bytes (~132 bits) rendered base64url.
sionzee marked this conversation as resolved
@ -0,0 +67,4 @@
}
const args = parseArgs(process.argv.slice(2));
const email = args.email?.trim().toLowerCase();
Author
Owner

Fixed. The parser now rejects unknown flags and flags with a missing value instead of coercing them to "true", and both fields are validated before any database work - email through a zod email schema, display name with control characters refused because it is echoed to a terminal.

Fixed. The parser now rejects unknown flags and flags with a missing value instead of coercing them to `"true"`, and both fields are validated before any database work - email through a zod email schema, display name with control characters refused because it is echoed to a terminal.
sionzee marked this conversation as resolved
@ -0,0 +87,4 @@
process.exit(1);
}
const password = args.password ?? process.env.PLAYTEST_ADMIN_PASSWORD ?? generatePassword();
Author
Owner

Fixed. --password is rejected outright now; the flag parser only accepts --email and --name. A password can still be supplied out of band via PLAYTEST_ADMIN_PASSWORD, which is not visible in the process list or shell history.

Fixed. `--password` is rejected outright now; the flag parser only accepts `--email` and `--name`. A password can still be supplied out of band via `PLAYTEST_ADMIN_PASSWORD`, which is not visible in the process list or shell history.
sionzee marked this conversation as resolved
@ -0,0 +95,4 @@
process.exit(1);
}
const client = new Bun.SQL(process.env.DATABASE_URL);
Author
Owner

Fixed. The database work is wrapped in try/finally so the client is always closed, and an unexpected failure exits non-zero with a message instead of an uncaught rejection.

Fixed. The database work is wrapped in `try/finally` so the client is always closed, and an unexpected failure exits non-zero with a message instead of an uncaught rejection.
sionzee marked this conversation as resolved
@ -0,0 +98,4 @@
const client = new Bun.SQL(process.env.DATABASE_URL);
const db = drizzle({ client, relations });
const [existing] = await db
Author
Owner

Fixed. The SELECT is gone; the insert uses onConflictDoNothing on the email target and an empty returning() produces the duplicate-admin message.

Fixed. The SELECT is gone; the insert uses `onConflictDoNothing` on the email target and an empty `returning()` produces the duplicate-admin message.
sionzee marked this conversation as resolved
@ -0,0 +133,4 @@
const line = "─".repeat(66);
console.log(`
Author
Owner

Partly fixed. Some channel has to deliver the first credential set, so the output stays, but it now refuses to run when stdout is not a TTY unless PLAYTEST_ALLOW_INSECURE_OUTPUT=1 is set explicitly. That covers the CI/redirection/supervisor cases you named while keeping the interactive bootstrap usable, and the footer tells the operator to clear their scrollback.

Partly fixed. Some channel has to deliver the first credential set, so the output stays, but it now refuses to run when stdout is not a TTY unless `PLAYTEST_ALLOW_INSECURE_OUTPUT=1` is set explicitly. That covers the CI/redirection/supervisor cases you named while keeping the interactive bootstrap usable, and the footer tells the operator to clear their scrollback.
sionzee marked this conversation as resolved
@ -0,0 +156,4 @@
const inviteToken = generateToken(32);
const expiresAt = new Date(Date.now() + ADMIN_INVITE_TTL_MS);
const [created] = await context.db
Author
Owner

Fixed. The preflight SELECT is removed and the insert uses onConflictDoNothing on the email target, with an empty result mapped to ADMIN_EXISTS. Covered by a test asserting the declared error rather than a raw constraint violation.

Fixed. The preflight SELECT is removed and the insert uses `onConflictDoNothing` on the email target, with an empty result mapped to `ADMIN_EXISTS`. Covered by a test asserting the declared error rather than a raw constraint violation.
sionzee marked this conversation as resolved
@ -0,0 +202,4 @@
export const acceptInvite = base
.input(
z.object({
token: z.string().min(1),
Author
Owner

Fixed - bounded to max(128), with a test that a 500-character token is rejected before it reaches hashing or the lookup.

Fixed - bounded to `max(128)`, with a test that a 500-character token is rejected before it reaches hashing or the lookup.
sionzee marked this conversation as resolved
@ -0,0 +241,4 @@
const totpSecret = generateTotpSecret();
await context.db
Author
Owner

Fixed. The consuming UPDATE now also matches the token hash and invited status, and a lost race (no rows returned) throws INVALID_INVITE.

Fixed. The consuming UPDATE now also matches the token hash and `invited` status, and a lost race (no rows returned) throws `INVALID_INVITE`.
sionzee marked this conversation as resolved
@ -0,0 +302,4 @@
.handler(async ({ input, context, errors }) => {
const meta = getRequestMeta(context.headers);
const limit = checkRateLimit(
Author
Owner

Correct - meta.ip comes from forwarded headers and I had already documented it as untrustworthy for authorisation, so keying a control on it was inconsistent. Login now charges a per-account bucket as well, which cannot be rotated. Deriving a trusted peer address needs proxy configuration this layer does not have, so the durable control remains the persisted per-account lockout; the IP bucket is explicitly labelled best-effort.

Correct - `meta.ip` comes from forwarded headers and I had already documented it as untrustworthy for authorisation, so keying a control on it was inconsistent. Login now charges a per-account bucket as well, which cannot be rotated. Deriving a trusted peer address needs proxy configuration this layer does not have, so the durable control remains the persisted per-account lockout; the IP bucket is explicitly labelled best-effort.
sionzee marked this conversation as resolved
@ -0,0 +308,4 @@
LOGIN_RATE_LIMIT_WINDOW_MS,
);
if (!limit.allowed) {
Author
Owner

Fixed - a rate-limited login now writes an admin.login.rate_limited entry before throwing, so brute-force pressure is visible in the audit trail rather than silently dropped.

Fixed - a rate-limited login now writes an `admin.login.rate_limited` entry before throwing, so brute-force pressure is visible in the audit trail rather than silently dropped.
sionzee marked this conversation as resolved
@ -0,0 +358,4 @@
// Password accepted - reset the failure counter but do NOT mark the session
// authenticated. That only happens after the second factor.
await context.db
Author
Owner

The most serious of the 40, and correct. I reproduced it: 12 consecutive wrong TOTP codes accepted with lockedUntil still null, because each adminLogin reset the counter and clearRateLimit emptied the IP bucket. Both resets moved into onAuthenticationComplete, called only after MFA or enrolment actually promotes the session. Enrolment now counts its failures too, and login additionally charges a per-account bucket. Two regression tests.

The most serious of the 40, and correct. I reproduced it: **12 consecutive wrong TOTP codes accepted with `lockedUntil` still null**, because each `adminLogin` reset the counter and `clearRateLimit` emptied the IP bucket. Both resets moved into `onAuthenticationComplete`, called only after MFA or enrolment actually promotes the session. Enrolment now counts its failures too, and login additionally charges a per-account bucket. Two regression tests.
sionzee marked this conversation as resolved
@ -0,0 +441,4 @@
throw errors.INVALID_CODE();
}
await context.db
Author
Owner

Fixed with a compare-and-set: the update predicate requires the stored counter to still equal the snapshot value (or still be NULL), and no rows updated means another request spent the step first, which is rejected as an invalid code.

Fixed with a compare-and-set: the update predicate requires the stored counter to still equal the snapshot value (or still be NULL), and no rows updated means another request spent the step first, which is rejected as an invalid code.
sionzee marked this conversation as resolved
@ -0,0 +538,4 @@
.errors({
INVALID_CODE: { message: "That recovery code is not valid" },
})
.handler(async ({ input, context, errors }) => {
Author
Owner

Confirmed and fixed. isLockedOut is now checked before the code is examined, and the same check was added to confirmTotpEnrollment, which had the same gap. Regression test included.

Confirmed and fixed. `isLockedOut` is now checked before the code is examined, and the same check was added to `confirmTotpEnrollment`, which had the same gap. Regression test included.
sionzee marked this conversation as resolved
@ -0,0 +558,4 @@
throw errors.INVALID_CODE();
}
const remaining = stored.filter((hash) => hash !== match);
Author
Owner

Fixed with a compare-and-swap: the update now requires the stored recoveryCodeHashes to still equal the snapshot it was read from, and an empty returning() is treated as an invalid code. A concurrent request therefore cannot spend the same code twice.

Fixed with a compare-and-swap: the update now requires the stored `recoveryCodeHashes` to still equal the snapshot it was read from, and an empty `returning()` is treated as an invalid code. A concurrent request therefore cannot spend the same code twice.
sionzee marked this conversation as resolved
@ -0,0 +657,4 @@
})
.where(eq(playtestAdminsTable.id, admin.id));
// Force every other device to re-authenticate with the new credential.
Author
Owner

Fixed. revokeAllSessions takes an optional exceptSessionId, and the password change passes the current session, which is what the comment always claimed. Two tests: the caller stays signed in, other devices do not.

Fixed. `revokeAllSessions` takes an optional `exceptSessionId`, and the password change passes the current session, which is what the comment always claimed. Two tests: the caller stays signed in, other devices do not.
sionzee marked this conversation as resolved
@ -0,0 +765,4 @@
await context.db
.update(playtestAdminsTable)
.set({
Author
Owner

Confirmed by reproducing the full path: disable, reset, then log in successfully. Fixed - the status is preserved when the target is disabled, so re-enabling now requires an explicit status change. Regression test asserts both the stored status and that login is still refused.

Confirmed by reproducing the full path: disable, reset, then log in successfully. Fixed - the status is preserved when the target is `disabled`, so re-enabling now requires an explicit status change. Regression test asserts both the stored status and that login is still refused.
sionzee marked this conversation as resolved
@ -0,0 +859,4 @@
.input(z.object({}).optional())
.output(z.object({ removed: z.number() }))
.handler(async ({ context }) => {
const removed = await context.db
Author
Owner

Fixed - the isNull(revokedAt) predicate is removed, so every session past expiresAt is pruned. Revoked rows have no retention requirement here; the audit log is the durable record.

Fixed - the `isNull(revokedAt)` predicate is removed, so every session past `expiresAt` is pruned. Revoked rows have no retention requirement here; the audit log is the durable record.
sionzee marked this conversation as resolved
@ -0,0 +26,4 @@
* Only http(s) URLs. Blocks `javascript:` and `data:` from reaching an href or
* a background-image on the branded page.
*/
const httpUrlSchema = z
Author
Owner

Confirmed - "https://" passed. Replaced with new URL() parsing plus explicit protocol and non-empty hostname checks. Test added.

Confirmed - `"https://"` passed. Replaced with `new URL()` parsing plus explicit protocol and non-empty hostname checks. Test added.
sionzee marked this conversation as resolved
@ -0,0 +41,4 @@
const productShapeSchema = z.object({
slug: slugSchema,
name: z.string().min(1).max(255).trim(),
Author
Owner

Confirmed - I checked the schema directly and " " parsed successfully to "". Reordered to .trim().min(1), with a test.

Confirmed - I checked the schema directly and `" "` parsed successfully to `""`. Reordered to `.trim().min(1)`, with a test.
sionzee marked this conversation as resolved
@ -0,0 +62,4 @@
ndaIntroText: z.string().max(20_000).nullish(),
disclaimerText: z.string().max(20_000).nullish(),
waitlistText: z.string().max(20_000).nullish(),
privacyPolicyUrl: httpUrlSchema.default("https://evobug.com/privacy"),
Author
Owner

Fixed - the default now reads PLAYTEST_PRIVACY_POLICY_URL and falls back to the Evobug URL only when unset.

Fixed - the default now reads `PLAYTEST_PRIVACY_POLICY_URL` and falls back to the Evobug URL only when unset.
sionzee marked this conversation as resolved
@ -0,0 +132,4 @@
assertConsistentProviders(input.allowedAuthProviders, input.requiredConnections);
const [existing] = await context.db
Author
Owner

Fixed - onConflictDoNothing on the slug target with an empty result mapped to SLUG_TAKEN, so the constraint decides rather than a check that cannot serialise with the insert.

Fixed - `onConflictDoNothing` on the slug target with an empty result mapped to `SLUG_TAKEN`, so the constraint decides rather than a check that cannot serialise with the insert.
sionzee marked this conversation as resolved
@ -0,0 +158,4 @@
if (!created) throw new ORPCError("INTERNAL_ERROR");
await writeAudit(context.db, meta, {
Author
Owner

Acknowledged, deliberately not changed. writeAudit swallows failures so an audit write can never fail a security action that already succeeded, nor mask the real error of one that did not - I would rather keep that than make a product change roll back because the audit insert failed. Failures are logged loudly; if you want alerting on that, it belongs at the logging layer.

Acknowledged, deliberately not changed. `writeAudit` swallows failures so an audit write can never fail a security action that already succeeded, nor mask the real error of one that did not - I would rather keep that than make a product change roll back because the audit insert failed. Failures are logged loudly; if you want alerting on that, it belongs at the logging layer.
sionzee marked this conversation as resolved
@ -0,0 +206,4 @@
if (clash) throw errors.SLUG_TAKEN();
}
const [updated] = await context.db
Author
Owner

Confirmed by reproducing it: updateProduct({id, status:"archived"}) returned an archived product with no key check. Fixed - that transition is rejected with a pointer to the archive action, so the guard cannot be routed around. Regression test included.

Confirmed by reproducing it: `updateProduct({id, status:"archived"})` returned an archived product with no key check. Fixed - that transition is rejected with a pointer to the archive action, so the guard cannot be routed around. Regression test included.
sionzee marked this conversation as resolved
@ -0,0 +262,4 @@
.orderBy(playtestProductsTable.id);
// Two grouped aggregates rather than N+1 per product.
const applicationCounts = await context.db
Author
Owner

Fixed - both aggregates are now restricted with inArray(productId, productIds) and run through Promise.all, with an early return when the list is empty.

Fixed - both aggregates are now restricted with `inArray(productId, productIds)` and run through `Promise.all`, with an early return when the list is empty.
sionzee marked this conversation as resolved
@ -0,0 +330,4 @@
.handler(async ({ input, context, errors }) => {
const { admin, meta } = context as unknown as AdminSessionContext;
const [assigned] = await context.db
Author
Owner

Fixed. The check and update run in one transaction that first takes FOR UPDATE on the product row, which serialises against assignKey, and a missing product now throws NOT_FOUND instead of reporting success - I confirmed the old code returned success: true for id 999999 and wrote an audit entry.

Fixed. The check and update run in one transaction that first takes `FOR UPDATE` on the product row, which serialises against `assignKey`, and a missing product now throws `NOT_FOUND` instead of reporting success - I confirmed the old code returned `success: true` for id 999999 and wrote an audit entry.
sionzee marked this conversation as resolved
@ -0,0 +42,4 @@
Thanks for signing up. Places in each round are limited, so we review applications in batches and pick a spread of players and hardware that suits whatever we're testing at the time.
If you're selected, you'll get an email with the NDA. Sign it, and your key appears on this page.
Author
Owner

Good catch - the copy contradicted the funnel for requireNda: false, where applications go straight to selected. Reworded to "an email with the next steps... once anything we need from you is done", which is true either way.

Good catch - the copy contradicted the funnel for `requireNda: false`, where applications go straight to `selected`. Reworded to "an email with the next steps... once anything we need from you is done", which is true either way.
sionzee marked this conversation as resolved
@ -0,0 +56,4 @@
fallback: string,
gameName: string,
): string {
return (template ?? fallback).replaceAll("{{GAME}}", gameName);
Author
Owner

Confirmed - renderCopy("", fallback, ...) returned "", so clearing the field in the admin UI would have removed the pre-alpha warning entirely. Now a blank or whitespace-only override falls back to the default. Test covers both.

Confirmed - `renderCopy("", fallback, ...)` returned `""`, so clearing the field in the admin UI would have removed the pre-alpha warning entirely. Now a blank or whitespace-only override falls back to the default. Test covers both.
Author
Owner

Confirmed, and worse than described: rendering a name of A $& B produced Welcome to A {{GAME}} B!, because $& re-inserts the matched placeholder. Switched to the callback form of replaceAll, with a test covering $&, $` and $'.

Confirmed, and worse than described: rendering a name of `A $& B` produced `Welcome to A {{GAME}} B!`, because `$&` re-inserts the matched placeholder. Switched to the callback form of `replaceAll`, with a test covering `$&`, `` $` `` and `$'`.
sionzee marked this conversation as resolved
@ -316,0 +342,4 @@
from: r.playtestApplicationsTable.productId,
to: r.playtestProductsTable.id,
}),
user: r.one.usersTable({
Author
Owner

Fixed - added selectedBy and rejectedBy as distinctly aliased r.one.playtestAdminsTable relations.

Fixed - added `selectedBy` and `rejectedBy` as distinctly aliased `r.one.playtestAdminsTable` relations.
sionzee marked this conversation as resolved
@ -316,0 +370,4 @@
from: r.playtestKeysTable.assignedUserId,
to: r.usersTable.id,
}),
application: r.one.playtestApplicationsTable({
Author
Owner

Fixed - added an aliased uploadedBy relation so the key-vault audit link is navigable.

Fixed - added an aliased `uploadedBy` relation so the key-vault audit link is navigable.
sionzee marked this conversation as resolved
@ -0,0 +1,73 @@
/**
* Fixed-window in-memory rate limiter.
Author
Owner

Correct - it is a sliding window and the docstring said fixed. Renamed, and noted why sliding is the behaviour we want here (no burst-across-the-boundary).

Correct - it is a sliding window and the docstring said fixed. Renamed, and noted why sliding is the behaviour we want here (no burst-across-the-boundary).
sionzee marked this conversation as resolved
@ -0,0 +12,4 @@
hits: number[];
}
const buckets = new Map<string, Bucket>();
Author
Owner

Fixed - the map is capped at 10,000 keys with least-recently-used eviction, and per-bucket hits are capped at 1,000. Worth stating the residual: an attacker who can vary the key can still evict their own bucket, which is why the durable control for admin login is the persisted per-account lockout rather than this map. Login now also charges a per-account key that cannot be rotated.

Fixed - the map is capped at 10,000 keys with least-recently-used eviction, and per-bucket hits are capped at 1,000. Worth stating the residual: an attacker who can vary the key can still evict their own bucket, which is why the durable control for admin login is the persisted per-account lockout rather than this map. Login now also charges a per-account key that cannot be rotated.
sionzee marked this conversation as resolved
@ -0,0 +24,4 @@
/**
* Record an attempt against `key` and report whether it may proceed.
*/
export function checkRateLimit(key: string, max: number, windowMs: number): RateLimitResult {
Author
Owner

Confirmed - I ran it with windowMs: 0 and got 50/50 allowed against a max of 3. It now throws RangeError on non-positive or non-finite parameters, so it fails closed. Test included.

Confirmed - I ran it with `windowMs: 0` and got 50/50 allowed against a max of 3. It now throws `RangeError` on non-positive or non-finite parameters, so it fails closed. Test included.
sionzee marked this conversation as resolved
@ -0,0 +64,4 @@
const now = Date.now();
for (const [key, bucket] of buckets) {
// One hour is longer than any window used here, so anything older is dead.
if (bucket.hits.every((t) => now - t > 60 * 60 * 1000)) {
Author
Owner

Fixed - each bucket stores the window it was created with and the sweeper expires it against that value. Test asserts a two-hour window still throttles.

Fixed - each bucket stores the window it was created with and the sweeper expires it against that value. Test asserts a two-hour window still throttles.
sionzee marked this conversation as resolved
@ -0,0 +50,4 @@
export function base32Decode(input: string): Uint8Array {
// Users retyping a secret tend to include spaces and padding; both are noise.
const normalized = input.replace(/[\s=]/g, "").toUpperCase();
Author
Owner

Confirmed - MZ=XW6YTBOI decoded happily. Padding is now only accepted at the end, non-zero trailing bits are rejected, and verifyTotp refuses a secret shorter than the 128 bits RFC 4226 requires. Tests added for the padding and trailing-bit cases.

Confirmed - `MZ=XW6YTBOI` decoded happily. Padding is now only accepted at the end, non-zero trailing bits are rejected, and `verifyTotp` refuses a secret shorter than the 128 bits RFC 4226 requires. Tests added for the padding and trailing-bit cases.
sionzee marked this conversation as resolved
@ -0,0 +153,4 @@
/**
* Check a submitted code, scanning the drift window and rejecting replays.
*/
export async function verifyTotp(
Author
Owner

Same shape of answer: the function stays pure, and the network-facing callers are fixed. That was a genuine hole - see the login finding, where the throttle and lockout both reset before MFA - and enrolment now counts failures and honours the lockout too. Documented here as an explicit caller obligation.

Same shape of answer: the function stays pure, and the network-facing callers are fixed. That was a genuine hole - see the login finding, where the throttle and lockout both reset before MFA - and enrolment now counts failures and honours the lockout too. Documented here as an explicit caller obligation.
sionzee marked this conversation as resolved
@ -0,0 +158,4 @@
token: string,
options: TotpVerifyOptions = {},
): Promise<TotpVerifyResult> {
const {
Author
Owner

Fixed - assertValidOptions bounds digits to 6-10, period to 1-600s and window to 0-10 steps, applied in both generation and verification. I confirmed period: 0 threw from BigInt before.

Fixed - `assertValidOptions` bounds digits to 6-10, period to 1-600s and window to 0-10 steps, applied in both generation and verification. I confirmed `period: 0` threw from `BigInt` before.
sionzee marked this conversation as resolved
@ -0,0 +184,4 @@
for (let offset = -window; offset <= window; offset++) {
const counter = current + offset;
// Replay guard - never accept a step at or below one already spent.
Author
Owner

Correct about the boundary, and the caller was the problem. verifyTotp stays pure - it holds no state and cannot consume anything - but verifyTotpChallenge now advances the counter with a compare-and-set predicate and rejects the code when no rows update. The obligation is documented at this function so the next caller does not miss it.

Correct about the boundary, and the caller was the problem. `verifyTotp` stays pure - it holds no state and cannot consume anything - but `verifyTotpChallenge` now advances the counter with a compare-and-set predicate and rejects the code when no rows update. The obligation is documented at this function so the next caller does not miss it.
sionzee marked this conversation as resolved
@ -0,0 +238,4 @@
* One-time recovery codes for when an admin loses their authenticator.
*
* Formatted `XXXX-XXXX-XXXX` from an unambiguous alphabet so they survive being
* written on paper. ~62 bits of entropy each; they are stored only as keyed
Author
Owner

Correct, 32^12 is exactly 60 bits - I had written ~62. Docstring fixed.

Correct, 32^12 is exactly 60 bits - I had written ~62. Docstring fixed.
sionzee marked this conversation as resolved
sionzee deleted branch feat/playtest-portal 2026-08-11 00:24:58 +02:00
Sign in to join this conversation.
No description provided.