M4: ALPN lock, WebSocket detection, HAR export#75
Open
kalil0321 wants to merge 4 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
2 issues found across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
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
Owner
Author
|
@greptile review Generated by Claude Code |
33bfb0d to
3c7f333
Compare
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
08f57a2 to
b80021e
Compare
Owner
Author
|
@greptile review Generated by Claude Code |
3c7f333 to
d9d7ff5
Compare
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
b80021e to
52acac0
Compare
d9d7ff5 to
c27a3ee
Compare
- 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
52acac0 to
0bfcc68
Compare
Contributor
There was a problem hiding this comment.
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) |
Contributor
There was a problem hiding this comment.
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>
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.
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
http/1.1on bothTLSContextFactory(server side) andUpstreamPump(client side). Clients capable of h2 negotiate down cleanly instead of failing the handshake or producing malformed frames against our HTTP/1 codec.ProxyHandlernow inspectsUpgrade: websocketbefore dispatching upstream and replies502 Bad Gatewaywith a captured error message in the UI. Better than a silent stall.HARExporterproduces Chrome/Firefox-compatible HAR with query string parsing, content-type detection, base64 fallback for binary bodies, and_errorfield for failed flows. Plumbed into the toolbar with anNSSavePanel-driven Export HAR button.Out of scope (deliberate)
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
postDataon bodyless requests, base64 for binary,_errorfor failures, and a shared ISO8601 formatter for lower overhead.NSSavePanel, timestamped filename, background serialization/write, keyboard shortcuts (export/clear/capture), and error alerts viaNSAlert.Bug Fixes
http/1.1on both proxy server and upstream client so h2-capable clients downgrade cleanly.Upgrade: websocketand return 502; detection handles comma-separated values, case differences, and multipleUpgradeheaders.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.1on both the server-sideTLSContextFactoryand 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 newHARExporterproduces Chrome/Firefox-compatible HAR 1.2 output plumbed into the toolbar via anNSSavePanel.http/1.1negotiation on both sides, preventing handshake failures against the HTTP/1 codec when h2-capable clients connect.ProxyHandler.isWebSocketUpgradescans comma-separated and multi-lineUpgradeheaders case-insensitively, emits a captured error flow to the bus, and responds 502; seven new unit tests cover the detection matrix.HARExporterhandles+-as-space form encoding, fragment stripping, conditionalpostData, base64 for binary bodies, and_errorfor failed flows; the toolbar write is dispatched off the main actor viaTask.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
Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "M4 review fixes + tests" | Re-trigger Greptile