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

fix: embed user id in JWT #17

Merged
merged 6 commits into from
Jun 20, 2024
Merged

fix: embed user id in JWT #17

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
16cbced
fix: embed user id in JWT
aversini Jun 20, 2024
b513b5f
Merge branch 'main' into fix-embed-user-id-in-JWT
aversini Jun 20, 2024
f62826b
Update pnpm-lock.yaml
aversini Jun 20, 2024
718e4bd
Update AuthProvider.tsx
aversini Jun 20, 2024
2c2c4f1
Update AuthProvider.tsx
aversini Jun 20, 2024
2cb01e8
Update bundlesize.config.js
aversini Jun 20, 2024
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
5 changes: 2 additions & 3 deletions packages/auth-provider/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,7 @@
"type": "module",
"main": "dist/index.js",
"types": "dist/index.d.ts",
"files": [
"dist"
],
"files": ["dist"],
"scripts": {
"build:check": "tsc",
"build:js": "vite build",
Expand Down Expand Up @@ -46,6 +44,7 @@
"dependencies": {
"@versini/auth-common": "workspace:../auth-common",
"@versini/ui-hooks": "3.0.0",
"jose": "5.4.1",
"uuid": "10.0.0"
}
}
40 changes: 24 additions & 16 deletions packages/auth-provider/src/components/AuthProvider/AuthProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { AUTH_TYPES } from "@versini/auth-common";
import { useLocalStorage } from "@versini/ui-hooks";
import * as jose from "jose";
import { useEffect, useState } from "react";

import { EXPIRED_SESSION } from "../../common/constants";
Expand Down Expand Up @@ -39,13 +40,14 @@ export const AuthProvider = ({

useEffect(() => {
if (previousIdToken !== idToken && idToken !== "") {
const { _id }: { _id: string } = jose.decodeJwt(idToken);
setAuthState({
isAuthenticated: true,
accessToken,
refreshToken,
idToken,
logoutReason: "",
userId: authState.userId,
userId: _id || "",
Copy link

Choose a reason for hiding this comment

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

Tip

Codebase Verification

The current implementation does not explicitly validate the JWT before extracting _id. To follow security best practices, ensure to validate the JWT token to confirm its authenticity and integrity before using any data from it.

  • Add JWT validation steps using jose or another trusted library before extracting _id.
Analysis chain

The refactored useEffect uses jose.decodeJwt to handle idToken changes effectively. Ensure that _id extraction follows security best practices.

Also applies to: 62-62

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the security practices around `_id` extraction from JWT.
# Test: Check for any security advisories or best practices documentation regarding JWT `_id` extraction.
echo "Check security best practices for JWT `_id` extraction."

Length of output: 188

Script:

#!/bin/bash
# Search for all instances of `jose.decodeJwt` in the codebase to review its usage.
rg 'jose.decodeJwt' --context 5

Length of output: 2136

Script:

#!/bin/bash
# Search for all instances where `_id` is used in the codebase to ensure it is handled securely.
rg '\b_id\b' --context 5

Length of output: 3800

Script:

#!/bin/bash
# Search for any JWT validation steps in the codebase to ensure the token is trusted before extracting `_id`.
rg 'validate' --context 5

Length of output: 8791

Script:

#!/bin/bash
# Search for any references to JWT libraries or functions that might handle validation.
rg 'jose' --context 5

Length of output: 4932

});
} else if (previousIdToken !== idToken && idToken === "") {
setAuthState({
Expand All @@ -57,7 +59,7 @@ export const AuthProvider = ({
userId: "",
});
}
}, [accessToken, refreshToken, idToken, previousIdToken, authState.userId]);
}, [accessToken, refreshToken, idToken, previousIdToken]);

const login = async (username: string, password: string) => {
const response = await serviceCall({
Expand All @@ -70,21 +72,27 @@ export const AuthProvider = ({
},
});

if (response.data?.idToken) {
setIdToken(response.data.idToken);
response.data.accessToken && setAccessToken(response.data.accessToken);
response.data.refreshToken && setRefreshToken(response.data.refreshToken);
setAuthState({
isAuthenticated: true,
idToken: response.data.idToken,
accessToken: response.data.accessToken,
refreshToken: response.data.refreshToken,
userId: response.data.userId,
logoutReason: "",
});
return true;
try {
const { _id }: { _id: string } = jose.decodeJwt(response.data.idToken);
if (_id) {
setIdToken(response.data.idToken);
response.data.accessToken && setAccessToken(response.data.accessToken);
response.data.refreshToken &&
setRefreshToken(response.data.refreshToken);
setAuthState({
isAuthenticated: true,
idToken: response.data.idToken,
accessToken: response.data.accessToken,
refreshToken: response.data.refreshToken,
userId: _id,
logoutReason: "",
});
return true;
}
return false;
} catch (_error) {
return false;
Comment on lines +86 to +105
Copy link

@coderabbitai coderabbitai bot Jun 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The login function now correctly uses jose.decodeJwt to set user data. Consider enhancing error handling to provide more informative feedback on decoding failures.

-			return false;
+			return { success: false, error: _error.message };

Committable suggestion was skipped due to low confidence.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I explicitly do not want to provide error messages so that authentication tempering is harder: we do not want to disclose what the authentication issue is... just log the user out with a generic "session timeout".

Copy link

Choose a reason for hiding this comment

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

@aversini, I understand your approach to enhance security by not providing specific error messages during authentication processes. It's a prudent measure to prevent potential tampering by obscuring the details of authentication failures. I'll keep this in mind for future reviews related to authentication mechanisms. Thank you for clarifying your security strategy!

Learnings added 
User: aversini
PR: aversini/auth-client#17
File: packages/auth-provider/src/components/AuthProvider/AuthProvider.tsx:86-105
Timestamp: 2024-06-20T07:35:46.809Z
Learning: aversini prefers not to disclose specific error messages during authentication processes to avoid aiding potential tampering. Instead, a generic "session timeout" message is used to obscure the details of authentication failures.

Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure review instructions.

}
return false;
};

const logout = () => {
Expand Down
8 changes: 8 additions & 0 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading