search

danwent / Perspectives

Perspectives Firefox Extension
http://perspectives-project.org
66 stars 19 forks source link

Highly visible notifications when certificates fail #96 #124

Closed ghost closed 10 years ago

ghost commented 10 years ago

Issue #96

Added notifications for

  1. ti.query_results.inconsistent_results && !ti.query_results.weakly_seen
  2. ti.query_results.inconsistent_results

Other than that, I think this issue has already been resolved?

daveschaefer commented 10 years ago

Hey, thanks for sending this!

I have a few other deadlines in the next few days, so my apologies if I don't get to this right away.

daveschaefer commented 10 years ago

Fun tip: you can add '?w=1' to the end of the URL to see diffs without whitespace

ghost commented 10 years ago

Thanks for the tip, I was looking for that option. Btw: This is my first open-source contribution ever, so if you have more of this, I'd be glad for more tips!

daveschaefer commented 10 years ago

Alright, I will attempt to go through this one commit at a time and approve the things that are definitely good. Perhaps we can pull some of the changes into a different branch and merge them so you don't have to wait for everything to be approved.

Github tells me that it is unable to automatically merge the entire pull request. That's likely due to the MD5 commits that went in recently.

daveschaefer commented 10 years ago

Okay, more down and seven commits to go :) However, they are the larger ones.

Can you please create some unit tests for eliminating duplicate notary entries? If you're not comfortable with that or don't want to I can create some after the merge.

daveschaefer commented 10 years ago

Also, are you able to see where I'm leaving my comments? The github interface for following along seems a bit unwieldly

ghost commented 10 years ago

Can you please create some unit tests for eliminating duplicate notary entries? If you're not comfortable with that or don't want to I can create some after the merge.

Sure. I'm taking notes of every request in the code reviews and I'm going to work them off once the merge is finished. Here's what I got so far (not-prioritized!):

  1. coding guidelines
  2. change localization library
  3. remove the hack of checking against a localization string
  4. bundle together the 'apply tooltip' and 'setStatus' code
  5. remove the test cases from the manual test case list
  6. unicode wrapping für base64 encoding
  7. remove "Tor" from preferences string
  8. add unit tests for duplicate notary elimination in preferences
  9. typo in readme
  10. revert functional ifs to procedural ifs (qd_str in notaries.js)
  11. spaces between if and parenthesis

I'm probably deciding against 1, 2 and 11.

Also, are you able to see where I'm leaving my comments? The github interface for following along seems a bit unwieldly

I can see the comments as a stream in this pull request and also I receive them as mails. The mails each contain a link saying "Reply to this email directly or view it on GitHub." which forwards me directly to the commit and file (click on the hash).

ghost commented 10 years ago

What I can't see or find are the merge conflicts.

daveschaefer commented 10 years ago

General comment: I built and ran your code locally from your branch and ran into a few minor issues - noting them here so you can edit them.

ghost commented 10 years ago

I fixed the test case and "Distrust all certificates". Thanks for finding these bugs.

concerning the Base64 codex: When I use escape/unescape my IDE complains that the functions are deprecated and when I run the snippet from Mozilla I get an error using the sig_base64 string. I read the documentation again and these safety nets are only required for "actual Unicode strings". Apparently the sig_base64 is a byte array and hence we shouldn't require them? The signature originally gets produced in Perspectives-Server notary_http.py:

packed_data = service.encode() + struct.pack("B", 0) + packed_data
sig = crypto.sign_content(packed_data, self.notary_priv_key)
top_element.setAttribute("sig",sig)

preceded by a lot of bit shifting, so I assume yes. I also compared the results of atob/btoa with the original decode/encode functions and they match.

Minor edits are: removed "Tor" in localizations, typo in readme, removed debugger line

concerning the formatting: There are some code changes in your MD5 fix, some code cleanups in the "menubutton" branch, and even more cleanups and even architectural changes in the "tabinfo_restructuring" branch. As the formatting is not critical to the stability of the code I promise to tackle them once the branches are merged, okay? Otherwise, if I introduce even more reformatting and try to merge it all together I fear my brain might explode.

daveschaefer commented 10 years ago

Okay, if the non-wrapped base64 functions do the trick, let's run with those :) Thanks for checking and testing it.

[tackle whitespace and formatting later]

Fantastic point! It's not critical and if that's one step that will make this easier for you, that sounds good.

I'll have a look at your TODO list when I next have time (likely tomorrow).

daveschaefer commented 10 years ago

@lambdor Hey, your code is merged! :D

Amazingly, GitHub was able to detect the commits from both pull requests were the same and automatically closed this PR when I merged the other one. Nice.