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

Commit

Permalink
feat($sce): handle URLs through the $sce service
Browse files Browse the repository at this point in the history
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.

BREAKING CHANGES:

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).
  • Loading branch information
petebacondarwin committed Dec 19, 2017
1 parent 7df2952 commit 36c4de9
Show file tree
Hide file tree
Showing 11 changed files with 542 additions and 248 deletions.
36 changes: 22 additions & 14 deletions src/ng/compile.js
Original file line number Diff line number Diff line change
Expand Up @@ -1528,9 +1528,9 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {

this.$get = [
'$injector', '$interpolate', '$exceptionHandler', '$templateRequest', '$parse',
'$controller', '$rootScope', '$sce', '$animate', '$$sanitizeUri',
'$controller', '$rootScope', '$sce', '$animate',
function($injector, $interpolate, $exceptionHandler, $templateRequest, $parse,
$controller, $rootScope, $sce, $animate, $$sanitizeUri) {
$controller, $rootScope, $sce, $animate) {

var SIMPLE_ATTR_NAME = /^\w/;
var specialAttrHolder = window.document.createElement('div');
Expand Down Expand Up @@ -1710,12 +1710,13 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {

nodeName = nodeName_(this.$$element);

if ((nodeName === 'a' && (key === 'href' || key === 'xlinkHref')) ||
(nodeName === 'img' && key === 'src') ||
(nodeName === 'image' && key === 'xlinkHref')) {
// sanitize a[href] and img[src] values
this[key] = value = $$sanitizeUri(value, nodeName === 'img' || nodeName === 'image');
} else if (nodeName === 'img' && key === 'srcset' && isDefined(value)) {
// img[srcset] is a bit too weird of a beast to handle through $sce, for now at least.
// Instead, we sanitize each of the URIs individually, which works, even dynamically.
// It's not possible to work around this using `$sce.trustAsUrl`.
// Instead, if you want several unsafe URLs as-is, you should probably
// use `$sce.trustAsHtml` on the whole `img` tag.
if (nodeName === 'img' && key === 'srcset' && value) {

// sanitize img[srcset] values
var result = '';

Expand All @@ -1733,16 +1734,16 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
for (var i = 0; i < nbrUrisWith2parts; i++) {
var innerIdx = i * 2;
// sanitize the uri
result += $$sanitizeUri(trim(rawUris[innerIdx]), true);
result += $sce.getTrustedUrl(trim(rawUris[innerIdx]));
// add the descriptor
result += (' ' + trim(rawUris[innerIdx + 1]));
result += ' ' + trim(rawUris[innerIdx + 1]);
}

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

// sanitize the last uri
result += $$sanitizeUri(trim(lastTuple[0]), true);
result += $sce.getTrustedUrl(trim(lastTuple[0]));

// and add the last descriptor if any
if (lastTuple.length === 2) {
Expand Down Expand Up @@ -3268,14 +3269,18 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
}
var tag = nodeName_(node);
// All tags with src attributes require a RESOURCE_URL value, except for
// img and various html5 media tags.
// img and various html5 media tags, which require the SRC context.
if (attrNormalizedName === 'src' || attrNormalizedName === 'ngSrc') {
if (['img', 'video', 'audio', 'source', 'track'].indexOf(tag) === -1) {
return $sce.RESOURCE_URL;
}
return $sce.SRC;
} else if (attrNormalizedName === 'xlinkHref') {
// Some xlink:href are okay, most aren't
if (tag === 'image') return $sce.SRC;
if (tag === 'a') return $sce.URL;
return $sce.RESOURCE_URL;
} else if (
// Some xlink:href are okay, most aren't
(attrNormalizedName === 'xlinkHref' && (tag !== 'image' && tag !== 'a')) ||
// Formaction
(tag === 'form' && attrNormalizedName === 'action') ||
// If relative URLs can go where they are not expected to, then
Expand All @@ -3285,6 +3290,9 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
(tag === 'link' && attrNormalizedName === 'href')
) {
return $sce.RESOURCE_URL;
} else if (tag === 'a' && (attrNormalizedName === 'href' ||
attrNormalizedName === 'ngHref')) {
return $sce.URL;
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/ng/directive/attrs.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
if (msie && propName) element.prop(propName, attr[name]);
});
}
Expand Down
65 changes: 52 additions & 13 deletions src/ng/interpolate.js
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,10 @@ function $InterpolateProvider() {
return unwatch;
}

function isConcatenationAllowed(context) {
return context === $sce.URL || context === $sce.SRC;
}

/**
* @ngdoc service
* @name $interpolate
Expand Down Expand Up @@ -260,7 +264,9 @@ function $InterpolateProvider() {
textLength = text.length,
exp,
concat = [],
expressionPositions = [];
expressionPositions = [],
singleExpression = false,
contextAllowsConcatenation = isConcatenationAllowed(trustedContext);

while (index < textLength) {
if (((startIndex = text.indexOf(startSymbol, index)) !== -1) &&
Expand All @@ -273,7 +279,7 @@ function $InterpolateProvider() {
parseFns.push($parse(exp, parseStringifyInterceptor));
index = endIndex + endSymbolLength;
expressionPositions.push(concat.length);
concat.push('');
concat.push(''); // Placeholder that will get replaced with the evaluated expression.
} else {
// we did not find an interpolation, so we have to add the remainder to the separators array
if (index !== textLength) {
Expand All @@ -283,27 +289,55 @@ function $InterpolateProvider() {
}
}

if (concat.length === 1 && expressionPositions.length === 1) {
singleExpression = true;
}

// Concatenating expressions makes it hard to reason about whether some combination of
// concatenated values are unsafe to use and could easily lead to XSS. By requiring that a
// single expression be used for iframe[src], object[src], etc., we ensure that the value
// that's used is assigned or constructed by some JS code somewhere that is more testable or
// make it obvious that you bound the value to some user controlled value. This helps reduce
// the load when auditing for XSS issues.
if (trustedContext && concat.length > 1) {
$interpolateMinErr.throwNoconcat(text);
}
// single expression be used for some $sce-managed secure contexts (RESOURCE_URLs mostly),
// we ensure that the value that's used is assigned or constructed by some JS code somewhere
// that is more testable or make it obvious that you bound the value to some user controlled
// value. This helps reduce the load when auditing for XSS issues.

// Note that URL and SRC $sce contexts do not need this, since `$sce` can sanitize the values
// passed to it. In that case, `$sce.getTrusted` will be called on either the single expression
// or on the overall concatenated string (losing trusted types used in the mix, by design).
// Both these methods will sanitize plain strings. Also, HTML could be included, but since it's
// only used in srcdoc attributes, this would not be very useful.

if (!mustHaveExpression || expressions.length) {
var compute = function(values) {
for (var i = 0, ii = expressions.length; i < ii; i++) {
if (allOrNothing && isUndefined(values[i])) return;
concat[expressionPositions[i]] = values[i];
}
return concat.join('');

if (contextAllowsConcatenation) {
if (singleExpression) {
// The raw value was left as-is by parseStringifyInterceptor
return $sce.getTrusted(trustedContext, concat[0]);
} else {
return $sce.getTrusted(trustedContext, concat.join(''));
}
} else if (trustedContext) {
if (concat.length > 1) {
// there's at least two parts, so expr + string or exp + exp, and this context
// doesn't allow that.
$interpolateMinErr.throwNoconcat(text);
} else {
return concat.join('');
}
} else { // In an unprivileged context, just concatenate and return.
return concat.join('');
}
};

var getValue = function(value) {
return trustedContext ?
// 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, that's handled in
// parseStringifyInterceptor below.
return (trustedContext && !contextAllowsConcatenation) ?
$sce.getTrusted(trustedContext, value) :
$sce.valueOf(value);
};
Expand Down Expand Up @@ -340,8 +374,13 @@ function $InterpolateProvider() {

function parseStringifyInterceptor(value) {
try {
value = getValue(value);
return allOrNothing && !isDefined(value) ? value : stringify(value);
if (contextAllowsConcatenation && singleExpression) {
// No stringification in this case, to keep the trusted value until unwrapping.
return value;
} else {
value = getValue(value);
return allOrNothing && !isDefined(value) ? value : stringify(value);
}
} catch (err) {
$exceptionHandler($interpolateMinErr.interr(text, err));
}
Expand Down
7 changes: 5 additions & 2 deletions src/ng/sanitizeUri.js
Original file line number Diff line number Diff line change
Expand Up @@ -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):/,
imgSrcSanitizationWhitelist = /^\s*((https?|ftp|file|blob):|data:image\/)/;

Expand All @@ -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.
*
* @param {RegExp=} regexp New regexp to whitelist urls with.
* @returns {RegExp|ng.$compileProvider} Current RegExp if called without value or self for
Expand All @@ -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.
*
* @param {RegExp=} regexp New regexp to whitelist urls with.
* @returns {RegExp|ng.$compileProvider} Current RegExp if called without value or self for
Expand Down
Loading

0 comments on commit 36c4de9

Please sign in to comment.