This task should also identify the ResourceLoader sizing differences reported by mw.inspect().

I know we've popularised mw.inspect() and I'm sorry about the confusion, but it's not a good metric to use for this purpose. As part of T194684, we'll improve its documentation and capabilities, including clarifying the meaning of some of its reports.

The byte sizes reported by mw.inspect() refer to the size of the source text of the module's script-self as serialised at run-time. It is not representative of user bandwidth, or execution time, nor memory use - which are useful metrics. It mw-inspect value closely approximates what the response size would be to an uncompressed and uncombined request for the module. Although not even that due to absence of closure wrapping and wrapping meta data, headers, and any messages/templates.

I'd recommend to inform decisions and other comparisons based on bandwidth and/or execution time. Where bandwidth is measured by comparing the network transfer sizes of a typical web requests. Eg. the request that would normally happen on a cold cache with several modules combined and gzip applied, and execution time potentially measured using a performance profile in the browser and finding the sub-slice where the web request executed its payload (combined).

Change 482745 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Hogan 3

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

This task should also identify the ResourceLoader sizing differences

Hogan 3 is actually larger than Hogan 2 by 1kb, however provides a path to removing Hogan from the client altogether (T94086).

@Jdlrobson (Jon Robson): What do you need from Security-Team-Reviews at this point?

@charlotteportero hey there! Would be great to know if there are any problems from us upgrading to Hogan 2 to Hogan 3. it seems from a security perspective using more recent code would make much more sense, but I'm not sure how thorough a security audit for bumping a major version would be. Do we even need to involve security team?

Change 482745 abandoned by Jdlrobson:
Hogan 3

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

Change 482745 restored by Jdlrobson:
Hogan 3

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

Change 482745 abandoned by Jdlrobson:
Hogan 3

Reason:
Waiting on security sign off. Abandoning until then.

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

In T205129#4878826, @Jdlrobson wrote:

@charlotteportero hey there! Would be great to know if there are any problems from us upgrading to Hogan 2 to Hogan 3. it seems from a security perspective using more recent code would make much more sense, but I'm not sure how thorough a security audit for bumping a major version would be. Do we even need to involve security team?

Of course, at this point, it's over 4.5 years old. I guess it's marginally better than 7 years old

So "better security" at this point is academic. Or based on the idea it's widely used and apparently nothing reported.

Commits of minified js are obviously basically impossible to review. There's also no tags/branches/whatever before 3.0.0 in https://github.com/twitter/hogan.js but https://www.npmjs.com/package/hogan.js/v/2.0.0 knows it exists (but of course, the git hashes/branches/tags have no guaranteed relevance to the actual "versions" in the npm repos)

There's been no commits on https://github.com/twitter/hogan.js/commits/master for nearly 4 years. Little to no activity by maintainers in the github issues. Is this abandonware? Twitter won't even tell us whether or not they're using it in production. Much hope here, right?

None of this gives me much confidence, but c'est la vie.

I don't see any history of (public) CVE's against Hogan, and I'm guessing if there were any (and older than the last 6M or so), they would've been made public in some way or another

I guess, if some (security) issue does come up in Hogan 2, it's less likely to be supported/fixed than Hogan 3. So that's definitely a bonus

https://www.npmjs.com/package/hogan.js/v/2.0.0 has no (non dev) dependancies

https://www.npmjs.com/package/hogan.js/v/3.0.2 has two, https://www.npmjs.com/package/nopt and https://www.npmjs.com/package/mkdirp

, "dependencies" : {
    "nopt" : "1.0.10"
  , "mkdirp": "0.3.0"
  }

nopt 1.0.10 is 7 years old, whereas version 3 is around the same age as version 3 of hogan. The latest is also 4.0.1. It also introduces 2 dependancies itself, abbrev and osenv. abbrev has 0 dependancies, osenv has 2 more. These are os-homedir and os-tmpdir (both are pointless and deprecated for features inside node itself). But they're still included for the moment based on the version tree.

Luckily, older version 1.0.10 only has abbrev "1" - https://github.com/npm/nopt/blob/1.0.10/package.json (what does 1 actually compare to? Is semver in use here? I see no 1.0.0 on https://www.npmjs.com/package/abbrev, just point releases of varying degrees)... But according to a depedancy analyzer/resolver... It means something that starts with 1! So we end up with the newest version of abbrev

So we can ignore the osenv ones listed above.

