28687 2009-08-24 14:49:59 -0700 should not pass URI fragments to libsoup 2009-09-08 13:07:43 -0700 1 1 1 Unclassified WebKit WebKitGTK 528+ (Nightly build) PC Linux RESOLVED FIXED P2 Normal --- 0 danw webkit-unassigned gustavo xan.lopez oldest_to_newest 142421 0 danw 2009-08-24 14:49:59 -0700 (pointed out by reinouts on irc) If you click on a link like "http://example.com/#comments" in WebKitGTK, it passes the whole URI (including the fragment) to libsoup. Normally libsoup doesn't pass the "#comments" to the remote server, but if you're using a proxy, then it does. And some servers will return a 404 or 403 in that case because they interpret the "#comments" as being part of the path. The fact that libsoup's behavior changes depending on whether you use a proxy is a libsoup bug, but you could argue about which of the two behaviors is the buggy one. I think WebKit should not be passing fragments to libsoup; WebKit wants the whole page back, and so it should be requesting the whole page from libsoup, not just a portion of it. Also, it could be confusing if messages with URIs that are not soup_uri_equal() would result in identical requests, and this could end up tripping up things like caching, etc, as well. It seems wrong for "soup_message_get_uri()" to return a URI with a fragment in it, since the response body actually contains all fragments. But it would be wrong for SoupMessage to clear the fragment from the URI itself too, since that could trip up the app if it expected the URI to match what it had originally set it to. So anyway, I think ResourceHandleSoup should strip the URI fragment from the URI as part of its process of canonicalizing it before passing it to libsoup. 142823 1 xan.lopez 2009-08-26 03:59:27 -0700 So, I've tried something like... diff --git a/WebCore/platform/network/soup/ResourceRequestSoup.cpp b/WebCore/platform/network/soup/ResourceRequestSoup.cpp index f2011bb..eec75e0 100644 --- a/WebCore/platform/network/soup/ResourceRequestSoup.cpp +++ b/WebCore/platform/network/soup/ResourceRequestSoup.cpp @@ -30,9 +30,20 @@ using namespace std; namespace WebCore { +static char* stripFragmentFromURI(char* uri) +{ + char* ptr = g_strrstr(uri, "#"); + if (ptr && *(ptr-1) != '&') + *ptr = '\0'; + + return uri; +} + SoupMessage* ResourceRequest::toSoupMessage() const { - SoupMessage* soupMessage = soup_message_new(httpMethod().utf8().data(), url().string().utf8().data()); + GOwnPtr<char> uri(stripFragmentFromURI(g_strdup(url().string().utf8().data()))); + SoupMessage* soupMessage = soup_message_new(httpMethod().utf8().data(), + const_cast<char*>(uri.get())); if (!soupMessage) return 0; @@ -53,7 +64,7 @@ SoupMessage* ResourceRequest::toSoupMessage() const void ResourceRequest::updateFromSoupMessage(SoupMessage* soupMessage) { SoupURI* soupURI = soup_message_get_uri(soupMessage); - GOwnPtr<gchar> uri(soup_uri_to_string(soupURI, FALSE)); + GOwnPtr<gchar> uri(stripFragmentFromURI(soup_uri_to_string(soupURI, FALSE))); m_url = KURL(KURL(), String::fromUTF8(uri.get())); And all the http tests seem to still pass. Looks reasonable Dan? I first didn't check that the previous character is a & and failed one test that dealt with espaced hex stuff in the URL. Is there any function in soup/glib I should be using to strip the fragment from a string? (Other than going through SoupURI somehow, I suppose). 142830 2 danw 2009-08-26 05:24:47 -0700 You probably want to operate on the parsed URL, not the string form. In ResourceHandle::start(), you have: KURL url = request().url(); String urlString = url.string(); and you could just add a call to url.removeFragmentIdentifier() in between. Likewise in updateFromSoupMessage. (Though I'm not sure if you want m_url to contain the fragment or not.) 145358 3 39187 xan.lopez 2009-09-08 08:12:40 -0700 Created attachment 39187 dontpassfragment.patch Don't pass fragments in URIs to libsoup. 145486 4 39187 gustavo 2009-09-08 12:18:24 -0700 Comment on attachment 39187 dontpassfragment.patch Looks good, thanks! 145512 5 xan.lopez 2009-09-08 13:07:43 -0700 Landed in r48178, closing. 39187 2009-09-08 08:12:40 -0700 2009-09-08 12:18:24 -0700 dontpassfragment.patch dontpassfragment.patch text/plain 2928 xan.lopez RnJvbSBkMWY4ZDVjMTZkY2QzMzljMTVjY2I1NTQ0YTdhNjc3NDhlY2FkMDc5IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBYYW4gTG9wZXogPHhsb3BlekBpZ2FsaWEuY29tPgpEYXRlOiBU dWUsIDggU2VwIDIwMDkgMTg6MTE6MzMgKzAzMDAKU3ViamVjdDogW1BBVENIXSAyMDA5LTA5LTA4 ICBYYW4gTG9wZXogIDx4bG9wZXpAaWdhbGlhLmNvbT4KCiAgICAgICAgUmV2aWV3ZWQgYnkgTk9C T0RZIChPT1BTISkuCgogICAgICAgIHNob3VsZCBub3QgcGFzcyBVUkkgZnJhZ21lbnRzIHRvIGxp YnNvdXAKICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9Mjg2 ODcKCiAgICAgICAgU3RyaXAgdGhlIGZyYWdtZW50IGZyb20gdGhlIFVSSSBiZWZvcmUgcGFzc2lu ZyBpdCB0byBzb3VwLCBzaW5jZQogICAgICAgIGl0IGZvcndhcmRzIGl0IHRvIHNlcnZlcnMgaW4g c29tZSBjYXNlcyAobGlrZSB3aGVuIHVzaW5nIGEgcHJveHkpCiAgICAgICAgd2hpY2ggY29uZnVz ZXMgdGhlbSBhbmQgbWFrZXMgdGhlbSByZXR1cm4gNDAzLzQwNC4KCiAgICAgICAgKiBwbGF0Zm9y bS9uZXR3b3JrL3NvdXAvUmVzb3VyY2VIYW5kbGVTb3VwLmNwcDoKICAgICAgICAoV2ViQ29yZTo6 c3RhcnRIdHRwKToKICAgICAgICAoV2ViQ29yZTo6KToKLS0tCiBXZWJDb3JlL0NoYW5nZUxvZyAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICB8ICAgMTUgKysrKysrKysrKysrKysrCiAu Li4vcGxhdGZvcm0vbmV0d29yay9zb3VwL1Jlc291cmNlSGFuZGxlU291cC5jcHAgICB8ICAgMTEg KysrKysrKystLS0KIDIgZmlsZXMgY2hhbmdlZCwgMjMgaW5zZXJ0aW9ucygrKSwgMyBkZWxldGlv bnMoLSkKCmRpZmYgLS1naXQgYS9XZWJDb3JlL0NoYW5nZUxvZyBiL1dlYkNvcmUvQ2hhbmdlTG9n CmluZGV4IGUxOWYxYzEuLjEzOTYyZjggMTAwNjQ0Ci0tLSBhL1dlYkNvcmUvQ2hhbmdlTG9nCisr KyBiL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTggQEAKKzIwMDktMDktMDggIFhhbiBM b3BleiAgPHhsb3BlekBpZ2FsaWEuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAo T09QUyEpLgorCisgICAgICAgIHNob3VsZCBub3QgcGFzcyBVUkkgZnJhZ21lbnRzIHRvIGxpYnNv dXAKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTI4Njg3 CisKKyAgICAgICAgU3RyaXAgdGhlIGZyYWdtZW50IGZyb20gdGhlIFVSSSBiZWZvcmUgcGFzc2lu ZyBpdCB0byBzb3VwLCBzaW5jZQorICAgICAgICBpdCBmb3J3YXJkcyBpdCB0byBzZXJ2ZXJzIGlu IHNvbWUgY2FzZXMgKGxpa2Ugd2hlbiB1c2luZyBhIHByb3h5KQorICAgICAgICB3aGljaCBjb25m dXNlcyB0aGVtIGFuZCBtYWtlcyB0aGVtIHJldHVybiA0MDMvNDA0LgorCisgICAgICAgICogcGxh dGZvcm0vbmV0d29yay9zb3VwL1Jlc291cmNlSGFuZGxlU291cC5jcHA6CisgICAgICAgIChXZWJD b3JlOjpzdGFydEh0dHApOgorICAgICAgICAoV2ViQ29yZTo6KToKKwogMjAwOS0wOS0wNyAgWGFu IExvcGV6ICA8eGxvcGV6QGlnYWxpYS5jb20+CiAKICAgICAgICAgVW5yZXZpZXdlZCBidWlsZCBm aXguCmRpZmYgLS1naXQgYS9XZWJDb3JlL3BsYXRmb3JtL25ldHdvcmsvc291cC9SZXNvdXJjZUhh bmRsZVNvdXAuY3BwIGIvV2ViQ29yZS9wbGF0Zm9ybS9uZXR3b3JrL3NvdXAvUmVzb3VyY2VIYW5k bGVTb3VwLmNwcAppbmRleCA3MTQwZWE3Li4wYTUyMWVmIDEwMDY0NAotLS0gYS9XZWJDb3JlL3Bs YXRmb3JtL25ldHdvcmsvc291cC9SZXNvdXJjZUhhbmRsZVNvdXAuY3BwCisrKyBiL1dlYkNvcmUv cGxhdGZvcm0vbmV0d29yay9zb3VwL1Jlc291cmNlSGFuZGxlU291cC5jcHAKQEAgLTQ1MCw3ICs0 NTAsNyBAQCBzdGF0aWMgdm9pZCBlbnN1cmVTZXNzaW9uSXNJbml0aWFsaXplZChTb3VwU2Vzc2lv biogc2Vzc2lvbikKICAgICBnX29iamVjdF9zZXRfZGF0YShHX09CSkVDVChzZXNzaW9uKSwgIndl YmtpdC1pbml0IiwgcmVpbnRlcnByZXRfY2FzdDx2b2lkKj4oMHhkZWFkYmVlZikpOwogfQogCi1z dGF0aWMgYm9vbCBzdGFydEh0dHAoUmVzb3VyY2VIYW5kbGUqIGhhbmRsZSwgU3RyaW5nIHVybFN0 cmluZykKK3N0YXRpYyBib29sIHN0YXJ0SHR0cChSZXNvdXJjZUhhbmRsZSogaGFuZGxlKQogewog ICAgIEFTU0VSVChoYW5kbGUpOwogCkBAIC00NTksNyArNDU5LDEyIEBAIHN0YXRpYyBib29sIHN0 YXJ0SHR0cChSZXNvdXJjZUhhbmRsZSogaGFuZGxlLCBTdHJpbmcgdXJsU3RyaW5nKQogCiAgICAg UmVzb3VyY2VIYW5kbGVJbnRlcm5hbCogZCA9IGhhbmRsZS0+Z2V0SW50ZXJuYWwoKTsKIAotICAg IGQtPm1fbXNnID0gaGFuZGxlLT5yZXF1ZXN0KCkudG9Tb3VwTWVzc2FnZSgpOworICAgIFJlc291 cmNlUmVxdWVzdCByZXF1ZXN0KGhhbmRsZS0+cmVxdWVzdCgpKTsKKyAgICBLVVJMIHVybChyZXF1 ZXN0LnVybCgpKTsKKyAgICB1cmwucmVtb3ZlRnJhZ21lbnRJZGVudGlmaWVyKCk7CisgICAgcmVx dWVzdC5zZXRVUkwodXJsKTsKKworICAgIGQtPm1fbXNnID0gcmVxdWVzdC50b1NvdXBNZXNzYWdl KCk7CiAgICAgaWYgKCFkLT5tX21zZykKICAgICAgICAgcmV0dXJuIGZhbHNlOwogCkBAIC01NzMs NyArNTc4LDcgQEAgYm9vbCBSZXNvdXJjZUhhbmRsZTo6c3RhcnQoRnJhbWUqIGZyYW1lKQogICAg ICAgICByZXR1cm4gc3RhcnREYXRhKHRoaXMsIHVybFN0cmluZyk7CiAKICAgICBpZiAoZXF1YWxJ Z25vcmluZ0Nhc2UocHJvdG9jb2wsICJodHRwIikgfHwgZXF1YWxJZ25vcmluZ0Nhc2UocHJvdG9j b2wsICJodHRwcyIpKSB7Ci0gICAgICAgIGlmIChzdGFydEh0dHAodGhpcywgdXJsU3RyaW5nKSkK KyAgICAgICAgaWYgKHN0YXJ0SHR0cCh0aGlzKSkKICAgICAgICAgICAgIHJldHVybiB0cnVlOwog ICAgIH0KIAotLSAKMS42LjQuMQoK