fix: an XSS in descriptions and two frontend bugs found by the coverage tests

src/app/app.component.spec.ts

@@ -1,6 +1,7 @@
 import { AppComponent } from './app.component';
 import { of, Subject, throwError } from 'rxjs';
 import { Download } from 'api-types';
+import { THEMES_CONFIG } from '../themes';
 
 describe('AppComponent', () => {
   let component: AppComponent;
@@ -483,6 +484,100 @@ describe('AppComponent notification bell', () => {
   });
 });
 
+describe('AppComponent themes', () => {
+  let component: AppComponent;
+  let posts_service_mock: any;
+  let overlay: HTMLElement;
+  let body_style: any;
+
+  beforeEach(() => {
+    localStorage.clear();
+    overlay = document.createElement('div');
+    body_style = {};
+    posts_service_mock = {
+      config_reloaded: of(false),
+      files_changed: new Subject<boolean>(),
+      open_create_default_admin_dialog: of(false),
+      service_initialized: new Subject<boolean>(),
+      initialized: false,
+      config: { Advanced: { multi_user_mode: false } },
+      hasPermission: () => true,
+      hasSession: () => false,
+      getVersionInfo: () => of({ version_info: {} }),
+      // What the real service does, so the toggle can read the theme it was given.
+      setTheme: vi.fn((theme: string) => { posts_service_mock.theme = THEMES_CONFIG[theme]; })
+    };
+    component = new AppComponent(posts_service_mock, {} as any, { openDialogs: [] } as any,
+      { events: of(), navigate: vi.fn(), url: '/' } as any, { getContainerElement: () => overlay } as any,
+      { nativeElement: { ownerDocument: { body: { style: body_style } } } } as any, { artworkURL: () => null } as any);
+  });
+
+  afterEach(() => {
+    localStorage.clear();
+    vi.restoreAllMocks();
+  });
+
+  function loadConfig(default_theme: string) {
+    posts_service_mock.config = {
+      Advanced: { multi_user_mode: false },
+      Extra: { title_top: 'ytdl-material', enable_downloads_manager: false, file_manager_enabled: false },
+      Subscriptions: { allow_subscriptions: false },
+      Themes: { default_theme, allow_theme_change: true }
+    };
+    component.loadConfig();
+  }
+
+  // A stored theme this version does not have used to be refused, so no theme was applied at
+  // all, and the toggle in the account menu then threw on the theme that was never set.
+  it('applies the default theme in place of a stored one it does not know, and can still switch', () => {
+    vi.spyOn(console, 'error').mockImplementation(() => { });
+    localStorage.setItem('theme', 'retired');
+
+    loadConfig('dark');
+
+    expect(posts_service_mock.setTheme).toHaveBeenCalledWith('dark');
+    expect(localStorage.getItem('theme')).toBe('dark');
+    expect(overlay.classList.contains('dark-theme')).toBe(true);
+
+    component.themeMenuItemClicked({ stopPropagation: () => { } });
+
+    expect(localStorage.getItem('theme')).toBe('default');
+    expect(overlay.classList.contains('dark-theme')).toBe(false);
+    expect(overlay.classList.contains('default-theme')).toBe(true);
+  });
+
+  it('falls back to the light theme when the configured default does not exist', () => {
+    vi.spyOn(console, 'error').mockImplementation(() => { });
+
+    loadConfig('neon');
+
+    expect(posts_service_mock.setTheme).toHaveBeenCalledWith('default');
+    expect(component.componentCssClass).toBe('default-theme');
+  });
+
+  it('switches theme before the config has loaded, whatever was stored', () => {
+    vi.spyOn(console, 'error').mockImplementation(() => { });
+    vi.spyOn(console, 'log').mockImplementation(() => { });
+    localStorage.setItem('theme', 'retired');
+
+    component.setTheme('dark');
+
+    expect(localStorage.getItem('theme')).toBe('dark');
+    expect(overlay.classList.contains('dark-theme')).toBe(true);
+    expect(body_style.backgroundColor).toBe('#0b0b0b');
+  });
+
+  it('does not take a name every object has for a theme', () => {
+    const error = vi.spyOn(console, 'error').mockImplementation(() => { });
+
+    component.setTheme('constructor');
+
+    expect(error).toHaveBeenCalledWith('Invalid theme: constructor');
+    expect(posts_service_mock.setTheme).not.toHaveBeenCalled();
+    expect(localStorage.getItem('theme')).toBeNull();
+  });
+});
+
 describe('AppComponent while browsing someone else\'s library', () => {
   let component: AppComponent;
   let posts_service_mock: any;

src/app/app.component.ts

@@ -183,7 +183,8 @@ export class AppComponent implements OnInit, AfterViewInit, OnDestroy {
     // loading config
     this.topBarTitle = this.postsService.config['Extra']['title_top'];
     const themingExists = this.postsService.config['Themes'];
-    this.defaultTheme = themingExists ? this.postsService.config['Themes']['default_theme'] : 'default';
+    const configuredTheme = themingExists ? this.postsService.config['Themes']['default_theme'] : 'default';
+    this.defaultTheme = this.isKnownTheme(configuredTheme) ? configuredTheme : 'default';
     this.allowThemeChange = themingExists ? this.postsService.config['Themes']['allow_theme_change'] : true;
     this.allowSubscriptions = this.postsService.config['Subscriptions']['allow_subscriptions'];
     this.enableDownloadsManager = this.postsService.config['Extra']['enable_downloads_manager'];
@@ -221,8 +222,15 @@ export class AppComponent implements OnInit, AfterViewInit, OnDestroy {
     return 'theme';
   }
 
+  // A theme this version does not have, such as one an older release stored or one edited in
+  // by hand, counts as no theme at all, so the default applies in its place.
+  isKnownTheme(theme: string): boolean {
+    return typeof theme === 'string' && Object.hasOwn(this.THEMES_CONFIG, theme);
+  }
+
   getStoredTheme(): string {
-    return localStorage.getItem(this.getThemeStorageKey()) || localStorage.getItem('theme');
+    const theme = localStorage.getItem(this.getThemeStorageKey()) || localStorage.getItem('theme');
+    return this.isKnownTheme(theme) ? theme : null;
   }
 
   setStoredTheme(theme: string): void {
@@ -234,33 +242,19 @@ export class AppComponent implements OnInit, AfterViewInit, OnDestroy {
   }
 
   setTheme(theme) {
-    // theme is registered, so set it to the stored cookie variable
-    let old_theme = null;
-    if (this.THEMES_CONFIG[theme]) {
-        const currentTheme = this.getStoredTheme();
-        if (currentTheme) {
-          old_theme = currentTheme;
-          if (!this.THEMES_CONFIG[old_theme]) {
-            console.log('bad theme found, setting to default');
-            if (this.defaultTheme === null) {
-              // means it hasn't loaded yet
-              console.error('No default theme detected');
-            } else {
-              this.setStoredTheme(this.defaultTheme);
-              old_theme = this.getStoredTheme(); // updates old_theme
-            }
-          }
-        }
-        this.setStoredTheme(theme);
-        this.elementRef.nativeElement.ownerDocument.body.style.backgroundColor = this.THEMES_CONFIG[theme]['background_color'];
-    } else {
-        console.error('Invalid theme: ' + theme);
-        return;
+    if (!this.isKnownTheme(theme)) {
+      console.error('Invalid theme: ' + theme);
+      return;
     }
 
+    // Only a theme that exists can have put its class on the page, so only one can be taken off.
+    const old_theme = this.getStoredTheme();
+    this.setStoredTheme(theme);
+    this.elementRef.nativeElement.ownerDocument.body.style.backgroundColor = this.THEMES_CONFIG[theme]['background_color'];
+
     this.postsService.setTheme(theme);
 
-    this.onSetTheme(this.THEMES_CONFIG[theme]['css_label'], old_theme ? this.THEMES_CONFIG[old_theme]['css_label'] : old_theme);
+    this.onSetTheme(this.THEMES_CONFIG[theme]['css_label'], old_theme ? this.THEMES_CONFIG[old_theme]['css_label'] : null);
   }
 
   onSetTheme(theme, old_theme) {

src/app/components/see-more/see-more.component.spec.ts

@@ -20,7 +20,48 @@ describe('SeeMoreComponent', () => {
     fixture.detectChanges();
   });
 
+  function render(text: string): HTMLElement {
+    component.text = text;
+    fixture.detectChanges();
+    return fixture.nativeElement.querySelector('.text');
+  }
+
   it('should create', () => {
     expect(component).toBeTruthy();
   });
+
+  // Descriptions come from wherever a file was downloaded from, so anyone can write one.
+  it('shows markup in a description as text instead of running it', () => {
+    const description = render('Great clip <img src=x onerror=alert(1)> <script>alert(2)</script>');
+
+    expect(description.querySelector('img, script')).toBeNull();
+    expect(description.textContent).toContain('<img src=x onerror=alert(1)>');
+    expect(description.textContent).toContain('<script>alert(2)</script>');
+  });
+
+  it('links web addresses without letting one add attributes to its link', () => {
+    const description = render('Q&A at https://example.test/a"onmouseover="alert(1) and https://example.test/b?x=1&y=2');
+    const links = description.querySelectorAll('a');
+
+    expect(links.length).toBe(2);
+    expect(links[0].getAttribute('onmouseover')).toBeNull();
+    expect(links[0].getAttribute('href')).toBe('https://example.test/a%22onmouseover=%22alert(1)');
+    expect(links[0].textContent).toBe('https://example.test/a"onmouseover="alert(1)');
+    expect(links[1].getAttribute('href')).toBe('https://example.test/b?x=1&y=2');
+    expect(links[1].getAttribute('target')).toBe('_blank');
+    expect(links[1].getAttribute('rel')).toBe('noopener noreferrer');
+    expect(description.textContent).toContain('Q&A at');
+  });
+
+  it('links only whole http and https addresses', () => {
+    const description = render('javascript:alert(1) httpd.conf ftp://example.test/file https:// http://');
+
+    expect(description.querySelector('a')).toBeNull();
+  });
+
+  it('keeps the line breaks in a description', () => {
+    const description = render('First line\nSecond line');
+
+    expect(description.innerHTML.split('<br>').map(line => line.trim()).filter(Boolean)).toEqual(['First line', 'Second line']);
+  });
 });

src/app/components/see-more/see-more.component.ts

@@ -1,34 +1,42 @@
 import { Component, Input, OnInit, Pipe, PipeTransform, ChangeDetectionStrategy } from '@angular/core';
-import { DomSanitizer } from '@angular/platform-browser';
 import { NgStyle } from '@angular/common';
 import { RouterLink } from '@angular/router';
 
-@Pipe({ name: 'linkify' })
-export class LinkifyPipe implements PipeTransform {
+const HTML_ESCAPES: Record<string, string> = {'&': '&amp;', '<': '&lt;', '>': '&gt;', '"': '&quot;', '\'': '&#39;'};
 
-  constructor(private _domSanitizer: DomSanitizer) {}
+function escapeHTML(text: string): string {
+  return text.replace(/[&<>"']/g, character => HTML_ESCAPES[character]);
+}
 
-  transform(value: any, args?: any): any {
-    return this._domSanitizer.bypassSecurityTrustHtml(this.stylize(value));
+// Only a whole http or https address becomes a link, so no word can pick a link's scheme.
+function webAddress(word: string): string | null {
+  if (!/^https?:\/\//i.test(word)) return null;
+  try {
+    return new URL(word).href;
+  } catch {
+    return null;
   }
+}
 
-  // Modify this method according to your custom logic
-  private stylize(text: string): string {
-    let stylizedText: string = '';
-    if (text && text.length > 0) {
-      for (const line of text.split("\n")) {
-        for (const t of line.split(" ")) {
-          if (t.startsWith("http") && t.length>7) {  
-            stylizedText += `<a target="_blank" href="${t}">${t}</a> `;
-          }
-          else
-            stylizedText += t + " ";
-        }
-        stylizedText += '<br>';
-      }
-      return stylizedText;
-    }
-    else return text;
+/*************************************************
+ * Turns the web addresses in a description into
+ * links, with every part of it escaped first. A
+ * description is written by whoever uploaded the
+ * file, so nothing in one may become markup. What
+ * this returns is still sanitized as [innerHTML]
+ * rather than marked as trusted.
+ ************************************************/
+@Pipe({ name: 'linkify' })
+export class LinkifyPipe implements PipeTransform {
+
+  transform(value: string): string {
+    if (!value) return value;
+    return value.split('\n').map(line => line.split(' ').map(word => {
+      const href = webAddress(word);
+      return href === null
+        ? escapeHTML(word)
+        : `<a target="_blank" rel="noopener noreferrer" href="${escapeHTML(href)}">${escapeHTML(word)}</a>`;
+    }).join(' ')).join('<br>');
   }
 
 }
@@ -57,8 +65,4 @@ export class SeeMoreComponent implements OnInit {
     this.see_more_active = !this.see_more_active;
   }
 
-  parseText() {
-    return this.text.replace(/(http.*?\s)/, "<a href=\"$1\">$1</a>")
-  }
-
 }

src/app/dialogs/update-progress-dialog/update-progress-dialog.component.spec.ts

@@ -1,7 +1,11 @@
 import { ComponentFixture, TestBed, waitForAsync } from '@angular/core/testing';
+import { HttpErrorResponse } from '@angular/common/http';
+import { of, Subject, throwError } from 'rxjs';
 
 import { UpdateProgressDialogComponent } from './update-progress-dialog.component';
 import { configureTestBed } from '../../../testing/test-bed';
+import { PostsService } from 'app/posts.services';
+import { UpdaterStatus } from '../../../api-types';
 
 describe('UpdateProgressDialogComponent', () => {
   let component: UpdateProgressDialogComponent;
@@ -25,3 +29,62 @@ describe('UpdateProgressDialogComponent', () => {
     expect(component).toBeTruthy();
   });
 });
+
+describe('UpdateProgressDialogComponent polling', () => {
+  let posts: { getUpdaterStatus: ReturnType<typeof vi.fn>; openSnackBar: ReturnType<typeof vi.fn> };
+  let component: UpdateProgressDialogComponent;
+  const restarting: UpdaterStatus = { updating: true, details: 'Update complete! Restarting server...' };
+
+  beforeEach(() => {
+    vi.useFakeTimers();
+    posts = { getUpdaterStatus: vi.fn(), openSnackBar: vi.fn() };
+    component = new UpdateProgressDialogComponent(posts as unknown as PostsService);
+  });
+
+  afterEach(() => {
+    vi.clearAllTimers();
+    vi.useRealTimers();
+  });
+
+  // It used to read the status before the first answer had set it, and threw every tick.
+  it('waits for the first status before asking again', () => {
+    const first = new Subject<UpdaterStatus>();
+    posts.getUpdaterStatus.mockReturnValue(first);
+
+    component.ngOnInit();
+    vi.advanceTimersByTime(1000);
+    expect(posts.getUpdaterStatus).toHaveBeenCalledOnce();
+
+    posts.getUpdaterStatus.mockReturnValue(of({ updating: true, details: 'Installing new dependencies...' }));
+    first.next({ updating: true, details: 'Backing up key server files...' });
+    vi.advanceTimersByTime(250);
+
+    expect(posts.getUpdaterStatus).toHaveBeenCalledTimes(2);
+    expect(component.updateStatus.details).toBe('Installing new dependencies...');
+  });
+
+  // The server restarts at the end of an update with its status still saying it is updating,
+  // so a dialog that kept its timer kept asking four times a second until the page reloaded.
+  it('stops asking once it is closed', () => {
+    posts.getUpdaterStatus.mockReturnValue(of(restarting));
+    component.ngOnInit();
+    vi.advanceTimersByTime(250);
+    expect(posts.getUpdaterStatus).toHaveBeenCalledTimes(2);
+
+    component.ngOnDestroy();
+    vi.advanceTimersByTime(5000);
+
+    expect(posts.getUpdaterStatus).toHaveBeenCalledTimes(2);
+  });
+
+  it('keeps showing the last status while the server restarts, and keeps asking', () => {
+    posts.getUpdaterStatus.mockReturnValue(of(restarting));
+    component.ngOnInit();
+    posts.getUpdaterStatus.mockReturnValue(throwError(() => new HttpErrorResponse({ status: 0 })));
+
+    vi.advanceTimersByTime(500);
+
+    expect(posts.getUpdaterStatus).toHaveBeenCalledTimes(3);
+    expect(component.updateStatus).toEqual(restarting);
+  });
+});