T09 · Insecure Skill Coding Practices
Note
- Location
scripts/spirit.sh:66- Finding
Unsafe JavaScript Source Construction in the Rename Command
- Content
View full analysis
" exit 1 fi node -e " const fs=require('fs'); const c=JSON.parse(fs.readFileSync('$COMPANION_FILE','utf8')); c.name=$(printf '%q' "$new_name"); fs.writeFileSync('$COMPANION_FILE',JSON.stringify(c,null,2)); " ``` ### Technical Analysis The rename operation embeds user-controlled input directly into JavaScript source passed to `node -e`. The command uses Bash `printf '%q'` as an escaping mechanism, but `%q` produces shell-escaped text, not a valid JavaScript string literal. For example, a normal invocation such as: ```bash spirit rename Rune ``` produces an effective JavaScript statement equivalent to: ```javascript c.name=Rune; ``` Rather than assigning the string `"Rune"`, JavaScript treats `Rune` as an identifier and raises a `ReferenceError` unless it happens to be defined. Inputs that are valid JavaScript primitive expressions can change the type of the stored field. For example, `spirit rename true` results in `c.name=true`, and `spirit rename undefined` can cause the `name` property to disappear when the object is serialized. Other specially formatted input can make the generated JavaScript syntactically invalid and reliably terminate the command. The reviewed implementation does not establish shell-command execution from this issue because shell expansion results are not reparsed as shell syntax. Nevertheless, constructing executable JavaScript from user-controlled data is an unsafe coding pattern and permits denial of service or malformed companion state. ### Attack Path 1. An attacker or local user invokes the documented rename command with a JavaScript primitive or malform ...[truncated 1097 chars]- Remediation
View remediation
" exit 1 fi node - "$COMPANION_FILE" "$new_name" <<'NODE' const fs = require('fs'); const [file, name] = process.argv.slice(2); const companion = JSON.parse(fs.readFileSync(file, 'utf8')); if (typeof name !== 'string' || name.length === 0 || name.length > 100) { throw new Error('Invalid companion name'); } companion.name = name; fs.writeFileSync(file, JSON.stringify(companion, null, 2)); NODE ``` Additional hardening should include: 1. Apply the correction to both `scripts/spirit.sh` and `openclaw-spirits/scripts/spirit.sh`. 2. Enforce a reasonable maximum name length. 3. Reject control characters, including terminal escape characters and line breaks, if names are displayed in a terminal. 4. Validate the complete companion schema before writing it. 5. Write updates atomically through a temporary file followed by a rename operation to reduce the risk of partial state corruption. 6. Add regression tests covering ordinary names, spaces, quotation marks, Unicode, JavaScript keywords, shell metacharacters, and empty input. ]]>