@sbassett, @Reedy, we believe the NPM dependencies to be very stable for the WVUI library (now hosted in Gerrit) and the feat/search of Vector and ready for review. Code directly in the library and skin themselves are under active development.

Security Review Summary - T257579 - 2020-09-16
WVUI - Last commit reviewed: 74f911a50a59bd39f88b686ba89c2b5ee80734a7
Vector:feat/seaerch - Last commit reviewed: b3aa4d437dfe2de44d2a25f2c4a5879cae302c8e

Summary

This is an initial review of WVUI and Vector's dependencies as requested by @Niedzielski. A more in-depth security review, once Vue3 has been released and the rest of the code is stable, will follow.

WVUI

Vulnerable Packages - Development and Production

Outdated Packages
As reported via npm outdated:
(no explicit vulnerabilities reported, simply noting for completeness' sake.)

Vector feat/search

Vulnerable Packages - Development and Production

Outdated Packages
As reported via npm outdated:
(no explicit vulnerabilities reported, simply noting for completeness' sake.)

Change 628877 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[wvui@master] [build] Fix audited NPM packages

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

Thank you, @sbassett! As a starting point, I've applied compatible fixes to WVUI with npm audit fix. This appears to have fixed all vulnerabilities detected by snyk.

tinycolor2

I see that this is a dependency of @storybook/addon-knobs. I don't see the jquery as a package.json dependency but it appears to be file copied into the repo's demos: node_modules/tinycolor2/demo/jquery-1.9.1.js. Can we ignore this?

Regarding the many outdated packages, I am happy to revise all or most of these if that's wanted. I would actually prefer to upgrade as much as possible but was pinning the dependencies with the intent of minimizing security review churn. Please let me know if it's ok to upgrade and I'll plan accordingly.

Once the WVUI packages are up to date, I'll revise Vector with matching package versions wherever possible.

In T257579#6481175, @Niedzielski wrote:

As a starting point, I've applied compatible fixes to WVUI with npm audit fix. This appears to have fixed all vulnerabilities detected by snyk.

Sounds good.

tinycolor2

I see that this is a dependency of @storybook/addon-knobs. I don't see the jquery as a package.json dependency but it appears to be file copied into the repo's demos: node_modules/tinycolor2/demo/jquery-1.9.1.js. Can we ignore this?

If it's never part of a production build or deployment step, then I will rate it a low risk.

Regarding the many outdated packages, I am happy to revise all or most of these if that's wanted. I would actually prefer to upgrade as much as possible but was pinning the dependencies with the intent of minimizing security review churn. Please let me know if it's ok to upgrade and I'll plan accordingly.

Totally fine to upgrade - these will get re-reviewed (even though we try to avoid that) once Vue3 is released and the Wikimedia code is stable.

Once the WVUI packages are up to date, I'll revise Vector with matching package versions wherever possible.

Sounds good, thanks.

Change 628877 merged by Niedzielski:
[wvui@master] [build] Fix audited NPM packages

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

Change 629213 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[wvui@master] [build] Upgrade dependencies

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

Change 629213 merged by Niedzielski:
[wvui@master] [build] Upgrade dependencies

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

Change 629463 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/Vector@feat/search] [build] Upgrade dependencies

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

The above patches audit fix and upgrade everything in WVUI and Vector.^^^

Change 629463 merged by jenkins-bot:
[mediawiki/skins/Vector@feat/search] [build] Upgrade dependencies

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

Hey @Niedzielski - I just wanted to check in and see if there are any updates on desired deployment dates. I still plan to finish this review (and the related Vue3 review) sometime this month, but was to hoping to set a more specific date/time based upon your current estimates. Thanks.

Hi @nnikkhoui - we were told that you are the new contact for both this ticket and https://phabricator.wikimedia.org/T257734. We're wondering if there are any changes we need to know about and whether you have a new deployment timeline? Thanks so much.

Hi @Jcross I'm not sure I would actually be the best contact for this, @Jdrewniak might be more helpful in providing a new deployment timeline estimate!

In T257579#6677898, @Jdlrobson wrote:

Moving this to the board as we're getting to the point where we are ready to do this.

Hey @Jdlrobson - we should be able to get this review scheduled for early next quarter.

Per our conversation today, security review could likely be limited to:

• The source code for the wvui library https://github.com/wikimedia/wvui
• The bundle created from wvui library https://gerrit.wikimedia.org/r/c/mediawiki/core/+/641052

At this point we don't need any security review for Vue 3 (or Vector itself)

Change 660959 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[wvui@master] [build] Update 'prettier' linter devDependency

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

Change 660961 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[wvui@master] [build] Update 'stylelint' linter devDependency

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

Update: I'm still hopeful to have this review completed by the end of this week (2021-02-05). @Volker_E - thanks for the version bumps, I'll pull those down for the review if they haven't been merged yet. Also - with the webpack build step still being in place, I'm going to rate the overall risk of wvui in its current state as at least medium, which will require manager/director acceptance of the risk, per our risk managment policy, as publicly summarized here: T249039#6309061.

Change 660959 merged by jenkins-bot:
[wvui@master] [build] Update 'prettier' linter devDependency

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

Change 660961 merged by jenkins-bot:
[wvui@master] [build] Update 'stylelint' linter devDependency

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

Security Review Summary - T257579 - 2020-02-05
Last commit reviewed: 7fd111ad5ed3165683a76f68c3d3504f24cc179f

Summary

Overall, the current wvui code looks pretty good from a security standpoint, with a few caveats mentioned below. I'm assigning an overall risk rating of medium largely due to the current webpack-based build steps.

Note: per conversations external to this task and T257579#6689830, this review does not include a review of any feat/search branch changes for the Vector skin.

Vulnerable Packages - Production

None found via npm audit, snyk test, auditjs. low risk.

Vulnerable Packages - Development

low risk.

Outdated Packages
As reported via npm outdated:
(no explicit vulnerabilities reported, simply noting for completeness' sake.)

Build/Test steps

1. Currently, the code uses vue-cli-services to build its dist objects and storybook ui/doc, which by default uses webpack. Given the complexity and known code quality issues of webpack, this will be categorized as a medium risk.

General Security Issues

1. As this is a component library, there is a lot of functionality that should not be restricted from developers. Though it should be noted that components like Button and TypeaheadSearch, which allow arbitrary html within their slots, are potentially dangerous if a developer doesn't understand what they are doing and/or potential sanitization is implemented incorrectly or not at all. The same can be said for certain bound attributes or if v-html is ever used, which it isn't within wvui. It's likely a good idea to note these specific examples within whatever high-level developer documentation exists (or may exist) for wvui. low risk.

Miscellanous Issues/Points of Discussion/Nits

1. I noticed in the TypeaheadSuggestion examples in the storybook install under https://doc.wikimedia.org/wvui/master/ui/, that the clickable components load the relative link address of /w/index.php?title=Special:Search&search=searchTerm under doc.wikimedia.org, which obviously 404s. no risk.

Per @sbassett 's review and the requirement for manager sign-off, and as the interim manager for the Web team, I am going to approve the deployment of WVUI per the discussions I've had with @phuedx and @Jdlrobson today.

For the time being we believe this code does not need to live on high-traffic wikis, and we believe that removing any potential threat, once detected, is a simple matter of deploying a config change to disable it.

@phuedx has suggested the possibility that we only enable this for long enough to complete an A/B test in production, however, after a conversation with @Jdlrobson I think we can leave it unless we see an issue, or if the team decides they would prefer not to have to look over their shoulders for potential security threats.

n.b. a wider deployment of this code would likely depend on completion of T272879 - which would remove the problems associated with Webpack. (I'm presently unsure to what extent there would be exciting new problems with Rollup)

In any case, consider this Manager Approved™. I will also put a cheeky +1 on the relevant patch with an abbreviated version of this message.

Thanks, everyone!

In T257579#6807916, @sbassett wrote:

Vulnerable Packages - Production

None found via npm audit, snyk test, auditjs. low risk. […]

It is a good starting point to look for known issues other people may have randomly found and reported to Snyk in recent history.

However, I think this is is similar to the work done in T257579#6474758, which was previously deemed insufficient for production deployment affecting up to a billion people when dealing with Webpack, and is what led to the Vue.js Task force.

We are still distributing the result of executing thousands of completely novel and unreviewed packages, many of which will have been published in the last few days or weeks.

Doing this would completely defeat the purpose of all past security reviews we've had for frontend libs that ship with MediaWiki, such as Mustache.js, Moment.js, and dozens of others. I would assume that we don't deploy Debian packages by the same measure, that if they're on some random third-party GitHub repo but nobody has filed CVEs for them, then it's good to go and deploy on production hardware?

Based on the outcome of the task force (cc-in @phuedx, @Volker_E, @nnikkhoui, @Catrope, @egardner) it was my understanding that we are to review the Webpack output on the initial check-in and treat it as if it were our own code (and 90% of it is, of course, our own code, with the other 10% being artefacts from Webpack's process). This sets a base line after which incremental updates can be reviewed simply by merely confriming the change is what you intended to release. This is how OOjs, OOUI and other in-house libraries have been deployed for years.

To faccilitate this review, and to allow for applying of security paches in production, it was recommended to disable minification of the build, and to use Rollup depending on how reviewable the unminified Webpack output is. This has been done by the team in past months, and the output has been ready for review in Gerrit for some time now. I'm not sure who is waiting for whom, however.

The big plus from all this is that review of the npm packages is completely unnececary since we won't be using those in the build service or otherwise including them blindly in any output. We're only deploying what we've seen and how it gets made doesn't matter. The same applies to the simpler Grunt or Rollup based libraries that we have deployed in past years already. Their development dependencies have not and need not be audited either.

In T257579#6812592, @MarkTraceur wrote:

[…] For the time being we believe this code does not need to live on high-traffic wikis, and we believe that removing any potential threat, once detected, is a simple matter of deploying a config change to disable it.
[…] n.b. a wider deployment of this code would likely depend on completion of T272879 - which would remove the problems associated with Webpack. (I'm presently unsure to what extent there would be exciting new problems with Rollup)

Reducing traffic helps from a product perspective to reduce visibility of experiences by larger portions of the community. It does not however reduce attack vectors much since *.wikipedia.org is a shared trust domain, and with CentralAuth and global login it is actually anecdotally more problematic to steer attacks to smaller wikis as there are fewer eyes on them. Note that JS can trivially make edits to larger wikis from a smaller wiki (and we faccilitate this in mw.Api.js with the centralauthauth feature).

No worries either way, but I don't think the number of wikis deployed to should play a role in how we assess security risk, or in justifying the forgoing of a security review.

I just want to state here that I'm looking into alternative build tooling (specifically, Evan You's new Vite library, which uses Rollup – as well as just using Rollup directly). There may be ways we can build the library that involve fewer packages but deliver equivalent results (modern + UMD builds, efficient minification, externalizing the dependency on Vue itself, type definitions, etc). Deciding at the outset that WVUI will never need to support IE11 will probably allow us to eliminate some problematic dependencies. I will have a better sense of how feasible this is once we can discuss what use-cases WVUI needs to support exactly.

Pasting from Gerrit as it seems a few people have read T257579#6812862 but not seen the related Gerrit discussion:

@krinkle: Where the the security review happen? The linked task (T262566) has no mention of it. The original security task T257579 also shows no code review of any kind, merely a repeaet of the same npm-audit of past CVEs found in some older versions of some of the packages involved. That is not a security review and not what was agreed upon in the task force. It still deployed results of 100s of recently uploaded packages never reviewed or seen packages by anyone.

@Jdlrobson: To check I understand the question, are you asking whether the webpack artifacts code (posted here: https://gist.github.com/jdlrobson/449282f1b41d9fbcb66704451fb9d7ac) was code reviewed?

@krinkle: Yep, the work is basically extracting that and reviewing it as our own. Security might be able to help with that if we encounter something we're unsure of, but if it's trivial code that you'd write similarly yourself or otherwise understand how it works and wouldn't mind approving it into regular source code if someone were to submit a patch containing code like it, then that's pretty  much it.

Thanks for linking that, that's most of it done already indeed. I must've missed that. Thanks!

All - I believe there are some fairly significant misunderstandings regarding items related to:

1. How the Security-Team performs various security reviews
2. What is included within Security-Team review deliverables
3. What the Security-Team will and will not review based upon current resources and priorities
4. The concepts of risk assessment and risk ownership as they relate to various Security-Team reviews
5. How to best communicate with the Security-Team through our official, documented processes

and likely a few others. We're going to have a few team discussions on some of the comments here with the hope of providing some clarifications to reset certain incorrect expectations and assumptions.

For the record, Scott had already addressed my concerns and has now shown me where on the Gerrit patch. I didn't realize. Thanks all.

I'm going to chime in here:

The Security team has an established risk-based process for providing security reviews that is designed to scale with the needs of the organization and with the capabilities of the team [0, 1]. Following along the thread of comments it is clear to me that not everybody understands the role or use of risk as it relates to this process. There are a few notions here that I will plainly state in an effort to put everyone on equal footing:

1. The Security team is authoritative for security reviews. We love and value everyone’s feedback and suggestions, and require it to improve our processes, but at the end of the day the persons on the Security team responsible for this work are the SMEs. If you have questions or concerns, we are happy to walk those out, but as far as I am concerned this review has been tagged as Medium risk and that risk has been accepted by the relevant project owners.

2. Teams have the ability to choose their own adventure via risk. What this means is that the Security team does not exist to say yes or no. Now, I need to acknowledge there are always exceptions to this rule but they are rare and this is not one of those cases. Modern security programs use risk to empower engineers and teams to make good security decisions. Folks are briefed on risks, potential mitigations and are allowed to make decisions based upon their risk appetite. With this risk acceptance comes a sliding scale of accountability and acknowledgement based upon severity that goes all the way up to the CEO.

3. The Security team practices due diligence. This means that when deliverables are included as part of a security review, it's intention is to surface items of concern or items that contribute to risk. There are a lot of other activities that occur that likely won't land within any deliverable but were part of a review. Just because you don't see it in phabricator or in the report doesn't mean it wasn’t covered. Sometimes security readiness reviews take the form of lightweight, conceptual discussions (T227820), sometimes the report deliverable is minimal when few issues are found (T260822), sometimes we leverage external vendors to perform reviews (T240869) and some reviews are quite complex with many issues found and many rounds of interaction and confirmation (T133408).

4. Looking forward it seems to me there are some issues around the way the Security team and others address, create and communicate the notion of a 'standard' or even just expectations and accountabilities in general. With that in mind it seems like we have an opportunity here to work together on some security concerns that might not be covered as part of the vue wg? I don't know if that is true or not, but that is the impression I get. Please raise your hand if that is interesting to you and I'll get wheels turning.

5. Have you ever been to a Security team office hour? We will be covering cybersecurity risk concepts on Feb. 25th (on staff calendar). Come hang out, learn about risk and ask lots of questions. I look forward to seeing you there.

[0] https://office.wikimedia.org/wiki/Security/Policy/Risk_Management
[1] https://www.mediawiki.org/wiki/Security/SOP/Security_Readiness_Reviews

Change 663695 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[wvui@master] [build] Fix audited npm package 'bundlesize'

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

Change 663695 merged by jenkins-bot:
[wvui@master] [build] Fix audited npm package 'bundlesize'

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

Change 663708 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[labs/libraryupgrader/config@master] Add the un-namespaced wvui to LibUp's list of repos

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

Change 663708 merged by jenkins-bot:
[labs/libraryupgrader/config@master] Add the un-namespaced wvui to LibUp's list of repos

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

This is a Security Readiness Review request, so it should be tagged with Application Security Reviews which allows to find all and any security readiness review requests by looking at tasks tagged with Application Security Reviews. If this task is not an actionable review request, then please set its task status to "stalled". Once this task becomes actionable, please change back the task status to "open". Thanks.

First and foremost, thanks @sbassett and @JBennett for chiming in here on the security readiness review process, and the risk-level-based vs binary yes/no assessment. I think this is topic worth repeatedly mentioning.

Looking forward it seems to me there are some issues around the way the Security team and others address, create and communicate the notion of a 'standard' or even just expectations and accountabilities in general. With that in mind it seems like we have an opportunity here to work together on some security concerns that might not be covered as part of the vue wg? I don't know if that is true or not, but that is the impression I get. Please raise your hand if that is interesting to you and I'll get wheels turning.

Not exactly what you asked about, but WMDE would be interested in hearing more about the security topic in regards to npm/webpack. This has seemed to be somehow pondered by the VueJS Task Force in late 2020, but I have only managed to identify the statements that the intended way of integrating with the open source JS/npm ecosystem would likely be having some handpicked (and I take very limited) set of packages (in particular versions?) as the only available set of dependencies to use.
If this is the final decision (I am unclear about this) what we are missing is a trail of considerations that have led to this decision. While it is clear to me that this might be most "air-tight" solution from the security perspective, security is only one of dimensions in the multi-dimensional software development process. I would be curious to see how the impact/influence of this approach has been weighted against other aspects like e.g. developer productivity, onboarding, being active part of the open source developer community, etc. Similarly, I would also like to read up what other approaches/solutions have been considered and on the reasoning that led the choosing the "subset of handpicked packages" as the most appropriate overall.

It might be the completely wrong venue to discuss this - please advise where I should move it. I am also aware this is not only a topic for the Security Team - but since you've asked...

In T257579#6840458, @WMDE-leszek wrote:

Not exactly what you asked about, but WMDE would be interested in hearing more about the security topic in regards to npm/webpack. This has seemed to be somehow pondered by the VueJS Task Force in late 2020, but I have only managed to identify the statements that the intended way of integrating with the open source JS/npm ecosystem would likely be having some handpicked (and I take very limited) set of packages (in particular versions?) as the only available set of dependencies to use.

Good question. I think that part of why there has been limited public messaging around JS/NPM security policy is that no official decision has been made yet. However, I can share some of the concerns that were discussed starting with last year's Task Force and continuing this year between the Vue Migration Team and the Web Team.

As we start looking at ways to introduce modern front-end tooling into our work, there is no escaping from the fact that NPM is basically the "wild west" when it comes to security and stability of packages in the ecosystem. Compounding this is the fact that many popular tools have a huge number of dependencies (for example babel-cli depends on 190 packages, not counting devDependencies). Webpack is somewhat more reasonable, but typically a lot of 3rd-party plugins are needed to support custom transformations.

A huge network of separate packages (maintained by different groups, or maybe not maintained regularly at all) represents a pretty broad surface area for attacks. This is a good example (from the end of last year) of the kind of thing I'd like to avoid – a package posing as an API client library was designed to compromise developers machines. But an even bigger danger would be something that participated in the build process and managed to ship compromised code to users. When build tools that have not been audited (or with un-audited dependencies) participate in generating a large and unreadable code bundle, this kind of attack becomes possible as well.

security is only one of dimensions in the multi-dimensional software development process. I would be curious to see how the impact/influence of this approach has been weighted against other aspects like e.g. developer productivity, onboarding, being active part of the open source developer community, etc

I very much agree with this. Personally I think our traditional approach (write all code in ES5, only use a small number of 3rd-party libraries, no build tools, etc) has too high a cost in terms of productivity, onboarding, and separates us from the wider front-end community. In the Vue Migration Team, we have been looking at ways to balance these needs.

No final decision around any of this has been made yet, but I'm hoping that we can come up with a compromise that provides developers with more modern capabilities without opening up a large new set of security or maintenance problems. I am particularly interested in some newer build tools like Rollup and Evan You's Vite – these libraries might be able to give us a lot what Webpack offers without the complex web of dependencies and accompanying problems (especially if we can drop the need to transpile code back to ES5). Typescript is another good example of a stand-alone tool that provides a lot of value. For tools like Storybook that are useful during development but don't participate in the final output generation, there may be ways to isolate them environments where the production build actually takes place – perhaps NPM's optionalDependencies feature would make sense here.

A final decision around how to manage front-end dependencies might need to come out of a Technical Decision Making Process (probably around introducing a CI build step). In the short term the Vue Migration Team is doing some research and might put out a propsal that fleshes out some of what I just mentioned above. We definitely should compare notes with WMDE in the near future, since you all have been working with some of these tools for some time now.

In T257579#6841524, @egardner wrote:

(especially if we can drop the need to transpile code back to ES5)

To elaborate on this: I am currently working on T272104: Allow modules to opt-in to ES6 syntax support and T272882: Upgrade ResourceLoader JS minifier to support ES6, which will allow us to write ES6 code and ship it directly to the browser without using Babel. Modules using this feature would not be loaded in browsers that don't support ES6 (mostly, IE11), but we think that's fine since IE11's market share is low, and Vue 3 doesn't support IE11 anyway.