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

feat($sce): handle URLs through the $sce service #16378

Merged
merged 1 commit into from
Jan 31, 2018

Conversation

petebacondarwin
Copy link
Contributor

@petebacondarwin petebacondarwin commented Dec 19, 2017

Replaces #14250

Thanks to @rjamet for the original work on this feature.

This is a large patch to handle URLs with the $sce service, similarly to HTML context.

However, to keep rough compatibility with existing apps, we need to allow URL-context
concatenation, since previously $sce contexts prevented sanitization. There's now a
set of special contexts (defined in $interpolate) that allow concatenation, in a roughly
intuitive way:

  • Trusted types alone are trusted, e.g. "{{safeType}}" will not be sanitized.
  • Any concatenation of values results in a non-trusted type that will be handled by
    getTrusted once the concatenation is done, e.g. "{{ 'javascript:foo' }}" and
    "javascript:{{safeType}}" will be sanitized.

This commit also introduces a new SCE context called SRC, which represents a URL
being used as a source for an image, video, audio, etc. The hierarchy is setup
so that the URL context is also a SRC context, in the same way that the RESOURCE_URL
context is also a URL (and now also a SRC) context.

Where we previously sanitized URL attributes inside the compiler, we now only apply
the $sce.URL or $sce.SRC context requirement.

  • When calling getTrustedSrc() a value that is not already a trusted SRC will be
    sanitized using the imgSrcSanitizationWhitelist
  • When calling getTrustedUrl() a value that is not already a trusted URL will be
    sanitized using the aHrefSanitizationWhitelist

This results in behaviour that closely matches the previous sanitization behaviour.

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

feature

What is the current behavior? (You can also link to an open issue here)

URLs are sanitized directly in the compiler or interpolator.

What is the new behavior (if this is a feature change)?

Now that sanitization is done via the SCE service.

Does this PR introduce a breaking change?
Yes

If you use attrs.$set for URL attributes there will be no automated sanitization
of the URL value. This is now in line with other contexts. If you are programmatically
writing URL values to attributes from untrusted input then you must sanitize
it yourself (possibly by calling the private $$sanitizeUri service).

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Fix/Feature: Docs have been added/updated
  • Fix/Feature: Tests have been added; existing tests pass

Other information:

@@ -283,27 +289,55 @@ function $InterpolateProvider() {
}
}

