fix: read the two shapes a shell string hid from the rm guard entry

.abcd/work/issues/open/iss-2610110025455728-guard-misses-an-alternative-in-a-string.md → .abcd/work/issues/resolved/iss-2610110025455728-guard-misses-an-alternative-in-a-string.md

@@ -10,6 +10,8 @@ origin: researcher-authored
 production_mode: hand-written
 found_at: "internal/core/guard/payload.go"
 remedy: "When a payload site's text is empty and no text it prints names a variable (an alternative such as ${X:+x}), write the empty text in the lead spelling as a reference that can be empty, so the string's re-read leads with a variable as the direct form does; one fixture, watched fail first."
+resolution: "The rm-unguarded-variable-path entry now refuses both shapes a double-quoted shell string hid from it: an alternative the enclosing shell expands to nothing (sh -c \"rm -rf ${X:+x}/y\"), and a default written as an escaped reference (sh -c \"rm -rf ${X:-\\${Y}}/y\"), which the string's shell reads as ${Y}/y. A site that prints nothing where no / follows it stays allowed."
+impact: fix
 ---
 
 The rm-unguarded-variable-path guard entry allows an alternative inside a double-quoted shell string: sh -c "rm -rf ${X:+x}/y" is allowed, although the enclosing shell expands ${X:+x} to nothing when X is unset and the string's shell then deletes /y. The direct form, rm -rf ${X:+x}/y, refuses. The cause is the payload re-read: the alternative's texts are the empty text and x, neither names a variable, so the string is written out as rm -rf /y and rm -rf x/y and neither reading leads with a variable (payload.go namedPayloads, varpath.go leadSpelling). It has been allowed since the entry landed in #889. The same re-read misses a default the enclosing shell writes as an escaped reference: sh -c "rm -rf ${X:-\${Y}}/y" and ${X:-\${Y:-}} are allowed, although every shell hands the string rm -rf ${Y}/y. The default's body ends at the first }, so its text is ${Y, and the written text ${Y}/y does not pair with the marked word, whose } follows the mark (payload.go fitsWritten); main allows it too. Separately, the edge fix refuses sh -c "rm -rf ${X:-\\ }/y", whose escaped blank the string's shell keeps as text; this is a deliberate over-refusal (varpath.go emptiedInside) and main refused it too.

internal/core/guard/defaultword_test.go

