Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/gradle.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ jobs:

steps:
- uses: actions/checkout@v4
- name: Set up JDK 11
- name: Set up JDK 17
uses: actions/setup-java@v4
with:
distribution: zulu
java-version: '11'
java-version: '17'
- uses: actions/cache@v4
with:
path: |
Expand Down
34 changes: 20 additions & 14 deletions build.gradle
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
plugins {
id 'org.springframework.boot' version '2.6.3'
id 'io.spring.dependency-management' version '1.0.11.RELEASE'
id 'org.springframework.boot' version '2.7.18'
id 'io.spring.dependency-management' version '1.1.6'
id 'java'
id "com.netflix.dgs.codegen" version "5.0.6"
id "com.diffplug.spotless" version "6.2.1"
id "com.netflix.dgs.codegen" version "5.6.9"
id "com.diffplug.spotless" version "6.25.0"
}

version = '0.0.1-SNAPSHOT'
sourceCompatibility = '11'
targetCompatibility = '11'
sourceCompatibility = '17'
targetCompatibility = '17'

spotless {
java {
Expand All @@ -24,6 +24,12 @@ repositories {
mavenCentral()
}

dependencyManagement {
imports {
mavenBom 'com.netflix.graphql.dgs:graphql-dgs-platform-dependencies:5.5.1'
}
}

configurations {
compileOnly {
extendsFrom annotationProcessor
Expand All @@ -35,14 +41,14 @@ dependencies {
implementation 'org.springframework.boot:spring-boot-starter-validation'
implementation 'org.springframework.boot:spring-boot-starter-hateoas'
implementation 'org.springframework.boot:spring-boot-starter-security'
implementation 'org.mybatis.spring.boot:mybatis-spring-boot-starter:2.2.2'
implementation 'com.netflix.graphql.dgs:graphql-dgs-spring-boot-starter:4.9.21'
implementation 'org.mybatis.spring.boot:mybatis-spring-boot-starter:2.3.2'
implementation 'com.netflix.graphql.dgs:graphql-dgs-spring-boot-starter'
implementation 'org.flywaydb:flyway-core'
implementation 'io.jsonwebtoken:jjwt-api:0.11.2'
runtimeOnly 'io.jsonwebtoken:jjwt-impl:0.11.2',
'io.jsonwebtoken:jjwt-jackson:0.11.2'
implementation 'joda-time:joda-time:2.10.13'
implementation 'org.xerial:sqlite-jdbc:3.36.0.3'
implementation 'io.jsonwebtoken:jjwt-api:0.12.6'
runtimeOnly 'io.jsonwebtoken:jjwt-impl:0.12.6',
'io.jsonwebtoken:jjwt-jackson:0.12.6'
implementation 'joda-time:joda-time:2.12.7'
implementation 'org.xerial:sqlite-jdbc:3.42.0.1'

compileOnly 'org.projectlombok:lombok'
annotationProcessor 'org.projectlombok:lombok'
Expand All @@ -53,7 +59,7 @@ dependencies {
testImplementation 'io.rest-assured:spring-mock-mvc:4.5.1'
testImplementation 'org.springframework.security:spring-security-test'
testImplementation 'org.springframework.boot:spring-boot-starter-test'
testImplementation 'org.mybatis.spring.boot:mybatis-spring-boot-starter-test:2.2.2'
testImplementation 'org.mybatis.spring.boot:mybatis-spring-boot-starter-test:2.3.2'
}

tasks.named('test') {
Expand Down
31 changes: 14 additions & 17 deletions src/main/java/io/spring/graphql/ArticleDatafetcher.java
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@
import com.netflix.graphql.dgs.DgsQuery;
import com.netflix.graphql.dgs.InputArgument;
import graphql.execution.DataFetcherResult;
import graphql.relay.DefaultConnectionCursor;
import graphql.relay.DefaultPageInfo;
import io.spring.graphql.types.PageInfo;
import graphql.schema.DataFetchingEnvironment;
import io.spring.api.exception.ResourceNotFoundException;
import io.spring.application.ArticleQueryService;
Expand Down Expand Up @@ -64,7 +63,7 @@ public DataFetcherResult<ArticlesConnection> getFeed(
current,
new CursorPageParameter<>(DateTimeCursor.parse(before), last, Direction.PREV));
}
graphql.relay.PageInfo pageInfo = buildArticlePageInfo(articles);
PageInfo pageInfo = buildArticlePageInfo(articles);
ArticlesConnection articlesConnection =
ArticlesConnection.newBuilder()
.pageInfo(pageInfo)
Expand Down Expand Up @@ -114,7 +113,7 @@ public DataFetcherResult<ArticlesConnection> userFeed(
target,
new CursorPageParameter<>(DateTimeCursor.parse(before), last, Direction.PREV));
}
graphql.relay.PageInfo pageInfo = buildArticlePageInfo(articles);
PageInfo pageInfo = buildArticlePageInfo(articles);
ArticlesConnection articlesConnection =
ArticlesConnection.newBuilder()
.pageInfo(pageInfo)
Expand Down Expand Up @@ -167,7 +166,7 @@ public DataFetcherResult<ArticlesConnection> userFavorites(
new CursorPageParameter<>(DateTimeCursor.parse(before), last, Direction.PREV),
current);
}
graphql.relay.PageInfo pageInfo = buildArticlePageInfo(articles);
PageInfo pageInfo = buildArticlePageInfo(articles);

ArticlesConnection articlesConnection =
ArticlesConnection.newBuilder()
Expand Down Expand Up @@ -221,7 +220,7 @@ public DataFetcherResult<ArticlesConnection> userArticles(
new CursorPageParameter<>(DateTimeCursor.parse(before), last, Direction.PREV),
current);
}
graphql.relay.PageInfo pageInfo = buildArticlePageInfo(articles);
PageInfo pageInfo = buildArticlePageInfo(articles);
ArticlesConnection articlesConnection =
ArticlesConnection.newBuilder()
.pageInfo(pageInfo)
Expand Down Expand Up @@ -276,7 +275,7 @@ public DataFetcherResult<ArticlesConnection> getArticles(
new CursorPageParameter<>(DateTimeCursor.parse(before), last, Direction.PREV),
current);
}
graphql.relay.PageInfo pageInfo = buildArticlePageInfo(articles);
PageInfo pageInfo = buildArticlePageInfo(articles);
ArticlesConnection articlesConnection =
ArticlesConnection.newBuilder()
.pageInfo(pageInfo)
Expand Down Expand Up @@ -356,16 +355,14 @@ public DataFetcherResult<Article> findArticleBySlug(@InputArgument("slug") Strin
.build();
}

private DefaultPageInfo buildArticlePageInfo(CursorPager<ArticleData> articles) {
return new DefaultPageInfo(
articles.getStartCursor() == null
? null
: new DefaultConnectionCursor(articles.getStartCursor().toString()),
articles.getEndCursor() == null
? null
: new DefaultConnectionCursor(articles.getEndCursor().toString()),
articles.hasPrevious(),
articles.hasNext());
private PageInfo buildArticlePageInfo(CursorPager<ArticleData> articles) {
return PageInfo.newBuilder()
.startCursor(
articles.getStartCursor() == null ? null : articles.getStartCursor().toString())
.endCursor(articles.getEndCursor() == null ? null : articles.getEndCursor().toString())
.hasPreviousPage(articles.hasPrevious())
.hasNextPage(articles.hasNext())
.build();
}

private Article buildArticleResult(ArticleData articleData) {
Expand Down
23 changes: 10 additions & 13 deletions src/main/java/io/spring/graphql/CommentDatafetcher.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@
import com.netflix.graphql.dgs.DgsDataFetchingEnvironment;
import com.netflix.graphql.dgs.InputArgument;
import graphql.execution.DataFetcherResult;
import graphql.relay.DefaultConnectionCursor;
import graphql.relay.DefaultPageInfo;
import io.spring.graphql.types.PageInfo;
import io.spring.application.CommentQueryService;
import io.spring.application.CursorPageParameter;
import io.spring.application.CursorPager;
Expand Down Expand Up @@ -78,7 +77,7 @@ public DataFetcherResult<CommentsConnection> articleComments(
current,
new CursorPageParameter<>(DateTimeCursor.parse(before), last, Direction.PREV));
}
graphql.relay.PageInfo pageInfo = buildCommentPageInfo(comments);
PageInfo pageInfo = buildCommentPageInfo(comments);
CommentsConnection result =
CommentsConnection.newBuilder()
.pageInfo(pageInfo)
Expand All @@ -99,16 +98,14 @@ public DataFetcherResult<CommentsConnection> articleComments(
.build();
}

private DefaultPageInfo buildCommentPageInfo(CursorPager<CommentData> comments) {
return new DefaultPageInfo(
comments.getStartCursor() == null
? null
: new DefaultConnectionCursor(comments.getStartCursor().toString()),
comments.getEndCursor() == null
? null
: new DefaultConnectionCursor(comments.getEndCursor().toString()),
comments.hasPrevious(),
comments.hasNext());
private PageInfo buildCommentPageInfo(CursorPager<CommentData> comments) {
return PageInfo.newBuilder()
.startCursor(
comments.getStartCursor() == null ? null : comments.getStartCursor().toString())
.endCursor(comments.getEndCursor() == null ? null : comments.getEndCursor().toString())
.hasPreviousPage(comments.hasPrevious())
.hasNextPage(comments.hasNext())
.build();
}

private Comment buildCommentResult(CommentData comment) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,36 +3,33 @@
import io.jsonwebtoken.Claims;
import io.jsonwebtoken.Jws;
import io.jsonwebtoken.Jwts;
import io.jsonwebtoken.SignatureAlgorithm;
import io.jsonwebtoken.security.Keys;
import io.spring.core.service.JwtService;
import io.spring.core.user.User;
import java.util.Date;
import java.util.Optional;
import javax.crypto.SecretKey;
import javax.crypto.spec.SecretKeySpec;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.stereotype.Component;

@Component
public class DefaultJwtService implements JwtService {
private final SecretKey signingKey;
private final SignatureAlgorithm signatureAlgorithm;
private int sessionTime;

@Autowired
public DefaultJwtService(
@Value("${jwt.secret}") String secret, @Value("${jwt.sessionTime}") int sessionTime) {
this.sessionTime = sessionTime;
signatureAlgorithm = SignatureAlgorithm.HS512;
this.signingKey = new SecretKeySpec(secret.getBytes(), signatureAlgorithm.getJcaName());
this.signingKey = Keys.hmacShaKeyFor(secret.getBytes());
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Keys.hmacShaKeyFor() silently changes signing algorithm for secrets shorter than 64 bytes

The migration from explicit SignatureAlgorithm.HS512 + SecretKeySpec to Keys.hmacShaKeyFor() introduces an implicit algorithm selection based on key byte length. Any deployment with a jwt.secret between 32-63 bytes will silently switch from HS512 to HS256/HS384, making all previously-issued JWT tokens unverifiable.

Detailed Explanation

The old code at DefaultJwtService.java:27-28 (before this PR) always used HS512:

signatureAlgorithm = SignatureAlgorithm.HS512;
this.signingKey = new SecretKeySpec(secret.getBytes(), signatureAlgorithm.getJcaName());

The new code at DefaultJwtService.java:25 uses Keys.hmacShaKeyFor() which auto-selects the algorithm:

  • 32-47 bytes → HS256
  • 48-63 bytes → HS384
  • ≥64 bytes → HS512

The default secret in application.properties is 86 bytes (safe, maps to HS512). However, a concrete example exists in the test at DefaultJwtServiceTest.java:16:

jwtService = new DefaultJwtService("123123123123123123123123123123123123123123123123123123123123", 3600);

This secret is 60 bytes, so the test now uses HS384 instead of HS512. The test still passes because should_get_null_with_expired_jwt fails for algorithm mismatch rather than expiration — masking the behavioral change.

Impact: Any production environment with a jwt.secret shorter than 64 bytes will have all existing user sessions invalidated after deployment, as tokens signed with HS512 cannot be verified with HS384/HS256. Users would need to re-authenticate.

Suggested change
this.signingKey = Keys.hmacShaKeyFor(secret.getBytes());
this.signingKey = Keys.hmacShaKeyFor(java.util.Arrays.copyOf(secret.getBytes(), 64));
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

}

@Override
public String toToken(User user) {
return Jwts.builder()
.setSubject(user.getId())
.setExpiration(expireTimeFromNow())
.subject(user.getId())
.expiration(expireTimeFromNow())
.signWith(signingKey)
.compact();
}
Expand All @@ -41,8 +38,8 @@ public String toToken(User user) {
public Optional<String> getSubFromToken(String token) {
try {
Jws<Claims> claimsJws =
Jwts.parserBuilder().setSigningKey(signingKey).build().parseClaimsJws(token);
return Optional.ofNullable(claimsJws.getBody().getSubject());
Jwts.parser().verifyWith(signingKey).build().parseSignedClaims(token);
return Optional.ofNullable(claimsJws.getPayload().getSubject());
} catch (Exception e) {
return Optional.empty();
}
Expand Down