Fix security vulnerabilities, bugs, and code quality issues#6
Fix security vulnerabilities, bugs, and code quality issues#6vulnix0x4 wants to merge 1 commit intoT3-Content:mainfrom
Conversation
Security fixes: - Fix path traversal in /assets/ route allowing arbitrary file reads - Remove admin secret from query string params (leak via logs/referer) - Add non-root user to Dockerfile - Validate broadcast WebSocket sink URL to prevent data exfiltration - Remove legacy /api/pause and /api/resume routes - Hide ADMIN_SECRET configuration status from error responses - Add HEALTHCHECK to Dockerfile Bug fixes: - Fix off-by-one in round counter after admin reset (skipped round 1) - Fix vote parsing using startsWith instead of exact match - Fix Math.random() as React key in history page (constant re-mount) - Fix race condition in history pagination (add AbortController) - Add res.ok check before parsing history API response - Clear stale error state on page change in history - Move saveRound() before 5-second display delay to prevent data loss - Fix _.log typo in .gitignore (was not ignoring log files) - Add .catch() to runGame promise to handle unhandled rejections - Fix check-db.ts to respect DATABASE_PATH env var - Add mkdirSync for logs dir in bulk-test.ts Performance/reliability: - Enable SQLite WAL mode for better concurrent read performance - Prepare SQL statements at module init instead of per-call - Add exponential backoff with jitter to WebSocket reconnect - Remove unused `reasoning` destructuring from AI calls - Fix isRealString to trim whitespace before length check Code quality: - Remove 8 duplicate prompts from prompts.ts - Fix voter dot keys to use stable model name instead of array index - Fix double getLogo() calls in history voter maps - Add admin status polling (10s interval) to prevent stale display - Add modal focus trap and Escape key handler for accessibility - Remove unused useMemo import in admin.tsx - Add .env.example documenting all 25+ environment variables - Add test and typecheck scripts to package.json - Fix module field in package.json (pointed to nonexistent index.ts) - Exclude dev files from Docker image via .dockerignore - Add *.sqlite-shm and *.sqlite-wal to .gitignore Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds comprehensive configuration documentation, enhances Docker security with non-root execution and health checks, optimizes database queries with prepared statements and WAL journaling, improves game logic for vote parsing and data archiving, adds WebSocket reconnection with exponential backoff, and strengthens server security through path validation and header-based authentication. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Harden admin auth and static asset serving, remove public pause/resume routes, and run the container as non‑root with port 5109 in server.ts and DockerfileTighten admin secret handling to header/cookie only, block path traversal under 📍Where to StartStart in the 📊 Macroscope summarized a3406d0. 14 files reviewed, 7 issues evaluated, 0 issues filtered, 1 comment posted. View details |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| RUN addgroup --system --gid 1001 app && adduser --system --uid 1001 --ingroup app app | ||
| USER app |
There was a problem hiding this comment.
🟠 High Dockerfile:15
The app user won't have write permissions to /app since COPY runs as root. SQLite database creation will fail. Consider adding RUN chown -R app:app /app before USER app.
| RUN addgroup --system --gid 1001 app && adduser --system --uid 1001 --ingroup app app | |
| USER app | |
| RUN addgroup --system --gid 1001 app && adduser --system --uid 1001 --ingroup app app | |
| RUN chown -R app:app /app | |
| USER app |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file Dockerfile around lines 15-16:
The `app` user won't have write permissions to `/app` since `COPY` runs as root. SQLite database creation will fail. Consider adding `RUN chown -R app:app /app` before `USER app`.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server.ts (1)
264-276:⚠️ Potential issue | 🟡 MinorPath traversal guard validates against
public/, notpublic/assets/.The safePath check ensures the resolved path stays under
PUBLIC_DIR(./public), but since the route only matches/assets/*, a request like/assets/../../somefilewould be blocked from leavingpublic/— yet/assets/../otherdir/filestill resolves to any file underpublic/. Ifpublic/contains files that shouldn't be served by this route, tighten the boundary topublic/assets/.Also, if
safePathpoints to a non-existent file,Bun.file()will produce an empty/error response. Consider checking existence first.Proposed fix to tighten the path boundary and check existence
if (url.pathname.startsWith("/assets/")) { - const PUBLIC_DIR = resolve("./public"); - const safePath = resolve(`./public${url.pathname}`); - if (!safePath.startsWith(PUBLIC_DIR + "/") && safePath !== PUBLIC_DIR) { + const ASSETS_DIR = resolve("./public/assets"); + const safePath = resolve(`./public${url.pathname}`); + if (!safePath.startsWith(ASSETS_DIR + "/") && safePath !== ASSETS_DIR) { return new Response("Forbidden", { status: 403 }); } const file = Bun.file(safePath); + if (!(await file.exists())) { + return new Response("Not Found", { status: 404 }); + } return new Response(file, {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.ts` around lines 264 - 276, The path-traversal guard currently compares safePath against PUBLIC_DIR (resolve("./public")), but the route only serves /assets/* so tighten the boundary by using PUBLIC_ASSETS_DIR = resolve("./public/assets") and validate safePath.startsWith(PUBLIC_ASSETS_DIR + "/") || safePath === PUBLIC_ASSETS_DIR; then obtain the file via Bun.file(safePath) and explicitly check that the file exists before returning (respond 404 or 403 as appropriate) instead of blindly returning Bun.file for non-existent paths; update references to PUBLIC_DIR, safePath, and Bun.file in the /assets handler accordingly.
🧹 Nitpick comments (6)
check-db.ts (1)
3-3: Consider opening the DB read-only for this diagnostic script.
bun:sqlitedefaults tocreate: true, so ifDATABASE_PATHdoesn't point to an existing file, the script silently creates an empty database. For a check/diagnostic tool,{ create: false }(or{ readonly: true }) produces a clearer "file not found" failure.♻️ Proposed change
-const db = new Database(dbPath); +const db = new Database(dbPath, { readonly: true });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@check-db.ts` at line 3, The script currently constructs the Database with new Database(dbPath) which uses bun:sqlite's default create: true and may silently create an empty DB; update the Database instantiation in check-db.ts (the new Database(...) call) to open the file read-only or disallow creation by passing options like { create: false } or { readonly: true } so the script fails clearly when DATABASE_PATH does not exist.game.ts (1)
3-3: Consider using Bun-native file APIs instead ofnode:fsappendFileSync.The coding guidelines prefer
Bun.fileovernode:fsfor file operations. WhileappendFileSyncworks well under Bun's Node.js compat layer and there's no direct one-liner replacement for append in the Bun API, you could useBun.file(LOG_FILE).writer()for a streaming approach that avoids repeated synchronous I/O on the event loop. This is optional since the current approach works correctly. Based on learnings, "PreferBun.fileover Node.jsnode:fsreadFile/writeFile for file operations."Also applies to: 111-111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@game.ts` at line 3, Replace use of node:fs appendFileSync with Bun-native APIs: update the import that currently references appendFileSync/mkdirSync and change code that calls appendFileSync(LOG_FILE, ...) to use Bun.file(LOG_FILE).writer() for streaming writes (and treat mkdirSync usage as needed—use Bun.mkdir or keep mkdirSync if compatibility required). Locate references to appendFileSync and LOG_FILE in the file and switch to opening a Bun.file(LOG_FILE).writer() once and write/appending via its write/close methods to avoid repeated synchronous I/O on the event loop.admin.tsx (3)
86-94: Polling effect looks good; consider adding an AbortController for cleanup.The interval-based polling is properly gated on
mode === "ready"and cleans up viaclearInterval. However, an in-flight fetch from the last interval tick can still resolve after cleanup (e.g., on logout), callingsetSnapshot(data)with stale admin data. This isn't user-visible since the locked screen ignoressnapshot, but for consistency with the pattern used inhistory.tsx, you could add abort-on-cleanup.Proposed fix
useEffect(() => { if (mode !== "ready") return; + const controller = new AbortController(); const interval = setInterval(() => { - requestAdminJson("/api/admin/status") + fetch("/api/admin/status", { cache: "no-store", signal: controller.signal }) + .then((res) => res.ok ? res.json() : Promise.reject()) .then((data) => setSnapshot(data)) .catch(() => {}); }, 10_000); - return () => clearInterval(interval); + return () => { + clearInterval(interval); + controller.abort(); + }; }, [mode]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin.tsx` around lines 86 - 94, The polling useEffect in admin.tsx (the effect that runs when mode === "ready") should use an AbortController to cancel any in-flight requestAdminJson calls on cleanup; create an AbortController inside the effect, pass its signal into requestAdminJson when invoking it in the interval callback, and call controller.abort() in the cleanup along with clearInterval(interval) so any pending promises won’t call setSnapshot after unmount/logout.
109-126: Focus trap implementation is solid for the modal's content.The
useCallbackwith a stable reference and the querySelectorAll approach correctly traps Tab/Shift+Tab within the modal. One minor edge case: if focus is somehow on the backdrop (outsidemodalRef),document.activeElementwon't matchfirstorlast, so the default Tab behavior would escape the trap. In practice this is unlikely since the modal<input>hasautoFocus, but for robustness you could redirect focus into the modal whenactiveElementis not among the focusable elements.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin.tsx` around lines 109 - 126, The focus-trap should also handle the case where document.activeElement lies outside the modal; inside the existing handleModalKeyDown (the useCallback using modalRef and the focusable NodeList), add a guard that checks whether document.activeElement is one of the focusable elements (compare against items in focusable or use Array.from(focusable).includes(document.activeElement as HTMLElement)); if it is not, call e.preventDefault() and call first.focus() to force focus into the modal, then proceed with the existing shift/tab wrap logic between first and last.
374-375: Good use ofrole="dialog"andaria-modal="true"for screen reader semantics.The modal also correctly combines focus trapping (via
onKeyDown) with the ref-based approach. Consider addingaria-labelledbypointing to the<h2>for better screen reader announcements.Proposed fix
- <div className="modal-backdrop" role="dialog" aria-modal="true" onKeyDown={handleModalKeyDown}> - <div className="modal" ref={modalRef}> - <h2>Reset all data?</h2> + <div className="modal-backdrop" role="dialog" aria-modal="true" aria-labelledby="reset-dialog-title" onKeyDown={handleModalKeyDown}> + <div className="modal" ref={modalRef}> + <h2 id="reset-dialog-title">Reset all data?</h2>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@admin.tsx` around lines 374 - 375, The modal div using role="dialog" should include aria-labelledby to point to the heading so screen readers announce the title: add an id (e.g. "adminModalTitle") to the <h2> element that renders the modal title and add aria-labelledby="adminModalTitle" to the <div className="modal" ref={modalRef}>; keep the existing handleModalKeyDown and modalRef logic intact and ensure the id is unique within the page (or generated) to avoid collisions.db.ts (1)
17-20: Usedb.query()consistently for static, reused prepared statements.In
bun:sqlite,db.query()caches the compiled statement on the Database instance (suitable for static, repeated queries), whiledb.prepare()returns a fresh, non-cached statement (suitable for dynamic or one-off queries). SinceinsertRoundandselectRoundsPageare static statements stored in variables and reused, they should usedb.query()for consistency with the other two statements and to align with bun:sqlite conventions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db.ts` around lines 17 - 20, The prepared statements insertRound and selectRoundsPage are created with db.prepare but are static, reused queries; replace their creation to use db.query (like countRounds and selectAllRounds) so the statements are cached by bun:sqlite — update the declarations for insertRound and selectRoundsPage to call db.query with the same SQL strings and keep using the same variable names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Around line 14-16: The non-root user creation (addgroup/adduser) and USER app
switch leave /app owned by root so the app cannot write the SQLite DB; fix by
ensuring ownership is changed to app before switching users — add a chown -R
app:app /app in the same RUN layer where addgroup/adduser are executed (before
USER app), or prefer using COPY --chown=app:app when copying files so /app is
owned by app and writable by the process that runs under USER app.
In `@frontend.tsx`:
- Around line 418-427: The cleanup currently calls ws?.close() which
asynchronously triggers the ws.onclose handler and creates a new reconnectTimer
that clearTimeout already cleared; to fix, before calling ws?.close() null out
the onclose handler (e.g., set ws.onclose = null) so the intentional close
doesn't schedule a reconnection, and keep the existing logic that clears
reconnectTimer and resets reconnectAttempt; apply the same change for the other
cleanup spot that mirrors the onclose/reconnect logic (the block that assigns
ws.onopen/ws.onclose and calls connect/reconnectTimer).
In `@history.tsx`:
- Around line 183-193: The voter logo <img> elements in HistoryCard are missing
alt text; update the img rendering inside the votersA and votersB maps (the
block using getLogo(v.name)) to include alt={v.name} (matching
HistoryContestant's behavior) so each <img key={v.name} src={logo} ... />
becomes <img ... alt={v.name} /> for accessibility.
---
Outside diff comments:
In `@server.ts`:
- Around line 264-276: The path-traversal guard currently compares safePath
against PUBLIC_DIR (resolve("./public")), but the route only serves /assets/* so
tighten the boundary by using PUBLIC_ASSETS_DIR = resolve("./public/assets") and
validate safePath.startsWith(PUBLIC_ASSETS_DIR + "/") || safePath ===
PUBLIC_ASSETS_DIR; then obtain the file via Bun.file(safePath) and explicitly
check that the file exists before returning (respond 404 or 403 as appropriate)
instead of blindly returning Bun.file for non-existent paths; update references
to PUBLIC_DIR, safePath, and Bun.file in the /assets handler accordingly.
---
Nitpick comments:
In `@admin.tsx`:
- Around line 86-94: The polling useEffect in admin.tsx (the effect that runs
when mode === "ready") should use an AbortController to cancel any in-flight
requestAdminJson calls on cleanup; create an AbortController inside the effect,
pass its signal into requestAdminJson when invoking it in the interval callback,
and call controller.abort() in the cleanup along with clearInterval(interval) so
any pending promises won’t call setSnapshot after unmount/logout.
- Around line 109-126: The focus-trap should also handle the case where
document.activeElement lies outside the modal; inside the existing
handleModalKeyDown (the useCallback using modalRef and the focusable NodeList),
add a guard that checks whether document.activeElement is one of the focusable
elements (compare against items in focusable or use
Array.from(focusable).includes(document.activeElement as HTMLElement)); if it is
not, call e.preventDefault() and call first.focus() to force focus into the
modal, then proceed with the existing shift/tab wrap logic between first and
last.
- Around line 374-375: The modal div using role="dialog" should include
aria-labelledby to point to the heading so screen readers announce the title:
add an id (e.g. "adminModalTitle") to the <h2> element that renders the modal
title and add aria-labelledby="adminModalTitle" to the <div className="modal"
ref={modalRef}>; keep the existing handleModalKeyDown and modalRef logic intact
and ensure the id is unique within the page (or generated) to avoid collisions.
In `@check-db.ts`:
- Line 3: The script currently constructs the Database with new Database(dbPath)
which uses bun:sqlite's default create: true and may silently create an empty
DB; update the Database instantiation in check-db.ts (the new Database(...)
call) to open the file read-only or disallow creation by passing options like {
create: false } or { readonly: true } so the script fails clearly when
DATABASE_PATH does not exist.
In `@db.ts`:
- Around line 17-20: The prepared statements insertRound and selectRoundsPage
are created with db.prepare but are static, reused queries; replace their
creation to use db.query (like countRounds and selectAllRounds) so the
statements are cached by bun:sqlite — update the declarations for insertRound
and selectRoundsPage to call db.query with the same SQL strings and keep using
the same variable names.
In `@game.ts`:
- Line 3: Replace use of node:fs appendFileSync with Bun-native APIs: update the
import that currently references appendFileSync/mkdirSync and change code that
calls appendFileSync(LOG_FILE, ...) to use Bun.file(LOG_FILE).writer() for
streaming writes (and treat mkdirSync usage as needed—use Bun.mkdir or keep
mkdirSync if compatibility required). Locate references to appendFileSync and
LOG_FILE in the file and switch to opening a Bun.file(LOG_FILE).writer() once
and write/appending via its write/close methods to avoid repeated synchronous
I/O on the event loop.
| # Run as non-root user | ||
| RUN addgroup --system --gid 1001 app && adduser --system --uid 1001 --ingroup app app | ||
| USER app |
There was a problem hiding this comment.
Missing chown: the app user cannot write to /app, breaking SQLite database creation.
All files under /app are copied as root and owned by root (permissions 755/644). After USER app, the server tries to create quipslop.sqlite (default DATABASE_PATH) inside /app — a directory where the non-root user has no write permission — resulting in EACCES at startup.
Add chown -R app:app /app in the same RUN layer before switching users:
🔒 Proposed fix
-RUN addgroup --system --gid 1001 app && adduser --system --uid 1001 --ingroup app app
+RUN addgroup --system --gid 1001 app && \
+ adduser --system --uid 1001 --ingroup app app && \
+ chown -R app:app /app
USER appAlternatively, use COPY --chown=app:app instead of a separate chown layer to keep image size minimal:
+RUN addgroup --system --gid 1001 app && adduser --system --uid 1001 --ingroup app app
COPY package.json bun.lock ./
RUN bun install --frozen-lockfile --production
-COPY . .
+COPY --chown=app:app . .
+USER app📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Run as non-root user | |
| RUN addgroup --system --gid 1001 app && adduser --system --uid 1001 --ingroup app app | |
| USER app | |
| # Run as non-root user | |
| RUN addgroup --system --gid 1001 app && \ | |
| adduser --system --uid 1001 --ingroup app app && \ | |
| chown -R app:app /app | |
| USER app |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` around lines 14 - 16, The non-root user creation
(addgroup/adduser) and USER app switch leave /app owned by root so the app
cannot write the SQLite DB; fix by ensuring ownership is changed to app before
switching users — add a chown -R app:app /app in the same RUN layer where
addgroup/adduser are executed (before USER app), or prefer using COPY
--chown=app:app when copying files so /app is owned by app and writable by the
process that runs under USER app.
| ws.onopen = () => { | ||
| setConnected(true); | ||
| reconnectAttempt = 0; | ||
| }; | ||
| ws.onclose = () => { | ||
| setConnected(false); | ||
| reconnectTimer = setTimeout(connect, 2000); | ||
| const delay = Math.min(1000 * Math.pow(2, reconnectAttempt), 30000) + Math.random() * 1000; | ||
| reconnectAttempt++; | ||
| reconnectTimer = setTimeout(connect, delay); | ||
| }; |
There was a problem hiding this comment.
WebSocket cleanup leaves an uncancellable reconnect timer.
ws?.close() in the cleanup triggers the onclose handler asynchronously — after the cleanup function has already returned. When onclose fires, it sets a brand-new reconnectTimer via setTimeout(connect, delay). Because clearTimeout(reconnectTimer) already ran, this new timer is never cleared. connect() then fires, creates a fresh WebSocket, and calls setState on the (already unmounted) component.
This is especially visible in React StrictMode (effects are mounted → cleaned up → re-mounted): the async onclose from the first cleanup races with the second mount, potentially creating two live WebSocket connections.
Fix: null out ws.onclose before closing so the handler doesn't fire on an intentional cleanup close.
🔒 Proposed fix
return () => {
clearTimeout(reconnectTimer);
+ ws.onclose = null; // prevent async onclose from scheduling a reconnect after cleanup
ws?.close();
};Also applies to: 443-446
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend.tsx` around lines 418 - 427, The cleanup currently calls ws?.close()
which asynchronously triggers the ws.onclose handler and creates a new
reconnectTimer that clearTimeout already cleared; to fix, before calling
ws?.close() null out the onclose handler (e.g., set ws.onclose = null) so the
intentional close doesn't schedule a reconnection, and keep the existing logic
that clears reconnectTimer and resets reconnectAttempt; apply the same change
for the other cleanup spot that mirrors the onclose/reconnect logic (the block
that assigns ws.onopen/ws.onclose and calls connect/reconnectTimer).
| {votersA.map((v) => { | ||
| const logo = getLogo(v.name); | ||
| return logo ? ( | ||
| <img | ||
| key={v.name} | ||
| src={logo} | ||
| title={v.name} | ||
| className="voter-mini-logo" | ||
| /> | ||
| ) : null; | ||
| })} |
There was a problem hiding this comment.
Missing alt attribute on voter logo <img> elements.
The inline voter logo rendering in HistoryCard (lines 186-191 and 221-226) omits the alt attribute, while the HistoryContestant component (line 115) correctly includes alt={v.name}. This is a minor accessibility gap.
Proposed fix
return logo ? (
<img
key={v.name}
src={logo}
title={v.name}
+ alt={v.name}
className="voter-mini-logo"
/>
) : null;Apply to both blocks (lines 183–193 and 218–228).
Also applies to: 218-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@history.tsx` around lines 183 - 193, The voter logo <img> elements in
HistoryCard are missing alt text; update the img rendering inside the votersA
and votersB maps (the block using getLogo(v.name)) to include alt={v.name}
(matching HistoryContestant's behavior) so each <img key={v.name} src={logo} ...
/> becomes <img ... alt={v.name} /> for accessibility.
1 participant
Summary
Comprehensive code review and fixes across 14 files (15 with new
.env.example). Addresses critical security vulnerabilities, logic bugs, performance issues, and code quality improvements.Security Fixes (Critical/High)
/assets/route — could read arbitrary server files (source code, DB, .env)?secret=param support (logged in browser history, proxy logs, referer headers)?sink=WebSocket URL to localhost only/api/pauseand/api/resumeroutes (duplicated admin routes)Bug Fixes
startsWith→ exact match)Math.random()as React key in history page — caused full re-mount on every renderres.okcheck before parsing history API JSON responsesaveRound()before 5-second display delay.gitignoretypo —_.log(underscore) instead of*.log(asterisk).catch()torunGame()promisePerformance/Reliability
reasoningvariable from AI callsCode Quality
getLogo()calls per voter in history maps.env.exampledocumenting all 25+ environment variablestestandtypecheckscripts to package.jsonmodulefield (pointed to nonexistentindex.ts).dockerignoreto exclude dev files*.sqlite-shmand*.sqlite-walto.gitignoreTest plan
/assets/../server.tsreturns 403 Forbidden (path traversal fix)?secret=param)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores