Skip to content

M4: ALPN lock, WebSocket detection, HAR export#75

Open
kalil0321 wants to merge 4 commits into
claude/proxy-monitor-m3-swiftui-uifrom
claude/proxy-monitor-m4-alpn-har
Open

M4: ALPN lock, WebSocket detection, HAR export#75
kalil0321 wants to merge 4 commits into
claude/proxy-monitor-m3-swiftui-uifrom
claude/proxy-monitor-m4-alpn-har

Conversation

@kalil0321
Copy link
Copy Markdown
Owner

@kalil0321 kalil0321 commented May 19, 2026

Stacked on top of #74.

This is a focused M4 — original plan was full HTTP/2 + WebSocket, but a full H2 MITM and a WS frame relay are each large enough to deserve their own milestone. For v0.1 the practical win is to make the proxy honest about what it supports.

What's in

  • ALPN locked to http/1.1 on both TLSContextFactory (server side) and UpstreamPump (client side). Clients capable of h2 negotiate down cleanly instead of failing the handshake or producing malformed frames against our HTTP/1 codec.
  • WebSocket detectionProxyHandler now inspects Upgrade: websocket before dispatching upstream and replies 502 Bad Gateway with a captured error message in the UI. Better than a silent stall.
  • HAR 1.2 exportHARExporter produces Chrome/Firefox-compatible HAR with query string parsing, content-type detection, base64 fallback for binary bodies, and _error field for failed flows. Plumbed into the toolbar with an NSSavePanel-driven Export HAR button.

Out of scope (deliberate)

  • Full HTTP/2 MITM (parse, route, re-encode h2 frames). Possible later if we want pinned-app or h2-only-API parity.
  • WebSocket transparent tunnel relay. Same — needs its own pipeline mode.

Generated by Claude Code


Summary by cubic

Locks ALPN to HTTP/1.1, blocks WebSocket upgrades with a clear 502, and adds HAR 1.2 export from the toolbar.

  • New Features

    • HAR 1.2 export for captured flows with improved query parsing (decodes + as space, strips fragments), omits postData on bodyless requests, base64 for binary, _error for failures, and a shared ISO8601 formatter for lower overhead.
    • “Export HAR” toolbar action with an NSSavePanel, timestamped filename, background serialization/write, keyboard shortcuts (export/clear/capture), and error alerts via NSAlert.
  • Bug Fixes

    • Force ALPN http/1.1 on both proxy server and upstream client so h2-capable clients downgrade cleanly.
    • Detect Upgrade: websocket and return 502; detection handles comma-separated values, case differences, and multiple Upgrade headers.
    • Avoid Keychain prompts by storing the CA private key on disk with 0600 perms (directory 0700); regenerate when the certificate exists without the key file.

Written for commit d18cd1b. Summary will update on new commits. Review in cubic

Greptile Summary

This PR delivers three focused improvements: ALPN is locked to http/1.1 on both the server-side TLSContextFactory and the upstream client so h2-capable clients negotiate down cleanly, WebSocket upgrade requests are intercepted early and returned as 502 so they surface in the UI rather than silently stalling, and a new HARExporter produces Chrome/Firefox-compatible HAR 1.2 output plumbed into the toolbar via an NSSavePanel.

  • ALPN lock — forces http/1.1 negotiation on both sides, preventing handshake failures against the HTTP/1 codec when h2-capable clients connect.
  • WebSocket detectionProxyHandler.isWebSocketUpgrade scans comma-separated and multi-line Upgrade headers case-insensitively, emits a captured error flow to the bus, and responds 502; seven new unit tests cover the detection matrix.
  • HAR 1.2 exportHARExporter handles +-as-space form encoding, fragment stripping, conditional postData, base64 for binary bodies, and _error for failed flows; the toolbar write is dispatched off the main actor via Task.detached.

Confidence Score: 5/5

The changes are self-contained and well-tested; the export path runs off the main actor and cannot block the UI.

All three feature areas are covered by new unit tests, the previously identified correctness issues have been addressed in this diff, and no new defects were found in the changed code paths.

No files require special attention.

Important Files Changed

Filename Overview
macos/Sources/ReverseAPIProxy/Export/HARExporter.swift New HAR 1.2 exporter with correct query-string parsing (+ as space, percent-decode), conditional postData, base64 fallback for binary bodies, and _error for failed flows. One minor allocation inefficiency: ISO8601DateFormatter is created per entry.
macos/Sources/ReverseAPIProxy/Proxy/ProxyHandler.swift Adds WebSocket upgrade detection with 502 response and flow emission; static method exposed for testability via internal wrapper. Logic is correct and well-covered by the new test suite.
macos/Sources/ReverseAPI/UI/CaptureToolbar.swift Adds Export HAR button with NSSavePanel, timestamped filename, Task.detached serialisation/write off the main actor, and NSAlert error handling. Keyboard shortcuts added for export, clear, and capture.
macos/Tests/ReverseAPIProxyTests/HARExporterTests.swift Comprehensive tests for queryString parsing, decodeFormComponent, entry shape (postData omission, base64 encoding, _error field), and full HAR structure validation.
macos/Tests/ReverseAPIProxyTests/WebSocketDetectionTests.swift Covers case-insensitive single-header, comma-separated, multi-header, and absent-header variants — good coverage of the detection logic.
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
macos/Sources/ReverseAPIProxy/Export/HARExporter.swift:16-19
A new `ISO8601DateFormatter` is created for every flow that gets serialised. `ISO8601DateFormatter` has non-trivial initialisation overhead; for a capture with hundreds of flows the repeated allocations add up noticeably during export. Hoisting it to a `static let` on the enum lets all calls share the same instance.

