Skip to content

fix: индекси за листовите сортове + екраниране на JSON-LD (перформанс + hardening) - #212

Merged
todorkolev merged 14 commits into
midt-bg:mainfrom
B353N:fix/list-hardening
Jul 29, 2026
Merged

Conversation

@B353N

@B353N B353N commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Консолидира двете доказуеми находки от прегледите (перформанс + сигурност) в един бранч. Заменя #211.

1. perf(db): индекси за подредбата на неполагащите листови сортове

Листовете странират с keyset ORDER BY <израз> <посока>, <id> <посока> LIMIT N. Дефолтните сортове имат съвпадащ индекс; шест избираеми сорта нямаха - планировчикът правеше SCAN <table> + USE TEMP B-TREE FOR ORDER BY, т.е. сканираше и сортираше цялата таблица преди LIMIT на всяка страница (D1 таксува сканираните редове).

Страница Сорт без индекс Причина
/contracts date-desc, date-asc idx_contracts_signed е на голото signed_at, а заявката подрежда по COALESCE(signed_at, …)
/companies count, authorities няма индекс на тези колони
/authorities count, avg няма индекс на тези колони

Миграция 0002_list_sort_indexes.sql добавя по един индекс, съвпадащ с точния ORDER BY израз + keyset id tiebreak. Адитивна, идемпотентна; rollup-ите се опресняват с DELETE+INSERT, затова индексите преживяват ship. Измерено при 200k договора: date-desc пада от ~0.30s на <10ms.

Съзнателно изключени (доказано, че не си струват): id-tiebreak на дефолтните индекси - бенчмаркът показа 0 измерима полза (single-col вече е ~0ms дори при много еднакви стойности), само излишен write-cost при refresh. ANALYZE - показано, че може да регресира /contracts value-desc до SCAN authorities.

2. fix(web): екраниране на < в JSON-LD data island-а (defense-in-depth)

root.tsx слага JSON-LD през dangerouslySetInnerHTML със суров JSON.stringify. JSON.stringify не екранира <, така че </script> в която и да е стойност би затворил <script> елемента рано (stored XSS) - точно sink-ът, който собственият ви стандарт (docs/review-security.md) изисква да е екраниран.

Не е експлоатируемо днес (единствената вкарана стойност е origin, а new URL() хвърля при host с </script>), затова е defense-in-depth: новият jsonLdScript helper затваря sink-а превантивно за всяко бъдещо поле от базата/потребителя в графа. Екранира << (JSON-еквивалентно) + U+2028/U+2029.

Доказателство (тестове)

  • packages/db/src/list-sort-indexes.test.ts - прилага миграциите на реален sqlite3 и за всеки от 6-те сорта проверява EXPLAIN QUERY PLAN: ПРЕДИ 0002 планът има USE TEMP B-TREE FOR ORDER BY (дефектът), СЛЕД - върви по новия индекс без sort step.
  • apps/web/app/lib/json-ld.test.ts - < се екранира (няма </script> breakout), U+2028/2029 се екранират, изходът остава JSON-еквивалентен.

Вид промяна

  • perf — индекси (без промяна в резултатите на заявките)
  • fix — security hardening (JSON-LD sink)

Как е тествано

  • pnpm typecheck - минава.
  • pnpm test - минава: @sigma/web 339 (вкл. 4 нови JSON-LD), @sigma/db 195 (вкл. 12 нови plan теста).
  • pnpm lint - чисто.

Чеклист

  • Conventional commits, без Co-Authored-By:
  • Форк → midt-bg/sigma:main
  • typecheck / test / lint минават
  • Миграцията е адитивна и идемпотентна (IF NOT EXISTS)
  • Няма тайни / .env* / .dev.vars

Бележки за координация

B353N added 2 commits July 4, 2026 12:17
The list pages keyset-paginate with ORDER BY <sortExpr> <dir>, <id> <dir> LIMIT N.
Six user-selectable sorts had no matching index, so the planner fell back to a
full table SCAN + temp-B-tree ORDER BY on every page (D1 bills rows scanned):

  /contracts   date-desc, date-asc   (idx_contracts_signed is on the bare column,
                                       not the COALESCE(signed_at, ...) expr the query uses)
  /companies   count, authorities
  /authorities count, avg

Add one index per missing sort, matching the exact ORDER BY expression plus the
keyset id tiebreak, so SQLite walks the index and stops at LIMIT. Additive,
idempotent; rollup tables are DELETE+INSERT-refreshed so the indexes survive ships.
A sqlite3 EXPLAIN QUERY PLAN test proves each sort full-scans before and index-walks
after.
root.tsx embeds JSON-LD via dangerouslySetInnerHTML with a raw JSON.stringify.
JSON.stringify does not escape '<', so a '</script>' in any string value would
close the <script> element early (stored XSS) — the exact sink the project's own
review standard (docs/review-security.md) requires be escaped. Today only the
request origin reaches the graph (new URL() cannot make it carry '</script>'), so
this is not currently exploitable; the jsonLdScript helper closes the sink
pre-emptively for any DB/user-derived field added later. A unit test proves '<' is
escaped, U+2028/U+2029 are escaped, and the output stays JSON-equivalent.
@ydimitrof

Copy link
Copy Markdown
Contributor

Благодаря за прегледната и добре документирана заявка. Прочетох целия diff, PR описанието, двата коммита и локално сверих индексите спрямо реалния код на заявките, keyset строителя, съществуващите миграции и XSS sink-а. По-долу е резултатът.

Обхват

