Closed kjkurtz closed 4 months ago
โฑ๏ธ Estimated effort to review [1-5] | 2, because the changes are straightforward and localized to specific functionalities. The PR includes both the implementation of the new error handling logic and the corresponding tests, which are well-documented and easy to follow. |
๐งช Relevant tests | Yes |
โก Possible issues | Time Manipulation in Tests: Using fake timers and manipulating system time in tests can lead to issues if not handled correctly across different environments or if the cleanup (useRealTimers) fails or is skipped due to test interruptions. |
๐ Security concerns | No |
Category | Suggestion | Score |
Security |
Return a generic error message instead of the actual error message for security reasons___ **Instead of returning the error message directly, it would be more secure to return ageneric error message to avoid leaking sensitive information about the token verification process.** [src/auth.ts [229]](https://github.com/FlourishHealth/ferns-api/pull/395/files#diff-84a5d130fb4cccb80f78601d140f1d5397dc0272f94cea672a8470539fcf6dc7R229-R229) ```diff -return res.status(401).json({message: error?.message}); +return res.status(401).json({message: "Invalid token"}); ``` Suggestion importance[1-10]: 9Why: This is a crucial security improvement to prevent leaking details about the internal workings of the token verification process. | 9 |
Best practice |
Ensure
___
**To ensure that the | 7 |
Reset the system time back to the original time after the test to avoid side effects___ **In the new test case for token expiration, it is better to reset the system time back tothe current time after the test completes to avoid side effects on other tests.** [src/auth.test.ts [259]](https://github.com/FlourishHealth/ferns-api/pull/395/files#diff-7e878fd5df9f5b949227e866aa85dc4383321531d0fc22ee586b850509000f33R259-R259) ```diff -jest.setSystemTime(new Date().getTime() + 1000 * 60 * 60 * 24 * 30); +const originalTime = new Date().getTime(); +jest.setSystemTime(originalTime + 1000 * 60 * 60 * 24 * 30); +// other test code +jest.setSystemTime(originalTime); ``` Suggestion importance[1-10]: 7Why: This suggestion is valid as it prevents potential side effects on other tests by ensuring the system time is reset after modifications. | 7 | |
Possible issue |
Add a check to ensure the token is valid before setting the system time forward in the token expiration test___ **In the new test case for token expiration, add a check to ensure that the token is stillvalid before setting the system time forward. This will make the test more robust by verifying the token's validity before simulating its expiration.** [src/auth.test.ts [257-261]](https://github.com/FlourishHealth/ferns-api/pull/395/files#diff-7e878fd5df9f5b949227e866aa85dc4383321531d0fc22ee586b850509000f33R257-R261) ```diff await agent.get("/auth/me").expect(200); jest.setSystemTime(new Date().getTime() + 1000 * 60 * 60 * 24 * 30); +await agent.get("/auth/me").expect(200); // Ensure token is still valid before expiration await agent.get("/auth/me").expect(401); ``` Suggestion importance[1-10]: 6Why: Adding a validity check before simulating token expiration would make the test more robust, although it's not critical for the test's primary purpose. | 6 |
User description
We weren't throwing an error if the token could not be decoded or was expired, so it just hung. This now throws an error.
PR Type
Bug fix, Tests
Description
Changes walkthrough ๐
auth.test.ts
Add tests for token expiration and timer handling.
src/auth.test.ts
auth.ts
Handle JWT verification errors and return 401 status.
src/auth.ts