fix(store): fail-closed canonicalisation + TOCTOU re-verify hardening - #2186
Conversation
|
Warning Review limit reached
Next review available in: 5 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 (2)
📝 WalkthroughWalkthroughThe install endpoint now performs asynchronous, fail-closed manifest signature re-verification. Tests cover filesystem tampering, missing signatures, malformed YAML, signature mismatches, and YAML date serialization failures. ChangesStore install signing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
|
|
Duplicate of #2050 — same branch, wrong PR number. Closing. |
PR Summary by QodoFail-closed manifest canonicalisation + async TOCTOU signature re-verify
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1. Signature missing blocks install
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| stored_sig = registry.get_signature(manifest_id) | ||
| if stored_sig is None: | ||
| return False |
There was a problem hiding this comment.
2. Signature missing blocks install 🐞 Bug ≡ Correctness
_verify_manifest_for_install() explicitly allows installs when registry.get_signature(manifest_id) is None, but the new TOCTOU re-verification hard-fails with 403 when stored_sig is None. This creates an inconsistent policy that can unexpectedly block installs (and emits a generic “re-verification failed” error after the initial gate already passed).
Agent Prompt
### Issue description
The initial signing gate (`_verify_manifest_for_install`) treats `stored_sig is None` as **allowed** (fail-open for legacy/unsigned manifests), but the new TOCTOU guard treats `stored_sig is None` as **blocked** (fail-closed), returning a 403.
This makes the system’s behavior inconsistent and confusing: the request passes the first verification gate and then deterministically fails later with a generic TOCTOU error whenever signatures are missing.
### Issue Context
- The gate and TOCTOU guard currently implement *different* policies for the same condition (missing stored signature).
- The file-level comment above the gate still states unsigned manifests are allowed through, which is no longer true once TOCTOU runs.
### Fix Focus Areas
- tinyagentos/routes/store_install.py[208-251]
- tinyagentos/routes/store_install.py[753-846]
### Suggested fix
Pick **one** policy and apply it consistently:
1) **If missing signatures should be allowed (legacy support):**
- In `_toctou_reverify()`, when `stored_sig is None`, *skip TOCTOU re-verification* (return `True`), mirroring `_verify_manifest_for_install()`.
2) **If missing signatures should be blocked (security hardening):**
- Change `_verify_manifest_for_install()` to return `(False, <reason>)` when `stored_sig is None` (optionally distinguishing `"never signed"` vs `"lost signature"`), so the request fails at the initial gate with a consistent error.
- Update the surrounding comments/docstring to reflect the new fail-closed behavior.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
@hognek routing this to you rather than the free build lanes, per Jay. This PR is CONFLICTING against dev, so none of the four required checks (test 3.12, test 3.13, lint, spa-build) have ever run on it. Worth stating precisely: a green check on a conflicted branch tested the OLD base, so "no red" here is evidence of nothing. Rebasing is what makes it testable at all. It cannot go to the free lanes: their harness builds a fresh worktree off origin/dev and gates on producing a commit, so any work on an EXISTING branch produces nothing and the card is destroyed. I proved that the expensive way today, losing four cards to it. Ask: rebase onto current origin/dev, preserve the author's commits, do not change behaviour while resolving, and comment listing what conflicted and how each hunk was resolved so a reviewer can check the resolution instead of re-deriving it. Please do NOT merge; the rebase is the whole job and I will review the new head. Context: A2A 1760 went to the bus, which you are not on. That was my error, so this is the same request on the channel you actually use. |
…ion + async TOCTOU - _canonical_manifest_bytes: add default=str to json.dumps so yaml.safe_load-produced date/datetime values don't cause signing failures on legitimate manifests (Kilo WARNING, registry.py:152) - TOCTOU re-verify: wrap disk read + Ed25519 verify in asyncio.to_thread to avoid blocking the event loop under load (Kilo SUGGESTION, store_install.py:796) - TOCTOU re-verify: fail-closed on read/parse/signature-lookup failures — _toctou_reverify now returns False (block install) when manifest.yaml is missing, unreadable, malformed, or the stored signature cannot be retrieved (CodeRabbit CRITICAL)
…peError Drop default=str from _canonical_manifest_bytes — yaml.safe_load parses unquoted 2026-01-01 into datetime.date but quoted into str, producing identical signing bytes (canonicalisation collision). Move _verify_sig inside the try/except in _toctou_reverify so a TypeError from json.dumps on non-primitive manifest values becomes return False (403) rather than a 500. Consistent with the PR's fail-closed thesis. Add 5 refusal-path tests for the TOCTOU re-verification guard: manifest missing, manifest unreadable, safe_load returns empty, stored_sig is None, and signature mismatch — each asserting 403.
- Add # noqa: BLE001 to the intentional catch-all in _toctou_reverify - Broaden the TOCTOU error message from 'manifest modified' to 'manifest signature re-verification failed' to cover all failure modes (unsigned, missing, unreadable, tampered) without claiming tampering for non-tampering cases - Skip test_toctou_manifest_unreadable_returns_403 when running as root (chmod 0o000 is a no-op for root in CI containers)
29679d5 to
32ec9c9
Compare
Rebase conflict resolutionRebased Merge-base: Two conflicts, both in the first commit ( 1.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tinyagentos/routes/store_install.py (1)
827-845: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the unsigned-manifest compatibility contract.
The initial gate explicitly allows manifests with no stored signature (for catalogs loaded before signing was enabled), but the new TOCTOU pass always rejects that same state. Consequently, enabling a public key can turn previously allowed unsigned catalog entries into 403 responses even without tampering. Carry whether the initial gate verified an existing signature; only run TOCTOU re-verification when it did, while still rejecting a signature that disappears after the initial check.
tinyagentos/routes/store_install.py#L827-L845: gate the TOCTOU check on an “initially signed and verified” result rather than treating every missing signature as tampering.tests/test_routes_store_install.py#L779-L779: change the no-signature case to assert the documented allowed behavior; add a separate test that removes a signature after the initial verification and expects 403.🤖 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/store_install.py` around lines 827 - 845, The TOCTOU re-verification currently rejects unsigned manifests that the initial gate permits. In tinyagentos/routes/store_install.py lines 827-845, carry forward whether the initial signature check found and verified an existing signature, run TOCTOU verification only for that state, and still return 403 if a previously verified signature disappears. In tests/test_routes_store_install.py line 779, update the no-signature case to assert it remains allowed and add coverage for removing the signature after initial verification returning 403.
🤖 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.
Outside diff comments:
In `@tinyagentos/routes/store_install.py`:
- Around line 827-845: The TOCTOU re-verification currently rejects unsigned
manifests that the initial gate permits. In tinyagentos/routes/store_install.py
lines 827-845, carry forward whether the initial signature check found and
verified an existing signature, run TOCTOU verification only for that state, and
still return 403 if a previously verified signature disappears. In
tests/test_routes_store_install.py line 779, update the no-signature case to
assert it remains allowed and add coverage for removing the signature after
initial verification returning 403.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6a857d2a-03d0-46f3-abd3-92acb2f80a79
📒 Files selected for processing (2)
tests/test_routes_store_install.pytinyagentos/routes/store_install.py
|
Adjudicated every open bot finding against head 32ec9c9. Close to merge: two small required fixes, @hognek fix-forward here. REQUIRED:
OPTIONAL, one line while you are there: logger.exception before the return False in _toctou_reverify's except block (qodo bug 3); every failure mode currently collapses into one unlogged 403. NOT this PR: the CodeRabbit Major (unsigned-manifest compat, store_install.py ~819-845) is REAL but byte-identical on dev; it came in with #2050, not with this branch. Follow-up card tsk-gr56lr covers it; do not touch that guard here. Rebase integrity checked: no dev work dropped; store_signing.py is byte-identical to dev. One narrative correction for the record: your conflict-2 report says you took the narrow try scope, but the head keeps the WIDE catch-all with the fail-closed noqa. The final state is the correct one (the tests require 403, not 500), only the written rationale describes the opposite; no code change needed. Green + these two fixes + a bot pass on the new head and I merge. |
…; fix em dash regression; add logger.exception in TOCTOU except - tests/test_routes_store_install.py: use getattr(os, 'geteuid', lambda: -1)() instead of os.geteuid() in skipif decorator (crashes on Windows at collection). - store_install.py line 824: replace em dash with hyphen in noqa comment. - tests/test_routes_store_install.py lines 843, 858: replace em dashes with hyphens. - store_install.py _toctou_reverify except block: add logger.exception() before return False for observability on fail-closed paths.
|
Fixed: geteuid guard + em dash cleanup.
All 6 TOCTOU tests pass locally. |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Reviewed by step-3.7-flash · Input: 103.5K · Output: 41.2K · Cached: 493.6K |
2 participants
Follow-up to #2050.
Changes:
32041496: Dropdefault=strfrom_canonical_manifest_bytes— fail-closed on TypeError (YAML date fields etc.)7a4e867e: Address CodeRabbit BLE001, unified error message, root guard2f559234: Resolve Kilo WARNING+SUGGESTION — date-safe canonicalisation + async TOCTOU29679d5e: Add TOCTOU date-field fail-closed regression testTests: 6 new TOCTOU tests + 13 store_signing tests — all pass.
Summary by CodeRabbit