Page MenuHomePhabricator

Enable Phan on Chameleon Skin
Closed, ResolvedPublic

Event Timeline

Reedy created this task.Thu, May 21, 4:02 PM

When I try to run phan locally, I get:

AssertionError: record_variable_context_and_scope must be enabled in /var/www/mediawiki/vendor/mediawiki/phan-taint-check-plugin/src/TaintednessBaseVisitor.php:98
Stack trace:
#0 /var/www/mediawiki/vendor/mediawiki/phan-taint-check-plugin/src/TaintednessBaseVisitor.php(298): assert(false, 'record_variable...')

Is that known behavior?

Reedy added a project: phan-taint-check-plugin.

When I try to run phan locally, I get:

AssertionError: record_variable_context_and_scope must be enabled in /var/www/mediawiki/vendor/mediawiki/phan-taint-check-plugin/src/TaintednessBaseVisitor.php:98
Stack trace:
#0 /var/www/mediawiki/vendor/mediawiki/phan-taint-check-plugin/src/TaintednessBaseVisitor.php(298): assert(false, 'record_variable...')

Is that known behavior?

What config are you using?

Looking at https://github.com/wikimedia/phan-taint-check-plugin/blob/7137343/src/SecurityCheckPlugin.php#L143 this is set in/at https://github.com/wikimedia/mediawiki-tools-phan/blob/edf21edb0efdff1a2938842516fd0740df9fd240/src/config.php#L371

So it looks like the .phan/config.php you've added (if you've added one) doesn't have a require __DIR__ . '/../vendor/mediawiki/mediawiki-phan-config/src/config.php'; line to bring that in.

Bare minimum .phan/config.php probably wants to be:

return require __DIR__ . '/../vendor/mediawiki/mediawiki-phan-config/src/config.php';

Bare minimum .phan/config.php probably wants to be:

return require __DIR__ . '/../vendor/mediawiki/mediawiki-phan-config/src/config.php';

(I confirm this)

I do indeed have the correct contents in .phan/config and have confirmed that https://github.com/wikimedia/mediawiki-tools-phan/blob/edf21edb0efdff1a2938842516fd0740df9fd240/src/config.php#L371 is executed. The assertion that is failing is

$varContext = $variableObj->getFileRef();
assert( $varContext instanceof Context, 'record_variable_context_and_scope must be enabled' );

When I print out the class name for $varContext, the assertion passes several times when $varContext is of class Phan\Language\Context, but it fails when it is Phan\Language\FileRef.

Hmmmm this is bad. Sounds like phan is violating the contract of getFileRef in the case with record_... enabled.

I need to check where the bug is exactly, and perhaps open an issue upstream.

Reedy added a comment.Sun, May 24, 2:35 AM

Is it worth Cindy trying an older version of the phan plugin etc?

Is it worth Cindy trying an older version of the phan plugin etc?

It's possible. I think this might be due to an older version of *phan* (not taint-check) used with a new version of taint-check, because I get no error locally on 3.0.2.

@CCicalese_WMF Just to confirm, taint-check was revamped lately, and some guides may be outdated. The "new" way of running it is to just:

  • composer require --dev "mediawiki/mediawiki-phan-config: 0.10.2"
  • vendor/bin/phan -d . --long-progress-bar

Which is different from the "old" style where you had to install taint-check separately, and it was up to you to install a compatible version.

@Daimona, thank you! I was following the instructions at https://www.mediawiki.org/wiki/Continuous_integration/Tutorials/Add_phan_to_a_MediaWiki_extension. When I switched to your instructions, it worked!

The difference is the version of phan and/or mediawiki-phan-config. I was previously using version 0.5.0 of mediawiki-phan-config, as stated in the instructions, instead of 0.10.2. I was also cloning https://github.com/phan/phan and checking out 1.2.4, as stated in the instructions, but composer installed version 2.6.1 of phan. I've updated the instructions to indicate the updated versions, but the Setup section should be edited/removed, since separate installation of phan does not appear to be necessary. The instructions for running phan should be updated to point to the version in vendor/bin. Do you agree?

Reedy added a comment.Sun, May 24, 4:45 PM

The instructions for running phan should be updated to point to the version in vendor/bin. Do you agree?

I'm partially wondering why we haven't put a "composer phan" type command into composer.json rather than people having to reference vendor/bin manually

Job for libraryupgrader etc?

CCicalese_WMF added a comment.EditedSun, May 24, 5:01 PM

The

composer require --dev "mediawiki/mediawiki-phan-config: 0.10.2"

installs phan as a dependency, so a separate require for phan is not necessary.

But, since the mediawiki/mediawiki-phan-config requirement exists in mediawiki/composer.json, phan and mediawiki-phan-config will already be in the MediaWiki vendor/bin and should not be necessary in the extension/skin, correct? It seems that the only requirement in the extension/skin directory is the one line .phan/config.php file that allows phan to be run from the subdirectory (which would be awesome to eliminate as well).

