From c0055e491d8756b1dff13464e2bf4aa394e49290 Mon Sep 17 00:00:00 2001
From: Jon Ayers <jon@coder.com>
Date: Sat, 30 Jul 2022 00:46:05 +0000
Subject: [PATCH 1/3] 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.
---
cli/server.go | 86 ++++++++++++++++++++++++++++++----------------
cli/server_test.go | 76 +++++++++++++++++++++++++++++++++++++---
2 files changed, 129 insertions(+), 33 deletions(-)
diff --git a/cli/server.go b/cli/server.go
index 41e5905b0f976..3fe33d8f9666a 100644
--- 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
index ae70324b33741..b2b73c93501ab 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 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) {
From c2d5e5ac4ab377d41bd9a4f3e8609d3518f40b8c Mon Sep 17 00:00:00 2001
From: Jon Ayers <jon@coder.com>
Date: Sat, 30 Jul 2022 00:54:06 +0000
Subject: [PATCH 2/3] update a test
---
cli/server_test.go | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/cli/server_test.go b/cli/server_test.go
index b2b73c93501ab..0636e994ba006 100644
--- a/cli/server_test.go
+++ b/cli/server_test.go
@@ -148,7 +148,7 @@ func TestServer(t *testing.T) {
})
// 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.
+ // and that a warning is printed about it not being externally reachable.
t.Run("NoSchemeRemoteAccessURL", func(t *testing.T) {
t.Parallel()
ctx, cancelFunc := context.WithCancel(context.Background())
@@ -158,7 +158,7 @@ func TestServer(t *testing.T) {
"server",
"--in-memory",
"--address", ":0",
- "--access-url", "1.2.3.4:3000/",
+ "--access-url", "foobarbaz.mydomain",
"--cache-dir", t.TempDir(),
)
buf := newThreadSafeBuffer()
@@ -177,8 +177,8 @@ func TestServer(t *testing.T) {
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")
+ 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) {
From dd99850d6b570f728db4abed2b3791b40492e2b6 Mon Sep 17 00:00:00 2001
From: Jon Ayers <jon@coder.com>
Date: Sun, 31 Jul 2022 22:28:30 +0000
Subject: [PATCH 3/3] use : in scheme parsing
---
cli/server.go | 4 ++--
cli/server_test.go | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/cli/server.go b/cli/server.go
index 3fe33d8f9666a..616d07e94af86 100644
--- a/cli/server.go
+++ b/cli/server.go
@@ -229,7 +229,7 @@ func server() *cobra.Command {
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"))
}
@@ -678,7 +678,7 @@ func server() *cobra.Command {
// 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")
+ hasScheme = strings.HasPrefix(u, "http:") || strings.HasPrefix(u, "https:")
)
if !hasScheme {
diff --git a/cli/server_test.go b/cli/server_test.go
index 0636e994ba006..5501ed79b5c33 100644
--- a/cli/server_test.go
+++ b/cli/server_test.go
@@ -148,7 +148,7 @@ func TestServer(t *testing.T) {
})
// Validate that an https scheme is prepended to a remote access URL
- // and that a warning is printed about it not being externally reachable.
+ // 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())