subdavis / Tusk

🐘 🔒 KeePass-compatible browser extension for filling passwords.
https://subdavis.com/Tusk
Other
482 stars 74 forks source link

google drive permissions are overly broad #188

Open rspier opened 6 years ago

rspier commented 6 years ago

This issue is a

Please describe the current behavior, and explain why it's bad.

Currently, for Google Drive, tusk requests: "Read and change your data on all googleusercontent.com sites, accounts.google.com, and www.googleapis.com"

That's a bit scary.

Please describe how you think it should change.

Scope should be limited to only the files requested.

The https://www.googleapis.com/auth/drive.file scope might be better, although it has the disadvantage of being read-write.

Anything else?

It's not clear why Tusk is getting modify permission. https://www.googleapis.com/auth/drive.readonly should be read-only.

Also, why does it need all of the origins? "https://www.googleapis.com/", "https://accounts.google.com/", "https://*.googleusercontent.com/*",

subdavis commented 6 years ago

Frankly this answer is more beuraucratic than technical.

It took me four weeks of begging and yelling at Google's developer support comtractor to get Tusk approved because I can't prove ownership of the oAuth origin (browser extensions are all like this).

Once I finally got the access needed, I put it in and didn't look back. I can revisit the read-only permission, so long as it doesn't require permissions declaration in my developer console, since that would risk having to go back through the approval process. I hate Google and really regret supporting Drive because of all the trouble it has caused.

As for the origin access, those are all completely necessary even for read only access.

The account endpoint is needed for auth. Googleapis is needed for the actual drive API calls, and user content is needed to finally fetch the file when we find it with API search.

I'll leave this issue open to investigate the read-only possibility. Keep in mind that it will change back in the future if and when database editing is supported, as this is the #1 requested feature.

I understand your caution. Thanks.

yacoob commented 6 years ago

As a person that wanted to move away from CKP, those overly broad permissions made me stay with it.

FWIW, should a "shared link" option work for a file stored on Drive? It seems to fail for me with:

Uncaught ReferenceError: getParameterByName is not defined
    at Object.googleGenerator [as drive.google.com] (options.build.js:15819)
    at SharedUrl.processSpecialSources (options.build.js:15841)
    at new SharedUrl (options.build.js:15796)
    at VueComponent.addLink (options.build.js:15867)
    at invoker (options.build.js:3587)
    at HTMLAnchorElement.fn._withTask.fn._withTask (options.build.js:3386)
subdavis commented 6 years ago

those overly broad permissions made me stay with it.

The permissions CKP requires are identical. Line 86 on https://github.com/perfectapi/CKP/blob/develop/services/googleDrivePasswordFileManager.js

I can't tell if I just misread that comment, but CKP is very dangerous to use because of https://subdavis.com/blog/jekyll/update/2017/01/02/ckp-security-flaw.html

yacoob commented 6 years ago

Oooh, interesting. I must have forgotten this. Thanks for pointing this out.

rspier commented 6 years ago

@subdavis Thanks for being willing to consider / look into reducing the scopes and breadth of access needed. Sorry about the frustration you had with the developer support. It's probably more complicated because you're doing everything dynamically (to support multiple backends) instead of just letting Chrome do it all up front.

https://developers.google.com/drive/api/v3/examples/#zip_extractor might be useful as an example that uses the file picker -- but only requests scopes that give it access to files that have been picked. It's not a drop-in solution, because you're not currently using the standard picker.

In some ways breadth of access is more important than readonly or not. (Especially as file editing may happen eventually, which would not be a bad thing.)

The shared link idea that @yacoob points out is also interesting, but comes with it's own set of issues.

Thanks!

-R