Skip to content

fix(cron): redact command output in delivery#95809

Closed
eleqtrizit wants to merge 1 commit into
openclaw:mainfrom
eleqtrizit:764
Closed

fix(cron): redact command output in delivery#95809
eleqtrizit wants to merge 1 commit into
openclaw:mainfrom
eleqtrizit:764

Conversation

@eleqtrizit

@eleqtrizit eleqtrizit commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Redacts sensitive and action-required command output before cron sends non-interactive announce or webhook delivery messages.

Changes

  • Added a cron delivery sanitizer that reuses the existing tool-output secret redactor and masks device-authorization prompt lines.
  • Applied the sanitizer at cron announce delivery and cron completion webhook payload boundaries.
  • Preserved stored cron run summaries/diagnostics for authorized inspection instead of changing command execution or run-log semantics.
  • Added announce and webhook regression tests with fake token/device-code examples.

Validation

  • node scripts/run-vitest.mjs src/cron/delivery.failure-notify.test.ts src/gateway/server-cron.test.ts src/cron/command-runner.test.ts src/cron/run-diagnostics.test.ts
  • corepack pnpm exec oxfmt --check --threads=1 src/cron/delivery-redaction.ts src/cron/delivery.failure-notify.test.ts src/cron/delivery.ts src/gateway/server-cron-notifications.ts src/gateway/server-cron.test.ts
  • git diff --check
  • .agents/skills/autoreview/scripts/autoreview --mode local

Notes

  • AI-assisted change.
  • No config, protocol, or changelog changes.
  • corepack pnpm tsgo:core was attempted but is blocked by an unrelated existing unused variable in src/config/sessions/session-accessor.ts:1545.
  • corepack pnpm format:check was attempted but is blocked by unrelated pre-existing formatting issues; scoped formatting for touched files passes.

@openclaw-barnacle openclaw-barnacle Bot added gateway Gateway runtime size: S maintainer Maintainer-authored PR labels Jun 22, 2026
@eleqtrizit

eleqtrizit commented Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Behavioral proof for the cron delivery hardening:

What was verified:

  • Primary cron announce delivery now sanitizes the message immediately before the durable channel send in src/cron/delivery.ts, via redactCronDeliveryText().
  • Completion webhook delivery now builds a delivery-safe payload in src/gateway/server-cron-notifications.ts, so webhook summary receives the same sanitization.
  • Stored cron run state remains unchanged for authorized inspection; the gateway regression asserts lastDiagnosticSummary still contains the original fake device-code output after webhook delivery sends a redacted payload.
  • The sanitizer uses the existing tool-output secret redactor and adds a narrow line-level matcher for device-authorization prompt lines. Ordinary status text stays intact.

Local behavioral proof run on this branch:

node scripts/run-vitest.mjs src/cron/delivery.failure-notify.test.ts src/gateway/server-cron.test.ts src/cron/command-runner.test.ts src/cron/run-diagnostics.test.ts
[test] passed 2 Vitest shards in 14.36s
Gateway shard: 2 files, 50 tests passed
Cron shard: 3 files, 23 tests passed

The new announce regression proves a message containing a fake device-login prompt and fake API key is delivered as:

stdout:
[redacted device authorization output]
OPENAI_API_KEY=sk-123…cdef
ordinary status

The new webhook regression proves the posted webhook payload redacts both the fake device code and fake key, and also proves the stored cron diagnostic summary still contains the original fake device code for run-history inspection.

Additional validation:

corepack pnpm exec oxfmt --check --threads=1 src/cron/delivery-redaction.ts src/cron/delivery.failure-notify.test.ts src/cron/delivery.ts src/gateway/server-cron-notifications.ts src/gateway/server-cron.test.ts
All matched files use the correct format.

git diff --check
(no output)

CI state checked on the PR:

  • Real behavior proof passed for the PR author/context gate.
  • Security and most Node lanes passed.
  • Current failing CI lanes are unrelated to this change: check-lint and check-prod-types both fail on the existing unused sessionStore in src/config/sessions/session-accessor.ts:1545, outside this PR's touched files.

No config, protocol, public API, or changelog changes were made.

@eleqtrizit

eleqtrizit commented Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot mentioned this pull request Jun 23, 2026
Open
@clawsweeper

clawsweeper Bot commented Jun 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 23, 2026, 10:25 AM ET / 14:25 UTC.

Summary
The PR adds a cron delivery sanitizer and applies it to command-summary announce delivery and completion webhook payloads while preserving stored cron diagnostics.

PR surface: Source +38, Tests +84. Total +122 across 5 files.

Reproducibility: yes. by source inspection: command stdout/stderr becomes result.summary, and current main sends that summary through announce and webhook delivery without redaction. I did not run tests because this review is read-only.

Review metrics: 1 noteworthy metric.

  • Outbound delivery surfaces: 2 sanitized. The PR changes both cron announce text and completion webhook summary payloads, which are user-visible delivery contracts.

Stored data model
Persistent data-model change detected: serialized state: src/gateway/server-cron.test.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Add real after-fix proof from an actual cron announce or webhook delivery path using fake token/device-code output, with private details redacted.
  • Narrow the generic /device URL matcher or note maintainer acceptance of that compatibility tradeoff.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR provides focused Vitest output and mocked gateway/fetch assertions, but no after-fix proof from a real cron announce or webhook delivery setup; please add terminal output, logs, or a recording with fake secrets only and redact private details before posting. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] The new sanitizer intentionally changes outbound cron text, so webhook consumers or channel recipients that currently depend on exact command output may see masked lines after merge.
  • [P1] The generic /device URL alternative can redact ordinary non-auth output lines that happen to mention a device endpoint.

