🔧 update: improve shield SQL detection and CLI channel routing#69
🔧 update: improve shield SQL detection and CLI channel routing#69warengonzaga wants to merge 3 commits intodevfrom
Conversation
📦 Package Build Flow — Monorepo Build🔀 Pull Request Build — Pre-release package for testing PR changes
📥 Quick InstallThis package was built automatically by the Package Build Flow action. |
There was a problem hiding this comment.
Pull request overview
Improves reliability of the shield SQL-injection matcher by reducing false positives on document-like tool arguments and avoiding markdown --- being treated as SQL comments, and fixes CLI owner routing by registering a cli channel alias that maps to the existing web sender.
Changes:
- Shield: exclude common content-body fields (
content,body,message,text) from SQL keyword scanning intoolArgs - Shield: refine
--detection so markdown separators (---) don’t match - CLI: register
clichannel alias when persisted ownerId iscli:*so nudges route correctly
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/cli/src/commands/start.ts | Registers a cli gateway alias backed by the web SSE sender for CLI-prefixed owners |
| packages/shield/src/matcher.ts | Filters tool args before SQL scanning and tightens -- matching to avoid --- false positives |
| packages/shield/tests/matcher.test.ts | Adds regression tests for markdown separators/content fields and a true-positive guard |
| // Only scan fields that could be SQL injection vectors. | ||
| // Exclude content-body fields (document payloads written to files, | ||
| // not SQL parameters) to prevent false positives. | ||
| const CONTENT_FIELDS: ReadonlySet<string> = new Set([ | ||
| 'content', | ||
| 'body', | ||
| 'message', | ||
| 'text', | ||
| ]); |
There was a problem hiding this comment.
CONTENT_FIELDS is re-created as a new Set every time evaluateCondition() runs, which adds avoidable allocations in a hot path (shield matching can run per event × per directive). Consider hoisting this ReadonlySet to module scope (or at least outside the function) so it’s constructed once.
| // If the owner was claimed via CLI setup, the persisted ownerId has | ||
| // prefix "cli:" but only a "web" channel is registered. Register a | ||
| // "cli" alias so nudges for "cli:owner" route through the web sender. | ||
| if (persistedOwnerId?.startsWith('cli:') && !gateway.getRegisteredChannels().includes('cli')) { | ||
| gateway.register('cli', { | ||
| ...webSender, | ||
| name: `${webSender.name} (cli alias)`, | ||
| }); | ||
| } |
There was a problem hiding this comment.
This introduces new routing behavior (registering a cli alias when persistedOwnerId starts with cli:), but src/cli/tests/commands/start.test.ts doesn’t assert that the alias is registered. Adding a focused test that verifies gateway.register is called for the cli prefix would help prevent regressions in owner routing.
|
@copilot apply changes based on the comments in this thread |
…tests Agent-Logs-Url: https://github.com/warengonzaga/tinyclaw/sessions/826a02ef-4bef-4226-84ab-b5891dcc060d Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
Applied all changes from the review thread (commit
Also fixed a pre-existing Biome formatter failure in |
Summary
Two reliability improvements: the shield package's SQL injection matcher was generating false positives by scanning content-body fields (e.g.
content,body,message,text) that carry document payloads rather than SQL parameters, and the--markdown separator (---) was being misidentified as a SQL comment marker. Separately, the CLI now registers a channel alias to correctly handle CLI-prefixed owner routing.Changes
content,body,message,text) from SQL injection argument scanning to prevent false positives on document writes--detection with a negative-lookahead regex so markdown---separators are not treated as SQL comment markersTest Plan
packages/shield/tests/matcher.test.tscover all three scenarios:---inside acontentfield → no matchdelete,drop) inside acontentfield → no matchDROP TABLE … --inside a non-contentqueryfield → match (true positive preserved)pnpm --filter shield test