Skip to content

fix(admin): auto-approve member logo uploads that pass format/size/safety checks#3393

Merged
bokelley merged 2 commits into
mainfrom
claude/issue-2568-logo-auto-approve
Apr 28, 2026
Merged

fix(admin): auto-approve member logo uploads that pass format/size/safety checks#3393
bokelley merged 2 commits into
mainfrom
claude/issue-2568-logo-auto-approve

Conversation

@bokelley

Copy link
Copy Markdown
Contributor

Closes #2568

Member logos uploaded during onboarding sit in a pending review queue indefinitely — no admin UI exists to process it, the only consumer is an Addie MCP tool (list_pending_brand_logos) most admins don't know to invoke, and members see a hardcoded "pending review" message with no resolution path. This is the escalation source identified in the Apr 11–18 Addie insights.

This PR removes the bottleneck by auto-approving all uploads that already pass the membership + ban + magic-bytes + SVG-sanitization + size gates. Source traceability (brand_owner vs community) is preserved. The reject/delete admin path is unchanged. The upload_brand_logo Addie tool was a separate code path with the same hardcoded pending logic — it's fixed in the same diff.

Non-breaking justification: No schema or wire changes. review_status was already returned in the HTTP upload response; callers that read it now see 'approved' instead of 'pending'. No existing client depends on community uploads being held in pending state.

Files changed:

  • server/src/routes/brand-logos.tsreviewStatus always 'approved'; manifest rebuild unconditional; edit summary records source
  • server/public/brand-viewer.html — success message uses result.review_status from API response
  • server/src/addie/mcp/brand-tools.tsupload_brand_logo tool: insert approved, call rebuildManifestLogos, return correct status; description updated
  • server/src/db/brand-logo-db.tsinsertBrandLogo default changed from 'pending' to 'approved' (latent footgun fix)
  • server/src/addie/jobs/brand-logo-digest.ts — comment updated: job is now a drain for pre-deploy pending logos only
  • .changeset/auto-approve-verified-owner-logos.md — removed contradicting note that community uploads still queue

Nits surfaced in review (not fixed here):

  • countBrandLogos and getByDomainAndSha256 still reference IN ('pending', 'approved') — functionally fine, cosmetic cleanup follow-up
  • rebuildManifestLogos + editDiscoveredBrand are two sequential writes to the same brand row per request with no transaction wrap — pre-existing race under concurrent same-domain uploads; tracked separately

Pre-PR review:

  • code-reviewer: approved — no blockers; XSS surface in HTML change is safe (message chosen from two hardcoded literals, not user input); rebuildManifestLogos placement correct in both code paths
  • internal-tools-strategist: approved — DB default fix and digest annotation address the two operational blockers; isOwner is still needed for source field and audit summary so the call is not dead

Triage-managed PR. This bot does not currently iterate on
review comments or PR conversation threads (only on the source
issue). To unblock:

  • Push fixup commits directly: gh pr checkout <num>
    fix → push.
  • Or re-trigger: comment /triage execute on the source
    issue.

See #3121
for context.

Session: https://claude.ai/code/session_019WyF5yW7B8jkypARtKHqcg


Generated by Claude Code

@bokelley bokelley added the claude-triaged Issue has been triaged by the Claude Code triage routine. Remove to re-triage. label Apr 28, 2026
@bokelley

Copy link
Copy Markdown
Contributor Author

TypeScript Build is failing — need to investigate which test imports member-context or auth chain that's now broken. Common pattern: lazy-import the problematic dependency, or add proper vi.mock() for auth/workos in the failing test. Reference: PR #3258's lazy-import fix.

@bokelley

Copy link
Copy Markdown
Contributor Author

Fixed in 634ff75.

admin-tools.ts line 21 had a top-level import { invalidateMemberContextCache } from '../member-context.js' that wasn't covered by PR #3258 (which fixed the same pattern in member-tools.ts). That import pulls in member-context.ts → middleware/auth.ts → new WorkOS(WORKOS_API_KEY!) at module load time — any test importing admin-tools.ts without WORKOS_API_KEY set fails.

Applied the same lazy-import fix: removed the top-level import and replaced both call sites (lines 7672 and 7784) with const { invalidateMemberContextCache } = await import('../member-context.js') inside the handler bodies.


Generated by Claude Code

@bokelley

Copy link
Copy Markdown
Contributor Author

