From 5db444ff8941f5e62c51fe3cb087da2c855b3af1 Mon Sep 17 00:00:00 2001 From: hdbjeff Date: Mon, 31 Aug 2026 10:44:59 -0700 Subject: [PATCH 01/12] fix: defer unrecognized app-port credential rejection until route ownership is known (#2418) Harper's app-port `authentication` middleware resolved `Authorization` before route matching, so a syntactically valid credential it did not recognize terminated the chain with a 401 and downstream route ownership never ran. An application could not both let Harper own its native routes and let its own routes receive their own credential scheme. A rejection is now recorded rather than answered. An unrecognized credential leaves `request.user` unset, leaves the inbound header byte-for-byte intact, and records request-local state behind a module-private Symbol. Any layer that establishes Harper owns the route settles that state first and renders the same generic 401 as before, so Harper-owned routes behave identically, protected or public. Only a URL no Harper route owns carries the original header on to an application catch-all. Internal authentication faults are never deferred: `isCredentialRejection` accepts only a 4xx-carrying error, and `validateToken` no longer masks a key-read or storage failure as `invalid token`. The operations API never defers, since it owns every route. No path exemption list, carrier header, credential rename, or pre-auth stripping shim. Co-Authored-By: Claude Opus 5 (1M context) --- .../deferred-credential-rejection.test.ts | 183 +++++++++++ .../appCatchAll.js | 18 ++ .../deferred-credential-rejection/config.yaml | 12 + .../resources.js | 7 + .../schema.graphql | 14 + security/auth.ts | 46 ++- security/deferredAuthentication.ts | 72 +++++ security/tokenAuthentication.ts | 24 +- server/DESIGN.md | 43 ++- server/REST.ts | 9 + server/graphqlQuerying.ts | 7 + .../security/authCredentialDeferral.test.js | 294 ++++++++++++++++++ .../security/deferredAuthentication.test.js | 121 +++++++ 13 files changed, 834 insertions(+), 16 deletions(-) create mode 100644 integrationTests/security/deferred-credential-rejection.test.ts create mode 100644 integrationTests/security/fixtures/deferred-credential-rejection/appCatchAll.js create mode 100644 integrationTests/security/fixtures/deferred-credential-rejection/config.yaml create mode 100644 integrationTests/security/fixtures/deferred-credential-rejection/resources.js create mode 100644 integrationTests/security/fixtures/deferred-credential-rejection/schema.graphql create mode 100644 security/deferredAuthentication.ts create mode 100644 unitTests/security/authCredentialDeferral.test.js create mode 100644 unitTests/security/deferredAuthentication.test.js diff --git a/integrationTests/security/deferred-credential-rejection.test.ts b/integrationTests/security/deferred-credential-rejection.test.ts new file mode 100644 index 0000000000..720f6c9b10 --- /dev/null +++ b/integrationTests/security/deferred-credential-rejection.test.ts @@ -0,0 +1,183 @@ +/** + * End-to-end proof for #2418: an app-port credential Harper does not recognize is not rejected + * until route ownership is known. + * + * The chain under test is the real one — `authentication -> rest -> application catch-all` — served + * by a real Harper instance. On the pre-fix revision `security/auth.ts` answered 401 while parsing + * the credential, so every "reaches the application catch-all" assertion below fails there. + * + * Reproduction: + * npm run test:integration -- "integrationTests/security/deferred-credential-rejection.test.ts" + */ +import { suite, test, before, after } from 'node:test'; +import { equal, ok } from 'node:assert'; +import { resolve } from 'node:path'; +import { setTimeout as sleep } from 'node:timers/promises'; + +import { setupHarperWithFixture, teardownHarper, type ContextWithHarper } from '@harperfast/integration-testing'; +import { createApiClient } from '../apiTests/utils/client.mjs'; + +const FIXTURE_PATH = resolve(import.meta.dirname, 'fixtures/deferred-credential-rejection'); +const skipSuite = process.env.HARPER_RUNTIME === 'bun' || process.platform === 'win32'; + +/** A WordPress Application Password, base64'd exactly as WordPress sends it — spaces and all. */ +const WORDPRESS_BASIC = `Basic ${Buffer.from('wordpress:abcd efgh ijkl mnop qrst uvwx').toString('base64')}`; +/** A session token belonging to the downstream application, not to Harper. */ +const DOWNSTREAM_BEARER = 'Bearer eyJhbGciOiJIUzI1NiJ9.d29vLXNlc3Npb24.not-a-harper-token'; + +/** A URL no Harper route owns — the shape WooCommerce's REST API uses. */ +const APP_ROUTE = '/wp-json/wc/v3/products'; +/** A Harper-owned resource that requires an authenticated principal. */ +const PROTECTED_ROUTE = '/Ledger/'; +/** A Harper-owned resource that an anonymous caller may read. */ +const PUBLIC_ROUTE = '/PublicNotice/'; + +suite( + '#2418 unrecognized app-port credentials defer until route ownership', + { skip: skipSuite }, + (ctx: ContextWithHarper) => { + let client: ReturnType; + let restURL = ''; + let adminAuthorization = ''; + + /** Issues a raw request so the exact Authorization header under test reaches the wire unchanged. */ + async function get(pathname: string, authorization?: string) { + const response = await fetch(`${restURL}${pathname}`, { + headers: authorization ? { Authorization: authorization } : {}, + }); + const text = await response.text(); + let body: any; + try { + body = JSON.parse(text); + } catch { + body = text; + } + return { status: response.status, body, text }; + } + + before(async () => { + await setupHarperWithFixture(ctx, FIXTURE_PATH, { + config: {}, + env: { + HARPER_BUILTIN_COMPONENTS: + 'deferredAuthAppCatchAll=@/integrationTests/security/fixtures/deferred-credential-rejection/appCatchAll.js', + }, + }); + client = createApiClient(ctx.harper); + restURL = ctx.harper.httpURL; + adminAuthorization = client.headers.Authorization; + + const deadline = Date.now() + 30_000; + while (Date.now() < deadline) { + const probe = await get(PUBLIC_ROUTE, adminAuthorization); + if (probe.status !== 404) break; + await sleep(250); + } + }); + + after(async () => { + await teardownHarper(ctx); + }); + + test('the application catch-all is mounted after rest, not before it', async () => { + // If the catch-all had been hoisted ahead of `rest`, it would claim this Harper-owned route + // too — which is exactly the trade the issue refuses to make. + const owned = await get(PUBLIC_ROUTE, adminAuthorization); + equal(owned.status, 200); + ok(owned.body?.servedBy !== 'application-catch-all', 'rest must own a Harper resource route'); + + const unowned = await get(APP_ROUTE); + equal(unowned.status, 200); + equal(unowned.body.servedBy, 'application-catch-all'); + }); + + test('valid Harper Basic credentials still authenticate a protected Harper resource', async () => { + const response = await get(PROTECTED_ROUTE, adminAuthorization); + + equal(response.status, 200, `expected the admin to read ${PROTECTED_ROUTE}: ${response.text}`); + }); + + test('an unrecognized WordPress Basic credential reaches the catch-all byte-for-byte', async () => { + const response = await get(APP_ROUTE, WORDPRESS_BASIC); + + equal(response.status, 200, `expected the catch-all to answer: ${response.text}`); + equal(response.body.servedBy, 'application-catch-all'); + // No rename, no carrier header, no stripping — the application gets what the client sent. + equal(response.body.authorization, WORDPRESS_BASIC); + // And Harper attached no principal on the way through. + equal(response.body.harperUser, null); + }); + + test('a downstream-owned Bearer token gets the same treatment as Basic', async () => { + const response = await get(APP_ROUTE, DOWNSTREAM_BEARER); + + equal(response.status, 200, `expected the catch-all to answer: ${response.text}`); + equal(response.body.authorization, DOWNSTREAM_BEARER); + equal(response.body.harperUser, null); + }); + + test('a protected Harper resource rejects an unrecognized credential instead of falling through', async () => { + const response = await get(PROTECTED_ROUTE, WORDPRESS_BASIC); + + equal(response.status, 401, `expected a generic unauthorized: ${response.text}`); + ok(response.body?.servedBy !== 'application-catch-all', 'a Harper-owned route must not reach the application'); + }); + + test('a protected Harper resource rejects an invalid Harper Bearer token', async () => { + const response = await get(PROTECTED_ROUTE, DOWNSTREAM_BEARER); + + equal(response.status, 401, `expected a generic unauthorized: ${response.text}`); + ok(response.body?.servedBy !== 'application-catch-all'); + }); + + test('an unrecognized credential never downgrades a Harper-owned route to public access', async () => { + // Anonymous callers may read PublicNotice, so if a deferred credential simply became + // "anonymous" this would return the record. Harper owns the route, so Harper decides it. + const anonymous = await get(PUBLIC_ROUTE); + equal(anonymous.status, 200, `PublicNotice must stay anonymously readable: ${anonymous.text}`); + ok(anonymous.body?.servedBy !== 'application-catch-all'); + + const withUnknownCredential = await get(PUBLIC_ROUTE, WORDPRESS_BASIC); + equal(withUnknownCredential.status, 401, `expected a generic unauthorized: ${withUnknownCredential.text}`); + ok(withUnknownCredential.body?.servedBy !== 'application-catch-all'); + }); + + test('a request with no credentials is unaffected on both owned and unowned routes', async () => { + const owned = await get(PUBLIC_ROUTE); + equal(owned.status, 200); + + const unowned = await get(APP_ROUTE); + equal(unowned.status, 200); + equal(unowned.body.servedBy, 'application-catch-all'); + equal(unowned.body.authorization, null); + equal(unowned.body.harperUser, null); + }); + + test('a deferred-credential response is kept out of shared caches', async () => { + // The application answered using the header Harper passed through, so the response varies by + // credential even though no Harper principal was resolved (#1565's identity floor). + const response = await fetch(`${restURL}${APP_ROUTE}`, { headers: { Authorization: WORDPRESS_BASIC } }); + + equal(response.status, 200); + ok( + response.headers.get('vary')?.toLowerCase().includes('authorization'), + `expected Vary: Authorization, got ${response.headers.get('vary')}` + ); + ok( + /private|no-store/i.test(response.headers.get('cache-control') ?? ''), + `expected a private cache scope, got ${response.headers.get('cache-control')}` + ); + }); + + test('the operations API still rejects an unrecognized credential in place', async () => { + // Every operations route is Harper-owned, so there is nothing to defer to and nothing changes. + const response = await fetch(ctx.harper.operationsAPIURL, { + method: 'POST', + headers: { 'Authorization': WORDPRESS_BASIC, 'Content-Type': 'application/json' }, + body: JSON.stringify({ operation: 'describe_all' }), + }); + + equal(response.status, 401); + }); + } +); diff --git a/integrationTests/security/fixtures/deferred-credential-rejection/appCatchAll.js b/integrationTests/security/fixtures/deferred-credential-rejection/appCatchAll.js new file mode 100644 index 0000000000..1814b4db28 --- /dev/null +++ b/integrationTests/security/fixtures/deferred-credential-rejection/appCatchAll.js @@ -0,0 +1,18 @@ +// The application's own middleware, mounted after `rest` so Harper's route ownership gets first +// refusal. It claims whatever reaches it and reports the Authorization header it received, which is +// how the test proves the header arrived byte-for-byte and that no Harper principal was attached. +export function handleApplication(scope) { + scope.server.http( + async (request) => ({ + status: 200, + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ + servedBy: 'application-catch-all', + authorization: request.headers.get('authorization') ?? null, + harperUser: request.user?.username ?? null, + pathname: request.pathname, + }), + }), + { port: 'all', after: 'rest' } + ); +} diff --git a/integrationTests/security/fixtures/deferred-credential-rejection/config.yaml b/integrationTests/security/fixtures/deferred-credential-rejection/config.yaml new file mode 100644 index 0000000000..87ce6a9a8a --- /dev/null +++ b/integrationTests/security/fixtures/deferred-credential-rejection/config.yaml @@ -0,0 +1,12 @@ +# Regression fixture for #2418 — credential rejection is deferred until route ownership is known. +# +# `deferredAuthAppCatchAll` is wired in through HARPER_BUILTIN_COMPONENTS (see the test file); it +# must ALSO appear as a top-level component key here for componentLoader to load it. It registers +# an application middleware with `after: rest`, reproducing the ordering an application needs: +# authentication -> rest -> application catch-all. +graphqlSchema: + files: '*.graphql' +jsResource: + files: resources.js +rest: true +deferredAuthAppCatchAll: true diff --git a/integrationTests/security/fixtures/deferred-credential-rejection/resources.js b/integrationTests/security/fixtures/deferred-credential-rejection/resources.js new file mode 100644 index 0000000000..86be5177a4 --- /dev/null +++ b/integrationTests/security/fixtures/deferred-credential-rejection/resources.js @@ -0,0 +1,7 @@ +// PublicNotice is readable by anyone, including an unauthenticated caller. Everything else keeps +// Harper's default authorization. +export class PublicNotice extends tables.PublicNotice { + allowRead() { + return true; + } +} diff --git a/integrationTests/security/fixtures/deferred-credential-rejection/schema.graphql b/integrationTests/security/fixtures/deferred-credential-rejection/schema.graphql new file mode 100644 index 0000000000..9896761e74 --- /dev/null +++ b/integrationTests/security/fixtures/deferred-credential-rejection/schema.graphql @@ -0,0 +1,14 @@ +# Regression fixture for #2418. +# +# Ledger is a protected Harper-owned resource: only an authenticated principal may read it. +# PublicNotice is Harper-owned but readable anonymously, which is what makes "Harper owns it, so +# Harper decides it" testable independently of whether the route happens to be protected. +type Ledger @table @export { + id: ID @primaryKey + note: String +} + +type PublicNotice @table @export { + id: ID @primaryKey + message: String +} diff --git a/security/auth.ts b/security/auth.ts index 7b1d13778d..dc1db66967 100644 --- a/security/auth.ts +++ b/security/auth.ts @@ -13,6 +13,11 @@ const { user } = serverHandlers; import { Headers, addVaryHeader } from '../server/serverHelpers/Headers.ts'; import { convertToMS } from '../utility/common_utils.ts'; import { verifyCertificate } from './certificateVerification/index.ts'; +import { + deferCredentialRejection, + getDeferredCredentialRejection, + isCredentialRejection, +} from './deferredAuthentication.ts'; import { serializeMessage } from '../server/serverHelpers/contentTypes.ts'; const authLogger = forComponent('authentication'); const { debug } = authLogger; @@ -210,6 +215,7 @@ export async function authentication(request, nextHandler) { const strategy = authorization.slice(0, spaceIndex); const credentials = authorization.slice(spaceIndex + 1); let username, password; + let credentialRejection; try { switch (strategy) { case 'Basic': @@ -256,18 +262,33 @@ export async function authentication(request, nextHandler) { } } - return applyResponseHeaders({ - status: 401, - body: serializeMessage({ error: err.message }, request), - }); + // #2418: route ownership isn't known yet, so an ordinary credential rejection is not + // decided here. Two cases are still answered in-line rather than deferred: + // - an internal fault (unreadable JWT keys, storage failure, a bug), which must fail + // closed instead of letting an outage hand the request to application authorization; + // - the operations API, where Harper owns every route, so there is nothing to defer to. + if (request.isOperationsServer || !isCredentialRejection(err)) { + return applyResponseHeaders({ + status: 401, + body: serializeMessage({ error: err.message }, request), + }); + } + credentialRejection = err; } - authorizationCache.set(authorization, newUser); - if (LOG_AUTH_SUCCESSFUL) authAuditLog(newUser.username, AUTH_AUDIT_STATUS.SUCCESS, strategy); - // Shallow-clone so verifyPerms's `role.permission = fullRolePerms` reassignment - // doesn't mutate the just-stored cache entry (defense-in-depth). - if (newUser?.role) { - newUser = { ...newUser, role: { ...newUser.role, permission: { ...newUser.role.permission } } }; + if (credentialRejection) { + // Continue with no principal and the inbound Authorization header untouched. Any + // Harper-owned layer rejects this via assertNoDeferredCredentialRejection; only a + // request no Harper route owns reaches an application catch-all. + deferCredentialRejection(request, credentialRejection, strategy); + } else { + authorizationCache.set(authorization, newUser); + if (LOG_AUTH_SUCCESSFUL) authAuditLog(newUser.username, AUTH_AUDIT_STATUS.SUCCESS, strategy); + // Shallow-clone so verifyPerms's `role.permission = fullRolePerms` reassignment + // doesn't mutate the just-stored cache entry (defense-in-depth). + if (newUser?.role) { + newUser = { ...newUser, role: { ...newUser.role, permission: { ...newUser.role.permission } } }; + } } } @@ -376,7 +397,10 @@ export async function authentication(request, nextHandler) { // if we are rejecting the credentials (401, possibly rewritten to a login redirect); such a // response must never be stored by a shared cache and served to a different principal (#1565) const rejectedAuth = response?.status === 401 || wasUnauthorized; - const identityDependent = !!request.user || rejectedAuth; + // A deferred credential is still a credential this response was produced under — whoever + // owned the route saw the untouched Authorization header — so #1565's identity floor applies + // even though no Harper principal was resolved and the status may be a plain 200. + const identityDependent = !!request.user || rejectedAuth || !!getDeferredCredentialRejection(request); // with CORS enabled the response is origin-dependent — ACAO reflects the request Origin, and // its absence when no Origin was sent is origin-dependent too — so a shared cache must // partition on Origin either way (#1518) diff --git a/security/deferredAuthentication.ts b/security/deferredAuthentication.ts new file mode 100644 index 0000000000..a9d58b0457 --- /dev/null +++ b/security/deferredAuthentication.ts @@ -0,0 +1,72 @@ +import { ClientError } from '../utility/errors/hdbError.ts'; + +/** + * Request-local state recorded when `security/auth.ts` accepts a syntactically valid credential it + * cannot resolve to a Harper principal. It is deliberately not a header, not a `Request` field, and + * not enumerable: nothing outside this module can read, forge, or clear it, and it cannot reach the + * wire or a downstream application. + */ +const DEFERRED_CREDENTIAL_REJECTION = Symbol('harper.deferredCredentialRejection'); + +export type DeferredCredentialRejection = { + /** Status the authentication middleware would have returned in-line. Always 401. */ + status: number; + /** The rejection message that middleware would have carried, so the deferred response is identical. */ + message: string; + /** `Basic`, `Bearer`, or whatever scheme token preceded the credential. */ + strategy: string; +}; + +/** + * The status every credential rejection resolves to, whether it is answered in-line or deferred. + * `security/auth.ts` has always answered a rejected credential with 401 regardless of the + * underlying error's own `statusCode` (a 403 `token expired`, for instance), so pinning it here is + * what keeps a deferred rejection byte-identical to the in-line one it replaces. + */ +const CREDENTIAL_REJECTION_STATUS = 401; + +/** + * Distinguishes an ordinary credential rejection — a well-formed credential Harper does not + * recognize — from an internal authentication fault such as unreadable JWT keys, a storage failure, + * or a bug. + * + * Only the former may be deferred. Deferring a fault would let an outage quietly downgrade a Harper + * request into one an application's own authorization decides, so anything that is not positively + * identifiable as a client-side rejection fails closed. Harper's authentication errors carry a 4xx + * `statusCode` (`ClientError`); an unexpected error type, a bare `Error`, and a 5xx all fall through + * to `false`. + */ +export function isCredentialRejection(error: unknown): boolean { + const status = (error as { statusCode?: unknown; status?: unknown })?.statusCode ?? (error as any)?.status; + return typeof status === 'number' && status >= 400 && status < 500; +} + +/** + * Records that this request presented a credential Harper rejected, without deciding the request. + * The caller leaves `request.user` unset and the inbound `Authorization` header untouched. + */ +export function deferCredentialRejection(request: any, error: { message?: string }, strategy: string): void { + const deferred: DeferredCredentialRejection = { + status: CREDENTIAL_REJECTION_STATUS, + message: error?.message ?? 'Unauthorized', + strategy, + }; + request[DEFERRED_CREDENTIAL_REJECTION] = deferred; +} + +export function getDeferredCredentialRejection(request: any): DeferredCredentialRejection | undefined { + return request?.[DEFERRED_CREDENTIAL_REJECTION]; +} + +/** + * Called by a layer that has just established Harper owns the route being served. A credential the + * authentication middleware deferred is decided here — where ownership is finally known — and never + * travels past a Harper-owned route to an application catch-all. + * + * Throws the same `ClientError` the authentication middleware would have produced in-line, so an + * owning layer's existing error path renders the identical unauthorized response. + */ +export function assertNoDeferredCredentialRejection(request: any): void { + const deferred = getDeferredCredentialRejection(request); + if (deferred) throw new ClientError(deferred.message, deferred.status); +} diff --git a/security/tokenAuthentication.ts b/security/tokenAuthentication.ts index 56b7548618..636adf8548 100644 --- a/security/tokenAuthentication.ts +++ b/security/tokenAuthentication.ts @@ -428,18 +428,18 @@ async function validateToken(token: string, tokenType: string): Promise { return buildUserFromScopedToken(tokenVerified); } if (tokenVerified.sub !== tokenType) { - throw new Error('Invalid token'); + throw new ClientError(AUTHENTICATION_ERROR_MSGS.INVALID_TOKEN, HTTP_STATUS_CODES.UNAUTHORIZED); } // If a role is present, it means the token is not an operation token. The validation of // the token will happen in the respective function/component that uses the token. if (tokenVerified.role) { - throw new Error('Invalid token'); + throw new ClientError(AUTHENTICATION_ERROR_MSGS.INVALID_TOKEN, HTTP_STATUS_CODES.UNAUTHORIZED); } const user: any = await findAndValidateUser(tokenVerified.username, undefined, false); if (tokenType === TOKEN_TYPE.REFRESH && !password.validate(user.refresh_token, token)) { - throw new Error('Invalid token'); + throw new ClientError(AUTHENTICATION_ERROR_MSGS.INVALID_TOKEN, HTTP_STATUS_CODES.UNAUTHORIZED); } // Surfaced as `tokenOperations` rather than merged into role.permission.operations: that field @@ -457,11 +457,27 @@ async function validateToken(token: string, tokenType: string): Promise { if (err?.name === 'TokenExpiredError') { throw new ClientError(AUTHENTICATION_ERROR_MSGS.TOKEN_EXPIRED, HTTP_STATUS_CODES.FORBIDDEN); } + // Only a client-side rejection may be reported as one. Everything else here — unreadable JWT + // keys (a 500 from getJWTRSAKeys), a storage failure inside findAndValidateUser, a bug — + // propagates unmasked, because callers now distinguish a rejected credential from an internal + // authentication fault and only the former is deferred past route matching (#2418). Masking a + // fault as `invalid token` would let a key or storage outage read as an unknown credential. + if (!isTokenRejection(err)) throw err; throw new ClientError(AUTHENTICATION_ERROR_MSGS.INVALID_TOKEN, HTTP_STATUS_CODES.UNAUTHORIZED); } } +/** + * True when `err` says the presented token is not acceptable, rather than that Harper failed to + * evaluate it. Covers `jsonwebtoken`'s verification errors and Harper's own 4xx `ClientError`s. + */ +function isTokenRejection(err: any): boolean { + if (err?.name === 'JsonWebTokenError' || err?.name === 'NotBeforeError') return true; + const status = err?.statusCode ?? err?.status; + return typeof status === 'number' && status >= 400 && status < 500; +} + /** * Builds the request user for a verified scoped token from its embedded role. No hdb_user lookup: * the signed claims are the whole identity. The downgrade is re-applied here as defense-in-depth, @@ -470,7 +486,7 @@ async function validateToken(token: string, tokenType: string): Promise { function buildUserFromScopedToken(claims: JwtPayload): User { const embedded = (claims.role as { permission?: Record })?.permission; if (!embedded || typeof embedded !== 'object' || Array.isArray(embedded) || typeof claims.username !== 'string') { - throw new Error('Invalid token'); + throw new ClientError(AUTHENTICATION_ERROR_MSGS.INVALID_TOKEN, HTTP_STATUS_CODES.UNAUTHORIZED); } const permission: Record = { ...embedded, super_user: false, cluster_user: false }; // Hashed from the server-side downgraded clone (before the _expandedOperations Set is attached), diff --git a/server/DESIGN.md b/server/DESIGN.md index 63e490f9c8..8274569397 100644 --- a/server/DESIGN.md +++ b/server/DESIGN.md @@ -194,12 +194,53 @@ mirror because it only widens what an allowlist may _name_; enforcement stays on `REST.ts → http(request, nextHandler)` is the chief integration point: it takes a `Request`, asks the `Resources` registry for a match, builds a `RequestTarget`, and dispatches into the Resource class's static method. Cache headers are translated to `request.expiresAt` / `onlyIfCached` / `noCache` flags within the same function. +### Deferred credential rejection (#2418) + +`authentication` runs before route matching, so when it meets an `Authorization` header it cannot +resolve it does not yet know whether Harper or an application owns the URL. Rejecting there forces +an application to choose between two things it needs: Harper owning its own routes (status, REST +resources) and its own routes receiving their own credential scheme untouched. + +So a rejection is recorded rather than answered: + +- **Valid Harper credentials** authenticate normally and populate `request.user`. Unchanged. +- **No credentials** continue anonymously. Unchanged. +- **A syntactically valid credential Harper does not recognize** leaves `request.user` unset, leaves + the inbound `Authorization` header byte-for-byte intact, and records request-local state through + `security/deferredAuthentication.ts`. The state lives behind a module-private `Symbol`: it is not + a header, not a `Request` field, not enumerable, and cannot be forged or read from outside that + module. +- **An internal authentication fault** — unreadable JWT keys, a storage failure, an unexpected error + type — is never deferred. `isCredentialRejection` only accepts an error carrying a 4xx status, so + anything else fails closed with the in-line 401. `tokenAuthentication.ts → validateToken` no longer + masks such faults as `invalid token`, which is what makes that distinction reachable. +- **The operations API** never defers: `request.isOperationsServer` short-circuits to the in-line + 401, because every operations route is Harper-owned and there is nothing to defer to. + +Any layer that establishes Harper owns the route then settles the deferred state before doing work: + +| Layer | Where | +| --------------------------- | ------------------------------------------------------------------ | +| `REST.ts → http()` | after `resources.getMatch` succeeds (and for the OpenAPI document) | +| `REST.ts` WebSocket handler | after `resources.getMatch(url, 'ws')` succeeds | +| `graphqlQuerying.ts` | after the `/graphql` prefix match, raised as its own `HTTPError` | + +Each renders the same generic 401 the middleware would have returned in-line — the deferred status +is pinned to 401 regardless of the underlying error's own status, so a 403 `token expired` reads +exactly as it did before. A Harper-owned route therefore behaves identically to the pre-deferral +build, protected or public: an unknown credential can never buy access an anonymous caller would +have received, and can never reach an application catch-all. Only a URL that reached +`nextHandler` — one no Harper route owns — carries the original header onward. + +The contract is route-ownership-based, not path-based. There is no exemption list, no carrier +header, no credential rename, and no pre-auth stripping shim. + ### Response Cache-Control / Vary policy (#1518, #1565) Three tiers, applied in two places: 1. **App/resource explicit** — a `Cache-Control` set by the resource (or `@table(cacheControl: "...")` for anonymous reads, emitted in `REST.ts → http()`) always wins. The declaration is required: anonymous readability alone never emits shared-cache headers, because a request-attribute-gated `allowRead` (IP, headers) would make inferred `public` unsound. -2. **Identity floor** — `security/auth.ts → applyResponseHeaders` stamps `Cache-Control: private, no-cache` + `Vary: Authorization` (+ `Cookie` when sessions are on) on any response where a principal was resolved or credentials were rejected (401), _unless_ the app opted into shared caching with `public`/`s-maxage` (the RFC 9111 opt-in). +2. **Identity floor** — `security/auth.ts → applyResponseHeaders` stamps `Cache-Control: private, no-cache` + `Vary: Authorization` (+ `Cookie` when sessions are on) on any response where a principal was resolved, credentials were rejected (401), or a credential rejection was deferred (#2418 — the application answered using the header Harper passed through, so the response is credential-dependent at a plain 200), _unless_ the app opted into shared caching with `public`/`s-maxage` (the RFC 9111 opt-in). 3. **CORS partitioning** — when CORS is enabled, every response gets `Vary: Origin` (the ACAO header is reflected per-origin, and its absence on no-Origin requests is origin-dependent too). The `@table(cacheControl:)` value is persisted on the primary-key attribute (like `expiration`), so all threads and future boots see it; `resources/databases.ts → table()` treats `null` as "schema explicitly has none" (clears on reload) and `undefined` as "caller is not schema-defining" (no clobber from `add_attribute`/cluster schema events). diff --git a/server/REST.ts b/server/REST.ts index e9b76af516..d1e29f9098 100644 --- a/server/REST.ts +++ b/server/REST.ts @@ -15,6 +15,7 @@ import { CONFIG_PARAMS } from '../utility/hdbTerms.ts'; import { ASIDE_STAGING_DIR } from '../components/Application.ts'; import { COMPONENT_PREPARATION_LOCK_DIR } from '../components/componentPreparationLock.ts'; import { restartNeeded } from '../components/requestRestart.ts'; +import { assertNoDeferredCredentialRejection } from '../security/deferredAuthentication.ts'; import { Request } from '../server/serverHelpers/Request.ts'; import { RequestTarget } from '../resources/RequestTarget'; @@ -222,6 +223,11 @@ async function http(request: Request, nextHandler, resources: Resources, httpOpt } } } + // Route ownership is now settled — either a resource matched or this is Harper's OpenAPI + // document — so a credential the authentication middleware deferred is decided here rather + // than travelling on to an application catch-all (#2418). Every path that reaches an + // application instead returned via `nextHandler` above. + assertNoDeferredCredentialRejection(request); if ((resource as any)?.isCaching) { const cacheControl = headersObject['cache-control']; if (cacheControl) { @@ -553,6 +559,9 @@ export function handleApplication(scope: import('../components/Scope.ts').Scope) // TODO: Ideally we would like to have a 404 response before upgrading to WebSocket protocol, probably return ws.close(1011, `No resource was found to handle ${request.pathname}`); } else { + // Harper owns this socket's route, so a deferred credential is rejected here too + // rather than being carried into the resource as anonymous (#2418). + assertNoDeferredCredentialRejection(request); request.handlerPath = entry.path; recordAction( (action) => ({ diff --git a/server/graphqlQuerying.ts b/server/graphqlQuerying.ts index b4fe105299..935aec8ee4 100644 --- a/server/graphqlQuerying.ts +++ b/server/graphqlQuerying.ts @@ -3,6 +3,7 @@ import type { RequestParams } from 'graphql-http'; import { getDeserializer } from './serverHelpers/contentTypes.ts'; import { resources } from '../resources/Resources.ts'; import logger from '../utility/logging/harper_logger.ts'; +import { getDeferredCredentialRejection } from '../security/deferredAuthentication.ts'; // This code makes heavy use of the word "node" to refer to a node in the GraphQL AST. @@ -581,6 +582,12 @@ export function handleApplication(scope: import('../components/Scope.ts').Scope) } try { + // Harper owns /graphql, so a credential the authentication middleware deferred is + // rejected here instead of reaching a resolver as an anonymous request (#2418). + // Raised as this file's own HTTPError so the status survives the handler's error + // mapping below, which would otherwise render an unrecognized Error as a 500. + const deferred = getDeferredCredentialRejection(request); + if (deferred) throw new HTTPError(deferred.message, deferred.status); // Await the `graphqlHandler` call here so that errors are caught. return await graphqlQueryingHandler(request as any); } catch (error) { diff --git a/unitTests/security/authCredentialDeferral.test.js b/unitTests/security/authCredentialDeferral.test.js new file mode 100644 index 0000000000..ed71255c34 --- /dev/null +++ b/unitTests/security/authCredentialDeferral.test.js @@ -0,0 +1,294 @@ +/** + * Drives the real `authentication` middleware through a real `authentication -> rest -> application + * catch-all` chain built by `server/middlewareChain.ts`. + * + * On the pre-fix revision every "reaches the application catch-all" case here fails: `security/auth.ts` + * answered 401 during credential parsing, so the chain terminated before route ownership was known. + */ +const assert = require('node:assert'); + +const testUtils = require('../testUtils.js'); +testUtils.preTestPrep(); + +const { makeCallbackChain } = require('#src/server/middlewareChain'); +const { Headers } = require('#src/server/serverHelpers/Headers'); +const { assertNoDeferredCredentialRejection } = require('#src/security/deferredAuthentication'); +const { ClientError, ServerError } = require('#src/utility/errors/hdbError'); +const serverModule = require('#src/server/Server'); +const tokenAuthentication = require('#src/security/tokenAuthentication'); +const { authentication } = require('#src/security/auth'); + +const HARPER_OWNED = '/Ledger/1'; +const APP_OWNED = '/wp-json/wc/v3/products'; + +// A WordPress Application Password, base64'd exactly as WordPress sends it — spaces and all. +const WORDPRESS_BASIC = `Basic ${Buffer.from('wordpress:abcd efgh ijkl mnop qrst uvwx').toString('base64')}`; +const HARPER_BASIC = `Basic ${Buffer.from('harper_admin:harper-pw').toString('base64')}`; +const DOWNSTREAM_BEARER = 'Bearer eyJhbGciOiJIUzI1NiJ9.d29vLXNlc3Npb24.not-a-harper-token'; + +function makeRequest(pathname, authorization, extra = {}) { + const headerObject = authorization ? { authorization } : {}; + return { + method: 'GET', + url: pathname, + pathname, + ip: '203.0.113.7', + headers: { + asObject: headerObject, + get: (name) => headerObject[name.toLowerCase()], + }, + peerCertificate: { subject: null }, + ...extra, + }; +} + +describe('deferred credential rejection through the app-port middleware chain', () => { + let originalGetUser; + let originalValidateOperationToken; + let originalValidateRefreshToken; + /** Records what each layer saw, so "which layer answered" is observable rather than inferred. */ + let trace; + /** Pathnames Harper claims ownership of, standing in for `resources.getMatch`. */ + let ownedPaths; + /** Users the Harper credential store recognizes, keyed by `username:password`. */ + let knownUsers; + /** When set, `getUser` raises this instead of resolving — an internal authentication fault. */ + let getUserFault; + + /** + * Mirrors `server/REST.ts`'s ownership branch: unowned URLs pass to the next layer untouched, + * owned ones settle any deferred credential through the same production assertion REST calls. + */ + function restLayer(request, nextHandler) { + if (!ownedPaths.has(request.pathname)) return nextHandler(request); + trace.push('rest'); + try { + assertNoDeferredCredentialRejection(request); + } catch (error) { + return { status: error.statusCode, headers: new Headers(), body: JSON.stringify({ error: error.message }) }; + } + if (!request.user) return { status: 401, headers: new Headers(), body: JSON.stringify({ error: 'Login failed' }) }; + return { + status: 200, + headers: new Headers(), + body: JSON.stringify({ servedBy: 'rest', user: request.user.username }), + }; + } + + /** The application's own middleware, mounted after `rest`, applying its own auth scheme. */ + function applicationCatchAll(request) { + trace.push('catch-all'); + return { + status: 200, + headers: new Headers(), + body: JSON.stringify({ + servedBy: 'catch-all', + authorization: request.headers.asObject.authorization ?? null, + harperUser: request.user?.username ?? null, + }), + }; + } + + const chain = makeCallbackChain( + [ + { listener: applicationCatchAll, port: 'all', name: 'applicationCatchAll', after: 'rest' }, + { listener: authentication, port: 'all', name: 'authentication' }, + { listener: restLayer, port: 'all', name: 'rest', after: 'authentication' }, + ], + 'all', + () => ({ status: 404, headers: new Headers(), body: 'Not found' }) + ); + + async function send(pathname, authorization, extra) { + const request = makeRequest(pathname, authorization, extra); + const response = await chain(request); + return { request, response, body: response?.body ? JSON.parse(response.body) : undefined }; + } + + before(() => { + originalGetUser = serverModule.server.getUser; + originalValidateOperationToken = tokenAuthentication.validateOperationToken; + originalValidateRefreshToken = tokenAuthentication.validateRefreshToken; + + serverModule.server.getUser = async (username, password) => { + if (getUserFault) throw getUserFault; + const user = knownUsers.get(`${username}:${password}`); + if (!user) throw new ClientError('Login failed', 401); + return user; + }; + tokenAuthentication.validateOperationToken = async () => { + throw new ClientError('invalid token', 401); + }; + tokenAuthentication.validateRefreshToken = async () => { + throw new ClientError('invalid token', 401); + }; + }); + + after(() => { + serverModule.server.getUser = originalGetUser; + tokenAuthentication.validateOperationToken = originalValidateOperationToken; + tokenAuthentication.validateRefreshToken = originalValidateRefreshToken; + }); + + beforeEach(() => { + trace = []; + ownedPaths = new Set([HARPER_OWNED]); + knownUsers = new Map([['harper_admin:harper-pw', { username: 'harper_admin', role: { permission: {} } }]]); + getUserFault = undefined; + }); + + it('resolves the chain as authentication -> rest -> application catch-all', async () => { + // The order is what makes the rest of this suite meaningful: `rest` must get first refusal on + // every URL, and the application middleware must only see what `rest` declined. + const { body } = await send(APP_OWNED, undefined); + + assert.deepStrictEqual(trace, ['catch-all']); + assert.strictEqual(body.servedBy, 'catch-all'); + + const owned = await send(HARPER_OWNED, HARPER_BASIC); + assert.deepStrictEqual(trace, ['catch-all', 'rest']); + assert.strictEqual(owned.body.servedBy, 'rest'); + }); + + it('authenticates valid Harper Basic credentials and serves the owned resource', async () => { + const { request, body, response } = await send(HARPER_OWNED, HARPER_BASIC); + + assert.strictEqual(response.status, 200); + assert.strictEqual(request.user.username, 'harper_admin'); + assert.strictEqual(body.user, 'harper_admin'); + assert.deepStrictEqual(trace, ['rest']); + }); + + it('hands an unrecognized WordPress Basic credential to the catch-all, byte-for-byte', async () => { + const { request, response, body } = await send(APP_OWNED, WORDPRESS_BASIC); + + assert.strictEqual(response.status, 200); + assert.strictEqual(body.servedBy, 'catch-all'); + // The header the application receives must be the header the client sent — no rename, no + // carrier header, no stripping. + assert.strictEqual(body.authorization, WORDPRESS_BASIC); + assert.strictEqual(request.headers.asObject.authorization, WORDPRESS_BASIC); + // And no Harper principal was invented along the way. + assert.strictEqual(body.harperUser, null); + assert.strictEqual(request.user, undefined); + }); + + it('gives a downstream-owned Bearer token the same treatment as Basic', async () => { + const { response, body } = await send(APP_OWNED, DOWNSTREAM_BEARER); + + assert.strictEqual(response.status, 200); + assert.strictEqual(body.authorization, DOWNSTREAM_BEARER); + assert.strictEqual(body.harperUser, null); + }); + + it('lets a Harper refresh token keep its declined (-1) handling instead of deferring', async () => { + // A refresh token is Harper's own credential: `authentication` declines the request so the + // operations API can handle it, and that must not turn into a deferral to the application. + tokenAuthentication.validateRefreshToken = async () => ({ username: 'harper_admin' }); + try { + const { response } = await send(APP_OWNED, 'Bearer harper-refresh-token'); + + assert.strictEqual(response.status, -1); + assert.deepStrictEqual(trace, []); + } finally { + tokenAuthentication.validateRefreshToken = async () => { + throw new ClientError('invalid token', 401); + }; + } + }); + + it('rejects an unrecognized credential at a Harper-owned route and never reaches the catch-all', async () => { + const { response, body } = await send(HARPER_OWNED, WORDPRESS_BASIC); + + assert.strictEqual(response.status, 401); + assert.strictEqual(body.error, 'Login failed'); + assert.deepStrictEqual(trace, ['rest']); + assert.ok(!trace.includes('catch-all'), 'a Harper-owned route must not fall through to the application'); + }); + + it('rejects an invalid Harper Bearer token at a Harper-owned route', async () => { + const { response, body } = await send(HARPER_OWNED, DOWNSTREAM_BEARER); + + assert.strictEqual(response.status, 401); + assert.strictEqual(body.error, 'invalid token'); + assert.deepStrictEqual(trace, ['rest']); + }); + + it('reports an expired Harper token as 401 at a Harper-owned route, as it did before deferral', async () => { + tokenAuthentication.validateOperationToken = async () => { + throw new ClientError('token expired', 403); + }; + try { + const { response, body } = await send(HARPER_OWNED, 'Bearer expired-harper-token'); + + assert.strictEqual(response.status, 401); + assert.strictEqual(body.error, 'token expired'); + assert.deepStrictEqual(trace, ['rest']); + } finally { + tokenAuthentication.validateOperationToken = async () => { + throw new ClientError('invalid token', 401); + }; + } + }); + + it('does not downgrade a Harper-owned route to public just because the credential was unknown', async () => { + // `rest` here serves anonymous requests happily; the deferred rejection still wins, so an + // unknown credential can never buy access an anonymous caller would have received. + const anonymous = await send(HARPER_OWNED, undefined); + assert.strictEqual(anonymous.response.status, 401); + + const withUnknownCredential = await send(HARPER_OWNED, WORDPRESS_BASIC); + assert.strictEqual(withUnknownCredential.response.status, 401); + assert.strictEqual(withUnknownCredential.body.error, 'Login failed'); + }); + + it('leaves a request with no credentials completely unchanged', async () => { + const { request, response, body } = await send(APP_OWNED, undefined); + + assert.strictEqual(response.status, 200); + assert.strictEqual(body.authorization, null); + assert.strictEqual(request.user, undefined); + assert.deepStrictEqual(trace, ['catch-all']); + }); + + it('fails closed on an internal authentication fault instead of deferring', async () => { + getUserFault = new ServerError('user store unavailable'); + + const { response } = await send(APP_OWNED, WORDPRESS_BASIC); + + assert.strictEqual(response.status, 401); + // The whole point: an outage must not hand the request to the application's own authorization. + assert.deepStrictEqual(trace, []); + }); + + it('fails closed when the credential store raises a bare, unclassified error', async () => { + getUserFault = new Error('ENOENT: hdb_user'); + + const { response } = await send(APP_OWNED, WORDPRESS_BASIC); + + assert.strictEqual(response.status, 401); + assert.deepStrictEqual(trace, []); + }); + + it('never defers on the operations API, where Harper owns every route', async () => { + const { response } = await send(APP_OWNED, WORDPRESS_BASIC, { isOperationsServer: true }); + + assert.strictEqual(response.status, 401); + assert.deepStrictEqual(trace, []); + }); + + it('marks a deferred-credential response as identity-dependent for shared caches', async () => { + // The application answered using the Authorization header Harper passed through, so the + // response is credential-dependent even though no Harper principal was resolved (#1565). + const { response } = await send(APP_OWNED, WORDPRESS_BASIC); + + assert.strictEqual(response.headers.get('Vary').includes('Authorization'), true); + assert.strictEqual(response.headers.get('Cache-Control'), 'private, no-cache'); + }); + + it('does not stamp the identity floor on an ordinary anonymous pass-through', async () => { + const { response } = await send(APP_OWNED, undefined); + + assert.strictEqual(response.headers?.get?.('Cache-Control') ?? null, null); + }); +}); diff --git a/unitTests/security/deferredAuthentication.test.js b/unitTests/security/deferredAuthentication.test.js new file mode 100644 index 0000000000..86bb16baa2 --- /dev/null +++ b/unitTests/security/deferredAuthentication.test.js @@ -0,0 +1,121 @@ +const assert = require('node:assert'); + +const { + assertNoDeferredCredentialRejection, + deferCredentialRejection, + getDeferredCredentialRejection, + isCredentialRejection, +} = require('#src/security/deferredAuthentication'); +const { ClientError, ServerError } = require('#src/utility/errors/hdbError'); + +describe('deferredAuthentication', () => { + describe('isCredentialRejection', () => { + it('accepts Harper 4xx authentication ClientErrors', () => { + assert.strictEqual(isCredentialRejection(new ClientError('Login failed', 401)), true); + assert.strictEqual(isCredentialRejection(new ClientError('token expired', 403)), true); + assert.strictEqual(isCredentialRejection(new ClientError('invalid token', 400)), true); + }); + + it('rejects internal faults so they fail closed instead of deferring', () => { + // A missing-JWT-keys fault is a 500 ClientError in this codebase; it must never defer. + assert.strictEqual(isCredentialRejection(new ClientError('no encryption keys', 500)), false); + assert.strictEqual(isCredentialRejection(new ServerError('storage unavailable')), false); + // A bare Error carries no status at all — a bug or a driver failure, not a rejection. + assert.strictEqual(isCredentialRejection(new Error('ENOENT')), false); + assert.strictEqual(isCredentialRejection(new TypeError('Invalid character')), false); + assert.strictEqual(isCredentialRejection(undefined), false); + assert.strictEqual(isCredentialRejection(null), false); + // A non-numeric status must not be coerced into the 4xx window. + assert.strictEqual(isCredentialRejection({ statusCode: '401' }), false); + }); + + it('reads a plain `status` as well as `statusCode`', () => { + assert.strictEqual(isCredentialRejection({ status: 401 }), true); + assert.strictEqual(isCredentialRejection({ status: 503 }), false); + }); + }); + + describe('deferCredentialRejection', () => { + it('records the rejection without exposing it as an enumerable property', () => { + const request = { headers: { authorization: 'Basic d3A6c2VjcmV0' } }; + deferCredentialRejection(request, new ClientError('Login failed', 401), 'Basic'); + + assert.deepStrictEqual(Object.keys(request), ['headers']); + assert.strictEqual(JSON.stringify(request), '{"headers":{"authorization":"Basic d3A6c2VjcmV0"}}'); + }); + + it('leaves the inbound Authorization header byte-for-byte unchanged', () => { + const authorization = 'Basic d29yZHByZXNzOmFiY2QgZWZnaCBpamtsIG1ub3AgcXJzdCB1dnd4'; + const request = { headers: { authorization } }; + deferCredentialRejection(request, new ClientError('Login failed', 401), 'Basic'); + + assert.strictEqual(request.headers.authorization, authorization); + }); + + it('pins the deferred status to 401 even when the underlying rejection was a 403', () => { + // The authentication middleware has always answered a rejected credential with 401 + // regardless of the error's own status, so a deferred rejection has to match that. + const request = {}; + deferCredentialRejection(request, new ClientError('token expired', 403), 'Bearer'); + + assert.deepStrictEqual(getDeferredCredentialRejection(request), { + status: 401, + message: 'token expired', + strategy: 'Bearer', + }); + }); + + it('falls back to a generic message when the rejection carries none', () => { + const request = {}; + deferCredentialRejection(request, {}, 'Bearer'); + + assert.strictEqual(getDeferredCredentialRejection(request).message, 'Unauthorized'); + }); + + it('is readable through a proxy of the request, as the urlPath-mount chain produces', () => { + const request = {}; + deferCredentialRejection(request, new ClientError('Login failed', 401), 'Basic'); + const proxied = new Proxy(request, { get: (target, prop) => Reflect.get(target, prop) }); + + assert.strictEqual(getDeferredCredentialRejection(proxied).status, 401); + }); + }); + + describe('getDeferredCredentialRejection', () => { + it('returns undefined for a request that presented no credential', () => { + assert.strictEqual(getDeferredCredentialRejection({}), undefined); + assert.strictEqual(getDeferredCredentialRejection(undefined), undefined); + }); + + it('cannot be forged through a string or well-known symbol key', () => { + const forged = { + 'deferredCredentialRejection': { status: 401, message: 'forged', strategy: 'Basic' }, + 'harper.deferredCredentialRejection': { status: 401, message: 'forged', strategy: 'Basic' }, + [Symbol.for('harper.deferredCredentialRejection')]: { status: 401, message: 'forged', strategy: 'Basic' }, + }; + + assert.strictEqual(getDeferredCredentialRejection(forged), undefined); + }); + }); + + describe('assertNoDeferredCredentialRejection', () => { + it('does nothing for a request with no deferred rejection', () => { + assert.doesNotThrow(() => assertNoDeferredCredentialRejection({})); + }); + + it('throws the unauthorized ClientError an owning Harper layer renders', () => { + const request = {}; + deferCredentialRejection(request, new ClientError('Login failed', 401), 'Basic'); + + assert.throws( + () => assertNoDeferredCredentialRejection(request), + (error) => { + assert.ok(error instanceof ClientError); + assert.strictEqual(error.statusCode, 401); + assert.strictEqual(error.message, 'Login failed'); + return true; + } + ); + }); + }); +}); From 8693319fb8f4cba4a8925765cd2f00f61a7d3861 Mon Sep 17 00:00:00 2001 From: hdbjeff Date: Mon, 31 Aug 2026 10:54:43 -0700 Subject: [PATCH 02/12] fix: reject an unimplemented auth scheme instead of continuing anonymously (#2418) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An `Authorization` header whose scheme Harper does not implement (`Digest`, an application's own, or a header with no scheme token at all) matched no case in the strategy switch and threw nothing, so it continued as an anonymous request. On an anonymously-readable Harper-owned route that hands the content over, which is the exact downgrade deferral exists to prevent — and it made the contract inconsistent, since an unrecognized Basic or Bearer credential was already rejected there. A `default` case now rejects it, routing it through the same audit, fail-closed, and deferral handling as any other unrecognized credential. The gate stays on a strictly undefined user so the legacy blank-Basic "no auth" form still continues anonymously. Also guards the success audit log against that legacy null user, which would have thrown a TypeError when logging.auditAuthEvents.logSuccessful is enabled. Co-Authored-By: Claude Opus 5 (1M context) --- security/auth.ts | 15 +++- .../security/authCredentialDeferral.test.js | 72 ++++++++++++++++--- 2 files changed, 78 insertions(+), 9 deletions(-) diff --git a/security/auth.ts b/security/auth.ts index dc1db66967..9a979e9e22 100644 --- a/security/auth.ts +++ b/security/auth.ts @@ -19,6 +19,8 @@ import { isCredentialRejection, } from './deferredAuthentication.ts'; import { serializeMessage } from '../server/serverHelpers/contentTypes.ts'; +import { ClientError, hdbErrors } from '../utility/errors/hdbError.ts'; +const { AUTHENTICATION_ERROR_MSGS, HTTP_STATUS_CODES } = hdbErrors; const authLogger = forComponent('authentication'); const { debug } = authLogger; const authEventLog = authLogger.withTag('auth-event'); @@ -252,6 +254,14 @@ export async function authentication(request, nextHandler) { throw error; } break; + default: + // A scheme Harper does not implement (`Digest`, an application's own, or a + // header with no scheme token at all) previously matched no case and threw + // nothing, so it continued as an anonymous request — on a Harper-owned route + // that is precisely the downgrade this change exists to prevent. Rejecting it + // here routes it through the same audit, fail-closed, and deferral handling as + // an unrecognized Basic or Bearer credential. + throw new ClientError(AUTHENTICATION_ERROR_MSGS.GENERIC_AUTH_FAIL, HTTP_STATUS_CODES.UNAUTHORIZED); } } catch (err) { if (LOG_AUTH_FAILED) { @@ -283,7 +293,10 @@ export async function authentication(request, nextHandler) { deferCredentialRejection(request, credentialRejection, strategy); } else { authorizationCache.set(authorization, newUser); - if (LOG_AUTH_SUCCESSFUL) authAuditLog(newUser.username, AUTH_AUDIT_STATUS.SUCCESS, strategy); + // `newUser` is null on the legacy blank-Basic-credentials path, which means "no auth" + // and stays anonymous; reading `.username` off it would crash the request. + if (LOG_AUTH_SUCCESSFUL && newUser != null) + authAuditLog(newUser.username, AUTH_AUDIT_STATUS.SUCCESS, strategy); // Shallow-clone so verifyPerms's `role.permission = fullRolePerms` reassignment // doesn't mutate the just-stored cache entry (defense-in-depth). if (newUser?.role) { diff --git a/unitTests/security/authCredentialDeferral.test.js b/unitTests/security/authCredentialDeferral.test.js index ed71255c34..925ba53a9b 100644 --- a/unitTests/security/authCredentialDeferral.test.js +++ b/unitTests/security/authCredentialDeferral.test.js @@ -19,6 +19,9 @@ const tokenAuthentication = require('#src/security/tokenAuthentication'); const { authentication } = require('#src/security/auth'); const HARPER_OWNED = '/Ledger/1'; +// A Harper-owned route that serves anonymous callers — the case where 'continued as anonymous' +// and 'rejected the credential' produce visibly different responses. +const HARPER_OWNED_PUBLIC = '/PublicNotice/1'; const APP_OWNED = '/wp-json/wc/v3/products'; // A WordPress Application Password, base64'd exactly as WordPress sends it — spaces and all. @@ -67,11 +70,12 @@ describe('deferred credential rejection through the app-port middleware chain', } catch (error) { return { status: error.statusCode, headers: new Headers(), body: JSON.stringify({ error: error.message }) }; } - if (!request.user) return { status: 401, headers: new Headers(), body: JSON.stringify({ error: 'Login failed' }) }; + if (!request.user && request.pathname !== HARPER_OWNED_PUBLIC) + return { status: 401, headers: new Headers(), body: JSON.stringify({ error: 'Login failed' }) }; return { status: 200, headers: new Headers(), - body: JSON.stringify({ servedBy: 'rest', user: request.user.username }), + body: JSON.stringify({ servedBy: 'rest', user: request.user?.username ?? null }), }; } @@ -132,7 +136,7 @@ describe('deferred credential rejection through the app-port middleware chain', beforeEach(() => { trace = []; - ownedPaths = new Set([HARPER_OWNED]); + ownedPaths = new Set([HARPER_OWNED, HARPER_OWNED_PUBLIC]); knownUsers = new Map([['harper_admin:harper-pw', { username: 'harper_admin', role: { permission: {} } }]]); getUserFault = undefined; }); @@ -232,16 +236,68 @@ describe('deferred credential rejection through the app-port middleware chain', }); it('does not downgrade a Harper-owned route to public just because the credential was unknown', async () => { - // `rest` here serves anonymous requests happily; the deferred rejection still wins, so an - // unknown credential can never buy access an anonymous caller would have received. - const anonymous = await send(HARPER_OWNED, undefined); - assert.strictEqual(anonymous.response.status, 401); + // This route serves anonymous callers, so an unknown credential that merely became + // "anonymous" would be handed the content. The deferred rejection wins instead. + const anonymous = await send(HARPER_OWNED_PUBLIC, undefined); + assert.strictEqual(anonymous.response.status, 200); - const withUnknownCredential = await send(HARPER_OWNED, WORDPRESS_BASIC); + const withUnknownCredential = await send(HARPER_OWNED_PUBLIC, WORDPRESS_BASIC); assert.strictEqual(withUnknownCredential.response.status, 401); assert.strictEqual(withUnknownCredential.body.error, 'Login failed'); }); + it('defers a scheme Harper does not implement rather than continuing anonymously', async () => { + // Reported by review on this PR: `Digest` matches no case in the strategy switch and throws + // nothing, so before this it fell through as an anonymous request. + const digest = 'Digest username="wp", realm="site", response="0123456789abcdef"'; + const { request, response, body } = await send(APP_OWNED, digest); + + assert.strictEqual(response.status, 200); + assert.strictEqual(body.servedBy, 'catch-all'); + assert.strictEqual(body.authorization, digest); + assert.strictEqual(request.user, undefined); + }); + + it('rejects a scheme Harper does not implement at an anonymously-readable Harper route', async () => { + // The decisive case: this route serves anonymous callers, so continuing as anonymous would + // return 200. Only an actual deferred rejection produces the 401. + const anonymous = await send(HARPER_OWNED_PUBLIC, undefined); + assert.strictEqual(anonymous.response.status, 200); + + const { response, body } = await send(HARPER_OWNED_PUBLIC, 'Digest username="wp", response="deadbeef"'); + assert.strictEqual(response.status, 401); + assert.strictEqual(body.error, 'Login failed'); + assert.deepStrictEqual(trace, ['rest', 'rest']); + }); + + it('treats an Authorization header with no scheme token as an unrecognized credential', async () => { + const { response } = await send(HARPER_OWNED_PUBLIC, 'aGFyZGx5LWEtc2NoZW1l'); + + assert.strictEqual(response.status, 401); + assert.deepStrictEqual(trace, ['rest']); + }); + + it('fails an unimplemented scheme closed in place on the operations API', async () => { + const { response } = await send(APP_OWNED, 'Digest username="wp"', { isOperationsServer: true }); + + assert.strictEqual(response.status, 401); + assert.deepStrictEqual(trace, []); + }); + + it('keeps the legacy blank Basic credential anonymous instead of deferring it', async () => { + // `Basic ` + base64(':') is the documented "no auth" form: it must stay anonymous, so the + // unrecognized-scheme rejection above is gated on a strictly `undefined` user, not a nullish one. + const blank = `Basic ${Buffer.from(':').toString('base64')}`; + const { request, response, body } = await send(APP_OWNED, blank); + + assert.strictEqual(response.status, 200); + assert.strictEqual(body.servedBy, 'catch-all'); + assert.strictEqual(request.user, null); + // Anonymous, not deferred — so an anonymously-readable Harper route still serves it. + const owned = await send(HARPER_OWNED_PUBLIC, blank); + assert.strictEqual(owned.response.status, 200); + }); + it('leaves a request with no credentials completely unchanged', async () => { const { request, response, body } = await send(APP_OWNED, undefined); From ba58410fece8aa36fd6b646bdd6f78a8d3225855 Mon Sep 17 00:00:00 2001 From: hdbjeff Date: Mon, 31 Aug 2026 12:29:17 -0700 Subject: [PATCH 03/12] fix: apply a late port:'all' middleware registration to bound ports (#2418) An entry registered on the 'all' pseudo-port is folded into each concrete port's chain when that chain is built, but nothing dispatches through chains.all itself. A registration arriving after the bound port's chain already existed therefore updated only chains.all and never reached a request. That is exactly the shape an application catch-all takes: `rest` registers first, so a handler ordered `after: 'rest'` is always late. buildChains() now rebuilds every already-built chain of that kind when the registration is on 'all', for http, upgrade, and websocket alike. Rebuilding is a pure function of the listener list and the port, so the extra passes can only reproduce a port's order or extend it. It also writes the get_status chain description in the same pass, which removes the #1573 caveat about a description outliving the chain it described. Also corrects the no-credentials case in the #2418 end-to-end test: the integration harness starts Harper with AUTHENTICATION_AUTHORIZELOCAL=true, so a loopback caller sending no Authorization header is still resolved to the local super user. That pre-existing path is untouched by credential deferral, and asserting it explicitly is what distinguishes it from the deferred-credential cases, which must attach no principal at all. Co-Authored-By: Claude Opus 5 (1M context) --- .../deferred-credential-rejection.test.ts | 8 +- server/DESIGN.md | 16 +++- server/http.ts | 52 +++++++++++-- unitTests/server/httpChainPortAll.test.js | 74 +++++++++++++++++++ 4 files changed, 141 insertions(+), 9 deletions(-) create mode 100644 unitTests/server/httpChainPortAll.test.js diff --git a/integrationTests/security/deferred-credential-rejection.test.ts b/integrationTests/security/deferred-credential-rejection.test.ts index 720f6c9b10..49723e1c4f 100644 --- a/integrationTests/security/deferred-credential-rejection.test.ts +++ b/integrationTests/security/deferred-credential-rejection.test.ts @@ -150,7 +150,13 @@ suite( equal(unowned.status, 200); equal(unowned.body.servedBy, 'application-catch-all'); equal(unowned.body.authorization, null); - equal(unowned.body.harperUser, null); + // No Authorization header means no credential to defer, so Harper's own principal + // resolution runs exactly as it did before: this harness starts Harper with + // AUTHENTICATION_AUTHORIZELOCAL=true, and a loopback caller with no credentials is + // therefore still the local super user. That untouched path is precisely why the + // deferred-credential cases above assert `harperUser === null` — a rejected credential + // must not reach this bypass and be answered as a privileged anonymous request. + equal(unowned.body.harperUser, ctx.harper.admin.username); }); test('a deferred-credential response is kept out of shared caches', async () => { diff --git a/server/DESIGN.md b/server/DESIGN.md index 8274569397..d5dea03cee 100644 --- a/server/DESIGN.md +++ b/server/DESIGN.md @@ -131,7 +131,8 @@ Every entry is a top-level function or named const. Jump via go-to-symbol or `gr | `getPorts()` | Resolves listener options → list of `{port, secure}`. | | `httpServer()` | Main listener registration entry point. | | `getHTTPServer(port, secure, options)` | **The largest function in the file.** Creates/retrieves the underlying Node HTTP/HTTPS server. Wires `request`, `upgrade`, error handlers, TLS context, and the per-port middleware chain. | -| `makeCallbackChain()` | Builds the per-port handler chain via `middlewareChain.topoSort`. | +| `makeCallbackChain()` | Builds the per-port handler chain via `middlewareChain.topoSort`, and records its resolved order for `get_status`. | +| `buildChains()` | Stores a built chain, and rebuilds every other already-built chain of that kind when the registration is on the `'all'` pseudo-port. See "Middleware ordering" below. | | `unhandled()` | Terminal 404 handler. | | `onRequest()` | Thin alias of `httpServer({requestOnly: true})`. | | `onUpgrade()` / `upgradeListeners` (const) | Register HTTP upgrade listener; underlying list. | @@ -151,6 +152,19 @@ Components register listeners with optional `before: 'name'` / `after: 'name'` o The default WebSocket upgrade handler is registered automatically inside `onWebSocket()` the first time it runs for a given port. +**`port: 'all'` is a pseudo-port, and its chain is not the one that serves traffic.** Chain building +folds every `'all'` entry into each concrete port's chain, so `chains.all` exists but nothing +dispatches through it — `httpChain[port]`/`upgradeChains[port]`/`websocketChains[port]` are looked up +by the bound port on every request (Node, Bun, and uWS alike). A registration therefore has to +rebuild the chains it affects, not just its own key: `http.ts → buildChains()` rebuilds every +already-built chain of that kind whenever the registration is on `'all'`. Without that, an entry +registered on `'all'` after the concrete port's chain was built — the shape an application catch-all +mounted `after: 'rest'` takes, since `rest` registers first — updates only `chains.all` and never +reaches a request (#2418). Rebuilding is a pure function of the listener list and the port, so the +extra passes can only reproduce a port's order or extend it. `buildChains()` also writes the +`get_status` chain description in the same pass, which is what keeps that report from ever +describing an order a port isn't running (#1573). + ### Application mounts (`host` / `urlPath` in the root config) An operator mounts an application by putting `host`/`urlPath` on its entry in the **root** config; `components/scopeMount.ts` models it and the loader threads it into every `Scope` for that application (both load paths — the root-config `package` recursion and the components-root directory scan). diff --git a/server/http.ts b/server/http.ts index 0a212cff16..3c853f7fe0 100644 --- a/server/http.ts +++ b/server/http.ts @@ -34,7 +34,7 @@ import { server, type ServerOptions, type HttpOptions, type UpgradeOptions, Upgr import { setPortServerMap, SERVERS, socketOptionDefaults } from './serverRegistry.ts'; import { getComponentName } from '../components/componentLoader.ts'; import { throttle } from './throttle.ts'; -import { makeCallbackChain as buildCallbackChain, describeChains } from './middlewareChain.ts'; +import { makeCallbackChain as buildCallbackChain, describeChains, type HttpEntry } from './middlewareChain.ts'; import { WebSocketServer } from 'ws'; const { errorToString, errorForLog } = harperLogger; @@ -504,7 +504,7 @@ export function httpServer(listener, options) { listener.isSecure = secure; registerServer(listener, port, false); } - httpChain[port] = makeCallbackChain(httpResponders, port); + buildChains(httpChain, httpResponders, port); } return servers; @@ -1507,14 +1507,52 @@ type SerializedRoute = { host?: string; urlPath?: string; order: string[] }; // Resolved order captured at chain-build time, keyed identically to httpChain/upgradeChains/ // websocketChains (kind → port → routes). Reporting the stored build-time order rather than // recomputing from current responders guarantees get_status matches the callback chain actually -// serving that port — including cases where a late `port: 'all'` registration rebuilds only the -// 'all' chain and leaves a concrete port's chain (and this description) unchanged (#1573). +// serving that port: buildChains() writes both in the same pass, so a description can never +// describe an order the port isn't running (#1573). const resolvedChainDescriptions: Record> = { http: {}, upgrade: {}, websocket: {}, }; +// Every port that has a built chain, per kind, mapping its stringified form to the port value as +// registered. The chain maps are plain objects, so their own keys are strings, but chain building +// selects responders with `port === portNum` — rebuilding port 9926 under the key '9926' would +// match no responder at all. +const builtChainPorts: Record> = { + http: new Map(), + upgrade: new Map(), + websocket: new Map(), +}; + +/** + * Builds `chains[port]` from the current `listeners` and, when `port` is the 'all' pseudo-port, + * rebuilds every other already-built chain of the same kind too. + * + * An entry registered on 'all' is folded into each concrete port's chain when that chain is built, + * so a registration arriving after those chains exist — an application catch-all mounted + * `after: 'rest'`, say — would otherwise update only `chains.all`, which nothing serves, leaving + * every bound port running the chain it had beforehand (#2418). Rebuilding is a pure function of + * the listener list and the port, so re-running it for an already-built port can only reproduce + * that port's order or extend it with the entry just registered. + */ +function buildChains( + chains: Record, + listeners: HttpEntry[], + port: number | string, + requestArgIndex: number = 0, + kind: string = 'http' +) { + builtChainPorts[kind].set(String(port), port); + if (port !== 'all') { + chains[port] = makeCallbackChain(listeners, port, requestArgIndex, kind); + return; + } + for (const builtPort of builtChainPorts[kind].values()) { + chains[builtPort] = makeCallbackChain(listeners, builtPort, requestArgIndex, kind); + } +} + function makeCallbackChain( responders: typeof httpResponders, portNum: number | string, @@ -1640,7 +1678,7 @@ function onUpgrade(listener: UpgradeListener, options: UpgradeOptions) { host: options?.host || undefined, }; upgradeListeners[options?.runFirst ? 'unshift' : 'push'](entry); - upgradeChains[port] = makeCallbackChain(upgradeListeners, port, 0, 'upgrade'); + buildChains(upgradeChains, upgradeListeners, port, 0, 'upgrade'); } } @@ -1779,10 +1817,10 @@ function onWebSocket(listener: (ws: WebSocket) => void, options: OnWebSocketOpti host: options?.host || undefined, }; websocketListeners[options?.runFirst ? 'unshift' : 'push'](wsEntry); - websocketChains[port] = makeCallbackChain(websocketListeners, port, 1, 'websocket'); + buildChains(websocketChains, websocketListeners, port, 1, 'websocket'); // mqtt doesn't invoke the http handler so this needs to be here to load up the http chains. - httpChain[port] = makeCallbackChain(httpResponders, port); + buildChains(httpChain, httpResponders, port); } return servers; diff --git a/unitTests/server/httpChainPortAll.test.js b/unitTests/server/httpChainPortAll.test.js new file mode 100644 index 0000000000..0f97d68b97 --- /dev/null +++ b/unitTests/server/httpChainPortAll.test.js @@ -0,0 +1,74 @@ +'use strict'; + +const testUtils = require('../testUtils.js'); +testUtils.preTestPrep(); + +const assert = require('node:assert'); + +const { httpServer, describeMiddlewareChains } = require('#src/server/http'); +const { server } = require('#src/server/Server'); + +// Ports are never bound here: httpServer()/server.upgrade() only construct a server object and +// register a middleware entry, and threadServer.listenOnPorts() is what binds. Two distinct ports +// so the "an 'all' rebuild doesn't cross-contaminate concrete ports" case is observable. +const PORT = 19418; +const OTHER_PORT = 19419; + +const passThrough = (request, next) => next(request); + +/** + * The resolved order of the default (unmounted) route for `port`, restricted to this file's own + * entries — `httpResponders` is process-global, so a sibling unit test file loaded into the same + * mocha run can legitimately have registered entries of its own. + */ +function orderFor(kind, port) { + const routes = describeMiddlewareChains()[kind][port] ?? []; + const defaultRoute = routes.find((route) => !route.host && !route.urlPath); + return (defaultRoute?.order ?? []).filter((name) => name.startsWith('chainSync')); +} + +describe('http middleware chains and the "all" pseudo-port', () => { + it('folds a late port:"all" registration into concrete port chains that were already built', () => { + httpServer(passThrough, { port: PORT, name: 'chainSyncAuthentication' }); + httpServer(passThrough, { port: PORT, name: 'chainSyncRest', after: 'chainSyncAuthentication' }); + + assert.deepStrictEqual(orderFor('http', PORT), ['chainSyncAuthentication', 'chainSyncRest']); + + // The shape an application catch-all uses: registered on every port, ordered after Harper's + // own route ownership, and arriving after the concrete port's chain already exists (#2418). + httpServer(passThrough, { port: 'all', name: 'chainSyncCatchAll', after: 'chainSyncRest' }); + + assert.deepStrictEqual(orderFor('http', PORT), ['chainSyncAuthentication', 'chainSyncRest', 'chainSyncCatchAll']); + }); + + it('applies a late port:"all" registration to every already-built port, not just the newest', () => { + httpServer(passThrough, { port: OTHER_PORT, name: 'chainSyncOtherPortRest' }); + httpServer(passThrough, { port: 'all', name: 'chainSyncSecondCatchAll', after: 'chainSyncOtherPortRest' }); + + assert.ok(orderFor('http', PORT).includes('chainSyncSecondCatchAll')); + // `chainSyncCatchAll` is on 'all' too, so it belongs to this port's chain as well; its + // `after: 'chainSyncRest'` names nothing registered here, leaving it in registration order. + assert.deepStrictEqual(orderFor('http', OTHER_PORT), [ + 'chainSyncCatchAll', + 'chainSyncOtherPortRest', + 'chainSyncSecondCatchAll', + ]); + }); + + it('does not leak a concrete port registration into another port chain', () => { + httpServer(passThrough, { port: OTHER_PORT, name: 'chainSyncOtherPortOnly' }); + + assert.ok(!orderFor('http', PORT).includes('chainSyncOtherPortOnly')); + assert.ok(orderFor('http', OTHER_PORT).includes('chainSyncOtherPortOnly')); + }); + + it('synchronizes upgrade chains on the same terms as http chains', () => { + server.upgrade(passThrough, { port: PORT, name: 'chainSyncUpgrade' }); + + assert.deepStrictEqual(orderFor('upgrade', PORT), ['chainSyncUpgrade']); + + server.upgrade(passThrough, { port: 'all', name: 'chainSyncUpgradeCatchAll', after: 'chainSyncUpgrade' }); + + assert.deepStrictEqual(orderFor('upgrade', PORT), ['chainSyncUpgrade', 'chainSyncUpgradeCatchAll']); + }); +}); From dac15de73bfb158b1cfa2c6edad7b1ec45f15332 Mon Sep 17 00:00:00 2001 From: hdbjeff Date: Mon, 31 Aug 2026 14:20:23 -0700 Subject: [PATCH 04/12] fix: assert credential-rejection provenance and settle deferral at every Harper owner (#2418) Deferred credential rejection classified provenance by status range and left several Harper-owned handlers unsettled. Both are addressed here. - Introduce security/credentialRejection.ts: a module-private symbol tag set only where authentication concludes a credential is unacceptable. isCredentialRejection() reads nothing else, so a default-status-400 ClientError raised while lazily loading the user cache is a fault that fails closed rather than a deferrable unknown credential. - validateToken() converts only tagged JWT syntax, signature, expiry, not-before, subject and credential-state rejections; verification key material is validated up front and key-material JsonWebTokenErrors stay faults. Untagged errors propagate unmasked. - The Bearer refresh-token probe propagates a refresh-validation fault and restores the operation-token rejection only after an ordinary tagged refresh rejection. A fail-closed response logs the original fault and returns the generic authentication failure. - Settlement goes through settleDeferredCredentialRejection(), which returns the authentication middleware's own descriptor (401, {error: message} in the negotiated content type) ahead of REST's Problem Details and GraphQL's {errors:[...]} mapping. The built-in MCP HTTP adapter, static file serving and the MQTT WebSocket handler now settle too; MCP previously served an unrecognized credential as anonymous. - Bun and uWS merge the middleware chain's headers into the legacy Fastify fallback response, unioning Vary and preserving the private cache floor unless the final response opts into shared caching. - Deferred state is installed with a non-enumerable descriptor so object spread and Reflect.ownKeys cannot carry it into an application. Co-Authored-By: Claude Opus 5 (1M context) --- components/mcp/adapters/harperHttp.ts | 13 + .../deferred-credential-rejection.test.ts | 42 ++- .../deferred-credential-rejection/config.yaml | 3 + security/auth.ts | 35 ++- security/credentialRejection.ts | 40 +++ security/deferredAuthentication.ts | 66 +++-- security/tokenAuthentication.ts | 62 +++-- security/user.ts | 10 +- server/DESIGN.md | 74 +++++- server/REST.ts | 12 +- server/graphqlQuerying.ts | 17 +- server/http.ts | 39 ++- server/mqtt.ts | 13 + server/serverHelpers/Headers.ts | 64 +++++ server/static.ts | 6 + .../mcp/adapters/harperHttp.test.js | 77 ++++++ .../security/authCredentialDeferral.test.js | 106 +++++++- .../security/deferredAuthentication.test.js | 149 +++++++++-- .../tokenRejectionClassification.test.js | 213 +++++++++++++++ unitTests/server/fallbackCacheFloor.test.js | 245 ++++++++++++++++++ 20 files changed, 1172 insertions(+), 114 deletions(-) create mode 100644 security/credentialRejection.ts create mode 100644 unitTests/security/tokenRejectionClassification.test.js create mode 100644 unitTests/server/fallbackCacheFloor.test.js diff --git a/components/mcp/adapters/harperHttp.ts b/components/mcp/adapters/harperHttp.ts index 2e19a692db..c5f7a4f332 100644 --- a/components/mcp/adapters/harperHttp.ts +++ b/components/mcp/adapters/harperHttp.ts @@ -12,6 +12,7 @@ * automatically and pipes the iterable to the wire. */ import { handleMcpRequest, type McpProfile, type NormRequest, type NormResponse } from '../transport.ts'; +import { settleDeferredCredentialRejection } from '../../../security/deferredAuthentication.ts'; import { toSseStream, type SseFrameSource } from '../sse.ts'; /** @@ -42,6 +43,9 @@ interface HarperHttpRequest { ip?: string; } +/** The settled authentication response, when a deferred credential rejection decides the request. */ +type SettledCredentialRejection = { status: number; headers: unknown; body: string | Buffer }; + interface HarperHttpResponse { status: number; headers: Record; @@ -56,6 +60,15 @@ export function createHarperHttpHandler(profile: McpProfile) { // WebSocket upgrades aren't ours — let the next handler take it. if (request.isWebSocket) return nextHandler(request); + // This mount is Harper-owned, so route ownership is settled the moment we decline to delegate. + // The authentication middleware defers an unrecognized credential rather than answering it in + // line (#2418), and `request.user` is simply unset in that case — which `norm.user` below would + // map to `''`, i.e. anonymous, letting an invalid credential open an MCP session that the base + // revision answered with 401. Settled before the body is read or a session is created. + const settledCredentialRejection = settleDeferredCredentialRejection(request) as + SettledCredentialRejection | undefined; + if (settledCredentialRejection) return settledCredentialRejection; + const norm: NormRequest = { method: request.method, headers: normalizeHeaders(request.headers), diff --git a/integrationTests/security/deferred-credential-rejection.test.ts b/integrationTests/security/deferred-credential-rejection.test.ts index 49723e1c4f..53dc7058be 100644 --- a/integrationTests/security/deferred-credential-rejection.test.ts +++ b/integrationTests/security/deferred-credential-rejection.test.ts @@ -41,9 +41,9 @@ suite( let adminAuthorization = ''; /** Issues a raw request so the exact Authorization header under test reaches the wire unchanged. */ - async function get(pathname: string, authorization?: string) { + async function get(pathname: string, authorization?: string, extraHeaders: Record = {}) { const response = await fetch(`${restURL}${pathname}`, { - headers: authorization ? { Authorization: authorization } : {}, + headers: { ...(authorization ? { Authorization: authorization } : {}), ...extraHeaders }, }); const text = await response.text(); let body: any; @@ -52,7 +52,7 @@ suite( } catch { body = text; } - return { status: response.status, body, text }; + return { status: response.status, body, text, headers: response.headers }; } before(async () => { @@ -175,6 +175,42 @@ suite( ); }); + test('a rejected credential on a REST route keeps the authentication error envelope', async () => { + // The wire contract every caller has seen for a rejected credential: `{error: message}` in + // the request's negotiated serialization. REST's own error mapping renders a thrown error as + // an RFC 9457 Problem Details document (`type`/`title`/`status`), which is NOT this, so a + // settlement that went through REST's catch would silently change the response shape. + const response = await get(PROTECTED_ROUTE, WORDPRESS_BASIC, { Accept: 'application/json' }); + + equal(response.status, 401, `expected a generic unauthorized: ${response.text}`); + equal(response.headers.get('content-type')?.split(';')[0], 'application/json'); + equal(typeof response.body?.error, 'string', `expected an {error} body, got: ${response.text}`); + ok(response.body.title === undefined, `expected no Problem Details envelope, got: ${response.text}`); + ok(response.body.errors === undefined, `expected no GraphQL error envelope, got: ${response.text}`); + }); + + test('a rejected credential on /graphql keeps the same envelope, not GraphQL errors', async () => { + const response = await fetch(`${restURL}/graphql?query=%7B__typename%7D`, { + headers: { Authorization: WORDPRESS_BASIC, Accept: 'application/json' }, + }); + const text = await response.text(); + + equal(response.status, 401, `expected a generic unauthorized: ${text}`); + equal(response.headers.get('content-type')?.split(';')[0], 'application/json'); + const body = JSON.parse(text); + equal(typeof body.error, 'string', `expected an {error} body, got: ${text}`); + ok(body.errors === undefined, `expected no GraphQL {errors:[...]} envelope, got: ${text}`); + }); + + test('/graphql still answers an anonymous request normally', async () => { + // The contrast case: without a credential to reject, GraphQL's own handling is untouched. + const response = await fetch(`${restURL}/graphql?query=%7B__typename%7D`, { + headers: { Accept: 'application/json' }, + }); + + ok(response.status !== 401, `an anonymous /graphql request must not be rejected: ${await response.text()}`); + }); + test('the operations API still rejects an unrecognized credential in place', async () => { // Every operations route is Harper-owned, so there is nothing to defer to and nothing changes. const response = await fetch(ctx.harper.operationsAPIURL, { diff --git a/integrationTests/security/fixtures/deferred-credential-rejection/config.yaml b/integrationTests/security/fixtures/deferred-credential-rejection/config.yaml index 87ce6a9a8a..a02f0f9526 100644 --- a/integrationTests/security/fixtures/deferred-credential-rejection/config.yaml +++ b/integrationTests/security/fixtures/deferred-credential-rejection/config.yaml @@ -9,4 +9,7 @@ graphqlSchema: jsResource: files: resources.js rest: true +# /graphql is Harper-owned too, so the deferred rejection has to be settled there with the same +# authentication error envelope REST returns — not GraphQL's own {errors:[...]} mapping. +graphql: true deferredAuthAppCatchAll: true diff --git a/security/auth.ts b/security/auth.ts index 9a979e9e22..a3e3d1dbf4 100644 --- a/security/auth.ts +++ b/security/auth.ts @@ -7,19 +7,20 @@ import { v4 as uuid } from 'uuid'; import * as env from '../utility/environment/environmentManager.ts'; import { CONFIG_PARAMS, AUTH_AUDIT_STATUS, AUTH_AUDIT_TYPES } from '../utility/hdbTerms.ts'; import harperLogger from '../utility/logging/harper_logger.ts'; -const { forComponent, AuthAuditLog } = harperLogger; +const { forComponent, AuthAuditLog, errorForLog } = harperLogger; import serverHandlers from '../server/itc/serverHandlers.js'; const { user } = serverHandlers; -import { Headers, addVaryHeader } from '../server/serverHelpers/Headers.ts'; +import { Headers, addVaryHeader, SHARED_CACHE_OPTIN, PRIVATE_SCOPE } from '../server/serverHelpers/Headers.ts'; import { convertToMS } from '../utility/common_utils.ts'; import { verifyCertificate } from './certificateVerification/index.ts'; import { + credentialRejectionError, deferCredentialRejection, getDeferredCredentialRejection, isCredentialRejection, } from './deferredAuthentication.ts'; import { serializeMessage } from '../server/serverHelpers/contentTypes.ts'; -import { ClientError, hdbErrors } from '../utility/errors/hdbError.ts'; +import { hdbErrors } from '../utility/errors/hdbError.ts'; const { AUTHENTICATION_ERROR_MSGS, HTTP_STATUS_CODES } = hdbErrors; const authLogger = forComponent('authentication'); const { debug } = authLogger; @@ -57,10 +58,6 @@ const LOG_AUTH_FAILED = env.get(CONFIG_PARAMS.LOGGING_AUDITAUTHEVENTS_LOGFAILED) const DEFAULT_COOKIE_EXPIRES = 'Tue, 01 Oct 8307 19:33:20 GMT'; -// RFC 9111 cache-scope directives; boundaries on both sides so a token like `public-foo` doesn't match -const SHARED_CACHE_OPTIN = /(^|[,\s])(public|s-maxage)($|[\s,;=])/i; -const PRIVATE_SCOPE = /(^|[,\s])(private|no-store)($|[\s,;=])/i; - let authorizationCache = new Map(); server.onInvalidatedUser(() => { // TODO: Eventually we probably want to be able to invalidate individual users @@ -245,7 +242,12 @@ export async function authentication(request, nextHandler) { // API has its own logic for handling this status: -1, }); - } catch { + } catch (refreshError) { + // A refresh-validation *fault* (user store down, a password-validation crash) + // must not be swallowed: rethrowing the outer ordinary rejection would tag an + // outage as a deferrable unknown credential. Only after an ordinary tagged + // refresh rejection is the original operation-token rejection restored. + if (!isCredentialRejection(refreshError)) throw refreshError; throw error; } } @@ -261,7 +263,10 @@ export async function authentication(request, nextHandler) { // that is precisely the downgrade this change exists to prevent. Rejecting it // here routes it through the same audit, fail-closed, and deferral handling as // an unrecognized Basic or Bearer credential. - throw new ClientError(AUTHENTICATION_ERROR_MSGS.GENERIC_AUTH_FAIL, HTTP_STATUS_CODES.UNAUTHORIZED); + throw credentialRejectionError( + AUTHENTICATION_ERROR_MSGS.GENERIC_AUTH_FAIL, + HTTP_STATUS_CODES.UNAUTHORIZED + ); } } catch (err) { if (LOG_AUTH_FAILED) { @@ -277,10 +282,18 @@ export async function authentication(request, nextHandler) { // - an internal fault (unreadable JWT keys, storage failure, a bug), which must fail // closed instead of letting an outage hand the request to application authorization; // - the operations API, where Harper owns every route, so there is nothing to defer to. - if (request.isOperationsServer || !isCredentialRejection(err)) { + const internalFault = !isCredentialRejection(err); + if (request.isOperationsServer || internalFault) { + // An internal fault's own message describes Harper's internals (a missing system + // table, a key path) and is not the client's to read, so it is logged here and the + // client gets the same generic failure a rejected credential gets. + if (internalFault) authLogger.error('Authentication failed internally', errorForLog(err)); return applyResponseHeaders({ status: 401, - body: serializeMessage({ error: err.message }, request), + body: serializeMessage( + { error: internalFault ? AUTHENTICATION_ERROR_MSGS.GENERIC_AUTH_FAIL : err.message }, + request + ), }); } credentialRejection = err; diff --git a/security/credentialRejection.ts b/security/credentialRejection.ts new file mode 100644 index 0000000000..c6122f4ce9 --- /dev/null +++ b/security/credentialRejection.ts @@ -0,0 +1,40 @@ +import { ClientError } from '../utility/errors/hdbError.ts'; + +/** + * Marks an error as "the presented credential is not acceptable", as opposed to "Harper could not + * evaluate the credential". Only the authentication code that actually reaches that conclusion sets + * it, so provenance is asserted at the throw site rather than inferred from a status code. + * + * Non-enumerable and symbol-keyed: it never serializes, never reaches a client, and cannot be set by + * anything that does not import this module. + */ +const CREDENTIAL_REJECTION = Symbol('harper.credentialRejection'); + +/** Tags an existing error as a positively identified credential rejection. Returns the same error. */ +export function markCredentialRejection(error: E): E { + Object.defineProperty(error, CREDENTIAL_REJECTION, { + value: true, + enumerable: false, + configurable: true, + writable: false, + }); + return error; +} + +/** Builds the tagged `ClientError` an authentication layer raises when it rejects a credential. */ +export function credentialRejectionError(message: string, statusCode: number): ClientError { + return markCredentialRejection(new ClientError(message, statusCode)); +} + +/** + * True only for an error explicitly tagged at the point authentication decided the credential itself + * is unacceptable. + * + * Provenance is never inferred from the status range. `ResourceBridge.searchByValue()` raises a + * default-status-400 `ClientError` when a system table is missing, and `findAndValidateUser()` + * reaches it while lazily loading the user cache — treating that 4xx as a rejected credential would + * let a storage outage hand a Harper request to an application's own authorization (#2418). + */ +export function isCredentialRejection(error: unknown): boolean { + return (error as Record | null | undefined)?.[CREDENTIAL_REJECTION] === true; +} diff --git a/security/deferredAuthentication.ts b/security/deferredAuthentication.ts index a9d58b0457..f801f9bfaf 100644 --- a/security/deferredAuthentication.ts +++ b/security/deferredAuthentication.ts @@ -1,4 +1,8 @@ import { ClientError } from '../utility/errors/hdbError.ts'; +import { serializeMessage, findBestSerializer } from '../server/serverHelpers/contentTypes.ts'; +import { Headers } from '../server/serverHelpers/Headers.ts'; + +export { isCredentialRejection, markCredentialRejection, credentialRejectionError } from './credentialRejection.ts'; /** * Request-local state recorded when `security/auth.ts` accepts a syntactically valid credential it @@ -25,25 +29,12 @@ export type DeferredCredentialRejection = { */ const CREDENTIAL_REJECTION_STATUS = 401; -/** - * Distinguishes an ordinary credential rejection — a well-formed credential Harper does not - * recognize — from an internal authentication fault such as unreadable JWT keys, a storage failure, - * or a bug. - * - * Only the former may be deferred. Deferring a fault would let an outage quietly downgrade a Harper - * request into one an application's own authorization decides, so anything that is not positively - * identifiable as a client-side rejection fails closed. Harper's authentication errors carry a 4xx - * `statusCode` (`ClientError`); an unexpected error type, a bare `Error`, and a 5xx all fall through - * to `false`. - */ -export function isCredentialRejection(error: unknown): boolean { - const status = (error as { statusCode?: unknown; status?: unknown })?.statusCode ?? (error as any)?.status; - return typeof status === 'number' && status >= 400 && status < 500; -} - /** * Records that this request presented a credential Harper rejected, without deciding the request. * The caller leaves `request.user` unset and the inbound `Authorization` header untouched. + * + * Installed non-enumerable so an application catch-all that spreads or `Reflect.ownKeys`-walks the + * request cannot observe it: object spread copies enumerable symbol-keyed properties. */ export function deferCredentialRejection(request: any, error: { message?: string }, strategy: string): void { const deferred: DeferredCredentialRejection = { @@ -51,7 +42,12 @@ export function deferCredentialRejection(request: any, error: { message?: string message: error?.message ?? 'Unauthorized', strategy, }; - request[DEFERRED_CREDENTIAL_REJECTION] = deferred; + Object.defineProperty(request, DEFERRED_CREDENTIAL_REJECTION, { + value: deferred, + enumerable: false, + configurable: true, + writable: true, + }); } export function getDeferredCredentialRejection(request: any): DeferredCredentialRejection | undefined { @@ -59,12 +55,38 @@ export function getDeferredCredentialRejection(request: any): DeferredCredential } /** - * Called by a layer that has just established Harper owns the route being served. A credential the - * authentication middleware deferred is decided here — where ownership is finally known — and never - * travels past a Harper-owned route to an application catch-all. + * The response an owning layer returns once it has established Harper owns the route: exactly the + * descriptor `security/auth.ts` used to return in-line, so the wire contract a rejected credential + * has always produced survives the move downstream. * - * Throws the same `ClientError` the authentication middleware would have produced in-line, so an - * owning layer's existing error path renders the identical unauthorized response. + * Owner-specific error mapping must not run first. REST renders a thrown error as an RFC 9457 + * Problem Details document and GraphQL as `{errors:[…]}`; before deferral existed, neither ever saw + * a rejected credential, because authentication answered `{error: message}` in the request's + * negotiated serialization before route matching (#2418). + * + * Returns `undefined` when nothing was deferred, so a caller can `return settled ?? …` inline. + */ +export function settleDeferredCredentialRejection( + request: any +): { status: number; headers: Headers; body: string | Buffer } | undefined { + const deferred = getDeferredCredentialRejection(request); + if (!deferred) return undefined; + // The negotiated serializer is the same one `serializeMessage` selects below; naming it in + // Content-Type keeps the body self-describing on a path that historically emitted none. + const contentType = (request?.headers ? findBestSerializer(request).type : undefined) ?? 'application/json'; + return { + status: deferred.status, + // A real Headers, not a plain object: the authentication middleware's own 401 post-processing + // calls `response.headers.set()` (WWW-Authenticate, or a Location when a login page is + // configured) on whatever an owning layer returns, and a plain object has no `set`. + headers: new Headers({ 'Content-Type': contentType }), + body: serializeMessage({ error: deferred.message }, request) as string | Buffer, + }; +} + +/** + * Throwing form of `settleDeferredCredentialRejection`, for owners with no response descriptor to + * return — a WebSocket upgrade closes the socket with a status-derived close code instead. */ export function assertNoDeferredCredentialRejection(request: any): void { const deferred = getDeferredCredentialRejection(request); diff --git a/security/tokenAuthentication.ts b/security/tokenAuthentication.ts index 636adf8548..50074d3685 100644 --- a/security/tokenAuthentication.ts +++ b/security/tokenAuthentication.ts @@ -10,7 +10,7 @@ import { SYSTEM_SCHEMA_NAME, SYSTEM_TABLE_NAMES, } from '../utility/hdbTerms.ts'; -import { ClientError, hdbErrors } from '../utility/errors/hdbError.ts'; +import { ClientError, ServerError, hdbErrors } from '../utility/errors/hdbError.ts'; const { HTTP_STATUS_CODES, AUTHENTICATION_ERROR_MSGS } = hdbErrors; import logger from '../utility/logging/harper_logger.ts'; import * as password from '../utility/password.ts'; @@ -23,6 +23,7 @@ import { markTokenAsWorkloadIdentity, } from './credentialProvenance.ts'; import { buildScopedTokenUser, syntheticRoleName } from './impersonation.ts'; +import { credentialRejectionError, isCredentialRejection } from './credentialRejection.ts'; import type { ImpersonatePayload } from '../server/operationsServer.ts'; import { expandOperationsPerms } from '../utility/operationPermissions.ts'; import { update } from '../dataLayer/insert.ts'; @@ -414,6 +415,7 @@ export async function validateLoginToken(token: string): Promise { async function validateToken(token: string, tokenType: string): Promise { try { const keys: JWTRSAKeys = await getJWTRSAKeys(); + assertUsableVerificationKey(keys.publicKey); // The OPERATION type also accepts scoped tokens, so the subject is checked after // verification rather than pinned in the verify options. const tokenVerified = jwt.verify( @@ -428,18 +430,18 @@ async function validateToken(token: string, tokenType: string): Promise { return buildUserFromScopedToken(tokenVerified); } if (tokenVerified.sub !== tokenType) { - throw new ClientError(AUTHENTICATION_ERROR_MSGS.INVALID_TOKEN, HTTP_STATUS_CODES.UNAUTHORIZED); + throw credentialRejectionError(AUTHENTICATION_ERROR_MSGS.INVALID_TOKEN, HTTP_STATUS_CODES.UNAUTHORIZED); } // If a role is present, it means the token is not an operation token. The validation of // the token will happen in the respective function/component that uses the token. if (tokenVerified.role) { - throw new ClientError(AUTHENTICATION_ERROR_MSGS.INVALID_TOKEN, HTTP_STATUS_CODES.UNAUTHORIZED); + throw credentialRejectionError(AUTHENTICATION_ERROR_MSGS.INVALID_TOKEN, HTTP_STATUS_CODES.UNAUTHORIZED); } const user: any = await findAndValidateUser(tokenVerified.username, undefined, false); if (tokenType === TOKEN_TYPE.REFRESH && !password.validate(user.refresh_token, token)) { - throw new ClientError(AUTHENTICATION_ERROR_MSGS.INVALID_TOKEN, HTTP_STATUS_CODES.UNAUTHORIZED); + throw credentialRejectionError(AUTHENTICATION_ERROR_MSGS.INVALID_TOKEN, HTTP_STATUS_CODES.UNAUTHORIZED); } // Surfaced as `tokenOperations` rather than merged into role.permission.operations: that field @@ -455,27 +457,53 @@ async function validateToken(token: string, tokenType: string): Promise { } catch (err) { logger.warn(err); if (err?.name === 'TokenExpiredError') { - throw new ClientError(AUTHENTICATION_ERROR_MSGS.TOKEN_EXPIRED, HTTP_STATUS_CODES.FORBIDDEN); + throw credentialRejectionError(AUTHENTICATION_ERROR_MSGS.TOKEN_EXPIRED, HTTP_STATUS_CODES.FORBIDDEN); } - // Only a client-side rejection may be reported as one. Everything else here — unreadable JWT - // keys (a 500 from getJWTRSAKeys), a storage failure inside findAndValidateUser, a bug — - // propagates unmasked, because callers now distinguish a rejected credential from an internal - // authentication fault and only the former is deferred past route matching (#2418). Masking a - // fault as `invalid token` would let a key or storage outage read as an unknown credential. + // Only a client-side rejection may be reported as one. Everything else here — unreadable or + // malformed JWT key material, a storage failure inside findAndValidateUser, a bug — propagates + // unmasked, because callers distinguish a rejected credential from an internal authentication + // fault and only the former is deferred past route matching (#2418). Masking a fault as + // `invalid token` would let a key or storage outage read as an unknown credential. if (!isTokenRejection(err)) throw err; - throw new ClientError(AUTHENTICATION_ERROR_MSGS.INVALID_TOKEN, HTTP_STATUS_CODES.UNAUTHORIZED); + throw credentialRejectionError(AUTHENTICATION_ERROR_MSGS.INVALID_TOKEN, HTTP_STATUS_CODES.UNAUTHORIZED); } } /** - * True when `err` says the presented token is not acceptable, rather than that Harper failed to - * evaluate it. Covers `jsonwebtoken`'s verification errors and Harper's own 4xx `ClientError`s. + * `jsonwebtoken` error names that describe the *token*: syntax, signature, subject/audience claims, + * and the not-before/expiry windows. Anything else it raises is about Harper's own configuration. + */ +const JWT_REJECTION_ERROR_NAMES = new Set(['JsonWebTokenError', 'NotBeforeError', 'TokenExpiredError']); +/** + * `jsonwebtoken` reports an unusable verification key through the same `JsonWebTokenError` type it + * uses for a bad token, distinguished only by message — either its own `secretOrPublicKey…` guards + * or a passed-through OpenSSL failure. Those are Harper-side faults and must never be reported to a + * client as a rejected credential. + */ +const KEY_MATERIAL_FAULT = /secretOrPublicKey|asymmetric key|PEM routines|^error:/i; + +/** + * True only when `err` says the presented token is not acceptable, rather than that Harper failed to + * evaluate it. Never inferred from the 4xx range: `findAndValidateUser()` lazily loads the user cache + * and can surface a default-status-400 `ClientError` from a missing system table, which is a storage + * fault wearing a client-error status (#2418). */ function isTokenRejection(err: any): boolean { - if (err?.name === 'JsonWebTokenError' || err?.name === 'NotBeforeError') return true; - const status = err?.statusCode ?? err?.status; - return typeof status === 'number' && status >= 400 && status < 500; + if (isCredentialRejection(err)) return true; + if (!JWT_REJECTION_ERROR_NAMES.has(err?.name)) return false; + return !KEY_MATERIAL_FAULT.test(String(err?.message ?? '')); +} + +/** + * Fails closed before `jwt.verify()` when the configured public key cannot be verification key + * material at all. Without this, `jsonwebtoken` folds the failure into a `JsonWebTokenError`, which + * is otherwise indistinguishable from a forged signature. + */ +function assertUsableVerificationKey(publicKey: unknown): void { + if (typeof publicKey !== 'string' || !publicKey.includes('-----BEGIN')) { + throw new ServerError(AUTHENTICATION_ERROR_MSGS.NO_ENCRYPTION_KEYS, HTTP_STATUS_CODES.INTERNAL_SERVER_ERROR); + } } /** @@ -486,7 +514,7 @@ function isTokenRejection(err: any): boolean { function buildUserFromScopedToken(claims: JwtPayload): User { const embedded = (claims.role as { permission?: Record })?.permission; if (!embedded || typeof embedded !== 'object' || Array.isArray(embedded) || typeof claims.username !== 'string') { - throw new ClientError(AUTHENTICATION_ERROR_MSGS.INVALID_TOKEN, HTTP_STATUS_CODES.UNAUTHORIZED); + throw credentialRejectionError(AUTHENTICATION_ERROR_MSGS.INVALID_TOKEN, HTTP_STATUS_CODES.UNAUTHORIZED); } const permission: Record = { ...embedded, super_user: false, cluster_user: false }; // Hashed from the server-side downgraded clone (before the _expandedOperations Set is attached), diff --git a/security/user.ts b/security/user.ts index 943cff4819..06e1eedec2 100644 --- a/security/user.ts +++ b/security/user.ts @@ -107,6 +107,7 @@ import { server } from '../server/Server.ts'; import * as terms from '../utility/hdbTerms.ts'; import { expandOperationsPerms } from '../utility/operationPermissions.ts'; import { activeSuperUserRemains } from './superUserGuard.ts'; +import { credentialRejectionError } from './credentialRejection.ts'; server.getUser = (username: string, password?: string | null): Promise => { return findAndValidateUser(username, password, password != null); @@ -420,11 +421,14 @@ async function findAndValidateUser(username: string, pw?: string | null, validat const userTmp = usersWithRolesMap.get(username); if (!userTmp) { if (!validatePassword) return { username }; - throw new ClientError(AUTHENTICATION_ERROR_MSGS.GENERIC_AUTH_FAIL, HTTP_STATUS_CODES.UNAUTHORIZED); + // Tagged as a credential rejection (#2418): callers must be able to tell "this credential is + // not acceptable" apart from a fault raised while loading the user cache above, which shares + // the 4xx range but must fail closed instead of deferring to application authorization. + throw credentialRejectionError(AUTHENTICATION_ERROR_MSGS.GENERIC_AUTH_FAIL, HTTP_STATUS_CODES.UNAUTHORIZED); } if (userTmp && !userTmp.active) - throw new ClientError(AUTHENTICATION_ERROR_MSGS.USER_INACTIVE, HTTP_STATUS_CODES.UNAUTHORIZED); + throw credentialRejectionError(AUTHENTICATION_ERROR_MSGS.USER_INACTIVE, HTTP_STATUS_CODES.UNAUTHORIZED); const user: User = { active: userTmp.active, @@ -449,7 +453,7 @@ async function findAndValidateUser(username: string, pw?: string | null, validat // argon2id hash validation is async so await it if it is a promise if (typeof validated === 'object' && (validated as Promise)?.then) validated = await validated; if (validated === true) passwordHashCache.set(pw, userTmp.password); - else throw new ClientError(AUTHENTICATION_ERROR_MSGS.GENERIC_AUTH_FAIL, HTTP_STATUS_CODES.UNAUTHORIZED); + else throw credentialRejectionError(AUTHENTICATION_ERROR_MSGS.GENERIC_AUTH_FAIL, HTTP_STATUS_CODES.UNAUTHORIZED); } } return user; diff --git a/server/DESIGN.md b/server/DESIGN.md index d5dea03cee..8a8bed1572 100644 --- a/server/DESIGN.md +++ b/server/DESIGN.md @@ -224,24 +224,62 @@ So a rejection is recorded rather than answered: `security/deferredAuthentication.ts`. The state lives behind a module-private `Symbol`: it is not a header, not a `Request` field, not enumerable, and cannot be forged or read from outside that module. -- **An internal authentication fault** — unreadable JWT keys, a storage failure, an unexpected error - type — is never deferred. `isCredentialRejection` only accepts an error carrying a 4xx status, so - anything else fails closed with the in-line 401. `tokenAuthentication.ts → validateToken` no longer - masks such faults as `invalid token`, which is what makes that distinction reachable. +- **An internal authentication fault** — unreadable or malformed JWT key material, a storage failure, + an unexpected error type — is never deferred, and fails closed with the in-line 401. - **The operations API** never defers: `request.isOperationsServer` short-circuits to the in-line 401, because every operations route is Harper-owned and there is nothing to defer to. -Any layer that establishes Harper owns the route then settles the deferred state before doing work: +**Rejection provenance is asserted, never inferred.** `security/credentialRejection.ts` holds a +module-private `Symbol` tag; only the code that actually concludes "this credential is unacceptable" +sets it, and `isCredentialRejection()` reads nothing else. Status ranges cannot carry that meaning: +`findAndValidateUser()` lazily loads the user cache, whose system-table searches raise a +default-status-400 `ClientError` when `system.hdb_role`/`system.hdb_user` is unavailable, so a 4xx +test would classify a storage outage as an unknown credential and hand it to application +authorization. The tag is set at exactly these points: + +| Tagged rejection | Where | +| ------------------------------------------------------------------ | ------------------------------------------ | +| unknown user, inactive user, bad password | `security/user.ts → findAndValidateUser()` | +| JWT syntax, signature, expiry, not-before, subject/claim rejection | `tokenAuthentication.ts → validateToken()` | +| refresh-token hash mismatch, malformed scoped-token claims | `tokenAuthentication.ts` | +| an `Authorization` scheme Harper does not implement | `security/auth.ts` | + +`validateToken()` separates the two in the same catch: `jsonwebtoken` reports unusable key material +through the very same `JsonWebTokenError` type it uses for a forged token, so the public key is +validated as key material before `jwt.verify()` runs and a residual key-material message is treated +as a fault. An untagged error propagates unmasked. + +The Bearer path's refresh-token probe follows the same rule. When operation-token validation says +`invalid token`, authentication retries the credential as a refresh token; a fault raised by that +retry propagates, and only an ordinary _tagged_ refresh rejection restores the original +operation-token rejection for deferral. An in-line fail-closed response logs the original fault +server-side and returns the same generic authentication failure a rejected credential gets, so +internal detail never reaches an unauthenticated client. -| Layer | Where | -| --------------------------- | ------------------------------------------------------------------ | -| `REST.ts → http()` | after `resources.getMatch` succeeds (and for the OpenAPI document) | -| `REST.ts` WebSocket handler | after `resources.getMatch(url, 'ws')` succeeds | -| `graphqlQuerying.ts` | after the `/graphql` prefix match, raised as its own `HTTPError` | +Any layer that establishes Harper owns the route then settles the deferred state before doing work: -Each renders the same generic 401 the middleware would have returned in-line — the deferred status -is pinned to 401 regardless of the underlying error's own status, so a 403 `token expired` reads -exactly as it did before. A Harper-owned route therefore behaves identically to the pre-deferral +| Layer | Where | +| --------------------------------------- | ------------------------------------------------------------------ | +| `REST.ts → http()` | after `resources.getMatch` succeeds (and for the OpenAPI document) | +| `REST.ts` WebSocket handler | after `resources.getMatch(url, 'ws')` succeeds | +| `graphqlQuerying.ts` | after the `/graphql` prefix match, ahead of its error mapping | +| `static.ts` | after a static file entry matches | +| `mqtt.ts` WebSocket handler | after the `mqtt` subprotocol claims the socket | +| `components/mcp/adapters/harperHttp.ts` | after the WebSocket hand-off, before the body is read | + +**Every Harper-owned handler registered `after: 'authentication'` owes this settlement**, because +declining to call `nextHandler` is precisely the moment ownership is settled. A handler that skips +it serves an unrecognized credential as anonymous — the MCP application mount did exactly that, and +returned 200 for an `initialize` the base revision answered with 401. + +Settlement goes through `settleDeferredCredentialRejection()`, which returns the response descriptor +`security/auth.ts` used to return in line: status 401 and `serializeMessage({error: message}, request)` +in the request's negotiated content type. That matters because an owner's own error mapping is not +that contract — REST renders a thrown error as an RFC 9457 Problem Details document and GraphQL as +`{errors:[{message}]}` — and a rejected credential never reached either before deferral existed. +`assertNoDeferredCredentialRejection()` is the throwing form, for a WebSocket upgrade that has no +descriptor to return. The deferred status is pinned to 401 regardless of the underlying error's own +status, so a 403 `token expired` reads exactly as it did before. A Harper-owned route therefore behaves identically to the pre-deferral build, protected or public: an unknown credential can never buy access an anonymous caller would have received, and can never reach an application catch-all. Only a URL that reached `nextHandler` — one no Harper route owns — carries the original header onward. @@ -249,6 +287,16 @@ have received, and can never reach an application catch-all. Only a URL that rea The contract is route-ownership-based, not path-based. There is no exemption list, no carrier header, no credential rename, and no pre-auth stripping shim. +**The identity cache floor survives the legacy Fastify fallbacks.** A response produced under a +deferred credential is credential-dependent (#1565), so `authentication` stamps +`Cache-Control: private, no-cache` and `Vary: Authorization, Cookie` on it. When the chain declines +a request (`status: -1`), Node carries those onto the `ServerResponse` before emitting `unhandled`, +but the Bun and uWS adapters used to rebuild their headers solely from Fastify's reply and drop +them. Before deferral an unrecognized credential could not reach a fallback at all, so this was +unreachable; now both adapters merge through `Headers.ts → mergeChainHeadersIntoFallback()` — +Fastify wins every header it set, `Vary` is unioned, and the private scope is re-applied unless the +final response explicitly opts into shared caching (`public`/`s-maxage`). + ### Response Cache-Control / Vary policy (#1518, #1565) Three tiers, applied in two places: diff --git a/server/REST.ts b/server/REST.ts index d1e29f9098..f7647e4114 100644 --- a/server/REST.ts +++ b/server/REST.ts @@ -15,7 +15,10 @@ import { CONFIG_PARAMS } from '../utility/hdbTerms.ts'; import { ASIDE_STAGING_DIR } from '../components/Application.ts'; import { COMPONENT_PREPARATION_LOCK_DIR } from '../components/componentPreparationLock.ts'; import { restartNeeded } from '../components/requestRestart.ts'; -import { assertNoDeferredCredentialRejection } from '../security/deferredAuthentication.ts'; +import { + assertNoDeferredCredentialRejection, + settleDeferredCredentialRejection, +} from '../security/deferredAuthentication.ts'; import { Request } from '../server/serverHelpers/Request.ts'; import { RequestTarget } from '../resources/RequestTarget'; @@ -227,7 +230,12 @@ async function http(request: Request, nextHandler, resources: Resources, httpOpt // document — so a credential the authentication middleware deferred is decided here rather // than travelling on to an application catch-all (#2418). Every path that reaches an // application instead returned via `nextHandler` above. - assertNoDeferredCredentialRejection(request); + // + // Returned as the authentication middleware's own response descriptor rather than thrown: a + // throw would be rendered by the catch below as an RFC 9457 Problem Details document, which is + // not the `{error: message}` body a rejected credential has always produced. + const settledCredentialRejection = settleDeferredCredentialRejection(request); + if (settledCredentialRejection) return settledCredentialRejection; if ((resource as any)?.isCaching) { const cacheControl = headersObject['cache-control']; if (cacheControl) { diff --git a/server/graphqlQuerying.ts b/server/graphqlQuerying.ts index 935aec8ee4..d17f4a3c6f 100644 --- a/server/graphqlQuerying.ts +++ b/server/graphqlQuerying.ts @@ -3,7 +3,7 @@ import type { RequestParams } from 'graphql-http'; import { getDeserializer } from './serverHelpers/contentTypes.ts'; import { resources } from '../resources/Resources.ts'; import logger from '../utility/logging/harper_logger.ts'; -import { getDeferredCredentialRejection } from '../security/deferredAuthentication.ts'; +import { settleDeferredCredentialRejection } from '../security/deferredAuthentication.ts'; // This code makes heavy use of the word "node" to refer to a node in the GraphQL AST. @@ -581,13 +581,16 @@ export function handleApplication(scope: import('../components/Scope.ts').Scope) return nextLayer(request); } + // Harper owns /graphql, so a credential the authentication middleware deferred is rejected + // here instead of reaching a resolver as an anonymous request (#2418). Settled ahead of the + // try/catch on purpose: the GraphQL error mapping below renders every failure as + // `{errors:[{message}]}` in a GraphQL media type, but a rejected credential has always been + // answered by authentication itself with `{error: message}` in the request's negotiated + // serialization, and that contract is what callers depend on. + const settledCredentialRejection = settleDeferredCredentialRejection(request); + if (settledCredentialRejection) return settledCredentialRejection; + try { - // Harper owns /graphql, so a credential the authentication middleware deferred is - // rejected here instead of reaching a resolver as an anonymous request (#2418). - // Raised as this file's own HTTPError so the status survives the handler's error - // mapping below, which would otherwise render an unrecognized Error as a 500. - const deferred = getDeferredCredentialRejection(request); - if (deferred) throw new HTTPError(deferred.message, deferred.status); // Await the `graphqlHandler` call here so that errors are caught. return await graphqlQueryingHandler(request as any); } catch (error) { diff --git a/server/http.ts b/server/http.ts index 3c853f7fe0..af44504ebc 100644 --- a/server/http.ts +++ b/server/http.ts @@ -18,7 +18,7 @@ import { createServer as createSecureServerHttp1 } from 'node:https'; import { createServer, IncomingMessage, validateHeaderName, validateHeaderValue } from 'node:http'; import { createServer as createNetServer } from 'node:net'; import { Request, BunRequest, UwsRequest, isBun } from './serverHelpers/Request.ts'; -import { appendHeader, Headers, toWriteHeadHeaders } from './serverHelpers/Headers.ts'; +import { appendHeader, Headers, mergeChainHeadersIntoFallback, toWriteHeadHeaders } from './serverHelpers/Headers.ts'; import { decodeProxyHeader, applyProxyHeader, @@ -490,7 +490,7 @@ export function httpServer(listener, options) { httpResponders[options?.runFirst ? 'unshift' : 'push'](entry); } else if (isBun) { // On Bun, store non-function listeners (e.g. Fastify's http.Server) for fallback delegation - fallbackServers[port] = listener; + registerFallbackServer(port, listener); } else if ((httpServers[port] as any)?.uws) { // uWS HTTP path (#914, HARPER_UWS_HTTP): the port is backed by uWebSockets.js, not a Node // http server, so a raw non-function listener (e.g. Fastify's http.Server via @@ -499,7 +499,7 @@ export function httpServer(listener, options) { // port. Divert it to the fallback map like the Bun path; makeUwsHandler delegates unhandled // requests to it via inject(). The { uws: true } marker is guaranteed present here: the // getServer(port) call above (same loop iteration) sets it before this branch runs. - fallbackServers[port] = listener; + registerFallbackServer(port, listener); } else { listener.isSecure = secure; registerServer(listener, port, false); @@ -984,7 +984,7 @@ function getHTTPServer(port: number, secure: boolean, options: ServerOptions) { * and a Fastify fallback is registered for the port, it delegates via inject() (see injectToFastify), * mirroring the Bun path — so legacy Fastify routes work behind uWS too. */ -function makeUwsHandler(port: number | string, isOperationsServer: boolean, requestQueueLimit?: number) { +export function makeUwsHandler(port: number | string, isOperationsServer: boolean, requestQueueLimit?: number) { // Build a fresh response descriptor rather than mutating what the chain returned: a handler may // return a WHATWG `Response` (read-only `status`/`body` accessors), which the Bun path also never // mutates. `headers` is normalized in place the same way the Bun path does. @@ -1020,6 +1020,12 @@ function makeUwsHandler(port: number | string, isOperationsServer: boolean, requ if (Array.isArray(v)) respHeaders.set(k, k.toLowerCase() === 'set-cookie' ? v : v.join(', ')); else respHeaders.set(k, String(v)); } + // Fastify's own headers win, but the chain's identity/cache floor is not discarded with + // them: authentication stamps `Cache-Control: private, no-cache` and + // `Vary: Authorization, Cookie` on a credential-dependent response, and a deferred + // credential can now reach this fallback (#2418, #1565). Node preserves these via + // `nodeResponse.setHeader` before emitting 'unhandled'; do the equivalent here. + mergeChainHeadersIntoFallback(headers, respHeaders); if (universalHeaders.length > 0) applyUniversalHeaders(respHeaders); logHttpRequest(request, injectResult.statusCode, requestId, performance.now() - startTime); const responseStream = injectResult.stream(); @@ -1043,6 +1049,9 @@ function makeUwsHandler(port: number | string, isOperationsServer: boolean, requ } logHttpRequest(request, 404, requestId, performance.now() - startTime); const notFoundHeaders = new Headers({ 'content-type': 'text/plain' }); + // A 404 produced under a credential is credential-dependent too, so it keeps the same floor + // the fallback branch above preserves. + mergeChainHeadersIntoFallback(headers, notFoundHeaders); if (universalHeaders.length > 0) applyUniversalHeaders(notFoundHeaders); return { status: 404, headers: notFoundHeaders, body: 'Not found\n' }; } @@ -1217,10 +1226,11 @@ function getBunHTTPServer(port: number, secure: boolean, options: ServerOptions) // Delegate to the fallback server (e.g. Fastify) via node:http compatibility. // We create a Node-compatible IncomingMessage/ServerResponse and emit 'request' // on the fallback server, then capture the response. - return await bunDelegateToNodeServer(fallbackServer, webRequest, request); + return await bunDelegateToNodeServer(fallbackServer, webRequest, request, response.headers); } logHttpRequest(request, 404, requestId, performance.now() - startTime); const notFoundHeaders = new globalThis.Headers(); + mergeChainHeadersIntoFallback(response.headers, notFoundHeaders); if (universalHeaders.length > 0) applyUniversalHeaders(notFoundHeaders); return new Response('Not found\n', { status: 404, headers: notFoundHeaders }); } @@ -1404,6 +1414,15 @@ let fastifyInstances: Record = {}; export function registerFastifyInstance(port: string | number, instance: any) { fastifyInstances[port] = instance; } + +/** + * Records the legacy Fastify `http.Server` the Bun and uWS adapters delegate an unhandled request to. + * Both runtimes divert a non-function `server.http()` listener here instead of binding it, because + * neither backs its port with a Node http server. + */ +export function registerFallbackServer(port: string | number, listener: any) { + fallbackServers[port] = listener; +} const INTERNAL_USER_HEADER = 'x-harper-internal-pre-auth-user'; /** @@ -1434,10 +1453,11 @@ function injectToFastify( return fastify.inject({ method: req.method, url: req.url, headers, payload: req.body, payloadAsStream: true }); } -async function bunDelegateToNodeServer( +export async function bunDelegateToNodeServer( nodeServer: any, webRequest: globalThis.Request, - bunRequest?: any + bunRequest?: any, + chainHeaders?: any ): Promise { // Check if there's a Fastify instance registered for this port (preferred path) for (const port in fallbackServers) { @@ -1465,6 +1485,10 @@ async function bunDelegateToNodeServer( if (webRequest.headers.get('connection')?.toLowerCase() === 'close') { webHeaders.set('connection', 'close'); } + // See mergeChainHeadersIntoFallback: Fastify's headers win, but authentication's + // identity/cache floor on a credential-dependent response is preserved rather than dropped + // with the rest of the chain response (#2418, #1565). + mergeChainHeadersIntoFallback(chainHeaders, webHeaders); if (universalHeaders.length > 0) applyUniversalHeaders(webHeaders); const responseStream = injectResult.stream(); // Event-stream responses (MCP SSE) must reach the client incrementally — return @@ -1499,6 +1523,7 @@ async function bunDelegateToNodeServer( } // No Fastify instance found — return 404 const notFoundHeaders = new globalThis.Headers(); + mergeChainHeadersIntoFallback(chainHeaders, notFoundHeaders); if (universalHeaders.length > 0) applyUniversalHeaders(notFoundHeaders); return new Response('Not found\n', { status: 404, headers: notFoundHeaders }); } diff --git a/server/mqtt.ts b/server/mqtt.ts index 682df230a0..1aa445c29b 100644 --- a/server/mqtt.ts +++ b/server/mqtt.ts @@ -19,6 +19,10 @@ import { forComponent as loggerForComponent } from '../utility/logging/harper_lo import { EventEmitter } from 'events'; import { verifyCertificate } from '../security/certificateVerification/index.ts'; import { registerShutdownDrain } from '../components/shutdownDrain.ts'; +import { getDeferredCredentialRejection } from '../security/deferredAuthentication.ts'; + +/** RFC 6455 private-use close code Harper already maps HTTP 401 to (see server/REST.ts). */ +const WEBSOCKET_UNAUTHORIZED_CLOSE_CODE = 3000; const authEventLog = loggerWithTag('auth-event'); const mqttLog = loggerForComponent('mqtt'); @@ -66,6 +70,15 @@ export function handleApplication(scope: import('../components/Scope.ts').Scope) return next(ws, request, chainCompletion); } + // Declining to delegate settles route ownership: this socket is Harper's. A credential the + // authentication middleware deferred rather than answering in line (#2418) leaves + // `request.user` unset, which would otherwise open an anonymous MQTT session where the + // base revision returned 401 before the upgrade. + const deferred = getDeferredCredentialRejection(request); + if (deferred) { + return ws.close(WEBSOCKET_UNAUTHORIZED_CLOSE_CODE, deferred.message); + } + emitEvent('connection', ws); mqttLog.debug?.('Received WebSocket connection for MQTT from', ws._socket.remoteAddress); const { onMessage, onClose } = onSocket( diff --git a/server/serverHelpers/Headers.ts b/server/serverHelpers/Headers.ts index a7ee3a20d5..e320a350ce 100644 --- a/server/serverHelpers/Headers.ts +++ b/server/serverHelpers/Headers.ts @@ -159,3 +159,67 @@ export function toWriteHeadHeaders(headers: any): any { } return result; } + +// RFC 9111 cache-scope directives; boundaries on both sides so a token like `public-foo` doesn't match. +export const SHARED_CACHE_OPTIN = /(^|[,\s])(public|s-maxage)($|[\s,;=])/i; +export const PRIVATE_SCOPE = /(^|[,\s])(private|no-store)($|[\s,;=])/i; + +function headerValueString(value: unknown): string { + if (value == null) return ''; + return Array.isArray(value) ? value.join(', ') : String(value); +} + +/** + * Folds the middleware chain's response headers into the headers a fallback server produced for the + * same request. + * + * When the chain declines a request (`status: -1`) the Bun and uWS adapters hand it to legacy Fastify + * and build their response headers solely from Fastify's reply, dropping everything the chain had + * already decided. The Node adapter does not: it copies the chain headers onto the `ServerResponse` + * before emitting `unhandled`. That divergence used to be invisible, because a request carrying an + * unrecognized credential never reached a fallback — authentication answered it in line. Deferral + * (#2418) makes it reachable, and with it the identity floor authentication stamps on a + * credential-dependent response (`Cache-Control: private, no-cache`, `Vary: Authorization, Cookie` + * — #1565). + * + * Fastify wins every header it actually set; the chain only fills gaps. `Vary` is unioned rather than + * replaced, and the chain's private cache scope is re-applied unless the final response explicitly + * opts into shared caching (`public`/`s-maxage`), which is RFC 9111's opt-in and the same signal + * `security/auth.ts` honours. + */ +export function mergeChainHeadersIntoFallback< + T extends { + get(name: string): any; + set(name: string, value: any): any; + has(name: string): boolean; + append?(name: string, value: any): any; + }, +>(chainHeaders: any, finalHeaders: T): T { + if (!chainHeaders?.[Symbol.iterator]) return finalHeaders; + const chainVary = headerValueString(chainHeaders.get('Vary')); + const chainCacheControl = headerValueString(chainHeaders.get('Cache-Control')); + for (const [name, value] of chainHeaders) { + const lowerName = String(name).toLowerCase(); + if (lowerName === 'vary' || lowerName === 'cache-control') continue; + if (finalHeaders.has(name)) continue; + if (Array.isArray(value)) { + // Set-Cookie is the multi-valued case that must never be comma-joined. + for (const single of value) appendHeader(finalHeaders, name, single, lowerName !== 'set-cookie'); + } else finalHeaders.set(name, value); + } + for (const token of chainVary.split(',')) { + const trimmed = token.trim(); + if (trimmed) addVaryHeader(finalHeaders as any, trimmed); + } + if (chainCacheControl) { + const finalCacheControl = headerValueString(finalHeaders.get('Cache-Control')); + if (!finalCacheControl) finalHeaders.set('Cache-Control', chainCacheControl); + else if ( + PRIVATE_SCOPE.test(chainCacheControl) && + !PRIVATE_SCOPE.test(finalCacheControl) && + !SHARED_CACHE_OPTIN.test(finalCacheControl) + ) + finalHeaders.set('Cache-Control', finalCacheControl + ', private'); + } + return finalHeaders; +} diff --git a/server/static.ts b/server/static.ts index 80e7bf3ed0..b33d3b4572 100644 --- a/server/static.ts +++ b/server/static.ts @@ -5,6 +5,7 @@ import { resolveBaseURLPath } from '../components/resolveBaseURLPath.ts'; import { convertToMS } from '../utility/common_utils.ts'; import { isMatch } from 'micromatch'; import send from 'send'; +import { settleDeferredCredentialRejection } from '../security/deferredAuthentication.ts'; /** * The static plugin handles serving static files from the respective application directory. @@ -393,6 +394,11 @@ export function handleApplication(scope: Scope) { // If an entry matched, serve it if (staticFile) { + // Harper owns this URL, so a credential the authentication middleware deferred rather than + // answering in line (#2418) is settled here — otherwise an unrecognized credential would + // be served static content that the base revision answered with 401. + const settledCredentialRejection = settleDeferredCredentialRejection(req); + if (settledCredentialRejection) return settledCredentialRejection; // The benefit to using `send` is that it handles a lot of edge cases and headers for us. return { handlesHeaders: true, diff --git a/unitTests/components/mcp/adapters/harperHttp.test.js b/unitTests/components/mcp/adapters/harperHttp.test.js index c80fc25c57..9414811f5e 100644 --- a/unitTests/components/mcp/adapters/harperHttp.test.js +++ b/unitTests/components/mcp/adapters/harperHttp.test.js @@ -2,6 +2,7 @@ const assert = require('node:assert'); const { Readable } = require('node:stream'); const { EventEmitter } = require('node:events'); const { createHarperHttpHandler } = require('#src/components/mcp/adapters/harperHttp'); +const { credentialRejectionError, deferCredentialRejection } = require('#src/security/deferredAuthentication'); const { _setSessionTableForTest, loadSession } = require('#src/components/mcp/session'); const { Headers } = require('#src/server/serverHelpers/Headers'); @@ -98,6 +99,82 @@ describe('mcp/adapters/harperHttp', () => { assert.equal(parsed.result.protocolVersion, '2025-06-18'); }); + describe('deferred credential rejection (#2418)', () => { + /** + * `mcp.application` mounts this handler `after: 'authentication'`, and REST declines an + * unmatched `/mcp`, so this handler is where route ownership is finally known. Authentication + * now defers an unrecognized credential instead of answering 401 in line, which leaves + * `request.user` unset — the same shape as an anonymous request. Without settlement here, an + * invalid credential opened an MCP session that the base revision answered with 401. + */ + function deferredRequest(overrides = {}) { + const request = { + method: 'POST', + headers: makeHeaders({ 'content-type': 'application/json' }), + body: bodyStream( + JSON.stringify({ jsonrpc: '2.0', id: 1, method: 'initialize', params: { protocolVersion: '2025-06-18' } }) + ), + ...overrides, + }; + deferCredentialRejection(request, credentialRejectionError('Login failed', 401), 'Basic'); + return request; + } + + it('answers 401 instead of creating an anonymous session', async () => { + const handler = createHarperHttpHandler('application'); + + const result = await handler(deferredRequest(), next); + + assert.equal(result.status, 401); + assert.deepEqual(JSON.parse(result.body.toString()), { error: 'Login failed' }); + assert.equal(result.headers['Mcp-Session-Id'], undefined); + }); + + it('does not read the request body before rejecting', async () => { + const handler = createHarperHttpHandler('application'); + let bodyRead = false; + const body = { + on(event, listener) { + if (event === 'data') bodyRead = true; + if (event === 'end') queueMicrotask(listener); + return body; + }, + }; + + await handler(deferredRequest({ body }), next); + + assert.equal(bodyRead, false, 'the body must not be consumed once the credential is rejected'); + }); + + it('still serves a request with no deferred rejection', async () => { + // The contrast case: identical request minus the deferral produces a real session. + const handler = createHarperHttpHandler('application'); + const request = { + method: 'POST', + headers: makeHeaders({ 'content-type': 'application/json' }), + body: bodyStream( + JSON.stringify({ jsonrpc: '2.0', id: 1, method: 'initialize', params: { protocolVersion: '2025-06-18' } }) + ), + user: { username: 'alice' }, + }; + + const result = await handler(request, next); + + assert.equal(result.status, 200); + assert.match(result.headers['Mcp-Session-Id'], /^[0-9a-f-]{36}$/); + }); + + it('still lets a WebSocket upgrade through to the next handler', async () => { + // Upgrades are not this handler's route, so ownership is not settled here and the deferred + // rejection is left for whichever layer does own the socket. + const handler = createHarperHttpHandler('application'); + + const out = await handler(deferredRequest({ method: 'GET', isWebSocket: true }), () => 'next-handler-result'); + + assert.equal(out, 'next-handler-result'); + }); + }); + it('hands off WebSocket-upgrade requests to the next handler', async () => { const handler = createHarperHttpHandler('application'); const out = await handler( diff --git a/unitTests/security/authCredentialDeferral.test.js b/unitTests/security/authCredentialDeferral.test.js index 925ba53a9b..bffbda59ce 100644 --- a/unitTests/security/authCredentialDeferral.test.js +++ b/unitTests/security/authCredentialDeferral.test.js @@ -12,7 +12,7 @@ testUtils.preTestPrep(); const { makeCallbackChain } = require('#src/server/middlewareChain'); const { Headers } = require('#src/server/serverHelpers/Headers'); -const { assertNoDeferredCredentialRejection } = require('#src/security/deferredAuthentication'); +const { credentialRejectionError, settleDeferredCredentialRejection } = require('#src/security/deferredAuthentication'); const { ClientError, ServerError } = require('#src/utility/errors/hdbError'); const serverModule = require('#src/server/Server'); const tokenAuthentication = require('#src/security/tokenAuthentication'); @@ -65,11 +65,10 @@ describe('deferred credential rejection through the app-port middleware chain', function restLayer(request, nextHandler) { if (!ownedPaths.has(request.pathname)) return nextHandler(request); trace.push('rest'); - try { - assertNoDeferredCredentialRejection(request); - } catch (error) { - return { status: error.statusCode, headers: new Headers(), body: JSON.stringify({ error: error.message }) }; - } + // The production settlement helper, not a local re-implementation: it is what decides the + // status, body, and content type an owning Harper layer returns. + const settled = settleDeferredCredentialRejection(request); + if (settled) return settled; if (!request.user && request.pathname !== HARPER_OWNED_PUBLIC) return { status: 401, headers: new Headers(), body: JSON.stringify({ error: 'Login failed' }) }; return { @@ -114,17 +113,19 @@ describe('deferred credential rejection through the app-port middleware chain', originalValidateOperationToken = tokenAuthentication.validateOperationToken; originalValidateRefreshToken = tokenAuthentication.validateRefreshToken; + // The stubs raise what production raises: `findAndValidateUser()` and `validateToken()` tag a + // rejected credential explicitly, and an untagged error is by construction an internal fault. serverModule.server.getUser = async (username, password) => { if (getUserFault) throw getUserFault; const user = knownUsers.get(`${username}:${password}`); - if (!user) throw new ClientError('Login failed', 401); + if (!user) throw credentialRejectionError('Login failed', 401); return user; }; tokenAuthentication.validateOperationToken = async () => { - throw new ClientError('invalid token', 401); + throw credentialRejectionError('invalid token', 401); }; tokenAuthentication.validateRefreshToken = async () => { - throw new ClientError('invalid token', 401); + throw credentialRejectionError('invalid token', 401); }; }); @@ -196,7 +197,7 @@ describe('deferred credential rejection through the app-port middleware chain', assert.deepStrictEqual(trace, []); } finally { tokenAuthentication.validateRefreshToken = async () => { - throw new ClientError('invalid token', 401); + throw credentialRejectionError('invalid token', 401); }; } }); @@ -220,7 +221,7 @@ describe('deferred credential rejection through the app-port middleware chain', it('reports an expired Harper token as 401 at a Harper-owned route, as it did before deferral', async () => { tokenAuthentication.validateOperationToken = async () => { - throw new ClientError('token expired', 403); + throw credentialRejectionError('token expired', 403); }; try { const { response, body } = await send(HARPER_OWNED, 'Bearer expired-harper-token'); @@ -230,7 +231,7 @@ describe('deferred credential rejection through the app-port middleware chain', assert.deepStrictEqual(trace, ['rest']); } finally { tokenAuthentication.validateOperationToken = async () => { - throw new ClientError('invalid token', 401); + throw credentialRejectionError('invalid token', 401); }; } }); @@ -326,6 +327,87 @@ describe('deferred credential rejection through the app-port middleware chain', assert.deepStrictEqual(trace, []); }); + it('fails closed on an internal fault that happens to carry a 4xx status', async () => { + // The exact production shape: `findAndValidateUser()` lazily loads the user cache, whose + // system-table searches reach `ResourceBridge.searchByValue()` and raise a `ClientError` with + // the default 400 status when `system.hdb_role`/`system.hdb_user` is unavailable. Classifying + // by status range read that as an ordinary unknown credential and deferred it, so an unowned + // URL reached the application catch-all during a storage outage. + getUserFault = new ClientError('Table system.hdb_role not found'); + assert.strictEqual(getUserFault.statusCode, 400, 'the fault must actually be in the 4xx range'); + + const { response } = await send(APP_OWNED, WORDPRESS_BASIC); + + assert.strictEqual(response.status, 401); + assert.deepStrictEqual(trace, [], 'a storage outage must never reach application authorization'); + }); + + it('returns a generic failure for an internal fault and does not echo its message', async () => { + getUserFault = new ClientError('Table system.hdb_role not found'); + + const { response, body } = await send(APP_OWNED, WORDPRESS_BASIC); + + assert.strictEqual(response.status, 401); + assert.strictEqual(body.error, 'Login failed'); + assert.ok(!JSON.stringify(body).includes('hdb_role'), 'internal detail must not reach the client'); + }); + + it('propagates a refresh-validation fault instead of restoring the deferrable outer rejection', async () => { + // The operation-token path falls back to refresh-token validation on `invalid token`. Discarding + // whatever that raises and rethrowing the outer ordinary rejection let a refresh-side storage or + // runtime fault be classified as a deferrable unknown credential. + tokenAuthentication.validateRefreshToken = async () => { + throw new ServerError('refresh token store unavailable'); + }; + try { + const { response, body } = await send(APP_OWNED, 'Bearer some-harper-looking-token'); + + assert.strictEqual(response.status, 401); + assert.strictEqual(body.error, 'Login failed'); + assert.deepStrictEqual(trace, [], 'a refresh-validation fault must not reach the catch-all'); + } finally { + tokenAuthentication.validateRefreshToken = async () => { + throw credentialRejectionError('invalid token', 401); + }; + } + }); + + it('propagates a refresh-validation fault that carries a 4xx status', async () => { + tokenAuthentication.validateRefreshToken = async () => { + throw new ClientError('Table system.hdb_user not found'); + }; + try { + const { response } = await send(APP_OWNED, 'Bearer some-harper-looking-token'); + + assert.strictEqual(response.status, 401); + assert.deepStrictEqual(trace, []); + } finally { + tokenAuthentication.validateRefreshToken = async () => { + throw credentialRejectionError('invalid token', 401); + }; + } + }); + + it('restores the operation-token rejection after an ordinary refresh rejection, and defers it', async () => { + // The other half of the same branch: an ordinary tagged refresh rejection still yields the + // original `invalid token`, which is deferrable. + const { response, body } = await send(APP_OWNED, DOWNSTREAM_BEARER); + + assert.strictEqual(response.status, 200); + assert.strictEqual(body.servedBy, 'catch-all'); + assert.strictEqual(body.authorization, DOWNSTREAM_BEARER); + }); + + it("answers a Harper-owned route with the authentication error envelope, not the owner's", async () => { + // `{error: message}` in the request's negotiated serialization is what authentication returned + // in line before deferral existed; REST's RFC 9457 Problem Details mapping must not replace it. + const { response, body } = await send(HARPER_OWNED, WORDPRESS_BASIC); + + assert.strictEqual(response.status, 401); + assert.deepStrictEqual(body, { error: 'Login failed' }); + assert.strictEqual(response.headers.get('Content-Type'), 'application/json'); + }); + it('never defers on the operations API, where Harper owns every route', async () => { const { response } = await send(APP_OWNED, WORDPRESS_BASIC, { isOperationsServer: true }); diff --git a/unitTests/security/deferredAuthentication.test.js b/unitTests/security/deferredAuthentication.test.js index 86bb16baa2..21b58af5e4 100644 --- a/unitTests/security/deferredAuthentication.test.js +++ b/unitTests/security/deferredAuthentication.test.js @@ -2,43 +2,105 @@ const assert = require('node:assert'); const { assertNoDeferredCredentialRejection, + credentialRejectionError, deferCredentialRejection, getDeferredCredentialRejection, isCredentialRejection, + markCredentialRejection, + settleDeferredCredentialRejection, } = require('#src/security/deferredAuthentication'); const { ClientError, ServerError } = require('#src/utility/errors/hdbError'); +/** A request shaped enough for content negotiation (`findBestSerializer` reads `headers.asObject`). */ +function requestAccepting(accept, extraHeaders = {}) { + const asObject = { ...extraHeaders }; + if (accept) asObject.accept = accept; + return { + method: 'GET', + url: '/Ledger/1', + headers: { asObject, get: (name) => asObject[name.toLowerCase()] }, + }; +} + describe('deferredAuthentication', () => { describe('isCredentialRejection', () => { - it('accepts Harper 4xx authentication ClientErrors', () => { - assert.strictEqual(isCredentialRejection(new ClientError('Login failed', 401)), true); - assert.strictEqual(isCredentialRejection(new ClientError('token expired', 403)), true); - assert.strictEqual(isCredentialRejection(new ClientError('invalid token', 400)), true); + it('accepts only an error tagged at the point authentication rejected the credential', () => { + assert.strictEqual(isCredentialRejection(credentialRejectionError('Login failed', 401)), true); + assert.strictEqual(isCredentialRejection(credentialRejectionError('token expired', 403)), true); + assert.strictEqual(isCredentialRejection(markCredentialRejection(new Error('invalid token'))), true); + }); + + it('never infers rejection from the 4xx range', () => { + // The regression this guards: `findAndValidateUser()` lazily loads the user cache, whose + // fixed system-table searches raise a default-status-400 ClientError when `system.hdb_role` + // or `system.hdb_user` is unavailable. Deferring that would hand a storage outage to an + // application's own authorization. + assert.strictEqual(isCredentialRejection(new ClientError('Table system.hdb_role not found')), false); + assert.strictEqual(isCredentialRejection(new ClientError('Login failed', 401)), false); + assert.strictEqual(isCredentialRejection(new ClientError('token expired', 403)), false); + assert.strictEqual(isCredentialRejection({ statusCode: 401 }), false); + assert.strictEqual(isCredentialRejection({ status: 401 }), false); }); it('rejects internal faults so they fail closed instead of deferring', () => { - // A missing-JWT-keys fault is a 500 ClientError in this codebase; it must never defer. assert.strictEqual(isCredentialRejection(new ClientError('no encryption keys', 500)), false); assert.strictEqual(isCredentialRejection(new ServerError('storage unavailable')), false); - // A bare Error carries no status at all — a bug or a driver failure, not a rejection. assert.strictEqual(isCredentialRejection(new Error('ENOENT')), false); assert.strictEqual(isCredentialRejection(new TypeError('Invalid character')), false); assert.strictEqual(isCredentialRejection(undefined), false); assert.strictEqual(isCredentialRejection(null), false); - // A non-numeric status must not be coerced into the 4xx window. - assert.strictEqual(isCredentialRejection({ statusCode: '401' }), false); }); - it('reads a plain `status` as well as `statusCode`', () => { - assert.strictEqual(isCredentialRejection({ status: 401 }), true); - assert.strictEqual(isCredentialRejection({ status: 503 }), false); + it('cannot be forged from outside the module', () => { + // The tag is a module-private symbol, so neither a string key nor a registered symbol works. + const forged = { + 'credentialRejection': true, + 'harper.credentialRejection': true, + [Symbol.for('harper.credentialRejection')]: true, + }; + + assert.strictEqual(isCredentialRejection(forged), false); + }); + + it('leaves the tag off the wire: it is neither enumerable nor serializable', () => { + const error = credentialRejectionError('Login failed', 401); + + assert.deepStrictEqual(Object.keys(error), ['statusCode']); + assert.deepStrictEqual(Object.getOwnPropertySymbols({ ...error }), []); + assert.strictEqual(JSON.stringify({ ...error }), '{"statusCode":401}'); }); }); describe('deferCredentialRejection', () => { - it('records the rejection without exposing it as an enumerable property', () => { + it('installs the state as a non-enumerable own property', () => { + const request = { headers: { authorization: 'Basic d3A6c2VjcmV0' } }; + deferCredentialRejection(request, credentialRejectionError('Login failed', 401), 'Basic'); + + const stateSymbol = Object.getOwnPropertySymbols(request).find( + (symbol) => symbol.description === 'harper.deferredCredentialRejection' + ); + assert.ok(stateSymbol, 'the deferred state should be recorded under its own symbol'); + const descriptor = Object.getOwnPropertyDescriptor(request, stateSymbol); + assert.strictEqual(descriptor.enumerable, false); + assert.strictEqual(descriptor.configurable, true); + }); + + it('does not survive object spread into a downstream application copy', () => { + // Object spread copies enumerable symbol-keyed properties, so an enumerable descriptor here + // would put internal authentication state into whatever an application catch-all builds + // from the request. + const request = { headers: { authorization: 'Basic d3A6c2VjcmV0' } }; + deferCredentialRejection(request, credentialRejectionError('Login failed', 401), 'Basic'); + + const copied = { ...request }; + + assert.deepStrictEqual(Object.getOwnPropertySymbols(copied), []); + assert.strictEqual(getDeferredCredentialRejection(copied), undefined); + }); + + it('stays out of Object.keys and JSON.stringify', () => { const request = { headers: { authorization: 'Basic d3A6c2VjcmV0' } }; - deferCredentialRejection(request, new ClientError('Login failed', 401), 'Basic'); + deferCredentialRejection(request, credentialRejectionError('Login failed', 401), 'Basic'); assert.deepStrictEqual(Object.keys(request), ['headers']); assert.strictEqual(JSON.stringify(request), '{"headers":{"authorization":"Basic d3A6c2VjcmV0"}}'); @@ -47,7 +109,7 @@ describe('deferredAuthentication', () => { it('leaves the inbound Authorization header byte-for-byte unchanged', () => { const authorization = 'Basic d29yZHByZXNzOmFiY2QgZWZnaCBpamtsIG1ub3AgcXJzdCB1dnd4'; const request = { headers: { authorization } }; - deferCredentialRejection(request, new ClientError('Login failed', 401), 'Basic'); + deferCredentialRejection(request, credentialRejectionError('Login failed', 401), 'Basic'); assert.strictEqual(request.headers.authorization, authorization); }); @@ -56,7 +118,7 @@ describe('deferredAuthentication', () => { // The authentication middleware has always answered a rejected credential with 401 // regardless of the error's own status, so a deferred rejection has to match that. const request = {}; - deferCredentialRejection(request, new ClientError('token expired', 403), 'Bearer'); + deferCredentialRejection(request, credentialRejectionError('token expired', 403), 'Bearer'); assert.deepStrictEqual(getDeferredCredentialRejection(request), { status: 401, @@ -74,7 +136,7 @@ describe('deferredAuthentication', () => { it('is readable through a proxy of the request, as the urlPath-mount chain produces', () => { const request = {}; - deferCredentialRejection(request, new ClientError('Login failed', 401), 'Basic'); + deferCredentialRejection(request, credentialRejectionError('Login failed', 401), 'Basic'); const proxied = new Proxy(request, { get: (target, prop) => Reflect.get(target, prop) }); assert.strictEqual(getDeferredCredentialRejection(proxied).status, 401); @@ -98,6 +160,59 @@ describe('deferredAuthentication', () => { }); }); + describe('settleDeferredCredentialRejection', () => { + it('returns undefined when nothing was deferred', () => { + assert.strictEqual(settleDeferredCredentialRejection(requestAccepting('application/json')), undefined); + }); + + it('returns a real Headers, which the middleware 401 post-processing writes into', () => { + // `security/auth.ts` calls `response.headers.set()` on whatever an owning layer returns — + // WWW-Authenticate, or a Location when a login page is configured. A plain object 500s there. + const request = requestAccepting('application/json'); + deferCredentialRejection(request, credentialRejectionError('Login failed', 401), 'Basic'); + + const settled = settleDeferredCredentialRejection(request); + + assert.strictEqual(typeof settled.headers.set, 'function'); + assert.doesNotThrow(() => settled.headers.set('WWW-Authenticate', 'Basic')); + assert.strictEqual(settled.headers.get('WWW-Authenticate'), 'Basic'); + }); + + it('reproduces the authentication middleware response: 401 with an {error} body', () => { + // This is the wire contract callers have always seen for a rejected credential. An owning + // layer's own error mapping (REST's RFC 9457 Problem Details, GraphQL's {errors:[…]}) must + // not replace it. + const request = requestAccepting('application/json'); + deferCredentialRejection(request, credentialRejectionError('Login failed', 401), 'Basic'); + + const settled = settleDeferredCredentialRejection(request); + + assert.strictEqual(settled.status, 401); + assert.strictEqual(settled.headers.get('Content-Type'), 'application/json'); + assert.deepStrictEqual(JSON.parse(settled.body.toString()), { error: 'Login failed' }); + }); + + it('serializes in the content type the request negotiated', () => { + const request = requestAccepting('application/cbor'); + deferCredentialRejection(request, credentialRejectionError('invalid token', 401), 'Bearer'); + + const settled = settleDeferredCredentialRejection(request); + + assert.strictEqual(settled.headers.get('Content-Type'), 'application/cbor'); + assert.ok(Buffer.isBuffer(settled.body), 'a CBOR body should be binary, not a JSON string'); + }); + + it('carries the underlying rejection message, not a fixed one', () => { + const request = requestAccepting('application/json'); + deferCredentialRejection(request, credentialRejectionError('token expired', 403), 'Bearer'); + + const settled = settleDeferredCredentialRejection(request); + + assert.strictEqual(settled.status, 401); + assert.deepStrictEqual(JSON.parse(settled.body.toString()), { error: 'token expired' }); + }); + }); + describe('assertNoDeferredCredentialRejection', () => { it('does nothing for a request with no deferred rejection', () => { assert.doesNotThrow(() => assertNoDeferredCredentialRejection({})); @@ -105,7 +220,7 @@ describe('deferredAuthentication', () => { it('throws the unauthorized ClientError an owning Harper layer renders', () => { const request = {}; - deferCredentialRejection(request, new ClientError('Login failed', 401), 'Basic'); + deferCredentialRejection(request, credentialRejectionError('Login failed', 401), 'Basic'); assert.throws( () => assertNoDeferredCredentialRejection(request), diff --git a/unitTests/security/tokenRejectionClassification.test.js b/unitTests/security/tokenRejectionClassification.test.js new file mode 100644 index 0000000000..febaad2f6d --- /dev/null +++ b/unitTests/security/tokenRejectionClassification.test.js @@ -0,0 +1,213 @@ +'use strict'; + +/** + * `validateToken()` decides whether a Bearer failure is "this token is not acceptable" or "Harper + * could not evaluate it". Only the first may be deferred past route matching (#2418), so this suite + * drives the real `validateOperationToken`/`validateRefreshToken` against real RSA key material and + * asserts the classification the authentication middleware then acts on. + * + * On the pre-fix revision the internal-fault cases here fail: every 4xx was converted to a tagged + * `invalid token`, so a user-store or key-material fault was indistinguishable from a forged token. + */ +const testUtils = require('../testUtils.js'); +testUtils.preTestPrep(); + +const assert = require('node:assert'); +const fs = require('node:fs'); +const path = require('node:path'); +const { generateKeyPairSync } = require('node:crypto'); +const jwt = require('jsonwebtoken'); + +const { JWT_ENUM, LICENSE_KEY_DIR_NAME } = require('#src/utility/hdbTerms'); +const env = require('#src/utility/environment/environmentManager'); +const { isCredentialRejection } = require('#src/security/deferredAuthentication'); +const { + clearJWTRSAKeysCache, + validateOperationToken, + validateRefreshToken, +} = require('#src/security/tokenAuthentication'); +const { setUsersWithRolesCache } = require('#src/security/user'); + +const keysDir = path.join(env.getHdbBasePath(), LICENSE_KEY_DIR_NAME); + +const { publicKey, privateKey } = generateKeyPairSync('rsa', { + modulusLength: 2048, + publicKeyEncoding: { type: 'spki', format: 'pem' }, + privateKeyEncoding: { type: 'pkcs8', format: 'pem' }, +}); +const otherKeyPair = generateKeyPairSync('rsa', { + modulusLength: 2048, + publicKeyEncoding: { type: 'spki', format: 'pem' }, + privateKeyEncoding: { type: 'pkcs8', format: 'pem' }, +}); + +function sign(claims, options = {}, key = privateKey) { + return jwt.sign(claims, key, { algorithm: 'RS256', ...options }); +} + +function writeKeys(publicKeyMaterial) { + fs.mkdirSync(keysDir, { recursive: true }); + fs.writeFileSync(path.join(keysDir, JWT_ENUM.JWT_PASSPHRASE_NAME), 'unused-for-verification'); + fs.writeFileSync(path.join(keysDir, JWT_ENUM.JWT_PRIVATE_KEY_NAME), privateKey); + fs.writeFileSync(path.join(keysDir, JWT_ENUM.JWT_PUBLIC_KEY_NAME), publicKeyMaterial); + clearJWTRSAKeysCache(); +} + +/** Captures the error `fn()` raises, so a resolving call fails loudly instead of silently passing. */ +async function raisedBy(fn) { + try { + await fn(); + } catch (error) { + return error; + } + assert.fail('expected the call to raise'); +} + +describe('token rejection versus internal authentication fault', () => { + before(async () => { + writeKeys(publicKey); + await setUsersWithRolesCache( + new Map([['known_user', { username: 'known_user', active: true, role: { permission: {} } }]]) + ); + }); + + after(async () => { + fs.rmSync(keysDir, { recursive: true, force: true }); + clearJWTRSAKeysCache(); + await setUsersWithRolesCache(new Map()); + }); + + beforeEach(() => writeKeys(publicKey)); + + describe('tagged credential rejections', () => { + it('classifies a syntactically malformed token as a rejection', async () => { + const error = await raisedBy(() => validateOperationToken('not-a-jwt')); + + assert.strictEqual(isCredentialRejection(error), true); + assert.strictEqual(error.message, 'invalid token'); + assert.strictEqual(error.statusCode, 401); + }); + + it('classifies a signature forged with another key as a rejection', async () => { + const forged = sign({ username: 'known_user' }, { subject: 'operation' }, otherKeyPair.privateKey); + + const error = await raisedBy(() => validateOperationToken(forged)); + + assert.strictEqual(isCredentialRejection(error), true); + assert.strictEqual(error.message, 'invalid token'); + }); + + it('classifies a wrong-subject token as a rejection', async () => { + // A refresh token replayed on the operation-token path. + const refreshToken = sign({ username: 'known_user' }, { subject: 'refresh' }); + + const error = await raisedBy(() => validateOperationToken(refreshToken)); + + assert.strictEqual(isCredentialRejection(error), true); + assert.strictEqual(error.message, 'invalid token'); + }); + + it('classifies an expired token as a rejection', async () => { + const expired = sign({ username: 'known_user' }, { subject: 'operation', expiresIn: '-1s' }); + + const error = await raisedBy(() => validateOperationToken(expired)); + + assert.strictEqual(isCredentialRejection(error), true); + assert.strictEqual(error.message, 'token expired'); + assert.strictEqual(error.statusCode, 403); + }); + + it('classifies a not-yet-valid token as a rejection', async () => { + const notYet = sign({ username: 'known_user' }, { subject: 'operation', notBefore: '1h' }); + + const error = await raisedBy(() => validateOperationToken(notYet)); + + assert.strictEqual(isCredentialRejection(error), true); + assert.strictEqual(error.message, 'invalid token'); + }); + + it('classifies a deactivated user as a credential-state rejection', async () => { + // The credential itself is unacceptable — the signature is genuine but the account is not + // usable — so this is a rejection, not a fault, even though it arises inside the user store. + await setUsersWithRolesCache(new Map([['retired_user', { username: 'retired_user', active: false }]])); + try { + const token = sign({ username: 'retired_user' }, { subject: 'operation' }); + + const error = await raisedBy(() => validateOperationToken(token)); + + assert.strictEqual(isCredentialRejection(error), true); + } finally { + await setUsersWithRolesCache( + new Map([['known_user', { username: 'known_user', active: true, role: { permission: {} } }]]) + ); + } + }); + + it('classifies a refresh token whose stored hash does not match as a rejection', async () => { + const refreshToken = sign({ username: 'known_user' }, { subject: 'refresh' }); + + const error = await raisedBy(() => validateRefreshToken(refreshToken)); + + assert.strictEqual(isCredentialRejection(error), true); + assert.strictEqual(error.message, 'invalid token'); + }); + + it('accepts a well-formed operation token, so the rejection cases mean something', async () => { + const valid = sign({ username: 'known_user' }, { subject: 'operation' }); + + const user = await validateOperationToken(valid); + + assert.strictEqual(user.username, 'known_user'); + }); + }); + + describe('internal faults', () => { + it('fails a non-PEM public key closed instead of reporting an invalid token', async () => { + writeKeys('this is not key material'); + const valid = sign({ username: 'known_user' }, { subject: 'operation' }); + + const error = await raisedBy(() => validateOperationToken(valid)); + + assert.strictEqual(isCredentialRejection(error), false); + assert.notStrictEqual(error.message, 'invalid token'); + assert.strictEqual(error.statusCode, 500); + }); + + it('fails a PEM-shaped but corrupt public key closed', async () => { + // The decisive case: `jsonwebtoken` reports unusable key material through the very same + // `JsonWebTokenError` type it uses for a forged token, so the name alone cannot classify it. + writeKeys('-----BEGIN PUBLIC KEY-----\nbm90LWEtcmVhbC1rZXk=\n-----END PUBLIC KEY-----\n'); + const valid = sign({ username: 'known_user' }, { subject: 'operation' }); + + const error = await raisedBy(() => validateOperationToken(valid)); + + assert.strictEqual(isCredentialRejection(error), false, `unexpectedly tagged: ${error.message}`); + assert.notStrictEqual(error.message, 'invalid token'); + }); + + it('propagates a user-store fault raised while resolving a validly signed token', async () => { + // `findAndValidateUser()` reaches storage through the user cache; a failure there is a + // Harper-side fault even when it arrives with a 4xx status. + const failingCache = { + get() { + const error = new Error('Table system.hdb_user not found'); + error.statusCode = 400; + throw error; + }, + }; + await setUsersWithRolesCache(failingCache); + try { + const valid = sign({ username: 'known_user' }, { subject: 'operation' }); + + const error = await raisedBy(() => validateOperationToken(valid)); + + assert.strictEqual(isCredentialRejection(error), false); + assert.strictEqual(error.message, 'Table system.hdb_user not found'); + } finally { + await setUsersWithRolesCache( + new Map([['known_user', { username: 'known_user', active: true, role: { permission: {} } }]]) + ); + } + }); + }); +}); diff --git a/unitTests/server/fallbackCacheFloor.test.js b/unitTests/server/fallbackCacheFloor.test.js new file mode 100644 index 0000000000..8585b953eb --- /dev/null +++ b/unitTests/server/fallbackCacheFloor.test.js @@ -0,0 +1,245 @@ +'use strict'; + +/** + * The Bun and uWS adapters hand a request the middleware chain declined (`status: -1`) to legacy + * Fastify and build their response headers from Fastify's reply. Node does not lose the chain's + * headers on that path — it copies them onto the `ServerResponse` before emitting 'unhandled' — and + * before #2418 the divergence was unreachable, because an unrecognized credential never survived + * authentication. Deferral makes it reachable, so the identity floor authentication stamps on a + * credential-dependent response (`Cache-Control: private, no-cache`, `Vary: Authorization, Cookie` + * — #1565) has to survive both fallbacks. + * + * These drive the real adapters (`makeUwsHandler`, `bunDelegateToNodeServer`) against a stub Fastify + * instance, not a re-implementation of their header assembly. + */ +const testUtils = require('../testUtils.js'); +testUtils.preTestPrep(); + +const assert = require('node:assert'); +const { Readable } = require('node:stream'); + +const { + bunDelegateToNodeServer, + httpServer, + makeUwsHandler, + registerFallbackServer, + registerFastifyInstance, +} = require('#src/server/http'); +const { Headers, mergeChainHeadersIntoFallback } = require('#src/server/serverHelpers/Headers'); + +const UWS_PORT = 19430; +const BUN_PORT = 19431; + +/** The headers `security/auth.ts` stamps on a response produced under a (deferred) credential. */ +function identityFloorHeaders() { + return new Headers({ 'Cache-Control': 'private, no-cache', 'Vary': 'Authorization, Cookie' }); +} + +/** A stub Fastify whose `inject()` answers with the given status/headers/body. */ +function fastifyReplying(statusCode, headers, body = 'ok') { + return { + inject: async () => ({ + statusCode, + headers, + stream: () => Readable.from([Buffer.from(body)]), + raw: {}, + }), + }; +} + +function uwsRequest(headers = {}) { + return { + method: 'GET', + url: '/wp-json/wc/v3/products', + pathname: '/wp-json/wc/v3/products', + headers: { asObject: headers, get: (name) => headers[name.toLowerCase()] }, + body: undefined, + }; +} + +function bunWebRequest() { + return new globalThis.Request('http://localhost/wp-json/wc/v3/products', { + method: 'GET', + headers: { authorization: 'Basic d29yZHByZXNzOnNlY3JldA==' }, + }); +} + +describe('legacy Fastify fallback preserves the chain cache floor', () => { + describe('uWS adapter', () => { + /** Registers a chain on UWS_PORT that declines with `chainHeaders`, and returns the handler. */ + function handlerDecliningWith(chainHeaders, fastify) { + httpServer(() => ({ status: -1, headers: chainHeaders, body: 'Not found' }), { + port: UWS_PORT, + name: `uwsFallbackDecline${UWS_PORT}`, + }); + registerFastifyInstance(UWS_PORT, fastify); + return makeUwsHandler(UWS_PORT, false); + } + + it('carries Cache-Control and Vary from the chain onto the Fastify response', async () => { + const handle = handlerDecliningWith( + identityFloorHeaders(), + fastifyReplying(200, { 'content-type': 'application/json' }, '{"products":[]}') + ); + + const response = await handle(uwsRequest({ authorization: 'Basic d29yZHByZXNzOnNlY3JldA==' })); + + assert.strictEqual(response.status, 200); + assert.strictEqual(response.headers.get('Cache-Control'), 'private, no-cache'); + assert.strictEqual(response.headers.get('Vary'), 'Authorization, Cookie'); + // Fastify's own headers are untouched. + assert.strictEqual(response.headers.get('content-type'), 'application/json'); + }); + + it('unions Vary rather than letting either side win outright', async () => { + const handle = handlerDecliningWith( + identityFloorHeaders(), + fastifyReplying(200, { 'vary': 'Accept-Encoding', 'content-type': 'text/plain' }) + ); + + const response = await handle(uwsRequest()); + + const vary = response.headers.get('Vary'); + for (const token of ['Accept-Encoding', 'Authorization', 'Cookie']) { + assert.ok(vary.includes(token), `Vary should include ${token}, got '${vary}'`); + } + }); + + it('re-applies the private scope when Fastify returns a cacheable response', async () => { + const handle = handlerDecliningWith( + identityFloorHeaders(), + fastifyReplying(200, { 'cache-control': 'max-age=600', 'content-type': 'text/html' }) + ); + + const response = await handle(uwsRequest()); + + assert.strictEqual(response.headers.get('Cache-Control'), 'max-age=600, private'); + }); + + it('honours an explicit shared-cache opt-in from the final response', async () => { + const handle = handlerDecliningWith( + identityFloorHeaders(), + fastifyReplying(200, { 'cache-control': 'public, max-age=600' }) + ); + + const response = await handle(uwsRequest()); + + assert.strictEqual(response.headers.get('Cache-Control'), 'public, max-age=600'); + }); + + it('keeps the floor on the 404 it produces when no fallback is registered', async () => { + httpServer(() => ({ status: -1, headers: identityFloorHeaders(), body: 'Not found' }), { + port: UWS_PORT + 5, + name: `uwsFallbackNoFastify${UWS_PORT}`, + }); + const handle = makeUwsHandler(UWS_PORT + 5, false); + + const response = await handle(uwsRequest()); + + assert.strictEqual(response.status, 404); + assert.strictEqual(response.headers.get('Cache-Control'), 'private, no-cache'); + assert.ok(response.headers.get('Vary').includes('Authorization')); + }); + }); + + describe('Bun adapter', () => { + it('carries the chain cache floor onto the delegated Fastify response', async () => { + const nodeServer = { bunFallback: 'uses reference identity' }; + registerFallbackServer(BUN_PORT, nodeServer); + registerFastifyInstance(BUN_PORT, fastifyReplying(200, { 'content-type': 'application/json' }, '{}')); + + const response = await bunDelegateToNodeServer( + nodeServer, + bunWebRequest(), + { user: undefined }, + identityFloorHeaders() + ); + + assert.strictEqual(response.status, 200); + assert.strictEqual(response.headers.get('cache-control'), 'private, no-cache'); + assert.strictEqual(response.headers.get('vary'), 'Authorization, Cookie'); + assert.strictEqual(response.headers.get('content-type'), 'application/json'); + }); + + it('unions Vary and re-applies the private scope over a cacheable Fastify response', async () => { + const nodeServer = { bunFallback: 'second instance' }; + registerFallbackServer(BUN_PORT + 1, nodeServer); + registerFastifyInstance( + BUN_PORT + 1, + fastifyReplying(200, { 'vary': 'Accept-Encoding', 'cache-control': 'max-age=600' }) + ); + + const response = await bunDelegateToNodeServer( + nodeServer, + bunWebRequest(), + { user: undefined }, + identityFloorHeaders() + ); + + assert.strictEqual(response.headers.get('cache-control'), 'max-age=600, private'); + const vary = response.headers.get('vary'); + for (const token of ['Accept-Encoding', 'Authorization', 'Cookie']) { + assert.ok(vary.includes(token), `Vary should include ${token}, got '${vary}'`); + } + }); + + it('leaves a response with no chain headers exactly as Fastify produced it', async () => { + const nodeServer = { bunFallback: 'third instance' }; + registerFallbackServer(BUN_PORT + 2, nodeServer); + registerFastifyInstance(BUN_PORT + 2, fastifyReplying(200, { 'cache-control': 'max-age=600' })); + + const response = await bunDelegateToNodeServer(nodeServer, bunWebRequest(), { user: undefined }, new Headers()); + + assert.strictEqual(response.headers.get('cache-control'), 'max-age=600'); + assert.strictEqual(response.headers.get('vary'), null); + }); + }); + + describe('mergeChainHeadersIntoFallback', () => { + it('never lets the chain overwrite a header the final response set', () => { + const chain = new Headers({ 'Content-Type': 'text/plain', 'X-From-Chain': 'yes' }); + const final = new Headers({ 'Content-Type': 'application/json' }); + + mergeChainHeadersIntoFallback(chain, final); + + assert.strictEqual(final.get('Content-Type'), 'application/json'); + assert.strictEqual(final.get('X-From-Chain'), 'yes'); + }); + + it('keeps a multi-valued Set-Cookie from the chain as separate values', () => { + const chain = new Headers(); + chain.set('Set-Cookie', ['a=1; Path=/', 'b=2; Path=/']); + const final = new Headers(); + + mergeChainHeadersIntoFallback(chain, final); + + assert.deepStrictEqual(final.get('Set-Cookie'), ['a=1; Path=/', 'b=2; Path=/']); + }); + + it('does not duplicate a Vary token the final response already declares', () => { + const chain = new Headers({ Vary: 'Authorization, Cookie' }); + const final = new Headers({ Vary: 'Authorization' }); + + mergeChainHeadersIntoFallback(chain, final); + + assert.strictEqual(final.get('Vary'), 'Authorization, Cookie'); + }); + + it('leaves an existing private scope alone rather than appending a second one', () => { + const chain = new Headers({ 'Cache-Control': 'private, no-cache' }); + const final = new Headers({ 'Cache-Control': 'no-store' }); + + mergeChainHeadersIntoFallback(chain, final); + + assert.strictEqual(final.get('Cache-Control'), 'no-store'); + }); + + it('is a no-op when the chain produced no headers at all', () => { + const final = new Headers({ 'Cache-Control': 'max-age=60' }); + + mergeChainHeadersIntoFallback(undefined, final); + + assert.strictEqual(final.get('Cache-Control'), 'max-age=60'); + }); + }); +}); From 6daf7b24f8f472204f19ac8eaf8307a7a91319a8 Mon Sep 17 00:00:00 2001 From: hdbjeff Date: Mon, 31 Aug 2026 15:15:16 -0700 Subject: [PATCH 05/12] test: resolve JWT test keys through installTestJwtKeys (#2418) tokenRejectionClassification.test.js resolved the JWT keys directory from env.getHdbBasePath() at module load and wrote the key files itself, then removed the whole shared keys directory in after(). Under full unit-suite ordering the base path in effect at module load is not the one getJWTRSAKeys() reads at test time, so every case in the suite failed with "no encryption keys" on all three Node unit jobs. Install the keys through testUtils.installTestJwtKeys(), which resolves the directory at call time and returns a cleanup scoped to the three files it wrote, and sign with the key material it installed. The internal-fault cases replace the installed public key in place and beforeEach() restores it, so no shared fixture outside those three files is created or deleted. Co-Authored-By: Claude Opus 5 (1M context) --- .../tokenRejectionClassification.test.js | 86 +++++++++++-------- 1 file changed, 48 insertions(+), 38 deletions(-) diff --git a/unitTests/security/tokenRejectionClassification.test.js b/unitTests/security/tokenRejectionClassification.test.js index febaad2f6d..0930519a05 100644 --- a/unitTests/security/tokenRejectionClassification.test.js +++ b/unitTests/security/tokenRejectionClassification.test.js @@ -28,30 +28,7 @@ const { } = require('#src/security/tokenAuthentication'); const { setUsersWithRolesCache } = require('#src/security/user'); -const keysDir = path.join(env.getHdbBasePath(), LICENSE_KEY_DIR_NAME); - -const { publicKey, privateKey } = generateKeyPairSync('rsa', { - modulusLength: 2048, - publicKeyEncoding: { type: 'spki', format: 'pem' }, - privateKeyEncoding: { type: 'pkcs8', format: 'pem' }, -}); -const otherKeyPair = generateKeyPairSync('rsa', { - modulusLength: 2048, - publicKeyEncoding: { type: 'spki', format: 'pem' }, - privateKeyEncoding: { type: 'pkcs8', format: 'pem' }, -}); - -function sign(claims, options = {}, key = privateKey) { - return jwt.sign(claims, key, { algorithm: 'RS256', ...options }); -} - -function writeKeys(publicKeyMaterial) { - fs.mkdirSync(keysDir, { recursive: true }); - fs.writeFileSync(path.join(keysDir, JWT_ENUM.JWT_PASSPHRASE_NAME), 'unused-for-verification'); - fs.writeFileSync(path.join(keysDir, JWT_ENUM.JWT_PRIVATE_KEY_NAME), privateKey); - fs.writeFileSync(path.join(keysDir, JWT_ENUM.JWT_PUBLIC_KEY_NAME), publicKeyMaterial); - clearJWTRSAKeysCache(); -} +const KNOWN_USER = new Map([['known_user', { username: 'known_user', active: true, role: { permission: {} } }]]); /** Captures the error `fn()` raises, so a resolving call fails loudly instead of silently passing. */ async function raisedBy(fn) { @@ -64,20 +41,57 @@ async function raisedBy(fn) { } describe('token rejection versus internal authentication fault', () => { + let removeJwtKeys; + let signingKey; + let publicKeyPath; + let installedPublicKey; + let otherKeyPair; + before(async () => { - writeKeys(publicKey); - await setUsersWithRolesCache( - new Map([['known_user', { username: 'known_user', active: true, role: { permission: {} } }]]) - ); + // The keys land in a directory another suite asserts is empty, so they must come back out — + // see testUtils.installTestJwtKeys. It also resolves the keys directory from the base path + // current at call time, which is what makes this suite order-independent in the full run. + removeJwtKeys = testUtils.installTestJwtKeys(); + clearJWTRSAKeysCache(); + + const keysDir = path.join(env.getHdbBasePath(), LICENSE_KEY_DIR_NAME); + publicKeyPath = path.join(keysDir, JWT_ENUM.JWT_PUBLIC_KEY_NAME); + installedPublicKey = fs.readFileSync(publicKeyPath, 'utf8'); + // Sign with the exact keys validateOperationToken will verify against. + signingKey = { + key: fs.readFileSync(path.join(keysDir, JWT_ENUM.JWT_PRIVATE_KEY_NAME), 'utf8'), + passphrase: fs.readFileSync(path.join(keysDir, JWT_ENUM.JWT_PASSPHRASE_NAME), 'utf8'), + }; + otherKeyPair = generateKeyPairSync('rsa', { + modulusLength: 2048, + publicKeyEncoding: { type: 'spki', format: 'pem' }, + privateKeyEncoding: { type: 'pkcs8', format: 'pem' }, + }); + + await setUsersWithRolesCache(new Map(KNOWN_USER)); }); after(async () => { - fs.rmSync(keysDir, { recursive: true, force: true }); + removeJwtKeys(); clearJWTRSAKeysCache(); await setUsersWithRolesCache(new Map()); }); - beforeEach(() => writeKeys(publicKey)); + // The internal-fault cases replace the installed public key in place; restore it so each case + // starts from usable key material without touching any file installTestJwtKeys does not own. + beforeEach(() => { + fs.writeFileSync(publicKeyPath, installedPublicKey); + clearJWTRSAKeysCache(); + }); + + function sign(claims, options = {}, key = signingKey) { + return jwt.sign(claims, key, { algorithm: 'RS256', ...options }); + } + + function replacePublicKey(publicKeyMaterial) { + fs.writeFileSync(publicKeyPath, publicKeyMaterial); + clearJWTRSAKeysCache(); + } describe('tagged credential rejections', () => { it('classifies a syntactically malformed token as a rejection', async () => { @@ -137,9 +151,7 @@ describe('token rejection versus internal authentication fault', () => { assert.strictEqual(isCredentialRejection(error), true); } finally { - await setUsersWithRolesCache( - new Map([['known_user', { username: 'known_user', active: true, role: { permission: {} } }]]) - ); + await setUsersWithRolesCache(new Map(KNOWN_USER)); } }); @@ -163,8 +175,8 @@ describe('token rejection versus internal authentication fault', () => { describe('internal faults', () => { it('fails a non-PEM public key closed instead of reporting an invalid token', async () => { - writeKeys('this is not key material'); const valid = sign({ username: 'known_user' }, { subject: 'operation' }); + replacePublicKey('this is not key material'); const error = await raisedBy(() => validateOperationToken(valid)); @@ -176,8 +188,8 @@ describe('token rejection versus internal authentication fault', () => { it('fails a PEM-shaped but corrupt public key closed', async () => { // The decisive case: `jsonwebtoken` reports unusable key material through the very same // `JsonWebTokenError` type it uses for a forged token, so the name alone cannot classify it. - writeKeys('-----BEGIN PUBLIC KEY-----\nbm90LWEtcmVhbC1rZXk=\n-----END PUBLIC KEY-----\n'); const valid = sign({ username: 'known_user' }, { subject: 'operation' }); + replacePublicKey('-----BEGIN PUBLIC KEY-----\nbm90LWEtcmVhbC1rZXk=\n-----END PUBLIC KEY-----\n'); const error = await raisedBy(() => validateOperationToken(valid)); @@ -204,9 +216,7 @@ describe('token rejection versus internal authentication fault', () => { assert.strictEqual(isCredentialRejection(error), false); assert.strictEqual(error.message, 'Table system.hdb_user not found'); } finally { - await setUsersWithRolesCache( - new Map([['known_user', { username: 'known_user', active: true, role: { permission: {} } }]]) - ); + await setUsersWithRolesCache(new Map(KNOWN_USER)); } }); }); From a7fc1368bf8af6a6ad04c96b6c6fe522bcc6b3d9 Mon Sep 17 00:00:00 2001 From: hdbjeff Date: Mon, 31 Aug 2026 16:11:02 -0700 Subject: [PATCH 06/12] fix: settle deferred credential rejection at every static-owned response (#2418) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The static handler settled a deferred credential rejection only on the ordinary file serve. When it is ordered `after: 'rest'` it runs downstream of authentication, so its other responses — the mount-root redirect, the registered-directory trailing-slash redirect, and both `fallthrough: false` not-found forms — answered a rejected credential as if it were anonymous. Before deferral existed those responses were unreachable for a rejected credential, because authentication returned 401 ahead of them. Settlement now happens immediately before each response this handler originates, with no URL exempted. The two index redirects are merged into a single branch so they share one settlement point; they were already mutually exclusive, since a `null` index entry is never the mount-root serve. The not-found settlement is placed ahead of `notFound` option validation and file resolution, so a rejected credential cannot turn a missing file or bad config into a 500 on a request the pre-deferral revision answered with 401. The actual `next(req)` fallthrough still leaves the rejection deferred: there Harper declines the URL, so a downstream owner or an application catch-all applies its own scheme to the untouched Authorization header. Co-Authored-By: Claude Opus 5 (1M context) --- server/static.ts | 68 +++++----- unitTests/server/static.test.js | 219 ++++++++++++++++++++++++++++++++ 2 files changed, 258 insertions(+), 29 deletions(-) diff --git a/server/static.ts b/server/static.ts index b33d3b4572..2c6bad4f13 100644 --- a/server/static.ts +++ b/server/static.ts @@ -312,6 +312,14 @@ export function handleApplication(scope: Scope) { }); scope.server.http( + // Ownership invariant (#2418): every response this handler originates — both index redirects, + // the served file, and both `fallthrough: false` not-found forms — is Harper answering for the + // URL, so a credential the authentication middleware deferred rather than rejecting in line is + // settled before that response is built, with no URL exempted. Settling before the response body + // is resolved also keeps a rejected credential from turning a missing file or bad `notFound` + // config into a 500 the pre-deferral revision answered with 401. Only the `next(req)` fallthrough + // leaves the deferral in place: there Harper declines the URL, so a downstream owner or an + // application catch-all applies its own scheme. (req, next) => { // TODO: Not sure if the isWebSocket check is still necessary if (req.method !== 'GET' || req.isWebSocket) return next(req); @@ -341,37 +349,39 @@ export function handleApplication(scope: Scope) { // The router strips both '/assets' and '/assets/' down to '/', so the mount root // must be disambiguated via the unstripped pathname (exposed by stripPrefix): // redirect the no-slash form so relative links on the index page resolve under - // the mount (#1583). Query string is preserved across both redirects; compute it - // lazily inside each branch so the common (non-redirect) index serve stays allocation-free. - // Gated on the EXTERNAL base path, not the plugin-local one: a root-level static - // plugin (baseURLPath === '/') still needs this redirect when the application - // itself carries a host/urlPath mount, since the client-visible mount root is then - // externalBaseURLPath, not '/' (review finding). - if (staticFile && req.pathname === '/' && externalBaseURLPath !== '/') { - const originalPathname: string | undefined = (req as any).originalPathname; - if (originalPathname && !originalPathname.endsWith('/')) { - const queryIndex = (req.url as string).indexOf('?'); - const query = queryIndex === -1 ? '' : (req.url as string).slice(queryIndex); - return { - status: 301, - headers: { - Location: externalBaseURLPath + query, - }, - }; - } - } + // the mount (#1583). Gated on the EXTERNAL base path, not the plugin-local one: a + // root-level static plugin (baseURLPath === '/') still needs this redirect when the + // application itself carries a host/urlPath mount, since the client-visible mount root + // is then externalBaseURLPath, not '/' (review finding). + // The other form is the `null` index entry — a registered directory redirecting to its + // trailing-slash form; req.pathname arrives with the mount prefix stripped, so the + // external path is rebuilt for the Location header (#1583). The two are mutually + // exclusive, since a `null` entry is never the mount-root serve. They share one branch + // so both settle a deferred credential rejection before redirecting; the query string + // is built inside it, keeping the common (non-redirect) index serve allocation-free. + const originalPathname: string | undefined = (req as any).originalPathname; + const redirectsMountRoot = !!( + staticFile && + req.pathname === '/' && + externalBaseURLPath !== '/' && + originalPathname && + !originalPathname.endsWith('/') + ); - // If `null`, redirect to trailing slash. req.pathname arrives with the mount - // prefix stripped, so rebuild the external path for the Location header (#1583) - if (staticFile === null) { - const externalPath = - externalBaseURLPath === '/' ? req.pathname : externalBaseURLPath.slice(0, -1) + req.pathname; + if (redirectsMountRoot || staticFile === null) { + const settledCredentialRejection = settleDeferredCredentialRejection(req); + if (settledCredentialRejection) return settledCredentialRejection; const queryIndex = (req.url as string).indexOf('?'); const query = queryIndex === -1 ? '' : (req.url as string).slice(queryIndex); + const location = redirectsMountRoot + ? externalBaseURLPath + query + : (externalBaseURLPath === '/' ? req.pathname : externalBaseURLPath.slice(0, -1) + req.pathname) + + '/' + + query; return { status: 301, headers: { - Location: externalPath + '/' + query, + Location: location, }, }; } @@ -394,9 +404,6 @@ export function handleApplication(scope: Scope) { // If an entry matched, serve it if (staticFile) { - // Harper owns this URL, so a credential the authentication middleware deferred rather than - // answering in line (#2418) is settled here — otherwise an unrecognized credential would - // be served static content that the base revision answered with 401. const settledCredentialRejection = settleDeferredCredentialRejection(req); if (settledCredentialRejection) return settledCredentialRejection; // The benefit to using `send` is that it handles a lot of edge cases and headers for us. @@ -411,7 +418,10 @@ export function handleApplication(scope: Scope) { return next(req); } - // Otherwise, handle not found + // Otherwise, handle not found — settled once here, covering both the built-in 404 and the + // configured `notFound` response below. + const settledCredentialRejection = settleDeferredCredentialRejection(req); + if (settledCredentialRejection) return settledCredentialRejection; const notFound = scope.options.get(['notFound']); diff --git a/unitTests/server/static.test.js b/unitTests/server/static.test.js index 3e2452221a..a230e9da1e 100644 --- a/unitTests/server/static.test.js +++ b/unitTests/server/static.test.js @@ -5,6 +5,11 @@ const { mkdtempSync, mkdirSync, writeFileSync, rmSync } = require('node:fs'); const { join } = require('node:path'); const { tmpdir } = require('node:os'); const { handleApplication } = require('#src/server/static'); +const { + credentialRejectionError, + deferCredentialRejection, + getDeferredCredentialRejection, +} = require('#src/security/deferredAuthentication'); // A minimal Scope stand-in: captures the http registration and warning log so tests can // assert on the middleware ordering options the plugin passes to the server. @@ -399,3 +404,217 @@ describe('static plugin mount-root redirect', () => { assert.notEqual(result.status, 301); }); }); + +// A request shaped enough for both the static handler and the settled-rejection serializer +// (`findBestSerializer` reads `headers.asObject`). +function staticRequest(pathname, { url = pathname, originalPathname, authorization } = {}) { + const asObject = { accept: 'application/json' }; + if (authorization) asObject.authorization = authorization; + return { + method: 'GET', + isWebSocket: false, + pathname, + url, + originalPathname, + headers: { asObject, get: (name) => asObject[name.toLowerCase()] }, + }; +} + +const NEXT_HANDLER = Symbol('next handler'); +const next = () => NEXT_HANDLER; + +function assertSettledUnauthorized(result, request, authorization) { + assert.equal(result.status, 401, 'a static-owned response must settle the deferred rejection'); + assert.equal(result.headers.get('Content-Type'), 'application/json'); + assert.deepStrictEqual(JSON.parse(result.body.toString()), { error: 'Login failed' }); + // The header the deferral exists to protect must survive byte-for-byte. + assert.equal(request.headers.get('authorization'), authorization); +} + +// A static handler ordered `after: 'rest'` runs downstream of authentication, so it is one of the +// Harper-owned layers that must settle a deferred credential rejection (#2418). Settlement covered +// only the ordinary file response, leaving redirects and both `fallthrough: false` not-found forms +// answering a rejected credential as if it were anonymous. +describe('static plugin deferred credential rejection', () => { + const BASIC = 'Basic d29yZHByZXNzOmFwcC1wYXNzd29yZA=='; + const BEARER = 'Bearer downstream-owned-token'; + + function deferred(request) { + deferCredentialRejection(request, credentialRejectionError('Login failed', 401), 'Basic'); + return request; + } + + it('settles the mount-root redirect instead of answering 301', () => { + const { scope, state } = fakeScope({ after: 'rest' }, { urlPath: '/v1' }); + handleApplication(scope); + state.entryCallback({ eventType: 'add', urlPath: '/index.html', absolutePath: '/fake/app/web/index.html' }); + + const request = deferred(staticRequest('/', { originalPathname: '/v1', authorization: BASIC })); + const result = state.listener(request, next); + + assertSettledUnauthorized(result, request, BASIC); + }); + + it('settles the trailing-slash directory redirect instead of answering 301', () => { + const { scope, state } = fakeScope({ after: 'rest' }); + handleApplication(scope); + state.entryCallback({ + eventType: 'add', + urlPath: '/docs/index.html', + absolutePath: '/fake/app/web/docs/index.html', + }); + + const request = deferred(staticRequest('/docs', { authorization: BEARER })); + const result = state.listener(request, next); + + assertSettledUnauthorized(result, request, BEARER); + }); + + it('settles the built-in fallthrough: false 404 instead of answering "File not found"', () => { + const { scope, state } = fakeScope({ fallthrough: false, after: 'rest' }); + handleApplication(scope); + + const request = deferred(staticRequest('/wp-json/wc/v3/orders', { authorization: BASIC })); + const result = state.listener(request, next); + + assertSettledUnauthorized(result, request, BASIC); + }); + + it('settles the configured notFound response instead of serving the fallback page', () => { + const directory = mkdtempSync(join(tmpdir(), 'harper-static-notfound-')); + writeFileSync(join(directory, 'spa.html'), 'spa'); + + try { + const { scope, state } = fakeScope({ + fallthrough: false, + after: 'rest', + notFound: { file: 'spa.html', statusCode: 200 }, + }); + scope.directory = directory; + handleApplication(scope); + + const request = deferred(staticRequest('/app/route', { authorization: BEARER })); + const result = state.listener(request, next); + + assertSettledUnauthorized(result, request, BEARER); + assert.equal(result.handlesHeaders, undefined, 'the settled 401 is not the send() stream response'); + } finally { + rmSync(directory, { recursive: true, force: true }); + } + }); + + it('settles the ordinary file response', () => { + const { scope, state } = fakeScope({ after: 'rest' }); + handleApplication(scope); + state.entryCallback({ eventType: 'add', urlPath: '/asset.js', absolutePath: __filename }); + + const request = deferred(staticRequest('/asset.js', { authorization: BASIC })); + const result = state.listener(request, next); + + assertSettledUnauthorized(result, request, BASIC); + }); + + it('leaves the rejection deferred on the actual fallthrough so a downstream owner still decides', () => { + // The whole point of deferral: Harper does not own this URL, so an application catch-all + // registered after this handler applies its own authentication scheme to the untouched header. + const { scope, state } = fakeScope({ after: 'rest' }); + handleApplication(scope); + + const request = deferred(staticRequest('/wp-json/wc/v3/orders', { authorization: BASIC })); + const result = state.listener(request, next); + + assert.strictEqual(result, NEXT_HANDLER); + assert.equal(getDeferredCredentialRejection(request).status, 401); + assert.equal(request.headers.get('authorization'), BASIC); + assert.equal(request.user, undefined); + }); + + it('leaves a non-GET request deferred', () => { + const { scope, state } = fakeScope({ fallthrough: false, after: 'rest' }); + handleApplication(scope); + + const request = deferred(staticRequest('/app/route', { authorization: BASIC })); + request.method = 'POST'; + + assert.strictEqual(state.listener(request, next), NEXT_HANDLER); + assert.equal(getDeferredCredentialRejection(request).status, 401); + }); +}); + +// The settlement points sit immediately before each response is built, so the responses themselves +// must be unchanged for every request that carries no deferred rejection. +describe('static plugin responses without a deferred rejection', () => { + it('still redirects the mount root, preserving the query string', () => { + const { scope, state } = fakeScope({ after: 'rest' }, { urlPath: '/v1' }); + handleApplication(scope); + state.entryCallback({ eventType: 'add', urlPath: '/index.html', absolutePath: '/fake/app/web/index.html' }); + + const result = state.listener(staticRequest('/', { url: '/?a=1', originalPathname: '/v1' }), next); + + assert.equal(result.status, 301); + assert.equal(result.headers.Location, '/v1/?a=1'); + }); + + it('still redirects a directory to its trailing-slash form, preserving the query string', () => { + const { scope, state } = fakeScope({ after: 'rest' }); + handleApplication(scope); + state.entryCallback({ + eventType: 'add', + urlPath: '/docs/index.html', + absolutePath: '/fake/app/web/docs/index.html', + }); + + const result = state.listener(staticRequest('/docs', { url: '/docs?a=1' }), next); + + assert.equal(result.status, 301); + assert.equal(result.headers.Location, '/docs/?a=1'); + }); + + it('still rebuilds the directory redirect against an application mount', () => { + const { scope, state } = fakeScope({ after: 'rest' }, { urlPath: '/v1' }); + handleApplication(scope); + state.entryCallback({ + eventType: 'add', + urlPath: '/docs/index.html', + absolutePath: '/fake/app/web/docs/index.html', + }); + + const result = state.listener(staticRequest('/docs'), next); + + assert.equal(result.status, 301); + assert.equal(result.headers.Location, '/v1/docs/'); + }); + + it('still answers the built-in 404 body', () => { + const { scope, state } = fakeScope({ fallthrough: false, after: 'rest' }); + handleApplication(scope); + + const result = state.listener(staticRequest('/missing'), next); + + assert.equal(result.status, 404); + assert.equal(result.body, 'File not found'); + }); + + it('still serves the configured notFound file with its status code', () => { + const directory = mkdtempSync(join(tmpdir(), 'harper-static-notfound-ok-')); + writeFileSync(join(directory, 'spa.html'), 'spa'); + + try { + const { scope, state } = fakeScope({ + fallthrough: false, + after: 'rest', + notFound: { file: 'spa.html', statusCode: 200 }, + }); + scope.directory = directory; + handleApplication(scope); + + const result = state.listener(staticRequest('/app/route'), next); + + assert.equal(result.status, 200); + assert.equal(result.handlesHeaders, true); + assert.equal(typeof result.body.pipe, 'function'); + } finally { + rmSync(directory, { recursive: true, force: true }); + } + }); +}); From 6862415ae2f87bec8d80482b055e16b7c9bc9477 Mon Sep 17 00:00:00 2001 From: hdbjeff Date: Wed, 2 Sep 2026 11:14:56 -0700 Subject: [PATCH 07/12] Trim narrative comments from deferred authentication change (#2418) --- components/mcp/adapters/harperHttp.ts | 8 ++--- .../deferred-credential-rejection.test.ts | 7 ++--- security/auth.ts | 29 +++++-------------- security/deferredAuthentication.ts | 17 +++++------ security/tokenAuthentication.ts | 4 +-- security/user.ts | 5 ++-- server/REST.ts | 13 ++------- server/graphqlQuerying.ts | 8 ++--- server/http.ts | 20 ++++--------- server/mqtt.ts | 5 +--- server/serverHelpers/Headers.ts | 17 +++-------- server/static.ts | 13 ++------- .../mcp/adapters/harperHttp.test.js | 5 ++-- .../tokenRejectionClassification.test.js | 5 +--- unitTests/server/fallbackCacheFloor.test.js | 5 ++-- 15 files changed, 48 insertions(+), 113 deletions(-) diff --git a/components/mcp/adapters/harperHttp.ts b/components/mcp/adapters/harperHttp.ts index c5f7a4f332..b5e83bf8cc 100644 --- a/components/mcp/adapters/harperHttp.ts +++ b/components/mcp/adapters/harperHttp.ts @@ -43,7 +43,6 @@ interface HarperHttpRequest { ip?: string; } -/** The settled authentication response, when a deferred credential rejection decides the request. */ type SettledCredentialRejection = { status: number; headers: unknown; body: string | Buffer }; interface HarperHttpResponse { @@ -60,11 +59,8 @@ export function createHarperHttpHandler(profile: McpProfile) { // WebSocket upgrades aren't ours — let the next handler take it. if (request.isWebSocket) return nextHandler(request); - // This mount is Harper-owned, so route ownership is settled the moment we decline to delegate. - // The authentication middleware defers an unrecognized credential rather than answering it in - // line (#2418), and `request.user` is simply unset in that case — which `norm.user` below would - // map to `''`, i.e. anonymous, letting an invalid credential open an MCP session that the base - // revision answered with 401. Settled before the body is read or a session is created. + // This endpoint owns every non-WebSocket request; settle before body or session handling so a + // rejected credential cannot be mapped from an unset `request.user` to an anonymous MCP user. const settledCredentialRejection = settleDeferredCredentialRejection(request) as SettledCredentialRejection | undefined; if (settledCredentialRejection) return settledCredentialRejection; diff --git a/integrationTests/security/deferred-credential-rejection.test.ts b/integrationTests/security/deferred-credential-rejection.test.ts index 53dc7058be..6af99bdc32 100644 --- a/integrationTests/security/deferred-credential-rejection.test.ts +++ b/integrationTests/security/deferred-credential-rejection.test.ts @@ -1,10 +1,9 @@ /** - * End-to-end proof for #2418: an app-port credential Harper does not recognize is not rejected - * until route ownership is known. + * End-to-end proof that an app-port credential Harper does not recognize is not rejected until + * route ownership is known. * * The chain under test is the real one — `authentication -> rest -> application catch-all` — served - * by a real Harper instance. On the pre-fix revision `security/auth.ts` answered 401 while parsing - * the credential, so every "reaches the application catch-all" assertion below fails there. + * by a real Harper instance. * * Reproduction: * npm run test:integration -- "integrationTests/security/deferred-credential-rejection.test.ts" diff --git a/security/auth.ts b/security/auth.ts index a3e3d1dbf4..9666b09362 100644 --- a/security/auth.ts +++ b/security/auth.ts @@ -243,10 +243,8 @@ export async function authentication(request, nextHandler) { status: -1, }); } catch (refreshError) { - // A refresh-validation *fault* (user store down, a password-validation crash) - // must not be swallowed: rethrowing the outer ordinary rejection would tag an - // outage as a deferrable unknown credential. Only after an ordinary tagged - // refresh rejection is the original operation-token rejection restored. + // Preserve refresh-validation faults; only a tagged rejection permits falling + // back to the original operation-token rejection. if (!isCredentialRejection(refreshError)) throw refreshError; throw error; } @@ -257,12 +255,8 @@ export async function authentication(request, nextHandler) { } break; default: - // A scheme Harper does not implement (`Digest`, an application's own, or a - // header with no scheme token at all) previously matched no case and threw - // nothing, so it continued as an anonymous request — on a Harper-owned route - // that is precisely the downgrade this change exists to prevent. Rejecting it - // here routes it through the same audit, fail-closed, and deferral handling as - // an unrecognized Basic or Bearer credential. + // Unsupported schemes are credential rejections so a Harper-owned route cannot + // interpret their lack of a Harper principal as anonymous access. throw credentialRejectionError( AUTHENTICATION_ERROR_MSGS.GENERIC_AUTH_FAIL, HTTP_STATUS_CODES.UNAUTHORIZED @@ -277,16 +271,11 @@ export async function authentication(request, nextHandler) { } } - // #2418: route ownership isn't known yet, so an ordinary credential rejection is not - // decided here. Two cases are still answered in-line rather than deferred: - // - an internal fault (unreadable JWT keys, storage failure, a bug), which must fail - // closed instead of letting an outage hand the request to application authorization; - // - the operations API, where Harper owns every route, so there is nothing to defer to. + // Only tagged credential rejections on the application port may defer. Operations + // routes are always Harper-owned, and internal faults must fail closed. const internalFault = !isCredentialRejection(err); if (request.isOperationsServer || internalFault) { - // An internal fault's own message describes Harper's internals (a missing system - // table, a key path) and is not the client's to read, so it is logged here and the - // client gets the same generic failure a rejected credential gets. + // Internal fault details belong in server logs, not authentication responses. if (internalFault) authLogger.error('Authentication failed internally', errorForLog(err)); return applyResponseHeaders({ status: 401, @@ -300,9 +289,7 @@ export async function authentication(request, nextHandler) { } if (credentialRejection) { - // Continue with no principal and the inbound Authorization header untouched. Any - // Harper-owned layer rejects this via assertNoDeferredCredentialRejection; only a - // request no Harper route owns reaches an application catch-all. + // Preserve the header and leave the principal unset until a route owner settles it. deferCredentialRejection(request, credentialRejection, strategy); } else { authorizationCache.set(authorization, newUser); diff --git a/security/deferredAuthentication.ts b/security/deferredAuthentication.ts index f801f9bfaf..757ea4584d 100644 --- a/security/deferredAuthentication.ts +++ b/security/deferredAuthentication.ts @@ -23,9 +23,8 @@ export type DeferredCredentialRejection = { /** * The status every credential rejection resolves to, whether it is answered in-line or deferred. - * `security/auth.ts` has always answered a rejected credential with 401 regardless of the - * underlying error's own `statusCode` (a 403 `token expired`, for instance), so pinning it here is - * what keeps a deferred rejection byte-identical to the in-line one it replaces. + * Authentication answers a rejected credential with 401 regardless of the underlying error's own + * `statusCode`, so pinning it here keeps immediate and deferred rejections byte-identical. */ const CREDENTIAL_REJECTION_STATUS = 401; @@ -56,13 +55,12 @@ export function getDeferredCredentialRejection(request: any): DeferredCredential /** * The response an owning layer returns once it has established Harper owns the route: exactly the - * descriptor `security/auth.ts` used to return in-line, so the wire contract a rejected credential - * has always produced survives the move downstream. + * descriptor `security/auth.ts` returns in-line, so immediate and deferred rejection share a wire + * contract. * * Owner-specific error mapping must not run first. REST renders a thrown error as an RFC 9457 - * Problem Details document and GraphQL as `{errors:[…]}`; before deferral existed, neither ever saw - * a rejected credential, because authentication answered `{error: message}` in the request's - * negotiated serialization before route matching (#2418). + * Problem Details document and GraphQL as `{errors:[…]}` rather than authentication's negotiated + * `{error: message}` response. * * Returns `undefined` when nothing was deferred, so a caller can `return settled ?? …` inline. */ @@ -71,8 +69,7 @@ export function settleDeferredCredentialRejection( ): { status: number; headers: Headers; body: string | Buffer } | undefined { const deferred = getDeferredCredentialRejection(request); if (!deferred) return undefined; - // The negotiated serializer is the same one `serializeMessage` selects below; naming it in - // Content-Type keeps the body self-describing on a path that historically emitted none. + // Name the serializer explicitly so the response body remains self-describing. const contentType = (request?.headers ? findBestSerializer(request).type : undefined) ?? 'application/json'; return { status: deferred.status, diff --git a/security/tokenAuthentication.ts b/security/tokenAuthentication.ts index 50074d3685..d25effaf5f 100644 --- a/security/tokenAuthentication.ts +++ b/security/tokenAuthentication.ts @@ -462,7 +462,7 @@ async function validateToken(token: string, tokenType: string): Promise { // Only a client-side rejection may be reported as one. Everything else here — unreadable or // malformed JWT key material, a storage failure inside findAndValidateUser, a bug — propagates // unmasked, because callers distinguish a rejected credential from an internal authentication - // fault and only the former is deferred past route matching (#2418). Masking a fault as + // fault and only the former is deferred past route matching. Masking a fault as // `invalid token` would let a key or storage outage read as an unknown credential. if (!isTokenRejection(err)) throw err; @@ -487,7 +487,7 @@ const KEY_MATERIAL_FAULT = /secretOrPublicKey|asymmetric key|PEM routines|^error * True only when `err` says the presented token is not acceptable, rather than that Harper failed to * evaluate it. Never inferred from the 4xx range: `findAndValidateUser()` lazily loads the user cache * and can surface a default-status-400 `ClientError` from a missing system table, which is a storage - * fault wearing a client-error status (#2418). + * fault wearing a client-error status. */ function isTokenRejection(err: any): boolean { if (isCredentialRejection(err)) return true; diff --git a/security/user.ts b/security/user.ts index 06e1eedec2..8e5206563a 100644 --- a/security/user.ts +++ b/security/user.ts @@ -421,9 +421,8 @@ async function findAndValidateUser(username: string, pw?: string | null, validat const userTmp = usersWithRolesMap.get(username); if (!userTmp) { if (!validatePassword) return { username }; - // Tagged as a credential rejection (#2418): callers must be able to tell "this credential is - // not acceptable" apart from a fault raised while loading the user cache above, which shares - // the 4xx range but must fail closed instead of deferring to application authorization. + // The tag distinguishes an absent user from user-cache faults that share the 4xx range but must + // fail closed instead of deferring to application authorization. throw credentialRejectionError(AUTHENTICATION_ERROR_MSGS.GENERIC_AUTH_FAIL, HTTP_STATUS_CODES.UNAUTHORIZED); } diff --git a/server/REST.ts b/server/REST.ts index f7647e4114..9f564f4c27 100644 --- a/server/REST.ts +++ b/server/REST.ts @@ -226,14 +226,8 @@ async function http(request: Request, nextHandler, resources: Resources, httpOpt } } } - // Route ownership is now settled — either a resource matched or this is Harper's OpenAPI - // document — so a credential the authentication middleware deferred is decided here rather - // than travelling on to an application catch-all (#2418). Every path that reaches an - // application instead returned via `nextHandler` above. - // - // Returned as the authentication middleware's own response descriptor rather than thrown: a - // throw would be rendered by the catch below as an RFC 9457 Problem Details document, which is - // not the `{error: message}` body a rejected credential has always produced. + // A matched resource or OpenAPI document settles ownership. Return authentication's response + // descriptor directly so REST's Problem Details mapping cannot change its wire contract. const settledCredentialRejection = settleDeferredCredentialRejection(request); if (settledCredentialRejection) return settledCredentialRejection; if ((resource as any)?.isCaching) { @@ -567,8 +561,7 @@ export function handleApplication(scope: import('../components/Scope.ts').Scope) // TODO: Ideally we would like to have a 404 response before upgrading to WebSocket protocol, probably return ws.close(1011, `No resource was found to handle ${request.pathname}`); } else { - // Harper owns this socket's route, so a deferred credential is rejected here too - // rather than being carried into the resource as anonymous (#2418). + // A matched resource owns this socket; do not carry rejection into it as anonymous. assertNoDeferredCredentialRejection(request); request.handlerPath = entry.path; recordAction( diff --git a/server/graphqlQuerying.ts b/server/graphqlQuerying.ts index d17f4a3c6f..cb001c05f1 100644 --- a/server/graphqlQuerying.ts +++ b/server/graphqlQuerying.ts @@ -581,12 +581,8 @@ export function handleApplication(scope: import('../components/Scope.ts').Scope) return nextLayer(request); } - // Harper owns /graphql, so a credential the authentication middleware deferred is rejected - // here instead of reaching a resolver as an anonymous request (#2418). Settled ahead of the - // try/catch on purpose: the GraphQL error mapping below renders every failure as - // `{errors:[{message}]}` in a GraphQL media type, but a rejected credential has always been - // answered by authentication itself with `{error: message}` in the request's negotiated - // serialization, and that contract is what callers depend on. + // GraphQL owns this route. Settle before its error mapping can change authentication's + // negotiated `{error: message}` response or expose the request to resolvers as anonymous. const settledCredentialRejection = settleDeferredCredentialRejection(request); if (settledCredentialRejection) return settledCredentialRejection; diff --git a/server/http.ts b/server/http.ts index af44504ebc..daa17b80ef 100644 --- a/server/http.ts +++ b/server/http.ts @@ -1020,11 +1020,8 @@ export function makeUwsHandler(port: number | string, isOperationsServer: boolea if (Array.isArray(v)) respHeaders.set(k, k.toLowerCase() === 'set-cookie' ? v : v.join(', ')); else respHeaders.set(k, String(v)); } - // Fastify's own headers win, but the chain's identity/cache floor is not discarded with - // them: authentication stamps `Cache-Control: private, no-cache` and - // `Vary: Authorization, Cookie` on a credential-dependent response, and a deferred - // credential can now reach this fallback (#2418, #1565). Node preserves these via - // `nodeResponse.setHeader` before emitting 'unhandled'; do the equivalent here. + // Preserve the chain's identity/cache floor while allowing Fastify-set headers to win, + // matching the Node fallback's header precedence. mergeChainHeadersIntoFallback(headers, respHeaders); if (universalHeaders.length > 0) applyUniversalHeaders(respHeaders); logHttpRequest(request, injectResult.statusCode, requestId, performance.now() - startTime); @@ -1485,9 +1482,7 @@ export async function bunDelegateToNodeServer( if (webRequest.headers.get('connection')?.toLowerCase() === 'close') { webHeaders.set('connection', 'close'); } - // See mergeChainHeadersIntoFallback: Fastify's headers win, but authentication's - // identity/cache floor on a credential-dependent response is preserved rather than dropped - // with the rest of the chain response (#2418, #1565). + // Preserve the chain's identity/cache floor while allowing Fastify-set headers to win. mergeChainHeadersIntoFallback(chainHeaders, webHeaders); if (universalHeaders.length > 0) applyUniversalHeaders(webHeaders); const responseStream = injectResult.stream(); @@ -1554,12 +1549,9 @@ const builtChainPorts: Record> = { * Builds `chains[port]` from the current `listeners` and, when `port` is the 'all' pseudo-port, * rebuilds every other already-built chain of the same kind too. * - * An entry registered on 'all' is folded into each concrete port's chain when that chain is built, - * so a registration arriving after those chains exist — an application catch-all mounted - * `after: 'rest'`, say — would otherwise update only `chains.all`, which nothing serves, leaving - * every bound port running the chain it had beforehand (#2418). Rebuilding is a pure function of - * the listener list and the port, so re-running it for an already-built port can only reproduce - * that port's order or extend it with the entry just registered. + * A late registration on 'all' must rebuild every concrete port; rebuilding only `chains.all` + * leaves bound ports with stale listener order. Chain construction is a pure function of the + * listener list and port, so rebuilding cannot alter earlier ordering decisions. */ function buildChains( chains: Record, diff --git a/server/mqtt.ts b/server/mqtt.ts index 1aa445c29b..ad7ca6f503 100644 --- a/server/mqtt.ts +++ b/server/mqtt.ts @@ -70,10 +70,7 @@ export function handleApplication(scope: import('../components/Scope.ts').Scope) return next(ws, request, chainCompletion); } - // Declining to delegate settles route ownership: this socket is Harper's. A credential the - // authentication middleware deferred rather than answering in line (#2418) leaves - // `request.user` unset, which would otherwise open an anonymous MQTT session where the - // base revision returned 401 before the upgrade. + // Reject any credential rejection already recorded before MQTT session initialization. const deferred = getDeferredCredentialRejection(request); if (deferred) { return ws.close(WEBSOCKET_UNAUTHORIZED_CLOSE_CODE, deferred.message); diff --git a/server/serverHelpers/Headers.ts b/server/serverHelpers/Headers.ts index e320a350ce..80ca460b82 100644 --- a/server/serverHelpers/Headers.ts +++ b/server/serverHelpers/Headers.ts @@ -173,19 +173,10 @@ function headerValueString(value: unknown): string { * Folds the middleware chain's response headers into the headers a fallback server produced for the * same request. * - * When the chain declines a request (`status: -1`) the Bun and uWS adapters hand it to legacy Fastify - * and build their response headers solely from Fastify's reply, dropping everything the chain had - * already decided. The Node adapter does not: it copies the chain headers onto the `ServerResponse` - * before emitting `unhandled`. That divergence used to be invisible, because a request carrying an - * unrecognized credential never reached a fallback — authentication answered it in line. Deferral - * (#2418) makes it reachable, and with it the identity floor authentication stamps on a - * credential-dependent response (`Cache-Control: private, no-cache`, `Vary: Authorization, Cookie` - * — #1565). - * - * Fastify wins every header it actually set; the chain only fills gaps. `Vary` is unioned rather than - * replaced, and the chain's private cache scope is re-applied unless the final response explicitly - * opts into shared caching (`public`/`s-maxage`), which is RFC 9111's opt-in and the same signal - * `security/auth.ts` honours. + * Bun and uWS construct a new response from legacy Fastify when the chain declines a request, so the + * chain's credential-dependent cache policy must be merged explicitly. Fastify wins headers it set, + * `Vary` is unioned, and private cache scope is re-applied unless the final response explicitly opts + * into RFC 9111 shared caching with `public` or `s-maxage`. */ export function mergeChainHeadersIntoFallback< T extends { diff --git a/server/static.ts b/server/static.ts index 2c6bad4f13..35804e6f3b 100644 --- a/server/static.ts +++ b/server/static.ts @@ -312,14 +312,8 @@ export function handleApplication(scope: Scope) { }); scope.server.http( - // Ownership invariant (#2418): every response this handler originates — both index redirects, - // the served file, and both `fallthrough: false` not-found forms — is Harper answering for the - // URL, so a credential the authentication middleware deferred rather than rejecting in line is - // settled before that response is built, with no URL exempted. Settling before the response body - // is resolved also keeps a rejected credential from turning a missing file or bad `notFound` - // config into a 500 the pre-deferral revision answered with 401. Only the `next(req)` fallthrough - // leaves the deferral in place: there Harper declines the URL, so a downstream owner or an - // application catch-all applies its own scheme. + // Every response this handler originates claims the URL, so settle before redirects, files, or + // non-fallthrough not-found handling. Only `next(req)` leaves ownership and rejection unsettled. (req, next) => { // TODO: Not sure if the isWebSocket check is still necessary if (req.method !== 'GET' || req.isWebSocket) return next(req); @@ -418,8 +412,7 @@ export function handleApplication(scope: Scope) { return next(req); } - // Otherwise, handle not found — settled once here, covering both the built-in 404 and the - // configured `notFound` response below. + // This handler owns both not-found forms, so settle before resolving the configured body. const settledCredentialRejection = settleDeferredCredentialRejection(req); if (settledCredentialRejection) return settledCredentialRejection; diff --git a/unitTests/components/mcp/adapters/harperHttp.test.js b/unitTests/components/mcp/adapters/harperHttp.test.js index 9414811f5e..ed835943bf 100644 --- a/unitTests/components/mcp/adapters/harperHttp.test.js +++ b/unitTests/components/mcp/adapters/harperHttp.test.js @@ -103,9 +103,8 @@ describe('mcp/adapters/harperHttp', () => { /** * `mcp.application` mounts this handler `after: 'authentication'`, and REST declines an * unmatched `/mcp`, so this handler is where route ownership is finally known. Authentication - * now defers an unrecognized credential instead of answering 401 in line, which leaves - * `request.user` unset — the same shape as an anonymous request. Without settlement here, an - * invalid credential opened an MCP session that the base revision answered with 401. + * leaves `request.user` unset — the same shape as an anonymous request. Settlement prevents the + * credential from opening an anonymous MCP session. */ function deferredRequest(overrides = {}) { const request = { diff --git a/unitTests/security/tokenRejectionClassification.test.js b/unitTests/security/tokenRejectionClassification.test.js index 0930519a05..1110f5795f 100644 --- a/unitTests/security/tokenRejectionClassification.test.js +++ b/unitTests/security/tokenRejectionClassification.test.js @@ -2,12 +2,9 @@ /** * `validateToken()` decides whether a Bearer failure is "this token is not acceptable" or "Harper - * could not evaluate it". Only the first may be deferred past route matching (#2418), so this suite + * could not evaluate it". Only the first may be deferred past route matching, so this suite * drives the real `validateOperationToken`/`validateRefreshToken` against real RSA key material and * asserts the classification the authentication middleware then acts on. - * - * On the pre-fix revision the internal-fault cases here fail: every 4xx was converted to a tagged - * `invalid token`, so a user-store or key-material fault was indistinguishable from a forged token. */ const testUtils = require('../testUtils.js'); testUtils.preTestPrep(); diff --git a/unitTests/server/fallbackCacheFloor.test.js b/unitTests/server/fallbackCacheFloor.test.js index 8585b953eb..2002240e7d 100644 --- a/unitTests/server/fallbackCacheFloor.test.js +++ b/unitTests/server/fallbackCacheFloor.test.js @@ -3,9 +3,8 @@ /** * The Bun and uWS adapters hand a request the middleware chain declined (`status: -1`) to legacy * Fastify and build their response headers from Fastify's reply. Node does not lose the chain's - * headers on that path — it copies them onto the `ServerResponse` before emitting 'unhandled' — and - * before #2418 the divergence was unreachable, because an unrecognized credential never survived - * authentication. Deferral makes it reachable, so the identity floor authentication stamps on a + * headers on that path — it copies them onto the `ServerResponse` before emitting 'unhandled'. The + * identity floor authentication stamps on a * credential-dependent response (`Cache-Control: private, no-cache`, `Vary: Authorization, Cookie` * — #1565) has to survive both fallbacks. * From 2d228f7516c9d8e65af7bdaff04ae880cef11638 Mon Sep 17 00:00:00 2001 From: hdbjeff Date: Thu, 3 Sep 2026 09:24:54 -0700 Subject: [PATCH 08/12] fix: settle MQTT deferral asynchronously and keep 401 provenance (#2418) Three defects review found in the deferred-credential-rejection change: - server/mqtt.ts read the deferred state synchronously while the HTTP chain was still pending, so the guard always saw undefined and an invalid Authorization header connected anonymously. It now settles on the same promise the session principal resolves from, with frame handlers still attached synchronously. - The Node fallback copied the chain's identity floor onto the ServerResponse and let a Fastify route replace Cache-Control and Vary outright, which can make a credential-dependent response shared-cacheable. All three bridges now reconcile through mergeChainHeadersIntoFallback. - security/auth.ts post-processed a settled or application-owned 401, overwriting WWW-Authenticate or rewriting it to a login redirect. That rewriting is now skipped when a rejection was deferred, so a settled rejection stays wire-identical to the in-line 401 it replaced and an application keeps its own challenge. Co-Authored-By: Claude Opus 5 (1M context) --- security/auth.ts | 26 +-- security/deferredAuthentication.ts | 8 +- server/DESIGN.md | 36 +++-- server/http.ts | 12 +- server/mqtt.ts | 29 +++- server/serverHelpers/Headers.ts | 60 +++++++ .../security/authCredentialDeferral.test.js | 115 ++++++++++++++ unitTests/server/fallbackCacheFloor.test.js | 148 ++++++++++++++++-- unitTests/server/mqtt.test.js | 134 ++++++++++++++++ 9 files changed, 524 insertions(+), 44 deletions(-) diff --git a/security/auth.ts b/security/auth.ts index 9666b09362..58c77a8cab 100644 --- a/security/auth.ts +++ b/security/auth.ts @@ -389,16 +389,22 @@ export async function authentication(request, nextHandler) { if (!response) return response; if (response.status === 401) { wasUnauthorized = true; - if ( - headers['user-agent']?.startsWith('Mozilla') && - headers.accept?.startsWith('text/html') && - resources.loginPath - ) { - // on the web if we have a login page, default to redirecting to it - response.status = 302; - response.headers.set('Location', resources.loginPath(request)); - } // the HTTP specified way of indicating HTTP authentication methods supported: - else response.headers.set('WWW-Authenticate', 'Basic'); + // A deferred rejection means this 401 came from downstream, not from the in-line rejection + // this middleware used to answer with. Harper's settled rejection has to stay wire-identical + // to that in-line 401 (which returned before any of this ran), and a 401 an application + // catch-all raised is that application's own challenge for its own scheme. + if (!getDeferredCredentialRejection(request)) { + if ( + headers['user-agent']?.startsWith('Mozilla') && + headers.accept?.startsWith('text/html') && + resources.loginPath + ) { + // on the web if we have a login page, default to redirecting to it + response.status = 302; + response.headers.set('Location', resources.loginPath(request)); + } // the HTTP specified way of indicating HTTP authentication methods supported: + else response.headers.set('WWW-Authenticate', 'Basic'); + } } return applyResponseHeaders(response); } catch (error) { diff --git a/security/deferredAuthentication.ts b/security/deferredAuthentication.ts index 757ea4584d..191b08f4f2 100644 --- a/security/deferredAuthentication.ts +++ b/security/deferredAuthentication.ts @@ -60,7 +60,8 @@ export function getDeferredCredentialRejection(request: any): DeferredCredential * * Owner-specific error mapping must not run first. REST renders a thrown error as an RFC 9457 * Problem Details document and GraphQL as `{errors:[…]}` rather than authentication's negotiated - * `{error: message}` response. + * `{error: message}` response. `security/auth.ts` likewise leaves a settled rejection's status and + * challenge headers alone, so it stays byte-identical to the in-line 401 it replaced. * * Returns `undefined` when nothing was deferred, so a caller can `return settled ?? …` inline. */ @@ -73,9 +74,8 @@ export function settleDeferredCredentialRejection( const contentType = (request?.headers ? findBestSerializer(request).type : undefined) ?? 'application/json'; return { status: deferred.status, - // A real Headers, not a plain object: the authentication middleware's own 401 post-processing - // calls `response.headers.set()` (WWW-Authenticate, or a Location when a login page is - // configured) on whatever an owning layer returns, and a plain object has no `set`. + // A real Headers, not a plain object: authentication stamps the #1565 identity floor onto + // whatever an owning layer returns, and the HTTP bridges read it back through `get`. headers: new Headers({ 'Content-Type': contentType }), body: serializeMessage({ error: deferred.message }, request) as string | Buffer, }; diff --git a/server/DESIGN.md b/server/DESIGN.md index 8a8bed1572..a18f799b9c 100644 --- a/server/DESIGN.md +++ b/server/DESIGN.md @@ -264,7 +264,7 @@ Any layer that establishes Harper owns the route then settles the deferred state | `REST.ts` WebSocket handler | after `resources.getMatch(url, 'ws')` succeeds | | `graphqlQuerying.ts` | after the `/graphql` prefix match, ahead of its error mapping | | `static.ts` | after a static file entry matches | -| `mqtt.ts` WebSocket handler | after the `mqtt` subprotocol claims the socket | +| `mqtt.ts` WebSocket handler | once the pending HTTP chain settles, before the first packet | | `components/mcp/adapters/harperHttp.ts` | after the WebSocket hand-off, before the body is read | **Every Harper-owned handler registered `after: 'authentication'` owes this settlement**, because @@ -278,7 +278,11 @@ in the request's negotiated content type. That matters because an owner's own er that contract — REST renders a thrown error as an RFC 9457 Problem Details document and GraphQL as `{errors:[{message}]}` — and a rejected credential never reached either before deferral existed. `assertNoDeferredCredentialRejection()` is the throwing form, for a WebSocket upgrade that has no -descriptor to return. The deferred status is pinned to 401 regardless of the underlying error's own +descriptor to return. A WebSocket owner cannot read the state synchronously: `server/http.ts` starts +`httpChain[port](request)` and invokes the WebSocket chain with the still-pending completion, so +authentication has not yet classified the credential. `mqtt.ts` therefore settles on that promise — +the same one the session's principal resolves from — and closes the socket from its rejection, while +its frame handlers still attach synchronously. The deferred status is pinned to 401 regardless of the underlying error's own status, so a 403 `token expired` reads exactly as it did before. A Harper-owned route therefore behaves identically to the pre-deferral build, protected or public: an unknown credential can never buy access an anonymous caller would have received, and can never reach an application catch-all. Only a URL that reached @@ -287,15 +291,29 @@ have received, and can never reach an application catch-all. Only a URL that rea The contract is route-ownership-based, not path-based. There is no exemption list, no carrier header, no credential rename, and no pre-auth stripping shim. +**Authentication does not re-decorate a 401 it did not raise.** `security/auth.ts` post-processes any +401 coming back up the chain — overwriting `WWW-Authenticate` with `Basic`, or rewriting the status to +a 302 at `resources.loginPath` for a browser. The in-line rejection deferral replaced returned before +that code, so a settled rejection must skip it to stay wire-identical; and a 401 an application +catch-all raised for its own scheme (a WooCommerce or Bearer challenge) is that application's to make. +Both cases are keyed on the deferred state, so a request that deferred nothing keeps the existing +behavior exactly. The #1565 identity floor still applies either way — it is stamped in +`applyResponseHeaders`, not in the challenge rewriting. + **The identity cache floor survives the legacy Fastify fallbacks.** A response produced under a deferred credential is credential-dependent (#1565), so `authentication` stamps -`Cache-Control: private, no-cache` and `Vary: Authorization, Cookie` on it. When the chain declines -a request (`status: -1`), Node carries those onto the `ServerResponse` before emitting `unhandled`, -but the Bun and uWS adapters used to rebuild their headers solely from Fastify's reply and drop -them. Before deferral an unrecognized credential could not reach a fallback at all, so this was -unreachable; now both adapters merge through `Headers.ts → mergeChainHeadersIntoFallback()` — -Fastify wins every header it set, `Vary` is unioned, and the private scope is re-applied unless the -final response explicitly opts into shared caching (`public`/`s-maxage`). +`Cache-Control: private, no-cache` and `Vary: Authorization, Cookie` on it. Before deferral an +unrecognized credential could not reach a fallback at all, so this was unreachable. All three adapters +now reconcile through one policy, `Headers.ts → mergeChainHeadersIntoFallback()`: Fastify wins every +header it set, `Vary` is unioned, and the private scope is re-applied unless the final response +explicitly opts into shared caching (`public`/`s-maxage`). + +Bun and uWS rebuild their headers from Fastify's reply and merge once. Node hands Fastify the same +`ServerResponse` the chain's headers were copied onto, so copying is not enough — a route calling +`reply.header('Cache-Control', …)` replaces the floor outright and can make a credential-dependent +response shared-cacheable. `bridgeChainHeadersToNodeResponse()` therefore runs the same merge from a +`writeHead` interception, the last point the header set is still mutable and the one Node also routes +implicit headers through. ### Response Cache-Control / Vary policy (#1518, #1565) diff --git a/server/http.ts b/server/http.ts index daa17b80ef..2677caf7a4 100644 --- a/server/http.ts +++ b/server/http.ts @@ -18,7 +18,13 @@ import { createServer as createSecureServerHttp1 } from 'node:https'; import { createServer, IncomingMessage, validateHeaderName, validateHeaderValue } from 'node:http'; import { createServer as createNetServer } from 'node:net'; import { Request, BunRequest, UwsRequest, isBun } from './serverHelpers/Request.ts'; -import { appendHeader, Headers, mergeChainHeadersIntoFallback, toWriteHeadHeaders } from './serverHelpers/Headers.ts'; +import { + appendHeader, + bridgeChainHeadersToNodeResponse, + Headers, + mergeChainHeadersIntoFallback, + toWriteHeadHeaders, +} from './serverHelpers/Headers.ts'; import { decodeProxyHeader, applyProxyHeader, @@ -679,9 +685,7 @@ function getHTTPServer(port: number, secure: boolean, options: ServerOptions) { // This means the HDB stack didn't handle the request, and we can then cascade the request // to the server-level handler, forming the bridge to the slower legacy fastify framework that expects // to interact with a node HTTP server object. - for (const headerPair of response.headers || []) { - nodeResponse.setHeader(headerPair[0], headerPair[1]); - } + bridgeChainHeadersToNodeResponse(response.headers, nodeResponse); nodeRequest.baseRequest = request; nodeResponse.baseResponse = response; return httpServers[port].emit('unhandled', nodeRequest, nodeResponse); diff --git a/server/mqtt.ts b/server/mqtt.ts index ad7ca6f503..5947b26415 100644 --- a/server/mqtt.ts +++ b/server/mqtt.ts @@ -19,7 +19,10 @@ import { forComponent as loggerForComponent } from '../utility/logging/harper_lo import { EventEmitter } from 'events'; import { verifyCertificate } from '../security/certificateVerification/index.ts'; import { registerShutdownDrain } from '../components/shutdownDrain.ts'; -import { getDeferredCredentialRejection } from '../security/deferredAuthentication.ts'; +import { + assertNoDeferredCredentialRejection, + getDeferredCredentialRejection, +} from '../security/deferredAuthentication.ts'; /** RFC 6455 private-use close code Harper already maps HTTP 401 to (see server/REST.ts). */ const WEBSOCKET_UNAUTHORIZED_CLOSE_CODE = 3000; @@ -70,21 +73,31 @@ export function handleApplication(scope: import('../components/Scope.ts').Scope) return next(ws, request, chainCompletion); } - // Reject any credential rejection already recorded before MQTT session initialization. - const deferred = getDeferredCredentialRejection(request); - if (deferred) { - return ws.close(WEBSOCKET_UNAUTHORIZED_CLOSE_CODE, deferred.message); - } - emitEvent('connection', ws); mqttLog.debug?.('Received WebSocket connection for MQTT from', ws._socket.remoteAddress); + // Both WebSocket entry points invoke this listener synchronously with the HTTP chain still + // pending (server/http.ts), so authentication has not recorded a credential rejection yet. + // It settles on the same promise the session principal comes from, which onSocket awaits + // before it processes any packet — the handlers below still attach synchronously, so no + // frame that arrives in the meantime is dropped. + const authenticated = Promise.resolve(chainCompletion).then(() => { + assertNoDeferredCredentialRejection(request); + return request?.user; + }); + authenticated.catch((error) => { + mqttLog.info?.('Closing MQTT WebSocket connection, authentication was rejected', error); + ws.close( + WEBSOCKET_UNAUTHORIZED_CLOSE_CODE, + getDeferredCredentialRejection(request)?.message ?? 'Unauthorized' + ); + }); const { onMessage, onClose } = onSocket( ws, (message) => { ws.send(message); }, request, - Promise.resolve(chainCompletion).then(() => request?.user), + authenticated, mqttSettings ); ws.on('message', onMessage); diff --git a/server/serverHelpers/Headers.ts b/server/serverHelpers/Headers.ts index 80ca460b82..65b3f45c9f 100644 --- a/server/serverHelpers/Headers.ts +++ b/server/serverHelpers/Headers.ts @@ -214,3 +214,63 @@ export function mergeChainHeadersIntoFallback< } return finalHeaders; } + +/** + * Presents a Node `ServerResponse`'s live header set through the Headers-like surface + * `mergeChainHeadersIntoFallback` and `addVaryHeader` expect. + */ +function nodeResponseHeaders(nodeResponse: any) { + return { + get: (name: string) => nodeResponse.getHeader(name), + set: (name: string, value: any) => nodeResponse.setHeader(name, value), + has: (name: string) => nodeResponse.hasHeader(name), + append: (name: string, value: any, commaDelimited?: boolean) => { + const existing = nodeResponse.getHeader(name); + if (existing == null) return nodeResponse.setHeader(name, value); + if (commaDelimited) + return nodeResponse.setHeader(name, (Array.isArray(existing) ? existing.join(', ') : existing) + ', ' + value); + return nodeResponse.setHeader(name, Array.isArray(existing) ? [...existing, value] : [existing, value]); + }, + }; +} + +/** `writeHead` accepts a flat `[name, value, …]` array, a `[name, value][]` array, or an object. */ +function applyWriteHeadHeaders(nodeResponse: any, headers: any): void { + if (Array.isArray(headers)) { + if (Array.isArray(headers[0])) { + for (const [name, value] of headers) nodeResponse.setHeader(name, value); + } else { + for (let i = 0; i + 1 < headers.length; i += 2) nodeResponse.setHeader(headers[i], headers[i + 1]); + } + return; + } + for (const name of Object.keys(headers)) { + const value = headers[name]; + if (value != null) nodeResponse.setHeader(name, value); + } +} + +/** + * Node's counterpart to the Bun and uWS fallback bridges: the chain's headers go onto the + * `ServerResponse` before legacy Fastify runs, so a Fastify route that sets `Cache-Control` or `Vary` + * replaces them outright and can make a credential-dependent response shared-cacheable (#1565). + * Reconciliation therefore runs at `writeHead` — the last point the header set is still mutable, and + * the one Node also routes implicit headers through — using the same policy as the other two bridges. + */ +export function bridgeChainHeadersToNodeResponse(chainHeaders: any, nodeResponse: any): void { + if (!chainHeaders?.[Symbol.iterator]) return; + for (const [name, value] of chainHeaders) nodeResponse.setHeader(name, value); + const originalWriteHead = nodeResponse.writeHead; + nodeResponse.writeHead = function (statusCode: number, statusMessage?: any, headers?: any) { + if (this.headersSent) return originalWriteHead.apply(this, arguments as any); + if (statusMessage != null && typeof statusMessage !== 'string') { + headers = statusMessage; + statusMessage = undefined; + } + if (headers) applyWriteHeadHeaders(this, headers); + mergeChainHeadersIntoFallback(chainHeaders, nodeResponseHeaders(this)); + return statusMessage === undefined + ? originalWriteHead.call(this, statusCode) + : originalWriteHead.call(this, statusCode, statusMessage); + }; +} diff --git a/unitTests/security/authCredentialDeferral.test.js b/unitTests/security/authCredentialDeferral.test.js index bffbda59ce..dd457ed8c7 100644 --- a/unitTests/security/authCredentialDeferral.test.js +++ b/unitTests/security/authCredentialDeferral.test.js @@ -15,6 +15,7 @@ const { Headers } = require('#src/server/serverHelpers/Headers'); const { credentialRejectionError, settleDeferredCredentialRejection } = require('#src/security/deferredAuthentication'); const { ClientError, ServerError } = require('#src/utility/errors/hdbError'); const serverModule = require('#src/server/Server'); +const resourcesModule = require('#src/resources/Resources'); const tokenAuthentication = require('#src/security/tokenAuthentication'); const { authentication } = require('#src/security/auth'); @@ -78,9 +79,13 @@ describe('deferred credential rejection through the app-port middleware chain', }; } + /** When set, the catch-all answers with this instead of its default 200. */ + let catchAllResponse; + /** The application's own middleware, mounted after `rest`, applying its own auth scheme. */ function applicationCatchAll(request) { trace.push('catch-all'); + if (catchAllResponse) return catchAllResponse(); return { status: 200, headers: new Headers(), @@ -135,11 +140,23 @@ describe('deferred credential rejection through the app-port middleware chain', tokenAuthentication.validateRefreshToken = originalValidateRefreshToken; }); + // `resources.loginPath` is read by authentication's 401 rewriting; the registry is a live binding + // that only exists once something has built it. + before(() => { + if (!resourcesModule.resources) resourcesModule.resetResources(); + }); + beforeEach(() => { trace = []; ownedPaths = new Set([HARPER_OWNED, HARPER_OWNED_PUBLIC]); knownUsers = new Map([['harper_admin:harper-pw', { username: 'harper_admin', role: { permission: {} } }]]); getUserFault = undefined; + catchAllResponse = undefined; + delete resourcesModule.resources.loginPath; + }); + + afterEach(() => { + delete resourcesModule.resources.loginPath; }); it('resolves the chain as authentication -> rest -> application catch-all', async () => { @@ -429,4 +446,102 @@ describe('deferred credential rejection through the app-port middleware chain', assert.strictEqual(response.headers?.get?.('Cache-Control') ?? null, null); }); + describe('401 post-processing ownership', () => { + // `security/auth.ts` rewrites any 401 that comes back up the chain: it overwrites + // `WWW-Authenticate` with `Basic`, or turns the 401 into a 302 to Harper's login page for a + // browser. Before deferral existed a rejected credential returned in line and never reached + // that code, and a rejected credential never reached an application catch-all at all. + + /** A browser request: the exact shape that triggers the login-page rewrite. */ + const BROWSER = { + 'user-agent': 'Mozilla/5.0 (Macintosh)', + 'accept': 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8', + }; + + function browserRequestExtra(authorization) { + const headerObject = { ...BROWSER }; + if (authorization) headerObject.authorization = authorization; + return { + headers: { asObject: headerObject, get: (name) => headerObject[name.toLowerCase()] }, + }; + } + + async function sendBrowser(pathname, authorization) { + const request = makeRequest(pathname, authorization, browserRequestExtra(authorization)); + const response = await chain(request); + return { request, response }; + } + + it("leaves an application catch-all's own 401 challenge untouched", async () => { + // The Woo/WordPress case the issue is about: the application owns the route, applies its own + // scheme, and answers with its own challenge. Harper must not rewrite it to `Basic`. + catchAllResponse = () => ({ + status: 401, + headers: new Headers({ 'WWW-Authenticate': 'Basic realm="WooCommerce", charset="UTF-8"' }), + body: JSON.stringify({ code: 'woocommerce_rest_authentication_error' }), + }); + + const { response } = await send(APP_OWNED, WORDPRESS_BASIC); + + assert.deepStrictEqual(trace, ['catch-all']); + assert.strictEqual(response.status, 401); + assert.strictEqual(response.headers.get('WWW-Authenticate'), 'Basic realm="WooCommerce", charset="UTF-8"'); + }); + + it("does not redirect a browser to Harper's login page over an application-owned 401", async () => { + resourcesModule.resources.loginPath = () => '/login'; + catchAllResponse = () => ({ + status: 401, + headers: new Headers({ 'WWW-Authenticate': 'Bearer realm="woo"' }), + body: JSON.stringify({ code: 'woocommerce_rest_authentication_error' }), + }); + + const { response } = await sendBrowser(APP_OWNED, WORDPRESS_BASIC); + + assert.strictEqual(response.status, 401); + assert.strictEqual(response.headers.get('Location') ?? null, null); + assert.strictEqual(response.headers.get('WWW-Authenticate'), 'Bearer realm="woo"'); + }); + + it('still applies the identity floor to an application-owned 401', async () => { + // Suppressing the rewrite must not also suppress #1565: the response was produced under the + // credential Harper passed through. + catchAllResponse = () => ({ status: 401, headers: new Headers(), body: '{}' }); + + const { response } = await send(APP_OWNED, WORDPRESS_BASIC); + + assert.strictEqual(response.headers.get('Cache-Control'), 'private, no-cache'); + assert.ok(response.headers.get('Vary').includes('Authorization')); + }); + + it('keeps a settled Harper-owned rejection wire-identical to the in-line 401 it replaced', async () => { + resourcesModule.resources.loginPath = () => '/login'; + + const { response } = await sendBrowser(HARPER_OWNED, WORDPRESS_BASIC); + + // The pre-deferral middleware returned this directly from its own catch: status 401, the + // `{error}` envelope, and no challenge or login redirect bolted on afterwards. + assert.strictEqual(response.status, 401); + assert.strictEqual(response.headers.get('Location') ?? null, null); + assert.strictEqual(response.headers.get('WWW-Authenticate') ?? null, null); + assert.deepStrictEqual(JSON.parse(response.body), { error: 'Login failed' }); + }); + + it('still redirects a browser with no credentials at all to the login page', async () => { + // The control: nothing was deferred, so Harper's own 401 handling is unchanged. + resourcesModule.resources.loginPath = () => '/login'; + + const { response } = await sendBrowser(HARPER_OWNED, undefined); + + assert.strictEqual(response.status, 302); + assert.strictEqual(response.headers.get('Location'), '/login'); + }); + + it('still challenges an uncredentialed non-browser 401 with WWW-Authenticate', async () => { + const { response } = await send(HARPER_OWNED, undefined); + + assert.strictEqual(response.status, 401); + assert.strictEqual(response.headers.get('WWW-Authenticate'), 'Basic'); + }); + }); }); diff --git a/unitTests/server/fallbackCacheFloor.test.js b/unitTests/server/fallbackCacheFloor.test.js index 2002240e7d..f4aa467643 100644 --- a/unitTests/server/fallbackCacheFloor.test.js +++ b/unitTests/server/fallbackCacheFloor.test.js @@ -1,15 +1,16 @@ 'use strict'; /** - * The Bun and uWS adapters hand a request the middleware chain declined (`status: -1`) to legacy - * Fastify and build their response headers from Fastify's reply. Node does not lose the chain's - * headers on that path — it copies them onto the `ServerResponse` before emitting 'unhandled'. The - * identity floor authentication stamps on a - * credential-dependent response (`Cache-Control: private, no-cache`, `Vary: Authorization, Cookie` - * — #1565) has to survive both fallbacks. + * All three adapters hand a request the middleware chain declined (`status: -1`) to legacy Fastify. + * Bun and uWS build their response headers from Fastify's reply; Node hands Fastify the same + * `ServerResponse` the chain's headers were copied onto, so a Fastify route that sets `Cache-Control` + * or `Vary` replaces them outright. The identity floor authentication stamps on a credential-dependent + * response (`Cache-Control: private, no-cache`, `Vary: Authorization, Cookie` — #1565) has to survive + * all three. * - * These drive the real adapters (`makeUwsHandler`, `bunDelegateToNodeServer`) against a stub Fastify - * instance, not a re-implementation of their header assembly. + * These drive the real adapters (`makeUwsHandler`, `bunDelegateToNodeServer`, + * `bridgeChainHeadersToNodeResponse`) against a stub Fastify instance for the first two and a real + * Fastify app over a real `http.Server` for Node, not a re-implementation of their header assembly. */ const testUtils = require('../testUtils.js'); testUtils.preTestPrep(); @@ -24,7 +25,13 @@ const { registerFallbackServer, registerFastifyInstance, } = require('#src/server/http'); -const { Headers, mergeChainHeadersIntoFallback } = require('#src/server/serverHelpers/Headers'); +const { + bridgeChainHeadersToNodeResponse, + Headers, + mergeChainHeadersIntoFallback, +} = require('#src/server/serverHelpers/Headers'); +const http = require('node:http'); +const Fastify = require('fastify'); const UWS_PORT = 19430; const BUN_PORT = 19431; @@ -194,6 +201,129 @@ describe('legacy Fastify fallback preserves the chain cache floor', () => { }); }); + describe('Node adapter', () => { + /** + * The production shape of `server/http.ts`'s `status === -1` branch: the chain's headers go onto + * the `ServerResponse`, then legacy Fastify writes the response through that same object. + */ + async function requestThroughFastify(chainHeaders, defineRoutes) { + const fastify = Fastify(); + defineRoutes(fastify); + await fastify.ready(); + const server = http.createServer((nodeRequest, nodeResponse) => { + bridgeChainHeadersToNodeResponse(chainHeaders, nodeResponse); + fastify.routing(nodeRequest, nodeResponse); + }); + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + try { + const { port } = server.address(); + return await new Promise((resolve, reject) => { + const request = http.get({ host: '127.0.0.1', port, path: '/wp-json/wc/v3/products' }, (response) => { + response.resume(); + response.on('end', () => resolve(response)); + }); + request.on('error', reject); + }); + } finally { + server.close(); + await fastify.close(); + } + } + + it('re-applies the private scope over a Fastify route that replaced Cache-Control', async () => { + const response = await requestThroughFastify(identityFloorHeaders(), (fastify) => { + fastify.get('/wp-json/wc/v3/products', (_request, reply) => + reply.header('Cache-Control', 'max-age=600, must-revalidate').send('{}') + ); + }); + + assert.strictEqual(response.headers['cache-control'], 'max-age=600, must-revalidate, private'); + }); + + it('unions Vary with a Fastify route that replaced it', async () => { + const response = await requestThroughFastify(identityFloorHeaders(), (fastify) => { + fastify.get('/wp-json/wc/v3/products', (_request, reply) => reply.header('Vary', 'Accept-Encoding').send('{}')); + }); + + for (const token of ['Accept-Encoding', 'Authorization', 'Cookie']) { + assert.ok( + response.headers.vary.includes(token), + `Vary should include ${token}, got '${response.headers.vary}'` + ); + } + }); + + it('keeps the floor intact when the Fastify route sets neither header', async () => { + const response = await requestThroughFastify(identityFloorHeaders(), (fastify) => { + fastify.get('/wp-json/wc/v3/products', (_request, reply) => reply.send('{}')); + }); + + assert.strictEqual(response.headers['cache-control'], 'private, no-cache'); + assert.strictEqual(response.headers.vary, 'Authorization, Cookie'); + }); + + it("honours a Fastify route's explicit shared-cache opt-in", async () => { + const response = await requestThroughFastify(identityFloorHeaders(), (fastify) => { + fastify.get('/wp-json/wc/v3/products', (_request, reply) => + reply.header('Cache-Control', 'public, max-age=600').send('{}') + ); + }); + + assert.strictEqual(response.headers['cache-control'], 'public, max-age=600'); + }); + + it('carries the chain headers Fastify did not set, without overriding those it did', async () => { + const chainHeaders = identityFloorHeaders(); + chainHeaders.set('Access-Control-Allow-Origin', 'https://shop.example'); + const response = await requestThroughFastify(chainHeaders, (fastify) => { + fastify.get('/wp-json/wc/v3/products', (_request, reply) => + reply.header('Content-Type', 'application/json').send('{}') + ); + }); + + assert.strictEqual(response.headers['access-control-allow-origin'], 'https://shop.example'); + assert.ok(response.headers['content-type'].startsWith('application/json')); + }); + + it('reconciles a response written without an explicit writeHead', async () => { + // Node generates headers implicitly through `writeHead` on `end()`, so a fallback that never + // calls it explicitly still has to pass through the same policy. + const chainHeaders = identityFloorHeaders(); + const server = http.createServer((nodeRequest, nodeResponse) => { + bridgeChainHeadersToNodeResponse(chainHeaders, nodeResponse); + nodeResponse.setHeader('Cache-Control', 'max-age=600'); + nodeResponse.end('{}'); + }); + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + try { + const { port } = server.address(); + const response = await new Promise((resolve, reject) => { + http + .get({ host: '127.0.0.1', port, path: '/' }, (res) => { + res.resume(); + res.on('end', () => resolve(res)); + }) + .on('error', reject); + }); + + assert.strictEqual(response.headers['cache-control'], 'max-age=600, private'); + } finally { + server.close(); + } + }); + + it('leaves a response alone when the chain produced no headers', async () => { + const response = await requestThroughFastify(new Headers(), (fastify) => { + fastify.get('/wp-json/wc/v3/products', (_request, reply) => + reply.header('Cache-Control', 'max-age=600').send('{}') + ); + }); + + assert.strictEqual(response.headers['cache-control'], 'max-age=600'); + assert.strictEqual(response.headers.vary, undefined); + }); + }); + describe('mergeChainHeadersIntoFallback', () => { it('never lets the chain overwrite a header the final response set', () => { const chain = new Headers({ 'Content-Type': 'text/plain', 'X-From-Chain': 'yes' }); diff --git a/unitTests/server/mqtt.test.js b/unitTests/server/mqtt.test.js index b742cd69cb..3c7c91852b 100644 --- a/unitTests/server/mqtt.test.js +++ b/unitTests/server/mqtt.test.js @@ -66,3 +66,137 @@ describe('mqtt.ts handleApplication raw-socket registration', () => { }); }); }); + +// The WebSocket entry points in server/http.ts call `httpChain[port](request)` and hand this +// listener the still-pending completion, so authentication has not resolved the credential yet when +// the listener runs. Reading the deferred-rejection state synchronously therefore always saw +// `undefined`, and an invalid Authorization header connected anonymously wherever MQTT allows +// anonymous connections (#2418). +describe('mqtt.ts WebSocket listener settles authentication before the session starts', () => { + const { credentialRejectionError, deferCredentialRejection } = require('#src/security/deferredAuthentication'); + const { generate } = require('mqtt-packet'); + + /** Captures the listener `handleApplication` registers through `server.ws()`. */ + function webSocketListener() { + let listener; + const server = { + ws: (fn) => ((listener = fn), []), + socket: () => ({}), + }; + handleApplication({ options: { getAll: () => ({ webSocket: {} }) }, server }); + return listener; + } + + function fakeWebSocket() { + const closes = []; + const sends = []; + const handlers = {}; + return { + closes, + sends, + handlers, + _socket: { remoteAddress: '127.0.0.1' }, + close: (code, reason) => closes.push({ code, reason }), + send: (message) => sends.push(message), + on: (event, handler) => { + handlers[event] = handler; + }, + }; + } + + function mqttUpgradeRequest(headers = {}) { + const asObject = { 'sec-websocket-protocol': 'mqtt', ...headers }; + return { headers: { asObject, get: (name) => asObject[name.toLowerCase()] } }; + } + + /** Lets every already-queued microtask and the `.catch` continuation run. */ + const settle = () => new Promise((resolve) => setImmediate(resolve)); + + it('closes the socket once an asynchronously-recorded credential rejection settles', async () => { + const listener = webSocketListener(); + const ws = fakeWebSocket(); + const request = mqttUpgradeRequest({ authorization: 'Basic d29yZHByZXNzOnNlY3JldA==' }); + // The real shape: `authentication` yields on the user lookup before it can classify the + // credential, so the rejection is recorded a turn after this listener is invoked. + const chainCompletion = (async () => { + await Promise.resolve(); + deferCredentialRejection(request, credentialRejectionError('Login failed', 401), 'Basic'); + return { status: 200 }; + })(); + + listener(ws, request, chainCompletion, () => { + throw new Error('a mqtt-subprotocol upgrade must not fall through to the next listener'); + }); + + // Nothing is knowable yet — that is exactly why a synchronous check could not work. + assert.deepStrictEqual(ws.closes, []); + await chainCompletion; + await settle(); + + assert.deepStrictEqual(ws.closes, [{ code: 3000, reason: 'Login failed' }]); + }); + + it('does not accept the subsequent CONNECT anonymously', async () => { + // The consequence the synchronous check was supposed to prevent: this scope allows anonymous + // connections, so a CONNECT that follows an invalid Authorization header was answered with a + // CONNACK and given an anonymous session. + const listener = webSocketListener(); + const ws = fakeWebSocket(); + const request = mqttUpgradeRequest({ authorization: 'Basic d29yZHByZXNzOnNlY3JldA==' }); + const chainCompletion = (async () => { + await Promise.resolve(); + deferCredentialRejection(request, credentialRejectionError('Login failed', 401), 'Basic'); + return { status: 200 }; + })(); + + listener(ws, request, chainCompletion, () => {}); + await chainCompletion; + await settle(); + + ws.handlers.message( + generate({ cmd: 'connect', protocolId: 'MQTT', protocolVersion: 4, clientId: 'woo-client', clean: true }) + ); + await settle(); + await settle(); + + assert.deepStrictEqual(ws.sends, [], 'no CONNACK may be sent for a rejected credential'); + assert.ok(ws.closes.length > 0, 'the connection must be closed rather than served anonymously'); + }); + + it('leaves an authenticated connection open and attaches its handlers synchronously', async () => { + const listener = webSocketListener(); + const ws = fakeWebSocket(); + const request = mqttUpgradeRequest({ authorization: 'Basic aGFycGVyOnB3' }); + const chainCompletion = (async () => { + await Promise.resolve(); + request.user = { username: 'harper_admin' }; + return { status: 200 }; + })(); + + listener(ws, request, chainCompletion, () => {}); + + // Handlers must be in place before the chain settles, or frames that arrive in that window + // would be dropped. + assert.strictEqual(typeof ws.handlers.message, 'function'); + assert.strictEqual(typeof ws.handlers.close, 'function'); + await chainCompletion; + await settle(); + + assert.deepStrictEqual(ws.closes, []); + }); + + it('passes a non-mqtt subprotocol upgrade straight to the next listener', async () => { + const listener = webSocketListener(); + const ws = fakeWebSocket(); + const request = mqttUpgradeRequest({ 'sec-websocket-protocol': 'graphql-ws' }); + let forwarded = false; + + listener(ws, request, Promise.resolve({ status: 200 }), () => { + forwarded = true; + }); + await settle(); + + assert.strictEqual(forwarded, true); + assert.deepStrictEqual(ws.closes, []); + }); +}); From b55f7bd52fd01a3d7d20fa04f5b8b65eb241e39a Mon Sep 17 00:00:00 2001 From: hdbjeff Date: Thu, 3 Sep 2026 10:25:00 -0700 Subject: [PATCH 09/12] Harden deferred authentication boundaries (#2418) --- components/mcp/adapters/harperHttp.ts | 7 +- .../deferred-credential-rejection.test.ts | 36 ---------- .../appCatchAll.js | 3 - .../resources.js | 2 - security/auth.ts | 17 +---- security/credentialRejection.ts | 20 +----- security/deferredAuthentication.ts | 40 +++-------- security/tokenAuthentication.ts | 28 +------- security/user.ts | 2 - server/DESIGN.md | 6 +- server/REST.ts | 3 - server/graphqlQuerying.ts | 2 - server/http.ts | 24 +------ server/serverHelpers/Headers.ts | 32 +++++---- server/static.ts | 17 +---- .../mcp/adapters/harperHttp.test.js | 9 --- .../security/authCredentialDeferral.test.js | 70 ------------------- .../security/deferredAuthentication.test.js | 46 +++++++----- .../tokenRejectionClassification.test.js | 15 ---- unitTests/server/fallbackCacheFloor.test.js | 46 ++++++------ unitTests/server/httpChainPortAll.test.js | 4 -- unitTests/server/mqtt.test.js | 15 ---- unitTests/server/static.test.js | 17 ----- 23 files changed, 94 insertions(+), 367 deletions(-) diff --git a/components/mcp/adapters/harperHttp.ts b/components/mcp/adapters/harperHttp.ts index b5e83bf8cc..93bee5e553 100644 --- a/components/mcp/adapters/harperHttp.ts +++ b/components/mcp/adapters/harperHttp.ts @@ -43,8 +43,6 @@ interface HarperHttpRequest { ip?: string; } -type SettledCredentialRejection = { status: number; headers: unknown; body: string | Buffer }; - interface HarperHttpResponse { status: number; headers: Record; @@ -59,10 +57,7 @@ export function createHarperHttpHandler(profile: McpProfile) { // WebSocket upgrades aren't ours — let the next handler take it. if (request.isWebSocket) return nextHandler(request); - // This endpoint owns every non-WebSocket request; settle before body or session handling so a - // rejected credential cannot be mapped from an unset `request.user` to an anonymous MCP user. - const settledCredentialRejection = settleDeferredCredentialRejection(request) as - SettledCredentialRejection | undefined; + const settledCredentialRejection = settleDeferredCredentialRejection(request); if (settledCredentialRejection) return settledCredentialRejection; const norm: NormRequest = { diff --git a/integrationTests/security/deferred-credential-rejection.test.ts b/integrationTests/security/deferred-credential-rejection.test.ts index 6af99bdc32..bc6092631c 100644 --- a/integrationTests/security/deferred-credential-rejection.test.ts +++ b/integrationTests/security/deferred-credential-rejection.test.ts @@ -1,13 +1,3 @@ -/** - * End-to-end proof that an app-port credential Harper does not recognize is not rejected until - * route ownership is known. - * - * The chain under test is the real one — `authentication -> rest -> application catch-all` — served - * by a real Harper instance. - * - * Reproduction: - * npm run test:integration -- "integrationTests/security/deferred-credential-rejection.test.ts" - */ import { suite, test, before, after } from 'node:test'; import { equal, ok } from 'node:assert'; import { resolve } from 'node:path'; @@ -19,16 +9,11 @@ import { createApiClient } from '../apiTests/utils/client.mjs'; const FIXTURE_PATH = resolve(import.meta.dirname, 'fixtures/deferred-credential-rejection'); const skipSuite = process.env.HARPER_RUNTIME === 'bun' || process.platform === 'win32'; -/** A WordPress Application Password, base64'd exactly as WordPress sends it — spaces and all. */ const WORDPRESS_BASIC = `Basic ${Buffer.from('wordpress:abcd efgh ijkl mnop qrst uvwx').toString('base64')}`; -/** A session token belonging to the downstream application, not to Harper. */ const DOWNSTREAM_BEARER = 'Bearer eyJhbGciOiJIUzI1NiJ9.d29vLXNlc3Npb24.not-a-harper-token'; -/** A URL no Harper route owns — the shape WooCommerce's REST API uses. */ const APP_ROUTE = '/wp-json/wc/v3/products'; -/** A Harper-owned resource that requires an authenticated principal. */ const PROTECTED_ROUTE = '/Ledger/'; -/** A Harper-owned resource that an anonymous caller may read. */ const PUBLIC_ROUTE = '/PublicNotice/'; suite( @@ -39,7 +24,6 @@ suite( let restURL = ''; let adminAuthorization = ''; - /** Issues a raw request so the exact Authorization header under test reaches the wire unchanged. */ async function get(pathname: string, authorization?: string, extraHeaders: Record = {}) { const response = await fetch(`${restURL}${pathname}`, { headers: { ...(authorization ? { Authorization: authorization } : {}), ...extraHeaders }, @@ -79,8 +63,6 @@ suite( }); test('the application catch-all is mounted after rest, not before it', async () => { - // If the catch-all had been hoisted ahead of `rest`, it would claim this Harper-owned route - // too — which is exactly the trade the issue refuses to make. const owned = await get(PUBLIC_ROUTE, adminAuthorization); equal(owned.status, 200); ok(owned.body?.servedBy !== 'application-catch-all', 'rest must own a Harper resource route'); @@ -101,9 +83,7 @@ suite( equal(response.status, 200, `expected the catch-all to answer: ${response.text}`); equal(response.body.servedBy, 'application-catch-all'); - // No rename, no carrier header, no stripping — the application gets what the client sent. equal(response.body.authorization, WORDPRESS_BASIC); - // And Harper attached no principal on the way through. equal(response.body.harperUser, null); }); @@ -130,8 +110,6 @@ suite( }); test('an unrecognized credential never downgrades a Harper-owned route to public access', async () => { - // Anonymous callers may read PublicNotice, so if a deferred credential simply became - // "anonymous" this would return the record. Harper owns the route, so Harper decides it. const anonymous = await get(PUBLIC_ROUTE); equal(anonymous.status, 200, `PublicNotice must stay anonymously readable: ${anonymous.text}`); ok(anonymous.body?.servedBy !== 'application-catch-all'); @@ -149,18 +127,10 @@ suite( equal(unowned.status, 200); equal(unowned.body.servedBy, 'application-catch-all'); equal(unowned.body.authorization, null); - // No Authorization header means no credential to defer, so Harper's own principal - // resolution runs exactly as it did before: this harness starts Harper with - // AUTHENTICATION_AUTHORIZELOCAL=true, and a loopback caller with no credentials is - // therefore still the local super user. That untouched path is precisely why the - // deferred-credential cases above assert `harperUser === null` — a rejected credential - // must not reach this bypass and be answered as a privileged anonymous request. equal(unowned.body.harperUser, ctx.harper.admin.username); }); test('a deferred-credential response is kept out of shared caches', async () => { - // The application answered using the header Harper passed through, so the response varies by - // credential even though no Harper principal was resolved (#1565's identity floor). const response = await fetch(`${restURL}${APP_ROUTE}`, { headers: { Authorization: WORDPRESS_BASIC } }); equal(response.status, 200); @@ -175,10 +145,6 @@ suite( }); test('a rejected credential on a REST route keeps the authentication error envelope', async () => { - // The wire contract every caller has seen for a rejected credential: `{error: message}` in - // the request's negotiated serialization. REST's own error mapping renders a thrown error as - // an RFC 9457 Problem Details document (`type`/`title`/`status`), which is NOT this, so a - // settlement that went through REST's catch would silently change the response shape. const response = await get(PROTECTED_ROUTE, WORDPRESS_BASIC, { Accept: 'application/json' }); equal(response.status, 401, `expected a generic unauthorized: ${response.text}`); @@ -202,7 +168,6 @@ suite( }); test('/graphql still answers an anonymous request normally', async () => { - // The contrast case: without a credential to reject, GraphQL's own handling is untouched. const response = await fetch(`${restURL}/graphql?query=%7B__typename%7D`, { headers: { Accept: 'application/json' }, }); @@ -211,7 +176,6 @@ suite( }); test('the operations API still rejects an unrecognized credential in place', async () => { - // Every operations route is Harper-owned, so there is nothing to defer to and nothing changes. const response = await fetch(ctx.harper.operationsAPIURL, { method: 'POST', headers: { 'Authorization': WORDPRESS_BASIC, 'Content-Type': 'application/json' }, diff --git a/integrationTests/security/fixtures/deferred-credential-rejection/appCatchAll.js b/integrationTests/security/fixtures/deferred-credential-rejection/appCatchAll.js index 1814b4db28..5d94cb0213 100644 --- a/integrationTests/security/fixtures/deferred-credential-rejection/appCatchAll.js +++ b/integrationTests/security/fixtures/deferred-credential-rejection/appCatchAll.js @@ -1,6 +1,3 @@ -// The application's own middleware, mounted after `rest` so Harper's route ownership gets first -// refusal. It claims whatever reaches it and reports the Authorization header it received, which is -// how the test proves the header arrived byte-for-byte and that no Harper principal was attached. export function handleApplication(scope) { scope.server.http( async (request) => ({ diff --git a/integrationTests/security/fixtures/deferred-credential-rejection/resources.js b/integrationTests/security/fixtures/deferred-credential-rejection/resources.js index 86be5177a4..20120611c2 100644 --- a/integrationTests/security/fixtures/deferred-credential-rejection/resources.js +++ b/integrationTests/security/fixtures/deferred-credential-rejection/resources.js @@ -1,5 +1,3 @@ -// PublicNotice is readable by anyone, including an unauthenticated caller. Everything else keeps -// Harper's default authorization. export class PublicNotice extends tables.PublicNotice { allowRead() { return true; diff --git a/security/auth.ts b/security/auth.ts index 58c77a8cab..02c3cf2aa9 100644 --- a/security/auth.ts +++ b/security/auth.ts @@ -255,8 +255,6 @@ export async function authentication(request, nextHandler) { } break; default: - // Unsupported schemes are credential rejections so a Harper-owned route cannot - // interpret their lack of a Harper principal as anonymous access. throw credentialRejectionError( AUTHENTICATION_ERROR_MSGS.GENERIC_AUTH_FAIL, HTTP_STATUS_CODES.UNAUTHORIZED @@ -271,11 +269,8 @@ export async function authentication(request, nextHandler) { } } - // Only tagged credential rejections on the application port may defer. Operations - // routes are always Harper-owned, and internal faults must fail closed. const internalFault = !isCredentialRejection(err); if (request.isOperationsServer || internalFault) { - // Internal fault details belong in server logs, not authentication responses. if (internalFault) authLogger.error('Authentication failed internally', errorForLog(err)); return applyResponseHeaders({ status: 401, @@ -289,12 +284,9 @@ export async function authentication(request, nextHandler) { } if (credentialRejection) { - // Preserve the header and leave the principal unset until a route owner settles it. deferCredentialRejection(request, credentialRejection, strategy); } else { authorizationCache.set(authorization, newUser); - // `newUser` is null on the legacy blank-Basic-credentials path, which means "no auth" - // and stays anonymous; reading `.username` off it would crash the request. if (LOG_AUTH_SUCCESSFUL && newUser != null) authAuditLog(newUser.username, AUTH_AUDIT_STATUS.SUCCESS, strategy); // Shallow-clone so verifyPerms's `role.permission = fullRolePerms` reassignment @@ -389,10 +381,7 @@ export async function authentication(request, nextHandler) { if (!response) return response; if (response.status === 401) { wasUnauthorized = true; - // A deferred rejection means this 401 came from downstream, not from the in-line rejection - // this middleware used to answer with. Harper's settled rejection has to stay wire-identical - // to that in-line 401 (which returned before any of this ran), and a 401 an application - // catch-all raised is that application's own challenge for its own scheme. + // Downstream owns the challenge or redirect after Harper deferred the credential decision. if (!getDeferredCredentialRejection(request)) { if ( headers['user-agent']?.startsWith('Mozilla') && @@ -416,9 +405,7 @@ export async function authentication(request, nextHandler) { // if we are rejecting the credentials (401, possibly rewritten to a login redirect); such a // response must never be stored by a shared cache and served to a different principal (#1565) const rejectedAuth = response?.status === 401 || wasUnauthorized; - // A deferred credential is still a credential this response was produced under — whoever - // owned the route saw the untouched Authorization header — so #1565's identity floor applies - // even though no Harper principal was resolved and the status may be a plain 200. + // A downstream response produced under the untouched credential remains identity-dependent (#1565). const identityDependent = !!request.user || rejectedAuth || !!getDeferredCredentialRejection(request); // with CORS enabled the response is origin-dependent — ACAO reflects the request Origin, and // its absence when no Origin was sent is origin-dependent too — so a shared cache must diff --git a/security/credentialRejection.ts b/security/credentialRejection.ts index c6122f4ce9..acc3dfc5e4 100644 --- a/security/credentialRejection.ts +++ b/security/credentialRejection.ts @@ -1,16 +1,8 @@ import { ClientError } from '../utility/errors/hdbError.ts'; -/** - * Marks an error as "the presented credential is not acceptable", as opposed to "Harper could not - * evaluate the credential". Only the authentication code that actually reaches that conclusion sets - * it, so provenance is asserted at the throw site rather than inferred from a status code. - * - * Non-enumerable and symbol-keyed: it never serializes, never reaches a client, and cannot be set by - * anything that does not import this module. - */ +// Explicit provenance prevents internal faults with a 4xx status from being deferred as rejected credentials. const CREDENTIAL_REJECTION = Symbol('harper.credentialRejection'); -/** Tags an existing error as a positively identified credential rejection. Returns the same error. */ export function markCredentialRejection(error: E): E { Object.defineProperty(error, CREDENTIAL_REJECTION, { value: true, @@ -21,20 +13,10 @@ export function markCredentialRejection(error: E): E { return error; } -/** Builds the tagged `ClientError` an authentication layer raises when it rejects a credential. */ export function credentialRejectionError(message: string, statusCode: number): ClientError { return markCredentialRejection(new ClientError(message, statusCode)); } -/** - * True only for an error explicitly tagged at the point authentication decided the credential itself - * is unacceptable. - * - * Provenance is never inferred from the status range. `ResourceBridge.searchByValue()` raises a - * default-status-400 `ClientError` when a system table is missing, and `findAndValidateUser()` - * reaches it while lazily loading the user cache — treating that 4xx as a rejected credential would - * let a storage outage hand a Harper request to an application's own authorization (#2418). - */ export function isCredentialRejection(error: unknown): boolean { return (error as Record | null | undefined)?.[CREDENTIAL_REJECTION] === true; } diff --git a/security/deferredAuthentication.ts b/security/deferredAuthentication.ts index 191b08f4f2..10bb517174 100644 --- a/security/deferredAuthentication.ts +++ b/security/deferredAuthentication.ts @@ -4,48 +4,35 @@ import { Headers } from '../server/serverHelpers/Headers.ts'; export { isCredentialRejection, markCredentialRejection, credentialRejectionError } from './credentialRejection.ts'; -/** - * Request-local state recorded when `security/auth.ts` accepts a syntactically valid credential it - * cannot resolve to a Harper principal. It is deliberately not a header, not a `Request` field, and - * not enumerable: nothing outside this module can read, forge, or clear it, and it cannot reach the - * wire or a downstream application. - */ const DEFERRED_CREDENTIAL_REJECTION = Symbol('harper.deferredCredentialRejection'); export type DeferredCredentialRejection = { - /** Status the authentication middleware would have returned in-line. Always 401. */ - status: number; - /** The rejection message that middleware would have carried, so the deferred response is identical. */ - message: string; - /** `Basic`, `Bearer`, or whatever scheme token preceded the credential. */ - strategy: string; + readonly status: number; + readonly message: string; + readonly strategy: string; }; -/** - * The status every credential rejection resolves to, whether it is answered in-line or deferred. - * Authentication answers a rejected credential with 401 regardless of the underlying error's own - * `statusCode`, so pinning it here keeps immediate and deferred rejections byte-identical. - */ const CREDENTIAL_REJECTION_STATUS = 401; /** * Records that this request presented a credential Harper rejected, without deciding the request. * The caller leaves `request.user` unset and the inbound `Authorization` header untouched. * - * Installed non-enumerable so an application catch-all that spreads or `Reflect.ownKeys`-walks the - * request cannot observe it: object spread copies enumerable symbol-keyed properties. + * The immutable, non-enumerable descriptor prevents downstream middleware from clearing it and + * keeps it out of request copies and serialization. */ export function deferCredentialRejection(request: any, error: { message?: string }, strategy: string): void { - const deferred: DeferredCredentialRejection = { + if (getDeferredCredentialRejection(request)) return; + const deferred: DeferredCredentialRejection = Object.freeze({ status: CREDENTIAL_REJECTION_STATUS, message: error?.message ?? 'Unauthorized', strategy, - }; + }); Object.defineProperty(request, DEFERRED_CREDENTIAL_REJECTION, { value: deferred, enumerable: false, - configurable: true, - writable: true, + configurable: false, + writable: false, }); } @@ -70,21 +57,14 @@ export function settleDeferredCredentialRejection( ): { status: number; headers: Headers; body: string | Buffer } | undefined { const deferred = getDeferredCredentialRejection(request); if (!deferred) return undefined; - // Name the serializer explicitly so the response body remains self-describing. const contentType = (request?.headers ? findBestSerializer(request).type : undefined) ?? 'application/json'; return { status: deferred.status, - // A real Headers, not a plain object: authentication stamps the #1565 identity floor onto - // whatever an owning layer returns, and the HTTP bridges read it back through `get`. headers: new Headers({ 'Content-Type': contentType }), body: serializeMessage({ error: deferred.message }, request) as string | Buffer, }; } -/** - * Throwing form of `settleDeferredCredentialRejection`, for owners with no response descriptor to - * return — a WebSocket upgrade closes the socket with a status-derived close code instead. - */ export function assertNoDeferredCredentialRejection(request: any): void { const deferred = getDeferredCredentialRejection(request); if (deferred) throw new ClientError(deferred.message, deferred.status); diff --git a/security/tokenAuthentication.ts b/security/tokenAuthentication.ts index d25effaf5f..926157a31d 100644 --- a/security/tokenAuthentication.ts +++ b/security/tokenAuthentication.ts @@ -459,47 +459,23 @@ async function validateToken(token: string, tokenType: string): Promise { if (err?.name === 'TokenExpiredError') { throw credentialRejectionError(AUTHENTICATION_ERROR_MSGS.TOKEN_EXPIRED, HTTP_STATUS_CODES.FORBIDDEN); } - // Only a client-side rejection may be reported as one. Everything else here — unreadable or - // malformed JWT key material, a storage failure inside findAndValidateUser, a bug — propagates - // unmasked, because callers distinguish a rejected credential from an internal authentication - // fault and only the former is deferred past route matching. Masking a fault as - // `invalid token` would let a key or storage outage read as an unknown credential. + // Only positively classified token failures may cross the route-ownership boundary as rejections. if (!isTokenRejection(err)) throw err; throw credentialRejectionError(AUTHENTICATION_ERROR_MSGS.INVALID_TOKEN, HTTP_STATUS_CODES.UNAUTHORIZED); } } -/** - * `jsonwebtoken` error names that describe the *token*: syntax, signature, subject/audience claims, - * and the not-before/expiry windows. Anything else it raises is about Harper's own configuration. - */ const JWT_REJECTION_ERROR_NAMES = new Set(['JsonWebTokenError', 'NotBeforeError', 'TokenExpiredError']); -/** - * `jsonwebtoken` reports an unusable verification key through the same `JsonWebTokenError` type it - * uses for a bad token, distinguished only by message — either its own `secretOrPublicKey…` guards - * or a passed-through OpenSSL failure. Those are Harper-side faults and must never be reported to a - * client as a rejected credential. - */ +// jsonwebtoken reports unusable verification keys and bad tokens through the same error type. const KEY_MATERIAL_FAULT = /secretOrPublicKey|asymmetric key|PEM routines|^error:/i; -/** - * True only when `err` says the presented token is not acceptable, rather than that Harper failed to - * evaluate it. Never inferred from the 4xx range: `findAndValidateUser()` lazily loads the user cache - * and can surface a default-status-400 `ClientError` from a missing system table, which is a storage - * fault wearing a client-error status. - */ function isTokenRejection(err: any): boolean { if (isCredentialRejection(err)) return true; if (!JWT_REJECTION_ERROR_NAMES.has(err?.name)) return false; return !KEY_MATERIAL_FAULT.test(String(err?.message ?? '')); } -/** - * Fails closed before `jwt.verify()` when the configured public key cannot be verification key - * material at all. Without this, `jsonwebtoken` folds the failure into a `JsonWebTokenError`, which - * is otherwise indistinguishable from a forged signature. - */ function assertUsableVerificationKey(publicKey: unknown): void { if (typeof publicKey !== 'string' || !publicKey.includes('-----BEGIN')) { throw new ServerError(AUTHENTICATION_ERROR_MSGS.NO_ENCRYPTION_KEYS, HTTP_STATUS_CODES.INTERNAL_SERVER_ERROR); diff --git a/security/user.ts b/security/user.ts index 8e5206563a..ae2d80058d 100644 --- a/security/user.ts +++ b/security/user.ts @@ -421,8 +421,6 @@ async function findAndValidateUser(username: string, pw?: string | null, validat const userTmp = usersWithRolesMap.get(username); if (!userTmp) { if (!validatePassword) return { username }; - // The tag distinguishes an absent user from user-cache faults that share the 4xx range but must - // fail closed instead of deferring to application authorization. throw credentialRejectionError(AUTHENTICATION_ERROR_MSGS.GENERIC_AUTH_FAIL, HTTP_STATUS_CODES.UNAUTHORIZED); } diff --git a/server/DESIGN.md b/server/DESIGN.md index a18f799b9c..f80d030844 100644 --- a/server/DESIGN.md +++ b/server/DESIGN.md @@ -221,9 +221,9 @@ So a rejection is recorded rather than answered: - **No credentials** continue anonymously. Unchanged. - **A syntactically valid credential Harper does not recognize** leaves `request.user` unset, leaves the inbound `Authorization` header byte-for-byte intact, and records request-local state through - `security/deferredAuthentication.ts`. The state lives behind a module-private `Symbol`: it is not - a header, not a `Request` field, not enumerable, and cannot be forged or read from outside that - module. + `security/deferredAuthentication.ts`. The state lives behind a module-private `Symbol`. Its + descriptor and value are immutable and non-enumerable, so downstream middleware cannot clear it + and it does not leak through request copies or serialization. - **An internal authentication fault** — unreadable or malformed JWT key material, a storage failure, an unexpected error type — is never deferred, and fails closed with the in-line 401. - **The operations API** never defers: `request.isOperationsServer` short-circuits to the in-line diff --git a/server/REST.ts b/server/REST.ts index 9f564f4c27..d86be3a777 100644 --- a/server/REST.ts +++ b/server/REST.ts @@ -226,8 +226,6 @@ async function http(request: Request, nextHandler, resources: Resources, httpOpt } } } - // A matched resource or OpenAPI document settles ownership. Return authentication's response - // descriptor directly so REST's Problem Details mapping cannot change its wire contract. const settledCredentialRejection = settleDeferredCredentialRejection(request); if (settledCredentialRejection) return settledCredentialRejection; if ((resource as any)?.isCaching) { @@ -561,7 +559,6 @@ export function handleApplication(scope: import('../components/Scope.ts').Scope) // TODO: Ideally we would like to have a 404 response before upgrading to WebSocket protocol, probably return ws.close(1011, `No resource was found to handle ${request.pathname}`); } else { - // A matched resource owns this socket; do not carry rejection into it as anonymous. assertNoDeferredCredentialRejection(request); request.handlerPath = entry.path; recordAction( diff --git a/server/graphqlQuerying.ts b/server/graphqlQuerying.ts index cb001c05f1..3af0de514b 100644 --- a/server/graphqlQuerying.ts +++ b/server/graphqlQuerying.ts @@ -581,8 +581,6 @@ export function handleApplication(scope: import('../components/Scope.ts').Scope) return nextLayer(request); } - // GraphQL owns this route. Settle before its error mapping can change authentication's - // negotiated `{error: message}` response or expose the request to resolvers as anonymous. const settledCredentialRejection = settleDeferredCredentialRejection(request); if (settledCredentialRejection) return settledCredentialRejection; diff --git a/server/http.ts b/server/http.ts index 2677caf7a4..39aca70162 100644 --- a/server/http.ts +++ b/server/http.ts @@ -1024,8 +1024,6 @@ export function makeUwsHandler(port: number | string, isOperationsServer: boolea if (Array.isArray(v)) respHeaders.set(k, k.toLowerCase() === 'set-cookie' ? v : v.join(', ')); else respHeaders.set(k, String(v)); } - // Preserve the chain's identity/cache floor while allowing Fastify-set headers to win, - // matching the Node fallback's header precedence. mergeChainHeadersIntoFallback(headers, respHeaders); if (universalHeaders.length > 0) applyUniversalHeaders(respHeaders); logHttpRequest(request, injectResult.statusCode, requestId, performance.now() - startTime); @@ -1050,8 +1048,6 @@ export function makeUwsHandler(port: number | string, isOperationsServer: boolea } logHttpRequest(request, 404, requestId, performance.now() - startTime); const notFoundHeaders = new Headers({ 'content-type': 'text/plain' }); - // A 404 produced under a credential is credential-dependent too, so it keeps the same floor - // the fallback branch above preserves. mergeChainHeadersIntoFallback(headers, notFoundHeaders); if (universalHeaders.length > 0) applyUniversalHeaders(notFoundHeaders); return { status: 404, headers: notFoundHeaders, body: 'Not found\n' }; @@ -1416,11 +1412,6 @@ export function registerFastifyInstance(port: string | number, instance: any) { fastifyInstances[port] = instance; } -/** - * Records the legacy Fastify `http.Server` the Bun and uWS adapters delegate an unhandled request to. - * Both runtimes divert a non-function `server.http()` listener here instead of binding it, because - * neither backs its port with a Node http server. - */ export function registerFallbackServer(port: string | number, listener: any) { fallbackServers[port] = listener; } @@ -1486,7 +1477,6 @@ export async function bunDelegateToNodeServer( if (webRequest.headers.get('connection')?.toLowerCase() === 'close') { webHeaders.set('connection', 'close'); } - // Preserve the chain's identity/cache floor while allowing Fastify-set headers to win. mergeChainHeadersIntoFallback(chainHeaders, webHeaders); if (universalHeaders.length > 0) applyUniversalHeaders(webHeaders); const responseStream = injectResult.stream(); @@ -1539,24 +1529,14 @@ const resolvedChainDescriptions: Record> = { http: new Map(), upgrade: new Map(), websocket: new Map(), }; -/** - * Builds `chains[port]` from the current `listeners` and, when `port` is the 'all' pseudo-port, - * rebuilds every other already-built chain of the same kind too. - * - * A late registration on 'all' must rebuild every concrete port; rebuilding only `chains.all` - * leaves bound ports with stale listener order. Chain construction is a pure function of the - * listener list and port, so rebuilding cannot alter earlier ordering decisions. - */ +// A late registration on 'all' must rebuild every already-bound concrete port (#2418). function buildChains( chains: Record, listeners: HttpEntry[], diff --git a/server/serverHelpers/Headers.ts b/server/serverHelpers/Headers.ts index 65b3f45c9f..f8a5fac091 100644 --- a/server/serverHelpers/Headers.ts +++ b/server/serverHelpers/Headers.ts @@ -194,7 +194,6 @@ export function mergeChainHeadersIntoFallback< if (lowerName === 'vary' || lowerName === 'cache-control') continue; if (finalHeaders.has(name)) continue; if (Array.isArray(value)) { - // Set-Cookie is the multi-valued case that must never be comma-joined. for (const single of value) appendHeader(finalHeaders, name, single, lowerName !== 'set-cookie'); } else finalHeaders.set(name, value); } @@ -215,10 +214,6 @@ export function mergeChainHeadersIntoFallback< return finalHeaders; } -/** - * Presents a Node `ServerResponse`'s live header set through the Headers-like surface - * `mergeChainHeadersIntoFallback` and `addVaryHeader` expect. - */ function nodeResponseHeaders(nodeResponse: any) { return { get: (name: string) => nodeResponse.getHeader(name), @@ -234,20 +229,31 @@ function nodeResponseHeaders(nodeResponse: any) { }; } -/** `writeHead` accepts a flat `[name, value, …]` array, a `[name, value][]` array, or an object. */ function applyWriteHeadHeaders(nodeResponse: any, headers: any): void { + const suppliedHeaders = new Map(); + const addHeader = (name: string, value: any) => { + const key = String(name).toLowerCase(); + const supplied = suppliedHeaders.get(key); + if (!supplied) { + suppliedHeaders.set(key, { name, value }); + return; + } + const values = Array.isArray(supplied.value) ? supplied.value : [supplied.value]; + supplied.value = Array.isArray(value) ? values.concat(value) : values.concat([value]); + }; if (Array.isArray(headers)) { if (Array.isArray(headers[0])) { - for (const [name, value] of headers) nodeResponse.setHeader(name, value); + for (const [name, value] of headers) addHeader(name, value); } else { - for (let i = 0; i + 1 < headers.length; i += 2) nodeResponse.setHeader(headers[i], headers[i + 1]); + for (let i = 0; i + 1 < headers.length; i += 2) addHeader(headers[i], headers[i + 1]); + } + } else { + for (const name of Object.keys(headers)) { + const value = headers[name]; + if (value != null) addHeader(name, value); } - return; - } - for (const name of Object.keys(headers)) { - const value = headers[name]; - if (value != null) nodeResponse.setHeader(name, value); } + for (const { name, value } of suppliedHeaders.values()) nodeResponse.setHeader(name, value); } /** diff --git a/server/static.ts b/server/static.ts index 35804e6f3b..e49e19065c 100644 --- a/server/static.ts +++ b/server/static.ts @@ -312,8 +312,6 @@ export function handleApplication(scope: Scope) { }); scope.server.http( - // Every response this handler originates claims the URL, so settle before redirects, files, or - // non-fallthrough not-found handling. Only `next(req)` leaves ownership and rejection unsettled. (req, next) => { // TODO: Not sure if the isWebSocket check is still necessary if (req.method !== 'GET' || req.isWebSocket) return next(req); @@ -340,19 +338,7 @@ export function handleApplication(scope: Scope) { // Retrieve index entry staticFile = indexEntries.get(req.pathname); - // The router strips both '/assets' and '/assets/' down to '/', so the mount root - // must be disambiguated via the unstripped pathname (exposed by stripPrefix): - // redirect the no-slash form so relative links on the index page resolve under - // the mount (#1583). Gated on the EXTERNAL base path, not the plugin-local one: a - // root-level static plugin (baseURLPath === '/') still needs this redirect when the - // application itself carries a host/urlPath mount, since the client-visible mount root - // is then externalBaseURLPath, not '/' (review finding). - // The other form is the `null` index entry — a registered directory redirecting to its - // trailing-slash form; req.pathname arrives with the mount prefix stripped, so the - // external path is rebuilt for the Location header (#1583). The two are mutually - // exclusive, since a `null` entry is never the mount-root serve. They share one branch - // so both settle a deferred credential rejection before redirecting; the query string - // is built inside it, keeping the common (non-redirect) index serve allocation-free. + // Prefix stripping makes `originalPathname` necessary to distinguish mounted roots (#1583). const originalPathname: string | undefined = (req as any).originalPathname; const redirectsMountRoot = !!( staticFile && @@ -412,7 +398,6 @@ export function handleApplication(scope: Scope) { return next(req); } - // This handler owns both not-found forms, so settle before resolving the configured body. const settledCredentialRejection = settleDeferredCredentialRejection(req); if (settledCredentialRejection) return settledCredentialRejection; diff --git a/unitTests/components/mcp/adapters/harperHttp.test.js b/unitTests/components/mcp/adapters/harperHttp.test.js index ed835943bf..7c0add61a6 100644 --- a/unitTests/components/mcp/adapters/harperHttp.test.js +++ b/unitTests/components/mcp/adapters/harperHttp.test.js @@ -100,12 +100,6 @@ describe('mcp/adapters/harperHttp', () => { }); describe('deferred credential rejection (#2418)', () => { - /** - * `mcp.application` mounts this handler `after: 'authentication'`, and REST declines an - * unmatched `/mcp`, so this handler is where route ownership is finally known. Authentication - * leaves `request.user` unset — the same shape as an anonymous request. Settlement prevents the - * credential from opening an anonymous MCP session. - */ function deferredRequest(overrides = {}) { const request = { method: 'POST', @@ -146,7 +140,6 @@ describe('mcp/adapters/harperHttp', () => { }); it('still serves a request with no deferred rejection', async () => { - // The contrast case: identical request minus the deferral produces a real session. const handler = createHarperHttpHandler('application'); const request = { method: 'POST', @@ -164,8 +157,6 @@ describe('mcp/adapters/harperHttp', () => { }); it('still lets a WebSocket upgrade through to the next handler', async () => { - // Upgrades are not this handler's route, so ownership is not settled here and the deferred - // rejection is left for whichever layer does own the socket. const handler = createHarperHttpHandler('application'); const out = await handler(deferredRequest({ method: 'GET', isWebSocket: true }), () => 'next-handler-result'); diff --git a/unitTests/security/authCredentialDeferral.test.js b/unitTests/security/authCredentialDeferral.test.js index dd457ed8c7..e8f28758df 100644 --- a/unitTests/security/authCredentialDeferral.test.js +++ b/unitTests/security/authCredentialDeferral.test.js @@ -1,10 +1,3 @@ -/** - * Drives the real `authentication` middleware through a real `authentication -> rest -> application - * catch-all` chain built by `server/middlewareChain.ts`. - * - * On the pre-fix revision every "reaches the application catch-all" case here fails: `security/auth.ts` - * answered 401 during credential parsing, so the chain terminated before route ownership was known. - */ const assert = require('node:assert'); const testUtils = require('../testUtils.js'); @@ -20,12 +13,9 @@ const tokenAuthentication = require('#src/security/tokenAuthentication'); const { authentication } = require('#src/security/auth'); const HARPER_OWNED = '/Ledger/1'; -// A Harper-owned route that serves anonymous callers — the case where 'continued as anonymous' -// and 'rejected the credential' produce visibly different responses. const HARPER_OWNED_PUBLIC = '/PublicNotice/1'; const APP_OWNED = '/wp-json/wc/v3/products'; -// A WordPress Application Password, base64'd exactly as WordPress sends it — spaces and all. const WORDPRESS_BASIC = `Basic ${Buffer.from('wordpress:abcd efgh ijkl mnop qrst uvwx').toString('base64')}`; const HARPER_BASIC = `Basic ${Buffer.from('harper_admin:harper-pw').toString('base64')}`; const DOWNSTREAM_BEARER = 'Bearer eyJhbGciOiJIUzI1NiJ9.d29vLXNlc3Npb24.not-a-harper-token'; @@ -50,24 +40,14 @@ describe('deferred credential rejection through the app-port middleware chain', let originalGetUser; let originalValidateOperationToken; let originalValidateRefreshToken; - /** Records what each layer saw, so "which layer answered" is observable rather than inferred. */ let trace; - /** Pathnames Harper claims ownership of, standing in for `resources.getMatch`. */ let ownedPaths; - /** Users the Harper credential store recognizes, keyed by `username:password`. */ let knownUsers; - /** When set, `getUser` raises this instead of resolving — an internal authentication fault. */ let getUserFault; - /** - * Mirrors `server/REST.ts`'s ownership branch: unowned URLs pass to the next layer untouched, - * owned ones settle any deferred credential through the same production assertion REST calls. - */ function restLayer(request, nextHandler) { if (!ownedPaths.has(request.pathname)) return nextHandler(request); trace.push('rest'); - // The production settlement helper, not a local re-implementation: it is what decides the - // status, body, and content type an owning Harper layer returns. const settled = settleDeferredCredentialRejection(request); if (settled) return settled; if (!request.user && request.pathname !== HARPER_OWNED_PUBLIC) @@ -79,10 +59,8 @@ describe('deferred credential rejection through the app-port middleware chain', }; } - /** When set, the catch-all answers with this instead of its default 200. */ let catchAllResponse; - /** The application's own middleware, mounted after `rest`, applying its own auth scheme. */ function applicationCatchAll(request) { trace.push('catch-all'); if (catchAllResponse) return catchAllResponse(); @@ -118,8 +96,6 @@ describe('deferred credential rejection through the app-port middleware chain', originalValidateOperationToken = tokenAuthentication.validateOperationToken; originalValidateRefreshToken = tokenAuthentication.validateRefreshToken; - // The stubs raise what production raises: `findAndValidateUser()` and `validateToken()` tag a - // rejected credential explicitly, and an untagged error is by construction an internal fault. serverModule.server.getUser = async (username, password) => { if (getUserFault) throw getUserFault; const user = knownUsers.get(`${username}:${password}`); @@ -140,8 +116,6 @@ describe('deferred credential rejection through the app-port middleware chain', tokenAuthentication.validateRefreshToken = originalValidateRefreshToken; }); - // `resources.loginPath` is read by authentication's 401 rewriting; the registry is a live binding - // that only exists once something has built it. before(() => { if (!resourcesModule.resources) resourcesModule.resetResources(); }); @@ -160,8 +134,6 @@ describe('deferred credential rejection through the app-port middleware chain', }); it('resolves the chain as authentication -> rest -> application catch-all', async () => { - // The order is what makes the rest of this suite meaningful: `rest` must get first refusal on - // every URL, and the application middleware must only see what `rest` declined. const { body } = await send(APP_OWNED, undefined); assert.deepStrictEqual(trace, ['catch-all']); @@ -186,11 +158,8 @@ describe('deferred credential rejection through the app-port middleware chain', assert.strictEqual(response.status, 200); assert.strictEqual(body.servedBy, 'catch-all'); - // The header the application receives must be the header the client sent — no rename, no - // carrier header, no stripping. assert.strictEqual(body.authorization, WORDPRESS_BASIC); assert.strictEqual(request.headers.asObject.authorization, WORDPRESS_BASIC); - // And no Harper principal was invented along the way. assert.strictEqual(body.harperUser, null); assert.strictEqual(request.user, undefined); }); @@ -204,8 +173,6 @@ describe('deferred credential rejection through the app-port middleware chain', }); it('lets a Harper refresh token keep its declined (-1) handling instead of deferring', async () => { - // A refresh token is Harper's own credential: `authentication` declines the request so the - // operations API can handle it, and that must not turn into a deferral to the application. tokenAuthentication.validateRefreshToken = async () => ({ username: 'harper_admin' }); try { const { response } = await send(APP_OWNED, 'Bearer harper-refresh-token'); @@ -254,8 +221,6 @@ describe('deferred credential rejection through the app-port middleware chain', }); it('does not downgrade a Harper-owned route to public just because the credential was unknown', async () => { - // This route serves anonymous callers, so an unknown credential that merely became - // "anonymous" would be handed the content. The deferred rejection wins instead. const anonymous = await send(HARPER_OWNED_PUBLIC, undefined); assert.strictEqual(anonymous.response.status, 200); @@ -265,8 +230,6 @@ describe('deferred credential rejection through the app-port middleware chain', }); it('defers a scheme Harper does not implement rather than continuing anonymously', async () => { - // Reported by review on this PR: `Digest` matches no case in the strategy switch and throws - // nothing, so before this it fell through as an anonymous request. const digest = 'Digest username="wp", realm="site", response="0123456789abcdef"'; const { request, response, body } = await send(APP_OWNED, digest); @@ -277,8 +240,6 @@ describe('deferred credential rejection through the app-port middleware chain', }); it('rejects a scheme Harper does not implement at an anonymously-readable Harper route', async () => { - // The decisive case: this route serves anonymous callers, so continuing as anonymous would - // return 200. Only an actual deferred rejection produces the 401. const anonymous = await send(HARPER_OWNED_PUBLIC, undefined); assert.strictEqual(anonymous.response.status, 200); @@ -303,15 +264,12 @@ describe('deferred credential rejection through the app-port middleware chain', }); it('keeps the legacy blank Basic credential anonymous instead of deferring it', async () => { - // `Basic ` + base64(':') is the documented "no auth" form: it must stay anonymous, so the - // unrecognized-scheme rejection above is gated on a strictly `undefined` user, not a nullish one. const blank = `Basic ${Buffer.from(':').toString('base64')}`; const { request, response, body } = await send(APP_OWNED, blank); assert.strictEqual(response.status, 200); assert.strictEqual(body.servedBy, 'catch-all'); assert.strictEqual(request.user, null); - // Anonymous, not deferred — so an anonymously-readable Harper route still serves it. const owned = await send(HARPER_OWNED_PUBLIC, blank); assert.strictEqual(owned.response.status, 200); }); @@ -331,7 +289,6 @@ describe('deferred credential rejection through the app-port middleware chain', const { response } = await send(APP_OWNED, WORDPRESS_BASIC); assert.strictEqual(response.status, 401); - // The whole point: an outage must not hand the request to the application's own authorization. assert.deepStrictEqual(trace, []); }); @@ -345,11 +302,6 @@ describe('deferred credential rejection through the app-port middleware chain', }); it('fails closed on an internal fault that happens to carry a 4xx status', async () => { - // The exact production shape: `findAndValidateUser()` lazily loads the user cache, whose - // system-table searches reach `ResourceBridge.searchByValue()` and raise a `ClientError` with - // the default 400 status when `system.hdb_role`/`system.hdb_user` is unavailable. Classifying - // by status range read that as an ordinary unknown credential and deferred it, so an unowned - // URL reached the application catch-all during a storage outage. getUserFault = new ClientError('Table system.hdb_role not found'); assert.strictEqual(getUserFault.statusCode, 400, 'the fault must actually be in the 4xx range'); @@ -370,9 +322,6 @@ describe('deferred credential rejection through the app-port middleware chain', }); it('propagates a refresh-validation fault instead of restoring the deferrable outer rejection', async () => { - // The operation-token path falls back to refresh-token validation on `invalid token`. Discarding - // whatever that raises and rethrowing the outer ordinary rejection let a refresh-side storage or - // runtime fault be classified as a deferrable unknown credential. tokenAuthentication.validateRefreshToken = async () => { throw new ServerError('refresh token store unavailable'); }; @@ -406,8 +355,6 @@ describe('deferred credential rejection through the app-port middleware chain', }); it('restores the operation-token rejection after an ordinary refresh rejection, and defers it', async () => { - // The other half of the same branch: an ordinary tagged refresh rejection still yields the - // original `invalid token`, which is deferrable. const { response, body } = await send(APP_OWNED, DOWNSTREAM_BEARER); assert.strictEqual(response.status, 200); @@ -416,8 +363,6 @@ describe('deferred credential rejection through the app-port middleware chain', }); it("answers a Harper-owned route with the authentication error envelope, not the owner's", async () => { - // `{error: message}` in the request's negotiated serialization is what authentication returned - // in line before deferral existed; REST's RFC 9457 Problem Details mapping must not replace it. const { response, body } = await send(HARPER_OWNED, WORDPRESS_BASIC); assert.strictEqual(response.status, 401); @@ -433,8 +378,6 @@ describe('deferred credential rejection through the app-port middleware chain', }); it('marks a deferred-credential response as identity-dependent for shared caches', async () => { - // The application answered using the Authorization header Harper passed through, so the - // response is credential-dependent even though no Harper principal was resolved (#1565). const { response } = await send(APP_OWNED, WORDPRESS_BASIC); assert.strictEqual(response.headers.get('Vary').includes('Authorization'), true); @@ -447,12 +390,6 @@ describe('deferred credential rejection through the app-port middleware chain', assert.strictEqual(response.headers?.get?.('Cache-Control') ?? null, null); }); describe('401 post-processing ownership', () => { - // `security/auth.ts` rewrites any 401 that comes back up the chain: it overwrites - // `WWW-Authenticate` with `Basic`, or turns the 401 into a 302 to Harper's login page for a - // browser. Before deferral existed a rejected credential returned in line and never reached - // that code, and a rejected credential never reached an application catch-all at all. - - /** A browser request: the exact shape that triggers the login-page rewrite. */ const BROWSER = { 'user-agent': 'Mozilla/5.0 (Macintosh)', 'accept': 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8', @@ -473,8 +410,6 @@ describe('deferred credential rejection through the app-port middleware chain', } it("leaves an application catch-all's own 401 challenge untouched", async () => { - // The Woo/WordPress case the issue is about: the application owns the route, applies its own - // scheme, and answers with its own challenge. Harper must not rewrite it to `Basic`. catchAllResponse = () => ({ status: 401, headers: new Headers({ 'WWW-Authenticate': 'Basic realm="WooCommerce", charset="UTF-8"' }), @@ -504,8 +439,6 @@ describe('deferred credential rejection through the app-port middleware chain', }); it('still applies the identity floor to an application-owned 401', async () => { - // Suppressing the rewrite must not also suppress #1565: the response was produced under the - // credential Harper passed through. catchAllResponse = () => ({ status: 401, headers: new Headers(), body: '{}' }); const { response } = await send(APP_OWNED, WORDPRESS_BASIC); @@ -519,8 +452,6 @@ describe('deferred credential rejection through the app-port middleware chain', const { response } = await sendBrowser(HARPER_OWNED, WORDPRESS_BASIC); - // The pre-deferral middleware returned this directly from its own catch: status 401, the - // `{error}` envelope, and no challenge or login redirect bolted on afterwards. assert.strictEqual(response.status, 401); assert.strictEqual(response.headers.get('Location') ?? null, null); assert.strictEqual(response.headers.get('WWW-Authenticate') ?? null, null); @@ -528,7 +459,6 @@ describe('deferred credential rejection through the app-port middleware chain', }); it('still redirects a browser with no credentials at all to the login page', async () => { - // The control: nothing was deferred, so Harper's own 401 handling is unchanged. resourcesModule.resources.loginPath = () => '/login'; const { response } = await sendBrowser(HARPER_OWNED, undefined); diff --git a/unitTests/security/deferredAuthentication.test.js b/unitTests/security/deferredAuthentication.test.js index 21b58af5e4..b1c744b2c4 100644 --- a/unitTests/security/deferredAuthentication.test.js +++ b/unitTests/security/deferredAuthentication.test.js @@ -11,7 +11,6 @@ const { } = require('#src/security/deferredAuthentication'); const { ClientError, ServerError } = require('#src/utility/errors/hdbError'); -/** A request shaped enough for content negotiation (`findBestSerializer` reads `headers.asObject`). */ function requestAccepting(accept, extraHeaders = {}) { const asObject = { ...extraHeaders }; if (accept) asObject.accept = accept; @@ -31,10 +30,6 @@ describe('deferredAuthentication', () => { }); it('never infers rejection from the 4xx range', () => { - // The regression this guards: `findAndValidateUser()` lazily loads the user cache, whose - // fixed system-table searches raise a default-status-400 ClientError when `system.hdb_role` - // or `system.hdb_user` is unavailable. Deferring that would hand a storage outage to an - // application's own authorization. assert.strictEqual(isCredentialRejection(new ClientError('Table system.hdb_role not found')), false); assert.strictEqual(isCredentialRejection(new ClientError('Login failed', 401)), false); assert.strictEqual(isCredentialRejection(new ClientError('token expired', 403)), false); @@ -52,7 +47,6 @@ describe('deferredAuthentication', () => { }); it('cannot be forged from outside the module', () => { - // The tag is a module-private symbol, so neither a string key nor a registered symbol works. const forged = { 'credentialRejection': true, 'harper.credentialRejection': true, @@ -82,13 +76,26 @@ describe('deferredAuthentication', () => { assert.ok(stateSymbol, 'the deferred state should be recorded under its own symbol'); const descriptor = Object.getOwnPropertyDescriptor(request, stateSymbol); assert.strictEqual(descriptor.enumerable, false); - assert.strictEqual(descriptor.configurable, true); + }); + + it('cannot be cleared or changed by downstream middleware using reflection', () => { + const request = requestAccepting('application/json'); + deferCredentialRejection(request, credentialRejectionError('Login failed', 401), 'Basic'); + const stateSymbol = Object.getOwnPropertySymbols(request).find( + (symbol) => symbol.description === 'harper.deferredCredentialRejection' + ); + + Reflect.deleteProperty(request, stateSymbol); + Reflect.set(request, stateSymbol, undefined); + const state = request[stateSymbol]; + if (state) Reflect.set(state, 'status', 200); + + const settled = settleDeferredCredentialRejection(request); + assert.strictEqual(settled.status, 401); + assert.deepStrictEqual(JSON.parse(settled.body.toString()), { error: 'Login failed' }); }); it('does not survive object spread into a downstream application copy', () => { - // Object spread copies enumerable symbol-keyed properties, so an enumerable descriptor here - // would put internal authentication state into whatever an application catch-all builds - // from the request. const request = { headers: { authorization: 'Basic d3A6c2VjcmV0' } }; deferCredentialRejection(request, credentialRejectionError('Login failed', 401), 'Basic'); @@ -115,8 +122,6 @@ describe('deferredAuthentication', () => { }); it('pins the deferred status to 401 even when the underlying rejection was a 403', () => { - // The authentication middleware has always answered a rejected credential with 401 - // regardless of the error's own status, so a deferred rejection has to match that. const request = {}; deferCredentialRejection(request, credentialRejectionError('token expired', 403), 'Bearer'); @@ -134,6 +139,18 @@ describe('deferredAuthentication', () => { assert.strictEqual(getDeferredCredentialRejection(request).message, 'Unauthorized'); }); + it('preserves the first rejection when multiple route chains authenticate the request', () => { + const request = {}; + deferCredentialRejection(request, credentialRejectionError('Login failed', 401), 'Basic'); + deferCredentialRejection(request, credentialRejectionError('invalid token', 401), 'Bearer'); + + assert.deepStrictEqual(getDeferredCredentialRejection(request), { + status: 401, + message: 'Login failed', + strategy: 'Basic', + }); + }); + it('is readable through a proxy of the request, as the urlPath-mount chain produces', () => { const request = {}; deferCredentialRejection(request, credentialRejectionError('Login failed', 401), 'Basic'); @@ -166,8 +183,6 @@ describe('deferredAuthentication', () => { }); it('returns a real Headers, which the middleware 401 post-processing writes into', () => { - // `security/auth.ts` calls `response.headers.set()` on whatever an owning layer returns — - // WWW-Authenticate, or a Location when a login page is configured. A plain object 500s there. const request = requestAccepting('application/json'); deferCredentialRejection(request, credentialRejectionError('Login failed', 401), 'Basic'); @@ -179,9 +194,6 @@ describe('deferredAuthentication', () => { }); it('reproduces the authentication middleware response: 401 with an {error} body', () => { - // This is the wire contract callers have always seen for a rejected credential. An owning - // layer's own error mapping (REST's RFC 9457 Problem Details, GraphQL's {errors:[…]}) must - // not replace it. const request = requestAccepting('application/json'); deferCredentialRejection(request, credentialRejectionError('Login failed', 401), 'Basic'); diff --git a/unitTests/security/tokenRejectionClassification.test.js b/unitTests/security/tokenRejectionClassification.test.js index 1110f5795f..d79410d50b 100644 --- a/unitTests/security/tokenRejectionClassification.test.js +++ b/unitTests/security/tokenRejectionClassification.test.js @@ -1,11 +1,5 @@ 'use strict'; -/** - * `validateToken()` decides whether a Bearer failure is "this token is not acceptable" or "Harper - * could not evaluate it". Only the first may be deferred past route matching, so this suite - * drives the real `validateOperationToken`/`validateRefreshToken` against real RSA key material and - * asserts the classification the authentication middleware then acts on. - */ const testUtils = require('../testUtils.js'); testUtils.preTestPrep(); @@ -27,7 +21,6 @@ const { setUsersWithRolesCache } = require('#src/security/user'); const KNOWN_USER = new Map([['known_user', { username: 'known_user', active: true, role: { permission: {} } }]]); -/** Captures the error `fn()` raises, so a resolving call fails loudly instead of silently passing. */ async function raisedBy(fn) { try { await fn(); @@ -54,7 +47,6 @@ describe('token rejection versus internal authentication fault', () => { const keysDir = path.join(env.getHdbBasePath(), LICENSE_KEY_DIR_NAME); publicKeyPath = path.join(keysDir, JWT_ENUM.JWT_PUBLIC_KEY_NAME); installedPublicKey = fs.readFileSync(publicKeyPath, 'utf8'); - // Sign with the exact keys validateOperationToken will verify against. signingKey = { key: fs.readFileSync(path.join(keysDir, JWT_ENUM.JWT_PRIVATE_KEY_NAME), 'utf8'), passphrase: fs.readFileSync(path.join(keysDir, JWT_ENUM.JWT_PASSPHRASE_NAME), 'utf8'), @@ -109,7 +101,6 @@ describe('token rejection versus internal authentication fault', () => { }); it('classifies a wrong-subject token as a rejection', async () => { - // A refresh token replayed on the operation-token path. const refreshToken = sign({ username: 'known_user' }, { subject: 'refresh' }); const error = await raisedBy(() => validateOperationToken(refreshToken)); @@ -138,8 +129,6 @@ describe('token rejection versus internal authentication fault', () => { }); it('classifies a deactivated user as a credential-state rejection', async () => { - // The credential itself is unacceptable — the signature is genuine but the account is not - // usable — so this is a rejection, not a fault, even though it arises inside the user store. await setUsersWithRolesCache(new Map([['retired_user', { username: 'retired_user', active: false }]])); try { const token = sign({ username: 'retired_user' }, { subject: 'operation' }); @@ -183,8 +172,6 @@ describe('token rejection versus internal authentication fault', () => { }); it('fails a PEM-shaped but corrupt public key closed', async () => { - // The decisive case: `jsonwebtoken` reports unusable key material through the very same - // `JsonWebTokenError` type it uses for a forged token, so the name alone cannot classify it. const valid = sign({ username: 'known_user' }, { subject: 'operation' }); replacePublicKey('-----BEGIN PUBLIC KEY-----\nbm90LWEtcmVhbC1rZXk=\n-----END PUBLIC KEY-----\n'); @@ -195,8 +182,6 @@ describe('token rejection versus internal authentication fault', () => { }); it('propagates a user-store fault raised while resolving a validly signed token', async () => { - // `findAndValidateUser()` reaches storage through the user cache; a failure there is a - // Harper-side fault even when it arrives with a 4xx status. const failingCache = { get() { const error = new Error('Table system.hdb_user not found'); diff --git a/unitTests/server/fallbackCacheFloor.test.js b/unitTests/server/fallbackCacheFloor.test.js index f4aa467643..78f5bab9dd 100644 --- a/unitTests/server/fallbackCacheFloor.test.js +++ b/unitTests/server/fallbackCacheFloor.test.js @@ -1,17 +1,5 @@ 'use strict'; -/** - * All three adapters hand a request the middleware chain declined (`status: -1`) to legacy Fastify. - * Bun and uWS build their response headers from Fastify's reply; Node hands Fastify the same - * `ServerResponse` the chain's headers were copied onto, so a Fastify route that sets `Cache-Control` - * or `Vary` replaces them outright. The identity floor authentication stamps on a credential-dependent - * response (`Cache-Control: private, no-cache`, `Vary: Authorization, Cookie` — #1565) has to survive - * all three. - * - * These drive the real adapters (`makeUwsHandler`, `bunDelegateToNodeServer`, - * `bridgeChainHeadersToNodeResponse`) against a stub Fastify instance for the first two and a real - * Fastify app over a real `http.Server` for Node, not a re-implementation of their header assembly. - */ const testUtils = require('../testUtils.js'); testUtils.preTestPrep(); @@ -36,12 +24,10 @@ const Fastify = require('fastify'); const UWS_PORT = 19430; const BUN_PORT = 19431; -/** The headers `security/auth.ts` stamps on a response produced under a (deferred) credential. */ function identityFloorHeaders() { return new Headers({ 'Cache-Control': 'private, no-cache', 'Vary': 'Authorization, Cookie' }); } -/** A stub Fastify whose `inject()` answers with the given status/headers/body. */ function fastifyReplying(statusCode, headers, body = 'ok') { return { inject: async () => ({ @@ -72,7 +58,6 @@ function bunWebRequest() { describe('legacy Fastify fallback preserves the chain cache floor', () => { describe('uWS adapter', () => { - /** Registers a chain on UWS_PORT that declines with `chainHeaders`, and returns the handler. */ function handlerDecliningWith(chainHeaders, fastify) { httpServer(() => ({ status: -1, headers: chainHeaders, body: 'Not found' }), { port: UWS_PORT, @@ -93,7 +78,6 @@ describe('legacy Fastify fallback preserves the chain cache floor', () => { assert.strictEqual(response.status, 200); assert.strictEqual(response.headers.get('Cache-Control'), 'private, no-cache'); assert.strictEqual(response.headers.get('Vary'), 'Authorization, Cookie'); - // Fastify's own headers are untouched. assert.strictEqual(response.headers.get('content-type'), 'application/json'); }); @@ -202,10 +186,6 @@ describe('legacy Fastify fallback preserves the chain cache floor', () => { }); describe('Node adapter', () => { - /** - * The production shape of `server/http.ts`'s `status === -1` branch: the chain's headers go onto - * the `ServerResponse`, then legacy Fastify writes the response through that same object. - */ async function requestThroughFastify(chainHeaders, defineRoutes) { const fastify = Fastify(); defineRoutes(fastify); @@ -286,8 +266,6 @@ describe('legacy Fastify fallback preserves the chain cache floor', () => { }); it('reconciles a response written without an explicit writeHead', async () => { - // Node generates headers implicitly through `writeHead` on `end()`, so a fallback that never - // calls it explicitly still has to pass through the same policy. const chainHeaders = identityFloorHeaders(); const server = http.createServer((nodeRequest, nodeResponse) => { bridgeChainHeadersToNodeResponse(chainHeaders, nodeResponse); @@ -312,6 +290,30 @@ describe('legacy Fastify fallback preserves the chain cache floor', () => { } }); + it('preserves repeated Set-Cookie fields passed through writeHead', async () => { + const server = http.createServer((_nodeRequest, nodeResponse) => { + bridgeChainHeadersToNodeResponse(identityFloorHeaders(), nodeResponse); + nodeResponse.writeHead(200, ['Set-Cookie', 'a=1; Path=/', 'Set-Cookie', 'b=2; Path=/']); + nodeResponse.end('{}'); + }); + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + try { + const { port } = server.address(); + const response = await new Promise((resolve, reject) => { + http + .get({ host: '127.0.0.1', port, path: '/' }, (res) => { + res.resume(); + res.on('end', () => resolve(res)); + }) + .on('error', reject); + }); + + assert.deepStrictEqual(response.headers['set-cookie'], ['a=1; Path=/', 'b=2; Path=/']); + } finally { + server.close(); + } + }); + it('leaves a response alone when the chain produced no headers', async () => { const response = await requestThroughFastify(new Headers(), (fastify) => { fastify.get('/wp-json/wc/v3/products', (_request, reply) => diff --git a/unitTests/server/httpChainPortAll.test.js b/unitTests/server/httpChainPortAll.test.js index 0f97d68b97..ea2d9a0c56 100644 --- a/unitTests/server/httpChainPortAll.test.js +++ b/unitTests/server/httpChainPortAll.test.js @@ -34,8 +34,6 @@ describe('http middleware chains and the "all" pseudo-port', () => { assert.deepStrictEqual(orderFor('http', PORT), ['chainSyncAuthentication', 'chainSyncRest']); - // The shape an application catch-all uses: registered on every port, ordered after Harper's - // own route ownership, and arriving after the concrete port's chain already exists (#2418). httpServer(passThrough, { port: 'all', name: 'chainSyncCatchAll', after: 'chainSyncRest' }); assert.deepStrictEqual(orderFor('http', PORT), ['chainSyncAuthentication', 'chainSyncRest', 'chainSyncCatchAll']); @@ -46,8 +44,6 @@ describe('http middleware chains and the "all" pseudo-port', () => { httpServer(passThrough, { port: 'all', name: 'chainSyncSecondCatchAll', after: 'chainSyncOtherPortRest' }); assert.ok(orderFor('http', PORT).includes('chainSyncSecondCatchAll')); - // `chainSyncCatchAll` is on 'all' too, so it belongs to this port's chain as well; its - // `after: 'chainSyncRest'` names nothing registered here, leaving it in registration order. assert.deepStrictEqual(orderFor('http', OTHER_PORT), [ 'chainSyncCatchAll', 'chainSyncOtherPortRest', diff --git a/unitTests/server/mqtt.test.js b/unitTests/server/mqtt.test.js index 3c7c91852b..64ae39eceb 100644 --- a/unitTests/server/mqtt.test.js +++ b/unitTests/server/mqtt.test.js @@ -67,16 +67,10 @@ describe('mqtt.ts handleApplication raw-socket registration', () => { }); }); -// The WebSocket entry points in server/http.ts call `httpChain[port](request)` and hand this -// listener the still-pending completion, so authentication has not resolved the credential yet when -// the listener runs. Reading the deferred-rejection state synchronously therefore always saw -// `undefined`, and an invalid Authorization header connected anonymously wherever MQTT allows -// anonymous connections (#2418). describe('mqtt.ts WebSocket listener settles authentication before the session starts', () => { const { credentialRejectionError, deferCredentialRejection } = require('#src/security/deferredAuthentication'); const { generate } = require('mqtt-packet'); - /** Captures the listener `handleApplication` registers through `server.ws()`. */ function webSocketListener() { let listener; const server = { @@ -109,15 +103,12 @@ describe('mqtt.ts WebSocket listener settles authentication before the session s return { headers: { asObject, get: (name) => asObject[name.toLowerCase()] } }; } - /** Lets every already-queued microtask and the `.catch` continuation run. */ const settle = () => new Promise((resolve) => setImmediate(resolve)); it('closes the socket once an asynchronously-recorded credential rejection settles', async () => { const listener = webSocketListener(); const ws = fakeWebSocket(); const request = mqttUpgradeRequest({ authorization: 'Basic d29yZHByZXNzOnNlY3JldA==' }); - // The real shape: `authentication` yields on the user lookup before it can classify the - // credential, so the rejection is recorded a turn after this listener is invoked. const chainCompletion = (async () => { await Promise.resolve(); deferCredentialRejection(request, credentialRejectionError('Login failed', 401), 'Basic'); @@ -128,7 +119,6 @@ describe('mqtt.ts WebSocket listener settles authentication before the session s throw new Error('a mqtt-subprotocol upgrade must not fall through to the next listener'); }); - // Nothing is knowable yet — that is exactly why a synchronous check could not work. assert.deepStrictEqual(ws.closes, []); await chainCompletion; await settle(); @@ -137,9 +127,6 @@ describe('mqtt.ts WebSocket listener settles authentication before the session s }); it('does not accept the subsequent CONNECT anonymously', async () => { - // The consequence the synchronous check was supposed to prevent: this scope allows anonymous - // connections, so a CONNECT that follows an invalid Authorization header was answered with a - // CONNACK and given an anonymous session. const listener = webSocketListener(); const ws = fakeWebSocket(); const request = mqttUpgradeRequest({ authorization: 'Basic d29yZHByZXNzOnNlY3JldA==' }); @@ -175,8 +162,6 @@ describe('mqtt.ts WebSocket listener settles authentication before the session s listener(ws, request, chainCompletion, () => {}); - // Handlers must be in place before the chain settles, or frames that arrive in that window - // would be dropped. assert.strictEqual(typeof ws.handlers.message, 'function'); assert.strictEqual(typeof ws.handlers.close, 'function'); await chainCompletion; diff --git a/unitTests/server/static.test.js b/unitTests/server/static.test.js index a230e9da1e..7873eb23c5 100644 --- a/unitTests/server/static.test.js +++ b/unitTests/server/static.test.js @@ -373,10 +373,6 @@ describe('static plugin ordering live reload', () => { }); describe('static plugin mount-root redirect', () => { - // A root-level static plugin (no urlPath of its own) has baseURLPath === '/', so gating the - // redirect on baseURLPath alone never fires — even though the application mount makes the - // client-visible root something other than '/'. Review finding: the mount root then serves - // without ever redirecting to its trailing-slash form. it('redirects the application mount root to its trailing-slash form even when static has no urlPath of its own', () => { const { scope, state } = fakeScope({}, { urlPath: '/v1' }); handleApplication(scope); @@ -393,8 +389,6 @@ describe('static plugin mount-root redirect', () => { it('does not redirect when the application has no mount (root stays root)', () => { const { scope, state } = fakeScope(); handleApplication(scope); - // A real, existing path — with no mount, the redirect guard is false and this falls through - // to actually serving the file (realpathSync must succeed). state.entryCallback({ eventType: 'add', urlPath: '/index.html', absolutePath: __filename }); const result = state.listener({ method: 'GET', pathname: '/', url: '/', originalPathname: '/' }, () => ({ @@ -405,8 +399,6 @@ describe('static plugin mount-root redirect', () => { }); }); -// A request shaped enough for both the static handler and the settled-rejection serializer -// (`findBestSerializer` reads `headers.asObject`). function staticRequest(pathname, { url = pathname, originalPathname, authorization } = {}) { const asObject = { accept: 'application/json' }; if (authorization) asObject.authorization = authorization; @@ -427,14 +419,9 @@ function assertSettledUnauthorized(result, request, authorization) { assert.equal(result.status, 401, 'a static-owned response must settle the deferred rejection'); assert.equal(result.headers.get('Content-Type'), 'application/json'); assert.deepStrictEqual(JSON.parse(result.body.toString()), { error: 'Login failed' }); - // The header the deferral exists to protect must survive byte-for-byte. assert.equal(request.headers.get('authorization'), authorization); } -// A static handler ordered `after: 'rest'` runs downstream of authentication, so it is one of the -// Harper-owned layers that must settle a deferred credential rejection (#2418). Settlement covered -// only the ordinary file response, leaving redirects and both `fallthrough: false` not-found forms -// answering a rejected credential as if it were anonymous. describe('static plugin deferred credential rejection', () => { const BASIC = 'Basic d29yZHByZXNzOmFwcC1wYXNzd29yZA=='; const BEARER = 'Bearer downstream-owned-token'; @@ -515,8 +502,6 @@ describe('static plugin deferred credential rejection', () => { }); it('leaves the rejection deferred on the actual fallthrough so a downstream owner still decides', () => { - // The whole point of deferral: Harper does not own this URL, so an application catch-all - // registered after this handler applies its own authentication scheme to the untouched header. const { scope, state } = fakeScope({ after: 'rest' }); handleApplication(scope); @@ -541,8 +526,6 @@ describe('static plugin deferred credential rejection', () => { }); }); -// The settlement points sit immediately before each response is built, so the responses themselves -// must be unchanged for every request that carries no deferred rejection. describe('static plugin responses without a deferred rejection', () => { it('still redirects the mount root, preserving the query string', () => { const { scope, state } = fakeScope({ after: 'rest' }, { urlPath: '/v1' }); From a931a39fc1a46c8c0f395d10e599583f95b3c000 Mon Sep 17 00:00:00 2001 From: hdbjeff Date: Thu, 3 Sep 2026 10:29:51 -0700 Subject: [PATCH 10/12] Preserve fallback session cookies (#2418) --- server/serverHelpers/Headers.ts | 8 +++++++- unitTests/server/fallbackCacheFloor.test.js | 14 ++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/server/serverHelpers/Headers.ts b/server/serverHelpers/Headers.ts index f8a5fac091..9ed2f01d43 100644 --- a/server/serverHelpers/Headers.ts +++ b/server/serverHelpers/Headers.ts @@ -192,7 +192,13 @@ export function mergeChainHeadersIntoFallback< for (const [name, value] of chainHeaders) { const lowerName = String(name).toLowerCase(); if (lowerName === 'vary' || lowerName === 'cache-control') continue; - if (finalHeaders.has(name)) continue; + if (finalHeaders.has(name)) { + if (lowerName === 'set-cookie') { + const values = Array.isArray(value) ? value : [value]; + for (const single of values) appendHeader(finalHeaders, name, single, false); + } + continue; + } if (Array.isArray(value)) { for (const single of value) appendHeader(finalHeaders, name, single, lowerName !== 'set-cookie'); } else finalHeaders.set(name, value); diff --git a/unitTests/server/fallbackCacheFloor.test.js b/unitTests/server/fallbackCacheFloor.test.js index 78f5bab9dd..77ffe91f37 100644 --- a/unitTests/server/fallbackCacheFloor.test.js +++ b/unitTests/server/fallbackCacheFloor.test.js @@ -347,6 +347,20 @@ describe('legacy Fastify fallback preserves the chain cache floor', () => { assert.deepStrictEqual(final.get('Set-Cookie'), ['a=1; Path=/', 'b=2; Path=/']); }); + it('keeps both Fastify and chain Set-Cookie fields', () => { + const chain = new Headers(); + chain.set('Set-Cookie', 'hdb-session=harper; Path=/; HttpOnly'); + const final = new Headers(); + final.set('Set-Cookie', 'app-session=wordpress; Path=/; HttpOnly'); + + mergeChainHeadersIntoFallback(chain, final); + + assert.deepStrictEqual(final.get('Set-Cookie'), [ + 'app-session=wordpress; Path=/; HttpOnly', + 'hdb-session=harper; Path=/; HttpOnly', + ]); + }); + it('does not duplicate a Vary token the final response already declares', () => { const chain = new Headers({ Vary: 'Authorization, Cookie' }); const final = new Headers({ Vary: 'Authorization' }); From 50fa6815347d03abfac61a29f31834d37cc9b1da Mon Sep 17 00:00:00 2001 From: hdbjeff Date: Thu, 3 Sep 2026 10:34:31 -0700 Subject: [PATCH 11/12] Keep fallback cookies distinct across runtimes (#2418) --- server/http.ts | 7 +++++- server/serverHelpers/Headers.ts | 6 ++++- unitTests/server/fallbackCacheFloor.test.js | 28 +++++++++++++++++++++ 3 files changed, 39 insertions(+), 2 deletions(-) diff --git a/server/http.ts b/server/http.ts index 39aca70162..f73807af50 100644 --- a/server/http.ts +++ b/server/http.ts @@ -1470,7 +1470,12 @@ export async function bunDelegateToNodeServer( }); const webHeaders = new globalThis.Headers(); for (const [k, v] of Object.entries(injectResult.headers)) { - if (v != null) webHeaders.set(k, Array.isArray(v) ? v.join(', ') : String(v)); + if (v == null) continue; + if (Array.isArray(v)) { + if (k.toLowerCase() === 'set-cookie') { + for (const single of v) webHeaders.append(k, String(single)); + } else webHeaders.set(k, v.join(', ')); + } else webHeaders.set(k, String(v)); } // Propagate Connection: close so Bun closes the TCP connection after this response, // preventing stale keep-alive sockets from causing silent hangs on subsequent requests. diff --git a/server/serverHelpers/Headers.ts b/server/serverHelpers/Headers.ts index 9ed2f01d43..62d295ec47 100644 --- a/server/serverHelpers/Headers.ts +++ b/server/serverHelpers/Headers.ts @@ -194,8 +194,12 @@ export function mergeChainHeadersIntoFallback< if (lowerName === 'vary' || lowerName === 'cache-control') continue; if (finalHeaders.has(name)) { if (lowerName === 'set-cookie') { + const existing = (finalHeaders as any).getSetCookie?.() ?? finalHeaders.get(name); + const existingValues = new Set((Array.isArray(existing) ? existing : [existing]).map(String)); const values = Array.isArray(value) ? value : [value]; - for (const single of values) appendHeader(finalHeaders, name, single, false); + for (const single of values) { + if (!existingValues.has(String(single))) appendHeader(finalHeaders, name, single, false); + } } continue; } diff --git a/unitTests/server/fallbackCacheFloor.test.js b/unitTests/server/fallbackCacheFloor.test.js index 77ffe91f37..c6cb411746 100644 --- a/unitTests/server/fallbackCacheFloor.test.js +++ b/unitTests/server/fallbackCacheFloor.test.js @@ -183,6 +183,24 @@ describe('legacy Fastify fallback preserves the chain cache floor', () => { assert.strictEqual(response.headers.get('cache-control'), 'max-age=600'); assert.strictEqual(response.headers.get('vary'), null); }); + + it('keeps Fastify Set-Cookie fields separate', async () => { + const nodeServer = { bunFallback: 'multiple Fastify cookies' }; + registerFallbackServer(BUN_PORT + 3, nodeServer); + registerFastifyInstance( + BUN_PORT + 3, + fastifyReplying(200, { + 'set-cookie': ['session=app; Path=/', 'expires=soon; Expires=Wed, 21 Oct 2037 07:28:00 GMT; Path=/'], + }) + ); + + const response = await bunDelegateToNodeServer(nodeServer, bunWebRequest(), { user: undefined }, new Headers()); + + assert.deepStrictEqual(response.headers.getSetCookie(), [ + 'session=app; Path=/', + 'expires=soon; Expires=Wed, 21 Oct 2037 07:28:00 GMT; Path=/', + ]); + }); }); describe('Node adapter', () => { @@ -314,6 +332,16 @@ describe('legacy Fastify fallback preserves the chain cache floor', () => { } }); + it('does not duplicate a chain Set-Cookie field during writeHead reconciliation', async () => { + const chainHeaders = new Headers(); + chainHeaders.set('Set-Cookie', 'hdb-session=harper; Path=/; HttpOnly'); + const response = await requestThroughFastify(chainHeaders, (fastify) => { + fastify.get('/wp-json/wc/v3/products', (_request, reply) => reply.send('{}')); + }); + + assert.deepStrictEqual(response.headers['set-cookie'], ['hdb-session=harper; Path=/; HttpOnly']); + }); + it('leaves a response alone when the chain produced no headers', async () => { const response = await requestThroughFastify(new Headers(), (fastify) => { fastify.get('/wp-json/wc/v3/products', (_request, reply) => From b1df138f5a4f7ccc3e59c39a6d1cdb03684a8f8e Mon Sep 17 00:00:00 2001 From: hdbjeff Date: Thu, 3 Sep 2026 11:41:20 -0700 Subject: [PATCH 12/12] docs: state the Set-Cookie exception to Fastify-wins merge (#2418) Co-Authored-By: Claude Opus 5 (1M context) --- server/DESIGN.md | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/server/DESIGN.md b/server/DESIGN.md index f80d030844..9dc439e3bb 100644 --- a/server/DESIGN.md +++ b/server/DESIGN.md @@ -305,8 +305,16 @@ deferred credential is credential-dependent (#1565), so `authentication` stamps `Cache-Control: private, no-cache` and `Vary: Authorization, Cookie` on it. Before deferral an unrecognized credential could not reach a fallback at all, so this was unreachable. All three adapters now reconcile through one policy, `Headers.ts → mergeChainHeadersIntoFallback()`: Fastify wins every -header it set, `Vary` is unioned, and the private scope is re-applied unless the final response -explicitly opts into shared caching (`public`/`s-maxage`). +single-valued header it set, `Vary` is unioned, and the private scope is re-applied unless the final +response explicitly opts into shared caching (`public`/`s-maxage`). + +`Set-Cookie` is the deliberate exception to Fastify-wins. It is a list-valued field, so the chain's +cookies are appended beside Fastify's and de-duplicated by exact value, never by cookie name. A cookie +is identified by name _plus_ `Domain` _plus_ `Path` (RFC 6265 §5.3), so collapsing by name drops +legitimately distinct cookies — a same-name pair scoped to `/` and `/wp-admin`, or a `Max-Age=0` +deletion paired with a set. Exact value is also what makes Node's `writeHead` re-merge idempotent, +because that path sees the chain's own cookie already on the response. Two cookies that do share a +full identity both reach the client, chain last, and the user agent resolves them last-wins. Bun and uWS rebuild their headers from Fastify's reply and merge once. Node hands Fastify the same `ServerResponse` the chain's headers were copied onto, so copying is not enough — a route calling