feat(playtest): add playtest portal backend #242
No reviewers
Labels
No labels
bug
dependencies
documentation
duplicate
enhancement
github-actions
good first issue
help wanted
invalid
question
wontfix
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
Evobug/api!242
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/playtest-portal"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.OpenCodeReview (
openai/gpt-5.6-luna) — 40 findings40 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@ -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],);[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.
@ -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) {[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 Opscreates an account with emailtrue, 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.@ -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;[security · medium] Reading
--passwordfromprocess.argvexposes 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.@ -0,0 +95,5 @@process.exit(1);}const client = new Bun.SQL(process.env.DATABASE_URL);const db = drizzle({ client, relations });[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 intry/finallyand report expected failures explicitly.@ -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);[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.@ -0,0 +133,6 @@const line = "─".repeat(66);console.log(`${line}Playtest admin created - id ${admin?.id}[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.
@ -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();[bug · medium] The preflight
findAdminByEmailcheck 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 declaredADMIN_EXISTSerror. Use an atomic conflict-safe insert and map an emptyreturning()result toADMIN_EXISTS.@ -0,0 +202,5 @@export const acceptInvite = base.input(z.object({token: z.string().min(1),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).
@ -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));[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,
invitedstatus, and expiry, usereturning()to detect a lost race, and reject the loser.@ -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,);[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).@ -0,0 +308,6 @@LOGIN_RATE_LIMIT_WINDOW_MS,);if (!limit.allowed) {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
writeAuditcall. 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).@ -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"}`);[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
adminLoginagain to reset the counter, and repeat indefinitely;confirmTotpEnrollmentalso 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.@ -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));[security · medium]
totpLastUsedCounteris 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.@ -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([security · medium] Unlike
verifyTotpChallenge, this recovery path never checksisLockedOut(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.@ -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));[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 sameremainingarray, 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.@ -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);[bug · low]
revokeAllSessionsrevokes 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.@ -0,0 +765,12 @@await context.db.update(playtestAdminsTable).set({totpEnabled: false,totpSecretCiphertext: null,totpConfirmedAt: null,totpLastUsedCounter: null,recoveryCodeHashes: [],status: "invited",updatedAt: new Date(),})[security · medium] Resetting TOTP unconditionally changes the target to
invited, even when the target wasdisabled. Because the existing password hash is retained, that disabled account can log in through the enrollment branch andconfirmTotpEnrollmentchanges it toactive, unintentionally re-enabling it. Preservedisableduntil a separate explicit status change enables the account.@ -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 });[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).@ -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");[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 eventualhrefor CSS context.@ -0,0 +41,4 @@const productShapeSchema = z.object({slug: slugSchema,name: z.string().min(1).max(255).trim(),[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.@ -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"),[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).
@ -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();[bug · medium] The preflight
SELECTdoes 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 declaredSLUG_TAKENerror. Use a conflict-safe insert (and map the update path's unique violation as well) instead of relying on this check.@ -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 },});[maintainability · medium] The mutation is committed before
writeAuditruns.writeAuditdeliberately 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.@ -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();[bug · high]
statusis part of the generic patch, soupdateProduct({ id, status: "archived" })bypasses the assigned-key guard inarchiveProduct. 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.@ -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);[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.
@ -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));[bug · medium] The assigned-key check and the archive update are separate operations, so a concurrent
assignKeycan 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.@ -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.[other · medium] This default confirmation is returned for every playtest, including products with
requireNda: false. Those applications move directly toselectedand 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.@ -0,0 +56,4 @@fallback: string,gameName: string,): string {return (template ?? fallback).replaceAll("{{GAME}}", gameName);[other · medium] An empty (or whitespace-only) admin override is accepted by
productShapeSchemaand 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).[other · low]
gameNameis 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 asA $& Bchanges the rendered copy. Use the callback form ofreplaceAll` to insert the name literally.@ -316,0 +342,7 @@from: r.playtestApplicationsTable.productId,to: r.playtestProductsTable.id,}),user: r.one.usersTable({from: r.playtestApplicationsTable.userId,to: r.usersTable.id,}),[maintainability · low] The schema defines two additional foreign keys from applications to
playtestAdminsTable(selectedByAdminIdandrejectedByAdminId), 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 aliasedr.one.playtestAdminsTablerelations (and inverse collections if those are queried from the admin side).@ -316,0 +370,7 @@from: r.playtestKeysTable.assignedUserId,to: r.usersTable.id,}),application: r.one.playtestApplicationsTable({from: r.playtestKeysTable.assignedApplicationId,to: r.playtestApplicationsTable.id,}),[maintainability · low]
playtestKeysTablehas anuploadedByAdminIdforeign key toplaytestAdminsTable, 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 namedr.one.playtestAdminsTablerelation (and an inverse admin collection if needed).@ -0,0 +1,73 @@/*** Fixed-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.
@ -0,0 +12,4 @@hits: number[];}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.
@ -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 {[security · low] Because this exported helper accepts runtime
maxandwindowMsvalues without validation, a non-positivewindowMsmakes every previous hit expire immediately and effectively disables throttling. Invalidmaxvalues can likewise produce permanent denial or nonsensicalremainingvalues. Validate positive finite parameters and fail closed before mutating the bucket.@ -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)) {[maintainability · medium] This cleanup threshold is hard-coded independently of the
windowMssupplied tocheckRateLimit. 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.@ -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();[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 allowingcrypto.subtle.importKeyerrors to escape.@ -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> {[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.
@ -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;[security · medium]
digits,period, andwindoware accepted without runtime validation. A zero/non-finite period can maketotpCounterproduce a value thatBigIntcannot convert, malformeddigitscan make the dynamicRegExpthrow, 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.@ -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;[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
lastUsedCountercan both returnvalid: truebefore 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.@ -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.[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.
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.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 inreview-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(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.
@ -0,0 +67,4 @@}const args = parseArgs(process.argv.slice(2));const email = args.email?.trim().toLowerCase();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.@ -0,0 +87,4 @@process.exit(1);}const password = args.password ?? process.env.PLAYTEST_ADMIN_PASSWORD ?? generatePassword();Fixed.
--passwordis rejected outright now; the flag parser only accepts--emailand--name. A password can still be supplied out of band viaPLAYTEST_ADMIN_PASSWORD, which is not visible in the process list or shell history.@ -0,0 +95,4 @@process.exit(1);}const client = new Bun.SQL(process.env.DATABASE_URL);Fixed. The database work is wrapped in
try/finallyso the client is always closed, and an unexpected failure exits non-zero with a message instead of an uncaught rejection.@ -0,0 +98,4 @@const client = new Bun.SQL(process.env.DATABASE_URL);const db = drizzle({ client, relations });const [existing] = await dbFixed. The SELECT is gone; the insert uses
onConflictDoNothingon the email target and an emptyreturning()produces the duplicate-admin message.@ -0,0 +133,4 @@const line = "─".repeat(66);console.log(`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=1is 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.@ -0,0 +156,4 @@const inviteToken = generateToken(32);const expiresAt = new Date(Date.now() + ADMIN_INVITE_TTL_MS);const [created] = await context.dbFixed. The preflight SELECT is removed and the insert uses
onConflictDoNothingon the email target, with an empty result mapped toADMIN_EXISTS. Covered by a test asserting the declared error rather than a raw constraint violation.@ -0,0 +202,4 @@export const acceptInvite = base.input(z.object({token: z.string().min(1),Fixed - bounded to
max(128), with a test that a 500-character token is rejected before it reaches hashing or the lookup.@ -0,0 +241,4 @@const totpSecret = generateTotpSecret();await context.dbFixed. The consuming UPDATE now also matches the token hash and
invitedstatus, and a lost race (no rows returned) throwsINVALID_INVITE.@ -0,0 +302,4 @@.handler(async ({ input, context, errors }) => {const meta = getRequestMeta(context.headers);const limit = checkRateLimit(Correct -
meta.ipcomes 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.@ -0,0 +308,4 @@LOGIN_RATE_LIMIT_WINDOW_MS,);if (!limit.allowed) {Fixed - a rate-limited login now writes an
admin.login.rate_limitedentry before throwing, so brute-force pressure is visible in the audit trail rather than silently dropped.@ -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.dbThe most serious of the 40, and correct. I reproduced it: 12 consecutive wrong TOTP codes accepted with
lockedUntilstill null, because eachadminLoginreset the counter andclearRateLimitemptied the IP bucket. Both resets moved intoonAuthenticationComplete, 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.@ -0,0 +441,4 @@throw errors.INVALID_CODE();}await context.dbFixed 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.
@ -0,0 +538,4 @@.errors({INVALID_CODE: { message: "That recovery code is not valid" },}).handler(async ({ input, context, errors }) => {Confirmed and fixed.
isLockedOutis now checked before the code is examined, and the same check was added toconfirmTotpEnrollment, which had the same gap. Regression test included.@ -0,0 +558,4 @@throw errors.INVALID_CODE();}const remaining = stored.filter((hash) => hash !== match);Fixed with a compare-and-swap: the update now requires the stored
recoveryCodeHashesto still equal the snapshot it was read from, and an emptyreturning()is treated as an invalid code. A concurrent request therefore cannot spend the same code twice.@ -0,0 +657,4 @@}).where(eq(playtestAdminsTable.id, admin.id));// Force every other device to re-authenticate with the new credential.Fixed.
revokeAllSessionstakes an optionalexceptSessionId, 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.@ -0,0 +765,4 @@await context.db.update(playtestAdminsTable).set({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.@ -0,0 +859,4 @@.input(z.object({}).optional()).output(z.object({ removed: z.number() })).handler(async ({ context }) => {const removed = await context.dbFixed - the
isNull(revokedAt)predicate is removed, so every session pastexpiresAtis pruned. Revoked rows have no retention requirement here; the audit log is the durable record.@ -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 = zConfirmed -
"https://"passed. Replaced withnew URL()parsing plus explicit protocol and non-empty hostname checks. Test added.@ -0,0 +41,4 @@const productShapeSchema = z.object({slug: slugSchema,name: z.string().min(1).max(255).trim(),Confirmed - I checked the schema directly and
" "parsed successfully to"". Reordered to.trim().min(1), with a test.@ -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"),Fixed - the default now reads
PLAYTEST_PRIVACY_POLICY_URLand falls back to the Evobug URL only when unset.@ -0,0 +132,4 @@assertConsistentProviders(input.allowedAuthProviders, input.requiredConnections);const [existing] = await context.dbFixed -
onConflictDoNothingon the slug target with an empty result mapped toSLUG_TAKEN, so the constraint decides rather than a check that cannot serialise with the insert.@ -0,0 +158,4 @@if (!created) throw new ORPCError("INTERNAL_ERROR");await writeAudit(context.db, meta, {Acknowledged, deliberately not changed.
writeAuditswallows 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.@ -0,0 +206,4 @@if (clash) throw errors.SLUG_TAKEN();}const [updated] = await context.dbConfirmed 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.@ -0,0 +262,4 @@.orderBy(playtestProductsTable.id);// Two grouped aggregates rather than N+1 per product.const applicationCounts = await context.dbFixed - both aggregates are now restricted with
inArray(productId, productIds)and run throughPromise.all, with an early return when the list is empty.@ -0,0 +330,4 @@.handler(async ({ input, context, errors }) => {const { admin, meta } = context as unknown as AdminSessionContext;const [assigned] = await context.dbFixed. The check and update run in one transaction that first takes
FOR UPDATEon the product row, which serialises againstassignKey, and a missing product now throwsNOT_FOUNDinstead of reporting success - I confirmed the old code returnedsuccess: truefor id 999999 and wrote an audit entry.@ -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.Good catch - the copy contradicted the funnel for
requireNda: false, where applications go straight toselected. Reworded to "an email with the next steps... once anything we need from you is done", which is true either way.@ -0,0 +56,4 @@fallback: string,gameName: string,): string {return (template ?? fallback).replaceAll("{{GAME}}", gameName);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, and worse than described: rendering a name of
A $& BproducedWelcome to A {{GAME}} B!, because$&re-inserts the matched placeholder. Switched to the callback form ofreplaceAll, with a test covering$&,$`and$'.@ -316,0 +342,4 @@from: r.playtestApplicationsTable.productId,to: r.playtestProductsTable.id,}),user: r.one.usersTable({Fixed - added
selectedByandrejectedByas distinctly aliasedr.one.playtestAdminsTablerelations.@ -316,0 +370,4 @@from: r.playtestKeysTable.assignedUserId,to: r.usersTable.id,}),application: r.one.playtestApplicationsTable({Fixed - added an aliased
uploadedByrelation so the key-vault audit link is navigable.@ -0,0 +1,73 @@/*** Fixed-window in-memory rate limiter.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).
@ -0,0 +12,4 @@hits: number[];}const buckets = new Map<string, Bucket>();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.
@ -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 {Confirmed - I ran it with
windowMs: 0and got 50/50 allowed against a max of 3. It now throwsRangeErroron non-positive or non-finite parameters, so it fails closed. Test included.@ -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)) {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.
@ -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();Confirmed -
MZ=XW6YTBOIdecoded happily. Padding is now only accepted at the end, non-zero trailing bits are rejected, andverifyTotprefuses a secret shorter than the 128 bits RFC 4226 requires. Tests added for the padding and trailing-bit cases.@ -0,0 +153,4 @@/*** Check a submitted code, scanning the drift window and rejecting replays.*/export async function verifyTotp(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.
@ -0,0 +158,4 @@token: string,options: TotpVerifyOptions = {},): Promise<TotpVerifyResult> {const {Fixed -
assertValidOptionsbounds digits to 6-10, period to 1-600s and window to 0-10 steps, applied in both generation and verification. I confirmedperiod: 0threw fromBigIntbefore.@ -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.Correct about the boundary, and the caller was the problem.
verifyTotpstays pure - it holds no state and cannot consume anything - butverifyTotpChallengenow 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.@ -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 keyedCorrect, 32^12 is exactly 60 bits - I had written ~62. Docstring fixed.