Add a lint check for innerHTML

oppia · kevinlee12 · Mar 28, 2020 · Mar 18, 2020 · Mar 18, 2020 · Mar 18, 2020
commit bcf1c39d072580b2e758e199ab337224184d2855
@@ -433,6 +433,7 @@
 /core/domain/change_domain.py @ankita240796
 /core/templates/tests/ @kevinlee12 @nithusha21
 /core/templates/domain/utilities/ @bansalnitish
+/core/templates/Polyfills.ts @ankita240796
 /schema_utils*.py @seanlip
 /utils*.py @aks681
 /python_utils*.py @kevinlee12

@@ -71,6 +71,8 @@ require('google-analytics.initializer.ts');
 // loaded after app.constants.ts
 require('I18nFooter.ts');
 
+require('Polyfills.ts');
+
 const sourceMappedStackTrace = require('sourcemapped-stacktrace');
 
 angular.module('oppia').config([
@@ -299,135 +301,3 @@ angular.module('oppia').factory('$exceptionHandler', [
     };
   }
 ]);
-
-// Add a String.prototype.trim() polyfill for IE8.
-if (typeof String.prototype.trim !== 'function') {
-  String.prototype.trim = function() {
-    return this.replace(/^\s+|\s+$/g, '');
-  };
-}
-
-// Add an Object.create() polyfill for IE8.
-if (typeof Object.create !== 'function') {
-  (function() {
-    var F = function() {};
-    Object.create = function(o) {
-      if (arguments.length > 1) {
-        throw Error('Second argument for Object.create() is not supported');
-      }
-      if (o === null) {
-        throw Error('Cannot set a null [[Prototype]]');
-      }
-      if (typeof o !== 'object') {
-        throw TypeError('Argument must be an object');
-      }
-      F.prototype = o;
-      return new F();
-    };
-  })();
-}
-
-// Add a Number.isInteger() polyfill for IE.
-Number.isInteger = Number.isInteger || function(value) {
-  return (
-    typeof value === 'number' && isFinite(value) &&
-    Math.floor(value) === value);
-};
-
-
-// Add Array.fill() polyfill for IE.
-if (!Array.prototype.fill) {
-  Object.defineProperty(Array.prototype, 'fill', {
-    value: function(value) {
-      // Steps 1-2.
-      if (this === null) {
-        throw new TypeError('this is null or not defined');
-      }
-
-      var O = Object(this);
-
-      // Steps 3-5.
-      var len = O.length >>> 0;
-
-      // Steps 6-7.
-      var start = arguments[1];
-      var relativeStart = start >> 0;
-
-      // Step 8.
-      var k = relativeStart < 0 ?
-        Math.max(len + relativeStart, 0) :
-        Math.min(relativeStart, len);
-
-      // Steps 9-10.
-      var end = arguments[2];
-      var relativeEnd = end === undefined ?
-        len : end >> 0;
-
-      // Step 11.
-      var final = relativeEnd < 0 ?
-        Math.max(len + relativeEnd, 0) :
-        Math.min(relativeEnd, len);
-
-      // Step 12.
-      while (k < final) {
-        O[k] = value;
-        k++;
-      }
-
-      // Step 13.
-      return O;
-    }
-  });
-}
-
-
-// Add SVGElement.prototype.outerHTML polyfill for IE
-if (!('outerHTML' in SVGElement.prototype)) {
-  Object.defineProperty(SVGElement.prototype, 'outerHTML', {
-    get: function() {
-      var $node, $temp;
-      $temp = document.createElement('div');
-      $node = this.cloneNode(true);
-      $temp.appendChild($node);
-      return $temp.innerHTML;
-    },
-    enumerable: false,
-    configurable: true
-  });
-}
-
-
-// Older browsers might not implement mediaDevices at all,
-// so we set an empty object first.
-if (navigator.mediaDevices === undefined) {
-  // @ts-ignore: mediaDevices is read-only error.
-  navigator.mediaDevices = {};
-}
-
-// Some browsers partially implement mediaDevices.
-// We can't just assign an object with getUserMedia
-// as it would overwrite existing properties.
-// Here, we will just add the getUserMedia property
-// if it's missing.
-if (navigator.mediaDevices.getUserMedia === undefined) {
-  navigator.mediaDevices.getUserMedia = function(constraints) {
-    // First get ahold of the legacy getUserMedia, if present.
-    var getUserMedia = (
-      // @ts-ignore: 'webkitGetUserMedia' and 'mozGetUserMedia'
-      // property does not exist error.
-      navigator.webkitGetUserMedia || navigator.mozGetUserMedia);
-
-    // If getUserMedia is not implemented, return a rejected promise
-    // with an error to keep a consistent interface.
-    if (!getUserMedia) {
-      return Promise.reject(
-        new Error('getUserMedia is not implemented in this browser'));
-    }
-
-    // Otherwise, wrap the call to the old navigator.getUserMedia
-    // with a Promise.
-    return new Promise(function(resolve, reject) {
-      getUserMedia.call(navigator, constraints, resolve, reject);
-    });
-  };
-}
@@ -0,0 +1,149 @@
+// Copyright 2020 The Oppia Authors. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS-IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+/**
+ * @fileoverview Polyfills for Oppia.
+ */
+
+// Add a String.prototype.trim() polyfill for IE8.
+if (typeof String.prototype.trim !== 'function') {
+  String.prototype.trim = function() {
+    return this.replace(/^\s+|\s+$/g, '');
+  };
+}
+
+// Add an Object.create() polyfill for IE8.
+if (typeof Object.create !== 'function') {
+  (function() {
+    var F = function() {};
+    Object.create = function(o) {
+      if (arguments.length > 1) {
+        throw Error('Second argument for Object.create() is not supported');
+      }
+      if (o === null) {
+        throw Error('Cannot set a null [[Prototype]]');
+      }
+      if (typeof o !== 'object') {
+        throw TypeError('Argument must be an object');
+      }
+      F.prototype = o;
+      return new F();
+    };
+  })();
+}
+
+// Add a Number.isInteger() polyfill for IE.
+Number.isInteger = Number.isInteger || function(value) {
+  return (
+    typeof value === 'number' && isFinite(value) &&
+    Math.floor(value) === value);
+};
+
+
+// Add Array.fill() polyfill for IE.
+if (!Array.prototype.fill) {
+  Object.defineProperty(Array.prototype, 'fill', {
+    value: function(value) {
+      // Steps 1-2.
+      if (this === null) {
+        throw new TypeError('this is null or not defined');
+      }
+
+      var O = Object(this);
+
+      // Steps 3-5.
+      var len = O.length >>> 0;
+
+      // Steps 6-7.
+      var start = arguments[1];
+      var relativeStart = start >> 0;
+
+      // Step 8.
+      var k = relativeStart < 0 ?
+        Math.max(len + relativeStart, 0) :
+        Math.min(relativeStart, len);
+
+      // Steps 9-10.
+      var end = arguments[2];
+      var relativeEnd = end === undefined ?
+        len : end >> 0;
+
+      // Step 11.
+      var final = relativeEnd < 0 ?
+        Math.max(len + relativeEnd, 0) :
+        Math.min(relativeEnd, len);
+
+      // Step 12.
+      while (k < final) {
+        O[k] = value;
+        k++;
+      }
+
+      // Step 13.
+      return O;
+    }
+  });
+}
+
+
+// Add SVGElement.prototype.outerHTML polyfill for IE
+if (!('outerHTML' in SVGElement.prototype)) {
+  Object.defineProperty(SVGElement.prototype, 'outerHTML', {
+    get: function() {
+      var $node, $temp;
+      $temp = document.createElement('div');
+      $node = this.cloneNode(true);
+      $temp.appendChild($node);
+      return $temp.innerHTML;
+    },
+    enumerable: false,
+    configurable: true
+  });
+}
+
+
+// Older browsers might not implement mediaDevices at all,
+// so we set an empty object first.
+if (navigator.mediaDevices === undefined) {
+  // @ts-ignore: mediaDevices is read-only error.
+  navigator.mediaDevices = {};
+}
+
+// Some browsers partially implement mediaDevices.
+// We can't just assign an object with getUserMedia
+// as it would overwrite existing properties.
+// Here, we will just add the getUserMedia property
+// if it's missing.
+if (navigator.mediaDevices.getUserMedia === undefined) {
+  navigator.mediaDevices.getUserMedia = function(constraints) {
+    // First get ahold of the legacy getUserMedia, if present.
+    var getUserMedia = (
+      // @ts-ignore: 'webkitGetUserMedia' and 'mozGetUserMedia'
+      // property does not exist error.
+      navigator.webkitGetUserMedia || navigator.mozGetUserMedia);
+
+    // If getUserMedia is not implemented, return a rejected promise
+    // with an error to keep a consistent interface.
+    if (!getUserMedia) {
+      return Promise.reject(
+        new Error('getUserMedia is not implemented in this browser'));
+    }
+
+    // Otherwise, wrap the call to the old navigator.getUserMedia
+    // with a Promise.
+    return new Promise(function(resolve, reject) {
+      getUserMedia.call(navigator, constraints, resolve, reject);
+    });
+  };
+}
@@ -133,11 +133,12 @@ export class ExtractImageFilenamesFromStateService {
     _extractFilepathValueFromOppiaNonInteractiveImageTag(
         strHtml: string): Array<any> {
       let filenames = [];
-      let dummyElement = document.createElement('div');
-      dummyElement.innerHTML = (
+      let unescapedHtmlString = (
         this.htmlEscaperService.escapedStrToUnescapedStr(strHtml));
+      let dummyDocument = (
+        new DOMParser().parseFromString(unescapedHtmlString, 'text/html'));
 
-      let imageTagList = dummyElement.getElementsByTagName(
+      let imageTagList = dummyDocument.getElementsByTagName(
         'oppia-noninteractive-image');
       for (let i = 0; i < imageTagList.length; i++) {
         // We have the attribute of filepath in oppia-noninteractive-image tag.

@@ -144,24 +144,29 @@ angular.module('oppia').directive('oppiaInteractivePencilCodeEditor', [
               if (errorIsHappening || hasSubmittedAnswer) {
                 return;
               }
-
-              pce.eval('document.body.innerHTML', function(pencilCodeHtml) {
-                var normalizedCode = getNormalizedCode();
-
-                // Get all the divs, and extract their textual content.
-                var output = $.map(
-                  $(pencilCodeHtml).filter('div'), function(elem) {
-                    return $(elem).text();
-                  }).join('\n');
-
-                hasSubmittedAnswer = true;
-                CurrentInteractionService.onSubmit({
-                  code: normalizedCode,
-                  output: output || '',
-                  evaluation: '',
-                  error: ''
-                }, PencilCodeEditorRulesService);
-              }, true);
+              // The first argument in the method below gets executed in the
+              // pencilcode output-frame iframe context. The code input by the
+              // user is sanitized by pencilcode so there is no security
+              // issue in this case.
+              pce.eval(
+                'document.body.innerHTML', // disable-bad-pattern-check
+                function(pencilCodeHtml) {
+                  var normalizedCode = getNormalizedCode();
+
+                  // Get all the divs, and extract their textual content.
+                  var output = $.map(
+                    $(pencilCodeHtml).filter('div'), function(elem) {
+                      return $(elem).text();
+                    }).join('\n');
+
+                  hasSubmittedAnswer = true;
+                  CurrentInteractionService.onSubmit({
+                    code: normalizedCode,
+                    output: output || '',
+                    evaluation: '',
+                    error: ''
+                  }, PencilCodeEditorRulesService);
+                }, true);
             });
 
             pce.on('error', function(error) {

diff --git a/scripts/pre_commit_linter.py b/scripts/pre_commit_linter.py
@@ -236,7 +236,13 @@
         'regexp': re.compile(r'require\(.*\.\..*\);'),
         'message': 'Please, don\'t use relative imports in require().',
         'excluded_files': (),
-        'excluded_dirs': ('core/tests/')
+        'excluded_dirs': ('core/tests/',)
+    },
+    {
+        'regexp': re.compile(r'innerHTML'),
+        'message': 'Please do not use innerHTML property.',
+        'excluded_files': ('core/templates/Polyfills.ts',),
+        'excluded_dirs': ('core/tests/',)
     }
 ]
 
@@ -1018,9 +1024,10 @@ def _check_bad_pattern_in_file(filepath, file_content, pattern):
         bool. True if there is bad pattern else false.
     """
     regexp = pattern['regexp']
-    if not (any(filepath.startswith(excluded_dir)
+    if not (any(filepath.startswith(os.path.join(os.getcwd(), excluded_dir))
                 for excluded_dir in pattern['excluded_dirs'])
-            or filepath in pattern['excluded_files']):
+            or filepath in [os.path.join(os.getcwd(), excluded_path)
+                            for excluded_path in pattern['excluded_files']]):
         bad_pattern_count = 0
         for line_num, line in enumerate(file_content.split('\n'), 1):
             if line.endswith('disable-bad-pattern-check'):