Hi @Pginer-WMF - The security team took a look at this today and would like to request instructions for producing a local test environment as we won't be able to test on beta. We've noted your target deployment date and will be in touch with any additional questions. Thanks!

^ to follow up: I think we'd be looking for something a bit beyond the basic installation instructions, whether that's a docker or similar that works out of the box with minimal effort and no missing pieces (elevated rights, credentials, dependencies, etc). If the installation instructions can accommodate this (have not tried them yet), that's fine.

In T260236#6380074, @sbassett wrote:

^ to follow up: I think we'd be looking for something a bit beyond the basic installation instructions, whether that's a docker or similar that works out of the box with minimal effort and no missing pieces (elevated rights, credentials, dependencies, etc). If the installation instructions can accommodate this (have not tried them yet), that's fine.

A docker based setup is documented in detail at https://www.mediawiki.org/wiki/MediaWiki-Docker/Extension/ContentTranslation

As we reach mid-October I wanted to check the status on this in order to start planning the deployment process (communication with the community, etc.):

• Is the information available in the ticket (and the link to documentation Santhosh provided) enough for the review?
• Do you have a time estimation for the review? Is there any blocker we can help with? We can adapt our deployment plans if needed.

Thanks!

@Pginer-WMF - this review is technically in progress. Is there a specific date you're targeting for deployment in October?

In T260236#6542460, @sbassett wrote:

@Pginer-WMF - this review is technically in progress. Is there a specific date you're targeting for deployment in October?

After checking with developers, they will still be developing key parts of the code that are worth including in the security review process during the upcoming weeks. They expect the code to be ready to be reviewed by the end of November. Thus, considering testing and community communications, deployment is expected by the end of December.

If that timeline does not provide enough room for the security review, we can adjust. We'll continue to add updates to the ticket in case of delays (or earlier completions).

Sounds good. thanks, @sbassett

@Pginer-WMF - I just wanted to check in and see what the status is for this work. Do the next couple of weeks still seem like the correct timeline for your team to complete the necessary work for the security review?

Yes, we are almost getting ready, fixing couple of major bugs. After that we will make sure you have correct version of code to test and make it available in deployed state at test server too. We will update it here.

@santhosh @Pginer-WMF - I'm not entirely sure how aware either of you are with the ongoing discussions regarding the adoption and usage of Vue, particularly around using vue-cli in conjunction with Webpack for any build steps, but the Security-Team would likely rate this as a high or critical risk as part of this security readiness review. This wouldn't be a hard blocker for production deployment, per se, but would require c-level risk acceptance prior to deployment, per our current risk management policy and as publicly summarized here: T249039#6309061.

@sbassett, yes I was part of those dicussions. We discussed this specific topic too. Alternatives to webpack is discussed and I think everybody is on same page that we need a bundler with relatively less risk profile(Rollup was one candidate). Language team will definitely switch to such bundlers once we decide it. Our code does not use any webpack plugins other than the ones coming with default configurations, so it should be trivial to change to bundler to something else. As per the vue task force discussions, it was observed that the current plan with section translation need not be blocked for this, but definitely an item to explore and fix without much delay(see vue task force meeting minutes, that I believe you have access).

Hi @sbasset, We are ready for the security review.

• We have completed the MVP of section translation, available at https://sx.wmflabs.org . You may need to create an account and then use some of the links given in main page of that wiki to try Section Translation. The application is responsive, but optimized for smaller screens.
• The technical details are documented at https://www.mediawiki.org/wiki/Content_translation/CX3. That page also has an FAQ outlining rationale behind some of the engineering decisions.
• A full local setup may use our docker setup, documented at https://www.mediawiki.org/wiki/MediaWiki-Docker/Extension/ContentTranslation
• The relevant source code for this feature is with in the app folder of Content translation source code repository https://github.com/wikimedia/mediawiki-extensions-ContentTranslation/tree/master/app/
• Later this year or early next year, we may deploy this in a smaller wikipedia for further evaluation. This will be hidden using configuration switch in all other wikis.
• The functionality of section translation is limited to Special:ContentTranslation special page when URL params are present.

