Skip to content

Conversation

@kontrollanten
Copy link
Contributor

@kontrollanten kontrollanten commented Feb 27, 2025

Description

  • Store thumbnails on object storage
  • Switch from Jimp to sharp. The reason was primary because of typescript issues when handling the output during generation of thumbnails in different sizes (biggestImage). Sharp also seems easier and should also be faster.

@Chocobozzz Will you have time to review this anytime soon? Otherwise I won't go further with ending this PR.

Known issues

  • Fix and add tests
  • Handle regenerate thumbnails CLI command
  • Handle fileUrl upon federation

Related issues

#5699

Has this been tested?

  • 👍 yes, I added tests to the test suite
  • 💭 no, because this PR is a draft and still needs work
  • 🙅 no, because this PR does not update server code
  • 🙋 no, because I need help

Screenshots

@socket-security
Copy link

socket-security bot commented Feb 27, 2025

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Alert Package NoteSourceCI
Protestware or potentially unwanted behavior npm/es5-ext@0.10.64
  • Note: The script attempts to run a local post-install script, which could potentially contain malicious code. The error handling suggests that it is designed to fail silently, which is a common tactic in malicious scripts.
⚠︎
Protestware or potentially unwanted behavior npm/es5-ext@0.10.64
  • Note: This package prints a protestware console message on install regarding Ukraine for users with Russian language locale
⚠︎
Possible typosquat attack npm/ip-set@2.2.0 ⚠︎

View full report↗︎

Next steps

What is protestware?

This package is a joke, parody, or includes undocumented or hidden behavior unrelated to its primary function.

Consider that consuming this package may come along with functionality unrelated to its primary purpose.

What is a typosquat?

Package name is similar to other popular packages and may not be the package you want.

Use care when consuming similarly named packages and ensure that you did not intend to consume a different package. Malicious packages often publish using similar names as existing popular packages.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/foo@1.0.0 or ignore all packages with @SocketSecurity ignore-all

  • @SocketSecurity ignore npm/es5-ext@0.10.64
  • @SocketSecurity ignore npm/ip-set@2.2.0

@kontrollanten kontrollanten force-pushed the feat-5699-thumbnail-obj-storage branch from cd14bc9 to 8e0cf4e Compare February 27, 2025 09:34
@socket-security
Copy link

Report too large to display inline

View full report↗︎

@Chocobozzz
Copy link
Owner

@Chocobozzz Will you have time to review this anytime soon? Otherwise I won't go further with ending this PR.

Unfortunately no in the following weeks, but I also wanted to refactor the way we manage thumbnails to mimic the avatar behaviours: having a thumbnails array of images with multiple width/height, that can be lazy loaded by remote servers. This particular work must be done before adding the possibility to move thumbnails to object storage.

@kontrollanten
Copy link
Contributor Author

Okay. Have you thought about moving the image processing to another service such as imgproxy or imagor/imagorvideo? Original/compressed images could be handled by Peertube, while the rest is handled by another service proxied by nginx.

@Chocobozzz
Copy link
Owner

Chocobozzz commented Mar 10, 2025

Original/compressed images could be handled by Peertube, while the rest is handled by another service proxied by nginx.

What's the main purpose? Have the perfect image size on client side or reduce the compression/resize processing time for PeerTube?

@kontrollanten
Copy link
Contributor Author

Original/compressed images could be handled by Peertube, while the rest is handled by another service proxied by nginx.

What's the main purpose? Have the perfect image size on client side or reduce the compression/resize processing time for PeerTube?

Main purpose is to remove complexity from Peertube. Probably perfect image size and reduced processing time will come as a bonus.

@Chocobozzz
Copy link
Owner

We don't want PeerTube to require an external service, so it won't reduce code complexity.

@kontrollanten
Copy link
Contributor Author

By external service, do you include services installed and hosted by the Peertube admin? The only difference from ffmpeg is the communication protocol.

@Chocobozzz
Copy link
Owner

By external service, do you include services installed and hosted by the Peertube admin? The only difference from ffmpeg is the communication protocol.

Unfortunately yes because every "hard" dependency is another step for admins to install PeerTube, and can increase PeerTube's maintenance burden. It's the reason why open-source & easy self installable projects have different specifications than private/internal projects. For example we chose Redis as key/value and pub/sub engine because it's well known and easy to install on any system. On a private project I might have chosen a different tool etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants