Page MenuHomePhabricator

Require original source file(s) to be committed with minified files
Closed, ResolvedPublic

Description

[17:14:27] <tgr> Reedy: I thought the plan was to use npm to fetch the source but commit it to the patch and use ResourceLoader or webpack to minify it locally?
[17:15:28] <tgr> I'd hope Security vetoes everything that does not snapshot the source

This somewhat goes along with the reproducible build idea - https://wiki.debian.org/ReproducibleBuilds/About and https://reproducible-builds.org/ . If I have the source files, I should be able to make the same minified file as the original author did who added the minified file (or, that version of) to the git repo. Ideally, CI would do these steps to confirm

Event Timeline

Some of the uniminifiers out there (unminify.com, beautifier.io, et al) might obviate this requirement. Though for any serious code review (maybe that includes Gerrit) I'd agree that the developer should provide the uncompressed files, somewhere.

The point is, we need to be confident that the minified files actually match the source (no one ever will review minified files in practice). There are two reliable ways to do that: do the minification on the fly in production (which is a high-security environment so if that gets compromised we have bigger problems), which is what ResourceLoader does; or bundle the minification tool with the repo, have the developer use it to create the file that will be loaded, and have CI redo it and check that the output matches (both the developer and the CI box are potentially unreliable sources, but both being compromised seems unlikely). The latter assumes the minification process is deterministic (which AIUI is the case with webpack as long as you are careful what plugins you use).

Including the source and not just the minified version is a hard requirement for Debian packaging of MediaWiki.

Reedy renamed this task from Stop committing resultant minified files to various repos without the original source file to Require original source file(s) to be committed with minified files.Mar 1 2019, 10:36 AM
Jcross triaged this task as Medium priority.Oct 4 2019, 4:40 PM
matmarex claimed this task.
matmarex subscribed.

I documented this requirement on MediaWiki.org: https://www.mediawiki.org/w/index.php?title=ResourceLoader/Foreign_resources&diff=prev&oldid=6105186

I think that's all that's needed for this task. It doesn't seem feasible to enforce it automatically…

I documented this requirement on MediaWiki.org: https://www.mediawiki.org/w/index.php?title=ResourceLoader/Foreign_resources&diff=prev&oldid=6105186

I think that's all that's needed for this task. It doesn't seem feasible to enforce it automatically…

Thanks. I'd agree that establishing as a best practice (or even policy) is likely the most we can do for now. And, in addition to keeping an eye out for this during CR, it's something the Security-Team can also keep an eye out for during our security reviews (we try to look for certain code quality issues during said reviews when we can).