Skip to content

fix(observability): redact secret fields in Sentry field-only log summaries - #1661

Closed
JSONbored wants to merge 1 commit into
mainfrom
codex/propose-fix-for-sentry-log-summary-leak
Closed

fix(observability): redact secret fields in Sentry field-only log summaries#1661
JSONbored wants to merge 1 commit into
mainfrom
codex/propose-fix-for-sentry-log-summary-leak

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • Field-only structured logs were being summarized into the synthetic Sentry Error.message without applying the repository's secret-key redaction, which could expose short secrets (API keys, passwords, tokens) in issue titles/notifications.
  • The existing scrubEvent() scrubber only covered request.headers, contexts, and extra, so the new summarization path duplicated unredacted scalar fields into the exception value and bypassed the privacy boundary.

Description

  • Route every summarized field through a new summarizeLogFieldValue() helper that applies SECRET_KEY checks and redacts top-level secret-like keys before formatting.
  • For nested/object values, reuse the existing scrubEvent() on a structuredClone of the value and then JSON.stringify() the scrubbed result so nested secret keys are also redacted.
  • Add a regression unit test that asserts scalar and nested secret-keyed fields are redacted in the synthesized Sentry message while safe fields remain visible.

Testing

  • Ran npm run typecheck, which completed successfully with no type errors.
  • Ran the unit tests for the modified file with npx vitest run test/unit/selfhost-sentry.test.ts, and the suite passed (27 tests passed).
  • Attempted coverage and full local gate runs: npm run test:coverage encountered a coverage-remapping provider error (TypeError: jsTokens is not a function) after tests completed, and npm run test:ci was blocked by actionlint setup network/fallback issues in this environment, while npm audit --audit-level=moderate was blocked by the registry audit endpoint returning 403.

Codex Task

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

Copy link
Copy Markdown
Owner Author

Closing as superseded by #1662. Both PRs target the same Sentry field-only summary leak, and #1662 covers the scalar case plus nested object/array redaction while preserving the title shape that omits secret-keyed top-level fields. Keeping both would duplicate and conflict in src/selfhost/sentry.ts and test/unit/selfhost-sentry.test.ts.

@JSONbored JSONbored closed this Jun 28, 2026
@codecov

codecov Bot commented Jun 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.52%. Comparing base (2c6241b) to head (587fd46).
⚠️ Report is 7 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1661   +/-   ##
=======================================
  Coverage   95.52%   95.52%           
=======================================
  Files         204      204           
  Lines       22196    22199    +3     
  Branches     8025     8027    +2     
=======================================
+ Hits        21202    21205    +3     
  Misses        415      415           
  Partials      579      579           
Files with missing lines Coverage Δ
src/selfhost/sentry.ts 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@loopover-orb

loopover-orb Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review — held for maintainer review

2 files · 1 AI reviewers · 1 blocker · readiness 48/100 · CI green · blocked

⏸️ Held for maintainer review — Readiness score is below the configured threshold — Address the short explicit PR panel actions, then re-run the gate.

Review summary
This change correctly moves field-only Sentry title summarization through the same secret-key scrubber used for outbound Sentry event data. Top-level secret-like scalar keys are redacted before formatting, nested object values are cloned and scrubbed before stringification, and the regression test exercises both paths while preserving safe fields. The implementation is narrow and matches the existing privacy boundary.

Concerns raised — review before merging

  • Readiness score is below the configured threshold — Address the short explicit PR panel actions, then re-run the gate.
Signal Result Evidence
Code review ❌ 1 blocker 1 reviewers, synthesized
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ⚠️ 2 scoped overlaps Top overlaps are listed below; lower-confidence bulk is hidden.
Review load ❌ 8/20 Readiness component derived from cached public PR metadata and labels; size label size:XS.
Validation evidence ❌ 5/25 Cached preflight status is hold.
Open PR queue ❌ 3/10 28 open PR(s), 15 likely reviewable, 13 unlinked.
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 80 PR(s), 290 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Nits — 5 non-blocking
  • nit: src/selfhost/sentry.ts:143 depends on `structuredClone()` for object summaries; that is fine for the JSON-parsed structured-log path, but a short comment would make clear this helper is not meant for arbitrary non-cloneable runtime objects.
  • nit: test/unit/selfhost-sentry.test.ts:318 covers nested object redaction, but not arrays containing secret-keyed objects; adding that case would lock down the recursive array path in `scrubEvent()`.
  • src/selfhost/sentry.ts:143: document that summarized values originate from `JSON.parse()` so `structuredClone(value)` is expected to be safe here.
  • test/unit/selfhost-sentry.test.ts:318: extend the regression payload with an array element like `{ sessionToken: "nested" }` if you want explicit coverage for array recursion.
  • Pull request duplicates other open work — Check for an existing pull request or issue covering this change and coordinate or consolidate before continuing.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
  • Official Gittensor activity: 80 PR(s), 290 issue(s).
  • Related work: Titles/paths share 8 meaningful terms. (PR #1662)
  • Related work: Titles/paths share 6 meaningful terms. (PR #1658, PR #1662)
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Review top overlaps.
  • Add scope summary.
  • Fix blocker.
  • Expect slower review.
  • Refresh registry data or choose a registered active repo.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
  • Check active issues and PRs before submitting.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Review load = cached public PR metadata such as size labels, changed paths, and preflight status.
  • Open PR queue = repo-wide review pressure; it is not a PR quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
Review details

Generated from public PR metadata and the diff. Advisory only; deterministic signals remain authoritative.

This change correctly moves field-only Sentry title summarization through the same secret-key scrubber used for outbound Sentry event data. Top-level secret-like scalar keys are redacted before formatting, nested object values are cloned and scrubbed before stringification, and the regression test exercises both paths while preserving safe fields. The implementation is narrow and matches the existing privacy boundary.

Nits (4)

  • nit: src/selfhost/sentry.ts:143 depends on `structuredClone()` for object summaries; that is fine for the JSON-parsed structured-log path, but a short comment would make clear this helper is not meant for arbitrary non-cloneable runtime objects.
  • nit: test/unit/selfhost-sentry.test.ts:318 covers nested object redaction, but not arrays containing secret-keyed objects; adding that case would lock down the recursive array path in `scrubEvent()`.
  • src/selfhost/sentry.ts:143: document that summarized values originate from `JSON.parse()` so `structuredClone(value)` is expected to be safe here.
  • test/unit/selfhost-sentry.test.ts:318: extend the regression payload with an array element like `{ sessionToken: "nested" }` if you want explicit coverage for array recursion.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@loopover-orb loopover-orb Bot added gittensor gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. labels Jun 28, 2026
@JSONbored
JSONbored deleted the codex/propose-fix-for-sentry-log-summary-leak branch June 29, 2026 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant