Breaking change in minor version 3.1.0

[dholms]

Hey there, I wanted to point out that minor version 3.1.0 included breaking changes.

Specifically: "If globalThis.Buffer is defined, it will be used in preference to globalThis.Uint8Array."

Our code is standardized around Uint8Arrays. This is important for apis such as Webcrypto which expect Uint8Arrays or ArrayBuffers and don't work correctly on Node buffers.

I think this is a misleading change as this is a uint8arrays library not a BufferLike library. Concatenating two Uint8Arrays and receiving a Buffer back breaks developer expectations.

However, if this change does stick around, I would encourage a major version bump.

[hugomrdias]

@achingbrain we have exactly the same issue

[achingbrain]

The node Buffer class extends Uint8Array so the interface contract is maintained hence there was no major version bump. Uint8Arrays are also slower to initialise and less memory efficient than node Buffers.

This is important for apis such as Webcrypto which expect Uint8Arrays or ArrayBuffers and don't work correctly on Node buffers.

Can you share a simple example of it not working correctly please?

[dholms]

sure thing, here's one I put together:

import * as uint8arrays from 'uint8arrays'
import { webcrypto } from 'crypto'

const toSign = uint8arrays.fromString('sign me', 'utf-8')

const keypair = await webcrypto.subtle.generateKey(
  { name: 'ECDSA', namedCurve: 'P-256' },
  true,
  ['sign', 'verify'],
)

const sig = await webcrypto.subtle.sign(
  { name: 'ECDSA', hash: { name: 'SHA-256' } },
  keypair.privateKey,
  toSign.buffer,
)

const toVerify = uint8arrays.fromString(uint8arrays.toString(toSign, 'utf-8'), 'utf-8')
const isValid = await webcrypto.subtle.verify(
  { name: 'ECDSA', hash: { name: 'SHA-256' } },
  keypair.publicKey,
  sig,
  toVerify.buffer,
)

console.log("Is Valid: ", isValid)

This return true on version 3.0.0 and false on 3.1.0.

[achingbrain]

The problem here is you are passing the underlying ArrayBuffer into webcrypto.subtle.sign and webcrypto.subtle.verify without reference to any potential .byteOffeset or .byteLength.

MDN says:

Because a typed array is a view of a buffer, the underlying buffer may be longer than the typed array itself.

It's dangerous to assume they are not because you even if you had no references to node Buffers anywhere you could have been passed the output from .subarray:

const buf = new TextEncoder().encode('sign me please')
const sub = arr.subarray(0, 7)

console.info(new TextDecoder().decode(buf)) // sign me
console.info(sub.byteLength) // 7
console.info(sub.buffer.byteLength) // 14

If you instead pass toSign and toVerify in, it will print true.

I think we should still merge #39 as Buffers are not 100% compatible with Uint8Arrays (see Buffer.slice) but your code above will still print false due to it ignoring the .byteOffset and .byteLength values.

[achingbrain]

@hugomrdias are you seeing the same issue or something different?

[dholms]

Thank you for the feedback on that. I believe that WebCrypto used to require ArrayBuffers but has since updated to allow TypedArrays. I've updated our code to pass in the Uint8Array instead of the underlying ArrayBuffer 👍

[hugomrdias]

39 fixes my issues so all good

Ty

[nazarhussain]

Same issue for me. It's introduced in following PR #36. The function fromString should not return any type other than Uint8Array. If it is the then it should be published as major release to mark a breaking change.

[achingbrain]

@nazarhussain node Buffers are Uint8Arrays:

% node
Welcome to Node.js v16.13.0.
Type ".help" for more information.
> const { fromString } = await import('uint8arrays')
undefined
> fromString('derp') instanceof Uint8Array
true

Can you please share a bit more about the issue you are seeing? What is the error message and how do I replicate it?

[nazarhussain]

@achingbrain Yes I agree to you to the level of the nodejs environment. But I believe the extended ecosystem is not yet ready. Uint8Array and Buffer are treated as different types.

Check this script live at https://runkit.com/nazarhussain/6332f59483f5b70008db9d8f For reference.

const sinon = require("sinon"); 
const assert = require("assert"); 

var object = {
  test(input) {
    // console.log(input);
  },
};

var spy = sinon.spy(object, "test");

object.test("String"); 
assert(spy.calledWith("String"));
spy.resetHistory();

object.test(Buffer.from([1]))
assert(spy.calledWith(Buffer.from([1])));
spy.resetHistory();

object.test(new Uint8Array([1]))
assert(spy.calledWith(new Uint8Array([1])));
spy.resetHistory();

object.test(Buffer.from([1]))
assert(spy.calledWith(new Uint8Array([1])), "Uint8Array is not considered as Buffer");
spy.resetHistory();

[achingbrain]

Uint8Array and Buffer are treated as different types.

This is because they are different types - sinon's matchers fail because the class name of Uint8Array and Buffer are different, which is completely expected.

In your test I think you're trying to assert that the contents of the arguments was the same, in which case you should compare the contents of the arguments:

const { equals } = require('uint8arrays/equals')

object.test(Buffer.from([1]))
assert(equals(spy.getCall(0).args[0], new Uint8Array([1]), "Not equal"));

[nazarhussain]

I believe you explained it yourself.

This is because they are different types

I am just stating as these are different types and can cause trouble across different projects in the eco-system, so should be considered as a breaking change. e.g. I updated the package and a ton of my test using sinon .calledWith starts failing and in most of the cases it's very tedious to use uint8arrays/equals. As sinon is a widely used framework, consider how many projects could affect by this change.

[achingbrain]

They have different class names so they are different types in the way that Object and Number are different types.