fix(store): fail-closed on sign_manifest failure — prevent install-gate bypass - #2050
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? |
📝 WalkthroughWalkthroughThe install route now performs TOCTOU manifest re-verification in a worker thread, fails closed for missing or invalid manifest data and signatures, and returns structured HTTP 403 errors. New tests cover refusal paths; canonicalization formatting is unchanged semantically. ChangesStore installation verification
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant InstallRoute
participant ManifestFile
participant AppRegistry
participant StoreSigning
InstallRoute->>ManifestFile: read manifest.yaml in worker thread
ManifestFile-->>InstallRoute: current manifest
InstallRoute->>AppRegistry: fetch stored signature
AppRegistry-->>InstallRoute: stored signature
InstallRoute->>StoreSigning: verify manifest signature
StoreSigning-->>InstallRoute: verification result
InstallRoute-->>InstallRoute: complete progress and return 403 on failure
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}
| "failed to sign manifest %s — catalog load continues unsigned", | ||
| catalog[-1].id, | ||
| ) | ||
| signing_failures.add(catalog[-1].id) |
There was a problem hiding this comment.
WARNING: Fail-closed signing failure can block valid manifests with date/timestamp fields
yaml.safe_load parses ISO date/timestamp strings (e.g. release_date: 2024-01-01) into datetime.date/datetime.datetime objects. _canonical_manifest_bytes then calls json.dumps(stripped, ...) (store_signing.py:209) with no default=, which raises TypeError on those values. That TypeError is swallowed by this broad except Exception: and the manifest is recorded in signing_failures. Under the new fail-closed policy (had_signing_failure → 403), any legitimate manifest containing a date field becomes permanently uninstallable (until restart), even though it is not tampered.
This turns a latent serialisation limitation into a hard availability regression. Consider making canonicalisation YAML-safe, e.g. json.dumps(stripped, ..., default=str) or re-encoding through yaml.safe_dump, so non-malicious values don't trip the fail-closed path.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| disk_path = _manifest_dir / "manifest.yaml" | ||
| try: | ||
| import yaml as _yaml | ||
| on_disk = _yaml.safe_load(disk_path.read_text()) if disk_path.exists() else None |
There was a problem hiding this comment.
SUGGESTION: Synchronous blocking I/O + crypto inside the async route
This TOCTOU re-read (disk_path.read_text() + yaml.safe_load + Ed25519 verify_manifest_signature) runs directly on the event loop (it is not awaited or offloaded to a thread). For an on-disk catalog this is usually fast, but under load or slow/network filesystems it blocks all concurrent requests for the duration of the disk read and verification.
Consider wrapping the read+verify in await asyncio.to_thread(...) (or reusing a threadpool) to keep the event loop responsive, consistent with how other blocking store operations in this app are handled.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Warning, 2 Suggestions | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous Review Summaries (3 snapshots, latest commit 471ad2c)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 471ad2c)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous review (commit e842727)Status: No Issues Found | Recommendation: Merge Both issues from the previous review have been resolved in the latest commits:
The incremental changes preserve the original gate logic and introduce no new issues. Files Reviewed (incremental — 2 changed files)
Previous review (commit b451956)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (7 files)
Reviewed by step-3.7-flash · Input: 161.4K · Output: 37.4K · Cached: 3.7M |
![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 (2)
tinyagentos/routes/store_install.py (1)
758-776: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPrimary signing gate still does blocking disk I/O + Ed25519 verify on the event loop.
The TOCTOU re-verify below (Line 812) is correctly offloaded via
asyncio.to_thread, but this primary gate calls_verify_manifest_for_installsynchronously, andregistry.verify_manifest_signaturere-readsmanifest.yamlfrom disk and runs Ed25519 verification inline. For a local on-disk catalog this is fast, but under concurrency or on slow/network filesystems it blocks all requests. Offload it for consistency with the TOCTOU path.♻️ Offload the primary gate
- verified, verify_err = _verify_manifest_for_install( - manifest_id, registry, _store_pub, - ) + verified, verify_err = await asyncio.to_thread( + _verify_manifest_for_install, manifest_id, registry, _store_pub, + )🤖 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 758 - 776, Offload the primary manifest verification call in the install route to a worker thread, matching the existing asynchronous TOCTOU verification path. Update the call to _verify_manifest_for_install to use asyncio.to_thread while preserving its arguments and the existing failure response handling.tests/test_routes_store_install.py (1)
340-357: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale test names after the 500→422 change. The unknown-backend response is now HTTP 422, but both test method names still say
500.
tests/test_routes_store_install.py#L340-L357: renametest_unknown_backend_returns_500→test_unknown_backend_returns_422.tests/routes/test_store_install_v2.py#L180-L227: renametest_unknown_backend_returns_500_not_exception→ reflect 422.🤖 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 `@tests/test_routes_store_install.py` around lines 340 - 357, Rename the unknown-backend test in tests/test_routes_store_install.py:340-357 from test_unknown_backend_returns_500 to test_unknown_backend_returns_422. Also rename test_unknown_backend_returns_500_not_exception in tests/routes/test_store_install_v2.py:180-227 to a name reflecting the 422 response; leave the test assertions and behavior unchanged.
🤖 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/app.py`:
- Around line 1597-1613: Move the store signing keypair initialization and
registry.set_signing_key() call out of create_app() and into the application
lifespan startup path. Ensure create_app() only prepares the relevant state,
while key loading and catalog reload occur lazily during lifespan startup;
update the comments to match the resulting behavior.
---
Nitpick comments:
In `@tests/test_routes_store_install.py`:
- Around line 340-357: Rename the unknown-backend test in
tests/test_routes_store_install.py:340-357 from test_unknown_backend_returns_500
to test_unknown_backend_returns_422. Also rename
test_unknown_backend_returns_500_not_exception in
tests/routes/test_store_install_v2.py:180-227 to a name reflecting the 422
response; leave the test assertions and behavior unchanged.
In `@tinyagentos/routes/store_install.py`:
- Around line 758-776: Offload the primary manifest verification call in the
install route to a worker thread, matching the existing asynchronous TOCTOU
verification path. Update the call to _verify_manifest_for_install to use
asyncio.to_thread while preserving its arguments and the existing failure
response handling.
🪄 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: 830833b2-14e6-44d5-8f3e-9d58eb7549f2
📒 Files selected for processing (7)
tests/routes/test_store_install_v2.pytests/test_routes_store_install.pytests/test_store_signing.pytinyagentos/app.pytinyagentos/registry.pytinyagentos/routes/store_install.pytinyagentos/store_signing.py
| # Load the store signing keypair lazily here in the lifespan, not in | ||
| # create_app(), so a read-only data_dir does not brick startup. | ||
| # When the keypair cannot be loaded (missing cryptography, unwritable | ||
| # data_dir), signing is simply disabled — the install gate falls | ||
| # through to unsigned (fail-open) and the pubkey endpoint returns 404. | ||
| _store_pub: bytes | None = None | ||
| try: | ||
| _store_priv, _store_pub = load_or_create_signing_keypair(data_dir) | ||
| if _store_priv is not None: | ||
| registry.set_signing_key(_store_priv) | ||
| except OSError: | ||
| logger.warning( | ||
| "store signing keypair could not be created (data_dir=%s may be " | ||
| "read-only) — catalog signatures will not be available", | ||
| data_dir, | ||
| ) | ||
| app.state.store_signing_pubkey = _store_pub |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the signing-key init is only in the eager create_app body and not duplicated in the lifespan,
# and confirm set_signing_key triggers an eager catalog reload.
rg -nP -C3 'set_signing_key|store_signing_pubkey|load_or_create_signing_keypair' tinyagentos/app.py
rg -nP -C3 'def reload|def _load_catalog|def set_signing_key' tinyagentos/registry.pyRepository: jaylfc/taOS
Length of output: 2722
🏁 Script executed:
#!/bin/bash
sed -n '1500,1620p' tinyagentos/app.py
printf '\n--- registry ---\n'
sed -n '100,175p' tinyagentos/registry.pyRepository: jaylfc/taOS
Length of output: 9774
Move this block into the lifespan, or fix the comments
It still runs in create_app(), so the deferred startup path never takes effect. registry.set_signing_key() also reloads the catalog immediately, making startup pay the manifest walk/parse up front.
🤖 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/app.py` around lines 1597 - 1613, Move the store signing keypair
initialization and registry.set_signing_key() call out of create_app() and into
the application lifespan startup path. Ensure create_app() only prepares the
relevant state, while key loading and catalog reload occur lazily during
lifespan startup; update the comments to match the resulting behavior.
|
This has gone to CONFLICT against Please rebase onto current Flagging one thing to watch during the rebase, because it bit #2068 badly tonight: when you resolve, check you are not carrying back anything dev has since changed. A branch cut before a hardening commit can silently revert it during a rebase and the merge will report clean, with no conflict marker to warn you. Worth a |
e842727 to
471ad2c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/store_install.py`:
- Around line 806-819: Update _toctou_reverify to fail closed: return False
whenever manifest.yaml is missing, unreadable, malformed, empty, the signature
lookup fails, or signature verification cannot complete. Wrap the entire on-disk
load, registry.get_signature(manifest_id), and verify_manifest_signature flow in
the existing catch-all handling, while preserving the successful verification
path; ensure the caller still handles the False result so progress.finish() is
reached when the check fails.
🪄 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: 9f64a4e4-15f6-4c91-9993-90ab782a7e84
📒 Files selected for processing (2)
tinyagentos/routes/store_install.pytinyagentos/store_signing.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tinyagentos/store_signing.py
…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)
471ad2c to
2f55923
Compare
|
Reviewed properly rather than on the bot verdicts, because two of three say "No Issues Found, Merge" and I think they missed something in The core change is right and I want it in. Converting the TOCTOU re-verify from fail-open to fail-closed is exactly correct: previously an unreadable manifest ( But this PR has no tests, and it is a security fix. Both changed files are production code: A fail-closed gate that has only ever been observed passing is unproven exactly where it counts. I want a test per refusal branch that proves it goes red: manifest missing, manifest unreadable, The finding the bots missed:
The good news first: for any manifest that already serialised, the output is byte-identical, so existing signatures are unaffected. The problem is that a canonical form must be injective, and this one no longer is. Worth noting what it is really fixing: without What I would rather see, in preference order:
Option 3 is the smallest change and fits this PR best: it makes the unserialisable case fail closed, which is what the PR is for. Happy to merge once there are refusal-path tests and the canonicalisation question is resolved. The |
|
Consolidating what is open on your side so you are not hunting across seven PRs at 1am. You have been pushing steadily (last commit 00:44) and I would rather hand you an ordered queue than a pile. Ordered by what unblocks the most:
Two things worth knowing that are not asks: #2177 merged and it was a genuinely good rebase. I verified the silent revert was gone by symbol-diffing against , and your streaming cap fix is correct: client open during the read, content-type gated before the body, cap checked inside . The size-cap test that shrinks to 100 and yields 200 bytes is exactly the right way to test a limit. #2068 closed as superseded, so nobody builds on the wrong branch. On #2180: do not add it to required status checks yet. Let it run advisory for a few days first. Against the real #2068 head it produced 85 findings, all technically true but unreadable, and a gate that trains people to waive is worse than none. On a stale branch the answer is always "rebase", not "waive 85 symbols". |
|
Ignore my previous comment, it posted mangled. I wrote it inside a double-quoted shell string and every backticked identifier got eaten by command substitution, so items 2, 3, 4 and 6 lost the exact technical detail that made them useful. My error and a good argument for heredocs. Corrected queue below. Ordered by what unblocks the most:
Two things that are not asks: #2177 merged, and it was a genuinely good rebase. I verified the silent revert was gone by symbol-diffing against #2068 closed as superseded so nobody builds on the wrong branch. On #2180: do not add it to required status checks yet. Let it run advisory for a few days. Against the real #2068 head it produced 85 findings, all technically true but unreadable, and a gate that trains people to waive is worse than none. On a stale branch the answer is always "rebase", never "waive 85 symbols individually". |
…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.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tinyagentos/routes/store_install.py (1)
824-825: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
# noqa: BLE001to the intentional catch-all.The blind
except Exceptionis deliberate (fail-closed), but Ruff flags it; other catch-alls in this file (Lines 976, 1043) already carry the suppression.Suggested tweak
- except Exception: + except Exception: # noqa: BLE001 - fail closed, never allow on error return False🤖 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 824 - 825, Annotate the intentional catch-all `except Exception` in the relevant installation flow with `# noqa: BLE001`, matching the existing suppressions on the other deliberate catch-alls in this file, while preserving its fail-closed `return False` behavior.Source: Linters/SAST tools
tests/test_routes_store_install.py (1)
604-635: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated catalog + signed-registry setup into a fixture/helper.
The same ~25 lines (catalog dir, manifest write, keypair,
AppRegistry, app-state wiring) are copied across five new tests; a helper returning(reg, pub, manifest_path)would make each test just its sabotage + assertion.🤖 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 `@tests/test_routes_store_install.py` around lines 604 - 635, Extract the repeated catalog, manifest, signing-key, AppRegistry, and client app-state setup from the affected tests into a shared fixture or helper that returns reg, pub, and manifest_path. Update the five tests to call this helper and retain only their scenario-specific sabotage and assertions, including the existing registry/client state initialization 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 `@tests/test_routes_store_install.py`:
- Around line 680-691: Guard the permission-sabotage scenario in the test around
manifest_path so it is skipped when running as root, where chmod(0o000) cannot
prevent reading. Preserve the existing chmod cleanup in finally and keep the
non-root assertions for the expected 403 response unchanged.
In `@tinyagentos/routes/store_install.py`:
- Around line 819-821: The unsigned-manifest policy must be consistent across
the initial verification and TOCTOU guard: in
tinyagentos/routes/store_install.py lines 819-821, allow stored_sig to be None
by returning True, matching _verify_manifest_for_install and
AppRegistry.verify_manifest_signature. Update tests/test_routes_store_install.py
lines 736-779 to expect successful handling of unsigned manifests and verify
behavior consistent with that fail-open policy; no tampering error should be
asserted for this case.
---
Nitpick comments:
In `@tests/test_routes_store_install.py`:
- Around line 604-635: Extract the repeated catalog, manifest, signing-key,
AppRegistry, and client app-state setup from the affected tests into a shared
fixture or helper that returns reg, pub, and manifest_path. Update the five
tests to call this helper and retain only their scenario-specific sabotage and
assertions, including the existing registry/client state initialization
behavior.
In `@tinyagentos/routes/store_install.py`:
- Around line 824-825: Annotate the intentional catch-all `except Exception` in
the relevant installation flow with `# noqa: BLE001`, matching the existing
suppressions on the other deliberate catch-alls in this file, while preserving
its fail-closed `return False` 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: 84026775-a4ae-484b-846d-22394d80adb2
📒 Files selected for processing (3)
tests/test_routes_store_install.pytinyagentos/routes/store_install.pytinyagentos/store_signing.py
| # Sabotage: revoke read permission. | ||
| try: | ||
| os.chmod(manifest_path, 0o000) | ||
| resp = await client.post("/api/store/install-v2", json={ | ||
| "manifest_id": "test-svc", | ||
| }) | ||
| assert resp.status_code == 403 | ||
| assert resp.json()["error"] == ( | ||
| "manifest modified between signature verification and install" | ||
| ) | ||
| finally: | ||
| os.chmod(manifest_path, 0o644) # restore so tmp_path can clean up |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
chmod 0o000 is a no-op for root — this test fails in root containers.
Many CI images run as root, where the read still succeeds, verification passes, and no 403 is returned. Guard it.
Suggested guard
+ `@pytest.mark.skipif`(
+ hasattr(os, "geteuid") and os.geteuid() == 0,
+ reason="chmod-based permission denial does not apply to root",
+ )
`@pytest.mark.asyncio`
async def test_toctou_manifest_unreadable_returns_403(self, client, tmp_path):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Sabotage: revoke read permission. | |
| try: | |
| os.chmod(manifest_path, 0o000) | |
| resp = await client.post("/api/store/install-v2", json={ | |
| "manifest_id": "test-svc", | |
| }) | |
| assert resp.status_code == 403 | |
| assert resp.json()["error"] == ( | |
| "manifest modified between signature verification and install" | |
| ) | |
| finally: | |
| os.chmod(manifest_path, 0o644) # restore so tmp_path can clean up | |
| `@pytest.mark.skipif`( | |
| hasattr(os, "geteuid") and os.geteuid() == 0, | |
| reason="chmod-based permission denial does not apply to root", | |
| ) | |
| `@pytest.mark.asyncio` | |
| async def test_toctou_manifest_unreadable_returns_403(self, client, tmp_path): | |
| # Sabotage: revoke read permission. | |
| try: | |
| os.chmod(manifest_path, 0o000) | |
| resp = await client.post("/api/store/install-v2", json={ | |
| "manifest_id": "test-svc", | |
| }) | |
| assert resp.status_code == 403 | |
| assert resp.json()["error"] == ( | |
| "manifest modified between signature verification and install" | |
| ) | |
| finally: | |
| os.chmod(manifest_path, 0o644) # restore so tmp_path can clean up |
🤖 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 `@tests/test_routes_store_install.py` around lines 680 - 691, Guard the
permission-sabotage scenario in the test around manifest_path so it is skipped
when running as root, where chmod(0o000) cannot prevent reading. Preserve the
existing chmod cleanup in finally and keep the non-root assertions for the
expected 403 response unchanged.
| stored_sig = registry.get_signature(manifest_id) | ||
| if stored_sig is None: | ||
| return False |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Unsigned-manifest policy diverges between the first gate and the TOCTOU guard. _verify_manifest_for_install and AppRegistry.verify_manifest_signature both document that a manifest with no stored signature is allowed through, but the TOCTOU guard blocks it with a "manifest modified" message; the new test locks that divergence in.
tinyagentos/routes/store_install.py#L819-L821: either returnTrueforstored_sig is Noneto match the documented fail-open gate, or keep the block and update the docstrings at Lines 220-246 plus the 403 error string so it does not claim tampering.tests/test_routes_store_install.py#L736-L779: update this test to match whichever policy is chosen, and assert an error message specific to the unsigned case rather than the tampering message.
📍 Affects 2 files
tinyagentos/routes/store_install.py#L819-L821(this comment)tests/test_routes_store_install.py#L736-L779
🤖 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 819 - 821, The
unsigned-manifest policy must be consistent across the initial verification and
TOCTOU guard: in tinyagentos/routes/store_install.py lines 819-821, allow
stored_sig to be None by returning True, matching _verify_manifest_for_install
and AppRegistry.verify_manifest_signature. Update
tests/test_routes_store_install.py lines 736-779 to expect successful handling
of unsigned manifests and verify behavior consistent with that fail-open policy;
no tampering error should be asserted for this case.
| stripped = {k: v for k, v in manifest_dict.items() if k != SIGNATURE_FIELD} | ||
| return json.dumps(stripped, sort_keys=True, ensure_ascii=False).encode("utf-8") | ||
| return json.dumps( | ||
| stripped, sort_keys=True, ensure_ascii=False, |
There was a problem hiding this comment.
WARNING: verify_manifest_signature can now raise TypeError on date-bearing manifests — json.dumps lacks default=str, but the exception tuple only covers ValueError/InvalidSignature, so TypeError propagates as a 500 instead of failing closed.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| # Sabotage: revoke read permission. | ||
| try: | ||
| os.chmod(manifest_path, 0o000) |
There was a problem hiding this comment.
SUGGESTION: os.chmod(path, 0o000) does not prevent root from reading — the unreadable-path test may pass vacuously in containerised CI.
Consider replacing the chmod sabotage with a mock that raises OSError/PermissionError on read_text so the failure path is exercised reliably regardless of runtime user.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # Restore the original so teardown is clean. | ||
| reg.verify_manifest_signature = original_verify # type: ignore[method-assign] | ||
|
|
||
| # ── TOCTOU refusal-path tests ────────────────────────────────────── |
There was a problem hiding this comment.
SUGGESTION: Missing coverage for the non-primitive type case that the canonicalisation changes concern
The 5 new tests cover refusal paths for missing, unreadable, empty, signature-less, and tampered manifests, but none exercise a manifest containing yaml.safe_load-produced datetime.date or datetime.datetime values. Given the explicit canonicalisation collision concern, a test asserting that such manifests fail cleanly (e.g. 403 or signing-failure block) would close the coverage gap.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
Both findings are fixed properly. Merging. Verified each against the code rather than the commit message. 1. 2. The fail-closed structure is now correct, and you took the option I hoped you would. 3. The tests are the right ones, and the comment in them says exactly what I wanted to see: "each test asserts 403 to prove the gate goes red where it counts". One thing worth recording, and it is not a criticism of this PR. Every bot comment here is dated 2026-07-19. Your last commit is 2026-07-28T03:14. So the "No Issues Found / Recommendation: Merge" verdicts, and the "Address before merge" one, all reviewed a version of this PR that is nine days old and no longer exists. No bot has looked at the current head. I am merging on my own review, not on those verdicts, and stating that explicitly so the record is honest. A stale bot approval is worse than no approval, because it reads as a second opinion when it is an opinion about different code. Worth everyone checking comment dates against the head SHA before treating a bot tick as review. Good fix. This closes a live install-gate bypass on |
2 participants
Summary
Fix Kilo WARNING from PR #2027: When
sign_manifestraises, the exception was logged but no signature was stored._verify_manifest_for_installsawNoneand short-circuited to(True, None)— allowing install with zero tamper protection for any manifest whose signing threw. An attacker inducing a signing failure for a target manifest could bypass the Ed25519 gate silently.Changes
registry.py: Added_signing_failures: set[str]tracking +had_signing_failure()method. Whensign_manifestraises during catalog load, the app_id is recorded in the failure set.store_install.py: In_verify_manifest_for_install, whenstored_sig is None, checkhad_signing_failure()first and return(False, msg)if signing failed — block the install gate rather than silently allowing it.Tests
Reference
Kilo:
sign_manifestfailures silently downgrade to unsigned (PR #2027 review on registry.py:141)Kanban: t_7f43f306
Summary by CodeRabbit
install_id) if the manifest was modified after the initial verification step.