Epic 9: Bug fixes & core workflow stability#55
Conversation
…signIn NextAuth v5's signIn() with redirect:false extracts error from the response URL's query params. Without an explicit callbackUrl, it defaults to window.location.href which on the session-expired page contains error=SessionExpired — causing successful logins to be misreported as failures.
There was a problem hiding this comment.
Pull request overview
This PR addresses the session-expiry re-authentication flow in the login page by explicitly passing callbackUrl to NextAuth signIn(), and updates supporting tests and planning docs for Epic 9 completion.
Changes:
- Pass
callbackUrlintosignIn('credentials', ...)on the login page. - Update login page test expectations to include
callbackUrl. - Mark Epic 9 retrospective as done and add the Epic 9 retrospective document.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| frontend/src/app/(auth)/login/page.tsx | Forwards callbackUrl into credentials signIn() and continues to navigate to callbackUrl on success |
| frontend/src/app/(auth)/login/tests/page.test.tsx | Updates test to assert callbackUrl is included in the signIn() call |
| docs/planning/sprint-status.yaml | Marks epic-9-retrospective as done |
| docs/planning/retrospectives/epic-9-retro-2026-03-10.md | Adds Epic 9 retrospective write-up |
Comments suppressed due to low confidence (1)
frontend/src/app/(auth)/login/page.tsx:55
callbackUrlultimately comes from a query string and is now forwarded to NextAuth and used for navigation. The current validation (startsWith('/')) still allows protocol-relative URLs like//evil.com(including encoded%2F%2Fevil.com), which can create an open-redirect. Tighten the allowlist to only accept internal paths (e.g., ensure it starts with exactly one/and does not begin with//, and consider rejecting values that parse as absolute URLs).
const result = await signIn('credentials', {
email: data.email,
password: data.password,
redirect: false,
callbackUrl,
});
if (result?.error) {
setError('Invalid email or password');
} else {
router.push(callbackUrl);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await user.click(screen.getByRole("button", { name: /sign in/i })); | ||
|
|
||
| await waitFor(() => { | ||
| expect(mockSignIn).toHaveBeenCalledWith("credentials", { | ||
| email: "test@example.com", | ||
| password: "password123", | ||
| redirect: false, | ||
| callbackUrl: "/", | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Tests cover the default callbackUrl of /, but the new behavior (reading callbackUrl from the URL and sanitizing it) isn’t exercised. Add cases that set useSearchParams to include a valid internal callback (e.g. /applications) and a malicious/external one (e.g. //evil.com or an encoded variant) and assert the sanitized value is what gets passed to signIn/router.push.
Addresses Copilot review: tests for valid internal callbackUrl, protocol- relative open redirect (//evil.com), absolute external URL, and session expired message display. Also tightens validation to reject // prefixed URLs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Epic 9: Bug Fixes & Core Workflow Stability | ||
| epic-9: backlog | ||
| 9-1-fix-application-status-transitions-and-draft: done | ||
| 9-2-fix-interview-status-tracking-and-dashboard: done | ||
| 9-3-fix-session-expiry-redirect: done | ||
| 9-4-application-form-error-display: done | ||
| epic-9-retrospective: done |
There was a problem hiding this comment.
epic-9 is still marked as backlog even though all Epic 9 stories and the retrospective are marked done. This makes the sprint-status tracking inconsistent with the status definitions in this file and with earlier epics (which are set to contexted once worked). Consider updating epic-9 to contexted to reflect that the epic has been actively worked/completed.
Summary
signOut({ redirect: false })+ manual navigation at all 3 call sites, callbackUrl validationcallbackUrlto NextAuthsignIn()to prevent URLerrorparam from being misinterpreted as a sign-in failureTest plan
pnpm buildsucceeds (strict TypeScript)/login?error=SessionExpired&callbackUrl=%2Fapplications, enter valid credentials, verify login succeeds and redirects to/applications/loginstill works normally