Conversation
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 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
- Form Actions: `url` Parameter Unavailable (1 line) - Error Handling in load() (1 line) - Auth Audit (1 line) Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughログイン前のページへ戻るリダイレクトを導入し、認証関連を Changes認証統合とリダイレクト実装
Sequence DiagramsequenceDiagram
participant User as ユーザー
participant Browser as ブラウザ
participant Server as アプリサーバ
participant Guard as authサービス
participant DB as セッションDB
User->>Browser: 保護ページへアクセス
Browser->>Server: GET /protected (url)
Server->>Guard: validateAdminAccess(locals, url)
Guard->>DB: セッション検証
DB-->>Guard: セッションなし
Guard-->>Server: 302 -> /login?redirectTo=%2Fprotected
Server-->>Browser: 302 Redirect (login)
Browser->>User: ログインページ表示
User->>Browser: 認証情報送信 (POST /login)
Browser->>Server: POST /login (含む redirectTo)
Server->>Server: isSameOriginRedirect(redirectTo, origin) を検証
Server->>DB: セッション作成
DB-->>Server: 作成成功
Server-->>Browser: 303 -> /protected (validated)
Browser->>Server: GET /protected
Server->>Guard: validateAdminAccess(locals, url)
Guard->>DB: セッション・権限確認
DB-->>Guard: admin OK
Server-->>Browser: 200 Protected page
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/lib/components/AuthForm.svelte (1)
89-92: 🧹 Nitpick | 🔵 Trivialゲストログインがリダイレクト先を無視している (FIXME)
handleLoginAsGuestは常にhomeHrefへ遷移するため、PR の目的（ログイン前のページへ戻る）がゲストログインでは達成されない。buildLoginPath/redirectToの仕組みが整ったので、alternativePageLinkからredirectToパラメータを取り出してgotoに渡すか、フォームのactionに組み込む対応が必要。既存の FIXME を解消する実装を生成するか、新しい Issue を作成しますか？
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/components/AuthForm.svelte` around lines 89 - 92, handleLoginAsGuest always navigates to homeHref, ignoring any intended post-login redirect; update handleLoginAsGuest to extract the redirectTo value (or full path) from alternativePageLink / buildLoginPath and pass it into goto (or construct the login form action with that redirectTo) instead of unconditionally using homeHref so guest login returns the user to the pre-login page; locate the handleLoginAsGuest function and change its navigation call to use the redirect value provided by buildLoginPath / alternativePageLink (falling back to homeHref if missing).src/routes/workbooks/+page.server.ts (1)
104-121:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win削除アクションが未ログインを拒否していません。
getLoggedInUserはこのPR内で nullable として使われています。今の条件だと匿名リクエストは Line 119 の権限チェックを素通りしてdeleteWorkBookに到達できるので、削除前にensureSessionOrRedirectか少なくとも 401/redirect を入れる必要があります。修正案
-import { getLoggedInUser } from '$features/auth/services/session'; +import { ensureSessionOrRedirect } from '$features/auth/services/session'; … delete: async ({ locals, request }) => { const url = new URL(request.url); - const loggedInUser = await getLoggedInUser(locals, url); + await ensureSessionOrRedirect(locals, url); + const loggedInUser = locals.user; … - if (loggedInUser && !canDelete(loggedInUser.id, workBook.authorId)) { + if (!canDelete(loggedInUser.id, workBook.authorId)) { error(FORBIDDEN, `問題集id: ${workBookId} にアクセスする権限がありません。`); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/workbooks/`+page.server.ts around lines 104 - 121, The delete handler currently allows anonymous requests through because getLoggedInUser can be null; update the delete action in +page.server.ts to enforce authentication before any authorization or deletion by calling ensureSessionOrRedirect(locals, url) or returning a 401/redirect when loggedInUser is null, then use the non-null loggedInUser for the canDelete check and before calling workBooksCrud.deleteWorkBook; ensure you reference the existing getLoggedInUser, canDelete, and workBooksCrud.deleteWorkBook symbols and perform the null-check/redirect early in the delete flow.src/routes/workbooks/[slug]/+page.server.ts (1)
35-36:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win未ログインの閲覧拒否が抜けており、未公開問題集を匿名閲覧できます。
Line 35 の条件は
loggedInUserがいる場合しか効かないので、isPublished === falseかつ未ログインでも本文まで到達します。匿名アクセスも含めて可視性を判定してください。修正案
- if (loggedInUser && !canRead(isPublished, loggedInUser.id, authorId)) { + if (!loggedInUser && !isPublished) { + error(FORBIDDEN, `問題集id: ${slug} にアクセスする権限がありません。`); + } + if (loggedInUser && !canRead(isPublished, loggedInUser.id, authorId)) { error(FORBIDDEN, `問題集id: ${slug} にアクセスする権限がありません。`); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/workbooks/`[slug]/+page.server.ts around lines 35 - 36, The current guard only runs when loggedInUser is present, allowing anonymous users to view unpublished workbooks; change the condition so the visibility check runs for all requests by calling canRead with the possibly-undefined user id (use loggedInUser?.id) and reject when canRead(...) returns false — update the if that references loggedInUser, canRead, isPublished, authorId and slug to perform the authorization regardless of loggedInUser so anonymous access to unpublished items is blocked.src/features/auth/services/admin_access.ts (1)
28-47: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
admin_accessは route 層へ寄せた方がいいです。service 層から
redirect()/error()を投げると、SvelteKit 依存がこのレイヤに漏れます。ここはAdminStatusのような純粋な結果だけ返し、+page.server.ts/+server.ts側で redirect/error に変換してください。As per coding guidelines "Service layer must return data or null; never call
error()orredirect(). HTTP error translation belongs in the route handler—services must stay framework-agnostic and unit-testable."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/auth/services/admin_access.ts` around lines 28 - 47, The service currently calls SvelteKit helpers (redirect/error) from validateAdminAccess and validateAdminAccessForApi which leaks framework logic into the service layer; change both functions to be pure and return an AdminStatus instead of redirecting or throwing: update validateAdminAccess(locals, url?) to call validateAdminStatus(locals) and return the AdminStatus (remove redirect/buildLoginPath usage), and update validateAdminAccessForApi(locals) to return the status (remove error(UNAUTHORIZED)/error(FORBIDDEN)); move all redirect/error handling into the route handlers (+page.server.ts / +server.ts) where the returned AdminStatus is translated into redirect(buildLoginPath(url)) or error(UNAUTHORIZED/FORBIDDEN).src/routes/(admin)/account_transfer/+page.server.ts (1)
33-68:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
actions.defaultに管理者アクセス検証が欠落しており、認可バイパスが可能です。
load()はvalidateAdminAccess(locals, url)で守られていますが、POST ハンドラ (actions.default) は保護されていません。SvelteKit の form actions はloadを経由せず直接呼ばれるため、認証済みの一般ユーザが直接 POST リクエストを送信してtaskResultService.copyTaskResultsを実行できる可能性があります。同 PR 内の他の管理ページ (
tasks/+page.server.tsの create/update、vote_management/+page.server.tsの setTaskGrade など) では、各 action 内でvalidateAdminAccess(locals)を呼び出しており、当ファイルと一貫性が取れていません。修正案
export const actions: Actions = { - default: async ({ request }) => { + default: async ({ locals, request, url }) => { + await validateAdminAccess(locals, url); + try { const form = await superValidate(request, zod4(accountTransferSchema));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/`(admin)/account_transfer/+page.server.ts around lines 33 - 68, actions.default is missing an admin authorization check which lets unauthenticated POSTs invoke taskResultService.copyTaskResults; fix by calling the existing validateAdminAccess(locals) at the start of actions.default (mirroring other admin handlers) and aborting/throwing if it fails before calling taskResultService.copyTaskResults, ensuring you reference the request handler (actions.default), validateAdminAccess, and taskResultService.copyTaskResults to locate where to add the check.src/routes/workbooks/create/+page.server.ts (1)
36-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
if (!author)は到達不能なコードです。
getLoggedInUserは、セッション欠如時にredirect()を投げる実装であり、nullを返しません。テストケース（session.test.ts）および既存コメント（problems/[slug]/+page.server.ts）で確認済みです。L40 の null チェックは削除してください。PR メッセージで「try-catch で包んだ」とありますが、このファイルには try-catch がありません。他のルート（users/[username]/+page.server.ts）では try-catch でラップされているので、整合性を取るか、コメントを追加して明記してください。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/workbooks/create/`+page.server.ts around lines 36 - 47, Remove the unreachable null-check for author in actions.default (the `if (!author)` block) because getLoggedInUser always throws a redirect on missing session and never returns null; then make the file consistent with other routes by either wrapping the body of actions.default in a try-catch (as done in users/[username]/+page.server.ts) to explicitly handle unexpected errors, or add a brief comment above the call to getLoggedInUser explaining that it throws redirect on missing session and therefore cannot return null; reference getLoggedInUser and actions.default when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/rules/auth.md:
- Around line 41-49: Add mention of the signup helper to the docs: update the
"Key Files / Redirect After Login Pattern" section to include buildSignupPath
alongside buildLoginPath, noting that src/features/auth/utils/signup.ts exports
buildSignupPath which generates the signup path with the same redirectTo
behavior as buildLoginPath; ensure the pattern description also indicates
validating redirectTo with isSameOriginRedirect() as already noted for
login/signup actions.
In @.claude/rules/sveltekit.md:
- Around line 54-56: The SvelteKit docs in the "Form Actions" rule incorrectly
state that the url parameter is unavailable; update .claude/rules/sveltekit.md
to reflect that form action handlers (e.g., +page.server.ts handlers) do receive
a destructured url parameter and show a concise usage example using url (or, if
you prefer consistency, state the alternative pattern and recommend using new
URL(request.url) across the codebase); mention the specific identifiers "url",
"request", and "new URL(request.url)" so readers can map the guidance to
existing handlers.
In @.claude/rules/testing.md:
- Around line 35-43: The doc currently shows using `await
expect(ensureSessionOrRedirect(mockLocals)).resolves;` for Promise<void>, which
is incorrect because Vitest's `.resolves` requires a matcher; update the text to
state that `.resolves` must be followed by a matcher (e.g.,
`.resolves.toBeUndefined()` for Promise<void>) or suggest the alternative `await
ensureSessionOrRedirect(mockLocals);` to simply assert no throw; update the
example lines and explanatory sentence accordingly and reference the
`ensureSessionOrRedirect` Promise<void> example so readers know which snippet to
change.
In `@docs/dev-notes/2026-05-06/auth-consolidation-review/review.md`:
- Around line 196-199: 見出し「Phase 1: CRITICAL & HIGH（✅ 完了）」と未完了チェックリスト行 "- [ ]
NPE リスク → null チェックに変更（3 箇所）← Phase 2へ延期" が矛盾しているため、見出しを「Phase 1:
部分完了」に変更するか、該当チェックリスト行を Phase 2 セクションへ移動して整合性を取ってください（対象テキストは見出し文言と該当チェックリスト行）。
- Around line 195-216: Phase 1 marked the NPE items as deferred to Phase 2 but
did not create a tracked Issue/PR; create a dedicated Issue (or PR) that tracks
the HIGH NPE work (items referencing loggedInUser!.id and the Phase 2 refactors:
ensureSessionOrRedirect, getLoggedInUser return type,
validateAdminStatus/ensureAdminOrRedirect, validateAdminAccessForApi), then
update this review doc to reference that Issue/PR by number/URL and change the
checklist lines for the three null-check items to point to the new Issue/PR so
the NPE work is clearly linked and can be triaged before merge.
In `@src/features/auth/services/admin_access.test.ts`:
- Around line 50-55: The test uses an incomplete async assertion—change the
`await expect(validateAdminAccess(mockLocals)).resolves;` pattern to a complete
assertion by either adding a matcher like `resolves.toBeUndefined()` or simply
awaiting the call (`await validateAdminAccess(mockLocals)`); update both
occurrences that mock `userService.getUser` with `Roles.ADMIN` (the test around
`validateAdminAccess`) so the test actually verifies success rather than leaving
the assertion unfinished.
In `@src/features/auth/services/session.test.ts`:
- Around line 49-54: The test uses await
expect(ensureSessionOrRedirect(mockLocals)).resolves; which doesn't assert
resolution; change the assertion to include a matcher (e.g. await
expect(ensureSessionOrRedirect(mockLocals)).resolves.toBeUndefined()) so the
Promise resolution is actually verified; keep the existing verify for
mockLocals.auth.validate toHaveBeenCalledTimes(1) and update the test around
ensureSessionOrRedirect and createMockLocalsWithValidSession accordingly.
In `@src/features/auth/services/session.ts`:
- Around line 1-49: サービス層の getLoggedInUser と ensureSessionOrRedirect が直接
redirect() を投げているためフレームワーク依存になっているので、これらから redirect 呼び出しを削除してサービスは純粋にデータ（user
または null）を返すようにリファクタしてください；具体的には getLoggedInUser / ensureSessionOrRedirect
を廃止または書き換えして getSession(locals) / getCurrentUser(locals)（または
getLoggedInUserOptional）を実装し、これらは locals.auth.validate() の結果をそのまま返す（ユーザがいなければ
null）；その後ルート側（+page.server.ts など）や新しい guard (例: requireLoggedInUser(event) を
src/lib/server/guards/) にて null チェックを行い、必要な場合にのみ redirect(TEMPORARY_REDIRECT,
buildLoginPath(url)) を投げるようにしてください。
In `@src/features/auth/utils/signup.ts`:
- Around line 8-14: The buildSignupPath function is API-asymmetric with
buildLoginPath (it currently accepts only string | null while buildLoginPath
accepts string | URL | null), so update buildSignupPath's signature to accept
string | URL | null and normalize a URL argument to the same string form used by
buildLoginPath (e.g., use the URL's pathname+search or equivalent) before
encoding and appending as redirectTo; alternatively, if you prefer not to change
the signature, add a clear JSDoc on buildSignupPath describing that callers must
pass "pathname+search" strings and update tests to match—make sure to reference
buildSignupPath and buildLoginPath when making the change so both APIs remain
consistent.
In `@src/lib/utils/url.test.ts`:
- Around line 30-32: Replace usage of Jest's toBeTruthy()/toBeFalsy() with
strict toBe(true)/toBe(false) in the url tests: update the runTests callback for
isValidUrl (the runTests('isValidUrl', testCases, ({ rawUrl }:
TestCaseForUrlValidation) => { ... }) call) to
expect(isValidUrl(rawUrl)).toBe(true) or .toBe(false) as appropriate, and make
the same change for isValidUrlSlug tests and any other assertions in this file
that use toBeTruthy()/toBeFalsy() (the other occurrences around the same test
blocks noted in the comment). Keep the existing test data and assertion logic
unchanged except for replacing the matcher to the strict boolean form.
In `@src/routes/`(admin)/tags/[tag_id]/+page.server.ts:
- Around line 13-17: tagService.getTag may return an empty array so reading
tags[0] can yield undefined and crash the page; update the load logic in
+page.server.ts to check the result of tagService.getTag (the tags variable) and
if tags.length === 0 (or tags[0] is falsy) throw a 404 error or perform a
redirect instead of returning tag: tags[0], ensuring any downstream code
receives a guaranteed non-null tag; reference tagService.getTag, the tags local,
and the returned tag property when implementing the guard.
In `@src/routes/`(auth)/signup/+page.server.ts:
- Line 30: The form action handler currently accepts and uses the `url`
parameter (in the `default` action) which violates the rule; replace uses of
`url` inside the `default: async ({ request, locals, url }) => {` handler by
removing `url` from the parameter list and creating `const parsedUrl = new
URL(request.url)` at the start of the handler, then update all references
`url.searchParams` -> `parsedUrl.searchParams` and `url.origin` ->
`parsedUrl.origin` (including the referenced block around the current 89-98
lines) so the action derives URL info from the request rather than the injected
`url` param.
In `@src/routes/problems/`[slug]/+page.server.ts:
- Around line 11-16: The code assumes getLoggedInUser(...) is non-null and uses
loggedInUser.id directly; instead call ensureSessionOrRedirect(locals, url) at
the start of load to enforce the protected route, then use locals.user.id for
the task lookup (replace uses of loggedInUser.id in getTaskResult and related
calls). Update the load function to call ensureSessionOrRedirect(locals, url)
before invoking crud.getTaskResult and remove the non-null assertion on
getLoggedInUser there; apply the same change for the other occurrences mentioned
(lines 21-29) that use loggedInUser.id.
In `@src/routes/users/edit/`+page.server.ts:
- Around line 41-44: The catch block that currently redirects to /login hides
downstream failures (e.g., getUser) even though ensureSessionOrRedirect already
handled auth; instead of calling redirect(buildLoginPath(...)), log the error
and surface a 500 so the guard can remain responsible for auth flows—i.e., in
the catch for the user lookup in +page.server.ts (referencing getUser,
ensureSessionOrRedirect, redirect, buildLoginPath, TEMPORARY_REDIRECT), remove
the redirect to login and either rethrow the error or raise a server 500 (use
SvelteKit's error helper or throw) after logging so actual failures are not
masked.
In `@src/routes/workbooks/edit/`[slug]/+page.server.ts:
- Around line 24-25: getLoggedInUser is guaranteed to throw redirect() and
always return a user, so remove redundant null checks and optional chaining:
replace loggedInUser?.role with loggedInUser.role when calling
isAdmin(loggedInUser.role as Roles), and remove any conditional guards like if
(loggedInUser && !canEdit(...))—call canEdit(loggedInUser, ...) directly; update
any related code blocks (e.g., the checks around canEdit and subsequent branches
between the earlier and later occurrences) to assume loggedInUser is non-null
and keep the Roles cast/use as before.
---
Outside diff comments:
In `@src/features/auth/services/admin_access.ts`:
- Around line 28-47: The service currently calls SvelteKit helpers
(redirect/error) from validateAdminAccess and validateAdminAccessForApi which
leaks framework logic into the service layer; change both functions to be pure
and return an AdminStatus instead of redirecting or throwing: update
validateAdminAccess(locals, url?) to call validateAdminStatus(locals) and return
the AdminStatus (remove redirect/buildLoginPath usage), and update
validateAdminAccessForApi(locals) to return the status (remove
error(UNAUTHORIZED)/error(FORBIDDEN)); move all redirect/error handling into the
route handlers (+page.server.ts / +server.ts) where the returned AdminStatus is
translated into redirect(buildLoginPath(url)) or error(UNAUTHORIZED/FORBIDDEN).
In `@src/lib/components/AuthForm.svelte`:
- Around line 89-92: handleLoginAsGuest always navigates to homeHref, ignoring
any intended post-login redirect; update handleLoginAsGuest to extract the
redirectTo value (or full path) from alternativePageLink / buildLoginPath and
pass it into goto (or construct the login form action with that redirectTo)
instead of unconditionally using homeHref so guest login returns the user to the
pre-login page; locate the handleLoginAsGuest function and change its navigation
call to use the redirect value provided by buildLoginPath / alternativePageLink
(falling back to homeHref if missing).
In `@src/routes/`(admin)/account_transfer/+page.server.ts:
- Around line 33-68: actions.default is missing an admin authorization check
which lets unauthenticated POSTs invoke taskResultService.copyTaskResults; fix
by calling the existing validateAdminAccess(locals) at the start of
actions.default (mirroring other admin handlers) and aborting/throwing if it
fails before calling taskResultService.copyTaskResults, ensuring you reference
the request handler (actions.default), validateAdminAccess, and
taskResultService.copyTaskResults to locate where to add the check.
In `@src/routes/workbooks/`[slug]/+page.server.ts:
- Around line 35-36: The current guard only runs when loggedInUser is present,
allowing anonymous users to view unpublished workbooks; change the condition so
the visibility check runs for all requests by calling canRead with the
possibly-undefined user id (use loggedInUser?.id) and reject when canRead(...)
returns false — update the if that references loggedInUser, canRead,
isPublished, authorId and slug to perform the authorization regardless of
loggedInUser so anonymous access to unpublished items is blocked.
In `@src/routes/workbooks/`+page.server.ts:
- Around line 104-121: The delete handler currently allows anonymous requests
through because getLoggedInUser can be null; update the delete action in
+page.server.ts to enforce authentication before any authorization or deletion
by calling ensureSessionOrRedirect(locals, url) or returning a 401/redirect when
loggedInUser is null, then use the non-null loggedInUser for the canDelete check
and before calling workBooksCrud.deleteWorkBook; ensure you reference the
existing getLoggedInUser, canDelete, and workBooksCrud.deleteWorkBook symbols
and perform the null-check/redirect early in the delete flow.
In `@src/routes/workbooks/create/`+page.server.ts:
- Around line 36-47: Remove the unreachable null-check for author in
actions.default (the `if (!author)` block) because getLoggedInUser always throws
a redirect on missing session and never returns null; then make the file
consistent with other routes by either wrapping the body of actions.default in a
try-catch (as done in users/[username]/+page.server.ts) to explicitly handle
unexpected errors, or add a brief comment above the call to getLoggedInUser
explaining that it throws redirect on missing session and therefore cannot
return null; reference getLoggedInUser and actions.default when making the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 43aa17d7-e54f-4eee-8f69-cae4bcb2ebe6
📒 Files selected for processing (45)
.claude/rules/auth.md.claude/rules/coding-style.md.claude/rules/sveltekit.md.claude/rules/testing-e2e.md.claude/rules/testing.mdAGENTS.mddocs/dev-notes/2026-05-06/auth-consolidation-review/review.mde2e/helpers/auth.tse2e/redirect_after_login.spec.tse2e/votes.spec.tse2e/workbook_order.spec.tse2e/workbooks_list.spec.tssrc/features/auth/services/admin_access.test.tssrc/features/auth/services/admin_access.tssrc/features/auth/services/session.test.tssrc/features/auth/services/session.tssrc/features/auth/utils/login.test.tssrc/features/auth/utils/login.tssrc/features/auth/utils/signup.test.tssrc/features/auth/utils/signup.tssrc/lib/components/AuthForm.sveltesrc/lib/components/SubmissionStatus/UpdatingDropdown.sveltesrc/lib/utils/authorship.test.tssrc/lib/utils/authorship.tssrc/lib/utils/url.test.tssrc/lib/utils/url.tssrc/routes/(admin)/account_transfer/+page.server.tssrc/routes/(admin)/tags/+page.server.tssrc/routes/(admin)/tags/[tag_id]/+page.server.tssrc/routes/(admin)/tasks/+page.server.tssrc/routes/(admin)/tasks/[task_id]/+page.server.tssrc/routes/(admin)/vote_management/+page.server.tssrc/routes/(admin)/workbooks/order/+page.server.tssrc/routes/(admin)/workbooks/order/+server.tssrc/routes/(auth)/login/+page.server.tssrc/routes/(auth)/login/+page.sveltesrc/routes/(auth)/signup/+page.server.tssrc/routes/problems/[slug]/+page.server.tssrc/routes/users/[username]/+page.server.tssrc/routes/users/edit/+page.server.tssrc/routes/votes/[slug]/+page.sveltesrc/routes/workbooks/+page.server.tssrc/routes/workbooks/[slug]/+page.server.tssrc/routes/workbooks/create/+page.server.tssrc/routes/workbooks/edit/[slug]/+page.server.ts
💤 Files with no reviewable changes (1)
- src/lib/utils/authorship.ts
Comment on lines
+13
to
+17
| 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], |
Contributor
There was a problem hiding this comment.
tags[0] が undefined になる可能性があります（存在しないタグIDの場合）。
tagService.getTag が空配列を返した場合、tags[0] は undefined となり、ページコンポーネントが tag を非 null として扱っていると実行時エラーになります。
🛡️ 修正案（タグ未存在時の404処理）
+import { error } from '@sveltejs/kit';
+
export async function load({ locals, params, url }) {
await validateAdminAccess(locals, url);
const tasks: Tasks = await taskTagService.getTasks(params.tag_id as string);
const tags: Tags = await tagService.getTag(params.tag_id as string);
+ if (!tags[0]) {
+ error(404, 'Tag not found');
+ }
+
return {
tasks: tasks,
tag: tags[0],
};
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/`(admin)/tags/[tag_id]/+page.server.ts around lines 13 - 17,
tagService.getTag may return an empty array so reading tags[0] can yield
undefined and crash the page; update the load logic in +page.server.ts to check
the result of tagService.getTag (the tags variable) and if tags.length === 0 (or
tags[0] is falsy) throw a 404 error or perform a redirect instead of returning
tag: tags[0], ensuring any downstream code receives a guaranteed non-null tag;
reference tagService.getTag, the tags local, and the returned tag property when
implementing the guard.
Comment on lines
+11
to
16
| 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); | ||
|
|
Contributor
There was a problem hiding this comment.
保護ルートで nullable な取得関数を non-null 前提にしています。
このPR内では getLoggedInUser を公開ページでも使っており、ここだけ loggedInUser.id に直行するのは危険です。未ログイン時に redirect-after-login ではなく 500 になるので、ensureSessionOrRedirect(locals, url) を先に通してから locals.user.id を使ってください。
修正案
-import { getLoggedInUser } from '$features/auth/services/session';
+import { ensureSessionOrRedirect } from '$features/auth/services/session';
…
- const loggedInUser = await getLoggedInUser(locals, url);
- const taskResult = await crud.getTaskResult(params.slug as string, loggedInUser.id);
+ await ensureSessionOrRedirect(locals, url);
+ const taskResult = await crud.getTaskResult(params.slug as string, locals.user.id);
…
- const loggedInUser = await getLoggedInUser(locals, url);
- const userId = loggedInUser.id;
+ await ensureSessionOrRedirect(locals, url);
+ const userId = locals.user.id;Also applies to: 21-29
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/problems/`[slug]/+page.server.ts around lines 11 - 16, The code
assumes getLoggedInUser(...) is non-null and uses loggedInUser.id directly;
instead call ensureSessionOrRedirect(locals, url) at the start of load to
enforce the protected route, then use locals.user.id for the task lookup
(replace uses of loggedInUser.id in getTaskResult and related calls). Update the
load function to call ensureSessionOrRedirect(locals, url) before invoking
crud.getTaskResult and remove the non-null assertion on getLoggedInUser there;
apply the same change for the other occurrences mentioned (lines 21-29) that use
loggedInUser.id.
Comment on lines
+24
to
25
| const loggedInUser = await getLoggedInUser(locals, url); | ||
| const loggedInAsAdmin = isAdmin(loggedInUser?.role as Roles); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
getLoggedInUser は null を返さない契約なので ?. / null ガードは冗長
getLoggedInUser は session が無ければ redirect() を throw し、ユーザを必ず返す契約です（session.ts の JSDoc 参照）。そのため loggedInUser?.role の optional chaining や if (loggedInUser && !canEdit(...)) の null 判定は到達不能パスで、読み手に「null がありうる」と誤解させます。型上も App.Locals['user'] が non-nullable に確定しているなら不要です。
♻️ リファクタ案
- const loggedInUser = await getLoggedInUser(locals, url);
- const loggedInAsAdmin = isAdmin(loggedInUser?.role as Roles);
+ const loggedInUser = await getLoggedInUser(locals, url);
+ const loggedInAsAdmin = isAdmin(loggedInUser.role as Roles);
@@
- if (
- loggedInUser &&
- !canEdit(
+ if (
+ !canEdit(
loggedInUser.id,
workBookWithAuthor.workBook.authorId,
loggedInUser.role as Roles,
workBookWithAuthor.workBook.isPublished,
)
) {Also applies to: 51-59
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/workbooks/edit/`[slug]/+page.server.ts around lines 24 - 25,
getLoggedInUser is guaranteed to throw redirect() and always return a user, so
remove redundant null checks and optional chaining: replace loggedInUser?.role
with loggedInUser.role when calling isAdmin(loggedInUser.role as Roles), and
remove any conditional guards like if (loggedInUser && !canEdit(...))—call
canEdit(loggedInUser, ...) directly; update any related code blocks (e.g., the
checks around canEdit and subsequent branches between the earlier and later
occurrences) to assume loggedInUser is non-null and keep the Roles cast/use as
before.
…iew 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 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…esolved 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…g rule Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/dev-notes/2026-05-06/auth-consolidation-review/review.md`:
- Around line 160-200: Fix the stray missing closing parenthesis in the Phase 2
checklist by appending the missing "）" to the "NPE リスク → null チェックに変更（3 箇所"
entry so it reads correctly, and optionally add a reference link to a tracking
Issue/PR for the HIGH NPE item (e.g., annotate the "NPE リスク" line with an
Issue/PR URL) to improve follow-up; locate the text within the Phase 2 block
header and the exact string "NPE リスク → null チェックに変更（3 箇所" to apply the edits.
In `@src/routes/users/edit/`+page.server.ts:
- Around line 20-29: The code calls locals.user without guaranteeing it exists
after ensureSessionOrRedirect, causing potential TypeErrors and an incorrect
isLoggedIn boolean; update the handler to explicitly check for locals.user (or
call the existing getLoggedInUser() helper from session.ts) before accessing
properties, bail or redirect when no user is present, and compute isLoggedIn as
a strict comparison of both defined ids (e.g., Boolean(loggedInUser && user &&
loggedInUser.id === user.id)); touch references to ensureSessionOrRedirect,
userService.getUser, locals.user, getLoggedInUser, and the isLoggedIn
calculation when making the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 89684b25-3858-4b0f-a51b-faebb05263ee
📒 Files selected for processing (9)
.claude/rules/auth.md.claude/rules/sveltekit.md.claude/rules/testing.mddocs/dev-notes/2026-05-06/auth-consolidation-review/review.mdsrc/features/auth/services/admin_access.test.tssrc/features/auth/services/session.test.tssrc/features/auth/utils/signup.tssrc/routes/(admin)/account_transfer/+page.server.tssrc/routes/users/edit/+page.server.ts
Comment on lines
+160
to
+200
| ### 完了済み（本レビュー対応） | ||
|
|
||
| ```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 時間 | ||
|
|
||
| --- |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
ドキュメントの軽微な指摘（任意）
- Line 170:
（3 箇所の閉じカッコ抜け（）欠落）。 - Phase 2 へ延期した HIGH の NPE 課題について、追跡用 Issue/PR への参照リンクを追記すると後追いが容易です（既出指摘の再掲。必須ではない）。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/dev-notes/2026-05-06/auth-consolidation-review/review.md` around lines
160 - 200, Fix the stray missing closing parenthesis in the Phase 2 checklist by
appending the missing "）" to the "NPE リスク → null チェックに変更（3 箇所" entry so it reads
correctly, and optionally add a reference link to a tracking Issue/PR for the
HIGH NPE item (e.g., annotate the "NPE リスク" line with an Issue/PR URL) to
improve follow-up; locate the text within the Phase 2 block header and the exact
string "NPE リスク → null チェックに変更（3 箇所" to apply the edits.
…tion Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rs/edit load Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
….test.ts Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/dev-notes/2026-05-06/auth-consolidation-review/review.md`:
- Around line 273-274: The two relative links in
docs/dev-notes/2026-05-06/auth-consolidation-review/review.md ("../../AGENTS.md"
and "../../.claude/rules/auth.md") are incorrect from this file's location;
update them to correct root-based relative paths (for example
"../../../../AGENTS.md" and "../../../../.claude/rules/auth.md" or whatever
reaches the repository root) so the links resolve properly, and verify by
opening the updated AGENTS.md and .claude/rules/auth.md targets from the
review.md location.
- Around line 38-44: The example in the load function incorrectly calls
fail(BAD_REQUEST, ...) which is only valid in form actions; update the load
handler to throw an HTTP error instead: replace the fail(...) call with throw
error(401, 'Unauthorized') (ensure the example references the same
loggedInUser/null check and userId assignment and import error from
`@sveltejs/kit` if imports are shown) so readers see the correct load() usage.
In `@src/lib/utils/url.test.ts`:
- Line 43: The comment in src/lib/utils/url.test.ts containing "Note:
記事のセクション(`#マーク`)を含む場合は、2024年8月時点では無効なURLとして処理。ユーザの要望が多ければ、修正。" must be rewritten
in English to comply with source comment rules; locate that inline comment in
url.test.ts and replace it with an equivalent English sentence such as "Note:
URLs containing article sections (# fragments) are treated as invalid as of Aug
2024; adjust if user demand increases." ensuring the meaning is preserved and no
other logic is changed.
- Around line 217-247: Add a regression test ensuring absolute same-origin URLs
are rejected: in the test suite for isSameOriginRedirect, add a case that calls
isSameOriginRedirect('http://localhost:5174/workbooks/bfs', requestOrigin) (or
equivalent origin pair) and expects false, verifying the implementation enforces
"relative-path-only" even for same-origin absolute URLs; update the describe
block for failure cases to include this new test so regressions where absolute
same-origin URLs are allowed will be caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 20f772d7-604d-4e0b-8c3d-fe44587530f2
📒 Files selected for processing (3)
docs/dev-notes/2026-05-06/auth-consolidation-review/review.mdsrc/lib/utils/url.test.tssrc/routes/users/edit/+page.server.ts
Comment on lines
+38
to
+44
| ```typescript | ||
| // ✅ Null チェック後に安全にアクセス | ||
| if (!loggedInUser) { | ||
| return fail(BAD_REQUEST, { error: 'Unauthorized' }); | ||
| } | ||
| const userId = loggedInUser.id; | ||
| ``` |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
SvelteKitの最新ドキュメントで、+page.server.ts の load 関数内で fail(...) を使用可能か確認してください。あわせて fail(...) が許可されるのは form actions のみか確認してください。
💡 Result:
SvelteKitの最新ドキュメントによると、+page.server.tsのload関数内でfail(...)を使用することはできません。load関数ではexpected errorのためにerror(...)をthrowする必要があります[1][2][3][4]。fail(...)はform actions（+page.server.tsのactionsオブジェクト内）専用で、ActionFailureオブジェクトを返すために設計されており、form submissionの失敗時にページを再レンダリングしてformデータを返すために使用されます[5][6][7][4]。load関数でfail(...)をreturnまたはthrowしても、loadの戻り値として正しく扱われず、エラーページが表示されない可能性があります[1][8]。公式ドキュメントのloadセクションではerror(...)のみが言及され、failはform actionsセクションでのみ説明されています[1][5]。
Citations:
- 1: https://kit.svelte.dev/docs/load
- 2: https://svelte.dev/docs/kit/load
- 3: https://kit.svelte.dev/docs/errors
- 4: https://svelte.dev/docs/kit/%40sveltejs-kit
- 5: https://kit.svelte.dev/docs/form-actions
- 6: https://stackoverflow.com/questions/76037679/what-are-the-differences-between-throw-error-and-return-fail-in-sveltekit/76038549#76038549
- 7: https://svelte.dev/docs/kit/form-actions
- 8: load function does not respect returned error status codes sveltejs/kit#1161
load 関数での fail() 使用は SvelteKit では不正です。修正が必須です
ドキュメント内のコード例（lines 38-44）は、load 関数内で fail(BAD_REQUEST, ...) を使用していますが、SvelteKit の公式仕様では fail() は form actions 専用です。load 関数ではこの方法は動作しません。
修正内容:
fail()→throw error(401, 'Unauthorized')に変更
このまま放置するとドキュメント読者が誤った実装に誘導されるため、優先度高で対応してください。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/dev-notes/2026-05-06/auth-consolidation-review/review.md` around lines
38 - 44, The example in the load function incorrectly calls fail(BAD_REQUEST,
...) which is only valid in form actions; update the load handler to throw an
HTTP error instead: replace the fail(...) call with throw error(401,
'Unauthorized') (ensure the example references the same loggedInUser/null check
and userId assignment and import error from `@sveltejs/kit` if imports are shown)
so readers see the correct load() usage.
Comment on lines
+273
to
+274
| - [AGENTS.md - Service Layer Guidelines](../../AGENTS.md) | ||
| - [.claude/rules/auth.md - Authentication Rules](../../.claude/rules/auth.md) |
Contributor
There was a problem hiding this comment.
参考リンクの相対パスが壊れている可能性が高いです
このファイル位置からだと ../../AGENTS.md / ../../.claude/rules/auth.md は到達先がずれます。ルート基準で正しい相対パスに修正してください（例: ../../../../AGENTS.md など）。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/dev-notes/2026-05-06/auth-consolidation-review/review.md` around lines
273 - 274, The two relative links in
docs/dev-notes/2026-05-06/auth-consolidation-review/review.md ("../../AGENTS.md"
and "../../.claude/rules/auth.md") are incorrect from this file's location;
update them to correct root-based relative paths (for example
"../../../../AGENTS.md" and "../../../../.claude/rules/auth.md" or whatever
reaches the repository root) so the links resolve properly, and verify by
opening the updated AGENTS.md and .claude/rules/auth.md targets from the
review.md location.
| const testCases = [ | ||
| { rawUrl: 'htt://' }, | ||
| { rawUrl: 'example.c' }, | ||
| // Note: 記事のセクション(#マーク)を含む場合は、2024年8月時点では無効なURLとして処理。ユーザの要望が多ければ、修正。 |
Contributor
There was a problem hiding this comment.
ソースコードコメントを英語に統一してください
Line 43 のコメントが日本語です。src 配下のコメントは英語ルールに合わせてください。
As per coding guidelines: "{src,prisma}/**/*.{ts,tsx,js,jsx,svelte}: Source code comments in English".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/utils/url.test.ts` at line 43, The comment in
src/lib/utils/url.test.ts containing "Note:
記事のセクション(`#マーク`)を含む場合は、2024年8月時点では無効なURLとして処理。ユーザの要望が多ければ、修正。" must be rewritten
in English to comply with source comment rules; locate that inline comment in
url.test.ts and replace it with an equivalent English sentence such as "Note:
URLs containing article sections (# fragments) are treated as invalid as of Aug
2024; adjust if user demand increases." ensuring the meaning is preserved and no
other logic is changed.
Comment on lines
+217
to
+247
| 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,<script>alert()</script>', 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); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
絶対URL（同一オリジン）拒否の回帰テストが不足しています
redirectTo は「同一オリジンかつ相対パスのみ許可」が要件なので、http://localhost:5174/workbooks/bfs のような同一オリジンの絶対URLを false にするテストを追加してください。ここが未検証だと、実装が「同一オリジンなら absolute も許可」に退行しても検知できません。
追加例
describe('failure cases (should return false)', () => {
+ test('rejects absolute URL even if same origin (relative path only)', () => {
+ expect(isSameOriginRedirect('http://localhost:5174/workbooks/bfs', requestOrigin)).toBe(false);
+ });
+
test('rejects absolute URL to different origin', () => {
expect(isSameOriginRedirect('https://danger.com', requestOrigin)).toBe(false);
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/utils/url.test.ts` around lines 217 - 247, Add a regression test
ensuring absolute same-origin URLs are rejected: in the test suite for
isSameOriginRedirect, add a case that calls
isSameOriginRedirect('http://localhost:5174/workbooks/bfs', requestOrigin) (or
equivalent origin pair) and expects false, verifying the implementation enforces
"relative-path-only" even for same-origin absolute URLs; update the describe
block for failure cases to include this new test so regressions where absolute
same-origin URLs are allowed will be caught.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
close #1582
Summary by CodeRabbit
新機能
テスト
ドキュメント
バグ修正