From bb9de210b250d048c371e1765bda9be75f7d942b Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Mon, 20 Apr 2026 15:28:59 -0700 Subject: [PATCH 01/15] fix: withOAuthValidation reads request from getContext() (closes #33) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Old code located the request with `args.find((arg) => arg?.session)`, a v4 / legacy pattern. Resource API v2 method signatures are `get(target)` / `receive(target, data)` / etc. — the request is not in the args, it lives on the resource context via `this.getContext()`. The wrapper silently passed through v2 calls without validating. Switch the lookup to `this.getContext()`. No legacy-args fallback — post-v5 migration, v2 is the only shape supported. Tests (`test/lib/withOAuthValidation.test.js`, new) cover: - valid-session passthrough - requireAuth: true + no session → 401 - requireAuth: false + no session → passthrough - custom onValidationError - stale-provider clearing (requireAuth: false) - stale-provider rejection (requireAuth: true) — rejection path that the fix was written to enable - expired token without refresh token (requireAuth: true) — same - non-HTTP methods pass through untouched - no-getContext fallthrough The last two requireAuth:true failure-path tests were added in response to a Claude review finding on the previous combined attempt (#43): the original test file only covered the happy path, leaving the "this is now broken → 401" branch unverified. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lib/withOAuthValidation.ts | 10 +- test/lib/withOAuthValidation.test.js | 273 +++++++++++++++++++++++++++ 2 files changed, 280 insertions(+), 3 deletions(-) create mode 100644 test/lib/withOAuthValidation.test.js diff --git a/src/lib/withOAuthValidation.ts b/src/lib/withOAuthValidation.ts index 297c5f1..b4c15c4 100644 --- a/src/lib/withOAuthValidation.ts +++ b/src/lib/withOAuthValidation.ts @@ -65,11 +65,15 @@ export function withOAuthValidation(resource: any, options: OAuthValidationOptio // Return wrapped method with OAuth validation return async function (this: any, ...args: any[]) { - // Extract request from arguments (usually last or second argument) - const request: Request | undefined = args.find((arg) => arg?.session !== undefined); + // Resource API v2: the request lives on the resource context + // (`this.getContext()`), not in the method arguments. Method + // signatures like `get(target)` / `receive(target, data)` do + // not pass the request directly. + const context = typeof this?.getContext === 'function' ? this.getContext() : undefined; + const request: Request | undefined = context?.session !== undefined ? (context as Request) : undefined; if (!request) { - // No request object found - just pass through + // No request context found - just pass through return originalMethod.apply(this, args); } diff --git a/test/lib/withOAuthValidation.test.js b/test/lib/withOAuthValidation.test.js new file mode 100644 index 0000000..cdc40dc --- /dev/null +++ b/test/lib/withOAuthValidation.test.js @@ -0,0 +1,273 @@ +/** + * Tests for withOAuthValidation + * + * Covers Resource API v2 integration: the wrapper must find the request + * via `this.getContext()` because v2 method signatures (`receive(target, data)`, + * `get(target)`, etc.) do not pass the request as an argument. + */ + +import { describe, it, beforeEach } from 'node:test'; +import assert from 'node:assert/strict'; +import { withOAuthValidation } from '../../dist/lib/withOAuthValidation.js'; + +describe('withOAuthValidation', () => { + let mockProviders; + let mockLogger; + + beforeEach(() => { + mockLogger = { + info: () => {}, + warn: () => {}, + error: () => {}, + debug: () => {}, + }; + + mockProviders = { + github: { + provider: { + refreshAccessToken: async () => ({ + access_token: 'new-access', + expires_in: 3600, + token_type: 'Bearer', + }), + config: { provider: 'github' }, + }, + config: { provider: 'github' }, + }, + }; + }); + + /** + * Build a mock Resource API v2 target: a class instance whose `getContext()` + * returns the request. Method arguments don't contain the request. + */ + function makeV2Resource(methodImpl, context) { + return { + getContext() { + return context; + }, + async get(target, data) { + return methodImpl('get', target, data); + }, + async post(target, data) { + return methodImpl('post', target, data); + }, + }; + } + + function makeSession(overrides = {}) { + return { + id: 'sess-1', + oauth: { + provider: 'github', + accessToken: 'token-abc', + refreshToken: undefined, + ...overrides.oauth, + }, + oauthUser: { username: 'alice', email: 'alice@example.com', role: 'user' }, + update: async () => {}, + ...overrides, + }; + } + + describe('Resource API v2 (request from this.getContext())', () => { + it('passes through to the underlying method when context has a valid OAuth session', async () => { + const context = { session: makeSession() }; + const calls = []; + const resource = makeV2Resource((method, target, data) => { + calls.push({ method, target, data }); + return { status: 200, body: { ok: true } }; + }, context); + + const wrapped = withOAuthValidation(resource, { providers: mockProviders, logger: mockLogger }); + + const result = await wrapped.get({ path: '/x' }, { q: 1 }); + + assert.equal(result.status, 200); + assert.equal(calls.length, 1); + assert.equal(calls[0].method, 'get'); + assert.deepEqual(calls[0].target, { path: '/x' }); + }); + + it('returns 401 when requireAuth is true and no OAuth data is in the session', async () => { + const context = { session: { id: 'sess-no-oauth' } }; // no .oauth field + const calls = []; + const resource = makeV2Resource((method) => { + calls.push({ method }); + return { status: 200 }; + }, context); + + const wrapped = withOAuthValidation(resource, { + providers: mockProviders, + logger: mockLogger, + requireAuth: true, + }); + + const result = await wrapped.get({ path: '/protected' }); + + assert.equal(result.status, 401); + assert.equal(result.body.error, 'Unauthorized'); + assert.equal(calls.length, 0, 'underlying method should not be called'); + }); + + it('passes through unvalidated when requireAuth is false and no OAuth data is in the session', async () => { + const context = { session: { id: 'sess-no-oauth' } }; + const calls = []; + const resource = makeV2Resource((method) => { + calls.push({ method }); + return { status: 200 }; + }, context); + + const wrapped = withOAuthValidation(resource, { + providers: mockProviders, + logger: mockLogger, + requireAuth: false, + }); + + const result = await wrapped.get({ path: '/public' }); + + assert.equal(result.status, 200); + assert.equal(calls.length, 1, 'underlying method should be called'); + }); + + it('invokes onValidationError when provided instead of returning a default 401', async () => { + const context = { session: { id: 'sess-no-oauth' } }; + const resource = makeV2Resource(() => ({ status: 200 }), context); + + const seenErrors = []; + const wrapped = withOAuthValidation(resource, { + providers: mockProviders, + logger: mockLogger, + requireAuth: true, + onValidationError: (request, error) => { + seenErrors.push({ hasRequest: !!request, error }); + return { status: 418, body: { custom: true } }; + }, + }); + + const result = await wrapped.get({ path: '/protected' }); + + assert.equal(result.status, 418); + assert.equal(result.body.custom, true); + assert.equal(seenErrors.length, 1); + assert.equal(seenErrors[0].hasRequest, true); + assert.equal(seenErrors[0].error, 'OAuth authentication required'); + }); + + it('clears stale session data when the provider referenced by session is not configured (requireAuth: false)', async () => { + const context = { + session: makeSession({ oauth: { provider: 'ghost-provider', accessToken: 'stale' } }), + }; + const calls = []; + const resource = makeV2Resource((method) => { + calls.push({ method, sessionOAuth: context.session.oauth }); + return { status: 200 }; + }, context); + + const wrapped = withOAuthValidation(resource, { + providers: mockProviders, // only has 'github' + logger: mockLogger, + requireAuth: false, + }); + + await wrapped.get({ path: '/x' }); + + assert.equal(calls.length, 1); + assert.equal(calls[0].sessionOAuth, undefined, 'stale oauth metadata should be cleared'); + }); + + // The two failure-path cases below are the ones the fix was actually + // written to enable: before the v2-context change they wouldn't even + // reach `validateAndRefreshSession` (the old `args.find` returned no + // request) and the wrapper silently passed through. Now they must + // genuinely reject with 401. + + it('returns 401 when the session references a provider not in the registry (requireAuth: true)', async () => { + const context = { + session: makeSession({ oauth: { provider: 'ghost-provider', accessToken: 'stale' } }), + }; + const calls = []; + const resource = makeV2Resource((method) => { + calls.push({ method }); + return { status: 200 }; + }, context); + + const wrapped = withOAuthValidation(resource, { + providers: mockProviders, // only has 'github' + logger: mockLogger, + requireAuth: true, + }); + + const result = await wrapped.get({ path: '/protected' }); + + assert.equal(result.status, 401, 'should reject rather than silently pass through'); + assert.equal(result.body.error, 'Unauthorized'); + assert.match(result.body.message, /not configured/); + assert.equal(calls.length, 0, 'underlying method must not run'); + assert.equal(context.session.oauth, undefined, 'stale oauth metadata should be cleared'); + }); + + it('returns 401 when the access token is expired and no refresh token is available (requireAuth: true)', async () => { + const context = { + session: makeSession({ + oauth: { + provider: 'github', + accessToken: 'expired-token', + expiresAt: Date.now() - 60_000, // expired one minute ago + refreshToken: undefined, + }, + }), + }; + const calls = []; + const resource = makeV2Resource((method) => { + calls.push({ method }); + return { status: 200 }; + }, context); + + const wrapped = withOAuthValidation(resource, { + providers: mockProviders, + logger: mockLogger, + requireAuth: true, + }); + + const result = await wrapped.get({ path: '/protected' }); + + assert.equal(result.status, 401, 'should reject on expired token with no refresh path'); + assert.equal(result.body.error, 'Unauthorized'); + assert.match(result.body.message, /expired/i); + assert.equal(calls.length, 0, 'underlying method must not run'); + }); + }); + + describe('non-HTTP methods pass through untouched', () => { + it('does not wrap arbitrary methods', async () => { + const context = { session: makeSession() }; + const resource = { + ...makeV2Resource(() => ({ status: 200 }), context), + helper: () => 'helper-result', + }; + const wrapped = withOAuthValidation(resource, { providers: mockProviders, logger: mockLogger }); + + assert.equal(wrapped.helper(), 'helper-result'); + }); + }); + + describe('fallthrough: no context available', () => { + it('passes through when the resource has no getContext', async () => { + // Simulates a method called without v2 context + const calls = []; + const resource = { + async get(target, data) { + calls.push({ target, data }); + return { status: 200 }; + }, + }; + const wrapped = withOAuthValidation(resource, { providers: mockProviders, logger: mockLogger }); + + const result = await wrapped.get({ path: '/noop' }, 'irrelevant'); + + assert.equal(result.status, 200); + assert.equal(calls.length, 1); + }); + }); +}); From 03f54fc88e20ed84d751448d57567601fd5bfcbb Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Mon, 20 Apr 2026 15:44:59 -0700 Subject: [PATCH 02/15] fix: fail-closed when requireAuth is true and no context is available MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the two findings Claude posted on this PR. Pre-existing bug inherited from the old args-based lookup: when the wrapper couldn't locate a request, it passed through to the protected method — regardless of `requireAuth`. Any v2 resource with `requireAuth: true` became silently accessible whenever `getContext()` was missing or returned a context without a session. Now: `requireAuth: true` + no context → 401. `requireAuth: false` + no context → passthrough (unchanged). `onValidationError` is called when provided, matching the existing no-session-data pattern. Test added: the fail-closed branch now has explicit coverage so a regression on this code path surfaces. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lib/withOAuthValidation.ts | 22 +++++++++++++++++++++- test/lib/withOAuthValidation.test.js | 27 ++++++++++++++++++++++++++- 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/src/lib/withOAuthValidation.ts b/src/lib/withOAuthValidation.ts index b4c15c4..ee38388 100644 --- a/src/lib/withOAuthValidation.ts +++ b/src/lib/withOAuthValidation.ts @@ -73,7 +73,27 @@ export function withOAuthValidation(resource: any, options: OAuthValidationOptio const request: Request | undefined = context?.session !== undefined ? (context as Request) : undefined; if (!request) { - // No request context found - just pass through + // Fail-closed when auth is required: if we can't identify + // the request (no v2 context, or context without a session), + // we can't verify OAuth and must reject rather than silently + // invoke the protected method. + if (requireAuth) { + const error = 'No request context available'; + if (onValidationError) { + // onValidationError expects a Request; pass `undefined as any` + // so handlers that ignore it still work, and those that read + // it receive a clear signal. + return onValidationError(undefined as any, error); + } + return { + status: 401, + body: { + error: 'Unauthorized', + message: error, + }, + }; + } + // No auth required — pass through return originalMethod.apply(this, args); } diff --git a/test/lib/withOAuthValidation.test.js b/test/lib/withOAuthValidation.test.js index cdc40dc..d23ded6 100644 --- a/test/lib/withOAuthValidation.test.js +++ b/test/lib/withOAuthValidation.test.js @@ -253,7 +253,7 @@ describe('withOAuthValidation', () => { }); describe('fallthrough: no context available', () => { - it('passes through when the resource has no getContext', async () => { + it('passes through when requireAuth is false and the resource has no getContext', async () => { // Simulates a method called without v2 context const calls = []; const resource = { @@ -269,5 +269,30 @@ describe('withOAuthValidation', () => { assert.equal(result.status, 200); assert.equal(calls.length, 1); }); + + it('returns 401 when requireAuth is true and no context is available (fail-closed)', async () => { + // When getContext() is missing (or yields no session) and auth is + // required, the wrapper must reject. Passing through would silently + // bypass OAuth on any code path that didn't provide a v2 context. + const calls = []; + const resource = { + async get(target) { + calls.push({ target }); + return { status: 200 }; + }, + }; + const wrapped = withOAuthValidation(resource, { + providers: mockProviders, + logger: mockLogger, + requireAuth: true, + }); + + const result = await wrapped.get({ path: '/protected' }); + + assert.equal(result.status, 401, 'must reject rather than silently pass through'); + assert.equal(result.body.error, 'Unauthorized'); + assert.match(result.body.message, /context/i); + assert.equal(calls.length, 0, 'underlying method must not run'); + }); }); }); From 0a5568a3eca4248dfe04abd70233f048b63643ae Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Mon, 20 Apr 2026 15:52:23 -0700 Subject: [PATCH 03/15] fix: stop calling onValidationError without a request; fix stale v4 JSDoc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more findings from the Claude review on this PR. 1. Don't call onValidationError when no request is available. Previous commit called `onValidationError(undefined as any, error)` from the no-context fail-closed path. The callback's published signature is `(request: Request, error: string) => any`; any handler reading `request.session` / `.ip` / `.headers` would TypeError at the exact moment auth is failing, converting a clean 401 into an unhandled exception. Now: the no-context branch always returns a default 401 when `requireAuth` is true. The JSDoc documents that `onValidationError` is not invoked for this specific case, since the callback's request parameter would be undefined. Removing the call also eliminates the previously-untested branch the reviewer flagged; no test is needed. 2. Update the @example block to Resource API v2. The example still showed `async get(target, request)` — the v4 pattern this PR removes. Anyone copying it would hit `request === undefined` on first call. Updated to a v2 class with `static loadAsInstance = false`, `async get(target)`, and `const request = this.getContext()`. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lib/withOAuthValidation.ts | 38 ++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/src/lib/withOAuthValidation.ts b/src/lib/withOAuthValidation.ts index ee38388..7c602ed 100644 --- a/src/lib/withOAuthValidation.ts +++ b/src/lib/withOAuthValidation.ts @@ -25,6 +25,17 @@ export interface OAuthValidationOptions { * This wrapper intercepts all resource method calls (get, post, put, patch, delete) * and validates/refreshes OAuth tokens before passing the request to the original resource. * + * Resource API v2: the wrapped resource's methods receive `(target, data)` + * and read the request from `this.getContext()`. The `@example` below + * reflects that shape; do not reintroduce a `request` method parameter. + * + * `onValidationError` is invoked for OAuth validation failures where a + * request was resolved (no OAuth data, provider not configured, token + * invalid, etc.). It is NOT called when the wrapper can't resolve a + * request at all (no v2 context, or context without a session); that + * path always returns a default 401 when `requireAuth` is true, since + * the callback's `request` parameter would be undefined. + * * @example * ```typescript * // In your application component: @@ -34,15 +45,17 @@ export interface OAuthValidationOptions { * // Get OAuth providers from the OAuth plugin * const oauthPlugin = scope.parent.resources.get('oauth'); * - * // Wrap your protected resource - * const myResource = { - * async get(target, request) { + * // Wrap your protected resource (Resource API v2) + * class MyResource { + * static loadAsInstance = false; + * async get(target) { + * const request = this.getContext(); * // This code only runs if OAuth session is valid * return { user: request.session.oauthUser }; * } - * }; + * } * - * scope.resources.set('protected', withOAuthValidation(myResource, { + * scope.resources.set('protected', withOAuthValidation(MyResource, { * providers: oauthPlugin.providers, * requireAuth: true, * logger: scope.logger @@ -77,19 +90,18 @@ export function withOAuthValidation(resource: any, options: OAuthValidationOptio // the request (no v2 context, or context without a session), // we can't verify OAuth and must reject rather than silently // invoke the protected method. + // + // `onValidationError` is deliberately NOT called here: its + // signature expects a `Request`, and callers are entitled + // to read `request.session` / `.ip` / `.headers`. Passing + // `undefined` would break that contract and turn a clean + // 401 into a runtime `TypeError` inside user code. if (requireAuth) { - const error = 'No request context available'; - if (onValidationError) { - // onValidationError expects a Request; pass `undefined as any` - // so handlers that ignore it still work, and those that read - // it receive a clear signal. - return onValidationError(undefined as any, error); - } return { status: 401, body: { error: 'Unauthorized', - message: error, + message: 'No request context available', }, }; } From 3a4fb458d42c4099baac71fff147a0d0b2d8a08a Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Tue, 21 Apr 2026 05:08:02 -0700 Subject: [PATCH 04/15] fix: JSDoc now shows instance-wrap pattern; test no-context callback skip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more findings from the Claude review on this PR. 1. JSDoc example passed the class constructor, not an instance. The wrapper Proxy's `get` trap reads `target[prop]`, which on a class constructor is undefined for instance methods (those live on `.prototype`). A user copying the example verbatim would get a TypeError or — if Harper instantiated the proxied constructor normally — a plain unwrapped instance with OAuth validation silently skipped. Tests all use plain objects, so this was undetected. Updated the example to wrap `new MyResource()` and added an inline note explaining the instance-vs-constructor distinction so the caveat isn't lost on the next editor. 2. The documented "onValidationError is not called when there's no request" behavior had no test. Added one: supply an onValidationError spy, hit the no-context fail-closed path, assert the spy never runs and the result is the default 401. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lib/withOAuthValidation.ts | 11 +++++++--- test/lib/withOAuthValidation.test.js | 31 ++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/src/lib/withOAuthValidation.ts b/src/lib/withOAuthValidation.ts index 7c602ed..7ef6645 100644 --- a/src/lib/withOAuthValidation.ts +++ b/src/lib/withOAuthValidation.ts @@ -45,9 +45,14 @@ export interface OAuthValidationOptions { * // Get OAuth providers from the OAuth plugin * const oauthPlugin = scope.parent.resources.get('oauth'); * - * // Wrap your protected resource (Resource API v2) + * // Wrap your protected resource (Resource API v2). + * // + * // Note: the wrapper Proxy intercepts property access on the object + * // you pass in, so pass an instance (or a plain object with methods) + * // — NOT a class constructor. Methods on a class constructor live on + * // `.prototype`, which the Proxy's `get` trap can't see; wrapping the + * // class directly would silently bypass OAuth validation. * class MyResource { - * static loadAsInstance = false; * async get(target) { * const request = this.getContext(); * // This code only runs if OAuth session is valid @@ -55,7 +60,7 @@ export interface OAuthValidationOptions { * } * } * - * scope.resources.set('protected', withOAuthValidation(MyResource, { + * scope.resources.set('protected', withOAuthValidation(new MyResource(), { * providers: oauthPlugin.providers, * requireAuth: true, * logger: scope.logger diff --git a/test/lib/withOAuthValidation.test.js b/test/lib/withOAuthValidation.test.js index d23ded6..14e0b2e 100644 --- a/test/lib/withOAuthValidation.test.js +++ b/test/lib/withOAuthValidation.test.js @@ -294,5 +294,36 @@ describe('withOAuthValidation', () => { assert.match(result.body.message, /context/i); assert.equal(calls.length, 0, 'underlying method must not run'); }); + + it('does NOT invoke onValidationError in the no-context path (its signature requires a Request)', async () => { + // The no-context fail-closed branch deliberately skips + // onValidationError: the callback's signature is + // `(request: Request, error) => any` and callers are entitled + // to read `request.session` / `.ip` / `.headers`. Passing + // `undefined` as `request` would break the contract and turn + // a clean 401 into a runtime TypeError inside user code. + const resource = { + async get() { + return { status: 200, passedThrough: true }; + }, + }; + + let handlerCalled = false; + const wrapped = withOAuthValidation(resource, { + providers: mockProviders, + logger: mockLogger, + requireAuth: true, + onValidationError: () => { + handlerCalled = true; + return { status: 418, body: { shouldNotSeeThis: true } }; + }, + }); + + const result = await wrapped.get({ path: '/protected' }); + + assert.equal(handlerCalled, false, 'onValidationError must not be called without a valid request'); + assert.equal(result.status, 401, 'must return the default 401, not the custom handler response'); + assert.equal(result.body.error, 'Unauthorized'); + }); }); }); From baedb5f70cbaf5acfa5c69a69d7b7978b2d3b395 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Tue, 21 Apr 2026 07:27:34 -0700 Subject: [PATCH 05/15] refactor!: withOAuthValidation wraps a Resource class, not an instance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BREAKING for anyone passing `withOAuthValidation(instance, opts)` — the signature is now `withOAuthValidation(Class, opts)` and returns a subclass of Class, not a Proxy around an instance. Why this changed ---------------- The Proxy-based wrapper silently didn't work with Harper v5's resource registration: `resources.set(path, X)` expects `X` to be a Resource class with `static getResource`, which Harper calls to instantiate the resource per request. A Proxy around an instance has no usable static surface; a Proxy around a class doesn't intercept methods that live on `.prototype`. Either way, `withOAuthValidation(...)` silently bypassed all OAuth validation when registered via the natural Harper API. Claude's review on this PR caught the failure-open behavior and the misleading JSDoc examples that led down this path. New design ---------- `withOAuthValidation(Cls, opts)` returns a subclass of `Cls` that overrides the five HTTP verbs (get/post/put/patch/delete). Each override runs OAuth validation first, then delegates to the parent's method (or returns undefined for methods the parent doesn't define). Harper's `static getResource` is inherited, so per-request instantiation + context injection Just Works. Validation helper is extracted to its own function so the semantics are the same across all five verbs. `onValidationError` is still invoked when a concrete request was resolved but semantic validation failed. It is deliberately NOT called on the no-context path (the callback's signature requires a Request and callers read `request.session` / `.ip` / `.headers`). Tests ----- Full rewrite of `test/lib/withOAuthValidation.test.js` to exercise the class-based API with a `MockResource` base: - Wrapped class is an `instanceof` the base (and grand-base). - Static props (e.g. `loadAsInstance`) inherited. - Valid session → underlying method runs. - No OAuth data + requireAuth:true → 401; requireAuth:false → passthrough. - `onValidationError` override path. - Unknown provider + requireAuth:false → clears stale session, passthrough. - Unknown provider + requireAuth:true → 401. - Expired token + no refresh token + requireAuth:true → 401. - Non-HTTP methods pass through untouched. - No session on context + requireAuth:true → 401 (fail-closed). - No session on context + requireAuth:true + `onValidationError` → callback NOT invoked, default 401 returned (contract preserved). - Base class without a given HTTP method → wrapper returns undefined. 438 / 0 / 2 on `bun test`. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lib/withOAuthValidation.ts | 348 +++++++++++++-------------- test/lib/withOAuthValidation.test.js | 339 +++++++++++++++----------- 2 files changed, 362 insertions(+), 325 deletions(-) diff --git a/src/lib/withOAuthValidation.ts b/src/lib/withOAuthValidation.ts index 7ef6645..06ba52c 100644 --- a/src/lib/withOAuthValidation.ts +++ b/src/lib/withOAuthValidation.ts @@ -1,10 +1,16 @@ /** * OAuth Session Validation Wrapper * - * Wraps Harper resources to add automatic OAuth session validation and token refresh - * before handling any request. This enables transparent token management for protected endpoints. + * Wraps a Harper Resource **class** so HTTP methods run OAuth session + * validation (and automatic token refresh) before the wrapped method + * executes. The returned value is a subclass of the input class: Harper + * registers it via `resources.set(...)` and invokes it through the + * standard `static getResource` lifecycle. Per-request instances inherit + * `getContext()` from the user's base `Resource`, so the validation runs + * against the real request context with no extra plumbing. */ +import type { Context, SourceContext } from 'harper'; import type { Request, Logger, ProviderRegistry } from '../types.ts'; import { validateAndRefreshSession } from './sessionValidator.ts'; @@ -19,208 +25,188 @@ export interface OAuthValidationOptions { onValidationError?: (request: Request, error: string) => any; } +type MaybeContext = Context | SourceContext | undefined; + /** - * Wraps a Harper resource to add automatic OAuth session validation - * - * This wrapper intercepts all resource method calls (get, post, put, patch, delete) - * and validates/refreshes OAuth tokens before passing the request to the original resource. + * Run OAuth validation for a request context. * - * Resource API v2: the wrapped resource's methods receive `(target, data)` - * and read the request from `this.getContext()`. The `@example` below - * reflects that shape; do not reintroduce a `request` method parameter. + * Returns `undefined` when the wrapped method should continue. Returns + * a response object (or the result of `onValidationError`) when the + * wrapped method should be short-circuited. * - * `onValidationError` is invoked for OAuth validation failures where a - * request was resolved (no OAuth data, provider not configured, token - * invalid, etc.). It is NOT called when the wrapper can't resolve a - * request at all (no v2 context, or context without a session); that - * path always returns a default 401 when `requireAuth` is true, since - * the callback's `request` parameter would be undefined. + * `onValidationError` is called only on paths where a concrete `request` + * exists and validation failed semantically (no OAuth data, invalid + * provider, expired session, etc.). It is **not** invoked on the + * no-context path, since the callback's signature requires a `Request` + * and callers are entitled to read `request.session` / `.ip` / `.headers`. + */ +async function validateOAuthForRequest(context: MaybeContext, options: OAuthValidationOptions): Promise { + const { providers, logger, requireAuth = false, onValidationError } = options; + const request: Request | undefined = + (context as any)?.session !== undefined ? (context as unknown as Request) : undefined; + + // No v2 context / no session → fail-closed when requireAuth, otherwise passthrough. + // We do NOT call onValidationError here: the callback signature requires + // a Request, and callers read request.session / .ip / .headers. Passing + // `undefined` would turn a clean 401 into a TypeError in user code. + if (!request) { + if (requireAuth) { + return { + status: 401, + body: { + error: 'Unauthorized', + message: 'No request context available', + }, + }; + } + return undefined; + } + + const hasOAuth = request.session?.oauth !== undefined; + if (!hasOAuth) { + if (requireAuth) { + const error = 'OAuth authentication required'; + if (onValidationError) return onValidationError(request, error); + return { status: 401, body: { error: 'Unauthorized', message: error } }; + } + return undefined; + } + + const providerName = request.session?.oauth?.provider; + if (!providerName) { + if (request.session) { + delete request.session.oauth; + delete request.session.oauthUser; + } + if (requireAuth) { + const error = 'Invalid OAuth session data'; + if (onValidationError) return onValidationError(request, error); + return { status: 401, body: { error: 'Unauthorized', message: error } }; + } + return undefined; + } + + const providerData = providers[providerName]; + if (!providerData) { + logger?.warn?.(`OAuth provider '${providerName}' not found for session validation`); + if (request.session) { + delete request.session.oauth; + delete request.session.oauthUser; + } + if (requireAuth) { + const error = `OAuth provider '${providerName}' not configured`; + if (onValidationError) return onValidationError(request, error); + return { status: 401, body: { error: 'Unauthorized', message: error } }; + } + return undefined; + } + + const validation = await validateAndRefreshSession(request, providerData.provider, logger); + if (!validation.valid) { + logger?.info?.(`OAuth session validation failed: ${validation.error}`); + if (requireAuth) { + const error = validation.error || 'OAuth session expired'; + if (onValidationError) return onValidationError(request, error); + return { + status: 401, + body: { + error: 'Unauthorized', + message: 'OAuth session expired. Please log in again.', + details: validation.error, + }, + }; + } + return undefined; + } + + if (validation.refreshed) { + logger?.debug?.(`OAuth token refreshed for ${providerName} session`); + } + + return undefined; +} + +/** + * Wrap a Harper `Resource` class so each HTTP method runs OAuth session + * validation before the user-defined method executes. * * @example * ```typescript * // In your application component: + * import { Resource } from 'harper'; * import { withOAuthValidation } from '@harperfast/oauth'; * * export function handleApplication(scope) { - * // Get OAuth providers from the OAuth plugin * const oauthPlugin = scope.parent.resources.get('oauth'); * - * // Wrap your protected resource (Resource API v2). - * // - * // Note: the wrapper Proxy intercepts property access on the object - * // you pass in, so pass an instance (or a plain object with methods) - * // — NOT a class constructor. Methods on a class constructor live on - * // `.prototype`, which the Proxy's `get` trap can't see; wrapping the - * // class directly would silently bypass OAuth validation. - * class MyResource { + * class MyResource extends Resource { + * static loadAsInstance = false; * async get(target) { * const request = this.getContext(); - * // This code only runs if OAuth session is valid * return { user: request.session.oauthUser }; * } * } * - * scope.resources.set('protected', withOAuthValidation(new MyResource(), { + * const Protected = withOAuthValidation(MyResource, { * providers: oauthPlugin.providers, * requireAuth: true, - * logger: scope.logger - * })); + * logger: scope.logger, + * }); + * + * // Register the wrapped class — Harper handles instantiation per request + * scope.resources.set('protected', Protected); * } * ``` + * + * Notes: + * - The wrapper returns a **subclass** of `ResourceClass`. All static + * properties (including `loadAsInstance`) and static methods + * (including `getResource`) are inherited, so Harper's registration + * and dispatch lifecycle works unchanged. + * - The five standard HTTP methods — `get`, `post`, `put`, `patch`, + * `delete` — are overridden to run validation first. Other methods + * (subscriptions, helpers, etc.) pass through untouched. + * - If the parent class does not define an HTTP method, the wrapper + * still runs validation, then returns `undefined` — matching + * Harper's "method not implemented" behavior. This means validation + * runs even on unhandled verbs (defense-in-depth). */ -export function withOAuthValidation(resource: any, options: OAuthValidationOptions): any { - const { providers, logger, requireAuth = false, onValidationError } = options; - - // Create a proxy that wraps all resource methods - return new Proxy(resource, { - get(target, prop: string) { - const originalMethod = target[prop]; - - // Only wrap HTTP methods - if (!['get', 'post', 'put', 'patch', 'delete'].includes(prop)) { - return originalMethod; - } - - // Return wrapped method with OAuth validation - return async function (this: any, ...args: any[]) { - // Resource API v2: the request lives on the resource context - // (`this.getContext()`), not in the method arguments. Method - // signatures like `get(target)` / `receive(target, data)` do - // not pass the request directly. - const context = typeof this?.getContext === 'function' ? this.getContext() : undefined; - const request: Request | undefined = context?.session !== undefined ? (context as Request) : undefined; - - if (!request) { - // Fail-closed when auth is required: if we can't identify - // the request (no v2 context, or context without a session), - // we can't verify OAuth and must reject rather than silently - // invoke the protected method. - // - // `onValidationError` is deliberately NOT called here: its - // signature expects a `Request`, and callers are entitled - // to read `request.session` / `.ip` / `.headers`. Passing - // `undefined` would break that contract and turn a clean - // 401 into a runtime `TypeError` inside user code. - if (requireAuth) { - return { - status: 401, - body: { - error: 'Unauthorized', - message: 'No request context available', - }, - }; - } - // No auth required — pass through - return originalMethod.apply(this, args); - } - - // Check if session has OAuth data - const hasOAuth = request.session?.oauth !== undefined; - - if (!hasOAuth) { - if (requireAuth) { - // OAuth authentication required but not present - const error = 'OAuth authentication required'; - if (onValidationError) { - return onValidationError(request, error); - } - return { - status: 401, - body: { - error: 'Unauthorized', - message: error, - }, - }; - } - // OAuth not required, pass through - return originalMethod.apply(this, args); - } - - // Get provider for this OAuth session - const providerName = request.session?.oauth?.provider; - if (!providerName) { - // No provider name in session - invalid OAuth data - if (request.session) { - delete request.session.oauth; - delete request.session.oauthUser; - } - if (requireAuth) { - const error = 'Invalid OAuth session data'; - if (onValidationError) { - return onValidationError(request, error); - } - return { - status: 401, - body: { - error: 'Unauthorized', - message: error, - }, - }; - } - return originalMethod.apply(this, args); - } - - const providerData = providers[providerName]; - - if (!providerData) { - logger?.warn?.(`OAuth provider '${providerName}' not found for session validation`); - // Provider not found - clear OAuth data and continue - if (request.session) { - delete request.session.oauth; - delete request.session.oauthUser; - } - if (requireAuth) { - const error = `OAuth provider '${providerName}' not configured`; - if (onValidationError) { - return onValidationError(request, error); - } - return { - status: 401, - body: { - error: 'Unauthorized', - message: error, - }, - }; - } - return originalMethod.apply(this, args); - } - - // Validate and refresh session - const validation = await validateAndRefreshSession(request, providerData.provider, logger); - - if (!validation.valid) { - // Session validation failed - logger?.info?.(`OAuth session validation failed: ${validation.error}`); - - if (requireAuth) { - const error = validation.error || 'OAuth session expired'; - if (onValidationError) { - return onValidationError(request, error); - } - return { - status: 401, - body: { - error: 'Unauthorized', - message: 'OAuth session expired. Please log in again.', - details: validation.error, - }, - }; - } - - // Not requiring auth, but validation failed - continue without OAuth - return originalMethod.apply(this, args); - } - - // Session is valid (and possibly refreshed) - if (validation.refreshed) { - logger?.debug?.(`OAuth token refreshed for ${providerName} session`); - } - - // Call original method with validated/refreshed session - return originalMethod.apply(this, args); - }; - }, - }); +export function withOAuthValidation any>( + ResourceClass: T, + options: OAuthValidationOptions +): T { + // Capture the parent prototype so we can look up HTTP methods directly + // without using `super` (TypeScript doesn't allow optional chaining on + // `super` member access, and the `delete` keyword as a method name via + // `super` trips up some compilation targets). Prototype lookup walks + // the chain, so inherited methods are found too. + const parentProto = (ResourceClass as any).prototype; + + const delegate = async (instance: any, method: string, args: any[]) => { + const deny = await validateOAuthForRequest(instance.getContext?.(), options); + if (deny !== undefined) return deny; + const parentMethod = parentProto?.[method]; + return typeof parentMethod === 'function' ? parentMethod.apply(instance, args) : undefined; + }; + + const Wrapped = class extends (ResourceClass as any) { + async get(...args: any[]) { + return delegate(this, 'get', args); + } + async post(...args: any[]) { + return delegate(this, 'post', args); + } + async put(...args: any[]) { + return delegate(this, 'put', args); + } + async patch(...args: any[]) { + return delegate(this, 'patch', args); + } + async delete(...args: any[]) { + return delegate(this, 'delete', args); + } + }; + return Wrapped as unknown as T; } /** diff --git a/test/lib/withOAuthValidation.test.js b/test/lib/withOAuthValidation.test.js index 14e0b2e..77769fc 100644 --- a/test/lib/withOAuthValidation.test.js +++ b/test/lib/withOAuthValidation.test.js @@ -1,9 +1,10 @@ /** * Tests for withOAuthValidation * - * Covers Resource API v2 integration: the wrapper must find the request - * via `this.getContext()` because v2 method signatures (`receive(target, data)`, - * `get(target)`, etc.) do not pass the request as an argument. + * Exercises the subclass-based wrapper: `withOAuthValidation(ResourceClass, opts)` + * returns a subclass of `ResourceClass`. Harper registers the subclass via + * `resources.set(...)` and instantiates it per request; the subclass's + * HTTP-method overrides run OAuth validation before delegating to `super`. */ import { describe, it, beforeEach } from 'node:test'; @@ -38,21 +39,21 @@ describe('withOAuthValidation', () => { }); /** - * Build a mock Resource API v2 target: a class instance whose `getContext()` - * returns the request. Method arguments don't contain the request. + * Minimal Resource-like base class for tests. Accepts `(id, context)` + * in the constructor and exposes `getContext()` — matching the real + * Harper Resource base class contract closely enough for wrapper tests. */ - function makeV2Resource(methodImpl, context) { - return { - getContext() { - return context; - }, - async get(target, data) { - return methodImpl('get', target, data); - }, - async post(target, data) { - return methodImpl('post', target, data); - }, - }; + class MockResource { + static loadAsInstance = false; + + constructor(id, context) { + this._id = id; + this._context = context ?? null; + } + + getContext() { + return this._context; + } } function makeSession(overrides = {}) { @@ -70,246 +71,275 @@ describe('withOAuthValidation', () => { }; } - describe('Resource API v2 (request from this.getContext())', () => { - it('passes through to the underlying method when context has a valid OAuth session', async () => { - const context = { session: makeSession() }; - const calls = []; - const resource = makeV2Resource((method, target, data) => { - calls.push({ method, target, data }); - return { status: 200, body: { ok: true } }; - }, context); + describe('Wrapped-class registration surface', () => { + it('returns a subclass of the input class (instanceof preserved)', () => { + class MyResource extends MockResource { + async get() { + return { status: 200 }; + } + } + const Wrapped = withOAuthValidation(MyResource, { providers: mockProviders, logger: mockLogger }); - const wrapped = withOAuthValidation(resource, { providers: mockProviders, logger: mockLogger }); + const instance = new Wrapped('x', { session: makeSession() }); + assert.ok(instance instanceof MyResource, 'wrapped instance must be an instance of the base class'); + assert.ok(instance instanceof MockResource, 'wrapped instance must be an instance of the grand-parent class'); + }); + + it('inherits static properties from the base class (e.g., loadAsInstance)', () => { + class MyResource extends MockResource { + static loadAsInstance = false; + } + const Wrapped = withOAuthValidation(MyResource, { providers: mockProviders, logger: mockLogger }); + + assert.equal(Wrapped.loadAsInstance, false); + }); + }); - const result = await wrapped.get({ path: '/x' }, { q: 1 }); + describe('HTTP methods run validation before delegating', () => { + it('passes through to the underlying method when OAuth session is valid', async () => { + const calls = []; + class MyResource extends MockResource { + async get(target) { + calls.push({ target }); + return { status: 200, body: { ok: true } }; + } + } + const Wrapped = withOAuthValidation(MyResource, { providers: mockProviders, logger: mockLogger }); + + const instance = new Wrapped('x', { session: makeSession() }); + const result = await instance.get({ path: '/x' }); assert.equal(result.status, 200); + assert.deepEqual(result.body, { ok: true }); assert.equal(calls.length, 1); - assert.equal(calls[0].method, 'get'); - assert.deepEqual(calls[0].target, { path: '/x' }); }); - it('returns 401 when requireAuth is true and no OAuth data is in the session', async () => { - const context = { session: { id: 'sess-no-oauth' } }; // no .oauth field + it('returns 401 when requireAuth is true and session has no OAuth data', async () => { const calls = []; - const resource = makeV2Resource((method) => { - calls.push({ method }); - return { status: 200 }; - }, context); - - const wrapped = withOAuthValidation(resource, { + class MyResource extends MockResource { + async get() { + calls.push('called'); + return { status: 200 }; + } + } + const Wrapped = withOAuthValidation(MyResource, { providers: mockProviders, logger: mockLogger, requireAuth: true, }); - const result = await wrapped.get({ path: '/protected' }); + const instance = new Wrapped('x', { session: { id: 'no-oauth' } }); + const result = await instance.get({ path: '/protected' }); assert.equal(result.status, 401); assert.equal(result.body.error, 'Unauthorized'); - assert.equal(calls.length, 0, 'underlying method should not be called'); + assert.equal(calls.length, 0, 'underlying method must not run'); }); - it('passes through unvalidated when requireAuth is false and no OAuth data is in the session', async () => { - const context = { session: { id: 'sess-no-oauth' } }; + it('passes through when requireAuth is false and session has no OAuth data', async () => { const calls = []; - const resource = makeV2Resource((method) => { - calls.push({ method }); - return { status: 200 }; - }, context); - - const wrapped = withOAuthValidation(resource, { + class MyResource extends MockResource { + async get() { + calls.push('called'); + return { status: 200 }; + } + } + const Wrapped = withOAuthValidation(MyResource, { providers: mockProviders, logger: mockLogger, requireAuth: false, }); - const result = await wrapped.get({ path: '/public' }); + const instance = new Wrapped('x', { session: { id: 'no-oauth' } }); + const result = await instance.get({ path: '/public' }); assert.equal(result.status, 200); - assert.equal(calls.length, 1, 'underlying method should be called'); + assert.equal(calls.length, 1); }); - it('invokes onValidationError when provided instead of returning a default 401', async () => { - const context = { session: { id: 'sess-no-oauth' } }; - const resource = makeV2Resource(() => ({ status: 200 }), context); + it('invokes onValidationError when provided and validation fails', async () => { + class MyResource extends MockResource { + async get() { + return { status: 200 }; + } + } - const seenErrors = []; - const wrapped = withOAuthValidation(resource, { + const seen = []; + const Wrapped = withOAuthValidation(MyResource, { providers: mockProviders, logger: mockLogger, requireAuth: true, onValidationError: (request, error) => { - seenErrors.push({ hasRequest: !!request, error }); + seen.push({ hasRequest: !!request, error }); return { status: 418, body: { custom: true } }; }, }); - const result = await wrapped.get({ path: '/protected' }); + const instance = new Wrapped('x', { session: { id: 'no-oauth' } }); + const result = await instance.get({ path: '/protected' }); assert.equal(result.status, 418); assert.equal(result.body.custom, true); - assert.equal(seenErrors.length, 1); - assert.equal(seenErrors[0].hasRequest, true); - assert.equal(seenErrors[0].error, 'OAuth authentication required'); + assert.equal(seen.length, 1); + assert.equal(seen[0].hasRequest, true); + assert.equal(seen[0].error, 'OAuth authentication required'); }); - it('clears stale session data when the provider referenced by session is not configured (requireAuth: false)', async () => { + it('clears stale session data when the provider is not in the registry (requireAuth: false)', async () => { + const calls = []; + class MyResource extends MockResource { + async get() { + calls.push({ sessionOAuth: this._context.session.oauth }); + return { status: 200 }; + } + } + const context = { session: makeSession({ oauth: { provider: 'ghost-provider', accessToken: 'stale' } }), }; - const calls = []; - const resource = makeV2Resource((method) => { - calls.push({ method, sessionOAuth: context.session.oauth }); - return { status: 200 }; - }, context); - - const wrapped = withOAuthValidation(resource, { + const Wrapped = withOAuthValidation(MyResource, { providers: mockProviders, // only has 'github' logger: mockLogger, requireAuth: false, }); - await wrapped.get({ path: '/x' }); + const instance = new Wrapped('x', context); + await instance.get({ path: '/x' }); assert.equal(calls.length, 1); assert.equal(calls[0].sessionOAuth, undefined, 'stale oauth metadata should be cleared'); }); - // The two failure-path cases below are the ones the fix was actually - // written to enable: before the v2-context change they wouldn't even - // reach `validateAndRefreshSession` (the old `args.find` returned no - // request) and the wrapper silently passed through. Now they must - // genuinely reject with 401. - - it('returns 401 when the session references a provider not in the registry (requireAuth: true)', async () => { + it('returns 401 when the session references an unknown provider (requireAuth: true)', async () => { + const calls = []; + class MyResource extends MockResource { + async get() { + calls.push('called'); + return { status: 200 }; + } + } const context = { session: makeSession({ oauth: { provider: 'ghost-provider', accessToken: 'stale' } }), }; - const calls = []; - const resource = makeV2Resource((method) => { - calls.push({ method }); - return { status: 200 }; - }, context); - - const wrapped = withOAuthValidation(resource, { - providers: mockProviders, // only has 'github' + const Wrapped = withOAuthValidation(MyResource, { + providers: mockProviders, logger: mockLogger, requireAuth: true, }); - const result = await wrapped.get({ path: '/protected' }); + const instance = new Wrapped('x', context); + const result = await instance.get({ path: '/protected' }); - assert.equal(result.status, 401, 'should reject rather than silently pass through'); - assert.equal(result.body.error, 'Unauthorized'); + assert.equal(result.status, 401); assert.match(result.body.message, /not configured/); - assert.equal(calls.length, 0, 'underlying method must not run'); + assert.equal(calls.length, 0); assert.equal(context.session.oauth, undefined, 'stale oauth metadata should be cleared'); }); it('returns 401 when the access token is expired and no refresh token is available (requireAuth: true)', async () => { + const calls = []; + class MyResource extends MockResource { + async get() { + calls.push('called'); + return { status: 200 }; + } + } const context = { session: makeSession({ oauth: { provider: 'github', accessToken: 'expired-token', - expiresAt: Date.now() - 60_000, // expired one minute ago + expiresAt: Date.now() - 60_000, refreshToken: undefined, }, }), }; - const calls = []; - const resource = makeV2Resource((method) => { - calls.push({ method }); - return { status: 200 }; - }, context); - - const wrapped = withOAuthValidation(resource, { + const Wrapped = withOAuthValidation(MyResource, { providers: mockProviders, logger: mockLogger, requireAuth: true, }); - const result = await wrapped.get({ path: '/protected' }); + const instance = new Wrapped('x', context); + const result = await instance.get({ path: '/protected' }); - assert.equal(result.status, 401, 'should reject on expired token with no refresh path'); - assert.equal(result.body.error, 'Unauthorized'); + assert.equal(result.status, 401); assert.match(result.body.message, /expired/i); - assert.equal(calls.length, 0, 'underlying method must not run'); + assert.equal(calls.length, 0); }); }); - describe('non-HTTP methods pass through untouched', () => { + describe('Non-HTTP methods pass through untouched', () => { it('does not wrap arbitrary methods', async () => { - const context = { session: makeSession() }; - const resource = { - ...makeV2Resource(() => ({ status: 200 }), context), - helper: () => 'helper-result', - }; - const wrapped = withOAuthValidation(resource, { providers: mockProviders, logger: mockLogger }); - - assert.equal(wrapped.helper(), 'helper-result'); + class MyResource extends MockResource { + async get() { + return { status: 200 }; + } + helper() { + return 'helper-result'; + } + } + const Wrapped = withOAuthValidation(MyResource, { providers: mockProviders, logger: mockLogger }); + + const instance = new Wrapped('x', { session: makeSession() }); + assert.equal(instance.helper(), 'helper-result'); }); }); - describe('fallthrough: no context available', () => { - it('passes through when requireAuth is false and the resource has no getContext', async () => { - // Simulates a method called without v2 context + describe('No context available', () => { + it('passes through when requireAuth is false and there is no session on the context', async () => { const calls = []; - const resource = { - async get(target, data) { - calls.push({ target, data }); + class MyResource extends MockResource { + async get() { + calls.push('called'); return { status: 200 }; - }, - }; - const wrapped = withOAuthValidation(resource, { providers: mockProviders, logger: mockLogger }); + } + } + const Wrapped = withOAuthValidation(MyResource, { providers: mockProviders, logger: mockLogger }); - const result = await wrapped.get({ path: '/noop' }, 'irrelevant'); + const instance = new Wrapped('x', {}); // context without session + const result = await instance.get({ path: '/noop' }); assert.equal(result.status, 200); assert.equal(calls.length, 1); }); - it('returns 401 when requireAuth is true and no context is available (fail-closed)', async () => { - // When getContext() is missing (or yields no session) and auth is - // required, the wrapper must reject. Passing through would silently - // bypass OAuth on any code path that didn't provide a v2 context. + it('returns 401 when requireAuth is true and there is no session on the context (fail-closed)', async () => { const calls = []; - const resource = { - async get(target) { - calls.push({ target }); + class MyResource extends MockResource { + async get() { + calls.push('called'); return { status: 200 }; - }, - }; - const wrapped = withOAuthValidation(resource, { + } + } + const Wrapped = withOAuthValidation(MyResource, { providers: mockProviders, logger: mockLogger, requireAuth: true, }); - const result = await wrapped.get({ path: '/protected' }); + const instance = new Wrapped('x', {}); // context without session + const result = await instance.get({ path: '/protected' }); - assert.equal(result.status, 401, 'must reject rather than silently pass through'); - assert.equal(result.body.error, 'Unauthorized'); + assert.equal(result.status, 401); assert.match(result.body.message, /context/i); - assert.equal(calls.length, 0, 'underlying method must not run'); + assert.equal(calls.length, 0); }); - it('does NOT invoke onValidationError in the no-context path (its signature requires a Request)', async () => { + it('does NOT invoke onValidationError in the no-context path', async () => { // The no-context fail-closed branch deliberately skips - // onValidationError: the callback's signature is - // `(request: Request, error) => any` and callers are entitled - // to read `request.session` / `.ip` / `.headers`. Passing - // `undefined` as `request` would break the contract and turn - // a clean 401 into a runtime TypeError inside user code. - const resource = { + // onValidationError: the callback signature is + // `(request: Request, error) => any` and callers read + // `request.session` / `.ip` / `.headers`. Passing `undefined` + // would turn a clean 401 into a TypeError in user code. + class MyResource extends MockResource { async get() { return { status: 200, passedThrough: true }; - }, - }; + } + } let handlerCalled = false; - const wrapped = withOAuthValidation(resource, { + const Wrapped = withOAuthValidation(MyResource, { providers: mockProviders, logger: mockLogger, requireAuth: true, @@ -319,11 +349,32 @@ describe('withOAuthValidation', () => { }, }); - const result = await wrapped.get({ path: '/protected' }); + const instance = new Wrapped('x', {}); // no session + const result = await instance.get({ path: '/protected' }); assert.equal(handlerCalled, false, 'onValidationError must not be called without a valid request'); - assert.equal(result.status, 401, 'must return the default 401, not the custom handler response'); + assert.equal(result.status, 401); assert.equal(result.body.error, 'Unauthorized'); }); }); + + describe('Undefined super methods', () => { + it('returns undefined if the base class does not define the HTTP method', async () => { + // Matches Harper's own "method not implemented" behavior: + // `resource.?.(…)` short-circuits to undefined, and + // Harper renders that as 404 / method-not-allowed. The + // wrapper still runs validation first so unreachable verbs + // are defense-in-depth protected. + class GetOnly extends MockResource { + async get() { + return { status: 200 }; + } + } + const Wrapped = withOAuthValidation(GetOnly, { providers: mockProviders, logger: mockLogger }); + + const instance = new Wrapped('x', { session: makeSession() }); + const result = await instance.post({ path: '/x' }, { data: 1 }); + assert.equal(result, undefined); + }); + }); }); From 7fe7d81c6f182fcc81481d494ef106dbe69ed75e Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Tue, 21 Apr 2026 09:57:02 -0700 Subject: [PATCH 06/15] fix: onValidationError sees un-mutated request; cover put/patch/delete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more findings from Claude's review on this PR. 1. onValidationError received a pre-cleared session in two branches. The "unknown provider" and "no provider name" branches both deleted `request.session.oauth` / `.oauthUser` BEFORE invoking the caller's `onValidationError` handler. An integrator logging "which provider caused this failure" in a custom handler would read `undefined` for both fields. The existing onValidationError test only exercised the "no OAuth data" branch — which has nothing to clear — so this mutation-before-notification ordering was never observed. Fix: in both clearing branches, call `onValidationError` (awaited) first, THEN clear the stale session fields. The handler sees the full state at time of failure; the session still ends up cleared for the next request. 2. put, patch, delete had zero test coverage. The verb string passed to `delegate(this, '', args)` is the only link between the override and the prototype lookup. A typo would silently bypass OAuth validation on that verb. Only `get` and (via one test) `post` were exercised. Added a loop-driven suite covering all five verbs both ways — 401 on requireAuth:true with no session, and delegation to the parent method on a valid session. Also added two callback- ordering tests for the two deferred-delete branches above. Bun tests: 450 pass, 0 fail, 2 skip. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lib/withOAuthValidation.ts | 30 ++++-- test/lib/withOAuthValidation.test.js | 132 +++++++++++++++++++++++++++ 2 files changed, 152 insertions(+), 10 deletions(-) diff --git a/src/lib/withOAuthValidation.ts b/src/lib/withOAuthValidation.ts index 06ba52c..fefa6ff 100644 --- a/src/lib/withOAuthValidation.ts +++ b/src/lib/withOAuthValidation.ts @@ -72,32 +72,42 @@ async function validateOAuthForRequest(context: MaybeContext, options: OAuthVali return undefined; } - const providerName = request.session?.oauth?.provider; - if (!providerName) { + const clearStaleOAuth = () => { if (request.session) { delete request.session.oauth; delete request.session.oauthUser; } + }; + + const providerName = request.session?.oauth?.provider; + if (!providerName) { if (requireAuth) { const error = 'Invalid OAuth session data'; - if (onValidationError) return onValidationError(request, error); - return { status: 401, body: { error: 'Unauthorized', message: error } }; + // Invoke the callback BEFORE clearing so it can read + // request.session.oauth / .oauthUser (audit logging, etc.). + const response = onValidationError + ? await onValidationError(request, error) + : { status: 401, body: { error: 'Unauthorized', message: error } }; + clearStaleOAuth(); + return response; } + clearStaleOAuth(); return undefined; } const providerData = providers[providerName]; if (!providerData) { logger?.warn?.(`OAuth provider '${providerName}' not found for session validation`); - if (request.session) { - delete request.session.oauth; - delete request.session.oauthUser; - } if (requireAuth) { const error = `OAuth provider '${providerName}' not configured`; - if (onValidationError) return onValidationError(request, error); - return { status: 401, body: { error: 'Unauthorized', message: error } }; + // Same ordering as above: callback sees un-mutated request. + const response = onValidationError + ? await onValidationError(request, error) + : { status: 401, body: { error: 'Unauthorized', message: error } }; + clearStaleOAuth(); + return response; } + clearStaleOAuth(); return undefined; } diff --git a/test/lib/withOAuthValidation.test.js b/test/lib/withOAuthValidation.test.js index 77769fc..f13b1d6 100644 --- a/test/lib/withOAuthValidation.test.js +++ b/test/lib/withOAuthValidation.test.js @@ -358,6 +358,138 @@ describe('withOAuthValidation', () => { }); }); + describe('All five HTTP verbs route through the delegate', () => { + // Each override in the wrapper calls `delegate(this, '', args)`. + // The verb string is the only connection between the override and the + // prototype lookup — a typo would silently bypass OAuth validation on + // that verb. Exercise all five both for enforcement and delegation. + for (const method of ['get', 'post', 'put', 'patch', 'delete']) { + it(`${method}: returns 401 with requireAuth and no OAuth data`, async () => { + const calls = []; + class MyResource extends MockResource { + async [method]() { + calls.push(method); + return { status: 200 }; + } + } + const Wrapped = withOAuthValidation(MyResource, { + providers: mockProviders, + logger: mockLogger, + requireAuth: true, + }); + + const instance = new Wrapped('x', { session: { id: 'no-oauth' } }); + const result = await instance[method]({ path: '/protected' }); + + assert.equal(result.status, 401, `${method} must enforce OAuth`); + assert.equal(calls.length, 0, `${method} must not run underlying method`); + }); + + it(`${method}: delegates to parent on valid session`, async () => { + const calls = []; + class MyResource extends MockResource { + async [method](target) { + calls.push({ method, target }); + return { status: 200, verb: method }; + } + } + const Wrapped = withOAuthValidation(MyResource, { providers: mockProviders, logger: mockLogger }); + + const instance = new Wrapped('x', { session: makeSession() }); + const result = await instance[method]({ path: '/x' }); + + assert.equal(result.status, 200); + assert.equal(result.verb, method, `${method} override must call the parent's ${method}`); + assert.equal(calls.length, 1); + }); + } + }); + + describe('onValidationError receives un-mutated request in clearing paths', () => { + // The "unknown provider" and "no provider name" branches clear stale + // session data. The callback must see the request BEFORE the clear, + // or audit/logging handlers reading request.session.oauth / oauthUser + // silently get `undefined`. + it('unknown-provider branch: callback sees full session, session is cleared after', async () => { + class MyResource extends MockResource { + async get() { + return { status: 200 }; + } + } + + const seen = []; + const context = { + session: makeSession({ + oauth: { provider: 'ghost-provider', accessToken: 'stale' }, + }), + }; + + const Wrapped = withOAuthValidation(MyResource, { + providers: mockProviders, // only has 'github' + logger: mockLogger, + requireAuth: true, + onValidationError: (request, error) => { + seen.push({ + oauthAtCall: request.session.oauth && { ...request.session.oauth }, + oauthUserAtCall: request.session.oauthUser && { ...request.session.oauthUser }, + error, + }); + return { status: 401, body: { custom: true } }; + }, + }); + + const instance = new Wrapped('x', context); + const result = await instance.get({ path: '/protected' }); + + assert.equal(result.status, 401); + assert.equal(seen.length, 1); + assert.ok(seen[0].oauthAtCall, 'callback must see oauth metadata BEFORE it is cleared'); + assert.equal(seen[0].oauthAtCall.provider, 'ghost-provider'); + assert.ok(seen[0].oauthUserAtCall, 'callback must see oauthUser metadata BEFORE it is cleared'); + // Session IS cleared after the callback returns + assert.equal(context.session.oauth, undefined); + assert.equal(context.session.oauthUser, undefined); + }); + + it('no-provider-name branch: callback sees full session, session is cleared after', async () => { + class MyResource extends MockResource { + async get() { + return { status: 200 }; + } + } + + const seen = []; + const context = { + session: makeSession({ + // OAuth data exists but has no `provider` field — invalid state + oauth: { accessToken: 'orphan', someOtherField: 'x' }, + }), + }; + + const Wrapped = withOAuthValidation(MyResource, { + providers: mockProviders, + logger: mockLogger, + requireAuth: true, + onValidationError: (request, error) => { + seen.push({ + oauthAtCall: request.session.oauth && { ...request.session.oauth }, + oauthUserAtCall: request.session.oauthUser && { ...request.session.oauthUser }, + error, + }); + return { status: 401, body: { custom: true } }; + }, + }); + + const instance = new Wrapped('x', context); + await instance.get({ path: '/protected' }); + + assert.equal(seen.length, 1); + assert.ok(seen[0].oauthAtCall, 'callback must see oauth before clearing'); + assert.equal(seen[0].oauthAtCall.accessToken, 'orphan'); + assert.equal(context.session.oauth, undefined, 'session cleared after callback'); + }); + }); + describe('Undefined super methods', () => { it('returns undefined if the base class does not define the HTTP method', async () => { // Matches Harper's own "method not implemented" behavior: From a81fb4098fc773ed134ca53055a970c8c9efb53d Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Tue, 21 Apr 2026 10:11:10 -0700 Subject: [PATCH 07/15] fix: re-export withOAuthValidation from package entry; explicit prop capture in tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from Claude's round-5 review on this PR. 1. `withOAuthValidation` was never re-exported from `src/index.ts`. The JSDoc example says `import { withOAuthValidation } from '@harperfast/oauth'`, but the package entry only exported HookManager / OAuthResource / TenantManager / provider utilities — the documented symbol was missing. Tests import directly from `dist/lib/withOAuthValidation.js`, which is why CI stayed green while the shipped public API was broken. Integrators following the JSDoc would get a module-resolution error. Added re-exports for `withOAuthValidation`, `getOAuthProviders`, and the `OAuthValidationOptions` type. 2. Callback tests captured `request.session.oauth` via spread. The "callback sees un-mutated request" tests used `{ ...request.session.oauth }` / `{ ...request.session.oauthUser }` to snapshot the fields. That's fine for the plain-object mocks used in tests, but in production these are Harper `GenericTrackedObject`s where spread copies nothing (per CLAUDE.md Non-Obvious Gotchas). If an integrator copied the callback pattern from this test for their own audit logger, they would silently get empty objects in production. Replaced both captures with explicit property access. Inline comment explains why so future edits don't re-introduce spread. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/index.ts | 4 ++++ test/lib/withOAuthValidation.test.js | 25 +++++++++++++++++++++---- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/src/index.ts b/src/index.ts index 354dce5..88e1fde 100644 --- a/src/index.ts +++ b/src/index.ts @@ -35,6 +35,10 @@ export { // Export provider utilities export { getProvider } from './lib/providers/index.ts'; +// Export OAuth session validation wrapper +export { withOAuthValidation, getOAuthProviders } from './lib/withOAuthValidation.ts'; +export type { OAuthValidationOptions } from './lib/withOAuthValidation.ts'; + // Store hooks registered at module load time and active hookManager let pendingHooks: OAuthHooks | null = null; let activeHookManager: HookManager | null = null; diff --git a/test/lib/withOAuthValidation.test.js b/test/lib/withOAuthValidation.test.js index f13b1d6..e371d82 100644 --- a/test/lib/withOAuthValidation.test.js +++ b/test/lib/withOAuthValidation.test.js @@ -429,9 +429,23 @@ describe('withOAuthValidation', () => { logger: mockLogger, requireAuth: true, onValidationError: (request, error) => { + // Explicit property access — Harper's GenericTrackedObject + // does NOT support `{ ...obj }` spread (per CLAUDE.md + // Non-Obvious Gotchas). Tests that snapshot via spread + // would silently yield empty objects in production and + // mislead integrators who copy the callback pattern. + const oauth = request.session.oauth; + const oauthUser = request.session.oauthUser; seen.push({ - oauthAtCall: request.session.oauth && { ...request.session.oauth }, - oauthUserAtCall: request.session.oauthUser && { ...request.session.oauthUser }, + oauthAtCall: oauth && { + provider: oauth.provider, + accessToken: oauth.accessToken, + }, + oauthUserAtCall: oauthUser && { + username: oauthUser.username, + email: oauthUser.email, + role: oauthUser.role, + }, error, }); return { status: 401, body: { custom: true } }; @@ -471,9 +485,12 @@ describe('withOAuthValidation', () => { logger: mockLogger, requireAuth: true, onValidationError: (request, error) => { + const oauth = request.session.oauth; seen.push({ - oauthAtCall: request.session.oauth && { ...request.session.oauth }, - oauthUserAtCall: request.session.oauthUser && { ...request.session.oauthUser }, + oauthAtCall: oauth && { + accessToken: oauth.accessToken, + someOtherField: oauth.someOtherField, + }, error, }); return { status: 401, body: { custom: true } }; From db072d5e8fd0c8e4abb04bd2a64cb2ee6976db1d Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Tue, 21 Apr 2026 10:26:48 -0700 Subject: [PATCH 08/15] test: cover getOAuthProviders + requireAuth:false expired-token path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps from Claude's round-6 review. 1. `getOAuthProviders` was newly exported as public API with zero tests. It has silent error-swallowing (returns null on failure), so the scope-traversal logic (`parent.resources.get('oauth')` → `resources.get('oauth')`) was completely unverified. Added four cases: parent-scope hit, same-scope fallback, miss-returns-null, and lookup-throws-returns-null. 2. The `requireAuth: false` + expired-token + no-refresh-token path was newly reachable (v4 couldn't reach validateAndRefreshSession because `args.find` never found the request). The session is cleared by validateAndRefreshSession before the wrapper falls through to the underlying method. Added a test that asserts (a) the method still runs and (b) `session.oauth` and `session.oauthUser` are cleared by the time the method observes the session. Co-Authored-By: Claude Opus 4.7 (1M context) --- test/lib/withOAuthValidation.test.js | 113 ++++++++++++++++++++++++++- 1 file changed, 112 insertions(+), 1 deletion(-) diff --git a/test/lib/withOAuthValidation.test.js b/test/lib/withOAuthValidation.test.js index e371d82..40c5282 100644 --- a/test/lib/withOAuthValidation.test.js +++ b/test/lib/withOAuthValidation.test.js @@ -9,7 +9,7 @@ import { describe, it, beforeEach } from 'node:test'; import assert from 'node:assert/strict'; -import { withOAuthValidation } from '../../dist/lib/withOAuthValidation.js'; +import { withOAuthValidation, getOAuthProviders } from '../../dist/lib/withOAuthValidation.js'; describe('withOAuthValidation', () => { let mockProviders; @@ -526,4 +526,115 @@ describe('withOAuthValidation', () => { assert.equal(result, undefined); }); }); + + describe('Expired token with requireAuth: false passes through and clears the session', () => { + // New reachable behavior post-v5: `validateAndRefreshSession` + // calls `clearOAuthSession` internally when the token is expired + // and no refresh token is available. When `requireAuth` is false + // the wrapper falls through to the underlying method — but the + // session's oauth data has already been cleared as a side effect. + it('underlying method runs; session.oauth is cleared', async () => { + const calls = []; + class MyResource extends MockResource { + async get(target) { + // Capture session state AT method invocation — by this + // point validateAndRefreshSession should have cleared + // the stale oauth fields. + calls.push({ + target, + oauthAfterValidate: this._context.session.oauth, + oauthUserAfterValidate: this._context.session.oauthUser, + }); + return { status: 200, body: { ran: true } }; + } + } + const context = { + session: makeSession({ + oauth: { + provider: 'github', + accessToken: 'expired-token', + expiresAt: Date.now() - 60_000, + refreshToken: undefined, + }, + }), + }; + const Wrapped = withOAuthValidation(MyResource, { + providers: mockProviders, + logger: mockLogger, + requireAuth: false, + }); + + const instance = new Wrapped('x', context); + const result = await instance.get({ path: '/mixed' }); + + assert.equal(result.status, 200, 'underlying method must run when requireAuth is false'); + assert.equal(result.body.ran, true); + assert.equal(calls.length, 1); + assert.equal( + calls[0].oauthAfterValidate, + undefined, + 'stale oauth metadata must be cleared before the underlying method observes the session' + ); + assert.equal( + calls[0].oauthUserAfterValidate, + undefined, + 'stale oauthUser metadata must be cleared before the underlying method observes the session' + ); + }); + }); +}); + +describe('getOAuthProviders', () => { + const fakeRegistry = { github: { provider: {}, config: { provider: 'github' } } }; + + it('returns the provider registry from a parent scope', () => { + const oauthResource = { providers: fakeRegistry }; + const scope = { + parent: { + resources: { + get: (name) => (name === 'oauth' ? oauthResource : undefined), + }, + }, + resources: { + get: () => undefined, + }, + }; + + assert.equal(getOAuthProviders(scope), fakeRegistry); + }); + + it('returns the provider registry from the same scope when no parent lookup matches', () => { + const oauthResource = { providers: fakeRegistry }; + const scope = { + parent: { resources: { get: () => undefined } }, + resources: { + get: (name) => (name === 'oauth' ? oauthResource : undefined), + }, + }; + + assert.equal(getOAuthProviders(scope), fakeRegistry); + }); + + it('returns null when no oauth resource is found in either scope', () => { + const scope = { + parent: { resources: { get: () => undefined } }, + resources: { get: () => undefined }, + }; + + assert.equal(getOAuthProviders(scope), null); + }); + + it('returns null when the lookup throws (swallows traversal errors)', () => { + const scope = { + parent: { + resources: { + get: () => { + throw new Error('boom'); + }, + }, + }, + }; + + assert.equal(getOAuthProviders(scope), null); + }); }); From 08b9f1edac4203f5031ea7eaaafaec1c33bd7825 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Tue, 21 Apr 2026 12:42:08 -0700 Subject: [PATCH 09/15] fix: fail-closed when onValidationError returns undefined MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Critical finding from Claude's round-7 review. `validateOAuthForRequest` uses `return undefined` as the sentinel meaning "no problem — delegate should continue." But `onValidationError` can legitimately return `undefined` (e.g., a plain logging callback: `async (req, err) => { log(err) }`). When that happens, the wrapper interprets the return as "continue," falls through to the protected method, and silently bypasses authentication — even when `requireAuth` is true. All four error paths were affected: - `!hasOAuth` - `!providerName` - `!providerData` - `!validation.valid` Fix: route every `onValidationError` call through a new `callCallbackOrDeny` helper that applies `?? defaultDeny` to the handler's return value. If the handler returns `undefined`, the wrapper falls back to the default 401 instead of propagating the undefined as a pass-through. No observable change for handlers that return a response; handlers that return nothing now fail closed (matching the documented contract). Tests added for the previously-uncovered `!validation.valid` path (expired-token + no refresh token + `requireAuth: true`): - Handler returns a response → wrapper returns it verbatim. - Handler returns `undefined` → wrapper returns the default 401 and the protected method is never invoked. This is the exact bypass pattern the fix prevents. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lib/withOAuthValidation.ts | 38 ++++++++---- test/lib/withOAuthValidation.test.js | 93 ++++++++++++++++++++++++++++ 2 files changed, 120 insertions(+), 11 deletions(-) diff --git a/src/lib/withOAuthValidation.ts b/src/lib/withOAuthValidation.ts index fefa6ff..1f8f30c 100644 --- a/src/lib/withOAuthValidation.ts +++ b/src/lib/withOAuthValidation.ts @@ -62,12 +62,27 @@ async function validateOAuthForRequest(context: MaybeContext, options: OAuthVali return undefined; } + // When onValidationError is provided, we still always need a + // fall-back 401 to return if the handler returns `undefined` — e.g. + // a plain logging callback like `async (req, err) => { log(err) }`. + // Without the fallback, `undefined` would propagate as "no problem" + // and silently bypass auth. We use `??` everywhere the handler is + // invoked so a no-return handler is indistinguishable, security-wise, + // from no handler at all. + const callCallbackOrDeny = async (request: Request, error: string, defaultDeny: any): Promise => { + if (!onValidationError) return defaultDeny; + const result = await onValidationError(request, error); + return result ?? defaultDeny; + }; + const hasOAuth = request.session?.oauth !== undefined; if (!hasOAuth) { if (requireAuth) { const error = 'OAuth authentication required'; - if (onValidationError) return onValidationError(request, error); - return { status: 401, body: { error: 'Unauthorized', message: error } }; + return callCallbackOrDeny(request, error, { + status: 401, + body: { error: 'Unauthorized', message: error }, + }); } return undefined; } @@ -85,9 +100,10 @@ async function validateOAuthForRequest(context: MaybeContext, options: OAuthVali const error = 'Invalid OAuth session data'; // Invoke the callback BEFORE clearing so it can read // request.session.oauth / .oauthUser (audit logging, etc.). - const response = onValidationError - ? await onValidationError(request, error) - : { status: 401, body: { error: 'Unauthorized', message: error } }; + const response = await callCallbackOrDeny(request, error, { + status: 401, + body: { error: 'Unauthorized', message: error }, + }); clearStaleOAuth(); return response; } @@ -101,9 +117,10 @@ async function validateOAuthForRequest(context: MaybeContext, options: OAuthVali if (requireAuth) { const error = `OAuth provider '${providerName}' not configured`; // Same ordering as above: callback sees un-mutated request. - const response = onValidationError - ? await onValidationError(request, error) - : { status: 401, body: { error: 'Unauthorized', message: error } }; + const response = await callCallbackOrDeny(request, error, { + status: 401, + body: { error: 'Unauthorized', message: error }, + }); clearStaleOAuth(); return response; } @@ -116,15 +133,14 @@ async function validateOAuthForRequest(context: MaybeContext, options: OAuthVali logger?.info?.(`OAuth session validation failed: ${validation.error}`); if (requireAuth) { const error = validation.error || 'OAuth session expired'; - if (onValidationError) return onValidationError(request, error); - return { + return callCallbackOrDeny(request, error, { status: 401, body: { error: 'Unauthorized', message: 'OAuth session expired. Please log in again.', details: validation.error, }, - }; + }); } return undefined; } diff --git a/test/lib/withOAuthValidation.test.js b/test/lib/withOAuthValidation.test.js index 40c5282..a3aad50 100644 --- a/test/lib/withOAuthValidation.test.js +++ b/test/lib/withOAuthValidation.test.js @@ -527,6 +527,99 @@ describe('withOAuthValidation', () => { }); }); + describe('Expired token with requireAuth: true invokes onValidationError', () => { + // The !validation.valid path is the most common real-world + // failure: a logged-in user's token has expired and can't be + // refreshed. Two cases matter here: + // + // (a) Handler returns a custom response → that response is what + // the caller sees. + // (b) Handler returns undefined (plausible — a pure logging + // callback). The wrapper MUST fall back to the default 401 + // rather than propagating `undefined` as a "no problem" + // sentinel, which would silently invoke the protected method. + it('handler returns a response — wrapper returns it verbatim', async () => { + const calls = []; + class MyResource extends MockResource { + async get() { + calls.push('called'); + return { status: 200 }; + } + } + const context = { + session: makeSession({ + oauth: { + provider: 'github', + accessToken: 'expired', + expiresAt: Date.now() - 60_000, + refreshToken: undefined, + }, + }), + }; + const seen = []; + const Wrapped = withOAuthValidation(MyResource, { + providers: mockProviders, + logger: mockLogger, + requireAuth: true, + onValidationError: (request, error) => { + seen.push({ error, hasRequest: !!request }); + return { status: 418, body: { custom: true } }; + }, + }); + + const instance = new Wrapped('x', context); + const result = await instance.get({ path: '/protected' }); + + assert.equal(result.status, 418); + assert.equal(result.body.custom, true); + assert.equal(calls.length, 0, 'protected method must not run'); + assert.equal(seen.length, 1); + assert.match(seen[0].error, /expired/i); + }); + + it('handler returns undefined — wrapper falls back to default 401 (no silent bypass)', async () => { + const calls = []; + class MyResource extends MockResource { + async get() { + calls.push('called'); + return { status: 200, shouldNeverHappen: true }; + } + } + const context = { + session: makeSession({ + oauth: { + provider: 'github', + accessToken: 'expired', + expiresAt: Date.now() - 60_000, + refreshToken: undefined, + }, + }), + }; + let handlerCalled = false; + const Wrapped = withOAuthValidation(MyResource, { + providers: mockProviders, + logger: mockLogger, + requireAuth: true, + // Plausible real-world shape: a pure logging callback + // that happens to return void. Must NOT be interpreted + // as "continue." + onValidationError: () => { + handlerCalled = true; + // no return — returns undefined + }, + }); + + const instance = new Wrapped('x', context); + const result = await instance.get({ path: '/protected' }); + + assert.equal(handlerCalled, true, 'handler must be invoked'); + assert.equal(result.status, 401, 'must fail closed on undefined handler return'); + assert.equal(result.body.error, 'Unauthorized'); + assert.match(result.body.message, /expired/i); + assert.equal(calls.length, 0, 'protected method must NOT run — this is the silent-bypass case'); + }); + }); + describe('Expired token with requireAuth: false passes through and clears the session', () => { // New reachable behavior post-v5: `validateAndRefreshSession` // calls `clearOAuthSession` internally when the token is expired From 2c90dfab877a383e6e02787cd1426e955d032328 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Tue, 21 Apr 2026 14:30:27 -0700 Subject: [PATCH 10/15] docs/test: document + cover clearOAuthSession production vs fallback paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-8 findings from Claude on this PR, both legitimate. 1. My expired-token tests only exercised `clearOAuthSession`'s in-memory fallback. `makeSession()` has no `delete()` method, so every call took the `delete session.oauth; delete session.oauthUser` branch. In production Harper sessions expose `session.delete(session.id)`, which destroys the entire DB session record — a materially different (and more destructive) outcome. Added `makeProductionLikeSession()` that provides a `delete()` spy and records the calls. Restructured the expired-token `requireAuth: false` suite into two explicit cases — fallback path (no delete method) and production path (delete called, in-memory oauth fields untouched). 2. The wrapper's stale-provider path and expired-token path clear sessions DIFFERENTLY: stale-provider uses the local `clearStaleOAuth()` helper (in-memory only, session survives), while expired-token inherits `validateAndRefreshSession`'s side effect (`clearOAuthSession` → `session.delete(id)` in production, terminal DB destruction). The divergence is defensible — "provider not configured" is a recoverable config issue, "expired token with no refresh" is terminal auth — but it was entirely undocumented, so a v2 integrator using `requireAuth: false` on a hybrid resource would silently and permanently log users out on any token expiry. Added a "Session-cleanup semantics" block to the withOAuthValidation JSDoc spelling out what happens on each path. Test comment on the expired-token case updated to reflect both branches accurately. No behavior change — just documentation + test coverage. A future PR could consider unifying the two paths (e.g. `clearStaleOAuth()` for expired-token too), but that's a semantic decision worth its own PR. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lib/withOAuthValidation.ts | 16 ++++ test/lib/withOAuthValidation.test.js | 111 ++++++++++++++++++++++----- 2 files changed, 109 insertions(+), 18 deletions(-) diff --git a/src/lib/withOAuthValidation.ts b/src/lib/withOAuthValidation.ts index 1f8f30c..5af47ba 100644 --- a/src/lib/withOAuthValidation.ts +++ b/src/lib/withOAuthValidation.ts @@ -196,6 +196,22 @@ async function validateOAuthForRequest(context: MaybeContext, options: OAuthVali * still runs validation, then returns `undefined` — matching * Harper's "method not implemented" behavior. This means validation * runs even on unhandled verbs (defense-in-depth). + * + * Session-cleanup semantics (intentional divergence — important for + * integrators using `requireAuth: false`): + * - Stale-provider paths (no provider name on session, or provider + * not in registry): the wrapper clears only the in-memory `oauth` + * and `oauthUser` fields via a local helper. The session record + * itself survives — "provider not configured" may be a recoverable + * config issue. + * - Expired-token path (`validateAndRefreshSession` returns + * `{valid: false}`): `validateAndRefreshSession` internally calls + * `clearOAuthSession`, which on a Harper production session + * invokes `session.delete(session.id)` — the DB record is destroyed. + * This is terminal: the user is logged out, not just detached from + * OAuth. `requireAuth: false` resources still receive the + * passthrough call, but they observe a session that is about to + * stop existing on the next request. */ export function withOAuthValidation any>( ResourceClass: T, diff --git a/test/lib/withOAuthValidation.test.js b/test/lib/withOAuthValidation.test.js index a3aad50..a6ead80 100644 --- a/test/lib/withOAuthValidation.test.js +++ b/test/lib/withOAuthValidation.test.js @@ -57,6 +57,12 @@ describe('withOAuthValidation', () => { } function makeSession(overrides = {}) { + // IMPORTANT: this session has NO `delete()` method. That matters + // for tests that hit `clearOAuthSession` (e.g. expired-token + // paths): those tests exercise the in-memory fallback inside + // `clearOAuthSession`, not the production `session.delete(id)` + // path. For production-shaped behavior use + // `makeProductionLikeSession` below. return { id: 'sess-1', oauth: { @@ -71,6 +77,21 @@ describe('withOAuthValidation', () => { }; } + // A Harper-production-shaped session: provides `delete(id)` like the + // real hdb_session record so `clearOAuthSession` takes the + // `session.delete(id)` branch (full DB destruction) instead of the + // in-memory fallback. + function makeProductionLikeSession(overrides = {}) { + const base = makeSession(overrides); + const deleteCalls = []; + base.delete = async (id) => { + deleteCalls.push(id); + }; + // Expose the spy ledger for assertions + base.__deleteCalls = deleteCalls; + return base; + } + describe('Wrapped-class registration surface', () => { it('returns a subclass of the input class (instanceof preserved)', () => { class MyResource extends MockResource { @@ -620,19 +641,28 @@ describe('withOAuthValidation', () => { }); }); - describe('Expired token with requireAuth: false passes through and clears the session', () => { - // New reachable behavior post-v5: `validateAndRefreshSession` - // calls `clearOAuthSession` internally when the token is expired - // and no refresh token is available. When `requireAuth` is false - // the wrapper falls through to the underlying method — but the - // session's oauth data has already been cleared as a side effect. - it('underlying method runs; session.oauth is cleared', async () => { + describe('Expired token with requireAuth: false passes through — cleanup semantics', () => { + // `validateAndRefreshSession` calls `clearOAuthSession` as a side + // effect when the token is expired and no refresh token is + // available. When `requireAuth` is false the wrapper falls + // through to the underlying method, which runs with a session + // that's about to be (or has already been) cleaned up. + // + // `clearOAuthSession` has TWO code paths depending on whether + // the session provides a `delete(id)` method: + // - with `delete`: production path — Harper destroys the DB + // session record. The in-memory session + // object's oauth fields are NOT touched. + // - without: in-memory fallback — deletes `oauth` and + // `oauthUser` fields directly. + // + // Both paths are exercised below so behavior is pinned down for + // integrators. + + it('fallback path (no session.delete): underlying method runs, oauth fields cleared in-memory', async () => { const calls = []; class MyResource extends MockResource { async get(target) { - // Capture session state AT method invocation — by this - // point validateAndRefreshSession should have cleared - // the stale oauth fields. calls.push({ target, oauthAfterValidate: this._context.session.oauth, @@ -663,15 +693,60 @@ describe('withOAuthValidation', () => { assert.equal(result.status, 200, 'underlying method must run when requireAuth is false'); assert.equal(result.body.ran, true); assert.equal(calls.length, 1); - assert.equal( - calls[0].oauthAfterValidate, - undefined, - 'stale oauth metadata must be cleared before the underlying method observes the session' + // In the fallback path, clearOAuthSession deletes the in-memory + // oauth fields directly, so the resource observes an empty session. + assert.equal(calls[0].oauthAfterValidate, undefined); + assert.equal(calls[0].oauthUserAfterValidate, undefined); + }); + + it('production path (session.delete present): underlying method runs, delete(id) called with session id', async () => { + const calls = []; + class MyResource extends MockResource { + async get(target) { + // In the production path, `clearOAuthSession` invokes + // `session.delete(session.id)` — it does NOT mutate the + // in-memory session object. So by the time the resource + // runs, the DB record is doomed but the in-memory oauth + // fields may still be populated. + calls.push({ + target, + oauthAfterValidate: this._context.session.oauth, + }); + return { status: 200, body: { ran: true } }; + } + } + const session = makeProductionLikeSession({ + oauth: { + provider: 'github', + accessToken: 'expired-token', + expiresAt: Date.now() - 60_000, + refreshToken: undefined, + }, + }); + const context = { session }; + const Wrapped = withOAuthValidation(MyResource, { + providers: mockProviders, + logger: mockLogger, + requireAuth: false, + }); + + const instance = new Wrapped('x', context); + const result = await instance.get({ path: '/mixed' }); + + assert.equal(result.status, 200, 'underlying method must still run when requireAuth is false'); + assert.equal(result.body.ran, true); + assert.equal(calls.length, 1); + // The production path destroys the DB record via session.delete(session.id). + assert.deepEqual( + session.__deleteCalls, + ['sess-1'], + 'clearOAuthSession must call session.delete(session.id) in the production path' ); - assert.equal( - calls[0].oauthUserAfterValidate, - undefined, - 'stale oauthUser metadata must be cleared before the underlying method observes the session' + // The in-memory session object isn't mutated by the production path — + // documenting this so integrators know what the resource observes. + assert.ok( + calls[0].oauthAfterValidate !== undefined, + 'production path does not mutate the in-memory session object' ); }); }); From edef825c3cd77077eaee68aa24e81e85fa038ac7 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Wed, 22 Apr 2026 11:46:49 -0700 Subject: [PATCH 11/15] doc/test: pin onValidationError contract (requireAuth coupling + state visibility) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more findings from Claude's review on this PR — this round as inline review comments rather than top-level issue comments. 1. `onValidationError` is silently never invoked when `requireAuth: false`. All four `callCallbackOrDeny` call sites are inside `if (requireAuth)` branches. The JSDoc on `onValidationError` said only "Custom error handler for validation failures" and didn't mention the coupling. An integrator wiring up `onValidationError` for audit logging on a mixed-auth resource (`requireAuth: false`) would get silent no-ops on every stale session with no diagnostic. Fix: expand the JSDoc to spell out the `requireAuth: true` requirement and redirect "audit logging on mixed-auth" users to `logger` (always invoked) instead. Tests: two new cases — stale-provider + `requireAuth: false` and expired-token + `requireAuth: false`, both asserting the callback is NOT fired and the underlying method runs. 2. Session visibility at callback time varies by failure path. - `!hasOAuth`: no oauth to see. - stale-provider paths: callback invoked BEFORE `clearStaleOAuth()` — full oauth/oauthUser readable. - expired-token path: `validateAndRefreshSession` has ALREADY called `clearOAuthSession` internally before the wrapper invokes the callback. On a production Harper session (`session.delete()` present) the in-memory fields are NOT mutated — callback sees full data. On a test-shape session (no `delete()`) the fallback clears in-memory fields first — callback sees `undefined`. My existing "handler returns a response" expired-token test uses the no-delete fallback and only asserts `!!request`, which masks this divergence. Adding a production-shape variant that asserts the callback DOES see full oauth data in the real production path pins the contract. JSDoc updated with a per-path session-state matrix so integrators know what's readable when. No observable behavior change. JSDoc and test coverage only. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lib/withOAuthValidation.ts | 26 +++++- test/lib/withOAuthValidation.test.js | 123 +++++++++++++++++++++++++++ 2 files changed, 148 insertions(+), 1 deletion(-) diff --git a/src/lib/withOAuthValidation.ts b/src/lib/withOAuthValidation.ts index 5af47ba..0f7a2e5 100644 --- a/src/lib/withOAuthValidation.ts +++ b/src/lib/withOAuthValidation.ts @@ -21,7 +21,31 @@ export interface OAuthValidationOptions { logger?: Logger; /** Whether to require OAuth authentication (401 if not present) */ requireAuth?: boolean; - /** Custom error handler for validation failures */ + /** + * Custom error handler for validation failures. + * + * **Only invoked when `requireAuth` is `true`**. When `requireAuth` + * is `false` the wrapper passes through silently on any validation + * failure (cleaning up stale session data as a side effect) — this + * callback is not called. If you need audit logging on a mixed-auth + * resource, set `requireAuth: true` on that resource, or log from + * your own `logger` (which IS always invoked). + * + * **Session state visibility depending on the failure path:** + * - `!hasOAuth` — `request.session.oauth` is already `undefined` + * (there never was any). + * - `!providerName` / `!providerData` (stale-provider paths) — the + * callback is invoked BEFORE session cleanup, so + * `request.session.oauth` and `.oauthUser` are readable. + * - `!validation.valid` (expired token with no refresh token) — + * `validateAndRefreshSession` has ALREADY called + * `clearOAuthSession` internally before the callback runs. On a + * production Harper session this calls `session.delete(session.id)` + * (DB record destroyed; in-memory fields untouched). On a session + * without a `delete()` method it falls back to in-memory deletion + * of `.oauth` / `.oauthUser`. The callback is still invoked, but + * the session state it observes depends on which path ran. + */ onValidationError?: (request: Request, error: string) => any; } diff --git a/test/lib/withOAuthValidation.test.js b/test/lib/withOAuthValidation.test.js index a6ead80..8f0f385 100644 --- a/test/lib/withOAuthValidation.test.js +++ b/test/lib/withOAuthValidation.test.js @@ -639,6 +639,129 @@ describe('withOAuthValidation', () => { assert.match(result.body.message, /expired/i); assert.equal(calls.length, 0, 'protected method must NOT run — this is the silent-bypass case'); }); + + it('production-path session: callback sees full oauth data (not mutated by clearOAuthSession)', async () => { + // `validateAndRefreshSession` calls `clearOAuthSession` as a + // side effect before returning `{valid: false}`. In the + // production path (session with a `delete()` method), + // `clearOAuthSession` calls `session.delete(session.id)` and + // does NOT mutate the in-memory session object. So the + // `onValidationError` callback — invoked after that — + // still observes the full oauth/oauthUser data. Pin this + // behavior so it can't regress silently. + const session = makeProductionLikeSession({ + oauth: { + provider: 'github', + accessToken: 'expired', + expiresAt: Date.now() - 60_000, + refreshToken: undefined, + }, + }); + const calls = []; + class MyResource extends MockResource { + async get() { + calls.push('called'); + return { status: 200 }; + } + } + + const seen = []; + const Wrapped = withOAuthValidation(MyResource, { + providers: mockProviders, + logger: mockLogger, + requireAuth: true, + onValidationError: (request) => { + const oauth = request.session.oauth; + const oauthUser = request.session.oauthUser; + seen.push({ + oauthProvider: oauth?.provider, + oauthAccessToken: oauth?.accessToken, + oauthUserEmail: oauthUser?.email, + }); + return { status: 401, body: { ok: true } }; + }, + }); + + const instance = new Wrapped('x', { session }); + await instance.get({ path: '/protected' }); + + assert.equal(calls.length, 0, 'protected method must not run'); + assert.equal(seen.length, 1); + assert.equal(seen[0].oauthProvider, 'github', 'oauth.provider must be readable in production path'); + assert.equal(seen[0].oauthAccessToken, 'expired', 'oauth.accessToken must be readable in production path'); + assert.equal(seen[0].oauthUserEmail, 'alice@example.com', 'oauthUser.email must be readable'); + assert.deepEqual(session.__deleteCalls, ['sess-1'], 'session.delete(id) was called'); + }); + }); + + describe('onValidationError is only invoked when requireAuth is true', () => { + // `onValidationError` exists to let integrators customize the + // 401 response (or add logging). It is documented to fire ONLY + // when `requireAuth` is true: with `requireAuth: false` the + // wrapper's contract is "pass through; clean up stale state as + // a side effect" and the callback is explicitly NOT called. + // Integrators reading the JSDoc should not expect to use + // `onValidationError` as an audit hook for mixed-auth resources. + + it('stale-provider + requireAuth: false → callback NOT invoked (session still cleaned)', async () => { + class MyResource extends MockResource { + async get() { + return { status: 200 }; + } + } + const context = { + session: makeSession({ oauth: { provider: 'ghost-provider', accessToken: 'stale' } }), + }; + let handlerCalled = false; + const Wrapped = withOAuthValidation(MyResource, { + providers: mockProviders, + logger: mockLogger, + requireAuth: false, + onValidationError: () => { + handlerCalled = true; + return { status: 500, body: { shouldNotSeeThis: true } }; + }, + }); + + const instance = new Wrapped('x', context); + const result = await instance.get({ path: '/mixed' }); + + assert.equal(handlerCalled, false, 'callback must NOT fire when requireAuth is false'); + assert.equal(result.status, 200, 'underlying method runs'); + assert.equal(context.session.oauth, undefined, 'stale session data still cleaned up'); + }); + + it('expired-token + requireAuth: false → callback NOT invoked (passthrough)', async () => { + class MyResource extends MockResource { + async get() { + return { status: 200 }; + } + } + const session = makeProductionLikeSession({ + oauth: { + provider: 'github', + accessToken: 'expired', + expiresAt: Date.now() - 60_000, + refreshToken: undefined, + }, + }); + let handlerCalled = false; + const Wrapped = withOAuthValidation(MyResource, { + providers: mockProviders, + logger: mockLogger, + requireAuth: false, + onValidationError: () => { + handlerCalled = true; + return { status: 500, body: { shouldNotSeeThis: true } }; + }, + }); + + const instance = new Wrapped('x', { session }); + const result = await instance.get({ path: '/mixed' }); + + assert.equal(handlerCalled, false, 'callback must NOT fire when requireAuth is false'); + assert.equal(result.status, 200, 'underlying method runs'); + }); }); describe('Expired token with requireAuth: false passes through — cleanup semantics', () => { From 1fa1e01b8b657bd6258830e139eacb0dc3e4c7f1 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Wed, 22 Apr 2026 13:13:02 -0700 Subject: [PATCH 12/15] fix: close static-method dispatch bypass; preserve 405 for unimplemented verbs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A local reviewer running a full pass on this PR caught two real issues that six rounds of CI review had missed. 1. Static-method dispatch bypass (blocker). Harper v5's REST dispatcher calls `Class.get(target, request)` at the STATIC level. When a user follows the v5-recommended pattern (`static async get(target)` — explicitly documented in release-notes/v5-lincoln/v5-migration.md as "We recommend using the static methods on Resources/Tables to implement endpoints"), static inheritance means `Wrapped.get` resolves to the user's static. Our instance-method overrides are bypassed and OAuth validation never runs — the endpoint silently serves unauthenticated traffic with zero diagnostic signal. This is the same failure shape as the original Proxy-vs-class issue the earlier commits addressed, just on a different dispatch surface. It's the hazard this PR's whole refactor exists to prevent. Fix: install static-method overrides on Wrapped for each verb the parent class has (including those inherited from Resource base via `transactional(...)`). The override runs validation first, then delegates to the inherited static via `.call(Wrapped, ...)` so `this` stays Wrapped (preserving `new this(...)` inside transactional). 2. 405 Method Not Allowed regression. Previously the wrapper defined all five HTTP verb overrides on `Wrapped.prototype` unconditionally. When the parent implemented only `get`, sending POST went: Harper → instance.post (our override, defined) → delegate → parentProto.post (undefined) → returns undefined → Harper serializes undefined as 204 No Content instead of the correct 405 Allow-header response. Harper's 405 path triggers when the instance-level property is falsy at dispatch time — our always-defined overrides masked it. Fix: only install instance overrides for verbs the parent actually implements. Unimplemented verbs remain undefined on Wrapped, so Harper sees the same "no method" signal it did pre-wrapper and returns the correct 405. Dedupe: with overrides installed at BOTH static and instance layers, naive dispatch would run validation twice per request (static entry, then Resource base's transactional creates an instance and calls instance get — which is also our override). A WeakSet keyed by the request/context marks contexts after the first validation pass, so subsequent calls in the same chain short-circuit. Non-mutating, garbage-collects with the request, safe even if the context is frozen. Tests: - `Wrapped.prototype.` left undefined when parent doesn't implement (405 preservation) - `Wrapped.` static left undefined when parent has no such static - Static-only user class (v5-recommended pattern) is wrapped: valid session → user static runs, invalid session → 401 at static entry before user code - Validation runs exactly once across the static→instance chain JSDoc expanded to document the dual-layer install and the WeakSet dedupe. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lib/withOAuthValidation.ts | 108 ++++++++++++------ test/lib/withOAuthValidation.test.js | 159 +++++++++++++++++++++++++-- 2 files changed, 227 insertions(+), 40 deletions(-) diff --git a/src/lib/withOAuthValidation.ts b/src/lib/withOAuthValidation.ts index 0f7a2e5..5540381 100644 --- a/src/lib/withOAuthValidation.ts +++ b/src/lib/withOAuthValidation.ts @@ -51,6 +51,8 @@ export interface OAuthValidationOptions { type MaybeContext = Context | SourceContext | undefined; +const HTTP_METHODS = ['get', 'post', 'put', 'patch', 'delete'] as const; + /** * Run OAuth validation for a request context. * @@ -213,13 +215,23 @@ async function validateOAuthForRequest(context: MaybeContext, options: OAuthVali * properties (including `loadAsInstance`) and static methods * (including `getResource`) are inherited, so Harper's registration * and dispatch lifecycle works unchanged. - * - The five standard HTTP methods — `get`, `post`, `put`, `patch`, - * `delete` — are overridden to run validation first. Other methods - * (subscriptions, helpers, etc.) pass through untouched. - * - If the parent class does not define an HTTP method, the wrapper - * still runs validation, then returns `undefined` — matching - * Harper's "method not implemented" behavior. This means validation - * runs even on unhandled verbs (defense-in-depth). + * - Validation is installed at BOTH static and instance levels so + * either Harper v5 dispatch pattern is intercepted: + * - User defines `async get(target)` (instance method): Harper's + * Resource base static `get` creates a Wrapped instance and + * invokes the instance method, which runs validation first. + * - User defines `static async get(target)` (v5-recommended pattern + * per Harper's migration guide): Harper calls `Wrapped.get` + * directly at the static level. Our static override catches this + * and runs validation before delegating. + * - Only the verbs the parent class actually implements get + * overridden. Unimplemented verbs remain undefined on the wrapper, + * so Harper's built-in "405 Method Not Allowed" response is + * preserved for them rather than being masked as a 204. + * - Validation is de-duplicated across the static→instance call chain + * via a per-request `WeakSet`. `validateAndRefreshSession` can hit + * the network for token refresh; running it twice per request would + * waste a round-trip. * * Session-cleanup semantics (intentional divergence — important for * integrators using `requireAuth: false`): @@ -241,37 +253,71 @@ export function withOAuthValidation a ResourceClass: T, options: OAuthValidationOptions ): T { - // Capture the parent prototype so we can look up HTTP methods directly - // without using `super` (TypeScript doesn't allow optional chaining on - // `super` member access, and the `delete` keyword as a method name via - // `super` trips up some compilation targets). Prototype lookup walks - // the chain, so inherited methods are found too. const parentProto = (ResourceClass as any).prototype; - const delegate = async (instance: any, method: string, args: any[]) => { - const deny = await validateOAuthForRequest(instance.getContext?.(), options); + // Dedupe validation across the static→instance call chain. When Harper's + // dispatch calls our static override, validation runs. If the parent + // static is Resource's base transactional wrapper, it will then create + // an instance and call the instance method — which is ALSO one of our + // overrides (when we added one). Without dedupe, validation runs twice + // per request and refreshAccessToken may hit the network twice. + // + // A WeakSet keyed by the request/context is the cleanest signal: + // non-mutating, garbage-collected with the request, and safe even if + // the context is a frozen object. + const validated = new WeakSet(); + + const validateOnce = async (context: any): Promise => { + if (context && typeof context === 'object' && validated.has(context)) { + return undefined; // already validated in this request + } + const deny = await validateOAuthForRequest(context, options); if (deny !== undefined) return deny; - const parentMethod = parentProto?.[method]; - return typeof parentMethod === 'function' ? parentMethod.apply(instance, args) : undefined; + if (context && typeof context === 'object') validated.add(context); + return undefined; }; - const Wrapped = class extends (ResourceClass as any) { - async get(...args: any[]) { - return delegate(this, 'get', args); - } - async post(...args: any[]) { - return delegate(this, 'post', args); - } - async put(...args: any[]) { - return delegate(this, 'put', args); - } - async patch(...args: any[]) { - return delegate(this, 'patch', args); + const Wrapped = class extends (ResourceClass as any) {}; + + // Instance-method overrides — ONLY for verbs the parent actually + // implements. If the parent has no `post`, leaving Wrapped.prototype.post + // undefined preserves Harper's "method not implemented" response + // (405 Allow-header) for unimplemented verbs. Defining an override for + // every verb unconditionally would cause Harper to invoke our wrapper, + // see `undefined` returned from the missing parent method, and serialize + // that as 204 No Content — the wrong wire-level signal. + for (const method of HTTP_METHODS) { + if (typeof parentProto?.[method] === 'function') { + const parentInstanceMethod = parentProto[method]; + (Wrapped.prototype as any)[method] = async function (this: any, ...args: any[]) { + const context = typeof this?.getContext === 'function' ? this.getContext() : undefined; + const deny = await validateOnce(context); + if (deny !== undefined) return deny; + return parentInstanceMethod.apply(this, args); + }; } - async delete(...args: any[]) { - return delegate(this, 'delete', args); + } + + // Static-method overrides — intercept Harper's direct class-level + // dispatch. Harper's REST dispatcher calls `Class.get(target, request)` + // at the STATIC level. When a user follows Harper v5's documented + // pattern (`static async get(target)`), static inheritance means + // Wrapped.get resolves to the user's static — our instance-method + // overrides are bypassed. Overriding statics too catches this case. + // + // Args from Harper's dispatch are `(target, request)`; the request + // (which carries `.session`) is what validation needs. + for (const method of HTTP_METHODS) { + const parentStatic = (ResourceClass as any)[method]; + if (typeof parentStatic === 'function') { + (Wrapped as any)[method] = async function (target: any, request: any) { + const deny = await validateOnce(request); + if (deny !== undefined) return deny; + return parentStatic.call(Wrapped, target, request); + }; } - }; + } + return Wrapped as unknown as T; } diff --git a/test/lib/withOAuthValidation.test.js b/test/lib/withOAuthValidation.test.js index 8f0f385..e3c2d30 100644 --- a/test/lib/withOAuthValidation.test.js +++ b/test/lib/withOAuthValidation.test.js @@ -528,13 +528,13 @@ describe('withOAuthValidation', () => { }); }); - describe('Undefined super methods', () => { - it('returns undefined if the base class does not define the HTTP method', async () => { - // Matches Harper's own "method not implemented" behavior: - // `resource.?.(…)` short-circuits to undefined, and - // Harper renders that as 404 / method-not-allowed. The - // wrapper still runs validation first so unreachable verbs - // are defense-in-depth protected. + describe('Unimplemented verbs preserve 405 semantics', () => { + // When the parent class doesn't implement an HTTP verb, the + // wrapper MUST NOT define one either — otherwise Harper's REST + // dispatcher sees a callable method that returns `undefined` + // and serializes that as 204 No Content (wrong), masking the + // correct 405 Method Not Allowed signal. + it('leaves Wrapped.prototype. undefined when the parent does not define it', () => { class GetOnly extends MockResource { async get() { return { status: 200 }; @@ -543,8 +543,149 @@ describe('withOAuthValidation', () => { const Wrapped = withOAuthValidation(GetOnly, { providers: mockProviders, logger: mockLogger }); const instance = new Wrapped('x', { session: makeSession() }); - const result = await instance.post({ path: '/x' }, { data: 1 }); - assert.equal(result, undefined); + assert.equal(typeof instance.get, 'function', 'get is defined — Wrapped wraps it'); + assert.equal(typeof instance.post, 'undefined', 'post is NOT wrapped — preserves 405'); + assert.equal(typeof instance.put, 'undefined'); + assert.equal(typeof instance.patch, 'undefined'); + assert.equal(typeof instance.delete, 'undefined'); + }); + + it('leaves Wrapped. static undefined when the parent has no static for it', () => { + // MockResource only defines the instance get. It has no + // static get/post/etc. on itself (test-shape base). Wrapped + // must match. + class GetOnly extends MockResource { + async get() { + return { status: 200 }; + } + } + const Wrapped = withOAuthValidation(GetOnly, { providers: mockProviders, logger: mockLogger }); + + // No static methods installed — MockResource doesn't have any. + assert.equal(typeof Wrapped.get, 'undefined'); + assert.equal(typeof Wrapped.post, 'undefined'); + }); + }); + + describe('Static-method dispatch bypass is closed', () => { + // Harper v5's migration guide recommends `static async get(target)` + // on Resource classes. When a user follows that pattern, Harper + // dispatches at the STATIC level — our instance-method overrides + // alone can't intercept, so validation would be silently skipped. + // The wrapper installs static overrides for exactly this reason. + + // A Resource-shaped base that also exposes statics (closer to the + // real Harper Resource base than plain `MockResource`). Each + // static dispatches to an instance method when present. + class StaticCapableResource { + static loadAsInstance = false; + constructor(id, context) { + this._id = id; + this._context = context ?? null; + } + getContext() { + return this._context; + } + } + for (const method of ['get', 'post', 'put', 'patch', 'delete']) { + StaticCapableResource[method] = function (target, request) { + const instance = new this(target?.id ?? 'sid', request); + return instance[method]?.(target); + }; + } + + it('intercepts user-defined static get (v5-recommended pattern)', async () => { + let staticRan = false; + class StaticUser extends StaticCapableResource { + // User-defined STATIC method — this would bypass the + // previous Proxy/instance-only wrapper. The wrapper's + // static override must fire validation BEFORE this runs. + static async get(target, request) { + staticRan = true; + return { status: 200, body: { user: request.session.oauthUser.email } }; + } + } + const Wrapped = withOAuthValidation(StaticUser, { + providers: mockProviders, + logger: mockLogger, + requireAuth: true, + }); + + // Call at the static level exactly like Harper does + const result = await Wrapped.get({ path: '/protected' }, { session: makeSession() }); + + assert.equal(result.status, 200); + assert.equal(result.body.user, 'alice@example.com'); + assert.equal(staticRan, true, 'user static ran (validation let it through)'); + }); + + it('returns 401 at the static entry when requireAuth is true and no OAuth data', async () => { + let staticRan = false; + class StaticUser extends StaticCapableResource { + static async get() { + staticRan = true; + return { status: 200, body: { shouldNotSeeThis: true } }; + } + } + const Wrapped = withOAuthValidation(StaticUser, { + providers: mockProviders, + logger: mockLogger, + requireAuth: true, + }); + + // No OAuth on the session — must be rejected at static entry, + // before the user's static runs. + const result = await Wrapped.get({ path: '/protected' }, { session: { id: 'no-oauth' } }); + + assert.equal(result.status, 401, 'static entry must enforce OAuth for user-static case'); + assert.equal(staticRan, false, 'user static must NOT run on auth failure'); + }); + + it('validation runs only once per request across the static→instance chain', async () => { + // Instance-method case: Harper calls the static (Resource + // base's transactional dispatch), which creates an instance + // and calls instance get. Both layers go through the wrapper. + // The WeakSet dedup must keep validation to a single network + // hit per request. + let validateCalls = 0; + const countingProvider = { + provider: { + refreshAccessToken: async () => ({ access_token: 'new', expires_in: 3600, token_type: 'Bearer' }), + config: { provider: 'github' }, + }, + config: { provider: 'github' }, + }; + let instanceRan = 0; + class InstanceUser extends StaticCapableResource { + async get() { + instanceRan += 1; + return { status: 200 }; + } + } + // Spy on validation: count how many times mockProviders.github is consulted. + // We approximate by counting how often the validation helper reaches the + // provider lookup — every validation pass reads providers[providerName]. + const spiedProviders = new Proxy( + { github: countingProvider }, + { + get(target, prop) { + if (prop === 'github') validateCalls += 1; + return target[prop]; + }, + } + ); + + const Wrapped = withOAuthValidation(InstanceUser, { + providers: spiedProviders, + logger: mockLogger, + requireAuth: true, + }); + + // Static entry, the realistic Harper dispatch path + await Wrapped.get({ path: '/x' }, { session: makeSession() }); + + assert.equal(instanceRan, 1, 'instance method runs exactly once'); + assert.equal(validateCalls, 1, 'validation runs exactly once (no double-hit)'); }); }); From e408f7d433dfecb9b524cf62a136cd1b6bb63e84 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Wed, 22 Apr 2026 17:42:05 -0700 Subject: [PATCH 13/15] fix: preserve 405 against Harper-shaped base; harden dedup across dispatch normalization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 9 review findings: 1. 405 regression. Harper's Resource base defines `static = transactional(...)` for every HTTP verb, so every user subclass inherits a truthy static for every verb. The previous `typeof ResourceClass[method] === 'function'` check matched the inherited transactional and installed an auth override for every verb — turning an expected 405 (from REST.ts's missingMethod branch) into a 401. Install only when the user owns the static (hasOwnProperty) OR owns an instance method on the prototype chain; otherwise shadow with `undefined` so REST dispatch's `resource. ? ... : missingMethod` check takes the 405 branch. 2. WeakSet identity across dispatch normalization. Harper's `transactional` and `getResource` normalize via `request.getContext?.() || request` before constructing the per-request instance, so `this.getContext()` in the instance path may be a different reference than the `request` arg the static received. Stamp both on mark; check both on lookup. 3. Stale-provider fail-closed tests. Added parallel tests for `!providerName` and `!providerData` branches with callbacks that return undefined — mirrors the existing expired-token "handler returns undefined" test. A future refactor that inlines callback invocation on one branch without `?? defaultDeny` would silently bypass auth on that branch; direct coverage catches it. Also adds a 405-preservation test using StaticCapableResource (all 5 statics inherited, closer to real Harper base) where the user only defines instance get — asserts sibling verbs are `undefined` on the Wrapped class. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lib/withOAuthValidation.ts | 120 ++++++++++++++++-------- test/lib/withOAuthValidation.test.js | 133 +++++++++++++++++++++++++++ 2 files changed, 217 insertions(+), 36 deletions(-) diff --git a/src/lib/withOAuthValidation.ts b/src/lib/withOAuthValidation.ts index 5540381..20a114a 100644 --- a/src/lib/withOAuthValidation.ts +++ b/src/lib/withOAuthValidation.ts @@ -224,14 +224,21 @@ async function validateOAuthForRequest(context: MaybeContext, options: OAuthVali * per Harper's migration guide): Harper calls `Wrapped.get` * directly at the static level. Our static override catches this * and runs validation before delegating. - * - Only the verbs the parent class actually implements get - * overridden. Unimplemented verbs remain undefined on the wrapper, - * so Harper's built-in "405 Method Not Allowed" response is - * preserved for them rather than being masked as a 204. + * - Only the verbs the user class actually implements (own static OR + * own instance method) get overridden. Unimplemented verbs are + * explicitly shadowed with `undefined` on the wrapper to blot out + * the Resource base's inherited transactional static, so Harper's + * native "405 Method Not Allowed" response (via `missingMethod` at + * server/REST.ts:117) is preserved — rather than our wrapper running + * validation on a verb the user never meant to expose. * - Validation is de-duplicated across the static→instance call chain - * via a per-request `WeakSet`. `validateAndRefreshSession` can hit - * the network for token refresh; running it twice per request would - * waste a round-trip. + * via a per-request `WeakSet`. Both the request object AND + * `request.getContext?.()` are stamped, because Harper's transactional + * wrapper normalizes the request during dispatch — the object the + * instance method observes via `this.getContext()` may be a different + * reference than the one the static override saw. `validateAndRefresh + * Session` can hit the network for token refresh; running it twice per + * request would waste a round-trip. * * Session-cleanup semantics (intentional divergence — important for * integrators using `requireAuth: false`): @@ -262,33 +269,72 @@ export function withOAuthValidation a // overrides (when we added one). Without dedupe, validation runs twice // per request and refreshAccessToken may hit the network twice. // - // A WeakSet keyed by the request/context is the cleanest signal: - // non-mutating, garbage-collected with the request, and safe even if - // the context is a frozen object. + // Harper's `transactional` wrapper and `getResource` normalize the + // request via `request.getContext?.() || request` before constructing + // the per-request instance, so `this.getContext()` in the instance + // path may resolve to a DIFFERENT object than the `request` argument + // the static received. We stamp both references (and check both on + // lookup) so dedup works regardless of which one the instance observes. const validated = new WeakSet(); + const markValidated = (ctx: any): void => { + if (!ctx || typeof ctx !== 'object') return; + validated.add(ctx); + const inner = typeof ctx.getContext === 'function' ? ctx.getContext() : undefined; + if (inner && typeof inner === 'object' && inner !== ctx) validated.add(inner); + }; + + const hasValidated = (ctx: any): boolean => { + if (!ctx || typeof ctx !== 'object') return false; + if (validated.has(ctx)) return true; + const inner = typeof ctx.getContext === 'function' ? ctx.getContext() : undefined; + return !!(inner && typeof inner === 'object' && validated.has(inner)); + }; + const validateOnce = async (context: any): Promise => { - if (context && typeof context === 'object' && validated.has(context)) { - return undefined; // already validated in this request - } + if (hasValidated(context)) return undefined; const deny = await validateOAuthForRequest(context, options); if (deny !== undefined) return deny; - if (context && typeof context === 'object') validated.add(context); + markValidated(context); return undefined; }; const Wrapped = class extends (ResourceClass as any) {}; - // Instance-method overrides — ONLY for verbs the parent actually - // implements. If the parent has no `post`, leaving Wrapped.prototype.post - // undefined preserves Harper's "method not implemented" response - // (405 Allow-header) for unimplemented verbs. Defining an override for - // every verb unconditionally would cause Harper to invoke our wrapper, - // see `undefined` returned from the missing parent method, and serialize - // that as 204 No Content — the wrong wire-level signal. + // Per-verb installation decision. Harper's Resource base defines a + // static transactional wrapper for every HTTP verb, so inheritance + // alone makes `(ResourceClass as any)[method]` truthy for every verb + // — we can't use the static's presence to tell whether the USER + // implements a verb. We infer user intent from: + // 1. an OWN static on the class (v5-recommended `static async get` + // pattern), or + // 2. an instance method on the prototype chain above Resource + // (Resource.prototype does not define these as functions, so a + // truthy parentProto[method] implies the user defined it). + // When neither is present we SHADOW the inherited static with + // `undefined` so REST dispatch (`resource.get ? ... : missingMethod`) + // falls through to Harper's native 405 path instead of our wrapper + // running validation on a verb the user never meant to expose. for (const method of HTTP_METHODS) { - if (typeof parentProto?.[method] === 'function') { - const parentInstanceMethod = parentProto[method]; + const hasOwnStatic = Object.prototype.hasOwnProperty.call(ResourceClass, method); + const parentInstanceMethod = typeof parentProto?.[method] === 'function' ? parentProto[method] : undefined; + const userImplements = hasOwnStatic || parentInstanceMethod !== undefined; + + if (!userImplements) { + // Shadow the inherited Resource base transactional so REST.ts's + // `resource.method ? ... : missingMethod(resource, method)` check + // takes the 405 branch. Without this, the inherited transactional + // would run and our wrapper would still validate — turning an + // expected 405 into a 401 (or, with requireAuth: false, a 204 from + // the transactional's `resource.method?.()` optional chain). + (Wrapped as any)[method] = undefined; + continue; + } + + // Instance-method override — validates when the parent's static + // dispatch (our own wrapper, or the inherited transactional) + // eventually creates an instance and calls the instance method. + if (parentInstanceMethod) { (Wrapped.prototype as any)[method] = async function (this: any, ...args: any[]) { const context = typeof this?.getContext === 'function' ? this.getContext() : undefined; const deny = await validateOnce(context); @@ -296,24 +342,26 @@ export function withOAuthValidation a return parentInstanceMethod.apply(this, args); }; } - } - // Static-method overrides — intercept Harper's direct class-level - // dispatch. Harper's REST dispatcher calls `Class.get(target, request)` - // at the STATIC level. When a user follows Harper v5's documented - // pattern (`static async get(target)`), static inheritance means - // Wrapped.get resolves to the user's static — our instance-method - // overrides are bypassed. Overriding statics too catches this case. - // - // Args from Harper's dispatch are `(target, request)`; the request - // (which carries `.session`) is what validation needs. - for (const method of HTTP_METHODS) { + // Static-method override — intercepts Harper's direct class-level + // dispatch before either the user's own static or the inherited + // transactional runs. Without this, a user defining `static async + // get` (the v5-recommended pattern) bypasses validation entirely, + // and a user defining an instance `get` would have validation + // skipped on the static-call leg before the transactional creates + // an instance. + // + // REST dispatch arg shapes (server/REST.ts:117–125): + // GET/DELETE: (target, request) + // POST/PUT/PATCH: (target, data, request) + // The request (carrying `.session`) is always the last argument. const parentStatic = (ResourceClass as any)[method]; if (typeof parentStatic === 'function') { - (Wrapped as any)[method] = async function (target: any, request: any) { + (Wrapped as any)[method] = async function (...args: any[]) { + const request = args.length > 0 ? args[args.length - 1] : undefined; const deny = await validateOnce(request); if (deny !== undefined) return deny; - return parentStatic.call(Wrapped, target, request); + return parentStatic.apply(Wrapped, args); }; } } diff --git a/test/lib/withOAuthValidation.test.js b/test/lib/withOAuthValidation.test.js index e3c2d30..7df2b10 100644 --- a/test/lib/withOAuthValidation.test.js +++ b/test/lib/withOAuthValidation.test.js @@ -687,6 +687,64 @@ describe('withOAuthValidation', () => { assert.equal(instanceRan, 1, 'instance method runs exactly once'); assert.equal(validateCalls, 1, 'validation runs exactly once (no double-hit)'); }); + + it('405 preservation: unimplemented verbs shadowed with undefined when base exposes all statics', () => { + // Harper's real Resource base defines `static = transactional(...)` + // for every HTTP verb — every user subclass INHERITS a truthy + // static for every verb. A simple `typeof ResourceClass[method] === + // 'function'` check would cause the wrapper to install an auth + // override for every verb, turning an expected 405 (from Harper's + // `missingMethod` branch in server/REST.ts:117) into a 401 when + // requireAuth is true, or a 204 passthrough otherwise. The wrapper + // MUST shadow unimplemented verbs with `undefined` so REST + // dispatch's `resource. ? ... : missingMethod(...)` check + // takes the 405 branch. + class GetOnly extends StaticCapableResource { + async get() { + return { status: 200 }; + } + } + const Wrapped = withOAuthValidation(GetOnly, { + providers: mockProviders, + logger: mockLogger, + requireAuth: true, + }); + + // User implements GET via instance method — wrapped (static leg + // intercepts for validation; inherited transactional reaches the + // instance method). + assert.equal(typeof Wrapped.get, 'function'); + // All other verbs: inherited from StaticCapableResource (mimicking + // Harper's Resource base). The wrapper MUST explicitly shadow + // these with `undefined`, otherwise Harper's REST dispatcher sees + // a truthy static and skips the 405 branch. + assert.equal(typeof Wrapped.post, 'undefined', 'post must be undefined to preserve Harper 405'); + assert.equal(typeof Wrapped.put, 'undefined', 'put must be undefined to preserve Harper 405'); + assert.equal(typeof Wrapped.patch, 'undefined', 'patch must be undefined to preserve Harper 405'); + assert.equal(typeof Wrapped.delete, 'undefined', 'delete must be undefined to preserve Harper 405'); + }); + + it('405 preservation: user-owned static overrides inherited shadowing', () => { + // When the user defines their OWN static for a verb (Harper v5 + // migration guide's recommended pattern), the wrapper must + // install a validation override — not shadow to undefined. + class PostAsStatic extends StaticCapableResource { + static async post(_target, request) { + return { status: 201, body: { email: request.session.oauthUser.email } }; + } + } + const Wrapped = withOAuthValidation(PostAsStatic, { + providers: mockProviders, + logger: mockLogger, + }); + + assert.equal(typeof Wrapped.post, 'function', 'user own-static gets wrapped for validation'); + // Verbs the user neither declared as own-static nor as instance + // method remain shadowed, even though the base class exposes them. + assert.equal(typeof Wrapped.get, 'undefined', 'get still shadowed'); + assert.equal(typeof Wrapped.put, 'undefined', 'put still shadowed'); + assert.equal(typeof Wrapped.delete, 'undefined', 'delete still shadowed'); + }); }); describe('Expired token with requireAuth: true invokes onValidationError', () => { @@ -781,6 +839,81 @@ describe('withOAuthValidation', () => { assert.equal(calls.length, 0, 'protected method must NOT run — this is the silent-bypass case'); }); + // The expired-token path has explicit fail-closed coverage above. + // The `!providerName` and `!providerData` stale-provider paths use + // the SAME `callCallbackOrDeny` helper with the SAME `?? defaultDeny` + // semantics — but each path deserves its own direct test, because a + // future refactor that inlines the callback invocation on ONE branch + // without `defaultDeny` would silently bypass auth there and the + // expired-token test would still pass, giving false confidence. + it('no-provider-name branch: handler returns undefined — wrapper falls back to default 401', async () => { + const calls = []; + class MyResource extends MockResource { + async get() { + calls.push('called'); + return { status: 200, shouldNeverHappen: true }; + } + } + const context = { + // Session has an oauth object but no `provider` — hits the + // `!providerName` branch in validateOAuthForRequest. + session: makeSession({ oauth: { accessToken: 'stale', provider: undefined } }), + }; + let handlerCalled = false; + const Wrapped = withOAuthValidation(MyResource, { + providers: mockProviders, + logger: mockLogger, + requireAuth: true, + onValidationError: () => { + handlerCalled = true; + // no return — returns undefined + }, + }); + + const instance = new Wrapped('x', context); + const result = await instance.get({ path: '/protected' }); + + assert.equal(handlerCalled, true, 'handler must be invoked on stale-session branch'); + assert.equal(result.status, 401, 'must fail closed on undefined handler return'); + assert.equal(result.body.error, 'Unauthorized'); + assert.match(result.body.message, /invalid/i); + assert.equal(calls.length, 0, 'protected method must NOT run — silent-bypass guard'); + }); + + it('unknown-provider branch: handler returns undefined — wrapper falls back to default 401', async () => { + const calls = []; + class MyResource extends MockResource { + async get() { + calls.push('called'); + return { status: 200, shouldNeverHappen: true }; + } + } + const context = { + // Session references a provider that's not in the registry — + // hits the `!providerData` branch in validateOAuthForRequest. + session: makeSession({ oauth: { provider: 'ghost-provider', accessToken: 'stale' } }), + }; + let handlerCalled = false; + const Wrapped = withOAuthValidation(MyResource, { + providers: mockProviders, + logger: mockLogger, + requireAuth: true, + onValidationError: () => { + handlerCalled = true; + // no return — returns undefined + }, + }); + + const instance = new Wrapped('x', context); + const result = await instance.get({ path: '/protected' }); + + assert.equal(handlerCalled, true, 'handler must be invoked on unknown-provider branch'); + assert.equal(result.status, 401, 'must fail closed on undefined handler return'); + assert.equal(result.body.error, 'Unauthorized'); + assert.match(result.body.message, /not configured/i); + assert.equal(calls.length, 0, 'protected method must NOT run — silent-bypass guard'); + }); + it('production-path session: callback sees full oauth data (not mutated by clearOAuthSession)', async () => { // `validateAndRefreshSession` calls `clearOAuthSession` as a // side effect before returning `{valid: false}`. In the From 1cbfb469a6898d65640b3e9e403066678421a674 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Wed, 22 Apr 2026 22:52:20 -0700 Subject: [PATCH 14/15] test: pin 3-arg static dispatch shape (target, data, request) Harper's REST dispatcher uses 2-arg (target, request) for GET/DELETE and 3-arg (target, data, request) for POST/PUT/PATCH. The wrapper extracts the request from args[args.length - 1] so both shapes work, but every existing static-dispatch test hit the 2-arg GET shape only. A future refactor that switched extraction to args[1] ("always the second arg") would silently mis-route 3-arg verbs without any test failing. New test calls Wrapped.post with the real (target, data, request) shape, asserts auth enforcement against the request arg and body survival in the data arg. Co-Authored-By: Claude Opus 4.7 (1M context) --- test/lib/withOAuthValidation.test.js | 50 ++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/test/lib/withOAuthValidation.test.js b/test/lib/withOAuthValidation.test.js index 7df2b10..0ff8321 100644 --- a/test/lib/withOAuthValidation.test.js +++ b/test/lib/withOAuthValidation.test.js @@ -641,6 +641,56 @@ describe('withOAuthValidation', () => { assert.equal(staticRan, false, 'user static must NOT run on auth failure'); }); + it('intercepts user-defined static post (3-arg shape: target, data, request)', async () => { + // Harper's REST dispatcher uses two distinct arg shapes: + // GET/DELETE → (target, request) — 2 args + // POST/PUT/PATCH → (target, data, request) — 3 args + // The wrapper picks the request up from `args[args.length - 1]` + // so both shapes work. The other static-dispatch tests only + // exercise the 2-arg GET shape, so a refactor that changes the + // extraction to `args[1]` ("always the second arg") would break + // 3-arg verbs silently — it'd read `data` into the context check + // and either 401 unconditionally (breaking requireAuth: false) or + // pass through without validation. Pin the 3-arg path directly. + let staticRan = false; + let seenData; + class PostResource extends StaticCapableResource { + static async post(_target, data, request) { + staticRan = true; + seenData = data; + return { + status: 201, + body: { user: request.session.oauthUser.email, created: data }, + }; + } + } + const Wrapped = withOAuthValidation(PostResource, { + providers: mockProviders, + logger: mockLogger, + requireAuth: true, + }); + + // Unauthenticated 3-arg call — validation must fire against args[2] + // (the real request), not args[1] (the body). + const denied = await Wrapped.post( + { path: '/items' }, + { name: 'new-item' }, + { session: { id: 'no-oauth' } } + ); + assert.equal(denied.status, 401, '3-arg dispatch must enforce OAuth against the request arg'); + assert.equal(staticRan, false, 'user static must NOT run on auth failure'); + + // Authenticated 3-arg call — user static runs and receives the body. + const ok = await Wrapped.post( + { path: '/items' }, + { name: 'new-item' }, + { session: makeSession() } + ); + assert.equal(ok.status, 201); + assert.equal(ok.body.user, 'alice@example.com'); + assert.deepEqual(seenData, { name: 'new-item' }, 'data arg (args[1]) must survive the wrapper'); + }); + it('validation runs only once per request across the static→instance chain', async () => { // Instance-method case: Harper calls the static (Resource // base's transactional dispatch), which creates an instance From 6e584e8bf6e9b5bfd6b361d8d5020df540b68ccc Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Wed, 22 Apr 2026 22:56:37 -0700 Subject: [PATCH 15/15] style: apply prettier to 3-arg static test Co-Authored-By: Claude Opus 4.7 (1M context) --- test/lib/withOAuthValidation.test.js | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/test/lib/withOAuthValidation.test.js b/test/lib/withOAuthValidation.test.js index 0ff8321..d09f5c9 100644 --- a/test/lib/withOAuthValidation.test.js +++ b/test/lib/withOAuthValidation.test.js @@ -672,20 +672,12 @@ describe('withOAuthValidation', () => { // Unauthenticated 3-arg call — validation must fire against args[2] // (the real request), not args[1] (the body). - const denied = await Wrapped.post( - { path: '/items' }, - { name: 'new-item' }, - { session: { id: 'no-oauth' } } - ); + const denied = await Wrapped.post({ path: '/items' }, { name: 'new-item' }, { session: { id: 'no-oauth' } }); assert.equal(denied.status, 401, '3-arg dispatch must enforce OAuth against the request arg'); assert.equal(staticRan, false, 'user static must NOT run on auth failure'); // Authenticated 3-arg call — user static runs and receives the body. - const ok = await Wrapped.post( - { path: '/items' }, - { name: 'new-item' }, - { session: makeSession() } - ); + const ok = await Wrapped.post({ path: '/items' }, { name: 'new-item' }, { session: makeSession() }); assert.equal(ok.status, 201); assert.equal(ok.body.user, 'alice@example.com'); assert.deepEqual(seenData, { name: 'new-item' }, 'data arg (args[1]) must survive the wrapper');