@@ -201,6 +201,11 @@ func TestSpellingSetsStayLinear(t *testing.T) {
 		{"payload defaults", func(n int) string {
 			return `sh -c "rm -rf ` + strings.Repeat("${A:-/}${B:-~} x ", n/18) + `"`
 		}},
+		// A site that can print nothing is written out for its lead too
+		// (emptyLead), which must stay inside the same bar.
+		{"payload alternatives", func(n int) string {
+			return `sh -c "rm -rf ` + strings.Repeat("${A:+/}${B:+~} x ", n/18) + `"`
+		}},
 	}
 	for _, s := range shapes {
 		t.Run(s.name, func(t *testing.T) {

internal/core/guard/payload.go

@@ -2,6 +2,7 @@ package guard
 
 import (
 	"path"
+	"slices"
 	"strings"
 )
 
@@ -143,7 +144,7 @@ func expandPayloads(segs []segment, depth int) ([]segment, []payloadSignal) {
 			var stdin, args []feed
 			inputRead := false
 			refs := payloadRefsOf(payloadView(s))
-			named, leadNamed := namedPayloads(s, refs)
+			named, leadNamed, leadAdds := namedPayloads(s, refs)
 			for r, ref := range refs {
 				kind, fam, payload, trailing := ref.kind, ref.family, ref.payload, ref.trailing
 				// Past the depth budget the guard cannot follow the nesting, so a
@@ -171,7 +172,7 @@ func expandPayloads(segs []segment, depth int) ([]segment, []payloadSignal) {
 						continue
 					}
 					psegs = pseg
-					spellPayload(psegs, named[r], leadNamed(r))
+					spellPayload(psegs, named[r], leadNamed(r), leadAdds)
 				case kindShellWarn:
 					signals = append(signals, shellUnresolvedSignal())
 					continue
@@ -187,7 +188,7 @@ func expandPayloads(segs []segment, depth int) ([]segment, []payloadSignal) {
 						continue
 					}
 					psegs = pseg
-					spellPayload(psegs, named[r], leadNamed(r))
+					spellPayload(psegs, named[r], leadNamed(r), leadAdds)
 				case kindExecStringWarn:
 					signals = append(signals, execStringWarnSignal(fam))
 					continue
@@ -295,15 +296,20 @@ func payloadView(s segment) segment {
 // lead is the same, written with segment.leadFrom's words spelled by
 // leadSpelling, for the varLead spellPayload takes alone, and nil where no
 // word has one. It is a function, asked only for a string whose reading
-// leads, so a string no reading leads in is not written out again.
-func namedPayloads(s segment, refs []payloadRef) (named [][]string, lead func(r int) func() []string) {
+// leads, so a string no reading leads in is not written out again, unless
+// adds: a word of s holds a site that can print the empty text, which only
+// lead's readings can lead with.
+func namedPayloads(s segment, refs []payloadRef) (named [][]string, lead func(r int) func() []string, adds bool) {
 	none := func(int) func() []string { return nil }
 	if len(refs) == 0 {
-		return nil, none
+		return nil, none, false
 	}
 	named = writtenPayloads(spelledViews(s), refs)
 	if len(s.leadFrom) == 0 {
-		return named, none
+		return named, none, false
+	}
+	for _, src := range s.leadFrom {
+		adds = adds || emptyOpensPath(src.word, src.sites)
 	}
 	var leads [][]string
 	asked := false
@@ -325,7 +331,7 @@ func namedPayloads(s segment, refs []payloadRef) (named [][]string, lead func(r
 	}
 	return named, func(r int) func() []string {
 		return func() []string { return all()[r] }
-	}
+	}, adds
 }
 
 // writtenPayloads is namedPayloads for the views given.
@@ -411,9 +417,10 @@ func spelledViews(s segment) []segment {
 // leads and lead, when not nil, has readings — the string written with each
 // guarded value as its guard (leadSpelling) — the leads are taken from those
 // instead: they differ only where a guard keeps a value from being empty,
-// so they can take a lead away, never add one. Nothing else of psegs is
-// changed.
-func spellPayload(psegs []segment, named []string, lead func() []string) {
+// which can take a lead away, and, where adds is set, where a site prints
+// the empty text, which can add one (emptyLead), so lead is asked even when
+// no named reading leads. Nothing else of psegs is changed.
+func spellPayload(psegs []segment, named []string, lead func() []string, adds bool) {
 	paired := make([]map[int][]string, len(psegs))
 	leads := make([]map[int]bool, len(psegs))
 	// pair calls fn for each word of psegs the string nm's reading pairs,
@@ -441,7 +448,12 @@ func spellPayload(psegs []segment, named []string, lead func() []string) {
 					}
 					ws = []string{n.tokens[j]}
 				}
-				if !fitsWritten(m.tokens[j], n.tokens[j]) {
+				// A text can run across a brace the string holds as text:
+				// `${X:-\${Y}}` prints `${Y` before the word's own `}`, so the
+				// reading's `${Y}/y` marks the brace as the variable's, and
+				// only its written text fits the string's word.
+				if !fitsWritten(m.tokens[j], n.tokens[j]) &&
+					!slices.ContainsFunc(ws, func(w string) bool { return fitsWritten(m.tokens[j], w) }) {
 					continue
 				}
 				fn(i, j, n, ws)
@@ -467,7 +479,7 @@ func spellPayload(psegs []segment, named []string, lead func() []string) {
 			}
 		})
 	}
-	if lead != nil && anyLead(leads) {
+	if lead != nil && (adds || anyLead(leads)) {
 		if ls := lead(); len(ls) > 0 {
 			// A word no guarded reading pairs keeps the lead named gave it.
 			named := leads

internal/core/guard/tokenize.go

@@ -109,9 +109,10 @@ type segment struct {
 	// word does.
 	varLead map[int]bool
 	// leadFrom records, per token index, a spelled word holding a value a
-	// guard keeps from being empty, as the word and its sites, from which
-	// only a payload re-read's varLead spells it again (leadSpelling in
-	// varpath.go, spellPayload). nil when no word does.
+	// guard keeps from being empty or a site whose empty text leaves a `/`
+	// first (emptyOpensPath in varpath.go), as the word and its sites, from which only a payload re-read's varLead spells
+	// it again (leadSpelling in varpath.go, spellPayload). nil when no word
+	// does.
 	leadFrom map[int]leadSource
 	// arrivals caches commandArrivals(tokens) once Check has its final
 	// segments (walked records that it is set), so the walk to command position
@@ -812,7 +813,7 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) {
 			spells[len(toks)] = spellWritten(cur, curVarAt, nil)
 			markIFSSplit(curVarAt)
 			markVarLead(cur, curVarAt)
-			if spellsGuards(curVarAt) {
+			if spellsGuards(curVarAt) || emptyOpensPath(cur, curVarAt) {
 				if leadFrom == nil {
 					leadFrom = map[int]leadSource{}
 				}

internal/core/guard/unguardedvar_test.go

@@ -237,3 +237,29 @@ func TestRmDefaultThatSplitsAwayIsNoGuard(t *testing.T) {
 		{`rm -rf ${X:-/tmp}/y`, VerdictAllow, ""},
 	})
 }
+
+// TestRmEmptyTextInAShellStringLeads — iss-2610110025455728. The enclosing
+// shell expands an alternative to nothing when its variable is unset, so
+// `sh -c "rm -rf ${X:+x}/y"` hands the string `rm -rf /y`, as the direct
+// `rm -rf ${X:+x}/y` deletes `/y`. A text a site in the string can print
+// empty leads the word as the variable would; one that cannot, or that no `/`
+// follows, does not.
+func TestRmEmptyTextInAShellStringLeads(t *testing.T) {
+	const id = "rm-unguarded-variable-path"
+	runVerdictCases(t, []verdictCase{
+		{`sh -c "rm -rf ${X:+x}/y"`, VerdictBlock, id},
+		{`bash -c "rm -rf \"${X:+x}\"/y"`, VerdictBlock, id},
+		{`sh -c "rm -rf ${X:+x}${Y:+z}/y"`, VerdictBlock, id},
+		{`sh -c "rm -rf ${X:+x}/y" _`, VerdictBlock, id},
+		// A default the enclosing shell writes as an escaped reference ends
+		// at its first `}`, so the string reads `${Y}/y` across the brace
+		// the enclosing word holds as text.
+		{`sh -c "rm -rf ${X:-\${Y}}/y"`, VerdictBlock, id},
+		{`sh -c "rm -rf ${X:-\${Y:-}}/y"`, VerdictBlock, id},
+		{`sh -c "rm -rf ${X:-\${HOME}}/y"`, VerdictAllow, ""},
+		{`sh -c "rm -rf ${X:?}${Y:+z}/y"`, VerdictAllow, ""},
+		{`sh -c "rm -rf ./${X:+x}/y"`, VerdictAllow, ""},
+		{`sh -c "rm -f ${X:+--force} /tmp/a"`, VerdictAllow, ""},
+		{`sh -c "echo ${X:+x}"`, VerdictAllow, ""},
+	})
+}

internal/core/guard/varpath.go

@@ -255,6 +255,41 @@ type leadSource struct {
 	sites []varSite
 }
 
+// emptyLead is how leadSpelling writes the empty text a site can print: a
+// reference that can be empty (`$_` is, at the top of a fresh shell,
+// emptyable), so the string's re-read leads with a variable where the
+// enclosing shell's expansion printed nothing (`sh -c "rm -rf ${X:+x}/y"`
+// hands the string `rm -rf /y`, iss-2610110025455728). Only varLead reads
+// it; the arg_values compare reads the empty text.
+const emptyLead = "${_}"
+
+// emptyOpensPath reports whether a site of sites can print the empty text
+// where a `/` follows it, which leadSpelling writes as emptyLead. It only
+// spares a string the second writing out, since the string's re-read decides
+// the lead, so it reads loosely: past a quote the string's shell takes off
+// (`bash -c "rm -rf \"${X:+x}\"/y"`), and whatever precedes the site. A site
+// that prints nothing where no `/` follows (`rm -f ${X:+--force} /tmp/a`,
+// `${X:+x}${Y:+z}/y` is read at Y) is not, so a string holding only such
+// sites is not written out again.
+func emptyOpensPath(word []byte, sites []varSite) bool {
+	for k, s := range sites {
+		if s.transform || capped(s.texts) || !slices.Contains(s.texts, "") {
+			continue
+		}
+		p := s.at + max(s.width, 1)
+		for p < len(word) && !(k+1 < len(sites) && sites[k+1].at == p) {
+			if c := word[p]; c != '"' && c != '\'' && c != '\\' && c != unknownMark {
+				break
+			}
+			p++
+		}
+		if p < len(word) && word[p] == '/' {
+			return true
+		}
+	}
+	return false
+}
+
 // spellsGuards reports whether leadSpelling would write any of sites
 // otherwise than spellWritten does.
 func spellsGuards(sites []varSite) bool {
@@ -314,23 +349,32 @@ func emptiedInside(t string, guarded []string) bool {
 // that can be empty, so `sh -c "rm -rf ${X:?}/y"` would read as the very
 // shape its `:?` rules out (iss-2610100938485695); here it is written as
 // `${X:?}`, which the re-read reads as guarded, as the enclosing shell did.
-// A site that can also print the empty text keeps its texts: the string
-// written out with that text holds no variable to lead with, and only the
-// `${X}` spelling still says the site can leave the `/` first (`${X:-}`).
+// A site that can also print the empty text keeps its `${X}`, and writes
+// the empty text as emptyLead: the string written out with nothing there
+// holds no variable to lead with, although the enclosing shell's expansion
+// leaves the `/` first (`${X:-}`, `${X:+x}`).
 func leadSpelling(src leadSource) []string {
 	lead := slices.Clone(src.sites)
 	for k, s := range src.sites {
-		if !respelled(s) {
-			continue
-		}
-		texts := make([]string, len(s.texts))
-		for n, t := range s.texts {
-			for _, g := range s.guarded {
-				t = strings.ReplaceAll(t, g, strings.TrimSuffix(g, "}")+":?}")
+		switch {
+		case respelled(s):
+			texts := make([]string, len(s.texts))
+			for n, t := range s.texts {
+				for _, g := range s.guarded {
+					t = strings.ReplaceAll(t, g, strings.TrimSuffix(g, "}")+":?}")
+				}
+				texts[n] = t
+			}
+			lead[k].texts = texts
+		case !capped(s.texts) && slices.Contains(s.texts, ""):
+			texts := slices.Clone(s.texts)
+			for n, t := range texts {
+				if t == "" {
+					texts[n] = emptyLead
+				}
 			}
-			texts[n] = t
+			lead[k].texts = texts
 		}
-		lead[k].texts = texts
 	}
 	return spellWritten(src.word, lead, nil)
 }