claustromaniac / detect-cloudflare-plus

True Sight Firefox extension.
https://addons.mozilla.org/firefox/addon/detect-cloudflare-plus/
GNU General Public License v3.0
37 stars 3 forks source link

nits #12

Closed earthlng closed 5 years ago

earthlng commented 5 years ago

oh wow you've been pretty busy updating this, A LOT! :)

a couple things I noticed while glancing over the code:

I avoid browser.storage.sync like the pest in all my addons, just out of principal. Maybe use local as the default and add an option to use sync? something like

var storageArea = browser.storage.local;
if (settings.sync) storageArea = browser.storage.sync;

you get the idea. Or just get rid of sync because the extension comes with good default settings anyway and there's no real need to sync them IMHO.

ghost commented 5 years ago

there's no real need to sync them IMHO

It comes handy if the extension is being used amongst different browser instances.

I avoid browser.storage.sync like the pest in all my addons, just out of principal.

any particular concern for being matter of principle?

claustromaniac commented 5 years ago

browser.runtime.getBackgroundPage() in options/page.js doesn't work in private windows. Not terrible but not ideal either.

True. I recall having seen that warning, but I guess I postponed looking into that at the time... and then I simply forgot to do so entirely. I only need the settings object anyway, so I should be able to ditch that method for others in the runtime API without adding much more complexity. Will do so.

this.hasOwnProperty(i) ? val[i] = this[i] : val[i] = this.defaults[i]; in classes.js is prettier like this: val[i] = this.hasOwnProperty(i) ? this[i] : this.defaults[i];

:+1: I used your suggested semantics in other parts of the code (too lazy to check where). I suppose that one line was a side effect of one of the several sleep-deprived rewrites :sweat_smile:

As for storage.sync, I don't see any reason to avoid using it by default, considering that we can (and do) disable sync. Is there such a reason? I mean, I know you said you avoid it out of principle, but you know this Firefox thing better than I do. Do you know of any vulnerabilities or something that make storage.sync risky somehow?

Or just get rid of sync because the extension comes with good default settings anyway and there's no real need to sync them IMHO.

Maybe not now, but the extension will keep growing and I eventually intend to implement some advanced options that some people will probably want to sync.

BTW, did you merely skim the code or are you just going easy on me this time? If those are your only nits, I should be so f'ing proud, man! I don't have much experience with Javascript, and I learn stuff as I go. If that really is all you've got, then I must be doing better than I thought :laughing:

earthlng commented 5 years ago

Do you know of any vulnerabilities or something that make storage.sync risky somehow?

No but why risk it. I'm just not a fan of storing anything in the cloud. There's a dedicated webextensions.storage.sync.serverURL pref but I'm not sure if anything is ever sent there if Sync is disabled. I have that pref set to empty string and webextensions.storage.sync.enabled=false, just in case. It's fine if you keep it as is, it was just a suggestion after all and I can always edit my local copy.

did you merely skim the code or are you just going easy on me this time?

:smile: mostly just skimmed it. Those few things just stood out to me immediately. As for the rest of the code there are always a few different ways to do something but one isn't necessarily better than another - if it works it works and that's what matters. I really like your clean style and well organized structure. Using unique filenames like options/options.*and popup/popup.* would be nice though. Makes dev-ing a bit easier when having multiple files open in the editor. You're definitely doing a damn fine job here, as usual :smile_cat: :+1:

btw Via: 1.1 google is another good indicator for Google Cloud.

