Skip to content

Commit 53ab8bc

Browse files
gkalpakgkalpak
committed
fix($routeProvider): do not deep-copy route definition objects
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
1 parent 04cad41 commit 53ab8bc

6 files changed

Lines changed: 119 additions & 28 deletions

File tree

‎angularFiles.js‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ var angularFiles = {
55
'src/minErr.js',
66
'src/Angular.js',
77
'src/loader.js',
8+
'src/shallowCopy.js',
89
'src/stringify.js',
910
'src/AngularPublic.js',
1011
'src/jqLite.js',
@@ -128,6 +129,7 @@ var angularFiles = {
128129
'src/ngResource/resource.js'
129130
],
130131
'ngRoute': [
132+
'src/shallowCopy.js',
131133
'src/ngRoute/route.js',
132134
'src/ngRoute/routeParams.js',
133135
'src/ngRoute/directive/ngView.js'

‎src/Angular.js‎

Lines changed: 0 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,6 @@
5757
includes: true,
5858
arrayRemove: true,
5959
copy: true,
60-
shallowCopy: true,
6160
equals: true,
6261
csp: true,
6362
jq: true,
@@ -933,31 +932,6 @@ function copy(source, destination) {
933932
}
934933
}
935934

936-
/**
937-
* Creates a shallow copy of an object, an array or a primitive.
938-
*
939-
* Assumes that there are no proto properties for objects.
940-
*/
941-
function shallowCopy(src, dst) {
942-
if (isArray(src)) {
943-
dst = dst || [];
944-
945-
for (var i = 0, ii = src.length; i < ii; i++) {
946-
dst[i] = src[i];
947-
}
948-
} else if (isObject(src)) {
949-
dst = dst || {};
950-
951-
for (var key in src) {
952-
if (!(key.charAt(0) === '$' && key.charAt(1) === '$')) {
953-
dst[key] = src[key];
954-
}
955-
}
956-
}
957-
958-
return dst || src;
959-
}
960-
961935

962936
/**
963937
* @ngdoc function

‎src/ngRoute/route.js‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,11 @@
11
'use strict';
22

3+
/* global shallowCopy: false */
4+
5+
// There are necessary for `shallowCopy()` (included via `src/shallowCopy.js`)
6+
var isArray = angular.isArray;
7+
var isObject = angular.isObject;
8+
39
/**
410
* @ngdoc module
511
* @name ngRoute
@@ -160,7 +166,7 @@ function $RouteProvider() {
160166
*/
161167
this.when = function(path, route) {
162168
//copy original route object to preserve params inherited from proto chain
163-
var routeCopy = angular.copy(route);
169+
var routeCopy = shallowCopy(route);
164170
if (angular.isUndefined(routeCopy.reloadOnSearch)) {
165171
routeCopy.reloadOnSearch = true;
166172
}

‎src/shallowCopy.js‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
'use strict';
2+
3+
/* global shallowCopy: true */
4+
5+
/**
6+
* Creates a shallow copy of an object, an array or a primitive.
7+
*
8+
* Assumes that there are no proto properties for objects.
9+
*/
10+
function shallowCopy(src, dst) {
11+
if (isArray(src)) {
12+
dst = dst || [];
13+
14+
for (var i = 0, ii = src.length; i < ii; i++) {
15+
dst[i] = src[i];
16+
}
17+
} else if (isObject(src)) {
18+
dst = dst || {};
19+
20+
for (var key in src) {
21+
if (!(key.charAt(0) === '$' && key.charAt(1) === '$')) {
22+
dst[key] = src[key];
23+
}
24+
}
25+
}
26+
27+
return dst || src;
28+
}

‎src/stringify.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
'use strict';
22

3-
/* global: toDebugString: true */
3+
/* global toDebugString: true */
44

55
function serializeObject(obj) {
66
var seen = [];

‎test/ngRoute/routeSpec.js‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -900,6 +900,87 @@ describe('$route', function() {
900900
});
901901

902902

903+
it('should not get affected by modifying the route definition object after route registration',
904+
function() {
905+
module(function($routeProvider) {
906+
var rdo = {};
907+
908+
rdo.templateUrl = 'foo.html';
909+
$routeProvider.when('/foo', rdo);
910+
911+
rdo.templateUrl = 'bar.html';
912+
$routeProvider.when('/bar', rdo);
913+
});
914+
915+
inject(function($location, $rootScope, $route) {
916+
$location.path('/bar');
917+
$rootScope.$digest();
918+
expect($location.path()).toBe('/bar');
919+
expect($route.current.templateUrl).toBe('bar.html');
920+
921+
$location.path('/foo');
922+
$rootScope.$digest();
923+
expect($location.path()).toBe('/foo');
924+
expect($route.current.templateUrl).toBe('foo.html');
925+
});
926+
}
927+
);
928+
929+
930+
it('should use the property values of the passed in route definition object directly',
931+
function() {
932+
var $routeProvider;
933+
934+
module(function(_$routeProvider_) {
935+
$routeProvider = _$routeProvider_;
936+
});
937+
938+
inject(function($location, $rootScope, $route, $sce) {
939+
var sceWrappedUrl = $sce.trustAsResourceUrl('foo.html');
940+
$routeProvider.when('/foo', {templateUrl: sceWrappedUrl});
941+
942+
$location.path('/foo');
943+
$rootScope.$digest();
944+
expect($location.path()).toBe('/foo');
945+
expect($route.current.templateUrl).toBe(sceWrappedUrl);
946+
});
947+
}
948+
);
949+
950+
951+
it('should support custom `$sce` implementations', function() {
952+
function MySafeResourceUrl(val) {
953+
var self = this;
954+
this._val = val;
955+
this.getVal = function() {
956+
return (this !== self) ? null : this._val;
957+
};
958+
}
959+
960+
var $routeProvider;
961+
962+
module(function($provide, _$routeProvider_) {
963+
$routeProvider = _$routeProvider_;
964+
965+
$provide.decorator('$sce', function($delegate) {
966+
$delegate.trustAsResourceUrl = function(url) { return new MySafeResourceUrl(url); };
967+
$delegate.getTrustedResourceUrl = function(v) { return v.getVal(); };
968+
$delegate.valueOf = function(v) { return v.getVal(); };
969+
return $delegate;
970+
});
971+
});
972+
973+
inject(function($location, $rootScope, $route, $sce) {
974+
$routeProvider.when('/foo', {templateUrl: $sce.trustAsResourceUrl('foo.html')});
975+
976+
$location.path('/foo');
977+
$rootScope.$digest();
978+
expect($location.path()).toBe('/foo');
979+
expect($sce.valueOf($route.current.templateUrl)).toBe('foo.html');
980+
});
981+
});
982+
983+
903984
describe('redirection', function() {
904985
it('should support redirection via redirectTo property by updating $location', function() {
905986
module(function($routeProvider) {

0 commit comments

Comments
 (0)