dmonad / lib0

Monorepo of isomorphic utility functions
MIT License
345 stars 62 forks source link

Decoder loops forever when constructed with a non-array value #42

Closed broofa closed 2 years ago

broofa commented 2 years ago

Describe the bug The following code loops forever due to r being undefined inside readVarUint().

import { createDecoder, readVarUint } from 'lib0/decoding';

// Create a Blob (not an actual Blob, since they aren't a thing in Node, but
// let's pretend we have one like you get in the Browser)
const blob = { size: 20, type: '' };

const decoder = createDecoder(blob);
readVarUint(decoder);

Expected behavior createDecoder() should throw if passed an unexpected value (such as a Blob)

... or something, anything, other than locking up the CPU/browser/system for all eternity. :-)

Screenshots CleanShot 2022-08-19 at 15 02 14@2x

dmonad commented 2 years ago

Hi @broofa,

I understand where you are coming from. Many JavaScript libraries use ducktyping to "verify" that the input is of a specific type.

Note that normal users don't have to interact with this library. I see lib0 more as an expert library, and I expect users who consume this library directly to use typescript, which would easily catch an issue like this.

Ducktyping complicates the codebase and, personally, I don't see it as a good practice in JavaScript libraries. However, I see that expectations differ as a majority of npm packages still use ducktyping instead of adding proper type information.

jamesopti commented 1 year ago

So we are able to reproduce this easily (see below), based on a unit test we have for invalid input to a Y.Doc:

const Y = require('yjs')
const tempYdoc = new Y.Doc();
const ydocBytes = Buffer.from("garbage", "base64");
Y.applyUpdate(tempYdoc, ydocBytes);
// Hangs forever

Note that normal users don't have to interact with this library. I see lib0 more as an expert library, and I expect users who consume this library directly to use typescript, which would easily catch an issue like this.

@dmonad Do you have a recommendation for how to validate input to applyUpdate before passing it in?

UPDATE:

Here's some sample code I have trying to validate a good update from a bad one:

const JSBase64 = require("js-base64");
const Y = require("yjs");
const lib0 = require("lib0");

const sampleYdoc = new Y.Doc();
sampleYdoc.getArray("myarray").insert(0, ["Hello doc2, you got this?"]);
const sampleYdocState = Y.encodeStateAsUpdate(sampleYdoc); // is a Uint8Array
const sampleYdocString = JSBase64.fromUint8Array(sampleYdocState);

const updateGood = Buffer.from(sampleYdocString, "base64");
const updateBad = Buffer.from("garbagedoc", "base64");

const decoderGood = lib0.decoding.createDecoder(updateGood);
const decoderBad = lib0.decoding.createDecoder(updateBad);

// How can I verify that updateBad is in fact bad?