fix: base64UrlEncode not working properly with specific characters #192
Conversation
…ith state having specific characters
WalkthroughChanges update base64 encoding to produce standard base64 with padding, expose Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related issues
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
pesickaa
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/utils/generateAuthUrl.ts (1)
158-177: Align crypto detection in generatePKCEPair with test robustness checksThe production code at line 165 uses a bare
if (!crypto)check, but the test at line 497–502 demonstrates a much more thorough capability check:typeof crypto !== "undefined" && crypto && crypto.subtle && typeof crypto.subtle.digest === "function".If
cryptoexists butcrypto.subtleis undefined ordigestis not a function, line 168's call tocrypto.subtle.digest("SHA-256", data)will throw a runtime error instead of falling back to the non-crypto path. The test suite even includes a try-catch aroundcrypto.subtle.digest()(lines 509–514) to verify the capability actually works.To make the production code match the availability checks demonstrated in the tests, consider using a safety check before calling
crypto.subtle.digest:export async function generatePKCEPair(): Promise<{ codeVerifier: string; codeChallenge: string; }> { const codeVerifier = generateRandomString(52); const data = new TextEncoder().encode(codeVerifier); let codeChallenge = ""; - if (!crypto) { + const hasWebCrypto = + typeof crypto !== "undefined" && + crypto && + crypto.subtle && + typeof crypto.subtle.digest === "function"; + + if (!hasWebCrypto) { codeChallenge = base64UrlEncode(codeVerifier); } else { const hashed = await crypto.subtle.digest("SHA-256", data); codeChallenge = base64UrlEncode(hashed); }This brings the production code in line with the robustness your tests expect.
🧹 Nitpick comments (3)
lib/utils/base64UrlEncode.ts (1)
1-23: Behavior change to standard base64 is consistent but naming is now misleadingThe implementation now cleanly returns standard padded base64 for both
stringandArrayBufferinputs and does so in a robust way (improved ArrayBuffer detection, explicit TextEncoder path). This matches the updated tests and the new PKCE flow, where URL‑safety and padding stripping are handled by the caller.The only minor concern is that the function name
base64UrlEncodeno longer reflects its behavior (it no longer produces URL‑safe base64). That’s fine for this PR but worth calling out in release notes and/or external docs so consumers aren’t surprised by the change in semantics.lib/utils/generateAuthUrl-nocrypto.test.ts (1)
1-12: No‑crypto PKCE tests accurately capture the intended fallback behaviorThe new
generatePKCEPair - no cryptosuite does a good job locking in the non‑crypto behavior:
- Verifies verifier length and URL‑safe challenge (no
+,/, or trailing=).- Confirms the challenge matches
base64UrlEncode(codeVerifier)with the same normalization you use in production.- Checks shape, character set, and randomness across calls.
The global
cryptostubbing (bothvi.stubGlobal("crypto", undefined)and theObject.defineProperty) is slightly redundant but functionally fine given this is a dedicated “no crypto” test file.Also applies to: 52-105
lib/utils/generateAuthUrl.test.ts (1)
3-7: PKCE tests are thorough and correctly express expected behaviorThe new
generatePKCEPairtests are comprehensive:
- They validate verifier length, URL‑safe challenge constraints, randomness, and result shape.
- The “crypto availability” test sensibly distinguishes between “Web Crypto present and working” vs. fallback, using a robust capability check and cross‑checking against a direct
base64UrlEncode(codeVerifier)path.These tests nicely document the intended contract for
generatePKCEPairand will help guard future changes. Note that the implementation’s currentif (!crypto)check is less strict than thehasWebCryptologic you use here; aligning the implementation with this capability check (as suggested in the implementation comment) would make behavior and tests fully consistent.Also applies to: 459-554
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/tests/expoLazyLoading.test.ts(2 hunks)lib/utils/base64UrlEncode.test.ts(3 hunks)lib/utils/base64UrlEncode.ts(1 hunks)lib/utils/generateAuthUrl-nocrypto.test.ts(2 hunks)lib/utils/generateAuthUrl.test.ts(2 hunks)lib/utils/generateAuthUrl.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: DanielRivers
Repo: kinde-oss/js-utils PR: 128
File: lib/utils/generateProfileUrl.ts:30-35
Timestamp: 2025-05-28T10:56:50.579Z
Learning: In the Kinde js-utils codebase, if the storage system returns non-string values for access tokens, it indicates wider systemic issues. The team prefers to keep simple type casting in generateProfileUrl rather than adding defensive type checking, as the root cause would need to be addressed elsewhere.
Learnt from: DanielRivers
Repo: kinde-oss/js-utils PR: 15
File: lib/utils/generateAuthUrl.test.ts:104-104
Timestamp: 2024-10-25T23:53:26.124Z
Learning: In `lib/utils/generateAuthUrl.test.ts`, the code is test code and may not require strict adherence to PKCE specifications.
📚 Learning: 2024-10-25T23:53:26.124Z
Learnt from: DanielRivers
Repo: kinde-oss/js-utils PR: 15
File: lib/utils/generateAuthUrl.test.ts:104-104
Timestamp: 2024-10-25T23:53:26.124Z
Learning: In `lib/utils/generateAuthUrl.test.ts`, the code is test code and may not require strict adherence to PKCE specifications.
Applied to files:
lib/utils/base64UrlEncode.test.tslib/tests/expoLazyLoading.test.tslib/utils/generateAuthUrl.tslib/utils/generateAuthUrl.test.tslib/utils/generateAuthUrl-nocrypto.test.ts
📚 Learning: 2024-11-19T20:31:59.197Z
Learnt from: DanielRivers
Repo: kinde-oss/js-utils PR: 11
File: lib/utils/token/index.test.ts:59-62
Timestamp: 2024-11-19T20:31:59.197Z
Learning: In `lib/utils/token/index.test.ts`, the test "getInsecureStorage returns active storage when no insecure storage is set" is intentional. It verifies that when insecure storage is not set, `getInsecureStorage()` returns the active storage as a fallback, preferring secure storage but allowing for insecure storage when needed by consumers of the library.
Applied to files:
lib/tests/expoLazyLoading.test.tslib/utils/generateAuthUrl-nocrypto.test.ts
📚 Learning: 2024-10-08T23:57:58.113Z
Learnt from: DanielRivers
Repo: kinde-oss/js-utils PR: 12
File: lib/main.ts:0-0
Timestamp: 2024-10-08T23:57:58.113Z
Learning: When exporting modules in `lib/main.ts`, ensure that paths to modules like `./utils/token` are explicitly set to include `index.ts` (e.g., `./utils/token/index.ts`) to prevent missing export issues.
Applied to files:
lib/tests/expoLazyLoading.test.tslib/utils/generateAuthUrl.test.ts
📚 Learning: 2025-01-16T21:47:40.307Z
Learnt from: DanielRivers
Repo: kinde-oss/js-utils PR: 61
File: lib/sessionManager/types.ts:20-20
Timestamp: 2025-01-16T21:47:40.307Z
Learning: Security documentation for configuration options in this codebase should be placed in the implementation file (e.g., index.ts) rather than the types file, as demonstrated with `useInsecureForRefreshToken` in `lib/sessionManager/index.ts`.
Applied to files:
lib/utils/generateAuthUrl.tslib/utils/generateAuthUrl-nocrypto.test.ts
📚 Learning: 2024-10-25T23:50:56.599Z
Learnt from: DanielRivers
Repo: kinde-oss/js-utils PR: 15
File: lib/utils/generateAuthUrl.ts:27-30
Timestamp: 2024-10-25T23:50:56.599Z
Learning: In `lib/utils/generateAuthUrl.ts`, `StorageKeys` is imported from `../main` and includes the `state` key.
Applied to files:
lib/utils/generateAuthUrl.tslib/utils/generateAuthUrl.test.tslib/utils/generateAuthUrl-nocrypto.test.ts
📚 Learning: 2024-10-08T23:57:58.113Z
Learnt from: DanielRivers
Repo: kinde-oss/js-utils PR: 8
File: lib/utils/token/testUtils/index.ts:3-39
Timestamp: 2024-10-08T23:57:58.113Z
Learning: The file `lib/utils/token/testUtils/index.ts` is a test utility, not production code.
Applied to files:
lib/utils/generateAuthUrl.test.tslib/utils/generateAuthUrl-nocrypto.test.ts
📚 Learning: 2025-05-28T10:56:50.579Z
Learnt from: DanielRivers
Repo: kinde-oss/js-utils PR: 128
File: lib/utils/generateProfileUrl.ts:30-35
Timestamp: 2025-05-28T10:56:50.579Z
Learning: In the Kinde js-utils codebase, if the storage system returns non-string values for access tokens, it indicates wider systemic issues. The team prefers to keep simple type casting in generateProfileUrl rather than adding defensive type checking, as the root cause would need to be addressed elsewhere.
Applied to files:
lib/utils/generateAuthUrl.test.tslib/utils/generateAuthUrl-nocrypto.test.ts
📚 Learning: 2024-11-10T23:29:36.293Z
Learnt from: DanielRivers
Repo: kinde-oss/js-utils PR: 11
File: lib/utils/exchangeAuthCode.ts:51-51
Timestamp: 2024-11-10T23:29:36.293Z
Learning: In `lib/utils/exchangeAuthCode.ts`, the `exchangeAuthCode` function intentionally uses `getInsecureStorage()` to store temporary authentication flow values locally between sessions. This is necessary for storing data needed for the return trip. The `getInsecureStorage()` function returns the secure storage if no active insecure storage is defined.
Applied to files:
lib/utils/generateAuthUrl-nocrypto.test.ts
🧬 Code graph analysis (3)
lib/utils/generateAuthUrl.ts (3)
lib/utils/base64UrlEncode.ts (1)
base64UrlEncode(6-24)lib/main.ts (1)
base64UrlEncode(6-6)lib/utils/index.ts (1)
base64UrlEncode(1-1)
lib/utils/generateAuthUrl.test.ts (2)
lib/utils/generateAuthUrl.ts (1)
generatePKCEPair(158-178)lib/utils/base64UrlEncode.ts (1)
base64UrlEncode(6-24)
lib/utils/generateAuthUrl-nocrypto.test.ts (2)
lib/utils/generateAuthUrl.ts (1)
generatePKCEPair(158-178)lib/utils/base64UrlEncode.ts (1)
base64UrlEncode(6-24)
🔇 Additional comments (2)
lib/tests/expoLazyLoading.test.ts (1)
190-211: Updated expectations correctly match new padded base64 behaviorThe new expectations for
base64UrlEncode("test")→"dGVzdA=="align with the updated implementation and still validate the tree‑shaking behavior as intended.lib/utils/base64UrlEncode.test.ts (1)
5-8: Tests correctly reflect standard padded base64 outputThe updated expectations for padded base64 (including
=) are correct for the provided inputs and match the newbase64UrlEncodesemantics for both strings andArrayBuffers.Also applies to: 26-29, 33-36, 40-50
update base64UrlEncode and generatePKCEPair functions to work with state having specific characters like =.
The base64UrlEncode method was clearly updated to handle specific use cases which were based off the generatePKCEPair function, where we strip out specific characters for the PKCE Pair. This was fine until we realized that when a user provides state
like in the React auth issue 203, it causes the call on lines 379 of the react KindeProvider.tsx to fail.
VERIFY ISSUE SETUP:
playground/LoggedOut.tsxto have somethinglike the following:{"user_type":"client","redirect_uri":"http://localhost:7001/?user_type=client","utm":"{}"}FIX SETUP
playground/LoggedOut.tsxto have somethinglike the following:{"user_type":"client","redirect_uri":"http://localhost:7001/?user_type=client","utm":"{}"}CHANGES:
Explain your changes
Suppose there is a related issue with enough detail for a reviewer to understand your changes fully. In that case, you can omit an explanation and instead include either “Fixes #XX” or “Updates #XX” where “XX” is the issue number.
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.