@Niedzielski - thanks for the update regarding the release candidate. Do we have any better sense of when the actual Vue3 release might occur? I'd prefer we wait to begin this review until an official release of Vue3 just in case there are major bug/security fixes from the release candidates.

(I see they just released rc.5 yesterday)

@sbassett, I've not heard anything new. Just "early August."

@Niedzielski - I see rc.9 was just released. Are you aware of any updates as to when an initial non-rc release of Vue3 might occur? I'm guessing we'll have to reset expectations on the timeline of August 30th in the task description as that cannot really happen at this point.

Are you aware of any updates as to when an initial non-rc release of Vue3 might occur?

@sbassett, no, I've not heard anything.

I'm guessing we'll have to reset expectations on the timeline of August 30th in the task description as that cannot really happen at this point.

Makes sense. The timeline should probably say "as soon as available."

Thanks for the update, @VulpesVulpes825.

@Volker_E @Jdlrobson - as the ostensible current requesters of this review, is there anything on your or your teams' end blocking this review? The Security-Team would like to attempt to complete this review this quarter (Q4 2021), if possible. Also - this will not be a line-by-line code review of Vue3, but more of a vendor review focused upon higher-level security models and best practices.

@Volker_E @Jdlrobson Please respond to our previous message by April 20 or we will need to move this to our backlog. Thanks!

Hey there I am not the right person to answer this right now. @Catrope should probably take over this request along with Volker instead of me. As I understand it they are overseeing the move to Vue 3.

cc @egardner

Nothing from web team should be blocking this review .

Since it looks like Vue is never going to support IE11 in Vue 3, and instead backport some Vue 3 features to a new Vue 2.7 release (that's what's currently being proposed, and it seems likely to happen), we don't think that any WMF code will likely migrate to Vue 3 for at least a year or more. I do think we'll likely upgrade to 2.7 once it's out, and we may want to use the composition API plugin before then, to get some of those 2.7 features early (the proposal calls for this plugin to be made part of Vue itself in 2.7). So I think a security readiness review of Vue 3 can be postponed for now, and we'll probably ask for a review of Vue 2.7 when it comes out. We may also ask for a review of the composition API plugin for 2.6, I'll ask the other Vue migration team members about that.

Thanks @sbassett! I asked the other engineers on the Vue team, and it sounds like we do plan on using the composition API plugin soon (which is a plugin for Vue 2.6 that essentially adds a subset of the anticipated 2.7 features). I've opened T281527: Security Readiness Review For Vue composition API plugin to request a review of that plugin. I'll link this task there too for context. We'll plan to reopen this task when Vue 2.7 comes out.

@Catrope - is there an updated deployment date for this and Vuex 4? End of September might be possible for us to turn these around, although both this and the Vuex 4 review technically missed our quarterly planning window, per our SOP.

I apologize for the late notice; the decision to move to Vue 3 sooner that previously planned was made at the Vue developer summit (Aug 10-12), and I updated this task as soon as I could. We don't have a solid deployment date yet, and we probably won't have more clarity on our plans until about a week from now, but for now my gut feeling is that we'd probably want to target late September or early October. Vue 3 is the big important blocker, Vuex 4 can wait, if that helps you prioritize.

@Catrope

In T257734#7293809, @Catrope wrote:

Vue 3 is the big important blocker, Vuex 4 can wait, if that helps you prioritize.

Yes, I think Vuex 4 (T288768) will likely need to wait until next quarter. We should be able to accommodate this (Vue 3) review by the end of September.

Thanks @sbassett! (And apologies for the late response, I started my vacation right when you posted your comment.) Pushing back Vuex 4 to the next quarter is fine. We're also encountering issues with Vue 3 (even in compatibility mode) breaking some of our code in local testing, so we're still working on that, and that may push back the date that we need this by to early October anyway.

@Catrope - I'm hopeful to keep working on this through this week, but the review likely won't be complete until the end of next week. I assume that's likely ok and wouldn't block any kind of launch at this time?

