fix(logging): remove PII/pastoral-note debug logging from src (F5) - #84
Merged
Merged
Conversation
F5 (Medium, 2026-09-12 auth security review): member PII and pastoral
notes were written to server logs at info level via console.log/debug.
Hosting/log-aggregation platforms retain this with broader access and
longer retention than the MP database itself.
Removed (all console.log/.debug/.info in non-test, non-script src/):
- table.service.ts: table name, $filter query params, and full fetched
result sets (contact names/emails/phones, dp_Users rows, Notes)
- http-client.ts: PUT request debug log (full URL + JSON body)
- client.ts: token-validity chatter ("Checking token validity...",
expiry timestamps, "Token refreshed")
- procedure.service.ts: procedure name/params/results dumps
- proxy.ts: per-request path logging ("Allowing public path",
"Allowing request to", "Redirecting to signin")
- signin/page.tsx: client-side callbackUrl/redirect-state logging
- contact-logs/actions.ts + contactLogService.ts: full Notes/record
JSON.stringify dumps on create/update/delete, plus success chatter
- contact-logs.tsx: client-side create/update payload logging
Kept and made safe (identifiers/shape only, never content):
- table.service.ts / procedure.service.ts / client.ts: catch blocks
still log via console.error(message, error) - the error object itself
no longer carries any response body or full URL (see http-client.ts
below), so this was already safe once that boundary was fixed.
- http-client.ts: GET/PUT failure logs now emit
{ method, endpoint (path only, no query string), status, statusText }
- no responseBody, no full URL. The GET failure's thrown Error message
no longer appends the response text (it could echo $filter or record
content back); PUT's thrown message never included it.
- proxy.ts: session-check catch now logs
error instanceof Error ? error.message : String(error) instead of the
raw error.
- lib/auth.ts: resolveMpUserId's catch now logs
{ userGuid, err: err instanceof Error ? err.message : String(err) }
instead of the raw err object (userGuid is an identifier, not content).
- getUserInfo's response.status log and the auth.userinfo.invalid_sub
structured error were already safe; unchanged.
- The four structured events (mp.read.unauthorized, mp.write.unauthorized
in authorizationService.ts, mp.write.non_user in
sessionContextService.ts, auth.userinfo.invalid_sub in auth.ts) are
unchanged - they already log identifiers only.
Tests:
- Updated no existing test asserted on a removed console.log call or on
the old GET-failure message format (all pre-existing assertions used
only status/statusText, never response text), so none needed rewriting.
- Added negative "Logging safety (F5)" coverage:
- table.service.test.ts: fetching records logs nothing on success; a
failure's console.error output never contains the $filter value or
record content.
- http-client.test.ts: a successful GET logs nothing; a failed GET's
log and thrown message never contain the response body, full URL, or
query string; a failed PUT's log never contains the request body.
- contactLogService.test.ts / contact-logs/actions.test.ts: creating,
updating, and deleting a contact log never logs Notes, including on
a forced failure.
- proxy.test.ts: added a non-Error-thrown case to cover both arms of
the safe-error-logging ternary.
Verification: npx vitest run --coverage (912 tests, all thresholds met:
statements 99.54%, branches 96.66%, functions 99.27%, lines 99.72%);
npx tsc --noEmit -p tsconfig.json (clean); npm run lint (clean).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- .claude/references/auth.md: added a "Logging policy" note under the
Authorization section (no debug/info logging in src/; errors log
identifiers and shape only, never MP record content, filters, or
request bodies; the four structured events remain the greppable
contract) and marked F5 closed (2026-09-12) in the Closed findings
table.
- CLAUDE.md: added item 12 under Key Development Practices restating
the no-debug-logging rule for future contributors/agents.
- eslint.config.mjs: added a scoped "no-console": ["error", { allow:
["warn", "error"] }] rule over src/**/*.{ts,tsx}, with an override
exempting src/lib/providers/ministry-platform/scripts/** (dev-only
codegen CLI) and *.test.{ts,tsx} files. Verified npm run lint passes
cleanly with no unrelated fallout, and that the rule actually fires
(manually added a console.log to a src file, confirmed eslint flagged
it, then reverted).
Verification: npm run lint (clean); npx tsc --noEmit -p tsconfig.json
(clean).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
Finding
F5 (Medium), from the 2026-09-12 auth security review: member PII and pastoral notes were written to server logs at info level (
console.log/.debug/.info), including full MP result sets, request bodies, and$filterquery strings. Hosting/log-aggregation platforms retain this with broader access and longer retention than the MP database itself.Policy
console.log,.debug,.info) from non-test source undersrc/, except the generator scripts undersrc/lib/providers/ministry-platform/scripts/(CLI tools, left alone).console.error/console.warnin catch blocks now log identifiers and shape only (table name, status, endpoint path, error message) — never MP result sets, request bodies,Notes, emails, phones, names, or a URL/query string containing$filter.Files touched
src/lib/providers/ministry-platform/services/table.service.ts$filterparams, full fetched result setsconsole.error(message, error)in each catch — safe now that the error object never carries a body/URL (see http-client.ts)src/lib/providers/ministry-platform/utils/http-client.ts{ method, endpoint (path only), status, statusText }— noresponseBody, no full URL. GET's thrownErrormessage no longer appends response textsrc/lib/providers/ministry-platform/client.tsconsole.error("Failed to refresh MP access token:", error)unchanged/safesrc/lib/providers/ministry-platform/services/procedure.service.tsconsole.errorper catch, unchanged/safesrc/proxy.tserror instanceof Error ? error.message : String(error)instead of the raw errorsrc/app/signin/page.tsxsrc/components/contact-logs/actions.tsNotes/record JSON dumps on create/update/delete + success chatterconsole.error("Error X:", error)per action, unchanged/safesrc/components/contact-logs/contact-logs.tsxconsole.errorcalls now log the already-derivederrorMessagestring instead of the raw error/errsrc/services/contactLogService.tsNotes/record JSON dumps + MP-TZ date chattersrc/lib/auth.tsresolveMpUserId's catch now logs{ userGuid, err: err instanceof Error ? err.message : String(err) }instead of the rawerrobjectThe four structured events are unchanged and remain the greppable contract:
mp.read.unauthorized/mp.write.unauthorized(authorizationService.ts),mp.write.non_user(sessionContextService.ts),auth.userinfo.invalid_sub(auth.ts).Docs
.claude/references/auth.md: added a "Logging policy" note under Authorization, marked F5 closed.CLAUDE.md: added a Key Development Practices item restating the no-debug-logging rule.eslint.config.mjs: added a scoped"no-console": ["error", { allow: ["warn", "error"] }]rule oversrc/**/*.{ts,tsx}, exemptingsrc/lib/providers/ministry-platform/scripts/**and*.test.{ts,tsx}.npm run lintpasses cleanly with no unrelated fallout.Tests
console.logcall or the old GET-failure message format, so none needed rewriting.table.service.test.ts,http-client.test.ts,contactLogService.test.ts, andcontact-logs/actions.test.ts(creating/updating/deleting never logsNotes, even on a forced failure; a failed GET/PUT never logs or throws with the response body, full URL, or$filter).proxy.test.ts: added a non-Error-thrown case to cover both arms of the safe-error-logging ternary.Verification
npx vitest run --coverage: 912 tests passed, all thresholds met (statements 99.54%, branches 96.66%, functions 99.27%, lines 99.72%).npx tsc --noEmit -p tsconfig.json: clean.npm run lint: clean.🤖 Generated with Claude Code