fix: критические цепочки из аудита — byreal-парсинг, rate-limiter, атомарность тика, sweep - #54
fix: критические цепочки из аудита — byreal-парсинг, rate-limiter, атомарность тика, sweep#54loficoded wants to merge 4 commits into
Conversation
…от инъекции конвертов parse.ts: close-интент не несёт side (closeShape), и buildOutcome безусловно негировал filledSize — закрытие шорта записывалось на credibility-поверхность с инвертированным знаком. Теперь знак выводится из остаточной позиции: отрицательный residual (шорт) => положительная дельта. Полное закрытие (позиция flat, residual нет) сохраняет историческое поведение — задокументировано. envelope.ts: parseEnvelope брал первый сбалансированный JSON-объект из stdout. Теперь собираются все top-level объекты: не-конвертный JSON-баннер пропускается, а >1 валидного конверта — fail-closed (ByrealParseError), чтобы подставленный фейковый конверт не мог заменить реальный fill. Заодно passthrough -> strip: неизвестные top-level ключи конверта не просачиваются в executions.response_json. Регрессионные тесты: закрытие шорта/лонга с residual, JSON-баннер, двойной конверт (инъекция), strip лишних ключей.
check() никогда не удалял записи из map — атакующий, варьирующий ключ (например, спуфингом X-Forwarded-For), наращивал по записи на запрос до OOM. Теперь раз в окно выполняется полная зачистка устаревших записей (амортизированный O(1) на вызов); map ограничен числом уникальных ключей, активных в пределах одного окна. prune() переиспользует ту же логику. Регрессионный тест: 50 устаревших ключей удаляются следующим check().
…й транзакции processAgentTick писал intent (с резервацией nonce), policy events, execution и outcome отдельными autocommit-стейтментами. Падение между резервацией и insertOutcome навсегда сжигало nonce: повторный прогон молча скипал тик (анти-replay читал собственную рваную запись), fill терялся. Тело тика обёрнуто в BEGIN..COMMIT на выделенном клиенте (тот же паттерн, что settleRound); при ошибке ROLLBACK откатывает и резервацию — повторный прогон дообрабатывает тик. settleCredibility вынесен ЗА транзакцию: он best-effort и глотает свои ошибки, но проглоченная ошибка БД внутри открытой транзакции Postgres абортила бы её и откатила seed-строки. Happy-path байт-в-байт идентичен (те же строки, тот же порядок) — детерминизм арки сохранён. ТРЕБУЕТ прогона integration-сьюта с DATABASE_URL.
…rator-actions sweep-attestations.ts: feedbackURI пишется on-chain навсегда (канонический ERC-8004 не имеет updateFeedback); дефолт https://localhost зашивал бы в чейн вечно недостижимый URI и ломал проверку целостности у любого верификатора. Теперь без явного PUBLIC_BASE_URL свип падает сразу. lib/db/repos/index.ts: operator-actions не реэкспортировался из barrel — единственный репозиторий вне индекса (коллизий имён нет).
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| // F-01 hardening: an attacker who can vary the key (e.g. by spoofing | ||
| // X-Forwarded-For) would otherwise add an entry per request that `check` | ||
| // never reclaimed, growing the map without bound until OOM. Sweep stale | ||
| // entries at most once per window so the map stays bounded by the number of | ||
| // *distinct keys seen within a single window* at amortized O(1) per call. | ||
| if (now - this.lastPrune >= this.windowMs) { | ||
| this.pruneBefore(cutoff); | ||
| this.lastPrune = now; | ||
| } |
There was a problem hiding this comment.
💡 Security: Rate-limiter map still unbounded within a single window
The amortized sweep in check() only runs once every windowMs (gated by now - this.lastPrune >= this.windowMs). Between sweeps, every distinct key adds an entry that is not reclaimed until the next window boundary. An attacker spoofing X-Forwarded-For at high rate can therefore still inflate the map to the number of distinct keys observed within one window before any prune occurs — a much smaller but still real memory-pressure vector, not a hard bound. This is acknowledged in the PR description as the intended tradeoff, so it is informational: if windowMs is large (e.g. 60s) the in-window peak can be very large. Consider also pruning opportunistically when map.size crosses a hard cap, or shortening the prune cadence under load.
Was this helpful? React with 👍 / 👎
| // B-06 (banner injection): collect *all* well-formed envelopes, not just the | ||
| // first balanced object. A non-envelope banner object (`{"debug":true}`) is | ||
| // skipped instead of aborting the parse. But the genuine CLI prints exactly | ||
| // one envelope, so if more than one envelope-shaped object appears we refuse | ||
| // to guess which is authoritative — a prepended/appended fake envelope must | ||
| // not be able to substitute a forged fill. Fail closed: the caller degrades to | ||
| // the deterministic seed fallback. | ||
| const envelopes: ByrealEnvelope[] = []; | ||
| for (const json of candidates) { | ||
| let raw: unknown; | ||
| try { | ||
| raw = JSON.parse(json); | ||
| } catch { | ||
| continue; // Not valid JSON (e.g. `{ not: json }`) — ignore this candidate. | ||
| } |
There was a problem hiding this comment.
💡 Edge Case: Benign CLI JSON status line can disable the byreal rail
parseEnvelope now treats any top-level JSON object with a boolean success field as an envelope, and fails closed (ByrealParseError) when more than one envelope-shaped object is present. This is the correct anti-injection posture, but it also means a genuine CLI (or a dependency) that prints an incidental JSON status/log line containing a success boolean before/after the real envelope (e.g. {"success":true,"level":"info"}) will be misclassified as a second envelope and permanently degrade the rail to the seed fallback. Because settleCredibility swallows the error, this failure is silent except for one console.error. Worth confirming the real CLI never emits a success-bearing banner, or tightening the envelope discriminator (e.g. require data/meta/error presence) so non-envelope JSON is reliably skipped rather than counted.
Was this helpful? React with 👍 / 👎
Code Review 👍 Approved with suggestions 0 resolved / 2 findingsAddresses critical audit findings including short-position delta sign errors, rate-limiter OOM risks, and transactional atomic ticks. Ensure the rate-limiter map bounds are strictly enforced and refine envelope detection to prevent benign CLI output from triggering rail failures. 💡 Security: Rate-limiter map still unbounded within a single window📄 lib/api/rate-limit.ts:46-54 📄 lib/api/rate-limit.ts:69-76 The amortized sweep in 💡 Edge Case: Benign CLI JSON status line can disable the byreal rail📄 lib/rail/byreal/envelope.ts:106-120 📄 lib/replay/orchestrator.ts:479-488
🤖 Prompt for agentsOptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change:
Important Your trial ends in 6 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more. Was this helpful? React with 👍 / 👎 | Gitar |
1 participant
Что это
Результат глубокого ревью шестью независимыми ревьюерами по зонам (intent/подписи, referee/router, chain/контракт, scoring/детерминизм, byreal-rail/парсинг, replay/DB). Итог ревью: 1 CRITICAL, 9 HIGH, 14 MEDIUM, 16 LOW. Этот PR вносит ту часть исправлений, которая проверяема локально юнит-тестами и не меняет детерминированную демо-арку и не требует редеплоя контракта. Остальное — осознанно отложено и перечислено ниже с готовыми решениями.
Исправления
1.
lib/rail/byreal/parse.ts— инвертированный знакposition_deltaпри закрытии шорта (HIGH)close-интент не несётside(closeShape), иbuildOutcomeбезусловно негировалfilledSize: закрытие шорта (покупка обратно) записывалось на верифицируемую credibility-поверхность как отрицательная дельта. Теперь знак выводится из остаточной позиции после филла: отрицательный residual (шорт) ⇒ положительная дельта. Полное закрытие (позиция flat — у венью нечего читать) сохраняет историческое поведение и задокументировано как ограничение. Скоринг не затронут — byreal-ауткамы в него не входят.2.
lib/rail/byreal/envelope.ts— инъекция конверта через stdout CLI (MEDIUM) + утечка top-level ключей (LOW)parseEnvelopeбрал первый сбалансированный JSON-объект из stdout. Скомпрометированный CLI (или его зависимость) мог напечатать фейковый конверт перед настоящим и подменить fill. Теперь:{"debug":true}) пропускается, а не валит парс;ByrealParseError(fail-closed, рейл деградирует к seed-фоллбеку);passthrough→ strip: неизвестные top-level ключи конверта (например, эхо-credential) не попадают вexecutions.response_json.3.
lib/api/rate-limit.ts— неограниченный рост памяти (MEDIUM)check()никогда не удалял записи: атакующий, варьируя ключ спуфингомX-Forwarded-For, наращивал по записи на запрос до OOM. Теперь раз в окно выполняется амортизированная зачистка; map ограничен числом уникальных ключей, активных в пределах одного окна.4.⚠️ требует integration-прогона
lib/replay/orchestrator.ts— torn writes в пути тика (HIGH)Тик писал intent (с резервацией nonce), policy events, execution и outcome отдельными autocommit-стейтментами. Падение между резервацией и
insertOutcomeнавсегда сжигало nonce: повторный прогон молча скипал тик (анти-replay читал собственную рваную запись), и fill терялся. Тело тика обёрнуто вBEGIN…COMMITна выделенном клиенте (тот же паттерн, чтоsettleRound);ROLLBACKоткатывает и резервацию.settleCredibilityвынесен за транзакцию: он best-effort и глотает свои ошибки, но проглоченная ошибка БД внутри открытой транзакции Postgres абортила бы её и откатила seed-строки. Happy-path байт-в-байт идентичен — детерминизм арки сохранён.5.
scripts/sweep-attestations.ts—https://localhostкак feedbackURI навсегда on-chain (MEDIUM)feedbackURIпишется в чейн без возможности обновления (канонический ERC-8004 не имеетupdateFeedback). Дефолт зашивал бы вечно недостижимый URI и ломал проверку целостности у любого верификатора. Теперь без явногоPUBLIC_BASE_URLсвип падает сразу.6.
lib/db/repos/index.ts—operator-actionsне реэкспортировался из barrel (LOW)Единственный репозиторий вне индекса; коллизий имён нет.
Верификация
bun run typecheck— 0 ошибокbun run lint— 0 ошибокbun run test:unit— 988 pass / 0 fail (+6 новых регрессионных тестов: знак закрытия шорта/лонга, JSON-баннер, двойной конверт, strip ключей, зачистка rate-limiter)bun run test:fuzz— 77 pass / 0 failtest:integration/test:e2e— не прогонялись (нетDATABASE_URLв среде) — см. п. 4Осознанно отложено (нужны продуктовые решения)
Эти находки подтверждены, но их исправление меняет записанную демо-арку (видео + on-chain аттестации уже опубликованы), требует редеплоя верифицированного в Sourcify контракта или знания семантики venue:
drawdown: '0'захардкожен во всех трёх местах сборкиRefereeState(orchestrator, inject-attack, parse) — правило R3 не может сработать никогдаencodeScoreValue: потеря точности при кодировании score on-chainremaining_budget = allocationкаждый тикVectorMeritRegistry: нетtransferOwnership-override,attestScoreне валидирует identity, нет cooldownmax_slippageне пробрасывается в CLIraw_r, UNIQUE на executionsКаждую из них можно вносить отдельным PR после хакатона — детальные патчи готовы.