Jsdom: XMLHttpRequest access the file:// when we on http:// domain

[selam]

Hi,

XMLHttpRequest can access the local files when we on http domain, you can produce from this repo https://github.com/selam/jsdom-xmlhttp-bug, this can be serius security problem, attacker can steal some your local files easly becouse XMLHttpRequest doesnt care window.location.href or window.document.URL

When i try to access local files from http domain from browser i got two error messages on console

XMLHttpRequest cannot load file:///home/timu/workspace/jsdom-xmlhttp-bug/test.html. Cross origin requests are only supported for protocol schemes: http, data, chrome, chrome-extension, https, chrome-extension-resource.(anonymous function) @ test.html:12

test.html:12 Uncaught NetworkError: Failed to execute 'send' on 'XMLHttpRequest': Failed to load 'file:///home/timu/workspace/jsdom-xmlhttp-bug/test.html'.

Also i haven't check yet but i believe XMLHttpRequest and jsdom also doesn't care Cross origin, X-Frame-Options, Content-Security-Policy, X-XSS-Protection headers from responses

[Sebmaster]

jsdom is not safe to execute arbitrary scripts at all. It is not properly sandboxed, cross-origin isn't prevented and there's probably more. It's an issue we have in mind, but this probably won't change for the foreseeable future.

[domenic]

I'd be happy to implement a rudimentary same-origin policy enforcement, e.g. checking the .origin properties of URLs. But without also implementing the CORS protocol, I am afraid of this breaking a bunch of our users, since we would suddenly become _more_ restrictive than web browsers.

And yeah, given how insecure running scripts in jsdom is anyway, it's not too high of a priority... Although I'd kind of like to fix that somehow...

[Joris-van-der-Wel]

One idea I've occasionally thought about is to not have jsdom sandbox the pages it visits, but just sandbox all of jsdom itself. Kind of like how firefox added support for PDF's: they made a pdf reader in javascript, so even if you find an exploit in their pdf implementation, you are still in the javascript sandbox.

You can already browserify jsdom so that it runs in chrome. However jsdom can not fetch pages over http in this case.

[selam]

jsdom uses lot of modules and these modules uses http/https or request module's so we can not track them all in once, to implement of these features/standarts we need to adding a new abstraction layer for networking so we can control all flow so we can use @Joris-van-der-Wel's idea but it's too hard to implement and i am not sure this is a correct goal for jsdom.

[domenic]

Yeah, I think some version of @Joris-van-der-Wel's idea (running all of jsdom's code inside a vm context) is the only way to get true security here. Otherwise, it's just too easy to break out.

Here's a simple test case that shows how, as long as you execute script with at least one object in the sandbox, you can access the fs module and thus anything on the filesystem: https://gist.github.com/domenic/d15dfd8f06ae5d1109b0

Note that in jsdom's case, the sandbox is window, so there are a lot of objects to go around.

The only way to counteract this is to not pass in anything to the script at all, but instead execute all of jsdom inside the sandbox, with a tightly controlled and audited message-passing interface for any I/O it needs to do (the messages would have to only be strings or similar). That's a lot of work, to the point of basically being a whole new project :-/.

[niftylettuce]

Should we plug this package into jsdom and set it as global.XMLHttpRequest? https://github.com/driverdan/node-XMLHttpRequest

[domenic]

What do you imagine that would accomplish?

[niftylettuce]

@domenic I was trying to use jsdomify and not have to run stuff with phantomjs or use html files to run client-side browser tests.

[niftylettuce]

I'm implementing a client-side test for https://github.com/niftylettuce/frisbee and going to have to just do the client-side testing in phantomjs probably.

[domenic]

I don't understand why you think replacing jsdom's new XHR implementation with its old one would help that.

[niftylettuce]

where is XMLHttpRequest already implemented in jsdom? its not accessible
at global.XMLHttpRequest
On Nov 29, 2015 6:00 PM, "Domenic Denicola" [email protected]
wrote:

I don't understand why you think replacing jsdom's new XHR implementation
with its old one would help that.

