close
Skip to content

fix(appstore): compute app trust fields server-side (#580) - #2408

Merged
iamwhatever merged 1 commit into
mainfrom
fix/appstore-server-trust-580
Aug 10, 2026
Merged

fix(appstore): compute app trust fields server-side (#580)#2408
iamwhatever merged 1 commit into
mainfrom
fix/appstore-server-trust-580

Conversation

@bolichen97

Copy link
Copy Markdown
Collaborator

Summary

Moves the App Store trust boundary server-side. /api/apps/registry rows now carry explicit server-computed provenance ("core" | "external" | "builtin") and verified fields, and featured is stripped from external rows — so the client no longer derives trust semantics from the absence of the internal _registry tag.

Backend (src/kiro_crew/apps/registry.py)

  • New _apply_trust_fields() pass at the tail of list_registry(), where the server-attached _registry tag is authoritative. It overwrites anything an index publishes, so an external registry can never spoof the fields.
  • verified derives from the index-declared author snapshotted before the app.json merge (_index_author), never from the repo-fetched manifest — a third-party core repo publishing "author": "KiroCrew" in its app.json cannot mint the badge. (Caught by the pre-push Opus review pass; the snapshot key is popped before the payload is served.)
  • featured is popped from external rows entirely, so an external index can never self-flag into the Discover spotlight regardless of client logic.
  • _registry stays in the payload (source-label text, older clients, appManifest.ts::keysFor, pickFeatured legacy arm) — this change adds fields only.

Frontend (website/src/components/appstore/types.ts, website/src/pages/AppsPage.tsx)

  • isVerified / sourceLabel / pickFeatured prefer the server fields; a _registry-tagged row is still rejected FIRST, so verified/provenance values smuggled through an older gateway (which computes nothing) can never relabel an external row.
  • Legacy origin/author derivation kept as fallback for rows from older gateways — existing tests pass unmodified as the back-compat proof.
  • Client-synthesized builtin rows in browseApps set provenance: 'builtin', verified: true so both code paths speak the new contract.

Docs: docs/system-specs/modules/app-kit-platform.md updated in the same commit.

Why

Three PR #532 security holes (author spoof, origin spoof, featured self-flagging) were all instances of one misplaced boundary: trust derived client-side from an internal tagging detail. Nothing prevented a future backend change from silently flipping the verified badge next to an Install button that runs third-party setup with gateway privileges. The trust decision now lives where _registry is authoritative.

Testing

  • Backend: TestApplyTrustFields (9 tests) in test/test_apps_registry.py — external spoof neutralization (author/origin/featured), index pre-seed overwrite, manifest-author spoof regression, non-string author, _index_author non-leak, plus a list_registry end-to-end.
  • Frontend: 8 new tests in website/src/test/appstoreCategories.test.ts — server-field precedence, older-gateway smuggling defenses, either-signal external exclusion in pickFeatured, legacy-fallback parity. Pre-existing suites pass unmodified.
  • Gates: isort / flake8 / mypy clean; npx tsc -b clean; vitest 866/866 files; full pytest run (residual failures verified identical on clean main — host-environment).
  • Live verification: isolated gateway's /api/apps/registry emits the new fields (launchdarkly → provenance: core, verified: false).

Rendering back-compat

Discover tab is pixel-identical before/after (spotlight, verified badge, source labels):

Before (main) After (this PR)
before after

Pre-push review

Dual model-pinned review fleet ran before push: GPT 5.6 Sol — CLEAN; Opus 5 — 1 BLOCKING (verified mintable from manifest author) fixed via the pre-merge index-author snapshot + regression tests, 1 advisory (docstring _registry dependants) fixed; focused verifier confirmed the fix on all 5 checked points.

Closes #580

@bolichen97
bolichen97 requested a review from a team August 9, 2026 21:29
@bolichen97
bolichen97 requested a review from a team as a code owner August 9, 2026 21:29
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 9, 2026
@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed d190c77e773066d234ddb46657909a7601b08269 — this comment is updated in place on each push.

Review details

I've verified the full flow. Let me confirm the snapshot survives all _resolve_manifest return paths and the merge.

All paths in _resolve_manifest (return entry, _merge_manifest(entry, ...) which does dict(entry)) preserve the _index_author key set on line 1580 before the merge, and author is overwritten by the manifest while _index_author is not. _apply_trust_fields pops it before returning, so it never leaks. The exception path in list_registry (r if isinstance(r, dict) else entries[i]) also retains it. Non-string author degrades to unverified without raising. Frontend fallbacks reject _registry rows first, then read server fields, then legacy derivation.

No injected security hole, no crash/data-loss path, no removed guard without replacement — the change is purely additive hardening that moves trust computation server-side.

No findings.

[OPUS-REVIEWED] d190c77

Verdict parsed from the review's SHA-scoped output markers for commit d190c77e773066d234ddb46657909a7601b08269.

False positive or not applicable? A repository writer can comment:
/ai-review override fable d190c77e773066d234ddb46657909a7601b08269: <one-sentence reason>

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

Advisory UX-level review of d190c77e773066d234ddb46657909a7601b08269 — updated in place on each push; does not block merge.

UX-Verdict: PASS

Pure trust-boundary refactor: no new strings, no flow changes, and the before/after screenshots confirm the Discover tab renders pixel-identical.

[UX-REVIEWED] d190c77

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of d190c77e773066d234ddb46657909a7601b08269 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] d190c77

False positive or not applicable? A repository writer can comment:
/ai-review override gpt d190c77e773066d234ddb46657909a7601b08269: <one-sentence reason>

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Advisory design-level review of d190c77e773066d234ddb46657909a7601b08269 — updated in place on each push; does not block merge.

Design-Verdict: PASS

Trust computation moves to the layer where _registry is authoritative — correct boundary fix at the root cause, additive contract, explicit old-gateway/old-client fallbacks both ways.

Suggestions

  • Drop temp-screenshots/appstore-trust-580/ (~250KB of binaries named "temp") before merge — attach images to the PR via GitHub upload instead; committed blobs are permanent git-history weight and the pinned raw-URL commit they cite won't survive a rebase.
  • The client now duplicates the trust contract for synthesized builtin rows (provenance: 'builtin', verified: true in AppsPage.tsx) — fine today, but consider routing builtins through the same server payload eventually so _apply_trust_fields is the single writer.

[DESIGN-REVIEWED] d190c77

/api/apps/registry rows now carry server-computed provenance
('core' | 'external' | 'builtin') and verified fields, stamped by a new
_apply_trust_fields pass at the end of list_registry() — where the
server-attached _registry tag is authoritative — and OVERWRITING anything
an index publishes. featured is stripped from external rows entirely, so
an external index can never self-flag into the Discover spotlight.

The client's three trust derivations (isVerified, sourceLabel,
pickFeatured) collapse to read the server fields, still reject a
_registry-tagged row first (nothing smuggled through an older gateway can
relabel an external row), and keep the legacy origin/author derivation
only as a fallback for rows from older gateways. Client-synthesized
builtin rows in browseApps speak the new contract directly.

