Changes for AMO listing

[cadorn]

AMO rejected the FirePHP release: https://twitter.com/cadorn/status/930355260261941248

Reasons:

• Use of eval() which I cannot get by without. domplate uses it.
• Use of bundled files without sources

My response to review

Thank you for the review.

1. Re eval

A couple of the UI libraries I use need eval() so I cannot remove the use of it.

I can put the whole UI of the plugin onto an external domain and load it into an iframe inside the devtools panel. Then communicate via post-message.

This will sandbox eval() to the iframe. It will also remove the need for the inclusion of jquery in the extension. I presume eventemitter3 [1] is not allowed either or is that considered custom extension code since it is not listed in the pre-approved libs?

Will relocating UI into an iframe be approved? i.e. The user must be online to load the UI of the extension. There can be a message when UI cannot be loaded due to being offline. That could be solved by allowing such UIs to be proxied using the proxy API where it can serve files from extension as an optional source.

2. Re generated code

Scripts like [2] are generated using browserify and include an npm package listed here [3].

What is the objective of the review that requires files to be split into a tree vs inlined into one file? The code is not obfuscated. If you deem it is, which part?

Including many modules from various packages into an extension in a fashion where this code loses its package structure is a common use-case.

I am trying to understand what exactly is needed for review.

Do you need to be able to generate the XPI from all original sources yourself? If so, what benefit is there to inspecting the code in their respective packages vs in one file that contains everything?

I would argue that it is too much of a burden to ask someone doing review to install everything needed to build an extension from source.

I would argue that it would be much more practical and relevant to review the exact code that is going to be going live. To make this happen the source code in the extension must be clear text. This is the case in the bundled files in the extension.

If the review process requires a package structure to create complimentary meaning for the reviewer when looking at the code I would ask which review objectives are being satisfied when such information is relevant. I would think the only relevance for source code is to review auto-flagged API-usage in context.

If the files are required to be broken up I can do so but I do not understand what value it adds to the review process.

I would argue the primary focus should be on use of APIs which the automated review system should flag without manual review involvement. My extension went live and then was taken down again based on criteria that could have been automatically enforced.

I am hoping to release many more Web Extensions built in this fashion and I am trying to arrive at a format for the source code that will make review easy going forward.

I can generate the extension code however it best fits for review (with some runtime considerations) but I do not think it is at all practical to put reviewers into a position to have to generate the same code themselves.

I would rather put you in a position to run static analysis linting code agains the bundles to do a deep analysis of which browser APIs are used. I see this as the root foundation for the review process as all other code just hides such calls.

I am looking forward to crafting a solution that meets everyone's needs.

Christoph

[1] - https://github.com/firephp/firephp-for-firefox-devtools/blob/amo/dist/firephp.raw/lib/eventemitter3.js
[2] - https://github.com/firephp/firephp-for-firefox-devtools/blob/amo/dist/firephp.raw/scripts/background.js
[3] - https://github.com/firephp/firephp-for-firefox-devtools/blob/amo/package.json#L8

[cadorn]

Landed #36 which makes all build tools available for others to build the firephp extension. This way, AMO reviewers can now build the extension from source which is a requirement for listed addons.

Use of eval() has been isolated to a single file and is only used by domplate to execute compiled templates.

Need to verify build on ubuntu to ensure it is identical to OSX one and can then re-submit to AMO.

[cadorn]

New build submitted to AMO. Fingers crossed.

To build extension from source, see: https://github.com/firephp/firephp-for-firefox-devtools#source

Build was tested on macOS and Ubuntu.

Go to http://firephp.org/ to see sample messages logged to 'FirePHP' devtools panel.

There is still two uses of 'eval()' here [1][2] which comes from domplate [3] which I embed.

domplate escapes all input and only ever evals self-generated code and was used by firebug before. I am hoping I can keep using domplate as-is.

[1] - https://github.com/firephp/firephp-for-firefox-devtools/blob/master/dist/firephp.build/scripts/devtools/dist/insight.rep.js#L1416-L1435
[2] - https://github.com/firephp/firephp-for-firefox-devtools/blob/master/dist/firephp.build/scripts/devtools/fireconsole.rep.js#L3752-L3771
[3] -
https://github.com/cadorn/domplate/blob/master/lib/explicit-unsafe-eval.js

[cadorn]

Review response:

Your add-on FirePHP has been reviewed and one or more of of its versions have been disabled due to critical issues discovered.

Details:
This version did not pass the review because of the following issues:

1. Issues listed on the previous reviews are still outstanding.

unsafe-eval & eval are not allowed due to security issues. In rare cases, it can be allowed by Admin Reviewers only. If you wish to discuss the issues further, you can reach admin reviewers on IRC #addon-reviewers on irc://mozilla.org/

Please fix them and submit again. Thank you.

I am sitting in #addon-reviewers on IRC hoping to get a an escalated review.

/cc @digitarald

[cadorn]

I do not think AMO will approve the use of eval().

I am working on a compiler for domplate so that all reps can be compiled server-side and used without eval() on client: domplate/domplate#3

[cadorn]

Server-side compile for domplate is working: domplate/domplate#3

Now I need to update the reps for the UI to compile on the server: https://github.com/insight/insight.renderers.default

[ErikKrause]

Just an idea... You need domplate for the dedicated devtools panel, right? The chromelogger extension https://addons.mozilla.org/de/firefox/addon/chromelogger/ writes to the devtools console. Why not do the same?

[cadorn]

writes to the devtools console. Why not do the same?

That could be added as an option.

The plan is to have a dedicated panel to do much more advanced stuff than what can be done with the stock console panel.

I am close to wrapping up a project that has been taking all my extra time. When that is done I will be back on FirePHP to get the extension into the store.

[cadorn]

I have a new build that no longer uses eval() and includes a bunch of other changes and improvements: #39

Would be great to get some feedback on the usability.

I have some work left on releasing all the sub-projects for this so that it can be compiled from source. Once that is done I'll be submitting it to AMO to get listed!

[cadorn]

I had submitted the extension to AMO just in case (without others being able to build it from source) and it was approved already: https://addons.mozilla.org/en-US/firefox/addon/firephp/

FirePHP is back as a listed addon!

Compiling it from source is coming next.