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: avoid potential default namespace collision #809

Merged

Conversation

ChALkeR
Copy link
Contributor

@ChALkeR ChALkeR commented Dec 14, 2021

Description:

protobufjs uses a package-wide global internal state for the protos in runtime, reachable at require('protobufjs').roots, and by default registers everything in the default root namespace:

> require('protobufjs').roots
{}
> Object.keys(require('protobufjs').roots)
[]
> require('@hashgraph/proto'), 0
0
> Object.keys(require('protobufjs').roots)
[ 'default' ]
> Object.keys(require('protobufjs').roots.default)
[ 'proto', 'google' ]
> Object.keys(require('protobufjs').roots.default.proto).slice(0, 5).join(', ')
'TokenUnitBalance, SingleAccountBalances, AllAccountBalances, ShardID, RealmID'

Among others, it creates e.g. Key, Duration, Timestamp protocols which are pretty generic names.

Now, the problem is in how the compiled code then references those:

const $root = $protobuf.roots["default"] || ($protobuf.roots["default"] = {});
exports.default = $root;

then

  proto.Key = function () {
    function Key(p) {
      if (p) for (var ks = Object.keys(p), i = 0; i < ks.length; ++i) if (p[ks[i]] != null) this[ks[i]] = p[ks[i]];
    }

    Key.prototype.contractID = null;
    Key.prototype.ed25519 = null;

and e.g.

    KeyList.encode = function encode(m, w) {
      if (!w) w = $Writer.create();

      if (m.keys != null && m.keys.length) {
        for (var i = 0; i < m.keys.length; ++i) $root.proto.Key.encode(m.keys[i], w.uint32(10).fork()).ldelim();
      }

      return w;
    };

Which means that while root is resolved to $protobuf.roots["default"] at load, specific protocols are then resolved against that root at runtime, so if something redefines e.g. require('protobufjs').roots.default.proto.Key, it will affect @hashgraph/proto.

Now, in a project that depends on @hashgraph/proto and e.g. another library depending on the same or compatible protobufjs version range could be used, and the package manager (e.g. yarn or npm) can deduplicate that, resulting in both @hashgraph/proto and another lib using the same shared require('protobufjs') object.

When using the default namespace root, the protocol namespaces might collide, causing unexpected effects, e.g. when that other protocol also defines a Key proto (or anything else that collides with this lib).

This PR changes the namespace from root to hashgraph, basically causing this exact diff in the compiled code:

diff lib.a/proto.js lib.b/proto.js
30c30
<     $root = $protobuf.roots["default"] || ($protobuf.roots["default"] = {});
---
>     $root = $protobuf.roots.hashgraph || ($protobuf.roots.hashgraph = {});

To reproduce:

  • checkout main, yarn && mv lib lib.a,
  • checkout this branch, yarn && mv lib lib.b,
  • ./node_modules/.bin/prettier --write lib.?/proto.js,
  • diff -r lib.a lib.b.

This should avoid potential protocol namespace collisions with another libs.

That said, it won't prevent namespace collisions if some setup ends up attempting to use two different versions of @hashgraph/proto, but that's likely to be a much less significant case. Ideally, not polluting (and not relying upon) the global state at all would have been ideal (like #269 did previously), but I'm unsure how to do that yet.
In the meantime, this should be just good enough.

Related issue(s):

Previously: #267, #268, #269.

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

Signed-off-by: Сковорода Никита Андреевич <chalkerx@gmail.com>
@sonarcloud
Copy link

sonarcloud bot commented Dec 14, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

@janaakhterov janaakhterov left a comment

Choose a reason for hiding this comment

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

Wow, this is an incredible write up, thank you; I was not aware of this issue. Using a default namespace of hashgraph makes sense to me.

@janaakhterov janaakhterov merged commit ea3a70d into hashgraph:main Dec 14, 2021
@janaakhterov
Copy link
Contributor

@ChALkeR Do you think we should maybe tie the namespace to the version of the protobufs? It'd be a bit of a hack, but would fix the namespace collision if someone were to use two different version of @hashgraph/proto. We can also just randomly generate a string for the namespace instead.

@ChALkeR
Copy link
Contributor Author

ChALkeR commented Dec 15, 2021

@danielakhterov Hm, that might be a good solution.
So far I haven't found an opt-out of this behavior (like was possible previously with protoc, though that had it's issues).

monokaijs pushed a commit to monostarter/u2u-sdk-js that referenced this pull request Jul 5, 2023
Signed-off-by: Сковорода Никита Андреевич <chalkerx@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants