>- contentscript/verify.js: > - fetchConf(): please wrap regexps. Done.
>- contentscript/conf.js: > - change the descriptor file to >https://tails.boum.org/install/v1/Tails/amd64/stable/latest.yml Done > - are we sure that no other URL could be injected here: ajaxData.url= > this.conf.descriptor; ? Yeah, the url is hard coded in config, it can't be changed. >- What kind of comments does this remove: > 44 data = data.replace(/^[^'"]*#.*/gm, ''); // remove most comments > I don't see any in >https://tails.boum.org/install/v1/Tails/amd64/stable/latest.yml We copied it from old extension: https://git-tails.immerda.ch/ma1/download-and-verify-extension/tree/tails-download-and-verify/lib/df.js > - Can we not verify automatically once the download is finished? As far as I know, chrome does not allow you to read local files. Someone has mentioned this workaround https://stackoverflow.com/questions/41767585/chrome-extension-to-access-content-of-downloaded-files but I'm not sure if it'll work, we'll have to try. >- origin of files: > vendor/forge.js: please add a URL of origin for this script as well as a version number so that we can update it in the future. Done. > - Please use double quotes instead of single quotes. Done. > - convert tabs to spaces Done. > - fart operator => > - example: this.fetchConf().done()=>{ > JSLint requires the parens around the parameters, and forbids a { left brace after the => fart to avoid syntactic ambiguity. See: http://www.jslint.com/help.html#function Skipping the left brace is only allowed for one line functions. > - Consider using strict-mode: >https://developer.mozilla.org/en-US/docs/Web/JavaScript/ Reference/Strict_mode > We want this code to be forwards compatible as much as possible as well as as secure as possible. Done. > - Whishlist: please document how to test the extension locally in a >README file. > - Exclude this README file from the resulting XPI. > - See as example the HACKING file in tails >[email protected]:download-and-verify-extension Added a README file On Mon, Nov 13, 2017 at 11:18 PM, u <[email protected]> wrote: > Dear Uzair, > > so here is a more complete review of the extension. > > As said, I think there are two urgent matters: > > - manifest.json: > - "permissions": [ > "http://*/*";, > -> please deactivate HTTP. We only want to download over HTTPS. > > - contentscript/verify.js: > - fetchConf(): please wrap regexps. > > And here are some other points I realized. If you think that any of > these points are not applicable, please don't hesitate to comment. I'm > not an expert in webextensions. > > - contentscript/conf.js: > - change the descriptor file to > https://tails.boum.org/install/v1/Tails/amd64/stable/latest.yml (I think > sajolida created a ticket on our bugtracker already. It's not urgent, > because the other files currently contains the same contents.) > > - contentscript/verify.js: > - are we sure that no other URL could be injected here: ajaxData.url= > this.conf.descriptor; ? > - if not let's try to at least verify that the URL starts with > https:// and comes from tails.boum.org > - What kind of comments does this remove: > 44 data = data.replace(/^[^'"]*#.*/gm, ''); // remove most comments > I don't see any in > https://tails.boum.org/install/v1/Tails/amd64/stable/latest.yml > > - setVerifyListener(){ > let self = this; > this.$(this.document).on("change", this.conf.verifySelector, (e) > => { > self.calculateHash(e.target); > }); > } > -> So here we assume that the person clicks nicely on our button to > verify and that nobody will interfere. > - Can we not verify automatically once the download is finished? > - Also, can we have a listener for the hash in the URL? > For example, if I closed the window but now I want to come back > and just do the verification without downloading again? > > - manifest.json: > - "description": "Verify downloaded file", -> please make it clear > that this verifies a Tails ISO image using a SHA256 checksum. (I think > sajolida will handle this.) > > - origin of files: > vendor/forge.js: please add a URL of origin for this script as well as > a version number so that we can update it in the future. > > - JSLint http://www.jslint.com/ - this is a tool to write JS code which > is not error prone and I think it would be nice to follow the > requirements of JSLint. > - Please use double quotes instead of single quotes. > - convert tabs to spaces > - fart operator => > - example: this.fetchConf().done()=>{ > JSLint requires the parens around the parameters, and forbids a { > left brace after the => fart to avoid syntactic ambiguity. See: > http://www.jslint.com/help.html#function > - replace for loops with foreach (low prio) > > - Consider using strict-mode: > https://developer.mozilla.org/en-US/docs/Web/JavaScript/ > Reference/Strict_mode > We want this code to be forwards compatible as much as possible as > well as as secure as possible. > > - Integrated jquery library. Can we get rid of it and use only vanillaJS? > > - Whishlist: please document how to test the extension locally in a > README file. > - Exclude this README file from the resulting XPI. > - See as example the HACKING file in tails > [email protected]:download-and-verify-extension > > Cheers! > u. >
_______________________________________________ Tails-dev mailing list [email protected] https://mailman.boum.org/listinfo/tails-dev To unsubscribe from this list, send an empty email to [email protected].
