Here's the latest scc output for the repo:

───────────────────────────────────────────────────────────────────────────────
Language                 Files     Lines   Blanks  Comments     Code Complexity
───────────────────────────────────────────────────────────────────────────────
JSON                        91     27864        0         0    27864          0
PHP                         16      1880      179       334     1367         38
JavaScript                  12      1684      131       354     1199        110
HTML                         9       131        2         0      129          0
Markdown                     2         7        2         0        5          0
LESS                         1        68       11         8       49          0
License                      1        21        4         0       17          0
Vue                          1       421       14        10      397          0
XML                          1         7        0         0        7          0
gitignore                    1        39       11        12       16          0
───────────────────────────────────────────────────────────────────────────────
Total                      135     32122      354       718    31050        148
───────────────────────────────────────────────────────────────────────────────
Estimated Cost to Develop (organic) $996,001
Estimated Schedule Effort (organic) 13.73 months
Estimated People Required (organic) 6.44
───────────────────────────────────────────────────────────────────────────────
Processed 1116771 bytes, 1.117 megabytes (SI)
───────────────────────────────────────────────────────────────────────────────

@NHillard-WMF is there a target date for this review that I should know about? If not, I will post this review by December 20 at the latest.

Hi @Mstyles! Adding here for target date: there's a deliverable for T&S dependent on the tool that's due this quarter, so any extra time you can give us before the end-of-year deployment pause would be immensely appreciated.

@TAndic good to know, I will try to have this done by December 9

Security Review Summary - T320992 - 2022-12-09
Last commit reviewed: b5be782

Summary

Overall, the current extension under consideration
has an overall risk rating of: low.

The minimist package has a critical level vulnerability. While is it only a dependency of a development package, it should still be fixed. This is not an issue that prevents deployment, but highly recommended to get updated in the near future. Overall there were no other major vulnerabilities found.

There is some commentary below about the policies which are good, but could be improved. It is good to see that the issue of only allowing HTTPS links for external surveys was resolved (T255291). It doesn't appear that privacy policy links have the same HTTPS validation as external survey links. I would highly recommend also validating those links as well.

Vulnerable Packages - Production

snyk or npm audit - none

Vulnerable Packages - Development
snyk - none

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

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

Other Vulnerable Code

1. Semgrep
   • there were 8 findings on from 892 rules ran on 119 files, however all were false positives
2. gitleaks
   • none
3. npm lockflie-lint
   • none

Policy Compliance

1. Survey usage is limited to staff and does require a review from legal. I don't see a way this is actually enforced, other than ensuring that changes to mediawiki config and other code changes are brought to the attention of the team to check that surveys are indeed being launched by staff members
2. Privacy policies - privacy policies are created per survey and are required for the extension to work
3. Data collection is made clear here - https://meta.wikimedia.org/wiki/QuickSurveys/Guidelines#What_data_do_all_quick_surveys_collect?

Change 868123 had a related patch set uploaded (by Nathillard; author: Nathillard):

[mediawiki/extensions/QuickSurveys@master] build: Updating npm dependencies

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

Hi @NHillard-WMF @Jdlrobson flagging TST's T325271 investigation ticket in case there are overlapping workflows on this extension update, cc @ERayfield @eigyan @Madalina

Change 868123 abandoned by Nathillard:

[mediawiki/extensions/QuickSurveys@master] build: Updating npm dependencies

Reason:

Abandoning this as a formal change but leaving as proof of concept

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

Hi @TAndic -
I took a look at this yesterday briefly to understand what fix might be required and to see if I could quickly address / there were any gotchas. I do think it's better for longer-term maintenance if T&S can fix.

You can see the patch I did here for reference, which I've now formally abandoned as it was mostly a proof-of-concept to see what might be required: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/QuickSurveys/+/868123 - feel free to reference this fix if need-be . Notably this is probably larger in scope than required for this particular vulnerability, as it ran a more general npm update instead of a targeted dependency bump.

All of this said: it's worth noting that the vulnerabilities that have been flagged are in development dependencies, and thus would not affect shipping code. I may be misinterpreting the original diagnosis, but @Mstyles to clarify do you mean to say the dev dependencies should be considered blockers for deployment? Given they only impact local development tooling, it shouldn't be possible to exploit these in production - would you agree with this assessment?

Thank you for looking into this fix @NHillard-WMF. I am especially happy to pick up a new command/package( ncu ) that I was not aware of to update package.json and dependencies. I am curious to hear what the conclusion will be from @Mstyles on whether this patch will still be needed given its exploit impact is limited to development. Thank you everyone for your input and feedback thus far.

After discussion with many team members, I am okay with setting the risk rating as low. Since the critical issue is with the linter and only used in dev, I don't think should prevent a production deploy. However I do think that the patch should still move ahead at some point soon, preferably next quarter.