Just wondering, is there a reason for the doing the new SiteMapper stuff, instead of using core's WikiMap (and related) class? (I haven't looked at WikiMap in a while, so the answer might just be that WikiMap really sucks)

In T214152#4892886, @Bawolff wrote:

Just wondering, is there a reason for the doing the new SiteMapper stuff, instead of using core's WikiMap (and related) class? (I haven't looked at WikiMap in a while, so the answer might just be that WikiMap really sucks)

WikiMap is mainly for dealing with wikis in a Wiki farm like setup while the SiteMapper has no such assumption. This SiteMapper code is originally taken from ContentTranslation. It helps a lot when you want to treat your local wiki as one of the sister wiki of wikipedia , fetch content from one of those language wiki, use APIs etc, by just having a domain URL template. Since we need a counter part of this in javascript, these templates helps to easity construct api URLs.

To help understanding what the project does,access our labs instance where this extension is deployed using google translate. You should see broken edit links, and page actions removed by a contribution invite banner. Which will take you to one of our wikis, explaining steps to contribute. (Currently that target wiki is same labs wiki due to constraints in testing)

@santhosh -

This is still technically unassigned, but I had a cursory look at it. Some initial findings:

1. package.lock looks fine from a quick run of security-checker.
2. I assume any potential privacy (and related) issues w/ the Google MT service are already addressed by WMF's agreement with Google for cxserver.
3. In SiteMapper::getPageURL, it looks like $title isn't being validated/sanitized and is just replacing $2 within //$1.wikipedia.org/wiki/$2. Given that Html::rawElement is used within specials/SpecialExternalGuidance.php and it's populating $sourcePage from the request via getVal(), I'm thinking there could be a potential for injection here, unless I'm just missing where this is being sent to the parser or sanitized, etc.
4. I'm not seeing anything egregious within the JS modules under modules/, but I (or whomever officially gets assigned this review) should definitely spend more time reviewing those.

In T214152#4925599, @sbassett wrote:

@santhosh -

This is still technically unassigned, but I had a cursory look at it. Some initial findings:

1. package.lock looks fine from a quick run of security-checker.
2. I assume any potential privacy (and related) issues w/ the Google MT service are already addressed by WMF's agreement with Google for cxserver.
3. In SiteMapper::getPageURL, it looks like $title isn't being validated/sanitized and is just replacing $2 within //$1.wikipedia.org/wiki/$2. Given that Html::rawElement is used within specials/SpecialExternalGuidance.php and it's populating $sourcePage from the request via getVal(), I'm thinking there could be a potential for injection here, unless I'm just missing where this is being sent to the parser or sanitized, etc.
4. I'm not seeing anything egregious within the JS modules under modules/, but I (or whomever officially gets assigned this review) should definitely spend more time reviewing those.

Following up on number 3... I added some use statements in https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/ExternalGuidance/+/488111/ which is causing Phan to complain about the $out->addHTML( Html::rawElement( blocks as Scott has highlighted above

Obviously the patch doesn't add them, but it does expose them as it gives Phan some more context as to the types (as they didn't exist in the namespace, they're in the global namespace)

Change 488314 had a related patch set uploaded (by Santhosh; owner: Santhosh):
[mediawiki/extensions/ExternalGuidance@master] SpecialExternalGuidance: Validate the language codes and title parameters

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

In T214152#4925599, @sbassett wrote:

1. In SiteMapper::getPageURL, it looks like $title isn't being validated/sanitized and is just replacing $2 within //$1.wikipedia.org/wiki/$2. Given that Html::rawElement is used within specials/SpecialExternalGuidance.php and it's populating $sourcePage from the request via getVal(), I'm thinking there could be a potential for injection here, unless I'm just missing where this is being sent to the parser or sanitized, etc.

In https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/ExternalGuidance/+/488314 I added validation for language codes and source page param before all these code execution. That should take care of this issue.

Change 488314 merged by jenkins-bot:
[mediawiki/extensions/ExternalGuidance@master] SpecialExternalGuidance: Validate the language codes and title parameters

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

@sbassett, @Reedy Are you planning to take another look on this? Or are we done? All the patches related to your comments are merged now. Thanks!

@santhosh - I think we'd like to (or at least I'd like to) review this code a bit more. We didn't have our weekly security review scrum during All-hands or the week after, so this is still kind of in limbo at the moment though we wanted to get you some initial feedback. We should be able to schedule a full review by next Tuesday (2/12) when we have our next scrum. Is there a more specific launch date other than "February 2019" you had in mind that would make this more pressing?

In T214152#4938843, @sbassett wrote:

@santhosh - I think we'd like to (or at least I'd like to) review this code a bit more. We didn't have our weekly security review scrum during All-hands or the week after, so this is still kind of in limbo at the moment though we wanted to get you some initial feedback. We should be able to schedule a full review by next Tuesday (2/12) when we have our next scrum. Is there a more specific launch date other than "February 2019" you had in mind that would make this more pressing?

We plan to deploy extension on 02/12.

@santhosh, @KartikMistry -

These changes look good and phan-taint-check likes them. I still want to review the aforementioned JS a bit more in depth, but I personally don't think that should stop your deployment timeline of 2/12.

Update: I did have a more in-depth look at the JavaScript under modules/ yesterday - again, I didn't find anything particularly concerning. I still wanted to pen-test it a bit further (was having some vagrant issues yesterday) so if I find anything from that, I'll post it here. Additionally, I see that the extension was deployed to simple.wikipedia.org and id.wikipedia.org earlier today, per T213076.

@sbassett we are doing tests now, and sometime early next week will have more clarity in terms of the timeline for deployment on enwiki. Approximately, a two week window is what we are looking at for the next deployment. Hope this is helpful for your task prioritization.

@Arrbee - that's good to know, thanks. Again, I think this code is looking pretty good overall with @santhosh's recent patch set. I was merely planning to perform some additional (kind of optional) analysis and pen-testing of the code for good measure. It shouldn't really be anything that prevents a full production deployment.