WIP: https://github.com/wikimedia/WhoWroteThat/pull/50

• If we upload manually we don't need to specify an extension ID in manifest.json.
• The version number in manifest.json and package.json needs to be updated for each release, and they obey different formats.
• The name from manifest.json is used to create the zip file that's uploaded to create a release, and the name from package.json is used in the generated gadget code.
• One idea is that we could keep a beta branch that has the different name ("WhoWroteThat BETA" maybe), and perhaps different config for e.g. logging.
• For example, the beta release process might be as follows (which is what the above PR is about):
  1. Change to the beta branch, and make sure it's up to date with master: git checkout beta; git pull origin/master
  2. Update the version number in package.json (SemVer) and manifest.json. The extension version number is defined (for both Chrome and Firefox) as 1.2.3.4, with the 4 being what we would increment for each beta release.
  3. Commit these changes. We could tag, but with only one of the version numbers (probably only a dot/hyphen difference though).
  4. Run grunt (default task; it's not part of the build task because there's no need to make the zip every time we want to build). This will create a file such as dist/whowrotethat_beta-0.1.0.1.zip.
  5. This file gets uploaded as a new release to the Beta add-on (which we'd all be admins on, so anyone could do this).
• The production release process would be much the same, but from the master branch instead, and with a non-suffixed version number.
• We need an add-on icon ("PNG and JPG supported. Icons will be resized to 128x128 pixels if larger.") and screenshot ("PNG and JPG supported, PNG recommended. The maximum and recommended size for screenshots is 2400x1800 pixels.").

The beta add-on is available now at https://addons.mozilla.org/en-US/firefox/addon/whowrotethat-beta/ and I've added all of CommTech as owners.

PR 50 is ready for review, as are my comments above about how this could all work.

@ifried is there already a task for the logo?

@Samwilson -- Thanks for reminding me! Just created a design ticket: T233895.

I was wrong about adding everyone to that add-on — you have to have registered a Firefox account first, and then I can add you via the email address you've registered with. Let me know what email addresses to use.

The Firefox add-on reviewer has replied:

This add-on is creating DOM nodes from HTML strings containing potentially unsanitized data, by assigning to innerHTML, jQuery.html, or through similar means. Aside from being inefficient, this is a major security risk. For more information, see https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Safely_inserting_external_content_into_a_page . Here are some examples that were discovered:
src\RevisionPopupWidget.js line 130

Oof. I think we knew this was a risk, right? Looks like we need to do sanitization or go toward some ES6 template sort of thing like @MusikAnimal was proposing at some point.

Pretty amazing of them to do this sort of code review for us!

Looks like we need to do sanitization or go toward some ES6 template sort of thing like MusikAnimal was proposing at some point.

Probably the opposite, where usage of template literals is the security risk, heh.

However I'm not seeing a glaring problem... the markup we're using for addedMsg on line 130 was built with jQuery, which as I understand should safeguard us? They might have been talking about what is now line 128, or line 67. If those need to be jQuery-ified (or just use DOM manipulation with vanilla JS), we can do that, but perhaps also Mozilla is unaware we are working with data provided our APIs which should be safe. @Mooeypoo would know best, I think :)

I guess I was thinking about a template library and not string literals. I misspoke.

Following the link that Sam provided, there are some specific recommendations for how to do this with jQuery.

As for using our own API, I imagine Mozilla isn't going to care. We should have safe methods for doing this regardless of the data source. That's the appropriate response for them to give and likely the appropriate thing for us to do.

This is more about good practice rather than strictly security.

Using good practice code standards solves for several things:

1. You don't need to constantly convince people that your code is safe. It looks safe.
2. If you ever change anything, you don't need to run around fixing all iterations of what *used* to be safe and aren't anymore, because everything is already making the assumption that it's safe.
3. When you get back to this code in x months/years of paused development, you can immediately understand what strings or literals are expected, where, and why, without having to follow the logic of the entire codebase.

That's partly why a lot of the helper methods in MediaWiki and in OOUI make very strict choices about the object types they're getting.
OOUI's setLabel() for example, will accept either a jQuery object it will present as html, or a string it will present as text, on purpose. If you want to send it a string that has HTML in it, your caller need to purposefully make sure that you wrap it properly, so it's clear to you, the caller, and tho whoever else will later read it, that you're using the text string intentionally, rather than accidentally, and that whatever value it is, it's safe. We make the caller's life "harder" on purpose. We want to make sure they think carefully about what they're sending in, and that other people will understand what was intended.

This is best practices in MediaWiki, as well. Here's a really good summary of this from our security best practices about demonstrable security.

It's not enough to assure yourself that you are perfect and that your code has no security vulnerabilities. Everyone makes mistakes. All core code, and a good deal of extension code, is reviewed by experienced developers to verify its security. This is a good practice and should be encouraged.
Write code in such a way that it is demonstrably secure, such that a reviewer can more easily tell that it's secure. Don't write code that looks suspicious but is, on careful examination, secure. Such code causes unnecessary reviewer anxiety.

I totally agree.

I started the patch to update this, but will wait till the MVC one is merged.

The other issues that the web-ext linter is complaining about are jQuery's use of innerHTML in 3 places (line numbers refer to the built result of PR50 but are probably similar in current master):

Code                    Message      Description              File          Line   Column
UNSAFE_VAR_ASSIGNMENT   Unsafe       Due to both security     js/generat…   1269   4     
                        assignment   and performance          ed.pageScr…                
                        to           concerns, this may not   ipt.js                     
                        innerHTML    be set using dynamic                                
                                     values which have not                               
                                     been adequately                                     
                                     sanitized. This can                                 
                                     lead to security                                    
                                     issues or fairly                                    
                                     serious performance                                 
                                     degradation.                                        
