Skip to content

fix: unquote environment variable passed to hook commands #410

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zimeg merged 9 commits into from
Mar 23, 2026
+22 −15
Merged

fix: unquote environment variable passed to hook commands #410

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5bd98a8
fix: unquote environment variable passed to hook commands
zimeg Mar 18, 2026
5b2cf70
Merge branch 'main' into zimeg-fix-env-quote
zimeg Mar 18, 2026
cc1ce45
Merge branch 'main' into zimeg-fix-env-quote
zimeg Mar 19, 2026
228907d
Merge branch 'main' into zimeg-fix-env-quote
zimeg Mar 19, 2026
d942d79
Merge branch 'zimeg-fix-env-quote' of github.com:slackapi/slack-cli i…
zimeg Mar 19, 2026
5122e45
Merge branch 'main' into zimeg-fix-env-quote
zimeg Mar 19, 2026
5eb7472
Merge branch 'main' into zimeg-fix-env-quote
zimeg Mar 23, 2026
d2f878b
test: continue preference to uppercase keys for environment
zimeg Mar 23, 2026
28ff7f1
test: confirm variables with space still arrives
zimeg Mar 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions internal/hooks/hook_executor_default_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,9 @@ func Test_Hook_Execute_Default_Protocol(t *testing.T) {
opts: HookExecOpts{
Hook: HookScript{Name: "happypath", Command: "echo {}"},
Env: map[string]string{
"batman": "robin",
"yin": "yang",
"BATMAN": "robin",
"WHOAMI": "lumpy space princess",
"YIN": "yang",
},
Exec: &MockExec{
mockCommand: &MockCommand{
Expand All @@ -74,8 +75,9 @@ func Test_Hook_Execute_Default_Protocol(t *testing.T) {
response, err := executor.Execute(ctx, opts)
require.Equal(t, "test output", response)
require.Equal(t, nil, err)
require.Contains(t, opts.Exec.(*MockExec).mockCommand.Env, `batman="robin"`)
require.Contains(t, opts.Exec.(*MockExec).mockCommand.Env, `yin="yang"`)
require.Contains(t, opts.Exec.(*MockExec).mockCommand.Env, `BATMAN=robin`)
require.Contains(t, opts.Exec.(*MockExec).mockCommand.Env, `WHOAMI=lumpy space princess`)
require.Contains(t, opts.Exec.(*MockExec).mockCommand.Env, `YIN=yang`)
},
},
"failed execution": {
Expand Down
18 changes: 10 additions & 8 deletions internal/hooks/hook_executor_v2_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,9 @@ func Test_Hook_Execute_V2_Protocol(t *testing.T) {
opts: HookExecOpts{
Hook: HookScript{Name: "happypath", Command: "echo {}"},
Env: map[string]string{
"batman": "robin",
"yin": "yang",
"BATMAN": "robin",
"WHOAMI": "lumpy space princess",
"YIN": "yang",
},
Exec: &MockExec{
mockCommand: &MockCommand{
Expand All @@ -68,16 +69,17 @@ func Test_Hook_Execute_V2_Protocol(t *testing.T) {
check: func(t *testing.T, response string, err error, mockExec ExecInterface) {
require.Equal(t, `{"message": "hello world"}`, response)
require.Equal(t, nil, err)
require.Contains(t, mockExec.(*MockExec).mockCommand.Env, `batman="robin"`)
require.Contains(t, mockExec.(*MockExec).mockCommand.Env, `yin="yang"`)
require.Contains(t, mockExec.(*MockExec).mockCommand.Env, `BATMAN=robin`)
require.Contains(t, mockExec.(*MockExec).mockCommand.Env, `WHOAMI=lumpy space princess`)
require.Contains(t, mockExec.(*MockExec).mockCommand.Env, `YIN=yang`)
},
},
"successful execution with payload > 64kb": {
opts: HookExecOpts{
Hook: HookScript{Name: "happypath", Command: "echo {}"},
Env: map[string]string{
"batman": "robin",
"yin": "yang",
"BATMAN": "robin",
"YIN": "yang",
},
Exec: &MockExec{
mockCommand: &MockCommand{
Expand All @@ -89,8 +91,8 @@ func Test_Hook_Execute_V2_Protocol(t *testing.T) {
check: func(t *testing.T, response string, err error, mockExec ExecInterface) {
require.Equal(t, sixtyFourKBString, response)
require.Equal(t, nil, err)
require.Contains(t, mockExec.(*MockExec).mockCommand.Env, `batman="robin"`)
require.Contains(t, mockExec.(*MockExec).mockCommand.Env, `yin="yang"`)
require.Contains(t, mockExec.(*MockExec).mockCommand.Env, `BATMAN=robin`)
require.Contains(t, mockExec.(*MockExec).mockCommand.Env, `YIN=yang`)
},
},
"successful execution with payload > 512kb": {
Expand Down
4 changes: 3 additions & 1 deletion internal/hooks/hooks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,9 @@ func processExecOpts(opts HookExecOpts) ([]string, []string, []string, error) {
// 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(opts.Env, "")...)
for name, value := range opts.Env {
cmdEnvVars = append(cmdEnvVars, name+"="+value)
Copy link
Member Author

@zimeg zimeg Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔭 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

}

return cmdArgs, cmdArgVars, cmdEnvVars, nil
}
5 changes: 3 additions & 2 deletions internal/pkg/platform/localserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import (
"github.com/gorilla/websocket"
"github.com/radovskyb/watcher"
"github.com/slackapi/slack-cli/internal/config"
"github.com/slackapi/slack-cli/internal/goutils"
"github.com/slackapi/slack-cli/internal/hooks"
"github.com/slackapi/slack-cli/internal/iostreams"
"github.com/slackapi/slack-cli/internal/pkg/apps"
Expand Down Expand Up @@ -307,7 +306,9 @@ func (r *LocalServer) StartDelegate(ctx context.Context) error {
// 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, "")...)
Copy link
Member Author

@zimeg zimeg Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🏁 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!

for name, value := range sdkManagedConnectionStartHookOpts.Env {
cmdEnvVars = append(cmdEnvVars, name+"="+value)
}
cmd := sdkManagedConnectionStartHookOpts.Exec.Command(cmdEnvVars, os.Stdout, os.Stderr, nil, cmdArgs[0], cmdArgVars...)

// Store command reference for lifecycle management
Expand Down
Loading