Topic on Project:Support desk

Tracked in Phabricator
Task T76461

In MW 1.24 variable styles which is defined in includes/OutputPage.php has been changed from public to protected. Because of this there is now a problem with extension wysiwyg where onBeforePageDisplay hook is trying to use $out->styles variable.

Only workaround which I have been able to find is to change type of variable "styles" back to "public". Does this cause some kind of security issues with MW ?

So far I have been unsuccessful with my attempts to "extend" visibility of protected variable "styles" from OutputPage.php to wysiwyg. Could some of you php experts give me a hint how this could be solved properly?

You should file a bug against MW for this. That way whatever is needed to fix it will be more likely to stick around in the future.

That said, you'll probably need to create an accessor and use it in the CKEditor code.

If you file a bug for this, I'll work on helping you fix it.

Alguna novedad sobre el tema, algún tio que pueda enviar paso a paso toda la instalación para mitigar esto.

I have opened this in MW bug tracker T76461.

Talk page of extension

Issue in github repo of wysiwyg.

Function onBeforePageDisplay of wysiwyg extension is trying to:

1. read certain .css definitons from the page
2. extract contents of each .css file and place it as inline style definition on page
3. delete orginal -css definition from page

I was trying to debug this with MW1.24.

1. For issue 1. I used function buildCssLinksArray of OutputPage.php. With my limited skills of php programming I was able to extract names of style sheets but unable to get all the other data needed (f.ex type of css).
2. Issue 2. is not a problem because public function addInlineStyle is available in OutputPage.php.
3. For issue 3. I was unable to find a solution.

Is it still possible to have above procedure in MW 1.24?

On the other hand... comments of code block (function onBeforePageDisplay, lines ~194-214) in wysiwyg is referring to some limitation of IE (after max.31 style sheets IE hangs). I do not know if this has been some limitation of some older version of IE or perhaps caused by old version of IE + old CKeditor together. So is this kind of reformatting for style sheets of page necessary at all?

Related code:

    public static function onBeforePageDisplay( &$out, &$text ) {
        global $wgRequest, $wgScriptPath;

        //var_dump($out->styles);
        $action = $wgRequest->getText( 'action' );
        if (! in_array($action, array('edit', 'submit'))) return $out;
        $inlineStyles = array();

        foreach ( $out->styles as $key => $val ) {
            if (count($out->styles[$key]) > 0) {
                if (isset($out->styles[$key]['condition']) ||
                    isset($out->styles[$key]['dir']) ||
                    strpos($key, '?') !== false ||
                    strpos($key, 'jquery.fancybox') !== false) continue;
                $count = 1;
                $cssFile = dirname(__FILE__) . '/../../' . str_replace($wgScriptPath, '', $key, $count);
                $cssFile = str_replace('//', '/', $cssFile);
                if (isset($out->styles[$key]['media']) &&
                    file_exists($cssFile)) {
                    $cssCont = file_get_contents($cssFile);
                    if ($cssCont !== false) {
                        if (! isset($inlineStyles[$out->styles[$key]['media']]))
                            $inlineStyles[$out->styles[$key]['media']] = '';
                        $inlineStyles[$out->styles[$key]['media']] .= $cssCont."\n";
                        unset($out->styles[$key]);
                    }
                }
            }
        }
        foreach($inlineStyles as $media => $css ) {
            $out->addInlineStyle( $css );
        }
        return $out;
    }

I guess CKeditor should use ResourceLoader, which already concatenates all styles in a single request

I received a like error after installing the latest version of the WYSIWYG CK editor on MW 1.24. As suggested, changing the "$style" variable from "protected" to "public" solves the problem. So I would also be interested to know whether this presents a security issue.

I would like to know as well. Is this a security risk?

The change from protected/private to public in a variable is not a security risk. The visibility of variables is just a coding practice to restrict the usage of such variables from external classes, so if you need to make a breaking change on a variable that's private or protected, you can be certain that it won't break external classes because they can't be using it directly.

But you should think about, that the maintainer of the code doesn't changed the visibility without any thoughts about it :) Maybe the variable changes unexpectedly if something happens, or will be set to null somewhere or will be renamed in a future version of the code. A private/protected variable is, mostly, private or protected because it shouldn't be (needed) to use outside the class (private) or in classes, that inherits this class.

But, like Ciencia Al Poder wrote, not really a security risk :)

Thanks. :)