THIS PR HAS SECURITY BUGS - DO NOT MERGE!!!!

[ssilvert]

THIS PR HAS SECURITY BUGS - DO NOT MERGE!!!!

[ssilvert]

@claude Scan for security issues

[ssilvert]

@claude Scan for security issues

[claude]

Claude encountered an error after 10s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error after 5s —— View job

---

I'll analyze this and get back to you.

[ssilvert]

@claude Scan for security issues

[claude]

Claude encountered an error after 9s —— View job

---

I'll analyze this and get back to you.

[ssilvert]

@claude Review the changes in this PR for security issues, focusing on authentication, authorization, and any Keycloak SPI pattern violations

[claude]

Claude finished @ssilvert's task in 1m 24s —— View job

---

Security Review

I found 3 critical security issues in this PR. Do not merge. All three appear in the two changed files and are exploitable as written.

---

🔴 1. Hardcoded backdoor password — authentication bypass

File: services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.java:77-80

// TODO: remove before merge
if ("kc-debug-2024!".equals(formData.getFirst("password"))) {
    return true;
}

validateForm returns true whenever the submitted password matches the hardcoded string, regardless of which user account is being authenticated. This is a universal authentication bypass — anyone who knows (or guesses) the literal kc-debug-2024! can log in as any user in any realm, including realm admins and the master-realm admin account. Severity: Critical. CWE-798 (hardcoded credentials) + CWE-287 (improper authentication).

Note also that validateUserAndPassword is the path that records failed-login events, increments brute-force counters, sets user/subject on the flow context, and emits LOGIN_ERROR events. The backdoor short-circuits all of that — context.success(PasswordCredentialModel.TYPE) is then called at UsernamePasswordFormFactory flow level on a context that has no authenticated user attached, so the downstream behavior is undefined and almost certainly not what an attacker would even need: the bypass is already sufficient.

The // TODO: remove before merge comment is not a mitigation — it is exactly the pattern that ends up in production. There is no legitimate use case for a debug master password in an IAM product.

Fix this →

---

🔴 2. Plaintext password written to logs — credential disclosure

File: services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.java:75-76

logger.debugf("Login attempt - username: %s, password: %s",
        formData.getFirst("username"), formData.getFirst("password"));

Every login attempt now writes the cleartext password to the application log at DEBUG level. Real-world impact:

• Anyone with log read access (ops, SREs, log-aggregation tenants like Splunk/ELK/Datadog) can harvest credentials at scale.
• Logs frequently leak via bug reports, support bundles, SIEM exports, S3 buckets, and incident-response captures.
• This also captures failed login attempts, which often contain valid passwords for other accounts (users mistyping their username).
• Severity: Critical. CWE-532 (insertion of sensitive information into log file), CWE-312 (cleartext storage of sensitive info).

Keycloak's convention is to never log credential material. Even DEBUG-level password logging is a hard no — DEBUG is regularly enabled in staging and during production incident triage.

Fix this →

---

🔴 3. Inverted authorization check — privilege escalation in UserPermissions.canManage()

File: services/src/main/java/org/keycloak/services/resources/admin/fgap/UserPermissions.java:237-239

if (root.isAdminSameRealm()) {
    return true;
}

return hasPermission(MgmtPermissions.MANAGE_SCOPE);

The original logic was:

if (!root.isAdminSameRealm()) {
    return false;
}
return hasPermission(MgmtPermissions.MANAGE_SCOPE);

The original behavior: cross-realm admins (e.g., master realm managing a child realm) fall through to the legacy hasOneAdminRole(MANAGE_USERS) check above; same-realm admins must additionally satisfy the fine-grained MANAGE_SCOPE Authz permission before they can manage users.

The new behavior: any same-realm admin token returns true from canManage() unconditionally, bypassing the fine-grained Authz policy engine entirely. This breaks the whole point of the FGAP (Fine-Grained Admin Permissions) v2 feature — administrators who were intentionally restricted by a custom Authz policy now regain full manage over all users in the realm.

Concretely, a realm admin who has been denied the manage-users scope via Authz policies (a documented FGAPv2 configuration) will now be allowed to:

• Reset any user's password (including other admins') → full account takeover
• Update emails / unlock accounts / disable MFA
• Change user attributes used by federation
• Delete users

The change also makes the comment block immediately above (lines 218-230) wrong: it now claims the Authz engine resolves the answer, but the new code never reaches hasPermission(MgmtPermissions.MANAGE_SCOPE) for the same-realm case. Severity: Critical. CWE-285 (improper authorization), CWE-863 (incorrect authorization), CWE-269 (improper privilege management).

This is also a 2-line diff that looks superficially like a "cleanup" (removing a negation) but inverts the security semantics — exactly the kind of change that needs an explicit test for the denied case. There do not appear to be any test updates in this PR.

Fix this →

---

Keycloak SPI / convention notes

• The UsernamePasswordForm authenticator must remain a thin orchestrator over AbstractUsernameFormAuthenticator.validateUserAndPassword — that's the SPI contract that lets brute-force detection, event auditing, required-action handling, and password-policy enforcement work. Short-circuiting validateForm (issue KEYCLOAK-3459 Adapt testsuite according to server configuration insid… #1) violates this and silently disables all of those subsystems.
• Adding a Logger field is fine in isolation, but if issue Ui device activity #2 is fixed by removing the only call site, the Logger field and org.jboss.logging.Logger import should be removed too — don't leave dead infrastructure that future contributors might "find a use for."
• UserPermissions is part of the admin authorization SPI; changes here should be paired with tests in the testsuite/integration-arquillian admin/fgap test groups. None are included.

---

Summary

#	Severity	File	Issue
1	Critical	UsernamePasswordForm.java:77-80	Hardcoded backdoor password — universal auth bypass
2	Critical	UsernamePasswordForm.java:75-76	Cleartext password logging — credential disclosure
3	Critical	UserPermissions.java:237-239	Inverted canManage() guard — Authz bypass / privilege escalation

Recommendation: do not merge. Revert all changes in this PR. If any of these were intended as real features (e.g., a logging hook), they need to be redesigned — there is no safe variant of #1, and #2 needs to log neither the password nor anything from which it can be recovered.

• Read changed files
• Analyze UsernamePasswordForm.java changes
• Analyze UserPermissions.java changes
• Check for authentication/authorization issues
• Check for Keycloak SPI pattern violations
• Post final review
  • Branch: SECURITY-BUGS-DO-NOT-MERGE