@Daimona, thank you! I was following the instructions at https://www.mediawiki.org/wiki/Continuous_integration/Tutorials/Add_phan_to_a_MediaWiki_extension. When I switched to your instructions, it worked!

Glad to hear that it worked :-)

The difference is the version of phan and/or mediawiki-phan-config. I was previously using version 0.5.0 of mediawiki-phan-config, as stated in the instructions, instead of 0.10.2. I was also cloning https://github.com/phan/phan and checking out 1.2.4, as stated in the instructions, but composer installed version 2.6.1 of phan.

Hm, so instructions were even more outdated than I originally thought.

I've updated the instructions to indicate the updated versions, but the Setup section should be edited/removed, since separate installation of phan does not appear to be necessary. The instructions for running phan should be updated to point to the version in vendor/bin. Do you agree?

I do, thanks for pointing this out! I'm going to take a look at that guide now.

I'm partially wondering why we haven't put a "composer phan" type command into composer.json rather than people having to reference vendor/bin manually
Job for libraryupgrader etc?

Yea, good idea.

The

composer require --dev "mediawiki/mediawiki-phan-config: 0.10.2"

installs phan as a dependency, so a separate require for phan is not necessary.

Correct, you only need to require mediawiki-phan-config.

But, since the mediawiki/mediawiki-phan-config requirement exists in mediawiki/composer.json, phan and mediawiki-phan-config will already be in the MediaWiki vendor/bin and should not be necessary in the extension/skin, correct?

While that's true, you usually want to add the dependency to each extension all the same, so you can use a specific version of phan. I think this is what we always do for dev dependencies / checkers.

It seems that the only requirement in the extension/skin directory is the one line .phan/config.php file that allows phan to be run from the subdirectory (which would be awesome to eliminate as well).

That can be eliminated only if you pass a config file to phan using the -k option. However, you usually want to keep that file in place so you can further customize it.

But, since the mediawiki/mediawiki-phan-config requirement exists in mediawiki/composer.json, phan and mediawiki-phan-config will already be in the MediaWiki vendor/bin and should not be necessary in the extension/skin, correct?

While that's true, you usually want to add the dependency to each extension all the same, so you can use a specific version of phan. I think this is what we always do for dev dependencies / checkers.

Ah, yes, good point.

It seems that the only requirement in the extension/skin directory is the one line .phan/config.php file that allows phan to be run from the subdirectory (which would be awesome to eliminate as well).

That can be eliminated only if you pass a config file to phan using the -k option. However, you usually want to keep that file in place so you can further customize it.

Makes sense.

I'm partially wondering why we haven't put a "composer phan" type command into composer.json rather than people having to reference vendor/bin manually

Sorry, I read that quickly and missed your point. Yes, that would be great!

Pull request that adds phan and fixes all but one error: https://github.com/ProfessionalWiki/chameleon/pull/156. The remaining error is:

src/Components/NavMenu.php:62 SecurityCheck-DoubleEscaped Calling method \Skins\Chameleon\Components\NavMenu::getDropdownForNavMenu() in \Skins\Chameleon\Components\NavMenu::getHtml that outputs using tainted argument $menuDescription. (Caused by: src/Components/NavMenu.php +89) (Caused by: src/Components/NavMenu.php +61)

Suggestions welcome!

Pull request that adds phan and fixes all but one error: https://github.com/ProfessionalWiki/chameleon/pull/156.

I skimmed through the PR and it looks good!

The remaining error is:

src/Components/NavMenu.php:62 SecurityCheck-DoubleEscaped Calling method \Skins\Chameleon\Components\NavMenu::getDropdownForNavMenu() in \Skins\Chameleon\Components\NavMenu::getHtml that outputs using tainted argument $menuDescription. (Caused by: src/Components/NavMenu.php +89) (Caused by: src/Components/NavMenu.php +61)

Suggestions welcome!

I've checked the issue here, it complains about $menuDescription being already HTML-escaped when passed to getDropdownForNavMenu(). Keep in mind that taint-check only tracks taintedness for the whole array, not for each array element on its own. Here we have to analyze two parts:

  • The call to getDropdownForNavMenu(). At line 89, it calls buildDropdownMenuStub() which, in turn, returns a string including htmlspecialchars( $menuDescription[ 'header' ] ), so it is indeed escaping $menuDescription;
  • Whether $menuDescription was already escaped before the call. $menuDescription is one of the elements of the array returned by ChameleonTemplate::getSidebar, which in turn uses the parent's implementation at BaseTemplate::getSidebar. The problem is, ::getSidebar() is very complicated. It has a lot of conditional logic, it calls several other methods, and both getSidear() and its callees make extensive use of $this->data which, AFAICT, is an array property holding all kinds of data, hence hard to track. AFAICS, everything returned by getSidebar() is unsafe, hence NOT HTML-escaped, but I have surely missed several spots.

