feat(web): add login page, cookie session auth, and logout - #578
feat(web): add login page, cookie session auth, and logout#578ligonfei wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved redirect-security, authentication compatibility, endpoint-contract, redirect-preservation, and accessibility findings remain.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds localized web login/logout flows with signed cookie sessions while retaining Basic Auth support.
Changes:
- Adds hybrid cookie and Basic Auth middleware.
- Adds login UI, session/logout endpoints, and logout controls.
- Adds authentication tests and translations.
File summaries
| File | Description |
|---|---|
proxy.ts |
Hybrid authentication, redirects, and route protection. |
package-lock.json |
Updates dependency metadata. |
lib/web-auth.ts |
Implements signed session tokens. |
lib/web-auth.test.mjs |
Tests authentication behavior. |
lib/i18n/messages/zh-CN.ts |
Adds Chinese authentication strings. |
lib/i18n/messages/en.ts |
Adds English authentication strings. |
components/AppShell.tsx |
Adds authentication status and logout controls. |
app/login/page.tsx |
Adds the localized login form. |
app/api/auth/web/session/route.ts |
Handles session status and login. |
app/api/auth/web/logout/route.ts |
Clears session cookies. |
Review details
Suppressed comments (4)
app/api/auth/web/session/route.ts:49
- The login form collects and sends a username, but this handler ignores it and accepts any username when the password matches. That diverges from the documented Basic Auth contract, where the username is fixed to
pi, and lets users create cookie sessions with credentials that Basic Auth would reject. Validatebody.usernameagainstPI_WEB_AUTH_USERNAMEor remove the username field from the web flow.
let body: { password?: string } = {};
try {
body = await request.json();
} catch {
return NextResponse.json(
{ error: "Invalid JSON payload" },
{ status: 400 },
);
}
const inputPassword = typeof body.password === "string" ? body.password : "";
if (inputPassword !== password) {
return NextResponse.json({ error: "Invalid password" }, { status: 401 });
app/api/auth/web/session/route.ts:48
request.json()can return JSONnull; the TypeScript annotation does not validate the runtime value, sobody.passwordthrows for that payload and turns a malformed login request into a 500. Guard the parsed value before reading its fields.
const inputPassword = typeof body.password === "string" ? body.password : "";
if (inputPassword !== password) {
app/api/auth/web/session/route.ts:49
- The unauthenticated login path compares the configured secret with
!==, unlike the existing Basic Auth verifier's constant-time comparison. Repeated remote login attempts can observe string-comparison timing; reuse the constant-time credential verifier (while validating the username) instead of comparing the password directly.
const inputPassword = typeof body.password === "string" ? body.password : "";
if (inputPassword !== password) {
return NextResponse.json({ error: "Invalid password" }, { status: 401 });
app/login/page.tsx:462
- This error is inserted asynchronously without a live-region or alert role, so screen-reader users are not notified when a login attempt fails. Mark the error container as
role="alert"(or provide an equivalent live region).
{error && (
<div
- Files reviewed: 9/10 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| export async function POST(request: NextRequest) { | ||
| const password = process.env.PI_WEB_PASSWORD; | ||
| const passwordRequired = isWebPasswordEnabled(password); | ||
|
|
||
| if (!passwordRequired) { | ||
| return NextResponse.json({ success: true }); | ||
| } |
| const cookieToken = request.cookies.get(PI_WEB_SESSION_COOKIE)?.value; | ||
| const authenticated = isValidSessionToken(cookieToken, password); | ||
|
|
||
| return NextResponse.json({ | ||
| authRequired: true, | ||
| authenticated, |
| {/* 显隐密码切换按钮 */} | ||
| <button | ||
| type="button" | ||
| tabIndex={-1} |
| const redirectParam = request.nextUrl.searchParams.get("redirect"); | ||
| const redirectUrl = | ||
| redirectParam && | ||
| redirectParam.startsWith("/") && | ||
| !redirectParam.startsWith("//") | ||
| ? new URL(redirectParam, request.url) | ||
| : new URL("/", request.url); |
| if (pathname && pathname !== "/") { | ||
| loginUrl.searchParams.set("redirect", pathname + request.nextUrl.search); | ||
| } |
| status: 401, | ||
| headers: { | ||
| "Cache-Control": "no-store", | ||
| }, |
…and fix route export
|
@copilot review |
There was a problem hiding this comment.
🟡 Changes recommended
Critical redirect validation issues and a moderate whitespace-password handling issue remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (8)
Previously missed (1) — in code that hasn't changed since the last review.
app/login/page.tsx:52
isWebPasswordEnabledtreats any non-empty string as a valid password, including whitespace-only values, but this trims the entered value and returns before submitting it. The submit button uses the same trimmed predicate, so a whitespace-onlyPI_WEB_PASSWORDworks with Basic Auth but cannot be used through the new login page; test emptiness without trimming in both places.
if (!password.trim()) return;
app/api/auth/web/logout/route.ts:24
- This only expires
pi_web_session, but the proxy still accepts a browser's cachedAuthorization: Basic ...header. A user who entered through the legacy Basic Auth flow therefore remains authenticated and is immediately redirected back home when this handler navigates to/login, so the new logout control does not actually log them out. Either hide this control for Basic-authenticated sessions or add browser-specific state that makes the proxy stop honoring the cached Basic credentials after logout.
const response = NextResponse.json({ success: true });
response.cookies.set({
name: PI_WEB_SESSION_COOKIE,
value: "",
httpOnly: true,
secure: isHttps,
sameSite: "lax",
path: "/",
maxAge: 0,
});
app/login/page.tsx:6
useEffectis imported but never referenced in this new page. Remove the unused import so the page remains lint-clean.
useCallback,
components/AppShell.tsx:85
- Because
proxy.tsaccepts a validAuthorizationheader as authenticated, a browser using the backward-compatible Basic Auth flow remains authenticated after this clears the cookie: the subsequent/loginnavigation is immediately redirected back to/. Hide or disable this control for Basic-auth sessions, or otherwise provide an auth flow that can actually terminate that browser authentication state.
await fetch("/api/auth/web/logout", { method: "POST" });
} catch {}
window.location.href = "/login";
components/AppShell.tsx:85
- Using
hrefleaves the authenticated application entry in browser history and may allow it to be restored from the back/forward cache after the cookie is cleared, exposing the previously rendered session to the next person using the browser. Replace the current entry when leaving after logout.
window.location.href = "/login";
lib/web-auth.ts:93
>leaves a token valid for the whole second in whichDate.now() / 1000equalsexpiresAt, so it remains usable after its declared expiration (and a zero-age token can be accepted briefly). Use>=so the signed expiry and cookie max-age have the same boundary.
if (Math.floor(Date.now() / 1000) > expiresAt) return false;
proxy.ts:61
- The added tests cover the pure token helpers, but not the new proxy/route integration: page redirects, API 401 responses, Basic-versus-cookie acceptance, or cookie set/clear attributes. Since these paths define the authentication boundary, add focused integration tests before relying on the helper tests alone.
const authorization = request.headers.get("authorization");
const cookieToken = request.cookies.get(PI_WEB_SESSION_COOKIE)?.value;
const isAuthenticated = isValidWebAuth(authorization, cookieToken, password);
proxy.ts:104
- Because this matcher now covers all non-static paths, it also intercepts Next's development HMR endpoint (
/_next/webpack-hmr). WithPI_WEB_PASSWORDenabled, the unauthenticated login page receives the/loginredirect instead of the HMR stream, so edits to the new login page do not hot-reload duringnpm run dev; exclude this internal endpoint from the matcher.
"/((?!_next/static|_next/image|favicon.ico|manifest.webmanifest|icons/|offline.html|sw.js).*)",
- Files reviewed: 10/11 changed files
- Comments generated: 2
- Review effort level: Lite
| function getSafeRedirectUrl(): string { | ||
| try { | ||
| const urlParams = new URLSearchParams(window.location.search); | ||
| const redirect = urlParams.get("redirect"); | ||
| if ( | ||
| redirect && | ||
| redirect.startsWith("/") && | ||
| !redirect.startsWith("//") && | ||
| !redirect.includes("\\") | ||
| ) { | ||
| return redirect; | ||
| } | ||
| } catch { | ||
| // 忽略异常 | ||
| } | ||
| return "/"; | ||
| } |
| function isSafeInternalPath(path: string | null | undefined): boolean { | ||
| if (!path || typeof path !== "string") return false; | ||
| return ( | ||
| path.startsWith("/") && | ||
| !path.startsWith("//") && | ||
| !path.includes("\\") && | ||
| !path.includes("\0") | ||
| ); | ||
| } |
Summary
Adds a web login page and cookie-based session authentication for
PI_WEB_PASSWORDprotection, while keeping full backward compatibility with HTTP Basic Auth.Changes
/login): Dedicated login interface with light/dark theme and English/Chinese (zh-CN) localization. Redirects back to the requested path after authentication.lib/web-auth.ts.proxy.tshandles both session cookies (browser) and Basic Auth (API/CLI).POST /api/auth/web/login: Validates credentials and sets HttpOnly session cookie.POST /api/auth/web/logout: Clears session cookie.Verification
npm test(588 tests passing).npm run buildpassed cleanly.