fix: unquote environment variable passed to hook commands

[zimeg]

Changelog

Fixes environment variable values set for hook commands to no longer be surrounded with double quotes.

Summary

This PR fixes an issue where environment variables were set with double quotes surrounding values:

slack-cli/internal/goutils/map.go

Lines 23 to 39 in 8056444

	// MapToStringSlice converts a map[string]string to a slice of strings with
	// elements key="value" from the map with an optional prefix
	func MapToStringSlice(args map[string]string, prefix string) []string {
	var res = []string{}
	for name, value := range args {
	if len(value) > 0 {
	var escapedValue string
	if runtime.GOOS == "windows" {
	escapedValue = strings.ReplaceAll(value, `"`, "`\"")
	} else {
	escapedValue = strings.ReplaceAll(value, `"`, `\"`)
	}
	res = append(res, fmt.Sprintf(`%s%s="%s"`, prefix, name, escapedValue))
	}
	}
	return res
	}

	// Env specifies the environment of the process.
	// Each entry is of the form "key=value".

📚 Reference: https://pkg.go.dev/os/exec#Cmd

Requirements

• I've read and understood the Contributing Guidelines and have done my best effort to follow them.
• I've read and agree to the Code of Conduct.

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.29%. Comparing base (ba1b784) to head (28ff7f1).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/pkg/platform/localserver.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #410      +/-   ##
==========================================
- Coverage   70.31%   70.29%   -0.02%     
==========================================
  Files         220      220              
  Lines       18482    18484       +2     
==========================================
- Hits        12996    12994       -2     
- Misses       4314     4316       +2     
- Partials     1172     1174       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[zimeg]

💌 Notes on code for the amazing reviewers! I am also planning to follow up with testing notes before asking for particular review 🧪 ✨

[zimeg]

🔭 note: These values are set for both the default and message-boundaries protocols:

slack-cli/internal/hooks/hook_executor_default.go

Line 70 in 8056444

cmd := opts.Exec.Command(cmdEnvVars, stdout, stderr, opts.Stdin, cmdArgs[0], cmdArgVars...)

slack-cli/internal/hooks/hook_executor_v2.go

Line 87 in 8056444

cmd := opts.Exec.Command(cmdEnvVars, &stdout, stderr, opts.Stdin, cmdArgs[0], cmdArgVars...)

slack-cli/internal/hooks/shell.go

