fix: resolve all 15 audit findings (2 high, 6 medium, 7 low) #42

Merged

ellamizuki merged 8 commits from fix/audit-findings into main

2026-09-10 16:29:10 +00:00

ellamizuki commented

2026-09-10 15:58:02 +00:00

(Migrated from github.com)

What

Fixes all 15 findings from the bug/regression audit of this repo (2 high, 6 medium, 7 low), each with the reproduction shown before the fix and a validating regression test after. Baseline commit 3c622de had type-check, lint, and the full test suite all green, so every finding below is a real bug, not a pre-existing failure.

Before fixing, every finding was checked against git blame, commit messages, existing tests, README, the Prisma schema, and (for the LivePix finding) the provider's own API docs, to rule out intentional behavior. Only one (API-001) changed classification after that check: LivePix does not sign webhooks by design, so the code was already doing the right thing by re-verifying against the LivePix API before fulfilling a payment; the real gap was narrower than first reported.

Findings and fixes

ID	Severity	Fix	Commit
AUTH-001	high	Owner privilege rederived from live `OWNER_USER_IDS` on every request via the `authenticate` decorator and on `/auth/refresh`, instead of trusting the JWT `isOwner` claim for the token lifetime	`ff045b2`, corrected in `64e0ae5`
BOT-002	high	`safe_event`/`safe_interaction_event` wrappers stop a handler rejection from becoming an `unhandledRejection` that kills the whole bot process	`9920d7b`
BOT-004	medium	`estatisticas` command added to `deploy-commands.ts` (it was registered at runtime but missing from the deploy manifest, so the next global deploy would delete it from Discord)	`13a65fd`
BOT-003	medium	Interactions arriving between `clientReady` and command-load completion get an ephemeral "still starting up" reply instead of being logged and dropped	`13a65fd`
BOT-001	medium	Readiness port default is clamped so `BOT_INTERNAL_PORT=65535` cannot derive an invalid `65536` port	`13a65fd`
BOT-007	medium	`DiscordDeliveryScheduler.stop()` is now async and awaits any in-flight delivery before resolving, so shutdown cannot disconnect Prisma mid-delivery	`13a65fd`
BOT-005	medium	`estatisticas xp` now filters `guildXpMember` by the interaction's `guildId` instead of returning an arbitrary guild's XP	`13a65fd`
API-002	low	Suggestion ranking fetches a bounded set and sorts by net score before truncating to `limit`, instead of truncating by raw upvotes first	`1f07171`
BOT-006	medium	`handle_open` is serialized per guild+user via the existing (previously unused) `KeyedSerialExecutor`, so concurrent ticket-open clicks cannot create two channels/rows	`508d08d`
API-001	low (reclassified from medium)	Webhook reference is resolved against a real `supportPayment` before any database write, so forged references are answered without persisting anything	`61e54b4`
WEB-001..005	low	nginx moved off the privileged port 80 (8080 internally, host mapping unchanged), security headers included in every location, API port bound to `127.0.0.1` only, Redis password fails closed instead of defaulting to `redispassword`, `.env.docker.example` matches the published compose defaults	`2249df6`

Validation

pnpm type-check, pnpm lint, pnpm test (full monorepo): all green on the final branch.
Final test counts: @yuebot/database 8/8, @yuebot/livepix 1/1, @yuebot/api 198/199 (+1 pre-existing skip), @yuebot/bot 199/201 (+2 pre-existing skips). Zero failures.
Every finding was reproduced with an executable probe before the fix and has a regression test after (see individual commits for the exact command/output pairs).

Note on `64e0ae5`

The first AUTH-001 fix (ff045b2) added a redundant second owner check inside guild_access.ts, on top of the correct fix already in the authenticate decorator. That broke ~30 existing route-module tests that construct their own user object with isOwner pre-set, because it re-derived owner status a second time using a function those tests don't control. Caught by running the full monorepo gate (not just the changed package) before opening this PR. 64e0ae5 reverts the redundant layer and adds the permanent regression test AUTH-001 was actually missing (a manually re-run inline probe was the only prior coverage).

