Page MenuHomePhabricator

Security Review Request: mck89/peast
Closed, ResolvedPublic

Description

Project Information

peast$ git l -1
* 982271d99d - (HEAD, origin/HEAD) Fixed parsing of async keyword as class method (2023-08-16) <Marco Marchiò>
peast$ scc doc/ lib/
───────────────────────────────────────────────────────────────────────────────
Language                 Files     Lines   Blanks  Comments     Code Complexity
───────────────────────────────────────────────────────────────────────────────
PHP                        151     20940     1670      8887    10383       1184
Markdown                     5       623      105         0      518          0
───────────────────────────────────────────────────────────────────────────────
Total                      156     21563     1775      8887    10901       1184
───────────────────────────────────────────────────────────────────────────────
Estimated Cost to Develop (organic) $331,844
Estimated Schedule Effort (organic) 9.04 months
Estimated People Required (organic) 3.26
───────────────────────────────────────────────────────────────────────────────
Processed 575240 bytes, 0.575 megabytes (SI)
───────────────────────────────────────────────────────────────────────────────

Description of the tool/project: Tokenizer and validator of JavaScript syntax.

Description of how the tool will be used at WMF:

In ResourceLoader as part of MediaWiki core, to determine the structural integrity of site scripts (like MediaWiki:Common.js) and gadgets (like MediaWiki:Gadget-foo.js) before minifying and concatenating them with other scripts. This prevents deployment of syntax errors, and ensures consistent browser support (i.e. only deploy code that is known-compatible with all browsers this payload is cached for and delivered to).

The status quo is that integrity check is carried out by calling JsMinPlus.php, which is a library checked into mediawiki/core.git/:/includes/libs/, predating use of Composer and mediawiki/vendor. There are three main problems with this library:

  1. The last upstream release was more than ten years ago in 2011.
  2. Upstream homepage and contact no longer online. Unofficial mirrors preserve copies of the code, but are equally inactive/frozen, e.g. https://github.com/JSMinPlus/JSMinPlus/blob/master/changelog.txt.
  3. JsMinPlus validates the ES5 specification for JavaScript code. ResourceLoader supports ES6 ("ES2015") and ES7 ("ES2016") code as of T272882: Upgrade ResourceLoader JS minifier to support ES6, T178356, and T277675. MediaWiki core and extension already make use of these capabilities. However, user scripts and gadgets are unable to use these capabilities as the validator considers the unknown/modern syntax as invalid.

The replacement of this internal dependency thus blocks the resolution of these two goals:

Dependencies: None! ✔️

Has this project been reviewed before? No.

Working test environment:

Post-deployment:

Primary contact for this dependency's listing at https://www.mediawiki.org/wiki/Upstream_projects will be MediaWiki-Platform-Team (@Krinkle).

Details

Risk Rating
Low

Event Timeline

sbassett changed the task status from Open to In Progress.Oct 3 2023, 5:44 PM
sbassett claimed this task.
sbassett triaged this task as Medium priority.
sbassett moved this task from Upcoming Quarter Planning Queue to In Progress on the secscrum board.
sbassett added a project: user-sbassett.

Quick update on this review: peast is looking very good/healthy according to many of the tools we typically run for security reviews (vuln deps, sast, etc.) A few (very likely) false-positives came back from phan-taint-check, semgrep, phpcs and some rudimentary greps. I plan to verify these results a bit more and include the raw output, but so far these seem low-risk, at worst. I then plan to manually review the code a bit, but won't be attempting to verify every distinct behavior of peast's feature set or subjecting it to any kind of performance review.

Thanks @sbassett! The Design-System-Team intends to work on T313945 in Q3 (January-March). Do you think the security review would be finished by, say, the end of January?

Thanks @sbassett! The Design-System-Team intends to work on T313945 in Q3 (January-March). Do you think the security review would be finished by, say, the end of January?

I hope to have the review posted here by the end of this week.

Excellent, that would definitely work for us! Thank you!

Security Review Summary - T347922 - 2023-12-20
Last tag reviewed: v1.15.4

Summary

Overall, the current mck89/peast@v1.15.4 code looks secure and well-maintained with 8,640 tests and 1,649,732 assertions, all passing. Known issues appear minimal for a complex project and seem to be resolved quickly. Automated security tooling and manual security review techniques were not able to find any significant issues at this time, thus the Security-Team would classify this code with an overall risk rating of: low.

General Information

Statistic/InfoValueRisk
Repositoryhttps://github.com/mck89/peast none
Relevant tag/branchv1.15.4 none
Last commit reviewed (if relevant)1df4dc28a6 none
Recent contributions to code (6 months)6 from 1 author medium
Active developers with > 10 commits3 low
Current overall usage164 stars, 6,932,231 installs low
Current open security issues0 none
Methods for reporting security issuesno obvious policies defined medium

Vulnerable Packages - Production and Development
Risk: none
peast only has one explicit dev dependency: phpunit/phpunit.

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

PackageCurrentLatestNotes
phpunit/phpunit9.6.1510.5.2The PHP Unit Testing framework.
phpunit/php-code-coverage9.2.2910.1.9Library that provides collection, processing, and rendering functionality for PH...
phpunit/php-file-iterator3.0.64.1.0FilterIterator implementation that filters files based on a list of suffixes.
phpunit/php-invoker3.1.14.0.0Invoke callables with a timeout
phpunit/php-text-template2.0.43.0.1Simple template engine.
phpunit/php-timer5.0.36.0.0Utility class for timing
sebastian/cli-parser1.0.12.0.0Library for parsing CLI options
sebastian/code-unit1.0.82.0.0Collection of value objects that represent the PHP code units
sebastian/code-unit-reverse-lookup2.0.33.0.0Looks up which function or method a line of code belongs to
sebastian/comparator4.0.85.0.1Provides the functionality to compare PHP values for equality
sebastian/complexity2.0.23.1.0Library for calculating the complexity of PHP code units
sebastian/diff4.0.55.0.3Diff implementation
sebastian/environment5.1.56.0.1Provides functionality to handle HHVM/PHP environments
sebastian/exporter4.0.55.1.1Provides the functionality to export PHP variables for visualization
sebastian/global-state5.0.66.0.1Snapshotting of global state
sebastian/lines-of-code1.0.32.0.1Library for counting the lines of code in PHP source code
sebastian/object-enumerator4.0.45.0.0Traverses array structures and object graphs to enumerate all referenced objects
sebastian/object-reflector2.0.43.0.0Allows reflection of object attributes, including inherited and non-public ones
sebastian/recursion-context4.0.55.0.0Provides functionality to recursively process PHP variables
sebastian/type3.2.14.0.0Collection of value objects that represent the types of the PHP type system
sebastian/version3.0.24.0.1Library that helps with managing the version number of Git-hosted PHP projects

Static Analysis Findings
Risk: low.
A handful of popular, FOSS static analysis tools were run against the peast v1.15.4 codebase. A summary of some of these tools' output exists below:

  1. semgrep with various custom and well-known policies (results: P54498)
  2. bearer sast (no sast results)
  3. scan.sh sast (no sast results)
  4. phpcs with security rule sets (results: P54496)
  5. phan-taint-check with general, non-MediaWiki config (results: P54495)

Overall, minimal findings resulted from these tools, with all being determined as either genuine false-positives or extremely low-risk.

Manual Review Findings
Risk: low.
No obvious findings were found during this review.

sbassett moved this task from In Progress to Our Part Is Done on the secscrum board.
sbassett moved this task from In Progress to Done on the user-sbassett board.