After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 730464 - Broken session management in google.com and live.com when DNT setting is enabled
Broken session management in google.com and live.com when DNT setting is enabled
Status: RESOLVED FIXED
Product: epiphany
Classification: Core
Component: Backend
git master
Other Linux
: Normal normal
: ---
Assigned To: Epiphany Maintainers
Epiphany Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-05-20 16:26 UTC by Carlos Garcia Campos
Modified: 2014-05-22 09:23 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
ephy-uri-helpers: Don't encode '-' or '_' in field names (980 bytes, patch)
2014-05-21 11:49 UTC, Bastien Nocera
none Details | Review
ephy-uri-helpers: Don't re-encode parameters ourselves (4.80 KB, patch)
2014-05-21 12:13 UTC, Bastien Nocera
none Details | Review
ephy-uri-helpers: Don't re-encode parameters ourselves (4.80 KB, patch)
2014-05-21 12:40 UTC, Bastien Nocera
committed Details | Review
ephy-uri-helpers: Handle keys without a value (2.16 KB, patch)
2014-05-21 19:58 UTC, Bastien Nocera
committed Details | Review

Description Carlos Garcia Campos 2014-05-20 16:26:47 UTC
It seems we are doing something wrong in ephy_remove_tracking_from_uri(). See https://bugs.webkit.org/show_bug.cgi?id=128730
Comment 1 Carlos Garcia Campos 2014-05-20 17:36:11 UTC
The problem looks similar to bug #724724, but this time due to an _ that is decoded, I've added a new test for this, see:

/lib/ephy-uri-helpers/remove_tracking: **
ERROR:ephy-uri-helpers-test.c:64:test_ephy_uri_helpers_remove_tracking: assertion failed (items[i].output == result): ("https://mail.google.com/mail/u/0/?ui=2&ik=37373eb942&rid=7cea..&auto=1&view=lno&_reqid=1168127&pcd=1&mb=0&rt=j" == "https://mail.google.com/mail/u/0/?ui=2&ik=37373eb942&rid=7cea..&auto=1&view=lno&%5Freqid=1168127&pcd=1&mb=0&rt=j")

The diff is "&_reqid" -> "&%5Freqi". But as you can see there's not garbage in this URL, so there's no reason to modify the request at all. I've just landed a patch to fix that case, which fixes the gmail problem, but only because there's no garbage in the gmail urls when closing the session. The real problem is still there as you can see in the test.

https://git.gnome.org/browse/epiphany/commit/?id=ea6537f38237f4e5cfb424ec793c7f0aee4e6feb
Comment 2 Bastien Nocera 2014-05-21 11:14:48 UTC
The fix was pushed here:
commit ea6537f38237f4e5cfb424ec793c7f0aee4e6feb
Author: Carlos Garcia Campos <cgarcia@igalia.com>
Date:   Tue May 20 19:24:51 2014 +0200

    ephy-uri-helpers: Dot no modify the URI when it doesn't have garbage
    
    We are currently decoding/encoding the uri query and returning it
    unchanged when it doesn't have garbage. We should return NULL instead in
    such case.
    This fixes the session management issues in some sites like gmail, but
    because the URLs processed don't actually have garbage. A URL with
    garbage and an underscore will still fail. This patch adds a unit test
    cover that particular case.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=730464


Thanks for fixing it!
Comment 3 Carlos Garcia Campos 2014-05-21 11:40:04 UTC
I left the bug opened, because the bug is still present, and one of the tests I added in that commit still fails
Comment 4 Bastien Nocera 2014-05-21 11:49:52 UTC
Created attachment 276929 [details] [review]
ephy-uri-helpers: Don't encode '-' or '_' in field names

'-' and '_' are allowed characters in form field names, so don't encode
them.
Comment 5 Bastien Nocera 2014-05-21 12:13:13 UTC
Created attachment 276933 [details] [review]
ephy-uri-helpers: Don't re-encode parameters ourselves

