XMLHttpRequest `.response` encoding error when `.responseType` is "arraybuffer" and synchronous

[jeremy-code]

Consider the following code (StackBlitz) where XHR_URL links to example.txt with content Héllo 🌍:

import XMLHttpRequest from 'xmlhttprequest-ssl';

const XHR_URL =
  'https://gist.githubusercontent.com/jeremy-code/19f2a087bcd74686cc68fd8657b43ec2/raw/77bd7608adf37e79dcc6f760a0aa0f9ee8f28d32/example.txt';

const getXhrResponse = () => new TextDecoder().decode(xhr.response);

const xhr = new XMLHttpRequest();
xhr.responseType = 'arraybuffer';

xhr.open('GET', XHR_URL, /* async */ false);
xhr.send(null);
console.log(`Synchronous:  ${getXhrResponse()}`);

xhr.onload = () => {
  console.log(`Asynchronous: ${getXhrResponse()}`);
};
xhr.open('GET', XHR_URL, /* async */ true);
xhr.send(null);

This outputs:

❯ node index.js
Synchronous:  H�llo <
Asynchronous: Héllo 🌍

While I can't verify the browser implementation of the synchronous call since all major browsers (WebKit, Firefox, Chrome) disable synchronous XHR with .responseType "arraybuffer", on WebKit, Firefox, Chrome it does output Asynchronous: Héllo 🌍.

I believe the issue is within this function's implementation

var stringToBuffer = function(str) {
  const ab = new ArrayBuffer(str.length)
  const buf = Buffer.from(ab);
  for (let k = 0; k < str.length; k++)
    buf[k] = Number(str.charCodeAt(k));
  return buf;
}

which I believe results in two errors:

1. .charCodeAt() outputs the UTF-16 code unit, which I don't think is intended since it's being stored in a Buffer (which is internally a Uint8Array)
2. Iterating through string indices causes unexpected behavior with charCodeAt, see String.prototype.charCodeAt() > Looping with codePointAt()

const INPUT = "Héllo 🌍";

var stringToBuffer1 = function (str) {
  const ab = new ArrayBuffer(str.length);
  const buf = Buffer.from(ab);
  for (let k = 0; k < str.length; k++) buf[k] = Number(str.charCodeAt(k));
  return buf;
};

var stringToBuffer2 = function (str) {
  const encoder = new TextEncoder();
  const buf = Buffer.from(encoder.encode(str));
  return buf;
};

console.log("stringToBuffer1:", stringToBuffer1(INPUT));
console.log("stringToBuffer2:", stringToBuffer2(INPUT));
console.log("stringToBuffer1.toString():", stringToBuffer1(INPUT).toString());
console.log("stringToBuffer2.toString():", stringToBuffer2(INPUT).toString());

❯ node test.js
stringToBuffer1: <Buffer 48 e9 6c 6c 6f 20 3c 0d>
stringToBuffer2: <Buffer 48 c3 a9 6c 6c 6f 20 f0 9f 8c 8d>
stringToBuffer1.toString(): H�llo <
stringToBuffer1.toString(): Héllo 🌍

I believe this code should work as expected. TextEncoder comes with node:util by default since version v8.3.0, which I believe should be fine since the package.json engines claims >=12.0.0.

var stringToBuffer = function(str) {
  const encoder = new TextEncoder();
  const buf = Buffer.from(encoder.encode(str));
  return buf;
}

