I think this happens since M66.
IDEOGRAPHIC SPACE is banned for avoiding URL spoofing with some UnescapeRule rules.
https://chromium-review.googlesource.com/1014367
https://bugs.chromium.org/p/chromium/issues/detail?id=824715
We'd need either to add new UnescapeRule for externalfile: case or to use another existing one that matches its purpose.
https://cs.chromium.org/chromium/src/net/base/escape.h?l=64

Weifang - I think this is a real issue, but probably lower priority (P2)?

It made a lot of my files inaccessible from Files app, so I'd still say it's P1.

> I think this happens since M66.
This is not true. Change 1014367 was merged to branch 3396, which should be M67.

Given this is a regression, we should prioritize as a P1 for M-68 since M-67 is headed for Stable next week.

Is this a regression? tfiga@, did this work before?

I confirmed this bug does not happen on 66.0.3359.181 (stable).

Joel -- could you PTAL at this for M68, with possibility to merge back to M67? :)

+mmenke as the owner of crrev.com/c/1014367 which affected this.

We should probably modify the externalfile handler to handle these URLs, since I don't think we want to start displaying spaces in the omnibox again.

I think this is going to impact not small number of the Japanese users (and possibly some other CJK languages?), because most major Japanese IMEs enters full-width space instead of ASCII space by default, and not a small number of Drive files with Japanese titles would be affected.

The workaround is to open the file from web Drive.

Considering M67 stable is tomorrow, I made a patch which I think to be the quickest for now.
https://chromium-review.googlesource.com/c/chromium/src/+/1085207

Re: #12
> We should probably modify the externalfile handler to handle these URLs, since I don't think we want to start displaying spaces in the omnibox again.
I think the URL unescape is not just for displaying in the address bar, but can be used for data that is read by programs (like mentioned in the source comment about copy-pasting, etc).
So I think one of the possible option is to add mode flag to allow/disallow the space-like characters like in my patch. WDYT?

Thank you yamaguchi-san! :)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f0ea677d7afe2052c321dff04d7021f426bf7c11

commit f0ea677d7afe2052c321dff04d7021f426bf7c11
Author: Tatsuhisa Yamaguchi <yamaguchi@google.com>
Date: Tue Jun 05 05:03:30 2018

Allow to unescape non-ASCII space characters for externalfile: URL.

The unescaped URL on this part is not shown as a part of UI. So it will
not be used for URL spoofing.

This change adds URL unescape mode flag to allow whitespace characters.
The option is OFF by default and thus should not affect existing code
using the function.

IDEOGRAPHIC_SPACE can be typed by hitting a space key while Japanese
IME is ON (and possibly in some other CJK languages as well). So user
can intentionally or unintentionally insert IDEOGRAPHIC_SPACE character
in Google Drive document title. Before this change, opening a hosted
Google Drive file from the Files app failed due to mismatch of the file
name when the doc title contained such characters.

Bug:  847039 
Test: net_unittests --gtest_filter=EscapeTest*:FilenameUtil*
Change-Id: Ica066e45df2ff1818aaa1a3325aaea5ea000ce9e
Reviewed-on: https://chromium-review.googlesource.com/1085207
Commit-Queue: Tatsuhisa Yamaguchi <yamaguchi@chromium.org>
Reviewed-by: Asanka Herath <asanka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564361}
[modify] https://crrev.com/f0ea677d7afe2052c321dff04d7021f426bf7c11/chrome/browser/chromeos/fileapi/external_file_url_util.cc
[modify] https://crrev.com/f0ea677d7afe2052c321dff04d7021f426bf7c11/net/base/escape.cc
[modify] https://crrev.com/f0ea677d7afe2052c321dff04d7021f426bf7c11/net/base/escape.h
[modify] https://crrev.com/f0ea677d7afe2052c321dff04d7021f426bf7c11/net/base/escape_unittest.cc

This bug requires manual review: Request affecting a post-stable build
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This seems like the same as  Issue 585422  but in the externalfile: protocol (rather than file:). I have a WIP fix for this in https://crrev.com/c/1053672 but that shouldn't be merged.

The fix that was landed earlier today isn't "great" because it's patching around the problem rather than addressing it properly. But it seems sufficient for a quick merge back to M67. I will comment on the CL and we can make a proper fix for M69, along with file URLs.

We have a customer who have this issue on Windows 7 and 10 devices with Chrome 67.0.3396.62 and 69.0.3449.2. Example file name 'テスト　全角スペース.pdf'
I can also reproduce on Linux machine with 67.0.3396.62 

