@SBisson - We'll likely schedule this to be completed early next quarter. So Q4 2022 (April - June 2022).

@sbassett, we're working hard to build our features in preparation for the upcoming security review. Having the extension on the beta cluster would be of great help. Would it be possible to have a quick pre-review or sanity check of our approach so we can have your blessing to deploy to beta before the full review?

Hey @SBisson - per the latest extension deployment doc (item 4 specifically), we've tried to soften the requirements for beta deployments as we know those can be critical for testing/development before a codebase is ready for an application security review. If you have some gerrit change sets or a dev branch you'd like us to review, we can take a quick look at that prior to a beta deployment, but then we'll want to wait until the code is closer to production-ready before we conduct this review.

Hi @sbassett thanks for the quick response. You can have a look at the code in the master branch.

In a nutshell, this project is a fancy layer on top of regular pages with the JSON content model. Most of the code, current and future, is in Vuejs apps that present and manipulate the JSON data in those pages. We do allow user inputs but it's all treated as plain text (or maybe wikitext in the future) and we'll do our best to make it go through the same checks as all the wikitext currently flowing through our systems. I'm happy to give more information and answer any questions you may have in writing or in a meeting.

It would be great if you could chime in T303004: Deploy Wikistories extension to beta cluster to let everyone know when we can go ahead with beta deployment.

@SBisson -

In a nutshell, this project is a fancy layer on top of regular pages with the JSON content model. Most of the code, current and future, is in Vuejs apps that present and manipulate the JSON data in those pages. We do allow user inputs but it's all treated as plain text (or maybe wikitext in the future) and we'll do our best to make it go through the same checks as all the wikitext currently flowing through our systems. I'm happy to give more information and answer any questions you may have in writing or in a meeting.

This all sounds ok, on its face.

It would be great if you could chime in T303004: Deploy Wikistories extension to beta cluster to let everyone know when we can go ahead with beta deployment.

Did you or your team have a preferred date to get this onto beta? Unfortunately, we're right at the end of the quarter when there's typically a pretty aggressive sprint to tie up any loose ends for work from the current quarter. So unless this is an emergency, I'd say a quick security "pre-review" of sorts likely couldn't start for at least a week.

In T301389#7801136, @sbassett wrote:

[...]
Did you or your team have a preferred date to get this onto beta? Unfortunately, we're right at the end of the quarter when there's typically a pretty aggressive sprint to tie up any lose ends for work from the current quarter. So unless this is an emergency, I'd say a quick security "pre-review" of sorts likely couldn't start for at least a week.

We hope to be on beta by the end of the quarter but we understand we all have our own priorities and we'll gladly take your help whenever you can provide it.

@sbassett we're all ready for deployment, just waiting for your feedback. Let us know if you have any questions about the product or the code.

In T301389#7848040, @SBisson wrote:

@sbassett we're all ready for deployment, just waiting for your feedback. Let us know if you have any questions about the product or the code.

Ok, I can start the review process for this. Is e375c21aa7 fine to work from?

In T301389#7848268, @sbassett wrote:

In T301389#7848040, @SBisson wrote:

@sbassett we're all ready for deployment, just waiting for your feedback. Let us know if you have any questions about the product or the code.

Ok, I can start the review process for this. Is e375c21aa7 fine to work from?

Yes. Just too be clear, this is quick pre-review just to get to beta. We will want a full review for production deployment when we have more features in place.

Hi @sbassett, just checking how this is progressing and if you have any questions or issues we can help with.

In T301389#7864402, @SBisson wrote:

Hi @sbassett, just checking how this is progressing and if you have any questions or issues we can help with.

Hey @SBisson - I should have an initial review completed by this Thursday or Friday.

Security Review Summary - T301389 - 2022-04-21
Last commit reviewed: f1e66e3387

Pre-Review/Beta Deployment Summary

After a fairly cursory glance of the dependencies and code for ext:Wikistories, I would currently assign a low risk for a beta deployment.

Vulnerable Packages - Production

None, as there are no production dependencies.

Vulnerable Packages - Development

Via scan, which had the most complete and robust results:

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

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

Static Analysis Findings

1. lockfile-lint returned no results. low risk
2. gitleaks returned no results. low risk
3. whispers returned no results. low risk
4. scan scan:latest returned no results, outside of the previously-mentioned vulnerable packages. low risk
5. phan with the phan-taint-check plugin was run against the extension as installed with a recent verison of mediawiki core. No results were found. low risk
6. semgrep 0.89.0 was run with several popular and custom policies and rule-sets. Some lightly-curated results are found within P26128. There was nothing extremely serious within these results IMO (most appear to be false-ish positives), but it might be a good idea to verify the following:
   1. The various v-html directives are not vulnerable to injection via user-controlled data.
   2. There isn't anything immediately actionable within the results for vue-router.common.js that we might wish to report to upstream as security issues.

