From abacef933667599de956fc9493166bf9f92c56f6 Mon Sep 17 00:00:00 2001 From: Christian Rocha Date: Sun, 3 May 2026 17:02:26 -0400 Subject: [PATCH] shell: switch config value expansion to lenient by default Missing env vars in crush.json used to fail loudly. They now quietly expand to an empty string, the same as bash. If you want the old strict behavior for a specific value, write it as ${VAR:?message} and Crush will still complain when the variable is not set. Co-Authored-By: Charm Crush --- internal/shell/expand.go | 41 +++++++++++++++++++----- internal/shell/expand_test.go | 60 +++++++++++++++++++++++++++++------ 2 files changed, 84 insertions(+), 17 deletions(-) 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()