replace momentjs with date-fns #11062

cezaraugusto · 2017-09-21T07:49:35Z

Initially, the plan was to reduce bundle cost given moment is enormous and we only use 2-3 methods from there:

What came to my attention regarding security was an issue created in moment's repo explaining that a given regular expression could be used to parse dates specified as strings being vulnerable to ReDoS. I first saw this in a Medium post from Node Security

This, in fact, didn't remove moment entirely as we still require Joi which has it as a dependency but may be a good starting point.

Fix #11061

Test plan:

This affected bookmarks exporter and ledger.

The general test plan is that you can still export a bookmark (with the same file name as before) and ledger/contribution statement should have their times as usual.

Ledger panel and ledger export util tests should pass too. npm run unittest should cover.

/cc @evq @darkdh

NejcZdovc · 2017-09-21T08:05:38Z

please don't merge this one until #11037 is done. Because any code change to ledger will impact delivery time for it

codecov-io · 2017-09-21T08:10:38Z

Codecov Report

Merging #11062 into master will decrease coverage by <.01%.
The diff coverage is 76.92%.

@@            Coverage Diff             @@
##           master   #11062      +/-   ##
==========================================
- Coverage   56.25%   56.25%   -0.01%     
==========================================
  Files         280      280              
  Lines       27516    27511       -5     
  Branches     4486     4486              
==========================================
- Hits        15479    15475       -4     
+ Misses      12037    12036       -1

Flag	Coverage Δ
#unittest	`56.25% <76.92%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
...renderer/components/preferences/payment/history.js	`34.17% <ø> (-3.18%)`	⬇️
app/common/lib/ledgerExportUtil.js	`95.72% <100%> (ø)`	⬆️
...r/components/preferences/payment/enabledContent.js	`75.23% <100%> (ø)`	⬆️
app/browser/api/ledger.js	`56.52% <50%> (ø)`	⬆️
app/browser/bookmarksExporter.js	`80.3% <50%> (ø)`	⬆️
app/common/lib/ledgerUtil.js	`97.57% <80%> (+0.47%)`	⬆️

cezaraugusto · 2017-09-23T21:29:00Z

blocked until 0.19.x is shipped

@diracdeltas

- browser-laptop has PR to remove dependency: #11062 - bat-* libraries should stop using moment Auditors: @diracdeltas, @evq

@diracdeltas

- browser-laptop has PR to remove dependency: #11062 - bat-* libraries should stop using moment Auditors: @diracdeltas, @evq

@diracdeltas

- browser-laptop has PR to remove dependency: #11062 - bat-* libraries should stop using moment Auditors: @diracdeltas, @evq

bsclifton · 2017-11-27T21:38:00Z

@cezaraugusto a node security alert came up today:
https://nodesecurity.io/advisories/532

I skipped this for now, per comment by @diracdeltas:
master 4e6c056
0.21.x c65f142
0.20.x ffe955e
0.19.x 648ce3b

Would you be able to rebase this please? 😄

bsclifton · 2017-11-27T21:46:00Z

app/renderer/components/preferences/payment/enabledContent.js

