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

Freeze Promise global on boot #7309

Merged
merged 12 commits into from
Oct 24, 2019

Conversation

rekmarks
Copy link
Member

@rekmarks rekmarks commented Oct 23, 2019

Per a security advisory from an audit, freezes the Promise global and prevents its reassignment on boot, in both the background and UI.

  • Pins the version of deep-freeze-strict as we use it to freeze the Promise, and the point here is to prevent certain supply chain attacks.

  • Adds new tests (test:unit:global) that are run in CI. These tests must be separate as freezing the Promise global breaks our normal test environments (at least the unit tests).

  • Removes tape from devDependencies, as we were not actually using that for anything.

Marked as needs QA since if a dependency modifies the Promise global, an error will be thrown that they probably won't catch. I imagine they'll usually do something like that in their entry file, but you never know.

@Gudahtt
Copy link
Member

Gudahtt commented Oct 23, 2019

Interesting - I wonder if we still need to mutate the Promise global in tests. It looks like it was done to enable catching of uncaught exceptions, but V8 has gotten better at that since it was written 🤔

@rekmarks
Copy link
Member Author

rekmarks commented Oct 23, 2019

Interesting - I wonder if we still need to mutate the Promise global in tests.

When I attempted to run this as a normal unit test, the error seemed to come from a test dependency (bluebird):

.../metamask-extension_feature/node_modules/bluebird/js/release/queue.js:38
    this[(j + 0) & wrapMask] = fn;
                             ^
TypeError: Cannot add property 6, object is not extensible
    at Queue.push (.../metamask-extension_feature/node_modules/bluebird/js/release/queue.js:38:30)

@Gudahtt

Edit: ah, the point of Bluebird in the testing environment is to help with the uncaught exceptions?

@Gudahtt
Copy link
Member

Gudahtt commented Oct 23, 2019

Yes, exactly. It's setup in test/helper.js in the enableFailureOnUnhandledPromiseRejection function

@rekmarks
Copy link
Member Author

Yes, exactly. It's setup in test/helper.js in the enableFailureOnUnhandledPromiseRejection function

We overwrite the global Promise with bluebird in that function. Removing this overwrite causes unit tests to hang on my machine. We should probably figure that one out at some point and freeze the Promise global in tests but I don't have time at the moment and I think it should be dealt with in a separate PR.

@rekmarks rekmarks requested a review from danfinlay October 23, 2019 17:29
@rekmarks rekmarks requested a review from Gudahtt October 24, 2019 11:52
}

if (value !== undefined) {
target[key] = deepFreeze()
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing this was meant to be

Suggested change
target[key] = deepFreeze()
target[key] = deepFreeze(target[key])

In which case it could just be taken out of the condition altogether, as both branches are identical.

Copy link
Member

@Gudahtt Gudahtt Oct 24, 2019

Choose a reason for hiding this comment

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

Wait, no.... maybe it should be

Suggested change
target[key] = deepFreeze()
opts.value = deepFreeze(value)

As target[key] is getting overwritten 🤦‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha, yes, the latter. Now shortened to: opts.value = deepFreeze(value)

It's my facepalm, not yours!

Gudahtt
Gudahtt previously approved these changes Oct 24, 2019
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

Looks good!

@Gudahtt Gudahtt dismissed their stale review October 24, 2019 13:00

The lockfile has a conflict

@rekmarks rekmarks merged commit 478d656 into MetaMask:develop Oct 24, 2019
@rekmarks rekmarks deleted the prevent-promise-overwrite branch October 24, 2019 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants