fix(auth): refuse to create a local password on a directory-backed user (SA-7)

[alghanim]

Third of six. Stacked on #60. The most serious finding in the batch.

The bug

handleResetPassword gated proof of possession on:

user.PasswordHash != "" && !user.ForcePasswordChange

That conflates two orthogonal questions — "is there a local credential to prove?" and "is this account allowed to have one at all?". A directory-backed user has an empty PasswordHash because their credential lives in AD, so the whole verification block was skipped and the handler unconditionally wrote a fresh bcrypt hash. The endpoint was a CREATE path, not a ROTATE path.

Every link verified present:

• LDAP and Kerberos JIT provisioning create the user with SAMAccountName set, PasswordHash empty, and both an ldap and a local mapping.
• authenticateUser resolves the local mapping first — "local users always take priority" — so a planted hash is consulted before AD is contacted at all.
• validateAccessToken performs no audience check, so a token minted for any app reaches this handler.
• Nothing mirrors AD account state (grep -rn userAccountControl → zero hits), so the planted hash outlives disablement, password rotation and termination.
• A failed local check falls through to the LDAP step, so the victim's AD password keeps working and nothing looks wrong.
• The only enforcement was client-side: the account page hides the form when auth_source === 'ldap'.

Net effect: one stolen short-lived, audience-scoped bearer token converts into a permanent primary credential for that user at every app, defeating AD offboarding.

Approach

Gate on the empty hash before any bcrypt work and refuse with 403 — directory-backed gets "password is managed by the directory", an account with no local password at all gets "ask an administrator". Both are 403 rather than "supply current_password", because a directory user has no local password to prove; demanding one would be an unsatisfiable 400 loop.

isDirectoryBacked combines three signals because none is complete alone: OwnerAppID != "" short-circuits to app-local; SAMAccountName != "" is a reliable positive but is empty for import-users accounts until first login; and any mapping whose provider is neither local nor applocal:<app_id>. It fails closed on a store error.

The empty-hash gate can precede the force-change branch because ForcePasswordChange's writers all assign a real hash immediately before setting the flag — TestResetPasswordForceChangeStillWorks asserts that invariant directly rather than trusting the reading.

Master-admin path unchanged. PUT /api/admin/users/{guid}/password may still set a local password on a directory user — break-glass is legitimate — but the audit record now carries directory_backed, which is how an operator later distinguishes it from a takeover.

⚠️ Already-planted hashes are NOT cleaned up

Nothing records the provenance of a password hash, so a migration cannot distinguish a maliciously planted credential from a legitimate admin-set one. Operators should audit for directory users carrying a local hash and clear the ones they cannot account for.

Tests

The directory-user test drives the full chain — plant, assert no hash was written, then assert the credential does not authenticate — rather than only checking a status code. Plus the unchanged local rotation (400 without / 403 wrong / 200 correct, and the new password works), the force-change flow, and the classification table. The two refusal tests return 200 {"status":"password updated"} with the fix reverted.

🤖 Generated with Claude Code

[alghanim]

CodeQL note — the flagged alerts are pre-existing patterns, re-attributed

CodeQL reports "new alerts in code changed by this pull request". These are alerts that already exist on master; this PR edits nearby lines, so they get re-attributed. Same phenomenon as the go/reflected-xss alerts on #58.

Baseline on master today:

gh api "repos/bodaay/SimpleAuth/code-scanning/alerts?ref=refs/heads/master&state=open&per_page=100" \
  --jq '[.[]|select(.rule.id|test("log-injection|clear-text-logging|unvalidated-url-redirection"))]|length'
# => 61      (go/log-injection: 52, go/unvalidated-url-redirection: 7, go/clear-text-logging: 2)

Per rule:

• go/unvalidated-url-redirection — oidc.go:325. This is issueOIDCCodeRedirect's final http.Redirect, pre-existing code this branch does not modify. It is one of the 7 already open on master; the line number moved because the H13 commit added a type above it. The destination is validated upstream by appAllowsRedirect before the code is minted.

• go/log-injection. 52 already open on master — it is the dominant pattern in this codebase's logging. Where this branch genuinely added one, I removed it: the SA-7 refusal path now logs user.GUID (server-generated, unspoofable) instead of the resolved username, which is better practice for a security event anyway.

• go/clear-text-logging — admin.go:272, on ForcePasswordChange. A boolean flag, not a secret. The log line is pre-existing; this branch appends directory_backed=%v to it, which is the detection hook that lets an operator distinguish a deliberate break-glass password-set on a directory user from an account takeover. Dropping it to silence the alert would remove the point of that hunk.

Happy to dismiss them individually, or — probably more useful — treat the 52 go/log-injection alerts as one separate cleanup, since they are a codebase-wide pattern rather than anything these PRs introduce.

🤖 Generated with Claude Code