fix: 자격증명 버전으로 변경과 겹친 로그인·회전 세션 차단, 블랙리스트 ms 비교 - #492
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
🧹 knip — dead-code 리포트전체 리포트
|
🩺 NestJS Doctor — 90/100 (Excellent)진단 469건 (error 12).
architecture / security 상위 항목
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1939dfa342
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| UPDATE `auth_refresh_session` s | ||
| JOIN `account_credential` c ON c.`account_id` = s.`account_id` | ||
| SET s.`credential_version` = c.`password_updated_at` | ||
| WHERE s.`revoked_at` IS NULL; |
There was a problem hiding this comment.
When production already contains an unrevoked session created by the pre-fix race this commit addresses—an old password was verified, the password change committed, and then the session was inserted—this update assigns that session the current credential version. assertSessionUsable will consequently see matching versions and allow the compromised session to rotate indefinitely, so the migration preserves precisely the sessions the fix is intended to reject. Because the verification-time version cannot be inferred from these rows, existing credential-backed sessions need to be revoked or otherwise treated as untrusted instead of being backfilled to the current version.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
반영: 백필 제거 — 버전 미기록(null) 세션은 비밀번호 변경 이력이 있는 계정이면 다음 refresh에서 폐기·거절, 이력 없는 계정(경쟁 불가)은 null=null로 유지. 배포 뒤 변경 이력 있는 판매자·관리자 1회 재로그인(사용자 확인).
Coverage report
Test suite run success3782 tests passing in 357 suites. Report generated by 🧪jest coverage report action from 89d9133 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
비밀번호 변경·초기화 트랜잭션(교체 + 전 세션 폐기)이 로그인의 비밀번호 검증 뒤·발급 전에, 또는 refresh의 세션 확인 뒤·회전 전에 커밋되면 그 뒤 만들어진 세션이 폐기를 비껴가 살아남았다. 블랙리스트는 iat(초)와 비교해 변경과 같은 초에 발급된 옛 토큰도 통과시켰다(1초 창). - 자격증명 버전 = account_credential.password_updated_at(ms, 이력 없으면 null/0) - auth_refresh_session.credential_version 추가 + 마이그레이션(살아 있는 세션은 현재 버전으로 backfill) - 로그인: 검증한 credential 행의 버전을 issueAuthTokens에 넘겨 세션·토큰(cv 클레임)에 싣는다(발급 전 재조회 없음) - OIDC 로그인은 null(구매자는 자격증명 없음), dev 토큰은 현재 버전 - refresh: 세션 버전 ≠ 현재 버전이면 그 세션을 폐기하고 INVALID_REFRESH_TOKEN. 역할 불일치는 기존대로 폐기 없이 거절 - 회전은 세션 버전을 새 세션·새 토큰에 그대로 복사 — 확인 뒤 커밋된 변경도 토큰은 블랙리스트에, 새 세션은 다음 refresh에서 막힌다 - 블랙리스트: 변경 시각을 ms로 저장, cv < cutoff면 거부. cv 없는 옛 토큰만 iat < floor(cutoff/1000) - 키 auth:blk:cr: → auth:blk:cv:, 표식 auth:blk:ready → auth:blk:ready:v2 — 배포 직후 옛 초 값을 ms로 읽지 않고 재구축 전까지 DB 폴백 - credentialCutoffSec 제거, issuedBeforeCredentialChange로 Redis 경로·DB 폴백이 같은 규칙 - 정지·탈퇴 경로는 그대로 테스트 - credential-version.service.spec(신규, real DB+Redis): 본인 변경·관리자 초기화·판매자 초기화 × 로그인 경쟁·회전 경쟁 → 토큰 거부(Redis·DB 폴백) + 세션 refresh 거절·폐기, 변경 뒤 새 비밀번호 세션 통과, 무변경 연속 회전·OIDC(null) 회전 - jwt-bearer.strategy.spec: cv·legacy iat 판정 표를 Redis 경로·DB 폴백 양쪽에 - token-blacklist.service.spec: ms 저장·같은 초 갱신·옛 키/표식 무시·판정 함수 전수 표 - refresh-session.repository.spec: 버전 저장·복사, 정지 트랜잭션과 겹친 발급이 잠금 대기 뒤 거절(기존 고정 없음 → 추가) - 반증: 세션 버전 비교 제거·로그인 버전 재조회·회전 토큰/새 세션 버전 재조회·초 비교·FOR UPDATE 제거·옛 키/표식 이름에서 각각 실패 확인
- 리뷰 지적: 백필이 수정 전 경쟁으로 살아남은 세션에도 현재 버전을 채워 새 대조를 통과시킴, 행만으로는 가를 수 없음 - 백필 제거. null 세션은 비밀번호를 바꾼 적 있는 계정이면 다음 refresh에서 폐기·거절(1회 재로그인), 바꾼 적 없는 계정(경쟁 불가)은 null끼리 일치해 유지 - 회귀: 버전 기록 전 세션 2건(변경 이력 없음 → 유지, 있음 → 폐기·거절)
1939dfa to
89d9133
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89d9133416
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return token.cv !== undefined | ||
| ? token.cv < cutoffMs | ||
| : token.iat < Math.floor(cutoffMs / 1000); |
There was a problem hiding this comment.
When two password changes/resets for the same account run concurrently and acquire the credential-row lock in the opposite order from their application-generated timestamps, the later commit can leave password_updated_at lower than the cutoff already retained by SET_IF_NEWER. A subsequent login uses that lower current value as cv, so this comparison rejects every newly issued access token on the normal Redis path until the stale cutoff expires, while DB fallback accepts it. Allocate the version while holding the row lock or otherwise use a monotonic value whose order matches the committed credential state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
미반영(기록): #493 — 유효하나 같은 계정 ms 단위 동시 변경에서만, 막는 쪽으로 실패하고 액세스 TTL(15분) 뒤 회복. 행 잠금 안 버전 할당은 후속으로(사용자 확인).
- 96/86/92/96 → 97/92/97/98(실측 정수 내림, 사용자 결정) - 근거: 이 PR 첫 CI(run 37221170062)의 샤드 병합 수치 97.90/92.42/97.45/98.51 - 10063/10278·3797/4108·2026/2079·9154/9292 — 같은 트리를 단일 실행한 #492 리포트와 개수까지 일치 - 임계는 jest.config.js 한 곳(로컬 test:cov는 jest가, CI는 coverage-report의 merge-coverage가 같은 값으로 검사) - README·README.en·가이드 §9·jest.scripts.config.js 주석의 수치 갱신
1 participant
배경
iat(초)로 비교해, 변경과 같은 초에 발급된 옛 토큰이 통과했습니다.변경 (자격증명 버전 대조)
password_updated_at(ms)입니다. 변경 이력이 없거나 자격증명이 없는 구매자(OIDC) 계정은 0입니다.auth_refresh_session.credential_version컬럼을 추가했습니다(마이그레이션).cv클레임)에 넣습니다. 발급 직전에 다시 읽지 않습니다.INVALID_REFRESH_TOKEN으로 거절합니다.cv < cutoffMs면 거절합니다. 같은 초 문제가 사라집니다.auth:blk:cr:→auth:blk:cv:)과 완전성 표식(auth:blk:ready→auth:blk:ready:v2)을 바꿔, 배포 직후 옛 초 단위 값을 ms로 읽지 않습니다. 새 표식이 서기 전에는 기존 설계대로 DB로 판정합니다.배포 전환
cv가 없는 옛 토큰은 지금 방식(iat초 비교)으로 판정합니다. 액세스 토큰 수명(15분)이 지나면 사라집니다.테스트
credential-version.service.spec(실DB·실Redis·실제 초기화 서비스)을 추가했습니다. 변경 3종(본인 변경·관리자 초기화·판매자 초기화)을it.each로 돌립니다.cv없는 옛 토큰은 앞 초를 막고 같은 초와 뒤 초는 통과합니다.ACCOUNT_NOT_ACTIVE로 거절되어 살아 있는 세션이 0개인지 확인합니다.FOR UPDATE를 빼면 정지 경쟁 테스트가 실패합니다.플랜 대조
auth_refresh_session.credential_version+ 백필 마이그레이션cv에, 다시 읽지 않음INVALID_REFRESH_TOKENcv없는 옛 토큰은iat규칙