sindresorhus / refined-twitter

Browser extension that simplifies the Twitter interface and adds useful features
MIT License
1.31k stars 89 forks source link

Don't touch other downloads with the download filename handler #189

Closed CollinChaffin closed 3 years ago

CollinChaffin commented 5 years ago

Please see this picture which occurs when downloading FROM SITES OTHER THAN TWITTER:

SNAG_6-22-2019_09-24-21

Not only that, it is auto-generating (aka overriding) the native auto-generated download name to "download.htm". This was on a non-Twitter site when attempting to ALT-click a file to force FDM to download it.

IMO this is a HUGE security issue. Why on earth does this extension need this permission?:

SNAG_6-22-2019_09-28-22

Yes, I have no issue granting full permission to Twitter sites, and yes if THOSE sites are all it acts upon (including downloads) then I also have no issue granting permissions for manipulating Twitter downloads (but actually prefer it didn't at all). I do NOT, however, want this extension touching ANYTHING outside of those sites and clearly it is due to either a bug or abuse of this blanket global download permission. This is beyond a bug that really needs to be corrected ASAP.

CollinChaffin commented 5 years ago

Also see my comment on #150 a simple url rewrite is all that is required I have userscripts that do this without ANY browser permissions or actions whatsoever.

jorgegonzalez commented 5 years ago

Feel free to open a PR if this is a simple fix

sindresorhus commented 5 years ago

We use the permission to fix the image filename: https://github.com/sindresorhus/refined-twitter/blob/3e20093ef7dae7819ab237e18fd6d3d974037394/source/background.js#L23-L28 I didn't realize it would affect other sites. That's very surprising.

The fix would be to check the absolute URL that initiated the download in that handler and make sure it's from Twitter: https://developers.chrome.com/extensions/downloads#type-DownloadItem


Removing it is not a good fix as it's a useful feature. Guarding it is enough.

mtlaird commented 5 years ago

You don't need to fix the filename, anymore. Twitter changed the way they load images, so they don't display images as "jpg:large" or "jpg:orig" anymore. This is what a twitter filename looks like to me, now:

https://pbs.twimg.com/media/D9sMEzzXUAAX_Yr?format=jpg&name=4096x4096

At least on my browser, this saves fine as a normal jpg, so the plugin no longer needs to do anything with image downloads (unless other people's twitter web experience is different than mine).

CollinChaffin commented 5 years ago

Sorry I've been behind. I will absolutely work on a PR using my userscript perhaps as a guide but I also fully agree with @mtlaird in that perhaps you want to look at whether it really is even a needed feature anymore since the naming has now changed. Obviously if it's not required for naming, then that will really simplify things and as far as I can tell, remove the entire need for this permission or any of the logic.

Let me know @sindresorhus if after reviewing @mtlaird info on the current naming whether I even need to work on a PR or if you think perhaps it can just be eliminated. TIA!

chrcoluk commented 5 years ago

I dont know if its related, but I just had to disable this extension as it is preventing chrome from remembering download location "globally".

All downloads are sent to the configured download folder only instead of chrome remembering the last location.

LesterCovax commented 4 years ago

Came here to submit an issue for this, but it seems I'm not alone. It's interfering with another (actual) download manager. Going to have to keep it disabled until this is fixed. Although the idea of limiting its scope to twitter URLs is good, my first thought was to include an option to disable its download features. I'm guessing that may not work though as the permissions are baked into the extension install.

refinedTwitter
craigeley commented 4 years ago

Just another user who had to troubleshoot for a while to figure why a site that I regularly download SRT files from was converting them to HTML. This is the culprit! Disabled until a fix can be issued, but I agree with the above—this "feature" is no longer useful and should just be disabled.

sindresorhus commented 3 years ago

Closing as this extension is now deprecated: https://github.com/sindresorhus/refined-twitter/commit/8dd9df749d3a4e82c95b2bd6628c8d65f0f72e6c