Replace old xhr2-cookies with xhr2-cookies-patched deps for 1.x, fix User-Agent header problem

[ghost]

Description

Unlike ethers.js, web3.js didn't support using custom User-Agent headers due to header restrictions inherited from the old, unmaintained xhr2-cookies dependencies.

Related issues:

#1803
#2158

Using old dependency would expose the OS and nodejs version to RPC providers.

Due to this problem, there are many forks of the old dependencies exist https://www.npmjs.com/package/xhr2-cookies-web3fix , only to fix this old restriction applied.

See also

souldreamer/xhr2-cookies#30

Type of change

Replace xhr2-cookies with https://www.npmjs.com/package/xhr2-cookies-patched (https://github.com/0xAyanami/xhr2-cookies.git)

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

• I have selected the correct base branch.
• I have performed a self-review of my own code.
• I have commented my code, particularly in hard-to-understand areas.
• I have made corresponding changes to the documentation.
• My changes generate no new warnings.
• Any dependent changes have been merged and published in downstream modules.
• I ran npm run dtslint with success and extended the tests and types if necessary.
• I ran npm run test:unit with success.
• I ran npm run test:cov and my test cases cover all the lines and branches of the added code.
• I ran npm run build and tested dist/web3.min.js in a browser.
• I have tested my code on the live network.
• I have checked the Deploy Preview and it looks correct.
• I have updated the CHANGELOG.md file in the root folder.

[nazarhussain]

@0xayanami Thanks for the PR. We will review it further.

BTW there are 8 commits differences between the original package master branch and your's but your relevant PR only had two commits. Anything particular you didn't push to upstream?

[ghost]

@nazarhussain Hello, feel free to review changes from the patched dependency that I have published on NPM.

Commits for library published on npm

souldreamer/xhr2-cookies@master...0xAyanami:master

Updated old cookiejar depencency

https://github.com/0xAyanami/xhr2-cookies/commit/e1ab976108383fba6e9155eff06556ac8072dc14

Lifted up restriction on User-Agent headers

https://github.com/0xAyanami/xhr2-cookies/commit/520537e6dba8dfd15948a5c5b3b9276f8b3e4498

Published dist folder for direct use of dependency from github

https://github.com/0xAyanami/xhr2-cookies/commit/5a9e8f480d24b62bc3e43647b5105db2f17f9cc4

initializing buffers in a non-obsolete way, remove warning for deprecation

https://github.com/0xAyanami/xhr2-cookies/commit/e4e357c6e4afbc88833beb36bd1cb8dd5debdab1

