Merge commit from fork

User.settings is an eager relation, so it rode along into every response embedding a User, exposing
pushover, pushbullet, telegram, discord and PGP values through the request, issue, issue comment,
blocklist, and watchlist endpoints. It now omits it by using User.toJSON(). GET /auth/me builds
settings from seven non-credential fields the web app reads, so locale and region defaults keep
working. Reported as a security advisory, GHSA-8j73-j6rc-q8xh.

server/entity/User.ts

@@ -51,6 +51,8 @@ export class User {
     'settings',
   ];
 
+  static readonly serializationExcludedFields: string[] = ['settings'];
+
   public displayName: string;
 
   @PrimaryGeneratedColumn()
@@ -173,6 +175,16 @@ export class User {
     return filtered;
   }
 
+  // settings is eager-loaded and would otherwise ride along into every nested User response
+  public toJSON(): Partial<User> {
+    return Object.assign(
+      {},
+      ...(Object.keys(this) as (keyof User)[])
+        .filter((k) => !User.serializationExcludedFields.includes(k))
+        .map((k) => ({ [k]: this[k] }))
+    );
+  }
+
   public hasPermission(
     permissions: Permission | Permission[],
     options?: PermissionCheckOptions

server/routes/auth.test.ts

@@ -12,6 +12,11 @@ import ImageProxy from '@server/lib/imageproxy';
 import { getSettings } from '@server/lib/settings';
 import { checkUser } from '@server/middleware/auth';
 import { setupTestDb } from '@server/test/db';
+import {
+  allowlistedSettings,
+  assertNoCredentials,
+  seedUserSettings,
+} from '@server/test/userSettings';
 import { ApiError } from '@server/types/error';
 import axios from 'axios';
 import type { Express } from 'express';
@@ -613,6 +618,30 @@ describe('GET /auth/me', () => {
 
     settings.notifications.agents.email.options.userEmailRequired = false;
   });
+
+  it('returns only the allowlisted settings fields', async () => {
+    await seedUserSettings('[email protected]');
+    const agent = await authenticatedAgent('[email protected]', 'test1234');
+
+    const res = await agent.get('/auth/me');
+
+    assert.strictEqual(res.status, 200);
+    assert.deepStrictEqual(Object.keys(res.body.settings).sort(), [
+      'discoverRegion',
+      'locale',
+      'notificationTypes',
+      'originalLanguage',
+      'streamingRegion',
+      'watchlistSyncMovies',
+      'watchlistSyncTv',
+    ]);
+    assert.strictEqual(res.body.settings.locale, allowlistedSettings.locale);
+    assert.strictEqual(
+      res.body.settings.discoverRegion,
+      allowlistedSettings.discoverRegion
+    );
+    assertNoCredentials(res.body);
+  });
 });
 
 describe('POST /auth/local', () => {

server/routes/auth.ts

@@ -52,7 +52,18 @@ authRoutes.get('/me', isAuthenticated(), async (req, res) => {
     logger.warn(`User ${user.username} has no valid email address`);
   }
 
-  return res.status(200).json(user);
+  return res.status(200).json({
+    ...user.toJSON(),
+    settings: user.settings && {
+      locale: user.settings.locale,
+      discoverRegion: user.settings.discoverRegion,
+      streamingRegion: user.settings.streamingRegion,
+      originalLanguage: user.settings.originalLanguage,
+      notificationTypes: user.settings.notificationTypes,
+      watchlistSyncMovies: user.settings.watchlistSyncMovies,
+      watchlistSyncTv: user.settings.watchlistSyncTv,
+    },
+  });
 });
 
 authRoutes.post('/plex', async (req, res, next) => {

server/routes/blocklist.test.ts

@@ -0,0 +1,106 @@
+import assert from 'node:assert/strict';
+import { before, describe, it } from 'node:test';
+
+import { MediaStatus, MediaType } from '@server/constants/media';
+import { getRepository } from '@server/datasource';
+import { Blocklist } from '@server/entity/Blocklist';
+import Media from '@server/entity/Media';
+import { getSettings } from '@server/lib/settings';
+import { checkUser } from '@server/middleware/auth';
+import { setupTestDb } from '@server/test/db';
+import {
+  assertNoCredentials,
+  seedUserSettings,
+} from '@server/test/userSettings';
+import type { Express } from 'express';
+import express from 'express';
+import session from 'express-session';
+import request from 'supertest';
+import authRoutes from './auth';
+import blocklistRoutes from './blocklist';
+
+let app: Express;
+
+function createApp() {
+  const app = express();
+  app.use(express.json());
+  app.use(
+    session({
+      secret: 'test-secret',
+      resave: false,
+      saveUninitialized: false,
+    })
+  );
+  app.use(checkUser);
+  app.use('/auth', authRoutes);
+  app.use('/blocklist', blocklistRoutes);
+  app.use(
+    (
+      err: { status?: number; message?: string },
+      _req: express.Request,
+      res: express.Response,
+      // eslint-disable-next-line @typescript-eslint/no-unused-vars
+      _next: express.NextFunction
+    ) => {
+      res
+        .status(err.status ?? 500)
+        .json({ status: err.status ?? 500, message: err.message });
+    }
+  );
+  return app;
+}
+
+before(async () => {
+  app = createApp();
+});
+
+setupTestDb();
+
+async function loginAs(email: string, password: string) {
+  const settings = getSettings();
+  const priorLocalLogin = settings.main.localLogin;
+  settings.main.localLogin = true;
+
+  try {
+    const agent = request.agent(app);
+    const res = await agent.post('/auth/local').send({ email, password });
+    assert.strictEqual(res.status, 200);
+    return agent;
+  } finally {
+    settings.main.localLogin = priorLocalLogin;
+  }
+}
+
+describe('GET /blocklist/:id', () => {
+  // this route replies with res.send() rather than res.json()
+  it('omits notification settings from the blocklisting user', async () => {
+    const blocklistUser = await seedUserSettings('[email protected]');
+
+    const media = await getRepository(Media).save(
+      new Media({
+        mediaType: MediaType.MOVIE,
+        tmdbId: 12345,
+        status: MediaStatus.BLOCKLISTED,
+        status4k: MediaStatus.UNKNOWN,
+      })
+    );
+
+    await getRepository(Blocklist).save(
+      new Blocklist({
+        mediaType: MediaType.MOVIE,
+        tmdbId: 12345,
+        title: 'Blocked Movie',
+        user: blocklistUser,
+        media,
+      })
+    );
+
+    const admin = await loginAs('[email protected]', 'test1234');
+    const res = await admin.get('/blocklist/12345?mediaType=movie');
+
+    assert.strictEqual(res.status, 200);
+    assert.strictEqual(res.body.user.email, '[email protected]');
+    assert.ok(!('settings' in res.body.user));
+    assertNoCredentials(res.body);
+  });
+});

server/routes/issue.test.ts

@@ -13,12 +13,17 @@ import { checkUser } from '@server/middleware/auth';
 import { IssueCommentSubscriber } from '@server/subscriber/IssueCommentSubscriber';
 import { IssueSubscriber } from '@server/subscriber/IssueSubscriber';
 import { setupTestDb } from '@server/test/db';
+import {
+  assertNoCredentials,
+  seedUserSettings,
+} from '@server/test/userSettings';
 import type { Express } from 'express';
 import express from 'express';
 import session from 'express-session';
 import request from 'supertest';
 import authRoutes from './auth';
 import issueRoutes from './issue';
+import issueCommentRoutes from './issueComment';
 
 const sendIssueNotificationMock = mock.method(
   IssueSubscriber.prototype as unknown as {
@@ -51,6 +56,7 @@ function createApp() {
   app.use(checkUser);
   app.use('/auth', authRoutes);
   app.use('/issue', issueRoutes);
+  app.use('/issueComment', issueCommentRoutes);
   app.use(
     (
       err: { status?: number; message?: string },
@@ -215,3 +221,30 @@ describe('POST /issue', () => {
     assert.strictEqual(res.body.message, 'Issue user not found');
   });
 });
+
+describe('GET /issueComment/:commentId', () => {
+  it('omits notification settings from the comment author', async () => {
+    const userRepo = getRepository(User);
+    const media = await seedMedia();
+    const friend = await userRepo.findOneOrFail({
+      where: { email: '[email protected]' },
+    });
+    await seedUserSettings('[email protected]');
+
+    const agent = await loginAs('[email protected]', 'test1234');
+    const created = await agent.post('/issue').send({
+      issueType: IssueType.VIDEO,
+      message: 'Playback stutters near the end.',
+      mediaId: media.id,
+      userId: friend.id,
+    });
+    assert.strictEqual(created.status, 201);
+
+    const res = await agent.get(`/issueComment/${created.body.comments[0].id}`);
+
+    assert.strictEqual(res.status, 200);
+    assert.strictEqual(res.body.user.email, '[email protected]');
+    assert.ok(!('settings' in res.body.user));
+    assertNoCredentials(res.body);
+  });
+});

server/routes/request.test.ts

@@ -24,6 +24,10 @@ import { getSettings } from '@server/lib/settings';
 import { checkUser } from '@server/middleware/auth';
 import { MediaRequestSubscriber } from '@server/subscriber/MediaRequestSubscriber';
 import { setupTestDb } from '@server/test/db';
+import {
+  assertNoCredentials,
+  seedUserSettings,
+} from '@server/test/userSettings';
 import type { Express } from 'express';
 import express from 'express';
 import session from 'express-session';
@@ -807,6 +811,27 @@ describe('PUT /request/:requestId (quota)', () => {
   });
 });
 
+describe('GET /request/:requestId', () => {
+  it('omits notification settings from requestedBy and modifiedBy', async () => {
+    const pending = await seedRequest();
+    await seedUserSettings('[email protected]');
+    await seedUserSettings('[email protected]');
+
+    const admin = await loginAs('[email protected]', 'test1234');
+    const approved = await admin.post(`/request/${pending.id}/approve`);
+    assert.strictEqual(approved.status, 200);
+
+    const res = await admin.get(`/request/${pending.id}`);
+
+    assert.strictEqual(res.status, 200);
+    assert.strictEqual(res.body.requestedBy.email, '[email protected]');
+    assert.strictEqual(res.body.modifiedBy.email, '[email protected]');
+    assert.ok(!('settings' in res.body.requestedBy));
+    assert.ok(!('settings' in res.body.modifiedBy));
+    assertNoCredentials(res.body);
+  });
+});
+
 describe('POST /request/:requestId/:status', () => {
   const cases = [
     { action: 'approve', expected: MediaRequestStatus.APPROVED },

server/routes/user/index.test.ts

@@ -0,0 +1,147 @@
+import assert from 'node:assert/strict';
+import { before, describe, it } from 'node:test';
+
+import { MediaStatus, MediaType } from '@server/constants/media';
+import { getRepository } from '@server/datasource';
+import Media from '@server/entity/Media';
+import { User } from '@server/entity/User';
+import { Watchlist } from '@server/entity/Watchlist';
+import { getSettings } from '@server/lib/settings';
+import { checkUser, isAuthenticated } from '@server/middleware/auth';
+import authRoutes from '@server/routes/auth';
+import { setupTestDb } from '@server/test/db';
+import {
+  assertNoCredentials,
+  seedUserSettings,
+} from '@server/test/userSettings';
+import type { Express } from 'express';
+import express from 'express';
+import session from 'express-session';
+import request from 'supertest';
+import userRoutes from '.';
+
+let app: Express;
+
+function createApp() {
+  const app = express();
+  app.use(express.json());
+  app.use(
+    session({
+      secret: 'test-secret',
+      resave: false,
+      saveUninitialized: false,
+    })
+  );
+  app.use(checkUser);
+  app.use('/auth', authRoutes);
+  app.use('/user', isAuthenticated(), userRoutes);
+  app.use(
+    (
+      err: { status?: number; message?: string },
+      _req: express.Request,
+      res: express.Response,
+      // eslint-disable-next-line @typescript-eslint/no-unused-vars
+      _next: express.NextFunction
+    ) => {
+      res
+        .status(err.status ?? 500)
+        .json({ status: err.status ?? 500, message: err.message });
+    }
+  );
+  return app;
+}
+
+before(async () => {
+  app = createApp();
+});
+
+setupTestDb();
+
+async function loginAs(email: string, password: string) {
+  const settings = getSettings();
+  const priorLocalLogin = settings.main.localLogin;
+  settings.main.localLogin = true;
+
+  try {
+    const agent = request.agent(app);
+    const res = await agent.post('/auth/local').send({ email, password });
+    assert.strictEqual(res.status, 200);
+    return agent;
+  } finally {
+    settings.main.localLogin = priorLocalLogin;
+  }
+}
+
+describe('GET /user/:id/watchlist', () => {
+  it('omits notification settings from every requestedBy in the page', async () => {
+    const owner = await seedUserSettings('[email protected]');
+
+    const media = await getRepository(Media).save(
+      new Media({
+        mediaType: MediaType.MOVIE,
+        tmdbId: 12345,
+        status: MediaStatus.UNKNOWN,
+        status4k: MediaStatus.UNKNOWN,
+      })
+    );
+
+    await getRepository(Watchlist).save(
+      new Watchlist({
+        mediaType: MediaType.MOVIE,
+        tmdbId: 12345,
+        title: 'Watchlisted Movie',
+        ratingKey: 'rk-12345',
+        requestedBy: owner,
+        media,
+      })
+    );
+
+    const admin = await loginAs('[email protected]', 'test1234');
+    const res = await admin.get(`/user/${owner.id}/watchlist`);
+
+    assert.strictEqual(res.status, 200);
+    assert.strictEqual(res.body.results.length, 1);
+    assert.ok(!('settings' in res.body.results[0].requestedBy));
+    assertNoCredentials(res.body);
+  });
+});
+
+describe('GET /user/:id', () => {
+  it('still returns full settings to the user themselves', async () => {
+    const owner = await seedUserSettings('[email protected]');
+
+    const agent = await loginAs('[email protected]', 'test1234');
+    const res = await agent.get(`/user/${owner.id}`);
+
+    assert.strictEqual(res.status, 200);
+    assert.strictEqual(res.body.settings.pgpKey, 'test-pgp-key');
+    assert.strictEqual(
+      res.body.settings.pushoverUserKey,
+      'test-pushover-user-key'
+    );
+  });
+
+  it('still returns full settings to a manage-users admin', async () => {
+    const owner = await seedUserSettings('[email protected]');
+
+    const admin = await loginAs('[email protected]', 'test1234');
+    const res = await admin.get(`/user/${owner.id}`);
+
+    assert.strictEqual(res.status, 200);
+    assert.strictEqual(res.body.settings.pgpKey, 'test-pgp-key');
+  });
+
+  it('strips settings for an unrelated user', async () => {
+    const admin = await getRepository(User).findOneOrFail({
+      where: { email: '[email protected]' },
+    });
+    await seedUserSettings('[email protected]');
+
+    const agent = await loginAs('[email protected]', 'test1234');
+    const res = await agent.get(`/user/${admin.id}`);
+
+    assert.strictEqual(res.status, 200);
+    assert.ok(!('settings' in res.body));
+    assertNoCredentials(res.body);
+  });
+});

server/test/userSettings.ts

@@ -0,0 +1,52 @@
+import { getRepository } from '@server/datasource';
+import { User } from '@server/entity/User';
+import { UserSettings } from '@server/entity/UserSettings';
+import assert from 'node:assert/strict';
+
+const credentials = {
+  pgpKey: 'test-pgp-key',
+  discordIds: ['112233445566'],
+  pushbulletAccessToken: 'test-pushbullet-token',
+  pushoverApplicationToken: 'test-pushover-app-token',
+  pushoverUserKey: 'test-pushover-user-key',
+  telegramChatId: '-1001234567',
+};
+
+export const allowlistedSettings = {
+  locale: 'de',
+  discoverRegion: 'DE',
+  streamingRegion: 'DE',
+  originalLanguage: 'de',
+  watchlistSyncMovies: true,
+  watchlistSyncTv: false,
+};
+
+export async function seedUserSettings(email: string): Promise<User> {
+  const userRepository = getRepository(User);
+  const user = await userRepository.findOneOrFail({ where: { email } });
+
+  user.settings = new UserSettings({
+    ...credentials,
+    ...allowlistedSettings,
+    notificationTypes: {},
+  });
+
+  return userRepository.save(user);
+}
+
+export function assertNoCredentials(body: unknown): void {
+  const serialized = JSON.stringify(body);
+
+  const secrets = [
+    credentials.pgpKey,
+    ...credentials.discordIds,
+    credentials.pushbulletAccessToken,
+    credentials.pushoverApplicationToken,
+    credentials.pushoverUserKey,
+    credentials.telegramChatId,
+  ];
+
+  for (const secret of secrets) {
+    assert.ok(!serialized.includes(secret), `${secret} was serialized`);
+  }
+}