Skip to content
This repository has been archived by the owner on Nov 6, 2023. It is now read-only.

Excuse me if this isnt the appropriate place to ask this, but I would like to know why the plugin needs new FF permission: access to all data of any website #16377

Closed
Daij-Djan opened this issue Aug 23, 2018 · 26 comments

Comments

@Daij-Djan
Copy link

Hi,
Excuse me if this isnt the appropriate place to ask this, but I would like to know

why the plugin needs new FF permission: access to all data of any website?

thanks
Dominik

screenshot 2018-08-22 17 00 26

@faultywarrior
Copy link

I'd like to know this as well...I removed the add-on until confirm of the reason for this.

@cschanaj
Copy link
Collaborator

cschanaj commented Aug 23, 2018

ping @Hainish

@HybridEidolon
Copy link

same here, what's going on? the changelog on the EFF website doesn't say anything about a new permission being needed.

@csdev
Copy link

csdev commented Aug 23, 2018

Probably related to FTP blocking: 57a41c2

However, the commit message states that this feature is still not working on FF, so perhaps it would have been better to only request this permission on Chrome.

@CodyWilson
Copy link

However, the commit message states that this feature is still not working on FF, so perhaps it would have been better to only request this permission on Chrome.

That commit was my guess. But I think what's likely happening here is we've already given the permission to "access to all data of any website" (Which I'm pretty sure is what *://*/* does, prior history shows that it was <all_urls> before, which would require that permission) and for whatever reason firefox saw the new rule and asked for it yet again.

(Either that, or Firefox always asks for that on an update, and I just hadn't paid attention before)

Which is why the EFF website wouldn't necessarily say anything about a new permission, because we had already granted that permission before.

@cw64
Copy link

cw64 commented Aug 23, 2018

So permission was already had, and has always been had?
This is needed to rewrite the https requests?

What is your privicy policy? Please don't send me to a wall of text, just tell me if you are:
a) looking (at/for anything other than the minimum of what is required to rewrite the https requests)
b) recording
c) selling

@lemenkov
Copy link
Contributor

I'd like to have an official explanation as well. Until then I blocked that update.

@Bisaloo
Copy link
Collaborator

Bisaloo commented Aug 23, 2018

Related: #13817

@johnp
Copy link
Contributor

johnp commented Aug 23, 2018

To sum this up (thanks @csdev and @CodyWilson) and expand a bit:

  1. It seems that this was originally <all_urls>, which effectively means (http|https|ws|wss|ftp|file)://*/*.
  2. This was then changed in this commit to *://*/*, which meant (http|https)://*/* (in Firefox >= 55 also (ws|wss)://*/*) and that reduced the set.
  3. The latest change added the ftp://*/* pattern which increased the set again and triggered the notification.

The Firefox notification seems imperfect, but any additional permission should be mentioned in the Changelog. If someone installed the extension at step 2 they didn't "agree" to ftp://*/* access, which has now been added.
I also found that Firefox has a testcase for this exact permission change and the test expects failure, i.e. the notification is expected to arise in this case.

Documentation Match Patterns

@CodyWilson
Copy link

CodyWilson commented Aug 23, 2018

This is needed to rewrite the https requests?

It looks like it would be needed to me. Otherwise, how would they add the handler to block/redirect non-https requests on all urls?

You can read what the onBeforeRequest handler does here:
https://github.com/EFForg/https-everywhere/blob/master/chromium/background-scripts/background.js#L291

I haven't found much that does recording. The util.log only calls console.log with various warning levels.

What is your privicy policy? Please don't send me to a wall of text, just tell me if you are:
a) looking (at/for anything other than the minimum of what is required to rewrite the https requests)
b) recording
c) selling

I know you said not to, but the privacy policy is in plain English. https://www.eff.org/code/privacy/policy

From what I've read of that, and the code thus far.

a) Not that I can see throughout the code. There's nothing looking through request bodies or anything weird like that.

b) Can't say I've read every line of code. But I've yet to see anything that records anything, really.

c) I can firmly say: "Probably not." There's nothing in their privacy policy that allows them to "sell data" per se. But, they can share it with research partners. So take that as you will.

Edit: Looking at old (and still open) discussions here and here, it seems that I'm correct in not finding any form of telemetry or otherwise in the code base.

@Hainish
Copy link
Member

Hainish commented Aug 23, 2018

This was an unexpected prompt on the part of Firefox. My guess is that this is due to a permissions change since the last version of the extension:

user@https-everywhere ~/workspace/https-everywhere (master) $ git diff 2018.6.21 chromium/manifest.json
diff --git a/chromium/manifest.json b/chromium/manifest.json
index 10b776f062..ccf65918c2 100644
--- a/chromium/manifest.json
+++ b/chromium/manifest.json
@@ -48,7 +48,11 @@
         "tabs",
         "cookies",
         "storage",
-        "*://*/*"
+        "*://*/*",
+        "ftp://*/*"
     ],
-    "version": "2018.6.21"
+    "version": "2018.8.22",
+    "web_accessible_resources": [
+        "/pages/cancel/index.html"
+    ]
 }
\ No newline at end of file

Adding ftp://*/* allows us to block ftp requests when the strict "Block all unencrypted requests" option is checked (#15813). This may have triggered the warning.

