Page MenuHomePhabricator

enotif_body incorrectly assumes helppage is a page name causing watchlist enotifs to be sent with invalid urls
Closed, ResolvedPublic

Description

Here's the last bit of an enotif email i got (for watchlist edit):

Feedback and further assistance:
http://www.mediawiki.org/wiki/Https://www.mediawiki.org/wiki/Special:MyLanguage/Help:Contents

Maybe something is assuming some msg is a page name when it could be a url.


Version: 1.22.5
Severity: major
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=54835

Details

Reference
bz63269

Event Timeline

bzimport raised the priority of this task from to Unbreak Now!.Nov 22 2014, 2:57 AM
bzimport set Reference to bz63269.
bzimport added a subscriber: Unknown Object (MLST).

Technically caused by I999b97729536dbab4a3a5 but code was broken before (Since 2004 via r6696/20ae0324).

One should not assume mediawiki:helppage is a local page since it can be a url. Instead it should be processed on the php side in UserMailier.php via something like Skin::makeInternalOrExternalUrl, and then substituted into the message like a normal parameter

Sigh, if only we could have used interwiki links (bug 54835).

(In reply to Bawolff (Brian Wolff) from comment #1)

substituted into
the message like a normal parameter

Like $HELPURL à la enotif body, or a standard numbered parameter?
But that's a detail; I don't remember what we decided about parameter changes in enotif body after Ibb795374 .

Change 125219 had a related patch set uploaded by Nemo bis:
Use an actual URL for helppage in enotif_body

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

I submitted a patch with the obvious solution, which however forces wikis to update (almost) any local version of 'enotif_body' or 'helppage' message.

Note, before fbc65f8972fc08a (2011-08-19, 1.19) we used fullurl, which has the same assumption as canonicalurl; before b7eb28ca (bug 8846) 'helppage' was not used in enotif.

For now I could only think of ugly alternatives:

  • regex to fix incorrect URLs after parsing;
  • somehow override the default 'helppage' passed to 'enotif_body', making it an interwiki link that canonicalurl can parse;
  • find some canonicalurl/fullurl trickery so that the magic word doesn't fiddle with full URLs;
  • replace canonicalurl with something smarter (probably pointless, one would have to edit existing messages anyway).

(In reply to Nemo from comment #4)

I submitted a patch with the obvious solution, which however forces wikis to
update (almost) any local version of 'enotif_body' or 'helppage' message.

Note, before fbc65f8972fc08a (2011-08-19, 1.19) we used fullurl, which has
the same assumption as canonicalurl; before b7eb28ca (bug 8846) 'helppage'
was not used in enotif.

For now I could only think of ugly alternatives:

  • regex to fix incorrect URLs after parsing;

ewww. If we consider the current patch unacceptable, I think this is best option.

  • somehow override the default 'helppage' passed to 'enotif_body', making it

an interwiki link that canonicalurl can parse;

Not really an option, since we can't make assumptions about the interwiki table of third party wikis

  • find some canonicalurl/fullurl trickery so that the magic word doesn't

fiddle with full URLs;

Don't think that's possible. Especially not without extensions.


I'm supportive of the patch you submitted. I think mentioning in release notes, and making sure WMF wikis know to change it, is probably enough.

Alternative of course would be to change the message so it doesn't like to the help page. Why would you want to go to a wikitext tutorial from a watchlist notification email?

This probably should be included in 1.22 and 1.23.

Change 125219 merged by jenkins-bot:
Use an actual URL for helppage in enotif_body

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

Change 131267 had a related patch set uploaded by MarkAHershberger:
Use an actual URL for helppage in enotif_body

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

Change 131267 merged by jenkins-bot:
Use an actual URL for helppage in enotif_body

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

Now merged and backported to 1.23, but *not* 1.22. Should we backport it there as well?

(In reply to Bartosz Dziewoński from comment #10)

Now merged and backported to 1.23, but *not* 1.22. Should we backport it
there as well?

Yep

It will need someone to manually do the backport. Cherry-picking doesn't work right now.

Removing 1.23.0 target since it is already merged. Leaving the bug open in case someone works on the backport.

I am backporting this manually to REL1_22 right now. I'd appreciate help with testing it :)

Change 132464 had a related patch set uploaded by Bartosz Dziewoński:
Use an actual URL for helppage in enotif_body

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

(In reply to Bartosz Dziewoński from comment #14)

I am backporting this manually to REL1_22 right now. I'd appreciate help
with testing it :)

I've asked Himeshi here at the Zurich hackathon to look at this.

Change 132464 merged by jenkins-bot:
Use an actual URL for helppage in enotif_body

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