deprecate crypto-js #8607

hkjpotato · 2021-07-20T23:28:50Z

Description of changes

Amplify has a dependency on crypto-js, yet we only use partial of its functions (Base64, SHA256, Hmac256). However, it is not actively maintained, and has introduced issue like NextJS bundle size to Amplify.

After reviewing available options, I decide to extract the Amplify needed codes from cryto-js, re-org and refactor a bit based on our own need, and put it in a single file CryptoJSHelper (the original partially implemented WordArray is replaced as well). Below is a brief summary of build size comparison.

	webpack	nextjs (first load js)	auth/dist/aws-amplify-auth-js
before	17.1 MB	281 kB	1.5 MB
after	15.7 MB	148 kB	437K

Alternatives comparison

	crypto-js	crypto-es	aws-crypto	native browser
pros	well adopted	es6 class based, familiar syntax to dev	already a dependency, maintained by aws
cons	prototype based, not actively maintained, bundle size issue	security concern due to Math.random	no Hmac support, does not work with WordArray	is async based, need refactor of all sync based functions

note: both crypto-js and crypto-es are forked and based on the original crypto-js from Jeff Wott, their codes (comments/logic/typing) are almost identical. This means the core algorithms/logics are stable. See more about its history.

This is perhaps a temporary solution before we have a better option (either crypto-js merge the bundle size fix PR), or when aws-cryto supports more algorithm and flexible data type.

Issue #, if available

#7570
#8256

Description of how you validated changes

local testing and run through the flows related to the used functions
I just modify a previous buggy test, but not adding any new unit tests as the imported code are basically from a third party lib.

Checklist

PR description included
yarn test passes
Tests are changed or added
Relevant documentation is changed or added (and PR referenced)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

lgtm-com · 2021-07-20T23:38:36Z

This pull request fixes 2 alerts when merging f602c6c into 34e7405 - view on LGTM.com

fixed alerts:

1 for Unused variable, import, function or class
1 for Use of password hash with insufficient computational effort

codecov-commenter · 2021-07-21T04:55:43Z

Codecov Report

Merging #8607 (bd4404a) into main (37422f2) will decrease coverage by 0.16%.
The diff coverage is 67.61%.

@@            Coverage Diff             @@
##             main    #8607      +/-   ##
==========================================
- Coverage   77.94%   77.78%   -0.17%     
==========================================
  Files         237      240       +3     
  Lines       16810    17112     +302     
  Branches     3613     3651      +38     
==========================================
+ Hits        13102    13310     +208     
- Misses       3581     3677      +96     
+ Partials      127      125       -2

Impacted Files	Coverage Δ
packages/amazon-cognito-identity-js/src/Client.js	`52.04% <0.00%> (ø)`
packages/datastore/__tests__/helpers.ts	`100.00% <ø> (ø)`
packages/datastore/src/types.ts	`84.78% <ø> (ø)`
...ckages/core/src/Providers/AWSCloudWatchProvider.ts	`58.36% <58.36%> (ø)`
packages/core/src/Parser.ts	`91.66% <66.66%> (-3.58%)`	⬇️
...ages/amazon-cognito-identity-js/src/CognitoUser.js	`79.32% <71.42%> (+0.96%)`	⬆️
packages/core/src/Logger/ConsoleLogger.ts	`82.89% <72.97%> (-1.11%)`	⬇️
...on-cognito-identity-js/src/AuthenticationHelper.js	`100.00% <100.00%> (ø)`
packages/amazon-cognito-identity-js/src/index.js	`100.00% <100.00%> (ø)`
packages/auth/src/OAuth/OAuth.ts	`87.58% <100.00%> (+0.08%)`	⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1c835b...bd4404a. Read the comment docs.

lgtm-com · 2021-07-21T17:44:58Z

This pull request fixes 2 alerts when merging bd4404a into 85e9b97 - view on LGTM.com

fixed alerts:

1 for Unused variable, import, function or class
1 for Use of password hash with insufficient computational effort

evcodes · 2021-07-21T18:03:32Z

packages/amazon-cognito-identity-js/src/CognitoUser.js