Another possibility is that the warning was triggered by the new "web_accessible_resources" key that was added in order to redirect users to an additional info page when they have the "Block all unencrypted requests" option enabled and they are prevented from viewing a page because only the http version is available (#16053). You can test this functionality by enabling the "Block all unencrypted requests" option and visiting http://http.badssl.com/.
Edit: the above addition of ftp://*/* triggered the warning.

Note that when upgrading the extension on the self-hosted version (downloaded from https://www.eff.org/https-everywhere/) I did not receive this warning. This is odd, and may be only because it is a self-hosted version.
Edit: the above is due to the fact that I was testing on Firefox ESR 52. This permissions dialogue is only triggered in Firefox 57+.

I apologize for this unexpected and frightening warning! I can certainly understand why users would be concerned about this, and I'll be reaching out to Firefox to follow up on the dialogue. I'll report back when I have additional information.

@Hainish
Copy link
Member

Hainish commented Aug 23, 2018

@jvillalobos can you clarify here?

@jvillalobos
Copy link

I think it was the addition of "ftp://*/*" that triggered the permission prompt. Firefox prompts for any permission addition in new versions (it shouldn't matter how it's hosted). If you have any suggestions for things we should change there, please file a bug.

@Hainish
Copy link
Member

Hainish commented Aug 23, 2018

Thanks @jvillalobos. This permission seems really misleading - for one, the addition of that permission doesn't even effect websites at all, only FTP directories. Secondly, permission to redirect URLs is already established in previous versions of the extension - why add a scary new warning now?

@jvillalobos
Copy link

Firefox always prompts for permission changes. This isn't new. But I get the confusion of ftp not being included in *, and Firefox grouping site permissions. Maybe worth filing a bug, but it's also a very unusual case.

@riking
Copy link

riking commented Aug 23, 2018

A mention of "It already has access to:" would be nice here. (Something for Firefox to fix.)

@Hainish
Copy link
Member

Hainish commented Aug 23, 2018

@jvillalobos as a more general point, I agree with this post that, while well-intended, the "technically-motivated security-conscious users will avoid downloading and using most Firefox add-ons" and "everyone else will simply ignore all the permission requests and install any add-on they want".

I realize presenting a meaningful UX around permissions is hard, but in this case the warning is far disproportionate to the actual permission being asked, to the point that the entire statement is false:

"Access your data" is not something the extension does, nor what the permission asks.
"for all websites" is also false, since it is not for websites at all in this case.

I sympathize with the challenges that Firefox faces here, but this seems like a UX failure, and the only one it injures is potential HTTPS Everywhere users who have been scared off by this.

@CodyWilson
Copy link

CodyWilson commented Aug 23, 2018

The problem with "It already has accessto:" here, is that it doesn't.

It has access to http/https schemes via: *://*/*, FTP isn't included in it per the document @johnp linked. https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Match_patterns

A better solution might be to break up that "access to all data of any website" permission into more specifics. Such as "access to all http/https requests", and "access to all ftp requests".

An even better solution would be to break that up even further.

Like such:

"permissions": {
    "*://*/*": [
        "redirect", 
        "read",
        "write",
        "*"
    ]
}

Such that a permission might show up as "Able to redirect any HTTP/HTTPS URL", "Able to Read/Write on any HTTP/HTTPS website".

Obviously that's not a trivial thing to implement, with such granularity in permissions. But since @Hainish brought up that the entire permission is pretty inaccurate, I figured I'd throw it out there.

@jvillalobos
Copy link

Please use the Bugzilla link I included before to request any of these changes. I'll also note that I'm Product Manager for the add-ons site, not Firefox, so I can't really say much about the product and UX decisions that lead to this. Just trying to be helpful here :)

@Hainish
Copy link
Member

Hainish commented Aug 23, 2018

@jvillalobos thank you for being responsive, I do appreciate it.

@Dutchgold
Copy link

Dutchgold commented Aug 23, 2018

https://github.com/Rob--W/https-by-default <<<<<<<For trying to sell my information to 3rd parties you have been replaced,id suggest everyone else do the same. HTTPS BY DEFAULT in firefox add ons

@CodyWilson
Copy link

CodyWilson commented Aug 23, 2018

@Dutchgold
I doubt you'll find so much as telemetry in this plugin. As I've yet to find it (And I'm not an EFF member), and I spent several hours digging through the code.

Before you fire alarm bells that your data is being sold by an organization whose privacy policy doesn't allow them to do that, at the very least point out the offending code that collects it.

Btw, that plugin requires the same permissions, minus the FTP handler.
https://github.com/Rob--W/https-by-default/blob/master/firefox/manifest.json#L15

So if you don't trust the EFF with that, I don't know why you'd trust some random on the internet with it.

@Dutchgold
Copy link

this warning is telling people that the postman is reading your post.time will tell.disabled for now.

@CodyWilson
Copy link

CodyWilson commented Aug 24, 2018

this warning is telling people that the postman is reading your post

It's telling you that the postman can read your post.

But unlike the postman, you can read what the process is doing.

It's actually rather straight forward javascript. It's not like we're trying to understand GNU's implementation of cat.

As well, maybe you don't understand plain English that spells out for you that your alternative is using the exact same permissions. So here, lets use an image.

stare-into-the-circle

@EFForg EFForg locked as off-topic and limited conversation to collaborators Aug 24, 2018
@Hainish
Copy link
Member

Hainish commented Aug 24, 2018

Thanks for the lively discussion everyone, I'm going to call it and lock the conversation so that users being directed to this thread will get information relevant to the upgrade process for the extension. If there is a separate bug, please file a separate issue.

@Hainish Hainish closed this as completed Sep 21, 2018
@ghostwords
Copy link
Member

Please use the Bugzilla link I included before to request any of these changes.

I filed "Adding ftp ws/wss permissions should not trigger extension permission warnings on extension upgrade" in Bugzilla.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests