Conversation
added 4 commits
February 16, 2026 16:54
Contributor
📝 WalkthroughWalkthroughCentralizes JWKS handling in Jwks: supports static JWK lists or JWKS URIs with scheduled refresh, exposes keysByKid and init(Router), adds per-JWK HTTP client config and demo-key warnings; JwtAuthInterceptor delegates key lookup to Jwks; AuthorizationService gains jwksRefreshInterval; tests for JWKS refresh added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as JWT Client
participant Validator as JWT Validator (JwtAuthInterceptor)
participant Jwks as JWKS Manager (Jwks)
participant Provider as JWKS Provider
Note over Validator,Jwks: Initialization
Validator->>Jwks: init(Router)
Jwks->>Provider: fetch initial JWKS (if configured)
Provider-->>Jwks: JWKS response
Jwks->>Jwks: build keysByKid / schedule periodic refresh
Note over Client,Validator: Request 1 (Key A)
Client->>Validator: Send JWT (signed with Key A)
Validator->>Jwks: getKeyByKid(kid)
Jwks-->>Validator: return Key A
Validator->>Validator: validate signature
Validator-->>Client: 200 OK
rect rgba(100,150,200,0.5)
Note over Jwks,Provider: Background refresh
Jwks->>Provider: fetch updated JWKS
Provider-->>Jwks: new JWKS (Key B)
Jwks->>Jwks: rebuild keysByKid
end
Note over Client,Validator: Request 2 (Key B)
Client->>Validator: Send JWT (signed with Key B)
Validator->>Jwks: getKeyByKid(kid)
Jwks-->>Validator: return Key B
Validator->>Validator: validate signature
Validator-->>Client: 200 OK
Note over Client,Validator: Request 3 (old Key A)
Client->>Validator: Send JWT (signed with Key A)
Validator->>Jwks: getKeyByKid(kid)
Jwks-->>Validator: key not found
Validator->>Validator: validation fails
Validator-->>Client: 400 Bad Request
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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)
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/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.
core/src/test/java/com/predic8/membrane/core/interceptor/jwt/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.
Comment on lines
+67
to
+81
| Thread.sleep(1000); | ||
|
|
||
| jwtValidator = new DefaultRouter(); | ||
| jwtValidator.add(proxyWithInterceptors( | ||
| AUTH_INTERCEPTOR_PORT, | ||
| jwtAuthInterceptor(), | ||
| new AbstractInterceptor() { | ||
| @Override | ||
| public Outcome handleRequest(Exchange exc) { | ||
| exc.setResponse(Response.ok().build()); | ||
| return Outcome.RETURN; | ||
| } | ||
| }) | ||
| ); | ||
| jwtValidator.start(); |
Contributor
There was a problem hiding this comment.
Startup readiness relies on fixed-delay sleeps — asymmetric and fragile.
Thread.sleep(1000) guards the JWKS provider, but no equivalent guard exists after jwtValidator.start() (Line 81). If DefaultRouter.start() returns before the HTTP listener is fully bound, the first hc.call(exc1) in testRefresh() will fail with a connection-refused error rather than a meaningful assertion failure. Using a polling/retry loop (or Awaitility) for both servers makes the boundary explicit and eliminates the race on loaded CI machines.
🤖 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 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.
| @Override public String getRevocationEndpoint() { return null; } | ||
| }; | ||
| authService.setJwksRefreshInterval(jwksRefreshInterval); | ||
| authService.setHttpClient(new HttpClient()); |
Contributor
There was a problem hiding this comment.
HttpClient created in buildAuthorizationService is never closed.
HttpClient is AutoCloseable (used with try-with-resources on Line 139). The instance created here has no owner that closes it — teardown() stops the routers but ignores this client, leaving its underlying connection pool open until GC.
🛠️ Proposed fix
Assign the client to a static field and close it in teardown():
+ 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();
+ if (authServiceHttpClient != null) authServiceHttpClient.close();
}🤖 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`
at 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.
core/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwksRefreshTest.java
Show resolved
Hide resolved
predic8
approved these changes
Feb 23, 2026
predic8
requested changes
Feb 23, 2026
| import com.predic8.membrane.core.transport.http.client.HttpClientConfiguration; | ||
| import com.predic8.membrane.core.util.ConfigurationException; | ||
| import com.predic8.membrane.core.util.text.TextUtil; | ||
| import org.jetbrains.annotations.NotNull; |
Member
There was a problem hiding this comment.
Use Resolver instead of HTTPClient.
Resolver uses proxy configuration
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
2 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
Bug Fixes
Tests