We will have a code-freeze in the next two weeks and would appreciate a security review.

@Lydia_Pintscher - We've tentatively scheduled this review for our 4th quarter, which began April 1st and will continue until June 30th, 2021. We should have this review completed by the end of this quarter at the latest. Please feel free to let us know if you have any additional questions or feel free to review our current security readiness reviews SOP.

Thank you!
If there is anything we should do on our side to help make it go faster please let me know.

Thanks. Greatly appreciated <3

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: Medium. See a public-facing summary of the WMF's risk management policy here: 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: 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 High Risk.

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

Security Headers
Similar to query.wikidata.org, the toolforge.com test site is missing a handful of desired security headers per this automated report. 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: Low, given extenuating circumstances.

DoS Vectors (including 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 from the Performance-Team prior to deployment (I did not see a Phab task for such a review after a quick search). Risk: 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 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 (@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: 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 None.

Thank you so much, @sbassett. We'll have a look.

Created T285761: Add proper security headers to Query Builder for headers.

Does T276366: Wikidata Query Builder: replace vue-cli with vite and webpack with rollup mitigate the medium security risk in packaging? If so, we can prioritize it.

Regarding performance review, I want to mention this will be on wikidata.org but a separate, statically served site (basically something like https://security.wikimedia.org/) and won't have any interaction with mediawiki (beside being in the same high level DNS domain). Do we still need to get performance review for it?

In T264822#7183569, @Ladsgroup wrote:

Created T285761: Add proper security headers to Query Builder for headers.

Sounds good. The defaults for service-template-node would likely be a good baseline to model.

Does T276366: Wikidata Query Builder: replace vue-cli with vite and webpack with rollup mitigate the medium security risk in packaging? If so, we can prioritize it.

Yes! I believe rollup has become somewhat agreed-upon as a less risky alternative to webpack.

Regarding performance review, I want to mention this will be on wikidata.org but a separate, statically served site (basically something like https://security.wikimedia.org/) and won't have any interaction with mediawiki (beside being in the same high level DNS domain). Do we still need to get performance review for it?

Ok, I just meant that it's something that would be hosted under a production TLD, as stated: "We intend to deploy it as a subpage of the existing Wikidata Query Service at query.wikidata.org". A perf review is never required for any production deployment, AIUI, but is strongly recommended in many cases. Again, I'd recommend asking the Performance-Team if they feel it would be a good idea to perform such a review for this codebase, largely as a way to surface any potential DoS-related issues.

We migrated to vite/rollup and here is the build patch for review: https://gerrit.wikimedia.org/r/c/wikidata/query-builder/deploy/+/708629

I'm about to create a performance review ticket now.

Performance review: T287769: Performance review of Query Builder

In T264822#7245462, @Ladsgroup wrote:

We migrated to vite/rollup and here is the build patch for review: https://gerrit.wikimedia.org/r/c/wikidata/query-builder/deploy/+/708629

I'm about to create a performance review ticket now.

This is done. And given that we now migrated to vite/rollup, does that improve the security risk? If so, can this be reflated somewhere? :D

In T264822#7269255, @Ladsgroup wrote:

This is done. And given that we now migrated to vite/rollup, does that improve the security risk? If so, can this be reflated somewhere? :D

That is the hope, yes, though both of those are still technically in security review this quarter (T284341, T284338)

Just to record it, as checked just now, with the current HEAD of the master branch, npm audit finds 0 vulnerabilities.

/me is really happy that we have made the migration to vite/rollup :)

In T264822#7270301, @Michael wrote:

Just to record it, as checked just now, with the current HEAD of the master branch, npm audit finds 0 vulnerabilities.

I arrived at the same result. Given that webpack/dev npm dependecies were the most substantial risks found during my security audit, I am now fine assigning an overall low risk for Wikidata Query Builder, which is automatically accepted.

AWESOOOME

\o/ \o/ \o/