if (concat.length === 1 && expressionPositions.length === 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove the initial singleExpression assignment and change this to singleExpression = (concat.length === 1 && expressionPositions.length === 1)?

// doesn't allow that.
$interpolateMinErr.throwNoconcat(text);
} else {
return concat.join('');
Copy link
Contributor

Choose a reason for hiding this comment

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

Drop this (and the next else) and just fallthru? Could then combine the condition above to trustedContext && concat.length > 1...

return allOrNothing && !isDefined(value) ? value : stringify(value);
if (contextAllowsConcatenation && singleExpression) {
// No stringification in this case, to keep the trusted value until unwrapping.
return value;
Copy link
Contributor

Choose a reason for hiding this comment

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

This basically makes parseStringifyInterceptor a noop, can we avoid parseStringifyInterceptor (and any interceptor) completely in that case? Maybe only if that case is common enough that it's worth avoiding interceptors...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Difficulty is that we assign the interpolator to the parseFn before we have calculated how many items are in contat and expressionPositions so we don't know whether singleExpression is true until we have finished.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I managed it...

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's difficult or ugly then we can leave it out of this PR and try later since it's not really directly related to this PR

@petebacondarwin petebacondarwin force-pushed the sceUrl branch 2 times, most recently from a1611a4 to 84750b9 Compare December 20, 2017 13:44
@@ -19,7 +20,8 @@ function $$SanitizeUriProvider() {
* Any url about to be assigned to a[href] via data-binding is first normalized and turned into
* an absolute url. Afterwards, the url is matched against the `aHrefSanitizationWhitelist`
* regular expression. If a match is found, the original url is written into the dom. Otherwise,
* the absolute url is prefixed with `'unsafe:'` string and only then is it written into the DOM.
* the absolute url is prefixed with `'unsafe:'` string and only then is it written into the DOM,
* making it inactive.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you maybe document that those are used in the $sce.URL sanitization now, so it's reflected in the API docs ? (and same below)

src/ng/sce.js Outdated
@@ -22,8 +22,11 @@ var SCE_CONTEXTS = {
// Style statements or stylesheets. Currently unused in AngularJS.
CSS: 'css',

// An URL used in a context where it does not refer to a resource that loads code. Currently
// unused in AngularJS.
// An URL used in a context where it refers to the source of media such as an image, audio, video, etc.
Copy link
Contributor

Choose a reason for hiding this comment

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

If you push it a little, iframes and scripts are media. I'd say "to the source of media which are not expected to run scripts such as ..." instead.

src/ng/sce.js Outdated
* the context and sanitizer availablility.
*
* The contexts that can be sanitized are $sce.MEDIA_URL, $sce.URL and $sce.HTML. The first two are available
* by default, and the second one relies on the $sanitize service (which may be loaded through
Copy link
Contributor

Choose a reason for hiding this comment

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

s/second/third/

src/ng/sce.js Outdated
* is available or possible.)
* This function will throw if the safe type isn't appropriate for this context, or if the
* value given cannot be accepted in the context (which might be caused by sanitization not
* being available, or the value being unsafe).
Copy link
Contributor

Choose a reason for hiding this comment

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

"the value not being recognized as safe" instead?

src/ng/sce.js Outdated
if (type === SCE_CONTEXTS.MEDIA_URL) {
return $$sanitizeUri(maybeTrusted, true);
} else if (type === SCE_CONTEXTS.URL) {
return $$sanitizeUri(maybeTrusted);
Copy link
Contributor

Choose a reason for hiding this comment

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

explicit false as a second argument there ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, OK then :-) I was saving bytes.

@@ -44,7 +46,8 @@ function $$SanitizeUriProvider() {
* Any url about to be assigned to img[src] via data-binding is first normalized and turned into
* an absolute url. Afterwards, the url is matched against the `imgSrcSanitizationWhitelist`
* regular expression. If a match is found, the original url is written into the dom. Otherwise,
* the absolute url is prefixed with `'unsafe:'` string and only then is it written into the DOM.
* the absolute url is prefixed with `'unsafe:'` string and only then is it written into the DOM,
* making it inactive.
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Below in this file, I would replace the isImage argument with isMediaUrl to be clearer. That shouldn't break anything.

@@ -6,6 +6,7 @@
* Private service to sanitize uris for links and images. Used by $compile and $sanitize.
*/
function $$SanitizeUriProvider() {

var aHrefSanitizationWhitelist = /^\s*(https?|s?ftp|mailto|tel|file):/,
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a tiny quirk hiding there, which is probably without consequences: mailto and tel are in the URL whitelist, but not in the MEDIA_URL. This makes sense, but "safe under URL" is supposed to be a subset of "safe under MEDIA_URL" in my interpretation of the subclassing in the $sce. I don't think anyone cares either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I wondered about that. One is not really a "super-set" of the other but I needed both to be covered by RESOURCE_URL, so I had to force a hierarchy on them. Given that users can manipulate these whitelists in anyway they like, I don't feel too bad about the defaults not quite aligning.

@@ -154,7 +153,7 @@ function $SanitizeProvider() {
return function(html) {
var buf = [];
htmlParser(html, htmlSanitizeWriter(buf, function(uri, isImage) {
return !/^unsafe:/.test($$sanitizeUri(uri, isImage));
return !/^unsafe:/.test($$sanitizeUri(uri), isImage);
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks wrong to me?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but no test is failing 😱

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$rootScope.testUrl = $sce.trustAsUrl('http://example.com/image2.mp4');
it('should accept trusted values', inject(function($rootScope, $compile, $sce) {
// As a MEDIA_URL URL
element = $compile('<' + tag + ' src="{{testUrl}}"></' + tag + '>')($rootScope);
Copy link
Contributor

@rjamet rjamet Dec 20, 2017

Choose a reason for hiding this comment

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

I think the CI errors you get are due to some browser protections on assigning javascript: to the src attributes of image elements... Those are annoying, I remember hitting these on the IE family before (and that was why I didn't bother with img there).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I wondered about that. I'll remove the javascript:s

@rjamet
Copy link
Contributor

rjamet commented Dec 20, 2017

LGTM overall, nice work!

@@ -283,29 +284,40 @@ function $InterpolateProvider() {
}
}

singleExpression = concat.length === 1 && expressionPositions.length === 1;
parseFns = contextAllowsConcatenation && singleExpression ?
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be any shorter/nicer doing...

parseFns = expressions.map(function(exp) {
    return $parse(exp, contextAllowsConcatenation && singleExpression ? undefined : parseStringifyInterceptor);
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that. It does mean evaluating that contextAllowsConcatenation && singleExpression for each item in concat. Also I was being cautious in case the $parse was doing parameter counting rather than just a check for undefined. If you feel strongly about it we could change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it reads a little nicer but don't feel strongly about it.

Could do var interceptor = ... outside the loop. That might also be a little more readable which I think is most important here (I don't think performance is a major concern since this isn't run per digest or anything like that).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@Narretz Narretz left a comment

Choose a reason for hiding this comment

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

As far as I can say, this looks reasonable, although I find it a bit weird that the strategy for sanitizing content that is programmtically written is to use the private $$sanitizeUri service. This feels wrong. Maybe we should make the service public?

The commit message would also be clearer if this section:

Where we previously sanitized URL attributes when setting attribute value inside the
$compile service, we now only apply the $sce.URL or $sce.SRC context requirement,
and leave the $interpolate service to deal with sanitization.

would come immediately after

This is a large patch to handle URLs with the $sce service, similarly to HTML context.

Btw,

we now only apply the $sce.URL or $sce.SRC context requirement

I don't think $sce.SRC is anywhere in this patch, is this line right?

value = getValue(value);
// In concatenable contexts, getTrusted comes at the end, to avoid sanitizing individual
// parts of a full URL. We don't care about losing the trustedness here.
// If non-concatenable contexts where there is only one expression then this interceptor
Copy link
Contributor

Choose a reason for hiding this comment

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

If > In?

src/ng/sce.js Outdated
* <div class="alert alert-warning">
* Be aware that `a[href]` and `img[src]` used to automatically sanitize their URLs and not pass them
* through {@link ng.$sce#getTrusted $sce.getTrusted}. **As of 1.7.0, this is no longer the case.**
* Now `getTrusted` will sanitize values for the `$sce.MEDIA_URL` and `$sce.URL` contexts.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear which context applies to what. How about "Now getTrusted will sanitize a[href] for the $sce.URL context, and img[src] for the $sce.MEDIA?URL context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not strictly related to DOM nodes. What this is saying is that, previously the a[href] and img[src] did not use $sce at all but did their own sanitization and that $sce did not do anything with the $sce.URL (and now also $sce.MEDIA_URL) contexts. Now the a[href] and img[src] do nothing but require the URL or MEDIA_URL context. It is $sce that now does the sanitizing as appropriate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, but the paragraph makes it sound like (at least to me) that the second part still refers to a[href] and img[src], and as such it's confusing that the last sentence isn't clear about which context gets applied to them. So I think adding the explicit context matching is a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doc comments updated

expect($sce.getTrustedUrl($sce.trustAsResourceUrl('javascript:foo'))).toEqual('javascript:foo');
}));

it('should use the $$sanitizeUri', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this technically an implementation detail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I see it as an interface (albeit internal) to the service. Testing it this way means that we are less tied to the implementation of the $$sanitizeUri service.

@petebacondarwin petebacondarwin force-pushed the sceUrl branch 3 times, most recently from 974f10e to 54c8c0f Compare December 22, 2017 12:51
@petebacondarwin petebacondarwin force-pushed the sceUrl branch 6 times, most recently from 6389ae3 to 90952ba Compare December 23, 2017 10:09
src/ng/sce.js Outdated
MEDIA_URL: 'mediaUrl',

// An URL used in a context where it does not refer to a resource that loads code.
// A value that is trusted as a URL is also trusted as a MEDIA_URL.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe changing it to ...can also be trusted... would be more accurate.
(It conveys the hierarchy relation without saying it is trusted, because it may not be depending on the whitelists.)

src/ng/sce.js Outdated
* | `$sce.JS` | For JavaScript that is safe to execute in your application's context. Currently, no bindings require this context. Feel free to use it in your own directives. |
* | `$sce.HTML` | For HTML that's safe to source into the application. The {@link ng.directive:ngBindHtml ngBindHtml} directive uses this context for bindings. If an unsafe value is encountered and the {@link ngSanitize $sanitize} module is present this will sanitize the value instead of throwing an error. |
* | `$sce.CSS` | For CSS that's safe to source into the application. Currently unused. Feel free to use it in your own directives. |
* | `$sce.MEDIA_URL` | For URLs that are safe to display as media. Is automatically converted from string by sanitizing when needed. |
Copy link
Member

Choose a reason for hiding this comment

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

AFAICT, "display" does not cover all usecases (e.g. audio) and might be confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going with render it is vague enough to cover audio IMO.

src/ng/sce.js Outdated
* | `$sce.CSS` | For CSS that's safe to source into the application. Currently unused. Feel free to use it in your own directives. |
* | `$sce.MEDIA_URL` | For URLs that are safe to display as media. Is automatically converted from string by sanitizing when needed. |
* | `$sce.URL` | For URLs that are safe to follow as links. Is automatically converted from string by sanitizing when needed. |
* | `$sce.RESOURCE_URL` | For URLs that are not only safe to follow as links, but whose contents are also safe to include in your application. Examples include `ng-include`, `src` / `ngSrc` bindings for tags other than `IMG` (e.g. `IFRAME`, `OBJECT`, etc.) <br><br>Note that `$sce.RESOURCE_URL` makes a stronger statement about the URL than `$sce.URL` does and therefore contexts requiring values trusted for `$sce.RESOURCE_URL` can be used anywhere that values trusted for `$sce.URL` are required. |
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you could mention MEDIA_URL here or in $sce.URL.



it('should interpolate and sanitize a multi-part expression when isTrustedContext is URL', inject(function($sce, $interpolate) {
/* jshint scripturl:true */
Copy link
Member

Choose a reason for hiding this comment

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

jshint???

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:-) picked up from @rjamet's original PR that was written before we moved to eslint. Removed

expect(function() {
element = $compile('<a ng-href="http://www.google.com/{{\'a%link\'}}">')($rootScope);
Copy link
Member

Choose a reason for hiding this comment

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

Is this a style-only change, or has the behavior changed?

Copy link
Contributor Author

@petebacondarwin petebacondarwin Jan 28, 2018

Choose a reason for hiding this comment

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

The behaviour has changed. The throw is now in the call to $compile.

* the $sce.MEDIA_URL security context. When interpolation occurs a call is made to `$sce.trustAsUrl(url)`
* which in turn may call `$$sanitizeUri(url, isMedia)` to sanitize the potentially malicious URL.
*
* If the URL to matches the `aHrefSanitizationWhitelist` regular expression, it is returned unchanged.
Copy link
Member

Choose a reason for hiding this comment

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

URL to matches --> URL matches
aHrefSanitizationWhitelist --> imgSrcSanitizationWhitelist

var normalizedVal;
normalizedVal = urlResolve(uri && uri.trim()).href;
return function sanitizeUri(uri, isMediaUrl) {
if (!uri) return uri;
Copy link
Member

Choose a reason for hiding this comment

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

AFAICT, this changes the current behavior. Previously, even if the URL was the empty string, it might return a non-empty URL (i.e. the current URL). Do I miss something? Is this change intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, you're right that urlResolve doesn't (mostly) return the empty string when passed an empty string... I need to look again at why I did this. There was definitely a reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't remember (or see) why I did this. So I am going to revert that change.

@@ -436,7 +436,7 @@ forEach(['src', 'srcset', 'href'], function(attrName) {
// On IE, if "ng:src" directive declaration is used and "src" attribute doesn't exist
// then calling element.setAttribute('src', 'foo') doesn't do anything, so we need
// to set the property as well to achieve the desired effect.
// We use attr[attrName] value since $set can sanitize the url.
// We use attr[attrName] value since $set might have sanitized the url.
Copy link
Member

Choose a reason for hiding this comment

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

Based on the commit message, I thought $set() doesn't sanitize it any more 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still sanitize img[srcset] when called through $set.

(nodeName === 'img' && key === 'src') ||
(nodeName === 'image' && key === 'xlinkHref')) {
// sanitize a[href] and img[src] values
this[key] = value = $$sanitizeUri(value, nodeName === 'img' || nodeName === 'image');
Copy link
Member

@gkalpak gkalpak Jan 26, 2018

Choose a reason for hiding this comment

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

So, doesn't this leave people using ng-href/src lessmore vulnerable (unless they use interpolation)?
(Not common to use ng-href/src without interpollation, but possible.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure what you mean here. Do you mean "more" vulnerable?
It is true that if you were programmatically setting the attribute via $set then it is no longer checked, but this is quite a common paradigm, where one has to be responsible for coding against the lower level API.

Copy link
Member

@gkalpak gkalpak Jan 29, 2018

Choose a reason for hiding this comment

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

Yeah, I meant "more" (everybody knows "less" is "more" 😛)
The thing is that some of the built-in directives (e.g. ngHref/ngSrc) use $set under the hood.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am pretty certain that ngHref and ngSrc are not affected. They only call $set with a value that is passed from $observe, which has already interpolated the value, and so has passed the value through SCE.

passing a "trusted" value provided by calls to `$sce.trustAsMediaUrl(value)`.

If you want to programmatically set explicitly trusted unsafe URLs, you should use `$sce.trustAsHtml`
on the whole `img` tag and inject it into the DOM using the `ng-bind-html` directive.
Copy link
Member

Choose a reason for hiding this comment

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

😱

@petebacondarwin petebacondarwin force-pushed the sceUrl branch 2 times, most recently from 04b6bed to 7202231 Compare January 29, 2018 11:09
@gkalpak
Copy link
Member

gkalpak commented Jan 29, 2018

I am a little concerned about #16378 (comment).
Other than that LGTM (as long as Travis is happy 😉)!

expressionPositions = [],
singleExpression,
contextAllowsConcatenation = trustedContext === $sce.URL || trustedContext === $sce.MEDIA_URL;
endIndex,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, yes, indentation 😱

@@ -118,221 +118,3 @@ describe('boolean attr directives', function() {
}));
});
});


describe('ngSrc', function() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All these tests here and below were in the wrong file. I have moved them to their appropriate places.

@@ -0,0 +1,104 @@
'use strict';

fdescribe('ngHref', function() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agh! fdescribe!!

element = $compile('<a ng-href="javascript:alert(1);">')($rootScope);
$rootScope.$digest();
expect(element.attr('href')).toBe('unsafe:javascript:alert(1);');
}));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the key new test for this commit

element = $compile('<img ng-src="javascript:alert(1);">')($rootScope);
$rootScope.$digest();
expect(element.attr('src')).toBe('unsafe:javascript:alert(1);');
}));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And this test too.

@petebacondarwin petebacondarwin force-pushed the sceUrl branch 2 times, most recently from 044a801 to 2848899 Compare January 30, 2018 14:40
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

LGTM (as soon as Travis is green)

Thanks to @rjamet for the original work on this feature.

This is a large patch to handle URLs with the $sce service, similarly to HTML context.

Where we previously sanitized URL attributes when setting attribute value inside the
`$compile` service, we now only apply an `$sce` context requirement and leave the
`$interpolate` service to deal with sanitization.

This commit introduces a new `$sce` context called `MEDIA_URL`, which represents
a URL used as a source for a media element that is not expected to execute code, such as
image, video, audio, etc.
The context hierarchy is setup so that a value trusted as `URL` is also trusted in the
`MEDIA_URL` context, in the same way that the a value trusted as `RESOURCE_URL` is also
trusted in the `URL` context (and transitively also the `MEDIA_URL` context).

The `$sce` service will now automatically attempt to sanitize non-trusted values that
require the `URL` or `MEDIA_URL` context:

* When calling `getTrustedMediaUrl()` a value that is not already a trusted `MEDIA_URL`
will be sanitized using the `imgSrcSanitizationWhitelist`.
* When calling `getTrustedUrl()` a value that is not already a trusted `URL` will be
sanitized using the `aHrefSanitizationWhitelist`.

This results in behaviour that closely matches the previous sanitization behaviour.

To keep rough compatibility with existing apps, we need to allow concatenation of values
that may contain trusted contexts. The following approach is taken for situations that
require a `URL` or `MEDIA_URL` secure context:

* A single trusted value is trusted, e.g. `"{{trustedUrl}}"` and will not be sanitized.
* A single non-trusted value, e.g. `"{{ 'javascript:foo' }}"`, will be handled by
  `getTrustedMediaUrl` or `getTrustedUrl)` and sanitized.
* Any concatenation of values (which may or may not be trusted) results in a
  non-trusted type that will be handled by `getTrustedMediaUrl` or `getTrustedUrl` once the
  concatenation is complete.
  E.g. `"javascript:{{safeType}}"` is a concatenation of a non-trusted and a trusted value,
  which will be sanitized as a whole after unwrapping the `safeType` value.
* An interpolation containing no expressions will still be handled by `getTrustedMediaUrl` or
  `getTrustedUrl`, whereas before this would have been short-circuited in the `$interpolate`
  service. E.g. `"some/hard/coded/url"`. This ensures that `ngHref` and similar directives
  still securely, even if the URL is hard-coded into a template or index.html (perhaps by
  server-side rendering).

BREAKING CHANGES:

If you use `attrs.$set` for URL attributes (a[href] and img[src]) there will no
longer be any automated sanitization of the value. This is in line with other
programmatic operations, such as writing to the innerHTML of an element.

If you are programmatically writing URL values to attributes from untrusted
input then you must sanitize it yourself. You could write your own sanitizer or copy
the private `$$sanitizeUri` service.

Note that values that have been passed through the `$interpolate` service within the
`URL` or `MEDIA_URL` will have already been sanitized, so you would not need to sanitize
these values again.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants