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

Fix dependency vulnerability by upgrading xmlhttprequest-ssl via yarn.lock #10990

Merged
merged 1 commit into from
May 5, 2021

Conversation

danjm
Copy link
Contributor

@danjm danjm commented May 5, 2021

This fixes these code vulnerabilities:

Screenshot from 2021-05-05 11-18-20

@danjm danjm requested a review from a team as a code owner May 5, 2021 13:48
@danjm danjm requested a review from darkwing May 5, 2021 13:48
@github-actions
Copy link
Contributor

github-actions bot commented May 5, 2021

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@danjm
Copy link
Contributor Author

danjm commented May 5, 2021

Further comments on this fix:

  • Currently xmlhttprequest-ssl is at version 1.5.5 in our codebase
  • These are the commits between that and the version with the security fix (1.6.2):

Screenshot from 2021-05-04 23-13-44

Of those, only 4 appear to be potentially functional:
mjwwit/node-XMLHttpRequest#6
mjwwit/node-XMLHttpRequest@bf53329
mjwwit/node-XMLHttpRequest@6a0a91d
mjwwit/node-XMLHttpRequest@ee1e81f

I think all of these will be non-breaking for engine-io.client:

Even if my read of all of those is incorrect, I am 99% sure that 3box's use of ipfs does not rely on libp2p-webrtc-star. 3box creates an ipfs instance, which in turn instantiates libp2p-webrtc-star. However, 3box only uses the following properties/methods on these ipfs instances:
ipfs.swarm
ipfs.id
ipfs.dag
ipfs.stop
ipfs.pubsub
ipfs.on
ipfs.add

And none of these rely on libp2p-webrtc-star

It also seems safe for eth-trezor-keyring, which only relies on the build-tx module of hd-wallet, which does not rely on socket.io-client (but that gets imported because the whole of hd-wallet is imported)

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.

LGTM!

@metamaskbot
Copy link
Collaborator

Builds ready [1850b7b]
Page Load Metrics (629 ± 60 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint498667105
domContentLoaded39984162812460
load40184262912460
domInteractive39984062812460

@danjm danjm merged commit 838fe95 into develop May 5, 2021
@danjm danjm deleted the xmlhttprequestssl-version-fix branch May 5, 2021 14:32
@github-actions github-actions bot locked and limited conversation to collaborators May 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants