Skip to content

feat(shared): redact paths from DatabaseError messages (W-3.5-SEC-L1)#45

Merged
h2devx merged 1 commit intodevelopfrom
feat/v0.5-hardening-redact-db-error
May 3, 2026
Merged

feat(shared): redact paths from DatabaseError messages (W-3.5-SEC-L1)#45
h2devx merged 1 commit intodevelopfrom
feat/v0.5-hardening-redact-db-error

Conversation

@h2devx
Copy link
Copy Markdown
Contributor

@h2devx h2devx commented May 3, 2026

Summary

Third fix of the v0.5 hardening cycle (HANDOFF.md §6.21 roadmap row 4). Closes warning W-3.5-SEC-L1 for DatabaseError (HANDOFF §6.6).

DatabaseError.openFailed(path, cause) and DatabaseError.migrationDirectoryInvalid(dir, ...) previously interpolated absolute paths into the message field. Pino redacts keys (DEFAULT_REDACT_PATHS) but does NOT redact message content — paths leaked to structured logs / observability exporters, revealing user home dir + repo layout.

This PR moves those paths from message to a new structured field details: { path } / details: { dir, reason }. Public factory signatures are unchanged (backward-compat). Pino DEFAULT_REDACT_PATHS gains 4 new globs covering details.path / details.dir (top-level + 1-segment-wrapped).

What changed

Code

  • code/src/shared/infrastructure/errors/database-error.ts (+49/-10) — details: Readonly<Record<string, unknown>> field added; `openFailed` and `migrationDirectoryInvalid` move path/dir from message to details; 8 other factories gain consistent `details` shape with non-sensitive data (extension name, sql length, operation name, version numbers).
  • code/src/shared/infrastructure/logger/pino-logger.ts (+12/-0) — DEFAULT_REDACT_PATHS gains: `details.path`, `details.dir`, `.details.path`, `.details.dir` (covers standalone + wrapped-in-err shapes).

Tests (+12 tests, all VALOR)

  • `openFailed`: `message NOT contains path` + `details.path === path`.
  • `migrationDirectoryInvalid`: `message NOT contains dir` + `details.dir === dir` + `details.reason === reason`.
  • 8 secondary factories: `details === { ... }` shape pinned per factory.
  • Backward-compat test: callers can read path via `details.path` (replaces old substring-on-message pattern).
  • Pino end-to-end redact: feed envelope to logger, capture stdout, assert path NOT in output, assert sibling field (reason) IS visible.

Audit factories inventory (10 total)

Factory Had path leak? Modified?
`openFailed(path, cause)` YES path → details.path
`migrationDirectoryInvalid(dir, reason, cause)` YES dir/reason → details
`encryptionKeyRejected` no details: {} (consistency)
`extensionLoadFailed` no details.extensionName
`prepareFailed` no details.sqlLength
`execFailed` no details: {}
`transactionFailed` no details: {}
`connectionClosed` no details.operation
`migrationAheadOfCode` no details.dbVersion + codeMaxVersion
`migrationFailed` no details.version + name

Security audit result

APPROVED WITH OBSERVATIONS (security-auditor). Backward-compat verified independently (no callers read path via message-substring). Tests pin redacted output end-to-end (not just config). No new wire-leak via JSON-RPC error-mapper (Tier 3.5 does NOT serialize `details` field).

