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 154 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)
+      //
+      // However, not unescaping these characters in data urls result in

[Tom Sepez, 2014/10/17 17:08:48]

Nit: "Not unescaping" is a double negative.  Maybe just say "escaping". Also
prefer "data:" rather than "data".

The comment also ties a specific notion (data urls) to a generic mechanism
(BIDI_CONTROL_CHARACTER flag).  The danger is that if we change the places that
call this, then the comment becomes incorrect.

I'm really not sure what to say here, other than there might need to be a
comment about "DO NOT use BIDI_CONTROL_CHARS flag without talking to a security
person" somewhere.

Feel free to ignore all this.

[meacer, 2014/10/17 20:41:54]

This is true. I wanted to test if the actual url being parsed is a data: url but
unfortunately there is no way to know about it at this point in the code. We'll
have to rely on callers to do the right thing.
There was a comment to that effect in escape.h. Making it clearer and adding it
here as well.

+      // escaped BiDi control characters being displayed in the rendered html,
+      // so the parsing for data urls is allowed force unescaping of these
+      // characters.
+      if (!(rules & UnescapeRule::BIDI_CONTROL_CHARS)) {
+        unsigned char second_byte;
+        // Check for ALM.

[Tom Sepez, 2014/10/17 17:08:49]

Nit: expand ALM to Arabic Letter Mark.

[meacer, 2014/10/17 20:41:54]

Done.

+        if ((first_byte == 0xD8) &&

[Tom Sepez, 2014/10/17 17:08:49]

Nit: It took me longer to understand this code than I would have liked. Perhaps
a pair of inline bools IsTwoByteBiDiControlChar() and
IsThreeByteBidiControlChar() might make this clearer, as that gets you away from
a single logical expression using ? && || etc. and allows for some block
structure, early returns, etc.

[meacer, 2014/10/17 20:41:54]

Pulled these into methods (with somewhat questionable method names).

+            UnescapeUnsignedCharAtIndex(escaped_text, i + 3, &second_byte) &&
+            (second_byte == 0x9c)) {
+          result.append(escaped_text, i, 6);
+          i += 5;
+          continue;
+        }
 
-      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)))) {
-          result.append(escaped_text, i, 9);
-          i += 8;
-          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)))) {
+            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.
            (first_byte == ' ' && (rules & UnescapeRule::SPACES)) ||
            // Allow any of the prohibited but non-control characters when
            // we're doing "special" chars.
@@ skipping 193 matching lines @@
                        1, kEscapeToChars[i].replacement);
           break;
         }
       }
     }
   }
   return text;
 }
 
 }  // namespace net