Unescape BiDi control chars while parsing data urls

net/base/escape.cc

 // Copyright (c) 2012 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.
 
 #include "net/base/escape.h"
 
 #include <algorithm>
 
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
@@ skipping 102 matching lines @@
   const typename STR::value_type least_sig_digit(
       static_cast<typename STR::value_type>(escaped_text[index + 2]));
   if (IsHexDigit(most_sig_digit) && IsHexDigit(least_sig_digit)) {
     *value = HexDigitToInt(most_sig_digit) * 16 +
       HexDigitToInt(least_sig_digit);
     return true;
   }
   return false;
 }
 
+// Returns true if there is an Arabic Language Mark at |index|. |first_byte|
+// is the byte at |index|.
+template<typename STR>
+bool HasArabicLanguageMarkAtIndex(const STR& escaped_text,
+                                  unsigned char first_byte,
+                                  size_t index) {
+  if (first_byte != 0xD8)
+    return false;
+  unsigned char second_byte;
+  if (!UnescapeUnsignedCharAtIndex(escaped_text, index + 3, &second_byte))
+    return false;
+  return second_byte == 0x9c;
+}
+
+// Returns true if there is a BiDi control char at |index|. |first_byte| is the
+// byte at |index|.
+template<typename STR>
+bool HasThreeByteBidiControlCharAtIndex(const STR& escaped_text,
+                                        unsigned char first_byte,
+                                        size_t index) {
+  if (first_byte != 0xE2)
+    return false;
+  unsigned char second_byte;
+  if (!UnescapeUnsignedCharAtIndex(escaped_text, index + 3, &second_byte))
+    return false;
+  if (second_byte != 0x80 && second_byte != 0x81)
+    return false;
+  unsigned char third_byte;
+  if (!UnescapeUnsignedCharAtIndex(escaped_text, index + 6, &third_byte))
+    return false;
+  if (second_byte == 0x80) {
+    return third_byte == 0x8E ||
+           third_byte == 0x8F ||
+           (third_byte >= 0xAA && third_byte <= 0xAE);
+  }
+  return third_byte >= 0xA6 && third_byte <= 0xA9;
+}
+
 // Unescapes |escaped_text| according to |rules|, returning the resulting
 // string.  Fills in an |adjustments| parameter, if non-NULL, so it reflects
 // the alterations done to the string that are not one-character-to-one-
 // character.  The resulting |adjustments| will always be sorted by increasing
 // offset.
 template<typename STR>
 STR UnescapeURLWithAdjustmentsImpl(
     const STR& escaped_text,
     UnescapeRule::Type rules,
     base::OffsetAdjuster::Adjustments* adjustments) {
@@ skipping 32 matching lines @@
       //
       // Additionally, the Unicode Technical Report (TR9) as referenced by RFC
       // 3987 above has since added some new BiDi control characters.
       // http://www.unicode.org/reports/tr9
       //
       // U+061C ARABIC LETTER MARK         (%D8%9C)
       // U+2066 LEFT-TO-RIGHT ISOLATE      (%E2%81%A6)
       // U+2067 RIGHT-TO-LEFT ISOLATE      (%E2%81%A7)
       // U+2068 FIRST STRONG ISOLATE       (%E2%81%A8)
       // U+2069 POP DIRECTIONAL ISOLATE    (%E2%81%A9)
-
-      unsigned char second_byte;
-      // Check for ALM.
-      if ((first_byte == 0xD8) &&
-          UnescapeUnsignedCharAtIndex(escaped_text, i + 3, &second_byte) &&
-          (second_byte == 0x9c)) {
-        result.append(escaped_text, i, 6);
-        i += 5;
-        continue;
-      }
-
-      // Check for other BiDi control characters.
-      if ((first_byte == 0xE2) &&
-          UnescapeUnsignedCharAtIndex(escaped_text, i + 3, &second_byte) &&
-          ((second_byte == 0x80) || (second_byte == 0x81))) {
-        unsigned char third_byte;
-        if (UnescapeUnsignedCharAtIndex(escaped_text, i + 6, &third_byte) &&
-            ((second_byte == 0x80) ?
-             ((third_byte == 0x8E) || (third_byte == 0x8F) ||
-              ((third_byte >= 0xAA) && (third_byte <= 0xAE))) :
-             ((third_byte >= 0xA6) && (third_byte <= 0xA9)))) {
+      //
+      // However, escaping these characters in data: urls result in

[brettw, 2014/10/20 18:04:42]

This sentence doesn't make sense to me. Here, we're unescaping characters but
this comment talks about escaping them (since they're not ASCII, they will
always be escaped when converted to a data URL). The old code was avoiding
unescaping them for security reasons. The new code unescapes them sometimes when
converting to the actual data, but not for display purposes. The comment should
make this clear.

[meacer, 2014/10/20 22:54:22]

Done.

+      // escaped BiDi control characters being displayed in the rendered html,

[brettw, 2014/10/20 18:04:42]

The use of the word "render" in this patch (both here and in the description) is
confusing to me. I think of rendering a URL as printing it to the user, as in
the omnibox. Maybe we can use the phrase "load the data URL" or something. To
blink, data URLs are loaded like any other URL.

[meacer, 2014/10/20 22:54:22]

Edited CL description to make it clear.

+      // so the parsing for data: urls is allowed to force unescaping of these
+      // characters. DO NOT use BIDI_CONTROL_CHARS flag without talking to a
+      // security person.
+      if (!(rules & UnescapeRule::BIDI_CONTROL_CHARS)) {
+        if (HasArabicLanguageMarkAtIndex(escaped_text, first_byte, i)) {
+          result.append(escaped_text, i, 6);
+          i += 5;
+          continue;
+        }
+        if (HasThreeByteBidiControlCharAtIndex(escaped_text, first_byte, i)) {
           result.append(escaped_text, i, 9);
           i += 8;
           continue;
         }
       }
 
       if (first_byte >= 0x80 ||  // Unescape all high-bit characters.
           // For 7-bit characters, the lookup table tells us all valid chars.
           (kUrlUnescape[first_byte] ||
            // ...and we allow some additional unescaping when flags are set.
@@ skipping 196 matching lines @@
                        1, kEscapeToChars[i].replacement);
           break;
         }
       }
     }
   }
   return text;
 }
 
 }  // namespace net