Skip to content

Commit 7f2802b

Browse files
committed
fix(container): address copilot review feedback and gofumpt 0.10 reformat
- Sort known profiles in ProfileFromName error message for stable output. - Validate AgentProfile.EnvFileRelPath before interpolating into the shell snippet to block command injection via dynamic profiles. - Gate the exit-137 swallow in DockerManager.WireMCPGateway to the openclaw profile so a real OOM under hermes is not masked. - gofumpt -w across files unchanged on main; CI pinned to gofumpt@latest pulled in v0.10.0 with stricter rules.
1 parent 3987ec6 commit 7f2802b

22 files changed

Lines changed: 225 additions & 62 deletions

cmd/sluice/mcp_test.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -648,7 +648,8 @@ func TestHandleMCPGatewayNoUpstreams(t *testing.T) {
648648
return
649649
}
650650
cmd := exec.Command(os.Args[0], "-test.run=TestHandleMCPGatewayNoUpstreams")
651-
cmd.Env = append(os.Environ(),
651+
cmd.Env = append(
652+
os.Environ(),
652653
"TEST_MCP_SUBPROCESS=no_upstreams",
653654
"TEST_DB_PATH="+dbPath,
654655
"TELEGRAM_BOT_TOKEN=",
@@ -686,7 +687,8 @@ func TestHandleMCPGatewayInvalidChatID(t *testing.T) {
686687
return
687688
}
688689
cmd := exec.Command(os.Args[0], "-test.run=TestHandleMCPGatewayInvalidChatID")
689-
cmd.Env = append(os.Environ(),
690+
cmd.Env = append(
691+
os.Environ(),
690692
"TEST_MCP_SUBPROCESS=invalid_chat_id",
691693
"TEST_DB_PATH="+dbPath,
692694
)

cmd/sluice/policy.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -261,7 +261,8 @@ func handlePolicyImport(args []string) error {
261261
return fmt.Errorf("import: %w", err)
262262
}
263263

264-
fmt.Printf("imported: %d rules (%d skipped), %d bindings (%d skipped), %d upstreams (%d skipped), %d config\n",
264+
fmt.Printf(
265+
"imported: %d rules (%d skipped), %d bindings (%d skipped), %d upstreams (%d skipped), %d config\n",
265266
result.RulesInserted, result.RulesSkipped,
266267
result.BindingsInserted, result.BindingsSkipped,
267268
result.UpstreamsInserted, result.UpstreamsSkipped,

e2e/apple_test.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -287,7 +287,8 @@ func (e *appleEnv) startTun2proxy() {
287287
e.t.Skipf("tun2proxy not in PATH: %v", err)
288288
}
289289

290-
cmd := exec.Command(tun2proxyBin,
290+
cmd := exec.Command(
291+
tun2proxyBin,
291292
"--proxy", "socks5://"+e.sluice.ProxyAddr,
292293
"--tun", e.tunIface,
293294
)
@@ -655,7 +656,8 @@ func TestAppleContainerSluiceStartsWithRuntimeFlag(t *testing.T) {
655656
ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
656657
defer cancel()
657658

658-
cmd := exec.CommandContext(ctx, binary,
659+
cmd := exec.CommandContext(
660+
ctx, binary,
659661
"--runtime", "apple",
660662
"--listen", fmt.Sprintf("127.0.0.1:%d", proxyPort),
661663
"--db", dbPath,

e2e/credential_test.go

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -278,7 +278,8 @@ func TestCredential_HeaderInjection(t *testing.T) {
278278
_, port := mustSplitAddr(t, echo.URL)
279279

280280
// Add credential with binding for the echo server.
281-
runCredAdd(t, setup.Proc, "test_api_key", "real-secret-value-123",
281+
runCredAdd(
282+
t, setup.Proc, "test_api_key", "real-secret-value-123",
282283
"--destination", "127.0.0.1",
283284
"--ports", port,
284285
"--header", "X-Api-Key",
@@ -308,7 +309,8 @@ func TestCredential_PhantomInBody(t *testing.T) {
308309
echo := startTLSEchoServerWithCA(t, setup.CA)
309310
_, port := mustSplitAddr(t, echo.URL)
310311

311-
runCredAdd(t, setup.Proc, "body_cred", "body-secret-42",
312+
runCredAdd(
313+
t, setup.Proc, "body_cred", "body-secret-42",
312314
"--destination", "127.0.0.1",
313315
"--ports", port,
314316
"--header", "Authorization",
@@ -352,7 +354,8 @@ func TestCredential_UnboundPhantomStripped(t *testing.T) {
352354
_, unboundPort := mustSplitAddr(t, unboundEcho.URL)
353355

354356
// Add credential bound to the first echo server only.
355-
runCredAdd(t, setup.Proc, "bound_cred", "bound-secret",
357+
runCredAdd(
358+
t, setup.Proc, "bound_cred", "bound-secret",
356359
"--destination", "127.0.0.1",
357360
"--ports", boundPort,
358361
"--header", "X-Cred",
@@ -480,7 +483,8 @@ func TestCredential_Rotation(t *testing.T) {
480483
_, port := mustSplitAddr(t, echo.URL)
481484

482485
// Add initial credential.
483-
runCredAdd(t, setup.Proc, "rotate_key", "original-value",
486+
runCredAdd(
487+
t, setup.Proc, "rotate_key", "original-value",
484488
"--destination", "127.0.0.1",
485489
"--ports", port,
486490
"--header", "X-Api-Key",
@@ -524,14 +528,16 @@ func TestCredential_MultipleDestinations(t *testing.T) {
524528
_, portB := mustSplitAddr(t, echoB.URL)
525529

526530
// Add credential A bound to echo server A.
527-
runCredAdd(t, setup.Proc, "cred_a", "secret-a",
531+
runCredAdd(
532+
t, setup.Proc, "cred_a", "secret-a",
528533
"--destination", "127.0.0.1",
529534
"--ports", portA,
530535
"--header", "X-Key-A",
531536
)
532537

533538
// Add credential B bound to echo server B.
534-
runCredAdd(t, setup.Proc, "cred_b", "secret-b",
539+
runCredAdd(
540+
t, setup.Proc, "cred_b", "secret-b",
535541
"--destination", "127.0.0.1",
536542
"--ports", portB,
537543
"--header", "X-Key-B",

e2e/grpc_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,8 @@ func TestGRPC_CredentialInjectionInMetadata(t *testing.T) {
174174
_, port := splitHostPort(t, h2Addr)
175175

176176
// Add credential with binding for the H2 server.
177-
runCredAdd(t, setup.Proc, "grpc_token", "grpc-real-secret-456",
177+
runCredAdd(
178+
t, setup.Proc, "grpc_token", "grpc-real-secret-456",
178179
"--destination", "127.0.0.1",
179180
"--ports", port,
180181
"--header", "Authorization",

e2e/helpers_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -589,7 +589,8 @@ func sluiceWithWebhook(t *testing.T, policyTOML, webhookURL string) *SluiceProce
589589
}
590590

591591
// Add the HTTP webhook channel to the pre-seeded DB.
592-
channelCmd := exec.Command(binary, "channel", "add",
592+
channelCmd := exec.Command(
593+
binary, "channel", "add",
593594
"--type", "http",
594595
"--url", webhookURL,
595596
"--db", dbPath,

e2e/websocket_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,8 @@ func TestWebSocket_CredentialInjectionInUpgradeHeaders(t *testing.T) {
209209
_, port := splitHostPort(t, wsAddr)
210210

211211
// Add credential bound to the WS echo server.
212-
runCredAdd(t, setup.Proc, "ws_api_key", "ws-real-secret-789",
212+
runCredAdd(
213+
t, setup.Proc, "ws_api_key", "ws-real-secret-789",
213214
"--destination", "127.0.0.1",
214215
"--ports", port,
215216
"--header", "X-Ws-Key",

internal/channel/channel_test.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -397,7 +397,8 @@ func TestBrokerPendingLimitZeroMeansUnlimited(t *testing.T) {
397397
}()
398398
}
399399

400-
broker = NewBroker([]Channel{ch1},
400+
broker = NewBroker(
401+
[]Channel{ch1},
401402
WithMaxPending(0),
402403
WithDestinationRateLimit(0, 0),
403404
)
@@ -429,7 +430,8 @@ func TestBrokerDestinationRateLimiting(t *testing.T) {
429430
}()
430431
}
431432

432-
broker = NewBroker([]Channel{ch1},
433+
broker = NewBroker(
434+
[]Channel{ch1},
433435
WithMaxPending(0),
434436
WithDestinationRateLimit(3, time.Minute),
435437
)
@@ -477,7 +479,8 @@ func TestBrokerDestinationRateLimitDisabled(t *testing.T) {
477479
}()
478480
}
479481

480-
broker = NewBroker([]Channel{ch1},
482+
broker = NewBroker(
483+
[]Channel{ch1},
481484
WithMaxPending(0),
482485
WithDestinationRateLimit(0, 0),
483486
)

internal/container/agent_profile.go

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@ package container
22

33
import (
44
"fmt"
5+
"path"
6+
"sort"
57
"strings"
68
)
79

@@ -110,9 +112,48 @@ func ProfileFromName(name string) (*AgentProfile, error) {
110112
for k := range builtinProfiles {
111113
known = append(known, k)
112114
}
115+
sort.Strings(known)
113116
return nil, fmt.Errorf("unknown agent profile %q (known: %s)", name, strings.Join(known, ", "))
114117
}
115118

119+
// validateEnvFileRelPath ensures the env file path declared by an
120+
// AgentProfile is safe to interpolate into a shell snippet inside
121+
// double quotes. The path must be relative (no leading "/"), must
122+
// not contain ".." segments, and must not contain shell metacharacters
123+
// or whitespace that would let a maliciously constructed profile run
124+
// arbitrary commands when BuildEnvInjectionScriptForProfile is called.
125+
//
126+
// All built-in profiles in this package are constants and pass this
127+
// check trivially. The validation exists to keep the surface safe if
128+
// a future caller in this internal package constructs an AgentProfile
129+
// dynamically.
130+
func validateEnvFileRelPath(p string) error {
131+
if p == "" {
132+
return fmt.Errorf("EnvFileRelPath is empty")
133+
}
134+
if strings.HasPrefix(p, "/") {
135+
return fmt.Errorf("EnvFileRelPath %q must be relative (not absolute)", p)
136+
}
137+
cleaned := path.Clean(p)
138+
if cleaned != p {
139+
return fmt.Errorf("EnvFileRelPath %q is not in canonical form (got %q)", p, cleaned)
140+
}
141+
if strings.HasPrefix(cleaned, "../") || cleaned == ".." || strings.Contains(cleaned, "/../") {
142+
return fmt.Errorf("EnvFileRelPath %q must not traverse parent directories", p)
143+
}
144+
for _, r := range p {
145+
switch {
146+
case r >= 'A' && r <= 'Z':
147+
case r >= 'a' && r <= 'z':
148+
case r >= '0' && r <= '9':
149+
case r == '/' || r == '.' || r == '_' || r == '-':
150+
default:
151+
return fmt.Errorf("EnvFileRelPath %q contains disallowed character %q", p, r)
152+
}
153+
}
154+
return nil
155+
}
156+
116157
// resolveProfile returns p when non-nil, or OpenclawProfile as the
117158
// default. This lets existing call sites that do not pass a profile
118159
// keep their previous behavior.

internal/container/agent_profile_test.go

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,58 @@ func TestHermesProfile_WireMCPUsesPython(t *testing.T) {
105105
}
106106
}
107107

108+
func TestProfileFromName_ErrorListsKnownSorted(t *testing.T) {
109+
_, err := ProfileFromName("does-not-exist")
110+
if err == nil {
111+
t.Fatal("expected error for unknown profile")
112+
}
113+
msg := err.Error()
114+
// "hermes" must appear before "openclaw" alphabetically when the list is sorted.
115+
hi := strings.Index(msg, "hermes")
116+
oi := strings.Index(msg, "openclaw")
117+
if hi < 0 || oi < 0 || hi > oi {
118+
t.Errorf("error message %q should list known profiles in sorted order (hermes before openclaw)", msg)
119+
}
120+
}
121+
122+
func TestValidateEnvFileRelPath(t *testing.T) {
123+
good := []string{
124+
".openclaw/.env",
125+
".hermes/.env",
126+
"home/agent/.env",
127+
"a-b_c.env",
128+
}
129+
for _, p := range good {
130+
if err := validateEnvFileRelPath(p); err != nil {
131+
t.Errorf("expected %q to be accepted, got: %v", p, err)
132+
}
133+
}
134+
bad := []string{
135+
"",
136+
"/abs/path",
137+
"../escape",
138+
"a/../b",
139+
`a"; rm -rf /`,
140+
"a$(whoami)",
141+
"a b",
142+
"a;b",
143+
"a\nb",
144+
}
145+
for _, p := range bad {
146+
if err := validateEnvFileRelPath(p); err == nil {
147+
t.Errorf("expected %q to be rejected", p)
148+
}
149+
}
150+
}
151+
152+
func TestBuildEnvInjectionScriptForProfile_RejectsUnsafePath(t *testing.T) {
153+
bad := &AgentProfile{Name: "evil", EnvFileRelPath: `evil"; touch /tmp/pwned`}
154+
_, err := BuildEnvInjectionScriptForProfile(bad, map[string]string{"K": "v"}, false, false)
155+
if err == nil {
156+
t.Fatal("expected error for unsafe EnvFileRelPath")
157+
}
158+
}
159+
108160
func TestResolveProfile_NilDefaultsToOpenclaw(t *testing.T) {
109161
if resolveProfile(nil) != OpenclawProfile {
110162
t.Error("nil profile should default to OpenclawProfile")

0 commit comments

Comments
 (0)