Skip to content

FIX : 챗봇 상담 내역 IDOR 과 권한 응답 코드·경로 매칭 정정 - #102

Open
RosieOh wants to merge 2 commits into
mainfrom
fix/authorization-review-followups
Open

FIX : 챗봇 상담 내역 IDOR 과 권한 응답 코드·경로 매칭 정정#102
RosieOh wants to merge 2 commits into
mainfrom
fix/authorization-review-followups

Conversation

@RosieOh

Copy link
Copy Markdown
Contributor

🏫 관련 이슈

Closes#100
Closes#101

🏫 PR 타입

  • 기능 추가
  • 기능 수정
  • 기능 삭제
  • 버그 수정
  • 의존성, 환경 변수, 빌드 관련 코드 업데이트

🏫 반영 브랜치

fix/authorization-review-followups -> main

🏫 변경 사항

1. 챗봇 상담 내역 IDOR ⭐ (#100)

조회 7종이 요청 파라미터의 userId 를 그대로 파사드에 넘긴다.
로그인만 하면 남의 userId 를 적어 다른 사람의 상담 내역과 세션을 읽을 수 있다.

publicResponseEntity<...> searchMessagesByKeyword(
@RequestParamStringuserId, // 클라이언트가 준 값@RequestParamStringkeyword) {
returnResponseEntity.ok(chatbotFacade.searchMessagesByKeyword(userId, keyword));
}

챗봇은 육아 상담이라 대화에 아이 건강·가정 사정이 담긴다. 서비스 계층에도 검증이 없다.

같은 도메인의 HealthControllerNotificationController이미 클라이언트 userId
무시하고 인증 주체를 쓴다. 챗봇만 빠져 있었다. 같은 방식으로 맞췄고,
파라미터는 기존 클라이언트 호환을 위해 남기되 사용하지 않는다.

2. BusinessException 이 언제나 400 (#101)

ErrorResponse.of(ErrorCode.INVALID_INPUT, ...) // ex.getErrorCode() 를 버린다
.status(HttpStatus.BAD_REQUEST) // ex.getHttpStatus() 를 버린다

BusinessException extends CareCodeException 이라 자기 ErrorCode 와 HttpStatus 를 이미 들고 있다.

영향이 큰 쪽은 인증이다. 프런트 인터셉터는 status === 401 에서만 토큰을 갱신하고
로그인으로 보낸다. 세션 만료가 400 으로 나가면 갱신도 재로그인도 일어나지 않고
"입력값이 유효하지 않습니다" 만 보인다. 해당 경로 9곳 —
HealthService 6, NotificationService 2, JwtService 1.

3. 고정 경로가 와일드카드에 먹혀 공개로 선언 (#101)

경로먹히던 규칙
GET /health/hospitals/likes/health/hospitals/*
GET /community/posts/liked/community/posts/*
GET /community/posts/bookmarked/community/posts/*

한 세그먼트라 상세 조회 와일드카드와 모양이 같아 눈에 띄지 않는다.
지금은 컨트롤러가 현재 사용자를 다시 확인해 401 을 내지만, 나중에 userId 파라미터를
받도록 바뀌면 그대로 남의 목록이 열린다.

4. 공공데이터 동기화가 비로그인으로 실행 (#101)

POST /api/public/care-facilities/sync-all
GET /api/public/care-facilities/swagger/sync

/api/public/care-facilities/** 가 통째로 permitAll 이라 동기화 트리거까지 열려 있었다.
외부 공공데이터 API 를 페이지 단위로 호출하고 DB 에 쓴다. 누구나 일일 한도를 태우고
DB 를 두드릴 수 있다.
이 프로젝트는 "공공데이터 한도 초과"를 운영 알림으로 잡는데,
그 상황을 외부에서 만들 수 있는 셈이었다. swagger/sync 는 GET 이라 브라우저 접속만으로도 실행된다.

같은 기능이 POST /api/admin/public-data/facilities/sync 로 이미 있어 ADMIN 으로 제한했다.
조회(swagger/stats, swagger/db-facilities)는 공개 그대로다.

🏫 테스트 결과

409 tests, 0 failures (기존 387 → +22)
BUILD SUCCESSFUL (jacocoTestCoverageVerification 포함)
신규 테스트고정하는 계약
ChatbotControllerOwnershipTest (8)남의 userId 를 넣어도 파사드에 인증 주체가 전달된다
BusinessExceptionStatusTest (6)UNAUTHORIZED→401, FORBIDDEN→403, INVALID_INPUT→400
AccessControlContractTest (+8)개인 목록 401, 동기화 401/403, 조회는 공개 유지

동기화 제한이 조회까지 막지 않는지도 함께 확인한다.

🏫 To Reviewer

  • 챗봇 userId 파라미터를 지우지 않고 남겼다. 지우면 기존 클라이언트가 400 을 받는다.
    HealthController 가 쓰는 표기((사용하지 않음) 대상은 인증 주체로 결정됩니다)를 따랐다.
    프런트 전환이 끝나면 [ Auth ] 사용자 API 계약 정리 (가입 전용 DTO, /users 구 경로 제거) #91 과 함께 제거하면 된다.
  • 경로 매칭 유형은 자동 점검할 가치가 있다. 이번에 컨트롤러의 고정 경로와
    SecurityConfig 규칙을 대조해 찾았는데, 같은 실수가 반복되고 있다
    (/health/**/hospitals/** 사고, 이메일 인증 /users/* vs /auth/*, 그리고 이번 3건).
    계약 테스트에 경로를 하나씩 적는 대신, 매핑 전수를 훑는 테스트를 두는 편이 낫다고 본다.
  • BusinessException 핸들러는 CareCodeException 핸들러와 사실상 같아졌다.
    지워도 상위 핸들러가 받지만, 로그 메시지 구분을 위해 남겼다.

전수 점검에서 나온 네 가지를 고친다.
1) 챗봇 조회 7종이 요청 파라미터의 userId 를 그대로 신뢰했다.
로그인만 하면 남의 userId 를 적어 다른 사람의 상담 내역과 세션을 읽을 수 있었다.
챗봇 대화에는 아이 건강·가정 사정이 담긴다.
GET /chatbot/messages/intent | date-range | helpful | search
GET /chatbot/sessions/status | date-range | count
조회 대상을 인증 주체로 바꿨다. 파라미터는 기존 클라이언트 호환을 위해 남기되
사용하지 않는다. HealthController·NotificationController 가 쓰는 방식과 같다.
(이 둘은 이미 클라이언트 userId 를 무시하고 있었다. 챗봇만 빠져 있었다.)
2) BusinessException 이 언제나 400 으로 나갔다.
전역 핸들러가 ErrorCode 를 INVALID_INPUT 으로, 상태를 400 으로 못 박고 있었다.
BusinessException 은 CareCodeException 의 하위 타입이라 자기 ErrorCode 와
HttpStatus 를 이미 들고 있는데 그 값을 통째로 버린 것이다.
영향이 큰 쪽은 인증이다. 프런트 인터셉터는 401 에서만 토큰을 갱신하고 로그인으로
보낸다. 세션 만료가 400 으로 나가면 갱신도 재로그인도 일어나지 않고
"입력값이 유효하지 않습니다" 만 보인다. 해당 경로가 9곳 있었다
(HealthService 6, NotificationService 2, JwtService 1).
3) 고정 경로가 앞선 와일드카드에 먹혀 공개로 선언돼 있었다.
GET /health/hospitals/likes ← /health/hospitals/* "내가 찜한 병원"
GET /community/posts/liked ← /community/posts/* "내가 좋아요한 글"
GET /community/posts/bookmarked ← /community/posts/* "내가 북마크한 글"
한 세그먼트라 상세 조회 와일드카드와 모양이 같아 눈에 띄지 않는다.
지금은 컨트롤러가 현재 사용자를 다시 확인해 401 을 내지만, 나중에 userId
파라미터를 받도록 바뀌면 그대로 남의 목록이 열린다. 규칙을 앞으로 옮겼다.
4) 공공데이터 동기화가 비로그인으로 실행 가능했다.
POST /api/public/care-facilities/sync-all
GET /api/public/care-facilities/swagger/sync
/api/public/care-facilities/** 가 통째로 permitAll 이라 그 아래 동기화 트리거까지
열려 있었다. 외부 공공데이터 API 를 페이지 단위로 호출하고 DB 에 쓴다. 누구나
일일 한도를 태우고 DB 를 두드릴 수 있었다는 뜻이다. 이 프로젝트는 "공공데이터
한도 초과" 를 운영 알림으로 잡는데, 그 상황을 외부에서 만들 수 있는 셈이었다.
swagger/sync 는 GET 이라 브라우저 접속이나 크롤러만으로도 실행된다.
같은 기능이 POST /api/admin/public-data/facilities/sync 로 이미 있어 ADMIN 으로 제한했다.
조회(swagger/stats, swagger/db-facilities)는 공개 그대로다.
회귀 테스트 22건. 접근제어 계약 테스트에는 위 경로들을 실제 응답 코드로 고정했다.
409 tests, 0 failures.
경로 매칭 실수를 네 번 반복했다.
/health/** 가 /hospitals/** 를 삼켜 병원 공개 조회가 통째로 401
이메일 인증 화이트리스트가 /users/* 를 가리켜 가입 흐름이 401
/health/hospitals/likes 가 /health/hospitals/* 에 먹혀 개인 목록이 공개
/community/posts/liked|bookmarked 가 게시글 상세 와일드카드에 먹힘
넷 다 규칙 자체는 멀쩡해 보이는데 매칭 순서 때문에 의도와 다르게 동작한 경우다.
계약 테스트에 경로를 하나씩 적는 방식으로는 새 엔드포인트가 생길 때마다 놓친다.
실제로 위 3·4번은 바로 직전에 머지한 기능이 만든 것이다.
기본값을 뒤집는다.
모든 매핑은 인증이 필요하다. 공개는 INTENTIONALLY_PUBLIC 에 적힌 것만.
목록에 없는 경로가 공개로 열리면 테스트가 깨지고 경로와 컨트롤러 이름을 찍어준다.
반대로 목록에 적어뒀는데 실제로는 막혀 있어도 깨진다(이메일 인증 사례가 그랬다).
RequestMappingHandlerMapping 에서 매핑을 모두 읽고, AuthorizationFilter 의 판단만
평가한다. 실제 요청을 보내지 않으므로 DB 변경이나 외부 API 호출 같은 부작용이 없다.
매핑을 하나도 읽지 못하면 아무것도 검사하지 않은 채 통과하므로, 훑은 매핑 수의
하한을 함께 확인한다. 조용히 무력화되는 게이트는 없는 것보다 나쁘다.
이 테스트를 붙이자 남은 미선언 공개 엔드포인트가 하나 나왔다.
POST /facilities/{id}/view
조회수 증가라 비로그인 방문자도 세야 한다. 의도된 공개로 판단해 근거와 함께
목록에 넣었다. 다만 이 값이 /facilities/popular 순위에 쓰이므로 부풀릴 수 있다는
점은 주석으로 남겼다.
411 tests, 0 failures.
@RosieOh

Copy link
Copy Markdown
ContributorAuthor

추가 — 경로 매칭 실수를 구조적으로 막는 테스트

리뷰에서 "같은 유형이 네 번째" 라고 적었는데, 경로를 하나씩 적는 방식으로는
새 엔드포인트가 생길 때마다 놓친다. 실제로 이번 3건 중 2건은 바로 직전에 머지한
#98·#99 가 만든 것
이다. 그래서 자동 점검을 붙였다.

EndpointAuthorizationCoverageTest

기본값을 뒤집는다.

모든 매핑은 인증이 필요하다. 공개는 INTENTIONALLY_PUBLIC 에 적힌 것만.

  • RequestMappingHandlerMapping 에서 컨트롤러 매핑을 전수 읽는다
  • 각 매핑에 대해 AuthorizationFilter 의 인가 판단만 평가한다
    (실제 요청을 보내지 않으므로 DB 변경·외부 API 호출 같은 부작용이 없다)
  • 목록에 없는데 공개로 열려 있으면 → 실패. 경로와 컨트롤러 이름을 찍어준다
  • 목록에 적어뒀는데 실제로는 막혀 있어도 → 실패
    (이메일 인증 경로가 정확히 그 상태로 오래 남아 있었다)

실패 메시지도 다음 행동을 알려준다.

비로그인으로 열려 있는데 공개 목록에 없는 엔드포인트다.
둘 중 하나다.
(1) 정말 공개여야 한다 → INTENTIONALLY_PUBLIC 에 추가하고 근거를 남긴다
(2) 공개면 안 된다 → SecurityConfig 에서 와일드카드보다 앞에 규칙을 선언한다

매핑을 하나도 읽지 못하면 아무것도 검사하지 않은 채 통과하므로,
훑은 매핑 수의 하한도 함께 확인한다. 조용히 무력화되는 게이트는 없는 것보다 나쁘다.
(CI 의 스키마 검증 skip 감지와 같은 이유다.)

붙이자마자 하나 더 나왔다

POST /facilities/{id}/view

조회수 증가라 비로그인 방문자도 세야 한다. 의도된 공개로 판단해 근거와 함께
목록에 넣었다. 다만 이 값이 /facilities/popular 순위에 쓰이므로 부풀릴 수 있다는 점은
주석으로 남겼다. 인기 순위를 조작에 민감하게 다뤄야 한다면 여기부터 손봐야 한다.

이 한 건 말고는 미선언 공개 엔드포인트가 없다. 앞선 수정들이 빠짐없이 반영됐다는 뜻이기도 하다.

docs/reference/access-control-matrix.md 에 "자동 점검" 절을 추가했다.


411 tests, 0 failures.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant

@RosieOh