[ES-2950][ES-2953] fixed htu and iat claim validation in dpop jwt#1669
[ES-2950][ES-2953] fixed htu and iat claim validation in dpop jwt#1669sacrana0 wants to merge 2 commits intomosip:developfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds HTU normalization/validation (strips query/fragment) and IAT validation with clock‑skew tolerance and configurable max age to DPoP verification; switches claim verifier to a parameterized DefaultJWTClaimsVerifier, centralizes InvalidDpopHeaderException handling, and adds unit tests and config property. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Filter as DpopValidationFilter
participant Verifier as DefaultJWTClaimsVerifier<SecurityContext>
participant TimeCheck as IAT/ClockSkewChecker
participant Chain as FilterChain/Controller
participant Error as InvalidDpopHandler
Client->>Filter: HTTP request with DPoP proof
Filter->>Verifier: Verify JWT claims (alg, typ, nonce, HTM/HTU presence)
Verifier-->>Filter: Claims map or throws
Filter->>Filter: Normalize request URI (strip query/fragment)
Filter->>Verifier: Compare HTU claim to normalized URI
Filter->>TimeCheck: Validate iat within clock‑skew and max‑age
alt all checks pass
Filter->>Chain: forward request
Chain-->>Client: 200 / next handler response
else any check fails
Filter->>Error: Log & throw InvalidDpopHeaderException
Error-->>Client: 400 with WWW-Authenticate: INVALID_DPOP_PROOF
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
esignet-service/src/main/java/io/mosip/esignet/advice/DpopValidationFilter.java (2)
275-288: HTU validation logic looks correct.The stripping of query parameters and fragments before comparison is correct per RFC 9449. The null check at line 277 is defensive since
DefaultJWTClaimsVerifieralready validates required claims, but it provides clearer error logging.One note: If
htuClaimcontains an invalid URI, theURISyntaxExceptionwill be caught by the genericExceptionhandler below, which logs "Invalid request URI" — that's reasonable but consider catchingURISyntaxExceptionseparately for a more specific error message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@esignet-service/src/main/java/io/mosip/esignet/advice/DpopValidationFilter.java` around lines 275 - 288, In DpopValidationFilter, when parsing htuClaim into a URI (the UriComponentsBuilder.fromUri(new URI(htuClaim)) call and subsequent builder.replaceQuery/fragment), add a dedicated catch for URISyntaxException so invalid htu URIs are logged with a clear, specific message (e.g., "Invalid htu claim URI") and then throw InvalidDpopHeaderException; keep the existing generic Exception handler for other cases, but ensure the URISyntaxException branch references htuClaim and the DpopValidationFilter/htuClaim parsing logic so the error is easy to trace.
290-299: Consider removing redundant IAT validation.
DefaultJWTClaimsVerifier.verify()(line 273) already validates thatiatis present (it's inREQUIRED_CLAIMS) and not in the future beyondmaxClockSkew. This manual check duplicates that logic.If the intent is to provide more specific error logging, the current approach is acceptable. Otherwise, this block could be removed to reduce redundancy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@esignet-service/src/main/java/io/mosip/esignet/advice/DpopValidationFilter.java` around lines 290 - 299, The iat validation block duplicates DefaultJWTClaimsVerifier.verify() behavior; remove the manual checks that call claims.getIssueTime(), the null check/log ("Missing iat claim"), and the future check against Instant.now().plusSeconds(maxClockSkewSeconds) along with the associated throws of InvalidDpopHeaderException so the code relies on DefaultJWTClaimsVerifier.verify() (invoked earlier) for presence and clock-skew validation; if you want to keep a log-only record instead, replace the throw statements with only log.debug/info calls mentioning iat and maxClockSkewSeconds so no duplicate validation occurs.esignet-service/src/test/java/io/mosip/esignet/advice/DpopValidationFilterTest.java (1)
339-371: Consider adding a boundary test case.The test validates that iat 20 seconds in the future fails with 10 seconds clock skew. Consider adding a test where iat is exactly at the boundary (e.g., iat = now + 10s with maxClockSkew = 10) to verify the boundary condition behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@esignet-service/src/test/java/io/mosip/esignet/advice/DpopValidationFilterTest.java` around lines 339 - 371, Add a new test method in DpopValidationFilterTest that creates a DPoP JWT with iat exactly set to now plus the maxClockSkewSeconds (e.g., iat = now + 10s if maxClockSkewSeconds is 10). This will test the boundary condition to ensure the filter either accepts or rejects the token correctly at the clock skew limit, complementing the existing testDpopHeader_withIatInFuture_thenFail method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@esignet-service/src/main/java/io/mosip/esignet/advice/DpopValidationFilter.java`:
- Around line 301-304: The catch-all in DpopValidationFilter currently logs
every error as "Invalid request URI" and throws InvalidDpopHeaderException;
modify the exception handling in the block around
DefaultJWTClaimsVerifier.verify() and the URI parsing so you catch and handle
specific exceptions (e.g., BadJWTException from
DefaultJWTClaimsVerifier.verify(), JOSEException,
ParseException/URISyntaxException from URI parsing, and any other known types)
rather than Exception; for each specific catch, log a clear, distinct message
including e.getMessage() and context (e.g., "DPoP claim verification failed" for
BadJWTException, "Invalid request URI" for URI parsing), and then throw
InvalidDpopHeaderException (or wrap the original where appropriate) so callers
get correct diagnostics while preserving existing error semantics in
DpopValidationFilter.
In
`@esignet-service/src/test/java/io/mosip/esignet/advice/DpopValidationFilterTest.java`:
- Around line 358-359: Update the misleading test comment: change the line
commenting "Set clock skew to 0 so the future iat is rejected" to accurately
reflect that maxClockSkewSeconds is being set to 10 (so an iat 20s in the future
exceeds the 10s tolerance). Locate the ReflectionTestUtils.setField call that
sets "maxClockSkewSeconds" on the filter instance and adjust the comment to
mention 10 seconds tolerance (or remove the incorrect part) so it matches the
test logic.
---
Nitpick comments:
In
`@esignet-service/src/main/java/io/mosip/esignet/advice/DpopValidationFilter.java`:
- Around line 275-288: In DpopValidationFilter, when parsing htuClaim into a URI
(the UriComponentsBuilder.fromUri(new URI(htuClaim)) call and subsequent
builder.replaceQuery/fragment), add a dedicated catch for URISyntaxException so
invalid htu URIs are logged with a clear, specific message (e.g., "Invalid htu
claim URI") and then throw InvalidDpopHeaderException; keep the existing generic
Exception handler for other cases, but ensure the URISyntaxException branch
references htuClaim and the DpopValidationFilter/htuClaim parsing logic so the
error is easy to trace.
- Around line 290-299: The iat validation block duplicates
DefaultJWTClaimsVerifier.verify() behavior; remove the manual checks that call
claims.getIssueTime(), the null check/log ("Missing iat claim"), and the future
check against Instant.now().plusSeconds(maxClockSkewSeconds) along with the
associated throws of InvalidDpopHeaderException so the code relies on
DefaultJWTClaimsVerifier.verify() (invoked earlier) for presence and clock-skew
validation; if you want to keep a log-only record instead, replace the throw
statements with only log.debug/info calls mentioning iat and maxClockSkewSeconds
so no duplicate validation occurs.
In
`@esignet-service/src/test/java/io/mosip/esignet/advice/DpopValidationFilterTest.java`:
- Around line 339-371: Add a new test method in DpopValidationFilterTest that
creates a DPoP JWT with iat exactly set to now plus the maxClockSkewSeconds
(e.g., iat = now + 10s if maxClockSkewSeconds is 10). This will test the
boundary condition to ensure the filter either accepts or rejects the token
correctly at the clock skew limit, complementing the existing
testDpopHeader_withIatInFuture_thenFail method.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b69942c2-12fe-4015-a0b9-7654fec17821
📒 Files selected for processing (2)
esignet-service/src/main/java/io/mosip/esignet/advice/DpopValidationFilter.javaesignet-service/src/test/java/io/mosip/esignet/advice/DpopValidationFilterTest.java
esignet-service/src/main/java/io/mosip/esignet/advice/DpopValidationFilter.java
Show resolved
Hide resolved
esignet-service/src/test/java/io/mosip/esignet/advice/DpopValidationFilterTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
esignet-service/src/main/java/io/mosip/esignet/advice/DpopValidationFilter.java (1)
290-299: Consider adding a past-bound check for theiatclaim.The current validation rejects tokens issued in the future (with clock skew tolerance), but accepts tokens with arbitrarily old
iatvalues. DPoP proofs are intended to be freshly generated per request. Without a past-bound check, a captured DPoP JWT could remain valid indefinitely (limited only by thejtireplay check cache TTL).Consider adding a maximum age constraint:
🛡️ Optional enhancement for freshness validation
if (iat.toInstant().isAfter(Instant.now().plusSeconds(maxClockSkewSeconds))) { log.error("iat claim is in the future"); throw new InvalidDpopHeaderException(); } + // Optional: reject tokens older than a reasonable threshold (e.g., 5 minutes) + int maxAgeSeconds = 300; // could be configurable + if (iat.toInstant().isBefore(Instant.now().minusSeconds(maxAgeSeconds + maxClockSkewSeconds))) { + log.error("iat claim is too old"); + throw new InvalidDpopHeaderException(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@esignet-service/src/main/java/io/mosip/esignet/advice/DpopValidationFilter.java` around lines 290 - 299, Add a past-bound validation for the DPoP `iat` claim in DpopValidationFilter: after obtaining claims.getIssueTime() and the existing future check that uses maxClockSkewSeconds, also enforce a maximum age (e.g., maxIatAgeSeconds from config) by verifying iat.toInstant().isBefore(Instant.now().minusSeconds(maxIatAgeSeconds)); if so, log an error (e.g., "iat claim is too old") and throw InvalidDpopHeaderException. Ensure you introduce/read a configurable maxIatAgeSeconds value and use the same InvalidDpopHeaderException path as the other iat checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@esignet-service/src/main/java/io/mosip/esignet/advice/DpopValidationFilter.java`:
- Around line 290-299: Add a past-bound validation for the DPoP `iat` claim in
DpopValidationFilter: after obtaining claims.getIssueTime() and the existing
future check that uses maxClockSkewSeconds, also enforce a maximum age (e.g.,
maxIatAgeSeconds from config) by verifying
iat.toInstant().isBefore(Instant.now().minusSeconds(maxIatAgeSeconds)); if so,
log an error (e.g., "iat claim is too old") and throw
InvalidDpopHeaderException. Ensure you introduce/read a configurable
maxIatAgeSeconds value and use the same InvalidDpopHeaderException path as the
other iat checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d1e53530-7224-47d4-9952-acfe80ddbba5
📒 Files selected for processing (2)
esignet-service/src/main/java/io/mosip/esignet/advice/DpopValidationFilter.javaesignet-service/src/test/java/io/mosip/esignet/advice/DpopValidationFilterTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- esignet-service/src/test/java/io/mosip/esignet/advice/DpopValidationFilterTest.java
@coderabbitai Can you point me to some documentation or rfc as I didn't see anything about iat validation being too old? |
|
Tip For best results, initiate chat on the files or code changes.
What RFC 9449 says about Section 4.3 of RFC 9449 requires that "the creation time of the JWT, as determined by either the The RFC's preferred mechanism for proof lifetime: RFC 9449 notes that "servers MAY limit DPoP proof lifetimes by using server-provided nonce values containing the time at the server rather than comparing the client-supplied Why a past-bound RFC 9449 Section 11.1 explicitly calls out that "an attacker in control of the client can pre-generate DPoP proofs for specific endpoints arbitrarily far into the future." It notes a legitimate user may pre-generate DPoP proofs and copy them to another machine, and "when DPoP proofs can be pre-generated and exfiltrated, all that is actually being proved in DPoP protocol interactions is possession of a DPoP proof — not of the proof-of-possession key." Practitioner guides also echo this: the Summary: The RFC does not explicitly mandate a maximum past age for You're free to skip it if your |
esignet-service/src/main/java/io/mosip/esignet/advice/DpopValidationFilter.java
Outdated
Show resolved
Hide resolved
esignet-service/src/main/java/io/mosip/esignet/advice/DpopValidationFilter.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
esignet-service/src/main/resources/application-default.properties (1)
25-28: Comment inaccuracy: clock-skew applies to multiple claims.The comment at line 25 states the clock-skew validates 'nbf' and 'exp' claims, but
DefaultJWTClaimsVerifier.setMaxClockSkew()applies tolerance toiat,nbf, andexpclaims. Additionally, the filter now usesmaxClockSkewSecondsfor the custom future-iat check. Consider updating the comment to reflect all affected claims.Also, consider documenting the relationship between
clock-skew(future tolerance) andiat.max-age-seconds(past tolerance) for clarity.📝 Suggested comment update
-# DPoP jwt clock skew in seconds to validate 'nbf' and 'exp' claim +# DPoP jwt clock skew in seconds for iat/nbf/exp claim tolerance (future allowance) mosip.esignet.dpop.clock-skew=10 mosip.esignet.dpop.nonce.expire.seconds=15 +# Maximum age in seconds for DPoP iat claim (past allowance) mosip.esignet.dpop.iat.max-age-seconds=60🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@esignet-service/src/main/resources/application-default.properties` around lines 25 - 28, Update the properties comment to correctly state that mosip.esignet.dpop.clock-skew applies to the iat, nbf and exp claims (as enforced by DefaultJWTClaimsVerifier.setMaxClockSkew()) and note that this is used as the future-iat tolerance (maxClockSkewSeconds) in the custom filter, while mosip.esignet.dpop.iat.max-age-seconds controls past-age tolerance for iat; change the single-line comment above the properties to reflect these three affected claims and the relationship between clock-skew (future tolerance) and iat.max-age-seconds (past tolerance).esignet-service/src/test/java/io/mosip/esignet/advice/DpopValidationFilterTest.java (1)
340-358: Minor: Comment inaccuracy on iat offset.The comment at line 342 states "iat set 10 seconds in the future" but line 353 actually sets
Instant.now().plusSeconds(20). Consider updating the comment to reflect the actual 20-second offset.📝 Proposed fix
- // Build a DPoP JWT with iat set 10 seconds in the future + // Build a DPoP JWT with iat set 20 seconds in the future (beyond 10s clock skew tolerance)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@esignet-service/src/test/java/io/mosip/esignet/advice/DpopValidationFilterTest.java` around lines 340 - 358, Update the inaccurate inline comment in the test method testDpopHeader_withIatOutsideTimeLimit_thenFail: the comment currently says "iat set 10 seconds in the future" but the code sets issueTime to Instant.now().plusSeconds(20); change the comment to reflect the 20-second offset (or adjust plusSeconds to 10 if you intended 10s) so the comment and the SignedJWT creation in the test are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@esignet-service/src/main/java/io/mosip/esignet/advice/DpopValidationFilter.java`:
- Around line 294-303: The iat past-bound check in DpopValidationFilter (using
claims.getIssueTime()) omits the clock-skew tolerance; update the past
comparison to subtract both maxDPOPIatAgeSeconds and maxClockSkewSeconds from
Instant.now() (i.e., use Instant.now().minusSeconds(maxDPOPIatAgeSeconds +
maxClockSkewSeconds) or equivalent) so the iat is allowed a skew window just
like the future-bound check that uses plusSeconds(maxClockSkewSeconds).
In
`@esignet-service/src/test/java/io/mosip/esignet/advice/DpopValidationFilterTest.java`:
- Around line 372-383: The test reuses the same MockHttpServletRequest and
MockHttpServletResponse which causes multiple DPoP headers and leftover response
state; for the second scenario in DpopValidationFilterTest (the block that
builds/signs the expired iat JWT and calls filter.doFilterInternal), create and
use a fresh request and response (or explicitly remove existing DPoP headers and
reset response) before adding the new DPoP header so that
DpopValidationFilter.getDpopHeader() sees only one header and the response
status starts from a clean state; keep using the same filter and filterChain
mocks but verify interactions (e.g., verify(filterChain, never()).doFilter(...))
against the fresh request/response.
---
Nitpick comments:
In `@esignet-service/src/main/resources/application-default.properties`:
- Around line 25-28: Update the properties comment to correctly state that
mosip.esignet.dpop.clock-skew applies to the iat, nbf and exp claims (as
enforced by DefaultJWTClaimsVerifier.setMaxClockSkew()) and note that this is
used as the future-iat tolerance (maxClockSkewSeconds) in the custom filter,
while mosip.esignet.dpop.iat.max-age-seconds controls past-age tolerance for
iat; change the single-line comment above the properties to reflect these three
affected claims and the relationship between clock-skew (future tolerance) and
iat.max-age-seconds (past tolerance).
In
`@esignet-service/src/test/java/io/mosip/esignet/advice/DpopValidationFilterTest.java`:
- Around line 340-358: Update the inaccurate inline comment in the test method
testDpopHeader_withIatOutsideTimeLimit_thenFail: the comment currently says "iat
set 10 seconds in the future" but the code sets issueTime to
Instant.now().plusSeconds(20); change the comment to reflect the 20-second
offset (or adjust plusSeconds to 10 if you intended 10s) so the comment and the
SignedJWT creation in the test are consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 957a457e-104a-40bf-b67e-ea165de010a9
📒 Files selected for processing (3)
esignet-service/src/main/java/io/mosip/esignet/advice/DpopValidationFilter.javaesignet-service/src/main/resources/application-default.propertiesesignet-service/src/test/java/io/mosip/esignet/advice/DpopValidationFilterTest.java
esignet-service/src/main/java/io/mosip/esignet/advice/DpopValidationFilter.java
Show resolved
Hide resolved
esignet-service/src/test/java/io/mosip/esignet/advice/DpopValidationFilterTest.java
Show resolved
Hide resolved
Signed-off-by: Sachin Rana <sacrana324@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
esignet-service/src/main/java/io/mosip/esignet/advice/DpopValidationFilter.java (1)
305-308: Catch block still re-catchesInvalidDpopHeaderExceptioncausing redundant logging.The log message improvement addresses part of the past review feedback, but the catch block still intercepts
InvalidDpopHeaderExceptionthrown from within the try block (lines 284, 292, 299, 303). This results in double logging—first the specific message (e.g., "Missing htu claim"), then "DPOP jwt claim validation failed".♻️ Proposed fix to rethrow InvalidDpopHeaderException without re-logging
- } catch (Exception e) { - log.error("DPOP jwt claim validation failed", e); + } catch (InvalidDpopHeaderException e) { + throw e; + } catch (Exception e) { + log.error("DPoP JWT claim validation failed", e); throw new InvalidDpopHeaderException(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@esignet-service/src/main/java/io/mosip/esignet/advice/DpopValidationFilter.java` around lines 305 - 308, The catch-all in DpopValidationFilter currently re-catches InvalidDpopHeaderException and logs it again; update the exception handling so InvalidDpopHeaderException is caught and immediately rethrown without logging (e.g., add an explicit catch(InvalidDpopHeaderException) { throw; }) and keep the generic catch(Exception e) to log and wrap other unexpected exceptions thrown during methods like validateDpopClaims/validateHtu/validateHtm referenced in the class; this ensures the specific validation exceptions (InvalidDpopHeaderException) are not double-logged while still handling other errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@esignet-service/src/main/java/io/mosip/esignet/advice/DpopValidationFilter.java`:
- Around line 305-308: The catch-all in DpopValidationFilter currently
re-catches InvalidDpopHeaderException and logs it again; update the exception
handling so InvalidDpopHeaderException is caught and immediately rethrown
without logging (e.g., add an explicit catch(InvalidDpopHeaderException) {
throw; }) and keep the generic catch(Exception e) to log and wrap other
unexpected exceptions thrown during methods like
validateDpopClaims/validateHtu/validateHtm referenced in the class; this ensures
the specific validation exceptions (InvalidDpopHeaderException) are not
double-logged while still handling other errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 72eeb2cc-b935-493a-8205-8b23630827ea
📒 Files selected for processing (3)
esignet-service/src/main/java/io/mosip/esignet/advice/DpopValidationFilter.javaesignet-service/src/main/resources/application-default.propertiesesignet-service/src/test/java/io/mosip/esignet/advice/DpopValidationFilterTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- esignet-service/src/test/java/io/mosip/esignet/advice/DpopValidationFilterTest.java
Signed-off-by: Sachin Rana <sacrana324@gmail.com>
3 participants
Summary by CodeRabbit
New Features
Behavior Changes
Tests
Configuration