Project

General

Profile

Feature #14945

Bug #12328: Tails Verification: Migrate DAVE to Web Extensions and support Chrome

Review dave_2.js

Added by sajolida about 1 month ago. Updated 19 days ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Category:
Installation
Target version:
Start date:
11/09/2017
Due date:
% Done:

0%

QA Check:
Feature Branch:
master
Type of work:
Code
Blueprint:
Starter:
Affected tool:
Download and Verification Extension

Description

Cody will review the display logic and the interaction with download_2.inline.html as it's quite complete but Ulrike said she would review the JavaScript for syntax, style, best practices, code structure, security, etc.

I'm pushing changes to master as I make it because I need the page to be on the production website for testing. So the review should cover /install/inc/js/dave_2.js on master. Yeah, rock'n'roll!

dave_2.js (10.5 KB) u, 11/12/2017 07:52 PM

History

#1 Updated by sajolida about 1 month ago

  • Description updated (diff)

#2 Updated by u 29 days ago

  • File dave_2.js added
  • Assignee changed from u to sajolida
  • QA Check set to Dev Needed

Hey!

I did not dare pushing to master and I did not test any of my modifications. I am attaching the file here for you so you can do a diff and test yourself.
I only found some minor issues, like the need to wrap regexps and the syntax of the EventListener function. Please test what I did. I also added some URLs for you to verify yourself.

#3 Updated by sajolida 28 days ago

  • Target version changed from Tails_3.3 to Tails_3.5

#4 Updated by sajolida 28 days ago

  • Assignee changed from sajolida to u
  • QA Check changed from Dev Needed to Ready for QA

Cool!

I applied your changes in a bunch of commits, please have a second look:

7a6deb9390 Fix grammar
d984f67ab2 Extract function
abff6a6ac0 Explain how the page is reset at first
5399fcfee2 Wrap regexp
e257655cff Use 'let' instead of 'var' in 'for' loops
873ddf7a35 Use a class instead of embedding CSS in the JS code
7bb9426dde Factorize
36aa0a13cc Explain why we're doing this with JS instead of CSS
a63118b976 Use double quotes instead of single quotes
  • Regarding "function showFloatingToggleableLinks" I added a comment to explain why I have to do this in JS. See 36aa0a13cc and tell me if that's not clear enough.
  • Regarding escaping the file name. I tried with '*?\~_-" as file name and it's displayed correctly, so I think we don't need that for display purpose. If you were thinking about security issues, please elaborate and maybe tell me a bit more about how I should escape (sanitize?) this.
  • I didn't merged the third parameter that you added to EventTarget.addEventListener() (wantsUntrusted=false) because Mozilla says it's Firefox specific: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener.
  • I didn't understand what you meant by "incorrect function call" so if what I copied from your version is not enough, please elaborate.

#5 Updated by intrigeri 24 days ago

  • Affected tool changed from Download and Verification Extension to Verification Extension

#6 Updated by u 22 days ago

  • Assignee changed from u to sajolida
  • QA Check changed from Ready for QA to Dev Needed
  • Affected tool changed from Verification Extension to Download and Verification Extension

I applied your changes in a bunch of commits, please have a second look:

Great!

  • Regarding "function showFloatingToggleableLinks" I added a comment to explain why I have to do this in JS. See 36aa0a13cc and tell me if that's not clear enough.

Ack.

  • Regarding escaping the file name. I tried with '*?\~_-" as file name and it's displayed correctly, so I think we don't need that for display purpose. If you were thinking about security issues, please elaborate and maybe tell me a bit more about how I should escape (sanitize?) this.

Yes that was my idea, see https://stackoverflow.com/questions/16860287/security-comparison-of-eval-and-innerhtml-for-clientside-javascript as an example.

Ok, but you could add it if it does not break Chrome, right?

  • I didn't understand what you meant by "incorrect function call" so if what I copied from your version is not enough, please elaborate.

I think you changed it already.

Thanks for adding the link "I already downloaded the ISO."
I just tried everything again and I have some comments on UX. Please add them yourself to relevant tickets if you agree:

  • When I see "Verification failed" I immediately had the feeling that I want to know why. Could we add something like "Checksums did not match"? Or something similar?
  • Actually I know now that the verification failed because I tried to verify Tails 3.2 and the checksum was matched against 3.3 - maybe we can make it clear somewhere what we compare or when people want to compare an older version tell them that this is not possible?
  • When I installed the extension, the "Verify" button becomes visible. But IMO one can easily miss this modification. Would it be possible to make it more visible? Either by using another colour or by making this button bigger or animate the background colour or something similar?

#7 Updated by sajolida 19 days ago

  • Status changed from Confirmed to Resolved
  • Assignee deleted (sajolida)
  • QA Check deleted (Dev Needed)

Cool! I moved your comments to #14998, #14997, and #14921.

I can close this ticket now.

Also available in: Atom PDF