@santhosh - thanks for the information, I'll got this in my in-progress column for now and should hopefully have the review completed within the next couple of weeks.

@santhosh et al - Apologies for the extreme delay on this review - it's been a busy quarter. Anyhow, I hope to have a look at this over the next couple of days, but due to the holiday break, this likely won't be completed until the first week or so of January 2021. Again, apologies for the delay.

@sbassett Happy New year! Language team wanted to check on this to see if early January was still the plan. Thank you!

In T260236#6731852, @Rileych wrote:

@sbassett Happy New year! Language team wanted to check on this to see if early January was still the plan. Thank you!

Yes. Likely not this week, but hopefully next.

Update: I won't be able to complete the review this week; I'm very hopeful to have it completed sometime by the middle of next week.

Hey hey @sbassett! Any news on this review? Thank you!

@Rileych - should be completed EOD (US CST) today. So far I have not found any major issues.

Security Review Summary - T260236 - 2021-01-22
Last commit reviewed: c5e9fb30452b6c605ea2d307fbc614b3ee03c3cb

Summary

Overall, the current Section Translation code (within the app directory) looks fairly secure with certain issues outlined below. I would currently rate the overall risk as: medium.

Vulnerable Packages - Production

None: as verified with auditjs (output below), snyk and npm audit. Still, I'd note that these dependencies add an additional 122,704 lines of code to Section Translation's codebase, thus dramatically increasing complexity and potential future risk. And with dev dependencies, that figure becomes 6,164,851 lines of code.

Vulnerable Packages - Development

Risk: Low

Via npm audit:

Via snyk:

Outdated Packages

Risk: Low

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

Static Analysis Findings
Both njsscan and vue-cli-service produced warnings around using vue's v-html directive within the vue template files below. While I'm guessing that these have been reviewed and are necessary for the proper functionality of the Section Translation app (and any relevant data is likely being processed by the PHP Parser or Parsoid), I wanted to enumerate them here so that code owners could confirm they are safe sinks. Given my previous assumptions, I'm going to rate these issues as low risk for now.

1. src/components/SXContentComparator/NewSectionPlaceholder.vue - line 5 - v-html="placeholderTitle"
2. src/components/SXSentenceSelector/SXTranslationSelector.vue - line 44 - v-html="proposedTranslations[originalTextProviderKey]"
3. src/components/SXSentenceSelector/SXTranslationSelector.vue - line 59 - v-html="proposedTranslations[mtProvider]"
4. src/components/SXQuickTutorial/SXQuickTutorial.vue - line 30 - v-html="tutorialSvgSections"
5. src/components/SXQuickTutorial/SXQuickTutorial.vue - line 25 - v-html="tutorialSvgMT"
6. src/lib/mediawiki.ui/components/MWDialog/MWDialog.vue - line 23 - v-html="title"
7. src/components/SXEditor/SXEditorOriginalContent.vue - line 15 - v-html="originalContent"
8. src/components/SXSentenceSelector/ProposedTranslationCard.vue - line 12 - v-html="proposedTranslation"
9. src/components/SXContentComparator/SXContentComparator.vue - line 22 - v-html="targetPageContent"
10. src/components/SXContentComparator/SXContentComparator.vue - line 18 - v-html="sourceSectionContent"
11. src/components/SXContentComparator/SXContentComparator.vue - line 28 - v-html="targetSectionContent"
12. src/components/SXPublisher/SXPublisher.vue - line 9 - v-html="panelResult"
13. src/components/SXPublisher/SXPublisher.vue - line 34 - v-html="currentPageSection.translationHtml"
14. src/components/SXPublisher/SXPublisherAnimationDialog.vue - line 12 - v-html="animationSvg"
15. src/components/SXSentenceSelector/SubSection.vue - line 1 - v-html="content"
16. src/components/SXSectionSelector/SXSectionSelectorSectionListMissing.vue - line 22 - v-html="sadRobotSVG"
17. src/components/SXSentenceSelector/SXSentenceSelectorSentence.vue - line 6 - v-html="sentence.content"
18. /src/components/SXPublisher/SXPublisherReviewInfo.vue - line 16 - v-i18n-html:cx-sx-publisher-review-info
19. /src/components/SXQuickTutorial/SXQuickTutorial.vue - lines 49, 55 - v-i18n-html:sx-quick-tutorial-secondary-point-step-1|2
20. /src/components/SXArticleSelector/SXArticleSelector.vue - line 9 - v-i18n-html:cx-license-agreement
21. /src/components/SXContentComparator/SXContentComparatorHeaderMappedSection.vue - lines 44, 49 - v-i18n-html:cx-sx-content-comparator-mapped-section-clarifications

HTTP Leaks

Build/Test steps

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

Great! Thanks, @sbassett

thank you @sbassett for this review! From my side I'd like to confirm that v-html directive is always used in a safe context in our application, as we only use it with content strictly generated on our side. Regarding lean to external url for the image inside MWCard.stories.js, I'd like to notice that this is never used in production, it is rather part of our UI library storybook documentation and examples. At this point I would also like to refer that vue-banana-i18n exposes v-i18n-html directive which does not use proper escaping/sanitization for html strings, leading to XSS vulnerabilities. This directive is used on master at a few points, but this is fixed by replacing it with v-i18n-html-safe directive, that uses mw.message.parse() under the hood to properly sanitize HTML localisation messages. Related patch is currently under review and will be merged to master soon.

In T260236#6773315, @ngkountas wrote:

From my side I'd like to confirm that v-html directive is always used in a safe context in our application, as we only use it with content strictly generated on our side.

Great, thanks for confirming this. I may pen-test sx.wmflabs.org a bit more to further confirm this.

Regarding lean to external url for the image inside MWCard.stories.js, I'd like to notice that this is never used in production, it is rather part of our UI library storybook documentation and examples.

Ok, thank you for confirming this.

At this point I would also like to refer that vue-banana-i18n exposes v-i18n-html directive which does not use proper escaping/sanitization for html strings, leading to XSS vulnerabilities. This directive is used on master at a few points, but this is fixed by replacing it with v-i18n-html-safe directive, that uses mw.message.parse() under the hood to properly sanitize HTML localisation messages. Related patch is currently under review and will be merged to master soon.

Ah, good catch. I'll add that to my checks and update the above review. I've also left a couple of comments on the gerrit change set.

@Pginer-WMF @ngkountas - just wanted to wrap up a few loose ends here:

