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
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | None | T2209 [DO NOT USE] HTML validity (tracking) | |||
| Resolved | None | T8986 h1 #firstHeading not HTML-escaped |
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.
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.