From 8bff94eeebcf38f532d8acb0933388be4a24af34 Mon Sep 17 00:00:00 2001 From: Joe Date: Wed, 5 Aug 2026 14:40:57 -0400 Subject: [PATCH 1/2] fix(plugin): renderNow fails closed without a token; v0.32.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `isRenderNowAuthorized` treated "no token configured" as "authorize anyone who sends the header": return token ? value === token : true; The schema documented that as an intentional unauthenticated mode, warned about at config-apply time. On a path that takes public crawler traffic it is a DoS lever, not a convenience: an authorized GET can skip the served cache and force a synchronous render that occupies the request for up to `timeoutMs` (default 30s). It is also the state a misconfiguration lands in, which is what makes the permissive reading dangerous rather than merely lax. `config.js` assigns from the environment only when the variable is actually set: if (renderNowEnv && process.env[renderNowEnv]) { ... } so `valueEnv` naming a variable that is unset — a typo, or an env that was never provisioned — leaves `token` at its empty default. Under the old reading that typo silently became an open door, and the failure mode of a secret-management mistake should never be "no secret required". Now: enabling is necessary but not sufficient. Without a non-empty token the levers are inert, and the config warning says so — it previously reported a "DoS risk" that can no longer occur, which would send someone hunting an exposure that does not exist. The warning also names an unresolved `valueEnv` variable, which is the difference between a five-second fix and a hunt. BREAKING for anyone deliberately running enabled-with-no-token to leave the levers open to any caller. That configuration was documented as a DoS vector and warned about, so it is being withdrawn rather than preserved; such deployments must now set `renderNow.token` (or a `valueEnv` that resolves) to keep the levers working. The three behavioral tests were each verified to fail with the change reverted, so they guard the semantics rather than merely restating them. Co-Authored-By: Claude Opus 5 (1M context) --- package-lock.json | 2 +- packages/plugin/package.json | 2 +- packages/plugin/src/config.js | 8 ++++++- packages/plugin/src/configSchema.js | 22 ++++++++++++----- packages/plugin/src/util/renderNow.js | 18 +++++++++----- packages/plugin/test/config.test.js | 27 +++++++++++++++++++++ packages/plugin/test/renderNow.test.js | 33 +++++++++++++++++++++++--- 7 files changed, 94 insertions(+), 18 deletions(-) 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..29a209c 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 but renderNow.valueEnv ("${valueEnv}") is not set in the environment and no renderNow.token is configured — the on-demand levers stay INERT (they fail closed rather than authorizing anyone)` + : 'renderNow.enabled but no renderNow.token is configured — the on-demand levers stay INERT (they 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..e562e22 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 ' + + 'makes the feature inert (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..b53781b 100644 --- a/packages/plugin/test/config.test.js +++ b/packages/plugin/test/config.test.js @@ -141,6 +141,33 @@ 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 that the levers are inert', () => { + // The warning is the operator's only signal that a feature they switched on is not actually + // running. It must say inert, not "DoS risk" — the levers fail closed now, so the old wording + // would send someone hunting for an exposure that no longer exists. + 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, /INERT/); + 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, /INERT/); +}); + +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); From 6ac0d55aba9e746d67879bba3df57a22076c2e2e Mon Sep 17 00:00:00 2001 From: Joe Date: Wed, 5 Aug 2026 14:46:58 -0400 Subject: [PATCH 2/2] fix(plugin): warning reports renderNow as DISABLED, mirroring the runtime gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback: a config warning must not read as though the feature is running. The message said "renderNow.enabled but ... stay INERT", which named the contradiction but left the reader to infer the state. It now says renderNow is DISABLED outright, while keeping "enabled is true but" so the contradiction is still diagnosable — someone who set enabled: true needs to see why that did not take effect, not just that something is off. Aligned the schema description to the same word so the docs and the log agree. Co-Authored-By: Claude Opus 5 (1M context) --- packages/plugin/src/config.js | 4 ++-- packages/plugin/src/configSchema.js | 2 +- packages/plugin/test/config.test.js | 11 ++++++----- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/packages/plugin/src/config.js b/packages/plugin/src/config.js index 29a209c..0992ee9 100644 --- a/packages/plugin/src/config.js +++ b/packages/plugin/src/config.js @@ -370,8 +370,8 @@ export const collectConfigWarnings = () => { 'warn', 'renderNow.token', valueEnv - ? `renderNow.enabled but renderNow.valueEnv ("${valueEnv}") is not set in the environment and no renderNow.token is configured — the on-demand levers stay INERT (they fail closed rather than authorizing anyone)` - : 'renderNow.enabled but no renderNow.token is configured — the on-demand levers stay INERT (they fail closed rather than authorizing anyone); set renderNow.token or renderNow.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 e562e22..4b4c7ee 100644 --- a/packages/plugin/src/configSchema.js +++ b/packages/plugin/src/configSchema.js @@ -303,7 +303,7 @@ export const configSchema = group('Prerender plugin configuration.', { token: option( '', 'Expected value of `header`. **Required** — there is no unauthenticated mode: an empty token ' + - 'makes the feature inert (the levers stay off even when `enabled` is true) rather than ' + + '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 ' + diff --git a/packages/plugin/test/config.test.js b/packages/plugin/test/config.test.js index b53781b..b36d968 100644 --- a/packages/plugin/test/config.test.js +++ b/packages/plugin/test/config.test.js @@ -141,14 +141,15 @@ 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 that the levers are inert', () => { +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. It must say inert, not "DoS risk" — the levers fail closed now, so the old wording - // would send someone hunting for an exposure that no longer exists. + // 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, /INERT/); + assert.match(finding.message, /DISABLED/); assert.match(finding.message, /fail closed/); }); @@ -160,7 +161,7 @@ test('an unresolved renderNow valueEnv is named in the warning', () => { 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, /INERT/); + assert.match(finding.message, /DISABLED/); }); test('renderNow with a token reports nothing', () => {