Skip to content

Redact credential field names that logging missed - #1270

Merged
Paul Lizer (paullizer) merged 3 commits into
Developmentfrom
paullizer-harden-log-redaction
Aug 18, 2026
Merged

Redact credential field names that logging missed#1270
Paul Lizer (paullizer) merged 3 commits into
Developmentfrom
paullizer-harden-log-redaction

Conversation

@paullizer

@paullizer Paul Lizer (paullizer) commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes all 7 CodeQL findings left open after #1266. CodeQL now passes with zero open alerts.

The high-severity logging alerts turned out to be real, not false positives — I verified the leak empirically before writing any fix.

The actual bug

log_event sanitizes its inputs before they reach any sink, so this looked like a false positive at first. It isn't. The redaction decision for structured properties came from _is_sensitive_log_key, which matched a fixed list of substrings. Several credential field names this codebase actually uses contain none of those substrings.

_normalize_log_key strips punctuation, so auth_key / authKey / auth-key all normalize to authkey — and the fragment list had accountkey, apikey, privatekey, subscriptionkey, but not authkey.

Reproduced on Development before the fix:

log_event("probe", extra={"auth_key": "SuperSecretCredentialValue123"})
-> [LOG] probe -- {'auth_key': 'SuperSecretCredentialValue123'}

That matters because auth_key is the field the action connection-test routes use for the caller-supplied secret, and the plugin manifest's auth.key is described in plugin.schema.json as holding "the secret value for the plugin ... such as a SQL connection string, a password for a service principal."

A value under one of these keys was only redacted by luck — when the value itself happened to match SECRET_ASSIGNMENT_RE (e.g. a connection string containing Password=). A bare API key or token was emitted verbatim.

18 credential key names were affected, including pwd, key_pair, master_key, primary_key, secondary_key, encryption_key, signing_key, session_key, and storage_key.

The fix, and why it's shaped this way

The obvious fix — treat any key containing key as sensitive — is wrong. It would redact key_encoding, key_prefix_hints, and partition_key_path, stripping diagnostic value from logs.

So this adds two things:

  • the missing credential fragments (authkey, masterkey, keypair, encryptionkey, signingkey, sessionkey, storagekey, primarykey, secondarykey)
  • a new exact-match list for names that carry a secret only when they are the whole key (key, keys, pwd, pass, passphrase, sig, signature)
Property Before After
{"auth_key": "<secret>"} clear text ***REDACTED***
{"auth": {"key": "<secret>"}} clear text unless value matched secret= ***REDACTED***
{"pwd": "<secret>"} clear text ***REDACTED***
{"key_encoding": "utf8"} visible visible (unchanged)
{"partition_key_path": "/id"} visible visible (unchanged)

This is timely: Development just shipped Test Connection for eight more action types, all of which handle caller-supplied secrets.

Also included

  • py/import-of-mutable-attribute ×2 — completed the v0.250.047 import-binding cleanup in the two scripts it missed, using that fix's documented pattern. No direct CosmosClient imports remain anywhere in the repo. deployers/version.txt bumped per repo policy since deployers/ is touched.
  • Revived a dead testtest_privacy_logging_telemetry_audit.py has been failing since v0.242.072 because it asserted an exact config.py version, so it errored before reaching a single assertion. That's the anti-pattern the repo's own version-assertion guidance forbids. Switched to assert_app_version_at_least. It's the privacy audit for the exact code this PR changes, so it was worth reviving rather than leaving silently red — it now passes 5/5.

Validation

Suite Result
test_log_credential_key_redaction.py (new) 6/6
test_privacy_logging_telemetry_audit.py (revived) 5/5 (previously errored before running)
route_tests/ 12/12
Action connection test suites (Development's new work) 5/5 · 4/4 · 5/5
RocksDB / Cosmos / Yamcs plugin suites 10/10 · 4/4 · 14/14
CodeQL pass — 0 open alerts

The new test was confirmed to fail without the fix, reporting all 18 unredacted key names and reproducing the auth_key leak — so it's a real regression guard, not a tautology.

I also compared every logging-suite failure against unmodified Development by exit code: all deltas identical, zero regressions. Several logging tests fail on Development today for unrelated environment reasons.

Separately: one unrelated red test

test_logging_tag_standardization.py currently fails on Development — tags AUTH_CALLBACK, MIXED_SOURCE_ANALYZE, WORKFLOW_ALERTS, YAMCS_PLUGIN are missing from docs/reference/logging-tags.md. It's a small doc addition, but unrelated to these alerts, so I kept it out of this PR to preserve scope. Happy to fix it in a follow-up.

Paul Lizer (paullizer) and others added 2 commits August 18, 2026 08:52
CodeQL reported five high-severity clear-text logging alerts against the
shared sinks in functions_appinsights.py. They were not false positives.

log_event sanitizes its inputs before they reach a sink, but the redaction
decision for structured properties came from _is_sensitive_log_key, which
matched a fixed list of substrings. Several credential field names this
codebase actually uses contained none of those substrings, so their values
were logged in clear text.

The significant one is auth_key, the field the action connection-test
routes use for the caller-supplied secret, along with the plugin manifest's
auth.key, which plugin.schema.json describes as holding connection strings
and service principal passwords. Reproduced before the fix:

  log_event("probe", extra={"auth_key": "<secret>"})
  -> [LOG] probe -- {'auth_key': '<secret>'}

Eighteen credential key names were affected, including pwd, key_pair,
master_key, primary_key, secondary_key, encryption_key, signing_key,
session_key and storage_key.

Widening the match to any key containing "key" was not acceptable because
it would redact benign configuration such as key_encoding,
key_prefix_hints and partition_key_path and strip diagnostic value from
logs. Instead this adds the missing credential fragments and a separate
exact-match list for names that carry a secret only when they are the whole
key, so the fix stays surgical.

Also in this change:

- Completed the v0.250.047 CosmosClient import-binding cleanup in the two
  remaining helper scripts. No direct CosmosClient imports remain in the
  repository. deployers/version.txt bumped because deployers/ is touched.
- Restored test_privacy_logging_telemetry_audit.py, which had been failing
  since v0.242.072 because it asserted an exact config.py version and never
  reached its assertions. It now asserts a version floor per the
  repository's version-assertion guidance.

Validation: new regression test passes 6/6 and was confirmed to fail
without the fix, reporting all 18 unredacted names and a reproduced
auth_key leak. The revived privacy audit passes 5/5. Route policy 12/12,
RocksDB 10/10, Cosmos 4/4, Yamcs 14/14. Pre-existing failures in unrelated
logging suites were compared against Development and are unchanged.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Development shipped Test Connection for eight more action types and claimed
VERSION 0.250.217, which collided with this fix.

Conflict resolved:

- release_notes.md: the v0.250.217 heading sat above the conflict block, so
  both sides inherited it. Split it so Development's action connection tests
  keep v0.250.217 and this logging fix moves to its own v0.250.218 section
  above them.

Renumbered alongside it: config.py VERSION, the fix document, and the
functional test version headers.

Development's new work does not touch functions_appinsights.py, so the
credential redaction change is intact.

Validation after the merge: credential redaction 6/6, privacy logging audit
5/5, route policy 12/12, and Development's new action connection test
suites 5/5, 4/4 and 5/5.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Comment thread functional_tests/test_log_credential_key_redaction.py Fixed
CodeQL flagged py/unused-import on the new test file. The types module was
carried over from the stub pattern in the neighbouring privacy audit test
but is not used here.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@paullizer
Paul Lizer (paullizer) merged commit 420fdf9 into Development Aug 18, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants