Name authentication middleware and add http.securityHeaders config#1568
Name authentication middleware and add http.securityHeaders config#1568kriszyp wants to merge 3 commits into
Conversation
Enablers for an upcoming WAF middleware component: - Register the authentication middleware under the name 'authentication' (both the app-worker registration in security/auth.ts and the operations-API registrations in server/operationsServer.ts), so other middleware can order relative to it with before/after in server.http() options. - Add an opt-in http.securityHeaders config block (map of header name -> value) appended to every REST/app HTTP response via universalHeaders, with hot-reload support. Header names/values are validated with node:http validateHeaderName/validateHeaderValue; invalid entries are logged and skipped. Hot reload tracks the entries this feature owns and splices only those, so entries pushed by other components are preserved. No defaults are applied when the block is absent (no behavior change for existing deployments). Note: the operations API is served directly by Fastify's own http.Server and does not flow through the Harper-native onRequest handler, so universalHeaders (and thus securityHeaders) do not apply to operations API responses. Documented in DESIGN.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces the http.securityHeaders configuration option, allowing users to define custom response headers (such as X-Frame-Options) that are appended to REST/app HTTP responses. It includes robust hot-reloading logic to update these headers dynamically without clobbering headers registered by other components, alongside comprehensive unit and integration tests. The review feedback suggests two key improvements: using Object.entries() instead of a for...in loop to avoid iterating over inherited properties, and wrapping test cleanup in a try...finally block to prevent test pollution in case of assertion failures.
| for (const name in securityHeaders) { | ||
| const value = '' + securityHeaders[name]; |
There was a problem hiding this comment.
Using a for...in loop without a hasOwnProperty check can lead to iterating over inherited properties if the prototype chain is modified or polluted. It is safer and more idiomatic to use Object.entries() to iterate over the object's own enumerable properties.
| for (const name in securityHeaders) { | |
| const value = '' + securityHeaders[name]; | |
| for (const [name, rawValue] of Object.entries(securityHeaders)) { | |
| const value = '' + rawValue; |
| const scope = mockScope({ securityHeaders: { 'X-Frame-Options': 'SAMEORIGIN' } }); | ||
| handleApplication(scope); | ||
| assert.ok(universalHeaders.some(([name]) => name === 'X-Frame-Options')); | ||
| assert.ok(universalHeaders.includes(foreignEntry)); | ||
|
|
||
| // Hot-reload with a different config: old owned entry should be gone, new one present, | ||
| // and the foreign entry must survive untouched. | ||
| scope._reload({ securityHeaders: { 'X-Content-Type-Options': 'nosniff' } }); | ||
| assert.ok(!universalHeaders.some(([name]) => name === 'X-Frame-Options')); | ||
| assert.ok(universalHeaders.some(([name]) => name === 'X-Content-Type-Options')); | ||
| assert.ok(universalHeaders.includes(foreignEntry)); | ||
|
|
||
| // Clean up the foreign entry so it doesn't leak into other tests. | ||
| const index = universalHeaders.indexOf(foreignEntry); | ||
| if (index !== -1) universalHeaders.splice(index, 1); |
There was a problem hiding this comment.
If any of the assertions fail, the cleanup code at the end of the test will be skipped, causing the foreignEntry to leak into subsequent tests. Wrapping the test logic in a try...finally block ensures that the cleanup always runs, preventing test pollution and potential flakiness.
try {
const scope = mockScope({ securityHeaders: { 'X-Frame-Options': 'SAMEORIGIN' } });
handleApplication(scope);
assert.ok(universalHeaders.some(([name]) => name === 'X-Frame-Options'));
assert.ok(universalHeaders.includes(foreignEntry));
// Hot-reload with a different config: old owned entry should be gone, new one present,
// and the foreign entry must survive untouched.
scope._reload({ securityHeaders: { 'X-Content-Type-Options': 'nosniff' } });
assert.ok(!universalHeaders.some(([name]) => name === 'X-Frame-Options'));
assert.ok(universalHeaders.some(([name]) => name === 'X-Content-Type-Options'));
assert.ok(universalHeaders.includes(foreignEntry));
} finally {
// Clean up the foreign entry so it doesn't leak into other tests.
const index = universalHeaders.indexOf(foreignEntry);
if (index !== -1) universalHeaders.splice(index, 1);
}| testUtils.preTestPrep(); | ||
|
|
||
| const assert = require('assert'); | ||
| const sinon = require('sinon'); |
There was a problem hiding this comment.
Suggestion (non-blocking): AGENTS.md says "do not add new uses of sinon or rewire" for new test files. The two tests that use sandbox.stub(harperLogger, 'error') (the "rejects invalid header names" and "rejects invalid header values" cases) already assert the meaningful side effects — the bad entry is absent from universalHeaders, and the valid entry is present. Dropping errorStub.calledOnce and removing the sinon import/sandbox would bring this file into line with the house style without losing coverage.
|
Reviewed; no blockers found. |
Addresses adversarial-review findings on the initial implementation: - handlesHeaders responses (e.g. static's send() stream) now receive universal headers: pre-set on nodeResponse (Node) / the pipe-shim responseHeaders (Bun) so streams can still override their own names. - Thrown-error responses now receive universal headers on both the Node onError path and the Bun catch path. - Precedence is now app-wins: universal headers are defaults and never override a header the application/route already set (has/set checks; works with both Harper Headers and web Headers responses). - Only the first (root) http scope owns securityHeaders: an app config.yaml with an http: block can no longer wipe root-configured headers on load or hot-reload. - Non-object securityHeaders values are rejected instead of for-in iterating (a string would yield digit-named headers). - DESIGN.md ops-API rationale corrected: ops requests DO flow through the Harper-native requestHandler and cascade to Fastify; the real reason headers are absent is that the main thread loads components with resources.isWorker=false, so http's handleApplication never populates the main thread's universalHeaders (threads:0 mode would carry them — benign). - Integration tests now cover the 200 writeHead path, static (handlesHeaders) path, thrown-error path, and app-wins precedence via a dedicated fixture app, plus the existing 404-cascade and opt-in-absent cases. All new per-response loops are guarded by universalHeaders.length so unconfigured deployments skip them entirely. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…array securityHeaders - Bun thrown-error responses now carry error-provided headers (e.g. WWW-Authenticate) merged with universal headers, error headers winning — mirrors the Node onError path's toWriteHeadHeaders semantics. (Previously the Bun catch dropped error.headers entirely, a pre-existing gap made worth fixing while touching this path.) - applySecurityHeaders rejects arrays (typeof object, would iterate indices). - Document the known limitation that a handler calling writeHead synchronously before returning handlesHeaders cannot receive universal headers. Refuted (no change): the claim that the Bun handlesHeaders path clobbers app-set headers — responseHeaders is empty at that point and the stream's later setHeader calls overwrite, so app/stream wins by ordering. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Cross-model review disposition (Gemini via agy, full diff):
An independent fresh-context review earlier drove commit 6300e32 (response-path coverage + app-wins precedence). — KrAIs (Claude) 🤖 Generated with Claude Code |
Summary
Two small core enablers for an upcoming WAF middleware component:
Explicitly named authentication middleware — honesty note: this is contract-hardening, not new behavior. Middleware entries already default their name to the registering component's name (
name: options?.name ?? getComponentName()inserver/http.ts), and the auth plugin loads under theauthenticationconfig key — sobefore: 'authentication'/after: 'authentication'ordering (used today by REST, MQTT, GraphQL, MCP, andstatic.ts) already resolved correctly. This change makes the name an explicit, test-locked contract so a future rename of the config/registry key can't silently break every consumer that targets'authentication'. No chain-order behavior changes.The operations-API registrations in
server/operationsServer.tsare a genuine (if inert) name change: they previously defaulted to theoperationsApicomponent name and now register asauthentication. Nothing in-repo targets'operationsApi'as a middleware name, and those chains are bypassed for real ops traffic (see caveat below), so this is forward-compat consistency only.http.securityHeadersconfig block — a zero-code way for ops to add response headers likeX-Frame-Options/X-Content-Type-Options:Applied on app ports across all response paths of the Harper-native request handler: the normal writeHead path,
handlesHeadersstreams (static files — where thesend()stream writes its own headers, universal headers are pre-set so the stream can still override its own names), thrown-error responses, and the Fastifystatus === -1cascade. Node and Bun handlers both covered.X-Frame-Options: DENYis not loosened by a configuredSAMEORIGIN).node:http'svalidateHeaderName/validateHeaderValue; invalid entries (and non-object block values) are logged and skipped, never thrown.scope.options.on('change')path; the feature tracks exactly the entries it pushed intouniversalHeadersand splices only those, preserving entries pushed by other components. Only the first (root) http scope owns the config — an applicationconfig.yamlwith anhttp:block cannot wipe root-configured headers.universalHeaders.lengthso unconfigured deployments skip them.Where to look
server/http.ts—applySecurityHeaders()(ownership-tracked splice, root-scope guard) and the four response-path application sites.resources.isWorker = false, so http'shandleApplicationnever populates the main thread's per-threaduniversalHeaders. Withthreads: 0the ops API shares a worker and would carry them (benign). Detailed in DESIGN.md. Flagging in case ops-API parity is wanted later.server/operationsServer.ts— ops-API auth registrations getname: 'authentication'for consistency (forward-compat naming, not a behavior change).Tests
unitTests/server/securityHeaders.test.js(new): apply/coerce/validate/non-object-reject/hot-reload-swap semantics, "don't clobber other components' entries", and root-scope ownership vs a second app scope.unitTests/server/middlewareChain.test.js: new case proving abefore: 'authentication'entry executes before auth regardless of registration order.unitTests/security/auth.test.js: assertshandleApplicationregisters the auth listener undername: 'authentication'.unitTests/validation/configValidator.test.js: 4 new schema cases (absent/map/coercible/rejected).integrationTests/server/security-headers.test.ts+ fixture app (new): real Harper instance — headers on 200 REST, 404 cascade, static-file (handlesHeaders), and thrown-error responses; app-set header wins over config; absent config adds nothing.Generated by KrAIs (Claude, Fable 5 model) via Claude Code.
🤖 Generated with Claude Code