Security review was performed. We need to go through the list of items and acknowledge if they will be fixed, won't fix, or just ack. See guidelines for the response:

In T227114#5553485, @sbassett wrote:

@Jhernandez - Unless there are high/critical issues surfaced during a security review (which we'd most likely use to block deployment) I think we tend to provide these reports to the requesters and let them work through them however they please. Feel free to create checkboxes for the review items within the original task description, within another on-task comment, in various subtasks or comment on them in-line within a new on-task comment. For items which you do not plan to address (perhaps certain tertiary vulnerable dependencies), feel free to mark them WONTFIX or similar. Any purely informational items can either be acked or ignored. The code would currently have an overall risk of low or low-medium until the issues noted within the review are addressed or resolved. If you have any further questions, let me know.

When done editing this description, copy the resulting response and paste it in the parent task for the Security-Team to see.

Security review

In T227114#5501496, @sbassett wrote:

Security Review Summary - T227114 - September 2019
In addition to the CSP discussion starting at T227114#5432647, I found a few more issues below. The vulnerable packages sections are more general and not necesarrily related to the specifics of the pcs//page/mobile-html endpoint, but are still something I try to call out during any review for any related code. For the rest of the review, I focused upon routes/page/mobile-html.js, lib/transformations/pcs/head.js, the wikimedia-page-library pcs code and various related packages. I've also indicated my assessment of risk for each of the items below, as the Security-Team is trying to socialize the concepts of risk assessment and ownership for teams/developers over simply providing some quasi-authoritative "this passes / doesn't" statement at the end of a review.

Vulnerable Packages
As reported by npm audit for mobileapps:

1. clean-css, ReDos (risk: low)
   • dependency of: dev dependecy less-plugin-clean-css
   • https://npmjs.com/advisories/785

As reported by retirejs for mobileapps:

1. Various jqueries
   • jquery 1.7.1 (CVE-2012-6708, CVE-2015-9251, CVE-2019-11358) (risk: low)
     • dependency of: clarinet, service-mobileapp-node (tests)
   • jquery 1.9.1 (CVE-2015-9251, CVE-2019-11358) (risk: low)
     • dependency of: domino, service-mobileapp-node (tests)
   • jquery 2.2.0 (CVE-2015-9251, CVE-2019-11358) (risk: low)
     • dependency of: domino, service-mobileapp-node (tests)

As reported by retirejs for wikipedia-page-library:

1. Various libraries
   • angularjs 1.4.14 (risk: medium)
     • dev dependency of: browser-sync-ui
       • https://github.com/angular/angular.js/blob/master/CHANGELOG.md#1230-patronal-resurrection-2016-07-21
       • https://github.com/mozilla/addons-linter/issues/1000#issuecomment-282083435
       • https://github.com/angular/angular.js/commit/8f31f1ff43b673a24f84422d5c13d6312b2c4d94
       • https://github.com/angular/angular.js/pull/15699
   • jquery 1.9.1 (CVE-2015-9251, CVE-2019-11358) (risk: low)
     • dependency of: domino, service-mobileapp-node (tests)
   • jquery 2.2.0 (CVE-2015-9251, CVE-2019-11358) (risk: low)
     • dependency of: domino, service-mobileapp-node (tests)

As reported by snyk test for mobileapps:

1. ms, ReDos (risk: low)
   • dependency of: kad@1.3.6, limitation@0.2.1, service-runner@2.7.3
   • https://snyk.io/vuln/npm:ms:20170412

Outdated Packages

1. Several packages came up for both mobileapps and wikimedia-page-library when running npm outdated. No obvious security issues per se, but something to be mindful of and reported here for completeness.

Security Headers

1. config.dev.yaml - the mobile_html_csp on line 64 here seems ok, though I might recommend further restricting media-src and image-src to localhost and any relevant wikimedia domains (as we shouldn't ever have to worry about external content leaks.) (risk: low)
2. app.js - app.conf.csp on line 51 also seems fine, but again, if we could more tightly restrict media-src and image-src as described above for config.dev.yaml, that would be ideal. (risk: low)
3. app.js - app.conf.mobile_html_csp on line 67 also seems fine, but again, if we could more tightly restrict media-src and image-src as described above for config.dev.yaml, that would be ideal. (risk: low)
4. The CSPs for the mobileapps service are only going to be set by the service itself and any consumers which also render the headers within a browser context. I assume this is how the Wikipedia mobile apps and other readers are intended to use the /page/mobile-html endpoint, but I wanted to explicitly call this out to make sure.
5. No Public-Key-Pins or Strict-Transport-Security security headers, but as the default mobileapps service doesn't use TLS (and possibly not the production service?) these are obviously not necessary. (risk: low)
6. The default Access-Control-Allow-Origin header for the /page/mobile-html endpont (and most others, from what I'm seeing within the /spec directory) is a wildcard, which is extremely permissive. I assume this is necessary though for the proper functioning of the service. (risk: medium)

TLS/SSL

1. The dev/local environment obviously doesn't make use of TLS, which is fine, but does the production environment? I'm not sure if there's an actual standard or best practice for Wikimedia services, but as a standard security best practice, I'd still at least recommend TLS even if there is no authn/z or sensitive data in transit. (risk: medium)

General Security Issues

1. Having code like this on a random test page <div style="position:absolute;top:0;left:0;width:100%;height:100%;background:black;">&nbsp;</div> seems to effectively serve as a sort of content DoS, as it obscures any content on the page. Such html doesn't render this way within production MediaWikis for various projects. See for example https://test.wikipedia.org/wiki/A_Special_New_Article (if it hasn't been deleted) vs. //localhost/test.wikipedia.org/v1/page/mobile-html/A_Special_New_Article. This would be low risk for sure, as it wouldn't be very exploitable, but it might make sense to have a transform, filter or warning for such styles. (risk: low)
2. Just a general note on various JavaScript sinks like innerHTML, appendChild(), etc. - while I'm under the impression that one of the primary requirements of the pcs//page/mobile-html endpoint is to accomodate a large swath of potential html, css and JavaScript and that most of the sources (mw API, parsoid, etc.) are generally deemed safe-ish, it is still preferable to further validate various values, especially any potentially dangerous code which could be rendered within the DOM after some delay, e.g. <img src=x onerror='alert(1)'>. (risk: low)

Code Cleanliness Issues

1. routes/page/mobile-html.js - line 102 - commented-out code should probably be removed before deployment, lest unintended effects occur if it were accidentally uncommented. (risk: low)

Other Oddities

1. Within my local testing build, I noticed that the /page/mobile-html route only supports a few varieties of MediaWiki namespaces, i.e. it does not support Special pages or User subpages. This isn't a security issue and I assume this is by design. (risk: none)

Checklist

Vulnerable packages and outdated packages

• clean-css, ReDos (risk: low)

Will track upstream issue that will fix it https://github.com/less/less-plugin-clean-css/issues/29

• jquery 1.9.1 (CVE-2015-9251, CVE-2019-11358) (mobileapps, pagelib) (risk: low)

WONTFIX: 3rd-party dev-dependency never deployed to production code, only used by domino tests see: https://github.com/fgnass/domino/blob/e19e8bf1570faa761d3145611a23d77dc21247b2/test/domino.js

• jquery 2.2.0 (CVE-2015-9251, CVE-2019-11358) (mobileapps, pagelib) (risk: low)

WONTFIX: 3rd-party dev-dependency never deployed to production code, only used by domino tests see: https://github.com/fgnass/domino/blob/e19e8bf1570faa761d3145611a23d77dc21247b2/test/domino.js

• jquery 1.7.1 (CVE-2012-6708, CVE-2015-9251, CVE-2019-11358) (risk: low)

WONTFIX: service-runner have various outdated dependencies that doesn't seem to be on the roadmap to be fixed soon

• ms, ReDos (risk: low)

WONTFIX: service-runner have various outdated dependencies that doesn't seem to be on the roadmap to be fixed soon

• angularjs 1.4.14 (risk: medium)

WONTFIX: dev dependency rarely used that doesn't have and upstream package is archived and doesn't seem inteded to fix it, see https://github.com/BrowserSync/UI/issues/58

• Outdated Packages

ACK

Security Headers

• 1. config.dev.yaml [...] (risk: low)

https://gerrit.wikimedia.org/r/c/mediawiki/services/mobileapps/+/553480

• 2. app.js - app.conf.csp [...] (risk: low)

https://gerrit.wikimedia.org/r/c/mediawiki/services/mobileapps/+/553480

• 3. app.js - app.conf.mobile_html_csp [...] (risk: low)

https://gerrit.wikimedia.org/r/c/mediawiki/services/mobileapps/+/553480

• 4. The CSPs for the mobileapps [...]

Yes. This is the intended behavior.

• 5. No Public-Key-Pins or Strict-Transport-Security security headers [...] (risk: low)

ACK. See T227114#5503143

• 6. The default Access-Control-Allow-Origin header [...] (risk: medium)

Yes. This is the intended behavior. I would highlight from T227114#5503143 that "all traffic to and from this service in production is entirely internal to the cluster".

TLS/SSL

• 1. The dev/local environment [...] (risk: medium)

ACK. See T227114#5503143

General Security Issues

• 1. Having code like this on a random test page [...] (risk: low)

Follow-up in T239615: mobile-html: filter potentially harmful style tags

• 2. Just a general note on various JavaScript sinks [...] (risk: low)

Follow-up in T239619: mobile-html: validate innerHTML and appendChild() for potentially dangerous code

Code Cleanliness Issues

• 1. routes/page/mobile-html.js [...] (risk: low)

DONE in https://gerrit.wikimedia.org/r/c/mediawiki/services/mobileapps/+/554150

Other Oddities

• 1. Within my local testing build [...] (risk: none)

ACK