Skip to content

fix(auth): restore sign-in recovery and safe return paths - #969

Open
seonghobae wants to merge 7 commits into
mainfrom
codex/signin-recovery-20260907
Open

fix(auth): restore sign-in recovery and safe return paths#969
seonghobae wants to merge 7 commits into
mainfrom
codex/signin-recovery-20260907

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

A failed sign-in previously replaced the page with the authentication error and removed the login action. This change keeps a bounded retry notice in the existing login card, preserves the intended post destination, removes callback artifacts from stored/restored/shared paths, and rejects paths that URL parsing resolves to another origin. The login card now includes padding in its width so its edges fit narrow screens.

Reuses StatusNotice, current translated strings, the shared return-path helper, and the existing OIDC redirect. ADR 0109/0220 and the dated gap baseline record the behavior and owner boundaries. Product-owned login/signup/recovery forms still require released Keyverse contracts.

Validation on exact head 583157c5d007d4903a78b2e30c0caac224f6460e:

  • Baseline: 8 failures across 16 focused recovery checks; implementation: all 16 passed. Expanded recovery coverage also passed across the five current compatibility locales, including keyboard activation.
  • Lint, TypeScript/production build, Storybook build, and 5 documentation checks passed. Actual browser fault injection confirmed repeated keyboard retry and readable layout at 320px. Built SignInRetry story: all 3 interaction steps passed.
  • Full three-file local regression: 53 passed, 73 failed (126 total), predominantly timeouts in existing authenticated journeys plus element-lookup failures. Heavy concurrent host load was observed but is not treated as proof that failures are harmless. No test limit or assertion was weakened. A four-case recheck passed three; the remaining case also failed with protected-main App and return-path source. This local evidence remains recorded rather than overwritten by hosted success.
  • Hosted Tests 34073159430 are terminal GREEN: frontend lint/full tests/production build/Storybook and the PostgreSQL-backed full backend suite passed on the exact head.
  • SAST 34073140374 and Security 34073140368 are terminal GREEN on the exact head.
  • Required CodeQL 34073140303 is terminal failure/fail-closed; it is not relabeled as source GREEN.
  • Qualifying independent current-head APPROVE: none.

Material UI delivery is therefore not complete. Intentionality/content fit are supported by the existing login-card composition and bounded retry copy; narrow-screen and keyboard recovery have browser evidence. Eight-locale DB-ledger delivery, full permission/error/recovery browser coverage against released Keyverse contracts, production authentication, and final independent evidence remain outstanding.

Keep Ready for Review but do not merge. Do not substitute hosted Tests/SAST/Security success, local browser evidence, historical review, synthetic status, or no-op head churn for the missing required CodeQL verdict and independent approval. No deployment, all-page p95 claim, or protected release is claimed.

@coderabbitai

coderabbitai Bot commented Sep 7, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: dc18d9ad-3531-4a35-b57c-f021c33ab313

📥 Commits

Reviewing files that changed from the base of the PR and between 83eba56 and 583157c.

📒 Files selected for processing (10)
  • docs/adr/0109-oidc-deep-link-state-recovery.md
  • docs/adr/0220-token-backed-status-notice.md
  • docs/product-technical-gap-baseline.md
  • docs/storybook-inventory.md
  • frontend/src/App.css
  • frontend/src/App.test.tsx
  • frontend/src/App.tsx
  • frontend/src/components/StatusNotice.stories.tsx
  • frontend/src/oidcReturnUrl.test.ts
  • frontend/src/oidcReturnUrl.ts

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


📝 Walkthrough

Walkthrough

OIDC 반환 URL의 안전성 검증과 콜백 파라미터 정제를 강화했습니다. 인증 실패와 토큰 누락을 로그인 재시도 화면으로 통합했습니다. 관련 테스트, Storybook 기록, ADR 및 기술 기준 문서를 갱신했습니다.

Changes

OIDC 로그인 복구

Layer / File(s) Summary
반환 URL 검증과 정제
frontend/src/oidcReturnUrl.ts, frontend/src/oidcReturnUrl.test.ts, docs/adr/0109-oidc-deep-link-state-recovery.md
반환 URL의 origin과 경로를 검증합니다. error, error_description, error_uri를 제거합니다. 저장과 복원 시 정제된 URL을 사용합니다.
로그인 실패 복구 흐름
frontend/src/App.tsx, frontend/src/components/StatusNotice.stories.tsx, frontend/src/App.css, docs/adr/0220-token-backed-status-notice.md
인증 오류와 토큰 누락을 로그인 복구 흐름으로 통합합니다. StatusNoticeLog in 재시도 액션을 사용합니다. 로그인 카드의 너비 계산을 수정합니다.
복구 동작 검증과 기록
frontend/src/App.test.tsx, docs/storybook-inventory.md, docs/product-technical-gap-baseline.md
모든 지원 로케일, 인증 오류, 키보드 재시도, 로딩 게이트와 Storybook 상태를 검증합니다. 관련 관찰과 수용 범위를 기록합니다.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 58315

Failed sign-ins now offer a safe retry while preserving sanitized in-app destinations and avoiding exposure of callback error details. The implemented recovery and return-path behavior has focused coverage, with no remaining concrete merge-blocking risk.

Sequence Diagram(s)

sequenceDiagram
  participant App
  participant StatusNotice
  participant restoreOidcReturnUrl
  participant signinRedirect
  App->>StatusNotice: 인증 실패 상태 표시
  StatusNotice->>App: Log in 액션 실행
  App->>restoreOidcReturnUrl: 정제된 목적지 복원
  App->>signinRedirect: 복원된 목적지로 OIDC 재시도
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 5 files. (5 skipped: 5 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 제목은 실패한 로그인 복구와 안전한 return path 처리를 명확하게 요약하며, 변경 사항의 주요 목적과 일치합니다.
Full details: Docstring Coverage

Explanation

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

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/signin-recovery-20260907

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

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

@seonghobae
seonghobae marked this pull request as ready for review September 7, 2026 01:29

Copy link
Copy Markdown
Contributor Author

DESIGN ASSURANCE — exact head 583157c5d007d4903a78b2e30c0caac224f6460e

The retry/return-path repair has substantive current-head evidence: frontend and backend suites and CodeQL completed successfully, and the patch preserves the authoritative OIDC destination while removing callback artifacts. I found no new causal defect in that flow.

The material sign-in surface is still not merge-ready under the current acceptance matrix. frontend/src/i18n.ts defines only en/ko/zh/ja/vi; es/de/fr are absent. The PR also records that successful production authentication and released product-facing Keyverse login/signup/recovery contracts are not proven, while central security/review admission jobs remain queued. Keep this as review admission, not protected acceptance.

Owner acceptance: retain domain/auth truth outside presentation state; supply ko/en/ja/zh/vi/es/de/fr screen-scoped versioned resources; prove retry/loading/error/offline/permission and successful-auth transitions in a real browser at desktop/intermediate/mobile widths with pointer, keyboard, visible focus, screen-reader role/name/state, reduced-motion and destination persistence/reload; then obtain terminal exact-head security/review gates. I left the dedicated writer’s source and refs unchanged.

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

Labels

bug Something isn't working priority: high type: bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant