Skip to content

Commit f413267

Browse files
committed
fix(code-placeholders): keep integer attributes across re-declaration and unset -f, guard shell values against subscript expansions
1 parent 2f3e963 commit f413267

2 files changed

Lines changed: 128 additions & 14 deletions

File tree

‎apps/sim/lib/execution/code-placeholders/compiler.test.ts‎

Lines changed: 62 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1058,6 +1058,8 @@ describe('code placeholder compiler', () => {
10581058
"echo $(( $(cat <<-'EOF'\n\t{{KEY}}\n\tEOF\n) + 1 ))",
10591059
'[[ {{KEY}} -eq 0 ]] && echo zero',
10601060
'[[ "{{KEY}}" -eq 0 ]] && echo zero',
1061+
'declare -i n; declare -a n; n={{KEY}}',
1062+
'declare -i n; unset -f n; n={{KEY}}',
10611063
'if [[ 0 -lt {{KEY}} ]]; then echo positive; fi',
10621064
'[[ -n x && ( "{{KEY}}" -ge 1 ) ]]',
10631065
'[[ $(printf "%s" "{{KEY}}") -ne 0 ]]',
@@ -1136,7 +1138,7 @@ describe('code placeholder compiler', () => {
11361138
})
11371139

11381140
it('preserves shell literal arithmetic text and leaves completed arithmetic frames', async () => {
1139-
const value = 'values[$(printf injected >&2)]'
1141+
const value = 'values[1]'
11401142
const compiled = await compileCodePlaceholders({
11411143
code: [
11421144
'printf "%s\\n" "{{KEY}}"',
@@ -1180,6 +1182,7 @@ describe('code placeholder compiler', () => {
11801182
'$(let x=1 <<EOF\n{{KEY}}\nEOF\n)',
11811183
// biome-ignore lint/suspicious/noTemplateCurlyInString: shell parameter expansion, not a JS template
11821184
'declare -A m; m[{{KEY}}]=x; printf "%s\\n" "${m[{{KEY}}]}"',
1185+
// biome-ignore lint/suspicious/noTemplateCurlyInString: shell parameter expansion, not a JS template
11831186
'declare -A m; printf "%s\\n" "${m[{{KEY}}]}"',
11841187
'declare -iA m; m[{{KEY}}]=1',
11851188
'read -p "items[{{KEY}}]" name',
@@ -1198,7 +1201,7 @@ describe('code placeholder compiler', () => {
11981201
compileCodePlaceholders({
11991202
code,
12001203
language: CodeLanguage.Shell,
1201-
environmentVariables: { KEY: 'values[$(printf injected >&2)]' },
1204+
environmentVariables: { KEY: 'values[1]' },
12021205
})
12031206
).resolves.toBeDefined()
12041207
})
@@ -1214,7 +1217,7 @@ describe('code placeholder compiler', () => {
12141217
})
12151218

12161219
it('compiles shell placeholders beside arithmetic that never evaluates them', async () => {
1217-
const value = 'values[$(printf injected >&2)]'
1220+
const value = 'values[1]'
12181221
const compiled = await compileCodePlaceholders({
12191222
code: [
12201223
'[ "{{KEY}}" -eq 0 ] 2>/dev/null || printf "%s\\n" not-zero',
@@ -1249,6 +1252,62 @@ describe('code placeholder compiler', () => {
12491252
)
12501253
})
12511254

1255+
it.each([
1256+
['an expansion in a subscript', 'a[$(cmd)]', 'an array subscript with an expansion'],
1257+
[
1258+
'a quoted bracket ending the walk early',
1259+
'a["]$(cmd)"]',
1260+
'an array subscript with an expansion',
1261+
],
1262+
['a backtick in a subscript', 'a[`cmd`]', 'an array subscript with an expansion'],
1263+
['a process substitution in a subscript', 'a[<(cmd)]', 'an array subscript with an expansion'],
1264+
['an escaped expansion in a subscript', 'a[\\$x]', 'an array subscript with an expansion'],
1265+
['an unclosed subscript', 'a[', 'an unbalanced array subscript'],
1266+
['a stray closing bracket', '$(cmd)]', 'an unbalanced array subscript'],
1267+
['an open quote in a subscript', 'a["]', 'an unbalanced array subscript'],
1268+
])('refuses a shell value with %s wherever it lands', async (_case, value, reason) => {
1269+
for (const code of ['printf "%s\\n" "{{KEY}}"', "cat <<'EOF'\n{{KEY}}\nEOF"]) {
1270+
await expect(
1271+
compileCodePlaceholders({
1272+
code,
1273+
language: CodeLanguage.Shell,
1274+
environmentVariables: { KEY: value },
1275+
})
1276+
).rejects.toThrow(reason)
1277+
}
1278+
})
1279+
1280+
it.each(['values[1]', '["a", "b"]', '{"items": [1, 2]}', '$HOME and $(date)', "it's [done]"])(
1281+
'compiles a shell value whose subscripts hold no expansion: %s',
1282+
async (value) => {
1283+
const compiled = await compileCodePlaceholders({
1284+
code: 'printf "%s\\n" "{{KEY}}"',
1285+
language: CodeLanguage.Shell,
1286+
environmentVariables: { KEY: value },
1287+
})
1288+
expect(executeShell(compiled.code, compiled.bindings)).toBe(`${value}\n`)
1289+
}
1290+
)
1291+
1292+
it('refuses arithmetic positions whose values are safe alone', async () => {
1293+
// Code can supply the brackets, or split a subscript across adjacent placeholders, so the value
1294+
// guard cannot judge these alone; the position scan refuses them.
1295+
await expect(
1296+
compileCodePlaceholders({
1297+
code: 'total=$(( a[{{KEY}}] ))',
1298+
language: CodeLanguage.Shell,
1299+
environmentVariables: { KEY: '$(cmd)' },
1300+
})
1301+
).rejects.toThrow('is not supported in shell arithmetic')
1302+
await expect(
1303+
compileCodePlaceholders({
1304+
code: 'total=$(( {{LEFT}}{{RIGHT}} ))',
1305+
language: CodeLanguage.Shell,
1306+
environmentVariables: { LEFT: 'a', RIGHT: '1' },
1307+
})
1308+
).rejects.toThrow('is not supported in shell arithmetic')
1309+
})
1310+
12521311
it('discovers shell arithmetic placeholders without compiling missing values', async () => {
12531312
const code = 'total=$(( {{MISSING}} + {{KEY}} ))'
12541313
await expect(analyzeCodePlaceholders(code, CodeLanguage.Shell)).resolves.toEqual([

‎apps/sim/lib/execution/code-placeholders/shell.ts‎

Lines changed: 66 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,8 @@ interface ShellCommandScan {
9292
indexedOption?: boolean
9393
/** The command is a builtin (`unset`/`read`/…) whose name arguments carry arithmetic subscripts. */
9494
nameArgumentBuiltin?: boolean
95+
/** An `unset -f` was read, so its arguments name functions and leave variable attributes alone. */
96+
unsetsFunctions?: boolean
9597
/** True once the command word (past any `name=value` assignment prefix) has been read. */
9698
sawCommandWord: boolean
9799
/** A non-arithmetic assignment prefix is being read, so a later keyword must not mark its value. */
@@ -545,6 +547,45 @@ function shellExpansion(name: string, quote: ShellQuote): string {
545547
return expansion
546548
}
547549

550+
/** Whether an expansion (`$…`, a backtick, `<(`/`>(`) starts at `index`. */
551+
function expansionStarts(value: string, index: number): boolean {
552+
const character = value[index]
553+
if (character === '$' || character === '`') return true
554+
return (character === '<' || character === '>') && value[index + 1] === '('
555+
}
556+
557+
/**
558+
* The reason a value could run a command if it reached arithmetic evaluation, or undefined if it
559+
* cannot. Arithmetic expands an array subscript again, so a subscript holding an expansion runs it.
560+
* This backs up the position scan: any `[ … ]` holding an expansion is refused, as is an
561+
* unbalanced bracket or an open quote inside one, since bash would then find the subscript's end in
562+
* surrounding code. Quotes and escapes count inside a subscript, so a quoted `]` does not end it.
563+
*/
564+
function unsafeArithmeticSubscript(value: string): string | undefined {
565+
let depth = 0
566+
let quote: '"' | "'" | undefined
567+
for (let index = 0; index < value.length; index += 1) {
568+
const character = value[index]
569+
if (depth > 0 && expansionStarts(value, index)) return 'an array subscript with an expansion'
570+
if (quote === "'") {
571+
if (character === "'") quote = undefined
572+
} else if (character === '\\') {
573+
index += 1
574+
if (depth > 0 && expansionStarts(value, index)) return 'an array subscript with an expansion'
575+
} else if (quote === '"') {
576+
if (character === '"') quote = undefined
577+
} else if (depth > 0 && (character === '"' || character === "'")) {
578+
quote = character
579+
} else if (character === '[') {
580+
depth += 1
581+
} else if (character === ']') {
582+
if (depth === 0) return 'an unbalanced array subscript'
583+
depth -= 1
584+
}
585+
}
586+
return depth > 0 || quote ? 'an unbalanced array subscript' : undefined
587+
}
588+
548589
function isLegacyShellPlaceholder(occurrence: CodePlaceholderOccurrence): boolean {
549590
const inner = occurrence.raw.slice(2, -2)
550591
return inner.trim() === occurrence.name && SHELL_BARE_NAME.test(occurrence.name)
@@ -756,12 +797,14 @@ function collectShellOccurrenceContexts<T extends ShellSpan>(
756797
/** A subscript frame for `name`'s array: a string key for an associative array, else an arithmetic index. */
757798
const subscriptFrameFor = (name: string | undefined) =>
758799
subscriptFrame(isAssociativeArray(name ?? '') ? 'keysubscript' : 'arithmetic')
759-
/** Forget a name's tracked type in every scope — on `unset`, or a re-declaration that changes it. */
800+
/** Forget a name's tracked array type in every scope — a re-declaration as `-a`/`-A` resets it. */
801+
const clearArrayType = (name: string) => {
802+
for (const frame of frames) frame.associativeArrays?.delete(name)
803+
}
804+
/** Forget everything tracked about a name in every scope — `unset` removes its attributes too. */
760805
const clearNameType = (name: string) => {
761-
for (const frame of frames) {
762-
frame.associativeArrays?.delete(name)
763-
frame.integerAttribute?.delete(name)
764-
}
806+
clearArrayType(name)
807+
for (const frame of frames) frame.integerAttribute?.delete(name)
765808
}
766809
/** Whether a `>`/`<` redirect operator immediately precedes `at`, across any intervening blanks. */
767810
const precededByRedirect = (at: number): boolean => {
@@ -895,19 +938,23 @@ function collectShellOccurrenceContexts<T extends ShellSpan>(
895938
) {
896939
const declared = SHELL_NAME.exec(word)?.[0]
897940
if (declared) {
898-
// A re-declaration resets the name's type before this one's attributes apply, so a later
899-
// `declare -a` (indexed) clears an earlier `-A` (associative) and vice versa.
900-
if (command.associativeOption || command.indexedOption) clearNameType(declared)
941+
// A re-declaration resets the name's array type before this one's attributes apply, so a
942+
// later `declare -a` (indexed) clears an earlier `-A` (associative) and vice versa. The
943+
// integer attribute survives it — only `+i` or `unset` removes that.
944+
if (command.associativeOption || command.indexedOption) clearArrayType(declared)
901945
if (command.associativeOption) (frame.associativeArrays ??= new Set()).add(declared)
902946
if (command.integerOption) {
903947
;(frame.integerAttribute ??= new Map()).set(declared, command.integerOption === 'set')
904948
}
905949
}
906950
}
907951
// `unset name` removes the variable and its attributes, so a later indexed reuse is arithmetic
908-
// again. A bare name only — `unset name[i]` removes one element, not the array's type.
909-
if (command.nameArgumentBuiltin && command.sawCommandWord && SHELL_BARE_NAME.test(word)) {
910-
clearNameType(word)
952+
// again. A bare name only — `unset name[i]` removes one element, not the array's type — and not
953+
// under `-f`, which removes a function of that name and leaves the variable alone.
954+
if (command.nameArgumentBuiltin && command.sawCommandWord) {
955+
const option = DECLARATION_OPTION.exec(word)
956+
if (option?.[1] === '-' && option[2].includes('f')) command.unsetsFunctions = true
957+
else if (!command.unsetsFunctions && SHELL_BARE_NAME.test(word)) clearNameType(word)
911958
}
912959
// An assignment is one only in command-prefix or declaration-argument position; `echo n=1` or
913960
// `printf a[i]=1` passes an ordinary string that bash never evaluates.
@@ -1435,6 +1482,14 @@ export async function compileShellPlaceholders(
14351482
occurrence.start
14361483
)
14371484
}
1485+
const unsafeSubscript = resolved && unsafeArithmeticSubscript(resolved.value)
1486+
if (unsafeSubscript && !input.analysisOnly) {
1487+
throw new CodePlaceholderCompileError(
1488+
`Variable placeholder "${occurrence.name}" cannot contain ${unsafeSubscript} in shell code`,
1489+
input.code,
1490+
occurrence.start
1491+
)
1492+
}
14381493
return resolved
14391494
}
14401495
const resolveShellOccurrence = (occurrence: CodePlaceholderOccurrence) =>

0 commit comments

Comments
 (0)