Два доказуеми hardening-a в един бранч (заменя #211):

  1. perf(db) — 6 ordering индекса за неполагащите листови сортове (0002_list_sort_indexes.sql).
  2. fix(web) — екраниране на < (+ U+2028/U+2029) в JSON-LD data island-а през новия jsonLdScript helper.

Сигурност и целостност на данните — чисто

SQL injection — няма. Сортовете минават през allowlist преди да достигнат SQL:

  • SORTS е lookup()-обект с null-прототип; normalize*Sort пропуска само стойност, за която value in SORTS, иначе пада на дефолта (value-desc/won/spent).
  • keyset.ts допълнително минава колоните през assertSafeColumn срещу allowedSortCols/allowedIdCols и assertSortDir преди интерполация. Никаква потребителска стойност не влиза сурова в ORDER BY.

Индексите съвпадат байт-по-байт с емитирания ORDER BY — сверено срещу изворния код, не само срещу PR текста:

  • contracts date-desc → COALESCE(c.signed_at, '') DESC, c.id DESCidx_contracts_signed_desc
  • contracts date-asc → COALESCE(c.signed_at, '9999-99') ASC, c.id ASCidx_contracts_signed_asc
  • company_totals count(contracts)/authorities DESC, bidder_id DESC ✔
  • authority_totals count(contracts)/avg_eur DESC, authority_id DESC ✔

keyset.ts:156 строи ORDER BY <col> <dir>, <idCol> <dir> с една и съща посока за двете колони, а tiebreak-ът на индекса е в същата посока — значи SQLite нито сортира, нито буферира; за before курсор посоката се инвертира и SQLite обхожда същия индекс наобратно. Тестовете list-sort-indexes.test.ts доказват това с EXPLAIN QUERY PLAN на реален sqlite3 без ANALYZE (както е в production D1): ПРЕДИ — USE TEMP B-TREE FOR ORDER BY, СЛЕД — index-walk без sort стъпка. Съзнателните изключения (id-tiebreak на дефолтите, ANALYZE) са аргументирани.

XSS sink — коректно затворен. jsonLdScript екранира <\u003c (достатъчно, за да не се формира </script>) плюс U+2028/2029; изходът остава JSON-еквивалентен (round-trip тестван). Sink-ът е <script type="application/ld+json" dangerouslySetInnerHTML> в root.tsx:128. Днес единствената вкарана стойност е new URL(request.url).origin, който не може да носи </script> — т.е. твърдението за defense-in-depth, а не текущ експлойт, е вярно. Съответства на docs/review-security.md и на safeJson от contract.json.tsx.

Проверих и за backdoor/обфускация/промяна на URL-и/нови зависимости — няма. Миграцията е адитивна и идемпотентна (IF NOT EXISTS), а rollup таблиците се опресняват с DELETE+INSERT, така че индексите преживяват ship.

Дребни бележки (не блокират)

  • list-sort-indexes.test.ts изисква наличен sqlite3 CLI в CI средата (execFileSync). Ако runner-ът го няма, тестът ще падне на средата, не на кода — струва си да се потвърди, че е наличен.
  • Планът се доказва само за нефилтрираната листа. С активни WHERE филтри планировчикът може да избере друг път — приемливо за дефолтната страница (най-честият случай), но е честно да се отбележи, че покритието не се простира върху филтрираните заявки.
  • Номерът 0002 се застъпва с този на feat(web): „Подобни договори" - ценови ориентир по CPV кохорта на страницата на договора #210 — вече отбелязано в описанието: който се мерджне втори, преномерира на 0003. Моля не забравяйте стъпката при мерджа.

Нищо от горното не е дефект в самата промяна.

Вердикт: Approve (одобрявам на същество) — сигурност и целостност на данните чисти, OWASP-съвместимо; преди мердж уредете само номерацията на миграцията спрямо #210 и потвърдете sqlite3 в CI.

@lyubomir-bozhinov

Copy link
Copy Markdown
Collaborator

Одобрявам (088476d): четирите covering индекса съвпадат точно с ORDER BY + keyset посоката на query слоя (idx_contracts_signed_desc (COALESCE(signed_at,'') DESC, id DESC)SORTS['date-desc'] + keyset({dir:'desc'}), ..._signed_ascdate-asc, company/authority count/avg също). list-sort-indexes.test.ts доказва през EXPLAIN QUERY PLAN, че преди миграцията се хваща USE TEMP B-TREE FOR ORDER BY, а след нея индексът се обхожда без sort — правилният метод за проверка. JSON-LD: << (затваря </script>/<!-- breakout) + U+2028/U+2029 — стандартното JSON-еквивалентно hardening; root.tsx подава само origin от new URL().

Бележка (cross-PR): този PR добавя 0002_list_sort_indexes.sql, а #172 (0002_contracts_overrun_index) и #210 (0002_cpv_division_stats) също claim-ват 0002. Който влезе пръв, останалите се преномерират на 0003+ преди merge (иначе два 0002 при merge).

@nedda76 nedda76 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Прегледах двете части — чисто.

A. JS екраниране (XSS). jsonLdScript е коректен фикс: екранира всяко < като < (обезврежда </script, <!-- и double-escape странностите) + U+2028/U+2029, и остава JSON-еквивалентно. Приложен е върху единствения inline-script sink в приложението (JSON-LD острова в root.tsx); всичко останало минава през auto-escape на React. Няма останал reflected/stored XSS път — включително през атакуемо DB поле (напр. фирма, регистрирана с <script в името).

B. Индекси. И шестте нови индекса точно съвпадат с ORDER BY <expr> <dir>, <id> <dir>, който keyset пейджърът издава (вкл. COALESCE(signed_at,'') изразите — затова не дублират заварения idx_contracts_signed); никой не дублира съществуващ индекс; migration-ът е нов, адитивен, идемпотентен (CREATE INDEX IF NOT EXISTS). Уговорка: индексите ускоряват само нефилтрирания списък — при активни филтри source() минава към derived GROUP BY, който не може да ги ползва. Присъщо на подхода; hot path-ът (списък по подразбиране) е покрит правилно.

Координация: #212 и #210 добавят по един 0002_* migration — който влезе втори, трябва да се преномерира на 0003, иначе един от двата тихо няма да се приложи.

@B353N
B353N requested a review from nedda76 July 8, 2026 05:31
@lyubomir-bozhinov

Copy link
Copy Markdown
Collaborator

Проверих отново (088476d) — по същество добър и проверен емпирично:

  • Индекси: EXPLAIN QUERY PLAN върху реална схема (0000+0001, без ANALYZE — както е в D1) потвърждава, че и шестте сорта минават по новите индекси; USE TEMP B-TREE FOR ORDER BY изчезва. , id DESC tie-break-ът е носещ за keyset страница 2 — новите индекси са по-добре оформени от вече наличните на main (idx_contracts_value_desc още прави частичен temp B-tree).
  • JSON-LD escaping: единствен emit site (root.tsx:127); адверсариални payload-и (</script>, <!--, U+2028/U+2029, </SCRIPT >) — нито един суров < не оцелява в изхода (всички се unicode-escape-ват), round-trip е запазен. Няма bypass.
  • Тестове: реални, адверсариални (breakout + unicode + before/after temp-B-tree).

Едно cross-PR нещо преди merge (merge-ordering, не дефект тук): миграцията е 0002_list_sort_indexes.sql, но #170/#171/#172 ползват 0002_contracts_overrun_index.sql — същият номер, различен файл. wrangler подрежда по име, така че няма runtime break, но нарушава конвенцията един-номер-един-файл, а migrations.test.ts от #170/#172 вероятно ще падне при два 0002. Който влезе втори — да преномерира (с оглед и на 0003 от #210/#188). Изборът е на @todorkolev.

Одобрявам кода; renumber-ът е merge-hygiene.

De-conflict the migration number: 0002 is claimed by the contracts_overrun_index
family (midt-bg#169/midt-bg#170/midt-bg#171/midt-bg#172), 0003 by midt-bg#188 (contract_health), and 0004 by midt-bg#210
(cpv_division_stats). 0005 is the next free number. Additive/idempotent, so final
merge order stays the maintainer's call; this just removes the known 0002 clash.
@B353N

B353N commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Преномерирах миграцията 0002_list_sort_indexes0005_list_sort_indexes в 7b60ff8, за да махна колизията на номера. Актуалното разпределение на новите миграции по отворените PR-ове:

Обextsingle референция в list-sort-indexes.test.ts (пътя + коментарите за преди/след); EXPLAIN QUERY PLAN тестът минава непроменен. Миграцията е адитивна и идемпотентна (CREATE INDEX IF NOT EXISTS), затова финалният ред на merge остава на @todorkolev - ако някой от по-долните номера не влезе преди този, може да се смъкне без риск.

@ydimitrof ydimitrof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Преглед на PR: индекси за листовите сортове + екраниране на JSON-LD

Обща оценка: солиден, добре мотивиран PR. Двете промени са атомарни, добре документирани в коментарите и покрити с тестове. Приемам с няколко бележки (COMMENT), нито една от които не е блокираща.

Фаза 0 — Security скан: ЧИСТО ✅

  • Няма хардкоднати тайни (API ключове/пароли/токени).
  • Няма нови/променени URL адреси извън whitelist (https://schema.org, https://sigma.midt.bg само в тест).
  • Няма зловредни шаблони (backdoor, инжекция, обфускация).
  • Няма нови зависимости.
  • Промяната в json-ld.ts е всъщност security hardening (затваря потенциален stored-XSS през </script> в JSON-LD).

Силни страни

  • jsonLdScript екранира << и U+2028/U+2029 — коректно и JSON-еквивалентно (round-trip се запазва). Тестовете доказват точно правилното нещо (breakout последователността не оцелява, а injection-free съдържанието остава байт-идентично на JSON.stringify).
  • Миграцията 0005 е адитивна и идемпотентна (CREATE INDEX IF NOT EXISTS), а всеки индекс съвпада ТОЧНО с ORDER BY израза, вкл. COALESCE формите и keyset tiebreak посоката.
  • Тестът с реален sqlite3 без ANALYZE доказва и дефекта (temp B-tree сорт ПРЕДИ), и поправката (walk на индекса БЕЗ сорт стъпка) — това е тест, който разкрива, а не заобикаля проблема.

Бележки (незадължителни)

  1. Тестът пропуска междинните миграции (0002–0004) — прилага само 0000, 0001 и 0005. Ако някоя междинна миграция вече добавя конкуриращ индекс или е нужна за схемата, „BEFORE" базата не отговаря на реалния main. Моля потвърдете, че company_totals/authority_totals се създават в 0000 и че нищо между 0001 и 0005 не влияе на плана.
  2. Опростени заявки в теста — тестваните SELECT-и нямат WHERE (нито keyset курсора, нито евентуални филтри на листовите страници). Планът може да е различен при филтрирана заявка. Ако страниците поддържат филтри, добре е да се покрие поне един филтриран вариант.
  3. jsonLdScript(undefined) би хвърлил (JSON.stringify(undefined) връща undefined, а .replace гърми). Извикващите винаги подават обект, така че е нискорисково, но за defense-in-depth помощник си струва да се обмисли.

Quality gates

  • Security: ✅ (Phase 0 clean + hardening)
  • Тестове: покриват новия код добре; лека липса при филтрирани заявки.
  • Code quality / стил: консистентен, следва съществуващите шаблони (safeJson в contract.json.tsx).
  • Документация: коментарите в кода са изчерпателни; няма breaking changes.
  • Performance: подобрение (спира full-scan + temp B-tree сорт).

Препоръка: COMMENT — може да се мърджне след кратко потвърждение по бележка №1.

Comment thread packages/db/src/list-sort-indexes.test.ts Outdated
Comment thread packages/db/src/list-sort-indexes.test.ts
Comment thread apps/web/app/lib/json-ld.ts Outdated
…pt(undefined)

Address the review notes on the list-sort-indexes PR:

1. The sort-index test now applies EVERY migration on the branch (discovered from
   the migrations dir), not a hardcoded 0000/0001/000N subset. The "BEFORE" base is
   exactly the real served schema minus this PR's index, and the test survives any
   renumbering. (Confirmed: company_totals/authority_totals are created in 0000 and
   nothing between affects these sort plans.)

2. Each sort now asserts the plan on the keyset page too - the real paginated path
   `WHERE (expr <cmp> ? OR (expr = ? AND id <cmp> ?))`, not only the first page.
   Full-scans BEFORE and index-walks (no temp B-tree) AFTER, on both pages.

3. jsonLdScript now returns "null" when JSON.stringify yields undefined (undefined /
   function / symbol) instead of throwing on the following .replace - defense-in-depth
   for the documented "safe for any future field" helper. Covered by a test.
@B353N

B353N commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Благодаря - и трите адресирани в bd6101a.

#1 (пропуснати междинни миграции): тестът вече чете директорията с миграциите и прилага всички (без sort-index за „BEFORE", + sort-index за „AFTER"). Така базата е точно реалната served схема минус този индекс, и тестът преживява преномериране. Потвърдено: company_totals/authority_totals се създават в 0000, а 0001 е само индекс на flow_pairs(bidder_id) - нищо между не влияе на плановете на тези сортове.

#2 (опростени заявки без WHERE): всеки сорт вече проверява и keyset страницата - реалният странициращ път WHERE (expr <cmp> ? OR (expr = ? AND id <cmp> ?)), не само първата. Планът: full-scan + temp B-tree ПРЕДИ, index walk без сорт стъпка СЛЕД - и на двете страници. (Курсорът seek-ва през същия композитен индекс, вкл. , id tiebreak-а.)

#3 (jsonLdScript(undefined)): добавен guard - връща 'null', ако JSON.stringify даде undefined (undefined/функция/символ), вместо .replace да гръмне. + тест.

typecheck / test (db 195, web 340) / lint минават.

@ydimitrof ydimitrof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Преглед на PR: индекси за листовите сортове + екраниране на JSON-LD

Обща оценка: 9.5/10 — APPROVE. PR-ът е малък, атомарен, добре документиран и с изключително стабилни тестове. Двете промени (перформанс индекси + XSS hardening) са свързани логично и всяка е покрита с целеви тест.

Фаза 0 — Security scan: ЧИСТО ✅

  • Няма hardcoded secrets, пароли или токени.
  • Няма нови зависимости.
  • Няма подозрителни/обфускирани шаблони или backdoor код.
  • Единствените URL-и са https://schema.org (стандартен JSON-LD context) и тестова стойност — не са външни/непроверени.
  • PR-ът реално подобрява сигурността (екраниране на JSON-LD sink-а).

Сигурност (agent-level): 1.0/1.0 ✅

Екранирането на << е коректното и достатъчно решение срещу </script> breakout вътре в inline <script>. Тъй като <script>, </script>, <!-- и ]]> в raw-text контекст на <script> елемент имат значение единствено чрез символа <, екранирането само на < затваря дупката напълно. Допълнителното екраниране на U+2028/U+2029 е коректна defense-in-depth мярка. Обработката на случая, когато JSON.stringify връща undefined (undefined/функция/символ) → 'null', предотвратява хвърляне на грешка при .replace. Много добре.

Тестове: 3.0/3.0 ✅

  • json-ld.test.ts покрива същественото: breakout последователността не оцелява, JSON-еквивалентност (round-trip), U+2028/U+2029 екраниране, byte-identичност за безопасно съдържание и undefined/функция → 'null'. Тестовете разкриват дефекти, а не минават тривиално.
  • list-sort-indexes.test.ts доказва дефекта (temp B-tree сортиране BEFORE) и поправката (walk на индекса без sort стъпка AFTER) върху реален sqlite3 без ANALYZE, и то за първа И за keyset страница — точно правилният подход. Прилага всички миграции на клона, не hardcoded подмножество, което е устойчиво на преномериране.

Перформанс: 2.0/2.0 ✅

Всеки индекс съвпада ТОЧНО с емитирания ORDER BY израз (включително COALESCE формите и посоката на id/bidder_id/authority_id tiebreak), така че SQLite нито сортира, нито буферира. Адитивна и идемпотентна миграция; коментарът обяснява защо оцелява ETL refresh на rollup таблиците. Тройният .replace при рендиране е пренебрежим.

Архитектура / Код качество: 2.0/2.0 ✅

Помощникът е изнесен в отделен модул, преизползван в root.tsx, съответства на съществуващия safeJson шаблон. Няма частична имплементация, дублиране, dead code или смесени отговорности.

Документация: 2.0/2.0 ✅

И двата файла имат подробни, точни коментари, обясняващи защо, а не само какво. Няма breaking changes — миграцията е адитивна.

Малки бележки (незадължителни, виж inline)

  • Именуване на jsonLdScript: функцията връща сериализиран JSON низ, а не <script> елемент.
  • Проверете дали CI (gh pr checks) е зелен преди merge — не можах да изпълня тестовете в тази среда.

Никакви блокиращи проблеми. Препоръка: APPROVE.

Comment thread apps/web/app/lib/json-ld.ts Outdated
Comment thread apps/web/app/lib/json-ld.ts
Comment thread packages/db/migrations/0005_list_sort_indexes.sql
…t sort-index sync

Address the (non-blocking) review nits:

- Rename jsonLdScript to serializeJsonForScript: the helper returns a serialized
  JSON string safe to embed in an inline <script>, not a <script> element (review
  ydimitrof). Updates root.tsx and the test.

- Document the sentinel sync: the COALESCE defaults in queries/contracts.ts SORTS
  ('' / '9999-99') must stay byte-identical to the expression indexes, or SQLite
  silently drops the index and falls back to a full scan + temp-B-tree sort. Added
  reciprocal SYNC comments in the migration and the SORTS map, both noting that
  list-sort-indexes.test.ts's EXPLAIN assertions catch a drift.
@B353N

B353N commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Благодаря за одобрението! Адресирах двете незадължителни бележки в 8fbf478:

Именуване: jsonLdScriptserializeJsonForScript (връща сериализиран JSON низ за вграждане в inline <script>, не <script> елемент). Обextssingle root.tsx и теста.

Синхрон на sentinel-ите: добавих реципрочни SYNC коментари в двете места - в migrations/0005_list_sort_indexes.sql и в queries/contracts.ts SORTS - че COALESCE default-ите ('' / '9999-99') трябва да останат byte-идентични с индексните изрази, иначе SQLite тихо спира да ползва индекса и пада на full scan + temp-B-tree. И двата коментара сочат, че list-sort-indexes.test.ts (EXPLAIN проверките) хваща евентуален drift.

(Бележка #2 за екранирането само на < беше „само потвърждение - няма нужда от промяна", затова не пипам нищо там.)

По CI: не мога да го стартирам от тази среда - gh pr checks показва, че workflow-ът чака ръчно одобрение (action_required), защото PR-ът е от външен форк. Локално pnpm typecheck / test (web 340, db 195) / lint минават. Ако маintainer одобри run-а, би трябвало да е зелен без промени.

@ydimitrof ydimitrof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Преглед на PR: индекси за листовите сортове + екраниране на JSON-LD

Обща оценка: 9.2/10 — качествена, добре тествана и документирана промяна.

Phase 0 — Security-Critical Scan: ✅ ЧИСТО

  • Няма hardcoded тайни (API ключове, пароли, токени).
  • Единственият нов URL е https://schema.org (@context на JSON-LD) — стандартен и безопасен.
  • Няма злонамерени шаблони (backdoor, code injection, обфускация).
  • Няма нови зависимости.
  • Промяната всъщност затваря XSS дупка (raw </script> в JSON-LD), а не отваря такава.

Силни страни

  • serializeJsonForScript е коректен sink: екранирането на << е JSON-еквивалентно (round-trip доказан в теста) и покрива и </script>, и <!--, и <script. Допълнителното екраниране на U+2028/U+2029 е правилен defense-in-depth за JS eval. Граничният случай undefined/функция/символ → 'null' е обмислен и покрит с тест.
  • Миграцията 0005 е адитивна и идемпотентна (CREATE INDEX IF NOT EXISTS), а expression индексите съвпадат точно с ORDER BY изразите на query слоя. list-sort-indexes.test.ts доказва дефекта (temp-B-tree сорт) ПРЕДИ и премахването му СЛЕД миграцията — на първата страница И на keyset страница. Това е смислен, не тривиален тест.
  • Документацията (коментари в SQL, json-ld.ts и contracts.ts) ясно описва SYNC couplings-ите между sentinel-ите и индексите.

Съответствие с CLAUDE.md

  • Няма частична имплементация, TODO, дублиран или мъртъв код. Тестовете са смислени (не „cheater"). Наименуването е консистентно. Разделянето на грижите е чисто (нов помощен модул вместо inline).

Забележки (незадължителни, не блокиращи)

Виж inline коментарите — две наблюдения относно (1) зависимостта на теста от външен sqlite3 бинар и разминаване с D1 планировчика, и (2) обхвата на гаранцията за индекса при филтрирани заявки.

Препоръка: COMMENT (одобрение след разглеждане на двете забележки — нито една не е дефект, изискващ промяна).

Comment thread packages/db/src/list-sort-indexes.test.ts
Comment thread packages/db/src/queries/contracts.ts
Document the two known limits of the EXPLAIN-plan proof, per review: (1) the local
sqlite3 CLI planner is not version-identical to Cloudflare D1's (a strong
indication, not a bit-exact production proof; the binary itself is a pre-existing
suite-wide dependency), and (2) the index-walk guarantee covers the UNFILTERED
sort paths - with an active filter the planner may prefer the filter's index and
temp-sort the much smaller filtered set, which is the correct trade. Comment-only.
@B353N

B353N commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Благодаря за прегледа! Проверих и двете наблюдения - прав си и за двете. Документирах ги в 0a59f1b (само коментари, без промяна в поведение):

1. sqlite3 бинар + версия на планировчика:

  • Зависимостта от външния sqlite3 CLI е съществуваща конвенция на suite-а, не нова от този PR - migrations.test.ts, refresh-slice.test.ts, ship-domain.test.ts, integrity-checks.test.ts и competition-sql.test.ts всички го exec-ват. При липсващ бинар целият db suite пада, не само този файл, затова умишлено не добавих it.skip guard само тук - той би замаскирал тихо покритие, докато останалите тестове и без това падат твърдо. Ако решите да направите зависимостта по-диагностична, най-смисленото място е общ helper/предварителна проверка за всичките шест файла (извън обхвата на този PR).
  • За версията на планировчика: напълно съгласен - EXPLAIN плановете от локалния sqlite3 са силна индикация, не bit-exact production доказателство за D1. Добавих го изрично в header коментара на теста, до вече споменатата липса на ANALYZE.

2. Обхват при филтрирани заявки: също вярно - гаранцията за index-walk покрива нефилтрираните сортове (дефолтните изгледи на списъците). При активен филтър планировчикът може да предпочете индекса на филтъра и да temp-сортира (много по-малкото) филтрирано множество - което е правилният trade. Отбелязах обхвата и в header-а на теста, и в SYNC коментара на SORTS в queries/contracts.ts, за да не се чете гаранцията по-широко, отколкото е доказана.

typecheck / test (12/12) / lint минават.

@ydimitrof ydimitrof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ревю на PR: fix — индекси за листовите сортове + екраниране на JSON-LD

Обща оценка: ~9.3/10 — Препоръка: APPROVE (одобрение)

PR-ът покрива две несвързани, но малки и добре обособени грижи: (1) hardening на JSON-LD data island срещу XSS и (2) индекси за keyset пагинацията на недефолтните сортове. И двете са придружени със смислени тестове.

Фаза 0 — Сигурностен скан: ЧИСТО ✅

  • Няма хардкоднати тайни (ключове/пароли/токени).
  • URL-ите са само https://schema.org и тестови стойности (https://sigma.midt.bg, https://x/...) — очаквани, не изискват whitelist.
  • Няма злонамерени шаблони (backdoor, eval, обфускация). Промяната всъщност затваря XSS дупка.
  • Няма нови зависимости.

Сигурност (агентско ниво): 1.0/1.0 ✅

serializeJsonForScript екранира <<, което спира ранното затваряне на <script> чрез </script> в стойност. Екранирането е JSON-еквивалентно (JSON.parse връща идентична стойност — покрито от тест). Допълнителните U+2028/U+2029 escape-ове предпазват при оценяване като JS. Много добра defense-in-depth практика, дори при това, че днешният единствен вход е origin от new URL().

Тестове: 3.0/3.0 ✅

  • json-ld.test.ts — покрива breakout-а, round-trip еквивалентността, separator-ите и ръба undefined/функция → null. Тестовете разкриват реални дефекти, не са тривиални.
  • list-sort-indexes.test.ts — доказва BEFORE (temp B-tree sort) и AFTER (index walk без sort стъпка) на реален sqlite3, за първа страница И за keyset страница. Ограниченията (D1 vs local planner, само нефилтрирани пътища) са честно документирани.

Производителност: 2.0/2.0 ✅

Всеки индекс съвпада точно с ORDER BY израза (вкл. COALESCE sentinel-ите и посоката на id tiebreak), така че keyset walk спира на LIMIT вместо да сканира цялата таблица (D1 таксува по SCANNED редове). Единствена дребна забележка (незадължителна) — виж inline коментар за write-amplification при рефреша на rollup таблиците.

Код и документация: 2.0/2.0 + 2.0/2.0 ✅

Стилът е консистентен, коментарите обясняват защо и явно посочват SYNC договорите между индекс израза и SORTS картата. Мигрцаията е additive + идемпотентна (IF NOT EXISTS), rollup таблиците се DELETE+INSERT рефрешват (не се drop-ват), така че индексите оцеляват ETL. Обратим е (индексите могат да се пуснат без загуба).

CLAUDE.md съответствие

Няма частична имплементация, TODO-та, дублиран или мъртъв код, смесени концерни или ресурсни течове. Промените са атомарни и фокусирани.

Дребни, незадължителни забележки

  • Коментарните следи „review ydimitrof“ в json-ld.ts са безобидни, но е добре да отпаднат преди merge (артефакт от вътрешно ревю).
  • Виж inline коментар относно index write-amplification.

Няма блокиращи проблеми. Единствената причина да не е чисто 10/10 е, че тестовете не са изпълнени в тази среда (потвърдете зеления CI преди merge) и дребните забележки по-горе.

Comment thread packages/db/migrations/0005_list_sort_indexes.sql
Comment thread apps/web/app/lib/json-ld.ts
Remove the '(review ydimitrof)' attribution artifacts from json-ld.ts and
list-sort-indexes.test.ts comments; the explanations stay. Comment-only.
@B353N

B353N commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Благодаря за одобрението! Адресирах и двете незадължителни бележки в b41b243:

1. Write-amplification - измерено, в приемливи граници. Симулирах пълния DELETE+INSERT refresh на реален sqlite3 при производствен обем, със и без новите индекси:

Стъпка без (main) с новите индекси делта
company_totals 60k реда (+2 индекса) 118 ms 185 ms +67 ms
authority_totals 3k реда (+2 индекса) 5 ms 6 ms +1 ms
contracts 200k реда, пълен reload (+2 израз-индекса) 1.53 s 2.03 s +0.50 s

Контекст, който смекчава и това: пълният 200k reload на contracts се случва само в CLI import-а (normalize-raw, и без това минутен процес); 6-часовият cron (refresh-slice) заменя само договорите от прозореца и scoped rollup-и, така че там амплификацията е върху порядъци по-малко редове. +0.5s на пълен import срещу премахнат full-scan на всяка листова заявка е добър trade.

2. Review-маркерите - махнати от коментарите в json-ld.ts и list-sort-indexes.test.ts (обясненията остават, отпада само атрибуцията). Проверих с grep, че по клона няма други.

По CI: workflow-ът чака ръчно "Approve and run" (PR от външен форк - action_required); локално typecheck / test / lint са зелени на този commit.

@ydimitrof ydimitrof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ревю: индекси за листови сортове + екраниране на JSON-LD

Оценка: силен PR. Препоръка: COMMENT — няма блокиращи проблеми; една незадължителна забележка за дублиране (DRY) и няколко бележки за деплой/производителност.

Фаза 0 — сигурност: ЧИСТО ✅

  • Няма твърдо кодирани тайни, ключове или пароли.
  • Единствените URL адреси са https://schema.org (константа в JSON-LD) и тестова стойност https://sigma.midt.bg — без промени в whitelist.
  • Няма зловредни шаблони/backdoor/обфускация. Обратното — serializeJsonForScript затваря реален stored-XSS вектор (</script> breakout) и екранира U+2028/U+2029.
  • Няма нови зависимости.

Силни страни

  • Екранирането е коректно и JSON-еквивалентно; тестовете покриват breakout, round-trip, U+2028/2029 и ръба undefined → "null".
  • Индексите съвпадат точно с ORDER BY изразите (COALESCE + id tiebreak в същата посока), а list-sort-indexes.test.ts доказва чрез EXPLAIN QUERY PLAN дефекта ПРЕДИ (temp B-tree sort) и поправката СЛЕД — на първа и на keyset страница. Тест, който разкрива, а не просто минава.
  • Миграцията е адитивна и идемпотентна (CREATE INDEX IF NOT EXISTS), с ясни SYNC коментари.

Забележки (незадължителни)

  1. DRY: коментарът сам признава, че serializeJsonForScript „огледва" safeJson в routes/contract.json.tsx — две независими реализации на едно и също екраниране за <script>. Обмислете споделен помощник.
  2. Rollback: миграцията няма явен DROP INDEX за отмяна; при нужда от reversible миграции добавете такъв.
  3. Write amplification: двата нови израз-индекса върху contracts добавят разход при ETL запис — приемливо, но заслужава да се знае.
  4. Граници на теста: локалният sqlite3 planner не е идентичен с D1; тестът сам го документира — силна индикация, не битово доказателство.

Заключение

Промяната е фокусирана, добре тествана и подобрява сигурността и производителността. Одобрима след (по избор) консолидиране на дублираното екраниране.

Comment thread apps/web/app/lib/json-ld.ts Outdated
Comment thread apps/web/app/lib/json-ld.ts
Comment thread packages/db/migrations/0005_list_sort_indexes.sql
Comment thread packages/db/src/list-sort-indexes.test.ts
…LD island and .json route

The .json contract endpoint had its own safeJson escaper, a second implementation
of the same <script>/separator escaping as serializeJsonForScript - a DRY smell the
comment itself admitted, and a drift risk (one could add a U+2028 escape the other
lacks). Route it through the shared serializer instead. It escapes every `<` (vs the
old `</`-only form) - JSON-equivalent, harmless for the JSON body, strictly safer.

Also document, in the shared helper, why `>` and `&` are deliberately left unescaped
(only `<` can start a token in a script raw-text context), with a test that locks it.
@B353N

B353N commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Благодаря за прегледа! Основната бележка (DRY) е адресирана в 11ae02a; останалите са потвърждения.

1. DRY - консолидирано. routes/contract.json.tsx вече ползва споделения serializeJsonForScript вместо собствен safeJson. Двете реализации бяха на едно и също екраниране; сега има един източник, така че не могат да се разминат. Споделеният helper е строг superset на стария safeJson (екранира всеки <, не само </; същите U+2028/U+2029; + undefined guard), JSON-еквивалентен е (JSON.parse връща идентична стойност), а промяната в изхода на .json (</\u003c/) е валиден JSON без тест или документиран byte-contract върху него. Целият web пакет е зелен (341 теста).

2. > / & не се екранират - вече изрично документирано. Добавих коментар в helper-а: в raw-text контекста на <script> само < може да започне markup/comment токен (</script, <!--, <script); това не е HTML-attribute контекст, затова >/& не се нуждаят от екраниране и остават byte-minimal. Заковано с тест.

3. Write-amplification + rollback:

  • Амплификацията я измерих в предишния отговор: +67 ms на company_totals (60k), +1 ms на authority_totals, +0.5 s на пълния 200k reload на contracts - а пълният reload е само в CLI import-а; 6-часовият cron пипа само прозореца. Приемливо срещу премахнатия full-scan на всяка листова заявка.
  • DROP INDEX за rollback: проектът е forward-only additivemigrations/ няма нито един down/rollback скрипт; wrangler d1 migrations не поддържа down-миграции, а конвенцията е IF NOT EXISTS). Индексите са тривиално обратими ръчно (DROP INDEX), но добавяне на down-скрипт тук би било непоследователно спрямо цялата миграционна история.

4. sqlite3 в CI: бинарът е съществуваща suite-wide зависимост - migrations, refresh-slice, ship-domain, integrity-checks, competition-sql тестовете вече го exec-ват; CI е ubuntu-latest, който го носи по подразбиране. Този PR не добавя нова инфраструктурна зависимост.

typecheck / test (web 341, json-ld 6/6) / lint минават.

@ydimitrof ydimitrof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Преглед на PR: индекси за листовите сортове + екраниране на JSON-LD

Обща оценка: 9.2/10 — силен, добре тестван PR. Препоръка: COMMENT (одобрим след дребните забележки по-долу).

Сигурност (Phase 0 + агентен преглед) ✅

  • Няма зашити тайни, нови външни URL адреси или зловредни шаблони; няма нови зависимости.
  • serializeJsonForScript е коректна имплементация: екранира << (затваря дупката за </script> breakout / stored XSS), плюс U+2028/U+2029. Решението да НЕ се екранира >/& е технически правилно за raw-text <script> контекст и е добре документирано.
  • Обработката на ръба (JSON.stringify връща undefined за undefined/функция/символ → връща 'null') предотвратява .replace върху не-стринг. Отлично.
  • Консолидацията на двете копия (root.tsx и contract.json.tsx) в един споделен sink премахва риска от разминаване — това е правилният ход.

Тестове ✅ (много добро покритие)

  • json-ld.test.ts покрива breakout, JSON-еквивалентност, U+2028/U+2029, байт-идентичност за чист вход и ръбовия случай undefined — тестовете разкриват дефекти, не са тривиални.
  • list-sort-indexes.test.ts доказва дефекта ПРЕДИ (temp B-tree sort) и поправката СЛЕД (index walk) за всичките 6 сорта, върху първа И keyset страница. Прилага реалните миграции динамично, без hardcode. Много добра инженерна дисциплина.

Производителност ✅

  • Индексите съвпадат точно с ORDER BY COALESCE(...) изразите + keyset id tiebreak в същата посока → SQLite нито сортира, нито буферира. Additive + идемпотентни (IF NOT EXISTS), преживяват DELETE+INSERT ETL. Реален изигриш при D1 (billing по scanned rows).

Забележки (не блокиращи)

  1. contract.json.tsx — коментарът твърди, че екранирането на < „defends the body if it is ever sniffed as HTML“. Екранирането помага, но истинската защита срещу MIME sniffing е X-Content-Type-Options: nosniff хедър. Виж inline коментара.
  2. Свързаност миграция ↔ query слой — гаранцията за index-walk зависи от байт-идентичност между sentinel-ите в миграцията и SORTS в queries/contracts.ts. Митигирано с EXPLAIN тест, но тестът разчита на локален sqlite3 planner, който (по признание в самия коментар) не е bit-exact спрямо D1. Виж inline коментара.

CLAUDE.md съответствие ✅

Без частична имплементация, без TODO, без дублиране (напротив — премахва дублиране), без мъртъв код, коментарите обясняват „защо“. Промените са атомарни и фокусирани върху двата обявени проблема.

Няма блокиращи проблеми. Препоръчвам сливане след разглеждане на двете дребни забележки.

Comment thread apps/web/app/routes/contract.json.tsx
Comment thread packages/db/migrations/0005_list_sort_indexes.sql
…tinel-sync test

- contract.json.tsx: the actual MIME-sniffing defense is X-Content-Type-Options:
  nosniff, not the content escaping. The worker already sets it globally
  (baseSecurityHeaders); set it explicitly on this resource route too so it is safe
  on its own, and correct the comment that over-credited the escaping (review).

- Add sort-index-sentinel-sync.test.ts: the date-sort index only matches while its
  COALESCE sentinel is byte-identical to SORTS in queries/contracts.ts. A .sql
  migration can't import a TS constant, so guard the coupling with a static
  cross-file check of the sentinels ('' and '9999-99') that fails on drift
  regardless of the DB engine - independent of the local sqlite3 planner the EXPLAIN
  test relies on (review).
@B353N

B353N commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Благодаря за прегледа! Адресирах и двете бележки в 1985da4:

1. nosniff вместо разчитане на екранирането. Прав си - реалната защита срещу MIME sniffing е хедърът. Проверих: worker-ът вече го слага глобално (baseSecurityHeadersX-Content-Type-Options: nosniff на всеки отговор), но по препоръката го добавих изрично и на .json route-а, за да е самодостатъчен. Поправих и коментара да не приписва sniffing защита на екранирането - екранирането е content-level defense-in-depth, nosniff е истинската защита.

2. Sentinel drift guard, независим от planner-а. Добавих sort-index-sentinel-sync.test.ts - статична cross-file проверка, че COALESCE sentinel-ите ('' за desc, '9999-99' за asc) са байт-идентични между migrations/0005 и SORTS в queries/contracts.ts. Понеже .sql миграция не може да импортира TS константа (миграциите са ръчни .sql, няма generator), споделена константа не е възможна; guard-ът сравнява директно двата сорс файла и пада при разминаване независимо от DB engine-а - не зависи от локалния sqlite3 planner, който EXPLAIN тестът ползва. Не минава вакуумно (асертира точно ['', '9999-99']).

typecheck / test (db 196, web 341) / lint минават.

@ydimitrof ydimitrof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Преглед на PR: индекси за листовите сортове + екраниране на JSON-LD

Обща оценка: 9.3/10 — силен, добре тестван PR. Препоръка: APPROVE (с малки бележки).

Фаза 0 — Сигурност (сканиране): CLEAN ✅

  • Няма hardcoded тайни, API ключове, пароли или токени.
  • URL-и: само https://schema.org (стандартен JSON-LD контекст) и https://sigma.midt.bg в тестова фикстура — без нови външни адреси.
  • Няма нови зависимости, няма обфускация, backdoor или code injection.
  • Промяната всъщност затваря XSS дупка (некоректно екраниране на < в inline <script>), което е подобрение в сигурността.

Силни страни

  • Правилен security fix. serializeJsonForScript екранира <<, което е JSON-еквивалентно (JSON.parse връща идентична стойност) и надеждно предотвратява ранно затваряне на </script>. Аргументът защо > и & НЕ се екранират в script raw-text контекст е коректен спрямо HTML спецификацията. U+2028/U+2029 екранирането е добра defense-in-depth за случай на eval.
  • Премахнато дублиране. Двете локални копия (safeJson в contract.json.tsx и inline JSON.stringify в root.tsx) са консолидирани в един споделен sink — точно спазване на „NO CODE DUPLICATION“.
  • Отлично тестово покритие. Юнит тестовете покриват breakout, JSON round-trip, U+2028/U+2029, byte-identичност и edge case-а undefined/функция → 'null' (иначе .replace би хвърлил). Това са смислени тестове, не „cheater“ тестове.
  • Индексите съвпадат точно с ORDER BY изразите (включително COALESCE sentinel-ите и посоката на id tiebreak-а), а EXPLAIN QUERY PLAN тестът доказва „преди/след“ (temp-B-tree scan → index walk) и на първа, и на keyset страница.
  • Защита срещу дрифт. sort-index-sentinel-sync.test.ts прави planner-независима статична проверка, че sentinel-ите ('', 9999-99) са байт-идентични между миграцията и query слоя — умна допълнителна мрежа върху EXPLAIN теста, чийто planner не е бит-идентичен на D1.
  • Добавеният X-Content-Type-Options: nosniff прави resource route-а безопасен и самостоятелно, а не само чрез глобалния слой.

Забележки (незадължителни, не блокират)

  1. Write amplification при ETL. Четирите индекса върху rollup таблиците се пресъздават при всяко DELETE+INSER опресняване, а двата expression индекса върху contracts оскъпяват вмъкванията. Компромисът е оправдан за read-latency и е документиран, но си струва да се следи времето на ETL ship.
  2. Именуване/обхват. serializeJsonForScript носи „Script“ в името, но в contract.json.tsx се ползва за отговор с application/json (не вграден в <script>). Екранирането е безвредно (defense-in-depth) и авторът го е отбелязал в коментара; чисто семантична бележка.
  3. Граница на гаранцията (вече документирана в тестовете): локалният sqlite3 planner не е идентичен на D1, и index-walk твърдението важи за нефилтрираните пътища. Приемливо, ясно описано.

CLAUDE.md съответствие

NO PARTIAL IMPL ✅ · NO TODO/SIMPLIFICATION ✅ · NO DUPLICATION ✅ (консолидирано) · NO DEAD CODE ✅ · COMPREHENSIVE TESTS ✅ · NO CHEATER TESTS ✅ · CONSISTENT NAMING ✅ · NO OVER-ENGINEERING ✅ · NO MIXED CONCERNS ✅ · NO RESOURCE LEAKS ✅ (tmp директорията се чисти в afterAll).

Готовност за deploy

Миграцията е адитивна и идемпотентна (CREATE INDEX IF NOT EXISTS), обратимостта е тривиална чрез DROP INDEX. Промените са атомични и фокусирани (перформанс + hardening), без scope creep.

Единствено уточнение: не можах да изпълня тестовия пакет в тази среда — верификацията е базирана на статичен анализ на diff-а. Тестовете изглеждат коректни и смислени; препоръчвам да се потвърди зелен CI преди merge.

Comment thread packages/db/migrations/0005_list_sort_indexes.sql
Comment thread apps/web/app/routes/contract.json.tsx
Comment thread apps/web/app/lib/json-ld.ts
Remove the bare '(review ...)' attribution notes I left in contract.json.tsx and
sort-index-sentinel-sync.test.ts; the explanations stay. The pre-existing
'(review midt-bg#80)' issue references elsewhere are an established convention and are
untouched. Comment-only.
@B353N

B353N commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Благодаря за одобрението! Това ревю е APPROVE и нито една от трите inline бележки не изисква промяна по код - потвърждавам ги накратко:

  1. Write amplification - съгласен, това е мониторинг след деплой, не код. Вече го измерих в предишен коментар (+0.5s на пълния 200k reload на contracts, който тече само в CLI import-а; 6-часовият cron пипа само прозореца). Ще следим времето на ETL ship след merge.
  2. Именуване serializeJsonForScript - както сам отбелязваш, „за момента е ОК": екранирането е безвредно, nosniff е реалната защита, а изходът е валиден JSON. Функцията беше именувана точно така по предходно ревю, затова не я преименувам. Ако някога потрябва чист JSON.stringify за API консуматори, ще е отделен път.
  3. undefined guard - благодаря, радвам се, че е покрито. 👍

Направих само една дребна хигиенна поправка (727038e): махнах двата останали bare (review ...) маркера, които сам бях отбелязал, че се промъкнаха - в contract.json.tsx и sort-index-sentinel-sync.test.ts. Съществуващите (review #80) референции другаде са установена конвенция (номер на issue) и не съм ги пипал.

typecheck / test / lint минават. По CI: workflow-ът чака ръчно "Approve and run" (PR от външен форк).

@ydimitrof ydimitrof left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ревю на PR: индекси за листовите сортове + екраниране на JSON-LD

Обща оценка: 9.3/10 — качествен, добре тестван и добре документиран PR.

Резюме на промените

  • Въвежда се един споделен сериализатор serializeJsonForScript (apps/web/app/lib/json-ld.ts), който екранира << (плюс U+2028/U+2029). Това затваря реален XSS вектор: JSON.stringify не екранира <, така че </script> в стойност би затворил <script> елемента предсрочно.
  • Дублираните локални имплементации (safeJson в contract.json.tsx и голият JSON.stringify в root.tsx) са заменени с общата функция → премахнато дублиране, двата sink-а вече не могат да се разминат.
  • Добавени изразови индекси (migrations/0005) за шестте недефолтни листови сорта, точно съвпадащи с ORDER BY изразите на query слоя, така че keyset страницирането обхожда индекс вместо да сканира + temp-B-tree сортира цялата таблица (D1 таксува сканирани редове).

Силни страни

  • Отлично тестово покритие. json-ld.test.ts покрива breakout, JSON-еквивалентност, U+2028/U+2029, byte-минималност и ръбовия случай undefined → "null". list-sort-indexes.test.ts доказва плана през реален EXPLAIN QUERY PLAN за първа И keyset страница, преди/след миграцията. sort-index-sentinel-sync.test.ts добавя planner-независима статична проверка на sentinel-ите — тестовете търсят дефекти, не минават тривиално.
  • Няма cheater тестове, спазено разделение на отговорностите, консистентно наименуване.
  • Сигурност (Phase 0 + agent-level): CLEAN. Няма тайни, няма нови зависимости, няма подозрителни URL-и. X-Content-Type-Options: nosniff е добавен експлицитно на JSON ресурс маршрута.
  • Коментарите/документацията в кода са изчерпателни и обясняват SYNC зависимостите между миграцията и query слоя.

Второстепенни забележки (не блокиращи)

  1. Миграцията е адитивна и идемпотентна, но няма явен rollback (DROP INDEX). Приемливо за индекси, но добре е да се документира rollback планът.
  2. Името serializeJsonForScript подсказва "за <script>", а се ползва и за чист application/json HTTP отговор (contract.json.tsx). Екранирането там е JSON-еквивалентно и безвредно (defense-in-depth), но наименуването леко се разминава с употребата — документирано в коментара, затова само nit.
  3. Индексите върху rollup таблиците (company_totals, authority_totals) добавят поддръжка на индекс при всеки DELETE+INSERT ETL refresh — приемлив компромис, документиран.

Спазване на quality gates

  • Тестове: 3.0/3.0 · Код: 2.0/2.0 · Документация: 2.0/2.0 · Производителност: 2.0/2.0 (подобрение) · Сигурност: 1.0/1.0
  • Няма частична имплементация, няма TODO/dead code, няма scope creep — промените са атомични и фокусирани.

Препоръка: одобрение с второстепенни (незадължителни) забележки.

Comment thread apps/web/app/lib/json-ld.ts
Comment thread apps/web/app/lib/json-ld.ts
Comment thread packages/db/migrations/0005_list_sort_indexes.sql
Comment thread packages/db/src/list-sort-indexes.test.ts

@lyubomir-bozhinov lyubomir-bozhinov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Издържано. JSON-LD escaping-ът (json-ld.ts:23 < → \u003c, плюс \u2028/\u2029) затваря breakout-а от <script type="application/ld+json">: композитната <!-- </script> атака иска литерален <, който вече е escape-нат, значи tokenizer-ът не влиза в script-data-escaped състояние. Индексите за листовите сортове съвпадат текстово с изразите в SORTS (COALESCE(signed_at, '') / '9999-99'), а list-sort-indexes.test.ts + sentinel-sync тестът ги заключват през реален EXPLAIN. Няма забележки.

@lyubomir-bozhinov

Copy link
Copy Markdown
Collaborator

Координационна бележка по номерацията на миграциите (не е за този PR — #212 е чист и вече одобрен). През отворените PR-и номерата се разминават спрямо main:

PR Миграция
main (HEAD) 0000_init, 0001_flow_pairs_bidder_index
#226 0002_related_persons_foundation
#188 0003_contract_health (+ погрешно дописва 0000_init)
(gap на 0004)
#212 0005_list_sort_indexes
#209 0006_recent_feed_indexes

D1 прилага миграциите по име, във възходящ ред, точно веднъж. При дупка (0004) редът на прилагане зависи от реда на merge — който merge-не пръв над 0003 заема номера. Preview/CI не го хваща, защото work DB се строи наново всеки път (виж и #188).

Предложение: пре-номерирай координирано по договорения merge ред, плътно и без дупки (0002, 0003, 0004, 0005…), финализирано преди първия merge от групата. Аз ще подравня #226 (0002) спрямо каквото се разберем.

@lyubomir-bozhinov lyubomir-bozhinov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Одобрявам делтата 088476d…727038e на връх 727038e. Разгледах наново разширения обхват (споделеният JSON-for-script сериализатор + .json route-а) — сигурен е:

  • serializeJsonForScript (json-ld.ts:23) escape-ва всяко <\u003c плюс U+2028/U+2029 — по-силно от стария <\/-only вариант (който пропускаше <!--/<script). </script><img src=x onerror=…> не пробива script рафта (проверих емпирично: изходът няма нито </script, нито <img, нито суров <; JSON round-trip се пази).
  • .json route-ът е Content-Type: application/json; charset=utf-8 + X-Content-Type-Options: nosniff, а hardenResponse/baseSecurityHeaders в workers/app.ts налага nosniff и глобално на всеки response — не може да се sniff-не като HTML.
  • Няма privacy регресия: делтата само смени safeJson→споделения сериализатор и добави nosniff; селекцията на полета е непроменена (.json и HTML route-ът минават през общия getContract).
  • Sort-index частта: renumber 0002→0005 е чисто преименуване; EXPLAIN тестът прилага всички миграции динамично (преживява преномерация) и покрива first-page + keyset; sort-index-sentinel-sync.test.ts е издържан planner-независим guard срещу sentinel-drift.

Единственото за внимание си остава координираната преномерация на миграциите (виж бележката ми по-горе) преди merge — да не се сблъскат номерата с #226/#188/#209.

@B353N

B353N commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

Здравейте! Може ли малко насока за следващата стъпка по този PR?

Доколкото виждам:

  • PR-ът е mergeable (без конфликти с main);
  • одобрен е от @ydimitrof и @lyubomir-bozhinov, всички inline бележки от ревютата са адресирани;
  • pnpm typecheck / pnpm test / pnpm lint минават локално.

Единственото, което го държи блокиран, изглежда е CI: workflow-ът Check стои в състояние action_required, защото PR-ът е от външен форк и Actions за такъв PR чакат член с write достъп да натисне „Approve and run workflows“. Аз (само с read достъп) не мога да го пусна сам.

Затова питам:

  1. Може ли някой от вас да одобри и пусне run-а на Check?
  2. Трябва ли да направя още нещо от моя страна (rebase, промяна, преномериране на миграцията и т.н.) преди merge?

Благодаря!

@lyubomir-bozhinov

lyubomir-bozhinov commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Прегледах #212 на head cc57e04.

JSON-LD escaping-ът е коректен и добре обоснован. serializeJsonForScript заменя всеки < с неговия JSON unicode escape (затова суров </script> не може да затвори script елемента) + U+2028/U+2029, и умишлено НЕ пипа >/& — правилно за script raw-text контекст (само < отваря markup токен там), байт-минимално. Едж-кейсът JSON.stringify → undefined'null' е покрит. Тестовете са реално адверсариални (</script><script>, главни </SCRIPT >, unicode сепаратори, JSON round-trip). Силно.

Две неща:

  1. Пълнота на sink-овете (потвърди): ефектът важи само ако ВСЕКИ inline JSON-LD sink минава през хелпъра. Виждам root.tsx + contract.json.tsx. Има ли друг route/компонент, който сериализира JSON-LD с dangerouslySetInnerHTML извън този път?
  2. Ред на миграциите: 0005 приема ред 0003(feat: индекс на качеството на договорите (ETL оценка 0..1 + страница) #188)→0004(feat(web): „Подобни договори" - ценови ориентир по CPV кохорта на страницата на договора #210)→0005. feat: индекс на качеството на договорите (ETL оценка 0..1 + страница) #188 е CONFLICTING/забуксувал. Потвърди, че apply логиката толерира пролука на 0003 (или пристигане не по ред) — трите са схемно независими, вероятно безопасно, но си струва да се провери; ако feat: индекс на качеството на договорите (ETL оценка 0..1 + страница) #188 не влиза скоро, обмисли преномериране.

Sign-off по сигурността.

Two review follow-ups on the ordering-index test:

- Filtered sorts were documented as out of scope, leaving the reader unable to
  tell whether an active list filter makes the ordering index redundant. It does
  not: with a sector (tenders.cpv_code) or eu-funded filter the planner still
  walks idx_contracts_signed_desc and drops the sort step, while the pre-index
  baseline sorts the whole table. Asserted both directions.
- A missing sqlite3 CLI surfaced as an opaque ENOENT. Probe it in beforeAll and
  fail with the fix. Deliberately not a skip: this is a perf/cost gate, and
  silently passing it on an image without sqlite3 would retire the gate.

@midt-admin midt-admin left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Одобрявам. Двете находки са реални и добре доказани: шест избираеми сорта сканираха и сортираха цялата таблица при всяко разлистване (а D1 таксува прочетени редове), а JSON-LD sink-ът стоеше без екраниране на <.

Тестът е силната част - доказва дефекта ПРЕДИ и поправката СЛЕД върху реален sqlite, вместо да твърди. С последните два комита покрива и филтрираните сортове: при филтър по сектор или по еврофинансиране индексът пак се обхожда и сортиращата стъпка пак изчезва, тоест ползата не е ограничена до подразбиращите се изгледи. Липсващият sqlite3 вече се хваща с указание, вместо неясен ENOENT.

Оценявам и отхвърлените промени - id-tiebreak индексите и ANALYZE, премерени и махнати, защото не дават полза. Миграцията 0005 е приложена върху sigma-stage-green предварително, така че деплоят пада върху готова схема.

@todorkolev
todorkolev merged commit ab24a38 into midt-bg:main Jul 29, 2026
2 checks passed
lyubomir-bozhinov added a commit to lyubomir-bozhinov/sigma that referenced this pull request Jul 29, 2026
…milar-contracts midt-bg#210, leaf-index+JSON-LD midt-bg#212, semgrep midt-bg#255); keep sharp pin + detailed osv notes
lyubomir-bozhinov added a commit to lyubomir-bozhinov/sigma that referenced this pull request Jul 29, 2026
…af-index+JSON-LD midt-bg#212, semgrep midt-bg#255, osv midt-bg#271); keep sharp pin + all security overrides
lyubomir-bozhinov added a commit to lyubomir-bozhinov/sigma that referenced this pull request Jul 29, 2026
lyubomir-bozhinov added a commit to lyubomir-bozhinov/sigma that referenced this pull request Jul 29, 2026
Pulls the euro-annex conversion fix (midt-bg#245/midt-bg#261), canonical value base (midt-bg#259),
identity canonicalization + Bulstat checksum + joint procurement (midt-bg#251-253),
app-layer read-only D1 guard (midt-bg#225), JSON-LD escaping (midt-bg#212), react-router 7.18.0.

Non-trivial resolutions:
- normalize-raw.sql: kept upstream's amendment_winner currency CTE + our
  is_synthetic column (both additive, one column-list collision).
- integrity-checks.mjs: upstream's (await rows()) wrapper carrying our
  is_synthetic != 1 filter on auth/bidder attribution.
- Migration collision: our 0002_contracts_is_synthetic renumbered to 0006
  (upstream took 0002 for current_value_currency); tests load all migrations.
- refresh-slice.test seedReattrContract: upstream's authority params + our
  real-tender-header seed so reattr contracts stay non-synthetic and reconcile.
- root.tsx/assistant.chat.tsx: adopted getDb read-only chokepoint + kept dock.
- describe-schema DATA_TRAPS: upstream canonical-base rule + our NULL detail.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants