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

refactor($templateRequest): move $sce checks and trust the cache #12240

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
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/ng/compile.js
Original file line number Diff line number Diff line change
Expand Up @@ -2205,7 +2205,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {

$compileNode.empty();

$templateRequest($sce.getTrustedResourceUrl(templateUrl))
$templateRequest(templateUrl)
.then(function(content) {
var compileNode, tempTemplateAttrs, $template, childBoundTranscludeFn;

Expand Down
6 changes: 3 additions & 3 deletions src/ng/directive/ngInclude.js
Original file line number Diff line number Diff line change
Expand Up @@ -178,8 +178,8 @@
* @param {Object} angularEvent Synthetic event object.
* @param {String} src URL of content to load.
*/
var ngIncludeDirective = ['$templateRequest', '$anchorScroll', '$animate', '$sce',
function($templateRequest, $anchorScroll, $animate, $sce) {
var ngIncludeDirective = ['$templateRequest', '$anchorScroll', '$animate',
function($templateRequest, $anchorScroll, $animate) {
return {
restrict: 'ECA',
priority: 400,
Expand Down Expand Up @@ -215,7 +215,7 @@ var ngIncludeDirective = ['$templateRequest', '$anchorScroll', '$animate', '$sce
}
};

scope.$watch($sce.parseAsResourceUrl(srcExp), function ngIncludeWatchAction(src) {
scope.$watch(srcExp, function ngIncludeWatchAction(src) {
var afterAnimation = function() {
if (isDefined(autoScrollExp) && (!autoScrollExp || scope.$eval(autoScrollExp))) {
$anchorScroll();
Expand Down
23 changes: 17 additions & 6 deletions src/ng/templateRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,23 +7,34 @@ var $compileMinErr = minErr('$compile');
* @name $templateRequest
*
* @description
* The `$templateRequest` service downloads the provided template using `$http` and, upon success,
* stores the contents inside of `$templateCache`. If the HTTP request fails or the response data
* of the HTTP request is empty, a `$compile` error will be thrown (the exception can be thwarted
* by setting the 2nd parameter of the function to true).
* The `$templateRequest` service runs security checks then downloads the provided template using
* `$http` and, upon success, stores the contents inside of `$templateCache`. If the HTTP request
* fails or the response data of the HTTP request is empty, a `$compile` error will be thrown (the
* exception can be thwarted by setting the 2nd parameter of the function to true). Note that the
* contents of `$templateCache` are trusted, so the call to `$sce.getTrustedUrl(tpl)` is omitted
* when `tpl` is of type string and `$templateCache` has the matching entry.
*
* @param {string} tpl The HTTP request template URL
* @param {string|TrustedResourceUrl} tpl The HTTP request template URL
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: an update on the description would be nice

* @param {boolean=} ignoreRequestError Whether or not to ignore the exception when the request fails or the template is empty
*
* @return {Promise} a promise for the HTTP response data of the given URL.
*
* @property {number} totalPendingRequests total amount of pending template requests being downloaded.
*/
function $TemplateRequestProvider() {
this.$get = ['$templateCache', '$http', '$q', function($templateCache, $http, $q) {
this.$get = ['$templateCache', '$http', '$q', '$sce', function($templateCache, $http, $q, $sce) {
function handleRequestFn(tpl, ignoreRequestError) {
handleRequestFn.totalPendingRequests++;

// We consider the template cache holds only trusted templates, so
// there's no need to go through whitelisting again for keys that already
// are included in there. This also makes Angular accept any script
// directive, no matter its name. However, we still need to unwrap trusted
// types.
if (!isString(tpl) || !$templateCache.get(tpl)) {
tpl = $sce.getTrustedResourceUrl(tpl);
}

var transformResponse = $http.defaults && $http.defaults.transformResponse;

if (isArray(transformResponse)) {
Expand Down
3 changes: 1 addition & 2 deletions src/ngRoute/route.js
Original file line number Diff line number Diff line change
Expand Up @@ -591,9 +591,8 @@ function $RouteProvider() {
if (angular.isFunction(templateUrl)) {
templateUrl = templateUrl(nextRoute.params);
}
templateUrl = $sce.getTrustedResourceUrl(templateUrl);
if (angular.isDefined(templateUrl)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a behavior change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • If the templateUrl is a string in the cache, then it is a behavior change, as it will not call getTrustedResourceUrl, but that's the point.
  • If the templateUrl is not a string or not in the cache, the call to valueOf below unwraps trusted types without side-effects, and we're just moving the call to getTrustedResourceUrl inside the call to templateRequest two lines below. You could maybe build a templateUrl equal to $sce.trustAs(SCE_CONTEXTS.RESOURCE_URL, undefined) which will get in the if block after the patch but not before, but the default $sce.trustAs implementation does not allow that, and it would only fail to load anyways.

nextRoute.loadedTemplateUrl = templateUrl;
nextRoute.loadedTemplateUrl = $sce.valueOf(templateUrl);
template = $templateRequest(templateUrl);
}
}
Expand Down
14 changes: 12 additions & 2 deletions test/ng/compileSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -1296,14 +1296,24 @@ describe('$compile', function() {
));

it('should not load cross domain templates by default', inject(
function($compile, $rootScope, $templateCache, $sce) {
function($compile, $httpBackend, $rootScope, $sce) {
Copy link
Member

Choose a reason for hiding this comment

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

Are $httpBackend and $sce needed here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are both useless, you are right. Removed.

expect(function() {
$templateCache.put('http://example.com/should-not-load.html', 'Should not load even if in cache.');
$compile('<div class="crossDomainTemplate"></div>')($rootScope);
}).toThrowMinErr('$sce', 'insecurl', 'Blocked loading resource from url not allowed by $sceDelegate policy. URL: http://example.com/should-not-load.html');
}
));

it('should trust what is already in the template cache', inject(
function($compile, $httpBackend, $rootScope, $templateCache, $sce) {
Copy link
Member

Choose a reason for hiding this comment

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

Is $sce needed here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not, I removed it.

$httpBackend.expect('GET', 'http://example.com/should-not-load.html').respond('<span>example.com/remote-version</span>');
$templateCache.put('http://example.com/should-not-load.html', '<span>example.com/cached-version</span>');
element = $compile('<div class="crossDomainTemplate"></div>')($rootScope);
expect(sortedHtml(element)).toEqual('<div class="crossDomainTemplate"></div>');
$rootScope.$digest();
expect(sortedHtml(element)).toEqual('<div class="crossDomainTemplate"><span>example.com/cached-version</span></div>');
}
));

it('should load cross domain templates when trusted', inject(
function($compile, $httpBackend, $rootScope, $sce) {
$httpBackend.expect('GET', 'http://example.com/trusted-template.html').respond('<span>example.com/trusted_template_contents</span>');
Expand Down