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

Open
zimeg wants to merge 6 commits into
base: main
Choose a base branch
from
+16 −13
Open

fix: unquote environment variable passed to hook commands #410

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 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
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
8 changes: 4 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,8 @@ 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 hood",
"YIN": "yang",
},
Exec: &MockExec{
mockCommand: &MockCommand{
Expand All @@ -74,8 +74,8 @@ 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 hood`)
require.Contains(t, opts.Exec.(*MockExec).mockCommand.Env, `YIN=yang`)
},
},
"failed execution": {
Expand Down
12 changes: 6 additions & 6 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,8 @@ 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 hood",
"YIN": "yang",
},
Exec: &MockExec{
mockCommand: &MockCommand{
Expand All @@ -68,8 +68,8 @@ 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 hood`)
require.Contains(t, mockExec.(*MockExec).mockCommand.Env, `YIN=yang`)
},
},
"successful execution with payload > 64kb": {
Expand All @@ -89,8 +89,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