mitm: address post-merge review on #150 (IPv6 + Host header + doc sync)#151
Merged
dangtony98 merged 5 commits intomainfrom May 6, 2026
Merged
mitm: address post-merge review on #150 (IPv6 + Host header + doc sync)#151dangtony98 merged 5 commits intomainfrom
dangtony98 merged 5 commits intomainfrom
Conversation
Two correctness bugs and two cosmetic misses surfaced after #150 merged. All four addressed here. 1. IPv6 literal target double-bracketed (forward.go:80) `POST http://[::1]/path HTTP/1.1` produced target "[[::1]]:80": net.SplitHostPort fails on bracketed-no-port, the fallback fed the still-bracketed string back through net.JoinHostPort, which sees ":" inside and re-wraps. Replace with r.URL.Hostname() (which strips brackets) and r.URL.Port() defaulting to "80". Adds TestMITMForwardIPv6LiteralCanonicalises (binds an upstream on ::1 and asserts upstream sees Host: [::1]:port). 2. Wire Host header strips port for non-default-port upstreams (forward.go: outReq.Host = host) RFC 7230 §5.4 requires Host to equal the URI authority including non-default port. Old line set outReq.Host = host (port-stripped) so http://internal.example:8080/api forwarded as Host: internal.example, breaking vhost routing on the dominant plain-HTTP forward case (k8s ClusterIP, dev servers, microservices). Fix: outReq.Host = target (port-included). Updates TestMITMForwardPlainHTTPInjectsCredentials to assert against upstreamAuthority (host:port) — the old assertion baked in the buggy behaviour. 3. Startup banner still printed "routing HTTPS" (cmd/run.go:149) Updated to "routing HTTP/HTTPS" + matching step comment, so the first thing operators see after launch matches the help text and docs. 4. Doc cross-sync: 14 pages still described HTTPS_PROXY-only Quickstarts (claude-code, codex, cursor, hermes-agent, opencode, nanoclaw, openclaw, custom-agent), docs/index.mdx, docs/agents/{overview,protocol}.mdx, docs/learn/{security, services}.mdx, docs/guides/connect-custom-agent.mdx, and docs/self-hosting/{local,docker,fly-io}.mdx now mention HTTP_PROXY alongside HTTPS_PROXY and describe both code paths the listener accepts. The "Set HTTPS_PROXY manually" section in the custom- agent guide gained an `export HTTP_PROXY="$HTTPS_PROXY"` line.
|
💬 Discussion in Slack: #pr-review-agent-vault-151-mitm-address-post-merge-review-on-150-ipv6-host-header Posted by Review Police — reviews, comments, new commits, and CI failures will stream into this channel. |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Wider sweep after the post-merge review surfaced more stale references
the four-comment review didn't enumerate. Highest-impact missing
surface was the five embedded server instruction files served to
invited agents:
- internal/server/instructions.txt
- internal/server/persistent_agent_instructions.txt
- internal/server/persistent_instructions_{admin,member,proxy}.txt
These are normative protocol guidance baked into the binary; without
this update, invited agents would set HTTPS_PROXY only and continue
bypassing the broker for plain-http:// upstreams even after #150
shipped.
Also updated:
- examples/daytona-openai-realtime/run.mjs — `delete env.HTTP_PROXY` /
`http_proxy` in the egress-block test (parity with HTTPS_PROXY) and
added `-e HTTP_PROXY=...` to the example docker run.
- web/src/pages/home/AllAgentsTab.tsx — manual proxy-setup snippet now
exports both HTTPS_PROXY and HTTP_PROXY.
- cmd/skill_{cli,http}.md — "transparent HTTPS proxy" → "transparent
HTTP/HTTPS proxy" in the canonical agent-facing skill blurb;
skill_http frontmatter description matched.
- README.md — server-startup line now describes the same.
- docs/self-hosting/environment-variables.mdx — AGENT_VAULT_ISOLATION
row and trailing CA-PEM paragraph mention HTTP_PROXY too.
- docs/quickstart/{nanoclaw,openclaw}.mdx — example bullets reference
both env vars.
- internal/brokercore/proxyauth.go — `Basic` form doc explains both
env vars take the same proxy URL.
- internal/brokercore/brokercore.go — broker-scoped header doc says
Proxy-Authorization arrives via either ingress shape.
- internal/server/handle_mitm.go — CA-PEM-not-listening guard mentions
both env vars.
No code-path or behavioural change beyond the original #151 fixes;
this is doc/instruction parity.
Two findings from the simplify pass: 1. Missing CONNECT-path regression gate for the outReq.Host = target change. Previous tests didn't capture r.Host on the upstream, so the post-#151 behaviour (Host header now includes non-default port, per RFC 7230 §5.4) had no test coverage on the legacy path. Added sawHost capture + assertion to TestMITMInjectsCredentials so any drift on the CONNECT side fails loudly. 2. TestMITMForwardIPv6LiteralCanonicalises used inline defer srv.Close() while the rest of the harness uses Shutdown(ctx) via t.Cleanup (proxy_test.go:124-128). Aligned with the existing pattern — drains in-flight connections cleanly, no Use-of-closed- network-connection error swallowed inside forwardRequest under slow CI.
The variable is now used to populate both HTTPS_PROXY and HTTP_PROXY exports in the manual proxy-setup snippet (since #151), so the HTTPS-only name was misleading. Cosmetic-only — no behavioural change.
Contributor
Author
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Two findings on the post-merge-review PR:
1. Renaming the heading at docs/guides/connect-custom-agent.mdx:119
from "Set HTTPS_PROXY manually" to "Set the proxy env vars
manually" changed the Mintlify slug to
#set-the-proxy-env-vars-manually, but two references still pointed
at the old #set-https-proxy-manually anchor:
- docs/guides/connect-custom-agent.mdx:41 (same-page link)
- docs/quickstart/custom-agent.mdx:62 (cross-page link)
Both now resolved to the right page but jumped nowhere. Updated to
the new slug.
2. docs/agents/protocol.mdx:89 documented the proxy URL as
http://{token}:{vault}@host:14322 — pre-existing typo, but
contradicted line 90 of the same table (which calls out the
listener as TLS-wrapped) and the parallel page at
docs/self-hosting/environment-variables.mdx:81 (which already had
https://). BuildProxyEnv emits https:// when the server reports
X-MITM-TLS=1, which it always does. Plaintext on the wrapped
listener fails the TLS handshake. Fixed to https:// and added a
short note explaining why.
2 tasks
dangtony98
added a commit
that referenced
this pull request
May 6, 2026
## Summary Three findings from the post-merge review on #151 that landed after the squash: - **Banner host (cmd/run.go:149)**: hardcoded `127.0.0.1` in the "routing HTTP/HTTPS through MITM proxy" banner while `augmentEnvWithMITM` derives the host from `--address`. Misleading on remote vault deploys. Extracted `resolveMITMHost(addr)` so banner and env-var path share one source. - **IPv6 test naming (internal/mitm/forward_test.go)**: `TestMITMForwardIPv6LiteralCanonicalises` was named for the no-port canonicalisation branch, but the request URL carries an explicit port — `net.SplitHostPort` succeeds in the old code and the buggy fallback is never taken. The assertion actually verifies the Host-header port-preservation fix. Renamed to `TestMITMForwardIPv6PreservesHostHeader` and rewrote the comment to match. - **Dead instruction files**: only `persistent_instructions_admin.txt` is `//go:embed`-ed; both invite-redeem paths in `handle_agents.go` hardcode `persistentInstructionsAdmin` regardless of role. The other four files (`persistent_instructions_member.txt`, `persistent_instructions_proxy.txt`, `persistent_agent_instructions.txt`, `instructions.txt`) shipped on disk but never reached any agent. Doc cross-syncs kept repainting them. Deleted. Plus a small `/simplify` follow-up tightening the `resolveMITMHost` doc comment. ## Test plan - [x] `go build ./...` - [x] `go test ./cmd/... ./internal/mitm/... ./internal/server/...` --------- Co-authored-by: Claude <noreply@anthropic.com>
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.
Follow-up to #150 — addresses the four findings from the post-merge automated review.
Two correctness bugs
1. IPv6 literal target double-bracketed (
internal/mitm/forward.go)For
POST http://[::1]/path HTTP/1.1:r.URL.Host == "[::1]"(Go preserves brackets so the URL round-trips).net.SplitHostPort("[::1]")fails withmissing port in address.host = "[::1]",port = "80".net.JoinHostPort("[::1]", "80")saw:inside the host string, applied its bracket rule, and produced[[::1]]:80— double-bracketed and unreachable.CONNECT path was unaffected because RFC 7230 §4.3.6 requires CONNECT request lines to be
host:port, soSplitHostPortalways succeeded there.Fix: switch to
r.URL.Hostname()(strips brackets) andr.URL.Port()(returns""when omitted). Idiomatic Go pattern, handles IPv4, named hosts, and bracketed IPv6 uniformly.Test: new
TestMITMForwardIPv6LiteralCanonicalisesbinds a real listener on[::1]:0, sends an absolute-form forward request, and asserts the upstream seesHost: [::1]:<port>(correctly bracketed exactly once). Skips on hosts without IPv6 loopback.2. Wire Host header strips port for non-default-port upstreams
Old code set
outReq.Host = host(port-stripped), sohttp://internal.example:8080/apiforwarded asHost: internal.example— RFC 7230 §5.4 violation. The legacy CONNECT path tolerated this because HTTPS:443 is canonical and the port can be omitted, but the new plain-HTTP forward path makes non-default ports the dominant case (k8s ClusterIP, dev servers, microservices, the Tailscale Aperture / Hermes scenario in #132).Fix:
outReq.Host = target(port-included), with a comment explaining the RFC and why the legacy behaviour was effectively dead code.Test:
TestMITMForwardPlainHTTPInjectsCredentialsupdated. The old assertionsawHost == upstreamHost(port-stripped) actively baked in the buggy behaviour; new assertion checks againstupstreamAuthority(host:port).Two cosmetic misses
3. Startup banner still said "routing HTTPS"
cmd/run.go:149 emits the user's first signal after
vault runsucceeds. Updated torouting HTTP/HTTPS through MITM proxy (...)and the surrounding step comment to match.4. Doc cross-sync — 14 pages still described HTTPS_PROXY-only
The original PR updated 8 pages but missed 14 others. Updated:
claude-code,codex,cursor,hermes-agent,opencode,nanoclaw,openclaw,custom-agentdocs/index.mdxdocs/agents/overview.mdxdocs/agents/protocol.mdx— frontmatter description and section headingdocs/learn/security.mdx— heading + bodydocs/learn/services.mdx— Twilio walkthroughdocs/guides/connect-custom-agent.mdx— frontmatter, body, env-var table (split into HTTPS_PROXY + HTTP_PROXY rows), heading, manual-setup snippet (export HTTP_PROXY="$HTTPS_PROXY")docs/self-hosting/{local,docker,fly-io}.mdxVerification
make test(full Go suite, race detector clean) —okacross all packagesmake build— succeedsgolangci-lint run ./...—0 issuesoutReq.Host = targetchangeFiles
internal/mitm/forward.go— IPv6 fix + Host header fix + clarifying commentsinternal/mitm/forward_test.go— IPv6 regression test + tightened Host assertioncmd/run.go— banner + step commentHTTP_PROXY/HTTPS_PROXYparity