1. The potential http leak I originally reported can be considered low risk if it never makes it to Wikimedia production. I'm curious though if there are any plans for the storybook doc to end up on doc.wikimedia.org (or similar). I know there's VE doc up there, but did not see any for CX/SX.
2. I know the patch was merged before I got there, but let me know if there were any questions regarding my comments (1, 2). There is a potential issue noted within my second comment.
3. After testing a decent number of XSS payloads against specific UIs within SX, particularly the actual editing step-throughs, I noticed that the app sometimes struggled with performance even after fairly small amounts of text (100 Kb or less) were pasted into the various editing interfaces. This would often lock up the app's controls and eventually result in "page unresponsive" browser warnings for me (Chrome 88.0.4324.96 under MacOS 10.14.6). I'm not all that concerned about self- Vuln-DoS' or if this is merely an issue with wmflabs resources, but I wanted to note this experience in case there may be additional underlying performance issues, either within the SX app or its dependencies.
4. Given the above clarifications and mitigations, I would still rate the overall risk of SX to be medium, due the vue/webpack-based build step. As you may or may not know, the Security-Team and other advocates are trying to encourage use of alternative js build tools (Rollup, etc.) as opposed to Webpack. Medium risk requires managerial risk acceptance per current risk acceptance policy (see: T249039#6309061).

@sbassett Thank you a lot for your answers!

In T260236#6788237, @sbassett wrote:

1. The potential http leak I originally reported can be considered low risk if it never makes it to Wikimedia production. I'm curious though if there are any plans for the storybook doc to end up on doc.wikimedia.org (or similar). I know there's VE doc up there, but did not see any for CX/SX.

I believe that it is still unclear what it the future of our UI lib. I believe that it will eventually be merged into Wikimedia Vue UI. After all the reason that we built our own UI library was the fact that we were moving too fast relatively to Wikimedia Vue UI lib and we would be blocked if not creating our own.

In T260236#6788237, @sbassett wrote:

2. I know the patch was merged before I got there, but let me know if there were any questions regarding my comments (1, 2). There is a potential issue noted within my second comment.

Thank you for that Scott! I originally missed your comments. This is something that needs further investigation, please let me come back with further details after doing some digging.

In T260236#6788237, @sbassett wrote:

3. After testing a decent number of XSS payloads against specific UIs within SX, particularly the actual editing step-throughs, I noticed that the app sometimes struggled with performance even after fairly small amounts of text (100 Kb or less) were pasted into the various editing interfaces. This would often lock up the app's controls and eventually result in "page unresponsive" browser warnings for me (Chrome 88.0.4324.96 under MacOS 10.14.6). I'm not all that concerned about self- Vuln-DoS' or if this is merely an issue with wmflabs resources, but I wanted to note this experience in case there may be additional underlying performance issues, either within the SX app or its dependencies.

Good to know. Thanks!

In T260236#6788237, @sbassett wrote:

4. Given the above clarifications and mitigations, I would still rate the overall risk of SX to be medium, due the vue/webpack-based build step. As you may or may not know, the Security-Team and other advocates are trying to encourage use of alternative js build tools (Rollup, etc.) as opposed to Webpack. Medium risk requires managerial risk acceptance per current risk acceptance policy (see: T249039#6309061).

Indeed, we are aware about alternative js build tools and concerns about Webpack. I expect that this is something that we'll support at some point, as I remember Santhosh referring in Vue-related discussions with Vue task force, that switching to another bundler won't be an issue for us. I do not think we'll take some action in this front before Santhosh is back, though.

In T260236#6790822, @ngkountas wrote:

I believe that it is still unclear what it the future of our UI lib. I believe that it will eventually be merged into Wikimedia Vue UI. After all the reason that we built our own UI library was the fact that we were moving too fast relatively to Wikimedia Vue UI lib and we would be blocked if not creating our own.

Ok, I guess I meant specifically in regards to the potential http leak mentioned within the review. I just don't want the storybook doc to be hosted anywhere (now or in the future) on a wikimedia.org, etc. domain where that external image creates a privacy issue.

Thank you for that Scott! I originally missed your comments. This is something that needs further investigation, please let me come back with further details after doing some digging.

No problem. Feel free to file a protected Phab task if you'd like to track this issue.

Indeed, we are aware about alternative js build tools and concerns about Webpack. I expect that this is something that we'll support at some point, as I remember Santhosh referring in Vue-related discussions with Vue task force, that switching to another bundler won't be an issue for us. I do not think we'll take some action in this front before Santhosh is back, though.

That's fine, but I'm going to document this as a medium risk within our risk register for the time being, with the owners of said risk being @MNovotny_WMF and @Arrbee.

@sbassett I will be signing off on this. We have a risk mitigation plan in place now that has been vetted by @ngkountas , @Nikerabbit, @Pginer-WMF and me. We have this in a shared document. Do let us know if you think we should add the content (partly or entirely) in the ticket itself. Thanks.

@Arrbee et al - Great! I'll link the mitigation plan document within the relevant risk registry entry. Hopefully said registry will become a bit cleaner and feature better automation once it is migrated to our new GRC platform (Archer). But for now this is still a very manual process for the Security-Team. The mitigation plan the Language Team has put together looks good (thanks!) though I did want to provide feedback for a few items:

1. It would be great if @Reedy and myself could be added to any gerrit change sets which include unminified webpack artifacts and any other relevant JS code as was recently done for WVUI, pre-merge/pre-deployment.
2. For any vuln-checking/SAST/etc that may occur in CI/CD for SX and its dependencies, it would be great if those results could be 1) protected somewhere and/or 2) relevant change sets were not merged until any resultant risk was either mitigated (by bumping dependencies to known secure versions) or by formal risk acceptance. I understand the first suggestion is, at best, extremely inconvenient given gerrit and its current configuration.
3. I would imagine that SX could leverage some of the result of T272879, once completed, though I'm not sure what the timeline estimate is for that task.