Skip to content

fix(kanban): redact the card body in kanban_create before persisting - #92366

Open
liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-92354
Open

liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-92354

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

What does this PR do?

kanban_create was the only one of the five kanban write paths that persisted free text verbatim: _handle_create passed body straight into create_task(...), while _handle_comment, _handle_complete, _handle_block, and the review path all call redact_sensitive_text(..., force=True) first. A credential pasted into a card body (e.g. an API key a worker needs to complete the task) therefore landed in plaintext in the board's kanban.db — and board SQLite files are routinely snapshotted/backed up, widening exposure.

This PR mirrors the _handle_comment contract at the _handle_create boundary: the body is run through redact_sensitive_text(str(body), force=True) before the DB write. Secret-free text passes through unchanged, so normal task descriptions are unaffected.

Related Issue

Fixes #92354

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • 🔒 Security fix

Changes Made

How to Test

  1. pytest tests/tools/test_kanban_redaction.py -q — 7 passed (5 pre-existing + 2 new). Observed result: test_kanban_create_body_scrubbed_api_key fails on unpatched main (the secret reads back verbatim) and passes with this change.
  2. pytest tests/tools/test_kanban_tools.py -q — 32 passed, no regressions on the rest of the create path (validation, parents, tenant, session stamping).

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate (fix(kanban): redact secrets in complete_task/block_task/add_comment #83733 covers complete/block/schedule/comment/review redaction at the DB layer; it does not touch the create_task body path this PR fixes)
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass (relevant suites: tests/tools/test_kanban_redaction.py, tests/tools/test_kanban_tools.py)
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15.5 (arm64)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — N/A (inline comment names the contract and the issue)
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — N/A (pure string transform at a tool boundary)
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Screenshots / Logs

N/A — DB assertions in the regression tests cover the persisted shape.

kanban_create was the only one of the five kanban write paths that
skipped redact_sensitive_text: _handle_create passed the body straight
into create_task, so a credential pasted into a card body persisted in
plaintext in the board's SQLite (and board DBs are routinely
snapshotted). Mirror the _handle_comment contract — force=True redaction
at the tool boundary; secret-free text passes through unchanged.

Fixes NousResearch#92354
@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Aug 22, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor Author

The single e2e failure on this PR is a known flaky test, unrelated to this change:

Failed test: tests/e2e/test_platform_commands.py::TestSlashCommands::test_plaintext_restart_gateway_in_group_stays_plain_text[telegram]AssertionError: Expected 'mock' to have been called once. Called 0 times. (60 passed, 7 skipped otherwise in that suite).

Why it can't be this PR: the change touches only tools/kanban_tools.py (_handle_create body redaction) and tests/tools/test_kanban_redaction.py. The failing test exercises the Telegram gateway restart / plaintext-restart path — no shared code path, module, or fixture with the kanban tool layer.

Known-flake evidence: this exact signature (send.assert_called_once → "Called 0 times" after a gateway restart) is timing-sensitive and has failed across many unrelated open PRs — e.g. #90737, #91543, #92130, #86168, #81067, #90321, #90128 all reference test_plaintext_restart_gateway_in_group_stays_plain_text — the classic "same failure on unrelated PRs = pre-existing main flake" pattern (previously triaged on #90737 / #90564 / #91228).

All checks relevant to this diff pass locally: tests/tools/test_kanban_redaction.py (7 passed, including the two new regressions) and tests/tools/test_kanban_tools.py (32 passed). The new test_kanban_create_body_scrubbed_api_key fails on unpatched main and passes with this change.

@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

Reviewed tools/kanban_tools.py (+53) and tests/tools/test_kanban_redaction.py.

What's good

  • Closes the last unredacted write path ([Bug]: kanban_create body skips redact_sensitive_text — only write path of 5 that persists secrets in plaintext #92354): _handle_create now applies the same redact_sensitive_text(force=True) contract as comment/complete/block, with a comment enumerating the sibling paths so the next write-path author knows the rule is structural, not local taste.
  • Tests cover both directions — masked persistence of an sk- key (asserting the secret is gone from the stored row and that the head/tail mask survives for debuggability) and byte-for-byte passthrough of secret-free bodies, which pins that force=True doesn't mangle ordinary prose.
  • Reading the row back through kb.get_task (not the tool's own return value) verifies what actually persisted, which is the assertion that matters for a data-at-rest fix.

Suggestions

  1. Title isn't redacted. Bodies were the reported leak, but title persists verbatim too and is more visible (board listings, dispatcher logs, notifications). A key pasted as "rotate sk-… gateway key" would survive. If redacting titles risks mangling task-matching heuristics, say so in the comment; otherwise extend the same call to title.
  2. Consider a small module-level constant or helper (_redact_persisted_text) shared by all five write paths — five identical calls invite a sixth that forgets.
  3. Nit: the docstring/comment says board DBs are "routinely snapshotted" — good threat-model context; consider also mentioning tenant-shared boards where cross-worker visibility is the norm.

Correct, focused security fix.

…n listings

Review follow-up on NousResearch#92354: the body fix left titles persisting verbatim,
and titles are the MORE visible free text (board listings, dispatcher
logs, notifications). Same force=True contract; task dispatch keys off
the task id, not the title, so matching heuristics are unaffected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: kanban_create body skips redact_sensitive_text — only write path of 5 that persists secrets in plaintext

3 participants