Merge pull request 'test: unit tests for the 10 candidates (#139)' (#142) from test/unit-tests-139 into develop

Reviewed-on: #142

apps/server/src/core/label/dto/label.dto.spec.ts

@@ -0,0 +1,78 @@
+import 'reflect-metadata';
+import { plainToInstance } from 'class-transformer';
+import { validate, Matches } from 'class-validator';
+import { AddLabelsDto } from './label.dto';
+
+// API-boundary validation for label names. `AddLabelsDto.names` applies the
+// matcher /^[a-z0-9_-][a-z0-9_~-]*$/ to every element (each: true): a name must
+// start with a lowercase letter, digit, hyphen or underscore (NOT a tilde) and
+// then contain only those plus tildes. This guards the label storage key against
+// uppercase, whitespace, accents and tilde-leading names.
+//
+// NOTE: the production DTO also runs `@Transform(normalizeLabelName)` BEFORE the
+// matcher (trim + collapse whitespace to '-' + lowercase). `normalizeLabelName`
+// itself is already covered (utils.spec.ts), so we deliberately do two things:
+//  1) lock the raw @Matches regex in isolation (a mirror DTO with ONLY the same
+//     matcher) for the exact accept/reject set the regex must enforce; and
+//  2) sanity-check the real AddLabelsDto end-to-end for inputs whose normalized
+//     form still exercises the matcher.
+
+// Mirrors ONLY the production matcher so we test the regex, not the transform.
+class NameMatchProbe {
+  @Matches(/^[a-z0-9_-][a-z0-9_~-]*$/)
+  name: string;
+}
+
+async function matcherErrors(name: string) {
+  const dto = plainToInstance(NameMatchProbe, { name });
+  return validate(dto as object);
+}
+
+function hasError(errors: any[], property: string, constraint?: string) {
+  const err = errors.find((e) => e.property === property);
+  if (!err) return false;
+  if (!constraint) return true;
+  return Object.keys(err.constraints ?? {}).includes(constraint);
+}
+
+describe('label name @Matches regex', () => {
+  it('accepts valid names', async () => {
+    for (const name of ['foo', 'a~b', '1-2_3', '-lead']) {
+      expect(hasError(await matcherErrors(name), 'name', 'matches')).toBe(false);
+    }
+  });
+
+  it('rejects a tilde-leading name', async () => {
+    expect(hasError(await matcherErrors('~lead'), 'name', 'matches')).toBe(true);
+  });
+
+  it('rejects whitespace, accents and empty', async () => {
+    expect(hasError(await matcherErrors('a b'), 'name', 'matches')).toBe(true);
+    expect(hasError(await matcherErrors('héllo'), 'name', 'matches')).toBe(true);
+    expect(hasError(await matcherErrors(''), 'name', 'matches')).toBe(true);
+  });
+});
+
+describe('AddLabelsDto.names (matcher applied per element)', () => {
+  async function validateNames(names: unknown) {
+    const dto = plainToInstance(AddLabelsDto, { pageId: 'p1', names });
+    return validate(dto as object);
+  }
+
+  it('accepts a list of valid names', async () => {
+    const errors = await validateNames(['foo', 'a~b', '1-2_3']);
+    expect(hasError(errors, 'names', 'matches')).toBe(false);
+  });
+
+  it('rejects a tilde-leading name even after normalization', async () => {
+    // normalizeLabelName lowercases/collapses whitespace but does not strip a
+    // leading tilde, so the matcher still fails.
+    const errors = await validateNames(['~lead']);
+    expect(hasError(errors, 'names', 'matches')).toBe(true);
+  });
+
+  it('rejects an accented name even after normalization', async () => {
+    const errors = await validateNames(['héllo']);
+    expect(hasError(errors, 'names', 'matches')).toBe(true);
+  });
+});

apps/server/src/core/notification/notification.constants.spec.ts

@@ -0,0 +1,77 @@
+import {
+  NotificationType,
+  DIRECT_NOTIFICATION_TYPES,
+  UPDATES_NOTIFICATION_TYPES,
+  getTypesForTab,
+} from './notification.constants';
+
+// Contract tests for `getTypesForTab` (notification.constants.ts), which maps a
+// notification tab to the set of notification types it should contain.
+//   - 'direct'  -> a 5-type whitelist (mentions / comments / permission grants)
+//   - 'updates' -> exactly [PAGE_UPDATED]
+//   - 'all'     -> undefined (no type filter)
+
+describe('getTypesForTab', () => {
+  it("returns exactly the 5 whitelisted types for 'direct'", () => {
+    expect(getTypesForTab('direct')).toEqual([
+      NotificationType.COMMENT_USER_MENTION,
+      NotificationType.COMMENT_CREATED,
+      NotificationType.COMMENT_RESOLVED,
+      NotificationType.PAGE_USER_MENTION,
+      NotificationType.PAGE_PERMISSION_GRANTED,
+    ]);
+    expect(getTypesForTab('direct')).toHaveLength(5);
+    expect(getTypesForTab('direct')).toBe(DIRECT_NOTIFICATION_TYPES);
+  });
+
+  it("returns [PAGE_UPDATED] for 'updates'", () => {
+    expect(getTypesForTab('updates')).toEqual([NotificationType.PAGE_UPDATED]);
+    expect(getTypesForTab('updates')).toBe(UPDATES_NOTIFICATION_TYPES);
+  });
+
+  it("returns undefined (no filter) for 'all'", () => {
+    expect(getTypesForTab('all')).toBeUndefined();
+  });
+});
+
+// CONTRACT vs the repository query (notification.repo.ts ~line 57):
+//   direct  -> WHERE type != PAGE_UPDATED
+//   updates -> WHERE type =  PAGE_UPDATED
+//
+// For 'updates' the whitelist and the SQL agree exactly. For 'direct' they
+// DIVERGE: the whitelist is a positive 5-type allow-list, but `type != PAGE_UPDATED`
+// returns EVERY non-PAGE_UPDATED type — including verification/approval types that
+// are NOT in the whitelist. So the repo would surface notifications the 'direct'
+// tab is not supposed to contain. We model the repo predicate and assert it should
+// match the whitelist; the 'direct' case genuinely fails today, so it is locked with
+// `test.failing` (suite stays green, flips red once repo + whitelist are reconciled).
+
+// What the repo's WHERE clause would actually return, given all known types.
+const ALL_TYPES = Object.values(NotificationType);
+function repoTypesForTab(tab: 'direct' | 'updates'): string[] {
+  if (tab === 'direct') {
+    return ALL_TYPES.filter((t) => t !== NotificationType.PAGE_UPDATED);
+  }
+  return ALL_TYPES.filter((t) => t === NotificationType.PAGE_UPDATED);
+}
+
+describe('getTypesForTab vs notification.repo query', () => {
+  it("'updates' whitelist matches the repo's `type = PAGE_UPDATED` filter", () => {
+    expect(new Set(repoTypesForTab('updates'))).toEqual(
+      new Set(getTypesForTab('updates')),
+    );
+  });
+
+  // BUG LOCK: the 'direct' whitelist (5 types) does not match what the repo's
+  // `type != PAGE_UPDATED` filter returns (all non-PAGE_UPDATED types). This SHOULD
+  // match; it currently does not. Flips green once the repo filters by the whitelist
+  // (e.g. `type IN (DIRECT_NOTIFICATION_TYPES)`).
+  test.failing(
+    "'direct' whitelist matches the repo's `type != PAGE_UPDATED` filter",
+    () => {
+      expect(new Set(repoTypesForTab('direct'))).toEqual(
+        new Set(getTypesForTab('direct')),
+      );
+    },
+  );
+});

apps/server/src/core/page/dto/move-page.dto.spec.ts

@@ -0,0 +1,70 @@
+import 'reflect-metadata';
+import { plainToInstance } from 'class-transformer';
+import { validate } from 'class-validator';
+// Imported exactly as page.service.ts does, so we test the real key generator
+// that feeds `position` at the API boundary.
+import { generateJitteredKeyBetween } from 'fractional-indexing-jittered';
+import { MovePageDto } from './move-page.dto';
+
+// PARITY BUG (Gitea #139, item 6): MovePageDto.position is bounded with
+// @MinLength(5) @MaxLength(12), but the actual positions are fractional-indexing
+// keys produced by `generateJitteredKeyBetween` (the same generator page.service
+// uses). Those bounds do NOT match the generator's real output range:
+//   - a freshly generated key (null,null) is short (~5 chars) and currently
+//     squeaks past MinLength(5);
+//   - but DENSE between-inserts (repeatedly inserting between two adjacent keys)
+//     grow the key well past 12 chars, which MaxLength(12) would WRONGLY reject —
+//     a valid ordering key the server itself generated would be refused on move.
+//
+// The tests below assert the CORRECT contract: any key the generator can produce
+// must satisfy the DTO. The genuinely-failing case is marked `test.failing` so the
+// suite stays green while locking the bug; it flips red (alerting us) once the DTO
+// bounds are widened to cover the generator's real range.
+
+function constraintErrors(position: unknown) {
+  const dto = plainToInstance(MovePageDto, {
+    pageId: 'page-1',
+    position,
+  });
+  return validate(dto as object);
+}
+
+function hasError(errors: any[], property: string) {
+  return errors.some((e) => e.property === property);
+}
+
+describe('MovePageDto.position vs generateJitteredKeyBetween parity', () => {
+  it('accepts a freshly generated first key', async () => {
+    const key = generateJitteredKeyBetween(null, null);
+    const errors = await constraintErrors(key);
+    expect(hasError(errors, 'position')).toBe(false);
+  });
+
+  it('accepts a key appended after an existing key', async () => {
+    const first = generateJitteredKeyBetween(null, null);
+    const next = generateJitteredKeyBetween(first, null);
+    const errors = await constraintErrors(next);
+    expect(hasError(errors, 'position')).toBe(false);
+  });
+
+  // BUG LOCK: dense between-inserts produce keys longer than 12 chars, which
+  // MaxLength(12) rejects even though they are valid ordering keys. This SHOULD
+  // pass; it currently fails. Flips green when the DTO bound is fixed.
+  test.failing(
+    'accepts dense between-inserted keys (currently rejected by MaxLength(12))',
+    async () => {
+      let lo = generateJitteredKeyBetween(null, null);
+      let hi = generateJitteredKeyBetween(lo, null);
+      // Repeatedly insert just above `lo`, shrinking the gap so the key grows.
+      let longest = lo;
+      for (let i = 0; i < 40; i++) {
+        const mid = generateJitteredKeyBetween(lo, hi);
+        if (mid.length > longest.length) longest = mid;
+        hi = mid;
+      }
+      expect(longest.length).toBeGreaterThan(12); // sanity: we produced a long key
+      const errors = await constraintErrors(longest);
+      expect(hasError(errors, 'position')).toBe(false);
+    },
+  );
+});

apps/server/src/core/search/search.service.spec.ts

@@ -1,4 +1,4 @@
-import { SearchService } from './search.service';
+import { SearchService, buildTsQuery } from './search.service';
 
 describe('SearchService', () => {
   it('should be defined', () => {
@@ -99,3 +99,59 @@ describe('SearchService.searchSuggestions — onlyTemplates filter', () => {
     expect(isTemplateWhereCall(pageBuilder)).toBeUndefined();
   });
 });
+
+// Unit tests for `buildTsQuery` (extracted from search.service.ts). It turns a raw
+// user query into a prefix tsquery string fed to `to_tsquery('english', ...)`.
+//
+// REAL BUG (Gitea #139, item 10): the previous inline `tsquery(query.trim() + '*')`
+// let to_tsquery operator characters through, so adversarial inputs could produce a
+// fragment that to_tsquery rejects -> 500. The extraction sanitizes the input
+// (strip everything but letters/numbers/whitespace) so these inputs degrade to a
+// safe, neutral query with NO throw, while normal queries keep working.
+describe('buildTsQuery', () => {
+  it('builds a prefix query for a normal single word', () => {
+    expect(buildTsQuery('hello')).toBe('hello:*');
+  });
+
+  it('joins multiple words with AND and a trailing prefix match', () => {
+    expect(buildTsQuery('foo bar')).toBe('foo&bar:*');
+  });
+
+  it('preserves accented and non-Latin words', () => {
+    expect(buildTsQuery('héllo café')).toBe('héllo&café:*');
+    expect(buildTsQuery('日本語')).toBe('日本語:*');
+  });
+
+  it('neutralizes to_tsquery operator inputs without throwing', () => {
+    // Each of these previously risked an invalid to_tsquery -> 500. They must now
+    // produce a safe (here empty) query and never throw.
+    for (const input of ['&', '!', '*', '<->', '\\']) {
+      expect(() => buildTsQuery(input)).not.toThrow();
+      expect(buildTsQuery(input)).toBe('');
+    }
+  });
+
+  it('handles stopword-only input safely', () => {
+    // pg-tsquery still tokenizes stopwords; to_tsquery reduces them to nothing.
+    // The important contract is: no throw, and a deterministic string.
+    expect(() => buildTsQuery('the a of')).not.toThrow();
+    expect(buildTsQuery('the a of')).toBe('the&a&of:*');
+  });
+
+  it('returns empty string for empty / whitespace-only / null-ish input', () => {
+    expect(buildTsQuery('')).toBe('');
+    expect(buildTsQuery('   ')).toBe('');
+    expect(buildTsQuery(undefined as unknown as string)).toBe('');
+  });
+
+  it('handles a very long input without throwing', () => {
+    const long = 'a'.repeat(10000);
+    expect(() => buildTsQuery(long)).not.toThrow();
+    expect(buildTsQuery(long)).toBe(`${long}:*`);
+  });
+
+  it('strips punctuation embedded in otherwise valid words', () => {
+    expect(buildTsQuery('c++ code')).toBe('c&code:*');
+    expect(buildTsQuery('a-b-c')).toBe('a&b&c:*');
+  });
+});

apps/server/src/core/search/search.service.ts

@@ -12,6 +12,28 @@ import { PagePermissionRepo } from '@docmost/db/repos/page/page-permission.repo'
 // eslint-disable-next-line @typescript-eslint/no-require-imports
 const tsquery = require('pg-tsquery')();
 
+// Build a safe prefix tsquery string from a raw user query.
+//
+// The previous inline form `tsquery(query.trim() + '*')` passed user input
+// (including to_tsquery operators like `&`, `|`, `!`, `<->`, `*`, backslashes)
+// straight through. pg-tsquery would then emit operator fragments that
+// `to_tsquery('english', ...)` can reject as a syntax error, turning a search
+// into a 500. We strip everything that is not a letter, number or whitespace
+// BEFORE handing the text to pg-tsquery, so adversarial input degrades to a
+// neutral (possibly empty) query instead of throwing, while normal word queries
+// (incl. accented / non-Latin words) are unaffected.
+export function buildTsQuery(raw: string): string {
+  const cleaned = (raw ?? '')
+    .normalize('NFC')
+    // Keep Unicode letters/numbers and whitespace; drop everything else.
+    .replace(/[^\p{L}\p{N}\s]+/gu, ' ')
+    .replace(/\s+/g, ' ')
+    .trim();
+
+  if (!cleaned) return '';
+  return tsquery(cleaned + '*');
+}
+
 @Injectable()
 export class SearchService {
   constructor(
@@ -34,7 +56,7 @@ export class SearchService {
     if (query.length < 1) {
       return { items: [] };
     }
-    const searchQuery = tsquery(query.trim() + '*');
+    const searchQuery = buildTsQuery(query);
 
     let queryResults = this.db
       .selectFrom('pages')