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

Commit 1e9eadc

Browse files
feat($sce): handle URL sanitization through the $sce service
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.
1 parent a8830d2 commit 1e9eadc

File tree

15 files changed

+824
-470
lines changed

15 files changed

+824
-470
lines changed
+12
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
@ngdoc error
2+
@name $compile:srcset
3+
@fullName Invalid value passed to `attr.$set('srcset', value)`
4+
@description
5+
6+
This error occurs if you try to programmatically set the `srcset` attribute with a non-string value.
7+
8+
This can be the case if you tried to avoid the automatic sanitization of the `srcset` value by
9+
passing a "trusted" value provided by calls to `$sce.trustAsMediaUrl(value)`.
10+
11+
If you want to programmatically set explicitly trusted unsafe URLs, you should use `$sce.trustAsHtml`
12+
on the whole `img` tag and inject it into the DOM using the `ng-bind-html` directive.

src/ng/compile.js

+31-17
Original file line numberDiff line numberDiff line change
@@ -1528,9 +1528,9 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
15281528

15291529
this.$get = [
15301530
'$injector', '$interpolate', '$exceptionHandler', '$templateRequest', '$parse',
1531-
'$controller', '$rootScope', '$sce', '$animate', '$$sanitizeUri',
1531+
'$controller', '$rootScope', '$sce', '$animate',
15321532
function($injector, $interpolate, $exceptionHandler, $templateRequest, $parse,
1533-
$controller, $rootScope, $sce, $animate, $$sanitizeUri) {
1533+
$controller, $rootScope, $sce, $animate) {
15341534

15351535
var SIMPLE_ATTR_NAME = /^\w/;
15361536
var specialAttrHolder = window.document.createElement('div');
@@ -1679,8 +1679,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
16791679
*/
16801680
$set: function(key, value, writeAttr, attrName) {
16811681
// TODO: decide whether or not to throw an error if "class"
1682-
//is set through this function since it may cause $updateClass to
1683-
//become unstable.
1682+
// is set through this function since it may cause $updateClass to
1683+
// become unstable.
16841684

16851685
var node = this.$$element[0],
16861686
booleanKey = getBooleanAttrName(node, key),
@@ -1710,13 +1710,20 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
17101710

17111711
nodeName = nodeName_(this.$$element);
17121712

1713-
if ((nodeName === 'a' && (key === 'href' || key === 'xlinkHref')) ||
1714-
(nodeName === 'img' && key === 'src') ||
1715-
(nodeName === 'image' && key === 'xlinkHref')) {
1716-
// sanitize a[href] and img[src] values
1717-
this[key] = value = $$sanitizeUri(value, nodeName === 'img' || nodeName === 'image');
1718-
} else if (nodeName === 'img' && key === 'srcset' && isDefined(value)) {
1719-
// sanitize img[srcset] values
1713+
// Sanitize img[srcset] values.
1714+
if (nodeName === 'img' && key === 'srcset' && value) {
1715+
if (!isString(value)) {
1716+
throw $compileMinErr('srcset', 'Can\'t pass trusted values to `$set(\'srcset\', value)`: "{0}"', value.toString());
1717+
}
1718+
1719+
// Such values are a bit too complex to handle automatically inside $sce.
1720+
// Instead, we sanitize each of the URIs individually, which works, even dynamically.
1721+
1722+
// It's not possible to work around this using `$sce.trustAsMediaUrl`.
1723+
// If you want to programmatically set explicitly trusted unsafe URLs, you should use
1724+
// `$sce.trustAsHtml` on the whole `img` tag and inject it into the DOM using the
1725+
// `ng-bind-html` directive.
1726+
17201727
var result = '';
17211728

17221729
// first check if there are spaces because it's not the same pattern
@@ -1733,16 +1740,16 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
17331740
for (var i = 0; i < nbrUrisWith2parts; i++) {
17341741
var innerIdx = i * 2;
17351742
// sanitize the uri
1736-
result += $$sanitizeUri(trim(rawUris[innerIdx]), true);
1743+
result += $sce.getTrustedMediaUrl(trim(rawUris[innerIdx]));
17371744
// add the descriptor
1738-
result += (' ' + trim(rawUris[innerIdx + 1]));
1745+
result += ' ' + trim(rawUris[innerIdx + 1]);
17391746
}
17401747

17411748
// split the last item into uri and descriptor
17421749
var lastTuple = trim(rawUris[i * 2]).split(/\s/);
17431750

17441751
// sanitize the last uri
1745-
result += $$sanitizeUri(trim(lastTuple[0]), true);
1752+
result += $sce.getTrustedMediaUrl(trim(lastTuple[0]));
17461753

17471754
// and add the last descriptor if any
17481755
if (lastTuple.length === 2) {
@@ -3268,14 +3275,18 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
32683275
}
32693276
var tag = nodeName_(node);
32703277
// All tags with src attributes require a RESOURCE_URL value, except for
3271-
// img and various html5 media tags.
3278+
// img and various html5 media tags, which require the MEDIA_URL context.
32723279
if (attrNormalizedName === 'src' || attrNormalizedName === 'ngSrc') {
32733280
if (['img', 'video', 'audio', 'source', 'track'].indexOf(tag) === -1) {
32743281
return $sce.RESOURCE_URL;
32753282
}
3283+
return $sce.MEDIA_URL;
3284+
} else if (attrNormalizedName === 'xlinkHref') {
3285+
// Some xlink:href are okay, most aren't
3286+
if (tag === 'image') return $sce.MEDIA_URL;
3287+
if (tag === 'a') return $sce.URL;
3288+
return $sce.RESOURCE_URL;
32763289
} else if (
3277-
// Some xlink:href are okay, most aren't
3278-
(attrNormalizedName === 'xlinkHref' && (tag !== 'image' && tag !== 'a')) ||
32793290
// Formaction
32803291
(tag === 'form' && attrNormalizedName === 'action') ||
32813292
// If relative URLs can go where they are not expected to, then
@@ -3285,6 +3296,9 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
32853296
(tag === 'link' && attrNormalizedName === 'href')
32863297
) {
32873298
return $sce.RESOURCE_URL;
3299+
} else if (tag === 'a' && (attrNormalizedName === 'href' ||
3300+
attrNormalizedName === 'ngHref')) {
3301+
return $sce.URL;
32883302
}
32893303
}
32903304

src/ng/directive/attrs.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -436,7 +436,7 @@ forEach(['src', 'srcset', 'href'], function(attrName) {
436436
// On IE, if "ng:src" directive declaration is used and "src" attribute doesn't exist
437437
// then calling element.setAttribute('src', 'foo') doesn't do anything, so we need
438438
// to set the property as well to achieve the desired effect.
439-
// We use attr[attrName] value since $set can sanitize the url.
439+
// We use attr[attrName] value since $set might have sanitized the url.
440440
if (msie && propName) element.prop(propName, attr[name]);
441441
});
442442
}

src/ng/interpolate.js

+50-25
Original file line numberDiff line numberDiff line change
@@ -238,16 +238,21 @@ function $InterpolateProvider() {
238238
* - `context`: evaluation context for all expressions embedded in the interpolated text
239239
*/
240240
function $interpolate(text, mustHaveExpression, trustedContext, allOrNothing) {
241+
var contextAllowsConcatenation = trustedContext === $sce.URL || trustedContext === $sce.MEDIA_URL;
242+
241243
// Provide a quick exit and simplified result function for text with no interpolation
242244
if (!text.length || text.indexOf(startSymbol) === -1) {
243-
var constantInterp;
244-
if (!mustHaveExpression) {
245-
var unescapedText = unescapeText(text);
246-
constantInterp = valueFn(unescapedText);
247-
constantInterp.exp = text;
248-
constantInterp.expressions = [];
249-
constantInterp.$$watchDelegate = constantWatchDelegate;
245+
if (mustHaveExpression && !contextAllowsConcatenation) return;
246+
247+
var unescapedText = unescapeText(text);
248+
if (contextAllowsConcatenation) {
249+
unescapedText = $sce.getTrusted(trustedContext, unescapedText);
250250
}
251+
var constantInterp = valueFn(unescapedText);
252+
constantInterp.exp = text;
253+
constantInterp.expressions = [];
254+
constantInterp.$$watchDelegate = constantWatchDelegate;
255+
251256
return constantInterp;
252257
}
253258

@@ -256,11 +261,13 @@ function $InterpolateProvider() {
256261
endIndex,
257262
index = 0,
258263
expressions = [],
259-
parseFns = [],
264+
parseFns,
260265
textLength = text.length,
261266
exp,
262267
concat = [],
263-
expressionPositions = [];
268+
expressionPositions = [],
269+
singleExpression;
270+
264271

265272
while (index < textLength) {
266273
if (((startIndex = text.indexOf(startSymbol, index)) !== -1) &&
@@ -270,10 +277,9 @@ function $InterpolateProvider() {
270277
}
271278
exp = text.substring(startIndex + startSymbolLength, endIndex);
272279
expressions.push(exp);
273-
parseFns.push($parse(exp, parseStringifyInterceptor));
274280
index = endIndex + endSymbolLength;
275281
expressionPositions.push(concat.length);
276-
concat.push('');
282+
concat.push(''); // Placeholder that will get replaced with the evaluated expression.
277283
} else {
278284
// we did not find an interpolation, so we have to add the remainder to the separators array
279285
if (index !== textLength) {
@@ -283,29 +289,42 @@ function $InterpolateProvider() {
283289
}
284290
}
285291

292+
singleExpression = concat.length === 1 && expressionPositions.length === 1;
293+
// Intercept expression if we need to stringify concatenated inputs, which may be SCE trusted
294+
// objects rather than simple strings
295+
// (we don't modify the expression if the input consists of only a single trusted input)
296+
var interceptor = contextAllowsConcatenation && singleExpression ? undefined : parseStringifyInterceptor;
297+
parseFns = expressions.map(function(exp) { return $parse(exp, interceptor); });
298+
286299
// Concatenating expressions makes it hard to reason about whether some combination of
287300
// concatenated values are unsafe to use and could easily lead to XSS. By requiring that a
288-
// single expression be used for iframe[src], object[src], etc., we ensure that the value
289-
// that's used is assigned or constructed by some JS code somewhere that is more testable or
290-
// make it obvious that you bound the value to some user controlled value. This helps reduce
291-
// the load when auditing for XSS issues.
292-
if (trustedContext && concat.length > 1) {
293-
$interpolateMinErr.throwNoconcat(text);
294-
}
301+
// single expression be used for some $sce-managed secure contexts (RESOURCE_URLs mostly),
302+
// we ensure that the value that's used is assigned or constructed by some JS code somewhere
303+
// that is more testable or make it obvious that you bound the value to some user controlled
304+
// value. This helps reduce the load when auditing for XSS issues.
305+
306+
// Note that URL and MEDIA_URL $sce contexts do not need this, since `$sce` can sanitize the values
307+
// passed to it. In that case, `$sce.getTrusted` will be called on either the single expression
308+
// or on the overall concatenated string (losing trusted types used in the mix, by design).
309+
// Both these methods will sanitize plain strings. Also, HTML could be included, but since it's
310+
// only used in srcdoc attributes, this would not be very useful.
295311

296312
if (!mustHaveExpression || expressions.length) {
297313
var compute = function(values) {
298314
for (var i = 0, ii = expressions.length; i < ii; i++) {
299315
if (allOrNothing && isUndefined(values[i])) return;
300316
concat[expressionPositions[i]] = values[i];
301317
}
302-
return concat.join('');
303-
};
304318

305-
var getValue = function(value) {
306-
return trustedContext ?
307-
$sce.getTrusted(trustedContext, value) :
308-
$sce.valueOf(value);
319+
if (contextAllowsConcatenation) {
320+
// If `singleExpression` then `concat[0]` might be a "trusted" value or `null`, rather than a string
321+
return $sce.getTrusted(trustedContext, singleExpression ? concat[0] : concat.join(''));
322+
} else if (trustedContext && concat.length > 1) {
323+
// This context does not allow more than one part, e.g. expr + string or exp + exp.
324+
$interpolateMinErr.throwNoconcat(text);
325+
}
326+
// In an unprivileged context or only one part: just concatenate and return.
327+
return concat.join('');
309328
};
310329

311330
return extend(function interpolationFn(context) {
@@ -340,7 +359,13 @@ function $InterpolateProvider() {
340359

341360
function parseStringifyInterceptor(value) {
342361
try {
343-
value = getValue(value);
362+
// In concatenable contexts, getTrusted comes at the end, to avoid sanitizing individual
363+
// parts of a full URL. We don't care about losing the trustedness here.
364+
// In non-concatenable contexts, where there is only one expression, this interceptor is
365+
// not applied to the expression.
366+
value = (trustedContext && !contextAllowsConcatenation) ?
367+
$sce.getTrusted(trustedContext, value) :
368+
$sce.valueOf(value);
344369
return allOrNothing && !isDefined(value) ? value : stringify(value);
345370
} catch (err) {
346371
$exceptionHandler($interpolateMinErr.interr(text, err));

src/ng/sanitizeUri.js

+24-14
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
* Private service to sanitize uris for links and images. Used by $compile and $sanitize.
77
*/
88
function $$SanitizeUriProvider() {
9+
910
var aHrefSanitizationWhitelist = /^\s*(https?|s?ftp|mailto|tel|file):/,
1011
imgSrcSanitizationWhitelist = /^\s*((https?|ftp|file|blob):|data:image\/)/;
1112

@@ -14,12 +15,16 @@ function $$SanitizeUriProvider() {
1415
* Retrieves or overrides the default regular expression that is used for whitelisting of safe
1516
* urls during a[href] sanitization.
1617
*
17-
* The sanitization is a security measure aimed at prevent XSS attacks via html links.
18+
* The sanitization is a security measure aimed at prevent XSS attacks via HTML anchor links.
19+
*
20+
* Any url due to be assigned to an `a[href]` attribute via interpolation is marked as requiring
21+
* the $sce.URL security context. When interpolation occurs a call is made to `$sce.trustAsUrl(url)`
22+
* which in turn may call `$$sanitizeUri(url, isMedia)` to sanitize the potentially malicious URL.
23+
*
24+
* If the URL matches the `aHrefSanitizationWhitelist` regular expression, it is returned unchanged.
1825
*
19-
* Any url about to be assigned to a[href] via data-binding is first normalized and turned into
20-
* an absolute url. Afterwards, the url is matched against the `aHrefSanitizationWhitelist`
21-
* regular expression. If a match is found, the original url is written into the dom. Otherwise,
22-
* the absolute url is prefixed with `'unsafe:'` string and only then is it written into the DOM.
26+
* If there is no match the URL is returned prefixed with `'unsafe:'` to ensure that when it is written
27+
* to the DOM it is inactive and potentially malicious code will not be executed.
2328
*
2429
* @param {RegExp=} regexp New regexp to whitelist urls with.
2530
* @returns {RegExp|ng.$compileProvider} Current RegExp if called without value or self for
@@ -39,12 +44,17 @@ function $$SanitizeUriProvider() {
3944
* Retrieves or overrides the default regular expression that is used for whitelisting of safe
4045
* urls during img[src] sanitization.
4146
*
42-
* The sanitization is a security measure aimed at prevent XSS attacks via html links.
47+
* The sanitization is a security measure aimed at prevent XSS attacks via HTML image src links.
48+
*
49+
* Any URL due to be assigned to an `img[src]` attribute via interpolation is marked as requiring
50+
* the $sce.MEDIA_URL security context. When interpolation occurs a call is made to
51+
* `$sce.trustAsMediaUrl(url)` which in turn may call `$$sanitizeUri(url, isMedia)` to sanitize
52+
* the potentially malicious URL.
53+
*
54+
* If the URL matches the `aImgSanitizationWhitelist` regular expression, it is returned unchanged.
4355
*
44-
* Any url about to be assigned to img[src] via data-binding is first normalized and turned into
45-
* an absolute url. Afterwards, the url is matched against the `imgSrcSanitizationWhitelist`
46-
* regular expression. If a match is found, the original url is written into the dom. Otherwise,
47-
* the absolute url is prefixed with `'unsafe:'` string and only then is it written into the DOM.
56+
* If there is no match the URL is returned prefixed with `'unsafe:'` to ensure that when it is written
57+
* to the DOM it is inactive and potentially malicious code will not be executed.
4858
*
4959
* @param {RegExp=} regexp New regexp to whitelist urls with.
5060
* @returns {RegExp|ng.$compileProvider} Current RegExp if called without value or self for
@@ -59,10 +69,10 @@ function $$SanitizeUriProvider() {
5969
};
6070

6171
this.$get = function() {
62-
return function sanitizeUri(uri, isImage) {
63-
var regex = isImage ? imgSrcSanitizationWhitelist : aHrefSanitizationWhitelist;
64-
var normalizedVal;
65-
normalizedVal = urlResolve(uri && uri.trim()).href;
72+
return function sanitizeUri(uri, isMediaUrl) {
73+
// if (!uri) return uri;
74+
var regex = isMediaUrl ? imgSrcSanitizationWhitelist : aHrefSanitizationWhitelist;
75+
var normalizedVal = urlResolve(uri && uri.trim()).href;
6676
if (normalizedVal !== '' && !normalizedVal.match(regex)) {
6777
return 'unsafe:' + normalizedVal;
6878
}

0 commit comments

Comments
 (0)