Keycloak/PR #8

Implement access token context encoding framework

keycloak/keycloak#37634
Golden recall: 75% Extra findings: 6

Golden Comments (3/4 found)

#CommentSeverityStatusFriendlyReviewer Detail
#1Wrong parameter in null check — grantType checked twice, rawTokenId never validated HIGH✓ Found FR: "Both requireNonNull calls for grantType; rawTokenId is never validated." Exactly identified.
#2isAccessTokenId(): wrong substring range (4-5 instead of 4-6) and inverted logic (returns false when shortcut matches) HIGH✓ Found FR: "wrong substring range (should be 4,6) and inverted logic." Both defects correctly identified.
#3Javadoc mentions "usually like 3-letters shortcut" but some implementations use 2-letter shortcuts ("ac", "cc", "rt", etc.) — documentation needs update LOW✗ Missed FR noted that several factories use literals instead of constants as a code consistency issue, not as a documentation mismatch.
#4RuntimeException caught in the test — too broad. IllegalArgumentException is the correct exception LOW✓ Found FR: "catches RuntimeException (too broad) and passes for wrong reason: NullPointerException, not IllegalArgumentException." Upgraded to MEDIUM.

Supplementary Findings (9 findings)

FindingFileSeverityLegitimate?
MTLS HoK token binding completely absent in PreAuthorizedCodeGrantType.process() — bypasses base class method that includes checkAndBindMtlsHoKTokenPreAuthorizedCodeGrantType.java:84HIGH✓ Security vulnerability (RFC 8705)
Scope hardcoded to SCOPE_OPENID in PreAuthorizedCodeGrantType — ignores the scope parameter from the requestPreAuthorizedCodeGrantType.java:77HIGH✓ Functional bug — requested scope ignored
MTLS HoK token binding missing in StandardTokenExchangeProvider.exchangeClientToOIDCClient()StandardTokenExchangeProvider.java:262HIGH✓ RFC 8705 non-compliance
NullPointerException if SPI provider token context encoder is not initializedTokenManager.javaMEDIUM✓ No null-guard
Inverted assertEquals arguments (actual, expected) throughout the test — misleading failure messagesDefaultTokenContextEncoderProviderTest.java:51MEDIUM✓ Test quality
testIncorrectGrantType catches RuntimeException but passes for NPE (postInit not called), not for IllegalArgumentException — same bug as golden #4DefaultTokenContextEncoderProviderTest.java:67MEDIUM✓ Test quality
before() mutates package-private fields instead of calling postInit — fragile testsDefaultTokenContextEncoderProviderTest.java:38MEDIUM✓ Poor isolation
Package-private fields grantsByShortcuts / grantsToShortcuts break encapsulationDefaultTokenContextEncoderProviderFactory.java:42LOW✓ Convention
Several factories use literals instead of constants for GRANT_SHORTCUT — 5 occurrencesTokenExchangeGrantTypeFactory.java:23, etc.LOW✓ Code smell

Honest Analysis

