chore(gateway): sanitize member headers#52
Conversation
![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 updates the AuthorizationHeaderFilter to prevent header spoofing by clearing member-related headers before populating them from JWT claims, adds PATCH to the CORS allowed methods, and introduces a unit test for the filter. Feedback highlights a security concern regarding a hardcoded secret key in the test, suggests consolidating header updates and using constants for header names to improve code quality, and recommends dynamically encoding expected values in tests to enhance maintainability.
| private static final String SECRET_KEY = | ||
| "c2lsdmVybmluZS10ZWNoLXNwcmluZy1ib290LWp3dC10dXRvcmlhbC1zZWNyZXQtc2lsdmVybmluZS10ZWNoLXNwcmluZy1ib290LWp3dC10dXRvcmlhbC1zZWNyZXQK"; |
There was a problem hiding this comment.
테스트 코드에 하드코딩된 시크릿 키가 포함되어 있습니다. 비록 테스트 환경이라 할지라도, 이는 보안상 좋지 않은 관행이며 실제 키가 실수로 커밋될 위험을 내포합니다. (스타일 가이드 8. Security & Stability, 99번 항목)
테스트용 키는 테스트 프로퍼티 파일(application-test.yml 등)을 통해 주입받거나, 테스트 실행 시 동적으로 생성하여 사용하는 것이 좋습니다. 이렇게 하면 실제 운영 환경과 유사한 방식으로 설정을 관리하면서도 민감한 정보의 노출을 막을 수 있습니다.
예를 들어, 테스트 클래스 상단에 @TestPropertySource를 사용하거나, src/test/resources에 테스트용 application.yml을 배치하여 관리할 수 있습니다.
References
- 민감 정보(Secret, Key, Token)가 코드 또는 레포에 포함되지 않았는지 확인합니다. (link)
| ServerHttpRequest.Builder requestBuilder = request.mutate() | ||
| .header("X-Member-Id", claims.getSubject()) | ||
| .header("X-Member-Email", claims.get("email", String.class)) | ||
| .header("X-Member-Role", claims.get("role", String.class)); | ||
| .headers(headers -> { | ||
| headers.remove("X-Member-Id"); | ||
| headers.remove("X-Member-Email"); | ||
| headers.remove("X-Member-Role"); | ||
| headers.remove("X-Member-Nickname"); | ||
|
|
||
| headers.set("X-Member-Id", claims.getSubject()); | ||
| headers.set("X-Member-Email", claims.get("email", String.class)); | ||
| headers.set("X-Member-Role", claims.get("role", String.class)); | ||
| }); | ||
|
|
||
| String nickname = claims.get("nickname", String.class); | ||
| if (nickname != null && !nickname.isBlank()) { | ||
| nickname = URLEncoder.encode(nickname, StandardCharsets.UTF_8); | ||
| requestBuilder.header("X-Member-Nickname", nickname); | ||
| String encodedNickname = URLEncoder.encode(nickname, StandardCharsets.UTF_8); | ||
| requestBuilder.headers(headers -> headers.set("X-Member-Nickname", encodedNickname)); | ||
| } |
There was a problem hiding this comment.
requestBuilder를 여러 번 호출하여 헤더를 수정하는 대신, 하나의 headers 블록으로 통합하는 것이 더 효율적이고 가독성이 좋습니다. 이렇게 하면 불필요한 중간 ServerHttpRequest.Builder 인스턴스 생성을 피할 수 있습니다.
또한, 반복적으로 사용되는 헤더 이름("X-Member-Id" 등)은 클래스 레벨의 상수로 선언하여 관리하는 것을 권장합니다. 이는 오타 가능성을 줄이고 코드의 유지보수성을 향상시킵니다. (스타일 가이드 7. Code Style & Clean Code, 90번 항목)
| ServerHttpRequest.Builder requestBuilder = request.mutate() | |
| .header("X-Member-Id", claims.getSubject()) | |
| .header("X-Member-Email", claims.get("email", String.class)) | |
| .header("X-Member-Role", claims.get("role", String.class)); | |
| .headers(headers -> { | |
| headers.remove("X-Member-Id"); | |
| headers.remove("X-Member-Email"); | |
| headers.remove("X-Member-Role"); | |
| headers.remove("X-Member-Nickname"); | |
| headers.set("X-Member-Id", claims.getSubject()); | |
| headers.set("X-Member-Email", claims.get("email", String.class)); | |
| headers.set("X-Member-Role", claims.get("role", String.class)); | |
| }); | |
| String nickname = claims.get("nickname", String.class); | |
| if (nickname != null && !nickname.isBlank()) { | |
| nickname = URLEncoder.encode(nickname, StandardCharsets.UTF_8); | |
| requestBuilder.header("X-Member-Nickname", nickname); | |
| String encodedNickname = URLEncoder.encode(nickname, StandardCharsets.UTF_8); | |
| requestBuilder.headers(headers -> headers.set("X-Member-Nickname", encodedNickname)); | |
| } | |
| ServerHttpRequest.Builder requestBuilder = request.mutate(); | |
| requestBuilder.headers(headers -> { | |
| headers.remove("X-Member-Id"); | |
| headers.remove("X-Member-Email"); | |
| headers.remove("X-Member-Role"); | |
| headers.remove("X-Member-Nickname"); | |
| headers.set("X-Member-Id", claims.getSubject()); | |
| headers.set("X-Member-Email", claims.get("email", String.class)); | |
| headers.set("X-Member-Role", claims.get("role", String.class)); | |
| String nickname = claims.get("nickname", String.class); | |
| if (nickname != null && !nickname.isBlank()) { | |
| String encodedNickname = URLEncoder.encode(nickname, StandardCharsets.UTF_8); | |
| headers.set("X-Member-Nickname", encodedNickname); | |
| } | |
| }); |
References
- 중복 로직이 유틸, 공통 컴포넌트, 도메인 메서드로 추출 가능한지 검토합니다. 헤더 이름과 같은 반복적인 문자열을 상수로 추출하는 것이 이 원칙에 부합합니다. (link)
| assertThat(mutatedRequest.getHeaders().get("X-Member-Nickname")).containsExactly( | ||
| "%EA%B4%80%EB%A6%AC%EC%9E%90" | ||
| ); |
There was a problem hiding this comment.
테스트 검증 로직에 URL 인코딩된 문자열 리터럴("%EA%B4%80%EB%A6%AC%EC%9E%90")이 직접 사용되었습니다. 이는 테스트의 가독성을 떨어뜨리고, 어떤 값이 기대되는지 파악하기 어렵게 만듭니다. 이는 구현 세부사항에 의존하는 테스트 방식으로, 인코딩 방식이 변경될 경우 테스트 코드를 수정해야 하는 단점이 있습니다. (스타일 가이드 6. Test & Maintainability, 80번 항목)
기대하는 원본 값("관리자")을 테스트 내에서 직접 인코딩하여 비교하면, 테스트의 의도가 훨씬 명확해지고 유지보수가 용이해집니다.
| assertThat(mutatedRequest.getHeaders().get("X-Member-Nickname")).containsExactly( | |
| "%EA%B4%80%EB%A6%AC%EC%9E%90" | |
| ); | |
| assertThat(mutatedRequest.getHeaders().get("X-Member-Nickname")).containsExactly( | |
| java.net.URLEncoder.encode("관리자", java.nio.charset.StandardCharsets.UTF_8) | |
| ); |
References
- 테스트가 구현 상세가 아닌 행위(Behavior)를 검증하는지 검토합니다. 하드코딩된 인코딩 결과값 대신, 인코딩 행위 자체를 테스트 코드에 포함하여 검증하는 것이 바람직합니다. (link)
1 participant
개요
게이트웨이에서 외부가 보낸 회원 헤더를 제거하고 JWT claims 기반 헤더로 재설정합니다.
@codex
변경 사항
테스트
./gradlew :gateway-service:test --tests com.comatching.gateway.filter.AuthorizationHeaderFilterTest체크리스트
스크린샷 / 참고 자료
feat/product-catalog-bundle(feat(product): add bundle flag to product catalog #50)