also looking at this
fix: validate site domain before it reaches nginx server_name
#129
5 files
+143
-5
Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.
| @@ -40,7 +40,7 @@ import { | ||
| 40 | 40 | setServerlessSecret, |
| 41 | 41 | updateFunctionConfig, |
| 42 | 42 | } from './serverless-operations' |
| 43 | import { addSiteToCloudConfig, removeSiteFromCloudConfig, renderAliasesValue, renderEnvValue, renderRedirectsValue, renderSslValue, renderStringValue, setSitePropertyInCloudConfig } from './site-config-editor' | |
| 43 | import { addSiteToCloudConfig, isValidHostname, removeSiteFromCloudConfig, renderAliasesValue, renderEnvValue, renderRedirectsValue, renderSslValue, renderStringValue, setSitePropertyInCloudConfig } from './site-config-editor' | |
| 44 | 44 | import { addSshKeyToCloudConfig, describeSshKeys, removeSshKeyFromCloudConfig } from './ssh-config-editor' |
| 45 | 45 | import { createTerminalSession } from './terminal-session' |
| 46 | 46 | |
| @@ -860,6 +860,15 @@ export async function startLocalDashboardServer(options: LocalDashboardServerOpt | ||
| 860 | 860 | if (body.port !== undefined && body.port !== null && body.port !== '' && (!Number.isInteger(Number(body.port)) || Number(body.port) < 1 || Number(body.port) > 65_535)) |
| 861 | 861 | return json({ ok: false, error: 'Port must be a number between 1 and 65535.' }, 422) |
| 862 | 862 | |
| 863 | // `domain` lands in the generated nginx `server_name`, so it must be a | |
| 864 | // hostname and nothing else — an unvalidated value can close the | |
| 865 | // server block and open an attacker-controlled one. `aliases` (same | |
| 866 | // destination) has always been checked; this closes the gap for the | |
| 867 | // primary domain. Validated before any write so a bad value can't | |
| 868 | // leave the config half-edited. | |
| 869 | if (typeof body.domain === 'string' && body.domain.trim() && !isValidHostname(body.domain.trim())) | |
| 870 | return json({ ok: false, error: `Domain '${body.domain.trim()}' is not a valid hostname.` }, 422) | |
| 871 | ||
| 863 | 872 | let text = await readFile(configPath, 'utf8') |
| 864 | 873 | const set = (key: string, valueText: string): void => { |
| 865 | 874 | text = setSitePropertyInCloudConfig({ configText: text, siteName: name, key, valueText }) |
| @@ -236,7 +236,16 @@ export function renderSiteSnippet(input: Omit<AddSiteConfigInput, 'configText'>) | ||
| 236 | 236 | } |
| 237 | 237 | |
| 238 | 238 | function escapeSingle(value: string): string { |
| 239 | return value.replace(/\\/g, '\\\\').replaceAll(String.fromCharCode(39), '\\\'') | |
| 239 | // Newlines matter as much as quotes here: these values land in single-quoted | |
| 240 | // TS string literals in the shared, box-wide cloud.config.ts. A raw newline | |
| 241 | // terminates the literal and leaves the file syntactically broken for every | |
| 242 | // tenant that loads it. It also keeps line-oriented sinks downstream (heredoc | |
| 243 | // delimiters in the deploy script) safe from values that span lines. | |
| 244 | return value | |
| 245 | .replace(/\\/g, '\\\\') | |
| 246 | .replaceAll(String.fromCharCode(39), '\\\'') | |
| 247 | .replace(/\r/g, '\\r') | |
| 248 | .replace(/\n/g, '\\n') | |
| 240 | 249 | } |
| 241 | 250 | |
| 242 | 251 | function normalizeSiteName(name: string): string { |
| @@ -0,0 +1,97 @@ | ||
| 1 | import { describe, expect, it } from 'bun:test' | |
| 2 | import { isValidHostname, renderStringValue } from '../../src/deploy/site-config-editor' | |
| 3 | import { buildNginxVhost } from '../../src/drivers/shared/nginx-vhost' | |
| 4 | ||
| 5 | /** | |
| 6 | * Regression tests for nginx `server_name` injection. | |
| 7 | * | |
| 8 | * A site's `domain` is member-editable and is interpolated straight into the | |
| 9 | * generated `server_name` directive. nginx is whitespace-insensitive, so an | |
| 10 | * unvalidated value can close the server block and open an attacker-controlled | |
| 11 | * one — e.g. `location / { root /; autoindex on; }`, which exposes the whole | |
| 12 | * filesystem (other tenants' .env files, the dashboard user store, SSH keys) | |
| 13 | * over HTTP on a shared box. | |
| 14 | */ | |
| 15 | ||
| 16 | // Closes the generated block, opens a filesystem-exposing one, then reopens a | |
| 17 | // server block so the result still parses. | |
| 18 | const INJECTION = 'x.com; } location / { root /; autoindex on; } server { server_name y.com' | |
| 19 | ||
| 20 | describe('server_name injection', () => { | |
| 21 | it('rejects a domain carrying nginx directives', () => { | |
| 22 | expect(isValidHostname(INJECTION)).toBe(false) | |
| 23 | }) | |
| 24 | ||
| 25 | it('refuses to build a vhost from an injected domain', () => { | |
| 26 | expect(() => buildNginxVhost({ | |
| 27 | siteName: 'app', | |
| 28 | domain: INJECTION, | |
| 29 | appDir: '/var/www/app/current', | |
| 30 | })).toThrow(/not a valid hostname/) | |
| 31 | }) | |
| 32 | ||
| 33 | it('refuses to build a vhost from an injected alias', () => { | |
| 34 | expect(() => buildNginxVhost({ | |
| 35 | siteName: 'app', | |
| 36 | domain: 'example.com', | |
| 37 | aliases: [INJECTION], | |
| 38 | appDir: '/var/www/app/current', | |
| 39 | })).toThrow(/not a valid hostname/) | |
| 40 | }) | |
| 41 | ||
| 42 | it('rejects whitespace, newlines and directive punctuation in a hostname', () => { | |
| 43 | for (const bad of ['a.com b.com', 'a.com\nserver_name evil.com', 'a.com;', 'a.com{', 'a.com}']) { | |
| 44 | expect(() => buildNginxVhost({ | |
| 45 | siteName: 'app', | |
| 46 | domain: bad, | |
| 47 | appDir: '/var/www/app/current', | |
| 48 | })).toThrow(/not a valid hostname/) | |
| 49 | } | |
| 50 | }) | |
| 51 | ||
| 52 | it('rejects an empty server_name rather than emitting `server_name ;`', () => { | |
| 53 | expect(() => buildNginxVhost({ | |
| 54 | siteName: 'app', | |
| 55 | domain: '', | |
| 56 | appDir: '/var/www/app/current', | |
| 57 | })).toThrow(/no server_name/) | |
| 58 | }) | |
| 59 | ||
| 60 | // compute-deploy falls back to `domain: site.domain || siteName`, so an | |
| 61 | // internal site with no configured domain arrives here as a single label. | |
| 62 | // The generator must keep accepting those or every such deploy breaks. | |
| 63 | it('accepts a single-label host (the siteName fallback)', () => { | |
| 64 | for (const host of ['main', 'docs', 'localhost']) { | |
| 65 | const vhost = buildNginxVhost({ | |
| 66 | siteName: host, | |
| 67 | domain: host, | |
| 68 | appDir: '/var/www/app/current', | |
| 69 | }) | |
| 70 | expect(vhost).toContain(`server_name ${host};`) | |
| 71 | } | |
| 72 | }) | |
| 73 | ||
| 74 | it('still builds a normal vhost, including wildcard aliases', () => { | |
| 75 | const vhost = buildNginxVhost({ | |
| 76 | siteName: 'app', | |
| 77 | domain: 'app.example.com', | |
| 78 | aliases: ['www.example.com', '*.cdn.example.com'], | |
| 79 | appDir: '/var/www/app/current', | |
| 80 | }) | |
| 81 | expect(vhost).toContain('server_name app.example.com www.example.com *.cdn.example.com;') | |
| 82 | }) | |
| 83 | }) | |
| 84 | ||
| 85 | describe('cloud.config.ts string escaping', () => { | |
| 86 | it('escapes newlines so a value cannot terminate the string literal', () => { | |
| 87 | const rendered = renderStringValue('a\nb') | |
| 88 | expect(rendered).not.toContain('\n') | |
| 89 | expect(rendered).toBe('\'a\\nb\'') | |
| 90 | }) | |
| 91 | ||
| 92 | it('escapes carriage returns, quotes and backslashes', () => { | |
| 93 | expect(renderStringValue('a\rb')).not.toContain('\r') | |
| 94 | expect(renderStringValue('it\'s')).toBe('\'it\\\'s\'') | |
| 95 | expect(renderStringValue('a\\b')).toBe('\'a\\\\b\'') | |
| 96 | }) | |
| 97 | }) | |