tsk-rll2pj [OPEN] Fix-forward PR 2202 (S2A follow-ups): slug fallbac - #2225
Conversation
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
|
Code layer reviewed and it is good - the GLOB tail bound with the 2-hex collision suffix handling is better than what I asked for, and the negative-test matrix is exactly right. Two items short of the original PR 2202 scope before this can supersede it: (1) merge origin/exec/tsk-f4nt53 into this branch (or cherry-pick its routes/init.py CSRF-dep hunk), and (2) add the docs/agent-coordination.md section for GET /api/share/destinations with the CORRECTED claims: caller is require_device only, discovery-only (a device token cannot write to the endpoints the old draft listed), and coverage is registry-backed agents only - plain deployed agents have no registry row and still resolve nothing. Re-push to this branch, do not open a new PR. On your push + green CI this merges and 2202 closes as superseded. |
PR Summary by QodoFix share destinations when DM members are stored as bare agent slugs
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
|
nemotron-ultra-orB review VERDICT: Clean implementation with comprehensive tests; no blocking issues found.
Automated first-pass review by the nemotron-ultra-orB lane. The lead still reviews before merge. |
|
nemotron-super review VERDICT: Potential correctness bug: get_by_slug does not escape GLOB special characters in slug, causing incorrect matching.
Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| if agent is None: | ||
| # DM channels store the agent slug (not its canonical_id) | ||
| # as the member, so slug members need a slug lookup. | ||
| agent = await registry.get_by_slug(member) |
There was a problem hiding this comment.
WARNING: False positive risk: get_by_slug fallback can incorrectly resolve user_ids as agents
When a DM channel contains a non-agent member (e.g., another user's user_id), registry.get(member) returns None, then get_by_slug(member) is called. If member happens to match the GLOB pattern {member}-[YYYYMMDD]-[HHMMSS]* for an existing agent (e.g., a user_id like "user-1" with a registered agent of the same slug), the user is incorrectly treated as an agent destination. Consider validating that member looks like a bare slug before applying the fallback.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| ).fetchone() | ||
| return _row_to_dict(row) if row else None | ||
|
|
||
| async def get_by_slug(self, slug: str, *, status: str = "active") -> Optional[dict]: |
There was a problem hiding this comment.
SUGGESTION: Type annotation status: str should be Optional[str]
The status parameter accepts None (to match any status) but is annotated as str. This is inconsistent with the docstring and could mislead type checkers.
| async def get_by_slug(self, slug: str, *, status: str = "active") -> Optional[dict]: | |
| async def get_by_slug(self, slug: str, *, status: Optional[str] = "active") -> Optional[dict]: |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # same-second slug collision, a -{2hex} suffix). GLOBing the 8-6 digit | ||
| # tail pins the slug to its own canonical-id boundary. | ||
| pattern = ( | ||
| f"{slug}-" |
There was a problem hiding this comment.
WARNING: GLOB special characters in slug are not escaped
If get_by_slug is called with a slug containing *, ?, [, or ], those characters are interpreted as GLOB wildcards rather than literals, producing incorrect matches. While _slugify produces safe values for internally generated slugs, this public method accepts arbitrary input and should escape GLOB metacharacters.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous Review Summary (commit 7add045)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 7add045)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (4 files)
Reviewed by step-3.7-flash · Input: 72K · Output: 29.9K · Cached: 535.6K |
Code Review by Qodo
1. Unsafe slug GLOB matching
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| pattern = ( | ||
| f"{slug}-" | ||
| "[0-9][0-9][0-9][0-9][0-9][0-9][0-9][0-9]" | ||
| "-" | ||
| "[0-9][0-9][0-9][0-9][0-9][0-9]" | ||
| "*" | ||
| ) | ||
| if status is None: | ||
| cursor = await self._db.execute( | ||
| "SELECT * FROM agent_registry WHERE canonical_id GLOB ? ORDER BY id LIMIT 1", | ||
| (pattern,), | ||
| ) | ||
| else: | ||
| cursor = await self._db.execute( | ||
| "SELECT * FROM agent_registry WHERE canonical_id GLOB ? AND status = ? ORDER BY id LIMIT 1", | ||
| (pattern, status), | ||
| ) |
There was a problem hiding this comment.
2. Unsafe slug glob matching 🐞 Bug ⛨ Security
AgentRegistryStore.get_by_slug builds a SQLite GLOB pattern from the provided slug and appends "*", which matches arbitrary suffixes beyond the documented optional 2-hex collision suffix; it also does not validate/escape GLOB metacharacters or scope by user_id. This can resolve the wrong agent (including cross-user) when slugs collide or when a crafted member contains GLOB wildcards like "*" or "?".
Agent Prompt
### Issue description
`AgentRegistryStore.get_by_slug()`:
- Uses `canonical_id GLOB ?` with a pattern ending in `*`, which is broader than the canonical format it documents.
- Interpolates `slug` into a GLOB pattern without validating/escaping metacharacters (`*`, `?`, `[...]`), allowing unintended matches if a channel member is not a real slug.
- Does not support scoping the lookup by `user_id`, even though registry rows carry `user_id`.
### Issue Context
Canonical IDs are minted as `{slug}-{YYYYMMDD}-{HHMMSS}` with an optional `-{2hex}` collision suffix. The repo already has an end-anchored regex for this canonical format. Also, chat channels can be created with arbitrary `members` via the admin route, so unsafe member strings can enter the system.
### Fix Focus Areas
- tinyagentos/agent_registry_store.py[542-580]
- tinyagentos/agent_registry_store.py[356-372]
- tinyagentos/agent_registry_store.py[438-505]
- tinyagentos/chat/orphan_reconcile.py[30-34]
- tinyagentos/routes/chat_admin.py[9-22]
### What to change
1. **Validate slug input** before building any pattern (e.g., allow only `^[a-z0-9]+(?:-[a-z0-9]+)*$`); if invalid, return `None`.
2. **Tighten the match** to only canonical forms:
- Either use two exact GLOB patterns (no trailing `*`):
- `{slug}-YYYYMMDD-HHMMSS`
- `{slug}-YYYYMMDD-HHMMSS-[0-9a-f][0-9a-f]`
- Or query by prefix and then apply the end-anchored canonical regex in Python to ensure the decoded slug equals the requested slug.
3. Add an optional `user_id` filter to `get_by_slug(..., user_id: str | None = None)` and include `AND user_id = ?` when provided.
4. Update the call site in `list_share_destinations` to pass the current user id when available.
### Acceptance criteria
- `get_by_slug('alpha')` cannot match `alpha-YYYYMMDD-HHMMSS-extra`.
- A member like `a*` does not broaden matches.
- In multi-user data, slug lookup can be constrained to the current user when needed.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
nemotron-ultra-kilo review VERDICT: Potential GLOB pattern injection in
Automated first-pass review by the nemotron-ultra-kilo lane. The lead still reviews before merge. |
…re-destinations docs
|
Completed the supersede myself for the same reason: cherry-ported the CSRF dep from exec/tsk-f4nt53 and wrote the CORRECTED share-destinations docs section (require_device only, discovery-only, registry-backed coverage). 136 route+store tests green locally. Merges on green CI, then 2202 closes. |
… gate get_by_slug input to slug shape (kills GLOB metachar surface)
|
Second fold round, both qodo/kilo Action-required findings adjudicated REAL and fixed: (1) a multi-user channel member that is a human username no longer falls through to slug resolution (auth.find_user guard - a username equal to an agent slug previously listed that agent as a share destination); (2) get_by_slug now rejects any input that is not strictly slug-shaped before building the GLOB pattern, with metachar tests (alpha[a-z], alpha*, alpha?, empty, uppercase). 137 tests green locally. Known gap, stated plainly: no route-level test exercises the human-username scenario because the destinations test fixture has no auth store wired; the guard is getattr-safe either way. Card that as a test follow-up if wanted. Kilo check state is the rate-limited fake-red; merging on qodo/gitar + my review once CI re-greens. |
| try: | ||
| if auth.find_user(member) is not None: | ||
| continue | ||
| except Exception: |
There was a problem hiding this comment.
WARNING: Broad except Exception around auth.find_user(member) could silently swallow errors
If find_user raises (e.g., DB error, misconfiguration), the exception is swallowed and the code falls through to get_by_slug, leaving the false-positive risk unmitigated. Narrow the caught exceptions to expected failures so genuine errors surface instead of being hidden.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
1 participant
Autonomous build of board card tsk-rll2pj.
Files:
tests/test_agent_registry_store.py | 69 +++++++++++++++++++++++++++
tests/test_routes_share_destinations.py | 83 +++++++++++++++++++++++++++++++++
tinyagentos/agent_registry_store.py | 40 ++++++++++++++++
tinyagentos/routes/share.py | 4 ++
4 files changed, 196 insertions(+)
Summary by Gitar
get_by_sluglookup method toAgentRegistryStorewith strict tail matching to prevent false prefix collisionslist_share_destinationsroute to fall back toget_by_slugwhen DM channel members are stored as bare slugsThis will update automatically on new commits.