diff --git a/.abcd/work/issues/open/iss-2610110025455728-guard-misses-an-alternative-in-a-string.md b/.abcd/work/issues/resolved/iss-2610110025455728-guard-misses-an-alternative-in-a-string.md similarity index 82% rename from .abcd/work/issues/open/iss-2610110025455728-guard-misses-an-alternative-in-a-string.md rename to .abcd/work/issues/resolved/iss-2610110025455728-guard-misses-an-alternative-in-a-string.md index b46b5e4e4..fa3454ea7 100644 --- a/.abcd/work/issues/open/iss-2610110025455728-guard-misses-an-alternative-in-a-string.md +++ b/.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. diff --git a/internal/core/guard/defaultword_test.go b/internal/core/guard/defaultword_test.go index 73e88f34f..f23a3c3ec 100644 --- a/internal/core/guard/defaultword_test.go +++ b/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) { diff --git a/internal/core/guard/payload.go b/internal/core/guard/payload.go index a7b683a2d..1f0f6b725 100644 --- a/internal/core/guard/payload.go +++ b/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 diff --git a/internal/core/guard/tokenize.go b/internal/core/guard/tokenize.go index 44b081e92..4276ad507 100644 --- a/internal/core/guard/tokenize.go +++ b/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{} } diff --git a/internal/core/guard/unguardedvar_test.go b/internal/core/guard/unguardedvar_test.go index 20d9def79..014485eef 100644 --- a/internal/core/guard/unguardedvar_test.go +++ b/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, ""}, + }) +} diff --git a/internal/core/guard/varpath.go b/internal/core/guard/varpath.go index 654a0c331..618ef6339 100644 --- a/internal/core/guard/varpath.go +++ b/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) }