Add Carbon (BuySellAds) ad provider for waiting room#529
Conversation
Introduces a pluggable ad-provider abstraction (gravity + carbon) on the server so adding ZeroClick later is a single-file drop-in. The Freebuff waiting room now fetches Carbon ads; in-chat ads continue to use Gravity. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Greptile SummaryThis PR introduces a pluggable server-side ad-provider abstraction ( Key changes:
Issues found:
Confidence Score: 3/5Hold for two targeted fixes before merging: the env.ts default bug means Carbon ads silently don't load in local dev, and the stale Chrome UA may cause Carbon to reject traffic as bot-like in all environments. The overall architecture is solid — the provider abstraction is clean, the DB migration is safe, and the impression flow is correctly secured (server-side pixel firing, user ownership check, rate limiting). However, two P1 issues undermine core functionality: the env.ts snapshot ordering bug breaks the documented dev setup, and Chrome 124 in a build dated April 2026 is meaningfully stale for fraud-detection purposes. The SSRF finding is real but lower severity given Carbon is a trusted party. Once the env default and UA version are fixed, this PR is ready to merge. packages/internal/src/env.ts (default capture order bug), cli/src/hooks/use-gravity-ad.ts (stale Chrome version), and web/src/app/api/v1/ads/impression/_post.ts (SSRF mitigation).
|
| Filename | Overview |
|---|---|
| packages/internal/src/env.ts | Non-prod CARBON_ZONE_KEY default is set via process.env mutation after serverProcessEnv is already captured as a static snapshot, so env.CARBON_ZONE_KEY remains undefined in local dev without explicit configuration — breaking the described dev workflow. |
| cli/src/hooks/use-gravity-ad.ts | provider option added and forwarded to the server correctly; however the hardcoded Chrome UA version (124.0.0.0) is two years stale relative to the 'Last bumped: 2026-04-21' comment, which may cause Carbon's fraud detection to reject traffic as bot-like. |
| web/src/app/api/v1/ads/impression/_post.ts | Impression firing extended to include extra_pixels from DB; server fetches these URLs without URL-scheme or host validation, creating a latent SSRF surface. |
| web/src/app/api/v1/ads/_post.ts | Provider dispatch, DB persistence, and client-stripping logic look correct; the .onConflictDoNothing() clause is a no-op since no relevant unique constraint exists on the table. |
| web/src/lib/ad-providers/carbon.ts | New Carbon/BuySellAds provider: correctly parses the response, normalizes to the shared shape, handles no-fill, and splits extra pixels. Logic is clean. |
| web/src/lib/ad-providers/types.ts | New shared type definitions for the ad-provider abstraction; well-structured and extensible for future providers. |
| web/src/lib/ad-providers/gravity.ts | Gravity logic extracted into the new provider pattern with no functional changes; correctly implements the AdProvider interface. |
| packages/internal/src/db/migrations/0045_mean_sleeper.sql | Clean three-statement migration: drops NOT NULL on payout, adds provider (NOT NULL with default), adds extra_pixels (nullable array) — all backward-compatible changes. |
| packages/internal/src/env-schema.ts | CARBON_ZONE_KEY added as optional string; schema and serverProcessEnv snapshot correctly updated. |
| cli/src/components/waiting-room-screen.tsx | Minimal, correct change: passes provider: 'carbon' to useGravityAd; existing hook wiring unchanged. |
| web/src/app/api/v1/ads/route.ts | Route handler correctly threads CARBON_ZONE_KEY and CB_ENVIRONMENT into postAds; no issues. |
| packages/internal/src/db/schema.ts | adImpression schema updated: provider column added with 'gravity' default, extra_pixels as nullable text[], payout made nullable — aligns with migration. |
Sequence Diagram
sequenceDiagram
participant CLI as CLI (waiting-room)
participant API as /api/v1/ads
participant Carbon as Carbon API
participant Gravity as Gravity API
participant DB as DB (ad_impression)
participant ImpAPI as /api/v1/ads/impression
CLI->>API: POST {provider: 'carbon', userAgent, ...}
alt CARBON_ZONE_KEY configured
API->>Carbon: GET /ads/{zoneKey}.json?useragent=&forwardedip=
Carbon-->>API: {ads: [{statlink, statimp, pixel, ...}]}
API->>DB: INSERT ad_impression (provider='carbon', extra_pixels=[...])
API-->>CLI: {ad: {adText, impUrl, ...}, variant: 'banner'}
else not configured
API-->>CLI: {ad: null, provider: 'carbon'}
end
CLI->>API: POST {provider: 'gravity', messages, ...}
API->>Gravity: POST /api/v1/ad
Gravity-->>API: [{adText, impUrl, payout, ...}]
API->>DB: INSERT ad_impression (provider='gravity', payout=X)
API-->>CLI: {ad: {...}, variant: 'banner' or 'choice'}
CLI->>ImpAPI: POST {impUrl}
ImpAPI->>DB: SELECT WHERE imp_url = impUrl
DB-->>ImpAPI: adRecord (provider, extra_pixels)
ImpAPI->>Carbon: GET statimp URL
ImpAPI->>Carbon: GET extra pixel URLs ([timestamp] substituted)
ImpAPI->>DB: UPDATE impression_fired_at = now()
ImpAPI-->>CLI: {success: true, creditsGranted: 0}
Comments Outside Diff (1)
-
cli/src/hooks/use-gravity-ad.ts, line 139-147 (link)Stale Chrome version will look bot-like to Carbon's fraud screening
The comment says "Last bumped: 2026-04-21" but
AD_CHROME_VERSIONis set to'124.0.0.0', which was released roughly two years before this date. Chrome updates approximately every four weeks, so the current stable release by April 2026 would be significantly higher. A UA that is two years behind the real Chrome stable channel is exactly the kind of signal that ad-network fraud-detection systems flag as non-human traffic.Consider updating
AD_CHROME_VERSIONto a value that matches Chrome stable around the "Last bumped" date, and also adding a quick sanity note in the on-boarding docs or a CI lint step so the version gets bumped on a real schedule.
Reviews (1): Last reviewed commit: "Add Carbon (BuySellAds) ad provider for ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!process.env.CARBON_ZONE_KEY) { | ||
| process.env.CARBON_ZONE_KEY = 'CVADC53U' | ||
| } |
There was a problem hiding this comment.
CARBON_ZONE_KEY default never reaches env.CARBON_ZONE_KEY
serverProcessEnv is a static snapshot of process.env values captured when env-schema.ts is first evaluated — which occurs during the import at the top of this file, before this block runs. So serverProcessEnv.CARBON_ZONE_KEY is already locked in as undefined, and serverEnvSchema.parse(serverProcessEnv) will produce env.CARBON_ZONE_KEY === undefined in non-prod environments where the var is not explicitly set.
The practical effect: the waiting room always returns { ad: null } locally unless the developer manually sets the env var, which contradicts the "no config needed in dev" intent stated in the comment.
The fix is to apply the non-prod fallback value inside serverProcessEnv itself (in env-schema.ts) rather than as a process.env mutation here, so the snapshot captures it before parse() is called.
| const extraPixels = (adRecord.extra_pixels ?? []).map((p) => | ||
| p.replaceAll('[timestamp]', now), | ||
| ) | ||
| const pixelUrls = [impUrl, ...extraPixels] | ||
|
|
||
| await Promise.all( | ||
| pixelUrls.map(async (pixelUrl) => { | ||
| try { | ||
| await fetch(pixelUrl) | ||
| } catch (error) { | ||
| logger.warn( | ||
| { | ||
| pixelUrl, | ||
| error: | ||
| error instanceof Error | ||
| ? { name: error.name, message: error.message } | ||
| : error, | ||
| }, | ||
| '[ads] Failed to fire impression pixel', | ||
| ) | ||
| } | ||
| }), | ||
| ) |
There was a problem hiding this comment.
extra_pixels are fetched unconditionally
extra_pixels is populated from Carbon's raw pixel field and stored in the DB by _post.ts. At impression time the server calls fetch(pixelUrl) for every stored URL with no allowlist or scheme/host validation.
Carbon is a trusted third-party, but if Carbon's API were ever compromised, or if a misconfiguration introduced attacker-controlled data into the extra_pixels column, this path would forward arbitrary server-side HTTP requests to any host reachable from your servers (internal VPCs, metadata endpoints, etc.).
A minimal mitigation is to validate that each pixel URL starts with https:// before firing:
const extraPixels = (adRecord.extra_pixels ?? [])
.map((p) => p.replaceAll('[timestamp]', now))
.filter((p) => p.startsWith('https://'))A stricter approach would also allowlist known Carbon domains (e.g. srv.buysellads.com, www.buysellads.com).
| await Promise.all( | ||
| adsToPersist.map((ad) => | ||
| db | ||
| .insert(schema.adImpression) | ||
| .values({ | ||
| user_id: userId, | ||
| provider: provider.id, | ||
| ad_text: ad.adText, | ||
| title: ad.title, | ||
| cta: ad.cta, | ||
| url: ad.url, | ||
| favicon: ad.favicon, | ||
| click_url: ad.clickUrl, | ||
| imp_url: ad.impUrl, | ||
| extra_pixels: ad.extraPixels ?? null, | ||
| payout: ad.payout != null ? String(ad.payout) : null, | ||
| credits_granted: 0, | ||
| }) | ||
| .onConflictDoNothing(), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
.onConflictDoNothing() will never trigger
The adImpression table's primary key id is generated via crypto.randomUUID() at insert time (unique by construction). There is no unique constraint on any other column such as imp_url. Because no uniqueness constraint can realistically be violated, .onConflictDoNothing() is effectively dead code here — it silently drops no conflicts.
If the intent is to deduplicate by imp_url (so the same ad is not double-inserted), add a unique index on imp_url in the schema and the migration. Otherwise, remove the clause to avoid misleading future readers.
Carbon's API doesn't expose a destination URL — `statlink` is a 302 tracker to the advertiser — so using it as `url` made the CLI render `srv.buysellads.com` as the ad's domain. Leave `url` empty; clicks still route through `clickUrl`. Also include `statview` (IAB viewable-impression pixel) alongside the advertiser `pixel` entries so viewable impressions actually fire. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
web/src/lib/ad-providers/—types,gravity,carbon) so adding ZeroClick later is a single-file drop-in./api/v1/adsnow acceptsprovider: 'gravity' | 'carbon'in the body and dispatches to the right provider; the impression endpoint firesimp_urlplus any storedextra_pixels(Carbon'spixelfield,[timestamp]substituted at fire time).waiting-room-screen.tsx) fetches Carbon ads; in-chat ads continue to use Gravity.0045_mean_sleeperaddsad_impression.providerandextra_pixels text[], and makespayoutnullable so Carbon (CPM) impressions don't pollute revenue dashboards with fake Gravity numbers.CARBON_ZONE_KEYenv var, defaulted to BSA's public test zoneCVADC53Uin non-prod only.Test plan
CARBON_ZONE_KEYlocally, load waiting room → request returns{ ad: null, provider: 'carbon' }without errors; chat still shows Gravity ads.CARBON_ZONE_KEY=CVADC53U(non-prod default), load waiting room → Carbon ad renders and anad_impressionrow is written withprovider='carbon'./api/v1/ads/impressionfiresstatimpand any extrapixelURLs ([timestamp]substituted); row getsimpression_fired_at.🤖 Generated with Claude Code