fix: read the edge shapes of the unguarded variable path guard entry

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

@@ -0,0 +1,15 @@
+---
+schema_version: 1
+id: "iss-2610110025455728"
+slug: "guard-misses-an-alternative-in-a-string"
+severity: "minor"
+category: "bug"
+source: "impl-review"
+found_during: "review of the rm-unguarded-variable-path edge fix (fix/guard-rm-varpath-edges)"
+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."
+---
+
+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.

.abcd/work/issues/open/iss-2610100938485695-the-rm-unguarded-variable-path-guard.md → .abcd/work/issues/resolved/iss-2610100938485695-the-rm-unguarded-variable-path-guard.md

@@ -10,6 +10,8 @@ origin: researcher-authored
 production_mode: hand-written
 found_at: "internal/core/guard/varpath.go"
 remedy: "Carry the non-empty guarantee of :? / :- / := through the named re-read of a double-quoted shell string, read nested defaults recursively, and treat a variable followed by a substitution before the slash as emptyable; one fixture per shape, watched fail first."
+resolution: "The rm-unguarded-variable-path entry now reads all three shapes: a double-quoted shell string's re-read takes its lead from the string written with each guarded value as its guard, so sh -c \"rm -rf ${X:?}/y\" is allowed; a guard nested in a default is read (and a name the expansion also writes unguarded, such as ${VAR:-${VAR}}, is not guarded); and a variable followed by a command substitution before the slash refuses. Arithmetic expansion stays allowed, since it always prints a number."
+impact: fix
 ---
 
 The rm-unguarded-variable-path guard entry misjudges three edge shapes found in its review. (1) Inside a double-quoted shell string the successor's own rewrite is refused: sh -c "rm -rf ${X:?}/y" blocks, because the re-read string spells ${X:?} as ${X} (payload.go spellParameterAt), so the refusal tells the agent to do what it did; single quotes avoid it. (2) A nested guard is not read: "${VAR:-${OTHER:?}}"/y and "${VAR:-${OTHER:-/tmp}}"/y block although both are guarded (varpath.go stripEmptyableRefs strips the inner reference). (3) A site followed by a substitution is not followed: rm -rf $VAR$(true)/x is allowed (varpath.go).

internal/core/guard/defaults/guard.json

@@ -143,13 +143,20 @@
           "make clean && rm -rf \"$OUT\"/*",
           "sh -c 'rm -rf \"$VAR\"/*'",
           "rm -rf \"$1\"/build",
