diff --git a/AGENTS.md b/AGENTS.md index 36c2730..41f596e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -71,7 +71,7 @@ No central `errors.ts` — error classes are defined locally near where they're ## Where design decisions live -Non-trivial issues carry their design and status in the issue body itself, kept current as work lands — read the issue before re-deciding something it already settled (current examples: #231, #244, #212, #213, #264). `docs/mcp-oauth-conformance.md` maps every MCP requirement to its implementing code and test and must be updated in the same PR as any behavior change. See **[docs/maintaining.md](docs/maintaining.md)** for the release process, CI/review automation, and the Harper peer-dependency relationship. +Non-trivial issues carry their design and status in the issue body itself, kept current as work lands — read the issue before re-deciding something it already settled (current examples: #231, #244, #212, #213, #264). `docs/mcp-oauth-conformance.md` maps every MCP requirement to its implementing code and test and must be updated in the same PR as any behavior change. See **[docs/maintaining.md](docs/maintaining.md)** for the release process, CI/review automation, and the Harper peer-dependency relationship. See `src/lib/DESIGN.md` for invariants around issuer derivation (OIDC discovery, Azure multi-tenant adoption, #264) that aren't obvious from the code alone. ## Dependencies diff --git a/CHANGELOG.md b/CHANGELOG.md index 89ade7e..653fe44 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,19 +6,38 @@ All notable changes to `@harperfast/oauth` are documented here. The format is ba ### Added +- **The ID-token issuer is now derived automatically for almost every configuration, closing the gap #231 §4/#263 left open** (#264). Three mechanisms, all operator-invisible unless something goes wrong: + - **Okta custom authorization servers**: `configure()` (and `TenantManager.registerTenant`) now accept an optional `authServer` alongside `domain` (e.g. `authServer: 'default'`), deriving both the path-inclusive endpoints and the issuer (`https://{domain}/oauth2/{authServer}`) for a named custom authorization server — previously only the org authorization server's shortcut existed, and a custom AS required explicit endpoints with no derived issuer at all. + - **OIDC discovery** for any other statically configured, explicit-endpoint JWKS provider with no usable `issuer`: a bounded background attempt probes `.well-known/openid-configuration` at ancestor path-prefixes of `authorizationUrl`'s own origin (never cross-origin), and accepts a result only when its `issuer`, `authorization_endpoint`, `jwks_uri`, and `token_endpoint` all exactly match the configured values. Fire-and-forget — never blocks startup, and is scheduled only once a reload's provider registry is actually published, never for a build later discarded. The first login that needs it awaits the in-flight attempt with a 3-second bound; every other login during that window doesn't wait, per the issue's own design. A successful result is cached and logged (`issuer derived via discovery: X — add issuer: X to pin it`) for the process lifetime; a failure is retried at most once every 5 minutes, and only via a fresh reload — never from a login. A dynamically resolved (`onResolveProvider`) provider never fetches on its own, but can read an already-cached result from a static provider sharing the same endpoints. + - **Azure multi-tenant adoption**: pinning `issuer` to a single tenant's issuer URI (either form — the v2 `https://login.microsoftonline.com//v2.0`, or the older v1 `https://sts.windows.net//`, matching whichever form `authorizationUrl` actually issues) alongside a multi-tenant `tenantId`/`authorizationUrl` (`common`/`organizations`/`consumers`) turns the provider single-tenant for that **one** tenant — verification is redirected to that tenant's own, non-shared JWKS endpoint (never Microsoft's shared, tenant-independent key set) and that tenant's issuer is enforced exactly like an ordinary single-tenant Azure config, so a login from any other tenant now fails ID-token verification outright, not just adoption. Unpinned, a multi-tenant provider (`authorizationUrl` and `jwksUri` both naming the shared alias — the normal shape, including the Azure preset's default) is completely unaffected (byte-identical to today: `issuerValidated` always `false`). **One exotic explicit-endpoint shape is the exception:** an `authorizationUrl` naming a shared alias (or a different tenant) paired with a `jwksUri` naming one specific real tenant GUID directly — previously had no issuer check at all (any tenant whose token verified against those keys could sign in); it now derives and enforces that one tenant's issuer the same way a pin would, so every other tenant's login now fails ID-token verification outright. A startup warning names the provider and both tenants when this happens (for a dynamically resolved provider, only when its hook returns a raw, unbuilt config for this plugin to build — a hook that calls `buildProviderConfig` itself derives the issuer, and would need to pass its own logger, before this plugin ever sees the result), with the fix (point `authorizationUrl`/`jwksUri` at the same tenant, or pin `issuer` explicitly to make the restriction obvious in your own config). Pinning an array of issuers on a multi-tenant provider is rejected at startup — it would make every listed tenant adoption-eligible through one provider, reopening a cross-tenant identity question that belongs to account-level `(iss, sub)` binding (#231 §3), not issuer validation; configure a separate provider entry per tenant instead. + - A new startup warning (not an error) for a statically configured provider with `fetchEmail: true` against a Microsoft Graph UserInfo endpoint whose `scope` omits `profile` — closing the silent-`oid`-loss gap #263 documented but didn't warn about. - **`onResolveEmail` hook: choose which of GitHub's several verified emails becomes the login identity** (#228). GitHub's `/user/emails` can return more than one address with independent `verified`/`primary` flags; the plugin previously picked one deterministically (public profile email, else `primary`) and discarded the rest with no way for an application to choose differently. Registering `onResolveEmail(candidates, provider)` now lets an application pick among them — e.g. "prefer the address on my company domain" — while the plugin independently validates the result against the exact set GitHub's authenticated fetch returned: the hook may only ever resolve to an address with `verified: true`, never an arbitrary or unverified one, so this cannot become a way to adopt a different account than the one the authenticated fetch actually confirmed. An invalid pick, a thrown error, or a hook that doesn't settle within 5 seconds fails the login rather than silently falling back to the default — a silent fallback could otherwise establish a session for a different account than the one the hook was trying to reach. **Registering the hook is opt-in**; the built-in default it sits above is not — see the matching entry below for the behavior change that applies regardless of whether this hook is used. See [Lifecycle Hooks: `onResolveEmail`](docs/lifecycle-hooks.md#onresolveemail). Note GitHub's default `usernameClaim` is `login` (the handle), not `email` — set `usernameClaim: 'email'` for the resolved address to also become `session.user`. ### Changed +- **The #231 §4 startup hard-fail is relaxed: an `https` explicit-endpoint JWKS provider with no usable `issuer` now boots and resolves it via OIDC discovery in the background, instead of failing to start** (#264). Only a config that's wrong _independent_ of the network — no `authorizationUrl`, or a non-`https` one — still fails fast at startup, naming the provider and the `issuer` key. **Upgrade note:** a provider that previously failed to start for this reason now boots; while discovery is pending, or if it never resolves, logins through it behave exactly as any other "signed but not issuer-validated" login already does (see the Account-Adoption Gate docs) — an existing-account login is denied, a new one is quarantined. This is not a trust downgrade: `issuerValidated` simply becomes `true` more often, once discovery (or an operator's own `issuer` pin) resolves it. +- **Azure's `configure()` no longer sets a literal, incorrect `issuer` for the `common`/`organizations`/`consumers` tenant aliases** (#264). Previously, explicitly setting `tenantId: 'common'` (redundant with the default, but sometimes written for clarity) produced a literal `issuer: 'https://login.microsoftonline.com/common/v2.0'` — a value no real multi-tenant token's `iss` could ever equal, since a real token's `iss` always names its actual tenant GUID. This was latent (Azure was previously excluded from the #231 §4 check entirely) but is now fixed outright: these aliases leave `issuer` unset unless the operator also pins one (see Added, above). +- **`buildProviderConfig`'s `configure()` dispatch now switches on the resolved preset's own `provider` field, not the final `config.provider` string** (#264). An explicit `provider: 'microsoft'` option previously bypassed `configure()` entirely — `getProvider('microsoft')` correctly resolved the Azure preset, but the dispatch switch compared against the literal string `'microsoft'`, which never matched `'azure'` — so a `provider: 'microsoft'` + `tenantId: ` config silently kept the default `/common` endpoints with no derived issuer. `provider: 'microsoft'` now derives endpoints and issuer exactly like `provider: 'azure'`. +- **An explicit `issuer` is no longer silently overwritten by a `domain`/`tenantId` shortcut** (#264). Previously, `configure()`'s returned `issuer` always overwrote an operator-pinned one via an unconditional `Object.assign`; an operator who paired `domain`/`tenantId` with an explicit `issuer` (e.g. to override a quirky custom authorization server, or to pin a multi-tenant Azure provider to one tenant) had that pin silently discarded — this affected Azure too: `providers/azure.ts` returned an `issuer` for every `tenantId`, including the multi-tenant aliases, and that return value was applied the same unconditional way. Only `issuer` is protected this way — every other `configure()`-returned field (the endpoint URLs) keeps today's unconditional-overwrite behavior, so a shortcut's endpoints stay mutually coherent rather than mixing a stale explicit endpoint with freshly-derived others. **Azure upgrade note:** a pin now fails in two independent ways — either is enough to take the provider down the same paths described below: + - **Naming a different tenant.** A `tenantId` paired with an explicit `issuer` that doesn't name that same tenant now fails instead of booting with the (previously silently discarded) stale pin — for example a pin left over after switching `tenantId` through an env var, the `.../common/v2.0` or `{tenantid}` placeholder value copied from the docs, or a pin missing its `/v2.0` suffix. Message: `OAuth provider '' (azure) has a jwksUri for tenant '' but an explicit 'issuer' naming a different tenant ... ()`. + - **Naming the right tenant, but the wrong issuer FORM.** Azure has two issuer forms — v2 (`https://login.microsoftonline.com//v2.0`) and the older v1 (`https://sts.windows.net//`) — and which one a real token carries depends on `authorizationUrl` (the `tenantId` preset shortcut always generates a v2 `authorizationUrl`, so a v1 pin — or a same-tenant `[v2, v1]` array — on a preset-derived config now fails even though it names the correct tenant). This is new: previously the preset's `configure()` silently overwrote any pin, so a v1 pin on a v2-authorize config (e.g. one copied from a Graph access token's `iss`, which is v1) booted and worked, just without adoption. Message: `OAuth provider '' (azure) pins 'issuer' to '', which names the right tenant but the wrong Azure issuer form for its 'authorizationUrl' ('') ...`. + + For a statically declared provider (`providers:` in config), that provider is skipped at startup (logged as an error) rather than aborting every other declared provider's startup — see the Fixed entry below. A **dynamically resolved** provider (`onResolveProvider` returning a raw Azure config with either kind of mismatch) fails differently, since there's no startup to abort: the resolving request gets a `500` (`Failed to resolve OAuth provider`), and — for a login already in session — the automatic session-validation middleware treats the unresolvable provider the same as a deleted one and clears that session on its next request. Nothing shows at startup for this case; look for `OAuth provider '' (dynamic resolution` in the log (`, session validation` is appended for the middleware's own copy of this check) — the rest of that line is one of the two messages above. A failed dynamic resolution of this kind is cached for 30 seconds so a bad config doesn't re-run the resolve hook on every single request; a corrected config takes effect on the next request after that window, no restart needed. + - **GitHub logins with several verified emails now match an existing Harper account before falling back to the profile-or-primary default** (#228). Previously, a user whose GitHub profile/primary address differed from the address their existing Harper account was created under could never reach that account through this plugin — only the exact profile-or-primary pick. **Only applies when `usernameClaim: 'email'` is configured** — `hdb_user` isn't keyed by email under any other username claim (including GitHub's default, `login`), so this step is skipped entirely with no reads and no behavior change for those deployments. When it applies: the plugin checks each VERIFIED address from GitHub's authenticated `/user/emails` fetch against `hdb_user`, using the exact same exact-match lookup the account-adoption gate itself already uses (no normalization): if exactly one matches an existing account, that address is used; if none match, today's original default (public profile email, else `primary`) applies unchanged. A registered [`onResolveEmail` hook](docs/lifecycle-hooks.md#onresolveemail) always overrides this default when it expresses a preference, and its `candidates` now also carry a `profile: boolean` field (which of them is the public profile address) so a hook can reconstruct the exact original default if needed. **New failure modes:** two or more verified addresses matching DIFFERENT existing accounts refuses the login (`reason=email_ambiguous`) rather than guessing — UNLESS the current profile-or-primary address is one of the matches, in which case that one wins (it reproduces exactly what every login already resolves to today, not a guess between strangers — this covers a user who changed their GitHub primary address, where both the old and new address may have existing accounts from when each was primary); the log names the match count, never the addresses. An `hdb_user` read failure during this check refuses with a retryable `reason=email_lookup_failed` rather than treating the failure as "no match". Reads `hdb_user` once per verified address, skipping entirely only when `usernameClaim: 'email'` isn't set, there are no verified addresses, or the lone verified address already IS the verified profile-or-primary default pick (nothing to disambiguate) — an unverified profile/primary address with exactly one (different) verified address still costs one read, since that's the #228 case itself. See [Account-Adoption Gate: GitHub default email](docs/configuration.md#github-default-email-when-there-are-several-verified-addresses). **Upgrade note:** with `usernameClaim: 'email'`, a GitHub user whose profile/primary address has no Harper account of its own, but who has another verified address matching exactly one existing Harper account B, will now log into B the first time they log in after upgrading. A user whose profile/primary address is itself verified and already has an account stays on it, even when another verified address also matches an account — the default-pick exception above keeps that login unchanged. An unverified profile/primary address isn't part of this matching at all: regardless of whether it happens to have an `hdb_user` row of its own, the login still resolves through whichever other verified address matches, same as if that account didn't exist. This is the intended fix for #228. To opt back out, register `onResolveEmail` returning `const pick = candidates.find((c) => c.profile) ?? candidates.find((c) => c.primary); return pick?.verified ? pick.email : undefined;` — this reproduces the previous pick whenever that address was verified; the `pick?.verified` guard matters because `onResolveEmail` can only ever return a verified address, so returning an unverified `pick` would fail the login outright (`reason=email_selection`) instead of reproducing it. A GitHub account whose profile/primary address is itself unverified (login still succeeds today, just without account adoption) has no hook-based way back to that exact unverified pick — only this behavior change itself; the snippet above returns `undefined` for that case, which defers to the built-in default rather than failing the login. Rolling upgrades and multi-node clusters: see the docs link above for replication-lag and per-node skew during the upgrade window. - **MCP access tokens now carry `typ: at+jwt` in their JWT header** (RFC 9068 §2.1), closing a gap `docs/mcp-oauth-conformance.md` tracked from the 2026-07-28 conformance pass (#202). `withMCPAuth`'s production verifier (`verifyAccessTokenWithKeySet`) now enforces the header (RFC 9068 §4: the resource server MUST verify `typ`), accepting `at+jwt` and the prior default `JWT` — a token already minted with `typ: JWT` keeps verifying rather than being invalidated outright; since every token minted from here on carries `typ: at+jwt`, the `JWT` allowance is self-limiting (it stops mattering once any pre-upgrade token could have expired) but isn't itself time-boxed in code — tracked for removal in #258. **Compatibility:** a consumer that pinned `typ: JWT` on these tokens must accept `at+jwt` going forward. ### Fixed +- **An unpinned Azure shared-alias provider on the older v1 keys shape (`.../common/discovery/keys`, no `v2.0` — Azure's actual v1 metadata `jwks_uri`) is recognized and excluded the same way the v2 alias shape already is, instead of falling through unrecognized** (#264/#271). Before this fix, this exact shape had no special handling at all: it fell into background OIDC discovery (which fails for it — Azure's v1 discovery document's `sts.windows.net` issuer can never equal a probed `login.microsoftonline.com` ancestor), and the resulting log told the operator to pin `issuer` explicitly — landing them on a config that binds a pin to Azure's shared, tenant-independent v1 key pool, which this plugin's whole Azure design (see `src/lib/DESIGN.md`) exists to prevent. Unpinned, this shape is now byte-identical to `main`, exactly like the v2 alias. **Pinning `issuer` on this shape now fails at startup instead** (for a statically declared provider, skipped with a logged error, same as any other invalid Azure pin — see the entries below; for a dynamically resolved one, the usual 500 + cooldown), naming the fix: point `jwksUri` at the tenant's own keys endpoint (v1 or v2) instead of the shared one. +- **The authorizationUrl/jwksUri Azure tenant-mismatch startup warning (#264/#271, see Added above) no longer false-positives on a verified tenant DOMAIN name.** `authorizationUrl`'s tenant segment can legitimately be a domain Azure accepts in place of a GUID (e.g. `contoso.onmicrosoft.com`), which is never equal to `jwksUri`'s GUID segment as a plain string — the warning previously fired for every such config, even though nothing is actually misconfigured and there's no offline way to confirm the domain and GUID name the same tenant. The warning now only compares `authorizationUrl`'s segment against `jwksUri`'s tenant when it's itself an alias (`common`/`organizations`/`consumers`) or another real tenant GUID; a domain segment is silently left alone, same as before #264. The same fix also stops a false positive on the one alias/GUID pair that _does_ name the same tenant: `authorizationUrl`'s `consumers` alias and `jwksUri`'s fixed `AZURE_CONSUMERS_TENANT_ID` GUID. +- **The authorizationUrl/jwksUri Azure tenant-mismatch warning now actually reaches the log for a dynamically resolved (`onResolveProvider`) provider whose hook returns a raw, unbuilt config.** Both dynamic-resolution call sites (`resource.ts`'s REST GET handler and `index.ts`'s session-validation middleware) call `buildProviderConfig` without a `logger` — this warning never fired for a dynamic provider at all, regardless of how the config was built. They're passed a logger now, deduped per provider (`DynamicProviderCache.wrapLoggerForDynamicResolution`): the first distinct warning message for a given provider name logs; a repeat of the exact same message for that provider is dropped, so a misconfigured dynamic provider warns once rather than on every request (or every TTL-refresh with `cacheDynamicProviders` set to a short window or `false`). `clear()` resets this dedup state along with the rest of the cache's per-provider tracking. - **`/login` now redirects with a reason code when CSRF state storage fails, instead of letting the raw error escape** (#227): `handleLogin` awaited `generateCSRFToken` → `csrfTokenManager.set()` with no error handling, so a table-write failure (e.g. a wedged write queue, HarperFast/harper#2450) surfaced to the browser as core's raw JSON error response instead of the app's sign-in page. `handleCallback` already follows this pattern for its own failures (e.g. the account-adoption gate's `account_lookup_failed`) rather than letting a raw error escape; `/login` now matches it: a `generateCSRFToken` rejection is logged server-side and redirects to `originalUrl` (sanitized by the same `buildErrorRedirect` the other reason codes use) with `error=server_error&reason=state_storage`, with no `Set-Cookie` (the browser secret is only useful once a state token exists). Sign-in still can't proceed while storage is down — this only changes where the failure lands. +- **A provider with an invalid Azure `issuer` pin (see the Changed entry above) is now skipped with a logged error at startup, instead of aborting every other declared provider** (#264). `initializeProviders` calls `buildProviderConfig` for each declared provider in one loop with no per-provider try/catch; a throw from one provider's config (previously only the pre-existing #231 §4 "issuer required" and `redirectUri` checks, now also this new Azure-pin validation) propagated out of the loop entirely, so a reload or boot carrying one misconfigured Azure provider failed to start every other provider too — inconsistent with #259/#260's own "skip the bad one, keep the others" precedent for every other kind of per-provider misconfiguration. Only this new failure mode is downgraded this way; #231 §4's issuer-required check and other `buildProviderConfig` failures are unrelated to one provider's own pin and keep today's fail-the-whole-reload behavior. +- **A dynamically resolved (`onResolveProvider`) provider with the same invalid Azure `issuer` pin no longer re-runs the resolve hook (and rebuilds, and re-throws) on every single request for that provider** (#264). Both of the dynamic-resolution call sites — the REST GET handler (`resource.ts`) and the automatic session-validation middleware (`index.ts`) — now cache the failure for a fixed 30 seconds (`DynamicProviderCache.recordAzurePinFailure`/`getAzurePinFailure`, independent of the operator-configured success TTL, which can be much longer or forever and would be the wrong duration to also gate failure recovery) and log the same specific tenant-mismatch message instead of a generic "Error resolving provider". A corrected config takes effect on the next request after that window elapses, with no restart or manual cache-clear needed. - **A failed OAuth session clear is no longer reported as a completed logout or invalidation** (#265, #266). `clearOAuthSession` previously returned as if it had succeeded whenever an _existing_ session's `.update` wasn't callable — silently leaving the stored session's `oauth`/`user` in place while the caller believed it was cleared. It now returns `false` in that case, when `.update` itself rejects, or when the store write succeeds but the in-memory field clear that follows throws (e.g. a non-configurable session object) — any of those, or a logger throwing while reporting one, is reported as `false` rather than escaping as a rejection — while still keeping its intentional quiet success for the no-session and anonymous-logout (no `id`) cases. `handleLogout` now returns `503` with `Cache-Control: no-store` instead of a `200` success body on a failed clear, so a client never treats a failed logout as done and can safely retry. The automatic session-validation HTTP middleware reacts the same way at every point it triggers a clear itself (an unrecognized provider config, or a `validateAndRefreshSession` result reporting the same failure): a request whose invalidation couldn't persist now gets the same `503`/`no-store` instead of continuing to serve it as the stale, supposedly-revoked identity. `withOAuthValidation`'s `requireAuth: false` pass-through is unaffected by design: it already proceeds on any invalidation, persisted or not. Separately, the token-refresh-failure path's own second `clearOAuthSession` call (used to log out an expired session whose refresh just failed) could itself reject — e.g. if the session store write failed — escaping as an unhandled rejection instead of resolving `validateAndRefreshSession`'s promise; that call is now wrapped so any failure is logged (without token values) and the function still resolves to a controlled `{ valid: false }`. Both bugs were surfaced while tracing #213's design; neither needed #213's core change. -- **A statically configured JWKS-enabled provider now requires a usable `issuer`, checked at startup** (#231 §4): a custom OIDC/JWKS provider configured with explicit endpoints — e.g. an Okta custom authorization server, or a `generic` provider with `jwksUri` set — but no `issuer` left `issuerValidated` always `false` (see 2.6.0's account-adoption gate). Its ID token signature still verified, but a hookless login that should have adopted an existing Harper account was silently denied, with nothing in the logs pointing at `issuer`. Config building for a provider declared in `providers:` now throws at startup, naming the provider and the `issuer` key, whenever it has `jwksUri` set but no usable `issuer` (empty string, `null`, or an array with no non-empty entry) — but only once that provider has also passed #259/#260's configured-ness check, so an unconfigured or half-configured provider is still just skipped, never evaluated for `issuer`. Okta/Auth0's `domain` shortcut (and Azure's `tenantId` shortcut) already derive `issuer` for you — this only affects a provider bypassing that shortcut with explicit endpoints. Azure's multi-tenant `/common` default is unaffected: it ships `issuer: null` deliberately (no single issuer exists for `/common`) and remains not-adoption-eligible exactly as before, not a startup error. A dynamically resolved provider (`onResolveProvider`) is also unaffected — enforcing this on the request path would repeatedly fail every request for a misconfigured tenant instead of just leaving it non-adoption-eligible, so the check runs only for statically declared providers. **Upgrade note:** an instance with a statically declared, fully configured provider that sets `jwksUri` and no `issuer` — Okta/Auth0/`generic` only, not Azure — now fails to start; the same config applied via a live reload is rejected and the previous configuration keeps serving. Set `issuer` explicitly, or use the `domain`/`tenantId` shortcut. Single-tenant Azure configs with explicit endpoints and no `issuer`, and deriving `issuer` from OIDC discovery, are tracked separately in #264. -- **UserInfo `sub` must match the ID token's `sub` on the `fetchEmail` fallback path** (#231 §5a, OIDC Core 5.3.2/5.3.4): when an ID token lacks an `email` claim and `fetchEmail` is configured, the plugin fetches the UserInfo endpoint to resolve it. A UserInfo response whose `sub` differs from (or omits, which OIDC Core 5.3.2 also disallows) the verified ID token's `sub` describes an uncorrelated subject — previously its fields (email, name, and any other claim not present on the ID token) were still merged into the login. The fetched UserInfo is now discarded entirely on a `sub` mismatch or absence, falling back to the ID token's own claims only, exactly as on a UserInfo fetch failure. This was not an adoption-gate bypass (that merge was always tagged `unauthenticated`, which the gate already denies for adoption), but it could still attach a different subject's profile fields to the session. Microsoft Graph's non-OIDC `/v1.0/me` UserInfo shape (the Azure preset's default) is correlated by `id`/`oid` instead of `sub`, so the Azure preset's `fetchEmail: true` behavior is unaffected — provided the id token carries `oid`, which requires the `profile` scope (the preset's default `scope` includes it; a config that overrides `scope` to drop `profile` loses `oid` and falls back to the discard above). +- **A statically configured JWKS-enabled provider now requires a usable `issuer`, checked at startup** (#231 §4): a custom OIDC/JWKS provider configured with explicit endpoints — e.g. an Okta custom authorization server, or a `generic` provider with `jwksUri` set — but no `issuer` left `issuerValidated` always `false` (see 2.6.0's account-adoption gate). Its ID token signature still verified, but a hookless login that should have adopted an existing Harper account was silently denied, with nothing in the logs pointing at `issuer`. Config building for a provider declared in `providers:` now throws at startup, naming the provider and the `issuer` key, whenever it has `jwksUri` set but no usable `issuer` (empty string, `null`, or an array with no non-empty entry) — but only once that provider has also passed #259/#260's configured-ness check, so an unconfigured or half-configured provider is still just skipped, never evaluated for `issuer`. Okta/Auth0's `domain` shortcut (and Azure's `tenantId` shortcut) already derive `issuer` for you — this only affects a provider bypassing that shortcut with explicit endpoints. Azure's multi-tenant `/common` default is unaffected: it ships `issuer: null` deliberately (no single issuer exists for `/common`) and remains not-adoption-eligible exactly as before, not a startup error. A dynamically resolved provider (`onResolveProvider`) is also unaffected — enforcing this on the request path would repeatedly fail every request for a misconfigured tenant instead of just leaving it non-adoption-eligible, so the check runs only for statically declared providers. **Upgrade note:** an instance with a statically declared, fully configured provider that sets `jwksUri` and no `issuer` — Okta/Auth0/`generic` only, not Azure — now fails to start; the same config applied via a live reload is rejected and the previous configuration keeps serving. Set `issuer` explicitly, or use the `domain`/`tenantId` shortcut. **Superseded by #264, below (same release):** an `https` `authorizationUrl` no longer hard-fails here at all — it derives `issuer` via background OIDC discovery instead; only a non-`https` `authorizationUrl` still fails fast as described above. +- **UserInfo `sub` must match the ID token's `sub` on the `fetchEmail` fallback path** (#231 §5a, OIDC Core 5.3.2/5.3.4): when an ID token lacks an `email` claim and `fetchEmail` is configured, the plugin fetches the UserInfo endpoint to resolve it. A UserInfo response whose `sub` differs from (or omits, which OIDC Core 5.3.2 also disallows) the verified ID token's `sub` describes an uncorrelated subject — previously its fields (email, name, and any other claim not present on the ID token) were still merged into the login. The fetched UserInfo is now discarded entirely on a `sub` mismatch or absence, falling back to the ID token's own claims only, exactly as on a UserInfo fetch failure. This was not an adoption-gate bypass (that merge was always tagged `unauthenticated`, which the gate already denies for adoption), but it could still attach a different subject's profile fields to the session. Microsoft Graph's non-OIDC `/v1.0/me` UserInfo shape (the Azure preset's default) is correlated by `id`/`oid` instead of `sub`, so the Azure preset's `fetchEmail: true` behavior is unaffected — provided the id token carries `oid`, which requires the `profile` scope (the preset's default `scope` includes it; a config that overrides `scope` to drop `profile` loses `oid` and falls back to the discard above — #264, below, adds a startup warning for exactly this combination). - **A transient `hdb_user` read error during account adoption now quarantines the login instead of denying it, unless `allowUnverifiedClaimInheritance` is enabled** (#231 §5b): the adoption gate's existence check previously treated a lookup error the same as a confirmed existing account — denying a login with an unverified claim, or, with the escape hatch enabled, adopting it via the hatch. With the escape hatch disabled (the default), the quarantine principal it would otherwise use is unpredictable and can never match any `hdb_user` name, confirmed-existing or not, so quarantining on an unknown lookup result is exactly as secure as denying — and more available, since a transient error on a login that was never going to adopt anything no longer fails it outright. With the escape hatch **enabled**, quarantining would be the wrong outcome — if the account does exist, the hatch should adopt it and inherit its role, and a quarantined session would stick roleless until the next login — so a lookup error instead fails the login with a retryable error (`error=server_error&reason=account_lookup_failed`, logged server-side), leaving no session established, so a retry once the storage fault clears gets the real adopt-vs-quarantine decision. A trusted claim (signed-oidc / github-authenticated) is unaffected either way. **Upgrade note:** with the escape hatch enabled, a login that hit a transient `hdb_user` read error previously still succeeded (adopted via the hatch); it now fails with the retryable error above instead and must be retried once the storage fault clears. - **Disabling `allowUnverifiedClaimInheritance` is applied independently of provider (re)initialization on a live config reload** (#231 §5c): a reload snapshot that both disabled the escape hatch and had an unrelated provider config error (e.g. the new issuer check above) previously aborted before the escape-hatch flag was ever re-applied, leaving the _previous_ snapshot's value — possibly still enabled — in effect, contrary to the operator's intent in the snapshot that failed. A _disabling_ value is now applied to the running configuration immediately, before any other reload validation (including MCP config checks) can throw. An _enabling_ value still waits for the reload to fully succeed, so a rejected reload can never turn the hatch on for the still-serving (old) providers. - **`_emailProvenance` is no longer stamped `signed-oidc` on the no-JWKS fallback** (#231 §5d): `OAuthProvider.getUserInfo` labeled any present `idTokenClaims` `signed-oidc` regardless of whether the ID token's signature was actually verified via JWKS — including the no-JWKS fallback, which only decodes claims. The account-adoption gate was not exploitable by this (it independently re-checks `signatureVerified`/`issuerValidated` before trusting `signed-oidc`), but the label overstated its own contract for any other consumer. `getUserInfo` now takes the verified-signature flag and only stamps `signed-oidc` when it is `true`. diff --git a/docs/configuration.md b/docs/configuration.md index 80d369e..51ab8c6 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -389,9 +389,9 @@ There are exactly **two** trusted email sources. A login may adopt an existing a **Source 1 — JWKS-signed OIDC id token (`signed-oidc`)**: - The email is a claim in a JWKS-signature-verified id token. -- The token's `iss` was validated against the provider's **expected issuer** (`config.issuer`). Providers without a known issuer — Azure `/common` (multi-tenant) — are not trusted for adoption. +- The token's `iss` was validated against the provider's **expected issuer** (`config.issuer`). An Azure `/common`/`/organizations`/`consumers` provider with no `issuer` pinned is not trusted for adoption — see [Multi-Tenant Apps and Account Adoption](./providers.md#multi-tenant-apps-and-account-adoption) for how to make one specific tenant adoption-eligible. - Any other JWKS-enabled provider (Okta, Auth0, or a `generic` OIDC config) that sets `jwksUri` but has no usable `issuer` fails at startup instead, naming the provider and the `issuer` key — this is almost always a bypassed shortcut (Okta/Auth0's `domain` option, which derives `issuer` for you) rather than an intentionally issuer-less provider. Set `issuer` explicitly, or use `domain`/`tenantId`. + **You almost never need to set `issuer` yourself.** Presets derive it from `domain`/`tenantId`/`authServer` whenever it's knowable, and any other JWKS-enabled provider (Okta, Auth0, or a `generic` OIDC config) that sets `jwksUri` with explicit endpoints but no usable `issuer` derives it automatically in the background via [OIDC discovery](./providers.md#issuer-is-usually-unnecessary) — this never blocks startup, and the first login that needs it waits only briefly for an in-flight attempt; every other login proceeds without waiting. Only a config that's wrong _independent_ of the network — no `authorizationUrl`, or a non-`https` one — still fails fast at startup, naming the provider and the `issuer` key. - `email_verified === true` for the **standard `email` claim**. Providers that use a custom `emailClaim` cannot earn adoption trust because `email_verified` only attests the standard claim. - The verified email equals the resolved username (`usernameClaim === 'email'`). @@ -434,21 +434,23 @@ Only VERIFIED addresses from GitHub's authenticated fetch are ever candidates fo **Denied by default:** -| Scenario | Reason | -| ------------------------------------------------------------------- | ---------------------------------------------------- | -| GitHub `login` claim → existing account | `login` ≠ email; username and email differ | -| Okta `preferred_username` ≠ email → existing account | username claim differs from email | -| `email_verified` absent or false | Provider has not attested the email | -| Custom `emailClaim` (non-`email` claim used as identity) | `email_verified` does not attest the custom claim | -| Unsigned / unverified id token | Signature not verified | -| JWKS-signed token from issuer-less Azure `/common` (multi-tenant) | Issuer cannot be validated; nOAuth class attack | -| Azure id token without `email_verified` | No verified-email attestation available | -| Plain UserInfo response (no id token) | No authenticated binding to the identity | -| GitHub `/user/emails` fetch failed or returned non-OK | Email not confirmed by an authenticated call | -| GitHub `/user/emails` returned `verified: false` | Email not verified by GitHub | -| Non-GitHub provider with custom `getUserInfo` adapter | `github-authenticated` requires `provider=github` | -| Custom adapter returning `_emailProvenance: 'github-authenticated'` | Provenance is plugin-assigned; adapter value ignored | -| `fetchEmail: true` resolved email from UserInfo (no signed token) | Email is present but provenance is unauthenticated | +| Scenario | Reason | +| ------------------------------------------------------------------------ | ---------------------------------------------------------------------------------------------------------- | +| GitHub `login` claim → existing account | `login` ≠ email; username and email differ | +| Okta `preferred_username` ≠ email → existing account | username claim differs from email | +| `email_verified` absent or false | Provider has not attested the email | +| Custom `emailClaim` (non-`email` claim used as identity) | `email_verified` does not attest the custom claim | +| Unsigned / unverified id token | Signature not verified | +| JWKS-signed token from an unpinned Azure `/common` (multi-tenant) | Issuer cannot be validated; nOAuth class attack | +| Azure id token from a tenant other than a pinned multi-tenant provider's | ID token verification fails outright (not just denied adoption) — pinning makes the provider single-tenant | +| Login arriving before background OIDC discovery resolves | No validated issuer yet for this attempt | +| Azure id token without `email_verified` | No verified-email attestation available | +| Plain UserInfo response (no id token) | No authenticated binding to the identity | +| GitHub `/user/emails` fetch failed or returned non-OK | Email not confirmed by an authenticated call | +| GitHub `/user/emails` returned `verified: false` | Email not verified by GitHub | +| Non-GitHub provider with custom `getUserInfo` adapter | `github-authenticated` requires `provider=github` | +| Custom adapter returning `_emailProvenance: 'github-authenticated'` | Provenance is plugin-assigned; adapter value ignored | +| `fetchEmail: true` resolved email from UserInfo (no signed token) | Email is present but provenance is unauthenticated | Two further denials happen earlier, during [email resolution itself](#github-default-email-when-there-are-several-verified-addresses) (`usernameClaim: 'email'` only) — not from the trust-source table above, but from which address gets picked in the first place: @@ -459,14 +461,16 @@ Two further denials happen earlier, during [email resolution itself](#github-def **Allowed:** -| Scenario | Reason | -| -------------------------------------------------------------- | ----------------------------------------------------------- | -| Google (JWKS-signed token, `email_verified=true`) | `signed-oidc`: issuer validated, email verified | -| Auth0 (JWKS-signed token, `email_verified=true`) | `signed-oidc`: issuer derived from domain | -| Okta (JWKS-signed token, `preferred_username` equals email) | `signed-oidc`: issuer derived from domain | -| Azure single-tenant (JWKS-signed, `email_verified=true`) | `signed-oidc`: issuer derived from tenant ID | -| GitHub (`provider=github`) with verified email, username=email | `github-authenticated`: successful `/user/emails`, verified | -| `onLogin` hook returns `{ user }` | Hook path, gate skipped | +| Scenario | Reason | +| --------------------------------------------------------------------- | ----------------------------------------------------------- | +| Google (JWKS-signed token, `email_verified=true`) | `signed-oidc`: issuer validated, email verified | +| Auth0 (JWKS-signed token, `email_verified=true`) | `signed-oidc`: issuer derived from domain | +| Okta (JWKS-signed token, `preferred_username` equals email) | `signed-oidc`: issuer derived from domain | +| Azure single-tenant (JWKS-signed, `email_verified=true`) | `signed-oidc`: issuer derived from tenant ID | +| Azure multi-tenant with `issuer` pinned to the matching tenant | `signed-oidc`: verified against that tenant's own keys | +| Okta custom authorization server, explicit endpoints, no `issuer` set | `signed-oidc`: issuer derived via OIDC discovery | +| GitHub (`provider=github`) with verified email, username=email | `github-authenticated`: successful `/user/emails`, verified | +| `onLogin` hook returns `{ user }` | Hook path, gate skipped | ### Escape hatch: `allowUnverifiedClaimInheritance` @@ -498,6 +502,10 @@ Default: `false` (off). Junk values, unresolved `${VAR}` placeholders (environme For a roleless login with no matching account and an untrusted claim, the plugin sets the session identity to a **quarantine principal** of the form `unverified:#`. The random suffix makes the value unpredictable, so it can never be created as an `hdb_user` and can never resolve to a role. Unlike a fixed reserved prefix, it requires no naming convention and no fail-closed collision check — its security comes from unpredictability, not from a reserved namespace. The claim is embedded before the `#` purely for log readability and is never parsed back out. +### OIDC discovery and startup behavior (upgrade note) + +Before this release, a statically configured JWKS-enabled provider with explicit endpoints and no usable `issuer` failed to start (see the `[Unreleased]`/2.9.0 CHANGELOG entry for #231 §4). As of this release, that config now boots normally whenever `authorizationUrl` is `https://` — the issuer is instead derived in the background via OIDC discovery (non-`https` is unchanged: it still fails fast, since no amount of waiting fixes that). While discovery is pending, or if it never resolves (e.g. the IdP doesn't publish a discovery document, or it doesn't validate against your configured endpoints), logins through that provider proceed exactly as any other "signed but not issuer-validated" login already does: a login that would adopt an **existing** account is **denied**, and a login with **no** existing account match proceeds as a quarantined, roleless session — the same behavior Azure `/common` has always had. A session already created while discovery was pending does not retroactively gain trust once discovery later succeeds; only a subsequent login benefits. A failed discovery attempt is retried only on a live config reload (or a restart) at least 5 minutes after the failure — never automatically, and never from a login path. A reload inside that 5-minute window is a no-op, so a transient DNS/network blip does **not** recover "on its own": it requires an operator-initiated reload after the cooldown, or a restart. Each Harper worker thread also holds its own discovery cache (module state, not shared), so the same blip can affect only some of a multi-worker deployment's threads — whether a given login is denied adoption depends on which worker served it. + ### Pre-existing sessions (upgrade note) Harper sessions do not expire by default. A session established **before this release** still holds a raw claim as its identity, which Harper resolves by name against `hdb_user` — and the login-time gate governs only _new_ logins, so it cannot retract an already-persisted session. Every OAuth login from this release forward records a provenance stamp (`authTrust`) on the session; a session whose `oauth` metadata carries no stamp is a **legacy** (pre-gate) session. diff --git a/docs/providers.md b/docs/providers.md index b3913f8..1b07f6b 100644 --- a/docs/providers.md +++ b/docs/providers.md @@ -190,6 +190,28 @@ export OAUTH_REDIRECT_URI="https://yourdomain.com/oauth/callback" [Microsoft Graph Permissions](https://learn.microsoft.com/en-us/graph/permissions-reference) +### Multi-Tenant Apps and Account Adoption + +`tenantId` set to a real tenant GUID derives a fixed issuer, same as any other provider. Leaving `tenantId` unset (or set to `common`/`organizations`/`consumers`) accepts sign-ins from any tenant, but — because there's no single fixed issuer for those, and no key set exclusive to one tenant — ID tokens aren't trusted for [account adoption](./configuration.md#account-adoption-gate): logins work, but every login is denied adoption (or quarantined), exactly as for any other unverified claim. + +Pinning `issuer` alongside a multi-tenant `tenantId` does **not** keep the provider multi-tenant with per-login adoption gating. It turns the provider into a single-tenant one, for exactly the one tenant named by the pin — the plugin redirects verification to that tenant's own, non-shared signing keys and enforces that tenant's issuer, exactly as if you had set `tenantId` to that tenant's GUID directly. Sign-ins from every other tenant stop working outright: ID token verification fails, and the Microsoft Graph `/v1.0/me` fallback can't rescue the login either, since Graph returns `mail`, not `email` — the preset's default `usernameClaim`. + +```yaml +'@harperfast/oauth': + providers: + azure: + tenantId: 'common' # only used to resolve the pin below to a real tenant + issuer: 'https://login.microsoftonline.com//v2.0' # provider becomes single-tenant: only this tenant signs in + clientId: ${OAUTH_AZURE_CLIENT_ID} + clientSecret: ${OAUTH_AZURE_CLIENT_SECRET} +``` + +If you need more than one tenant to sign in, configure a separate provider entry per tenant (each pinned to its own tenant GUID) rather than pinning one multi-tenant provider to an array of issuers — an array would let those tenants adopt each other's Harper accounts if they ever shared an email, so it's rejected outright. + +### `userInfoUrl` fetchEmail and the `profile` scope + +If you override `userInfoUrl` away from Microsoft Graph, or set a custom `scope`, keep `profile` in `scope` whenever `fetchEmail: true` is set — the Graph UserInfo correlation needs the ID token's `oid` claim, which Azure only includes when `profile` was requested. The plugin warns at startup if this combination can't work. + --- ## Auth0 (OIDC) @@ -313,6 +335,22 @@ To include groups in the ID token: [Okta OAuth Documentation](https://developer.okta.com/docs/guides/implement-oauth-for-okta/main/) +### Using a Custom Authorization Server + +`domain` alone derives the **org authorization server** (`/oauth2/v1/*`), whose issuer is just `https://{domain}`. If you're using a [custom authorization server](https://developer.okta.com/docs/concepts/auth-servers/) instead (e.g. `default`, or a generated ID), set `authServer` alongside `domain` — the plugin derives both the path-inclusive endpoints and the issuer for you: + +```yaml +'@harperfast/oauth': + providers: + okta: + domain: ${OAUTH_OKTA_DOMAIN} + authServer: 'default' # or your custom auth server's ID + clientId: ${OAUTH_OKTA_CLIENT_ID} + clientSecret: ${OAUTH_OKTA_CLIENT_SECRET} +``` + +Without `authServer`, pointing `authorizationUrl`/`tokenUrl`/`userInfoUrl`/`jwksUri` at a custom authorization server directly (bypassing `domain`) no longer requires setting `issuer` explicitly — OIDC discovery derives it for you in the background instead. See [`issuer` is usually unnecessary](#issuer-is-usually-unnecessary) below. + --- ## Custom OIDC Provider @@ -347,6 +385,16 @@ export OAUTH_CUSTOM_USERINFO_URL="https://provider.com/oauth/userinfo" export OAUTH_CUSTOM_JWKS_URL="https://provider.com/.well-known/jwks.json" ``` +### `issuer` is usually unnecessary + +When `jwksUri` is set but `issuer` isn't, the plugin derives it in the background via [OIDC discovery](https://openid.net/specs/openid-connect-discovery-1_0.html): it probes `.well-known/openid-configuration` at your `authorizationUrl`'s own origin and validates the result strictly against your configured endpoints before trusting it. This never blocks startup, and the first login that needs it waits only briefly for an in-flight attempt. On success, the server log names the exact value — copy it into `issuer` to pin it and skip discovery on future logins: + +``` +OIDC discovery for provider 'custom': issuer derived via discovery: https://provider.com/ — add issuer: https://provider.com/ to pin it. +``` + +Discovery only runs for an `https://` `authorizationUrl` — a non-`https` one (and, separately, `jwksUri` with no usable `issuer` and no way to derive one) still fails fast at startup, since no amount of waiting fixes that. See [Understanding `issuer`](./configuration.md#account-adoption-gate) for what a missing/undiscoverable issuer means for account adoption. + --- ## Testing Your Configuration diff --git a/src/index.ts b/src/index.ts index 0a0550d..9029691 100644 --- a/src/index.ts +++ b/src/index.ts @@ -12,7 +12,9 @@ import { extractPluginDefaults, normalizeMcpSecurityConfig, coerceConfigBoolean, + collectDiscoveryTargets, } from './lib/config.ts'; +import { startIssuerDiscovery } from './lib/discovery.ts'; import { OAuthResource, toHttpResponse } from './lib/resource.ts'; import { validateAndRefreshSession } from './lib/sessionValidator.ts'; import { clearOAuthSession, SESSION_CLEAR_FAILED_HEADERS } from './lib/handlers.ts'; @@ -22,7 +24,7 @@ import { registerWellKnownHandlers } from './lib/mcp/wellKnown.ts'; import { interactivePrivateKeyJwtEnabled } from './lib/mcp/clientAuthMethod.ts'; import { algFromPrivateKeyPem } from './lib/mcp/keyStore.ts'; import { redactSecrets } from './lib/redact.ts'; -import type { Scope, OAuthPluginConfig, ProviderRegistry, OAuthHooks } from './types.ts'; +import type { Scope, OAuthPluginConfig, ProviderRegistry, OAuthHooks, OAuthProviderConfig } from './types.ts'; // Export HookManager class, OAuthResource class, and types export { HookManager } from './lib/hookManager.ts'; @@ -370,6 +372,17 @@ export async function handleApplication(scope: Scope): Promise { Object.keys(providers).forEach((key) => delete providers[key]); Object.assign(providers, newProviders); + // Schedule OIDC discovery (#264) right after this registry goes live — + // see src/lib/DESIGN.md for why this exact placement matters. try/catch + // is defense in depth only; neither call below throws by design. + try { + for (const target of collectDiscoveryTargets(providers)) { + startIssuerDiscovery(target.authorizationUrl, target.jwksUri, target.tokenUrl, target.providerName, logger); + } + } catch (error) { + logger?.error?.('OAuth: failed to schedule OIDC issuer discovery:', error); + } + // Extract plugin defaults for dynamic provider resolution pluginDefaults = extractPluginDefaults(options); @@ -457,8 +470,27 @@ export async function handleApplication(scope: Scope): Promise { const providerConfigId = request.session.oauth.providerConfigId || request.session.oauth.provider; let providerData = providers[providerConfigId] ?? dynamicProviderCache.get(providerConfigId); - // If still not found, try to resolve via hook - if (!providerData && hookManager?.hasHook('onResolveProvider')) { + // If still not found, try to resolve via hook — unless a recent attempt + // for this exact provider already failed on an invalid Azure issuer pin + // (HarperFast/oauth#264/#271): skip straight to treating this session + // the same as "provider not found" below (logging it out) rather than + // re-running the hook and rebuilding the config only to re-throw the + // same AzureIssuerBindingError on every request carrying this session. + // The cache lookup itself is gated on `!providerData` first — it's + // dynamic-resolution-only state, and must not cost every OAuth-session + // request a Map lookup on the already-resolved (static or cached) path. + const cachedAzurePinFailure = !providerData ? dynamicProviderCache.getAzurePinFailure(providerConfigId) : undefined; + if (cachedAzurePinFailure) { + // Advisory only, and deliberately NOT error level: the cause was + // already logged at error level once, when recorded below — logging + // it again on every cached hit for every request of this session + // during the cooldown is exactly the log flood this cache exists to + // avoid. + logger?.debug?.( + `OAuth provider '${providerConfigId}' (dynamic resolution, session validation) still has an invalid ` + + `Azure issuer pin (cooling down, not re-resolving): ${cachedAzurePinFailure}` + ); + } else if (!providerData && hookManager?.hasHook('onResolveProvider')) { try { logger?.debug?.( `Provider config "${providerConfigId}" not in registry or cache, attempting dynamic resolution` @@ -469,13 +501,32 @@ export async function handleApplication(scope: Scope): Promise { if (hookConfig) { const { OAuthProvider } = await import('./lib/OAuthProvider.ts'); const { buildProviderConfig } = await import('./lib/config.ts'); - - const config = buildProviderConfig(hookConfig, providerConfigId, pluginDefaults); - const provider = new OAuthProvider(config, logger); - - providerData = { provider, config }; - logger?.info?.(`Dynamically resolved provider for session validation: ${providerConfigId}`); - dynamicProviderCache.set(providerConfigId, providerData); + const { AzureIssuerBindingError } = await import('./lib/azureIssuer.ts'); + + const dedupedLogger = dynamicProviderCache.wrapLoggerForDynamicResolution(providerConfigId, logger); + let config: OAuthProviderConfig | undefined; + try { + config = buildProviderConfig(hookConfig, providerConfigId, pluginDefaults, false, dedupedLogger); + } catch (buildError) { + if (buildError instanceof AzureIssuerBindingError) { + dynamicProviderCache.recordAzurePinFailure(providerConfigId, buildError.message); + logger?.error?.( + `OAuth provider '${providerConfigId}' (dynamic resolution, session validation) has an invalid ` + + `Azure issuer pin: ${buildError.message}` + ); + config = undefined; + } else { + throw buildError; + } + } + + if (config) { + const provider = new OAuthProvider(config, logger); + providerData = { provider, config }; + logger?.info?.(`Dynamically resolved provider for session validation: ${providerConfigId}`); + dynamicProviderCache.set(providerConfigId, providerData); + dynamicProviderCache.clearAzurePinFailure(providerConfigId); + } } } catch (error) { logger?.error?.( diff --git a/src/lib/DESIGN.md b/src/lib/DESIGN.md new file mode 100644 index 0000000..18673fe --- /dev/null +++ b/src/lib/DESIGN.md @@ -0,0 +1,54 @@ +# Design notes for `src/lib` + +Invariants that aren't obvious from reading the code in isolation. + +## Azure multi-tenant adoption never consults the shared key pool (HarperFast/oauth#264) + +`azureIssuer.ts`'s `resolveAzureIssuerBinding` rewrites a pinned alias authority +(`common`/`organizations`/`consumers`) to the pinned tenant's own, +non-shared `/{guid}/discovery/v2.0/keys` endpoint, and `OAuthProvider` then +verifies against that rewritten `jwksUri` through the exact same code path as +any ordinary single-tenant config. This is deliberate, not a simplification +taken for convenience: Microsoft's tenant-independent endpoints (`/common`, +`/organizations`) serve a _shared_ key set where a key's cryptographic +validity does not, by itself, prove which tenant it was intended for — only a +per-key `issuer` property in the raw JWKS response does, and that property +isn't reachable through `jwks-rsa`'s public API. Redirecting to the one +pinned tenant's own endpoint sidesteps that ambiguity entirely instead of +trying to parse around it. **Do not reintroduce a path that fetches keys from +`/common`/`/organizations`/`consumers` directly for a pinned provider** — it +would reopen the gap this design closes. + +A multi-tenant authority with **no** pin is left completely untouched +(`issuer` never set, `jwksUri` never rewritten) — adoption-eligibility for an +Azure alias is opt-in, one tenant at a time, by design (an array pin is +rejected at config-build/`OAuthProvider`-construction time for the same +reason: it would let two of an operator's own pinned tenants adopt each +other's Harper account if they ever shared a verified email). + +## Where OIDC discovery is scheduled from (`src/index.ts`) + +Discovery for a statically configured, issuer-less JWKS provider is scheduled +from `updateConfiguration` immediately after `Object.assign(providers, +newProviders)` — not after `resources.set('oauth', OAuthResource)`, and not +as the function's last statement. That specific line is the earliest point +guaranteed to be _definitely late enough_ (a build that fails inside +`initializeProviders`, e.g. the reserved `'mcp'` provider name, never reaches +it) and _definitely not too late_ (on a **reload**, `OAuthResource.providers` +already references this same object from a prior `configure()` call, so the +registry is already being served the instant this line runs — before +`resources.set` or any later log statement, which can therefore no longer +retroactively make the schedule "too late"). Moving this call later (e.g. +after `resources.set`, or to the end of the function) silently reintroduces a +window where an already-live, already-serving provider never gets discovery +scheduled for it, because a later, unrelated statement (a throwing logger +on the "ready" message, for instance) made the overall reload report failure. + +## Module layering: `azureIssuer.ts` and `discovery.ts` do not import `config.ts`/`OAuthProvider.ts` + +Both modules are called from `config.ts`'s `buildProviderConfig` **and** from +`OAuthProvider`'s own constructor (the latter is the only enforcement point a +config built outside `buildProviderConfig` — e.g. `TenantManager` — ever +crosses). Keeping them dependency-free of both callers avoids a three-way +import cycle; each duplicates the few trivial predicates it needs (e.g. +`hasUsableIssuer`) rather than importing them. diff --git a/src/lib/OAuthProvider.ts b/src/lib/OAuthProvider.ts index 890df39..efd53c8 100644 --- a/src/lib/OAuthProvider.ts +++ b/src/lib/OAuthProvider.ts @@ -19,6 +19,41 @@ import type { } from '../types.ts'; import { csrfTokenManager } from './CSRFTokenManager.ts'; import { ADAPTER_EMAIL_PROVENANCE } from './emailProvenance.ts'; +import { resolveAzureIssuerBinding, isAzureJwksUri } from './azureIssuer.ts'; +import { awaitDiscoveredIssuer } from './discovery.ts'; + +/** + * True when `issuer` is a value `jwt.verify`/`verifyIdTokenClaims` will + * actually check against — a non-empty string, or a non-empty array (an + * empty array normalizes to "no issuer configured"). Duplicated from + * `config.ts` (not imported) to avoid a module cycle — `config.ts` + * constructs `OAuthProvider`. + */ +function hasUsableIssuer(issuer: OAuthProviderConfig['issuer']): boolean { + if (Array.isArray(issuer)) return issuer.some((value) => typeof value === 'string' && value !== ''); + return typeof issuer === 'string' && issuer !== ''; +} + +/** + * True when an issuer-less, JWKS-enabled config is eligible to consult the + * OIDC discovery cache (HarperFast/oauth#264): `authorizationUrl` parses as + * `https:` (discovery must not depend on the network for a config that's + * wrong independent of it), and the endpoint isn't Azure's own host (an + * *unpinned* alias authority is intentionally issuer-less — see + * `azureIssuer.ts` — and generic discovery must never probe it). Mirrors + * `config.ts`'s `needsIssuerDiscovery` (not imported, to avoid a module + * cycle); the `jwksUri`/`!hasUsableIssuer` half of that check is already + * established by the caller. + */ +function canAwaitDiscoveredIssuer(config: OAuthProviderConfig): boolean { + if (!config.jwksUri) return false; + if (isAzureJwksUri(config.jwksUri)) return false; + try { + return new URL(config.authorizationUrl).protocol === 'https:'; + } catch { + return false; + } +} import { ResolveEmailError } from './resolveEmailError.ts'; export class OAuthProvider implements IOAuthProvider { @@ -33,6 +68,14 @@ export class OAuthProvider implements IOAuthProvider { if (!csrfTokenManager['logger']) { csrfTokenManager['logger'] = logger; } + // Enforced here too (not only in `config.ts`'s `buildProviderConfig`), + // because a config built outside that path — e.g. `TenantManager`, + // which assembles `OAuthProviderConfig` directly and never calls + // `buildProviderConfig` — must not reach a shared Azure alias endpoint + // with a pinned issuer and no tenant-specific binding (HarperFast/oauth#264). + // A no-op for a non-Azure-shaped `jwksUri`, and idempotent once the + // rewrite has already happened. + resolveAzureIssuerBinding(this.config, this.config.provider, logger); this.validateConfig(); this.initializeJwksClient(); } @@ -308,10 +351,14 @@ export class OAuthProvider implements IOAuthProvider { * Returns `{ claims, signatureVerified, issuerValidated }`: * - `signatureVerified` — true only when the token was checked against a * JWKS-fetched public key. The no-JWKS fallback always yields false. - * - `issuerValidated` — true only when `config.issuer` is set AND the token's - * `iss` was verified to equal that value by jwt.verify. Providers without a - * known issuer (Azure /common, issuer-less generic) yield false; their tokens - * are not trusted for account adoption. + * - `issuerValidated` — true only when the token's `iss` was verified + * against a known-good issuer: `config.issuer` if already set (by a + * preset, an explicit pin, or a prior call's discovered/Azure-derived + * value — see `config.ts`), or, failing that, one derived via OIDC + * discovery (HarperFast/oauth#264) and checked explicitly against the + * already-signature-verified claims. An unpinned Azure alias + * authority (`/common`) or any other provider discovery can't help + * yields false; their tokens are not trusted for account adoption. */ async verifyIdToken(idToken: string): Promise<{ claims: any; signatureVerified: boolean; issuerValidated: boolean }> { // First decode to get the header and payload @@ -334,13 +381,20 @@ export class OAuthProvider implements IOAuthProvider { const key = await this.jwksClient.getSigningKey(kid); const publicKey = key.getPublicKey(); + const configuredIssuer = hasUsableIssuer(this.config.issuer); // jsonwebtoken's `issuer` accepts a string or a non-empty tuple; normalize - // (an array issuer lets a provider accept several valid iss forms). - const expectedIssuer: string | [string, ...string[]] | undefined = Array.isArray(this.config.issuer) - ? this.config.issuer.length + // (an array issuer lets a provider accept several valid iss forms). When no + // issuer is configured yet, pass none at all to jwt.verify — a discovered + // issuer (below) is checked afterward as an explicit equality check, never + // baked into this call. That ordering matters (HarperFast/oauth#264): a + // bogus/forged token fails signature verification here, before the + // discovery-await branch is ever reached, so it can never consume the one + // bounded wait a concurrent, genuinely signature-valid login needs. + const expectedIssuer: string | [string, ...string[]] | undefined = configuredIssuer + ? Array.isArray(this.config.issuer) ? (this.config.issuer as [string, ...string[]]) - : undefined - : this.config.issuer || undefined; + : (this.config.issuer as string) + : undefined; // Verify signature and claims const verified = jwt.verify(idToken, publicKey, { @@ -361,9 +415,32 @@ export class OAuthProvider implements IOAuthProvider { if (!verified.exp) throw new Error('ID token missing required exp claim'); this.logger?.debug?.('ID token signature verified successfully'); - // issuerValidated only when a non-empty issuer was actually passed to - // jwt.verify (an empty array normalizes to undefined and skips the check). - return { claims: verified, signatureVerified: true, issuerValidated: expectedIssuer != null }; + + let issuerValidated = configuredIssuer; + if (!configuredIssuer && canAwaitDiscoveredIssuer(this.config)) { + const discovered = await awaitDiscoveredIssuer( + this.config.authorizationUrl, + this.config.jwksUri!, + this.config.tokenUrl, + this.logger + ); + if (discovered) { + issuerValidated = verified.iss === discovered; + // Cache (idempotent: later calls take the `configuredIssuer` + // branch above) only on a match. A mismatch is this one + // login's own problem (e.g. a non-compliant IdP whose tokens + // disagree with its own discovery document) — caching + // `discovered` anyway would make every FUTURE login's `iss` + // (including ones that previously had no issuer check at all) + // fail `jwt.verify` against a value their real tokens don't + // carry, breaking logins that worked before this one. + if (issuerValidated) { + this.config.issuer = discovered; + } + } + } + + return { claims: verified, signatureVerified: true, issuerValidated }; } catch (error) { // Signature verification failed - this is a security issue this.logger?.error?.( diff --git a/src/lib/azureIssuer.ts b/src/lib/azureIssuer.ts new file mode 100644 index 0000000..5964ae5 --- /dev/null +++ b/src/lib/azureIssuer.ts @@ -0,0 +1,478 @@ +/** + * Azure AD / Entra ID issuer resolution (HarperFast/oauth#264). + * + * `/common`, `/organizations` and `/consumers` are shared, tenant-independent + * authorities with no single fixed issuer and no key set exclusive to one + * tenant — a signature alone cannot prove which tenant a token belongs to. + * A real tenant GUID, by contrast, is a tenant-exclusive authority: its + * issuer and its JWKS response are both specific to that one tenant. + * + * This module gives every alias authority exactly one way to become usable: + * the operator pins `issuer` to a single Azure tenant-GUID issuer string. + * `resolveAzureIssuerBinding` then rewrites the effective `jwksUri` to that + * same tenant's own, non-shared endpoint and sets `issuer` to its canonical + * form — collapsing the pinned-alias case into the already-safe, already- + * simple tenant-GUID case, with no new verification branch, resolver, or + * cache: `OAuthProvider` ends up with an ordinary single-tenant config. + * Left unpinned, an alias authority is untouched (byte-identical to today). + * + * Called from both `buildProviderConfig` (surfacing a misconfiguration as a + * startup error as early as possible) and `OAuthProvider`'s own constructor + * (the one point every construction path — including `TenantManager`, which + * builds its config without going through `buildProviderConfig` at all — + * is guaranteed to cross). Safe to call twice: once the rewrite has already + * happened, the config simply looks like a real-tenant-GUID config, and a + * second call is a no-op. + */ + +import type { Logger, OAuthProviderConfig } from '../types.ts'; + +/** Entra ID's documented, permanent nickname for the "personal Microsoft accounts" tenant. */ +export const AZURE_CONSUMERS_TENANT_ID = '9188040d-6c67-4c5b-b112-36a304b66dad'; + +/** + * A configured, but invalid, Azure `issuer` pin — distinguished from any + * other `buildProviderConfig` failure so `initializeProviders` (#259/#260's + * "skip the bad provider, keep the others" pattern) can treat it the same + * way: this one provider's own pin is wrong, which says nothing about + * whether every OTHER declared provider is safe to start. + */ +export class AzureIssuerBindingError extends Error {} + +const AZURE_HOST = 'login.microsoftonline.com'; +const AZURE_STS_HOST = 'sts.windows.net'; +const GUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; +const ALIAS_SEGMENTS = new Set(['common', 'organizations', 'consumers']); + +function azureTenantUri(kind: 'issuer' | 'keys', guid: string): string { + return kind === 'issuer' ? `https://${AZURE_HOST}/${guid}/v2.0` : `https://${AZURE_HOST}/${guid}/discovery/v2.0/keys`; +} + +/** + * The `{segment}` of an exact `https://login.microsoftonline.com/{segment}/discovery/v2.0/keys` + * URL — full-URL match: exact host, no port/userinfo/query/fragment, exactly + * one path segment before the fixed suffix. `null` for anything else, + * including a lookalike with extra path segments or a different host. + */ +function azureJwksSegment(jwksUri: string | null | undefined): string | null { + if (!jwksUri) return null; + let url: URL; + try { + url = new URL(jwksUri); + } catch { + return null; + } + if (url.protocol !== 'https:') return null; + if (url.hostname !== AZURE_HOST) return null; + if (url.port !== '' || url.username !== '' || url.password !== '' || url.search !== '' || url.hash !== '') { + return null; + } + const match = /^\/([^/]+)\/discovery\/v2\.0\/keys$/.exec(url.pathname); + return match ? match[1] : null; +} + +/** + * The `{segment}` of an exact Azure v1 JWKS keys-endpoint URL + * (`https://login.microsoftonline.com/{segment}/discovery/keys` — no + * `v2.0`; Azure's actual v1 metadata `jwks_uri` for a shared alias + * authority), when `{segment}` is one of the shared aliases. `null` for + * anything else, including the same path shape for a real tenant GUID — + * out of scope here; this module's GUID branch only recognizes the v2 JWKS + * shape (`azureJwksSegment`), and a v1-shaped GUID `jwksUri` is unaffected + * by this function. + */ +function azureV1AliasJwksSegment(jwksUri: string | null | undefined): string | null { + if (!jwksUri) return null; + let url: URL; + try { + url = new URL(jwksUri); + } catch { + return null; + } + if (url.protocol !== 'https:') return null; + if (url.hostname !== AZURE_HOST) return null; + if (url.port !== '' || url.username !== '' || url.password !== '' || url.search !== '' || url.hash !== '') { + return null; + } + const match = /^\/([^/]+)\/discovery\/keys$/.exec(url.pathname); + if (!match) return null; + const segment = match[1].toLowerCase(); + return ALIAS_SEGMENTS.has(segment) ? segment : null; +} + +/** True when `issuer` is a value `jwt.verify` would actually check against. */ +function hasUsableIssuer(issuer: OAuthProviderConfig['issuer']): boolean { + if (Array.isArray(issuer)) return issuer.some((value) => typeof value === 'string' && value !== ''); + return typeof issuer === 'string' && issuer !== ''; +} + +/** + * The tenant GUID of a single Azure v1 (`https://sts.windows.net/{guid}/`) + * or v2 (`https://login.microsoftonline.com/{guid}/v2.0`) tenant-exclusive + * issuer string, lowercased. `null` for anything else — a different host, + * an alias segment, or a malformed value. + */ +function azureIssuerGuidAnyVersion(value: unknown): string | null { + if (typeof value !== 'string' || value === '') return null; + let url: URL; + try { + url = new URL(value); + } catch { + return null; + } + if (url.protocol !== 'https:') return null; + if (url.port !== '' || url.username !== '' || url.password !== '' || url.search !== '' || url.hash !== '') { + return null; + } + let match: RegExpExecArray | null = null; + if (url.hostname === AZURE_HOST) match = /^\/([^/]+)\/v2\.0\/?$/.exec(url.pathname); + else if (url.hostname === AZURE_STS_HOST) match = /^\/([^/]+)\/?$/.exec(url.pathname); + if (!match) return null; + const guid = match[1].toLowerCase(); + return GUID_RE.test(guid) ? guid : null; +} + +/** + * True when every value of a usable `issuer` (string or array) names the + * same tenant `guid`, in either Azure issuer form + * (`login.microsoftonline.com/.../v2.0` or `sts.windows.net/...`). Case and + * a trailing slash are tolerated — `jwt.verify` itself does exact string + * comparison, so this is what lets a pin in either acceptable shape or + * casing be canonicalized rather than rejected or left to fail later. + */ +function azureIssuerNamesOnlyTenant(issuer: OAuthProviderConfig['issuer'], guid: string): boolean { + const values = Array.isArray(issuer) ? issuer : [issuer]; + return values.every((value) => azureIssuerGuidAnyVersion(value) === guid); +} + +const V1_AUTHORIZE_SUFFIX = '/oauth2/authorize'; +const V2_AUTHORIZE_SUFFIX = '/oauth2/v2.0/authorize'; + +/** + * Which Azure authorize-endpoint shape `authorizationUrl` is — the thing + * that actually determines a real token's `iss` host (v1 `/oauth2/authorize` + * issues `sts.windows.net` tokens; v2 `/oauth2/v2.0/authorize` issues + * `login.microsoftonline.com` ones). Neither the pin's own form nor the + * jwksUri's shape says anything about this — the Azure preset, for example, + * always generates a v2 `authorizationUrl` regardless of what the operator + * pins. `null` for a non-Azure host, or an authorize shape this function + * doesn't recognize (e.g. a B2C custom-policy URL) — callers must not + * additionally constrain the pin's form in that case, only its tenant. + */ +function azureAuthorizeIssuerHost(authorizationUrl: string | null | undefined): string | null { + if (!authorizationUrl) return null; + let url: URL; + try { + url = new URL(authorizationUrl); + } catch { + return null; + } + if (url.hostname !== AZURE_HOST) return null; + if (url.pathname.endsWith(V2_AUTHORIZE_SUFFIX)) return AZURE_HOST; + if (url.pathname.endsWith(V1_AUTHORIZE_SUFFIX)) return AZURE_STS_HOST; + return null; +} + +/** + * True when every value of a usable `issuer` is in the one form a real + * token from `authorizationUrl` would actually carry — or when + * `authorizationUrl`'s shape isn't recognized, in which case this imposes + * no additional constraint (see {@link azureAuthorizeIssuerHost}). + */ +function azureIssuerMatchesAuthorizeForm(issuer: OAuthProviderConfig['issuer'], authorizationUrl: string): boolean { + const expectedHost = azureAuthorizeIssuerHost(authorizationUrl); + if (!expectedHost) return true; + const values = Array.isArray(issuer) ? issuer : [issuer]; + return values.every((value) => { + if (typeof value !== 'string') return false; + try { + return new URL(value).hostname === expectedHost; + } catch { + return false; + } + }); +} + +/** + * Canonicalize one already-validated (`azureIssuerGuidAnyVersion(value) === + * guid`) issuer value to its own form's exact string — case and a trailing + * slash only. Which `iss` form a real token carries depends on the + * *authorize* endpoint, not the JWKS URL (a v1 `/oauth2/authorize` issues + * `sts.windows.net` tokens; v2 issues `login.microsoftonline.com` tokens), + * so a v1 pin must stay v1 and a v2 pin must stay v2 — collapsing both to + * one form would silently break whichever form the operator's real tokens + * actually use. + */ +function azureCanonicalIssuerForm(value: string, guid: string): string { + return new URL(value).hostname === AZURE_HOST ? azureTenantUri('issuer', guid) : `https://${AZURE_STS_HOST}/${guid}/`; +} + +/** {@link azureCanonicalIssuerForm}, applied to every value of a string-or-array issuer, preserving its shape. */ +function normalizeAzureIssuerPin(issuer: OAuthProviderConfig['issuer'], guid: string): OAuthProviderConfig['issuer'] { + if (Array.isArray(issuer)) return issuer.map((value) => azureCanonicalIssuerForm(value as string, guid)); + return azureCanonicalIssuerForm(issuer as string, guid); +} + +/** + * The `{segment}` of an Azure authorize URL's path — either the v1 + * (`/{segment}/oauth2/authorize`) or v2 (`/{segment}/oauth2/v2.0/authorize`) + * shape, lowercased. `null` for a non-Azure host or an unrecognized shape + * (e.g. a B2C custom-policy URL) — callers must not warn on `null`; there is + * no information to compare. + */ +function azureAuthorizeTenantSegment(authorizationUrl: string | null | undefined): string | null { + if (!authorizationUrl) return null; + let url: URL; + try { + url = new URL(authorizationUrl); + } catch { + return null; + } + if (url.hostname !== AZURE_HOST) return null; + const match = + /^\/([^/]+)\/oauth2\/v2\.0\/authorize$/.exec(url.pathname) ?? /^\/([^/]+)\/oauth2\/authorize$/.exec(url.pathname); + return match ? match[1].toLowerCase() : null; +} + +/** + * The issuer to DERIVE (no pin given) for tenant `guid`, in the form + * `authorizationUrl` actually issues — v1 (`sts.windows.net`) when it's a + * recognized v1 authorize endpoint, v2 otherwise (including an unrecognized + * shape, matching this function's behavior before v1 derivation existed). + * Mirrors the pinned branch's `azureIssuerMatchesAuthorizeForm` check — a + * real-tenant-GUID config with NO pin must derive the same form that one + * would have been required to match. + * + * Also warns (never throws — advisory only) when `authorizationUrl` names a + * DIFFERENT tenant (an alias, or another real GUID) than `jwksUri`'s `guid`: + * deriving an issuer here is still the right, secure direction (it collapses + * this config into the already-safe tenant-exclusive case, same as the + * pinned branch), but it is also a silent behavior change — on `main`, this + * exact shape (e.g. `authorizationUrl` pointed at the shared `/common` + * endpoint, `jwksUri` pointed at one real tenant) had no issuer check at + * all, so any tenant whose token verified against those keys could sign in; + * now only `guid`'s tokens can. The warning exists so an operator who relied + * on that (deliberately or not) finds out at startup, not from a wave of + * `jwt issuer invalid` login failures. + * + * Only fires when `authorizeSegment` is something we can actually compare + * against `guid`: an alias (`common`/`organizations`/`consumers`) or another + * real tenant GUID. A verified-domain segment (e.g. `contoso.onmicrosoft.com`) + * is neither — Azure accepts a tenant's registered domain name here, and we + * have no offline way to map that domain to a GUID, so warning on it would be + * a guess, not a finding; staying silent there is deliberate, not an + * oversight. + */ +function azureDerivedIssuer( + guid: string, + authorizationUrl: string | null | undefined, + providerName: string, + logger?: Logger +): string { + const rawAuthorizeSegment = azureAuthorizeTenantSegment(authorizationUrl); + const authorizeSegment = + rawAuthorizeSegment !== null && (ALIAS_SEGMENTS.has(rawAuthorizeSegment) || GUID_RE.test(rawAuthorizeSegment)) + ? rawAuthorizeSegment + : null; + // 'consumers' and AZURE_CONSUMERS_TENANT_ID name the SAME tenant (Azure's + // fixed GUID for personal Microsoft accounts) — not a mismatch, even + // though the alias string and the GUID string are never equal. + const isConsumersAliasMatch = authorizeSegment === 'consumers' && guid === AZURE_CONSUMERS_TENANT_ID; + if (authorizeSegment && authorizeSegment !== guid && !isConsumersAliasMatch) { + try { + logger?.warn?.( + `OAuth provider '${providerName}' (azure) has a jwksUri for tenant '${guid}' but an 'authorizationUrl' ` + + `naming a different tenant ('${authorizeSegment}'). This provider now accepts sign-ins from '${guid}' ` + + `only — a real token from any other tenant now fails ID-token verification outright, where it ` + + `previously had no issuer check at all. If that's not intentional, point 'authorizationUrl' and ` + + `'jwksUri' at the same tenant. If it is, pin 'issuer' explicitly to '${guid}'s issuer to make this ` + + `startup-time derivation explicit in your config.` + ); + } catch { + /* advisory log only */ + } + } + return azureAuthorizeIssuerHost(authorizationUrl) === AZURE_STS_HOST + ? `https://${AZURE_STS_HOST}/${guid}/` + : azureTenantUri('issuer', guid); +} + +/** + * True only for an UNPINNED shared alias authority's exact v2.0 keys-endpoint + * shape (`https://login.microsoftonline.com/common|organizations|consumers/ + * discovery/v2.0/keys`) — the one case `resolveAzureIssuerBinding` above + * deliberately leaves issuer-less by design (byte-identical to today; see + * its own comments). Generic OIDC discovery (`discovery.ts`) and the #231 §4 + * issuer-required check must both skip only THAT case — every caller already + * runs `hasUsableIssuer(config.issuer)` first, so this is only ever reached + * once that's already `false`. + * + * Deliberately NOT "any shape `resolveAzureIssuerBinding` recognizes": a + * real-tenant-GUID segment never reaches here at all (that function always + * either sets a usable issuer or throws for it, so `hasUsableIssuer` above + * already short-circuited), and a tenant-DOMAIN segment (e.g. + * `contoso.onmicrosoft.com` — Azure accepts a verified domain name here, not + * only a GUID), or the older, non-`v2.0` `/discovery/keys` shape on a real + * tenant GUID, is not a case `resolveAzureIssuerBinding` touches at all. + * Exempting either of those too (an earlier, broader version of this check + * did) would boot them silently, `issuerValidated` permanently `false` and + * no error — defeating the point of the check this guards. Falling through + * instead lets the normal startup error fire, or lets generic discovery run + * (Azure supports the standard `.well-known/openid-configuration` path for + * these shapes too). + * + * The v1 alias shape (`.../common/discovery/keys`, no `v2.0`) IS included + * here, alongside the v2 one: it is the same shared, tenant-independent key + * pool under an older URL, and `resolveAzureIssuerBinding` handles it the + * same way (byte-identical when unpinned; throws when pinned, below) — not + * exempting it would route an unpinned one into generic discovery (which + * fails for it) and tell the operator to pin, landing on the pinned case + * that would otherwise bind an issuer to the unvalidated shared pool. + */ +export function isAzureJwksUri(jwksUri: string | null | undefined): boolean { + const segment = azureJwksSegment(jwksUri); + if (segment !== null) return ALIAS_SEGMENTS.has(segment.toLowerCase()); + return azureV1AliasJwksSegment(jwksUri) !== null; +} + +/** + * Resolve (and validate) an Azure issuer binding on `config`, in place. + * A no-op for any `jwksUri` that isn't a recognized Azure JWKS shape (the + * v2 keys endpoint for any segment, or the v1 keys endpoint for a shared + * alias specifically). Throws naming the provider for an unsafe or + * malformed combination (an array/non-Azure pin on an alias authority, a + * pin naming a different tenant than a real-tenant-GUID `jwksUri`, or ANY + * pin on a v1-shaped shared alias) — never silently builds a config that + * could widen trust or that could never validate anything. + */ +export function resolveAzureIssuerBinding(config: OAuthProviderConfig, providerName: string, logger?: Logger): void { + const segment = azureJwksSegment(config.jwksUri); + if (!segment) { + // Not the v2 shape at all — the only other case this module + // recognizes is the v1 shared-alias shape. A v1-shaped real-tenant-GUID + // `jwksUri` is NOT recognized (falls through, untouched, same as + // before): this module's GUID handling only ever operates on the v2 + // JWKS shape. + const v1AliasSegment = azureV1AliasJwksSegment(config.jwksUri); + if (!v1AliasSegment) return; + if (!hasUsableIssuer(config.issuer)) return; // Byte-identical to today — same as an unpinned v2 alias. + throw new AzureIssuerBindingError( + `OAuth provider '${providerName}' (azure) pins 'issuer' on a shared v1 authority ('${v1AliasSegment}', ` + + `jwksUri '.../${v1AliasSegment}/discovery/keys') — a pinned issuer can't be bound to Azure's shared v1 ` + + `key pool. Use the tenant's own keys ('https://login.microsoftonline.com//discovery/keys') ` + + `or the v2 endpoint ('https://login.microsoftonline.com//discovery/v2.0/keys') instead.` + ); + } + const lowerSegment = segment.toLowerCase(); + + if (GUID_RE.test(lowerSegment)) { + // A real, tenant-exclusive authority: the issuer is fully determined by + // the URL itself. An operator-pinned issuer must name only that same + // tenant (in either Azure issuer form, string or array) — Azure's + // signing keys are not guaranteed exclusive to one tenant-specific + // endpoint over time, so a pin naming a different tenant could otherwise + // accept a different tenant's token. A same-tenant pin is canonicalized + // (case, trailing slash) within its OWN form rather than left as-is — + // `jwt.verify` compares issuer strings exactly, so a differently-cased + // GUID or a trailing slash would otherwise fail verification for a + // real, correctly signed token — but never rewritten to a DIFFERENT + // form: which `iss` a real token carries depends on the authorize + // endpoint (v1 `/oauth2/authorize` issues `sts.windows.net` tokens, v2 + // issues `login.microsoftonline.com` ones), so collapsing a v1 pin to + // v2 (or vice versa) would break verification for whichever form the + // operator's real tokens actually use. + if (hasUsableIssuer(config.issuer)) { + if (!azureIssuerNamesOnlyTenant(config.issuer, lowerSegment)) { + throw new AzureIssuerBindingError( + `OAuth provider '${providerName}' (azure) has a jwksUri for tenant '${lowerSegment}' but an ` + + `explicit 'issuer' naming a different tenant, or an unrecognized value, ` + + `(${JSON.stringify(config.issuer)}). Every value of the pinned issuer's tenant and the jwksUri's ` + + `tenant must be the same GUID.` + ); + } + // Naming the right tenant is necessary but not sufficient: the pin's + // FORM must also match what `authorizationUrl` actually issues (e.g. + // the Azure preset always generates a v2 authorizationUrl, regardless + // of what the operator pins) — otherwise every real token's `iss` + // fails `jwt.verify`'s exact-string comparison against the wrong form. + if (!azureIssuerMatchesAuthorizeForm(config.issuer, config.authorizationUrl)) { + throw new AzureIssuerBindingError( + `OAuth provider '${providerName}' (azure) pins 'issuer' to ${JSON.stringify(config.issuer)}, which ` + + `names the right tenant but the wrong Azure issuer form for its 'authorizationUrl' ` + + `(${JSON.stringify(config.authorizationUrl)}). A v2 authorize endpoint ` + + `('.../oauth2/v2.0/authorize') issues '${AZURE_HOST}' tokens; a v1 one ('.../oauth2/authorize') ` + + `issues '${AZURE_STS_HOST}' tokens. Pin the form that matches 'authorizationUrl'.` + ); + } + config.issuer = normalizeAzureIssuerPin(config.issuer, lowerSegment); + return; + } + config.issuer = azureDerivedIssuer(lowerSegment, config.authorizationUrl, providerName, logger); + return; + } + + if (!ALIAS_SEGMENTS.has(lowerSegment)) return; // Not a recognized Azure shape; leave it alone. + + // Shared, tenant-independent authority (/common, /organizations, /consumers): + // adoption-eligible only when the operator pins exactly one tenant-GUID + // issuer. Unpinned, this is byte-identical to today (plain jwks-rsa against + // the shared endpoint, issuer never set, issuerValidated always false). + if (!hasUsableIssuer(config.issuer)) return; + + if (Array.isArray(config.issuer)) { + throw new AzureIssuerBindingError( + `OAuth provider '${providerName}' (azure) pins 'issuer' to an array on a shared authority ` + + `('${lowerSegment}'). Pin exactly one Azure tenant issuer (e.g. ` + + `'https://login.microsoftonline.com//v2.0') — an array would make every listed tenant ` + + `adoption-eligible through one provider, re-opening the cross-tenant adoption question account-level ` + + `identity binding (not issuer validation) is meant to answer. Configure a separate provider entry ` + + `per tenant if more than one tenant needs to be trusted.` + ); + } + + // Either Azure issuer form is accepted here, same as the real-tenant-GUID + // case above: which form a real token's `iss` carries depends on the + // authorize endpoint (v1 vs v2), not on this jwksUri being the shared v2 + // alias endpoint, and Azure signs v1 and v2 tokens with the same keys — + // so a v1-authorize operator pinning a v1 (`sts.windows.net`) issuer is + // exactly as safe as a v2 pin, and rejecting it would leave that + // combination permanently unable to adopt. The pin's form must still + // match `authorizationUrl`'s own shape when that shape is recognized + // (checked below) — otherwise every real token's `iss` fails `jwt.verify`. + const pinnedGuid = azureIssuerGuidAnyVersion(config.issuer); + if (!pinnedGuid) { + throw new AzureIssuerBindingError( + `OAuth provider '${providerName}' (azure) pins 'issuer' to ${JSON.stringify(config.issuer)} on a shared ` + + `authority ('${lowerSegment}'), which is not a usable Azure tenant issuer. Set 'issuer' to exactly one ` + + `tenant issuer URI, e.g. 'https://login.microsoftonline.com//v2.0' or ` + + `'https://sts.windows.net//' (use '${AZURE_CONSUMERS_TENANT_ID}' for the ` + + `personal-Microsoft-account tenant).` + ); + } + if (!azureIssuerMatchesAuthorizeForm(config.issuer, config.authorizationUrl)) { + throw new AzureIssuerBindingError( + `OAuth provider '${providerName}' (azure) pins 'issuer' to ${JSON.stringify(config.issuer)}, which names a ` + + `usable tenant but the wrong Azure issuer form for its 'authorizationUrl' ` + + `(${JSON.stringify(config.authorizationUrl)}). A v2 authorize endpoint ('.../oauth2/v2.0/authorize') ` + + `issues '${AZURE_HOST}' tokens; a v1 one ('.../oauth2/authorize') issues '${AZURE_STS_HOST}' tokens. ` + + `Pin the form that matches 'authorizationUrl'.` + ); + } + + // Collapse into the tenant-exclusive case above: verify against that one + // tenant's own, non-shared JWKS endpoint — never the shared alias pool. + // Advisory only: a throwing logger must not abort issuer binding. + try { + logger?.info?.( + `OAuth provider '${providerName}' (azure): pinned issuer resolves to tenant '${pinnedGuid}'; verifying ` + + `against that tenant's own JWKS endpoint instead of the shared '${lowerSegment}' key set.` + ); + } catch { + /* advisory log only */ + } + config.jwksUri = azureTenantUri('keys', pinnedGuid); + // Canonicalized WITHIN the pin's own form (case, trailing slash) — never + // rewritten to the other form, same rationale as the real-tenant-GUID + // case above. + config.issuer = azureCanonicalIssuerForm(config.issuer as string, pinnedGuid); +} diff --git a/src/lib/config.ts b/src/lib/config.ts index c359c75..09d894c 100644 --- a/src/lib/config.ts +++ b/src/lib/config.ts @@ -9,6 +9,7 @@ import { getProvider } from './providers/index.ts'; import { redactSecrets } from './redact.ts'; import { algFromPrivateKeyPem } from './mcp/keyStore.ts'; import { isCimdClientId, cimdEnabled } from './mcp/cimd.ts'; +import { resolveAzureIssuerBinding, isAzureJwksUri, AzureIssuerBindingError } from './azureIssuer.ts'; import type { OAuthProviderConfig, OAuthPluginConfig, ProviderRegistry, Logger } from '../types.ts'; /** @@ -555,13 +556,16 @@ export function skipUndefined(source: Record | null | undefined): R * `true` only from a static, startup-time build (`initializeProviders`), never * from a request-path dynamic resolution (`onResolveProvider`): throwing there * would fail every request for a misconfigured tenant instead of just leaving - * it non-adoption-eligible. + * it non-adoption-eligible. `logger`, when passed, only reaches advisory + * warnings (`checkGraphProfileScope`) and `resolveAzureIssuerBinding`'s own + * info log — nothing here throws because of a missing logger. */ export function buildProviderConfig( providerConfig: Record, providerName: string, pluginDefaults: Partial = {}, - enforceIssuerForJwks = false + enforceIssuerForJwks = false, + logger?: Logger ): OAuthProviderConfig { const options = providerConfig || {}; @@ -644,31 +648,63 @@ export function buildProviderConfig( redirectUri, }; - // Handle provider-specific configuration - if (providerPreset?.configure) { - let providerConfig; - - switch (config.provider) { + // Handle provider-specific configuration. Dispatches on the *resolved + // preset's own* `provider` field, not `config.provider` — an explicit + // `provider: 'microsoft'` option overwrites `config.provider` with that + // alias string (via the `...expandedOptions` spread above) even though + // `getProvider('microsoft')` already resolved the Azure preset; switching + // on `config.provider` would silently skip `configure()` for that case. + // Gated on `config.provider` being non-empty, though: an explicit + // `provider: ''` is a deliberate escape hatch (keep the preset's other + // defaults but never run its `configure()`), and must still suppress + // dispatch even though `providerPreset` still resolved. + if (config.provider && providerPreset?.configure) { + let derivedConfig; + + switch (providerPreset.provider) { case 'azure': if (expandedOptions.tenantId) { - providerConfig = providerPreset.configure(expandedOptions.tenantId); + derivedConfig = providerPreset.configure(expandedOptions.tenantId); } break; case 'auth0': case 'okta': if (expandedOptions.domain) { - providerConfig = providerPreset.configure(expandedOptions.domain); + derivedConfig = providerPreset.configure(expandedOptions.domain, expandedOptions.authServer); } break; } - if (providerConfig) { - Object.assign(config, providerConfig); + if (derivedConfig) { + // An explicit, usable operator `issuer` wins over a shortcut-derived + // one (#264) — narrowly: every other `configure()`-returned field + // (the endpoint URLs) keeps today's unconditional-overwrite behavior, + // so a `domain`/`tenantId` shortcut's endpoints stay mutually + // coherent rather than mixing a stale explicit endpoint with + // freshly-derived others. "Usable" excludes `null`/`''`/an + // all-empty array (same as `hasUsableIssuer` below) and an + // unresolved `${VAR}` placeholder left by `expandEnvVar` when the + // variable is unset — otherwise that literal placeholder string would + // win over the shortcut-derived issuer and fail every login. + const pinnedIssuer = expandedOptions.issuer; + Object.assign(config, derivedConfig); + if (isPinnedIssuerUsable(pinnedIssuer)) { + config.issuer = pinnedIssuer; + } } } + // Azure issuer binding (#264): a real tenant GUID's issuer is fully + // determined by its own jwksUri; a shared alias authority (/common, + // /organizations, /consumers) only becomes usable when the operator pins + // a single Azure-shaped tenant issuer, in which case verification is + // redirected to that tenant's own, non-shared endpoint. A no-op for any + // non-Azure-shaped jwksUri. Throws for an unsafe/malformed combination. + resolveAzureIssuerBinding(config, providerName, logger); + if (enforceIssuerForJwks) { - validateIssuerForJwks(config, providerName, providerPreset); + validateIssuerForJwks(config, providerName); + checkGraphProfileScope(config, providerName, logger); } return config; @@ -684,39 +720,153 @@ function hasUsableIssuer(issuer: OAuthProviderConfig['issuer']): boolean { return typeof issuer === 'string' && issuer !== ''; } +/** + * {@link hasUsableIssuer}, plus rejecting an unresolved `${VAR}` placeholder + * (string, or any array element) — used only for the explicit-pin-wins-over- + * shortcut check above, so a placeholder `expandEnvVar` left untouched + * because the variable is unset can't win over a `domain`/`tenantId` + * shortcut's derived issuer and then fail every login against that literal + * placeholder text. + */ +function isPinnedIssuerUsable(issuer: OAuthProviderConfig['issuer']): boolean { + const values = Array.isArray(issuer) ? issuer : [issuer]; + return values.some((value) => typeof value === 'string' && value !== '' && !isUnresolvedEnvPlaceholder(value)); +} + +/** + * True when a JWKS-enabled, issuer-less provider is a candidate for + * background OIDC discovery (HarperFast/oauth#264): `jwksUri` is set, no + * usable `issuer` resulted (from a preset shortcut or an explicit pin — + * includes the Azure cases `resolveAzureIssuerBinding` already resolved), + * `authorizationUrl` parses as `https:` (discovery must not depend on the + * network for a config that's wrong independent of it — a non-https + * `authorizationUrl` can never be fixed by a fetch), and the endpoint isn't + * Azure's own host (an *unpinned* alias authority is intentionally + * issuer-less — generic discovery must never probe it; see `azureIssuer.ts`). + */ +export function needsIssuerDiscovery(config: OAuthProviderConfig): boolean { + if (!config.jwksUri) return false; + if (hasUsableIssuer(config.issuer)) return false; + if (isAzureJwksUri(config.jwksUri)) return false; + try { + return new URL(config.authorizationUrl).protocol === 'https:'; + } catch { + return false; + } +} + /** * A JWKS-enabled provider (so `issuerValidated` can in principle be `true` — * HarperFast/oauth#231 §4) with no usable `issuer` would otherwise deny * adoption silently at login, forever. `domain`/`tenantId` already derive - * `issuer` for Okta/Auth0/Azure; this only fires when that's bypassed. + * `issuer` for Okta/Auth0/Azure; `resolveAzureIssuerBinding` derives it for + * a pinned Azure alias or a real tenant GUID (HarperFast/oauth#264); this + * only fires when all of that is bypassed. + * + * Boot must not depend on the network: a provider `needsIssuerDiscovery` + * defers to that background attempt (`src/index.ts` starts it once this + * provider is published) instead of throwing — only a config that's wrong + * *independent* of the network (no `authorizationUrl`, or a non-`https` one) + * still hard-fails here, exactly as before #264. * - * Azure is excluded by provider type, not just its `/common` default: single- - * tenant Azure configs with explicit endpoints and no `issuer` are unchanged - * by this PR (tracked as a known gap in HarperFast/oauth#264, not fixed - * here). Checked against the - * preset's own `provider` field (`providerPreset`), not `config.provider` — - * the `microsoft` alias resolves to the Azure preset but an explicit - * `provider: 'microsoft'` option carries that string into `config.provider`. + * Excluded by `jwksUri` shape, not declared provider type: any Azure-host + * `jwksUri` that still has no usable `issuer` at this point can only be an + * *unpinned* alias authority — `resolveAzureIssuerBinding` above already + * resolved a pinned alias or a real tenant GUID to a usable issuer, or + * thrown for anything unsafe/malformed. An unpinned alias is intentionally + * issuer-less (HarperFast/oauth#264 §"adoption requires a pin") and must not + * hard-fail here — regardless of whether the config says `provider: azure`, + * uses the `microsoft` alias, or is a `generic`/explicit-endpoint provider + * manually pointed at the same host. */ -function validateIssuerForJwks( - config: OAuthProviderConfig, - providerName: string, - providerPreset: OAuthProviderConfig | null -): void { +function validateIssuerForJwks(config: OAuthProviderConfig, providerName: string): void { if (!config.jwksUri) return; - if (config.provider === 'azure' || providerPreset?.provider === 'azure') return; if (hasUsableIssuer(config.issuer)) return; + if (isAzureJwksUri(config.jwksUri)) return; + if (needsIssuerDiscovery(config)) return; throw new Error( - `OAuth provider '${providerName}' (${config.provider}) has a 'jwksUri' but no usable 'issuer'. ` + - `Without a validated issuer, ID token signature verification still runs but 'issuerValidated' is ` + - `always false, so this provider's logins can never satisfy the account-adoption gate (a hookless ` + - `login that should adopt an existing Harper account is silently denied instead). Set 'issuer' ` + - `explicitly on provider '${providerName}' (e.g. your OIDC server's issuer URI), or use the preset's ` + - `'domain'/'tenantId' shortcut if you're not already, which derives it for you.` + `OAuth provider '${providerName}' (${config.provider}) has a 'jwksUri' but no usable 'issuer', and OIDC ` + + `discovery cannot run for it (its 'authorizationUrl' is missing or not https). Without a validated ` + + `issuer, ID token signature verification still runs but 'issuerValidated' is always false, so this ` + + `provider's logins can never satisfy the account-adoption gate. Set 'issuer' explicitly on provider ` + + `'${providerName}' (e.g. your OIDC server's issuer URI), or use the preset's 'domain'/'tenantId' ` + + `shortcut if you're not already, which derives it for you.` ); } +/** + * The exact condition `OAuthProvider`'s private `isAzureGraphUserInfo` checks + * — `config.provider === 'azure'`, **or** `userInfoUrl`'s host is + * `graph.microsoft.com` — duplicated here (deliberately, not imported: it's + * a private method) so this startup check warns for exactly the same + * configs that method's alternate `id`/`oid` correlation applies to. + */ +function isAzureGraphUserInfoConfig(config: OAuthProviderConfig): boolean { + if (config.provider === 'azure') return true; + try { + return new URL(config.userInfoUrl).hostname === 'graph.microsoft.com'; + } catch { + return false; + } +} + +/** + * Warns (never throws) when a statically configured provider's `fetchEmail` + * correlation against Microsoft Graph's non-OIDC `/v1.0/me` can never + * succeed: that correlation needs the ID token's `oid` claim, which Azure + * only includes when the `profile` scope was requested (#263; the Azure + * preset's default `scope` already includes it). A custom `scope` that + * drops `profile` silently loses the correlation at login time with no + * indication why — this surfaces it at startup instead. Advisory only: it + * degrades a UserInfo correlation, not the account-adoption gate's safety, + * so a throwing logger must not abort provider initialization either. + */ +function checkGraphProfileScope(config: OAuthProviderConfig, providerName: string, logger?: Logger): void { + if (!config.fetchEmail) return; + if (!isAzureGraphUserInfoConfig(config)) return; + // A YAML list (`scope: [openid, profile, ...]`) survives expandEnvVarsDeep + // as an array, not a string — this function is documented as "never + // throws", so a non-string `scope` must be skipped, not crash `.split()`. + if (config.scope != null && typeof config.scope !== 'string') return; + const scopes = (config.scope ?? '').split(/\s+/).filter(Boolean); + if (scopes.includes('profile')) return; + try { + logger?.warn?.( + `OAuth provider '${providerName}': 'fetchEmail' is enabled against a Microsoft Graph UserInfo endpoint, ` + + `but 'scope' does not include 'profile'. Azure only includes the 'oid' claim on the ID token when ` + + `'profile' is requested, and 'oid' is required to correlate Graph's '/v1.0/me' response (which has ` + + `no 'sub') with the ID token — without it, that fallback discards the UserInfo response instead of ` + + `merging the email. Add 'profile' to provider '${providerName}''s 'scope'.` + ); + } catch { + /* a throwing logger must not abort provider initialization */ + } +} + +/** + * Re-derive OIDC discovery targets from an *already-built* provider + * registry (HarperFast/oauth#264) — a pure function with no side effects. + * Call only after the registry is confirmed published (see `src/index.ts`); + * this function itself never starts a fetch. + */ +export function collectDiscoveryTargets( + providers: ProviderRegistry +): Array<{ authorizationUrl: string; jwksUri: string; tokenUrl: string; providerName: string }> { + const targets: Array<{ authorizationUrl: string; jwksUri: string; tokenUrl: string; providerName: string }> = []; + for (const [providerName, { config }] of Object.entries(providers)) { + if (needsIssuerDiscovery(config)) { + targets.push({ + authorizationUrl: config.authorizationUrl, + jwksUri: config.jwksUri!, + tokenUrl: config.tokenUrl, + providerName, + }); + } + } + return targets; +} + /** * Extract plugin-level defaults from options */ @@ -799,7 +949,36 @@ export function initializeProviders(options: OAuthPluginConfig, logger?: Logger) // enforceIssuerForJwks (#231 §4) only runs here, after the configured-ness // precheck above (#259/#260) — an unconfigured or half-configured provider // is already skipped and never reaches the issuer check. - const config = buildProviderConfig(providerConfig, providerName, pluginDefaults, /* enforceIssuerForJwks */ true); + // + // An AzureIssuerBindingError means only THIS provider's own `issuer` pin + // is invalid (a stale pin, one copied from docs, one missing `/v2.0`) — + // it says nothing about whether every other declared provider is safe to + // start, so it gets the same "skip the bad one, keep the others" ##259/ + // #260 already established, logged as an error rather than silently + // dropped. Every other `buildProviderConfig` failure (e.g. #231 §4's + // issuer-required check, a bad `redirectUri`) is unrelated to this one + // provider's pin and keeps today's fail-the-whole-reload behavior — + // only this specific, newly-introduced failure mode is downgraded. + let config: OAuthProviderConfig; + try { + config = buildProviderConfig( + providerConfig, + providerName, + pluginDefaults, + /* enforceIssuerForJwks */ true, + logger + ); + } catch (error) { + if (error instanceof AzureIssuerBindingError) { + try { + logger?.error?.(`OAuth provider '${providerName}' skipped — invalid Azure issuer pin:`, error); + } catch { + /* advisory log only — a throwing logger must not turn a skip into a failed initializeProviders */ + } + continue; + } + throw error; + } // Check if this provider is properly configured const requiredFields = ['clientId', 'clientSecret', 'authorizationUrl', 'tokenUrl', 'userInfoUrl']; diff --git a/src/lib/discovery.ts b/src/lib/discovery.ts new file mode 100644 index 0000000..bf48f28 --- /dev/null +++ b/src/lib/discovery.ts @@ -0,0 +1,416 @@ +/** + * OIDC Discovery 1.0 — deriving an ID-token issuer for a statically + * configured, explicit-endpoint JWKS provider that has no usable `issuer` + * (HarperFast/oauth#264). Presets (`domain`/`tenantId`) already derive an + * issuer directly for the common shortcuts; this module covers the case + * where an operator bypassed those shortcuts with explicit endpoints. + * + * Fetch safety: reuses `fetchPinnedBoundedJson` (pinned-connect, no-redirect, + * size- and time-bounded) — no new fetch primitive. Opts out of its + * private/loopback-address block (`allowPrivateAddresses`): the endpoints + * discovered against here are operator-configured, not attacker-controlled, + * the same trust level as `jwksUri` itself (which `jwks-rsa` fetches with no + * SSRF gate at all) — unlike CIMD's own `client_id` input, which that block + * exists for. + * + * Validation (per OIDC Discovery 1.0 §4.3 and #264's own design): a + * candidate issuer is accepted only when its document's `issuer` equals the + * exact URL prefix it was fetched from (root accepts the documented + * trailing-slash exception), `authorization_endpoint` exactly equals the + * configured `authorizationUrl`, `jwks_uri` exactly equals the configured + * `jwksUri`, and `token_endpoint` exactly equals the configured `tokenUrl` + * (required by OIDC Discovery for the authorization-code flow this plugin + * uses — a document that omits it refuses too, never bypassing the check). + * Every candidate is constructed from the configured `authorizationUrl`'s + * own origin, so cross-origin is structurally impossible, not just checked. + * + * Performance/safety contract: + * - Never blocks boot: `startIssuerDiscovery` is fire-and-forget, called only + * after a provider registry is *published* (see `src/index.ts`), never + * from config building. + * - Never fetches per request: dynamic (`onResolveProvider`) providers only + * ever read the cache (`awaitDiscoveredIssuer`), keyed by the full + * validated tuple so they can share a result with a static provider that + * already resolved the same endpoints — never starting a fetch of their + * own. + * - The first adoption-eligible login (one whose ID-token signature already + * verified — `OAuthProvider.verifyIdToken` awaits this only *after* that) + * awaits the in-flight attempt with a short, explicit bound + * (`DISCOVERY_LOGIN_AWAIT_BOUND_MS`); any other login during the same + * pending window does not wait, by #264's own design. + * - Bounded everywhere: `MAX_DISCOVERY_PREFIXES` ancestor path-prefixes, + * `DISCOVERY_OVERALL_BUDGET_MS` total wall-clock per discovery attempt + * (checked only between fully-awaited fetches — never racing an + * outstanding one), `MAX_CONCURRENT_DISCOVERY_ATTEMPTS` in-flight at once. + * - Cached for the process lifetime on success; a failure is retried at most + * once per `RETRY_COOLDOWN_MS`, and only from a fresh `startIssuerDiscovery` + * call (a live config reload) — never from the read path. + * - Fully failure-contained: nothing in this module can produce an unhandled + * rejection or let a throwing logger escape a caller. + */ + +import { + fetchPinnedBoundedJson, + CimdClientError, + DEFAULT_FETCH_TIMEOUT_MS, + DEFAULT_MAX_DOCUMENT_BYTES, +} from './mcp/cimd.ts'; +import type { Logger } from '../types.ts'; + +/** Bound on a first adoption-eligible login awaiting an in-flight discovery attempt. */ +export const DISCOVERY_LOGIN_AWAIT_BOUND_MS = 3_000; +/** Total wall-clock budget for one discovery attempt, across every candidate prefix. */ +export const DISCOVERY_OVERALL_BUDGET_MS = 15_000; +/** Ancestor path-prefixes tried, longest first, before giving up. */ +export const MAX_DISCOVERY_PREFIXES = 10; +/** Minimum spacing between a failed discovery attempt and the next one for the same key. */ +export const RETRY_COOLDOWN_MS = 5 * 60_000; +/** Discovery attempts allowed in flight at once, across every provider. */ +export const MAX_CONCURRENT_DISCOVERY_ATTEMPTS = 8; + +/** Delay before retrying a fetch that only failed because CIMD's shared DNS-lookup permit (2, process-wide) was busy — never a "no document here" signal. */ +export const CAPACITY_RETRY_DELAY_MS = 50; + +// --- Injected timeouts for testing (mirrors cimd.ts's _setFetch/_setDnsLookup seams) --- +let _perFetchTimeoutMs = DEFAULT_FETCH_TIMEOUT_MS; +let _overallBudgetMs = DISCOVERY_OVERALL_BUDGET_MS; +let _retryCooldownMs = RETRY_COOLDOWN_MS; +let _capacityRetryDelayMs = CAPACITY_RETRY_DELAY_MS; + +/** Override the per-fetch, overall-budget, retry-cooldown, and capacity-retry-delay timeouts (tests only); `null` restores the defaults. @internal */ +export function _setDiscoveryTimeouts( + overrides: { + perFetchMs?: number; + overallBudgetMs?: number; + retryCooldownMs?: number; + capacityRetryDelayMs?: number; + } | null +): void { + _perFetchTimeoutMs = overrides?.perFetchMs ?? DEFAULT_FETCH_TIMEOUT_MS; + _overallBudgetMs = overrides?.overallBudgetMs ?? DISCOVERY_OVERALL_BUDGET_MS; + _retryCooldownMs = overrides?.retryCooldownMs ?? RETRY_COOLDOWN_MS; + _capacityRetryDelayMs = overrides?.capacityRetryDelayMs ?? CAPACITY_RETRY_DELAY_MS; +} + +interface DiscoveryEntry { + promise: Promise; + /** `undefined` while pending; the settled value (success or `null`) once known. */ + result?: string | null; + /** Whether some login has already consumed the one bounded-await slot for this key. */ + awaited: boolean; + settledAt?: number; +} + +const discoveryCache = new Map(); +let activeDiscoveryAttempts = 0; +const discoveryQueue: Array<() => void> = []; + +/** Key space is operator-configured endpoints, not request input — no eviction needed (unlike CIMD's attacker-keyed cache). */ +function cacheKey(authorizationUrl: string, jwksUri: string, tokenUrl: string): string { + return `${authorizationUrl}\n${jwksUri}\n${tokenUrl}`; +} + +/** Clear all discovery state (tests only). @internal */ +export function _clearDiscoveryCache(): void { + discoveryCache.clear(); + activeDiscoveryAttempts = 0; + discoveryQueue.length = 0; +} + +/** + * Ancestor path-prefixes of `pathname`, longest first, root last (`''`), + * capped at `MAX_DISCOVERY_PREFIXES`. E.g. `/oauth2/id/v1/authorize` -> + * `['/oauth2/id/v1', '/oauth2/id', '/oauth2', '']`. + * + * Root (`''`) is always included, even when capping: a `pathname` of `/` + * itself has zero segments, so the drop-one-segment-at-a-time loop below + * would otherwise never run at all and skip root entirely; a `pathname` + * deeper than the cap would otherwise only ever try its longest prefixes + * and never reach root, which is often exactly where a provider's discovery + * document actually lives. + */ +function candidatePrefixes(pathname: string): string[] { + const segments = pathname.split('/').filter(Boolean); + if (segments.length === 0) return ['']; + const all: string[] = []; + for (let drop = 1; drop <= segments.length; drop++) { + const remaining = segments.slice(0, segments.length - drop); + all.push(remaining.length === 0 ? '' : '/' + remaining.join('/')); + } + if (all.length <= MAX_DISCOVERY_PREFIXES) return all; + return [...all.slice(0, MAX_DISCOVERY_PREFIXES - 1), all[all.length - 1]]; +} + +/** `true` only for CIMD's shared, process-wide DNS-lookup-permit rejection — never a real transport/validation failure. */ +function isDnsCapacityRejection(error: unknown): boolean { + return error instanceof CimdClientError && error.oauthError === 'temporarily_unavailable'; +} + +/** + * Fetch one discovery document, deadlined at `deadlineAt` (an absolute + * `Date.now()`-style timestamp — not a duration, so retries below share one + * budget instead of each restarting a fresh per-fetch timeout). A DNS + * capacity rejection (CIMD's `checkHostSsrf` shares a process-wide, + * 2-permit DNS-lookup gate with every other CIMD/discovery caller; with + * several discovery attempts started at once, the 3rd and later synchronously + * see the permit already taken) retries the SAME fetch after a short delay + * instead of being treated as "no document at this prefix" — that gate is a + * transient capacity signal, not a result about this endpoint, so advancing + * to the next (shorter) prefix on it would reach a wrong, and wrongly + * CACHED, "not found" outcome purely from how many other discovery attempts + * happened to start at the same moment. Returns `null` for every other + * failure (transport, timeout, non-JSON, oversize, a genuinely exhausted + * deadline) exactly as before. + */ +interface DiscoveryDocumentResult { + doc: any | null; + /** `true` only when the deadline ran out while every attempt was still hitting the DNS capacity gate — never a real negative result about this endpoint. */ + capacityExhausted: boolean; +} + +async function fetchDiscoveryDocument( + discoveryUrl: string, + deadlineAt: number, + logger?: Logger +): Promise { + while (true) { + const remaining = deadlineAt - Date.now(); + if (remaining <= 0) return { doc: null, capacityExhausted: false }; + try { + const { body } = await fetchPinnedBoundedJson(discoveryUrl, { + label: 'OIDC discovery document', + tag: 'OIDC discovery', + accept: 'application/json', + contentTypes: ['application/json'], + timeoutMs: Math.min(_perFetchTimeoutMs, remaining), + maxBytes: DEFAULT_MAX_DOCUMENT_BYTES, + logger, + // `authorizationUrl`/`jwksUri` are operator-configured, not + // attacker-controlled (unlike CIMD's own `client_id`) — the same + // trust level as `jwksUri` itself, which `jwks-rsa` already fetches + // with no SSRF gate at all. Blocking a private/loopback address here + // would only break discovery for a self-hosted IdP on a private + // network for no safety benefit over that already-ungated fetch. + allowPrivateAddresses: true, + }); + const doc = JSON.parse(body); + return { doc: doc && typeof doc === 'object' && !Array.isArray(doc) ? doc : null, capacityExhausted: false }; + } catch (error) { + if (isDnsCapacityRejection(error)) { + if (remaining > _capacityRetryDelayMs) { + await new Promise((resolve) => setTimeout(resolve, _capacityRetryDelayMs)); + continue; + } + return { doc: null, capacityExhausted: true }; + } + return { doc: null, capacityExhausted: false }; // Transport, timeout, non-JSON, oversize, etc. + } + } +} + +async function discoverIssuerUnsafe( + authorizationUrl: string, + jwksUri: string, + tokenUrl: string, + providerName: string, + logger?: Logger +): Promise { + let url: URL; + try { + url = new URL(authorizationUrl); + } catch { + return null; + } + const startedAt = Date.now(); + const deadlineAt = startedAt + _overallBudgetMs; + let lastMismatch: string | undefined; + let lastCapacityExhausted = false; + for (const prefix of candidatePrefixes(url.pathname)) { + if (Date.now() >= deadlineAt) break; + const issuerCandidates = prefix === '' ? [url.origin, `${url.origin}/`] : [`${url.origin}${prefix}`]; + const discoveryUrl = + prefix === '' + ? `${url.origin}/.well-known/openid-configuration` + : `${url.origin}${prefix}/.well-known/openid-configuration`; + + const { doc, capacityExhausted } = await fetchDiscoveryDocument(discoveryUrl, deadlineAt, logger); + if (!doc) { + lastCapacityExhausted = capacityExhausted; + continue; + } + lastCapacityExhausted = false; + + if (!issuerCandidates.includes(doc.issuer)) { + lastMismatch = `issuer ${JSON.stringify(doc.issuer)} does not match ${JSON.stringify(issuerCandidates[0])}`; + continue; + } + if (doc.authorization_endpoint !== authorizationUrl) { + lastMismatch = `authorization_endpoint ${JSON.stringify(doc.authorization_endpoint)} does not match the configured authorizationUrl ${JSON.stringify(authorizationUrl)}`; + continue; + } + if (doc.jwks_uri !== jwksUri) { + lastMismatch = `jwks_uri ${JSON.stringify(doc.jwks_uri)} does not match the configured jwksUri ${JSON.stringify(jwksUri)}`; + continue; + } + // OIDC Discovery requires token_endpoint for the authorization-code flow + // this plugin uses — an absent one is a non-compliant document, not a + // safer bypass of the check, so it refuses the same as a mismatched one. + if (doc.token_endpoint !== tokenUrl) { + lastMismatch = `token_endpoint ${JSON.stringify(doc.token_endpoint)} does not match the configured tokenUrl ${JSON.stringify(tokenUrl)}`; + continue; + } + + try { + logger?.info?.( + `OIDC discovery for provider '${providerName}': issuer derived via discovery: ${doc.issuer} — add issuer: ${doc.issuer} to pin it.` + ); + } catch { + /* never let a throwing logger discard a successful result */ + } + return doc.issuer; + } + + try { + const cause = lastCapacityExhausted + ? "the shared DNS-resolution capacity (CIMD's process-wide lookup permit) never freed up within the discovery budget — likely several discovery attempts starting at once; this will retry on the next reload" + : (lastMismatch ?? 'no .well-known/openid-configuration document was found matching the configured endpoints'); + logger?.warn?.( + `OIDC discovery failed for provider '${providerName}' at ${url.origin}: ${cause}. ` + + `Set 'issuer' explicitly on provider '${providerName}' to your OIDC server's issuer URI.` + ); + } catch { + /* never let a throwing logger escape discovery */ + } + return null; +} + +/** Never rejects — any unexpected failure resolves to `null` after a best-effort, guarded log. */ +async function discoverIssuer( + authorizationUrl: string, + jwksUri: string, + tokenUrl: string, + providerName: string, + logger?: Logger +): Promise { + try { + return await discoverIssuerUnsafe(authorizationUrl, jwksUri, tokenUrl, providerName, logger); + } catch (error) { + try { + logger?.error?.(`OIDC discovery crashed for provider '${providerName}':`, error); + } catch { + /* never let a throwing logger escape discovery */ + } + return null; + } +} + +function runQueued(): void { + while (activeDiscoveryAttempts < MAX_CONCURRENT_DISCOVERY_ATTEMPTS && discoveryQueue.length > 0) { + discoveryQueue.shift()!(); + } +} + +/** + * Start (or retry, after `RETRY_COOLDOWN_MS` since a prior failure) a + * bounded, cached discovery attempt for one `(authorizationUrl, jwksUri, + * tokenUrl)` tuple. Fire-and-forget: never throws, never blocks the caller. + * Call only after the provider using these endpoints has been fully built + * and published — never for a config that might still be discarded. + */ +export function startIssuerDiscovery( + authorizationUrl: string, + jwksUri: string, + tokenUrl: string, + providerName: string, + logger?: Logger +): void { + try { + const key = cacheKey(authorizationUrl, jwksUri, tokenUrl); + const existing = discoveryCache.get(key); + if (existing) { + const stale = + existing.result === null && + existing.settledAt !== undefined && + Date.now() - existing.settledAt >= _retryCooldownMs; + if (!stale) return; + } + + try { + logger?.debug?.(`OIDC discovery: starting for provider '${providerName}' at ${authorizationUrl}`); + } catch { + /* never let a throwing logger break discovery kickoff */ + } + + const run = (): Promise => { + activeDiscoveryAttempts++; + return discoverIssuer(authorizationUrl, jwksUri, tokenUrl, providerName, logger).finally(() => { + activeDiscoveryAttempts--; + runQueued(); + }); + }; + const scheduled = new Promise((resolve) => { + const task = (): void => { + run().then(resolve, () => resolve(null)); + }; + if (activeDiscoveryAttempts < MAX_CONCURRENT_DISCOVERY_ATTEMPTS) task(); + else discoveryQueue.push(task); + }); + + const entry: DiscoveryEntry = { promise: scheduled, awaited: false }; + scheduled.then( + (value) => { + entry.result = value; + entry.settledAt = Date.now(); + }, + () => { + entry.result = null; + entry.settledAt = Date.now(); + } + ); + discoveryCache.set(key, entry); + } catch (error) { + try { + logger?.error?.(`OIDC discovery: failed to start for provider '${providerName}':`, error); + } catch { + /* swallow */ + } + } +} + +/** + * Read (and, for the first caller only, bounded-await) the discovery result + * for one `(authorizationUrl, jwksUri, tokenUrl)` tuple. Performs **zero** + * network I/O itself — a cache miss (no static provider ever started + * discovery for this exact tuple) returns `null` immediately, which is how a + * dynamically-resolved provider reaches this function safely. + */ +export async function awaitDiscoveredIssuer( + authorizationUrl: string, + jwksUri: string, + tokenUrl: string, + logger?: Logger, + boundMs: number = DISCOVERY_LOGIN_AWAIT_BOUND_MS +): Promise { + const key = cacheKey(authorizationUrl, jwksUri, tokenUrl); + const entry = discoveryCache.get(key); + if (!entry) { + try { + logger?.debug?.( + `OIDC discovery: no cached result for ${authorizationUrl}; proceeding without a validated issuer for this login.` + ); + } catch { + /* never let a throwing logger break verification */ + } + return null; + } + if (entry.result !== undefined) return entry.result; + if (entry.awaited) return null; + entry.awaited = true; + return new Promise((resolve) => { + const timer = setTimeout(() => resolve(null), boundMs); + entry.promise.then((value) => { + clearTimeout(timer); + resolve(value); + }); + }); +} diff --git a/src/lib/dynamicProviderCache.ts b/src/lib/dynamicProviderCache.ts index daf4158..a7db6b4 100644 --- a/src/lib/dynamicProviderCache.ts +++ b/src/lib/dynamicProviderCache.ts @@ -21,7 +21,7 @@ * (unset) — DEFAULT_DYNAMIC_PROVIDER_CACHE_TTL_SECONDS (bounded; see below) */ -import type { ProviderRegistryEntry } from '../types.ts'; +import type { Logger, ProviderRegistryEntry } from '../types.ts'; /** * Default TTL (seconds) when `cacheDynamicProviders` is not set. Bounded rather @@ -30,14 +30,31 @@ import type { ProviderRegistryEntry } from '../types.ts'; */ export const DEFAULT_DYNAMIC_PROVIDER_CACHE_TTL_SECONDS = 300; +/** + * Fixed, short cooldown for a dynamically-resolved provider's invalid Azure + * issuer pin (HarperFast/oauth#264/#271) — independent of the success TTL + * above, and not configurable: a config error should recover fast once + * fixed, which the (often much longer, or infinite) success TTL is wrong + * for. Without this, a bad pin re-runs the resolve hook and re-throws on + * every single request for that provider until an operator notices. + */ +const AZURE_PIN_FAILURE_COOLDOWN_MS = 30_000; + interface CacheEntry { entry: ProviderRegistryEntry; cachedAt: number; } +interface AzurePinFailure { + message: string; + failedAt: number; +} + export class DynamicProviderCache { private cache = new Map(); private ttlMs: number; + private azurePinFailures = new Map(); + private warnedOnce = new Set(); constructor(ttl: boolean | number = DEFAULT_DYNAMIC_PROVIDER_CACHE_TTL_SECONDS) { this.ttlMs = DynamicProviderCache.parseTTL(ttl); @@ -70,6 +87,8 @@ export class DynamicProviderCache { clear(): void { this.cache.clear(); + this.azurePinFailures.clear(); + this.warnedOnce.clear(); } updateTTL(ttl: boolean | number): void { @@ -80,4 +99,74 @@ export class DynamicProviderCache { get size(): number { return this.cache.size; } + + /** + * Record a dynamically-resolved provider's `AzureIssuerBindingError` so + * {@link getAzurePinFailure} can short-circuit the resolve hook for + * `AZURE_PIN_FAILURE_COOLDOWN_MS` instead of re-running (and re-throwing) + * it on every request for `name` until the cooldown elapses. + */ + recordAzurePinFailure(name: string, message: string): void { + this.azurePinFailures.set(name, { message, failedAt: Date.now() }); + } + + /** The still-cooling-down `AzureIssuerBindingError` message for `name`, or `undefined` if there isn't one or it has expired. */ + getAzurePinFailure(name: string): string | undefined { + const failure = this.azurePinFailures.get(name); + if (!failure) return undefined; + if (Date.now() - failure.failedAt > AZURE_PIN_FAILURE_COOLDOWN_MS) { + this.azurePinFailures.delete(name); + return undefined; + } + return failure.message; + } + + /** Clear any cooling-down Azure-pin failure for `name` — called on a successful resolution. */ + clearAzurePinFailure(name: string): void { + this.azurePinFailures.delete(name); + } + + /** + * True the first time `name`+`message` is seen, `false` every time after + * (until {@link clear} runs) — records the pair as seen either way. Used + * by {@link wrapLoggerForDynamicResolution} to stop an advisory warning + * from `buildProviderConfig` (e.g. the Azure tenant-mismatch warning) + * from repeating on every dynamic resolution of the same provider — which, + * with `cacheDynamicProviders: false` or a short TTL, can mean every + * single request. Independent of the success-cache TTL and the Azure-pin + * cooldown above: once warned, a provider stays quiet about that exact + * message for the life of this cache instance, not just one TTL window. + */ + private shouldWarnOnce(name: string, message: string): boolean { + const key = `${name}\u0000${message}`; + if (this.warnedOnce.has(key)) return false; + this.warnedOnce.add(key); + return true; + } + + /** + * Wrap `logger` so `warn` calls for `providerName` made while building its + * config dynamically (`onResolveProvider`) are deduped via + * {@link shouldWarnOnce} — the first occurrence of each distinct message + * logs; later ones are silently dropped. `info`/`error`/`debug` pass + * through unchanged: those are either one-shot events (a successful + * resolution) or already covered by their own cooldown (`AzureIssuerBindingError`, + * via {@link recordAzurePinFailure}), not messages that repeat identically + * on every request. + */ + wrapLoggerForDynamicResolution(providerName: string, logger?: Logger): Logger | undefined { + if (!logger) return logger; + // Bind each method explicitly rather than `{ ...logger }` — a + // class-based logger's methods usually live on its prototype, not as + // its own properties, so a spread would silently drop `info`/`error`/ + // `debug` (only `warn`, replaced below, would survive). + return { + info: (message: string, ...args: any[]) => logger.info?.(message, ...args), + error: (message: string, ...args: any[]) => logger.error?.(message, ...args), + debug: (message: string, ...args: any[]) => logger.debug?.(message, ...args), + warn: (message: string, ...args: any[]) => { + if (this.shouldWarnOnce(providerName, message)) logger.warn?.(message, ...args); + }, + }; + } } diff --git a/src/lib/mcp/cimd.ts b/src/lib/mcp/cimd.ts index 8361a74..10b86fa 100644 --- a/src/lib/mcp/cimd.ts +++ b/src/lib/mcp/cimd.ts @@ -463,11 +463,27 @@ async function boundedDnsLookup(hostname: string): Promise { } /** - * DNS SSRF gate: resolves all addresses for `hostname`, rejects if any falls - * outside the allowed (global unicast) ranges, and returns the validated set so - * the caller can PIN the connection to it (closing the rebind TOCTOU). + * DNS SSRF gate: resolves all addresses for `hostname` and returns the + * validated set so the caller can PIN the connection to it (closing the + * rebind TOCTOU). Rejects an address outside the allowed (global unicast) + * ranges unless `allowPrivateAddresses` is set. + * + * `allowPrivateAddresses` exists for exactly one caller (OIDC discovery, + * `discovery.ts`): that endpoint is already operator-configured input — the + * same trust level as `jwksUri`, which `jwks-rsa` fetches with no SSRF gate + * at all — not attacker-controlled input the way a CIMD `client_id` is. + * Blocking a private/loopback address there only breaks self-hosted IdPs on + * a private network (Docker, a VPC) with a misleading "could not be + * resolved" message, for no safety benefit over the ungated JWKS fetch the + * same config already makes. An unclassified address family still fails + * closed either way — that's a resolver-sanity check, not part of the SSRF + * range gate itself. */ -async function checkHostSsrf(hostname: string, logger?: Logger): Promise { +async function checkHostSsrf( + hostname: string, + logger?: Logger, + allowPrivateAddresses = false +): Promise { let addresses: ResolvedAddress[]; try { addresses = await boundedDnsLookup(hostname); @@ -482,8 +498,14 @@ async function checkHostSsrf(hostname: string, logger?: Logger): Promise { - const { label, tag, accept, contentTypes, timeoutMs, maxBytes, logger } = options; + const { label, tag, accept, contentTypes, timeoutMs, maxBytes, logger, allowPrivateAddresses } = options; const parsedUrl = new URL(url); // One deadline across DNS gate, connect, headers, AND body read — a @@ -1105,7 +1129,7 @@ export async function fetchPinnedBoundedJson( // addresses are PINNED into the fetch so the connection can't race a // rebind to a fresh, unvalidated resolution. const validatedAddresses = await withAbort( - checkHostSsrf(parsedUrl.hostname, logger), + checkHostSsrf(parsedUrl.hostname, logger, allowPrivateAddresses), controller.signal, `${tag} DNS lookup timed out` ); diff --git a/src/lib/providers/azure.ts b/src/lib/providers/azure.ts index 9039914..6e76b32 100644 --- a/src/lib/providers/azure.ts +++ b/src/lib/providers/azure.ts @@ -22,16 +22,27 @@ export const AzureADProvider: OAuthProviderConfig = { emailClaim: 'email', nameClaim: 'displayName', - // Azure-specific: configure endpoints based on tenant + // Azure-specific: configure endpoints based on tenant (`tenantId`: a real + // tenant GUID, or one of the shared aliases 'common'/'organizations'/ + // 'consumers' — see `resolveAzureIssuerBinding`, HarperFast/oauth#264). configure: (tenantId: string): Partial => { // Validate Azure tenant ID format validateAzureTenantId(tenantId); - return { + const config: Partial = { authorizationUrl: `https://login.microsoftonline.com/${tenantId}/oauth2/v2.0/authorize`, tokenUrl: `https://login.microsoftonline.com/${tenantId}/oauth2/v2.0/token`, jwksUri: `https://login.microsoftonline.com/${tenantId}/discovery/v2.0/keys`, - issuer: `https://login.microsoftonline.com/${tenantId}/v2.0`, }; + + // A real tenant GUID's issuer is fully determined by it. The shared + // aliases have no single fixed issuer — leave unset for + // `resolveAzureIssuerBinding` to resolve (or intentionally leave + // unresolved). + if (!/^(common|organizations|consumers)$/i.test(tenantId)) { + config.issuer = `https://login.microsoftonline.com/${tenantId}/v2.0`; + } + + return config; }, }; diff --git a/src/lib/providers/okta.ts b/src/lib/providers/okta.ts index d942473..f4c2e48 100644 --- a/src/lib/providers/okta.ts +++ b/src/lib/providers/okta.ts @@ -6,7 +6,7 @@ */ import type { OAuthProviderConfig } from '../../types.ts'; -import { validateDomainSafety, validateDomainAllowlist } from './validation.ts'; +import { validateDomainSafety, validateDomainAllowlist, validateOktaAuthServer } from './validation.ts'; export const OktaProvider: OAuthProviderConfig = { provider: 'okta', @@ -27,8 +27,11 @@ export const OktaProvider: OAuthProviderConfig = { // Okta includes user info in ID token, prefer that preferIdToken: true, - // Okta-specific: configure endpoints based on domain - configure: (domain: string): Partial => { + // Okta-specific: configure endpoints based on domain, and optionally a + // named custom authorization server (HarperFast/oauth#264) — e.g. + // `authServer: 'default'` for the `default` custom AS, or a generated ID. + // Omitted, this derives the org authorization server exactly as before. + configure: (domain: string, authServer?: string): Partial => { // Validate domain safety (SSRF protection, private IPs, etc.) const hostname = validateDomainSafety(domain, 'Okta'); @@ -36,8 +39,21 @@ export const OktaProvider: OAuthProviderConfig = { const ALLOWED_OKTA_DOMAINS = ['.okta.com', '.okta-emea.com', '.oktapreview.com']; validateDomainAllowlist(hostname, ALLOWED_OKTA_DOMAINS, 'Okta'); + if (authServer !== undefined) { + validateOktaAuthServer(authServer); + // Custom authorization server: endpoints AND issuer both live under + // /oauth2/{authServer} — unlike the org AS, the issuer includes the path. + const authServerPath = `/oauth2/${authServer}`; + return { + authorizationUrl: `https://${hostname}${authServerPath}/v1/authorize`, + tokenUrl: `https://${hostname}${authServerPath}/v1/token`, + userInfoUrl: `https://${hostname}${authServerPath}/v1/userinfo`, + jwksUri: `https://${hostname}${authServerPath}/v1/keys`, + issuer: `https://${hostname}${authServerPath}`, + }; + } + // Use /oauth2/v1 (org authorization server - most compatible) - // For /oauth2/default or custom auth servers, set authorizationUrl/tokenUrl/userInfoUrl directly in config const authServerPath = '/oauth2/v1'; return { diff --git a/src/lib/providers/validation.ts b/src/lib/providers/validation.ts index 52f1a78..67ab461 100644 --- a/src/lib/providers/validation.ts +++ b/src/lib/providers/validation.ts @@ -135,6 +135,28 @@ export function validateTenantId(tenantId: string): void { } } +/** + * Validates an Okta custom authorization server identifier (e.g. 'default', + * or a generated ID like 'aus1a2b3c4d5e6f7g8h9'). Same shape constraints as + * a tenant ID — a single, safe path segment — so it can never redirect the + * generated endpoint URLs anywhere but under + * `https://{domain}/oauth2/{authServer}/...` (HarperFast/oauth#264). + * + * @param authServer - Auth server identifier to validate + * @throws Error if the identifier is invalid or unsafe + */ +export function validateOktaAuthServer(authServer: string): void { + if (!authServer || typeof authServer !== 'string') { + throw new Error('Okta authServer must be a non-empty string'); + } + if (authServer.length < 1 || authServer.length > 64) { + throw new Error('Okta authServer must be 1-64 characters long'); + } + if (!/^[a-zA-Z0-9_-]+$/.test(authServer)) { + throw new Error('Okta authServer must contain only alphanumeric characters, hyphens, and underscores'); + } +} + /** * Sanitizes tenant name for safe HTML output * diff --git a/src/lib/resource.ts b/src/lib/resource.ts index 731eddf..44e4289 100644 --- a/src/lib/resource.ts +++ b/src/lib/resource.ts @@ -360,7 +360,33 @@ export class OAuthResource extends Resource { // Check if provider exists in static registry or dynamic cache let providerData = providers[providerName] ?? OAuthResource.dynamicProviderCache?.get(providerName); - // If not found, try to resolve via hook + // If not found, try to resolve via hook — unless a recent attempt for + // this exact provider already failed on an invalid Azure issuer pin + // (HarperFast/oauth#264/#271): re-running the hook and rebuilding the + // config would just re-throw the same AzureIssuerBindingError, so skip + // straight to the same response every other request gets during the + // cooldown, rather than hammering the hook on every single request for + // a config that hasn't changed. The cache lookup itself is gated on + // `!providerData` first — it's dynamic-resolution-only state, and must + // not cost every request a Map lookup on the already-resolved path. + if (!providerData) { + const cachedAzurePinFailure = OAuthResource.dynamicProviderCache?.getAzurePinFailure(providerName); + if (cachedAzurePinFailure) { + // Advisory only, and deliberately NOT error level: the cause was + // already logged at error level once, when recorded below — logging + // it again on every cached hit during the cooldown is exactly the + // log flood this cache exists to avoid. + logger?.debug?.( + `OAuth provider '${providerName}' (dynamic resolution) still has an invalid Azure issuer pin ` + + `(cooling down, not re-resolving): ${cachedAzurePinFailure}` + ); + return { + status: 500, + body: { error: 'Failed to resolve OAuth provider' }, + }; + } + } + if (!providerData && OAuthResource.hookManager?.hasHook('onResolveProvider')) { try { logger?.debug?.(`Provider "${providerName}" not found in registry or cache, calling onResolveProvider hook`); @@ -370,15 +396,36 @@ export class OAuthResource extends Resource { if (hookConfig) { const { OAuthProvider } = await import('./OAuthProvider.ts'); const { buildProviderConfig } = await import('./config.ts'); + const { AzureIssuerBindingError } = await import('./azureIssuer.ts'); const pluginDefaults = OAuthResource.pluginDefaults || {}; - const config = buildProviderConfig(hookConfig, providerName, pluginDefaults); + const dedupedLogger = OAuthResource.dynamicProviderCache?.wrapLoggerForDynamicResolution( + providerName, + logger + ); + let config: OAuthProviderConfig; + try { + config = buildProviderConfig(hookConfig, providerName, pluginDefaults, false, dedupedLogger); + } catch (buildError) { + if (buildError instanceof AzureIssuerBindingError) { + OAuthResource.dynamicProviderCache?.recordAzurePinFailure(providerName, buildError.message); + logger?.error?.( + `OAuth provider '${providerName}' (dynamic resolution) has an invalid Azure issuer pin: ${buildError.message}` + ); + return { + status: 500, + body: { error: 'Failed to resolve OAuth provider' }, + }; + } + throw buildError; + } const provider = new OAuthProvider(config, logger); providerData = { provider, config }; logger?.info?.(`Dynamically resolved provider: ${providerName}`); OAuthResource.dynamicProviderCache?.set(providerName, providerData); + OAuthResource.dynamicProviderCache?.clearAzurePinFailure(providerName); } } catch (error) { logger?.error?.( diff --git a/src/lib/tenantManager.ts b/src/lib/tenantManager.ts index 0b71240..04dbff9 100644 --- a/src/lib/tenantManager.ts +++ b/src/lib/tenantManager.ts @@ -19,6 +19,8 @@ export interface TenantConfig { provider: 'okta' | 'azure' | 'auth0' | string; /** Provider-specific domain (for Okta/Auth0) or tenant ID (for Azure) */ domain?: string; + /** Okta-specific custom authorization server identifier (e.g. 'default'); omitted uses the org authorization server. */ + authServer?: string; /** Azure AD specific tenant ID (alternative to domain) */ azureTenantId?: string; /** OAuth client ID for this tenant */ @@ -86,7 +88,7 @@ export class TenantManager { if (!tenant.domain) { throw new Error(`${normalizedProvider} provider requires domain configuration`); } - providerSpecificConfig = baseProvider.configure(tenant.domain); + providerSpecificConfig = baseProvider.configure(tenant.domain, tenant.authServer); break; case 'azure': diff --git a/src/types.ts b/src/types.ts index 817d339..ef2d63b 100644 --- a/src/types.ts +++ b/src/types.ts @@ -760,8 +760,12 @@ export interface OAuthProviderConfig { validateToken?: (accessToken: string, logger?: Logger) => Promise; /** Interval for periodic token validation (ms) - only for tokens without expiration */ tokenValidationInterval?: number; - /** Provider-specific configuration function (e.g., for tenant/domain setup) */ - configure?: (param: string) => Partial; + /** + * Provider-specific configuration function (e.g., for tenant/domain setup). + * `extra` is preset-specific — currently only Okta's, for an optional + * custom authorization server identifier (HarperFast/oauth#264). + */ + configure?: (param: string, extra?: string) => Partial; } /** diff --git a/test/lib/OAuthProvider.test.js b/test/lib/OAuthProvider.test.js index 4fab75e..ca844e1 100644 --- a/test/lib/OAuthProvider.test.js +++ b/test/lib/OAuthProvider.test.js @@ -2,12 +2,17 @@ * Tests for OAuthProvider */ -import { describe, it, before, after, beforeEach } from 'node:test'; +import { describe, it, before, after, beforeEach, afterEach } from 'node:test'; import assert from 'node:assert/strict'; +import { generateKeyPairSync } from 'node:crypto'; +import jwt from 'jsonwebtoken'; +import jwksRsa from 'jwks-rsa'; import { OAuthProvider } from '../../dist/lib/OAuthProvider.js'; import { GitHubProvider } from '../../dist/lib/providers/github.js'; import { AzureADProvider } from '../../dist/lib/providers/azure.js'; import { resetCSRFTableCache } from '../../dist/lib/CSRFTokenManager.js'; +import { startIssuerDiscovery, _clearDiscoveryCache } from '../../dist/lib/discovery.js'; +import { _setFetch, _setDnsLookup } from '../../dist/lib/mcp/cimd.js'; describe('OAuthProvider', () => { let provider; @@ -1628,4 +1633,412 @@ describe('OAuthProvider', () => { } }); }); + + describe('ID Token Verification with a real JWKS signature + OIDC discovery (#264)', () => { + // jwks-rsa supports an injected `fetcher`, so these tests exercise real + // RS256 signature verification (jwt.verify against a real key pair) + // without any network I/O — the same approach `OAuthProvider`'s own + // `initializeJwksClient` would use in production, just with the + // fetcher substituted for a real jwks_uri the way the account-adoption + // gate's own test fixtures substitute a real local server. The + // private `jwksClient` field is overwritten after construction, the + // same way this file already reaches into other private fields. + let keyPair; + const KID = 'test-key-1'; + + before(() => { + keyPair = generateKeyPairSync('rsa', { modulusLength: 2048 }); + }); + + beforeEach(() => { + _clearDiscoveryCache(); + _setDnsLookup(async () => [{ address: '93.184.216.34', family: 4 }]); + }); + afterEach(() => { + _setFetch(null); + _setDnsLookup(null); + }); + + function publicJwk() { + const jwk = keyPair.publicKey.export({ format: 'jwk' }); + return { ...jwk, kid: KID, alg: 'RS256', use: 'sig' }; + } + + function attachRealJwksClient(provider, keys = [publicJwk()]) { + provider['jwksClient'] = jwksRsa({ + jwksUri: 'https://idp.example.com/jwks', // never actually fetched — see `fetcher` + fetcher: async () => ({ keys }), + cache: true, + }); + } + + function sign(claims, { kid = KID, key = keyPair.privateKey } = {}) { + return jwt.sign(claims, key, { algorithm: 'RS256', keyid: kid }); + } + + function jsonResponse(body, status = 200) { + const bytes = Buffer.from(JSON.stringify(body)); + return { + ok: status >= 200 && status < 300, + status, + headers: new Map([['content-type', 'application/json']]), + body: { + getReader: () => { + let sent = false; + return { + read: async () => { + if (!sent) { + sent = true; + return { done: false, value: bytes }; + } + return { done: true, value: undefined }; + }, + cancel: () => {}, + }; + }, + }, + }; + } + + describe('generic OIDC discovery (non-Azure)', () => { + const discoveryConfig = { + provider: 'generic', + clientId: 'test-client-id', + clientSecret: 'test-client-secret', + authorizationUrl: 'https://idp.example.com/authorize', + tokenUrl: 'https://idp.example.com/token', + userInfoUrl: 'https://idp.example.com/userinfo', + jwksUri: 'https://idp.example.com/jwks', + redirectUri: 'https://app.test.com/oauth', + issuer: undefined, + }; + function discoveryDoc() { + return { + issuer: 'https://idp.example.com', + authorization_endpoint: discoveryConfig.authorizationUrl, + token_endpoint: discoveryConfig.tokenUrl, + jwks_uri: discoveryConfig.jwksUri, + }; + } + + it('a signature-verified token is upgraded to issuerValidated once discovery resolves', async () => { + _setFetch(async () => jsonResponse(discoveryDoc())); + startIssuerDiscovery( + discoveryConfig.authorizationUrl, + discoveryConfig.jwksUri, + discoveryConfig.tokenUrl, + 'custom-idp' + ); + + const p = new OAuthProvider({ ...discoveryConfig }, mockLogger); + attachRealJwksClient(p); + + const now = Math.floor(Date.now() / 1000); + const token = sign({ + iss: 'https://idp.example.com', + sub: 'user-1', + aud: discoveryConfig.clientId, + iat: now, + exp: now + 3600, + }); + + const result = await p.verifyIdToken(token); + assert.equal(result.signatureVerified, true); + assert.equal(result.issuerValidated, true); + assert.equal(p.config.issuer, 'https://idp.example.com', 'the discovered issuer is cached onto config'); + }); + + it('a verified token whose own iss disagrees with the discovered issuer is NOT cached onto config — caching it would break every future login’s issuer check', async () => { + _setFetch(async () => jsonResponse(discoveryDoc())); + startIssuerDiscovery( + discoveryConfig.authorizationUrl, + discoveryConfig.jwksUri, + discoveryConfig.tokenUrl, + 'custom-idp' + ); + + const p = new OAuthProvider({ ...discoveryConfig }, mockLogger); + attachRealJwksClient(p); + + const now = Math.floor(Date.now() / 1000); + // A non-compliant IdP: its real token's iss disagrees with its own + // discovery document's issuer (which discoveryDoc() sets to + // 'https://idp.example.com', no trailing slash). + const token = sign({ + iss: 'https://idp.example.com/', + sub: 'user-1', + aud: discoveryConfig.clientId, + iat: now, + exp: now + 3600, + }); + + const result = await p.verifyIdToken(token); + assert.equal(result.signatureVerified, true, 'signature verification itself still succeeds'); + assert.equal(result.issuerValidated, false, 'not adoption-eligible on a mismatch'); + assert.equal(p.config.issuer, undefined, 'the mismatched discovered issuer must not be cached onto config'); + + // A second, otherwise-identical login must still succeed exactly as + // the first did — nothing was cached that could break it. + const secondToken = sign({ + iss: 'https://idp.example.com/', + sub: 'user-2', + aud: discoveryConfig.clientId, + iat: now, + exp: now + 3600, + }); + const secondResult = await p.verifyIdToken(secondToken); + assert.equal(secondResult.signatureVerified, true); + assert.equal(secondResult.issuerValidated, false); + }); + + it('a forged/invalid-signature token never consumes the bounded first-login discovery wait', async () => { + // A fetch a short real delay (the previous version of this test) + // does NOT pin the ordering: by the time the valid call ran, + // discovery had already SETTLED either way, and + // `awaitDiscoveredIssuer` returns a settled entry's cached result + // immediately regardless of which call "consumed" the one bounded + // wait — the `awaited` flag only matters while the entry is still + // PENDING. So this holds the fetch open on a promise released by + // hand, keeping discovery pending across every step below, and + // checks the actual ordering effect (does the valid call get + // upgraded, or does it see `awaited` already set and get `null`) + // instead of a timing proxy for it. + let releaseFetch; + _setFetch( + () => + new Promise((resolve) => { + releaseFetch = () => resolve(jsonResponse(discoveryDoc())); + }) + ); + startIssuerDiscovery( + discoveryConfig.authorizationUrl, + discoveryConfig.jwksUri, + discoveryConfig.tokenUrl, + 'custom-idp' + ); + + const p = new OAuthProvider({ ...discoveryConfig }, mockLogger); + attachRealJwksClient(p); + + const now = Math.floor(Date.now() / 1000); + const otherKeyPair = generateKeyPairSync('rsa', { modulusLength: 2048 }); + const forgedToken = sign( + { iss: 'https://idp.example.com', sub: 'attacker', aud: discoveryConfig.clientId, iat: now, exp: now + 3600 }, + { key: otherKeyPair.privateKey } // signed with a key NOT in the JWKS response + ); + + // 1. The forged call must reject while discovery is still pending — + // the fetch has not been released, so if `verifyIdToken` awaited + // discovery before checking the signature, this would not settle + // until the bounded wait times out (seconds, not the test's normal + // runtime) rather than rejecting promptly on the bad signature. + await assert.rejects(() => p.verifyIdToken(forgedToken)); + + // 2. Start the valid call (don't await it yet) — with the correct + // ordering, the forged call above never touched discovery, so this + // is the first real awaiter and should genuinely wait on it. + const validToken = sign({ + iss: 'https://idp.example.com', + sub: 'user-1', + aud: discoveryConfig.clientId, + iat: now, + exp: now + 3600, + }); + const validPromise = p.verifyIdToken(validToken); + + // 3. Before releasing the fetch, confirm the valid call is still + // genuinely pending — not settled early. With the correct ordering, + // its own discovery-await is a fresh wait on the still-unresolved + // entry, so it cannot have settled yet. With the ordering reversed, + // the forged call already consumed the one bounded-await slot + // (`entry.awaited`); this call's own discovery-await would then see + // `awaited` already set and return `null` IMMEDIATELY (no I/O wait + // at all, since the entry is checked, not awaited, once `awaited` is + // already true), so the whole call would settle fast — with + // `issuerValidated: false` — well before the fetch is ever released. + const STILL_PENDING = Symbol('still-pending'); + const racedResult = await Promise.race([ + validPromise, + new Promise((resolve) => setTimeout(() => resolve(STILL_PENDING), 100)), + ]); + assert.equal( + racedResult, + STILL_PENDING, + 'the valid call must still be genuinely waiting on discovery, not already settled before the fetch is released' + ); + + // 4. Release the fetch; the valid call's genuine wait resolves, and + // it gets upgraded. + releaseFetch(); + const result = await validPromise; + assert.equal(result.signatureVerified, true); + assert.equal(result.issuerValidated, true, 'the valid call was still the first caller and got upgraded'); + }); + + it('audience is still enforced on the discovery-eligible branch (shared verification options, not a parallel check)', async () => { + _setFetch(async () => jsonResponse(discoveryDoc())); + startIssuerDiscovery( + discoveryConfig.authorizationUrl, + discoveryConfig.jwksUri, + discoveryConfig.tokenUrl, + 'custom-idp' + ); + + const p = new OAuthProvider({ ...discoveryConfig }, mockLogger); + attachRealJwksClient(p); + + const now = Math.floor(Date.now() / 1000); + const wrongAudienceToken = sign({ + iss: 'https://idp.example.com', + sub: 'user-1', + aud: 'some-other-client-id', + iat: now, + exp: now + 3600, + }); + + await assert.rejects(() => p.verifyIdToken(wrongAudienceToken), /audience/i); + }); + + it('a genuinely concurrent second login does not wait, even if discovery resolves moments later', async () => { + let resolveFetch; + _setFetch( + () => + new Promise((resolve) => { + resolveFetch = () => resolve(jsonResponse(discoveryDoc())); + }) + ); + startIssuerDiscovery( + discoveryConfig.authorizationUrl, + discoveryConfig.jwksUri, + discoveryConfig.tokenUrl, + 'custom-idp' + ); + + const p = new OAuthProvider({ ...discoveryConfig }, mockLogger); + attachRealJwksClient(p); + + const now = Math.floor(Date.now() / 1000); + const tokenA = sign({ + iss: 'https://idp.example.com', + sub: 'user-a', + aud: discoveryConfig.clientId, + iat: now, + exp: now + 3600, + }); + const tokenB = sign({ + iss: 'https://idp.example.com', + sub: 'user-b', + aud: discoveryConfig.clientId, + iat: now, + exp: now + 3600, + }); + + const firstCall = p.verifyIdToken(tokenA); + await new Promise((resolve) => setTimeout(resolve, 5)); // let the first call claim the await slot + const secondResult = await p.verifyIdToken(tokenB); + assert.equal(secondResult.issuerValidated, false, 'a concurrent second login does not wait'); + + resolveFetch(); + const firstResult = await firstCall; + assert.equal(firstResult.issuerValidated, true, 'the first login is upgraded once discovery resolves'); + }); + }); + + describe('Azure alias authority pinned to a single tenant (collapses into the ordinary configured-issuer path)', () => { + const GUID = '12345678-1234-1234-1234-123456789012'; + + it('constructing OAuthProvider directly (bypassing buildProviderConfig, like TenantManager) still resolves the safe tenant-specific binding', () => { + const handBuiltConfig = { + provider: 'azure', + clientId: 'c', + clientSecret: 's', + authorizationUrl: 'https://login.microsoftonline.com/common/oauth2/v2.0/authorize', + tokenUrl: 'https://login.microsoftonline.com/common/oauth2/v2.0/token', + userInfoUrl: 'https://graph.microsoft.com/v1.0/me', + jwksUri: 'https://login.microsoftonline.com/common/discovery/v2.0/keys', + issuer: `https://login.microsoftonline.com/${GUID}/v2.0`, + redirectUri: 'https://app.test.com/oauth', + }; + + const p = new OAuthProvider(handBuiltConfig, mockLogger); + assert.equal(p.config.jwksUri, `https://login.microsoftonline.com/${GUID}/discovery/v2.0/keys`); + assert.equal(p.config.issuer, `https://login.microsoftonline.com/${GUID}/v2.0`); + }); + + it('constructing OAuthProvider directly with an array pin on an alias still throws', () => { + const handBuiltConfig = { + provider: 'azure', + clientId: 'c', + clientSecret: 's', + authorizationUrl: 'https://login.microsoftonline.com/common/oauth2/v2.0/authorize', + tokenUrl: 'https://login.microsoftonline.com/common/oauth2/v2.0/token', + userInfoUrl: 'https://graph.microsoft.com/v1.0/me', + jwksUri: 'https://login.microsoftonline.com/common/discovery/v2.0/keys', + issuer: [`https://login.microsoftonline.com/${GUID}/v2.0`, 'https://login.microsoftonline.com/other/v2.0'], + redirectUri: 'https://app.test.com/oauth', + }; + + assert.throws(() => new OAuthProvider(handBuiltConfig, mockLogger), /array/); + }); + + it('a token matching the pinned tenant verifies through the ordinary issuer-configured path', async () => { + const handBuiltConfig = { + provider: 'azure', + clientId: 'c', + clientSecret: 's', + authorizationUrl: 'https://login.microsoftonline.com/common/oauth2/v2.0/authorize', + tokenUrl: 'https://login.microsoftonline.com/common/oauth2/v2.0/token', + userInfoUrl: 'https://graph.microsoft.com/v1.0/me', + jwksUri: 'https://login.microsoftonline.com/common/discovery/v2.0/keys', + issuer: `https://login.microsoftonline.com/${GUID}/v2.0`, + redirectUri: 'https://app.test.com/oauth', + }; + const p = new OAuthProvider(handBuiltConfig, mockLogger); + attachRealJwksClient(p); + + const now = Math.floor(Date.now() / 1000); + const token = sign({ + iss: `https://login.microsoftonline.com/${GUID}/v2.0`, + sub: 'user-1', + tid: GUID, + aud: 'c', + iat: now, + exp: now + 3600, + }); + + const result = await p.verifyIdToken(token); + assert.equal(result.signatureVerified, true); + assert.equal(result.issuerValidated, true); + }); + + it('a token claiming a different tenant than the pin is refused (not adoption-eligible)', async () => { + const otherGuid = '87654321-4321-4321-4321-210987654321'; + const handBuiltConfig = { + provider: 'azure', + clientId: 'c', + clientSecret: 's', + authorizationUrl: 'https://login.microsoftonline.com/common/oauth2/v2.0/authorize', + tokenUrl: 'https://login.microsoftonline.com/common/oauth2/v2.0/token', + userInfoUrl: 'https://graph.microsoft.com/v1.0/me', + jwksUri: 'https://login.microsoftonline.com/common/discovery/v2.0/keys', + issuer: `https://login.microsoftonline.com/${GUID}/v2.0`, + redirectUri: 'https://app.test.com/oauth', + }; + const p = new OAuthProvider(handBuiltConfig, mockLogger); + attachRealJwksClient(p); + + const now = Math.floor(Date.now() / 1000); + const token = sign({ + iss: `https://login.microsoftonline.com/${otherGuid}/v2.0`, + sub: 'attacker', + tid: otherGuid, + aud: 'c', + iat: now, + exp: now + 3600, + }); + + await assert.rejects(() => p.verifyIdToken(token), /issuer/i); + }); + }); + }); }); diff --git a/test/lib/OAuthResource.caching.test.js b/test/lib/OAuthResource.caching.test.js index 2d8f8a0..fbc175e 100644 --- a/test/lib/OAuthResource.caching.test.js +++ b/test/lib/OAuthResource.caching.test.js @@ -188,4 +188,165 @@ describe('OAuthResource - cacheDynamicProviders', () => { } }); }); + + describe('dynamic resolution — invalid Azure issuer pin (#264/#271)', () => { + const badAzureHookConfig = { + provider: 'azure', + clientId: 'c', + clientSecret: 's', + // A stale pin that doesn't name the configured tenant — buildProviderConfig throws AzureIssuerBindingError. + tenantId: '12345678-1234-1234-1234-123456789012', + issuer: 'https://login.microsoftonline.com/87654321-4321-4321-4321-210987654321/v2.0', + redirectUri: 'https://app.test.com/oauth', + }; + + it('returns 500 and does not cache a provider, naming the real cause in the log', async () => { + const callResolveProvider = createMockFn(async () => badAzureHookConfig); + const mockHookManager = { + hasHook: (name) => name === 'onResolveProvider', + callResolveProvider, + }; + + const cache = new DynamicProviderCache(true); + OAuthResource.configure({}, false, mockHookManager, {}, mockLogger, cache); + + const resource = new OAuthResource(); + resource.getContext = () => ({ session: { id: 'session-123' }, headers: {} }); + const target = { id: 'bad-azure-tenant/login', get: () => null }; + + const result = await resource.get(target); + assert.equal(result.status, 500); + assert.equal(cache.get('bad-azure-tenant'), undefined, 'no provider is cached for a failed resolution'); + assert.ok( + mockLogger.error.mock.calls.some((call) => + call.arguments.some( + (arg) => + typeof arg === 'string' && arg.includes('invalid Azure issuer pin') && arg.includes('different tenant') + ) + ), + 'the log names the real cause, not a generic "Error resolving provider"' + ); + }); + + it('does not re-run the resolve hook for the same provider within the cooldown — short-circuits to the same 500', async () => { + const callResolveProvider = createMockFn(async () => badAzureHookConfig); + const mockHookManager = { + hasHook: (name) => name === 'onResolveProvider', + callResolveProvider, + }; + + const cache = new DynamicProviderCache(true); + OAuthResource.configure({}, false, mockHookManager, {}, mockLogger, cache); + + const resource = new OAuthResource(); + resource.getContext = () => ({ session: { id: 'session-123' }, headers: {} }); + const target = { id: 'bad-azure-tenant/login', get: () => null }; + + const first = await resource.get(target); + assert.equal(first.status, 500); + assert.equal(callResolveProvider.mock.calls.length, 1); + + const second = await resource.get(target); + assert.equal(second.status, 500); + assert.equal( + callResolveProvider.mock.calls.length, + 1, + 'the hook must not be re-run while the failure is cooling down' + ); + }); + + it('re-runs the resolve hook once the cooldown elapses', async () => { + const callResolveProvider = createMockFn(async () => badAzureHookConfig); + const mockHookManager = { + hasHook: (name) => name === 'onResolveProvider', + callResolveProvider, + }; + + const cache = new DynamicProviderCache(true); + OAuthResource.configure({}, false, mockHookManager, {}, mockLogger, cache); + + const resource = new OAuthResource(); + resource.getContext = () => ({ session: { id: 'session-123' }, headers: {} }); + const target = { id: 'bad-azure-tenant/login', get: () => null }; + + await resource.get(target); + assert.equal(callResolveProvider.mock.calls.length, 1); + + const realNow = Date.now; + Date.now = () => realNow() + 31_000; + try { + await resource.get(target); + assert.equal(callResolveProvider.mock.calls.length, 2, 'the hook runs again once the 30s cooldown elapses'); + } finally { + Date.now = realNow; + } + }); + }); + + describe('dynamic resolution — Azure tenant-mismatch warning reaches the log, deduped (#271 follow-up)', () => { + const mismatchedAzureHookConfig = { + provider: 'azure', + clientId: 'c', + clientSecret: 's', + // authorizationUrl names the shared 'common' alias; jwksUri names one real + // tenant. Unpinned, buildProviderConfig derives the tenant-exclusive + // issuer (the safe direction) but also warns about the mismatch — this + // only reaches the log at all once a logger is passed through. + authorizationUrl: 'https://login.microsoftonline.com/common/oauth2/v2.0/authorize', + jwksUri: 'https://login.microsoftonline.com/12345678-1234-1234-1234-123456789012/discovery/v2.0/keys', + redirectUri: 'https://app.test.com/oauth', + }; + + it('passes the logger through so the mismatch warning fires for a dynamically-resolved provider', async () => { + const callResolveProvider = createMockFn(async () => mismatchedAzureHookConfig); + const mockHookManager = { + hasHook: (name) => name === 'onResolveProvider', + callResolveProvider, + }; + + const cache = new DynamicProviderCache(true); + OAuthResource.configure({}, false, mockHookManager, {}, mockLogger, cache); + + const resource = new OAuthResource(); + resource.getContext = () => ({ session: { id: 'session-123' }, headers: {} }); + const target = { id: 'mismatched-azure/login', get: () => null }; + + const result = await resource.get(target); + assert.equal(result.status, 302, 'the mismatch is advisory only — the provider still resolves'); + assert.ok( + mockLogger.warn.mock.calls.some((call) => + call.arguments.some((arg) => typeof arg === 'string' && arg.includes('mismatched-azure')) + ), + 'the warning reaches the log now that resource.ts passes the logger to buildProviderConfig' + ); + }); + + it('does not repeat the same warning on every request — dedup survives even with caching disabled', async () => { + const callResolveProvider = createMockFn(async () => mismatchedAzureHookConfig); + const mockHookManager = { + hasHook: (name) => name === 'onResolveProvider', + callResolveProvider, + }; + + // Cache disabled: every request re-runs the hook and rebuilds the + // config, which is exactly the "every request" case the dedup exists + // to stop from flooding the log. + const cache = new DynamicProviderCache(false); + OAuthResource.configure({}, false, mockHookManager, {}, mockLogger, cache); + + const resource = new OAuthResource(); + resource.getContext = () => ({ session: { id: 'session-123' }, headers: {} }); + const target = { id: 'mismatched-azure/login', get: () => null }; + + await resource.get(target); + await resource.get(target); + await resource.get(target); + assert.equal(callResolveProvider.mock.calls.length, 3, 'the hook does re-run every request (cache disabled)'); + + const matchingWarnings = mockLogger.warn.mock.calls.filter((call) => + call.arguments.some((arg) => typeof arg === 'string' && arg.includes('mismatched-azure')) + ); + assert.equal(matchingWarnings.length, 1, 'the warning logs once, not once per request'); + }); + }); }); diff --git a/test/lib/azureIssuer.test.js b/test/lib/azureIssuer.test.js new file mode 100644 index 0000000..cc202db --- /dev/null +++ b/test/lib/azureIssuer.test.js @@ -0,0 +1,443 @@ +/** + * Tests for Azure AD / Entra ID issuer resolution (HarperFast/oauth#264). + */ + +import { describe, it } from 'node:test'; +import assert from 'node:assert/strict'; +import { AZURE_CONSUMERS_TENANT_ID, isAzureJwksUri, resolveAzureIssuerBinding } from '../../dist/lib/azureIssuer.js'; + +const GUID_A = '12345678-1234-1234-1234-123456789012'; +const GUID_B = '87654321-4321-4321-4321-210987654321'; + +function baseConfig(overrides = {}) { + return { + provider: 'azure', + clientId: 'c', + clientSecret: 's', + authorizationUrl: 'https://login.microsoftonline.com/common/oauth2/v2.0/authorize', + tokenUrl: 'https://login.microsoftonline.com/common/oauth2/v2.0/token', + userInfoUrl: 'https://graph.microsoft.com/v1.0/me', + jwksUri: 'https://login.microsoftonline.com/common/discovery/v2.0/keys', + issuer: null, + ...overrides, + }; +} + +describe('isAzureJwksUri', () => { + it('is true only for an UNPINNED shared alias authority’s exact v2.0 keys-endpoint shape', () => { + assert.equal(isAzureJwksUri('https://login.microsoftonline.com/common/discovery/v2.0/keys'), true); + assert.equal(isAzureJwksUri('https://login.microsoftonline.com/organizations/discovery/v2.0/keys'), true); + assert.equal(isAzureJwksUri('https://login.microsoftonline.com/consumers/discovery/v2.0/keys'), true); + }); + + it('is false for a real-tenant-GUID segment — resolveAzureIssuerBinding always sets a usable issuer for it (or throws), so hasUsableIssuer already short-circuits every caller before this is reached', () => { + assert.equal(isAzureJwksUri(`https://login.microsoftonline.com/${GUID_A}/discovery/v2.0/keys`), false); + }); + + it('is false for a tenant-DOMAIN segment (e.g. a verified .onmicrosoft.com domain, which Azure also accepts here) — resolveAzureIssuerBinding never recognizes or binds it, so it must not be silently exempted from #231 §4/discovery', () => { + assert.equal( + isAzureJwksUri('https://login.microsoftonline.com/contoso.onmicrosoft.com/discovery/v2.0/keys'), + false + ); + }); + + it('is also true for the older v1 shared-alias keys shape (no v2.0) — resolveAzureIssuerBinding handles it the same way as the v2 alias shape', () => { + assert.equal(isAzureJwksUri('https://login.microsoftonline.com/common/discovery/keys'), true); + assert.equal(isAzureJwksUri('https://login.microsoftonline.com/organizations/discovery/keys'), true); + assert.equal(isAzureJwksUri('https://login.microsoftonline.com/consumers/discovery/keys'), true); + }); + + it('is false for the same v1 keys shape on a real tenant GUID, or any other unrecognized shape — resolveAzureIssuerBinding never touches those, so they must not be silently exempted from #231 §4/discovery', () => { + assert.equal(isAzureJwksUri(`https://login.microsoftonline.com/${GUID_A}/discovery/keys`), false); + assert.equal(isAzureJwksUri('https://login.microsoftonline.com/contoso.onmicrosoft.com/discovery/keys'), false); + assert.equal(isAzureJwksUri('https://login.microsoftonline.com/anything/at/all'), false); + }); + + it('is false for a different host, malformed URL, or absent value', () => { + assert.equal(isAzureJwksUri('https://idp.example.com/jwks'), false); + assert.equal(isAzureJwksUri('not a url'), false); + assert.equal(isAzureJwksUri(null), false); + assert.equal(isAzureJwksUri(undefined), false); + assert.equal(isAzureJwksUri(''), false); + }); +}); + +describe('resolveAzureIssuerBinding', () => { + it('is a no-op for a non-Azure jwksUri, regardless of issuer', () => { + const config = { + jwksUri: 'https://idp.example.com/jwks', + authorizationUrl: 'https://idp.example.com/authorize', + issuer: null, + }; + resolveAzureIssuerBinding(config, 'custom-idp'); + assert.equal(config.jwksUri, 'https://idp.example.com/jwks'); + assert.equal(config.issuer, null); + }); + + it('is a no-op for an Azure-host lookalike with extra path segments', () => { + const config = baseConfig({ jwksUri: 'https://login.microsoftonline.com/a/b/discovery/v2.0/keys' }); + resolveAzureIssuerBinding(config, 'azure'); + assert.equal(config.jwksUri, 'https://login.microsoftonline.com/a/b/discovery/v2.0/keys'); + assert.equal(config.issuer, null); + }); + + describe('real tenant GUID (tenant-exclusive, no shared pool)', () => { + it('sets a direct issuer when none is pinned', () => { + const config = baseConfig({ + jwksUri: `https://login.microsoftonline.com/${GUID_A}/discovery/v2.0/keys`, + issuer: null, + }); + resolveAzureIssuerBinding(config, 'azure'); + assert.equal(config.issuer, `https://login.microsoftonline.com/${GUID_A}/v2.0`); + // Unchanged — no rewrite needed for an already tenant-exclusive endpoint. + assert.equal(config.jwksUri, `https://login.microsoftonline.com/${GUID_A}/discovery/v2.0/keys`); + }); + + it('lowercases an uppercase tenant GUID segment before building the issuer', () => { + const config = baseConfig({ + jwksUri: `https://login.microsoftonline.com/${GUID_A.toUpperCase()}/discovery/v2.0/keys`, + issuer: null, + }); + resolveAzureIssuerBinding(config, 'azure'); + assert.equal(config.issuer, `https://login.microsoftonline.com/${GUID_A}/v2.0`); + }); + + it('warns at startup when authorizationUrl names a different tenant (an alias here) than the unpinned jwksUri’s real tenant — the derived issuer now excludes every other tenant, where main had no issuer check at all', () => { + const warnings = []; + const logger = { warn: (msg) => warnings.push(msg) }; + const config = baseConfig({ + // authorizationUrl stays at baseConfig's default: the 'common' alias. + jwksUri: `https://login.microsoftonline.com/${GUID_A}/discovery/v2.0/keys`, + issuer: null, + }); + resolveAzureIssuerBinding(config, 'azure-provider', logger); + assert.equal(config.issuer, `https://login.microsoftonline.com/${GUID_A}/v2.0`); + assert.ok( + warnings.some( + (msg) => + msg.includes('azure-provider') && msg.includes(GUID_A) && msg.includes('common') && msg.includes('issuer') + ), + 'expected a warning naming the provider, the effective tenant, and the alias authorizationUrl names' + ); + }); + + it('warns when authorizationUrl names a DIFFERENT real tenant GUID than jwksUri’s', () => { + const warnings = []; + const logger = { warn: (msg) => warnings.push(msg) }; + const config = baseConfig({ + authorizationUrl: `https://login.microsoftonline.com/${GUID_B}/oauth2/v2.0/authorize`, + jwksUri: `https://login.microsoftonline.com/${GUID_A}/discovery/v2.0/keys`, + issuer: null, + }); + resolveAzureIssuerBinding(config, 'azure-provider', logger); + assert.equal(config.issuer, `https://login.microsoftonline.com/${GUID_A}/v2.0`); + assert.ok(warnings.some((msg) => msg.includes(GUID_A) && msg.includes(GUID_B))); + }); + + it('does not warn when authorizationUrl and jwksUri name the same tenant (the normal, unaffected shape)', () => { + const warnings = []; + const logger = { warn: (msg) => warnings.push(msg) }; + const config = baseConfig({ + authorizationUrl: `https://login.microsoftonline.com/${GUID_A}/oauth2/v2.0/authorize`, + jwksUri: `https://login.microsoftonline.com/${GUID_A}/discovery/v2.0/keys`, + issuer: null, + }); + resolveAzureIssuerBinding(config, 'azure-provider', logger); + assert.equal(warnings.length, 0); + }); + + it('does not warn when authorizationUrl names a verified tenant DOMAIN (not an alias or a GUID) — we have no offline way to compare a domain name to jwksUri’s GUID, so warning would be a guess, not a finding', () => { + const warnings = []; + const logger = { warn: (msg) => warnings.push(msg) }; + const config = baseConfig({ + authorizationUrl: 'https://login.microsoftonline.com/contoso.onmicrosoft.com/oauth2/v2.0/authorize', + jwksUri: `https://login.microsoftonline.com/${GUID_A}/discovery/v2.0/keys`, + issuer: null, + }); + resolveAzureIssuerBinding(config, 'azure-provider', logger); + assert.equal(config.issuer, `https://login.microsoftonline.com/${GUID_A}/v2.0`); + assert.equal(warnings.length, 0); + }); + + it('does not warn when authorizationUrl names the "consumers" alias and jwksUri names AZURE_CONSUMERS_TENANT_ID — they name the same tenant, even though the strings never match', () => { + const warnings = []; + const logger = { warn: (msg) => warnings.push(msg) }; + const config = baseConfig({ + authorizationUrl: 'https://login.microsoftonline.com/consumers/oauth2/v2.0/authorize', + jwksUri: `https://login.microsoftonline.com/${AZURE_CONSUMERS_TENANT_ID}/discovery/v2.0/keys`, + issuer: null, + }); + resolveAzureIssuerBinding(config, 'azure-provider', logger); + assert.equal(config.issuer, `https://login.microsoftonline.com/${AZURE_CONSUMERS_TENANT_ID}/v2.0`); + assert.equal(warnings.length, 0); + }); + + it('a throwing warn logger does not abort the derivation', () => { + const config = baseConfig({ + jwksUri: `https://login.microsoftonline.com/${GUID_A}/discovery/v2.0/keys`, + issuer: null, + }); + const throwingLogger = { + warn: () => { + throw new Error('logger blew up'); + }, + }; + resolveAzureIssuerBinding(config, 'azure', throwingLogger); + assert.equal(config.issuer, `https://login.microsoftonline.com/${GUID_A}/v2.0`); + }); + + it('derives the v1 (sts.windows.net) issuer form when no pin is given and authorizationUrl is a v1 authorize endpoint', () => { + // Mirrors the pinned branch's form check: an unpinned real-tenant-GUID + // config must derive the SAME form a pin would have been required to + // match, not always v2 regardless of what authorizationUrl actually + // issues. + const config = baseConfig({ + authorizationUrl: `https://login.microsoftonline.com/${GUID_A}/oauth2/authorize`, + jwksUri: `https://login.microsoftonline.com/${GUID_A}/discovery/v2.0/keys`, + issuer: null, + }); + resolveAzureIssuerBinding(config, 'azure'); + assert.equal(config.issuer, `https://sts.windows.net/${GUID_A}/`); + }); + + it('still derives the v2 issuer form when authorizationUrl is unrecognized (not v1 or v2 shaped)', () => { + const config = baseConfig({ + authorizationUrl: `https://login.microsoftonline.com/${GUID_A}/custom/authorize-path`, + jwksUri: `https://login.microsoftonline.com/${GUID_A}/discovery/v2.0/keys`, + issuer: null, + }); + resolveAzureIssuerBinding(config, 'azure'); + assert.equal(config.issuer, `https://login.microsoftonline.com/${GUID_A}/v2.0`); + }); + + it('leaves an already-canonical matching explicit pin untouched', () => { + const config = baseConfig({ + jwksUri: `https://login.microsoftonline.com/${GUID_A}/discovery/v2.0/keys`, + issuer: `https://login.microsoftonline.com/${GUID_A}/v2.0`, + }); + resolveAzureIssuerBinding(config, 'azure'); + assert.equal(config.issuer, `https://login.microsoftonline.com/${GUID_A}/v2.0`); + }); + + it('canonicalizes a same-tenant pin with a trailing slash — jwt.verify compares issuer strings exactly', () => { + const config = baseConfig({ + jwksUri: `https://login.microsoftonline.com/${GUID_A}/discovery/v2.0/keys`, + issuer: `https://login.microsoftonline.com/${GUID_A}/v2.0/`, + }); + resolveAzureIssuerBinding(config, 'azure'); + assert.equal(config.issuer, `https://login.microsoftonline.com/${GUID_A}/v2.0`); + }); + + it('canonicalizes a same-tenant pin with an uppercase GUID', () => { + const config = baseConfig({ + jwksUri: `https://login.microsoftonline.com/${GUID_A}/discovery/v2.0/keys`, + issuer: `https://login.microsoftonline.com/${GUID_A.toUpperCase()}/v2.0`, + }); + resolveAzureIssuerBinding(config, 'azure'); + assert.equal(config.issuer, `https://login.microsoftonline.com/${GUID_A}/v2.0`); + }); + + it('keeps a same-tenant pin given in the older sts.windows.net (v1) form AS v1 — when authorizationUrl is ALSO a v1 authorize endpoint', () => { + // Which `iss` form a real token carries depends on the authorize + // endpoint, not the jwksUri — collapsing this to v2 would break + // verification for every real v1 token. But the pin must actually + // match that endpoint's form (see the mismatch tests below) — a v1 + // pin is only safe when `authorizationUrl` really is v1. + const config = baseConfig({ + authorizationUrl: `https://login.microsoftonline.com/${GUID_A}/oauth2/authorize`, + jwksUri: `https://login.microsoftonline.com/${GUID_A}/discovery/v2.0/keys`, + issuer: `https://sts.windows.net/${GUID_A}/`, + }); + resolveAzureIssuerBinding(config, 'azure'); + assert.equal(config.issuer, `https://sts.windows.net/${GUID_A}/`); + }); + + it('canonicalizes a v1 pin’s case and trailing slash without changing its form', () => { + const config = baseConfig({ + authorizationUrl: `https://login.microsoftonline.com/${GUID_A}/oauth2/authorize`, + jwksUri: `https://login.microsoftonline.com/${GUID_A}/discovery/v2.0/keys`, + issuer: `https://sts.windows.net/${GUID_A.toUpperCase()}`, + }); + resolveAzureIssuerBinding(config, 'azure'); + assert.equal(config.issuer, `https://sts.windows.net/${GUID_A}/`); + }); + + it('rejects a v1 pin when authorizationUrl is a v2 authorize endpoint — the preset always generates v2, so a stray v1 pin would fail jwt.verify on every real token', () => { + const config = baseConfig({ + authorizationUrl: `https://login.microsoftonline.com/${GUID_A}/oauth2/v2.0/authorize`, + jwksUri: `https://login.microsoftonline.com/${GUID_A}/discovery/v2.0/keys`, + issuer: `https://sts.windows.net/${GUID_A}/`, + }); + assert.throws(() => resolveAzureIssuerBinding(config, 'azure'), /wrong Azure issuer form/); + }); + + it('rejects a v2 pin when authorizationUrl is a v1 authorize endpoint', () => { + const config = baseConfig({ + authorizationUrl: `https://login.microsoftonline.com/${GUID_A}/oauth2/authorize`, + jwksUri: `https://login.microsoftonline.com/${GUID_A}/discovery/v2.0/keys`, + issuer: `https://login.microsoftonline.com/${GUID_A}/v2.0`, + }); + assert.throws(() => resolveAzureIssuerBinding(config, 'azure'), /wrong Azure issuer form/); + }); + + it('rejects a same-tenant array mixing v1 and v2 forms when authorizationUrl has a recognized (v2) shape — one element always names the wrong form', () => { + const config = baseConfig({ + authorizationUrl: `https://login.microsoftonline.com/${GUID_A}/oauth2/v2.0/authorize`, + jwksUri: `https://login.microsoftonline.com/${GUID_A}/discovery/v2.0/keys`, + issuer: [`https://login.microsoftonline.com/${GUID_A}/v2.0`, `https://sts.windows.net/${GUID_A}/`], + }); + assert.throws(() => resolveAzureIssuerBinding(config, 'azure'), /wrong Azure issuer form/); + }); + + it('does not additionally constrain the pin’s form when authorizationUrl is an unrecognized Azure shape — only the tenant is checked there', () => { + const config = baseConfig({ + // Neither the v1 (`/oauth2/authorize`) nor v2 (`/oauth2/v2.0/authorize`) + // suffix — a shape this module doesn't classify. + authorizationUrl: `https://login.microsoftonline.com/${GUID_A}/custom/authorize-path`, + jwksUri: `https://login.microsoftonline.com/${GUID_A}/discovery/v2.0/keys`, + issuer: `https://sts.windows.net/${GUID_A}/`, + }); + resolveAzureIssuerBinding(config, 'azure'); + assert.equal(config.issuer, `https://sts.windows.net/${GUID_A}/`); + }); + + it('throws when an explicit pin names a different tenant than the jwksUri', () => { + const config = baseConfig({ + jwksUri: `https://login.microsoftonline.com/${GUID_A}/discovery/v2.0/keys`, + issuer: `https://login.microsoftonline.com/${GUID_B}/v2.0`, + }); + assert.throws(() => resolveAzureIssuerBinding(config, 'azure'), /different tenant/); + }); + + it('throws when an array pin has one element naming a different tenant than the jwksUri', () => { + const config = baseConfig({ + jwksUri: `https://login.microsoftonline.com/${GUID_A}/discovery/v2.0/keys`, + issuer: [`https://login.microsoftonline.com/${GUID_A}/v2.0`, `https://sts.windows.net/${GUID_B}/`], + }); + assert.throws(() => resolveAzureIssuerBinding(config, 'azure'), /different tenant/); + }); + + it('is idempotent — calling it twice does not throw or change the result', () => { + const config = baseConfig({ + jwksUri: `https://login.microsoftonline.com/${GUID_A}/discovery/v2.0/keys`, + issuer: null, + }); + resolveAzureIssuerBinding(config, 'azure'); + const afterFirst = { ...config }; + resolveAzureIssuerBinding(config, 'azure'); + assert.deepEqual(config, afterFirst); + }); + }); + + describe('shared alias authorities (common/organizations/consumers) — adoption requires a pin', () => { + for (const alias of ['common', 'organizations', 'consumers']) { + it(`'${alias}' with no pin is untouched (byte-identical to today)`, () => { + const config = baseConfig({ + jwksUri: `https://login.microsoftonline.com/${alias}/discovery/v2.0/keys`, + issuer: null, + }); + resolveAzureIssuerBinding(config, 'azure'); + assert.equal(config.jwksUri, `https://login.microsoftonline.com/${alias}/discovery/v2.0/keys`); + assert.equal(config.issuer, null); + }); + + it(`'${alias}' with a single valid tenant-GUID pin redirects to that tenant's own endpoint`, () => { + const config = baseConfig({ + jwksUri: `https://login.microsoftonline.com/${alias}/discovery/v2.0/keys`, + issuer: `https://login.microsoftonline.com/${GUID_A}/v2.0`, + }); + resolveAzureIssuerBinding(config, 'azure'); + assert.equal(config.jwksUri, `https://login.microsoftonline.com/${GUID_A}/discovery/v2.0/keys`); + assert.equal(config.issuer, `https://login.microsoftonline.com/${GUID_A}/v2.0`); + }); + + it(`'${alias}' on the older v1 keys shape (no v2.0) with no pin is ALSO untouched (byte-identical to today) — this is the same shared key pool under an older URL`, () => { + const config = baseConfig({ + jwksUri: `https://login.microsoftonline.com/${alias}/discovery/keys`, + issuer: null, + }); + resolveAzureIssuerBinding(config, 'azure'); + assert.equal(config.jwksUri, `https://login.microsoftonline.com/${alias}/discovery/keys`); + assert.equal(config.issuer, null); + }); + + it(`'${alias}' on the older v1 keys shape (no v2.0) with ANY pin throws — a pinned issuer can never be safely bound to this shared, unvalidated key pool`, () => { + const config = baseConfig({ + jwksUri: `https://login.microsoftonline.com/${alias}/discovery/keys`, + issuer: `https://login.microsoftonline.com/${GUID_A}/v2.0`, + }); + assert.throws(() => resolveAzureIssuerBinding(config, 'azure'), /shared v1 (key pool|authority)/); + // jwksUri must be left exactly as configured — no partial rewrite before the throw. + assert.equal(config.jwksUri, `https://login.microsoftonline.com/${alias}/discovery/keys`); + }); + } + + it('consumers can be pinned to the documented fixed tenant GUID constant', () => { + const config = baseConfig({ + jwksUri: 'https://login.microsoftonline.com/consumers/discovery/v2.0/keys', + issuer: `https://login.microsoftonline.com/${AZURE_CONSUMERS_TENANT_ID}/v2.0`, + }); + resolveAzureIssuerBinding(config, 'azure'); + assert.equal( + config.jwksUri, + `https://login.microsoftonline.com/${AZURE_CONSUMERS_TENANT_ID}/discovery/v2.0/keys` + ); + assert.equal(config.issuer, `https://login.microsoftonline.com/${AZURE_CONSUMERS_TENANT_ID}/v2.0`); + }); + + it('rejects an array pin — exactly one tenant, never several, through one provider (#264)', () => { + const config = baseConfig({ + jwksUri: 'https://login.microsoftonline.com/common/discovery/v2.0/keys', + issuer: [ + `https://login.microsoftonline.com/${GUID_A}/v2.0`, + `https://login.microsoftonline.com/${GUID_B}/v2.0`, + ], + }); + assert.throws(() => resolveAzureIssuerBinding(config, 'azure'), /array/); + }); + + it('rejects a non-Azure-shaped pin', () => { + const config = baseConfig({ + jwksUri: 'https://login.microsoftonline.com/common/discovery/v2.0/keys', + issuer: 'https://sts.example.com/not-azure', + }); + assert.throws(() => resolveAzureIssuerBinding(config, 'azure'), /not a usable Azure tenant issuer/); + }); + + it('a throwing advisory logger does not abort the pinned-alias rewrite', () => { + const config = baseConfig({ + jwksUri: 'https://login.microsoftonline.com/common/discovery/v2.0/keys', + issuer: `https://login.microsoftonline.com/${GUID_A}/v2.0`, + }); + const throwingLogger = { + info: () => { + throw new Error('logger blew up'); + }, + }; + resolveAzureIssuerBinding(config, 'azure', throwingLogger); + assert.equal(config.jwksUri, `https://login.microsoftonline.com/${GUID_A}/discovery/v2.0/keys`); + assert.equal(config.issuer, `https://login.microsoftonline.com/${GUID_A}/v2.0`); + }); + + it('rewrite collapses into the tenant-exclusive case: a second call is a no-op', () => { + const config = baseConfig({ + jwksUri: 'https://login.microsoftonline.com/organizations/discovery/v2.0/keys', + issuer: `https://login.microsoftonline.com/${GUID_A}/v2.0`, + }); + resolveAzureIssuerBinding(config, 'azure'); + const afterFirst = { ...config }; + resolveAzureIssuerBinding(config, 'azure'); + assert.deepEqual(config, afterFirst); + }); + + it('uppercase GUID in the pin still resolves and compares correctly', () => { + const config = baseConfig({ + jwksUri: 'https://login.microsoftonline.com/common/discovery/v2.0/keys', + issuer: `https://login.microsoftonline.com/${GUID_A.toUpperCase()}/v2.0`, + }); + resolveAzureIssuerBinding(config, 'azure'); + assert.equal(config.jwksUri, `https://login.microsoftonline.com/${GUID_A}/discovery/v2.0/keys`); + assert.equal(config.issuer, `https://login.microsoftonline.com/${GUID_A}/v2.0`); + }); + }); +}); diff --git a/test/lib/config.test.js b/test/lib/config.test.js index 3f6299a..a52ac1a 100644 --- a/test/lib/config.test.js +++ b/test/lib/config.test.js @@ -15,6 +15,8 @@ import { normalizeMcpSecurityConfig, isUnresolvedEnvPlaceholder, skipUndefined, + needsIssuerDiscovery, + collectDiscoveryTargets, } from '../../dist/lib/config.js'; import { interactivePrivateKeyJwtEnabled } from '../../dist/lib/mcp/clientAuthMethod.js'; @@ -1394,6 +1396,67 @@ describe('OAuth Configuration', () => { assert.ok(config.userInfoUrl.includes('myapp.auth0.com')); }); + it('an explicit, usable issuer pin wins over the domain-derived one (#264)', () => { + const providerConfig = { + provider: 'auth0', + clientId: 'auth0-client', + clientSecret: 'auth0-secret', + domain: 'myapp.auth0.com', + issuer: 'https://custom-issuer.example.com/', + redirectUri: 'https://app.test.com/oauth', + }; + + const config = buildProviderConfig(providerConfig, 'auth0', {}); + + assert.equal(config.issuer, 'https://custom-issuer.example.com/'); + }); + + it('an unresolved ${VAR} issuer placeholder does not win over the domain-derived issuer (#264)', () => { + delete process.env.OAUTH_TEST_UNSET_ISSUER_VAR; + const providerConfig = { + provider: 'auth0', + clientId: 'auth0-client', + clientSecret: 'auth0-secret', + domain: 'myapp.auth0.com', + issuer: '${OAUTH_TEST_UNSET_ISSUER_VAR}', + redirectUri: 'https://app.test.com/oauth', + }; + + const config = buildProviderConfig(providerConfig, 'auth0', {}); + + assert.equal(config.issuer, 'https://myapp.auth0.com/'); + }); + + it('a null issuer does not win over the domain-derived issuer (#264)', () => { + const providerConfig = { + provider: 'auth0', + clientId: 'auth0-client', + clientSecret: 'auth0-secret', + domain: 'myapp.auth0.com', + issuer: null, + redirectUri: 'https://app.test.com/oauth', + }; + + const config = buildProviderConfig(providerConfig, 'auth0', {}); + + assert.equal(config.issuer, 'https://myapp.auth0.com/'); + }); + + it('an empty-string issuer does not win over the domain-derived issuer (#264)', () => { + const providerConfig = { + provider: 'auth0', + clientId: 'auth0-client', + clientSecret: 'auth0-secret', + domain: 'myapp.auth0.com', + issuer: '', + redirectUri: 'https://app.test.com/oauth', + }; + + const config = buildProviderConfig(providerConfig, 'auth0', {}); + + assert.equal(config.issuer, 'https://myapp.auth0.com/'); + }); + it('should clean Auth0 domain input', () => { const providerConfig = { provider: 'auth0', @@ -1410,8 +1473,13 @@ describe('OAuth Configuration', () => { }); }); - describe('issuer required for JWKS-enabled providers, static startup only (#231 §4)', () => { - it('throws naming the provider and key when a generic provider sets jwksUri but no issuer', () => { + describe('issuer required for JWKS-enabled providers, static startup only (#231 §4, relaxed by #264)', () => { + it('does NOT throw for a generic provider with an https authorizationUrl, jwksUri and no issuer — defers to OIDC discovery instead (#264)', () => { + // #264 relaxes #231 §4's hard-fail: an `https` explicit-endpoint + // config is no longer necessarily wrong — it may just need one + // background discovery round-trip the operator never has to think + // about. Only a config that's wrong independent of the network + // (next test) still hard-fails. const providerConfig = { provider: 'generic', clientId: 'c', @@ -1423,6 +1491,23 @@ describe('OAuth Configuration', () => { redirectUri: 'https://app.test.com/oauth', }; + const config = buildProviderConfig(providerConfig, 'custom-idp', {}, true); + assert.equal(config.issuer, undefined); + assert.equal(needsIssuerDiscovery(config), true, 'eligible for background OIDC discovery'); + }); + + it('still throws naming the provider and key when authorizationUrl is not https — discovery cannot help a config that is wrong independent of the network (#264)', () => { + const providerConfig = { + provider: 'generic', + clientId: 'c', + clientSecret: 's', + authorizationUrl: 'http://idp.example.com/authorize', + tokenUrl: 'http://idp.example.com/token', + userInfoUrl: 'http://idp.example.com/userinfo', + jwksUri: 'https://idp.example.com/jwks', + redirectUri: 'https://app.test.com/oauth', + }; + assert.throws( () => buildProviderConfig(providerConfig, 'custom-idp', {}, true), (error) => { @@ -1433,10 +1518,11 @@ describe('OAuth Configuration', () => { ); }); - it('throws when Okta is configured with explicit endpoints (bypassing domain) and no issuer', () => { + it('does NOT throw when Okta is configured with explicit endpoints (bypassing domain) and no issuer — defers to discovery (#264)', () => { // A custom Okta authorization-server config with explicit endpoints — - // the regression this closes: previously issuer stayed '' and adoption - // was silently denied at login instead of failing fast at startup. + // #231 §4 used to fail fast at startup for this; #264 relaxes that to + // a background discovery attempt instead, since the https endpoint + // may well be resolvable. const providerConfig = { provider: 'okta', clientId: 'c', @@ -1448,7 +1534,9 @@ describe('OAuth Configuration', () => { redirectUri: 'https://app.test.com/oauth', }; - assert.throws(() => buildProviderConfig(providerConfig, 'okta-custom-as', {}, true), /issuer/); + const config = buildProviderConfig(providerConfig, 'okta-custom-as', {}, true); + assert.ok(!config.issuer, 'no usable issuer (Okta preset default is an empty string)'); + assert.equal(needsIssuerDiscovery(config), true); }); it('does NOT throw the same config without enforceIssuerForJwks — the dynamic/request-time path (#231 follow-up)', () => { @@ -1536,6 +1624,101 @@ describe('OAuth Configuration', () => { assert.equal(config.issuer, null); }); + it('does not throw for a generic provider manually pointed at an unpinned Azure alias jwksUri — excluded by jwksUri shape, not declared provider type (#264)', () => { + // Before the fix, this hard-failed with a misleading "authorizationUrl + // is missing or not https" error (authorizationUrl IS https here) — + // the real reason was the Azure-host carve-out only firing for + // `provider: 'azure'`/`microsoft`, not any config whose jwksUri + // happens to be Azure's shared, tenant-independent host. + const providerConfig = { + provider: 'generic', + clientId: 'c', + clientSecret: 's', + authorizationUrl: 'https://login.microsoftonline.com/common/oauth2/v2.0/authorize', + tokenUrl: 'https://login.microsoftonline.com/common/oauth2/v2.0/token', + userInfoUrl: 'https://graph.microsoft.com/v1.0/me', + jwksUri: 'https://login.microsoftonline.com/common/discovery/v2.0/keys', + redirectUri: 'https://app.test.com/oauth', + }; + + const config = buildProviderConfig(providerConfig, 'custom-azure', {}, true); + assert.equal(config.provider, 'generic'); + assert.equal(config.issuer, undefined); + assert.equal( + needsIssuerDiscovery(config), + false, + 'an unpinned alias must never be probed by generic discovery' + ); + }); + + it('does not throw for an unpinned Azure alias on the older v1 keys shape (no v2.0) either — byte-identical to main, same as the v2 alias shape (#271 follow-up)', () => { + // Before this fix, this shape was NOT recognized at all, so it fell + // through into generic OIDC discovery (which fails for it) and was + // told to pin `issuer` — landing on a config that binds a pin to + // Azure's shared, unvalidated v1 key pool. + const providerConfig = { + provider: 'generic', + clientId: 'c', + clientSecret: 's', + authorizationUrl: 'https://login.microsoftonline.com/common/oauth2/authorize', + tokenUrl: 'https://login.microsoftonline.com/common/oauth2/token', + userInfoUrl: 'https://graph.microsoft.com/v1.0/me', + jwksUri: 'https://login.microsoftonline.com/common/discovery/keys', + redirectUri: 'https://app.test.com/oauth', + }; + + const config = buildProviderConfig(providerConfig, 'custom-azure-v1', {}, true); + assert.equal(config.issuer, undefined); + assert.equal( + needsIssuerDiscovery(config), + false, + 'an unpinned v1 alias must never be probed by generic discovery' + ); + }); + + it('throws for a PINNED Azure alias on the older v1 keys shape (no v2.0) — a pin can never be safely bound to this shared key pool (#271 follow-up)', () => { + const providerConfig = { + provider: 'generic', + clientId: 'c', + clientSecret: 's', + authorizationUrl: 'https://login.microsoftonline.com/common/oauth2/authorize', + tokenUrl: 'https://login.microsoftonline.com/common/oauth2/token', + userInfoUrl: 'https://graph.microsoft.com/v1.0/me', + jwksUri: 'https://login.microsoftonline.com/common/discovery/keys', + issuer: 'https://sts.windows.net/12345678-1234-1234-1234-123456789012/', + redirectUri: 'https://app.test.com/oauth', + }; + + assert.throws(() => buildProviderConfig(providerConfig, 'custom-azure-v1', {}, true), /shared v1/); + }); + + it('still requires an issuer (or defers to discovery) for a tenant-DOMAIN Azure jwksUri — resolveAzureIssuerBinding never recognizes it, so it must not get the unpinned-alias exemption (#264/#271)', () => { + // A verified .onmicrosoft.com domain is a real, Azure-accepted + // segment here — not a GUID, and not one of the shared aliases — + // so resolveAzureIssuerBinding leaves it untouched, and this must + // fall through to the normal check instead of booting silently + // with issuerValidated permanently false. + const httpsConfig = { + provider: 'generic', + clientId: 'c', + clientSecret: 's', + authorizationUrl: 'https://login.microsoftonline.com/contoso.onmicrosoft.com/oauth2/v2.0/authorize', + tokenUrl: 'https://login.microsoftonline.com/contoso.onmicrosoft.com/oauth2/v2.0/token', + userInfoUrl: 'https://graph.microsoft.com/v1.0/me', + jwksUri: 'https://login.microsoftonline.com/contoso.onmicrosoft.com/discovery/v2.0/keys', + redirectUri: 'https://app.test.com/oauth', + }; + const config = buildProviderConfig(httpsConfig, 'contoso-azure', {}, true); + assert.equal(config.issuer, undefined); + assert.equal(needsIssuerDiscovery(config), true, 'eligible for background OIDC discovery, unlike an alias'); + + const nonHttpsConfig = { + ...httpsConfig, + authorizationUrl: 'http://login.microsoftonline.com/contoso.onmicrosoft.com/oauth2/v2.0/authorize', + }; + assert.throws(() => buildProviderConfig(nonHttpsConfig, 'contoso-azure', {}, true), /issuer/); + }); + it('does not throw when no jwksUri is configured at all (non-OIDC provider)', () => { const providerConfig = { provider: 'generic', @@ -1563,7 +1746,7 @@ describe('OAuth Configuration', () => { assert.ok(!config.jwksUri, 'GitHub preset has no jwksUri (null)'); }); - it('a non-empty issuer array with only blank entries is NOT usable (hasUsableIssuer)', () => { + it('a non-empty issuer array with only blank entries is NOT usable (hasUsableIssuer) — still eligible for discovery with an https authorizationUrl', () => { const providerConfig = { provider: 'generic', clientId: 'c', @@ -1576,10 +1759,27 @@ describe('OAuth Configuration', () => { redirectUri: 'https://app.test.com/oauth', }; + const config = buildProviderConfig(providerConfig, 'custom-idp', {}, true); + assert.equal(needsIssuerDiscovery(config), true); + }); + + it('a non-empty issuer array with only blank entries still hard-fails with a non-https authorizationUrl', () => { + const providerConfig = { + provider: 'generic', + clientId: 'c', + clientSecret: 's', + authorizationUrl: 'http://idp.example.com/authorize', + tokenUrl: 'http://idp.example.com/token', + userInfoUrl: 'http://idp.example.com/userinfo', + jwksUri: 'https://idp.example.com/jwks', + issuer: [''], + redirectUri: 'https://app.test.com/oauth', + }; + assert.throws(() => buildProviderConfig(providerConfig, 'custom-idp', {}, true), /issuer/); }); - it('initializeProviders propagates the throw (fails fast at startup, not a silent per-login deny)', () => { + it('initializeProviders does NOT throw for an https explicit-endpoint provider missing issuer — it defers to background discovery (#264)', () => { const options = { providers: { 'custom-idp': { @@ -1595,6 +1795,91 @@ describe('OAuth Configuration', () => { }, }; + const providers = initializeProviders(options, undefined); + assert.ok(providers['custom-idp'], 'provider still initializes'); + assert.equal(providers['custom-idp'].config.issuer, undefined); + assert.equal(collectDiscoveryTargets(providers).length, 1, 'eligible for background OIDC discovery'); + }); + + it('initializeProviders still propagates the throw for a non-https authorizationUrl (fails fast at startup, not a silent per-login deny)', () => { + const options = { + providers: { + 'custom-idp': { + provider: 'generic', + clientId: 'c', + clientSecret: 's', + authorizationUrl: 'http://idp.example.com/authorize', + tokenUrl: 'http://idp.example.com/token', + userInfoUrl: 'http://idp.example.com/userinfo', + jwksUri: 'https://idp.example.com/jwks', + redirectUri: 'https://app.test.com/oauth', + }, + }, + }; + + assert.throws(() => initializeProviders(options, undefined), /issuer/); + }); + + it('a provider with an invalid Azure issuer pin is skipped with a logged error — every other provider still initializes (#271 follow-up)', () => { + const warnings = []; + const logger = { + error: (...args) => warnings.push(args.join(' ')), + warn: () => {}, + info: () => {}, + debug: () => {}, + }; + const options = { + providers: { + // A stale pin left over after switching tenantId, or copied from + // docs — does not name the configured tenant. + 'bad-azure': { + provider: 'azure', + clientId: 'c', + clientSecret: 's', + tenantId: '12345678-1234-1234-1234-123456789012', + issuer: 'https://login.microsoftonline.com/87654321-4321-4321-4321-210987654321/v2.0', + redirectUri: 'https://app.test.com/oauth', + }, + 'github': { + provider: 'github', + clientId: 'github-client', + clientSecret: 'github-secret', + redirectUri: 'https://app.test.com/oauth', + }, + }, + }; + + const providers = initializeProviders(options, logger); + assert.ok(!providers['bad-azure'], 'the misconfigured provider is skipped, not published'); + assert.ok(providers['github'], 'every other provider still initializes'); + assert.ok( + warnings.some((w) => w.includes('bad-azure') && w.includes('invalid Azure issuer pin')), + 'the skip is logged as an error naming the provider' + ); + }); + + it('a non-Azure buildProviderConfig failure still aborts every provider — only the Azure-pin case is downgraded to skip-with-error', () => { + const options = { + providers: { + 'custom-idp': { + provider: 'generic', + clientId: 'c', + clientSecret: 's', + authorizationUrl: 'http://idp.example.com/authorize', // non-https: #231 §4 hard-fail, unrelated to Azure + tokenUrl: 'http://idp.example.com/token', + userInfoUrl: 'http://idp.example.com/userinfo', + jwksUri: 'https://idp.example.com/jwks', + redirectUri: 'https://app.test.com/oauth', + }, + 'github': { + provider: 'github', + clientId: 'github-client', + clientSecret: 'github-secret', + redirectUri: 'https://app.test.com/oauth', + }, + }, + }; + assert.throws(() => initializeProviders(options, undefined), /issuer/); }); @@ -1631,10 +1916,11 @@ describe('OAuth Configuration', () => { ); }); - it('a configured provider alongside an unconfigured one in the same config still gets the §4 fail-fast', () => { + it('a configured provider alongside an unconfigured one in the same config still gets the §4 fail-fast for a non-https authorizationUrl', () => { // Mirrors the test above but with a second, fully configured provider // present: the unconfigured one is skipped silently while the - // configured one still throws for its missing issuer. + // configured one still throws for its missing issuer (non-https — + // discovery can't help, so #231 §4's hard-fail still applies). delete process.env.OAUTH_TEST_231_UNSET_CLIENT_ID_2; const options = { providers: { @@ -1650,9 +1936,9 @@ describe('OAuth Configuration', () => { provider: 'generic', clientId: 'c', clientSecret: 's', - authorizationUrl: 'https://idp.example.com/authorize', - tokenUrl: 'https://idp.example.com/token', - userInfoUrl: 'https://idp.example.com/userinfo', + authorizationUrl: 'http://idp.example.com/authorize', + tokenUrl: 'http://idp.example.com/token', + userInfoUrl: 'http://idp.example.com/userinfo', jwksUri: 'https://idp.example.com/jwks', redirectUri: 'https://app.test.com/oauth', }, @@ -2237,4 +2523,165 @@ describe('OAuth Configuration', () => { }); }); }); + + describe('Graph profile-scope warning (#264)', () => { + function graphProvider(overrides = {}) { + return { + provider: 'azure', + clientId: 'c', + clientSecret: 's', + fetchEmail: true, + redirectUri: 'https://app.test.com/oauth', + ...overrides, + }; + } + + it('warns when fetchEmail is enabled on an Azure provider with scope missing profile', () => { + const warnings = []; + const logger = { ...mockLogger, warn: (msg) => warnings.push(msg) }; + + const options = { + redirectUri: 'https://app.test.com/oauth', + providers: { azure: graphProvider({ scope: 'openid email' }) }, + }; + initializeProviders(options, logger); + + assert.ok( + warnings.some((msg) => msg.includes('azure') && msg.includes('profile')), + 'expected a warning naming the provider and the fix' + ); + }); + + it('does not throw when scope is a non-string (e.g. a YAML list surviving as an array) — this check is documented as never throwing, and must not take down every other provider', () => { + const options = { + redirectUri: 'https://app.test.com/oauth', + providers: { + azure: graphProvider({ scope: ['openid', 'profile', 'email', 'User.Read'] }), + github: { + provider: 'github', + clientId: 'github-client', + clientSecret: 'github-secret', + redirectUri: 'https://app.test.com/oauth', + }, + }, + }; + + const providers = initializeProviders(options, mockLogger); + assert.ok(providers['azure'], 'the azure provider still initializes despite the non-string scope'); + assert.ok(providers['github'], 'every other provider still initializes'); + }); + + it('warns for a generic provider pointed at graph.microsoft.com too (host-based, not provider-name-based)', () => { + const warnings = []; + const logger = { ...mockLogger, warn: (msg) => warnings.push(msg) }; + + const options = { + redirectUri: 'https://app.test.com/oauth', + providers: { + 'custom-graph': { + provider: 'generic', + clientId: 'c', + clientSecret: 's', + authorizationUrl: 'https://login.microsoftonline.com/common/oauth2/v2.0/authorize', + tokenUrl: 'https://login.microsoftonline.com/common/oauth2/v2.0/token', + userInfoUrl: 'https://graph.microsoft.com/v1.0/me', + fetchEmail: true, + scope: 'openid email', + redirectUri: 'https://app.test.com/oauth', + }, + }, + }; + initializeProviders(options, logger); + + assert.ok(warnings.some((msg) => msg.includes('custom-graph') && msg.includes('profile'))); + }); + + it('warns for an azure provider with a non-Graph userInfoUrl override too (provider===azure is sufficient on its own)', () => { + const warnings = []; + const logger = { ...mockLogger, warn: (msg) => warnings.push(msg) }; + + const options = { + redirectUri: 'https://app.test.com/oauth', + providers: { + azure: graphProvider({ + userInfoUrl: 'https://idp.example.com/userinfo', + scope: 'openid email', + }), + }, + }; + initializeProviders(options, logger); + + assert.ok(warnings.some((msg) => msg.includes('azure') && msg.includes('profile'))); + }); + + it('does NOT warn when scope already includes profile', () => { + const warnings = []; + const logger = { ...mockLogger, warn: (msg) => warnings.push(msg) }; + + const options = { + redirectUri: 'https://app.test.com/oauth', + providers: { azure: graphProvider({ scope: 'openid profile email' }) }, + }; + initializeProviders(options, logger); + + assert.ok(!warnings.some((msg) => msg.includes('profile'))); + }); + + it('does NOT warn when fetchEmail is not enabled', () => { + const warnings = []; + const logger = { ...mockLogger, warn: (msg) => warnings.push(msg) }; + + const options = { + redirectUri: 'https://app.test.com/oauth', + providers: { azure: graphProvider({ fetchEmail: false, scope: 'openid email' }) }, + }; + initializeProviders(options, logger); + + assert.ok(!warnings.some((msg) => msg.includes('profile'))); + }); + + it('does NOT warn for a non-Azure, non-Graph provider', () => { + const warnings = []; + const logger = { ...mockLogger, warn: (msg) => warnings.push(msg) }; + + const options = { + redirectUri: 'https://app.test.com/oauth', + providers: { + 'custom-idp': { + provider: 'generic', + clientId: 'c', + clientSecret: 's', + authorizationUrl: 'https://idp.example.com/authorize', + tokenUrl: 'https://idp.example.com/token', + userInfoUrl: 'https://idp.example.com/userinfo', + fetchEmail: true, + scope: 'openid email', + redirectUri: 'https://app.test.com/oauth', + }, + }, + }; + initializeProviders(options, logger); + + assert.ok(!warnings.some((msg) => msg.includes('profile'))); + }); + + it('a throwing logger does not abort provider initialization', () => { + const throwingLogger = { + ...mockLogger, + warn: () => { + throw new Error('boom'); + }, + }; + + const options = { + redirectUri: 'https://app.test.com/oauth', + providers: { azure: graphProvider({ scope: 'openid email' }) }, + }; + + assert.doesNotThrow(() => { + const providers = initializeProviders(options, throwingLogger); + assert.ok(providers.azure, 'provider still initializes despite the throwing logger'); + }); + }); + }); }); diff --git a/test/lib/discovery.test.js b/test/lib/discovery.test.js new file mode 100644 index 0000000..b0de66a --- /dev/null +++ b/test/lib/discovery.test.js @@ -0,0 +1,617 @@ +/** + * Tests for OIDC Discovery 1.0 issuer derivation (HarperFast/oauth#264). + */ + +import { describe, it, beforeEach, afterEach } from 'node:test'; +import assert from 'node:assert/strict'; +import { + startIssuerDiscovery, + awaitDiscoveredIssuer, + _clearDiscoveryCache, + _setDiscoveryTimeouts, + DISCOVERY_LOGIN_AWAIT_BOUND_MS, + MAX_DISCOVERY_PREFIXES, + MAX_CONCURRENT_DISCOVERY_ATTEMPTS, +} from '../../dist/lib/discovery.js'; +import { _setFetch, _setDnsLookup } from '../../dist/lib/mcp/cimd.js'; + +const PUBLIC_IP = { address: '93.184.216.34', family: 4 }; + +function makeDnsOk() { + return async () => [PUBLIC_IP]; +} + +function jsonResponse(body, status = 200) { + const text = typeof body === 'string' ? body : JSON.stringify(body); + const bytes = Buffer.from(text); + return { + ok: status >= 200 && status < 300, + status, + headers: new Map([['content-type', 'application/json']]), + body: { + getReader: () => { + let sent = false; + return { + read: async () => { + if (!sent) { + sent = true; + return { done: false, value: bytes }; + } + return { done: true, value: undefined }; + }, + cancel: () => {}, + }; + }, + }, + }; +} + +/** A fetch mock dispatching by exact URL, from a `{ [url]: doc | status }` map. Anything else 404s. */ +function fetchFromMap(map) { + return async (url) => { + const entry = map[url]; + if (entry === undefined) return jsonResponse({ error: 'not found' }, 404); + if (typeof entry === 'number') return jsonResponse({ error: 'no' }, entry); + return jsonResponse(entry); + }; +} + +describe('OIDC discovery (#264)', () => { + beforeEach(() => { + _clearDiscoveryCache(); + _setDnsLookup(makeDnsOk()); + }); + afterEach(() => { + _setFetch(null); + _setDnsLookup(null); + _setDiscoveryTimeouts(null); + }); + + const AUTH_URL = 'https://idp.example.com/authorize'; + const TOKEN_URL = 'https://idp.example.com/token'; + const JWKS_URI = 'https://idp.example.com/jwks'; + const WELL_KNOWN = 'https://idp.example.com/.well-known/openid-configuration'; + + function validDoc(overrides = {}) { + return { + issuer: 'https://idp.example.com', + authorization_endpoint: AUTH_URL, + token_endpoint: TOKEN_URL, + jwks_uri: JWKS_URI, + ...overrides, + }; + } + + describe('accepted', () => { + it('derives the issuer at the root prefix', async () => { + _setFetch(fetchFromMap({ [WELL_KNOWN]: validDoc() })); + startIssuerDiscovery(AUTH_URL, JWKS_URI, TOKEN_URL, 'custom-idp'); + const issuer = await awaitDiscoveredIssuer(AUTH_URL, JWKS_URI, TOKEN_URL); + assert.equal(issuer, 'https://idp.example.com'); + }); + + it('accepts a root issuer with a trailing slash too', async () => { + _setFetch(fetchFromMap({ [WELL_KNOWN]: validDoc({ issuer: 'https://idp.example.com/' }) })); + startIssuerDiscovery(AUTH_URL, JWKS_URI, TOKEN_URL, 'custom-idp'); + const issuer = await awaitDiscoveredIssuer(AUTH_URL, JWKS_URI, TOKEN_URL); + assert.equal(issuer, 'https://idp.example.com/'); + }); + + it('tries ancestor prefixes and accepts a non-root match', async () => { + const authUrl = 'https://idp.example.com/oauth2/custom/v1/authorize'; + const tokenUrl = 'https://idp.example.com/oauth2/custom/v1/token'; + const jwksUri = 'https://idp.example.com/oauth2/custom/v1/keys'; + const longestWellKnown = 'https://idp.example.com/oauth2/custom/v1/.well-known/openid-configuration'; + const targetWellKnown = 'https://idp.example.com/oauth2/custom/.well-known/openid-configuration'; + + _setFetch( + fetchFromMap({ + [longestWellKnown]: 404, // longest prefix: no document here + [targetWellKnown]: { + issuer: 'https://idp.example.com/oauth2/custom', + authorization_endpoint: authUrl, + token_endpoint: tokenUrl, + jwks_uri: jwksUri, + }, + }) + ); + startIssuerDiscovery(authUrl, jwksUri, tokenUrl, 'okta-custom-as'); + const issuer = await awaitDiscoveredIssuer(authUrl, jwksUri, tokenUrl); + assert.equal(issuer, 'https://idp.example.com/oauth2/custom'); + }); + }); + + describe('refused', () => { + it('issuer mismatch', async () => { + _setFetch(fetchFromMap({ [WELL_KNOWN]: validDoc({ issuer: 'https://wrong.example.com' }) })); + startIssuerDiscovery(AUTH_URL, JWKS_URI, TOKEN_URL, 'custom-idp'); + const issuer = await awaitDiscoveredIssuer(AUTH_URL, JWKS_URI, TOKEN_URL); + assert.equal(issuer, null); + }); + + it('authorization_endpoint mismatch', async () => { + _setFetch(fetchFromMap({ [WELL_KNOWN]: validDoc({ authorization_endpoint: 'https://idp.example.com/other' }) })); + startIssuerDiscovery(AUTH_URL, JWKS_URI, TOKEN_URL, 'custom-idp'); + const issuer = await awaitDiscoveredIssuer(AUTH_URL, JWKS_URI, TOKEN_URL); + assert.equal(issuer, null); + }); + + it('jwks_uri mismatch', async () => { + _setFetch(fetchFromMap({ [WELL_KNOWN]: validDoc({ jwks_uri: 'https://idp.example.com/other-jwks' }) })); + startIssuerDiscovery(AUTH_URL, JWKS_URI, TOKEN_URL, 'custom-idp'); + const issuer = await awaitDiscoveredIssuer(AUTH_URL, JWKS_URI, TOKEN_URL); + assert.equal(issuer, null); + }); + + it('token_endpoint mismatch (#264: two authorities can share an authorization endpoint and jwks_uri but differ in token endpoint)', async () => { + _setFetch(fetchFromMap({ [WELL_KNOWN]: validDoc({ token_endpoint: 'https://idp.example.com/other-token' }) })); + startIssuerDiscovery(AUTH_URL, JWKS_URI, TOKEN_URL, 'custom-idp'); + const issuer = await awaitDiscoveredIssuer(AUTH_URL, JWKS_URI, TOKEN_URL); + assert.equal(issuer, null); + }); + + it('a document that omits token_endpoint entirely refuses rather than bypassing the check', async () => { + const doc = validDoc(); + delete doc.token_endpoint; + _setFetch(fetchFromMap({ [WELL_KNOWN]: doc })); + startIssuerDiscovery(AUTH_URL, JWKS_URI, TOKEN_URL, 'custom-idp'); + const issuer = await awaitDiscoveredIssuer(AUTH_URL, JWKS_URI, TOKEN_URL); + assert.equal(issuer, null); + }); + + it('cross-origin is structurally impossible — a document naming another origin never matches any candidate', async () => { + _setFetch( + fetchFromMap({ + [WELL_KNOWN]: validDoc({ + issuer: 'https://attacker.example.com', + authorization_endpoint: 'https://attacker.example.com/authorize', + jwks_uri: 'https://attacker.example.com/jwks', + token_endpoint: 'https://attacker.example.com/token', + }), + }) + ); + startIssuerDiscovery(AUTH_URL, JWKS_URI, TOKEN_URL, 'custom-idp'); + const issuer = await awaitDiscoveredIssuer(AUTH_URL, JWKS_URI, TOKEN_URL); + assert.equal(issuer, null); + }); + + it('a redirect (3xx) surfaces as non-200 via the pinned, no-follow transport', async () => { + _setFetch(fetchFromMap({ [WELL_KNOWN]: 302 })); + startIssuerDiscovery(AUTH_URL, JWKS_URI, TOKEN_URL, 'custom-idp'); + const issuer = await awaitDiscoveredIssuer(AUTH_URL, JWKS_URI, TOKEN_URL); + assert.equal(issuer, null); + }); + + it('a hanging fetch is bounded by the per-fetch timeout', async () => { + _setDiscoveryTimeouts({ perFetchMs: 25, overallBudgetMs: 200 }); + // A faithful mock respects the abort signal, exactly like the real + // https.request-based transport (which node:https aborts natively). + _setFetch( + (url, init) => + new Promise((_resolve, reject) => { + init.signal.addEventListener('abort', () => reject(new Error('aborted'))); + }) + ); + startIssuerDiscovery(AUTH_URL, JWKS_URI, TOKEN_URL, 'custom-idp'); + const issuer = await awaitDiscoveredIssuer(AUTH_URL, JWKS_URI, TOKEN_URL, undefined, 500); + assert.equal(issuer, null); + }); + + it('an oversize document is rejected', async () => { + const big = { ...validDoc(), padding: 'x'.repeat(10 * 1024 * 1024) }; + _setFetch(fetchFromMap({ [WELL_KNOWN]: big })); + startIssuerDiscovery(AUTH_URL, JWKS_URI, TOKEN_URL, 'custom-idp'); + const issuer = await awaitDiscoveredIssuer(AUTH_URL, JWKS_URI, TOKEN_URL); + assert.equal(issuer, null); + }); + }); + + describe('bounds', () => { + it('prefix count is capped at MAX_DISCOVERY_PREFIXES for a pathological long path', async () => { + const segments = Array.from({ length: 30 }, (_, i) => `seg${i}`); + const authUrl = `https://idp.example.com/${segments.join('/')}`; + let calls = 0; + _setFetch(async () => { + calls++; + return jsonResponse({ error: 'no' }, 404); + }); + startIssuerDiscovery(authUrl, JWKS_URI, TOKEN_URL, 'deep-idp'); + await awaitDiscoveredIssuer(authUrl, JWKS_URI, TOKEN_URL); + assert.ok(calls <= MAX_DISCOVERY_PREFIXES, `expected at most ${MAX_DISCOVERY_PREFIXES} attempts, got ${calls}`); + }); + + it('root is still tried for a pathological long path even though capped at MAX_DISCOVERY_PREFIXES', async () => { + // Before the fix, the cap always kept the MAX_DISCOVERY_PREFIXES + // LONGEST prefixes and never reached root (dropping all segments) — + // so a document that only exists at root was never found. + const segments = Array.from({ length: 30 }, (_, i) => `seg${i}`); + const authUrl = `https://idp.example.com/${segments.join('/')}`; + _setFetch( + fetchFromMap({ + 'https://idp.example.com/.well-known/openid-configuration': { + issuer: 'https://idp.example.com', + authorization_endpoint: authUrl, + token_endpoint: TOKEN_URL, + jwks_uri: JWKS_URI, + }, + }) + ); + startIssuerDiscovery(authUrl, JWKS_URI, TOKEN_URL, 'deep-idp'); + const issuer = await awaitDiscoveredIssuer(authUrl, JWKS_URI, TOKEN_URL); + assert.equal(issuer, 'https://idp.example.com'); + }); + + it('a root authorizationUrl path ("/") is still a candidate, not skipped entirely', async () => { + // `/`'s pathname has zero segments, so the drop-one-segment loop never + // ran at all before the fix, and root was never tried. + const authUrl = 'https://idp.example.com/'; + _setFetch( + fetchFromMap({ + 'https://idp.example.com/.well-known/openid-configuration': { + issuer: 'https://idp.example.com', + authorization_endpoint: authUrl, + token_endpoint: TOKEN_URL, + jwks_uri: JWKS_URI, + }, + }) + ); + startIssuerDiscovery(authUrl, JWKS_URI, TOKEN_URL, 'root-idp'); + const issuer = await awaitDiscoveredIssuer(authUrl, JWKS_URI, TOKEN_URL); + assert.equal(issuer, 'https://idp.example.com'); + }); + + it('the overall budget stops trying further candidates once exhausted', async () => { + _setDiscoveryTimeouts({ perFetchMs: 500, overallBudgetMs: 30 }); + let calls = 0; + _setFetch(async () => { + calls++; + await new Promise((resolve) => setTimeout(resolve, 50)); + return jsonResponse({ error: 'no' }, 404); + }); + const authUrl = 'https://idp.example.com/a/b/c/d/authorize'; + startIssuerDiscovery(authUrl, JWKS_URI, TOKEN_URL, 'slow-idp'); + await awaitDiscoveredIssuer(authUrl, JWKS_URI, TOKEN_URL, undefined, 2000); + assert.ok(calls < 5, `expected the overall budget to stop further candidates, got ${calls} calls`); + }); + }); + + describe('concurrency cap', () => { + it('caps simultaneous discovery attempts at MAX_CONCURRENT_DISCOVERY_ATTEMPTS, and every target still resolves to its own issuer', async () => { + // More targets than CIMD's real, shared 2-permit DNS gate, started + // synchronously — several will see `temporarily_unavailable` on their + // first lookup. A short capacity-retry delay keeps that from + // depending on a race against this test's own drain timing. + _setDiscoveryTimeouts({ capacityRetryDelayMs: 5 }); + let inFlight = 0; + let maxObservedInFlight = 0; + let pendingResolvers = []; + _setFetch( + (url) => + new Promise((resolve) => { + inFlight++; + maxObservedInFlight = Math.max(maxObservedInFlight, inFlight); + pendingResolvers.push(() => { + inFlight--; + // Per-target document, not a fixed one — a target's own + // authorizationUrl/jwksUri/tokenUrl must validate against it. + const origin = new URL(url).origin; + resolve( + jsonResponse({ + issuer: origin, + authorization_endpoint: `${origin}/authorize`, + jwks_uri: `${origin}/jwks`, + token_endpoint: `${origin}/token`, + }) + ); + }); + }) + ); + + const providerCount = MAX_CONCURRENT_DISCOVERY_ATTEMPTS + 5; + const targets = []; + for (let i = 0; i < providerCount; i++) { + const authUrl = `https://idp${i}.example.com/authorize`; + const jwksUri = `https://idp${i}.example.com/jwks`; + const tokenUrl = `https://idp${i}.example.com/token`; + startIssuerDiscovery(authUrl, jwksUri, tokenUrl, `idp-${i}`); + targets.push({ authUrl, jwksUri, tokenUrl }); + } + + // Let every immediately-startable attempt begin. + await new Promise((resolve) => setTimeout(resolve, 10)); + assert.ok( + maxObservedInFlight <= MAX_CONCURRENT_DISCOVERY_ATTEMPTS, + `expected at most ${MAX_CONCURRENT_DISCOVERY_ATTEMPTS} in flight, saw ${maxObservedInFlight}` + ); + + // Drain in rounds until every target has settled, so the test leaves + // no dangling promises behind. + for (let round = 0; round < providerCount && pendingResolvers.length > 0; round++) { + const toResolve = pendingResolvers; + pendingResolvers = []; + toResolve.forEach((resolve) => resolve()); + await new Promise((resolve) => setTimeout(resolve, 5)); + } + const issuers = await Promise.all(targets.map((t) => awaitDiscoveredIssuer(t.authUrl, t.jwksUri, t.tokenUrl))); + targets.forEach((t, i) => { + assert.equal( + issuers[i], + new URL(t.authUrl).origin, + `target ${i} (${t.authUrl}) must resolve to its own issuer` + ); + }); + }); + + it('a DNS-capacity rejection (CIMD’s shared, process-wide 2-permit lookup gate) retries instead of being cached as "not found" (#271 follow-up)', async () => { + // Two deliberately-held-open "slot holders" exhaust CIMD's real + // (unmocked) MAX_CONCURRENT_DNS=2 permit; a third target started + // while both slots are held synchronously sees `temporarily_unavailable` + // on its first lookup. Before the fix, that was treated identically to + // "no document at this prefix" and cached as a definitive failure. + _setDiscoveryTimeouts({ overallBudgetMs: 2000, capacityRetryDelayMs: 10 }); + const pendingDnsResolvers = []; + _setDnsLookup(() => new Promise((resolve) => pendingDnsResolvers.push(resolve))); + const fetchedOrigins = []; + _setFetch( + (url) => + new Promise((resolve) => { + const origin = new URL(url).origin; + fetchedOrigins.push(origin); + resolve( + jsonResponse({ + issuer: origin, + authorization_endpoint: `${origin}/authorize`, + jwks_uri: `${origin}/jwks`, + token_endpoint: `${origin}/token`, + }) + ); + }) + ); + + const slotHolder = (i) => ({ + authUrl: `https://slot${i}.example.com/authorize`, + jwksUri: `https://slot${i}.example.com/jwks`, + tokenUrl: `https://slot${i}.example.com/token`, + }); + const holders = [slotHolder(0), slotHolder(1)]; + const contender = { + authUrl: 'https://contender.example.com/authorize', + jwksUri: 'https://contender.example.com/jwks', + tokenUrl: 'https://contender.example.com/token', + }; + + // Synchronous, back-to-back: the first two calls take CIMD's two real + // DNS permits and hold them (their `_setDnsLookup` promise is never + // resolved yet); the third's own `boundedDnsLookup` call synchronously + // observes both permits already taken. + startIssuerDiscovery(holders[0].authUrl, holders[0].jwksUri, holders[0].tokenUrl, 'slot-0'); + startIssuerDiscovery(holders[1].authUrl, holders[1].jwksUri, holders[1].tokenUrl, 'slot-1'); + startIssuerDiscovery(contender.authUrl, contender.jwksUri, contender.tokenUrl, 'contender'); + + // The contender must still be retrying (never reached `_fetch`, never + // cached a result) while both real permits stay held — checked by + // fetch-call tracking rather than `awaitDiscoveredIssuer`, since that + // call itself consumes the one-time bounded-await slot for this key. + await new Promise((resolve) => setTimeout(resolve, 60)); + assert.ok( + !fetchedOrigins.includes('https://contender.example.com'), + 'contender must still be retrying the DNS capacity gate, not have reached the fetch step yet' + ); + + // Free both held permits (and auto-resolve every lookup from here on, + // including the contender's own first real one); the contender's + // retry loop should now pick up a free slot and resolve to its issuer. + pendingDnsResolvers.forEach((resolve) => resolve([PUBLIC_IP])); + _setDnsLookup(makeDnsOk()); + const issuer = await awaitDiscoveredIssuer(contender.authUrl, contender.jwksUri, contender.tokenUrl); + assert.equal(issuer, 'https://contender.example.com'); + }); + }); + + describe('boot does not wait; first-caller-only bounded await', () => { + it('startIssuerDiscovery returns immediately without waiting for the fetch to resolve', async () => { + _setFetch(() => new Promise(() => {})); // never resolves + const start = Date.now(); + startIssuerDiscovery(AUTH_URL, JWKS_URI, TOKEN_URL, 'custom-idp'); + assert.ok(Date.now() - start < 50, 'startIssuerDiscovery must not block'); + // Give the mocked DNS lookup's microtask a tick to resolve and the + // (still-mocked, intentionally never-resolving) fetch to actually get + // invoked before this test ends and `afterEach` resets the fetch mock + // — otherwise that call races the reset and can hit the real network + // (against the mocked DNS address) instead of the mock. + await new Promise((resolve) => setTimeout(resolve, 10)); + }); + + it('the first caller awaits within the bound and is upgraded if discovery resolves in time', async () => { + _setFetch(async () => { + await new Promise((resolve) => setTimeout(resolve, 20)); + return jsonResponse(validDoc()); + }); + startIssuerDiscovery(AUTH_URL, JWKS_URI, TOKEN_URL, 'custom-idp'); + const issuer = await awaitDiscoveredIssuer( + AUTH_URL, + JWKS_URI, + TOKEN_URL, + undefined, + DISCOVERY_LOGIN_AWAIT_BOUND_MS + ); + assert.equal(issuer, 'https://idp.example.com'); + }); + + it('a second, concurrent caller does not wait, even though discovery resolves moments later', async () => { + let resolveFetch; + _setFetch( + () => + new Promise((resolve) => { + resolveFetch = () => resolve(jsonResponse(validDoc())); + }) + ); + startIssuerDiscovery(AUTH_URL, JWKS_URI, TOKEN_URL, 'custom-idp'); + + const firstCallerPromise = awaitDiscoveredIssuer(AUTH_URL, JWKS_URI, TOKEN_URL); + // Give the first caller's `entry.awaited = true` a tick to land before the second caller checks it. + await new Promise((resolve) => setTimeout(resolve, 5)); + const secondCallerResult = await awaitDiscoveredIssuer(AUTH_URL, JWKS_URI, TOKEN_URL); + assert.equal(secondCallerResult, null, 'the second, concurrent caller does not wait'); + + resolveFetch(); + const firstCallerResult = await firstCallerPromise; + assert.equal( + firstCallerResult, + 'https://idp.example.com', + 'the first caller is upgraded once discovery resolves' + ); + }); + }); + + describe('cache: settled results, retry cooldown, dynamic providers', () => { + it('a settled entry (success or failure) costs one lookup — no further fetches', async () => { + let calls = 0; + _setFetch(async () => { + calls++; + return jsonResponse(validDoc()); + }); + startIssuerDiscovery(AUTH_URL, JWKS_URI, TOKEN_URL, 'custom-idp'); + await awaitDiscoveredIssuer(AUTH_URL, JWKS_URI, TOKEN_URL); + const callsAfterFirst = calls; + for (let i = 0; i < 5; i++) { + await awaitDiscoveredIssuer(AUTH_URL, JWKS_URI, TOKEN_URL); + } + assert.equal(calls, callsAfterFirst, 'no additional fetches for a settled, cached result'); + }); + + it('a failed entry is retried only via a fresh startIssuerDiscovery call after the cooldown, never from the read path', async () => { + _setFetch(fetchFromMap({ [WELL_KNOWN]: 500 })); + startIssuerDiscovery(AUTH_URL, JWKS_URI, TOKEN_URL, 'custom-idp'); + assert.equal(await awaitDiscoveredIssuer(AUTH_URL, JWKS_URI, TOKEN_URL), null); + + // Reading again (no reload) never retries, regardless of how long we wait. + let calls = 0; + _setFetch(async () => { + calls++; + return jsonResponse(validDoc()); + }); + await awaitDiscoveredIssuer(AUTH_URL, JWKS_URI, TOKEN_URL); + assert.equal(calls, 0, 'the read path never starts a retry'); + + // A fresh startIssuerDiscovery call (a live reload) is a no-op within the cooldown window. + startIssuerDiscovery(AUTH_URL, JWKS_URI, TOKEN_URL, 'custom-idp'); + await new Promise((resolve) => setTimeout(resolve, 10)); + assert.equal(calls, 0, 'still within the cooldown window'); + }); + + it('a failed entry IS retried once the cooldown has elapsed, via a fresh startIssuerDiscovery call', async () => { + _setDiscoveryTimeouts({ retryCooldownMs: 20 }); + _setFetch(fetchFromMap({ [WELL_KNOWN]: 500 })); + startIssuerDiscovery(AUTH_URL, JWKS_URI, TOKEN_URL, 'custom-idp'); + assert.equal(await awaitDiscoveredIssuer(AUTH_URL, JWKS_URI, TOKEN_URL), null); + + await new Promise((resolve) => setTimeout(resolve, 30)); // past the (shortened) cooldown + _setFetch(fetchFromMap({ [WELL_KNOWN]: validDoc() })); + startIssuerDiscovery(AUTH_URL, JWKS_URI, TOKEN_URL, 'custom-idp'); + const issuer = await awaitDiscoveredIssuer(AUTH_URL, JWKS_URI, TOKEN_URL); + assert.equal(issuer, 'https://idp.example.com'); + }); + + it('a dynamic provider reads an already-cached result with zero fetches of its own', async () => { + _setFetch(fetchFromMap({ [WELL_KNOWN]: validDoc() })); + startIssuerDiscovery(AUTH_URL, JWKS_URI, TOKEN_URL, 'static-idp'); + // Settle the entry first (this is the "static provider" side). + const settled = await awaitDiscoveredIssuer(AUTH_URL, JWKS_URI, TOKEN_URL); + assert.equal(settled, 'https://idp.example.com'); + + // A "dynamic provider" reading the same already-settled key must never fetch. + _setFetch(() => { + throw new Error('a dynamic provider must never fetch'); + }); + const issuer = await awaitDiscoveredIssuer(AUTH_URL, JWKS_URI, TOKEN_URL); + assert.equal(issuer, 'https://idp.example.com'); + }); + + it('a dynamic provider with no matching cache entry gets null immediately, with no fetch', async () => { + _setFetch(() => { + throw new Error('must never fetch for an uncached key'); + }); + const issuer = await awaitDiscoveredIssuer( + 'https://never-started.example.com/authorize', + 'https://never-started.example.com/jwks', + 'https://never-started.example.com/token' + ); + assert.equal(issuer, null); + }); + }); + + describe('containment', () => { + it('a throwing logger at every level does not prevent kickoff, success logging, or failure logging from completing', async () => { + const throwingLogger = { + debug: () => { + throw new Error('boom'); + }, + info: () => { + throw new Error('boom'); + }, + warn: () => { + throw new Error('boom'); + }, + error: () => { + throw new Error('boom'); + }, + }; + _setFetch(fetchFromMap({ [WELL_KNOWN]: validDoc() })); + startIssuerDiscovery(AUTH_URL, JWKS_URI, TOKEN_URL, 'custom-idp', throwingLogger); + const issuer = await awaitDiscoveredIssuer(AUTH_URL, JWKS_URI, TOKEN_URL, throwingLogger); + assert.equal(issuer, 'https://idp.example.com'); + }); + + it('a throwing logger does not prevent a failure from being reported as null', async () => { + const throwingLogger = { + debug: () => { + throw new Error('boom'); + }, + info: () => { + throw new Error('boom'); + }, + warn: () => { + throw new Error('boom'); + }, + error: () => { + throw new Error('boom'); + }, + }; + _setFetch(fetchFromMap({ [WELL_KNOWN]: 500 })); + startIssuerDiscovery(AUTH_URL, JWKS_URI, TOKEN_URL, 'custom-idp', throwingLogger); + const issuer = await awaitDiscoveredIssuer(AUTH_URL, JWKS_URI, TOKEN_URL, throwingLogger); + assert.equal(issuer, null); + }); + + it('a DNS failure resolves to null, not a rejection', async () => { + _setDnsLookup(async () => { + throw new Error('NXDOMAIN'); + }); + startIssuerDiscovery(AUTH_URL, JWKS_URI, TOKEN_URL, 'custom-idp'); + const issuer = await awaitDiscoveredIssuer(AUTH_URL, JWKS_URI, TOKEN_URL); + assert.equal(issuer, null); + }); + }); + + describe('private/loopback IdP endpoints (#264) — operator-configured input, not attacker-controlled', () => { + it('succeeds against a host that resolves to a private address, unlike CIMD client_id resolution', async () => { + // CIMD's own SSRF gate blocks this; discovery deliberately opts out + // (`allowPrivateAddresses`) because the endpoint here is + // operator-configured, the same trust level as `jwksUri` itself. + _setDnsLookup(async () => [{ address: '10.0.0.5', family: 4 }]); + _setFetch(fetchFromMap({ [WELL_KNOWN]: validDoc() })); + startIssuerDiscovery(AUTH_URL, JWKS_URI, TOKEN_URL, 'self-hosted-idp'); + const issuer = await awaitDiscoveredIssuer(AUTH_URL, JWKS_URI, TOKEN_URL); + assert.equal(issuer, 'https://idp.example.com'); + }); + + it('succeeds against a loopback-resolving host', async () => { + _setDnsLookup(async () => [{ address: '127.0.0.1', family: 4 }]); + _setFetch(fetchFromMap({ [WELL_KNOWN]: validDoc() })); + startIssuerDiscovery(AUTH_URL, JWKS_URI, TOKEN_URL, 'self-hosted-idp'); + const issuer = await awaitDiscoveredIssuer(AUTH_URL, JWKS_URI, TOKEN_URL); + assert.equal(issuer, 'https://idp.example.com'); + }); + }); +}); diff --git a/test/lib/dynamicProviderCache.test.js b/test/lib/dynamicProviderCache.test.js index b6c614f..55798b1 100644 --- a/test/lib/dynamicProviderCache.test.js +++ b/test/lib/dynamicProviderCache.test.js @@ -306,4 +306,138 @@ describe('DynamicProviderCache', () => { } }); }); + + describe('Azure issuer-pin failure cooldown (#264/#271)', () => { + it('returns undefined when no failure has been recorded', () => { + const cache = new DynamicProviderCache(); + assert.equal(cache.getAzurePinFailure('azure-tenant-a'), undefined); + }); + + it('returns the recorded message within the cooldown window', () => { + const cache = new DynamicProviderCache(); + cache.recordAzurePinFailure('azure-tenant-a', 'issuer names a different tenant'); + assert.equal(cache.getAzurePinFailure('azure-tenant-a'), 'issuer names a different tenant'); + }); + + it('expires after the fixed 30s cooldown, independent of the success TTL', () => { + // A long success TTL must not also extend the failure cooldown — a + // fixed config should recover fast, not wait for a long/forever TTL. + const cache = new DynamicProviderCache(true); + cache.recordAzurePinFailure('azure-tenant-a', 'issuer names a different tenant'); + + const restoreWithin = advanceTime(29_000); + try { + assert.equal(cache.getAzurePinFailure('azure-tenant-a'), 'issuer names a different tenant'); + } finally { + restoreWithin(); + } + + const restoreAfter = advanceTime(31_000); + try { + assert.equal(cache.getAzurePinFailure('azure-tenant-a'), undefined); + } finally { + restoreAfter(); + } + }); + + it('is cleared by clearAzurePinFailure (a subsequent successful resolution)', () => { + const cache = new DynamicProviderCache(); + cache.recordAzurePinFailure('azure-tenant-a', 'issuer names a different tenant'); + cache.clearAzurePinFailure('azure-tenant-a'); + assert.equal(cache.getAzurePinFailure('azure-tenant-a'), undefined); + }); + + it('is scoped per provider name', () => { + const cache = new DynamicProviderCache(); + cache.recordAzurePinFailure('azure-tenant-a', 'tenant A pin invalid'); + assert.equal(cache.getAzurePinFailure('azure-tenant-b'), undefined); + }); + + it('clear() also clears recorded Azure-pin failures', () => { + const cache = new DynamicProviderCache(); + cache.recordAzurePinFailure('azure-tenant-a', 'tenant A pin invalid'); + cache.clear(); + assert.equal(cache.getAzurePinFailure('azure-tenant-a'), undefined); + }); + }); + + describe('wrapLoggerForDynamicResolution', () => { + it('returns undefined unchanged when no logger is given', () => { + const cache = new DynamicProviderCache(); + assert.equal(cache.wrapLoggerForDynamicResolution('azure-provider', undefined), undefined); + }); + + it('logs the first warn() call for a provider', () => { + const cache = new DynamicProviderCache(); + const warnings = []; + const logger = { warn: (msg) => warnings.push(msg) }; + const wrapped = cache.wrapLoggerForDynamicResolution('azure-provider', logger); + wrapped.warn('tenant mismatch'); + assert.deepEqual(warnings, ['tenant mismatch']); + }); + + it('drops a repeat of the exact same message for the same provider — the dynamic-resolution path can otherwise re-log it on every request', () => { + const cache = new DynamicProviderCache(); + const warnings = []; + const logger = { warn: (msg) => warnings.push(msg) }; + const wrapped = cache.wrapLoggerForDynamicResolution('azure-provider', logger); + wrapped.warn('tenant mismatch'); + wrapped.warn('tenant mismatch'); + wrapped.warn('tenant mismatch'); + assert.deepEqual(warnings, ['tenant mismatch']); + }); + + it('logs a DIFFERENT message for the same provider — dedup is keyed by message, not just by provider', () => { + const cache = new DynamicProviderCache(); + const warnings = []; + const logger = { warn: (msg) => warnings.push(msg) }; + const wrapped = cache.wrapLoggerForDynamicResolution('azure-provider', logger); + wrapped.warn('tenant mismatch'); + wrapped.warn('a different warning'); + assert.deepEqual(warnings, ['tenant mismatch', 'a different warning']); + }); + + it('is scoped per provider — the same message still logs once for a different provider', () => { + const cache = new DynamicProviderCache(); + const warnings = []; + const logger = { warn: (msg) => warnings.push(msg) }; + cache.wrapLoggerForDynamicResolution('azure-provider-a', logger).warn('tenant mismatch'); + cache.wrapLoggerForDynamicResolution('azure-provider-b', logger).warn('tenant mismatch'); + assert.deepEqual(warnings, ['tenant mismatch', 'tenant mismatch']); + }); + + it('passes info/error/debug through unchanged, with no dedup', () => { + const cache = new DynamicProviderCache(); + const infos = []; + const logger = { info: (msg) => infos.push(msg) }; + const wrapped = cache.wrapLoggerForDynamicResolution('azure-provider', logger); + wrapped.info('resolved'); + wrapped.info('resolved'); + assert.deepEqual(infos, ['resolved', 'resolved']); + }); + + it('delegates to a class-based logger whose methods live on its prototype, not as its own properties — a `{ ...logger }` spread would silently drop them', () => { + const infos = []; + class ClassLogger { + info(msg) { + infos.push(msg); + } + } + const cache = new DynamicProviderCache(); + const wrapped = cache.wrapLoggerForDynamicResolution('azure-provider', new ClassLogger()); + wrapped.info('resolved'); + assert.deepEqual(infos, ['resolved']); + }); + + it('clear() resets the dedup state, so a warning can log again afterward', () => { + const cache = new DynamicProviderCache(); + const warnings = []; + const logger = { warn: (msg) => warnings.push(msg) }; + const wrapped = cache.wrapLoggerForDynamicResolution('azure-provider', logger); + wrapped.warn('tenant mismatch'); + cache.clear(); + wrapped.warn('tenant mismatch'); + assert.deepEqual(warnings, ['tenant mismatch', 'tenant mismatch']); + }); + }); }); diff --git a/test/lib/mcp/cimd.test.js b/test/lib/mcp/cimd.test.js index a60a631..3ac634e 100644 --- a/test/lib/mcp/cimd.test.js +++ b/test/lib/mcp/cimd.test.js @@ -20,6 +20,7 @@ import { resolveClient, cimdEnabled, CimdClientError, + fetchPinnedBoundedJson, _setDnsLookup, _setFetch, _clearCimdCache, @@ -329,6 +330,60 @@ describe('resolveCimdClient — SSRF DNS gate', () => { }); }); +describe('fetchPinnedBoundedJson — allowPrivateAddresses (HarperFast/oauth#264)', () => { + afterEach(() => { + _setDnsLookup(null); + _setFetch(null); + }); + + const OPTS = { + label: 'test document', + tag: 'test', + accept: 'application/json', + contentTypes: ['application/json'], + timeoutMs: 1000, + maxBytes: 65536, + }; + + it('rejects a private address by default, same as every other caller', async () => { + _setDnsLookup(makeDnsOk([{ address: '10.0.0.5', family: 4 }])); + _setFetch(makeOkFetch({ ok: true })); + await assert.rejects( + () => fetchPinnedBoundedJson('https://internal.example.com/doc.json', OPTS), + /could not be resolved to a permitted address/ + ); + }); + + it('allows a private address when allowPrivateAddresses is set', async () => { + _setDnsLookup(makeDnsOk([{ address: '10.0.0.5', family: 4 }])); + _setFetch(makeOkFetch({ ok: true })); + const { body } = await fetchPinnedBoundedJson('https://internal.example.com/doc.json', { + ...OPTS, + allowPrivateAddresses: true, + }); + assert.deepEqual(JSON.parse(body), { ok: true }); + }); + + it('allows a loopback address when allowPrivateAddresses is set', async () => { + _setDnsLookup(makeDnsOk([{ address: '127.0.0.1', family: 4 }])); + _setFetch(makeOkFetch({ ok: true })); + const { body } = await fetchPinnedBoundedJson('https://internal.example.com/doc.json', { + ...OPTS, + allowPrivateAddresses: true, + }); + assert.deepEqual(JSON.parse(body), { ok: true }); + }); + + it('still rejects an unclassified address family even with allowPrivateAddresses set', async () => { + _setDnsLookup(makeDnsOk([{ address: '10.0.0.5', family: 5 }])); + _setFetch(makeOkFetch({ ok: true })); + await assert.rejects( + () => fetchPinnedBoundedJson('https://internal.example.com/doc.json', { ...OPTS, allowPrivateAddresses: true }), + /could not be resolved to a permitted address/ + ); + }); +}); + describe('resolveCimdClient — concurrency + pinning', () => { beforeEach(() => _clearCimdCache()); afterEach(() => { diff --git a/test/lib/providers/azure.test.js b/test/lib/providers/azure.test.js index 3945ccd..c3f29df 100644 --- a/test/lib/providers/azure.test.js +++ b/test/lib/providers/azure.test.js @@ -67,4 +67,30 @@ describe('Azure Provider', () => { message: 'Azure AD provider requires tenantId configuration', }); }); + + describe('alias tenantIds no longer set a literal (wrong) issuer (#264)', () => { + // configure('common'/'organizations'/'consumers') previously set a + // literal issuer like 'https://login.microsoftonline.com/common/v2.0', + // which no real token's `iss` could ever equal. Deriving — or + // intentionally leaving unset — the issuer for these aliases is now + // `resolveAzureIssuerBinding`'s job (src/lib/azureIssuer.ts); the + // preset itself only derives endpoints for them. + for (const alias of ['common', 'organizations', 'consumers']) { + it(`configure('${alias}') derives endpoints but no literal issuer`, () => { + const azure = getProvider('azure'); + const configured = azure.configure(alias); + assert.ok(configured.authorizationUrl.includes(`/${alias}/`)); + assert.ok(configured.tokenUrl.includes(`/${alias}/`)); + assert.ok(configured.jwksUri.includes(`/${alias}/`)); + assert.equal(configured.issuer, undefined); + }); + } + + it('a real tenant GUID still gets a direct issuer (unaffected)', () => { + const azure = getProvider('azure'); + const tenantId = '12345678-1234-1234-1234-123456789012'; + const configured = azure.configure(tenantId); + assert.equal(configured.issuer, `https://login.microsoftonline.com/${tenantId}/v2.0`); + }); + }); }); diff --git a/test/lib/providers/okta.test.js b/test/lib/providers/okta.test.js index 29cf3e6..cee6565 100644 --- a/test/lib/providers/okta.test.js +++ b/test/lib/providers/okta.test.js @@ -111,4 +111,65 @@ describe('Okta Provider', () => { assert.equal(configured.authorizationUrl, 'https://dev-12345.oktapreview.com/oauth2/v1/authorize'); assert.equal(configured.issuer, 'https://dev-12345.oktapreview.com'); }); + + describe('custom authorization server (authServer, #264)', () => { + it('derives path-inclusive endpoints and issuer when authServer is given', () => { + const okta = getProvider('okta'); + const configured = okta.configure('dev-1.okta.com', 'default'); + + assert.equal(configured.authorizationUrl, 'https://dev-1.okta.com/oauth2/default/v1/authorize'); + assert.equal(configured.tokenUrl, 'https://dev-1.okta.com/oauth2/default/v1/token'); + assert.equal(configured.userInfoUrl, 'https://dev-1.okta.com/oauth2/default/v1/userinfo'); + assert.equal(configured.jwksUri, 'https://dev-1.okta.com/oauth2/default/v1/keys'); + // Unlike the org AS, a custom AS issuer includes the auth-server path. + assert.equal(configured.issuer, 'https://dev-1.okta.com/oauth2/default'); + }); + + it('supports a generated auth-server id', () => { + const okta = getProvider('okta'); + const configured = okta.configure('dev-1.okta.com', 'aus1a2b3c4d5e6f7g8h9'); + + assert.equal(configured.authorizationUrl, 'https://dev-1.okta.com/oauth2/aus1a2b3c4d5e6f7g8h9/v1/authorize'); + assert.equal(configured.issuer, 'https://dev-1.okta.com/oauth2/aus1a2b3c4d5e6f7g8h9'); + }); + + it('omitted authServer keeps the org authorization server (unchanged)', () => { + const okta = getProvider('okta'); + const configured = okta.configure('dev-1.okta.com'); + + assert.equal(configured.authorizationUrl, 'https://dev-1.okta.com/oauth2/v1/authorize'); + assert.equal(configured.issuer, 'https://dev-1.okta.com'); + }); + + it('rejects an explicitly empty authServer instead of silently falling back to the org authorization server', () => { + // `if (authServer)` would treat '' the same as omitted (undefined) and + // silently derive the org AS — losing any custom-AS access policies the + // operator thought they were pinning. `authServer !== undefined` makes + // an explicit '' reach validateOktaAuthServer, which rejects it. + const okta = getProvider('okta'); + assert.throws(() => okta.configure('dev-1.okta.com', ''), /authServer/); + }); + + it('rejects an explicit null authServer the same way', () => { + const okta = getProvider('okta'); + assert.throws(() => okta.configure('dev-1.okta.com', null), /authServer/); + }); + + it('rejects an authServer containing a path separator', () => { + const okta = getProvider('okta'); + assert.throws(() => okta.configure('dev-1.okta.com', 'a/../b'), /authServer/); + assert.throws(() => okta.configure('dev-1.okta.com', '/etc/passwd'), /authServer/); + }); + + it('rejects an authServer with disallowed characters', () => { + const okta = getProvider('okta'); + assert.throws(() => okta.configure('dev-1.okta.com', 'a b'), /authServer/); + assert.throws(() => okta.configure('dev-1.okta.com', 'a?b'), /authServer/); + }); + + it('rejects an authServer over the length limit', () => { + const okta = getProvider('okta'); + assert.throws(() => okta.configure('dev-1.okta.com', 'a'.repeat(65)), /authServer/); + }); + }); }); diff --git a/test/lib/tenantManager.test.js b/test/lib/tenantManager.test.js index 3a6e235..e445799 100644 --- a/test/lib/tenantManager.test.js +++ b/test/lib/tenantManager.test.js @@ -5,6 +5,7 @@ import { describe, it } from 'node:test'; import assert from 'node:assert/strict'; import { TenantManager } from '../../dist/lib/tenantManager.js'; +import { OAuthProvider } from '../../dist/lib/OAuthProvider.js'; describe('TenantManager', () => { describe('Okta tenant registration', () => { @@ -48,6 +49,41 @@ describe('TenantManager', () => { ); }); + it('passes authServer through to Okta configure() for a custom authorization server (#264)', () => { + const manager = new TenantManager(); + + manager.registerTenant({ + tenantId: 'acme-corp', + name: 'Acme Corporation', + provider: 'okta', + domain: 'acme.okta.com', + authServer: 'default', + clientId: 'okta-client-id', + clientSecret: 'okta-secret', + }); + + const tenant = manager.getTenant('acme-corp'); + assert.equal(tenant.providerConfig.authorizationUrl, 'https://acme.okta.com/oauth2/default/v1/authorize'); + assert.equal(tenant.providerConfig.issuer, 'https://acme.okta.com/oauth2/default'); + }); + + it('omitted authServer keeps the org authorization server, unchanged', () => { + const manager = new TenantManager(); + + manager.registerTenant({ + tenantId: 'acme-corp', + name: 'Acme Corporation', + provider: 'okta', + domain: 'acme.okta.com', + clientId: 'okta-client-id', + clientSecret: 'okta-secret', + }); + + const tenant = manager.getTenant('acme-corp'); + assert.equal(tenant.providerConfig.authorizationUrl, 'https://acme.okta.com/oauth2/v1/authorize'); + assert.equal(tenant.providerConfig.issuer, 'https://acme.okta.com'); + }); + it('should throw error for a mixed-case "Okta" tenant without domain', () => { const manager = new TenantManager(); @@ -137,6 +173,62 @@ describe('TenantManager', () => { assert.ok(tenant); assert.equal(tenant.config.provider, 'microsoft'); }); + + it('a tenant built here (bypassing buildProviderConfig entirely) still gets the safe Azure binding once an OAuthProvider is constructed from it (#264)', () => { + // TenantManager assembles OAuthProviderConfig directly and never calls + // config.ts's buildProviderConfig — resolveAzureIssuerBinding must + // still run, at OAuthProvider construction time, for this path. + const manager = new TenantManager(); + const guid = '12345678-1234-1234-1234-123456789012'; + + manager.registerTenant({ + tenantId: 'globex-corp', + name: 'Globex Corporation', + provider: 'azure', + azureTenantId: 'common', + clientId: 'azure-client-id', + clientSecret: 'azure-secret', + additionalConfig: { + issuer: `https://login.microsoftonline.com/${guid}/v2.0`, + redirectUri: 'https://app.test.com/oauth', + }, + }); + + const config = manager.getTenant('globex-corp').providerConfig; + assert.ok(config); + // Still the shared, unrewritten alias endpoint at this point — + // TenantManager itself does nothing Azure-specific. + assert.ok(config.jwksUri.includes('/common/')); + + const provider = new OAuthProvider(config, undefined); + assert.equal(provider.config.jwksUri, `https://login.microsoftonline.com/${guid}/discovery/v2.0/keys`); + assert.equal(provider.config.issuer, `https://login.microsoftonline.com/${guid}/v2.0`); + }); + + it('an array pin on a tenant built here still throws once an OAuthProvider is constructed (no silent shared-pool fallback)', () => { + const manager = new TenantManager(); + const guidA = '12345678-1234-1234-1234-123456789012'; + const guidB = '87654321-4321-4321-4321-210987654321'; + + manager.registerTenant({ + tenantId: 'globex-corp', + name: 'Globex Corporation', + provider: 'azure', + azureTenantId: 'common', + clientId: 'azure-client-id', + clientSecret: 'azure-secret', + additionalConfig: { + issuer: [ + `https://login.microsoftonline.com/${guidA}/v2.0`, + `https://login.microsoftonline.com/${guidB}/v2.0`, + ], + redirectUri: 'https://app.test.com/oauth', + }, + }); + + const { providerConfig } = manager.getTenant('globex-corp'); + assert.throws(() => new OAuthProvider(providerConfig, undefined), /array/); + }); }); describe('Auth0 tenant registration', () => { diff --git a/test/options-watcher.test.js b/test/options-watcher.test.js index 79baaa8..4c118f1 100644 --- a/test/options-watcher.test.js +++ b/test/options-watcher.test.js @@ -2,6 +2,8 @@ import { describe, it, beforeEach } from 'node:test'; import assert from 'node:assert/strict'; import { handleApplication } from '../dist/index.js'; import { OAuthResource } from '../dist/lib/resource.js'; +import { _clearDiscoveryCache } from '../dist/lib/discovery.js'; +import { _setFetch, _setDnsLookup } from '../dist/lib/mcp/cimd.js'; /** * Poll `condition` instead of sleeping a fixed delay after firing a config @@ -868,4 +870,112 @@ describe('OAuth Plugin Options Watcher', () => { 'expected an error citing the missing scope field' ); }); + + describe('OIDC issuer discovery scheduling (#264)', () => { + beforeEach(() => { + _clearDiscoveryCache(); + _setDnsLookup(async () => [{ address: '93.184.216.34', family: 4 }]); + }); + + it('a reload rejected before publication (the reserved "mcp" provider name) starts no discovery', async () => { + let fetchCount = 0; + _setFetch(async () => { + fetchCount++; + throw new Error('must never fetch for a discarded reload'); + }); + + await handleApplication(scope); + + let errorLogged = false; + scope.logger.error = (msg) => { + if (typeof msg === 'string' && msg.includes('Failed to update OAuth configuration')) errorLogged = true; + }; + + scope.options._config = { + debug: false, + redirectUri: 'https://app.test.com/oauth', + providers: { + // A valid, issuer-less, https explicit-endpoint provider that would + // otherwise be eligible for background discovery... + 'custom-idp': { + provider: 'generic', + clientId: 'c', + clientSecret: 's', + authorizationUrl: 'https://idp.example.com/authorize', + tokenUrl: 'https://idp.example.com/token', + userInfoUrl: 'https://idp.example.com/userinfo', + jwksUri: 'https://idp.example.com/jwks', + }, + // ...processed in the same providers map as a reserved name that + // makes the whole build throw, before `providers` is ever published. + 'mcp': { + provider: 'generic', + clientId: 'c', + clientSecret: 's', + }, + }, + }; + configChangeListeners[0](); + await waitFor(() => errorLogged); + + assert.ok(errorLogged, 'the rejected reload is logged'); + await new Promise((resolve) => setTimeout(resolve, 20)); + assert.equal(fetchCount, 0, 'no discovery fetch was ever attempted for the discarded reload'); + }); + + it('a reload whose later log call throws still schedules discovery for the already-published registry', async () => { + let fetchCount = 0; + _setFetch(async () => { + fetchCount++; + return { + ok: false, + status: 404, + headers: new Map([['content-type', 'application/json']]), + body: { + getReader: () => ({ read: async () => ({ done: true, value: undefined }), cancel: () => {} }), + }, + }; + }); + + scope.options._config.providers = { + 'custom-idp': { + provider: 'generic', + clientId: 'c', + clientSecret: 's', + authorizationUrl: 'https://idp.example.com/authorize', + tokenUrl: 'https://idp.example.com/token', + userInfoUrl: 'https://idp.example.com/userinfo', + jwksUri: 'https://idp.example.com/jwks', + }, + }; + await handleApplication(scope); + const publishedProviders = OAuthResource.getProviders(); + assert.ok(publishedProviders['custom-idp'], 'the issuer-less provider is published on first boot'); + + let errorLogged = false; + scope.logger.error = (msg) => { + if (typeof msg === 'string' && msg.includes('Failed to update OAuth configuration')) errorLogged = true; + }; + // Throws specifically on the LAST statement `updateConfiguration` can + // reach (the debug-mode-change message) — well after the registry + // swap (`Object.assign`) that this feature schedules discovery from. + scope.logger.info = (msg) => { + if (typeof msg === 'string' && msg.startsWith('OAuth debug mode')) { + throw new Error('boom'); + } + }; + + // debug: true (changed from the initial false) is required to make + // the debug-mode-change log line fire at all. + scope.options._config = { ...scope.options._config, debug: true }; + configChangeListeners[0](); + await waitFor(() => errorLogged); + + assert.ok(errorLogged, 'the reload is still reported as failed overall'); + // The registry swap (and therefore discovery scheduling) already + // happened before the throwing log call — a later failure must not + // suppress it. + await waitFor(() => fetchCount > 0); + }); + }); }); diff --git a/test/sessionValidationMiddleware.test.js b/test/sessionValidationMiddleware.test.js index 9d4a759..8b4c6e4 100644 --- a/test/sessionValidationMiddleware.test.js +++ b/test/sessionValidationMiddleware.test.js @@ -6,7 +6,7 @@ */ import { describe, it, beforeEach } from 'node:test'; import assert from 'node:assert/strict'; -import { handleApplication } from '../dist/index.js'; +import { handleApplication, registerHooks } from '../dist/index.js'; import { createMockLogger } from './helpers/mockFn.js'; describe('OAuth session-validation middleware — persist-failure handling (#265, #266)', () => { @@ -156,4 +156,87 @@ describe('OAuth session-validation middleware — persist-failure handling (#265 assert.equal(result.status, 200); assert.equal(result.body.ran, true); }); + + describe('dynamic resolution — invalid Azure issuer pin, cooldown (#264/#271)', () => { + // A stale pin that doesn't name the configured tenant — buildProviderConfig throws AzureIssuerBindingError. + const badAzureHookConfig = { + provider: 'azure', + clientId: 'c', + clientSecret: 's', + tenantId: '12345678-1234-1234-1234-123456789012', + issuer: 'https://login.microsoftonline.com/87654321-4321-4321-4321-210987654321/v2.0', + redirectUri: 'https://app.test.com/oauth', + }; + + it('clears the session (the stale identity is never served) and does not re-run the resolve hook within the 30s cooldown', async () => { + let resolveCount = 0; + registerHooks({ + onResolveProvider: async () => { + resolveCount++; + return badAzureHookConfig; + }, + }); + + let updateCalled = 0; + // Each call gets its OWN session object — one per request, same as + // production (Harper loads a fresh session per request). Reusing one + // mutable object across calls would hide a real bug: clearOAuthSession + // wipes `.oauth` in memory, so a second call against the SAME object + // would hit the "no OAuth session data" early-return before ever + // reaching the cooldown logic, trivially (and wrongly) appearing to + // pass. + const makeRequest = () => ({ + session: { + id: 'sess-1', + update: async () => { + updateCalled++; + }, + oauth: { providerConfigId: 'bad-azure-tenant', accessToken: 'tok' }, + }, + }); + const next = () => ({ status: 200, body: { ran: true } }); + + const first = await middleware(makeRequest(), next); + assert.equal(first.status, 200, 'the session is cleared, then next() proceeds'); + assert.equal(updateCalled, 1, 'the session clear was persisted — the stale identity is not served'); + assert.equal(resolveCount, 1); + + // A second, independent request for the same provider, still within + // the cooldown, must not re-run the resolve hook (and re-throw). + const second = await middleware(makeRequest(), next); + assert.equal(second.status, 200, 'that session is cleared too, independent of the cooldown'); + assert.equal(resolveCount, 1, 'the hook must not be re-run while the failure is cooling down'); + }); + + it('re-runs the resolve hook once the 30s cooldown elapses', async () => { + let resolveCount = 0; + registerHooks({ + onResolveProvider: async () => { + resolveCount++; + return badAzureHookConfig; + }, + }); + + const makeRequest = () => ({ + session: { + id: 'sess-1', + update: async () => {}, + oauth: { providerConfigId: 'bad-azure-tenant-2', accessToken: 'tok' }, + }, + }); + const next = () => ({ status: 200, body: { ran: true } }); + + await middleware(makeRequest(), next); + assert.equal(resolveCount, 1); + + const realNow = Date.now; + Date.now = () => realNow() + 31_000; + try { + await middleware(makeRequest(), next); + assert.equal(resolveCount, 2, 'the hook runs again once the 30s cooldown elapses'); + } finally { + Date.now = realNow; + } + }); + }); });