feat: additional end to end tests - #208
Open
mrudatsprint wants to merge 110 commits into
Open
Conversation
- Add `dpop` v2.1.1 runtime dependency and `fake-indexeddb` devDependency to packages/core/package.json - SDKConfig: add optional `useDpop` and `dpopTokenStorage` fields - UrlHelper: add `getAuthorizeUrl(state?, dpopJkt?, codeChallenge?)` targeting FusionAuth /oauth2/authorize directly; update UrlHelperTypes to include response_type, code_challenge, code_challenge_method, dpop_jkt - DPoPStorage: IndexedDB abstraction for ES256 CryptoKeyPair persistence (db: fusionauth-sdk:dpop, store: keypair, keyed by clientId) - DPoPTokenStore: localStorage/memory token storage for DPoP-bound tokens (key: fusionauth-sdk:tokens:<clientId>); includes getAccessToken() and isExpired getter - packages/core/src/DPoP/index.ts re-exports both classes - 54 tests passing (21 DPoPTokenStore, 6 DPoPStorage, 16 UrlHelper, 7 SDKCore, 4 CookieHelpers)
- Remove 'as any' cast in catch block — reject() accepts unknown directly - Add tests for indexedDB unavailable (SSR/non-browser): all three public methods (getKeyPair, setKeyPair, clearKeyPair) reject with a descriptive error - Add test for indexedDB.open() throwing synchronously (e.g. security policy block)
All three DPoPStorage methods (getKeyPair, setKeyPair, clearKeyPair) now resolve on tx.oncomplete and reject on tx.onerror / tx.onabort. Previously, resolving on req.onsuccess meant the caller was told 'success' before the transaction had fully committed — a transaction abort occurring after the request succeeded (e.g. quota exceeded) would go undetected. Applies the same fix consistently to all three methods, including getKeyPair (readonly, lower risk, but now consistent) and setKeyPair (readwrite, same durability concern as clearKeyPair). Adds a test that aborts a clearKeyPair transaction synchronously inside the request onsuccess handler and verifies the promise rejects and the key pair is still present in IndexedDB.
…84/central-coordinator' of github.com:FusionAuth/fusionauth-javascript-sdk into miker/eng-4784/central-coordinator
…g object - Export DEFAULT_DPOP_DB_NAME, DEFAULT_DPOP_DB_VERSION, DEFAULT_DPOP_STORE_NAME as named constants (no hardcoded magic strings anywhere in the codebase) - Add DPoPStorageConfig interface with clientId (required) and optional dbName, dbVersion, storeName fields — each defaults to the exported constant - Refactor DPoPStorage constructor from positional (clientId: string) to config object, matching the UrlHelperConfig convention in this monorepo - openDb() and all three public methods now reference instance fields (this.dbName, this.dbVersion, this.storeName) instead of module constants - Add tests: defaults apply when no config overrides provided; custom dbName and storeName land data in the right database; two instances with different dbNames but the same clientId do not share keys; dbVersion downgrade produces a clean rejection (VersionError) - Update AGENTS.md: note the config-object constructor convention and the IndexedDB dbVersion must-only-increase constraint
…central-coordinator
…ENG-4782 refactor
…t (ENG-4786) - Add Pkce module (generateCodeVerifier, generateCodeChallenge) with RFC 7636 Appendix B test vector coverage; runs under @vitest-environment node - Extend RedirectHelper to persist code_verifier as a second colon-delimited segment alongside state; add public getCodeVerifier() getter; add test file - SDKCore: construct DPoPManager when config.useDpop is true; startLogin() is now async — DPoP branch calls getOrCreateKeyPair()/getThumbprint() and generates PKCE params then redirects to /oauth2/authorize directly; isLoggedIn delegates to DPoPManager.isLoggedIn in DPoP mode (not app.at_exp cookie) - SDKCore.test.ts: add DPoP-mode describe block with mocked DPoPManager and Pkce (jsdom lacks crypto.subtle); all existing cookie-mode tests unaffected - e2e/dpop-smoke.test.ts: replace local generatePkce() helper with shared Pkce module; add Tier 0 tests exercising SDKCore.startLogin() in DPoP mode end-to-end (no live FusionAuth required for Tier 0) - Export Pkce from packages/core/src/index.ts Note: yarn test:core cannot run in this sandbox environment due to a missing @rollup/rollup-linux-arm64-gnu native binary (arch mismatch); TypeScript compilation (tsc --noEmit) and ESLint/Prettier are clean.
…edirect assertion Without the explicit jsdom annotation, vitest inherits the 'node' environment from DPoPManager.test.ts when the full suite runs, causing 'document is not defined' and 'window is not defined' failures in all SDKCore tests. Also corrects the handlePreRedirect spy assertion: cookie-mode startLogin() passes one argument (state), not two — the codeVerifier arg is only added in DPoP mode.
SDKCore's constructor calls scheduleTokenExpiration() which calls
getAccessTokenExpirationMoment(). In a Node/Playwright process document
doesn't exist, so CookieHelpers catches the ReferenceError and logs
'Error accessing cookies...' to console.error. The tests still pass, but the
stderr noise is confusing.
Fix: extract a shared DPOP_CONFIG constant in the Tier 0 describe block that
includes a no-op cookieAdapter ({ at_exp: () => undefined }). This causes
getAccessTokenExpirationMoment() to take the adapter path and skip
document.cookie entirely, eliminating the noise.
Also fixes T0-1 where the await core.startLogin() call was accidentally
dropped during the previous config refactor.
…ormat RedirectHelper now stores nonce:codeVerifier:state (three colon-delimited segments) instead of the previous nonce:state (two segments). The Angular sdkcore/ directory is generated by 'yarn get-sdk-core' which copies packages/core/src/ verbatim — so in CI the Angular RedirectHelper picks up the updated parser automatically. The test was writing the old two-segment format 'abc123:/welcome-page', which the new parser splits as [nonce='abc123', codeVerifier='/welcome-page', state=''] — returning undefined for state instead of '/welcome-page'. Fix: write 'abc123::/welcome-page' (empty codeVerifier segment, matching cookie mode where no verifier is stored).
…value format
RedirectHelper now stores nonce:codeVerifier:state (three colon-delimited
segments) instead of nonce:state (two segments). Both sdk-vue and sdk-react
import SDKCore directly from @fusionauth-sdk/core (via the @fusionauth-sdk/*
tsconfig path alias), so their tests exercise the live, current
RedirectHelper — same root cause as the earlier Angular fix.
- packages/sdk-vue/src/createFusionAuth/createFusionAuth.test.ts: was seeding
the old 2-segment format ('rAnd0mStR1ng:<state>'), causing the new state
getter to return undefined instead of the expected state value. Fixed to
'rAnd0mStR1ng::<state>' (empty codeVerifier segment).
- packages/sdk-react/src/components/providers/FusionAuthProvider.test.tsx:
had the same stale 2-segment seed, but wasn't caught by CI because the
assertion only checked toHaveBeenCalled() (no argument check). Fixed the
seed format and strengthened the assertion to toHaveBeenCalledWith(stateValue)
to restore real coverage of the callback argument.
… dropping one (PR #202 review) _resolveHeaders() previously returned early with init.headers whenever it was present, silently discarding any headers already set on a Request object passed as input. This contradicted the _doFetch documentation's promise to never drop caller headers. _resolveHeaders() now returns a merged Headers object: init.headers is the base, and Request.headers are layered on top, winning on any conflicting header name.
…mption (PR #202 Copilot review) fetch() previously passed the same input reference to both the initial attempt and the nonce-triggered retry. If input was a Request with a body, the first attempt consumed it, and the retry would throw a 'body already used' error instead of succeeding. - Clone the Request twice up front (before either is read from) so each attempt gets an independent, unconsumed body. Request.clone() safely tees any internal streaming body per spec, so this also covers a Request built with a ReadableStream body. - A raw ReadableStream passed via init.body (not wrapped in a Request) cannot be cloned this way. On retry, this now throws a clear, actionable error instead of letting native fetch throw an opaque one.
…opilot review) generateProof() documented htu as being 'without query/fragment' but passed it through unmodified. fetch() supplies Request.url, which can include a query string, so proofs generated via dpopFetch() could carry an htu that includes query parameters — a subtle interop bug with strict DPoP verifiers. htm was also not normalised to uppercase, which most DPoP verifiers require. Both are now normalised inside generateProof() itself, so this is correct regardless of whether callers go through fetch() or call generateProof() directly with arbitrary casing/query strings.
…dpop-smoke.test.ts (PR #202 Copilot review) T2-1 declared capturedAuthHeader/capturedDpopHeader that were never assigned and only suppressed via void, alongside a comment claiming Playwright route interception captures DPoPManager.fetch()'s headers — no such interception exists since fetch() runs in the Node test process, not the browser page. Removed the dead variables and replaced the comment with an accurate explanation of how correctness is actually validated (end-to-end via FusionAuth's server-side verification, plus T2-2's direct proof decoding).
'refresh token grant — issues new DPoP-bound tokens' had two compounding bugs after being resurrected via a merge: 1. test.skip(!refreshToken, ...) referenced a variable that was never declared in this file (ReferenceError). Every other test in the file uses the !accessToken skip-guard convention -- switch to that. 2. The test requires core to still be logged in (asserts core.isLoggedIn and calls core.refreshToken()), but it ran *after* 'startLogout() clears DPoP state...', which already logs core out. Move it back to run right after the authorization code grant test and before startLogout(), matching its actual dependency.
…-4790) Extends dpop-endpoints.test.ts with three new tests covering the remaining ENG-4790 scenarios not yet exercised end-to-end: - dpopFetch() sets Authorization: DPoP and DPoP proof headers, with the proof's ath claim verified against base64url(SHA-256(accessToken)) - dpopFetch() retries exactly once with the server-provided nonce after a 401 use_dpop_nonce challenge - dpopFetch() does not retry a second time when the retry also returns 401 use_dpop_nonce These tests inject a second, independent SDKCore instance built directly from packages/core/dist/index.js (see injectDpopSdkCore()). No changes to the consuming quickstart application are required: DPoP key pairs (IndexedDB, keyed by clientId) and tokens (localStorage, keyed by fusionauth-sdk:tokens:<clientId>) are namespaced by clientId/origin rather than by JS object identity, so the injected instance transparently reuses state the quickstart's own React app already created via a normal login.
…n 401s The nonce-retry tests mock https://api.example.com, which is a different origin from the quickstart app. For cross-origin cors-mode fetches, the browser filters Response.headers down to the CORS-safelisted set unless Access-Control-Expose-Headers is present. DPoPManager.fetch() reads WWW-Authenticate and DPoP-Nonce off the response to detect a nonce challenge, so without exposing them the SDK silently saw no challenge and returned the first 401 without retrying (requestCount stayed at 1 instead of 2). Add Access-Control-Allow-Origin and Access-Control-Expose-Headers to the mocked 401 responses in both nonce-retry tests, and document the gotcha in the file header for future edits to these mocked routes.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds additional DPoP-focused end-to-end coverage (Playwright) and refines existing DPoP-related test wording to better reflect the flow being tested.
Changes:
- Add new Playwright e2e tests validating
dpopFetch()headers/athclaim and nonce-challenge retry behavior. - Update Vue SDK unit test descriptions/comments from “settles” to “completes” for clearer intent.
- Remove a now-unneeded JSDoc block for a private DPoP refresh method in
SDKCore.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/sdk-vue/src/createFusionAuth/createFusionAuth.test.ts | Wording tweaks in DPoP redirect-related unit tests for clarity. |
| packages/core/src/SDKCore/SDKCore.ts | Removes a doc comment on a private DPoP refresh helper. |
| e2e/tests/dpop-endpoints.test.ts | Adds new Playwright e2e coverage around dpopFetch() header/proof behavior and nonce retry logic. |
Suppressed comments (2)
e2e/tests/dpop-endpoints.test.ts:396
- Same as the other dpopFetch() route handlers: without handling OPTIONS preflight separately, the browser may never make it to the intended request, and requestCount will not reflect the actual GET attempts. The fulfilled responses should also include Access-Control-Allow-Origin so the browser doesn't block the fetch.
await page.route('https://api.example.com/always-nonce', route => {
e2e/tests/dpop-endpoints.test.ts:352
- This route handler likely intercepts the CORS preflight (OPTIONS) request as well as the actual GET, which will inflate requestCount and can cause the browser to block the response if the 200 retry doesn't include Access-Control-Allow-Origin. Handle OPTIONS separately and only increment requestCount for the real request method.
await page.route('https://api.example.com/nonce-protected', route => {
requestCount += 1;
if (requestCount === 1) {
route.fulfill({
status: 401,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix typos in injectDpopSdkCore() doc comment (dropFetch -> dpopFetch, getAcessToken -> getAccessToken). - Handle CORS preflight OPTIONS requests separately in all three mocked https://api.example.com routes via a new handleCorsPreflight() helper. dpopFetch() sends non-safelisted Authorization/DPoP headers, so cross-origin requests trigger a browser preflight OPTIONS request first. Since page.route() matches by URL regardless of method, the preflight was previously falling through to the same handler as the real request, which could inflate request counters and, when answered with a non-2xx status (e.g. the mocked 401), fail the preflight and block the real request from ever being sent.
Addresses the third (unaddressed) Copilot comment on PR #208: these tests require packages/core/dist/index.js to exist (injectDpopSdkCore imports it directly), but test:e2e:dpop-endpoints didn't build @fusionauth-sdk/core first, so a clean checkout would fail. Chain yarn build:core into test:e2e:dpop-endpoints, matching the existing build:sdk-react/build:sdk-vue convention of building core before consuming it. Update the file header's run instructions and note the manual yarn build:core step needed when running via npx playwright test directly.
mrudatsprint
commented
Aug 3, 2026
| } | ||
|
|
||
| /** | ||
| * Injects a second, independent `SDKCore` instance into the page. |
Collaborator
Author
There was a problem hiding this comment.
I added three dropFetch() tests. I believe they provide coverage for when a Resource Server API is not directly available.
| let requestCount = 0; | ||
| let retryDpopHeader: string | undefined; | ||
|
|
||
| await page.route('https://api.example.com/nonce-protected', route => { |
Collaborator
Author
There was a problem hiding this comment.
Note, the re-try logic is in dropFetch()
mrudatsprint
commented
Aug 3, 2026
mrudatsprint
left a comment
mrudatsprint
left a comment
Collaborator
Author
There was a problem hiding this comment.
self-review completed
mrudatsprint
marked this pull request as ready for review
August 3, 2026 15:16
Base automatically changed from
mike/eng-4787/dpop-sdks
to
parent/dpop-in-the-javascript-sdk
August 3, 2026 22:01
mrudatsprint
commented
Aug 3, 2026
mrudatsprint
left a comment
mrudatsprint
left a comment
Collaborator
Author
There was a problem hiding this comment.
self-review completed
mrudatsprint
changed the title
mrudatsprint
requested review from
lyleschemmerling
and removed request for
wied03
August 3, 2026 23:04
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issues:
Description:
Add the testing of dropFetch() which is responsible for making resource server API calls and implementing re-try logic. These tests are not using a live resource server. They are using a second SDKCore instance to provide the functionality.