Maintainer options:

  1. Tighten the device URL matcher (recommended)
    Narrow the generic /device match to known auth/device-code contexts or require nearby prompt language so ordinary device endpoint output is not masked.
  2. Accept broad redaction for safety
    Maintainers may decide that broad /device line redaction is an intentional security tradeoff, but that choice should be visible before merge.
  3. Wait for real delivery proof
    Pause merge until the contributor shows a real cron announce or webhook delivery with fake secret/device-code output and redacted after-fix results.

Next step before merge

  • [P1] The remaining actions are contributor real-proof follow-up and maintainer acceptance of the redaction breadth/protected-label handling, not a safe autonomous repair lane.

Security
Cleared: The diff adds outbound redaction only and does not change dependencies, workflows, permissions, package metadata, secret storage, or execution sources.

Review findings

  • [P3] Narrow generic device URL redaction — src/cron/delivery-redaction.ts:5
Review details

Best possible solution:

Keep the delivery-boundary redaction approach, but narrow or explicitly accept the generic device-URL masking and require a real cron delivery proof before merge.

Do we have a high-confidence way to reproduce the issue?

Yes by source inspection: command stdout/stderr becomes result.summary, and current main sends that summary through announce and webhook delivery without redaction. I did not run tests because this review is read-only.

Is this the best way to solve the issue?

Mostly yes: redacting at the outbound delivery boundary preserves stored diagnostics while preventing cron egress leaks. The /device URL heuristic is broader than the stated device-auth prompt scope and should be narrowed or accepted explicitly.

Full review comments:

  • [P3] Narrow generic device URL redaction — src/cron/delivery-redaction.ts:5
    This pattern treats any URL path ending in /device as a device-authorization prompt, so ordinary cron output like a device health-check URL would be replaced with [redacted device authorization output]. Please narrow this alternative to auth/device-code contexts or known login hosts so non-secret output is not suppressed.
    Confidence: 0.78

Overall correctness: patch is correct
Overall confidence: 0.84

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 273eed4c51cb.

Label changes

Label justifications:

  • P1: The PR addresses sensitive command output leaving cron delivery channels/webhooks, which can expose credentials to real recipients.
  • merge-risk: 🚨 compatibility: Existing cron webhook or announce consumers may receive masked text instead of exact command output after this change.
  • merge-risk: 🚨 message-delivery: The new /device URL heuristic can suppress non-secret output lines in delivered cron messages.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR provides focused Vitest output and mocked gateway/fetch assertions, but no after-fix proof from a real cron announce or webhook delivery setup; please add terminal output, logs, or a recording with fake secrets only and redact private details before posting. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +38, Tests +84. Total +122 across 5 files.

View PR surface stats
Area Files Added Removed Net
Source 3 40 2 +38
Tests 2 85 1 +84
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 5 125 3 +122

What I checked:

  • Repository policy read: Root AGENTS.md and src/gateway/AGENTS.md were read fully; their deep-review, security/proof, and gateway-test guidance shaped this review. (AGENTS.md:18, 273eed4c51cb)
  • Current announce leak path: Current main passes cron announce text directly to durable channel payloads without a delivery-boundary redaction step. (src/cron/delivery.ts:117, 273eed4c51cb)
  • Current webhook leak path: Current main posts the full CronEvent as the completion webhook payload when evt.summary exists, so command summaries can leave through webhook delivery. (src/gateway/server-cron-notifications.ts:270, 273eed4c51cb)
  • Command summary source: Command cron jobs build deliverable summaries directly from stdout/stderr and keep those values in diagnostics, matching the PR's decision to redact only outbound delivery. (src/cron/command-runner.ts:27, 273eed4c51cb)
  • PR patch surface: The diff adds redactCronDeliveryText, uses it in announce delivery and completion webhook payloads, and adds focused announce/webhook regression coverage. (src/cron/delivery-redaction.ts:1, b7c0d728d565)
  • Proof assessment: The contributor supplied a copied Vitest run and mocked gateway/fetch proof, but no real cron job/channel or real webhook receiver output from an actual setup.

Likely related people:

  • mbelinky: GitHub file history shows feat(cron): support command jobs introduced command-backed cron summaries, which are the source of this delivery payload. (role: introduced behavior; confidence: high; commits: b8adc11977ab; files: src/cron/command-runner.ts)
  • steipete: Recent history shows repeated cron delivery and notification routing work, including splitting notification routing and consolidating message delivery APIs. (role: area owner by history; confidence: high; commits: 85148f3b2099, a4b17d65a8ff; files: src/cron/delivery.ts, src/gateway/server-cron-notifications.ts, src/cron/delivery.failure-notify.test.ts)
  • vincentkoc: Recent GitHub history shows a cron failure-destination delivery fix touching both cron delivery and gateway notification files shortly before this PR. (role: recent area contributor; confidence: medium; commits: 28b374a8a7f6; files: src/cron/delivery.ts, src/gateway/server-cron-notifications.ts, src/cron/delivery.failure-notify.test.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. labels Jun 23, 2026
This was referenced Jun 24, 2026
Closed
Closed
@Pluviobyte Pluviobyte mentioned this pull request Jun 24, 2026
Closed
@wanyongstar wanyongstar mentioned this pull request Jun 24, 2026
Closed
This was referenced Jun 24, 2026
Closed
Merged
@eleqtrizit eleqtrizit closed this Jun 30, 2026
@eleqtrizit
eleqtrizit deleted the 764 branch June 30, 2026 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

gateway Gateway runtime maintainer Maintainer-authored PR merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. P1 High-priority user-facing bug, regression, or broken workflow. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@eleqtrizit