-
-
Notifications
You must be signed in to change notification settings - Fork 244
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
fix: rewrite avatarproxy and CachedImage #1016
Conversation
try { | ||
const jellyfinAvatar = req.url.match( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This regex should be set on router part and not inside the route no ?
And if you want to keep it here the regex look like wrong as it's missing ^
, would be catched : https://test.fake/Users/efefe/Images/Primary/?tag=efefe&quality=90 (even if it's useless)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This regex should be set on router part and not inside the route no ?
It could be set in the router path yes. But I did it this way to be consistent with what is made on front-end. Both ways seems fine to me.
And having it in the router path would require to reconstruct the URL with the URL param and the URL query
// tmdb stuff | ||
imageUrl = | ||
currentSettings.cacheImages && !src.startsWith('/') | ||
? src.replace('https://image.tmdb.org', '/imageproxy') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src.replace doesn't check if this string is present at the beginning so https://fake.test/https://image.tmdb.org?pic=mypic
would be converted to https://fake.test/imageproxy?pic=mypic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm
Tested. Seems to be working as intended |
🎉 This PR is included in version 2.0.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
* fix: rewrite avatarproxy and CachedImage Avatar proxy was allowing every request to be proxied, no matter the original ressource's origin or filetype. This PR fixes it be allowing only relevant resources to be cached, i.e. Jellyfin/Emby images and TMDB images. fix Fallenbagel#1012, Fallenbagel#1013 * fix: resolve CodeQL error * fix: resolve CodeQL error * fix: resolve review comments * fix: resolve review comment * fix: resolve CodeQL error * fix: update imageproxy path
Description
Avatar proxy was allowing every request to be proxied, no matter the original ressource's origin or filetype. This PR fixes it be allowing only relevant resources to be cached, i.e. Jellyfin/Emby images and TMDB images.
Screenshot (if UI-related)
To-Dos
pnpm build
pnpm i18n:extract
Issues Fixed or Closed