3 non-blocking observations tracked for v0.5+ follow-up:

  • O-1 (LOW, OUT-OF-SCOPE): 9+ Error factories in workspace/secrets/curator modules still interpolate `rootPath`/`startPath`/`hookPath` into `message` — same leak pattern as W-3.5-SEC-L1, also flowing to wire JSON-RPC via error-mapper. W-3.5-SEC-L1 is NOT categorically closed by this PR — only closed for DatabaseError. Recommendation: open W-3.5-SEC-L2 (or extend L1) follow-up to apply same `details: { path }` pattern across all error factories before v0.5 GA. Affected files:
    • `workspace/infrastructure/errors/workspace-infrastructure-error.ts` (9 factories)
    • `workspace/application/errors/workspace-application-error.ts` (NoWorkspaceAtPathError)
    • `secrets/infrastructure/errors/foreign-hook-exists-error.ts`
    • `curator/infrastructure/errors/curator-infrastructure-error.ts` (scanFailed)
  • O-2 (INFO): pino `` glob is single-segment only. Wrapped envelopes deeper than `{ outer: { err: { details: { path } } } }` would NOT be redacted. No call-site affected today (verified). For v0.5 audit logging, consider proactive `.*.details.path` patterns.
  • O-3 (INFO): Add JSDoc warning on `DatabaseError.details` advising NOT to include in JSON-RPC `data` envelope (avoids future regression where audit data would leak to wire).

Test plan

  • `npm run typecheck` EXIT=0
  • `npm run lint` EXIT=0 (--max-warnings 0)
  • `npm run lint:tests` EXIT=0 (--max-warnings 0)
  • `npm run validate:modules` EXIT=0
  • `npm run build` EXIT=0
  • `npm test` EXIT=0 — 2578/2578 tests pass (+4 vs baseline 2574)

OWASP mapping

  • A09:2021 Security Logging Failures — closed (for DatabaseError; tracked O-1 for other classes).
  • A05:2021 Security Misconfiguration — strengthened (defaults expanded to redact structured path fields).

🤖 Generated with Claude Code

Move absolute filesystem paths out of `DatabaseError.message` and into
a new structured `details` bag (`{ path }`, `{ dir, reason }`). Pino's
redactor walks structured keys but not message content, so paths
concatenated into the message would otherwise leak into structured
audit / observability logs even with `DEFAULT_REDACT_PATHS` configured.

The factory signatures (`openFailed(path, cause)`,
`migrationDirectoryInvalid(dir, reason)`, ...) remain unchanged — only
the internal storage shape moves. Other factories also gain a
populated `details` bag (operation, version, name, sqlLength,
extensionName, dbVersion, codeMaxVersion) so callers have a single
non-message channel for structured fields.

Adds matching `details.path` / `details.dir` (plus `*.details.path` /
`*.details.dir`) globs to `DEFAULT_REDACT_PATHS` so the canonical
caller pattern `logger.error({ err }, "...")` redacts the path before
it lands in the JSON line.

Tests:
- `errors.test.ts`: assert `message` does NOT contain the absolute
  path and that `details.path` / `details.dir` carry the original
  value (backward-compat pivot for previous message-substring asserts).
- `pino-logger.test.ts`: end-to-end redact test feeding a synthetic
  DatabaseError envelope through `logger.error` and asserting the
  path never appears in stdout, while non-sensitive sibling fields
  (`reason`) stay visible.

Refs: HANDOFF.md §6.6 / §6.21 row 4 — Security-Low observation noted
during Fase 2 to be addressed before v0.5 introduces structured audit
logging.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@h2devx h2devx merged commit 30cfaa0 into develop May 3, 2026
1 check passed
@h2devx h2devx deleted the feat/v0.5-hardening-redact-db-error branch May 3, 2026 17:04
h2devx added a commit that referenced this pull request May 3, 2026
)

## Summary

Phase-17 close docs-only PR. Pattern matches PR #25 (Phase-12), #28
(Phase-13), #32 (Phase-14), #36 (Phase-15), #42 (Phase-16) — each
cycle/phase ends with a docs-only PR synthesizing the closure.

## What Phase-17 delivered

