diff --git a/src-ui/src/app/services/document-list-view.service.spec.ts b/src-ui/src/app/services/document-list-view.service.spec.ts index 6258c42b2..da3d06444 100644 --- a/src-ui/src/app/services/document-list-view.service.spec.ts +++ b/src-ui/src/app/services/document-list-view.service.spec.ts @@ -21,6 +21,7 @@ import { FILTER_HAS_TAGS_ANY, } from '../data/filter-rule-type' import { SavedView } from '../data/saved-view' +import { DOCUMENT_LIST_SERVICE } from '../data/storage-keys' import { SETTINGS_KEYS } from '../data/ui-settings' import { PermissionsGuard } from '../guards/permissions.guard' import { DocumentListViewService } from './document-list-view.service' @@ -248,6 +249,29 @@ describe('DocumentListViewService', () => { expect(documentListViewService.sortReverse).toBeTruthy() }) + it('restores only known list view state fields from local storage', () => { + try { + localStorage.setItem( + DOCUMENT_LIST_SERVICE.CURRENT_VIEW_CONFIG, + '{"currentPage":3,"sortField":"title","sortReverse":false,"__proto__":{"polluted":true},"injected":"ignored"}' + ) + + const restoredService = TestBed.runInInjectionContext( + () => new DocumentListViewService() + ) + + expect(restoredService.currentPage).toEqual(3) + expect(restoredService.sortField).toEqual('title') + expect(restoredService.sortReverse).toBeFalsy() + expect( + (restoredService as any).activeListViewState.injected + ).toBeUndefined() + expect(({} as any).polluted).toBeUndefined() + } finally { + localStorage.removeItem(DOCUMENT_LIST_SERVICE.CURRENT_VIEW_CONFIG) + } + }) + it('should load from query params', () => { expect(documentListViewService.currentPage).toEqual(1) const page = 2 diff --git a/src-ui/src/app/services/document-list-view.service.ts b/src-ui/src/app/services/document-list-view.service.ts index 6989db8ed..f2460add2 100644 --- a/src-ui/src/app/services/document-list-view.service.ts +++ b/src-ui/src/app/services/document-list-view.service.ts @@ -24,6 +24,20 @@ const LIST_DEFAULT_DISPLAY_FIELDS: DisplayField[] = DEFAULT_DISPLAY_FIELDS.map( (f) => f.id ).filter((f) => f !== DisplayField.ADDED) +const RESTORABLE_LIST_VIEW_STATE_KEYS: (keyof ListViewState)[] = [ + 'title', + 'documents', + 'currentPage', + 'collectionSize', + 'sortField', + 'sortReverse', + 'filterRules', + 'selected', + 'pageSize', + 'displayMode', + 'displayFields', +] + /** * Captures the current state of the list view. */ @@ -112,6 +126,32 @@ export class DocumentListViewService { private displayFieldsInitialized: boolean = false + private restoreListViewState(savedState: unknown): ListViewState { + const newState = this.defaultListViewState() + + if ( + !savedState || + typeof savedState !== 'object' || + Array.isArray(savedState) + ) { + return newState + } + + const parsedState = savedState as Partial< + Record + > + const mutableState = newState as Record + + for (const key of RESTORABLE_LIST_VIEW_STATE_KEYS) { + const value = parsedState[key] + if (value != null) { + mutableState[key] = value + } + } + + return newState + } + get activeSavedViewId() { return this._activeSavedViewId } @@ -127,14 +167,7 @@ export class DocumentListViewService { if (documentListViewConfigJson) { try { let savedState: ListViewState = JSON.parse(documentListViewConfigJson) - // Remove null elements from the restored state - Object.keys(savedState).forEach((k) => { - if (savedState[k] == null) { - delete savedState[k] - } - }) - // only use restored state attributes instead of defaults if they are not null - let newState = Object.assign(this.defaultListViewState(), savedState) + let newState = this.restoreListViewState(savedState) this.listViewStates.set(null, newState) } catch (e) { localStorage.removeItem(DOCUMENT_LIST_SERVICE.CURRENT_VIEW_CONFIG) diff --git a/src-ui/src/app/services/settings.service.spec.ts b/src-ui/src/app/services/settings.service.spec.ts index 22ae3e504..5190bc549 100644 --- a/src-ui/src/app/services/settings.service.spec.ts +++ b/src-ui/src/app/services/settings.service.spec.ts @@ -166,6 +166,23 @@ describe('SettingsService', () => { expect(settingsService.get(SETTINGS_KEYS.THEME_COLOR)).toEqual('#9fbf2f') }) + it('ignores unsafe top-level keys from loaded settings', () => { + const req = httpTestingController.expectOne( + `${environment.apiBaseUrl}ui_settings/` + ) + const payload = JSON.parse( + JSON.stringify(ui_settings).replace( + '"settings":{', + '"settings":{"__proto__":{"polluted":"yes"},' + ) + ) + payload.settings.app_title = 'Safe Title' + req.flush(payload) + + expect(settingsService.get(SETTINGS_KEYS.APP_TITLE)).toEqual('Safe Title') + expect(({} as any).polluted).toBeUndefined() + }) + it('correctly allows updating settings of various types', () => { const req = httpTestingController.expectOne( `${environment.apiBaseUrl}ui_settings/` diff --git a/src-ui/src/app/services/settings.service.ts b/src-ui/src/app/services/settings.service.ts index f006cae12..8768a2058 100644 --- a/src-ui/src/app/services/settings.service.ts +++ b/src-ui/src/app/services/settings.service.ts @@ -276,6 +276,8 @@ const ISO_LANGUAGE_OPTION: LanguageOption = { dateInputFormat: 'yyyy-mm-dd', } +const UNSAFE_OBJECT_KEYS = new Set(['__proto__', 'prototype', 'constructor']) + @Injectable({ providedIn: 'root', }) @@ -291,7 +293,7 @@ export class SettingsService { protected baseUrl: string = environment.apiBaseUrl + 'ui_settings/' - private settings: Object = {} + private settings: Record = {} currentUser: User public settingsSaved: EventEmitter = new EventEmitter() @@ -320,6 +322,21 @@ export class SettingsService { this._renderer = rendererFactory.createRenderer(null, null) } + private isSafeObjectKey(key: string): boolean { + return !UNSAFE_OBJECT_KEYS.has(key) + } + + private assignSafeSettings(source: Record) { + if (!source || typeof source !== 'object' || Array.isArray(source)) { + return + } + + for (const key of Object.keys(source)) { + if (!this.isSafeObjectKey(key)) continue + this.settings[key] = source[key] + } + } + // this is called by the app initializer in app.module public initializeSettings(): Observable { return this.http.get(this.baseUrl).pipe( @@ -338,7 +355,7 @@ export class SettingsService { }) }), tap((uisettings) => { - Object.assign(this.settings, uisettings.settings) + this.assignSafeSettings(uisettings.settings) if (this.get(SETTINGS_KEYS.APP_TITLE)?.length) { environment.appTitle = this.get(SETTINGS_KEYS.APP_TITLE) } @@ -533,7 +550,11 @@ export class SettingsService { let settingObj = this.settings keys.forEach((keyPart, index) => { keyPart = keyPart.replace(/-/g, '_') - if (!settingObj.hasOwnProperty(keyPart)) return + if ( + !this.isSafeObjectKey(keyPart) || + !Object.prototype.hasOwnProperty.call(settingObj, keyPart) + ) + return if (index == keys.length - 1) value = settingObj[keyPart] else settingObj = settingObj[keyPart] }) @@ -579,7 +600,9 @@ export class SettingsService { const keys = key.replace('general-settings:', '').split(':') keys.forEach((keyPart, index) => { keyPart = keyPart.replace(/-/g, '_') - if (!settingObj.hasOwnProperty(keyPart)) settingObj[keyPart] = {} + if (!this.isSafeObjectKey(keyPart)) return + if (!Object.prototype.hasOwnProperty.call(settingObj, keyPart)) + settingObj[keyPart] = {} if (index == keys.length - 1) settingObj[keyPart] = value else settingObj = settingObj[keyPart] }) @@ -602,7 +625,10 @@ export class SettingsService { maybeMigrateSettings() { if ( - !this.settings.hasOwnProperty('documentListSize') && + !Object.prototype.hasOwnProperty.call( + this.settings, + 'documentListSize' + ) && localStorage.getItem(SETTINGS_KEYS.DOCUMENT_LIST_SIZE) ) { // lets migrate @@ -610,8 +636,7 @@ export class SettingsService { const errorMessage = $localize`Unable to migrate settings to the database, please try saving manually.` try { - for (const setting in SETTINGS_KEYS) { - const key = SETTINGS_KEYS[setting] + for (const key of Object.values(SETTINGS_KEYS)) { const value = localStorage.getItem(key) this.set(key, value) }