Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cmd/ssh.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ func NewSSHCmd(f *flags.GlobalFlags) *cobra.Command {
sshCmd.Flags().
StringArrayVarP(&cmd.ForwardPorts, "forward-ports", "L", []string{},
"Specifies that connections to the given TCP port or Unix socket on the local (client) "+
"host are to be forwarded to the given host and port, or Unix socket, on the remote side.")
"host are to be forwarded to the given host, service name, and port, or Unix socket, on the remote side.")
sshCmd.Flags().
StringArrayVarP(&cmd.ReverseForwardPorts, "reverse-forward-ports", "R", []string{},
"Specifies that connections to the given TCP port or Unix socket on the local (client) "+
Expand Down Expand Up @@ -452,7 +452,7 @@ func (cmd *SSHCmd) forwardPorts(

errChan := make(chan error, len(cmd.ForwardPorts))
for _, portMapping := range cmd.ForwardPorts {
mapping, err := port.ParsePortSpec(portMapping)
mapping, err := parseForwardPortSpec(portMapping)
if err != nil {
return fmt.Errorf("parse port mapping: %w", err)
}
Expand Down
86 changes: 86 additions & 0 deletions cmd/ssh_forward_ports.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
package cmd

import (
"fmt"
"net"
"strconv"
"strings"

"github.com/skevetter/devpod/pkg/port"
)

func parseForwardPortSpec(raw string) (port.Mapping, error) {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cmd has historically been abused as a dumping ground. Let's move this over pkg if needed.

Also, is this logic a duplicate of

func splitParts(rawport string) (string, string, string, string, error) {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I moved the parsing into pkg/port and removed the cmd-local parser. The final shape now uses a single shared parser for both --forward-ports and --reverse-forward-ports: the listen side stays strict, while the dial side accepts hostnames and service names.

parts := strings.Split(raw, ":")

switch len(parts) {
case 1, 2:
return port.ParsePortSpec(raw)
case 3:
if !isPortNumber(parts[0]) {
return port.ParsePortSpec(raw)
}

return newForwardPortMapping("", parts[0], parts[1], parts[2])
case 4:
if parts[0] == "" {
return port.Mapping{}, fmt.Errorf("local host is empty")
}

return newForwardPortMapping(parts[0], parts[1], parts[2], parts[3])
default:
return port.Mapping{}, fmt.Errorf("unexpected port format: %s", raw)
}
}

func newForwardPortMapping(localHost, localPort, remoteHost, remotePort string) (port.Mapping, error) {

Check failure on line 35 in cmd/ssh_forward_ports.go

View workflow job for this annotation

GitHub Actions / Lint

File is not properly formatted (golines)
hostAddress, err := parseForwardLocalAddress(localHost, localPort)
if err != nil {
return port.Mapping{}, fmt.Errorf("parse host address: %w", err)
}

containerAddress, err := parseForwardRemoteAddress(remoteHost, remotePort)
if err != nil {
return port.Mapping{}, fmt.Errorf("parse container address: %w", err)
}

return port.Mapping{
Host: hostAddress,
Container: containerAddress,
}, nil
}

func parseForwardLocalAddress(host, rawPort string) (port.Address, error) {
return parseForwardTCPAddress(host, rawPort, false)
}

func parseForwardRemoteAddress(host, rawPort string) (port.Address, error) {
if host == "" {
return port.Address{}, fmt.Errorf("remote host is empty")
}

return parseForwardTCPAddress(host, rawPort, true)
}

func parseForwardTCPAddress(host, rawPort string, allowHostnames bool) (port.Address, error) {
if !isPortNumber(rawPort) {
return port.Address{}, fmt.Errorf("invalid port %s", rawPort)
}

if host == "" {
host = "localhost"
}

if !allowHostnames && host != "localhost" && net.ParseIP(host) == nil {
return port.Address{}, fmt.Errorf("not an ip address %s", host)
}

return port.Address{
Protocol: "tcp",
Address: net.JoinHostPort(host, rawPort),
}, nil
}

func isPortNumber(raw string) bool {
_, err := strconv.Atoi(raw)
return err == nil
}
65 changes: 65 additions & 0 deletions cmd/ssh_forward_ports_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
package cmd

import (
"testing"

"github.com/skevetter/devpod/pkg/port"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestParseForwardPortSpec_ServiceNameTarget(t *testing.T) {
mapping, err := parseForwardPortSpec("8080:nginx:80")
require.NoError(t, err)
assert.Equal(t, "tcp", mapping.Host.Protocol)
assert.Equal(t, "localhost:8080", mapping.Host.Address)
assert.Equal(t, "tcp", mapping.Container.Protocol)
assert.Equal(t, "nginx:80", mapping.Container.Address)
}

func TestParseForwardPortSpec_ServiceNameTargetWithLocalBindHost(t *testing.T) {
mapping, err := parseForwardPortSpec("127.0.0.1:8080:nginx:80")
require.NoError(t, err)
assert.Equal(t, "127.0.0.1:8080", mapping.Host.Address)
assert.Equal(t, "nginx:80", mapping.Container.Address)
}

func TestParseForwardPortSpec_PreservesLocalBindDisambiguation(t *testing.T) {
mapping, err := parseForwardPortSpec("localhost:8080:80")
require.NoError(t, err)
assert.Equal(t, "localhost:8080", mapping.Host.Address)
assert.Equal(t, "localhost:80", mapping.Container.Address)
}

func TestParseForwardPortSpec_AllowsRemoteIPTargets(t *testing.T) {
mapping, err := parseForwardPortSpec("8080:10.0.0.2:80")
require.NoError(t, err)
assert.Equal(t, "localhost:8080", mapping.Host.Address)
assert.Equal(t, "10.0.0.2:80", mapping.Container.Address)
}

func TestParseForwardPortSpec_RejectsNonIPLocalBindHost(t *testing.T) {
_, err := parseForwardPortSpec("app:8080:nginx:80")
require.Error(t, err)
assert.ErrorContains(t, err, "not an ip address app")
}

func TestParseForwardPortSpec_RejectsEmptyRemoteHost(t *testing.T) {
_, err := parseForwardPortSpec("8080::80")
require.Error(t, err)
assert.ErrorContains(t, err, "remote host is empty")
}

func TestParseForwardPortSpec_DelegatesUnixSocketMappings(t *testing.T) {
mapping, err := parseForwardPortSpec("/tmp/local.sock:/tmp/remote.sock")
require.NoError(t, err)
assert.Equal(t, port.Mapping{
Host: port.Address{Protocol: "unix", Address: "/tmp/local.sock"},
Container: port.Address{Protocol: "unix", Address: "/tmp/remote.sock"},
}, mapping)
}

func TestParseForwardPortSpec_DoesNotChangeSharedParser(t *testing.T) {
_, err := port.ParsePortSpec("8080:nginx:80")
require.Error(t, err)
}
71 changes: 71 additions & 0 deletions e2e/tests/up-docker-compose/up_docker_compose.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
"context"
"fmt"
"io"
"net"
"net/http"
"os"
"os/exec"
Expand Down Expand Up @@ -161,6 +162,76 @@
))
}, ginkgo.SpecTimeout(framework.GetTimeout()))

ginkgo.It("ssh forward ports support remote service names", func(ctx context.Context) {
_, workspace, err := tc.setupAndStartWorkspace(
ctx,
"tests/up-docker-compose/testdata/docker-compose-forward-ports",
"--debug",
)
framework.ExpectNoError(err)

ids, err := findComposeContainer(
ctx,
tc.dockerHelper,
tc.composeHelper,
workspace.UID,
"app",
)
framework.ExpectNoError(err)
gomega.Expect(ids).To(gomega.HaveLen(1), "1 compose container to be created")

listener, err := net.Listen("tcp", "127.0.0.1:0")
framework.ExpectNoError(err)
localPort := listener.Addr().(*net.TCPAddr).Port
framework.ExpectNoError(listener.Close())
Comment on lines +183 to +186
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Dynamic port allocation has a minor TOCTOU window, but acceptable for e2e.

Closing the listener and reusing the port has a brief race where another process could grab it before devpod ssh binds. In practice this is fine for e2e and matches common Go test patterns, so no change required — just flagging it.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@e2e/tests/up-docker-compose/up_docker_compose.go` around lines 183 - 186, The
current pattern uses net.Listen to get a free port then closes the listener
(listener.Close()) and reuses localPort, which introduces a TOCTOU race; to fix
it eliminate the race by keeping the listener open and handing the active
listener (or its file descriptor) directly to the code that will serve SSH
rather than closing and re-binding — modify the code around net.Listen,
listener, localPort and the devpod ssh invocation so the produced listener is
passed through to the server routine (or use the listener's FD transfer) instead
of closing and re-opening the port.


done := make(chan error)
sshContext, sshCancel := context.WithCancel(context.Background())
go func() {
cmd := exec.CommandContext(

Check failure on line 191 in e2e/tests/up-docker-compose/up_docker_compose.go

View workflow job for this annotation

GitHub Actions / Lint

G204: Subprocess launched with a potential tainted input or cmd arguments (gosec)
sshContext,
filepath.Join(tc.f.DevpodBinDir, tc.f.DevpodBinName),
"ssh",
"--forward-ports",
fmt.Sprintf("%d:nginx:8080", localPort),
workspace.ID,
)

if err := cmd.Start(); err != nil {
done <- err
return
}

if err := cmd.Wait(); err != nil {
done <- err
return
}

done <- nil
}()

gomega.Eventually(func(g gomega.Gomega) {
response, err := http.Get(fmt.Sprintf("http://127.0.0.1:%d", localPort))
g.Expect(err).NotTo(gomega.HaveOccurred())
defer response.Body.Close()

Check failure on line 216 in e2e/tests/up-docker-compose/up_docker_compose.go

View workflow job for this annotation

GitHub Actions / Lint

Error return value of `response.Body.Close` is not checked (errcheck)

body, err := io.ReadAll(response.Body)
g.Expect(err).NotTo(gomega.HaveOccurred())
g.Expect(body).To(gomega.ContainSubstring("Thank you for using nginx."))
}).
WithPolling(1 * time.Second).
WithTimeout(20 * time.Second).
Should(gomega.Succeed())

sshCancel()
err = <-done

gomega.Expect(err).To(gomega.Or(
gomega.MatchError("signal: killed"),
gomega.MatchError(context.Canceled),
))
}, ginkgo.SpecTimeout(framework.GetTimeout()))

ginkgo.It("features", func(ctx context.Context) {
tempDir, workspace, err := tc.setupAndStartWorkspace(
ctx,
Expand Down
Loading