Reverse Port Forwarding (ssh -R) [Spec + Impl]

.gitignore

@@ -40,3 +40,4 @@ Thumbs.db
 !**/.specify/memory/constitution.md
 docs/book/
 .playwright-mcp/
+.mcp.json

CLAUDE.md

@@ -11,5 +11,5 @@ export OPENAI_API_KEY="$OPENSHELL_OPENAI_API_KEY"
 <!-- SPECKIT START -->
 For additional context about technologies to be used, project structure,
 shell commands, and other important information, read the current plan
-at specs/024-global-policy-flag/plan.md
+at specs/025-reverse-port-forwarding/plan.md
 <!-- SPECKIT END -->

brainstorm/030-upstream-review-findings.md

@@ -145,6 +145,87 @@ gateway unless it explicitly strips the field.
 This is not a code fix but an API design clarification to raise with the
 upstream team during the next SDK PR review.
 
+## PR #47 Findings (Reverse Port Forwarding)
+
+From the multi-agent review of PR #47, these findings improve the SDK
+code that will be contributed upstream.
+
+### Must Fix (bug)
+
+#### Fake Forward missing sandboxName validation
+
+**File:** `fake/tcp.go` (Forward method)
+**Severity:** Critical (flagged by 4 independent agents)
+
+Fake `Forward` was missing `sandboxName` validation that the real client
+has. Real client rejects empty sandboxName with `ErrorInvalidArgument`,
+but the fake skipped from closedFunc check to port validation. This
+violates the fake-real parity invariant.
+
+**Applied in:** `bd0909b1` (downstream)
+
+### Should Fix (robustness)
+
+#### Real RemoteListen discards context parameter
+
+**File:** `tcp_client.go` (RemoteListen)
+**Severity:** Important
+
+The stub used `_ context.Context`, hiding the requirement for context in
+the future implementation. Forward and Listen both name their context
+parameter. When RemoteListen gets a real implementation, the developer
+copying the stub signature would miss wiring up context.
+
+**Applied in:** `bd0909b1` (downstream)
+
+#### remoteListenConfig.bindAddress default disagrees with documentation
+
+**File:** `tcp.go` (remoteListenConfig struct)
+**Severity:** Minor, security-relevant
+
+The struct's zero-value for `bindAddress` is empty string, but
+`WithRemoteBindAddress` documents the default as `"127.0.0.1"`. When the
+real implementation lands, an empty bind address could default to
+`0.0.0.0` server-side, exposing the forwarded port on all interfaces.
+
+**Fix:** When implementing RemoteListen, initialize config with
+`bindAddress: "127.0.0.1"`, matching the `listenConfig` pattern.
+
+#### tcpForwardConn.Close() drops readLoop errors
+
+**File:** `tcp_client.go` (Close method, pre-existing)
+**Severity:** Notable
+
+If CloseSend succeeds but the readLoop encountered a transport error,
+that error is silently dropped. The caller of Close() gets nil even
+though the connection had an error.
+
+**Fix:** After `<-c.done`, return `c.err` if CloseSend returned nil.
+
+### Design Considerations (for real implementation)
+
+#### RemoteListen return type
+
+**File:** `tcp.go` (TCPInterface)
+
+`RemoteListen` returns bare `error`, providing no ready-signal. Unlike
+`Listen` which returns `net.Listener` (caller knows the listener is
+ready when the call returns), `RemoteListen` gives no way to know when
+the remote side is accepting connections. Consider returning a richer
+type (e.g., `RemoteListener` with `Ready() <-chan struct{}` and
+`Close()`).
+
+#### Closed-client detection in real TCP client
+
+**File:** `tcp_client.go`
+
+The real TCP client has no explicit closed-state check in any method
+(Forward, Listen, RemoteListen). It relies on gRPC transport errors to
+surface closed connections. The fake client uses `closedFunc()`. This
+asymmetry means error messages differ between real and fake when the
+client is closed. Consider whether the real client should check its
+connection state before making gRPC calls.
+
 ## Downstream Commits
 
 All code fixes were applied in the downstream repo and will be included