@@ -660,6 +657,7 @@ export default class CognitoUser {
 		authParameters.USERNAME = this.username;
 		authParameters.DEVICE_KEY = this.deviceKey;
 		authenticationHelper.getLargeAValue((errAValue, aValue) => {
+			console.log('get aValue', aValue);


Do you want to keep this log here?

good catch! no it is for my local debug, will remove it

evcodes · 2021-07-21T18:13:32Z

packages/amazon-cognito-identity-js/src/utils/CryptoJSHelper.js

+	 * Converts this word array to a string.
+	 *
+	 * @param {Encoder} encoder (Optional) The encoding strategy to use. Default: CryptoJS.enc.Hex
+	 *


I would remove the extra spacing between @param and @return

I believe it is copied from the original file, somehow I feel like each version of the crypto-js keeps the same styles to respect the original author, but I will do a clean up on the format :)

evcodes · 2021-07-21T18:15:47Z

packages/amazon-cognito-identity-js/src/utils/CryptoJSHelper.js

+};
+
+/**
+ * Abstract buffered block algorithm template. (checked, similar to crypto-es)


remove "(checked, similar to crypto-es)"

evcodes · 2021-07-21T18:25:16Z

packages/amazon-cognito-identity-js/src/utils/CryptoJSHelper.js

+/**
+ * HMAC algorithm.
+ */
+(function (algo) {


Can you explain what the algo parameter is in this scenario?

it is basically an "IIFE" style to create namespace, because they are all put in the same file.
This is to avoid naming conflict between SHA256algo (the algorithm) and the actual CryptoJSHelper.SHA256 function exposed, like this

evcodes · 2021-07-21T18:45:08Z

packages/amazon-cognito-identity-js/src/utils/CryptoJSHelper.js

+(function (algo) {
+
+	// Initialization and round constants tables
+	const H = [];


why are the variable names H,K?

it is copied from the original codebase
https://github.com/brix/crypto-js/blob/develop/src/sha256.js#L10
https://github.com/entronad/crypto-es/blob/master/lib/sha256.js#L7

evcodes · 2021-07-21T19:08:27Z

packages/auth/__tests__/auth-federation-unit-tests.ts

-		default: jest.fn(() => ''),
-	};
-});
+


we should still have some tests for CryptoJSHelper.SHA256 I believe

I need to think more of whether we want to add tests for it, as it is basically copied from third party logic. If we add test, we probably add similar tests like the original crypto-js. Adding test also means we want to maintain it, which I am trying to avoid.

hkjpotato · 2021-07-22T17:12:14Z

close as the fix in crypto-js has been released #8626

github-actions · 2022-07-23T02:14:43Z

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.

hkjpotato added 2 commits July 20, 2021 19:20

deprecate crypto-js

3d485a9

update and fix the unit tests

f602c6c

hkjpotato changed the title ~~Kj/deprecate crypto js~~ deprecate crypto js Jul 21, 2021

hkjpotato changed the title ~~deprecate crypto js~~ deprecate crypto-js Jul 21, 2021

deprecate original WordArray, update typing

bd4404a

hkjpotato requested review from evcodes and elorzafe July 21, 2021 17:31

hkjpotato marked this pull request as ready for review July 21, 2021 17:32

hkjpotato requested a review from manueliglesias as a code owner July 21, 2021 17:32

evcodes reviewed Jul 21, 2021

View reviewed changes

hkjpotato mentioned this pull request Jul 22, 2021

upgrade crypto-js to 4.1.1 #8626

Merged

4 tasks

hkjpotato closed this Jul 22, 2021

github-actions bot locked as resolved and limited conversation to collaborators Jul 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deprecate crypto-js #8607

deprecate crypto-js #8607

hkjpotato commented Jul 20, 2021 •

edited

Loading

lgtm-com bot commented Jul 20, 2021

codecov-commenter commented Jul 21, 2021 •

edited

Loading

lgtm-com bot commented Jul 21, 2021

evcodes Jul 21, 2021

hkjpotato Jul 21, 2021 •

edited

Loading

evcodes Jul 21, 2021

hkjpotato Jul 21, 2021

evcodes Jul 21, 2021

evcodes Jul 21, 2021

hkjpotato Jul 21, 2021

evcodes Jul 21, 2021

hkjpotato Jul 21, 2021

evcodes Jul 21, 2021

hkjpotato Jul 21, 2021

hkjpotato commented Jul 22, 2021

github-actions bot commented Jul 23, 2022

deprecate crypto-js #8607

deprecate crypto-js #8607

Conversation

hkjpotato commented Jul 20, 2021 • edited Loading

Description of changes

Alternatives comparison

Issue #, if available

Description of how you validated changes

Checklist

lgtm-com bot commented Jul 20, 2021

codecov-commenter commented Jul 21, 2021 • edited Loading

Codecov Report

lgtm-com bot commented Jul 21, 2021

Choose a reason for hiding this comment

hkjpotato Jul 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkjpotato commented Jul 22, 2021

github-actions bot commented Jul 23, 2022

hkjpotato commented Jul 20, 2021 •

edited

Loading

codecov-commenter commented Jul 21, 2021 •

edited

Loading

hkjpotato Jul 21, 2021 •

edited

Loading