From 8ea508fc8f2291592e477dc1dcba243ea4a30c58 Mon Sep 17 00:00:00 2001 From: Volodymyr Onofriichuk Date: Mon, 17 Aug 2026 20:05:30 +0300 Subject: [PATCH 1/4] [CFX-6924] fix(tls): repair Windows cert export and stop swallowing TLS errors Three defects that compounded into an undiagnosable failure: the export broke, the error explaining it was discarded, and there was no fallback diagnostic. 1. Pin PSModulePath for the cert-export subprocess. ExportWindowsCerts spawned powershell.exe with the parent environment. The Cert: drive comes from Microsoft.PowerShell.Security, which autoloads only if PSModulePath includes the Windows PowerShell system module directory. pwsh 7 sets a value that omits it, and CI runners often mangle it -- the child then could not enumerate any store, -ErrorAction SilentlyContinue ate the DriveNotFoundException, and empty stdout surfaced as "no certificates found in Windows cert store". The child now gets a pinned PSModulePath. The script also imports the module explicitly and asserts the Cert: drive exists, so a genuine module problem reports itself instead of masquerading as an empty store. Per-store SilentlyContinue is kept, since an individual store may legitimately be empty. Verified on Windows 11 25H2 against the same broken PSModulePath that reproduces the bug: v0.2.83 exits 1 with no bundle, the fixed binary exits 0 and writes 73 certificates. 2. Make cmd.ReportError the single error reporter. Cobra skips printing when the executed command sets SilenceErrors: true, and main.go discarded the error entirely -- so anything raised by the root PersistentPreRunE vanished for the 11 commands setting that flag, including all of auth. "dr --ca-cert auth check" exited 1 with zero bytes on both streams, on every platform. A typo in a CA path silently fell back to system trust. RootCmd now sets SilenceErrors so cobra never prints, and main reports through cmd.ReportError, which honours the existing cli.ErrSilent contract. Errors are printed exactly once. ExecuteContext returns unwrapped, since its "execute root command:" prefix would otherwise leak into user-facing output. 3. Surface the subprocess's stderr. cmd.Output already captures the child's stderr into ExitError.Stderr and the code dropped it. It is now included in the error, and the empty-store message says which stores were checked. Tests: table tests for the PSModulePath helpers (kept build-tag free so they run on every host, not only Windows) and for ReportError, plus a guard that RootCmd.SilenceErrors stays true. task lint passes for linux, darwin and windows; task test passes. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/exit.go | 21 +++++ cmd/exit_test.go | 84 +++++++++++++++++++ cmd/root.go | 18 ++-- internal/tls/psmodulepath.go | 73 ++++++++++++++++ internal/tls/psmodulepath_test.go | 134 ++++++++++++++++++++++++++++++ internal/tls/tls_windows.go | 34 +++++++- main.go | 5 ++ 7 files changed, 361 insertions(+), 8 deletions(-) create mode 100644 cmd/exit_test.go create mode 100644 internal/tls/psmodulepath.go create mode 100644 internal/tls/psmodulepath_test.go diff --git a/cmd/exit.go b/cmd/exit.go index 6ce413a51..6d9fc17dc 100644 --- a/cmd/exit.go +++ b/cmd/exit.go @@ -15,10 +15,31 @@ package cmd import ( + "errors" + "fmt" + "io" "os" "time" + + "github.com/datarobot/cli/internal/cli" ) +// ReportError writes a user-facing "Error: ..." line for err to w, unless err is +// cli.ErrSilent — the sentinel meaning a command already printed its own message. +// +// RootCmd sets SilenceErrors so cobra never prints errors itself, which makes this +// the single place errors reach the user. Previously cobra was the only reporter, +// and every command setting SilenceErrors: true silently swallowed failures raised +// by the root PersistentPreRunE (config loading, TLS setup) that it never had a +// chance to print — a bad --ca-cert path exited 1 with no output at all (CFX-6924). +func ReportError(w io.Writer, err error) { + if err == nil || errors.Is(err, cli.ErrSilent) { + return + } + + fmt.Fprintln(w, "Error:", err) +} + // Exit flushes any pending telemetry events then terminates the process with // code. Call this from main (instead of os.Exit) when ExecuteContext returns // an error, so that Amplitude events are delivered even though diff --git a/cmd/exit_test.go b/cmd/exit_test.go new file mode 100644 index 000000000..2eb770a35 --- /dev/null +++ b/cmd/exit_test.go @@ -0,0 +1,84 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package cmd + +import ( + "bytes" + "errors" + "fmt" + "testing" + + "github.com/datarobot/cli/internal/cli" + "github.com/stretchr/testify/assert" +) + +func TestReportError(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + err error + want string + }{ + { + name: "nil error prints nothing", + err: nil, + want: "", + }, + { + name: "ErrSilent prints nothing because the command already reported it", + err: cli.ErrSilent, + want: "", + }, + { + name: "an error wrapping ErrSilent is still silent", + err: fmt.Errorf("auth check: %w", cli.ErrSilent), + want: "", + }, + { + name: "an ordinary error is reported", + err: errors.New("boom"), + want: "Error: boom\n", + }, + { + name: "a TLS setup failure from the root PersistentPreRunE is reported", + err: errors.New(`apply tls options: reading CA cert "C:\nope.pem": no such file or directory`), + want: "Error: apply tls options: reading CA cert \"C:\\nope.pem\": no such file or directory\n", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + var buf bytes.Buffer + + ReportError(&buf, tt.err) + + assert.Equal(t, tt.want, buf.String()) + }) + } +} + +// TestRootSilencesErrorsForSingleReporting locks in the contract that makes +// ReportError the only error reporter. If cobra were allowed to print too, errors +// raised by the root PersistentPreRunE would either double-print or, for commands +// setting SilenceErrors: true, disappear entirely (CFX-6924). +func TestRootSilencesErrorsForSingleReporting(t *testing.T) { + t.Parallel() + + assert.True(t, RootCmd.SilenceErrors, + "RootCmd.SilenceErrors must stay true so cmd.ReportError is the single error reporter") +} diff --git a/cmd/root.go b/cmd/root.go index 9b476fab4..6c425fbe5 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -194,12 +194,13 @@ using pre-built templates. Get from idea to production in minutes, not hours. // ExecuteContext executes the root command with the given context. // It adds all child commands to the root command and sets flags appropriately. // This is called by main.main(). It only needs to happen once to the rootCmd. +// The error is returned unwrapped on purpose. main.go reports it verbatim through +// cmd.ReportError, so an "execute root command: ..." wrapper would surface in +// user-facing output and add nothing — this is the process boundary, and there is no +// sibling call site an extra frame could disambiguate. func ExecuteContext(ctx context.Context) error { - if err := RootCmd.ExecuteContext(ctx); err != nil { - return fmt.Errorf("execute root command: %w", err) - } - - return nil + //nolint:wrapcheck // top-level passthrough; wrapping would leak into user output + return RootCmd.ExecuteContext(ctx) } // bindUniversal binds name to viper and annotates the flag for forwarding to @@ -224,6 +225,13 @@ func init() { // Disable Cobra's default completion command since we have our own under 'self' RootCmd.CompletionOptions.DisableDefaultCmd = true + // Silence cobra's own error printing so main.go, via cmd.ReportError, is the single + // reporter. Cobra skips printing whenever the executed command sets + // SilenceErrors: true, which used to mean errors raised by the root + // PersistentPreRunE (config, TLS) vanished entirely for those commands. Centralizing + // it also removes the risk of the same error being printed twice. See CFX-6924. + RootCmd.SilenceErrors = true + // Set custom version template to match our unified format RootCmd.SetVersionTemplate(internalVersion.GetAppNameVersionText() + "\n\nTo update: dr self update\n") diff --git a/internal/tls/psmodulepath.go b/internal/tls/psmodulepath.go new file mode 100644 index 000000000..a3be779e8 --- /dev/null +++ b/internal/tls/psmodulepath.go @@ -0,0 +1,73 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package tls + +import "strings" + +// psModulePathKey is the environment variable telling PowerShell where to look for +// modules. Matched case-insensitively, since Windows environment variable names are. +const psModulePathKey = "PSMODULEPATH" + +// defaultSystemRoot is the fallback when SystemRoot is absent from the environment. +const defaultSystemRoot = `C:\Windows` + +// These helpers deliberately carry no build constraint so they can be unit tested on +// any host, even though only the Windows build calls them. + +// windowsPowerShellModulePath returns the Windows PowerShell 5.1 system module +// directory under systemRoot. That directory holds Microsoft.PowerShell.Security, +// the module providing the Cert: drive — without it on PSModulePath, powershell.exe +// cannot enumerate the certificate stores at all. +// +// The path is assembled with backslashes rather than filepath.Join so the result is +// identical no matter which OS runs the tests. +func windowsPowerShellModulePath(systemRoot string) string { + if systemRoot == "" { + systemRoot = defaultSystemRoot + } + + return strings.Join([]string{ + strings.TrimRight(systemRoot, `\`), + "System32", + "WindowsPowerShell", + "v1.0", + "Modules", + }, `\`) +} + +// withPSModulePath returns env with every PSModulePath entry replaced by a single +// entry pointing at modulePath. +// +// A child powershell.exe otherwise inherits the parent's PSModulePath, and a value +// omitting the Windows PowerShell system module directory stops +// Microsoft.PowerShell.Security from autoloading. The Cert: drive then does not +// exist, the export yields nothing, and the failure surfaces as the misleading +// "no certificates found in Windows cert store" (CFX-6924). pwsh 7 sets exactly +// such a value, and CI runners frequently mangle it. +func withPSModulePath(env []string, modulePath string) []string { + out := make([]string, 0, len(env)+1) + + for _, entry := range env { + key, _, found := strings.Cut(entry, "=") + + if found && strings.EqualFold(key, psModulePathKey) { + continue + } + + out = append(out, entry) + } + + return append(out, "PSModulePath="+modulePath) +} diff --git a/internal/tls/psmodulepath_test.go b/internal/tls/psmodulepath_test.go new file mode 100644 index 000000000..f6dfba5b9 --- /dev/null +++ b/internal/tls/psmodulepath_test.go @@ -0,0 +1,134 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package tls + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestWindowsPowerShellModulePath(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + systemRoot string + want string + }{ + { + name: "typical SystemRoot", + systemRoot: `C:\WINDOWS`, + want: `C:\WINDOWS\System32\WindowsPowerShell\v1.0\Modules`, + }, + { + name: "empty SystemRoot falls back to C:\\Windows", + systemRoot: "", + want: `C:\Windows\System32\WindowsPowerShell\v1.0\Modules`, + }, + { + name: "trailing separator is not doubled", + systemRoot: `D:\Windows\`, + want: `D:\Windows\System32\WindowsPowerShell\v1.0\Modules`, + }, + { + name: "relocated system root", + systemRoot: `E:\CustomWin`, + want: `E:\CustomWin\System32\WindowsPowerShell\v1.0\Modules`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, tt.want, windowsPowerShellModulePath(tt.systemRoot)) + }) + } +} + +func TestWithPSModulePath(t *testing.T) { + t.Parallel() + + const pinned = `C:\WINDOWS\System32\WindowsPowerShell\v1.0\Modules` + + tests := []struct { + name string + env []string + want []string + }{ + { + name: "replaces an inherited pwsh 7 value", + env: []string{ + "PATH=C:\\bin", + `PSModulePath=C:\Program Files\PowerShell\7\Modules`, + "SystemRoot=C:\\WINDOWS", + }, + want: []string{"PATH=C:\\bin", "SystemRoot=C:\\WINDOWS", "PSModulePath=" + pinned}, + }, + { + name: "matches the key case-insensitively", + env: []string{"psmodulepath=junk", "PATH=C:\\bin"}, + want: []string{"PATH=C:\\bin", "PSModulePath=" + pinned}, + }, + { + name: "adds the entry when absent", + env: []string{"PATH=C:\\bin"}, + want: []string{"PATH=C:\\bin", "PSModulePath=" + pinned}, + }, + { + name: "drops every duplicate, not just the first", + env: []string{"PSModulePath=a", "PATH=C:\\bin", "PSMODULEPATH=b"}, + want: []string{"PATH=C:\\bin", "PSModulePath=" + pinned}, + }, + { + name: "empty environment still yields the pinned entry", + env: nil, + want: []string{"PSModulePath=" + pinned}, + }, + { + name: "entries without a separator are preserved", + env: []string{"MALFORMED", "PSModulePath=x"}, + want: []string{"MALFORMED", "PSModulePath=" + pinned}, + }, + { + name: "a key merely prefixed with PSModulePath is left alone", + env: []string{"PSModulePathExtra=keep", "PSModulePath=drop"}, + want: []string{"PSModulePathExtra=keep", "PSModulePath=" + pinned}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, tt.want, withPSModulePath(tt.env, pinned)) + }) + } +} + +// TestWithPSModulePathDoesNotMutateInput guards against aliasing: os.Environ()'s +// slice must not be rewritten underneath the caller. +func TestWithPSModulePathDoesNotMutateInput(t *testing.T) { + t.Parallel() + + env := []string{"PATH=C:\\bin", "PSModulePath=original"} + before := make([]string, len(env)) + copy(before, env) + + withPSModulePath(env, "pinned") + + assert.Equal(t, before, env) +} diff --git a/internal/tls/tls_windows.go b/internal/tls/tls_windows.go index 5b4021080..80492b17f 100644 --- a/internal/tls/tls_windows.go +++ b/internal/tls/tls_windows.go @@ -28,9 +28,18 @@ import ( // certificate store and writes them as a PEM bundle to dest. func ExportWindowsCerts(dest string) error { script := strings.Join([]string{ + // Load the module providing the Cert: drive explicitly and fail loudly when it + // is unavailable. Relying on autoloading turned a broken PSModulePath into a + // silent empty result that reported itself as "no certificates found". + `Import-Module Microsoft.PowerShell.Security -ErrorAction Stop`, + `if (-not (Get-PSDrive -PSProvider Certificate -ErrorAction SilentlyContinue)) ` + + `{ throw 'Cert: drive unavailable - Microsoft.PowerShell.Security did not load' }`, `$out = @()`, `foreach ($store in 'Root','CA') {`, ` foreach ($loc in 'LocalMachine','CurrentUser') {`, + // SilentlyContinue is kept here on purpose: an individual store may legitimately + // be empty or absent. The precondition above distinguishes that from the whole + // certificate provider being missing. ` Get-ChildItem -Path "Cert:\$loc\$store" -ErrorAction SilentlyContinue | ForEach-Object {`, ` $out += '-----BEGIN CERTIFICATE-----'`, ` $out += [Convert]::ToBase64String($_.RawData, 'InsertLineBreaks')`, @@ -41,17 +50,36 @@ func ExportWindowsCerts(dest string) error { `$out -join [Environment]::NewLine`, }, "; ") - out, err := exec.Command( + cmd := exec.Command( "powershell.exe", "-NoProfile", "-NonInteractive", "-Command", script, - ).Output() + ) + + // Pin PSModulePath instead of inheriting it; see withPSModulePath for why. + cmd.Env = withPSModulePath( + os.Environ(), + windowsPowerShellModulePath(os.Getenv("SystemRoot")), + ) + + out, err := cmd.Output() if err != nil { + // cmd.Output captures the child's stderr into ExitError.Stderr. Surfacing it is + // the difference between "something went wrong" and a message naming the cause. + if exitErr, ok := errors.AsType[*exec.ExitError](err); ok { + if stderr := strings.TrimSpace(string(exitErr.Stderr)); stderr != "" { + return fmt.Errorf("exporting Windows cert store: %w: %s", err, stderr) + } + } + return fmt.Errorf("exporting Windows cert store: %w", err) } pem := strings.TrimSpace(string(out)) if pem == "" { - return errors.New("no certificates found in Windows cert store") + return errors.New( + "no certificates found in Windows cert store: the Root and CA stores are " + + "empty for both LocalMachine and CurrentUser", + ) } if err := os.WriteFile(dest, []byte(pem+"\n"), 0o600); err != nil { diff --git a/main.go b/main.go index d157b3f07..addac82e3 100644 --- a/main.go +++ b/main.go @@ -34,6 +34,11 @@ func main() { defer log.Stop() if err := cmd.ExecuteContext(ctx); err != nil { + // RootCmd sets SilenceErrors, so nothing has printed this yet. Report it here + // before exiting; ReportError skips cli.ErrSilent, which commands return once + // they have printed their own message. + cmd.ReportError(os.Stderr, err) + log.Stop() cmd.Exit(1) } From 371a3ad51f399d02c9c5bb82372a2a86fff8011d Mon Sep 17 00:00:00 2001 From: Volodymyr Onofriichuk Date: Tue, 18 Aug 2026 16:11:29 +0300 Subject: [PATCH 2/4] [CFX-6924] fix(auth): return ErrSilent from paths that already reported Addresses Cursor bugbot on #786. Centralizing error reporting in main.go made the cli.ErrSilent contract load-bearing, and three paths that print their own message still returned plain errors -- so those failures printed twice. cmd/auth/login RunE had the pattern right in two places (return cli.ErrSilent after log.Error) and wrong in three; those three now follow the file's own convention. auth.EnsureAuthenticatedE returned errors.New("authentication failed") after EnsureAuthenticated had already written a user-facing message; verified that every path returning false reports first -- the env-credential report, GetBaseURLOrAsk, both skip-auth-flow branches, and the two log.Error calls -- so returning ErrSilent cannot hide a failure. Verified: "dr --skip-auth auth login" now prints one line, not two. Left alone deliberately: cmd/self/update and cmd/self/completion never set SilenceErrors, so cobra printed their errors before and main prints them now -- no change in output. auth.WriteConfigFileSilent logs and returns the error, but its only command-level caller (auth/logout) already returns ErrSilent. And component/add plus component/update set SilenceErrors while never returning ErrSilent, so their errors were previously swallowed whole; centralizing fixed a second silent-failure case ("dr component add" outside a repo root now reports instead of exiting 1 in silence). Tests: EnsureAuthenticatedE returns ErrSilent on failure and nil on success; login RunE returns ErrSilent for the --skip-auth short circuit. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/auth/login/cmd.go | 9 ++++--- cmd/auth/login/cmd_test.go | 48 ++++++++++++++++++++++++++++++++++++++ internal/auth/auth.go | 12 +++++++--- internal/auth/auth_test.go | 46 ++++++++++++++++++++++++++++++++++++ 4 files changed, 107 insertions(+), 8 deletions(-) create mode 100644 cmd/auth/login/cmd_test.go diff --git a/cmd/auth/login/cmd.go b/cmd/auth/login/cmd.go index 156cde747..44ad7f30f 100644 --- a/cmd/auth/login/cmd.go +++ b/cmd/auth/login/cmd.go @@ -32,10 +32,9 @@ func RunE(cmd *cobra.Command, args []string) error { //nolint: cyclop // when authentication is intentionally disabled, say if the user is offline, or in // a CI/CD environment, or in a script. if viperx.GetBool(config.SkipAuthKey) { - err := errors.New("Login has been disabled via the '--skip-auth' flag.") - log.Error(err) + log.Error(errors.New("Login has been disabled via the '--skip-auth' flag.")) - return err + return cli.ErrSilent } var url string @@ -86,7 +85,7 @@ func RunE(cmd *cobra.Command, args []string) error { //nolint: cyclop cmd.SilenceUsage = true - return err + return cli.ErrSilent } if key == "" { @@ -101,7 +100,7 @@ func RunE(cmd *cobra.Command, args []string) error { //nolint: cyclop cmd.SilenceUsage = true - return err + return cli.ErrSilent } return nil diff --git a/cmd/auth/login/cmd_test.go b/cmd/auth/login/cmd_test.go new file mode 100644 index 000000000..a81761651 --- /dev/null +++ b/cmd/auth/login/cmd_test.go @@ -0,0 +1,48 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package login + +import ( + "testing" + + "github.com/datarobot/cli/internal/cli" + "github.com/datarobot/cli/internal/config" + "github.com/datarobot/cli/internal/config/viperx" + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestRunE_SkipAuthReturnsErrSilent covers the --skip-auth short circuit, the one +// login failure path that needs no network or config access. RunE logs the reason +// itself, so it must return cli.ErrSilent; a plain error would have main.go print +// the same thing again (CFX-6924). +func TestRunE_SkipAuthReturnsErrSilent(t *testing.T) { + viperx.Set(config.SkipAuthKey, true) + t.Cleanup(func() { viperx.Set(config.SkipAuthKey, false) }) + + err := RunE(&cobra.Command{}, nil) + + require.Error(t, err) + assert.ErrorIs(t, err, cli.ErrSilent, + "the reason is already logged, so the error must not be printed again") +} + +// TestCmd_SilenceErrors documents that the command still opts out of cobra's +// printing. Reporting is centralized in main.go via cmd.ReportError, which honours +// cli.ErrSilent; this flag keeps cobra from adding a line of its own. +func TestCmd_SilenceErrors(t *testing.T) { + assert.True(t, Cmd().SilenceErrors) +} diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 0ed525fd2..caae3e526 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -26,6 +26,7 @@ import ( "testing" "github.com/charmbracelet/lipgloss" + "github.com/datarobot/cli/internal/cli" "github.com/datarobot/cli/internal/config" "github.com/datarobot/cli/internal/config/viperx" "github.com/datarobot/cli/internal/log" @@ -309,11 +310,16 @@ func VerifyEnvCredentials(ctx context.Context) (*EnvCredentials, error) { // EnsureAuthenticatedE checks if valid authentication exists, and if not, // triggers the login flow automatically (see EnsureAuthenticated for the -// exceptions). Returns an error if authentication fails, suitable for use in -// Cobra PreRunE hooks. +// exceptions). Suitable for use in Cobra PreRunE hooks. +// +// On failure it returns cli.ErrSilent, not a descriptive error: EnsureAuthenticated +// has already told the user what went wrong, and main.go prints anything else. func EnsureAuthenticatedE(cmd *cobra.Command, _ []string) error { if !EnsureAuthenticated(cmd.Context()) { - return errors.New("authentication failed") + // Every path where EnsureAuthenticated reports false has already written a + // user-facing message, so signal that rather than returning a fresh error + // main.go would print a second time. + return cli.ErrSilent } return nil diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index ffe3e7109..0cdb4ec2c 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -26,9 +26,11 @@ import ( "path/filepath" "testing" + "github.com/datarobot/cli/internal/cli" "github.com/datarobot/cli/internal/config" "github.com/datarobot/cli/internal/config/viperx" "github.com/datarobot/cli/internal/testutil" + "github.com/spf13/cobra" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gopkg.in/yaml.v3" @@ -814,3 +816,47 @@ func TestVerifyEnvCredentials(t *testing.T) { assert.Equal(t, "valid-token", creds.Token) }) } + +// TestEnsureAuthenticatedE_ReturnsErrSilent locks in the reporting contract: +// EnsureAuthenticated writes a user-facing message on every failure path, so the +// PreRunE wrapper must return cli.ErrSilent rather than a fresh error. Returning a +// plain error would make main.go print a second line for something the user was +// already told about (CFX-6924). +func TestEnsureAuthenticatedE_ReturnsErrSilent(t *testing.T) { + server, cleanup := setupTestEnvironment(t) + defer cleanup() + + // A complete but invalid env pair fails without falling back or starting a + // login flow, and reports the reason itself. + t.Setenv("DATAROBOT_ENDPOINT", server.URL+"/api/v2") + t.Setenv("DATAROBOT_API_TOKEN", "expired-token") + + 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") + } + + cmd := &cobra.Command{} + cmd.SetContext(context.Background()) + + err := EnsureAuthenticatedE(cmd, nil) + + require.Error(t, err) + assert.ErrorIs(t, err, cli.ErrSilent, + "authentication failures are already reported, so the error must be silent") +} + +// TestEnsureAuthenticatedE_NilOnSuccess is the companion case: a valid stored +// profile must produce no error at all. +func TestEnsureAuthenticatedE_NilOnSuccess(t *testing.T) { + _, cleanup := setupTestEnvironment(t) + defer cleanup() + + viperx.Set(config.DataRobotAPIKey, "valid-token") + + cmd := &cobra.Command{} + cmd.SetContext(context.Background()) + + assert.NoError(t, EnsureAuthenticatedE(cmd, nil)) +} From f51b8133cff6214c0ec3bc54b689b49afa0a1f16 Mon Sep 17 00:00:00 2001 From: Volodymyr Onofriichuk Date: Wed, 19 Aug 2026 17:06:22 +0300 Subject: [PATCH 3/4] [CFX-6924] feat(tls): explain an empty Windows certificate store Reported while validating the export fix: when there is genuinely nothing to export, "no certificates found in Windows cert store" states the outcome without telling the user what to do about it. With the PSModulePath fix in place, that message now means what it says -- an unreachable store reports itself separately -- so an empty store is a machine-configuration problem the user can act on. ExportWindowsCerts returns a new ErrNoWindowsCerts sentinel for that case, and applyWindowsCerts pairs it with guidance: how to inspect both Root stores, how to import a missing organization CA, and the --ca-cert escape hatch. The call site reports the error itself and returns cli.ErrSilent rather than letting main.go print. Printing guidance at the call site while main printed the error afterwards put the help block above the message it explains; owning both keeps the order right, and ErrSilent suppresses the duplicate. Everything goes to stderr, so stdout stays parseable under --output-format json. The guidance helper lives in an untagged file so it is unit tested on every host, matching the psmodulepath helpers earlier in this branch. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/tls_help.go | 41 +++++++++++++++++++++++++++++++ cmd/tls_help_test.go | 49 +++++++++++++++++++++++++++++++++++++ cmd/tls_windows.go | 14 +++++++++++ internal/tls/tls_windows.go | 14 ++++++++--- 4 files changed, 114 insertions(+), 4 deletions(-) create mode 100644 cmd/tls_help.go create mode 100644 cmd/tls_help_test.go diff --git a/cmd/tls_help.go b/cmd/tls_help.go new file mode 100644 index 000000000..9c2f14240 --- /dev/null +++ b/cmd/tls_help.go @@ -0,0 +1,41 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package cmd + +import ( + "fmt" + "io" +) + +// fprintNoWindowsCertsHelp explains what to do when --export-windows-certs finds an +// empty certificate store. The export itself worked; there is simply nothing to +// export, which is a machine-configuration problem the user can resolve. +// +// Kept build-tag free so it can be unit tested on any host, although only the +// Windows build calls it. +func fprintNoWindowsCertsHelp(w io.Writer) { + fmt.Fprintln(w, "The Windows certificate store contains no Root or CA certificates to export.") + fmt.Fprintln(w, "") + fmt.Fprintln(w, " Check what is installed:") + fmt.Fprintln(w, " Get-ChildItem Cert:\\LocalMachine\\Root") + fmt.Fprintln(w, " Get-ChildItem Cert:\\CurrentUser\\Root") + fmt.Fprintln(w, " or open certlm.msc (machine) / certmgr.msc (user).") + fmt.Fprintln(w, "") + fmt.Fprintln(w, " If your organization's root CA is missing, import it and retry:") + fmt.Fprintln(w, " Import-Certificate -FilePath -CertStoreLocation Cert:\\CurrentUser\\Root") + fmt.Fprintln(w, "") + fmt.Fprintln(w, " To trust a PEM bundle directly instead of the Windows store:") + fmt.Fprintln(w, " dr --ca-cert ") +} diff --git a/cmd/tls_help_test.go b/cmd/tls_help_test.go new file mode 100644 index 000000000..879ff32ec --- /dev/null +++ b/cmd/tls_help_test.go @@ -0,0 +1,49 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package cmd + +import ( + "bytes" + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestFprintNoWindowsCertsHelp checks the guidance is actionable rather than merely +// restating the failure: it must name how to inspect the store, how to import a CA, +// and the --ca-cert escape hatch. +func TestFprintNoWindowsCertsHelp(t *testing.T) { + t.Parallel() + + var buf bytes.Buffer + + fprintNoWindowsCertsHelp(&buf) + + out := buf.String() + + for _, want := range []string{ + "no Root or CA certificates", + `Cert:\LocalMachine\Root`, + `Cert:\CurrentUser\Root`, + "certlm.msc", + "Import-Certificate", + "dr --ca-cert", + } { + assert.Contains(t, out, want) + } + + assert.True(t, strings.HasSuffix(out, "\n"), "guidance must end with a newline") +} diff --git a/cmd/tls_windows.go b/cmd/tls_windows.go index 670ce550d..56fa0dfd4 100644 --- a/cmd/tls_windows.go +++ b/cmd/tls_windows.go @@ -17,9 +17,12 @@ package cmd import ( + "errors" "fmt" + "os" "path/filepath" + "github.com/datarobot/cli/internal/cli" "github.com/datarobot/cli/internal/config" internaltls "github.com/datarobot/cli/internal/tls" "github.com/spf13/cobra" @@ -54,6 +57,17 @@ func applyWindowsCerts(cmd *cobra.Command, caCert *string) error { } if err := internaltls.ExportWindowsCerts(dest); err != nil { + // An empty store is fixable by the user, so report it and say how. Both go to + // stderr, keeping stdout parseable under --output-format json. Reporting here + // rather than letting main.go do it keeps the guidance below its own error + // message instead of above it; ErrSilent then suppresses the second print. + if errors.Is(err, internaltls.ErrNoWindowsCerts) { + ReportError(os.Stderr, fmt.Errorf("--export-windows-certs: %w", err)) + fprintNoWindowsCertsHelp(os.Stderr) + + return cli.ErrSilent + } + return fmt.Errorf("--export-windows-certs: %w", err) } diff --git a/internal/tls/tls_windows.go b/internal/tls/tls_windows.go index 80492b17f..0884e48e8 100644 --- a/internal/tls/tls_windows.go +++ b/internal/tls/tls_windows.go @@ -24,6 +24,15 @@ import ( "strings" ) +// ErrNoWindowsCerts reports that enumerating the certificate stores succeeded but +// found nothing. It is a sentinel so callers can offer guidance: an empty store is a +// machine-configuration problem the user can act on, unlike a failure to reach the +// store at all, which the script reports separately. +var ErrNoWindowsCerts = errors.New( + "no certificates found in the Windows certificate store: " + + "Root and CA are empty for both LocalMachine and CurrentUser", +) + // ExportWindowsCerts exports Root and CA certificates from the Windows // certificate store and writes them as a PEM bundle to dest. func ExportWindowsCerts(dest string) error { @@ -76,10 +85,7 @@ func ExportWindowsCerts(dest string) error { pem := strings.TrimSpace(string(out)) if pem == "" { - return errors.New( - "no certificates found in Windows cert store: the Root and CA stores are " + - "empty for both LocalMachine and CurrentUser", - ) + return ErrNoWindowsCerts } if err := os.WriteFile(dest, []byte(pem+"\n"), 0o600); err != nil { From c967dcb25037d031ff370541e077ff0173829e77 Mon Sep 17 00:00:00 2001 From: Volodymyr Onofriichuk Date: Wed, 19 Aug 2026 17:22:35 +0300 Subject: [PATCH 4/4] [CFX-6924] fix(tls): only claim the cert store is empty when it provably is Raised while reviewing the new guidance: on a machine whose stores clearly do contain certificates, telling the user to import a CA is exactly the wrong advice -- the same trap the original message set. Two residual paths could still produce a false "no certificates found": a restricted PowerShell language mode forbids the [Convert]::ToBase64String call, so enumeration succeeds while every encode fails and stdout comes back empty with exit code 0; and any other per-item encode failure behaves the same way. The script now refuses to run outside FullLanguage, naming the mode it found, and reports the enumerated count as a CERTCOUNT header before attempting any encoding. ErrNoWindowsCerts is returned only for CERTCOUNT=0. A non-zero count with no PEM body returns the new ErrCertEncodeFailed instead, which states how many certificates were seen -- so the advice can never contradict what the user sees in certmgr. Header parsing lives in an untagged file with table tests covering CRLF output, stray whitespace, a missing header, and the empty-versus-unencodable distinction, since that is where a regression would quietly restore the misleading message. Missing or unparseable headers are errors rather than a silent zero. Co-Authored-By: Claude Opus 5 (1M context) --- internal/tls/export_output.go | 70 ++++++++++++++++ internal/tls/export_output_test.go | 129 +++++++++++++++++++++++++++++ internal/tls/tls_windows.go | 30 ++++++- 3 files changed, 226 insertions(+), 3 deletions(-) create mode 100644 internal/tls/export_output.go create mode 100644 internal/tls/export_output_test.go diff --git a/internal/tls/export_output.go b/internal/tls/export_output.go new file mode 100644 index 000000000..7b7495aa3 --- /dev/null +++ b/internal/tls/export_output.go @@ -0,0 +1,70 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package tls + +import ( + "fmt" + "strconv" + "strings" +) + +// certCountPrefix marks the first stdout line of the export script, which reports how +// many certificates were enumerated before any encoding was attempted. +const certCountPrefix = "CERTCOUNT=" + +// No build constraint: only the Windows build calls these, but the parsing is where a +// regression would quietly bring back the misleading "no certificates found" message, +// so it is unit tested on every host. + +// parseExportOutput splits the export script's stdout into its CERTCOUNT header and +// the PEM body. The header is what lets the caller tell an empty store apart from a +// store it could not encode. +func parseExportOutput(raw string) (pem string, count int, err error) { + trimmed := strings.TrimSpace(raw) + + header, body, _ := strings.Cut(trimmed, "\n") + header = strings.TrimSpace(header) + + if !strings.HasPrefix(header, certCountPrefix) { + return "", 0, fmt.Errorf( + "exporting Windows cert store: unexpected output from powershell.exe: %s", + truncateForError(trimmed), + ) + } + + count, convErr := strconv.Atoi(strings.TrimPrefix(header, certCountPrefix)) + if convErr != nil { + return "", 0, fmt.Errorf( + "exporting Windows cert store: unreadable certificate count %q: %w", header, convErr, + ) + } + + return strings.TrimSpace(body), count, nil +} + +// truncateForError keeps unexpected subprocess output short enough to read in a +// terminal while still showing what came back. +func truncateForError(s string) string { + const maxLen = 200 + + s = strings.ReplaceAll(s, "\r\n", " ") + s = strings.ReplaceAll(s, "\n", " ") + + if len(s) > maxLen { + return strconv.Quote(s[:maxLen] + "…") + } + + return strconv.Quote(s) +} diff --git a/internal/tls/export_output_test.go b/internal/tls/export_output_test.go new file mode 100644 index 000000000..d7d56f7ad --- /dev/null +++ b/internal/tls/export_output_test.go @@ -0,0 +1,129 @@ +// Copyright 2026 DataRobot, Inc. and its affiliates. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package tls + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const oneCert = "-----BEGIN CERTIFICATE-----\nAAAA\n-----END CERTIFICATE-----" + +func TestParseExportOutput(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + raw string + wantCount int + wantPEM string + wantErr bool + }{ + { + name: "count and body", + raw: "CERTCOUNT=2\n" + oneCert, + wantCount: 2, + wantPEM: oneCert, + }, + { + name: "empty store reports zero with no body", + raw: "CERTCOUNT=0\n", + wantCount: 0, + wantPEM: "", + }, + { + name: "certificates enumerated but nothing encoded", + raw: "CERTCOUNT=7\n", + wantCount: 7, + wantPEM: "", + }, + { + name: "CRLF line endings from powershell.exe", + raw: "CERTCOUNT=1\r\n" + oneCert + "\r\n", + wantCount: 1, + wantPEM: oneCert, + }, + { + name: "surrounding whitespace is tolerated", + raw: "\n CERTCOUNT=3 \n" + oneCert + "\n\n", + wantCount: 3, + wantPEM: oneCert, + }, + { + name: "missing header is an error, not a silent zero", + raw: oneCert, + wantErr: true, + }, + { + name: "empty output is an error, not an empty store", + raw: "", + wantErr: true, + }, + { + name: "non-numeric count", + raw: "CERTCOUNT=lots\n" + oneCert, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + pem, count, err := parseExportOutput(tt.raw) + + if tt.wantErr { + require.Error(t, err) + + return + } + + require.NoError(t, err) + assert.Equal(t, tt.wantCount, count) + assert.Equal(t, tt.wantPEM, pem) + }) + } +} + +// TestParseExportOutputDistinguishesEmptyFromUnencodable is the case that motivated +// the header: an empty store must be told apart from a populated store whose +// certificates could not be encoded, because the advice for each is opposite. +func TestParseExportOutputDistinguishesEmptyFromUnencodable(t *testing.T) { + t.Parallel() + + _, empty, err := parseExportOutput("CERTCOUNT=0\n") + require.NoError(t, err) + + pem, populated, err := parseExportOutput("CERTCOUNT=31\n") + require.NoError(t, err) + + assert.Zero(t, empty, "an empty store must report zero") + assert.Equal(t, 31, populated, "a populated store must report its count even with no PEM body") + assert.Empty(t, pem) +} + +func TestTruncateForError(t *testing.T) { + t.Parallel() + + assert.Equal(t, `"short output"`, truncateForError("short output")) + assert.Equal(t, `"a b c"`, truncateForError("a\r\nb\nc"), "newlines collapse to spaces for one-line errors") + + long := truncateForError(strings.Repeat("x", 500)) + assert.Contains(t, long, "…") + assert.Less(t, len(long), 250, "long output must be truncated") +} diff --git a/internal/tls/tls_windows.go b/internal/tls/tls_windows.go index 0884e48e8..95eed7b33 100644 --- a/internal/tls/tls_windows.go +++ b/internal/tls/tls_windows.go @@ -33,6 +33,12 @@ var ErrNoWindowsCerts = errors.New( "Root and CA are empty for both LocalMachine and CurrentUser", ) +// ErrCertEncodeFailed reports that the stores did contain certificates but none +// could be encoded. Distinguishing this from ErrNoWindowsCerts matters: telling a +// user to import a CA when their store is already populated sends them down exactly +// the wrong path, which is how the original bug wasted so much time. +var ErrCertEncodeFailed = errors.New("could not encode any certificate from the Windows certificate store") + // ExportWindowsCerts exports Root and CA certificates from the Windows // certificate store and writes them as a PEM bundle to dest. func ExportWindowsCerts(dest string) error { @@ -43,19 +49,29 @@ func ExportWindowsCerts(dest string) error { `Import-Module Microsoft.PowerShell.Security -ErrorAction Stop`, `if (-not (Get-PSDrive -PSProvider Certificate -ErrorAction SilentlyContinue)) ` + `{ throw 'Cert: drive unavailable - Microsoft.PowerShell.Security did not load' }`, + // A restricted language mode forbids the [Convert] call below. Without this + // check the enumeration would succeed, every encode would fail, and the empty + // result would masquerade as an empty certificate store. + `if ($ExecutionContext.SessionState.LanguageMode -ne 'FullLanguage') ` + + `{ throw "PowerShell language mode is $($ExecutionContext.SessionState.LanguageMode), ` + + `which forbids certificate encoding; FullLanguage is required" }`, `$out = @()`, + `$total = 0`, `foreach ($store in 'Root','CA') {`, ` foreach ($loc in 'LocalMachine','CurrentUser') {`, // SilentlyContinue is kept here on purpose: an individual store may legitimately // be empty or absent. The precondition above distinguishes that from the whole // certificate provider being missing. - ` Get-ChildItem -Path "Cert:\$loc\$store" -ErrorAction SilentlyContinue | ForEach-Object {`, + ` $items = @(Get-ChildItem -Path "Cert:\$loc\$store" -ErrorAction SilentlyContinue)`, + ` $total += $items.Count`, + ` $items | ForEach-Object {`, ` $out += '-----BEGIN CERTIFICATE-----'`, ` $out += [Convert]::ToBase64String($_.RawData, 'InsertLineBreaks')`, ` $out += '-----END CERTIFICATE-----'`, ` }`, ` }`, `}`, + `"CERTCOUNT=$total"`, `$out -join [Environment]::NewLine`, }, "; ") @@ -82,12 +98,20 @@ func ExportWindowsCerts(dest string) error { return fmt.Errorf("exporting Windows cert store: %w", err) } - pem := strings.TrimSpace(string(out)) + pem, count, err := parseExportOutput(string(out)) + if err != nil { + return err + } - if pem == "" { + // Only claim the store is empty when the script actually enumerated nothing. + if count == 0 { return ErrNoWindowsCerts } + if pem == "" { + return fmt.Errorf("%w: enumerated %d certificate(s) but encoded none", ErrCertEncodeFailed, count) + } + if err := os.WriteFile(dest, []byte(pem+"\n"), 0o600); err != nil { return fmt.Errorf("writing CA bundle to %q: %w", dest, err) }