fix(peer): address Kilo WARNINGs — os.environ leak, design doc, rate limiter docs#2032
fix(peer): address Kilo WARNINGs — os.environ leak, design doc, rate limiter docs#2032hognek wants to merge 2 commits into
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Next review available in: 21 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 (3)
📝 WalkthroughWalkthroughAdds SQLite-backed contact and peer-link storage, signed peer envelopes, bearer-authenticated ChangesPeer Channel
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| CREATE TABLE IF NOT EXISTS peer_links ( | ||
| contact_id TEXT PRIMARY KEY REFERENCES contacts(contact_id), | ||
| inbound_token_hash TEXT NOT NULL, -- token WE minted for their instance (SHA-256); indexed for lookup | ||
| outbound_token TEXT NOT NULL, -- token THEY minted for us (stored in plaintext; encryption deferred to post-MVP) |
There was a problem hiding this comment.
WARNING: outbound_token (the token the remote instance minted for us) is stored in plaintext.
This is a credential-at-rest risk: anyone with read access to contacts.db can impersonate this node when talking to the peer (calling their /api/peer/* endpoints as us). The PR explicitly defers encryption to post-MVP, but it should be tracked as a known accepted risk rather than silently accepted. Consider at minimum documenting the threat model/blast radius in the design doc, and hashing or encrypting this value before MVP launch.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # versions, so /ws/ remains exempt at the middleware layer; the per-endpoint | ||
| # check is the authoritative guard for all WebSocket endpoints. | ||
| EXEMPT_PREFIXES = ("/static/", "/desktop/", "/chat-pwa/", "/ws/", "/shortcut/") | ||
| EXEMPT_PREFIXES = ("/static/", "/desktop/", "/chat-pwa/", "/ws/", "/shortcut/", "/api/peer/") |
There was a problem hiding this comment.
WARNING: Adding /api/peer/ to EXEMPT_PREFIXES means the global auth/session/CSRF middleware is fully bypassed for the entire prefix. Correctness of the whole surface now depends on every route calling _authenticate_peer.
This is a fragile trust boundary: a future route added under /api/peer/ that forgets _authenticate_peer becomes completely unauthenticated and reachable instance-to-instance. The module docstring warns about this, but it is convention-only. Consider enforcing the bearer check centrally (e.g. a router-level dependency applied to all routes in routes/peer.py) so a missing per-route call cannot expose the surface.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| return True | ||
| except sqlite3.IntegrityError: | ||
| # Nonce already exists for this (contact_id, kind) — replay detected. | ||
| await self._db.rollback() |
There was a problem hiding this comment.
SUGGESTION: The rollback() here also undoes the opportunistic DELETE FROM peer_nonces WHERE seen_at < ? prune executed earlier in the same (uncommitted) transaction. On a replay (IntegrityError) the prune is silently lost for that call.
Low impact — the prune is best-effort housekeeping and will simply run on the next insert — but if you want pruning to be reliable on replays, commit() the prune before the INSERT, or split it into its own transaction. Not a correctness bug.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No New Issues Found (prior findings resolved/improved) | Recommendation: Merge Overview
Prior Findings — Re-verified Against Current CodeThis increment is a remediation pass on PR #2025's carry-over WARNINGs. All three changed-code findings from the previous review are now addressed:
No new defects were introduced by these changes. The router dependency correctly sets Files Reviewed (3 files this increment)
Previous Review Summaries (3 snapshots, latest commit c7f0a6a)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit c7f0a6a)Status: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (2 files this increment)
Incremental review of PR #2032 vs #2025. This PR is documentation-only (design doc wording + rate-limiter comment/FIXME); it does not introduce new code issues. The Previous review (commit 2a448af)Status: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (8 files)
Fix these issues in Kilo Cloud Previous review (commit d2937c8)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (8 files)
Reviewed by hy3:free · Input: 40.6K · Output: 3.3K · Cached: 149.2K |
|
Thanks for the peer cleanup. #2025 has now landed on dev, so this conflicts (8 files) and needs a rebase onto latest dev before I can take it. On the os.environ item: reading TAOS_DATA_DIR straight from the environment is a coupling nit, not a live leak, so no urgency on the merged code. Once rebased and CI is green I will fold and merge. |
d2937c8 to
2a448af
Compare
| if not _rate_limit_ok(contact_id): | ||
| raise HTTPException(status_code=429, detail="rate limit exceeded") | ||
|
|
||
| await store.mark_peer_seen(contact_id) |
There was a problem hiding this comment.
SUGGESTION: /api/peer/ack has no nonce/replay protection — unlike /inbox and /chat, record_nonce is never called here. A valid authenticated contact can replay POST /ack indefinitely and it always returns 200, only updating last_seen_at (line 335). The body.envelope_id is also never validated against any actually-sent envelope. Low impact today (the action is just a timestamp update + log), but worth either (a) recording the ack's envelope_id as a nonce via record_nonce for replay detection, or (b) explicitly documenting that /ack is idempotent-by-design and the envelope_id is informational only.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # Opportunistic eviction: when the dict exceeds the max size, sweep out | ||
| # every entry whose window has expired. This is O(n) but only runs | ||
| # occasionally when the dict is full. | ||
| if len(_rate_hits) >= _RATE_HITS_MAX_SIZE: |
There was a problem hiding this comment.
SUGGESTION: The eviction only sweeps entries whose window has expired. If the server ever has more than _RATE_HITS_MAX_SIZE (2000) distinct active contacts all within their 60s window (e.g. a busy hub with >2000 peers each sending <60 req/min), len(_rate_hits) >= _RATE_HITS_MAX_SIZE is true but expired is empty, so nothing is evicted and the dict grows unbounded. For human-scale contact graphs this is unlikely, but the guard does not actually bound memory under sustained load. Consider also evicting the oldest windows (e.g. LRU) when over the cap, or failing closed/evicting least-recently-used when full.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tinyagentos/routes/peer.py (1)
230-305: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared envelope-validation flow.
peer_chatduplicates ~50 lines frompeer_inbox(size checks, sender-binding, recipient check,verify_envelope, signature verification, nonce recording,mark_peer_seen). A helper like_validate_and_record(request, store, contact_id, envelope, expected_kind=None)would remove the divergence risk (e.g. the size-guard fix above would otherwise need to be applied twice).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/routes/peer.py` around lines 230 - 305, Extract the duplicated envelope validation and recording logic from peer_chat and peer_inbox into a shared helper such as _validate_and_record, including size checks, sender and recipient binding, envelope verification, signature validation, nonce recording, and mark_peer_seen. Update both handlers to call the helper with their expected kind while preserving their existing HTTP errors and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tinyagentos/routes/peer.py`:
- Around line 153-170: The size limit in the peer request handling currently
runs after FastAPI deserializes body: PeerEnvelope, allowing oversized payloads
to consume memory first. Move enforcement into middleware or implement bounded
body streaming before request parsing, covering both the shown peer route and
peer_chat, while preserving the existing 400 invalid Content-Length and 413
oversized-request behavior.
---
Nitpick comments:
In `@tinyagentos/routes/peer.py`:
- Around line 230-305: Extract the duplicated envelope validation and recording
logic from peer_chat and peer_inbox into a shared helper such as
_validate_and_record, including size checks, sender and recipient binding,
envelope verification, signature validation, nonce recording, and
mark_peer_seen. Update both handlers to call the helper with their expected kind
while preserving their existing HTTP errors and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c3520835-cfce-4c0c-8bca-aa0857cfb988
📒 Files selected for processing (8)
docs/design/cross-user-collaboration.mdtests/test_contacts_peer.pytinyagentos/app.pytinyagentos/auth_middleware.pytinyagentos/contacts_store.pytinyagentos/peer.pytinyagentos/routes/__init__.pytinyagentos/routes/peer.py
| # Size limit — reject on Content-Length before we re-serialise. | ||
| cl = request.headers.get("content-length") | ||
| if cl is not None: | ||
| try: | ||
| cl_int = int(cl) | ||
| except ValueError: | ||
| raise HTTPException(status_code=400, detail="invalid Content-Length") | ||
| if cl_int > _MAX_ENVELOPE_BYTES: | ||
| raise HTTPException( | ||
| status_code=413, | ||
| detail=f"request too large: {cl_int} bytes > {_MAX_ENVELOPE_BYTES}", | ||
| ) | ||
| raw = json.dumps(body.envelope, separators=(",", ":"), ensure_ascii=False).encode("utf-8") | ||
| if len(raw) > _MAX_ENVELOPE_BYTES: | ||
| raise HTTPException( | ||
| status_code=413, | ||
| detail=f"envelope too large: {len(raw)} bytes > {_MAX_ENVELOPE_BYTES}", | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for any global request/body size limit middleware or server config.
rg -nP -C3 '(max_?(request|body|content)_?(size|length)|MAX_.*SIZE|limit_.*body|client_max_body)' --type=py
rg -nP -C3 'add_middleware' tinyagentos/app.pyRepository: jaylfc/taOS
Length of output: 149
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files and inspect the peer handlers plus app setup.
git ls-files tinyagentos/routes/peer.py tinyagentos/app.py tinyagentos | sed 's#^`#FILE` #'
echo '--- peer.py outline ---'
ast-grep outline tinyagentos/routes/peer.py --view expanded || true
echo '--- app.py outline ---'
ast-grep outline tinyagentos/app.py --view expanded || true
echo '--- peer.py excerpt ---'
sed -n '1,340p' tinyagentos/routes/peer.py | cat -n | sed -n '130,310p'
echo '--- app.py excerpt ---'
sed -n '1,220p' tinyagentos/app.py | cat -nRepository: jaylfc/taOS
Length of output: 46899
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any request-body limiting middleware or custom ASGI wrappers in the repo.
rg -n --hidden -S 'BaseHTTPMiddleware|Middleware\(|Request\.stream\(|receive\(\)|content-length|body.*limit|max.*body|max.*request|client_max_body|limit.*request' tinyagentos . --glob '!**/.git/**' || trueRepository: jaylfc/taOS
Length of output: 50368
Move the size cap ahead of request parsing tinyagentos/routes/peer.py:153-170. body: PeerEnvelope is already deserialized before this 413 runs, so an oversized request can still burn memory first. Enforce the cap in middleware or stream the body with a hard limit; the same pattern applies in peer_chat.
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 164-164: use jsonify instead of json.dumps for JSON output
Context: json.dumps(body.envelope, separators=(",", ":"), ensure_ascii=False)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 Ruff (0.15.21)
[warning] 159-159: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tinyagentos/routes/peer.py` around lines 153 - 170, The size limit in the
peer request handling currently runs after FastAPI deserializes body:
PeerEnvelope, allowing oversized payloads to consume memory first. Move
enforcement into middleware or implement bounded body streaming before request
parsing, covering both the shown peer route and peer_chat, while preserving the
existing 400 invalid Content-Length and 413 oversized-request behavior.
- Design doc: outbound_token comment now matches code (plaintext; deferred to post-MVP) instead of misleading 'encrypted at rest'. - Rate limiter: add explicit FIXME for shared-store backing across workers, document per-worker aggregate limit semantics. Note: monkeypatch.setenv fixture fix was already applied in upstream merge of jaylfc#2025, so this commit carries only the two remaining warnings.
2a448af to
c7f0a6a
Compare
…rate-limit LRU, prune commit, token docs Fix #1 (WARNING): Document outbound_token plaintext threat model in contacts_store schema comment — token must be presented on outbound requests; at-rest encryption deferred to post-MVP. Fix #2 (WARNING): Centralized /api/peer/ authentication via router-level _peer_auth_dep dependency. Previously EXEMPT_PREFIXES bypassed all auth middleware and correctness depended on every route calling _authenticate_peer. Now every route under /api/peer/ gets bearer-auth automatically; route handlers read contact_id from request.state. Fix jaylfc#3 (SUGGESTION): Commit the opportunistic nonce prune in its own transaction so a replay (IntegrityError) rollback does not undo it. Fix jaylfc#4 (SUGGESTION): Add record_nonce(…, kind='ack') to /api/peer/ack for replay protection. A replayed ack now returns 409 Conflict, matching the /inbox and /chat contract. Fix jaylfc#5 (SUGGESTION): Add LRU fallback eviction to the rate limiter. When all 2000+ entries have active windows (no expired entries to sweep), the oldest entry is evicted to prevent unbounded dict growth under sustained load.
2 participants
Summary
Addresses three Kilo WARNINGs carried over on #2025:
app_with_contacts fixture — mutated
os.environ["TAOS_DATA_DIR"]without restore. Fixed by usingmonkeypatch.setenv(scoped, auto-restored). Also removed now-unusedimport os.outbound_token plaintext — design doc (
cross-user-collaboration.md) claimedencrypted at restbut code saysstored in plaintext; encryption deferred to post-MVP. Aligned design doc to match code.Rate limiter — added explicit
FIXME(post-MVP)for shared-store backing across workers, and documented the per-worker aggregate limit semantics (60×N/min across N workers).Testing
pytest tests/test_contacts_peer.py -v)Related
Summary by CodeRabbit
New Features
Documentation
Tests