sbassett added subscribers: Mstyles, Reedy.
sbassett added a comment.
!!**Security Review Summary - TT264822 - 2021-06-25**!!
**Last commit reviewed: 2d65299a44**
**Summary**
Overall, the current Query Builder code looks fairly secure with certain
issues outlined below. I would currently rate the overall risk as: {icon
exclamation-triangle color=yellow} **Medium**. See a public-facing summary of
the WMF's risk management policy here: T249039#6309061
<https://phabricator.wikimedia.org/T249039#6309061> (sadly, the full version is
still protected under officewiki.)
**Vulnerable Packages - Production**
**None**: as verified with `auditjs`, `snyk` and `npm audit`. Still, I'd note
that these dependencies add an additional **584,927** lines of code to Query
Builder's codebase, thus dramatically increasing complexity and potential
future risk. And with dev dependencies, that figure becomes **9,678,194** lines
of code. Risk: {icon check-circle color=green} **Low**.
**Vulnerable Packages - Development**
`npm audit` (though curiously //not// `snyk` or `auditjs`) found a
//massive// number of development dependency vulnerabilites: **5,551** to be
exact. They break down as 1 low, 303 moderate and 5,247 high from 2,875
scanned packages. Allegedly, `npm audit fix` can be used to automatically
upgrade the vast majority to secure versions, while 35 require manual review.
While development dependency vulnerabilities typically pose a //substantially
smaller// risk than those found within production dependencies, the risk is not
zero, especially for development tools used to build production artifacts like
`vue-cli-service`. Just scanning the results, I'd note that a large volume of
these appear to be for the `@vue/cli-service`, `@vue/cli-plugin-unit-jest` and
`netlify-cli` dependencies, so bumping those to more recent versions (if
feasible) would likely substantially reduce this risk. For now, given the
sheer volume of vulnerabilities, and the fact these are for somewhat-critical
development tools, particularly `vue-cli-service`, this will be rated as a
{icon exclamation-triangle color=orange} **High Risk**.
**Outdated Packages**
As reported via `npm outdated`:
(no explicit vulnerabilities reported, simply noting for completeness' sake.)
Risk: {icon smile-o color=sky} **None**.
| Package
| Current | Wanted | Latest |
|
-------------------------------------------------------------------------------------------------
| -------------- | -------------- | -------- |
| Package <https://www.npmjs.com/package/Package>
| Current | Wanted | Latest |
| @types/jest <https://www.npmjs.com/package/@types/jest>
| 24.9.1 | 24.9.1 | 26.0.23 |
| @types/lodash <https://www.npmjs.com/package/@types/lodash>
| 4.14.168 | 4.14.170 | 4.14.170 |
| @types/node <https://www.npmjs.com/package/@types/node>
| 14.14.28 | 14.17.4 | 15.12.4 |
| @typescript-eslint/eslint-plugin
<https://www.npmjs.com/package/@typescript-eslint/eslint-plugin> | 2.34.0
| 2.34.0 | 4.28.0 |
| @typescript-eslint/parser
<https://www.npmjs.com/package/@typescript-eslint/parser> |
2.34.0 | 2.34.0 | 4.28.0 |
| @vue/cli-plugin-babel <https://www.npmjs.com/package/@vue/cli-plugin-babel>
| 4.5.11 | 4.5.13 | 4.5.13 |
| @vue/cli-plugin-eslint
<https://www.npmjs.com/package/@vue/cli-plugin-eslint> |
4.5.11 | 4.5.13 | 4.5.13 |
| @vue/cli-plugin-typescript
<https://www.npmjs.com/package/@vue/cli-plugin-typescript> | 4.5.11
| 4.5.13 | 4.5.13 |
| @vue/cli-plugin-unit-jest
<https://www.npmjs.com/package/@vue/cli-plugin-unit-jest> |
4.5.11 | 4.5.13 | 4.5.13 |
| @vue/cli-plugin-vuex <https://www.npmjs.com/package/@vue/cli-plugin-vuex>
| 4.5.11 | 4.5.13 | 4.5.13 |
| @vue/cli-service <https://www.npmjs.com/package/@vue/cli-service>
| 4.5.11 | 4.5.13 | 4.5.13 |
| @vue/eslint-config-typescript
<https://www.npmjs.com/package/@vue/eslint-config-typescript> | 5.1.0
| 5.1.0 | 7.0.0 |
| @vue/test-utils <https://www.npmjs.com/package/@vue/test-utils>
| 1.1.3 | 1.2.1 | 1.2.1 |
| @wmde/wikit-tokens <https://www.npmjs.com/package/@wmde/wikit-tokens>
| 2.0.0-alpha.12 | 2.0.0-alpha.12 | 1.1.2 |
| @wmde/wikit-vue-components
<https://www.npmjs.com/package/@wmde/wikit-vue-components> |
2.0.0-alpha.12 | 2.0.0-alpha.12 | 1.1.2 |
| core-js <https://www.npmjs.com/package/core-js>
| 3.8.3 | 3.15.1 | 3.15.1 |
| cypress <https://www.npmjs.com/package/cypress>
| 6.3.0 | 6.9.1 | 7.6.0 |
| eslint <https://www.npmjs.com/package/eslint>
| 7.20.0 | 7.29.0 | 7.29.0 |
| eslint-config-wikimedia
<https://www.npmjs.com/package/eslint-config-wikimedia> |
0.17.0 | 0.17.0 | 0.20.0 |
| eslint-plugin-chai-friendly
<https://www.npmjs.com/package/eslint-plugin-chai-friendly> | 0.6.0
| 0.6.0 | 0.7.1 |
| eslint-plugin-cypress <https://www.npmjs.com/package/eslint-plugin-cypress>
| 2.11.2 | 2.11.3 | 2.11.3 |
| eslint-plugin-vue <https://www.npmjs.com/package/eslint-plugin-vue>
| 6.2.2 | 6.2.2 | 7.12.1 |
| geckodriver <https://www.npmjs.com/package/geckodriver>
| 1.22.1 | 1.22.3 | 2.0.0 |
| jest-axe <https://www.npmjs.com/package/jest-axe>
| 4.1.0 | 4.1.0 | 5.0.1 |
| netlify-cli <https://www.npmjs.com/package/netlify-cli>
| 2.71.0 | 2.71.0 | 3.38.10 |
| ress <https://www.npmjs.com/package/ress>
| 3.0.0 | 3.0.0 | 4.0.0 |
| sass <https://www.npmjs.com/package/sass>
| 1.32.7 | 1.35.1 | 1.35.1 |
| sass-loader <https://www.npmjs.com/package/sass-loader>
| 8.0.2 | 8.0.2 | 12.1.0 |
| sparqljs <https://www.npmjs.com/package/sparqljs>
| 3.2.0 | 3.4.2 | 3.4.2 |
| start-server-and-test <https://www.npmjs.com/package/start-server-and-test>
| 1.12.0 | 1.12.5 | 1.12.5 |
| stylelint <https://www.npmjs.com/package/stylelint>
| 13.10.0 | 13.13.1 | 13.13.1 |
| stylelint-config-standard
<https://www.npmjs.com/package/stylelint-config-standard> |
20.0.0 | 20.0.0 | 22.0.0 |
| typescript <https://www.npmjs.com/package/typescript>
| 3.9.9 | 3.9.10 | 4.3.4 |
| vue <https://www.npmjs.com/package/vue>
| 2.6.12 | 2.6.14 | 2.6.14 |
| vue-banana-i18n <https://www.npmjs.com/package/vue-banana-i18n>
| 1.3.0 | 1.4.0 | 1.4.0 |
| vue-template-compiler <https://www.npmjs.com/package/vue-template-compiler>
| 2.6.12 | 2.6.14 | 2.6.14 |
|
**Security Headers**
Similar to query.wikidata.org, the toolforge.com test site is missing a
handful of desired security headers per this automated report
<https://securityheaders.com/?q=https%3A%2F%2Fquery-builder-test.toolforge.org%2F&followRedirects=on>.
I understand that the lack of an enforcing CSP and x-frame-options are likely
due to various global policies and stalled implementations across //all//
Wikimedia projects, though I'd note there are overriding options for this
specific project, if it is to be run as a Nodejs app within production. Risk:
{icon check-circle color=green} **Low**, given extenuating circumstances.
**DoS Vectors (includeing ReDoS)**
I wasn't able to perform any exhaustive performance-testing for this
application, but did not see anything within the code that seemed out of the
ordinary for Wikimedia typescript/javascript apps that would indicate a cause
for concern. Still, as this app is intended to be deployed under wikidata.org
at some point, I would recommend a performance review
<https://www.mediawiki.org/wiki/Wikimedia_Performance_Team/Performance_Review>
from the #performance-team
<https://phabricator.wikimedia.org/tag/performance-team/> prior to deployment
(I did not see a Phab task for such a review after a quick search). Risk:
{icon smile-o color=sky} **None**, pending a formal performance review.
**Build/Test steps**
Currently, the code uses vue-cli-services to build its dist artifacts, which
by default uses certain webpack dependencies. Given the complexity and known
code quality issues of webpack, this will be categorized as a {icon
exclamation-triangle color=yellow} **Medium Risk**. I's also note that any
webpack-generated dist artifacts should be posted to gerrit for CR with //at
least one// member of the AppSec contingent of the #security-team
<https://phabricator.wikimedia.org/tag/security-team/> (@reedy, @mstyles,
@sbassett) added to the review.
**General Security Issues**
I didn't find much here, based upon a cursory, manual review of the `/src`
directory. Some automated scanners (`njsscan`, `semgrep`) picked up a few
things - uses of `v-html` and `Math.random()` (which isn't
cryptographically-secure of course), all of which seemed to be false positives,
though I've included a raw report of these issues as a protected paste here:
P16733. Risk: {icon check-circle color=green} **Low**.
**Policy Compliance**
Just noting for completeness' sake that the privacy policy referenced under
the query-builder-test.toolforge.org test site will need to be updated to a
valid Wikimedia privacy policy when the tool is eventually hosted under
query.wikidata.org. This has a current risk of {icon smile-o color=sky}
**None**.
TASK DETAIL
https://phabricator.wikimedia.org/T264822
EMAIL PREFERENCES
https://phabricator.wikimedia.org/settings/panel/emailpreferences/
To: sbassett
Cc: Reedy, Mstyles, karapayneWMDE, Addshore, sbassett, Michael, Ladsgroup,
Lydia_Pintscher, Jakob_WMDE, guergana.tzatchkova, conny-kawohl_WMDE, bete,
Aklapper, Invadibot, Devnull, maantietaja, Akuckartz, Jcross, Dsharpe,
DannyS712, Nandana, Lahi, Gq86, GoranSMilovanovic, QZanden, LawExplorer,
_jensen, rosalieper, Scott_WUaS, Wikidata-bugs, aude, Bawolff, Mbch331, Legoktm
_______________________________________________
Wikidata-bugs mailing list -- [email protected]
To unsubscribe send an email to [email protected]
