fix(store): update test assertions 500→422 for unknown backend#2027
fix(store): update test assertions 500→422 for unknown backend#2027hognek wants to merge 8 commits into
Conversation
…l-v2 Add code signing to the app store install flow (jaylfc#647): - store_signing.py: Ed25519 keypair generation, persistence, sign/verify utilities that mirror the hub/identity.py pattern - registry.py: AppRegistry now accepts a signing key, signs every manifest at catalog load time, and exposes verify_manifest_signature() for re-verification at install time - store_install.py: signature verification gate in install-v2 before any installer or script runs; tampered manifests are rejected with 403 - GET /api/store/signing-pubkey: public key endpoint for clients and auditing tools - app.py: loads/creates the store signing keypair on boot Design: one Ed25519 keypair per taOS instance, generated on first boot, private key never leaves the node. Signatures are computed at catalog load time; install-v2 re-verifies against the stored signature to detect post-boot catalog tampering. Tests: 13 unique unit tests covering keypair lifecycle, sign/verify, tamper detection, deterministic signatures, _signature field stripping, and file permissions.
- registry.py: wrap sign_manifest() in try/except so a malformed signing key does not crash the entire catalog load; add logging import - app.py: wrap load_or_create_signing_keypair() in try/except OSError so a read-only data_dir does not prevent server startup - store_signing.py: enforce 0600 permissions on existing keyfile load (from round 1) - store_install.py: document threat model limitation; change pubkey endpoint 500→404 when unconfigured - tests: update pubkey endpoint test to expect 404
- store_signing.py: atomic keyfile creation with O_CREAT|O_EXCL+0o600, derive public key from loaded private key instead of trusting data['public_pem'], handle FileExistsError race by loading winner - registry.py: verify_manifest_signature now re-reads manifest.yaml from disk at install time, so post-boot catalog tampering is actually detected (previously compared two in-memory copies loaded at boot) - store_install.py: add _verify_manifest_for_install helper that distinguishes 'no stored signature' (graceful skip) from 'bad signature' (hard 403); verify backend manifests in install chain before running their installer - test_store_signing.py: fix flaky sig[:-2]+'ff' to XOR last byte so it always differs from original (~1/256 failure fixed) - test_routes_store_install.py: add end-to-end real-registry tamper test that mutates on-disk YAML and asserts 403 Tests: 13/13 store_signing pass. Install route fixture has pre-existing aiosqlite hang (CI will cover).
…e redundant os import - FileExistsError fallback now uses a PID-unique tmp path on retry so it never collides with a still-running winner's tmp file. - Added an early keyfile.exists() check after the 3s wait (the competing process may have completed os.replace by then). - Removed local 'import os as _os_module' and 'import time as _time' — os was already a top-level import and time is now imported at module level. - Replaced all _os_module.* and _time.* references with os.* and time.* respectively. Fixes: Kilo WARNING (store_signing.py:153) + SUGGESTION (line 132)
…1924 1. store_signing.py: enforce 0o600 BEFORE reading keyfile (umask bypass) 2. registry.py: fail-open for unsigned manifests, document policy, add set_signing_key() 3. store_install.py: TOCTOU guard — re-verify manifest from disk at execution time 4. app.py: defer keypair loading to lifespan (read-only data_dir no longer bricks startup) 5. store_install.py: 500→422 for backend without installer mapping Also fix test_real_registry_detects_post_load_tampering empty installed_path init.
…ecovery Skip the shared tmp name entirely after a FileExistsError contention timeout instead of unlinking it. Unlinking the shared tmp could race with a still-running winner process, causing a FileNotFoundError crash for the winner or inconsistent on-disk state. Jump directly to the unique PID-based tmp path, which is safe because every process gets its own name.
…_signature + second signature re-verify TOCTOU guard - registry.py: Change verify_manifest_signature from fail-open to fail-closed (returns False for unsigned manifests). The install gate (_verify_manifest_for_install) already short-circuits for unsigned manifests via get_signature() check, so the install path is unaffected. This prevents future callers from accidentally allowing unsigned manifests through. - store_install.py: Replace the TOCTOU field-comparison guard with a second signature re-verify against the re-read disk bytes. This is more robust: catches any change (not just the whitelisted fields), does not false-positive on legitimate catalog reloads, and aligns with the existing Ed25519 trust model. Fixes the 2 remaining Kilo SUGGESTIONS on jaylfc#2023.
PR jaylfc#2023 changed the _BACKEND_TO_METHOD lookup failure from 500 to 422. Update both test_unknown_backend_returns_500 assertions and docstrings: - tests/routes/test_store_install_v2.py::TestBackendToMethodMapping - tests/test_routes_store_install.py::TestInstallV2
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: 1 minute 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 (7)
✨ 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}
| sig = sign_manifest(raw_dict, self._signing_key) | ||
| signatures[catalog[-1].id] = sig | ||
| manifest_dicts[catalog[-1].id] = raw_dict | ||
| except Exception: |
There was a problem hiding this comment.
WARNING: Signing failure silently downgrades a manifest to "unsigned", which the install gate then treats as fail-open.
When sign_manifest raises, this block logs and continues without populating signatures[app_id]. At install time, _verify_manifest_for_install calls registry.get_signature(manifest_id), sees None, and short-circuits to (True, None) — i.e. the signing gate is skipped entirely for that app. The net effect is that any manifest whose signing throws (unexpected content, transient crypto/serialization error, etc.) installs with no tamper protection, and the only trace is a log line. An attacker who can induce a signing failure for a target manifest (or a manifest that legitimately fails to sign) thereby bypasses the whole Ed25519 gate silently. Consider treating a signing failure as fail-closed for that app (e.g. record a sentinel so the install gate blocks rather than skips), or at minimum surfacing it more loudly than an exception log.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (7 files)
Fix these issues in Kilo Cloud Reviewed by hy3:free · Input: 70.1K · Output: 7.1K · Cached: 529.3K |
|
Folded into #2023 |
1 participant
Summary
PR #2023 changed the
_BACKEND_TO_METHODlookup failure from 500 to 422 (jaylfc requested 4xx). Update both test assertions to match.Changes
tests/routes/test_store_install_v2.py:test_unknown_backend_returns_500_not_exception— assertion 500→422 + docstring updatetests/test_routes_store_install.py:TestInstallV2::test_unknown_backend_returns_500— assertion 500→422 + docstring updateDependency
Depends on #2023 (which changes the code from 500 to 422). CI will be red until #2023 merges.
Tests
2/2 targeted tests pass locally against the PR branch.
Kanban: t_7353a27e
Summary by Gitar
store_signing.py._verify_manifest_for_installgate tostore_install.pyto prevent installing tampered manifests.GET /api/store/signing-pubkeyto allow external auditing of catalog integrity.AppRegistryto support manifest signature tracking and re-verification logic.This will update automatically on new commits.