Lines 56 to 59 in 8056444

	// Command creates a command ready to be run with the current processes shell
	func (sh ShellExec) Command(env []string, stdout io.Writer, stderr io.Writer, stdin io.Reader, name string, arg ...string) ShellCommand {
	cmd := sh.command(name, arg...)
	cmd.Env = env

[zimeg]

🏁 note: I understand the MapToStringSlice utilities to be useful in escaping command line arguments but we might not need this with the direct environment variable value setting in commands.

🏆 ramble: This implementation in localserver is the other find of a direct call to the hooks command executor so shares the change!

[zimeg]

🔬 More findings on the scope of this bug and confidence in a safe changes!

🐛 Hooks impacted

This PR changes how environment variables are set for both hook scripts and the run command for SDK managed connections. Our current hook implementations, however, don't make use of variables set from the CLI right now and so no problems were noticed here.

The run command for SDK managed connections had two environment variables set in hooks that can be used to demonstrate this error:

slack-cli/internal/pkg/platform/localserver.go

Lines 279 to 311 in e1bebf8

	// StartDelegate passes along required opts to SDK, delegating
	// connection for running app locally to script hook start
	func (r *LocalServer) StartDelegate(ctx context.Context) error {
	// Set up hook execution options
	var sdkManagedConnectionStartHookOpts = hooks.HookExecOpts{
	Env: map[string]string{
	"SLACK_CLI_XAPP": r.token,
	"SLACK_CLI_XOXB": r.localHostedContext.BotAccessToken,
	},
	Exec: hooks.ShellExec{},
	Hook: r.clients.SDKConfig.Hooks.Start,
	}
	// Check whether hook script is available
	if !r.clients.SDKConfig.Hooks.Start.IsAvailable() {
	return slackerror.New(slackerror.ErrSDKHookNotFound).WithMessage("The command for '%s' was not found", r.clients.SDKConfig.Hooks.Start.Name)
	}
	cmdStr, err := r.clients.SDKConfig.Hooks.Start.Get()
	if err != nil {
	return slackerror.New(slackerror.ErrSDKHookNotFound).WithRootCause(err)
	}
	// We're taking the script and separating it into individual fields to be compatible with Exec.Command,
	// then appending any additional arguments as flag --key=value pairs.
	cmdArgs := strings.Fields(cmdStr)
	var cmdArgVars = cmdArgs[1:] // omit the first item because that is the command name
	// Whatever cmd.Env is set to will be the ONLY environment variables that the `cmd` will have access to when it runs.
	// To avoid removing any environment variables that are set in the current environment, we first set the cmd.Env to the current environment.
	// before adding any new environment variables.
	var cmdEnvVars = os.Environ()
	cmdEnvVars = append(cmdEnvVars, goutils.MapToStringSlice(sdkManagedConnectionStartHookOpts.Env, "")...)
	cmd := sdkManagedConnectionStartHookOpts.Exec.Command(cmdEnvVars, os.Stdout, os.Stderr, nil, cmdArgs[0], cmdArgVars...)

$ slack create asdf -t slack-samples/bolt-js-starter-template
$ cd asdf
$ vim app.js

const app = new App({
  ...
  socketMode: true,
  appToken: process.env.SLACK_CLI_XAPP,  // Change this from SLACK_APP_TOKEN to SLACK_CLI_XAPP
});

$ slack run
...
[ERROR]  bolt-app Failed to start the app Error: An API error occurred: invalid_auth

With the changes of this branch, the token is passed as the expected string! The common SLACK_BOT_TOKEN used in templates avoided this issue because it's set using different logic:

slack-cli/internal/pkg/apps/install.go

Lines 844 to 873 in e1bebf8

	// setAppEnvironmentTokens adds the app and bot token to the process environment
	func setAppEnvironmentTokens(ctx context.Context, clients *shared.ClientFactory, result api.DeveloperAppInstallResult) error {
	if token, ok := clients.Os.LookupEnv("SLACK_APP_TOKEN"); !ok {
	if err := clients.Os.Setenv("SLACK_APP_TOKEN", result.APIAccessTokens.AppLevel); err != nil {
	return err
	}
	} else if token != result.APIAccessTokens.AppLevel {
	clients.IO.PrintWarning(ctx, "%s", style.Sectionf(style.TextSection{
	Text: fmt.Sprintf("The app token differs from the set %s environment variable", style.Highlight("SLACK_APP_TOKEN")),
	Secondary: []string{
	"The environment variable will continue to be used",
	"Proceed with caution as this might be associated to an unexpected app ID",
	},
	}))
	}
	if token, ok := clients.Os.LookupEnv("SLACK_BOT_TOKEN"); !ok {
	if err := clients.Os.Setenv("SLACK_BOT_TOKEN", result.APIAccessTokens.Bot); err != nil {
	return err
	}
	} else if token != result.APIAccessTokens.Bot {
	clients.IO.PrintWarning(ctx, "%s", style.Sectionf(style.TextSection{
	Text: fmt.Sprintf("The bot token differs from the set %s environment variable", style.Highlight("SLACK_BOT_TOKEN")),
	Secondary: []string{
	"The environment variable will continue to be used",
	"Proceed with caution as this might be associated to an unexpected bot ID",
	},
	}))
	}
	return nil
	}

The run command for Deno apps passes environment variables in context through the CLI managed connection:

slack-cli/internal/pkg/platform/run.go

Lines 103 to 130 in e1bebf8

	// Gather environment variables from an environment file
	variables, err := clients.Config.GetDotEnvFileVariables()
	if err != nil {
	return "", slackerror.Wrap(err, slackerror.ErrLocalAppRun).
	WithMessage("Failed to read the local .env file")
	}
	// Set SLACK_API_URL to the resolved host value found in the environment
	if value, ok := variables["SLACK_API_URL"]; ok {
	_ = clients.Os.Setenv("SLACK_API_URL", value)
	} else {
	variables["SLACK_API_URL"] = fmt.Sprintf("%s/api/", clients.Config.APIHostResolved)
	}
	var localHostedContext = LocalHostedContext{
	BotAccessToken: localInstallResult.APIAccessTokens.Bot,
	AppID: installedApp.AppID,
	TeamID: *authSession.TeamID,
	Variables: variables,
	}
	var server = LocalServer{
	clients: clients,
	token: localInstallResult.APIAccessTokens.AppLevel,
	localHostedContext: localHostedContext,
	cliConfig: cliConfig,
	Connection: nil,
	}

slack-cli/internal/pkg/platform/localserver.go

Lines 197 to 221 in e1bebf8

	var socketEvent = SocketEvent{
	Body: msg.Payload,
	Context: r.localHostedContext,
	}
	body, err := json.Marshal(socketEvent)
	if err != nil {
	errChan <- slackerror.Wrap(err, slackerror.ErrSocketConnection)
	return
	}
	_, err = r.clients.SDKConfig.Hooks.Start.Get()
	if err != nil {
	errChan <- err
	return
	}
	// Mimic the hosted app by executing the SDKs run command with the message as a param
	var startHookOpts = hooks.HookExecOpts{
	Hook: r.clients.SDKConfig.Hooks.Start,
	Stdin: bytes.NewBuffer(body),
	Stdout: r.clients.IO.WriteSecondary(r.clients.IO.WriteOut()),
	Stderr: r.clients.IO.WriteSecondary(r.clients.IO.WriteErr()),
	}
	out, err := r.clients.HookExecutor.Execute(ctx, startHookOpts)

Overall I'm feeling alright that this change is a fix for unnoticed bugs in our hook implementation and we'll match specs closer more 🤓

📣 Please do callout misunderstandings in these scopes! I'm hoping these findings can be useful in testing too!

[mwbrooks]

✅ Great catch on a real bug @zimeg! 🐛 🪤

🧪 Looking deeper into this, it's been a legit bug that has existed for quite a while.

📝 Left non-blocking suggestions and comments. Feel free to ignore them and hit merge!

[mwbrooks]

note: I think we want to keep it as Batman and Robin (the duo/sidekick to Batman), not Batman and Robinhood (two separate fictional characters).

[zimeg]

@mwbrooks I didn't want to take from the fun! 😉

I'll follow up with an update to add another example that includes a space elsewhere, which is the intention of initial change I meant!

[zimeg]

@mwbrooks A favorite LSP is added in 28ff7f1!

[zimeg]

@mwbrooks Thanks for reviewing this and searches prior! 🔍 ✨

I'll merge this with the kind improvements to tests. This unblocks a lot more for environment variables ahead that I'm excited for 🌲