Thanks @sbassett! We will go ahead and schedule the extension deployment on the beta cluster. I already see some improvements such as reducing usage of v-html and upgrading npm dependencies we can make ahead of the full security review, which I hope to start in 2-3 weeks.

We were considering replacing vue-router with our own router implementation based on the assumption that getting this external library approved would be time consuming and/or risky. Writing our own router would also be time consuming and risky, and with the results of your initial review, I'm not so sure it's a good idea anymore. What do you think?

In T301389#7873837, @SBisson wrote:

Thanks @sbassett! We will go ahead and schedule the extension deployment on the beta cluster. I already see some improvements such as reducing usage of v-html and upgrading npm dependencies we can make ahead of the full security review, which I hope to start in 2-3 weeks.

Sounds good. Unless there are significant code changes, the "full" review likely won't look too much different than this one, at least in terms of the analyses performed.

We were considering replacing vue-router with our own router implementation based on the assumption that getting this external library approved would be time consuming and/or risky. Writing our own router would also be time consuming and risky, and with the results of your initial review, I'm not so sure it's a good idea anymore. What do you think?

It's always a trade-off. Leveraging upstream is often a good idea so as to not reinvent the wheel (and accidentally make it a hexagon in the process) and to (likely) absorb additional battle-testing from code that is used and reviewed more frequently than something your team would build. But if there are serious security flaws with upstream, those can result in high or critical risk that needs to be mitigated or accepted on our end. And getting security patches applied upstream can be a tricky or tedious process. But for now I'd recommend leveraging upstream and I can plan to review vue-router.common.js more thoroughly during the "full" review. Unless there are additional, compelling reasons for your team to build an ad-hoc replacement router.

Can I be added as a subscriber to P26128 please? Its currently private

In T301389#7918535, @DannyS712 wrote:

Can I be added as a subscriber to P26128 please? Its currently private

Done.

Change 791042 had a related patch set uploaded (by Sbisson; author: Sbisson):

[mediawiki/extensions/Wikistories@master] Update npm and composer dependencies

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

Change 791042 merged by jenkins-bot:

[mediawiki/extensions/Wikistories@master] Update npm and composer dependencies

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

Hi @sbassett , we're ready for the full security review.

We've addressed some of the early feedback (upgrade npm/composer dependencies, remove all v-html that are not absolutely necessary) .We've decided to keep the vue-router for now but we know we will have to upgrade it in the future so we'll reevaluate at that point if we want to get rid of it.

Our target deployment to production date is about a month from now. Let me know by when you think the full review can be completed and if you have any other questions.

In T301389#7924583, @SBisson wrote:

Hi @sbassett , we're ready for the full security review.

We've addressed some of the early feedback (upgrade npm/composer dependencies, remove all v-html that are not absolutely necessary) .We've decided to keep the vue-router for now but we know we will have to upgrade it in the future so we'll reevaluate at that point if we want to get rid of it.

Our target deployment to production date is about a month from now. Let me know by when you think the full review can be completed and if you have any other questions.

Ok, sounds good. I see the above couple of patches, related to the first round of security findings, were merged. Are there any other outstanding changes that will land soon? If not, I can go ahead and begin the re-review.

In T301389#7932611, @sbassett wrote:

[...] Are there any other outstanding changes that will land soon? If not, I can go ahead and begin the re-review.

Development will continue for another month before going to production and a few more months after but we believe all the major features are in and only minor and UI only changes are outstanding. I think you can go ahead with the full review now.

Hi @sbassett, please let us know when you have an estimated completion date on this or if you have any questions about the code or the product. Thanks!

@SBisson - Hoping for the end of this week or early next.

Security Review Summary - T301389 - 2022-06-16
Last commit reviewed: 96c13a2676

Re-review Summary

Overall, not much has changed from my pre-review above. Most of the vulnerable package and static analysis findings are low-risk or non-issues IMO. I did find a few more, likely actionable issues via semgrep - see below. And I wouldn't mind performing some additional pentesting of the extension on beta, if possible - see my note below. I'd still classify the current state of the extension as having an overall risk rating of: low, for now.

Vulnerable Packages - Production

None, as there are no production dependencies. Overall risk: none.

Vulnerable Packages - Development

Via scan.sh. These are all dev or indirect dependencies and scan.sh recommends that no action is needed. So overall risk for these is low, especially given that no other tools (npm audit, auditjs, snyk, etc.) returned any actionable vulnerabilities.

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

