feat(db,web): app-layer read-only D1 guard — getDb chokepoint над env.DB (#199)

[nikimilenkov]

Какво

App-layer read-only D1 guard за web worker-а. D1 няма native runtime read-only binding — env.DB е пълен read+write — затова least-privilege за четене се постига на app-layer: тънък read-only wrapper над env.DB, изложен от @sigma/db, през който минава целият web D1 достъп.

Как

• readonlyD1(db) (@sigma/db) — обвива D1Database така, че .prepare() / .exec() отхвърлят всичко освен четене, а .batch() / .withSession() / .dump() хвърлят грешка. Връща реалния prepared statement (чист passthrough — четенето не се променя).
• getDb(env) — единственият chokepoint, изложен от @sigma/db; целият web чете D1 само през него, никога директно през env.DB. Чиста функция, без module state, за да остане @sigma/db stateless.
• Read-only проверката — текстова и literal-aware (write verb в '…' литерал е данни, не запис). Умишлено НЕ е AST разбор: loader заявките се сглобяват динамично и node-sql-parser fail-closes на валиден SQLite, който не може да анализира — това би отхвърлило реални четения и би счупило жива страница. node-sql-parser остава само по assistant run_sql пътя.
• 27-те места в web, които ползваха env.DB директно, минаха през getDb. ETL worker-ът запазва raw binding-а — той легитимно пише в D1.

Ефект

Assistant run_sql вече минава през read-only handle — AST guard-ът в sql-guard.ts спира да е единствената носеща защита срещу запис (тройна защита: L1 текст → L2 AST → wrapper).

Тестове

• Adversarial проверка: 45 варианта на запис (CTE-DML, RETURNING, stacked, comment/case/BOM трикове, PRAGMA / ATTACH / VACUUM, load_extension) — всички отхвърлени; cross-check срещу реален sqlite: нито един statement не променя базата и не минава read-only проверката.
• Corpus guard: всеки read loader (unfiltered + напълно филтриран) се прекарва през read-only проверката → 0 false-reject.
• Chokepoint guard: CI проверка, че web няма нито едно директно env.DB. ETL guard: че ETL не ползва wrapper-а.

Обхват и граници

• App-layer мярка — raw write-capable binding-ът физически съществува; getDb ограничава достъпа на ниво код. Не е и не заменя:
  • неотменяемия per-query timeout за run_sql (AI асистент (#80) — launch-gate чеклист преди provision/изложение #83);
  • capability-level read-only binding (spec §9.4) — по-силният бъдещ слой.
• Enforcement-ът е source-grep vitest guard, а не lint правило: репото lint-ва само с prettier (няма ESLint).
• Имплементира read-only частта на assistant: run_sql must use a read-only D1 binding before the prod BgGPT key #134; част от launch-gate AI асистент (#80) — launch-gate чеклист преди provision/изложение #83.

Closes #199

[ydimitrof]

Ревю на PR #199 — feat(db,web): read-only D1 guard (getDb chokepoint над env.DB)

Забележка: Съгласно указанието не публикувам inline коментари и не задавам автоматично REQUEST_CHANGES — това е чернова за преглед преди изпращане. Целият текст е на български, както е изисквано; при нужда мога да дам и версия на английски.

Резюме

PR въвежда least-privilege chokepoint: getDb(env) обвива env.DB в ReadonlyD1, който пропуска .prepare()/.exec() само през текстов read-only предикат и блокира .batch(), .withSession(), .dump(). Всички web loader-и минават към getDb(...), а ETL запазва write-capable binding. Два теста-пазача (web chokepoint и ETL) фиксират правилото на ниво изходен код.

Промяната е малка по риск, добре мотивирана и повишава сигурността (defense-in-depth над AST guard-а на assistant run_sql).

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

• Твърдо кодирани тайни: няма.
• Промени по URL/whitelist: няма.
• Зловреден код / backdoor / обфускация / инжекция: няма. Логиката е защитна — блокира write/DDL/side-effect функции.
• Нови зависимости: няма (кодът дори избягва node-sql-parser тук умишлено).

Силни страни

• Пълно покритие на входните точки за SQL. D1Database има точно пет метода, изпълняващи SQL — prepare/exec са gated, batch/withSession/dump хвърлят. class ReadonlyD1 implements D1Database дава ценно свойство: бъдещ нов метод в D1 типа ще счупи компилацията, вместо да отвори тих write-канал.
• Устойчив предикат (readonly-sql.ts). Literal-aware: коментари, string литерали и stacked стейтмънти се обработват коректно. Покрити са реалните заобикаляния, които наивен ^(select|with) би пропуснал: CTE-префиксиран DML (WITH … DELETE), RETURNING, EXPLAIN над write, REPLACE INTO vs. скаларния replace(), side-effect функции (load_extension/writefile/readfile/fts3_tokenizer).
• Fail-closed поведение е доказано: тестът does not reach the underlying prepare() гарантира отхвърляне ПРЕДИ делегиране.
• Отлични тестове. readonly-sql.test.ts (таблици WRITES/READS с гранични случаи), readonly-d1.test.ts (passthrough + disabled методи + multi-statement exec) и особено readonly-corpus.test.ts — регресия срещу false-reject през всеки реален loader, с явна non-vacuity проверка (home_totals) срещу счупен импорт.

Находки (некритични — за уточнение, не блокиращи)

1. Регресионен риск при .batch() в assistant run_sql. Обвивката прави .batch() да хвърля напълно. Loader-ите не ползват batch (corpus тестът минава само през prepare/exec), но пътят assistant.chat.tsx → getDb(env) вече е read-only. Ако run_sql (или която и да е read логика) някога изпълнява батчове от read-only заявки през .batch(), това ще се счупи в runtime. Моля потвърдете, че assistant пътят ползва само .prepare().

2. Web workers в обхвата на chokepoint теста. readonly-db-chokepoint.test.ts включва и apps/web/workers/**. Ако някой web worker легитимно пише в D1 (scheduled/queue consumer), правилото „само getDb“ би го направило read-only и счупило. Допускам, че web слоят е read-only по дизайн (затова тестът минава), но моля потвърдете, че няма write-път от страна на web.

3. Двойни кавички при идентификатори (нисък приоритет). stripStringLiterals изчиства само '…'. Идентификатор в двойни кавички, съдържащ write-verb (напр. SELECT "DELETE" FROM t), би дал false-reject. Реалните loader-и не ползват такива идентификатори (corpus тестът пази), затова е теоретично; струва си бележка при бъдещи заявки.

4. Дублиране на логиката за скан на низове/коментари (поддръжка). Коментарите отбелязват, че обработката „mirrors apps/web assistant sql-guard.ts“. Двете реализации моделират string/comment-и независимо; при разминаване двата защитни слоя биха се разошли. Обмислете изнасяне на споделените помощници (stripComments/splitStatements) в @sigma/shared, за да не дрейфват. Разбираемо е, че packages/db не бива да зависи от apps/web — затова споделеният пакет е правилното място.

OWASP / съответствие

• A03 Injection: SQL входните точки са централизирани и gated; параметризацията през .bind() е запазена; текстовият предикат добавя слой над съществуващия guard. Съответства.
• A01/A04 (least privilege / secure design): точно това реализира PR-ът. Съответства.
• Няма промени по автентикация, крипто, десериализация или логване, които да въвеждат нови рискове.

Оценки по критериите

• Сигурност (Phase 0 + agent-level): чисто, без критични уязвимости.
• Тестове: силни, целенасочени, без „cheater“ тестове; покриват реални заобикаляния и false-reject регресии.
• Качество/патърни: последователно; чисто прилагане на един и същ chokepoint из целия web слой.
• Производителност: пренебрежима (една алокация на извикване; няма proxy на горещия .bind/.all/.first път).
• Документация: изчерпателни code-коментари; няма нужда от външна документация за вътрешна инфраструктура.

Композитна оценка: ~9.3/10. Няма блокиращи проблеми със сигурността или коректността в предоставения diff. Оставащото е потвърждение по т.1 и т.2 (не могат да се проверят само от diff-а).

ВЕРДИКТ: COMMENT — препоръка за APPROVE след потвърждение по въпроси 1 и 2; без блокиращи проблеми със сигурността.

[lyubomir-bozhinov]

Силна работа — chokepoint-ът е реален и добре затворен: целият web D1 достъп минава през getDb/readonlyD1, а readonly-db-chokepoint.test.ts глобва целия web source и assert-ва нула сурови env.DB (единственият остатък е ETL, който легитимно пише). Write-rejection корпусът е пълен, асистентът е коректно двойно-guard-нат, нито един read път не се чупи, тестовете са истински.

Един реален дефект в класификатора (Medium — не е live-exploitable днес, но чупи собствената гаранция на кода):

packages/db/src/readonly-sql.ts:44-62 — stripComments/string-стриппърът следят state само за '…', но не за "…" / `…` / […] quoted identifiers. Затова ' вътре в идентификатор с двойни кавички отваря фантомен string и изтрива write-глагола преди WRITE_VERB_RE. Емпирично (проследено на HEAD):

• WITH t("x'") AS (SELECT 1) DELETE FROM secrets WHERE x='y' → класифицира се като read-only, а SQLite изпълнява DELETE-а ("x'" за SQLite е идентификатор, не string).
• същото с `…` и […]; .exec вариантът крие stacked write зад -- вътре в "…".

Защо не е live днес: web .prepare() ползва статичен SQL (атакуващият контролира само bound стойности), а асистентският run_sql има свой node-sql-parser AST guard, който независимо отхвърля DELETE-а. Но коментарът в кода твърди, че wrapper-ът „closes every write entry point even if the assistant run_sql AST guard is bypassed“ — точно тази completeness гаранция не е вярна, а целият смисъл на #225 е да е независим backstop.

Fix (евтин, без read регресия — нито един loader не ползва quoted identifiers): накарай скенера да consume-ва и четирите вида кавички (""/ `` escape; [...] няма escape) в `stripComments`/`splitStatements`/`stripStringLiterals`, и добави трите bypass стринга в `readonly-sql.test.ts` като регресия. Тогава completeness claim-ът става верен.

[lyubomir-bozhinov]

Одобрявам на връх 6a6c1e1 — снемам предишния CHANGES_REQUESTED. Единственият дефект от ревюто (quoted-identifier bypass в класификатора) е затворен коректно; проверих го наново на HEAD:

• readonly-sql.ts вече проследява и четирите SQLite quoted региона през QUOTE_CLOSE = { ' , " , \ , [ }—'…'string и"…"/`` … ``/[…]идентификатори — с правилната escape семантика (удвояване за''/""/`` `` ``; […]без escape, край при първото]). Тъй 'вътре в"x'"е идентификаторен символ, не отваря фантомен string, и write-глаголът вече не се изтрива предиWRITE_VERB_RE`.
• Тестовият корпус (44 случая) носи точните bypass низове, които подадох — WITH t("x'") AS (SELECT 1) DELETE …, същото с `x'` и [x'] — с коментар „#225 review bypass", плюс добавени side-effect функции (load_extension/writefile/readfile/fts3_tokenizer), което е над поисканото.

С това completeness гаранцията на wrapper-а („закрива всеки write път, дори при заобиколен AST guard") става вярна — точно смисълът на #225 като независим backstop. Одобрено от моя страна; mergeable_state може още да е blocked на required CI/branch-protection (извън ревюто ми).

[cefothe]

🔍 Ревю на PR #225 — read-only D1 wrapper + getDb chokepoint

Резултат: APPROVE (9.5/10). Чиста, добре тествана defense-in-depth мярка. Нула blocking проблема.

Какво е прегледано

Security-scan (Phase 0): чист — няма hardcoded secrets, няма нови зависимости, няма URL промени (sitemap-ите ползват request.origin, https://sigma.bg е само в тестове), няма съмнителни patterns. 31-те env.DB → getDb(env) замени са механични и коректни.

Проследена е реалната верига на защита за run_sql (apps/web/app/lib/assistant/tools.ts):
L1 текстов guard (assertReadOnlySelect) → L2 AST guard (guardSelect) → L3 wrapper (ctx.db.prepare() вече минава през ReadonlyD1.assertReadOnly). Тройната защита от описанието е реална.

Силни страни

• readonly-sql.ts е коректен. Единственият начин за запис е write verb ИЗВЪН quoted регион — а той винаги се хваща от WRITE_VERB_RE след stripQuoted. Проследяването и на четирите SQLite quote региона ('…', "…", `…`, […]) с правилния escape (удвояване, а […] без escape) затваря identifier-quote bypass-а — SQLite не изпълнява write verb вътре в quoted регион, така че blank-натото от stripQuoted съвпада с това, което базата третира като данни. Unterminated quote → fail-safe (SQLite и без това хвърля syntax error).
• CTE-DML, RETURNING, stacked, EXPLAIN <write>, REPLACE INTO, comment/case/BOM трикове — всички покрити и коректно отхвърлени.
• Тестовете са отлични: adversarial corpus + non-vacuity guard (home_totals), read-loader corpus срещу 0 false-reject (unfiltered + напълно филтриран param set), CI chokepoint guard и ETL guard.
• Архитектурата е чиста: getDb е stateless pure функция, hot path (.bind/.all/.first) остава без proxy — връща реалния prepared statement. ETL коректно запазва raw binding-а.

Незадължителни бележки (low, non-blocking)

1. readonly-db-chokepoint.test.ts — обхват на glob. Сканира само apps/web/app/** и apps/web/workers/**. Файлове извън тях (напр. бъдещ server entry в apps/web/*.ts) няма да бъдат хванати. Освен това regex-ът лови само литералния env.DB — деструктуриране (const { DB } = env) или env['DB'] биха заобиколили guard-а. Приемливо за defense-in-depth, но си струва да се отбележи.
2. Няма тест, който гарантира, че read loader-ите не ползват .batch()/.withSession()/.dump(). Проверих ръчно — .batch() е само в packages/ingest (ETL), така че днес няма runtime риск. Но corpus тестът swallow-ва rejection-и, така че бъдещ loader с .batch() би се счупил тихо, без тестът да го хване. Обмислете отделен assert.
3. Дребно: описанието казва „45 варианта на запис", а committed adversarial таблицата има ~30 реда (вероятно броят се разпределя през няколко test файла) — несъществено.

Заключение

CLAUDE.md compliance: няма partial impl, няма TODO/dead code, тестове заедно с кода, чисто разделяне на отговорностите. Готово за merge след (по желание) бележки 1–2.

[ydimitrof]

Ревю на PR #199 — „app-layer read-only D1 guard (getDb chokepoint над env.DB)“

Забележка: анализът е извършен само върху дифа (целевото repo @sigma/db не е налично локално). Не са публикувани inline коментари — по твоя изрична молба изчаквам одобрение. По-долу има драфт на български и на английски, за да избереш кой да изпратиш.

---

ВЕРДИКТ: COMMENT — силна, добре тествана реализация; няма блокиращи проблеми по сигурността. 3 некритични бележки за проверка преди merge.

---

Фаза 0 — Security-Critical Scan (задължителна)

• Хардкоднати тайни: НЯМА.
• Промени по URL / whitelist: НЯМА.
• Злонамерени шаблони (backdoor, code injection, обфускация): НЯМА.
• Нови/променени зависимости: НЯМА.
• Резултат: CLEAN — продължавам към същинското ревю.

Оценка по същество

Целта на PR-а (least-privilege chokepoint: web чете D1 само през getDb → readonlyD1, а суровият write-capable env.DB остава само за ETL) е постигната последователно. Дизайнът е „deny-by-default“: изявлението трябва да започва със SELECT/WITH/EXPLAIN И да няма write-глагол извън кавички — това прави фалшивите одобрения (пропуснат write) практически невъзможни. Токенайзерът в readonly-sql.ts коректно проследява и четирите SQLite quoted региона ('…', "…", `…`, […]), включително удвоените delimiter-escape-и, и затваря bypass-а с кавичка вътре в идентификатор (#225). .batch()/.withSession()/.dump() са fail-closed, а implements D1Database гарантира, че нов write-метод от Cloudflare ще счупи компилацията, вместо да отвори тиха вратичка. Разсъждението „SQL е фиксиран на prepare(), bind() подава само стойности“ е вярно — връщането на суровия D1PreparedStatement не отваря write-път.

OWASP: входната валидация (A03 Injection) е адекватна и defense-in-depth (AST guard на assistant-а + този wrapper). Fail-closed поведение при отхвърляне (не достига до делегиране). Съответства.

Тестове: много добро покритие — readonly-sql.test.ts таргетира именно bypass-ите (CTE-prefixed DML, RETURNING, stacked, comment/whitespace, quoted-identifier), а readonly-corpus.test.ts прокарва ВСИЧКИ реални read-loader-и (включително филтрирани варианти) и доказва липса на false-reject + non-vacuity. Enforcement тестовете (readonly-db-chokepoint, readonly-guard) заместват липсващо lint правило.

Некритични бележки (за проверка, не блокиращи)

1. Покритие на chokepoint теста. import.meta.glob в readonly-db-chokepoint.test.ts сканира само app/** и ../../workers/**. Ако в apps/web има D1 достъп извън тези две директории (напр. root-level server entry / load-context / functions), той няма да бъде уловен. Моля потвърдете, че целият D1 достъп на web е в обхвата.
2. Регексът лови само литерал env.DB. Деструктуриране (const { DB } = context.cloudflare.env) би заобиколило guard-а и теста. Не е дефект в този PR, но си струва бележка за бъдещето.
3. Интеграция wrapper × реални loader-и. readonly-corpus.test.ts валидира предиката, но подава суров capturing-fake, не readonlyD1(...). Ако някой read-loader използва .batch() за четене, getDb ще го счупи в runtime и никой тест няма да го хване. Моля потвърдете, че никой web read-loader не вика .batch()/.withSession().

Оценка: ~9.3/10. Препоръка: одобрение след потвърждение по т.1–3 и зелени CI тестове.

---

---

ЧЕРНОВА ЗА ПУБЛИКУВАНЕ (Български)

Благодаря за много прегледната работа по този chokepoint. 🙌

Вердикт: Коментар (не блокиращо) — силна реализация по сигурността, няма намерени пробойни.

Предикатът isReadOnlySql е „deny-by-default“ (изисква водещ SELECT/WITH/EXPLAIN + липса на write-глагол извън кавички) и токенайзерът коректно покрива и четирите SQLite quoted региона, включително escape-ите и bypass-а от #225. Блокирането на batch/withSession/dump и implements D1Database затварят и останалите write-пътища. Тестовете таргетират реалните bypass-и и доказват липса на false-reject върху всички loader-и.

Три некритични въпроса за потвърждение преди merge:

1. import.meta.glob в chokepoint теста покрива app/** и workers/** — има ли D1 достъп извън тях в apps/web?
2. Тестът лови само литерал env.DB; деструктуриране const { DB } = env би го заобиколило (бележка за бъдещето).
3. Corpus тестът валидира предиката, но не и wrapper-а спрямо реалните loader-и — потвърдете, че никой read-loader не ползва .batch().

Благодаря!

---

DRAFT FOR POSTING (English)

Thanks for the very thorough work on this chokepoint. 🙌

Verdict: Comment (non-blocking) — strong security implementation, no bypasses found.

The isReadOnlySql predicate is deny-by-default (requires a leading SELECT/WITH/EXPLAIN and no write verb outside quotes), and the tokenizer faithfully tracks all four SQLite quoted regions, including doubled-delimiter escapes and the #225 identifier-quote bypass. Blocking batch/withSession/dump plus implements D1Database closes the remaining write paths. The tests target the real bypasses and prove no false-reject across every loader.

Three non-blocking items to confirm before merge:

1. The chokepoint test's import.meta.glob covers app/** and workers/** — is there any D1 access in apps/web outside those paths?
2. The test only catches the literal env.DB; a destructured const { DB } = env would slip past both the test and the guard (future-proofing note).
3. The corpus test validates the predicate but not the wrapper against real loaders — please confirm no read-loader uses .batch().

Thanks!

[nikimilenkov]

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

1. Покритие на chokepoint теста — тестът вече сканира цялата apps/web/** рекурсивно (не само app/ + workers/), с изключени build/ и node_modules. Така бъдеща нова директория не може да вкара D1 достъп извън обхвата.

2. Само литерал env.DB — добавих проверка и за деструктуриране (const/let/var { DB } = …env), която залавя и const { DB } = context.cloudflare.env.

3. Corpus vs. реални loader-и — добавих защита, която забранява .batch()/.withSession()/.dump() в изходния код на web слоя и на @sigma/db заявките — методите, на които getDb хвърля. Така нито един read-loader не може да ги достигне.

По въпросите за потвърждение от по-ранното ревю: web пътят (вкл. assistant.chat.tsx / run_sql) минава само през .prepare()/.exec() — новата защита намира нула извиквания на .batch/.withSession/.dump, така че „web е read-only" вече е доказано, не само допуснато.

Оставих настрана предложението за изнасяне на stripComments/splitStatements в @sigma/shared — това е презпакетен рефактор, който засяга и assistant sql-guard.ts; предлагам да е отделен issue, за да остане PR-ът един логически change.

Всички проверки са успешни (typecheck, prettier, тестове).

[ydimitrof]

Ревю на PR #199 — app-layer read-only D1 guard (getDb chokepoint над env.DB)

Забележка: подготвени са два варианта (български и английски) — избери който да изпратиш. Не са публикувани inline коментари, за да можеш първо да провериш заключенията. Verdict-ът е на отделен ред.

---

🇧🇬 Български вариант

Здравейте и благодаря за много добре структурирания PR. Направих задълбочен преглед с приоритет върху сигурността и целостта на данните (SQL инжекции, заобикаляне на guard-а, OWASP). Ето обобщението.

Фаза 0 — Security-critical сканиране: ЧИСТО

• Няма hardcoded тайни (API ключове, пароли, токени).
• Няма промени по URL/whitelist.
• Няма нови зависимости — предикатът е нативен, без външни пакети.
• Няма backdoor/obfuscation/code-injection модели.

Съответствие с issue #199: Реализацията отговаря на описанието — въвежда се least-privilege chokepoint (getDb), който обвива write-capable env.DB в read-only обвивка за web runtime, докато ETL worker-ът съзнателно запазва суровия binding. Това е защита в дълбочина: дори при заобикаляне на AST guard-а по run_sql пътя на асистента, записът в D1 остава невъзможен.

Оценка на SQL предиката (readonly-sql.ts) — силна страна на PR-а.
Прегледах класическите вектори за заобикаляне и всички са покрити:

• CTE-префиксиран DML (WITH … DELETE/UPDATE/INSERT/REPLACE) — отхвърля се.
• INSERT … RETURNING, stacked statements (SELECT 1; DROP …) — отхвърлят се (изисква се точно един statement).
• Инжекция през коментари (--, /* */) — quote-aware премахване.
• Скриване на write чрез кавичка в идентификатор ("x'", `x'`, [x']) — това е тънкият байпас от ревю #225 и е коректно затворен: проследяват се и четирите SQLite quoted региона.
• Странични функции без write-глагол (load_extension, writefile, readfile, fts3_tokenizer) — блокирани.
• pragma_table_info(...) (read table-valued form) коректно не се блокира заради \b, докато PRAGMA table_info се блокира — правилно разграничение.
• EXPLAIN <write> се отхвърля, защото WRITE_VERB се проверява върху целия statement.

Оценка по gate-ове:

• Сигурност (Phase 0 + agent-level): ✅ покрито, OWASP A03 (Injection) адресирано с fail-closed поведение.
• Тестове: ✅ обширни — предикатни таблици за writes/reads, corpus regression срещу false-reject, chokepoint enforcement скенери. Много добра защита от false-reject (който би 500-нал жив loader).
• Код/архитектура: ✅ чист chokepoint, implements D1Database гарантира покриване на всички методи; .batch/.withSession/.dump са fail-closed.
• Консистентност: ✅ всички web loader-и минаха от env.DB към getDb(...).

Наблюдения (незадължителни, не блокират):

1. Text-based blocklist може теоретично да false-reject-не легитимен read, който селектира некотиран идентификатор, съвпадащ с write-глагол (напр. колона/алиас analyze, rename, trigger, merge). Текущите loader-и са покрити от corpus теста, но бъдещ loader с такова име би получил 500 — струва си кратък коментар в кода или warning.
2. getDb алокира нова обвивка при всяко извикване (3× в contracts.tsx) — пренебрежимо, но може да се отбележи.
3. Асистентският път вече е двойно защитен (AST guard + readonlyD1). Ако някой read-пат на асистента ползва .batch() за батчване на четения, вече ще хвърля — добре е да се потвърди, че няма такъв случай.
4. capturingDb връща празни редове, затова loader-и с условна втора заявка може да не изпълнят всичките си statements — покритието на corpus теста може да е леко непълно (само бележка по тестовете).

Заключение / Verdict:
APPROVE — качество над прага; препоръчвам да се адресира само наблюдение №1 (коментар/бележка), останалите са опционални.

---

🇬🇧 English version

Hi, and thanks for a very well-structured PR. I did a thorough review prioritising security and data integrity (SQL injection, guard bypasses, OWASP). Summary below.

Phase 0 — security-critical scan: CLEAN

• No hardcoded secrets, no URL/whitelist changes, no new dependencies (native predicate), no backdoor/obfuscation/injection patterns.

Alignment with issue #199: The implementation matches the description — a least-privilege getDb chokepoint wraps the write-capable env.DB in a read-only view for the web runtime, while the ETL worker deliberately keeps the raw binding. This is defense-in-depth: even if the assistant run_sql AST guard is bypassed, a D1 write is unreachable.

SQL predicate (readonly-sql.ts) — the strong point. I checked the classic bypass vectors and all are covered:

• CTE-prefixed DML (WITH … DELETE/UPDATE/INSERT/REPLACE) — rejected.
• INSERT … RETURNING, stacked statements — rejected (exactly one statement required).
• Comment injection (--, /* */) — quote-aware stripping.
• Write hidden by a quote inside an identifier ("x'", `x'`, [x']) — the #225 bypass, correctly closed by tracking all four SQLite quoted regions.
• Side-effect functions with no write verb (load_extension, writefile, readfile, fts3_tokenizer) — blocked.
• pragma_table_info(...) read form correctly allowed via \b, while PRAGMA table_info is blocked — correct distinction.
• EXPLAIN <write> rejected (verb checked over the whole statement).

Gates: Security ✅ (OWASP A03 Injection, fail-closed). Tests ✅ extensive (write/read predicate tables, false-reject corpus regression, chokepoint enforcement scanners). Code/architecture ✅ clean chokepoint, implements D1Database guarantees full method coverage, .batch/.withSession/.dump fail-closed. Consistency ✅ all web loaders moved from env.DB to getDb(...).

Non-blocking observations:

1. The text blocklist could in theory false-reject a legitimate read that selects an unquoted identifier equal to a write verb (e.g. a column/alias named analyze, rename, trigger, merge). Current loaders are covered by the corpus test, but a future loader with such a name would 500 — worth a code note/warning.
2. getDb allocates a fresh wrapper per call (3× in contracts.tsx) — negligible.
3. The assistant path is now double-guarded (AST + readonlyD1); confirm no assistant read path relies on .batch().
4. capturingDb returns empty rows, so loaders with a conditional second query may not emit all statements — corpus SQL coverage may be slightly incomplete (test-thoroughness note only).

Verdict:
APPROVE — quality is above the gate; I'd only address observation #1 (a note/comment); the rest are optional.

[lyubomir-bozhinov]

Одобрявам наново на връх c911727 — потвърждавам APPROVE-а (предишният ми беше на 6a6c1e1). Делтата 6a6c1e1…c911727 е чисто тестова и затваря и трите конкретни бележки на @cefothe:

1. Обхват на chokepoint скенера. Глобът вече е ../../**/*.{ts,tsx} (целият apps/web, с изключено /build/), а не само app/** + workers/** — нищо в apps/web не остава извън обхвата.
2. Деструктуриране. Нов тест лови const { DB } = …env — точно случаят, който би се промъкнал покрай литералния env.DB скан.
3. Runtime-блокирани методи. .batch()/.withSession()/.dump() вече са забранени и в web source, и в packages/db query source (chokepoint + corpus) — read-loader, който посегне към някой от тях, се улавя статично, а не чак в runtime.

Тестовете са истински (assert-ват празен offender-списък срещу реалния, comment-stripped source), не котешки.

Дребно, не блокира: скенерът остава евристичен backstop — трите регекса ловят env.DB (точка) и деструктуриране, но не и bracket-достъп env['DB'], който връща същия суров write-capable handle. Не е повдигнато в ревюто и не е идиоматично, а реалната гаранция е getDb + implements D1Database; ако решите — тривиално е да се добави env\[\s*['\"]DB към offender-регекса. Одобрението стои; mergeable_state може още да е blocked на required CI (извън ревюто ми).

[ydimitrof]

Преглед на PR #199 — app-layer read-only D1 guard (getDb chokepoint над env.DB)

ВЕРДИКТ: APPROVE (с незадължителни бележки) — не са открити уязвимости за сигурност или данни; изисква вашето финално потвърждение преди изпращане.

---

🇧🇬 Български вариант (за изпращане)

Здравейте! Направих задълбочен преглед с акцент върху сигурност и цялост на данните. Това е PR за подсилване на сигурността и е с много високо качество.

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

• Няма hardcoded тайни (ключове/пароли/токени).
• Няма промени по продукционни URL адреси (https://sigma.bg е само в тестови fixtures).
• Няма нови/променени зависимости (package.json не е пипан).
• Няма злонамерени шаблони — напротив, това е least-privilege chokepoint.

Анализ на ядрото (isReadOnlySql) — не е открит байпас:
Дизайнът е стабилен: allow-list за водещата ключова дума (SELECT/WITH/EXPLAIN) + deny-list за write-глаголи, разпознати само ИЗВЪН цитирани региони, с проследяване и на четирите вида SQLite кавички ('…', "…", `…`, […]). Проследих ръчно всички класове байпас:

• CTE-DML (WITH … DELETE/UPDATE/INSERT/REPLACE) — прихваща се. ✅
• INSERT … RETURNING — отхвърля се от водещата проверка. ✅
• Наредени заявки (SELECT 1; DROP …) — отхвърлят се (!== 1 statement). ✅
• Скрит write чрез коментар/whitespace/EXPLAIN — прихваща се. ✅
• Байпас с кавичка в идентификатор ("x'", `x'`, [x']) от ревю #225 — покрит. ✅
• Side-effect функции (load_extension/writefile/readfile/fts3_tokenizer) — блокирани. ✅
• Fail-closed: при отхвърляне не се достига до реалния prepare() (доказано с тест).

OWASP: least-privilege, defense-in-depth (guard-ът е втора преграда след AST guard-а на run_sql, а не единствената), fail-closed поведение и валидация на вход. Съответства.

Тестове: отлични — corpus тест срещу всички реални read-loader-и (без false-reject), byte-level byпас тестове, passthrough/exec/batch тестове, ETL guard и chokepoint enforcement тестове. Рефакторът на ~30 файла (env.DB → getDb(env)) е механичен, последователен и защитен от регресия чрез статичните enforcement тестове.

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

1. withSession() хвърля грешка — това затваря вратата към D1 Sessions API за read-replica маршрутизиране. За четящо-интензивно уеб приложение репликите за четене са естественият път за скалиране. Днес няма регресия (не се използва), но заслужава осъзнато решение/бележка за бъдещето.
2. Латентен риск от false-reject: списъкът WRITE_VERBS включва и не-SQLite глаголи (MERGE, UPSERT, GRANT, REVOKE, RENAME, TRUNCATE, TRIGGER). Бъдеща колона/alias с такова име (напр. SELECT trigger FROM …) ще предизвика 500 на жив loader. Днес corpus тестът го покрива, но помислете за коментар, документиращ това ограничение, или отрязване на неприложимите глаголи.
3. contracts.tsx извиква getDb(env) три пъти в един loader — за яснота го присвоете веднъж (const db = getDb(env)).

Оценка: ~9.3/10. Одобрявам след ваша проверка. Благодаря за задълбочената работа по тестовете! 🙏

---

🇬🇧 English version (for sending)

Hi! I did a thorough review focused on security and data integrity. This is a security-hardening PR and it is very high quality.

Phase 0 — security-critical scan: CLEAN

• No hardcoded secrets (keys/passwords/tokens).
• No production URL changes (https://sigma.bg appears only in test fixtures).
• No new/changed dependencies (package.json untouched).
• No malicious patterns — this is a least-privilege chokepoint.

Core analysis (isReadOnlySql) — no bypass found:
The design is sound: allow-list on the leading keyword (SELECT/WITH/EXPLAIN) + deny-list of write verbs matched only OUTSIDE quoted regions, with tracking of all four SQLite quote kinds ('…', "…", `…`, […]). I manually traced every bypass class:

• CTE-DML (WITH … DELETE/UPDATE/INSERT/REPLACE) — caught. ✅
• INSERT … RETURNING — rejected by the leading-keyword gate. ✅
• Stacked statements (SELECT 1; DROP …) — rejected (!== 1 statement). ✅
• Write hidden via comment/whitespace/EXPLAIN — caught. ✅
• Identifier-quote bypass ("x'", `x'`, [x']) from review #225 — covered. ✅
• Side-effect functions (load_extension/writefile/readfile/fts3_tokenizer) — blocked. ✅
• Fail-closed: on rejection the real prepare() is never reached (proven by test).

OWASP: least-privilege, defense-in-depth (this guard is a second barrier behind the run_sql AST guard, not the only one), fail-closed behavior and input validation. Compliant.

Tests: excellent — a corpus test driving every real read loader (no false-reject), byte-level bypass tests, passthrough/exec/batch tests, plus ETL-guard and chokepoint enforcement tests. The ~30-file refactor (env.DB → getDb(env)) is mechanical, consistent, and regression-guarded by the static enforcement tests.

Optional notes (non-blocking):

1. withSession() throws — this forecloses the D1 Sessions API for read-replica routing. For a read-heavy web app, read replicas are the natural scaling path. No regression today (it's unused), but worth a conscious decision/note for the future.
2. Latent false-reject risk: WRITE_VERBS includes non-SQLite verbs (MERGE, UPSERT, GRANT, REVOKE, RENAME, TRUNCATE, TRIGGER). A future column/alias with such a name (e.g. SELECT trigger FROM …) would 500 a live loader. The corpus test covers it today, but consider a comment documenting the constraint, or trimming the inapplicable verbs.
3. contracts.tsx calls getDb(env) three times in one loader — assign it once (const db = getDb(env)) for clarity.

Score: ~9.3/10. Approving pending your verification. Thanks for the excellent test work! 🙏

---

Забележка: не публикувам inline коментари и не задавам автоматично REQUEST_CHANGES/APPROVE — оставям решението на вас, както поискахте. (No inline comments posted; final decision left to you as requested.)

[lyubomir-bozhinov]

Главата мръдна на 92f8e5a след одобрението ми на c911727; препроверих делтата, защото пипна guard-файла — чиста е, одобрението стои.

• readonly-sql.ts (+4/-0): само коментар — обяснява, че WRITE_VERBS е нарочно широк (MERGE/UPSERT/GRANT/…/TRIGGER като forward-proofing) и признава теоретичния false-reject при колона/alias с такова име, изключен от read-loader corpus теста. Масивът WRITE_VERBS и whole-word-outside-quotes matching-логиката са непроменени — quote-bypass fix-ът (c2e9a31, „…"/`…`/[…]) стои.
• readonly-d1.ts (+3/-0): само коментар — withSession() хвърля нарочно (foreclose-ва D1 Sessions/replica routing) като least-privilege избор.
• contracts.tsx: hoist на getDb(env) в една променлива (беше 3 повиквания) — все още минава през read-only chokepoint-а, без промяна в enforcement-а.
• 26e0f77: само typecheck на guard теста.

Логиката на read-only предиката е байт-непроменена спрямо одобрената. Одобрявам.

[ydimitrof]

Ревю на PR #199 — app-layer read-only D1 guard (getDb chokepoint над env.DB)

ВЕРДИКТ: КОМЕНТАР — препоръчвам одобрение след ръчна проверка; не блокиращи находки. (Не публикувам коментари по твое искане — това е чернова за преглед.)

---

Фаза 0 — Сканиране за критична сигурност (задължително)

• Твърдо кодирани тайни: няма (0 API ключове/пароли/токени).
• Промени по URL адреси: няма. Единствените стрингове с URL са тестови (https://sigma.bg, https://sigma.org) в корпус-тестове — не са продукционни.
• Зловреден код (backdoor/инжекция/обфускация): няма. Промяната всъщност намалява повърхността на атака.
• Зависимости: няма нови пакети. Тестовете за корпуса умишлено НЕ въвеждат node-sql-parser в web (обяснено), използват само node:fs/vitest.
• Резултат: ЧИСТО — продължавам към ревюто.

OWASP / SQL инжекция — задълбочен анализ на readonly-sql.ts

Предикатът isReadOnlySql е текстов, но е стриктно quote-aware и издържа на класическите заобикаляния:

• CTE-префиксирано DML (WITH … DELETE/UPDATE/INSERT/REPLACE) — хваща се (write verb извън кавички).
• INSERT … RETURNING — отхвърля се (не води със SELECT/WITH/EXPLAIN).
• Стакнати заявки (SELECT 1; DROP …) — splitStatements изисква точно 1 стейтмънт.
• EXPLAIN <write> — хваща се от write-verb проверката.
• Коментари преди write (-- …\nDELETE, /* */ DELETE) — коректно се премахват.
• Заобикалянето от ревю #225 (кавичка вътре в идентификатор "x'", `x'`, [x'], който да отвори фалшив стринг и да скрие DELETE) — коректно затворено: следят се и четирите quoted-региона на SQLite, [...] без escape, а '/"/` с doubled-delimiter escape.
• Разцепване на ключова дума чрез коментар (DEL/**/ETE) — не е bypass, защото SQLite също третира коментара като разделител (двата подхода са последователни).
• Функции със странични ефекти без write-глагол (load_extension, writefile, readfile, fts3_tokenizer) — блокирани (defense-in-depth; на Cloudflare D1 и без това не са достъпни).

Обвивката ReadonlyD1 затваря всички входни точки на D1Database интерфейса: prepare/exec минават през предиката, а batch/withSession/dump хвърлят. Тъй като SQL е фиксиран в prepare() и bind() подава само стойности, gating-ът върху prepare/exec наистина покрива всеки път за запис. Fail-closed поведение (тестът потвърждава, че при отхвърляне не се стига до underlying prepare()).

Заключение по сигурност: солидна least-privilege защита в дълбочина. Предикатът е коректен по OWASP за целта (предотвратяване на запис/инжекция на мутиращ SQL).

Тестове (покритие)

Отлично. Регресионният корпус (readonly-corpus.test.ts) прекарва всеки реален read-loader (вкл. филтрирани и нефилтрирани варианти на динамичните списъци и източваните CSV/sitemap стриймъри) през capturing fake D1 и доказва, че нито един не издава не-read-only SQL — предпазва от false-reject, който би дал 500 на жива страница. Проверка за не-вакуумност (home_totals) е налична. Позитивни/негативни таблици в readonly-sql.test.ts са смислени, а не тривиални. Chokepoint-тестовете (readonly-db-chokepoint.test.ts, ETL guard) enforce-ват архитектурата вместо lint правило.

Качество на кода / архитектура

• Единна точка (getDb) заменя всички директни context.cloudflare.env.DB в web — последователно и атомарно.
• Няма частична имплементация, TODO, дублиране или мъртъв код.
• ETL умишлено запазва write-capable binding — тестът го гарантира. Ясно разделяне на отговорностите.
• Коментарите обясняват „защо", вкл. компромиса с D1 Sessions/read-replica.

Дребни, НЕ-блокиращи бележки (по избор)

1. readonly-db-chokepoint.test.ts хваща env.DB и деструктуриране, но не и bracket-достъп (env['DB']). Няма такъв в кода днес; ако искаш пълна херметичност на guard-теста, добави и този шаблон.
2. exec() е типизиран като Promise<D1ExecResult>, но assertReadOnlyExec хвърля синхронно. Тестовете разчитат на това (и е ок), но извикващ, който очаква rejected promise (.catch), няма да улови грешката. Съзнателно и покрито — само за сведение.
3. getDb алокира нова обвивка при всяко извикване (документирано като „pure, no cache") — пренебрежимо.
4. WRITE_VERBS е нарочно широк (MERGE/GRANT/RENAME/TRIGGER и др.) — теоретичен false-reject, ако бъдеща колона/alias се казва така; корпус-тестът покрива всички текущи заявки, така че на практика е не-проблем.

Съответствие с CLAUDE.md / готовност за деплой

Всички абсолютни правила спазени. Няма счупващи промени за main (web е read-only по дизайн). Няма нужда от миграции. Нисък риск за rollback (чиста, изолирана промяна).

Оценка: ~9.5/10. Силна, добре тествана промяна за сигурност. Препоръчвам одобрение след твоята ръчна проверка на находките по-горе.

[nikimilenkov]

Благодаря за двата прегледа (07-17 и 07-18) — оценявам ръчното проследяване на всеки вид заобикаляне. По бележките:

Вече включено (92f8e5a, прегледано и одобрено от @lyubomir-bozhinov на 07-17):

• withSession() хвърля нарочно → добавен коментар, че това е съзнателен least-privilege избор (foreclose-ва D1 Sessions / read-replica routing).
• WRITE_VERBS е нарочно широк (MERGE/UPSERT/GRANT/…/TRIGGER) → коментар, който документира forward-proofing-а и признава теоретичния false-reject при бъдеща колона/alias с такова име (покрит от corpus теста днес).
• contracts.tsx — getDb(env) вече е hoist-нат в един const db (беше 3 повиквания).

Новото (07-18) — приех т.1, добавих го в 7ac00b1:

• Bracket-достъп env['DB'] / env["DB"] — readonly-db-chokepoint.test.ts вече има четвърта проверка за него (/\benv\s*\[\s*['"]DB['"]\s*\]/). Няма такъв достъп в кода днес, но затваря пролуката в самия guard-тест — от същия вид като вече покритите env.DB и деструктурирането. Валидирах, че шаблонът лови context.cloudflare.env['DB']/["DB"] и НЕ дава false-positive на getDb(env) или env.DB. Тестът минава (4/4), typecheck 0, prettier чист.

Останалите (не-блокиращи, оставям както са — за протокола):

• exec() е типизиран Promise<D1ExecResult>, но assertReadOnlyExec хвърля синхронно — съзнателно и покрито от теста. Всъщност web изобщо не вика .exec() (само .prepare()); методът е gate-нат само за пълнота на обвивката, тъй че несъвместимостта с .catch е чисто теоретична — няма реален call site. Оставям типа както е (съвпада с D1Database).
• getDb алокира нова обвивка при всяко извикване — документирано като „pure, no cache"; пренебрежимо (една алокация, без proxy на горещия .bind/.all/.first път).

@ydimitrof — по твоята бележка за WRITE_VERBS: засега ги оставям широки, с коментара, който документира компромиса (forward-proofing — bulk write през D1 един ден не е изключено). Ако прецениш, че е по-чисто да отрежа не-SQLite глаголите, ще го направя — само кажи.

[ydimitrof]

Преглед на PR #199 — app-layer read-only D1 guard (getDb chokepoint над env.DB)

ВЕРДИКТ: COMMENT — не блокиращо; препоръчвам одобрение след ваша проверка (композитен резултат ~9.5/10). Не са публикувани inline коментари по ваше желание.

---

Резюме (Български)

Промяната въвежда least-privilege chokepoint: web runtime достъпва D1 само през getDb(env), който връща read-only обвивка (readonlyD1) над env.DB. ETL работникът задържа write-способния binding. Това е defense-in-depth — дори при пробив на assistant run_sql AST guard-а, web не може да пише в D1.

Сигурност (Phase 0 — CHECK, ЧИСТО):

• Няма hardcoded тайни, няма нови/променени URL-и, няма нови зависимости, няма обфускация/backdoor.
• Проследих readonly-sql.ts за заобикаляния: CTE-префиксиран DML (WITH … DELETE), RETURNING, стекнати заявки, EXPLAIN <write>, коментар-/кавичен-smuggling, скриване на write чрез кавичка в идентификатор ("x'"), опасни функции (load_extension/writefile/readfile/fts3_tokenizer). Всички се отхвърлят коректно.
• Проверих специфики за SQLite/D1: SELECT … INTO не се поддържа в SQLite (не е път за запис), pragma_* table-valued функции коректно НЕ се хващат от \bPRAGMA\b (без false-reject), created_at/updated_at минават заради word-boundary. Всичките три write-способни метода (batch/withSession/dump) хвърлят.
• OWASP: покрива A03 (Injection) чрез fail-closed предикат + параметризирани заявки; A01 (Broken Access Control) чрез least-privilege binding. Съобщенията за грешка орязват SQL до 80 символа (без изтичане на чувствителни данни).

Тестове (отлично покритие): предикатът има изчерпателна таблица от write/read кейсове; corpus тестът кара ВСЕКИ реален loader (вкл. филтрирани варианти) срещу capturing fake D1 и доказва, че никоя заявка не е false-rejected; отделни тестове забраняват blocked методи в query-източника, chokepoint скенер за env.DB/destructuring/bracket/blocked методи, и ETL guard срещу readonlyD1. Non-vacuity проверката (home_totals) предотвратява vacuous pass.

Качество/архитектура: промените са атомарни, следват съществуващ patterns (thin wrapper, getDb(env) навсякъде), няма дублиране/dead code/mixed concerns. getDb е чиста (без кеш), запазвайки @sigma/db stateless. Коментарите обясняват решенията (защо text-guard, а не AST тук).

Незначителна забележка (не блокира): ETL guard тестът (apps/etl/src/readonly-guard.test.ts) забранява само \breadonlyD1\b, но не и getDb. Тъй като getDb също връща read-only обвивка, ако ETL някога импортне getDb (вместо readonlyD1), тестът няма да го хване. Ниска вероятност (ETL легитимно пише и не би ползвал getDb), но добавянето на getDb в регекса би направило guard-а херметичен.

CLAUDE.md съответствие: без частична имплементация, без TODO опростявания, без дублиране/dead code, изчерпателни смислени тестове, консистентно именуване, без over-engineering, без resource leaks.

---

Summary (English)

Introduces a least-privilege chokepoint: the web runtime accesses D1 only through getDb(env), which returns a read-only wrapper (readonlyD1) over env.DB; the ETL worker keeps the write-capable binding. Defense-in-depth — even if the assistant run_sql AST guard is bypassed, web cannot write to D1.

Security (Phase 0 — CLEAN): no secrets, no new/changed URLs, no new deps, no obfuscation/backdoor. Traced readonly-sql.ts for bypasses (CTE-prefixed DML, RETURNING, stacked statements, EXPLAIN <write>, comment/identifier-quote smuggling, dangerous funcs) — all rejected. SQLite/D1 specifics verified: no SELECT … INTO, pragma_* TVFs not false-rejected, word-boundary spares created_at; batch/withSession/dump all throw. OWASP A03/A01 covered; error messages truncate SQL to 80 chars.

Tests: exhaustive write/read predicate table; corpus test drives every real loader against a capturing fake D1 proving no false-reject; blocked-method scanners and chokepoint scanner (env.DB/destructure/bracket/blocked methods); non-vacuity assertion prevents vacuous pass.

Quality/architecture: atomic, pattern-consistent, no duplication/dead code, getDb is pure (stateless package). CLAUDE.md rules satisfied.

Minor (non-blocking): the ETL guard test forbids only readonlyD1, not getDb; adding getDb to the regex would make the guard hermetic since getDb also returns a read-only handle.

Recommendation: No blocking issues found. Solid, security-hardening change with strong test coverage. Safe to APPROVE after your review; consider the minor ETL-guard hardening as a follow-up nit.

[todorkolev]

Одобрявам.

Проверено: 0 отворени нишки, проверките са зелени, клонът е в крак с main (9 напред, 0 назад). @lyubomir-bozhinov е одобрявал два пъти (07-14 и 07-17), @ydimitrof и @cefothe също — одобренията отпаднаха само защото всяко бутане ги отхвърля, не заради нерешен въпрос по същество.

По същество: това е вторият слой под вече влезлия sql-ast-guard от #80. Първият проверява какво иска да пусне асистентът; този прави записа невъзможен, дори проверката да бъде заобиколена. Затова е предусловие за пускането на ключа по #80 — виж #134.

[lyubomir-bozhinov]

Ре-верифицирах на текущия HEAD (7ac00b1b): новият тест забранява и bracket-достъп env['DB']/env["DB"] — затваря дупката спрямо dot/destructure сканирането (същият raw binding, само изписан така, че да заобиколи точковия скан). Guard-ът остава защита срещу честен drift (regex-ът не лови backtick/computed достъп, но PR-ът го признава и това е приемливо). Държи. Одобрявам.