In T257734#7380540, @sbassett wrote:

@Catrope - I'm hopeful to keep working on this through this week, but the review likely won't be complete until the end of next week. I assume that's likely ok and wouldn't block any kind of launch at this time?

Yes, that's OK. I'm still having "fun" figuring out how to install all the MW extensions that use Vue and testing that the Vue 3 compat build works with them (in practice, most code has needed some tweaks to make it work). I'm also waiting for upstream to merge https://github.com/vuejs/vue-next/pull/4647 and do a release, we need that to fix a serious bug that we encountered in testing.

@sbassett I'd like to request one additional item for inclusion here if you're amenable – petite-vue, which is a "distribution" of Vue 3 optimized for progressive-enhancement usage. The library is really just a wrapper around Vue 3's reactivity module, so I'm hoping that including it here is ok. It is maintained by Evan You, the leader of the Vue.js project, and is deliberately minimal in scope. More info in the README.

We are considering using this subset of Vue in order to provide a set of UI components that can be used in places where shipping the whole Vue.js runtime + full component library aren't feasible (above the fold on article pages for logged-out users, for example); see T291666 and related tasks for more on our anticipated usage.

In T257734#7380540, @sbassett wrote:

@Catrope - I'm hopeful to keep working on this through this week, but the review likely won't be complete until the end of next week. I assume that's likely ok and wouldn't block any kind of launch at this time?

Yes, that's fine. We still need to resolve some blockers before we can deploy Vue 3, see T289019: Test Vue 3 migration build with extensions/skins using Vue, and that will probably take a few more weeks. But as soon as those extension tests and fixes are done we would like to be able to deploy, and that would require this security review to be done.

In T257734#7483248, @Catrope wrote:

Yes, that's fine. We still need to resolve some blockers before we can deploy Vue 3, see T289019: Test Vue 3 migration build with extensions/skins using Vue, and that will probably take a few more weeks. But as soon as those extension tests and fixes are done we would like to be able to deploy, and that would require this security review to be done.

The Security-Team is meeting to discuss this review and T288768 tomorrow and we hope to sprint on reviews for both of these over the next two weeks. I'm hopeful that can work with your deployment timeline.

@Catrope - quick update here - I hope to post this review by next Tuesday, November 30th, 2021. And the Security-Team also still plans to have the Vuex4 review (T288768) completed by the end of the quarter, December 31st, 2021.

Quick update - this is looking more like the end of this week due to some competing priorities. Apologies for the delays.

Security Review Summary - T257734 - 2021-12-02

Overall, Vue3 (aka Vue-next) v3.2.21 (as is currently proposed to be merged within Ibd61876) looks to be in good shape as an external dependency. The following application security review has determined it to have an overall risk rating of: low pending a couple of mitigations described below.

Note: this review specifically looked at the Vue3 source code and did not dive deeply into the myriad other ecosystem components, related third-party code or other applications which feature Vue3 as a dependency.

Vendor App/Package/Utility 1

General Security Information

Vulnerable Packages - Production

None found via pnpm audit, risk: low

Vulnerable Packages - Development

As reported via pnpm audit:

As reported by scan's dependency... scanning:

Overall risk for these dependencies (as they are dev dependencies) is low.

Outdated Packages

As reported via pnpm outdated:

