Skip to content

Harden worker secret logging#2784

Merged
jesse-merhi merged 19 commits into
mainfrom
jesse/ghsa-scan-worker-url-redaction
Jun 25, 2026
Merged

Harden worker secret logging#2784
jesse-merhi merged 19 commits into
mainfrom
jesse/ghsa-scan-worker-url-redaction

Conversation

@jesse-merhi

@jesse-merhi jesse-merhi commented Jun 23, 2026

Copy link
Copy Markdown
Member

Summary

This hardens ClawHub production worker logging so signed artifact URLs and auth headers do not move through GitHub Actions logs, diagnostics artifacts, or persisted Convex failure fields. The worker-side cleanup is intentionally narrow: it strips URL/auth transport leak paths, masks exact runtime values in GitHub Actions, and structurally redacts diagnostics content instead of trying to detect arbitrary secrets. TruffleHog remains the broad verified-secret detector before diagnostics artifacts can be uploaded.

What Changed

  • Shared worker logging: added a Pino logger with fixed structured redaction paths for known worker fields such as auth headers, artifact URLs, stdout/stderr, raw scanner output, and error fields.
  • Shared worker redaction: workerRedaction now handles URL cleanup, bearer/basic auth header cleanup, safe artifact path labels, and GitHub Actions exact-value masking. It does not maintain generic token/API-key/provider-key regex detection.
  • Security scan worker: download failures report safe artifact identity and status, lifecycle/failure logs are structured, dynamic artifact URLs are masked in GitHub Actions, diagnostics content fields are summarized, and failCodexScanJob receives sanitized error text.
  • Convex defense-in-depth: scan failure persistence still removes signed URLs and auth headers from worker errors before writing job/request/GitHub scan failure fields or worker-failure analysis text.
  • Skill Card worker: file download failures, caught errors, structured logs, claim failures, and failSkillCardJob arguments use the same narrow logging and masking boundary.
  • Worker workflows: runtime secrets are scoped only to the auth/worker steps that need them, and security scan diagnostics run through a tag-and-digest pinned TruffleHog verified-secret filesystem scan before upload. Diagnostics upload is skipped if that scan fails.
  • Regression guard: production worker scripts have a test guard against raw console.log, console.warn, or console.error creeping back into job lifecycle/failure paths.

Proof

Behavioral proof:

  • Failed security scan and Skill Card artifact downloads preserve useful context like job id, artifact path, HTTP status, retry decision, worker id/shard, and elapsed time without logging the signed URL or signing query parameters.
  • Text cleanup removes full http(s) URLs and bearer/basic auth headers, but deliberately leaves generic token-looking text to TruffleHog instead of duplicating secret-detection rules.
  • Diagnostic JSON preserves explicit metadata keys such as type, status, verdict, and severity; content-like and unknown string fields are structurally summarized.
  • Exact GitHub Actions ::add-mask:: commands are emitted for dynamic artifact URLs and known runtime worker secrets before later logs are written.
  • TruffleHog runs before actions/upload-artifact for security scan diagnostics, and upload is conditioned on the scan step succeeding.

Reviewer-checkable examples:

Before: Download failed 403: https://signed.example.invalid/file?token=secret&X-Amz-Signature=abc123
After:  Download failed 403 for artifact file SKILL.md
Mask command: ::add-mask::https://signed.example.invalid/file?token=secret&X-Amz-Signature=abc123
Later log:    {"event":"security_scan_job_failed","reason":"Download failed 403 for artifact file SKILL.md",...}
Diagnostics gate: docker run ghcr.io/trufflesecurity/trufflehog:3.95.5@sha256:56c25710275c4b8d74c4f1346a5e7c606fa7ff4afe996f680b288d0fae3fcd9c filesystem /scan --only-verified --fail --no-update --github-actions

Verification

