From 5593b41bafaf92a50bd746aa3b64581a5e2a2c2a Mon Sep 17 00:00:00 2001 From: Gabriel Diaz Date: Tue, 10 Mar 2026 13:07:03 -0300 Subject: [PATCH 1/4] fix: Asset-packs id parent when upserting Force asset_pack_id to match the URL parameter instead of trusting the client-supplied value in the request body. --- src/AssetPack/AssetPack.router.spec.ts | 80 ++++++++++++++++++++++++++ src/AssetPack/AssetPack.router.ts | 6 +- 2 files changed, 85 insertions(+), 1 deletion(-) diff --git a/src/AssetPack/AssetPack.router.spec.ts b/src/AssetPack/AssetPack.router.spec.ts index 5c890427..5f4b2a56 100644 --- a/src/AssetPack/AssetPack.router.spec.ts +++ b/src/AssetPack/AssetPack.router.spec.ts @@ -2,9 +2,11 @@ import { ILoggerComponent } from '@well-known-components/interfaces' import { ExpressApp } from '../common/ExpressApp' import { AssetPackRouter } from './AssetPack.router' import { AssetPack } from './AssetPack.model' +import { Asset } from '../Asset' import { getDefaultEthAddress } from './utils' jest.mock('./AssetPack.model') +jest.mock('../Asset/Asset.model') const anAssetPack = { id: 'anId', @@ -375,4 +377,82 @@ describe('AssetPack router', () => { }) }) }) + + describe('when upserting an asset pack', () => { + const anAssetPackId = '8c251928-fb34-40e9-86d2-868a60d2fa78' + const anotherAssetPackId = '49c9ae13-6779-4ced-b208-dd352c9b7541' + const anOwnerAddress = 'anOwnerAddress' + let upsertReq: any + let assetUpsertSpy: jest.SpyInstance + let upsertedAssetAttrs: any[] + + beforeEach(() => { + upsertedAssetAttrs = [] + const mockUpsert = jest.fn().mockResolvedValue({}) + ;((Asset as unknown) as jest.Mock).mockImplementation((attrs: any) => { + upsertedAssetAttrs.push(attrs) + return { upsert: mockUpsert, attributes: attrs } + }) + assetUpsertSpy = mockUpsert + ;(AssetPack.count as jest.Mock).mockResolvedValue(0) + ;(Asset.existsAnyWithADifferentEthAddress as jest.Mock).mockResolvedValue( + false + ) + ;(AssetPack.findOneWithAssets as jest.Mock).mockResolvedValue(null) + ;((AssetPack as unknown) as jest.Mock).mockImplementation( + (attrs: any) => ({ + upsert: jest.fn().mockResolvedValue(attrs), + attributes: attrs, + }) + ) + }) + + describe('when an asset has a different asset_pack_id than the request id', () => { + beforeEach(() => { + upsertReq = { + params: { id: anAssetPackId }, + body: { + assetPack: { + id: anAssetPackId, + title: 'an-asset-pack', + assets: [ + { + id: '1e27cbda-5582-4219-8f83-2db817344cc1', + asset_pack_id: anotherAssetPackId, + name: 'an-asset', + model: 'model.glb', + category: 'decorations', + contents: {}, + tags: ['test'], + metrics: { + triangles: 0, + materials: 0, + textures: 0, + meshes: 0, + bodies: 0, + entities: 0, + }, + }, + ], + }, + }, + auth: { ethAddress: anOwnerAddress }, + } + }) + + it('should use the asset_pack_id with the request id, not the body value', async () => { + await router.upsertAssetPack(upsertReq) + + expect(assetUpsertSpy).toHaveBeenCalled() + expect(upsertedAssetAttrs).toHaveLength(1) + expect(upsertedAssetAttrs[0].asset_pack_id).toBe(anAssetPackId) + }) + + it('should never persist the body-supplied asset_pack_id', async () => { + await router.upsertAssetPack(upsertReq) + + expect(upsertedAssetAttrs[0].asset_pack_id).not.toBe(anotherAssetPackId) + }) + }) + }) }) diff --git a/src/AssetPack/AssetPack.router.ts b/src/AssetPack/AssetPack.router.ts index b9fcb819..4d5fdc5d 100644 --- a/src/AssetPack/AssetPack.router.ts +++ b/src/AssetPack/AssetPack.router.ts @@ -315,7 +315,11 @@ export class AssetPackRouter extends Router { } const upsertResult = await new AssetPack(attributes).upsert() - await Promise.all(assets.map((asset) => new Asset(asset).upsert())) + await Promise.all( + assets.map((asset) => + new Asset({ ...asset, asset_pack_id: id }).upsert() + ) + ) return upsertResult } From 2226ead5a171a1b74cf72954a49cd44f5ef91519 Mon Sep 17 00:00:00 2001 From: Gabriel Diaz Date: Wed, 11 Mar 2026 13:10:49 -0300 Subject: [PATCH 2/4] fix: restrict asset endpoints to authenticated owners Add authentication and ownership checks to GET /assets and GET /assets/:id. Assets are now filtered by the caller's address or the default address, preventing cross-user asset discovery. --- src/Asset/Asset.model.ts | 21 +++-- src/Asset/Asset.router.spec.ts | 151 +++++++++++++++++++++++++++++++++ src/Asset/Asset.router.ts | 27 ++++-- 3 files changed, 188 insertions(+), 11 deletions(-) create mode 100644 src/Asset/Asset.router.spec.ts diff --git a/src/Asset/Asset.model.ts b/src/Asset/Asset.model.ts index 910f6b4e..723a724e 100644 --- a/src/Asset/Asset.model.ts +++ b/src/Asset/Asset.model.ts @@ -32,11 +32,22 @@ export class Asset extends Model { return counts[0].count > 0 } - static findByIds(ids: string[]) { - return this.query(SQL` - SELECT * - FROM ${SQL.raw(this.tableName)} - WHERE id = ANY(${ids})`) + static findByIds(ids: string[], ethAddresses?: string[]) { + const query = SQL` + SELECT a.* FROM ${SQL.raw(this.tableName)} a` + + if (ethAddresses) { + query.append(SQL` + INNER JOIN ${SQL.raw(AssetPack.tableName)} ap ON a.asset_pack_id = ap.id + WHERE a.id = ANY(${ids}) + AND ap.eth_address = ANY(${ethAddresses}) + AND ap.is_deleted = FALSE`) + } else { + query.append(SQL` + WHERE a.id = ANY(${ids})`) + } + + return this.query(query) } static findByAssetPackId(assetPackId: string, limit: number | null = null) { diff --git a/src/Asset/Asset.router.spec.ts b/src/Asset/Asset.router.spec.ts new file mode 100644 index 00000000..0de21a42 --- /dev/null +++ b/src/Asset/Asset.router.spec.ts @@ -0,0 +1,151 @@ +import { ExpressApp } from '../common/ExpressApp' +import { AssetRouter } from './Asset.router' +import { Asset } from './Asset.model' +import { getDefaultEthAddress } from '../AssetPack/utils' + +jest.mock('./Asset.model') +jest.mock('../AssetPack/utils') + +const mockGetDefaultEthAddress = getDefaultEthAddress as jest.MockedFunction< + typeof getDefaultEthAddress +> + +const DEFAULT_ETH_ADDRESS = 'defaultEthAddress' +const anOwnerAddress = 'anOwnerAddress' + +const anAsset = { + id: 'anAssetId', + asset_pack_id: 'anAssetPackId', + name: 'anAsset', + model: 'model.glb', + category: 'decorations', + contents: {}, + tags: [], + metrics: { + triangles: 0, + materials: 0, + textures: 0, + meshes: 0, + bodies: 0, + entities: 0, + }, +} + +const anotherAsset = { + id: 'anotherAssetId', + asset_pack_id: 'anotherAssetPackId', + name: 'anotherAsset', + model: 'another.glb', + category: 'decorations', + contents: {}, + tags: [], + metrics: { + triangles: 0, + materials: 0, + textures: 0, + meshes: 0, + bodies: 0, + entities: 0, + }, +} + +describe('Asset router', () => { + let router: AssetRouter + let req: { params: Record; query: Record; auth: { ethAddress: string } } + + beforeEach(() => { + mockGetDefaultEthAddress.mockReturnValue(DEFAULT_ETH_ADDRESS) + router = new AssetRouter(new ExpressApp()) + req = { params: {}, query: {}, auth: { ethAddress: anOwnerAddress } } + }) + + afterEach(() => { + jest.resetAllMocks() + }) + + describe('when getting a single asset', () => { + beforeEach(() => { + req.params = { id: anAsset.id } + }) + + describe('when the asset belongs to the caller', () => { + beforeEach(() => { + ;(Asset.findByIds as jest.Mock).mockResolvedValueOnce([anAsset]) + }) + + it('should call findByIds with the caller and default addresses', async () => { + await (router as any).getAsset(req) + expect(Asset.findByIds).toHaveBeenCalledWith( + [anAsset.id], + [anOwnerAddress, DEFAULT_ETH_ADDRESS] + ) + }) + + it('should return the asset', async () => { + const result = await (router as any).getAsset(req) + expect(result).toEqual(anAsset) + }) + }) + + describe('when the asset does not belong to the caller or the default address', () => { + beforeEach(() => { + ;(Asset.findByIds as jest.Mock).mockResolvedValueOnce([]) + }) + + it('should return null', async () => { + const result = await (router as any).getAsset(req) + expect(result).toBeNull() + }) + }) + }) + + describe('when getting multiple assets', () => { + beforeEach(() => { + req.query = { id: [anAsset.id, anotherAsset.id] } + }) + + describe('when all assets belong to the caller or the default address', () => { + beforeEach(() => { + ;(Asset.findByIds as jest.Mock).mockResolvedValueOnce([ + anAsset, + anotherAsset, + ]) + }) + + it('should call findByIds with the caller and default addresses', async () => { + await (router as any).getAssets(req) + expect(Asset.findByIds).toHaveBeenCalledWith( + [anAsset.id, anotherAsset.id], + [anOwnerAddress, DEFAULT_ETH_ADDRESS] + ) + }) + + it('should return all visible assets', async () => { + const result = await (router as any).getAssets(req) + expect(result).toEqual([anAsset, anotherAsset]) + }) + }) + + describe('when some assets belong to another user', () => { + beforeEach(() => { + ;(Asset.findByIds as jest.Mock).mockResolvedValueOnce([anAsset]) + }) + + it('should return only the visible assets', async () => { + const result = await (router as any).getAssets(req) + expect(result).toEqual([anAsset]) + }) + }) + + describe('when all assets belong to another user', () => { + beforeEach(() => { + ;(Asset.findByIds as jest.Mock).mockResolvedValueOnce([]) + }) + + it('should return an empty array', async () => { + const result = await (router as any).getAssets(req) + expect(result).toEqual([]) + }) + }) + }) +}) diff --git a/src/Asset/Asset.router.ts b/src/Asset/Asset.router.ts index 182b8490..3c60a6c4 100644 --- a/src/Asset/Asset.router.ts +++ b/src/Asset/Asset.router.ts @@ -10,8 +10,11 @@ import { } from '../middleware' import { getUploader, S3Content } from '../S3' import { AssetPack } from '../AssetPack' +import { getDefaultEthAddress } from '../AssetPack/utils' import { Asset } from './Asset.model' -import { withAuthentication } from '../middleware/authentication' +import { withAuthentication, AuthRequest } from '../middleware/authentication' + +const DEFAULT_ETH_ADDRESS = getDefaultEthAddress() export class AssetRouter extends Router { mount() { @@ -54,6 +57,7 @@ export class AssetRouter extends Router { this.router.get( '/assets/:id', withCors, + withAuthentication, withAssetExists, server.handleRequest(this.getAsset) ) @@ -61,7 +65,12 @@ export class AssetRouter extends Router { /** * Get a multiple assets */ - this.router.get('/assets', withCors, server.handleRequest(this.getAssets)) + this.router.get( + '/assets', + withCors, + withAuthentication, + server.handleRequest(this.getAssets) + ) } async assetBelongsToPackMiddleware(req: Request) { @@ -80,14 +89,20 @@ export class AssetRouter extends Router { // This handler is only here so `server.handleRequest` has a valid callback and it can return the appropiate formated response } - private getAsset(req: Request) { + private async getAsset(req: AuthRequest) { const id = server.extractFromReq(req, 'id') - return Asset.findOne(id) + const eth_address = req.auth.ethAddress + const [asset] = await Asset.findByIds( + [id], + [eth_address, DEFAULT_ETH_ADDRESS] + ) + return asset ?? null } - private getAssets(req: Request) { + private async getAssets(req: AuthRequest) { const reqIds = server.extractFromReq(req, 'id') const ids: string[] = Array.isArray(reqIds) ? reqIds : [reqIds] - return Asset.findByIds(ids) + const eth_address = req.auth.ethAddress + return Asset.findByIds(ids, [eth_address, DEFAULT_ETH_ADDRESS]) } } From 3326d7ec2cc28388bc07a742708f5549fc5b4152 Mon Sep 17 00:00:00 2001 From: Gabriel Diaz Date: Wed, 11 Mar 2026 13:13:35 -0300 Subject: [PATCH 3/4] fix: Remove unrequired mock --- src/Asset/Asset.router.spec.ts | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/Asset/Asset.router.spec.ts b/src/Asset/Asset.router.spec.ts index 0de21a42..d441f296 100644 --- a/src/Asset/Asset.router.spec.ts +++ b/src/Asset/Asset.router.spec.ts @@ -6,11 +6,6 @@ import { getDefaultEthAddress } from '../AssetPack/utils' jest.mock('./Asset.model') jest.mock('../AssetPack/utils') -const mockGetDefaultEthAddress = getDefaultEthAddress as jest.MockedFunction< - typeof getDefaultEthAddress -> - -const DEFAULT_ETH_ADDRESS = 'defaultEthAddress' const anOwnerAddress = 'anOwnerAddress' const anAsset = { @@ -51,10 +46,13 @@ const anotherAsset = { describe('Asset router', () => { let router: AssetRouter - let req: { params: Record; query: Record; auth: { ethAddress: string } } + let req: { + params: Record + query: Record + auth: { ethAddress: string } + } beforeEach(() => { - mockGetDefaultEthAddress.mockReturnValue(DEFAULT_ETH_ADDRESS) router = new AssetRouter(new ExpressApp()) req = { params: {}, query: {}, auth: { ethAddress: anOwnerAddress } } }) @@ -77,7 +75,7 @@ describe('Asset router', () => { await (router as any).getAsset(req) expect(Asset.findByIds).toHaveBeenCalledWith( [anAsset.id], - [anOwnerAddress, DEFAULT_ETH_ADDRESS] + [anOwnerAddress, getDefaultEthAddress()] ) }) @@ -116,7 +114,7 @@ describe('Asset router', () => { await (router as any).getAssets(req) expect(Asset.findByIds).toHaveBeenCalledWith( [anAsset.id, anotherAsset.id], - [anOwnerAddress, DEFAULT_ETH_ADDRESS] + [anOwnerAddress, getDefaultEthAddress()] ) }) From 532ba8a76c320afd8d4d0aec03cc8678c613c4a9 Mon Sep 17 00:00:00 2001 From: Gabriel Diaz Date: Fri, 13 Mar 2026 10:48:44 -0300 Subject: [PATCH 4/4] fix: validate asset ownership and filter deleted packs in asset queries - Filter out assets from deleted packs in findByIds (both branches) - Reject upsert with 400 if assets already belong to a different pack - Use generic error messages to avoid leaking ownership info --- src/Asset/Asset.model.ts | 17 ++++++++++++++++- src/AssetPack/AssetPack.router.spec.ts | 25 ++++++++++++++----------- src/AssetPack/AssetPack.router.ts | 19 ++++++++++++------- 3 files changed, 42 insertions(+), 19 deletions(-) diff --git a/src/Asset/Asset.model.ts b/src/Asset/Asset.model.ts index 723a724e..328e510d 100644 --- a/src/Asset/Asset.model.ts +++ b/src/Asset/Asset.model.ts @@ -44,12 +44,27 @@ export class Asset extends Model { AND ap.is_deleted = FALSE`) } else { query.append(SQL` - WHERE a.id = ANY(${ids})`) + INNER JOIN ${SQL.raw(AssetPack.tableName)} ap ON a.asset_pack_id = ap.id + WHERE a.id = ANY(${ids}) + AND ap.is_deleted = FALSE`) } return this.query(query) } + static async existsAnyWithADifferentAssetPackId( + ids: string[], + assetPackId: string + ): Promise { + const counts = await this.query(SQL` + SELECT COUNT(*) as count + FROM ${SQL.raw(this.tableName)} + WHERE id = ANY(${ids}) + AND asset_pack_id != ${assetPackId}`) + + return counts[0].count > 0 + } + static findByAssetPackId(assetPackId: string, limit: number | null = null) { return this.query(SQL` SELECT * diff --git a/src/AssetPack/AssetPack.router.spec.ts b/src/AssetPack/AssetPack.router.spec.ts index 5f4b2a56..97f67006 100644 --- a/src/AssetPack/AssetPack.router.spec.ts +++ b/src/AssetPack/AssetPack.router.spec.ts @@ -398,6 +398,9 @@ describe('AssetPack router', () => { ;(Asset.existsAnyWithADifferentEthAddress as jest.Mock).mockResolvedValue( false ) + ;(Asset.existsAnyWithADifferentAssetPackId as jest.Mock).mockResolvedValue( + false + ) ;(AssetPack.findOneWithAssets as jest.Mock).mockResolvedValue(null) ;((AssetPack as unknown) as jest.Mock).mockImplementation( (attrs: any) => ({ @@ -407,8 +410,11 @@ describe('AssetPack router', () => { ) }) - describe('when an asset has a different asset_pack_id than the request id', () => { + describe('when an asset already belongs to a different asset pack', () => { beforeEach(() => { + ;(Asset.existsAnyWithADifferentAssetPackId as jest.Mock).mockResolvedValue( + true + ) upsertReq = { params: { id: anAssetPackId }, body: { @@ -440,18 +446,15 @@ describe('AssetPack router', () => { } }) - it('should use the asset_pack_id with the request id, not the body value', async () => { - await router.upsertAssetPack(upsertReq) - - expect(assetUpsertSpy).toHaveBeenCalled() - expect(upsertedAssetAttrs).toHaveLength(1) - expect(upsertedAssetAttrs[0].asset_pack_id).toBe(anAssetPackId) + it('should throw an error', async () => { + await expect(router.upsertAssetPack(upsertReq)).rejects.toThrow( + "One of the assets you're trying to upload is invalid" + ) }) - it('should never persist the body-supplied asset_pack_id', async () => { - await router.upsertAssetPack(upsertReq) - - expect(upsertedAssetAttrs[0].asset_pack_id).not.toBe(anotherAssetPackId) + it('should not upsert any assets', async () => { + await expect(router.upsertAssetPack(upsertReq)).rejects.toThrow() + expect(assetUpsertSpy).not.toHaveBeenCalled() }) }) }) diff --git a/src/AssetPack/AssetPack.router.ts b/src/AssetPack/AssetPack.router.ts index 4d5fdc5d..cb014cfe 100644 --- a/src/AssetPack/AssetPack.router.ts +++ b/src/AssetPack/AssetPack.router.ts @@ -279,8 +279,17 @@ export class AssetPackRouter extends Router { const assetIds = assets.map((asset) => asset.id) if (await Asset.existsAnyWithADifferentEthAddress(assetIds, eth_address)) { throw new HTTPError( - "One of the assets you're trying to upload belongs to a different address. Check the ids", - { eth_address, assetIds } + "One of the assets you're trying to upload is invalid", + {}, + STATUS_CODES.badRequest + ) + } + + if (await Asset.existsAnyWithADifferentAssetPackId(assetIds, id)) { + throw new HTTPError( + "One of the assets you're trying to upload is invalid", + {}, + STATUS_CODES.badRequest ) } @@ -315,11 +324,7 @@ export class AssetPackRouter extends Router { } const upsertResult = await new AssetPack(attributes).upsert() - await Promise.all( - assets.map((asset) => - new Asset({ ...asset, asset_pack_id: id }).upsert() - ) - ) + await Promise.all(assets.map((asset) => new Asset(asset).upsert())) return upsertResult }