@@ -155,3 +236,4 @@ when the SDK code is contributed upstream:
 | `6fc1eda6` | Coverage tests (contextError, WaitReady DeadlineExceeded/Deleting) |
 | `e0039c78` | Deterministic deadline in fake sandbox test |
 | `c2fa08db` | All six cc-review fixes (helper, race, rename, TLS, tests, comment) |
+| `bd0909b1` | PR #47 fixes (fake Forward parity, context param, message assertions, IPv6 case) |

openshell/v1/doc.go

@@ -242,6 +242,26 @@
 //	    v1.WithForwardServiceID("billing-db"),
 //	)
 //
+// # Reverse Port Forwarding
+//
+// Expose a local service to a sandbox (ssh -R equivalent). RemoteListen
+// blocks until context cancellation or a permanent error. Each connection
+// made to the remote port inside the sandbox is tunneled back to the local
+// target:
+//
+//	err := client.TCP().RemoteListen(ctx, "default", "my-sandbox", 8080, "localhost:8080")
+//	if err != nil {
+//	    log.Fatal(err)
+//	}
+//
+// Use options to customize the sandbox-side bind address or attach a
+// service identifier for audit logging:
+//
+//	err := client.TCP().RemoteListen(ctx, "default", "my-sandbox", 8080, "localhost:8080",
+//	    v1.WithRemoteBindAddress("0.0.0.0"),
+//	    v1.WithRemoteListenServiceID("mcp-proxy"),
+//	)
+//
 // # SSH Tunneling
 //
 // Create an SSH tunnel to a sandbox port in a single call. Tunnel combines

openshell/v1/fake/tcp.go

@@ -24,12 +24,15 @@ func newFakeTCPClient(closedFunc func() bool) *fakeTCPClient {
 	return &fakeTCPClient{closedFunc: closedFunc}
 }
 
