Altinity / Altinity/altinity-sql-browser

Tighten shell-guardrail 'provably clean' classification to fail-closed by default

オープン
#673 コメント 0 件 リアクション 0 件 担当者 0 名 GitHub で見る

まだ誰も着手していません。

inbox
主要言語
TypeScript
スター
8
フォーク
2
平均マージ
1時間 34分
マージ済み PR(30日)
6

説明

Context

Filed while shipping #592 (PR #672 — mechanical shell-primitive architecture guards in
build/check-boundaries.mjs / build/lib/check-legacy-owners.mjs).

Two formal ChatGPT pr-mode review sessions (6 total passes) each found and the
coordinator verified/fixed real defects in the new shell-body-mount and
shell-capture-escape analyzers. The first session's 3 passes were all variants of one
root cause — a hand-rolled scope-resolution layer approximating JS/TS binding semantics
— fixed by a structural restructuring to real TypeScript-checker-based symbol
resolution (checker.getSymbolAtLocation), independently confirmed sound by a second
internal review.

The second session's 3 passes (after the restructuring) were a different, adjacent
problem: proving a captured JS value/identifier is never mutated or aliased after its
own declaration. Each pass closed one concrete shape and the next pass found another:

  • let/var reassignment of a capture-options identifier or a Document/Window alias
    (fixed for resolveCaptureFlag/resolveHandlerNode/the body resolver, then found
    missing in the Document/Window resolver itself in the very next pass).
  • Direct property mutation on a const-declared options object (opts.capture = true).
  • The SAME mutation via an alias of the original identifier (const a = opts; a.capture = true), and via a compound assignment operator (opts.capture ||= true).
  • An unresolved const ESC = 'Escape' alias for the Escape-comparison string literal.
  • A bracket-notation call spelling (document['addEventListener'](...)) never
    considered a candidate at all.

All of the above were fixed and are live on main as of PR #672's merge. But "does this
identifier's bound value ever change, through any alias, any operator, anywhere in
scope" is a full points-to/alias-analysis problem — genuinely open-ended. The
coordinator and the repo owner judged shipping the current state as the right call (real
adversarial code exploiting this is a much narrower threat than #592's actual concern —
an ordinary developer re-introducing a copy-pasted overlay lifecycle — and a mechanical
grep/AST-light architecture guard, per this repo's own stated design philosophy for
check-boundaries.mjs, was never going to fully close arbitrary adversarial JS), but the
gap is real and worth a deliberate follow-up decision rather than silent acceptance.

Proposal

Rather than continuing to chase individual mutation/aliasing shapes as they're found (the
pattern that produced 6 review passes), consider narrowing what the analyzer classifies
as "provably safe"
to an intentionally small, easily-exhaustible set of syntactic
shapes — e.g., a bare literal directly in the call, or a const binding with a literal
initializer that a cheap same-file check proves is never re-referenced by any OTHER
identifier and never has any property-mutation expression (<name>.<prop> = ... in any
form, including compound operators) anywhere in the enclosing file. Everything else
(aliases, mutation of any kind, unresolved identifiers) fails closed as
uncheckable-options/uncheckable-handler — an unconditional violation requiring a
human-reviewed, explicit allowlist entry — rather than the analyzer trying to prove a
negative about ever-more-exotic mutation shapes.

This flips the maintenance burden: today, each new bypass shape discovered requires a
resolver code change to "understand" it; under the proposal, only the (much smaller,
finite) set of provably-safe shapes needs code, and everything else is conservatively
rejected by construction — closer to this repo's existing check-boundaries.mjs
philosophy elsewhere (frozen exact allowlists, fail loud rather than fail permissive).

Files

  • build/lib/check-legacy-owners.mjsresolveCaptureFlag, resolveHandlerNode,
    resolveGlobalKind/classifyGlobalDeclaration, resolvesToDocumentBody,
    hasCapturePropertyMutation.
  • tests/unit/shell-guardrails-arch.test.ts — the sabotage matrix would need updating to
    match a narrower "provably safe" contract (many currently-passing-because-permissive
    fixtures would flip to expecting uncheckable-*).

Why deferred

Out of scope for #592 itself (enforcement-only, and the current state already ships
real, valuable protection against the actual regrowth pattern #586/#587 fixed); this is
a deliberate hardening decision for a human to schedule, not an emergency.

コントリビューションガイド

コントリビューションガイドを開く

はじめの一歩

  1. issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
  2. 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
  3. リポジトリをフォークし、ブランチを切って変更します。
  4. issue 番号を参照したプルリクエストを送ります。

調査の方向性

build/lib/check-legacy-owners.mjs から始めて、resolveCaptureFlag、resolveHandlerNode、resolveGlobalKind/classifyGlobalDeclaration、resolvesToDocumentBody、hasCapturePropertyMutation を読みます。次に tests/unit/shell-guardrails-arch.test.ts とその sabotage matrix を調べます。完了とは、analyzer が意図的に狭く定義された、証明可能に安全な形だけを受け入れ、alias、mutation、未解決の identifier を更新されたテスト期待値とともに uncheckable-* として分類することです。

索引モデルが issue の本文から書いたものです。

評価

技術スタック
typescript
領域
build-system, testing-qa, tooling
issue の種類
リファクタリング
難易度
5/5
見積もり時間
1週間以上
活発さ
静か
明瞭さ
おおむね明確
初心者へのやさしさ
35/100

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。