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.

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

feat: evaluation logic in typescript for nodejs sdk #1258

Draft
wants to merge 61 commits into
base: main
Choose a base branch
from

Conversation

duyhungtnn
Copy link
Collaborator

@duyhungtnn duyhungtnn commented Sep 25, 2024

Part of #1260

Local setup

cd evaluation/typescript
export NPM_TOKEN=""
make init
make gen_proto


private hash(userID: string, featureID: string, samplingSeed: string): string {
const concat = `${featureID}-${userID}${samplingSeed}`;
return crypto.createHash('md5').update(concat).digest('hex');

Check failure

Code scanning / CodeQL

Use of a broken or weak cryptographic algorithm High

A broken or weak cryptographic algorithm
depends on
sensitive data from an access to userID
.
A broken or weak cryptographic algorithm
depends on
sensitive data from an access to userID
.
A broken or weak cryptographic algorithm
depends on
sensitive data from an access to userID
.
A broken or weak cryptographic algorithm
depends on
sensitive data from an access to userID
.

Copilot Autofix AI about 1 month ago

To fix the problem, we need to replace the weak MD5 hashing algorithm with a stronger one, such as SHA-256. This change will ensure that the hash function used is secure and resistant to collision attacks.

  • Replace the crypto.createHash('md5') call with crypto.createHash('sha256').
  • Ensure that the rest of the code remains unchanged to maintain existing functionality.
Suggested changeset 1
evaluation/typescript/src/strategyEvaluator.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/evaluation/typescript/src/strategyEvaluator.ts b/evaluation/typescript/src/strategyEvaluator.ts
--- a/evaluation/typescript/src/strategyEvaluator.ts
+++ b/evaluation/typescript/src/strategyEvaluator.ts
@@ -62,3 +62,3 @@
     const concat = `${featureID}-${userID}${samplingSeed}`;
-    return crypto.createHash('md5').update(concat).digest('hex');
+    return crypto.createHash('sha256').update(concat).digest('hex');
   }
EOF
@@ -62,3 +62,3 @@
const concat = `${featureID}-${userID}${samplingSeed}`;
return crypto.createHash('md5').update(concat).digest('hex');
return crypto.createHash('sha256').update(concat).digest('hex');
}
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's the same cryptographic algorithm as the Golang package, so it's okay.

@cre8ivejp cre8ivejp changed the title Feat/node evaluation package feat: evaluation logic in typescript for nodejs sdk Sep 25, 2024
@kentakozuka
Copy link
Contributor

@duyhungtnn Thanks for creating PR! Please make this ready to review when it is 👍

@duyhungtnn duyhungtnn marked this pull request as ready for review October 1, 2024 08:27
@duyhungtnn
Copy link
Collaborator Author

hi @kentakozuka , it's ready. Please help me to review it.

kentakozuka
kentakozuka previously approved these changes Oct 3, 2024
Copy link
Contributor

@kentakozuka kentakozuka left a comment

Choose a reason for hiding this comment

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

Amazing! LGTM 👍

@cre8ivejp cre8ivejp marked this pull request as draft October 11, 2024 07:17
@duyhungtnn duyhungtnn marked this pull request as ready for review November 19, 2024 02:30
@duyhungtnn
Copy link
Collaborator Author

Fixes #1362 in 71a4932

@cre8ivejp cre8ivejp marked this pull request as draft December 6, 2024 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants