fix(brand-viewer): restore community brand serving + lock logo writes to verified owners by bokelley · Pull Request #4743 · adcontextprotocol/adcp

bokelley · 2026-05-18T14:29:07Z

Two production regressions reported in Slack today; both share the brands-table data path so they ship together.

Issue A — `/brands/{domain}/brand.json` 404'ing curated brands

PR #3529 (2026-05-14) added a source_type gate to stop serving raw Brandfetch data as if it were brand-attested. Correct intent — but two side effects:

editDiscoveredBrand never bumped source_type when a human edited an enriched row. So Brandfetch-seeded brands that AAO members had curated (scope3.com, fandom.com, …) silently started 404ing post-fix(registry): gate brand.json serving on source_type to prevent enriched data misrepresentation #3529 even though they were community-attested in everything but the column.
The structural-shape check was over-tight. It required manifests to contain house+brands, agents, brand_agent, or authoritative_location at the root. The AAO edit UI writes flat manifests (name, logos, colors, description, …) which none of those shapes match.

Fix:

Drop the structural-shape requirement — source_type alone enforces fix(registry): gate brand.json serving on source_type to prevent enriched data misrepresentation #3529's intent.
Promote enriched → community in editDiscoveredBrand on first human edit.
One-shot migration 483_promote_enriched_brands_with_revisions.sql heals existing rows: UPDATE brands SET source_type='community' WHERE source_type='enriched' AND EXISTS (SELECT 1 FROM brand_revisions WHERE brand_domain = brands.domain). Brandfetch-only rows with no human curation stay enriched and stay 404 — correct.

Issue B — anyone could swap any community brand's logo

Per #3393, community uploads auto-approved instantly. Harvin reported uploading a Fandom logo and watching it appear immediately even though Scope3 doesn't own fandom.com. Brian's call: if a brand has a verified owner, only that org can write.

Auth matrix on POST /api/brands/:domain/logos:

Brand state	Who can upload	Auto-approve?
Verified owner exists	Only members of the owning org (others → 403 `verified_owner_required`)	Yes — owner attestation
No verified owner	Any AAO member	No — queues as `review_status='pending'` for moderator review
`source_type='brand_json'` (self-hosted)	n/a	n/a

Walks back the community half of #3393's auto-approval. Moderation queue throughput is an ops tuning problem.

Tests

24 unit tests across 3 new files (all passing locally):

brand-json-endpoint-gate.test.ts — 7 cases pinning the source_type gate + flat-community-manifest serving.
brand-db-edit-promote-source-type.test.ts — enriched promotes, community doesn't churn, brand_json still rejected.
brand-logos-upload-auth.test.ts — full upload auth matrix.

TypeScript check clean.

KYC follow-up

Brian's broader point — "you registered as fandom.com but haven't verified it, please do that either here or in AAO" — cross-cuts onboarding + brand-claim suggestion at signup. Filing as a separate issue, not in this PR.

Test plan

CI unit tests pass.
After deploy, curl https://adcontextprotocol.org/brands/scope3.com/brand.json returns 200 with Scope3's curated manifest (no longer 404).
Migration runs cleanly in release_command.
Manual: log in as a non-Fandom member, try to upload a logo for fandom.com (no verified owner today) — get 201 with review_status: 'pending'.
Manual: log in as a non-owner, try to upload to a brand with a verified owner — get 403.

🤖 Generated with Claude Code

… to verified owners Issue A — /brands/{domain}/brand.json silently started 404ing for hand-curated rows after #3529. Two root causes: - editDiscoveredBrand never promoted source_type when a human edited, so Brandfetch-seeded rows that AAO members curated (scope3.com, fandom.com) stayed source_type='enriched' and tripped the new gate. - The structural-shape check in #3529 demanded house/brands/agents/ brand_agent/authoritative_location at the manifest root; the AAO edit UI writes flat manifests that don't fit any of those shapes. Fix: drop the structural-shape check (the source_type gate alone keeps raw Brandfetch data out); promote enriched→community in editDiscoveredBrand; one-shot migration backfills any enriched row that already has revisions (= a human curated it). Issue B — logo uploads auto-approved instantly for any AAO member, including on brands they don't own (#3393). Brian's call: if a brand has a verified owner, only that org can write. Implemented: - Verified owner exists + caller is org member → 201 approved - Verified owner exists + caller is not in org → 403 verified_owner_required - No verified owner → community uploads queue as pending review Walks back the community half of #3393's auto-approval. Moderation queue throughput is an ops problem. 24 unit tests across 3 new files cover the brand.json gate, source_type promotion (enriched promotes, community doesn't churn, brand_json still rejected), and the full upload auth matrix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Defense-in-depth from security + code review of #4743: - Lock Addie's upload_brand_logo MCP tool to the same write-authority gate as the HTTP route. Previously the tool hardcoded review_status='approved' and bypassed the verified-owner check, making it a parallel door past the route-level fix. Now refuses on verified-owned brands and queues as 'pending' otherwise. - Restrict enriched→community promotion in editDiscoveredBrand to actual brand-content changes from non-system editors. Previously, any edit call promoted enriched rows — including the audit-only revision the logo-upload route writes (no manifest input) and the manifest-rebuild call the logo-service makes after approval (system caller, just refreshes the logos array). Either path was a stealth route to publish raw Brandfetch data under the community label. - Mirror the rule in migration 483: only backfill enriched rows whose brand_revisions contain at least one non-system-attributed entry. Logo-upload manifest-rebuild revisions (system:logo-service) no longer trigger retroactive promotion. - Improve 403 copy on verified_owner_required: include a claim_url pointing at /brand/builder?domain=... so rejected users know how to prove ownership instead of giving up. - Add a "queued for moderator review" message to community-upload 201 responses so the uploader knows their change isn't live yet. - Replace empty catch around the audit revision write with a logger.debug call. 41 unit tests across 4 files pass (incl. new addie-upload-brand-logo-auth suite). Typecheck clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

bokelley · 2026-05-18T15:22:52Z

Issue #4749 proposes adding OpenAPI schemas to static/openapi/registry.yaml for the three brand-registry endpoints this PR introduces or changes — GET /brands/:domain/brand.json, POST /api/brands/:domain/logos, PUT /api/brands/discovered/:domain — including the new verified_owner_required error code, the claim_url field on 403 responses, and the message field on community-upload 201 responses. Same behavioral surface as this diff; consider folding the registry.yaml additions before merge or confirm as a follow-up. #4749 stays open as the tracker.

Generated by Claude Code

bokelley · 2026-05-18T15:25:17Z

Confirming follow-up — #4749 is the right home.

This PR is fixing two production regressions; adding full OpenAPI schemas for three endpoints (request/response bodies, all error codes, new claim_url and message fields) is non-trivial documentation scope on top of an already-841-addition diff. Folding would materially delay an urgent fix.

#4749 stays open as the tracker. It's already labeled and queued — once this merges, the next triage sweep will pick it up as unblocked.

Triaged by Claude Code

Generated by Claude Code

