feat(brand-logos): finish #4748 — abuse signals, owner slots, threaded Slack replies#4757
Merged
Conversation
…rt 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>
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>
…otify' into bokelley/4748-finish
…d 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>
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>
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>
…' into bokelley/4748-finish
…otify' into bokelley/4748-finish
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>
# Conflicts: # server/src/addie/mcp/brand-tools.ts # server/src/notifications/registry.ts # server/src/routes/brand-logos.ts # server/tests/unit/brand-logos-moderator-queue.test.ts # server/tests/unit/brand-logos-upload-auth.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 `pending_queue_full` on the next attempt. Defends against:
Verified owners bypass the check — they're attesting their own brand. Soft-pause: just rejects new attempts; existing pending stays. Same threshold applied to Addie's `upload_brand_logo` tool (counted as `system:addie`).
B. Per-brand reserved owner slots
`MAX_LOGOS_PER_BRAND` stays at 10. Community uploads (any status, `source='community'`) are capped at 5 of those slots so a verified owner who claims tomorrow always has room for their own logos even if community-pending filled first. Owner uploads still respect the overall 10. Returns 400 `community_cap_reached` when tripped.
C. Threaded approve/reject Slack replies
Migration 484 adds `slack_thread_ts TEXT` to `brand_logos`. The HTTP route + Addie tool now persist the ts returned by `notifyPendingBrandLogo` on the row. The review endpoint loads it before mutation and threads a verdict reply (`notifyBrandLogoReviewed`) under the original "Logo pending review" announcement. Channel reads as a conversation instead of disconnected verdicts. Silently skips when the row predates this PR or Slack isn't configured at upload time.
Tests
10 new tests in `brand-logos-abuse-signals.test.ts` cover:
38 logo-related tests pass together. TypeScript clean.
Test plan
Closes
Closes #4748.
🤖 Generated with Claude Code