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).

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 Application Security 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.

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

@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.

@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?

@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.

@zeljkofilipin rebased. It's defined in jjb/job-templates.yaml#L169-L180 and invoked in jjb/mediawiki.yaml#L307-L310 I think?

@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.

(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!?!

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

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

In T162181#3426331, @Tgr wrote:

@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.