FileURLToFilePath: Don't unescape '/' and '\\'.

net/base/escape.h

 // 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.
 
 #ifndef NET_BASE_ESCAPE_H_
 #define NET_BASE_ESCAPE_H_
 
 #include <stdint.h>
 
 #include <string>
@@ skipping 75 matching lines @@
     // by other applications.
     SPACES = 2,
 
     // Unescapes various characters that will change the meaning of URLs,
     // including '%', '+', '&', '/', '#'. If we unescaped these characters, the
     // resulting URL won't be the same as the source one. This flag is used when
     // generating final output like filenames for URLs where we won't be
     // interpreting as a URL and want to do as much unescaping as possible.
     URL_SPECIAL_CHARS = 4,
 
+    // Subset of "URL_SPECIAL_CHARS" - excludes '/' and '\\'. For use with file
+    // URLs.
+    // TODO(mmenke): Should GURL unescape those two characters for file URLs?
+    // that's what FireFox and IE do.

[eroman, 2016/02/18 19:37:28]

Worth noting the obvious, that there are compatibility and web-security
ramifications to changing this for general URLs.

Apache for instance by default does not unescape these in paths:
http://httpd.apache.org/docs/current/mod/core.html#allowencodedslashes

Web servers may be checking for directory escaping in urls by just ../ and not
the escaped variants, so if Chrome started transforming could be problematic.

As I understand this comment though it is specifically for file URLs, so sounds
fine.

[mmenke, 2016/02/18 20:30:22]

Actually, I'm thinking that the compatibility and web-security ramifications are
more around *not* using this for URLs (Of any type), and that's where the scary
comment should go instead.  Unescaping slashes on GURLs or GURL components for
any reason seems like something that's generally not a good idea, with the
exception of Javascript URLs.  We mostly unescape for either display, or picking
local file names (On file or http URLs).  For display, it changes the semantic
meaning of a URL, so seems like a bad idea.  For local files, it seems like a
potential attack vector, regardless of whether we're talking file or http URLs.

I think we should get rid of URL_SPECIAL_CHARS in favor of this, and a separate
flag just for path characters - I've added a TODO about that, and a warning to
URL_SPECIAL_CHARS.

+    URL_SPECIAL_CHARS_EXCEPT_PATH_SEPERATORS = 8,
+
     // Unescapes characters that can be used in spoofing attempts (such as LOCK)
     // and control characters (such as BiDi control characters and %01).  This
     // INCLUDES NULLs.  This is used for rare cases such as data: URL decoding
     // where the result is binary data.
     //
     // DO NOT use SPOOFING_AND_CONTROL_CHARS if the URL is going to be displayed
     // in the UI for security reasons.
-    SPOOFING_AND_CONTROL_CHARS = 8,
+    SPOOFING_AND_CONTROL_CHARS = 16,

[eroman, 2016/02/18 19:37:28]

optional style: I find the 1 << X notation simpler to edit for bit flags.

[mmenke, 2016/02/18 20:30:22]

SGTM, done.

 
     // URL queries use "+" for space. This flag controls that replacement.
-    REPLACE_PLUS_WITH_SPACE = 16,
+    REPLACE_PLUS_WITH_SPACE = 32,
   };
 };
 
 // Unescapes |escaped_text| and returns the result.
 // Unescaping consists of looking for the exact pattern "%XX", where each X is
 // a hex digit, and converting to the character with the numerical value of
 // those digits. Thus "i%20=%203%3b" unescapes to "i = 3;".
 //
 // Watch out: this doesn't necessarily result in the correct final result,
 // because the encoding may be unknown. For example, the input might be ASCII,
@@ skipping 20 matching lines @@
     UnescapeRule::Type rules,
     base::OffsetAdjuster::Adjustments* adjustments);
 
 // Unescapes the following ampersand character codes from |text|:
 // < > & " '
 NET_EXPORT base::string16 UnescapeForHTML(const base::string16& text);
 
 }  // namespace net
 
 #endif  // NET_BASE_ESCAPE_H_