From 93ac5494145c9f3617c5299bf7d789722afc0ae3 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 20:30:39 +0000 Subject: [PATCH 1/2] fix(plugin-auth): single written definition for the auth basePath default DEFAULT_AUTH_BASE_PATH ('/api/v1/auth') is now the one place this literal is written. Before this, it existed independently at four sites: the AuthPlugin constructor default, two later re-derivations inside AuthPlugin (registerAuthRoutes, the OIDC discovery well-known alias), and AuthManager.configuredBasePath()'s own fallback. Every site evaluates byte-identically to before -- this collapses where the value is WRITTEN, not what any site evaluates to, and does not touch the configuredBasePath -> rootedBasePath -> getBasePath normalisation chain (#16399) or the published OAuth iss / RFC 8707 aud identifiers. Adds a source-scan regression test (auth-manager-base-path.test.ts) that fails if a future edit reintroduces a second hardcoded literal at any of the four sites -- the divergence class this card is about is otherwise unfalsifiable by construction on the live path (AuthPlugin always supplies basePath, so AuthManager's own fallback never runs there). Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj Co-authored-by: Claude --- .../16384-auth-base-path-single-definition.md | 27 +++++++ .../src/auth-manager-base-path.test.ts | 70 ++++++++++++++++++- .../plugins/plugin-auth/src/auth-manager.ts | 20 +++++- .../plugin-auth/src/auth-plugin.test.ts | 51 ++++++++++++++ .../plugins/plugin-auth/src/auth-plugin.ts | 9 ++- 5 files changed, 172 insertions(+), 5 deletions(-) create mode 100644 .changeset/16384-auth-base-path-single-definition.md diff --git a/.changeset/16384-auth-base-path-single-definition.md b/.changeset/16384-auth-base-path-single-definition.md new file mode 100644 index 0000000000..201fac462e --- /dev/null +++ b/.changeset/16384-auth-base-path-single-definition.md @@ -0,0 +1,27 @@ +--- +'@objectstack/plugin-auth': minor +--- + +fix(plugin-auth): give the auth `basePath` default a single written definition (#16384) + +`'/api/v1/auth'`, the shipped default for `AuthPlugin`'s `basePath` option, was +written independently at four sites: the `AuthPlugin` constructor, two later +re-derivations inside `AuthPlugin` (`registerAuthRoutes`, the OIDC discovery +`.well-known` alias), and `AuthManager.configuredBasePath()`'s own fallback. +Nothing was broken by the duplication — `AuthPlugin` always supplies `basePath` +to `AuthManager`, so the manager's copy was dead on the live path and +unfalsifiable by construction: no test could have caught one copy drifting from +the other three. + +The default now lives in exactly one place, `DEFAULT_AUTH_BASE_PATH` (exported +from `@objectstack/plugin-auth`, declared beside `readMcpServerEnabledEnv` in +`auth-manager.ts`); all four sites import it instead of retyping the literal. +Every site evaluates byte-identically to before — this is a consolidation of +where the value is *written*, not a change to what any site *evaluates to*, and +in particular does **not** touch `AuthManager`'s `configuredBasePath` → +`rootedBasePath` → `getBasePath` normalisation chain (#16399) or the published +OAuth `iss` / RFC 8707 `aud` identifiers those getters produce. + +This is additive and non-breaking — no existing call site's behaviour changes — +but it does add one new named export (`DEFAULT_AUTH_BASE_PATH`) to the +package's public surface, which is what makes this `minor` rather than `patch`. diff --git a/packages/plugins/plugin-auth/src/auth-manager-base-path.test.ts b/packages/plugins/plugin-auth/src/auth-manager-base-path.test.ts index 3839854f8d..92ef33758e 100644 --- a/packages/plugins/plugin-auth/src/auth-manager-base-path.test.ts +++ b/packages/plugins/plugin-auth/src/auth-manager-base-path.test.ts @@ -48,7 +48,10 @@ // that can hold a live better-auth and this manager at once. import { describe, it, expect } from 'vitest'; -import { AuthManager } from './auth-manager'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { resolve, dirname } from 'node:path'; +import { AuthManager, DEFAULT_AUTH_BASE_PATH } from './auth-manager'; import type { AuthManagerOptions } from './auth-manager'; const managerWith = (basePath?: unknown) => @@ -332,3 +335,68 @@ describe('#16399 one normalisation chain, and an MCP resource URL that is always } }); }); + +/** + * #16384 — the literal `'/api/v1/auth'` used to be written independently at + * FOUR sites: this file's own `configuredBasePath()` fallback, plus three in + * `auth-plugin.ts` (the constructor default, `registerAuthRoutes`'s fallback, + * `registerOidcDiscoveryRoutes`'s fallback). Nothing was BROKEN by that — see + * the card: `AuthPlugin` always supplies `basePath`, so this manager's own + * fallback is dead on the live path, unfalsifiable by construction (measured + * during #16025's round: mutating it to `/api/v7/elsewhere` was INERT). That + * is exactly why a VALUE-comparison test cannot catch a future divergence: + * two independently-typed copies of the same string are byte-identical today + * and would stay green right up until someone edited only one of them — and + * the edited one might be the manager's, which no test on the live path can + * observe at all. + * + * The fix is structural, not behavioural: `DEFAULT_AUTH_BASE_PATH` (declared + * above, next to `readMcpServerEnabledEnv`) is now the only place the literal + * is WRITTEN; every one of the four readers imports it instead of retyping + * it. What this describes pins is that structural fact, by reading the two + * files' own source text — a future edit that reintroduces a hardcoded + * default at any of the four sites (however well-intentioned: "it's just a + * string, inlining it is simpler") reappears here as a second literal and + * fails immediately, before it has any chance to drift from the other three. + */ +describe('#16384 the default base path is written in exactly ONE place', () => { + const HERE = dirname(fileURLToPath(import.meta.url)); + const AUTH_MANAGER_SRC = readFileSync(resolve(HERE, 'auth-manager.ts'), 'utf8'); + const AUTH_PLUGIN_SRC = readFileSync(resolve(HERE, 'auth-plugin.ts'), 'utf8'); + + /** + * Lines of source that carry the literal, MINUS comment lines (this file's + * docblocks use a leading `*` per continuation line; ordinary comments use + * `//`). A `@default` TSDoc annotation or a worked example inside a + * docblock is documentation, not a second definition, and stays out of + * this card's scope by design — the dispatch that opened it counts four + * CODE sites and two doc comments separately, and only the four move here. + */ + const codeLinesCarryingTheLiteral = (source: string): string[] => + source + .split('\n') + .map((line) => line.trim()) + .filter((line) => line.includes(`'${DEFAULT_AUTH_BASE_PATH}'`)) + .filter((line) => !line.startsWith('*') && !line.startsWith('//')); + + it('sanity: DEFAULT_AUTH_BASE_PATH is still the shipped literal this pin reasons about', () => { + // Hardcoded on purpose, not a re-read of the constant — if this constant's + // VALUE ever moves, this line (not the structural assertions below) is + // what should turn red. + expect(DEFAULT_AUTH_BASE_PATH).toBe('/api/v1/auth'); + }); + + it('auth-manager.ts writes the literal exactly once — the constant declaration itself', () => { + expect(codeLinesCarryingTheLiteral(AUTH_MANAGER_SRC)).toEqual([ + `export const DEFAULT_AUTH_BASE_PATH = '${DEFAULT_AUTH_BASE_PATH}';`, + ]); + }); + + it('auth-plugin.ts never writes the literal — every site imports the shared constant', () => { + expect(codeLinesCarryingTheLiteral(AUTH_PLUGIN_SRC)).toEqual([]); + // And it isn't simply missing the import either — the three collapsed + // sites (constructor default, registerAuthRoutes, registerOidcDiscoveryRoutes) + // must actually reference the shared binding. + expect(AUTH_PLUGIN_SRC.match(/DEFAULT_AUTH_BASE_PATH/g)?.length ?? 0).toBeGreaterThanOrEqual(4); + }); +}); diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index ae8b121cd6..f0362ccea1 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -305,6 +305,21 @@ function readSsoOnlyEnv(): boolean | undefined { return readBooleanEnv('OS_AUTH_SSO_ONLY'); } +/** + * [#16384] The auth plugin's shipped `basePath` default — the ONE place this + * literal is written. Before this card it existed independently at four + * sites: this file's own `configuredBasePath()` fallback below, plus three in + * `auth-plugin.ts` (the constructor default and two later re-derivations of + * "what if the caller cleared `basePath`?"). A future edit to one could drift + * from the other three silently — `configuredBasePath()`'s own fallback is + * unfalsifiable by construction on the live path (`AuthPlugin` always supplies + * `basePath`, per its constructor default below), so no runtime test could + * have caught that drift. Every one of the four readers now evaluates BYTE + * IDENTICALLY to before this constant existed — see `configuredBasePath()`'s + * docblock for the normalisation chain this does NOT touch. + */ +export const DEFAULT_AUTH_BASE_PATH = '/api/v1/auth'; + /** * Whether this runtime serves the HTTP MCP surface (`/api/v1/mcp`). * Delegates to the platform-wide decision point (`isMcpServerEnabled` in @@ -5667,6 +5682,9 @@ export class AuthManager { * configured. Unchanged from before this card — only the reading of it moved * here, so `getBasePath()` and this cannot drift apart by accident. * + * [#16384] The fallback is `DEFAULT_AUTH_BASE_PATH`, not a re-typed literal + * — see that constant's docblock above. + * * ## ⛔ Never normalise here * * better-auth stamps the OAuth access-token `iss` from `ctx.context.baseURL`, @@ -5692,7 +5710,7 @@ export class AuthManager { * #16399's decision, not this card's. */ private configuredBasePath(): string { - return this.config.basePath || '/api/v1/auth'; + return this.config.basePath || DEFAULT_AUTH_BASE_PATH; } /** diff --git a/packages/plugins/plugin-auth/src/auth-plugin.test.ts b/packages/plugins/plugin-auth/src/auth-plugin.test.ts index 4327cf8c19..8ff8861281 100644 --- a/packages/plugins/plugin-auth/src/auth-plugin.test.ts +++ b/packages/plugins/plugin-auth/src/auth-plugin.test.ts @@ -1240,6 +1240,57 @@ describe('AuthPlugin', () => { ); }); + // [#16384] Before this card, the DEFAULT base path was independently + // written at the constructor (line ~326) AND re-derived here in + // `registerAuthRoutes` (`this.options.basePath || '/api/v1/auth'`) — two + // literals that happened to agree, with nothing wiring them together. This + // is the sibling of "should use custom base path" above: that test never + // exercised the DEFAULT reaching the mount at all. Asserted against the + // external contract literal, not the `DEFAULT_AUTH_BASE_PATH` binding the + // implementation now shares — a typo in that constant must still fail + // this test. + it('should mount the default base path when none is configured', async () => { + const { hookFn, trigger } = createHookCapture(); + mockContext.hook = hookFn; + + authPlugin = new AuthPlugin({ + secret: 'test-secret-at-least-32-chars-long', + baseUrl: 'http://localhost:3000', + }); + + await authPlugin.init(mockContext); + + const mockRawApp = { + all: vi.fn(), + get: vi.fn(), + post: vi.fn(), + }; + + const mockHttpServer = { + post: vi.fn(), + get: vi.fn(), + put: vi.fn(), + delete: vi.fn(), + patch: vi.fn(), + use: vi.fn(), + getRawApp: vi.fn(() => mockRawApp), + }; + + // Cast, not the bare assignment the sibling test above uses: the bare + // form is TS2322 against `getService`'s generic signature — an + // ALREADY-tracked debt (`test-typecheck-debt.json`, 11 instances) this + // new test must not grow to 12. + mockContext.getService = vi.fn(() => mockHttpServer) as unknown as typeof mockContext.getService; + + await authPlugin.start(mockContext); + await trigger('kernel:ready'); + + expect(mockRawApp.all).toHaveBeenCalledWith( + '/api/v1/auth/*', + expect.any(Function) + ); + }); + it('should configure session options', async () => { authPlugin = new AuthPlugin({ secret: 'test-secret-at-least-32-chars-long', diff --git a/packages/plugins/plugin-auth/src/auth-plugin.ts b/packages/plugins/plugin-auth/src/auth-plugin.ts index 6564c23a47..b3f42b95c3 100644 --- a/packages/plugins/plugin-auth/src/auth-plugin.ts +++ b/packages/plugins/plugin-auth/src/auth-plugin.ts @@ -35,6 +35,9 @@ import { AuthManager, resolveOidcProviderEnabled, readMcpServerEnabledEnv, + // [#16384] The one place `'/api/v1/auth'` is written — see its docblock in + // auth-manager.ts. This file no longer carries an independent copy. + DEFAULT_AUTH_BASE_PATH, type AuthManagerOptions, } from './auth-manager.js'; import { @@ -323,7 +326,7 @@ export class AuthPlugin implements Plugin { constructor(options: AuthPluginOptions = {}) { this.options = { registerRoutes: true, - basePath: '/api/v1/auth', + basePath: DEFAULT_AUTH_BASE_PATH, ...options }; } @@ -2023,7 +2026,7 @@ export class AuthPlugin implements Plugin { private registerAuthRoutes(httpServer: IHttpServer, ctx: PluginContext): void { if (!this.authManager) return; - const basePath = this.options.basePath || '/api/v1/auth'; + const basePath = this.options.basePath || DEFAULT_AUTH_BASE_PATH; // Get raw Hono app to use native wildcard routing // Type assertion is safe here because we explicitly require Hono server as a dependency @@ -3096,7 +3099,7 @@ export class AuthPlugin implements Plugin { // (including every MCP client bootstrapping from protected-resource // metadata) request `/.well-known/oauth-authorization-server/api/v1/auth` // — alias it to the same document. - const basePath = (this.options.basePath ?? '/api/v1/auth').replace(/\/$/, ''); + const basePath = (this.options.basePath ?? DEFAULT_AUTH_BASE_PATH).replace(/\/$/, ''); rawApp.get(`/.well-known/oauth-authorization-server${basePath}`, (c: any) => withDiscoveryCache(authServerHandler, c.req.raw), ); From 04a25ccc92ac78b16ee79f4d99700db914a57116 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 12 Sep 2026 20:52:09 +0000 Subject: [PATCH 2/2] fix(scripts): teach check-auth-mount-ledger to resolve an imported basePath default deriveBasePath() read AuthPlugin's basePath fallback as a re-typed string literal at a fixed regex position. #16384 replaced that literal with an import of the package's single DEFAULT_AUTH_BASE_PATH definition, so the regex could no longer find a value and the gate refused (NOT MEASURED) instead of running its census. The fallback is still read from source rather than hardcoded here -- the same justification the function already carried -- just one hop further: when the fallback is a bare identifier instead of a literal, resolve it from the module that actually declares it (auth-manager.ts), which joins MOUNT_SOURCE and LEDGER_SOURCE as a third dispatch-gates-derivable input. In-place per this repo's bounded-fix exemption: same file MOUNT_SOURCE already names (so squarely dispatch-gates-derived for this same diff), mechanical (a second alternative in one regex plus a same-shape lookup), held by no other claim, and no new verification surface -- the two new self-test assertions extend an EXISTING battery's existing coverage of the existing "the base path is derived" invariant rather than opening a new one. Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj Co-authored-by: Claude --- scripts/check-auth-mount-ledger.mjs | 58 +++++++++++++++++++++++------ 1 file changed, 47 insertions(+), 11 deletions(-) diff --git a/scripts/check-auth-mount-ledger.mjs b/scripts/check-auth-mount-ledger.mjs index cd7b24ab4a..6d348f1f3d 100644 --- a/scripts/check-auth-mount-ledger.mjs +++ b/scripts/check-auth-mount-ledger.mjs @@ -137,7 +137,7 @@ import { isEntrypoint } from './invoked-as.mjs'; // must not red. A battery BELOW its floor means cases stopped running; the // remedy is to find what stopped registering. const SELF_TEST_BATTERIES = Object.freeze({ - 'The base path is DERIVED, and its absence is not an empty population.': 2, + 'The base path is DERIVED, and its absence is not an empty population.': 4, 'LOAD-BEARING NEGATIVE: a mount with an exact row is clean.': 1, 'LOAD-BEARING POSITIVE: a mount added with no row REDDENS, naming the route.': 2, 'THE RIGHT BOUNDARY, both directions. This is the defect class #10534 fell into.': 4, @@ -154,7 +154,7 @@ const SELF_TEST_BATTERIES = Object.freeze({ '#8435 remedy authority. PLACEMENT is pinned here, per-gate, because the': 4, 'Parse anchors: a moved anchor is a REFUSAL input, never an empty population.': 2, 'The escaped-quote shape the real notes use is measured, not truncated.': 1, - 'And the real inputs on disk are readable, so the anchors have not moved.': 2, + 'And the real inputs on disk are readable, so the anchors have not moved.': 3, }); // DELETING an entry silences that battery's floor exactly as effectively as @@ -168,11 +168,15 @@ const UNATTRIBUTED_BATTERY = '(no battery open)'; const ROOT = resolve(new URL('..', import.meta.url).pathname); -/** The two inputs. Module-scope literals, so `dispatch-gates` derives this - * family for a diff touching either of them (#10309: a gate nothing can - * derive is a gate that runs only when someone remembers it). */ +/** The three inputs. Module-scope literals, so `dispatch-gates` derives this + * family for a diff touching any of them (#10309: a gate nothing can + * derive is a gate that runs only when someone remembers it). + * [#16384] `AUTH_MANAGER_SOURCE` joined the set the day the `basePath` + * fallback in `MOUNT_SOURCE` stopped being a re-typed literal and became an + * identifier declared over there instead — see `deriveBasePath`. */ const MOUNT_SOURCE = 'packages/plugins/plugin-auth/src/auth-plugin.ts'; const LEDGER_SOURCE = 'packages/plugins/plugin-auth/src/auth-route-ledger.ts'; +const AUTH_MANAGER_SOURCE = 'packages/plugins/plugin-auth/src/auth-manager.ts'; export const EXIT_CLEAN = 0; export const EXIT_FINDINGS = 1; @@ -230,11 +234,26 @@ const LANE_VERBS = new Set(['all', 'use']); /** * The auth base path, DERIVED from the plugin rather than re-typed here, so a * rename moves this gate with it instead of silently emptying its population. + * + * [#16384] The fallback used to be a re-typed string literal at this exact + * spot; it is now the package's single `DEFAULT_AUTH_BASE_PATH` definition, + * imported from `auth-manager.ts` rather than retyped in the plugin. The + * literal-string match stays first (still the shape of every OTHER fallback + * this gate might meet); when the fallback is a bare identifier instead, + * resolve it the same way this function was already justified — one hop + * through the module that actually DECLARES it, never a second hardcoded + * copy — via `importedSource`, the text of the module the plugin imports the + * identifier from. */ -export function deriveBasePath(mountSource) { +export function deriveBasePath(mountSource, importedSource) { const masked = maskComments(mountSource); - const m = /basePath\s*=\s*this\.options\.basePath\s*(?:\|\||\?\?)\s*'([^']+)'/.exec(masked); - return m ? m[1] : null; + const m = + /basePath\s*=\s*this\.options\.basePath\s*(?:\|\||\?\?)\s*(?:'([^']+)'|([A-Za-z_$][\w$]*))/.exec(masked); + if (!m) return null; + if (m[1] != null) return m[1]; + if (importedSource == null) return null; + const decl = new RegExp(`export const ${m[2]}\\s*=\\s*'([^']+)'`).exec(maskComments(importedSource)); + return decl ? decl[1] : null; } /** @@ -627,6 +646,20 @@ function selfTest() { battery('The base path is DERIVED, and its absence is not an empty population.'); ok(deriveBasePath(FIXTURE_PREAMBLE) === FIXTURE_BASE, 'basePath was not derived from the plugin'); ok(deriveBasePath('const basePath = 42;') === null, 'a plugin with no derivable basePath did not refuse'); + // [#16384] The fallback is now a named identifier import in the real plugin, + // not a re-typed literal — resolved one hop through the module it is + // DECLARED in, never guessed at. + ok( + deriveBasePath( + "const basePath = this.options.basePath || DEFAULT_AUTH_BASE_PATH;\n", + "export const DEFAULT_AUTH_BASE_PATH = '/api/v1/auth';\n", + ) === FIXTURE_BASE, + 'an identifier fallback was not resolved through the imported module', + ); + ok( + deriveBasePath("const basePath = this.options.basePath || DEFAULT_AUTH_BASE_PATH;\n") === null, + 'an identifier fallback resolved to something without an imported module to read', + ); // -- LOAD-BEARING NEGATIVE: a mount with an exact row is clean. battery('LOAD-BEARING NEGATIVE: a mount with an exact row is clean.'); @@ -895,7 +928,7 @@ function selfTest() { // -- And the real inputs on disk are readable, so the anchors have not moved. battery('And the real inputs on disk are readable, so the anchors have not moved.'); - for (const rel of [MOUNT_SOURCE, LEDGER_SOURCE]) { + for (const rel of [MOUNT_SOURCE, LEDGER_SOURCE, AUTH_MANAGER_SOURCE]) { ok(existsSync(join(ROOT, rel)), `${rel} does not exist -- this gate's anchor moved`); } @@ -981,14 +1014,17 @@ function main() { const mountAbs = join(ROOT, MOUNT_SOURCE); const ledgerAbs = join(ROOT, LEDGER_SOURCE); + const authManagerAbs = join(ROOT, AUTH_MANAGER_SOURCE); if (!existsSync(mountAbs)) refuse(`${MOUNT_SOURCE} does not exist`); if (!existsSync(ledgerAbs)) refuse(`${LEDGER_SOURCE} does not exist`); + if (!existsSync(authManagerAbs)) refuse(`${AUTH_MANAGER_SOURCE} does not exist`); const mountText = readFileSync(mountAbs, 'utf8'); const ledgerText = readFileSync(ledgerAbs, 'utf8'); + const authManagerText = readFileSync(authManagerAbs, 'utf8'); - const basePath = deriveBasePath(mountText); - if (!basePath) refuse(`no \`basePath\` default could be derived from ${MOUNT_SOURCE}`); + const basePath = deriveBasePath(mountText, authManagerText); + if (!basePath) refuse(`no \`basePath\` default could be derived from ${MOUNT_SOURCE} (or its import from ${AUTH_MANAGER_SOURCE})`); const rows = parseLedgerRows(ledgerText); if (rows === null) refuse(`the AUTH_ROUTE_LEDGER anchor was not found in ${LEDGER_SOURCE}`);