## What Fixes all 15 findings from the bug/regression audit of this repo (2 high, 6 medium, 7 low), each with the reproduction shown before the fix and a validating regression test after. Baseline commit `3c622de` had type-check, lint, and the full test suite all green, so every finding below is a real bug, not a pre-existing failure. Before fixing, every finding was checked against git blame, commit messages, existing tests, README, the Prisma schema, and (for the LivePix finding) the provider's own API docs, to rule out intentional behavior. Only one (API-001) changed classification after that check: LivePix does not sign webhooks by design, so the code was already doing the right thing by re-verifying against the LivePix API before fulfilling a payment; the real gap was narrower than first reported. ## Findings and fixes | ID | Severity | Fix | Commit | |----|----------|-----|--------| | AUTH-001 | high | Owner privilege rederived from live `OWNER_USER_IDS` on every request via the `authenticate` decorator and on `/auth/refresh`, instead of trusting the JWT `isOwner` claim for the token lifetime | `ff045b2`, corrected in `64e0ae5` | | BOT-002 | high | `safe_event`/`safe_interaction_event` wrappers stop a handler rejection from becoming an `unhandledRejection` that kills the whole bot process | `9920d7b` | | BOT-004 | medium | `estatisticas` command added to `deploy-commands.ts` (it was registered at runtime but missing from the deploy manifest, so the next global deploy would delete it from Discord) | `13a65fd` | | BOT-003 | medium | Interactions arriving between `clientReady` and command-load completion get an ephemeral "still starting up" reply instead of being logged and dropped | `13a65fd` | | BOT-001 | medium | Readiness port default is clamped so `BOT_INTERNAL_PORT=65535` cannot derive an invalid `65536` port | `13a65fd` | | BOT-007 | medium | `DiscordDeliveryScheduler.stop()` is now async and awaits any in-flight delivery before resolving, so shutdown cannot disconnect Prisma mid-delivery | `13a65fd` | | BOT-005 | medium | `estatisticas xp` now filters `guildXpMember` by the interaction's `guildId` instead of returning an arbitrary guild's XP | `13a65fd` | | API-002 | low | Suggestion ranking fetches a bounded set and sorts by net score before truncating to `limit`, instead of truncating by raw upvotes first | `1f07171` | | BOT-006 | medium | `handle_open` is serialized per guild+user via the existing (previously unused) `KeyedSerialExecutor`, so concurrent ticket-open clicks cannot create two channels/rows | `508d08d` | | API-001 | low (reclassified from medium) | Webhook reference is resolved against a real `supportPayment` *before* any database write, so forged references are answered without persisting anything | `61e54b4` | | WEB-001..005 | low | nginx moved off the privileged port 80 (8080 internally, host mapping unchanged), security headers included in every location, API port bound to `127.0.0.1` only, Redis password fails closed instead of defaulting to `redispassword`, `.env.docker.example` matches the published compose defaults | `2249df6` | ## Validation - `pnpm type-check`, `pnpm lint`, `pnpm test` (full monorepo): all green on the final branch. - Final test counts: `@yuebot/database` 8/8, `@yuebot/livepix` 1/1, `@yuebot/api` 198/199 (+1 pre-existing skip), `@yuebot/bot` 199/201 (+2 pre-existing skips). Zero failures. - Every finding was reproduced with an executable probe before the fix and has a regression test after (see individual commits for the exact command/output pairs). ## Note on `64e0ae5` The first AUTH-001 fix (`ff045b2`) added a redundant second owner check inside `guild_access.ts`, on top of the correct fix already in the `authenticate` decorator. That broke ~30 existing route-module tests that construct their own `user` object with `isOwner` pre-set, because it re-derived owner status a second time using a function those tests don't control. Caught by running the full monorepo gate (not just the changed package) before opening this PR. `64e0ae5` reverts the redundant layer and adds the permanent regression test AUTH-001 was actually missing (a manually re-run inline probe was the only prior coverage).