Original PR (souldreamer/xhr2-cookies#29)

Fixed Test Suite & Updated dist from previous commit

https://github.com/0xAyanami/xhr2-cookies/commit/0e336f477b2a697688bfdf9494960626ba2558bb

Updated self signed certificate for local tests

https://github.com/0xAyanami/xhr2-cookies/commit/3d3b1e5f1758ce9030a13a42d4a1e611d55c1c7b
https://github.com/0xAyanami/xhr2-cookies/commit/61f93422632e0d63d4300de33aba5e3e73c59d81

Changed naming of library to publish on npm

https://github.com/0xAyanami/xhr2-cookies/commit/f4fc83c700758b6b76aa56d815488b3d1a701e73

Most of them are misc fix or removal of deprecation warning, fixing broken test suites as well.

[ghost]

Added commit to fix the unittest, looks like it would work as normal again.

[coveralls]

Pull Request Test Coverage Report for Build 2003763047

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 429 unchanged lines in 7 files lost coverage.
• Overall coverage decreased (-2.3%) to 72.21%

---

Files with Coverage Reduction	New Missed Lines	%
packages/web3-core-requestmanager/src/jsonrpc.js	1	88.0%
packages/web3-core-helpers/src/formatters.js	25	81.07%
packages/web3-core-helpers/src/errors.js	31	4.41%
packages/web3-utils/src/soliditySha3.js	55	5.13%
packages/web3-utils/src/index.js	62	29.31%
packages/web3-utils/src/utils.js	92	12.86%
packages/web3-eth-accounts/src/index.js	163	23.77%

Totals
Change from base Build 2002669572:	-2.3%
Covered Lines:	3368
Relevant Lines:	4396

---

💛 - Coveralls

[nazarhussain]

@0xayanami Please update the CHANGELOG.md with the description of updating that dependency.

[ghost]

@nazarhussain I have updated the CHANGELOG.md, please check.

[spacesailor24]

This PR has immense security implications and should not be merged as is. @0xayanami controls the package xhr2-cookies-patched and could update it arbitrarily after we merge this PR. If we'd like to implement this change, I think ChainSafe would need to publish the patched version of the package

[ghost]

@spacesailor24 Yes, that is what I was talking on the previous closed PR #4796. It would be great for ChainSafe to deploy the package.

However, as the dependency have fixed version, and since the NPM's policy is not to allow republishing on the same version https://stackoverflow.com/questions/27873515/is-there-a-workaround-for-npm-publish-f, would it be impossible to update it arbitrarily?

[ghost]

@spacesailor24 @nazarhussain @luu-alex Perhaps this PR could address your concerns #4856 , could you please check out?

[spacesailor24]

@spacesailor24 Yes, that is what I was talking on the previous closed PR #4796. It would be great for ChainSafe to deploy the package.

However, as the dependency have fixed version, and since the NPM's policy is not to allow republishing on the same version https://stackoverflow.com/questions/27873515/is-there-a-workaround-for-npm-publish-f, would it be impossible to update it arbitrarily?

Just to keep you in the loop, we're having an internal discussion about this, but this is my opinoin:

I think if we use a static version in package.json for xhr2-cookies-patched, my concerns are mitigated, but there will always be a threat if/when the package releases a new version

The big difference between xhr2-cookies and xhr2-cookies-patched is the former has 377,857 weekly downloads, meaning 377,857 people are testing that the code works as expected every week and doesn't perform any malicious behavior. On the contrary, xhr2-cookies-patched has only ever been used by the user proposing the changes. If our tests pass, then it lessens the concern that their package will break something, but we can't ever get rid of the security threat that comes with using a package forked by someone whom doesn't have a track record for maintaining the package

We can review the changes from xhr2-cookies to xhr2-cookies-patched, which seems to be doing what the user is saying it does, and we can hardcode that version in our package.json. However, maintainers of Web3.js must now be explicitly aware of the potential security vulnerability of this particular package if a new version is ever released and it's suggested we update it. This is my biggest concern as it could be very easy for a maintainer to assume this package is like any of our other dependencies, and update the package version to a malicious one without vetting that changes made between versions. Even if the differences between the current and proposed version are reviewed, malicious JavaScript can be greatly obscured which means the maintainer must have a pretty good idea of how the package works to determine whether or not the code changes are relevant to the package

Am I being paranoid? Yes, but Web3.js has 540k+ weekly downloads and each of those users could completely boycott the project (and even ChainSafe) because we make a decision like this and introduce a vulnerability that does something as drastic stealing private keys

---

@0xayanami it's not that you would do this, but it's that you could do this and there's even a possibility that your account gets compromised and someone else does it. I just think concerns like these should be discussed and understood by everyone before moving forward, especially because of the work this project does and the amount of access a hacker could obtain by exploiting Web3.js

[ghost]

@spacesailor24 So to simplify your concerns,

1. xhr2-cookies has a history of download and tests while xhr2-cookies-patched is not

2. Although the static, pinned version of npm package could not be modified however there is a chance that I could publish a newer version and create another pull request that could potentially introduce new, unauthorized changes to the project.

I could rephrase this with

1. xhr2-cookies is an unmaintained, unused fork of xhr2 package that is only being used by 1.x of web3-providers-http and its various forks used for private projects

As you can check here https://www.npmjs.com/package/xhr2-cookies, the library is only being used by the variant of web3 and I would assume that no one ever tested or thoroughly audit the code since when I tried to test the library using the built-in test cases, it was already broken so I had no choice but to fix them to make it work again

https://github.com/0xAyanami/xhr2-cookies/commit/e1ab976108383fba6e9155eff06556ac8072dc14

https://github.com/0xAyanami/xhr2-cookies/commit/e4e357c6e4afbc88833beb36bd1cb8dd5debdab1

https://github.com/0xAyanami/xhr2-cookies/commit/0e336f477b2a697688bfdf9494960626ba2558bb

https://github.com/0xAyanami/xhr2-cookies/commit/3d3b1e5f1758ce9030a13a42d4a1e611d55c1c7b

https://github.com/0xAyanami/xhr2-cookies/commit/61f93422632e0d63d4300de33aba5e3e73c59d81

These are the changes to make the library work properly again, so your term of 'xhr2-cookies' have been tested by various projects are wrong.

So, 'The big difference between xhr2-cookies and xhr2-cookies-patched is the former has 377,857 weekly downloads, meaning 377,857 people are testing that the code works as expected every week and doesn't perform any malicious behavior.' - this would be a false statement since nobody actually checked out the code.

2. I have already tried to contact the original authors of the project to update those codes, but due to the on going assault on Ukraine and given that the authors live inside or closer to Ukraine, I believe it couldn't be done somehow.

4. Leaving users to use unmaintained, improper code since it is the best behavior to protect codes, is not a good cause. The main reason that I am pushing those changes is that Web3 has a long history to leak privacy of user's information to remote rpc providers like infura, alchemy, etc by not updating this outdated 'xhr2-cookies' library for a long time since it was introduced.

https://github.com/0xAyanami/xhr2-cookies/commit/520537e6dba8dfd15948a5c5b3b9276f8b3e4498#diff-f0d05cf99bfd0dfe2ca402e8f899c21258425956bf96219462fb4ec3d5a0ac7cL102

private _userAgent = `Mozilla/5.0 (${os.type()} ${os.arch()}) node.js/${process.versions.node} v8/${process.versions.v8}`;

As you could see this line of code, exposes any kind of information that could be used to easily identify which device, which development environment is being used for individual dapp users, developers only because it uses web3.js.

To mitigate this, as a privacy project contributor I decided to use the modified wrapper of web3.js provider https://github.com/tornadocash/tornado-cli/tree/master/local_modules/web3-providers-http but I don't think it should be a practice for everyone using Web3.js.

They have the rights to conceal their privacy, and if ChainSafe does care about users' privacy, the best behavior would be to help to stop the leak instead of being paranoid about something unlikely to happen.

Why would someone need to giveaway their device information for free just because they interact with a project that integrated web3.js?

@spacesailor24 Again, could you be paranoid for not only the project development, but for the user's privacy as well? Do we really want to giveaway the privacy of millions of DeFi users to node providers for free?

I am not blaming for being paranoid for the unwanted changes, but as I believe that as long as your maintainer's audit for proper changes care for user's privacy it wouldn't hurt the reputation of anyone.

And due to the fact that you are already using third-party dependencies for web3.js as well as that xhr2-cookies is a third party library itself, just that criticizing on a single PR without reviewing changes wouldn't make such differences.

Being careful of security is okay and it is applaudable, but it doesn't justify not to review changes and block the changes that could fix the privacy for millions.

[ghost]

Also, some incident like node-ipc https://gist.github.com/MidSpike/f7ae3457420af78a54b38a31cc0c809c happened not because people used the dynamic version statement of node-ipc, but because people actually trusted their dependency would not cause a problem just because many people is using it.

Not only did people using dynamic versioning of node-ipc got in trouble but also people using the linked package of 'node-ipc' got in trouble as well, even if you use the static version, your npm package manager will still refer to the dynamic version of the package that is defined as a dependency of that static versioned dependency, it is called as supply chain attack as well.

And btw, have you also audited every change of the updated projects yesterday that you give approval of? #4860

'Web3.js' already use tons of third party package with dynamic versioning https://raw.githubusercontent.com/ChainSafe/web3.js/695637798ad6b440b32dbad16f9ca301e4c3b5df/package-lock.json

and any incident like node-ipc could happen anytime from them, not this package.