From 9f87ccb46039406be61ecaa76d7d81bc7aaa1bbc Mon Sep 17 00:00:00 2001 From: Daniel Salazar Date: Sat, 21 Feb 2026 00:10:55 -0800 Subject: [PATCH] fix: cleanup some app es and app service validation (#2523) we saw some errors with icons not allowed when being objects, this likely user error, but this adds some validation to at least avoid nulls being flagged wrong --- .../src/modules/data-access/AppService.js | 28 +++++--- .../modules/data-access/AppService.test.js | 67 +++++++++++++++++++ src/backend/src/om/definitions/PropType.js | 9 +-- .../src/om/definitions/PropType.test.js | 37 ++++++++++ src/backend/src/om/entitystorage/AppES.js | 4 +- src/backend/src/om/proptypes/__all__.test.js | 4 ++ 6 files changed, 133 insertions(+), 16 deletions(-) create mode 100644 src/backend/src/om/definitions/PropType.test.js diff --git a/src/backend/src/modules/data-access/AppService.js b/src/backend/src/modules/data-access/AppService.js index bca8060f7..d59c60f60 100644 --- a/src/backend/src/modules/data-access/AppService.js +++ b/src/backend/src/modules/data-access/AppService.js @@ -659,11 +659,15 @@ export default class AppService extends BaseService { object.icon = normalizeRawBase64ImageString(object.icon); object.icon = migrateRelativeAppIconEndpointUrl(object.icon); } - if ( typeof object.icon === 'string' && object.icon.startsWith('data:') ) { + if ( typeof object.icon !== 'string' ) { + throw APIError.create('field_invalid', null, { key: 'icon' }); + } + object.icon = object.icon.trim(); + if ( ! object.icon ) { + // Empty icon is allowed to clear current icon. + } else if ( object.icon.startsWith('data:') ) { validate_image_base64(object.icon, { key: 'icon' }); - } else if ( isAllowedAppIconEndpointUrl(object.icon) ) { - // Allow existing relative app icon endpoint references. - } else { + } else if ( ! isAllowedAppIconEndpointUrl(object.icon) ) { throw APIError.create('field_invalid', null, { key: 'icon' }); } } @@ -736,7 +740,7 @@ export default class AppService extends BaseService { url: '', }; await svc_event.emit('app.new-icon', event); - if ( event.url ) { + if ( typeof event.url === 'string' && event.url ) { this.db_write.write('UPDATE apps SET icon = ? WHERE uid = ? LIMIT 1', [event.url, uid]); } @@ -912,11 +916,15 @@ export default class AppService extends BaseService { object.icon = normalizeRawBase64ImageString(object.icon); object.icon = migrateRelativeAppIconEndpointUrl(object.icon); } - if ( typeof object.icon === 'string' && object.icon.startsWith('data:') ) { + if ( typeof object.icon !== 'string' ) { + throw APIError.create('field_invalid', null, { key: 'icon' }); + } + object.icon = object.icon.trim(); + if ( ! object.icon ) { + // Empty icon is allowed to clear current icon. + } else if ( object.icon.startsWith('data:') ) { validate_image_base64(object.icon, { key: 'icon' }); - } else if ( isAllowedAppIconEndpointUrl(object.icon) ) { - // Allow existing relative app icon endpoint references. - } else { + } else if ( ! isAllowedAppIconEndpointUrl(object.icon) ) { throw APIError.create('field_invalid', null, { key: 'icon' }); } } @@ -1200,7 +1208,7 @@ export default class AppService extends BaseService { data_url: object.icon, }; await svc_event.emit('app.new-icon', event); - if ( event.url ) { + if ( typeof event.url === 'string' && event.url ) { await this.db_write.write('UPDATE apps SET icon = ? WHERE uid = ? LIMIT 1', [event.url, old_app.uid]); } diff --git a/src/backend/src/modules/data-access/AppService.test.js b/src/backend/src/modules/data-access/AppService.test.js index 7fb3ebedd..6c7b51d6a 100644 --- a/src/backend/src/modules/data-access/AppService.test.js +++ b/src/backend/src/modules/data-access/AppService.test.js @@ -875,6 +875,40 @@ describe('AppService', () => { expect(validate_url).toHaveBeenCalledWith('https://example.com', expect.objectContaining({ key: 'index_url' })); }); + it('should reject object icon payloads on create', async () => { + setupContextForWrite(createMockUserActor(1)); + mockDb.read.mockResolvedValue([createMockAppRow()]); + + const crudQ = AppService.IMPLEMENTS['crud-q']; + await expect(crudQ.create.call(appService, { + object: { + name: 'test-app', + title: 'Test', + index_url: 'https://example.com', + icon: { url: '/app-icon/app-uid-123/64' }, + }, + })).rejects.toMatchObject({ + fields: { code: 'field_invalid', key: 'icon' }, + }); + }); + + it('should allow empty icon string on create', async () => { + setupContextForWrite(createMockUserActor(1)); + mockDb.read.mockResolvedValue([createMockAppRow()]); + + const crudQ = AppService.IMPLEMENTS['crud-q']; + await crudQ.create.call(appService, { + object: { + name: 'test-app', + title: 'Test', + index_url: 'https://example.com', + icon: '', + }, + }); + + expect(mockEventService.emit).not.toHaveBeenCalledWith('app.new-icon', expect.anything()); + }); + it('should migrate legacy app-icons host URL to app-icon endpoint URL on create', async () => { setupContextForWrite(createMockUserActor(1)); mockDb.read.mockResolvedValue([createMockAppRow()]); @@ -1247,6 +1281,39 @@ describe('AppService', () => { })); }); + it('should reject object icon payloads on update', async () => { + setupContextForWrite(createMockUserActor(1)); + + const crudQ = AppService.IMPLEMENTS['crud-q']; + await expect(crudQ.update.call(appService, { + object: { + uid: 'app-uid-123', + icon: { url: '/app-icon/app-uid-123/64' }, + }, + })).rejects.toMatchObject({ + fields: { code: 'field_invalid', key: 'icon' }, + }); + }); + + it('should allow empty icon string on update', async () => { + setupContextForWrite(createMockUserActor(1)); + + const crudQ = AppService.IMPLEMENTS['crud-q']; + await crudQ.update.call(appService, { + object: { + uid: 'app-uid-123', + icon: '', + }, + }); + + expect(mockEventService.emit).toHaveBeenCalledWith( + 'app.new-icon', + expect.objectContaining({ + app_uid: 'app-uid-123', + data_url: '', + })); + }); + it('should migrate legacy app-icons host URL to app-icon endpoint URL on update', async () => { setupContextForWrite(createMockUserActor(1)); diff --git a/src/backend/src/om/definitions/PropType.js b/src/backend/src/om/definitions/PropType.js index 1b5739630..9520b7ca5 100644 --- a/src/backend/src/om/definitions/PropType.js +++ b/src/backend/src/om/definitions/PropType.js @@ -44,7 +44,7 @@ class PropType extends AdvancedBase { } for ( const k in data ) { - if ( ! chains.hasOwnProperty(k) ) { + if ( ! Object.prototype.hasOwnProperty.call(chains, k) ) { chains[k] = []; } chains[k].push(data[k]); @@ -57,7 +57,7 @@ class PropType extends AdvancedBase { populate_subtype_ (chains) { for ( const k in this.chains ) { - if ( ! chains.hasOwnProperty(k) ) { + if ( ! Object.prototype.hasOwnProperty.call(chains, k) ) { chains[k] = []; } chains[k].push(...this.chains[k]); @@ -65,8 +65,9 @@ class PropType extends AdvancedBase { } async adapt (value, extra) { - const adapters = this.chains.adapt || []; - adapters.reverse(); + const adapters = this.chains.adapt + ? [...this.chains.adapt].reverse() + : []; for ( const adapter of adapters ) { value = await adapter(value, extra); diff --git a/src/backend/src/om/definitions/PropType.test.js b/src/backend/src/om/definitions/PropType.test.js new file mode 100644 index 000000000..fd712ee7a --- /dev/null +++ b/src/backend/src/om/definitions/PropType.test.js @@ -0,0 +1,37 @@ +import { describe, expect, it } from 'vitest'; + +const { PropType } = require('./PropType'); + +describe('PropType adapt chain ordering', () => { + it('runs subtype adapters before supertype adapters on every call', async () => { + const callOrder = []; + const typ = new PropType({ + name: 'test', + chains: { + adapt: [ + value => { + callOrder.push('super'); + if ( typeof value !== 'string' ) { + throw new Error('expected string'); + } + return value; + }, + value => { + callOrder.push('sub'); + if ( value && typeof value === 'object' && typeof value.url === 'string' ) { + return value.url; + } + return value; + }, + ], + }, + }); + + await expect(typ.adapt({ url: 'https://example.com/icon-a.png' })) + .resolves.toBe('https://example.com/icon-a.png'); + await expect(typ.adapt({ url: 'https://example.com/icon-b.png' })) + .resolves.toBe('https://example.com/icon-b.png'); + + expect(callOrder).toEqual(['sub', 'super', 'sub', 'super']); + }); +}); diff --git a/src/backend/src/om/entitystorage/AppES.js b/src/backend/src/om/entitystorage/AppES.js index 311d966ec..2973b4450 100644 --- a/src/backend/src/om/entitystorage/AppES.js +++ b/src/backend/src/om/entitystorage/AppES.js @@ -198,7 +198,7 @@ class AppES extends BaseES { url: '', }; await svc_event.emit('app.new-icon', event); - if ( event.url ) { + if ( typeof event.url === 'string' && event.url ) { this.db.write('UPDATE apps SET icon = ? WHERE id = ? LIMIT 1', [event.url, insert_id]); await entity.set('icon', event.url); @@ -289,7 +289,7 @@ class AppES extends BaseES { data_url: icon, }; await svc_event.emit('app.new-icon', event); - if ( ! event.url ) return; + if ( typeof event.url !== 'string' || !event.url ) return; await this.db.write('UPDATE apps SET icon = ? WHERE uid = ? LIMIT 1', [event.url, app_uid]); diff --git a/src/backend/src/om/proptypes/__all__.test.js b/src/backend/src/om/proptypes/__all__.test.js index 439a2ef9a..f86557350 100644 --- a/src/backend/src/om/proptypes/__all__.test.js +++ b/src/backend/src/om/proptypes/__all__.test.js @@ -67,6 +67,10 @@ describe('OM image-base64 proptype', () => { expect(validateIcon('not-an-icon')).toBeInstanceOf(Error); }); + it('rejects object icon values', () => { + expect(validateIcon({ url: '/app-icon/app-uid-123/64' })).toBeInstanceOf(Error); + }); + it('rejects foreign absolute app-icon endpoint URLs', () => { expect(validateIcon('https://evil.example/app-icon/app-uid-123/64')).toBeInstanceOf(Error); });