fix(security): admin token auth, localhost-only API, path validation, input limits, per-user port#2062
fix(security): admin token auth, localhost-only API, path validation, input limits, per-user port#2062thedotmack wants to merge 2 commits intomainfrom
Conversation
… input limits, per-user port - #1932: Add bearer token auth for admin endpoints (crypto.randomBytes stored in ~/.claude-mem/admin.token), use req.socket.remoteAddress instead of req.ip to prevent X-Forwarded-For spoofing, disable trust proxy - #1933: Apply requireLocalhost middleware to ALL /api/* routes at the app level, not just admin endpoints - #1934: Validate watch.context.path stays within project root or ~/.claude-mem/ after resolution, blocking path traversal attacks - #1935: Reduce JSON body limit from 50MB to 1MB, add in-memory rate limiting (100 req/min per endpoint group), truncate observation payloads to 10KB before storage, sanitize injected content by escaping system-like XML tags - #1936: Derive per-user default port from OS UID (basePort + uid % 1000) to prevent cross-user data leakage on shared machines Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 13 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThe changes introduce admin token authentication, rate limiting, request body size constraints, and path-safety validation across the middleware and service layers. New exports enable token-based access control, sliding-window rate limiting per endpoint group, and payload truncation. A per-user port derivation mechanism replaces fixed ports, and content sanitization prevents XML injection attacks. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RateLimiter as Rate Limiter<br/>(Middleware)
participant AuthCheck as requireAdminToken<br/>(Middleware)
participant Server as API Endpoint<br/>(Server)
Client->>RateLimiter: HTTP Request /api/admin/restart
alt Rate limit exceeded
RateLimiter->>Client: 429 Too Many Requests
else Rate limit OK
RateLimiter->>AuthCheck: Pass to next middleware
alt Non-localhost request
AuthCheck->>Client: 403 Forbidden (non-local)
else Localhost request
alt Bearer token provided
AuthCheck->>AuthCheck: Validate token vs getAdminToken()
alt Invalid token
AuthCheck->>Client: 401 Unauthorized
else Valid token
AuthCheck->>Server: Proceed to endpoint
Server->>Client: 200 OK (restart/shutdown/doctor)
end
else No token (localhost permitted)
AuthCheck->>Server: Proceed to endpoint
Server->>Client: 200 OK
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Review — Security Hardening (#1932–#1936)Good direction overall — this closes real attack surface. A few issues worth addressing before merging. 🔴 Critical
// Check bearer token if provided; if not provided, localhost is sufficient
const authHeader = req.headers.authorization;
if (authHeader) { ... }
next(); // ← passes through for ANY localhost caller with no Authorization headerSince if (!authHeader || !authHeader.startsWith('Bearer ')) {
return res.status(401).json({ error: 'Unauthorized', message: 'Admin token required' });
}
const token = authHeader.slice(7);
// ... constant-time compareThe comment doc ("OR come from verified localhost") perpetuates the confusion — pick one model and document it clearly. 🟡 Medium
const bytes = Buffer.from(payload, 'utf8');
if (bytes.byteLength <= MAX_OBSERVATION_PAYLOAD_BYTES) return payload;
return bytes.subarray(0, MAX_OBSERVATION_PAYLOAD_BYTES).toString('utf8') + '\n[... truncated at 10KB]';
Every unique path group adds an entry. Buckets whose window has expired sit in memory forever (they're only reset when the same group is hit again). For a long-running worker this is low risk in practice (bounded by path diversity), but a simple periodic cleanup or max-entries cap would be safer: // Prune stale buckets occasionally
if (rateLimitBuckets.size > 500) {
const now = Date.now();
for (const [k, v] of rateLimitBuckets) {
if (now >= v.resetAt) rateLimitBuckets.delete(k);
}
}Per-user port collision on Linux (
🟢 Minor / NitpickLocalhost check duplicated in Since
const valid = token.length === expectedToken.length &&
crypto.timingSafeEqual(Buffer.from(token), Buffer.from(expectedToken));
The function is private — that's fine — but it only covers three known tag names. Any future system tags added to the codebase won't be covered automatically. Consider a more general pattern (e.g., match What's done well
🤖 Generated with Claude Code |
Greptile SummaryThis PR hardens the local HTTP server against five security issues: spoofing via
Confidence Score: 4/5Safe to merge after fixing the admin token bypass; remaining findings are minor. One P1 finding: the admin bearer token is only validated when the header is present, so any localhost process can invoke shutdown/restart without a token. The path traversal fix, payload limits, localhost enforcement, and per-user port derivation are all correct. The P2 port-collision concern (uid % 1000) and dead-code localhost string check don't block merge but should be addressed. src/services/worker/http/middleware.ts — requireAdminToken token-bypass logic needs fixing before merge.
|
| Filename | Overview |
|---|---|
| src/services/worker/http/middleware.ts | Core security hardening: localhost socket check, bearer token for admin routes, rate limiter, reduced JSON limit — but token is optional for localhost callers, leaving admin endpoints unprotected against any local process. |
| src/services/server/Server.ts | Disables trust proxy and applies requireLocalhost + rateLimiter to all /api/* routes at the app level — clean, correct fix for #1933. |
| src/shared/SettingsDefaultsManager.ts | Per-user port via uid % 1000 prevents same-name collision on most systems, but the 1000-slot modulo space allows collisions for users whose UIDs differ by exactly 1000. |
| src/services/transcripts/processor.ts | Path traversal guard added for watch.context.path — resolves path and verifies it stays within projectRoot or ~/.claude-mem before writing AGENTS.md. |
| src/utils/claude-md-utils.ts | Adds truncateObservationPayload (10KB cap) and sanitizeInjectedContent (escapes system XML tags) — straightforward and correct for the stated goals. |
| src/services/sqlite/observations/store.ts | Calls truncateObservationPayload on narrative and each fact before storage — correctly wires up the 10KB payload limit from claude-md-utils. |
| src/services/server/Middleware.ts | Re-export shim exposing the new getAdminToken and rateLimiter exports — no logic, backward-compatible. |
Sequence Diagram
sequenceDiagram
participant C as Caller
participant MW as requireLocalhost
participant RL as rateLimiter
participant AT as requireAdminToken
participant H as Route Handler
C->>MW: POST /api/admin/shutdown
MW->>MW: Check req.socket.remoteAddress
alt Not localhost
MW-->>C: 403 Forbidden
else Is localhost
MW->>RL: next()
RL->>RL: Check rate bucket
alt Limit exceeded
RL-->>C: 429 Too Many Requests
else Under limit
RL->>AT: next()
AT->>AT: Re-check localhost (socket)
alt Not localhost
AT-->>C: 403 Forbidden
else Is localhost
AT->>AT: Check Authorization header
alt Header present AND invalid token
AT-->>C: 401 Unauthorized
else No header (bypasses token check!)
AT->>H: next()
else Valid token
AT->>H: next()
end
H-->>C: 200 shutting_down
end
end
end
Reviews (1): Last reviewed commit: "fix(security): admin token auth, localho..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const info = userInfo(); | ||
| if (info.uid >= 0) { | ||
| return String(basePort + (info.uid % 1000)); | ||
| } |
There was a problem hiding this comment.
uid % 1000 gives only 1000 distinct ports across an arbitrarily large user population. On multi-user Linux servers (UIDs often starting at 1000), users with UIDs 1001, 2001, and 3001 all map to the same offset and therefore the same port, defeating the cross-user isolation goal. Consider using a wider modulo (e.g. uid % 10000) so the port range spans 37777–47776 and collisions are far less likely:
| const info = userInfo(); | |
| if (info.uid >= 0) { | |
| return String(basePort + (info.uid % 1000)); | |
| } | |
| return String(basePort + (info.uid % 10000)); |
| const isLocalhost = | ||
| clientIp === '127.0.0.1' || | ||
| clientIp === '::1' || | ||
| clientIp === '::ffff:127.0.0.1' || | ||
| clientIp === 'localhost'; |
There was a problem hiding this comment.
'localhost' string check is dead code
req.socket.remoteAddress always returns an IP address string ('127.0.0.1', '::1', or '::ffff:127.0.0.1'). The Node.js net module never resolves a hostname back to a string name in remoteAddress, so clientIp === 'localhost' can never be true. The three IP checks above it are sufficient; this branch can be removed to avoid misleading reviewers.
| function sanitizeInjectedContent(content: string): string { | ||
| // Escape any tags that look like our system tags | ||
| return content | ||
| .replace(/<\/?claude-mem-context>/g, (match) => match.replace('<', '<').replace('>', '>')) | ||
| .replace(/<\/?system-reminder>/g, (match) => match.replace('<', '<').replace('>', '>')) | ||
| .replace(/<\/?private>/g, (match) => match.replace('<', '<').replace('>', '>')); | ||
| } |
There was a problem hiding this comment.
Only <claude-mem-context>, <system-reminder>, and <private> are escaped. If the injected content originates from an LLM or external source, other injection vectors (<system>, <instructions>, <human>, etc.) are passed through unmodified. For a local tool this is low-risk today, but as the tag set grows the allow-list approach should either be kept in sync or replaced with a generic "escape any <tag> that looks structural" heuristic.
| // 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(); |
There was a problem hiding this comment.
The Authorization header is validated only if present. A caller from localhost that sends no Authorization header skips all token checks and falls straight to next(). This means any local process can call POST /api/admin/shutdown or /api/admin/restart without knowing the admin token. The bearer-token protection added by this PR is opt-in rather than enforced, which contradicts the stated "Fix #1932: Add bearer token auth for admin endpoints" goal.
The fix is to remove the if (authHeader) guard and always require a valid token, returning 401 when the header is absent or contains an invalid value.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/services/sqlite/observations/store.ts (1)
68-74: Avoid mutating the caller’sobservationobject in place.This rewrites
observation.narrative/observation.factsdirectly. If the caller reuses the object later, it will unexpectedly see truncated values. Prefer derived locals for hash/insert.♻️ Suggested refactor
- if (observation.narrative) { - observation.narrative = truncateObservationPayload(observation.narrative); - } - if (observation.facts && observation.facts.length > 0) { - observation.facts = observation.facts.map(fact => truncateObservationPayload(fact)); - } + const narrative = observation.narrative + ? truncateObservationPayload(observation.narrative) + : observation.narrative; + const facts = observation.facts?.length + ? observation.facts.map(fact => truncateObservationPayload(fact)) + : observation.facts; - const contentHash = computeObservationContentHash(memorySessionId, observation.title, observation.narrative); + const contentHash = computeObservationContentHash(memorySessionId, observation.title, narrative); const result = stmt.run( @@ - JSON.stringify(observation.facts), - observation.narrative, + JSON.stringify(facts), + narrative,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/sqlite/observations/store.ts` around lines 68 - 74, The code currently mutates the caller's observation object in place by assigning truncated values to observation.narrative and observation.facts; instead, create a derived local (e.g., const processedObservation or localNarrative/localFacts) and copy/compute truncated values using truncateObservationPayload for observation.narrative and each fact in observation.facts, then use those derived values for hashing/insertion (the places that previously read observation.narrative/observation.facts) while leaving the original observation object untouched; reference the truncateObservationPayload helper and the observation.narrative / observation.facts accessors in store.ts to locate and replace the in-place assignments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/transcripts/processor.ts`:
- Around line 369-375: The current allowlist check uses path.resolve
(resolvedAgentsPath) which does not resolve symlinks and can be bypassed; change
the code to compute the canonical path (e.g., via fs.realpathSync or
fs.realpathSync.native) for agentsPath and for the project/home dataDir roots
before performing the prefix equality checks, use those canonical values in the
allowlist conditional, and pass the canonical agents path into writeAgentsMd
(and any downstream consumers) so writes cannot escape via symlinked components.
In `@src/utils/claude-md-utils.ts`:
- Around line 100-109: The function truncateObservationPayload currently
measures characters and appends a suffix after slicing, which can exceed the
stated byte cap and corrupt multibyte UTF-8; fix truncateObservationPayload to
enforce MAX_OBSERVATION_PAYLOAD_BYTES in bytes by using TextEncoder to get the
UTF-8 byte array, reserve bytes for the truncation suffix (the '\n[... truncated
at 10KB]' string) and then slice the encoded bytes to fit (maxBytes -
suffixBytes), then decode that byte slice back to a UTF-8 string with
TextDecoder and append the suffix only when truncation occurred; reference
truncateObservationPayload, MAX_OBSERVATION_PAYLOAD_BYTES, and the truncation
suffix when implementing.
---
Nitpick comments:
In `@src/services/sqlite/observations/store.ts`:
- Around line 68-74: The code currently mutates the caller's observation object
in place by assigning truncated values to observation.narrative and
observation.facts; instead, create a derived local (e.g., const
processedObservation or localNarrative/localFacts) and copy/compute truncated
values using truncateObservationPayload for observation.narrative and each fact
in observation.facts, then use those derived values for hashing/insertion (the
places that previously read observation.narrative/observation.facts) while
leaving the original observation object untouched; reference the
truncateObservationPayload helper and the observation.narrative /
observation.facts accessors in store.ts to locate and replace the in-place
assignments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c14ebec2-503d-4f87-ab45-d707750b6d29
📒 Files selected for processing (10)
plugin/scripts/context-generator.cjsplugin/scripts/mcp-server.cjsplugin/scripts/worker-service.cjssrc/services/server/Middleware.tssrc/services/server/Server.tssrc/services/sqlite/observations/store.tssrc/services/transcripts/processor.tssrc/services/worker/http/middleware.tssrc/shared/SettingsDefaultsManager.tssrc/utils/claude-md-utils.ts
| const resolvedAgentsPath = path.resolve(agentsPath); | ||
| const projectRoot = path.resolve(cwd); | ||
| const dataDir = path.resolve(os.homedir(), '.claude-mem'); | ||
| if (!resolvedAgentsPath.startsWith(projectRoot + path.sep) && | ||
| resolvedAgentsPath !== projectRoot && | ||
| !resolvedAgentsPath.startsWith(dataDir + path.sep) && | ||
| resolvedAgentsPath !== dataDir) { |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js, does path.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:
- 1: https://nodejs.org/docs/latest/api/path.html
- 2: https://nodejs.org/dist/latest/docs/api/path.html
- 3: https://stackoverflow.com/questions/26372371/resolve-filesystem-paths-possibly-with-symlinks-with-node-js
- 4: https://docs.deno.com/api/node/fs/~/realpath
- 5: https://nodejs.org/api/path.html
- 6: https://nodejs.org/api/fs.html
- 7: fs.realpath does not follow links and relative paths nodejs/node-v0.x-archive#8563
- 8: https://runebook.dev/en/articles/node/fs/fsrealpathsyncpath-options
- 9: https://runebook.dev/en/articles/node/fs/fspromisesrealpathpath-options
🏁 Script executed:
cat -n src/services/transcripts/processor.ts | sed -n '360,385p'Repository: thedotmack/claude-mem
Length of output: 1295
🏁 Script executed:
cat -n src/utils/agents-md-utils.ts | sed -n '1,50p'Repository: thedotmack/claude-mem
Length of output: 1663
🏁 Script executed:
cat -n src/services/transcripts/config.ts | sed -n '100,120p'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-controlled watch.context.path containing a symlink component (e.g., <projectRoot>/link/AGENTS.md where link → /etc/) will pass the prefix check but cause writeAgentsMd() to write outside allowed roots when the filesystem follows the symlink target. Validate the canonical path using fs.realpathSync() or fs.realpathSync.native() before the allowlist check, and pass the canonical path to writeAgentsMd().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/transcripts/processor.ts` around lines 369 - 375, The current
allowlist check uses path.resolve (resolvedAgentsPath) which does not resolve
symlinks and can be bypassed; change the code to compute the canonical path
(e.g., via fs.realpathSync or fs.realpathSync.native) for agentsPath and for the
project/home dataDir roots before performing the prefix equality checks, use
those canonical values in the allowlist conditional, and pass the canonical
agents path into writeAgentsMd (and any downstream consumers) so writes cannot
escape via symlinked components.
| export const MAX_OBSERVATION_PAYLOAD_BYTES = 10 * 1024; // 10KB | ||
|
|
||
| /** | ||
| * Truncate an observation payload string to the configured max size. | ||
| * Appends a truncation marker if content was clipped. | ||
| */ | ||
| export function truncateObservationPayload(payload: string): string { | ||
| if (payload.length <= MAX_OBSERVATION_PAYLOAD_BYTES) return payload; | ||
| return payload.slice(0, MAX_OBSERVATION_PAYLOAD_BYTES) + '\n[... truncated at 10KB]'; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
MAX = 10 * 1024
suffix = "\n[... truncated at 10KB]"
s = "😀" * 10240 # 10,240 chars; 40,960 UTF-8 bytes
print("chars:", len(s))
print("utf8_bytes:", len(s.encode("utf-8")))
stored = s[:MAX] + suffix
print("current_logic_stored_utf8_bytes:", len(stored.encode("utf-8")))
print("exceeds_10KB:", len(stored.encode("utf-8")) > MAX)
PYRepository: thedotmack/claude-mem
Length of output: 153
🏁 Script executed:
# Check the actual implementation in the file
cat -n src/utils/claude-md-utils.ts | sed -n '95,125p'Repository: thedotmack/claude-mem
Length of output: 1547
🏁 Script executed:
# Find usages of truncateObservationPayload and MAX_OBSERVATION_PAYLOAD_BYTES
rg -n "truncateObservationPayload|MAX_OBSERVATION_PAYLOAD_BYTES" --type ts --type jsRepository: thedotmack/claude-mem
Length of output: 872
🏁 Script executed:
# Check the store.ts context around the truncateObservationPayload usages
sed -n '60,85p' src/services/sqlite/observations/store.tsRepository: thedotmack/claude-mem
Length of output: 1312
🏁 Script executed:
# Search for Bug `#1935` references or related comments
rg -n "Bug `#1935`|1935|byte.*limit|payload.*byte" --type ts --type js -C 2Repository: thedotmack/claude-mem
Length of output: 2056
🏁 Script executed:
# Check if there are database schema or constraint definitions
find . -type f \( -name "*.sql" -o -name "*schema*" -o -name "*migration*" \) | head -20Repository: thedotmack/claude-mem
Length of output: 288
🏁 Script executed:
# Check the migrations file for observations table definition
rg -n "observations|CREATE TABLE" src/services/sqlite/migrations.ts -A 10 | head -100Repository: thedotmack/claude-mem
Length of output: 4000
🏁 Script executed:
# Look for any constraints or limits on narrative or facts columns
rg -n "narrative|facts" src/services/sqlite/migrations.ts -B 3 -A 3Repository: thedotmack/claude-mem
Length of output: 2210
🏁 Script executed:
# Find the observations table creation
rg -n "CREATE TABLE IF NOT EXISTS observations" src/services/sqlite/migrations.ts -A 20Repository: thedotmack/claude-mem
Length of output: 1105
🏁 Script executed:
# Search for the complete observations table including ALTER statements
rg -n "ALTER TABLE observations" src/services/sqlite/migrations.ts -B 2 -A 2Repository: thedotmack/claude-mem
Length of output: 581
🏁 Script executed:
# Check if there are tests for truncateObservationPayload
find . -type f \( -name "*.test.ts" -o -name "*.spec.ts" \) -exec grep -l "truncateObservationPayload\|MAX_OBSERVATION_PAYLOAD" {} \;Repository: thedotmack/claude-mem
Length of output: 47
🏁 Script executed:
# Search for narrative and facts column definitions
rg -n "narrative|facts" src/services/sqlite/migrations.ts | grep -v "fts\|INSERT\|VALUES\|SELECT" | head -20Repository: thedotmack/claude-mem
Length of output: 160
🏁 Script executed:
# Check if observations table has narrative and facts columns added later
rg -n "ALTER TABLE observations ADD COLUMN (narrative|facts)" src/services/sqlite/migrations.ts -B 1 -A 1Repository: thedotmack/claude-mem
Length of output: 47
🏁 Script executed:
# Check the observation entity or interface definition to understand the structure
rg -n "narrative|facts" src/services/sqlite/observations/ --type ts | grep -E "(interface|type|class)" -A 5 | head -30Repository: thedotmack/claude-mem
Length of output: 1021
🏁 Script executed:
# Look for all ALTER TABLE statements that add columns to observations
rg -n "ALTER TABLE observations" src/services/sqlite/migrations.tsRepository: thedotmack/claude-mem
Length of output: 326
🏁 Script executed:
# Check the full observation type definition
cat -n src/services/sqlite/observations/types.tsRepository: thedotmack/claude-mem
Length of output: 2266
MAX_OBSERVATION_PAYLOAD_BYTES is not actually enforced as bytes.
Line 107 uses payload.length (character count), and Line 108 appends a suffix after slicing, so stored content can exceed 10KB. This misses the stated 10KB cap for multibyte text and even for ASCII due to suffix growth.
🐛 Byte-accurate truncation (including suffix budget)
export const MAX_OBSERVATION_PAYLOAD_BYTES = 10 * 1024; // 10KB
+const TRUNCATION_SUFFIX = '\n[... truncated at 10KB]';
export function truncateObservationPayload(payload: string): string {
- if (payload.length <= MAX_OBSERVATION_PAYLOAD_BYTES) return payload;
- return payload.slice(0, MAX_OBSERVATION_PAYLOAD_BYTES) + '\n[... truncated at 10KB]';
+ if (Buffer.byteLength(payload, 'utf8') <= MAX_OBSERVATION_PAYLOAD_BYTES) return payload;
+
+ const suffixBytes = Buffer.byteLength(TRUNCATION_SUFFIX, 'utf8');
+ const budget = Math.max(0, MAX_OBSERVATION_PAYLOAD_BYTES - suffixBytes);
+
+ // Binary search the largest prefix that fits in byte budget
+ let lo = 0;
+ let hi = payload.length;
+ while (lo < hi) {
+ const mid = Math.ceil((lo + hi) / 2);
+ if (Buffer.byteLength(payload.slice(0, mid), 'utf8') <= budget) lo = mid;
+ else hi = mid - 1;
+ }
+
+ return payload.slice(0, lo) + TRUNCATION_SUFFIX;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/claude-md-utils.ts` around lines 100 - 109, The function
truncateObservationPayload currently measures characters and appends a suffix
after slicing, which can exceed the stated byte cap and corrupt multibyte UTF-8;
fix truncateObservationPayload to enforce MAX_OBSERVATION_PAYLOAD_BYTES in bytes
by using TextEncoder to get the UTF-8 byte array, reserve bytes for the
truncation suffix (the '\n[... truncated at 10KB]' string) and then slice the
encoded bytes to fit (maxBytes - suffixBytes), then decode that byte slice back
to a UTF-8 string with TextDecoder and append the suffix only when truncation
occurred; reference truncateObservationPayload, MAX_OBSERVATION_PAYLOAD_BYTES,
and the truncation suffix when implementing.
…e-based truncation, larger port range - Remove dead 'localhost' string check from requireLocalhost and requireAdminToken (req.socket.remoteAddress never returns the string 'localhost') - Make admin bearer token mandatory on every admin request instead of optional fallback to no-auth when no token header is provided - Use fs.realpathSync() instead of path.resolve() for symlink-safe path validation in transcript processor context path checking - Fix truncation to use Buffer.byteLength instead of string length so multi-byte characters (emoji, CJK) are measured correctly at the 10KB limit - Increase port modulo space from uid % 1000 to uid % 10000 to reduce collision probability on shared machines Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code Review — Security Hardening PR #2062Good set of targeted fixes for real attack surface. Overall direction is correct; a few issues warrant attention before merge. OverviewFive separate security issues addressed in one PR: admin token auth, localhost enforcement, path traversal, payload limits/sanitization, and per-user port derivation. The scope is appropriate and the fixes are generally well-implemented. Issues🐛 Bug:
|
|
Closing to start fresh from main — will redo fixes isolated in Docker container. |
1 participant
Summary
crypto.randomBytes(32)stored in~/.claude-mem/admin.tokenwith 0600 perms), usereq.socket.remoteAddressinstead ofreq.ipto prevent X-Forwarded-For spoofing, disabletrust proxyon Express apprequireLocalhostmiddleware to ALL/api/*routes at the app level viathis.app.use('/api', requireLocalhost, rateLimiter), not just admin endpointswatch.context.pathstays within project root or~/.claude-mem/directory after resolution, blocking path traversal attacks like../../.bashrc<claude-mem-context>,<system-reminder>,<private>)37777 + uid % 1000) to prevent cross-user data leakage on shared macOS machines; falls back to base port on Windows where UID is unavailableTest plan
/api/*routes reject non-localhost requests (checkreq.socket.remoteAddress)../../.bashrc) inwatch.context.pathare blocked with SECURITY warning<claude-mem-context>tags in injected content are escaped to<claude-mem-context>Closes #1932, closes #1933, closes #1934, closes #1935, closes #1936
🤖 Generated with Claude Code