-          "rm -rf ${VAR:-}/build"
+          "rm -rf ${VAR:-}/build",
+          "rm -rf \"${VAR:-${OTHER}}\"/build",
+          "rm -rf \"${VAR:-${VAR}}\"/build",
+          "rm -rf $VAR$(true)/build",
+          "rm -rf \"$VAR`true`\"/build"
         ],
         "known_good": [
           "rm -rf \"${VAR:?}\"/build",
           "rm -f -- \"${VAR:?}\"/*",
           "rm -rf ${VAR:?VAR is unset}/build",
           "rm -rf \"${TMPDIR:-/tmp}/abcd-x\"",
+          "rm -rf \"${VAR:-${OTHER:?}}\"/build",
+          "rm -rf \"${VAR:-${OTHER:-/tmp}}\"/build",
+          "rm -rf $VAR$((1))/build",
           "rm -rf /tmp/build",
           "rm -rf ./build/$name",
           "rm -rf \"$tmp\"",

internal/core/guard/payload.go

@@ -143,7 +143,7 @@ func expandPayloads(segs []segment, depth int) ([]segment, []payloadSignal) {
 			var stdin, args []feed
 			inputRead := false
 			refs := payloadRefsOf(payloadView(s))
-			named := namedPayloads(s, refs)
+			named, leadNamed := 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 +171,7 @@ func expandPayloads(segs []segment, depth int) ([]segment, []payloadSignal) {
 						continue
 					}
 					psegs = pseg
-					spellPayload(psegs, named[r])
+					spellPayload(psegs, named[r], leadNamed(r))
 				case kindShellWarn:
 					signals = append(signals, shellUnresolvedSignal())
 					continue
@@ -187,7 +187,7 @@ func expandPayloads(segs []segment, depth int) ([]segment, []payloadSignal) {
 						continue
 					}
 					psegs = pseg
-					spellPayload(psegs, named[r])
+					spellPayload(psegs, named[r], leadNamed(r))
 				case kindExecStringWarn:
 					signals = append(signals, execStringWarnSignal(fam))
 					continue
@@ -291,12 +291,47 @@ func payloadView(s segment) segment {
 // takes from them; every reading of the string reads the marks
 // (iss-2609290321312087). The words are paired by payloadsOf's own order, and
 // a pair whose kind or family differs is not paired.
-func namedPayloads(s segment, refs []payloadRef) [][]string {
+//
+// 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) {
+	none := func(int) func() []string { return nil }
 	if len(refs) == 0 {
-		return nil
+		return nil, none
+	}
+	named = writtenPayloads(spelledViews(s), refs)
+	if len(s.leadFrom) == 0 {
+		return named, none
+	}
+	var leads [][]string
+	asked := false
+	all := func() [][]string {
+		if asked {
+			return leads
+		}
+		asked = true
+		ls := s
+		ls.spelled = map[int][]string{}
+		for i, texts := range s.spelled {
+			ls.spelled[i] = texts
+			if src, ok := s.leadFrom[i]; ok && !capped(texts) {
+				ls.spelled[i] = leadSpelling(src)
+			}
+		}
+		leads = writtenPayloads(spelledViews(ls), refs)
+		return leads
+	}
+	return named, func(r int) func() []string {
+		return func() []string { return all()[r] }
 	}
+}
+
+// writtenPayloads is namedPayloads for the views given.
+func writtenPayloads(views []segment, refs []payloadRef) [][]string {
 	named := make([][]string, len(refs))
-	for _, v := range spelledViews(s) {
+	for _, v := range views {
 		nrefs := payloadRefsOf(v)
 		if len(nrefs) != len(refs) {
 			continue
@@ -372,15 +407,21 @@ func spelledViews(s segment) []segment {
 // bound, is spellCapped. A paired word also takes its segment.varLead from
 // the readings it was paired with, any one of which opening a path with a
 // variable that can be empty marks it: the mark-view word holds the value's
-// mark, whose name it no longer has (opensUnguardedPath). Nothing else of
-// psegs is changed.
-func spellPayload(psegs []segment, named []string) {
+// mark, whose name it no longer has (opensUnguardedPath). Where one of named
+// 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) {
 	paired := make([]map[int][]string, len(psegs))
 	leads := make([]map[int]bool, len(psegs))
-	for _, nm := range named {
+	// pair calls fn for each word of psegs the string nm's reading pairs,
+	// with that reading's segment and the word's texts in it.
+	pair := func(nm string, fn func(i, j int, n segment, ws []string)) {
 		nsegs, err := tokenize(nm)
 		if err != nil || len(nsegs) != len(psegs) {
-			continue
+			return
 		}
 		for i := range psegs {
 			m, n := psegs[i], nsegs[i]
@@ -403,17 +444,46 @@ func spellPayload(psegs []segment, named []string) {
 				if !fitsWritten(m.tokens[j], n.tokens[j]) {
 					continue
 				}
-				if leads[i] == nil {
-					leads[i] = map[int]bool{}
+				fn(i, j, n, ws)
+			}
+		}
+	}
+	markLead := func(i, j int, n segment, _ []string) {
+		if leads[i] == nil {
+			leads[i] = map[int]bool{}
+		}
+		leads[i][j] = leads[i][j] || n.varLead[j]
+	}
+	for _, nm := range named {
+		pair(nm, func(i, j int, n segment, ws []string) {
+			markLead(i, j, n, ws)
+			for _, w := range ws {
+				if fitsWritten(psegs[i].tokens[j], w) {
+					if paired[i] == nil {
+						paired[i] = map[int][]string{}
+					}
+					paired[i][j] = appendText(paired[i][j], w)
 				}
-				leads[i][j] = leads[i][j] || n.varLead[j]
-				for _, w := range ws {
-					if fitsWritten(m.tokens[j], w) {
-						if paired[i] == nil {
-							paired[i] = map[int][]string{}
-						}
-						paired[i][j] = appendText(paired[i][j], w)
+			}
+		})
+	}
+	if lead != nil && anyLead(leads) {
+		if ls := lead(); len(ls) > 0 {
+			// A word no guarded reading pairs keeps the lead named gave it.
+			named := leads
+			leads = make([]map[int]bool, len(psegs))
+			for _, nm := range ls {
+				pair(nm, markLead)
+			}
+			for i, words := range named {
+				for j, lead := range words {
+					if _, ok := leads[i][j]; ok {
+						continue
+					}
+					if leads[i] == nil {
+						leads[i] = map[int]bool{}
 					}
+					leads[i][j] = lead
 				}
 			}
 		}
@@ -440,6 +510,18 @@ func spellPayload(psegs []segment, named []string) {
 	}
 }
 
+// anyLead reports whether any word of leads is marked.
+func anyLead(leads []map[int]bool) bool {
+	for _, words := range leads {
+		for _, lead := range words {
+			if lead {
+				return true
+			}
+		}
+	}
+	return false
+}
+
 // fitsWritten reports whether word, read from a string's named text, can be
 // marked, read from its mark view, at the same place: marked's known text,
 // split at its marks, stands in word in the same order, the first part

internal/core/guard/tokenize.go

@@ -108,6 +108,11 @@ type segment struct {
 	// an entry's arg_shapes read it (ShapeUnguardedVariablePath). nil when no
 	// 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.
+	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
 	// is paid once per segment rather than once per entry. A segment built
@@ -497,8 +502,10 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) {
 		curVarAt []varSite
 		// splits rides with the segment (segment.ifsSplit).
 		splits map[int]bool
-		// leads rides with the segment (segment.varLead).
-		leads map[int]bool
+		// leads rides with the segment (segment.varLead), and leadFrom with
+		// it as segment.leadFrom.
+		leads    map[int]bool
+		leadFrom map[int]leadSource
 		// curMask is parallel to cur and records, per byte, whether it reached
 		// the tokenizer unquoted (wordStruct) and whether it began its word
 		// (wordRawStart) — what the brace expander needs to read a word the way
@@ -805,6 +812,12 @@ 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 leadFrom == nil {
+					leadFrom = map[int]leadSource{}
+				}
+				leadFrom[len(toks)] = leadSource{word: bytes.Clone(cur), sites: append([]varSite(nil), curVarAt...)}
+			}
 		case isUnknown(word):
 			spells[len(toks)] = []string{unknownText}
 		}
@@ -919,7 +932,7 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) {
 				tokens: toks, chain: chain, braceGroup: braceGroup, globbed: globsOrNil(globs),
 				stdinStream: curStdin || pipeNext || len(groupIn) > 0, literal: lits, feeds: feeds, piped: piped,
 				stdinIn: groupIn, home: list, at: len(segs), variable: vars, spelled: spells,
-				ifsSplit: splits, varLead: leads, redirects: curRedirs, afterAnd: andNext, end: pos,
+				ifsSplit: splits, varLead: leads, leadFrom: leadFrom, redirects: curRedirs, afterAnd: andNext, end: pos,
 			})
 			curRedirs, andNext = nil, false
 			toks = nil
@@ -929,6 +942,7 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) {
 			spells = nil
 			splits = nil
 			leads = nil
+			leadFrom = nil
 			feeds = nil
 			braceGroup = false
 			pipeNext = false
@@ -1075,7 +1089,7 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) {
 	openSubstitution := func(kind parenKind, pos int, procSub bool) {
 		saved := &enclosing{
 			toks: toks, globs: globs, lits: lits, vars: vars, curVar: curVar, curSub: curSub,
-			spells: spells, curVarAt: curVarAt, splits: splits, leads: leads,
+			spells: spells, curVarAt: curVarAt, splits: splits, leads: leads, leadFrom: leadFrom,
 			cur: cur, curMask: curMask, hasCur: hasCur, curGlob: curGlob,
 			curBrace: curBrace, braceGroup: braceGroup, chain: chain, procSub: procSub,
 			curStdin: curStdin, pipeNext: pipeNext, curDocs: curDocs, pieces: curPieces,
@@ -1085,7 +1099,7 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) {
 		toks, globs, lits, cur, curMask, hasCur, curGlob, curBrace, braceGroup = nil, nil, nil, nil, nil, false, false, false, false
 		curRedirs, andNext = nil, false
 		curPieces, vars, curVar, curSub = nil, nil, false, false
-		spells, curVarAt, splits, leads = nil, nil, nil, nil
+		spells, curVarAt, splits, leads, leadFrom = nil, nil, nil, nil, nil
 		// A substitution is a command string of its own: its pipelines begin
 		// inside it. Its standard input is its command's: what was piped into
 		// the groups around it, and the pipe into the command it sits in
@@ -1157,7 +1171,7 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) {
 		curStdin, pipeNext, curDocs, curPieces = e.curStdin, e.pipeNext, e.curDocs, e.pieces
 		feeds, curFeeds, pipeFrom, braceFrom, groupIn = e.feeds, e.curFeeds, e.pipeFrom, e.braceFrom, e.groupIn
 		vars, curVar, curSub = e.vars, e.curVar, e.curSub
-		spells, curVarAt, splits, leads = e.spells, e.curVarAt, e.splits, e.leads
+		spells, curVarAt, splits, leads, leadFrom = e.spells, e.curVarAt, e.splits, e.leads, e.leadFrom
 		curRedirs, andNext = e.redirs, e.andNext
 		resumeDocs(e)
 		if !f.bare {
@@ -1182,7 +1196,7 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) {
 		curStdin, pipeNext, curDocs, curPieces = e.curStdin, e.pipeNext, e.curDocs, e.pieces
 		feeds, curFeeds, pipeFrom, braceFrom, groupIn = e.feeds, e.curFeeds, e.pipeFrom, e.braceFrom, e.groupIn
 		vars, curVar, curSub = e.vars, e.curVar, e.curSub
-		spells, curVarAt, splits, leads = e.spells, e.curVarAt, e.splits, e.leads
+		spells, curVarAt, splits, leads, leadFrom = e.spells, e.curVarAt, e.splits, e.leads, e.leadFrom
 		curRedirs, andNext = e.redirs, e.andNext
 		feedFrom(e.segStart)
 		if e.procSub {
@@ -1213,7 +1227,7 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) {
 		addVar(spellParameter(body, split)...)
 		site := &curVarAt[len(curVarAt)-1]
 		site.split = split
-		site.guarded = guardedValue(body)
+		site.guarded = guardedValues(body)
 		site.transform = transformsValue(body)
 		if len(segs) > start {
 			curSub = true
@@ -2406,6 +2420,7 @@ type enclosing struct {
 	curVarAt   []varSite
 	splits     map[int]bool
 	leads      map[int]bool
+	leadFrom   map[int]leadSource
 	cur        []byte
 	curMask    []byte
 	hasCur     bool