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 saving screenshots to the default folder to an IPC call #6636

Merged
merged 1 commit into from
Jan 27, 2025

Conversation

absidue
Copy link
Member

@absidue absidue commented Jan 25, 2025

Move saving screenshots to the default folder to an IPC call

Pull Request Type

  • Refactor
  • Security improvement

Description

Currently the code for saving video screenshots to the default save location uses the Node.js path and fs/promises modules in the renderer processes. While it does work, it is one of the reasons that we currently have to have nodeIntegration turned on in our renderer processes. This pull request moves it to an IPC call, which allows us to add a few security measures like making sure that IPC call can only write inside the users configured folder and gets us one step closer to turning off nodeIntegration.

Just a reminder why having nodeIntegration turned on in the renderer is a bad idea. Having that turned on means that you can use any Node.js and Electron APIs in there, which is not ideal considering that we load data, including HTML, from third party sources and have the devtools enabled.

P.S. I would recommend turning on the "Hide Whitespace" setting when reviewing this pull request.

Testing

  1. Check that prompting for the save location still works when "Ask for Save Folder" is enabled.
    1. Turn on the "Ask for Save Folder" setting.
    2. Take a screenshot.
    3. Check that it saved it to your chosen location.
  2. Check that saving to the selected default save location works.
    1. Turn off the "Ask for Save Folder" setting.
    2. Choose a default save folder.
    3. Take a screenshot.
    4. Check that it saved it to the folder that you had previously selected.
  3. Check that saving to the default save location works when the user hasn't set it yet.
    1. Close FreeTube
    2. Go to the data location (https://docs.freetubeapp.io/usage/data-location/) but open the Electron folder instead of the FreeTube one.
    3. Remove the screenshotFolderPath line from the settings.db file.
    4. Run FreeTube again with yarn dev.
    5. Take a screenshot.
    6. Check that the screenshot was saved to a FreeTube folder inside the pictures folder in your user profile.

Desktop

  • OS: Windows
  • OS Version: 10
  • FreeTube version: 89d7296

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) January 25, 2025 11:49
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Jan 25, 2025
@FreeTubeBot FreeTubeBot merged commit 829435e into FreeTubeApp:development Jan 27, 2025
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 Jan 27, 2025
@absidue absidue deleted the screenshot-ipc branch January 27, 2025 06:35
SudharakaP pushed a commit to SudharakaP/FreeTube that referenced this pull request Jan 28, 2025
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Jan 29, 2025
* development: (35 commits)
  Shrink mime-db even further (FreeTubeApp#6659)
  * Update play next recommended video setting to be "by default" (FreeTubeApp#6400)
  Miscellaneous performance improvements (FreeTubeApp#6658)
  Bump stylelint in the stylelint group across 1 directory (FreeTubeApp#6660)
  Bump the stylelint group across 1 directory with 4 updates (FreeTubeApp#6605)
  Fixes FreeTubeApp#5476: Adjusted z-index for tooltips to avoid overlapping with bars (FreeTubeApp#6656)
  Bump shaka-player from 4.12.8 to 4.13.0 (FreeTubeApp#6649)
  Migrate ProfileSettings, FtProfileBubble and FtProfileEdit to the composition API (FreeTubeApp#6639)
  Translated using Weblate (Arabic)
  Bump the eslint group with 4 updates (FreeTubeApp#6645)
  Bump bgutils-js from 3.1.2 to 3.1.3 (FreeTubeApp#6650)
  Translated using Weblate (Arabic)
  Translated using Weblate (Arabic)
  Bump electron from 34.0.0 to 34.0.1 (FreeTubeApp#6648)
  Bump lefthook from 1.10.9 to 1.10.10 (FreeTubeApp#6647)
  Bump the babel group with 2 updates (FreeTubeApp#6644)
  Translated using Weblate (Arabic)
  Avoid logging an error when a player cache entry does not exist (FreeTubeApp#6640)
  Move saving screenshots to the default folder to an IPC call (FreeTubeApp#6636)
  Replace rimraf dev dependency with clean script (FreeTubeApp#6638)
  ...
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