From 8d56e1b024cb12f58beac0d3fea204d1f3644b02 Mon Sep 17 00:00:00 2001
From: Josh Berry <josh.berry@temporal.io>
Date: Tue, 16 Jul 2024 07:04:41 -0700
Subject: [PATCH] Fix nil deref if the env file can't be parsed (#625)

Closes #605.
---
 temporalcli/commands.go | 62 ++++++++++++++++++++++++++++-------------
 1 file changed, 42 insertions(+), 20 deletions(-)

diff --git a/temporalcli/commands.go b/temporalcli/commands.go
index c5131420..0cf665b7 100644
--- a/temporalcli/commands.go
+++ b/temporalcli/commands.go
@@ -77,10 +77,17 @@ type CommandOptions struct {
 	AdditionalClientGRPCDialOptions []grpc.DialOption
 }
 
+// NewCommandContext creates a CommandContext for use by the rest of the CLI.
+// Among other things, this parses the env config file and modifies
+// options/flags according to the parameters set there.
+//
+// A CommandContext and CancelFunc are always returned, even in the event of an
+// error; this is so the CommandContext can be used to print an appropriate
+// error message.
 func NewCommandContext(ctx context.Context, options CommandOptions) (*CommandContext, context.CancelFunc, error) {
 	cctx := &CommandContext{Context: ctx, Options: options}
 	if err := cctx.preprocessOptions(); err != nil {
-		return nil, nil, err
+		return cctx, func(){}, err
 	}
 
 	// Setup interrupt handler
@@ -109,6 +116,28 @@ func (c *CommandContext) preprocessOptions() error {
 		c.Options.Stderr = os.Stderr
 	}
 
+	// Setup default fail callback
+	if c.Options.Fail == nil {
+		c.Options.Fail = func(err error) {
+			// If context is closed, say that the program was interrupted and ignore
+			// the actual error
+			if c.Err() != nil {
+				err = fmt.Errorf("program interrupted")
+			}
+			if c.Logger != nil {
+				c.Logger.Error(err.Error())
+			} else {
+				fmt.Fprintln(os.Stderr, err)
+			}
+			os.Exit(1)
+		}
+	}
+
+	// Update options according to the env file. MUST BE DONE LAST.
+	//
+	// Why last?  Callers need the CommandContext to be usable no matter what,
+	// because they rely on it to print errors even if env parsing fails.  In
+	// that situation, we will return both the CommandContext AND an error.
 	if !c.Options.DisableEnvConfig {
 		if c.Options.EnvConfigFile == "" {
 			// Default to --env-file, prefetched from CLI args
@@ -145,22 +174,6 @@ func (c *CommandContext) preprocessOptions() error {
 		}
 	}
 
-	// Setup default fail callback
-	if c.Options.Fail == nil {
-		c.Options.Fail = func(err error) {
-			// If context is closed, say that the program was interrupted and ignore
-			// the actual error
-			if c.Err() != nil {
-				err = fmt.Errorf("program interrupted")
-			}
-			if c.Logger != nil {
-				c.Logger.Error(err.Error())
-			} else {
-				fmt.Fprintln(os.Stderr, err)
-			}
-			os.Exit(1)
-		}
-	}
 	return nil
 }
 
@@ -314,17 +327,26 @@ func (c *CommandContext) promptString(message string, expected string, autoConfi
 // intentionally does not return an error but rather invokes Fail on the
 // options.
 func Execute(ctx context.Context, options CommandOptions) {
-	// Create context and run
+	// Create context and run. We always get a context and cancel func back even
+	// if an error was returned. This is so we can use the context to print an
+	// error message using the appropriate Fail() method, regardless of why the
+	// failure occurred.
+	//
+	// (In most cases, an error here likely means a problem with the user's env
+	// config file, or some other issue in their environment.)
 	cctx, cancel, err := NewCommandContext(ctx, options)
+	defer cancel()
+
 	if err == nil {
-		defer cancel()
+		// We have a context; let's actually run the command.
 		cmd := NewTemporalCommand(cctx)
 		cmd.Command.SetArgs(cctx.Options.Args)
 		err = cmd.Command.ExecuteContext(cctx)
 	}
 
-	// Use failure handler, but can still return
 	if err != nil {
+		// Either we failed to create the context, OR the command itself failed.
+		// Either way, we need to print an error message.
 		cctx.Options.Fail(err)
 	}