feat: per-issuer session cache — stop re-running the full auth flow (popup included) on every 401

[jeswr]

Problem

DPoPTokenProvider.upgrade() runs the entire flow on every 401: discovery, dynamic client registration, a fresh DPoP key, and a new authorization popup. In a real app (observed live with a Solid pod browser against a Solid-OIDC broker) that means:

• every authenticated request that races another one opens its own popup;
• nothing is remembered between requests, so users keep being prompted;
• the token (and its refresh_token, if any) is dropped on the floor after a single request.

Change

DPoPTokenProvider now keeps a single-flight, per-issuer session cache:

• concurrent 401 upgrades share one authorization-code flow → one popup;
• later upgrades reuse the established access token and only sign a fresh per-request DPoP proof;
• expires_in is tracked (with 30 s skew); an expired session re-runs the flow, which stays silent while the IdP cookie lives thanks to the existing prompt=none-first behaviour;
• a failed flow is not cached, so the next request retries;
• the shared flow work is provider-owned rather than tied to one request's AbortSignal — aborting one request no longer cancels the login other concurrent upgrades are waiting on (small behaviour change, called out deliberately).

Public API is unchanged.

Tests

The repo had no test runner, so this adds a minimal vitest setup (npm test) with a compact in-memory authorization server (discovery + JWKS + registration + token endpoint, ES256-signed ID tokens) — 6 tests covering token attachment, single-flight, reuse, per-request proofs, expiry re-auth, and failed-flow retry.

