Fix AUTH_SECRET#352
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the ensureAuthSecret() function to implement proper caching of generated authentication secrets using a module-level variable. The changes ensure that the same secret is consistently returned across multiple calls, addressing potential issues where different secrets might be generated in different contexts.
Key Changes
- Added a module-level
cachedAuthSecretvariable to store generated secrets in memory - Refactored
ensureAuthSecret()to have a clear priority order: environment variable → cached value → newly generated value - Added a fallback code path for non-Node.js runtime environments (edge runtime, build time)
- Updated tests to verify the new caching behavior and ensure the function returns the correct value
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/server/auth-config.ts |
Implements caching mechanism for AUTH_SECRET with module-level variable and refactors function logic to handle different runtime environments |
__tests__/unit/server/auth-config.test.ts |
Adds new test for checking existing AUTH_SECRET and refactors existing tests to verify caching behavior and return values |
Comments suppressed due to low confidence (1)
src/server/auth-config.ts:38
- This negation always evaluates to true.
if (!cachedAuthSecret) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it('should return the same cached secret on subsequent calls', () => { | ||
| const mockRandomBytes = new Uint8Array(32) | ||
| for (let i = 0; i < 32; i++) { | ||
| mockRandomBytes[i] = i | ||
| } | ||
| mockGetRandomValues.mockImplementation((array: Uint8Array) => { | ||
| array.set(mockRandomBytes) | ||
| return array | ||
| }) | ||
|
|
||
| ensureAuthSecret() | ||
| const result1 = ensureAuthSecret() | ||
| const result2 = ensureAuthSecret() | ||
| const result3 = ensureAuthSecret() | ||
|
|
||
| expect(mockGetRandomValues).not.toHaveBeenCalled() | ||
| expect(process.env.AUTH_SECRET).toBe(existingSecret) | ||
| expect(mockGetRandomValues).toHaveBeenCalledTimes(1) | ||
| expect(result1).toBe(result2) | ||
| expect(result2).toBe(result3) | ||
| expect(result1).toBe(Buffer.from(mockRandomBytes).toString('base64')) | ||
| }) |
There was a problem hiding this comment.
The test should verify that process.env.AUTH_SECRET is set correctly on the first call, similar to the assertion in the test at line 71. This would ensure that the caching test covers the full behavior of setting both the cached variable and the environment variable.
|
No description provided.