ps: Having just upgraded to your new and improved True Sight, it surely doesn't paint a pretty picture of today's internet. All this talk all the time about de-centralization but in reality the exact opposite is happening. And you can't even trust certs anymore. It's shady AF :(

claustromaniac commented 5 years ago

Having just upgraded to your new and improved True Sight, it surely doesn't paint a pretty picture of today's internet. All this talk all the time about de-centralization but in reality the exact opposite is happening. And you can't even trust certs anymore. It's shady AF :(

I hear ya. It wasn't until I got to work on this that I noticed just how serious that shit is. Yet, you look everywhere and web developers seem unanimously happy and super grateful that CDNs exist because they make the internet FASTER. It's like no one stopped to ask themselves at what cost. And that shit about handing them your private keys seems to be some sort of a standard these days, too.

At least I'm glad I'm not the only one concerned about this... We're not crazy, are we?

Anyway, thanks a bunch mate. I believe you'll be happy with that commit. I even renamed the popup and options files so they're unique again. Let me know if you ever have any other nits and/or encouraging words :smile:

claustromaniac commented 5 years ago

@n8v8R

It comes handy if the extension is being used amongst different browser instances.

This got me thinking. The way the extension works currently is that it loads the settings from the storage area on startup, and then it keeps them in memory, for performance. For that use case that you mentioned, the extension should instead read settings from the sync area all the time, otherwise the settings won't be in sync in your secondary instances until you reload the extension (by disabling + enabling it, or by restarting Firefox). At least, that's how I think it should be. I'll have to look into that...

EDIT: For a moment I thought this would take a lot of refactoring, but I forgot there is an onChanged event for this :heart: will definitely implement it.

earthlng commented 5 years ago

We're not crazy, are we?

Maybe it's just that we remember the good ol' days when the https icon still meant something and private keys were ... kept private maybe? IDK. Crazy, right?

IMHO the really crazy thing is that some of these "CDNs" offer their services for free. I can't help but wonder what kind of shitstorm would happen if these were russian companies.

Breaking News: Kremlin-sponsored companies are logging, storing and inspecting 90% of our internet traffic! And it's growing!! We now go live to our correspondent Rachel Anderson who is in 1 of the 159 2-minute-hate centers in DC to hear more about the story - right after this commercial break brought to you by Lockheed Martin

Damn, maybe I am going crazy :)

It's like no one stopped to ask themselves at what cost.

I guess they either don't fully realize what they're signing up for or they simply don't care. In a time where a large majority of the population post their whole lives on social media, who can blame them.

btw re: the Via: google header - the 1.1 is the HTTP version and could probably also be 2.0. Better to just check for "google" with indexOf or so. Sorry, I should've made that clearer in my 1st post.

ghost commented 5 years ago

I guess they either don't fully realize what they're signing up

This is telling on the side to the domain owners

https://blog.mozilla.org/security/2018/10/10/delaying-further-symantec-tls-certificate-distrust/

They are apparently unaware of the planned distrust

and then this thread

https://www.reddit.com/r/DemonoidP2P/comments/9m5hkk/securityprivacy_concerns_with_the_traffic_routed/

is hosted on 6 continents. Cloudflare is only seeing the reverse proxy which is the first layer. There are several more layers, hence Cloudflare can't get any information.

As for SSL, that is only a front-end flexible SSL certificate,


and this for the blissful ignorance of the user also from the last link

Wouldn't simply just getting a VPN resolve all these scary concerns? I don't think they would use something that could potentially spell something bad for it's members

Thorin-Oakenpants commented 5 years ago

it surely doesn't paint a pretty picture of today's internet

It's depressing - for me, almost every webpage sets off the counter badge. Might as well just state the obvious now then, and declare the entire web as MitM'ed

ghost commented 5 years ago

@Thorin-Oakenpants

suppose serving fonts. libraries, css and media files through a CDN makes sense in a many ways. Potential user profiling can be countered to a large extent by the user, at least by those being aware and know how-to.

Also protecting the domains against malicious actors with firewalls that run on powerful hardware, considering the scale of today's attacks with botnets and/or being run off GPU servers, is a legitimate concern for domain owners. I trust here is where the main issue, decrypting the TLS packets at the edge server (MitM) for packet inspection, starts. Suppose some domains owners do not want to be bothered with properly configuring a firewall in the first place and are happy to hand this off to a 3rd party, let alone the processes of intrusion detection/prevention and/or web application firewall which also require the necessary processing power.

The other issue of a CDN providing a SNI certificate for a multitude of unrelated domains is perhaps owed to convenience and saving the cost for domain owner, plus obscuring the hosting location of back-ends.

Domain ownerd sharing the private key of the TLS certificate with 3rd party providers for packet inspection and/or SNI certificates I find really bothersome and thence I am grateful that this WX is providing instant visual awareness of such potential scenario.