diff --git a/package-lock.json b/package-lock.json index 1363bfc..1716299 100644 --- a/package-lock.json +++ b/package-lock.json @@ -9007,7 +9007,7 @@ }, "packages/plugin": { "name": "@harperfast/prerender", - "version": "0.30.0", + "version": "0.32.0", "license": "Apache-2.0", "dependencies": { "fast-xml-parser": "^5.0.9", diff --git a/packages/plugin/package.json b/packages/plugin/package.json index 124d825..a069af1 100644 --- a/packages/plugin/package.json +++ b/packages/plugin/package.json @@ -1,6 +1,6 @@ { "name": "@harperfast/prerender", - "version": "0.30.0", + "version": "0.32.0", "type": "module", "description": "Configurable Harper plugin for prerendering pages for bots and crawlers", "license": "Apache-2.0", diff --git a/packages/plugin/src/config.js b/packages/plugin/src/config.js index a41cfba..0992ee9 100644 --- a/packages/plugin/src/config.js +++ b/packages/plugin/src/config.js @@ -362,10 +362,16 @@ export const collectConfigWarnings = () => { if (!config.renderNow.header) { add('warn', 'renderNow.header', 'renderNow.enabled but renderNow.header is empty — on-demand render is disabled'); } else if (!config.renderNow.token) { + // Inert, not open: isRenderNowAuthorized fails closed without a token. Still worth + // reporting, because the operator asked for a feature that is not actually on — and + // naming the unresolved variable is the difference between a five-second fix and a hunt. + const { valueEnv } = config.renderNow; add( 'warn', 'renderNow.token', - `renderNow ENABLED WITHOUT A TOKEN — any client sending "${config.renderNow.header}" can force cache/origin-bypassing renders (DoS risk); set renderNow.token or renderNow.valueEnv` + valueEnv + ? `renderNow.enabled is true but renderNow.valueEnv ("${valueEnv}") is not set in the environment and no renderNow.token is configured — renderNow is DISABLED (the levers fail closed rather than authorizing anyone)` + : 'renderNow.enabled is true but no renderNow.token is configured — renderNow is DISABLED (the levers fail closed rather than authorizing anyone); set renderNow.token or renderNow.valueEnv' ); } } diff --git a/packages/plugin/src/configSchema.js b/packages/plugin/src/configSchema.js index 93880fd..4b4c7ee 100644 --- a/packages/plugin/src/configSchema.js +++ b/packages/plugin/src/configSchema.js @@ -289,17 +289,27 @@ export const configSchema = group('Prerender plugin configuration.', { 'So `defaultMissMode: prerender` + no Cache-Control = "serve cache, else render now" ' + '(warm-on-demand); adding `Cache-Control: no-cache` = "always render fresh now".', { - enabled: option(false, 'Enable the on-demand render levers.'), + enabled: option( + false, + 'Enable the on-demand render levers. Enabling is necessary but not sufficient — a non-empty ' + + '`token` (or a `valueEnv` that resolves to one) is also required, so this cannot open the ' + + 'levers on its own.' + ), header: option( 'x-harper-render-now', - 'Request header that authorizes the on-demand levers. Authorization is gated on its presence; ' + - 'when a `token` is set the header VALUE must equal it.' + 'Request header that authorizes the on-demand levers. The header VALUE must equal the ' + + 'configured `token`; presence alone never authorizes.' ), token: option( '', - 'Expected value of `header`. An empty token leaves the feature unauthenticated (any client ' + - 'sending the header can force renders — a DoS vector), which is warned about at config-apply ' + - 'time.', + 'Expected value of `header`. **Required** — there is no unauthenticated mode: an empty token ' + + 'leaves renderNow DISABLED (the levers stay off even when `enabled` is true) rather than ' + + 'authorizing anyone who sends the header, and is reported at config-apply time.\n\n' + + 'This fails CLOSED deliberately. The levers let a caller bypass the served cache and force ' + + 'a synchronous render that occupies the request for up to `timeoutMs`, so on a path that ' + + 'takes public crawler traffic an absent or unresolved token must not degrade to "authorize ' + + 'everyone". Prefer `valueEnv` so the secret stays out of config.yaml, and never commit a ' + + 'guessable placeholder — a value like "true" is not meaningfully better than none.', { secret: true } ), valueEnv: option( diff --git a/packages/plugin/src/util/renderNow.js b/packages/plugin/src/util/renderNow.js index b78d30a..1961a69 100644 --- a/packages/plugin/src/util/renderNow.js +++ b/packages/plugin/src/util/renderNow.js @@ -29,10 +29,16 @@ export const resolveServingPolicy = (routeClass, method, headers) => { /** * Whether a request is an authorized on-demand render ("render now") request. * - * The feature must be enabled and a header name configured; the request must - * carry that header. When a `token` is configured the header value must equal it - * (the shared secret gate). When no token is configured, mere presence of the - * header authorizes — the feature is then unauthenticated (see config warning). + * Requires the feature enabled, a header name configured, AND a non-empty token; + * the request must carry that header with a value equal to the token. + * + * This fails CLOSED on a missing token rather than treating "no token" as + * "authorize anyone". The levers bypass the served cache and can occupy a request + * for up to `timeoutMs` forcing a synchronous render, so on a path that takes + * public crawler traffic the unauthenticated reading is a DoS lever, not a + * convenience. It is also the state a misconfiguration lands in: `valueEnv` + * pointing at an unset variable leaves `token` at its empty default, so the + * permissive reading would turn a typo in a variable name into an open door. * * `headers` is anything with a `.get(name)` accessor (Harper request headers or a * `Headers` instance). An unauthorized-but-present header returns false so the @@ -41,10 +47,10 @@ export const resolveServingPolicy = (routeClass, method, headers) => { */ export const isRenderNowAuthorized = (headers) => { const { enabled, header, token } = config.renderNow; - if (!enabled || !header) return false; + if (!enabled || !header || !token) return false; const value = headers.get(header); if (value === null || value === undefined) return false; - return token ? value === token : true; + return value === token; }; /** diff --git a/packages/plugin/test/config.test.js b/packages/plugin/test/config.test.js index a06fa6f..b36d968 100644 --- a/packages/plugin/test/config.test.js +++ b/packages/plugin/test/config.test.js @@ -141,6 +141,34 @@ test('prefix mode is not held to the prerender-route requirement', () => { assert.equal(findingKeys().includes('ingress.routes'), false); }); +test('renderNow enabled without a token reports the feature as disabled', () => { + // The warning is the operator's only signal that a feature they switched on is not actually + // running, so it has to mirror the runtime gate: say DISABLED, not "DoS risk". The old wording + // would send someone hunting an exposure that no longer exists, and anything that reads as + // "enabled" would hide the fact that nothing is on. + applyOptions({ renderNow: { enabled: true } }); + const finding = collectConfigWarnings().find((f) => f.key === 'renderNow.token'); + assert.ok(finding, 'expected a renderNow.token finding'); + assert.match(finding.message, /DISABLED/); + assert.match(finding.message, /fail closed/); +}); + +test('an unresolved renderNow valueEnv is named in the warning', () => { + // Naming the variable is the difference between a five-second fix and a hunt: config.js only + // assigns from the environment when the variable is set, so a typo silently leaves token empty. + delete process.env.__TEST_RENDER_NOW_ABSENT; + applyOptions({ renderNow: { enabled: true, valueEnv: '__TEST_RENDER_NOW_ABSENT' } }); + const finding = collectConfigWarnings().find((f) => f.key === 'renderNow.token'); + assert.ok(finding, 'expected a renderNow.token finding'); + assert.match(finding.message, /__TEST_RENDER_NOW_ABSENT/); + assert.match(finding.message, /DISABLED/); +}); + +test('renderNow with a token reports nothing', () => { + applyOptions({ renderNow: { enabled: true, token: 'a-real-secret' } }); + assert.equal(findingKeys().includes('renderNow.token'), false); +}); + test('applyOptions sources the security token from valueEnv (overriding the literal)', () => { process.env.__TEST_PR_TOKEN = 'env-secret'; try { diff --git a/packages/plugin/test/renderNow.test.js b/packages/plugin/test/renderNow.test.js index 9ab47c5..7f8fc79 100644 --- a/packages/plugin/test/renderNow.test.js +++ b/packages/plugin/test/renderNow.test.js @@ -38,13 +38,40 @@ test('isRenderNowAuthorized requires the header value to match the token', () => assert.equal(isRenderNowAuthorized(headersWith({ 'x-harper-render-now': 'wrong' })), false); }); -test('isRenderNowAuthorized authorizes on presence when no token is configured', () => { +test('isRenderNowAuthorized fails CLOSED when no token is configured', () => { + // Previously presence alone authorized here, which made an absent token an open door on a + // path that takes public crawler traffic: any caller could skip the cache and force a + // synchronous render occupying the request for up to timeoutMs. Enabling is now necessary + // but not sufficient — without a token the levers are inert. applyOptions({ renderNow: { enabled: true } }); - assert.equal(isRenderNowAuthorized(headersWith({ 'x-harper-render-now': 'anything' })), true); - assert.equal(isRenderNowAuthorized(headersWith({ 'x-harper-render-now': '' })), true); + assert.equal(config.renderNow.token, ''); + assert.equal(isRenderNowAuthorized(headersWith({ 'x-harper-render-now': 'anything' })), false); + assert.equal(isRenderNowAuthorized(headersWith({ 'x-harper-render-now': '' })), false); assert.equal(isRenderNowAuthorized(headersWith({})), false); }); +test('an unresolved valueEnv leaves the levers inert, not open', () => { + // The misconfiguration this exists for: config.js only assigns from the environment when the + // variable is actually set, so a typo in the variable name leaves token at its empty default. + // Under the old permissive reading that typo silently became an open door. + delete process.env.RENDER_NOW_TOKEN_ABSENT; + applyOptions({ renderNow: { enabled: true, valueEnv: 'RENDER_NOW_TOKEN_ABSENT' } }); + assert.equal(config.renderNow.token, ''); + assert.equal(isRenderNowAuthorized(headersWith({ 'x-harper-render-now': 'anything' })), false); +}); + +test('resolveServingPolicy grants no levers when the token is missing', () => { + // The end-to-end consequence: a miss must proxy the origin as normal rather than forcing a + // render, and Cache-Control must not be honored as a cache skip. + applyOptions({ renderNow: { enabled: true, defaultMissMode: 'prerender' } }); + const policy = resolveServingPolicy( + PRERENDER, + 'GET', + headersWith({ 'x-harper-render-now': 'anything', 'cache-control': 'no-cache' }) + ); + assert.deepEqual(policy, { skipCache: false, missMode: 'origin' }); +}); + test('isRenderNowAuthorized honors a custom header name', () => { applyOptions({ renderNow: { enabled: true, header: 'x-acme-render', token: 't' } }); assert.equal(isRenderNowAuthorized(headersWith({ 'x-acme-render': 't' })), true);