Repository navigation
feat(fase2): MCP review gate, Browser Guard, AG-UI, OTel e PII brasileira — com a auditoria respondida - #13
Merged
Conversation
…R PII recognizers Brings the Fase 2 surface from `superpowers-on-v3.8.51` onto the audited release line. That branch also carried the pre-audit Loop/Buzz code and a package.json that reverts this fork's identity, downgrades hono and drops build scripts, so this is a selective transplant, not a merge: only the 18 genuinely new files, minus one, plus three additive flags and the two BR recognizers. Four deterministic policy modules, all pure and all fail-closed. The MCP review gate denies a flagged package or a forbidden capability outright, sends anything new or permission-broadening back to human review, and re-reviews an update whose publisher failed verification even when it broadens nothing. The browser guard denies everything while the flag is off, denies a host outside the allowlist, and — the part that matters against prompt injection — denies an external effect whose origin is the page, so text read from a site can never escalate into a submit, download, upload or purchase. AG-UI defines the agent→UI event contract with a sequence validator, so a console can replay a run deterministically. OTel-lite speaks W3C Trace Context and drops every attribute outside an allowlist, so a prompt, a response or a secret cannot reach telemetry even by mistake. Two things the transplant had to fix rather than carry: The two new mutating routes read `request.json()` with hand-rolled shape checks. That is the defect the Loop/Buzz audit already ruled on, and the route-validation gate flags it. Both now parse with strict zod schemas and real bounds. `origin` in particular is a closed enum, not a free string: it is the field the injection defence turns on, and an unexpected value must not fall into the trusted branch. Neither route required admin scope, so a plain write token could approve an MCP package or rewrite the browser allowlist. Both prefixes are now in ADMIN_MUTATION_PREFIXES, beside /api/loop and /api/buzz. Also dropped: `open-sse/buzz-bridge/outbox.ts`, an in-memory outbox nothing imports, superseded by the persistent SQLite repository already on the release line; and migration `174_loop_engine_and_buzz_bridge.sql`, whose number collides with the 175 that shipped. `fase2-endpoints.test.ts` hardcoded that 174 filename and broke on the renumber — it now resolves the migration by its name suffix, which is the part that carries meaning. Verified: 34/34 Fase 2 tests, 61/61 flag and access-scope tests, typecheck:core and `tsc -p open-sse` clean, eslint --max-warnings=0 on every changed file, route-validation PASS (706 routes), cycles, error-helper, env-doc-sync and migration-numbering OK, and knip reports zero dead exports in the changed files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…back Two gaps a user hits on a remote install. The manual OAuth step showed the authorization URL to copy, but nothing opened it; navigating there from the same tab replaced the panel, and the panel is what has to stay open to receive the callback. And when the login replaced the tab rather than opening a popup, the callback page had no opener to close and no link anywhere, so the user was simply stranded on it. The panel now opens the URL in a new tab from a user gesture, which the browser does not block, and the callback page offers a way back to the providers page. Both strings go through the translation catalogue rather than being hardcoded. The change arrived from the Fase 2 branch with Portuguese literals baked into the markup, which is the same defect the Loop/Buzz audit rejected in that panel: this product is English-first with 41 locales. `oauthModal.openInNewTab` and `auth.backToOmniRoute` are synced across all of them and translated in the seven that matter most, including vi, whose completeness test does not accept a placeholder. The test that came with the change searched for the Portuguese label, which would have pinned the button to one language. It now matches the translation key, which is what the next-intl mock returns. Not taken from that branch: the same commit rewrote the OmniCopilot help link back to the upstream repository, undoing this fork's identity work. Verified: vitest oauth-open-in-new-tab 2/2, typecheck:core clean, and the three i18n gates (ui-coverage, value-drift, glossary) pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
MCP_REVIEW_ENABLED, BROWSER_USE_ENABLED and OTEL_TRACING_ENABLED, each with what the route does while the flag is off and what the module actually guarantees when it is on. The env-doc-sync gate requires a flag named in one file to exist in the other, and check:docs-all (doc-links, fabricated-docs strict) passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ainst the code
An adversarial audit of the Fase 2 surface came back REPROVADO with reproductions
executed against the real HTTP handlers. Two of the failures were the exact
sentences the operator reads in the feature-flag description.
**Browser guard claimed page-originated external effects are denied.** It
classified by the declared `kind`, and `click` and `type` were not in the
external-effect set. Clicking a "Confirm purchase" button is a `click`:
{"action":{"kind":"click","origin":"page","url":"https://example.com/confirm-purchase"}}
-> {"decision":"allow","reason":"ação de leitura/navegação em domínio permitido"}
Page content can now only ask for more reading. Anything else it asks for is
denied before the allowlist is even consulted, because the guard cannot know what
is on the other side of a click.
**The allowlist was skipped entirely when `url` was absent**, so `{"kind":"click",
"origin":"page"}` with an empty allowlist returned `allow` — the product's default
state answered allow. Every kind that reaches the network now requires a url and
is denied without one: not being able to decide has to mean deny.
**The MCP gate took the caller's word for its own approval.** `prior.approved`
arrived in the request body, and nothing tied that prior to a record — not even
the name was compared. A candidate asking for `shell:exec`, `fs:delete` and
`secrets:read` with `{"approved":true}` came back `approved`, no human. The route
no longer accepts `prior` at all. Prior approval is server state, there is no
approval store yet, so every candidate is evaluated as new and returns
`review_required`. The pure engine keeps the parameter for when that store lands.
**Forbidden capabilities escaped on case.** `KEYS:READ` and `Secrets:Exfiltrate`
passed the forbidden set untouched and reached `approved`. Permissions are now
compared trimmed and lowercased, which also fixes broadening detection and the
"declares sensitive permissions" warning the human reviewer was not getting.
**`publisherVerified` being absent counted as verified.** Only `=== false` forced
re-review, so a candidate whose verification step never ran was auto-approved by
the field's default. Absent now counts as unverified.
**The CEP recognizer corrupted output.** `NNNNN-NNN` bit the middle of any
hyphenated identifier — "Pedido 12345-678", "Rastreio 90210-123", "id
ABC-12345-678-X" were all redacted. Since the sanitizer walks the whole model
response, that was silent output corruption, not PII protection. It now requires
the "cep" clue nearby, the way the PIX recognizer already did.
**The PIX recognizer missed keys in the direction that matters.** The clue had to
be within 30 characters BEFORE the key and could not cross a newline, so "<uuid>
é a minha chave pix" and "chave Pix\n<uuid>" — the shape of a pasted chat message
— went through untouched. The clue now counts on either side and crosses lines.
**Span names bypassed the allowlist.** Only `attributes` were governed, so a
1,000,000-character name was retained whole in the ring buffer. Names are now
stripped of control characters and bounded.
**A NaN sequence defeated the AG-UI monotonicity check.** `NaN <= x` is always
false, so 0,5,NaN,2,3 validated clean despite the 5 → 2 regression.
Also in this commit: the four routes are documented in both `docs/openapi.yaml`
and the served `public/openapi.yaml`, with the contracts as they stand AFTER these
fixes; and the two Fase 2 access-scope prefixes finally have test cases — the audit
noted the suite passed identically with or without those lines.
Every finding above has a regression test that reproduces the audit's own input.
Verified: 61/61 across the seven Fase 2 suites, typecheck:core and open-sse tsc
clean, eslint --max-warnings=0 on every changed file, route-validation PASS (706
routes), check:api-docs-refs OK (703 spec paths, all backed by a real route).
Not fixed here, and recorded rather than hidden: the browser allowlist is read
from a `key_value` row that nothing in the repository writes, so the persisted
override described in the route header does not exist yet — callers must pass
`allowedDomains` in the body. And the Loop stream endpoint builds its whole body
in memory rather than streaming, so an EventSource client reconnects in a loop.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The verdict, each finding with the reproduction that was actually executed, what resisted the attack, and the four pendencies that were recorded rather than hidden — including that neither policy module has a real executor behind it yet, which is what frames the severity. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… byte, undo two complexity regressions Four CI failures, all introduced by the previous commit. **Raw SQL in a route.** `src/app/api/browser/check/route.ts` read the persisted allowlist with an inline `SELECT` against `key_value`. The repository's db rule forbids that in a handler, and `check-db-rules` caught it. The query moves to `src/lib/db/browserGuard.ts`, alongside the other `key_value` modules, and the read stays fail-closed: a missing row, invalid JSON or a non-array all collapse to an empty allowlist, which denies everything. The module also carries the single write path for when a panel screen to configure the allowlist exists — and says plainly in its header that nothing writes the key today, so callers must pass `allowedDomains` in the body. **A raw NUL byte in a source file.** The character class in `safeSpanName` was written as literal control bytes instead of escapes, which makes git, GitHub and ripgrep treat the file as binary. `source-no-raw-nul-bytes` caught it. It is written with unicode escape sequences now. **Two complexity regressions**, both from the previous commit and both back at the base level: - `decideBrowserAction` went from 0 to 1 cognitive-complexity violation when the url/allowlist handling grew two branches. The target check is now `rejectTarget`, which returns the refusal verdict or null — the decision function reads as the ordered list of rules its docblock claims to be. - `OAuthManualInputPanel` crossed the 80-line ceiling when the open-in-new-tab button landed. Step 1 of the manual flow is now `OAuthAuthUrlRow`, which is a coherent unit on its own. The remaining violation in that file, `OAuthLoopbackMismatchPanel` at 94 lines, is pre-existing and untouched. Verified: check-db-rules OK (731 files), source-no-raw-nul-bytes OK, 32/32 across the affected suites, vitest oauth 2/2, typecheck:core clean, eslint --max-warnings=0 on every changed file, and the ratchet config reports 0 violations for browser-guard and 1 for OAuthModalPanels — the same count the base has. The ratchet gate itself cannot run here: it symlinks node_modules into a temp worktree, which Windows refuses without elevation, so it was checked per file with the gate's own config. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ng /api/mcp/review The agent-skills generator derives SKILL.md from the OpenAPI catalogue, and the merge-integrity gate fails when a committed file no longer matches what the generator produces. Documenting POST /api/mcp/review made omni-mcp stale. Regenerated with --only=omni-mcp so the commit carries that one skill and not the line-ending churn the generator produces for the other 45 on a CRLF checkout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`setBrowserAllowedDomains` had no caller. It was added for a panel screen that does not exist, and the dead-code gate is right to refuse it: an export nothing uses is dead code regardless of intent. The module keeps the read, and its header now says why there is no writer rather than shipping one on speculation — the configuration screen, when it exists, will bring its own write path together with the use case that justifies it. Verified: knip reports 0 dead exports in the changed files, typecheck:core clean, 12/12 across fase2-endpoints and check-db-rules. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fase 2 sobre a base auditada, com a auditoria já respondida
Traz da branch
superpowers-on-v3.8.51o que restava da Fase 2. Aquela branch também carregava o Loop/Buzz pré-auditoria e umpackage.jsonque reverte a identidade deste fork, rebaixa ohonoe remove scripts de build. Por isso o que entra aqui é transplante seletivo, não merge: só os 18 arquivos realmente novos, menos um, mais três flags aditivas e dois reconhecedores de PII.Quatro módulos de política determinísticos, puros e fail-closed, todos desligados por padrão: MCP review gate, Browser Guard, AG-UI e OTel-lite. Mais os reconhecedores brasileiros de CEP e chave PIX, e duas correções no fluxo de OAuth.
A auditoria adversarial voltou REPROVADA — e está respondida
Um auditor independente atacou as próprias afirmações dos módulos, com reprodução executada no handler HTTP real. Duas delas eram exatamente as frases que o operador lê na descrição do feature flag.
O Browser Guard não negava efeito externo originado na página. Classificava pelo
kinddeclarado, eclick/typenão estavam no conjunto de efeitos externos. Clicar num botão "Confirmar compra" é umclick:Agora conteúdo de página só pode pedir mais leitura. Qualquer outra coisa é negada antes mesmo de olhar a allowlist, porque o guarda não sabe o que há do outro lado de um clique.
Sem
url, a allowlist inteira era pulada, então{"kind":"click","origin":"page"}com allowlist vazia devolviaallow— o estado padrão do produto respondia allow. Toda ação que alcança a rede passa a exigir url.O gate de MCP acreditava no chamador sobre a própria aprovação.
prior.approvedvinha no corpo e nada amarrava aquele prior a um registro, nem onameera comparado. Um candidato pedindoshell:exec,fs:deleteesecrets:readcom{"approved":true}saíaapproved, sem humano. O corpo não aceita maisprior.Permissões proibidas escapavam por caixa alta:
KEYS:READeSecrets:Exfiltrateatravessavam intactas. Agora a comparação é normalizada.publisherVerifiedausente contava como verificado — só=== falseforçava re-revisão, então um candidato cuja verificação nunca rodou era auto-aprovado pelo valor padrão do campo.O reconhecedor de CEP corrompia saída. O padrão
NNNNN-NNNmordia o miolo de qualquer identificador hifenizado: "Pedido 12345-678", "Rastreio 90210-123", "id ABC-12345-678-X". Como o sanitizador percorre a resposta inteira do modelo, isso era corrupção silenciosa, não proteção.O reconhecedor de PIX perdia chaves na direção que importa. A pista tinha que estar 30 caracteres antes e não cruzava quebra de linha, então
"<uuid> é a minha chave pix"e"chave Pix\n<uuid>"passavam intactos.Nome de span escapava da allowlist (1.000.000 de caracteres retidos inteiros) e um
seqNaN derrotava a checagem de monotonicidade do AG-UI.Cada achado tem teste de regressão que reproduz a entrada usada pela auditoria.
Verificação
61/61 nas sete suítes da Fase 2 ·
typecheck:coreetsc -p open-sselimpos ·eslint --max-warnings=0em todo arquivo alterado ·route-validationPASS (706 rotas) ·check:api-docs-refsOK (703 caminhos, todos com rota real) · gates de i18n (cobertura, drift, glossário) PASS · knip sem símbolos mortos nos arquivos tocados.As quatro rotas estão documentadas nas duas especificações, incluindo a servida em
/api/docs, já com os contratos pós-correção.Pendências honestas
key_valueque nada no repositório escreve; o override persistido descrito no cabeçalho da rota ainda não existe, então o chamador precisa passarallowedDomainsno corpo.EventSourcereconecta em laço.review_required. É a resposta correta enquanto não houver onde guardar a aprovação, mas significa que o caminhoapprovednão é alcançável por esta rota hoje.🤖 Generated with Claude Code