The Timeless skin has been prepared by @Isarra and needs a security review before deployment on the beta cluster.
Description
Description
Details
Details
| Subject | Repo | Branch | Lines +/- | |
|---|---|---|---|---|
| Fix category double escaping | mediawiki/skins/Timeless | master | +1 -2 | |
| Refactor TimelessTemplate.php | mediawiki/skins/Timeless | master | +647 -470 |
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | Isarra | T154371 Review and deploy Timeless skin | |||
| Resolved | Isarra | T158011 Security review for Timeless skin |
Event Timeline
Comment Actions
Review of 9613a9d4bc. Overall this looks good. I'm excited to see this be a skin on Wikimedia wikis. Some very minor issues with double escaping and not escaping "safe" values.
Minor security
- in assemblePortlet: '<div role="navigation" class="' . $box['class'] . '" id="' . Sanitizer::escapeId( $box['id'] ) . '"' . Linker::tooltip( $box['id'] ) . '>'; - as a precaution, it'd be good to put the $box['class'] through htmlspecialchars. Even better would be to use Html::openElement for this.
- "TimelessTemplate.php" line 300 - Run the id through Sanitizer::escapeId just in case. Even in the case that we know the value is safe, if the value comes from a different function, I would prefer it be escaped to protect against any future changes.
- "TimelessTemplate.php" line 312 - ditto
- "TimelessTemplate.php" line 652 - ditto
- Double escaped messages:
- "TimelessTemplate.php" line 358 - The placeholder values for makeSearchInput should not be escaped as it gets escaped later
- "TimelessTemplate.php" line 500 - timeless-pagelog is double escaped
- "TimelessTemplate.php" line 506 - timeless-more is double escaped
- "TimelessTemplate.php" line 512 - timeless-languages is double escaped
- "TimelessTemplate.php" line 660 - second argument to Linker::link should go through htmlspecialchars (This is not exploitable due to constraints on what a valid Title is, but nonetheless it should still be escaped. Imagine if someone was writing a story about a person nicknamed Lt, and put all the pages in [[category:Me<]] - then the category would display wrong)
Other misc comments
- The way echo notification bar is shown is kind of a bit hidden/looks kind of odd.
- Some of the SVG files could perhaps be minimized to save bandwidth
- If the sitename is really long, it can overlap the Navigation drop-down in the mid-width view.
- The javascript is kind of confusing - skins.timeless.js has only empty js files - perhaps it should be killed. skins.timeless.mobile despite its name seems to be used for all clients - perhaps it should be renamed.
Comment Actions
Just as a note, since these are all minor non-exploitable issues, they should not prevent putting the skin on betawiki (They should be fixed before deploying to a real wiki).
Comment Actions
"TimelessTemplate.php" line 300 - Run the id through Sanitizer::escapeId just in case. Even in the case that we know the value is safe, if the value comes from a different function, I would prefer it be escaped to protect against any future changes.
"TimelessTemplate.php" line 312 - ditto
"TimelessTemplate.php" line 652 - ditto
If those are all using Html::rawElement or whatever now, do I still need to worry about this? Because eeeeeverything is using Html::stuff now.
skins.timeless.mobile despite its name seems to be used for all clients
I don't actually know how to make it only load at a certain width aside from checking in the file itself. Is there a better way?
Comment Actions
Change 344748 had a related patch set uploaded (by Isarra):
[mediawiki/skins/Timeless@master] Refactor TimelessTemplate.php
Comment Actions
Change 344748 merged by jenkins-bot:
[mediawiki/skins/Timeless@master] Refactor TimelessTemplate.php
Comment Actions
Okay, so it looks like you still need to do escapeId when working with the Html functions. That should be fixed now. Double escaping is probably fixed (documentation is a mite unclear on getMsg, so it's possible I screwed that up, though). Echo was already done, as least sort of (still some issues on mobile). Filed a bug about the images, and for the too long site name.
Still dunno what to do about the js, but we can come back to that later, or something.
Other than that, I think I've addressed everything.
Comment Actions
Still dunno what to do about the js, but we can come back to that later, or something.
I just thought it was slightly confusing, it really doesn't matter though.
Other than that, I think I've addressed everything.
Looks good, although I think there is now too much escaping on category names (e.g. & is now showing up as & in the user interface).
Comment Actions
Change 347169 had a related patch set uploaded (by Isarra):
[mediawiki/skins/Timeless@master] Fix category double escaping
Comment Actions
Change 347169 merged by Isarra:
[mediawiki/skins/Timeless@master] Fix category double escaping