Automated checks:

  • GitHub PR checks passed: static, unit, types-build, packages, e2e-http, playwright-smoke, all playwright-local-auth shards, both verified-secret scans, Vercel, dispatch, and auto-response. CodeQL Light Analyze is skipped as expected for this matrix.
  • Local worker log canary captured security-scan and Skill Card worker stdout, stderr, safe action summaries, and diagnostics for signed-URL/download-failure paths. Known fixture values for signed URLs, signing query params, worker token, API key, and lease token were absent from the capture; pinned TruffleHog verified scan exited 0; pinned TruffleHog broad local scan with --no-verification --results=verified,unknown,unverified exited 0.
  • bun run test -- scripts/lib/workerRedaction.test.ts scripts/lib/workerLogger.test.ts scripts/security/run-codex-scan-worker.test.ts scripts/skill-cards/run-skill-card-worker.test.ts scripts/security/security-scan-worker-workflow.test.ts scripts/skill-cards/skill-card-worker-workflow.test.ts scripts/worker-console-guard.test.ts convex/securityScan.test.ts passed: 8 files, 97 tests.
  • bun run ci:unit passed: 288 files passed, 3697 tests passed, 1 skipped; coverage above threshold.
  • bun run format:check -- convex/securityScan.ts convex/securityScan.test.ts scripts/lib/workerRedaction.ts scripts/lib/workerRedaction.test.ts scripts/security/run-codex-scan-worker.ts scripts/security/run-codex-scan-worker.test.ts scripts/skill-cards/run-skill-card-worker.ts passed.
  • bunx oxlint --type-aware --tsconfig ./tsconfig.oxlint.json convex/securityScan.ts convex/securityScan.test.ts scripts/lib/workerRedaction.ts scripts/lib/workerRedaction.test.ts scripts/security/run-codex-scan-worker.ts scripts/security/run-codex-scan-worker.test.ts scripts/skill-cards/run-skill-card-worker.ts passed.
  • actionlint .github/workflows/security-scan-codex.yml .github/workflows/skill-card-worker.yml passed.
  • bunx tsc -p convex/tsconfig.json --noEmit passed.
  • bunx tsc -p packages/schema/tsconfig.json --noEmit passed.
  • bunx tsc -p packages/clawhub/tsconfig.json --noEmit passed.
  • git diff --check passed.

Local-only baseline observations:

  • bun run ci:static failed locally on unrelated oxlint findings in convex/search.test.ts:2288 and convex/skills.resolve.test.ts:21, but the GitHub static check passed on this PR head.
  • bun run ci:types-build failed locally on the known unrelated root TypeScript issue in e2e/prod-http-smoke.e2e.test.ts:94: Cannot find name 'lastError', but the GitHub types-build check passed on this PR head.

Screenshots

No screenshots are included because this is backend worker, Convex persistence, and CI workflow hardening with no reviewer-visible UI change.

@vercel

vercel Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
clawhub Ready Ready Preview, Comment Jun 25, 2026 1:33am

Request Review

@jesse-merhi jesse-merhi changed the title Redact scan worker artifact errors Harden worker secret logging Jun 23, 2026
@socket-security

socket-security Bot commented Jun 23, 2026

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedpino@​10.3.19910010090100

View full report

@blacksmith-sh

This comment has been minimized.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Jun 23, 2026
@clawsweeper

clawsweeper Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed June 24, 2026, 9:26 PM ET / 01:26 UTC.

Summary
The branch adds shared worker redaction/logging helpers, applies them to security-scan and Skill Card worker logs and Convex failure persistence, scopes worker secrets in two workflows, gates diagnostics upload with TruffleHog, adds Pino, and adds regression tests.

Reproducibility: yes. Source inspection on current main shows failed worker downloads can include raw signed URLs and raw worker errors can flow into logs or Convex failure fields; I did not run a live production worker in this read-only review.

Review metrics: 3 noteworthy metrics.

  • Diff Size: +1788/-168 across 20 files. The patch spans runtime workers, Convex persistence, workflows, dependencies, and tests, so it needs multi-surface review.
  • Production Workflows Changed: 2 workflows changed. Both scheduled worker workflows change secret scoping or diagnostics behavior outside ordinary unit-test coverage.
  • Direct Runtime Dependency: 1 added: pino@10.3.1. A new runtime logger affects production worker supply-chain and log-redaction behavior before merge.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🌊 off-meta tidepool
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

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

Rank-up moves:

  • [P2] Have a worker/security owner confirm the intended redaction and diagnostics artifact boundary before merge.

Risk before merge

  • [P1] Maintainers should explicitly accept the intended redaction boundary: URL/auth transport cleanup, fixed structured paths, key-value cleanup, and reliance on TruffleHog rather than generic token-looking text detection.
  • [P1] The workflow change adds a pinned Docker-based diagnostics secret scan before artifact upload and narrows job-level secret scoping, so production runner behavior and operator expectations need review beyond green CI.

Maintainer options:

  1. Finish Worker Security Review (recommended)
    Have a worker/security owner confirm the redaction paths, generic-secret non-goals, persisted failure sanitization, and diagnostics upload gate before merge.
  2. Document The Boundary First
    Add a focused spec note for the worker log, diagnostics artifact, and persisted failure redaction contract if maintainers want the security intent captured before landing.
  3. Split The Diagnostics Gate
    If the TruffleHog Docker diagnostics gate needs separate operator rollout, split workflow artifact gating from the worker redaction and Convex persistence hardening.

Next step before merge

  • [P2] Human security/operator review is the next action because the PR changes production redaction, persisted failure fields, a runtime logger dependency, and workflow diagnostics behavior; no narrow automation repair was found.

Security
Cleared: No concrete security or supply-chain regression was found; the remaining concern is maintainer acceptance of the new worker redaction, dependency, and diagnostics boundary.

