Page MenuHomePhabricator

h1 #firstHeading not HTML-escaped
Closed, ResolvedPublic

Description

Author: wp-asm

Description:
When editing an article with a title containing special characters, such as the
&, the heading in the h1#firstHeading-tag is not being escaped.

Example: http://de.wikipedia.org/w/index.php?title=AT%26T&action=edit

According to Duesentrieb, this bug does not occur in the Cologne Blue skin but
in all other skins (tested on Monobook, Simple, Myskin).


Version: 1.7.x
Severity: trivial
URL: http://de.wikipedia.org/w/index.php?title=AT%26T&action=edit

Details

Reference
bz6986

Related Objects

StatusAssignedTask
ResolvedNone
ResolvedNone

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 9:20 PM
bzimport added a project: MediaWiki-Parser.
bzimport set Reference to bz6986.
bzimport added a subscriber: Unknown Object (MLST).
bzimport created this task.Aug 11 2006, 7:01 PM

ayg wrote:

(In reply to comment #0)

According to Duesentrieb, this bug does not occur in the Cologne Blue skin but
in all other skins (tested on Monobook, Simple, Myskin).

It's a Monobook issue. Simple and Myskin are Monobook-based, Cologne Blue is
Standard-based.

*** This bug has been marked as a duplicate of 3243 ***

wp-asm wrote:

Since I am new to this, I want to apologize in advance in case I am infringing
any policies here, but nevertheless I would like to state what I (think to) have
found out about this bug.

AFAICS, the bug is caused by line 96 of MonoBook.php:
<?php $this->data['displaytitle']!=""?$this->text('title'):$this->html('title') ?>

The displaytitle-if-condition was initially added in revision 13572. Previously,
the code was only $this->text('title'). These two member functions only differ
by the htmlspecialchars function, which is applied in text() but not in html().
After searching the whole source code, it seems to me as if the value of
displaytitle is never assigned (only by a hook, which is disabled by default by
$wgAllowDisplayTitle), therefor always an empty string. That means, logically,
that in the MonoBook-skin, titles always lack html escaping. Regarding all these
facts, my conclusion would be just to change the above (current) code back to
the previous one.

As I initially said, I'm sorry for posting this in case you don't want any code
analyses here, otherwise I would appreciate any comments on this.

  • I have reopened the bug because I don't think it is helpful to mark it as a

duplicate of a bug which is more than one year old and not yet solved. ***

ayg wrote:

(In reply to comment #2)

The displaytitle-if-condition was initially added in revision 13572. Previously,
the code was only $this->text('title'). These two member functions only differ
by the htmlspecialchars function, which is applied in text() but not in html().
After searching the whole source code, it seems to me as if the value of
displaytitle is never assigned (only by a hook, which is disabled by default by
$wgAllowDisplayTitle), therefor always an empty string. That means, logically,
that in the MonoBook-skin, titles always lack html escaping. Regarding all these
facts, my conclusion would be just to change the above (current) code back to
the previous one.

Upon some poking, I agree with all but your suggested fix. The correct fix is
to swap the text and HTML functions:

-<?php $this->data['displaytitle']!=""?$this->text('title'):$this->html('title') ?>
+<?php $this->data['displaytitle']!=""?$this->html('title'):$this->text('title') ?>

This means that if a hook *does* want to change the displayed title somehow, it
will be able to feed HTML tags to the skin, after escaping the title itself.
Not that, as you point out, any official extension currently appears to use
this. I assume this was the intent of r13572.

As I initially said, I'm sorry for posting this in case you don't want any code
analyses here, otherwise I would appreciate any comments on this.

Code analyses do belong here, yep. Submitted patches are also good.

  • I have reopened the bug because I don't think it is helpful to mark it as a

duplicate of a bug which is more than one year old and not yet solved. ***

Well, no, the whole point is to coordinate discussion. However, I erred in
duping it to bug 3243, because 3243 is much broader (probably too broad to be a
single bug, really). I could've sworn I saw this bug before, but whatever.

fixed in r16048

ayg wrote:

*** Bug 6816 has been marked as a duplicate of this bug. ***