From 8be3d0549eb3f596c45a1084ed435d1267af0657 Mon Sep 17 00:00:00 2001
From: "Philip K. Warren" <pkwarren@users.noreply.github.com>
Date: Wed, 24 Jul 2024 11:09:44 -0500
Subject: [PATCH] Validate domain in buf registry login and logout (#3182)

We don't do explicit validation of the domain in 'buf registry login',
which leads to some confusing error messages. Update the command to
validate the domain with netext.ValidateHostname and update
ValidateHostname to not accept URLs (it previously didn't look up the
port number so it didn't perform proper validation).
---
 .../buf/command/registry/registrylogin/registrylogin.go  | 4 ++++
 .../command/registry/registrylogout/registrylogout.go    | 4 ++++
 private/pkg/netext/netext.go                             | 9 +++++++--
 private/pkg/netext/netext_test.go                        | 5 +++++
 4 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/private/buf/cmd/buf/command/registry/registrylogin/registrylogin.go b/private/buf/cmd/buf/command/registry/registrylogin/registrylogin.go
index 9d10a67241..0f4798ea3b 100644
--- a/private/buf/cmd/buf/command/registry/registrylogin/registrylogin.go
+++ b/private/buf/cmd/buf/command/registry/registrylogin/registrylogin.go
@@ -29,6 +29,7 @@ import (
 	"github.com/bufbuild/buf/private/pkg/app/appcmd"
 	"github.com/bufbuild/buf/private/pkg/app/appext"
 	"github.com/bufbuild/buf/private/pkg/connectclient"
+	"github.com/bufbuild/buf/private/pkg/netext"
 	"github.com/bufbuild/buf/private/pkg/netrc"
 	"github.com/spf13/pflag"
 )
@@ -135,6 +136,9 @@ func inner(
 	remote := bufconnect.DefaultRemote
 	if container.NumArgs() == 1 {
 		remote = container.Arg(0)
+		if _, err := netext.ValidateHostname(remote); err != nil {
+			return err
+		}
 	}
 	// Do not print unless we are prompting
 	if !flags.TokenStdin {
diff --git a/private/buf/cmd/buf/command/registry/registrylogout/registrylogout.go b/private/buf/cmd/buf/command/registry/registrylogout/registrylogout.go
index a2e076cd41..8b68bd564a 100644
--- a/private/buf/cmd/buf/command/registry/registrylogout/registrylogout.go
+++ b/private/buf/cmd/buf/command/registry/registrylogout/registrylogout.go
@@ -21,6 +21,7 @@ import (
 	"github.com/bufbuild/buf/private/bufpkg/bufconnect"
 	"github.com/bufbuild/buf/private/pkg/app/appcmd"
 	"github.com/bufbuild/buf/private/pkg/app/appext"
+	"github.com/bufbuild/buf/private/pkg/netext"
 	"github.com/bufbuild/buf/private/pkg/netrc"
 	"github.com/spf13/pflag"
 )
@@ -63,6 +64,9 @@ func run(
 	remote := bufconnect.DefaultRemote
 	if container.NumArgs() == 1 {
 		remote = container.Arg(0)
+		if _, err := netext.ValidateHostname(remote); err != nil {
+			return err
+		}
 	}
 	modified1, err := netrc.DeleteMachineForName(container, remote)
 	if err != nil {
diff --git a/private/pkg/netext/netext.go b/private/pkg/netext/netext.go
index b6727b3e17..734de0cbd8 100644
--- a/private/pkg/netext/netext.go
+++ b/private/pkg/netext/netext.go
@@ -18,6 +18,7 @@ import (
 	"errors"
 	"fmt"
 	"net"
+	"strconv"
 )
 
 const (
@@ -37,8 +38,12 @@ func ValidateHostname(hostname string) (string, error) {
 	}
 
 	parsedHost := hostname
-	if host, _, err := net.SplitHostPort(hostname); err == nil {
-		parsedHost = host
+	if host, port, err := net.SplitHostPort(hostname); err == nil {
+		// net.SplitHostPort performs very lax validation of ports (allowing to be resolved from /etc/services).
+		// Only accept numeric ports here.
+		if portNum, err := strconv.ParseUint(port, 10, 32); err == nil && portNum <= 65535 {
+			parsedHost = host
+		}
 	}
 	if net.ParseIP(parsedHost) != nil {
 		// hostname is a valid IP address
diff --git a/private/pkg/netext/netext_test.go b/private/pkg/netext/netext_test.go
index 0c42388349..44dce6f8e1 100644
--- a/private/pkg/netext/netext_test.go
+++ b/private/pkg/netext/netext_test.go
@@ -97,6 +97,11 @@ func TestValidateHostname(t *testing.T) {
 			hostname:    "",
 			isValid:     false,
 		},
+		{
+			description: "urls are invalid",
+			hostname:    "https://buf.build",
+			isValid:     false,
+		},
 	}
 
 	for _, test := range tests {