-
Notifications
You must be signed in to change notification settings - Fork 14
feat(redirect): link safety states and owner-restriction gate #37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
91b8cd4
feat(redirect): add link safety states and owner-restriction gate
onamfc 63fce4c
test(redirect): route-level coverage for the link safety gate
onamfc e5c8a75
fix(redirect): reject non-http schemes in the interstitial, and corre…
onamfc 5924a04
docs(redirect): note that owner-restriction probing depends on the or…
onamfc dc78b49
fix(sdk): close the owner-restriction bypass through the resolve endp…
onamfc c2d04b8
fix(sdk): select the same settings columns as the redirect, not just …
onamfc File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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('<script>alert("x")</script>')).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/"><script>alert(1)</script>'); | ||
| expect(html).not.toContain('<script>alert(1)</script>'); | ||
| 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('<script'); | ||
| expect(html).not.toMatch(/\son[a-z]+=/i); | ||
| }); | ||
|
|
||
| // The CSP case above used a benign destination, so it could never have failed for | ||
| // the reason it appeared to cover. These pass the hostile input instead. | ||
| describe('hostile destinations', () => { | ||
| 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,<script>alert(1)</script>'); | ||
| 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(); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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, '"') | ||
| .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 | ||
| ? `<a class="go" href="${escapeHtml(href)}" rel="nofollow noopener noreferrer">Continue anyway</a>` | ||
| : `<p class="inert">This link has no usable web destination, so there is nothing to continue to.</p>`; | ||
| const reportLink = options.reportUrl | ||
| ? `<p class="report"><a href="${escapeHtml(options.reportUrl)}" rel="nofollow noopener">Report this link</a></p>` | ||
| : ''; | ||
|
|
||
| return `<!doctype html> | ||
| <html lang="en"> | ||
| <head> | ||
| <meta charset="utf-8"> | ||
| <meta name="viewport" content="width=device-width,initial-scale=1"> | ||
| <meta name="robots" content="noindex,nofollow"> | ||
| <title>Check this link before continuing</title> | ||
| <style> | ||
| :root { color-scheme: light dark; } | ||
| body { margin:0; min-height:100vh; display:flex; align-items:center; justify-content:center; | ||
| background:#f6f7f8; color:#16191d; padding:24px; | ||
| font:16px/1.55 -apple-system,BlinkMacSystemFont,"Segoe UI",Roboto,sans-serif; } | ||
| .card { max-width:34rem; width:100%; background:#fff; border:1px solid #e3e6ea; | ||
| border-radius:10px; padding:28px; } | ||
| h1 { margin:0 0 12px; font-size:1.35rem; line-height:1.25; } | ||
| p { margin:0 0 14px; } | ||
| .dest { display:block; word-break:break-all; background:#f0f2f4; border-radius:6px; | ||
| padding:10px 12px; font-family:ui-monospace,SFMono-Regular,Menlo,monospace; | ||
| font-size:.9rem; margin-bottom:18px; } | ||
| a.go { display:inline-block; text-decoration:none; border:1px solid #c9ced4; color:#16191d; | ||
| border-radius:6px; padding:9px 15px; font-size:.95rem; font-weight:600; } | ||
| .inert { margin:0; font-size:.9rem; color:#5b636d; } | ||
| .report { margin:16px 0 0; font-size:.85rem; } | ||
| .report a { color:#5b636d; } | ||
| @media (prefers-color-scheme: dark) { | ||
| body { background:#14171a; color:#e8eaed; } | ||
| .card { background:#1d2126; border-color:#2c3238; } | ||
| .dest { background:#14171a; } | ||
| a.go { border-color:#3a4149; color:#e8eaed; } | ||
| .report a { color:#98a1ab; } | ||
| .inert { color:#98a1ab; } | ||
| } | ||
| </style> | ||
| </head> | ||
| <body> | ||
| <main class="card"> | ||
| <h1>Check this link before continuing</h1> | ||
| <p>This short link has been flagged as possibly unsafe, so we have not sent you | ||
| straight there. It leads to:</p> | ||
| <span class="dest">${shown}</span> | ||
| <p>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.</p> | ||
| ${continueButton} | ||
| ${reportLink} | ||
| </main> | ||
| </body> | ||
| </html>`; | ||
| } | ||
|
|
||
| /** | ||
| * 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<string> { | ||
| let probe: Promise<string> | 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; | ||
| }; | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirming this holds up — I tried to break it 22 ways and couldn't:
JaVaScRiPt:,JAVASCRIPT:),\t,\n,\r) includingjava\nscript:alert(1), which the URL parser strips before scheme detectiondata:(raw and base64),vbscript:,file:,blob:,about://evil.example.com/*--></title></style></textarea></script><svg onload=alert(1)>breakout payloadAll return
null; genuinehttp/httpsdestinations pass through unchanged.One thing worth keeping deliberate: this returns the original string rather than
url.href, so the parsed value is what gets validated while the raw value is what gets rendered. That's correct as written becauseescapeHtmlhandles the raw string at the call site — but it does mean the two must stay paired. Worth a short note here saying the return value is still untrusted markup-wise and must be escaped by callers, so nobody later usessafeHrefoutput directly in a template.