Reject an invalid host in auth login and auth token

.nextchanges/cli/login-reject-empty-host.md

@@ -0,0 +1 @@
+* `databricks auth login` and `databricks auth token` now reject a host that is not a URL with a hostname, such as `/`, `?o=123` or `https://`, instead of falling back to `DATABRICKS_HOST` or failing later. An empty host after "Enter a host URL manually" in the login profile picker no longer starts discovery login. ([#7195](https://github.com/databricks/cli/pull/7195))

acceptance/cmd/auth/login/invalid-host/out.test.toml

@@ -0,0 +1 @@
+Cloud = false

acceptance/cmd/auth/login/invalid-host/output.txt

@@ -0,0 +1,6 @@
+
+>>> musterr [CLI] auth login --host ?o=123 --profile test
+Error: invalid host "?o=123". Expected a URL like https://<databricks-instance>.cloud.databricks.com
+
+>>> musterr [CLI] auth login --host https:// --profile test
+Error: invalid host "https://". Expected a URL like https://<databricks-instance>.cloud.databricks.com

acceptance/cmd/auth/login/invalid-host/script

@@ -0,0 +1,9 @@
+sethome "./home"
+
+# A host that is empty without its query params is rejected before login
+# can fall back to discovery or DATABRICKS_HOST.
+trace musterr $CLI auth login --host '?o=123' --profile test
+
+# A host without a hostname is rejected instead of being read as a host
+# named "https".
+trace musterr $CLI auth login --host 'https://' --profile test

acceptance/cmd/auth/login/invalid-host/test.toml

@@ -0,0 +1,3 @@
+Ignore = [
+    "home"
+]

acceptance/cmd/auth/token/invalid-host/out.test.toml

@@ -0,0 +1 @@
+Cloud = false

acceptance/cmd/auth/token/invalid-host/output.txt

@@ -0,0 +1,6 @@
+
+>>> musterr [CLI] auth token --host ?o=123
+Error: invalid host "?o=123". Expected a URL like https://<databricks-instance>.cloud.databricks.com
+
+>>> musterr [CLI] auth token --host https://
+Error: invalid host "https://". Expected a URL like https://<databricks-instance>.cloud.databricks.com

acceptance/cmd/auth/token/invalid-host/script

@@ -0,0 +1,13 @@
+sethome "./home"
+
+unset DATABRICKS_HOST
+unset DATABRICKS_TOKEN
+unset DATABRICKS_CONFIG_PROFILE
+
+# A host that is empty without its query params is rejected instead of
+# falling back to DATABRICKS_HOST.
+trace musterr $CLI auth token --host '?o=123'
+
+# A host without a hostname is rejected instead of being read as a host
+# named "https".
+trace musterr $CLI auth token --host 'https://'

acceptance/cmd/auth/token/invalid-host/test.toml

@@ -0,0 +1,3 @@
+Ignore = [
+    "home"
+]

cmd/auth/auth.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strings"
 
 	"github.com/databricks/cli/libs/auth"
 	"github.com/databricks/cli/libs/cmdio"
@@ -50,10 +51,20 @@ func promptForHost(ctx context.Context) (string, error) {
 	// promptui's screenbuf does not account for terminal line wrapping, and a
 	// long "label + value" line that wraps causes each keystroke to leave a
 	// stale render on screen instead of overwriting the previous one.
-	cmdio.LogString(ctx, "Example: https://<databricks-instance>.cloud.databricks.com")
-	return cmdio.RunPrompt(ctx, cmdio.PromptOptions{
+	cmdio.LogString(ctx, "Example: "+exampleHost)
+	result, err := cmdio.RunPrompt(ctx, cmdio.PromptOptions{
 		Label: "Databricks host",
+		Validate: func(input string) error {
+			if strings.TrimSpace(input) == "" {
+				return errors.New("host is required")
+			}
+			// Reject hosts that setHost would reject after the prompt,
+			// e.g. "/" or "?o=123", while the user can still edit them.
+			_, err := parseLoginHost(input)
+			return err
+		},
 	})
+	return strings.TrimSpace(result), err
 }
 
 func promptForAccountID(ctx context.Context) (string, error) {

cmd/auth/auth_test.go

@@ -3,10 +3,12 @@ package auth
 import (
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/databricks/cli/cmd/root"
 	"github.com/databricks/cli/libs/cmdctx"
+	"github.com/databricks/cli/libs/cmdio/cmdiotest/termtest"
 	"github.com/databricks/cli/libs/databrickscfg/profile"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -141,3 +143,27 @@ func TestProfileHostCompatibleViaCobra(t *testing.T) {
 		assert.NotContains(t, err.Error(), "conflicts with --host")
 	}
 }
+
+func TestPromptForHostRejectsEmptyHost(t *testing.T) {
+	tm := termtest.Run(t, promptForHost)
+	tm.WaitFor("Databricks host")
+	tm.Type(termtest.KeyEnter)
+	tm.Golden("01-empty")
+	tm.Type("  " + termtest.KeyEnter)
+	tm.Golden("02-spaces")
+	tm.Type("https://workspace.test " + termtest.KeyEnter)
+	host, err := tm.Result()
+	require.NoError(t, err)
+	assert.Equal(t, "https://workspace.test", host)
+}
+
+func TestPromptForHostRejectsHostEmptyAfterNormalization(t *testing.T) {
+	tm := termtest.Run(t, promptForHost)
+	tm.WaitFor("Databricks host")
+	tm.Type("?o=123" + termtest.KeyEnter)
+	tm.Golden("01-query-params")
+	tm.Type(strings.Repeat(termtest.KeyBackspace, len("?o=123")) + "https://workspace.test/?o=123" + termtest.KeyEnter)
+	host, err := tm.Result()
+	require.NoError(t, err)
+	assert.Equal(t, "https://workspace.test/?o=123", host)
+}

cmd/auth/login.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"net/url"
 	"runtime"
 	"slices"
 	"strconv"
@@ -246,6 +247,17 @@ a new profile is created.
 			}
 		}
 
+		// setHost normalizes the host only after the keyring probe and the
+		// profile name prompt, so reject an invalid host before them. Only trim
+		// it here: an account ID from the query params would make the prompt
+		// suggest ACCOUNT-<id> for a workspace login.
+		if authArguments.Host != "" {
+			if err := normalizeLoginHost(&auth.AuthArguments{Host: authArguments.Host}); err != nil {
+				return err
+			}
+			authArguments.Host = strings.TrimSpace(authArguments.Host)
+		}
+
 		if err := validateDeviceCodeCompletionFlags(cmd); err != nil {
 			return err
 		}
@@ -315,11 +327,6 @@ a new profile is created.
 				if err != nil {
 					return err
 				}
-				// Fail here rather than prompting for the host again after
-				// the profile name.
-				if useDeviceCode && host == "" {
-					return errDeviceCodeNeedsHost
-				}
 				authArguments.Host = host
 			case profilePickerCreateNew:
 				// Reject the names of existing profiles, including those the
@@ -339,13 +346,8 @@ a new profile is created.
 		// only populated by setHostAndAccountId further down, which runs after
 		// the profile-name prompt.
 		if skipWorkspace && profileName == "" && authArguments.Host != "" && authArguments.AccountID == "" {
-			params := auth.ExtractHostQueryParams(authArguments.Host)
-			authArguments.Host = params.Host
-			if authArguments.AccountID == "" {
-				authArguments.AccountID = params.AccountID
-			}
-			if authArguments.WorkspaceID == "" {
-				authArguments.WorkspaceID = params.WorkspaceID
+			if err := normalizeLoginHost(authArguments); err != nil {
+				return err
 			}
 			runHostDiscovery(ctx, authArguments)
 		}
@@ -417,15 +419,8 @@ a new profile is created.
 		// Check the host once it is resolved, before setAccountID prompts for
 		// an account ID for account and unified hosts. A workspace without
 		// device code login enabled is rejected later by deviceCodeLogin.
-		if useDeviceCode {
-			// The host prompt accepts an empty answer. Without this check,
-			// login would resolve the host from DATABRICKS_HOST instead.
-			if authArguments.Host == "" {
-				return errDeviceCodeNeedsHost
-			}
-			if !isDeviceCodeSupportedForHost(authArguments.Host, authArguments.DiscoveryURL) {
-				return errDeviceCodeWorkspaceOnly
-			}
+		if useDeviceCode && !isDeviceCodeSupportedForHost(authArguments.Host, authArguments.DiscoveryURL) {
+			return errDeviceCodeWorkspaceOnly
 		}
 		err = setAccountID(ctx, existingProfile, authArguments)
 		if err != nil {
@@ -629,6 +624,62 @@ func setHostAndAccountId(ctx context.Context, existingProfile *profile.Profile,
 	return setAccountID(ctx, existingProfile, authArguments)
 }
 
+var errInvalidHost = errors.New("invalid host")
+
+const exampleHost = "https://<databricks-instance>.cloud.databricks.com"
+
+// normalizeLoginHost strips the trailing slash and the query params from the
+// host and moves the workspace and account ID query params into WorkspaceID and
+// AccountID unless they are set.
+// It fails if the host is not a URL with a hostname, e.g. for "/", "?o=123",
+// "https://" or "https://my workspace.test", because login would then use discovery
+// or DATABRICKS_HOST, connect to a host named "https", or fail later.
+func normalizeLoginHost(authArguments *auth.AuthArguments) error {
+	params, err := parseLoginHost(authArguments.Host)
+	if err != nil {
+		// The host may come from a profile or DATABRICKS_HOST, so say what
+		// shape is expected.
+		return fmt.Errorf("%w. Expected a URL like %s", err, exampleHost)
+	}
+	authArguments.Host = params.Host
+	if authArguments.WorkspaceID == "" {
+		authArguments.WorkspaceID = params.WorkspaceID
+	}
+	if authArguments.AccountID == "" {
+		authArguments.AccountID = params.AccountID
+	}
+	return nil
+}
+
+// parseLoginHost trims host and extracts its query params. It returns
+// errInvalidHost if host is not a URL with a hostname.
+func parseLoginHost(host string) (auth.HostParams, error) {
+	// Extract query parameters from the host URL (?o=workspace_id, ?a=account_id).
+	// URL params from explicit --host override stale profile values.
+	params := auth.ExtractHostQueryParams(strings.TrimSuffix(strings.TrimSpace(host), "/"))
+	if !hasHostname(params.Host) {
+		return auth.HostParams{}, fmt.Errorf("%w %q", errInvalidHost, host)
+	}
+	return params, nil
+}
+
+// hasHostname reports whether host, with or without a scheme, is an http or
+// https URL with a hostname.
+func hasHostname(host string) bool {
+	u, err := url.Parse(host)
+	// url.Parse can't read a host without a scheme, e.g. "localhost:8080", so
+	// check it as an https host. A host with "//" has a scheme, possibly a
+	// malformed one like in "https//x.test", so it is checked as is.
+	if (err != nil || !isHTTPScheme(u.Scheme)) && !strings.Contains(host, "//") {
+		u, err = url.Parse("https://" + host)
+	}
+	return err == nil && isHTTPScheme(u.Scheme) && u.Hostname() != ""
+}
+
+func isHTTPScheme(scheme string) bool {
+	return scheme == "http" || scheme == "https"
+}
+
 // Sets the host in the persistentAuth object based on the provided arguments and flags.
 // Follows the following precedence:
 // 1. [HOST] (first positional argument) or --host flag. Error if both are specified.
@@ -660,17 +711,8 @@ func setHost(ctx context.Context, existingProfile *profile.Profile, authArgument
 		}
 	}
 
-	authArguments.Host = strings.TrimSuffix(authArguments.Host, "/")
-
-	// Extract query parameters from the host URL (?o=workspace_id, ?a=account_id).
-	// URL params from explicit --host override stale profile values.
-	params := auth.ExtractHostQueryParams(authArguments.Host)
-	authArguments.Host = params.Host
-	if authArguments.WorkspaceID == "" {
-		authArguments.WorkspaceID = params.WorkspaceID
-	}
-	if authArguments.AccountID == "" {
-		authArguments.AccountID = params.AccountID
+	if err := normalizeLoginHost(authArguments); err != nil {
+		return err
 	}
 
 	// Inherit workspace_id from the existing profile AFTER URL param extraction.

cmd/auth/login_device_code_test.go

@@ -307,8 +307,10 @@ func TestLoginDeviceCodeNewProfilePromptsForHost(t *testing.T) {
 	tm := startDeviceCodeLogin(t, "", "--profile", "newprof")
 	tm.WaitFor("Databricks host")
 	tm.Type(termtest.KeyEnter)
+	tm.WaitFor(">> host is required")
+	tm.Type(termtest.KeyCtrlC)
 	_, err := tm.Result()
-	assert.ErrorIs(t, err, errDeviceCodeNeedsHost)
+	assert.EqualError(t, err, "^C")
 }
 
 func TestLoginDeviceCodePickerEmptyHost(t *testing.T) {
@@ -323,8 +325,10 @@ func TestLoginDeviceCodePickerEmptyHost(t *testing.T) {
 	tm.Type(termtest.KeyDown + termtest.KeyEnter)
 	tm.WaitFor("Databricks host")
 	tm.Type(termtest.KeyEnter)
+	tm.WaitFor(">> host is required")
+	tm.Type(termtest.KeyCtrlC)
 	_, err := tm.Result()
-	assert.ErrorIs(t, err, errDeviceCodeNeedsHost)
+	assert.EqualError(t, err, "^C")
 }
 
 func TestLoginDeviceCodeCreateNewProfileRejectsHiddenProfileName(t *testing.T) {
@@ -350,6 +354,23 @@ account_id = acct-123
 	assert.EqualError(t, err, "^C")
 }
 
+func TestLoginDeviceCodeHostEmptyAfterNormalization(t *testing.T) {
+	tests := []struct {
+		name string
+		host string
+	}{
+		{name: "slash", host: "/"},
+		{name: "query params only", host: "?o=123"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			tm := startDeviceCodeLogin(t, "", "--profile", "newprof", "--host", tt.host)
+			_, err := tm.Result()
+			assert.ErrorIs(t, err, errInvalidHost)
+		})
+	}
+}
+
 func TestMatchDeviceCodeProfiles(t *testing.T) {
 	tests := []struct {
 		name    string