@@ -141,7 +141,7 @@ class EnabledContent extends ImmutableComponent {
    let l10nDataId = 'statusNextReconcileDate'

    if (!nextReconcileDateRelative) {
-      nextReconcileDateRelative = formattedDateFromTimestamp(moment().add(1, 'months'), 'MMMM Do')
+      nextReconcileDateRelative = formattedDateFromTimestamp(addDays(1, 30), 'MMMM Do')


This is a change in logic, right? Since February only has 28 days for example. Can you use the add months method instead?
https://date-fns.org/v1.29.0/docs/addMonths

bsclifton

Comments left- otherwise looks good 😄 (month issue, needs rebase)

Once those issues are addressed I can manually test and confirm by removing the NSP alert skip for https://nodesecurity.io/advisories/532 (to make sure browser-laptop is clear)

cezaraugusto · 2017-12-19T01:58:36Z

app/renderer/components/preferences/payment/enabledContent.js

@@ -176,7 +176,7 @@ class EnabledContent extends ImmutableComponent {
    let l10nDataId = 'statusNextReconcileDate'

    if (!nextReconcileDateRelative) {
-      nextReconcileDateRelative = formattedDateFromTimestamp(moment().add(1, 'months'), 'MMMM Do')
+      nextReconcileDateRelative = formattedDateFromTimestamp(addMonths(Date.now(), 1), 'MMMM Do')


this now adds 1 month starting from the current date cc @bsclifton

cezaraugusto · 2017-12-19T22:02:49Z

rebased, ready for review again

Test Plan: npm run test -- --grep="ledger export utilities test"

@bsclifton

auditors: @bsclifton

bsclifton · 2018-02-08T17:49:27Z

app/browser/api/ledger.js

@@ -995,7 +995,7 @@ const pageDataChanged = (state, viewData = {}, keepInfo = false) => {
 }

 const backupKeys = (state, backupAction) => {
-  const date = moment().format('L')
+  const date = format(new Date(), 'MM/DD/YYYY')


Most of the world uses DD/MM/YYYY format... does moment().format('L') always return the date in MM/DD/YYYY format? or does it respect the user's locale

yes, the 'L' parameter forces month numeral/day of month/year.

ref: https://momentjs.com/docs/#/manipulating/local/ and search for "Localized formats".

bsclifton · 2018-02-08T17:51:07Z

test/about/ledgerPanelAdvancedPanelTest.js

@@ -91,7 +91,7 @@ let generateAndSaveRecoveryFile = function (recoveryFilePath, passphrase) {
  let recoveryFileContents = ''

  if (typeof passphrase === 'string') {
-    const date = moment().format('L')
+    const date = format(new Date(), 'MM/DD/YYYY')


same concern as above- although, I know this is just a test 😄

yes same as above answer

bsclifton

Concern raised in comment- but otherwise, looks great! I think with our new process, we'll want to do a security review. I'll create one real quick and link it 😄

bsclifton · 2018-02-08T17:54:52Z

Security review created here:
https://github.com/brave/internal/issues/202

bsclifton · 2018-02-13T20:55:46Z

@cezaraugusto security review completed by @flamsmark 😄 See my comments above (regarding the ordering of MM DD). If no changes are needed, we're ready for a merge 😄 👍

bsclifton

++

cezaraugusto added perf PR/needs-QA-attention ☕ sec-low security labels Sep 21, 2017

cezaraugusto added this to the 0.21.x (Nightly Channel) milestone Sep 21, 2017

cezaraugusto self-assigned this Sep 21, 2017

cezaraugusto requested review from bsclifton, evq, NejcZdovc and darkdh September 21, 2017 07:49

cezaraugusto mentioned this pull request Sep 21, 2017

replace moment with date-fns for date helpers #11061

Closed

cezaraugusto added the PR/blocked label Sep 23, 2017

luixxiul added the PR/needs-rebase ۞ label Oct 3, 2017

bbondy modified the milestones: 0.21.x (Developer Channel), 0.20.x (Beta Channel), Backlog Oct 25, 2017

luixxiul added the QA/test-plan-specified label Oct 31, 2017

bsclifton added a commit that referenced this pull request Nov 27, 2017

Skip momentjs node security alert for now

c65f142

- browser-laptop has PR to remove dependency: #11062 - bat-* libraries should stop using moment Auditors: @diracdeltas, @evq

bsclifton added a commit that referenced this pull request Nov 27, 2017

Skip momentjs node security alert for now

ffe955e

- browser-laptop has PR to remove dependency: #11062 - bat-* libraries should stop using moment Auditors: @diracdeltas, @evq

bsclifton added a commit that referenced this pull request Nov 27, 2017

Skip momentjs node security alert for now

648ce3b

- browser-laptop has PR to remove dependency: #11062 - bat-* libraries should stop using moment Auditors: @diracdeltas, @evq

bsclifton reviewed Nov 27, 2017

View reviewed changes

bsclifton suggested changes Nov 27, 2017

View reviewed changes

cezaraugusto force-pushed the remove-moment branch from b29c9b8 to 44ce177 Compare December 19, 2017 01:56

cezaraugusto commented Dec 19, 2017

View reviewed changes

cezaraugusto removed the PR/blocked label Dec 19, 2017

cezaraugusto removed the PR/needs-rebase ۞ label Dec 19, 2017

bsclifton modified the milestones: Triage Backlog, 0.23.x (Nightly Channel) Feb 8, 2018

cezaraugusto added 9 commits February 8, 2018 10:45

add date-fns dep

bb73a11

replace moment in ledgerexportutil

2cdc207

Test Plan: npm run test -- --grep="ledger export utilities test"

replace moment in advanced panel tests (skipped)

81fe4ac

replace moment in contrib statement

b1a0df4

replace moment in bm exporter

b62210d

replace moment in ledgerUtil

0e3f8a3

replace moment in ledger api

e451d57

remove moment call in ledger history

371babe

replace moment in enabledContent

f6aad45

auditors: @bsclifton

bsclifton force-pushed the remove-moment branch from 44ce177 to f6aad45 Compare February 8, 2018 17:45

bsclifton reviewed Feb 8, 2018

View reviewed changes

bsclifton approved these changes Feb 14, 2018

View reviewed changes

bsclifton merged commit 5edb21e into master Feb 14, 2018

bsclifton deleted the remove-moment branch February 14, 2018 16:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace momentjs with date-fns #11062

replace momentjs with date-fns #11062

cezaraugusto commented Sep 21, 2017 •

edited by bsclifton

NejcZdovc commented Sep 21, 2017

codecov-io commented Sep 21, 2017 •

edited

cezaraugusto commented Sep 23, 2017

bsclifton commented Nov 27, 2017 •

edited

bsclifton Nov 27, 2017

cezaraugusto Dec 19, 2017

bsclifton left a comment •

edited

cezaraugusto Dec 19, 2017

cezaraugusto commented Dec 19, 2017

bsclifton Feb 8, 2018

cezaraugusto Feb 14, 2018

bsclifton Feb 8, 2018

cezaraugusto Feb 14, 2018

bsclifton left a comment

bsclifton commented Feb 8, 2018

bsclifton commented Feb 13, 2018

bsclifton left a comment

replace momentjs with date-fns #11062

replace momentjs with date-fns #11062

Conversation

cezaraugusto commented Sep 21, 2017 • edited by bsclifton

Test plan:

NejcZdovc commented Sep 21, 2017

codecov-io commented Sep 21, 2017 • edited

Codecov Report

cezaraugusto commented Sep 23, 2017

bsclifton commented Nov 27, 2017 • edited

bsclifton Nov 27, 2017

Choose a reason for hiding this comment

cezaraugusto Dec 19, 2017

Choose a reason for hiding this comment

bsclifton left a comment • edited

Choose a reason for hiding this comment

cezaraugusto Dec 19, 2017

Choose a reason for hiding this comment

cezaraugusto commented Dec 19, 2017

bsclifton Feb 8, 2018

Choose a reason for hiding this comment

cezaraugusto Feb 14, 2018

Choose a reason for hiding this comment

bsclifton Feb 8, 2018

Choose a reason for hiding this comment

cezaraugusto Feb 14, 2018

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

bsclifton commented Feb 8, 2018

bsclifton commented Feb 13, 2018

bsclifton left a comment

Choose a reason for hiding this comment

cezaraugusto commented Sep 21, 2017 •

edited by bsclifton

codecov-io commented Sep 21, 2017 •

edited

bsclifton commented Nov 27, 2017 •

edited

bsclifton left a comment •

edited