**v0.5 hardening defensivo cycle** — 4 incremental PRs squash-merged to
\`develop\`:

| # | PR | Warning | Fix |
|---|---|---|---|
| 1 | [#43](#43) | W-3.5-SEC-M2
| chmod 0o600 on recall.db |
| 2 | [#44](#44) | W-3.5-SEC-M1
| atomic write+rename on .gitignore + writeConfig consolidated with
CSPRNG suffix |
| 3 | [#45](#45) | W-3.5-SEC-L1
(partial) | redact absolute paths from DatabaseError messages →
\`details.path\` + 4 new pino redact globs |
| 4 | [#46](#46) | W-3.1-SEC-M1
| configurable buffer cap on StdioJsonRpcServer (default 10 MiB) + env
var override + transport closure on overflow |

Zero security-auditor rejections (4 APPROVED WITH OBSERVATIONS). 1 CI
round-trip in PR-2 over S7735 negated condition trivial fix. 36 new
VALOR-asserting tests consolidated, 5+1 EXIT=0 green in each PR,
SonarQube quality gate PASSED in each PR.

## Key finding tracked: W-3.5-SEC-L2 follow-up

PR #45's security-auditor revealed that **W-3.5-SEC-L1 is NOT
categorically closed** — only closed for DatabaseError. **9+ Error
factories in workspace/secrets/curator modules** still interpolate
\`rootPath\`/\`startPath\`/\`hookPath\` into \`message\`, and they flow
to the wire JSON-RPC via \`error-mapper.ts\` Tier 3.5. Same leak
pattern, also flowing to MCP clients.

Affected files (tracked as W-3.5-SEC-L2 for next hardening cycle):
- \`workspace/infrastructure/errors/workspace-infrastructure-error.ts\`
(9 factories)
- \`workspace/application/errors/workspace-application-error.ts\`
(NoWorkspaceAtPathError)
- \`secrets/infrastructure/errors/foreign-hook-exists-error.ts\`
- \`curator/infrastructure/errors/curator-infrastructure-error.ts\`
(scanFailed)

Recommendation: apply same \`details: { path }\` pattern across all
error factories before v0.5 GA.

## What this PR adds

Pure HANDOFF.md changes (212 insertions / 8 deletions):

- **§0**: 6 rows updated (Fecha, Fase actual, Lineas codigo, Tests,
Issues abiertos, Proximo paso).
- **§6.21**: roadmap row 4 (hardening defensivo) marked CLOSED in
Phase-17.
- **§6.22 NEW**: full Phase-17 cycle close section (decisions,
sub-phases, detail per PR, consolidated observations table with 12
entries, 8 orchestrator decisions D-1701..D-1708, 5 durable lessons,
repo state, next-action with 3 options for release).
- **Footer** "Ultima actualizacion" updated to reflect Phase-17 closure.

## State of repo post-merge

| Item | Value |
|---|---|
| HEAD develop | \`f23457e\` (4 commits ahead of main) |
| HEAD main | \`29371f8\` (unchanged) |
| Tag latest | \`v0.1.2\` (unchanged) |
| npm dist-tags | \`{ latest: '0.1.2', beta: '0.1.2-beta.6' }\`
(unchanged — Phase-17 publishes nothing) |
| Tests | 2588 passing in 212 files (+28 vs Phase-16 baseline) |
| Coverage | new 100% / overall 96.4% |
| Hardening warnings closed | 4/4 |
| Follow-ups tracked | 12 (1 medium W-3.5-SEC-L2 + 11 low/info) |
| Issues open | 0 |
| PRs open | 0 (after this merge) |

## Test plan

- [x] No code changes — pure docs PR (HANDOFF.md only).
- [x] Hooks pre-commit no-op (no \`code/src/\` changes → typecheck not
triggered).
- [x] CI required status check \`ci\`.
- [x] SonarQube quality gate (no source files affected).

## Decision pending after merge

**Cut \`release/0.1.3-beta.0\` now or later?**

- **Option A** — cut now with 4 hardening fixes alone (Phase-9/12/14
cooling pattern: ship beta, dogfood real, fix what surfaces).
- **Option B** — accumulate more changes (item #1 multi-key envelope,
item #3 perf hardening, item #5 swap embedder) before next release.
- **Option C** — defer release until a real bug surfaces in 0.1.2 stable
("first new bug + feature plus" pattern from §6.21).

Recommendation: Option A aligns with project's historical cadence. Final
call belongs to the human.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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