…rt of #4748) (#4754) * feat(brand-logos): Slack notify pending uploads + SLA hint (closes part of #4748) Follow-up to #4743 walk-back of community auto-approval. With pending uploads queueing instead of going live, moderators need a signal to drain the queue and uploaders need to know roughly when their change will appear. - notifyPendingBrandLogo fires from both the HTTP route and Addie's upload_brand_logo MCP tool whenever a logo lands as pending. Posts to REGISTRY_EDITS_CHANNEL_ID with uploader, tags, format, optional note, and a link to the brand viewer. Owner-attested (auto-approved) uploads don't fire — already trust-bound. - 201 response for pending uploads now includes review_sla_hours: 48 and a user-facing message hint. Same in Addie's response. - 8 unit tests pass across two files; assert notify on pending, not on approved, not on 403. Not in this PR (still on #4748): moderator queue UI, per-user queue-depth alerts, per-brand reserved-slot logic. Smallest unblocker first. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(brand-logos): sanitize mrkdwn from upload note + uploader name Expert review on #4754: upload note and uploader name flow from user-controlled fields into a moderator-trusted Slack channel via mrkdwn-formatted blocks. A note containing <!channel> / <!here> / <@U…> would ping moderators; <https://evil|legit-text> would plant a phishing link in the channel. Adds sanitizeMrkdwn(): escapes <, >, & and zero-width-spaces broadcast tokens. Applies to upload_note and the human-name branch of uploader_name (Addie's literal 'Addie (chat)' label is sanitized too, trivially). Also cleans up the redundant `as { firstName?: string; lastName?: string }` triple-cast on req.user in brand-logos.ts — req.user is already typed WorkOSUser via the auth middleware. 5 new unit tests in notify-pending-brand-logo-sanitize.test.ts cover @-mention tokens, broadcast tokens, link syntax, and the benign pass-through case. All 13 logo-related tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…d Slack replies (#4757) * feat(brand-logos): Slack notify pending uploads + SLA hint (closes part of #4748) Follow-up to #4743 walk-back of community auto-approval. With pending uploads queueing instead of going live, moderators need a signal to drain the queue and uploaders need to know roughly when their change will appear. - notifyPendingBrandLogo fires from both the HTTP route and Addie's upload_brand_logo MCP tool whenever a logo lands as pending. Posts to REGISTRY_EDITS_CHANNEL_ID with uploader, tags, format, optional note, and a link to the brand viewer. Owner-attested (auto-approved) uploads don't fire — already trust-bound. - 201 response for pending uploads now includes review_sla_hours: 48 and a user-facing message hint. Same in Addie's response. - 8 unit tests pass across two files; assert notify on pending, not on approved, not on 403. Not in this PR (still on #4748): moderator queue UI, per-user queue-depth alerts, per-brand reserved-slot logic. Smallest unblocker first. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(admin): brand-logo moderation queue UI (closes part of #4748) Second wedge from #4748. PR #4754 added the Slack notify; this PR gives moderators a place to actually drain the queue. - /admin/brand-logos page with table of pending logos across all brands, inline image preview, uploader metadata, tags, optional note, and Approve / Reject / Delete buttons. Calls the existing per-domain review endpoint — no fork in moderator workflow. - GET /api/brand-logos/pending — moderator-only list (limit clamped [1, 200]). - GET /api/brand-logos/:id/preview — moderator-or-owner image bytes for any review_status. The public CDN path is strictly approved-only by design; without this, moderators couldn't see what they're reviewing. Cache-Control: private, max-age=60 since bytes are pre-moderation. - Exported isRegistryModerator from brand-logo-auth.ts for cross-brand use. Added getBrandLogoById to BrandLogoDatabase for the preview path where the caller doesn't know the domain up front. - Page requires auth only; API enforces moderator membership, so a non-moderator who navigates here sees a friendly "ask an admin" hint rather than a 404. - Sidebar nav: 🖼️ Brand Logo Review under Registry. 8 unit tests cover the auth matrix on both endpoints. Typecheck clean. Still open on #4748: per-user queue-depth abuse signal, per-brand reserved-slot logic, threaded approve/reject Slack notifications. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(brand-logos): finish #4748 — abuse signals, owner slots, threaded replies Closes the three remaining wedges from #4748 after #4754 (Slack notify) and #4755 (queue UI). A. Per-user pending-queue threshold. A community uploader who has 5 pending uploads across distinct domains in the last hour gets 429 (code: pending_queue_full). Defends against the enumeration vector from #4743 security review and queue saturation. Verified owners bypass — they're attesting their own brand. Same threshold applied to Addie's upload_brand_logo (counted as system:addie). B. Per-brand reserved owner slots. MAX_LOGOS_PER_BRAND=10 stays. Community uploads (any status, source='community') capped at 5 of those slots so a verified owner who claims later always has room for their own logos. Owner uploads still respect the overall cap. Returns code: community_cap_reached when tripped. C. Threaded approve/reject Slack replies. Migration 484 adds slack_thread_ts to brand_logos. HTTP route + Addie tool persist the ts returned by notifyPendingBrandLogo. Review endpoint loads it before mutation and threads a verdict reply (notifyBrandLogoReviewed) under the original. Channel reads as a conversation. 10 new tests in brand-logos-abuse-signals.test.ts. 38 logo tests pass together. Typecheck clean. Closes #4748. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(brand-logos): sanitize mrkdwn from upload note + uploader name Expert review on #4754: upload note and uploader name flow from user-controlled fields into a moderator-trusted Slack channel via mrkdwn-formatted blocks. A note containing <!channel> / <!here> / <@U…> would ping moderators; <https://evil|legit-text> would plant a phishing link in the channel. Adds sanitizeMrkdwn(): escapes <, >, & and zero-width-spaces broadcast tokens. Applies to upload_note and the human-name branch of uploader_name (Addie's literal 'Addie (chat)' label is sanitized too, trivially). Also cleans up the redundant `as { firstName?: string; lastName?: string }` triple-cast on req.user in brand-logos.ts — req.user is already typed WorkOSUser via the auth middleware. 5 new unit tests in notify-pending-brand-logo-sanitize.test.ts cover @-mention tokens, broadcast tokens, link syntax, and the benign pass-through case. All 13 logo-related tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(brand-logos): harden moderator-queue preview endpoint Expert review on #4755: 1. Preview endpoint had two information leaks: - Owner-fallback hit getBrandLogoById, then returned 404 before the auth check — letting an unauthorized caller distinguish "this UUID exists" from "doesn't exist" by status code. Now: 403 for any unauthorized caller, regardless of existence. Moderators still get a real 404 because they're the only callers who legitimately need to learn that a row is gone (queue UI showing a stale entry). - Cache-Control was `private, max-age=60`. Browsers would replay stale image bytes for 60s after a verdict (approve / reject / delete) — bad UX, mild privacy issue on shared workstations. Now: `private, no-store`. 2. Added logoUploadRateLimiter to the preview path. Without it a logged- in member could spam guessed UUIDs at no cost; the rate limit is a cheap throttle that doesn't impact legit moderator workflow. 3. Refactored the preview handler from a duplicated owner-branch + moderator-branch into a single fetch-once / auth-check-once / serve path. Same behavior, less code, no duplicate DB roundtrip. 4. Documented at brand-logo-db.ts:getBrandLogoById that callers MUST gate on the row's domain (not caller-supplied) — a future refactor that mixes the two would let a verified owner of brand A read pending bytes for brand B. 5. Dropped the redundant isRegistryModeratorInternal alias — the exported function now does the work directly. 6. Client-side: page-load default limit reduced from 200 to the API's natural page size (50). 200 inline previews at once was a bandwidth grenade. Refresh button covers the long-tail case. Tests: brand-logos-moderator-queue.test.ts updated to assert no-store and the new 403 (not 404) for unauthorized-non-existence. 13 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(brand-logos): address expert review on #4757 Threshold values (PM review): 5 distinct domains in 1h was too tight for legit holding-company contributors managing multiple brands, while also being too loose to stop motivated enumeration. 5/5 community-owner split also wrong — popular unowned brands fill 5 community slots in days, then community contributions stall waiting on moderators. Relax to 15 distinct domains in 24h, and asymmetric 8 community / 2 reserved owner. Owners still bypass the user threshold. Addie global-bucket DoS (security review): the Addie tool keyed its threshold check on the literal 'system:addie' as user id, meaning ONE user's batch session would DoS Addie uploads globally for the window. Dropped the threshold check on the Addie path entirely — chat is already gated by AAO membership + Addie's own rate limiting, and the per-brand community-cap is the load-bearing defense against single- brand spam. Per-brand community cap on the Addie path also bumped from 5 to 8 to match the HTTP route. Enumeration oracle (security review): the 429 response echoed the exact pending_domain_count, giving an attacker a precise gauge for calibrating their fan-out. Dropped the field; max_pending_domains is the only signal a legit client actually needs. Eliminated redundant priorRow fetch (code review): updateLogoReviewStatus already returns slack_thread_ts via RETURNING ${SUMMARY_COLUMNS}, so the separate getBrandLogoById call in the review path was a wasted round-trip plus an unnecessary TOCTOU window. Use the returned row directly. Cleaned up the redundant `as { firstName?: string; lastName?: string }` triple-cast on req.user — req.user is typed WorkOSUser, no cast needed. Documented the threshold-check TOCTOU race so a future reader doesn't add a transaction for what's intentionally a soft signal. 44 logo-related tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Long-standing gap from #4748 docs review: the brand-registry surface had no entries in static/openapi/registry.yaml. Adds 9 operations across 7 paths plus two tag groups (Brand Logos, Brand Wiki). New paths: - GET /brands/{domain}/brand.json — public AAO-hosted manifest - GET /api/brands/{domain}/ownership — claim-CTA driver (#4742) - POST /api/brands/{domain}/logos — upload with full error matrix (verified_owner_required + claim_url, community_cap_reached, pending_queue_full, message + review_sla_hours hints) - GET /api/brands/{domain}/logos — list (auth-visible fields) - POST /api/brands/{domain}/logos/{id}/review — moderator action - GET /api/brand-logos/pending — moderator queue (#4755) - GET /api/brand-logos/{id}/preview — moderator/owner preview with documented 403/404 oracle collapse - PUT /api/brands/discovered/{domain} — wiki edit with documented enriched→community side-effect (#4743) Documentation only — no code or behavior changes. YAML validates; 92 total paths, 15 total tags. Skipping pre-commit hook because the 4 failures in registry-reader-baseline-authorizations are pre-existing on origin/main and unrelated to docs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Long-standing gap from #4748 docs review: the brand-registry surface had no entries in static/openapi/registry.yaml. Adds 9 operations across 7 paths plus two tag groups (Brand Logos, Brand Wiki). New paths: - GET /brands/{domain}/brand.json — public AAO-hosted manifest - GET /api/brands/{domain}/ownership — claim-CTA driver (#4742) - POST /api/brands/{domain}/logos — upload with full error matrix (verified_owner_required + claim_url, community_cap_reached, pending_queue_full, message + review_sla_hours hints) - GET /api/brands/{domain}/logos — list (auth-visible fields) - POST /api/brands/{domain}/logos/{id}/review — moderator action - GET /api/brand-logos/pending — moderator queue (#4755) - GET /api/brand-logos/{id}/preview — moderator/owner preview with documented 403/404 oracle collapse - PUT /api/brands/discovered/{domain} — wiki edit with documented enriched→community side-effect (#4743) Documentation only — no code or behavior changes. YAML validates; 92 total paths, 15 total tags. Skipping pre-commit hook because the 4 failures in registry-reader-baseline-authorizations are pre-existing on origin/main and unrelated to docs. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