-// Forward returns Unimplemented. Ports outside 1-65535 are rejected with
-// InvalidArgument to match the real client's behavior.
-func (c *fakeTCPClient) Forward(_ context.Context, _, _ string, port uint32, _ ...v1.ForwardOption) (io.ReadWriteCloser, error) {
+// Forward returns Unimplemented. Empty sandboxName and ports outside 1-65535
+// are rejected with InvalidArgument to match the real client's behavior.
+func (c *fakeTCPClient) Forward(_ context.Context, _, sandboxName string, port uint32, _ ...v1.ForwardOption) (io.ReadWriteCloser, error) {
 	if c.closedFunc() {
 		return nil, &types.StatusError{Code: types.ErrorUnavailable, Message: "client is closed"}
 	}
+	if sandboxName == "" {
+		return nil, &types.StatusError{Code: types.ErrorInvalidArgument, Message: "sandbox name must not be empty"}
+	}
 	if port == 0 || port > 65535 {
 		return nil, &types.StatusError{Code: types.ErrorInvalidArgument, Message: fmt.Sprintf("port must be in range 1-65535, got %d", port)}
 	}
@@ -55,5 +58,24 @@ func (c *fakeTCPClient) Listen(_ context.Context, _, sandboxName string, remoteP
 	return nil, &types.StatusError{Code: types.ErrorUnimplemented, Message: "Listen is not supported by the fake client"}
 }
 
+// RemoteListen validates inputs then returns Unimplemented. The fake does not
+// set up any reverse tunnel; it checks that sandboxName is non-empty,
+// remotePort is in the range 1-65535, and localTarget parses as host:port.
+func (c *fakeTCPClient) RemoteListen(_ context.Context, _, sandboxName string, remotePort uint32, localTarget string, _ ...v1.RemoteListenOption) error {
+	if c.closedFunc() {
+		return &types.StatusError{Code: types.ErrorUnavailable, Message: "client is closed"}
+	}
+	if sandboxName == "" {
+		return &types.StatusError{Code: types.ErrorInvalidArgument, Message: "sandbox name must not be empty"}
+	}
+	if remotePort == 0 || remotePort > 65535 {
+		return &types.StatusError{Code: types.ErrorInvalidArgument, Message: fmt.Sprintf("port must be in range 1-65535, got %d", remotePort)}
+	}
+	if _, _, err := net.SplitHostPort(localTarget); err != nil {
+		return &types.StatusError{Code: types.ErrorInvalidArgument, Message: fmt.Sprintf("invalid localTarget %q: %v", localTarget, err)}
+	}
+	return &types.StatusError{Code: types.ErrorUnimplemented, Message: "RemoteListen is not supported by the fake client"}
+}
+
 // Compile-time check that fakeTCPClient implements v1.TCPInterface.
 var _ v1.TCPInterface = (*fakeTCPClient)(nil)

openshell/v1/fake/tcp_test.go

@@ -37,6 +37,14 @@ func TestFakeTCP_Forward_WithForwardOption(t *testing.T) {
 	assert.True(t, types.IsUnimplemented(err))
 }
 
+func TestFakeTCP_Forward_EmptySandboxName(t *testing.T) {
+	c := newFakeTCPClient(func() bool { return false })
+	_, err := c.Forward(context.Background(), "default", "", 8080)
+	require.Error(t, err)
+	assert.True(t, types.IsInvalidArgument(err))
+	assert.Contains(t, err.Error(), "sandbox name")
+}
+
 func TestFakeTCP_Forward_InvalidPort(t *testing.T) {
 	c := newFakeTCPClient(func() bool { return false })
 	_, err := c.Forward(context.Background(), "default", "sandbox-1", 0)
@@ -102,3 +110,125 @@ func TestFakeTCP_Listen_WithOptions(t *testing.T) {
 	require.Error(t, err)
 	assert.True(t, types.IsUnimplemented(err))
 }
+
+// --- RemoteListen tests ---
+
+func TestFakeTCP_RemoteListen_ReturnsUnimplemented(t *testing.T) {
+	c := newFakeTCPClient(func() bool { return false })
+	err := c.RemoteListen(context.Background(), "default", "my-sandbox", 8080, "localhost:8080")
+	require.Error(t, err)
+	assert.True(t, types.IsUnimplemented(err))
+}
+
+func TestFakeTCP_RemoteListen_ValidationParity(t *testing.T) {
+	tests := []struct {
+		name        string
+		closed      bool
+		sandboxName string
+		port        uint32
+		localTarget string
+		checkErr    func(error) bool
+		errName     string
+	}{
+		{
+			name:        "closed client",
+			closed:      true,
+			sandboxName: "my-sandbox",
+			port:        8080,
+			localTarget: "localhost:8080",
+			checkErr:    types.IsUnavailable,
+			errName:     "Unavailable",
+		},
+		{
+			name:        "empty sandbox name",
+			sandboxName: "",
+			port:        8080,
+			localTarget: "localhost:8080",
+			checkErr:    types.IsInvalidArgument,
+			errName:     "InvalidArgument",
+		},
+		{
+			name:        "port zero",
+			sandboxName: "my-sandbox",
+			port:        0,
+			localTarget: "localhost:8080",
+			checkErr:    types.IsInvalidArgument,
+			errName:     "InvalidArgument",
+		},
+		{
+			name:        "port too high",
+			sandboxName: "my-sandbox",
+			port:        65536,
+			localTarget: "localhost:8080",
+			checkErr:    types.IsInvalidArgument,
+			errName:     "InvalidArgument",
+		},
+		{
+			name:        "malformed target missing port",
+			sandboxName: "my-sandbox",
+			port:        8080,
+			localTarget: "localhost",
+			checkErr:    types.IsInvalidArgument,
+			errName:     "InvalidArgument",
+		},
+		{
+			name:        "malformed target empty",
+			sandboxName: "my-sandbox",
+			port:        8080,
+			localTarget: "",
+			checkErr:    types.IsInvalidArgument,
+			errName:     "InvalidArgument",
+		},
+		{
+			name:        "malformed target bare IPv6",
+			sandboxName: "my-sandbox",
+			port:        8080,
+			localTarget: "::1",
+			checkErr:    types.IsInvalidArgument,
+			errName:     "InvalidArgument",
+		},
+		{
+			name:        "boundary port 1",
+			sandboxName: "my-sandbox",
+			port:        1,
+			localTarget: "localhost:8080",
+			checkErr:    types.IsUnimplemented,
+			errName:     "Unimplemented",
+		},
+		{
+			name:        "boundary port 65535",
+			sandboxName: "my-sandbox",
+			port:        65535,
+			localTarget: "localhost:8080",
+			checkErr:    types.IsUnimplemented,
+			errName:     "Unimplemented",
+		},
+		{
+			name:        "ipv6 target",
+			sandboxName: "my-sandbox",
+			port:        8080,
+			localTarget: "[::1]:8080",
+			checkErr:    types.IsUnimplemented,
+			errName:     "Unimplemented",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			c := newFakeTCPClient(func() bool { return tt.closed })
+			err := c.RemoteListen(context.Background(), "default", tt.sandboxName, tt.port, tt.localTarget)
+			require.Error(t, err)
+			assert.True(t, tt.checkErr(err), "expected %s, got: %v", tt.errName, err)
+		})
+	}
+}
+
+func TestFakeTCP_RemoteListen_WithOptions(t *testing.T) {
+	c := newFakeTCPClient(func() bool { return false })
+	err := c.RemoteListen(context.Background(), "default", "my-sandbox", 8080, "localhost:8080",
+		v1.WithRemoteBindAddress("0.0.0.0"),
+		v1.WithRemoteListenServiceID("mcp-proxy"),
+	)
+	require.Error(t, err)
+	assert.True(t, types.IsUnimplemented(err))
+}

openshell/v1/tcp.go

@@ -60,9 +60,35 @@ func WithListenServiceID(id string) ListenOption {
 	}
 }
 
+// remoteListenConfig accumulates options for the RemoteListen method.
+type remoteListenConfig struct {
+	bindAddress string
+	serviceID   string
+}
+
+// RemoteListenOption configures a reverse listener opened via [TCPInterface.RemoteListen].
+type RemoteListenOption func(*remoteListenConfig)
+
+// WithRemoteBindAddress overrides the default sandbox-side bind address ("127.0.0.1").
+// Pass "0.0.0.0" to accept connections from any interface within the sandbox.
+func WithRemoteBindAddress(addr string) RemoteListenOption {
+	return func(c *remoteListenConfig) {
+		c.bindAddress = addr
+	}
+}
+
+// WithRemoteListenServiceID sets an optional service identifier on the reverse
+// listener for audit and correlation purposes.
+func WithRemoteListenServiceID(id string) RemoteListenOption {
+	return func(c *remoteListenConfig) {
+		c.serviceID = id
+	}
+}
+
 // TCPInterface defines operations for TCP port forwarding to sandboxes.
 // Methods accept a sandbox name and resolve it to an ID internally.
 type TCPInterface interface {
 	Forward(ctx context.Context, workspace, sandboxName string, port uint32, opts ...ForwardOption) (io.ReadWriteCloser, error)
 	Listen(ctx context.Context, workspace, sandboxName string, remotePort uint32, localPort uint32, opts ...ListenOption) (net.Listener, error)
+	RemoteListen(ctx context.Context, workspace, sandboxName string, remotePort uint32, localTarget string, opts ...RemoteListenOption) error
 }

openshell/v1/tcp_client.go

@@ -85,6 +85,26 @@ func (t *tcpClient) Forward(ctx context.Context, workspace, sandboxName string,
 	return conn, nil
 }
 
+func (t *tcpClient) RemoteListen(ctx context.Context, _, sandboxName string, remotePort uint32, localTarget string, _ ...RemoteListenOption) error {
+	_ = ctx
+	if sandboxName == "" {
+		return &StatusError{Code: ErrorInvalidArgument, Message: "sandbox name must not be empty"}
+	}
+	if remotePort == 0 || remotePort > 65535 {
+		return &StatusError{
+			Code:    ErrorInvalidArgument,
+			Message: fmt.Sprintf("port must be in range 1-65535, got %d", remotePort),
+		}
+	}
+	if _, _, err := net.SplitHostPort(localTarget); err != nil {
+		return &StatusError{
+			Code:    ErrorInvalidArgument,
+			Message: fmt.Sprintf("invalid localTarget %q: %v", localTarget, err),
+		}
+	}
+	return &StatusError{Code: ErrorUnimplemented, Message: "RemoteListen requires upstream proto extension (not yet available)"}
+}
+
 func (t *tcpClient) Listen(ctx context.Context, workspace, sandboxName string, remotePort uint32, localPort uint32, opts ...ListenOption) (net.Listener, error) {
 	if sandboxName == "" {
 		return nil, &StatusError{Code: ErrorInvalidArgument, Message: "sandbox name must not be empty"}