—
Reply to this email directly or view it on GitHub
https://github.com/tmpvar/jsdom/issues/1203#issuecomment-160479568.

[domenic]

global is a part of node, not jsdom, so I don't know what you're talking about. jsdom implements window.

[domenic]

Maybe you are using one of those libraries which tries to copy a few things from window to global but fails to copy everything. Those libraries are very bad.

[niftylettuce]

Im trying to write tests for whatwg-fetch, that's exactly it. {facepalm}
On Nov 29, 2015 6:05 PM, "Domenic Denicola" [email protected]
wrote:

Maybe you are using one of those libraries which tries to copy a few
things from window to global but fails to copy everything. Those libraries
are very bad.

—
Reply to this email directly or view it on GitHub
https://github.com/tmpvar/jsdom/issues/1203#issuecomment-160479834.

[niftylettuce]

@domenic https://github.com/niftylettuce/frisbee/blob/master/test/unit/browser.test.js#L28-L33 because this https://github.com/github/fetch/pull/239 doesn't use window :frowning:

[Sebmaster]

Seems like we should actually implement window.self = window. Do we not have that?

[domenic]

We definitely do.

The problem here is jsdomify. You should not be using it. You should be running your tests inside the jsdom window, not inside Node.js with a random smattering of globals transferred from the jsdom window to Node.js.

[niftylettuce]

@domenic is it possible for you to show me how to rewrite this then, even a snippet? https://github.com/niftylettuce/frisbee/blob/master/test/unit/browser.test.js#L28-L33

[niftylettuce]

@domenic oh wow, this is easy after all!

jsdom.env(
  "https://iojs.org/dist/",
  ["http://code.jquery.com/jquery.js"],
  function (err, window) {
    console.log("there have been", window.$("a").length - 4, "io.js releases!");
  }
);

nice! awesome project.

[Sebmaster]

You should be able to just put your frisbee.js file into the scripts array you provide to jsdom (although you may have to transpile to node-supported constructs + use browserify to resolve dependencies) and then be able to access it over window (not sure how that'd work with modules though).

[domenic]

Something like this should hopefully provide some guidance:

const jsdom = require('jsdom');
const fs = require('fs');
const myLibraryToTest = fs.readFileSync(require.resolve('../../whatever.js'));

let window;
beforeEach(() => {
  const document = jsdom.jsdom();
  window = document.defaultView;

  // Execute my library by inserting a <script> tag containing it.
  const scriptEl = document.createElement('script');
  scriptEl.textcontent = myLibraryToTest;
  document.body.appendChild(scriptEl);
});

it('should do the right thing', () => {
  assert.equal(window.myLibrary.doThing('foo'), 'bar');
});

The jsdom.env version is a bit simpler:

const jsdom = require('jsdom');

let window;
beforeEach(done => {
  jsdom.env({
    html: '',
    scripts: [require.resolve('../../whatever.js')],
    done(err, createdWindow) {
      if (err) return done(err);

      window = createdWindow;
      done();
    }
  });
});

it('should do the right thing', () => {
  assert.equal(window.myLibrary.doThing('foo'), 'bar');
});

[niftylettuce]

very cool @domenic -- does this define self as well?

[domenic]

@niftylettuce window.self is defined, and your whatever.js can use self since it's just running in the jsdom.

[niftylettuce]

thank you so much @domenic and @Sebmaster https://github.com/niftylettuce/frisbee/blob/master/package.json#L12-L13

https://github.com/niftylettuce/frisbee/blob/master/test/unit/browser.test.js#L12-L21

[dmatteo]

Hi @domenic, I just read the thread, and I can understand your critiques, but I really don't see another way of running tests in Node.js without "smattering" some of the globals from jsdom to Node.js.

The only purpose of jsdomify is to run tests in Node.js instead of the jsdom window, and that's because libraries like React requires a DOM instance to run, even when not using the DOM at all.

That said, what approach would you suggest to achieve this?

[Sebmaster]

The only purpose of jsdomify is to run tests in Node.js instead of the jsdom window, and that's because libraries like React requires a DOM instance to run, even when not using the DOM at all.

Why though? That might work fine for React because they don't actually use any other objects, but other libraries will constantly break because they access some other non-cloned object, reference etc.

The recommended approach is to always use the sandbox for code that was intended to run in a browser - because the sandbox behaves more closely like it.

[dmatteo]

Why though? That might work fine for React because they don't actually use any other objects, but other libraries will constantly break because they access some other non-cloned object, reference etc.

Because the whole point of it was to get away from the browser, to run unit tests in Node.js fast.

I suppose I can iterate on every property of the window and just clone it to the global scope, although I'm not sure about the performance of this, while done on every single test suite

[Sebmaster]

The sandbox isn't a browser though, it's just a different context in v8 than the one node uses by default. You're not paying any additional performance costs.

[dmatteo]

So what's the recommended approach to create, use and destroy a sandbox for each test suite, that can run tests described in Node.js?

[domenic]

but I really don't see another way of running tests in Node.js without "smattering" some of the globals from jsdom to Node.js

Did you not see https://github.com/tmpvar/jsdom/issues/1203#issuecomment-160725194 ?

So what's the recommended approach to create, use and destroy a sandbox for each test suite, that can run tests described in Node.js?

The approach in https://github.com/tmpvar/jsdom/issues/1203#issuecomment-160725194

[dmatteo]

But this will never work with React.js, as they expect to have both document and window as globals when you require it!

[domenic]

Don't require it! Insert it into the jsdom window, as shown in that example.

[dmatteo]

So if I have to load React, ReactTestUtils and a few components, I should fetch them all with fs from their respective node_modules path and then inline them into the jsdom document?

It doesn't look very practical to me

[domenic]

Why? That's exactly what require is doing (for the Node.js global context), just require is more indirect about it. What is not practical is assuming that a Node.js global can somehow be identified with a window, despite basic properties (like this === window) failing.

[dmatteo]

Introducing such a change in a big codebase is huuuuge work, as you have to change how everything has been required.
Reusing node's require makes it plug & play, and that's the best advantage of it.

What is the issue in "leaking" window properties to the global scope?

[domenic]

Introducing such a change in a big codebase is huuuuge work, as you have to change how everything has been required.

You don't have to change anything. The codebase is meant to run in a browser, so if it's using require there, then browserify it, like @niftylettuce did.

What is the issue in "leaking" window properties to the global scope?

The Node global environment is not a browser global environment. Creating some monster hybrid where parts of it become pointers to a newly-created jsdom window, and parts of it are Node.js, causes many issues. Not the least of which is that jsdom is not designed to work in that environment, accessed via external pointers.

What you are doing is like trying to do a test in the browser by creating an iframe, setting window.window = iframe.contentWindow, window.document = iframe.contentWindow.document. You would never actually do this. You would just load your tests in the iframe.

[dmatteo]

The codebase is meant to run in a browser, so if it's using require there, then browserify it, like @niftylettuce did.

That's not entirely true. In the case of isomorphic applications, the components can be run both on Node and the browser. In our case, requirejs is used in the browser with AMD modules, for added complexity.

Back to what I described as a huge effort, it doesn't really matter where they are meant to be executed.
When you have hundreds of test suites built for Karma and PhantomJS, keeping the require calls intact is paramount if you don't want to refactor everything for months.

[domenic]

Isomorphic applications do not require jsdom when run on the server. Browser applications run inside the jsdom.

[Joris-van-der-Wel]

Isomorphic applications do not require jsdom when run on the server.

That is not true in all cases though. My isomorphic project simply used DOM to create or update its view. I used jsdom.jsdom() to create a document for each request, however I did not use any global state, nor did I use the sandbox. After the first request the client side DOM takes over (same code, but now browserified).

[samarpanda]

Fantastic suggestion @domenic #1203(comment) is working like a champ :+1:

[domenic]

jsdom is still not super safe, but we do have some CORS checking now!