mkdirp 0.3.0 is also 7 years old (I see a pattern here). Though, the latest version 0.5.1 is around the same age as version 3 of hogan. Latest version adds one dependancy itself, minimist (which doesn't have any more dependancies, yay).

0.3.0 has no dependancies, so we do make things a bit easier

So, adding Hogan 3, we add 3 other dependancies (as many as 7 if the packages had been updated down the tree)... None of these are in MobileFrontend/package.json, neither have they been mentioned above. It's impossible to tell from the minified js at https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/482745/ if/when/where they're included/used/inlined or whatever.

I don't see any CVE's for those packages

This is all, of course, without even looking at the code.

From what I understand... This is stuff only run locally on your laptops (like many other tools that are unreviewed) (as shown by it being listed as a dev dependancy in https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/482745/4/package.json (even though it's committed, when most of the rest of them in that list, aren't, uh whatever))..

So why exactly is a security review actually necessary here? I could maybe understand asking for a review of the rebuild resultant files (built using Hogan 3 rather than Hogan 2) if there are much differences...

From what I understand... This is stuff only run locally on your laptops
So why exactly is a security review actually necessary here? I could maybe understand asking for a review of the rebuild resultant files (built using Hogan 3 rather than Hogan 2) if there are much differences...

I'm still trying to understand the security review process so it's definitely possible I'm being overly cautious! The Hogan.js library is shipped to end users, so this is why I involved security. The templates are compiled on the /client/. I was curious how involved in this decision the security team would want to be given templates can be passed mediawiki messages as input. Are you involved in jQuery version bumps for example?

here's been no commits on https://github.com/twitter/hogan.js/commits/master for nearly 4 years. Little to no activity by maintainers in the github issues. Is this abandonware? Twitter won't even tell us whether or not they're using it in production. Much hope here, right?

Other thoughts we've been discussing in the team are moving towards Mustache (T94086#4525712) - as this is also used in core (https://www.npmjs.com/package/mustache) and React/Preact with JSX. This will take a little more time as we need to ensure our templates are Mustache compatible.

I'm curious how involved you would want to be in this decision!

In T205129#4991315, @Jdlrobson wrote:

From what I understand... This is stuff only run locally on your laptops
So why exactly is a security review actually necessary here? I could maybe understand asking for a review of the rebuild resultant files (built using Hogan 3 rather than Hogan 2) if there are much differences...

I'm still trying to understand the security review process so it's definitely possible I'm being overly cautious! The Hogan.js library is shipped to end users, so this is why I involved security. The templates are compiled on the /client/. I was curious how involved in this decision the security team would want to be given templates can be passed mediawiki messages as input. Are you involved in jQuery version bumps for example?

Nope, and I don't think Security has ever reviewed it either. I think those mostly relies on Timo, and the fact we have close links to upstream via him. If we have a security issue, we'd be reporting it upstream and knowing we should be able to get a fix as appropriate. Then updating our versions to match

Although we haven't turned it into a proper policy as of yet.. There are discussion (and other library/vendor reviews of PHP code have already been deferred for that reason) of basically where the code is coming from. If it's a big, well know, well tooled, with security teams/procedures (for example Symfony), and the library hasn't got a history of being full of security holes.... The review mostly becomes a paperwork exercise. It's generally not worth the time of us doing a third party review when we have many other things to be working on (often code that hasn't got anywhere near as many eyes on, such as the code written here)

As always, if someone introduces a new library/similar, it's partially their responsibility to keep an eye on upstream for any releases that might be necessary to pull in, in terms of both security and other bug fixes. They are the owners of this. Unfortunately, this has broken down in other cases with for example our old version. No one is around to support/help get things upgraded, which is disappointing.

If it's shipped to users... When/where are the dependancies brought in? (This is obviously much more obvious when we don't commit minified files to git and let RL take care of it) Is it in the combined file somewhere

In T205129#4991315, @Jdlrobson wrote:

here's been no commits on https://github.com/twitter/hogan.js/commits/master for nearly 4 years. Little to no activity by maintainers in the github issues. Is this abandonware? Twitter won't even tell us whether or not they're using it in production. Much hope here, right?

Other thoughts we've been discussing in the team are moving towards Mustache (T94086#4525712) - as this is also used in core (https://www.npmjs.com/package/mustache) and React/Preact with JSX. This will take a little more time as we need to ensure our templates are Mustache compatible.

I'm curious how involved you would want to be in this decision!

I'm not sure if the Security Team "care" so much. We could advise against using a Tool because of various reasons, but you could still use said tool if you decided you needed/wanted to do so (more so probably in a "I have no other choice" way). That'd be a case of you accepting the risk (which is a whole other discussion). I think it'd be pretty rare we would completely veto things, but there's definitely room for it to be done.

That being said, anything we can do to reduce the number of random dependancies only used by a few (or even one) extension, and re-using what is in core (or vendored in as appropriate), seems like a net win for all. Maintainability, Security, Performance et al.

A question that is basically outstanding here is what is to be gained by upgrading from 2 to 3; there are little to no security benefits as far as I can tell. As such, we definitely can't say we find the upgrade necessary from that grounds. As also said above, it only of questionably likelihood that version 3 is more likely to get proper security support if it actually needed it.

The extra dependancies obviously add a security overhead, and potentially a great attack vector. For example, this line, again - https://github.com/npm/nopt/blob/1.0.10/package.json#L12 would be a great candidate to introduce something like https://www.theregister.co.uk/2018/11/26/npm_repo_bitcoin_stealer/ as it would just update to the newest version (because of such vague and unspecific version constraints) in that library and bring in a plethora of crap with it. A smaller less used/known library would be a great place to attempt something like that

What is the reason for investing time/effort in this upgrade? Without reading through T94086: [EPIC] Migrate MobileFrontend templates from hogan to inlined mustache templates, but looking at the title, it seems odd out of band to upgrade something if we're going to be removing/replacing it...

Not new in your commit either, but T217351: Require original source file(s) to be committed with minified files feels kinda relevant here too. We're committing minified files that are basically impossible to verify. We should be committing the unminified sources too, so we can do verifiable and reproducible builds (see https://reproducible-builds.org/ and https://wiki.debian.org/ReproducibleBuilds/About )