Skip to content

Commit 6cde047

Browse files
lpcoxlpcox
authored
refactor: move ExpandEnvArgs and NormalizeScopeKind to correct packages (#2645)
Two functions were living in packages that don't reflect their concerns — Docker arg expansion in `mcp/` and scope normalization in `server/` — while their natural homes already existed elsewhere. ## Changes ### `ExpandEnvArgs` → `internal/config/docker_helpers.go` - Moved from `internal/mcp/dockerenv.go` (deleted) - Docker `-e VAR_NAME` expansion is Docker utility logic, not MCP protocol logic; `config/docker_helpers.go` already houses other Docker utilities - Updated call site in `mcp/connection.go` to `config.ExpandEnvArgs` - Tests migrated from `internal/mcp/dockerenv_test.go` (deleted) into `internal/config/docker_helpers_and_env_test.go` - `mcp/connection_test.go` updated to reference `config.ExpandEnvArgs` ### `normalizeScopeKind` → `internal/config/guard_policy.go` (exported as `NormalizeScopeKind`) - Moved from `internal/server/guard_init.go`, where it was the only guard-policy normalization function outside `config/` - Centralizes all policy normalization alongside `NormalizeGuardPolicy` and `normalizeAndValidateScopeArray` - Call site in `guard_init.go` updated to `config.NormalizeScopeKind` - Tests in `server/resolve_guard_policy_test.go` updated accordingly > [!WARNING] > > <details> > <summary>Firewall rules blocked me from connecting to one or more addresses (expand for details)</summary> > > #### I tried to connect to the following addresses, but was blocked by firewall rules: > > - `example.com` > - Triggering command: `/tmp/go-build3253343390/b334/launcher.test /tmp/go-build3253343390/b334/launcher.test -test.testlogfile=/tmp/go-build3253343390/b334/testlog.txt -test.paniconexit0 -test.timeout=10m0s s-th�� s-through=-lgcc_s .cfg 64/pkg/tool/linux_amd64/vet --gdwarf-5 --64` (dns block) > - `invalid-host-that-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build139304951/b221/config.test /tmp/go-build139304951/b221/config.test -test.testlogfile=/tmp/go-build139304951/b221/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build2095751038/b302/vet.cfg 64/src/runtime/c/tmp/go-build3935344048/b241/symabis 64/src/internal/-c=4 64/bin/as` (dns block) > - `nonexistent.local` > - Triggering command: `/tmp/go-build3253343390/b334/launcher.test /tmp/go-build3253343390/b334/launcher.test -test.testlogfile=/tmp/go-build3253343390/b334/testlog.txt -test.paniconexit0 -test.timeout=10m0s s-th�� s-through=-lgcc_s .cfg 64/pkg/tool/linux_amd64/vet --gdwarf-5 --64` (dns block) > - `slow.example.com` > - Triggering command: `/tmp/go-build3253343390/b334/launcher.test /tmp/go-build3253343390/b334/launcher.test -test.testlogfile=/tmp/go-build3253343390/b334/testlog.txt -test.paniconexit0 -test.timeout=10m0s s-th�� s-through=-lgcc_s .cfg 64/pkg/tool/linux_amd64/vet --gdwarf-5 --64` (dns block) > - `this-host-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build2095751038/b263/mcp.test /tmp/go-build2095751038/b263/mcp.test -test.testlogfile=/tmp/go-build2095751038/b263/testlog.txt -test.paniconexit0 -test.timeout=10m0s conf�� go --local sh committer.name` (dns block) > > If you need me to access, download, or install something from one of these locations, you can either: > > - Configure [Actions setup steps](https://gh.io/copilot/actions-setup-steps) to set up my environment, which run before the firewall is enabled > - Add the appropriate URLs or hosts to the custom allowlist in this repository's [Copilot coding agent settings](https://github.com/github/gh-aw-mcpg/settings/copilot/coding_agent) (admins only) > > </details> <!-- START COPILOT CODING AGENT TIPS --> --- 📱 Kick off Copilot coding agent tasks wherever you are with [GitHub Mobile](https://gh.io/cca-mobile-docs), available on iOS and Android.
2 parents f0fef19 + d11e5aa commit 6cde047

9 files changed

Lines changed: 212 additions & 222 deletions

‎internal/config/docker_helpers.go‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,12 @@ import (
66
"os/exec"
77
"regexp"
88
"strings"
9+
10+
"github.com/github/gh-aw-mcpg/internal/logger"
911
)
1012

13+
var logDockerHelpers = logger.New("config:docker_helpers")
14+
1115
// containerIDPattern validates that a container ID only contains valid characters (hex digits)
1216
// Container IDs are 64 character hex strings, but short form (12 chars) is also valid
1317
var containerIDPattern = regexp.MustCompile(`^[a-f0-9]{12,64}$`)
@@ -113,3 +117,33 @@ func checkLogDirMounted(containerID, logDir string) bool {
113117
// Check if the log directory is in the mounts
114118
return strings.Contains(output, logDir)
115119
}
120+
121+
// ExpandEnvArgs expands Docker -e flags that reference environment variables.
122+
// Converts "-e VAR_NAME" to "-e VAR_NAME=value" by reading from the process environment.
123+
func ExpandEnvArgs(args []string) []string {
124+
logDockerHelpers.Printf("Expanding env args: input_count=%d", len(args))
125+
result := make([]string, 0, len(args))
126+
for i := 0; i < len(args); i++ {
127+
arg := args[i]
128+
129+
// Check if this is a -e flag
130+
if arg == "-e" && i+1 < len(args) {
131+
nextArg := args[i+1]
132+
// If next arg doesn't contain '=', it's a variable reference
133+
if len(nextArg) > 0 && !strings.Contains(nextArg, "=") {
134+
// Look up the variable in the environment
135+
if value, exists := os.LookupEnv(nextArg); exists {
136+
logDockerHelpers.Printf("Expanding env var: name=%s", nextArg)
137+
result = append(result, "-e")
138+
result = append(result, fmt.Sprintf("%s=%s", nextArg, value))
139+
i++ // Skip the next arg since we processed it
140+
continue
141+
}
142+
logDockerHelpers.Printf("Env var not found in process environment: name=%s", nextArg)
143+
}
144+
}
145+
result = append(result, arg)
146+
}
147+
logDockerHelpers.Printf("Env args expansion complete: output_count=%d", len(result))
148+
return result
149+
}

‎internal/config/docker_helpers_and_env_test.go‎

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,150 @@ func TestValidateContainerID_SecurityCritical(t *testing.T) {
119119
}
120120
}
121121

122+
// TestExpandEnvArgs tests ExpandEnvArgs with various -e flag combinations.
123+
func TestExpandEnvArgs(t *testing.T) {
124+
tests := []struct {
125+
name string
126+
args []string
127+
envVars map[string]string
128+
want []string
129+
}{
130+
{
131+
name: "nil input returns empty slice",
132+
args: nil,
133+
want: []string{},
134+
},
135+
{
136+
name: "empty input returns empty slice",
137+
args: []string{},
138+
want: []string{},
139+
},
140+
{
141+
name: "args without -e flag pass through unchanged",
142+
args: []string{"run", "--rm", "-i", "ghcr.io/org/image:latest"},
143+
want: []string{"run", "--rm", "-i", "ghcr.io/org/image:latest"},
144+
},
145+
{
146+
name: "-e VAR_NAME is expanded when env var is set",
147+
args: []string{"-e", "MY_TOKEN"},
148+
envVars: map[string]string{"MY_TOKEN": "secret-value"},
149+
want: []string{"-e", "MY_TOKEN=secret-value"},
150+
},
151+
{
152+
name: "-e VAR_NAME is left unchanged when env var is not set",
153+
args: []string{"-e", "_DOCKERENV_TEST_TRULY_UNSET_XYZ999"},
154+
want: []string{"-e", "_DOCKERENV_TEST_TRULY_UNSET_XYZ999"},
155+
},
156+
{
157+
name: "-e VAR=VALUE (already has =) is passed through unchanged",
158+
args: []string{"-e", "MY_VAR=already-set"},
159+
envVars: map[string]string{"MY_VAR": "other-value"},
160+
want: []string{"-e", "MY_VAR=already-set"},
161+
},
162+
{
163+
name: "-e at end of args (no following value) is passed through",
164+
args: []string{"run", "-e"},
165+
want: []string{"run", "-e"},
166+
},
167+
{
168+
name: "multiple -e flags are all expanded",
169+
args: []string{"-e", "TOKEN_A", "-e", "TOKEN_B"},
170+
envVars: map[string]string{"TOKEN_A": "val-a", "TOKEN_B": "val-b"},
171+
want: []string{"-e", "TOKEN_A=val-a", "-e", "TOKEN_B=val-b"},
172+
},
173+
{
174+
name: "mix of set and unset env vars in -e flags",
175+
args: []string{"-e", "SET_VAR", "-e", "_DOCKERENV_TEST_TRULY_UNSET_XYZ999"},
176+
envVars: map[string]string{"SET_VAR": "value"},
177+
want: []string{"-e", "SET_VAR=value", "-e", "_DOCKERENV_TEST_TRULY_UNSET_XYZ999"},
178+
},
179+
{
180+
name: "realistic docker run command with env var expansion",
181+
args: []string{"run", "--rm", "-e", "GITHUB_PERSONAL_ACCESS_TOKEN", "-i", "ghcr.io/github/github-mcp-server:latest"},
182+
envVars: map[string]string{"GITHUB_PERSONAL_ACCESS_TOKEN": "ghp_abc123"},
183+
want: []string{"run", "--rm", "-e", "GITHUB_PERSONAL_ACCESS_TOKEN=ghp_abc123", "-i", "ghcr.io/github/github-mcp-server:latest"},
184+
},
185+
{
186+
name: "-e VAR where VAR is empty string is passed through unchanged",
187+
args: []string{"-e", ""},
188+
want: []string{"-e", ""},
189+
},
190+
{
191+
name: "env var with empty value is expanded with empty value",
192+
args: []string{"-e", "EMPTY_VAR"},
193+
envVars: map[string]string{"EMPTY_VAR": ""},
194+
want: []string{"-e", "EMPTY_VAR="},
195+
},
196+
{
197+
name: "env var value containing = is expanded correctly",
198+
args: []string{"-e", "URL_VAR"},
199+
envVars: map[string]string{"URL_VAR": "https://example.com?key=val"},
200+
want: []string{"-e", "URL_VAR=https://example.com?key=val"},
201+
},
202+
{
203+
name: "non -e flags between expanded flags are preserved",
204+
args: []string{"-e", "VAR_A", "--name", "mycontainer", "-e", "VAR_B"},
205+
envVars: map[string]string{"VAR_A": "alpha", "VAR_B": "beta"},
206+
want: []string{"-e", "VAR_A=alpha", "--name", "mycontainer", "-e", "VAR_B=beta"},
207+
},
208+
{
209+
name: "same var referenced multiple times is expanded each time",
210+
args: []string{"-e", "SHARED_VAR", "-e", "SHARED_VAR"},
211+
envVars: map[string]string{"SHARED_VAR": "shared"},
212+
want: []string{"-e", "SHARED_VAR=shared", "-e", "SHARED_VAR=shared"},
213+
},
214+
{
215+
name: "-e immediately followed by another -e flag where first -e var is unset",
216+
args: []string{"-e", "-e"},
217+
// The first "-e" tries to expand the second "-e" as a var name.
218+
// Since no env var named "-e" is set, the expansion is skipped.
219+
// Both "-e" args are emitted as-is.
220+
want: []string{"-e", "-e"},
221+
},
222+
}
223+
224+
for _, tt := range tests {
225+
t.Run(tt.name, func(t *testing.T) {
226+
for k, v := range tt.envVars {
227+
t.Setenv(k, v)
228+
}
229+
230+
got := ExpandEnvArgs(tt.args)
231+
232+
require.NotNil(t, got, "ExpandEnvArgs should never return nil")
233+
assert.Equal(t, tt.want, got)
234+
})
235+
}
236+
}
237+
238+
// TestExpandEnvArgs_DoesNotMutateInput verifies that the original args slice
239+
// is not modified by ExpandEnvArgs.
240+
func TestExpandEnvArgs_DoesNotMutateInput(t *testing.T) {
241+
t.Setenv("MY_SECRET", "secret-value")
242+
243+
original := []string{"-e", "MY_SECRET", "--rm"}
244+
// Make a copy to compare against after the call
245+
copyOfOriginal := make([]string, len(original))
246+
copy(copyOfOriginal, original)
247+
248+
ExpandEnvArgs(original)
249+
250+
assert.Equal(t, copyOfOriginal, original, "ExpandEnvArgs must not mutate the input slice")
251+
}
252+
253+
// TestExpandEnvArgs_OutputIsIndependentOfInput verifies that modifications to
254+
// the returned slice do not affect the original input.
255+
func TestExpandEnvArgs_OutputIsIndependentOfInput(t *testing.T) {
256+
t.Setenv("SOME_VAR", "value")
257+
258+
args := []string{"run", "-e", "SOME_VAR"}
259+
result := ExpandEnvArgs(args)
260+
261+
// Modifying the result should not affect the original
262+
result[0] = "MODIFIED"
263+
assert.Equal(t, "run", args[0], "Modifying result should not affect original slice")
264+
}
265+
122266
// TestGetGatewayPortFromEnv tests the env-based gateway port parsing.
123267
func TestGetGatewayPortFromEnv_Comprehensive(t *testing.T) {
124268
tests := []struct {

‎internal/config/guard_policy.go‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -700,3 +700,22 @@ func validateGuardPolicies(cfg *Config) error {
700700
}
701701
return nil
702702
}
703+
704+
// NormalizeScopeKind returns a copy of the policy map with the scope_kind field
705+
// normalized to lowercase trimmed string form. Other fields are preserved as-is.
706+
func NormalizeScopeKind(policy map[string]interface{}) map[string]interface{} {
707+
if policy == nil {
708+
return nil
709+
}
710+
711+
normalized := make(map[string]interface{}, len(policy))
712+
for key, value := range policy {
713+
normalized[key] = value
714+
}
715+
716+
if scopeKind, ok := normalized["scope_kind"].(string); ok {
717+
normalized["scope_kind"] = strings.ToLower(strings.TrimSpace(scopeKind))
718+
}
719+
720+
return normalized
721+
}

‎internal/mcp/connection.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"sync"
1515
"time"
1616

17+
"github.com/github/gh-aw-mcpg/internal/config"
1718
"github.com/github/gh-aw-mcpg/internal/difc"
1819
"github.com/github/gh-aw-mcpg/internal/logger"
1920
"github.com/github/gh-aw-mcpg/internal/logger/sanitize"
@@ -105,7 +106,7 @@ func NewConnection(ctx context.Context, serverID, command string, args []string,
105106

106107
// Expand Docker -e flags that reference environment variables
107108
// Docker's `-e VAR_NAME` expects VAR_NAME to be in the environment
108-
expandedArgs := ExpandEnvArgs(args)
109+
expandedArgs := config.ExpandEnvArgs(args)
109110
logConn.Printf("Expanded args for Docker env: %v", sanitize.SanitizeArgs(expandedArgs))
110111

111112
// Create command transport

‎internal/mcp/connection_test.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"strings"
1111
"testing"
1212

13+
"github.com/github/gh-aw-mcpg/internal/config"
1314
"github.com/github/gh-aw-mcpg/internal/difc"
1415
"github.com/github/gh-aw-mcpg/internal/logger"
1516
"github.com/stretchr/testify/assert"
@@ -256,7 +257,7 @@ func TestExpandDockerEnvArgs(t *testing.T) {
256257
}
257258
})
258259

259-
result := ExpandEnvArgs(tt.args)
260+
result := config.ExpandEnvArgs(tt.args)
260261
assert.Equal(t, tt.expected, result)
261262
})
262263
}

‎internal/mcp/dockerenv.go‎

Lines changed: 0 additions & 41 deletions
This file was deleted.

0 commit comments

Comments
 (0)