```suggestion
    private static let iso8601Formatter: ISO8601DateFormatter = {
        let f = ISO8601DateFormatter()
        f.formatOptions = [.withInternetDateTime, .withFractionalSeconds]
        return f
    }()

    static func entry(for flow: CapturedFlow) -> [String: Any] {
        let formatter = Self.iso8601Formatter
        let started = formatter.string(from: flow.startedAt)
```

Reviews (3): Last reviewed commit: "M4 review fixes + tests" | Re-trigger Greptile

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 5 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread macos/Sources/ReverseAPI/UI/CaptureToolbar.swift Outdated
Comment thread macos/Sources/ReverseAPIProxy/Proxy/ProxyHandler.swift Outdated
Comment thread macos/Sources/ReverseAPIProxy/Export/HARExporter.swift
Comment thread macos/Sources/ReverseAPIProxy/Export/HARExporter.swift Outdated
Comment thread macos/Sources/ReverseAPI/UI/CaptureToolbar.swift
@kind-agent
Copy link
Copy Markdown

kind-agent Bot commented May 19, 2026

⚠️ Error — The test run failed unexpectedly.

Grok 4 Fast is deprecated. xAI recommends switching to Grok 4.3 (https://openrouter.ai/x-ai/grok-4.3)

This is likely a transient issue. You can re-trigger a run from the dashboard.

kalil0321 pushed a commit that referenced this pull request May 19, 2026
Fixes for PR #75 review comments (greptile, cubic):

- HARExporter:
  * decode application/x-www-form-urlencoded "+" as space in query
    string keys/values (Chrome/Firefox HAR exports do this; we did
    not, producing literal "hello+world" instead of "hello world")
  * omit postData from GET/HEAD/OPTIONS entries entirely so HAR
    validators that require postData only for bodied requests do
    not flag generated files
  * strip query string fragments (#section) before parsing
- ProxyHandler.isWebSocketUpgrade: split Upgrade header on comma,
  trim, lowercase, then check for "websocket" anywhere in the
  resulting token set. A request with "h2c, websocket" or multiple
  Upgrade header lines is now correctly detected.
- CaptureToolbar.exportHAR: serialize and write the HAR off the
  main thread so large captures do not freeze the UI; surface errors
  via NSAlert back on the main actor.

Tests:
- HARExporterTests: 14 cases covering queryString parsing
  (+/percent/fragment/empty/key-only), decodeFormComponent, postData
  presence for POST vs absence for GET, base64 binary bodies,
  _error field, full export structure
- WebSocketDetectionTests: 7 cases including comma-separated values,
  case-insensitivity, multiple Upgrade headers, missing header
Copy link
Copy Markdown
Owner Author

@greptile review


Generated by Claude Code

@kalil0321 kalil0321 force-pushed the claude/proxy-monitor-m3-swiftui-ui branch from 33bfb0d to 3c7f333 Compare May 19, 2026 12:57
Comment thread macos/Sources/ReverseAPIProxy/Proxy/ProxyHandler.swift
kalil0321 pushed a commit that referenced this pull request May 19, 2026
Fixes for PR #75 review comments (greptile, cubic):

- HARExporter:
  * decode application/x-www-form-urlencoded "+" as space in query
    string keys/values (Chrome/Firefox HAR exports do this; we did
    not, producing literal "hello+world" instead of "hello world")
  * omit postData from GET/HEAD/OPTIONS entries entirely so HAR
    validators that require postData only for bodied requests do
    not flag generated files
  * strip query string fragments (#section) before parsing
- ProxyHandler.isWebSocketUpgrade: split Upgrade header on comma,
  trim, lowercase, then check for "websocket" anywhere in the
  resulting token set. A request with "h2c, websocket" or multiple
  Upgrade header lines is now correctly detected.
- CaptureToolbar.exportHAR: serialize and write the HAR off the
  main thread so large captures do not freeze the UI; surface errors
  via NSAlert back on the main actor.

Tests:
- HARExporterTests: 14 cases covering queryString parsing
  (+/percent/fragment/empty/key-only), decodeFormComponent, postData
  presence for POST vs absence for GET, base64 binary bodies,
  _error field, full export structure
- WebSocketDetectionTests: 7 cases including comma-separated values,
  case-insensitivity, multiple Upgrade headers, missing header
@kalil0321 kalil0321 force-pushed the claude/proxy-monitor-m4-alpn-har branch from 08f57a2 to b80021e Compare May 19, 2026 12:58
Copy link
Copy Markdown
Owner Author

@greptile review


Generated by Claude Code

@kalil0321 kalil0321 force-pushed the claude/proxy-monitor-m3-swiftui-ui branch from 3c7f333 to d9d7ff5 Compare May 19, 2026 13:20
kalil0321 pushed a commit that referenced this pull request May 19, 2026
Fixes for PR #75 review comments (greptile, cubic):

- HARExporter:
  * decode application/x-www-form-urlencoded "+" as space in query
    string keys/values (Chrome/Firefox HAR exports do this; we did
    not, producing literal "hello+world" instead of "hello world")
  * omit postData from GET/HEAD/OPTIONS entries entirely so HAR
    validators that require postData only for bodied requests do
    not flag generated files
  * strip query string fragments (#section) before parsing
- ProxyHandler.isWebSocketUpgrade: split Upgrade header on comma,
  trim, lowercase, then check for "websocket" anywhere in the
  resulting token set. A request with "h2c, websocket" or multiple
  Upgrade header lines is now correctly detected.
- CaptureToolbar.exportHAR: serialize and write the HAR off the
  main thread so large captures do not freeze the UI; surface errors
  via NSAlert back on the main actor.

Tests:
- HARExporterTests: 14 cases covering queryString parsing
  (+/percent/fragment/empty/key-only), decodeFormComponent, postData
  presence for POST vs absence for GET, base64 binary bodies,
  _error field, full export structure
- WebSocketDetectionTests: 7 cases including comma-separated values,
  case-insensitivity, multiple Upgrade headers, missing header
@kalil0321 kalil0321 force-pushed the claude/proxy-monitor-m4-alpn-har branch from b80021e to 52acac0 Compare May 19, 2026 13:21
@kalil0321 kalil0321 force-pushed the claude/proxy-monitor-m3-swiftui-ui branch from d9d7ff5 to c27a3ee Compare May 19, 2026 16:53
claude and others added 3 commits May 20, 2026 01:32
- TLSContextFactory + UpstreamPump now advertise http/1.1 only via
  ALPN on both sides, so h2-capable clients downgrade cleanly
- ProxyHandler detects Upgrade: websocket and responds 502 with a
  surfaced capture error rather than producing a broken response
- HARExporter renders the live flow list as HAR 1.2 (Chrome/Firefox
  compatible), with base64 fallback for binary bodies and query string
  parsing
- CaptureToolbar gains an Export HAR action using NSSavePanel
Fixes for PR #75 review comments (greptile, cubic):

- HARExporter:
  * decode application/x-www-form-urlencoded "+" as space in query
    string keys/values (Chrome/Firefox HAR exports do this; we did
    not, producing literal "hello+world" instead of "hello world")
  * omit postData from GET/HEAD/OPTIONS entries entirely so HAR
    validators that require postData only for bodied requests do
    not flag generated files
  * strip query string fragments (#section) before parsing
- ProxyHandler.isWebSocketUpgrade: split Upgrade header on comma,
  trim, lowercase, then check for "websocket" anywhere in the
  resulting token set. A request with "h2c, websocket" or multiple
  Upgrade header lines is now correctly detected.
- CaptureToolbar.exportHAR: serialize and write the HAR off the
  main thread so large captures do not freeze the UI; surface errors
  via NSAlert back on the main actor.

Tests:
- HARExporterTests: 14 cases covering queryString parsing
  (+/percent/fragment/empty/key-only), decodeFormComponent, postData
  presence for POST vs absence for GET, base64 binary bodies,
  _error field, full export structure
- WebSocketDetectionTests: 7 cases including comma-separated values,
  case-insensitivity, multiple Upgrade headers, missing header
@kalil0321 kalil0321 force-pushed the claude/proxy-monitor-m4-alpn-har branch from 52acac0 to 0bfcc68 Compare May 19, 2026 23:36
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 2 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="macos/Sources/ReverseAPIProxy/CA/CAStore.swift">

<violation number="1" location="macos/Sources/ReverseAPIProxy/CA/CAStore.swift:81">
P1: `exists()` now ignores previously keychain-stored CA keys, so upgrades can silently rotate the root CA instead of loading the existing one.</violation>
</file>

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

public func exists() -> Bool {
guard FileManager.default.fileExists(atPath: certificateURL.path) else { return false }
return privateKeyExists()
return FileManager.default.fileExists(atPath: privateKeyURL.path)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: exists() now ignores previously keychain-stored CA keys, so upgrades can silently rotate the root CA instead of loading the existing one.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At macos/Sources/ReverseAPIProxy/CA/CAStore.swift, line 81:

<comment>`exists()` now ignores previously keychain-stored CA keys, so upgrades can silently rotate the root CA instead of loading the existing one.</comment>

<file context>
@@ -69,86 +67,33 @@ public final class CAStore: @unchecked Sendable {
     public func exists() -> Bool {
         guard FileManager.default.fileExists(atPath: certificateURL.path) else { return false }
-        return privateKeyExists()
+        return FileManager.default.fileExists(atPath: privateKeyURL.path)
     }
 
</file context>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants