-
Notifications
You must be signed in to change notification settings - Fork 705
security: add input validation, rate limiting, and prototype pollution guard #718
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 |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ import settings from './settings.js'; | |
| import { Task } from './tasks/tasks.js'; | ||
| import { speak } from './speak.js'; | ||
| import { log, validateNameFormat, handleDisconnection } from './connection_handler.js'; | ||
| import { validateMinecraftMessage, validateUsername } from '../utils/message_validator.js'; | ||
|
|
||
| export class Agent { | ||
| async start(load_mem=false, init_message=null, count_id=0) { | ||
|
|
@@ -157,19 +158,34 @@ export class Agent { | |
| const respondFunc = async (username, message) => { | ||
| if (message === "") return; | ||
| if (username === this.name) return; | ||
|
|
||
| // Validate username and message before processing | ||
| const userValidation = validateUsername(username); | ||
| if (!userValidation.valid) { | ||
| console.warn(`[MessageValidator] Rejected message from invalid username: "${username}" (${userValidation.error})`); | ||
| return; | ||
| } | ||
|
|
||
| const msgValidation = validateMinecraftMessage(message); | ||
| if (!msgValidation.valid) { | ||
| console.warn(`[MessageValidator] Rejected message: ${msgValidation.error}`); | ||
| return; | ||
| } | ||
| const cleanMessage = msgValidation.sanitized; | ||
|
|
||
| if (settings.only_chat_with.length > 0 && !settings.only_chat_with.includes(username)) return; | ||
| try { | ||
| if (ignore_messages.some((m) => message.startsWith(m))) return; | ||
| if (ignore_messages.some((m) => cleanMessage.startsWith(m))) return; | ||
|
|
||
|
Comment on lines
+174
to
179
|
||
| this.shut_up = false; | ||
|
|
||
| console.log(this.name, 'received message from', username, ':', message); | ||
| console.log(this.name, 'received message from', username, ':', cleanMessage); | ||
|
|
||
| if (convoManager.isOtherAgent(username)) { | ||
| console.warn('received whisper from other bot??') | ||
| } | ||
| else { | ||
| let translation = await handleEnglishTranslation(message); | ||
| let translation = await handleEnglishTranslation(cleanMessage); | ||
| this.handleMessage(username, translation); | ||
| } | ||
| } catch (error) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,99 @@ | ||||||||||||||||
| /** | ||||||||||||||||
| * 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) { | ||||||||||||||||
| return { valid: false, error: `Message exceeds ${MAX_DISCORD_MESSAGE_LENGTH} characters` }; | ||||||||||||||||
| } | ||||||||||||||||
|
Comment on lines
+22
to
+27
|
||||||||||||||||
|
|
||||||||||||||||
| // 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 = [ | ||||||||||||||||
| // eslint-disable-next-line no-control-regex | ||||||||||||||||
| /[\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) { | ||||||||||||||||
| return { valid: false, error: `Message exceeds ${MAX_MINECRAFT_MESSAGE_LENGTH} characters` }; | ||||||||||||||||
| } | ||||||||||||||||
|
Comment on lines
+64
to
+69
|
||||||||||||||||
|
|
||||||||||||||||
| // Minecraft chat allows most characters; filter only dangerous ones | ||||||||||||||||
| const dangerousPatterns = [ | ||||||||||||||||
| // eslint-disable-next-line no-control-regex | ||||||||||||||||
| /[\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' }; | ||||||||||||||||
| if (!/^[a-zA-Z0-9_]{3,16}$/.test(username)) { | ||||||||||||||||
|
Comment on lines
+93
to
+95
|
||||||||||||||||
| if (!username) return { valid: false, error: 'Username is empty' }; | |
| if (typeof username !== 'string') return { valid: false, error: 'Username must be a string' }; | |
| if (!/^[a-zA-Z0-9_]{3,16}$/.test(username)) { | |
| if (typeof username !== 'string') return { valid: false, error: 'Username must be a string' }; | |
| const trimmedUsername = username.trim(); | |
| if (!trimmedUsername) return { valid: false, error: 'Username is empty' }; | |
| if (!/^[a-zA-Z0-9_]{3,16}$/.test(trimmedUsername)) { |
| 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(); | ||
| } | ||
| } | ||
|
Comment on lines
+1
to
+18
|
||
|
|
||
| /** | ||
| * 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.
deepSanitizerecreatesBLOCKED_KEYSon every recursive call, which is unnecessary work (especially on large nested inputs). MoveBLOCKED_KEYSto a module-level constant (or at least outside the function body) so recursion reuses the same set.