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

Move local API player cache internals into the main process #5068

Merged
merged 1 commit into from
May 11, 2024

Conversation

absidue
Copy link
Member

@absidue absidue commented May 5, 2024

Move local API player cache internals into the main process

Pull Request Type

  • Security improvement

Description

Currently the player cache implementation that we use in Electron, writes to the file system in the renderer process (the stuff that actually runs inside of the window), this pull request moves it to the main process with IPC calls. That allows us to add checks to ensure that the player cache can only be used to read and write files in the player_cache directory. It also avoids exposing where the player_cache directory is located on the file system.

As an added bonus this pull request shaves off 1591 bytes from the renderer.js file but only adds 240 bytes to the main.js file.

Testing

  1. Open the dev data location (same as normal one but in the Electron folder instead of the FreeTube one)
  2. Delete the player_cache folder
  3. yarn run dev
  4. Open a video with the local API
  5. Check that the player_cache folder gets created and an item appears inside of it
  6. Open another video
  7. Check in the Network tab of the dev tools that only one request was made to https://www.youtube.com/s/player/{identifier}/player_ias.vflset/en_US/base.js. If there was only one, that means the second video used the cached player data.

After step 5 you've tested that adding an item to the cache works and after step 7 that reading from the cache works.

Desktop

  • OS: Windows
  • OS Version: 10
  • FreeTube version: 0.20.0

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label May 5, 2024
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) May 5, 2024 13:58
@efb4f5ff-1298-471a-8973-3d47447115dc

I cant seem to find the request in the Network tab, also tried filtering for it without any luck. Any suggestions how I can easily find the request?

@absidue
Copy link
Member Author

absidue commented May 5, 2024

Don't copy the exact text, as the {identifier} part is the placeholder for whatever id the current player version has.

Filtering for base.js should work though.

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented May 5, 2024

Filtering by base.js gives me 0 results so its not working or im just being incompetent again?

@PikachuEXE
Copy link
Collaborator

Test result pass checking code later

1st window after playing 2 videos
image

2nd window after playing 3 videos
image

Copy link
Member

Choose a reason for hiding this comment

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

Step 5: Opened first vid, confirmed that item appeared inside it. Also saw a base.js request that matched the item.
Step 6: Opened second vid, saw another request to base.js. So that means that it didnt use the cached one

After that i opened a few other videos and no other base.js requests were made. 2nd video somehow not using the already cached request

Copy link
Member

Choose a reason for hiding this comment

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

Approving as the id was different and thats why i saw a new base.js file

@FreeTubeBot FreeTubeBot merged commit 7685d75 into FreeTubeApp:development May 11, 2024
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 May 11, 2024
@absidue absidue deleted the player-cache-ipc branch May 11, 2024 20:31
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.

5 participants