Cross-domain policy regexp is too narrow
Closed, ResolvedPublic

Description

Our regexp in OutputHandler, ApiFormatJson and ApiFormatPhp ('/\<\s*cross-domain-policy\s*\>/i') counts on this tag to never have any attributes. This is not the case, e.g. https://twitter.com/crossdomain.xml has <cross-domain-policy xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="http://www.adobe.com/xml/schemas/PolicyFile.xsd">. In any case, we must not assume that the spec will never change or that there will never be non-compliant plugin implementations.

MaxSem created this task.Jan 14 2016, 7:11 PM
MaxSem added projects: Security-Core, Security.
MaxSem changed the visibility from "Public (No Login Required)" to "Custom Policy".
MaxSem changed the edit policy from "All Users" to "Custom Policy".
MaxSem changed Security from None to Software security bug.
MaxSem added a subscriber: MaxSem.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 14 2016, 7:11 PM

Alternatively, just nuke teh whole thing? Unless someone has an insane rewrite that allows to actually output this XML at /crossdomain.xml, this technique shouldn't be needed anymore.

I believe it's still needed, since any site can helpfully point to a custom url for crossdomain.xml on a site. I believe /crossdomain.xml will override the custom policy, but I haven't yet deployed a /crossdomain.xml yet ({T75574}).

Anomie added a subscriber: Anomie.

I think we should be able to get away with just changing the regex, along the lines of '/\<\s*cross-domain-policy(?=\s|>)/i', and the replacements similarly. This would be a slight change in that it would turn <cross-domain-policy> into \u003Ccross-domain-policy> instead of \u003Ccross-domain-policy\u003E for ApiFormatJson, but I don't think that will matter for the purpose.

csteipp triaged this task as "High" priority.Jan 19 2016, 10:26 PM

This patch looks fine to me, although I wonder if it would be simpler to do /\<\s*cross-domain-policy/i instead

Well, that would catch things like <cross-domain-policy-foobar that aren't the problematic tag.

Updated for short array format in the tests.

dpatrick closed this task as "Resolved".Mar 29 2016, 10:53 PM
dpatrick claimed this task.
dpatrick added a subscriber: dpatrick.

Patch deployed (https://wikitech.wikimedia.org/wiki/Server_Admin_Log):

22:47 dapatrick: Deployed patch for T123653 to wmf18 and wmf19

demon changed the visibility from "Custom Policy" to "Public (No Login Required)".May 20 2016, 5:24 PM
demon changed the edit policy from "Custom Policy" to "All Users".
demon changed Security from Software security bug to None.
Restricted Application added a subscriber: Malyacko. · View Herald TranscriptMay 20 2016, 5:24 PM