diff --git a/apps/server/src/core/page/dto/move-page.dto.spec.ts b/apps/server/src/core/page/dto/move-page.dto.spec.ts index 7b71995a..0f894ca5 100644 --- a/apps/server/src/core/page/dto/move-page.dto.spec.ts +++ b/apps/server/src/core/page/dto/move-page.dto.spec.ts @@ -17,9 +17,10 @@ import { MovePageDto } from './move-page.dto'; // 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. +// must satisfy the DTO. FIXED (#495 item 9): the DTO now validates `position` by +// CHARSET ([0-9A-Za-z], the generator's base-62 alphabet) instead of the wrong +// @MaxLength(12) length bound, so dense between-inserts are accepted; the former +// `test.failing` bug-lock is now a passing assertion. function constraintErrors(position: unknown) { const dto = plainToInstance(MovePageDto, { @@ -47,24 +48,33 @@ describe('MovePageDto.position vs generateJitteredKeyBetween parity', () => { 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); - }, - ); + // FIXED: dense between-inserts produce keys longer than 12 chars, which the old + // MaxLength(12) rejected even though they are valid ordering keys. Now accepted. + it('accepts dense between-inserted keys longer than 12 chars', async () => { + let lo = generateJitteredKeyBetween(null, null); + let hi = generateJitteredKeyBetween(lo, null); + // Repeatedly insert just above `lo`, shrinking the gap so the key grows. The + // generator is JITTERED (random), so use enough iterations that the longest + // key reliably clears the old 12-char bound: measured min-over-50-trials is + // ~11 at 40 iterations (flaky) but ~36 at 200 (robust margin). + let longest = lo; + for (let i = 0; i < 200; 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); + }); + + // The charset guard replaces the length bound: reject anything outside the + // generator's [0-9A-Za-z] alphabet (control chars, separators, injection) and + // the empty string, while still accepting every real key. + it('rejects a position with characters outside the fractional-index alphabet', async () => { + for (const bad of ['a0/b', 'a b', 'a\n0', 'a.b', '', "a';--"]) { + const errors = await constraintErrors(bad); + expect(hasError(errors, 'position')).toBe(true); + } + }); }); diff --git a/apps/server/src/core/page/dto/move-page.dto.ts b/apps/server/src/core/page/dto/move-page.dto.ts index 92dc2c5c..a8baabee 100644 --- a/apps/server/src/core/page/dto/move-page.dto.ts +++ b/apps/server/src/core/page/dto/move-page.dto.ts @@ -1,8 +1,8 @@ import { IsString, IsOptional, - MinLength, MaxLength, + Matches, IsNotEmpty, } from 'class-validator'; @@ -10,9 +10,19 @@ export class MovePageDto { @IsString() pageId: string; + // `position` is a fractional-indexing key from `generateJitteredKeyBetween` + // (the SAME generator page.service uses). Validate by CHARSET, not length: the + // generator's default base-62 alphabet is [0-9A-Za-z], and DENSE between-inserts + // legitimately grow a key well past a dozen chars (measured >40), so the old + // @MinLength(5)/@MaxLength(12) bounds wrongly 400'd valid ordering keys the + // server itself produced (Gitea #139 item 6). The charset regex rejects control + // chars / separators / injection, and a generous MaxLength stays only as a + // DoS guard — far above any realistic key, so it never rejects a real move. @IsString() - @MinLength(5) - @MaxLength(12) + @Matches(/^[0-9A-Za-z]+$/, { + message: 'position must be a fractional-index key ([0-9A-Za-z])', + }) + @MaxLength(256) position: string; @IsOptional()