Acknowledged — lazy-import fix in 634ff75 looks correct, same pattern as the #3258 fix in member-tools.ts. No further action needed on this thread.


Generated by Claude Code

@bokelley bokelley marked this pull request as ready for review April 28, 2026 18:34
@bokelley

Copy link
Copy Markdown
Contributor Author

Routine: this PR is DIRTY against main. Please rebase against origin/main, resolve conflicts (likely in admin-side logo/brand files since other admin PRs landed), and re-push. No content changes needed — just the rebase. Mary launch blocker, please prioritize.

@bokelley

Copy link
Copy Markdown
Contributor Author

Routine: persistently DIRTY against main as other PRs land. Please rebase locally against origin/main, resolve conflicts, and force-push. Once green and rebased, will admin-merge.

claude added 2 commits April 28, 2026 19:03
…fety checks

Closes #2568. Mary launch blocker — logos uploaded by members sit in a
pending queue with no admin UI and no member-visible resolution path.

- brand-logos.ts: community uploads now auto-approve (review_status:
  'approved') after passing the existing membership + ban + magic-bytes +
  SVG-sanitization + size gates. Manifest rebuild extended to all uploads
  (was owner-only). Source still tracks brand_owner vs community.
- brand-viewer.html: success message now reflects actual review_status
  from the API response ('Logo uploaded and live.' vs 'pending review.')
  instead of hardcoding the pending message.
- brand-tools.ts: Addie upload_brand_logo tool had the same pending-only
  logic as a separate code path. Fixed to match HTTP route: inserts as
  approved, rebuilds manifest, returns correct status.
- brand-logo-db.ts: insertBrandLogo default changed from 'pending' to
  'approved' so future callers that omit review_status don't silently
  re-introduce the queue.
- brand-logo-digest.ts: comment updated to explain the job is now a
  drain for pre-deploy historical pending logos only.
- changeset auto-approve-verified-owner-logos.md: removed now-incorrect
  note that community uploads still queue.

https://claude.ai/code/session_019WyF5yW7B8jkypARtKHqcg
Top-level import of member-context.ts transitively loads middleware/auth.ts,
which constructs a WorkOS client at module load time. Any test that imports
admin-tools.ts without WORKOS_API_KEY set (or without mocking middleware/auth)
fails with a WorkOS constructor error.

Matches PR #3258's fix for the same pattern in member-tools.ts: remove the
top-level import and use `await import('../member-context.js')` inside the
two handlers that call invalidateMemberContextCache().

https://claude.ai/code/session_01J5x2do9fKEfGg3uZGZtAzw
@bokelley bokelley force-pushed the claude/issue-2568-logo-auto-approve branch from 634ff75 to b0e2ece Compare April 28, 2026 19:03
@bokelley

Copy link
Copy Markdown
Contributor Author

Rebased against origin/main and force-pushed.

One modify/delete conflict on .changeset/auto-approve-verified-owner-logos.md (deleted in main, present in the PR commit) — resolved by accepting the deletion; the replacement changeset logo-auto-approve-onboarding.md was already in the commit and carried through cleanly. Branch is now two commits ahead of main with no conflicts.


Generated by Claude Code

@bokelley

Copy link
Copy Markdown
Contributor Author

Got it — rebase noted, conflict resolved cleanly, logo-auto-approve-onboarding.md carried through. Branch looks good from here.


Generated by Claude Code

@bokelley

Copy link
Copy Markdown
Contributor Author

Acknowledged — glad the rebase carried through cleanly. No action needed on my end.


Generated by Claude Code

@bokelley

Copy link
Copy Markdown
Contributor Author

Noted, thanks for confirming. No further action from this end.


Generated by Claude Code


Generated by Claude Code

@bokelley

Copy link
Copy Markdown
Contributor Author

Acknowledged — standing by if anything else comes up.


Generated by Claude Code


Generated by Claude Code

@bokelley bokelley merged commit d881467 into main Apr 28, 2026
15 checks passed
@bokelley bokelley deleted the claude/issue-2568-logo-auto-approve branch April 28, 2026 19:46
bokelley added a commit that referenced this pull request May 18, 2026
… to verified owners (#4743)

* fix(brand-viewer): restore community brand serving + lock logo writes 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>

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

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

* fix(brand-viewer): address expert-review feedback

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>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

claude-triaged Issue has been triaged by the Claude Code triage routine. Remove to re-triage.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

admin: logo approval queue — auto-approve or build review UI

2 participants