security: comprehensive security audit fixes#5
Closed
alfredocristofano wants to merge 5 commits into
Closed
Conversation
- Add 10 MiB body size limit to prevent OOM - Validate sessionId length/format and cap sessionSeqs map size - Return generic 500/404 messages instead of internal details - Strip query string from request logs - Add security headers (X-Content-Type-Options, X-Frame-Options, Referrer-Policy) - Filter sensitive upstream headers (set-cookie, server, via, x-request-id) - Add runtime validation for Anthropic request body Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Expand REDACT_KEYS with password, secret, api_key, cookie, bearer, etc. - Handle circular references in JSON.stringify to prevent crashes - Enforce 0o700 on log directory and 0o600 on log file - Add 10 MiB limit to SSE buffer to prevent unbounded growth Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix PKCE randomness bias by using base64url encoding directly - Use ephemeral port 0 for OAuth callback to prevent port squatting - Validate Host header in OAuth callback server - HTML-escape error messages in OAuth callback response - Clear stored Codex tokens on 401/403 refresh failure - Enforce 0o700 on auth storage directories on Linux Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add safeJsonStringify helper to prevent crashes on circular refs - Validate image URLs before forwarding (allow http/https/data only) - Guard against missing image block source - Validate retry-after header format before reflection - Add sessionTimeline eviction to prevent unbounded memory growth - Remove raw console.error(err) in CLI that bypassed log redaction Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add private: true, engines, packageManager, and repository metadata - Pin @types/bun to exact version and regenerate bun.lock - Reduce CI workflow permissions to least-privilege per job - Pin Bun version in CI for reproducible builds - Add timeout-minutes to all CI jobs - Expand .gitignore to exclude logs, env files, and auth artifacts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
|
Thanks for the PR. I'm reluctant to take changes like this in directly, and rather do things with my own workflow which I trust. Running the findings through my own review process and applying what makes sense to me. I've already done a security hardening pass in main and addressed some things. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR addresses findings from a comprehensive security audit of the proxy. All changes are defensive hardening with no breaking changes to the CLI or API surface.
Server & DoS Hardening
req.json()on huge payloadssessionSeqsandsessionTimelinenow have size limits and periodic cleanup to prevent unbounded memory growth[a-zA-Z0-9_-]to prevent key injection and memory exhaustionInformation Disclosure
url.searchno longer logged to avoid accidental secret leakagepassword,secret,api_key,cookie,bearer,token, and more toREDACT_KEYSJSON.stringifyin logger now handles cyclic objects without crashing0o700, log file with0o600Authentication & OAuth
Hostheader validation and HTML-escaping of error messages0o700Input Validation
model,messages,max_tokens,temperature,top_p, andtoolshttp:,https:, anddata:URLs are forwarded upstreamimageToUrlno longer crashes on malformed image blocksJSON.stringifyin request builders wrapped to survive circular inputsCI/CD & Supply Chain
contents: read, write only onreleaseandupdate-tapjobspackageManagerfield locked to1.3.10@types/bun: moved fromlatestto exact version1.3.12bun.lock: fixed stale project name mismatchprivate: true,engines,packageManager, and repository metadata topackage.jsontimeout-minutesto all workflow jobs.gitignore: excludes logs, env files, auth artifactsTest Plan
bun run typecheckpasses/healthzresponds correctlybun install --frozen-lockfileworks with regenerated lockfileNotes
The macOS Keychain
securityCLI still passes tokens as command-line arguments (visible viaps). Fixing this requires migrating to a native Keychain API, which is left as a follow-up due to dependency complexity.