dxos / codec-protobuf

GNU Affero General Public License v3.0
2 stars 2 forks source link

bytes decoded as Buffer fails in browser #5

Closed dboreham closed 4 years ago

dboreham commented 4 years ago

Found this while debugging credentials code in the browser:

We make messages with keys, signatures etc that have JS Object fields with type Uint8Array. When these fields are sent through protobufjs they come back decoded as a kind of Buffer object with fields :

{
  type: 'Buffer',
  data: <int array>
}

Our code proceeds to use these values in various ways assuming that they are compatible with the default Buffer implementation which in the case of the browser runtime is this one.

Unfortunately this doesn't work. For example the function Buffer.isBuffer(something we think is a Buffer) returns false. This is because the Buffer objects created by protobufjs lack the _isBuffer field that the browser Buffer implementation adds to objects it creates.

I suspect that our code all works in Node.JS because all the Buffer objects are created by and used by the native Buffer implementation.

dboreham commented 4 years ago

Looks like the right thing should be happening here: https://github.com/dxos/codec-protobuf/blob/master/src/codec-protobuf.js#L8

Will debug to see why this isn't working for me.

dboreham commented 4 years ago

Looks like perhaps we are depending on the wrong version of @dxos/codec-protobuf?

The workspaces depend on the beta tagged releases but the hoisted version of 1.0.0, which is actually older than the latest beta2 (and doesn't inject the browser Buffer class).

yarn why v1.19.2
[1/4] Why do we have the module "@dxos/codec-protobuf"...?
[2/4] Initialising dependency graph...
[3/4] Finding dependency...
[4/4] Calculating file sizes...
=> Found "@dxos/codec-protobuf@1.0.0"
info Has been hoisted to "@dxos/codec-protobuf"
info Reasons this module exists
   - "workspace-aggregator-71a6c710-9a80-41c8-9d86-1bbc4796d141" depends on it
   - Hoisted from "_project_#@wirelineio#data-client#@dxos#feed-store#@dxos#codec-protobuf"
   - Hoisted from "_project_#@wirelineio#data-client#@wirelineio#protocol#@dxos#codec-protobuf"
   - Hoisted from "_project_#@wirelineio#data-client#@wirelineio#protocol#@wirelineio#broadcast#@dxos#codec-protobuf"
info Disk size without dependencies: "76KB"
info Disk size with unique dependencies: "16.17MB"
info Disk size with transitive dependencies: "18.19MB"
info Number of shared dependencies: 17
=> Found "@wirelineio/credentials#@dxos/codec-protobuf@1.1.0-beta.2"
info This module exists because "_project_#@wirelineio#credentials" depends on it.
info Disk size without dependencies: "5.57MB"
info Disk size with unique dependencies: "21.82MB"
info Disk size with transitive dependencies: "23.88MB"
info Number of shared dependencies: 21
=> Found "@dxos/echodb#@dxos/codec-protobuf@1.1.0-beta.2"
info This module exists because "_project_#@wirelineio#playground#@dxos#echodb" depends on it.
info Disk size without dependencies: "5.57MB"
info Disk size with unique dependencies: "21.82MB"
info Disk size with transitive dependencies: "23.88MB"
info Number of shared dependencies: 21
=> Found "@wirelineio/data-client#@dxos/codec-protobuf@1.1.0-beta.2"
info Reasons this module exists
   - "_project_#@wirelineio#data-client#@dxos#echodb" depends on it
   - Hoisted from "_project_#@wirelineio#data-client#@dxos#echodb#@dxos#codec-protobuf"
info Disk size without dependencies: "5.57MB"
info Disk size with unique dependencies: "21.82MB"
info Disk size with transitive dependencies: "23.88MB"
info Number of shared dependencies: 21
dboreham commented 4 years ago

Workaround:

const deepMapBufferValues = o => {
  Object.keys(o).forEach(k => {
    const v = o[k];
    // We need to catch both Objects and Arrays, so use isObject() first
    if (isObject(v)) {
      if (isPlainObject(v) && Object.prototype.hasOwnProperty.call(v, 'type') && v.type === 'Buffer') {
        o[k] = Buffer.from(v);
      } else {
        deepMapBufferValues(v);
      }
    }
  });
};
dboreham commented 4 years ago

btw I'm also seeing this in code running in Node.js, under jest.

dboreham commented 4 years ago

Reproduction PR with fix from tinchoz49 https://github.com/wirelineio/incubator/pull/160

dboreham commented 4 years ago

Merged fix into feature-auth

dboreham commented 4 years ago

Re-tested in browser environment : problem is fixed there too by @tinchoz49 fix.