Fix sce context #15039
Fix sce context #15039
Conversation
…ack. Previously, video, audio, and track sources were $sce.RESOURCE_URL. This is not justified as no attacks are possible through these attributes as far as I can tell. This change is not breaking, and uses of $sce.trustAsResourceUrl before assigning to src or ng-src attributes will just be silently ignored.
Older msies reject my video element test, and format things better, no more tests for video[src] contexts under a img[src] sanitization describe.
The context reduction didn't test retrocompatibility with trustAsResourceUrl(...) values.
Track files might contain CSS, and haven't been around for long. Keeping the high security context as a precaution is justified.
Source src is only for media files (videos, audio, images), and present no known script execution possibilities. We also don't expect new ones to pop up, as this tags is only supported on recent browsers.
Forgot to update the $sce doc, now done.
| @@ -10152,6 +10151,32 @@ describe('$compile', function() { | |||
| expect(element.attr('src')).toEqual('http://example.com/image2.png'); | |||
| })); | |||
|
|
|||
| // Older IEs seem to reject the video tag with "Error: Not implemented" | |||
| // if (!msie || msie > 9) { | |||
There was a problem hiding this comment.
This seems necessary. Tests are failing on IE9 without it.
There was a problem hiding this comment.
yes, I was just checking if all of the elemnts fail
Narretz
force-pushed
the
fix-sce-context
branch
from
1278b7a to
b775385
Compare
| @@ -10152,6 +10151,33 @@ describe('$compile', function() { | |||
| expect(element.attr('src')).toEqual('http://example.com/image2.png'); | |||
| })); | |||
|
|
|||
| // IE9 rejects the video / audio tag with "Error: Not implemented" and the source tag with | |||
| // "Unable to get value of the property 'childNodes': object is null or undefined" | |||
There was a problem hiding this comment.
Why is that so? IE 9 supports video & audio elements.
There was a problem hiding this comment.
But not if we try to do it like we do in the test. It's possible that we need different HTML for it to work: http://stackoverflow.com/questions/7358689/audio-tag-not-working-in-ie9
There was a problem hiding this comment.
The OP answered that adding a DOCTYPE helped. Do our test cases not have a DOCTYPE? If that's the case we should definitely add them, we should make sure all our tests are running in standards mode, not quirks mode that we don't even officially support AFAIK.
All IE versions older than 10 in quirks mode invoke a separate engine simulating IE 5.5 and, as a consequence, missing features from newer versions (including <video> and <audio>, obviously).
There was a problem hiding this comment.
I've looked at a couple of pages used for e2e tests and they all have a DOCTYPE. The template used by Karma also specifies a DOCTYPE. So this shouldn't be a problem.
There was a problem hiding this comment.
Maybe it's because of this? videojs/video.js#290 (comment)
|
Can I merge this in the meantime or do you want this resolved first @mgol? |
|
@Narretz Go ahead & merge, we can look into it later since it requires further investigation, apparently. |
|
Hey, sorry, I left for a few days right when you started the conversation :/ As you spotted, there won't be a $sce context anymore for these three. I think having no sanitization there is fine since no browser will execute script there, but if you want consistency you'd just need to add them where sanitization happens: https://github.com/angular/angular.js/blob/master/src/ng/compile.js#L1631 . Also, I didn't have time to update this PR, but for Angular 2, we ended up putting track src in the same basket. I was a little concerned about the video subset of CSS, but after looking at the specs and testings, it looks harmless (angular/angular#10187) |
|
@rjamet Ok, I will then re-add track and merge the PR |
…ce, track Previously, video, audio, source, and track sources were $sce.RESOURCE_URL. This is not justified as no attacks (script execution) are possible through these attributes as far as we can tell. Angular2 also uses the same categorization. This change is not breaking, and uses of $sce.trustAsResourceUrl before assigning to src or ng-src attributes will just be silently ignored. This has also been given a LGTM by @mprobst via email. Commit 4853201 on the master branch contains the same changes, but is missing this commit description. This commit does not backport the BC introduced in 04cad41. PR (#15039) Closes #14019
…ce, track Previously, video, audio, source, and track sources were $sce.RESOURCE_URL. This is not justified as no attacks (script execution) are possible through these attributes as far as we can tell. Angular2 also uses the same categorization. This change is not breaking, and uses of $sce.trustAsResourceUrl before assigning to src or ng-src attributes will just be silently ignored. This has also been given a LGTM by @mprobst via email. Commit 4853201 on the master branch contains the same changes, but is missing this commit description. This commit does not backport the BC introduced in 04cad41. PR (angular#15039) Closes angular#14019
…ce, track Previously, video, audio, source, and track sources were $sce.RESOURCE_URL. This is not justified as no attacks (script execution) are possible through these attributes as far as we can tell. Angular2 also uses the same categorization. This change is not breaking, and uses of $sce.trustAsResourceUrl before assigning to src or ng-src attributes will just be silently ignored. This has also been given a LGTM by @mprobst via email. Commit 4853201 on the master branch contains the same changes, but is missing this commit description. This commit does not backport the BC introduced in 04cad41. PR (angular#15039) Closes angular#14019
rebased and slightly tweaked #14019