fix: prepend scheme to access url

- Problems can arise spawning workspaces if a schemeless URL is passed as the access URL. If an access url is detected to not have an "http" or "https" scheme then it is prepended with "https". If the hostname is detected to be a loopback device then "http" is preferred.
coder · sreya · Jul 31, 2022 · Jul 30, 2022 · Jul 30, 2022 · Jul 31, 2022
commit c0055e491d8756b1dff13464e2bf4aa394e49290
diff --git a/cli/server.go b/cli/server.go
@@ -217,8 +217,13 @@ func server() *cobra.Command {
 				accessURL = devTunnel.URL
 			}
 
+			accessURLParsed, err := parseURL(ctx, accessURL)
+			if err != nil {
+				return xerrors.Errorf("parse URL: %w", err)
+			}
+
 			// Warn the user if the access URL appears to be a loopback address.
-			isLocal, err := isLocalURL(ctx, accessURL)
+			isLocal, err := isLocalURL(ctx, accessURLParsed)
 			if isLocal || err != nil {
 				reason := "could not be resolved"
 				if isLocal {
@@ -227,12 +232,8 @@ func server() *cobra.Command {
 				cmd.Printf("%s The access URL %s %s, this may cause unexpected problems when creating workspaces. Generate a unique *.try.coder.app URL with:\n", cliui.Styles.Warn.Render("Warning:"), cliui.Styles.Field.Render(accessURL), reason)
 				cmd.Println(cliui.Styles.Code.Render(strings.Join(os.Args, " ") + " --tunnel"))
 			}
-			cmd.Printf("View the Web UI: %s\n", accessURL)
 
-			accessURLParsed, err := url.Parse(accessURL)
-			if err != nil {
-				return xerrors.Errorf("parse access url %q: %w", accessURL, err)
-			}
+			cmd.Printf("View the Web UI: %s\n", accessURLParsed.String())
 
 			// Used for zero-trust instance identity with Google Cloud.
 			googleTokenValidator, err := idtoken.NewValidator(ctx, option.WithoutAuthentication())
@@ -324,7 +325,7 @@ func server() *cobra.Command {
 			}
 
 			// Parse the raw telemetry URL!
-			telemetryURL, err := url.Parse(telemetryURL)
+			telemetryURL, err := parseURL(ctx, telemetryURL)
 			if err != nil {
 				return xerrors.Errorf("parse telemetry url: %w", err)
 			}
@@ -440,7 +441,7 @@ func server() *cobra.Command {
 			if !hasFirstUser && err == nil {
 				cmd.Println()
 				cmd.Println("Get started by creating the first user (in a new terminal):")
-				cmd.Println(cliui.Styles.Code.Render("coder login " + accessURL))
+				cmd.Println(cliui.Styles.Code.Render("coder login " + accessURLParsed.String()))
 			}
 
 			cmd.Println("\n==> Logs will stream in below (press ctrl+c to gracefully exit):")
@@ -672,6 +673,54 @@ func server() *cobra.Command {
 	return root
 }
 
+// parseURL parses a string into a URL. It works around some technically correct
+// but undesired behavior of url.Parse by prepending a scheme if one does not
+// exist so that the URL does not get parsed improprely.
+func parseURL(ctx context.Context, u string) (*url.URL, error) {
+	var (
+		hasScheme = strings.HasPrefix(u, "http") || strings.HasPrefix(u, "https")
+	)
+
+	if !hasScheme {
+		// Append a scheme if it doesn't have one. Otherwise the hostname
+		// will likely get parsed as the scheme and cause methods like Hostname()
+		// to return an empty string, largely obviating the purpose of this
+		// function.
+		u = "https://" + u
+	}
+
+	parsed, err := url.Parse(u)
+	if err != nil {
+		return nil, err
+	}
+
+	// If the specified url is a loopback device and no scheme has been
+	// specified, prefer http over https. It's unlikely anyone intends to use
+	// https on a loopback and if they do they can specify a scheme.
+	if local, _ := isLocalURL(ctx, parsed); local && !hasScheme {
+		parsed.Scheme = "http"
+	}
+
+	return parsed, nil
+}
+
+// isLocalURL returns true if the hostname of the provided URL appears to
+// resolve to a loopback address.
+func isLocalURL(ctx context.Context, u *url.URL) (bool, error) {
+	resolver := &net.Resolver{}
+	ips, err := resolver.LookupIPAddr(ctx, u.Hostname())
+	if err != nil {
+		return false, err
+	}
+
+	for _, ip := range ips {
+		if ip.IP.IsLoopback() {
+			return true, nil
+		}
+	}
+	return false, nil
+}
+
 func shutdownWithTimeout(s interface{ Shutdown(context.Context) error }, timeout time.Duration) error {
 	ctx, cancel := context.WithTimeout(context.Background(), timeout)
 	defer cancel()
@@ -922,27 +971,6 @@ func serveHandler(ctx context.Context, logger slog.Logger, handler http.Handler,
 	return func() { _ = srv.Close() }
 }
 
-// isLocalURL returns true if the hostname of the provided URL appears to
-// resolve to a loopback address.
-func isLocalURL(ctx context.Context, urlString string) (bool, error) {
-	parsedURL, err := url.Parse(urlString)
-	if err != nil {
-		return false, err
-	}
-	resolver := &net.Resolver{}
-	ips, err := resolver.LookupIPAddr(ctx, parsedURL.Hostname())
-	if err != nil {
-		return false, err
-	}
-
-	for _, ip := range ips {
-		if ip.IP.IsLoopback() {
-			return true, nil
-		}
-	}
-	return false, nil
-}
-
 // embeddedPostgresURL returns the URL for the embedded PostgreSQL deployment.
 func embeddedPostgresURL(cfg config.Root) (string, error) {
 	pgPassword, err := cfg.PostgresPassword().Read()

diff --git a/cli/server_test.go b/cli/server_test.go
@@ -21,7 +21,6 @@ import (
 	"time"
 
 	"github.com/go-chi/chi"
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"go.uber.org/goleak"
 
@@ -113,7 +112,10 @@ func TestServer(t *testing.T) {
 		pty.ExpectMatch("psql")
 	})
 
-	t.Run("NoWarningWithRemoteAccessURL", func(t *testing.T) {
+	// Validate that an http scheme is prepended to a loopback
+	// access URL and that a warning is printed that it may not be externally
+	// reachable.
+	t.Run("NoSchemeLocalAccessURL", func(t *testing.T) {
 		t.Parallel()
 		ctx, cancelFunc := context.WithCancel(context.Background())
 		defer cancelFunc()
@@ -122,7 +124,7 @@ func TestServer(t *testing.T) {
 			"server",
 			"--in-memory",
 			"--address", ":0",
-			"--access-url", "http://1.2.3.4:3000/",
+			"--access-url", "localhost:3000/",
 			"--cache-dir", t.TempDir(),
 		)
 		buf := newThreadSafeBuffer()
@@ -141,8 +143,74 @@ func TestServer(t *testing.T) {
 
 		cancelFunc()
 		require.ErrorIs(t, <-errC, context.Canceled)
+		require.Contains(t, buf.String(), "this may cause unexpected problems when creating workspaces")
+		require.Contains(t, buf.String(), "View the Web UI: http://localhost:3000/\n")
+	})
+
+	// Validate that an https scheme is prepended to a remote access URL
+	// and that a warning is not printed about it not being externally reachable.
+	t.Run("NoSchemeRemoteAccessURL", func(t *testing.T) {
+		t.Parallel()
+		ctx, cancelFunc := context.WithCancel(context.Background())
+		defer cancelFunc()
 
-		assert.NotContains(t, buf.String(), "Workspaces must be able to reach Coder from this URL")
+		root, cfg := clitest.New(t,
+			"server",
+			"--in-memory",
+			"--address", ":0",
+			"--access-url", "1.2.3.4:3000/",
+			"--cache-dir", t.TempDir(),
+		)
+		buf := newThreadSafeBuffer()
+		root.SetOutput(buf)
+		errC := make(chan error, 1)
+		go func() {
+			errC <- root.ExecuteContext(ctx)
+		}()
+
+		// Just wait for startup
+		require.Eventually(t, func() bool {
+			var err error
+			_, err = cfg.URL().Read()
+			return err == nil
+		}, 15*time.Second, 25*time.Millisecond)
+
+		cancelFunc()
+		require.ErrorIs(t, <-errC, context.Canceled)
+		require.NotContains(t, buf.String(), "this may cause unexpected problems when creating workspaces")
+		require.Contains(t, buf.String(), "View the Web UI: https://1.2.3.4:3000/\n")
+	})
+
+	t.Run("NoWarningWithRemoteAccessURL", func(t *testing.T) {
+		t.Parallel()
+		ctx, cancelFunc := context.WithCancel(context.Background())
+		defer cancelFunc()
+
+		root, cfg := clitest.New(t,
+			"server",
+			"--in-memory",
+			"--address", ":0",
+			"--access-url", "https://google.com",
+			"--cache-dir", t.TempDir(),
+		)
+		buf := newThreadSafeBuffer()
+		root.SetOutput(buf)
+		errC := make(chan error, 1)
+		go func() {
+			errC <- root.ExecuteContext(ctx)
+		}()
+
+		// Just wait for startup
+		require.Eventually(t, func() bool {
+			var err error
+			_, err = cfg.URL().Read()
+			return err == nil
+		}, 15*time.Second, 25*time.Millisecond)
+
+		cancelFunc()
+		require.ErrorIs(t, <-errC, context.Canceled)
+		require.NotContains(t, buf.String(), "this may cause unexpected problems when creating workspaces")
+		require.Contains(t, buf.String(), "View the Web UI: https://google.com\n")
 	})
 
 	t.Run("TLSBadVersion", func(t *testing.T) {