-
Notifications
You must be signed in to change notification settings - Fork 705
feat: security hardening — message validator, rate limiter, key loader #720
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,24 +1,49 @@ | ||||||||||||||||||
| import { readFileSync } from 'fs'; | ||||||||||||||||||
| import { deepSanitize } from '../../settings.js'; | ||||||||||||||||||
|
|
||||||||||||||||||
| let keys = {}; | ||||||||||||||||||
| try { | ||||||||||||||||||
| const data = readFileSync('./keys.json', 'utf8'); | ||||||||||||||||||
| keys = JSON.parse(data); | ||||||||||||||||||
| } catch (err) { | ||||||||||||||||||
| console.warn('keys.json not found. Defaulting to environment variables.'); // still works with local models | ||||||||||||||||||
| let keysLoaded = false; | ||||||||||||||||||
| const _warnedKeys = new Set(); | ||||||||||||||||||
|
|
||||||||||||||||||
| // Try to load keys.json only if it exists, but prefer environment variables | ||||||||||||||||||
| function loadKeysFile() { | ||||||||||||||||||
| if (keysLoaded) return; | ||||||||||||||||||
| try { | ||||||||||||||||||
| const data = readFileSync('./keys.json', 'utf8'); | ||||||||||||||||||
| keys = deepSanitize(JSON.parse(data)); | ||||||||||||||||||
| console.warn('⚠️ WARNING: keys.json loaded into memory. Use environment variables instead for better security.'); | ||||||||||||||||||
| } catch (_err) { | ||||||||||||||||||
| // keys.json not found or unreadable — that's fine, use env vars | ||||||||||||||||||
|
Comment on lines
+15
to
+16
|
||||||||||||||||||
| } catch (_err) { | |
| // keys.json not found or unreadable — that's fine, use env vars | |
| } catch (err) { | |
| // Only ignore missing file; other errors should be surfaced | |
| if (!err || err.code !== 'ENOENT') { | |
| console.error('Failed to load keys.json:', err); | |
| throw err; | |
| } |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,97 @@ | ||||||||||
| /** | ||||||||||
| * Validates and sanitizes user messages for safety. | ||||||||||
| * Prevents protocol exploits, injection attacks, and abuse. | ||||||||||
| */ | ||||||||||
|
|
||||||||||
| const MAX_DISCORD_MESSAGE_LENGTH = 512; | ||||||||||
| const MAX_MINECRAFT_MESSAGE_LENGTH = 256; | ||||||||||
|
|
||||||||||
| // Patterns that may indicate command injection or abuse | ||||||||||
| const COMMAND_INJECTION_PATTERNS = [ | ||||||||||
| /;\s*(rm|del|format|shutdown|reboot|kill|wget|curl)\b/i, | ||||||||||
| /\$\(.*\)/, // Shell command substitution | ||||||||||
| /`[^`]*`/, // Backtick command execution | ||||||||||
| /\|\s*(bash|sh|cmd|powershell)/i, // Pipe to shell | ||||||||||
| ]; | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Validates a Discord message | ||||||||||
| * @param {string} message - Raw message from Discord user | ||||||||||
| * @returns {object} { valid: boolean, error?: string, sanitized?: string, warnings?: string[] } | ||||||||||
| */ | ||||||||||
| export function validateDiscordMessage(message) { | ||||||||||
| if (!message) return { valid: false, error: 'Empty message' }; | ||||||||||
| if (typeof message !== 'string') return { valid: false, error: 'Message must be a string' }; | ||||||||||
| if (message.length > MAX_DISCORD_MESSAGE_LENGTH) { | ||||||||||
|
Comment on lines
+22
to
+25
|
||||||||||
| return { valid: false, error: `Message exceeds ${MAX_DISCORD_MESSAGE_LENGTH} characters` }; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // Block command injection patterns outright — do not pass these to agents. | ||||||||||
| for (const pattern of COMMAND_INJECTION_PATTERNS) { | ||||||||||
| if (pattern.test(message)) { | ||||||||||
| console.warn(`[MessageValidator] Blocked message containing disallowed pattern: ${pattern.source}`); | ||||||||||
| return { valid: false, error: `Message contains a disallowed pattern and was blocked.` }; | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| const warnings = []; | ||||||||||
|
|
||||||||||
| // Alert on suspicious patterns (but allow them for agents to handle) | ||||||||||
| const suspiciousPatterns = [ | ||||||||||
| /[\x00-\x08\x0B-\x0C\x0E-\x1F]/g, // Control characters | ||||||||||
| /[\uFEFF]/g, // BOM | ||||||||||
| ]; | ||||||||||
|
|
||||||||||
| let sanitized = message; | ||||||||||
| for (const pattern of suspiciousPatterns) { | ||||||||||
| sanitized = sanitized.replace(pattern, ''); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| if (sanitized !== message) { | ||||||||||
| warnings.push('Removed control characters from message'); | ||||||||||
| console.warn('[MessageValidator] Removed control characters from Discord message'); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return { valid: true, sanitized, warnings: warnings.length > 0 ? warnings : undefined }; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Validates a Minecraft in-game message | ||||||||||
| * @param {string} message - Raw message from Minecraft chat | ||||||||||
| * @returns {object} { valid: boolean, error?: string, sanitized?: string } | ||||||||||
| */ | ||||||||||
| export function validateMinecraftMessage(message) { | ||||||||||
| if (!message) return { valid: false, error: 'Empty message' }; | ||||||||||
| if (typeof message !== 'string') return { valid: false, error: 'Message must be a string' }; | ||||||||||
| if (message.length > MAX_MINECRAFT_MESSAGE_LENGTH) { | ||||||||||
|
Comment on lines
+63
to
+66
|
||||||||||
| return { valid: false, error: `Message exceeds ${MAX_MINECRAFT_MESSAGE_LENGTH} characters` }; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // Minecraft chat allows most characters; filter only dangerous ones | ||||||||||
| const dangerousPatterns = [ | ||||||||||
| /[\x00]/g, // Null byte (protocol exploit) | ||||||||||
| /\n/g, // Newlines (message splitting) | ||||||||||
| /\r/g, // Carriage return | ||||||||||
| ]; | ||||||||||
|
|
||||||||||
| let sanitized = message; | ||||||||||
| for (const pattern of dangerousPatterns) { | ||||||||||
| sanitized = sanitized.replace(pattern, ' '); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return { valid: true, sanitized }; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Validates a username (bot or player name) | ||||||||||
| * @param {string} username - Username to validate | ||||||||||
| * @returns {object} { valid: boolean, error?: string } | ||||||||||
| */ | ||||||||||
| export function validateUsername(username) { | ||||||||||
| if (!username) return { valid: false, error: 'Username is empty' }; | ||||||||||
| if (typeof username !== 'string') return { valid: false, error: 'Username must be a string' }; | ||||||||||
|
Comment on lines
+91
to
+92
|
||||||||||
| if (!username) return { valid: false, error: 'Username is empty' }; | |
| if (typeof username !== 'string') return { valid: false, error: 'Username must be a string' }; | |
| if (typeof username !== 'string') return { valid: false, error: 'Username must be a string' }; | |
| if (username.length === 0) return { valid: false, error: 'Username is empty' }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| /** | ||
| * Simple in-memory rate limiter to prevent abuse. | ||
| * Includes automatic stale entry cleanup to prevent memory leaks. | ||
| */ | ||
|
|
||
| export class RateLimiter { | ||
| constructor(maxRequests = 5, windowMs = 60000, cleanupIntervalMs = 300000) { | ||
| this.maxRequests = maxRequests; // Max requests per window | ||
| this.windowMs = windowMs; // Time window in milliseconds | ||
| this.requests = new Map(); // userId → [timestamps] | ||
|
|
||
| // Periodically purge stale entries to prevent unbounded memory growth | ||
| this._cleanupInterval = setInterval(() => this._purgeStale(), cleanupIntervalMs); | ||
| // Allow the process to exit even if the interval is still running | ||
| if (this._cleanupInterval.unref) { | ||
| this._cleanupInterval.unref(); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Check if a user has exceeded rate limit | ||
| * @param {string} userId - Discord user ID | ||
| * @returns {object} { allowed: boolean, retryAfterSeconds?: number } | ||
| */ | ||
| checkLimit(userId) { | ||
| const now = Date.now(); | ||
| const userRequests = this.requests.get(userId) || []; | ||
|
|
||
| // Remove old requests outside the window | ||
| const recentRequests = userRequests.filter(ts => now - ts < this.windowMs); | ||
|
|
||
| if (recentRequests.length >= this.maxRequests) { | ||
| // Rate limited | ||
| const oldestRequest = recentRequests[0]; | ||
| const retryAfterMs = oldestRequest + this.windowMs - now; | ||
| const retryAfterSeconds = Math.ceil(retryAfterMs / 1000); | ||
| return { allowed: false, retryAfterSeconds }; | ||
| } | ||
|
|
||
| // Allow and record | ||
| recentRequests.push(now); | ||
| this.requests.set(userId, recentRequests); | ||
| return { allowed: true }; | ||
| } | ||
|
|
||
| /** | ||
| * Reset limits for a user (useful for admins) | ||
| */ | ||
| reset(userId) { | ||
| this.requests.delete(userId); | ||
| } | ||
|
|
||
| /** | ||
| * Reset all tracked users | ||
| */ | ||
| resetAll() { | ||
| this.requests.clear(); | ||
| } | ||
|
|
||
| /** | ||
| * Get current stats for a user | ||
| */ | ||
| getStats(userId) { | ||
| const now = Date.now(); | ||
| const requests = this.requests.get(userId) || []; | ||
| const recentRequests = requests.filter(ts => now - ts < this.windowMs); | ||
| return { | ||
| current: recentRequests.length, | ||
| max: this.maxRequests, | ||
| windowSeconds: this.windowMs / 1000, | ||
| trackedUsers: this.requests.size, | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Remove entries for users with no recent requests (prevents memory leak) | ||
| * @private | ||
| */ | ||
| _purgeStale() { | ||
| const now = Date.now(); | ||
| for (const [userId, timestamps] of this.requests) { | ||
| const recent = timestamps.filter(ts => now - ts < this.windowMs); | ||
| if (recent.length === 0) { | ||
| this.requests.delete(userId); | ||
| } else { | ||
| this.requests.set(userId, recent); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Stop the automatic cleanup interval (for graceful shutdown) | ||
| */ | ||
| destroy() { | ||
| if (this._cleanupInterval) { | ||
| clearInterval(this._cleanupInterval); | ||
| this._cleanupInterval = null; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deepSanitizeis imported from../../settings.js, butsettings.jsonly exports a defaultsettingsobject and does not provide a nameddeepSanitizeexport. This will throw at module load time and break every model that importsgetKey. Define/exportdeepSanitize(or import it from the correct module) before using it here.