Skip to content

fix: harden secret redaction, cache perms, and CRLF parsing#1

Open
marcelo-maciel wants to merge 1 commit into
mainfrom
harden/security-crossos
Open

fix: harden secret redaction, cache perms, and CRLF parsing#1
marcelo-maciel wants to merge 1 commit into
mainfrom
harden/security-crossos

Conversation

@marcelo-maciel

Copy link
Copy Markdown
Owner

Context

Full-source security + cross-OS audit of ccvitals. Verdict: cross-OS solid, security good. Findings below are hardening, not exploitable vulns.

Changes

  • S1 (secret redaction): buildTodoStr and parseTranscript now redact todo content/activeForm, matching lookupTask. Stops a secret pasted into a todo from rendering raw or persisting raw to the state cache.
  • S4 (cache perms): all atomicWrite caches (utils.js + both hooks) written with mode 0600 so cost/rate-limit data stays owner-only on shared *nix hosts. No-op on Windows.
  • C1 (CRLF): contentStr trimmed in transcript parsing so a CRLF /clear record still triggers the reset across OSes.
  • C4 (CI): GitHub Actions matrix (ubuntu/macos/windows x node 20/22) running node --test — verifies cross-OS behavior that cannot be run locally (dev is win32-only).

Verification

  • node --test: 25/25 pass (baseline and post-edit).
  • S1 exercised via real functions: display redacted, in-memory redacted, on-disk state cache has no raw secret.
  • C1 exercised: /clear\r record resets turnCount.

Out of scope (deliberate)

S2/S3/S5/S6, C2, C3 — info or already mitigated. No schema change, no new dependency.

Security and cross-OS hardening from a full-source audit:

- Redact todo content/activeForm in buildTodoStr and before persisting
  lastTodos to the state cache, matching lookupTask. Prevents a secret
  pasted into a todo from rendering raw or landing raw on disk.
- Write all atomicWrite caches (utils + both hooks) with mode 0600 so
  cost / rate-limit / account-adjacent data stays owner-only on shared
  *nix hosts. No-op on Windows.
- Trim contentStr in transcript parsing so a CRLF /clear record still
  triggers the conversation reset across OSes.
- Add GitHub Actions matrix (ubuntu/macos/windows x node 20/22) running
  node --test to verify cross-OS behavior that cannot be run locally.
@marcelo-maciel marcelo-maciel changed the title Harden secret redaction, cache perms, and CRLF parsing fix: harden secret redaction, cache perms, and CRLF parsing Jun 18, 2026
@marcelo-maciel

Copy link
Copy Markdown
Owner Author

VEREDITO: APROVAR — hardening correto e bem escopado; redação, perms 0600 e trim de CRLF estão certos, sem bug nem regressão (25/25 testes verdes).

Resumo

PR pequeno (6 arquivos, ~50 linhas), só hardening (sem breaking change, sem dependência nova). Revisei o diff inteiro nas 5 dimensões (aderência ao repo, bugs no diff, histórico, código adjacente, comentários). Nenhum achado 🔴/🟡 sobreviveu ao corte de confiança.

Validação local

  • node --test25/25 pass (bate com o claim do body).
  • redact() é sempre chamada com string nos dois call sites novos: em buildTodoStr o fallback || '?' garante string; em parseTranscript o typeof td.content === 'string' ? … : td.content guarda o tipo. Sem risco de text.replace is not a function.
  • C1 (CRLF): contentStr só é usado no check de igualdade do /clear (linha 140); o texto real da mensagem do usuário vem de raw/txt mais abaixo (linha ~161), separado. Então o .trim() não altera conteúdo persistido nem exibido — efeito é só tornar o match de /clear robusto a \r. Correto.
  • S1 (redação no parse): redigir content/activeForm em parse-time + a redação já existente em buildTodoStr é idempotente (texto já redigido não re-casa os regex). status e o tamanho do array não mudam, então contagem e label do todo seguem corretos. O segredo não chega mais cru ao state cache em disco — que era o objetivo.
  • S4 (0600): renameSync preserva os perm bits do tmp, então o cache final fica 0600. No-op no Windows. Ok.

Achados

🔵 Sugestão — sem teste de regressão para os dois comportamentos novos

transcript.js (parse-time redaction de TodoWrite) e transcript.js:138 (trim p/ /clear\r).

A matriz CI nova (.github/workflows/test.yml) roda node --test nos 3 OSes — bom — mas os testes existentes não cobrem nenhum dos dois caminhos que este PR adiciona: não há um caso /clear\r (os testes de clear usam /clear puro) nem um caso de parseTranscript com TodoWrite carregando segredo. O body diz "exercised", mas esse exercício não ficou commitado, então a matriz cross-OS não pega uma regressão exatamente no comportamento cross-OS que o PR mira. Sugestão (não bloqueia o merge):

+ // test-effort.js — /clear com CRLF ainda reseta
+ test('clear-crlf-resets', () => {
+   const r = run(userCmd('/clear\r', '', '2026-06-03T11:00:00Z') + assistantTurn());
+   assert.strictEqual(r.turnCount, 0, 'clear-crlf-resets');
+ });
+ // test-util.js (ou test-effort.js) — segredo em TodoWrite não persiste cru
+ test('todo-secret-redacted-on-parse', () => {
+   const secret = 'a'.repeat(40);
+   const r = run(todoWrite([{content:`sk-ant-${secret}`, status:'in_progress', activeForm:`sk-ant-${secret}`}]));
+   assert.ok(!JSON.stringify(r.lastTodos).includes(secret), 'todo-secret-redacted-on-parse');
+ });

(Os helpers userCmd/run/todoWrite precisam casar com a infra de teste real; ajuste a assinatura.)

Descoberta de teste confirmada: node --test casa test-effort.js/test-util.js pelo glob default **/test-*.js (o run local achou os 25). O CI roda o mesmo comando, então pega os dois sem package.json test script.

Perguntas

  1. Vale fixar node-version do CI com algo além do major (20/22), ou seguir o latest do major tá ok?
  2. Os out of scope (S2/S3/S5/S6, C2/C3) ficam como issue rastreada ou são fecho definitivo?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant