Conversation
github-actions
bot
requested review from
ZaMan-O and
hongjm0912
and removed request for
Copilot
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 OAuth 시스템에 ThirdPartyScope 기능을 도입하고, JWT 서명 방식을 RSA로 전환하여 외부 서비스와의 연동을 강화합니다. Application 도메인을 통해 외부 서비스가 자체적인 권한 범위를 정의하고 관리할 수 있게 하며, JWT에 이 ThirdPartyScope 정보를 포함시켜 유연한 권한 관리를 지원합니다. 또한, 외부에서 JWT 유효성을 검증할 수 있도록 JWK Set API를 제공합니다. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements ThirdPartyScope functionality, including RSA-based JWT signing and the addition of Application and ThirdPartyScope domains, laying the groundwork for external service integration. However, a security analysis identified two potential Denial of Service (DoS) vulnerabilities related to unbounded data retrieval: one in the application search API due to unrestricted pagination, and another from an unbounded findAll() call when querying available OAuth scopes. These issues could lead to resource exhaustion. Additionally, the review identified general improvements related to performance (e.g., N+1 problems). It is recommended to enforce maximum page sizes and implement more efficient querying for scope lists to mitigate these risks.
...tagsm/common/domain/application/repository/custom/impl/ApplicationJpaCustomRepositoryImpl.kt
Outdated
Show resolved
Hide resolved
...eam/themoment/datagsm/web/domain/client/service/impl/QueryAvailableOauthScopesServiceImpl.kt
Outdated
Show resolved
Hide resolved
| @field:Min(1) | ||
| @param:Schema(description = "페이지 크기", example = "100", defaultValue = "100") | ||
| val size: Int = 100, |
There was a problem hiding this comment.
The size parameter for pagination has a @Min(1) constraint but lacks a @Max constraint. An attacker can provide an extremely large value for the size parameter (e.g., size=1000000), which is passed directly to the database query's limit clause. This can lead to excessive memory consumption and CPU usage on both the database and application servers, potentially causing a Denial of Service (DoS).
| @field:Min(1) | |
| @param:Schema(description = "페이지 크기", example = "100", defaultValue = "100") | |
| val size: Int = 100, | |
| @field:Max(100) | |
| @param:Schema(description = "페이지 크기", example = "100", defaultValue = "100") | |
| val size: Int = 100, |
There was a problem hiding this comment.
Code Review
이 PR은 ThirdPartyScope 기능을 구현하고, JWT 서명 방식을 RSA로 변경하는 등 여러 변경 사항을 포함합니다. 새로운 Application 및 ThirdPartyScope 도메인에 대한 CRUD API와 관련 로직이 체계적으로 잘 구현되었으며, OAuthScope 리팩토링, JWKS 엔드포인트 추가, 상세한 테스트 코드 작성 등 긍정적인 변경 사항이 많습니다. 하지만, 스코프 해결 중 N+1 쿼리를 수행하여 서비스 성능 저하 또는 충돌을 유발할 수 있는 잠재적인 서비스 거부(DoS) 취약점이 확인되었습니다. 또한, ApplicationJpaCustomRepositoryImpl에서 thirdPartyScopes에 대한 N+1 문제가 발생할 수 있으므로, default_batch_fetch_size에 의존하기보다는 ID 기반 페이지네이션과 IN 절을 사용하는 2단계 쿼리 패턴을 적용하여 이 문제를 해결하는 것을 권장합니다.
| override fun searchApplicationWithPaging( | ||
| name: String?, | ||
| id: String?, | ||
| pageable: Pageable, | ||
| ): Page<ApplicationJpaEntity> { | ||
| val content = | ||
| jpaQueryFactory | ||
| .selectFrom(applicationJpaEntity) | ||
| .leftJoin(applicationJpaEntity.account) | ||
| .fetchJoin() | ||
| .where( | ||
| name?.let { applicationJpaEntity.name.startsWith(it) }, | ||
| id?.let { applicationJpaEntity.id.eq(it) }, | ||
| ).offset(pageable.offset) | ||
| .limit(pageable.pageSize.toLong()) | ||
| .fetch() | ||
|
|
||
| val countQuery = | ||
| jpaQueryFactory | ||
| .select(applicationJpaEntity.count()) | ||
| .from(applicationJpaEntity) | ||
| .where( | ||
| name?.let { applicationJpaEntity.name.startsWith(it) }, | ||
| id?.let { applicationJpaEntity.id.eq(it) }, | ||
| ) | ||
|
|
||
| return PageableExecutionUtils.getPage(content, pageable) { countQuery.fetchOne() ?: 0L } | ||
| } |
There was a problem hiding this comment.
현재 구현은 searchApplicationWithPaging 메서드에서 ApplicationJpaEntity를 조회할 때 thirdPartyScopes 컬렉션을 함께 조회하지 않아, 이후 toResDto 변환 과정에서 N+1 쿼리 문제가 발생할 수 있습니다. default_batch_fetch_size 설정으로 문제가 완화될 수는 있지만, 프로젝트의 일반 규칙에서는 이 방식보다 'ID 페이지네이션 -> IN 절 fetchJoin' 2단계 쿼리 패턴 사용을 권장하고 있습니다.
N+1 문제를 근본적으로 해결하고 명시적인 데이터 로딩을 위해, 먼저 페이지에 해당하는 Application ID 목록을 조회한 뒤, 해당 ID들을 사용하여 IN 절과 fetchJoin으로 thirdPartyScopes를 포함한 전체 Application 엔티티를 조회하는 방식으로 수정하는 것을 제안합니다. 이렇게 하면 페이지네이션과 컬렉션 fetch join을 함께 사용할 때 발생할 수 있는 문제를 피하고 성능을 개선할 수 있습니다.
override fun searchApplicationWithPaging(
name: String?,
id: String?,
pageable: Pageable,
): Page<ApplicationJpaEntity> {
val conditions = listOfNotNull(
name?.let { applicationJpaEntity.name.startsWith(it) },
id?.let { applicationJpaEntity.id.eq(it) },
)
val ids =
jpaQueryFactory
.select(applicationJpaEntity.id)
.from(applicationJpaEntity)
.where(*conditions.toTypedArray())
.offset(pageable.offset)
.limit(pageable.pageSize.toLong())
.fetch()
if (ids.isEmpty()) {
return Page.empty(pageable)
}
val content =
jpaQueryFactory
.selectFrom(applicationJpaEntity)
.leftJoin(applicationJpaEntity.account).fetchJoin()
.leftJoin(applicationJpaEntity.thirdPartyScopes).fetchJoin()
.where(applicationJpaEntity.id.`in`(ids))
.fetch()
.distinct()
.sortedBy { ids.indexOf(it.id) }
val countQuery =
jpaQueryFactory
.select(applicationJpaEntity.count())
.from(applicationJpaEntity)
.where(*conditions.toTypedArray())
return PageableExecutionUtils.getPage(content, pageable) { countQuery.fetchOne() ?: 0L }
}References
- To resolve N+1 problems with pagination, use the 'ID pagination -> IN clause fetchJoin 2-query pattern' instead of relying on
default_batch_fetch_sizeas a long-term solution.
| .map { scopeStr -> | ||
| OAuthScope.fromString(scopeStr) | ||
| ?: resolveThirdPartyScope(scopeStr) | ||
| }.toSet() |
There was a problem hiding this comment.
The calculateGrantedScopes method iterates over requested scopes and calls resolveThirdPartyScope for each one that is not a built-in scope. resolveThirdPartyScope performs a database query. An attacker could create a client with a large number of third-party scopes and then request a token with all of them, triggering many database queries in a single request (N+1 problem), which can lead to database connection pool exhaustion and service degradation. It is recommended to optimize the scope resolution by querying all third-party scopes in a single database call using an IN clause.
| if (application.account != currentAccount && !isAdmin) { | ||
| throw ExpectedException("ThirdPartyScope 추가 권한이 없습니다.", HttpStatus.FORBIDDEN) | ||
| } | ||
|
|
There was a problem hiding this comment.
현재는 Hibernate 1차 캐시 덕분에 동일 트랜젝션 내부에서 단일 인스턴스가 반환되며 문제가 없는것은 맞으나 간단한 수정으로 비교 실패 이슈를 완전히 차단할 수 있을 것 같아 제안드립니다!
| if (application.account != currentAccount && !isAdmin) { | |
| throw ExpectedException("ThirdPartyScope 추가 권한이 없습니다.", HttpStatus.FORBIDDEN) | |
| } | |
| if (application.account.id != currentAccount.id && !isAdmin) { | |
| throw ExpectedException("ThirdPartyScope 추가 권한이 없습니다.", HttpStatus.FORBIDDEN) | |
| } | |
이런식으로 교체하여 명시적인 비교를 하면 좋을 것 같습니다,해당 코드가 사용되는 DeleteApplicationServiceImpl.kt, ModifyApplicationServiceImpl.kt 등에서도요!
| val privateKey: String, | ||
| val publicKey: String, | ||
| val keyId: String, | ||
| val accessTokenExpiration: Long, |
There was a problem hiding this comment.
해당 키가 추가되며 userinfo 모듈에도 동일한 설정이 필요해지는데 개선 방안이 없을까요? Environment를 분리하거나 기본값 할당을 하고 코드에 주석달아놓는 식으로요
| @param:Schema(description = "Third-party 스코프 목록") | ||
| val scopes: List<ScopeReqDto>, |
There was a problem hiding this comment.
리스트의 크기에 제한이 없는 것 같습니다. 이론상 수천개 이상의 리스트를 전송할 수 있는 것 같아요.
There was a problem hiding this comment.
같은 scopeName으로 스코프를 중복 추가할 수 있는 검증이 없습니다. ThirdPartyScopeJpaRepository.findByApplicationIdAndScopeName()이 이미
존재하는데 활용되지 않고 있습니다.
There was a problem hiding this comment.
해당 클래스가 enum에서 클래스가 되며 equals(),hashCode() 메서드가 사라졌는데 내부적으로 이를 사용하는 JPA 등의 의존성이나 toSet() 과 같은 메서드의 반환값이 변동될 수 있을 것 같습니다.
There was a problem hiding this comment.
해당 클래스 역시 enum에서 클래스가 되며 equals(),hashCode() 메서드가 사라졌는데 내부적으로 이를 사용하는 JPA 등의 의존성이나 toSet() 과 같은 메서드의 반환값이 변동될 수 있을 것 같습니다.
2 participants
개요
#106 에서 언급되었던 ThirdPartyScope 기능을 구현합니다.
본문
category:name이 아닌applicationId:name으로 나타냅니다.