From ce473f8e4c8b48bfa431c55e2dfe34b0ed92ac08 Mon Sep 17 00:00:00 2001 From: Jeffrey Jagoda Date: Thu, 26 Mar 2015 11:22:32 -0400 Subject: [PATCH] Avoid Node URL bug. --- src/assert.js | 5 +++-- src/document.js | 5 +++-- src/dom/jsdom_patches.js | 19 ++++++++++++++++++- src/index.js | 3 ++- src/resources.js | 15 ++++++--------- src/xhr.js | 7 ++++--- test/history_test.js | 24 ++++++++++++++++++++++++ test/jsdom_patches_test.js | 22 ++++++++++++++++++++++ 8 files changed, 82 insertions(+), 18 deletions(-) create mode 100644 test/jsdom_patches_test.js diff --git a/src/assert.js b/src/assert.js index 4303fc3af..d5a7f970b 100644 --- a/src/assert.js +++ b/src/assert.js @@ -3,6 +3,7 @@ const assert = require('assert'); const { isRegExp } = require('util'); const URL = require('url'); +const Utils = require('jsdom/lib/jsdom/utils'); // Used to assert that actual matches expected value, where expected may be a function or a string. @@ -58,7 +59,7 @@ module.exports = class Assert { // query). url(expected, message) { if (typeof expected === 'string') { - const absolute = URL.resolve(this.browser.location.href, expected); + const absolute = Utils.resolveHref(this.browser.location.href, expected); assertMatch(this.browser.location.href, absolute, message); } else if (isRegExp(expected) || typeof expected === 'function') assertMatch(this.browser.location.href, expected, message); @@ -182,7 +183,7 @@ module.exports = class Assert { const matchedRegexp = matchingText.filter(element => url.test(element.href)); assert(matchedRegexp.length, message || `Expected at least one link matching the given text and URL`); } else { - const absolute = URL.resolve(this.browser.location.href, url); + const absolute = Utils.resolveHref(this.browser.location.href, url); const matchedURL = matchingText.filter(element => element.href === absolute); assert(matchedURL.length, message || `Expected at least one link matching the given text and URL`); } diff --git a/src/document.js b/src/document.js index 43835ac29..01e4e4073 100644 --- a/src/document.js +++ b/src/document.js @@ -9,6 +9,7 @@ const EventSource = require('eventsource'); const WebSocket = require('ws'); const XMLHttpRequest = require('./xhr'); const URL = require('url'); +const Utils = require('jsdom/lib/jsdom/utils'); // File access, not implemented yet @@ -51,7 +52,7 @@ class DOMURL { constructor(url, base) { assert(url != null, new DOM.DOMException('Failed to construct \'URL\': Invalid URL')); if (base) - url = URL.resolve(base, url); + url = Utils.resolveHref(base, url); const parsed = URL.parse(url || 'about:blank'); const origin = parsed.protocol && parsed.hostname && `${parsed.protocol}//${parsed.hostname}`; Object.defineProperties(this, { @@ -491,7 +492,7 @@ module.exports = function loadDocument(args) { let { url } = args; if (url && browser.site) { const site = /^(https?:|file:)/i.test(browser.site) ? browser.site : `http://${browser.site}`; - url = URL.resolve(site, URL.parse(URL.format(url))); + url = Utils.resolveHref(site, URL.format(url)); } url = url || 'about:blank'; diff --git a/src/dom/jsdom_patches.js b/src/dom/jsdom_patches.js index a40490495..8f888326f 100644 --- a/src/dom/jsdom_patches.js +++ b/src/dom/jsdom_patches.js @@ -1,7 +1,9 @@ // Fix things that JSDOM doesn't do quite right. -const DOM = require('./index'); +const DOM = require('./index'); +const Utils = require('jsdom/lib/jsdom/utils'); +const URL = require('url'); DOM.HTMLDocument.prototype.__defineGetter__('scripts', function() { @@ -200,3 +202,18 @@ DOM.resourceLoader.load = function(element, href, callback) { } }; +// Fix residual Node bug. See https://github.com/joyent/node/pull/14146 +const jsdomResolveHref = Utils.resolveHref; +Utils.resolveHref = function (baseUrl, href) { + const pattern = /file:?/; + const protocol = URL.parse(baseUrl).protocol; + const original = URL.parse(href); + const resolved = URL.parse(jsdomResolveHref(baseUrl, href)); + + if (!pattern.test(protocol) && pattern.test(original.protocol) && !original.host && resolved.host) { + return URL.format(original); + } + + return URL.format(resolved); +}; + diff --git a/src/index.js b/src/index.js index b4aa1f7d2..a1d685430 100644 --- a/src/index.js +++ b/src/index.js @@ -19,6 +19,7 @@ const Storages = require('./storage'); const Tough = require('tough-cookie'); const { Cookie } = Tough; const URL = require('url'); +const Utils = require('jsdom/lib/jsdom/utils'); // Version number. We get this from package.json. @@ -536,7 +537,7 @@ class Browser extends EventEmitter { [options, callback] = [{}, options]; const site = /^(https?:|file:)/i.test(this.site) ? this.site : `http://${this.site || 'localhost'}/`; - url = URL.resolve(site, URL.parse(URL.format(url))); + url = Utils.resolveHref(site, URL.format(url)); if (this.window) this.tabs.close(this.window); diff --git a/src/resources.js b/src/resources.js index 2b33aee10..df805f35f 100644 --- a/src/resources.js +++ b/src/resources.js @@ -21,6 +21,7 @@ const Path = require('path'); const QS = require('querystring'); const request = require('request'); const URL = require('url'); +const Utils = require('jsdom/lib/jsdom/utils'); const Zlib = require('zlib'); @@ -269,19 +270,15 @@ Resources.addHandler = function(handler) { // It turns relative URLs into absolute URLs based on the current document URL // or base element, or if no document open, based on browser.site property. // -// Also handles file: URLs and creates query string from request.params for +// Also creates query string from request.params for // GET/HEAD/DELETE requests. Resources.normalizeURL = function(req, next) { - if (/^file:/.test(req.url)) - // File URLs are special, need to handle missing slashes and not attempt - // to parse (downcases path) - req.url = req.url.replace(/^file:\/{1,3}/, 'file:///'); - else if (this.document) + if (this.document) // Resolve URL relative to document URL/base, or for new browser, using // Browser.site req.url = DOM.resourceLoader.resolve(this.document, req.url); else - req.url = URL.resolve(this.site || 'http://localhost', req.url); + req.url = Utils.resolveHref(this.site || 'http://localhost', req.url); if (req.params) { const { method } = req; @@ -442,10 +439,10 @@ Resources.handleHTTPResponse = function(req, res, next) { if ((statusCode === 301 || statusCode === 307) && (req.method === 'GET' || req.method === 'HEAD')) // Do not follow POST redirects automatically, only GET/HEAD - redirectUrl = URL.resolve(req.url, res.headers.location || ''); + redirectUrl = Utils.resolveHref(req.url, res.headers.location || ''); else if (statusCode === 302 || statusCode === 303) // Follow redirect using GET (e.g. after form submission) - redirectUrl = URL.resolve(req.url, res.headers.location || ''); + redirectUrl = Utils.resolveHref(req.url, res.headers.location || ''); if (redirectUrl) { diff --git a/src/xhr.js b/src/xhr.js index e96d8e03b..ea71c1d42 100644 --- a/src/xhr.js +++ b/src/xhr.js @@ -2,8 +2,9 @@ // See http://www.w3.org/TR/XMLHttpRequest/#the-abort()-method -const DOM = require('./dom'); -const URL = require('url'); +const DOM = require('./dom'); +const URL = require('url'); +const Utils = require('jsdom/lib/jsdom/utils'); class XMLHttpRequest extends DOM.EventTarget { @@ -59,7 +60,7 @@ class XMLHttpRequest extends DOM.EventTarget { const headers = {}; // Normalize the URL and check security - url = URL.parse(URL.resolve(this._window.location.href, url)); + url = URL.parse(Utils.resolveHref(this._window.location.href, url)); // Don't consider port if they are standard for http and https if ((url.protocol === 'https:' && url.port === '443') || (url.protocol === 'http:' && url.port === '80')) diff --git a/test/history_test.js b/test/history_test.js index ffdf35e12..4a64dc0e3 100644 --- a/test/history_test.js +++ b/test/history_test.js @@ -337,6 +337,30 @@ describe('History', function() { }); }); + // Node has a bug that causes the root path element to be lowercased which + // causes problems when loading files from the file system. + // See https://github.com/joyent/node/pull/14146 + describe('open from file system with capitalized root', function() { + const FILE_URL = 'file:///Users/foo/index.html'; + + before(function (done) { + browser.visit(FILE_URL, function () { + // Ignore errors -- the file isn't real... + done(); + }); + }); + + it('should change location URL', function() { + browser.assert.url(FILE_URL); + }); + it('should set window location', function() { + assert.equal(browser.window.location.href, FILE_URL); + }); + it('should set document location', function() { + assert.equal(browser.document.location.href, FILE_URL); + }); + }); + describe('change pathname', function() { before(()=> browser.visit('/')); before(function(done) { diff --git a/test/jsdom_patches_test.js b/test/jsdom_patches_test.js new file mode 100644 index 000000000..d52de28b7 --- /dev/null +++ b/test/jsdom_patches_test.js @@ -0,0 +1,22 @@ +const assert = require('assert'); +const Utils = require('jsdom/lib/jsdom/utils'); + + +describe('Utils', function() { + describe('resolving HREFs', function() { + const patterns = [ + ['http://localhost', 'foo', 'http://localhost/foo'], + ['http://localhost/foo/bar', 'baz', 'http://localhost/foo/baz'], + ['http://localhost/foo/bar', '/bar', 'http://localhost/bar'], + ['http://localhost', 'file://foo/Users', 'file://foo/Users'], + ['http://localhost', 'file:///Users/foo', 'file:///Users/foo'], + ['file://foo/Users', 'file:bar', 'file://foo/bar'] + ]; + + it('returns the correct URL', function() { + patterns.forEach(function(pattern) { + assert.equal(Utils.resolveHref(pattern[0], pattern[1]), pattern[2]); + }); + }); + }); +});