search

woodruffw / ff2mpv

A Firefox/Chrome add-on for playing URLs in mpv.
https://addons.mozilla.org/en-US/firefox/addon/ff2mpv/
Other
521 stars 52 forks source link

Feature: cookie support #52

Closed jzbor closed 2 years ago

jzbor commented 3 years ago

Implementing #19 for the python backend. Improved version of #20.

jzbor commented 3 years ago

Please note that I haven't implemented opening the url with (optional) cookies on addon icon click yet. I am completely new to browser extensions and not quite sure how to do this the right way, but it should be trivial to fix this.

woodruffw commented 3 years ago

Thanks; I'll review this tonight.

jzbor commented 3 years ago

I will probably also add a feature to append your own ytdl-raw-options, cause I for example have a problem with IPv6 on my network so it would be nice if you could add you own options. I will also look into documenting these config files in the README.

jzbor commented 3 years ago

Ok it seems avoiding the overwriting of ytdl-raw-options was much easier than creating an extra config file.

jzbor commented 3 years ago

Any clue, why the linter keeps crying?

jzbor commented 3 years ago

@woodruffw Do you have an ETA on the review?

woodruffw commented 3 years ago

Sorry, I've been pretty busy. Taking a look now.

woodruffw commented 3 years ago

Any clue, why the linter keeps crying?

I pushed an update to master that should make the lint-python job more verbose. If you git rebase, you should get more explicit messages from it.

Alternatively, you can pip3 install black and run the linter locally; you should get the same results.

woodruffw commented 3 years ago

As a high level concern: I understand your rationale for using regexps in the allowlist, but I think the end result will be a large number of users adding .* or similar, and thereby unconditionally sharing their browser cookie state with any requests made through youtube-dl. That's absolutely necessary for some services, but I think it's a potential information hazard in the general case.

For the time being, I'm inclined to say that the initial version of this should just be a domain list, with direct matching against the requested URL's host. We can reevaluate including regexps (or a subset, like glob-style patterns) once users actually begin to report services that benefit from it.

jzbor commented 3 years ago

But why drop a feature, that we have already implemented? I really like the regexes, cause I dont have do add every subdomain my university feels like spreading lecture videos across. I think promoting safe use of regexes, for example adding an explicit disclaimer not to use .* should be fine. I think the user should be allowed to do stupid things if he definitely wants to.

jzbor commented 3 years ago

I also just discovered another advantage of regexes: Passing youtube cookies only when playing (potentially private) playlists. That way you can use private playlists, but don't pass your cookies elsewise.

boi4 commented 3 years ago

@woodruffw You also have to remember that only the cookies associated with the video domain are passed to mpv (I think this could actually lead to some failures, e.g. if the youtube-dl extractor needs to do requests to other domains that need cookies, however this is rather unlikely). So the only information you give the site is that you are now using a different browser/video player to access the content. One problem I see with regexes is that the period "." is used in a lot of domain names and could lead to some problems. I think that the best way is to use glob patterns, similar to a gitignore file.

jzbor commented 3 years ago

One problem I see with regexes is that the period "." is used in a lot of domain names and could lead to some problems.

Yeah I have thought about that. The thing is you can only get false positives, as regex '.' also matches the character '.'. So youtube.com/.* would still match youtube.com/somevideo. I have also provided some examples in the README. I think regexes are nice, because they are powerful. For example you can add multiple superdomains without whitelisting all of them (.*\.google\.(de|com)/.*). Of course this particular example isn't a regular use case, but that is the point: It is hard to know the usecases of all users and this way it is as flexible as possible. You don't have a new issue every month, just because a user wants to do something fancy with their whitelist.

jzbor commented 3 years ago

@woodruffw Any feedback/update/whatever?

woodruffw commented 3 years ago

Sorry for the delay. I'll push some corresponding changes to the Ruby client this evening.

For my 0.02c: I agree with @boi4 that glob patterns make the most sense here. Both Ruby and Python have standard library bindings for fnmatch(3), so let's go with that.

jzbor commented 3 years ago

Ok sounds good to me :+1:

woodruffw commented 3 years ago

Could you grant me write access to your branch? I have some fixes to push up.

jzbor commented 3 years ago

Tbh I don't know how granting write access for a branch works, but I invited you as a collaborator... that should work

boi4 commented 3 years ago

What is missing for the merge?

woodruffw commented 3 years ago

Only some more of my attention (sorry, I’ve been very busy) and a few changes that I have locally. I’ll have some time tonight/tomorrow to do those.

Sent from mobile. Please excuse my brevity.

On May 20, 2021, at 8:48 AM, Jan Fecht @.***> wrote:

 What is missing for the merge?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

woodruffw commented 3 years ago

I took some time to clean this up. I'll give it another round of tests tonight/tomorrow, and then I'll commit the Ruby client changes to this branch as well. Thanks again for bearing with me, I know it's been pretty slow.

jzbor commented 2 years ago

So is this ever going to be merged?

woodruffw commented 2 years ago

Sorry, it's fallen off my radar. I think it's diverged a bit with the recent changes, so that would need to be fixed up.

More generally, I've gotten more conservative as a policy around these sorts of changes: I think equivalent functionality here could be accomplished with a special MPV profile that passes a cookie file into youtube-dl.

For example, you can patch your native client to run mpv as mpv --profile=ff2mpv ... and then, in your MPV config:

[ff2mpv]
ytdl-raw-options=cookies=/path/to/your/cookiefile.txt

Could you see if that works locally for you? If it does, then the "don't duplicate preexisting MPV/youtube-dl functionality" policy will apply.

jzbor commented 2 years ago

AFAIK that would require manually exporting the cookies on each session, which is not worth it imo.

woodruffw commented 2 years ago

AFAIK that would require manually exporting the cookies on each session, which is not worth it imo.

I'm not sure I understand -- which part requires manually exporting the cookies each time?

I think the workflow would be identical: you'd export your cookies to a text file once, and then youtube-dl will take care of loading it, parsing it, and sending the appropriate cookies for the domain being requested.

jzbor commented 2 years ago

Some websites seem to not give you a cookie that is valid for long, but instead just a short-lived session cookie, where you have to log back in after some time.

woodruffw commented 2 years ago

Some websites seem to not give you a cookie that is valid for long, but instead just a short-lived session cookie, where you have to log back in after some time.

I understand that, but I'm not understanding how the approach in this PR makes that better. In both cases the website is ultimately in control of cookie expiration, and in both cases you'd need to update your local cookie file.

Sorry, my initial response was wrong. I forgot that this changeset includes the cookie permission to grab the site's cookies directly.

woodruffw commented 2 years ago

Okay, here are the different tradeoffs as I see them.

This PR's approach:

The "profile/cookiefile" approach:

jzbor commented 2 years ago

I am sorry. I don't want to waste any more time on this. The cookie thing was the only reason for me to use this addon anyway, as mpv supports drag-and-drop for urls anyway (so I guess you could consider your whole addon duplicating mpv features). You clearly don't want to support this feature, which is fine, it is your project, but I don't see any reason for keeping this PR open and wasting my time with it anymore.

jzbor commented 2 years ago

Just to clarify AFAIK this PR wasn't implemented using any cookie file, but rather sending the cookies through the IPC so they get transferred individually on each launch without requiring a cookie file.