-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
fix(security): admin token auth, localhost-only API, path validation, input limits, per-user port #2062
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
fix(security): admin token auth, localhost-only API, path validation, input limits, per-user port #2062
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,15 +2,97 @@ | |
| * HTTP Middleware for Worker Service | ||
| * | ||
| * Extracted from WorkerService.ts for better organization. | ||
| * Handles request/response logging, CORS, JSON parsing, and static file serving. | ||
| * Handles request/response logging, CORS, JSON parsing, static file serving, | ||
| * admin token auth, and rate limiting. | ||
| */ | ||
|
|
||
| import express, { Request, Response, NextFunction, RequestHandler } from 'express'; | ||
| import cors from 'cors'; | ||
| import crypto from 'crypto'; | ||
| import fs from 'fs'; | ||
| import path from 'path'; | ||
| import os from 'os'; | ||
| import { getPackageRoot } from '../../../shared/paths.js'; | ||
| import { logger } from '../../../utils/logger.js'; | ||
|
|
||
| /** | ||
| * Admin token file path — generated once on first startup, stored per-user. | ||
| */ | ||
| const ADMIN_TOKEN_PATH = path.join(os.homedir(), '.claude-mem', 'admin.token'); | ||
|
|
||
| /** | ||
| * Lazily-initialized admin token. Generated via crypto.randomBytes if not on disk. | ||
| */ | ||
| let cachedAdminToken: string | null = null; | ||
|
|
||
| /** | ||
| * Get or create the admin bearer token. | ||
| * On first call, reads from ~/.claude-mem/admin.token or generates a new one. | ||
| */ | ||
| export function getAdminToken(): string { | ||
| if (cachedAdminToken) return cachedAdminToken; | ||
|
|
||
| const tokenDir = path.dirname(ADMIN_TOKEN_PATH); | ||
| if (!fs.existsSync(tokenDir)) { | ||
| fs.mkdirSync(tokenDir, { recursive: true }); | ||
| } | ||
|
|
||
| if (fs.existsSync(ADMIN_TOKEN_PATH)) { | ||
| const stored = fs.readFileSync(ADMIN_TOKEN_PATH, 'utf-8').trim(); | ||
| if (stored.length >= 32) { | ||
| cachedAdminToken = stored; | ||
| return cachedAdminToken; | ||
| } | ||
| } | ||
|
|
||
| // Generate a new token | ||
| cachedAdminToken = crypto.randomBytes(32).toString('hex'); | ||
| fs.writeFileSync(ADMIN_TOKEN_PATH, cachedAdminToken, { mode: 0o600 }); | ||
| logger.info('SECURITY', 'Generated new admin token', { path: ADMIN_TOKEN_PATH }); | ||
| return cachedAdminToken; | ||
| } | ||
|
|
||
| /** | ||
| * Simple in-memory rate limiter. | ||
| * Tracks request counts per endpoint group in a sliding window. | ||
| */ | ||
| const rateLimitWindowMs = 60_000; // 1 minute | ||
| const rateLimitMaxRequests = 100; // max requests per window per endpoint group | ||
| const rateLimitBuckets = new Map<string, { count: number; resetAt: number }>(); | ||
|
|
||
| function getRateLimitGroup(requestPath: string): string { | ||
| // Group by first two path segments: /api/search, /api/data, /api/admin, etc. | ||
| const segments = requestPath.split('/').filter(Boolean); | ||
| return `/${segments.slice(0, 2).join('/')}`; | ||
| } | ||
|
|
||
| /** | ||
| * Rate limiting middleware — max 100 requests/minute per endpoint group. | ||
| */ | ||
| export function rateLimiter(req: Request, res: Response, next: NextFunction): void { | ||
| const group = getRateLimitGroup(req.path); | ||
| const now = Date.now(); | ||
|
|
||
| let bucket = rateLimitBuckets.get(group); | ||
| if (!bucket || now >= bucket.resetAt) { | ||
| bucket = { count: 0, resetAt: now + rateLimitWindowMs }; | ||
| rateLimitBuckets.set(group, bucket); | ||
| } | ||
|
|
||
| bucket.count++; | ||
|
|
||
| if (bucket.count > rateLimitMaxRequests) { | ||
| logger.warn('SECURITY', 'Rate limit exceeded', { group, count: bucket.count }); | ||
| res.status(429).json({ | ||
| error: 'Too Many Requests', | ||
| message: `Rate limit exceeded for ${group}. Max ${rateLimitMaxRequests} requests per minute.` | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| next(); | ||
| } | ||
|
|
||
| /** | ||
| * Create all middleware for the worker service | ||
| * @param summarizeRequestBody - Function to summarize request bodies for logging | ||
|
|
@@ -21,8 +103,8 @@ export function createMiddleware( | |
| ): RequestHandler[] { | ||
| const middlewares: RequestHandler[] = []; | ||
|
|
||
| // JSON parsing with 50mb limit | ||
| middlewares.push(express.json({ limit: '50mb' })); | ||
| // JSON parsing with 1mb limit (hardened from 50mb — Bug #1935) | ||
| middlewares.push(express.json({ limit: '1mb' })); | ||
|
|
||
| // CORS - restrict to localhost origins only | ||
| middlewares.push(cors({ | ||
|
|
@@ -79,30 +161,75 @@ export function createMiddleware( | |
| } | ||
|
|
||
| /** | ||
| * Middleware to require localhost-only access | ||
| * Used for admin endpoints that should not be exposed when binding to 0.0.0.0 | ||
| * Middleware to require localhost-only access. | ||
| * Uses req.socket.remoteAddress (not req.ip) to avoid X-Forwarded-For spoofing. | ||
| * Used for all API endpoints since this is a local-only tool. | ||
| */ | ||
| export function requireLocalhost(req: Request, res: Response, next: NextFunction): void { | ||
| const clientIp = req.ip || req.connection.remoteAddress || ''; | ||
| // Use socket-level address to ignore X-Forwarded-For entirely (Bug #1932) | ||
| const clientIp = req.socket.remoteAddress || ''; | ||
| const isLocalhost = | ||
| clientIp === '127.0.0.1' || | ||
| clientIp === '::1' || | ||
| clientIp === '::ffff:127.0.0.1' || | ||
| clientIp === 'localhost'; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| if (!isLocalhost) { | ||
| logger.warn('SECURITY', 'Admin endpoint access denied - not localhost', { | ||
| logger.warn('SECURITY', 'API access denied - not localhost', { | ||
| endpoint: req.path, | ||
| clientIp, | ||
| method: req.method | ||
| }); | ||
| res.status(403).json({ | ||
| error: 'Forbidden', | ||
| message: 'API endpoints are only accessible from localhost' | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| next(); | ||
| } | ||
|
|
||
| /** | ||
| * Middleware to require admin bearer token for admin endpoints. | ||
| * Admin routes must include `Authorization: Bearer <token>` header | ||
| * OR come from verified localhost (Bug #1932). | ||
| */ | ||
| export function requireAdminToken(req: Request, res: Response, next: NextFunction): void { | ||
| // First, verify localhost via socket (not req.ip) | ||
| const clientIp = req.socket.remoteAddress || ''; | ||
| const isLocalhost = | ||
| clientIp === '127.0.0.1' || | ||
| clientIp === '::1' || | ||
| clientIp === '::ffff:127.0.0.1' || | ||
| clientIp === 'localhost'; | ||
|
|
||
| if (!isLocalhost) { | ||
| res.status(403).json({ | ||
| error: 'Forbidden', | ||
| message: 'Admin endpoints are only accessible from localhost' | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| // Check bearer token if provided; if not provided, localhost is sufficient | ||
| const authHeader = req.headers.authorization; | ||
| if (authHeader) { | ||
| const token = authHeader.startsWith('Bearer ') ? authHeader.slice(7) : ''; | ||
| const expectedToken = getAdminToken(); | ||
| if (!token || token !== expectedToken) { | ||
| logger.warn('SECURITY', 'Admin endpoint: invalid bearer token', { | ||
| endpoint: req.path, | ||
| method: req.method | ||
| }); | ||
| res.status(401).json({ | ||
| error: 'Unauthorized', | ||
| message: 'Invalid admin token' | ||
| }); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| next(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The The fix is to remove the |
||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -7,10 +7,28 @@ | |||||||||||
|
|
||||||||||||
| import { readFileSync, writeFileSync, existsSync, mkdirSync } from 'fs'; | ||||||||||||
| import { join, dirname } from 'path'; | ||||||||||||
| import { homedir } from 'os'; | ||||||||||||
| import { homedir, userInfo } from 'os'; | ||||||||||||
| // NOTE: Do NOT import logger here - it creates a circular dependency | ||||||||||||
| // logger.ts depends on SettingsDefaultsManager for its initialization | ||||||||||||
|
|
||||||||||||
| /** | ||||||||||||
| * Derive a per-user default port from the OS UID to prevent cross-user | ||||||||||||
| * collisions on shared machines (Bug #1936). | ||||||||||||
| * Falls back to base port 37777 if UID is unavailable (e.g., Windows). | ||||||||||||
| */ | ||||||||||||
| function derivePerUserDefaultPort(): string { | ||||||||||||
| const basePort = 37777; | ||||||||||||
| try { | ||||||||||||
| const info = userInfo(); | ||||||||||||
| if (info.uid >= 0) { | ||||||||||||
| return String(basePort + (info.uid % 1000)); | ||||||||||||
| } | ||||||||||||
|
Comment on lines
+22
to
+25
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
| } catch { | ||||||||||||
| // userInfo() can throw on some platforms — fall back to base port | ||||||||||||
| } | ||||||||||||
| return String(basePort); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| export interface SettingsDefaults { | ||||||||||||
| CLAUDE_MEM_MODEL: string; | ||||||||||||
| CLAUDE_MEM_CONTEXT_OBSERVATIONS: string; | ||||||||||||
|
|
@@ -85,7 +103,7 @@ export class SettingsDefaultsManager { | |||||||||||
| private static readonly DEFAULTS: SettingsDefaults = { | ||||||||||||
| CLAUDE_MEM_MODEL: 'claude-sonnet-4-6', | ||||||||||||
| CLAUDE_MEM_CONTEXT_OBSERVATIONS: '50', | ||||||||||||
| CLAUDE_MEM_WORKER_PORT: '37777', | ||||||||||||
| CLAUDE_MEM_WORKER_PORT: derivePerUserDefaultPort(), | ||||||||||||
| CLAUDE_MEM_WORKER_HOST: '127.0.0.1', | ||||||||||||
| CLAUDE_MEM_SKIP_TOOLS: 'ListMcpResourcesTool,SlashCommand,Skill,TodoWrite,AskUserQuestion', | ||||||||||||
| // AI Provider Configuration | ||||||||||||
|
|
||||||||||||
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.
🧩 Analysis chain
🌐 Web query:
In Node.js, doespath.resolve()resolve symlinks or only normalize path segments? What API should be used to canonicalize a path through symlinked directories before enforcing a directory allowlist?💡 Result:
In Node.js, path.resolve only normalizes path segments (resolves '.', '..', and makes absolute using cwd if needed) but does not resolve symlinks, as it performs string manipulation without filesystem access. To canonicalize a path by resolving symlinks (for secure directory allowlist enforcement), use fs.realpath (async callback), fs.realpathSync (sync), or fs.promises.realpath (async promise). These compute the canonical pathname by resolving '.', '..', and symbolic links, equivalent to POSIX realpath(3). Example for promises: import { realpath } from 'fs/promises'; const canonicalPath = await realpath(userPath); // Check if canonicalPath starts with allowed directory This prevents symlink attacks like /allowed/../etc/passwd.
Citations:
🏁 Script executed:
Repository: thedotmack/claude-mem
Length of output: 1295
🏁 Script executed:
Repository: thedotmack/claude-mem
Length of output: 1663
🏁 Script executed:
Repository: thedotmack/claude-mem
Length of output: 828
The allowlist check is bypassable through symlinked directories.
path.resolve()on lines 369–375 only normalizes path segments without resolving symlinks. A user-controlledwatch.context.pathcontaining a symlink component (e.g.,<projectRoot>/link/AGENTS.mdwherelink→/etc/) will pass the prefix check but causewriteAgentsMd()to write outside allowed roots when the filesystem follows the symlink target. Validate the canonical path usingfs.realpathSync()orfs.realpathSync.native()before the allowlist check, and pass the canonical path towriteAgentsMd().🤖 Prompt for AI Agents