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.

Sign up for GitHub

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

Jump to bottom

fix: rewrite avatarproxy and CachedImage #1016

Merged
merged 7 commits into from
Oct 17, 2024
Merged

fix: rewrite avatarproxy and CachedImage #1016

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
27d7371
fix: rewrite avatarproxy and CachedImage
gauthier-th Oct 16, 2024
7ff242d
fix: resolve CodeQL error
gauthier-th Oct 16, 2024
7429297
fix: resolve CodeQL error
gauthier-th Oct 16, 2024
a17be16
fix: resolve review comments
gauthier-th Oct 16, 2024
c3bf6be
fix: resolve review comment
gauthier-th Oct 17, 2024
386f563
fix: resolve CodeQL error
gauthier-th Oct 17, 2024
7a7330f
fix: update imageproxy path
gauthier-th Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 4 additions & 11 deletions server/routes/auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,8 +262,6 @@ authRoutes.post('/jellyfin', async (req, res, next) => {
urlBase: body.urlBase,
});

const { externalHostname } = getSettings().jellyfin;

// Try to find deviceId that corresponds to jellyfin user, else generate a new one
let user = await userRepository.findOne({
where: { jellyfinUsername: body.username },
Expand All @@ -281,11 +279,6 @@ authRoutes.post('/jellyfin', async (req, res, next) => {
// First we need to attempt to log the user in to jellyfin
const jellyfinserver = new JellyfinAPI(hostname ?? '', undefined, deviceId);

const jellyfinHost =
externalHostname && externalHostname.length > 0
? externalHostname
: hostname;

const ip = req.ip;
let clientIp;

Expand Down Expand Up @@ -336,7 +329,7 @@ authRoutes.post('/jellyfin', async (req, res, next) => {
jellyfinAuthToken: account.AccessToken,
permissions: Permission.ADMIN,
avatar: account.User.PrimaryImageTag
? `${jellyfinHost}/Users/${account.User.Id}/Images/Primary/?tag=${account.User.PrimaryImageTag}&quality=90`
? `/Users/${account.User.Id}/Images/Primary/?tag=${account.User.PrimaryImageTag}&quality=90`
: gravatarUrl(body.email || account.User.Name, {
default: 'mm',
size: 200,
Expand All @@ -355,7 +348,7 @@ authRoutes.post('/jellyfin', async (req, res, next) => {
jellyfinAuthToken: account.AccessToken,
permissions: Permission.ADMIN,
avatar: account.User.PrimaryImageTag
? `${jellyfinHost}/Users/${account.User.Id}/Images/Primary/?tag=${account.User.PrimaryImageTag}&quality=90`
? `/Users/${account.User.Id}/Images/Primary/?tag=${account.User.PrimaryImageTag}&quality=90`
: gravatarUrl(body.email || account.User.Name, {
default: 'mm',
size: 200,
Expand Down Expand Up @@ -410,7 +403,7 @@ authRoutes.post('/jellyfin', async (req, res, next) => {
);
// Update the users avatar with their jellyfin profile pic (incase it changed)
if (account.User.PrimaryImageTag) {
const avatar = `${jellyfinHost}/Users/${account.User.Id}/Images/Primary/?tag=${account.User.PrimaryImageTag}&quality=90`;
const avatar = `/Users/${account.User.Id}/Images/Primary/?tag=${account.User.PrimaryImageTag}&quality=90`;
if (avatar !== user.avatar) {
const avatarProxy = new ImageProxy('avatar', '');
avatarProxy.clearCachedImage(user.avatar);
Expand Down Expand Up @@ -467,7 +460,7 @@ authRoutes.post('/jellyfin', async (req, res, next) => {
jellyfinDeviceId: deviceId,
permissions: settings.main.defaultPermissions,
avatar: account.User.PrimaryImageTag
? `${jellyfinHost}/Users/${account.User.Id}/Images/Primary/?tag=${account.User.PrimaryImageTag}&quality=90`
? `/Users/${account.User.Id}/Images/Primary/?tag=${account.User.PrimaryImageTag}&quality=90`
: gravatarUrl(body.email || account.User.Name, {
default: 'mm',
size: 200,
Expand Down
14 changes: 12 additions & 2 deletions server/routes/avatarproxy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,25 @@
import ImageProxy from '@server/lib/imageproxy';
import logger from '@server/logger';
import { getHostname } from '@server/utils/getHostname';
import { Router } from 'express';

const router = Router();

const avatarImageProxy = new ImageProxy('avatar', '');
// Proxy avatar images
router.get('/*', async (req, res) => {
const imagePath = req.url.startsWith('/') ? req.url.slice(1) : req.url;

let imagePath = '';
try {
const jellyfinAvatar = req.url.match(
Copy link

@M0NsTeRRR M0NsTeRRR Oct 16, 2024 •
edited
Loading

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)

Copy link
Collaborator Author

@gauthier-th gauthier-th Oct 17, 2024 •
edited
Loading

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

/(\/Users\/\w+\/Images\/Primary\/?\?tag=\w+&quality=90)$/
)?.[1];
if (!jellyfinAvatar) {
throw new Error('Provided URL is not a Jellyfin avatar.');
gauthier-th marked this conversation as resolved.
Show resolved Hide resolved
}

const imageUrl = new URL(jellyfinAvatar, getHostname());
imagePath = imageUrl.toString();

const imageData = await avatarImageProxy.getImage(imagePath);

res.writeHead(200, {
Expand Down
7 changes: 1 addition & 6 deletions server/routes/settings/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,11 +377,6 @@ settingsRoutes.get('/jellyfin/library', async (req, res, next) => {

settingsRoutes.get('/jellyfin/users', async (req, res) => {
const settings = getSettings();
const { externalHostname } = settings.jellyfin;
const jellyfinHost =
externalHostname && externalHostname.length > 0
? externalHostname
: getHostname();

const userRepository = getRepository(User);
const admin = await userRepository.findOneOrFail({
Expand All @@ -401,7 +396,7 @@ settingsRoutes.get('/jellyfin/users', async (req, res) => {
username: user.Name,
id: user.Id,
thumb: user.PrimaryImageTag
? `${jellyfinHost}/Users/${user.Id}/Images/Primary/?tag=${user.PrimaryImageTag}&quality=90`
? `/Users/${user.Id}/Images/Primary/?tag=${user.PrimaryImageTag}&quality=90`
: gravatarUrl(user.Name, { default: 'mm', size: 200 }),
email: user.Name,
}));
Expand Down
8 changes: 1 addition & 7 deletions server/routes/user/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,12 +516,6 @@ router.post(

//const jellyfinUsersResponse = await jellyfinClient.getUsers();
const createdUsers: User[] = [];
const { externalHostname } = getSettings().jellyfin;

const jellyfinHost =
externalHostname && externalHostname.length > 0
? externalHostname
: hostname;

jellyfinClient.setUserId(admin.jellyfinUserId ?? '');
const jellyfinUsers = await jellyfinClient.getUsers();
Expand All @@ -546,7 +540,7 @@ router.post(
email: jellyfinUser?.Name,
permissions: settings.main.defaultPermissions,
avatar: jellyfinUser?.PrimaryImageTag
? `${jellyfinHost}/Users/${jellyfinUser.Id}/Images/Primary/?tag=${jellyfinUser.PrimaryImageTag}&quality=90`
? `/Users/${jellyfinUser.Id}/Images/Primary/?tag=${jellyfinUser.PrimaryImageTag}&quality=90`
: gravatarUrl(jellyfinUser?.Name ?? '', {
default: 'mm',
size: 200,
Expand Down
3 changes: 3 additions & 0 deletions src/components/Blacklist/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,7 @@ const BlacklistedItem = ({ item, revalidateList }: BlacklistedItemProps) => {
{title && title.backdropPath && (
<div className="absolute inset-0 z-0 w-full bg-cover bg-center xl:w-2/3">
<CachedImage
type="tmdb"
src={`https://image.tmdb.org/t/p/w1920_and_h800_multi_faces/${title.backdropPath}`}
alt=""
style={{ width: '100%', height: '100%', objectFit: 'cover' }}
Expand All @@ -293,6 +294,7 @@ const BlacklistedItem = ({ item, revalidateList }: BlacklistedItemProps) => {
className="relative h-auto w-12 flex-shrink-0 scale-100 transform-gpu overflow-hidden rounded-md transition duration-300 hover:scale-105"
>
<CachedImage
type="tmdb"
src={
title?.posterPath
? `https://image.tmdb.org/t/p/w600_and_h900_bestv2${title.posterPath}`
Expand Down Expand Up @@ -355,6 +357,7 @@ const BlacklistedItem = ({ item, revalidateList }: BlacklistedItemProps) => {
<Link href={`/users/${item.user.id}`}>
<span className="group flex items-center truncate">
<CachedImage
type="avatar"
src={item.user.avatar}
alt=""
className="avatar-sm ml-1.5"
Expand Down
2 changes: 2 additions & 0 deletions src/components/CollectionDetails/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ const CollectionDetails = ({ collection }: CollectionDetailsProps) => {
{data.backdropPath && (
<div className="media-page-bg-image">
<CachedImage
type="tmdb"
alt=""
src={`https://image.tmdb.org/t/p/w1920_and_h800_multi_faces/${data.backdropPath}`}
style={{ width: '100%', height: '100%', objectFit: 'cover' }}
Expand Down Expand Up @@ -228,6 +229,7 @@ const CollectionDetails = ({ collection }: CollectionDetailsProps) => {
<div className="media-header">
<div className="media-poster">
<CachedImage
type="tmdb"
src={
data.posterPath
? `https://image.tmdb.org/t/p/w600_and_h900_bestv2${data.posterPath}`
Expand Down
32 changes: 21 additions & 11 deletions src/components/Common/CachedImage/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,24 +4,34 @@ import Image from 'next/image';

const imageLoader: ImageLoader = ({ src }) => src;

export type CachedImageProps = ImageProps & {
src: string;
type: 'tmdb' | 'avatar';
};

/**
* The CachedImage component should be used wherever
* we want to offer the option to locally cache images.
**/
const CachedImage = ({ src, ...props }: ImageProps) => {
const CachedImage = ({ src, type, ...props }: CachedImageProps) => {
const { currentSettings } = useSettings();

let imageUrl = src;

if (typeof imageUrl === 'string' && imageUrl.startsWith('http')) {
const parsedUrl = new URL(imageUrl);
let imageUrl: string;

if (parsedUrl.host === 'image.tmdb.org') {
if (currentSettings.cacheImages)
imageUrl = imageUrl.replace('https://image.tmdb.org', '/imageproxy');
} else if (parsedUrl.host !== 'gravatar.com') {
imageUrl = '/avatarproxy/' + imageUrl;
}
if (type === 'tmdb') {
// tmdb stuff
imageUrl =
currentSettings.cacheImages && !src.startsWith('/')
? src.replace('https://image.tmdb.org', '/imageproxy')
Copy link

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

: src;
} else if (type === 'avatar') {
// jellyfin avatar (in any)
const jellyfinAvatar = src.match(
/(\/Users\/\w+\/Images\/Primary\/?\?tag=\w+&quality=90)$/
gauthier-th marked this conversation as resolved.
Show resolved Hide resolved
)?.[1];
imageUrl = jellyfinAvatar ? `/avatarproxy` + jellyfinAvatar : src;
} else {
return null;
}

return <Image unoptimized loader={imageLoader} src={imageUrl} {...props} />;
Expand Down
1 change: 1 addition & 0 deletions src/components/Common/ImageFader/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ const ImageFader: ForwardRefRenderFunction<HTMLDivElement, ImageFaderProps> = (
{...props}
>
<CachedImage
type="tmdb"
className="absolute inset-0 h-full w-full"
alt=""
src={imageUrl}
Expand Down
1 change: 1 addition & 0 deletions src/components/Common/Modal/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ const Modal = React.forwardRef<HTMLDivElement, ModalProps>(
{backdrop && (
<div className="absolute top-0 left-0 right-0 z-0 h-64 max-h-full w-full">
<CachedImage
type="tmdb"
alt=""
src={backdrop}
style={{ width: '100%', height: '100%', objectFit: 'cover' }}
Expand Down
1 change: 1 addition & 0 deletions src/components/CompanyCard/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ const CompanyCard = ({ image, url, name }: CompanyCardProps) => {
>
<div className="relative h-full w-full">
<CachedImage
type="tmdb"
src={image}
alt={name}
className="relative z-40 h-full w-full"
Expand Down
1 change: 1 addition & 0 deletions src/components/GenreCard/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ const GenreCard = ({ image, url, name, canExpand = false }: GenreCardProps) => {
tabIndex={0}
>
<CachedImage
type="tmdb"
src={image}
alt=""
style={{ width: '100%', height: '100%', objectFit: 'cover' }}
Expand Down
3 changes: 2 additions & 1 deletion src/components/IssueDetails/IssueComment/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,8 @@ const IssueComment = ({
</Transition>
<Link href={isActiveUser ? '/profile' : `/users/${comment.user.id}`}>
<CachedImage
src={`${comment.user.avatar}`}
type="avatar"
src={comment.user.avatar}
alt=""
className="h-10 w-10 scale-100 transform-gpu rounded-full object-cover ring-1 ring-gray-500 transition duration-300 hover:scale-105"
width={40}
Expand Down
5 changes: 4 additions & 1 deletion src/components/IssueDetails/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ const IssueDetails = () => {
{data.backdropPath && (
<div className="media-page-bg-image">
<CachedImage
type="tmdb"
alt=""
src={`https://image.tmdb.org/t/p/w1920_and_h800_multi_faces/${data.backdropPath}`}
style={{ width: '100%', height: '100%', objectFit: 'cover' }}
Expand All @@ -235,6 +236,7 @@ const IssueDetails = () => {
<div className="media-header">
<div className="media-poster">
<CachedImage
type="tmdb"
src={
data.posterPath
? `https://image.tmdb.org/t/p/w600_and_h900_bestv2${data.posterPath}`
Expand Down Expand Up @@ -287,7 +289,8 @@ const IssueDetails = () => {
className="group ml-1 inline-flex h-full items-center xl:ml-1.5"
>
<CachedImage
src={`${issueData.createdBy.avatar}`}
type="avatar"
src={issueData.createdBy.avatar}
alt=""
className="mr-0.5 h-5 w-5 scale-100 transform-gpu rounded-full object-cover transition duration-300 group-hover:scale-105 xl:mr-1 xl:h-6 xl:w-6"
width={20}
Expand Down
5 changes: 4 additions & 1 deletion src/components/IssueList/IssueItem/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ const IssueItem = ({ issue }: IssueItemProps) => {
{title.backdropPath && (
<div className="absolute inset-0 z-0 w-full bg-cover bg-center xl:w-2/3">
<CachedImage
type="tmdb"
src={`https://image.tmdb.org/t/p/w1920_and_h800_multi_faces/${title.backdropPath}`}
alt=""
style={{ width: '100%', height: '100%', objectFit: 'cover' }}
Expand All @@ -137,6 +138,7 @@ const IssueItem = ({ issue }: IssueItemProps) => {
className="relative h-auto w-12 flex-shrink-0 scale-100 transform-gpu overflow-hidden rounded-md transition duration-300 hover:scale-105"
>
<CachedImage
type="tmdb"
src={
title.posterPath
? `https://image.tmdb.org/t/p/w600_and_h900_bestv2${title.posterPath}`
Expand Down Expand Up @@ -226,7 +228,8 @@ const IssueItem = ({ issue }: IssueItemProps) => {
className="group flex items-center truncate"
>
<CachedImage
src={'/avatarproxy/' + issue.createdBy.avatar}
type="avatar"
src={issue.createdBy.avatar}
alt=""
className="avatar-sm ml-1.5 object-cover"
width={20}
Expand Down
2 changes: 2 additions & 0 deletions src/components/Layout/UserDropdown/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ const UserDropdown = () => {
data-testid="user-menu"
>
<CachedImage
type="avatar"
className="h-8 w-8 rounded-full object-cover sm:h-10 sm:w-10"
src={user ? user.avatar : ''}
alt=""
Expand All @@ -80,6 +81,7 @@ const UserDropdown = () => {
<div className="flex flex-col space-y-4 px-4 py-4">
<div className="flex items-center space-x-2">
<CachedImage
type="avatar"
className="h-8 w-8 rounded-full object-cover sm:h-10 sm:w-10"
src={user ? user.avatar : ''}
alt=""
Expand Down
2 changes: 2 additions & 0 deletions src/components/ManageSlideOver/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,7 @@ const ManageSlideOver = ({
content={user.displayName}
>
<CachedImage
type="avatar"
src={user.avatar}
alt={user.displayName}
className="h-8 w-8 scale-100 transform-gpu rounded-full object-cover ring-1 ring-gray-500 transition duration-300 hover:scale-105"
Expand Down Expand Up @@ -530,6 +531,7 @@ const ManageSlideOver = ({
content={user.displayName}
>
<CachedImage
type="avatar"
src={user.avatar}
alt={user.displayName}
className="h-8 w-8 scale-100 transform-gpu rounded-full object-cover ring-1 ring-gray-500 transition duration-300 hover:scale-105"
Expand Down
3 changes: 3 additions & 0 deletions src/components/MovieDetails/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,7 @@ const MovieDetails = ({ movie }: MovieDetailsProps) => {
{data.backdropPath && (
<div className="media-page-bg-image">
<CachedImage
type="tmdb"
alt=""
src={`https://image.tmdb.org/t/p/w1920_and_h800_multi_faces/${data.backdropPath}`}
style={{ width: '100%', height: '100%', objectFit: 'cover' }}
Expand Down Expand Up @@ -494,6 +495,7 @@ const MovieDetails = ({ movie }: MovieDetailsProps) => {
<div className="media-header">
<div className="media-poster">
<CachedImage
type="tmdb"
src={
data.posterPath
? `https://image.tmdb.org/t/p/w600_and_h900_bestv2${data.posterPath}`
Expand Down Expand Up @@ -741,6 +743,7 @@ const MovieDetails = ({ movie }: MovieDetailsProps) => {
<div className="group relative z-0 scale-100 transform-gpu cursor-pointer overflow-hidden rounded-lg bg-gray-800 bg-cover bg-center shadow-md ring-1 ring-gray-700 transition duration-300 hover:scale-105 hover:ring-gray-500">
<div className="absolute inset-0 z-0">
<CachedImage
type="tmdb"
src={`https://image.tmdb.org/t/p/w1440_and_h320_multi_faces/${data.collection.backdropPath}`}
alt=""
style={{
Expand Down
1 change: 1 addition & 0 deletions src/components/PersonCard/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ const PersonCard = ({
{profilePath ? (
<div className="relative h-full w-3/4 overflow-hidden rounded-full ring-1 ring-gray-700">
<CachedImage
type="tmdb"
src={`https://image.tmdb.org/t/p/w600_and_h900_bestv2${profilePath}`}
alt=""
style={{
Expand Down
1 change: 1 addition & 0 deletions src/components/PersonDetails/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ const PersonDetails = () => {
{data.profilePath && (
<div className="relative mb-6 mr-0 h-36 w-36 flex-shrink-0 overflow-hidden rounded-full ring-1 ring-gray-700 lg:mb-0 lg:mr-6 lg:h-44 lg:w-44">
<CachedImage
type="tmdb"
src={`https://image.tmdb.org/t/p/w600_and_h900_bestv2${data.profilePath}`}
alt=""
style={{ width: '100%', height: '100%', objectFit: 'cover' }}
Expand Down
Loading
Loading