Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added 2 checks for the function. 1. exp check for iaAuthenticatedFact… #224

Closed
wants to merge 1 commit into from
Closed

added 2 checks for the function. 1. exp check for iaAuthenticatedFact… #224

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 38 additions & 3 deletions src/session/isAuthenticated.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import {getUserFactory} from './getUser';
import { removeTrailingSlash } from '../utils/removeTrailingSlash';
import { getUserFactory } from './getUser';
import jwtDecode from 'jwt-decode';

/**
*
Expand All @@ -8,5 +10,38 @@ import {getUserFactory} from './getUser';
*/
export const isAuthenticatedFactory = (req, res) => async () => {
const user = await getUserFactory(req, res)();
return Boolean(user);
};

if (!user) {
return false;
}

const token = req.headers.authorization?.split(' ')[1];
if (!token) {
return false;
}

try {
const decoded = jwtDecode(token);
if (!decoded) {
return false;
}

const { exp, iss } = decoded;
const currentTime = Math.floor(Date.now() / 1000);

Comment on lines +29 to +31
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

// Check expiration
if (exp < currentTime) {
return false;
}

// Verify issuer
if (iss !== removeTrailingSlash(process.env.KINDE_ISSUER_URL)) {
return false;
}
Comment on lines +38 to +40
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
if (iss !== removeTrailingSlash(process.env.KINDE_ISSUER_URL)) {
return false;
}
// Verify issuer


return true;
Comment on lines +24 to +42
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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 publicKey in 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 algorithms array.

Committable suggestion was skipped due to low confidence.

} catch (error) {
console.error('Token decoding failed:', error);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
console.error('Token decoding failed:', error);
console.error('Token verification failed');

return false;
}
};