[Fix] Keychain 저장 시 토큰 유실 방지 및 README −74% 귀속 정정 (#82)

[ddodle]

Close #82

✨ PR 유형

어떤 변경 사항이 있나요??

• 새로운 기능 추가
• 버그 수정
• 사용자 UI 디자인 변경 및 추가
• 코드에 영향을 주지 않는 변경사항(오타 수정, 탭 사이즈 변경, 변수명 변경)
• 코드 리팩토링
• 주석 추가 및 수정
• 문서 수정
• 테스트 추가, 테스트 리팩토링
• 빌드 부분 혹은 패키지 매니저 수정
• 파일 혹은 폴더명 수정
• 파일 혹은 폴더 삭제

🛠️ 작업내용

커밋 2개. 1은 실버그 수정, 2는 README 사실 정정.

1. KeychainTokenStore.saveToKeychain — delete → add를 update 우선으로

기존은 SecItemDelete 후 SecItemAdd였다. add가 실패하면(재부팅 후 첫 잠금 해제 전 백그라운드 갱신 등) 옛 값은 이미 지워진 상태이고, save()의 롤백이 access까지 지워 로그아웃된다. 지워진 토큰은 어떤 재시도 로직으로도 복구할 수 없다.

before:  delete(key) → add(key, new)        add 실패 시 옛 값 없음
after:   update(key, new)                    실패해도 옛 값 유지
         └ errSecItemNotFound → add(key, new, accessible)

save()의 롤백도 바꿨다. refresh 저장이 실패하면 access를 삭제하던 것을 이전 값 복원으로. 이전 값이 없었을 때만 삭제한다. 쌍이 어긋나지 않게 하면서 로그아웃까지 가지 않게 한다.

kSecAttrAccessible은 add 경로에만 넣는다. 이 저장소가 만든 항목은 전부 같은 접근성이라 update 때 바꿀 이유가 없다.

테스트: SecItem 실패를 단위 테스트에서 주입할 수 없어 새 테스트는 없다. 기존 KeychainTokenStoreTests 6개(저장·조회·덮어쓰기·삭제·service 격리·직렬화)가 update 경로의 회귀를 덮는다. 덮어쓰기 테스트가 이제 update 분기를, 최초 저장 테스트가 add 분기를 지난다.

2. README 기술 포인트 −74% 귀속 정정

"경계에 맞춰 페이로드 −74%"는 경계 정렬이 페이로드를 줄인 것처럼 읽힌다. BASELINE_RESULTS.md 기준:

수치	비교 기준	원인
페이로드 −74%	레거시 원본 업로드 5,733KB → 1,466KB	다운샘플 + JPEG 재인코딩 (#34)
전처리 시간 −22%(A16) · −24%(A13)	A → D, 둘 다 transform:true	서브샘플 경계 정렬 (#49)

두 효과의 원인을 분리해 다시 썼다.

📋 추후 진행 상황

코드 수정은 이 PR로 마감. 리뷰에서 함께 제안된 "갱신 완료 후 도착한 옛 토큰 401의 재갱신"과 "refreshTask defer 위치"는 사용자 영향이 없어 제외했고, 근거는 #82에 기록했다.

📌 리뷰 포인트

• saveToKeychain의 switch updateStatus 분기 — errSecItemNotFound 외의 실패는 그대로 throw하는 게 맞는지
• save() 롤백에서 try? saveToKeychain(previous) — 복원 자체가 실패하면 조용히 넘어가는데, 이 시점엔 원 에러를 던지는 것 외에 할 수 있는 게 없다고 판단

✅ Checklist

PR이 다음 요구 사항을 충족하는지 확인해주세요!!!

• 커밋 메시지 컨벤션에 맞게 작성했습니다
  • 해당 링크 : 깃 모지 컨벤션
• 유지-보수를 위해 주석처리를 잘 작성하였는가?
  • 해당 링크 : Xcode 주석 정리

Summary by CodeRabbit

• 버그 수정

  • 리프레시 토큰 저장에 실패하면 기존 액세스 토큰을 복원하고, 기존 토큰이 없던 경우 새로 저장된 토큰을 삭제해 토큰 정보가 일관되게 유지됩니다.
  • 키체인 항목을 먼저 삭제하지 않고 기존 항목을 갱신하며, 갱신 또는 저장에 실패하면 오류를 반환합니다.

• 문서

  • 업로드 페이로드 감소율을 원본 업로드와 비교해 명시했습니다. 4032px 사진 1장 기준으로 페이로드가 74% 감소합니다.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: deebdfc1-d6e5-4c74-87ed-bb9559803845

📥 Commits

Reviewing files that changed from the base of the PR and between 2d1a78b and c32995c.

📒 Files selected for processing (2)

• README.md
• Rephoto_iOS/Utilities/Keychain/KeychainTokenStore.swift

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

---

Walkthrough

README에서 페이로드 74% 감소의 비교 기준을 원본 업로드로 명시합니다. Keychain 토큰 저장은 기존 항목 업데이트를 우선하고, refresh token 저장이 실패하면 이전 access token을 복원합니다.

Changes

업로드 전처리 설명

Layer / File(s)	Summary
페이로드 감소 기준 명시 README.md	4032px 사진 1장 기준 페이로드 74% 감소가 원본 업로드 대비 수치임을 명시합니다. 기존 JPEG 서브샘플 경계 설명과 기기별 처리 시간은 유지됩니다.

Keychain 토큰 저장

Layer / File(s)	Summary
토큰 저장 및 롤백 Rephoto_iOS/Utilities/Keychain/KeychainTokenStore.swift	저장 전에 기존 access token을 읽습니다. refresh token 저장이 실패하면 이전 값이 있을 때 복원하고, 없을 때는 새 access token을 삭제합니다. Keychain 항목은 먼저 업데이트하며, 항목이 없을 때만 기존 접근성 설정으로 추가합니다. 그 외 업데이트 오류와 추가 실패는 해당 상태 코드로 KeychainError.saveFailed를 발생시킵니다.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to c3299

The documentation now identifies the comparison baseline, and token saving preserves the previous access token on the established failure path. No actionable merge-blocking issue is established.

Security Architecture Review

Security architecture risk: 🔵 Low · up to c3299

The change reduces the chance of losing an existing token, but an uncommon failure during recovery could leave the two saved tokens out of sync. The identified impact is limited to the affected device’s authentication state; no new route for an attacker to obtain tokens or bypass authorization was established.

Retained concerns

• Low · reliability · inferred: A failed compensating write can leave a newly written access token stored after save throws, while the refresh token remains old or absent. The existing access-only login check can then report an authenticated session without a verified usable token pair.

Security review details

Security Blast Radius

• inferred — The demonstrated failure affects the token pair in the selected app Keychain service and its NetworkClient session. The inspected production path shows no new caller or cross-service token authority introduced by this PR.

Security Findings and Attack Paths

• inferred — If refresh persistence and subsequent restoration both fail after access is updated, requests may continue to use that access token despite the failed save, while a later refresh uses the old or missing refresh token. An attacker-controlled way to cause this sequence was not established.

Trust Boundaries and Controls

• observed — NetworkClient applies its authentication policy before attaching the stored access token to a request. Token persistence remains behind TokenStore, with the Security-framework operations confined to the Keychain implementation.

Resilience and Maintainability Implications

• observed — NetworkClient joins concurrent refresh requests through one refresh task and waits for an in-flight task during logout. Those controls do not verify the Keychain pair following a failed compensating write.

Hardening Proposals

• proposed — Consider making restoration failure distinguishable to the caller and defining a verifiable recovery state for the token pair. Persisting the pair as one item is another possible design if pairwise atomicity is required.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. (1 skipped: 1…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	제목은 Keychain 토큰 유실 방지와 README 수치 귀속 정정이라는 두 가지 주요 변경 사항을 명확하게 요약합니다.
Description check	✅ Passed	PR 설명은 변경 유형, 작업 내용, 테스트 범위, 추후 진행 상황, 리뷰 포인트, 체크리스트를 포함합니다. 템플릿의 필수 항목을 충족하며 변경 목적과 범위도 구체적으로 설명합니다.
Linked Issues check	✅ Passed	직접 연결된 이슈 #82의 코딩 요구사항을 충족합니다. README.md는 74% 페이로드 감소를 원본 업로드 대비 수치로 설명하고, 다운샘플링·JPEG 재인코딩과 서브샘플 경계 정렬에 대한 귀속을 수정합니다. KeychainTokenStore.saveToKeychain은 SecItemUpdate를 먼저 시도하고 `errSecItemNotFoun…
Out of Scope Changes check	✅ Passed	변경 사항은 이슈 #82의 두 목표인 README 귀속 수정과 Keychain 토큰 보존에 한정됩니다. README 변경과 기존 테스트 지원은 해당 목표를 검증하거나 구현하는 변경입니다. stale-token 401 처리와 refreshTask의 defer 배치는 변경하지 않았으며, 이슈 #82가 명시적으로 범위에서 제외했습니다.

Full details: Docstring Coverage

Explanation

Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. (1 skipped: 1 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{토끼가 토큰을 살펴보네
옛 값은 안전히 남겨 두고
없는 항목만 새로 담네
사진의 기준도 밝혀 적네
당근 한 입, 저장 완료!}

_{Comment @coderabbitai help to get the list of available commands.}