[Refactor ]認証機能のコンポーネントやヘルパー関数などを features/authに集約しましょう

[KATO-Hiro]

対象範囲

AuthForm.svelte などのコンポーネント

• 現在：src/lib/components/AuthForm.svelte（共有コンポーネント）
• 理由：
  • UI コンポーネントは全プロジェクトで共有資産
  • feature-scoped にするとインポートパスが深くなる（$features/auth/components/AuthForm.svelte）
  • Props/API 変更に伴うリスク > メリット
  • ルート層（login, signup ページ）でのみ使用だが、独立した UI 部品として価値あり

Component 関連の E2E テストの大規模変更

• テストセレクタの変更リスク
• スクリーンショット差分が大量に生成される可能性

Lucia auth ライブラリの統合層

• locals.auth.validate() など、ライブラリ提供 API は src/hooks.server.ts に留める
• フロントエンドロジックのみを feature-scoped にする

検討事項

• AuthForm.svelte が十分に「auth 機能特化」か確認
• login form と signup form のニーズが同じかどうか
• その時点で移行するなら、十分なテストカバレッジがある状態で実施
• 段階的マイグレーション：リスク評価 → テスト準備 → 移行実行

[KATO-Hiro]

#1582 の残タスク

• sveltekit.md の Auth Audit ルールに「getLoggedInUserOptional 導入時は呼び出し側の null チェックを全件監査する」を追記

• service 層にredicrect が混入している問題とリファクタリングの方針
  getSession(locals) / getCurrentUser(locals) などを services に置き、user か null を返す。
  ルート側（+page.server.ts）で null チェックして redirect(...) を投げる、もしくは requireLoggedInUser(event) のような guard を src/lib/server/guards/ 等のルート層ヘルパーに置く。

• 絶対URL（同一オリジン）拒否の回帰テストが不足（src/lib/utils/url.test.ts）

redirectTo は「同一オリジンかつ相対パスのみ許可」が要件なので、http://localhost:5174/workbooks/bfs のような同一オリジンの絶対URLを false にするテストを追加してください。ここが未検証だと、実装が「同一オリジンなら absolute も許可」に退行しても検知できません。

テスト追加（isSameOriginRedirect('http://localhost:5174/workbooks/bfs', requestOrigin) → false）
同時に実装修正：絶対 URL（プロトコル付き）を先にフィルタリング

if (redirectTarget.includes('://') || redirectTarget.startsWith('//')) {
return false;
}

[KATO-Hiro]

🔒 Security Findings

⚠️ HIGH: Non-null Assertion での NPE

Files:

• src/routes/problems/[slug]/+page.server.ts:12-13, 23-24
• src/routes/workbooks/edit/[slug]/+page.server.ts:25

Issue:

// ❌ loggedInUser が null でも !.id でアクセス
const userId = loggedInUser!.id;

Recommendation:

// ✅ Null チェック後に安全にアクセス
if (!loggedInUser) {
  throw error(UNAUTHORIZED, 'Unauthorized');
}
const userId = loggedInUser.id;

---

⚠️ MEDIUM: Service 層の設計違反

Files:

• src/features/auth/services/session.ts:13-25 - redirect() を呼び出し
• src/features/auth/services/admin_access.ts:28-32 - redirect() を呼び出し
• src/features/auth/services/admin_access.ts:38-48 - error() を呼び出し

Issue: AGENTS.md ガイドラインに違反

Service functions return data or null; never call error() or redirect(). HTTP error translation belongs in route handlers.

Current (❌):

export async function validateAdminAccess(locals: App.Locals, url?: URL): Promise<void> {
  if ((await validateAdminStatus(locals)) !== AdminStatus.OK) {
    redirect(TEMPORARY_REDIRECT, buildLoginPath(url)); // ❌ Service で redirect
  }
}

Recommended (✅): Wrapper パターン

Service 層は純粋、Route helper で framework 処理を集約：

// src/features/auth/services/admin_access.ts （純粋、テスト可能）
export async function validateAdminStatus(locals: App.Locals): Promise<AdminStatus> {
  // ...
}

// src/features/auth/utils/admin_access.ts （Route helper）
export async function ensureAdminOrRedirect(locals: App.Locals, url?: URL): Promise<void> {
  const status = await validateAdminStatus(locals);
  if (status !== AdminStatus.OK) {
    redirect(TEMPORARY_REDIRECT, buildLoginPath(url));
  }
}

呼び出し側：

// Route handler
await ensureAdminOrRedirect(locals, url);

// Form action
await ensureAdminOrRedirect(locals);

利点：Service は純粋 + 呼び出し側は冗長性ゼロ

ただし utils/ は「副作用禁止」の規約があるため、ensureAdminOrRedirect の配置先は規約上グレー。

---

⚠️ MEDIUM: Module-scope Mutable State

File: src/routes/(admin)/account_transfer/+page.server.ts:14

let accountTransferMessages: FloatingMessage[] = []; // ❌ 全リクエスト間で共有

Risks:

• 競合状態（複数ユーザーが同時実行時に値が上書き）
• 情報漏洩（ユーザーAの結果がユーザーBに表示）

Recommendation: Superforms の message 機能または cookies を使用してリクエスト単位に管理

---

🐛 CodeRabbit Findings（×2回実施）

HIGH Issues（残存）

Issue	File	Line	Count
Null チェック漏れ	src/routes/problems/[slug]/+page.server.ts	12-13, 23-24	2
Service で redirect()	src/features/auth/services/session.ts	13-25	2 箇所
Service で error()	src/features/auth/services/admin_access.ts	38-48	1
Task 型不整合	src/routes/(admin)/tasks/[task_id]/+page.server.ts	14, 44-45	2

---

MEDIUM Issues

Issue	File	Line	Detail
テストアサーション不完全	src/features/auth/services/session.test.ts	52	.resolves にマッチャーなし
テストアサーション不完全	src/features/auth/services/admin_access.test.ts	54-55, 99-100	.resolves にマッチャーなし
Module-scope mutable	src/routes/(admin)/account_transfer/+page.server.ts	14	情報漏洩リスク
型不整合	src/routes/(admin)/tags/[tag_id]/+page.server.ts	13	getTag が Tags 返却 → Tag 単一返却が正しい
E2E パスワード不一致	e2e/helpers/auth.ts	6-11	signup 'TestPass123' vs login 'xxxxxxx'

---

NITPICK Issues（オプション）

Issue	File	Count
テストファイル不足	src/features/auth/utils/login.ts, signup.ts	2
Helper 関数重複	src/lib/utils/url.test.ts	3 箇所
冗長な型キャスト	src/features/auth/services/session.test.ts	2
型キャスト as never	src/features/auth/services/admin_access.test.ts	4
型安全性低下	src/lib/components/AuthForm.svelte:31	string に拡張
Reactive 漏れ	src/lib/components/SubmissionStatus/UpdatingDropdown.svelte:47	const → $derived

---

📊 優先度別修正スケジュール

Phase 1: Service 層リファクタ（短期）

- [ ] NPE リスク → null チェックに変更（3 箇所
- [ ] ensureSessionOrRedirect → 純粋関数 + Wrapper パターン
- [ ] getLoggedInUser の戻り値型を正確に
- [ ] validateAdminStatus → 純粋関数、ensureAdminOrRedirect Wrapper を提供
- [ ] validateAdminAccessForApi → 純粋関数化、error() は Route に移動

見積: 3-4 時間 + テスト修正 | パターン: Wrapper 関数で Service 純粋性 + Route 簡潔性を両立

Phase 2: テスト・ドキュメント（並行可）

- [ ] テストアサーション補完（.resolves → .resolves.toBeUndefined()）
- [ ] login.test.ts, signup.test.ts を追加
- [ ] E2E パスワード統一
- [ ] Module-scope state をリクエスト単位に変更

見積: 2-3 時間

Phase 3: 最適化（オプション）

- [ ] runTests helper 統一
- [ ] 型安全性改善
- [ ] Reactive store 修正

見積: 1-2 時間

---

🔍 Critical Code Review - レビュー指摘事項の批判的検討

対処済み（詳細はコミット履歴参照）

Issue	判定	対処内容
workbooks/[slug] 可視性チェック	却下	getLoggedInUser が guarantee するため現状正しい。ゲスト閲覧要件が来た場合は canRead 型含め変更が必要
workbooks delete 匿名許可	却下	同上。ただし Phase 2 pure 化時に if (loggedInUser && が穴になる（非可逆削除）— Phase 2 要注意

未解決・議論が必要な案件

❓ Issue 10: Guest Login の Redirect 対応は必要か？

File: src/lib/components/AuthForm.svelte:89-92

ユーザー質問: 「ゲストログインもリダイレクトに対応すべき？自分のアカウント作らないのに、そこまでする必要はなくない？」

批判的分析:

• Pro: UI/UX の一貫性（ゲスト login も admin login も同じ flow）
• Con: ゲスト = セッションレス、redirectTo 保持の意味が薄い。セッション破棄時に redirectTo も消える可能性
• 設計質問: ゲストが「ログイン前のページに戻る」動作は要件か？

提案: 要件確認 → 不要なら NITPICK、必要なら MEDIUM に格上げ

---

❓ Issue 11: workbooks/create の null-check は本当に不要か？

File: src/routes/workbooks/create/+page.server.ts:36-47

指摘: getLoggedInUser() が常に throw するので、その下の if (!author) は unreachable。

批判的評価:

• 指摘は 技術的に正確
• ただし「この check は将来の refactor 時の safeguard」という観点もある
• Service 層が pure 化される Phase 2 では、getLoggedInUser() は User | null を返す可能性 → 再度必要になる
• 今削除 → Phase 2 で復活するデッド・コードの往復

提案: Phase 2 roadmap が確定まで保留、またはコメント付きで明示的に「Phase 2 refactor 後に復活する可能性」と注記

---

参考

• AGENTS.md - Service Layer Guidelines
• .claude/rules/auth.md - Authentication Rules