The WMF core features team tracks this bug on Mingle card https://mingle.corp.wikimedia.org/projects/flow/cards/495, but people from the community are welcome to contribute here and in Gerrit.

[Setting this to high priority during to the Wed Dec 4 schedule.]

I'm still working through this, but wanted to get these documented so they can be fixed sooner.

• {{done}} The sql handling really needed extra sanitization (otherwise, prevention of sqli depended on several classes all correctly sanitizing and calling the db handler a certain way). This was addressed in gerrit 98759.

• includes/ParsoidUtils.php. Needs to disable external entities in createDOM

• includes/Templating.php, Line 349, a suppressed revision can be displayed by removing the message. Unlikely it can be maliciously exploited, but not good code.

And from our in-person meeting:

• Username suppression needs to be checked (check for a block with ipb_deleted/mHideName set to 1)

• (not a blocker for deployment) User renaming needs to be handleable by the RenameUser extension

While doing some blackbox testing, I'm also noticing that

• Different users are getting the same token value
• Usernames containing a ' cause a lot of problems

A few more specific issues:

Hooks.php

• Line 234 - please escape $action in query

includes/RecentChanges/Formatter.php

• Should use Linker instead of building <a>'s yourself. Not a blocker.
• Please use escaped() instead of text() for messages in topicHistoryLink, postHistoryLink, topicLink, postLink; or use Html::element instead of rawElement

In general, I'm working on reviewing the templates, although the structure makes them very difficult to review. I'm probably not going to be able to complete the code review by tomorrow. I've been doing some testing on the frontend, I'm happy with the xss filtering for the page itself and recent changes, but I'm not able to use the board-history, either in the labs ee-flow instance, or my local dev which is running both master and the version I'm review from last Friday. If that can be fixed before tomorrow, I'll work on fuzzing it. If not, then I'm assuming the deployment will be held off for it anyway?

Hooks.php line 234 - addressed in https://gerrit.wikimedia.org/r/99019

The board-history is also now working again, you shoulsd be able to run fuzz testing.

Thanks for digging through this stuff, we will be addressing these other issues shortly.