As reported via composer outdated:
(no explicit vulnerabilities reported, simply noting for completeness' sake.)
Overall risk: low

Static Analysis Findings

1. lockfile-lint returned no results. low risk
2. gitleaks returned no results. low risk
3. whispers returned no results. low risk
4. scan scan:latest returned no results, outside of the previously-mentioned vulnerable packages. low risk
5. phan with the phan-taint-check plugin was run against the extension as installed with a recent verison of mediawiki core. No results were found. low risk
6. semgrep 0.98.0 was run with several popular and custom policies and rule-sets. I've posted an updated paste of the results here: P29905. I'm going to rate the below issues as medium risk now, since a few other items turned up and I've had to look at a few more of them a bit closer:
   1. The span.innerHTML = a.innerHTML; assignment within resources/mw.ext.wikistories.builder/store/article.js does seem potentially vulnerable if a malicious user can control the a elements on a relevant page or if they are generated from user-input, which should be confirmed or disconfirmed. If they can be manipulated, additional sanitization should be employed as a mitigation.
   2. The potential prototype pollution within resources/mw.ext.wikistories.builder/api/searchImages.js via fileUrlMap[ i.title ] = i.srcset[ 0 ].src; also seems legitimate if i.title can be easily manipulated by a malicious user. Some not-too-painful mitigations are described at: https://learn.snyk.io/lessons/prototype-pollution/javascript/.
   3. The various v-html directives are not vulnerable to injection via user-controlled data as they either end in text() or are innocuous in the case of licenseHtml (assuming, of course, that various messages have not been compromised).
   4. The issues within vue-router.common.js could also be legitimate but appear to be lower risk on their face and would need to be filed upstream, correct? Is there still a desire to find another alternative to this router at some point?

Miscellanous Issues/Points of Discussion/Nits

1. There doesn't appear to be a composer.lock file committed to the repo. I understand that's less of an issue for the dev dependencies used here, but the same would be true for package-lock.json, which is committed to the repo.

DAST Findings

1. If https://en.wikipedia.beta.wmflabs.org/wiki/Special:StoryBuilder could be made functional (I'm getting a fatal though I have no idea if there are other things which need happen for that entry point to work), I wouldn't mind performing some additional pentesting and automated scans.

Change 806415 had a related patch set uploaded (by Sbisson; author: Sbisson):

[mediawiki/extensions/Wikistories@master] Commit composer.lock

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

Hi @sbassett and thank you for this analysis.

In T301389#8010527, @sbassett wrote:

[...]

About the various vulnerable or outdated packages, it looks like there's no need to do anything about it now. Let me know if I missed anything.

1. The span.innerHTML = a.innerHTML; assignment within resources/mw.ext.wikistories.builder/store/article.js does seem potentially vulnerable if a malicious user can control the a elements on a relevant page or if they are generated from user-input, which should be confirmed or disconfirmed. If they can be manipulated, additional sanitization should be employed as a mitigation.

This code is fetching the article HTML from restbase and replacing all the <a> with <span>. If the content of the link tags were compromised, would replacing them with spans make them any more harmful?

1. The potential prototype pollution within resources/mw.ext.wikistories.builder/api/searchImages.js via fileUrlMap[ i.title ] = i.srcset[ 0 ].src; also seems legitimate if i.title can be easily manipulated by a malicious user. Some not-too-painful mitigations are described at: https://learn.snyk.io/lessons/prototype-pollution/javascript/.

i.title is coming from the search API and is unlikely to be compromised but we never know. Also, page titles can be almost anything so it may still be possible to conflict with something on the prototype. Do you think if we guard against __proto__ and constructor as well as ensuring i.srcset[ 0 ].src is a string we're ok?

I see the article recommend a merge function from a trusted library but a merge sounds overkill for this single assignment and I'm reluctant to add another library. Do you know if we have something in Core already?

1. The various v-html directives are not vulnerable to injection via user-controlled data as they either end in text() or are innocuous in the case of licenseHtml (assuming, of course, that various messages have not been compromised).

I don't know what to think of this. This is what we give to v-html:

const html = this.$i18n(
	'wikistories-builder-licensing-with-terms',
	'https://foundation.wikimedia.org/wiki/Terms_of_Use',
	'https://en.wikipedia.org/wiki/Wikipedia:Text_of_Creative_Commons_Attribution-ShareAlike_3.0_Unported_License',
	'https://en.wikipedia.org/wiki/Wikipedia:Text_of_the_GNU_Free_Documentation_License'
).parse();

Do you know if Message.parse() is doing anything to make it safe if the message is compromised?

1. The issues within vue-router.common.js could also be legitimate but appear to be lower risk on their face and would need to be filed upstream, correct? Is there still a desire to find another alternative to this router at some point?

We still want to get rid of this dependency but we decided to postpone it based on our last conversation. I expect it to happen next quarter while the app is in production.

Miscellanous Issues/Points of Discussion/Nits

1. There doesn't appear to be a composer.lock file committed to the repo. I understand that's less of an issue for the dev dependencies used here, but the same would be true for package-lock.json, which is committed to the repo.

I made a patch to fix this.

DAST Findings

1. If https://en.wikipedia.beta.wmflabs.org/wiki/Special:StoryBuilder could be made functional (I'm getting a fatal though I have no idea if there are other things which need happen for that entry point to work), I wouldn't mind performing some additional pentesting and automated scans.

I filed T310889: Unhandled exception when StoryBuilder is accessed without a valid subpage to fix the error handling

Special:StoryBuilder can be used in two ways.

1. With as article as subpage: Special:StoryBuilder/Cat to create a new story based on that article
2. With a story as subpage: Special:StoryBuilder/Story:Cats to edit that story

Change 806415 merged by jenkins-bot:

[mediawiki/extensions/Wikistories@master] Commit composer.lock

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

In T301389#8011635, @SBisson wrote:

About the various vulnerable or outdated packages, it looks like there's no need to do anything about it now. Let me know if I missed anything.

Correct.

This code is fetching the article HTML from restbase and replacing all the <a> with <span>. If the content of the link tags were compromised, would replacing them with spans make them any more harmful?

Potentially, but not in any way that would likely be worse than restbase already containing malicious or erroneous content, since that should always be a trusted source.

i.title is coming from the search API and is unlikely to be compromised but we never know. Also, page titles can be almost anything so it may still be possible to conflict with something on the prototype. Do you think if we guard against __proto__ and constructor as well as ensuring i.srcset[ 0 ].src is a string we're ok?

This seems like it would be pretty decent protection, yes. Stopping someone from doing obj["__proto__"] is key here, since we aren't doing anything eval-ish or with multi-dimensional arrays.

I see the article recommend a merge function from a trusted library but a merge sounds overkill for this single assignment and I'm reluctant to add another library. Do you know if we have something in Core already?

Sorry, I meant more the other mitigations around preferring obj = Object.create(null) vs. obj = {} and using Object.freeze(obj.prototype) && Object.freeze(obj.__proto__), if possible.

I don't know what to think of this. This is what we give to v-html:

const html = this.$i18n(
	'wikistories-builder-licensing-with-terms',
	'https://foundation.wikimedia.org/wiki/Terms_of_Use',
	'https://en.wikipedia.org/wiki/Wikipedia:Text_of_Creative_Commons_Attribution-ShareAlike_3.0_Unported_License',
	'https://en.wikipedia.org/wiki/Wikipedia:Text_of_the_GNU_Free_Documentation_License'
).parse();

Do you know if Message.parse() is doing anything to make it safe if the message is compromised?

As is, Message.parse should just run a simple mw.html.escape. So if we're in a fairly standard html context with standard character sets, etc. then there should be little risk of an injection here, assuming Message.parse or Message.escaped or even mw.html.escape are used.

We still want to get rid of this dependency but we decided to postpone it based on our last conversation. I expect it to happen next quarter while the app is in production.

Ok, sounds good. We'll likely want to have a quick look (maybe just in CR) at whatever the replacement ends up being.

I made a patch to fix this.

Thanks.

I filed T310889: Unhandled exception when StoryBuilder is accessed without a valid subpage to fix the error handling

Special:StoryBuilder can be used in two ways.

1. With as article as subpage: Special:StoryBuilder/Cat to create a new story based on that article
2. With a story as subpage: Special:StoryBuilder/Story:Cats to edit that story

Sounds good. I may attempt some supplemental scans of these URLs this week, if I have a chance, but that shouldn't change the overall risk rating of this review.

Change 809606 had a related patch set uploaded (by Sbisson; author: Sbisson):

[mediawiki/extensions/Wikistories@master] Prevent potential prototype polution

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

Change 809606 merged by jenkins-bot:

[mediawiki/extensions/Wikistories@master] Prevent potential prototype polution

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

@sbassett FYI we got rid of the vue-router dependency in this patch: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikistories/+/822111

In T301389#8148187, @SBisson wrote:

@sbassett FYI we got rid of the vue-router dependency in this patch: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Wikistories/+/822111

Sounds good, thanks for the update. Just glancing at the patch, I didn't see anything that seemed immediately egregious. In fact, it seems substantially simpler than vue-router.common.js.