fix(page): move-position — валидация charset вместо длины
MovePageDto.position — это fractional-indexing ключ (generateJitteredKeyBetween, тот же генератор, что в page.service). @MinLength(5)/@MaxLength(12) не совпадали с реальным диапазоном генератора: плотные between-вставки в глубоком дереве растят ключ далеко за 12 символов (замерено >40), и валидный ключ, который сервер сам сгенерил, отклонялся 400 (Gitea #139, п.6). Теперь валидируем по charset — base-62 алфавит [0-9A-Za-z] — плюс щедрый @MaxLength(256) как чистый DoS-guard, сильно выше любого реального ключа. test.failing (bug-lock) распинен в обычный it; добавлен кейс на отклонение символов вне алфавита (control/separator/инъекция/пустая строка). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user