-
Notifications
You must be signed in to change notification settings - Fork 27.4k
refactor($templateRequest): move $sce checks and trust the cache #12240
Conversation
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 |
There was a problem hiding this comment.
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
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.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is $sce
needed here ?
There was a problem hiding this comment.
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.
@lgalfaso this likely has performance implications on |
@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. |
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
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.