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
Merged

fix: unquote environment variable passed to hook commands #410

Show file tree
Hide file tree
Changes from 2 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
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",
Copy link
Copy Markdown
Member

@mwbrooks mwbrooks Mar 23, 2026

Choose a reason for hiding this comment

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

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).

Copy link
Copy Markdown
Member Author

@zimeg zimeg Mar 23, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@zimeg zimeg Mar 23, 2026

Choose a reason for hiding this comment

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

@mwbrooks A favorite LSP is added in 28ff7f1!

},
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`)
Comment thread
zimeg marked this conversation as resolved.
Outdated
},
},
"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
Copy Markdown
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
Copy Markdown
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