Implement Dan Williams' approach to detecting unwanted parameters
by reusing the original key pair values (without decoding/encoding)
when they are wanted, and simply dropping them when not.

See https://bugzilla.gnome.org/show_bug.cgi?id=724724#c14
Comment 6 Dan Winship 2014-05-21 12:30:47 UTC
(In reply to comment #5)
> Implement Dan Williams' approach to detecting unwanted parameters

Oh, come on! I understand when strangers get us confused, but you know both of us! :)
Comment 7 Bastien Nocera 2014-05-21 12:40:19 UTC
Created attachment 276934 [details] [review]
ephy-uri-helpers: Don't re-encode parameters ourselves

Implement Dan Winship's approach to detecting unwanted parameters
by reusing the original key pair values (without decoding/encoding)
when they are wanted, and simply dropping them when not.

See https://bugzilla.gnome.org/show_bug.cgi?id=724724#c14
Comment 8 Bastien Nocera 2014-05-21 12:42:08 UTC
(In reply to comment #6)
> (In reply to comment #5)
> > Implement Dan Williams' approach to detecting unwanted parameters
> 
> Oh, come on! I understand when strangers get us confused, but you know both of
> us! :)

I swear I was thinking of you :)
Comment 9 Carlos Garcia Campos 2014-05-21 12:54:58 UTC
Review of attachment 276934 [details] [review]:

Thanks!
Comment 10 Bastien Nocera 2014-05-21 13:04:18 UTC
Attachment 276934 [details] pushed as aa861a7 - ephy-uri-helpers: Don't re-encode parameters ourselves
Comment 11 Sergio Villar 2014-05-21 19:39:51 UTC
(In reply to comment #10)
> Attachment 276934 [details] pushed as aa861a7 - ephy-uri-helpers: Don't re-encode
> parameters ourselves

I don't think the patch is correct. It's assuming that each element in the query is a key/value pair, something that is indeed common but not required. That triggers crashes in popular websites as www.yahoo.com. You can see it crashing as well simply by adding some url to the ephy-uri-helpers test case like this:

http://www.test.com/?some&valid&query

Also it isn't fixing the broken session management in google.com for me.
Comment 12 Sergio Villar 2014-05-21 19:40:12 UTC
reopening...
Comment 13 Bastien Nocera 2014-05-21 19:58:43 UTC
Created attachment 276949 [details] [review]
ephy-uri-helpers: Handle keys without a value

Fixes a crash with a URI such as:
http://www.test.com/?some&valid&query
Comment 14 Carlos Garcia Campos 2014-05-22 06:29:30 UTC
(In reply to comment #11)
> (In reply to comment #10)
> > Attachment 276934 [details] [details] pushed as aa861a7 - ephy-uri-helpers: Don't re-encode
> > parameters ourselves
> 
> I don't think the patch is correct. It's assuming that each element in the
> query is a key/value pair, something that is indeed common but not required.

Oh! good point.

> That triggers crashes in popular websites as www.yahoo.com. You can see it
> crashing as well simply by adding some url to the ephy-uri-helpers test case
> like this:
> 
> http://www.test.com/?some&valid&query
> 
> Also it isn't fixing the broken session management in google.com for me.

hmm, I only tried with gmail, not with google, assuming it was the same
Comment 15 Carlos Garcia Campos 2014-05-22 06:30:55 UTC
Review of attachment 276949 [details] [review]:

Ok.
Comment 16 Carlos Garcia Campos 2014-05-22 06:35:58 UTC
(In reply to comment #14)
> > Also it isn't fixing the broken session management in google.com for me.
> 
> hmm, I only tried with gmail, not with google, assuming it was the same

Works for me in google.com as well.
Comment 17 Bastien Nocera 2014-05-22 09:22:58 UTC
Attachment 276949 [details] pushed as 63bd650 - ephy-uri-helpers: Handle keys without a value