Bug 62811 – [filesystem/Chromium] Filesystem paths need proper URL escaping

• Component: DOM
• Status: RESOLVED FIXED

[Eric U.]

Paths aren't being escaped when they're being passed to the KURL constructor in [Worker]AsyncFileSystemChromium::virtualPathToFileSystemURL.  This is breaking paths that contain escape sequences [e.g. %2F] among other things.  See http://code.google.com/p/chromium/issues/detail?id=78860.

[Eric U.]

Created attachment 97512 [details]
Make encodeWithURLEscapeSequences in KURLGoogle.cpp actually do what it says, then use it on filesystem paths.

This fixes KURLGoogle's encodeWithEscapeSequences, which was apparently crippled due to a callsite that no longer exists.  I've removed the obsolete comments, made it actually escape, and used it in the filesystem code.

Tested by running the Blob layout tests.  The blob URL code assumes that this does some escaping, I believe, but so far the lack of it hasn't broken anything [since the strings to be escaped didn't contain anything interesting].  This change doesn't break anything either, as the escaping is harmless in those cases.

[Adam Barth]

Comment on attachment 97512 [details]
Make encodeWithURLEscapeSequences in KURLGoogle.cpp actually do what it says, then use it on filesystem paths.

Looks great, but we'll need a ChangeLog.  Can you write a Chromium API test for this change?

[Eric U.]

Created attachment 97532 [details]
Added changelogs + updated the test code.

Sorry--totally spaced on the changelogs.  I haven't gotten webkit-patch to work since I switched to git, so my process is a bit more ad-hoc.

It turned out there was already a test [I didn't know about that whole directory!], so I've updated it with the new behavior.

[Eric U.]

Created attachment 97534 [details]
Added missing "reviewed by" line to one changelog.

[Adam Barth]

Comment on attachment 97534 [details]
Added missing "reviewed by" line to one changelog.

Looks perfect.  Thanks!

[WebKit Review Bot]

Comment on attachment 97534 [details]
Added missing "reviewed by" line to one changelog.

Clearing flags on attachment: 97534

Committed r89143: <http://trac.webkit.org/changeset/89143>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric U.]

Reopening.  The patch apparently bounced due to a ChromeOS problem.  I can't repro the problem, and it's been 7 weeks [I was on leave], so I'm going to try relanding and watching the bots to see what happens [if anything].

[Eric U.]

Created attachment 103267 [details]
The same patch, merged and up to date.

Here's the patch for archival purposes; I'm going to land it manually.

[Eric U.]

Created attachment 104581 [details]
Added the needed unescaping fix.

Adam, I've added the fix for the CrOS problem.  It turns out that it should have broken a bunch of our other tests, but as DumpRenderTree runs fast/files tests with security turned off, the broken code never got run.  Please take another look just to make sure I've put this in the right place.

[Adam Barth]

Comment on attachment 104581 [details]
Added the needed unescaping fix.

This seems related to https://bugs.webkit.org/show_bug.cgi?id=30225.  How do these two bugs relate?

[Eric U.]

(In reply to comment #11)
> (From update of attachment 104581 [details])
> This seems related to https://bugs.webkit.org/show_bug.cgi?id=30225.  How do these two bugs relate?

I think they're actually independent.  My change is in KURLGoogle.cpp, and in the decoding of security origins for blob and filesystem URLs.  That one is mostly in KURL.cpp [so not interacting with KURLGoogle.cpp changes], and in location.href for all kinds of URLs.  There will be some overlap where you need to fetch location.href of a page loaded from a blob or filesystem URL, but then we just need make sure that intersection works, and both changes are still needed.

Note that my change makes KURLGoogle.cpp match the behavior of KURL.cpp more closely, in that their encodeWithURLEscapeSequences actually does what it says on the tin, and ours hasn't up to now.  I'd expect that to make things easier for 30225, not harder, so I expect no clash.

If we want to make sure of that, we can use a Chromium test that checks location.href from a filesystem URL of a file with % in the name.  That doesn't test non-Chromium filesystem URL support, but AFAIK there aren't any yet.

[Adam Barth]

Comment on attachment 104581 [details]
Added the needed unescaping fix.

Ok.  Thanks for looking at the other bug.

[WebKit Review Bot]

Comment on attachment 104581 [details]
Added the needed unescaping fix.

Clearing flags on attachment: 104581

Committed r93750: <http://trac.webkit.org/changeset/93750>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.