fix: production-readiness overhaul — boot fix, security hardening, CI, /wipe rewrite - #28
Merged
Merged
Conversation
- Add express-rate-limit (5 req/min/IP) on POST /issue and POST /data
- Set trust proxy so rate limiting keys on real client IPs behind a reverse proxy
- Add per-field length caps (title 200, description 8KB, etc.)
- Add hidden honeypot field ("company") to both report forms; silently drop hits
- Add optional ALLOWED_ORIGINS allowlist for origin/referer checking
- Validate GITHUB_TOKEN, REPO_DEV, REPO_DATA_REPORT at startup (fail fast)
- Fix DATA_REPO default: tarkov-data-overlay (hyphenated), not tarkov-data/overlay
- Remove undocumented GITHUB_REPO fallback
- Sanitize error logging: log only status + message, never the full axios error
object (which contains the raw Authorization header with the GitHub token)
- Fix .env.example: typo (YOUR_DICORD_TOKEN), remove unused GUILD_ID/SUPPORT_CHANNEL,
add ADMIN_IDS and ALLOWED_ORIGINS, add comments and grouping
Deployment (restored from 5e84a83, rewritten for main branch): - New ecosystem.config.cjs with TWO separate PM2 apps (bot.js, webserver.js) instead of the old single "npm start" app — this fixes the production dependency on concurrently (devDependency) that broke npm start under npm ci --omit=dev - New deploy-prod.sh targeting main (not the deleted prod branch) with npm ci --omit=dev for production installs - New deploy-prod.yml triggered on push to main + workflow_dispatch, with a concurrency group to prevent overlapping deploys; uses ubuntu-latest + SSH (safer than self-hosted runner for a public repo) Crash resilience: - Add try/catch error boundary in interactionHandler.js: unhandled rejections no longer crash the process (Node default is process exit); user gets a generic ephemeral error reply - Add inGuild() guard: slash commands and buttons require a guild context; interaction.member is null in DMs and would throw TypeError. A stranger DM-ing /faq1 could previously crash the bot Cleanup: - Import REST from discord.js instead of @discordjs/rest (was an undeclared transitive dependency) - Remove unused multer dependency
Replaces the broken /wipe from PR #27 (which was an invalid module with no handler wiring) with a proper implementation: - Valid exported module (executeWipe), wired into slashHandlers.js - Admin check via ADMIN_IDS env var (guarded against unset/empty values) - Confirmation dialog with Confirm + Cancel buttons using ButtonBuilder - Uses awaitMessageComponent on the reply (one-shot, no cross-file state) - Batched deletion: loops fetch(100) + bulkDelete(fetched, true) until empty or MAX_DELETE_BATCHES reached; filterOld=true skips >14-day messages instead of throwing - Honest description: "Bulk-delete recent messages (14-day limit)" - Uses MessageFlags.Ephemeral (named constant, not deprecated ephemeral:true) - Channel type guard (GuildText only) - Timeout and error handling with sanitized editReply fallbacks
Code consistency: - Standardize ephemeral flags: replace flags: 64 with MessageFlags.Ephemeral across slashHandlers.js and buttonHandlers.js for readability - Translate French comments to English in slashHandlers.js (put-member-role) - Make allowedCommandRoles env-configurable via ALLOWED_COMMAND_ROLE_IDS (falls back to the existing hardcoded role ID) - Document the in-memory welcomeMessages Map limitation in memberHandlers.js Metadata: - Fill in package.json description, keywords, author, engines (node >=18) - Change npm test from no-op to "node --test" Tests: - Add test/env.test.js: smoke tests for env.js validation (getRequiredEnv, ensureEnvVars pass/fail cases). 4 tests, all passing. - Add "Run tests" step to ci.yml so tests run on PRs and pushes Docs: - README: add /wipe to command table, add markdown H1 above the HTML banner, label the project-structure code fence as text, update env var list to include ADMIN_IDS and ALLOWED_ORIGINS, recommend fine-grained PAT CI: - Fix update-license-year.yml sed regex to handle year ranges (2025-2026) in addition to single years
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds CI and package metadata updates, reworks ChangesCI and Package Infrastructure
Wipe Command and Environment Configuration
Interaction Routing and Ephemeral Replies
Webserver Security Hardening
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
codeant-ai
Bot
added
the
size:XL
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Closed
Addresses CodeRabbit and CodeAnt review comments on PR #28: Security: - webserver: fix origin allowlist bypass — was using startsWith (prefix match) which let https://tarkovtracker.org.evil.com through; now uses exact match + trailing-slash boundary. Also normalizes trailing slashes on allowlist entries at config time. (CR11/CA3) - webserver: split rate limiter into separate dataLimiter and issueLimiter instances so /data and /issue have independent per-IP quotas. (CR12) - webserver: add 10s timeout to both GitHub API axios.post calls to prevent indefinite hangs. (CR13) Correctness: - interactionHandler: handle deferred interactions in error recovery — was calling interaction.reply() after deferReply() which throws InteractionAlreadyReplied; now branches on interaction.deferred to use editReply() instead. No flags on editReply (ephemerality is fixed at defer time). (CR10/CA2) - wipeCommand: report partial wipes when MAX_DELETE_BATCHES cap is hit, instead of always saying "Channel Wiped". (CR8) - wipeCommand: remove overly broad err.message.includes("time") fallback in timeout check — rely only on err.code === "InteractionCollectorError" as verified empirically on discord.js 14.26.4. (CR9) Config: - ecosystem.config.cjs: set NODE_ENV=production on both PM2 apps so Express runs in production mode (no stack traces in error responses). (CR5) Tests: - test/env.test.js: add afterEach cleanup to delete all env vars set during tests, preventing state leakage. (CR14) Docs: - README: document comma-separated format for ADMIN_IDS and ALLOWED_ORIGINS, note ALLOWED_ORIGINS uses exact match. (CR6) - README: /wipe description now says "Confirmed bulk-delete" to indicate the confirmation step. (CR7)
- ci.yml: add concurrency group to cancel superseded runs on rapid pushes and PRs (mirrors the pattern in deploy-prod.yml). (CR1) - ci.yml: add persist-credentials: false to actions/checkout to prevent git credential persistence before npm ci runs third-party postinstall scripts. SHA pinning of actions deferred to a future PR with Dependabot/Renovate setup. (CR2) - deploy-prod.yml: move SSH secrets out of inline shell interpolation into env: blocks, reducing template-injection risk (flagged by zizmor). Multi-line SSH key is double-quoted to preserve newlines. (CR3)
Member
Author
Review feedback addressed — commits
|
| Comment | Reviewer | Resolution |
|---|---|---|
| Origin allowlist bypassable (CR11/CA3) | CodeRabbit + CodeAnt | Fixed: exact match + trailing-slash boundary, with trailing-slash normalization on allowlist entries at config time. Blocks https://tarkovtracker.org.evil.com. |
| Deferred interaction error recovery (CR10/CA2) | CodeRabbit + CodeAnt | Fixed: catch block now branches on interaction.deferred → uses editReply() (no flags, since ephemerality is fixed at defer time). |
| No timeout on GitHub API calls (CR13) | CodeRabbit | Fixed: timeout: 10_000 on both axios.post calls. |
| Shared rate limiter (CR12) | CodeRabbit | Fixed: split into dataLimiter + issueLimiter with independent per-IP quotas. |
| Partial wipe not reported (CR8) | CodeRabbit | Fixed: hitCap flag appended to success message when MAX_DELETE_BATCHES is reached. |
| Timeout check too broad (CR9) | CodeRabbit | Fixed: removed err.message?.includes("time") fallback; relies only on err.code === "InteractionCollectorError". |
| NODE_ENV not set (CR5) | CodeRabbit | Fixed: env: { NODE_ENV: "production" } on both PM2 apps. |
| Test env var cleanup (CR14) | CodeRabbit | Fixed: afterEach hook deletes all env vars set during tests. |
| README env var format (CR6) | CodeRabbit | Fixed: documented comma-separated format for ADMIN_IDS and ALLOWED_ORIGINS (exact match). |
| README /wipe confirmation (CR7) | CodeRabbit | Fixed: "Confirmed bulk-delete" in command table. |
| CI concurrency (CR1) | CodeRabbit | Fixed: added concurrency block with cancel-in-progress: true. |
| Checkout persist-credentials (CR2) | CodeRabbit | Fixed: persist-credentials: false on checkout step. |
| Deploy secrets via env (CR3) | CodeRabbit | Fixed: all SSH secrets moved to env: blocks; multi-line key double-quoted. |
Deferred (2 comments — not blocking this PR)
| Comment | Reason |
|---|---|
| SHA pinning of actions (CR2 partial) | Needs Dependabot/Renovate setup to keep SHAs updated. Stale SHA pins are worse than moving tags for a single-maintainer repo. Will address in a separate PR with the tooling. |
| Deploy atomicity / symlink release (CR4) | Needs server-side testing and Niv's involvement. Current set -e + PM2 autorestart is adequate at this scale. Future PR. |
False positive (1 comment — no change needed)
| Comment | Reason |
|---|---|
awaitMessageComponent on interaction.reply() return value (CA1) |
Verified empirically on discord.js 14.26.4: interaction.reply() returns an InteractionResponse by default (not void), and InteractionResponse.prototype.awaitMessageComponent is a function. Adding withResponse: true would actually break the code (returns InteractionCallbackResponse, which lacks awaitMessageComponent). The current code is the idiomatic v14 pattern. |
All CI checks, syntax checks, and tests pass on the updated branch.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Member
|
Removing deploy-prod because there is already Coolify who auto deploy it when there is a commit on main |
Nivmizz7
approved these changes
Jul 8, 2026
DysektAI
changed the title
Flagged by github-code-quality reviewer. PermissionFlagsBits was imported but never referenced in slashHandlers.js.
Member
Author
Post-review fixup (commit e31a9a5)Fixed:
False positive addressed:
PR description updated:
|
Nivmizz7
approved these changes
Jul 8, 2026
…D_COMMAND_ROLE_IDS
Member
Author
Final production-readiness check — PR #28@Nivmizz7 — final pass complete. Everything previously flagged is now fixed, verified, or resolved: Verified fixed (threads resolved):
Small fixup pushed ( Checks: CI, CodeQL, CodeRabbit, Socket, and both Snyk checks are all green; tests pass locally (4/4). Three things I'd like you to confirm before merge:
The stale CodeRabbit "changes requested" review may still show on the PR — all its comments are addressed and its status check is green, so a dismissal or re-review should clear it if branch protection counts it. Disclaimer: this review and comment were made with the help of Claude Fable 5. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
48-53: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winClarify
ALLOWED_ORIGINSmatching semantics.The webserver does not use exact-match-only checking here; it trims trailing slashes and also accepts the configured origin with a
/-bounded path. Please update the README wording so admins configure this correctly.Suggested wording
-- **Web server:** `PORT`, `ALLOWED_ORIGINS` (comma-separated origins, exact match) +- **Web server:** `PORT`, `ALLOWED_ORIGINS` (comma-separated URL prefixes; trailing slashes are ignored)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 48 - 53, The README description for ALLOWED_ORIGINS is inaccurate because the web server does not enforce exact-match-only origin checks. Update the wording in the configuration section to reflect the actual matching behavior: origins are normalized by trimming trailing slashes and may also match the configured origin with a path segment bounded by “/”. Refer to the ALLOWED_ORIGINS entry in the Web server settings so admins know how to format allowed origins correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@README.md`:
- Around line 48-53: The README description for ALLOWED_ORIGINS is inaccurate
because the web server does not enforce exact-match-only origin checks. Update
the wording in the configuration section to reflect the actual matching
behavior: origins are normalized by trimming trailing slashes and may also match
the configured origin with a path segment bounded by “/”. Refer to the
ALLOWED_ORIGINS entry in the Web server settings so admins know how to format
allowed origins correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d4434f6f-60d1-46b7-abd1-dcedc506ab64
📒 Files selected for processing (2)
.env.exampleREADME.md
…N_ROLE_ID) Replace the per-user ADMIN_IDS list with a reusable ADMIN_ROLE_ID env var (comma-separated Discord role IDs). /wipe now checks role membership via a shared hasAdminRole() helper in constants.js, so future admin-only commands can reuse the same gate without maintaining a user list or requiring full Discord server admin privileges.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/wipeCommand.js (1)
62-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded hex color instead of shared constant.
The confirmation embed uses a raw
0xffb347while other embeds in this file usecolors.error/colors.success/colors.info. If acolors.warning(or similar) exists, use it for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/wipeCommand.js` around lines 62 - 76, The confirm embed in wipeCommand uses a hardcoded hex color instead of the shared color palette. Update the createCardEmbed call in wipeCommand to use the existing colors constant pattern (for example colors.warning if available, matching the other colors.error/colors.success/colors.info usages) so the confirmation card stays consistent with the rest of the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/commands/wipeCommand.js`:
- Around line 62-76: The confirm embed in wipeCommand uses a hardcoded hex color
instead of the shared color palette. Update the createCardEmbed call in
wipeCommand to use the existing colors constant pattern (for example
colors.warning if available, matching the other
colors.error/colors.success/colors.info usages) so the confirmation card stays
consistent with the rest of the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4e9432a3-e690-4590-812b-5668e5d3cce2
📒 Files selected for processing (6)
.env.exampleREADME.mdsrc/commands/wipeCommand.jssrc/config/constants.jssrc/config/env.jstest/env.test.js
Replace inline 0xffb347 with colors.warning in the shared colors constant, matching the pattern used by colors.error/success/info elsewhere in the file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A full production-readiness review found that
mainwas not bootable (PR #27 introduced syntax errors), had no CI (deployment was deleted in a "styles" refactor), and had an unauthenticated, unrate-limited GitHub-issue-creation endpoint with a token-leak path in error logs. This PR fixes all of it, organized so each concern is independently reviewable.Commit breakdown
36877e6/wipeprevented bot startupc0811c9ci.yml:node --checkon all JS +npm ci+ tests on every PR/push to main9363f4041ca9260da4702/wipedone right — proper module, confirmation, batched deletionc6d72be1145e14+79b7947075763f–02c9251Critical fixes
];array terminator inregisterSlashCommands.jsand added an invalidwipeCommand.js(bareasync execute()with no export). Reverted;/wipere-added cleanly.ci.ymlwithnode --checkon every JS file +npm test. This would have caught the SyntaxError before merge.Security hardening (webserver)
express-rate-limit(5 req/min/IP) onPOST /issueandPOST /datawith separate limiter instances so each endpoint has an independent quota;trust proxyconfigured for reverse-proxy environmentsconsole.error(err.response?.data || err); the full axios error object containserr.config.headers.Authorization(the raw GitHub token). Now logs onlystatus+messagecompanyfield in both forms; bots fill it, real users don't; hits are silently droppedALLOWED_ORIGINSenv var; uses exact match (ororigin + "/"path prefix) — blockshttps://tarkovtracker.org.evil.com-style bypassesGITHUB_TOKEN,REPO_DEV,REPO_DATA_REPORTvalidated at startup (fail fast instead of silent 500s)timeout: 10_000on bothaxios.postcallstarkov-data-overlay(hyphenated, verified via GitHub API)Crash resilience
interaction.memberisnullin DMs →TypeError→ unhandled rejection → Node process exit. AddedinGuild()guard + try/catch error boundary ininteractionHandler.js.editReply()correctly./wiperewriteThe original was an invalid module with no handler wiring. The new version:
executeWipefunction, wired intoslashHandlers.jsADMIN_IDSenv var (guarded against unset/empty)ButtonBuilder,awaitMessageComponent)fetch(100)+bulkDelete(fetched, true)until empty or cap reached;filterOld=trueskips >14-day messages instead of throwingMessageFlags.Ephemeral(named constant, not deprecatedephemeral: true)Deployment
Coolify handles deployment automatically — it deploys on every push to
main. The SSH-based deploy workflow and PM2 config files that were added earlier in this branch have been removed as they are no longer needed.Polish
PermissionFlagsBitsimport fromslashHandlers.jsflags: 64→MessageFlags.Ephemeralacross all handlersslashHandlers.jsallowedCommandRolesnow env-configurable viaALLOWED_COMMAND_ROLE_IDSpackage.jsonmetadata (description, keywords, author,engines)env.jsvalidation (4 tests, passing)update-license-year.ymlsed regex to handle year ranges (2025-2026)/wipeto command table, markdown H1, labeled code fence, updated env var docsStill needs Niv's sign-off
GITHUB_TOKENscope — recommend downscoping to a fine-grained PAT withissues:writeon the two target repos onlyREPO_DEVvalue — verify the production env var points to the correct repo in CoolifyALLOWED_ORIGINS— set to the production domain(s) in Coolify env/wipe— if PR feat: add wipe command #27 ever registered/wipeglobally, it may need manual unregistering. New code re-registers correctly on next boot.Test plan
node --checknode --test)webserver.jsenv validation tested (fails fast on missing vars)discord.jsv14.26.4 API usage verified (awaitMessageComponentonInteractionResponse,MessageFlags.Ephemeral === 64)REPO_DEVandALLOWED_ORIGINSenv values in Coolify/health,/issueand/dataforms work