Page MenuHomePhabricator

Should we add psy/psysh to wmf vendor repo for use on WMF servers?
Closed, ResolvedPublic

Description

So in https://github.com/wikimedia/mediawiki/commit/7671b2b71e28fa013917f65a1d49828ebb6a9725 @hashar and @Tgr added shell.php as an improvement on eval.php

Would people find it useful to have this available on WMF servers over and beyond the usual eval.php?

It adds quite a dependancy tree, as can be seen in https://gerrit.wikimedia.org/r/#/c/339584

Event Timeline

Reedy created this task.Apr 4 2017, 6:45 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 4 2017, 6:45 PM
Reedy renamed this task from Should we add psy/psysh to wmf vendor repo for use on WMF servers to Should we add psy/psysh to wmf vendor repo for use on WMF servers?.Apr 4 2017, 6:48 PM
Tgr added a subscriber: Tgr.Apr 4 2017, 7:25 PM

It would be great to have it on the server, yes. psysh has all kinds of cool stuff (tab autocompletion, nice dumping, inspection, phpdoc extraction, recovering from fatals...). Not sure about the security impact, but it's only used from shell so intentionally malicious code is the only threat model I can think of, and all of the dependencies are reputable (Symfony, nikic, Jakub Onderka) or trivial (dnoegel/php-xdg-base-dir).

hashar updated the task description. (Show Details)Apr 4 2017, 7:55 PM

Change 339584 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/vendor@master] Add psy/psysh 0.8.5

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

This still needs a Security Readiness Reviews before it can go forward.

My apologies for the delay on this. In my review I focused on verifying safe interaction with the environment, safe shell invocation, resource consumption, php-parser not accidentally invoking code around the AST construction process, and appropriate remediation of any previously reported security vulnerabilities. I did not find any issues nor did I uncover any malcode (however, there is a huge amount of code here).

I'm +1'ing the patchset because I also agree with @Tgr in T162181#3154887 that the dependences are either reputable (well-regarded author) or trivial (easily reviewed). However, I'm a bit a freaked out at the amount of code that will need to be kept up-to-date, and the review of that code that should happen. This problem, however, is not unique to this particular subproject.

Reedy added a comment.Jul 11 2017, 5:26 PM

The patch is against 0.8.5. 0.8.9 is now out.

https://github.com/bobthecow/psysh/compare/v0.8.5...v0.8.9

We should probably update to that (and rebase) before looking to merge.

And see what other new dependancies should be updated at the same time

Tgr added a comment.Jul 11 2017, 6:08 PM

@dpatrick thanks for the detailed review! It is somewhat scary, yeah, but this is a command-line tool used by already trusted users so the attack surface will be very limited.

Out of curiosity, do you notify the maintainers of external projects in such cases? For smallish projects it might be helpful information to know that someone who does security for a job did a review.

Tgr added a comment.Jul 11 2017, 6:09 PM

@hashar any advice on the CI issue (see last few comments on the patch)?

@zeljkofilipin can you maybe advise? https://gerrit.wikimedia.org/r/#/c/339584/ fails because one of the unit tests contains something that's only valid in PHP7. The patch changes the composer test command to exclude test directories, but that has no effect, presumably because linting happens directly and not through composer test. What would be the best way to fix this?

zeljkofilipin added a comment.EditedAug 14 2017, 3:36 PM

@Tgr I have left a few comments at the patch in Gerrit. I can not even find where mwgate-php55lint is defined. :|
Anyway, looks like the patch should be rebased before anything can be done. It's hard to debug since I can not run mwgate-php55lint for the patch.

@Tgr that looks correct. And phplint builder is defined in jjb/macro.yaml#L455-L462

Looks like it's running php -l on files changed in HEAD and it returns an error for symfony/var-dumper/Tests/Fixtures/FooInterface.php. I am not familiar with how PHP linting and composer are done in CI.

Tgr added a comment.Oct 13 2017, 12:12 AM

(for posterity's sake, the permanent versions of the links above: jjb/job-templates.yaml#L169-L180, jjb/mediawiki.yaml#L307-L310, jjb/macro.yaml#L455-L462)

@hashar can you help with how we can opt out mediawiki/vendor from the phplint portion of mwgate? The 5.5 linter breaks on some test files and '{name}-composer-{phpflavor}-{image}' runs composer test which runs phplint (but in a saner way where directory exclusions can be controlled from the target repo).

I thought we already fixed that by switching to a composer job that runs parallel-lint!?!

Tgr added a comment.Oct 13 2017, 7:18 AM

No, mwgate still runs plain phplint as you can see from the links - or the jenkins output:

23:46:30 Building remotely on integration-slave-jessie-1003 (DebianGlue contintLabsSlave DebianJessie) in workspace /srv/jenkins-workspace/workspace/mwgate-php55lint
...
23:46:34 + xargs -n1 -t php -l
...
23:47:01 php -l symfony/var-dumper/Tests/Fixtures/GeneratorDemo.php 
23:47:01 PHP Parse error:  syntax error, unexpected 'bar' (T_STRING) in symfony/var-dumper/Tests/Fixtures/GeneratorDemo.php on line 14

That is related to T135161 T136010

We had jakub-onderka/php-parallel-lint added to vendor via https://gerrit.wikimedia.org/r/340238

https://gerrit.wikimedia.org/r/340241 added composer as an experimental job but never got promoted to replace the php lint job.

Change 384002 had a related patch set uploaded (by Hashar; owner: Hashar):
[integration/config@master] mediawiki/vendor switch to composer test

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

mwgate-composer-hhvm-jessieSUCCESS in 3m 41s
mwgate-composer-php55-jessieSUCCESS in 23s

No idea why HHVM takes so long though

Change 384002 merged by jenkins-bot:
[integration/config@master] mediawiki/vendor switch to composer test

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

CI works now

Change 339584 merged by jenkins-bot:
[mediawiki/vendor@master] Add psy/psysh 0.8.11

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

Tgr closed this task as Resolved.Oct 13 2017, 8:24 PM
Tgr claimed this task.

Thanks @hashar!

@dpatrick thanks for the detailed review! It is somewhat scary, yeah, but this is a command-line tool used by already trusted users so the attack surface will be very limited.

Out of curiosity, do you notify the maintainers of external projects in such cases? For smallish projects it might be helpful information to know that someone who does security for a job did a review.

Generally no, @Tgr unless we find a vulnerability that has not already been reported.