r111263 MediaWiki - Code Review archive

Some comments:

function efEtherpadLiteParser_Initialize( &$parser )

Doing the & in &$parser isn't needed anymore as far as I am aware. (OTOH, it doesn't hurt anything either)

The empty function is generally discouraged in the coding conventions.

$wgEtherpadLiteDefaultPadUrl    = "http://www.example.com/p/";

One alternative possibility is to do $wgEtherpadLiteDefaultPadUrl = null instead as default, and when you're about to display the pad, check for null and yell at user to configure the extension if the variable is null.

$wgEtherpadLiteShowControls     = 'true';
...

I'm not really a fan of using a string true. It would be better to use a boolean true value, and then convert that to a string when making the iframe url parameters.

 $padId            = ( !empty( $args['pad-id'] ) ) ? $args['pad-id'] : "" ;
 ...

There's no validation of user input here. I'm not sure how much evil can be accomplished via the url parameters. Worst case scenario would be of course if etherpad had an xss vulnrability in terms of its url parameters (Of course you have to allow most input here because pads can be named everything, but should at least check for & since those separate url parameters.

 ( !empty( $args['pad-url'] ) ) ? $args['pad-url'] : $wgEtherpadLiteDefaultPadUrl

this is kind of scary, an evil user can then embed an iframe to an arbitrary site. This could perhaps be used to do some kind of clickjacking attack against some other site, track users of a wiki, other evil stuff etc.

This is also an XSS attack by doing something like <eplite pad-url="javascript:alert(1)//"/>

+		"id"         => "epframe$padId",

Minor issue, but user could potentially make an id that is not a valid one. (HTML has some restrictions on ids if i recall). Also what if someone embeds the same pad twice (ids must be unique)?

$userName  = $wgUser->getName();

No garuntee the user who parsed the page is the one viewing the page. Either need to call $parser->disableCache(), add the username process at some later stage (like use js, or possibly some hook that runs right before outputting the page).

"style"      => "width:$width;height:$height",

CSS needs to be sanitized (There's a method somewhere in the Sanitizer for that i think). Certain browsers can be caused to execute js via certain css rules.

return array( $output, 'noparse' => true, 'isHTML' => true );

That's the return value format for parser functions not parser tags. Parser tags you just return the html as a string. (There is an extended return value system for parser tags where you can specify markerType for example, but the element names of the array are different from the parser function return value)

$wgExtensionCredits['other'][]

Generally this would probably be considered a "hook" extension not an "other" extension.

Cheers.

p.s. You should perhaps coordinate with NeilK. If i recall he was interested in etherpad <-> mediawiki stuff at one point.

Also two other things:

Might be nicer to use wfAppendQuery() instead of constructing the url with string concatenation, and for the boolean parameters (!empty( $args['show-controls'] ) )...) it should definitly validate that they are either true or false.

Thanks for your deep analysism, which I appreciate very much.

As you might have noticed, I already use Sanitizer::cleanUrl and Html:rawElement methods as mentioned in the Security Guides.

Regarding the pad-id, and the query string parameters - which are then forming part of the url - I now think of sanitizing the whole "src" parameter like in this example

"src" => Sanitzer::cleanUrl("$epliteHostUrl/$padId" . "?showControls=$showControls" . "&showChat=$showChat" . "&showLineNumbers=$showLineNumbers" . "&useMonospaceFont=$useMonospaceFont" . "&userName=$userName" . "&noColors=$noColors"),

If this works, will it fulfill the mw requirements for a sane code ?

The Html:rawElement was mentioned as a good way to sanitize here https://www.mediawiki.org/wiki/Security_checklist_for_developers#Output_.28API.2C_CSS.2C_JavaScript.2C_HTML.2C_XML.2C_etc..29 , so I thought, it is enough to use this method.

Pls. correct me, if I am wrong.

The Html:rawElement was mentioned as a good way to sanitize here [https://www.mediawiki.org/wiki/Security_checklist_for_developers#Output_.28API.2C_CSS.2C_JavaScript.2C_HTML.2C_XML.2C_etc..29 https://www.mediawiki.org/wiki/Security_checklist_for_developers#Output_.28API.2C_CSS.2C_JavaScript.2C_HTML.2C_XML.2C_etc..29] , so I thought, it is enough to use this method. 

That page was misleading (I changed it). Html::rawElement makes sure your attributes are encoded (So people don't do crap like width='foo" ><script>doEvil()...'. They don't look at the actual contents of the attribute. Hence, style attributes have to be further filtered with Sanitizer::checkCss (or Sanitizer::validateAttributes whic will do stuff for some other attributes in addition to style ) if the CSS comes from the user.

Sanitizer::cleanUrl doesn't do a whole lot for security in the context of your extension (It basically just normalizes entity references to stop people from getting around spam blacklists, which to be honest I'm not even sure spam blacklists would be checked for a parser hook attribute). The most scary part is allowing users to override the pad url - embedding iframe to a potentially evil site opens up users to at the very least phising attacks (Because it's embedded into the page, looks like part of the page, evil person could set up a website asking for users passwords, etc.

wfAppendUrl would probably help somewhat with security of building the url parameters, since it would prevent someone from passing a parameter to the other server other then the one's that you explicitly want them to (Of course in general, this extension is working on the assumption that the etherpad server is not evil, so the url paramter validation is not super-critical...). The best thing though would to check what all the valid values for those parameters could be, and make sure the user cannot pass in anything but those.

p.s. I just noticed there is a typo:

$wgEtherpadLiteDefaultHeigth

Should be

$wgEtherpadLiteDefaultHeight

Cheers.

Le me answer the uncritical question "what if someone embeds the same pad twice (ids must be unique)? ".

A: This is not a problem; the client then connects to both pads, i.e. you have two windows and can type in one of the two and see the output in both; it was my first test when I started embedding epl in mw.

The issue I'm concerned about is this results in invalid html, since the same id cannot appear twice in one page. So if someone embeds two pads in the same page, there will be two iframes in the page, both with the id eplite-iframe-, which will cause the html not to be valid. Since you're not using the id attribute for anything, it might make sense just not to put an id on the iframes. (From a pragmatic perspective, the invalid html won't hurt anything, it will just cause the validator to whine)

"...since the same id cannot appear twice in one page."

Of course, you are 100% right, I overlooked this (never ignored it, but today.) Will use, as suggested by you, "class=" instead.

"Since you're not using the id attribute for anything":

This is just for the case, that another extension will have the need to manipulate that specific frame. I took it over from the example "Etherpad lite and jQuery" https://github.com/johnyma22/etherpad-lite-jquery-plugin/blob/master/js/etherpad.js where the content also can be read. I did not implement this part in the current version.

I filed this as issue on https://github.com/johnyma22/etherpad-lite-jquery-plugin/issues/8 so that the epl people are informed to also replace id= by class= in their etherpad.js example.

+	'url' => '//www.mediawiki.org/wiki/Extension:EtherpadLite',

Should be a direct HTTPS link based on the standards we seem to be using now.

// ==> https:// Are you really sure, I thought we have to use "//" ?

Yes, Brion went thought and changed them awhile ago, Because otherwise it's basically pointless (not many people seem to run their wikis on Https)

Re: "Doing the & in &$parser isn't needed anymore as far as I am aware. (OTOH, it doesn't hurt anything either) "

I verbatim copied that "&" from https://www.mediawiki.org/wiki/Manual:Tag_extensions - wanted to fix this now but found luckily: you did already, thanks.

@Bawolff: in r111313 I fixed (I think) all of the issues.

Let me explain here: 1. I changed the attribute names from pad-id => id and pad-url => src. This allows to treat (id, src, height, width) as standard attributes and to use

$sanitizedAttributes = Sanitizer::validateAttributes( $args, array ( "width", "height", "id", "src" ) );

for this, and then to use $santizedAttributes['src'] for example.

The boolean parameters are manually checked.

Please have a look to r111313 . Documentation https://www.mediawiki.org/wiki/Extension:EtherpadLite has been updated as well.

You're getting closer, but there's still some issues. (From r111313)

+	
+	$sanitizedAttributes = Sanitizer::validateAttributes( $args, array ( "width", "height", "id", "src" ) );

You really shouldn't use validateAttributes for the individual width and height elements, unless you put them on an iframe as the old-style indvidual width and height attributes. (This is because the html width attribute is interpreted differently then the style attribute by web browsers, so different things are evil (or really, its only possible to do evil things with the style attribute...)). If you're going to use validateAttributes, run it on the fully constructed array of attributes ( $iframeAttributes ) so the Sanitizer see's exactly what attributes you're going to pass to Html::rawElement.

Some other minor issues:

$noColors         = ! ( wfEtherpadLiteStringFromTestedBoolean( $args['show-colors'], $wgEtherpadLiteShowAuthorColors ) );

The ! mark operator working on a string result of wfEtherpadLiteStringFromTestedBoolean might do different things then what you expect. (Since (!"false") === false

$args['id']       = ( isset( $args['id'] ) ) ? $args['id'] : "";

Any particular reason why you set an id attribute at all (instead of say a class attribute that could be non-unique and the same for all the iframes). Getting users to ensure the id is always unique is probably not ideal

I'm still very nervous about allowing just any website as a source. The potential bad is as following (Even with nowiki, CR messes with my urls... pretend that is not using wiki external url syntax):

<eplite src="[http://example.com/website-that-tells-user-they-have-been-logged-out-and-asks-for-password http://example.com/website-that-tells-user-they-have-been-logged-out-and-asks-for-password]" width="200px;border:none;overflow:hidden"/>

It's a lot less convincing if width is validated fully (You might want to consider also stripping semi-colons from width in addition to Sanitizer related stuff), but still, that could easily trick gullible users to revealing there password to some third party. If you really want to support multiple etherpad sites, perhaps the best way would be to have another config variable that has a list of alternative allowed etherpad websites in addition to the default.

Last of all, some of the checks for various args (introduced in r111313) need to use isset to prevent Notice: Undefined index: show-colors in /var/www/w/extensions/EtherpadLite/EtherpadLite.php on line 116.

Cheers.

"I'm still very nervous about allowing just any website as a source."

I was and I am also. Do you have a proposal for me ?

the "id" replaced my original "pad-id" which is the unique identifier on a pad-server (src=pad-server url). "id" is not a class.

I mean the id for the iframe. At the moment, the iframe id is set to the same thing as the pad id (earlier i didn't realize that that value was also used for what pad to get, which is why my comment is a little confusing. I thought it was solely for setting the id attribtue on the iframe). id's on html elements have to be unique (in a page), so it might be best just not to put an id attribute on the iframe. (I mentioned the class attribute, because typically classes are used for the same purpose as the id attribute, but don't have the uniqueness requirement).

"Last of all, some of the checks for various args (introduced in r111313) need to use isset to prevent Notice: Undefined index: show-colors "

Arrrghh. mea culpa. I put the isset() into the function where I passed the "unset value" to - which was so stupid (shame on me).

That is a good extension idea Wikinaut. Well done 8-)

Hashar, perhaps you can contact me to help me to solve the remaining issues, which I fully understand. Unfortunately, I lost the overview, what MW methods can be used best for sanitizing.

Any help is welcome, either here or by mail (you have my e-mail address)