Re: [Tails-dev] Review of verification-extension

[intrigeri]

Uzair Farooq:
>> Hm, but if it's not needed, why don't we remove this piece of code? Can you
>> try? Also make sure that the input we receive here can be trusted.

> I think it's meant to remove comments from the end of a line e.g.

> build-target: amd64 # some comments here

Indeed, this is valid YAML.

(In passing, I'm still concerned we do ad-hoc, manual YAML parsing but
well, fixing this was not part of the job you've been tasked with :)
___
Tails-dev mailing list
Tails-dev@boum.org
https://mailman.boum.org/listinfo/tails-dev
To unsubscribe from this list, send an empty email to 
tails-dev-unsubscr...@boum.org.

Re: [Tails-dev] Review of verification-extension

[Uzair Farooq]

> Hm, but if it's not needed, why don't we remove this piece of code? Can you
try? Also make sure that the input we receive here can be trusted.

I think it's meant to remove comments from the end of a line e.g.

build-target: amd64 # some comments here

This might be useful if you add some comments to the file in future.

On Fri, Nov 24, 2017 at 1:49 PM, u  wrote:

> Hi,
>
> Uzair Farooq:
>
> >> - 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
>
> Hm, but if it's not needed, why don't we remove this piece of code? Can
> you try? Also make sure that the input we receive here can be trusted.
>
> Thanks for the other modifications :)
>
> u.
>
___
Tails-dev mailing list
Tails-dev@boum.org
https://mailman.boum.org/listinfo/tails-dev
To unsubscribe from this list, send an empty email to 
tails-dev-unsubscr...@boum.org.

Re: [Tails-dev] Review of verification-extension

[u]

Hi,

Uzair Farooq:

>> - 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

Hm, but if it's not needed, why don't we remove this piece of code? Can
you try? Also make sure that the input we receive here can be trusted.

Thanks for the other modifications :)

u.
___
Tails-dev mailing list
Tails-dev@boum.org
https://mailman.boum.org/listinfo/tails-dev
To unsubscribe from this list, send an empty email to 
tails-dev-unsubscr...@boum.org.

[Tails-dev] Review of verification-extension

[u]

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
ta...@git.tails.boum.org:download-and-verify-extension

Cheers!
u.
___
Tails-dev mailing list
Tails-dev@boum.org
https://mailman.boum.org/listinfo/tails-dev
To unsubscribe from this list, send an empty email to 
tails-dev-unsubscr...@boum.org.