diff --git a/.claude/rules/auth.md b/.claude/rules/auth.md index 569103cb8..6fc8c9b5b 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,26 @@ 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=...` +- `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 + +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..b83ecd119 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`: @@ -47,6 +51,20 @@ if (!(Object.values(TaskGrade) as string[]).includes(gradeRaw)) { const grade = gradeRaw as TaskGrade; // Safe now ``` +## Form Actions: `url` Parameter + +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 `+page.svelte` accepts only `data` and `form` props (`svelte/valid-prop-names-in-kit-pages`). @@ -81,17 +99,11 @@ 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: - -```typescript -const data = await service.fetch(...).catch(() => []); -``` - -Prevents single service error from crashing entire page. +All async operations must be inside the try-catch block, not before it. Errors from async calls outside propagate unhandled. ## 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 are a critical vulnerability. ## success Flag & message Consistency 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..8533dcfc1 100644 --- a/.claude/rules/testing.md +++ b/.claude/rules/testing.md @@ -32,6 +32,18 @@ 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`: `.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 +// ✓ Preferred: simplest way to assert Promise does not throw +await ensureSessionOrRedirect(mockLocals); + +// ✓ 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/AGENTS.md b/AGENTS.md index 7b394d932..c944564e2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -19,6 +19,34 @@ 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.) + +## 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) 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..a6787cafc --- /dev/null +++ b/docs/dev-notes/2026-05-06/auth-consolidation-review/review.md @@ -0,0 +1,274 @@ +# Auth Feature Consolidation (#1582) - 総合レビューレポート + +**実施日:** 2026-05-06 +**ブランチ:** #1582 +**実施内容:** Security Review + CodeRabbit ×2 + +--- + +## 📋 Executive Summary + +### セキュリティレビュー結論 + +**⚠️ 残存課題（Phase 2 以降）：MEDIUM-HIGH リスク** + +- Service 層が redirect()/error() を呼び出し（設計違反） +- Null チェック不足による NPE リスク（HIGH） + +--- + +## 🔒 Security Findings + +### ⚠️ HIGH: Non-null Assertion での NPE + +**Files:** + +- `src/routes/problems/[slug]/+page.server.ts:12-13, 23-24` +- `src/routes/workbooks/edit/[slug]/+page.server.ts:25` + +**Issue:** + +```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 (✅): Wrapper パターン** + +Service 層は純粋、Route helper で framework 処理を集約： + +```typescript +// src/features/auth/services/admin_access.ts （純粋、テスト可能） +export async function validateAdminStatus(locals: App.Locals): Promise { + // ... +} + +// 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 は純粋 + 呼び出し側は冗長性ゼロ + +ただし `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 + +**File:** `src/routes/(admin)/account_transfer/+page.server.ts:14` + +```typescript +let accountTransferMessages: FloatingMessage[] = []; // ❌ 全リクエスト間で共有 +``` + +**Risks:** + +- 競合状態（複数ユーザーが同時実行時に値が上書き） +- 情報漏洩（ユーザーAの結果がユーザーBに表示） + +**Recommendation:** Superforms の message 機能または cookies を使用してリクエスト単位に管理 + +--- + +## 🐛 CodeRabbit Findings（×2回実施） + +### HIGH Issues（残存） + +| Issue | File | Line | Count | +| --------------------- | ---------------------------------------------------- | ------------ | ------ | +| Null チェック漏れ | `src/routes/problems/[slug]/+page.server.ts` | 12-13, 23-24 | 2 | +| Service で redirect() | `src/features/auth/services/session.ts` | 13-25 | 2 箇所 | +| Service で error() | `src/features/auth/services/admin_access.ts` | 38-48 | 1 | +| Task 型不整合 | `src/routes/(admin)/tasks/[task_id]/+page.server.ts` | 14, 44-45 | 2 | + +--- + +### MEDIUM Issues + +| Issue | File | Line | Detail | +| ------------------------ | ----------------------------------------------------- | ------------- | -------------------------------------------- | +| テストアサーション不完全 | `src/features/auth/services/session.test.ts` | 52 | `.resolves` にマッチャーなし | +| テストアサーション不完全 | `src/features/auth/services/admin_access.test.ts` | 54-55, 99-100 | `.resolves` にマッチャーなし | +| Module-scope mutable | `src/routes/(admin)/account_transfer/+page.server.ts` | 14 | 情報漏洩リスク | +| 型不整合 | `src/routes/(admin)/tags/[tag_id]/+page.server.ts` | 13 | `getTag` が Tags 返却 → Tag 単一返却が正しい | +| E2E パスワード不一致 | `e2e/helpers/auth.ts` | 6-11 | signup 'TestPass123' vs login '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 | + +--- + +## 📊 優先度別修正スケジュール + +### 完了済み（本レビュー対応） + +```markdown +- [x] Account Transfer actions.default に validateAdminAccess(locals, url) 追加 + → src/routes/(admin)/account_transfer/+page.server.ts +``` + +### Phase 2: Service 層リファクタ（短期） + +```markdown +- [ ] NPE リスク → null チェックに変更（3 箇所 +- [ ] ensureSessionOrRedirect → 純粋関数 + Wrapper パターン +- [ ] getLoggedInUser の戻り値型を正確に +- [ ] validateAdminStatus → 純粋関数、ensureAdminOrRedirect Wrapper を提供 +- [ ] validateAdminAccessForApi → 純粋関数化、error() は Route に移動 +``` + +**見積:** 3-4 時間 + テスト修正 | **パターン:** Wrapper 関数で Service 純粋性 + Route 簡潔性を両立 + +### 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 時間 + +--- + +## 🔍 Critical Code Review - レビュー指摘事項の批判的検討 + +### 対処済み（詳細はコミット履歴参照） + +| 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 対応は必要か？ + +**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) 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..056bf528b --- /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 validateAdminAccess(mockLocals); + }); + }); + + 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 validateAdminAccessForApi(mockLocals); + }); + }); + + 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..7b754eeab --- /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 ensureSessionOrRedirect(mockLocals); + 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..4eb97bf92 --- /dev/null +++ b/src/features/auth/services/session.ts @@ -0,0 +1,49 @@ +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. + * + * **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 (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 => { + 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..2f83dc5ae --- /dev/null +++ b/src/features/auth/utils/signup.ts @@ -0,0 +1,15 @@ +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 | URL | null): string { + if (!redirectTo) { + return SIGNUP_PAGE; + } + + const pathname = redirectTo instanceof URL ? redirectTo.pathname + redirectTo.search : redirectTo; + return `${SIGNUP_PAGE}?redirectTo=${encodeURIComponent(pathname)}`; +} 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..d07f204af 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)); @@ -44,7 +31,9 @@ export async function load({ locals }) { } export const actions: Actions = { - default: async ({ request }) => { + default: async ({ request, locals, url }) => { + await validateAdminAccess(locals, url); + try { const form = await superValidate(request, 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..4590f9dad 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; @@ -126,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(); @@ -152,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/(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..32e33e8a9 100644 --- a/src/routes/problems/[slug]/+page.server.ts +++ b/src/routes/problems/[slug]/+page.server.ts @@ -1,30 +1,31 @@ -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 }) { + // 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); 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; + // 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; 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..1af3022c3 100644 --- a/src/routes/users/[username]/+page.server.ts +++ b/src/routes/users/[username]/+page.server.ts @@ -1,22 +1,18 @@ //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 }) { + let loggedInUser: Awaited> | undefined; try { + loggedInUser = await getLoggedInUser(locals, url); const user = await userService.getUser(params.username as string); if (!user) { @@ -43,11 +39,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..39f884086 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'; @@ -7,6 +7,8 @@ import type { Roles } from '$lib/types/user'; import * as userService from '$lib/services/users'; import * as verificationService from '$features/account/services/atcoder_verification'; +import { getLoggedInUser } from '$features/auth/services/session'; + import { BAD_REQUEST, UNAUTHORIZED, @@ -15,20 +17,16 @@ import { } from '$lib/constants/http-response-status-codes'; export async function load({ locals, url }) { - const session = await locals.auth.validate(); - - if (!session) { - redirect(302, '/login'); - } + const loggedInUser = await getLoggedInUser(locals, url); try { - const user = await userService.getUser(session?.user.username as string); + const user = await userService.getUser(loggedInUser.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: Boolean(loggedInUser && user && loggedInUser.id === user.id), atCoderAccount: { handle: user?.atCoderAccount?.handle ?? '', validationCode: user?.atCoderAccount?.validationCode ?? '', @@ -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(302, '/login'); + } catch (e) { + console.error('Failed to fetch user:', e); + error(INTERNAL_SERVER_ERROR, 'ユーザー情報の取得に失敗しました。'); } } 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 @@