From 6c20e97fc5074fb07081e8391f7c367c61c122b8 Mon Sep 17 00:00:00 2001 From: chas Date: Tue, 11 Aug 2026 20:20:52 -0400 Subject: [PATCH 01/10] [CFX-7263] fix(auth): fail on unverifiable env credentials instead of silently using the stored profile Why: - EnsureAuthenticated verified DATAROBOT_ENDPOINT/DATAROBOT_API_TOKEN but on failure fell back to the stored profile with no output when the stored token was valid. a script passing a project's .env creds got the caller's own instance with exit 0, so dr llm-gateway list returned the wrong catalog. - dr auth check and dr auth export already treat a complete env pair as authoritative. EnsureAuthenticated was the one path that substituted silently. Changes: - a complete env pair that fails verification now fails the command before the stored-profile check. classified reason (timeout, malformed endpoint, invalid token) goes to stderr plus a line naming both the requested endpoint and the stored profile that was not used. - extracted the classification into ReportEnvCredentialsError, shared with dr auth check (drops its duplicated block). - partial or absent env credentials keep the existing fallback and login flow. - regression tests: env-invalid with stored-valid must return false and must not start the login flow; malformed quoted endpoint variant; classifier and refusal-message unit tests. --- cmd/auth/check/cmd.go | 26 +-------- internal/auth/auth.go | 106 +++++++++++++++++++++++++++++-------- internal/auth/auth_test.go | 98 ++++++++++++++++++++++++++++++++++ 3 files changed, 182 insertions(+), 48 deletions(-) diff --git a/cmd/auth/check/cmd.go b/cmd/auth/check/cmd.go index 99ca6f273..232bbdc2a 100644 --- a/cmd/auth/check/cmd.go +++ b/cmd/auth/check/cmd.go @@ -45,31 +45,7 @@ func checkCLICredentials(w io.Writer) bool { // If env vars were set but invalid, report the error if !errors.Is(err, auth.ErrEnvCredentialsNotSet) { - if errors.Is(err, context.DeadlineExceeded) { - envDatarobotHost, _ := config.SchemeHostOnly(creds.Endpoint) - - fmt.Fprint(w, tui.BaseTextStyle.Render("❌ Connection to ")) - fmt.Fprint(w, tui.InfoStyle.Render(envDatarobotHost)) - fmt.Fprintln(w, tui.BaseTextStyle.Render(" timed out. Check your network and try again.")) - - return false - } - - // Distinguish a malformed DATAROBOT_ENDPOINT from an invalid token so - // the user fixes the right thing. A quoted endpoint (e.g. from running - // `$(dr auth export)` instead of `eval "$(dr auth export)"`) fails here - // and must not be reported as a token problem. - if epErr := auth.ValidateEndpoint(creds.Endpoint); epErr != nil { - fmt.Fprintln(w, tui.BaseTextStyle.Render("❌ DATAROBOT_ENDPOINT environment variable is invalid: "+epErr.Error())) - fmt.Fprintln(w, tui.BaseTextStyle.Render("Set it to a valid DataRobot URL and try again.")) - } else { - fmt.Fprintln(w, tui.BaseTextStyle.Render("❌ DATAROBOT_API_TOKEN environment variable is invalid or expired.")) - fmt.Fprintln(w, tui.BaseTextStyle.Render("Unset it and try again:")) - fmt.Fprint(w, tui.InfoStyle.Render(" unset DATAROBOT_API_TOKEN")) - fmt.Fprint(w, tui.BaseTextStyle.Render(" (or ")) - fmt.Fprint(w, tui.InfoStyle.Render("Remove-Item Env:\\DATAROBOT_API_TOKEN")) - fmt.Fprintln(w, tui.BaseTextStyle.Render(" on Windows)")) - } + auth.ReportEnvCredentialsError(w, creds, err) return false } diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 7fc15738c..f816a5ace 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -18,6 +18,7 @@ import ( "context" "errors" "fmt" + "io" "os" "strings" "testing" @@ -45,10 +46,16 @@ var ErrEnvCredentialsNotSet = errors.New("environment credentials not set") // PrintUnsetTokenInstructions prints platform-specific instructions for unsetting DATAROBOT_API_TOKEN. func PrintUnsetTokenInstructions() { - fmt.Print(tui.InfoStyle.Render(" unset DATAROBOT_API_TOKEN")) - fmt.Print(tui.BaseTextStyle.Render(" (or ")) - fmt.Print(tui.InfoStyle.Render("Remove-Item Env:\\DATAROBOT_API_TOKEN")) - fmt.Println(tui.BaseTextStyle.Render(" on Windows)")) + FprintUnsetTokenInstructions(os.Stdout) +} + +// FprintUnsetTokenInstructions writes platform-specific instructions for +// unsetting DATAROBOT_API_TOKEN to w. +func FprintUnsetTokenInstructions(w io.Writer) { + fmt.Fprint(w, tui.InfoStyle.Render(" unset DATAROBOT_API_TOKEN")) + fmt.Fprint(w, tui.BaseTextStyle.Render(" (or ")) + fmt.Fprint(w, tui.InfoStyle.Render("Remove-Item Env:\\DATAROBOT_API_TOKEN")) + fmt.Fprintln(w, tui.BaseTextStyle.Render(" on Windows)")) } // EnvCredentials holds environment variable authentication credentials. @@ -93,6 +100,59 @@ func ValidateEndpoint(endpoint string) error { return err } +// ReportEnvCredentialsError writes a classified explanation of why an +// explicitly supplied DATAROBOT_ENDPOINT/DATAROBOT_API_TOKEN pair failed +// verification: a network timeout, a malformed endpoint, or an invalid token. +// Shared by EnsureAuthenticated (writing to stderr) and `dr auth check`. +func ReportEnvCredentialsError(w io.Writer, creds *EnvCredentials, err error) { + if errors.Is(err, context.DeadlineExceeded) { + envDatarobotHost, _ := config.SchemeHostOnly(creds.Endpoint) + + fmt.Fprint(w, tui.BaseTextStyle.Render("❌ Connection to ")) + fmt.Fprint(w, tui.InfoStyle.Render(envDatarobotHost)) + fmt.Fprintln(w, tui.BaseTextStyle.Render(" timed out. Check your network and try again.")) + + return + } + + // Distinguish a malformed DATAROBOT_ENDPOINT from an invalid token so the + // user fixes the right thing. A quoted endpoint (e.g. from running + // `$(dr auth export)` instead of `eval "$(dr auth export)"`) fails here + // and must not be reported as a token problem. + if epErr := ValidateEndpoint(creds.Endpoint); epErr != nil { + fmt.Fprintln(w, tui.BaseTextStyle.Render("❌ DATAROBOT_ENDPOINT environment variable is invalid: "+epErr.Error())) + fmt.Fprintln(w, tui.BaseTextStyle.Render("Set it to a valid DataRobot URL and try again.")) + + return + } + + fmt.Fprintln(w, tui.BaseTextStyle.Render("❌ DATAROBOT_API_TOKEN environment variable is invalid or expired.")) + fmt.Fprintln(w, tui.BaseTextStyle.Render("Unset it and try again:")) + FprintUnsetTokenInstructions(w) +} + +// reportStoredProfileNotUsed names both sides of the substitution this CLI +// refuses to make: the endpoint the environment asked for and the stored +// profile it will NOT fall back to. Printed only when a stored profile exists, +// since otherwise there is nothing to substitute. +func reportStoredProfileNotUsed(w io.Writer, creds *EnvCredentials) { + storedHost := config.GetBaseURL() + if storedHost == "" { + return + } + + requestedHost := creds.Endpoint + if host, err := config.SchemeHostOnly(creds.Endpoint); err == nil { + requestedHost = host + } + + fmt.Fprint(w, tui.BaseTextStyle.Render("Environment credentials for ")) + fmt.Fprint(w, tui.InfoStyle.Render(requestedHost)) + fmt.Fprint(w, tui.BaseTextStyle.Render(" failed to verify; not falling back to the stored profile for ")) + fmt.Fprint(w, tui.InfoStyle.Render(storedHost)) + fmt.Fprintln(w, tui.BaseTextStyle.Render(".")) +} + // VerifyEnvCredentials checks if environment variable credentials are valid. // Returns credentials and nil error if valid, credentials and error otherwise. func VerifyEnvCredentials(ctx context.Context) (*EnvCredentials, error) { @@ -140,6 +200,17 @@ func EnsureAuthenticated(ctx context.Context) bool { //nolint: cyclop return true } + // A complete pair of environment credentials is an explicit request for + // that instance (the same precedence `dr auth export` documents). Falling + // back to the stored profile here would silently swap instances, so fail + // loudly instead and never touch the profile. + if !errors.Is(envErr, ErrEnvCredentialsNotSet) { + ReportEnvCredentialsError(os.Stderr, creds, envErr) + reportStoredProfileNotUsed(os.Stderr, creds) + + return false + } + datarobotHost := GetBaseURLOrAsk() if datarobotHost == "" { // Appropriate error message was already displayed in GetBaseURLOrAsk() and SetURLAction() @@ -154,25 +225,14 @@ func EnsureAuthenticated(ctx context.Context) bool { //nolint: cyclop skipAuthFlow := false - if errors.Is(envErr, context.DeadlineExceeded) { - envDatarobotHost, _ := config.SchemeHostOnly(creds.Endpoint) - - fmt.Print(tui.BaseTextStyle.Render("❌ Connection to ")) - fmt.Print(tui.InfoStyle.Render(envDatarobotHost)) - fmt.Println(tui.BaseTextStyle.Render(" from DATAROBOT_ENDPOINT environment variable timed out.")) - fmt.Println(tui.BaseTextStyle.Render("Check your network and try again.")) - - skipAuthFlow = true - } else if creds.Token != "" { - if epErr := ValidateEndpoint(creds.Endpoint); epErr != nil { - fmt.Println(tui.BaseTextStyle.Render("Your DATAROBOT_ENDPOINT environment variable is invalid:")) - fmt.Println(tui.BaseTextStyle.Render(epErr.Error())) - fmt.Println(tui.BaseTextStyle.Render("Set it to a valid DataRobot URL and try again.")) - } else { - fmt.Println(tui.BaseTextStyle.Render("Your DATAROBOT_API_TOKEN environment variable")) - fmt.Println(tui.BaseTextStyle.Render("contains an expired or invalid token. Unset it:")) - PrintUnsetTokenInstructions() - } + if creds.Token != "" { + // Partial env: DATAROBOT_API_TOKEN is set but no endpoint accompanies + // it (a complete pair was handled above), so the token was never + // verified. Point at it rather than starting a login flow that would + // shadow it. + fmt.Println(tui.BaseTextStyle.Render("Your DATAROBOT_API_TOKEN environment variable")) + fmt.Println(tui.BaseTextStyle.Render("contains an expired or invalid token. Unset it:")) + PrintUnsetTokenInstructions() skipAuthFlow = true } diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index f1e8f4a79..9b4dd31c9 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -15,6 +15,7 @@ package auth import ( + "bytes" "context" "errors" "net/http" @@ -161,6 +162,103 @@ func TestEnsureAuthenticated_ValidEnvironmentToken(t *testing.T) { assert.Equal(t, "valid-token", apiKey, "Expected GetAPIKey after EnsureAuthenticated to return valid token") } +// TestEnsureAuthenticated_EnvInvalidStoredValid is the regression test for the +// silent-fallback bug: a complete pair of environment credentials that fails +// verification must fail the command, never silently substitute the stored +// profile (which is valid here and would have made the old code return true). +func TestEnsureAuthenticated_EnvInvalidStoredValid(t *testing.T) { + server, cleanup := setupTestEnvironment(t) + defer cleanup() + + viperx.Set(config.DataRobotAPIKey, "valid-token") + t.Setenv("DATAROBOT_ENDPOINT", server.URL+"/api/v2") + t.Setenv("DATAROBOT_API_TOKEN", "expired-token") + + // The login flow must not start either; fail the test if it does. + APIKeyCallbackFunc = func(_ context.Context, _ string) (string, error) { + t.Error("login flow must not start when explicit env credentials fail") + + return "", errors.New("unexpected login flow") + } + + result := EnsureAuthenticated(context.Background()) + assert.False(t, result, + "Expected EnsureAuthenticated to fail instead of falling back to the valid stored profile") +} + +// TestEnsureAuthenticated_EnvMalformedEndpointStoredValid covers the malformed +// endpoint variant of the same rule (e.g. a quoted URL from running +// `$(dr auth export)` without eval). +func TestEnsureAuthenticated_EnvMalformedEndpointStoredValid(t *testing.T) { + server, cleanup := setupTestEnvironment(t) + defer cleanup() + + viperx.Set(config.DataRobotAPIKey, "valid-token") + t.Setenv("DATAROBOT_ENDPOINT", `"`+server.URL+`/api/v2"`) + t.Setenv("DATAROBOT_API_TOKEN", "valid-token") + + result := EnsureAuthenticated(context.Background()) + assert.False(t, result, + "Expected EnsureAuthenticated to fail on a malformed endpoint instead of using the stored profile") +} + +func TestReportEnvCredentialsError(t *testing.T) { + t.Run("timeout", func(t *testing.T) { + var buf bytes.Buffer + + creds := &EnvCredentials{Endpoint: "https://app.example.com/api/v2", Token: "some-token"} + ReportEnvCredentialsError(&buf, creds, context.DeadlineExceeded) + + assert.Contains(t, buf.String(), "timed out") + assert.Contains(t, buf.String(), "https://app.example.com") + }) + + t.Run("malformed endpoint", func(t *testing.T) { + var buf bytes.Buffer + + creds := &EnvCredentials{Endpoint: `"https://app.example.com/api/v2"`, Token: "some-token"} + ReportEnvCredentialsError(&buf, creds, errors.New("invalid token")) + + assert.Contains(t, buf.String(), "DATAROBOT_ENDPOINT environment variable is invalid") + assert.NotContains(t, buf.String(), "DATAROBOT_API_TOKEN environment variable is invalid or expired") + }) + + t.Run("invalid token", func(t *testing.T) { + var buf bytes.Buffer + + creds := &EnvCredentials{Endpoint: "https://app.example.com/api/v2", Token: "bad-token"} + ReportEnvCredentialsError(&buf, creds, errors.New("invalid token")) + + assert.Contains(t, buf.String(), "DATAROBOT_API_TOKEN environment variable is invalid or expired") + assert.Contains(t, buf.String(), "unset DATAROBOT_API_TOKEN") + }) +} + +func TestReportStoredProfileNotUsed(t *testing.T) { + _, cleanup := setupTestEnvironment(t) + defer cleanup() + + creds := &EnvCredentials{Endpoint: "https://requested.example.com/api/v2", Token: "bad-token"} + + t.Run("names both endpoints when a stored profile exists", func(t *testing.T) { + var buf bytes.Buffer + + reportStoredProfileNotUsed(&buf, creds) + + assert.Contains(t, buf.String(), "https://requested.example.com") + assert.Contains(t, buf.String(), "not falling back to the stored profile") + }) + + t.Run("silent without a stored profile", func(t *testing.T) { + var buf bytes.Buffer + + viperx.Set(config.DataRobotURL, "") + reportStoredProfileNotUsed(&buf, creds) + + assert.Empty(t, buf.String()) + }) +} + func TestEnsureAuthenticated_SkipAuth(t *testing.T) { _, cleanup := setupTestEnvironment(t) defer cleanup() From e84d89e6427b026c5aa5649b78eb9955616e9186 Mon Sep 17 00:00:00 2001 From: chas Date: Tue, 11 Aug 2026 20:22:55 -0400 Subject: [PATCH 02/10] Document the no-fallback rule on EnsureAuthenticated --- internal/auth/auth.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index f816a5ace..319edaee2 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -167,8 +167,9 @@ func VerifyEnvCredentials(ctx context.Context) (*EnvCredentials, error) { } // EnsureAuthenticatedE checks if valid authentication exists, and if not, -// triggers the login flow automatically. Returns an error if authentication -// fails, suitable for use in Cobra PreRunE hooks. +// triggers the login flow automatically (see EnsureAuthenticated for the +// exceptions). Returns an error if authentication fails, suitable for use in +// Cobra PreRunE hooks. func EnsureAuthenticatedE(cmd *cobra.Command, _ []string) error { if !EnsureAuthenticated(cmd.Context()) { return errors.New("Authentication failed.") @@ -180,6 +181,12 @@ func EnsureAuthenticatedE(cmd *cobra.Command, _ []string) error { // EnsureAuthenticated checks if valid authentication exists, and if not, // triggers the login flow automatically. Returns true if authentication // is valid or was successfully obtained. +// +// A complete DATAROBOT_ENDPOINT/DATAROBOT_API_TOKEN pair that fails +// verification returns false without falling back to the stored profile and +// without starting the login flow: environment credentials are an explicit +// instance request, and substituting the profile would silently run against +// the wrong instance. func EnsureAuthenticated(ctx context.Context) bool { //nolint: cyclop if viperx.GetBool(config.SkipAuthKey) { log.Warn("Authentication checks are disabled via the '--skip-auth' flag. This may cause API calls to fail.") From 98caf431733764b5688af09e62e31f0891d6fa0e Mon Sep 17 00:00:00 2001 From: chas Date: Tue, 11 Aug 2026 21:03:06 -0400 Subject: [PATCH 03/10] Sharpen the env-credential failure diagnosis in ReportEnvCredentialsError Why: - a transport failure (connection refused, DNS, TLS) fell through to the "DATAROBOT_API_TOKEN is invalid or expired, unset it" message. harmful advice when the token was never judged because the instance was never reached. - failure messages always named DATAROBOT_ENDPOINT even when the endpoint came from the SDK-style DATAROBOT_API_ENDPOINT fallback var, telling the user to fix a variable that is unset. - the tui styles probe stdout for color support, so styled stderr output carried ANSI codes when stderr was redirected while stdout stayed a TTY. Changes: - classify *url.Error from VerifyToken as "Could not connect to " before the invalid-token fallthrough. a rejected token is a plain non-200 error, so the fallthrough now only fires when the instance actually answered. - EnvCredentials carries EndpointVar (which env var supplied the endpoint); the malformed-endpoint and could-not-connect messages name it. - writerStyles binds BaseTextStyle/InfoStyle to the destination writer's renderer in the three writer-taking message funcs. - tests: unreachable-endpoint classification (unit + end-to-end via a closed port, proving VerifyToken transport errors are *url.Error), DATAROBOT_API_ENDPOINT naming. --- internal/auth/auth.go | 103 ++++++++++++++++++++++++++++--------- internal/auth/auth_test.go | 53 +++++++++++++++++++ 2 files changed, 132 insertions(+), 24 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 319edaee2..472e58000 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -19,10 +19,12 @@ import ( "errors" "fmt" "io" + "net/url" "os" "strings" "testing" + "github.com/charmbracelet/lipgloss" "github.com/datarobot/cli/internal/config" "github.com/datarobot/cli/internal/config/viperx" "github.com/datarobot/cli/internal/log" @@ -52,16 +54,43 @@ func PrintUnsetTokenInstructions() { // FprintUnsetTokenInstructions writes platform-specific instructions for // unsetting DATAROBOT_API_TOKEN to w. func FprintUnsetTokenInstructions(w io.Writer) { - fmt.Fprint(w, tui.InfoStyle.Render(" unset DATAROBOT_API_TOKEN")) - fmt.Fprint(w, tui.BaseTextStyle.Render(" (or ")) - fmt.Fprint(w, tui.InfoStyle.Render("Remove-Item Env:\\DATAROBOT_API_TOKEN")) - fmt.Fprintln(w, tui.BaseTextStyle.Render(" on Windows)")) + base, info := writerStyles(w) + + fmt.Fprint(w, info.Render(" unset DATAROBOT_API_TOKEN")) + fmt.Fprint(w, base.Render(" (or ")) + fmt.Fprint(w, info.Render("Remove-Item Env:\\DATAROBOT_API_TOKEN")) + fmt.Fprintln(w, base.Render(" on Windows)")) +} + +// writerStyles binds the shared text styles to w's renderer. The package +// defaults probe stdout for color support, so styled text sent to a different +// stream (stderr redirected to a file while stdout is a TTY, or a buffer in +// tests) would otherwise carry ANSI codes the destination cannot display. +func writerStyles(w io.Writer) (base, info lipgloss.Style) { + r := lipgloss.NewRenderer(w) + + return tui.BaseTextStyle.Renderer(r), tui.InfoStyle.Renderer(r) } // EnvCredentials holds environment variable authentication credentials. type EnvCredentials struct { Endpoint string Token string + // EndpointVar is the environment variable Endpoint was read from: + // DATAROBOT_ENDPOINT, or the SDK-style DATAROBOT_API_ENDPOINT fallback. + // Error messages name it so the user fixes the variable that is actually + // set. + EndpointVar string +} + +// endpointVarName returns EndpointVar, defaulting to DATAROBOT_ENDPOINT for +// zero-value credentials constructed outside GetEnvCredentials. +func (c *EnvCredentials) endpointVarName() string { + if c.EndpointVar == "" { + return "DATAROBOT_ENDPOINT" + } + + return c.EndpointVar } // GetEnvCredentials reads DATAROBOT_ENDPOINT and DATAROBOT_API_TOKEN from environment. @@ -73,14 +102,18 @@ type EnvCredentials struct { // need to report why env credentials failed should use ValidateEndpoint to // distinguish a malformed endpoint from an invalid token. func GetEnvCredentials() EnvCredentials { - endpoint := os.Getenv("DATAROBOT_ENDPOINT") + endpointVar := "DATAROBOT_ENDPOINT" + + endpoint := os.Getenv(endpointVar) if endpoint == "" { - endpoint = os.Getenv("DATAROBOT_API_ENDPOINT") + endpointVar = "DATAROBOT_API_ENDPOINT" + endpoint = os.Getenv(endpointVar) } return EnvCredentials{ - Endpoint: endpoint, - Token: os.Getenv("DATAROBOT_API_TOKEN"), + Endpoint: endpoint, + Token: os.Getenv("DATAROBOT_API_TOKEN"), + EndpointVar: endpointVar, } } @@ -102,32 +135,52 @@ func ValidateEndpoint(endpoint string) error { // ReportEnvCredentialsError writes a classified explanation of why an // explicitly supplied DATAROBOT_ENDPOINT/DATAROBOT_API_TOKEN pair failed -// verification: a network timeout, a malformed endpoint, or an invalid token. -// Shared by EnsureAuthenticated (writing to stderr) and `dr auth check`. +// verification: a network timeout, a malformed endpoint, an unreachable +// endpoint, or an invalid token. Shared by EnsureAuthenticated (writing to +// stderr) and `dr auth check`. func ReportEnvCredentialsError(w io.Writer, creds *EnvCredentials, err error) { + base, info := writerStyles(w) + if errors.Is(err, context.DeadlineExceeded) { envDatarobotHost, _ := config.SchemeHostOnly(creds.Endpoint) - fmt.Fprint(w, tui.BaseTextStyle.Render("❌ Connection to ")) - fmt.Fprint(w, tui.InfoStyle.Render(envDatarobotHost)) - fmt.Fprintln(w, tui.BaseTextStyle.Render(" timed out. Check your network and try again.")) + fmt.Fprint(w, base.Render("❌ Connection to ")) + fmt.Fprint(w, info.Render(envDatarobotHost)) + fmt.Fprintln(w, base.Render(" timed out. Check your network and try again.")) return } - // Distinguish a malformed DATAROBOT_ENDPOINT from an invalid token so the - // user fixes the right thing. A quoted endpoint (e.g. from running + // Distinguish a malformed endpoint from an invalid token so the user + // fixes the right thing. A quoted endpoint (e.g. from running // `$(dr auth export)` instead of `eval "$(dr auth export)"`) fails here // and must not be reported as a token problem. if epErr := ValidateEndpoint(creds.Endpoint); epErr != nil { - fmt.Fprintln(w, tui.BaseTextStyle.Render("❌ DATAROBOT_ENDPOINT environment variable is invalid: "+epErr.Error())) - fmt.Fprintln(w, tui.BaseTextStyle.Render("Set it to a valid DataRobot URL and try again.")) + fmt.Fprintln(w, base.Render("❌ "+creds.endpointVarName()+" environment variable is invalid: "+epErr.Error())) + fmt.Fprintln(w, base.Render("Set it to a valid DataRobot URL and try again.")) + + return + } + + // A transport-level failure (connection refused, DNS, TLS, proxy) means + // the instance was never reached and the token was never judged, so + // advising the user to unset the token would point them at the wrong + // thing. VerifyToken wraps transport errors in *url.Error; a rejected + // token comes back as a plain error from the HTTP status check. + var urlErr *url.Error + if errors.As(err, &urlErr) { + envDatarobotHost, _ := config.SchemeHostOnly(creds.Endpoint) + + fmt.Fprint(w, base.Render("❌ Could not connect to ")) + fmt.Fprint(w, info.Render(envDatarobotHost)) + fmt.Fprintln(w, base.Render(": "+urlErr.Err.Error())) + fmt.Fprintln(w, base.Render("Check "+creds.endpointVarName()+" and your network, then try again.")) return } - fmt.Fprintln(w, tui.BaseTextStyle.Render("❌ DATAROBOT_API_TOKEN environment variable is invalid or expired.")) - fmt.Fprintln(w, tui.BaseTextStyle.Render("Unset it and try again:")) + fmt.Fprintln(w, base.Render("❌ DATAROBOT_API_TOKEN environment variable is invalid or expired.")) + fmt.Fprintln(w, base.Render("Unset it and try again:")) FprintUnsetTokenInstructions(w) } @@ -146,11 +199,13 @@ func reportStoredProfileNotUsed(w io.Writer, creds *EnvCredentials) { requestedHost = host } - fmt.Fprint(w, tui.BaseTextStyle.Render("Environment credentials for ")) - fmt.Fprint(w, tui.InfoStyle.Render(requestedHost)) - fmt.Fprint(w, tui.BaseTextStyle.Render(" failed to verify; not falling back to the stored profile for ")) - fmt.Fprint(w, tui.InfoStyle.Render(storedHost)) - fmt.Fprintln(w, tui.BaseTextStyle.Render(".")) + base, info := writerStyles(w) + + fmt.Fprint(w, base.Render("Environment credentials for ")) + fmt.Fprint(w, info.Render(requestedHost)) + fmt.Fprint(w, base.Render(" failed to verify; not falling back to the stored profile for ")) + fmt.Fprint(w, info.Render(storedHost)) + fmt.Fprintln(w, base.Render(".")) } // VerifyEnvCredentials checks if environment variable credentials are valid. diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 9b4dd31c9..9302c9b74 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -20,6 +20,7 @@ import ( "errors" "net/http" "net/http/httptest" + "net/url" "os" "path/filepath" "testing" @@ -232,6 +233,58 @@ func TestReportEnvCredentialsError(t *testing.T) { assert.Contains(t, buf.String(), "DATAROBOT_API_TOKEN environment variable is invalid or expired") assert.Contains(t, buf.String(), "unset DATAROBOT_API_TOKEN") }) + + t.Run("unreachable endpoint is not a token problem", func(t *testing.T) { + var buf bytes.Buffer + + creds := &EnvCredentials{Endpoint: "https://app.example.com/api/v2", Token: "valid-but-never-judged"} + transportErr := &url.Error{ + Op: "Get", + URL: creds.Endpoint + "/version/", + Err: errors.New("dial tcp 203.0.113.1:443: connect: connection refused"), + } + ReportEnvCredentialsError(&buf, creds, transportErr) + + assert.Contains(t, buf.String(), "Could not connect to") + assert.Contains(t, buf.String(), "connection refused") + assert.NotContains(t, buf.String(), "unset DATAROBOT_API_TOKEN") + }) + + t.Run("names DATAROBOT_API_ENDPOINT when the endpoint came from the fallback var", func(t *testing.T) { + var buf bytes.Buffer + + t.Setenv("DATAROBOT_ENDPOINT", "") + t.Setenv("DATAROBOT_API_ENDPOINT", `"https://app.example.com/api/v2"`) + t.Setenv("DATAROBOT_API_TOKEN", "some-token") + + creds := GetEnvCredentials() + ReportEnvCredentialsError(&buf, &creds, errors.New("parse error")) + + assert.Contains(t, buf.String(), "DATAROBOT_API_ENDPOINT environment variable is invalid") + assert.NotContains(t, buf.String(), "DATAROBOT_ENDPOINT environment variable is invalid") + }) +} + +// TestEnsureAuthenticated_EnvUnreachableStoredValid proves VerifyToken's +// transport errors really surface as *url.Error and classify as +// could-not-connect end to end, still failing instead of using the stored +// profile. +func TestEnsureAuthenticated_EnvUnreachableStoredValid(t *testing.T) { + _, cleanup := setupTestEnvironment(t) + defer cleanup() + + // A second server, closed immediately, donates a port that refuses + // connections. + deadServer := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {})) + deadServer.Close() + + viperx.Set(config.DataRobotAPIKey, "valid-token") + t.Setenv("DATAROBOT_ENDPOINT", deadServer.URL+"/api/v2") + t.Setenv("DATAROBOT_API_TOKEN", "valid-token") + + result := EnsureAuthenticated(context.Background()) + assert.False(t, result, + "Expected EnsureAuthenticated to fail on an unreachable endpoint instead of using the stored profile") } func TestReportStoredProfileNotUsed(t *testing.T) { From 6a306792af67a44c82b1007af4ef93b20adab41a Mon Sep 17 00:00:00 2001 From: chas Date: Tue, 11 Aug 2026 21:05:14 -0400 Subject: [PATCH 04/10] docs: describe env-credential precedence and the could-not-connect check output --- docs/commands/auth.md | 5 +++++ docs/development/authentication.md | 9 +++++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/docs/commands/auth.md b/docs/commands/auth.md index 75d84b82b..b60e2eed6 100644 --- a/docs/commands/auth.md +++ b/docs/commands/auth.md @@ -170,6 +170,11 @@ $ dr auth check ❌ DATAROBOT_API_TOKEN environment variable is invalid or expired. Unset it and try again: unset DATAROBOT_API_TOKEN (or Remove-Item Env:\DATAROBOT_API_TOKEN on Windows) + +# Unreachable endpoint (the token was never checked, so it is not blamed) +$ dr auth check +❌ Could not connect to https://app.example.com: dial tcp: lookup app.example.com: no such host +Check DATAROBOT_ENDPOINT and your network, then try again. ``` > [!TIP] diff --git a/docs/development/authentication.md b/docs/development/authentication.md index df950f032..77556fa3c 100644 --- a/docs/development/authentication.md +++ b/docs/development/authentication.md @@ -29,10 +29,11 @@ var MyCmd = &cobra.Command{ The hook functions are outlined below. -1. **Checks for valid credentials**: Checks if a valid API key already exists. -2. **Auto-configures URL if missing**: If no DataRobot URL is configured, prompts you to set it up. -3. **Retrieves new credentials**: If credentials are missing or expired, the hook automatically triggers the browser-based login flow. -4. **Fails early**: If authentication cannot be established, the command will not run and returns an error. +1. **Checks environment credentials first**: A complete `DATAROBOT_ENDPOINT`/`DATAROBOT_API_TOKEN` pair takes precedence over the config file. If the pair fails verification, the command fails with the reason (timeout, malformed endpoint, unreachable endpoint, or invalid token). It never falls back to the stored profile and never starts the login flow, because that would silently run the command against a different DataRobot instance than the one requested. +2. **Checks for valid credentials**: With no complete environment pair, checks if a valid API key already exists in the config file. +3. **Auto-configures URL if missing**: If no DataRobot URL is configured, prompts you to set it up. +4. **Retrieves new credentials**: If config-file credentials are missing or expired, the hook automatically triggers the browser-based login flow. +5. **Fails early**: If authentication cannot be established, the command will not run and returns an error. ### Direct call for non-command code From 91aa6b4cab6f786d00a61af65b272724b814780f Mon Sep 17 00:00:00 2001 From: chas Date: Wed, 12 Aug 2026 11:27:45 -0400 Subject: [PATCH 05/10] Addressing copilot review: name the DATAROBOT_API_ENDPOINT fallback in the precedence step, drop a double space --- docs/development/authentication.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/development/authentication.md b/docs/development/authentication.md index 77556fa3c..6f9fefc7c 100644 --- a/docs/development/authentication.md +++ b/docs/development/authentication.md @@ -29,11 +29,11 @@ var MyCmd = &cobra.Command{ The hook functions are outlined below. -1. **Checks environment credentials first**: A complete `DATAROBOT_ENDPOINT`/`DATAROBOT_API_TOKEN` pair takes precedence over the config file. If the pair fails verification, the command fails with the reason (timeout, malformed endpoint, unreachable endpoint, or invalid token). It never falls back to the stored profile and never starts the login flow, because that would silently run the command against a different DataRobot instance than the one requested. +1. **Checks environment credentials first**: A complete `DATAROBOT_ENDPOINT` (or `DATAROBOT_API_ENDPOINT`) and `DATAROBOT_API_TOKEN` pair takes precedence over the config file. If the pair fails verification, the command fails with the reason (timeout, malformed endpoint, unreachable endpoint, or invalid token). It never falls back to the stored profile and never starts the login flow, because that would silently run the command against a different DataRobot instance than the one requested. 2. **Checks for valid credentials**: With no complete environment pair, checks if a valid API key already exists in the config file. 3. **Auto-configures URL if missing**: If no DataRobot URL is configured, prompts you to set it up. 4. **Retrieves new credentials**: If config-file credentials are missing or expired, the hook automatically triggers the browser-based login flow. -5. **Fails early**: If authentication cannot be established, the command will not run and returns an error. +5. **Fails early**: If authentication cannot be established, the command will not run and returns an error. ### Direct call for non-command code From 5fb15e4aa7326ba2c85bb829e4e2eb5a0275a026 Mon Sep 17 00:00:00 2001 From: chas Date: Wed, 12 Aug 2026 11:28:34 -0400 Subject: [PATCH 06/10] Addressing cursor review: classify undialable endpoints as invalid, not could-not-connect Cursor flagged the scheme-less case: ValidateEndpoint forgives what SchemeHostOnly can clean up while VerifyToken dials the raw value, so the transport branch printed a normalized https host that was never requested. Fixed the whole class, not just that case: a missing scheme and a raw url.Parse failure (Op parse, e.g. leading whitespace) both classify as an invalid endpoint now, so the could-not-connect branch only ever names a host that was actually dialed. Two classifier tests added. --- internal/auth/auth.go | 25 +++++++++++++++++++++++-- internal/auth/auth_test.go | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 472e58000..21cc25f93 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -162,13 +162,34 @@ func ReportEnvCredentialsError(w io.Writer, creds *EnvCredentials, err error) { return } + // VerifyToken dials the raw endpoint value, while ValidateEndpoint above + // forgives what SchemeHostOnly can clean up (a bare host without a + // scheme, stray whitespace). Catch those as endpoint problems here so the + // transport branch below only ever names a host that was actually dialed. + var urlErr *url.Error + + hasURLErr := errors.As(err, &urlErr) + + if !strings.Contains(strings.TrimSpace(creds.Endpoint), "://") { + fmt.Fprintln(w, base.Render("❌ "+creds.endpointVarName()+" environment variable is invalid: missing URL scheme (https://...)")) + fmt.Fprintln(w, base.Render("Set it to a valid DataRobot URL and try again.")) + + return + } + + if hasURLErr && urlErr.Op == "parse" { + fmt.Fprintln(w, base.Render("❌ "+creds.endpointVarName()+" environment variable is invalid: "+urlErr.Err.Error())) + fmt.Fprintln(w, base.Render("Set it to a valid DataRobot URL and try again.")) + + return + } + // A transport-level failure (connection refused, DNS, TLS, proxy) means // the instance was never reached and the token was never judged, so // advising the user to unset the token would point them at the wrong // thing. VerifyToken wraps transport errors in *url.Error; a rejected // token comes back as a plain error from the HTTP status check. - var urlErr *url.Error - if errors.As(err, &urlErr) { + if hasURLErr { envDatarobotHost, _ := config.SchemeHostOnly(creds.Endpoint) fmt.Fprint(w, base.Render("❌ Could not connect to ")) diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 9302c9b74..70a6b571d 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -234,6 +234,38 @@ func TestReportEnvCredentialsError(t *testing.T) { assert.Contains(t, buf.String(), "unset DATAROBOT_API_TOKEN") }) + t.Run("scheme-less endpoint is an endpoint problem, not transport", func(t *testing.T) { + var buf bytes.Buffer + + creds := &EnvCredentials{Endpoint: "app.example.com/api/v2", Token: "some-token"} + dialErr := &url.Error{ + Op: "Get", + URL: creds.Endpoint + "/version/", + Err: errors.New(`unsupported protocol scheme ""`), + } + ReportEnvCredentialsError(&buf, creds, dialErr) + + assert.Contains(t, buf.String(), "missing URL scheme") + assert.NotContains(t, buf.String(), "Could not connect") + }) + + t.Run("raw parse failure is an endpoint problem, not transport", func(t *testing.T) { + var buf bytes.Buffer + + // Leading whitespace fails VerifyToken's raw url.Parse but survives + // ValidateEndpoint, which trims before parsing. + creds := &EnvCredentials{Endpoint: " https://app.example.com/api/v2", Token: "some-token"} + parseErr := &url.Error{ + Op: "parse", + URL: creds.Endpoint, + Err: errors.New("first path segment in URL cannot contain colon"), + } + ReportEnvCredentialsError(&buf, creds, parseErr) + + assert.Contains(t, buf.String(), "environment variable is invalid") + assert.NotContains(t, buf.String(), "Could not connect") + }) + t.Run("unreachable endpoint is not a token problem", func(t *testing.T) { var buf bytes.Buffer From 118d6cd872e916f631f68eaca390a131db4da775 Mon Sep 17 00:00:00 2001 From: chas Date: Wed, 12 Aug 2026 11:28:58 -0400 Subject: [PATCH 07/10] Addressing cursor review: partial-env message stops claiming the unverified token is expired --- internal/auth/auth.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 21cc25f93..a3742f917 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -313,8 +313,8 @@ func EnsureAuthenticated(ctx context.Context) bool { //nolint: cyclop // it (a complete pair was handled above), so the token was never // verified. Point at it rather than starting a login flow that would // shadow it. - fmt.Println(tui.BaseTextStyle.Render("Your DATAROBOT_API_TOKEN environment variable")) - fmt.Println(tui.BaseTextStyle.Render("contains an expired or invalid token. Unset it:")) + fmt.Println(tui.BaseTextStyle.Render("Your DATAROBOT_API_TOKEN environment variable is set")) + fmt.Println(tui.BaseTextStyle.Render("without a DATAROBOT_ENDPOINT. Set that too, or unset the token:")) PrintUnsetTokenInstructions() skipAuthFlow = true From a9647625f3063fe445e92a57d0ecdbad05e8c31d Mon Sep 17 00:00:00 2001 From: chas Date: Thu, 13 Aug 2026 14:13:28 -0400 Subject: [PATCH 08/10] Addressing c-h-russell-walker review: reword the endpoint-guard comment in plain language --- internal/auth/auth.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index a3742f917..a6f914387 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -162,10 +162,12 @@ func ReportEnvCredentialsError(w io.Writer, creds *EnvCredentials, err error) { return } - // VerifyToken dials the raw endpoint value, while ValidateEndpoint above - // forgives what SchemeHostOnly can clean up (a bare host without a - // scheme, stray whitespace). Catch those as endpoint problems here so the - // transport branch below only ever names a host that was actually dialed. + // VerifyToken uses the endpoint value exactly as the user set it, but + // ValidateEndpoint above is more forgiving: it accepts a bare host with + // no scheme and trims stray whitespace. The two checks below catch the + // values that slipped through that gap, so they are reported as a bad + // endpoint instead of as a connection failure naming a URL the CLI never + // actually requested. var urlErr *url.Error hasURLErr := errors.As(err, &urlErr) From 12a873295a1b5052542026a574f600f11ca430fd Mon Sep 17 00:00:00 2001 From: chas Date: Thu, 13 Aug 2026 14:27:17 -0400 Subject: [PATCH 09/10] Addressing ajalon1 review: lowercase the authentication-failed error per Go error-string convention --- internal/auth/auth.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index a6f914387..1277b2225 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -250,7 +250,7 @@ func VerifyEnvCredentials(ctx context.Context) (*EnvCredentials, error) { // Cobra PreRunE hooks. func EnsureAuthenticatedE(cmd *cobra.Command, _ []string) error { if !EnsureAuthenticated(cmd.Context()) { - return errors.New("Authentication failed.") + return errors.New("authentication failed") } return nil From 63b30774da668816278d4f670e6fbbcc5dfc24fb Mon Sep 17 00:00:00 2001 From: chas Date: Thu, 13 Aug 2026 14:27:17 -0400 Subject: [PATCH 10/10] Addressing ajalon1 review: note why auth check skips the not-falling-back line --- cmd/auth/check/cmd.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cmd/auth/check/cmd.go b/cmd/auth/check/cmd.go index 232bbdc2a..0139b1686 100644 --- a/cmd/auth/check/cmd.go +++ b/cmd/auth/check/cmd.go @@ -43,7 +43,10 @@ func checkCLICredentials(w io.Writer) bool { return true } - // If env vars were set but invalid, report the error + // If env vars were set but invalid, report the error. Unlike + // EnsureAuthenticated this deliberately skips the "not falling back to + // the stored profile" line: check evaluates the stored profile itself + // right below and reports that result on its own. if !errors.Is(err, auth.ErrEnvCredentialsNotSet) { auth.ReportEnvCredentialsError(w, creds, err)