(no explicit vulnerabilities reported, simply noting for completeness' sake.)

Other Vulnerable Code
I had a look through snyk's vuln database, Mitre's CVE database, NVD's vuln database and osv.dev's vuln database and found no current, known vulnerabilities for Vue3. snyk had a few issues with older 2.x versions of Vue, but these should be fixed or irrelevant in Vue3. There also do not appear to be any current security advisories published under Vue's canonical repo at Github nor are there any obviously security-impacting public issues, at least from a cursory review of said issues.

Risk: low.

Static Analysis Findings

1. lockfile-lint 4.6.2 returned no results. low risk
2. git secrets, as run with default AWS patterns and rudimentary secret keyword rule sets, returned only false positives. low risk
3. gitleaks returned no results. low risk
4. whispers 1.5.3 returned only MINOR, non-issue results. low risk
5. scan scan:latest returned one critical dev sub-dependency, see list above. low risk
6. semgrep 0.75.0 which returned a number of results for various rule sets:
   1. p/javascript: The vast majority of the issues found by this rule set were for lower-risk prototype pollution and object injection via bracket notation. A few potentially insecure eval issues were found within packages/runtime-core/__tests__/hmr.spec.ts, packages/compiler-dom/src/transforms/stringifyStatic.ts and packages/compiler-core/src/validateExpression.ts along with some potental re-dos issues within packages/compiler-ssr/src/transforms/ssrTransformElement.ts and packages/compiler-core/src/validateExpression.ts. For now, I would rate these results as a medium risk. Please review this supplemental report of the findings for further details:

1. p/typescript: This rule set produced the most concerning issues, namely a handful of sinks that permit unsanitized, user-controlled data. But given the nature of what Vue3 does as a JavaScript component framework, this architecture is a necessity. The mitigation here is to review Vue's Security Model documentation (as noted within the succeeding section below) and incorporate relevant best practices into Wikimedia developer documentation and hopefully, eventually, automate checks of anti-patterns in tests/CI. For now, I would rate these results as a High risk, without the described mitigations in place. Please review this supplemental report of the findings for further details:

1. p/nodejscan: The most egregious findings for this rule set were a few potential regex re-dos issues within some of packages/compiler-dom's code. I would rate these as a medium risk for now. Please review this supplemental report of the findings for further details:

1. 1. r/generic.unicode.security.bidi.contains-bidirectional-characters: bidi/trojan source detection, no results found. low risk
   2. p/secrets returned no results. low risk
2. ossf scorecards (via their docker image) found a score of 6.3/10 for vue-next/master. Since this falls in the middle of their defined range, I'm going to rate it as a medium risk, with the obvious caveat that there are several limitations with tools like these. However such tools cover similar checks (and provide another lens of analysis) to the Security-Team's existing third party code review checklist. Please review this supplemental report of the findings for further details:

DoS Vectors
I was not able to complete any exhaustive performance-testing for Vue3, but did not see anything within the code that seemed immediately concerning within both the aforementioned static analysis results and some manual code-review. Of course I would be remiss in failing to note that performance-testing a JavaScript framework like Vue3 can be extremely difficult and filled with uncertainty, especially when tested outside of production-like environments. I did not find a performance review request for Vue3 within Phabricator, but I would recommend a consultation with the Performance-Team to solicit their opinion on the matter.

Vue's Security Model
Vue has provided a security best practices section within their primary documentation, which has been updated for Vue3. The key sections here are what Vue3 does as a default to protect against potentially-exploitable code (namely its handling of HTML content and attribute bindings) and how developers should best handle issues related to the injection of HTML, URLs, JavaScript and styles. If these ideas are incorporated into Wikimedia Vue3 developer documentation and eventually enforced via lints, tests, etc. that would mitigate some of the static analysis findings related to injection within the previous section of this review to being a low risk.

Thank you!

We already have a lint rule prohibiting the use of v-html. We intend to write developer documentation about this too, in which we will tell people to avoid using v-html except when absolutely necessary (which requires an explicit <!-- eslint-disable-next-line vue/no-v-html --> comment to bypass the lint rule). We think the most common use case for v-htmlwill be to parse an i18n message, so we've created our own v-i18n-html directive to simplify v-html="mw.message( 'foobar' ).params( [ ... ] ).parse()" to v-i18n-html:foobar="[ ... ]". This is easier to use for developers, doesn't raise a lint warning, and ensures the HTML generation takes place in trusted, security-reviewed code in the i18n system. Our current documentation explains this directive but doesn't explain that v-html should be avoided. We will need to update that.

We currently don't have lint rules to enforce the rest of the Vue security guidelines. We could, for example, raise a lint warning every time a dynamic href attribute is used, but I think that would cause a lot of false positives, because hrefs are commonly generated using safe functions like mw.util.getUrl(). I don't think URLs lend themselves to a you're-only-allowed-to-build-them-using-a-safe-function approach quite as well as parsed messages do, but we could reconsider that. Regardless, we will link to the Vue security guidelines from our documentation (these guidelines are new in Vue 3 as far as I know, so I will plan to do this as part of updating our documentation for the Vue 3 migration generally).

I'll review the supplemental reports and then comment about them separately.

In T257734#7547334, @sbassett wrote:

1. p/javascript: The vast majority of the issues found by this rule set were for lower-risk prototype pollution and object injection via bracket notation. A few potentially insecure eval issues were found within packages/runtime-core/__tests__/hmr.spec.ts, packages/compiler-dom/src/transforms/stringifyStatic.ts and packages/compiler-core/src/validateExpression.ts along with some potental re-dos issues within packages/compiler-ssr/src/transforms/ssrTransformElement.ts and packages/compiler-core/src/validateExpression.ts. For now, I would rate these results as a medium risk. Please review this supplemental report of the findings for further details:

vue3-2021-semgrep-p-javascript.txt62 KBDownload

.

The "object injection via bracket notation" warnings are difficult to review because there are hundreds of them, and a large proportion of them appear to be false positives where brackets are used to index arrays with numerical indexes (e.g. node.children[i]). I don't know if there's a TypeScript-aware version of this check that only flags cases where the expression in the brackets is a string as opposed to a number, but if that exists that would be helpful.

I did review the evals manually because there are only a few of them. All of the dynamic eval calls (outside of test files) are either on code generated by the template compiler, or on expressions in a template that are intended as code. Technically these evals execute inputs to the library, but the nature of Vue as a framework is that part of its input is code, so I think that's unavoidable.

1. p/typescript: This rule set produced the most concerning issues, namely a handful of sinks that permit unsanitized, user-controlled data. But given the nature of what Vue3 does as a JavaScript component framework, this architecture is a necessity. The mitigation here is to review Vue's Security Model documentation (as noted within the succeeding section below) and incorporate relevant best practices into Wikimedia developer documentation and hopefully, eventually, automate checks of anti-patterns in tests/CI. For now, I would rate these results as a High risk, without the described mitigations in place. Please review this supplemental report of the findings for further details:

vue3-2021-semgrep-p-typescript.txt4 KBDownload

.

Note that some of these are false positives because React-specific rules flag createRef() as unsafe, saying that "refs give direct DOM access". That might be true in React, but in Vue a "ref" is a different concept that doesn't give direct DOM access and isn't an output sink. Disregarding those and the ones in test files, that leaves only two cases, and as you say those are both necessary due to Vue's nature as a framework. One of them has an explanatory comment in the Vue source code acknowledging that the innerHTML usage is unsafe, but that it only operates on templates, and that it's the caller's responsibility not to pass in untrusted templates, since templates are essentially code. As you say, the mitigation for this should be to treat templates as code, and to institute security practices for them.

1. p/nodejscan: The most egregious findings for this rule set were a few potential regex re-dos issues within some of packages/compiler-dom's code. I would rate these as a medium risk for now. Please review this supplemental report of the findings for further details:

vue3-2021-semgrep-p-nodejsscant.txt5 KBDownload

.

I've reviewed these regexes, and they're all very simple ones that don't have the kind of complexity that would cause performance or re-dos problems. For a regex like /^(data|aria)-/ there is no pathological or malicious input you could give it that would cause it to be any slower. I reviewed all the regexes in the supplemental report, and all but one of them are very simple like that. There's only one that isn't (here), and after reviewing it I'm satisfied that it isn't susceptible to catastrophic backtracking either.

1. ossf scorecards (via their docker image) found a score of 6.3/10 for vue-next/master. Since this falls in the middle of their defined range, I'm going to rate it as a medium risk, with the obvious caveat that there are several limitations with tools like these. However such tools cover similar checks (and provide another lens of analysis) to the Security-Team's existing third party code review checklist. Please review this supplemental report of the findings for further details:

vue3-2021-scorecards.txt55 KBDownload

.

I think this tool might have been a little confused by the monorepo setup that Vue uses (where multiple packages are maintained in the same repo), since it means that the package.json file at the root of the git repo doesn't correspond to an npm package. However, some of the criticisms the tool makes are fair. For example, Vue does not appear to have a practice of universal code review, as I've seen members of the core team push changes directly without review, not just bug fixes but also bigger changes.

In T257734#7551885, @Catrope wrote:

We already have a lint rule prohibiting the use of v-html. We intend to write developer documentation about this too, in which we will tell people to avoid using v-html except when absolutely necessary (which requires an explicit <!-- eslint-disable-next-line vue/no-v-html --> comment to bypass the lint rule).

Excellent.

Our current documentation explains this directive but doesn't explain that v-html should be avoided. We will need to update that.

Sounds good.

We could, for example, raise a lint warning every time a dynamic href attribute is used, but I think that would cause a lot of false positives, because hrefs are commonly generated using safe functions like mw.util.getUrl().

Likely, yes. For any security-related linting/static analysis rules like this, it's important to find a balance between things that are obviously bad and should be flagged always versus things that are likely very low-risk or even non-issues which just create a lot of noise.

Regardless, we will link to the Vue security guidelines from our documentation (these guidelines are new in Vue 3 as far as I know, so I will plan to do this as part of updating our documentation for the Vue 3 migration generally).

Great.

The "object injection via bracket notation" warnings are difficult to review because there are hundreds of them, and a large proportion of them appear to be false positives where brackets are used to index arrays with numerical indexes (e.g. node.children[i])...

Yes, these are definitely low-risk IMO and could likely even be excluded rules from future semgrep scans, and likely will be once we implement it as a security CI tool within Gitlab.

I did review the evals manually because there are only a few of them. All of the dynamic eval calls (outside of test files) are either on code generated by the template compiler, or on expressions in a template that are intended as code. Technically these evals execute inputs to the library, but the nature of Vue as a framework is that part of its input is code, so I think that's unavoidable.

Ok, thanks.

...As you say, the mitigation for this should be to treat templates as code, and to institute security practices for them.

Yes, thanks.

I've reviewed these regexes, and they're all very simple ones that don't have the kind of complexity that would cause performance or re-dos problems. For a regex like /^(data|aria)-/ there is no pathological or malicious input you could give it that would cause it to be any slower. I reviewed all the regexes in the supplemental report, and all but one of them are very simple like that. There's only one that isn't (here), and after reviewing it I'm satisfied that it isn't susceptible to catastrophic backtracking either.

Great, thanks.

So again, the overall risk rating of low means the risk is automatically accepted by the WMF, per our risk management framework. If you have any other questions or concerns, feel free to let me know.

Update: the documentation has been updated to discourage use of v-html; to remove v-html from code examples; to encourage use of v-i18n-html; and to document the latter better. See https://www.mediawiki.org/wiki/Manual:Coding_conventions/Vue#Template and https://www.mediawiki.org/wiki/Vue.js#Internationalization

In T257734#7615302, @Catrope wrote:

Update: the documentation has been updated to discourage use of v-html; to remove v-html from code examples; to encourage use of v-i18n-html; and to document the latter better. See https://www.mediawiki.org/wiki/Manual:Coding_conventions/Vue#Template and https://www.mediawiki.org/wiki/Vue.js#Internationalization

Looks good, thanks. I especially like the idea of it being required for a developer to override the v-html ESLint error, thus acknowledging and documenting their confirmation and acceptance of the risk.

Also, while I believe many of the issues concerning this still-protected bug - T251032 - have been addressed via other fixes, there are still some lingering issues around the safety of using that particular function.