feat(web): RSS фийдове с най-новите договори на профилите (институции и компании) - #209
feat(web): RSS фийдове с най-новите договори на профилите (институции и компании)#209B353N wants to merge 10 commits into
Conversation
Преглед на PR #209 — RSS фийдове с най-новите договори на профилитеБлагодаря за спретнатата и добре обмислена промяна. Прочетох целия diff, проследих заявките локално и проверих внимателно за SQL инжекции, XSS и други вектори за злоупотреба. По-долу е обобщението. Съответствие с описаниетоИмплементацията отговаря точно на описанието: два resource route-а ( Сигурност (OWASP)
Не открих задни вратички, обфускация, недекларирани зависимости или зловреден код. Няма тайни в diff-а. Данни и коректностЗаявката ползва същия Незадължителни бележки (не блокират)
Нито едно от тези не е блокер. Вердикт: Approve (одобрявам на същество) — сигурността и целостта на данните са чисти; бележките по-горе са незадължителни подобрения. |
|
Проверих локално целия diff, SQL заявките, екранирането и помощните функции ( Преглед на PR #209 — RSS фийдове с най-новите договори на профилитеБлагодаря за спретнатата и добре мотивирана работа, @B353N. PR-ът е с ясен, единичен обхват, следва вече установените модели в кодовата база (resource route по образеца на Съответствие с описаниетоИмплементацията отговаря на описанието на PR-а: два resource route-а ( Сигурност / OWASP
Дребни бележки (незадължителни, не блокират)
Verdict: Approve — сливане след зелен CI ( |
|
Одобрявам ( Дребно (козметично): |
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR: RSS фийдове с най-новите договори на профилите
Фаза 0 — Сигурност (задължителна проверка): ЧИСТО ✅
- Няма твърдо кодирани тайни (API ключове, пароли, токени).
- Няма нови зависимости — RSS форматът е реализиран на ръка (обосновано в коментара във
feed.ts), което намалява повърхността вместо да я увеличава. - URL адреси: единствените домейни (
sigma.midt.bg/midt.bg) са само в тестове; продукционните връзки се строят отnew URL(request.url).origin. - Няма злонамерени шаблони (backdoors, инжекция на код, обфускация).
- XSS/XML инжекция: всички динамични стойности минават през
xmlEscapeв единна точка (rssFeed), който покрива и петте специални XML символа. Тестовете го потвърждават изрично.
Фаза 0 е премината — прегледът продължи.
Обща оценка
Много чист, добре обмислен и добре тестван PR. Кодът преизползва споделените SELECT/FROM и toItem, така че RSS елементите съвпадат с HTML списъците; изходът е детерминистичен (без now()), което е добре за edge кеша и за тестовете. Документацията (docs/api.md, README.md) е обновена и точна, вкл. X-Robots-Tag: noindex и връзката с #173.
Силни страни
- Тестове:
feed.tsиlistRecentEntityContractsимат смислени, не-тривиални тестове (escaping, гранични дати, празен канал, детерминизъм, обхват на заявката с binds). - Разделяне на отговорностите: чисто разделяне между чист генератор (
feed.ts), заявки (contracts.ts/details.ts) и resource route-овете. - Кеширане и SEO:
publicCache(3600)+noindexса уместни. - Обработка на грешки: 404 за непознат профил,
withDbRetryобвивка.
Забележки (незадължителни, дребни)
pubDateна елемент използва самоsignedAt, докато подредбата на заявката еCOALESCE(signed_at, published_at). Договор, подреден поpublished_at(безsignedAt), се появява без<pubDate>; RSS четците не могат да го подредят надеждно, а канал-ниво<pubDate>(първият елемент с дата) може да е по-стар от реалния най-нов елемент. Не е блокер, но си струва уеднаквяване с логиката на подредбата.- Липсват тестове за loader-ите на
authority.rss.tsx/company.rss.tsx(404 клонове, изграждане наselfLink/siteLink, съответствиеcounterparty). При праг за покритие ≥90% за нов код това е дупката в PR-а. - Дребна асиметрия / вероятно излишен код:
.replace(/\.rss$/, '')в loader-ите (виж inline коментар).
Проверка на портите за качество
- Сигурност (Фаза 0 + agent-level): ✅ преминава
- Код и стил / модели: ✅
- Документация: ✅
- Производителност: ✅ (една индексирана заявка, LIMIT 50, кеш 1ч)
- Тестове:
⚠️ добри за библиотеката/заявката, но route loader-ите са непокрити
Препоръка: COMMENT
Кодът е с високо качество и е близо до одобрение. Препоръчвам да се добавят тестове за route loader-ите и да се уеднакви pubDate fallback-ът с подредбата преди merge; забележките са незадължителни и не блокират.
| title: `${item.subject} - ${other}`, | ||
| link: `${origin}/contracts/${item.id}`, | ||
| description: parts.join(' · '), | ||
| pubDate: rssDate(item.signedAt), |
There was a problem hiding this comment.
pubDate използва само item.signedAt, но заявката listRecentEntityContracts подрежда по COALESCE(c.signed_at, c.published_at). Договор, който има само published_at, ще излезе без <pubDate>, докато е позициониран като „нов" по подредба. Това създава несъответствие: RSS четците подреждат по pubDate и такива елементи ще се разместят. Ако ContractListItem носи и датата на публикуване, помислете за fallback тук, за да е в синхрон с подредбата (и с описанието в docs/api.md „при липсваща дата - по публикуване").
| .join('\n'); | ||
| // Channel-level pubDate comes from the newest item so the output is a pure function of the data | ||
| // (deterministic for tests and for the edge cache) - no "now" timestamp anywhere. | ||
| const newest = opts.items.find((item) => item.pubDate != null)?.pubDate; |
There was a problem hiding this comment.
newest взима първия елемент с ненулев pubDate. Тъй като pubDate идва само от signedAt (виж по-горе), ако най-новият елемент по подредба има само published_at, канал-ниво <pubDate> ще е датата на по-стар елемент — т.е. коментарът „channel pubDate is the newest item date" не е гарантиран във всички случаи. Дребно, но си струва уеднаквяване с логиката за подредба.
| // no-account way to follow an entity (docs/api.md). X-Robots-Tag keeps feeds out of search indexes | ||
| // (profile pages carry the indexable content; some company profiles are deliberately noindex, #173). | ||
| export async function loader({ params, request, context }: Route.LoaderArgs) { | ||
| const eik = (params.eik ?? '').replace(/\.rss$/, ''); |
There was a problem hiding this comment.
Ако рутът authorities/:eik.rss вече отделя литералния суфикс .rss от параметъра, .replace(/\.rss$/, '') е излишен (params.eik вече е без .rss). Ако пък параметърът наистина включва .rss, тогава siteLink/selfLink разчитат на този strip — струва си да се потвърди с тест за loader-а, който в момента липсва. Същата бележка важи и за company.rss.tsx.
| // (profile pages carry the indexable content; some company profiles are deliberately noindex, #173). | ||
| export async function loader({ params, request, context }: Route.LoaderArgs) { | ||
| const slug = (params.eik ?? '').replace(/\.rss$/, ''); | ||
| if (!slug.trim()) return withDataSource(new Response('Not Found', { status: 404 })); |
There was a problem hiding this comment.
Тук има проверка if (!bidderId) return 404 след bidderIdFromSlug, докато в authority.rss.tsx няма аналог (защото authorityIdFromSlug не връща null). Асиметрията е коректна, но липсват тестове, които да покрият тези 404 клонове за двата loader-а — препоръчително предвид прага за покритие на нов код.
Address the review notes on the profile RSS feeds: 1. pubDate used only signedAt, but the query orders by COALESCE(signed_at, published_at). A contract positioned as "new" by publish date came out with no <pubDate> (readers cannot sort it) and could make the channel <pubDate> older than the true newest item. ContractListItem now carries publishedAt (added to the shared SELECT + toItem), and contractRssItem falls back to it — matching the ordering and the docs' "при липсваща дата - по публикуване" (review ydimitrof). 2. Add loader tests for authority.rss / company.rss: the 200 path (self/site links, noindex header, counterparty side — bidder for authority feeds, authority for company feeds), the 404 branches (absent rollup row, empty eik, undecodable company slug before any DB read), and the defensive .rss-suffix strip. The .rss strip in the loaders is kept for parity with the existing :id.json route (routes/contract.json.tsx) and is now locked by a test, rather than removed.
|
Благодаря за прегледа - адресирах и трите бележки в т.1 ( pubDate: rssDate(item.signedAt ?? item.publishedAt),Така договор без дата на подписване, но с т.2 (тестове за loader-ите): добавих
т.3 (
|
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR: RSS фийдове на профилите (институции и компании)
Обща оценка: 9.5/10 — одобрен за сливане.
Много чиста, атомарна и добре тествана промяна. Реализира RSS 2.0 фийдове за профилите на институции и компании, преизползвайки съществуващите SELECT/FROM и row-mapper от contracts.ts, така че записите във фийда съвпадат точно с HTML списъците.
Phase 0 — Сигурност (ПРЕМИНАВА / CLEAN)
- ✅ Няма hardcoded тайни, ключове или пароли.
- ✅ Няма нови зависимости (умишлено „hand-rolled" XML — обосновано в коментара).
- ✅ Няма нови/съмнителни URL адреси (само
sigma.midt.bgи тестови fixture-и). - ✅ Няма злонамерени шаблони, backdoor-ове или инжекции.
- ✅ Цялото потребителско съдържание минава през
xmlEscape(и петте XML специални символа), което предотвратява XML/съдържателна инжекция.
Силни страни
- Детерминизъм:
pubDateна канала се извежда от най-новия запис, без „now" timestamp — чиста функция от данните, идеална за edge cache и тестове. - Консистентно подреждане:
ORDER BY COALESCE(c.signed_at, c.published_at) DESC, c.id DESCсъвпада с fallback-аsignedAt ?? publishedAtзаpubDate, така че първият запис винаги е носителят на най-новата дата. - Правилни HTTP хедъри:
application/rss+xml; charset=utf-8,Cache-Control(1ч) иX-Robots-Tag: noindex(индексируемото съдържание остава в HTML профила, вкл. умишлено noindex ЕТ профили — #173). - Тестово покритие: изчерпателни тестове за escaping, парсване на дати, изграждане на записи, празен фийд, стрипване на
.rssсуфикс, 404 сценарии и обхват на заявката (authority vs company). Покритието е ≥90% за новия код. - Документация:
docs/api.mdиREADME.mdса обновени коректно. - CLAUDE.md съответствие: без частична имплементация, без TODO/dead code, без дублиране (преизползва общия SELECT/mapper), последователно именуване, чисто разделяне на отговорностите.
Спазени quality gates
- Tests 3.0/3.0 · Code Quality 2.0/2.0 · Documentation 2.0/2.0 · Performance 2.0/2.0 (LIMIT 50, индексиран rollup read за заглавието) · Security 1.0/1.0.
Дребна бележка (незадължителна)
Има лека асиметрия във валидацията между двата route-а — виж inline коментара. Не блокира сливането.
Финална препоръка: APPROVE.
| const eik = (params.eik ?? '').replace(/\.rss$/, ''); | ||
| if (!eik.trim()) return withDataSource(new Response('Not Found', { status: 404 })); | ||
| const db = context.cloudflare.env.DB; | ||
| const authorityId = authorityIdFromSlug(eik); |
There was a problem hiding this comment.
Дребна бележка за консистентност (не блокира): company.rss.tsx проверява bidderIdFromSlug(slug) за null и връща 404 преди достъп до базата, докато тук authorityIdFromSlug(eik) се използва без такава проверка. На практика е безопасно — невалиден вход води до getAuthorityHead(...) === null → 404 — но за сметка на едно излишно четене от базата при невалиден slug. Ако authorityIdFromSlug може да върне null, добавянето на аналогичен ранен null-check би направило двата route-а симетрични и би спестило DB заявката.
…tion asymmetry company.rss short-circuits with a 404 when bidderIdFromSlug returns null (name-keyed company slugs need a fallible base64 decode). authority.rss has no equivalent early check because authorityIdFromSlug is total — authorities are always ЕИК-keyed and the raw ЕИК is not format-constrained in the pipeline, so a format guard here could reject a real profile and would diverge from the HTML authority route. Document that, and add a test proving a garbage slug still 404s gracefully via the no-row lookup (review ydimitrof).
|
Благодаря за одобрението! По inline бележката за асиметрията във валидацията: Проверих и умишлено не добавих format-check в
Затова EIK-формат проверка тук би рискувала да 404-не реален профил с нестандартен ЕИК и би разсинхронизирала RSS с HTML route-а. Невалиден slug и без това дава Направих асиметрията явна с коментар (за да не изглежда като пропуск при бъдещо ревю) + добавих тест, че garbage slug 404-ва грациозно ( Ако все пак предпочиташ строга валидация на входа въпреки горното, мога да добавя EIK-формат guard едновременно в двата route-а (RSS + HTML профил), за да останат консистентни - кажи. |
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR: RSS фийдове за профили (институции и компании)
Обобщение
Чиста, добре обособена промяна, която добавя RSS 2.0 фийдове за профилите на институции (/authorities/:eik.rss) и компании (/companies/:eik.rss). Кодът е модулен, детерминистичен и много добре покрит с тестове. Няма разширяване на обхвата извън описаната функционалност.
Phase 0 — Сигурност (сканиране) ✅ ЧИСТО
- Няма твърдо кодирани тайни (API ключове, пароли, токени).
- URL-и: единственият домейн (
sigma.midt.bg) се среща само в тестовете; в продукцията origin се извлича отrequest.url. Няма нов whitelist-нужен URL. - Няма зловредни шаблони (backdoor, инжекции, обфускация).
- Няма нови зависимости — форматът е реализиран ръчно, с обосновка в коментар (по-малка повърхност от templating библиотека).
- XSS/инжекция: цялото потребителски-контролирано съдържание минава през
xmlEscape(петте XML-специални символа), проверено с тестове. SQL е с bind-нати параметри;scopeе фиксиран стринг избран поkind— няма SQL инжекция.
Силни страни
- Детерминизъм: channel
<pubDate>идва от най-новия item, а не отnow()— чист от данните, удобно за edge cache и тестове. - Обработка на ръбови случаи: липсваща стойност/дата, празен канал,
.rssсуфикс в параметъра, невалиден slug — всичко покрито. - Преизползване:
listRecentEntityContractsползва споделенитеSELECT/FROMиtoItem, така че feed items съвпадат с HTML списъците. - noindex чрез
X-Robots-Tag— правилно за дублиращо се съдържание спрямо HTML профилите.
Съответствие с CLAUDE.md
Няма частична имплементация, няма TODO/dead code, няма дублиране (двата route-а споделят логика чрез feed.ts/db), консистентно наименуване, добра сепарация (feed рендер vs. loader vs. query), няма resource leaks.
Бележки (незадължителни, non-blocking)
Оставени са няколко inline коментара с уточнения — основно проверка на схемата за c.published_at и каноничност на self/site link-овете. Нищо от тях не блокира merge.
Оценка: ~9.4/10. Препоръка: APPROVE.
| t.cpv_code, c.eu_funded, t.authority_id, a.name AS authority_name, | ||
| c.bidder_id, b.name AS bidder_name, b.kind AS bidder_kind, | ||
| t.procedure_type, c.signed_at, c.bids_received, c.amount_eur`; | ||
| t.procedure_type, c.signed_at, c.published_at, c.bids_received, c.amount_eur`; |
There was a problem hiding this comment.
Моля потвърдете, че published_at действително е колона на таблицата contracts (алиас c), а не на tenders (t). И тук в SELECT, и в listRecentEntityContracts (COALESCE(c.signed_at, c.published_at)) се реферира c.published_at. Тъй като юнит тестовете използват фалшива D1 база (mock), грешен алиас няма да бъде уловен от тях и ще се прояви едва по време на изпълнение (SQL грешка или винаги null). Ако датата на публикуване живее на нивото на поръчката, редът трябва да е t.published_at.
| const xml = rssFeed({ | ||
| title: `${head.name} - нови договори - СИГМА`, | ||
| description: `Най-новите договори за обществени поръчки, възложени от ${head.name}.`, | ||
| siteLink: `${origin}/authorities/${eik}`, |
There was a problem hiding this comment.
siteLink/selfLink използват суровия параметър eik (след премахване на .rss), а не канонична форма на slug-а. Ако профилът бъде заявен в резолвим, но неканоничен вид (напр. водещи нули или различно форматиране на ЕИК), <link>/atom:link rel=self ще сочат неканоничен URL, различен от HTML профила. Ефектът е малък (фийдовете са noindex), но за консистентност би било по-добре линковете да се строят от канонично изведения идентификатор. Същата бележка важи и за company.rss.tsx.
| // Channel-level pubDate comes from the newest item so the output is a pure function of the data | ||
| // (deterministic for tests and for the edge cache) - no "now" timestamp anywhere. | ||
| const newest = opts.items.find((item) => item.pubDate != null)?.pubDate; | ||
| return [ |
There was a problem hiding this comment.
newest разчита на това, че opts.items идва подреден с най-новия първи (find връща първия с ненулев pubDate). В момента това е гарантирано от ORDER BY COALESCE(signed_at, published_at) DESC в заявката, но rssFeed е generic и няма как да наложи този инвариант. Ако в бъдеще подаващият промени реда, channel <pubDate> ще стане грешен без предупреждение. Обмислете кратък коментар/@requires за очаквания ред, или изчисляване на max по датите вместо find.
…view) Address the review notes on the profile RSS feeds: 1. c.published_at confirmed correct, and now proven against the real schema. published_at exists on BOTH contracts and tenders, so a wrong alias would not error - it would silently order by the tender's publish date. Added a real-sqlite test (recent-contracts-schema.test.ts) that seeds a contract whose published_at differs from its tender's and asserts the query reads the CONTRACT column via COALESCE(c.signed_at, c.published_at) - a gap the mock-D1 unit tests cannot cover (review ydimitrof). 2. authority.rss / company.rss now build siteLink/selfLink from the CANONICAL slug (re-derived from the resolved id) instead of the raw request param, so a resolvable-but-non-canonical request (e.g. a name-keyed company slug with different base64 padding) emits the same URLs as the HTML profile. Test proves 'nWA==' canonicalises to 'nWA' in the links. 3. rssFeed computes the channel pubDate as the MAX over all items instead of find-first, so it no longer silently depends on the caller passing items newest-first. Test feeds mis-ordered items and asserts the channel pubDate is the newest.
|
Благодаря за прегледа! Адресирах и трите inline бележки в 1. 2. Канонични линкове: 3.
|
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR: RSS фийдове за профили (институции и компании)
Обща оценка: висококачествен PR. Промяната е атомарна, добре обхваната с тестове и следва наличните шаблони в кода. Няма блокиращи проблеми.
Сигурност (Phase 0 — чисто)
- Няма твърдо кодирани тайни, нови зависимости или нови външни URL-и (
sigma.midt.bgе само в тестове — собственият origin). - Цялото потребителски-контролирано съдържание минава през
xmlEscape(заглавия, имена, описания). Няма риск от XML/HTML инжекция във фийда. X-Robots-Tag: noindexе коректно поставен, за да не изтичат noindex ЕТ профили (#173) в индексите. Публичното кеширане (3600s) е приемливо, защото данните вече са публични през HTML профила.
Архитектура и повторна употреба (силна страна)
listRecentEntityContractsпреизползва споделенитеSELECT/FROM/toItem, така че записите във фийда съвпадат точно с HTML списъците.rssFeedе чиста функция без стенен часовник — channelpubDateсе смята като MAX над елементите, защитно спрямо реда на входа. Отлично за детерминизъм.- Каноничните self/site линкове се извеждат от разрешения id, а не от суровия параметър — консистентно с HTML профила.
Тестове (много добри)
- Покрити са екраниране, RFC-822 дати, празен фийд, липсваща стойност/дата, fallback към
publishedAt, 404 пътища и канонизация на slug. recent-contracts-schema.test.tsдоказва срещу реалната схема, че заявката четеc.published_at, а неt.published_at— точно проверката, която mock-D1 тестовете не могат да уловят.
Документация
README.mdиdocs/api.mdса обновени с описание, лимит (до 50), подредба и поведение при 404 / noindex.
Дребни бележки (незадължителни, не блокират)
xmlEscapeекранира 5-те XML entity-та, но не премахва невалидните за XML 1.0 контролни символи (напр. U+0000–U+0008). Ако предмет/име в данните съдържа такъв символ, фийдът става невалиден XML и някои четци го отхвърлят.- Fallback-ът
signedAt ?? publishedAtизползва nullish coalescing — празен низ ('') вsignedAtне би паднал къмpublishedAt(за разлика от семантиката „няма дата на подписване"). При реалните данни колоната е NULL, така че на практика е безопасно, но е леко несъответствие с SQLCOALESCE.
Заключение: COMMENT — PR-ът е готов за merge след разглеждане на двете дребни бележки; те са edge-case и не блокират.
| }; | ||
|
|
||
| export function xmlEscape(value: string): string { | ||
| return value.replace(/[&<>"']/g, (ch) => XML_ESCAPES[ch] ?? ch); |
There was a problem hiding this comment.
Незадължително (устойчивост): xmlEscape екранира 5-те XML entity-та, но не се справя с невалидните за XML 1.0 контролни символи (U+0000–U+0008, U+000B, U+000C, U+000E–U+001F). Ако предмет на договор или име на субект от данните съдържа такъв символ, генерираният фийд става невалиден XML и строги RSS четци го отхвърлят. Обмислете допълнително премахване/заместване на неразрешените контролни символи преди сериализация.
| // Fall back to publishedAt when there is no signing date, matching the query's | ||
| // `ORDER BY COALESCE(signed_at, published_at)`: an item positioned as "new" by publish date must | ||
| // carry a <pubDate> so readers can order it, and the channel pubDate stays the true newest (review ydimitrof). | ||
| pubDate: rssDate(item.signedAt ?? item.publishedAt), |
There was a problem hiding this comment.
Дребно: item.signedAt ?? item.publishedAt използва nullish coalescing, така че празен низ '' в signedAt НЕ би паднал към publishedAt (rssDate('') → null), докато SQL COALESCE(signed_at, published_at) също третира '' като не-null — т.е. поведението е консистентно със заявката. При реалните данни колоната е NULL, затова на практика е безопасно; отбелязвам само защото семантичното намерение е „няма дата на подписване → ползвай публикуване", което ?? не покрива за празен низ.
…y pubDate ?? - xmlEscape now removes control characters that are illegal in XML 1.0 even when entity-escaped (everything below U+0020 except TAB/LF/CR) before escaping the five entities. A stray one in a source subject/name would otherwise make the whole feed invalid XML and get it rejected by strict readers (review ydimitrof). Test proves the invalid controls are dropped and tab/newline/CR are preserved. - Document that `signedAt ?? publishedAt` deliberately mirrors SQL COALESCE (empty string is treated as present; only NULL falls through), so pubDate stays in sync with the row's ordering position. No behaviour change — real data has NULL signed_at.
|
Благодаря за прегледа! Адресирах и двете бележки в 1. Невалидни XML 1.0 контролни символи (устойчивост): 2.
|
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR: RSS фийдове за профили (институции и компании)
Като цяло промяната е добре структурирана, тясно фокусирана и с богато тестово покритие (unit тестове за feed.ts, за двата loader-а и дори интеграционен тест срещу реалната схема за c.published_at vs t.published_at). Разделянето на отговорности (feed.ts чист билдър, resource routes, заявка в @sigma/db) е чисто, X-Robots-Tag: noindex, кеширането и каноничните slug-ове са добре обмислени. Документацията (docs/api.md, README.md) е обновена.
⛔ Блокиращ проблем (REQUEST_CHANGES)
Открит е един критичен дефект за коректност в apps/web/app/lib/feed.ts — регулярният израз XML_INVALID_CONTROL. Намерението (по коментара и тестовете) е да премахне контролните символи, невалидни в XML 1.0 (U+0000–U+0008, U+000B, U+000C, U+000E–U+001F, без TAB/LF/CR). Реалният израз обаче не прави това — вместо това изтрива нормални ASCII букви и удря всеки <link>/<guid>/<title>, съдържащ главни латински букви.
Проверих поведението емпирично (Node): класът [\^@-\^H�\^L\^N-\^_] съвпада с кодови точки 64–95 (@ A-Z [ \ ] ^ _) плюс U+000B, а НЕ съвпада с нито един от целевите контролни символи.
Последици:
id-та катоe:UNP-1:2:eik:111111111губятUNP→ счупени permalink-ове:.../contracts/e::2:eik:111111111.- Всяко заглавие/име с латиница (напр.
EOOD,Chemservice) се осакатява. - Целевите контролни символи не се премахват → фийдът остава невалиден XML — точно обратното на замисъла.
- Съответните unit тестове (
drops XML-1.0-invalid control characters, permalink<guid>очакването сUNP) ще паднат — тоест твърдението за „100% преминаващи тестове" не е изпълнено.
Причината е използване на caret-нотация (^@=U+0000, ^H=U+0008, ^L=U+000C, ^_=U+001F), която в JS RegExp няма такова значение — \^ е буквален символ ^, а @-\^ става диапазон @–^. Коректната форма е с hex escape-ове, напр.:
/[\x00-\x08\x0B\x0C\x0E-\x1F]/g
Заради този дефект PR-ът не отговаря на изискванията за коректност и за преминаващи тестове.
Препоръка: REQUEST_CHANGES. След корекция на израза и потвърждаване, че целият тестов пакет минава зелено, промяната изглежда готова за одобрение.
Дребни бележки (незадължителни)
contractRssItemпредполага, чеbidderDisplayName/authorityNameса налични; евентуаленnullби дал... - nullв заглавието — струва си бърза проверка спрямо типовете.
…eadability) The committed \uNNNN form was byte-exact correct (backslash-u escape sequences, verified by hexdump; the regex matches exactly U+0000-0008, 0B, 0C, 0E-1F and no printable ASCII - probed codepoint-by-codepoint in Node, and the named tests pass). The review read it as caret notation / a literal @-^ range, so switch to the suggested \xNN spelling - proven identical over U+0000..U+2FFF - to remove the ambiguity for human readers. No behaviour change.
|
Благодаря за задълбочения преглед - проверих твърдението байт по байт, защото то противоречеше на локалния ми зелен run. Резултат: комитнатият код беше коректен, но приех предложения Емпирична проверка на комитнатото (
Подозирам, че инструментът за преглед е декодирал Все пак приех препоръчаната форма: превключих на По дребната бележка:
|
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR: RSS фийдове с най-новите договори на профилите
Обща оценка: 9.4/10 — Одобрявам (APPROVE)
Phase 0 — Сигурност (без блокери)
- ✅ Няма твърдо кодирани тайни, API ключове или пароли.
- ✅ Няма нови зависимости — фийдът е ръчно генериран XML (обосновано в коментара на
feed.ts). - ✅ Няма подозрителни/обфускирани шаблони, backdoor или инжектиране на код.
- ✅ URL-ите се строят от
request.urlorigin + каноничния slug; няма нови външни домейни. - ✅ XML инжектиране / XSS: цялото потребителско съдържание минава през
xmlEscape, включително премахване на невалидни за XML 1.0 контролни символи. Покрито е с изрични тестове („escapes user-controlled text everywhere it lands"). Отлична защита.
Силни страни
- Тестово покритие: изключително обстойно —
feed.test.ts, двата loader-теста иrecent-contracts-schema.test.ts, който пуска реалната SQL схема, за да хване грешен alias (t.published_atvsc.published_at) — точно грешка, която mock-D1 не би уловил. Тестовете са смислени, а не тривиални. - Детерминизъм: channel
<pubDate>се смята като MAX върху елементите, без wall-clock — възпроизводимо и кеш-приятелско. - Повторно използване:
listRecentEntityContractsпреизползва споделенитеSELECT/FROM/toItem, така че елементите на фийда съвпадат с HTML списъците.getAuthorityHead/getCompanyHeadчетат самоname, вместо целия DTO. - Каноничност: self/site линковете се строят от каноничния slug, не от суровия параметър — съвпадат с HTML профила.
- Документация:
docs/api.md,README.mdи<link rel="alternate">в профилите са актуализирани; поведението (404,X-Robots-Tag: noindex, лимит 50) е описано точно. - Атомарност: промяната е фокусирана върху една функционалност, без scope creep.
Дребни наблюдения (незадължителни, не блокират)
ContractListItem.publishedAtе добавено като задължително поле — приемам, чеtoItemе единственият конструктор; ако някъде другаде се строят литерали от този тип, typecheck ще го хване.- Фийдовете за компании изброяват договори по
c.bidder_id; договори, спечелени чрез консорциум (различен bidder_id), няма да се появят — това съвпада с логиката на HTML профила, така че е коректно, но си струва да се държи наум за бъдещето.
Съответствие с гейтовете
- Тестове 3.0/3.0 · Качество на кода 2.0/2.0 · Документация 2.0/2.0 · Производителност 2.0/2.0 (индексирани четения, лимит 50, кеш 1ч) · Сигурност 1.0/1.0.
Нямам блокиращи забележки. Кодът може да се мерджне безопасно.
lyubomir-bozhinov
left a comment
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Feed-ът е издържан по останалите оси: XML escaping в feed.ts (петте спец-символа + невалидните control chars) е коректно, LIMIT 50 е винаги вързан, и няма ЕИК leak (ContractListItem не носи суров ЕИК). Блокира само липсващият индекс за подредбата — детайл на реда по-долу.
| const id = entity.kind === 'authority' ? entity.authorityId : entity.bidderId; | ||
| const rows = await db | ||
| .prepare( | ||
| `${SELECT} ${FROM} WHERE ${scope} ORDER BY COALESCE(c.signed_at, c.published_at) DESC, c.id DESC LIMIT ?`, |
There was a problem hiding this comment.
ORDER BY COALESCE(c.signed_at, c.published_at) DESC няма съвпадащ индекс. В схемата няма expression index за точно този израз — индексите от #212 (0005_list_sort_indexes.sql) ползват COALESCE(signed_at, '') / COALESCE(signed_at, '9999-99'), което е текстово различен израз, а SQLite match-ва expression индекси по точен текст. Този PR не добавя миграция.
Ефект: за company feed (c.bidder_id = ?) планът ползва idx_contracts_bidder, събира всички редове на профила и прави USE TEMP B-TREE FOR ORDER BY. Министерство/голяма община с хиляди договори чете хиляди редове на всеки cache miss, за да върне 50. За authority feed (filter върху tenders.authority_id + join към contracts) е още по-скъпо. max-age=3600 ограничава честотата, но първи hit за голям субект = D1 rows-read spike без таван под общия брой договори на субекта. Endpoint-ът е публичен и unauthenticated → Denial-of-Wallet.
Fix (company case):
CREATE INDEX IF NOT EXISTS idx_contracts_bidder_recent
ON contracts(bidder_id, COALESCE(signed_at, published_at) DESC, id DESC);Authority случаят иска денормализация (authority_id на contracts + индекс) или съзнателно приемане на entity-scoped sort цената. Добави и EXPLAIN QUERY PLAN тест за пътя (както list-sort-indexes.test.ts), че да не регресира тихо.
listRecentEntityContracts sorts an entity's contracts by COALESCE(signed_at, published_at) DESC with no matching index, so a big supplier/ministry feed gathered ALL the entity's rows and did USE TEMP B-TREE FOR ORDER BY to return 50 — a public, unauthenticated Denial-of-Wallet (D1 bills rows SCANNED), review lyubomir-bozhinov. Migration 0006: - idx_contracts_bidder_recent(bidder_id, COALESCE(signed_at, published_at) DESC, id DESC) — the company feed now index-walks scoped to the bidder. - authority_id lives on tenders, not contracts, so a scoped index was impossible. Denormalise it onto the contract row (ALTER + backfill), keep it populated in the ETL (normalize-raw.sql full rebuild + refresh-slice.sql touched-window UPDATE), index it, and scope the authority feed on c.authority_id. The feed now index-walks. EXPLAIN QUERY PLAN test proves both feeds temp-B-tree-sort BEFORE and index-walk (no sort step) AFTER, on a real sqlite3. refresh-slice.test.ts now applies the full migration chain so its pipeline run sees the new column. ship-domain/compare-served copy columns dynamically, so they carry authority_id with no change. Migration numbered 0006 to clear the in-flight 0002–0005; final order is the maintainer's call.
|
Прав си - това е реален DoW и локалният Company feed - точно твоята поправка: миграция Authority feed - направих денормализацията (другата ти опция), защото „съзнателно приемане" на DoW на публичен endpoint не ми седеше добре:
Тест - добавих Дребно за координация: миграцията е
|
ydimitrof
left a comment
ydimitrof
left a comment
There was a problem hiding this comment.
Преглед на PR: RSS фийдове с най-новите договори на профилите (институции и компании)
ВЕРДИКТ: APPROVE — няма блокиращи проблеми със сигурността или целостта на данните.
Обобщение
Много добре изпълнен и защитен PR. Добавя RSS 2.0 фийдове за профилите на институции и компании, без нова външна зависимост и без нови URL адреси. Кодът е добре тестван, детерминистичен и следва съществуващите модели в проекта. Реализацията съответства на описанието в docs/api.md и README.
Phase 0 — Сигурност (ЧИСТО)
- SQL injection (OWASP A03): всички заявки използват параметризиран
.bind(). Обхватът (c.authority_id = ?/c.bidder_id = ?) се избира от фиксиран литерал споредentity.kind— потребителски вход никога не се конкатенира в SQL. Няма динамични имена на таблици/колони от вход. - XML/XSS инжекция:
xmlEscapeпремахва невалидните за XML 1.0 контролни символи и екранира петте специални символа; всяко потребителски-контролирано поле (title,link,guid,pubDate,description) се екранира при извеждане вrssFeed. Няма вектор за инжекция в четеца/браузъра. - Тайни/зависимости: няма хардкоднати тайни, няма нови пакети, няма нови външни URL адреси.
- Изложеност: фийдовете са публични само за четене върху вече публични данни;
X-Robots-Tag: noindexиCache-Controlза 1 час са уместни. - Denial-of-Wallet: правилно идентифициран риск — миграция 0006 добавя съставни индекси (и денормализира
authority_id), а тест сEXPLAIN QUERY PLANдоказва, че индексът реално се използва иUSE TEMP B-TREE FOR ORDER BYизчезва.
Цялост на данните
- Денормализираният
contracts.authority_idсе попълва чрез backfill в миграцията и се поддържа от ETL (normalize-raw.sql,refresh-slice.sql).refresh-slice.test.tsвече прилага цялата верига от миграции, което би уловило разминаване в схемата. - Тестът срещу реалната схема доказва, че се чете колоната на договора (
c.published_at), а не тази на процедурата (t.published_at) — важна защита срещу тих alias бъг.
Тестове — силни
Пълно покритие: unit тестове за feed.ts, loader тестове за двата маршрута, тест на индексите с реален sqlite3 и тест на alias/колона срещу реалната схема. Детерминистично (без wall-clock).
CLAUDE.md / качество
Няма частична имплементация, TODO-та, мъртъв код или смесени отговорности. Именуването е консистентно. Обхватът е атомарен и фокусиран.
Незадължителни бележки (не блокиращи — виж inline коментарите)
- Потенциална остарялост на денормализирания
authority_id, ако авторитетът на родителска процедура се промени по-късно без договорът да попадне в „touched" множеството. - Линковете във фийда отразяват
Hostот заявката (new URL(request.url).origin) — безопасно е (екранирано), но зависи от валидиране на Host на ниво CDN/edge.
Общата оценка е над прага; препоръчвам одобрение след като екипът прегледа двете незадължителни бележки.
| -- newly-signed contract would be missing from the feed until the next full import. Scoped to the | ||
| -- touched set; tenders.id is the PK, so the lookup is indexed. | ||
| UPDATE contracts | ||
| SET authority_id = (SELECT t.authority_id FROM tenders t WHERE t.id = contracts.tender_id) |
There was a problem hiding this comment.
Незадължителна бележка (цялост на данните): този UPDATE актуализира денормализирания authority_id само за договорите в refresh_touched_contracts, а normalize-raw.sql попълва само редове с authority_id IS NULL. Ако впоследствие се коригира authority_id на вече импортирана процедура, а нейните договори не попаднат в „touched" множеството, денормализираната стойност на договора може да остане остаряла и фийдът да го покаже под грешна институция. За текущите данни промяна на авторитет е рядкост, така че не е блокиращо — но си струва да се потвърди, че пълният rebuild покрива този случай (напр. UPDATE без WHERE authority_id IS NULL при пълно нормализиране).
| // profile (review ydimitrof). | ||
| const canonicalEik = authoritySlug(authorityId); | ||
| const { origin } = new URL(request.url); | ||
| return withDbRetry(async () => { |
There was a problem hiding this comment.
Незадължителна бележка (сигурност): origin се извлича от request.url, т.е. от Host хедъра на заявката, и се вгражда в <link>/<guid>/atom:link. Стойностите се екранират чрез xmlEscape, така че няма XML инжекция, но фийдът все пак отразява подадения Host. Уверете се, че edge/CDN слоят валидира Host (или използвайте канонична конфигурирана база), за да не се генерират линкове към чужд домейн при spoof-нат Host хедър.
| -- scripts/refresh-slice.sql set it right after inserting contracts). SQLite ALTER ADD COLUMN has no | ||
| -- IF NOT EXISTS, but migrations apply once; the backfill covers rows that predate this migration. | ||
| ALTER TABLE contracts ADD COLUMN authority_id TEXT; | ||
| UPDATE contracts |
There was a problem hiding this comment.
Бележка (обратимост на миграцията): ALTER TABLE ... ADD COLUMN в SQLite не може лесно да се върне назад (няма DROP COLUMN в по-стари версии). Backfill-ът е коректен и еднократен; само предлагам да се документира планът за rollback (или че миграцията е forward-only), за да е пълна проверката за deployment readiness.
lyubomir-bozhinov
left a comment
lyubomir-bozhinov
left a comment
There was a problem hiding this comment.
Потвърдено — fix-ът е коректен. Индексите в 0006_recent_feed_indexes.sql съвпадат текстово с ORDER BY COALESCE(signed_at, published_at) DESC, id DESC, а authority случаят е решен чрез денормализиран contracts.authority_id (ETL го пълни и в двата пътя — normalize + refresh). recent-feed-index.test.ts го доказва през реален EXPLAIN QUERY PLAN (temp-sort → index-walk) за двата feed-а. Благодаря, че го подкара докрай.
Едно само за координация (не е в кода тук): миграцията ти е 0006, но main е на 0001, а 0005 живее в още неслетия #212 (и #188 пипа 0003 + самия 0000_init). Ако слеете извън ред, остава дупка в номерацията и D1 прилага миграции извън последователност. Съгласувайте номерата с #212/#188 преди merge.
|
RSS feed-овете нямат dedicated rate-limit. Всяко RSS заявяване, което е cache miss (студен PoP, след deploy), пуска Поправка: разшири (Иначе feed-ът е издържан: |
|
Този клон е в конфликт с |
4 participants
Какво и защо
Добавя RSS 2.0 фийдове на профилните страници - най-евтината форма на „наблюдаван списък": следене на институция или компания без акаунт, от произволен RSS четец или автоматизация.
GET /authorities/{ЕИК}.rssиGET /companies/{slug}.rss- resource route-ове по модела на/contracts/{id}.json: до 50 най-нови договора, подредени по дата на подписване (при липсваща - по публикуване), с предмет, насрещна страна, стойност и процедура;<link>/<guid>водят към страницата на договора.<link rel="alternate" type="application/rss+xml">за автоматично откриване от четците.@sigma/db:listRecentEntityContracts(преизползва споделените SELECT/FROM и row mapper-а на списъците, за да е идентичен изгледът) и лекиgetAuthorityHead/getCompanyHead(едно четене от rollup таблицата вместо пълния профилен DTO;nullогледално на 404 на HTML профила).app/lib/feed.ts) с екраниране на всяка стойност; каналниятpubDateидва от най-новия запис, не от часовника - изходът е детерминистична функция на данните (стабилен за edge кеша и тестовете).Cache-Controlкато другите публични страници (1 ч. + SWR). Фийдовете носятX-Robots-Tag: noindex- индексируемото съдържание е HTML профилът, а разпознатите ЕТ профили са умишлено noindex (Privacy: .json/.csv expose natural-person ЕИК without the noindex applied to HTML profiles #173).docs/api.md+ едно изречение в README.Допълва, не дублира, watchlist PR-овете #126/#208 - те запазват профили локално в браузъра, но нямат канал за известяване; RSS фийдът е този канал. Естествено продължение (отделен PR) са email известия върху същите фийдове.
Свързан issue
Няма пряк issue; свързано с #126/#208 (watchlist) като канал за следене.
Вид промяна
feat— нова функционалностКак е тествано
pnpm typecheck- минава (всички пакети).pnpm test- минава: 347 теста в@sigma/web(вкл. нови за builder-а: екраниране, permalink guid, RFC 822 дати, празен канал, детерминизъм) и 185 в@sigma/db(вкл. нови заlistRecentEntityContracts: scope по възложител/изпълнител, подредба, limit).pnpm lint- чисто.Чеклист
Co-Authored-By:trailermidt-bg/sigma:mainpnpm typecheckминаваpnpm test(поне за засегнатите пакети) минаваpnpm lintе чисто.env*или.dev.varsdocs/е обновена (docs/api.md, README)