added 2 checks for the function. 1. exp check for iaAuthenticatedFact… #224
Conversation
…ory function 2. issuer check, both are to determine the token is expired or not
mohitsinhchavda
requested review from
DaveOrDead and
peterphanouvong
as code owners
WalkthroughThe changes in this pull request significantly update the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/session/isAuthenticated.js (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
src/session/isAuthenticated.js (1)
18-21: Ensure Authorization Header FormatThe extraction of the token assumes the
Authorizationheader follows the formatBearer <token>. If the format varies, the token extraction might fail.Please verify that all clients send the
Authorizationheader in the expected format. Run the following script to check for any inconsistencies in the codebase:✅ Verification successful
Authorization Header Format is consistent across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the format of the Authorization header usage in the codebase. # Search for usage of 'Authorization' header. rg --type javascript "Authorization" # Check if there are any variations in how the token is extracted. rg --type javascript "req.headers.authorization"Length of output: 155
Script:
#!/bin/bash # Description: Verify the format of the Authorization header usage in the codebase. # Search for usage of 'Authorization' header. rg --type js "Authorization" # Check if there are any variations in how the token is extracted. rg --type js "req.headers.authorization"Length of output: 158
Script:
#!/bin/bash # Description: Verify the format of the Authorization header usage in the codebase. # Search for usage of 'Authorization' header. rg --type js "Authorization" # Check if there are any variations in how the token is extracted. rg --type js "req.headers.authorization"Length of output: 158
|
|
||
| return true; | ||
| } catch (error) { | ||
| console.error('Token decoding failed:', error); |
There was a problem hiding this comment.
Avoid Logging Sensitive Error Information
Logging the entire error object with console.error('Token decoding failed:', error); may expose sensitive information in your logs. This could include stack traces or token contents, which could be a security concern. Consider logging only a generic error message.
Apply this diff to fix the issue:
- console.error('Token decoding failed:', error);
+ console.error('Token verification failed');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.error('Token decoding failed:', error); | |
| console.error('Token verification failed'); |
| const { exp, iss } = decoded; | ||
| const currentTime = Math.floor(Date.now() / 1000); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential Redundancy in Expiration Check
After using jwt.verify, the token's expiration is already validated. The jwt.verify function will throw an error if the token is expired. Therefore, the manual expiration check may be unnecessary.
You can simplify the code by removing the manual expiration check:
const { exp, iss } = decoded;
const currentTime = Math.floor(Date.now() / 1000);
- // Check expiration
- if (exp < currentTime) {
- return false;
- }Committable suggestion was skipped due to low confidence.
| if (iss !== removeTrailingSlash(process.env.KINDE_ISSUER_URL)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Issuer Validation Already Handled
The issuer validation is specified in the jwt.verify options. The manual issuer check might be redundant if you've included the issuer option in jwt.verify.
You can remove the manual issuer check:
// Verify issuer
- if (iss !== removeTrailingSlash(process.env.KINDE_ISSUER_URL)) {
- return false;
- }Ensure that the issuer is correctly specified in the jwt.verify options to handle this validation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (iss !== removeTrailingSlash(process.env.KINDE_ISSUER_URL)) { | |
| return false; | |
| } | |
| // Verify issuer |
| const decoded = jwtDecode(token); | ||
| if (!decoded) { | ||
| return false; | ||
| } | ||
|
|
||
| const { exp, iss } = decoded; | ||
| const currentTime = Math.floor(Date.now() / 1000); | ||
|
|
||
| // Check expiration | ||
| if (exp < currentTime) { | ||
| return false; | ||
| } | ||
|
|
||
| // Verify issuer | ||
| if (iss !== removeTrailingSlash(process.env.KINDE_ISSUER_URL)) { | ||
| return false; | ||
| } | ||
|
|
||
| return true; |
There was a problem hiding this comment.
Security Issue: JWT Signature Not Verified
Currently, the JWT is decoded using jwtDecode without verifying its signature, which poses a significant security risk. An attacker could provide a forged token that passes decoding, expiration, and issuer checks. It is crucial to verify the token's signature to ensure the token is authentic and has not been tampered with.
Apply this diff to fix the issue:
-import jwtDecode from 'jwt-decode';
+import jwt from 'jsonwebtoken';
export const isAuthenticatedFactory = (req, res) => async () => {
const user = await getUserFactory(req, res)();
if (!user) {
return false;
}
const token = req.headers.authorization?.split(' ')[1];
if (!token) {
return false;
}
try {
- const decoded = jwtDecode(token);
+ const decoded = jwt.verify(token, publicKey, {
+ issuer: removeTrailingSlash(process.env.KINDE_ISSUER_URL),
+ algorithms: ['RS256'], // Use the appropriate algorithm
+ });
const { exp, iss } = decoded;
const currentTime = Math.floor(Date.now() / 1000);
// Check expiration
if (exp < currentTime) {
return false;
}
// Verify issuer (already handled in jwt.verify options)
return true;
} catch (error) {
- console.error('Token decoding failed:', error);
+ console.error('Token verification failed');
return false;
}
};Note: You'll need to:
- Import the public key used to sign the JWTs, which can often be obtained from your authentication provider's discovery document or metadata endpoint.
- Replace
publicKeyin the code with the actual key or a reference to where you've stored it. - Specify the correct algorithm used to sign your JWTs in the
algorithmsarray.
Committable suggestion was skipped due to low confidence.
|
Thanks for creating this PR, this was fixed in created PR which handled it more inline with the rest of the SDK. We really appreciate you taking the time to contribute and welcome future contributions. 🙏 |
mohitsinhchavda
deleted the
bugfix/add-exp-check-to-isAuthenticatedFactory-function
branch
|
Thanks for informing @DanielRivers . |
…ory function 2. issuer check, both are to determine the token is expired or not
Bug/Issue
If the tokens just exists in the cookies storage, the
isAuthenticatedFactory()function returns true, without checking expiration of issuer of the token, which is causing the issue.Changes
Added few checks for
isAuthenticatedFactoryfunction.Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.