UNSAFE_VAR_ASSIGNMENT   Unsafe       Due to both security     js/generat…   4818   5     
                        assignment   and performance          ed.pageScr…                
                        to           concerns, this may not   ipt.js                     
                        innerHTML    be set using dynamic                                
                                     values which have not                               
                                     been adequately                                     
                                     sanitized. This can                                 
                                     lead to security                                    
                                     issues or fairly                                    
                                     serious performance                                 
                                     degradation.                                        
UNSAFE_VAR_ASSIGNMENT   Unsafe       Due to both security     js/generat…   6165   8     
                        assignment   and performance          ed.pageScr…                
                        to           concerns, this may not   ipt.js                     
                        innerHTML    be set using dynamic                                
                                     values which have not                               
                                     been adequately                                     
                                     sanitized. This can                                 
                                     lead to security                                    
                                     issues or fairly                                    
                                     serious performance                                 
                                     degradation.

I think the issues we were seeing 15 hours ago were maybe because the released version 0.3.0.0 predated the changes to jquery bundling. Because that was just an experiment with releasing, I didn't tag it — sorry! So I've just tagged the latest master with 0.4.0, and released that. Now at least we're in a known position (even if it's actually the same one).

The beta add-on is now available, and although there are more issues to solve, the basic system is in place for releasing new versions. We've all got access to do it, and there is documentation. I think the main QA here is to make sure it can be installed in Firefox.

It looks like on-going code review from the Mozilla side is less onerous than the initial creation of the add-on, but there is still going to be the possibility with every new release that we don't pass. I don't think it's super likely, as we're always going to be making sure we pass the web-ext linter with zero warnings.

The process for creating the production add-on is going to be identical to the beta one (just without 'beta' in the title).

We should still fix the security issues, (perceived or real, regardless)

Should we open a new task for that?

Sorry, yes, of course! After a weekend of not thinking about all this I totally forgot where I was up to. :)

PR for fixing the string-built HTML: https://github.com/wikimedia/WhoWroteThat/pull/72

But really, the basis of this tool is that we take raw HTML from an external API and inject it wholesale and without question. So I dunno how safe we can go. (Not that I'm not happier to remove the manual HTML stuff, and I think doing so will make it easier for the browser people to review.)

The other part of that might be to remove HTML from the i18n messages. Is that worth it? (i.e. if we want to get rid of all usages of $.html() and similar).

I wanted to share a general bit of guidance with regards to these "security" fixes. I think it would be helpful for us to think more in terms of defensive coding instead of secure or insecure or even more specifically whether we trust the source of the data or not.

If we adopt a stance of being defensive in our coding where that includes sanitizing inputs, catching exceptions, providing fallbacks for failures, checking types, etc, we will then naturally build up our defenses against these kinds of issues.

This is a good learning opportunity as a group where we could look at these changes once they are done and deconstruct how we might have identified these gaps even before we wrote the code. I think Moriel did warn us but we maybe didn't step back from the HTML or not discussion to see the larger point she was making about being defensive.

I've made a patch for removing HTML from the message files: https://github.com/wikimedia/WhoWroteThat/pull/76

@MBinder_WMF is there something strange doing on with the above Herald rule?

Should be fixed now. I was hoping that Phab was smart enough to know that Milestones should be exempt, but it is not...

The two patches waiting for review here are:

• Change string-built HTML to jQuery objects: https://github.com/wikimedia/WhoWroteThat/pull/72
• Move HTML out of i18n messages: https://github.com/wikimedia/WhoWroteThat/pull/76

It looks like this task is done. Please close it and open a new task for any remaining issues, otherwise, it is hard to track the progress of the project. Thanks!

Yep, the last part of this was merged on November 7. Ready for QA (via https://addons.mozilla.org/en-US/firefox/addon/whowrotethat-beta/ ).

@Samwilson I am seeing some differences between how this appears in the Firefox Add-On repo and in the Chrome Store. For example, the name is slightly different ("Who Wrote That? (BETA)" vs. "WhoWroteThat for Wikipedia (BETA)"). Is this due to the Firefox release being slightly older?

Also, the link to WikiWho under "About this extension" is https://www.f-squared.org/wikiwho/, but should be https://f-squared.org/wikiwho/ (former link gives an SSL error).

Acceptance criteria from T236226:

• The extension should be called "WhoWroteThat for Wikipedia (BETA)"

Name is now "Who Wrote That? (BETA)", which is same as on Chrome Store, but different from acceptance criteria in T236226. Is this OK?

If I change the language in the bottom right of the Firefox Add-Ons web page, the name and description remain in English.

• Credit the team, if possible: "By Wikimedia Foundation, Community Tech Team"

Yep.

• You can use the following description: "Explore authorship and revision information visually and directly in Wikipedia articles. Powered by WikiWho."

This is the description in the Firefox Add-Ons repo and in about:addons after installing the extension. But, it differs from Chrome (see T239223).

• Use the WWT logo, as developed in T233895

Yep.

• Mark the extension as experimental/beta, if possible

The name states it is BETA. And it is tagged with "Experimental" by Firefox.

In T232802#5690588, @dom_walden wrote:

Also, the link to WikiWho under "About this extension" is https://www.f-squared.org/wikiwho/, but should be https://f-squared.org/wikiwho/ (former link gives an SSL error).

This issue is now resolved. Link is correct.

Also:

• Tested that the links and email are correct on Firefox Add-Ons page and in about:addons (and the same as on Chrome Store). Unlike Chrome, we link to the wikiwho project and to the WWT Phabricator board. Chrome does not support these links in descriptions.
• After installing it, briefly tested it on enwiki. Tour appears. Basic functionality works. No errors seen in console.