Skip to content

Commit

Permalink
fix($routeProvider): do not deep-copy route definition objects
Browse files Browse the repository at this point in the history
Deep-copying route definition objects can break specific custom implementations of `$sce` (used to
trust a `templateUrl` as RESOURCE_URL). The purpose of copying route definition objects was to guard
against the user's modifying the route definition object after route registration, while still
capturing inherited properties.
As suggested by @IgorMinar in angular#14699 (comment),
we can achieve both _and_ support custom `$sce` implementations, by shallow-copying instead.

This is an alternative implementation for angular#14699, which avoids the breaking change.

Fixes angular#14478
Closes angular#14699

Closes angular#14750
  • Loading branch information
gkalpak committed Jun 10, 2016
1 parent 04cad41 commit 53ab8bc
Show file tree
Hide file tree
Showing 6 changed files with 119 additions and 28 deletions.
2 changes: 2 additions & 0 deletions angularFiles.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ var angularFiles = {
'src/minErr.js',
'src/Angular.js',
'src/loader.js',
'src/shallowCopy.js',
'src/stringify.js',
'src/AngularPublic.js',
'src/jqLite.js',
Expand Down Expand Up @@ -128,6 +129,7 @@ var angularFiles = {
'src/ngResource/resource.js'
],
'ngRoute': [
'src/shallowCopy.js',
'src/ngRoute/route.js',
'src/ngRoute/routeParams.js',
'src/ngRoute/directive/ngView.js'
Expand Down
26 changes: 0 additions & 26 deletions src/Angular.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@
includes: true,
arrayRemove: true,
copy: true,
shallowCopy: true,
equals: true,
csp: true,
jq: true,
Expand Down Expand Up @@ -933,31 +932,6 @@ function copy(source, destination) {
}
}

/**
* Creates a shallow copy of an object, an array or a primitive.
*
* Assumes that there are no proto properties for objects.
*/
function shallowCopy(src, dst) {
if (isArray(src)) {
dst = dst || [];

for (var i = 0, ii = src.length; i < ii; i++) {
dst[i] = src[i];
}
} else if (isObject(src)) {
dst = dst || {};

for (var key in src) {
if (!(key.charAt(0) === '$' && key.charAt(1) === '$')) {
dst[key] = src[key];
}
}
}

return dst || src;
}


/**
* @ngdoc function
Expand Down
8 changes: 7 additions & 1 deletion src/ngRoute/route.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
'use strict';

/* global shallowCopy: false */

// There are necessary for `shallowCopy()` (included via `src/shallowCopy.js`)
var isArray = angular.isArray;
var isObject = angular.isObject;

/**
* @ngdoc module
* @name ngRoute
Expand Down Expand Up @@ -160,7 +166,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 = shallowCopy(route);
if (angular.isUndefined(routeCopy.reloadOnSearch)) {
routeCopy.reloadOnSearch = true;
}
Expand Down
28 changes: 28 additions & 0 deletions src/shallowCopy.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
'use strict';

/* global shallowCopy: true */

/**
* Creates a shallow copy of an object, an array or a primitive.
*
* Assumes that there are no proto properties for objects.
*/
function shallowCopy(src, dst) {
if (isArray(src)) {
dst = dst || [];

for (var i = 0, ii = src.length; i < ii; i++) {
dst[i] = src[i];
}
} else if (isObject(src)) {
dst = dst || {};

for (var key in src) {
if (!(key.charAt(0) === '$' && key.charAt(1) === '$')) {
dst[key] = src[key];
}
}
}

return dst || src;
}
2 changes: 1 addition & 1 deletion src/stringify.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use strict';

/* global: toDebugString: true */
/* global toDebugString: true */

function serializeObject(obj) {
var seen = [];
Expand Down
81 changes: 81 additions & 0 deletions test/ngRoute/routeSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -900,6 +900,87 @@ describe('$route', function() {
});


it('should not get affected by modifying the route definition object after route registration',
function() {
module(function($routeProvider) {
var rdo = {};

rdo.templateUrl = 'foo.html';
$routeProvider.when('/foo', rdo);

rdo.templateUrl = 'bar.html';
$routeProvider.when('/bar', rdo);
});

inject(function($location, $rootScope, $route) {
$location.path('/bar');
$rootScope.$digest();
expect($location.path()).toBe('/bar');
expect($route.current.templateUrl).toBe('bar.html');

$location.path('/foo');
$rootScope.$digest();
expect($location.path()).toBe('/foo');
expect($route.current.templateUrl).toBe('foo.html');
});
}
);


it('should use the property values of the passed in route definition object directly',
function() {
var $routeProvider;

module(function(_$routeProvider_) {
$routeProvider = _$routeProvider_;
});

inject(function($location, $rootScope, $route, $sce) {
var sceWrappedUrl = $sce.trustAsResourceUrl('foo.html');
$routeProvider.when('/foo', {templateUrl: sceWrappedUrl});

$location.path('/foo');
$rootScope.$digest();
expect($location.path()).toBe('/foo');
expect($route.current.templateUrl).toBe(sceWrappedUrl);
});
}
);


it('should support custom `$sce` implementations', function() {
function MySafeResourceUrl(val) {
var self = this;
this._val = val;
this.getVal = function() {
return (this !== self) ? null : this._val;
};
}

var $routeProvider;

module(function($provide, _$routeProvider_) {
$routeProvider = _$routeProvider_;

$provide.decorator('$sce', function($delegate) {
$delegate.trustAsResourceUrl = function(url) { return new MySafeResourceUrl(url); };
$delegate.getTrustedResourceUrl = function(v) { return v.getVal(); };
$delegate.valueOf = function(v) { return v.getVal(); };
return $delegate;
});
});

inject(function($location, $rootScope, $route, $sce) {
$routeProvider.when('/foo', {templateUrl: $sce.trustAsResourceUrl('foo.html')});

$location.path('/foo');
$rootScope.$digest();
expect($location.path()).toBe('/foo');
expect($sce.valueOf($route.current.templateUrl)).toBe('foo.html');
});
});


describe('redirection', function() {
it('should support redirection via redirectTo property by updating $location', function() {
module(function($routeProvider) {
Expand Down

0 comments on commit 53ab8bc

Please sign in to comment.