Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable use of session cookies in chrome #9747

Merged

Conversation

StefanLobbenmeier
Copy link
Contributor

@StefanLobbenmeier StefanLobbenmeier commented Apr 20, 2024

IMPORTANT: PRs without the template will be CLOSED

Description of your pull request and other information

Partially Fixes #5534

The chrome (and brave, have not checked other chrome based browsers) cookies database has 2 columns related to cookie expiry: expires_utc and has_expires. yt-dlp only checks the expires_utc column:

  • For session cookies expires_utc is set to 0 and has_expires to 0.
  • For expiring cookies expires_utc is set to a utc timestamp and has_expires to 1

The current logic in yt-dlp takes expires_utc only and the phyton cookie will consider a value of 0 as expired:

class Cookie:
    # ...
    def is_expired(self, now=None):
        if now is None: now = time.time()
        if (self.expires is not None) and (self.expires <= now):
            return True
        return False

To support session_cookies we should therefore set expires to None, since they do not expire.

Template

Before submitting a pull request make sure you have:

In order to be accepted and merged into yt-dlp each piece of code must be in public domain or released under Unlicense. Check all of the following options that apply:

  • I am the original author of this code and I am willing to release it under Unlicense
  • I am not the original author of this code but it is in public domain or released under Unlicense (provide reliable evidence)

What is the purpose of your pull request?

yt_dlp/cookies.py Outdated Show resolved Hide resolved
@StefanLobbenmeier StefanLobbenmeier marked this pull request as ready for review April 20, 2024 13:44
yt_dlp/cookies.py Outdated Show resolved Hide resolved
@bashonly bashonly added enhancement New feature or request needs-testing Patch needs testing labels Apr 20, 2024
@StefanLobbenmeier
Copy link
Contributor Author

StefanLobbenmeier commented Apr 20, 2024

Browsers tested on macOS:

Browser Result
Chrome
Chromium (cannot get chromium to start on my mac, not sure why)
Brave
Opera
Edge
Vivaldi

Will test one all those now 😄

Test command is this:
python -m yt_dlp -v -J --cookies-from-browser vivaldi https://vimeo.com/68375962 --print-traffic

I check that now the session cookie is sent in the request and it prints the JSON metadata

yt_dlp/cookies.py Outdated Show resolved Hide resolved
@bashonly
Copy link
Member

does this still work if the "Continue where you left off" option is not enabled?

@StefanLobbenmeier
Copy link
Contributor Author

Surprisingly yes, at least for brave. I would expect that this setting to be necessary, or that you would need the browser still open, but it seems the session cookies are no longer cleared on close. Let me also verify with other browsers, but that is my brave configuration:

image

@StefanLobbenmeier
Copy link
Contributor Author

StefanLobbenmeier commented Apr 20, 2024

Continue where you left off:

Browser Result
Chrome disabled by default, same as brave
Brave disabled (not sure if by default, its my main browser), still works. Closing the browser does not clear session cookies, but they are gone when I open it again.
Opera enabled by default, but after disabling same as brave
Edge disabled by default, behaviour same as brave
Vivaldi does not have that setting, but Startup with Last session was on. Closing the browser with the setting to "Homepage" also does not seem to clear session cookies on close, but rather on start up.

So in summary, the browsers all behave the same that session cookies are not cleared on close. Their defaults for that setting vary. When started with "Open the new tab page" they all cleared the session cookies on startup.

Copy link
Member

@bashonly bashonly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the patch no longer extracts a new column from the cookies DB, this is not a potentially breaking change and rigorous testing is no longer needed. The testing you've already done looks good, thanks!

I'm thinking that instead of closing the linked issue with this PR, we should repurpose it to track session cookie extraction for Firefox, since the issue has some useful information pertaining to that

@Grub4K Grub4K removed the needs-testing Patch needs testing label Apr 21, 2024
@pukkandan
Copy link
Member

@StefanLobbenmeier In light of the discussion about firefox on the other issue, do you want us to merge this first? Or would you prefer to add firefox impl into this PR?

@StefanLobbenmeier
Copy link
Contributor Author

Let’s to Firefox in a separate PR

@bashonly bashonly merged commit f1f1589 into yt-dlp:master May 11, 2024
15 checks passed
mikf added a commit to mikf/gallery-dl that referenced this pull request May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get session cookies when using --cookies-from-browser
4 participants