From 0903c7cd15ac86898f8ff6aa0f6633f995044c52 Mon Sep 17 00:00:00 2001 From: Jason Bedard Date: Sat, 23 Jun 2018 11:42:44 -0700 Subject: [PATCH 1/2] test($browser): update MockWindow to normalize URLs similar to real window.location --- test/ng/browserSpecs.js | 43 +++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/test/ng/browserSpecs.js b/test/ng/browserSpecs.js index 07f3683b76ad..306cda81abdd 100644 --- a/test/ng/browserSpecs.js +++ b/test/ng/browserSpecs.js @@ -11,8 +11,9 @@ function MockWindow(options) { } var events = {}; var timeouts = this.timeouts = []; - var locationHref = 'http://server/'; - var committedHref = 'http://server/'; + var locationHref = window.document.createElement('a'); + var committedHref = window.document.createElement('a'); + locationHref.href = committedHref.href = 'http://server/'; var mockWindow = this; var msie = options.msie; var ieState; @@ -60,28 +61,28 @@ function MockWindow(options) { this.location = { get href() { - return committedHref; + return committedHref.href; }, set href(value) { - locationHref = value; + locationHref.href = value; mockWindow.history.state = null; historyEntriesLength++; if (!options.updateAsync) this.flushHref(); }, get hash() { - return getHash(committedHref); + return getHash(committedHref.href); }, set hash(value) { - locationHref = replaceHash(locationHref, value); + locationHref.href = replaceHash(locationHref.href, value); if (!options.updateAsync) this.flushHref(); }, replace: function(url) { - locationHref = url; + locationHref.href = url; mockWindow.history.state = null; if (!options.updateAsync) this.flushHref(); }, flushHref: function() { - committedHref = locationHref; + committedHref.href = locationHref.href; } }; @@ -91,13 +92,13 @@ function MockWindow(options) { historyEntriesLength++; }, replaceState: function(state, title, url) { - locationHref = url; - if (!options.updateAsync) committedHref = locationHref; + locationHref.href = url; + if (!options.updateAsync) committedHref.href = locationHref.href; mockWindow.history.state = copy(state); if (!options.updateAsync) this.flushHref(); }, flushHref: function() { - committedHref = locationHref; + committedHref.href = locationHref.href; } }; // IE 10-11 deserialize history.state on each read making subsequent reads @@ -398,18 +399,18 @@ describe('browser', function() { it('should return current location.href', function() { fakeWindow.location.href = 'http://test.com'; - expect(browser.url()).toEqual('http://test.com'); + expect(browser.url()).toEqual('http://test.com/'); fakeWindow.location.href = 'https://another.com'; - expect(browser.url()).toEqual('https://another.com'); + expect(browser.url()).toEqual('https://another.com/'); }); it('should strip an empty hash fragment', function() { - fakeWindow.location.href = 'http://test.com#'; - expect(browser.url()).toEqual('http://test.com'); + fakeWindow.location.href = 'http://test.com/#'; + expect(browser.url()).toEqual('http://test.com/'); - fakeWindow.location.href = 'https://another.com#foo'; - expect(browser.url()).toEqual('https://another.com#foo'); + fakeWindow.location.href = 'https://another.com/#foo'; + expect(browser.url()).toEqual('https://another.com/#foo'); }); it('should use history.pushState when available', function() { @@ -440,7 +441,7 @@ describe('browser', function() { sniffer.history = false; browser.url('http://new.org'); - expect(fakeWindow.location.href).toEqual('http://new.org'); + expect(fakeWindow.location.href).toEqual('http://new.org/'); expect(pushState).not.toHaveBeenCalled(); expect(replaceState).not.toHaveBeenCalled(); @@ -507,9 +508,9 @@ describe('browser', function() { it('should not set URL when the URL is already set', function() { var current = fakeWindow.location.href; sniffer.history = false; - fakeWindow.location.href = 'dontchange'; + fakeWindow.location.href = 'http://dontchange/'; browser.url(current); - expect(fakeWindow.location.href).toBe('dontchange'); + expect(fakeWindow.location.href).toBe('http://dontchange/'); }); it('should not read out location.href if a reload was triggered but still allow to change the url', function() { @@ -812,7 +813,7 @@ describe('browser', function() { it('should not fire urlChange if changed by browser.url method', function() { sniffer.history = false; browser.onUrlChange(callback); - browser.url('http://new.com'); + browser.url('http://new.com/'); fakeWindow.fire('hashchange'); expect(callback).not.toHaveBeenCalled(); From fe0d4f360e74a59fc6887472b8624753ab810225 Mon Sep 17 00:00:00 2001 From: Jason Bedard Date: Sat, 23 Jun 2018 20:50:15 -0700 Subject: [PATCH 2/2] fix($browser): normalize inputted URLs Calls to `$browser.url` now normalize the inputted URL ensuring multiple calls only differing in formatting do not force a browser `pushState`. Normalization is done the same as the browser location URL and may differ per browser and may be changed by browsers. Today no browsers fully normalize URLs so this does not fix all instances of this issue. See #16100 Closes #16606 --- src/ng/browser.js | 3 ++ test/ng/browserSpecs.js | 75 ++++++++++++++++++++++++++++++++++++++--- 2 files changed, 74 insertions(+), 4 deletions(-) diff --git a/src/ng/browser.js b/src/ng/browser.js index 699602bc9b2d..4bb96ee21f1f 100644 --- a/src/ng/browser.js +++ b/src/ng/browser.js @@ -108,6 +108,9 @@ function Browser(window, document, $log, $sniffer, $$taskTrackerFactory) { if (url) { var sameState = lastHistoryState === state; + // Normalize the inputted URL + url = urlResolve(url).href; + // Don't change anything if previous and current URLs and states match. This also prevents // IE<10 from getting into redirect loop when in LocationHashbangInHtml5Url mode. // See https://github.com/angular/angular.js/commit/ffb2701 diff --git a/test/ng/browserSpecs.js b/test/ng/browserSpecs.js index 306cda81abdd..46a7325c95a9 100644 --- a/test/ng/browserSpecs.js +++ b/test/ng/browserSpecs.js @@ -418,7 +418,7 @@ describe('browser', function() { browser.url('http://new.org'); expect(pushState).toHaveBeenCalledOnce(); - expect(pushState.calls.argsFor(0)[2]).toEqual('http://new.org'); + expect(pushState.calls.argsFor(0)[2]).toEqual('http://new.org/'); expect(replaceState).not.toHaveBeenCalled(); expect(locationReplace).not.toHaveBeenCalled(); @@ -430,7 +430,7 @@ describe('browser', function() { browser.url('http://new.org', true); expect(replaceState).toHaveBeenCalledOnce(); - expect(replaceState.calls.argsFor(0)[2]).toEqual('http://new.org'); + expect(replaceState.calls.argsFor(0)[2]).toEqual('http://new.org/'); expect(pushState).not.toHaveBeenCalled(); expect(locationReplace).not.toHaveBeenCalled(); @@ -474,7 +474,7 @@ describe('browser', function() { sniffer.history = false; browser.url('http://new.org', true); - expect(locationReplace).toHaveBeenCalledWith('http://new.org'); + expect(locationReplace).toHaveBeenCalledWith('http://new.org/'); expect(pushState).not.toHaveBeenCalled(); expect(replaceState).not.toHaveBeenCalled(); @@ -689,6 +689,73 @@ describe('browser', function() { expect(replaceState).not.toHaveBeenCalled(); expect(locationReplace).not.toHaveBeenCalled(); }); + + it('should not do pushState with a URL using relative protocol', function() { + browser.url('http://server/'); + + pushState.calls.reset(); + replaceState.calls.reset(); + locationReplace.calls.reset(); + + browser.url('//server'); + expect(pushState).not.toHaveBeenCalled(); + expect(replaceState).not.toHaveBeenCalled(); + expect(locationReplace).not.toHaveBeenCalled(); + }); + + it('should not do pushState with a URL only adding a trailing slash after domain', function() { + // A domain without a trailing / + browser.url('http://server'); + + pushState.calls.reset(); + replaceState.calls.reset(); + locationReplace.calls.reset(); + + // A domain from something such as window.location.href with a trailing slash + browser.url('http://server/'); + expect(pushState).not.toHaveBeenCalled(); + expect(replaceState).not.toHaveBeenCalled(); + expect(locationReplace).not.toHaveBeenCalled(); + }); + + it('should not do pushState with a URL only removing a trailing slash after domain', function() { + // A domain from something such as window.location.href with a trailing slash + browser.url('http://server/'); + + pushState.calls.reset(); + replaceState.calls.reset(); + locationReplace.calls.reset(); + + // A domain without a trailing / + browser.url('http://server'); + expect(pushState).not.toHaveBeenCalled(); + expect(replaceState).not.toHaveBeenCalled(); + expect(locationReplace).not.toHaveBeenCalled(); + }); + + it('should do pushState with a URL only adding a trailing slash after the path', function() { + browser.url('http://server/foo'); + + pushState.calls.reset(); + replaceState.calls.reset(); + locationReplace.calls.reset(); + + browser.url('http://server/foo/'); + expect(pushState).toHaveBeenCalledOnce(); + expect(fakeWindow.location.href).toEqual('http://server/foo/'); + }); + + it('should do pushState with a URL only removing a trailing slash after the path', function() { + browser.url('http://server/foo/'); + + pushState.calls.reset(); + replaceState.calls.reset(); + locationReplace.calls.reset(); + + browser.url('http://server/foo'); + expect(pushState).toHaveBeenCalledOnce(); + expect(fakeWindow.location.href).toEqual('http://server/foo'); + }); }; } }); @@ -1093,7 +1160,7 @@ describe('browser', function() { it('should not interfere with legacy browser url replace behavior', function() { inject(function($rootScope) { var current = fakeWindow.location.href; - var newUrl = 'notyet'; + var newUrl = 'http://notyet/'; sniffer.history = false; expect(historyEntriesLength).toBe(1); browser.url(newUrl, true);