feat(web): обща маркирана стойност на договорите със сигнали за риск (#218) - #236
feat(web): обща маркирана стойност на договорите със сигнали за риск (#218)#236DiyanaDimitrova wants to merge 7 commits into
Conversation
ydimitrof
left a comment
There was a problem hiding this comment.
Ревю на PR #218 — „обща маркирана стойност на договорите със сигнали за риск"
ВЕРДИКТ: APPROVE (условно) — няма блокиращи проблеми по сигурност или цялост на данните; остават 3 дребни точки за проверка.
(Това е чернова за преглед — по Ваша инструкция не публикувам решение. Английска версия мога да предоставя при поискване.)
Фаза 0 — Сканиране за критична сигурност: ЧИСТО ✅
- Твърдо кодирани тайни: няма (нито ключове, пароли, токени).
- URL промени: няма нови външни URL-и — само вътрешни връзки (
/contracts,/methodology#flagged). Няма нужда от whitelist. - Зловреден код / backdoor / обфускация / инжекции: няма открити.
- Зависимости: няма нови пакети.
node:sqliteиnode:fsсе използват само в тестовия файл (flagged.test.ts), не в продукционен код.
SQL инжекции / OWASP A03 — ЧИСТО ✅
Прегледах внимателно целия слой за заявки:
flagged.tsизгражда SQL само от статични литерали (FLAG_SQL,ANY_FLAG_SQL, ключове отFLAG_TYPES,CPV_SECTORS). Никаква част не идва от вход на потребителя. Няма конкатенация на потребителски данни.?type=(authorityTypes) минава през параметризирани placeholder-и (qs(...)+params.push(...)), а стойностите се валидират двойно срещу затворен allow-list (AUTHORITY_TYPE_GROUPS) — веднъж на ниво маршрут (filters.ts) и веднъж на ниво БД (flagPredicate).?flag=минава презflagPredicate, който връща предикат само за познати токени; непознат токен →null→ изхвърля се, а при нула валидни токени филтърът връща1=0(нищо), не „всичко" — правилно поведение, покрито с тест.
Цялост на данните / кеш — добре обмислено ✅
flagе добавен къмCACHE_QUERY_PARAMSи къмARRAY_FILTERSвcsv-export.ts— предотвратява отравяне на споделения „unfiltered" кеш (същият клас бъгове #56/#122/#138). Много добре.- Дедупликацията на общата сума спрямо застъпващите се
byTypeразбивки е коректно документирана и покрита с интеграционен тест срещу реален SQLite от миграциите. - Базата „само достоверна стойност" (
SUM(amount_eur)пропуска NULL приvalue_suspect, но броячът брои реда) е консистентна с #98 и проверена в теста (c4 → €0, но +1 брой).
GDPR / ЗЗЛД (OWASP A01 — контрол на достъпа/експозиция) — добре адресирано ✅
noindexпри?flag=вcontracts.tsxпази филтрираните по риск изгледи от индексиране (по аналогия с ЕТ профилите).topFlaggedизрично филтрира физически лица (isNaturalPersonProfileName) преди показване под „сигнали за риск".privacy.tsxиmethodology.tsxдобавят прозрачно описание на производните показатели с правно основание (чл. 6, пар. 1 (e)/(f); права по чл. 16 и чл. 21). Съответствието изглежда пълно.
XSS (OWASP A03) — ЧИСТО ✅
React екранира изхода; encodeURIComponent е приложен върху typeGroup в линковете; тестът потвърждава, че ?type=<script> се изхвърля от allow-list.
Съответствие с CLAUDE.md и качествени гейтове
- Тестове (3.0/3.0): налице са и unit, и интеграционни тестове срещу реална БД; покриват дедупликация, застъпване, NULL основа, OR-комбиниране, отхвърляне на непознати токени и композиране на филтри. Силно покритие.
- Код (2.0/2.0): предикатите са единствен източник на истина, преизползвани от агрегата и от
/contractsфилтъра — без дублиране. Именуването е консистентно. - Документация (2.0/2.0): методология + privacy обновени; api-contract с JSDoc.
- Производителност (виж бележките по-долу): заявките са пълни сканирания, но под 1ч edge-кеш и с валидиран
?type=срещу кардиналност — приемливо; вижте т.1 по-долу. - Сигурност (1.0/1.0): валидиране на вход, обработка на грешки и защита от инжекции — налице.
Дребни точки за проверка (незадължителни, непубликувани като искане за промени)
- Кеш ключ за
type— PR добавя самоflagкъмCACHE_QUERY_PARAMS. Убедете се, чеtypeвече присъства в този set (вероятно е, защото/authoritiesго ползва). Ако не е — два различни?type=заявки биха споделили кеш запис (същият клас #56/#122/#138). Оставих inline бележка. high_markupпри отрицателнаsigning_value_eur— предикатът проверява<> 0, но не и отрицателна база; при аномални данни съотношението може да е подвеждащо. Малко вероятно, но си струва проверка. Inline бележка.topFlaggedпод-запълване — при over-fetch 40 и филтриране на физически лица, ако >30 от топ 40 са ЕТ, таблицата ще покаже <10 реда. Приемливо, но добре е да се отбележи.
Оценка: 9.4/10. Много добре инженерно решен, защитен и тестван PR.
| 'count', | ||
| 'cursor', | ||
| 'eu', | ||
| 'flag', // /contracts: risk-signal filter (#218) — changes the result set + headline totals |
There was a problem hiding this comment.
Тук се добавя само flag към CACHE_QUERY_PARAMS. /contracts вече приема и ?type= (нов authorityTypes филтър), който също променя резултатния набор. Моля, потвърдете, че type вече присъства в този set (вероятно, тъй като /authorities го ползва). Ако липсва, два различни ?type= заявки биха споделили един кеш запис и биха сервирали грешни данни — същият клас cache-poisoning като #56/#122/#138, който PR-ът иначе внимателно избягва.
| // value anomaly isn't double-counted here (mirrors details.ts deltaPct, which is null when suspect). | ||
| high_markup: | ||
| `c.value_flag NOT IN ${SUSPECT_VALUE_FLAGS} AND c.signing_value_eur IS NOT NULL ` + | ||
| 'AND c.signing_value_eur <> 0 AND (c.current_value_eur - c.signing_value_eur) > 0.2 * c.signing_value_eur', |
There was a problem hiding this comment.
high_markup пази срещу signing_value_eur = 0 (<> 0), но не и срещу отрицателна база. При аномални ETL данни (отрицателна стойност при подписване) сравнението (current - signing) > 0.2 * signing може да даде подвеждащ резултат. Малко вероятно предвид източника, но обмислете добавяне на c.signing_value_eur > 0 за пълна коректност.
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Прегледах стриктно на връх e119556, с локална емпирична проверка на value basis-а (истинска схема от 0000_init.sql, adversarial seed през всичките 5 value_flag-а + NULL-ове). Value basis-ът е коректен: total-ът е SUM(CASE WHEN flagged THEN c.amount_eur END) — пропуска NULL-овете, брои value_suspect като €0 и съвпада с базата на home_totals. Single-source FLAG_SQL + интеграционни тестове върху реалната схема са точно правилният подход; cache-key / allow-list guard-овете за ?flag=/?type= са налице.
Едно should-fix (parity/accuracy):
packages/db/src/queries/flagged.ts:31—high_markupползваc.signing_value_eur <> 0, аapps/web/app/lib/riskLogic.ts:24ползваdeltaPct > 0.2(=(current − signing) / signing). При отрицателенsigning_value_eurдвете се разминават: SQL-ът fire-ва (началната страница го брои вhigh_markup), а contract страницата НЕ показва badge → видимо противоречие между двете повърхности. Поправка:c.signing_value_eur > 0— алгебрично е точноdeltaPct > 0.2за положителен signing, тъй че прави двете повърхности доказуемо еднакви. (Не потвърдих дали отрицателен signing реално се среща в корпуса, но fix-ът е безплатен и затваря разминаването; @ydimitrof го маркира inline като „consider" — потвърждавам, че е реално разминаване, не козметика.)
Дребни (не блокират):
home.ts— natural-person филтърът (isNaturalPersonProfileName) хваща само префикс „ЕТ "/„ET "; ЕТ без префикс минава, а при много ЕТ реда в топ-40 таблицата може да покаже <10 реда без отделен empty-state. GDPR-плитко — струва си по-дълбока проверка.home.tsx—FLAG_LABELSе втори source of truth; направете гоsatisfies Record<FlagType, string>, за да гарантира TypeScript пълнотата на ключовете.
Иначе — силна работа. Изисквам само > 0 fix-а на high_markup.
…ng, topFlagged cushion - flagged.ts: high_markup guards `signing_value_eur > 0` (was `<> 0`). For a negative signing base the SQL fired while riskLogic's badge (deltaPct > 0.2) did not, so the homepage counted a contract the contract page never badged. `> 0` makes the aggregate and the per-contract badge provably identical. Adds a regression row (negative signing) proving exclusion. - home.tsx: FLAG_LABELS now `satisfies Record<RiskFlagType, string>` so a renamed/added signal type fails the build instead of falling through. - home.ts: widen the topFlagged over-fetch (40 → 60) so the table still fills 10 rows after dropping sole-trader (ЕТ) bidders; 0 rows already degrades to the empty-state. Verified: `type` is already in CACHE_QUERY_PARAMS (no ?type= cache split). Note: bidderKind is only company|consortium — no data-level natural-person flag exists, so isNaturalPersonProfileName stays a conservative name prefix to avoid false-positively hiding real companies; robust detection needs an upstream flag (follow-up).
ydimitrof
left a comment
There was a problem hiding this comment.
PR #218 — обща маркирана стойност на договорите със сигнали за риск
Направен е строг преглед с акцент върху сигурност, SQL инжекции, целостта на данните, GDPR/ЗЗЛД и OWASP. Проверих локално две ключови допускания. Не намерих блокиращи проблеми.
🇧🇬 Чернова на български (проект)
Обхват и съответствие с #218. Промяната добавя на началната страница обобщена (де-дублирана) стойност на договорите с поне един структурен сигнал за риск, разбивки по вид сигнал / сектор / тип институция, таблица с най-скъпите маркирани договори, както и филтри ?flag= и ?type= на /contracts. Обновени са методологията и политиката за поверителност. Реализацията отговаря на описанието на тикета и е с минимални, фокусирани промени.
Сигурност (Фаза 0 — чисто).
- Няма твърдо кодирани тайни, нови URL адреси, зависимости или обфускиран/зловреден код.
- SQL инжекции — няма.
?flag=и?type=минават през позитивни allow-list проверки (KNOWN_FLAGS,KNOWN_TYPES) вfilters.ts;authorityTypesдопълнително се предава като параметризирани placeholder-и (IN (?, ?)). Предикатите за сигналите са статични низове;flagPredicateвръщаnullза непознат токен, а при липса на валиден предикат филтърът дава1=0(нищо), а не „всичко“. Дори при директно извикване на DB слоя (заобикаляйки уеб allow-list), инжекция не е възможна. - DoS / cache-cardinality guard. Ограничаването на
?type=до затворения наборAUTHORITY_TYPE_GROUPSе правилно и добре обосновано — предотвратява генериране на неограничени edge-cache ключове и некеширани пълни сканирания. - Cache-key коректност (проверено локално).
flagе добавен вCACHE_QUERY_PARAMS; потвърдих, чеtypeвече присъства там — значи няма cache poisoning от новия филтър.
Цялост на данните. Логиката на high_markup е приведена в съответствие с riskLogic чрез signing_value_eur > 0 (не само <> 0) — коректно за отрицателна базова стойност и покрито от тест (c8). Де-дублираният общ сбор, застъпващите се byType срезове и NULL amount_eur основата (value_suspect → брои се, но 0 €) са коректни и добре тествани с реална SQLite база върху продукционните миграции.
GDPR/ЗЗЛД. Много добре обмислено: noindex за изгледи ?flag=, изключване на физически лица (ЕТ) от таблицата на началната страница, обновени методология и privacy с правно основание. Обобщените суми са анонимни; поименният списък изключва физически лица — правилно разделение.
Забележки (незадължителни, не блокират):
- Производителност:
getFlaggedValueдобавя ~3 пълни агрегиращи сканирания + едноlistContractsсканиране към loader-а на началната страница при cache miss. Съответства на съществуващия single-offer модел и е под 1-часов edge cache, но при голяма таблица студеният кеш ще е по-тежък — струва си да се наблюдава. - Ръбов случай:
topFlaggedпрезарежда 60 реда и реже до 10 след изключване на физ. лица; при страница, доминирана от ЕТ, таблицата може да се напълни с < 10 реда. Документирано и деградира плавно. - Не можах да пусна тестовете локално (клонът не е наличен тук), затова покритието ≥90% и „всички тестове минават“ не са независимо потвърдени — моля потвърдете от CI.
🇬🇧 English draft
Scope & alignment with #218. Adds a de-duplicated total value of contracts carrying at least one structural risk signal on the homepage, with breakdowns by signal type / sector / authority type, a top-flagged table, and ?flag= / ?type= filters on /contracts. Methodology and privacy pages updated. The implementation matches the ticket and is minimal and focused.
Security (Phase 0 — clean).
- No hardcoded secrets, new URLs, dependencies, or obfuscated/malicious code.
- No SQL injection.
?flag=and?type=pass positive allow-list validation (KNOWN_FLAGS,KNOWN_TYPES) infilters.ts;authorityTypesis additionally bound via parameterized placeholders. Flag predicates are static strings;flagPredicatereturnsnullfor unknown tokens, and with no valid predicate the filter yields1=0(nothing), not "everything." Even bypassing the web allow-list and calling the DB layer directly, injection is not possible. - DoS / cache-cardinality guard. Bounding
?type=to the closedAUTHORITY_TYPE_GROUPSset is correct and well justified. - Cache-key correctness (verified locally).
flagwas added toCACHE_QUERY_PARAMS; I confirmedtypeis already present there — so no cache poisoning from the new filter.
Data integrity. high_markup is aligned with riskLogic via signing_value_eur > 0 (not just <> 0) — correct for a negative signing base and covered by a test (c8). The de-duplicated total, overlapping byType slices, and the NULL amount_eur basis (value_suspect → counted but €0) are correct and well tested against a real SQLite DB built from the production migrations.
GDPR. Thoughtful: noindex on ?flag= views, natural-person (sole-trader) exclusion from the homepage table, and updated methodology/privacy with a legal basis. Aggregate sums are anonymous; the named list excludes individuals — a correct separation.
Non-blocking notes:
- Performance:
getFlaggedValueadds ~3 full-scan aggregates + onelistContractsscan to the homepage loader on cache miss. Consistent with the existing single-offer pattern and under the 1h edge cache, but worth monitoring on a large table. - Edge case:
topFlaggedover-fetches 60 and slices to 10 after excluding individuals; a sole-trader-heavy page may under-fill. Documented; degrades gracefully. - I could not run the tests locally (branch not available here), so ≥90% coverage and "all tests pass" are not independently confirmed — please confirm from CI.
Вердикт / Verdict
COMMENT — няма блокиращи проблеми по сигурност или цялост на данните; препоръчвам одобрение след потвърждаване на CI (покритие + зелени тестове). / No blocking security or data-integrity issues; recommend approval once CI is confirmed green (coverage + passing tests).
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Прегледах делтата e119556→3b5cb57 — трите бележки от предния кръг са адресирани коректно. Блокерът падна; одобрявам.
high_markup> 0(блокерът):signing_value_eur <> 0→> 0. За положителна база(current − signing) > 0.2·signing ⟺ deltaPct > 0.2— агрегатът и badge-ът на договорната страница вече са доказуемо еднакви; при отрицателна база двете се разминаваха и> 0изключва точно редовете, които страницата никога не badge-ва. Коментарът описва алгебрата вярно.- Тестът доказва чувствителност: новият
c8(signing = -1000) минава стария<> 0guard (1000 > -200), но пада под> 0;bids = 3+value_flag = okго изолират само в high_markup среза. Fail-ва на стария код, минава на новия — реален discriminating тест, no cheater test. - Двете дребни:
FLAG_LABELS … satisfies Record<RiskFlagType, string>дава compile-time пълнота на ключовете (преименуван/нов тип чупи build-а тук вместо тихо да падне на raw key); natural-person under-fill смекчен (pageSize 40→60) с честно документиран empty-state fallback — приемливо, не е PII риск (drop-натите редове са козметична загуба, не изтичане).
Одобрявам.
A leading number on the homepage: the total EUR running through contracts that carry a risk signal, with a breakdown by signal type and by category (sector / authority type), each drillable to the contracts behind it, next to a non-accusatory methodology link. Computed LIVE over existing columns under the 1h edge cache — NO schema / precompute / migration change (ships as a pure Worker deploy per docs/deploy.md). Signal predicates mirror the per-contract RiskIndicators (riskLogic.ts) so the homepage number stays consistent with the badge on a contract page: no-competition (admitted bids = 1), cost growth (current > 1.2x signing), value/date anomaly. - packages/db/queries/flagged.ts: shared FLAG_SQL predicates (single source of truth) + getFlaggedValue (de-duplicated total, overlapping by-type, category breakdowns summing to the total; canonical amount_eur basis, midt-bg#98). - /contracts gains a risk-signal filter (flag) and an authority-type filter (type), query-layer only, so every homepage number drills down; registered in the CSV cache classifier + the edge cache-key allowlist. - Homepage section + methodology "Сигнали за риск" (de-dup vs overlap, tone). - Tests: real-SQLite aggregate + filter narrowing, parse + riskLogic parity. Closes midt-bg#218
…ents Strict review fixes: - HIGH: methodology listed "рискови сигнали" as not-yet-built while this PR ships them (section 10) — remove from the in-development list, point to section 10. - MEDIUM: the by-sector (top 6) and by-authority-type breakdowns exclude rows with a NULL cpv_code/type_group and cap sector to 6, so they do NOT necessarily sum to the total. Correct the flagged.ts docstring + the methodology copy, and add a NULL-dimension fixture row proving the slices sum to less than the total. - LOW: the total/by-type aggregate references only c.*, so give it FROM contracts c (no joins) — fewer rows read on the hottest scan. UI: render the flagged section with the same components as "Поръчки с една оферта" — a SingleOfferPortion bar (flagged EUR as a share of all contract value) and a SingleOfferTable of the top flagged contracts (via listContracts flag=all) — while keeping the by-type / by-sector / by-authority-type drill-down breakdowns. Refs midt-bg#218
Data protection (blocking): - Exclude natural-person (sole-trader ЕТ) bidders from the homepage „Договори със сигнали за риск" table: over-fetch and drop names via isNaturalPersonProfileName so an identifiable individual is never shown under a risk label on the indexed, edge-cached homepage (mirrors the existing noindex on sole-trader profiles). - Add robots noindex to /contracts when a ?flag= filter is active, so risk-filtered, name-bearing lists stay out of search indexes. Transparency / accuracy (blocking): - methodology.tsx §1 no longer claims the site „не маркира фирми като рискови" (contradicted §10); reworded to: marks contracts with structural signals, not entities as offenders. - privacy.tsx: add a „Производни показатели (сигнали за риск)" section disclosing the derived risk-signal processing, its lawful basis, and the rectification (Art. 16) / objection (Art. 21) rights (GDPR Art. 13/14). Security: - Validate ?type= on /contracts against the closed authority type_group set (new AUTHORITY_TYPE_GROUPS). /contracts is not rate-limited, so an unvalidated ?type= let each distinct value mint a fresh edge-cache key and an uncached full-table scan (cache-cardinality / DoS). Adds a regression test. Accessibility (WCAG 2.1 AA): - methodology §10 Callout uses the title prop (renders <h3>) instead of a raw <h2>, restoring the heading outline. - Parametrize SingleOfferTable's caption so the flagged table has a correct, distinct accessible name instead of the single-offer caption. - Add a „Смятате сигнал за грешен?" rectification link to the homepage flagged section.
…ng, topFlagged cushion - flagged.ts: high_markup guards `signing_value_eur > 0` (was `<> 0`). For a negative signing base the SQL fired while riskLogic's badge (deltaPct > 0.2) did not, so the homepage counted a contract the contract page never badged. `> 0` makes the aggregate and the per-contract badge provably identical. Adds a regression row (negative signing) proving exclusion. - home.tsx: FLAG_LABELS now `satisfies Record<RiskFlagType, string>` so a renamed/added signal type fails the build instead of falling through. - home.ts: widen the topFlagged over-fetch (40 → 60) so the table still fills 10 rows after dropping sole-trader (ЕТ) bidders; 0 rows already degrades to the empty-state. Verified: `type` is already in CACHE_QUERY_PARAMS (no ?type= cache split). Note: bidderKind is only company|consortium — no data-level natural-person flag exists, so isNaturalPersonProfileName stays a conservative name prefix to avoid false-positively hiding real companies; robust detection needs an upstream flag (follow-up).
3b5cb57 to
40920ab
Compare
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Прегледах разширения PR на връх 40920ab (обхватът е пораснал доста след одобрението ми на 3b5cb57 — flag филтър на /contracts, CSV, breakdown-и). Силна работа; проверих критичните повърхности и държат. Един should-fix преди merge (PII) и една зависимост от #245.
Каквото проверих, че е коректно:
- Parity aggregate ↔ badge: и петте
FLAG_SQLпредиката съвпадат сriskLogic.ts—admitted = bids_received − COALESCE(bids_rejected,0) = 1(разделено поeu_funded),high_markupсsigning_value_eur > 0(доказуемо =deltaPct > 0.2),anomalies = date_flag='signed_after_publication' OR value_flag IN {4-те suspect}. Числото на началната страница няма да противоречи на badge-а на договора;flagged.test.tsзаковаваFLAG_TYPES ≡ RiskFlagType. - Cache-key чист:
flagе добавен вCANONICAL_QUERY_PARAMS,typeвече беше там — и двата се ключат (drift guard-ът покрива). Няма CWE-349. - Инжекция:
?flag=минава през затворен allow-list (KNOWN_FLAGS) →flagPredicateвръща само статични фрагменти;authorityTypesе параметризиран + валидиран срещуAUTHORITY_TYPE_GROUPS. Чисто. - Value basis:
SUM(CASE WHEN … THEN amount_eur END)пропуска NULL-овете (каноничната база, #98); count-ът брои всеки flagged ред → value_suspect се брои, но дава €0. byType се застъпват (документирано), bySector/byAuthorityType са TOP срезове. ?flag=листът еnoindex(contracts.tsx meta), CSVARRAY_FILTERSе разширен сflags/authorityTypes(пази #56/#122/#138 cache-класификацията), keyset signature включва новите филтри.
Should-fix преди merge (PII, висок приоритет): topFlagged е нова индексирана повърхност, която показва имена на изпълнители под заглавие „сигнали за риск", а guard-ът е само isNaturalPersonProfileName — потвърдих, че той хваща само префикс ЕТ /ET (format.ts:198). ЕТ, записан без точно този префикс (напр. „ЕДНОЛИЧЕН ТЪРГОВЕЦ …" или голо име), минава → идентифицируемо физическо лице може да излезе под рисков етикет на индексирана страница. На value-desc топ-10 е малко вероятно (едрите победители са фирми), затова не блокирам, но suppression-ът не бива да зависи от изписването на името тук. Ползвай по-широката преценка (същата като в #244 subject-risk) или — по-надеждно — изключи физическите лица на SQL ниво по правната форма, не post-hoc по низа.
Свързано (pre-existing, извън обхвата на този PR — маркирам за отделно проследяване): single-offer таблиците на същата начална страница (recentSingleOffer/topSingleOffer) нямат никакъв natural-person филтър — връщат се сурови. recentSingleOffer е по свежест (не по стойност), тъй че дребен договор на ЕТ спокойно може да излезе там без guard. SQL-нивовото изключване би покрило и двете повърхности наведнъж.
Зависимост от #245: high_markup/anomalies и сумите четат current_value_eur/amount_eur, които по #245 са наполовина за договори в лева с евро-анекс от 2026 → flagged числото под-брои точно тези до като #245 не се оправи. Не е дефект на този PR, само за координация.
Одобрявам след затягане на PII guard-а (или изрична обосновка защо плиткият е достатъчен тук).
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR #218 — „Обща маркирана стойност на договорите със сигнали за риск"
ВЕРДИКТ
COMMENT — технически PR-ът покрива критериите за одобрение; оставям вердикта неутрален, за да можеш да прегледаш анализа преди публикуване (по твоя изрична молба да не се коментира в PR-а). Няма блокиращи проблеми.
Фаза 0 — Сканиране за сигурност (задължително, изпълнено първо)
- Твърдо кодирани тайни: няма (без API ключове, пароли, токени).
- Промени по URL адреси: няма нови/променени външни URL адреси. Вътрешните
Link-ове (/contracts?flag=…,/methodology#flagged) са с параметри от затворен набор;typeеencodeURIComponent-нат. - Злонамерени шаблони: няма бекдори, инжекция на код, обфускация или динамично изпълнение.
- Зависимости: няма нови пакети.
node:sqliteиnode:fsсе ползват само в тестове.
Резултат от Фаза 0: CLEAN — продължавам с прегледа.
OWASP / SQL инжекция / инжекция (проверено локално)
- A03 Injection: Проверих генерирането на SQL в
packages/db/src/queries/flagged.tsиcontracts.ts. ПредикатитеFLAG_SQLса константи (не идват от вход на потребителя);flagPredicateмапва само токени от затворен набор. Стойностите на?type=минават през параметризиранIN (?, …)(qs()), не се конкатенират в SQL. Няма SQL инжекция. - Валидация на входа (defense-in-depth):
flagиtypeсе филтрират двойно — първоgetMulti(капва наMAX_MULTI_VALUES = 50, де-дублира), после allow-list (KNOWN_FLAGS,KNOWN_TYPES = AUTHORITY_TYPE_GROUPS). Неразпознати токени → празен резултат (или1=0), а не „покажи всичко". Много добро решение и коректна защита срещу cache-cardinality/DoS. - A01/XSS: Всички стойности се рендират като текст в React (авто-escape); етикетите идват от константна карта
FLAG_LABELS. НямаdangerouslySetInnerHTML. - Поверителност (GDPR/ЗЗЛД):
noindexза?flag=изгледите (contracts.tsx) и филтрирането на физически лица (ЕТ) вtopFlagged(home.ts) са в синхрон с политиката за профили на самоосигуряващи се. Разделите вprivacy.tsx/methodology.tsxдокументират правното основание. Отлично.
Коректност — паритет с per-contract логиката
Сравних FLAG_SQL с evaluateRiskIndicators (riskLogic.ts) — пълен паритет:
no_competition/eu_no_competition:(received − coalesce(rejected,0)) = 1+ разделяне поeu_funded— съвпада сadmitted === 1.high_markup: пазачътsigning_value_eur > 0(не<> 0) прави агрегата алгебрично идентичен на бейджаdeltaPct > 0.2при отрицателна база — коректна поправка (#236), покрита от тестc8.anomalies:date_flag = 'signed_after_publication' OR value_flag IN (…)— съвпада сdateSuspect || value.suspect.- Колоните
value_flag/date_flagсаNOT NULL DEFAULT 'ok', така чеNOT IN/INне крият NULL-капан. - Паричната основа (
SUM(amount_eur), NULL заvalue_suspect) означава, че value-suspect договор се брои, но добавя 0 € — документирано и тествано.
Тестове
packages/db/src/queries/flagged.test.tsе интеграционен срещу реален SQLite от продукционните миграции — доказва, че WHERE-клаузите наистина стесняват/сумират редове (за разлика от fake-D1). Покрива де-дублиран total, застъпване по тип, суми-под-total за сектор/тип институция, отрицателна база (c8), NULL cpv/type_group (c7), OR-комбиниране, неразпознат токен → 0. Тестовете са смислени, а не тривиални.- Обновени са
filters.test.ts,keyset.test.ts,csv-export.test.tsза новите полета и подписа на филтъра. Compile-time guard-ът (CONTRACT_FILTER_KEYS satisfies …+filterSignature) пази срещу класа грешки #138.
CLAUDE.md / качество
- Без частична имплементация, без TODO, без дублиране (единствен източник
FLAG_SQL, преизползван от агрегата и филтъра), без мъртъв код. Именуването е последователно. Разделението на грижите е чисто (SQL слой вdb, UI вweb, контракт вapi-contract). - Каскадата за кеша (edge cache 1h) е същата основа като single-offer скана — без регресия в производителността; пълните сканирания се изпълняват рядко.
Незадължителни бележки (не блокиращи)
?type=линкове ↔ покритие наTYPE_LABELS.byAuthorityTypeгрупира поa.type_groupот БД, а allow-listът за?type=еObject.keys(TYPE_LABELS). Ако някога в БД се появиtype_groupизвън тези 7 канонични кофи (дрейф в нормализацията), линкът от началната страница ще бъде „изяден" от allow-listа и ще покаже всички маркирани договори вместо конкретния тип — тихо разминаване етикет↔резултат. Днес е ОК (каноничен набор), но си струва пазач/тест, който гарантираbyAuthorityType.typeGroup ⊆ AUTHORITY_TYPE_GROUPS.topFlaggedпод-запълване. Свръх-извличането на 60 реда → филтър на физ. лица →slice(0,10)може да не запълни 10 реда при value-desc страница, наситена с ЕТ (вече отбелязано в коментара). Прието като приемливо влошаване към празно състояние; ако темата стане видима, обмисли до-извличане.
Тези две са дребни; не променят вердикта.
Оценка за качество
Сигурност (Фаза 0): CLEAN · SQL/OWASP: чисто · Тестове: 3.0/3.0 · Качество: 2.0/2.0 · Документация: 2.0/2.0 (methodology §10, privacy) · Производителност: 2.0/2.0 · Сигурност (агент): 1.0/1.0. Композитна оценка ≈ 9.6/10.
| // Validate against the closed bucket set: unlike /authorities (a rate-limited aggregation page), | ||
| // /contracts is not rate-limited, so an unvalidated ?type= would let each distinct value mint a | ||
| // fresh edge-cache key AND an uncached full-table scan (cache-cardinality / DoS guard, #218 review). | ||
| authorityTypes: getMulti(sp, 'type').filter((v) => KNOWN_TYPES.has(v)), |
There was a problem hiding this comment.
Allow-listът тук (KNOWN_TYPES = AUTHORITY_TYPE_GROUPS = Object.keys(TYPE_LABELS)) е коректна и добра защита срещу cache-cardinality/DoS. Една бележка за консистентност: линковете на началната страница по „тип институция" се строят от a.type_group в БД (getFlaggedValue.byAuthorityType). Ако някога в БД попадне type_group извън седемте канонични кофи в TYPE_LABELS, съответният линк ?type=<x> ще бъде отхвърлен тук и изгледът тихо ще покаже ВСИЧКИ маркирани договори вместо конкретния тип (разминаване етикет↔резултат). Днес наборът е затворен и е ОК; струва си малък guard/тест, който да гарантира, че всяка стойност type_group от разбивката е ∈ AUTHORITY_TYPE_GROUPS. Не блокиращо.
| 'AND c.signing_value_eur > 0 AND (c.current_value_eur - c.signing_value_eur) > 0.2 * c.signing_value_eur', | ||
| // Value or date anomaly. | ||
| anomalies: `c.date_flag = 'signed_after_publication' OR c.value_flag IN ${SUSPECT_VALUE_FLAGS}`, | ||
| } as const; |
There was a problem hiding this comment.
Пазачът signing_value_eur > 0 (вместо <> 0) е правилен: прави агрегата алгебрично идентичен на per-contract бейджа deltaPct > 0.2 при отрицателна база. Проверих паритета с riskLogic.ts и покритието с c8 в теста — коректно (#236). Няма нужда от промяна.
Address PR midt-bg#236 review feedback: - topFlagged now drops natural-person (sole-trader ЕТ) bidders at the SQL level via `excludeNaturalPersons`, keying on both the "ЕТ " name convention and the registry legal_form rather than the winner's name spelling alone. Running pre-LIMIT also removes the over-fetch-and-slice, so the table fills 10 rows without a post-hoc filter. - getFlaggedValue.byAuthorityType keeps only canonical type_group buckets, so a drifted DB value can never emit a ?type= link the /contracts allow-list silently swallows (label↔result mismatch). - Tests: ЕТ suppression (name + legal_form), consortium kept, drifted type_group dropped from the breakdown.
ydimitrof
left a comment
There was a problem hiding this comment.
Ревю на PR #218 — „Обща маркирана стойност на договорите със сигнали за риск"
ВЕРДИКТ: COMMENT — не блокирам, но има точки за потвърждение преди merge (по-долу). Не публикувам коментари по ваше указание — това е чернова за вашата проверка.
Фаза 0 — Сканиране за сигурност (задължително, изпълнено първо): ЧИСТО ✅
- Твърдо кодирани тайни: няма (никакви ключове/пароли/токени в дифа).
- Промени по URL/домейни: няма нови външни URL адреси; вътрешните линкове (
/contracts?flag=…) са относителни. - Нови зависимости: няма нови пакети (използват се само
node:sqlite,node:fsв тестове и съществуващи@sigma/*). - Зловреден код / backdoor / обфускация / инжекция: не открих.
- SQL инжекция (проверено ръчно, много внимателно):
flagsсе филтрира срещу затворен allow-listKNOWN_FLAGS(FLAG_TYPES ∪ {all}), аflagPredicateвръща само предефинирани, статични SQL фрагменти — потребителският токен никога не влиза в SQL низа.authorityTypesсе филтрира срещуAUTHORITY_TYPE_GROUPSи се подава като параметри презqs(...)+params.push(...)— не чрез конкатенация.flagged.tsстрои SQL само от статични константи (FLAG_SQL,FLAG_TYPES,ANY_FLAG_SQL) — няма интерполация на вход.- При невалидни токени →
1=0(default-deny), а не „покажи всичко". Много добре.
Съответствие с тикета #218 (описание ↔ реализация)
Реализацията покрива описанието: начална страница с обща (де-дублирана) маркирана стойност + разбивки (по вид сигнал / сектор / тип институция) + таблица с топ договори, ?flag= и ?type= филтри на /contracts, и документация (методология §10, privacy — производни показатели). Логиката на сигналите огледва riskLogic и е обвързана с compile-time проверки (satisfies Record<RiskFlagType,string>, assertCovers, тестовете за паритет). Обхватът е фокусиран, без забележим scope creep.
OWASP
- A03 Injection: предотвратено (виж по-горе).
- A01/A08: allow-list за
type=/flag=спира неограничена cache-cardinality/DoS и label↔result несъответствия. - XSS: React екранира текста;
encodeURIComponentе приложен наtype=линка. ОК. - Поверителност (GDPR/ЗЗЛД):
noindexза?flag=изгледи + SQL-ниво изключване на ЕТ (физически лица) от индексируемата начална таблица — добра защита.
CLAUDE.md / качество
- Тестове: отлични — интеграционни тестове върху реален SQLite от продукционните миграции (де-дублиране, застъпване по вид, суми по сектор/тип,
amount_eurNULL базис, ЕТ-подтискане, консорциум-guard, drift-guard). Няма „cheater" тестове. Няма частична имплементация / TODO / мъртъв код. Именуването е консистентно. Единен източник на истината за предикатите (FLAG_SQL) реизползван от агрегата и филтъра — без дублиране.
Точки за потвърждение преди merge (не-блокиращи)
- Производителност:
getFlaggedValueдобавя 3 пълни сканирания наcontracts(общо + по сектор + по тип, с JOIN къмtenders/authorities), аhome.tsдобавя иlistContracts(flags:['all'])— още едно пълно сканиране, върху вече съществуващите. Смекчено от 1ч edge cache, но при cache-miss/студен старт латентността расте. Моля потвърдете индекси и лимитите на D1/Workers (subrequest/CPU/rows). Виж инлайн бележка вflagged.ts. high_markupприvalue_flag IS NULL:c.value_flag NOT IN (...)при NULL дава NULL → редът се изключва. Ако NULL някъде означава „ok", маркупът ще е подценен спрямоriskLogic. Потвърдете, чеvalue_flagе гарантирано non-NULL. Виж инлайн бележка.- Остатъчен PII риск при ЕТ-подтискането: евристиката разчита основно на префикс
„ЕТ "в името (+legal_form, който е предимно NULL). ЕТ без този префикс и с NULLlegal_formби се промъкнал в индексируемата таблица. Освен това SQLiteLIKEе case-insensitive само за ASCII — кирилско„ет "с малки букви не би съвпаднало (разчита се на uppercase нормализацията на регистъра). Приемливо и документирано, но си струва да се потвърди, че входните имена са нормализирани. Виж инлайн бележка. excludeNaturalPersonsизвън cache-подписа: коректно днес, защото се задава само сървърно за началната таблица (цялатаgetHomeDataе edge-cached). Ако някогаlistContractsсе кешира по URL-подпис самостоятелно, два извиквания със същите филтри но различенexcludeNaturalPersonsбиха се сблъскали в кеша. Документирано въвfilter-guard.ts; дръжте инварианта.
Обща оценка: висококачествен, добре тестван и внимателен към сигурността/поверителността PR. Няма открити критични уязвимости или зловреден код. Препоръчвам APPROVE след потвърждаване на точки 1–3.
(Забележка: инструкциите изискват целият текст на ревюто да е на български, затото не прилагам английски вариант тук; мога да предоставя английски превод при поискване.)
| // (current − signing) > 0.2·signing — so `<> 0` would count a negative-base row here that the contract | ||
| // page never badges. `> 0` makes the aggregate and the per-contract badge provably identical (#236 review). | ||
| high_markup: | ||
| `c.value_flag NOT IN ${SUSPECT_VALUE_FLAGS} AND c.signing_value_eur IS NOT NULL ` + |
There was a problem hiding this comment.
Бележка (паритет с riskLogic): c.value_flag NOT IN (...) при value_flag IS NULL дава NULL → редът се третира като неистина и се изключва от high_markup. Ако в продукцията value_flag може да е NULL и NULL се тълкува като „ok", този сигнал ще е подценен спрямо баджа на страницата на договора. Моля потвърдете, че value_flag е гарантирано non-NULL (напр. NOT NULL DEFAULT 'ok' в миграциите); иначе добавете (c.value_flag IS NULL OR c.value_flag NOT IN ...) за явна семантика.
| `COUNT(CASE WHEN (${FLAG_SQL[t]}) THEN 1 END) AS ${t}_n`, | ||
| ]); | ||
|
|
||
| const [totalRow, sectors, authTypes] = await Promise.all([ |
There was a problem hiding this comment.
Производителност: този Promise.all пуска 3 пълни сканирания на contracts (общо + bySector + byAuthorityType с JOIN към tenders/authorities), а home.ts добавя и четвърто чрез listContracts(flags:['all']). Смекчено от 1ч edge cache, но при студен кеш/инвалидиране латентността и rows-scanned растат линейно с корпуса. Моля потвърдете: (а) наличие на индекси, които тези предикати/GROUP BY могат да ползват, и (б) че сумарно оставаме в лимитите на D1/Workers (rows read / CPU / subrequests).
| // GDPR/ЗЗЛД — mirrors the noindex on sole-trader company profiles). | ||
| const NATURAL_PERSON_BIDDER_SQL = `( | ||
| b.kind <> 'consortium' AND ( | ||
| TRIM(b.name) LIKE 'ЕТ %' OR TRIM(b.name) LIKE 'ET %' |
There was a problem hiding this comment.
Остатъчен PII риск: подтискането на физически лица разчита основно на префикс LIKE 'ЕТ %' (полето legal_form е предимно NULL по коментара). ЕТ, чието име не започва с „ЕТ " и няма legal_form, ще се промъкне в индексируемата начална таблица под етикет „сигнали за риск". Освен това SQLite LIKE е case-insensitive само за ASCII — малки кирилски букви ('ет %') няма да съвпаднат, така че защитата зависи от uppercase нормализацията на имената в регистъра. Моля потвърдете, че имената на bidder-ите са нормализирани към горен регистър при ETL; в противен случай обмислете нормализация в самия предикат (UPPER(TRIM(b.name)) LIKE 'ЕТ %').
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Прегледах делтата 40920ab→5e21fad — PII should-fix-ът е адресиран точно както трябва. Одобрявам, условието падна.
- SQL-ниво изключване:
excludeNaturalPersonsдропва физическите лица вbuildFiltersпреди LIMIT,pageSizeсе върна на 10 (без post-hoc filter, който изяждаше реда). Точно това исках. - Широка преценка, не по изписване:
NATURAL_PERSON_BIDDER_SQLOR-ва две сигнали — конвенцията „ЕТ " по име И registrylegal_form(ЕТ/ЕДНОЛИЧЕН ТЪРГОВЕЦ/SOLE TRADER/INDIVIDUAL), сkind <> 'consortium'. Проверих:bidders.legal_formиkind NOT NULLсъществуват (0000_init:92,95), тъй че заявката е валидна;NOT (...)изключва коректно (консорциум остава, ЕТ пада). - filter-guard:
excludeNaturalPersonsе изведен вNonFilterField— server-internal, извънCONTRACT_FILTER_KEYSи cache-signature-а. Правилно (винаги true за единственото вътрешно повикване). - Бонус:
CANONICAL_TYPE_GROUPSguard наbyAuthorityTypeзатваря label↔result разминаването — дрейфналtype_groupвече не може да емитне?type=линк, който allow-list-ът мълчаливо поглъща.
Зависимостта от #245 (euro-annex под-броене) стои — но е извън този PR. Одобрявам.
|
Малък perf-пропуск: Поправка: подай Отделно: euro-annex капанът (#245) удря |
…NULL/perf invariants ydimitrof review (non-blocking confirmations): - PII / Cyrillic case: `bidders.name` keeps the raw source case (normalize-raw.sql stores MIN(contractor_name); only the id is uppercased), and SQLite LIKE/UPPER fold case for ASCII only — a lowercased Cyrillic „ет " slipped past `LIKE 'ЕТ %'`. Match the name prefix with a GLOB character class `[ЕеEe][ТтTt] *` that covers both cases of both scripts independently of any ETL uppercasing. Added a test for lowercase and mixed-case Cyrillic prefixes. - high_markup NULL-swallow: documented that `value_flag` is `NOT NULL DEFAULT 'ok'` (0000_init.sql:126), so `NOT IN (...)` can never yield NULL and silently drop a row. - Performance: documented that the flag predicates are computed expressions (no index applies), so getFlaggedValue is by-design full aggregate scans of a bounded corpus — same shape as the pre-existing single-offer scan — well within D1 rows-read limits / the DoW budget and edge-cached 1h; precompute is the escalation path if the corpus grows an order of magnitude.
…ed-value # Conflicts: # apps/web/app/routes/methodology.tsx # packages/db/src/queries/home.ts
nedda76
left a comment
There was a problem hiding this comment.
Прегледах целия PR — ядрото flagged.ts, филтрите, кеш подписа и тестовете. Много добре обмислена работа; особено ми хареса, че предикатите на сигналите са единствен източник, споделен от агрегата и ?flag= филтъра, тъй че числото на началната страница и значката на страницата на договора не могат да се разминат.
Потвърждава се
- Дедупликацията е коректна: тоталът брои договора веднъж (
CASE WHEN (${ANY_FLAG}) THEN amount_eur), аbyTypeсе застъпва — тестът го заковава (сборът на срезовете > тотала). - Базата за парите е канонична:
SUM(amount_eur)пропуска NULL приvalue_suspect, тъй че такъв договор влиза в бройката, но с 0 € (c4в теста). no_competitionиeu_no_competitionса взаимно изключващи се (eu_funded = 1срещуIS NULL OR = 0) — няма двойно броене.high_markupпазиsigning_value_eur > 0(не<> 0), за да съвпадне сriskLogicпри отрицателна база —c8го покрива. Иvalue_flag NOT IN (...)е защитено срещу NULL-swallow (колоната еNOT NULL DEFAULT 'ok'). Внимателно.- Сигурност:
?type=е валидиран срещу затворения наборAUTHORITY_TYPE_GROUPS, а самата разбивкаbyAuthorityTypeсъщо е ограничена до каноничните кофи — тъй че дрифтналtype_groupне може да излъчи линк, който allow-list-ът тихо поглъща (тестътстранна-кофаго доказва).flagе добавен и към кеш ключа и към подписа. - GDPR:
excludeNaturalPersonsпада ЕТ на SQL ниво предиLIMIT, на два сигнала (име +legal_form), с GLOB, устойчив на малки/смесени букви и кирилица/латиница; консорциум не пада. Добре покрито. - Разходът/DoW е описан честно (4 скана на ограничен корпус под 1ч edge cache, в рамките на
D1_ROWS_READ_BUDGET) — пряко адресира тревогата от #122.
Въпроси (не блокиращи)
- Парност на
high_markupпри чужда валута. Предикатът смята ръста върху*_eurколоните.riskLogicвърху същите*_eurколони ли смятаdeltaPct, или върху нативнитеsigning_value/current_value? При договор в чужда валута, където подписването и анексът се конвертират по различен курс (различни дати), съотношението в евро може да се разминае с това по нативните стойности — и числото на началната страница да не съвпадне със значката. При лев (фиксинг) няма ефект; питам само за чуждовалутните. - Обхват на изключването на физически лица. ЕТ падат от индексираната начална таблица, но на
/contracts?flag=…(noindex) все още се показват поименно. Това ли е желаната позиция спрямо #173/#183 — двустепенно (без индексиране, но публично видимо), или изключването да важи и за списъка с?flag=?
Дребно (по желание)
FlaggedValue.byType[].typeе типизирано катоstringвapi-contract, а UI-ят го привежда къмRiskFlagType. Може да се стегне до самия юнион за end-to-end типове (runtime парността вече е гарантирана от теста).
Солиден, готов за мърдж откъм моя страна. Само преглед — самият мърдж не е мой.
Какво прави
Добавя на началната страница общата стойност на договорите със сигнали за риск (issue #218) — водещо число + разбивки по вид сигнал, по сектор и по тип институция, всяка от които води до филтриран списък с договори. До него — препратка към методологията с прозрачно, неутрално обяснение на сигналите.
Сигналите огледалят per-contract
RiskIndicators(riskLogic.ts), така че числото на началната страница съвпада със значките на страницата на всеки договор. Всичко се смята на живо върху съществуващи колони под 1ч edge cache — без промени по схемата/precompute/миграции (чист Worker deploy, вж.docs/deploy.md).Ключови файлове
packages/db/src/queries/flagged.ts— предикатите на сигналите (единствен източник, споделен от агрегата и?flag=филтъра) + живия агрегат.packages/db/src/queries/home.ts— интеграция в loader-а на началната страница.apps/web/app/routes/{home,methodology}.tsx— рендер + методология §10.apps/web/app/lib/filters.ts,apps/web/workers/cache-key.ts,packages/api-contract—?flag=/?type=drill-down, cache keying, типове.Ревюта — сигурност / GDPR / достъпност
Адресирани в последния коммит (
fix(web): address government security/GDPR/a11y review of #218):noindexна/contracts?flag=списъците. Политиката за поверителност описва производните показатели (чл. 13/14) с правно основание и права по чл. 16/21.?type=на/contractsсе валидира срещу затворения набор отtype_groupкофи (guard за cache-cardinality / DoS, тъй като/contractsне е rate-limited). SQL е само от статични фрагменти + bound параметри;flagе добавен към cache ключа.Проверки
pnpm typecheck7/7 ·@sigma/web366 теста ·@sigma/db203 теста ·pnpm lintчист ·git diffне съдържа*.sql/migrations/промени.