From 8879aa1df4c69d795bb58f872390373e7d7cc087 Mon Sep 17 00:00:00 2001
From: Martin Probst <martin@probst.io>
Date: Thu, 16 Jun 2016 16:09:12 -0700
Subject: [PATCH] feat(security): fail more detectably when using a safe value
 in an interpolation.

If a user ends up with a safe value in an interpolation context, that's probably
a bug. Returning `"SafeValue must use [property]= binding"` will make it easier
to detect and correct the situation. Detecting the situation and throwing an
error for it could cause performance issues, so we're not doing this at this
point (but might revisit later).

Part of #8511 and #9253.
---
 .../test/linker/security_integration_spec.ts  | 41 ++++++++++++++-----
 .../src/security/dom_sanitization_service.ts  |  9 +++-
 2 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/modules/@angular/core/test/linker/security_integration_spec.ts b/modules/@angular/core/test/linker/security_integration_spec.ts
index e27c791ca7..eb49785e27 100644
--- a/modules/@angular/core/test/linker/security_integration_spec.ts
+++ b/modules/@angular/core/test/linker/security_integration_spec.ts
@@ -29,12 +29,12 @@ class SecuredComponent {
   constructor() { this.ctxProp = 'some value'; }
 }
 
-function itAsync(msg: string, injections: Function[], f: Function): any; /** TODO #???? */
-function itAsync(msg: string, f: (tcb: TestComponentBuilder, atc: AsyncTestCompleter) => void):
-    any; /** TODO #???? */
+function itAsync(msg: string, injections: Function[], f: Function): void;
+function itAsync(
+    msg: string, f: (tcb: TestComponentBuilder, atc: AsyncTestCompleter) => void): void;
 function itAsync(
     msg: string, f: Function[] | ((tcb: TestComponentBuilder, atc: AsyncTestCompleter) => void),
-    fn?: Function): any /** TODO #???? */ {
+    fn?: Function): void {
   if (f instanceof Function) {
     it(msg, inject([TestComponentBuilder, AsyncTestCompleter], <Function>f));
   } else {
@@ -63,8 +63,7 @@ function declareTests({useJit}: {useJit: boolean}) {
 
 
     itAsync(
-        'should disallow binding on*',
-        (tcb: TestComponentBuilder, async: any /** TODO #???? */) => {
+        'should disallow binding on*', (tcb: TestComponentBuilder, async: AsyncTestCompleter) => {
           let tpl = `<div [attr.onclick]="ctxProp"></div>`;
           tcb = tcb.overrideView(SecuredComponent, new ViewMetadata({template: tpl}));
           PromiseWrapper.catchError(tcb.createAsync(SecuredComponent), (e) => {
@@ -81,7 +80,7 @@ function declareTests({useJit}: {useJit: boolean}) {
       itAsync(
           'should not escape values marked as trusted',
           [TestComponentBuilder, AsyncTestCompleter, DomSanitizationService],
-          (tcb: TestComponentBuilder, async: any /** TODO #???? */,
+          (tcb: TestComponentBuilder, async: AsyncTestCompleter,
            sanitizer: DomSanitizationService) => {
             let tpl = `<a [href]="ctxProp">Link Title</a>`;
             tcb.overrideView(SecuredComponent, new ViewMetadata({template: tpl, directives: []}))
@@ -101,7 +100,7 @@ function declareTests({useJit}: {useJit: boolean}) {
       itAsync(
           'should error when using the wrong trusted value',
           [TestComponentBuilder, AsyncTestCompleter, DomSanitizationService],
-          (tcb: TestComponentBuilder, async: any /** TODO #???? */,
+          (tcb: TestComponentBuilder, async: AsyncTestCompleter,
            sanitizer: DomSanitizationService) => {
             let tpl = `<a [href]="ctxProp">Link Title</a>`;
             tcb.overrideView(SecuredComponent, new ViewMetadata({template: tpl, directives: []}))
@@ -116,12 +115,32 @@ function declareTests({useJit}: {useJit: boolean}) {
                   async.done();
                 });
           });
+
+      itAsync(
+          'should warn when using in string interpolation',
+          [TestComponentBuilder, AsyncTestCompleter, DomSanitizationService],
+          (tcb: TestComponentBuilder, async: AsyncTestCompleter,
+           sanitizer: DomSanitizationService) => {
+            let tpl = `<a href="/foo/{{ctxProp}}">Link Title</a>`;
+            tcb.overrideView(SecuredComponent, new ViewMetadata({template: tpl, directives: []}))
+                .createAsync(SecuredComponent)
+                .then((fixture) => {
+                  let e = fixture.debugElement.children[0].nativeElement;
+                  let trusted = sanitizer.bypassSecurityTrustUrl('bar/baz');
+                  let ci = fixture.debugElement.componentInstance;
+                  ci.ctxProp = trusted;
+                  fixture.detectChanges();
+                  expect(getDOM().getProperty(e, 'href')).toMatch(/SafeValue(%20| )must(%20| )use/);
+
+                  async.done();
+                });
+          });
     });
 
     describe('sanitizing', () => {
       itAsync(
           'should escape unsafe attributes',
-          (tcb: TestComponentBuilder, async: any /** TODO #???? */) => {
+          (tcb: TestComponentBuilder, async: AsyncTestCompleter) => {
             let tpl = `<a [href]="ctxProp">Link Title</a>`;
             tcb.overrideView(SecuredComponent, new ViewMetadata({template: tpl, directives: []}))
                 .createAsync(SecuredComponent)
@@ -144,7 +163,7 @@ function declareTests({useJit}: {useJit: boolean}) {
 
       itAsync(
           'should escape unsafe style values',
-          (tcb: TestComponentBuilder, async: any /** TODO #???? */) => {
+          (tcb: TestComponentBuilder, async: AsyncTestCompleter) => {
             let tpl = `<div [style.background]="ctxProp">Text</div>`;
             tcb.overrideView(SecuredComponent, new ViewMetadata({template: tpl, directives: []}))
                 .createAsync(SecuredComponent)
@@ -169,7 +188,7 @@ function declareTests({useJit}: {useJit: boolean}) {
 
       itAsync(
           'should escape unsafe HTML values',
-          (tcb: TestComponentBuilder, async: any /** TODO #???? */) => {
+          (tcb: TestComponentBuilder, async: AsyncTestCompleter) => {
             let tpl = `<div [innerHTML]="ctxProp">Text</div>`;
             tcb.overrideView(SecuredComponent, new ViewMetadata({template: tpl, directives: []}))
                 .createAsync(SecuredComponent)
diff --git a/modules/@angular/platform-browser/src/security/dom_sanitization_service.ts b/modules/@angular/platform-browser/src/security/dom_sanitization_service.ts
index cf34236797..d5bd275221 100644
--- a/modules/@angular/platform-browser/src/security/dom_sanitization_service.ts
+++ b/modules/@angular/platform-browser/src/security/dom_sanitization_service.ts
@@ -9,7 +9,9 @@ import {sanitizeUrl} from './url_sanitizer';
 export {SecurityContext};
 
 
-/** Marker interface for a value that's safe to use in a particular context. */
+/**
+ * Marker interface for a value that's safe to use in a particular context.
+ */
 export interface SafeValue {}
 /** Marker interface for a value that's safe to use as HTML. */
 export interface SafeHtml extends SafeValue {}
@@ -151,7 +153,12 @@ abstract class SafeValueImpl implements SafeValue {
   constructor(public changingThisBreaksApplicationSecurity: string) {
     // empty
   }
+
   abstract getTypeName(): string;
+
+  toString() {
+    return `SafeValue must use [property]=binding: ${this.changingThisBreaksApplicationSecurity}`;
+  }
 }
 
 class SafeHtmlImpl extends SafeValueImpl implements SafeHtml {