T09 · Insecure Skill Coding Practices
Warning
- Location
- scripts/notify_closed_bottles.py:126
- Finding
- Corrupt Notifier State Silently Resets the Delivery Cursor## Vulnerability Details **File Location**: `scripts/notify_closed_bottles.py`, lines 126–134 and 392–393 **Vulnerability Type**: Improper state validation and fail-open cursor recovery **Risk Level**: Medium ### Vulnerable Code ```python def load_state() -> dict: p = state_file() p.parent.mkdir(parents=True, exist_ok=True) if p.exists(): try: return json.loads(p.read_text(encoding="utf-8")) except Exception: pass return {"enabled": True, "last_processed_line": 0} ``` The processing path also explicitly rewinds a cursor that exceeds the current ledger size: ```python cursor = state.get("last_processed_line", 0) if cursor > total: cursor = 0 ``` ### Technical Analysis The closed-bottle notifier treats every state read or JSON parsing failure as a fresh installation. It silently suppresses the exception, enables delivery, and resets `last_processed_line` to zero. It also fails to verify that the decoded value is a JSON object or that `enabled`, `last_processed_line`, and `delivered_ids` have valid types and values. Consequently, a truncated, malformed, unreadable, or deliberately replaced `closed_bottle_state.json` can erase the effective delivery cursor and deduplication history. The next `process` invocation scans the ledger from its beginning and reconstructs historical closed-bottle messages for external delivery. The separate `cursor > total` recovery has the same unsafe effect when the ledger is truncated or rotated: it automatically rewinds to zero instead of requiring explicit operator recovery. This behavior contrasts with `scripts/interagent_queue.py`, which rejects an existing but unusable state file rather than silently rewinding its cursor. ### Attack Path 1. The notifier processes closed bottles and records its cursor and delivered identifiers in `closed_bottle_state.json`. 2. An attacker with write access to that state pat ...[truncated 1742 chars]
- Remediation
- ## Remediation Suggestions 1. Fail closed when an existing state file cannot be read or parsed. Emit a structured error and exit with a nonzero status without sending messages or modifying the cursor. 2. Validate the complete state schema before use: - The top-level value must be a JSON object. - `enabled` must be a Boolean. - `last_processed_line` must be a non-negative integer and must not be a Boolean. - `delivered_ids` must be a list containing only strings. 3. Distinguish a genuinely absent state file from an existing but unusable state file. Only the absent-file case should initialize a fresh cursor. 4. Do not automatically reset a cursor greater than the ledger length. Treat ledger truncation or rotation as an exceptional condition and require explicit operator confirmation or a documented recovery command. 5. Reuse the fail-closed validation approach already implemented by `scripts/interagent_queue.py`. 6. Preserve atomic state updates and consider flushing and synchronizing the temporary file before replacement where durability across abrupt shutdowns is required. 7. Add regression tests covering malformed JSON, non-object JSON, negative or incorrectly typed cursors, invalid `delivered_ids`, unreadable state, and a cursor greater than the ledger length. Each case should verify that no external delivery occurs.