Rendering is identical for legitimate data before/after (screenshots
under temp-screenshots/appstore-trust-580/).

Closes #580
@bolichen97
bolichen97 force-pushed the fix/appstore-server-trust-580 branch from 4dd0490 to d190c77 Compare August 9, 2026 21:39
@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Aug 9, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator Author

🤖 Kiro Crew Auto-Pipeline [operator: bolichen97]

Disposition of the Design Review suggestion (single-writer trust contract for synthesized builtin rows): accepted-and-deferred#2416.

Rationale for deferring: builtin rows never pass through /api/apps/registry today — they are merged client-side from the installed-apps list, whose origin comes from server-authoritative install records, so the duplicated provenance: 'builtin', verified: true cannot be influenced by index or manifest content. Folding builtins into the registry payload changes the API's row population beyond this fix's minimal surface, so it goes to the linked follow-up rather than widening this PR.

@iamwhatever
iamwhatever merged commit 3991e7e into main Aug 10, 2026
51 checks passed
@iamwhatever
iamwhatever deleted the fix/appstore-server-trust-580 branch August 10, 2026 04:54
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Aug 10, 2026
encomjp pushed a commit to encomjp/kirocrew-customapi that referenced this pull request Aug 22, 2026
…irodotdev#2408)

/api/apps/registry rows now carry server-computed provenance
('core' | 'external' | 'builtin') and verified fields, stamped by a new
_apply_trust_fields pass at the end of list_registry() — where the
server-attached _registry tag is authoritative — and OVERWRITING anything
an index publishes. featured is stripped from external rows entirely, so
an external index can never self-flag into the Discover spotlight.

The client's three trust derivations (isVerified, sourceLabel,
pickFeatured) collapse to read the server fields, still reject a
_registry-tagged row first (nothing smuggled through an older gateway can
relabel an external row), and keep the legacy origin/author derivation
only as a fallback for rows from older gateways. Client-synthesized
builtin rows in browseApps speak the new contract directly.

Rendering is identical for legitimate data before/after (screenshots
under temp-screenshots/appstore-trust-580/).

Closes kirodotdev#580
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

App Store: move trust/provenance derivation from client to /api/apps/registry

2 participants