feat: add getCookieOptions utility function#184
feat: add getCookieOptions utility function#184dtoxvanilla1991 wants to merge 9 commits intokinde-oss:mainfrom
Conversation
WalkthroughAdds a new cookie-options utility with defaults and a merging function, tests for its behavior, and re-exports the utility and its type from utils and main; updates expected public exports accordingly. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧠 Learnings (5)📓 Common learnings📚 Learning: 2024-09-23T22:08:18.788ZApplied to files:
📚 Learning: 2025-12-18T23:04:04.194ZApplied to files:
📚 Learning: 2025-01-16T21:47:40.307ZApplied to files:
📚 Learning: 2024-11-10T23:29:36.293ZApplied to files:
🔇 Additional comments (1)
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! |
dtoxvanilla1991
force-pushed
the
feat/getCookiesOptions
branch
from
5d69edc to
c77f83d
Compare
dtoxvanilla1991
changed the title
dtoxvanilla1991
force-pushed
the
feat/getCookiesOptions
branch
from
c77f83d to
9524dd0
Compare
…xchangeAuthCode tests
![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
🧹 Nitpick comments (3)
lib/utils/getCookieOptions.test.ts (1)
50-73: Consider usingvi.stubEnvfor cleaner environment variable mocking.The manual save/restore pattern works but is verbose. Vitest's
vi.stubEnvprovides cleaner isolation:it("falls back to runtime environment variables when env param is omitted", () => { - const previousNodeEnv = process.env.NODE_ENV; - const previousCookieDomain = process.env.KINDE_COOKIE_DOMAIN; - - process.env.NODE_ENV = "production"; - process.env.KINDE_COOKIE_DOMAIN = "runtime-domain.io/"; + vi.stubEnv("NODE_ENV", "production"); + vi.stubEnv("KINDE_COOKIE_DOMAIN", "runtime-domain.io/"); const result = getCookieOptions(); expect(result.domain).toBe("runtime-domain.io"); expect(result.secure).toBe(true); - if (previousNodeEnv === undefined) { - delete process.env.NODE_ENV; - } else { - process.env.NODE_ENV = previousNodeEnv; - } - - if (previousCookieDomain === undefined) { - delete process.env.KINDE_COOKIE_DOMAIN; - } else { - process.env.KINDE_COOKIE_DOMAIN = previousCookieDomain; - } + vi.unstubAllEnvs(); });lib/utils/getCookieOptions.ts (2)
7-7: Consider removingnullfrom CookieOptionValue.Including both
undefinedandnullis unusual in TypeScript. Typically, optional values use onlyundefined. Having both can lead to confusion and requires callers to handle both cases.Apply this diff if
nullis not specifically required:-export type CookieOptionValue = string | number | boolean | undefined | null; +export type CookieOptionValue = string | number | boolean | undefined;
29-38: Clarify the environment detection check.The check
typeof globalThis === "undefined"is confusing becauseglobalThisis a standard feature in modern JavaScript and should always be available. The comment mentionsprocessbeing undefined, but the check is forglobalThis.While the code works correctly (falling back to
{}in browsers whereprocessdoesn't exist), the logic would be clearer if it directly checked forprocess:const getRuntimeEnv = (): CookieEnv => { - // In browser/react-native bundles process is undefined - if (typeof globalThis === "undefined") { + // In browser/react-native environments, process may not be defined + if (typeof process === "undefined") { return {}; } - const maybeProcess = (globalThis as { process?: { env?: CookieEnv } }) - .process; - return maybeProcess?.env ?? {}; + return process?.env ?? {}; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/main.test.ts(1 hunks)lib/main.ts(1 hunks)lib/utils/exchangeAuthCode.test.ts(13 hunks)lib/utils/getCookieOptions.test.ts(1 hunks)lib/utils/getCookieOptions.ts(1 hunks)lib/utils/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
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`.
📚 Learning: 2024-09-23T22:08:18.788Z
Learnt from: DanielRivers
Repo: kinde-oss/js-utils PR: 12
File: lib/main.ts:0-0
Timestamp: 2024-09-23T22:08:18.788Z
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/main.tslib/main.test.tslib/utils/index.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/main.tslib/utils/index.tslib/utils/exchangeAuthCode.test.tslib/utils/getCookieOptions.test.tslib/utils/getCookieOptions.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/main.tslib/utils/exchangeAuthCode.test.ts
📚 Learning: 2024-09-19T22:17:02.939Z
Learnt from: DanielRivers
Repo: kinde-oss/js-utils PR: 8
File: lib/utils/token/testUtils/index.ts:3-39
Timestamp: 2024-09-19T22:17:02.939Z
Learning: The file `lib/utils/token/testUtils/index.ts` is a test utility, not production code.
Applied to files:
lib/main.test.tslib/utils/exchangeAuthCode.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/index.tslib/utils/exchangeAuthCode.test.ts
📚 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/exchangeAuthCode.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/utils/exchangeAuthCode.test.ts
🧬 Code graph analysis (1)
lib/utils/getCookieOptions.test.ts (1)
lib/utils/getCookieOptions.ts (4)
getCookieOptions(57-94)TWENTY_NINE_DAYS(20-20)MAX_COOKIE_LENGTH(21-21)removeTrailingSlash(40-55)
🔇 Additional comments (17)
lib/utils/exchangeAuthCode.test.ts (6)
13-21: LGTM! Clean fetch mock abstraction.The
fetchMocktyped asvi.fn<typeof fetch>()and thejsonResponsehelper provide a clean, type-safe way to mock fetch responses. This is a good improvement over the vitest-fetch-mock dependency.
35-49: LGTM! Proper test lifecycle management.The
beforeEach/afterEachsetup ensures proper test isolation by resetting the mock before each test and cleaning up globals after each test.
146-182: LGTM!The mock setup and assertions are well-structured, properly verifying both the fetch URL and request headers.
304-322: LGTM! Proper HTTP error simulation.Using
new Response("error", { status: 500, statusText: "Internal Server Error" })correctly simulates server error responses, allowing the code under test to handle the response as it would in production.
461-474: LGTM!Correctly uses
mockRejectedValueOnceto simulate network-level fetch failures, distinct from HTTP error responses.
563-584: LGTM!The test properly verifies auto-refresh functionality, including storage interaction for the refresh token.
lib/main.test.ts (1)
50-50: LGTM!The test correctly includes
getCookieOptionsin the expected exports, maintaining the export contract validation.lib/main.ts (1)
27-29: LGTM!The new exports follow the existing pattern in the file. The
export typesyntax correctly handles the type-only exports.lib/utils/index.ts (1)
12-17: LGTM!The new exports follow the existing barrel export pattern in this file, correctly separating the function export from the type exports.
lib/utils/getCookieOptions.test.ts (5)
1-9: LGTM!Clean imports with proper separation of concerns - test utilities from vitest and the module under test.
10-26: LGTM!Good test coverage for the default configuration, including verification that trailing slashes are removed from the domain.
28-48: LGTM!Good test coverage for option overrides, including verification that custom passthrough options are preserved.
75-102: LGTM!Good coverage of the warning scenarios with proper spy cleanup. Both tests verify the specific warning messages emitted.
105-122: LGTM!Comprehensive test coverage for
removeTrailingSlashincluding edge cases for nullish values and whitespace-only strings.lib/utils/getCookieOptions.ts (3)
23-27: LGTM: Secure defaults configured.The cookie options provide good security defaults with
httpOnly: trueto prevent XSS access andsameSite: "lax"to provide CSRF protection while maintaining usability.
40-55: LGTM: Edge cases handled correctly.The function properly handles null, undefined, whitespace-only strings, and empty strings by returning
undefined, ensuring consistent behavior for malformed inputs.
57-94: LGTM: Merge logic and warnings are well-structured.The function correctly:
- Merges options in the proper order (defaults → global options → user overrides)
- Handles the
secureflag with environment-aware defaults- Emits diagnostic warnings for malformed domains and missing
NODE_ENV- Allows user options to override environment-derived values
Note: Domain validation is intentionally minimal—the function trims whitespace and removes trailing slashes but does not validate domain format (e.g., invalid characters or protocol prefixes). This is consistent with the library's design to delegate format validation to the cookie-setting layer or consuming code.
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 (2)
lib/utils/exchangeAuthCode.test.ts (2)
188-188: Unusedstore2makes assertion meaningless.
store2is created at line 188 but never set as any storage (active or insecure). The assertion at lines 231-234 checksstore2forrefreshToken, but sincestore2was never connected to the system, it will always be null regardless of the test's behavior.If the intent is to verify refresh token storage behavior, this should either be removed or corrected to check the appropriate storage.
🔎 Option 1: Remove unused code
- const store2 = new MemoryStorage(); const state = "state";- const insecureRefreshToken = await store2.getSessionItem( - StorageKeys.refreshToken, - ); - expect(insecureRefreshToken).toBeNull();🔎 Option 2: If intent is to verify refresh token is stored in insecure storage
const postCodeVerifier = await store.getSessionItem( StorageKeys.codeVerifier, ); expect(postCodeVerifier).toBeNull(); - const insecureRefreshToken = await store2.getSessionItem( + const insecureRefreshToken = await store.getSessionItem( StorageKeys.refreshToken, ); - expect(insecureRefreshToken).toBeNull(); + expect(insecureRefreshToken).toBe("refresh_token");Also applies to: 231-234
600-606: Fix:mockResponseOnceis not a function — causes pipeline failure.This test still uses the old vitest-fetch-mock API (
mockResponseOnce), but the file has migrated to native Vitest mocking withvi.fn(). This is the direct cause of the pipeline failure.🔎 Apply this diff to fix the issue:
- fetchMock.mockResponseOnce( - JSON.stringify({ + fetchMock.mockResolvedValueOnce( + jsonResponse({ access_token: "access_token", refresh_token: "refresh_token", id_token: "id_token", }), );
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/main.test.ts(1 hunks)lib/main.ts(1 hunks)lib/utils/exchangeAuthCode.test.ts(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/main.test.ts
- lib/main.ts
🧰 Additional context used
🧠 Learnings (6)
📚 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/exchangeAuthCode.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/exchangeAuthCode.test.ts
📚 Learning: 2024-09-19T22:17:02.939Z
Learnt from: DanielRivers
Repo: kinde-oss/js-utils PR: 8
File: lib/utils/token/testUtils/index.ts:3-39
Timestamp: 2024-09-19T22:17:02.939Z
Learning: The file `lib/utils/token/testUtils/index.ts` is a test utility, not production code.
Applied to files:
lib/utils/exchangeAuthCode.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/exchangeAuthCode.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/utils/exchangeAuthCode.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/exchangeAuthCode.test.ts
🪛 GitHub Actions: Build and test
lib/utils/exchangeAuthCode.test.ts
[error] 1-1: TypeError: fetchMock.mockResponseOnce is not a function
🔇 Additional comments (2)
lib/utils/exchangeAuthCode.test.ts (2)
13-21: LGTM!Clean setup for native Vitest fetch mocking. The
jsonResponsehelper provides a good abstraction for creating properly typed Response objects.
35-49: LGTM!Proper mock lifecycle management with
mockReset(),stubGlobal(), and cleanup inafterEach.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/utils/exchangeAuthCode.test.ts (1)
563-569: Consider usingmockResolvedValueOncefor consistency.This test uses
mockResolvedValuewhile all others usemockResolvedValueOnce. Although it works (only one fetch call occurs), usingmockResolvedValueOncewould be consistent with the rest of the file and make the intent clearer.🔎 Apply this diff for consistency:
- fetchMock.mockResolvedValue( + fetchMock.mockResolvedValueOnce( jsonResponse({ access_token: "access", id_token: "id", refresh_token: "refresh", }), );
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/utils/exchangeAuthCode.test.ts(14 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 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/exchangeAuthCode.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/exchangeAuthCode.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/exchangeAuthCode.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/utils/exchangeAuthCode.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/exchangeAuthCode.test.ts
🔇 Additional comments (2)
lib/utils/exchangeAuthCode.test.ts (2)
13-21: LGTM! Clean mock implementation.The local fetch mock with the
jsonResponsehelper is well-designed. The helper properly sets Content-Type headers and allows customization via theinitparameter.
36-37: LGTM! Proper test isolation.The setup and teardown correctly manage the fetch mock lifecycle, ensuring clean state between tests.
Also applies to: 46-47
lib/utils/getCookieOptions.ts
Outdated
| export function removeTrailingSlash( | ||
| url: string | undefined | null, | ||
| ): string | undefined { | ||
| if (url === undefined || url === null) return undefined; | ||
|
|
||
| url = url.trim(); | ||
| if (url.length === 0) { | ||
| return undefined; | ||
| } | ||
|
|
||
| if (url.endsWith("/")) { | ||
| url = url.slice(0, -1); | ||
| } | ||
|
|
||
| return url; | ||
| } |
There was a problem hiding this comment.
We have a method for this called sanatizeUrl
There was a problem hiding this comment.
Addressed 🟢 and simplified, since I think after all trimming logic here is an overkill, additionally, sanitizeUrl is designed for URL paths (normalizing slashes, preserving :// protocols) whereas cookie domains are hostnames that should never contain protocols. I think the caller should remain responsible for providing a clean domain value.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/utils/getCookieOptions.ts (1)
7-7: Consider using a literal type forsameSite.For better type safety,
sameSiteshould use a literal union type instead ofstring:sameSite?: "strict" | "lax" | "none";This prevents invalid values and provides better IDE autocomplete.
🔎 View suggested change
- sameSite?: string; + sameSite?: "strict" | "lax" | "none";
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/main.ts(1 hunks)lib/utils/getCookieOptions.test.ts(1 hunks)lib/utils/getCookieOptions.ts(1 hunks)lib/utils/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/main.ts
- lib/utils/getCookieOptions.test.ts
🧰 Additional context used
🧠 Learnings (4)
📚 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/getCookieOptions.tslib/utils/index.ts
📚 Learning: 2024-11-19T20:35:25.531Z
Learnt from: DanielRivers
Repo: kinde-oss/js-utils PR: 11
File: lib/utils/token/refreshToken.ts:3-3
Timestamp: 2024-11-19T20:35:25.531Z
Learning: The function `sanitizeUrl` is correctly named with a lowercase 'u' in 'Url'. There is no function named `sanitizeURL` in the codebase. Ensure that import statements use `sanitizeUrl` to match the function name.
Applied to files:
lib/utils/getCookieOptions.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/utils/index.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/index.ts
🔇 Additional comments (3)
lib/utils/index.ts (1)
12-13: LGTM! Exports follow existing patterns.The re-exports for
getCookieOptionsand its types are correctly structured and consistent with other exports in this file.lib/utils/getCookieOptions.ts (2)
14-15: Well-chosen default constants.The 29-day maxAge and 3000-byte maxCookieLength are sensible defaults that balance usability with browser limitations.
25-32: Implementation is correct but PR description is outdated.The function correctly merges user options with global defaults, giving user options precedence. However, the PR description mentions features that are not present in the code:
- "emits warnings when runtime configuration is incomplete"
- "whitespace handling and diagnostic logs to detect malformed cookie domains"
- "Handles removeTrailingSlash edge cases"
Based on the past review comments, these features were intentionally removed. Please update the PR description to accurately reflect the current implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/utils/getCookieOptions.ts (2)
11-11: Consider removing the index signature unless arbitrary properties are required.The
[key: string]: CookieOptionValueindex signature allows any additional properties beyond the explicitly defined ones. While this provides flexibility, it may silently accept typos or unintended properties (e.g.,httpOnyl: truewould be accepted without error).If cookie options are meant to be strictly typed to the defined properties, consider removing the index signature. If there's a specific need for extensibility, document why arbitrary properties are allowed.
🔎 Proposed fix to remove index signature
export interface CookieOptions { maxAge?: number; domain?: string; maxCookieLength?: number; sameSite?: string; httpOnly?: boolean; secure?: boolean; path?: string; - [key: string]: CookieOptionValue; }
7-7: Consider stricter typing forsameSite.The
sameSiteproperty is currently typed asstring?, which allows any string value. Standard cookie sameSite values are"strict","lax", or"none". Using a string literal union would provide better type safety and prevent invalid values.🔎 Proposed fix for stricter sameSite typing
export interface CookieOptions { maxAge?: number; domain?: string; maxCookieLength?: number; - sameSite?: string; + sameSite?: "strict" | "lax" | "none"; httpOnly?: boolean; secure?: boolean; path?: string; [key: string]: CookieOptionValue; }Note: If you remove the index signature as suggested in the other comment, this change becomes straightforward. If you keep the index signature, you may need to adjust it to accommodate the union type, e.g., by using
CookieOptionValue | ("strict" | "lax" | "none")for the signature.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/utils/getCookieOptions.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: dtoxvanilla1991
Repo: kinde-oss/js-utils PR: 184
File: lib/utils/getCookieOptions.ts:17-23
Timestamp: 2025-12-18T23:03:56.010Z
Learning: In lib/utils/getCookieOptions.ts, the `secure` flag is intentionally omitted from GLOBAL_COOKIE_OPTIONS by design because this is a universal js-utils library used across multiple Kinde SDKs and needs to support various environments including local development over HTTP. Consumers can override with `secure: true` when needed.
📚 Learning: 2025-12-18T23:03:56.010Z
Learnt from: dtoxvanilla1991
Repo: kinde-oss/js-utils PR: 184
File: lib/utils/getCookieOptions.ts:17-23
Timestamp: 2025-12-18T23:03:56.010Z
Learning: In lib/utils/getCookieOptions.ts, the `secure` flag is intentionally omitted from GLOBAL_COOKIE_OPTIONS by design because this is a universal js-utils library used across multiple Kinde SDKs and needs to support various environments including local development over HTTP. Consumers can override with `secure: true` when needed.
Applied to files:
lib/utils/getCookieOptions.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/getCookieOptions.ts
📚 Learning: 2024-11-19T20:35:25.531Z
Learnt from: DanielRivers
Repo: kinde-oss/js-utils PR: 11
File: lib/utils/token/refreshToken.ts:3-3
Timestamp: 2024-11-19T20:35:25.531Z
Learning: The function `sanitizeUrl` is correctly named with a lowercase 'u' in 'Url'. There is no function named `sanitizeURL` in the codebase. Ensure that import statements use `sanitizeUrl` to match the function name.
Applied to files:
lib/utils/getCookieOptions.ts
📚 Learning: 2025-01-28T14:24:26.982Z
Learnt from: DanielRivers
Repo: kinde-oss/js-utils PR: 69
File: lib/utils/exchangeAuthCode.ts:152-154
Timestamp: 2025-01-28T14:24:26.982Z
Learning: Security implications of insecure token storage for non-custom domains in js-utils are documented in the library documentation, and warnings are implemented through the localStorage implementation.
Applied to files:
lib/utils/getCookieOptions.ts
📚 Learning: 2025-01-16T21:47:40.306Z
Learnt from: DanielRivers
Repo: kinde-oss/js-utils PR: 61
File: lib/sessionManager/types.ts:20-20
Timestamp: 2025-01-16T21:47:40.306Z
Learning: The `useInsecureForRefreshToken` setting in `lib/sessionManager/index.ts` should only be used when not using a custom domain and when there's no backend app to authenticate on, as documented in the implementation file.
Applied to files:
lib/utils/getCookieOptions.ts
📚 Learning: 2025-01-20T20:01:21.460Z
Learnt from: DanielRivers
Repo: kinde-oss/js-utils PR: 69
File: lib/utils/token/refreshToken.ts:54-57
Timestamp: 2025-01-20T20:01:21.460Z
Learning: In the Kinde JS utils library, when not using a custom domain, local storage is used by default for storing refresh tokens. This is an intentional design decision that will be documented. Users can explicitly opt out of this behavior if needed.
Applied to files:
lib/utils/getCookieOptions.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/getCookieOptions.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/getCookieOptions.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/utils/getCookieOptions.ts
🧬 Code graph analysis (1)
lib/utils/getCookieOptions.ts (2)
lib/utils/index.ts (3)
CookieOptionValue(13-13)CookieOptions(13-13)getCookieOptions(12-12)lib/main.ts (3)
CookieOptionValue(29-29)CookieOptions(29-29)getCookieOptions(27-27)
🔇 Additional comments (3)
lib/utils/getCookieOptions.ts (3)
14-15: LGTM!The constant values are correct. TWENTY_NINE_DAYS correctly represents 29 days in seconds (29 × 86,400 = 2,505,600), and MAX_COOKIE_LENGTH of 3000 bytes is a sensible limit well below typical browser cookie size constraints.
17-34: Excellent documentation addressing security concerns!The JSDoc comment effectively explains the design decision to omit the
secureflag and provides clear guidance for production deployments. The default values are sensible for a framework-agnostic utility.
54-61: LGTM!The implementation is clean and correct. The object spread merge gives proper precedence to user-provided options while maintaining sensible defaults. The JSDoc documentation with examples and security warnings effectively guides consumers.
lib/utils/getCookieOptions.ts
Outdated
| httpOnly?: boolean; | ||
| secure?: boolean; | ||
| path?: string; | ||
| [key: string]: CookieOptionValue; |
There was a problem hiding this comment.
Remove this item, as it will break typesafety.
Can remove CookieOptionValue also
There was a problem hiding this comment.
Yea I was not sure if flexible option is needed just in case. Happily removed.
lib/utils/getCookieOptions.ts
Outdated
| } | ||
|
|
||
| export const TWENTY_NINE_DAYS = 2505600; | ||
| export const MAX_COOKIE_LENGTH = 3000; |
There was a problem hiding this comment.
This should be taken from the storageSettings.maxLength
There was a problem hiding this comment.
addressed 👍🏼 . Removed 🟢
lib/utils/getCookieOptions.ts
Outdated
| */ | ||
| export const GLOBAL_COOKIE_OPTIONS: CookieOptions = { | ||
| maxAge: TWENTY_NINE_DAYS, | ||
| maxCookieLength: MAX_COOKIE_LENGTH, |
There was a problem hiding this comment.
This can be read from storageSettings.maxLength once its defined, its already exported.
…ength: Daniel feedback
dtoxvanilla1991
force-pushed
the
feat/getCookiesOptions
branch
from
c59ff0a to
70e9189
Compare
2 participants
Explain your changes
getCookieOptionswith explicit typing, optionalenvinjection, and warnings when runtime config is incomplete.NODE_ENVdon’t slip through silently.Vitestcoverage for defaults, overrides, runtime fallbacks, warnings, and removeTrailingSlash edge cases; all tests pass.Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.