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

Set new consent cookie and supress tracking cookies on the watch page #4013

Merged
merged 2 commits into from
Oct 8, 2023

Conversation

absidue
Copy link
Member

@absidue absidue commented Sep 9, 2023

Set new consent cookie and supress tracking cookies on the watch page

Pull Request Type

  • Bugfix

Description

This pull request set the new consent cookie, based on what yt-dlp uses, although the old cookie still seems to be used too.

It also gets rid of tracking cookies that YouTube is returning, maybe we should consider changing the values of the consent cookies at some point, so that YouTube doesn't return those cookies.

Screenshots

before:
before

after:
after

Testing

  1. Open a video
  2. In the devtools look at the network log
  3. Click on https://www.youtube.com/sw.js_data and https://www.youtube.com/iframe_api
  4. Switch to their cookies tab
  5. Check that only the CONSENT and SOCS cookies are listed

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) September 9, 2023 16:11
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Sep 9, 2023
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

@kommunarr
Copy link
Collaborator

kommunarr commented Sep 10, 2023

Could you explain the implications of this change and/or share a resource or another example of this? It's not that I don't trust you, but I'm not currently able to review this with knowledgeable confidence (due to my own ignorance on the subject).

@absidue
Copy link
Member Author

absidue commented Sep 11, 2023

yt-dlp uses the SOCS cookie too now (fixed some extraction errors for them), as for nuking tracking cookies before Electron can save them, so that it doesn't send them along with future requests, that's something that should be done regardless of whether you want the new consent cookie or not.

@PikachuEXE
Copy link
Collaborator

PR for SOCS cookie on yt-dlp: yt-dlp/yt-dlp#7774

@PikachuEXE
Copy link
Collaborator

PikachuEXE commented Sep 11, 2023