Files in [Downloads] and local disks (like USB flash drive) will also be affected by this pattern at M67.
(The bug became visible at the same timing as this issue, however that will be fixed by the fix crrev.com/c/1053672 for  Issue 585422 .)

Workaround for this issue would be to rename files and folders manually.

For Windows and Linux, it's tracked by  Issue 585422 .
(That issue had happened with smaller set of characters before. At the current version of M67, it also affects full-width space. Same as #21.)

M67 for Desktop already went out on 05/29, we're planning M67 stable respin release tomorrow which includes critical bug fixes. We won't be able to take this merge in for tomorrow's M67 stable release, reason below:
* Change listed at #16 is not baked in canary yet.
*  Issue 585422  is very old, exists since M49.
* Workaround is available at #21.

Pls let us know ASAP if there is any concern here. Thank you,

I got that the fix won't be merged to M67 stable respin for desktops.
What is our decision with ChromeOS?

> *  Issue 585422  is very old, exists since M49.
This issue is a bit different from 585422. 
- 585422: local files on all platforms
- 847039: Drive files on CrOS
Especially from users perspective, this would look like a new issue.

But the other 2 reasons are ture. So I don't have objection on the decision.

 Issue 585422  is old, but an M67 change made the issue apply to a lot more characters (And characters that people are more likely to actually use in files - various unicode spaces, as opposed to just weird icons and byte order characters).

Chrome OS stable is eminent as well, so any changes need to be high impact / priority / blocking.  I'm also inclined to pass for Chrome OS per the workaround at #21, and per the size of the change linked in the CL.

#26, how do we communicate the need to rename the files to users who can't open their files? Do you realize that for some people it would mean going over tens of files and renaming them? Can I ask you to rename my files for me (since I don't want to spent my time on doing so)?

I don't think this issue is blocked on 585422. A fix for this issue will modify external_file_util.cc whereas fix for 585422 is onto filename_util.cc.

Your change meets the bar and is auto-approved for M68. Please go ahead and merge the CL to branch 3440 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bee593475b9b1c76b819cc6ebd2ca68e70b0b073

commit bee593475b9b1c76b819cc6ebd2ca68e70b0b073
Author: Tatsuhisa Yamaguchi <yamaguchi@google.com>
Date: Mon Jun 11 11:55:30 2018

Allow to unescape non-ASCII space characters for externalfile: URL.

The unescaped URL on this part is not shown as a part of UI. So it will
not be used for URL spoofing.

This change adds URL unescape mode flag to allow whitespace characters.
The option is OFF by default and thus should not affect existing code
using the function.

IDEOGRAPHIC_SPACE can be typed by hitting a space key while Japanese
IME is ON (and possibly in some other CJK languages as well). So user
can intentionally or unintentionally insert IDEOGRAPHIC_SPACE character
in Google Drive document title. Before this change, opening a hosted
Google Drive file from the Files app failed due to mismatch of the file
name when the doc title contained such characters.

Bug:  847039 
Test: net_unittests --gtest_filter=EscapeTest*:FilenameUtil*
Change-Id: Ica066e45df2ff1818aaa1a3325aaea5ea000ce9e
Reviewed-on: https://chromium-review.googlesource.com/1085207
Commit-Queue: Tatsuhisa Yamaguchi <yamaguchi@chromium.org>
Reviewed-by: Asanka Herath <asanka@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#564361}(cherry picked from commit f0ea677d7afe2052c321dff04d7021f426bf7c11)
Reviewed-on: https://chromium-review.googlesource.com/1095234
Reviewed-by: Tatsuhisa Yamaguchi <yamaguchi@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#273}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/bee593475b9b1c76b819cc6ebd2ca68e70b0b073/chrome/browser/chromeos/fileapi/external_file_url_util.cc
[modify] https://crrev.com/bee593475b9b1c76b819cc6ebd2ca68e70b0b073/net/base/escape.cc
[modify] https://crrev.com/bee593475b9b1c76b819cc6ebd2ca68e70b0b073/net/base/escape.h
[modify] https://crrev.com/bee593475b9b1c76b819cc6ebd2ca68e70b0b073/net/base/escape_unittest.cc

Per #28, no need to get snarky.  We review many many bugs over the course of the release so capturing context and impact is critical. That's not helping matters.

Per #24,if it's not critical for desktop what makes it critical for Chrome OS?  Not snark, but working to determine if there's a reason unique to Chrome OS that makes it worse?

This issue never happens on desktop, because it doesn't use the externalfile:// scheme other than on Chrome OS.

Desktop was mentioned because this bug had once been confused with  Issue 585422  having OS:Windows label in the past.
The 2 bugs are similar but are happening in individual places. Thus needs to be fixed separately.

Releasing for now, due to other priorities.
So far I found there's the ARC URL which is escaped partially by some reason which I don't know and also invokes the externalfile: URL converter after that. If we just change code for externalfile URL converter, it doubles the decoding/encoding for some letters like '&'. We'd need to resolve it simultaneously.

What's the next steps here?

From #33, I understand that this is now fixed in ToT and 68 (thanks for quick fix yamaguchi@-san).

On my Chromebook running Beta it is still broken. I personally can wait few more days for 68 to be promoted to Beta, but I wonder if we are really fine with people losing access to their data on stable 67.

Have the M68 / ToT updates been confirmed as working as intended with no unintended impacts?  If so I'll approve for M67.  Thanks.

This needs immediate response per #38 or it won't be included on M67.  Testing confirmation, owner assignment.  Thanks

[yamaguchi]:  Think the ball's in your court here?

I am unable to confirm my change on M67 canary because my devices fails to boot with the latest stable image. It is potentially owing to the workstation on which I make bootable USB drive. I am still investigating, but can someone else give a try?

Additionally I don't know systematic way to probe that there's "no unintended impacts". Should we have it certified by the test team with the proper formal process, or is it fine to just do some random ad-hoc tests?

As the owner of the change, I think it would be suffice to see if local/Drive files can be opened from Files app, as well as smoke testing the browser with some web URLs just in case.

Re-assigning to the original owner.... Please delegate rather than unassigning this late into M67.  
- I'm asking if this has been verified in M68/M69/ToT rather than M67?  Is the fix working?  
- Has it been tested on more than one board?  
- What's your feel for risk given M67 is already in stable?

Thanks

Thanks for explanation.
After a discussion the Files app team decided not to fix the issue on M67, considering that we don't have bandwidth to do enough test to be very confident with the fix.

I can confirm that my files are accessible again on M68. Thanks for a fix.

This is resolved on M68. Closing as fixed for issue tracking.
There are some related issues but can be tracked individually.
- The fix is ad-hoc and not essential. We'll refactor the fix applied by this Issue. See Issue 849998.
- Similar case is seen with the files under Downloads. See  Issue 864434 
- We cannot open PDF in Drive due to Issue 862282 at this moment, but that's not specific to full-width space.

We have tested with our Windows 10 version 1803 (OS Build 17134.165) with Chrome Browser stable version 68.0.3440.75 (Official Build) 64 bit and found this issue was reproduced. Can you re-confirm if any complete solutions steps to offer to our Japanese customer ?

I don't believe we support the externalfile scheme on Windows.  Do you mean file URLs?

I think Chrome on Windows would also be affected by  Issue 585422  when opening local files (regardless of if it's synced from Drive by "Backup and Sync") as file: URLs, and it will be resolved by M69.

yamaguchi@: I'm trying to understand this fix to clean up the code. As I said above, having this fix for decoding non-ASCII spaces isn't ideal, since technically we should be decoding *every* character.

Maybe I'm missing something, but why does the code currently use UnescapeRule::NONASCII_SPACES but not UnescapeRule::SPACES? Doesn't that mean we can properly open a file with a U+3000 IDEOGRAPHIC SPACE but not a file with a simple U+0020 SPACE?

(Note: I can't test now due to Issue 867340.)

I remember like U+00020 had a separate logic to be handled specifically. We can see it appears without encoded in the address bar text. I will examine where it was and how it is in ToT.

Anyways I am about to delete NONASCII_SPACE which I added before, by crrev.com/c/1142854/ .

Re: #49
VirtualPathToExternalFileURL is a function that generates externalfile: URL.
This function (and the GURL constructor called by that) doesn't URL-encode [U+0020 SPACE]. Therefore it did not trigger the issue with U+0020. On the other hand IDEOGRAPHIC SPACE is escaped by the GURL constructor. I've left some comments in crrev.com/c/1142854/ about those.

GURL doesn't escape spaces in external file URLs?  That sounds unexpected.  It certainly does in HTTP urls and file URLs.

> Anyways I am about to delete NONASCII_SPACE which I added before, by crrev.com/c/1142854/.

Thanks. I'll take a look at that. I'm trying to come up with a general fix for all of the URL decoders but there are a *lot* of them (around 50 places in Chrome that use URL_SPECIAL_CHARS_EXCEPT_PATH_SEPARATORS). I'll take your CL into account.

#52 Indeed... this is probably something to do with the fact that externalfile: is "not a special scheme", while http:, https: and file: are "special schemes" (per https://url.spec.whatwg.org/#special-scheme). Special and non-special have radically different parsing behaviour. Filed Issue 867766.

It does seem quite problematic that space isn't being encoded when producing an externalfile URL (and that we're relying on that when going the other way; presumably if I manually replace space with %20, it won't look up the correct file). All the more reason to decode every character, not just non-ASCII spaces.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/37ec335ea923b4004d10593f13ce009c96850607

commit 37ec335ea923b4004d10593f13ce009c96850607
Author: Tatsuhisa Yamaguchi <yamaguchi@chromium.org>
Date: Tue Jul 31 20:07:40 2018

Unescape all escaped characters in externalfile: URL.

Resolve error opening files whose name contain blacklisted characters
for URL spoofing, such as "🔒" (U+1F512).

This change also removes the tentative hack at crrev.com/c/1085207 for
 crbug.com/847039 .


Bug:  847039 ,849998
Test: unit_tests --gtest_filter=ExternalFileURLUtilTest.*:FileManagerPath.*
Test: net_unittests --gtest_filter=EscapeTest.*
Change-Id: Icd8ba4c73f09b97e05c9af182d338e709d5876f6
Reviewed-on: https://chromium-review.googlesource.com/1142854
Commit-Queue: Asanka Herath <asanka@chromium.org>
Reviewed-by: Asanka Herath <asanka@chromium.org>
Reviewed-by: Matt Giuca <mgiuca@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579543}
[modify] https://crrev.com/37ec335ea923b4004d10593f13ce009c96850607/chrome/browser/chromeos/fileapi/external_file_url_util.cc
[modify] https://crrev.com/37ec335ea923b4004d10593f13ce009c96850607/chrome/browser/chromeos/fileapi/external_file_url_util_unittest.cc
[modify] https://crrev.com/37ec335ea923b4004d10593f13ce009c96850607/net/base/escape.cc
[modify] https://crrev.com/37ec335ea923b4004d10593f13ce009c96850607/net/base/escape.h
[modify] https://crrev.com/37ec335ea923b4004d10593f13ce009c96850607/net/base/escape_unittest.cc

We would like to merge https://chromium-review.googlesource.com/1142854 to M69.
It resolves same type of problem with this issue report. We had issue with some special characters like U+1F512 and so in additino to a full-width space.
The other one merged at #33 was an ad-hoc fix only for that character, and this one is more sophisticated one that also covers all the other cases.

This bug requires manual review: M69 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Merge approved, M69.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2b721e07201aecd7d0eb4f5596a4d8ca1cf5d3d7

commit 2b721e07201aecd7d0eb4f5596a4d8ca1cf5d3d7
Author: Tatsuhisa Yamaguchi <yamaguchi@chromium.org>
Date: Fri Aug 10 01:43:09 2018

Unescape all escaped characters in externalfile: URL.

Resolve error opening files whose name contain blacklisted characters
for URL spoofing, such as "🔒" (U+1F512).

This change also removes the tentative hack at crrev.com/c/1085207 for
 crbug.com/847039 .


Bug:  847039 ,849998
Test: unit_tests --gtest_filter=ExternalFileURLUtilTest.*:FileManagerPath.*
Test: net_unittests --gtest_filter=EscapeTest.*
Change-Id: Icd8ba4c73f09b97e05c9af182d338e709d5876f6
Reviewed-on: https://chromium-review.googlesource.com/1142854
Commit-Queue: Asanka Herath <asanka@chromium.org>
Reviewed-by: Asanka Herath <asanka@chromium.org>
Reviewed-by: Matt Giuca <mgiuca@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#579543}(cherry picked from commit 37ec335ea923b4004d10593f13ce009c96850607)
Reviewed-on: https://chromium-review.googlesource.com/1170362
Reviewed-by: Tatsuhisa Yamaguchi <yamaguchi@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#532}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/2b721e07201aecd7d0eb4f5596a4d8ca1cf5d3d7/chrome/browser/chromeos/fileapi/external_file_url_util.cc
[modify] https://crrev.com/2b721e07201aecd7d0eb4f5596a4d8ca1cf5d3d7/chrome/browser/chromeos/fileapi/external_file_url_util_unittest.cc
[modify] https://crrev.com/2b721e07201aecd7d0eb4f5596a4d8ca1cf5d3d7/net/base/escape.cc
[modify] https://crrev.com/2b721e07201aecd7d0eb4f5596a4d8ca1cf5d3d7/net/base/escape.h
[modify] https://crrev.com/2b721e07201aecd7d0eb4f5596a4d8ca1cf5d3d7/net/base/escape_unittest.cc