Review details

Best possible solution:

Land the centralized redaction, structured logging, persistence sanitization, and diagnostics scan gate after worker/security owners accept the redaction non-goals and workflow artifact boundary, with a focused spec note if maintainers want the contract recorded.

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

Yes. Source inspection on current main shows failed worker downloads can include raw signed URLs and raw worker errors can flow into logs or Convex failure fields; I did not run a live production worker in this read-only review.

Is this the best way to solve the issue?

Yes. The PR addresses the implicated layers: shared cleanup, structured worker logging, download error text, diagnostics redaction, workflow secret scope, and Convex persistence; the remaining question is maintainer acceptance of the security/operator boundary.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 3ef5f14d84a7.

Label changes

Label justifications:

  • P2: Worker secret logging hardening is a normal-priority security improvement with limited operational surface, not an active outage or emergency.
  • merge-risk: 🚨 security-boundary: Merging changes how production workers redact, mask, persist, and upload potentially secret-bearing diagnostics, which CI cannot fully prove as a security boundary.
  • merge-risk: 🚨 automation: The diff changes production GitHub Actions worker secret scoping and diagnostics artifact upload gating, so runner behavior needs maintainer attention beyond green checks.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The external-contributor proof gate does not apply to this MEMBER-authored backend and CI hardening PR, though the PR body includes runtime canary and CI verification details.
Evidence reviewed

What I checked:

  • Repository policy read: AGENTS.md was read fully; its security-sensitive intent and validation guidance affected the manual-review routing for this worker secret-handling change. (AGENTS.md:9, 3ef5f14d84a7)
  • Live PR state: GitHub reports the PR open, non-draft, MEMBER-authored, mergeable but blocked, at head 26a2a7b with 20 files changed and +1788/-168. (26a2a7b2893e)
  • Current-main hazard: Current main throws failed Skill Card artifact downloads with the raw signed URL in the error message, which is then forwarded through worker failure handling. (scripts/skill-cards/run-skill-card-worker.ts:135, 3ef5f14d84a7)
  • Current-main persistence hazard: Current main persists raw worker failure strings by slicing args.error in security scan failure records, so upstream worker errors can reach Convex fields without a shared worker-boundary sanitizer. (convex/securityScan.ts:1798, 3ef5f14d84a7)
  • Shared redaction implementation: The PR adds shared URL/auth-header and key-value worker text redaction used by both scripts and Convex failure persistence. (convex/lib/workerTextRedaction.ts:7, 26a2a7b2893e)
  • Worker integration: The security worker masks dynamic artifact URLs before fetch, reports download failures with safe artifact identity, and sanitizes process failure text before logging or failing the Convex job. (scripts/security/run-codex-scan-worker.ts:470, 26a2a7b2893e)

Likely related people:

  • Patrick-Erichsen: Authored or merged the prior Codex diagnostics persistence and Skill Card generation work, and shortlog shows the heaviest history across the touched worker, Convex, and workflow paths. (role: feature owner and recent area contributor; confidence: high; commits: 01e4418cccae, b62d8ca81345, 8841ac777196; files: scripts/security/run-codex-scan-worker.ts, scripts/skill-cards/run-skill-card-worker.ts, convex/securityScan.ts)
  • steipete: Authored the merged PR that routed ClawScan classification through the queued GitHub Actions Codex worker and established the production worker credential boundary. (role: introduced behavior; confidence: medium; commits: 35aa372b24a3; files: convex/securityScan.ts, scripts/security/run-codex-scan-worker.ts, .github/workflows/security-scan-codex.yml)
  • Vincent Koc: Recently changed worker-related GitHub Actions behavior for Blacksmith runner bursts, adjacent to this PR's workflow and automation-risk surface. (role: recent automation contributor; confidence: medium; commits: 01bc23c0c725; files: .github/workflows/security-scan-codex.yml, .github/workflows/skill-card-worker.yml)
  • vyctorbrzezowski: Previously changed local Codex worker gating, which is adjacent to this PR's worker runtime and secret-environment boundary. (role: adjacent worker contributor; confidence: low; commits: 74aa61086e69; files: scripts/security/run-codex-scan-worker.ts, scripts/skill-cards/run-skill-card-worker.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.

@jesse-merhi jesse-merhi marked this pull request as ready for review June 24, 2026 16:04
@jesse-merhi jesse-merhi requested a review from a team as a code owner June 24, 2026 16:04
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. label Jun 24, 2026
@jesse-merhi jesse-merhi merged commit 2deb1b7 into main Jun 25, 2026
27 of 28 checks passed
@jesse-merhi jesse-merhi deleted the jesse/ghsa-scan-worker-url-redaction branch June 25, 2026 01:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal backlog priority with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant