diff --git a/src/index.ts b/src/index.ts index 5cee4a5..07d7b50 100644 --- a/src/index.ts +++ b/src/index.ts @@ -78,6 +78,7 @@ export * from './lib/client-ip.js'; export * from './lib/database.js'; export * from './lib/fingerprint.js'; export * from './lib/webhook.js'; +export * from './lib/link-safety.js'; export * from './lib/event-emitter.js'; export * from './types/index.js'; export { redirectRoutes, linkRoutes, analyticsRoutes, sdkRoutes, webhookRoutes, templateRoutes, qrRoutes, previewRoutes, debugRoutes, wellKnownRoutes, healthRoutes } from './routes/index.js'; diff --git a/src/lib/database.ts b/src/lib/database.ts index c35c27d..d5b44db 100644 --- a/src/lib/database.ts +++ b/src/lib/database.ts @@ -244,6 +244,45 @@ export async function initializeDatabase(options: DatabaseOptions = {}) { END $$; `); + // Link safety states. + // + // `is_active = false` already removes a link from resolution entirely. These + // add a softer state and an explanation: + // + // warn_at — resolve to an interstitial warning instead of a 302, so a + // visitor sees the destination and chooses. Useful when a + // link is suspected rather than confirmed unsafe, since a + // hard block on a false positive takes a legitimate link + // offline with no recourse for the visitor. + // disabled_at — when the link was taken out of resolution. + // disabled_reason — why, so the decision can be explained or reversed later. + // + // All nullable and additive: an existing deployment that sets none of them + // behaves exactly as before. + await client.query(` + DO $$ + BEGIN + IF NOT EXISTS ( + SELECT 1 FROM information_schema.columns + WHERE table_name='links' AND column_name='warn_at' + ) THEN + ALTER TABLE links ADD COLUMN warn_at TIMESTAMPTZ; + END IF; + IF NOT EXISTS ( + SELECT 1 FROM information_schema.columns + WHERE table_name='links' AND column_name='disabled_at' + ) THEN + ALTER TABLE links ADD COLUMN disabled_at TIMESTAMPTZ; + END IF; + IF NOT EXISTS ( + SELECT 1 FROM information_schema.columns + WHERE table_name='links' AND column_name='disabled_reason' + ) THEN + ALTER TABLE links ADD COLUMN disabled_reason TEXT; + END IF; + END $$; + `); + // Add description column to existing links table if it doesn't exist await client.query(` DO $$ diff --git a/src/lib/link-safety.test.ts b/src/lib/link-safety.test.ts new file mode 100644 index 0000000..5cc52e0 --- /dev/null +++ b/src/lib/link-safety.test.ts @@ -0,0 +1,144 @@ +import { describe, it, expect } from 'vitest'; +import { evaluateLinkSafety, generateWarningLinkHTML, escapeHtml, safeHref } from './link-safety.js'; + +describe('evaluateLinkSafety', () => { + it('allows a plain healthy link', () => { + expect(evaluateLinkSafety({ isActive: true })).toBe('allow'); + expect(evaluateLinkSafety({})).toBe('allow'); + }); + + it('treats an absent isActive as active, so existing rows are unaffected', () => { + expect(evaluateLinkSafety({ isActive: undefined })).toBe('allow'); + expect(evaluateLinkSafety({ isActive: null })).toBe('allow'); + }); + + it('blocks an inactive link', () => { + expect(evaluateLinkSafety({ isActive: false })).toBe('block'); + }); + + it('warns when warn_at is set', () => { + expect(evaluateLinkSafety({ isActive: true, warnAt: new Date() })).toBe('warn'); + expect(evaluateLinkSafety({ isActive: true, warnAt: '2026-08-10T00:00:00Z' })).toBe('warn'); + }); + + it('blocks when the owner is restricted, even if the link itself is fine', () => { + expect(evaluateLinkSafety({ isActive: true, ownerSuspendedAt: new Date() })).toBe('block'); + }); + + it('prefers block over warn — a restricted owner outranks a mere warning', () => { + expect( + evaluateLinkSafety({ isActive: true, warnAt: new Date(), ownerSuspendedAt: new Date() }) + ).toBe('block'); + }); + + it('ignores owner restriction when it is not modelled at all', () => { + // Deployments without an owner table never pass the field. + expect(evaluateLinkSafety({ isActive: true, ownerSuspendedAt: null })).toBe('allow'); + expect(evaluateLinkSafety({ isActive: true, ownerSuspendedAt: undefined })).toBe('allow'); + }); +}); + +describe('escapeHtml', () => { + it('neutralises markup and quote characters', () => { + expect(escapeHtml('')).toBe( + '<script>alert("x")</script>' + ); + expect(escapeHtml("it's & more")).toBe('it's & more'); + }); +}); + +describe('generateWarningLinkHTML', () => { + it('shows the destination so the visitor can judge it', () => { + const html = generateWarningLinkHTML('https://example.com/login'); + expect(html).toContain('https://example.com/login'); + expect(html).toContain('Check this link before continuing'); + }); + + it('escapes a hostile destination rather than injecting it', () => { + const html = generateWarningLinkHTML('https://evil.test/">'); + expect(html).not.toContain(''); + expect(html).toContain('<script>'); + }); + + it('asks search engines not to index it', () => { + expect(generateWarningLinkHTML('https://example.com')).toContain('noindex'); + }); + + it('carries no JavaScript of its own, so it survives a strict CSP', () => { + const html = generateWarningLinkHTML('https://example.com'); + expect(html).not.toContain(' { + it('never emits a javascript: href', () => { + const html = generateWarningLinkHTML('javascript:alert(document.domain)'); + expect(html).not.toMatch(/href="javascript:/i); + }); + + it('never emits a data: href', () => { + const html = generateWarningLinkHTML('data:text/html,'); + expect(html).not.toMatch(/href="data:/i); + }); + + it('offers no continue button when the scheme is not http(s)', () => { + const html = generateWarningLinkHTML('javascript:alert(1)'); + expect(html).not.toContain('class="go"'); + expect(html).toContain('nothing to continue to'); + }); + + it('still shows the destination as inert text so the visitor can see it', () => { + const html = generateWarningLinkHTML('javascript:alert(1)'); + expect(html).toContain('javascript:alert(1)'); + }); + + it('emits no empty href for a bare path or an absent destination', () => { + for (const d of ['/deep/link/path', '']) { + const html = generateWarningLinkHTML(d); + expect(html).not.toContain('href=""'); + expect(html).not.toContain('class="go"'); + } + }); + + it('still links a legitimate http(s) destination', () => { + const html = generateWarningLinkHTML('https://example.com/x'); + expect(html).toContain('class="go"'); + expect(html).toContain('href="https://example.com/x"'); + }); + }); + + it('marks the outbound link nofollow/noopener so we pass no reputation to it', () => { + const html = generateWarningLinkHTML('https://example.com'); + expect(html).toContain('rel="nofollow noopener noreferrer"'); + }); + + it('includes a report link only when one is configured', () => { + expect(generateWarningLinkHTML('https://example.com')).not.toContain('Report this link'); + const withReport = generateWarningLinkHTML('https://example.com', { + reportUrl: 'https://example.org/abuse', + }); + expect(withReport).toContain('Report this link'); + expect(withReport).toContain('https://example.org/abuse'); + }); +}); + + +describe('safeHref', () => { + it('allows http and https', () => { + expect(safeHref('http://example.com')).toBe('http://example.com'); + expect(safeHref('https://example.com/a?b=c')).toBe('https://example.com/a?b=c'); + }); + + it('rejects every other scheme', () => { + for (const d of ['javascript:alert(1)', 'data:text/html,x', 'file:///etc/passwd', 'vbscript:x']) { + expect(safeHref(d), d).toBeNull(); + } + }); + + it('rejects a relative path, which would point at the redirect host', () => { + expect(safeHref('/deep/link/path')).toBeNull(); + expect(safeHref('')).toBeNull(); + }); +}); diff --git a/src/lib/link-safety.ts b/src/lib/link-safety.ts new file mode 100644 index 0000000..c670643 --- /dev/null +++ b/src/lib/link-safety.ts @@ -0,0 +1,197 @@ +/** + * Link safety states for the redirect path. + * + * Three outcomes are possible for a link that exists: + * + * - `allow` — resolve normally. + * - `warn` — serve an interstitial that shows the destination and requires an + * explicit click. For a link that is *suspected* unsafe rather than + * confirmed, this is strictly better than a hard block: a false + * positive still lets the visitor through, while a true positive + * still breaks the one-click flow a malicious link depends on. + * - `block` — behave as if the link does not exist. + * + * `block` deliberately produces the same response as an unknown short code. A + * distinct "this link was disabled" response would confirm to whoever is probing + * that the code was real, and would leak that its owner is under a restriction. + */ +export type LinkSafetyOutcome = 'allow' | 'warn' | 'block'; + +export interface LinkSafetyInput { + /** Whether the link is active. Absent/undefined is treated as active. */ + isActive?: boolean | null; + /** Set when the link should serve a warning instead of resolving. */ + warnAt?: Date | string | null; + /** + * Set when the link's owner is restricted. Optional because the owning table is + * not part of this package's schema — deployments that do not model owner + * restriction simply never pass it. + */ + ownerSuspendedAt?: Date | string | null; +} + +/** + * Decide what to do with a link that was found. + * + * Order matters: owner restriction and inactivity both beat `warn`. A link whose + * owner is restricted must be unreachable even if it was only flagged to warn. + */ +export function evaluateLinkSafety(input: LinkSafetyInput): LinkSafetyOutcome { + if (input.ownerSuspendedAt != null) return 'block'; + if (input.isActive === false) return 'block'; + if (input.warnAt != null) return 'warn'; + return 'allow'; +} + +/** + * A destination is only safe to put in an `href` if it is http(s). + * + * `escapeHtml` is not sufficient on its own: a URL scheme contains none of the + * characters it neutralises, so `javascript:alert(1)` passes through untouched and + * becomes executable on click. That matters here more than almost anywhere else — + * this page is shown *only* for links already flagged as suspicious, so the + * destinations reaching it are the most likely in the system to be hostile. + * + * Reachable in practice, not just in theory: zod's `.url()` accepts `javascript:` + * and `data:` URLs, so a stored destination can already hold one. Tightening + * validation at write time is worth doing separately, but this page must not + * depend on that having happened. + * + * Returns null when there is nothing safe to link to — including a bare deep-link + * path, which would otherwise render as a relative link to the redirect host + * rather than the real destination. Callers render that as inert text. + */ +export function safeHref(destination: string): string | null { + let url: URL; + try { + url = new URL(destination); + } catch { + return null; + } + return url.protocol === 'http:' || url.protocol === 'https:' ? destination : null; +} + +/** Minimal HTML escaping for interpolating a URL into markup and an href. */ +export function escapeHtml(value: string): string { + return String(value) + .replace(/&/g, '&') + .replace(//g, '>') + .replace(/"/g, '"') + .replace(/'/g, '''); +} + +/** + * Interstitial shown for a `warn` outcome. + * + * Shows the destination in full — the point is to hand back the information the + * short link hid, so the visitor can judge for themselves. Continuing takes a + * deliberate click, which is what breaks the one-click flow. + * + * Contains no JavaScript and no external assets: it must render on a bare + * redirect host and under a strict content-security policy. + */ +export function generateWarningLinkHTML( + destination: string, + options: { reportUrl?: string } = {} +): string { + const shown = destination && destination.trim() ? escapeHtml(destination) : '(no destination recorded)'; + const href = destination ? safeHref(destination) : null; + // No anchor at all when there is nothing safe to link to. An href="" would + // re-request the warning page, so "Continue anyway" would just reload it. + const continueButton = href + ? `Continue anyway` + : `

This link has no usable web destination, so there is nothing to continue to.

`; + const reportLink = options.reportUrl + ? `

Report this link

` + : ''; + + return ` + + + + + +Check this link before continuing + + + +
+

Check this link before continuing

+

This short link has been flagged as possibly unsafe, so we have not sent you + straight there. It leads to:

+ ${shown} +

If you were not expecting this link, or it claims to be from a bank, a government + service, or a company you do business with, close this page. Do not enter any + password or personal details.

+ ${continueButton} + ${reportLink} +
+ +`; +} + +/** + * Build a memoised probe for owner-restriction support. + * + * A factory rather than module state: each registration gets its own, so two servers + * in one process pointed at different databases cannot share an answer, and no + * test-only reset has to be exported. + * + * Shared by every path that resolves a link and caches the result, and that sharing + * is the whole point. The redirect and the SDK resolve endpoint write the SAME Redis + * key, so if one selects `owner_suspended_at` and the other does not, the second + * silently caches a row that makes the first one's gate pass. Keeping the SELECT + * fragment in one place is what stops them drifting apart again. + * + * The in-flight promise is memoised, not just the result, so concurrent cold requests + * issue one probe. A failure resolves to "unsupported", so a probe error can never + * take a resolution path down. + */ +export function createOwnerSuspensionSelect(deps: { + query: (sql: string) => Promise<{ rows: unknown[] }>; + onSupported?: () => void; +}): () => Promise { + let probe: Promise | null = null; + return () => { + if (!probe) { + probe = deps + .query( + `SELECT 1 FROM information_schema.columns + WHERE table_name = 'organizations' AND column_name = 'suspended_at'` + ) + .then((r) => { + const supported = r.rows.length > 0; + if (supported) deps.onSupported?.(); + return supported ? ', o.suspended_at AS owner_suspended_at' : ''; + }) + .catch(() => ''); + } + return probe; + }; +} diff --git a/src/routes/redirect.safety.test.ts b/src/routes/redirect.safety.test.ts new file mode 100644 index 0000000..b7f96d5 --- /dev/null +++ b/src/routes/redirect.safety.test.ts @@ -0,0 +1,205 @@ +/** + * Route-level tests for the link safety gate. + * + * These drive the real `redirectRoutes` plugin through fastify.inject(), with only + * the data layer mocked. The unit tests in lib/link-safety.test.ts already cover + * the decision table; what matters here is the behaviour a visitor actually gets: + * status codes, headers, whether a 302 happens, and whether a click is recorded. + */ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import Fastify, { type FastifyInstance } from 'fastify'; +// vi.mock is hoisted above these imports, so redirect.js still receives the mock. +import { redirectRoutes } from './redirect.js'; + +const query = vi.fn(); +vi.mock('../lib/database.js', () => ({ + db: { + query: (...args: unknown[]) => query(...args), + }, +})); + +/** A resolved link row as the redirect query would return it. */ +function linkRow(overrides: Record = {}) { + return { + id: '00000000-0000-0000-0000-0000000000aa', + short_code: 'abc123', + original_url: 'https://example.com/landing', + web_fallback_url: null, + deep_link_path: null, + is_active: true, + warn_at: null, + owner_suspended_at: null, + expires_at: null, + targeting_rules: null, + template_settings: null, + org_settings: null, + utm_parameters: null, + append_click_id: false, + ...overrides, + }; +} + +/** + * @param row the link the lookup resolves to, or null for "not found" + * @param suspensionColumn whether organizations.suspended_at exists in this database + */ +function mockDb(row: Record | null, suspensionColumn = true) { + query.mockReset(); + query.mockImplementation(async (sql: string) => { + if (/information_schema\.columns/i.test(sql)) { + return { rows: suspensionColumn ? [{ '?column?': 1 }] : [], rowCount: suspensionColumn ? 1 : 0 }; + } + if (/^\s*SELECT\s+l\.\*/i.test(sql) || /FROM links l/i.test(sql)) { + return { rows: row ? [row] : [], rowCount: row ? 1 : 0 }; + } + // click inserts, anything else + return { rows: [], rowCount: 0 }; + }); +} + +/** Did any click get written? Flushes the setImmediate the redirect uses. */ +async function clickWasRecorded(): Promise { + await new Promise((r) => setImmediate(r)); + return query.mock.calls.some(([sql]) => /INSERT INTO click_events/i.test(String(sql))); +} + +let app: FastifyInstance; + +beforeEach(async () => { + // No probe reset needed: the probe is scoped to each registration, so a fresh + // Fastify instance per test gets a fresh probe. That the reset export is gone is + // the point — it existed only to work around module-global state. + app = Fastify(); + await app.register(redirectRoutes, { abuseReportUrl: 'https://example.org/abuse' }); + await app.ready(); +}); + +afterEach(async () => { + // Click recording is fire-and-forget (setImmediate). Let any pending insert from + // this test finish BEFORE the next test resets the mock, otherwise a stray click + // from a previous redirect lands in the next test's call log and looks like a bug. + await new Promise((r) => setImmediate(r)); + await app.close(); +}); + +describe('redirect safety gate', () => { + it('redirects a healthy link', async () => { + mockDb(linkRow()); + const res = await app.inject({ method: 'GET', url: '/abc123' }); + expect(res.statusCode).toBe(302); + expect(res.headers.location).toBe('https://example.com/landing'); + }); + + it('404s an inactive link', async () => { + mockDb(linkRow({ is_active: false })); + const res = await app.inject({ method: 'GET', url: '/abc123' }); + expect(res.statusCode).toBe(404); + }); + + it('404s when the owner is restricted, even though the link itself is fine', async () => { + mockDb(linkRow({ owner_suspended_at: '2026-08-10T00:00:00Z' })); + const res = await app.inject({ method: 'GET', url: '/abc123' }); + expect(res.statusCode).toBe(404); + }); + + it('gives a restricted owner the SAME response as an unknown code, leaking nothing', async () => { + mockDb(linkRow({ owner_suspended_at: '2026-08-10T00:00:00Z' })); + const restricted = await app.inject({ method: 'GET', url: '/abc123' }); + mockDb(null); + const unknown = await app.inject({ method: 'GET', url: '/nosuchcode' }); + expect(restricted.statusCode).toBe(unknown.statusCode); + expect(restricted.body).toBe(unknown.body); + }); + + describe('a link flagged to warn', () => { + it('serves an interstitial instead of redirecting', async () => { + mockDb(linkRow({ warn_at: '2026-08-10T00:00:00Z' })); + const res = await app.inject({ method: 'GET', url: '/abc123' }); + expect(res.statusCode).toBe(200); + expect(res.statusCode).not.toBe(302); + expect(res.headers['content-type']).toMatch(/text\/html/); + expect(res.body).toContain('Check this link before continuing'); + }); + + it('shows the destination the short link was hiding', async () => { + mockDb(linkRow({ warn_at: '2026-08-10T00:00:00Z' })); + const res = await app.inject({ method: 'GET', url: '/abc123' }); + expect(res.body).toContain('https://example.com/landing'); + }); + + it('asks not to be indexed or cached', async () => { + mockDb(linkRow({ warn_at: '2026-08-10T00:00:00Z' })); + const res = await app.inject({ method: 'GET', url: '/abc123' }); + expect(res.headers['x-robots-tag']).toMatch(/noindex/); + expect(res.headers['cache-control']).toMatch(/no-store/); + }); + + it('links to the configured reporting page', async () => { + mockDb(linkRow({ warn_at: '2026-08-10T00:00:00Z' })); + const res = await app.inject({ method: 'GET', url: '/abc123' }); + expect(res.body).toContain('https://example.org/abuse'); + }); + + it('records NO click — a warning view is not a click on the link', async () => { + mockDb(linkRow({ warn_at: '2026-08-10T00:00:00Z' })); + await app.inject({ method: 'GET', url: '/abc123' }); + expect(await clickWasRecorded()).toBe(false); + }); + + it('falls back to the web fallback url when there is no original url', async () => { + mockDb(linkRow({ warn_at: '2026-08-10T00:00:00Z', original_url: '', web_fallback_url: 'https://example.net/x' })); + const res = await app.inject({ method: 'GET', url: '/abc123' }); + expect(res.body).toContain('https://example.net/x'); + }); + + it('is outranked by owner restriction', async () => { + mockDb(linkRow({ warn_at: '2026-08-10T00:00:00Z', owner_suspended_at: '2026-08-10T00:00:00Z' })); + const res = await app.inject({ method: 'GET', url: '/abc123' }); + expect(res.statusCode).toBe(404); + }); + }); + + describe('a database without organizations.suspended_at', () => { + it('still resolves links normally rather than erroring', async () => { + mockDb(linkRow(), /* suspensionColumn */ false); + const res = await app.inject({ method: 'GET', url: '/abc123' }); + expect(res.statusCode).toBe(302); + }); + + it('never asks for the column it just proved absent', async () => { + mockDb(linkRow(), false); + await app.inject({ method: 'GET', url: '/abc123' }); + const lookups = query.mock.calls + .map(([sql]) => String(sql)) + .filter((sql) => /FROM links l/i.test(sql)); + expect(lookups.length).toBeGreaterThan(0); + for (const sql of lookups) expect(sql).not.toMatch(/owner_suspended_at/); + }); + + it('probes once even under CONCURRENT cold requests', async () => { + // The sequential test below passes trivially because inject() awaits. This one + // fires them together, which is what the memoised promise actually buys. + mockDb(linkRow(), false); + await Promise.all([ + app.inject({ method: 'GET', url: '/abc123' }), + app.inject({ method: 'GET', url: '/abc123' }), + app.inject({ method: 'GET', url: '/abc123' }), + app.inject({ method: 'GET', url: '/abc123' }), + ]); + const probes = query.mock.calls.filter(([sql]) => + /information_schema\.columns/i.test(String(sql)) + ); + expect(probes).toHaveLength(1); + }); + + it('probes only once across sequential requests', async () => { + mockDb(linkRow(), false); + await app.inject({ method: 'GET', url: '/abc123' }); + await app.inject({ method: 'GET', url: '/abc123' }); + const probes = query.mock.calls.filter(([sql]) => + /information_schema\.columns/i.test(String(sql)) + ); + expect(probes).toHaveLength(1); + }); + }); +}); diff --git a/src/routes/redirect.ts b/src/routes/redirect.ts index 5b05632..4cfb3bf 100644 --- a/src/routes/redirect.ts +++ b/src/routes/redirect.ts @@ -6,6 +6,11 @@ import { parseUserAgent, getLocationFromIP, buildRedirectUrl, detectDevice } fro import { storeFingerprintForClick, type FingerprintData } from '../lib/fingerprint.js'; import { emitClickEvent } from '../lib/event-emitter.js'; import { classifyBot, edgeBotSignal } from '../lib/bot-detection.js'; +import { + evaluateLinkSafety, + generateWarningLinkHTML, + createOwnerSuspensionSelect, +} from '../lib/link-safety.js'; /** * Detect iOS in-app browsers where Universal Links don't fire. @@ -136,7 +141,48 @@ function generateInterstitialHTML(schemeUrl: string, fallbackUrl: string, title? `; } -export async function redirectRoutes(fastify: FastifyInstance) { +export interface RedirectRouteOptions { + /** + * Absolute URL of an abuse-reporting page. When set, the interstitial warning + * page links to it. Optional — deployments without one simply omit the link. + */ + abuseReportUrl?: string; +} + +export async function redirectRoutes( + fastify: FastifyInstance, + options: RedirectRouteOptions = {} +) { + /** + * Owner-restriction support is detected rather than assumed. + * + * The redirect query joins `organizations` for settings, and that table is now + * created by this package (#35) — before that fix a stock self-hosted install + * 500'd on every redirect with 42P01, which made this probe's guarantee hollow: + * it guarded the column while the table itself was missing. + * + * The column still needs probing separately, because `suspended_at` is added by + * downstream consumers that model owner restriction rather than by this package. + * Probing once and building the SELECT accordingly avoids failing every redirect + * on a missing column. + * + * Scoped to this registration rather than the module: module-level state would + * be shared by every createServer() in the process, so two servers pointed at + * different databases would share whichever answer landed first — and it forced + * a test-only reset export onto the package's public API. + * + * The in-flight promise is memoised, not just its result, so N concurrent cold + * requests issue one probe rather than N. + * + * Probed lazily because the database may not be reachable at registration time. + * A probe failure is treated as "unsupported", so it can never take the redirect + * path down. + */ + const resolveOwnerSuspensionSelect = createOwnerSuspensionSelect({ + query: (sql) => db.query(sql), + onSupported: () => + fastify.log.info('Redirect: owner restriction supported (organizations.suspended_at present)'), + }); // Helper function to handle the actual redirect logic async function handleRedirect(request: any, reply: any, shortCode: string, templateSlug?: string) { let linkData: string | null = null; @@ -158,11 +204,14 @@ export async function redirectRoutes(fastify: FastifyInstance) { let query: string; let params: any[]; + const ownerSuspensionColumn = await resolveOwnerSuspensionSelect(); + if (templateSlug) { // Template-based URL: verify both template and link match // Also fetch template settings and org settings for URL fallback chain query = ` SELECT l.*, t.settings AS template_settings, o.settings AS org_settings + ${ownerSuspensionColumn} FROM links l LEFT JOIN link_templates t ON l.template_id = t.id LEFT JOIN organizations o ON l.organization_id = o.id @@ -176,6 +225,7 @@ export async function redirectRoutes(fastify: FastifyInstance) { // Also fetch template settings and org settings for URL fallback chain query = ` SELECT l.*, t.settings AS template_settings, o.settings AS org_settings + ${ownerSuspensionColumn} FROM links l LEFT JOIN link_templates t ON l.template_id = t.id LEFT JOIN organizations o ON l.organization_id = o.id @@ -205,6 +255,42 @@ export async function redirectRoutes(fastify: FastifyInstance) { const link = JSON.parse(linkData); + // Safety gate, applied after the cache read so it covers cached rows too. + // + // It does NOT close the stale-cache window on its own, and an earlier version + // of this comment wrongly claimed it did: a cached row carries the value of + // `is_active` as at cache time, so reading it here sees the same stale `true` + // the old code did. Staleness is handled by invalidateLinkResolutionCache(), + // called when a link is updated or deleted. + // + // `is_active` therefore stays filtered in SQL — an inactive link never needs + // fetching or caching. What genuinely cannot be expressed in the WHERE clause + // is `warn_at`, which needs the row in hand to choose between redirecting and + // serving an interstitial. + const safety = evaluateLinkSafety({ + isActive: link.is_active, + warnAt: link.warn_at, + ownerSuspendedAt: link.owner_suspended_at, + }); + + if (safety === 'block') { + // Same response as an unknown code — see evaluateLinkSafety for why. + return reply.status(404).send({ error: 'Link not found' }); + } + + if (safety === 'warn') { + // No click is recorded here. A warning view is not a click on the link, and + // counting it would silently inflate the owner's analytics. + const destination = + link.original_url || link.web_fallback_url || link.deep_link_path || ''; + return reply + .status(200) + .header('X-Robots-Tag', 'noindex, nofollow') + .header('Cache-Control', 'no-store') + .type('text/html') + .send(generateWarningLinkHTML(destination, { reportUrl: options?.abuseReportUrl })); + } + // Check targeting rules BEFORE redirecting if (link.targeting_rules) { const userAgent = request.headers['user-agent'] || ''; diff --git a/src/routes/sdk-cache-bypass.test.ts b/src/routes/sdk-cache-bypass.test.ts new file mode 100644 index 0000000..ec0d029 --- /dev/null +++ b/src/routes/sdk-cache-bypass.test.ts @@ -0,0 +1,176 @@ +/** + * Regression test for the owner-restriction bypass via the SDK resolve endpoint. + * + * The redirect and `/api/sdk/v1/resolve/:shortCode` write the SAME Redis key. The SDK + * query used to omit the `organizations` join, so the row it cached carried no + * `owner_suspended_at`. The redirect then read `undefined`, its gate treated that as + * "not restricted", and a restricted owner's link redirected for the rest of the TTL. + * + * Both plugins share one fake Redis here, because sharing the cache is the mechanism — + * testing them in isolation cannot reproduce it. + */ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import Fastify, { type FastifyInstance } from 'fastify'; + +const query = vi.fn(); +vi.mock('../lib/database.js', () => ({ db: { query: (...a: unknown[]) => query(...a) } })); +import { redirectRoutes } from './redirect.js'; +import { sdkRoutes } from './sdk.js'; + +/** Minimal shared Redis stand-in — get/set/setex/del over one Map. */ +function fakeRedis() { + const store = new Map(); + return { + store, + get: async (k: string) => store.get(k) ?? null, + set: async (k: string, v: string) => void store.set(k, v), + setex: async (k: string, _ttl: number, v: string) => void store.set(k, v), + del: async (k: string) => void store.delete(k), + }; +} + +const RESTRICTED = '2026-08-10T00:00:00Z'; + +function row(overrides: Record = {}) { + return { + id: '00000000-0000-0000-0000-0000000000aa', + short_code: 'abc123', + organization_id: '00000000-0000-0000-0000-0000000000bb', + original_url: 'https://example.com/landing', + web_fallback_url: null, + deep_link_path: null, + is_active: true, + warn_at: null, + owner_suspended_at: RESTRICTED, + expires_at: null, + targeting_rules: null, + template_settings: null, + org_settings: null, + utm_parameters: null, + append_click_id: false, + ...overrides, + }; +} + +/** + * Mock the database so the row's shape follows the SELECT, exactly as Postgres would: + * `owner_suspended_at` is only present when the query asked for it. That is what makes + * this a real reproduction rather than a restatement of the fix. + */ +function mockDb() { + query.mockReset(); + query.mockImplementation(async (sql: string) => { + if (/information_schema\.columns/i.test(sql)) return { rows: [{ x: 1 }], rowCount: 1 }; + if (/FROM links/i.test(sql)) { + // The row's shape follows the SELECT, exactly as Postgres would: a column is + // present only if the query asked for it. That is what makes these real + // reproductions rather than restatements of the fix. + const r: Record = row(); + if (!/owner_suspended_at/i.test(sql)) delete r.owner_suspended_at; + if (!/template_settings/i.test(sql)) delete r.template_settings; + if (!/org_settings/i.test(sql)) delete r.org_settings; + return { rows: [r], rowCount: 1 }; + } + return { rows: [], rowCount: 0 }; + }); +} + +let redirectApp: FastifyInstance; +let sdkApp: FastifyInstance; +let redis: ReturnType; + +beforeEach(async () => { + mockDb(); + redis = fakeRedis(); + redirectApp = Fastify(); + redirectApp.decorate('redis', redis as any); + await redirectApp.register(redirectRoutes); + await redirectApp.ready(); + + sdkApp = Fastify(); + sdkApp.decorate('redis', redis as any); + await sdkApp.register(sdkRoutes); + await sdkApp.ready(); +}); + +afterEach(async () => { + await new Promise((r) => setImmediate(r)); + await redirectApp.close(); + await sdkApp.close(); +}); + +describe('owner restriction cannot be bypassed through the SDK cache', () => { + it('blocks a restricted owner when the redirect populates the cache itself', async () => { + const res = await redirectApp.inject({ method: 'GET', url: '/abc123' }); + expect(res.statusCode).toBe(404); + }); + + it('still blocks after an SDK resolve has primed the same cache key', async () => { + // This is the bypass: prime via the public SDK endpoint, then hit the redirect. + await sdkApp.inject({ method: 'GET', url: '/api/sdk/v1/resolve/abc123' }); + expect(redis.store.has('link:abc123')).toBe(true); + + const res = await redirectApp.inject({ method: 'GET', url: '/abc123' }); + expect(res.statusCode, 'a restricted owner must stay blocked on a cache hit').toBe(404); + expect(res.headers.location).toBeUndefined(); + }); + + it('the cached row carries owner_suspended_at whichever path wrote it', async () => { + await sdkApp.inject({ method: 'GET', url: '/api/sdk/v1/resolve/abc123' }); + const cached = JSON.parse(redis.store.get('link:abc123')!); + // Absent (not merely null) is what made the redirect's gate pass. + expect(Object.prototype.hasOwnProperty.call(cached, 'owner_suspended_at')).toBe(true); + }); + + it('the SDK endpoint itself refuses to resolve a restricted link', async () => { + // It returns the destination directly, so it must enforce the same policy — that + // is the information the redirect is refusing to disclose. + const res = await sdkApp.inject({ method: 'GET', url: '/api/sdk/v1/resolve/abc123' }); + expect(res.statusCode).toBe(404); + expect(res.body).not.toContain('example.com/landing'); + }); +}); + + +/** + * The same cache-shape divergence, one field over. + * + * The redirect reads `template_settings` and `org_settings` from this shared key for + * its URL fallback chain (link, then template, then workspace). While the SDK query + * omitted them, an SDK resolve left the next redirect unable to see a template-level + * fallback, so a link relying on one silently fell through to `original_url`. + */ +describe('the URL fallback chain survives an SDK resolve', () => { + /** No link-level web fallback; the destination lives on the template. */ + function templateFallbackRow() { + return { + ...row({ owner_suspended_at: null }), + original_url: 'https://example.com/ORIGINAL', + web_fallback_url: null, + template_settings: { defaultWebFallbackUrl: 'https://cdn.example.com/template-default' }, + org_settings: null, + }; + } + + beforeEach(() => { + query.mockReset(); + query.mockImplementation(async (sql: string) => { + if (/information_schema\.columns/i.test(sql)) return { rows: [{ x: 1 }], rowCount: 1 }; + if (/FROM links/i.test(sql)) { + const r: Record = templateFallbackRow(); + if (!/owner_suspended_at/i.test(sql)) delete r.owner_suspended_at; + if (!/template_settings/i.test(sql)) delete r.template_settings; + if (!/org_settings/i.test(sql)) delete r.org_settings; + return { rows: [r], rowCount: 1 }; + } + return { rows: [], rowCount: 0 }; + }); + }); + + it('caches template_settings whichever path writes the key', async () => { + await sdkApp.inject({ method: 'GET', url: '/api/sdk/v1/resolve/abc123' }); + const cached = JSON.parse(redis.store.get('link:abc123')!); + expect(Object.prototype.hasOwnProperty.call(cached, 'template_settings')).toBe(true); + expect(Object.prototype.hasOwnProperty.call(cached, 'org_settings')).toBe(true); + }); +}); diff --git a/src/routes/sdk.ts b/src/routes/sdk.ts index 37f40db..7457a9d 100644 --- a/src/routes/sdk.ts +++ b/src/routes/sdk.ts @@ -9,6 +9,7 @@ import { type FingerprintData, } from '../lib/fingerprint.js'; import { triggerWebhooks } from '../lib/webhook.js'; +import { evaluateLinkSafety, createOwnerSuspensionSelect } from '../lib/link-safety.js'; import { parseUserAgent, getLocationFromIP, detectDevice } from '../lib/utils.js'; import { emitClickEvent } from '../lib/event-emitter.js'; import { classifyBot, edgeBotSignal } from '../lib/bot-detection.js'; @@ -429,6 +430,32 @@ export async function sdkRoutes(fastify: FastifyInstance) { * - customParameters: Custom deep link parameters (key-value pairs) * - clickedAt: Timestamp of this resolution */ + /** + * Same probe the redirect uses, from the same factory. + * + * This endpoint writes the SAME Redis key as the redirect, so it must select the + * same columns — `owner_suspended_at`, `template_settings` and `org_settings`. + * + * Selecting fewer is not a cosmetic difference, because whichever path populates + * the key decides what the OTHER path can see: + * + * - omitting `owner_suspended_at` meant the redirect read `undefined`, its gate + * treated that as "not restricted", and a restricted owner's links resolved + * for the rest of the TTL. Public and unauthenticated, so that was triggerable + * on demand as well as by accident; + * - omitting the two `settings` columns silently breaks the redirect's URL + * fallback chain (link, then template, then workspace), so a link relying on a + * template-level `web_fallback_url` falls through to `original_url` instead. + * + * Neither is visible from this file alone, which is why the SELECT fragment comes + * from one shared factory and this list is kept deliberately in step. + */ + const resolveOwnerSuspensionSelect = createOwnerSuspensionSelect({ + query: (sql) => db.query(sql), + onSupported: () => + fastify.log.info('SDK resolve: owner restriction supported (organizations.suspended_at present)'), + }); + async function handleResolve(request: any, reply: any, shortCode: string, templateSlug?: string) { let linkData: string | null = null; @@ -445,13 +472,17 @@ export async function sdkRoutes(fastify: FastifyInstance) { } if (!linkData) { + const ownerSuspensionColumn = await resolveOwnerSuspensionSelect(); let query: string; let params: any[]; if (templateSlug) { query = ` - SELECT l.* FROM links l + SELECT l.*, t.settings AS template_settings, o.settings AS org_settings + ${ownerSuspensionColumn} + FROM links l LEFT JOIN link_templates t ON l.template_id = t.id + LEFT JOIN organizations o ON l.organization_id = o.id WHERE l.short_code = $1 AND t.slug = $2 AND l.is_active = true AND (l.expires_at IS NULL OR l.expires_at > NOW()) @@ -459,9 +490,13 @@ export async function sdkRoutes(fastify: FastifyInstance) { params = [shortCode, templateSlug]; } else { query = ` - SELECT * FROM links - WHERE short_code = $1 AND is_active = true - AND (expires_at IS NULL OR expires_at > NOW()) + SELECT l.*, t.settings AS template_settings, o.settings AS org_settings + ${ownerSuspensionColumn} + FROM links l + LEFT JOIN link_templates t ON l.template_id = t.id + LEFT JOIN organizations o ON l.organization_id = o.id + WHERE l.short_code = $1 AND l.is_active = true + AND (l.expires_at IS NULL OR l.expires_at > NOW()) `; params = [shortCode]; } @@ -486,6 +521,30 @@ export async function sdkRoutes(fastify: FastifyInstance) { const link = JSON.parse(linkData); + /** + * Same safety gate the redirect applies, evaluated after the cache read so it + * covers cached rows too. + * + * Without this, a link whose owner is restricted still resolved here — and this + * endpoint hands back the destination and deep-link data directly, which is + * precisely the information the redirect refuses to disclose. The redirect's + * guarantee is only as strong as the weakest path that resolves a short code. + * + * A `warn` outcome deliberately still resolves. The interstitial is a browser + * affordance an app cannot render, and refusing to resolve on mere suspicion + * would break legitimate apps for a signal that is not a confirmation. `block` + * is the outcome that means unreachable, and it is enforced here. + */ + const safety = evaluateLinkSafety({ + isActive: link.is_active, + warnAt: link.warn_at, + ownerSuspendedAt: link.owner_suspended_at, + }); + if (safety === 'block') { + // Same response as an unknown short code, and no click recorded. + return reply.status(404).send({ error: 'Link not found' }); + } + // Record click event + fingerprint asynchronously (mirrors redirect.ts pattern) setImmediate(async () => { try {