diff --git a/CHANGELOG.md b/CHANGELOG.md index 7517ac1..bf6b914 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,45 @@ CalVer image tags (`stable-YYYY.MM.N`, `dev-YYYY.MM.N`) are produced for every C ## [Unreleased] +## [0.54.8] — 2026-05-13 + +### Changed + +- **BREAKING** Store upload — inline guardrail failures now hard-reject + before any DB row, bundle, photo, or doc is persisted. Two tiers: + - **Validation tier** (manifest + content checks) returns 422 with + `code='validation_failed'` and the corresponding `checks` payload. + Pure schema / description-quality issues a submitter fixes in seconds; + no audit trail. + - **Security tier** (static-security deny-list) returns 422 with + `code='security_blocked'` and writes a single `audit_log` row tagged + `store.upload.security_blocked` carrying the findings + SHA256 + size. + Forensic-only trace; no entity row, no submission row, no bundle on disk. + Quarantine + admin rescan/override now apply ONLY to the async LLM + review path (`blocked_llm` / `review_error`). The legacy + `submission_blocked` response code is no longer emitted; the wizard + + edit + restore frontends still understand it for one release as a + fallback for stale clients hitting an older deploy. +- Spam-quota counter (`count_blocked_for_submitter_since`) narrows to + `blocked_llm` + `review_error` rows. Inline failures no longer create + rows so they don't contribute. Slowapi rate limit + audit-log + visibility cover HTTP-level abuse on the inline path. +- Admin queue (`/admin/store/submissions`) — the "Needs review" filter + chip drops `blocked_inline` from its status set. Legacy `blocked_inline` + rows from instances that ran the v30 contract remain reachable via the + "All" tab (historical audit). Bundle-purge job (`purge.py`) likewise + stops covering `blocked_inline`; legacy rows linger but the live + contract no longer needs the sweep. + +### Internal + +- New `_reject_inline_or_continue` helper in `app/api/store.py` + centralises the two-tier rejection across `create_entity`, + `update_entity`, and `restore_version`. +- New `_seed_quarantined_entity` test helper replaces the older + `_make_eval_skill_zip`-driven setup for tests that need an entity in + the hidden + blocked_llm state. + ## [0.54.7] — 2026-05-13 ### Added diff --git a/app/api/store.py b/app/api/store.py index db403ae..af565ff 100644 --- a/app/api/store.py +++ b/app/api/store.py @@ -233,6 +233,96 @@ def _audit( pass +def _reject_inline_or_continue( + *, + conn: duckdb.DuckDBPyConnection, + user: dict, + inline: InlineResult, + bundle_meta: Any, + cleanup_paths: List[Path], + type_: str, + name: str, + context: str, +) -> None: + """Hard-reject sync guardrail failures; return None on pass. + + Two tiers, aligned to the *nature* of the failure rather than the + synchronicity of the check: + + * **Validation tier** — ``manifest_check`` and ``content_check`` + failures are fixable-by-submitter mistakes (missing files, bad + name regex, description too short). Return 422 ``validation_failed`` + with no DB writes and no audit trail; the upload wizard surfaces + a banner and the submitter retries. Matches the + ``/api/store/entities/preview`` step: invalid input → 4xx with + no side effects. + + * **Security tier** — ``static_scan`` failures are deny-list regex + hits (eval, leaked tokens, reverse-shell idioms). Return 422 + ``security_blocked`` with no DB writes, but emit one ``audit_log`` + row tagged ``store.upload.security_blocked`` carrying the + findings + SHA256 + size. Forensically interesting; the audit + row is the *only* trace. + + Quality is never blocking (``status='warn'`` max — checked elsewhere). + + Validation failures shadow security failures: if the bundle's + manifest is broken, the submitter sees only the manifest issues + (no security findings). This stops attackers from enumerating the + static_scan rule set by submitting bundles with adversarial bytes + inside otherwise-malformed manifests. + """ + validation_fail = ( + inline.manifest.get("status") != "pass" + or inline.content.get("status") != "pass" + ) + if validation_fail: + for p in cleanup_paths: + shutil.rmtree(p, ignore_errors=True) + raise HTTPException( + status_code=422, + detail={ + "code": "validation_failed", + "checks": { + "manifest": inline.manifest, + "content": inline.content, + "quality": inline.quality, + }, + }, + ) + + if inline.static_security.get("status") != "pass": + findings = inline.static_security.get("findings") or [] + try: + AuditRepository(conn).log( + user_id=user["id"], + action="store.upload.security_blocked", + resource=f"store_upload:{bundle_meta.sha256}", + params={ + "context": context, + "type": type_, + "name": name, + "findings": findings, + "finding_count": len(findings), + "file_size": bundle_meta.file_size, + "bundle_sha256": bundle_meta.sha256, + "submitter_email": user.get("email"), + }, + result="blocked", + ) + except Exception: + pass + for p in cleanup_paths: + shutil.rmtree(p, ignore_errors=True) + raise HTTPException( + status_code=422, + detail={ + "code": "security_blocked", + "checks": {"static_security": inline.static_security}, + }, + ) + + def _schedule_llm_review( background_tasks: BackgroundTasks, submission_id: str, @@ -1164,81 +1254,27 @@ async def create_entity( # ---- Guardrail pipeline ------------------------------------------ # - # Inline checks (manifest, static security, quality+templating) - # run synchronously against the BAKED plugin tree. - # - # On fail (v30): we KEEP the bundle on disk, create the entity - # row at ``visibility_status='hidden'`` (invisible in flea - # browse + served marketplace), persist the submission with - # entity_id + sha + size. Admin can Rescan / Override / - # Download from /admin/store/submissions. The 30-day TTL job - # purges bundle bytes later but keeps the audit row. - # - # On pass: create the entity in 'pending' visibility, schedule - # the (slow) LLM review on BackgroundTasks. Entity only - # becomes visible after the review approves it (or admin - # overrides). See docs/STORE_GUARDRAILS.md. + # Inline checks (manifest, content, static-security, quality) + # run synchronously against the BAKED plugin tree. Failure is + # hard-rejected — no entity row, no submission row, no bundle on + # disk. Quarantine + admin rescan apply ONLY to the async LLM + # path (see runner.run_llm_review). See docs/STORE_GUARDRAILS.md. from src.store_guardrails.bundle_meta import compute_bundle_meta bundle_meta = compute_bundle_meta(plugin_dir) inline = run_inline_checks( plugin_dir, type_=type, description=final_description, ) + _reject_inline_or_continue( + conn=conn, + user=user, + inline=inline, + bundle_meta=bundle_meta, + cleanup_paths=[_entity_dir(entity_id)], + type_=type, + name=final_name, + context="create", + ) subs_repo = StoreSubmissionsRepository(conn) - if not inline.passed: - # Persist the bundle on disk so admins can Rescan/Download. - # No rmtree here — TTL purge owns deletion now. - photo_rel = await _save_photo(photo, entity_id) if photo else None - doc_rels = await _save_docs(docs, entity_id) - repo.create( - id=entity_id, - owner_user_id=user["id"], - owner_username=username, - type=type, - name=final_name, - description=final_description, - category=category, - version=version, - photo_path=photo_rel, - video_url=video_url, - doc_paths=doc_rels, - file_size=file_size, - visibility_status="hidden", - ) - sub_id = subs_repo.create( - submitter_id=user["id"], - submitter_email=user.get("email"), - type=type, - name=final_name, - version=version, - status="blocked_inline", - entity_id=entity_id, - inline_checks=inline.to_response_dict(), - file_size=bundle_meta.file_size, - bundle_sha256=bundle_meta.sha256, - ) - _audit( - conn, user["id"], "store.submission.blocked_inline", - sub_id, { - "type": type, "name": final_name, - "entity_id": entity_id, - "manifest": inline.manifest.get("status"), - "static_security": inline.static_security.get("status"), - "static_findings": len(inline.static_security.get("findings") or []), - "content": inline.content.get("status"), - "content_issues": len(inline.content.get("issues") or []), - "sha256": bundle_meta.sha256, - "file_size": bundle_meta.file_size, - }, - ) - raise HTTPException( - status_code=422, - detail={ - "code": "submission_blocked", - "submission_id": sub_id, - "entity_id": entity_id, - "checks": inline.to_response_dict(), - }, - ) guardrails_on = get_guardrails_enabled() # When the pipeline is disabled (local dev / no ANTHROPIC_API_KEY) @@ -1473,46 +1509,23 @@ async def update_entity( description=description if description is not None else entity.get("description"), ) - if not inline_after_update.passed: - # Compute the rejected bundle's sha+size BEFORE the - # staging dir is removed so the submission row carries - # forensics about what was tried. - from src.store_guardrails.bundle_meta import compute_bundle_meta - rejected_meta = compute_bundle_meta(staging_plugin) - # Live tree was never touched — no rollback work to do. - # The `finally` below cleans the staging dir. - subs_repo = StoreSubmissionsRepository(conn) - sub_id = subs_repo.create( - submitter_id=user["id"], - submitter_email=user.get("email"), - type=entity["type"], - name=entity["name"], - version=new_version, - status="blocked_inline", - entity_id=entity_id, - inline_checks=inline_after_update.to_response_dict(), - file_size=rejected_meta.file_size, - bundle_sha256=rejected_meta.sha256, - ) - _audit( - conn, user["id"], "store.submission.blocked_inline", - sub_id, {"entity_id": entity_id, "on": "update", - "manifest": inline_after_update.manifest.get("status"), - "static_security": inline_after_update.static_security.get("status"), - "content": inline_after_update.content.get("status"), - "content_issues": len(inline_after_update.content.get("issues") or []), - "sha256": rejected_meta.sha256, - "file_size": rejected_meta.file_size}, - ) - raise HTTPException( - status_code=422, - detail={ - "code": "submission_blocked", - "submission_id": sub_id, - "entity_id": entity_id, - "checks": inline_after_update.to_response_dict(), - }, - ) + from src.store_guardrails.bundle_meta import compute_bundle_meta + staging_meta = compute_bundle_meta(staging_plugin) + # Hard-reject on inline failure. _reject_inline_or_continue + # cleans the staged version dir (no live state to roll back — + # the live ``plugin/`` tree was never touched) and raises 422 + # with code=validation_failed or code=security_blocked. The + # outer `finally` still wipes `scratch` regardless. + _reject_inline_or_continue( + conn=conn, + user=user, + inline=inline_after_update, + bundle_meta=staging_meta, + cleanup_paths=[version_root], + type_=entity["type"], + name=entity["name"], + context="update", + ) # Checks passed — but DO NOT swap live yet. Live ``plugin/`` # keeps serving the prior approved version to existing @@ -1529,18 +1542,11 @@ async def update_entity( # accepted submission is implicitly approved. finally: shutil.rmtree(scratch, ignore_errors=True) - # The version dir IS the source of truth — don't remove it - # on failure paths. But if the bundle was rejected before - # the swap (failed inline check raised 422), the version - # dir is incomplete: the row hasn't been bumped yet, the - # bytes are blocked-only forensics. Decision: keep the - # version dir for the admin to download via the - # submission's bundle download endpoint, but mark the - # row's version_history WITHOUT this incomplete entry - # (we never called append_version on the failed path). - # The version dir at versions/v/plugin/ stays orphan - # on disk linked from the submission row's hash — admin - # can still inspect. + # Inline-fail cleanup is owned by _reject_inline_or_continue + # (rmtrees the staged version dir before raising). The + # backup_plugin orphan check below covers the historical + # swap path that no longer triggers here, kept defensively + # in case a future refactor reintroduces an atomic swap. if backup_plugin is not None and backup_plugin.exists(): logger.error( "PUT version-swap left orphan backup at %s — " @@ -1837,48 +1843,28 @@ async def restore_version( type_=entity["type"], description=entity.get("description"), ) - if not inline.passed: - from src.store_guardrails.bundle_meta import compute_bundle_meta - rejected_meta = compute_bundle_meta(target_plugin) - subs_repo = StoreSubmissionsRepository(conn) - sub_id = subs_repo.create( - submitter_id=user["id"], - submitter_email=user.get("email"), - type=entity["type"], - name=entity["name"], - version=new_version, - status="blocked_inline", - entity_id=entity_id, - inline_checks=inline.to_response_dict(), - file_size=rejected_meta.file_size, - bundle_sha256=rejected_meta.sha256, - ) - _audit( - conn, user["id"], "store.submission.blocked_inline", - sub_id, - {"entity_id": entity_id, "on": "restore", - "restored_from_version_no": version_no, - "sha256": rejected_meta.sha256, - "file_size": rejected_meta.file_size}, - ) - # Live tree untouched. Version dir kept for forensic download. - raise HTTPException( - status_code=422, - detail={ - "code": "submission_blocked", - "submission_id": sub_id, - "entity_id": entity_id, - "checks": inline.to_response_dict(), - }, - ) + from src.store_guardrails.bundle_meta import compute_bundle_meta + target_meta = compute_bundle_meta(target_plugin) + # Hard-reject on inline failure. Wipe the staged version dir + # entirely — live tree untouched, no entity/submission row to + # create. The submitter sees the structured 422 and either fixes + # the source version (rare) or restores a different version. + _reject_inline_or_continue( + conn=conn, + user=user, + inline=inline, + bundle_meta=target_meta, + cleanup_paths=[target_root], + type_=entity["type"], + name=entity["name"], + context="restore", + ) # Inline checks passed. DO NOT swap live yet — same invariant as # the PUT edit path: existing installers keep getting the prior # approved version through the LLM review window. Promotion (live # swap + version_no/version/file_size bump) waits on LLM approval. guardrails_on = get_guardrails_enabled() - from src.store_guardrails.bundle_meta import compute_bundle_meta - accepted_meta = compute_bundle_meta(target_plugin) subs_repo = StoreSubmissionsRepository(conn) sub_id = subs_repo.create( submitter_id=user["id"], @@ -1889,14 +1875,14 @@ async def restore_version( status="approved" if not guardrails_on else "pending_llm", entity_id=entity_id, inline_checks=inline.to_response_dict(), - file_size=accepted_meta.file_size, - bundle_sha256=accepted_meta.sha256, + file_size=target_meta.file_size, + bundle_sha256=target_meta.sha256, ) appended_n = repo.append_version_history( entity_id, version_hash=new_version, - sha256=accepted_meta.sha256, - size=accepted_meta.file_size, + sha256=target_meta.sha256, + size=target_meta.file_size, submission_id=sub_id, created_by=user["id"], ) diff --git a/app/web/templates/_flea_versions.html b/app/web/templates/_flea_versions.html index 6b8606a..f36bc09 100644 --- a/app/web/templates/_flea_versions.html +++ b/app/web/templates/_flea_versions.html @@ -127,13 +127,33 @@ async function restoreVersion(versionNo) { const j = await r.json(); if (j.detail) { const code = j.detail.code || ''; - if (code === 'submission_blocked') { - // Land on detail to see the blocked banner. + const checks = j.detail.checks || {}; + if (code === 'validation_failed') { + // Stay on the detail page; surface manifest/content issues. + // The restored version's source bundle predates today's + // rules — admin or owner can either fix the source version + // or restore a different one. + const issues = (checks.manifest?.issues || []) + .concat((checks.content?.issues || []).map(i => i.code || 'issue')); + msg = 'Restore blocked: today\'s validation rules reject the v' + + versionNo + ' bundle.'; + if (issues.length) msg += '\n• ' + issues.slice(0, 5).join('\n• '); + } else if (code === 'security_blocked') { + const findings = (checks.static_security?.findings) || []; + msg = 'Restore blocked: security review found risky patterns in the v' + + versionNo + ' bundle.'; + if (findings.length) { + msg += '\n' + findings.slice(0, 5).map(f => + '• ' + (f.file || '?') + ':' + (f.line || '?') + ' — ' + (f.reason || f.category || '') + ).join('\n'); + } + } else if (code === 'submission_blocked') { + // Legacy server response (pre-cutover). Land on the detail + // page so the existing quarantine banner UX still works. const eid = j.detail.entity_id || '{{ entity.id }}'; window.location = `/marketplace/flea/${eid}`; return; - } - if (code === 'prior_version_pending') { + } else if (code === 'prior_version_pending') { msg = 'A previous edit is still under review. Wait for the verdict before restoring.'; } else if (code === 'version_not_found') { msg = 'That version is no longer on disk.'; diff --git a/app/web/templates/admin_store_submissions.html b/app/web/templates/admin_store_submissions.html index 93f4293..ce79b9b 100644 --- a/app/web/templates/admin_store_submissions.html +++ b/app/web/templates/admin_store_submissions.html @@ -225,7 +225,11 @@ All {% set _pending = "pending_llm,pending_inline" %} Pending - {% set _needs = "blocked_inline,blocked_llm,review_error" %} + {# 'blocked_inline' deliberately omitted — inline failures are now + hard-rejected upstream (no submission row created). Legacy + blocked_inline rows from instances that ran the v30 contract + remain reachable via the 'All' tab. #} + {% set _needs = "blocked_llm,review_error" %} Needs review Approved Overridden diff --git a/app/web/templates/store_edit.html b/app/web/templates/store_edit.html index 1579eb2..301b756 100644 --- a/app/web/templates/store_edit.html +++ b/app/web/templates/store_edit.html @@ -188,6 +188,7 @@ function showError(msg) { banner.className = 'banner error'; bannerText.textContent = msg; banner.hidden = false; + banner.scrollIntoView({behavior: 'smooth', block: 'start'}); } function clearBanner() { banner.hidden = true; } @@ -195,12 +196,53 @@ function humanizeError(detail) { if (!detail) return 'Save failed.'; if (typeof detail === 'object') { const code = detail.code || ''; - if (code === 'submission_blocked') { - const lines = ['New version blocked by automated checks.']; - const inline = (detail.checks?.static_security?.findings) || []; - for (const f of inline.slice(0, 5)) { + const checks = detail.checks || {}; + + function appendFindings(lines, payload) { + const findings = (payload?.findings) || []; + for (const f of findings.slice(0, 5)) { lines.push('• ' + (f.file || '?') + ':' + (f.line || '?') + ' — ' + (f.reason || f.category || '')); } + if (findings.length > 5) lines.push(' …and ' + (findings.length - 5) + ' more.'); + } + function appendManifest(lines, payload) { + const issues = (payload?.issues) || []; + for (const m of issues.slice(0, 5)) lines.push('• manifest: ' + m); + if (issues.length > 5) lines.push(' …and ' + (issues.length - 5) + ' more.'); + } + function appendContent(lines, payload) { + const issues = (payload?.issues) || []; + for (const i of issues.slice(0, 5)) { + const where = i.component_type === 'submission' + ? 'Description on the form' + : ((i.component_type || 'component') + (i.name ? ' — ' + i.name : '')); + const code = (i.code || '').replace(/_/g, ' '); + lines.push('• ' + where + ' — ' + (i.field || 'description') + ' ' + code); + } + if (issues.length > 5) lines.push(' …and ' + (issues.length - 5) + ' more.'); + } + + if (code === 'validation_failed') { + const lines = ['New version needs fixing before it can be saved.']; + appendManifest(lines, checks.manifest); + appendContent(lines, checks.content); + lines.push(''); + lines.push('Fix the issues above and try again. The previous version is still live.'); + return lines.join('\n'); + } + if (code === 'security_blocked') { + const lines = ['Save blocked: security review found risky patterns in the new bundle.']; + appendFindings(lines, checks.static_security); + lines.push(''); + lines.push('Remove the flagged code/secrets and try again. The previous version stays live.'); + return lines.join('\n'); + } + if (code === 'submission_blocked') { + // Legacy server response (pre-cutover) — kept for one release. + const lines = ['New version blocked by automated checks.']; + appendFindings(lines, checks.static_security); + appendManifest(lines, checks.manifest); + appendContent(lines, checks.content); lines.push(''); lines.push('Fix the issues and re-upload, or open the detail page to see the full report.'); return lines.join('\n'); diff --git a/app/web/templates/store_upload.html b/app/web/templates/store_upload.html index 7b8e6c6..e116887 100644 --- a/app/web/templates/store_upload.html +++ b/app/web/templates/store_upload.html @@ -632,72 +632,108 @@ const ERROR_MESSAGES = { not_owner: 'You don\'t own this entity, so you can\'t change it.', }; +function _renderManifestIssues(lines, manifest) { + const issues = (manifest && manifest.issues) || []; + for (const m of issues.slice(0, 5)) { + lines.push('• manifest: ' + m); + } + if (issues.length > 5) lines.push(' …and ' + (issues.length - 5) + ' more.'); +} + +function _renderContentIssues(lines, content) { + const issues = (content && content.issues) || []; + if (!issues.length) return; + // Plain-language labels — match the server-side rendering in + // _content_findings.html so the wording stays consistent. + const FIELD_LABEL = { + 'frontmatter.description': 'Description (top of the file, after `description:`)', + 'plugin.json.description': 'Description (in `.claude-plugin/plugin.json`)', + 'description': 'Description (on the upload form)', + 'body': 'Content (rest of the file after the description line)', + }; + const CODE_LABEL = { + 'empty': 'is missing', + 'too_short': 'is too short', + 'low_word_count': 'needs more distinct words', + 'placeholder_text': 'still has placeholder text (TODO, template, etc.)', + 'body_too_short': 'is too short', + }; + const COMPONENT_LABEL = { + 'skill': 'skill', 'agent': 'agent', 'plugin': 'plugin', + 'command': 'command', 'submission': 'description', + }; + lines.push(''); + lines.push('What needs fixing:'); + for (const issue of issues.slice(0, 6)) { + const comp = COMPONENT_LABEL[issue.component_type] || 'component'; + const fieldLabel = FIELD_LABEL[issue.field] || issue.field || 'description'; + const codeLabel = CODE_LABEL[issue.code] || (issue.code || 'issue').replace(/_/g, ' '); + const where = issue.component_type === 'submission' + ? 'Description on the upload form' + : (comp.charAt(0).toUpperCase() + comp.slice(1)) + + (issue.name ? ' — ' + issue.name : ''); + lines.push('• ' + where); + lines.push(' ' + fieldLabel + ' ' + codeLabel + '.'); + if (issue.hint) lines.push(' ' + issue.hint); + } + if (issues.length > 6) { + lines.push(' …and ' + (issues.length - 6) + ' more.'); + } +} + +function _renderSecurityFindings(lines, staticSecurity) { + const findings = (staticSecurity && staticSecurity.findings) || []; + for (const f of findings.slice(0, 5)) { + const where = (f.file || '?') + ':' + (f.line || '?'); + lines.push('• ' + where + ' — ' + (f.reason || f.category || 'security finding')); + } + if (findings.length > 5) lines.push(' …and ' + (findings.length - 5) + ' more.'); +} + function humanizeError(detail) { if (!detail) return 'Something went wrong. Please try again.'; - // Structured detail (FastAPI wraps the dict at .detail). Two shapes: - // {code: "submission_blocked", submission_id, checks: {...}} — guardrails - // {code: "quota_exceeded", limit, blocked_in_last_24h, hint} — quota - // Drop the bare ``Upload failed: [object Object]`` fallback. + // Structured detail (FastAPI wraps the dict at .detail). Shapes: + // {code: "validation_failed", checks: {manifest, content, quality}} — manifest/content fail + // {code: "security_blocked", checks: {static_security}} — static_scan deny-list hit + // {code: "submission_blocked", entity_id, checks: {...}} — legacy, kept for back-compat + // {code: "quota_exceeded", limit, blocked_in_last_24h, hint} — LLM-tier spam quota if (typeof detail === 'object') { const code = detail.code || ''; + const checks = detail.checks || {}; + + if (code === 'validation_failed') { + const lines = ['Bundle needs fixing before it can be submitted.']; + _renderManifestIssues(lines, checks.manifest); + _renderContentIssues(lines, checks.content); + lines.push(''); + lines.push('Fix the issues above and try again. Nothing was uploaded.'); + lines.push('See /store/examples for full before/after examples (opens in new tab).'); + return lines.join('\n'); + } + + if (code === 'security_blocked') { + const lines = ['Upload blocked: security review found risky patterns in the bundle.']; + _renderSecurityFindings(lines, checks.static_security); + lines.push(''); + lines.push('Nothing was uploaded. Remove the flagged code or secrets and try again.'); + lines.push('If a finding is a false positive, contact your administrator.'); + return lines.join('\n'); + } + if (code === 'submission_blocked') { + // Legacy server response (pre-cutover). Render same payload shape + // so refreshed pages on an older deploy still get a usable banner. const lines = ['Upload blocked by automated checks.']; - const checks = detail.checks || {}; - const inline = (checks.static_security && checks.static_security.findings) || []; - for (const f of inline.slice(0, 5)) { - const where = (f.file || '?') + ':' + (f.line || '?'); - lines.push('• ' + where + ' — ' + (f.reason || f.category || 'security finding')); - } - if (inline.length > 5) lines.push(' …and ' + (inline.length - 5) + ' more.'); - const manifestIssues = (checks.manifest && checks.manifest.issues) || []; - for (const m of manifestIssues.slice(0, 3)) { - lines.push('• manifest: ' + m); - } - const contentIssues = (checks.content && checks.content.issues) || []; - if (contentIssues.length) { - // Plain-language labels — match the server-side rendering in - // _content_findings.html so the wording stays consistent. - const FIELD_LABEL = { - 'frontmatter.description': 'Description (top of the file, after `description:`)', - 'plugin.json.description': 'Description (in `.claude-plugin/plugin.json`)', - 'description': 'Description (on the upload form)', - 'body': 'Content (rest of the file after the description line)', - }; - const CODE_LABEL = { - 'empty': 'is missing', - 'too_short': 'is too short', - 'low_word_count': 'needs more distinct words', - 'placeholder_text': 'still has placeholder text (TODO, template, etc.)', - 'body_too_short': 'is too short', - }; - const COMPONENT_LABEL = { - 'skill': 'skill', 'agent': 'agent', 'plugin': 'plugin', - 'command': 'command', 'submission': 'description', - }; - lines.push(''); - lines.push('What needs fixing:'); - for (const issue of contentIssues.slice(0, 6)) { - const comp = COMPONENT_LABEL[issue.component_type] || 'component'; - const fieldLabel = FIELD_LABEL[issue.field] || issue.field || 'description'; - const codeLabel = CODE_LABEL[issue.code] || (issue.code || 'issue').replace(/_/g, ' '); - const where = issue.component_type === 'submission' - ? 'Description on the upload form' - : (comp.charAt(0).toUpperCase() + comp.slice(1)) - + (issue.name ? ' — ' + issue.name : ''); - lines.push('• ' + where); - lines.push(' ' + fieldLabel + ' ' + codeLabel + '.'); - if (issue.hint) lines.push(' ' + issue.hint); - } - if (contentIssues.length > 6) { - lines.push(' …and ' + (contentIssues.length - 6) + ' more.'); - } - } + _renderSecurityFindings(lines, checks.static_security); + _renderManifestIssues(lines, checks.manifest); + _renderContentIssues(lines, checks.content); lines.push(''); lines.push('Fix the issues above and re-upload as a new version.'); lines.push('See /store/examples for full before/after examples (opens in new tab).'); return lines.join('\n'); } + if (code === 'quota_exceeded') { return 'Upload blocked: too many rejected uploads in the last 24 hours ' + '(' + (detail.blocked_in_last_24h || '?') + '/' + (detail.limit || '?') + '). ' @@ -761,6 +797,11 @@ function showError(msg, {showExamplesLink = false} = {}) { bannerText.textContent = msg; banner.hidden = false; bannerActions.hidden = !showExamplesLink; + // Scroll the banner into view — the Finish button lives at the bottom + // of step 2, so a rejection banner that flips on at the top is + // off-screen on a tall form. Submitter needs to SEE the issues to + // act on them. + banner.scrollIntoView({behavior: 'smooth', block: 'start'}); } function clearBanner() { banner.hidden = true; bannerActions.hidden = true; } @@ -1059,8 +1100,13 @@ finishBtn.addEventListener('click', async () => { if (!name) { showError('Name is required.'); return; } function isContentBlock(detail) { - return detail && detail.code === 'submission_blocked' - && detail.checks && detail.checks.content + if (!detail || !detail.checks) return false; + // Show the "see examples" link when content-tier issues are the + // dominant fix needed — both new + legacy codes can carry them. + const isFixable = detail.code === 'validation_failed' + || detail.code === 'submission_blocked'; + return isFixable + && detail.checks.content && (detail.checks.content.issues || []).length > 0; } const type = document.querySelector('input[name=type]:checked').value; @@ -1087,24 +1133,28 @@ finishBtn.addEventListener('click', async () => { window.location = `/marketplace/flea/${encodeURIComponent(entity.id)}`; return; } - // Inline-blocked uploads (422 with submission_blocked) now persist - // the entity at visibility=hidden so admins can rescan/override and - // submitters can see the full quarantine banner. Redirect to the - // detail page when the response carries an entity_id — same UX as - // a successful upload, just landing on a banner instead of an - // approved card. + // Inline failures (validation_failed / security_blocked) are hard + // rejections — no entity row, no bundle on disk. Render the banner + // inline and stay on step 2 so the submitter can fix and retry. + // + // The legacy ``submission_blocked`` branch is retained for one + // release cycle. Older deploys may still emit it with an + // ``entity_id`` pointing at a hidden row; redirect to the detail + // page so the existing quarantine banner UX still works. Fresh + // deploys never hit this branch. let msg = 'Upload failed.'; let showLink = false; try { const j = await res.json(); - const eid = j && j.detail && j.detail.entity_id; - if (eid && j.detail.code === 'submission_blocked') { + const detail = j && j.detail; + const eid = detail && detail.entity_id; + if (eid && detail.code === 'submission_blocked') { window.location = `/marketplace/flea/${encodeURIComponent(eid)}`; return; } - if (j.detail) { - msg = humanizeError(j.detail); - showLink = isContentBlock(j.detail); + if (detail) { + msg = humanizeError(detail); + showLink = isContentBlock(detail); } } catch(_) {} showError(msg, {showExamplesLink: showLink}); diff --git a/pyproject.toml b/pyproject.toml index fa468ad..0453e42 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "agnes-the-ai-analyst" -version = "0.54.7" +version = "0.54.8" description = "Agnes — AI Data Analyst platform for AI analytical systems" requires-python = ">=3.11,<3.14" license = "MIT" diff --git a/src/repositories/store_submissions.py b/src/repositories/store_submissions.py index 50a0f92..9dcb12c 100644 --- a/src/repositories/store_submissions.py +++ b/src/repositories/store_submissions.py @@ -141,21 +141,29 @@ class StoreSubmissionsRepository: self, submitter_id: str, since, ) -> int: """Spam-quota helper. Counts submissions by ``submitter_id`` whose - verdict is one of the rejected/error states - (``blocked_inline | blocked_llm | review_error``) newer than + verdict is ``blocked_llm`` or ``review_error`` newer than ``since`` (a ``datetime`` — typically now - 24h). Called from - the POST entry point; refusal bounds disk growth from a single - bot looping on malformed/risky ZIPs. + the POST entry point; refusal bounds the load placed on the + async LLM reviewer by a single bot looping risky bundles. - Pre-fix this counted ONLY ``blocked_inline``. A bad-actor - submitter who triggered ten ``blocked_llm`` verdicts was - unbounded. All three states represent rejected uploads — count - them together. + Inline failures (manifest/content validation, static-security + deny-list) are hard-rejected upstream without creating a + submission row — they don't consume the LLM-tier quota. + Slowapi rate limits + the audit_log + ``store.upload.security_blocked`` trail cover that path. + + Historical note: pre-#9 this counted only ``blocked_inline``; + a bot triggering ``blocked_llm`` verdicts was unbounded. + Post-#9 it widened to all three. The current incarnation + narrows back to LLM-tier states since inline failures no + longer create rows. Legacy ``blocked_inline`` rows in DBs that + ran the v30 contract are still present (historical audit) but + intentionally excluded from the live counter. """ row = self.conn.execute( "SELECT COUNT(*) FROM store_submissions " "WHERE submitter_id = ? " - " AND status IN ('blocked_inline', 'blocked_llm', 'review_error') " + " AND status IN ('blocked_llm', 'review_error') " " AND created_at >= ?", [submitter_id, since], ).fetchone() diff --git a/src/store_guardrails/purge.py b/src/store_guardrails/purge.py index 1546fdf..23bc263 100644 --- a/src/store_guardrails/purge.py +++ b/src/store_guardrails/purge.py @@ -30,8 +30,13 @@ logger = logging.getLogger(__name__) # serve the user, but admins can still want it for forensics. Excludes # `approved` (live entity, never purge), `overridden` (admin already # decided to publish), and `pending_*` (still in review). +# +# `blocked_inline` was removed when inline failures became hard-rejects +# (no submission row, no bundle persisted). Legacy `blocked_inline` rows +# from pre-cutover instances stay on disk indefinitely; reachable via +# audit_log lookups but no longer swept here. Re-add to the tuple if a +# one-shot legacy-cleanup pass is wanted. TERMINAL_BLOCKED_STATUSES = ( - "blocked_inline", "blocked_llm", "review_error", ) diff --git a/tests/test_admin_store_submissions.py b/tests/test_admin_store_submissions.py index d14770e..5be7413 100644 --- a/tests/test_admin_store_submissions.py +++ b/tests/test_admin_store_submissions.py @@ -86,6 +86,102 @@ def _make_eval_skill_zip(skill_name: str = "bad") -> bytes: return buf.getvalue() +def _seed_quarantined_entity( + user_id: str, + user_email: str, + skill_name: str = "quarantined", + description: str = "Description seeded for tests — long enough to pass content checks.", + *, + status: str = "blocked_llm", + static_findings=None, + llm_summary: str = "test stub finding", +): + """Seed a hidden flea entity + matching submission row + on-disk + bundle, mimicking the post-LLM-review-blocked state. + + Inline failures (manifest, static-security, content) are now + hard-rejected upstream and never create DB rows. Tests that + previously triggered the v30 ``submission_blocked`` path by + uploading a bad bundle must seed the quarantined state directly + via this helper. The default status is ``blocked_llm`` — the only + status path that still creates a hidden+pending entity. + + Returns ``(entity_id, submission_id)``. + """ + from src.repositories.store_entities import StoreEntitiesRepository + from src.repositories.store_submissions import StoreSubmissionsRepository + from src.store_naming import suffixed_name + import uuid as _uuid + + entity_id = _uuid.uuid4().hex + username = user_email.split("@")[0] + + store_dir = get_store_dir() + entity_dir = store_dir / entity_id + plugin_root = entity_dir / "plugin" + skill_subdir = plugin_root / "skills" / suffixed_name(skill_name, username) + skill_subdir.mkdir(parents=True, exist_ok=True) + skill_md = skill_subdir / "SKILL.md" + skill_md.write_text( + f"---\nname: {suffixed_name(skill_name, username)}\n" + f"description: {description}\n---\n\n" + + ("Body content. " * 30), + ) + run_sh = skill_subdir / "run.sh" + run_sh.write_text("#!/bin/sh\neval $1\n") + # v1 seed dir so the version download / restore endpoints find it. + v1_plugin = entity_dir / "versions" / "v1" / "plugin" + v1_plugin.parent.mkdir(parents=True, exist_ok=True) + import shutil as _shutil + _shutil.copytree(plugin_root, v1_plugin) + + # Mirror the InlineResult.to_response_dict() shape that the runner + # would have produced. Static findings are surfaced verbatim in the + # quarantine banner template (_quarantine_banner.html). + findings = static_findings if static_findings is not None else [ + {"file": "run.sh", "line": 2, "category": "code_exec", + "severity": "high", + "reason": "shell eval expanding a variable", + "snippet": "eval $1"}, + ] + inline_checks = { + "manifest": {"status": "pass", "issues": []}, + "static_security": {"status": "fail", "findings": findings}, + "content": {"status": "pass", "issues": []}, + "quality": {"status": "pass", "issues": []}, + } + + conn = get_system_db() + StoreEntitiesRepository(conn).create( + id=entity_id, + owner_user_id=user_id, + owner_username=username, + type="skill", + name=skill_name, + description=description, + category=None, + version="1.0.0", + file_size=512, + visibility_status="hidden", + ) + sub_id = StoreSubmissionsRepository(conn).create( + submitter_id=user_id, + submitter_email=user_email, + type="skill", + name=skill_name, + version="1.0.0", + status=status, + entity_id=entity_id, + inline_checks=inline_checks, + llm_findings={"risk_level": "high", "summary": llm_summary, + "findings": findings}, + file_size=512, + bundle_sha256="0" * 64, + ) + conn.close() + return entity_id, sub_id + + # --------------------------------------------------------------------------- # /api/admin/store/submissions — listing # --------------------------------------------------------------------------- @@ -97,36 +193,119 @@ class TestAdminListing: r = web_client.get("/api/admin/store/submissions", cookies=user_cookies) assert r.status_code == 403 - def test_admin_sees_blocked_inline_submission(self, web_client): - # Bad upload from a regular user → inline-blocked → submission row. - _, user_cookies = _create_user(web_client, "u@x.com") + def test_security_upload_creates_no_submission_row(self, web_client): + """Static-security findings are hard-rejected — no submission row, + no entity row, no bundle on disk. Replaces the v30 contract + where inline failures landed in admin's queue at + ``blocked_inline``. + """ + from src.repositories.store_entities import StoreEntitiesRepository + from src.repositories.store_submissions import StoreSubmissionsRepository + + user_id, user_cookies = _create_user(web_client, "u@x.com") c = web_client.post( "/api/store/entities", files={"file": ("s.zip", _make_eval_skill_zip("bad"), "application/zip")}, data={"type": "skill"}, cookies=user_cookies, ) assert c.status_code == 422 - # 422 detail must include both submission_id AND entity_id so - # the upload-page JS can redirect the submitter to the detail - # page (same UX as a successful upload — they land on the - # quarantine banner instead of staying stuck on /store/new). detail = c.json()["detail"] - assert detail["code"] == "submission_blocked" - assert detail["submission_id"] - assert detail["entity_id"], ( - "422 body must carry entity_id so the uploader can be " - "redirected to /marketplace/flea/{entity_id}" - ) + assert detail["code"] == "security_blocked" + # Findings are exposed inline so the wizard banner can render them. + assert detail["checks"]["static_security"]["status"] == "fail" + assert detail["checks"]["static_security"]["findings"] + # No DB rows, no quarantined entity for the submitter to inspect. + assert "submission_id" not in detail + assert "entity_id" not in detail + conn = get_system_db() + items, _total = StoreSubmissionsRepository(conn).list_for_admin( + submitter_id=user_id, + ) + assert items == [] + ent_items, _ = StoreEntitiesRepository(conn).list(owner_user_id=user_id) + assert ent_items == [] + conn.close() + + # Admin queue is empty: no row was ever created. _, admin_cookies = _create_admin(web_client) r = web_client.get( - "/api/admin/store/submissions?status=blocked_inline", - cookies=admin_cookies, + "/api/admin/store/submissions", cookies=admin_cookies, ) assert r.status_code == 200 - body = r.json() - assert body["total"] >= 1 - assert any(s["status"] == "blocked_inline" for s in body["items"]) + items = r.json()["items"] + assert not any(s["submitter_id"] == user_id for s in items), ( + "security_blocked upload must not surface in admin queue" + ) + + def test_security_upload_emits_audit_log_entry(self, web_client): + """A static-security rejection writes one ``store.upload.security_blocked`` + audit_log row carrying the findings + sha256 + size. That row is + the *only* trace of the attempt; admin can grep audit_log for + repeated offenders. + """ + from src.repositories.audit import AuditRepository + + user_id, user_cookies = _create_user(web_client, "spammer@x.com") + c = web_client.post( + "/api/store/entities", + files={"file": ("s.zip", _make_eval_skill_zip("audit"), "application/zip")}, + data={"type": "skill"}, cookies=user_cookies, + ) + assert c.status_code == 422 + assert c.json()["detail"]["code"] == "security_blocked" + + conn = get_system_db() + rows, _cursor = AuditRepository(conn).query( + user_id=user_id, action="store.upload.security_blocked", + limit=10, + ) + conn.close() + assert len(rows) == 1 + params = rows[0].get("params") or {} + if isinstance(params, str): + params = json.loads(params) + assert params.get("finding_count", 0) >= 1 + assert params.get("bundle_sha256") + assert params.get("submitter_email") == "spammer@x.com" + + def test_validation_failure_creates_no_audit_trail(self, web_client): + """A bundle that fails manifest validation (missing SKILL.md) is + a fixable user error — no submission row, no entity row, and + NO audit_log entry. The submitter just sees the wizard banner. + """ + from src.repositories.audit import AuditRepository + from src.repositories.store_submissions import StoreSubmissionsRepository + + # Skill ZIP without the required SKILL.md — manifest_check fails. + buf = io.BytesIO() + with zipfile.ZipFile(buf, "w") as zf: + zf.writestr("broken/notes.md", "no manifest here\n") + bad_zip = buf.getvalue() + + user_id, user_cookies = _create_user(web_client, "validation@x.com") + c = web_client.post( + "/api/store/entities", + files={"file": ("s.zip", bad_zip, "application/zip")}, + data={"type": "skill"}, cookies=user_cookies, + ) + assert c.status_code == 422 + # zip_missing_skill_md fires at metadata-extract (pre-bake), so + # the response is a plain ``detail: "zip_missing_skill_md"`` — + # but the contract under test is "no DB rows, no audit trail", + # which is what we assert below. + conn = get_system_db() + items, _total = StoreSubmissionsRepository(conn).list_for_admin( + submitter_id=user_id, + ) + assert items == [] + rows, _cursor = AuditRepository(conn).query( + user_id=user_id, action_prefix="store.upload.", limit=10, + ) + conn.close() + assert rows == [], ( + "validation-tier rejection must not write audit_log entries" + ) # --------------------------------------------------------------------------- @@ -135,48 +314,6 @@ class TestAdminListing: class TestAdminOverride: - def test_override_inline_blocked_publishes_entity(self, web_client): - """v30: inline-blocked submissions now persist the bundle + entity - row at visibility=hidden, so override flips them to approved - identically to blocked_llm.""" - from src.repositories.store_entities import StoreEntitiesRepository - from src.repositories.store_submissions import StoreSubmissionsRepository - - _, user_cookies = _create_user(web_client, "u@x.com") - c = web_client.post( - "/api/store/entities", - files={"file": ("s.zip", _make_eval_skill_zip("bad"), "application/zip")}, - data={"type": "skill"}, cookies=user_cookies, - ) - assert c.status_code == 422 - sub_id = c.json()["detail"]["submission_id"] - - # Confirm v30 invariants: submission carries entity_id + sha + size, - # entity row exists at visibility=hidden. - conn = get_system_db() - sub = StoreSubmissionsRepository(conn).get(sub_id) - assert sub["entity_id"] is not None - assert sub["bundle_sha256"] and len(sub["bundle_sha256"]) == 64 - assert sub["file_size"] and sub["file_size"] > 0 - ent = StoreEntitiesRepository(conn).get(sub["entity_id"]) - assert ent and ent["visibility_status"] == "hidden" - conn.close() - - _, admin_cookies = _create_admin(web_client) - r = web_client.post( - f"/api/admin/store/submissions/{sub_id}/override", - json={"reason": "false positive — internal-only"}, - cookies=admin_cookies, - ) - assert r.status_code == 200, r.text - - conn = get_system_db() - sub = StoreSubmissionsRepository(conn).get(sub_id) - assert sub["status"] == "overridden" - ent = StoreEntitiesRepository(conn).get(sub["entity_id"]) - assert ent["visibility_status"] == "approved" - conn.close() - def test_override_blocked_llm_publishes_entity(self, web_client): """Manually stage a blocked_llm row + entity, then override — the entity must flip to visibility_status='approved' and the @@ -594,15 +731,13 @@ class TestAdminRescan: class TestAdminBundleDownload: def test_download_returns_zip(self, web_client): - """Live blocked bundle is downloadable as a fresh ZIP.""" - _, user_cookies = _create_user(web_client, "u@x.com") - c = web_client.post( - "/api/store/entities", - files={"file": ("s.zip", _make_eval_skill_zip("dl"), "application/zip")}, - data={"type": "skill"}, cookies=user_cookies, + """Live blocked-LLM bundle is downloadable as a fresh ZIP. Inline + rejections no longer persist bundles, so this exercises the LLM + path via the seed helper.""" + user_id, _ = _create_user(web_client, "u@x.com") + _entity_id, sub_id = _seed_quarantined_entity( + user_id, "u@x.com", skill_name="dl", ) - assert c.status_code == 422 - sub_id = c.json()["detail"]["submission_id"] _, admin_cookies = _create_admin(web_client) r = web_client.get( @@ -612,8 +747,6 @@ class TestAdminBundleDownload: assert r.status_code == 200 assert r.headers["content-type"] == "application/zip" assert "attachment" in r.headers["content-disposition"] - # Body is a valid ZIP - import io, zipfile with zipfile.ZipFile(io.BytesIO(r.content)) as zf: assert any("SKILL.md" in n for n in zf.namelist()) assert any("run.sh" in n for n in zf.namelist()) @@ -709,56 +842,74 @@ class TestAdminSortBySize: class TestQuota: def test_quota_blocks_after_threshold(self, web_client, monkeypatch): - # Tiny quota for the test. - monkeypatch.setenv("AGNES_QUOTA_DUMMY", "1") # noop, just to use monkeypatch + """Quota gate triggers on the LLM-tier reject count. Inline + failures no longer create rows, so the quota is seeded via + the repo (mimicking two prior blocked_llm verdicts in the + last 24h). The third upload is gated upstream by 429.""" from app import instance_config as ic + from src.repositories.store_submissions import StoreSubmissionsRepository monkeypatch.setattr(ic, "get_guardrails_blocked_quota_per_day", lambda: 2) - _, user_cookies = _create_user(web_client, "spammer@x.com") - - # First two bad uploads land as blocked_inline 422, third hits quota 429. + user_id, user_cookies = _create_user(web_client, "spammer@x.com") + conn = get_system_db() + repo = StoreSubmissionsRepository(conn) for i in range(2): - r = web_client.post( - "/api/store/entities", - files={"file": ("s.zip", _make_eval_skill_zip(f"bad{i}"), "application/zip")}, - data={"type": "skill"}, cookies=user_cookies, + repo.create( + submitter_id=user_id, submitter_email="spammer@x.com", + type="skill", name=f"seed-{i}", version="1.0.0", + status="blocked_llm", entity_id=None, ) - assert r.status_code == 422, f"upload {i}: {r.status_code} {r.text}" + conn.close() + # Third upload — any clean ZIP would do; expect 429 before the + # guardrail pipeline runs. r = web_client.post( "/api/store/entities", - files={"file": ("s.zip", _make_eval_skill_zip("bad-3"), "application/zip")}, + files={"file": ("s.zip", _make_skill_zip("clean-after-quota"), "application/zip")}, data={"type": "skill"}, cookies=user_cookies, ) - assert r.status_code == 429 + assert r.status_code == 429, r.text body = r.json()["detail"] assert body["code"] == "quota_exceeded" assert body["limit"] == 2 def test_quota_disabled_with_zero(self, web_client, monkeypatch): + """quota=0 disables the gate entirely. Seed many blocked_llm + rows; clean uploads still succeed.""" from app import instance_config as ic + from src.repositories.store_submissions import StoreSubmissionsRepository monkeypatch.setattr(ic, "get_guardrails_blocked_quota_per_day", lambda: 0) - _, user_cookies = _create_user(web_client, "trusted@x.com") - for i in range(3): - r = web_client.post( - "/api/store/entities", - files={"file": ("s.zip", _make_eval_skill_zip(f"q{i}"), "application/zip")}, - data={"type": "skill"}, cookies=user_cookies, + user_id, user_cookies = _create_user(web_client, "trusted@x.com") + conn = get_system_db() + for i in range(5): + StoreSubmissionsRepository(conn).create( + submitter_id=user_id, submitter_email="trusted@x.com", + type="skill", name=f"history-{i}", version="1.0.0", + status="blocked_llm", entity_id=None, ) - assert r.status_code == 422, f"upload {i}" + conn.close() + + r = web_client.post( + "/api/store/entities", + files={"file": ("s.zip", _make_skill_zip("clean-zero-quota"), "application/zip")}, + data={"type": "skill"}, cookies=user_cookies, + ) + # Clean upload — passes inline guardrails. With ANTHROPIC_API_KEY + # absent in tests the guardrail pipeline auto-disables so the + # entity lands at ``approved`` (201). Anything other than 429 is + # the quota-disabled outcome we care about. + assert r.status_code != 429, r.text def test_quota_counter_includes_blocked_llm_and_review_error(self, web_client): - """#9 — pre-fix the counter only counted blocked_inline. A - submitter triggering ten blocked_llm verdicts was unbounded. - Post-fix: counter includes blocked_inline + blocked_llm + - review_error so all three reject states share the cap.""" + """The counter narrows to ``blocked_llm`` + ``review_error`` — + inline failures no longer create rows. Legacy ``blocked_inline`` + rows from pre-cutover instances are intentionally excluded + (kept in DB as historical audit, not counted toward the live + quota).""" from datetime import datetime, timezone, timedelta from src.repositories.store_submissions import StoreSubmissionsRepository - # Seed three blocked submissions of different types directly via - # the repo so we don't depend on triggering each verdict path - # through the API (LLM mocking is involved). _, user_cookies = _create_user(web_client, "spammer-9@x.com") conn = get_system_db() repo = StoreSubmissionsRepository(conn) @@ -772,8 +923,8 @@ class TestQuota: since = datetime.now(timezone.utc) - timedelta(hours=24) count = repo.count_blocked_for_submitter_since("spammer-9", since) conn.close() - assert count == 3, ( - f"counter must include all three reject states; got {count}" + assert count == 2, ( + f"counter must skip legacy blocked_inline; got {count}" ) @@ -784,21 +935,10 @@ class TestQuota: class TestQuarantineGates: def test_owner_cannot_delete_quarantined(self, web_client): - """Owner trying to DELETE their own blocked_inline entity must - be refused — admin investigates first.""" - _, user_cookies = _create_user(web_client, "u@x.com") - c = web_client.post( - "/api/store/entities", - files={"file": ("s.zip", _make_eval_skill_zip("q1"), "application/zip")}, - data={"type": "skill"}, cookies=user_cookies, - ) - eid = c.json()["detail"]["submission_id"] - # The submission row carries entity_id; fetch it. - from src.repositories.store_submissions import StoreSubmissionsRepository - conn = get_system_db() - sub = StoreSubmissionsRepository(conn).get(eid) - entity_id = sub["entity_id"] - conn.close() + """Owner trying to DELETE their own quarantined (blocked_llm) + entity must be refused — admin investigates first.""" + user_id, user_cookies = _create_user(web_client, "u@x.com") + entity_id, _sub_id = _seed_quarantined_entity(user_id, "u@x.com", "q1") r = web_client.delete( f"/api/store/entities/{entity_id}", cookies=user_cookies, @@ -808,17 +948,8 @@ class TestQuarantineGates: assert body["code"] == "quarantined_owner_cannot_delete" def test_admin_can_delete_quarantined(self, web_client): - _, user_cookies = _create_user(web_client, "u@x.com") - c = web_client.post( - "/api/store/entities", - files={"file": ("s.zip", _make_eval_skill_zip("q2"), "application/zip")}, - data={"type": "skill"}, cookies=user_cookies, - ) - sid = c.json()["detail"]["submission_id"] - from src.repositories.store_submissions import StoreSubmissionsRepository - conn = get_system_db() - entity_id = StoreSubmissionsRepository(conn).get(sid)["entity_id"] - conn.close() + user_id, _ = _create_user(web_client, "u@x.com") + entity_id, _sub_id = _seed_quarantined_entity(user_id, "u@x.com", "q2") _, admin_cookies = _create_admin(web_client) r = web_client.delete( @@ -831,36 +962,22 @@ class TestQuarantineGates: 404 — same as if the entity didn't exist (no leak via 403). Covers every ``_enforce_visibility`` caller in app/api/store.py + the marketplace flea detail.""" - _, owner_cookies = _create_user(web_client, "owner@x.com") - c = web_client.post( - "/api/store/entities", - files={"file": ("s.zip", _make_eval_skill_zip("q3"), "application/zip")}, - data={"type": "skill"}, cookies=owner_cookies, - ) - sid = c.json()["detail"]["submission_id"] - from src.repositories.store_submissions import StoreSubmissionsRepository - conn = get_system_db() - entity_id = StoreSubmissionsRepository(conn).get(sid)["entity_id"] - conn.close() + owner_id, _ = _create_user(web_client, "owner@x.com") + entity_id, _sub_id = _seed_quarantined_entity(owner_id, "owner@x.com", "q3") _, intruder_cookies = _create_user(web_client, "snoop@x.com") - # Detail r = web_client.get( f"/api/store/entities/{entity_id}", cookies=intruder_cookies, ) assert r.status_code == 404, "detail must 404 for non-owner" - # Files listing r = web_client.get( f"/api/store/entities/{entity_id}/files", cookies=intruder_cookies, ) assert r.status_code == 404, "files must 404 for non-owner" - # Photo (404 even when no photo uploaded — we want no leak via - # status code differences anyway) r = web_client.get( f"/api/store/entities/{entity_id}/photo", cookies=intruder_cookies, ) assert r.status_code == 404, "photo must 404 for non-owner" - # Docs sub-path r = web_client.get( f"/api/store/entities/{entity_id}/docs/anything.md", cookies=intruder_cookies, @@ -872,17 +989,10 @@ class TestQuarantineGates: (`/api/store/entities`) must NOT see another user's quarantined entry. Mirrors the marketplace-items coverage but on the store-namespaced listing.""" - _, owner_cookies = _create_user(web_client, "qowner@x.com") - c = web_client.post( - "/api/store/entities", - files={"file": ("s.zip", _make_eval_skill_zip("q-list"), "application/zip")}, - data={"type": "skill"}, cookies=owner_cookies, + owner_id, owner_cookies = _create_user(web_client, "qowner@x.com") + entity_id, _sub_id = _seed_quarantined_entity( + owner_id, "qowner@x.com", "q-list", ) - sid = c.json()["detail"]["submission_id"] - from src.repositories.store_submissions import StoreSubmissionsRepository - conn = get_system_db() - entity_id = StoreSubmissionsRepository(conn).get(sid)["entity_id"] - conn.close() _, intruder_cookies = _create_user(web_client, "qsnoop@x.com") r = web_client.get("/api/store/entities", cookies=intruder_cookies) @@ -893,8 +1003,6 @@ class TestQuarantineGates: "in /api/store/entities listing" ) - # Owner sees own entry on the same listing (auto-include via - # include_owner_id widening). r = web_client.get("/api/store/entities", cookies=owner_cookies) owner_ids = {it["id"] for it in r.json().get("items", [])} assert entity_id in owner_ids, ( @@ -902,17 +1010,8 @@ class TestQuarantineGates: ) def test_owner_can_view_their_quarantined_entity(self, web_client): - _, owner_cookies = _create_user(web_client, "owner@x.com") - c = web_client.post( - "/api/store/entities", - files={"file": ("s.zip", _make_eval_skill_zip("q4"), "application/zip")}, - data={"type": "skill"}, cookies=owner_cookies, - ) - sid = c.json()["detail"]["submission_id"] - from src.repositories.store_submissions import StoreSubmissionsRepository - conn = get_system_db() - entity_id = StoreSubmissionsRepository(conn).get(sid)["entity_id"] - conn.close() + owner_id, owner_cookies = _create_user(web_client, "owner@x.com") + entity_id, _sub_id = _seed_quarantined_entity(owner_id, "owner@x.com", "q4") r = web_client.get( f"/api/store/entities/{entity_id}", cookies=owner_cookies, @@ -921,17 +1020,8 @@ class TestQuarantineGates: def test_install_quarantined_refused_for_non_admin(self, web_client): """Even owner cannot add their own quarantined item to my-stack.""" - _, owner_cookies = _create_user(web_client, "owner@x.com") - c = web_client.post( - "/api/store/entities", - files={"file": ("s.zip", _make_eval_skill_zip("q5"), "application/zip")}, - data={"type": "skill"}, cookies=owner_cookies, - ) - sid = c.json()["detail"]["submission_id"] - from src.repositories.store_submissions import StoreSubmissionsRepository - conn = get_system_db() - entity_id = StoreSubmissionsRepository(conn).get(sid)["entity_id"] - conn.close() + owner_id, owner_cookies = _create_user(web_client, "owner@x.com") + entity_id, _sub_id = _seed_quarantined_entity(owner_id, "owner@x.com", "q5") r = web_client.post( f"/api/store/entities/{entity_id}/install", cookies=owner_cookies, @@ -950,22 +1040,12 @@ class TestMarketplaceFleaConsolidation: """Random non-owner non-admin pasting an entity_id into /marketplace/flea/{id} gets 404 — same policy as the now-deleted /store/{id}.""" - _, owner_cookies = _create_user(web_client, "owner@x.com") - c = web_client.post( - "/api/store/entities", - files={"file": ("s.zip", _make_eval_skill_zip("c1"), "application/zip")}, - data={"type": "skill"}, cookies=owner_cookies, - ) - sid = c.json()["detail"]["submission_id"] - from src.repositories.store_submissions import StoreSubmissionsRepository - conn = get_system_db() - eid = StoreSubmissionsRepository(conn).get(sid)["entity_id"] - conn.close() + owner_id, _ = _create_user(web_client, "owner@x.com") + eid, _sub_id = _seed_quarantined_entity(owner_id, "owner@x.com", "c1") _, intruder_cookies = _create_user(web_client, "snoop@x.com") r = web_client.get(f"/marketplace/flea/{eid}", cookies=intruder_cookies) assert r.status_code == 404 - # API equivalent r = web_client.get(f"/api/marketplace/flea/{eid}/detail", cookies=intruder_cookies) assert r.status_code == 404 @@ -973,31 +1053,32 @@ class TestMarketplaceFleaConsolidation: """Owner landing on /marketplace/flea/{id} sees the quarantine banner with the failure summary AND the actual finding details — not just a generic "Quarantined" header.""" - _, owner_cookies = _create_user(web_client, "owner@x.com") - c = web_client.post( - "/api/store/entities", - files={"file": ("s.zip", _make_eval_skill_zip("c2"), "application/zip")}, - data={"type": "skill"}, cookies=owner_cookies, + owner_id, owner_cookies = _create_user(web_client, "owner@x.com") + eid, _sub_id = _seed_quarantined_entity( + owner_id, "owner@x.com", "c2", + llm_summary="reviewer flagged the bash eval", + static_findings=[ + {"file": "run.sh", "line": 2, "severity": "high", + "category": "code_exec", + "reason": "shell eval expanding a variable", + "explanation": "shell eval expanding a variable", + "snippet": "eval $1"}, + ], ) - sid = c.json()["detail"]["submission_id"] - from src.repositories.store_submissions import StoreSubmissionsRepository - conn = get_system_db() - eid = StoreSubmissionsRepository(conn).get(sid)["entity_id"] - conn.close() r = web_client.get(f"/marketplace/flea/{eid}", cookies=owner_cookies) assert r.status_code == 200 body = r.text - # Banner partial rendered. assert "vis-banner" in body assert "Quarantined" in body - # Concrete reason — the eval-shell rule was the offender; banner - # must surface the finding details so the submitter knows WHY. - assert "security:" in body, ( - "banner missing static_security findings list — user sees " - "'Quarantined' label but no actionable reason" + # blocked_llm path renders the LLM verdict summary + per-finding + # list. Banner must surface BOTH so the submitter knows WHY + # without having to ping an admin. + assert "Security findings" in body, ( + "banner missing 'Security findings' section" ) assert "run.sh" in body, "banner missing path of offending file" + assert "shell eval" in body, "banner missing reviewer summary" def test_review_error_banner_shows_error_detail(self, web_client): """#review_error — banner must surface the underlying error @@ -1060,17 +1141,8 @@ class TestMarketplaceFleaConsolidation: def test_marketplace_listing_includes_owner_quarantined(self, web_client): """Submitter sees their own non-approved entries in the /api/marketplace/items?tab=flea grid; non-owner does not.""" - _, owner_cookies = _create_user(web_client, "owner@x.com") - c = web_client.post( - "/api/store/entities", - files={"file": ("s.zip", _make_eval_skill_zip("c4"), "application/zip")}, - data={"type": "skill"}, cookies=owner_cookies, - ) - sid = c.json()["detail"]["submission_id"] - from src.repositories.store_submissions import StoreSubmissionsRepository - conn = get_system_db() - eid = StoreSubmissionsRepository(conn).get(sid)["entity_id"] - conn.close() + owner_id, owner_cookies = _create_user(web_client, "owner@x.com") + eid, _sub_id = _seed_quarantined_entity(owner_id, "owner@x.com", "c4") # Owner — their own quarantined card surfaces with is_viewer_owner=True. r = web_client.get("/api/marketplace/items?tab=flea", cookies=owner_cookies) @@ -1356,17 +1428,8 @@ class TestArchiveSoftDelete: def test_owner_cannot_archive_quarantined(self, web_client): """Owner Delete on quarantined still refused (existing v32 policy).""" - _, user_cookies = _create_user(web_client, "u@x.com") - c = web_client.post( - "/api/store/entities", - files={"file": ("s.zip", _make_eval_skill_zip("q-arch"), "application/zip")}, - data={"type": "skill"}, cookies=user_cookies, - ) - sid = c.json()["detail"]["submission_id"] - from src.repositories.store_submissions import StoreSubmissionsRepository - conn = get_system_db() - eid = StoreSubmissionsRepository(conn).get(sid)["entity_id"] - conn.close() + user_id, user_cookies = _create_user(web_client, "u@x.com") + eid, _sub_id = _seed_quarantined_entity(user_id, "u@x.com", "q-arch") r = web_client.delete(f"/api/store/entities/{eid}", cookies=user_cookies) assert r.status_code == 403 @@ -1376,17 +1439,8 @@ class TestArchiveSoftDelete: """Admin can archive a quarantined entity (separate from override + hard-delete paths — admin keeps full control).""" from src.repositories.store_entities import StoreEntitiesRepository - from src.repositories.store_submissions import StoreSubmissionsRepository - _, user_cookies = _create_user(web_client, "u@x.com") - c = web_client.post( - "/api/store/entities", - files={"file": ("s.zip", _make_eval_skill_zip("q-arch2"), "application/zip")}, - data={"type": "skill"}, cookies=user_cookies, - ) - sid = c.json()["detail"]["submission_id"] - conn = get_system_db() - eid = StoreSubmissionsRepository(conn).get(sid)["entity_id"] - conn.close() + user_id, _ = _create_user(web_client, "u@x.com") + eid, _sub_id = _seed_quarantined_entity(user_id, "u@x.com", "q-arch2") _, admin_cookies = _create_admin(web_client) r = web_client.delete(f"/api/store/entities/{eid}", cookies=admin_cookies) @@ -1400,14 +1454,9 @@ class TestArchiveSoftDelete: def test_owners_endpoint_filters_quarantined_for_non_admin(self, web_client): """A user with only quarantined uploads must NOT appear in the public /api/store/owners dropdown.""" - _, user_cookies = _create_user(web_client, "spammer@x.com") - web_client.post( - "/api/store/entities", - files={"file": ("s.zip", _make_eval_skill_zip("only-bad"), "application/zip")}, - data={"type": "skill"}, cookies=user_cookies, - ) + user_id, _ = _create_user(web_client, "spammer@x.com") + _seed_quarantined_entity(user_id, "spammer@x.com", "only-bad") - # Different non-admin viewing owners. _, other_cookies = _create_user(web_client, "other@x.com") r = web_client.get("/api/store/owners", cookies=other_cookies) assert r.status_code == 200 @@ -1420,33 +1469,21 @@ class TestArchiveSoftDelete: marketplace.py (drift risk against repo); this test locks the parity with marketplace items so a future change to the repo clause that misses the inline copy gets caught.""" - # Owner uploads ONE bad skill (lands at visibility=hidden). - _, owner_cookies = _create_user(web_client, "qcat-owner@x.com") - web_client.post( - "/api/store/entities", - files={"file": ("s.zip", _make_eval_skill_zip("qcat"), "application/zip")}, - data={"type": "skill"}, cookies=owner_cookies, - ) + owner_id, owner_cookies = _create_user(web_client, "qcat-owner@x.com") + _seed_quarantined_entity(owner_id, "qcat-owner@x.com", "qcat") - # Different non-admin user. Categories listing must NOT count - # the quarantined entry in any bucket. _, snoop_cookies = _create_user(web_client, "qcat-snoop@x.com") r = web_client.get( "/api/marketplace/categories?tab=flea", cookies=snoop_cookies, ) assert r.status_code == 200, r.text body = r.json() - # Response shape is `{"items": [{name, count, icon_key}, …]}`. - # Non-owner non-admin must see 0 total since no approved entries - # exist for this fresh user. total = sum(c.get("count", 0) for c in body.get("items", [])) assert total == 0, ( "non-owner saw quarantined entry counted in /categories: " f"{body}" ) - # Owner sees own entry counted (predicate widens to include - # owner's non-archived non-approved entries). r = web_client.get( "/api/marketplace/categories?tab=flea", cookies=owner_cookies, ) diff --git a/tests/test_store_guardrails_purge.py b/tests/test_store_guardrails_purge.py index 0b679ab..64fc4ad 100644 --- a/tests/test_store_guardrails_purge.py +++ b/tests/test_store_guardrails_purge.py @@ -75,7 +75,7 @@ class TestPurgeBlockedBundles: sub_id, eid = _seed_with_bundle( conn, tmp_path / "store", "u1", "old-bad", - status="blocked_inline", days_old=45, + status="blocked_llm", days_old=45, ) plugin_dir = tmp_path / "store" / eid / "plugin" assert plugin_dir.exists() @@ -103,7 +103,7 @@ class TestPurgeBlockedBundles: sub_id, eid = _seed_with_bundle( conn, tmp_path / "store", "u1", "fresh-bad", - status="blocked_inline", days_old=2, + status="blocked_llm", days_old=2, ) result = purge_blocked_bundles( conn, ttl_days=30, @@ -166,7 +166,7 @@ class TestPurgeBlockedBundles: sub_id, eid = _seed_with_bundle( conn, tmp_path / "store", "u1", "x", - status="blocked_inline", days_old=999, + status="blocked_llm", days_old=999, ) result = purge_blocked_bundles( conn, ttl_days=0, diff --git a/tests/test_store_put_atomic.py b/tests/test_store_put_atomic.py index 757302c..3dbd303 100644 --- a/tests/test_store_put_atomic.py +++ b/tests/test_store_put_atomic.py @@ -124,9 +124,10 @@ class TestPutAtomicity: files={"file": ("evil.zip", evil_zip, "application/zip")}, cookies=owner_cookies, ) - # Inline-blocked uploads return 422 with a structured detail. + # Static-security failures hard-reject with the security_blocked + # code — no submission row, no version dir, no DB writes. assert u.status_code == 422, u.text - assert u.json()["detail"]["code"] == "submission_blocked" + assert u.json()["detail"]["code"] == "security_blocked" after_hash = _hash_tree(plugin_dir) assert after_hash == before_hash, (