diff --git a/internal/shell/expand.go b/internal/shell/expand.go index ee110a1df7b6b60b9ac2d51b7ae5f6abcbf3ac79..cb35ed2aade3006da80b1eb96e7496df0d37b343 100644 --- a/internal/shell/expand.go +++ b/internal/shell/expand.go @@ -7,6 +7,7 @@ import ( "io" "os" "strings" + "sync/atomic" "mvdan.cc/sh/v3/expand" "mvdan.cc/sh/v3/interp" @@ -18,11 +19,28 @@ import ( // to be embedded in a failing inner command. const maxInnerStderrBytes = 512 +// NoUnset controls whether ExpandValue treats unset variables as an +// error. Default false matches bash: $UNSET expands to "". Store true +// to re-enable strict mode globally. Not exposed in crush.json; this is +// an internal escape hatch in case the lenient default turns out to be +// the wrong call. +// +// Declared atomic because ExpandValue is invoked concurrently (multiple +// MCP / LSP / provider loads in flight at startup, hook execution, etc.) +// and an unsynchronised read/write pair is a data race under the Go +// memory model regardless of test-level happens-before reasoning. The +// atomic load on the hot path is negligible against the cost of parsing +// and running through mvdan. +// +// See PLAN.md Phase 2 design decisions #11 and #12 for the full +// rationale. +var NoUnset atomic.Bool + // ExpandValue expands shell-style substitutions in a single config value. // // Supported constructs match the bash tool: // -// - $VAR and ${VAR} (unset is an error; see nounset below). +// - $VAR and ${VAR}. // - ${VAR:-default} / ${VAR:+alt} / ${VAR:?msg}. // - $(command) with full quoting and nesting. // - escaped and quoted strings ("...", '...'). @@ -32,9 +50,11 @@ const maxInnerStderrBytes = 512 // - Returns exactly one string. No field splitting, no globbing, no // pathname generation. Multi-word command output is preserved // verbatim; it is never split into multiple values. -// - Nounset is on: unset variables produce an error instead of -// expanding to the empty string. Use ${VAR:-default} to opt in to -// an empty fallback. +// - Nounset is off by default, matching bash: unset variables expand +// to "". Opt in to strict behaviour per-reference with +// ${VAR:?msg}, which errors loudly when VAR is unset regardless of +// the global toggle. Flip the global default via +// shell.NoUnset.Store(true) as an internal escape hatch. // - Embedded whitespace and newlines in the input are preserved // verbatim. Command substitution strips trailing newlines only // (POSIX), never leading or internal whitespace. @@ -63,22 +83,27 @@ func ExpandValue(ctx context.Context, value string, env []string) (string, error logger: noopLogger{}, } + strict := NoUnset.Load() + var stderrBuf bytes.Buffer cfg := &expand.Config{ Env: expand.ListEnviron(env...), - NoUnset: true, + NoUnset: strict, CmdSubst: func(w io.Writer, cs *syntax.CmdSubst) error { stderrBuf.Reset() - runner, rerr := interp.New( + runnerOpts := []interp.RunnerOption{ interp.StdIO(nil, w, &stderrBuf), interp.Interactive(false), interp.Env(expand.ListEnviron(env...)), interp.Dir(s.cwd), interp.ExecHandlers(s.execHandlers()...), + } + if strict { // Match the outer NoUnset: an unset $VAR inside // $(...) is also an error, not a silent empty. - interp.Params("-u"), - ) + runnerOpts = append(runnerOpts, interp.Params("-u")) + } + runner, rerr := interp.New(runnerOpts...) if rerr != nil { return rerr } diff --git a/internal/shell/expand_test.go b/internal/shell/expand_test.go index 8661d6682370d31666185576db7edddb17a65b97..77d04454abf4447405e5b319b785dcff6e9fc760 100644 --- a/internal/shell/expand_test.go +++ b/internal/shell/expand_test.go @@ -105,22 +105,25 @@ func TestExpandValue_Success(t *testing.T) { func TestExpandValue_Errors(t *testing.T) { t.Parallel() - t.Run("unset var is an error, not empty", func(t *testing.T) { + t.Run("unset var expands to empty under lenient default", func(t *testing.T) { t.Parallel() - _, err := ExpandValue(t.Context(), "$MISSING", nil) - require.Error(t, err) + got, err := ExpandValue(t.Context(), "$MISSING", nil) + require.NoError(t, err) + require.Equal(t, "", got) }) - t.Run("unset var inside braces is an error", func(t *testing.T) { + t.Run("unset var inside braces expands to empty", func(t *testing.T) { t.Parallel() - _, err := ExpandValue(t.Context(), "${MISSING}", nil) - require.Error(t, err) + got, err := ExpandValue(t.Context(), "${MISSING}", nil) + require.NoError(t, err) + require.Equal(t, "", got) }) - t.Run("unset var inside cmdsubst is an error", func(t *testing.T) { + t.Run("unset var inside cmdsubst expands to empty", func(t *testing.T) { t.Parallel() - _, err := ExpandValue(t.Context(), `$(printf '%s' "$MISSING")`, nil) - require.Error(t, err) + got, err := ExpandValue(t.Context(), `$(printf '%s' "$MISSING")`, nil) + require.NoError(t, err) + require.Equal(t, "", got) }) t.Run("bad syntax returns error", func(t *testing.T) { @@ -173,6 +176,45 @@ func TestExpandValue_Errors(t *testing.T) { }) } +// TestExpandValue_StrictToggle pins the NoUnset escape hatch: when a +// caller flips strict mode on, bare $UNSET must error instead of +// expanding to the empty string. Must not run in parallel: it mutates +// the package-level NoUnset atomic, so a parallel peer observing the +// flipped value would break the lenient default other tests assume. +func TestExpandValue_StrictToggle(t *testing.T) { + NoUnset.Store(true) + t.Cleanup(func() { NoUnset.Store(false) }) + + _, err := ExpandValue(t.Context(), "$UNSET", nil) + require.Error(t, err) + + _, err = ExpandValue(t.Context(), "${UNSET}", nil) + require.Error(t, err) + + _, err = ExpandValue(t.Context(), `$(printf '%s' "$UNSET")`, nil) + require.Error(t, err) +} + +// TestExpandValue_RequiredOptIn pins the per-reference opt-in strict +// idiom ${VAR:?msg}: it must error whether or not the global NoUnset +// toggle is on, so config authors can mark individual credentials as +// required without flipping the global default. +func TestExpandValue_RequiredOptIn(t *testing.T) { + t.Parallel() + + _, err := ExpandValue(t.Context(), "${REQUIRED:?must be set}", nil) + require.Error(t, err) + require.Contains(t, err.Error(), "must be set") + + got, err := ExpandValue( + t.Context(), + "${REQUIRED:?must be set}", + []string{"REQUIRED=ok"}, + ) + require.NoError(t, err) + require.Equal(t, "ok", got) +} + func TestSanitizeStderr(t *testing.T) { t.Parallel()