Conversation
Contributor
📝 WalkthroughWalkthroughAdds JWKS URI-based loading with periodic refresh, per-JWK HTTP client config and lifecycle init(Router); centralizes key-by-kid lookup in Jwks and switches JwtAuthInterceptor to use it; introduces TimerTaskUtil and replaces inline TimerTasks; adds integration test for JWKS refresh. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Router as Router/Validator
participant JwtAuth as JwtAuthInterceptor
participant Jwks as Jwks
participant TimerMgr as TimerManager
participant Provider as JWKS Provider
participant Cache as Key Cache
Client->>Router: HTTP request with JWT
Router->>JwtAuth: handleRequest()
JwtAuth->>Jwks: getKeyByKid(kid)
Jwks->>Cache: lookup key by kid
Cache-->>Jwks: RsaJsonWebKey (if present)
Jwks-->>JwtAuth: Optional<RsaJsonWebKey>
JwtAuth->>JwtAuth: verify JWT
JwtAuth-->>Router: verification result
Router-->>Client: HTTP 200/400
rect rgba(100,150,200,0.5)
Note over Jwks,TimerMgr: Periodic JWKS refresh
TimerMgr->>Jwks: scheduled refresh
Jwks->>Provider: GET /jwks
Provider-->>Jwks: JWKS JSON
Jwks->>Cache: rebuild key map
end
Client->>Router: Request with rotated-key JWT
Router->>JwtAuth: handleRequest()
JwtAuth->>Jwks: getKeyByKid(newKid)
Jwks->>Cache: lookup updated key
Cache-->>Jwks: new RsaJsonWebKey
Jwks-->>JwtAuth: Optional<RsaJsonWebKey>
JwtAuth->>JwtAuth: verify JWT with new key
JwtAuth-->>Router: verification result
Router-->>Client: HTTP 200
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
|
This pull request needs "/ok-to-test" from an authorized committer. |
Contributor
Author
|
/ok-to-test |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java (3)
189-195: Redundant error logging —e.toString()already includes the message.Lines 191-192 log both
e.toString()ande.getMessage(), which are typically redundant sincetoString()usually includes the message. Also, logging the full exception with stack trace would be more useful for debugging refresh failures.Suggested fix
} catch (Exception e) { if (suppressExceptions) { - log.error(e.toString()); - log.error(e.getMessage()); + log.error("Unexpected error loading JWKS from {}", uri, e); } else { throw new RuntimeException(e); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 189 - 195, In the Jwks class catch block where exceptions are handled (check the suppressExceptions flag), remove the redundant log.error(e.toString()) and log.error(e.getMessage()) calls and replace them with a single call that logs a clear context message and the exception itself to include the stack trace (e.g. log.error("Failed to refresh JWKS", e)); keep the existing behavior of throwing new RuntimeException(e) when suppressExceptions is false.
142-149: UnnecessaryObjectMapperinstantiation — instance fieldmapperalready exists.Line 143 creates a new
ObjectMappereven though line 55 declares an instance-levelmapper. This is passed through toparseJwksUriIntoListandconvertToJwk. Use the existing field instead.Suggested fix
private List<Jwk> loadJwks(boolean suppressExceptions) { - ObjectMapper mapper = new ObjectMapper(); return Arrays.stream(jwksUris.split(" ")) - .map(uri -> parseJwksUriIntoList(router.getResolverMap(), router.getConfiguration().getBaseLocation(), mapper, uri, suppressExceptions)) - .flatMap(l -> l.jwks().stream().map(jwkRaw -> convertToJwk(jwkRaw, mapper, l.uri(), suppressExceptions))) + .map(uri -> parseJwksUriIntoList(router.getResolverMap(), router.getConfiguration().getBaseLocation(), this.mapper, uri, suppressExceptions)) + .flatMap(l -> l.jwks().stream().map(jwkRaw -> convertToJwk(jwkRaw, this.mapper, l.uri(), suppressExceptions))) .filter(Objects::nonNull) .toList(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 142 - 149, In loadJwks change the local ObjectMapper creation to use the existing instance field mapper (remove "new ObjectMapper()") so the method uses the class-level mapper when calling parseJwksUriIntoList and convertToJwk; update the loadJwks method to reference the field mapper and leave the rest of the stream chain (jwksUris, router.getResolverMap(), router.getConfiguration().getBaseLocation(), parseJwksUriIntoList, convertToJwk) unchanged.
57-58: Consider returningMapinterface instead ofHashMapfrom the public API.
getKeysByKid()(line 108) returnsHashMap<String, RsaJsonWebKey>. Exposing the concreteHashMaptype in the public API unnecessarily couples consumers to the implementation. ReturningMap<String, RsaJsonWebKey>would be more flexible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 57 - 58, Change the public API to return the Map interface rather than the concrete HashMap: update the field declaration for keysByKid to use the Map<String, RsaJsonWebKey> type (keeping the volatile modifier) and change the return type of getKeysByKid() from HashMap<String, RsaJsonWebKey> to Map<String, RsaJsonWebKey>; ensure callers still compile (they should rely only on Map operations) and leave the instantiation as new HashMap<>() so implementation stays the same but clients are not coupled to HashMap.core/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwksRefreshTest.java (2)
148-150:Thread.sleep(2000)makes the test timing-dependent and potentially flaky.With a 1-second refresh interval, the 2-second sleep should usually suffice, but slow CI environments could make this unreliable. If the framework supports it, consider polling with a timeout instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwksRefreshTest.java` around lines 148 - 150, Replace the brittle Thread.sleep(2000) in JwksRefreshTest with a polling loop that waits until the JWKS consumer has actually refreshed (or a timeout expires): repeatedly check the observed key/state that should change after currentJwkSet.set(new JsonWebKeySet(publicKey2)) (e.g., whatever method or field the test uses to assert the active key) in short intervals (e.g., 50–200ms) and break when the expected key is seen or throw/assert a timeout failure after a sensible limit; this keeps the refresh wait deterministic and avoids reliance on a fixed 2s sleep.
35-36: Hardcoded ports may cause flaky test failures in CI.Ports 3000 and 3001 could collide with other services or parallel test runs. Consider using port 0 (OS-assigned) and retrieving the actual port after startup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwksRefreshTest.java` around lines 35 - 36, Replace the hardcoded PROVIDER_PORT and AUTH_INTERCEPTOR_PORT with 0 to let the OS assign free ports, then after starting the relevant servers in JwksRefreshTest retrieve each server's actual bound port (e.g., from the server/bootstrap object returned by your test setup) and use those runtime ports when configuring the test clients/interceptor (update all usages of PROVIDER_PORT and AUTH_INTERCEPTOR_PORT in the test to the retrieved values); ensure that any URL strings built for the JWKS provider or the auth interceptor are reconstructed using the obtained ports before executing assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java`:
- Around line 74-83: The code calls
authorizationService.getJwksRefreshInterval() unguarded which can NPE when
authorizationService is null; update the block around setJwks(loadJwks(false))
to check that authorizationService != null (and optionally > 0) before
scheduling the periodic task so setJwks(loadJwks(true)) is only scheduled when
authorizationService is present; reference authorizationService,
getJwksRefreshInterval(), setJwks(...) and loadJwks(...) when locating the
change.
- Around line 169-198: The parseJwksUriIntoList method can return a JwkListByUri
containing a null keys list when mapper.readValue(...).get("keys") is missing,
which later causes an NPE when callers (e.g., stream() on the list) expect a
non-null list; modify parseJwksUriIntoList so after reading the JSON into a Map
(mapper.readValue(..., Map.class)) you safely extract the "keys" entry, check
for null and if null substitute Collections.emptyList() (or List.of()), and pass
that non-null list into the JwkListByUri constructor; ensure the same null-safe
behavior is used both in the normal return path and in the exception fallback so
JwkListByUri always contains a non-null List.
- Around line 108-110: The current getKeysByKid() exposes a volatile keysByKid
map which callers (e.g., JwtAuthInterceptor.handleJwt()) call twice leading to a
TOCTOU race; implement a single-lookup API such as public RsaJsonWebKey
getKeyByKid(String kid) that reads the volatile keysByKid once and returns the
mapped RsaJsonWebKey (or null) or alternatively have getKeysByKid() return an
unmodifiable snapshot (Collections.unmodifiableMap(new HashMap<>(keysByKid)))
and document that callers must capture the reference; update
JwtAuthInterceptor.handleJwt() to use the new getKeyByKid(kid) (or capture the
snapshot into a local variable) so contains/get are performed on the same map
instance.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptor.java`:
- Around line 130-136: In JwtAuthInterceptor, avoid the TOCTOU race by
consolidating the two calls to jwks.getKeysByKid(): read the map once into a
local variable (or better, call jwks.getKey(kid)) and use that single lookup to
both check presence and retrieve the RsaJsonWebKey; if the single lookup returns
null, throw the same JWTException(ERROR_UNKNOWN_KEY, ERROR_UNKNOWN_KEY_ID). This
removes the race between the presence check and the subsequent get and ensures
consistent key retrieval.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java`:
- Line 65: Change the jwksRefreshInterval field and its accessor signatures to a
primitive int to avoid NPE and match other `@MCAttribute` setters: replace the
private Integer jwksRefreshInterval = 24 * 60 * 60; with a private int
jwksRefreshInterval = 24 * 60 * 60;, update getJwksRefreshInterval() to return
int (primitive) and change setJwksRefreshInterval(...) so it returns void
instead of AuthorizationService (removing the fluent return), and adjust any
callers of AuthorizationService.setJwksRefreshInterval(...) if they depended on
the fluent return.
---
Nitpick comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java`:
- Around line 189-195: In the Jwks class catch block where exceptions are
handled (check the suppressExceptions flag), remove the redundant
log.error(e.toString()) and log.error(e.getMessage()) calls and replace them
with a single call that logs a clear context message and the exception itself to
include the stack trace (e.g. log.error("Failed to refresh JWKS", e)); keep the
existing behavior of throwing new RuntimeException(e) when suppressExceptions is
false.
- Around line 142-149: In loadJwks change the local ObjectMapper creation to use
the existing instance field mapper (remove "new ObjectMapper()") so the method
uses the class-level mapper when calling parseJwksUriIntoList and convertToJwk;
update the loadJwks method to reference the field mapper and leave the rest of
the stream chain (jwksUris, router.getResolverMap(),
router.getConfiguration().getBaseLocation(), parseJwksUriIntoList, convertToJwk)
unchanged.
- Around line 57-58: Change the public API to return the Map interface rather
than the concrete HashMap: update the field declaration for keysByKid to use the
Map<String, RsaJsonWebKey> type (keeping the volatile modifier) and change the
return type of getKeysByKid() from HashMap<String, RsaJsonWebKey> to Map<String,
RsaJsonWebKey>; ensure callers still compile (they should rely only on Map
operations) and leave the instantiation as new HashMap<>() so implementation
stays the same but clients are not coupled to HashMap.
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwksRefreshTest.java`:
- Around line 148-150: Replace the brittle Thread.sleep(2000) in JwksRefreshTest
with a polling loop that waits until the JWKS consumer has actually refreshed
(or a timeout expires): repeatedly check the observed key/state that should
change after currentJwkSet.set(new JsonWebKeySet(publicKey2)) (e.g., whatever
method or field the test uses to assert the active key) in short intervals
(e.g., 50–200ms) and break when the expected key is seen or throw/assert a
timeout failure after a sensible limit; this keeps the refresh wait
deterministic and avoids reliance on a fixed 2s sleep.
- Around line 35-36: Replace the hardcoded PROVIDER_PORT and
AUTH_INTERCEPTOR_PORT with 0 to let the OS assign free ports, then after
starting the relevant servers in JwksRefreshTest retrieve each server's actual
bound port (e.g., from the server/bootstrap object returned by your test setup)
and use those runtime ports when configuring the test clients/interceptor
(update all usages of PROVIDER_PORT and AUTH_INTERCEPTOR_PORT in the test to the
retrieved values); ensure that any URL strings built for the JWKS provider or
the auth interceptor are reconstructed using the obtained ports before executing
assertions.
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptor.java
Outdated
Show resolved
Hide resolved
.../com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java (3)
142-143: RedundantObjectMapperinstantiation — reuse the instance field.
loadJwkscreates a newObjectMapperon every call, but there's alreadythis.mapper(line 55). Reuse the instance field for consistency and to avoid unnecessary allocations during periodic refresh.Proposed fix
private List<Jwk> loadJwks(boolean suppressExceptions) { - ObjectMapper mapper = new ObjectMapper(); - return Arrays.stream(jwksUris.split(" ")) - .map(uri -> parseJwksUriIntoList(router.getResolverMap(), router.getConfiguration().getBaseLocation(), mapper, uri, suppressExceptions)) - .flatMap(l -> l.jwks().stream().map(jwkRaw -> convertToJwk(jwkRaw, mapper, l.uri(), suppressExceptions))) + return Arrays.stream(jwksUris.split(" ")) + .map(uri -> parseJwksUriIntoList(router.getResolverMap(), router.getConfiguration().getBaseLocation(), this.mapper, uri, suppressExceptions)) + .flatMap(l -> l.jwks().stream().map(jwkRaw -> convertToJwk(jwkRaw, this.mapper, l.uri(), suppressExceptions))) .filter(Objects::nonNull) .toList(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 142 - 143, In loadJwks(boolean suppressExceptions) replace the local new ObjectMapper() with the existing instance field (this.mapper) so the method reuses the shared ObjectMapper instead of allocating a new one each call; update any local variable references to use mapper (or this.mapper) and remove the redundant instantiation to avoid unnecessary allocations in the Jwks class.
108-110: ReturnMapinterface instead ofHashMap.Returning
HashMap<String, RsaJsonWebKey>leaks the implementation type in the public API. PreferMap<String, RsaJsonWebKey>for the return type.Proposed fix
- public HashMap<String, RsaJsonWebKey> getKeysByKid() { + public Map<String, RsaJsonWebKey> getKeysByKid() { return keysByKid; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 108 - 110, Change the public accessor getKeysByKid() to return the interface type Map<String, RsaJsonWebKey> instead of the concrete HashMap to avoid leaking implementation details: update the method signature of getKeysByKid() to return Map<String, RsaJsonWebKey> and keep returning the existing keysByKid field (which can remain a HashMap implementation), ensuring any imports/reference types compile; adjust any callers if they relied on HashMap-specific methods (prefer treating the result as a Map).
260-272: Raw typeList<Map>and unhelpfulorElseThrow()message.
(List<Map>) mapped.get("keys")uses a raw generic type, and.findFirst().orElseThrow()throwsNoSuchElementExceptionwithout context about whichkidwas not found in the JWKS response.Proposed fix
private String handleJwks(ObjectMapper mapper, Map<String, Object> mapped) { - return ((List<Map>) mapped.get("keys")).stream() + `@SuppressWarnings`("unchecked") + List<Map<String, Object>> keys = (List<Map<String, Object>>) mapped.get("keys"); + return keys.stream() .filter(m -> m.get("kid").toString().equals(kid)) .map(m -> { try { return mapper.writeValueAsString(m); } catch (JsonProcessingException e) { throw new RuntimeException(e); } }) - .findFirst().orElseThrow(); + .findFirst() + .orElseThrow(() -> new RuntimeException("No JWK found with kid '%s' in JWKS response".formatted(kid))); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 260 - 272, The method handleJwks currently casts raw types and throws an unhelpful NoSuchElementException; change the cast to a typed collection (e.g., List<Map<String,Object>> keys = (List<Map<String,Object>>) mapped.get("keys")) and null-check keys, then use .findFirst().orElseThrow(() -> new NoSuchElementException("JWKS does not contain key with kid='" + kid + "'")) so the error includes the missing kid; keep the existing mapper.writeValueAsString(m) handling (including JsonProcessingException wrapping) and adjust any imports or generics to satisfy the compiler.core/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwksRefreshTest.java (1)
148-150:Thread.sleep(2000)is a fragile way to wait for a 1-second refresh cycle.With a 1-second refresh interval and a 2-second sleep, this leaves only ~1 second of margin for the refresh to actually complete (including HTTP round-trip and key parsing). Under CI load, this could be flaky. Consider polling with a timeout (e.g., retry JWT validation with the new key up to N times with short sleeps) instead of a fixed sleep.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwksRefreshTest.java` around lines 148 - 150, The test uses Thread.sleep(2000) after swapping currentJwkSet which is fragile; replace the fixed sleep with a polling loop that retries validating the JWT (or invoking the same validation path used elsewhere in JwksRefreshTest) until the new key is accepted or a timeout elapses (e.g., total 5s) with short pauses (e.g., 100-200ms) between attempts; specifically, after currentJwkSet.set(new JsonWebKeySet(publicKey2)) poll by calling the JWT validation helper used in this test (or the interceptor validation method) in a loop, break when validation succeeds with the new key, and fail the test if the timeout is reached. Ensure the loop uses a reasonable timeout and sleep interval to avoid flakiness under CI.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java`:
- Around line 74-83: The TimerTask scheduled via
router.getTimerManager().schedulePeriodicTask currently calls
setJwks(loadJwks(true)) directly, so any RuntimeException from
buildKeyMap/setJwks will escape and kill the scheduler; wrap the body of the
anonymous TimerTask.run in a try-catch that catches Exception (or Throwable),
logs the failure (include context like "JWKS refresh failed") via the existing
logger or router logger, and suppresses the exception so the task continues to
be scheduled; keep the original behavior of not overwriting keysByKid on failure
by only calling setJwks on success and do not rethrow the exception.
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwksRefreshTest.java`:
- Around line 35-36: Replace the hardcoded PROVIDER_PORT and
AUTH_INTERCEPTOR_PORT constants in JwksRefreshTest with 0 so the OS assigns free
ports, then, after each server/startup call, read the actual bound port from the
server instance (e.g. providerServer.getPort() or providerServer.getLocalPort(),
and authInterceptor.getPort()/getLocalPort()) and use those values wherever the
constants were referenced; ensure any URL constructions use the retrieved port
variables instead of the old constants.
---
Duplicate comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java`:
- Line 75: The condition in Jwks (around the use of
authorizationService.getJwksRefreshInterval()) can throw an NPE when
authorizationService is null; change the guard to check authorizationService !=
null before calling getJwksRefreshInterval (e.g. authorizationService != null &&
authorizationService.getJwksRefreshInterval() > 0) so the JWKS refresh logic
only runs when authorizationService is present—this keeps behavior consistent
with parseJwksUriIntoList which already handles a null authorizationService.
- Around line 169-198: parseJwksUriIntoList currently assumes
mapper.readValue(...).get("keys") returns a List and leaves loadJwks vulnerable
to NPE if "keys" is missing or not a list; change parseJwksUriIntoList to read
the JSON into a Map (e.g., Map<?,?> parsed = mapper.readValue(..., Map.class)),
then extract Object keysObj = parsed.get("keys') and if keysObj is an instance
of List<?> use that list, otherwise treat it as Collections.emptyList();
construct and return new JwkListByUri(uri, keysList) so downstream code (e.g.,
loadJwks) never receives null.
- Around line 108-110: The method getKeysByKid() exposes the volatile HashMap
reference and causes a TOCTOU if callers (e.g., JwtAuthInterceptor.handleJwt)
perform multiple lookups; replace it with a single-lookup accessor getKey(String
kid) that reads the volatile keysByKid reference once into a local variable and
returns local.get(kid), remove or make getKeysByKid() non-public, and update
callers (JwtAuthInterceptor.handleJwt and any other usages) to call getKey(kid)
so each key lookup is atomic with respect to the map reference.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptor.java`:
- Around line 130-136: The code does two separate reads of jwks.getKeysByKid()
(containsKey(kid) then get(kid)), causing a TOCTOU race if the JWKS map is
refreshed between calls; fix by reading the map once into a local variable
(e.g., Map<String,?> keys = jwks.getKeysByKid()) and then perform a single
lookup (keys.get(kid)) and null-check, or add a jwks.getKey(kid) helper that
atomically returns the RsaJsonWebKey (or null) so you only access the map once
before assigning to the RsaJsonWebKey variable.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java`:
- Line 65: Change the boxed Integer field jwksRefreshInterval to a primitive int
and make its setter setJwksRefreshInterval return void (not
AuthorizationService) to match other `@MCAttribute` setters; update the getter to
return int and remove any fluent return usage so Jwks.init() won’t risk NPE from
auto-unboxing. Apply the same change to the other attribute/setter pair
mentioned around lines 179-187 (convert boxed types to primitives, change their
setters to void, and adjust corresponding getters/usages accordingly).
---
Nitpick comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java`:
- Around line 142-143: In loadJwks(boolean suppressExceptions) replace the local
new ObjectMapper() with the existing instance field (this.mapper) so the method
reuses the shared ObjectMapper instead of allocating a new one each call; update
any local variable references to use mapper (or this.mapper) and remove the
redundant instantiation to avoid unnecessary allocations in the Jwks class.
- Around line 108-110: Change the public accessor getKeysByKid() to return the
interface type Map<String, RsaJsonWebKey> instead of the concrete HashMap to
avoid leaking implementation details: update the method signature of
getKeysByKid() to return Map<String, RsaJsonWebKey> and keep returning the
existing keysByKid field (which can remain a HashMap implementation), ensuring
any imports/reference types compile; adjust any callers if they relied on
HashMap-specific methods (prefer treating the result as a Map).
- Around line 260-272: The method handleJwks currently casts raw types and
throws an unhelpful NoSuchElementException; change the cast to a typed
collection (e.g., List<Map<String,Object>> keys = (List<Map<String,Object>>)
mapped.get("keys")) and null-check keys, then use .findFirst().orElseThrow(() ->
new NoSuchElementException("JWKS does not contain key with kid='" + kid + "'"))
so the error includes the missing kid; keep the existing
mapper.writeValueAsString(m) handling (including JsonProcessingException
wrapping) and adjust any imports or generics to satisfy the compiler.
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwksRefreshTest.java`:
- Around line 148-150: The test uses Thread.sleep(2000) after swapping
currentJwkSet which is fragile; replace the fixed sleep with a polling loop that
retries validating the JWT (or invoking the same validation path used elsewhere
in JwksRefreshTest) until the new key is accepted or a timeout elapses (e.g.,
total 5s) with short pauses (e.g., 100-200ms) between attempts; specifically,
after currentJwkSet.set(new JsonWebKeySet(publicKey2)) poll by calling the JWT
validation helper used in this test (or the interceptor validation method) in a
loop, break when validation succeeds with the new key, and fail the test if the
timeout is reached. Ensure the loop uses a reasonable timeout and sleep interval
to avoid flakiness under CI.
.../test/java/com/predic8/membrane/integration/withoutinternet/interceptor/JwksRefreshTest.java
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java (1)
270-280:⚠️ Potential issue | 🟡 MinorNPE in
handleJwksif any JWK in the"keys"array is missing a"kid"field.
m.get("kid").toString()at line 272 throws NPE for any key entry where"kid"is absent, aborting key setup entirely.orElseThrow()on line 280 also gives no diagnostic context when no match is found.🐛 Proposed fix
- return ((List<Map>) mapped.get("keys")).stream() - .filter(m -> m.get("kid").toString().equals(kid)) + return ((List<Map<String, Object>>) mapped.get("keys")).stream() + .filter(m -> kid != null && kid.equals(Objects.toString(m.get("kid"), null))) .map(m -> { try { return mapper.writeValueAsString(m); } catch (JsonProcessingException e) { throw new RuntimeException(e); } }) - .findFirst().orElseThrow(); + .findFirst().orElseThrow(() -> + new RuntimeException("No JWK with kid='" + kid + "' found in JWKS response"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 270 - 280, handleJwks currently assumes every entry in mapped.get("keys") has a "kid" and calls m.get("kid").toString(), causing NPE and an unhelpful orElseThrow() when no match is found; update handleJwks to safely handle missing "kid" by checking m.containsKey("kid") or m.get("kid") != null before calling toString (or use Objects.equals(kid, String.valueOf(m.get("kid")))), skip entries without a kid, and replace the bare orElseThrow() with an exception that includes context (e.g., the requested kid and a short description of the JWKS payload) so failures are diagnostic; refer to handleJwks, mapped, mapper, kid, and the "keys" list when making the change.core/src/main/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptor.java (1)
76-82:⚠️ Potential issue | 🟠 MajorNPE if
<jwks>is omitted from configuration.
jwks.init(router)at line 81 dereferencesjwksunconditionally. If the<jwks>child element is missing from the proxies configuration,jwksisnulland startup throws a bareNullPointerExceptionrather than a descriptive error. A pre-flight null check with aConfigurationExceptionwould surface a clear root cause.🐛 Proposed fix
`@Override` public void init() { super.init(); if(jwtRetriever == null) jwtRetriever = new HeaderJwtRetriever("Authorization","Bearer"); + + if (jwks == null) + throw new ConfigurationException("jwtAuth requires a <jwks> child element to be configured."); + jwks.init(router); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptor.java` around lines 76 - 82, In JwtAuthInterceptor.init(), avoid the NullPointerException by checking if the jwks field is null before calling jwks.init(router); if jwks is null throw a ConfigurationException with a clear message like "JwtAuthInterceptor: <jwks> element is missing in configuration" so startup fails with a descriptive error, otherwise call jwks.init(router); keep the existing jwtRetriever defaulting logic intact.
🧹 Nitpick comments (4)
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java (4)
198-204: Redundant double logging in the genericcatchblock — no stack trace captured.
e.toString()already includes the message; the secondlog.error(e.getMessage())is a no-op in terms of new information. Neither call records the stack trace, making diagnosis of unexpected failures difficult.♻️ Proposed fix
} catch (Exception e) { if (suppressExceptions) { - log.error(e.toString()); - log.error(e.getMessage()); + log.error("Unexpected error resolving JWKS URI {}", uri, e); } else { throw new RuntimeException(e); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 198 - 204, In Jwks.catch block where exceptions are handled, remove the redundant log.error(e.getMessage()) and replace the existing log.error call with one that logs the exception and its stack trace (e.g., log.error("...context...", e)) so that when suppressExceptions is true the full stacktrace is recorded; if suppressExceptions is false keep throwing new RuntimeException(e) unchanged.
151-158:loadJwkscreates a redundantObjectMapperthat shadows the instance field.
ObjectMapperis thread-safe after configuration and is already available asthis.mapper(line 55). Creating a new instance on every call (including every refresh cycle) is unnecessary and the shadowing makes the scope ambiguous.♻️ Proposed refactor
private List<Jwk> loadJwks(boolean suppressExceptions) { - ObjectMapper mapper = new ObjectMapper(); - return Arrays.stream(jwksUris.split(" ")) - .map(uri -> parseJwksUriIntoList(router.getResolverMap(), router.getConfiguration().getBaseLocation(), mapper, uri, suppressExceptions)) - .flatMap(l -> l.jwks().stream().map(jwkRaw -> convertToJwk(jwkRaw, mapper, l.uri(), suppressExceptions))) + return Arrays.stream(jwksUris.split(" ")) + .map(uri -> parseJwksUriIntoList(router.getResolverMap(), router.getConfiguration().getBaseLocation(), this.mapper, uri, suppressExceptions)) + .flatMap(l -> l.jwks().stream().map(jwkRaw -> convertToJwk(jwkRaw, this.mapper, l.uri(), suppressExceptions))) .filter(Objects::nonNull) .toList(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 151 - 158, The loadJwks method creates a new local ObjectMapper that shadows the instance field; replace the local "ObjectMapper mapper = new ObjectMapper();" with usage of the existing instance field "mapper" so the method uses the configured, thread-safe mapper (keep calls to parseJwksUriIntoList and convertToJwk unchanged, i.e., pass the instance field mapper into those calls), and remove the redundant local variable to avoid shadowing and repeated allocations.
117-119:getKeysByKid()exposes the mutable internalHashMap— prefer returning an unmodifiableMapview.The method returns the live, mutable
HashMapunder a volatile reference. Any caller that accidentally mutates it corrupts the in-memory key store without going through the volatile write, silently breaking JWKS validation until the next refresh. Changing the return type toMap<String, RsaJsonWebKey>and wrapping withCollections.unmodifiableMapcloses this gap.♻️ Proposed refactor
- public HashMap<String, RsaJsonWebKey> getKeysByKid() { - return keysByKid; + public Map<String, RsaJsonWebKey> getKeysByKid() { + return Collections.unmodifiableMap(keysByKid); }and update
buildKeyMap's return type:- private HashMap<String, RsaJsonWebKey> buildKeyMap(List<Jwk> jwks) { + private Map<String, RsaJsonWebKey> buildKeyMap(List<Jwk> jwks) {- private volatile HashMap<String, RsaJsonWebKey> keysByKid = new HashMap<>(); + private volatile Map<String, RsaJsonWebKey> keysByKid = new HashMap<>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 117 - 119, Change getKeysByKid to return a read-only view instead of exposing the mutable HashMap: change its return type to Map<String, RsaJsonWebKey> and return Collections.unmodifiableMap(keysByKid). Also update buildKeyMap to return Map<String, RsaJsonWebKey> (not HashMap) so the types align, and ensure any assignment to the volatile keysByKid still stores a mutable map internally before wrapping for callers; reference getKeysByKid, keysByKid, and buildKeyMap when making these edits.
74-92: Silent no-op refresh whenauthorizationServiceis not wired — worth logging.If
jwksUrisis configured butauthorizationServiceisnull, the JWKS is loaded once at startup and never refreshed. There's no log message to alert operators to this condition. Consider adding alog.info(orlog.warn) so the absence of auto-refresh is observable.💡 Suggested addition
setJwks(loadJwks(false)); if (authorizationService != null && authorizationService.getJwksRefreshInterval() > 0) { router.getTimerManager().schedulePeriodicTask(...); + } else if (authorizationService == null) { + log.info("No authorizationService configured — JWKS will not be refreshed automatically."); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 74 - 92, When jwksUris is configured but authorizationService is null the code calls setJwks(loadJwks(false)) once and never schedules a refresh; add a concise log (info or warn) immediately after the initial setJwks/loadJwks call to surface that no periodic JWKS refresh will be scheduled because authorizationService is null (referencing methods/fields: setJwks, loadJwks, authorizationService and router.getTimerManager().schedulePeriodicTask) so operators can observe the absence of auto-refresh.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java`:
- Around line 270-280: handleJwks currently assumes every entry in
mapped.get("keys") has a "kid" and calls m.get("kid").toString(), causing NPE
and an unhelpful orElseThrow() when no match is found; update handleJwks to
safely handle missing "kid" by checking m.containsKey("kid") or m.get("kid") !=
null before calling toString (or use Objects.equals(kid,
String.valueOf(m.get("kid")))), skip entries without a kid, and replace the bare
orElseThrow() with an exception that includes context (e.g., the requested kid
and a short description of the JWKS payload) so failures are diagnostic; refer
to handleJwks, mapped, mapper, kid, and the "keys" list when making the change.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptor.java`:
- Around line 76-82: In JwtAuthInterceptor.init(), avoid the
NullPointerException by checking if the jwks field is null before calling
jwks.init(router); if jwks is null throw a ConfigurationException with a clear
message like "JwtAuthInterceptor: <jwks> element is missing in configuration" so
startup fails with a descriptive error, otherwise call jwks.init(router); keep
the existing jwtRetriever defaulting logic intact.
---
Duplicate comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java`:
- Around line 178-207: The parseJwksUriIntoList method must explicitly guard
against a missing or non-array "keys" node before calling mapper.convertValue to
avoid JsonProcessingException or null propagation; update parseJwksUriIntoList
to read the tree into a JsonNode, fetch JsonNode keysNode = tree.path("keys"),
check keysNode.isMissingNode() or !keysNode.isArray() and in that case log an
appropriate message (respecting suppressExceptions) and return new
JwkListByUri(uri, emptyList()) instead of calling convertValue on the
MissingNode; only call mapper.convertValue(keysNode, List.class) when keysNode
is present and an array so JwkListByUri never receives null and loadJwks won't
hit an NPE.
---
Nitpick comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java`:
- Around line 198-204: In Jwks.catch block where exceptions are handled, remove
the redundant log.error(e.getMessage()) and replace the existing log.error call
with one that logs the exception and its stack trace (e.g.,
log.error("...context...", e)) so that when suppressExceptions is true the full
stacktrace is recorded; if suppressExceptions is false keep throwing new
RuntimeException(e) unchanged.
- Around line 151-158: The loadJwks method creates a new local ObjectMapper that
shadows the instance field; replace the local "ObjectMapper mapper = new
ObjectMapper();" with usage of the existing instance field "mapper" so the
method uses the configured, thread-safe mapper (keep calls to
parseJwksUriIntoList and convertToJwk unchanged, i.e., pass the instance field
mapper into those calls), and remove the redundant local variable to avoid
shadowing and repeated allocations.
- Around line 117-119: Change getKeysByKid to return a read-only view instead of
exposing the mutable HashMap: change its return type to Map<String,
RsaJsonWebKey> and return Collections.unmodifiableMap(keysByKid). Also update
buildKeyMap to return Map<String, RsaJsonWebKey> (not HashMap) so the types
align, and ensure any assignment to the volatile keysByKid still stores a
mutable map internally before wrapping for callers; reference getKeysByKid,
keysByKid, and buildKeyMap when making these edits.
- Around line 74-92: When jwksUris is configured but authorizationService is
null the code calls setJwks(loadJwks(false)) once and never schedules a refresh;
add a concise log (info or warn) immediately after the initial setJwks/loadJwks
call to surface that no periodic JWKS refresh will be scheduled because
authorizationService is null (referencing methods/fields: setJwks, loadJwks,
authorizationService and router.getTimerManager().schedulePeriodicTask) so
operators can observe the absence of auto-refresh.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptor.java (1)
74-81:⚠️ Potential issue | 🟠 MajorNPE when
jwksis not configured.
jwks.init(router)is called unconditionally, butjwksis never initialized to a non-null default — it staysnullif the user omits the<jwks>child element. The resultingNullPointerExceptionis far less informative than a proactive configuration check. Contrast withgetLongDescription()(line 228), which already guardsjwks != null.🛡️ Proposed fix
`@Override` public void init() { super.init(); if(jwtRetriever == null) jwtRetriever = new HeaderJwtRetriever("Authorization","Bearer"); + if (jwks == null) + throw new ConfigurationException("A <jwks> child element is required for jwtAuth."); jwks.init(router); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptor.java` around lines 74 - 81, The init() method in JwtAuthInterceptor calls jwks.init(router) unconditionally which causes an NPE when the <jwks> element is omitted; update init() to check jwks != null before invoking jwks.init(router) and handle the missing configuration proactively (e.g., throw an IllegalStateException with a clear message about missing jwks configuration or log an explicit error and return), while leaving the existing defaulting of jwtRetriever (HeaderJwtRetriever) intact; refer to JwtAuthInterceptor.init(), the jwks field, jwtRetriever/ HeaderJwtRetriever and the getLongDescription() guard for guidance.core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java (2)
269-280:⚠️ Potential issue | 🟠 MajorNPE when a JWK in the set has no
"kid"field.
m.get("kid")returnsnullfor any JWK that omits the"kid"member; calling.toString()on that null immediately throwsNullPointerException. Additionally,orElseThrow()at line 279 carries no message, making the "not found" case equally opaque.🛡️ Proposed fix
private String handleJwks(ObjectMapper mapper, Map<String, Object> mapped) { return ((List<Map>) mapped.get("keys")).stream() - .filter(m -> m.get("kid").toString().equals(kid)) + .filter(m -> kid != null && kid.equals(Objects.toString(m.get("kid"), null))) .map(m -> { try { return mapper.writeValueAsString(m); } catch (JsonProcessingException e) { throw new RuntimeException(e); } }) - .findFirst().orElseThrow(); + .findFirst().orElseThrow(() -> new RuntimeException( + "No JWK with kid '%s' found in JWKS response.".formatted(kid))); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 269 - 280, handleJwks currently calls m.get("kid").toString() which throws NPE if "kid" is missing and uses orElseThrow() with no message; fix by filtering null-safe: use Optional.ofNullable(m.get("kid")).map(Object::toString).filter(s -> s.equals(kid)) (or check m.containsKey("kid") && kid.equals(String.valueOf(m.get("kid")))) before mapping to JSON, and replace orElseThrow() with orElseThrow(() -> new RuntimeException("JWK with kid='" + kid + "' not found in set")) so the method (handleJwks) returns a clear error when the key is absent while avoiding NPEs from toString().
239-241:⚠️ Potential issue | 🟠 MajorChange
@MCAttributeto@MCChildElementfor thesetHttpClientConfigmethod.
HttpClientConfigurationis a complex nested configuration bean with multiple sub-properties (ConnectionConfiguration, ProxyConfiguration, AuthenticationConfiguration, SSLParser, etc.).@MCAttributeis only for simple scalar attributes; complex objects require@MCChildElement. All 7 other similar setters throughout the codebase correctly use@MCChildElement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 239 - 241, The setter method setHttpClientConfig currently annotated with `@MCAttribute` should be changed to `@MCChildElement` because HttpClientConfiguration is a nested configuration bean (with ConnectionConfiguration, ProxyConfiguration, AuthenticationConfiguration, SSLParser, etc.); locate the setHttpClientConfig(HttpClientConfiguration httpClientConfig) method in class Jwks and replace the `@MCAttribute` annotation with `@MCChildElement` so the parser treats it as a complex child element (consistent with the other seven similar setters).
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java (2)
197-203: Redundant double-logging in the genericcatchblock.
e.toString()already contains the class name and the message, solog.error(e.getMessage())on the next line prints a duplicate (and incomplete) substring of the first line.♻️ Proposed fix
- } catch (Exception e) { - if (suppressExceptions) { - log.error(e.toString()); - log.error(e.getMessage()); - } else { - throw new RuntimeException(e); - } + } catch (Exception e) { + if (suppressExceptions) { + log.error("Unexpected error resolving JWKS from {}: {}", uri, e.toString()); + } else { + throw new RuntimeException(e); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 197 - 203, In Jwks class inside the catch block that checks suppressExceptions, remove the redundant log.error(e.getMessage()) and replace the remaining logging with a single call that includes the exception (e.g. log.error("message", e)) so you log a clear contextual message plus the full stacktrace; keep the suppressExceptions conditional logic intact and only change the logging lines in that catch block.
121-132:buildKeyMapstream collector can be simplified.The three-arg
collectwith an explicitSupplier/BiConsumer/BiConsumeris more verbose than the built-inCollectors.toMap, and the lambda() -> new HashMap<>()can be written as a method reference.♻️ Proposed refactor
- var keyMap = jwks.stream() - .map(this::extractRsaJsonWebKey) - .collect( - () -> new HashMap<String, RsaJsonWebKey>(), - (m,e) -> m.put(e.getKeyId(),e), - HashMap::putAll - ); + var keyMap = jwks.stream() + .map(this::extractRsaJsonWebKey) + .collect(Collectors.toMap(RsaJsonWebKey::getKeyId, Function.identity()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 121 - 132, The current buildKeyMap method uses a verbose three-arg collect; simplify it by using Collectors.toMap: after mapping with extractRsaJsonWebKey, collect into a HashMap with Collectors.toMap(RsaJsonWebKey::getKeyId, Function.identity(), (a,b) -> b, HashMap::new) to create the map and resolve duplicate keyIds by keeping the latter; keep the existing empty-check and RuntimeException. Update imports as needed and reference buildKeyMap and extractRsaJsonWebKey when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java`:
- Around line 269-280: handleJwks currently calls m.get("kid").toString() which
throws NPE if "kid" is missing and uses orElseThrow() with no message; fix by
filtering null-safe: use
Optional.ofNullable(m.get("kid")).map(Object::toString).filter(s ->
s.equals(kid)) (or check m.containsKey("kid") &&
kid.equals(String.valueOf(m.get("kid")))) before mapping to JSON, and replace
orElseThrow() with orElseThrow(() -> new RuntimeException("JWK with kid='" + kid
+ "' not found in set")) so the method (handleJwks) returns a clear error when
the key is absent while avoiding NPEs from toString().
- Around line 239-241: The setter method setHttpClientConfig currently annotated
with `@MCAttribute` should be changed to `@MCChildElement` because
HttpClientConfiguration is a nested configuration bean (with
ConnectionConfiguration, ProxyConfiguration, AuthenticationConfiguration,
SSLParser, etc.); locate the setHttpClientConfig(HttpClientConfiguration
httpClientConfig) method in class Jwks and replace the `@MCAttribute` annotation
with `@MCChildElement` so the parser treats it as a complex child element
(consistent with the other seven similar setters).
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptor.java`:
- Around line 74-81: The init() method in JwtAuthInterceptor calls
jwks.init(router) unconditionally which causes an NPE when the <jwks> element is
omitted; update init() to check jwks != null before invoking jwks.init(router)
and handle the missing configuration proactively (e.g., throw an
IllegalStateException with a clear message about missing jwks configuration or
log an explicit error and return), while leaving the existing defaulting of
jwtRetriever (HeaderJwtRetriever) intact; refer to JwtAuthInterceptor.init(),
the jwks field, jwtRetriever/ HeaderJwtRetriever and the getLongDescription()
guard for guidance.
---
Duplicate comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java`:
- Around line 117-119: No change required: the getKeyByKid(String kid) method's
current implementation returning Optional.ofNullable(keysByKid.get(kid)) is fine
under the publish-once refresh pattern; leave Jwks.getKeyByKid as-is and no
modifications to keysByKid access or volatility are needed.
- Around line 177-206: In parseJwksUriIntoList, the call
mapper.convertValue(mapper.readTree(resolve).path("keys"), List.class) can
receive a MissingNode (or wrap to IllegalArgumentException) so add an explicit
guard: read the JSON tree first into a JsonNode (e.g., JsonNode root =
mapper.readTree(resolve)), check root.has("keys") &&
!root.get("keys").isMissingNode() && root.get("keys").isArray() before
converting; if the keys field is absent or not an array, handle according to
suppressExceptions (log and return new JwkListByUri(uri, emptyList()) or throw
ConfigurationException), otherwise call mapper.convertValue(root.get("keys"),
List.class) to avoid the opaque IllegalArgumentException path; keep existing
exception handling for ResourceRetrievalException and JsonProcessingException.
- Around line 64-93: The timer-task error handling in init(Router) is correct:
the TimerTask around loadJwks(true) already catches exceptions and the
empty-list guard prevents overwriting keysByKid, so no code changes are needed;
leave the init, loadJwks, setJwks and the scheduled TimerTask as implemented.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptor.java`:
- Line 131: Replace the previous two-step lookup with a single atomic retrieval
to avoid TOCTOU: in JwtAuthInterceptor, use jwks.getKeyByKid(kid).orElseThrow(()
-> new JWTException(ERROR_UNKNOWN_KEY, ERROR_UNKNOWN_KEY_ID)) so the lookup and
exception handling occur in one call (refers to the getKeyByKid method and kid
variable).
---
Nitpick comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java`:
- Around line 197-203: In Jwks class inside the catch block that checks
suppressExceptions, remove the redundant log.error(e.getMessage()) and replace
the remaining logging with a single call that includes the exception (e.g.
log.error("message", e)) so you log a clear contextual message plus the full
stacktrace; keep the suppressExceptions conditional logic intact and only change
the logging lines in that catch block.
- Around line 121-132: The current buildKeyMap method uses a verbose three-arg
collect; simplify it by using Collectors.toMap: after mapping with
extractRsaJsonWebKey, collect into a HashMap with
Collectors.toMap(RsaJsonWebKey::getKeyId, Function.identity(), (a,b) -> b,
HashMap::new) to create the map and resolve duplicate keyIds by keeping the
latter; keep the existing empty-check and RuntimeException. Update imports as
needed and reference buildKeyMap and extractRsaJsonWebKey when making the
change.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
core/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwksRefreshTest.java (2)
150-151:Thread.sleep-based refresh wait is inherently flaky.
JWKS_REFRESH_INTERVAL * 1_000 * 2(2 s) leaves no margin for scheduler jitter, GC pauses, or a loaded CI runner. If the refresh fires even slightly after the sleep elapses the assertions on Lines 159 and 167 will produce misleading failures.Consider replacing the sleep with a short polling loop that retries the request until the expected status appears (or a hard timeout is exceeded):
// Poll until key2 is accepted (max ~5 s) long deadline = System.currentTimeMillis() + 5_000; int status; do { Exchange poll = new Request.Builder() .get(authInterceptorUrl) .header("Authorization", "Bearer " + createJwt(privateKey2)) .buildExchange(); hc.call(poll); status = poll.getResponse().getStatusCode(); if (status != 200) Thread.sleep(200); } while (status != 200 && System.currentTimeMillis() < deadline); assertEquals(200, status, "JWKS refresh did not propagate in time");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwksRefreshTest.java` around lines 150 - 151, Replace the fragile Thread.sleep-based wait (where currentJwkSet.set(...) is followed by Thread.sleep(JWKS_REFRESH_INTERVAL * 1_000 * 2)) with a polling loop that repeatedly issues the same request used in the test (use authInterceptorUrl and createJwt(privateKey2) to build the request and call hc.call on the Exchange) until the response status becomes 200 or a hard deadline is exceeded; poll with a short delay (e.g., 200ms) between attempts and assert after the loop that the final status is 200 with a helpful message like "JWKS refresh did not propagate in time" so scheduler jitter/GC pauses don’t cause flaky failures.
154-167:exc2/exc3variable names are out of execution order.
exc3is declared and used beforeexc2, making the numeric suffixes misleading. Rename them toexc3→exc2andexc2→exc3(or useexcNewKey/excOldKey) to match the execution sequence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwksRefreshTest.java` around lines 154 - 167, In JwksRefreshTest.java swap or rename the misordered Exchange variables so names reflect execution order: change the first-created "exc3" to "exc2" (or "excNewKey") and the subsequently-created "exc2" to "exc3" (or "excOldKey"), and update all usages (the Request.Builder(), hc.call(...), and assertEquals(...).getResponse() checks) to use the new identifiers so the newer-key request is declared/used first and the old-key request second.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwksRefreshTest.java`:
- Around line 67-81: Replace the brittle Thread.sleep(1000) / implicit startup
assumption with an explicit readiness poll for both the JWKS provider and the
jwtValidator router: after calling DefaultRouter.start() on jwtValidator (and
after the JWKS provider start), loop with short delays and a timeout to attempt
a lightweight TCP/HTTP probe (e.g., connect to AUTH_INTERCEPTOR_PORT or send an
HTTP HEAD/GET) until successful before proceeding to hc.call(exc1) in
testRefresh(); this ensures the listener bound state is detected
deterministically (you can use Awaitility or a simple retry loop) and reference
the Thread.sleep(1000), jwtValidator, DefaultRouter.start(), and hc.call(exc1)
locations when implementing the change.
- Around line 130-132: teardown currently calls jwksProvider.stop() then
jwtValidator.stop(), but if jwksProvider.stop() throws the jwtValidator.stop()
call is skipped; wrap the jwksProvider.stop() in a try block and ensure
jwtValidator.stop() is always executed in a finally (or vice versa), capturing
any thrown exceptions and rethrowing one (adding the other as suppressed) so
both servers are attempted to be stopped; update the teardown() method to use
try/finally (or try/catch with addSuppressed) around jwksProvider.stop() and
jwtValidator.stop() to guarantee jwtValidator.stop() runs even when
jwksProvider.stop() fails.
- Line 125: The HttpClient created in buildAuthorizationService via
authService.setHttpClient(new HttpClient()) is never closed; change
buildAuthorizationService to assign that HttpClient instance to a static (or
otherwise shared) field (e.g., a private static HttpClient testHttpClient) so it
can be referenced later, and then close that stored HttpClient in teardown()
(invoke close() on testHttpClient and null it) to ensure the connection pool is
released; keep references to HttpClient, buildAuthorizationService,
authService.setHttpClient(...), and teardown() to locate the changes.
---
Duplicate comments:
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwksRefreshTest.java`:
- Around line 35-36: The test currently uses hardcoded port constants
PROVIDER_PORT and AUTH_INTERCEPTOR_PORT in JwksRefreshTest which can cause
collisions in parallel or CI runs; change these to dynamically allocated/free
ports (e.g., via a test utility like getFreePort()/PortUtil or by binding
ServerSocket(0) and using its assigned port) and update any places that
reference PROVIDER_PORT/AUTH_INTERCEPTOR_PORT to use the allocated values so
tests no longer rely on fixed ports.
---
Nitpick comments:
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwksRefreshTest.java`:
- Around line 150-151: Replace the fragile Thread.sleep-based wait (where
currentJwkSet.set(...) is followed by Thread.sleep(JWKS_REFRESH_INTERVAL * 1_000
* 2)) with a polling loop that repeatedly issues the same request used in the
test (use authInterceptorUrl and createJwt(privateKey2) to build the request and
call hc.call on the Exchange) until the response status becomes 200 or a hard
deadline is exceeded; poll with a short delay (e.g., 200ms) between attempts and
assert after the loop that the final status is 200 with a helpful message like
"JWKS refresh did not propagate in time" so scheduler jitter/GC pauses don’t
cause flaky failures.
- Around line 154-167: In JwksRefreshTest.java swap or rename the misordered
Exchange variables so names reflect execution order: change the first-created
"exc3" to "exc2" (or "excNewKey") and the subsequently-created "exc2" to "exc3"
(or "excOldKey"), and update all usages (the Request.Builder(), hc.call(...),
and assertEquals(...).getResponse() checks) to use the new identifiers so the
newer-key request is declared/used first and the old-key request second.
.../test/java/com/predic8/membrane/integration/withoutinternet/interceptor/JwksRefreshTest.java
Show resolved
Hide resolved
core/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwksRefreshTest.java
Outdated
Show resolved
Hide resolved
.../test/java/com/predic8/membrane/integration/withoutinternet/interceptor/JwksRefreshTest.java
Show resolved
Hide resolved
predic8
approved these changes
Feb 23, 2026
predic8
requested changes
Feb 23, 2026
added 11 commits
March 2, 2026 10:56
bridgerdier
force-pushed
the
jwks-auto-refresh
branch
from
bfb09aa to
8597a2e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java (1)
177-183:⚠️ Potential issue | 🟠 MajorHarden JWKS URI parsing and close the response stream.
loadJwks()expects a non-null list (Line 154 stream use), and this parse path should also closeInputStreamto avoid leaks during periodic refresh.🔧 Proposed fix
private JwkListByUri parseJwksUriIntoList(ResolverMap resolverMap, String baseLocation, ObjectMapper mapper, String uri, boolean suppressExceptions) { try { - InputStream resolve = authorizationService != null ? - authorizationService.resolve(resolverMap, baseLocation, uri) : - resolverMap.resolve(ResolverMap.combine(baseLocation, uri)); - return new JwkListByUri(uri, mapper.convertValue(mapper.readTree(resolve).path("keys"), List.class)); + try (InputStream resolve = authorizationService != null + ? authorizationService.resolve(resolverMap, baseLocation, uri) + : resolverMap.resolve(ResolverMap.combine(baseLocation, uri))) { + var keysNode = mapper.readTree(resolve).path("keys"); + var keys = keysNode.isArray() ? mapper.convertValue(keysNode, List.class) : emptyList(); + return new JwkListByUri(uri, keys); + } } catch (JsonProcessingException e) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 177 - 183, parseJwksUriIntoList currently opens an InputStream via authorizationService.resolve / resolverMap.resolve but doesn't close it and may return a null keys list; change the implementation to use a try-with-resources around the InputStream obtained from authorizationService.resolve or resolverMap.resolve so the stream is always closed, catch JsonProcessingException/IOException as before, and ensure the JwkListByUri is constructed with a non-null List (use Collections.emptyList() as a fallback if mapper.convertValue or readTree.path("keys") yields null). Refer to parseJwksUriIntoList, JwkListByUri, authorizationService.resolve, resolverMap.resolve and mapper.readTree when making the change.core/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwksRefreshTest.java (1)
125-132:⚠️ Potential issue | 🟡 MinorEnsure test resources are always released in teardown.
Line 125 creates a
HttpClientwith no owning close path, and Lines 130-132 can skipjwtValidator.stop()if the first stop throws.🛠️ Suggested cleanup
public class JwksRefreshTest { + static HttpClient authServiceHttpClient; @@ private static `@NotNull` AuthorizationService buildAuthorizationService(int jwksRefreshInterval) { @@ - authService.setHttpClient(new HttpClient()); + authServiceHttpClient = new HttpClient(); + authService.setHttpClient(authServiceHttpClient); return authService; } @@ `@AfterAll` public static void teardown() throws IOException { - jwksProvider.stop(); - jwtValidator.stop(); + IOException thrown = null; + try { + jwksProvider.stop(); + } catch (IOException e) { + thrown = e; + } finally { + try { + jwtValidator.stop(); + } catch (IOException e) { + if (thrown != null) thrown.addSuppressed(e); + else thrown = e; + } + if (authServiceHttpClient != null) { + authServiceHttpClient.close(); + authServiceHttpClient = null; + } + } + if (thrown != null) throw thrown; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwksRefreshTest.java` around lines 125 - 132, The test creates a new HttpClient via authService.setHttpClient(new HttpClient()) without closing it and the `@AfterAll` teardown calls jwksProvider.stop() then jwtValidator.stop() so an exception from jwksProvider.stop() can skip jwtValidator.stop(); fix by retaining the HttpClient instance you create (e.g., a field or getter from authService) and close it in teardown, and make teardown robust by ensuring both stops run (wrap jwksProvider.stop() in try/catch or use try { jwksProvider.stop(); } finally { jwtValidator.stop(); } so jwtValidator.stop() always executes), updating JwksRefreshTest accordingly to reference authService.setHttpClient/new HttpClient, jwksProvider.stop(), and jwtValidator.stop().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java`:
- Around line 270-272: The stream filter in Jwks.java risks a NPE by calling
m.get("kid").toString(); change the predicate to safely handle missing kid
values (e.g., retrieve Object k = m.get("kid"); return k != null &&
k.toString().equals(kid)) or use Objects.equals(String.valueOf(m.get("kid")),
kid) to avoid nullable dereference; apply the same safe-check pattern to the
other occurrence that compares kid (the map variable m in the stream and its
subsequent map/flatMap usage).
---
Duplicate comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java`:
- Around line 177-183: parseJwksUriIntoList currently opens an InputStream via
authorizationService.resolve / resolverMap.resolve but doesn't close it and may
return a null keys list; change the implementation to use a try-with-resources
around the InputStream obtained from authorizationService.resolve or
resolverMap.resolve so the stream is always closed, catch
JsonProcessingException/IOException as before, and ensure the JwkListByUri is
constructed with a non-null List (use Collections.emptyList() as a fallback if
mapper.convertValue or readTree.path("keys") yields null). Refer to
parseJwksUriIntoList, JwkListByUri, authorizationService.resolve,
resolverMap.resolve and mapper.readTree when making the change.
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwksRefreshTest.java`:
- Around line 125-132: The test creates a new HttpClient via
authService.setHttpClient(new HttpClient()) without closing it and the `@AfterAll`
teardown calls jwksProvider.stop() then jwtValidator.stop() so an exception from
jwksProvider.stop() can skip jwtValidator.stop(); fix by retaining the
HttpClient instance you create (e.g., a field or getter from authService) and
close it in teardown, and make teardown robust by ensuring both stops run (wrap
jwksProvider.stop() in try/catch or use try { jwksProvider.stop(); } finally {
jwtValidator.stop(); } so jwtValidator.stop() always executes), updating
JwksRefreshTest accordingly to reference authService.setHttpClient/new
HttpClient, jwksProvider.stop(), and jwtValidator.stop().
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.javacore/src/main/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.javacore/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwksRefreshTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptorUnitTests.javacore/src/test/java/com/predic8/membrane/core/openapi/validators/security/JWTInterceptorAndSecurityValidatorTest.javacore/src/test/java/com/predic8/membrane/core/security/JWTSecuritySchemeTest.java
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java
Outdated
Show resolved
Hide resolved
…e, non-empty `kid` values.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java (2)
99-101:⚠️ Potential issue | 🟡 MinorValidate before mutating internal state in
setJwks.Line 99 assigns
this.jwksbefore the null check, so a failing call can still leave internal state asnull. Move the validation ahead of assignment.Suggested fix
public Jwks setJwks(List<Jwk> jwks) { - this.jwks = jwks; // unnecessary, mainly for consistency when debugging if (jwks == null) throw new ConfigurationException("JWKs list must not be null."); + this.jwks = jwks; // unnecessary, mainly for consistency when debugging if (router != null) // set in init, so we can't update prior to that call this.keysByKid = buildKeyMap(jwks); return this; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 99 - 101, In setJwks, perform the null validation on the parameter jwks before mutating internal state: check if jwks == null and throw ConfigurationException if so, then assign this.jwks = jwks; this ensures the object's state is not changed when validation fails (references: setJwks method, this.jwks, parameter jwks).
178-180:⚠️ Potential issue | 🟠 MajorGuard missing/non-array
"keys"to keepjwks()non-null.Line 180 can still produce a
nullor invalid list shape for"keys", and downstream Line 155 streamsl.jwks()directly. Normalize to an always non-null list before constructingJwkListByUri.Suggested fix
private JwkListByUri parseJwksUriIntoList(ResolverMap resolverMap, String baseLocation, ObjectMapper mapper, String uri, boolean suppressExceptions) { try (InputStream jwksResponse = resolveToken(resolverMap, baseLocation, uri)) { - return new JwkListByUri(uri, mapper.convertValue(mapper.readTree(jwksResponse).path("keys"), List.class)); + var root = mapper.readTree(jwksResponse); + var keysNode = root.path("keys"); + List<?> keys = keysNode.isArray() + ? mapper.convertValue(keysNode, List.class) + : emptyList(); + return new JwkListByUri(uri, keys == null ? emptyList() : keys); } catch (Exception e) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 178 - 180, The parseJwksUriIntoList method can pass a null or non-list value for the "keys" field into the JwkListByUri constructor; update parseJwksUriIntoList so it reads the "keys" node via mapper.readTree(jwksResponse).path("keys"), checks if it is an array (or non-null), and if not replaces it with an empty List before calling new JwkListByUri(uri, ...); ensure the normalized non-null List result is what JwkListByUri and its jwks() accessor receive so downstream streaming over l.jwks() never gets a null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java`:
- Around line 153-154: The tokenization of jwksUris currently uses split(" ")
which yields empty strings for consecutive spaces and misses other whitespace;
change the splitting to use a whitespace regex (e.g. "\\s+") and filter out
blank/empty entries before mapping to parseJwksUriIntoList so only valid URIs
are passed to parseJwksUriIntoList(router.getResolverMap(),
router.getConfiguration().getBaseLocation(), mapper, uri, suppressExceptions);
ensure each token is trimmed and skipped if blank.
- Around line 271-272: In handleJwks (class Jwks) avoid the unchecked cast by
first validating that mapped.get("keys") is a List and that each element is a
Map before using it; if the value is missing or not the expected shape,
throw/return a clear error (e.g., IllegalArgumentException with a descriptive
message) instead of casting directly, then convert the validated List<Map> and
continue the stream/filter by comparing the "kid" value as before.
---
Duplicate comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java`:
- Around line 99-101: In setJwks, perform the null validation on the parameter
jwks before mutating internal state: check if jwks == null and throw
ConfigurationException if so, then assign this.jwks = jwks; this ensures the
object's state is not changed when validation fails (references: setJwks method,
this.jwks, parameter jwks).
- Around line 178-180: The parseJwksUriIntoList method can pass a null or
non-list value for the "keys" field into the JwkListByUri constructor; update
parseJwksUriIntoList so it reads the "keys" node via
mapper.readTree(jwksResponse).path("keys"), checks if it is an array (or
non-null), and if not replaces it with an empty List before calling new
JwkListByUri(uri, ...); ensure the normalized non-null List result is what
JwkListByUri and its jwks() accessor receive so downstream streaming over
l.jwks() never gets a null.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.javacore/src/main/java/com/predic8/membrane/core/transport/http/HttpEndpointListener.javacore/src/main/java/com/predic8/membrane/core/util/TimerTaskUtil.java
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/predic8/membrane/core/util/TimerTaskUtil.java
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java (2)
178-181:⚠️ Potential issue | 🟠 MajorNPE if JWKS response does not contain a
"keys"field.
mapper.readTree(jwksResponse).path("keys")returns aMissingNodewhen the field is absent. WhenconvertValueis called onMissingNodetargetingList.class, it returnsnull. This causes an NPE later whenl.jwks().stream()is called inloadJwks().🐛 Proposed fix: handle missing or null "keys"
private JwkListByUri parseJwksUriIntoList(ResolverMap resolverMap, String baseLocation, ObjectMapper mapper, String uri, boolean suppressExceptions) { try (InputStream jwksResponse = resolveToken(resolverMap, baseLocation, uri)) { - return new JwkListByUri(uri, mapper.convertValue(mapper.readTree(jwksResponse).path("keys"), List.class)); + List<?> keys = mapper.convertValue(mapper.readTree(jwksResponse).path("keys"), List.class); + return new JwkListByUri(uri, keys != null ? keys : emptyList()); } catch (Exception e) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 178 - 181, parseJwksUriIntoList can produce a JwkListByUri with a null keys list because mapper.readTree(...).path("keys") yields a MissingNode and mapper.convertValue returns null; update parseJwksUriIntoList to inspect the JsonNode returned from mapper.readTree(...).path("keys") and if it is missing or null, substitute an empty List before constructing the JwkListByUri (so downstream loadJwks() can safely call l.jwks().stream()); specifically check the node with JsonNode methods (e.g., isMissingNode/isNull) and only call mapper.convertValue when a real node exists, otherwise use Collections.emptyList().
98-105:⚠️ Potential issue | 🟠 MajorNull check occurs after assignment, corrupting state on invalid input.
Line 99 assigns
this.jwks = jwksbefore the null check on lines 100-101. Ifjwksis null, the field gets assigned null, and then the exception is thrown—leaving the object in a corrupted state.🐛 Proposed fix: check null before assignment
`@MCChildElement` public Jwks setJwks(List<Jwk> jwks) { - this.jwks = jwks; // unnecessary, mainly for consistency when debugging if (jwks == null) throw new ConfigurationException("JWKs list must not be null."); + this.jwks = jwks; // unnecessary, mainly for consistency when debugging if (router != null) // set in init, so we can't update prior to that call this.keysByKid = buildKeyMap(jwks); return this; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 98 - 105, In Jwks.setJwks, the method currently assigns this.jwks before validating the input which can corrupt object state; change the logic so you validate jwks for null first (throw ConfigurationException if null) and only then assign this.jwks and, if router != null, update this.keysByKid via buildKeyMap(jwks); ensure references are to the existing method Jwks.setJwks, the field this.jwks, the router check, and the keysByKid = buildKeyMap(jwks) call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java`:
- Around line 182-192: In Jwks exception handling, avoid double-logging when
suppressExceptions is true by removing the log.error(e.toString()) inside the
switch default branch and either (a) set message = e.getMessage() in the default
branch and let the later conditional log.error(message) handle logging once, or
(b) prefer a single detailed log.error("Could not retrieve JWK keys from %s:
%s", uri, e) in the later suppressExceptions block to include the full
exception; update the switch over `e` in class Jwks so only one log.error call
happens (refer to variables/methods: e, suppressExceptions, message, log.error,
JwkListByUri).
---
Duplicate comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java`:
- Around line 178-181: parseJwksUriIntoList can produce a JwkListByUri with a
null keys list because mapper.readTree(...).path("keys") yields a MissingNode
and mapper.convertValue returns null; update parseJwksUriIntoList to inspect the
JsonNode returned from mapper.readTree(...).path("keys") and if it is missing or
null, substitute an empty List before constructing the JwkListByUri (so
downstream loadJwks() can safely call l.jwks().stream()); specifically check the
node with JsonNode methods (e.g., isMissingNode/isNull) and only call
mapper.convertValue when a real node exists, otherwise use
Collections.emptyList().
- Around line 98-105: In Jwks.setJwks, the method currently assigns this.jwks
before validating the input which can corrupt object state; change the logic so
you validate jwks for null first (throw ConfigurationException if null) and only
then assign this.jwks and, if router != null, update this.keysByKid via
buildKeyMap(jwks); ensure references are to the existing method Jwks.setJwks,
the field this.jwks, the router check, and the keysByKid = buildKeyMap(jwks)
call.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/com/predic8/membrane/core/exchangestore/FileExchangeStore.javacore/src/main/java/com/predic8/membrane/core/interceptor/balancer/BalancerHealthMonitor.javacore/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/main/java/com/predic8/membrane/core/interceptor/balancer/BalancerHealthMonitor.java
- core/src/main/java/com/predic8/membrane/core/exchangestore/FileExchangeStore.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java (3)
183-193:⚠️ Potential issue | 🟡 MinorConsolidate suppressed-exception logging to a single entry.
In the default branch, suppressed mode logs
e.toString()and then logsmessageagain, creating duplicate error logs for one failure.🔧 Suggested fix
default -> { - if (suppressExceptions) log.error(e.toString()); yield e.getMessage(); } }; if (suppressExceptions) { - log.error(message); + log.error(message, e); return new JwkListByUri(uri, emptyList());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 183 - 193, The default branch in Jwks error handling currently logs e.toString() and then logs message again when suppressExceptions is true, producing duplicate logs; remove the early log.error(e.toString()) and instead log once (e.g., call log.error(message, e) or log.error(message) depending on desired detail) where suppressExceptions is checked before returning the new JwkListByUri(uri, emptyList()); update the default switch branch to only set message (or include e details in message) and let the later suppressExceptions block perform the single logging action.
271-272:⚠️ Potential issue | 🟡 MinorValidate JWKS
keysshape before unchecked cast.
(List<Map>) mapped.get("keys")can throwClassCastExceptionfor malformed payloads and loses a clear config/error message.🔧 Suggested fix
private String handleJwks(ObjectMapper mapper, Map<String, Object> mapped) { - return ((List<Map>) mapped.get("keys")).stream() + Object keysObj = mapped.get("keys"); + if (!(keysObj instanceof List<?> keys)) + throw new RuntimeException("Invalid JWKS: 'keys' must be an array."); + + return keys.stream() + .filter(Map.class::isInstance) + .map(Map.class::cast) .filter(m -> Objects.equals(m.get("kid"), kid)) .map(m -> { try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 271 - 272, The cast (List<Map>) mapped.get("keys") in Jwks.java is unsafe and can throw ClassCastException; before casting, check that mapped.get("keys") is non-null and an instance of List, and that each element is a Map (or Map<?,?>). If the shape is invalid, throw a clear, descriptive exception (e.g., IllegalArgumentException or a custom JWKS parsing exception) mentioning the bad "keys" payload and include the actual type/value for diagnostics; then safely cast and continue with the existing .stream().filter(...) logic that matches on "kid".
99-101:⚠️ Potential issue | 🟡 MinorValidate
jwksbefore assigning to field.
this.jwksis set before the null check. If callers catch the exception, the object can remain in an invalid state (jwks == null).🔧 Suggested fix
public Jwks setJwks(List<Jwk> jwks) { - this.jwks = jwks; // unnecessary, mainly for consistency when debugging if (jwks == null) throw new ConfigurationException("JWKs list must not be null."); + this.jwks = jwks; // unnecessary, mainly for consistency when debugging if (router != null) // set in init, so we can't update prior to that call this.keysByKid = buildKeyMap(jwks); return this; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 99 - 101, The constructor (or setter) in class Jwks currently assigns this.jwks before validating the parameter; change the order so you validate jwks first (if (jwks == null) throw new ConfigurationException(...)) and only then assign this.jwks = jwks, ensuring the Jwks instance cannot be left with a null jwks field if an exception is thrown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java`:
- Around line 79-80: The check treating jwksUris as unset should also consider
whitespace-only values; update the condition in the Jwks class where jwksUris is
tested (the if that currently uses jwksUris == null || jwksUris.isEmpty()) to
treat strings like " " as unset by trimming or using isBlank (e.g., jwksUris
== null || jwksUris.trim().isEmpty() or jwksUris == null || jwksUris.isBlank()),
so subsequent logic that relies on a configured JWKS URI does not receive
whitespace-only input.
---
Duplicate comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java`:
- Around line 183-193: The default branch in Jwks error handling currently logs
e.toString() and then logs message again when suppressExceptions is true,
producing duplicate logs; remove the early log.error(e.toString()) and instead
log once (e.g., call log.error(message, e) or log.error(message) depending on
desired detail) where suppressExceptions is checked before returning the new
JwkListByUri(uri, emptyList()); update the default switch branch to only set
message (or include e details in message) and let the later suppressExceptions
block perform the single logging action.
- Around line 271-272: The cast (List<Map>) mapped.get("keys") in Jwks.java is
unsafe and can throw ClassCastException; before casting, check that
mapped.get("keys") is non-null and an instance of List, and that each element is
a Map (or Map<?,?>). If the shape is invalid, throw a clear, descriptive
exception (e.g., IllegalArgumentException or a custom JWKS parsing exception)
mentioning the bad "keys" payload and include the actual type/value for
diagnostics; then safely cast and continue with the existing
.stream().filter(...) logic that matches on "kid".
- Around line 99-101: The constructor (or setter) in class Jwks currently
assigns this.jwks before validating the parameter; change the order so you
validate jwks first (if (jwks == null) throw new ConfigurationException(...))
and only then assign this.jwks = jwks, ensuring the Jwks instance cannot be left
with a null jwks field if an exception is thrown.
added 3 commits
March 3, 2026 14:56
…onflict resolution, and streamline JWK parsing.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java (1)
184-194:⚠️ Potential issue | 🟡 MinorConsolidate suppress-path logging to a single entry.
In the default case, Line 188 logs
e.toString(), and Line 193 logsmessageagain for the same failure. This duplicates error logs.🔧 Proposed fix
String message = switch (e) { case JsonProcessingException ignored -> "Could not parse JWK keys retrieved from %s.".formatted(uri); case ResourceRetrievalException ignored -> "Could not retrieve JWK keys from %s.".formatted(uri); default -> { - if (suppressExceptions) log.error(e.toString()); yield e.getMessage(); } }; if (suppressExceptions) { - log.error(message); + log.error(message, e); return new JwkListByUri(uri, emptyList()); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 184 - 194, The code in Jwks that handles exception logging duplicates logs when suppressExceptions is true by calling log.error(e.toString()) in the switch default and then log.error(message) afterward; update the flow so only a single log call occurs: remove the log.error(e.toString()) from the switch default, build the message as before (including e.getMessage() or formatted URI messages), and then, if suppressExceptions is true, call log.error(...) once with a combined message that includes both the human-readable message and the exception details (e.g., e.toString() or the throwable) before returning new JwkListByUri(uri, emptyList()); ensure references: e, suppressExceptions, log, JwkListByUri, uri, and the switch block in class Jwks are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java`:
- Around line 291-294: The getLongDescription() method incorrectly uses jwksUris
!= null to detect URI mode but jwksUris is initialized to an empty list, so
inline JWK configs still hit the URI branch; update the branch condition in
getLongDescription() to check that jwksUris contains elements (e.g., jwksUris !=
null && !jwksUris.isEmpty() or simply !jwksUris.isEmpty()) so only non-empty URI
lists produce the "JWKs from ..." description; reference the
getLongDescription() method and the jwksUris field when making the change.
- Around line 102-106: In Jwks.setJwks, validate the incoming jwks parameter
before mutating object state: perform the null check (and any router-related
checks like the router != null condition) and throw ConfigurationException if
invalid, then assign this.jwks = jwks only after validation succeeds; update the
method to avoid assigning to this.jwks prior to the null check to prevent
leaving the object in an inconsistent state.
- Around line 116-120: The setter Jwks.setJwksUris currently calls
jwksUris.trim() unconditionally which throws NPE for null input; update
setJwksUris to guard against null by checking if jwksUris == null (or blank)
first and assign an empty list (e.g. List.of()) in that case, otherwise run the
existing
Arrays.stream(jwksUris.trim().split("\\s+")).map(StringUtils::strip).filter(StringUtils::isNotBlank).toList();
ensure the method still sets this.jwksUris and returns this.
---
Duplicate comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java`:
- Around line 184-194: The code in Jwks that handles exception logging
duplicates logs when suppressExceptions is true by calling
log.error(e.toString()) in the switch default and then log.error(message)
afterward; update the flow so only a single log call occurs: remove the
log.error(e.toString()) from the switch default, build the message as before
(including e.getMessage() or formatted URI messages), and then, if
suppressExceptions is true, call log.error(...) once with a combined message
that includes both the human-readable message and the exception details (e.g.,
e.toString() or the throwable) before returning new JwkListByUri(uri,
emptyList()); ensure references: e, suppressExceptions, log, JwkListByUri, uri,
and the switch block in class Jwks are updated accordingly.
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java
Outdated
Show resolved
Hide resolved
added 2 commits
March 3, 2026 15:58
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java (3)
188-198:⚠️ Potential issue | 🟡 MinorSuppressed exception path still logs the same failure twice.
When
suppressExceptionsis true and execution hits the default branch, it logs once in the switch and again in the suppress block.🔧 Proposed fix
String message = switch (e) { case JsonProcessingException ignored -> "Could not parse JWK keys retrieved from %s.".formatted(uri); case ResourceRetrievalException ignored -> "Could not retrieve JWK keys from %s.".formatted(uri); - default -> { - if (suppressExceptions) log.error(e.toString()); - yield e.getMessage(); - } + default -> e.getMessage() != null ? e.getMessage() : e.toString(); }; if (suppressExceptions) { - log.error(message); + log.error(message, e); return new JwkListByUri(uri, emptyList()); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 188 - 198, The suppress-exceptions path in Jwks currently logs the same error twice: once inside the switch default branch and again just after when suppressExceptions is true; update the switch expression in class Jwks so the default branch does not call log.error(e.toString()) — just set the message to e.getMessage() (or a formatted fallback) and let the existing suppressExceptions block perform the single log.error(message) and return new JwkListByUri(uri, emptyList()); this removes the duplicate logging while preserving the existing behavior for other switch cases (JsonProcessingException and ResourceRetrievalException) and the suppressExceptions conditional.
103-107:⚠️ Potential issue | 🔴 CriticalValidate
jwksunconditionally insetJwksto avoid pre-init NPE state.
setJwks(null)beforeinit()still setsthis.jwks = null, theninit()dereferencesjwks.isEmpty()(Line 84). This can crash startup.🔧 Proposed fix
`@MCChildElement` public Jwks setJwks(List<Jwk> jwks) { + if (jwks == null) + throw new ConfigurationException("JWKs list must not be null."); if (router != null) { // set in init, so we can't update prior to that call - if (jwks == null) throw new ConfigurationException("JWKs list must not be null."); this.keysByKid = buildKeyMap(jwks); } this.jwks = jwks; // unnecessary, mainly for consistency when debugging return this; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 103 - 107, setJwks currently allows jwks to be set to null before init(), which leads to a NPE in init() when it dereferences jwks; fix by validating jwks != null at the start of setJwks (throw ConfigurationException immediately if null), then proceed with the existing router check and call keysByKid = buildKeyMap(jwks) only after that validation; also remove or make conditional the trailing assignment this.jwks = jwks so you don't overwrite the field with null prior to init; refer to the setJwks method, the jwks field, keysByKid field, init method, and buildKeyMap function when making the change.
132-137:⚠️ Potential issue | 🟠 MajorDo not silently overwrite duplicate
kidvalues inbuildKeyMap.Current merge strategy (
second entry wins) hides configuration errors and can resolve the wrong verification key. Also reject null/blankkidexplicitly.🛡️ Proposed fix
private Map<String, RsaJsonWebKey> buildKeyMap(List<Jwk> jwks) { - var keyMap = jwks.stream() - .map(this::extractRsaJsonWebKey) - .collect(toMap(RsaJsonWebKey::getKeyId, identity(), - (k1, k2) -> k2 // second entry wins - )); + var keyMap = new HashMap<String, RsaJsonWebKey>(); + for (RsaJsonWebKey key : jwks.stream().map(this::extractRsaJsonWebKey).toList()) { + String keyId = key.getKeyId(); + if (StringUtils.isBlank(keyId)) + throw new ConfigurationException("Each JWK must provide a non-empty 'kid'."); + if (keyMap.putIfAbsent(keyId, key) != null) + throw new ConfigurationException("Duplicate JWK kid '%s'.".formatted(keyId)); + } if (keyMap.isEmpty()) throw new RuntimeException("No JWKs given or none resolvable - please specify at least one resolvable JWK"); return keyMap; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 132 - 137, In buildKeyMap, stop silently overwriting duplicate key IDs and reject null/blank kids: validate each RsaJsonWebKey returned by extractRsaJsonWebKey to ensure getKeyId() is non-null/non-blank and change the collector merge function to throw an IllegalStateException (or custom exception) when a duplicate kid is encountered (include both conflicting kid values in the message). This ensures buildKeyMap fails fast on invalid or duplicated JWKs instead of picking the second entry silently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java`:
- Around line 188-198: The suppress-exceptions path in Jwks currently logs the
same error twice: once inside the switch default branch and again just after
when suppressExceptions is true; update the switch expression in class Jwks so
the default branch does not call log.error(e.toString()) — just set the message
to e.getMessage() (or a formatted fallback) and let the existing
suppressExceptions block perform the single log.error(message) and return new
JwkListByUri(uri, emptyList()); this removes the duplicate logging while
preserving the existing behavior for other switch cases (JsonProcessingException
and ResourceRetrievalException) and the suppressExceptions conditional.
- Around line 103-107: setJwks currently allows jwks to be set to null before
init(), which leads to a NPE in init() when it dereferences jwks; fix by
validating jwks != null at the start of setJwks (throw ConfigurationException
immediately if null), then proceed with the existing router check and call
keysByKid = buildKeyMap(jwks) only after that validation; also remove or make
conditional the trailing assignment this.jwks = jwks so you don't overwrite the
field with null prior to init; refer to the setJwks method, the jwks field,
keysByKid field, init method, and buildKeyMap function when making the change.
- Around line 132-137: In buildKeyMap, stop silently overwriting duplicate key
IDs and reject null/blank kids: validate each RsaJsonWebKey returned by
extractRsaJsonWebKey to ensure getKeyId() is non-null/non-blank and change the
collector merge function to throw an IllegalStateException (or custom exception)
when a duplicate kid is encountered (include both conflicting kid values in the
message). This ensures buildKeyMap fails fast on invalid or duplicated JWKs
instead of picking the second entry silently.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java (2)
132-140:⚠️ Potential issue | 🟠 MajorReject duplicate or blank
kidvalues inbuildKeyMap.Line 136 currently makes “second entry wins”, which silently overwrites keys with the same
kid. That can make key resolution ambiguous and unsafe.Suggested fix
private Map<String, RsaJsonWebKey> buildKeyMap(List<Jwk> jwks) { var keyMap = jwks.stream() .map(this::extractRsaJsonWebKey) + .peek(k -> { + if (StringUtils.isBlank(k.getKeyId())) + throw new ConfigurationException("Each JWK must provide a non-empty 'kid'."); + }) .collect(toMap(RsaJsonWebKey::getKeyId, identity(), - (k1, k2) -> k2 // second entry wins + (k1, k2) -> { + throw new ConfigurationException("Duplicate JWK kid: " + k1.getKeyId()); + }, + HashMap::new )); if (keyMap.isEmpty()) throw new RuntimeException("No JWKs given or none resolvable - please specify at least one resolvable JWK"); return keyMap; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 132 - 140, In buildKeyMap(List<Jwk> jwks) validate that each RsaJsonWebKey returned by extractRsaJsonWebKey has a non-null, non-blank key id (RsaJsonWebKey::getKeyId) and reject duplicates instead of silently keeping the second entry: during collection, detect empty/blank keyIds and throw a RuntimeException, and change the merge behavior so that when two entries share the same kid you throw a descriptive exception (e.g., "Duplicate JWK kid: {kid}") rather than returning k2; ensure the exception is thrown before returning keyMap.
188-198:⚠️ Potential issue | 🟡 MinorAvoid duplicate error logs in suppressed exception path.
Line 192 and Line 197 log the same failure twice when
suppressExceptionsis true.Suggested fix
String message = switch (e) { case JsonProcessingException ignored -> "Could not parse JWK keys retrieved from %s.".formatted(uri); case ResourceRetrievalException ignored -> "Could not retrieve JWK keys from %s.".formatted(uri); - default -> { - if (suppressExceptions) log.error(e.toString()); - yield e.getMessage(); - } + default -> Optional.ofNullable(e.getMessage()).orElse(e.toString()); }; if (suppressExceptions) { - log.error(message); + log.error(message, e); return new JwkListByUri(uri, emptyList()); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 188 - 198, In Jwks where exceptions are handled (building the `message` and returning a `JwkListByUri`), avoid logging the same error twice: remove the inner conditional `log.error(e.toString())` inside the switch default branch and only call `log.error(message)` (or vice versa) so when `suppressExceptions` is true the error is logged exactly once; update the switch/default branch in the method to only set `message` (no logging) and let the existing `if (suppressExceptions) log.error(message);` perform the single log call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java`:
- Around line 132-140: In buildKeyMap(List<Jwk> jwks) validate that each
RsaJsonWebKey returned by extractRsaJsonWebKey has a non-null, non-blank key id
(RsaJsonWebKey::getKeyId) and reject duplicates instead of silently keeping the
second entry: during collection, detect empty/blank keyIds and throw a
RuntimeException, and change the merge behavior so that when two entries share
the same kid you throw a descriptive exception (e.g., "Duplicate JWK kid:
{kid}") rather than returning k2; ensure the exception is thrown before
returning keyMap.
- Around line 188-198: In Jwks where exceptions are handled (building the
`message` and returning a `JwkListByUri`), avoid logging the same error twice:
remove the inner conditional `log.error(e.toString())` inside the switch default
branch and only call `log.error(message)` (or vice versa) so when
`suppressExceptions` is true the error is logged exactly once; update the
switch/default branch in the method to only set `message` (no logging) and let
the existing `if (suppressExceptions) log.error(message);` perform the single
log call.
predic8
requested changes
Mar 3, 2026
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java
Outdated
Show resolved
Hide resolved
.../test/java/com/predic8/membrane/integration/withoutinternet/interceptor/JwksRefreshTest.java
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java (2)
185-195:⚠️ Potential issue | 🟡 MinorAvoid double logging in suppressed-exception branch.
For the default exception case, the code logs
e.toString()and then logsmessage, producing duplicate error records for the same failure.Suggested log consolidation
String message = switch (e) { case JsonProcessingException ignored -> "Could not parse JWK keys retrieved from %s.".formatted(uri); case ResourceRetrievalException ignored -> "Could not retrieve JWK keys from %s.".formatted(uri); - default -> { - if (suppressExceptions) log.error(e.toString()); - yield e.getMessage(); - } + default -> e.getMessage(); }; if (suppressExceptions) { - log.error(message); + log.error(message, e); return new JwkListByUri(uri, emptyList()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 185 - 195, In Jwks change the default-exception branch to avoid logging twice: stop calling log.error(e.toString()) and instead log a single message when suppressExceptions is true (preferably include the exception as payload via log.error(message, e) or append e.getMessage() into message); update the switch default branch so it only prepares the message (and does not log), and let the subsequent suppressExceptions block perform the single log and return the new JwkListByUri(uri, emptyList()).
130-135:⚠️ Potential issue | 🟠 MajorReject duplicate/blank
kidvalues instead of silently overwriting.The current merge strategy (
(k1, k2) -> k2) lets later keys silently replace earlier ones for the samekid. That makes key resolution ambiguous and can mask bad JWKS documents.Suggested validation-focused implementation
private Map<String, RsaJsonWebKey> buildKeyMap(List<Jwk> jwks) { - var keyMap = jwks.stream() - .map(this::extractRsaJsonWebKey) - .collect(toMap(RsaJsonWebKey::getKeyId, identity(), - (k1, k2) -> k2 // second entry wins - )); + var keyMap = new HashMap<String, RsaJsonWebKey>(); + for (RsaJsonWebKey key : jwks.stream().map(this::extractRsaJsonWebKey).toList()) { + String kid = key.getKeyId(); + if (kid == null || kid.isBlank()) { + throw new ConfigurationException("Each JWK must provide a non-empty 'kid'."); + } + if (keyMap.putIfAbsent(kid, key) != null) { + throw new ConfigurationException("Duplicate JWK kid '" + kid + "'."); + } + } if (keyMap.isEmpty()) throw new RuntimeException("No JWKs given or none resolvable - please specify at least one resolvable JWK"); return keyMap; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 130 - 135, The buildKeyMap method currently silently overwrites duplicate key IDs; change it to reject blank/null and duplicate kids by validating each RsaJsonWebKey from extractRsaJsonWebKey: if getKeyId() is null/blank throw an IllegalArgumentException (or a specific runtime exception used in this module) and when collecting into the map use a merge handler that throws on duplicate instead of (k1,k2)->k2 (e.g. (a,b)->{ throw new IllegalArgumentException("Duplicate kid: "+a.getKeyId()); }). Ensure the exception type matches existing error handling so callers see a clear failure when JWKS contains blank or duplicate kid values.core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/JwksRefreshTest.java (2)
126-130:⚠️ Potential issue | 🟡 MinorEnsure all resources are stopped/closed even if one teardown step fails.
If
jwksProvider.stop()throws,jwtValidator.stop()andhttpClient.close()are skipped. That can leak test resources and cause follow-up port conflicts.Suggested teardown hardening
`@AfterAll` public static void teardown() throws IOException { - jwksProvider.stop(); - jwtValidator.stop(); - httpClient.close(); + IOException first = null; + try { + if (jwksProvider != null) jwksProvider.stop(); + } catch (IOException e) { + first = e; + } finally { + try { + if (jwtValidator != null) jwtValidator.stop(); + } catch (IOException e) { + if (first == null) first = e; else first.addSuppressed(e); + } + try { + if (httpClient != null) httpClient.close(); + } catch (IOException e) { + if (first == null) first = e; else first.addSuppressed(e); + } + } + if (first != null) throw first; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/JwksRefreshTest.java` around lines 126 - 130, The AfterAll teardown (method teardown) currently calls jwksProvider.stop(), jwtValidator.stop(), and httpClient.close() sequentially so an exception from jwksProvider.stop() can skip the others; change teardown to attempt each resource shutdown independently by wrapping each call (jwksProvider.stop, jwtValidator.stop, httpClient.close) in its own try/catch, record the first thrown exception (or add others as suppressed) and after all attempts rethrow the recorded exception so failures are reported but all resources are still stopped/closed.
70-79:⚠️ Potential issue | 🟡 MinorReplace fixed sleeps with explicit readiness/wait conditions.
Thread.sleep(...)here makes the test timing-sensitive, and there is still no explicit readiness check afterjwtValidator.start()before the first call (Line 144). This can create intermittent CI failures.Suggested stabilization patch
@@ `@BeforeAll` public static void setup() throws Exception { @@ jwksProvider.start(); - - // Wait for jwksProvider to start - Thread.sleep(1000); + waitUntilReachable(PROVIDER_PORT, 5000); @@ jwtValidator.start(); + waitUntilReachable(AUTH_INTERCEPTOR_PORT, 5000); } @@ public void testRefresh() throws Exception { @@ currentJwkSet.set(new JsonWebKeySet(publicKey2)); - Thread.sleep(JWKS_REFRESH_INTERVAL * 1_000 * 2); // wait for refresh + waitForStatus(hc, authInterceptorUrl, createJwt(privateKey2), 200, JWKS_REFRESH_INTERVAL * 4_000L); @@ } + + private static void waitUntilReachable(int port, long timeoutMs) throws InterruptedException { + long deadline = System.currentTimeMillis() + timeoutMs; + while (System.currentTimeMillis() < deadline) { + try (java.net.Socket s = new java.net.Socket("localhost", port)) { + return; + } catch (IOException ignored) { + Thread.sleep(100); + } + } + throw new RuntimeException("Timeout waiting for localhost:" + port); + } + + private static void waitForStatus(HttpClient hc, String url, String jwt, int expectedStatus, long timeoutMs) throws Exception { + long deadline = System.currentTimeMillis() + timeoutMs; + while (System.currentTimeMillis() < deadline) { + Exchange exc = new Request.Builder() + .get(url) + .header("Authorization", "Bearer " + jwt) + .buildExchange(); + hc.call(exc); + if (exc.getResponse().getStatusCode() == expectedStatus) return; + Thread.sleep(100); + } + throw new RuntimeException("Timeout waiting for status " + expectedStatus); + }Also applies to: 149-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/JwksRefreshTest.java` around lines 70 - 79, The Thread.sleep(1000) in JwksRefreshTest makes the test flaky; replace the fixed sleep with an explicit readiness/wait: wait for the jwks provider and the jwtValidator to be ready instead of sleeping (e.g., poll a health/readiness endpoint or use a CountDownLatch/awaitStartup mechanism), then only create HttpClient and proceed; also add the same explicit wait after jwtValidator.start() before the first request to ensure jwtValidator.isStarted() or the test HTTP port is accepting connections. Use identifiers jwksProvider, Thread.sleep, jwtValidator, jwtValidator.start() and the test’s first request location to find where to add the polling/await logic.
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/cors/CorsUtil.java (1)
41-42: Consider renamingsplitBySpaceto match broader delimiter support.
StringList.parseToSet(...)handles commas and whitespace, so the current method name is now narrower than behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/cors/CorsUtil.java` around lines 41 - 42, The method name splitBySpace in CorsUtil is misleading because StringList.parseToSet accepts commas and whitespace; rename splitBySpace to a clearer name such as splitByWhitespaceOrComma (or splitByDelimiters/parseOrigins) and update its Javadoc to reflect comma+whitespace support, then update all call sites to the new name; to avoid breakage, add a deprecated splitBySpace wrapper that delegates to the new method (keeping the same signature) and mark it `@Deprecated` so callers can migrate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java`:
- Around line 185-195: In Jwks change the default-exception branch to avoid
logging twice: stop calling log.error(e.toString()) and instead log a single
message when suppressExceptions is true (preferably include the exception as
payload via log.error(message, e) or append e.getMessage() into message); update
the switch default branch so it only prepares the message (and does not log),
and let the subsequent suppressExceptions block perform the single log and
return the new JwkListByUri(uri, emptyList()).
- Around line 130-135: The buildKeyMap method currently silently overwrites
duplicate key IDs; change it to reject blank/null and duplicate kids by
validating each RsaJsonWebKey from extractRsaJsonWebKey: if getKeyId() is
null/blank throw an IllegalArgumentException (or a specific runtime exception
used in this module) and when collecting into the map use a merge handler that
throws on duplicate instead of (k1,k2)->k2 (e.g. (a,b)->{ throw new
IllegalArgumentException("Duplicate kid: "+a.getKeyId()); }). Ensure the
exception type matches existing error handling so callers see a clear failure
when JWKS contains blank or duplicate kid values.
In
`@core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/JwksRefreshTest.java`:
- Around line 126-130: The AfterAll teardown (method teardown) currently calls
jwksProvider.stop(), jwtValidator.stop(), and httpClient.close() sequentially so
an exception from jwksProvider.stop() can skip the others; change teardown to
attempt each resource shutdown independently by wrapping each call
(jwksProvider.stop, jwtValidator.stop, httpClient.close) in its own try/catch,
record the first thrown exception (or add others as suppressed) and after all
attempts rethrow the recorded exception so failures are reported but all
resources are still stopped/closed.
- Around line 70-79: The Thread.sleep(1000) in JwksRefreshTest makes the test
flaky; replace the fixed sleep with an explicit readiness/wait: wait for the
jwks provider and the jwtValidator to be ready instead of sleeping (e.g., poll a
health/readiness endpoint or use a CountDownLatch/awaitStartup mechanism), then
only create HttpClient and proceed; also add the same explicit wait after
jwtValidator.start() before the first request to ensure jwtValidator.isStarted()
or the test HTTP port is accepting connections. Use identifiers jwksProvider,
Thread.sleep, jwtValidator, jwtValidator.start() and the test’s first request
location to find where to add the polling/await logic.
---
Nitpick comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/cors/CorsUtil.java`:
- Around line 41-42: The method name splitBySpace in CorsUtil is misleading
because StringList.parseToSet accepts commas and whitespace; rename splitBySpace
to a clearer name such as splitByWhitespaceOrComma (or
splitByDelimiters/parseOrigins) and update its Javadoc to reflect
comma+whitespace support, then update all call sites to the new name; to avoid
breakage, add a deprecated splitBySpace wrapper that delegates to the new method
(keeping the same signature) and mark it `@Deprecated` so callers can migrate.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/com/predic8/membrane/core/interceptor/cors/CorsUtil.javacore/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.javacore/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/JwksRefreshTest.java
predic8
previously requested changes
Mar 4, 2026
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java (3)
117-122:⚠️ Potential issue | 🟠 MajorFix null handling in
setJwksUris(null branch is currently overwritten).When
jwksUris == null, Line 119 setsemptyList(), but Line 121 still executes and reprocesses the null value. This can reintroduce null-path failures depending onStringList.parseToListbehavior.Proposed fix
`@MCAttribute` public void setJwksUris(String jwksUris) { if (jwksUris == null) { this.jwksUris = emptyList(); + return; } this.jwksUris = StringList.parseToList(jwksUris); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 117 - 122, The null branch in setJwksUris currently sets this.jwksUris = emptyList() then continues to call StringList.parseToList(jwksUris), potentially reintroducing a null; change the method so that when jwksUris == null it assigns emptyList() and returns (or use an else) and only call StringList.parseToList(jwksUris) when jwksUris is non-null; update setJwksUris and keep references to the jwksUris field and StringList.parseToList to locate the fix.
128-133:⚠️ Potential issue | 🟠 MajorDo not silently overwrite duplicate
kidvalues inkeysByKid.The current merge function (
(k1, k2) -> k2) makes the “last key wins”, which hides configuration errors and can make key selection ambiguous. Reject duplicates (and blank/null kids) instead.Proposed fix
private Map<String, RsaJsonWebKey> buildKeyMap(List<Jwk> jwks) { - var keyMap = jwks.stream() - .map(this::extractRsaJsonWebKey) - .collect(toMap(RsaJsonWebKey::getKeyId, identity(), - (k1, k2) -> k2 // second entry wins - )); + var keyMap = jwks.stream() + .map(this::extractRsaJsonWebKey) + .peek(k -> { + String kid = k.getKeyId(); + if (kid == null || kid.isBlank()) { + throw new IllegalArgumentException("Each JWK must provide a non-empty 'kid'."); + } + }) + .collect(toMap( + RsaJsonWebKey::getKeyId, + identity(), + (k1, k2) -> { throw new IllegalArgumentException("Duplicate JWK kid: " + k1.getKeyId()); } + )); if (keyMap.isEmpty()) throw new RuntimeException("No JWKs given or none resolvable - please specify at least one resolvable JWK"); return keyMap; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 128 - 133, The key-building logic in buildKeyMap currently silently overwrites duplicate or blank key IDs by using a "last wins" merge; change it to validate and reject invalid keys instead: in buildKeyMap (and where extractRsaJsonWebKey produces a RsaJsonWebKey) ensure that the keyId (RsaJsonWebKey::getKeyId) is non-null/non-blank and when collecting to a map, replace the merge function with logic that throws an IllegalArgumentException (or a custom exception) on duplicate keyIds so duplicates are rejected rather than overwritten; include a clear error message mentioning the offending kid value and the source JWK to aid debugging.
186-193:⚠️ Potential issue | 🟡 MinorAvoid double logging for one suppressed exception path.
In the default branch,
log.error(e.toString())and the laterlog.error(message)produce duplicate entries for the same failure.Proposed fix
String message = switch (e) { case JsonProcessingException ignored -> "Could not parse JWK keys retrieved from %s.".formatted(uri); case ResourceRetrievalException ignored -> "Could not retrieve JWK keys from %s.".formatted(uri); default -> { - if (suppressExceptions) log.error(e.toString()); yield e.getMessage(); } }; if (suppressExceptions) { - log.error(message); + log.error(message, e); return new JwkListByUri(uri, emptyList()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 186 - 193, The code in Jwks (default branch handling exceptions) currently logs the same suppressed exception twice: once in the default -> block via log.error(e.toString()) and again later when suppressExceptions is true via log.error(message); remove the redundant log call in the default branch so that logging happens only once at the later central point that constructs and returns new JwkListByUri(uri, emptyList()); ensure you keep the unique variables (suppressExceptions, message, e, JwkListByUri, uri) intact and only eliminate the duplicate log.error(e.toString()) in the default branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java`:
- Around line 117-122: The null branch in setJwksUris currently sets
this.jwksUris = emptyList() then continues to call
StringList.parseToList(jwksUris), potentially reintroducing a null; change the
method so that when jwksUris == null it assigns emptyList() and returns (or use
an else) and only call StringList.parseToList(jwksUris) when jwksUris is
non-null; update setJwksUris and keep references to the jwksUris field and
StringList.parseToList to locate the fix.
- Around line 128-133: The key-building logic in buildKeyMap currently silently
overwrites duplicate or blank key IDs by using a "last wins" merge; change it to
validate and reject invalid keys instead: in buildKeyMap (and where
extractRsaJsonWebKey produces a RsaJsonWebKey) ensure that the keyId
(RsaJsonWebKey::getKeyId) is non-null/non-blank and when collecting to a map,
replace the merge function with logic that throws an IllegalArgumentException
(or a custom exception) on duplicate keyIds so duplicates are rejected rather
than overwritten; include a clear error message mentioning the offending kid
value and the source JWK to aid debugging.
- Around line 186-193: The code in Jwks (default branch handling exceptions)
currently logs the same suppressed exception twice: once in the default -> block
via log.error(e.toString()) and again later when suppressExceptions is true via
log.error(message); remove the redundant log call in the default branch so that
logging happens only once at the later central point that constructs and returns
new JwkListByUri(uri, emptyList()); ensure you keep the unique variables
(suppressExceptions, message, e, JwkListByUri, uri) intact and only eliminate
the duplicate log.error(e.toString()) in the default branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f2c6db95-60dd-4ce2-bc2a-619a2d810ec4
📒 Files selected for processing (1)
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java
rrayst
approved these changes
Mar 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Tests