diff --git a/.abcd/work/issues/open/iss-2610110025455728-guard-misses-an-alternative-in-a-string.md b/.abcd/work/issues/open/iss-2610110025455728-guard-misses-an-alternative-in-a-string.md new file mode 100644 index 000000000..b46b5e4e4 --- /dev/null +++ b/.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. diff --git a/.abcd/work/issues/open/iss-2610100938485695-the-rm-unguarded-variable-path-guard.md b/.abcd/work/issues/resolved/iss-2610100938485695-the-rm-unguarded-variable-path-guard.md similarity index 70% rename from .abcd/work/issues/open/iss-2610100938485695-the-rm-unguarded-variable-path-guard.md rename to .abcd/work/issues/resolved/iss-2610100938485695-the-rm-unguarded-variable-path-guard.md index 63f64d049..d34f58907 100644 --- a/.abcd/work/issues/open/iss-2610100938485695-the-rm-unguarded-variable-path-guard.md +++ b/.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). diff --git a/internal/core/guard/defaults/guard.json b/internal/core/guard/defaults/guard.json index b62ffb084..bdc142922 100644 --- a/internal/core/guard/defaults/guard.json +++ b/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\"", diff --git a/internal/core/guard/payload.go b/internal/core/guard/payload.go index 1c3c6c86a..a7b683a2d 100644 --- a/internal/core/guard/payload.go +++ b/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 diff --git a/internal/core/guard/tokenize.go b/internal/core/guard/tokenize.go index e8ed4825c..44b081e92 100644 --- a/internal/core/guard/tokenize.go +++ b/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 diff --git a/internal/core/guard/unguardedvar_test.go b/internal/core/guard/unguardedvar_test.go index fbacd0807..20d9def79 100644 --- a/internal/core/guard/unguardedvar_test.go +++ b/internal/core/guard/unguardedvar_test.go @@ -130,3 +130,110 @@ func TestArgShapesRejectsAnUnknownShape(t *testing.T) { t.Fatal("Validate accepted an unknown arg_shapes name") } } + +// TestRmGuardedRewriteInsideADoubleQuotedShellString — iss-2610100938485695 +// shape 1. Inside a double-quoted string the enclosing shell expands the +// variable, and `${X:?}` stops it there on an empty value, so the string's +// re-read of `${X:?}/y` names no path from the root; neither does a default +// that cannot be empty, nor a guard nested in a default. Written out for the +// re-read, `${X:?}` is the `${X}` it prints, which alone can be empty: the +// refusal told the agent to write what it had written. A default that can +// be empty, a plain reference, an escaped `\${X}` the string re-reads +// itself, and a name the string also writes unguarded still refuse. +func TestRmGuardedRewriteInsideADoubleQuotedShellString(t *testing.T) { + const id = "rm-unguarded-variable-path" + for _, cmd := range []string{ + `sh -c "rm -rf ${X:?}/y"`, + `bash -c "rm -f \"${VAR:?}\"/*"`, + `sh -c "rm -rf ${X:-/tmp}/y"`, + `sh -c "rm -rf ${X:=/tmp}/y"`, + `sh -c "rm -rf ${X:-${Y:?}}/y"`, + `sh -c "rm -rf ${X:-${Y:-/tmp}}/y"`, + } { + d := verdictOf(t, cmd) + if d.Verdict == VerdictBlock || contains(d.Matches, id) { + t.Errorf("Check(%q) = %q via %v, want no %s match", cmd, d.Verdict, d.Matches, id) + } + } + runVerdictCases(t, []verdictCase{ + {`sh -c "rm -rf ${X}/y"`, VerdictBlock, id}, + {`sh -c "rm -rf ${X:-}/y"`, VerdictBlock, id}, + {`sh -c "rm -rf ${X:-$Y}/y"`, VerdictBlock, id}, + {`sh -c "rm -rf ${X:-${Y}}/y"`, VerdictBlock, id}, + {`sh -c "rm -rf \${X}/y ${X:?}/a"`, VerdictBlock, id}, + {`sh -c "rm -rf ${X:?}/a ${X}/b"`, VerdictBlock, id}, + {`sh -c "rm -rf ${X:?}/a $X/b"`, VerdictBlock, id}, + // A default the string's shell empties is no guard there: its quotes + // come off (`${X:-""}`) and its unquoted blanks split (`${X:- }`). + {`sh -c "rm -rf ${X:-\"\"}/y"`, VerdictBlock, id}, + {`sh -c "rm -rf ${X:- }/y"`, VerdictBlock, id}, + {`sh -c "rm -rf ${X:-a }/y"`, VerdictBlock, id}, + {`sh -c "rm -rf ${X:-\$Y}/y"`, VerdictBlock, id}, + {`sh -c "rm -rf ${X:-\$(true)}/y"`, VerdictBlock, id}, + {"sh -c \"rm -rf ${X:-\\`true\\`}/y\"", VerdictBlock, id}, + // The home's own entry still reads the guarded spelling. + {`sh -c "rm -rf ${HOME:?}"`, VerdictBlock, "rm-rf-root-or-home"}, + {`sh -c "rm -rf '${HOME:?}'"`, VerdictBlock, "rm-rf-root-or-home"}, + }) +} + +// TestRmNestedGuardIsRead — iss-2610100938485695 shape 2. A default whose +// word is itself guarded (`${VAR:-${OTHER:?}}`, `${VAR:-${OTHER:-/tmp}}`) +// never prints nothing, so the path is not one from the root. A nested +// reference that can be empty still refuses, and so does a name the +// expansion also writes unguarded: `${VAR:-${VAR}}` prints VAR's empty value. +func TestRmNestedGuardIsRead(t *testing.T) { + const id = "rm-unguarded-variable-path" + runVerdictCases(t, []verdictCase{ + {`rm -rf "${VAR:-${OTHER:?}}"/y`, VerdictAllow, ""}, + {`rm -rf "${VAR:-${OTHER:-/tmp}}"/y`, VerdictAllow, ""}, + {`rm -rf "${VAR:=${OTHER:?}}"/y`, VerdictAllow, ""}, + {`rm -rf "${VAR:-${OTHER:?}/tmp}"/y`, VerdictAllow, ""}, + {`rm -rf "${VAR:-${OTHER}}"/y`, VerdictBlock, id}, + {`rm -rf "${VAR:-${OTHER:-}}"/y`, VerdictBlock, id}, + {`rm -rf "${VAR:-${OTHER-/tmp}}"/y`, VerdictBlock, id}, + {`rm -rf "${VAR:-${VAR}}"/y`, VerdictBlock, id}, + {`rm -rf "${VAR:-$VAR}"/y`, VerdictBlock, id}, + }) +} + +// TestRmVariableBeforeASubstitutionIsFollowed — iss-2610100938485695 shape +// 3. A command substitution can print nothing as the variable can, so +// `$VAR$(true)/x` is `/x` with VAR empty: the site is followed past the +// substitution to the `/`. An arithmetic expansion always prints a number, +// so `$VAR$((1))/x` is never a path from the root. +func TestRmVariableBeforeASubstitutionIsFollowed(t *testing.T) { + const id = "rm-unguarded-variable-path" + runVerdictCases(t, []verdictCase{ + {`rm -rf $VAR$(true)/x`, VerdictBlock, id}, + {"rm -rf $VAR`true`/x", VerdictBlock, id}, + {`rm -rf "$VAR$(true)"/x`, VerdictBlock, id}, + {`rm -rf "$VAR$(true)$(true)/x"`, VerdictBlock, id}, + {`rm -rf "$VAR$(true)$B"/x`, VerdictBlock, id}, + {`sh -c 'rm -rf $VAR$(true)/x'`, VerdictBlock, id}, + {`rm -rf $VAR$((1))/x`, VerdictAllow, ""}, + {`rm -rf "${VAR:?}$(true)"/x`, VerdictAllow, ""}, + {`rm -rf "$VAR$(date +%s)"`, VerdictAllow, ""}, + }) +} + +// TestRmDefaultThatSplitsAwayIsNoGuard — a default written unquoted is split +// on its blanks, so `${X:- }/y` with X unset is the field `/y` in bash, sh and +// dash, and `${X:-a }/y` is the fields `a` and `/y`: the blank leaves the `/` +// opening a field of its own. Quoted, the blank is text (`"${X:- }"/y` is +// ` /y`), and so is an escaped one (`${X:-\ }/y`) or an ANSI-C string's +// (`${X:-$'\x20'}/y`). +func TestRmDefaultThatSplitsAwayIsNoGuard(t *testing.T) { + const id = "rm-unguarded-variable-path" + runVerdictCases(t, []verdictCase{ + {`rm -rf ${X:- }/y`, VerdictBlock, id}, + {`rm -rf ${X:-a }/y`, VerdictBlock, id}, + {`rm -rf ${X:-${Y:?} }/y`, VerdictBlock, id}, + {`rm -rf ${X:-a ${Y:?}}/y`, VerdictAllow, ""}, + {`rm -rf "${X:- }"/y`, VerdictAllow, ""}, + {`rm -rf ${X:- a}/y`, VerdictAllow, ""}, + {`rm -rf ${X:-\ }/y`, VerdictAllow, ""}, + {`rm -rf ${X:-$'\x20'}/y`, VerdictAllow, ""}, + {`rm -rf ${X:-/tmp}/y`, VerdictAllow, ""}, + }) +} diff --git a/internal/core/guard/unknown.go b/internal/core/guard/unknown.go index d5e1cc20a..20d9eacd6 100644 --- a/internal/core/guard/unknown.go +++ b/internal/core/guard/unknown.go @@ -122,10 +122,10 @@ type varSite struct { bare bool split bool width int - // guarded is the text of the variable's own value (`${X}`) where the - // expansion never prints that value empty — `${X:?}`, `${X:-w}`, - // `${X:=w}` — and "" otherwise (guardedValue in varpath.go). - guarded string + // guarded is the texts of the variables' own values (`${X}`) the + // expansion never prints empty — `${X:?}`, `${X:-w}`, `${X:=w}`, and a + // guard nested in w — and nil otherwise (guardedValues in varpath.go). + guarded []string // transform records a trim, a replacement, a substring or a case // change (transformsValue in varpath.go). transform bool diff --git a/internal/core/guard/varpath.go b/internal/core/guard/varpath.go index 7b9fda3b6..654a0c331 100644 --- a/internal/core/guard/varpath.go +++ b/internal/core/guard/varpath.go @@ -1,6 +1,9 @@ package guard -import "strings" +import ( + "slices" + "strings" +) // varpath.go reads the one operand shape an entry's arg_shapes can name // (Pattern.ArgShapes): a word that opens with a variable whose value can be @@ -24,26 +27,59 @@ var argShapes = map[string]bool{ ShapeUnguardedVariablePath: true, } -// guardedValue is the text of the variable's own value, as spellParameter -// writes it (`${X}`), where the `${…}` whose text between the braces is body -// never prints that value empty, and "" otherwise: `${X:?}` stops the shell -// on an empty or unset X, and `${X:-w}` and `${X:=w}` print w then. Whether -// w can be empty is read from w's own texts, which the expansion's texts -// hold beside the value. A plain reference (`${X}`), a test without the -// colon (`${X?}`, `${X-w}`, `${X=w}`, which print a value set to empty), an -// alternative and a subscript keep "". An indirection is not read: the -// value it names is not X's. -func guardedValue(body string) string { - body = paramText(body) - indirect, n := paramNameEnd(body) - if indirect || n <= 0 { - return "" +// guardedValues is the texts of the variables' own values, as spellParameter +// writes them (`${X}`), that the `${…}` whose text between the braces is body +// never prints empty, and nil where there is none: `${X:?}` stops the shell +// on an empty or unset X, and `${X:-w}` and `${X:=w}` print w then, so X's +// own value is printed only when it is not empty — and so is the value of a +// guard nested in w (`${X:-${Y:?}}`, `${X:-${Y:-/tmp}}`). Whether w can be +// empty is read from w's own texts, which the expansion's texts hold beside +// the values. A text names a variable, not the expansion that printed it, so +// a name the body also writes any other way (`${X:-$X}`, `${X:-${X}}`, which +// print X's empty value) is not guarded. A plain reference (`${X}`), a test +// without the colon (`${X?}`, `${X-w}`, `${X=w}`, which print a value set to +// empty), an alternative, a subscript and an indirection guard nothing: the +// value an indirection names is not X's. Every `$` of the body is read, +// quoted or not, which can only take a name out. +func guardedValues(body string) []string { + if !strings.Contains(body, ":") { + return nil } - rest := body[n:] - if strings.HasPrefix(rest, ":?") || strings.HasPrefix(rest, ":-") || strings.HasPrefix(rest, ":=") { - return "${" + body[:n] + "}" + w := "${" + paramText(body) + "}" + guarded, unguarded := map[string]bool{}, map[string]bool{} + var names []string + for i := 0; i+1 < len(w); i++ { + if w[i] != '$' { + continue + } + if w[i+1] != '{' { + if e := simpleParamEnd(w, i+1); e > i+1 { + unguarded[w[i+1:e]] = true + } + continue + } + b := w[i+2:] + indirect, n := paramNameEnd(b) + if n <= 0 { + continue + } + name, rest := strings.TrimPrefix(b[:n], "!"), b[n:] + if !indirect && (strings.HasPrefix(rest, ":?") || strings.HasPrefix(rest, ":-") || strings.HasPrefix(rest, ":=")) { + if !guarded[name] { + names = append(names, name) + } + guarded[name] = true + continue + } + unguarded[name] = true + } + var out []string + for _, name := range names { + if !unguarded[name] { + out = append(out, "${"+name+"}") + } } - return "" + return out } // transformsValue reports whether the `${…}` whose text between the braces @@ -92,17 +128,23 @@ func opensUnguardedPath(word []byte, sites []varSite) bool { // siteOpensPath reports whether the site sites[k] can print nothing, or a // text that opens with a variable that can, where what follows in the word // or the text is `/` — or is another site of which the same holds -// (`"$A$B"/x`). A transformsValue expansion is not read. Each of the -// expansion's texts is read: the value a guardedValue expansion guards is -// skipped, and a text past a bound is read -// as able to print nothing, so a bound refuses only where a `/` follows. A -// site whose texts are not known is not read: a raw 0x01 byte at the top of -// a line is text, and a mark a payload's text carries from the enclosing +// (`"$A$B"/x`). A command substitution between them can print nothing too, +// and is stepped over (`$VAR$(true)/x`); an arithmetic expansion always +// prints a number, which the word holds as text. A transformsValue expansion +// is not read. Each of the expansion's texts is read: a value a +// guardedValues expansion guards cannot be empty, and a text past a bound is +// read as able to print nothing, so a bound refuses only where a `/` follows. +// A site whose texts are not known is not read: a raw 0x01 byte at the top +// of a line is text, and a mark a payload's text carries from the enclosing // shell is read from the string with its variables written out instead // (spellPayload). func siteOpensPath(word []byte, sites []varSite, k int) bool { s := sites[k] next := s.at + max(s.width, 1) + siteAt := func(p int) bool { return k+1 < len(sites) && sites[k+1].at == p } + for next < len(word) && word[next] == unknownMark && !siteAt(next) { + next++ + } // followed is asked once whatever the number of texts, so a run of // sites is read once each, never once per combination of their texts. asked, answer := false, false @@ -115,7 +157,7 @@ func siteOpensPath(word []byte, sites []varSite, k int) bool { case next >= len(word): case word[next] == '/': answer = true - case k+1 < len(sites) && sites[k+1].at == next: + case siteAt(next): answer = siteOpensPath(word, sites, k+1) } return answer @@ -127,10 +169,13 @@ func siteOpensPath(word []byte, sites []varSite, k int) bool { return followed() } for _, t := range s.texts { - if s.guarded != "" && t == s.guarded { - continue + if s.split { + // An unquoted blank ends a field, so only the text after a + // text's last one runs on into what follows: `${X:- }/y` and + // `${X:-a }/y` leave `/y` a field of its own. + t = t[strings.LastIndexByte(t, fieldMark)+1:] } - rest, stripped := stripEmptyableRefs(t) + rest, stripped := stripEmptyableRefs(t, s.guarded) switch { case rest == "": if followed() { @@ -145,15 +190,16 @@ func siteOpensPath(word []byte, sites []varSite, k int) bool { // stripEmptyableRefs is t with its leading run of variable references that // can print nothing taken off (`$X`, `${X}`, `$1`), and whether any was. -// A reference that cannot be empty ends the run: a length (`${#X}`), the -// shell's own numbers (`$$`, `$?`, `$#`, `$0`), and the home and the working -// directory, which the login and the shell set to absolute paths and whose -// own entries name a delete of them. -func stripEmptyableRefs(t string) (string, bool) { +// A reference that cannot be empty ends the run: a value one of guarded +// names (guardedValues), a length (`${#X}`), the shell's own numbers (`$$`, +// `$?`, `$#`, `$0`), and the home and the working directory, which the login +// and the shell set to absolute paths and whose own entries name a delete of +// them. +func stripEmptyableRefs(t string, guarded []string) (string, bool) { stripped := false for { end, empty := leadingRef(t) - if end <= 0 || !empty { + if end <= 0 || !empty || slices.Contains(guarded, t[:end]) { return t, stripped } t, stripped = t[end:], true @@ -201,3 +247,90 @@ func refCanBeEmpty(body string) bool { } return !(len(body) > 1 && body[0] == '#') } + +// leadSource is a word as the tokenizer built it and its sites, kept for +// leadSpelling (segment.leadFrom). +type leadSource struct { + word []byte + sites []varSite +} + +// spellsGuards reports whether leadSpelling would write any of sites +// otherwise than spellWritten does. +func spellsGuards(sites []varSite) bool { + for _, s := range sites { + if respelled(s) { + return true + } + } + return false +} + +// respelled reports whether leadSpelling writes the site s as its guards. +func respelled(s varSite) bool { + if len(s.guarded) == 0 || capped(s.texts) { + return false + } + for _, t := range s.texts { + if emptiedInside(t, s.guarded) { + return false + } + } + return true +} + +// emptiedInside reports whether the string's shell, re-reading the text t a +// site printed in the enclosing one, can make it print nothing, or end a +// field before what follows: its quotes come off (`${X:-\"\"}` is `${X:-""}` +// there), its unquoted blanks split (`${X:- }`, whose text holds the blank as +// quotedFieldMark), and a reference the enclosing shell wrote as text is read +// (`${X:-\$Y}`). A guard written out as `${X:?}` would hide that default, so +// the site keeps its `${X}`, which can be empty. An escaped blank is read as +// one that splits, and a `$` or backquote opening nothing the guard follows +// as one that can print nothing (`${X:-\${Y}}`, whose body ends at the first +// `}`, and `${X:-\$(true)}`), refusing where the guard could allow. +func emptiedInside(t string, guarded []string) bool { + u := strings.Map(func(r rune) rune { + if r == '"' || r == '\'' || r == '\\' { + return -1 + } + return r + }, t) + if strings.ContainsAny(u, " \t\n"+fieldText+quotedFieldText) { + return true + } + rest, _ := stripEmptyableRefs(u, guarded) + if rest == "" { + return true + } + end, _ := leadingRef(rest) + return end == 0 && (rest[0] == '$' || rest[0] == '`') +} + +// leadSpelling is a word's spelling (spellWritten) for a payload re-read's +// varLead alone (spellPayload). +// segment.spelled writes the value a guard keeps from being empty as the +// `${X}` it prints, which the string re-read takes for a plain reference +// 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:-}`). +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, "}")+":?}") + } + texts[n] = t + } + lead[k].texts = texts + } + return spellWritten(src.word, lead, nil) +} diff --git a/internal/core/mdrecord/fence_shape_canonical_test.go b/internal/core/mdrecord/fence_shape_canonical_test.go index f3d172608..fae8b9977 100644 --- a/internal/core/mdrecord/fence_shape_canonical_test.go +++ b/internal/core/mdrecord/fence_shape_canonical_test.go @@ -25,6 +25,7 @@ import ( var fenceShapeReaders = map[string]fenceWriter{ "internal/core/positioning/check.go": {1, "emphasisRe strips inline emphasis and code markers from one tagline candidate, so a bolded tagline still reads as the tagline; it judges one string and tracks no lines"}, "internal/termsafe/prose.go": {2, "BlockText escapes a line-leading value's first byte when it would open a block (a backtick run, a tilde, a heading or list marker), so the rendered record keeps its text as prose; it judges one string and tracks no lines"}, + "internal/core/guard/varpath.go": {1, "emptiedInside reads whether a default's text opens with a backquote, which opens a command substitution in the shell string that re-reads it: a shell grammar, not markdown; it judges one string and tracks no lines"}, "internal/core/lifeboat/mdrender.go": {2, "escapeLeadingMarker escapes a value's first byte when it opens a block, a backtick run termsafe judges unbalanced or a tilde among the other markers, so a rendered field stays prose; it judges one string and tracks no lines"}, } diff --git a/internal/termsafe/codespan_canonical_test.go b/internal/termsafe/codespan_canonical_test.go index 32340773e..02c0a8499 100644 --- a/internal/termsafe/codespan_canonical_test.go +++ b/internal/termsafe/codespan_canonical_test.go @@ -33,6 +33,7 @@ var backtickScanners = map[string]backtickScanner{ "internal/core/capture/promote.go": {1, "a WRITER: codeSpan measures the longest backtick run to choose a fence the value cannot close; nothing is paired"}, "internal/core/guard/tokenize.go": {25, "the shell tokenizer: a backtick there is command substitution, a shell grammar, not markdown"}, "internal/core/guard/payload.go": {3, "targetsAnExpansion reads a shell line's raw text for an assignment target that holds an expansion: a backtick there opens or closes a command substitution, a shell grammar, not markdown; nothing is paired"}, + "internal/core/guard/varpath.go": {1, "emptiedInside reads whether a default's text opens with a backquote, a command substitution the shell string's own shell runs and which can print nothing: a shell grammar, not markdown; nothing is paired"}, "internal/core/guard/unknown.go": {2, "spellWord spells a default's or an alternative's shell word, and readPattern reads a trim's or a replacement's pattern: a backtick in either opens a command substitution, whose output the spelling drops or the pattern reads as unknown text; nothing is paired"}, "internal/core/history/reconstruct_render.go": {1, "a WRITER: longestBacktickRun sizes a fence longer than any run in the body; nothing is paired"}, "internal/core/lifeboat/mdrender.go": {1, "escapeLeadingMarker asks whether a value opens with a backtick, then asks OpensBalancedCodeSpan, which pairs through PairCodeSpan"},