socketio / socket.io

Realtime application framework (Node.JS server)
https://socket.io
MIT License
61.1k stars 10.11k forks source link

'_placeholder' in ESM module is replaced with 'it' which breaks binary data #5215

Open digulla opened 6 days ago

digulla commented 6 days ago

Describe the bug The file socket.io.esm.min.js from the NPM package socket.io-client 4.8.0 contains a bug which breaks binary data when talking to other SocketIO implementations (for example python-socjetio).

Instead of {_placeholder: true, num: 0}, the code produces {it: true, num: 0}. It's correct in the source. The problem is somewhere in the ESM build.

To Reproduce Search for function _deconstructPacket in socket.io.js. It looks like this:

  function _deconstructPacket(data, buffers) {
    if (!data) return data;
    if (isBinary(data)) {
      var placeholder = {
        _placeholder: true,
        num: buffers.length
      };

In the ESM, it gets compiled into this code:

function ot(t, s) {
    if (!t)
        return t;
    if (et(t)) {
        const i = {
            it: !0,
            num: s.length
        };
        return s.push(t),
        i
    }

The same happens in _reconstructPacket. Source:

  function _reconstructPacket(data, buffers) {
    if (!data) return data;
    if (data && data._placeholder === true) {
      var isIndexValid = typeof data.num === "number" && data.num >= 0 && data.num < buffers.length;

which is compiled into

function ct(t, s) {
    if (!t)
        return t;
    if (t && !0 === t.it) {
        if ("number" == typeof t.num && t.num >= 0 && t.num < s.length)

Expected behavior The binary representation in the serialized message must be correct.

Platform:

Additional context n.a.

darrachequesne commented 6 days ago

I could indeed reproduce the issue, I'm digging into this.

digulla commented 6 days ago

Thanks for the quick response. My workaround is to use a unit test to copy the file into the production folder and patch the two places. So it's not a killer at the moment.

darrachequesne commented 3 days ago

@digulla this should be fixed in version 4.8.1. Could you please check?

digulla commented 14 hours ago

Thanks for the quick update! But it's now mangled to const i={et:!0,num:s.length}; :-(

Can you add a unit test which loads the minified version and checks the output of the function? Or at least add a unit test which fails when the minified version doesn't contain the string _placeholder twice.

Or if you want to do it manually, search for num:; that's the easiest way to find it in the code. I guess it doesn't mangle 3 letter identifiers.

digulla commented 14 hours ago

For reference, I'm using the Python unit test to fix the bug in 4.8.0:

from pathlib import Path
import shutil
import unittest

root = Path('.').resolve()
lib_path = root / 'lib'
node_modules_path = root / 'node_modules'
socket_io_client_path = node_modules_path / 'socket.io-client' / 'dist'

class TestFixJavaScript(unittest.TestCase):
    def test_fix_placeholder(self):
        input = socket_io_client_path / 'socket.io.esm.min.js'
        output = lib_path / input.name

        source = input.read_text(encoding='utf8')
        patched = source
        patched = self.apply_single_fix(patched, '{it:!0,num:s.length}', '{_placeholder:!0,num:s.length}')
        patched = self.apply_single_fix(patched, 'if(t&&!0===t.it){if("number"', 'if(t&&!0===t._placeholder){if("number"')

        lib_path.mkdir(parents=True, exist_ok=True)
        output.write_text(patched, encoding='utf8')

    def apply_single_fix(self, source: str, pattern: str, fix: str) -> str:
        count = 0
        start = 0
        while True:
            pos = source.find(pattern, start)
            if pos == -1:
                break

            count += 1
            start = pos + len(pattern)

        self.assertEqual(1, count, f'Expected {pattern!r} only once but found it {count} times')
        result = source.replace(pattern, fix)
        if result == source:
            self.fail(f'Replacing {pattern!r} failed')

        return result