Security: SSRF, path traversal, shell injection, and rate limiting protections#2580
Security: SSRF, path traversal, shell injection, and rate limiting protections#2580joefulfill wants to merge 1 commit intoopenclaw:mainfrom
Conversation
…g protections - SSRF protection in media fetch: block private IPs, localhost, link-local addresses using ipaddr.js; manually follow redirects to validate each hop - Path traversal protection in web media: allowlist ~/.clawdbot and tmpdir, follow symlinks to prevent escape attacks - Shell injection fix in CLI credentials: use spawnSync with argument arrays instead of execSync with string interpolation - Rate limiting for gateway auth: 5 failures = 1 minute lockout, prevents brute-force attacks - Tailscale auth failure tracking: record failures for whois mismatches - Logger permissions: set 0o700 on log directory, warn if chmod fails Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
|
||
| export function checkRateLimit(req?: IncomingMessage): { allowed: boolean; retryAfterMs?: number } { | ||
| ensureCleanupInterval(); | ||
| const key = getRateLimitKey(req); | ||
| const now = Date.now(); | ||
| const entry = rateLimitMap.get(key); | ||
|
|
||
| if (entry && now < entry.lockedUntil) { | ||
| return { allowed: false, retryAfterMs: entry.lockedUntil - now }; | ||
| } | ||
|
|
||
| return { allowed: true }; | ||
| } | ||
|
|
||
| export function recordAuthFailure(req?: IncomingMessage): void { | ||
| ensureCleanupInterval(); | ||
| const key = getRateLimitKey(req); | ||
| const now = Date.now(); | ||
| const entry = rateLimitMap.get(key) ?? { failures: 0, lastAttempt: 0, lockedUntil: 0 }; | ||
|
|
||
| entry.failures += 1; | ||
| entry.lastAttempt = now; | ||
|
|
||
| if (entry.failures >= MAX_FAILURES) { | ||
| entry.lockedUntil = now + LOCKOUT_MS; | ||
| entry.failures = 0; // Reset after lockout | ||
| } | ||
|
|
||
| rateLimitMap.set(key, entry); | ||
| } | ||
|
|
||
| // Test helper | ||
| export function resetRateLimiter(): void { | ||
| rateLimitMap.clear(); | ||
| } | ||
|
|
There was a problem hiding this comment.
[P1] Rate limiter uses a process-wide setInterval and map keyed by IP; in test environments or multi-instance usage this can leak intervals/state across module reloads.
ensureCleanupInterval() starts a global timer that is never cleared, and resetRateLimiter() only clears the map. In vitest/watch (or when this module is reloaded), this can accumulate timers and cause flaky tests or keep resources alive unexpectedly. Consider clearing the interval in resetRateLimiter() (and setting cleanupInterval = null), or lazily cleaning without an interval (e.g., cleanup on access).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/auth.ts
Line: 40:75
Comment:
[P1] Rate limiter uses a process-wide `setInterval` and map keyed by IP; in test environments or multi-instance usage this can leak intervals/state across module reloads.
`ensureCleanupInterval()` starts a global timer that is never cleared, and `resetRateLimiter()` only clears the map. In vitest/watch (or when this module is reloaded), this can accumulate timers and cause flaky tests or keep resources alive unexpectedly. Consider clearing the interval in `resetRateLimiter()` (and setting `cleanupInterval = null`), or lazily cleaning without an interval (e.g., cleanup on access).
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (4)
In the new secure path, If the intended behavior is “upsert”, consider treating missing Prompt To Fix With AIThis is a comment left during a code review.
Path: src/agents/cli-credentials.ts
Line: 300:330
Comment:
[P0] `writeClaudeCliKeychainCredentials` returns `false` when existing keychain entry is missing, which can break “first write” flows.
In the new secure path, `existingResult` is treated as required (`if (!existingResult) return false;`). Previously, absence of a keychain item could be handled by creating/upserting it. If this function is called when the keychain service hasn’t been created yet (fresh install), this will now always fail.
If the intended behavior is “upsert”, consider treating missing `find-generic-password` as an empty object and proceeding to `add-generic-password`, or specifically handle the `security` exit status for “item not found”.
How can I resolve this? If you propose a fix, please make it concise.
This is OK for tests, but if any non-test code passes Prompt To Fix With AIThis is a comment left during a code review.
Path: src/agents/cli-credentials.ts
Line: 160:210
Comment:
[P1] Shell-injection mitigation is bypassed when `execSyncImpl` is provided (tests/mocking), since the fallback still uses string-interpolated `security ... -a "${account}"`.
This is OK for tests, but if any non-test code passes `options.execSync` (or if it’s exposed via dependency injection), it reintroduces the original vulnerability. If you want to keep the hook for tests, consider accepting a `spawnSync`-like runner instead, or having `execSync` code path also avoid interpolation by escaping/quoting safely (or only enabling it under test).
How can I resolve this? If you propose a fix, please make it concise.
This changes behavior from “no auth required” to “token mode but missing token,” which can break existing configs/tests and may lock users out unless they explicitly set a token or rely on Tailscale. If this was intentional, it may need a migration path / clearer error messaging; otherwise consider preserving Prompt To Fix With AIThis is a comment left during a code review.
Path: src/gateway/auth.ts
Line: 220:245
Comment:
[P1] `resolveGatewayAuth` no longer supports `mode: "none"`, and defaults to `"token"` even when no token is configured.
This changes behavior from “no auth required” to “token mode but missing token,” which can break existing configs/tests and may lock users out unless they explicitly set a token or rely on Tailscale. If this was intentional, it may need a migration path / clearer error messaging; otherwise consider preserving `none` when neither token nor password is set (or when `allowTailscale` is enabled).
How can I resolve this? If you propose a fix, please make it concise.
After Prompt To Fix With AIThis is a comment left during a code review.
Path: src/web/media.ts
Line: 150:175
Comment:
[P2] `file://` URL paths are checked against `isPathAllowed(mediaUrl)` (the URL string) rather than the decoded filesystem path.
After `fileURLToPath(mediaUrl)` succeeds, the allowlist check should be performed on the resulting `filePath`, not on the original `file://...` string. As written, `path.resolve("file:///...")` won’t behave as intended on most platforms, so allowed files may be incorrectly blocked (or the check may be effectively meaningless).
How can I resolve this? If you propose a fix, please make it concise. |
bfc1ccb to
f92900f
Compare
|
Thanks for your submission however we are closing your PR as stale, if you need to re-open please review contributing guide and if you feel like its required re-open under a new PR. Ensure you have addressed all checks, conflicts and issues. Thanks. |
Summary
This PR adds several security hardening measures:
ipaddr.js; manually follows redirects to validate each hop against SSRF rules~/.clawdbotandtmpdironly; follows symlinks viarealpathSyncto prevent escape attacksspawnSyncwith argument arrays instead ofexecSyncwith string interpolation for keychain operations0o700on log directory, warns if chmod failsTest plan
🤖 Generated with Claude Code
Greptile Overview
Greptile Summary
This PR hardens several security-sensitive paths: adds SSRF checks and redirect validation to
fetchRemoteMedia, constrains web media local file access to~/.clawdbotand the OS temp dir (with symlink-aware realpath checks), switches keychain operations tospawnSyncto avoid shell interpolation, and introduces a simple per-IP auth failure rate limiter for gateway auth with accompanying tests.Key areas to re-check are the new gateway auth mode defaults (removal of
none), the correctness of the newfile://allowlist check (currently validates the URL string vs the decoded path), and first-time keychain writes (now fail if the service entry doesn’t already exist).Confidence Score: 3/5
file://allowlist check appears to validate the wrong value, gateway auth defaults removednone(behavior change), and keychain writing may fail on first run if no item exists. These could impact real users and should be fixed before merge.(2/5) Greptile learns from your feedback when you react with thumbs up/down!