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 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
> 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
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
Uzair Farooq: >> - 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. Uzair: I think we shouldn't bother doing this. We took as an hypothesis when designing the page that we couldn't detect the end of the download (as you told us already). So the current page has been designed (and briefly tested) with this assumption in mind and if we could detect the end of the download we would have to redesign important parts which I'd rather not do at this point. ___ 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
>- 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
>ta...@git.tails.boum.org:download-and-verify-extension
Added a README file
On Mon, Nov 13, 2017 at 11:18 PM, u 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 w
[Tails-dev] Review of verification-extension
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.
