From 0ee9ecf7db9898ecd8b2c872ad90f700629bbd53 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Thu, 17 Sep 2026 01:07:54 +1000 Subject: [PATCH 1/2] feat(identity): count email clicks as daily aggregates Loop 2 is judged on digest click-throughs, and #240 removed the only measure along with the per-reader click history. Links now carry the campaign, issue, placement, and path, and a daily counter stores nothing that names a reader. --- layers/admin/server/api/admin/weekly.get.ts | 18 +- .../admin/server/utils/weekly-engagement.ts | 20 +- .../identity/server/api/e/[campaign].get.ts | 39 +++ layers/identity/server/api/e/weekly.get.ts | 28 -- .../server/utils/daily-health-check.spec.ts | 8 + .../server/utils/daily-health-check.ts | 27 ++ .../identity/server/utils/digest-delivery.ts | 1 + .../identity/server/utils/digest-template.ts | 3 + layers/identity/server/utils/email-clicks.ts | 166 ++++++++++++ .../identity/server/utils/weekly-delivery.ts | 1 + .../identity/server/utils/weekly-template.ts | 30 ++- .../identity/server/utils/weekly-tracking.ts | 45 ---- migrations/0120_email_click_counts.sql | 21 ++ schema.sql | 12 + .../daily-health-check-x-discovery.test.ts | 2 +- test/unit/email-clicks.test.ts | 251 ++++++++++++++++++ test/unit/weekly-admin-report.test.ts | 10 +- test/unit/weekly-click-redirect.test.ts | 60 ----- test/unit/weekly-email-improvements.test.ts | 8 - 19 files changed, 598 insertions(+), 152 deletions(-) create mode 100644 layers/identity/server/api/e/[campaign].get.ts delete mode 100644 layers/identity/server/api/e/weekly.get.ts create mode 100644 layers/identity/server/utils/email-clicks.ts delete mode 100644 layers/identity/server/utils/weekly-tracking.ts create mode 100644 migrations/0120_email_click_counts.sql create mode 100644 test/unit/email-clicks.test.ts delete mode 100644 test/unit/weekly-click-redirect.test.ts diff --git a/layers/admin/server/api/admin/weekly.get.ts b/layers/admin/server/api/admin/weekly.get.ts index c1260a6e..8e14d1f4 100644 --- a/layers/admin/server/api/admin/weekly.get.ts +++ b/layers/admin/server/api/admin/weekly.get.ts @@ -25,7 +25,9 @@ interface WindowRow { trending_rows: number first_claimed_at: number last_sent_at: number | null + clicks: number unsubscribes: number + share_clicks: number } interface ProblemRow { @@ -56,8 +58,12 @@ export interface AdminWeeklyResponse { trendingRows: number firstClaimedAt: number lastAcceptedAt: number | null + /** Aggregate link clicks for this issue, share links excluded. */ + clicks: number + clicksPerAccepted: number | null unsubscribes: number unsubscribeRate: number | null + shareClicks: number }> /** Rows without a resolved provider outcome, newest first. */ problems: Array<{ @@ -94,6 +100,11 @@ export default defineApiHandler({ SUM(trending_count) AS trending_rows, MIN(claimed_at) AS first_claimed_at, MAX(r.sent_at) AS last_sent_at, + (SELECT COALESCE(SUM(c.clicks), 0) + FROM email_click_counts c + WHERE c.campaign = 'weekly' + AND c.issue = r.window_end + AND c.placement != 'share') AS clicks, (SELECT COUNT(DISTINCT e.user_id) FROM email_preference_events e WHERE e.list = 'weekly' @@ -105,7 +116,12 @@ export default defineApiHandler({ AND er.sent_at IS NOT NULL AND e.occurred_at >= er.sent_at AND e.occurred_at < er.sent_at + 604800 - )) AS unsubscribes + )) AS unsubscribes, + (SELECT COALESCE(SUM(c.clicks), 0) + FROM email_click_counts c + WHERE c.campaign = 'weekly' + AND c.issue = r.window_end + AND c.placement = 'share') AS share_clicks FROM weekly_runs r GROUP BY r.window_end ORDER BY r.window_end DESC diff --git a/layers/admin/server/utils/weekly-engagement.ts b/layers/admin/server/utils/weekly-engagement.ts index f0151d09..a56aa413 100644 --- a/layers/admin/server/utils/weekly-engagement.ts +++ b/layers/admin/server/utils/weekly-engagement.ts @@ -1,18 +1,36 @@ export interface WeeklyEngagementCounts { accepted: number + /** Counted clicks on the issue's links, share links excluded. */ + clicks: number unsubscribes: number + /** Counted clicks on the share link. */ + share_clicks: number } export interface WeeklyEngagementSummary { + clicks: number + /** + * Clicks per accepted email. Counts are aggregate, so one reader can click + * twice; this is not a unique click rate. + */ + clicksPerAccepted: number | null unsubscribes: number unsubscribeRate: number | null + shareClicks: number +} + +function perAccepted(count: number, accepted: number): number | null { + return accepted > 0 ? count / accepted : null } export function summarizeWeeklyEngagement( counts: WeeklyEngagementCounts, ): WeeklyEngagementSummary { return { + clicks: counts.clicks, + clicksPerAccepted: perAccepted(counts.clicks, counts.accepted), unsubscribes: counts.unsubscribes, - unsubscribeRate: counts.accepted > 0 ? counts.unsubscribes / counts.accepted : null, + unsubscribeRate: perAccepted(counts.unsubscribes, counts.accepted), + shareClicks: counts.share_clicks, } } diff --git a/layers/identity/server/api/e/[campaign].get.ts b/layers/identity/server/api/e/[campaign].get.ts new file mode 100644 index 00000000..ac51b364 --- /dev/null +++ b/layers/identity/server/api/e/[campaign].get.ts @@ -0,0 +1,39 @@ +import { getQuery, getRouterParam, sendRedirect, setHeader } from 'h3' +import { defineApiHandler } from '#shared/server/handler' +import { parseEmailClick, recordEmailClick } from '../../utils/email-clicks' + +/** + * Counts a click from the weekly or digest email, then sends the reader on. + * + * Only the aggregate counter changes. The link names no reader, and nothing + * from the request is stored beyond the campaign, issue, placement, and path. + * The wide event carries the campaign and placement, never the path. + * + * Counting is best effort. A reader who clicked is owed the page, so a failed + * write is reported and the redirect still happens. + */ +export default defineApiHandler({ + handler: async ({ event, platform }) => { + const click = parseEmailClick(getRouterParam(event, 'campaign'), getQuery(event)) + + if (click._tag === 'counted') { + const { campaign, placement } = click.key + await recordEmailClick(platform.db, click.key, new Date()).then( + () => emitOperationalEvent(createWideEvent({ operation: 'email-click', outcome: 'success', reason: `${campaign}/${placement}` }), 'info'), + (error: unknown) => emitOperationalEvent(createWideEvent({ + operation: 'email-click', + outcome: 'failed', + reason: `${campaign}/${placement}: ${error instanceof Error ? error.name : 'unknown'}`, + })), + ) + } + else { + emitOperationalEvent(createWideEvent({ operation: 'email-click', outcome: 'skipped', reason: click.reason }), 'info') + } + + // A click is a one-off, and a cached redirect would hide every later one. + setHeader(event, 'cache-control', 'private, no-store') + // An invalid target names no page. Home is the safe landing. + return sendRedirect(event, click._tag === 'invalid' ? '/' : click.to, 302) + }, +}) diff --git a/layers/identity/server/api/e/weekly.get.ts b/layers/identity/server/api/e/weekly.get.ts deleted file mode 100644 index effc86ea..00000000 --- a/layers/identity/server/api/e/weekly.get.ts +++ /dev/null @@ -1,28 +0,0 @@ -import { defineApiHandler } from '#shared/server/handler' -import { parseWeeklyClick } from '../../utils/weekly-tracking' - -/** - * Redirects the tracked links inside already delivered weekly and digest - * emails. - * - * The click recording is retired, but every send still sitting in a subscriber's - * inbox routes its links through here, so the endpoint stays: it validates the - * destination and sends the reader on. It records nothing. - * - * The destination is a site-relative path and the origin is rebuilt by the - * browser, so this endpoint cannot redirect off-site whatever the query says. - * A tracking redirect that accepts an absolute URL is an open redirect, and an - * open redirect reached from an email is a phishing primitive. - */ -export default defineApiHandler({ - handler: ({ event }) => { - const target = parseWeeklyClick(getQuery(event)) - - // An invalid target names no page. Home is the safe landing. - const path = target._tag === 'ok' ? target.path : '/' - - // A click is a one-off, and a cached redirect would hide every later one. - setHeader(event, 'cache-control', 'private, no-store') - return sendRedirect(event, path, 302) - }, -}) diff --git a/layers/identity/server/utils/daily-health-check.spec.ts b/layers/identity/server/utils/daily-health-check.spec.ts index 08a233e3..c513797a 100644 --- a/layers/identity/server/utils/daily-health-check.spec.ts +++ b/layers/identity/server/utils/daily-health-check.spec.ts @@ -44,6 +44,7 @@ function summary(overrides: Partial = {}): DailyHealthC newUsers24h: 1, digestsSent24h: 4, digestsFailed24h: 0, + emailClicks: { fromDay: '2026-07-21', weekly: 2, digest: 5 }, }, pipeline: { syncJobs: [{ name: 'sync-github-skills', status: 'ok', lastRunAt: 1_774_473_600, stale: false, error: null }], @@ -332,6 +333,7 @@ describe('buildDailyHealthCheck', () => { CREATE TABLE skill_subscriptions (owner TEXT, repo TEXT); CREATE TABLE collection_skills_v2 (owner TEXT, repo TEXT); CREATE TABLE activity (owner TEXT, repo TEXT, name TEXT, occurred_at INTEGER); + CREATE TABLE email_click_counts (day TEXT, campaign TEXT, issue INTEGER, placement TEXT, path TEXT, clicks INTEGER); CREATE TABLE digest_runs ( status TEXT, sent_at INTEGER, @@ -399,6 +401,11 @@ describe('buildDailyHealthCheck', () => { INSERT INTO collections_v2 VALUES (NULL); INSERT INTO user_starred_repos VALUES ('owner', 'repo'); INSERT INTO activity VALUES ('owner', 'repo', 'skill', ${nowSec - 60}); + -- Counters are per UTC day. The window starts on 07-21, so 07-20 is out. + INSERT INTO email_click_counts VALUES ('2026-07-20', 'digest', 1, 'overflow', '/me/likes', 40); + INSERT INTO email_click_counts VALUES ('2026-07-21', 'digest', 1, 'overflow', '/me/likes', 2); + INSERT INTO email_click_counts VALUES ('2026-07-22', 'digest', 1, 'footer', '/', 3); + INSERT INTO email_click_counts VALUES ('2026-07-22', 'weekly', 2, 'cta', '/skills', 1); INSERT INTO digest_runs VALUES ('sent', ${nowSec - 60}, ${nowSec - 60}, ${nowSec - 60}, ${nowSec - 60}); -- The discovery cursor is a single fixed row in production, and the -- health query reads it by key. Without it the query returns no row at @@ -470,6 +477,7 @@ describe('buildDailyHealthCheck', () => { expect(built.warnings).toEqual([]) expect(built.inventory).toMatchObject({ skills: 1, repos: 2, users: 1, watchedRepos: 1 }) expect(built.activity).toMatchObject({ newSkills24h: 1, repoChanges24h: 1, digestsSent24h: 1 }) + expect(built.activity.emailClicks).toEqual({ fromDay: '2026-07-21', weekly: 1, digest: 5 }) expect(built.pipeline).toMatchObject({ newlyBrokenReposTotal24h: 1, newlyBrokenReposImpacted24h: 0, diff --git a/layers/identity/server/utils/daily-health-check.ts b/layers/identity/server/utils/daily-health-check.ts index 8681613c..851ec0e0 100644 --- a/layers/identity/server/utils/daily-health-check.ts +++ b/layers/identity/server/utils/daily-health-check.ts @@ -66,6 +66,18 @@ export interface DailyHealthCheckSummary { newUsers24h: number digestsSent24h: number digestsFailed24h: number + /** + * Aggregate email link clicks, per campaign. + * + * Counters are kept per UTC day, so this sums whole days from `fromDay` + * onwards rather than an exact 24 hours. Digest clicks are the Loop 2 + * evidence bar. + */ + emailClicks: { + fromDay: string + weekly: number + digest: number + } /** * The most recent weekly run, not a 24 hour count. * @@ -783,6 +795,17 @@ async function loadInventory(db: D1Database): Promise { + const row = await first<{ weekly: number | null, digest: number | null }>(db, ` + SELECT + SUM(clicks) FILTER (WHERE campaign = 'weekly') AS weekly, + SUM(clicks) FILTER (WHERE campaign = 'digest') AS digest + FROM email_click_counts + WHERE day >= ?1 + `, [fromDay]) + return { fromDay, weekly: numberValue(row.weekly), digest: numberValue(row.digest) } +} + async function loadActivity( db: D1Database, sinceSec: number, @@ -799,6 +822,8 @@ async function loadActivity( + (SELECT COUNT(*) FROM digest_runs WHERE status IN ('sending', 'uncertain'))) AS digests_failed_24h `, [sinceSec]) + const fromDay = new Date(sinceSec * 1000).toISOString().slice(0, 10) + const emailClicks = await capture(warnings, 'email clicks', { fromDay, weekly: 0, digest: 0 }, () => loadEmailClicks(db, fromDay)) const weekly = await capture(warnings, 'weekly delivery', null, () => loadWeeklyRun(db)) return { newSkills24h: numberValue(row.new_skills_24h), @@ -806,6 +831,7 @@ async function loadActivity( newUsers24h: numberValue(row.new_users_24h), digestsSent24h: numberValue(row.digests_sent_24h), digestsFailed24h: numberValue(row.digests_failed_24h), + emailClicks, weekly, } } @@ -1153,6 +1179,7 @@ export async function buildDailyHealthCheck( newUsers24h: 0, digestsSent24h: 0, digestsFailed24h: 0, + emailClicks: { fromDay: since.toISOString().slice(0, 10), weekly: 0, digest: 0 }, weekly: null, }, () => loadActivity(db, sinceSec, warnings)) const pipeline = await capture(warnings, 'pipeline', { diff --git a/layers/identity/server/utils/digest-delivery.ts b/layers/identity/server/utils/digest-delivery.ts index 7201a541..4229c8ce 100644 --- a/layers/identity/server/utils/digest-delivery.ts +++ b/layers/identity/server/utils/digest-delivery.ts @@ -615,6 +615,7 @@ export async function runDigestDeliveryForUser( const rendered = deps.render({ login: user.login, recipientName: user.name ?? null, + countClicks: true, windowStart: selection.windowStart, windowEnd: selection.windowEnd, unsubscribeUrl, diff --git a/layers/identity/server/utils/digest-template.ts b/layers/identity/server/utils/digest-template.ts index 37b2d592..5fec4193 100644 --- a/layers/identity/server/utils/digest-template.ts +++ b/layers/identity/server/utils/digest-template.ts @@ -21,6 +21,8 @@ export interface DigestRepoEntry { export interface DigestRenderInput { login: string recipientName?: string | null + /** Route same-site links through the aggregate click counter. Real sends only. */ + countClicks?: boolean windowStart: number windowEnd: number entries: DigestRepoEntry[] @@ -56,6 +58,7 @@ export function renderDigest(input: DigestRenderInput): DigestRender { return renderWeekly({ edition: 'digest', recipientName: input.recipientName ?? null, + countClicks: input.countClicks ?? false, windowStart: input.windowStart, windowEnd: input.windowEnd, likedChanges, diff --git a/layers/identity/server/utils/email-clicks.ts b/layers/identity/server/utils/email-clicks.ts new file mode 100644 index 00000000..b0d5455d --- /dev/null +++ b/layers/identity/server/utils/email-clicks.ts @@ -0,0 +1,166 @@ +/** + * Aggregate click counts for the weekly and digest emails. + * + * Loop 2 is judged on digest click-throughs, and an email is the one surface + * where nothing else can observe a click. So same-site links in a sent email + * pass through `/api/e/{campaign}`, which adds one to a daily counter and + * redirects. + * + * The link carries no reader. It names the campaign, the issue (the send's + * `window_end`), the placement, and the destination path. Every recipient of an + * issue gets identical links, so a click cannot be tied to a person, and the + * counter stores nothing that could. + * + * The destination is a site-relative path, never a URL. An off-site redirect + * is not expressible, because a tracking redirect that accepts a URL is an open + * redirect, and an open redirect reached from an email is a phishing primitive. + */ + +export type EmailCampaign = 'weekly' | 'digest' + +export type EmailPlacement = 'liked' | 'trending' | 'cta' | 'footer' | 'overflow' | 'share' + +export interface EmailClickKey { + campaign: EmailCampaign + /** The send's `window_end`, in Unix seconds. Shared by every recipient. */ + issue: number + placement: EmailPlacement + /** The destination path with any query or fragment removed. */ + path: string +} + +export type EmailClick + = | { _tag: 'counted', to: string, key: EmailClickKey } + /** A safe destination that is not counted. Old links still land. */ + | { _tag: 'uncounted', to: string, reason: 'legacy-link' | 'bad-campaign' | 'bad-placement' | 'bad-issue' } + | { _tag: 'invalid', reason: 'missing-path' | 'not-site-relative' | 'path-too-long' } + +export const EMAIL_CLICK_ROUTE = '/api/e' + +/** Longer paths are not a page this site sends, and they bloat the counter. */ +const MAX_PATH_LENGTH = 512 + +const CAMPAIGNS = new Set(['weekly', 'digest']) +const PLACEMENTS = new Set(['liked', 'trending', 'cta', 'footer', 'overflow', 'share']) + +/** Backslash, whitespace, or a control character. */ +function unsafePathCharacter(character: string): boolean { + const code = character.codePointAt(0)! + return character === '\\' || code <= 0x20 || code === 0x7F || /\s/.test(character) +} + +/** + * A path that cannot leave the site, or the reason it could. + * + * `//host` and `/\host` resolve to another origin in browsers. Browsers also + * strip tabs and newlines before parsing, so `/\t/host` becomes `//host`. + * This rejects any backslash, whitespace, or control character for that reason. + */ +export function parseSiteRelativePath(value: unknown): { _tag: 'ok', path: string } | { _tag: 'invalid', reason: 'missing-path' | 'not-site-relative' | 'path-too-long' } { + if (typeof value !== 'string' || !value) + return { _tag: 'invalid', reason: 'missing-path' } + if (value.length > MAX_PATH_LENGTH) + return { _tag: 'invalid', reason: 'path-too-long' } + if (!value.startsWith('/') || value.startsWith('//') || [...value].some(unsafePathCharacter)) + return { _tag: 'invalid', reason: 'not-site-relative' } + + // Second opinion from a real URL parser: the path must keep the base origin. + const base = 'https://origin.invalid' + if (!URL.canParse(value, base) || new URL(value, base).origin !== base) + return { _tag: 'invalid', reason: 'not-site-relative' } + + return { _tag: 'ok', path: value } +} + +function parseIssue(value: unknown): number | null { + if (typeof value !== 'string' || !/^\d{1,12}$/.test(value)) + return null + const issue = Number(value) + return issue > 0 ? issue : null +} + +function countedPath(to: string): string { + const end = to.search(/[?#]/) + return end === -1 ? to : to.slice(0, end) +} + +/** + * Parses one click request at the boundary. + * + * `campaign` is the route segment. The query carries `to` (path), `p` + * (placement), and `i` (issue). Links sent before counting was restored carry + * `p` as the path and `k` as the placement; they still redirect, uncounted. + */ +export function parseEmailClick(campaign: unknown, query: Record): EmailClick { + if (query.to === undefined && query.k !== undefined) { + const legacy = parseSiteRelativePath(query.p) + return legacy._tag === 'ok' ? { _tag: 'uncounted', to: legacy.path, reason: 'legacy-link' } : legacy + } + + const target = parseSiteRelativePath(query.to) + if (target._tag === 'invalid') + return target + const to = target.path + + if (typeof campaign !== 'string' || !CAMPAIGNS.has(campaign)) + return { _tag: 'uncounted', to, reason: 'bad-campaign' } + if (typeof query.p !== 'string' || !PLACEMENTS.has(query.p)) + return { _tag: 'uncounted', to, reason: 'bad-placement' } + const issue = parseIssue(query.i) + if (issue === null) + return { _tag: 'uncounted', to, reason: 'bad-issue' } + + return { + _tag: 'counted', + to, + key: { + campaign: campaign as EmailCampaign, + issue, + placement: query.p as EmailPlacement, + path: countedPath(to), + }, + } +} + +export interface EmailLinkContext { + siteUrl: string + campaign: EmailCampaign + issue: number +} + +/** + * The counted form of one email link, or the link unchanged. + * + * Off-site links stay as they are. A link this cannot rewrite costs a count, + * never a broken email. + */ +export function countedEmailUrl(context: EmailLinkContext, url: string, placement: EmailPlacement): string { + const origin = context.siteUrl.replace(/\/+$/, '') + const path = url === origin + ? '/' + : url.startsWith(`${origin}/`) ? url.slice(origin.length) : null + if (path === null || parseSiteRelativePath(path)._tag === 'invalid') + return url + + const params = new URLSearchParams({ to: path, p: placement, i: String(context.issue) }) + return `${origin}${EMAIL_CLICK_ROUTE}/${context.campaign}?${params.toString()}` +} + +/** The UTC calendar day a click lands in, as `YYYY-MM-DD`. */ +export function emailClickDay(now: Date): string { + return now.toISOString().slice(0, 10) +} + +/** + * Adds one click to its daily counter row. + * + * The row key is the whole record: day, campaign, issue, placement, and path. + * No reader, address, or client detail is stored. + */ +export async function recordEmailClick(db: D1Database, key: EmailClickKey, now: Date): Promise { + await db.prepare( + `INSERT INTO email_click_counts (day, campaign, issue, placement, path, clicks) + VALUES (?1, ?2, ?3, ?4, ?5, 1) + ON CONFLICT (day, campaign, issue, placement, path) DO UPDATE SET clicks = clicks + 1`, + ).bind(emailClickDay(now), key.campaign, key.issue, key.placement, key.path).run() +} diff --git a/layers/identity/server/utils/weekly-delivery.ts b/layers/identity/server/utils/weekly-delivery.ts index 63116876..c4c6e5c4 100644 --- a/layers/identity/server/utils/weekly-delivery.ts +++ b/layers/identity/server/utils/weekly-delivery.ts @@ -121,6 +121,7 @@ export async function runWeeklyForUser( const unsubscribeUrl = `${input.siteUrl}/api/unsubscribe?t=${encodeURIComponent(unsubscribeToken)}&list=weekly` const rendered = deps.render({ recipientName: user.name ?? null, + countClicks: true, windowStart: input.windowStart, windowEnd: input.windowEnd, likedChanges: [], diff --git a/layers/identity/server/utils/weekly-template.ts b/layers/identity/server/utils/weekly-template.ts index b9b132e0..dc1f1f16 100644 --- a/layers/identity/server/utils/weekly-template.ts +++ b/layers/identity/server/utils/weekly-template.ts @@ -11,8 +11,10 @@ * inlined at every use rather than being set through classes. */ +import type { EmailPlacement } from './email-clicks' import { githubAvatarProxyUrl } from '#shared/image-proxy' import { canonicalRepoSkillPath } from '#shared/skill-routes' +import { countedEmailUrl } from './email-clicks' export type WeeklyTheme = 'light' | 'dark' @@ -160,6 +162,14 @@ export interface WeeklyRenderInput { login?: string | null /** A person's display name. Omit the greeting when unavailable. */ recipientName?: string | null + /** + * Route same-site links through the aggregate click counter. + * + * Only real sends set this. Previews and the homepage demo leave it off, so + * an operator or a visitor never adds to an issue's count. The links carry + * the edition and `windowEnd`, never the recipient. + */ + countClicks?: boolean /** Unix seconds. */ windowStart: number /** Unix seconds. */ @@ -630,6 +640,12 @@ export function renderWeekly(input: WeeklyRenderInput): WeeklyRender { const trendsOnly = edition === 'weekly' const empty = !input.likedChanges.length && !input.trackedCount && !input.trending.length + // Only the HTML is counted. Plain-text clients show the raw URL and often + // follow a redirect poorly, so a bare skilld.dev link is worth more there. + const track = (url: string, placement: EmailPlacement): string => input.countClicks + ? countedEmailUrl({ siteUrl: input.siteUrl, campaign: edition, issue: input.windowEnd }, url, placement) + : url + const likedRows = input.likedChanges.map((change) => { const subjects = commitSubjects(change.commitMessages) const sourceUrl = change.sourceUrl ?? skillUrl(input.siteUrl, change) @@ -656,7 +672,7 @@ export function renderWeekly(input: WeeklyRenderInput): WeeklyRender { const overflowRow = input.likedOverflow ? ` - +${input.likedOverflow} more ${plural(input.likedOverflow, 'Skill', 'Skills')} you like were updated + +${input.likedOverflow} more ${plural(input.likedOverflow, 'Skill', 'Skills')} you like were updated ` : '' @@ -672,7 +688,7 @@ export function renderWeekly(input: WeeklyRenderInput): WeeklyRender { // skills to go like a skill is the product failing to notice it worked. const likePrompt = !trendsOnly && !input.trackedCount && !input.likedChanges.length && input.trending.length ? ` - ${LIKE_PROMPT} Browse the registry. + ${LIKE_PROMPT} Browse the registry. ` : '' @@ -695,7 +711,7 @@ export function renderWeekly(input: WeeklyRenderInput): WeeklyRender { : empty ? ` Like a few Skills and they will show up here the week they change. - Browse the registry. + Browse the registry. ` : `${input.likedChanges.length || input.trackedCount ? `${sectionLabel(t, 'Skills you like')}${likedRows}${overflowRow}${quietRow}` @@ -703,7 +719,7 @@ export function renderWeekly(input: WeeklyRenderInput): WeeklyRender { ? `${sectionLabel(t, 'Trending this week')}${trendingRows}${likePrompt}` : ''}` - const shareUrl = `${input.siteUrl}/api/share/weekly` + const shareUrl = track(`${input.siteUrl}/api/share/weekly`, 'share') const card = ` @@ -726,7 +742,7 @@ export function renderWeekly(input: WeeklyRenderInput): WeeklyRender { ${body} - ${input.trending.length ? button(t, `${input.siteUrl}/skills/trending`, 'Browse trending Skills') : ''} + ${input.trending.length ? button(t, track(`${input.siteUrl}/skills/trending`, 'cta'), 'Browse trending Skills') : ''} ${input.trending.length ? `` : ''} @@ -735,7 +751,7 @@ export function renderWeekly(input: WeeklyRenderInput): WeeklyRender { ${edition === 'weekly' ? 'You get this once a week because you have a skilld account.' : 'You get this because you enabled the digest.'} - Settings + Settings · Unsubscribe diff --git a/layers/identity/server/utils/weekly-tracking.ts b/layers/identity/server/utils/weekly-tracking.ts deleted file mode 100644 index 621d4150..00000000 --- a/layers/identity/server/utils/weekly-tracking.ts +++ /dev/null @@ -1,45 +0,0 @@ -/** - * Parsing for the links old weekly and digest emails carry. - * - * Delivered emails rewrote every same-origin link through `/api/e/weekly`, - * and those links are still sitting in inboxes. The recording is retired, but - * the endpoint lives on as a redirect, so this module keeps the one job that - * remains: parsing a query into a destination that cannot redirect off-site. - */ - -export type WeeklyPlacement = 'liked' | 'trending' | 'cta' | 'footer' | 'overflow' | 'share' - -export type WeeklyClickTarget - = | { _tag: 'ok', path: string, placement: WeeklyPlacement, windowEnd: number, userId: number | null } - | { _tag: 'invalid', reason: 'missing-path' | 'not-site-relative' | 'bad-placement' } - -const PLACEMENTS = new Set(['liked', 'trending', 'cta', 'footer', 'overflow', 'share']) - -/** - * Parses the query once, at the boundary, into something that cannot redirect - * off-site. Everything downstream trusts it. - */ -export function parseWeeklyClick(query: Record): WeeklyClickTarget { - const path = typeof query.p === 'string' ? query.p : '' - if (!path) - return { _tag: 'invalid', reason: 'missing-path' } - // A single leading slash and nothing that resolves to another origin. The - // backslash forms are rejected because some clients normalise them to '/'. - if (!path.startsWith('/') || path.startsWith('//') || path.startsWith('/\\')) - return { _tag: 'invalid', reason: 'not-site-relative' } - - const placement = typeof query.k === 'string' ? query.k : '' - if (!PLACEMENTS.has(placement)) - return { _tag: 'invalid', reason: 'bad-placement' } - - const windowEnd = Number(query.w) - const userId = Number(query.u) - - return { - _tag: 'ok', - path, - placement: placement as WeeklyPlacement, - windowEnd: Number.isFinite(windowEnd) ? Math.trunc(windowEnd) : 0, - userId: Number.isFinite(userId) && userId > 0 ? Math.trunc(userId) : null, - } -} diff --git a/migrations/0120_email_click_counts.sql b/migrations/0120_email_click_counts.sql new file mode 100644 index 00000000..4c0f5c28 --- /dev/null +++ b/migrations/0120_email_click_counts.sql @@ -0,0 +1,21 @@ +-- Aggregate click counts for the weekly and digest emails. +-- +-- 0118 dropped `weekly_click_events`, which stored one row per click with the +-- reader's id. Loop 2 still needs digest click-throughs as evidence, so this +-- table keeps a daily counter per link instead. Email links carry no reader +-- and no per-send id, and no row here names a person, address, or client. +-- +-- `issue` is the send's `window_end`, shared by every recipient of that send. +-- `path` is site-relative with no query or fragment. +CREATE TABLE email_click_counts ( + day TEXT NOT NULL, + campaign TEXT NOT NULL CHECK (campaign IN ('weekly', 'digest')), + issue INTEGER NOT NULL, + placement TEXT NOT NULL, + path TEXT NOT NULL, + clicks INTEGER NOT NULL DEFAULT 0, + PRIMARY KEY (day, campaign, issue, placement, path) +); + +-- The admin report reads one issue at a time. +CREATE INDEX email_click_counts_issue ON email_click_counts (campaign, issue); diff --git a/schema.sql b/schema.sql index 674e78eb..4cb6b6a6 100644 --- a/schema.sql +++ b/schema.sql @@ -49,6 +49,8 @@ CREATE UNIQUE INDEX weekly_runs_user_window ON weekly_runs (user_id, window_end) CREATE INDEX weekly_skill_sends_recent ON weekly_skill_sends (sent_at DESC, owner, repo, name); +CREATE INDEX email_click_counts_issue ON email_click_counts (campaign, issue); + CREATE INDEX idx_owners_followers ON owners (followers DESC); CREATE INDEX idx_skill_revisions_lookup @@ -277,6 +279,16 @@ CREATE TABLE digest_runs ( error TEXT ); +CREATE TABLE email_click_counts ( + day TEXT NOT NULL, + campaign TEXT NOT NULL CHECK (campaign IN ('weekly', 'digest')), + issue INTEGER NOT NULL, + placement TEXT NOT NULL, + path TEXT NOT NULL, + clicks INTEGER NOT NULL DEFAULT 0, + PRIMARY KEY (day, campaign, issue, placement, path) +); + CREATE TABLE email_preference_events ( id INTEGER PRIMARY KEY AUTOINCREMENT, user_id INTEGER NOT NULL, diff --git a/test/unit/daily-health-check-x-discovery.test.ts b/test/unit/daily-health-check-x-discovery.test.ts index 9d0467b7..a3e76287 100644 --- a/test/unit/daily-health-check-x-discovery.test.ts +++ b/test/unit/daily-health-check-x-discovery.test.ts @@ -16,7 +16,7 @@ function healthy( frontDoor: { checks: [{ url: 'https://skilld.dev/', status: 200 }] }, trendingSkills: { checks: [] }, inventory: { skills: 100, repos: 10, owners: 5, users: 5, collections: 3, watchedRepos: 1, brokenRepos: 0 }, - activity: { newSkills24h: 1, repoChanges24h: 1, newUsers24h: 0, digestsSent24h: 0, digestsFailed24h: 0 }, + activity: { newSkills24h: 1, repoChanges24h: 1, newUsers24h: 0, digestsSent24h: 0, digestsFailed24h: 0, emailClicks: { fromDay: '2026-07-21', weekly: 0, digest: 0 } }, pipeline: { syncJobs: [], scheduledRuns: [], diff --git a/test/unit/email-clicks.test.ts b/test/unit/email-clicks.test.ts new file mode 100644 index 00000000..2860d747 --- /dev/null +++ b/test/unit/email-clicks.test.ts @@ -0,0 +1,251 @@ +import type { H3Event } from 'h3' +import type { WeeklyRenderInput } from '../../layers/identity/server/utils/weekly-template' +import type { SqliteD1 } from './helpers/d1-sqlite' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import weeklyReport from '../../layers/admin/server/api/admin/weekly.get' +import clickHandler from '../../layers/identity/server/api/e/[campaign].get' +import { renderDigest } from '../../layers/identity/server/utils/digest-template' +import { parseEmailClick, parseSiteRelativePath, recordEmailClick } from '../../layers/identity/server/utils/email-clicks' +import { renderWeekly } from '../../layers/identity/server/utils/weekly-template' +import { allMigrations, createSqliteD1 } from './helpers/d1-sqlite' + +vi.hoisted(() => { + vi.stubGlobal('defineEventHandler', (handler: unknown) => handler) +}) + +const ISSUE = 1_787_500_000 + +describe('email click destination', () => { + it.each([ + '/skills/trending', + '/', + '/me/likes?tab=recent#top', + ])('accepts the site path %s', (to) => { + expect(parseSiteRelativePath(to)).toEqual({ _tag: 'ok', path: to }) + }) + + // Each of these reads as "a path" to a naive check and resolves off-site, + // or smuggles a header break into Location. + it.each([ + ['//evil.test/phish', 'not-site-relative'], + ['/\\evil.test', 'not-site-relative'], + ['\\\\evil.test', 'not-site-relative'], + ['/skills\\..\\..\\evil', 'not-site-relative'], + ['/\t/evil.test', 'not-site-relative'], + ['/\n/evil.test', 'not-site-relative'], + ['/skills\r\nLocation: https://evil.test', 'not-site-relative'], + ['https://evil.test/phish', 'not-site-relative'], + ['javascript:alert(1)', 'not-site-relative'], + ['evil.test', 'not-site-relative'], + ['', 'missing-path'], + [undefined, 'missing-path'], + [['/skills', '//evil.test'], 'missing-path'], + [`/${'a'.repeat(600)}`, 'path-too-long'], + ])('rejects %j', (to, reason) => { + expect(parseSiteRelativePath(to)).toEqual({ _tag: 'invalid', reason }) + }) +}) + +describe('parsing an email click', () => { + it('counts a well formed link by its path without the query', () => { + expect(parseEmailClick('digest', { to: '/me/likes?tab=recent', p: 'overflow', i: String(ISSUE) })).toEqual({ + _tag: 'counted', + to: '/me/likes?tab=recent', + key: { campaign: 'digest', issue: ISSUE, placement: 'overflow', path: '/me/likes' }, + }) + }) + + it('refuses an open redirect even when every other field is valid', () => { + expect(parseEmailClick('weekly', { to: '//evil.test', p: 'cta', i: String(ISSUE) })) + .toEqual({ _tag: 'invalid', reason: 'not-site-relative' }) + }) + + it.each([ + ['newsletter', { p: 'cta', i: String(ISSUE) }, 'bad-campaign'], + ['weekly', { p: 'made-up', i: String(ISSUE) }, 'bad-placement'], + ['weekly', { p: 'cta', i: 'nope' }, 'bad-issue'], + ['weekly', { p: 'cta', i: '-5' }, 'bad-issue'], + ])('still lands a safe link it cannot count (%s)', (campaign, query, reason) => { + expect(parseEmailClick(campaign, { to: '/skills', ...query })).toEqual({ _tag: 'uncounted', to: '/skills', reason }) + }) + + it('lands links from sends before counting returned without counting them', () => { + expect(parseEmailClick('weekly', { p: '/skills', k: 'cta', w: '1787000000', u: '2' })) + .toEqual({ _tag: 'uncounted', to: '/skills', reason: 'legacy-link' }) + expect(parseEmailClick('weekly', { p: '//evil.test', k: 'cta', w: '1787000000', u: '2' })) + .toEqual({ _tag: 'invalid', reason: 'not-site-relative' }) + }) +}) + +describe('email click counter', () => { + let d1: SqliteD1 + + beforeEach(() => { + d1 = createSqliteD1(allMigrations()) + }) + + // Globals stay stubbed: the setup file's wide event stubs must survive. + afterEach(() => { + d1.close() + }) + + function rows() { + return d1.raw.prepare('SELECT * FROM email_click_counts ORDER BY day, path').all() + } + + it('adds to one row per day and link, and stores nothing else', async () => { + const key = { campaign: 'digest', issue: ISSUE, placement: 'overflow', path: '/me/likes' } as const + const monday = new Date('2026-09-14T23:59:00Z') + + await recordEmailClick(d1.db, key, monday) + await recordEmailClick(d1.db, key, monday) + await recordEmailClick(d1.db, key, new Date('2026-09-15T00:01:00Z')) + await recordEmailClick(d1.db, { ...key, path: '/' }, monday) + + expect(rows()).toEqual([ + { day: '2026-09-14', campaign: 'digest', issue: ISSUE, placement: 'overflow', path: '/', clicks: 1 }, + { day: '2026-09-14', campaign: 'digest', issue: ISSUE, placement: 'overflow', path: '/me/likes', clicks: 2 }, + { day: '2026-09-15', campaign: 'digest', issue: ISSUE, placement: 'overflow', path: '/me/likes', clicks: 1 }, + ]) + }) + + async function serve(campaign: string, query: Record) { + const headers = new Map() + const event = { + method: 'GET', + path: `/api/e/${campaign}?${new URLSearchParams(query)}`, + context: { platform: { db: d1.db }, params: { campaign } }, + node: { + req: { headers: {}, url: `/api/e/${campaign}?${new URLSearchParams(query)}` }, + res: { + statusCode: 200, + setHeader: (name: string, value: string) => headers.set(name.toLowerCase(), value), + getHeader: (name: string) => headers.get(name.toLowerCase()), + end: () => {}, + }, + }, + handled: false, + } as unknown as H3Event + vi.stubGlobal('getUserSession', () => Promise.resolve(null)) + await clickHandler(event) + return { + status: (event.node.res as { statusCode: number }).statusCode, + location: headers.get('location') ?? '', + cacheControl: headers.get('cache-control') ?? '', + } + } + + it('redirects a counted link to its path and counts it for the admin report', async () => { + d1.raw.exec(` + DELETE FROM users; + INSERT INTO users (id, github_id, login, email, created_at, last_login_at) VALUES (1, 1, 'reader', 'r@example.com', 1, 1); + INSERT INTO weekly_runs (user_id, window_start, window_end, status, provider_status, claimed_at, sent_at) + VALUES (1, ${ISSUE - 604_800}, ${ISSUE}, 'sent', 'accepted', ${ISSUE}, ${ISSUE}); + `) + vi.stubGlobal('requireAdmin', async () => ({ email: 'admin@example.com' })) + + const redirect = await serve('weekly', { to: '/skills/trending', p: 'cta', i: String(ISSUE) }) + await serve('weekly', { to: '/skills/trending', p: 'cta', i: String(ISSUE) }) + await serve('weekly', { to: '/api/share/weekly', p: 'share', i: String(ISSUE) }) + + expect(redirect).toMatchObject({ status: 302, location: '/skills/trending', cacheControl: 'private, no-store' }) + const report = await weeklyReport({ context: { platform: { db: d1.db } }, node: { req: { headers: {} } } } as unknown as H3Event) + expect(report.windows[0]).toMatchObject({ windowEnd: ISSUE, accepted: 1, clicks: 2, clicksPerAccepted: 2, shareClicks: 1 }) + }) + + it('lands an open redirect attempt on home and counts nothing', async () => { + const redirect = await serve('weekly', { to: '//evil.test/phish', p: 'cta', i: String(ISSUE) }) + + expect(redirect).toMatchObject({ status: 302, location: '/' }) + expect(rows()).toEqual([]) + }) +}) + +describe('counted email links', () => { + function hrefs(html: string): string[] { + return [...html.matchAll(/href="([^"]+)"/g)].map(match => match[1]!.replaceAll('&', '&')) + } + + function counted(html: string): URL[] { + return hrefs(html).map(href => new URL(href)).filter(url => url.pathname.startsWith('/api/e/')) + } + + function weekly(overrides: Partial): WeeklyRenderInput { + return { + edition: 'weekly', + windowStart: ISSUE - 604_800, + windowEnd: ISSUE, + likedChanges: [], + likedOverflow: 0, + trackedCount: 0, + trending: [{ + owner: 'antfu', + repo: 'skills', + slug: 'vitest', + canonicalName: 'vitest', + description: null, + stars: 1, + sourceUrl: 'https://github.com/antfu/skills/blob/sha/SKILL.md', + reason: { _tag: 'named', authorCount: 2, mentionCount: 3, latestAt: ISSUE }, + evidence: null, + }], + siteUrl: 'https://skilld.dev', + unsubscribeUrl: 'https://skilld.dev/api/unsubscribe?t=token-a&list=weekly', + settingsUrl: 'https://skilld.dev/me', + countClicks: true, + ...overrides, + } + } + + it('gives two recipients of one weekly issue identical counted links naming no reader', () => { + const a = renderWeekly(weekly({ recipientName: 'Ada', login: 'ada' })) + const b = renderWeekly(weekly({ recipientName: 'Bo', login: 'bo', unsubscribeUrl: 'https://skilld.dev/api/unsubscribe?t=token-b&list=weekly' })) + + const links = counted(a.html) + expect(links.map(String)).toEqual(counted(b.html).map(String)) + expect(links.map(url => url.pathname)).toContain('/api/e/weekly') + for (const url of links) + expect([...url.searchParams.keys()].sort()).toEqual(['i', 'p', 'to']) + expect(links.map(url => parseEmailClick('weekly', Object.fromEntries(url.searchParams))._tag)).not.toContain('invalid') + }) + + it('counts digest links under the digest campaign with no reader in them', () => { + const digest = (login: string) => renderDigest({ + login, + recipientName: login, + countClicks: true, + windowStart: ISSUE - 2_592_000, + windowEnd: ISSUE, + siteUrl: 'https://skilld.dev', + unsubscribeUrl: `https://skilld.dev/api/unsubscribe?t=${login}&list=digest`, + entries: [{ + owner: 'antfu', + repo: 'skills', + skillNames: ['a', 'b', 'c', 'd', 'e', 'f'], + changeCount: 6, + skills: ['a', 'b', 'c', 'd', 'e', 'f'].map(name => ({ + name, + description: null, + changeCount: 1, + commitMessages: ['fix'], + changedAt: ISSUE - 60, + sourceUrl: `https://github.com/antfu/skills/blob/sha/${name}/SKILL.md`, + changeUrl: 'https://github.com/antfu/skills/compare/a...b', + })), + }], + }) + + const links = counted(digest('ada').html) + expect(links.length).toBeGreaterThan(0) + expect(links.map(String)).toEqual(counted(digest('bo').html).map(String)) + expect(new Set(links.map(url => url.pathname))).toEqual(new Set(['/api/e/digest'])) + expect(links.map(url => url.searchParams.get('to'))).toContain('/me/likes') + }) + + it('leaves previews uncounted', () => { + const preview = renderWeekly(weekly({ countClicks: undefined })) + + expect(counted(preview.html)).toEqual([]) + expect(hrefs(preview.html)).toContain('https://skilld.dev/skills/trending') + }) +}) diff --git a/test/unit/weekly-admin-report.test.ts b/test/unit/weekly-admin-report.test.ts index ed66d5c7..22b137db 100644 --- a/test/unit/weekly-admin-report.test.ts +++ b/test/unit/weekly-admin-report.test.ts @@ -11,21 +11,29 @@ vi.hoisted(() => { }) describe('weekly admin engagement', () => { - it('reports the unsubscribe rate per accepted recipient', () => { + it('reports clicks, unsubscribes, and share clicks per accepted email', () => { expect(summarizeWeeklyEngagement({ accepted: 12, + clicks: 3, unsubscribes: 1, + share_clicks: 2, })).toEqual({ + clicks: 3, + clicksPerAccepted: 0.25, unsubscribes: 1, unsubscribeRate: 1 / 12, + shareClicks: 2, }) }) it('does not invent rates when nobody was accepted', () => { expect(summarizeWeeklyEngagement({ accepted: 0, + clicks: 4, unsubscribes: 0, + share_clicks: 0, })).toMatchObject({ + clicksPerAccepted: null, unsubscribeRate: null, }) }) diff --git a/test/unit/weekly-click-redirect.test.ts b/test/unit/weekly-click-redirect.test.ts deleted file mode 100644 index cc5c75fd..00000000 --- a/test/unit/weekly-click-redirect.test.ts +++ /dev/null @@ -1,60 +0,0 @@ -import type { H3Event } from 'h3' -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' - -describe('gET /api/e/weekly', () => { - beforeEach(() => { - vi.resetModules() - vi.stubGlobal('defineEventHandler', (handler: unknown) => handler) - vi.stubGlobal('getUserSession', () => Promise.resolve(null)) - }) - - afterEach(() => { - vi.unstubAllGlobals() - }) - - it('302s a delivered tracked link to its site-relative path and records nothing', async () => { - const prepare = vi.fn() - const redirect = await serve(prepare, { p: '/skills', k: 'cta', w: '1787000000' }) - - expect(redirect.status).toBe(302) - expect(redirect.location).toBe('/skills') - expect(redirect.cacheControl).toBe('private, no-store') - expect(prepare).not.toHaveBeenCalled() - }) - - it('lands a protocol-relative target on home instead of another origin', async () => { - const redirect = await serve(vi.fn(), { p: '//evil.test/skills', k: 'cta', w: '1787000000' }) - - expect(redirect.status).toBe(302) - expect(redirect.location).toBe('/') - }) - - async function serve(prepare: ReturnType, query: Record) { - const headers = new Map() - let redirected: { location: string, status: number } | undefined - - vi.stubGlobal('getQuery', () => query) - vi.stubGlobal('setHeader', (_event: unknown, name: string, value: string) => { - headers.set(name, value) - }) - vi.stubGlobal('sendRedirect', (_event: unknown, location: string, status: number) => { - redirected = { location, status } - return Promise.resolve(undefined) - }) - - const { default: handler } = await import('../../layers/identity/server/api/e/weekly.get') - const event = { - method: 'GET', - context: { platform: { db: { prepare } } }, - node: { req: { headers: {} } }, - } as unknown as H3Event - - await handler(event) - - return { - status: redirected?.status ?? 0, - location: redirected?.location ?? '', - cacheControl: headers.get('cache-control') ?? '', - } - } -}) diff --git a/test/unit/weekly-email-improvements.test.ts b/test/unit/weekly-email-improvements.test.ts index d85f3b1b..bc7397a3 100644 --- a/test/unit/weekly-email-improvements.test.ts +++ b/test/unit/weekly-email-improvements.test.ts @@ -85,12 +85,4 @@ describe('weekly email improvement contract', () => { expect(rendered.html).not.toContain('🔥') expect(rendered.html).toContain('Hi Harlan,') }) - - it('links straight to pages with no per-reader click tracking', () => { - const rendered = renderWeekly({ ...input(), edition: 'weekly' }) - const hrefs = [...rendered.html.matchAll(/href="([^"]+)"/g)].map(match => match[1]!) - expect(hrefs).toContain('https://skilld.dev/api/share/weekly') - expect(hrefs).toContain('https://skilld.dev/skills/trending') - expect(hrefs.filter(href => href.includes('/api/e/weekly'))).toEqual([]) - }) }) From aa109d7504d8bd1164fcfce48d79ec27f6541740 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Tue, 22 Sep 2026 12:29:06 +1000 Subject: [PATCH 2/2] feat(identity): count the GitHub clicks the digest was missing Liked and trending rows link to SKILL.md on GitHub, which is where a reader judges a change. Those were the only digest clicks left uncounted, and they are the clearest Loop 2 evidence. The link now carries `g`, a path on GitHub, and the route puts the constant `github.com` origin back. No request supplies a host, so the redirect cannot be steered off GitHub. The counter stores `gh:/owner/repo`, so a new commit sha does not start a new row and weeks stay comparable. --- layers/identity/server/utils/email-clicks.ts | 63 ++++++++++++++----- .../identity/server/utils/weekly-template.ts | 9 ++- test/unit/email-clicks.test.ts | 21 ++++++- 3 files changed, 71 insertions(+), 22 deletions(-) diff --git a/layers/identity/server/utils/email-clicks.ts b/layers/identity/server/utils/email-clicks.ts index b0d5455d..3f2d344b 100644 --- a/layers/identity/server/utils/email-clicks.ts +++ b/layers/identity/server/utils/email-clicks.ts @@ -11,21 +11,37 @@ * issue gets identical links, so a click cannot be tied to a person, and the * counter stores nothing that could. * - * The destination is a site-relative path, never a URL. An off-site redirect - * is not expressible, because a tracking redirect that accepts a URL is an open - * redirect, and an open redirect reached from an email is a phishing primitive. + * The destination is never a URL. A same-site link carries a site-relative + * path. A GitHub link carries only its path, and the route puts the + * `github.com` origin back. A tracking redirect that accepts a URL is an open + * redirect, and an open redirect reached from an email is a phishing + * primitive, so neither form can express one. */ export type EmailCampaign = 'weekly' | 'digest' export type EmailPlacement = 'liked' | 'trending' | 'cta' | 'footer' | 'overflow' | 'share' +/** + * The only off-site host an email link may reach. + * + * Trending and liked rows point at SKILL.md on GitHub, which is where a reader + * judges a change. Those clicks are the clearest Loop 2 evidence, so they are + * counted rather than left dark. The origin is a constant here, so no request + * can steer the redirect elsewhere. + */ +export const GITHUB_ORIGIN = 'https://github.com' + export interface EmailClickKey { campaign: EmailCampaign /** The send's `window_end`, in Unix seconds. Shared by every recipient. */ issue: number placement: EmailPlacement - /** The destination path with any query or fragment removed. */ + /** + * What was clicked. A same-site link stores its path with any query or + * fragment removed. A GitHub link stores `gh:/owner/repo`, so a new commit + * sha does not start a new row and weeks stay comparable. + */ path: string } @@ -84,23 +100,33 @@ function countedPath(to: string): string { return end === -1 ? to : to.slice(0, end) } +/** `gh:/owner/repo`, which is the counter identity of every GitHub link. */ +export function githubCountedPath(path: string): string { + const [owner, repo] = path.split('/').filter(Boolean) + return `gh:/${owner ?? ''}${repo ? `/${repo}` : ''}` +} + /** * Parses one click request at the boundary. * - * `campaign` is the route segment. The query carries `to` (path), `p` - * (placement), and `i` (issue). Links sent before counting was restored carry - * `p` as the path and `k` as the placement; they still redirect, uncounted. + * `campaign` is the route segment. The query carries `p` (placement), `i` + * (issue), and one destination: `to` for a path on this site, or `g` for a + * path on GitHub. Links sent before counting was restored carry `p` as the + * path and `k` as the placement; they still redirect, uncounted. */ export function parseEmailClick(campaign: unknown, query: Record): EmailClick { - if (query.to === undefined && query.k !== undefined) { + if (query.to === undefined && query.g === undefined && query.k !== undefined) { const legacy = parseSiteRelativePath(query.p) return legacy._tag === 'ok' ? { _tag: 'uncounted', to: legacy.path, reason: 'legacy-link' } : legacy } - const target = parseSiteRelativePath(query.to) + // A GitHub link is validated with the same parser, then the constant origin + // is put back. The request never supplies a host. + const offSite = query.g !== undefined + const target = parseSiteRelativePath(offSite ? query.g : query.to) if (target._tag === 'invalid') return target - const to = target.path + const to = offSite ? `${GITHUB_ORIGIN}${target.path}` : target.path if (typeof campaign !== 'string' || !CAMPAIGNS.has(campaign)) return { _tag: 'uncounted', to, reason: 'bad-campaign' } @@ -117,7 +143,7 @@ export function parseEmailClick(campaign: unknown, query: Record { const subjects = commitSubjects(change.commitMessages) - const sourceUrl = change.sourceUrl ?? skillUrl(input.siteUrl, change) + // Liked rows point at SKILL.md on GitHub, which is where a reader judges a + // change. Those clicks are counted too: the destination path is rewritten, + // never a whole URL. + const sourceUrl = track(change.sourceUrl ?? skillUrl(input.siteUrl, change), 'liked') return row(t, { siteUrl: input.siteUrl, owner: change.owner, @@ -664,7 +667,7 @@ export function renderWeekly(input: WeeklyRenderInput): WeeklyRender { relativeDay(change.changedAt, now), ]), links: rowLinks(t, [ - ...(change.changeUrl ? [{ href: change.changeUrl, label: 'View change' }] : []), + ...(change.changeUrl ? [{ href: track(change.changeUrl, 'liked'), label: 'View change' }] : []), { href: sourceUrl, label: 'Open SKILL.md' }, ]), }) @@ -693,7 +696,7 @@ export function renderWeekly(input: WeeklyRenderInput): WeeklyRender { : '' const trendingRows = input.trending.map((skill) => { - const sourceUrl = skill.sourceUrl ?? skillUrl(input.siteUrl, { ...skill, name: skill.slug }) + const sourceUrl = track(skill.sourceUrl ?? skillUrl(input.siteUrl, { ...skill, name: skill.slug }), 'trending') return trendingRow(t, { siteUrl: input.siteUrl, owner: skill.owner, diff --git a/test/unit/email-clicks.test.ts b/test/unit/email-clicks.test.ts index 2860d747..394021d5 100644 --- a/test/unit/email-clicks.test.ts +++ b/test/unit/email-clicks.test.ts @@ -55,6 +55,19 @@ describe('parsing an email click', () => { }) }) + it('counts a GitHub link by owner and repository, not by commit sha', () => { + expect(parseEmailClick('digest', { g: '/antfu/skills/blob/abc123/vite/SKILL.md', p: 'liked', i: String(ISSUE) })).toEqual({ + _tag: 'counted', + to: 'https://github.com/antfu/skills/blob/abc123/vite/SKILL.md', + key: { campaign: 'digest', issue: ISSUE, placement: 'liked', path: 'gh:/antfu/skills' }, + }) + }) + + it('cannot be steered off GitHub by the destination it is handed', () => { + for (const g of ['//evil.test/phish', 'https://evil.test', '/\\evil.test']) + expect(parseEmailClick('digest', { g, p: 'liked', i: String(ISSUE) })).toEqual({ _tag: 'invalid', reason: 'not-site-relative' }) + }) + it('refuses an open redirect even when every other field is valid', () => { expect(parseEmailClick('weekly', { to: '//evil.test', p: 'cta', i: String(ISSUE) })) .toEqual({ _tag: 'invalid', reason: 'not-site-relative' }) @@ -204,8 +217,12 @@ describe('counted email links', () => { const links = counted(a.html) expect(links.map(String)).toEqual(counted(b.html).map(String)) expect(links.map(url => url.pathname)).toContain('/api/e/weekly') - for (const url of links) - expect([...url.searchParams.keys()].sort()).toEqual(['i', 'p', 'to']) + // A counted link carries the issue, the placement, and one destination: + // `to` for a page here, `g` for a path on GitHub. Nothing else. + for (const url of links) { + expect([...url.searchParams.keys()].sort()).toSatisfy((keys: string[]) => + keys.join() === 'i,p,to' || keys.join() === 'g,i,p') + } expect(links.map(url => parseEmailClick('weekly', Object.fromEntries(url.searchParams))._tag)).not.toContain('invalid') })