Security: prevent prototype pollution in settings and list view (#12438)

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

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<keyof ListViewState, unknown>
+    >
+    const mutableState = newState as Record<keyof ListViewState, unknown>
+
+    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)

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/`

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<string, any> = {}
   currentUser: User
 
   public settingsSaved: EventEmitter<any> = 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<string, any>) {
+    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<UiSettings> {
     return this.http.get<UiSettings>(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)
         }