Supporting "Switch HTTPS" might be a pain to implement---I am assuming you want to add some sort of per-session HTTPS Everywhere entry. Also, the website might not support HTTPS so this might break that workflow. So, I suggest keeping it simple and using the same backend as "New Circuit for this Site".
Our detection will probably have false positives, so we might want to word it cautiously and include the copied text so that user can make the best judgement.
It might be worth adding a about:preferences toggle for this and letting "Advanced Security Settings" point to that toggle. (I am flexible on this.)
For HTTPS, I was imagining just rewriting the url with https; not creating a HSTS entry. Yes, this could have breakage, but it would only occur after a false positive which I think would be rare to non-exsistant.
Sure, we could let the user turn it off if they wanted; but I think it should be on by default.
Currently, the extension (see https://github.com/sgmenda/badbitcoinaddr#how-it-works) flags any 25 character base58 decodable string starting with a 1 or 3 as a bitcoin address. I think 25 character base64-type strings are rare, but it might be worth being cautious. Also, good point about onion services.
On a related note, we can make the parsing better (like checking SHA256 checksums) but I'm concerned that that might add a noticeable delay. We can check this theory once we have a better prototype.
Rewriting the entry seems like a good compromise.
On second thought, I am gonna rollback on the easy to disable part, if implemented ideally (low false positives), the user may not want to disable this, and the few that want to might be able to dig up the setting.
I'm not entirely sure, but this might be easier to implement directly as a patch in Tor Browser. I don't think this would make rebasing patches more difficult for us, since the patch should be pretty much independent (except probably some minimal registration code in BrowserGlue.jsm and some common moz.build files). It would probably require to build the firefox part of TB, but I hope that would not be a big problem for you.
For the IPC with the content that you mention, you could follow the Actor pattern already in Firefox, see *Child.jsm and corresponding *Parent.jsm in JSWINDOWACTORS in BrowserGlue.jsm (it's better that than LEGACY_ACTORS, as those are gone in non-esr Firefox).
From your mockups I understand that they would involve merging the crypto warning UI with the existing Security Level UI. I think there's some value on keeping the crypto warning UI as independent as possible of existing Tor Browser UI and patches (in this case, the security level patch), at least for a start. We might consider at some point merging it (e.g. if we end up suggesting users to switch to a higher security level), but I think it's safer to start with something independent, as this patch might be ready before we have a https-only mode in ESR78.
For the prompt, you may consider using a Firefox notification (with PopupNotifications.show(...)). We use it in a couple of patches in Tor Browser already (you can check the #21952 (closed) patch for an example with no additional markup or #30237 (closed) for one where the notification is tweaked a bit). Not completely sure if it's the perfect UI for a warning (maybe the button colors can be changed?), but it should be easy to show programatically without any additional HTML/CSS and it will have a native Firefox look by default. Not sure what would be the best place to anchor it, though. Maybe the broken urlbar padlock?
Just as an example, this is Firefox usage of PopupNotifications for password leaks warnings:
I don't think doing SHA256 on the copied bitcoin address should have any noticeable impact.
Would it be feasible to handle at least another crypto address case (maybe Ethereum)? If not (e.g. too many 3rd party deps), it might be good to write the patch in a way that can be extended later to handle cases other than Bitcoin addresses.
I'm not entirely sure, but this might be easier to implement directly as a patch in Tor Browser. I don't think this would make rebasing patches more difficult for us, since the patch should be pretty much independent (except probably some minimal registration code in BrowserGlue.jsm and some common moz.build files).
Thanks for the suggestion. I am not familiar with this part of the Firefox codebase, lemme look deeper and get back.
It would probably require to build the firefox part of TB, but I hope that would not be a big problem for you.
I am not sure if this is a reference to the SVG patch. Anyway, until recently, I was only using Windows, but a couple of days ago (for this reason) I setup a dual boot with Ubuntu so I shouldn't have much trouble with it.
From your mockups I understand that they would involve merging the crypto warning UI with the existing Security Level UI. I think there's some value on keeping the crypto warning UI as independent as possible of existing Tor Browser UI and patches (in this case, the security level patch), at least for a start.
After looking at the code, I think I agree.
For the prompt, you may consider using a Firefox notification (with PopupNotifications.show(...)). [...]
Thanks for the suggestion and the attached patches, this looks promising.
Would it be feasible to handle at least another crypto address case (maybe Ethereum)? If not (e.g. too many 3rd party deps), it might be good to write the patch in a way that can be extended later to handle cases other than Bitcoin addresses.
From wikipedia it seems like Ethereum addresses are 20 byte hash outputs in hex with the "0x" prefix. This shouldn't be hard to include, but it might have more false positives.
From their docs it seems like Monero uses their own variant of base58. If we want to go the crude route, this stackexchange answer provides a regex.
Hi, I quickly iterated over your ideas. Some comments:
Let's simplify the copy. The current copy proposal seems to be attacking the user. I like how warning pages use "What can you do about it" section. Let's replicate that pattern. My suggestion A cryptocurrency address has been copied from an insecure website. It could've been modified. What can you do about it? You can try reconnecting with a new circuit or dismiss this warning if you trust this website.
Copy Anyway is tricky. Let's allow users to dismiss the warning.
We don't need users to go to security settings at this point. Am I missing anything? See v1.
If we deploy this, we will need a support.tpo.org entry about this feature. Could you help me with that content?
The current warning popup doesn't look like a real warning. I've tried a version using the warning color palette in Photon. See v2 and v3.
In the short future, the intent should be "Upgrade your security level", where HTTPS will occur. See v4.
For patching torbutton, I am having trouble figuring out where the addEventListener for copy should go. I tried putting it in a bunch of places in the existing content scripts with no avail, then tried adding a new content script, but I couldn't get it to load. I would much appreciate any insights here.
I am not sure if this applies to the TorButton extension, but the Webextension UI docs on Popups (which seems to be the UI element closest to the mockups) say:
However, you can't open the popup programmatically from an extension's JavaScript; it can be opened only in response to a user action.
I got the basic functionality working, see attached screenrecording (it is crappy but I hope it conveys a sense of the current state). I still need to:
get the UI to look more like the mockups,
improve the crypto address recognition (currently I am only using regexes and for the most part it seems to be fine, Bech32 (a kind of Bitcoin addresses) don't seem amenable to this though, so I am going to add an external module for them),
make it work only on non-onion, HTTP sites (currently, for testing purposes, it works only on HTTPS sites, but this should be easy to fix), and
make it work only when a pref is active (this should also be easy to fix).
I also noticed that this breaks on https://donate.torproject.org/cryptocurrency/ and https://freedom.press/donate/cryptocurrency/, I need to look into why that is happening. My quick guess is that these sites are doing something funky with scripts and not sending a vanilla "copy" event (from some quick fiddling I noticed that they do not set document.getSelection()).
h/t acat for digging up the script used to get "New Circuit" working. Thanks!!
Update: I improved the bitcoin address recognition by adding a stripped-down version of the reference Bech32 decoder, added an exemption for onion websites, and disabled this feature under the hidden pref security.cryptoSafety.
I spent some time trying to get the UI to look like antonela's mockups, but it might be harder than I thought. See this comment from a few days ago. I essentially need to hook a window to a webextension icon and that might be hard to do. My current plan is to look at how the browser does it in response to user action (that is, what exactly does the browser do when you click on the shield) and try to replicate it programmatically.
h/t tom for helping me investigate the copy shenanigans. Thanks!!
*For the video, I modified the patch to work on HTTPS sites but the real patch only works on HTTP sites. (I have also tested independently that the real patch works on HTTP sites.)
I think it would be good to dig in a little more into why it doesn't work in some cases. I pm-ed you some ideas.
I'm supportive of moving forward with the current UI, getting it into Nightly/Alpha for testing and riding the trains, and seeing if we can improve the UI without holding back on shipping.
I think it would be good to trim long addresses to a reasonable number of characters and add an ellipses...
I also think we should develop the page where the Learn More link is going to go, and start writing that copy.
*again, modified the patch to work on HTTPS pages to demonstrate the fix.
Some Details
We now hook to ContentParent::RecvSetClipboard which captures all clipboard changes, but is very noisy (for instance, I noticed that it is called every time I select some text), I work around this noise by keeping state of the last crypto address (for which a prompt was shown) and only showing the prompt on a new address. But this has the effect of showing false positives, like if you copy a crypto address from a HTTPS site, navigate to a HTTP site, and select some text, it would prompt. (This situation seems unlikely to happen in practice to me.)
We also need to use the parent process and content process message managers and "manually" listen to the clipboard-change event because of Bug 1644992 (which says that, if we want to use the JSWindowActor protocol, we need a window, which we don't have in ContentParent::RecvSetClipboard). Thankfully, this doesn't complicate the code that much. (I am mentioning this here because this wasn't obvious to me and this information might come in handy elsewhere.)
Since we are not using the JSWindowActor protocol, the Child module is not dynamically instantiated on the clipboard-change event. So, we need to manually instantiate it. I chose to hook it to the DOMContentLoaded event like the AboutReaderChild module (this is wasteful since we instantiate this module for every frame but the pref impact seems minimal (it adds <100 lines of JS to every webpage) so I am not going try to optimize this.
Here is a screenshot of the prompt. Maybe @acat can suggest a workaround but the given API for the popup notification takes a string (not HTML or anything that allows for custom formatting) so it seems hard to do line breaks or boldface required to reproduce the mockups as-is.
I wrote a first draft of the support entry. I am not sure about the level of detail I should go into and what the recommendations for website owners should be (I mentioned HTTPS Everywhere but there might be something else.)
IMO it might make more sense to consider this as a crypto feature rather than a cryptocurrency feature. I.e. I think OpenPGP and Signify keys should be considered untrustworthy in a similar way to Bitcoin addresses in this circumstance. (Slightly different wording in the error message might be warranted though.) There's no need necessarily to implement every crypto pubkey/fingerprint format at the same time, but it does seem like it would be wise to at least keep this code generalized enough that we could later add things like OpenPGP and Signify support without needing major reworking. (In particular, I would recommend using "crypto" terminology rather than "cryptocurrency" in the code.)