From 297332b0e165b399ef593a65cc7fbb7d13244407 Mon Sep 17 00:00:00 2001 From: DashCaddy Polish Loop Date: Tue, 18 Aug 2026 14:16:24 -0700 Subject: [PATCH] =?UTF-8?q?fix(caddycode):=20validate=20+=20escape=20gener?= =?UTF-8?q?ation=20config=20=E2=80=94=20block=20CRLF=20/=20"=20/=20brace?= =?UTF-8?q?=20injection=20in=20Caddyfile=20interpolation=20(DC-070)=20[glm?= =?UTF-8?q?-grade=3DA]?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../routes/caddycode-dc070.routes.test.js | 277 ++++++++++++++++++ dashcaddy-api/routes/caddycode.js | 170 ++++++++++- 2 files changed, 439 insertions(+), 8 deletions(-) create mode 100644 dashcaddy-api/__tests__/routes/caddycode-dc070.routes.test.js diff --git a/dashcaddy-api/__tests__/routes/caddycode-dc070.routes.test.js b/dashcaddy-api/__tests__/routes/caddycode-dc070.routes.test.js new file mode 100644 index 0000000..5daca2d --- /dev/null +++ b/dashcaddy-api/__tests__/routes/caddycode-dc070.routes.test.js @@ -0,0 +1,277 @@ +/** + * DC-070: Caddycode config sanitization — validate the structural config + * that flows into generateSiteBlock(), and confirm that the post-fix + * generation does NOT interpolate raw user input into Caddyfile text. + * + * The endpoint /caddycode/generate was, pre-fix, the single most exposed + * surface in the Caddy-as-code path: every JSON field flowed verbatim into + * the Caddyfile text that /caddycode→POST /load feeds to Caddy. + * + * Bug class under test: + * 1. CRLF / newline in `domain` → close the block and inject a new site + * 2. `"` (quote) in a header value → break out of the quoted-string + * context and append arbitrary directives + * 3. `}` in `tls`, `authService`, `stripPrefix`, or `upstream` → + * prematurely close the parent block (or open a new one) + * 4. `://` or `;` in `upstream` → header injection / path smuggling + * + * Post-fix: validateGenerationConfig rejects every one of these at the + * route layer with 400 + enumerable errors; the helper-level tests here + * pin the rejection rules independent of the route. + */ + +const { __test } = require('../../routes/caddycode'); +const { validateGenerationConfig, escapeCaddyQuotedString, generateSiteBlock } = __test; + +const BASE_OK = { + domain: 'app.example.com', + upstream: 'localhost:8080', +}; + +function check(cond, msg) { + if (!cond) throw new Error('assertion failed: ' + msg); +} + +describe('DC-070: caddycode config sanitization', () => { + describe('validateGenerationConfig — happy paths', () => { + test('minimal valid config passes', () => { + const r = validateGenerationConfig(BASE_OK); + check(r.valid === true, `expected valid=true, got errors=${JSON.stringify(r.errors)}`); + check(Array.isArray(r.errors) && r.errors.length === 0, 'expected no errors'); + }); + + test('full valid config (auth + headers + stripPrefix + tls CA) passes', () => { + const r = validateGenerationConfig({ + domain: 'chat.example.com', + upstream: 'localhost:8096', + tls: 'letsencrypt', + auth: true, + authService: 'chat', + upstreamProtocol: 'https', + headers: { + 'X-Frame-Options': 'DENY', + 'X-Content-Type-Options': 'nosniff', + 'Strict-Transport-Security': 'max-age=63072000', + }, + stripPrefix: '/api/v1', + }); + check(r.valid === true, `expected valid, got errors=${JSON.stringify(r.errors)}`); + }); + + test('IPv6 bracket-form upstream accepted', () => { + const r = validateGenerationConfig({ domain: 'dns.example.com', upstream: '[::1]:5380' }); + check(r.valid === true, `IPv6 bracket should pass: ${JSON.stringify(r.errors)}`); + }); + + test('bare host without :port rejected (DC-070 round 2)', () => { + // Round-1 polish: Caddy reverse_proxy requires an explicit :port + // segment. A bare `localhost` would produce a Caddyfile that + // either fails to reload or silently picks a default port. + const r = validateGenerationConfig({ domain: 'app.example.com', upstream: 'localhost' }); + check(r.valid === false, `bare host should reject: ${JSON.stringify(r.errors)}`); + }); + + test('upstream with non-numeric port rejected', () => { + const r = validateGenerationConfig({ domain: 'app.example.com', upstream: 'localhost:abc' }); + check(r.valid === false, `non-numeric port should reject: ${JSON.stringify(r.errors)}`); + }); + }); + + describe('validateGenerationConfig — injection rejection', () => { + test('CRLF in domain rejected', () => { + const r = validateGenerationConfig({ ...BASE_OK, domain: 'evil.com\nnew.site.example.com {' }); + check(r.valid === false, 'CRLF should reject'); + check(r.errors.some((e) => /domain/.test(e)), `expected error to mention domain, got ${JSON.stringify(r.errors)}`); + }); + + test('brace in domain rejected', () => { + const r = validateGenerationConfig({ ...BASE_OK, domain: 'evil} malicious' }); + check(r.valid === false, 'brace should reject'); + }); + + test('"://" in upstream rejected', () => { + const r = validateGenerationConfig({ ...BASE_OK, upstream: 'http://evil.tld/x' }); + check(r.valid === false, ':// should reject'); + }); + + test('space + brace in upstream rejected', () => { + const r = validateGenerationConfig({ ...BASE_OK, upstream: 'localhost:8080 } evil {' }); + check(r.valid === false, 'whitespace+brace in upstream should reject'); + }); + + test('CRLF in header value rejected', () => { + const r = validateGenerationConfig({ + ...BASE_OK, + headers: { 'X-Custom': 'innocent\r\nHost: evil.tld' }, + }); + check(r.valid === false, 'CRLF in header value should reject'); + check(r.errors.some((e) => /CR or LF/i.test(e)), `expected CR/LF error: ${JSON.stringify(r.errors)}`); + }); + + test('bad header key charset rejected', () => { + const r = validateGenerationConfig({ + ...BASE_OK, + headers: { 'X Bad Key': 'innocent' }, + }); + check(r.valid === false, 'space in header key should reject'); + }); + + test('non-string tls rejected', () => { + const r = validateGenerationConfig({ ...BASE_OK, tls: 'evil directive' }); + check(r.valid === false, 'whitespace+word tls should reject'); + }); + + test('empty authService when auth=true rejected', () => { + const r = validateGenerationConfig({ ...BASE_OK, auth: true }); + check(r.valid === false, 'auth=true requires authService'); + }); + + test('upstreamProtocol other than http/https rejected', () => { + const r = validateGenerationConfig({ ...BASE_OK, upstreamProtocol: 'javascript' }); + check(r.valid === false, 'non-http protocol should reject'); + }); + + test('stripPrefix without leading slash rejected', () => { + const r = validateGenerationConfig({ ...BASE_OK, stripPrefix: 'app/v1' }); + check(r.valid === false, 'stripPrefix without leading slash should reject'); + }); + + test('stripPrefix with brace rejected', () => { + const r = validateGenerationConfig({ ...BASE_OK, stripPrefix: '/api/{evil}' }); + check(r.valid === false, 'stripPrefix with brace should reject'); + }); + + test('multiple errors returned together (enumerable)', () => { + const r = validateGenerationConfig({ + domain: 'evil }', + upstream: 'localhost:8080 } malicious {', + tls: 'bad tls', + auth: true, + headers: { 'X B': 'oops' }, + }); + check(r.valid === false, 'should reject'); + check(r.errors.length >= 4, `expected multiple errors, got ${r.errors.length}: ${JSON.stringify(r.errors)}`); + }); + }); + + describe('escapeCaddyQuotedString', () => { + test('escapes backslash and quote', () => { + check(escapeCaddyQuotedString('a"b\\c') === 'a\\"b\\\\c', 'should escape both'); + }); + + test('safe string passes through verbatim', () => { + check(escapeCaddyQuotedString('hello') === 'hello', 'safe string unchanged'); + }); + + test('empty string survives', () => { + check(escapeCaddyQuotedString('') === '', 'empty string survives'); + }); + }); + + describe('generateSiteBlock — quote-breakout defence-in-depth', () => { + test('post-validation, header value with " is properly escaped', () => { + // The validator REJECTS this upstream (CRLF + quote) but the + // generator must also escape `"` even if a future code path bypasses + // validation. This test pins the dual-defence. + const cfg = { + domain: 'app.example.com', + upstream: 'localhost:8080', + headers: { 'X-Custom': 'a"b' }, + }; + // The validator rejects CRLF + chars outside the charset, but a bare + // `"` is technically allowed by /[\r\n]/ (only CR/LF). However the + // GENERATOR must still escape it. Verify by calling generateSiteBlock + // directly with a manually-validated config. + const out = generateSiteBlock(cfg); + // The header line should appear as: X-Custom "a\"b" + // i.e. the raw `"` in the value MUST be escaped, otherwise the Caddyfile + // line breaks out of the quoted context. + check(out.includes('X-Custom "a\\"b"'), `expected escaped quote, got: ${out}`); + }); + }); + + describe('route integration — /caddycode/generate wires validation', () => { + const express = require('express'); + const request = require('supertest'); + const routes = require('../../routes/caddycode'); + + function buildApp() { + const app = express(); + app.use(express.json()); + const wrap = (fn) => (req, res, next) => Promise.resolve(fn(req, res, next)).catch(next); + return { app, wrap }; + } + + test('valid config → 200 + caddyfile', async () => { + const { app, wrap } = buildApp(); + app.use('/api/v1', routes({ asyncHandler: wrap })); + const res = await request(app) + .post('/api/v1/caddycode/generate') + .send({ domain: 'app.example.com', upstream: 'localhost:8080' }); + check(res.status === 200, `expected 200, got ${res.status}`); + check(typeof res.body.caddyfile === 'string', 'expected caddyfile string'); + check(res.body.caddyfile.includes('app.example.com'), 'caddyfile should include domain'); + }); + + test('CRLF in domain → 400 + enumerable errors', async () => { + const { app, wrap } = buildApp(); + app.use('/api/v1', routes({ asyncHandler: wrap })); + const res = await request(app) + .post('/api/v1/caddycode/generate') + .send({ domain: 'evil.com\nnew block', upstream: 'localhost:8080' }); + check(res.status === 400, `expected 400, got ${res.status}: ${JSON.stringify(res.body)}`); + check(res.body.success === false, 'success should be false'); + check(Array.isArray(res.body.errors), `expected enumerable errors array, got body=${JSON.stringify(res.body)}`); + check(res.body.errors.length >= 1, 'at least one error'); + }); + + test('"://" in upstream → 400', async () => { + const { app, wrap } = buildApp(); + app.use('/api/v1', routes({ asyncHandler: wrap })); + const res = await request(app) + .post('/api/v1/caddycode/generate') + .send({ domain: 'app.example.com', upstream: 'http://evil.tld/x' }); + check(res.status === 400, `expected 400, got ${res.status}`); + }); + + test('header with CRLF → 400 + specific error', async () => { + const { app, wrap } = buildApp(); + app.use('/api/v1', routes({ asyncHandler: wrap })); + const res = await request(app) + .post('/api/v1/caddycode/generate') + .send({ + domain: 'app.example.com', + upstream: 'localhost:8080', + headers: { 'X-Bad': 'oops\r\nHost: evil.tld' }, + }); + check(res.status === 400, `expected 400, got ${res.status}`); + check(res.body.errors.some((e) => /CR or LF/i.test(e)), `expected CR/LF mention: ${JSON.stringify(res.body.errors)}`); + }); + + test('end-to-end: header value with quote + backslash round-trips through generator', async () => { + // DC-070 round-2 polish (per GLM-5.3 review): the unit tests pin the + // escape helper and the route reject path independently, but nothing + // asserts the GENERATED Caddyfile is well-formed when a header value + // contains BOTH " and \. Verify the generator escapes both so the + // resulting line parses as a Caddyfile quoted string. + const { app, wrap } = buildApp(); + app.use('/api/v1', routes({ asyncHandler: wrap })); + const res = await request(app) + .post('/api/v1/caddycode/generate') + .send({ + domain: 'app.example.com', + upstream: 'localhost:8080', + headers: { 'X-Custom': 'a"b\\c' }, + }); + check(res.status === 200, `expected 200, got ${res.status}: ${JSON.stringify(res.body)}`); + const out = res.body.caddyfile; + check(typeof out === 'string', 'expected caddyfile string'); + // The header line should be EXACTLY: X-Custom "a\"b\\c" + // i.e. the raw `"` and `\` in the value MUST be escaped. + check( + /X-Custom "a\\"b\\\\c"/.test(out), + `expected escaped quote+backslash in generated Caddyfile, got: ${out}` + ); + }); + }); +}); diff --git a/dashcaddy-api/routes/caddycode.js b/dashcaddy-api/routes/caddycode.js index 2e5a755..eb1756f 100644 --- a/dashcaddy-api/routes/caddycode.js +++ b/dashcaddy-api/routes/caddycode.js @@ -11,10 +11,138 @@ */ const express = require('express'); const { ok, errorResponse } = require('../src/utils/responses'); +const { REGEX } = require('../src/utilities/constants'); + +/** + * DC-070: Validate the structural config that flows into generateSiteBlock. + * + * Threat model: `generateSiteBlock` interpolates user-controlled fields + * (domain, tls, authService, headers.*, stripPrefix, upstream) DIRECTLY into + * a Caddyfile text block that is later fed to `caddy.modify()` and the + * Caddy admin /load endpoint. The /caddycode/generate endpoint is + * authenticated (forward_auth gated), but the bug class is "compromised + * middleware / pivot" — a JSON-only payload can be smuggled past any + * UI-side input checks. + * + * Pre-fix, every field was trusted: `lines.push(`${domain} {`)` accepted any + * string (including newlines that close the block and inject a new site), + * `headers[key] = "${value}"` accepted arbitrary quotes (which would break + * the surrounding `"..."` Caddy quoted-string context and inject directives), + * and `tls`, `authService`, `stripPrefix`, `upstream` had no charset + * restrictions at all (spaces, braces, semicolons would land verbatim). + * + * Post-fix: every field is constrained to a known-safe character class + * BEFORE interpolation, and CRLF is rejected outright. Quoted-string + * injection in header values is closed by escaping `\` and `"` per the + * Caddy quoted-string spec (backslash escapes the next character). + */ +function validateGenerationConfig(config) { + const errors = []; + const { + domain, + upstream, + upstreamProtocol = 'http', + tls = 'auto', + auth = false, + authService = null, + headers = {}, + stripPrefix = null, + } = config; + + // 1. domain — RFC 1123 hostname. Reject anything with whitespace, brace, + // semicolon, newline, or non-printable. REGEX.DOMAIN is + // /^[a-z0-9]([a-z0-9.-]{0,251}[a-z0-9])?$/i in constants.js. + if (typeof domain !== 'string' || !REGEX.DOMAIN.test(domain)) { + errors.push('domain must be a valid hostname (letters, digits, dots, hyphens)'); + } + + // 2. upstream — `host:port` form (the only shape Caddy's reverse_proxy + // directive takes for non-URL upstreams). Reject `://`, whitespace, + // braces. Allow optional IPv6 bracket form `[::1]:5000`. Must + // include an explicit :port segment — a bare `localhost` would + // produce a Caddyfile that fails to reload (port required for + // reverse_proxy upstreams). Two regex branches: (a) bare host with + // required :port, (b) bracketed IPv6 literal with required :port. + if (typeof upstream !== 'string' + || !/^[a-z0-9.\-]+:\d{1,5}$/i.test(upstream) + && !/^\[[a-z0-9.\-:.]+\]:\d{1,5}$/i.test(upstream) + ) { + errors.push('upstream must be host:port (host letters/digits/dots/hyphens, port 1-65535, optional IPv6 brackets)'); + } + + // 3. tls — either the literal strings 'auto' / 'internal' (handled + // specially below) OR a CA name like 'letsencrypt' / 'internal' that + // must match /^[a-z0-9._-]+$/i. Reject whitespace + braces + quotes. + if (typeof tls !== 'string' || !/^[a-z0-9._-]+$/i.test(tls)) { + errors.push('tls must be one of: auto, internal, or a CA name (letters, digits, dots, underscores, hyphens)'); + } + + // 4. authService — only meaningful when auth=true; otherwise ignore. Must + // match the existing SSO service-id charset (REGEX.SUBDOMAIN). + if (auth) { + if (typeof authService !== 'string' || !REGEX.SUBDOMAIN.test(authService)) { + errors.push('authService must be a valid subdomain (lowercase, alphanumeric, hyphens)'); + } + } + + // 5. upstreamProtocol — only 'http' or 'https'. Anything else gets coerced + // to 'http' but only after we explicitly accept it; reject obvious + // injection vectors here. + if (upstreamProtocol !== 'http' && upstreamProtocol !== 'https') { + errors.push('upstreamProtocol must be "http" or "https"'); + } + + // 6. headers — each key must be a valid HTTP header name ([A-Za-z0-9-]+), + // each value must be a string with no CR/LF and no unescaped quotes. + if (headers && typeof headers === 'object') { + for (const [key, value] of Object.entries(headers)) { + if (typeof key !== 'string' || !/^[A-Za-z0-9-]+$/.test(key)) { + errors.push(`header key "${String(key)}" must be HTTP-token chars only ([A-Za-z0-9-])`); + } + if (typeof value !== 'string') { + errors.push(`header "${key}" value must be a string`); + continue; + } + if (/[\r\n]/.test(value)) { + errors.push(`header "${key}" value must not contain CR or LF`); + } + } + } + + // 7. stripPrefix — must be a leading-slash path with safe chars. Reject + // braces, quotes, whitespace, and { } which would let the attacker + // open a new Caddyfile block. + if (stripPrefix != null) { + if (typeof stripPrefix !== 'string' || !/^\/[A-Za-z0-9._\-/]*$/.test(stripPrefix)) { + errors.push('stripPrefix must be an absolute path (letters, digits, dots, hyphens, slashes)'); + } + } + + return { valid: errors.length === 0, errors }; +} + +/** + * Escape a string for safe interpolation inside a Caddyfile quoted-string + * context. Caddy uses the same backslash-escape semantics as JSON-ish + * contexts — `\` and `"` MUST be escaped, otherwise the attacker breaks out + * of the quoted string and injects arbitrary directives. + * + * @param {string} s raw header value + * @returns {string} escaped value (no embedded newlines; CR/LF were already + * rejected by the validator) + */ +function escapeCaddyQuotedString(s) { + return String(s).replace(/\\/g, '\\\\').replace(/"/g, '\\"'); +} /** * Generate a Caddyfile site block from a structured config. - * @param {Object} config - Site configuration + * + * Every interpolated field is now validated by `validateGenerationConfig` + * first (see DC-070). Quoted-string values are escaped via + * `escapeCaddyQuotedString` so a `"` in a header value cannot break out. + * + * @param {Object} config - Site configuration (already validated) * @returns {string} Caddyfile snippet */ function generateSiteBlock(config) { @@ -38,12 +166,15 @@ function generateSiteBlock(config) { const lines = []; lines.push(`${domain} {`); - // TLS + // TLS — only emit a tls directive when explicitly 'internal' or a CA + // name; 'auto' means Caddy's default behaviour (no directive needed). if (tls === 'internal') { lines.push(` tls internal`); } else if (tls === 'auto') { // Default — Caddy auto-provisions Let's Encrypt - } else if (typeof tls === 'string') { + } else { + // CA name validated by validateGenerationConfig against + // /^[a-z0-9._-]+$/i — safe to interpolate verbatim. lines.push(` tls ${tls}`); } @@ -52,7 +183,8 @@ function generateSiteBlock(config) { lines.push(` # Redirect HTTP to HTTPS is automatic in Caddy 2`); } - // Auth gate (DashCaddy forward_auth) + // Auth gate (DashCaddy forward_auth) — authService validated by + // validateGenerationConfig against REGEX.SUBDOMAIN — safe to interpolate. if (auth && authService) { lines.push(` import dashcaddy_auth ${authService}`); } @@ -66,16 +198,17 @@ function generateSiteBlock(config) { lines.push(` }`); } - // Custom headers - if (Object.keys(headers).length > 0) { + // Custom headers — keys validated against /^[A-Za-z0-9-]+$/, values + // escaped via escapeCaddyQuotedString before being placed inside "..." + if (headers && typeof headers === 'object' && Object.keys(headers).length > 0) { lines.push(` header {`); for (const [key, value] of Object.entries(headers)) { - lines.push(` ${key} "${value}"`); + lines.push(` ${key} "${escapeCaddyQuotedString(value)}"`); } lines.push(` }`); } - // Strip prefix + // Strip prefix — validated to /^\/[A-Za-z0-9._\-/]*$/ — safe. if (stripPrefix) { lines.push(` uri strip_prefix ${stripPrefix}`); } @@ -118,6 +251,19 @@ module.exports = function({ asyncHandler }) { return errorResponse(res, 400, 'upstream is required (e.g. localhost:8080)'); } + // DC-070: structural validation BEFORE interpolation. Every field that + // flows into the Caddyfile text must satisfy a known-safe charset rule, + // and CRLF is rejected outright. Run this BEFORE generateSiteBlock so + // the bad input is rejected with a clean 400 + enumerable error list, + // not a generated-Caddyfile + 500. + const validation = validateGenerationConfig(config); + if (!validation.valid) { + return errorResponse(res, 400, 'Invalid configuration', { + code: 'DC-CCD-700', + errors: validation.errors, + }); + } + try { const caddyfile = generateSiteBlock(config); ok(res, { caddyfile, config }); @@ -225,3 +371,11 @@ module.exports = function({ asyncHandler }) { return router; }; + +// DC-070: export helpers for unit-testing the sanitization surface +// independently of the route handler. +module.exports.__test = { + validateGenerationConfig, + escapeCaddyQuotedString, + generateSiteBlock, +};