(In reply to comment #5)

• Different users are getting the same token value

This was unrelated

• includes/ParsoidUtils.php disable external entities - https://gerrit.wikimedia.org/r/99164

• includes/Templating.php - display of suppressed revision - https://gerrit.wikimedia.org/r/99166

• Username suppression - working on this, it will be a day or two to complete as we display the names in a variety of placs.

• Usernames containing a ' - Could you provide more details on where this causes issues? I ran through the various pages and actions and don't see anything yet, will be looking more thoroughly.

• RecentChanges/Formatter - escaping link text - https://gerrit.wikimedia.org/r/99170

Looking over the code involved, we are going to be holding off on the deployment until either tomorrow or next week, probably next week.

includes/Model/UUID.php

• only show backtrace if $wgShowExceptionDetails is true

includes/Repository/SelectQueryBuilder.php

• escape or validate table, field and op in query()

includes/Data/BoardHistoryStorage.php

• findTopicListHistory(): validate/filter $queries, or address in RevisionStorage

includes/Data/RevisionStorage.php

• RevisionStorage::findInternal() - need to validate or sanitize $attributes and $options
• RevisionStorage::insert() - validate $rev keys
• RevisionStorage::findMostRecent() - broken?? $keys is undefined, needs to be validated if it's not static
• PostRevisionStorage::insertRelated() - validate $tree keys
• HeaderRevisionStorage::insertRelated() - validate $header keys

(In reply to comment #0)

The Flow extension deployment to a handful of pages on mediawiki.org is
scheduled for Wednesday Dec 4.

(In reply to comment #2)

[Setting this to high priority during to the Wed Dec 4 schedule.]

Re-scheduled for Tuesday, December 10, 2013.

Ok, I've finished reviewing all of the codes, so this should be the last of the issues:

includes/View/PostActionMenu.php

• Document getAction()'s $content is unescaped

includes/View/History/HistoryRenderer.php

• Document that keys of getTimespans are passed raw into html output

templates/history-line.html.php

• please escape $class

templates/post.html.php

• document that the return of AbstractRevision::getModerationState() is written to raw html, or escape it here
• exploitable xss: escape usernames when used in attribute
• double quote all attributes (htmlspecialchars does not escape ')

includes/View/Post.php

• editPostButton(), hidePostButton, deletePostButton, etc, use escaped() instead of plain(), to comply with assumptions of PostActionMenu::getButton()

templates/topic.html.php

• double quote attributes
• document AbstractRevision::getModerationState() is written to raw html, or escape it here
• use escaped() messages in call to getButton()
• echo wfMessage( 'flow-topic-comments', $comments )->text(); should use escaped()

In general for the templates, it would be great if you all could standardize where the escaping for the items written into the template are escaped. There's a mix of escaping parameter before you render the template, in the template, and relying on functions called from within the templates to produce correct html for the context. That makes it really hard to catch errors.

(In reply to comment #9)

• Usernames containing a ' - Could you provide more details on where this

causes issues? I ran through the various pages and actions and don't see
anything yet, will be looking more thoroughly.

I found at least one place (in post.html.php) where you were putting the username into a '-quoted attribute. There were a couple places on http://ee-flow.wmflabs.org/wiki/User_talk:CSTest where I replied to a message by a user with a ' in the name, and the generated user link left off everything after the '. Looks like this is fixed now.

includes/Model/UUID

• removed debugging backtrace in https://gerrit.wikimedia.org/r/99285

includes/Repository/SelectQueryBuilder

• removed unused class in https://gerrit.wikimedia.org/r/99282

includes/Data/BoardHistoryStorage

• share query sanitization with BasicDbStorage https://gerrit.wikimedia.org/r/99035

includes/Data/RevisionStorage

• RevisionStorage::findInternal $attributes, RevisionStorage::insert $rev, RevisionStorage::insertRelated $tree and HeadeRevisionStorage::insertRelated -validate query conditions - https://gerrit.wikimedia.org/r/99035
• $options in RevisionStorage:findInternal was missed, patch is not yet in gerrit but is coming soon
• RevisionStorage::findMostRecent - was broken, $keys replaced with static value (from self::relatedPk method which returns static string in all implementations) - https://gerrit.wikimedia.org/r/99559

includes/View/PostActionMenu

• documented getAction()'s $content is unescaped - https://gerrit.wikimedia.org/r/99561

includes/View/History/HistoryRenderer

• docuemnted getTimespans return values as raw html - https://gerrit.wikimedia.org/r/99562

templates/history-line.html.php

• escaped $class - https://gerrit.wikimedia.org/r/99293

templates/post.html.php

• escaped $post->getModerationState() - https://gerrit.wikimedia.org/r/99564
• double quote all html attributes - https://gerrit.wikimedia.org/r/99573
• exploitable xss - https://gerrit.wikimedia.org/r/99287

includes/View/Post

• use escaped() in editPostButton, hidePostButton, deletePostButton, etc instead of plain() to comply with (now) documented assumptions of PostActionMenu::getButton - https://gerrit.wikimedia.org/r/99561

templates/topic.html.php

• double quote all attributes - https://gerrit.wikimedia.org/r/99573
• escape AbstractRevision::getModerationState() - https://gerrit.wikimedia.org/r/99564 and https://gerrit.wikimedia.org/r/99287
• escape calls to PostActionMenu::getButton - https://gerrit.wikimedia.org/r/99561
• escape flow-topic-comments message - https://gerrit.wikimedia.org/r/99585

Escape $options in RevisionStorage:findInternal - https://gerrit.wikimedia.org/r/100521

Handle username suppression and renames - For this one we took a different path, we removed all usernames from the flow database and intead query the wiki's user and ipblocks tables directly - https://gerrit.wikimedia.org/r/99789

What's the status of this bug report? Given that bug 56506 is marked resolved/fixed, I hope that this bug is largely resolved/fixed as well. :-)

As I understand it, done assuming all of the patches are reviewed, but I'd like to keep it open until I can poke the devs on Monday.