From 8b17bf98eacd6a25d6ff6931f11a2405b72a6f60 Mon Sep 17 00:00:00 2001 From: Jon Ayers Date: Sun, 31 Jul 2022 17:49:25 -0500 Subject: [PATCH] fix: prepend scheme to access url (#3317) - 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. --- cli/server.go | 88 ++++++++++++++++++++++++++++++---------------- cli/server_test.go | 76 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 130 insertions(+), 34 deletions(-) diff --git a/cli/server.go b/cli/server.go index 41e5905b0f..616d07e94a 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 ae70324b33..5501ed79b5 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") + }) - assert.NotContains(t, buf.String(), "Workspaces must be able to reach Coder from this URL") + // 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() + + 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) {