|
| 1 | +# OpenDevBot — Core Architecture Code Review by ClaudeAI |
| 2 | + |
| 3 | +Scope: `authProvider.ts`, `chat.ts`, `createApp.ts`, `EventSubEvents.ts`, `index.ts` |
| 4 | +Date: 2026-07-13 |
| 5 | + |
| 6 | +--- |
| 7 | + |
| 8 | +## 🔴 Critical |
| 9 | + |
| 10 | +### 1. `chat.ts:289` — `setInterval` created on every single chat message, never cleared |
| 11 | + |
| 12 | +Inside `commandHandler` (which runs on **every** incoming chat message), there's: |
| 13 | + |
| 14 | +```ts |
| 15 | +void setInterval(async () => { |
| 16 | + if (isIntervalRunning) { |
| 17 | + await intervalHandler(); |
| 18 | + } |
| 19 | +}, intervalDuration); |
| 20 | +``` |
| 21 | + |
| 22 | +This has no guard flag and no stored handle. Compare this to `periodicSaveIntervalId` and |
| 23 | +`socialIntervalId`, which *are* correctly guarded/cleared elsewhere in this same file (e.g. the |
| 24 | +`periodicSocialTimerStarted` flag around line 454). |
| 25 | + |
| 26 | +**Impact:** every chat message in every joined channel spins up a brand-new interval that fetches |
| 27 | +chatters, checks live status, and credits wallets — and it runs forever. With any real chat |
| 28 | +activity this is an unbounded interval leak: |
| 29 | +- growing memory use over time |
| 30 | +- growing Twitch API call volume (rate-limit risk) |
| 31 | +- duplicate/overlapping wallet credits for the same users each interval tick |
| 32 | + |
| 33 | +**This is almost certainly the top production risk right now.** |
| 34 | + |
| 35 | +**Fix direction:** this "periodically credit chatters while live" block shouldn't live inside the |
| 36 | +message handler at all. Pull it out into a one-time setup step (same pattern you already used |
| 37 | +correctly for the periodic social-message timer), guarded by a module-level flag so it only starts |
| 38 | +once per process, and store/clear its interval handle like `periodicSaveIntervalId`. |
| 39 | + |
| 40 | +--- |
| 41 | + |
| 42 | +## 🟠 High |
| 43 | + |
| 44 | +### 2. `createApp.ts` — admin token handling |
| 45 | + |
| 46 | +- Token comparisons use plain `!==` (`provided !== token`, `token !== expected`) instead of a |
| 47 | + constant-time comparison. Low real-world risk for a personal bot, but cheap to harden with |
| 48 | + `crypto.timingSafeEqual`. |
| 49 | +- The admin token can be supplied via `?admin_token=` query param, and the setup/login tokens via |
| 50 | + `?setup_token=` / `?token=` query params. These end up in server access logs, any reverse-proxy |
| 51 | + logs, and browser history. Recommend dropping query-param support for tokens entirely and |
| 52 | + requiring the header/cookie only. |
| 53 | + |
| 54 | +### 3. `/api/v1/admin/setup` writes directly to `.env` on disk |
| 55 | + |
| 56 | +`fs.writeFileSync` is used to persist `ADMIN_API_TOKEN` whenever the endpoint is hit with a valid |
| 57 | +`ADMIN_SETUP_TOKEN`. If that setup token ever leaks, or is left set in a prod `.env` after initial |
| 58 | +setup, anyone who finds it gets full admin API access. Confirm `ADMIN_SETUP_TOKEN` is unset in prod |
| 59 | +once initial setup is complete — maybe add a startup warning if it's still set alongside |
| 60 | +`ENVIRONMENT=prod`. |
| 61 | + |
| 62 | +--- |
| 63 | + |
| 64 | +## 🟡 Medium |
| 65 | + |
| 66 | +### 4. Chatter-fetch/crediting logic duplicated in two places |
| 67 | + |
| 68 | +Once as the (broken) interval inside `commandHandler`, and again in the `onJoin` watch-time |
| 69 | +interval logic. Worth consolidating into a single periodic service so there's one source of truth |
| 70 | +for "credit points every N minutes while live." |
| 71 | + |
| 72 | +### 5. `chat.ts:623-624` — dead code |
| 73 | + |
| 74 | +```ts |
| 75 | +if (stream === null) clearInterval(intervalId); |
| 76 | +``` |
| 77 | + |
| 78 | +This runs right after the function already `return`s earlier when `stream === null` (~line 557), |
| 79 | +so this branch can never be reached. Harmless, but confusing — safe to delete. |
| 80 | + |
| 81 | +### 6. `chat.ts` `onJoin` — potential interval leak on reconnect without `part` |
| 82 | + |
| 83 | +If a user rejoins without a `part` event firing first (reconnect edge case), `viewerWatchTimes.set(user, ...)` |
| 84 | +overwrites the map entry without clearing the *previous* interval tied to that user — a smaller-scale |
| 85 | +version of the same leak pattern as issue #1. Worth clearing any existing interval for `user` before |
| 86 | +overwriting the map entry. |
| 87 | + |
| 88 | +### 7. `authProvider.ts` — fragile Twurple SDK fallback chain |
| 89 | + |
| 90 | +`getChatAuthProvider()`'s intent-registration logic (~lines 120–193) tries four different fallback |
| 91 | +paths wrapped in nested try/catch to work around Twurple SDK version differences between |
| 92 | +`addUserForToken`, `addUser` with options, `addUser`, and `addIntentsToUser`. It works, but it's |
| 93 | +fragile — any Twurple upgrade could silently change which path fires and you might not notice until |
| 94 | +chat auth breaks. Worth a code comment noting the exact Twurple version this was validated against. |
| 95 | + |
| 96 | +--- |
| 97 | + |
| 98 | +## 🟢 Low / cleanup |
| 99 | + |
| 100 | +### 8. `index.ts` — `printEnvironmentVariables()` logs secrets, but is dead code |
| 101 | + |
| 102 | +Not called anywhere in the codebase currently, but if it's ever wired up it will `logger.debug` |
| 103 | +every `.env` key/value pair verbatim — including `TWITCH_CLIENT_SECRET`, Discord webhook tokens, |
| 104 | +`MONGO_PASS`, `NEXON_API_KEY`, etc. Either delete it, or if you want to keep it for future |
| 105 | +debugging, redact any key containing `TOKEN`, `SECRET`, `PASS`, or `KEY` before logging. |
| 106 | + |
| 107 | +### 9. Silent error swallowing in `authProvider.ts` |
| 108 | + |
| 109 | +Multiple `catch (e) { /* ignore */ }` blocks around the SDK-fallback probing in |
| 110 | +`getChatAuthProvider()`. Reasonable for probing which SDK method exists, but worth double-checking |
| 111 | +none of these are hiding a genuine auth failure in prod — consider at least a `logger.debug` in |
| 112 | +each ignored catch so there's a breadcrumb if chat auth ever silently stops working. |
| 113 | + |
| 114 | +--- |
| 115 | + |
| 116 | +## Suggested order of attack |
| 117 | + |
| 118 | +1. Fix #1 (interval leak) — this is the one actually degrading the running bot over time. |
| 119 | +2. Fix #2/#3 (admin auth hardening) — quick wins, closes real exposure. |
| 120 | +3. #4–#7 as time allows — quality-of-life and future-proofing. |
| 121 | +4. #8/#9 — cleanup whenever convenient. |
0 commit comments