• npm run build (tsc): clean
• npm test: 6/6 passing
• Verified live: Playwright-driven login against a deployed pod manager + Solid-OIDC broker (https://app.solid-test.jeswr.org) — one popup per session, repeat reads reuse the token.

Stacked work: refresh-token support builds on this cache in a follow-up PR.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR introduces a per-issuer, single-flight session cache inside DPoPTokenProvider to avoid re-running the full authorization-code flow (including popups) on every 401, and adds a new Vitest-based test harness to validate the new behavior.

Changes:

• Cache authentication sessions per issuer so concurrent upgrades share one auth flow and later upgrades reuse the access token until expiry.
• Track token expiry (expires_in with skew) to trigger re-auth when needed, while still generating a fresh DPoP proof per request.
• Add a minimal in-memory OIDC/OAuth AS plus Vitest tests covering single-flight, reuse, expiry re-auth, and failed-flow retry.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
src/DPoPTokenProvider.ts	Adds per-issuer single-flight session caching and expiry-based renewal for DPoP upgrades.
test/fakeAuthorizationServer.ts	Introduces an in-memory discovery/JWKS/registration/token endpoint to support deterministic unit tests.
test/DPoPTokenProvider.test.ts	Adds Vitest coverage for caching/reuse/expiry and failure retry behavior.
package.json	Adds vitest and a npm test script to run the new unit tests.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[jeswr]

Is there a library by @panva which does this out of the box rather than needing to entirely re-define our own authorisation server here?

[jeswr]

I spiked this rather than guessing. Short answer: yes for the signing, no for the whole server.

Signing — adopted. The hand-rolled base64url + subtle.sign JWT construction is gone, replaced with panva jose (46e4362). Zero-dependency, and it removes the only real crypto we were writing ourselves.

The whole server — oidc-provider is the candidate, and it does work. I stood it up and drove a full code grant against it. Discovery, dynamic registration, PKCE and DPoP are all supported out of the box, and it is the certified reference implementation. But the costs are concrete:

1. You do not escape the fetch stub. It is a Node HTTP listener, so it serves plain http, and oauth4webapi refuses non-https issuers. The test still has to stub globalThis.fetch to rewrite https://as.test → http://127.0.0.1:PORT — or take the insecure opt-in from fix: allow opting in to insecure OAuth requests #18.
2. The authorization endpoint is interactive. It 303s to /interaction/:uid. Getting a code took 3 redirect hops, a cookie jar, and regex-scraping the HTML login and consent forms out of devInteractions — a feature it prints a startup warning telling you to replace. My working spike was ~90 lines against the fake’s ~190, and the 90 are the fragile kind.
3. Four startup warnings on a default config: dev in-memory adapter, dev signing keys, devInteractions, and unsupported runtime.
4. ~680 KB and ten transitive packages (koa, @koa/router, @koa/cors, eta, raw-body, quick-lru, nanoid, jsesc, debug, jose).

The deciding factor is that the fake is a test double, not a server. The suite needs to force expires_in, toggle refresh-token rotation, and count how many times the user was prompted — that is what the tests actually assert on. With oidc-provider those become configuration archaeology; here they are constructor options.

So I have kept the fake, cut its header comment down, and taken jose for the part that was genuinely reinventing a library. oidc-provider is the right tool for a conformance or integration suite against the real client, and I would happily use it there — just not as the unit-test double.

[jeswr]

Rather than caching being in-memory only; we should have configurable CacheProviders to enable context specific caching including:

• Having session in memory (as is the case here)
• Saving session, or refresh tokens in browser storage
• In vscode, saving things to their secrets API

[jeswr]

Done in 46e4362 — SessionCache<T> is now the storage seam:

export interface SessionCache<T> {
    get(key: string): Promise<T | undefined>
    set(key: string, value: T): Promise<void>
    delete(key: string): Promise<void>
}

Async throughout, so IndexedDB and the VS Code secrets API are both implementable. MemorySessionCache is the default, and DPoPTokenProvider takes an override via the new options argument.

Two things worth flagging:

Single-flight had to move out of the cache. It worked before only because the map stored Promise<IssuerSession>, and a pending promise cannot be persisted anywhere. In-flight flows now live in a separate in-memory #pending map and only resolved sessions reach the cache. One popup per key is unchanged, and a failed flow is still not cached.

Not every store can hold a whole session. DPoPSession (renamed from IssuerSession, now exported so caches can be typed) carries a non-extractable CryptoKeyPair. That is structured cloneable, so IndexedDB is fine — but it is not JSON serialisable, so localStorage and the VS Code secrets API cannot take it as-is. Those are exactly the string-only stores where you would want to persist a refresh token instead, which is #14. So the seam is in place here and the string-store case lands naturally on top of it.

[jeswr]

Follow-up in e34467f — two concrete implementations, and the localStorage question turned out to have a sharp edge worth writing down.

IndexedDbSessionCache is the one you want for DPoP. IndexedDB stores by structured clone, which preserves a non-extractable CryptoKey. Verified rather than assumed:

structuredClone -> privateKey is CryptoKey: true | extractable: false | usages: ["sign"]
clone can still sign: 64 bytes

That is the property that makes persistence safe here. The key survives a browser restart, but stays unreadable by script on the origin, so DPoP still buys what it is supposed to buy: a stolen access token is useless without a key that cannot be exfiltrated. Covered by a test that runs a real flow, builds a second provider over the same database to stand in for a reload, and asserts no prompt and a fresh proof.

localStorage cannot hold a DPoP session, and fails silently if you try. This is the sharp edge:

JSON.stringify -> {"dpopKey":{"publicKey":{},"privateKey":{}}}

No error. You get the session back with dpopKey: {publicKey: {}, privateKey: {}} and then a baffling failure much later inside DPoP.generateProof. So WebStorageSessionCache.set() throws on a CryptoKey instead, naming IndexedDbSessionCache. Storing the key there would also mean generating it extractable: true, which hands any XSS on the origin a portable, permanently reusable credential — it defeats the point of DPoP rather than just weakening it.

Where web storage does earn its place is sessions that genuinely are just JSON — a bare refresh token being the obvious one, which is #14. So WebStorageSessionCache takes the Storage explicitly rather than defaulting, because localStorage (survives a restart) vs sessionStorage (dies with the tab) is a real security choice and should read as one at the call site. Keys are namespaced, and an unparseable entry is discarded rather than thrown.

Suite is 24 tests, tsc clean. One incidental benefit: IndexedDB is shared between a page and its service worker, so this is also a plausible route out of the realm-isolation problem noted on #18 — not wired up here.

[jeswr]

We need to think about whether we always want to be caching by issuer - I am not sure that this will always be the case.

[jeswr]

Agreed — made it a caller decision in 46e4362 rather than baking the assumption in.

GetSessionKeyCallback derives the cache key and defaults to the issuer, preserving current behaviour:

export type GetSessionKeyCallback = (request: Request, issuer: URL) => Promise<string>

It gets the Request as well as the issuer, so the cases where per-issuer is wrong are all expressible — per-pod/origin, per-account when one person has two WebIDs at the same IdP, or per-storage where an issuer fronts several. Covered by a test that scopes to new URL(request.url).origin and gets two prompts for two origins where the default gets one.

I have deliberately not tried to guess a better default. Per-issuer is right for the common case, and anything cleverer would be the same kind of heuristic we backed out of in #18.