feat(slack): daily channel-privacy audit backstop (#2849)#2924
Merged
Conversation
#2735 added a send-time recheck so a channel that drifted private → public stops receiving sensitive posts on the first send after the drift. Channels that sit idle between writes could linger in a drifted state until someone tries to post, so the fix only handled the hot path. Adds a daily audit that proactively checks each of the six admin-settings channels (billing, escalation, admin, prospect, error, editorial) against Slack and emits a structured `channel_privacy_drift_audit` log on drift or unverifiable states. When drift is found, posts a summary to `admin_slack_channel` — unless that channel itself drifted, in which case the summary is suppressed so we don't leak drift details into the now-public channel. Non-destructive: the audit does NOT auto-null settings. The send-time gate (#2735) is the enforcement path; this job is pure observability. Log aggregation alerting should key on `event: 'channel_privacy_drift_audit'`. Registered with the existing jobScheduler at 24h interval / 10min initial delay. Reuses `verifyChannelStillPrivate` + the `sendChannelMessage({ requirePrivate: 'strict-public-only' })` mode from #2861. 9 unit tests cover the orchestration logic: unconfigured channels skipped, admin-channel self-drift suppresses the summary (the core #2849 acceptance), throws collapse to 'unknown', non-destructive behavior pinned against future refactors. Typecheck clean, 1923 server unit tests pass, 631 root unit tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
All Should-Fix items from the code / security / testing reviewers: - Suppress the admin summary when admin_slack_channel is 'unknown', not just 'public'. Security reviewer correctly flagged the 'strict-public-only' mode lets 'unknown' through — narrow window where the admin channel could actually be public while Slack info returns unverifiable. The #2849 spec explicitly prioritizes "not using the drifted channel as the notification surface" and structured-log aggregation is the documented backstop. Now the summary is attempted only when admin is confirmed 'private' AND sent with the strict `requirePrivate: true` gate (belt-and-braces against a drift that raced between the audit loop and the send). (security Should-Fix) - Sanitize the `err` payload in the verify-threw log — use `err instanceof Error ? .message : String(err)` so pg / library errors don't spill `.query` / `.parameters` through pino's default err serializer. Mirrors the pattern from #2830. (code Should-Fix) - Tighten `shouldLogResult` type in job-definitions.ts by importing the exported `ChannelPrivacyAuditResult` instead of the inline structural shape. (code Should-Fix) - Spy on the logger in tests and assert the structured `channel_privacy_drift_audit` record fires even when the summary is suppressed — it's the only remaining alert signal in those cases and wasn't being verified. (testing Should-Fix) - Pin the admin-'unknown' + billing-drift branch with an explicit test: confirms the summary is suppressed, both bucket assignments are correct, and the structured log captures the drift. (testing Should-Fix) - New test for multi-drift fan-out (billing + editorial both drifted → single summary mentioning both) to guard against a future refactor that sends per-channel posts. (testing Nice-to-Have) - New test for admin + billing both drifted → summary suppressed, structured log still carries BOTH settings. Distinct from the admin-only case because it proves suppression doesn't accidentally drop billing from the drifted array. (testing Nice-to-Have) - Renamed the non-destructive test for clarity: "does not write to the drifted setting — audit is pure observability". (testing Nice-to-Have) 12 scenarios total (up from 9), 1932 server + 631 root unit tests pass, typecheck clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Expert pass complete — no Must-Fix. All Should-Fix addressed: Security:
Code:
Testing:
12 scenarios total (up from 9). 1932 server + 631 root unit tests pass, typecheck clean. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #2849 — backstop for #2735.
#2735 catches channel-privacy drift at send time. A channel that flipped private → public stops receiving sensitive posts on the first send after the drift. What it misses: channels that sit idle between writes. This job runs once a day, checks each of the six configured admin-settings channels (billing / escalation / admin / prospect / error / editorial) against Slack, and emits a structured `channel_privacy_drift_audit` log.
What changed
Test plan
🤖 Generated with Claude Code