diff --git a/cli/server.go b/cli/server.go index 41e5905b0f976..616d07e94af86 100644 --- a/cli/server.go +++ b/cli/server.go @@ -217,22 +217,23 @@ 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 { reason = "isn't externally reachable" } - 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.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(accessURLParsed.String()), 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 index ae70324b33741..5501ed79b5c33 100644 --- 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 printed for a host that cannot be resolved. + 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", "foobarbaz.mydomain", + "--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.Contains(t, buf.String(), "this may cause unexpected problems when creating workspaces") + require.Contains(t, buf.String(), "View the Web UI: https://foobarbaz.mydomain\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) {