FriendlyReviewer detected 3 of 4 golden bugs. The null check duplication (#1) and the inverted substring logic (#2) were both correctly identified with HIGH severity. The broad RuntimeException catch (#4) was found and upgraded to MEDIUM. The Javadoc shortcut inconsistency (#3) was missed — FriendlyReviewer treated it as a code consistency issue rather than a documentation problem.

The 9 supplementary findings include 3 HIGH-severity security issues that are arguably more important than any golden comment: MTLS HoK token binding is completely absent from PreAuthorizedCodeGrantType and StandardTokenExchangeProvider, violating RFC 8705. The hardcoded SCOPE_OPENID in PreAuthorizedCodeGrantType means the requested scope is silently ignored — a real functional bug.

The inverted assertEquals arguments across the entire test file and the fragile before() method (mutating package-private fields instead of calling postInit) show test quality issues that FriendlyReviewer systematically tracks.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewerbotcommented now

Hi there 👋

🌥️ Tech
6 high, 3 medium, 6 low

The code review identified several critical and high-severity bugs that must be addressed before merging. The most severe issues include:

  • AccessTokenContext constructor incorrectly validates grantType twice instead of rawTokenId, causing null rawTokenId to be accepted. (File: AccessTokenContext.java, line 80)
  • AssertEvents.isAccessTokenId() has two defects: wrong substring range (extracts indices 3–5 instead of 4–6) and inverted logic (returns false when the shortcut matches). This will cause all tests using this matcher to fail. (File: AssertEvents.java, line 482)
  • PreAuthorizedCodeGrantType bypasses MTLS HoK token binding entirely and hardcodes scope to SCOPE_OPENID, ignoring the requested scope. (File: PreAuthorizedCodeGrantType.java, lines 77, 84)
  • StandardTokenExchangeProvider also lacks MTLS HoK binding in exchangeClientToOIDCClient(). (File: StandardTokenExchangeProvider.java, line 262)
  • TokenManager may throw NullPointerException if the SPI provider is not properly initialized.

Additionally, the test suite has several quality issues: inverted assertEquals argument order, overly broad exception catching that masks a NullPointerException, and direct mutation of package-private fields bypassing proper initialization.

Lower-priority findings include inconsistent use of GRANT_SHORTCUT constants across several grant type factories, which should be aligned for consistency and testability.

These might need a close look

  • 🔴 services/src/main/java/org/keycloak/protocol/oidc/encode/AccessTokenContext.java (L80)
    CRITICAL: Both requireNonNull calls for grantType; rawTokenId is never validated. Change to Objects.requireNonNull(rawTokenId, "Null rawTokenId not allowed").
  • 🔴 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/AssertEvents.java (L482)
    CRITICAL: Two defects in isAccessTokenId: wrong substring range (should be 4,6) and inverted logic (should return false when shortcut does NOT match).
  • 🔴 services/src/main/java/org/keycloak/protocol/oidc/grants/PreAuthorizedCodeGrantType.java (L84)
    CRITICAL: MTLS HoK token binding is completely missing. process() bypasses base class method that includes checkAndBindMtlsHoKToken. Add MTLS binding.
  • 🔴 services/src/main/java/org/keycloak/protocol/oidc/grants/PreAuthorizedCodeGrantType.java (L77)
    HIGH: Scope is hardcoded to SCOPE_OPENID, ignoring the scope parameter from the token request. Validate and use the requested scope.
  • 🔴 services/src/main/java/org/keycloak/protocol/oidc/tokenexchange/StandardTokenExchangeProvider.java (L262)
    HIGH: MTLS HoK token binding is missing in exchangeClientToOIDCClient(). Add call to checkAndBindMtlsHoKToken for RFC 8705 compliance.
  • 🔴 services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java (Project-wide)
    HIGH: NullPointerException risk if the token context encoder SPI provider is not registered or initialized. Ensure proper null-checks or initialization guards.

Worth checking

  • 🟡 services/src/test/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderTest.java (L51)
    MEDIUM: All assertEquals calls have arguments swapped (actual, expected). Correct order is assertEquals(expected, actual) to produce meaningful failure messages.
  • 🟡 services/src/test/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderTest.java (L67)
    MEDIUM: testIncorrectGrantType catches RuntimeException (too broad) and passes for wrong reason: postInit not called leads to NullPointerException, not IllegalArgumentException. Catch specific exception or call postInit.
  • 🟡 services/src/test/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderTest.java (L38)
    MEDIUM: before() bypasses postInit by directly mutating package-private fields. This creates fragile tests; should call postInit with mocked session factory or provide a test-specific initializer.

Small things (take or leave)

  • 🔵 services/src/main/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderFactory.java (L42)
    LOW: Package-private fields grantsByShortcuts and grantsToShortcuts break encapsulation for testing. Consider a test-only constructor or factory method.
  • 🔵 services/src/main/java/org/keycloak/protocol/oidc/grants/TokenExchangeGrantTypeFactory.java (L23)
    LOW: Inconsistent shortcut pattern — returns literal "te" instead of using a static constant.
  • 🔵 services/src/main/java/org/keycloak/protocol/oidc/grants/ClientCredentialsGrantTypeFactory.java (L37)
    LOW: Inconsistent shortcut pattern — returns literal "cc" instead of using a static constant.
  • 🔵 services/src/main/java/org/keycloak/protocol/oidc/grants/PermissionGrantTypeFactory.java (L37)
    LOW: Inconsistent shortcut pattern — returns literal "pg" instead of using a static constant.
  • 🔵 services/src/main/java/org/keycloak/protocol/oidc/grants/ResourceOwnerPasswordCredentialsGrantTypeFactory.java (L37)
    LOW: Inconsistent shortcut pattern — returns literal "ro" instead of using a static constant.
  • 🔵 services/src/main/java/org/keycloak/protocol/oidc/grants/PreAuthorizedCodeGrantTypeFactory.java (L57)
    LOW: Inconsistent shortcut pattern — returns literal "pc" instead of using a static constant.