Решать, что уходит наружу, до того как ревьюеры что-то прочитают - #2
Решать, что уходит наружу, до того как ревьюеры что-то прочитают#2jojoprison wants to merge 1 commit into
Conversation
The reviewers were told to list the working tree themselves, untracked files included. That is the honest description of an uncommitted change and also the one place a repo keeps .env.local, a stray deploy key, a service-account JSON and this plugin's own providers.env — so on a private repo the instruction hands those to an outside model, and by the time any filter could look at the content, the content is already in somebody else's prompt. safe-paths.sh settles the scope first, by path name only: it never opens a file, prints an explicit allow-list plus every withheld path with the rule that caught it, and fails closed — an unresolvable range allows nothing instead of degrading into 'review the whole working tree'. Both reviewer prompts now say not to widen the scope themselves, and both skills run the step before launching anything. --self-test covers three checks, one of them a positive control: an ordinary source file must still come through, or a script that withheld everything would pass the secret checks too. Verified by mutation — disabling the env rule fails the negative checks, breaking collection fails the positive one.
|
Дополняю собственный PR замером, который его же ослабляет. Открыв PR, я решил проверить, насколько часто класс вообще встречается, и прогнал по всем репозиториям на своей машине: 90 репозиториев, 621 untracked-файл, под deny-правила попало 0. Позитивный контроль пройден — в тестовом репозитории Что это значит: аргумент про риск в описании выше подан сильнее, чем заслуживает. У дисциплинированного пользователя со зрелым Чего замер не доказывает: это выборка одного человека. Про типичного пользователя плагина у меня данных нет, а дефолтные бэкенды — бесплатные пулы, куда я как раз ничего не отправляю, так что мой профиль риска не показателен. Решай с полной картиной. Отказ будет обоснованным — скажи, и закрою сам. Если решишь брать, есть минимальная версия, и я бы на твоём месте выбрал её: взять только правки в двух промптах ( |
The reviewer prompts used to say "run git status --untracked-files=all and read what you find". Untracked is where a working tree keeps what was never meant to travel, so that sentence invited an external model to open a stray deploy key or a .env nobody remembered to ignore. Now the orchestrator decides, in code both reviewer scripts already call, and hands over a finished list. PR #2 proposed the same idea as a separate 210-line script plus a step in the skill markdown; this is the same protection without a script that can be skipped and without a second copy of the path collection that already lives in collect-context.sh. Most of the work turned out to be git's. `--exclude-standard` already drops everything .gitignore covers, so the rules only ever see files that slipped past it -- which is exactly how a secret usually escapes. What the reviews caught, all measured rather than argued: - Holding shift bypassed every rule: .ENV, ID_RSA, SECRETS.JSON, .NETRC and backup.PEM all sailed through. nocasematch works on bash 3.2 and costs no subprocess, unlike ${var,,}, which needs bash 4. - A symlink is judged by where it points, so we refuse to look: an innocent notes.txt -> ~/.ssh/id_rsa passed every name rule and the reviewer read the target. - From a subdirectory the note silently dropped every new file elsewhere in the repo, under a sentence telling the reviewer not to look for more. That was worse than the instruction it replaced. Anchored on the repo root. - The note ignored --paths while the git diff beside it honoured it, so a scoped review disclosed names from the whole tree. - A git failure printed nothing, and nothing reads as "no new files". It now says so out loud. - Shell metacharacters in a filename reached a model whose tool calls are pre-approved. Same guard as --paths, reused. - Missing names: .git-credentials, .pypirc, .dockercfg, .boto, kubeconfig, terraform.tfstate, and my-service-account.json, which the prefix-only rule let through. Deliberately NOT claimed: this is prompt hygiene, not isolation. The reviewers run inside the real tree with pre-approved tools, so any of them can read a secret it decides to open. Real isolation means copying the allowed files into a scratch tree and pointing --dir at that, which is separate work. Tests: 16 checks, 5 of them positive controls, because a filter that withholds everything passes every negative check. Four mutations verified each new guard is actually load-bearing. Green on bash 5.3 and bash 3.2.
|
Привет! Спасибо — проблему ты нашёл настоящую, и она уехала в main коммитом 6a7158e. Но реализацию я сильно урезал, и ты вправе знать почему. Пишу подробно. Главное: git половину делает бесплатноЯ прогнал замер, прежде чем что-то решать. Оказалось, Проверка: репозиторий, где в Отсюда неприятный вывод про примеры из описания твоего PR: Но это не отменяет твою находку, а сужает её до самого частого способа слить секрет: файл, который забыли добавить в Что из этого вышлоРаз git отбирает игнорируемое сам, задача сжалась до одного: список имён, которые не отдаём, даже если они не в Остальные ~170 строк убрал:
Итог: две функции в Два бага в самом PR1. Пустой список путей роняет скрипт на bash 3.2 (стоковый bash на маке). Секреты не утекают, но скрипт падает и при этом сообщает код возврата 0, то есть «всё хорошо». Забавно, что рабочая версия этой же проверки лежит в соседнем файле: 2. Не вызывается Что нашлось уже в моём кодеЧтобы не выглядело, будто я тут умнее. Я прогнал на своей версии полное ревью несколькими моделями, и вот что она пропускала:
Честно про границыТвоё описание говорит, что содержимое «здесь не читается никогда». Это правда про сам скрипт, но не про итог. Внешние ревьюеры запускаются внутри репозитория с доступом к шеллу: То есть и твой фильтр, и мой убирают приглашение («перечисли рабочее дерево и прочитай что найдёшь»), но не убирают возможность. Я это прямо написал в комментарии к коду, чтобы следующий человек не принял его за настоящую изоляцию. Настоящая — копировать разрешённые файлы во временную папку и указывать Что с PRОставляю решение за тобой: закрыть как «идея уехала другим коммитом», или ты хочешь довести свою версию. Я не мержил твою ветку, так что авторство коммита моё — если тебе это важно, скажи, придумаем как отразить твой вклад нормально. И ещё раз: находка твоя, замер лишь показал, где именно она стреляет. |
2 participants
Привет! Разбирал плагин, чтобы понять, ставить ли его глобально — и наткнулся на одну вещь, которую починил сразу, вместо того чтобы просто завести issue. Пишу по-русски, раз мы оба, коммит оставил на английском, как и всё в репо.
Проблема
Ревьюерам сейчас передаётся инструкция найти изменения самим, включая untracked:
Для описания незакоммиченной правки это честно: свежесозданный файл и есть изменение, без него ревью неполное. Но untracked — ещё и то место, где рабочее дерево держит вещи, которые уезжать не должны:
.env.local, забытый в корне deploy-ключ, service-account JSON, и — отдельная ирония —providers.envсамого плагина. На публичной песочнице это ничего не стоит, на приватном репозитории это отправка секретов наружу, причём фильтр после сбора уже не помогает: чтобы решить, можно ли показывать содержимое, его придётся прочитать, а прочитанное уже в чужом промпте.Отказ тихий вдвойне. В отчёте ничего не появляется, скоуп при этом самый естественный — «мои текущие изменения», то есть ровно то, что человек выберет по умолчанию.
Что предлагаю
Развернуть порядок: сначала имена, содержимое здесь не читается никогда.
Новый
scripts/safe-paths.shсобирает пути в скоупе (git diff --name-only,git ls-files— ни одного открытия файла), прогоняет их через список deny-правил по имени и печатает машиночитаемо:Дальше разрешённые пути уходят обоим внешним ревьюерам через
--paths, а в их промптах теперь прямо сказано не расширять скоуп самостоятельно. Число withheld просится в отчёт — ревьюер, видевший не всю правку, должен сказать это вслух, иначе «чисто» означает не то, что кажется.Три свойства, на которых всё держится:
rc=2и пустой allow-list. Пустой список значит «не отправлять ничего», никогда «отправить всё».5 allowed, 0 withheld..env.exampleи родня — в белом списке, их работа как раз в том, чтобы лежать в репозитории.Дифф
scripts/safe-paths.sh--self-testscripts/review-codex.shscripts/review-opencode.shskills/code-review/SKILL.mdskills/check-if-done/SKILL.md34 добавленные строки в существующих файлах, остальное — новый скрипт. Логику ревью не трогал.
Как проверено
--self-testподнимает временный репозиторий сsrc/app.py,.env.localиdeploy_rsaи делает три проверки, из них одна позитивная. Позитивная тут не для красоты: скрипт, который прячет вообще всё, проходит обе негативные проверки, и без «обычный файл обязан пройти» тест был бы зелёным на сломанном коде.Проверил мутациями, что тест действительно ловит:
env-fileself-test OK — 3 checksПлюс живой прогон на этом репозитории (5 разрешено, 0 исключено) и на несуществующем ref (
rc=2, пустой allow-list).Чего намеренно не делал
codex execпотерял накопленные обходы, полез проверять — и нет: в коммите v2 у тебя прямо написано, почему плагинные команды не оркестрируются, а обе гочи самого CLI (кастомный промпт несовместим с флагами скоупа;effortпо умолчаниюnone) уже учтены в комментарияхreview-codex.sh. Претензия снята, решение обоснованное.Отдельно, follow-up (в этот PR не тащу)
gh pr viewне встречается в плагине ни разу, то есть «отревьюй PR 42» сейчас никак не превращается в скоуп. Наивная версия (main...HEAD) тихо ревьюит не тот дифф в двух случаях: PR открыт против нерелизного base (стек веток, релизные линии) и когда локальныйmainотстал. Резолв —gh pr view N --json baseRefName,headRefName,headRefOid. Скажи, нужно ли — сделаю вторым PR, мешать с безопасностью не хочу.Спасибо за плагин,
check-if-done— лучшая часть, «модель, только что написавшая код, — худший судья того, работает ли он» стоит того, чтобы быть отдельной командой.