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

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

Open
digulla opened this issue Oct 22, 2024 · 6 comments
Open
Labels
bug Something isn't working

Comments

@digulla
Copy link

digulla commented Oct 22, 2024

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:

  • Device: any
  • OS: any

Additional context
n.a.

@digulla digulla added the to triage Waiting to be triaged by a member of the team label Oct 22, 2024
@darrachequesne
Copy link
Member

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

@darrachequesne darrachequesne added bug Something isn't working and removed to triage Waiting to be triaged by a member of the team labels Oct 22, 2024
darrachequesne added a commit that referenced this issue Oct 22, 2024
The "_placeholder" attribute is used when sending binary data, and was
incorrectly mangled (converted to a random short property, like "it",
to reduce the bundle size).

This bug was introduced in [1], included in `[email protected]`.

[1]: 7085f0e

Related: #5215
@digulla
Copy link
Author

digulla commented Oct 22, 2024

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
Copy link
Member

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

@digulla
Copy link
Author

digulla commented Oct 28, 2024

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
Copy link
Author

digulla commented Oct 28, 2024

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

@pgn-dev
Copy link

pgn-dev commented Dec 20, 2024

@darrachequesne has this been fixed? we are seeing a similar regression in 4.8.1 where the minified file socket.io.esm.min.js does not work with binary data. 4.7.5 is the latest version that works for us.

socket.io-client:4.8.1 works fine on nodejs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants