fix: comprehensive audit — security, bugs, cleanup#9
Merged
Conversation
Security: - Redact API keys in request bodies (api_key, token, secret, etc.) - Fix XSS in generateCurl: use shellEscape for header keys and body Bug fixes: - Fix transparent proxy data loss race: buffer data during TLS bridge connect - Add capture start/stop mutex to prevent concurrent state mutations - Fix UI contrast: bump --text-muted to #8a8a8a, --text-dim to #6b6b6b, --border to #2e2e2e Code cleanup: - Remove dead code: getLogsSinceId, verifyCapture, unused `running` variable - Extract getCertExpiresAt helper to eliminate duplicated X509 parsing - Replace nested ternaries with switch/if-else for readability - Remove unnecessary async/await in canConnectToForwarder - Simplify getOptionalStringParam Documentation: - Fix AGENTS.md architecture diagram (ElectroBun, not SwiftUI; Unix socket, not XPC) - Fix AGENTS.md traffic flow to describe current DNS-based interception - Add missing providers (deepseek, xai) to AGENTS.md - Update component descriptions to match actual implementation
|
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 14 minutes and 54 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughDocumentation updated to reflect Bun/ElectroBun menu bar app with Unix socket IPC and JSON protocol. DNS interception refactored to use Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun/index.ts (1)
382-395:⚠️ Potential issue | 🟠 Major
curl -Xis still injectable via the request method.
url, headers, and body are escaped now, butmethodis still interpolated raw into the shell command. A crafted method token likePOST$(...)would survive into the copied command. ``Proposed fix
- let cmd = `curl -X ${method} ${shellEscape(url)}`; + let cmd = `curl -X ${shellEscape(method)} ${shellEscape(url)}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun/index.ts` around lines 382 - 395, The generateCurl function interpolates trace.request_method into the shell command raw, allowing command injection via crafted methods; fix it by normalizing and validating method before use (e.g., read method from trace.request_method, uppercase it, then check against an explicit whitelist like GET, POST, PUT, PATCH, DELETE, HEAD, OPTIONS and fallback to GET or omit -X for invalid values) so only safe HTTP verbs are allowed; reference the method variable in generateCurl and replace the direct interpolation with the validated/whitelisted method (or remove -X when using the default GET) rather than inserting the raw value.
🧹 Nitpick comments (1)
src/views/mainview/index.css (1)
6-10: LGTM — Contrast improvements look solid.The lightened values for
--border,--text-muted, and--text-dimshould improve readability against the dark background. Hierarchy is maintained.If you want to verify accessibility compliance, consider checking WCAG contrast ratios:
🔍 Optional verification script
#!/bin/bash # Verify WCAG contrast ratios for the updated colors python3 << 'EOF' def hex_to_rgb(hex_color): hex_color = hex_color.lstrip('#') return tuple(int(hex_color[i:i+2], 16) for i in (0, 2, 4)) def relative_luminance(rgb): r, g, b = [x / 255.0 for x in rgb] r = r / 12.92 if r <= 0.03928 else ((r + 0.055) / 1.055) ** 2.4 g = g / 12.92 if g <= 0.03928 else ((g + 0.055) / 1.055) ** 2.4 b = b / 12.92 if b <= 0.03928 else ((b + 0.055) / 1.055) ** 2.4 return 0.2126 * r + 0.7152 * g + 0.0722 * b def contrast_ratio(hex1, hex2): lum1 = relative_luminance(hex_to_rgb(hex1)) lum2 = relative_luminance(hex_to_rgb(hex2)) lighter = max(lum1, lum2) darker = min(lum1, lum2) return (lighter + 0.05) / (darker + 0.05) bg = "#0a0a0a" text_muted = "#8a8a8a" text_dim = "#6b6b6b" border = "#2e2e2e" print(f"WCAG Contrast Ratios (AA requires 4.5:1 for normal text, 3:1 for large)") print(f"--text-muted on --bg: {contrast_ratio(text_muted, bg):.2f}:1") print(f"--text-dim on --bg: {contrast_ratio(text_dim, bg):.2f}:1") print(f"--border on --bg: {contrast_ratio(border, bg):.2f}:1") EOF🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/mainview/index.css` around lines 6 - 10, The CSS variable updates (--border, --border-light, --text, --text-muted, --text-dim) look good and require no code change; to verify accessibility, run the provided WCAG contrast check (or add it to CI) using the updated variables (--text-muted, --text-dim, --border) against your background color (e.g., --bg) and adjust any of those variables if any contrast ratios fall below WCAG thresholds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 12-29: The Tech Stack still claims IPC is via "Swift FFI" which
conflicts with the new Unix-socket privileged helper described earlier; update
the Tech Stack IPC bullet to reflect the actual mechanism: replace the "Swift
FFI" IPC mention with "Privileged Helper via Unix domain socket (JSON line
protocol)" or clarify that Swift FFI is only used for local bindings while IPC
to the privileged helper uses a Unix socket, so the documentation is consistent
with the Privileged Helper/Unix-socket IPC described in the components.
In `@src/bun/ca/ca-manager.ts`:
- Around line 35-36: ensureCA() is currently accepting malformed/failed-to-parse
PEMs because parse failures return null and downstream checks
(isCertExpired/isCertExpiringSoon) treat that as false; update the logic so that
a parse failure is treated as invalid and triggers regeneration. Specifically,
when you call the cert parsing function (the code path that returns null on
failure), if the result is null treat it the same as "expired" (i.e., force
regeneration) and do not assign unusable material to the loaded variable;
alternatively, make isCertExpired and isCertExpiringSoon return true for null
inputs so ensureCA() will regenerate. Ensure loaded is only set after a
successful parse and validation.
In `@src/bun/index.ts`:
- Around line 194-195: The capture mutex (captureTransitionLock) is only used
for tray clicks; fix all entry points that call doStartCapture() or
doStopCapture() (including the viewer RPC handlers and the debug endpoints) to
serialize transitions by chaining calls onto captureTransitionLock instead of
calling doStartCapture()/doStopCapture() directly; replace direct calls with
captureTransitionLock = captureTransitionLock.then(() => doStartCapture() /* or
doStopCapture() */).catch(()=>{}); return/await that chained promise so callers
observe completion and errors are propagated. Ensure every location that
currently invokes doStartCapture()/doStopCapture() (e.g., viewer RPC handlers
and debug endpoints) is updated to use this pattern.
In `@src/bun/storage/trace-writer.ts`:
- Around line 48-61: The redactRequestBody function only removes sensitive keys
at the top level; update it to recursively traverse the parsed JSON (objects and
arrays) and replace any property whose key matches an entry in
BODY_SENSITIVE_KEYS with REDACTED. Implement a helper (e.g., walkAndRedact or
recurse) that walks objects and arrays, checks keys against BODY_SENSITIVE_KEYS,
and mutates values to REDACTED when matched, then use that helper on the parsed
result before JSON.stringify; keep the try/catch and non-JSON fallback behavior
intact.
---
Outside diff comments:
In `@src/bun/index.ts`:
- Around line 382-395: The generateCurl function interpolates
trace.request_method into the shell command raw, allowing command injection via
crafted methods; fix it by normalizing and validating method before use (e.g.,
read method from trace.request_method, uppercase it, then check against an
explicit whitelist like GET, POST, PUT, PATCH, DELETE, HEAD, OPTIONS and
fallback to GET or omit -X for invalid values) so only safe HTTP verbs are
allowed; reference the method variable in generateCurl and replace the direct
interpolation with the validated/whitelisted method (or remove -X when using the
default GET) rather than inserting the raw value.
---
Nitpick comments:
In `@src/views/mainview/index.css`:
- Around line 6-10: The CSS variable updates (--border, --border-light, --text,
--text-muted, --text-dim) look good and require no code change; to verify
accessibility, run the provided WCAG contrast check (or add it to CI) using the
updated variables (--text-muted, --text-dim, --border) against your background
color (e.g., --bg) and adjust any of those variables if any contrast ratios fall
below WCAG thresholds.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a09ce436-7949-49ec-a6e9-a04f80c12566
📒 Files selected for processing (11)
AGENTS.mdsrc/bun/ca/ca-manager.tssrc/bun/index.tssrc/bun/proxy/proxy-server.tssrc/bun/proxy/transparent-proxy.tssrc/bun/proxy/upstream-forwarder.tssrc/bun/storage/trace-writer.tssrc/bun/storage/write-batcher.tssrc/bun/system/log-manager.tssrc/bun/system/redirect-manager.tssrc/views/mainview/index.css
💤 Files with no reviewable changes (3)
- src/bun/storage/write-batcher.ts
- src/bun/system/log-manager.ts
- src/bun/system/redirect-manager.ts
Comment on lines
+48
to
+61
| const BODY_SENSITIVE_KEYS = ["api_key", "apiKey", "secret", "token", "password", "credentials", "access_token"]; | ||
|
|
||
| function redactRequestBody(body: string | null): string | null { | ||
| if (!body) return null; | ||
| try { | ||
| const parsed = JSON.parse(body); | ||
| for (const key of BODY_SENSITIVE_KEYS) { | ||
| if (parsed[key]) parsed[key] = REDACTED; | ||
| } | ||
| return JSON.stringify(parsed); | ||
| } catch { | ||
| return body; // non-JSON body, return as-is | ||
| } | ||
| } |
There was a problem hiding this comment.
Redact nested JSON secrets, not just top-level fields.
This helper only checks direct properties on the parsed root object. Bodies like {"auth":{"api_key":"..."}} or arrays of credential objects will still be stored with secrets intact, so the storage redaction is incomplete. ``
Proposed fix
-const BODY_SENSITIVE_KEYS = ["api_key", "apiKey", "secret", "token", "password", "credentials", "access_token"];
+const BODY_SENSITIVE_KEYS = new Set([
+ "api_key",
+ "apikey",
+ "secret",
+ "token",
+ "password",
+ "credentials",
+ "access_token",
+]);
+function redactJsonSecrets(value: unknown): unknown {
+ if (Array.isArray(value)) {
+ return value.map(redactJsonSecrets);
+ }
+
+ if (value && typeof value === "object") {
+ return Object.fromEntries(
+ Object.entries(value as Record<string, unknown>).map(([key, child]) => [
+ key,
+ BODY_SENSITIVE_KEYS.has(key.toLowerCase()) ? REDACTED : redactJsonSecrets(child),
+ ]),
+ );
+ }
+
+ return value;
+}
+
function redactRequestBody(body: string | null): string | null {
if (!body) return null;
try {
const parsed = JSON.parse(body);
- for (const key of BODY_SENSITIVE_KEYS) {
- if (parsed[key]) parsed[key] = REDACTED;
- }
- return JSON.stringify(parsed);
+ return JSON.stringify(redactJsonSecrets(parsed));
} catch {
return body; // non-JSON body, return as-is
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bun/storage/trace-writer.ts` around lines 48 - 61, The redactRequestBody
function only removes sensitive keys at the top level; update it to recursively
traverse the parsed JSON (objects and arrays) and replace any property whose key
matches an entry in BODY_SENSITIVE_KEYS with REDACTED. Implement a helper (e.g.,
walkAndRedact or recurse) that walks objects and arrays, checks keys against
BODY_SENSITIVE_KEYS, and mutates values to REDACTED when matched, then use that
helper on the parsed result before JSON.stringify; keep the try/catch and
non-JSON fallback behavior intact.
…ests CodeRabbit findings (all 5): - Fix curl method injection: shellEscape the HTTP method in generateCurl - Fix malformed CA cert handling: treat unparseable certs as expired - Apply captureTransitionLock to ALL entry points (RPC + debug server) - Recursive JSON secret redaction for nested request body fields - Update AGENTS.md Tech Stack IPC bullet to reflect Unix socket + FFI Strategic items: - Helper orphan detection: auto-cleanup /etc/hosts + pf rules when app crashes (heartbeat timer probes port 8443 every 60s, cleans after 2 misses) - Forwarder port 443 restriction: reject non-443 upstream connections - Extract duplicated findProjectRoot/getAppContentsDir to shared binary-resolver.ts module (was copy-pasted in 3 files) Tests (5 → 108): - New: http-parser (20), sni-parser (7), cost-estimator (13), trace-utils (38) - Expanded: sse-reassembler (1 → 21)
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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
1 participant
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
Comprehensive codebase audit from 13 specialized subagents (security, architecture, code quality, tests, UI/UX, simplification, debugging, documentation, build analysis, verification, and more). This PR implements safe, non-breaking fixes plus the strategic items approved by the maintainer.
Security fixes
api_key,token,secret,password,credentials,access_token)Bug fixes
enqueueCaptureTransition()/etc/hosts+ pf rules when app crashes (heartbeat probes port 8443 every 60s, cleans after 2 consecutive misses / 120s)--text-mutedto#8a8a8a,--text-dimto#6b6b6b,--borderto#2e2e2eCode cleanup
findProjectRoot/getAppContentsDir/shouldSearchProjectRootfrom 3 files (native-bridge, helper-client, upstream-forwarder)getLogsSinceId,verifyCapture, unusedrunningvariablegetCertExpiresAthelper to eliminate duplicated X509 parsinggetOptionalStringParamTests (5 → 108)
parseRequestHead,parseResponseHead,findHeaderEnd,dechunkextractSNIwith valid/invalid/truncated ClientHelloDocumentation
Deferred to future PRs
getpeereid+ console UID check is already solid securityTest plan
bun test— 108 pass, 0 failclang -fsyntax-only)