Page MenuHomePhabricator

Escaping error in old URL format
Closed, ResolvedPublic


I was checking if T65696: Flow: display, behavior, and API failures when displaying a post revision still applies, and it partly does. I'm extracting this part out because there is leakage of an HTML attribute that could lead to an XSS.


The actual post is just 'C'. It's also a post, not a topic title. See

Event Timeline

Maniphest changed the visibility from "Public (No Login Required)" to "Custom Policy".Jun 19 2015, 4:50 AM
Maniphest changed the edit policy from "All Users" to "Custom Policy".
Mattflaschen-WMF triaged this task as Unbreak Now! priority.
Mattflaschen-WMF updated the task description. (Show Details)
Mattflaschen-WMF changed Security from None to Software security bug.
Mattflaschen-WMF edited subscribers, added: EBernhardson, Quiddity, Spage and 2 others; removed: Aklapper.

Yep, looks like <p data-parsoid='{"dsr":[0,1,0,0]}'>C</p> is being dropped unescaped into data-flow-topic-title. Let's get that fixed asap.

There is a Handlebars helper in Flow called escapeContent, which is dangerously misnamed. It doesn't escape anything; instead, it potentially armors its input string against the escaping that Handlebars performs by default. The implementation is basically return format === 'html' ? SafeString(content) : content; .

This would be appropriate for outputting content that could be either HTML or wikitext; in production it doesn't seem like we will ever call this with a non-HTML format parameter, but I think we might call it with format='wikitext' in standalone installs (I haven't checked). However, this is never appropriate to call inside of an attribute value, which is what was happening here. I audited the repo and found no other cases of escapeContent being called in an attribute value.

We should also rename escapeContent to something less misleading and more appropriately scary-sounding.

Patch that fixes the reported issue. Keeping it secret for now while I figure out if XSS is actually possible.

Patch looks good

@mmodell, can you deploy this and confirm here when it's on the cluster?

@mmodell, actually, Roan is going to upload one for wmf10, so the "binary" part works.

wmf10 patch

@mmodell, we deployed it for wfm10. We'll need to figure out releasing this and getting it into wmf11.

@demon, should we add this Flow patch to the next security release? Or should we just make it public since Flow is technically beta on

@demon, should we add this Flow patch to the next security release? Or should we just make it public since Flow is technically beta on

It's not bundled and it's beta software, I'd say no.

Catrope changed Security from Software security bug to None.Jun 29 2015, 8:29 PM
Catrope changed the visibility from "Custom Policy" to "Public (No Login Required)".Jun 29 2015, 9:34 PM
Catrope changed the edit policy from "Custom Policy" to "All Users".