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

FakeXMLHttpRequest falsly sets responseXML #25

Closed
mroderick opened this issue Jan 26, 2018 · 5 comments
Closed

FakeXMLHttpRequest falsly sets responseXML #25

mroderick opened this issue Jan 26, 2018 · 5 comments

Comments

@mroderick
Copy link
Member


Migrated from sinonjs/sinon#1678
Originally created by @Ninerian on Fri, 26 Jan 2018 14:41:19 GMT


  • Sinon version : 4.2.1
  • Environment: Node 9.4.0, Chrome, Firefox, Safari
  • Other libraries you are using: karma, chai, mocha

What did you expect to happen?
When a invalid xml file is loaded with XMLHttpRequest, the responseXML is null.

What actually happens
The FakeXMLHttpRequest fills the responseXML with the parsed xml document with included parseErrors.

How to reproduce

const invalidXMLData = `!!!<?xml version="1.0" encoding="UTF-8"?><start>`;

function testXMLRequest(options) {
  var xhr = new XMLHttpRequest();
  xhr.open('GET', options.url, true);

  // If specified, responseType must be empty string or "document"
  xhr.responseType = 'document';

  // overrideMimeType() can be used to force the response to be parsed as XML
  xhr.overrideMimeType('text/xml');

  xhr.onload = function() {
    if (xhr.readyState === xhr.DONE) {
      if (xhr.status === 200) {
        if (xhr.responseXML) {
          options.onSuccess();
        } else {
          options.onErrorCallback();
        }
      }
    }
  };

  xhr.send(null);
}

let xhr, requests, requestor;

before(() => {
  xhr = sinon.useFakeXMLHttpRequest();
  requests = [];
  xhr.onCreate = function(xhr) {
    requests.push(xhr);
  };
});

after(() => {
  xhr.restore();
});

it('Should throw an error, when the xml is invalid', () => {
  let successCallback = sinon.spy();
  let errorCallback = sinon.spy();

  const reqSettings = {
    url: '/xml',
    onSuccess: successCallback,
    onErrorCallback: errorCallback,
  };

  testXMLRequest(reqSettings);

  requests[0].respond(200, { 'Content-Type': 'text/text' }, invalidXMLData);

  expect(successCallback.notCalled, 'Success callback is not called').to.be.true;
  expect(errorCallback.calledOnce, 'Error callback is called').to.be.true;
});

Solution
After parsing the response, you should check for parseerrors and return null.

@mnahkies
Copy link
Contributor

mnahkies commented Feb 9, 2018

When running tests in node using JSDom then I believe this is a deficiency in https://github.com/jindw/xmldom which appears to be a dead project.

However, nise does not seem to check for parsererror tags, which would normally be returned in an error document by a browser (ref: https://developer.mozilla.org/en-US/docs/Web/API/DOMParser) - rather than an exception being thrown as nise seems to expect.

It would seem sensible for nise to check for parsererror and return null if found here https://github.com/sinonjs/nise/blob/master/lib/fake-xhr/index.js#L319-L322 - which I believe would solve this issue when run in a browser.

Eg:

(new DOMParser()).parseFromString('!!!<xml></xml>', 'text/xml').getElementsByTagName('parsererror').length;
1

See also https://stackoverflow.com/questions/11563554/how-do-i-detect-xml-parsing-errors-when-using-javascripts-domparser-in-a-cross

@stale
Copy link

stale bot commented Apr 10, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@fatso83
Copy link
Contributor

fatso83 commented Jun 1, 2018

Should have been fixed by #50.

@fatso83 fatso83 closed this as completed Jun 1, 2018
@fatso83
Copy link
Contributor

fatso83 commented Jun 1, 2018

@mnahkies Want to check that this works as you think before we cut a new release?

Clone, npm link in the nise repo, and do npm link nise in your project repo

@mnahkies
Copy link
Contributor

mnahkies commented Jun 1, 2018

@fatso83 sorry I'm travelling for the next month or so with limited computer access. I had a glance over the pull request and it looked fine to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants