Bug 30225 (CVE-2012-3696) - window.location.href and others needlessly decodes URI-encoded characters
Summary: window.location.href and others needlessly decodes URI-encoded characters
Status: RESOLVED FIXED
Alias: CVE-2012-3696
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Erik Arvidsson
URL:
Keywords:
: 22899 66836 76320 83485 (view as bug list)
Depends on: 69511
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-08 11:20 PDT by Andrei Popescu
Modified: 2016-07-15 17:15 PDT (History)
22 users (show)

See Also:


Attachments
Make KURL::prettyUrl (used by window.location.href) not URI-decode the URI path. (7.29 KB, patch)
2009-10-08 11:28 PDT, Andrei Popescu
no flags Details | Formatted Diff | Diff
Make KURL::prettyUrl (used by window.location.href) not URI-decode the URI path. (7.29 KB, patch)
2009-10-08 11:31 PDT, Andrei Popescu
no flags Details | Formatted Diff | Diff
Patch (12.82 KB, patch)
2011-08-17 15:42 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (56.49 KB, patch)
2011-09-23 17:41 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (61.68 KB, patch)
2011-10-04 15:59 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (69.69 KB, patch)
2011-10-05 17:26 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch for landing (69.68 KB, patch)
2011-10-05 17:52 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrei Popescu 2009-10-08 11:20:05 PDT
Navigate to a URL that contains URI escape sequences in its path, e.g. http://example.com/foo%3dbar and query window.location.href. This property should return the URI unmodified in any way. What actually happens is that the property contains the URI with its path component being URI-decoded, i.e. http://example.com/foo=bar. 

IE, Mozilla FF, Chrome all return the path unmodified. Safari 4.x and the Android browser decode the path. This problem was actually meant to be addressed after the patch in the bug below landed:

https://bugs.webkit.org/show_bug.cgi?id=23546

but it never happened.

Also note the following bug filed against Android:

http://code.google.com/p/android/issues/detail?id=3861

Uploading patch soon.
Comment 1 Andrei Popescu 2009-10-08 11:28:54 PDT
Created attachment 40895 [details]
Make KURL::prettyUrl (used by window.location.href) not URI-decode the URI path.
Comment 2 Andrei Popescu 2009-10-08 11:31:02 PDT
Created attachment 40896 [details]
Make KURL::prettyUrl (used by window.location.href) not URI-decode the URI path.

Forgot to mark the previous upload as 'patch'.
Comment 3 Eric Seidel (no email) 2009-10-08 12:11:51 PDT
Comment on attachment 40896 [details]
Make KURL::prettyUrl (used by window.location.href) not URI-decode the URI path.

You ChangeLog is duplicated.

I don't really like pathRaw(), maybe encodedPath()?  Or should path be decodedPath()?

fast/dom/Window/Location/location-path-has-is-unmodified.htm is sorta a js test, except you didn't generate the tempate using make-script-test-wrappers.

Why does this need to be run from onload?  It needs to wait for the iframe to load?  Will body onload do that?

(If this needs to wait for onload, then it won't work quite right as a js test, so maybe that's why you ddid it this way.)

+        correctValue = "path%2Dwith-escape%2Dcharacters.html";
+        shouldBe("result", "correctValue");

shouldBeEqualToString("normalizeURL(String(window.frames[0].location.href))", "path%2Dwith-escape%2Dcharacters.html");
would have been better.

Does this actually need to load a valid resource to work?
Comment 4 Darin Adler 2009-10-08 13:19:10 PDT
This should not change the behavior of prettyUrl. You can add a new function with the desired behavior and possibly remove prettyUrl if no one uses it. It doesn't make sense to have a “pretty” URL function that does not decode the path!

You should look at what window.location.href needs and think about what the right name is for it.
Comment 5 Andrei Popescu 2009-10-12 08:53:06 PDT
(In reply to comment #3)
> (From update of attachment 40896 [details])
> You ChangeLog is duplicated.
> 
> I don't really like pathRaw(), maybe encodedPath()?  Or should path be
> decodedPath()?
> 

Actually, I think that using the conversion to String operator is enough. This returns the string that was built in the KURL::parse() method.

> fast/dom/Window/Location/location-path-has-is-unmodified.htm is sorta a js
> test, except you didn't generate the tempate using make-script-test-wrappers.
> 
> Why does this need to be run from onload?  It needs to wait for the iframe to
> load?  Will body onload do that?
> 

Yes.

> (If this needs to wait for onload, then it won't work quite right as a js test,
> so maybe that's why you ddid it this way.)
>

That's right. In fact, I just used the pattern I found in the fast/dom/HTMLObjectElement/object-as-frame.html test that I had to modify. Note that that test also uses an onload handler to test the window.location.href property of an iframe.


 
> +        correctValue = "path%2Dwith-escape%2Dcharacters.html";
> +        shouldBe("result", "correctValue");
> 
> shouldBeEqualToString("normalizeURL(String(window.frames[0].location.href))",
> "path%2Dwith-escape%2Dcharacters.html");
> would have been better.
> 

Done.

> Does this actually need to load a valid resource to work?

Actually it doesn't if I use a data URL. Done.
Comment 6 Andrei Popescu 2009-10-12 08:58:31 PDT
Hi Darin,

(In reply to comment #4)
> This should not change the behavior of prettyUrl. You can add a new function
> with the desired behavior and possibly remove prettyUrl if no one uses it. It
> doesn't make sense to have a “pretty” URL function that does not decode the
> path!
> 


You're right, it would not be "pretty" anymore. prettyURL() is used by the Console so I cannot remove it.

> You should look at what window.location.href needs and think about what the
> right name is for it.

I looked and, if I am right, then the String conversion operator of KURL should be enough, so I do not need to add anything new. This operator calls KURL::string(), which just returns the string that was the output of the parse method. So simply removing the call to prettyURL in Location.cpp seems to do the job and all the layout tests pass.

Thanks,
Andrei
Comment 7 Darin Adler 2009-10-12 09:13:39 PDT
(In reply to comment #6)
> I looked and, if I am right, then the String conversion operator of KURL should
> be enough, so I do not need to add anything new. This operator calls
> KURL::string(), which just returns the string that was the output of the parse
> method. So simply removing the call to prettyURL in Location.cpp seems to do
> the job and all the layout tests pass.

Sounds good.

The way to prove this is with tests. Think about cases where you think this might fail, and write a new test or tests that covers as many cases as possible. At least one test for each distinct thing prettyURL used to do, and also tests based on guesses you might have about what could go wrong. And then make sure you run the tests in other browsers too.
Comment 8 Andrei Popescu 2009-10-12 09:27:29 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > I looked and, if I am right, then the String conversion operator of KURL should
> > be enough, so I do not need to add anything new. This operator calls
> > KURL::string(), which just returns the string that was the output of the parse
> > method. So simply removing the call to prettyURL in Location.cpp seems to do
> > the job and all the layout tests pass.
> 
> Sounds good.
> 
> The way to prove this is with tests. Think about cases where you think this
> might fail, and write a new test or tests that covers as many cases as
> possible. At least one test for each distinct thing prettyURL used to do, and
> also tests based on guesses you might have about what could go wrong. And then
> make sure you run the tests in other browsers too.

I was just about to upload a new patch but I will hold off for now and add more tests. Thanks for the comments!

Andrei
Comment 9 Thomas Steinacher 2010-09-07 01:01:16 PDT
Please fix this bug as it makes it impossible to distinguish URLs like /%25 and /%2525 in a cross-browser manner.
Comment 10 Darin Adler 2010-09-07 14:29:18 PDT
Adam, have you looked into this issue while planning your URL work?
Comment 11 Adam Barth 2010-09-07 14:48:13 PDT
(In reply to comment #10)
> Adam, have you looked into this issue while planning your URL work?

I haven't.  I've been studying the behavior of <a href>.  In general, it makes sense to canonicalize URLs before returning them to web content.  For example, returning % unescaped is clearly problematic because % is a control character in URLs.
Comment 12 Erik Arvidsson 2011-05-20 10:30:03 PDT
For what it is worth HTMLAnchorElement href and other URL DOM bindings use KURL::string which seems to do the right thing. Changing Location::href to use KURL::string as well seems to solve this particular bug without having to update KURL.
Comment 13 jashkenas 2011-05-26 08:42:17 PDT
Chiming in with another request to please fix this issue -- it has the side effect of effectively making the new pushState() API unusable in Safari. If your URL happens to have encoded data in it, like say, the name of a user-entered field -- it's impossible to distinguish their "/" in the title from a true "/', because the auto-decoding makes them appear the same to window.location.pathname.
Comment 14 Adam Barth 2011-05-26 10:09:55 PDT
Thanks.  I'll look at this in connection with the prettyURL audit.
Comment 15 Adam Barth 2011-05-26 22:39:35 PDT
My investigation in Bug 61201 seems to indicate that all callers of prettyURL are wrong.  I'm going to wrap that up first and then continue on to this bug.
Comment 16 jashkenas 2011-05-27 06:35:24 PDT
Many thanks, Adam. -- I very much appreciate you taking a look at this issue.
Comment 17 Erik Arvidsson 2011-08-16 17:44:13 PDT
Taking... My intention is to make Safari match the other browsers here
Comment 18 Erik Arvidsson 2011-08-17 15:42:06 PDT
Created attachment 104257 [details]
Patch
Comment 19 Erik Arvidsson 2011-08-17 15:48:53 PDT
This makes Safari a whole lot more compatible with IE, Firefox and Opera which all return the encoded form for these things.

Sorry about the incomplete ChangeLog files, I forgot that webkit-patch does not pick up changes to these things.
Comment 20 Adam Barth 2011-08-17 16:00:42 PDT
Comment on attachment 104257 [details]
Patch

This patch is great.
Comment 21 Adam Barth 2011-08-17 16:02:23 PDT
 > Sorry about the incomplete ChangeLog files, I forgot that webkit-patch does not pick up changes to these things.

(I should also say that I'm assuming you wrote a nice ChangeLog entry that you'll include when landing the patch.)
Comment 22 Darin Adler 2011-08-17 16:04:11 PDT
Comment on attachment 104257 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104257&action=review

> Source/WebCore/page/Location.cpp:68
> -    const KURL& url = this->url();
> -    // FIXME: Stop using deprecatedString(): https://bugs.webkit.org/show_bug.cgi?id=30225
> -    return url.hasPath() ? url.deprecatedString() : url.deprecatedString() + "/";
> +    return this->url().string();

I don’t see any coverage of "http://foo.com", which is a case that I’d expect to be affected by the hasPath check. Is that covered and I just missed it?

The old code also might do things like omit a "@" or a ":" where the new code would keep it. Is that covered?

> Source/WebCore/platform/KURL.cpp:604
> -    return decodeURLEscapeSequences(m_string.substring(start, m_hostEnd - start));
> +    return m_string.substring(start, m_hostEnd - start);

This change seems entirely separate from the Location.href/toString one. Is there sufficient test coverage? I don’t see tests where hostnames are seen URL-encoded above.

> Source/WebCore/platform/KURL.cpp:729
> -    return decodeURLEscapeSequences(m_string.substring(m_portEnd, m_pathEnd - m_portEnd)); 
> +    return m_string.substring(m_portEnd, m_pathEnd - m_portEnd); 

Are you sure that all callers of path want the URL-escaped form? For example, if I was going to take a file URL’s path, encode as UTF-8 and use it to access a file, I would not want it URL-escaped. Is there any code that does that?
Comment 23 WebKit Review Bot 2011-08-17 16:05:51 PDT
Comment on attachment 104257 [details]
Patch

Attachment 104257 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9419246

New failing tests:
fast/dom/javascript-backslash.html
fast/dom/HTMLObjectElement/object-as-frame.html
Comment 24 Darin Adler 2011-08-17 16:06:12 PDT
Comment on attachment 104257 [details]
Patch

Looks like this will break DOMFileSystemBase::crackFileSystemURL
Comment 25 Darin Adler 2011-08-17 16:07:08 PDT
Fixing the location object seems fine, but the KURL changes are likely to have effects elsewhere. Please thoroughly investigate this before landing the change.
Comment 26 Darin Adler 2011-08-17 16:07:44 PDT
Looks like this will also break createGlobalHDropContent in ClipboardWin.cpp
Comment 27 Darin Adler 2011-08-17 16:09:13 PDT
Looks like this breaks KURL::fileSystemPath in KURLQt.cpp
Comment 28 Darin Adler 2011-08-17 16:09:56 PDT
This might break FrameLoader::defaultObjectContentType when the path extension has characters that are URL-encoded in it.
Comment 29 Adam Barth 2011-08-17 16:10:21 PDT
Comment on attachment 104257 [details]
Patch

Darin's comment make me less enthusiastic about this patch.  :)
Comment 30 Adam Barth 2011-08-17 16:12:31 PDT
Comment on attachment 104257 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104257&action=review

>> Source/WebCore/page/Location.cpp:68
>> +    return this->url().string();
> 
> I don’t see any coverage of "http://foo.com", which is a case that I’d expect to be affected by the hasPath check. Is that covered and I just missed it?
> 
> The old code also might do things like omit a "@" or a ":" where the new code would keep it. Is that covered?

http://trac.webkit.org/browser/trunk/LayoutTests/fast/url/script-tests/standard-url.js is a good place to add this test.  It is covered there, but in an oblique way.  A direct test would be better.
Comment 31 Erik Arvidsson 2011-08-17 16:13:19 PDT
Thanks Darin. I did expect this to need more work.

I'll look into these issues.

Another option is to introduce KURL::encodedPath() and KURL::encodedHost()
Comment 32 Erik Arvidsson 2011-08-17 18:21:38 PDT
Darin, for the record Chromium has used encoded host() and path() for KURL since 1.0 without any currently known issues so I'm hoping I can get this to be the case for KURL in non Chromium code as well.
Comment 33 Adam Barth 2011-08-17 18:31:55 PDT
ClipboardWin.cpp and KURLQt.cpp aren't used by Chromium, so it's important to make sure we don't break these.  I'm less worried about FrameLoader::defaultObjectContentType, although it would be nice to have a test.

DOMFileSystemBase::crackFileSystemURL is used by Chromium, so if this change breaks that function, there must be something going on that I don't fully understand.
Comment 34 Darin Adler 2011-08-18 10:32:07 PDT
(In reply to comment #33)
> DOMFileSystemBase::crackFileSystemURL is used by Chromium, so if this change breaks that function, there must be something going on that I don't fully understand.

I believe this patch changes the behavior of that function so that file extensions that are percent-escaped are be handled differently from the same file extensions when not percent-escaped. I don’t know whether file extensions that are percent-escaped are legal or not.

So, for example if the extension was ".%6Apeg" instead of ".jpeg".
Comment 35 Eric U. 2011-08-24 13:56:43 PDT
(In reply to comment #34)
> (In reply to comment #33)
> > DOMFileSystemBase::crackFileSystemURL is used by Chromium, so if this change breaks that function, there must be something going on that I don't fully understand.
> 
> I believe this patch changes the behavior of that function so that file extensions that are percent-escaped are be handled differently from the same file extensions when not percent-escaped. I don’t know whether file extensions that are percent-escaped are legal or not.
> 
> So, for example if the extension was ".%6Apeg" instead of ".jpeg".

The filesystem should support files named .%6Apeg and .jpeg, and should see them as distinct.  It currently has some bugs in the area, which I'm working on with https://bugs.webkit.org/show_bug.cgi?id=62811.

If you call toURL() on a FileEntry called ".%6Apeg", you should get "filesystem:http://your.origin.com/temporary/.%256Apeg".
Comment 36 Darin Adler 2011-08-24 16:02:49 PDT
My comment is not about whether the old or new behavior is correct. My comment is about the fact that this patch changes behavior, and is not fixing a bug with a test case.

I think a patch targeted at window.location should fix specifically window.location. Fixing the underlying function without testing all the call sites is unnecessarily dangerous.
Comment 37 Adam Barth 2011-08-24 16:58:14 PDT
Yes, definitely.  I asked EricU to look at this bug to help us understand what the correct behavior is and to see how it related to the other bug he's working on.
Comment 38 Darin Adler 2011-08-24 17:10:28 PDT
(In reply to comment #37)
> I asked EricU to look at this bug to help us understand what the correct behavior is and to see how it related to the other bug he's working on.

Thanks EricU. I appreciate having the information!
Comment 39 Erik Arvidsson 2011-09-23 17:41:30 PDT
Created attachment 108568 [details]
Patch
Comment 40 Erik Arvidsson 2011-09-23 17:44:25 PDT
Sorry for not getting back to this sooner.

I ended up decoding the path in the places where it was expecting decoded values like before.

I only tested this on Safari/Mac so I expect some Chromium failures. I'll look at the bots when they come in.
Comment 41 WebKit Review Bot 2011-09-23 20:04:02 PDT
Comment on attachment 108568 [details]
Patch

Attachment 108568 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9842119

New failing tests:
fast/loader/subframe-navigate-during-main-frame-load.html
fast/history/history-back-initial-vs-final-url.html
fast/url/standard-url.html
Comment 42 Erik Arvidsson 2011-10-04 15:59:01 PDT
Created attachment 109710 [details]
Patch
Comment 43 Erik Arvidsson 2011-10-04 16:01:27 PDT
(In reply to comment #42)
> Created an attachment (id=109710) [details]
> Patch

There are still KURL issues that this patch does not fix but (fragments encodes non ascii characters for example) but it does fix the path issue and cleans up the non hierarchical URLs too.
Comment 44 WebKit Review Bot 2011-10-04 16:30:06 PDT
Comment on attachment 109710 [details]
Patch

Attachment 109710 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9958092

New failing tests:
fast/url/standard-url.html
Comment 45 Erik Arvidsson 2011-10-04 17:12:03 PDT
Adam, the bot output does not include "standard-url" and it passing locally (built and tested with --chromium). Any advice?
Comment 46 Adam Barth 2011-10-04 17:22:54 PDT
You're probably testing on a Mac.  I suspect there are different expectations for standard URL on Windows and then Linux needs to further override the windows expectations.
Comment 47 Adam Barth 2011-10-04 17:23:53 PDT
Notice that the fallback path for chromium-linux goes through chromium-win:

https://docs.google.com/drawings/d/1z65SKkWrD4Slm6jobIphHwwRADyUtjOAxwGBVKBY8Kc/edit?hl=en_US
Comment 48 Darin Adler 2011-10-04 17:28:46 PDT
Comment on attachment 109710 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109710&action=review

> LayoutTests/fast/dom/anchor-origin-expected.txt:12
> +data:text/html,foo => null

This shows the test is now broken. The URL has <b> tags in it, but now they disappear. We should fix the test so the <b> tags don’t disappear. I understand they should not be %-escaped, but it’s bad to leave this test in a broken state.

> LayoutTests/fast/url/script-tests/file-http-base.js:39
> -    ["file:///C:/asdf#\\xc2", "file:///C:/asdf#\\xef\\xbf\\xbd"],
> +    ["file:///C:/asdf#\xc2", "file:///C:/asdf#\xc2"],

I don’t understand why you changed what’s being tested here. Changing the result is one thing, but you changed the test input URL as well. Don’t we still want to test that case? Maybe we should test both?

> LayoutTests/fast/url/script-tests/standard-url.js:64
> +  ["javascript:alert(\\t 1 \\n\\r)", "javascript:alert( 1 )"],

Is this really an expected, good test result? Why are the spaces being collapsed here?

> LayoutTests/fast/url/segments.html:4
> -<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
> +<!DOCTYPE html>
>  <html>
>  <head>
> +<meta charset="utf-8">

Why this change? Change log doesn’t mention it.

> Source/WebCore/fileapi/DOMFileSystemBase.cpp:46
> +#include "KURL.h"

This file already uses KURL. No need to add a new include.

> Source/WebCore/loader/FrameLoader.cpp:75
> +#include "KURL.h"

This file already uses KURL. No need to add a new include.

> Source/WebCore/page/Location.cpp:154
>  String Location::toString() const
>  {
> -    if (!m_frame)
> -        return String();
> -
> -    const KURL& url = this->url();
> -    // FIXME: Stop using deprecatedString(): https://bugs.webkit.org/show_bug.cgi?id=30225
> -    return url.hasPath() ? url.deprecatedString() : url.deprecatedString() + "/";
> +    return href();
>  }

If this is just turning around and calling another function, then lets inline this in the header.

> Source/WebCore/platform/win/ClipboardWin.cpp:43
> +#include "KURL.h"

No need to add this include. The file already used KURL member functions so it must already have been including this.

> Source/WebCore/workers/WorkerLocation.cpp:80
>  String WorkerLocation::toString() const
>  {
> -    // FIXME: Stop using deprecatedString(): https://bugs.webkit.org/show_bug.cgi?id=30225
> -    return m_url.hasPath() ? m_url.deprecatedString() : m_url.deprecatedString() + "/";
> +    return href();
>  }

Same here.
Comment 49 Erik Arvidsson 2011-10-05 17:26:25 PDT
Comment on attachment 109710 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109710&action=review

>> LayoutTests/fast/dom/anchor-origin-expected.txt:12
>> +data:text/html,foo => null
> 
> This shows the test is now broken. The URL has <b> tags in it, but now they disappear. We should fix the test so the <b> tags don’t disappear. I understand they should not be %-escaped, but it’s bad to leave this test in a broken state.

Fixed.

>> LayoutTests/fast/url/script-tests/file-http-base.js:39
>> +    ["file:///C:/asdf#\xc2", "file:///C:/asdf#\xc2"],
> 
> I don’t understand why you changed what’s being tested here. Changing the result is one thing, but you changed the test input URL as well. Don’t we still want to test that case? Maybe we should test both?

Fixed. I added a new test line here too.

>> LayoutTests/fast/url/script-tests/standard-url.js:64
>> +  ["javascript:alert(\\t 1 \\n\\r)", "javascript:alert( 1 )"],
> 
> Is this really an expected, good test result? Why are the spaces being collapsed here?

This is how all other browsers behave.

>> LayoutTests/fast/url/segments.html:4
>> +<meta charset="utf-8">
> 
> Why this change? Change log doesn’t mention it.

The test script is now UTF-8. I added a comment to the ChangeLog.

>> Source/WebCore/page/Location.cpp:154
>>  }
> 
> If this is just turning around and calling another function, then lets inline this in the header.

Fixed.
Comment 50 Erik Arvidsson 2011-10-05 17:26:41 PDT
Created attachment 109887 [details]
Patch
Comment 51 Darin Adler 2011-10-05 17:31:01 PDT
Comment on attachment 109887 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109887&action=review

> LayoutTests/ChangeLog:23
> +        * fast/url/segments.html: Use UT-8

Typo
Comment 52 Erik Arvidsson 2011-10-05 17:52:15 PDT
Created attachment 109895 [details]
Patch for landing
Comment 53 WebKit Review Bot 2011-10-05 18:39:14 PDT
Comment on attachment 109895 [details]
Patch for landing

Clearing flags on attachment: 109895

Committed r96779: <http://trac.webkit.org/changeset/96779>
Comment 54 WebKit Review Bot 2011-10-05 18:39:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 55 Ryosuke Niwa 2011-10-05 19:22:56 PDT
It seems like this patch caused fast/history/history-back-initial-vs-final-url.html to fail on Qt:
http://build.webkit.org/results/Qt%20Linux%20Release/r96779%20(38208)/results.html
Comment 56 Erik Arvidsson 2011-10-05 19:51:53 PDT
(In reply to comment #55)
> It seems like this patch caused fast/history/history-back-initial-vs-final-url.html to fail on Qt:
> http://build.webkit.org/results/Qt%20Linux%20Release/r96779%20(38208)/results.html

http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp#L834

Needs to be changed to

QString url = item.url().toString();

since the extra encoding is no longer needed to match KURL.

I can make this change tomorrow when I get in to the office.
Comment 57 Csaba Osztrogonác 2011-10-06 03:48:21 PDT
Fix landed in http://trac.webkit.org/changeset/96789.
But unfortunately there is one more bug: https://bugs.webkit.org/show_bug.cgi?id=69511
Comment 58 Erik Arvidsson 2011-10-06 13:22:20 PDT
*** Bug 22899 has been marked as a duplicate of this bug. ***
Comment 59 Darin Adler 2012-01-23 14:07:23 PST
*** Bug 76320 has been marked as a duplicate of this bug. ***
Comment 60 Randy Hudson 2012-03-28 15:46:39 PDT
Has this fix made its way into Safari 5.1.5?

Using that version of Safari, if I go to this page:
http://limpet.net/mbrubeck/temp/chromium-location-test.html/foo%20bar?foo%20bar

The non-query part of the location is still being decoded.
Comment 61 Adam Barth 2012-04-09 11:41:33 PDT
*** Bug 83485 has been marked as a duplicate of this bug. ***
Comment 62 Brent Fulgham 2016-07-15 17:15:30 PDT
*** Bug 66836 has been marked as a duplicate of this bug. ***