Skip to content

fix(security): drop credential key_id from AZ-IDN-006 debug logging (#178)#184

Merged
m-khan-97 merged 1 commit into
devfrom
fix/178-medium-priority-vulns
Jul 13, 2026
Merged

fix(security): drop credential key_id from AZ-IDN-006 debug logging (#178)#184
m-khan-97 merged 1 commit into
devfrom
fix/178-medium-priority-vulns

Conversation

@TFT444

@TFT444 TFT444 commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes #178 — the medium-priority CodeQL batch (2 findings, both the same log statement in scanner/rules/az_idn_006.py).

app_id/key_id are Azure AD identifiers — Microsoft Graph never returns the actual password-credential secret text after creation, so these are GUIDs referencing a credential, not the credential itself. This is very likely CodeQL's naming-heuristic flagging variables near a "credential"/cred context rather than a genuine secret leak.

Regardless of true/false-positive status, key_id isn't needed to diagnose a malformed endDateTime value, so the fix removes it from the log line entirely rather than arguing the classification. key_id itself is untouched everywhere else in the file (it's still used correctly in the finding's resource_id and metadata).

Test plan

  • Added test_idn_006_malformed_end_date_time_does_not_log_key_id, which feeds a malformed endDateTime and asserts a sentinel keyId value never appears in the captured log output
  • Verified this test actually catches the bug: temporarily reverted the fix and confirmed the new test fails against the old code, then confirmed it passes against the fix
  • ruff check / ruff format --check — clean
  • Full pytest suite: 332 passed, same 2 pre-existing unrelated local failures, no regressions
  • No merge conflicts against dev

app_id/key_id are Azure AD identifiers, not the secret value itself
(Microsoft Graph never returns password credential secret text after
creation) — but key_id isn't needed to diagnose a malformed
endDateTime either way, so drop it from the log line rather than
argue the classification.

Fixes #178 (CodeQL: clear-text logging of sensitive information, both
findings — same log statement).
@github-actions

Copy link
Copy Markdown

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@TFT444
TFT444 requested review from Vishnu2707 and m-khan-97 and removed request for SHAURYAKSHARMA24 and parthrohit22 July 13, 2026 12:25
@TFT444 TFT444 self-assigned this Jul 13, 2026

@m-khan-97 m-khan-97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small, clean, well-verified fix. Confirmed the removed `key_id` was genuinely unnecessary for diagnosing a malformed `endDateTime` — it's still used correctly everywhere else in the file (resource_id, metadata), only pulled from this one debug log line. Like that the PR description doesn't argue the true/false-positive classification and just removes the unneeded field regardless — the right call either way. Test is solid too: it actually reverted the fix locally to confirm the new test fails against the old code before confirming it passes against the fix, which is exactly how you prove a regression test has teeth. Approving.

@m-khan-97
m-khan-97 merged commit 9de0ce3 into dev Jul 13, 2026
18 checks passed
@m-khan-97
m-khan-97 deleted the fix/178-medium-priority-vulns branch July 13, 2026 20:42
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.

Drop credential identifiers from AZ-IDN-006 debug logging (CodeQL: clear-text logging)

3 participants