T09 · Insecure Skill Coding Practices
Warning
- Location
- cli.js:105
- Finding
- Store Delimiter Injection Can Corrupt Parsing and Erase Existing TODO Data<![CDATA[ ## Vulnerability Details **File Location**: `cli.js:105-120`, with the attacker-controlled fields introduced at `cli.js:216-237` **Vulnerability Type**: Unsafe delimiter-based parsing of attacker-controlled persistent data **Risk Level**: Medium ### Vulnerable Code ```js function readStore() { if (!fs.existsSync(TODOS_FILE)) return { ...EMPTY_STORE, tasks: [], recurringTasks: [] }; const content = fs.readFileSync(TODOS_FILE, "utf8"); const s = content.indexOf(STORE_START); const e = content.indexOf(STORE_END); if (s === -1 || e === -1) return { ...EMPTY_STORE }; try { return JSON.parse(content.slice(s + STORE_START.length, e).trim()); } catch { return { ...EMPTY_STORE }; } } function writeStore(store) { const json = JSON.stringify(store, null, 2); fs.writeFileSync(TODOS_FILE, renderMarkdown(store, json), "utf8"); } ``` The affected input fields are populated directly from command-line input: ```js function cmdAdd(pos, flags) { const title = pos[0] || flags["title"]; if (!title) return die('task title required: node cli.js add "<title>"'); const store = readStore(); const task = { id: nextId(store, "T"), title, notes: flags["notes"] || undefined, dueDate: flags["due"] || undefined, priority: flags["priority"] || "medium", status: "active", createdAt: new Date().toISOString(), tags: flags["tags"] ? flags["tags"].split(",").map(s => s.trim()) : [], }; store.tasks.push(task); writeStore(store); const due = task.dueDate ? ` (due ${task.dueDate})` : ""; const prio = task.priority !== "medium" ? ` [${task.priority}]` : ""; console.log(`Added ${task.id}: ${task.title}${due}${prio}`); } ``` ### Technical Analysis The application embeds a JSON document inside Markdown using the textual markers `<!-- STORE_JSON` and `STORE_JSON -->`. It retrieves the end of the JSON document with: ```js const e = content.indexOf(STORE_END); ``` Task ...[truncated 2456 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. **Separate machine-readable storage from rendered Markdown.** Persist authoritative state in a dedicated JSON file and generate `todos.md` only as a human-readable view. Parse the JSON file directly rather than extracting JSON with textual sentinels. 2. **Fail closed on malformed storage.** Do not return an empty store after a parsing error. Report the corruption, terminate with a nonzero status, and avoid all writes until the data has been recovered. ```js try { return JSON.parse(rawData); } catch (error) { throw new Error(`TODO store is corrupted: ${error.message}`); } ``` 3. **Reject reserved delimiters if embedded JSON must be retained.** Validate every string field, including titles, notes, and tags, and reject values containing `STORE_START` or `STORE_END`. This is a defense-in-depth measure rather than a substitute for eliminating delimiter-based parsing. 4. **Use an unambiguous extraction strategy.** If the existing format must remain compatible, locate and validate the genuine final marker rather than the first occurrence, and verify that the extracted content forms exactly one valid JSON document. This reduces exposure but remains less robust than separate storage. 5. **Implement atomic writes.** Write the new database to a temporary file in the same directory, flush it, and atomically rename it over the destination. This prevents partial writes from causing additional corruption. 6. **Preserve recoverable backups.** Before replacing a valid store, retain a restricted-permission backup. Never replace the current store when reading or schema validation has failed. 7. **Add regression tests.** Test titles, notes, and tags containing `STORE_JSON -->`, `<!-- STORE_JSON`, Markdown syntax, newlines, and Unicode. Verify that malformed storage causes a visible error and never results in an automatic overwrite. ]]>