Page MenuHomePhabricator

parsing legacy GeoIP cookies fails (no regex match), enwiki geonotice broken for users with those legacy cookies
Closed, ResolvedPublic

Description

parsing legacy GeoIP cookies fails (no regex match), enwiki geonotice broken for users with those legacy cookies. (I guess this is related to https://gerrit.wikimedia.org/r/217231 I wonder if this is messing up centralnotice too? fundraising? )

log in with existing cookie, go to watchlist, no notice. in JS console there's an empty window.Geo. (just af=vx, the others are empty strings)

log in with a clean cookie jar, go to watchlist and I get the notice.

on Main Page, window.Geo is populated regardless of cookie jar.

high priority because existing functionality has broken but I guess a quick hack would be fine, doesn't have to be a perfect fix. See recent changes at https://en.wikipedia.org/w/index.php?title=MediaWiki:Gadget-geonotice-list.js&action=history for some more context.

P.S. maybe this cookie should be versioned? :)

Event Timeline

jeremyb-phone raised the priority of this task from to Unbreak Now!.
jeremyb-phone updated the task description. (Show Details)
jeremyb-phone set Security to None.
jeremyb-phone removed a subscriber: gerritbot.
jeremyb-phone added a subscriber: aude.

Confirmed that this is a problem, and I think @jeremyb is right that the bug was caused by work for T101819.

I believe geoip.inc.vcl sets GeoIP to persist as long as the session, which could be indefinitely long on a mac for example, so backwards compatibility is a must-have.

I see, so the old format is getting cached for a long time and rejected because it doesn't match the new regexp.

Gilles claimed this task.Jun 24 2015, 6:55 PM

Change 220559 had a related patch set uploaded (by Gilles):
Parse older format of Geo cookies

https://gerrit.wikimedia.org/r/220559

Where can I see an example of a legacy cookie, for smoke testing? On my local machine I'm not seeing any problems, so I guess I must note have one...

You can make a legacy cookie by deleting the ":REGION" term

Example of new format cookie:

$.cookie( 'GeoIP', 'FR:B7:Chaillevette:45.7333:-1.0500:v4' );

Old format:

$.cookie( 'GeoIP', 'FR:Chaillevette:45.7333:-1.0500:v4' );

OK... If I remove region from the .wikipedia.org GeoIP cookie, I don't get any console errors, but apparently on special pages (not just Watchlist) window.Geo is messed up, as above. However it seems fine on article pages (not just on Main Page). Also on the article pages, I get a en.wikipedia.org GeoIP cookie that always corrects itself even if I remove the region.

I wasn't getting console errors but I did use the console to interactively inspect/parse window.Geo in various cases.

so, we already know how to identify cookies that are not the current format: regex does not match.

can we just make that case take the same code path as cookie does not exist?

Hello! Do you guys know when this started? We've got campaigns starting again in earnest for the next FY July 8, but starting to spin up again before that.

It looks like the breaking change was deployed on June 16.

So far, it seems this doesn't affect normal article pages. Not sure exactly which pages it does affect; I have only seen it affect special pages, though it may appear on other namespaces, too... CentralNotice doesn't display banners on special pages. So, I think fundraising and banners in general are not affected, at least for the most part.

We still need to fix this, of course! Also, I'd really like to understand more details of how, why and when all the different GeoIP cookies get set, and why the bug works differently for different namespaces.

Thanks!!

Rrrrg, quick update! It looks like CentralNotice does not get access to the corrected window.Geo value. So, please strike my previous comment! It looks like CentralNotice, FR and banners are affected. Sorry about that!!!

Change 220559 merged by jenkins-bot:
Parse older format of Geo cookies

https://gerrit.wikimedia.org/r/220559

OK, my previous comment was also silly, many apologies... /me sticks neurons together with duct tape... Here is what's really going on. (Really!!)

  • When an old GeoIP cookie format is found, it isn't correctly parsed.
  • CN tries to delete the cookie, but doesn't succeed in doing so, because of a subdomain mixup. (The cookie is at ".wikipedia.org" and CN tries to delete one at "en.wikipedia.org", or wherever we really happen to be.)
  • CN makes a background call to geoiplookup.wikimedia.org, which correctly populates window.Geo.
  • CN sets a GeoIP cookie for the full host name (ex. "en.wikipedia.org").

That's why if you change your .wikipedia.org GeoIP cookie to the old format, it stays that way, and a new en.wikpedia.org (or whatever) GeoIP cookie is created in the new format, and window.Geo is correctly populated...

...unless you're on a special page. In that case, CN bows out before the background call to geoiplookup.wikimedia.org, and window.Geo is left mostly empty.

tl;dr Unless I'm mistaken (again ;p ), this is not affecting CentralNotice or banners. However, for users that have the old-format cookie, there are extra calls to geoiplookup.wikimedia.org. Also, any other scripts or gadgets that use window.Geo will not have access to it on special pages, for those users.

Thanks!!

Gilles removed Gilles as the assignee of this task.Jun 26 2015, 6:08 AM

Can someone take care of backporting my fix on Monday? I'll be on vacation next week.

Change 221057 had a related patch set uploaded (by Ori.livneh):
Parse older format of Geo cookies

https://gerrit.wikimedia.org/r/221057

ori claimed this task.Jun 26 2015, 6:15 AM

Change 221057 merged by Ori.livneh:
Parse older format of Geo cookies

https://gerrit.wikimedia.org/r/221057

so, backport was merged (gerrit 221057) but I don't see anything
relevant in SAL. and it doesn't seem to have propagated to
https://en.wikipedia.org/static/1.26wmf11/extensions/CentralNotice/modules/ext.centralNotice.bannerController/bannerController.js?bustthecache

could someone clarify what the current status is? (I also don't see it
on the SWAT schedule)

so, backport was merged (gerrit 221057) but I don't see anything
relevant in SAL. and it doesn't seem to have propagated to
https://en.wikipedia.org/static/1.26wmf11/extensions/CentralNotice/modules/ext.centralNotice.bannerController/bannerController.js?bustthecache

could someone clarify what the current status is? (I also don't see it
on the SWAT schedule)

Not deployed.

ori closed this task as Resolved.Jun 28 2015, 11:54 PM

Deployed now.

sbassett moved this task from Intake to Done on the Privacy board.Oct 16 2019, 4:35 PM
sbassett removed a project: Patch-For-Review.