refactor(security): replace JWT library with Nimbus JOSE and improve token validation#911
Conversation
|
Caution Review failedPull request was closed or merged during review 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:
WalkthroughReplaces JJWT and Spring OAuth2 client with Nimbus JOSE JWT; reimplements token creation, parsing, signature verification, and claim validation in JwtUtils; adds startup secret validation via Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant JwtUtils
participant SecretRepo as SecretRepository
participant SignedJWT
participant ClaimsVerifier as DefaultJWTClaimsVerifier
Client->>JwtUtils: Present JWT
JwtUtils->>SecretRepo: Use stored signing secret (initialized at startup)
JwtUtils->>SignedJWT: Parse token and verify signature with secret
SignedJWT-->>JwtUtils: signature valid / invalid
JwtUtils->>ClaimsVerifier: Validate issuer, exp, iat, sub and custom claims (userId numeric)
ClaimsVerifier-->>JwtUtils: claims valid / invalid
JwtUtils-->>Client: Return success or ApiError.JWT_INVALID
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
backend/src/main/java/org/booklore/config/security/JwtUtils.java (2)
157-164: Double logging on extract failures.This catch logs at
warnand rethrows; the callers inWebSocketAuthInterceptorandQueryParameterJwtFilter(per the provided snippets) already log on failure (atdebuganderrorrespectively). The same failure will therefore be logged twice at different levels, which is noisy and inconsistent. Either drop the catch here and letextractClaimshandle logging, or just log without rethrowing if extraction failure must be observable here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/booklore/config/security/JwtUtils.java` around lines 157 - 164, The extractUsername method currently catches exceptions, logs a warning, and rethrows, causing duplicate logs because callers (WebSocketAuthInterceptor, QueryParameterJwtFilter) already log failures; remove the try/catch in JwtUtils.extractUsername so it simply returns extractClaims(token).getSubject() and lets extractClaims or the callers handle logging and error propagation, keeping logging centralized and avoiding double logging.
96-103: Narrowthrows Exceptionto the concrete Nimbus exceptions.
SignedJWT.parsethrowsParseExceptionandMACVerifier/signedJWT.verifythrowJOSEException. Declaringthrows Exceptionforces all callers (includingvalidateToken/extractClaims) into broadcatch (Exception …)blocks and obscures real bugs (e.g., NPEs in surrounding code) by lumping them in with token errors.Also, throwing a bare
RuntimeExceptionfor the "Invalid token signature" case is hard to discriminate downstream — consider a small typed exception (e.g.,BadJWTExceptionfrom Nimbus or a project-localInvalidTokenException).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/booklore/config/security/JwtUtils.java` around lines 96 - 103, The parseAndVerify method currently declares throws Exception and throws a bare RuntimeException on signature failure; change its signature to throw the concrete Nimbus exceptions (java.text.ParseException and com.nimbusds.jose.JOSEException) and replace the RuntimeException("Invalid token signature") with a specific, typed exception (either reuse com.nimbusds.jwt.proc.BadJWTException or introduce a project-specific InvalidTokenException) so callers like validateToken and extractClaims can catch/token-handle explicitly; update those callers to catch ParseException, JOSEException, and the typed invalid-token exception rather than a broad Exception to avoid masking other errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/main/java/org/booklore/config/security/JwtUtils.java`:
- Around line 131-135: The issuer check in JwtUtils currently lets tokens
without an iss claim pass; update the validation (the block using
claims.getIssuer() in JwtUtils, which complements generateToken()) to require
the issuer to be present and exactly "booklore" by changing the condition to
throw a RuntimeException whenever issuer is null or not equal to "booklore"
(i.e., replace the current issuer != null && !"booklore".equals(issuer) guard
with a strict presence-and-equality check and an explanatory error message).
- Around line 35-53: validateSecret() currently swallows IllegalStateException
from getSecretBytes() and duplicates the length check; change it so it simply
calls getSecretBytes() (remove the inline < 32 check) and only catch database
connectivity exceptions (e.g., org.springframework.dao.DataAccessException
and/or org.hibernate.exception.JDBCConnectionException) so that
IllegalStateException from getSecretBytes() propagates and breaks startup on a
too-short secret; leave jwtSecretService.getSecret() usage in getSecretBytes()
unchanged.
---
Nitpick comments:
In `@backend/src/main/java/org/booklore/config/security/JwtUtils.java`:
- Around line 157-164: The extractUsername method currently catches exceptions,
logs a warning, and rethrows, causing duplicate logs because callers
(WebSocketAuthInterceptor, QueryParameterJwtFilter) already log failures; remove
the try/catch in JwtUtils.extractUsername so it simply returns
extractClaims(token).getSubject() and lets extractClaims or the callers handle
logging and error propagation, keeping logging centralized and avoiding double
logging.
- Around line 96-103: The parseAndVerify method currently declares throws
Exception and throws a bare RuntimeException on signature failure; change its
signature to throw the concrete Nimbus exceptions (java.text.ParseException and
com.nimbusds.jose.JOSEException) and replace the RuntimeException("Invalid token
signature") with a specific, typed exception (either reuse
com.nimbusds.jwt.proc.BadJWTException or introduce a project-specific
InvalidTokenException) so callers like validateToken and extractClaims can
catch/token-handle explicitly; update those callers to catch ParseException,
JOSEException, and the typed invalid-token exception rather than a broad
Exception to avoid masking other errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: bc01e289-c95e-448b-be98-fc86fc77ae77
📒 Files selected for processing (2)
backend/build.gradle.ktsbackend/src/main/java/org/booklore/config/security/JwtUtils.java
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Packaging Smoke Test
🧰 Additional context used
📓 Path-based instructions (1)
backend/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
backend/src/**/*.java: Use 4-space indentation and match surrounding Java style in backend code
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection in backend code
Use MapStruct for entity/DTO mapping in backend code
Keep JPA entities on the *Entity suffix in backend code
Files:
backend/src/main/java/org/booklore/config/security/JwtUtils.java
🧠 Learnings (4)
📚 Learning: 2026-03-31T06:22:25.311Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 113
File: booklore-api/build.gradle.kts:89-90
Timestamp: 2026-03-31T06:22:25.311Z
Learning: When reviewing build logic or CI checks that call the JitPack build status API, do not treat a response like {"status":"none"} as evidence that a dependency is unavailable or that a build has failed. JitPack typically builds packages on-demand the first time they’re requested (e.g., via Gradle); "none" means the package isn’t pre-built/cached yet. Only raise a build failure concern when the status explicitly indicates an error/failure (e.g., failed/cancelled), or when dependent resolution actually fails.
Applied to files:
backend/build.gradle.kts
📚 Learning: 2026-04-02T09:12:48.158Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 334
File: booklore-api/build.gradle.kts:0-0
Timestamp: 2026-04-02T09:12:48.158Z
Learning: In this repo, nightcompress (groupId `com.github.gotson.nightcompress`, artifact `nightcompress`) is the preferred library for archive/unarchive operations (including RAR and other formats). During code review, flag changes to Gradle dependency declarations that remove nightcompress or replace it with an alternative for unarchiving (e.g., junrar, commons-compress, JNI-based libraries, or standard Java `ZipInputStream`/`ZipFile`) unless there’s a justified exception. Exceptions are acceptable only when nightcompress cannot handle a specific archive format; in that case, require the PR to clearly document the format limitation, the alternative being used, and get maintainer confirmation of the trade-off.
Applied to files:
backend/build.gradle.kts
📚 Learning: 2026-04-10T08:15:37.436Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 449
File: booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java:139-145
Timestamp: 2026-04-10T08:15:37.436Z
Learning: When using Spring `ContentDisposition.builder(...).filename(name, StandardCharsets.UTF_8).build()` (i.e., explicitly providing UTF-8), the resulting header value should include both the quoted `filename="=?UTF-8?..."` and the RFC 5987 `filename*=` parameters. In this case, any extra ASCII fallback computation (e.g., deriving an ASCII `fallbackFilename` via `NON_ASCII_PATTERN` and calling `.filename(fallbackFilename)`) is likely redundant—prefer calling only `.filename(fallbackName?, StandardCharsets.UTF_8)` as appropriate and let Spring handle the UTF-8 header parameters. Verify by comparing the emitted header for `filename` and `filename*` before deciding to keep an ASCII fallback.
Applied to files:
backend/src/main/java/org/booklore/config/security/JwtUtils.java
📚 Learning: 2026-04-14T12:43:08.698Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 502
File: booklore-api/src/main/java/org/booklore/service/reader/ChapterCacheService.java:0-0
Timestamp: 2026-04-14T12:43:08.698Z
Learning: For this codebase (booklore-api), target Java 25 with `--enable-preview`, so `_` is intentionally used as an unnamed/ignored variable (e.g., lambda parameter or pattern variable) per Java’s preview feature JEP 456. Do not flag `_` in those contexts as an invalid/reserved identifier; only flag it if it’s used in a non-supported position (e.g., where an unnamed variable is not applicable for the Java preview rules).
Applied to files:
backend/src/main/java/org/booklore/config/security/JwtUtils.java
🔇 Additional comments (1)
backend/build.gradle.kts (1)
103-103: Dependency change approved; Nimbus version is valid and OAuth2 client removal is clean.
com.nimbusds:nimbus-jose-jwt:10.9is a valid published version (Nimbus uses 2-segment versioning). Verification confirms no lingering Spring OAuth2 client imports (org.springframework.security.oauth2.client.*,OidcUser,OAuth2AuthorizedClient, etc.) remain inbackend/src, so the removal ofspring-boot-starter-oauth2-clientis complete and safe.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/src/main/java/org/booklore/config/security/JwtUtils.java (1)
133-141: Avoid collapsing all validation failures intofalse.Catching all exceptions here makes operational failures (e.g., secret retrieval issues) indistinguishable from normal invalid-token cases. Consider returning
falseonly for known token-invalid errors and logging infra/internal failures at higher severity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/booklore/config/security/JwtUtils.java` around lines 133 - 141, The validateToken method currently catches all Exception types and returns false, hiding operational errors; change validateToken to only swallow known token-invalid exceptions (e.g., JWTVerificationException, ParseException, JOSEException, or your custom InvalidTokenException) and return false for those, while letting other exceptions (such as secret retrieval or infrastructure failures from parseAndVerify or validateClaims) bubble up or be logged at error level; update the catch blocks around parseAndVerify and validateClaims in validateToken to handle specific exceptions and add a final catch for RuntimeException/Exception that logs with log.error and rethrows (or wraps) so internal failures are not conflated with invalid-token results.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/main/java/org/booklore/config/security/JwtUtils.java`:
- Around line 48-51: validateToken() and extractUserId() disagree on required
JWT claims: add "userId" to the claims contract so tokens accepted by
validateToken() won't later fail in extractUserId(). Update the
DefaultJWTClaimsVerifier construction (the Set.of(...) that currently lists
"exp","iat","iss","sub") to include "userId", and make the same change for the
other verifier instances referenced around the file (the other
DefaultJWTClaimsVerifier usages at the later blocks). Ensure JWT_ISSUER and
JWTClaimsSet.Builder usage remains the same but the required claims set includes
"userId" so both validateToken() and extractUserId() enforce the same contract.
---
Nitpick comments:
In `@backend/src/main/java/org/booklore/config/security/JwtUtils.java`:
- Around line 133-141: The validateToken method currently catches all Exception
types and returns false, hiding operational errors; change validateToken to only
swallow known token-invalid exceptions (e.g., JWTVerificationException,
ParseException, JOSEException, or your custom InvalidTokenException) and return
false for those, while letting other exceptions (such as secret retrieval or
infrastructure failures from parseAndVerify or validateClaims) bubble up or be
logged at error level; update the catch blocks around parseAndVerify and
validateClaims in validateToken to handle specific exceptions and add a final
catch for RuntimeException/Exception that logs with log.error and rethrows (or
wraps) so internal failures are not conflated with invalid-token results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 7c2a1776-6695-4b80-aa43-dc86a9b4ee99
📒 Files selected for processing (2)
backend/src/main/java/org/booklore/config/security/JwtUtils.javabackend/src/main/java/org/booklore/exception/ApiError.java
✅ Files skipped from review due to trivial changes (1)
- backend/src/main/java/org/booklore/exception/ApiError.java
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (1)
backend/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
backend/src/**/*.java: Use 4-space indentation and match surrounding Java style in backend code
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection in backend code
Use MapStruct for entity/DTO mapping in backend code
Keep JPA entities on the *Entity suffix in backend code
Files:
backend/src/main/java/org/booklore/config/security/JwtUtils.java
🧠 Learnings (3)
📚 Learning: 2026-03-25T19:09:09.638Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 189
File: booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java:113-116
Timestamp: 2026-03-25T19:09:09.638Z
Learning: In `booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java`, when deciding whether to forward the `Authorization` header to the upstream Kobo server proxy on the `/v1/library/sync` endpoint, a simple null check (`request.getHeader(HttpHeaders.AUTHORIZATION) != null`) is the correct guard. The header is either absent or present — if present it may or may not be a valid token, but validity is determined by the upstream Kobo server, not by Grimmory. Blank/whitespace intermediate states do not occur in practice, and pre-validating the token value before forwarding is not the responsibility of this service.
Applied to files:
backend/src/main/java/org/booklore/config/security/JwtUtils.java
📚 Learning: 2026-04-10T08:15:37.436Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 449
File: booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java:139-145
Timestamp: 2026-04-10T08:15:37.436Z
Learning: When using Spring `ContentDisposition.builder(...).filename(name, StandardCharsets.UTF_8).build()` (i.e., explicitly providing UTF-8), the resulting header value should include both the quoted `filename="=?UTF-8?..."` and the RFC 5987 `filename*=` parameters. In this case, any extra ASCII fallback computation (e.g., deriving an ASCII `fallbackFilename` via `NON_ASCII_PATTERN` and calling `.filename(fallbackFilename)`) is likely redundant—prefer calling only `.filename(fallbackName?, StandardCharsets.UTF_8)` as appropriate and let Spring handle the UTF-8 header parameters. Verify by comparing the emitted header for `filename` and `filename*` before deciding to keep an ASCII fallback.
Applied to files:
backend/src/main/java/org/booklore/config/security/JwtUtils.java
📚 Learning: 2026-04-14T12:43:08.698Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 502
File: booklore-api/src/main/java/org/booklore/service/reader/ChapterCacheService.java:0-0
Timestamp: 2026-04-14T12:43:08.698Z
Learning: For this codebase (booklore-api), target Java 25 with `--enable-preview`, so `_` is intentionally used as an unnamed/ignored variable (e.g., lambda parameter or pattern variable) per Java’s preview feature JEP 456. Do not flag `_` in those contexts as an invalid/reserved identifier; only flag it if it’s used in a non-supported position (e.g., where an unnamed variable is not applicable for the Java preview rules).
Applied to files:
backend/src/main/java/org/booklore/config/security/JwtUtils.java
🔀 Multi-repo context grimmory-tools/grimmory-docs
Linked repositories findings
grimmory-tools/grimmory-docs
-
OIDC settings reference documents that the backend validates ID tokens by checking signature against provider JWKS and verifying issuer, audience, expiration, nonce, etc. This is directly relevant to tightened verification behavior in JwtUtils. [::grimmory-tools/grimmory-docs::docs/authentication/oidc-settings.md:456-458]
-
The OIDC docs define the "Issuer URI" behavior and note Grimmory appends /.well-known/openid-configuration for discovery; any change in issuer validation semantics may require doc updates or operator guidance. [::grimmory-tools/grimmory-docs::docs/authentication/oidc-settings.md:45,57]
-
Provider-specific guides (Authentik, Pocket ID) state Grimmory typically uses public clients with PKCE (no client secret) and show examples where Client Secret is left empty. The PR's stricter secret length checks (minimum bytes for HS256) could affect configurations only when confidential clients / symmetric HS256 are used; these guides highlight common setups that are unaffected but may need clarification for confidential-client setups. [::grimmory-tools/grimmory-docs::docs/authentication/authentik.md: Client Secret guidance; ::grimmory-tools/grimmory-docs::docs/authentication/pocket-id.md: Client Secret guidance]
-
The docs describe Test Connection checks including that "Authorization, token, and JWKS endpoints" are present and "Signing keys fetchable". Since JwtUtils migration affects local HS256 secret handling and validation flow, reviewers should ensure documentation distinguishes when Grimmory expects provider-signed tokens (asymmetric JWKS/RS*) vs locally signed HS256 tokens and whether operators must supply secrets of a minimum length for confidential-client scenarios. [::grimmory-tools/grimmory-docs::docs/authentication/oidc-settings.md:97; ::grimmory-tools/grimmory-docs::docs/authentication/oidc-settings.md: Test Connection section]
Summary assessment: grimmory-docs contains OIDC guidance that is relevant to the PR's behavioral changes (issuer/JWKS validation and secret handling). Recommend verifying docs mention/clarify:
- When HS256 (shared secret) is used vs provider JWKS (RS*/EC*), and secret length requirements for HS256.
- Any changes to issuer strictness (trailing slash handling) given earlier guidance differences (e.g., Authelia note). [::grimmory-tools/grimmory-docs::docs/authentication/authelia.md:69]
🔇 Additional comments (1)
backend/src/main/java/org/booklore/config/security/JwtUtils.java (1)
54-63: Nice fix on startup secret validation behavior.
validateSecret()now correctly fails fast on misconfiguration and only tolerates startup DB unavailability. This aligns well with the PR’s security objective.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/src/main/java/org/booklore/config/security/JwtUtils.java (1)
193-198: Remove duplicateuserIdtype validation inextractUserId().
extractClaims()already callsvalidateClaims()(Line 165), andvalidateClaims()already enforces numericuserId(Lines 151-153). Rechecking it here duplicates the claim contract and can drift later.Refactor option
public Long extractUserId(String token) { Object userIdClaim = extractClaims(token).getClaim("userId"); - if (userIdClaim instanceof Number) { - return ((Number) userIdClaim).longValue(); - } - throw ApiError.JWT_INVALID.createException("Invalid userId claim type"); + return ((Number) userIdClaim).longValue(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/java/org/booklore/config/security/JwtUtils.java` around lines 193 - 198, The extractUserId() method duplicates userId type validation already performed by validateClaims() called inside extractClaims(); remove the redundant instanceof Number check and related exception throw in extractUserId(), and instead directly retrieve and return the userId value (casting/transforming as appropriate) from the claim returned by extractClaims(); keep references to extractUserId(), extractClaims(), and validateClaims() so the code relies on the single source of truth for userId validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/main/java/org/booklore/config/security/JwtUtils.java`:
- Around line 97-100: The catch-all in generateToken() is hiding
IllegalStateException thrown by getSecretBytes(); update generateToken() so
IllegalStateException from getSecretBytes() is allowed to propagate (or is
rethrown) and only other exceptions are logged/wrapped: e.g., add a specific
catch for IllegalStateException that rethrows it, then keep a catch(Exception e)
for all other errors where you log error and throw new RuntimeException("Could
not generate token", e). Reference generateToken() and getSecretBytes() to
locate the change.
---
Nitpick comments:
In `@backend/src/main/java/org/booklore/config/security/JwtUtils.java`:
- Around line 193-198: The extractUserId() method duplicates userId type
validation already performed by validateClaims() called inside extractClaims();
remove the redundant instanceof Number check and related exception throw in
extractUserId(), and instead directly retrieve and return the userId value
(casting/transforming as appropriate) from the claim returned by
extractClaims(); keep references to extractUserId(), extractClaims(), and
validateClaims() so the code relies on the single source of truth for userId
validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 6d9dc46b-639d-48c3-ba1a-d94da63e2d39
📒 Files selected for processing (1)
backend/src/main/java/org/booklore/config/security/JwtUtils.java
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test Suite / Frontend Tests
- GitHub Check: Test Suite / Backend Tests
- GitHub Check: Analyze (java-kotlin)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Frontend Lint Threshold Check
🧰 Additional context used
📓 Path-based instructions (1)
backend/src/**/*.java
📄 CodeRabbit inference engine (AGENTS.md)
backend/src/**/*.java: Use 4-space indentation and match surrounding Java style in backend code
Prefer constructor injection via Lombok patterns already used in the codebase. Do not introduce@Autowiredfield injection in backend code
Use MapStruct for entity/DTO mapping in backend code
Keep JPA entities on the *Entity suffix in backend code
Files:
backend/src/main/java/org/booklore/config/security/JwtUtils.java
🧠 Learnings (3)
📚 Learning: 2026-03-25T19:09:09.638Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 189
File: booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java:113-116
Timestamp: 2026-03-25T19:09:09.638Z
Learning: In `booklore-api/src/main/java/org/booklore/service/kobo/KoboLibrarySyncService.java`, when deciding whether to forward the `Authorization` header to the upstream Kobo server proxy on the `/v1/library/sync` endpoint, a simple null check (`request.getHeader(HttpHeaders.AUTHORIZATION) != null`) is the correct guard. The header is either absent or present — if present it may or may not be a valid token, but validity is determined by the upstream Kobo server, not by Grimmory. Blank/whitespace intermediate states do not occur in practice, and pre-validating the token value before forwarding is not the responsibility of this service.
Applied to files:
backend/src/main/java/org/booklore/config/security/JwtUtils.java
📚 Learning: 2026-04-10T08:15:37.436Z
Learnt from: imnotjames
Repo: grimmory-tools/grimmory PR: 449
File: booklore-api/src/main/java/org/booklore/service/book/BookDownloadService.java:139-145
Timestamp: 2026-04-10T08:15:37.436Z
Learning: When using Spring `ContentDisposition.builder(...).filename(name, StandardCharsets.UTF_8).build()` (i.e., explicitly providing UTF-8), the resulting header value should include both the quoted `filename="=?UTF-8?..."` and the RFC 5987 `filename*=` parameters. In this case, any extra ASCII fallback computation (e.g., deriving an ASCII `fallbackFilename` via `NON_ASCII_PATTERN` and calling `.filename(fallbackFilename)`) is likely redundant—prefer calling only `.filename(fallbackName?, StandardCharsets.UTF_8)` as appropriate and let Spring handle the UTF-8 header parameters. Verify by comparing the emitted header for `filename` and `filename*` before deciding to keep an ASCII fallback.
Applied to files:
backend/src/main/java/org/booklore/config/security/JwtUtils.java
📚 Learning: 2026-04-14T12:43:08.698Z
Learnt from: balazs-szucs
Repo: grimmory-tools/grimmory PR: 502
File: booklore-api/src/main/java/org/booklore/service/reader/ChapterCacheService.java:0-0
Timestamp: 2026-04-14T12:43:08.698Z
Learning: For this codebase (booklore-api), target Java 25 with `--enable-preview`, so `_` is intentionally used as an unnamed/ignored variable (e.g., lambda parameter or pattern variable) per Java’s preview feature JEP 456. Do not flag `_` in those contexts as an invalid/reserved identifier; only flag it if it’s used in a non-supported position (e.g., where an unnamed variable is not applicable for the Java preview rules).
Applied to files:
backend/src/main/java/org/booklore/config/security/JwtUtils.java
🔀 Multi-repo context grimmory-tools/grimmory-docs
Findings for grimmory-tools/grimmory-docs
-
OIDC reference (docs/authentication/oidc-settings.md) documents backend behavior: discovery via /.well-known/openid-configuration, Test Connection checks include JWKS and "Signing keys fetchable", and explicit step saying "Backend validates the ID token: Checks the signature against the provider's public keys (JWKS), verifies the issuer, audience, expiration, nonce, and other security claims." This directly intersects the PR's tightened Nimbus-based verification. [::grimmory-tools/grimmory-docs::docs/authentication/oidc-settings.md:5,44,456-458]
-
The same file explicitly documents Client Secret usage and that public clients using PKCE (no secret) are the common case; confidential clients (with secrets) are supported. The docs include examples showing empty client secret and show populated secrets in examples — relevant because the PR now enforces a minimum secret length for HS256 symmetric secrets. [::grimmory-tools/grimmory-docs::docs/authentication/oidc-settings.md:44-52,345,413]
-
Provider-specific guides:
- Authentik guide (docs/authentication/authentik.md) marks Grimmory as a public client by default (PKCE/no client secret) and shows examples where the Issuer URL may include a trailing slash; docs also instruct including trailing slash for Authentik. This matters because JwtUtils now enforces stricter issuer validation and Authelia has different trailing-slash behavior. [::grimmory-tools/grimmory-docs::docs/authentication/authentik.md:42,131]
- Pocket ID guide (docs/authentication/pocket-id.md) similarly documents public client/PKCE usage and leaving client secret empty. [::grimmory-tools/grimmory-docs::docs/authentication/pocket-id.md:27,71]
- Authelia guide (docs/authentication/authelia.md) explicitly warns Authelia's issuer does NOT use a trailing slash; it instructs users to omit trailing slash — relevant to stricter issuer checks. [::grimmory-tools/grimmory-docs::docs/authentication/authelia.md:65-69]
Assessment / impact for the PR
- Documentation already emphasizes provider-signed tokens (JWKS) and public-client PKCE flows (no secret). The PR's stricter Nimbus verification and minimum secret length primarily affect confidential-client setups where Grimmory or clients use symmetric HS256 secrets. Reviewers should confirm docs clarify:
- When Grimmory expects provider-signed (asymmetric) tokens vs locally-signed HS256 tokens.
- The new minimum secret length requirement and guidance for operators using confidential clients / symmetric secrets.
- Any changes to issuer normalization/strictness (trailing slash handling) given provider-specific guidance in the docs.
[::grimmory-tools/grimmory-docs::]
🔇 Additional comments (1)
backend/src/main/java/org/booklore/config/security/JwtUtils.java (1)
45-52: Good initialization hardening and claim-contract alignment.Nice improvement: startup secret validation plus a required
userIdclaim in the verifier keepsvalidateToken()and claim extraction behavior consistent.
imnotjames
left a comment
There was a problem hiding this comment.
I have tested this thoroughly and see no regressions.
I also confirmed that it's not possible to use the Algorithm.NONE algorithm in this case, so that's good. It fails at multiple different levels, so no leaks there.
Co-authored-by: James Ward <james@notjam.es>
…token validation (grimmory-tools#911) Co-authored-by: James Ward <james@notjam.es>
Description
Linked Issue: Fixes #
Changes
This pull request migrates JWT handling in the backend from the
jjwtlibrary to thenimbus-jose-jwtlibrary. The migration includes updating the build dependencies, refactoring theJwtUtilsclass to use Nimbus APIs, and improving security and validation around JWT secret handling and claim extraction.Dependency changes:
jjwtdependencies withcom.nimbusds:nimbus-jose-jwt:10.9inbackend/build.gradle.ktsand removed the OAuth2 client starter, as it's no longer required for JWT handling.JWT utility refactor:
JwtUtils.javato use Nimbus JOSE JWT APIs for token generation, validation, and claim extraction, replacing all usages of the previousio.jsonwebtoken(jjwt) library.Security improvements:
Summary by CodeRabbit