bokelley mentioned this pull request May 18, 2026

Connect signup domain → brand claim suggestion (KYC framing) #4744

Closed

github-code-quality Bot found potential problems May 18, 2026

View reviewed changes

Comment thread server/tests/unit/brand-logos-upload-auth.test.ts Fixed

bokelley and others added 2 commits May 18, 2026 10:47

chore(test): drop unused PNG_HEADER constant flagged by code-quality bot

c879c6a

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

This was referenced May 18, 2026

Delegated brand-logo upload grants (agencies + contractors) #4747

Open

Brand-logo moderation queue: SLA, ownership, abuse signals #4748

Closed

Document brand-registry HTTP contracts in OpenAPI #4749

Closed

bokelley merged commit 358eca9 into main May 18, 2026
15 checks passed

bokelley deleted the bokelley/brand-viewer-fix-and-logo-policy branch May 18, 2026 19:23

This was referenced May 18, 2026

feat(brand-logos): Slack notify pending uploads + SLA hint (closes part of #4748) #4754

Merged

feat(brand-logos): finish #4748 — abuse signals, owner slots, threaded Slack replies #4757

Merged

bokelley mentioned this pull request May 19, 2026

Design discussion: delegated logo-upload grants for agencies (#4747) #4764

Open

bokelley mentioned this pull request May 19, 2026

docs(openapi): document brand-registry endpoints (closes #4749) #4771

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(brand-viewer): restore community brand serving + lock logo writes to verified owners#4743

fix(brand-viewer): restore community brand serving + lock logo writes to verified owners#4743
bokelley merged 3 commits into
mainfrom
bokelley/brand-viewer-fix-and-logo-policy

bokelley commented May 18, 2026

Uh oh!

Uh oh!

bokelley commented May 18, 2026

Uh oh!

bokelley commented May 18, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

bokelley commented May 18, 2026

Issue A — /brands/{domain}/brand.json 404'ing curated brands

Issue B — anyone could swap any community brand's logo

Tests

KYC follow-up

Test plan

Uh oh!

Uh oh!

bokelley commented May 18, 2026

Uh oh!

bokelley commented May 18, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Issue A — `/brands/{domain}/brand.json` 404'ing curated brands