In T227209#5567722, @Arlolra wrote:

I might recommend setting up the SecurityCheckPlugin as a test script for Parsoid/PHP, as it's already using Phan.

Working on this in https://gerrit.wikimedia.org/r/c/mediawiki/services/parsoid/+/542604

1. I noticed a few issues with bin/parse.php. Since this is a test script, these may not really be issues:
   1. The --inputfile, --domain and --apiURL options do not do much in the way of validation. It's trivial to pass garbage data to them to cause PHP warnings and exceptions.

Additionally, I noticed that Phan's current config for Parsoid/PHP doesn't appear to leverage any of its bundled plugins. Some of these create a lot of noise (UnknownElementTypePlugin, NumericalComparison, WhitespacePlugin et al) but some of them are fairly useful from a security-adjacent standpoint. For example, there were a few things that caught my eye, which are probably fine, but might warrant some review:

src/Utils/TokenUtils.php:376 PhanPluginDollarDollar $$ Variables are not allowed.
src/Utils/TokenUtils.php:377 PhanPluginDollarDollar $$ Variables are not allowed.
src/Wt2Html/TT/ParserFunctions.php:116 PhanPluginAlwaysReturnMethod Method \Parsoid\Wt2Html\TT\ParserFunctions::noTrimRes has a return type of array, but may fail to return a value
tests/porting/hybrid/runDOMPass.php:411 PhanPluginDuplicateArrayKey Duplicate/Equivalent array key value('omit') detected in array - the earlier entry will be ignored.

These were found using some of the basic Phan plugins documented here. And here's a handy way to list and format some of the bundled plugins:

ls -p1 vendor/phan/phan/.phan/plugins/ |grep -Ev "(/|README)" |sed -e s/\.php// |xargs printf "\t\'%s\',\n"

Oh, and the above log is from the beta cluster deploy.

Some notes from today's deploy.

If that just happened once or few times on October 29th then i think that was me doing the tests

One thing to consider (maybe a separate bug needs filing) is that Parsoid/JS has been sending dsr info that has been used in browser by JS to show where errors occur in the markup. dsr info from Parsoid/PHP is using a different encoding so the offsets might not line up. We probably want to clear out old lints and then update the Linter extension to account for the difference.

This one is a little more interesting,

From an example query, https://logstash.wikimedia.org/app/kibana#/dashboard/AW4Y6bumP44edBvO7lRc?_g=h@2d95062&_a=h@67f6f50

Might be similar to T214649

Alas, we're not quite there for beta,

I will have a look at this from RESTBase perspective tomorrow.

From scandium, if I curl -H "Host: en.wikipedia.org" https://parsoid-php.discovery.wmnet/w/rest.php/en.wikipedia.org/v3/page/pagebundle/User%3ABSitzmann_%28WMF%29%2FMCS%2FTest%2FFrankenstein/860183819

Looking at the log, there are some oddities,

Hmm, this was implemented in https://github.com/wikimedia/parsoid/commit/96ae83f3806b85e42d235777cba3e21947c14406

3. I was able to get an exception in src/Utils/Util.php when testing with bin/parse.php --html2html with the following payload: <script\x20type=\"text/javascript\">javascript:alert(1);</script> (note the \x20 character): BadMethodCallException from line 61 of src/Utils/Util.php: Use lowercase tag names

2. While testing with bin/parse.php and the --html2html and --wt2html options I found an odd notice when sending data with the <poem> tag: PHP Notice: Uninitialized string offset: 0 in src/Ext/Poem/Poem.php on line 55

When trying to deploy to the beta cluster today, we hit,

The language conversion code is also still being reviewed, which is necessary to call this task done.

There's a proposed solution to this in,
https://gerrit.wikimedia.org/r/c/mediawiki/services/parsoid/+/545391/4/src/Config/Api/PageConfig.php

Unless the LC endpoints are done, how difficult is it to pull out the code that sets these headers into a separate patch so we can merge that and unblock Marko?

This is fixed in https://gerrit.wikimedia.org/r/c/mediawiki/services/parsoid/+/545391/3/src/PageBundle.php#77

The revised plan is to deploy this tomorrow around 10 am PT.

https://phabricator.wikimedia.org/P9393

This is not because of the indicator extension but because of the maplink extension that surrounds it. This was traced to the ParserAfterParse parser hook not being invoked for the Kartographer extension.

Probably depends on T227352

With the above patches merged, the mocha tests for this endpoint are passing. They can be run with the following,

wonder if we should handle T234057: Get rid of hybrid testing code

npm audit and snyk found some vulnerable Node dependencies for the dev dependency nsp (oddly enough, the old package which turned into audit) and service-runner's ms dependency. As I understand it, these packages are not relevant for Parsoid/PHP. Reporting here for the sake of completeness.

Presumably selser will not call getContent() in fallback paths ... but if so, that is one place where this edge case will pop up.

Well, we have to be able to create a page from scratch (with no previous revision). So a zero-length string seems reasonable to me as a fallback, in both the Api config and the integrated config.

I think I just fixed this in I62868e8dc9eabea4da68a556e692f5f135924a8a

I think we're just waiting for https://gerrit.wikimedia.org/r/c/mediawiki/libs/RemexHtml/+/535291 to get in as well.

Env, for one, is specifically not intended for use outside of Parsoid.

Should I rebase the security branch with latest changes to master?

On my laptop

Note function names are case insensitive for when you're grepping.

For development and debugging purposes, Parsoid also supports accessing an external MediaWiki installation via its action API. Parsoid/JS doesn't require that external wiki to have installed the ParsoidBatchAPI extension, but Parsoid/PHP depends on that currently (* TODO: Verify *).

The extension implementation is as follow,

After the upgrade, I confirmed the host was still running the currently deployed version,

I ran, SSH_AUTH_SOCK=/run/keyholder/proxy.sock ssh -l deploy-service wtp2011 'depool service=parsoid' from deployment.eqiad.wmnet and it's now showing {"wtp2011.codfw.wmnet": {"weight": 10, "pooled": "no"}, "tags": "dc=codfw,cluster=parsoid,service=parsoid"}

With that ^ the rt test script at least runs

A quick test with, node bin/roundtrip-test.js --domain ve --parsoidURL http://localhost/rest.php "Main Page", is returning,

I don't think the last fix deployment has fixed anything regarding that ticket. The Parsoid output is still broken so far I can see.

In the meantime, here's an edit that fixes it,
https://en.wikipedia.org/w/index.php?title=Maine&type=revision&diff=908732865&oldid=908276611&diffmode=source

The content seems to contain {{#tag:ref|Maine does not ... <references group="nb" /> |group="nb"}}, which results in references in references.

How do we pass in the rt testing flag to Parsoid?

The parsoid config is only used to get prefixes, which isn't really needed.

There should probably be better test coverage of this bin or at least fixing this,
https://github.com/wikimedia/parsoid/blob/master/bin/toolcheck.sh#L19-L21