From 00707f45ba639a8a59954b87cce60ac31628e262 Mon Sep 17 00:00:00 2001 From: "k.hiro1818" Date: Wed, 6 May 2026 09:03:33 +0000 Subject: [PATCH 01/23] refactor: consolidate authentication logic into feature services Reorganizes auth code into `src/features/auth/` with dedicated services and utilities: - New services: session, admin_access (moved from routes/_utils/) - New utilities: login, signup (auth-specific logic extraction) - Updated routes to use new service layer (admin, login, signup pages) - Added E2E test for post-login redirect - Updated rules and AGENTS.md with refined guidelines Co-Authored-By: Claude Haiku 4.5 --- .claude/rules/auth.md | 18 +- .claude/rules/coding-style.md | 9 + .claude/rules/sveltekit.md | 4 + .claude/rules/testing-e2e.md | 33 ++ .claude/rules/testing.md | 9 + AGENTS.md | 5 + .../2026-05-04/redirect-after-login/plan.md | 320 ++++++++++++++++++ .../auth-feature-consolidation/plan.md | 278 +++++++++++++++ e2e/helpers/auth.ts | 21 +- e2e/redirect_after_login.spec.ts | 203 +++++++++++ e2e/votes.spec.ts | 2 +- e2e/workbook_order.spec.ts | 2 +- e2e/workbooks_list.spec.ts | 2 +- .../auth/services/admin_access.test.ts | 123 +++++++ .../auth/services/admin_access.ts} | 10 +- src/features/auth/services/session.test.ts | 136 ++++++++ src/features/auth/services/session.ts | 38 +++ src/features/auth/utils/login.test.ts | 31 ++ src/features/auth/utils/login.ts | 15 + src/features/auth/utils/signup.test.ts | 31 ++ src/features/auth/utils/signup.ts | 14 + src/lib/components/AuthForm.svelte | 2 +- .../SubmissionStatus/UpdatingDropdown.svelte | 17 +- src/{test => }/lib/utils/authorship.test.ts | 115 +------ src/lib/utils/authorship.ts | 32 -- src/lib/utils/url.test.ts | 249 ++++++++++++++ src/lib/utils/url.ts | 16 + .../(admin)/account_transfer/+page.server.ts | 21 +- src/routes/(admin)/tags/+page.server.ts | 17 +- .../(admin)/tags/[tag_id]/+page.server.ts | 32 +- src/routes/(admin)/tasks/+page.server.ts | 29 +- .../(admin)/tasks/[task_id]/+page.server.ts | 37 +- .../(admin)/vote_management/+page.server.ts | 7 +- .../(admin)/workbooks/order/+page.server.ts | 6 +- src/routes/(admin)/workbooks/order/+server.ts | 2 +- src/routes/(auth)/login/+page.server.ts | 15 +- src/routes/(auth)/login/+page.svelte | 8 +- src/routes/(auth)/signup/+page.server.ts | 15 +- src/routes/problems/[slug]/+page.server.ts | 25 +- src/routes/users/[username]/+page.server.ts | 15 +- src/routes/users/edit/+page.server.ts | 16 +- src/routes/votes/[slug]/+page.svelte | 15 +- src/routes/workbooks/+page.server.ts | 11 +- src/routes/workbooks/[slug]/+page.server.ts | 8 +- src/routes/workbooks/create/+page.server.ts | 15 +- .../workbooks/edit/[slug]/+page.server.ts | 7 +- 46 files changed, 1698 insertions(+), 338 deletions(-) create mode 100644 docs/dev-notes/2026-05-04/redirect-after-login/plan.md create mode 100644 docs/dev-notes/2026-05-05/auth-feature-consolidation/plan.md create mode 100644 e2e/redirect_after_login.spec.ts create mode 100644 src/features/auth/services/admin_access.test.ts rename src/{routes/(admin)/_utils/auth.ts => features/auth/services/admin_access.ts} (79%) create mode 100644 src/features/auth/services/session.test.ts create mode 100644 src/features/auth/services/session.ts create mode 100644 src/features/auth/utils/login.test.ts create mode 100644 src/features/auth/utils/login.ts create mode 100644 src/features/auth/utils/signup.test.ts create mode 100644 src/features/auth/utils/signup.ts rename src/{test => }/lib/utils/authorship.test.ts (74%) create mode 100644 src/lib/utils/url.test.ts diff --git a/.claude/rules/auth.md b/.claude/rules/auth.md index 569103cb8..6246a9c60 100644 --- a/.claude/rules/auth.md +++ b/.claude/rules/auth.md @@ -3,7 +3,7 @@ description: Authentication rules paths: - 'src/lib/server/auth.ts' - 'src/routes/(auth)/**' - - 'src/routes/(admin)/_utils/auth.ts' + - 'src/features/auth/**' - 'src/routes/(admin)/**' - 'src/hooks.server.ts' --- @@ -32,13 +32,25 @@ paths: - `src/lib/server/auth.ts`: Lucia configuration - `src/hooks.server.ts`: Global request handler -- `src/routes/(admin)/_utils/auth.ts`: - - `validateAdminAccess(locals)` — for page routes; redirects to `/login` for both unauthenticated and non-admin users (do not use in `+server.ts`) +- `src/features/auth/services/session.ts`: + - `getLoggedInUser(locals, url?)` — returns logged-in user or redirects to `/login` + - `ensureSessionOrRedirect(locals, url?)` — guard-only; redirects if no session +- `src/features/auth/services/admin_access.ts`: + - `validateAdminAccess(locals, url?)` — for page routes; redirects to `/login` for both unauthenticated and non-admin users (do not use in `+server.ts`) - `validateAdminAccessForApi(locals)` — for API routes (`+server.ts`); throws `error(401)` if unauthenticated, `error(403)` if not admin +- `src/features/auth/utils/login.ts`: `buildLoginPath(url?)` — generates `/login` or `/login?redirectTo=...` - `prisma/schema.prisma`: User, Session, Key models +## Redirect After Login Pattern + +After login/signup, redirect users back to their original intended page instead of hardcoding `/`. + +- Protected `load()`: pass `url` to `getLoggedInUser(locals, url)` — appends `?redirectTo=` automatically +- Login/signup actions: validate `redirectTo` with `isSameOriginRedirect()` before using (see `src/lib/utils/url.ts`) + ## Security - Never expose session secrets in client code - Use HTTPS in production - Validate all user inputs server-side +- **SvelteKit's `redirect()` does not validate origin** — always validate user-provided redirect destinations with `isSameOriginRedirect()` diff --git a/.claude/rules/coding-style.md b/.claude/rules/coding-style.md index d19e42138..43cb71b43 100644 --- a/.claude/rules/coding-style.md +++ b/.claude/rules/coding-style.md @@ -62,3 +62,12 @@ Delete function only if: (1) zero callers, (2) replacement exists, (3) dependent - No Prisma imports in route handlers - Validate input at system boundaries - Return safe defaults on service errors + +## Comments: Why, Not What + +Comment the **why** (non-obvious reason), not the **what** (code already says). + +- Bad: `// increment counter` +- Good: `// use task_id as deterministic ID to avoid SSR/hydration mismatch` + +Keep to one line. If longer, refactor code (naming, structure) instead. diff --git a/.claude/rules/sveltekit.md b/.claude/rules/sveltekit.md index 97232b062..e3cf7ff7d 100644 --- a/.claude/rules/sveltekit.md +++ b/.claude/rules/sveltekit.md @@ -12,6 +12,10 @@ paths: - Page routes: use `redirect()` to navigate - API routes (`+server.ts`): use `error()` — `redirect()` returns 3xx; `fetch` follows and gets HTML not JSON +## User-Provided Redirect Destinations + +**SvelteKit's `redirect()` does NOT validate origin.** If the redirect destination comes from user input (URL query params, form data), always validate with `isSameOriginRedirect(redirectTo, url.origin)` from `src/lib/utils/url.ts` before redirecting. + ## Internal Navigation: `resolve()` All internal navigation must use `resolve()` from `$app/paths`: diff --git a/.claude/rules/testing-e2e.md b/.claude/rules/testing-e2e.md index 7c8063c4e..4e1303c51 100644 --- a/.claude/rules/testing-e2e.md +++ b/.claude/rules/testing-e2e.md @@ -99,6 +99,28 @@ Prefer in order: - `page.pause()` for interactive debugging - Logs: `page.on('console', msg => console.log(msg.text()))` +## Multi-Route Infrastructure Changes: Test Ordering + +When implementing infrastructure changes that span multiple routes (e.g., redirect behavior, auth flow), write E2E tests **after** the core mechanism is implemented but **before** applying it to all routes: + +1. **Implement core mechanism** (unit-tested) — e.g., `isSameOriginRedirect()`, `redirect()` logic +2. **Write E2E tests** (RED state) — tests fail until all routes are updated +3. **Apply to all routes** (GREEN state) — update each load/action function to use the mechanism + +**Why this order?** + +- If you write E2E tests before the mechanism exists, they'll fail immediately and block progress +- E2E tests verify integration end-to-end; unit tests verify the building blocks +- The RED→GREEN cycle for E2E tests spans multiple phases, unlike unit tests which turn GREEN immediately + +**Example:** Redirect After Login pattern + +- Phase 1-2: Unit test `isSameOriginRedirect()` (GREEN) +- Phase 3-4: Unit test auth helpers (GREEN) +- Phase 5-6: Implement login/signup actions (GREEN) +- Phase 7: Write E2E tests for all routes (RED) +- Phase 8-9: Update all load functions to pass `url` (GREEN) + ## E2E Lessons from Votes Tests Recent fixes (detailed in plan.md): @@ -106,3 +128,14 @@ Recent fixes (detailed in plan.md): - Dropdown selectors drift when UI changes; re-verify on refactors - Abort signal prevents stale response race; update signal if fetch signature changes - Toast messages are temporary; use waiter pattern, not fixed delay + +## Timeout: Element Not Found + +When `page.locator()` or `getByRole()` times out: + +1. Verify element exists in DevTools +2. Check ARIA roles in component UI libraries often lack semantic role definitions +3. If `role="menuitem"` fails: use href pattern or functional marker instead (e.g., `a[href^="/login?redirectTo"]` vs navbar's `/login`) +4. Never use `.first()` / `.last()` (DOM order is unstable) + +**Example:** `getByRole('menuitem')` times out because DropdownItem renders as `
  • ` without the role. Use `a[href^="/login?redirectTo"]` to distinguish dropdown from navbar. diff --git a/.claude/rules/testing.md b/.claude/rules/testing.md index 22fce4087..d2681fa93 100644 --- a/.claude/rules/testing.md +++ b/.claude/rules/testing.md @@ -32,6 +32,15 @@ E2E files: **must** use `.spec.ts` extension (`.test.ts` not detected). - Use `toBe(true)` / `toBe(false)` not `toBeTruthy()` / `toBeFalsy()` - DB query tests: assert `orderBy`, `include` with `expect.objectContaining`, not just `where` - Enum membership: `Object.hasOwn(Enum, value)` not `in` (avoids prototype chain) +- `Promise`: use `resolves` alone (not `toBeUndefined()`); `.resolves` confirms the promise resolves without asserting the value + +```typescript +// ✓ Correct: confirms promise resolves without throwing +await expect(ensureSessionOrRedirect(mockLocals)).resolves; + +// ✗ Avoid: toBeUndefined() is redundant for Promise +await expect(ensureSessionOrRedirect(mockLocals)).resolves.toBeUndefined(); +``` ### Describe Organization diff --git a/AGENTS.md b/AGENTS.md index 7b394d932..7d7e8c9c0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -19,6 +19,11 @@ Always prefer simplicity over pathological correctness. YAGNI, KISS, DRY. No bac 5. After all phases complete (feature and refactor branches only — not hotfixes or dependency bumps): run a mandatory refactor cycle. Write to `plan.md`: novel lessons (implementation blockers, non-obvious patterns not already in rules) and remaining tasks. Discard `phase-N.md` files. Run `coderabbit review --plain`; write all findings of `critical` / `high` / `potential_issue` (medium) to a `## CodeRabbit Findings` section in `plan.md`. The user decides which to fix before opening a PR; do not fix any finding unilaterally. `nitpick` findings defer to PR CI. 6. Run `/session-close` at the end of each session: updates plan checklist, proposes rule/skill additions, checks for bloat, and detects repeated instructions +**Plan Approval ≠ Implementation Start:** Generating a plan (`/writing-plans`) does NOT authorize implementation. Always: + +- Wait for explicit user consent ("let's start", "implement", etc.) +- Use AskUserQuestion before starting if requirements are ambiguous (data model, preferences, test scope, etc.) + ## Tech Stack SvelteKit 2 + Svelte 5 (Runes) + TypeScript | PostgreSQL + Prisma | Flowbite Svelte + Tailwind 4 | Vitest + Playwright | oxlint (JS/TS) + ESLint (Svelte) diff --git a/docs/dev-notes/2026-05-04/redirect-after-login/plan.md b/docs/dev-notes/2026-05-04/redirect-after-login/plan.md new file mode 100644 index 000000000..da8ab783d --- /dev/null +++ b/docs/dev-notes/2026-05-04/redirect-after-login/plan.md @@ -0,0 +1,320 @@ +# ログイン後リダイレクト実装計画 + +## 概要 + +ログイン後のリダイレクト先をハードコード（`/`）から、ログイン前に訪問しようとしていたページへ戻す仕組みを実装する。 + +**目的：** + +- ログイン必須ページ（例：`workbooks/create`）にアクセス後、ログインして同ページへ戻るUXを実現 +- 登録フロー（signup）でも同様の動作を提供 +- 将来の保護ルートにも一貫して適用できる汎用的な仕組みとして設計 + +**方針：** URLクエリパラメータ `?redirectTo=...` を使用（SvelteKit公式docs Form Actionsページのコード例にも記載のパターン） + +--- + +## 設計方針 + +### セキュリティ：Open Redirect 対策 + +`redirectTo` は外部URLへの誘導（フィッシング等）に悪用可能なため、**同一オリジンの相対パスのみ**を許可する。 + +`isSameOriginRedirect` を `src/lib/utils/url.ts` に追加： + +```typescript +export function isSameOriginRedirect(redirectTarget: string, requestOrigin: string): boolean { + try { + const targetUrl = new URL(redirectTarget, requestOrigin); + return targetUrl.origin === requestOrigin; + } catch { + // new URL() throws TypeError on malformed input; treat as unsafe. + return false; + } +} +``` + +- 標準 `URL` API のみ使用（外部ライブラリ不要） +- `//danger.com` 等のプロトコル相対URL、絶対URLを確実に弾く +- `startsWith('/')` だけでは `//danger.com` を突破されるため不十分 + +### ヘルパー関数の修正方針 + +既存の `ensureSessionOrRedirect` / `getLoggedInUser` に `url?: URL` オプショナルパラメータを追加。既存呼び出しとの互換性を維持しつつ、`url` を渡した場合のみ `?redirectTo=` を付与する。 + +```typescript +// src/lib/utils/authorship.ts（変更後） +export const getLoggedInUser = async ( + locals: App.Locals, + url?: URL, +): Promise => { + await ensureSessionOrRedirect(locals, url); + const loggedInUser = locals.user; + + if (!loggedInUser) { + redirect(TEMPORARY_REDIRECT, buildLoginPath(url)); + } + + return loggedInUser; +}; + +export const ensureSessionOrRedirect = async (locals: App.Locals, url?: URL): Promise => { + const session = await locals.auth.validate(); + + if (!session) { + redirect(TEMPORARY_REDIRECT, buildLoginPath(url)); + } +}; + +function buildLoginPath(url?: URL): string { + let path = '/login'; + + if (url) { + path += `?redirectTo=${encodeURIComponent(url.pathname + url.search)}`; + } + + return path; +} +``` + +### ログイン・登録アクションの変更 + +両アクションで `redirectTo` を読み取り、検証後にリダイレクト： + +```typescript +// login/signup action内（共通） +const redirectTo = url.searchParams.get('redirectTo'); +let destination = HOME_PAGE; + +if (redirectTo && isSameOriginRedirect(redirectTo, url.origin)) { + destination = redirectTo; +} + +redirect(SEE_OTHER, destination); +``` + +### votes/[slug] の扱い + +`votes/[slug]` はログインなしでも閲覧可能なページ。未ログイン時はページ内に「ログイン」「新規登録」ボタンを表示する設計になっている。これらのリンクに `?redirectTo=/votes/[slug]` を付与することで、ログイン後に投票ページへ戻れるようにする（ページ自体のガードは追加しない）。 + +--- + +## 実装フェーズ + +### Phase 1: `isSameOriginRedirect` テスト（先に書く） + +`src/lib/utils/url.test.ts` に追加（実装前に書いてREDにする）。 + +**通過すべきケース（`true`）:** + +- `/workbooks/bfs`（同一オリジンの相対パス） +- `/workbooks/bfs?tab=foo`（クエリ付き相対パス） +- `/%2F%2Fexternal.example.com`（`%2F` はパス区切りでなくパス文字として解釈→ドメインでなくパスとして扱われるため同一オリジン内の遷移） +- `/../../../external.example.com`（パストラバーサルはURL パーサーが正規化→ドメインでなくパスとして扱われるため同一オリジン内の遷移） + +**弾くべきケース（`false`）:** + +- `https://danger.com`（異オリジンの絶対URL） +- `//danger.com`（プロトコル相対URL） +- `https://app.com@danger.com/path`（`app.com` がusernameに解釈され origin は `danger.com`） +- `javascript:alert()`（opaque origin `"null"`） +- `data:text/html,`（opaque origin `"null"`） +- `not-a-url!!!`（不正な文字列、`new URL` がthrowし `false`） + +### Phase 2: `isSameOriginRedirect` 実装 + +- `src/lib/utils/url.ts` に `isSameOriginRedirect` を追加 +- `pnpm test:unit` でPhase 1のテストがGREENになることを確認 + +### Phase 3: 認証ヘルパーのテスト（先に書く） + +`src/lib/utils/authorship.test.ts`（既存があれば追記）に追加。`locals.auth` は `vi.mock` でモック。 + +- `url` を渡した場合 → `/login?redirectTo=%2Fworkbooks%2Fbfs` へリダイレクトすること +- `url` を渡さない場合 → `/login` へリダイレクトすること（既存動作の維持確認） + +### Phase 4: 認証ヘルパー実装 + +- `src/lib/utils/authorship.ts` の `ensureSessionOrRedirect` / `getLoggedInUser` に `url?: URL` を追加 +- `src/routes/(admin)/_utils/auth.ts` の `validateAdminAccess` にも同様の修正 +- `pnpm test:unit` でGREENを確認 + +### Phase 5: ログイン・登録アクションのテスト（先に書く） + +`src/routes/(auth)/login/` および `signup/` に隣接するテストファイルに追加。 + +- 有効な `redirectTo`（`/workbooks/bfs`）→ ログイン後そのパスへリダイレクト +- 無効な `redirectTo`（`https://danger.com`）→ ホーム (`/`) へリダイレクト +- `redirectTo` なし → ホームへリダイレクト + +### Phase 6: ログイン・登録アクション実装 + +- `src/routes/(auth)/login/+page.server.ts` で `redirectTo` を読み取り・検証・リダイレクト +- `src/routes/(auth)/signup/+page.server.ts` で同じ処理を実装 +- `pnpm test:unit` でGREENを確認 + +### Phase 7: E2E テスト（先に書く・Playwright） + +Phase 6完了時点でログイン機構が `redirectTo` を処理できる状態になる。 +ここでE2Eテストを書いて **RED** にし、Phase 8・9の実装で **GREEN** にする。 + +`e2e/` 以下に新規テストファイルを追加。 + +**各ルートの `redirectTo` 付与確認（Phase 8・9 で変更する全ルート）** + +未ログイン状態でアクセスしたとき、`/login?redirectTo=<そのパス>` にリダイレクトされることを確認する。 +ログイン後に元のページへ戻ることを確認するのは代表ルートのみで十分（機構は共通のため）。 + +| ルート | 確認内容 | +| ------------------------- | --------------------------------------------------------------------------------------------- | +| `/workbooks` | `?redirectTo=%2Fworkbooks` が付与される | +| `/workbooks/` | `?redirectTo=%2Fworkbooks%2F` が付与される ★ログイン後の復帰もここで確認 | +| `/workbooks/create` | `?redirectTo=%2Fworkbooks%2Fcreate` が付与される | +| `/problems/` | `?redirectTo=...` が付与される | +| `/users/edit` | `?redirectTo=%2Fusers%2Fedit` が付与される | +| `/users/` | `?redirectTo=...` が付与される | +| `/votes/` | ログイン・アカウント作成ボタンをクリックした際のリンク href に `?redirectTo=...` が付与される | +| `/admin/account_transfer` | `?redirectTo=...` が付与される（管理者ユーザーでログイン後に復帰も確認） | +| `/admin/tasks` | `?redirectTo=...` が付与される | +| `/admin/tasks/` | `?redirectTo=...` が付与される | +| `/admin/tags` | `?redirectTo=...` が付与される | +| `/admin/tags/` | `?redirectTo=...` が付与される | + +**登録フロー（一般ユーザーの典型的なユースケース）：** + +- 未ログイン → `/workbooks/` → ログインページ → 登録ページへ遷移した場合 `redirectTo` が引き継がれること +- 登録成功後 → `/workbooks/` に戻ること + +**votes/[slug] のログインボタン経由フロー：** + +`/votes/[slug]` はページ自体は公開だが、ページ内のログイン・登録リンクに `redirectTo` が付与される。 + +- 未ログイン状態で `/votes/` を表示 → ログインリンクのhrefに `?redirectTo=%2Fvotes%2F` が含まれること +- 未ログイン状態で `/votes/` を表示 → アカウント作成リンクのhrefに `?redirectTo=%2Fvotes%2F` が含まれること +- ログインリンクからログイン → ログイン後 `/votes/` に戻ること +- 登録リンクから登録ページへ遷移した場合も `redirectTo` が引き継がれ、登録後 `/votes/` に戻ること + +※「ログイン・アカウント作成ボタンが表示されること」自体はvotes機能のテストの責務。このプランではリンクの `href` に `redirectTo` が正しく付与されているかのみを検証する。 + +**open redirect 防止（不正な `redirectTo`）：** + +- `?redirectTo=https://danger.com` → ログイン後はホーム (`/`) に遷移すること +- `?redirectTo=//danger.com` → 同上 +- `?redirectTo=javascript:alert()` → 同上 + +### Phase 8: ロード関数の一括更新 + +`getLoggedInUser(locals)` → `getLoggedInUser(locals, url)` に変更。 +対象ルート（`load` 関数のシグネチャに `url` を追加する必要あり）： + +| ファイル | 現在の呼び出し | +| --------------------------------------------- | ------------------------- | +| `src/routes/workbooks/[slug]/+page.server.ts` | `getLoggedInUser(locals)` | +| その他 `ensureSessionOrRedirect` 呼び出し箇所 | 同様 | + +- `pnpm test:e2e` で対象ルートのテストがGREENになることを確認 + +### Phase 8.5: 既存E2Eテストの修正 + +Phase 8実装後、`/login` への完全一致アサーションが `/login?redirectTo=...` にマッチしなくなるため修正が必要。 + +| ファイル | 行 | 変更内容 | +| ---------------------------- | --- | ---------------------------------------------- | +| `e2e/votes.spec.ts` | 169 | `toHaveURL('/login')` → `toHaveURL(/\/login/)` | +| `e2e/workbook_order.spec.ts` | 16 | `toHaveURL('/login')` → `toHaveURL(/\/login/)` | +| `e2e/workbooks_list.spec.ts` | 32 | `toHaveURL('/login')` → `toHaveURL(/\/login/)` | + +影響なし（変更不要）： + +- `e2e/user_edit.spec.ts:13` — すでに `toHaveURL(/\/login/)` を使用 +- `e2e/helpers/auth.ts:22` — `/login` を直接 `goto` するため `redirectTo` が付かない → `/` アサーションはそのまま正しい + +### Phase 9: 直接 `redirect('/login')` の統一 + +以下のファイルは `ensureSessionOrRedirect` を使わず直接 `redirect(30x, '/login')` を呼び出している。ヘルパー経由に統一する： + +| ファイル | 件数 | +| ----------------------------------------------------- | ---- | +| `src/routes/problems/[slug]/+page.server.ts` | 1 | +| `src/routes/users/edit/+page.server.ts` | 2 | +| `src/routes/users/[username]/+page.server.ts` | 2 | +| `src/routes/workbooks/create/+page.server.ts` | 1 | +| `src/routes/(admin)/account_transfer/+page.server.ts` | 2 | +| `src/routes/(admin)/tasks/+page.server.ts` | 2 | +| `src/routes/(admin)/tasks/[task_id]/+page.server.ts` | 2 | +| `src/routes/(admin)/tags/+page.server.ts` | 2 | +| `src/routes/(admin)/tags/[tag_id]/+page.server.ts` | 2 | + +- `pnpm test:e2e` で全ルートのテストがGREENになることを確認 + +### Phase 10: auth.md 更新 + +`.claude/rules/auth.md` に「Redirect After Login Pattern」セクションを追加し、今後の実装が一貫した書き方になるよう文書化。 + +### Phase 11: ルール・ドキュメント追加 + +今回の実装で得られた、既存ルールに存在しない知見を英語で簡潔に追記する。 + +**1. `auth.md`（またはルート `security.md` 新規作成）: Open Redirect 防止パターン** + +- `startsWith('/')` だけでは `//danger.com`（プロトコル相対URL）を突破されるため不十分 +- 正しい検証は `new URL(redirectTarget, origin).origin === origin` +- 適用範囲：ユーザー入力のURLを受け取って `redirect()` する箇所すべて + +**2. `sveltekit.md`: `redirect()` は origin 検証を行わない** + +- SvelteKit の `redirect()` は宛先URLの origin を検証しない（フレームワーク側に保護なし） +- ユーザー由来の redirect 先を使う場合は必ず開発者側で検証する + +**3. `testing-e2e.md`: インフラ変更時の E2E テスト配置** + +- 複数ルートへ段階的に展開するインフラ変更では、「機構が動く Phase の後・全ルート適用 Phase の前」に E2E を書いて RED にする +- 機構が整っていない状態で E2E を書いても通らないため、通常のユニットテストより一段遅らせるのが正しい順序 + +--- + +## 影響ファイル一覧 + +**修正（既存）:** + +- `src/lib/utils/authorship.ts` +- `src/routes/(admin)/_utils/auth.ts` +- `src/routes/(admin)/account_transfer/+page.server.ts` +- `src/routes/(admin)/tasks/+page.server.ts` +- `src/routes/(auth)/login/+page.server.ts` +- `src/routes/(auth)/signup/+page.server.ts` +- `src/routes/workbooks/[slug]/+page.server.ts` +- `src/routes/workbooks/create/+page.server.ts` +- `src/routes/problems/[slug]/+page.server.ts` +- `src/routes/users/edit/+page.server.ts` +- `src/routes/users/[username]/+page.server.ts` +- `src/routes/(admin)/tasks/[task_id]/+page.server.ts` +- `src/routes/(admin)/tags/+page.server.ts` +- `src/routes/(admin)/tags/[tag_id]/+page.server.ts` +- `.claude/rules/auth.md` + +**追加（新規）:** + +- `src/lib/utils/url.test.ts`（`isSameOriginRedirect` のユニットテスト） +- `e2e/redirect_after_login.spec.ts`（ログイン・登録後リダイレクトのE2Eテスト） + +**修正（既存E2E）:** + +- `e2e/votes.spec.ts`（`/login` 完全一致 → 正規表現に変更） +- `e2e/workbook_order.spec.ts`（同上） +- `e2e/workbooks_list.spec.ts`（同上） + +--- + +## 却下した代替案 + +### URLクエリパラメータ以外のアプローチ + +| 案 | 却下理由 | +| ------------------- | ------------------------------------------------------------------------------------------ | +| **短命Cookie** | 「ストレージへの保存を避けたい」というプロジェクト方針に反する。管理コストも増大 | +| **Refererヘッダー** | ブラウザによって送信されないケースが多く信頼性が低い。ブックマーク・直リンクでは機能しない | + +### `startsWith('/')` のみの検証 + +`//danger.com` はプロトコル相対URLとしてブラウザが `https://danger.com` に解釈するため、この検証では open redirect を防げない。標準 `URL` API による origin 比較が必要。 diff --git a/docs/dev-notes/2026-05-05/auth-feature-consolidation/plan.md b/docs/dev-notes/2026-05-05/auth-feature-consolidation/plan.md new file mode 100644 index 000000000..5571b907a --- /dev/null +++ b/docs/dev-notes/2026-05-05/auth-feature-consolidation/plan.md @@ -0,0 +1,278 @@ +# Auth Feature 統合計画 + +## 概要 + +現在、認証関連のロジックが散在している状態を、`src/features/auth/` への feature-scoped 設計に統合する。 + +**目的：** + +- 認証関連のコード（services, utils, types）を一箇所に集約 +- AGENTS.md の feature-scoped 設計に準拠 +- テストの co-location を実現 +- 技術負債（分散したコード）の解消 + +**現状の問題：** + +- `src/lib/utils/authorship.ts` - login 関連の関数と authorship 権限判定が混在 +- `src/routes/(admin)/_utils/auth.ts` - admin 専用の auth ロジック +- `buildLoginPath()` - 2 箇所で重複実装 +- login/signup ロジックは route handlers に直接記述 + +--- + +## 設計方針 + +### 新規作成: `src/features/auth/` + +``` +src/features/auth/ +├── services/ +│ ├── login.ts # ログイン処理 +│ ├── signup.ts # 登録処理 +│ ├── session.ts # セッション管理・検証 +│ ├── admin_access.ts # admin 権限検証（API用も含む） +│ └── _.test.ts +├── utils/ +│ ├── login.ts # ← buildLoginPath() をここに統合 +│ └── _.test.ts +├── types/ +│ ├── auth_status.ts # AdminStatus enum +│ └── session.ts # Session type +└── constants/ + └── messages.ts # エラーメッセージなど +``` + +### 既存コードの移行 + +#### Phase 1: Service層を抽出（新規ファイル） + +**`src/features/auth/services/session.ts`** - セッション検証の共通化 + +```typescript +// 既存の authorship.ts から移動・リネーム +export async function validateSession(locals: App.Locals): Promise<{ valid: boolean }> { + const session = await locals.auth.validate(); + return { valid: !!session }; +} + +export async function getLoggedInUser( + locals: App.Locals, + url?: URL, +): Promise { + const session = await locals.auth.validate(); + if (!session) { + const loginPath = buildLoginPath(url); + redirect(TEMPORARY_REDIRECT, loginPath); + } + + if (!locals.user) { + redirect(TEMPORARY_REDIRECT, buildLoginPath(url)); + } + + return locals.user; +} + +export async function ensureSessionOrRedirect(locals: App.Locals, url?: URL): Promise { + const session = await locals.auth.validate(); + if (!session) { + redirect(TEMPORARY_REDIRECT, buildLoginPath(url)); + } +} +``` + +**`src/features/auth/services/admin_access.ts`** - admin 権限検証 + +```typescript +// 既存の (admin)/_utils/auth.ts から移動 +export enum AdminStatus { + OK = 'ok', + UNAUTHENTICATED = 'unauthenticated', + UNAUTHORIZED = 'unauthorized', +} + +export async function validateAdminStatus(locals: App.Locals): Promise { + // 既存実装 +} + +export async function validateAdminAccess(locals: App.Locals, url?: URL): Promise { + if ((await validateAdminStatus(locals)) !== AdminStatus.OK) { + const loginPath = buildLoginPath(url); + redirect(TEMPORARY_REDIRECT, loginPath); + } +} + +export async function validateAdminAccessForApi(locals: App.Locals): Promise { + // 既存実装（error() 返却） +} +``` + +**`src/features/auth/utils/login.ts`** + +```typescript +// 2 箇所の重複を統合 +import { LOGIN_PAGE } from '$lib/constants/navbar-links'; + +export function buildLoginPath(url?: URL): string { + if (!url) { + return LOGIN_PAGE; + } + + return `${LOGIN_PAGE}?redirectTo=${encodeURIComponent(url.pathname + url.search)}`; +} +``` + +#### Phase 2: Authorship 権限判定の分離 + +**`src/lib/utils/authorship.ts`** - 権限判定に特化（削減版） + +```typescript +// ログイン関連は削除、権限判定のみ残す +export const isAdmin = (role: Roles): boolean => { + return role === Roles.ADMIN; +}; + +export const hasAuthority = (userId: string, authorId: string): boolean => { + return userId.toLowerCase() === authorId.toLowerCase(); +}; + +export const canRead = (...) => { ... }; +export const canEdit = (...) => { ... }; +export const canDelete = (...) => { ... }; +``` + +新しいエクスポート： + +```typescript +// src/features/auth/index.ts +export { validateSession, getLoggedInUser, ensureSessionOrRedirect } from './services/session'; + +export { validateAdminAccess, validateAdminAccessForApi } from './services/admin_access'; + +export { buildLoginPath } from './utils/login'; +``` + +--- + +## スコープの限定：含めないもの（後のフェーズ） + +### ❌ 今回は含めない + +**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 にする + +### ✅ 後のフェーズで検討 + +**Phase 1（次の iteration 以降）：Component 統合の再評価** + +- AuthForm.svelte が十分に「auth 機能特化」か確認 +- login form と signup form のニーズが同じかどうか +- その時点で移行するなら、十分なテストカバレッジがある状態で実施 +- 段階的マイグレーション：リスク評価 → テスト準備 → 移行実行 + +--- + +#### Phase 3: Route handlers の更新 + +**`src/routes/(auth)/login/+page.server.ts`** + +```typescript +// 既存: auth.createSession() 直後にredirect +// 修正: buildLoginPath を import して使用 + +import { buildLoginPath } from '$features/auth/utils/login'; + +// ...existing login logic... +redirect(SEE_OTHER, destination); // dest = buildLoginPath() の結果 +``` + +**`src/routes/(auth)/signup/+page.server.ts`** - 同様 + +**`src/routes/workbooks/create/+page.server.ts`** など + +```typescript +import { ensureSessionOrRedirect } from '$features/auth/services/session'; + +export const load = async ({ locals, url }) => { + await ensureSessionOrRedirect(locals, url); + // ... +}; +``` + +**`src/routes/(admin)/_utils/auth.ts`** - 削除（すべて `$features/auth` に移行） + +--- + +## マイグレーション検査リスト + +- [ ] `src/features/auth/` ディレクトリ作成 +- [ ] `session.ts` を新規作成（既存 authorship.ts から関数を移動） +- [ ] `admin_access.ts` を新規作成（既存 (admin)/\_utils/auth.ts から移動） +- [ ] `login.ts` を新規作成（2 箇所から統合） +- [ ] `src/lib/utils/authorship.ts` を権限判定のみに削減 +- [ ] `src/features/auth/index.ts` でエクスポート +- [ ] Route handlers で新しい import パス に更新 + - `src/routes/(auth)/login/+page.server.ts` + - `src/routes/(auth)/signup/+page.server.ts` + - `src/routes/workbooks/create/+page.server.ts` + - その他保護ルート +- [ ] `src/routes/(admin)/_utils/auth.ts` を削除 +- [ ] 単体テストを `src/features/auth/**/*.test.ts` に移行 + - `authorship.test.ts` から auth 関連を分離 + - `session.test.ts`, `admin_access.test.ts` を新規作成 +- [ ] E2E テストが新しい redirect_after_login フローをカバーしているか確認 +- [ ] 型チェック・linting を実行 + +--- + +## 検証方法 + +1. **ビルド確認** - `pnpm build` で型エラーがないか +2. **テスト確認** - `pnpm test:unit` で既存テストが引き続き pass +3. **E2E確認** - `pnpm test:e2e` で redirect フローが正常に動作 +4. **インポート確認** - grep で旧パスのインポートが残っていないか + +--- + +## 技術負債削減の効果 + +✅ **コード重複の排除** - `buildLoginPath()` の 2 重複を統合 +✅ **責任の明確化** - auth と authorship 権限判定を分離 +✅ **テスト構造の改善** - co-located テストで保守性向上 +✅ **拡張性の向上** - OAuth, 2FA 等の新しい auth 機能追加時に `src/features/auth/` に集約可能 +✅ **AGENTS.md 準拠** - feature-scoped 設計への段階的移行 + +--- + +## 注意点 + +### 破壊リスク最小化 + +- **段階的実行が必須** - すべてを一度に移行しない + - Phase 1: services のみ → テスト → デプロイ + - Phase 2: utils のみ → テスト → デプロイ + - Phase 3: route handlers 更新 → テスト → デプロイ +- **各 phase ごとに git commit** - ロールバック可能にする +- **E2E テストを各段階で実行** - redirect フロー が壊れていないか確認 + +### 実装上の注意 + +- 既存の `src/lib/utils/authorship.ts` は「著者権」関連の関数をコアとしているため、完全には削除しない +- Route handlers が `src/features/auth` を import する際、アクセス権の問題がないか確認（通常は問題なし） +- Lucia auth ライブラリの API は変更しない（`locals.auth.validate()` は継続使用） +- **コンポーネント移行は今回のスコープ外** - Phase 1 で必要性を再評価 diff --git a/e2e/helpers/auth.ts b/e2e/helpers/auth.ts index 60eacfb1d..8b3d0762b 100644 --- a/e2e/helpers/auth.ts +++ b/e2e/helpers/auth.ts @@ -3,6 +3,13 @@ import { expect, type Page } from '@playwright/test'; const TIMEOUT = 60 * 1000; const SHARED_PASSWORD = 'Ch0kuda1'; +/** Fills and submits the signup form with a unique username. */ +export async function submitSignupForm(page: Page): Promise { + await page.locator('input[name="username"]').fill(`testuser_${Date.now()}`); + await page.locator('input[name="password"]').fill('TestPass123'); + await page.getByRole('button', { name: 'アカウントを作成', exact: true }).click(); +} + /** Logs in as the admin user and waits for redirect to home. */ export async function loginAsAdmin(page: Page): Promise { await loginAs(page, 'admin'); @@ -13,11 +20,23 @@ export async function loginAsUser(page: Page): Promise { await loginAs(page, 'guest'); } +/** + * Fills and submits the login form without navigating first. + * Use when already on /login?redirectTo=... to preserve the redirect destination. + */ +export async function submitLoginForm(page: Page, username: 'admin' | 'guest'): Promise { + await fillLoginForm(page, username); +} + async function loginAs(page: Page, username: string): Promise { await page.goto('/login'); await expect(page).toHaveURL('/login', { timeout: TIMEOUT }); + await fillLoginForm(page, username); + await expect(page).toHaveURL('/', { timeout: TIMEOUT }); +} + +async function fillLoginForm(page: Page, username: string): Promise { await page.locator('input[name="username"]').fill(username); await page.locator('input[name="password"]').fill(SHARED_PASSWORD); await page.getByRole('button', { name: 'ログイン', exact: true }).click(); - await expect(page).toHaveURL('/', { timeout: TIMEOUT }); } diff --git a/e2e/redirect_after_login.spec.ts b/e2e/redirect_after_login.spec.ts new file mode 100644 index 000000000..3ab908c75 --- /dev/null +++ b/e2e/redirect_after_login.spec.ts @@ -0,0 +1,203 @@ +import { test, expect, type Page } from '@playwright/test'; +import { submitLoginForm, submitSignupForm } from './helpers/auth'; + +/** + * Opens the submission status dropdown for a given task on the /problems page. + * + * Steps: + * 1. Click the contest table tab (in case it is not the default) + * 2. Click the contest group button identified by contestGroupAriaLabel + * 3. Click the dropdown trigger inside the cell identified by contestId and taskIndex + * + * The dropdown items are not in the DOM until the trigger is clicked (Flowbite renders them lazily). + * + * @param contestGroupAriaLabel - aria-label of the contest group button (e.g. 'Filter contests from ABC 319 onwards') + * @param contestId - contest ID portion of the cell ID (e.g. 'abc422') + * @param taskIndex - task column index (e.g. 'A') + */ +async function openProblemSubmissionDropdown( + page: Page, + contestGroupAriaLabel: string, + contestId: string, + taskIndex: string, +): Promise { + await page.getByRole('tab', { name: 'コンテスト別（アルファ版）' }).click(); + await page.getByLabel(contestGroupAriaLabel).click(); + await page.locator(`#${contestId}-${taskIndex}`).getByLabel('Update submission status').click(); +} + +test.describe('redirect with redirectTo parameter', () => { + test.describe('login', () => { + test.describe('redirectTo preserved in login links', () => { + test.describe('server-side (protected routes)', () => { + test.describe('workbooks', () => { + const workbookRoutes = [ + '/workbooks', + '/workbooks/bfs', + '/workbooks/create', + '/workbooks/edit/bfs', + ]; + + for (const route of workbookRoutes) { + test(`redirects to login for ${route}`, async ({ page }) => { + await page.goto(route); + await expect(page).toHaveURL( + new RegExp(`/login\\?redirectTo=${encodeURIComponent(route)}`), + ); + }); + } + }); + + test.describe('users', () => { + test('edit page redirects without login', async ({ page }) => { + await page.goto('/users/edit'); + await expect(page).toHaveURL(/\/login\?redirectTo=%2Fusers%2Fedit/); + }); + }); + + test.describe('admin', () => { + const adminRoutes = [ + '/account_transfer', + '/tasks', + '/tasks/1', + '/tags', + '/tags/1', + '/vote_management', + '/workbooks/order', + ]; + + for (const route of adminRoutes) { + test(`redirects to login for ${route}`, async ({ page }) => { + await page.goto(route); + await expect(page).toHaveURL( + new RegExp(`/login\\?redirectTo=${encodeURIComponent(route)}`), + ); + }); + } + }); + }); + + test.describe('client-side (action pages)', () => { + test.describe('problems', () => { + test('login dropdown item includes redirectTo parameter', async ({ page }) => { + await page.goto('/problems'); + await openProblemSubmissionDropdown( + page, + 'Filter contests from ABC 319 onwards', + 'abc422', + 'A', + ); + // Href pattern distinguishes dropdown (/login?redirectTo=*) from navbar (/login) + const href = await page.locator('a[href^="/login?redirectTo"]').getAttribute('href'); + expect(href).toMatch(/^\/login\?redirectTo=%2Fproblems/); + }); + }); + + test.describe('votes', () => { + test('login link includes redirectTo parameter', async ({ page }) => { + await page.goto('/votes/abc422_a'); + const href = await page.locator('a[href^="/login?redirectTo"]').getAttribute('href'); + expect(href).toMatch(/^\/login\?redirectTo=%2Fvotes%2Fabc422_a/); + }); + }); + }); + }); + + test.describe('redirects back to original page after login', () => { + // Note: Representative paths only: one user route and one admin route. + // The redirect mechanism is shared, so exhaustive per-route coverage adds no value over + // the redirect-to-login tests above. + test('user can login and be redirected to /workbooks/bfs', async ({ page }) => { + await page.goto('/workbooks/bfs'); + await expect(page).toHaveURL(/\/login\?redirectTo=%2Fworkbooks%2Fbfs/); + await submitLoginForm(page, 'guest'); + await expect(page).toHaveURL('/workbooks/bfs'); + }); + + test('admin can login and be redirected to admin page', async ({ page }) => { + await page.goto('/tags'); + await expect(page).toHaveURL(/\/login\?redirectTo=%2Ftags/); + await submitLoginForm(page, 'admin'); + await expect(page).toHaveURL('/tags'); + }); + }); + }); + + test.describe('signup', () => { + test.describe('redirectTo preserved in signup links', () => { + test.describe('client-side (action pages)', () => { + test.describe('problems', () => { + test('signup dropdown item includes redirectTo parameter', async ({ page }) => { + await page.goto('/problems'); + await openProblemSubmissionDropdown( + page, + 'Filter contests from ABC 319 onwards', + 'abc422', + 'A', + ); + // Href pattern distinguishes dropdown (/signup?redirectTo=*) from navbar (/signup) + const href = await page.locator('a[href^="/signup?redirectTo"]').getAttribute('href'); + expect(href).toMatch(/^\/signup\?redirectTo=%2Fproblems/); + }); + }); + + test.describe('votes', () => { + test('signup link includes redirectTo parameter', async ({ page }) => { + await page.goto('/votes/abc422_a'); + const href = await page.locator('a[href^="/signup?redirectTo"]').getAttribute('href'); + expect(href).toMatch(/^\/signup\?redirectTo=%2Fvotes%2Fabc422_a/); + }); + }); + }); + }); + + test.describe('redirects back to original page after signup', () => { + test('user can signup via login page link and be redirected to /workbooks/bfs', async ({ + page, + }) => { + await page.goto('/workbooks/bfs'); + await expect(page).toHaveURL(/\/login\?redirectTo=%2Fworkbooks%2Fbfs/); + + await page.getByRole('link', { name: 'アカウントを作成' }).click(); + await expect(page).toHaveURL(/\/signup/); + + await submitSignupForm(page); + await expect(page).toHaveURL('/workbooks/bfs', { timeout: 60 * 1000 }); + }); + + test('preserves redirectTo from votes page through signup link', async ({ page }) => { + await page.goto('/votes/abc422_a'); + const href = await page.locator('a[href^="/signup?redirectTo"]').getAttribute('href'); + await page.goto(href!); + await expect(page).toHaveURL( + new RegExp(`/signup\\?redirectTo=${encodeURIComponent('/votes/abc422_a')}`), + ); + + await submitSignupForm(page); + await expect(page).toHaveURL('/votes/abc422_a', { timeout: 60 * 1000 }); + }); + + test('defaults to home page when no redirectTo', async ({ page }) => { + await page.goto('/signup'); + await submitSignupForm(page); + await expect(page).toHaveURL('/', { timeout: 60 * 1000 }); + }); + }); + }); + + test.describe('open redirect prevention', () => { + const maliciousRedirects = [ + { label: 'external domain', redirectTo: 'https://danger.com' }, + { label: 'protocol-relative URL', redirectTo: '%2F%2Fdanger.com' }, + { label: 'javascript: URL', redirectTo: 'javascript%3Aalert()' }, + ]; + + for (const { label, redirectTo } of maliciousRedirects) { + test(`prevents redirect to ${label}`, async ({ page }) => { + await page.goto(`/login?redirectTo=${redirectTo}`); + await submitLoginForm(page, 'guest'); + await expect(page).toHaveURL('/'); + }); + } + }); +}); diff --git a/e2e/votes.spec.ts b/e2e/votes.spec.ts index 6d104ec38..8cf06b4c5 100644 --- a/e2e/votes.spec.ts +++ b/e2e/votes.spec.ts @@ -166,7 +166,7 @@ test.describe('vote detail page (/votes/[slug])', () => { test.describe('vote management page (/vote_management)', () => { test('unauthenticated user is redirected to /login', async ({ page }) => { await page.goto(VOTE_MANAGEMENT_URL); - await expect(page).toHaveURL('/login', { timeout: TIMEOUT }); + await expect(page).toHaveURL(/\/login/, { timeout: TIMEOUT }); }); test('non-admin user is redirected to /', async ({ page }) => { diff --git a/e2e/workbook_order.spec.ts b/e2e/workbook_order.spec.ts index 2e3485e8c..34751385a 100644 --- a/e2e/workbook_order.spec.ts +++ b/e2e/workbook_order.spec.ts @@ -13,7 +13,7 @@ const ORDER_URL = '/workbooks/order'; test.describe('access control', () => { test('unauthenticated user is redirected to /login', async ({ page }) => { await page.goto(ORDER_URL); - await expect(page).toHaveURL('/login', { timeout: TIMEOUT }); + await expect(page).toHaveURL(/\/login/, { timeout: TIMEOUT }); }); }); diff --git a/e2e/workbooks_list.spec.ts b/e2e/workbooks_list.spec.ts index 45de0310b..6a773e7eb 100644 --- a/e2e/workbooks_list.spec.ts +++ b/e2e/workbooks_list.spec.ts @@ -29,7 +29,7 @@ const CATEGORY_SEARCH = 'SEARCH_SIMULATION'; test.describe('access control', () => { test('unauthenticated user is redirected to /login', async ({ page }) => { await page.goto(WORKBOOK_LIST_URL); - await expect(page).toHaveURL('/login', { timeout: TIMEOUT }); + await expect(page).toHaveURL(/\/login/, { timeout: TIMEOUT }); }); }); diff --git a/src/features/auth/services/admin_access.test.ts b/src/features/auth/services/admin_access.test.ts new file mode 100644 index 000000000..d351bfc21 --- /dev/null +++ b/src/features/auth/services/admin_access.test.ts @@ -0,0 +1,123 @@ +import { expect, test, describe, vi, afterEach } from 'vitest'; + +vi.mock('@sveltejs/kit', () => { + const redirectImpl = (status: number, location: string) => { + const err = new Error('Redirect') as Error & { status: number; location: string }; + err.name = 'Redirect'; + err.status = status; + err.location = location; + throw err; + }; + + const errorImpl = (status: number) => { + const err = new Error('HttpError') as Error & { status: number }; + err.name = 'HttpError'; + err.status = status; + throw err; + }; + + return { redirect: vi.fn(redirectImpl), error: vi.fn(errorImpl) }; +}); + +vi.mock('$lib/services/users', () => ({ + getUser: vi.fn(), +})); + +afterEach(() => { + vi.clearAllMocks(); +}); + +import * as userService from '$lib/services/users'; +import { Roles } from '$lib/types/user'; +import { validateAdminAccess, validateAdminAccessForApi } from './admin_access'; + +const createMockLocalsWithSession = (username = 'admin-user') => + ({ + auth: { + validate: vi.fn().mockResolvedValue({ user: { username } }), + }, + }) as unknown as App.Locals; + +const createMockLocalsWithoutSession = () => + ({ + auth: { + validate: vi.fn().mockResolvedValue(null), + }, + }) as unknown as App.Locals; + +describe('validateAdminAccess', () => { + describe('successful cases', () => { + test('does not throw when user is admin', async () => { + const mockLocals = createMockLocalsWithSession(); + vi.mocked(userService.getUser).mockResolvedValue({ role: Roles.ADMIN } as never); + + await expect(validateAdminAccess(mockLocals)).resolves; + }); + }); + + describe('error cases', () => { + test('redirects to /login when no session', async () => { + const mockLocals = createMockLocalsWithoutSession(); + + await expect(validateAdminAccess(mockLocals)).rejects.toMatchObject({ + name: 'Redirect', + status: expect.any(Number), + location: '/login', + }); + }); + + test('redirects to /login when user is not admin', async () => { + const mockLocals = createMockLocalsWithSession(); + vi.mocked(userService.getUser).mockResolvedValue({ role: Roles.USER } as never); + + await expect(validateAdminAccess(mockLocals)).rejects.toMatchObject({ + name: 'Redirect', + status: expect.any(Number), + location: '/login', + }); + }); + + test('redirects to /login?redirectTo=... when no session and url provided', async () => { + const mockLocals = createMockLocalsWithoutSession(); + const mockUrl = new URL('http://localhost:5174/admin/tasks'); + + await expect(validateAdminAccess(mockLocals, mockUrl)).rejects.toMatchObject({ + name: 'Redirect', + status: expect.any(Number), + location: '/login?redirectTo=%2Fadmin%2Ftasks', + }); + }); + }); +}); + +describe('validateAdminAccessForApi', () => { + describe('successful cases', () => { + test('does not throw when user is admin', async () => { + const mockLocals = createMockLocalsWithSession(); + vi.mocked(userService.getUser).mockResolvedValue({ role: Roles.ADMIN } as never); + + await expect(validateAdminAccessForApi(mockLocals)).resolves; + }); + }); + + describe('error cases', () => { + test('throws 401 when no session', async () => { + const mockLocals = createMockLocalsWithoutSession(); + + await expect(validateAdminAccessForApi(mockLocals)).rejects.toMatchObject({ + name: 'HttpError', + status: 401, + }); + }); + + test('throws 403 when user is not admin', async () => { + const mockLocals = createMockLocalsWithSession(); + vi.mocked(userService.getUser).mockResolvedValue({ role: Roles.USER } as never); + + await expect(validateAdminAccessForApi(mockLocals)).rejects.toMatchObject({ + name: 'HttpError', + status: 403, + }); + }); + }); +}); diff --git a/src/routes/(admin)/_utils/auth.ts b/src/features/auth/services/admin_access.ts similarity index 79% rename from src/routes/(admin)/_utils/auth.ts rename to src/features/auth/services/admin_access.ts index 916262bda..7850b6d22 100644 --- a/src/routes/(admin)/_utils/auth.ts +++ b/src/features/auth/services/admin_access.ts @@ -5,14 +5,14 @@ import * as userService from '$lib/services/users'; import { Roles } from '$lib/types/user'; import { isAdmin } from '$lib/utils/authorship'; - -import { LOGIN_PAGE } from '$lib/constants/navbar-links'; import { TEMPORARY_REDIRECT, UNAUTHORIZED, FORBIDDEN, } from '$lib/constants/http-response-status-codes'; +import { buildLoginPath } from '../utils/login'; + enum AdminStatus { OK = 'ok', UNAUTHENTICATED = 'unauthenticated', @@ -22,10 +22,12 @@ enum AdminStatus { /** * Validates that the current session belongs to an admin user. * Redirects to the login page if the session is missing or the user is not an admin. + * @param locals - The application locals containing auth and user information + * @param url - The current URL; when provided, appends ?redirectTo= so the user returns after login */ -export async function validateAdminAccess(locals: App.Locals): Promise { +export async function validateAdminAccess(locals: App.Locals, url?: URL): Promise { if ((await validateAdminStatus(locals)) !== AdminStatus.OK) { - redirect(TEMPORARY_REDIRECT, LOGIN_PAGE); + redirect(TEMPORARY_REDIRECT, buildLoginPath(url)); } } diff --git a/src/features/auth/services/session.test.ts b/src/features/auth/services/session.test.ts new file mode 100644 index 000000000..10b83e407 --- /dev/null +++ b/src/features/auth/services/session.test.ts @@ -0,0 +1,136 @@ +import { expect, test, describe, vi, afterEach } from 'vitest'; + +vi.mock('@sveltejs/kit', () => { + const redirectImpl = (status: number, location: string) => { + const err = new Error('Redirect') as Error & { status: number; location: string }; + err.name = 'Redirect'; + err.status = status; + err.location = location; + throw err; + }; + + return { redirect: vi.fn(redirectImpl) }; +}); + +afterEach(() => { + vi.clearAllMocks(); +}); + +import { ensureSessionOrRedirect, getLoggedInUser } from './session'; + +const createMockLocalsWithValidSession = (user = { id: 'test-user', name: 'Test User' }) => + ({ + auth: { + validate: vi.fn().mockResolvedValue({ user: { id: user.id } }), + }, + user, + }) as unknown as App.Locals; + +const createMockLocalsWithoutSession = () => + ({ + auth: { + validate: vi.fn().mockResolvedValue(null), + }, + }) as unknown as App.Locals; + +const createMockLocalsWithSessionButNoUser = () => + ({ + auth: { + validate: vi.fn().mockResolvedValue({ user: { id: 'test-user' } }), + }, + user: null, + }) as unknown as App.Locals; + +const createMockUrlWorkbooksSlug = (slug = 'bfs') => + new URL(`http://localhost:5174/workbooks/${slug}`); + +describe('ensureSessionOrRedirect', () => { + describe('successful cases', () => { + test('does not throw when user has valid session', async () => { + const mockLocals = createMockLocalsWithValidSession(); + + await expect(ensureSessionOrRedirect(mockLocals)).resolves; + expect(mockLocals.auth.validate).toHaveBeenCalledTimes(1); + }); + }); + + describe('error cases', () => { + test('redirects to /login when no session', async () => { + const mockLocals = createMockLocalsWithoutSession(); + + await expect(ensureSessionOrRedirect(mockLocals)).rejects.toMatchObject({ + name: 'Redirect', + status: expect.any(Number), + location: '/login', + }); + }); + + test('redirects to /login?redirectTo=... when no session exists and url is provided', async () => { + const mockLocals = createMockLocalsWithoutSession(); + const mockUrl = createMockUrlWorkbooksSlug(); + + await expect(ensureSessionOrRedirect(mockLocals, mockUrl)).rejects.toMatchObject({ + name: 'Redirect', + status: expect.any(Number), + location: '/login?redirectTo=%2Fworkbooks%2Fbfs', + }); + }); + }); +}); + +describe('getLoggedInUser', () => { + describe('successful cases', () => { + test('returns user when session and user exist', async () => { + const mockUser = { id: 'test-user', name: 'Test User' }; + const mockLocals = createMockLocalsWithValidSession(mockUser) as unknown as App.Locals; + + const result = await getLoggedInUser(mockLocals); + + expect(result).toEqual(mockUser); + expect(mockLocals.auth.validate).toHaveBeenCalledTimes(1); + }); + + test('returns user when session and user exist (with url param)', async () => { + const mockUser = { id: 'test-user', name: 'Test User' }; + const mockLocals = createMockLocalsWithValidSession(mockUser) as unknown as App.Locals; + const mockUrl = createMockUrlWorkbooksSlug(); + + const result = await getLoggedInUser(mockLocals, mockUrl); + + expect(result).toEqual(mockUser); + }); + }); + + describe('error cases', () => { + test('redirects to /login when no session', async () => { + const mockLocals = createMockLocalsWithoutSession(); + + await expect(getLoggedInUser(mockLocals)).rejects.toMatchObject({ + name: 'Redirect', + status: expect.any(Number), + location: '/login', + }); + }); + + test('redirects to /login when session exists but no user', async () => { + const mockLocals = createMockLocalsWithSessionButNoUser(); + + await expect(getLoggedInUser(mockLocals)).rejects.toMatchObject({ + name: 'Redirect', + status: expect.any(Number), + location: '/login', + }); + }); + + test('redirects to /login?redirectTo=... when no session exists and url is provided', async () => { + const mockLocals = createMockLocalsWithoutSession(); + const mockUrl = createMockUrlWorkbooksSlug(); + + await expect(getLoggedInUser(mockLocals, mockUrl)).rejects.toMatchObject({ + name: 'Redirect', + status: expect.any(Number), + location: '/login?redirectTo=%2Fworkbooks%2Fbfs', + }); + }); + }); +}); diff --git a/src/features/auth/services/session.ts b/src/features/auth/services/session.ts new file mode 100644 index 000000000..ce14c3db2 --- /dev/null +++ b/src/features/auth/services/session.ts @@ -0,0 +1,38 @@ +import { redirect } from '@sveltejs/kit'; + +import { TEMPORARY_REDIRECT } from '$lib/constants/http-response-status-codes'; + +import { buildLoginPath } from '../utils/login'; + +/** + * Get the current logged-in user or redirect to login + * @param locals - The application locals containing auth and user information + * @param url - The current URL; when provided, appends ?redirectTo= so the user returns after login + * @returns The logged-in user or null + */ +export const getLoggedInUser = async ( + locals: App.Locals, + url?: URL, +): Promise => { + await ensureSessionOrRedirect(locals, url); + const loggedInUser = locals.user; + + if (!loggedInUser) { + redirect(TEMPORARY_REDIRECT, buildLoginPath(url)); + } + + return loggedInUser; +}; + +/** + * Ensure user has a valid session or redirect to login + * @param locals - The application locals containing auth and user information + * @param url - The current URL; when provided, appends ?redirectTo= so the user returns after login + */ +export const ensureSessionOrRedirect = async (locals: App.Locals, url?: URL): Promise => { + const session = await locals.auth.validate(); + + if (!session) { + redirect(TEMPORARY_REDIRECT, buildLoginPath(url)); + } +}; diff --git a/src/features/auth/utils/login.test.ts b/src/features/auth/utils/login.test.ts new file mode 100644 index 000000000..e12329cc8 --- /dev/null +++ b/src/features/auth/utils/login.test.ts @@ -0,0 +1,31 @@ +import { expect, test, describe } from 'vitest'; + +import { buildLoginPath } from './login'; + +const createUrl = (path: string) => new URL(`http://localhost:5174${path}`); + +describe('buildLoginPath', () => { + describe('without url', () => { + test('returns base login path', () => { + expect(buildLoginPath()).toBe('/login'); + }); + }); + + describe('with url', () => { + test('appends redirectTo with encoded pathname', () => { + expect(buildLoginPath(createUrl('/workbooks/bfs'))).toBe( + '/login?redirectTo=%2Fworkbooks%2Fbfs', + ); + }); + + test('appends redirectTo including query string', () => { + expect(buildLoginPath(createUrl('/workbooks?tab=solution'))).toBe( + '/login?redirectTo=%2Fworkbooks%3Ftab%3Dsolution', + ); + }); + + test('returns redirectTo for root path', () => { + expect(buildLoginPath(createUrl('/'))).toBe('/login?redirectTo=%2F'); + }); + }); +}); diff --git a/src/features/auth/utils/login.ts b/src/features/auth/utils/login.ts new file mode 100644 index 000000000..192840e32 --- /dev/null +++ b/src/features/auth/utils/login.ts @@ -0,0 +1,15 @@ +import { LOGIN_PAGE } from '$lib/constants/navbar-links'; + +/** + * Builds the login path with an optional redirect parameter. + * @param redirectTo - The decoded path to redirect back to after login; if not provided, returns the base login path + * @returns The login path with optional redirectTo query parameter + */ +export function buildLoginPath(redirectTo?: string | URL | null): string { + if (!redirectTo) { + return LOGIN_PAGE; + } + + const pathname = redirectTo instanceof URL ? redirectTo.pathname + redirectTo.search : redirectTo; + return `${LOGIN_PAGE}?redirectTo=${encodeURIComponent(pathname)}`; +} diff --git a/src/features/auth/utils/signup.test.ts b/src/features/auth/utils/signup.test.ts new file mode 100644 index 000000000..4b031735e --- /dev/null +++ b/src/features/auth/utils/signup.test.ts @@ -0,0 +1,31 @@ +import { expect, test, describe } from 'vitest'; + +import { buildSignupPath } from './signup'; + +describe('buildSignupPath', () => { + describe('without redirectTo', () => { + test('returns base signup path when undefined', () => { + expect(buildSignupPath()).toBe('/signup'); + }); + + test('returns base signup path when null', () => { + expect(buildSignupPath(null)).toBe('/signup'); + }); + }); + + describe('with redirectTo', () => { + test('appends redirectTo with encoded pathname', () => { + expect(buildSignupPath('/workbooks/bfs')).toBe('/signup?redirectTo=%2Fworkbooks%2Fbfs'); + }); + + test('appends redirectTo including query string', () => { + expect(buildSignupPath('/workbooks?tab=solution')).toBe( + '/signup?redirectTo=%2Fworkbooks%3Ftab%3Dsolution', + ); + }); + + test('returns redirectTo for root path', () => { + expect(buildSignupPath('/')).toBe('/signup?redirectTo=%2F'); + }); + }); +}); diff --git a/src/features/auth/utils/signup.ts b/src/features/auth/utils/signup.ts new file mode 100644 index 000000000..acb0682dc --- /dev/null +++ b/src/features/auth/utils/signup.ts @@ -0,0 +1,14 @@ +import { SIGNUP_PAGE } from '$lib/constants/navbar-links'; + +/** + * Builds the signup path with an optional redirect parameter. + * @param redirectTo - The decoded path to redirect back to after signup; if not provided, returns the base signup path + * @returns The signup path with optional redirectTo query parameter + */ +export function buildSignupPath(redirectTo?: string | null): string { + if (!redirectTo) { + return SIGNUP_PAGE; + } + + return `${SIGNUP_PAGE}?redirectTo=${encodeURIComponent(redirectTo)}`; +} diff --git a/src/lib/components/AuthForm.svelte b/src/lib/components/AuthForm.svelte index c0e691eda..b06d72562 100644 --- a/src/lib/components/AuthForm.svelte +++ b/src/lib/components/AuthForm.svelte @@ -28,7 +28,7 @@ submitButtonLabel: string; confirmationMessage: string; alternativePageName: string; - alternativePageLink: '/login' | '/signup'; + alternativePageLink: string; } let { diff --git a/src/lib/components/SubmissionStatus/UpdatingDropdown.svelte b/src/lib/components/SubmissionStatus/UpdatingDropdown.svelte index a6ed99a42..d8e5dfd2b 100644 --- a/src/lib/components/SubmissionStatus/UpdatingDropdown.svelte +++ b/src/lib/components/SubmissionStatus/UpdatingDropdown.svelte @@ -20,7 +20,7 @@ -->
    Safe content
    ', + expected: '<script>alert("XSS")</script>
    Safe content
    ', + }, + { + rawUrl: 'https://.hatenablog.com', + expected: 'https://<script>alert("XSS")</script>.hatenablog.com', + }, + ]; + + runTests('sanitizeUrl', testCases, ({ rawUrl, expected }: TestCaseForUrlSanitization) => { + expect(sanitizeUrl(rawUrl)).toEqual(expected); + }); + }); + + function runTests( + testName: string, + testCases: TestCasesForUrlSanitization, + testFunction: (testCase: TestCaseForUrlSanitization) => void, + ) { + test.each(testCases)(`${testName}(rawUrl: $rawUrl)`, testFunction); + } + }); + + describe('isSameOriginRedirect', () => { + const requestOrigin = 'http://localhost:5174'; + + describe('successful cases (should return true)', () => { + test('accepts relative path without query', () => { + expect(isSameOriginRedirect('/workbooks/bfs', requestOrigin)).toBe(true); + }); + + test('accepts relative path with query string', () => { + expect(isSameOriginRedirect('/workbooks/bfs?tab=foo', requestOrigin)).toBe(true); + }); + + test('accepts root path', () => { + expect(isSameOriginRedirect('/', requestOrigin)).toBe(true); + }); + + test('accepts path with encoded forward slashes (%2F) as path segment', () => { + expect(isSameOriginRedirect('/%2F%2Fexternal.example.com', requestOrigin)).toBe(true); + }); + + test('accepts path with path traversal attempts (normalized by URL parser)', () => { + expect(isSameOriginRedirect('/../../../external.example.com', requestOrigin)).toBe(true); + }); + + test('accepts path with hash fragment', () => { + expect(isSameOriginRedirect('/workbooks#section', requestOrigin)).toBe(true); + }); + }); + + describe('failure cases (should return false)', () => { + test('rejects absolute URL to different origin', () => { + expect(isSameOriginRedirect('https://danger.com', requestOrigin)).toBe(false); + }); + + test('rejects protocol-relative URL (security issue)', () => { + expect(isSameOriginRedirect('//danger.com', requestOrigin)).toBe(false); + }); + + test('rejects URL with userinfo attempt (username@host)', () => { + expect(isSameOriginRedirect('https://app.com@danger.com/path', requestOrigin)).toBe(false); + }); + + test('rejects javascript: URL (opaque origin)', () => { + expect(isSameOriginRedirect('javascript:alert()', requestOrigin)).toBe(false); + }); + + test('rejects data: URL (opaque origin)', () => { + expect(isSameOriginRedirect('data:text/html,', requestOrigin)).toBe( + false, + ); + }); + + test('rejects absolute URL with different protocol', () => { + expect(isSameOriginRedirect('https://localhost:5174', 'http://localhost:5174')).toBe(false); + }); + + test('rejects absolute URL with different port', () => { + expect(isSameOriginRedirect('http://localhost:5175', requestOrigin)).toBe(false); + }); + }); + }); +}); diff --git a/src/lib/utils/url.ts b/src/lib/utils/url.ts index ec76453ad..d01476537 100644 --- a/src/lib/utils/url.ts +++ b/src/lib/utils/url.ts @@ -44,3 +44,19 @@ export function isValidUrlSlug(slug: string): boolean { export function sanitizeUrl(rawUrl: string) { return xss(rawUrl); } + +/** + * Validates if a redirect target is safe (same origin) to prevent open redirect vulnerabilities. + * + * @param redirectTarget - The target URL to redirect to (relative or absolute) + * @param requestOrigin - The origin of the current request (e.g., 'http://localhost:5174') + * @returns A boolean indicating whether the redirect target is same-origin as the request + */ +export function isSameOriginRedirect(redirectTarget: string, requestOrigin: string): boolean { + try { + const targetUrl = new URL(redirectTarget, requestOrigin); + return targetUrl.origin === requestOrigin; + } catch { + return false; + } +} diff --git a/src/routes/(admin)/account_transfer/+page.server.ts b/src/routes/(admin)/account_transfer/+page.server.ts index 5063dff8f..a7a9c267d 100644 --- a/src/routes/(admin)/account_transfer/+page.server.ts +++ b/src/routes/(admin)/account_transfer/+page.server.ts @@ -1,33 +1,20 @@ // See: // https://superforms.rocks/get-started -import { redirect, type Actions } from '@sveltejs/kit'; +import { type Actions } from '@sveltejs/kit'; import { superValidate } from 'sveltekit-superforms/server'; import { zod4 } from 'sveltekit-superforms/adapters'; -import * as userService from '$lib/services/users'; import * as taskResultService from '$lib/services/task_results'; import { accountTransferSchema } from '$lib/zod/schema'; import type { FloatingMessage } from '$lib/types/floating_message'; -import { TEMPORARY_REDIRECT } from '$lib/constants/http-response-status-codes'; -import { LOGIN_PAGE } from '$lib/constants/navbar-links'; -import { Roles } from '$lib/types/user'; +import { validateAdminAccess } from '$features/auth/services/admin_access'; let accountTransferMessages: FloatingMessage[] = []; -export async function load({ locals }) { - const session = await locals.auth.validate(); - - if (!session) { - throw redirect(TEMPORARY_REDIRECT, LOGIN_PAGE); - } - - const user = await userService.getUser(session?.user.username as string); - - if (user && user.role !== Roles.ADMIN) { - throw redirect(TEMPORARY_REDIRECT, LOGIN_PAGE); - } +export async function load({ locals, url }) { + await validateAdminAccess(locals, url); const form = await superValidate(null, zod4(accountTransferSchema)); diff --git a/src/routes/(admin)/tags/+page.server.ts b/src/routes/(admin)/tags/+page.server.ts index c119c71df..b720872ce 100644 --- a/src/routes/(admin)/tags/+page.server.ts +++ b/src/routes/(admin)/tags/+page.server.ts @@ -3,22 +3,11 @@ import { redirect, type Actions } from '@sveltejs/kit'; import type { Tag } from '$lib/types/tag'; import * as tagService from '$lib/services/tags'; -import * as userService from '$lib/services/users'; -//import { sha256 } from '$lib/utils/hash'; +import { validateAdminAccess } from '$features/auth/services/admin_access'; -import { Roles } from '$lib/types/user'; - -export async function load({ locals }) { - const session = await locals.auth.validate(); - if (!session) { - redirect(302, '/login'); - } - - const user = await userService.getUser(session?.user.username as string); - if (user?.role !== Roles.ADMIN) { - redirect(302, '/login'); - } +export async function load({ locals, url }) { + await validateAdminAccess(locals, url); const tags = await tagService.getTags(); diff --git a/src/routes/(admin)/tags/[tag_id]/+page.server.ts b/src/routes/(admin)/tags/[tag_id]/+page.server.ts index 51d2371f8..e83c37ed1 100644 --- a/src/routes/(admin)/tags/[tag_id]/+page.server.ts +++ b/src/routes/(admin)/tags/[tag_id]/+page.server.ts @@ -1,35 +1,19 @@ -import { redirect } from '@sveltejs/kit'; +import type { Tasks } from '$lib/types/task'; +import type { Tags } from '$lib/types/tag'; -//import type { Roles } from '$lib/types/user'; -import type { Tag } from '$lib/types/tag'; -import type { Task } from '$lib/types/task'; import * as tagService from '$lib/services/tags'; import * as taskTagService from '$lib/services/task_tags'; -import * as userService from '$lib/services/users'; -import { Roles } from '$lib/types/user'; -export async function load({ locals, params }) { - const session = await locals.auth.validate(); - if (!session) { - redirect(302, '/login'); - } +import { validateAdminAccess } from '$features/auth/services/admin_access'; - const user = await userService.getUser(session?.user.username as string); - if (user?.role !== Roles.ADMIN) { - redirect(302, '/login'); - } - const tags: Tag[] = await tagService.getTag(params.tag_id as string); - const tasks: Task[] = await taskTagService.getTasks(params.tag_id as string); +export async function load({ locals, params, url }) { + await validateAdminAccess(locals, url); - //Jsonから、必要なtag - - console.log(tags[0]); - console.log(user.role); - console.log(session?.user.role); + const tasks: Tasks = await taskTagService.getTasks(params.tag_id as string); + const tags: Tags = await tagService.getTag(params.tag_id as string); return { - tag: tags[0], tasks: tasks, - isAdmin: user?.role !== Roles.ADMIN, + tag: tags[0], }; } diff --git a/src/routes/(admin)/tasks/+page.server.ts b/src/routes/(admin)/tasks/+page.server.ts index e6c582bd5..aaf4ccee5 100644 --- a/src/routes/(admin)/tasks/+page.server.ts +++ b/src/routes/(admin)/tasks/+page.server.ts @@ -1,6 +1,5 @@ import { redirect, type Actions } from '@sveltejs/kit'; -import { Roles } from '$lib/types/user'; import type { Contests, ContestForImport, ContestsForImport } from '$lib/types/contest'; import { type Task, @@ -11,19 +10,15 @@ import { getTaskGrade, } from '$lib/types/task'; -import * as taskService from '$lib/services/tasks'; -import * as userService from '$lib/services/users'; import * as apiClient from '$lib/clients'; +import * as taskService from '$lib/services/tasks'; +import { validateAdminAccess } from '$features/auth/services/admin_access'; -import { isAdmin } from '$lib/utils/authorship'; -import { sha256 } from '$lib/utils/hash'; import { classifyContest } from '$lib/utils/contest'; +import { sha256 } from '$lib/utils/hash'; -import { LOGIN_PAGE } from '$lib/constants/navbar-links'; -import { TEMPORARY_REDIRECT } from '$lib/constants/http-response-status-codes.js'; - -export async function load({ locals }) { - await validateAdminAccess(locals); +export async function load({ locals, url }) { + await validateAdminAccess(locals, url); const { contestsForImport, tasksForImport } = await fetchContestsAndTasksFromAPI(); @@ -45,20 +40,6 @@ export async function load({ locals }) { }; } -async function validateAdminAccess(locals: App.Locals): Promise { - const session = await locals.auth.validate(); - - if (!session) { - redirect(TEMPORARY_REDIRECT, LOGIN_PAGE); - } - - const user = await userService.getUser(session?.user.username as string); - - if (!isAdmin(user?.role as Roles)) { - redirect(TEMPORARY_REDIRECT, LOGIN_PAGE); - } -} - async function fetchContestsAndTasksFromAPI(): Promise<{ contestsForImport: ContestsForImport; tasksForImport: TasksForImport; diff --git a/src/routes/(admin)/tasks/[task_id]/+page.server.ts b/src/routes/(admin)/tasks/[task_id]/+page.server.ts index 319230dbf..70bf0bbe4 100644 --- a/src/routes/(admin)/tasks/[task_id]/+page.server.ts +++ b/src/routes/(admin)/tasks/[task_id]/+page.server.ts @@ -1,33 +1,18 @@ -import { redirect } from '@sveltejs/kit'; +import type { Tasks } from '$lib/types/task'; +import type { Tag } from '$lib/types/tag'; +import type { ImportTaskTag } from '$lib/types/tasktag'; -//import type { Roles } from '$lib/types/user'; -import type { Task } from '$lib/types/task'; import * as taskService from '$lib/services/tasks'; -import * as userService from '$lib/services/users'; -//import * as tagService from '$lib/services/tags'; -import { Roles } from '$lib/types/user'; -import type { ImportTaskTag } from '$lib/types/tasktag'; -import type { Tag } from '$lib/types/tag'; import * as taskTagsApiService from '$lib/services/tasktagsApiService'; import * as taskTagsService from '$lib/services/task_tags'; -export async function load({ locals, params }) { - const session = await locals.auth.validate(); - if (!session) { - redirect(302, '/login'); - } +import { validateAdminAccess } from '$features/auth/services/admin_access'; - const user = await userService.getUser(session?.user.username as string); - if (user?.role !== Roles.ADMIN) { - redirect(302, '/login'); - } - const task: Task[] = await taskService.getTask(params.task_id as string); +export async function load({ locals, params, url }) { + await validateAdminAccess(locals, url); - //console.log(task); - //console.log(user.role); - //console.log(session?.user.role); + const task: Tasks = await taskService.getTask(params.task_id as string); - //jsonデータから必要なTask情報を取り出す。 const importTagsJson = await taskTagsApiService.getTaskTags(); const taskTagsJson = importTagsJson[0].data.filter( (taskTag: ImportTaskTag) => taskTag.task_id === params.task_id, @@ -38,11 +23,8 @@ export async function load({ locals, params }) { taskTags.map(async (tag: Tag) => { tagMap.set(tag.name, tag); - //console.log(tag.name, tag) }); - //console.log(taskTags) - if (taskTagsJson.length > 0) { const importTags: string[] = taskTagsJson[0].tags; @@ -57,15 +39,10 @@ export async function load({ locals, params }) { tagMap.set(importTags[i], tmpTag); } } - //console.log(importTags) } - //console.log(tagMap) - //console.log(tagMap.values()) - return { task: task[0], tags: Array.from(tagMap.values()), - isAdmin: user?.role !== Roles.ADMIN, }; } diff --git a/src/routes/(admin)/vote_management/+page.server.ts b/src/routes/(admin)/vote_management/+page.server.ts index a610f4d56..f36e13721 100644 --- a/src/routes/(admin)/vote_management/+page.server.ts +++ b/src/routes/(admin)/vote_management/+page.server.ts @@ -1,15 +1,16 @@ import type { Actions, PageServerLoad } from './$types'; import { type TaskGrade, TaskGrade as TaskGradeEnum } from '$lib/types/task'; + import { getTasksByTaskId, updateTask } from '$lib/services/tasks'; import { getAllVoteStatisticsAsArray, getAllVoteCounters, } from '$features/votes/services/vote_statistics'; -import { validateAdminAccess } from '../_utils/auth'; +import { validateAdminAccess } from '$features/auth/services/admin_access'; -export const load: PageServerLoad = async ({ locals }) => { - await validateAdminAccess(locals); +export const load: PageServerLoad = async ({ locals, url }) => { + await validateAdminAccess(locals, url); const [allStats, tasksMap, allCounters] = await Promise.all([ getAllVoteStatisticsAsArray(), diff --git a/src/routes/(admin)/workbooks/order/+page.server.ts b/src/routes/(admin)/workbooks/order/+page.server.ts index df4010a8a..a9a0c0190 100644 --- a/src/routes/(admin)/workbooks/order/+page.server.ts +++ b/src/routes/(admin)/workbooks/order/+page.server.ts @@ -5,10 +5,10 @@ import { createInitialPlacements, } from '$features/workbooks/services/workbook_placements/crud'; -import { validateAdminAccess } from '../../_utils/auth'; +import { validateAdminAccess } from '$features/auth/services/admin_access'; -export async function load({ locals }) { - await validateAdminAccess(locals); +export async function load({ locals, url }) { + await validateAdminAccess(locals, url); const workbooks = await getWorkbooksWithPlacements(); const hasUnplacedWorkbooks = workbooks.some((workbook) => !workbook.placement); diff --git a/src/routes/(admin)/workbooks/order/+server.ts b/src/routes/(admin)/workbooks/order/+server.ts index 4e2549d4f..14f23e328 100644 --- a/src/routes/(admin)/workbooks/order/+server.ts +++ b/src/routes/(admin)/workbooks/order/+server.ts @@ -4,7 +4,7 @@ import { validateAndUpdatePlacements } from '$features/workbooks/services/workbo import { updatePlacementsSchema } from '$features/workbooks/zod/schema'; -import { validateAdminAccessForApi } from '../../_utils/auth'; +import { validateAdminAccessForApi } from '$features/auth/services/admin_access'; import { BAD_REQUEST } from '$lib/constants/http-response-status-codes'; diff --git a/src/routes/(auth)/login/+page.server.ts b/src/routes/(auth)/login/+page.server.ts index b48892d11..88ae77c45 100644 --- a/src/routes/(auth)/login/+page.server.ts +++ b/src/routes/(auth)/login/+page.server.ts @@ -6,9 +6,11 @@ import { fail, redirect } from '@sveltejs/kit'; import { LuciaError } from 'lucia'; -import { initializeAuthForm, validateAuthFormWithFallback } from '$lib/utils/auth_forms'; import { auth } from '$lib/server/auth'; +import { initializeAuthForm, validateAuthFormWithFallback } from '$lib/utils/auth_forms'; +import { isSameOriginRedirect } from '$lib/utils/url'; + import { BAD_REQUEST, SEE_OTHER, @@ -23,7 +25,7 @@ export const load: PageServerLoad = async ({ locals }) => { }; export const actions: Actions = { - default: async ({ request, locals }) => { + default: async ({ request, locals, url }) => { const form = await validateAuthFormWithFallback(request); if (!form.valid) { @@ -73,8 +75,15 @@ export const actions: Actions = { }); } + const redirectTo = url.searchParams.get('redirectTo'); + let destination = HOME_PAGE; + + if (redirectTo && isSameOriginRedirect(redirectTo, url.origin)) { + destination = redirectTo; + } + // redirect to // make sure you don't throw inside a try/catch block! - redirect(SEE_OTHER, HOME_PAGE); + redirect(SEE_OTHER, destination); }, }; diff --git a/src/routes/(auth)/login/+page.svelte b/src/routes/(auth)/login/+page.svelte index d0195bc54..77e6c7593 100644 --- a/src/routes/(auth)/login/+page.svelte +++ b/src/routes/(auth)/login/+page.svelte @@ -1,8 +1,12 @@ diff --git a/src/routes/(auth)/signup/+page.server.ts b/src/routes/(auth)/signup/+page.server.ts index 9f8b4feb9..ac0c0415b 100644 --- a/src/routes/(auth)/signup/+page.server.ts +++ b/src/routes/(auth)/signup/+page.server.ts @@ -7,9 +7,11 @@ import { fail, redirect } from '@sveltejs/kit'; import { PrismaClientKnownRequestError } from '@prisma/client/runtime/library'; import { LuciaError } from 'lucia'; -import { initializeAuthForm, validateAuthFormWithFallback } from '$lib/utils/auth_forms'; import { auth } from '$lib/server/auth'; +import { initializeAuthForm, validateAuthFormWithFallback } from '$lib/utils/auth_forms'; +import { isSameOriginRedirect } from '$lib/utils/url'; + import { SEE_OTHER, BAD_REQUEST, @@ -25,7 +27,7 @@ export const load: PageServerLoad = async ({ locals }) => { // FIXME: エラー処理に共通部分があるため、リファクタリングをしましょう。 export const actions: Actions = { - default: async ({ request, locals }) => { + default: async ({ request, locals, url }) => { const form = await validateAuthFormWithFallback(request); if (!form.valid) { @@ -84,8 +86,15 @@ export const actions: Actions = { }); } + const redirectTo = url.searchParams.get('redirectTo'); + let destination = HOME_PAGE; + + if (redirectTo && isSameOriginRedirect(redirectTo, url.origin)) { + destination = redirectTo; + } + // redirect to // make sure you don't throw inside a try/catch block! - return redirect(SEE_OTHER, HOME_PAGE); + return redirect(SEE_OTHER, destination); }, }; diff --git a/src/routes/problems/[slug]/+page.server.ts b/src/routes/problems/[slug]/+page.server.ts index 6f7e3b7d3..43ec7c1c4 100644 --- a/src/routes/problems/[slug]/+page.server.ts +++ b/src/routes/problems/[slug]/+page.server.ts @@ -1,30 +1,27 @@ -import { fail, type Actions } from '@sveltejs/kit'; -import * as crud from '$lib/services/task_results'; -import { BAD_REQUEST, TEMPORARY_REDIRECT } from '$lib/constants/http-response-status-codes'; -import { redirect } from '@sveltejs/kit'; +import { fail, type Actions, redirect } from '@sveltejs/kit'; +import * as crud from '$lib/services/task_results'; import { getButtons } from '$lib/services/submission_status'; +import { getLoggedInUser } from '$features/auth/services/session'; -const buttons = await getButtons(); +import { BAD_REQUEST, TEMPORARY_REDIRECT } from '$lib/constants/http-response-status-codes'; -export async function load({ locals, params }) { - const session = await locals.auth.validate(); - if (!session) { - redirect(302, '/login'); - } +const buttons = await getButtons(); - const taskResult = await crud.getTaskResult(params.slug as string, session?.user.userId); +export async function load({ locals, params, url }) { + const loggedInUser = await getLoggedInUser(locals, url); + const taskResult = await crud.getTaskResult(params.slug as string, loggedInUser!.id); return { taskResult: taskResult, buttons: buttons }; } export const actions = { - default: async ({ request, params, locals }) => { + default: async ({ request, params, locals, url }) => { const response = await request.formData(); const slug = params.slug as string; - const session = await locals.auth.validate(); - const userId = session?.user.userId; + const loggedInUser = await getLoggedInUser(locals, url); + const userId = loggedInUser!.id; try { const submissionStatus = response.get('submissionStatus') as string; diff --git a/src/routes/users/[username]/+page.server.ts b/src/routes/users/[username]/+page.server.ts index cfbc98b3f..76daebfbc 100644 --- a/src/routes/users/[username]/+page.server.ts +++ b/src/routes/users/[username]/+page.server.ts @@ -1,20 +1,15 @@ //See https://tech-blog.rakus.co.jp/entry/20230209/sveltekit#%E3%82%B9%E3%83%AC%E3%83%83%E3%83%89%E6%8A%95%E7%A8%BF%E7%94%BB%E9%9D%A2 -import { redirect } from '@sveltejs/kit'; - import * as userService from '$lib/services/users'; import * as taskResultService from '$lib/services/task_results'; import type { Roles } from '$lib/types/user'; import type { TaskResult } from '$lib/types/task'; -import { TEMPORARY_REDIRECT } from '$lib/constants/http-response-status-codes'; +import { getLoggedInUser } from '$features/auth/services/session'; -export async function load({ locals, params }) { - const session = await locals.auth.validate(); - if (!session) { - redirect(TEMPORARY_REDIRECT, '/login'); - } +export async function load({ locals, params, url }) { + const loggedInUser = await getLoggedInUser(locals, url); try { const user = await userService.getUser(params.username as string); @@ -43,11 +38,11 @@ export async function load({ locals, params }) { username: user.username, atcoder_username: user.atCoderAccount?.handle ?? '', role: user.role as Roles, - isLoggedIn: session?.user.userId === user.id, + isLoggedIn: loggedInUser?.id === user.id, taskResults: taskResults, }; } catch (e) { - console.error('Failed to load user or taskResults: ', session?.user.username); + console.error('Failed to load user or taskResults: ', loggedInUser?.name); console.error(e); //500を投げたい //throw redirect(302, '/login'); diff --git a/src/routes/users/edit/+page.server.ts b/src/routes/users/edit/+page.server.ts index 77e2f44a9..8d6e5cead 100644 --- a/src/routes/users/edit/+page.server.ts +++ b/src/routes/users/edit/+page.server.ts @@ -7,28 +7,28 @@ import type { Roles } from '$lib/types/user'; import * as userService from '$lib/services/users'; import * as verificationService from '$features/account/services/atcoder_verification'; +import { ensureSessionOrRedirect } from '$features/auth/services/session'; +import { buildLoginPath } from '$features/auth/utils/login'; + import { BAD_REQUEST, UNAUTHORIZED, FORBIDDEN, INTERNAL_SERVER_ERROR, + TEMPORARY_REDIRECT, } from '$lib/constants/http-response-status-codes'; export async function load({ locals, url }) { - const session = await locals.auth.validate(); - - if (!session) { - redirect(302, '/login'); - } + await ensureSessionOrRedirect(locals, url); try { - const user = await userService.getUser(session?.user.username as string); + const user = await userService.getUser(locals.user.name); return { userId: user?.id as string, username: user?.username as string, role: user?.role as Roles, - isLoggedIn: (session?.user.userId === user?.id) as boolean, + isLoggedIn: (locals.user.id === user?.id) as boolean, atCoderAccount: { handle: user?.atCoderAccount?.handle ?? '', validationCode: user?.atCoderAccount?.validationCode ?? '', @@ -40,7 +40,7 @@ export async function load({ locals, url }) { }; } catch (error) { console.error('User lookup failed during session validation', error); - redirect(302, '/login'); + redirect(TEMPORARY_REDIRECT, buildLoginPath(url)); } } diff --git a/src/routes/votes/[slug]/+page.svelte b/src/routes/votes/[slug]/+page.svelte index 021bfa3d9..c2804013e 100644 --- a/src/routes/votes/[slug]/+page.svelte +++ b/src/routes/votes/[slug]/+page.svelte @@ -1,6 +1,7 @@ `（opaque origin `"null"`） -- `not-a-url!!!`（不正な文字列、`new URL` がthrowし `false`） - -### Phase 2: `isSameOriginRedirect` 実装 - -- `src/lib/utils/url.ts` に `isSameOriginRedirect` を追加 -- `pnpm test:unit` でPhase 1のテストがGREENになることを確認 - -### Phase 3: 認証ヘルパーのテスト（先に書く） - -`src/lib/utils/authorship.test.ts`（既存があれば追記）に追加。`locals.auth` は `vi.mock` でモック。 - -- `url` を渡した場合 → `/login?redirectTo=%2Fworkbooks%2Fbfs` へリダイレクトすること -- `url` を渡さない場合 → `/login` へリダイレクトすること（既存動作の維持確認） - -### Phase 4: 認証ヘルパー実装 - -- `src/lib/utils/authorship.ts` の `ensureSessionOrRedirect` / `getLoggedInUser` に `url?: URL` を追加 -- `src/routes/(admin)/_utils/auth.ts` の `validateAdminAccess` にも同様の修正 -- `pnpm test:unit` でGREENを確認 - -### Phase 5: ログイン・登録アクションのテスト（先に書く） - -`src/routes/(auth)/login/` および `signup/` に隣接するテストファイルに追加。 - -- 有効な `redirectTo`（`/workbooks/bfs`）→ ログイン後そのパスへリダイレクト -- 無効な `redirectTo`（`https://danger.com`）→ ホーム (`/`) へリダイレクト -- `redirectTo` なし → ホームへリダイレクト - -### Phase 6: ログイン・登録アクション実装 - -- `src/routes/(auth)/login/+page.server.ts` で `redirectTo` を読み取り・検証・リダイレクト -- `src/routes/(auth)/signup/+page.server.ts` で同じ処理を実装 -- `pnpm test:unit` でGREENを確認 - -### Phase 7: E2E テスト（先に書く・Playwright） - -Phase 6完了時点でログイン機構が `redirectTo` を処理できる状態になる。 -ここでE2Eテストを書いて **RED** にし、Phase 8・9の実装で **GREEN** にする。 - -`e2e/` 以下に新規テストファイルを追加。 - -**各ルートの `redirectTo` 付与確認（Phase 8・9 で変更する全ルート）** - -未ログイン状態でアクセスしたとき、`/login?redirectTo=<そのパス>` にリダイレクトされることを確認する。 -ログイン後に元のページへ戻ることを確認するのは代表ルートのみで十分（機構は共通のため）。 - -| ルート | 確認内容 | -| ------------------------- | --------------------------------------------------------------------------------------------- | -| `/workbooks` | `?redirectTo=%2Fworkbooks` が付与される | -| `/workbooks/` | `?redirectTo=%2Fworkbooks%2F` が付与される ★ログイン後の復帰もここで確認 | -| `/workbooks/create` | `?redirectTo=%2Fworkbooks%2Fcreate` が付与される | -| `/problems/` | `?redirectTo=...` が付与される | -| `/users/edit` | `?redirectTo=%2Fusers%2Fedit` が付与される | -| `/users/` | `?redirectTo=...` が付与される | -| `/votes/` | ログイン・アカウント作成ボタンをクリックした際のリンク href に `?redirectTo=...` が付与される | -| `/admin/account_transfer` | `?redirectTo=...` が付与される（管理者ユーザーでログイン後に復帰も確認） | -| `/admin/tasks` | `?redirectTo=...` が付与される | -| `/admin/tasks/` | `?redirectTo=...` が付与される | -| `/admin/tags` | `?redirectTo=...` が付与される | -| `/admin/tags/` | `?redirectTo=...` が付与される | - -**登録フロー（一般ユーザーの典型的なユースケース）：** - -- 未ログイン → `/workbooks/` → ログインページ → 登録ページへ遷移した場合 `redirectTo` が引き継がれること -- 登録成功後 → `/workbooks/` に戻ること - -**votes/[slug] のログインボタン経由フロー：** - -`/votes/[slug]` はページ自体は公開だが、ページ内のログイン・登録リンクに `redirectTo` が付与される。 - -- 未ログイン状態で `/votes/` を表示 → ログインリンクのhrefに `?redirectTo=%2Fvotes%2F` が含まれること -- 未ログイン状態で `/votes/` を表示 → アカウント作成リンクのhrefに `?redirectTo=%2Fvotes%2F` が含まれること -- ログインリンクからログイン → ログイン後 `/votes/` に戻ること -- 登録リンクから登録ページへ遷移した場合も `redirectTo` が引き継がれ、登録後 `/votes/` に戻ること - -※「ログイン・アカウント作成ボタンが表示されること」自体はvotes機能のテストの責務。このプランではリンクの `href` に `redirectTo` が正しく付与されているかのみを検証する。 - -**open redirect 防止（不正な `redirectTo`）：** - -- `?redirectTo=https://danger.com` → ログイン後はホーム (`/`) に遷移すること -- `?redirectTo=//danger.com` → 同上 -- `?redirectTo=javascript:alert()` → 同上 - -### Phase 8: ロード関数の一括更新 - -`getLoggedInUser(locals)` → `getLoggedInUser(locals, url)` に変更。 -対象ルート（`load` 関数のシグネチャに `url` を追加する必要あり）： - -| ファイル | 現在の呼び出し | -| --------------------------------------------- | ------------------------- | -| `src/routes/workbooks/[slug]/+page.server.ts` | `getLoggedInUser(locals)` | -| その他 `ensureSessionOrRedirect` 呼び出し箇所 | 同様 | - -- `pnpm test:e2e` で対象ルートのテストがGREENになることを確認 - -### Phase 8.5: 既存E2Eテストの修正 - -Phase 8実装後、`/login` への完全一致アサーションが `/login?redirectTo=...` にマッチしなくなるため修正が必要。 - -| ファイル | 行 | 変更内容 | -| ---------------------------- | --- | ---------------------------------------------- | -| `e2e/votes.spec.ts` | 169 | `toHaveURL('/login')` → `toHaveURL(/\/login/)` | -| `e2e/workbook_order.spec.ts` | 16 | `toHaveURL('/login')` → `toHaveURL(/\/login/)` | -| `e2e/workbooks_list.spec.ts` | 32 | `toHaveURL('/login')` → `toHaveURL(/\/login/)` | - -影響なし（変更不要）： - -- `e2e/user_edit.spec.ts:13` — すでに `toHaveURL(/\/login/)` を使用 -- `e2e/helpers/auth.ts:22` — `/login` を直接 `goto` するため `redirectTo` が付かない → `/` アサーションはそのまま正しい - -### Phase 9: 直接 `redirect('/login')` の統一 - -以下のファイルは `ensureSessionOrRedirect` を使わず直接 `redirect(30x, '/login')` を呼び出している。ヘルパー経由に統一する： - -| ファイル | 件数 | -| ----------------------------------------------------- | ---- | -| `src/routes/problems/[slug]/+page.server.ts` | 1 | -| `src/routes/users/edit/+page.server.ts` | 2 | -| `src/routes/users/[username]/+page.server.ts` | 2 | -| `src/routes/workbooks/create/+page.server.ts` | 1 | -| `src/routes/(admin)/account_transfer/+page.server.ts` | 2 | -| `src/routes/(admin)/tasks/+page.server.ts` | 2 | -| `src/routes/(admin)/tasks/[task_id]/+page.server.ts` | 2 | -| `src/routes/(admin)/tags/+page.server.ts` | 2 | -| `src/routes/(admin)/tags/[tag_id]/+page.server.ts` | 2 | - -- `pnpm test:e2e` で全ルートのテストがGREENになることを確認 - -### Phase 10: auth.md 更新 - -`.claude/rules/auth.md` に「Redirect After Login Pattern」セクションを追加し、今後の実装が一貫した書き方になるよう文書化。 - -### Phase 11: ルール・ドキュメント追加 - -今回の実装で得られた、既存ルールに存在しない知見を英語で簡潔に追記する。 - -**1. `auth.md`（またはルート `security.md` 新規作成）: Open Redirect 防止パターン** - -- `startsWith('/')` だけでは `//danger.com`（プロトコル相対URL）を突破されるため不十分 -- 正しい検証は `new URL(redirectTarget, origin).origin === origin` -- 適用範囲：ユーザー入力のURLを受け取って `redirect()` する箇所すべて - -**2. `sveltekit.md`: `redirect()` は origin 検証を行わない** - -- SvelteKit の `redirect()` は宛先URLの origin を検証しない（フレームワーク側に保護なし） -- ユーザー由来の redirect 先を使う場合は必ず開発者側で検証する - -**3. `testing-e2e.md`: インフラ変更時の E2E テスト配置** - -- 複数ルートへ段階的に展開するインフラ変更では、「機構が動く Phase の後・全ルート適用 Phase の前」に E2E を書いて RED にする -- 機構が整っていない状態で E2E を書いても通らないため、通常のユニットテストより一段遅らせるのが正しい順序 - ---- - -## 影響ファイル一覧 - -**修正（既存）:** - -- `src/lib/utils/authorship.ts` -- `src/routes/(admin)/_utils/auth.ts` -- `src/routes/(admin)/account_transfer/+page.server.ts` -- `src/routes/(admin)/tasks/+page.server.ts` -- `src/routes/(auth)/login/+page.server.ts` -- `src/routes/(auth)/signup/+page.server.ts` -- `src/routes/workbooks/[slug]/+page.server.ts` -- `src/routes/workbooks/create/+page.server.ts` -- `src/routes/problems/[slug]/+page.server.ts` -- `src/routes/users/edit/+page.server.ts` -- `src/routes/users/[username]/+page.server.ts` -- `src/routes/(admin)/tasks/[task_id]/+page.server.ts` -- `src/routes/(admin)/tags/+page.server.ts` -- `src/routes/(admin)/tags/[tag_id]/+page.server.ts` -- `.claude/rules/auth.md` - -**追加（新規）:** - -- `src/lib/utils/url.test.ts`（`isSameOriginRedirect` のユニットテスト） -- `e2e/redirect_after_login.spec.ts`（ログイン・登録後リダイレクトのE2Eテスト） - -**修正（既存E2E）:** - -- `e2e/votes.spec.ts`（`/login` 完全一致 → 正規表現に変更） -- `e2e/workbook_order.spec.ts`（同上） -- `e2e/workbooks_list.spec.ts`（同上） - ---- - -## 却下した代替案 - -### URLクエリパラメータ以外のアプローチ - -| 案 | 却下理由 | -| ------------------- | ------------------------------------------------------------------------------------------ | -| **短命Cookie** | 「ストレージへの保存を避けたい」というプロジェクト方針に反する。管理コストも増大 | -| **Refererヘッダー** | ブラウザによって送信されないケースが多く信頼性が低い。ブックマーク・直リンクでは機能しない | - -### `startsWith('/')` のみの検証 - -`//danger.com` はプロトコル相対URLとしてブラウザが `https://danger.com` に解釈するため、この検証では open redirect を防げない。標準 `URL` API による origin 比較が必要。 diff --git a/docs/dev-notes/2026-05-05/auth-feature-consolidation/plan.md b/docs/dev-notes/2026-05-05/auth-feature-consolidation/plan.md deleted file mode 100644 index 5571b907a..000000000 --- a/docs/dev-notes/2026-05-05/auth-feature-consolidation/plan.md +++ /dev/null @@ -1,278 +0,0 @@ -# Auth Feature 統合計画 - -## 概要 - -現在、認証関連のロジックが散在している状態を、`src/features/auth/` への feature-scoped 設計に統合する。 - -**目的：** - -- 認証関連のコード（services, utils, types）を一箇所に集約 -- AGENTS.md の feature-scoped 設計に準拠 -- テストの co-location を実現 -- 技術負債（分散したコード）の解消 - -**現状の問題：** - -- `src/lib/utils/authorship.ts` - login 関連の関数と authorship 権限判定が混在 -- `src/routes/(admin)/_utils/auth.ts` - admin 専用の auth ロジック -- `buildLoginPath()` - 2 箇所で重複実装 -- login/signup ロジックは route handlers に直接記述 - ---- - -## 設計方針 - -### 新規作成: `src/features/auth/` - -``` -src/features/auth/ -├── services/ -│ ├── login.ts # ログイン処理 -│ ├── signup.ts # 登録処理 -│ ├── session.ts # セッション管理・検証 -│ ├── admin_access.ts # admin 権限検証（API用も含む） -│ └── _.test.ts -├── utils/ -│ ├── login.ts # ← buildLoginPath() をここに統合 -│ └── _.test.ts -├── types/ -│ ├── auth_status.ts # AdminStatus enum -│ └── session.ts # Session type -└── constants/ - └── messages.ts # エラーメッセージなど -``` - -### 既存コードの移行 - -#### Phase 1: Service層を抽出（新規ファイル） - -**`src/features/auth/services/session.ts`** - セッション検証の共通化 - -```typescript -// 既存の authorship.ts から移動・リネーム -export async function validateSession(locals: App.Locals): Promise<{ valid: boolean }> { - const session = await locals.auth.validate(); - return { valid: !!session }; -} - -export async function getLoggedInUser( - locals: App.Locals, - url?: URL, -): Promise { - const session = await locals.auth.validate(); - if (!session) { - const loginPath = buildLoginPath(url); - redirect(TEMPORARY_REDIRECT, loginPath); - } - - if (!locals.user) { - redirect(TEMPORARY_REDIRECT, buildLoginPath(url)); - } - - return locals.user; -} - -export async function ensureSessionOrRedirect(locals: App.Locals, url?: URL): Promise { - const session = await locals.auth.validate(); - if (!session) { - redirect(TEMPORARY_REDIRECT, buildLoginPath(url)); - } -} -``` - -**`src/features/auth/services/admin_access.ts`** - admin 権限検証 - -```typescript -// 既存の (admin)/_utils/auth.ts から移動 -export enum AdminStatus { - OK = 'ok', - UNAUTHENTICATED = 'unauthenticated', - UNAUTHORIZED = 'unauthorized', -} - -export async function validateAdminStatus(locals: App.Locals): Promise { - // 既存実装 -} - -export async function validateAdminAccess(locals: App.Locals, url?: URL): Promise { - if ((await validateAdminStatus(locals)) !== AdminStatus.OK) { - const loginPath = buildLoginPath(url); - redirect(TEMPORARY_REDIRECT, loginPath); - } -} - -export async function validateAdminAccessForApi(locals: App.Locals): Promise { - // 既存実装（error() 返却） -} -``` - -**`src/features/auth/utils/login.ts`** - -```typescript -// 2 箇所の重複を統合 -import { LOGIN_PAGE } from '$lib/constants/navbar-links'; - -export function buildLoginPath(url?: URL): string { - if (!url) { - return LOGIN_PAGE; - } - - return `${LOGIN_PAGE}?redirectTo=${encodeURIComponent(url.pathname + url.search)}`; -} -``` - -#### Phase 2: Authorship 権限判定の分離 - -**`src/lib/utils/authorship.ts`** - 権限判定に特化（削減版） - -```typescript -// ログイン関連は削除、権限判定のみ残す -export const isAdmin = (role: Roles): boolean => { - return role === Roles.ADMIN; -}; - -export const hasAuthority = (userId: string, authorId: string): boolean => { - return userId.toLowerCase() === authorId.toLowerCase(); -}; - -export const canRead = (...) => { ... }; -export const canEdit = (...) => { ... }; -export const canDelete = (...) => { ... }; -``` - -新しいエクスポート： - -```typescript -// src/features/auth/index.ts -export { validateSession, getLoggedInUser, ensureSessionOrRedirect } from './services/session'; - -export { validateAdminAccess, validateAdminAccessForApi } from './services/admin_access'; - -export { buildLoginPath } from './utils/login'; -``` - ---- - -## スコープの限定：含めないもの（後のフェーズ） - -### ❌ 今回は含めない - -**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 にする - -### ✅ 後のフェーズで検討 - -**Phase 1（次の iteration 以降）：Component 統合の再評価** - -- AuthForm.svelte が十分に「auth 機能特化」か確認 -- login form と signup form のニーズが同じかどうか -- その時点で移行するなら、十分なテストカバレッジがある状態で実施 -- 段階的マイグレーション：リスク評価 → テスト準備 → 移行実行 - ---- - -#### Phase 3: Route handlers の更新 - -**`src/routes/(auth)/login/+page.server.ts`** - -```typescript -// 既存: auth.createSession() 直後にredirect -// 修正: buildLoginPath を import して使用 - -import { buildLoginPath } from '$features/auth/utils/login'; - -// ...existing login logic... -redirect(SEE_OTHER, destination); // dest = buildLoginPath() の結果 -``` - -**`src/routes/(auth)/signup/+page.server.ts`** - 同様 - -**`src/routes/workbooks/create/+page.server.ts`** など - -```typescript -import { ensureSessionOrRedirect } from '$features/auth/services/session'; - -export const load = async ({ locals, url }) => { - await ensureSessionOrRedirect(locals, url); - // ... -}; -``` - -**`src/routes/(admin)/_utils/auth.ts`** - 削除（すべて `$features/auth` に移行） - ---- - -## マイグレーション検査リスト - -- [ ] `src/features/auth/` ディレクトリ作成 -- [ ] `session.ts` を新規作成（既存 authorship.ts から関数を移動） -- [ ] `admin_access.ts` を新規作成（既存 (admin)/\_utils/auth.ts から移動） -- [ ] `login.ts` を新規作成（2 箇所から統合） -- [ ] `src/lib/utils/authorship.ts` を権限判定のみに削減 -- [ ] `src/features/auth/index.ts` でエクスポート -- [ ] Route handlers で新しい import パス に更新 - - `src/routes/(auth)/login/+page.server.ts` - - `src/routes/(auth)/signup/+page.server.ts` - - `src/routes/workbooks/create/+page.server.ts` - - その他保護ルート -- [ ] `src/routes/(admin)/_utils/auth.ts` を削除 -- [ ] 単体テストを `src/features/auth/**/*.test.ts` に移行 - - `authorship.test.ts` から auth 関連を分離 - - `session.test.ts`, `admin_access.test.ts` を新規作成 -- [ ] E2E テストが新しい redirect_after_login フローをカバーしているか確認 -- [ ] 型チェック・linting を実行 - ---- - -## 検証方法 - -1. **ビルド確認** - `pnpm build` で型エラーがないか -2. **テスト確認** - `pnpm test:unit` で既存テストが引き続き pass -3. **E2E確認** - `pnpm test:e2e` で redirect フローが正常に動作 -4. **インポート確認** - grep で旧パスのインポートが残っていないか - ---- - -## 技術負債削減の効果 - -✅ **コード重複の排除** - `buildLoginPath()` の 2 重複を統合 -✅ **責任の明確化** - auth と authorship 権限判定を分離 -✅ **テスト構造の改善** - co-located テストで保守性向上 -✅ **拡張性の向上** - OAuth, 2FA 等の新しい auth 機能追加時に `src/features/auth/` に集約可能 -✅ **AGENTS.md 準拠** - feature-scoped 設計への段階的移行 - ---- - -## 注意点 - -### 破壊リスク最小化 - -- **段階的実行が必須** - すべてを一度に移行しない - - Phase 1: services のみ → テスト → デプロイ - - Phase 2: utils のみ → テスト → デプロイ - - Phase 3: route handlers 更新 → テスト → デプロイ -- **各 phase ごとに git commit** - ロールバック可能にする -- **E2E テストを各段階で実行** - redirect フロー が壊れていないか確認 - -### 実装上の注意 - -- 既存の `src/lib/utils/authorship.ts` は「著者権」関連の関数をコアとしているため、完全には削除しない -- Route handlers が `src/features/auth` を import する際、アクセス権の問題がないか確認（通常は問題なし） -- Lucia auth ライブラリの API は変更しない（`locals.auth.validate()` は継続使用） -- **コンポーネント移行は今回のスコープ外** - Phase 1 で必要性を再評価 From 02b00be95925a860284faaef91c69149417b4354 Mon Sep 17 00:00:00 2001 From: "k.hiro1818" Date: Wed, 6 May 2026 09:38:04 +0000 Subject: [PATCH 03/23] chore: add comments (#1582) --- src/features/auth/services/session.ts | 17 ++++++++++++++--- src/routes/problems/[slug]/+page.server.ts | 8 ++++++-- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/src/features/auth/services/session.ts b/src/features/auth/services/session.ts index ce14c3db2..4eb97bf92 100644 --- a/src/features/auth/services/session.ts +++ b/src/features/auth/services/session.ts @@ -5,15 +5,26 @@ import { TEMPORARY_REDIRECT } from '$lib/constants/http-response-status-codes'; import { buildLoginPath } from '../utils/login'; /** - * Get the current logged-in user or redirect to login + * Get the current logged-in user or redirect to login. + * + * **Control flow guarantee:** This function either returns a user object or throws redirect(). + * It never returns null — if no session exists, redirect() is thrown before returning. + * Safe to use with non-null assertion (!) in calling code, as null is unreachable. + * + * **Future refactor:** Consider separating into: + * - getLoggedInUser(locals, url) — current behavior (redirect on no session) + * - getLoggedInUserOptional(locals) — returns null on no session + * See GitHub issue #XXXX for discussion. + * * @param locals - The application locals containing auth and user information * @param url - The current URL; when provided, appends ?redirectTo= so the user returns after login - * @returns The logged-in user or null + * @returns The logged-in user (never null; redirect is thrown instead) + * @throws SvelteKit redirect(307, '/login?redirectTo=...') if session does not exist */ export const getLoggedInUser = async ( locals: App.Locals, url?: URL, -): Promise => { +): Promise => { await ensureSessionOrRedirect(locals, url); const loggedInUser = locals.user; diff --git a/src/routes/problems/[slug]/+page.server.ts b/src/routes/problems/[slug]/+page.server.ts index 43ec7c1c4..32e33e8a9 100644 --- a/src/routes/problems/[slug]/+page.server.ts +++ b/src/routes/problems/[slug]/+page.server.ts @@ -9,8 +9,10 @@ import { BAD_REQUEST, TEMPORARY_REDIRECT } from '$lib/constants/http-response-st const buttons = await getButtons(); export async function load({ locals, params, url }) { + // getLoggedInUser either returns a user or throws redirect(); null is unreachable. + // Non-null assertion (!) is safe here. const loggedInUser = await getLoggedInUser(locals, url); - const taskResult = await crud.getTaskResult(params.slug as string, loggedInUser!.id); + const taskResult = await crud.getTaskResult(params.slug as string, loggedInUser.id); return { taskResult: taskResult, buttons: buttons }; } @@ -20,8 +22,10 @@ export const actions = { const response = await request.formData(); const slug = params.slug as string; + // Note: getLoggedInUser either returns a user or throws redirect(); null is unreachable. + // This action implicitly requires login (redirect on no session). const loggedInUser = await getLoggedInUser(locals, url); - const userId = loggedInUser!.id; + const userId = loggedInUser.id; try { const submissionStatus = response.get('submissionStatus') as string; From 3c85b33fcad9d651d34f2b4f614936a42ed9d2ad Mon Sep 17 00:00:00 2001 From: "k.hiro1818" Date: Wed, 6 May 2026 09:38:26 +0000 Subject: [PATCH 04/23] chore: update AGENTS.md (#1582) --- AGENTS.md | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 7d7e8c9c0..c944564e2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,6 +24,29 @@ Always prefer simplicity over pathological correctness. YAGNI, KISS, DRY. No bac - Wait for explicit user consent ("let's start", "implement", etc.) - Use AskUserQuestion before starting if requirements are ambiguous (data model, preferences, test scope, etc.) +## Working with the User + +### Communication + +- **No social pleasantries:** Skip opening remarks like "お疲れ様です", "了解しました", "ありがとうございます" +- **Direct & concise:** Lead with substance (findings, next steps, decisions) +- **Task-focused:** Avoid flattery, apologizing, or performative agreement +- **Respect the work:** Speak plainly about tradeoffs and real issues; don't sugar-coat + +### Responding to Feedback + +- **Verify before accepting:** Check for misunderstanding, missing context, or counterpoints +- **Question, don't defer:** Ask clarifying questions or offer alternatives with reasoning—don't blindly follow + +### Critical Review as Default + +When proposing approaches, designs, or solutions: + +- **Lead with the proposal:** State the recommendation clearly +- **Then critique it:** Point out tradeoffs, risks, limitations, and when it fails +- **Offer alternatives:** Present 2–3 viable options with reasoning for each +- **Let user decide:** Don't pretend one option is obviously best if it isn't + ## Tech Stack SvelteKit 2 + Svelte 5 (Runes) + TypeScript | PostgreSQL + Prisma | Flowbite Svelte + Tailwind 4 | Vitest + Playwright | oxlint (JS/TS) + ESLint (Svelte) From 15b34349dc309137643a14b3fa0bbc6104af8c72 Mon Sep 17 00:00:00 2001 From: "k.hiro1818" Date: Wed, 6 May 2026 09:45:37 +0000 Subject: [PATCH 05/23] fix: auth consolidation Phase 1 (security fixes excluding NPE) - Fix form actions URL parameter: construct from request.url in workbooks delete - Add admin validation to tasks create/update form actions - Move getLoggedInUser inside try-catch in users page for error handling - Update Phase 1 checklist in review document All unit tests pass (2224 passed). Non-null Assertion NPE fixes deferred to Phase 2 as per user preference for separate PR handling. Co-Authored-By: Claude Haiku 4.5 --- .../auth-consolidation-review/review.md | 351 ++++++++++++++++++ src/routes/(admin)/tasks/+page.server.ts | 8 +- src/routes/users/[username]/+page.server.ts | 3 +- src/routes/workbooks/+page.server.ts | 3 +- 4 files changed, 361 insertions(+), 4 deletions(-) create mode 100644 docs/dev-notes/2026-05-06/auth-consolidation-review/review.md diff --git a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md new file mode 100644 index 000000000..a1a09cbde --- /dev/null +++ b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md @@ -0,0 +1,351 @@ +# Auth Feature Consolidation (#1582) - 総合レビューレポート + +**実施日:** 2026-05-06 +**ブランチ:** #1582 +**実施内容:** Security Review + CodeRabbit ×2 + +--- + +## 📋 Executive Summary + +### セキュリティレビュー結論 + +**✅ 認証リダイレクト機構：STRONG** + +- `isSameOriginRedirect()` は標準 URL API で正しく実装 +- Open redirect テスト（13ケース）が包括的 +- Login/Signup 両アクションが適切に `redirectTo` 検証 + +**⚠️ その他の課題：MEDIUM-HIGH リスク** + +- Form actions での admin 検証漏れ（CRITICAL） +- Service 層が redirect()/error() を呼び出し（設計違反） +- Null チェック不足による NPE リスク（HIGH） + +--- + +## 🔒 Security Findings + +### ✅ STRONG: Open Redirect Prevention + +**Implementation:** `src/lib/utils/url.ts:55-62` + +```typescript +export function isSameOriginRedirect(redirectTarget: string, requestOrigin: string): boolean { + try { + const targetUrl = new URL(redirectTarget, requestOrigin); + return targetUrl.origin === requestOrigin; + } catch { + return false; + } +} +``` + +**Coverage:** + +- ✓ 相対パス（`/workbooks/bfs`）を受け入れ +- ✓ クエリ付き相対パス（`/workbooks?tab=solution`）を受け入れ +- ✓ Protocol-relative URL（`//danger.com`）を拒否 +- ✓ JavaScript URL（`javascript:alert()`）を拒否 +- ✓ Data URL（`data:text/html,...`）を拒否 +- ✓ Userinfo bypass（`https://app.com@danger.com`）を拒否 +- ✓ 13 unit tests で包括的にカバー + +**E2E Testing:** `e2e/redirect_after_login.spec.ts:188-202` で open redirect シナリオを実装 + +--- + +### ⚠️ CRITICAL: Form Actions 未保護 + +**File:** `src/routes/(admin)/tasks/+page.server.ts:20-21` + +**Issue:** + +```typescript +export const actions: Actions = { + create: async ({ request }) => { + // ❌ locals なし → 検証未実行 + // ... + }, + update: async ({ request }) => { + // ... + }, +}; +``` + +**Impact:** 攻撃者が直接 POST リクエストで admin 機能を実行可能 + +**Recommendation:** + +```typescript +export const actions: Actions = { + create: async ({ request, locals, url }) => { + await validateAdminAccess(locals, url); // ✅ 追加必須 + // ... + }, +}; +``` + +--- + +### ⚠️ 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:** + +```typescript +// ❌ loggedInUser が null でも !.id でアクセス +const userId = loggedInUser!.id; +``` + +**Recommendation:** + +```typescript +// ✅ Null チェック後に安全にアクセス +if (!loggedInUser) { + return fail(BAD_REQUEST, { error: '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 (❌):** + +```typescript +export async function validateAdminAccess(locals: App.Locals, url?: URL): Promise { + if ((await validateAdminStatus(locals)) !== AdminStatus.OK) { + redirect(TEMPORARY_REDIRECT, buildLoginPath(url)); // ❌ Service で redirect + } +} +``` + +**Recommended (✅):** + +```typescript +export async function validateAdminAccess(locals: App.Locals): Promise { + return validateAdminStatus(locals); // ✅ 純粋な値返却 +} + +// Route handler で処理 +if ((await validateAdminAccess(locals)) !== AdminStatus.OK) { + redirect(TEMPORARY_REDIRECT, buildLoginPath(url)); +} +``` + +--- + +### ⚠️ MEDIUM: Module-scope Mutable State + +**File:** `src/routes/(admin)/account_transfer/+page.server.ts:14` + +```typescript +let accountTransferMessages: FloatingMessage[] = []; // ❌ 全リクエスト間で共有 +``` + +**Risks:** + +- 競合状態（複数ユーザーが同時実行時に値が上書き） +- 情報漏洩（ユーザーAの結果がユーザーBに表示） + +**Recommendation:** Superforms の message 機能または cookies を使用してリクエスト単位に管理 + +--- + +## 🐛 CodeRabbit Findings（×2回実施） + +### CRITICAL Issues + +| Issue | File | Line | Detail | +| --------------------- | -------------------------------------- | ------- | -------------------------------------------------------- | +| Form actions URL 不可 | `src/routes/workbooks/+page.server.ts` | 104-106 | `url` パラメータが form actions では不可（実行時エラー） | + +**Fix:** + +```typescript +delete: async ({ locals, request }) => { + const url = new URL(request.url); // ✅ request から構築 + const loggedInUser = await getLoggedInUser(locals, url); +``` + +--- + +### HIGH Issues + +| Issue | File | Line | Count | +| ---------------------------- | ---------------------------------------------------- | ------------ | ------ | +| Null チェック漏れ | `src/routes/problems/[slug]/+page.server.ts` | 12-13, 23-24 | 2 | +| getLoggedInUser try-catch 外 | `src/routes/users/[username]/+page.server.ts` | 11-12 | 1 | +| 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 'Ch0kuda1' | + +--- + +### 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: CRITICAL & HIGH（今すぐ） + +```markdown +- [x] Form actions の url パラメータ修正 +- [x] POST actions への admin 検証追加 +- [ ] NPE リスク → null チェックに変更（3 箇所）← Phase 2へ延期 +- [x] getLoggedInUser を try-catch 内に移動 +``` + +**見積:** 1-2 時間 | **実績:** 完了 (NPE は別PRで対応) + +### Phase 2: Service 層リファクタ（短期） + +```markdown +- [ ] ensureSessionOrRedirect を純粋関数に変更 +- [ ] getLoggedInUser の戻り値型を正確に +- [ ] validateAdminAccess → AdminStatus 返却に変更 +- [ ] validateAdminAccessForApi → error() 削除 +``` + +**見積:** 3-4 時間 + テスト修正 + +### Phase 3: テスト・ドキュメント（並行可） + +```markdown +- [ ] テストアサーション補完（.resolves → .resolves.toBeUndefined()） +- [ ] login.test.ts, signup.test.ts を追加 +- [ ] E2E パスワード統一 +- [ ] Module-scope state をリクエスト単位に変更 +``` + +**見積:** 2-3 時間 + +### Phase 4: 最適化（オプション） + +```markdown +- [ ] runTests helper 統一 +- [ ] 型安全性改善 +- [ ] Reactive store 修正 +``` + +**見積:** 1-2 時間 + +--- + +## ✅ Verified Strengths + +### 1. Open Redirect 対策 + +- ✅ 標準 URL API の正しい使用 +- ✅ 包括的なテストカバレッジ +- ✅ E2E テストで悪意あるリダイレクト検証 + +### 2. 認証フロー実装 + +- ✅ Login/Signup アクションで `redirectTo` 検証 +- ✅ `isSameOriginRedirect()` で多くの攻撃ベクトル対応 +- ✅ Default fallback (`HOME_PAGE`) で未検証リダイレクトを安全に処理 + +### 3. テスト戦略 + +- ✅ Unit test（`isSameOriginRedirect` 13 ケース） +- ✅ E2E test（18+ ケース） +- ✅ 攻撃シナリオをカバー + +--- + +## 🎯 Remaining Work + +**セッション中に対応すべき項目:** + +1. **CRITICAL (実行時エラー)** + - Form actions の `url` パラメータ修正 + +2. **HIGH (セキュリティ)** + - POST actions への admin 検証追加 + - NPE リスクを安全な null チェックに + +3. **MEDIUM (設計)** + - Service 層の設計ガイドライン準拠 + - Module-scope mutable state 削除 + +**留意点:** + +- Service 層リファクタは既存テスト修正を含む +- E2E テストで複数ルートが影響を受ける +- 修正後は `pnpm test:unit` + `pnpm test:e2e` で検証必須 + +--- + +## ✅ Phase 1 完了報告 + +**実施内容:** + +1. **Form actions URL パラメータ修正** (`src/routes/workbooks/+page.server.ts:104`) + - SvelteKit form actions は `url` を受け取れないため `request.url` から構築 + - `getLoggedInUser(locals, url)` が正しく URL を受け取るように修正 + +2. **POST actions 認証検証追加** (`src/routes/(admin)/tasks/+page.server.ts:110, 138`) + - `create`, `update` form actions に `locals` パラメータ追加 + - `validateAdminAccess(locals)` を各アクションの最初に追加 + - 未認証ユーザーの直接 POST を防止 + +3. **エラーハンドリング改善** (`src/routes/users/[username]/+page.server.ts:12`) + - `getLoggedInUser` を try-catch ブロック内に移動 + - 認証失敗時のエラーを適切にキャッチ + - `loggedInUser` 型を `Awaited>` で正確に宣言 + +**テスト結果:** +- ✓ Unit tests: 2224 passed | 1 skipped +- ✓ 既存テストで回帰なし + +**注記:** +- Non-null Assertion NPE (3 箇所) は Phase 2 で大規模修正として対応 +- すべての修正は既存テストを通過 + +--- + +## 参考 + +- [AGENTS.md - Service Layer Guidelines](../../AGENTS.md) +- [.claude/rules/auth.md - Authentication Rules](../../.claude/rules/auth.md) +- [CodeRabbit Output (1st)](../coderabbit-1st-review.txt) +- [CodeRabbit Output (2nd)](../coderabbit-2nd-review.txt) diff --git a/src/routes/(admin)/tasks/+page.server.ts b/src/routes/(admin)/tasks/+page.server.ts index aaf4ccee5..4590f9dad 100644 --- a/src/routes/(admin)/tasks/+page.server.ts +++ b/src/routes/(admin)/tasks/+page.server.ts @@ -107,7 +107,9 @@ function mergeContestsAndUnregisteredTasks( } export const actions: Actions = { - create: async ({ request }) => { + create: async ({ request, locals }) => { + await validateAdminAccess(locals); + try { console.log('users->actions->generate'); const formData = await request.formData(); @@ -133,7 +135,9 @@ export const actions: Actions = { }; }, - update: async ({ request }) => { + update: async ({ request, locals }) => { + await validateAdminAccess(locals); + try { console.log('users->actions->generate'); const formData = await request.formData(); diff --git a/src/routes/users/[username]/+page.server.ts b/src/routes/users/[username]/+page.server.ts index 76daebfbc..1af3022c3 100644 --- a/src/routes/users/[username]/+page.server.ts +++ b/src/routes/users/[username]/+page.server.ts @@ -9,9 +9,10 @@ import type { TaskResult } from '$lib/types/task'; import { getLoggedInUser } from '$features/auth/services/session'; export async function load({ locals, params, url }) { - const loggedInUser = await getLoggedInUser(locals, url); + let loggedInUser: Awaited> | undefined; try { + loggedInUser = await getLoggedInUser(locals, url); const user = await userService.getUser(params.username as string); if (!user) { diff --git a/src/routes/workbooks/+page.server.ts b/src/routes/workbooks/+page.server.ts index e3cdbe817..2268c4946 100644 --- a/src/routes/workbooks/+page.server.ts +++ b/src/routes/workbooks/+page.server.ts @@ -101,7 +101,8 @@ export async function load({ locals, url }) { } export const actions = { - delete: async ({ locals, url }) => { + delete: async ({ locals, request }) => { + const url = new URL(request.url); const loggedInUser = await getLoggedInUser(locals, url); const slug = url.searchParams.get('slug'); const workBookId = parseWorkBookId(slug as string); From 00068adb9873f527223601afb7872e109d75fefd Mon Sep 17 00:00:00 2001 From: "k.hiro1818" Date: Wed, 6 May 2026 09:48:56 +0000 Subject: [PATCH 06/23] docs: add SvelteKit form actions and error handling rules - Form Actions: `url` Parameter Unavailable Form action handlers cannot receive `url` directly; construct from request.url - Error Handling in load() All async operations must be inside try-catch block, not before - Auth Audit (expanded) Clarify that form actions also require audit when one is protected Co-Authored-By: Claude Haiku 4.5 --- .claude/rules/sveltekit.md | 58 +++++++++++++++++++++++++++++++++++--- 1 file changed, 54 insertions(+), 4 deletions(-) diff --git a/.claude/rules/sveltekit.md b/.claude/rules/sveltekit.md index e3cf7ff7d..15e29b2bc 100644 --- a/.claude/rules/sveltekit.md +++ b/.claude/rules/sveltekit.md @@ -51,6 +51,25 @@ if (!(Object.values(TaskGrade) as string[]).includes(gradeRaw)) { const grade = gradeRaw as TaskGrade; // Safe now ``` +## Form Actions: `url` Parameter Unavailable + +Form action handlers cannot access `url`. Construct it from `request.url`: + +```typescript +// ❌ Runtime error +export const actions = { + delete: async ({ request, url }) => { ... } +}; + +// ✅ Correct +export const actions = { + delete: async ({ request }) => { + const url = new URL(request.url); + // ... + } +}; +``` + ## Page Component Props `+page.svelte` accepts only `data` and `form` props (`svelte/valid-prop-names-in-kit-pages`). @@ -85,17 +104,48 @@ Consume with `$derived` in `+page.svelte` to sync after `load()` re-runs. ## Error Handling in load() -Wrap service calls in try-catch, return safe defaults: +All async operations must be **inside** the try-catch block, not before: ```typescript -const data = await service.fetch(...).catch(() => []); +// ❌ getLoggedInUser outside try-catch; errors escape unhandled +const loggedInUser = await getLoggedInUser(locals, url); +try { + const data = await service.fetch(...); +} catch (e) { ... } + +// ✅ All async inside try-catch +try { + const loggedInUser = await getLoggedInUser(locals, url); + const data = await service.fetch(...); +} catch (e) { ... } ``` -Prevents single service error from crashing entire page. +Errors from async calls before the try block propagate unhandled and crash the page. ## Auth Audit -When adding guard to one action in `+page.server.ts`, audit all other actions. Asymmetric guards (some protected, others not) are a recurring vulnerability. +When protecting one action (in `load()` or form actions), **audit all others**. +Asymmetric guards (some protected, others not) are a critical vulnerability. + +```typescript +// ❌ Only delete is protected +export const actions = { + create: async ({ request }) => { ... }, + delete: async ({ request, locals }) => { + await validateAdminAccess(locals); + }, +}; + +// ✅ All admin actions protected equally +export const actions = { + create: async ({ request, locals }) => { + await validateAdminAccess(locals); + }, + delete: async ({ request, locals }) => { + await validateAdminAccess(locals); + }, +}; +``` ## success Flag & message Consistency From 53510c50b3fb4f3610379cb5ed1c64e86f490d3e Mon Sep 17 00:00:00 2001 From: "k.hiro1818" Date: Wed, 6 May 2026 09:49:56 +0000 Subject: [PATCH 07/23] docs: simplify SvelteKit rules to essence only - Form Actions: `url` Parameter Unavailable (1 line) - Error Handling in load() (1 line) - Auth Audit (1 line) Co-Authored-By: Claude Haiku 4.5 --- .claude/rules/sveltekit.md | 58 +------------------ .../auth-consolidation-review/review.md | 2 + 2 files changed, 5 insertions(+), 55 deletions(-) diff --git a/.claude/rules/sveltekit.md b/.claude/rules/sveltekit.md index 15e29b2bc..783f241da 100644 --- a/.claude/rules/sveltekit.md +++ b/.claude/rules/sveltekit.md @@ -53,22 +53,7 @@ const grade = gradeRaw as TaskGrade; // Safe now ## Form Actions: `url` Parameter Unavailable -Form action handlers cannot access `url`. Construct it from `request.url`: - -```typescript -// ❌ Runtime error -export const actions = { - delete: async ({ request, url }) => { ... } -}; - -// ✅ Correct -export const actions = { - delete: async ({ request }) => { - const url = new URL(request.url); - // ... - } -}; -``` +Form action handlers cannot access `url`. Use `new URL(request.url)` instead. ## Page Component Props @@ -104,48 +89,11 @@ Consume with `$derived` in `+page.svelte` to sync after `load()` re-runs. ## Error Handling in load() -All async operations must be **inside** the try-catch block, not before: - -```typescript -// ❌ getLoggedInUser outside try-catch; errors escape unhandled -const loggedInUser = await getLoggedInUser(locals, url); -try { - const data = await service.fetch(...); -} catch (e) { ... } - -// ✅ All async inside try-catch -try { - const loggedInUser = await getLoggedInUser(locals, url); - const data = await service.fetch(...); -} catch (e) { ... } -``` - -Errors from async calls before the try block propagate unhandled and crash the page. +All async operations must be inside the try-catch block, not before it. Errors from async calls outside propagate unhandled. ## Auth Audit -When protecting one action (in `load()` or form actions), **audit all others**. -Asymmetric guards (some protected, others not) are a critical vulnerability. - -```typescript -// ❌ Only delete is protected -export const actions = { - create: async ({ request }) => { ... }, - delete: async ({ request, locals }) => { - await validateAdminAccess(locals); - }, -}; - -// ✅ All admin actions protected equally -export const actions = { - create: async ({ request, locals }) => { - await validateAdminAccess(locals); - }, - delete: async ({ request, locals }) => { - await validateAdminAccess(locals); - }, -}; -``` +When protecting one action (in `load()` or form actions), audit all others. Asymmetric guards are a critical vulnerability. ## success Flag & message Consistency diff --git a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md index a1a09cbde..be73abda4 100644 --- a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md +++ b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md @@ -334,10 +334,12 @@ delete: async ({ locals, request }) => { - `loggedInUser` 型を `Awaited>` で正確に宣言 **テスト結果:** + - ✓ Unit tests: 2224 passed | 1 skipped - ✓ 既存テストで回帰なし **注記:** + - Non-null Assertion NPE (3 箇所) は Phase 2 で大規模修正として対応 - すべての修正は既存テストを通過 From 7b7efe57205b6dbf4effd38590fb59944d43c9a0 Mon Sep 17 00:00:00 2001 From: "k.hiro1818" Date: Wed, 6 May 2026 09:57:38 +0000 Subject: [PATCH 08/23] docs: update Phase 2 Service refactor approach to Wrapper pattern Replace pure function return with Wrapper pattern: - validateAdminStatus (pure) + ensureAdminOrRedirect (wrapper) - Solves: Service layer purity + Route handler brevity Benefits: No redundancy, testable, framework-independent Service layer Co-Authored-By: Claude Haiku 4.5 --- .../auth-consolidation-review/review.md | 37 ++++++++++++++----- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md index be73abda4..ae2223a30 100644 --- a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md +++ b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md @@ -136,19 +136,36 @@ export async function validateAdminAccess(locals: App.Locals, url?: URL): Promis } ``` -**Recommended (✅):** +**Recommended (✅): Wrapper パターン** + +Service 層は純粋、Route helper で framework 処理を集約： ```typescript -export async function validateAdminAccess(locals: App.Locals): Promise { - return validateAdminStatus(locals); // ✅ 純粋な値返却 +// src/features/auth/services/admin_access.ts （純粋、テスト可能） +export async function validateAdminStatus(locals: App.Locals): Promise { + // ... } -// Route handler で処理 -if ((await validateAdminAccess(locals)) !== AdminStatus.OK) { - redirect(TEMPORARY_REDIRECT, buildLoginPath(url)); +// src/features/auth/utils/admin_access.ts （Route helper） +export async function ensureAdminOrRedirect(locals: App.Locals, url?: URL): Promise { + const status = await validateAdminStatus(locals); + if (status !== AdminStatus.OK) { + redirect(TEMPORARY_REDIRECT, buildLoginPath(url)); + } } ``` +**呼び出し側：** +```typescript +// Route handler +await ensureAdminOrRedirect(locals, url); + +// Form action +await ensureAdminOrRedirect(locals); +``` + +利点：Service は純粋 + 呼び出し側は冗長性ゼロ + --- ### ⚠️ MEDIUM: Module-scope Mutable State @@ -239,13 +256,13 @@ delete: async ({ locals, request }) => { ### Phase 2: Service 層リファクタ（短期） ```markdown -- [ ] ensureSessionOrRedirect を純粋関数に変更 +- [ ] ensureSessionOrRedirect → 純粋関数 + Wrapper パターン - [ ] getLoggedInUser の戻り値型を正確に -- [ ] validateAdminAccess → AdminStatus 返却に変更 -- [ ] validateAdminAccessForApi → error() 削除 +- [ ] validateAdminStatus → 純粋関数、ensureAdminOrRedirect Wrapper を提供 +- [ ] validateAdminAccessForApi → 純粋関数化、error() は Route に移動 ``` -**見積:** 3-4 時間 + テスト修正 +**見積:** 3-4 時間 + テスト修正 | **パターン:** Wrapper 関数で Service 純粋性 + Route 簡潔性を両立 ### Phase 3: テスト・ドキュメント（並行可） From 0b9ec3466e5e63a3bfa4d5a0b6bb66f7c18d1ab7 Mon Sep 17 00:00:00 2001 From: "k.hiro1818" Date: Wed, 6 May 2026 09:59:40 +0000 Subject: [PATCH 09/23] chore: fix format (#1582) --- .../dev-notes/2026-05-06/auth-consolidation-review/review.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md index ae2223a30..54dd13141 100644 --- a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md +++ b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md @@ -1,7 +1,7 @@ # Auth Feature Consolidation (#1582) - 総合レビューレポート -**実施日:** 2026-05-06 -**ブランチ:** #1582 +**実施日:** 2026-05-06 +**ブランチ:** #1582 **実施内容:** Security Review + CodeRabbit ×2 --- @@ -156,6 +156,7 @@ export async function ensureAdminOrRedirect(locals: App.Locals, url?: URL): Prom ``` **呼び出し側：** + ```typescript // Route handler await ensureAdminOrRedirect(locals, url); From 4933a5c51142897ad3bb8007cd4c7fbc9d9b0513 Mon Sep 17 00:00:00 2001 From: "k.hiro1818" Date: Wed, 6 May 2026 10:00:41 +0000 Subject: [PATCH 10/23] docs: remove completed Phase 1 tasks from checklist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1 complete: ✓ Form actions URL parameter fix ✓ POST actions admin validation ✓ getLoggedInUser error handling Remaining: NPE fixes deferred to Phase 2 Co-Authored-By: Claude Haiku 4.5 --- .../2026-05-06/auth-consolidation-review/review.md | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md index 54dd13141..374b162f0 100644 --- a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md +++ b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md @@ -243,15 +243,17 @@ delete: async ({ locals, request }) => { ## 📊 優先度別修正スケジュール -### Phase 1: CRITICAL & HIGH（今すぐ） +### Phase 1: CRITICAL & HIGH（✅ 完了） ```markdown -- [x] Form actions の url パラメータ修正 -- [x] POST actions への admin 検証追加 - [ ] NPE リスク → null チェックに変更（3 箇所）← Phase 2へ延期 -- [x] getLoggedInUser を try-catch 内に移動 ``` +**完了済み:** +- ✓ Form actions の url パラメータ修正 +- ✓ POST actions への admin 検証追加 +- ✓ getLoggedInUser を try-catch 内に移動 + **見積:** 1-2 時間 | **実績:** 完了 (NPE は別PRで対応) ### Phase 2: Service 層リファクタ（短期） From 9de77f268b6f16ac3b8c34f5a23314988414584e Mon Sep 17 00:00:00 2001 From: "k.hiro1818" Date: Wed, 6 May 2026 10:02:16 +0000 Subject: [PATCH 11/23] docs: remove Phase 1 completed findings from review document MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove: ✓ Form Actions 未保護 (CRITICAL) - fixed in Phase 1 ✓ Form actions URL parameter (CRITICAL CodeRabbit) - fixed ✓ getLoggedInUser try-catch (HIGH CodeRabbit) - fixed Update: - Executive Summary now shows only Phase 2+ remaining issues - CodeRabbit sections reflect current state Document now focuses on Phase 2 and beyond work. Co-Authored-By: Claude Haiku 4.5 --- .../auth-consolidation-review/review.md | 68 +++---------------- 1 file changed, 9 insertions(+), 59 deletions(-) diff --git a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md index 374b162f0..ffc97f5b7 100644 --- a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md +++ b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md @@ -16,9 +16,8 @@ - Open redirect テスト（13ケース）が包括的 - Login/Signup 両アクションが適切に `redirectTo` 検証 -**⚠️ その他の課題：MEDIUM-HIGH リスク** +**⚠️ 残存課題（Phase 2 以降）：MEDIUM-HIGH リスク** -- Form actions での admin 検証漏れ（CRITICAL） - Service 層が redirect()/error() を呼び出し（設計違反） - Null チェック不足による NPE リスク（HIGH） @@ -55,39 +54,6 @@ export function isSameOriginRedirect(redirectTarget: string, requestOrigin: stri --- -### ⚠️ CRITICAL: Form Actions 未保護 - -**File:** `src/routes/(admin)/tasks/+page.server.ts:20-21` - -**Issue:** - -```typescript -export const actions: Actions = { - create: async ({ request }) => { - // ❌ locals なし → 検証未実行 - // ... - }, - update: async ({ request }) => { - // ... - }, -}; -``` - -**Impact:** 攻撃者が直接 POST リクエストで admin 機能を実行可能 - -**Recommendation:** - -```typescript -export const actions: Actions = { - create: async ({ request, locals, url }) => { - await validateAdminAccess(locals, url); // ✅ 追加必須 - // ... - }, -}; -``` - ---- - ### ⚠️ HIGH: Non-null Assertion での NPE **Files:** @@ -188,31 +154,14 @@ let accountTransferMessages: FloatingMessage[] = []; // ❌ 全リクエスト ## 🐛 CodeRabbit Findings（×2回実施） -### CRITICAL Issues - -| Issue | File | Line | Detail | -| --------------------- | -------------------------------------- | ------- | -------------------------------------------------------- | -| Form actions URL 不可 | `src/routes/workbooks/+page.server.ts` | 104-106 | `url` パラメータが form actions では不可（実行時エラー） | - -**Fix:** - -```typescript -delete: async ({ locals, request }) => { - const url = new URL(request.url); // ✅ request から構築 - const loggedInUser = await getLoggedInUser(locals, url); -``` - ---- - -### HIGH Issues +### HIGH Issues（残存） -| Issue | File | Line | Count | -| ---------------------------- | ---------------------------------------------------- | ------------ | ------ | -| Null チェック漏れ | `src/routes/problems/[slug]/+page.server.ts` | 12-13, 23-24 | 2 | -| getLoggedInUser try-catch 外 | `src/routes/users/[username]/+page.server.ts` | 11-12 | 1 | -| 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 | +| 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 | --- @@ -250,6 +199,7 @@ delete: async ({ locals, request }) => { ``` **完了済み:** + - ✓ Form actions の url パラメータ修正 - ✓ POST actions への admin 検証追加 - ✓ getLoggedInUser を try-catch 内に移動 From a043ccbf300df887ada57341aa7bb19482b01519 Mon Sep 17 00:00:00 2001 From: "k.hiro1818" Date: Wed, 6 May 2026 10:03:08 +0000 Subject: [PATCH 12/23] docs: remove completed work and implementation details from review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove noise: - ✅ Verified Strengths (already implemented) - 🎯 Remaining Work (already done in Phase 1) - ✅ Phase 1 完了報告 (implementation record) Keep focus on: PR #1582 review findings and Phase 2+ schedule Document is now a pure "issues to address" resource, not a session record. Co-Authored-By: Claude Haiku 4.5 --- .../auth-consolidation-review/review.md | 85 ++----------------- 1 file changed, 5 insertions(+), 80 deletions(-) diff --git a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md index ffc97f5b7..f9e79c380 100644 --- a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md +++ b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md @@ -156,12 +156,12 @@ let accountTransferMessages: FloatingMessage[] = []; // ❌ 全リクエスト ### HIGH Issues（残存） -| Issue | File | Line | Count | -| --------------------- | ---------------------------------------------------- | -------- | ------ | +| 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 | +| 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 | --- @@ -240,81 +240,6 @@ let accountTransferMessages: FloatingMessage[] = []; // ❌ 全リクエスト --- -## ✅ Verified Strengths - -### 1. Open Redirect 対策 - -- ✅ 標準 URL API の正しい使用 -- ✅ 包括的なテストカバレッジ -- ✅ E2E テストで悪意あるリダイレクト検証 - -### 2. 認証フロー実装 - -- ✅ Login/Signup アクションで `redirectTo` 検証 -- ✅ `isSameOriginRedirect()` で多くの攻撃ベクトル対応 -- ✅ Default fallback (`HOME_PAGE`) で未検証リダイレクトを安全に処理 - -### 3. テスト戦略 - -- ✅ Unit test（`isSameOriginRedirect` 13 ケース） -- ✅ E2E test（18+ ケース） -- ✅ 攻撃シナリオをカバー - ---- - -## 🎯 Remaining Work - -**セッション中に対応すべき項目:** - -1. **CRITICAL (実行時エラー)** - - Form actions の `url` パラメータ修正 - -2. **HIGH (セキュリティ)** - - POST actions への admin 検証追加 - - NPE リスクを安全な null チェックに - -3. **MEDIUM (設計)** - - Service 層の設計ガイドライン準拠 - - Module-scope mutable state 削除 - -**留意点:** - -- Service 層リファクタは既存テスト修正を含む -- E2E テストで複数ルートが影響を受ける -- 修正後は `pnpm test:unit` + `pnpm test:e2e` で検証必須 - ---- - -## ✅ Phase 1 完了報告 - -**実施内容:** - -1. **Form actions URL パラメータ修正** (`src/routes/workbooks/+page.server.ts:104`) - - SvelteKit form actions は `url` を受け取れないため `request.url` から構築 - - `getLoggedInUser(locals, url)` が正しく URL を受け取るように修正 - -2. **POST actions 認証検証追加** (`src/routes/(admin)/tasks/+page.server.ts:110, 138`) - - `create`, `update` form actions に `locals` パラメータ追加 - - `validateAdminAccess(locals)` を各アクションの最初に追加 - - 未認証ユーザーの直接 POST を防止 - -3. **エラーハンドリング改善** (`src/routes/users/[username]/+page.server.ts:12`) - - `getLoggedInUser` を try-catch ブロック内に移動 - - 認証失敗時のエラーを適切にキャッチ - - `loggedInUser` 型を `Awaited>` で正確に宣言 - -**テスト結果:** - -- ✓ Unit tests: 2224 passed | 1 skipped -- ✓ 既存テストで回帰なし - -**注記:** - -- Non-null Assertion NPE (3 箇所) は Phase 2 で大規模修正として対応 -- すべての修正は既存テストを通過 - ---- - ## 参考 - [AGENTS.md - Service Layer Guidelines](../../AGENTS.md) From ae52cffaacf2745ee6e9951e6f666f2c3319a6eb Mon Sep 17 00:00:00 2001 From: "k.hiro1818" Date: Wed, 6 May 2026 13:37:40 +0000 Subject: [PATCH 13/23] fix: add admin auth check to account transfer action and document review findings - Add `validateAdminAccess(locals, url)` to account_transfer form action to prevent unauthenticated POST bypass (load() is not called for form actions) - Document critical code review findings in review.md: issue resolutions, Phase 2 refactor risks, and corrected SvelteKit rule for url parameter Co-Authored-By: Claude Sonnet 4.6 --- .../auth-consolidation-review/review.md | 376 +++++++++++++++--- .../(admin)/account_transfer/+page.server.ts | 4 +- 2 files changed, 328 insertions(+), 52 deletions(-) diff --git a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md index f9e79c380..7359446be 100644 --- a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md +++ b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md @@ -10,12 +10,6 @@ ### セキュリティレビュー結論 -**✅ 認証リダイレクト機構：STRONG** - -- `isSameOriginRedirect()` は標準 URL API で正しく実装 -- Open redirect テスト（13ケース）が包括的 -- Login/Signup 両アクションが適切に `redirectTo` 検証 - **⚠️ 残存課題（Phase 2 以降）：MEDIUM-HIGH リスク** - Service 層が redirect()/error() を呼び出し（設計違反） @@ -25,35 +19,6 @@ ## 🔒 Security Findings -### ✅ STRONG: Open Redirect Prevention - -**Implementation:** `src/lib/utils/url.ts:55-62` - -```typescript -export function isSameOriginRedirect(redirectTarget: string, requestOrigin: string): boolean { - try { - const targetUrl = new URL(redirectTarget, requestOrigin); - return targetUrl.origin === requestOrigin; - } catch { - return false; - } -} -``` - -**Coverage:** - -- ✓ 相対パス（`/workbooks/bfs`）を受け入れ -- ✓ クエリ付き相対パス（`/workbooks?tab=solution`）を受け入れ -- ✓ Protocol-relative URL（`//danger.com`）を拒否 -- ✓ JavaScript URL（`javascript:alert()`）を拒否 -- ✓ Data URL（`data:text/html,...`）を拒否 -- ✓ Userinfo bypass（`https://app.com@danger.com`）を拒否 -- ✓ 13 unit tests で包括的にカバー - -**E2E Testing:** `e2e/redirect_after_login.spec.ts:188-202` で open redirect シナリオを実装 - ---- - ### ⚠️ HIGH: Non-null Assertion での NPE **Files:** @@ -192,23 +157,10 @@ let accountTransferMessages: FloatingMessage[] = []; // ❌ 全リクエスト ## 📊 優先度別修正スケジュール -### Phase 1: CRITICAL & HIGH（✅ 完了） - -```markdown -- [ ] NPE リスク → null チェックに変更（3 箇所）← Phase 2へ延期 -``` - -**完了済み:** - -- ✓ Form actions の url パラメータ修正 -- ✓ POST actions への admin 検証追加 -- ✓ getLoggedInUser を try-catch 内に移動 - -**見積:** 1-2 時間 | **実績:** 完了 (NPE は別PRで対応) - ### Phase 2: Service 層リファクタ（短期） ```markdown +- [ ] NPE リスク → null チェックに変更（3 箇所 - [ ] ensureSessionOrRedirect → 純粋関数 + Wrapper パターン - [ ] getLoggedInUser の戻り値型を正確に - [ ] validateAdminStatus → 純粋関数、ensureAdminOrRedirect Wrapper を提供 @@ -240,9 +192,331 @@ let accountTransferMessages: FloatingMessage[] = []; // ❌ 全リクエスト --- +## 🔍 Critical Code Review - レビュー指摘事項の批判的検討 + +### 新規追加: HIGH セキュリティ問題（ドキュメント未記載） + +#### ✅ Issue 1: 指摘は誤り — 現状は正しい（将来の要件変更への懸念あり） + +**File:** `src/routes/workbooks/[slug]/+page.server.ts:17, 35` + +**レビューツールの指摘（却下）:** + +> `loggedInUser === null` の場合、未発行 workbook にゲストがアクセス可能。 + +**現状:** このページはログインユーザー専用の設計方針。`getLoggedInUser()` はセッションなしなら必ず redirect を throw するため（`session.ts` のコントラクト）、ゲストは 35 行に到達しない。セキュリティギャップなし。 + +**将来の懸念（ゲスト閲覧を許可したい要件が来た場合）:** + +現在のコードはゲスト対応に変更しにくい構造になっている： + +- `getLoggedInUser()` を `getLoggedInUserOptional()` に変えると、35 行の `canRead(isPublished, loggedInUser.id, authorId)` が型エラー（`userId: string` が null/undefined を受け取れない） +- `canRead()` の型シグネチャ変更 + 35 行の null-guard 調整が同時に必要 +- 42 行の `loggedInUser?.id as string` という型強制キャストも問題になる + +「ゲストでも公開 workbook を見せる」変更は、単純な1行修正では済まない。 + +--- + +#### ✅ Issue 2: 指摘は誤り — 現状は正しい（Phase 2 refactor 時に高リスクの落とし穴あり） + +**File:** `src/routes/workbooks/+page.server.ts:104-121` + +**レビューツールの指摘（却下）:** + +> 匿名リクエストが delete ハンドラーを通過できる。 + +`getLoggedInUser()` はセッションなしなら必ず redirect を throw するため、匿名ユーザーは到達しない。現状セキュリティギャップなし。 + +**Phase 2 refactor 時の高リスク懸念:** + +`getLoggedInUser` を pure 化（`User | null` 返却）すると、119 行の null-guard が問題になる： + +```typescript +if (loggedInUser && !canDelete(loggedInUser.id, workBook.authorId)) { + error(FORBIDDEN, ...); +} +// loggedInUser が null → 条件が false → guard をスキップ → 誰でも削除可能 +``` + +閲覧と違い、**削除は非可逆操作**。Phase 2 で pure 化する際に、この null-guard を `if (!loggedInUser || !canDelete(...))` に修正するか、手前で `ensureSessionOrRedirect` を呼ぶことをセットで行う必要がある。Phase 2 チェックリストに明記しておくこと。 + +--- + +#### 🚨 Issue 3: Account Transfer Action が admin 認可チェックなし（指摘は正当、修正方法が誤り） + +**File:** `src/routes/(admin)/account_transfer/+page.server.ts:33-68` + +**問題（正当）:** + +実際のコードは `actions.default` が `{ request }` のみ受け取っており `locals` を省略している。`load()` の `validateAdminAccess` は form への直接 POST で bypass 可能（load は form action 実行時には呼ばれない）。**unauthenticated POST が task results を複写できる。** + +**レビューツールの修正案（誤り）:** + +```typescript +await validateAdminAccessForApi(locals); // ❌ +server.ts 用のメソッド +``` + +auth.md のルール： +- `validateAdminAccess` — **page routes 用**（`+page.server.ts`）、未認証・非 admin どちらも `/login` へ redirect +- `validateAdminAccessForApi` — **API routes 用**（`+server.ts`）、`error(401/403)` を throw + +ここは `+page.server.ts` の form action なので `validateAdminAccess` が正しい。 + +**正しい修正案:** + +```typescript +export const actions: Actions = { + default: async ({ request, locals, url }) => { + await validateAdminAccess(locals, url); // ← page route 用 + // ... + } +} +``` + +--- + +### 新規追加: MEDIUM 問題（優先度順） + +#### Issue 4: SvelteKit ドキュメント記述が不正確 + +**File:** `.claude/rules/sveltekit.md:54-56` + +**現状 (❌):** + +```markdown +## Form Actions: `url` Parameter Unavailable + +Form action handlers cannot access `url`. Use `new URL(request.url)` instead. +``` + +**問題:** 実際には form actions は `url` を **受け取れる**。ドキュメントが誤り。 + +**例（実際に動作）:** + +```typescript +export const actions = { + default: async ({ request, locals, url }) => { + // ← url は受け取れる + const redirectTo = url.searchParams.get('redirectTo'); + }, +}; +``` + +**批判的評価:** + +- ドキュメント誤りは開発者の混乱を招く +- ただし「new URL(request.url) パターン」も正しいアプローチなので、削除ではなく「両方サポート、url は convenience」と修正すべき +- 実装側との乖離が大 + +**修正案:** + +````markdown +## Form Actions: `url` Parameter Usage + +Form action handlers can access `url` directly: + +```typescript +export const actions = { + default: async ({ request, locals, url }) => { + const redirectTo = url.searchParams.get('redirectTo'); + }, +}; +``` +```` + +または、request から再構築することも可能： + +```typescript +const url = new URL(request.url); + +（両方正しい） + +``` + +--- + +#### Issue 5: Signup Helper のドキュメント漏れ + +**File:** `.claude/rules/auth.md:41-49` (Redirect After Login Pattern) + +**問題:** `buildSignupPath()` が存在するが、auth.md では `buildLoginPath` のみ記載。signup フロー用の同等ヘルパーが隠れている。 + +**修正案:** auth.md の Key Files セクションに追記： + +```markdown +- `src/features/auth/utils/signup.ts`: `buildSignupPath(url?)` — generates `/signup` or `/signup?redirectTo=...` +``` + +--- + +#### Issue 6: buildSignupPath API が非対称 + +**File:** `src/features/auth/utils/signup.ts:8-14` + +**現状:** + +```typescript +export function buildSignupPath(redirectTo: string | null): string { + // ... +} + +// vs. buildLoginPath: +export function buildLoginPath(url: string | URL | null): string { + // ... +} +``` + +**問題:** `buildLoginPath()` は `URL` を受け入れるが、`buildSignupPath()` は `string` のみ。呼び出し側で手動変換が必要（DRY 違反）。 + +**批判的評価:** + +- 些細だが一貫性欠落 +- 今後新しい redirect builder が増えるなら署名を統一すべき + +**修正案:** `buildSignupPath` の署名を `buildLoginPath` に合わせる： + +```typescript +export function buildSignupPath(url: string | URL | null): string { + // ... +} +``` + +--- + +#### Issue 7: Testing.md の `.resolves` マッチャー説明が不正確 + +**File:** `.claude/rules/testing.md:35-43` + +**現状 (❌ 不完全):** + +```typescript +// ✓ Correct: confirms promise resolves without throwing +await expect(ensureSessionOrRedirect(mockLocals)).resolves; + +// ✗ Avoid: toBeUndefined() is redundant for Promise +await expect(ensureSessionOrRedirect(mockLocals)).resolves.toBeUndefined(); +``` + +**問題:** Vitest では `.resolves` だけではアサーションが完成しない。以下いずれかが必須： + +1. `.resolves.toBeUndefined()` （Promise\ の場合） +2. `await ensureSessionOrRedirect(mockLocals)` （throw しないことで成功） + +**ユーザーコメント:** 「toBeUndefined をつかうことに違和感があってあえて外した」 +→ **これは誤解**。Vitest 構文上、matcher が必須。 + +**修正案:** + +```typescript +// ✓ Option 1: Explicit matcher (Promise) +await expect(ensureSessionOrRedirect(mockLocals)).resolves.toBeUndefined(); + +// ✓ Option 2: No expectation wrapper (rely on throw) +await ensureSessionOrRedirect(mockLocals); // Asserts it doesn't throw +``` + +--- + +#### Issue 8: Users/Edit ハンドラーのエラーマスキング + +**File:** `src/routes/users/edit/+page.server.ts:41-44` + +**現状 (❌):** + +```typescript +try { + const user = await userService.getUser(userId); + // ... +} catch (e) { + redirect(buildLoginPath(...)); // ❌ getUser の失敗を login に誤誘導 +} +``` + +**問題:** `ensureSessionOrRedirect()` で auth 済みなので、catch ブロックは `getUser` の **DB エラー** を捕捉しているはずだが、redirect(login) で隠蔽。リクエストログやエラートレースが失われる。 + +**批判的評価:** + +- 「エラーをログイン画面で隠す」はセキュリティと UX の両面で悪い +- ユーザーは意味不明なログイン画面に誘導される（既にログイン済みなのに） +- 実装者は何が失敗したか分からない + +**修正案:** + +```typescript +try { + const user = await userService.getUser(userId); + // ... +} catch (e) { + console.error('Failed to fetch user:', e); + error(500, 'ユーザー情報の取得に失敗しました。'); +} +``` + +--- + +#### Issue 9: Phase 1 の見出し vs チェックリスト矛盾 + +**File:** Review doc lines 195-199 + +**現状 (❌ 矛盾):** + +```markdown +### Phase 1: CRITICAL & HIGH（✅ 完了） + +- [ ] NPE リスク → null チェックに変更（3 箇所）← Phase 2へ延期 +``` + +**問題:** 見出しは「完了」だが、チェックリストは「未完了（延期）」。どちらが true か不明。 + +**批判的評価:** + +- PR の「完了」判定が曖昧 +- 責任者が Phase 2 に進むべき判断ができない + +**修正案:** いずれかを選択： + +1. 見出しを「Phase 1: 部分完了」に修正 +2. NPE チェックリストを 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](../../AGENTS.md) - [.claude/rules/auth.md - Authentication Rules](../../.claude/rules/auth.md) -- [CodeRabbit Output (1st)](../coderabbit-1st-review.txt) -- [CodeRabbit Output (2nd)](../coderabbit-2nd-review.txt) diff --git a/src/routes/(admin)/account_transfer/+page.server.ts b/src/routes/(admin)/account_transfer/+page.server.ts index a7a9c267d..d07f204af 100644 --- a/src/routes/(admin)/account_transfer/+page.server.ts +++ b/src/routes/(admin)/account_transfer/+page.server.ts @@ -31,7 +31,9 @@ export async function load({ locals, url }) { } export const actions: Actions = { - default: async ({ request }) => { + default: async ({ request, locals, url }) => { + await validateAdminAccess(locals, url); + try { const form = await superValidate(request, zod4(accountTransferSchema)); From 12a53dbf2b8c7654843f7eb3910c6bcaf740d8a0 Mon Sep 17 00:00:00 2001 From: "k.hiro1818" Date: Wed, 6 May 2026 13:40:26 +0000 Subject: [PATCH 14/23] docs: mark Issue 3 as resolved and add completion checklist in review Co-Authored-By: Claude Sonnet 4.6 --- .../2026-05-06/auth-consolidation-review/review.md | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md index 7359446be..fc59cd0c3 100644 --- a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md +++ b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md @@ -157,6 +157,13 @@ let accountTransferMessages: FloatingMessage[] = []; // ❌ 全リクエスト ## 📊 優先度別修正スケジュール +### 完了済み（本レビュー対応） + +```markdown +- [x] Account Transfer actions.default に validateAdminAccess(locals, url) 追加 + → src/routes/(admin)/account_transfer/+page.server.ts +``` + ### Phase 2: Service 層リファクタ（短期） ```markdown @@ -243,7 +250,7 @@ if (loggedInUser && !canDelete(loggedInUser.id, workBook.authorId)) { --- -#### 🚨 Issue 3: Account Transfer Action が admin 認可チェックなし（指摘は正当、修正方法が誤り） +#### ✅ Issue 3: Account Transfer Action が admin 認可チェックなし（対処済み） **File:** `src/routes/(admin)/account_transfer/+page.server.ts:33-68` @@ -258,6 +265,7 @@ await validateAdminAccessForApi(locals); // ❌ +server.ts 用のメソッド ``` auth.md のルール： + - `validateAdminAccess` — **page routes 用**（`+page.server.ts`）、未認証・非 admin どちらも `/login` へ redirect - `validateAdminAccessForApi` — **API routes 用**（`+server.ts`）、`error(401/403)` を throw @@ -270,8 +278,8 @@ export const actions: Actions = { default: async ({ request, locals, url }) => { await validateAdminAccess(locals, url); // ← page route 用 // ... - } -} + }, +}; ``` --- From 00d42d381ea9e99f9567d80cd08cdcf04bd2ff75 Mon Sep 17 00:00:00 2001 From: "k.hiro1818" Date: Wed, 6 May 2026 13:42:23 +0000 Subject: [PATCH 15/23] docs: fix sveltekit rule for form action url param and mark Issue 4 resolved Form action handlers do receive `url` as part of RequestEvent; the old rule incorrectly stated it was unavailable. Co-Authored-By: Claude Sonnet 4.6 --- .claude/rules/sveltekit.md | 14 ++++- .../auth-consolidation-review/review.md | 54 ++----------------- 2 files changed, 15 insertions(+), 53 deletions(-) diff --git a/.claude/rules/sveltekit.md b/.claude/rules/sveltekit.md index 783f241da..b83ecd119 100644 --- a/.claude/rules/sveltekit.md +++ b/.claude/rules/sveltekit.md @@ -51,9 +51,19 @@ if (!(Object.values(TaskGrade) as string[]).includes(gradeRaw)) { const grade = gradeRaw as TaskGrade; // Safe now ``` -## Form Actions: `url` Parameter Unavailable +## Form Actions: `url` Parameter -Form action handlers cannot access `url`. Use `new URL(request.url)` instead. +Form action handlers receive the full `RequestEvent`, including `url`. Destructure it directly: + +```typescript +export const actions = { + default: async ({ request, locals, url }) => { + const redirectTo = url.searchParams.get('redirectTo'); + }, +}; +``` + +`url` is guaranteed non-null. `new URL(request.url)` is an equivalent alternative but unnecessary when `url` is already available. ## Page Component Props diff --git a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md index fc59cd0c3..74be6fde4 100644 --- a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md +++ b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md @@ -286,61 +286,13 @@ export const actions: Actions = { ### 新規追加: MEDIUM 問題（優先度順） -#### Issue 4: SvelteKit ドキュメント記述が不正確 +#### ✅ Issue 4: SvelteKit ドキュメント記述が不正確（対処済み） **File:** `.claude/rules/sveltekit.md:54-56` -**現状 (❌):** - -```markdown -## Form Actions: `url` Parameter Unavailable - -Form action handlers cannot access `url`. Use `new URL(request.url)` instead. -``` - -**問題:** 実際には form actions は `url` を **受け取れる**。ドキュメントが誤り。 - -**例（実際に動作）:** - -```typescript -export const actions = { - default: async ({ request, locals, url }) => { - // ← url は受け取れる - const redirectTo = url.searchParams.get('redirectTo'); - }, -}; -``` - -**批判的評価:** - -- ドキュメント誤りは開発者の混乱を招く -- ただし「new URL(request.url) パターン」も正しいアプローチなので、削除ではなく「両方サポート、url は convenience」と修正すべき -- 実装側との乖離が大 +**指摘（正当）:** `url` は form actions の RequestEvent の標準プロパティとして保証されており、null にならない。公式ドキュメントでも `url.searchParams` を直接使う例が示されている。旧ルール「`url` は受け取れない」は誤りだった。 -**修正案:** - -````markdown -## Form Actions: `url` Parameter Usage - -Form action handlers can access `url` directly: - -```typescript -export const actions = { - default: async ({ request, locals, url }) => { - const redirectTo = url.searchParams.get('redirectTo'); - }, -}; -``` -```` - -または、request から再構築することも可能： - -```typescript -const url = new URL(request.url); - -（両方正しい） - -``` +**対処:** sveltekit.md を修正済み。`url` は destructure して直接使える、`new URL(request.url)` は等価な代替手段と記載。 --- From 6cbfe412a05cb3ed705ca57ce2e22b0fbb94ba4d Mon Sep 17 00:00:00 2001 From: "k.hiro1818" Date: Wed, 6 May 2026 13:45:03 +0000 Subject: [PATCH 16/23] fix: align buildSignupPath signature with buildLoginPath and document it Accept `string | URL | null` (matching buildLoginPath) so callers don't need manual URL.pathname extraction. Document buildSignupPath in auth.md Key Files. Co-Authored-By: Claude Sonnet 4.6 --- .claude/rules/auth.md | 1 + .../auth-consolidation-review/review.md | 47 ++----------------- src/features/auth/utils/signup.ts | 5 +- 3 files changed, 7 insertions(+), 46 deletions(-) diff --git a/.claude/rules/auth.md b/.claude/rules/auth.md index 6246a9c60..6fc8c9b5b 100644 --- a/.claude/rules/auth.md +++ b/.claude/rules/auth.md @@ -39,6 +39,7 @@ paths: - `validateAdminAccess(locals, url?)` — for page routes; redirects to `/login` for both unauthenticated and non-admin users (do not use in `+server.ts`) - `validateAdminAccessForApi(locals)` — for API routes (`+server.ts`); throws `error(401)` if unauthenticated, `error(403)` if not admin - `src/features/auth/utils/login.ts`: `buildLoginPath(url?)` — generates `/login` or `/login?redirectTo=...` +- `src/features/auth/utils/signup.ts`: `buildSignupPath(url?)` — generates `/signup` or `/signup?redirectTo=...` (same signature as `buildLoginPath`) - `prisma/schema.prisma`: User, Session, Key models ## Redirect After Login Pattern diff --git a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md index 74be6fde4..9e50b7318 100644 --- a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md +++ b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md @@ -296,51 +296,10 @@ export const actions: Actions = { --- -#### Issue 5: Signup Helper のドキュメント漏れ +#### ✅ Issue 5 & 6: Signup Helper のドキュメント漏れ・API 非対称（対処済み） -**File:** `.claude/rules/auth.md:41-49` (Redirect After Login Pattern) - -**問題:** `buildSignupPath()` が存在するが、auth.md では `buildLoginPath` のみ記載。signup フロー用の同等ヘルパーが隠れている。 - -**修正案:** auth.md の Key Files セクションに追記： - -```markdown -- `src/features/auth/utils/signup.ts`: `buildSignupPath(url?)` — generates `/signup` or `/signup?redirectTo=...` -``` - ---- - -#### Issue 6: buildSignupPath API が非対称 - -**File:** `src/features/auth/utils/signup.ts:8-14` - -**現状:** - -```typescript -export function buildSignupPath(redirectTo: string | null): string { - // ... -} - -// vs. buildLoginPath: -export function buildLoginPath(url: string | URL | null): string { - // ... -} -``` - -**問題:** `buildLoginPath()` は `URL` を受け入れるが、`buildSignupPath()` は `string` のみ。呼び出し側で手動変換が必要（DRY 違反）。 - -**批判的評価:** - -- 些細だが一貫性欠落 -- 今後新しい redirect builder が増えるなら署名を統一すべき - -**修正案:** `buildSignupPath` の署名を `buildLoginPath` に合わせる： - -```typescript -export function buildSignupPath(url: string | URL | null): string { - // ... -} -``` +- `buildSignupPath` の型シグネチャを `string | URL | null` に統一（`buildLoginPath` と同一） +- `auth.md` の Key Files セクションに `buildSignupPath` を追記 --- diff --git a/src/features/auth/utils/signup.ts b/src/features/auth/utils/signup.ts index acb0682dc..2f83dc5ae 100644 --- a/src/features/auth/utils/signup.ts +++ b/src/features/auth/utils/signup.ts @@ -5,10 +5,11 @@ import { SIGNUP_PAGE } from '$lib/constants/navbar-links'; * @param redirectTo - The decoded path to redirect back to after signup; if not provided, returns the base signup path * @returns The signup path with optional redirectTo query parameter */ -export function buildSignupPath(redirectTo?: string | null): string { +export function buildSignupPath(redirectTo?: string | URL | null): string { if (!redirectTo) { return SIGNUP_PAGE; } - return `${SIGNUP_PAGE}?redirectTo=${encodeURIComponent(redirectTo)}`; + const pathname = redirectTo instanceof URL ? redirectTo.pathname + redirectTo.search : redirectTo; + return `${SIGNUP_PAGE}?redirectTo=${encodeURIComponent(pathname)}`; } From b6f67b21944d4e3bd4bc0fcfa05696aaeb763a1e Mon Sep 17 00:00:00 2001 From: "k.hiro1818" Date: Wed, 6 May 2026 13:51:22 +0000 Subject: [PATCH 17/23] test: fix Promise assertions and document the pattern in testing rule Co-Authored-By: Claude Sonnet 4.6 --- .claude/rules/testing.md | 11 ++++--- .../auth-consolidation-review/review.md | 33 ++----------------- .../auth/services/admin_access.test.ts | 4 +-- src/features/auth/services/session.test.ts | 2 +- 4 files changed, 13 insertions(+), 37 deletions(-) diff --git a/.claude/rules/testing.md b/.claude/rules/testing.md index d2681fa93..8533dcfc1 100644 --- a/.claude/rules/testing.md +++ b/.claude/rules/testing.md @@ -32,14 +32,17 @@ E2E files: **must** use `.spec.ts` extension (`.test.ts` not detected). - Use `toBe(true)` / `toBe(false)` not `toBeTruthy()` / `toBeFalsy()` - DB query tests: assert `orderBy`, `include` with `expect.objectContaining`, not just `where` - Enum membership: `Object.hasOwn(Enum, value)` not `in` (avoids prototype chain) -- `Promise`: use `resolves` alone (not `toBeUndefined()`); `.resolves` confirms the promise resolves without asserting the value +- `Promise`: `.resolves` requires a matcher — without one it does not assert and becomes a false-positive; use `await fn()` to assert no throw, or `.resolves.toBeUndefined()` for explicit form ```typescript -// ✓ Correct: confirms promise resolves without throwing -await expect(ensureSessionOrRedirect(mockLocals)).resolves; +// ✓ Preferred: simplest way to assert Promise does not throw +await ensureSessionOrRedirect(mockLocals); -// ✗ Avoid: toBeUndefined() is redundant for Promise +// ✓ Also correct: explicit resolves form await expect(ensureSessionOrRedirect(mockLocals)).resolves.toBeUndefined(); + +// ✗ Wrong: .resolves without a matcher does not assert anything (false-positive) +await expect(ensureSessionOrRedirect(mockLocals)).resolves; ``` ### Describe Organization diff --git a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md index 9e50b7318..1757c3225 100644 --- a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md +++ b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md @@ -303,37 +303,10 @@ export const actions: Actions = { --- -#### Issue 7: Testing.md の `.resolves` マッチャー説明が不正確 +#### ✅ Issue 7: Testing.md の `.resolves` マッチャー説明が不正確（対処済み） -**File:** `.claude/rules/testing.md:35-43` - -**現状 (❌ 不完全):** - -```typescript -// ✓ Correct: confirms promise resolves without throwing -await expect(ensureSessionOrRedirect(mockLocals)).resolves; - -// ✗ Avoid: toBeUndefined() is redundant for Promise -await expect(ensureSessionOrRedirect(mockLocals)).resolves.toBeUndefined(); -``` - -**問題:** Vitest では `.resolves` だけではアサーションが完成しない。以下いずれかが必須： - -1. `.resolves.toBeUndefined()` （Promise\ の場合） -2. `await ensureSessionOrRedirect(mockLocals)` （throw しないことで成功） - -**ユーザーコメント:** 「toBeUndefined をつかうことに違和感があってあえて外した」 -→ **これは誤解**。Vitest 構文上、matcher が必須。 - -**修正案:** - -```typescript -// ✓ Option 1: Explicit matcher (Promise) -await expect(ensureSessionOrRedirect(mockLocals)).resolves.toBeUndefined(); - -// ✓ Option 2: No expectation wrapper (rely on throw) -await ensureSessionOrRedirect(mockLocals); // Asserts it doesn't throw -``` +- `.resolves` のみでは false-positive になることを公式ドキュメントで確認 +- testing.md・session.test.ts・admin_access.test.ts を修正済み（`await fn()` パターンに統一） --- diff --git a/src/features/auth/services/admin_access.test.ts b/src/features/auth/services/admin_access.test.ts index d351bfc21..056bf528b 100644 --- a/src/features/auth/services/admin_access.test.ts +++ b/src/features/auth/services/admin_access.test.ts @@ -51,7 +51,7 @@ describe('validateAdminAccess', () => { const mockLocals = createMockLocalsWithSession(); vi.mocked(userService.getUser).mockResolvedValue({ role: Roles.ADMIN } as never); - await expect(validateAdminAccess(mockLocals)).resolves; + await validateAdminAccess(mockLocals); }); }); @@ -96,7 +96,7 @@ describe('validateAdminAccessForApi', () => { const mockLocals = createMockLocalsWithSession(); vi.mocked(userService.getUser).mockResolvedValue({ role: Roles.ADMIN } as never); - await expect(validateAdminAccessForApi(mockLocals)).resolves; + await validateAdminAccessForApi(mockLocals); }); }); diff --git a/src/features/auth/services/session.test.ts b/src/features/auth/services/session.test.ts index 10b83e407..7b754eeab 100644 --- a/src/features/auth/services/session.test.ts +++ b/src/features/auth/services/session.test.ts @@ -49,7 +49,7 @@ describe('ensureSessionOrRedirect', () => { test('does not throw when user has valid session', async () => { const mockLocals = createMockLocalsWithValidSession(); - await expect(ensureSessionOrRedirect(mockLocals)).resolves; + await ensureSessionOrRedirect(mockLocals); expect(mockLocals.auth.validate).toHaveBeenCalledTimes(1); }); }); From d1a30feedaae6ee9eb4b8f84cf91fc9b3116a155 Mon Sep 17 00:00:00 2001 From: "k.hiro1818" Date: Wed, 6 May 2026 13:54:33 +0000 Subject: [PATCH 18/23] fix: replace misleading login redirect with 500 error in users/edit load DB errors in getUser() were masked as login redirects, hiding the real failure. Now logs the error and returns INTERNAL_SERVER_ERROR to the caller. Co-Authored-By: Claude Sonnet 4.6 --- .../auth-consolidation-review/review.md | 62 +------------------ src/routes/users/edit/+page.server.ts | 10 ++- 2 files changed, 7 insertions(+), 65 deletions(-) diff --git a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md index 1757c3225..e1c7203bd 100644 --- a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md +++ b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md @@ -310,66 +310,10 @@ export const actions: Actions = { --- -#### Issue 8: Users/Edit ハンドラーのエラーマスキング +#### ✅ Issue 8: Users/Edit ハンドラーのエラーマスキング（対処済み） -**File:** `src/routes/users/edit/+page.server.ts:41-44` - -**現状 (❌):** - -```typescript -try { - const user = await userService.getUser(userId); - // ... -} catch (e) { - redirect(buildLoginPath(...)); // ❌ getUser の失敗を login に誤誘導 -} -``` - -**問題:** `ensureSessionOrRedirect()` で auth 済みなので、catch ブロックは `getUser` の **DB エラー** を捕捉しているはずだが、redirect(login) で隠蔽。リクエストログやエラートレースが失われる。 - -**批判的評価:** - -- 「エラーをログイン画面で隠す」はセキュリティと UX の両面で悪い -- ユーザーは意味不明なログイン画面に誘導される（既にログイン済みなのに） -- 実装者は何が失敗したか分からない - -**修正案:** - -```typescript -try { - const user = await userService.getUser(userId); - // ... -} catch (e) { - console.error('Failed to fetch user:', e); - error(500, 'ユーザー情報の取得に失敗しました。'); -} -``` - ---- - -#### Issue 9: Phase 1 の見出し vs チェックリスト矛盾 - -**File:** Review doc lines 195-199 - -**現状 (❌ 矛盾):** - -```markdown -### Phase 1: CRITICAL & HIGH（✅ 完了） - -- [ ] NPE リスク → null チェックに変更（3 箇所）← Phase 2へ延期 -``` - -**問題:** 見出しは「完了」だが、チェックリストは「未完了（延期）」。どちらが true か不明。 - -**批判的評価:** - -- PR の「完了」判定が曖昧 -- 責任者が Phase 2 に進むべき判断ができない - -**修正案:** いずれかを選択： - -1. 見出しを「Phase 1: 部分完了」に修正 -2. NPE チェックリストを Phase 2 セクションに移動 +- catch ブロックの `redirect(login)` を `error(INTERNAL_SERVER_ERROR, ...)` に変更 +- 不要になった `redirect`・`buildLoginPath`・`TEMPORARY_REDIRECT` の import も削除 --- diff --git a/src/routes/users/edit/+page.server.ts b/src/routes/users/edit/+page.server.ts index 8d6e5cead..b59e54ce0 100644 --- a/src/routes/users/edit/+page.server.ts +++ b/src/routes/users/edit/+page.server.ts @@ -1,5 +1,5 @@ //See https://tech-blog.rakus.co.jp/entry/20230209/sveltekit#%E3%82%B9%E3%83%AC%E3%83%83%E3%83%89%E6%8A%95%E7%A8%BF%E7%94%BB%E9%9D%A2 -import { redirect, fail } from '@sveltejs/kit'; +import { error, fail } from '@sveltejs/kit'; import type { Actions } from './$types'; import type { Roles } from '$lib/types/user'; @@ -8,14 +8,12 @@ import * as userService from '$lib/services/users'; import * as verificationService from '$features/account/services/atcoder_verification'; import { ensureSessionOrRedirect } from '$features/auth/services/session'; -import { buildLoginPath } from '$features/auth/utils/login'; import { BAD_REQUEST, UNAUTHORIZED, FORBIDDEN, INTERNAL_SERVER_ERROR, - TEMPORARY_REDIRECT, } from '$lib/constants/http-response-status-codes'; export async function load({ locals, url }) { @@ -38,9 +36,9 @@ export async function load({ locals, url }) { message: '', openAtCoderTab: url.searchParams.get('tab') === 'atcoder', }; - } catch (error) { - console.error('User lookup failed during session validation', error); - redirect(TEMPORARY_REDIRECT, buildLoginPath(url)); + } catch (e) { + console.error('Failed to fetch user:', e); + error(INTERNAL_SERVER_ERROR, 'ユーザー情報の取得に失敗しました。'); } } From 75e90a5a7c75852246878dea96649830bb9b408e Mon Sep 17 00:00:00 2001 From: "k.hiro1818" Date: Wed, 6 May 2026 13:58:49 +0000 Subject: [PATCH 19/23] docs: condense resolved issues in review to summaries only Co-Authored-By: Claude Sonnet 4.6 --- .../auth-consolidation-review/review.md | 130 ++---------------- 1 file changed, 13 insertions(+), 117 deletions(-) diff --git a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md index e1c7203bd..0d52c6bb7 100644 --- a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md +++ b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md @@ -201,123 +201,19 @@ let accountTransferMessages: FloatingMessage[] = []; // ❌ 全リクエスト ## 🔍 Critical Code Review - レビュー指摘事項の批判的検討 -### 新規追加: HIGH セキュリティ問題（ドキュメント未記載） - -#### ✅ Issue 1: 指摘は誤り — 現状は正しい（将来の要件変更への懸念あり） - -**File:** `src/routes/workbooks/[slug]/+page.server.ts:17, 35` - -**レビューツールの指摘（却下）:** - -> `loggedInUser === null` の場合、未発行 workbook にゲストがアクセス可能。 - -**現状:** このページはログインユーザー専用の設計方針。`getLoggedInUser()` はセッションなしなら必ず redirect を throw するため（`session.ts` のコントラクト）、ゲストは 35 行に到達しない。セキュリティギャップなし。 - -**将来の懸念（ゲスト閲覧を許可したい要件が来た場合）:** - -現在のコードはゲスト対応に変更しにくい構造になっている： - -- `getLoggedInUser()` を `getLoggedInUserOptional()` に変えると、35 行の `canRead(isPublished, loggedInUser.id, authorId)` が型エラー（`userId: string` が null/undefined を受け取れない） -- `canRead()` の型シグネチャ変更 + 35 行の null-guard 調整が同時に必要 -- 42 行の `loggedInUser?.id as string` という型強制キャストも問題になる - -「ゲストでも公開 workbook を見せる」変更は、単純な1行修正では済まない。 - ---- - -#### ✅ Issue 2: 指摘は誤り — 現状は正しい（Phase 2 refactor 時に高リスクの落とし穴あり） - -**File:** `src/routes/workbooks/+page.server.ts:104-121` - -**レビューツールの指摘（却下）:** - -> 匿名リクエストが delete ハンドラーを通過できる。 - -`getLoggedInUser()` はセッションなしなら必ず redirect を throw するため、匿名ユーザーは到達しない。現状セキュリティギャップなし。 - -**Phase 2 refactor 時の高リスク懸念:** - -`getLoggedInUser` を pure 化（`User | null` 返却）すると、119 行の null-guard が問題になる： - -```typescript -if (loggedInUser && !canDelete(loggedInUser.id, workBook.authorId)) { - error(FORBIDDEN, ...); -} -// loggedInUser が null → 条件が false → guard をスキップ → 誰でも削除可能 -``` - -閲覧と違い、**削除は非可逆操作**。Phase 2 で pure 化する際に、この null-guard を `if (!loggedInUser || !canDelete(...))` に修正するか、手前で `ensureSessionOrRedirect` を呼ぶことをセットで行う必要がある。Phase 2 チェックリストに明記しておくこと。 - ---- - -#### ✅ Issue 3: Account Transfer Action が admin 認可チェックなし（対処済み） - -**File:** `src/routes/(admin)/account_transfer/+page.server.ts:33-68` - -**問題（正当）:** - -実際のコードは `actions.default` が `{ request }` のみ受け取っており `locals` を省略している。`load()` の `validateAdminAccess` は form への直接 POST で bypass 可能（load は form action 実行時には呼ばれない）。**unauthenticated POST が task results を複写できる。** - -**レビューツールの修正案（誤り）:** - -```typescript -await validateAdminAccessForApi(locals); // ❌ +server.ts 用のメソッド -``` - -auth.md のルール： - -- `validateAdminAccess` — **page routes 用**（`+page.server.ts`）、未認証・非 admin どちらも `/login` へ redirect -- `validateAdminAccessForApi` — **API routes 用**（`+server.ts`）、`error(401/403)` を throw - -ここは `+page.server.ts` の form action なので `validateAdminAccess` が正しい。 - -**正しい修正案:** - -```typescript -export const actions: Actions = { - default: async ({ request, locals, url }) => { - await validateAdminAccess(locals, url); // ← page route 用 - // ... - }, -}; -``` - ---- - -### 新規追加: MEDIUM 問題（優先度順） - -#### ✅ Issue 4: SvelteKit ドキュメント記述が不正確（対処済み） - -**File:** `.claude/rules/sveltekit.md:54-56` - -**指摘（正当）:** `url` は form actions の RequestEvent の標準プロパティとして保証されており、null にならない。公式ドキュメントでも `url.searchParams` を直接使う例が示されている。旧ルール「`url` は受け取れない」は誤りだった。 - -**対処:** sveltekit.md を修正済み。`url` は destructure して直接使える、`new URL(request.url)` は等価な代替手段と記載。 - ---- - -#### ✅ Issue 5 & 6: Signup Helper のドキュメント漏れ・API 非対称（対処済み） - -- `buildSignupPath` の型シグネチャを `string | URL | null` に統一（`buildLoginPath` と同一） -- `auth.md` の Key Files セクションに `buildSignupPath` を追記 - ---- - -#### ✅ Issue 7: Testing.md の `.resolves` マッチャー説明が不正確（対処済み） - -- `.resolves` のみでは false-positive になることを公式ドキュメントで確認 -- testing.md・session.test.ts・admin_access.test.ts を修正済み（`await fn()` パターンに統一） - ---- - -#### ✅ Issue 8: Users/Edit ハンドラーのエラーマスキング（対処済み） - -- catch ブロックの `redirect(login)` を `error(INTERNAL_SERVER_ERROR, ...)` に変更 -- 不要になった `redirect`・`buildLoginPath`・`TEMPORARY_REDIRECT` の import も削除 - ---- - -### 議論が必要な案件 +### 対処済み（詳細はコミット履歴参照） + +| Issue | 判定 | 対処内容 | +| ----- | ---- | -------- | +| workbooks/[slug] 可視性チェック | 却下 | `getLoggedInUser` が guarantee するため現状正しい。ゲスト閲覧要件が来た場合は `canRead` 型含め変更が必要 | +| workbooks delete 匿名許可 | 却下 | 同上。ただし Phase 2 pure 化時に `if (loggedInUser &&` が穴になる（非可逆削除）— Phase 2 要注意 | +| Account Transfer 認可チェックなし | 修正済 | `actions.default` に `validateAdminAccess(locals, url)` を追加（`validateAdminAccessForApi` は `+server.ts` 専用のため不可） | +| sveltekit.md の `url` 記述誤り | 修正済 | `url` は form actions でも使用可能・non-null。公式ドキュメントで確認済み | +| buildSignupPath ドキュメント漏れ・型非対称 | 修正済 | `auth.md` に追記、型を `string \| URL \| null` に統一 | +| testing.md の `.resolves` 説明誤り | 修正済 | `.resolves` のみは false-positive。`await fn()` パターンに統一 | +| users/edit エラーマスキング | 修正済 | catch 内の `redirect(login)` を `error(500)` に変更 | + +### 未解決・議論が必要な案件 #### ❓ Issue 10: Guest Login の Redirect 対応は必要か？ From 27886d956f1731271e8d01a763450da803a13bec Mon Sep 17 00:00:00 2001 From: "k.hiro1818" Date: Wed, 6 May 2026 13:59:19 +0000 Subject: [PATCH 20/23] chore: fix format (#1582) --- .../auth-consolidation-review/review.md | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md index 0d52c6bb7..3204b10b2 100644 --- a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md +++ b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md @@ -203,15 +203,15 @@ let accountTransferMessages: FloatingMessage[] = []; // ❌ 全リクエスト ### 対処済み（詳細はコミット履歴参照） -| Issue | 判定 | 対処内容 | -| ----- | ---- | -------- | -| workbooks/[slug] 可視性チェック | 却下 | `getLoggedInUser` が guarantee するため現状正しい。ゲスト閲覧要件が来た場合は `canRead` 型含め変更が必要 | -| workbooks delete 匿名許可 | 却下 | 同上。ただし Phase 2 pure 化時に `if (loggedInUser &&` が穴になる（非可逆削除）— Phase 2 要注意 | -| Account Transfer 認可チェックなし | 修正済 | `actions.default` に `validateAdminAccess(locals, url)` を追加（`validateAdminAccessForApi` は `+server.ts` 専用のため不可） | -| sveltekit.md の `url` 記述誤り | 修正済 | `url` は form actions でも使用可能・non-null。公式ドキュメントで確認済み | -| buildSignupPath ドキュメント漏れ・型非対称 | 修正済 | `auth.md` に追記、型を `string \| URL \| null` に統一 | -| testing.md の `.resolves` 説明誤り | 修正済 | `.resolves` のみは false-positive。`await fn()` パターンに統一 | -| users/edit エラーマスキング | 修正済 | catch 内の `redirect(login)` を `error(500)` に変更 | +| Issue | 判定 | 対処内容 | +| ------------------------------------------ | ------ | ---------------------------------------------------------------------------------------------------------------------------- | +| workbooks/[slug] 可視性チェック | 却下 | `getLoggedInUser` が guarantee するため現状正しい。ゲスト閲覧要件が来た場合は `canRead` 型含め変更が必要 | +| workbooks delete 匿名許可 | 却下 | 同上。ただし Phase 2 pure 化時に `if (loggedInUser &&` が穴になる（非可逆削除）— Phase 2 要注意 | +| Account Transfer 認可チェックなし | 修正済 | `actions.default` に `validateAdminAccess(locals, url)` を追加（`validateAdminAccessForApi` は `+server.ts` 専用のため不可） | +| sveltekit.md の `url` 記述誤り | 修正済 | `url` は form actions でも使用可能・non-null。公式ドキュメントで確認済み | +| buildSignupPath ドキュメント漏れ・型非対称 | 修正済 | `auth.md` に追記、型を `string \| URL \| null` に統一 | +| testing.md の `.resolves` 説明誤り | 修正済 | `.resolves` のみは false-positive。`await fn()` パターンに統一 | +| users/edit エラーマスキング | 修正済 | catch 内の `redirect(login)` を `error(500)` に変更 | ### 未解決・議論が必要な案件 From 9702ac3840a6fc947d9635be784e8365723ec41e Mon Sep 17 00:00:00 2001 From: "k.hiro1818" Date: Wed, 6 May 2026 14:12:04 +0000 Subject: [PATCH 21/23] docs: add SvelteKit layout-based alternative for admin auth consolidation Co-Authored-By: Claude Sonnet 4.6 --- .../auth-consolidation-review/review.md | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md index 3204b10b2..a6787cafc 100644 --- a/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md +++ b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md @@ -98,6 +98,26 @@ await ensureAdminOrRedirect(locals); 利点：Service は純粋 + 呼び出し側は冗長性ゼロ +ただし `utils/` は「副作用禁止」の規約があるため、`ensureAdminOrRedirect` の配置先は規約上グレー。 + +**代替案：SvelteKit レイアウト階層による一元化** + +admin ページが `/admin/` 配下に集約されているなら、layout で一度だけ書くことで書き漏れを構造的に防げる： + +```typescript +// src/routes/(admin)/+layout.server.ts +export async function load({ locals, url }) { + const status = await validateAdminStatus(locals); + if (status !== AdminStatus.OK) redirect(TEMPORARY_REDIRECT, buildLoginPath(url)); +} +``` + +- `redirect()` が規約上の正規の場所（route handler）に収まる +- 各ページへの書き漏れリスクがゼロ +- wrapper 関数が不要になるため utils の副作用問題も消える + +制約：admin ルートが複数の route group に散らばっている場合は適用できない。 + --- ### ⚠️ MEDIUM: Module-scope Mutable State From 5fbbc41b2983f62c5467b229b3a3f69e17d1b87a Mon Sep 17 00:00:00 2001 From: "k.hiro1818" Date: Wed, 6 May 2026 14:15:00 +0000 Subject: [PATCH 22/23] refactor: replace ensureSessionOrRedirect with getLoggedInUser in users/edit load Co-Authored-By: Claude Sonnet 4.6 --- src/routes/users/edit/+page.server.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/routes/users/edit/+page.server.ts b/src/routes/users/edit/+page.server.ts index b59e54ce0..39f884086 100644 --- a/src/routes/users/edit/+page.server.ts +++ b/src/routes/users/edit/+page.server.ts @@ -7,7 +7,7 @@ import type { Roles } from '$lib/types/user'; import * as userService from '$lib/services/users'; import * as verificationService from '$features/account/services/atcoder_verification'; -import { ensureSessionOrRedirect } from '$features/auth/services/session'; +import { getLoggedInUser } from '$features/auth/services/session'; import { BAD_REQUEST, @@ -17,16 +17,16 @@ import { } from '$lib/constants/http-response-status-codes'; export async function load({ locals, url }) { - await ensureSessionOrRedirect(locals, url); + const loggedInUser = await getLoggedInUser(locals, url); try { - const user = await userService.getUser(locals.user.name); + const user = await userService.getUser(loggedInUser.name); return { userId: user?.id as string, username: user?.username as string, role: user?.role as Roles, - isLoggedIn: (locals.user.id === user?.id) as boolean, + isLoggedIn: Boolean(loggedInUser && user && loggedInUser.id === user.id), atCoderAccount: { handle: user?.atCoderAccount?.handle ?? '', validationCode: user?.atCoderAccount?.validationCode ?? '', From 15e836576c3c2b47b1afb0d3940aa90220b3cc38 Mon Sep 17 00:00:00 2001 From: "k.hiro1818" Date: Wed, 6 May 2026 14:17:52 +0000 Subject: [PATCH 23/23] test: replace toBeTruthy/toBeFalsy with toBe(true)/toBe(false) in url.test.ts Co-Authored-By: Claude Sonnet 4.6 --- src/lib/utils/url.test.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/lib/utils/url.test.ts b/src/lib/utils/url.test.ts index 0f2218290..7fa190b20 100644 --- a/src/lib/utils/url.test.ts +++ b/src/lib/utils/url.test.ts @@ -28,12 +28,12 @@ describe('URL', () => { ]; runTests('isValidUrl', testCases, ({ rawUrl }: TestCaseForUrlValidation) => { - expect(isValidUrl(rawUrl)).toBeTruthy(); + expect(isValidUrl(rawUrl)).toBe(true); }); }); test('when an empty URL is given', () => { - expect(isValidUrl('')).toBeTruthy(); + expect(isValidUrl('')).toBe(true); }); describe('when invalid URL are given', () => { @@ -48,7 +48,7 @@ describe('URL', () => { ]; runTests('isValidUrl', testCases, ({ rawUrl }: TestCaseForUrlValidation) => { - expect(isValidUrl(rawUrl)).toBeFalsy(); + expect(isValidUrl(rawUrl)).toBe(false); }); }); @@ -78,7 +78,7 @@ describe('URL', () => { ]; runTests('isValidUrlSlug', testCases, ({ rawUrl }: TestCaseForUrlValidation) => { - expect(isValidUrlSlug(rawUrl)).toBeTruthy(); + expect(isValidUrlSlug(rawUrl)).toBe(true); }); }); @@ -117,7 +117,7 @@ describe('URL', () => { ]; runTests('isValidUrlSlug', testCases, ({ rawUrl }: TestCaseForUrlValidation) => { - expect(isValidUrlSlug(rawUrl)).toBeFalsy(); + expect(isValidUrlSlug(rawUrl)).toBe(false); }); });