Security review of version 413910ce6d15e (ie HEAD as of august 18)

Overall, this extension looks well done :D

Low severity issues

• Use of raw html for messages. Most of the messages used in the mustache templates are included as raw html. We are trying to avoid use of raw html messages in new code. Please html escape all messages unless absolutely necessary.

Non-security

This is stuff that jumped out at me when I was looking at the extension but aren't security issues. Most of these things are minor and some are rather nitpicky. This part is just meant to be helpful, but is not part of the security review, so feel free to ignore if you disagree with any of the points.

• [extension.json] i18n is marked as for the Boilerplate extension. This should be changed, as could mess up localizationCache if another extension used the same name.
• Lines like: var moduleCollector = function runModuleCollector( data ) { - Is that correct (having a function name and treating it as an anonymous function). I'm not super familiar with the nitty gritty of js, so maybe that does do something, but it seems like having the second name would not do anything (?)
• The toolbox entry is shown always, but only works when doing a preview of a page (And you have js enabled). Toolbox entry really shouldn't be shown in cases where it wouldn't work, or at least some sort of error should pop-up in cases where it doesn't work.
• Be nicer it the parser cache section of the report actually parsed the data (e.g. date should be human readable form. Cache expiry should specify the unit is seconds, etc.)
• The bar-graph should probably have whitespace: nowrap for the module names, otherwise long module names smush into next row.
• [modulesize collector toHumanSize()] - Note we also already have i18n messages for size units - size-megabytes and friends. Perhaps those should be used for the human readable sizes.
• Missing message performanceinspector-imagesize-title, performanceinspector-newpp-title messages. (The current mustache template turns them into tags)

Thanks very much @Bawolff!

In T137197#2576786, @Bawolff wrote:

• Lines like: var moduleCollector = function runModuleCollector( data ) { - Is that correct (having a function name and treating it as an anonymous function). I'm not super familiar with the nitty gritty of js, so maybe that does do something, but it seems like having the second name would not do anything (?)

It's not a pattern we use very frequently, but it does have a point: the second name means stack traces and other debugging contexts (profiler output, etc.) display a descriptive name, rather than 'anonymous function'.

See https://kangax.github.io/nfe/

In T137197#2577144, @ori wrote:

Thanks very much @Bawolff!

In T137197#2576786, @Bawolff wrote:

• Lines like: var moduleCollector = function runModuleCollector( data ) { - Is that correct (having a function name and treating it as an anonymous function). I'm not super familiar with the nitty gritty of js, so maybe that does do something, but it seems like having the second name would not do anything (?)

It's not a pattern we use very frequently, but it does have a point: the second name means stack traces and other debugging contexts (profiler output, etc.) display a descriptive name, rather than 'anonymous function'.

See https://kangax.github.io/nfe/

Oh cool. Good to know.

Thanks @Bawolff I'll take your non-security input, I'll create a couple of issues for those.