Long story short, I suggest suppressing the issue with @phan-suppress-next-line.

Thank you so much for the analysis and advice, @Daimona!

CCicalese_WMF closed this task as Resolved.Mon, May 25, 10:54 PM
CCicalese_WMF claimed this task.
CCicalese_WMF triaged this task as Medium priority.

I just pushed the commits to gerrit, and then rechecked my patch...

https://integration.wikimedia.org/ci/job/mwskin-php72-phan-docker/5477/console

00:01:02 <checkstyle version="6.5">
00:01:02   <file name="src/Chameleon.php">
00:01:02     <error line="75" severity="error" message="Call to method getInstance from undeclared class \Bootstrap\BootstrapManager" source="PhanUndeclaredClassMethod"/>
00:01:02   </file>
00:01:02   <file name="src/Hooks/SetupAfterCache.php">
00:01:02     <error line="57" severity="warning" message="Parameter $bootstrapManager has undeclared type \Bootstrap\BootstrapManager" source="PhanUndeclaredTypeParameter"/>
00:01:02     <error line="108" severity="error" message="Call to method addAllBootstrapModules from undeclared class \Bootstrap\BootstrapManager" source="PhanUndeclaredClassMethod"/>
00:01:02     <error line="111" severity="error" message="Call to method addStyleFile from undeclared class \Bootstrap\BootstrapManager" source="PhanUndeclaredClassMethod"/>
00:01:02     <error line="116" severity="error" message="Call to method addStyleFile from undeclared class \Bootstrap\BootstrapManager" source="PhanUndeclaredClassMethod"/>
00:01:02     <error line="121" severity="error" message="Call to method addStyleFile from undeclared class \Bootstrap\BootstrapManager" source="PhanUndeclaredClassMethod"/>
00:01:02     <error line="126" severity="error" message="Call to method addStyleFile from undeclared class \Bootstrap\BootstrapManager" source="PhanUndeclaredClassMethod"/>
00:01:02     <error line="131" severity="error" message="Call to method addStyleFile from undeclared class \Bootstrap\BootstrapManager" source="PhanUndeclaredClassMethod"/>
00:01:02     <error line="136" severity="error" message="Call to method addStyleFile from undeclared class \Bootstrap\BootstrapManager" source="PhanUndeclaredClassMethod"/>
00:01:02     <error line="141" severity="error" message="Call to method addStyleFile from undeclared class \Bootstrap\BootstrapManager" source="PhanUndeclaredClassMethod"/>
00:01:02     <error line="146" severity="error" message="Call to method setScssVariable from undeclared class \Bootstrap\BootstrapManager" source="PhanUndeclaredClassMethod"/>
00:01:02     <error line="159" severity="error" message="Call to method addStyleFile from undeclared class \Bootstrap\BootstrapManager" source="PhanUndeclaredClassMethod"/>
00:01:02     <error line="168" severity="error" message="Call to method setScssVariable from undeclared class \Bootstrap\BootstrapManager" source="PhanUndeclaredClassMethod"/>
00:01:02   </file>
00:01:02 </checkstyle>

I guesss I need to add a phan dependancy. Patch incoming

Change 598547 had a related patch set uploaded (by Reedy; owner: Reedy):
[integration/config@master] Add Bootstrap as Chameleon phan dependancy

https://gerrit.wikimedia.org/r/598547

Change 598547 merged by jenkins-bot:
[integration/config@master] Add Bootstrap as Chameleon phan dependancy

https://gerrit.wikimedia.org/r/598547

@CCicalese_WMF I think you missed an extra '..'

00:09:09 WARNING: Caught exception while listing files in '/mediawiki/skins/chameleon/.phan/../..//extensions/Bootstrap': RecursiveDirectoryIterator::__construct(/mediawiki/skins/chameleon/.phan/../..//extensions/Bootstrap): failed to open dir: No such file or directory

I think

$IP = getenv( 'MW_INSTALL_PATH' ) !== false
	? str_replace( '\\', '/', getenv( 'MW_INSTALL_PATH' ) )
	: __DIR__ . '/../../';

Needs to be

$IP = getenv( 'MW_INSTALL_PATH' ) !== false
	? str_replace( '\\', '/', getenv( 'MW_INSTALL_PATH' ) )
	: __DIR__ . '/../../..';

Fixes the extra leading / too

Looks like IP doesn't usually have a trailing /

reedy@deploy1001:~$ mwscript eval.php enwiki
> echo $IP;
/srv/mediawiki-staging/php-1.35.0-wmf.31

Ah, thanks for catching that! (My tests were with MW_INSTALL_PATH set, since my docker environment has skins/extensions in an alternative location.) Do you have a pull request for that, or shall I create one?

I haven't made a PR, but I can. Not fussed either way :)

If you create it, I'll merge it :-)