M1: SwiftNIO MITM proxy core#72
Open
kalil0321 wants to merge 5 commits into
Open
Conversation
- SwiftPM package targeting macOS 14+ - ReverseAPIProxy library: proxy engine, CA + leaf cert factory, TLS bumping pipeline, upstream pump, flow capture bus - rae-proxy executable for headless debug - Initial XCTest suite for CA
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54aa85d7ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| try await channel.writeAndFlush(HTTPClientRequestPart.end(nil)).get() | ||
|
|
||
| let response = try await collector.awaitResponse() |
There was a problem hiding this comment.
For fast upstreams and small responses, the HTTP response can be fully delivered between the request flush on line 70 and this awaitResponse() call. In that case ResponseCollector.finish sees no stored continuation and drops the result, so this later call installs a continuation that will never be resumed and the proxied request hangs until the client disconnects. Start waiting before flushing the request, or have the collector retain a completed result for late waiters.
Useful? React with 👍 / 👎.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
10 issues found across 15 files
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/Proxy/ProxyHandler.swift">
<violation number="1" location="macos/Sources/ReverseAPIProxy/Proxy/ProxyHandler.swift:55">
P2: Non-CONNECT absolute URI parsing can route bracketed IPv6 HTTP requests to the wrong default port (443 instead of 80).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| beginBump(channelContext: channelContext, head: head) | ||
| return | ||
| } | ||
| guard let parsed = HostPort.parseAbsoluteURI(head.uri) else { |
Contributor
There was a problem hiding this comment.
P2: Non-CONNECT absolute URI parsing can route bracketed IPv6 HTTP requests to the wrong default port (443 instead of 80).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At macos/Sources/ReverseAPIProxy/Proxy/ProxyHandler.swift, line 55:
<comment>Non-CONNECT absolute URI parsing can route bracketed IPv6 HTTP requests to the wrong default port (443 instead of 80).</comment>
<file context>
@@ -0,0 +1,229 @@
+ beginBump(channelContext: channelContext, head: head)
+ return
+ }
+ guard let parsed = HostPort.parseAbsoluteURI(head.uri) else {
+ respondError(channelContext: channelContext, status: .badRequest)
+ return
</file context>
This is likely a transient issue. You can re-trigger a run from the dashboard. |
Fixes for PR #72 review comments (greptile, cubic, codex): - HostPort: validate ports against 1..65535, fix IPv6 default-port bug (http://[::1]/path was routed to 443), handle absolute URIs whose query/fragment appears before any "/" - CapturedFlow.url: wrap IPv6 host literals in brackets per RFC 3986 - FlowBus: bounded (.bufferingNewest) buffer per subscriber, expose subscriberCount for testing - LeafCertificateFactory: bounded LRU cache with configurable limit - TLSContextFactory: deduplicate concurrent context creation per host - UpstreamPump: register response continuation BEFORE flushing so fast responses don't drop the result; channelInactive now always fails the continuation (not just when head is missing); cancel() hook for upstream connect/setup errors - ProxyEngine.stop: surface real errors instead of swallowing them with try?; preserve serverChannel cleanup via defer - ProxyHandler: enforce a maxBodyBytes ceiling (32 MiB by default) and reply 413 Payload Too Large when exceeded - Package.swift: declare the NIOFoundationCompat dependency that ProxyHandler.swift already imports - .gitignore: commit Package.resolved to pin dependencies Tests: - HostPortTests: 24 cases (IPv4/IPv6/bracketed, port validation, absolute URI parsing, query/fragment handling) - CapturedFlowTests: IPv6 bracket wrapping, port-segment elision - FlowBusTests: multi-subscriber delivery, unsubscribe on stream termination, bounded-buffer drops oldest under back-pressure - LeafCertificateFactoryTests: caching, distinct hosts, LRU eviction, IPv4 host - TLSContextFactoryTests: cache, concurrent dedupe, distinct hosts
Contributor
There was a problem hiding this comment.
2 issues found across 15 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/Proxy/ProxyHandler.swift">
<violation number="1" location="macos/Sources/ReverseAPIProxy/Proxy/ProxyHandler.swift:75">
P2: Oversized request bodies are only rejected on `.end`, so clients can keep streaming indefinitely after crossing the limit. Return 413 and close as soon as the limit is exceeded.</violation>
</file>
<file name="macos/Sources/ReverseAPIProxy/Proxy/UpstreamPump.swift">
<violation number="1" location="macos/Sources/ReverseAPIProxy/Proxy/UpstreamPump.swift:89">
P2: Add an upstream response timeout; this await can block indefinitely when the upstream connection stalls without closing.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| guard case .buffering(var inflight) = phase else { return } | ||
| inflight.appendBody(buffer) | ||
| if inflight.body.readableBytes > maxBodyBytes { | ||
| phase = .rejected |
Contributor
There was a problem hiding this comment.
P2: Oversized request bodies are only rejected on .end, so clients can keep streaming indefinitely after crossing the limit. Return 413 and close as soon as the limit is exceeded.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At macos/Sources/ReverseAPIProxy/Proxy/ProxyHandler.swift, line 75:
<comment>Oversized request bodies are only rejected on `.end`, so clients can keep streaming indefinitely after crossing the limit. Return 413 and close as soon as the limit is exceeded.</comment>
<file context>
@@ -66,10 +71,19 @@ final class ProxyHandler: ChannelInboundHandler, RemovableChannelHandler, @unche
guard case .buffering(var inflight) = phase else { return }
inflight.appendBody(buffer)
+ if inflight.body.readableBytes > maxBodyBytes {
+ phase = .rejected
+ return
+ }
</file context>
| } | ||
|
|
||
| do { | ||
| let response = try await resultTask.value |
Contributor
There was a problem hiding this comment.
P2: Add an upstream response timeout; this await can block indefinitely when the upstream connection stalls without closing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At macos/Sources/ReverseAPIProxy/Proxy/UpstreamPump.swift, line 89:
<comment>Add an upstream response timeout; this await can block indefinitely when the upstream connection stalls without closing.</comment>
<file context>
@@ -35,74 +35,127 @@ actor UpstreamPump {
- try? await channel.close().get()
- return response
+ do {
+ let response = try await resultTask.value
+ try? await channel.close().get()
+ return response
</file context>
Suggested change
| let response = try await resultTask.value | |
| let response = try await withThrowingTaskGroup(of: UpstreamResponse.self) { group in | |
| group.addTask { try await resultTask.value } | |
| group.addTask { | |
| try await Task.sleep(nanoseconds: 60_000_000_000) | |
| throw UpstreamError.unexpected("upstream response timeout") | |
| } | |
| let first = try await group.next()! | |
| group.cancelAll() | |
| return first | |
| } |
Owner
Author
|
@greptile review Generated by Claude Code |
1 similar comment
Owner
Author
|
@greptile review Generated by Claude Code |
Contributor
There was a problem hiding this comment.
5 issues found across 10 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/Proxy/ProxyHandler.swift">
<violation number="1" location="macos/Sources/ReverseAPIProxy/Proxy/ProxyHandler.swift:161">
P2: Do not send `Content-Length` on successful CONNECT responses; RFC 7231 forbids `Content-Length`/`Transfer-Encoding` on 2xx CONNECT.</violation>
</file>
<file name="macos/Sources/rae-proxy/main.swift">
<violation number="1" location="macos/Sources/rae-proxy/main.swift:70">
P2: Using `-d` when trusting into `login.keychain-db` can cause trust installation to fail under normal user permissions.</violation>
</file>
<file name="macos/Sources/ReverseAPIProxy/Proxy/UpstreamPump.swift">
<violation number="1" location="macos/Sources/ReverseAPIProxy/Proxy/UpstreamPump.swift:59">
P2: Avoid creating `NIOSSLContext` inside `eventLoop.submit`; this moves expensive TLS context setup onto the event-loop thread and can degrade throughput/latency under concurrent HTTPS traffic.</violation>
</file>
<file name="macos/Sources/ReverseAPIProxy/CA/RootCertificateStore.swift">
<violation number="1" location="macos/Sources/ReverseAPIProxy/CA/RootCertificateStore.swift:30">
P1: `loadOrCreate` uses an unsynchronized check-then-create flow; concurrent calls can persist a mismatched certificate/private-key pair.</violation>
</file>
<file name="macos/Sources/ReverseAPIProxy/CA/CertificateAuthority.swift">
<violation number="1" location="macos/Sources/ReverseAPIProxy/CA/CertificateAuthority.swift:64">
P1: Validate that the loaded certificate and private key belong to the same keypair before returning `RootCertificate`.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| try fileManager.createDirectory(at: directory, withIntermediateDirectories: true) | ||
| try fileManager.setAttributes([.posixPermissions: 0o700], ofItemAtPath: directory.path) | ||
|
|
||
| if fileManager.fileExists(atPath: certificateURL.path), fileManager.fileExists(atPath: privateKeyURL.path) { |
Contributor
There was a problem hiding this comment.
P1: loadOrCreate uses an unsynchronized check-then-create flow; concurrent calls can persist a mismatched certificate/private-key pair.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At macos/Sources/ReverseAPIProxy/CA/RootCertificateStore.swift, line 30:
<comment>`loadOrCreate` uses an unsynchronized check-then-create flow; concurrent calls can persist a mismatched certificate/private-key pair.</comment>
<file context>
@@ -0,0 +1,51 @@
+ try fileManager.createDirectory(at: directory, withIntermediateDirectories: true)
+ try fileManager.setAttributes([.posixPermissions: 0o700], ofItemAtPath: directory.path)
+
+ if fileManager.fileExists(atPath: certificateURL.path), fileManager.fileExists(atPath: privateKeyURL.path) {
+ let certificatePEM = try String(contentsOf: certificateURL, encoding: .utf8)
+ let privateKeyPEM = try String(contentsOf: privateKeyURL, encoding: .utf8)
</file context>
| public static func loadRoot(certificatePEM: String, privateKeyPEM: String) throws -> RootCertificate { | ||
| let certificate = try Certificate(pemEncoded: certificatePEM) | ||
| let privateKey = try Certificate.PrivateKey(pemEncoded: privateKeyPEM) | ||
| return RootCertificate(certificate: certificate, privateKey: privateKey) |
Contributor
There was a problem hiding this comment.
P1: Validate that the loaded certificate and private key belong to the same keypair before returning RootCertificate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At macos/Sources/ReverseAPIProxy/CA/CertificateAuthority.swift, line 64:
<comment>Validate that the loaded certificate and private key belong to the same keypair before returning `RootCertificate`.</comment>
<file context>
@@ -53,4 +57,10 @@ public enum CertificateAuthority {
+ public static func loadRoot(certificatePEM: String, privateKeyPEM: String) throws -> RootCertificate {
+ let certificate = try Certificate(pemEncoded: certificatePEM)
+ let privateKey = try Certificate.PrivateKey(pemEncoded: privateKeyPEM)
+ return RootCertificate(certificate: certificate, privateKey: privateKey)
+ }
}
</file context>
Suggested change
| return RootCertificate(certificate: certificate, privateKey: privateKey) | |
| guard certificate.publicKey == privateKey.publicKey else { | |
| throw NSError(domain: "CertificateAuthority", code: 1, userInfo: [NSLocalizedDescriptionKey: "Certificate and private key do not match"]) | |
| } | |
| return RootCertificate(certificate: certificate, privateKey: privateKey) |
| let tlsContext = try await proxyContext.tlsContexts.serverContext(for: target.host) | ||
| try await eventLoop.submit { | ||
| var okHead = HTTPResponseHead(version: .http1_1, status: .ok) | ||
| okHead.headers.add(name: "Content-Length", value: "0") |
Contributor
There was a problem hiding this comment.
P2: Do not send Content-Length on successful CONNECT responses; RFC 7231 forbids Content-Length/Transfer-Encoding on 2xx CONNECT.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At macos/Sources/ReverseAPIProxy/Proxy/ProxyHandler.swift, line 161:
<comment>Do not send `Content-Length` on successful CONNECT responses; RFC 7231 forbids `Content-Length`/`Transfer-Encoding` on 2xx CONNECT.</comment>
<file context>
@@ -157,14 +157,15 @@ final class ProxyHandler: ChannelInboundHandler, RemovableChannelHandler, @unche
try await eventLoop.submit {
- let okHead = HTTPResponseHead(version: .http1_1, status: .ok)
+ var okHead = HTTPResponseHead(version: .http1_1, status: .ok)
+ okHead.headers.add(name: "Content-Length", value: "0")
channel.write(HTTPServerResponsePart.head(okHead), promise: nil)
channel.writeAndFlush(HTTPServerResponsePart.end(nil), promise: nil)
</file context>
Suggested change
| okHead.headers.add(name: "Content-Length", value: "0") | |
| // RFC 7231 §4.3.6: omit Content-Length for successful CONNECT |
Comment on lines
+70
to
+71
| "-d", | ||
| "-r", |
Contributor
There was a problem hiding this comment.
P2: Using -d when trusting into login.keychain-db can cause trust installation to fail under normal user permissions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At macos/Sources/rae-proxy/main.swift, line 70:
<comment>Using `-d` when trusting into `login.keychain-db` can cause trust installation to fail under normal user permissions.</comment>
<file context>
@@ -45,7 +54,166 @@ struct RAEProxyCLI {
+ process.executableURL = URL(fileURLWithPath: "/usr/bin/security")
+ process.arguments = [
+ "add-trusted-cert",
+ "-d",
+ "-r",
+ "trustRoot",
</file context>
Suggested change
| "-d", | |
| "-r", | |
| "-r", |
| try await channel.eventLoop.submit { | ||
| var clientConfig = TLSConfiguration.makeClientConfiguration() | ||
| clientConfig.applicationProtocols = ["http/1.1"] | ||
| let sslContext = try NIOSSLContext(configuration: clientConfig) |
Contributor
There was a problem hiding this comment.
P2: Avoid creating NIOSSLContext inside eventLoop.submit; this moves expensive TLS context setup onto the event-loop thread and can degrade throughput/latency under concurrent HTTPS traffic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At macos/Sources/ReverseAPIProxy/Proxy/UpstreamPump.swift, line 59:
<comment>Avoid creating `NIOSSLContext` inside `eventLoop.submit`; this moves expensive TLS context setup onto the event-loop thread and can degrade throughput/latency under concurrent HTTPS traffic.</comment>
<file context>
@@ -52,11 +53,13 @@ actor UpstreamPump {
+ try await channel.eventLoop.submit {
+ var clientConfig = TLSConfiguration.makeClientConfiguration()
+ clientConfig.applicationProtocols = ["http/1.1"]
+ let sslContext = try NIOSSLContext(configuration: clientConfig)
+ let sslHandler = try NIOSSLClientHandler(context: sslContext, serverHostname: host)
+ try channel.pipeline.syncOperations.addHandler(sslHandler, position: .first)
</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
2 participants
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.
First slice of the macOS app: a pure-Swift MITM HTTP/HTTPS proxy built on SwiftNIO + NIOSSL + swift-certificates.
What's in
macos/Swift Package targeting macOS 14+ReverseAPIProxylibraryCertificateAuthority— generates an in-memory P-256 root CA viaswift-certificatesLeafCertificateFactory— mints per-host leaf certs on demand (DNS + IPv4/IPv6 SAN), caches them, returnsNIOSSLCertificate/NIOSSLPrivateKeyTLSContextFactory— builds and cachesNIOSSLContextper hostProxyEngine— bootstraps a NIO server on127.0.0.1:8888ProxyHandler— handles plain HTTP forwarding and CONNECT-based TLS bumping with synchronous pipeline reconfigurationUpstreamPump— opens upstream connections (TLS or plain), forwards the buffered request, returns the responseFlowBus/CapturedFlow— actor-backed async event stream for captured requestsrae-proxyCLI — generates a root CA, starts the proxy, prints captured flows liveOut of scope (later milestones)
How to try (on macOS)
Stacked PRs will follow on top of this branch.
Generated by Claude Code
Summary by cubic
Add a pure‑Swift MITM HTTP/HTTPS proxy core for macOS with TLS bumping and live flow capture. Ships the
ReverseAPIProxylibrary andrae-proxyCLI, with fixes for IPv6 parsing, upstream disconnects, device system‑proxy capture, and removal of the macOS certificate trust prompt.Bug Fixes
LeafCertificateFactory; deduplicated TLS context creation inTLSContextFactory; improvedProxyEngine.stop()cleanup.rae-proxy: removed the certificate trust prompt and fixed system proxy application to capture device traffic.Dependencies
NIOFoundationCompattoReverseAPIProxyand restored the SwiftPM macOS build.Written for commit a16b471. Summary will update on new commits. Review in cubic
Greptile Summary
This PR introduces the core SwiftNIO MITM proxy for the macOS app: a pure-Swift HTTP/HTTPS interceptor built on NIO + NIOSSL + swift-certificates, along with the
rae-proxyCLI and a solid XCTest suite covering CA, LRU caching, HostPort parsing, FlowBus, and TLS context deduplication.ReverseAPIProxylibrary shipsCertificateAuthority+LeafCertificateFactory(LRU-capped per-host cert minting),TLSContextFactory(actor with inflight-task deduplication),ProxyHandler(CONNECT bump + plain HTTP forwarding, 32 MiB body cap),UpstreamPump(async request/response with a lock-guardedCheckedContinuation), andFlowBus(bounded async event stream with per-subscriber backpressure).HostPort, upstream mid-response disconnect now always resumes the continuation viachannelInactive, and per-subscriber buffer bounding inFlowBus.Confidence Score: 5/5
Safe to merge; all findings are non-blocking quality improvements with no correctness regressions on the happy path.
The core proxy logic is well-structured and all previously identified bugs (IPv6 default port, continuation hang on upstream disconnect) are demonstrably fixed with corresponding tests. The two remaining observations are improvements for a later milestone rather than blockers.
ProxyHandler.swift and UpstreamPump.swift carry the two P2 notes worth tracking for a follow-up; no files require blocking attention.
Important Files Changed
Comments Outside Diff (3)
macos/Sources/ReverseAPIProxy/CA/LeafCertificateFactory.swift, line 137-141 (link)Materials.rootCertificateis populated inLeafCertificateFactorybut is never read byTLSContextFactory.serverContextor any other consumer — the field is dead code. If the intent was to include the root in the certificate chain sent to clients during the TLS handshake,TLSContextFactorywould need to add it tocertificateChain. If it was intentionally omitted (roots are typically not sent in the chain), the unused field should be removed to avoid confusion.Prompt To Fix With AI
macos/Sources/ReverseAPIProxy/Proxy/UpstreamPump.swift, line 736-750 (link)TaskinsideProxyHandler.dispatchindefinitely, holding the downstream client connection open and accumulating unresolved continuations. Even a conservative timeout (e.g. 30 s for connect, 60 s for the full response) would bound the resource leak.Prompt To Fix With AI
macos/Sources/ReverseAPIProxy/ProxyEngine.swift, line 856-868 (link)ProxyEngineis aclassmarked@unchecked Sendablewith a mutableserverChannelproperty.start()andstop()are bothasyncand can be called concurrently — neither is protected by an actor, a lock, or any other exclusion mechanism. Concurrent calls could race on theserverChannelnil-check and leave bothserverChannelset and the channel closed, or start two server sockets. ConvertingProxyEngineto anactorwould eliminate this without requiring manual locking.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "M1 review fixes + tests" | Re-trigger Greptile