Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix(ngRoute): Don't deep-copy the route object, just plain copy it instead. #14699

Closed
wants to merge 2 commits into from
Closed

fix(ngRoute): Don't deep-copy the route object, just plain copy it instead. #14699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6eb9b01
fix(ngRoute): Don't deep-copy the route object, just copy it instead.
rjamet May 31, 2016
f527578
fix(ngRoute): jshint complaints
rjamet May 31, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/ngRoute/route.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ function $RouteProvider() {
*/
this.when = function(path, route) {
//copy original route object to preserve params inherited from proto chain
var routeCopy = angular.copy(route);
var routeCopy = route;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that the intention of the copying the object was to avoid the possibility of the user mutating the passed in object after the router partially processed it. This could get the router into an unknown state.

By copying we know for sure that the state we read has not changed.

The problem this causes is that if the templateUrl is set to be a TrustedResourceUrl instance, the instance is not copied correctly and the templateUrl fails to be properly unwrapped by sce later.

Since both template and templateUrl properties are both (wrapped) immutable strings, we could use shallowCopy instead to protect the router from internal state corruption while not corrupting the sce wrappers.

Another option is to teach angular.copy how to copy sce wrappers safely, but given that there could be custom implementations of these wrappers (e.g. at google we have our own), making the the mechanism generic might be tricky.

shallowCopy seems to give us all of the benefits with little down-sides, so I'd prefer that one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the intention of using copy instead of extend was to preserve inherited properties (talk about using a sledgehammer to crack a nut 😃). shallowCopy (which we need to copy over to angular-route btw) would also shallow-copy inherited properties, so we are covered.

if (angular.isUndefined(routeCopy.reloadOnSearch)) {
routeCopy.reloadOnSearch = true;
}
Expand Down
33 changes: 33 additions & 0 deletions test/ngRoute/routeSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1297,6 +1297,39 @@ describe('$route', function() {
});
});


it('should not deep-copy or alter route fields', function() {
// Object.seal is used to block future modifications of the object: we do not want
// to alter the objects passed as parameters.
var canaryObject = Object.seal({});

// No point faking a full template, use the syntax from above
var canaryTemplate = function() {
return "<p>hi</p>";
};
canaryTemplate.someField = canaryObject;
Object.seal(canaryTemplate); // sealed as well, just in case

module(function($routeProvider) {
$routeProvider.when('/foo',
{
template: canaryTemplate,
myObject: canaryObject
});
});

inject(function($route, $location, $rootScope) {
$location.path('/foo');
$rootScope.$digest();

// Copies of the canaries won't be their originals.
expect($route.current.myObject).toBe(canaryObject);
expect($route.current.template).toBe(canaryTemplate);
expect($route.current.template.someField).toBe(canaryObject);
});
});


describe('reload', function() {
var $location;
var $log;
Expand Down