User Manager MD User Pods

ui/login/account_picker/user_pod_row.js

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 /**
  * @fileoverview User pod row implementation.
  */
 
 cr.define('login', function() {
   /**
@@ skipping 10 matching lines @@
    * @const
    */
   var MARGIN_BY_COLUMNS = [undefined, 40, 40, 40, 40, 40, 12];
 
   /**
    * Mapping between number of columns in the desktop pod-row and margin
    * between user pods for such layout.
    * @type {Array<number>}
    * @const
    */
-  var DESKTOP_MARGIN_BY_COLUMNS = [undefined, 15, 15, 15, 15, 15, 15];
+  var DESKTOP_MARGIN_BY_COLUMNS = [undefined, 32, 32, 32, 32, 32, 32];
 
   /**
    * Maximal number of columns currently supported by pod-row.
    * @type {number}
    * @const
    */
   var MAX_NUMBER_OF_COLUMNS = 6;
 
   /**
    * Maximal number of rows if sign-in banner is displayed alonside.
    * @type {number}
    * @const
    */
   var MAX_NUMBER_OF_ROWS_UNDER_SIGNIN_BANNER = 2;
 
   /**
    * Variables used for pod placement processing. Width and height should be
    * synced with computed CSS sizes of pods.
    */
-  var POD_WIDTH = 180;
+  var CROS_POD_WIDTH = 180;
+  var DESKTOP_POD_WIDTH = 160;
   var PUBLIC_EXPANDED_BASIC_WIDTH = 500;
   var PUBLIC_EXPANDED_ADVANCED_WIDTH = 610;
   var CROS_POD_HEIGHT = 213;
-  var DESKTOP_POD_HEIGHT = 226;
+  var DESKTOP_POD_HEIGHT = 200;
   var POD_ROW_PADDING = 10;
-  var DESKTOP_ROW_PADDING = 15;
+  var DESKTOP_ROW_PADDING = 32;
   var CUSTOM_ICON_CONTAINER_SIZE = 40;
 
   /**
    * Minimal padding between user pod and virtual keyboard.
    * @type {number}
    * @const
    */
   var USER_POD_KEYBOARD_MIN_PADDING = 20;
 
   /**
@@ skipping 1474 matching lines @@
      * @param {string} message The message to be written.
      * @param {number|string=} count The number or string to replace $1 in
      * |message|. Can be omitted if $1 is not present in |message|.
      */
     updateRemoveWarningDialogSetMessage_: function(profilePath, message,
                                                    count) {
       if (profilePath !== this.user.profilePath)
         return;
       // Add localized messages where $1 will be replaced with
       // <span class="total-count"></span>.
+      // and $2 will be replaced with <span class="email"></span>.
+      message = message.replace('$1', '<span class="total-count">' + count +
+          '</span>');
+      message = message.replace('$2', '<span class="email">' +
+          this.user.emailAddress + '</span>');
       var element = this.querySelector('.action-box-remove-user-warning-text');
-      element.textContent = '';
+      element.innerHTML = message;

[Dan Beam, 2016/04/07 18:35:00]

why are you moving from textContent to innerHTML?

[Moe, 2016/04/07 22:48:55]

the existing logic was too complex. dzhioev@ suggested I replace it with this
simpler logic.

[Dan Beam, 2016/04/08 00:16:28]

innerHTML = '<script>alert(/xss/)</script>' // actually executes
textContent = '<script>alert(1)</script>' // doesn't

[lwchkg, 2016/04/12 18:28:38]

Dan, WDYT about using innerHTML but escaping |count| and
|this.user.emailAddress|?

[Dan Beam, 2016/04/12 18:33:50]

i think replacing $1 and $2 manually is silly when we've got code that does this
and could be factored out:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...

i think using innerHTML is a bummer, and prefer textContent ... for text.

and if we realllly needed to parse as HTML, we should use... parseHtmlSubset():
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...

[lwchkg, 2016/04/13 17:46:54]

The sad part it the messages to process is already preprocessed with ICU. So
getStringF doesn't work here.
(P.S. passing to C++ (async) just to fetch an ICU formatted text is lame. WDYT
about creating a ICUMessageFormat library in the Javascript code? Note: we may
be able to adapt the code from AngularJS.)
If an real exploit is possible I prefer not to use innerHTML even if the input
is filtered. Trusting a dev for proper escaping/filtering is almost as bad as
trusting user inputs. Even if the code is correct at a certain time, it is too
easy to have an security-ignorant dev to mess it up in the future.

Now I'm inclined to continue using DOM manipulation even if the code is weird.
Disclaimer to Moe: Dan has the final decision, so listen to him, not me.
parseHtmlSubset(): Why the tag 'A' is the default set allowed tags? No good. The
adversary can craft a malicious URL for users to click, so proper escaping is
still needed.

[Dan Beam, 2016/04/13 23:00:31]

read harder:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...

I have no idea what the rest of your text said

[lwchkg, 2016/04/14 17:43:03]

Thanks for pointing out. I'm still concerned about allowing "https://" in the
URL. This allows linking to attacker sites, or possibly also sending sensitive
information in the URL.
Sorry, I though quite many of us knows about ICU for processing strings, but
turns out it is not after checking Git Blame results.

ICU message is something that deals with singular/plurals and gender in
translations. There's an C++ implementation used in Chromium, and so far the
only users are jshin, msramek and me. (Git Blame also shows Dan because he
touched jshin's strings.)

Here's some documents ICU MessageFormat:
http://userguide.icu-project.org/formatparse/messages. (Note: the use of $1 and
$2 in the strings is this CL is not related to ICU. The braces are though.)

Anyway, in user_pod_row.js, some C++ code is called async to process a ICU
message. (see line 1514, which calls
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...,
which calls l10n_util::GetPluralStringFUTF16 which executes some underlying code
in ICU4C library.

Actually I hoped that the library can be called directly from JavaScript, but
I'm out of luck.

 
-      messageParts = message.split('$1');
-      var numParts = messageParts.length;
-      for (var j = 0; j < numParts; j++) {
-        element.appendChild(document.createTextNode(messageParts[j]));
-        if (j < numParts - 1) {
-          var elementToAdd = document.createElement('span');
-          elementToAdd.classList.add('total-count');
-          elementToAdd.textContent = count;
-          element.appendChild(elementToAdd);
-        }
-      }
       this.moveActionMenuUpIfNeeded_();
     },
 
     /**
      * Handles a click event on remove user confirmation button.
      * @param {Event} e Click event.
      */
     handleRemoveUserConfirmationClick_: function(e) {
       if (this.isActionBoxMenuActive) {
         this.isActionBoxMenuActive = false;
@@ skipping 731 matching lines @@
         focus: [this.handleFocus_.bind(this), true /* useCapture */],
         click: [this.handleClick_.bind(this), true],
         mousemove: [this.handleMouseMove_.bind(this), false],
         keydown: [this.handleKeyDown.bind(this), false]
       };
 
       var isDesktopUserManager = Oobe.getInstance().displayType ==
           DISPLAY_TYPE.DESKTOP_USER_MANAGER;
       this.userPodHeight_ = isDesktopUserManager ? DESKTOP_POD_HEIGHT :
                                                    CROS_POD_HEIGHT;
-      // Same for Chrome OS and desktop.
-      this.userPodWidth_ = POD_WIDTH;
+      this.userPodWidth_ = isDesktopUserManager ? DESKTOP_POD_WIDTH :
+                                                  CROS_POD_WIDTH;
     },
 
     /**
      * Returns all the pods in this pod row.
      * @type {NodeList}
      */
     get pods() {
       return Array.prototype.slice.call(this.children);
     },
 
@@ skipping 446 matching lines @@
       return 2 * rowPadding + rows * this.userPodHeight_;
     },
 
     /**
      * Calculates number of columns and rows that podrow should have in order to
      * hold as much its pods as possible for current screen size. Also it tries
      * to choose layout that looks good.
      * @return {{columns: number, rows: number}}
      */
     calculateLayout_: function() {
+      var isDesktopUserManager = Oobe.getInstance().displayType ==
+          DISPLAY_TYPE.DESKTOP_USER_MANAGER;
       var preferredColumns = this.pods.length < COLUMNS.length ?
           COLUMNS[this.pods.length] : COLUMNS[COLUMNS.length - 1];
       var maxWidth = Oobe.getInstance().clientAreaSize.width;
       var columns = preferredColumns;
       while (maxWidth < this.columnsToWidth_(columns) && columns > 1)
         --columns;
       var rows = Math.floor((this.pods.length - 1) / columns) + 1;
       if (getComputedStyle(
           $('signin-banner'), null).getPropertyValue('display') != 'none') {
         rows = Math.min(rows, MAX_NUMBER_OF_ROWS_UNDER_SIGNIN_BANNER);
       }
-      var maxHeigth = Oobe.getInstance().clientAreaSize.height;
-      while (maxHeigth < this.rowsToHeight_(rows) && rows > 1)
-        --rows;
+      if (!isDesktopUserManager) {
+        var maxHeigth = Oobe.getInstance().clientAreaSize.height;
+        while (maxHeigth < this.rowsToHeight_(rows) && rows > 1)
+         --rows;
+      }
       // One more iteration if it's not enough cells to place all pods.
       while (maxWidth >= this.columnsToWidth_(columns + 1) &&
              columns * rows < this.pods.length &&
              columns < MAX_NUMBER_OF_COLUMNS) {
          ++columns;
       }
       return {columns: columns, rows: rows};
     },
 
     /**
@@ skipping 513 matching lines @@
        if (pod && pod.multiProfilesPolicyApplied) {
          pod.userTypeBubbleElement.classList.remove('bubble-shown');
        }
      }
   };
 
   return {
     PodRow: PodRow
   };
 });