I got 1 extra: VISITOR_INFO1_LIVE via https://youtu.be/nXkgbmr3dRA
(via checking https://www.youtube.com/sw.js_data)
(same for https://www.youtube.com/iframe_api)
Expected?
image

@absidue
Copy link
Member Author

absidue commented Sep 11, 2023

This pull request should block it, try restarting FT, after switching to this branch.

@PikachuEXE
Copy link
Collaborator

When I review a PR I always kill Electron (normal quit or force kill), check out branch via gh pr checkout xxx, yarn && yarn dev
I have double checked I have done this (so done it twice) and the cookies is still there

Maybe additional steps required to clear cookies? (no idea)

@PikachuEXE
Copy link
Collaborator

To be sure it isn't my local env screwed up I tried development too (same video https://youtu.be/nXkgbmr3dRA)
image

Maybe I need to manually remove that key?

@absidue
Copy link
Member Author

absidue commented Sep 11, 2023

development has the cookies, this pull request aims to get rid of them, weird that some are gone for you but others are still there

@PikachuEXE
Copy link
Collaborator

PikachuEXE commented Sep 12, 2023

I think you need to add code to remove existing cookies
Since they (e.g. VISITOR_INFO1_LIVE) have long expiry time (probably 1 year)
Note: I have switched back to dev branch, play video to receive VISITOR_INFO1_LIVE cookie, then switch to the following code to ensure it's not sent on request
Which means the test section need to be updated

image

@absidue
Copy link
Member Author

absidue commented Sep 12, 2023

@PikachuEXE The problem is that you are quitting FreeTube instead of closing all windows. When you close the last window FreeTube clears up browsing data including cookies, we can't run it when quitting because it's an asynchronous operation but the quit event handler only waits for synchronous code.

https://github.com/FreeTubeApp/FreeTube/blob/development/src/main/index.js#L1035-L1061

This sounds like an issue that will be exclusive to macOS as quitting without closing the windows isn't possible on Windows or Linux (unless you force kill it in the task manager but then you'll be fully aware that the app hasn't had time to clean up).

@PikachuEXE
Copy link
Collaborator

https://www.electronjs.org/docs/latest/api/app#event-window-all-closed

If the user pressed Cmd + Q, or the developer called app.quit(), Electron will first try to close all the windows and then emit the will-quit event, and in this case the window-all-closed event would not be emitted.

See if you want this issue to be fixed in another PR or not
Or I can try to fix myself coz I got access to MacOS

@PikachuEXE
Copy link
Collaborator

I have updated code locally and tested

Code
  let resourcesCleanUpDone = false

  app.on('window-all-closed', () => {
    // Clean up resources (datastores' compaction + Electron cache and storage data clearing)
    cleanUpResources().finally(() => {
      if (process.platform !== 'darwin') {
        app.quit()
      }
    })
  })

  if (process.platform === 'darwin') {
    // `window-all-closed` will no fire on Cmd+Q
    // https://www.electronjs.org/docs/latest/api/app#event-window-all-closed
    // It's also fired when `app.quit` called
    // Not using `before-quit` since that one is fired before windows are closed
    app.on('will-quit', (e) => {
      // Let app quit of resource cleanup done
      if (resourcesCleanUpDone) { return }

      e.preventDefault()
      cleanUpResources().finally(() => {
        // Auto quit again AFTER resource cleanup done
        // Which will call this listener again
        app.quit()
      })
    })
  }

  function cleanUpResources() {
    if (resourcesCleanUpDone) {
      return Promise.resolve()
    }

    return Promise.allSettled([
      baseHandlers.compactAllDatastores(),
      session.defaultSession.clearCache(),
      session.defaultSession.clearStorageData({
        storages: [
          'appcache',
          'cookies',
          'filesystem',
          'indexdb',
          'shadercache',
          'websql',
          'serviceworkers',
          'cachestorage'
        ]
      })
    ])
      .then(() => {
        resourcesCleanUpDone = true
      })
  }

My test steps:

  • Run in dev, ensure cookie present (VISITOR_INFO1_LIVE)
  • Run in new branch, ensure cookie present in request (cleanup not done yet)
  • Cmd+Q, start again, ensure cookie absent in request

Not tested in other OSes

@ChunkyProgrammer
Copy link
Member

@absidue are you planning on adding the macos code to this pr?

@PikachuEXE
Copy link
Collaborator

I can put it into a new PR if you prefer
But I guess I am still the only one testing MacOS :P

@absidue
Copy link
Member Author

absidue commented Oct 2, 2023

I can add it to this one, I just can't test it.

@PikachuEXE
Copy link
Collaborator

Please add and I will test it again

@absidue
Copy link
Member Author

absidue commented Oct 6, 2023

Updated this pull request.

@PikachuEXE
Copy link
Collaborator

Re-tested on MacOS with steps in
#4013 (comment)

@efb4f5ff-1298-471a-8973-3d47447115dc

@ChunkyProgrammer @jasonhenriquez please review when u have time

@FreeTubeBot FreeTubeBot merged commit b72091a into FreeTubeApp:development Oct 8, 2023
5 checks passed
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Oct 8, 2023
@absidue absidue deleted the cookies branch October 8, 2023 16:29
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Oct 11, 2023
* development:
  IV: Show channel handle in search results (FreeTubeApp#3791)
  Bump electron from 22.3.25 to 22.3.26 (FreeTubeApp#4120)
  Bump lefthook from 1.5.0 to 1.5.2 (FreeTubeApp#4122)
  Bump the eslint group with 4 updates (FreeTubeApp#4119)
  Bump sass from 1.68.0 to 1.69.0 (FreeTubeApp#4121)
  Bump marked from 9.0.3 to 9.1.0 (FreeTubeApp#4123)
  Bump version number to v0.19.1
  Fix styling for Channel page on desktop view (FreeTubeApp#4112)
  Set new consent cookie and supress tracking cookies on the watch page (FreeTubeApp#4013)
  Translated using Weblate (Italian)
  Update translation files
  Added translation using Weblate (Belarusian)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants