Skip to content

fix: prepend scheme to access url #3317

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 31, 2022
Merged

fix: prepend scheme to access url #3317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c0055e4
fix: prepend scheme to access url
sreya Jul 30, 2022
c2d5e5a
update a test
sreya Jul 30, 2022
dd99850
use : in scheme parsing
sreya Jul 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 58 additions & 30 deletions cli/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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):")
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down
76 changes: 72 additions & 4 deletions cli/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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()
Expand All @@ -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()
Expand All @@ -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) {
Expand Down