git.iko.soy/crush
on main help sign in
shell: switch config value expansion to lenient by default
abacef9Christian Rocha
internal/shell/expand.go+33−8
compare with main
unifiedsplit
@@ -7,6 +7,7 @@ import (
7 "io"
7 "io"
8 "os"
8 "os"
9 "strings"
9 "strings"
10+ "sync/atomic"
10
11
11 "mvdan.cc/sh/v3/expand"
12 "mvdan.cc/sh/v3/expand"
12 "mvdan.cc/sh/v3/interp"
13 "mvdan.cc/sh/v3/interp"
@@ -18,11 +19,28 @@ import (
18// to be embedded in a failing inner command.
19// to be embedded in a failing inner command.
19const maxInnerStderrBytes = 512
20const maxInnerStderrBytes = 512
20
21
22+// NoUnset controls whether ExpandValue treats unset variables as an
23+// error. Default false matches bash: $UNSET expands to "". Store true
24+// to re-enable strict mode globally. Not exposed in crush.json; this is
25+// an internal escape hatch in case the lenient default turns out to be
26+// the wrong call.
27+//
28+// Declared atomic because ExpandValue is invoked concurrently (multiple
29+// MCP / LSP / provider loads in flight at startup, hook execution, etc.)
30+// and an unsynchronised read/write pair is a data race under the Go
31+// memory model regardless of test-level happens-before reasoning. The
32+// atomic load on the hot path is negligible against the cost of parsing
33+// and running through mvdan.
34+//
35+// See PLAN.md Phase 2 design decisions #11 and #12 for the full
36+// rationale.
37+var NoUnset atomic.Bool
38+
21// ExpandValue expands shell-style substitutions in a single config value.
39// ExpandValue expands shell-style substitutions in a single config value.
22//
40//
23// Supported constructs match the bash tool:
41// Supported constructs match the bash tool:
24//
42//
25−// - $VAR and ${VAR} (unset is an error; see nounset below).
43+// - $VAR and ${VAR}.
26// - ${VAR:-default} / ${VAR:+alt} / ${VAR:?msg}.
44// - ${VAR:-default} / ${VAR:+alt} / ${VAR:?msg}.
27// - $(command) with full quoting and nesting.
45// - $(command) with full quoting and nesting.
28// - escaped and quoted strings ("...", '...').
46// - escaped and quoted strings ("...", '...').
@@ -32,9 +50,11 @@ const maxInnerStderrBytes = 512
32// - Returns exactly one string. No field splitting, no globbing, no
50// - Returns exactly one string. No field splitting, no globbing, no
33// pathname generation. Multi-word command output is preserved
51// pathname generation. Multi-word command output is preserved
34// verbatim; it is never split into multiple values.
52// verbatim; it is never split into multiple values.
35−// - Nounset is on: unset variables produce an error instead of
53+// - Nounset is off by default, matching bash: unset variables expand
36−// expanding to the empty string. Use ${VAR:-default} to opt in to
54+// to "". Opt in to strict behaviour per-reference with
37−// an empty fallback.
55+// ${VAR:?msg}, which errors loudly when VAR is unset regardless of
56+// the global toggle. Flip the global default via
57+// shell.NoUnset.Store(true) as an internal escape hatch.
38// - Embedded whitespace and newlines in the input are preserved
58// - Embedded whitespace and newlines in the input are preserved
39// verbatim. Command substitution strips trailing newlines only
59// verbatim. Command substitution strips trailing newlines only
40// (POSIX), never leading or internal whitespace.
60// (POSIX), never leading or internal whitespace.
@@ -63,22 +83,27 @@ func ExpandValue(ctx context.Context, value string, env []string) (string, error
63 logger: noopLogger{},
83 logger: noopLogger{},
64 }
84 }
65
85
86+ strict := NoUnset.Load()
87+
66 var stderrBuf bytes.Buffer
88 var stderrBuf bytes.Buffer
67 cfg := &expand.Config{
89 cfg := &expand.Config{
68 Env: expand.ListEnviron(env...),
90 Env: expand.ListEnviron(env...),
69− NoUnset: true,
91+ NoUnset: strict,
70 CmdSubst: func(w io.Writer, cs *syntax.CmdSubst) error {
92 CmdSubst: func(w io.Writer, cs *syntax.CmdSubst) error {
71 stderrBuf.Reset()
93 stderrBuf.Reset()
72− runner, rerr := interp.New(
94+ runnerOpts := []interp.RunnerOption{
73 interp.StdIO(nil, w, &stderrBuf),
95 interp.StdIO(nil, w, &stderrBuf),
74 interp.Interactive(false),
96 interp.Interactive(false),
75 interp.Env(expand.ListEnviron(env...)),
97 interp.Env(expand.ListEnviron(env...)),
76 interp.Dir(s.cwd),
98 interp.Dir(s.cwd),
77 interp.ExecHandlers(s.execHandlers()...),
99 interp.ExecHandlers(s.execHandlers()...),
100+ }
101+ if strict {
78 // Match the outer NoUnset: an unset $VAR inside
102 // Match the outer NoUnset: an unset $VAR inside
79 // $(...) is also an error, not a silent empty.
103 // $(...) is also an error, not a silent empty.
80− interp.Params("-u"),
104+ runnerOpts = append(runnerOpts, interp.Params("-u"))
81− )
105+ }
106+ runner, rerr := interp.New(runnerOpts...)
82 if rerr != nil {
107 if rerr != nil {
83 return rerr
108 return rerr
84 }
109 }
internal/shell/expand_test.go+51−9
@@ -105,22 +105,25 @@ func TestExpandValue_Success(t *testing.T) {
105func TestExpandValue_Errors(t *testing.T) {
105func TestExpandValue_Errors(t *testing.T) {
106 t.Parallel()
106 t.Parallel()
107
107
108− t.Run("unset var is an error, not empty", func(t *testing.T) {
108+ t.Run("unset var expands to empty under lenient default", func(t *testing.T) {
109 t.Parallel()
109 t.Parallel()
110− _, err := ExpandValue(t.Context(), "$MISSING", nil)
110+ got, err := ExpandValue(t.Context(), "$MISSING", nil)
111− require.Error(t, err)
111+ require.NoError(t, err)
112+ require.Equal(t, "", got)
112 })
113 })
113
114
114− t.Run("unset var inside braces is an error", func(t *testing.T) {
115+ t.Run("unset var inside braces expands to empty", func(t *testing.T) {
115 t.Parallel()
116 t.Parallel()
116− _, err := ExpandValue(t.Context(), "${MISSING}", nil)
117+ got, err := ExpandValue(t.Context(), "${MISSING}", nil)
117− require.Error(t, err)
118+ require.NoError(t, err)
119+ require.Equal(t, "", got)
118 })
120 })
119
121
120− t.Run("unset var inside cmdsubst is an error", func(t *testing.T) {
122+ t.Run("unset var inside cmdsubst expands to empty", func(t *testing.T) {
121 t.Parallel()
123 t.Parallel()
122− _, err := ExpandValue(t.Context(), `$(printf '%s' "$MISSING")`, nil)
124+ got, err := ExpandValue(t.Context(), `$(printf '%s' "$MISSING")`, nil)
123− require.Error(t, err)
125+ require.NoError(t, err)
126+ require.Equal(t, "", got)
124 })
127 })
125
128
126 t.Run("bad syntax returns error", func(t *testing.T) {
129 t.Run("bad syntax returns error", func(t *testing.T) {
@@ -173,6 +176,45 @@ func TestExpandValue_Errors(t *testing.T) {
173 })
176 })
174}
177}
175
178
179+// TestExpandValue_StrictToggle pins the NoUnset escape hatch: when a
180+// caller flips strict mode on, bare $UNSET must error instead of
181+// expanding to the empty string. Must not run in parallel: it mutates
182+// the package-level NoUnset atomic, so a parallel peer observing the
183+// flipped value would break the lenient default other tests assume.
184+func TestExpandValue_StrictToggle(t *testing.T) {
185+ NoUnset.Store(true)
186+ t.Cleanup(func() { NoUnset.Store(false) })
187+
188+ _, err := ExpandValue(t.Context(), "$UNSET", nil)
189+ require.Error(t, err)
190+
191+ _, err = ExpandValue(t.Context(), "${UNSET}", nil)
192+ require.Error(t, err)
193+
194+ _, err = ExpandValue(t.Context(), `$(printf '%s' "$UNSET")`, nil)
195+ require.Error(t, err)
196+}
197+
198+// TestExpandValue_RequiredOptIn pins the per-reference opt-in strict
199+// idiom ${VAR:?msg}: it must error whether or not the global NoUnset
200+// toggle is on, so config authors can mark individual credentials as
201+// required without flipping the global default.
202+func TestExpandValue_RequiredOptIn(t *testing.T) {
203+ t.Parallel()
204+
205+ _, err := ExpandValue(t.Context(), "${REQUIRED:?must be set}", nil)
206+ require.Error(t, err)
207+ require.Contains(t, err.Error(), "must be set")
208+
209+ got, err := ExpandValue(
210+ t.Context(),
211+ "${REQUIRED:?must be set}",
212+ []string{"REQUIRED=ok"},
213+ )
214+ require.NoError(t, err)
215+ require.Equal(t, "ok", got)
216+}
217+
176func TestSanitizeStderr(t *testing.T) {
218func TestSanitizeStderr(t *testing.T) {
177 t.Parallel()
219 t.Parallel()
178
220