Skip to content

Commit

Permalink
fix(server): prevent leaking isFavorite and isArchived info (#7580)
Browse files Browse the repository at this point in the history
* fix: prevent leaking favorites info

* add e2e test

* fix: e2e test

* fix: isArchived

* fix: keep old version
  • Loading branch information
martabal committed Mar 2, 2024
1 parent f03381a commit ebe7a14
Show file tree
Hide file tree
Showing 5 changed files with 28 additions and 14 deletions.
13 changes: 12 additions & 1 deletion e2e/src/api/specs/album.e2e-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ describe('/album', () => {
]);

[user1Asset1, user1Asset2] = await Promise.all([
apiUtils.createAsset(user1.accessToken),
apiUtils.createAsset(user1.accessToken, { isFavorite: true }),
apiUtils.createAsset(user1.accessToken),
]);

Expand Down Expand Up @@ -119,6 +119,17 @@ describe('/album', () => {
expect(body).toEqual(errorDto.badRequest(['assetId must be a UUID']));
});

it("should not show other users' favorites", async () => {
const { status, body } = await request(app)
.get(`/album/${user1Albums[0].id}?withoutAssets=false`)
.set('Authorization', `Bearer ${user2.accessToken}`);
expect(status).toEqual(200);
expect(body).toEqual({
...user1Albums[0],
assets: [expect.objectContaining({ isFavorite: false })],
});
});

it('should not return shared albums with a deleted owner', async () => {
const { status, body } = await request(app)
.get('/album?shared=true')
Expand Down
5 changes: 3 additions & 2 deletions server/src/domain/album/album-response.dto.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { AlbumEntity } from '@app/infra/entities';
import { ApiProperty } from '@nestjs/swagger';
import { AssetResponseDto, mapAsset } from '../asset';
import { AuthDto } from '../auth/auth.dto';
import { UserResponseDto, mapUser } from '../user';

export class AlbumResponseDto {
Expand All @@ -24,7 +25,7 @@ export class AlbumResponseDto {
isActivityEnabled!: boolean;
}

export const mapAlbum = (entity: AlbumEntity, withAssets: boolean): AlbumResponseDto => {
export const mapAlbum = (entity: AlbumEntity, withAssets: boolean, auth?: AuthDto): AlbumResponseDto => {
const sharedUsers: UserResponseDto[] = [];

if (entity.sharedUsers) {
Expand Down Expand Up @@ -59,7 +60,7 @@ export const mapAlbum = (entity: AlbumEntity, withAssets: boolean): AlbumRespons
hasSharedLink,
startDate,
endDate,
assets: (withAssets ? assets : []).map((asset) => mapAsset(asset)),
assets: (withAssets ? assets : []).map((asset) => mapAsset(asset, { auth })),
assetCount: entity.assets?.length || 0,
isActivityEnabled: entity.isActivityEnabled,
};
Expand Down
2 changes: 1 addition & 1 deletion server/src/domain/album/album.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ export class AlbumService {
const [albumMetadataForIds] = await this.albumRepository.getMetadataForIds([album.id]);

return {
...mapAlbum(album, withAssets),
...mapAlbum(album, withAssets, auth),
startDate: albumMetadataForIds.startDate,
endDate: albumMetadataForIds.endDate,
assetCount: albumMetadataForIds.assetCount,
Expand Down
14 changes: 7 additions & 7 deletions server/src/domain/asset/asset.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ export class AssetService {

return {
title: `${years} year${years > 1 ? 's' : ''} since...`,
asset: mapAsset(asset),
asset: mapAsset(asset, { auth }),
};
})
.groupBy((asset) => asset.title)
Expand Down Expand Up @@ -230,8 +230,8 @@ export class AssetService {
const timeBucketOptions = await this.buildTimeBucketOptions(auth, dto);
const assets = await this.assetRepository.getTimeBucket(dto.timeBucket, timeBucketOptions);
return !auth.sharedLink || auth.sharedLink?.showExif
? assets.map((asset) => mapAsset(asset, { withStack: true }))
: assets.map((asset) => mapAsset(asset, { stripMetadata: true }));
? assets.map((asset) => mapAsset(asset, { withStack: true, auth }))
: assets.map((asset) => mapAsset(asset, { stripMetadata: true, auth }));
}

async buildTimeBucketOptions(auth: AuthDto, dto: TimeBucketDto): Promise<TimeBucketOptions> {
Expand Down Expand Up @@ -261,7 +261,7 @@ export class AssetService {

async getRandom(auth: AuthDto, count: number): Promise<AssetResponseDto[]> {
const assets = await this.assetRepository.getRandom(auth.user.id, count);
return assets.map((a) => mapAsset(a));
return assets.map((a) => mapAsset(a, { auth }));
}

async getUserAssetsByDeviceId(auth: AuthDto, deviceId: string) {
Expand Down Expand Up @@ -292,10 +292,10 @@ export class AssetService {
}

if (auth.sharedLink && !auth.sharedLink.showExif) {
return mapAsset(asset, { stripMetadata: true, withStack: true });
return mapAsset(asset, { stripMetadata: true, withStack: true, auth });
}

const data = mapAsset(asset, { withStack: true });
const data = mapAsset(asset, { withStack: true, auth });

if (auth.sharedLink) {
delete data.owner;
Expand All @@ -315,7 +315,7 @@ export class AssetService {
await this.updateMetadata({ id, description, dateTimeOriginal, latitude, longitude });

const asset = await this.assetRepository.save({ id, ...rest });
return mapAsset(asset);
return mapAsset(asset, { auth });
}

async updateAll(auth: AuthDto, dto: AssetBulkUpdateDto): Promise<void> {
Expand Down
8 changes: 5 additions & 3 deletions server/src/domain/asset/response-dto/asset-response.dto.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { AuthDto } from '@app/domain/auth/auth.dto';
import { AssetEntity, AssetFaceEntity, AssetType } from '@app/infra/entities';
import { ApiProperty } from '@nestjs/swagger';
import { PersonWithFacesResponseDto, mapFacesWithoutPerson, mapPerson } from '../../person/person.dto';
Expand Down Expand Up @@ -50,6 +51,7 @@ export class AssetResponseDto extends SanitizedAssetResponseDto {
export type AssetMapOptions = {
stripMetadata?: boolean;
withStack?: boolean;
auth?: AuthDto;
};

const peopleWithFaces = (faces: AssetFaceEntity[]): PersonWithFacesResponseDto[] => {
Expand Down Expand Up @@ -103,8 +105,8 @@ export function mapAsset(entity: AssetEntity, options: AssetMapOptions = {}): As
fileModifiedAt: entity.fileModifiedAt,
localDateTime: entity.localDateTime,
updatedAt: entity.updatedAt,
isFavorite: entity.isFavorite,
isArchived: entity.isArchived,
isFavorite: options.auth?.user.id === entity.ownerId ? entity.isFavorite : false,
isArchived: options.auth?.user.id === entity.ownerId ? entity.isArchived : false,
isTrashed: !!entity.deletedAt,
duration: entity.duration ?? '0:00:00.00000',
exifInfo: entity.exifInfo ? mapExif(entity.exifInfo) : undefined,
Expand All @@ -117,7 +119,7 @@ export function mapAsset(entity: AssetEntity, options: AssetMapOptions = {}): As
stack: withStack
? entity.stack?.assets
.filter((a) => a.id !== entity.stack?.primaryAssetId)
.map((a) => mapAsset(a, { stripMetadata }))
.map((a) => mapAsset(a, { stripMetadata, auth: options.auth }))
: undefined,
stackCount: entity.stack?.assets?.length ?? null,
isExternal: entity.isExternal,
Expand Down

0 comments on commit ebe7a14

Please sign in to comment.