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

Conversation

rjamet
Copy link
Contributor

@rjamet rjamet commented Jun 30, 2015

Move all the calls to $sce.getTrustedUrl inside $templateRequest, and
also trust the contents of the cache. This allows prefetching templates
and to bypass the checks on templates directly populated in the cache,
like for instance templates specified in script tags.

Fixes both #12219 and #12220.

Review on Reviewable

Move all the calls to $sce.getTrustedUrl inside $templateRequest, and
also trust the contents of the cache. This allows prefetching templates
and to bypass the checks on things where they make no sense, like
templates specified in script tags.

Fixes both angular#12219 and angular#12220.
@@ -12,7 +12,7 @@ var $compileMinErr = minErr('$compile');
* 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).
*
* @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

@lgalfaso
Copy link
Contributor

Overall, looks good. If possible, would like to see some numbers to know if this PR has any performance implications

The previous changes to $templateRequest were not documented, they now are.
@rjamet
Copy link
Contributor Author

rjamet commented Jun 30, 2015

I updated the description as you asked. Regarding performance, the only impact I see is that we will do more cache lookups and less white/blacklisting, but I'm not sure of the overall effect or how to measure it.

$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.

@wesleycho
Copy link
Contributor

@lgalfaso this likely has performance implications on ng-include, as the $watch is now watching an object as opposed to a string - not sure if it's considered a huge issue, but is worth noting.

@IgorMinar
Copy link
Contributor

@wesleycho, @lgalfaso I don't expect any significant perf impact. the watch in ngInclude should not cause problems because ngInclude doesn't do any deep watching.

@IgorMinar IgorMinar closed this in 3c6e8ce Jul 1, 2015
IgorMinar pushed a commit that referenced this pull request Jul 1, 2015
Move all the calls to $sce.getTrustedUrl inside $templateRequest, and
also trust the contents of the cache. This allows prefetching templates
and to bypass the checks on things where they make no sense, like
templates specified in script tags.

Closes #12219
Closes #12220
Closes #12240
netman92 pushed a commit to netman92/angular.js that referenced this pull request Aug 8, 2015
Move all the calls to $sce.getTrustedUrl inside $templateRequest, and
also trust the contents of the cache. This allows prefetching templates
and to bypass the checks on things where they make no sense, like
templates specified in script tags.

Closes angular#12219
Closes angular#12220
Closes angular#12240
@rjamet rjamet deleted the validation-move-cache branch September 8, 2015 09:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants