mitm: address remaining review on #151

cmd/run.go

@@ -6,11 +6,13 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"net"
 	"net/http"
 	"net/url"
 	"os"
 	"os/exec"
 	"path/filepath"
+	"strconv"
 	"strings"
 	"syscall"
 
@@ -146,7 +148,7 @@ func runCmdRunE(cmd *cobra.Command, args []string) error {
 		return err
 	}
 	env = newEnv
-	fmt.Fprintf(os.Stderr, "%s routing HTTP/HTTPS through MITM proxy (127.0.0.1:%d)\n", successText("agent-vault:"), mitmPort)
+	fmt.Fprintf(os.Stderr, "%s routing HTTP/HTTPS through MITM proxy (%s)\n", successText("agent-vault:"), net.JoinHostPort(resolveMITMHost(addr), strconv.Itoa(mitmPort)))
 
 	// 7. If the target command is a supported agent, offer to install the
 	//    Agent Vault skill (only when not already present).
@@ -369,6 +371,18 @@ func stripEnvKeys(env []string, keys map[string]struct{}) []string {
 	return out
 }
 
+// resolveMITMHost extracts the host the child process should dial for
+// the MITM proxy from the configured server address. Falls back to
+// loopback when addr is unparseable or has no host.
+func resolveMITMHost(addr string) string {
+	if u, err := url.Parse(addr); err == nil {
+		if h := u.Hostname(); h != "" {
+			return h
+		}
+	}
+	return "127.0.0.1"
+}
+
 // requireMITMEnv calls augmentEnvWithMITM and converts both transport
 // failures and a server-side --mitm-port 0 into actionable errors.
 // MITM is the only ingress, so neither case is recoverable for vault run.
@@ -424,16 +438,9 @@ func augmentEnvWithMITM(env []string, addr, token, vault, caPath string) ([]stri
 		return env, 0, false, fmt.Errorf("write CA: %w", err)
 	}
 
-	mitmHost := "127.0.0.1"
-	if u, err := url.Parse(addr); err == nil {
-		if h := u.Hostname(); h != "" {
-			mitmHost = h
-		}
-	}
-
 	env = stripEnvKeys(env, mitmInjectedKeys)
 	env = append(env, isolation.BuildProxyEnv(isolation.ProxyEnvParams{
-		Host:    mitmHost,
+		Host:    resolveMITMHost(addr),
 		Port:    port,
 		Token:   token,
 		Vault:   vault,

internal/isolation/env.go

@@ -4,7 +4,9 @@ package isolation
 
 import (
 	"fmt"
+	"net"
 	"net/url"
+	"strconv"
 )
 
 const (
@@ -46,7 +48,7 @@ func BuildProxyEnv(p ProxyEnvParams) []string {
 	proxyURL := (&url.URL{
 		Scheme: scheme,
 		User:   url.UserPassword(p.Token, p.Vault),
-		Host:   fmt.Sprintf("%s:%d", p.Host, p.Port),
+		Host:   net.JoinHostPort(p.Host, strconv.Itoa(p.Port)),
 	}).String()
 	return []string{
 		"HTTPS_PROXY=" + proxyURL,

internal/isolation/env_test.go

@@ -95,6 +95,34 @@ func TestBuildContainerEnv_FirewallPortsEmitted(t *testing.T) {
 	}
 }
 
+// IPv6 literals must be bracketed in the proxy URL authority so
+// net.SplitHostPort and downstream HTTP clients accept them. The bare
+// "::1:port" form is rejected as "too many colons".
+func TestBuildProxyEnv_IPv6HostIsBracketed(t *testing.T) {
+	env := BuildProxyEnv(ProxyEnvParams{
+		Host:    "::1",
+		Port:    14322,
+		Token:   "tok",
+		Vault:   "v",
+		CAPath:  "/tmp/ca.pem",
+		MITMTLS: true,
+	})
+	vars := envMap(env)
+	u, err := url.Parse(vars["HTTPS_PROXY"])
+	if err != nil {
+		t.Fatalf("parse HTTPS_PROXY %q: %v", vars["HTTPS_PROXY"], err)
+	}
+	if u.Hostname() != "::1" {
+		t.Errorf("hostname = %q, want ::1", u.Hostname())
+	}
+	if u.Port() != "14322" {
+		t.Errorf("port = %q, want 14322", u.Port())
+	}
+	if !strings.Contains(vars["HTTPS_PROXY"], "[::1]:14322") {
+		t.Errorf("HTTPS_PROXY = %q, want bracketed [::1]:14322 authority", vars["HTTPS_PROXY"])
+	}
+}
+
 // HTTP_PROXY mirrors HTTPS_PROXY: both point at the same TLS-wrapped
 // MITM ingress so plain http:// upstreams route through the broker via
 // absolute-form forward-proxy requests.

internal/mitm/forward_test.go

@@ -591,17 +591,19 @@ func TestMITMForwardKeepalivePersistsAcrossRequests(t *testing.T) {
 	}
 }
 
-// TestMITMForwardIPv6LiteralCanonicalises guards against the IPv6
-// double-bracket regression: net.SplitHostPort/JoinHostPort on the
-// bracketed-no-port form ("[::1]") produces "[[::1]]:80". Going
-// through r.URL.Hostname() / r.URL.Port() instead yields the
-// canonical "[::1]:80" target. Sending raw via dialProxyTLS because
-// Go's http.Client routinely rewrites URLs through ProxyURL in ways
+// TestMITMForwardIPv6PreservesHostHeader locks in the Host-header
+// port-preservation fix on the IPv6 forward-proxy path: outReq.Host
+// must equal target ("[::1]:port"), not the port-stripped form ("::1")
+// the old code emitted. The request line carries an explicit port so
+// the no-port canonicalisation branch (URL.Hostname/Port instead of
+// net.SplitHostPort) is not driven here — exercising that end-to-end
+// would require binding port 80 on ::1. Sending raw via dialProxyTLS
+// because Go's http.Client rewrites URLs through ProxyURL in ways
 // that would obscure what we want to assert.
-func TestMITMForwardIPv6LiteralCanonicalises(t *testing.T) {
-	// Bind an upstream on ::1 so the URL we send is exactly the
-	// IPv6-literal-no-port form. SkipNow if the host has no IPv6
-	// loopback (CI sometimes).
+func TestMITMForwardIPv6PreservesHostHeader(t *testing.T) {
+	// Bind an upstream on an ephemeral ::1 port so we can send an
+	// IPv6-literal-with-port URL through the forward proxy. SkipNow if
+	// the host has no IPv6 loopback (CI sometimes).
 	l, err := net.Listen("tcp", "[::1]:0")
 	if err != nil {
 		t.Skipf("no IPv6 loopback available: %v", err)