and it passes all tests with the exception of exactly one test in test-sync-response.js:

  xhr.onreadystatechange = function () {
    if (xhr.readyState === 4) {
      // xhr.response is an ArrayBuffer
      var binaryStr = Buffer.from(xhr.response).toString('binary');
      var f32 = stringToFloat32Array(binaryStr);
      log('/binary2', f32);
      var answer = new Float32Array([1, 5, 6, 7]);
      assert.equal(isEqual(f32, answer), true);
      ...

The issue with that test is a bit harder to fix. If you read the buffer of the response for [ 1, 5, 6, 7 ], yyou get 00 00 ef bf bd 3f 00 00 ef bf bd 40 00 00 ef bf bd 40 00 00 ef bf bd 40 or "\u0000\u0000�?\u0000\u0000�@\u0000\u0000�@\u0000\u0000�@" which comes from being interpreted as utf8 in the server data: responseData.toString('utf8')}. The actual spec claims:

If charset is null, prescan the first 1024 bytes of xhr’s received bytes and if that does not terminate unsuccessfully then let charset be the return value.

which is a bit complicated.

Let me know what you think.

[mjwwit]

Well, that's a bug alright. I appreciate the effort to get to the bottom of this. It is probably fixable, but as you said, this behavior is disabled in major browsers. This module solely exists to have browser API compatibility, so maybe the approach is to disable sync arraybuffer requests in this module as well?

[Bhpsngum]

After a bit of looking around, I noticed that the root problem does not lie in the stringToBuffer function, it instead lies in the way this library reads content if the loading operation is synchronous

By being "synchronous", the library just spawns a process, fetches the resource, and writes to a file, while the current process blocks the execution by constantly calling read operation (fs.readFileSync) on that written file(s). Here are the things that go wrong with the implementation:

Library

The content is written as a decoded UTF-8 string to the content file, and in many cases, converting from binary to UTF-8 can corrupt the binary, causing the problem as stated in the original issue.

So, to solve the problem, simply write the data as pure binary to an additional file if the fetch success (for example, node-xmlhttprequest-<pid>.bin), then later remove it.

This is one of my suggestion:

lib/XMLHttpRequest.js:L802

-        + "      fs.writeFileSync('" + contentFile + "', JSON.stringify({err: null, data: {url: url.href, statusCode: response.statusCode, statusText: response.statusMessage, headers: response.headers, data: responseData.toString('utf8') }}), 'utf8');"
+        + "      fs.writeFileSync('" + contentFile + "', JSON.stringify({err: null, data: {url: url.href, statusCode: response.statusCode, statusText: response.statusMessage, headers: response.headers }}), 'utf8');"
+        + "      fs.writeFileSync('" + contentFile + ".bin', responseData);"

lib/XMLHttpRequest.js:L842

-        self.response = stringToBuffer(resp.data.data);
+        try { self.response = fs.readFileSync(contentFile + ".bin"); } catch (e) {}
+        try { fs.unlinkSync(contentFile + ".bin"); } catch (e) {}

The try-catch on the unlink is a safety measure, in case an external cause deleted/changed the permission for the temp file and errored our process, if it's needed, one more try-catch is required also when removing the metadata temp file.

Since this new approach doesn't use the stringToBuffer function, it is safe to remove it completely from library code.

Test case

There is also problem on constructing the test case. First, on a test server:

tests/server.js:L33

case "/binary2":
            const ta = new Float32Array([1, 5, 6, 7]);
            const buf = Buffer.from(ta.buffer);
            const str = buf.toString('binary');
            res.writeHead(200, {"Content-Type": "application/octet-stream"})
            res.end(str);
            return;

buf.toString('binary') is totally unnecessary, and can also cause the double-encoding problem --> corrupts the binary
The solution is to just send the buffer directly:

tests/server.js:L33

case "/binary2":
            const ta = new Float32Array([1, 5, 6, 7]);
-           const buf = Buffer.from(ta.buffer);
-           const str = buf.toString('binary');
+           const buf = Buffer.from(ta); // More safe than `ta.buffer`
            res.writeHead(200, {"Content-Type": "application/octet-stream"})
-           res.end(str);
+           res.end(buf);
            return;

The test-sync-response.js file also encounters the same problem, here i'll just provide the diff for simplicity:

tests/test-sync-response.js

xhr.onreadystatechange = function () {
    if (xhr.readyState === 4) {
      // xhr.response is an ArrayBuffer
-     var binaryStr = Buffer.from(xhr.response).toString('binary');
-     var f32 = stringToFloat32Array(binaryStr);
+     var f32 = new Float32Array(Buffer.from(xhr.response));
      log('/binary2', f32);
      var answer = new Float32Array([1, 5, 6, 7]);
      assert.equal(isEqual(f32, answer), true);
      assert.equal(xhr.getResponseHeader('content-type'), 'application/octet-stream')
      isSync = true;
    }
  }

(here stringToFloat32Array function is also unused and can be totally removed from this file)

Doing all of this fixes the problem and still has 100% test cases passed. Let me know if you have any questions about this 😄

[jeremy-code]

Well, that's a bug alright. I appreciate the effort to get to the bottom of this. It is probably fixable, but as you said, this behavior is disabled in major browsers. This module solely exists to have browser API compatibility, so maybe the approach is to disable sync arraybuffer requests in this module as well?

To be honest, I'm not opposed to disabling XHR synchronous requests in general.

The official standard actually says:

Synchronous XMLHttpRequest outside of workers is in the process of being removed from the web platform as it has detrimental effects to the end user’s experience. (This is a long process that takes many years.) Developers must not pass false for the async argument when the current global object is a Window object. User agents are strongly encouraged to warn about such usage in developer tools and may experiment with throwing an "InvalidAccessError" DOMException when it occurs.

Probably the closest analogue to this project is kitsonk/xhr written by an ex-Deno contributor and used in Deno CLI/Deploy, and they have also disabled synchronous XHRs, saying:

Sync is not supported (passing false to the async argument). Most browsers have deprecated it in the main thread. Since this polyfill works by calling Deno's fetch() it is nearly impossible to generate a sync version, plus it is a really bad idea to block a thread while waiting for a server response. Don't do it, don't use software that requires it.

And Node.js officially recommends that child_process.spawnSync should not be used in servers but for scripting since it blocks the event loop.

But I also don't know how much sync XHR is actually used in general, and it would probably be a major breaking change. Personally, I rewrote my use case to work async so I'm unconcerned.

EDIT:
Sorry, I forgot that this library actually uses writeFileSync() for sync XHR, but Node.js also doesn't recommend using the sync fs APIs — so my point still stands.

[Bhpsngum]

The usage of write sync and read sync is the "trick" that replicates the synchronicity of the library. Honestly, I agree that this is kind of hacky and is not recommended to use them for servers. And since this feature is also deprecated by most browsers, as for now, to discourage users from using sync, we can just add an option like this:

var xhr = new XMLHttpRequest({ allowSync: true }); // default is false

When sync is disabled, any attempt to fetch synchronously will throw the error Synchronous XMLHttpRequest is disabled by configuration or a similar message.

[mjwwit]

When sync is disabled, any attempt to fetch synchronously will throw the error Synchronous XMLHttpRequest is disabled by configuration or a similar message.

I like the idea, but this is still a breaking change. Maybe we can log a warning (to stderr) every time someone uses sync, and allow them to disable these warnings through a new option?

Also, since you've basically fixed the issue at this point, would you mind sending in a PR? This will at least allow you to take credit for all the work you already put in.

[Bhpsngum]

As for now, I'm thinking of this approach:

When user creates a XMLHttpRequest class with allowSync = true, prompts a warning in console:

[Deprecation] Synchronous XMLHttpRequest is deprecated because of its detrimental effects to the end user's experience. Set allowSync to false to disable this feature.

and when allowSync is false, any attempt to fetch synchronously will throw the error Synchronous XMLHttpRequest is disabled by configuration.

About the PR, i could create one after we agreed on what workflow to do with the sync operation, and after I tried to see if i could fix the issue #27

[mjwwit]

I'm not sure we should deviate from browser behavior by design. Any options passed to XMLHttpRequest are already non-standard, so having to specify one to enable standard behavior seems odd. People using this library probably do so because it supports synchronous requests and ssl, so disabling one of these by default seems counter intuitive. The XHR API is quirky and, especially its synchronous functionality, is sub-optimal. However, most users probably have external requirements forcing them to use a library that supports this. Therefore I'd rather have the non-intrusive logs when using synchronous requests.

[Bhpsngum]

The default option does allow synchronous requests (if allowSync is not set, the default is true), but since you mentioned not wanting intrusive logs for synchronous requests (although some browsers do show it, except Chrome),

So, maybe we can either show a warning whenever user calls xhr.open(...) with sync enabled (that's where most browsers warn user about) or just throw an error when allowSync is disabled.

Does that sound more reasonable?

Edit: After a bit more of reading, synchronous XMLHttpRequest could be disabled and throws these possible errors after user calls .send()

Synchronous requests are disabled for this page.
Synchronous requests are disabled by permissions policy.
Synchronous requests are disabled by document policy.

So, by that, we could also throws our error only when user calls .send() with disabled sync:

Synchronous requests are disabled for this instance.

Overall process:

• Calls new XMLHttpRequest class: nothing happens
• Calls .open: gives a warning (or not, up to your opinion)

[Deprecation] Synchronous XMLHttpRequest is deprecated because of its detrimental effects to the end user's experience. Set allowSync to false to disable this feature.

• Calls .send(): If sync is disabled, throws this error:

Synchronous requests are disabled for this instance.

[mjwwit]

The default option does allow synchronous requests (if allowSync is not set, the default is true), but since you mentioned not wanting intrusive logs for synchronous requests (although some browsers do show it, except Chrome),

So, maybe we can either show a warning whenever user calls xhr.open(...) with sync enabled (that's where most browsers warn user about) or just throw an error when allowSync is disabled.

Does that sound more reasonable?

I must have misunderstood, and that does sound more reasonable. I'm not opposed to any logging, especially since major browsers have also started moving in this direction, but I do feel that a user should have the option to disable this. Maybe allowSync can be a boolean value or "logWarning", with the default value being "logWarning" (allowing sync requests, but logging a warning to the console every time one is sent). The user can then still disable synchronous requests by setting the allowSync option to false, or enable it without any warnings being logged by setting it to true.

Edit: After a bit more of reading, synchronous XMLHttpRequest could be disabled and throws these possible errors after user calls .send()

Synchronous requests are disabled for this page.
Synchronous requests are disabled by permissions policy.
Synchronous requests are disabled by document policy.

So, by that, we could also throws our error only when user calls .send() with disabled sync:

Synchronous requests are disabled for this instance.

Overall process:

* Calls new `XMLHttpRequest` class: nothing happens

* Calls `.open`: gives a warning (or not, up to your opinion)

[Deprecation] Synchronous XMLHttpRequest is deprecated because of its detrimental effects to the end user's experience. Set allowSync to false to disable this feature.

* Calls `.send()`: If sync is disabled, throws this error:

Synchronous requests are disabled for this instance.

I agree with your proposed process, with the additions I mentioned above. So:

When simply using new XMLHttpRequest() (same as new XMLHttpRequest({ allowSync: 'logWarning' })):

1. Calls new XMLHttpRequest(); nothing special happens
2. Calls .open; logs a warning to the console (using console.warn)
3. Calls .send(); works as expected.

When using new XMLHttpRequest({ allowSync: false }):

1. Calls new XMLHttpRequest(); nothing special happens
2. Calls .open; logs a warning to the console (using console.warn)
3. Calls .send(); an error is thrown and the request isn't sent.

When using new XMLHttpRequest({ allowSync: true }):

1. Calls new XMLHttpRequest(); nothing special happens
2. Calls .open; nothing special happens
3. Calls .send(); works as expected.

I'm curious to hear what you think.

[Bhpsngum]

Maybe allowSync can be a boolean value or "logWarning", with the default value being "logWarning" (allowing sync requests, but logging a warning to the console every time one is sent). The user can then still disable synchronous requests by setting the allowSync option to false, or enable it without any warnings being logged by setting it to true.

Creating an option with mixed types between string and boolean is not a good practice, as it could cause confusion and be counterintuitive. My proposed way instead is to enforce string type:

1. "warn": Runs with warning (default)
2. "enabled": Runs without warnings
3. "disabled": Throws error

By this naming, we should also renamed the flag too, from allowSync to like either sync or syncMode or syncPolicy.

What do you think about this way?

EDIT: Another way is to have 2 separate flags: allowSync: true/false and syncWarning: true/false, but imo having 2 flags for only our 3 cases of feature is just... not right.

[mjwwit]

We are in agreement 💯. Let's use syncPolicy with "warn", "enabled", and "disabled".

[mjwwit]

Fixed in #31, released as 4.0.0