T09 · Insecure Skill Coding Practices
Error
- Location
- src/channel-bridge.js:50
- Finding
- Fail-Open and Substring-Based Filters Allow Unauthorized Cross-Channel Message Routing<![CDATA[ ## Vulnerability Details **File Location**: `src/channel-bridge.js:50-63` **Vulnerability Type**: Fail-open authorization filtering and imprecise identity matching **Risk Level**: High ### Vulnerable Code ```js _matchFilter(message, filter) { const text = ((message.body || '') + ' ' + (message.subject || '')).toLowerCase(); const from = (message.from || '').toLowerCase(); if (filter.includes('contains:')) { const term = filter.split('contains:')[1].split(' ')[0].toLowerCase(); if (text.includes(term)) return true; } if (filter.includes('from:')) { const sender = filter.split('from:')[1].split(' ')[0].toLowerCase(); if (from.includes(sender)) return true; } // Unknown filter types pass through (don't silently drop messages) if (!filter.includes('contains:') && !filter.includes('from:')) return true; return false; } ``` The affected behavior is exercised by the documented configuration in `SKILL.md:38-41`: ```yaml - name: "announcements" from: slack filter: "channel:#announcements" to: [discord, telegram, email] transform: "forward" ``` ### Technical Analysis The filter implementation fails open for every filter that does not contain the literal strings `contains:` or `from:`. The documented `channel:#announcements` expression is not implemented, so `_matchFilter()` returns `true` for it regardless of the actual source channel. Consequently, messages from any Slack channel can satisfy a route that appears to be restricted to `#announcements`. The `from:` implementation is also imprecise because it uses substring matching: ```js if (from.includes(sender)) return true; ``` A filter such as `from:boss` therefore accepts identities including `evilboss`, `boss-attacker`, or any other string containing `boss`. It does not compare a canonical account identifier or require an exact identity match. Filters govern whether a message is copied to route destinations, making them an authorization boundary for poten ...[truncated 1832 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Replace ad hoc string splitting with a strict parser for a documented filter grammar. 2. Explicitly implement every advertised filter type, including channel filters, before presenting it as supported. 3. Reject unknown, malformed, empty, or partially parsed filters during route creation. Filtering errors must fail closed. 4. Resolve platform-specific usernames and channels to canonical immutable identifiers where possible. 5. Compare sender and channel identifiers exactly rather than with `String.prototype.includes()`. 6. Define `AND` and `OR` precedence explicitly and reject expressions that cannot be parsed completely. 7. Validate route objects in `addRoute()` and the constructor, including `from`, `to`, `filter`, `transform`, and `schedule`. 8. Add regression tests proving that: - `channel:#announcements` rejects messages from every other channel. - `from:boss` rejects `evilboss` and `boss-attacker`. - Unknown and malformed filter types reject messages. - Mixed expressions follow their documented Boolean semantics. 9. Return or log a clear configuration error when a filter is unsupported instead of silently forwarding messages. ]]>