[2/4] feat(web): Gateway connection settings and session cookies - #77
[2/4] feat(web): Gateway connection settings and session cookies#77immanuel-peter wants to merge 3 commits into
Conversation
Add the Next.js app shell, shadcn UI kit, Compose/Kubernetes web service, and getting-started docs. Feature routes are stubs until later PRs in the stack land connection and resource views. Co-authored-by: Cursor <cursoragent@cursor.com>
Store Gateway URL, API key, and optional admin token in httpOnly session cookies, validate connectivity through the Next.js connection routes, and replace the Settings stub with the real form. Co-authored-by: Cursor <cursoragent@cursor.com>
|
🔍 OpenCodeReview found 39 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| export async function POST(request: Request) { | ||
| const resolved = await resolveConnectionInput(request); |
There was a problem hiding this comment.
[bug · medium]
Unexpected failures from input resolution, probing, or cookie writes are not handled. For example, a cookie write can fail when credentials exceed browser cookie limits, causing Next.js to return a generic 500 rather than the documented JSON response and a user-friendly error. Wrap the handler's async operations in try/catch, log the internal error safely, and return a typed ConnectionApiResponse with an appropriate status. The GET and DELETE handlers have the same error-response concern.
| return NextResponse.json(payload, { status: 400 }); | ||
| } | ||
|
|
||
| const probe = await probeConnection(resolved.credentials); |
There was a problem hiding this comment.
[security · high]
This turns a client-supplied Gateway URL into server-side HTTP requests. Without an authorization boundary and destination validation, callers can use this endpoint to probe services reachable only from the web server (loopback, private networks, or cloud metadata), making it an SSRF primitive. Restrict destinations to trusted Gateway hosts, or resolve and reject loopback/link-local/private addresses (including redirects and DNS rebinding), and protect this route with authentication/network policy if private Gateway addresses must remain supported.
| const NAV = [ | ||
| { href: "/dashboard", label: "Dashboard", icon: LayoutDashboardIcon }, | ||
| { href: "/nodes", label: "Nodes", icon: ServerIcon }, | ||
| { href: "/sandboxes", label: "Sandboxes", icon: BoxIcon }, | ||
| { href: "/snapshots", label: "Snapshots", icon: CameraIcon }, | ||
| { href: "/templates", label: "Templates", icon: LayersIcon }, | ||
| { href: "/settings", label: "Settings", icon: SettingsIcon }, | ||
| ] as const; |
There was a problem hiding this comment.
[maintainability · medium]
These business route paths are hardcoded here, and /dashboard is already repeated by the branding link and the root redirect. A route rename can therefore leave navigation, redirection, and active-state detection inconsistent. Define these paths in a shared typed route map (for example, APP_ROUTES) and consume those constants both here and at other navigation/redirect call sites.
| <Button | ||
| data-slot="alert-dialog-action" | ||
| className={cn(className)} | ||
| {...props} | ||
| /> |
There was a problem hiding this comment.
[bug · medium]
AlertDialogAction is rendered as a plain button, so activating the confirmation action does not invoke the alert dialog's close behavior. Unlike AlertDialogCancel, this leaves an uncontrolled dialog open unless every caller manually manages its state. Compose the button through AlertDialogPrimitive.Close (while retaining the desired button props), so both standard actions dismiss the dialog as expected.
Suggestion:
| <Button | |
| data-slot="alert-dialog-action" | |
| className={cn(className)} | |
| {...props} | |
| /> | |
| <AlertDialogPrimitive.Close | |
| data-slot="alert-dialog-action" | |
| render={<Button className={cn(className)} />} | |
| {...props} | |
| /> |
| import { cn } from "@/lib/utils" | ||
|
|
||
| const alertVariants = cva( | ||
| "group/alert relative grid w-full gap-0.5 rounded-lg border px-2.5 py-2 text-left text-sm has-data-[slot=alert-action]:relative has-data-[slot=alert-action]:pr-18 has-[>svg]:grid-cols-[auto_1fr] has-[>svg]:gap-x-2 *:[svg]:row-span-2 *:[svg]:translate-y-0.5 *:[svg]:text-current *:[svg:not([class*='size-'])]:size-4", |
There was a problem hiding this comment.
[bug · medium]
AlertAction accepts arbitrary content, but the alert always reserves only pr-18 while positioning that content absolutely. A longer/localized action label or multiple controls can therefore overlap the title/description, especially in a narrow container. Please make the action part of the grid flow (or otherwise size the reserved column from its content) instead of relying on a fixed 4.5rem allowance.
| const jar = await cookies(); | ||
| const gatewayUrl = jar.get(SESSION_COOKIE.gatewayUrl)?.value; | ||
| const apiKey = jar.get(SESSION_COOKIE.apiKey)?.value; | ||
| const adminToken = jar.get(SESSION_COOKIE.adminToken)?.value; |
There was a problem hiding this comment.
[maintainability · low]
This cookie-read and conversion logic is duplicated in getStoredConnectionFields, so validation or normalization changes can easily diverge between full-session and partial-update paths. Extract one internal raw/normalized field reader and have both public helpers apply their respective completeness rules to its result.
| if (!gatewayUrl || !apiKey) { | ||
| return null; | ||
| } | ||
|
|
||
| return { | ||
| gatewayUrl: normalizeGatewayUrl(gatewayUrl), |
There was a problem hiding this comment.
[bug · medium]
Completeness is checked before normalization. A forged or legacy truthy cookie such as whitespace or / normalizes to an empty string, yet this function returns a configured session; summaries then report configured: true, while network calls fail with malformed URLs. Normalize and fully parse first, then return null unless the normalized URL and trimmed credentials are valid.
| return { | ||
| gatewayUrl: normalizeGatewayUrl(gatewayUrl), | ||
| apiKey, | ||
| adminToken: adminToken || undefined, | ||
| }; |
There was a problem hiding this comment.
[security · high]
gatewayUrl comes from an unsigned, client-controlled cookie and is returned without URL or destination validation. gatewayFetch subsequently concatenates this value into a server-side fetch, so a caller can forge aenv_gateway_url to probe loopback, link-local/cloud-metadata, or other internal services; the validation in resolveConnectionInput does not protect this read path. Validate at the point of use/read, enforce an authenticated allowlist or explicit private-network policy, and cryptographically protect the session against tampering.
| jar.set(SESSION_COOKIE.apiKey, session.apiKey, options); | ||
| if (session.adminToken) { | ||
| jar.set(SESSION_COOKIE.adminToken, session.adminToken, options); |
There was a problem hiding this comment.
[security · high]
This persists the full API key and privileged admin token directly in browser cookies. httpOnly blocks JavaScript access but does not encrypt the values at rest or protect them from browser-profile theft, cookie/header logging, backups, or replay; these cookies are also attached to every request under /. Prefer an opaque server-side session identifier, or at minimum authenticated encryption with a server-held secret and narrowly scoped cookie settings.
| jar.set(SESSION_COOKIE.apiKey, session.apiKey, options); | ||
| if (session.adminToken) { | ||
| jar.set(SESSION_COOKIE.adminToken, session.adminToken, options); |
There was a problem hiding this comment.
[bug · medium]
There is no encoded-size limit for either credential before writing it as an individual cookie. Browsers and proxies commonly cap each cookie near 4 KiB and may silently discard an oversized Set-Cookie, leaving the API response successful but the stored session absent or partially updated. Reject values that exceed a conservative encoded cookie budget (with a user-facing validation error), or move credentials to server-side session storage.
Refuse redirect-following with auth headers, require a fresh API key when the Gateway URL changes, and return JSON errors from connection routes instead of unhandled 500s. Co-authored-by: Cursor <cursoragent@cursor.com>
| {session | ||
| ? `Connected to ${session.gatewayUrl}.` | ||
| : "Not connected yet."} |
There was a problem hiding this comment.
[bug · medium]
getConnectionSessionSummary() always returns an object, including EMPTY_SESSION_SUMMARY when no connection is configured, so this condition is always truthy and the disconnected state renders Connected to null.. Branch on session.configured (and preferably ensure gatewayUrl is non-null) instead.
Suggestion:
| {session | |
| ? `Connected to ${session.gatewayUrl}.` | |
| : "Not connected yet."} | |
| {session.configured && session.gatewayUrl | |
| ? `Connected to ${session.gatewayUrl}.` | |
| : "Not connected yet."} |
| message: string, | ||
| ): Promise<NextResponse<ConnectionApiResponse>> { | ||
| const payload: ConnectionApiResponse = { | ||
| session: await getConnectionSessionSummary(), |
There was a problem hiding this comment.
[bug · medium]
The error path can repeat the operation that just failed. For example, if GET catches a failure from getConnectionSessionSummary(), errorResponse() calls it again; if it throws again, the handler rejects instead of returning the intended JSON 500 response. Build the error payload with EMPTY_SESSION_SUMMARY, or catch summary retrieval independently and fall back to that value.
| return NextResponse.json(payload, { status: 400 }); | ||
| } | ||
|
|
||
| const probe = await probeConnection(resolved.credentials); |
There was a problem hiding this comment.
[security · high]
This turns a request-controlled gatewayUrl into server-side fetches. parseGatewayUrl only restricts the scheme, so an unauthenticated caller can target loopback, RFC1918/internal services, or cloud metadata addresses and use the returned status/timing for service discovery (and trigger GET endpoints). Restrict destinations using a deployment allowlist and resolved-IP validation (including DNS rebinding protection), or require authorization before allowing arbitrary internal destinations.
| const payload: ConnectionApiResponse = { | ||
| session, | ||
| probe: await probeConnection(resolved.credentials), | ||
| }; |
There was a problem hiding this comment.
[security · high]
This endpoint performs server-side requests to a caller-controlled gatewayUrl. Since URL validation only restricts the scheme to HTTP(S), a caller can probe loopback, link-local/cloud-metadata, or private-network addresses and receive HTTP status/timing details in the response (SSRF). Before calling probeConnection, enforce an allowlist or resolve and reject loopback, link-local, private, multicast, and metadata destinations (including redirects/DNS rebinding), and require appropriate authentication/authorization for this route.
| import { redirect } from "next/navigation"; | ||
|
|
||
| export default function HomePage() { | ||
| redirect("/dashboard"); |
There was a problem hiding this comment.
[maintainability · low]
The dashboard URL is hardcoded here and is also repeated in app-sidebar.tsx. This can drift when routes are renamed. Define the path in a shared route constants module and reuse it for redirects and links.
Suggestion:
| redirect("/dashboard"); | |
| redirect(ROUTES.dashboard); |
| if (nodes && nodes.outcome !== "ok") { | ||
| return "Gateway reachable. Node views are unavailable with the current admin token."; | ||
| } |
There was a problem hiding this comment.
[bug · medium]
This attributes every node-check failure to the admin token, but runCheck maps transport errors, timeouts, and all non-401/403 HTTP responses (including 5xx) to "failed". Those cases will show a misleading credential diagnosis. Use the token-specific message only for outcome === "unauthorized"; provide a general node-endpoint failure summary for "failed".
| return { | ||
| httpOnly: true, | ||
| sameSite: "lax" as const, | ||
| secure: process.env.NODE_ENV === "production", |
There was a problem hiding this comment.
[bug · high]
Production mode unconditionally marks these cookies Secure, but the newly added Docker Compose exposure is plain HTTP on port 3000 (and the base Kubernetes Service also only defines HTTP). On a non-localhost HTTP deployment, browsers will reject/withhold these cookies, so saving a connection appears successful but no session persists. Derive this from an explicit HTTPS configuration and/or ensure every supported production deployment terminates TLS before exposing the app.
| jar.set(SESSION_COOKIE.apiKey, session.apiKey, options); | ||
| if (session.adminToken) { | ||
| jar.set(SESSION_COOKIE.adminToken, session.adminToken, options); |
There was a problem hiding this comment.
[security · high]
This persists the raw gateway API key and privileged admin token as browser cookies. httpOnly prevents JavaScript reads but does not make these credentials opaque or encrypted; they remain bearer credentials stored client-side and are sent with every same-origin request because the path is /. Prefer an opaque, short-lived session ID backed by a server-side credential store (or, if server-side storage is impossible, authenticated encryption with rotation and expiration) so compromise of the browser cookie does not directly disclose reusable gateway credentials.
| } else { | ||
| jar.delete(SESSION_COOKIE.adminToken); | ||
| } |
There was a problem hiding this comment.
[bug · high]
The cookie was created with Path=/, but this deletion does not emit the same path attribute. When invoked from /api/connection, an expiration cookie without Path=/ may only target the API route's default path and leave the original root-scoped admin-token cookie active. Expire it by setting an empty value with the original options and maxAge: 0 (and apply the same approach in clearConnectionSession).
Suggestion:
| } else { | |
| jar.delete(SESSION_COOKIE.adminToken); | |
| } | |
| } else { | |
| jar.set(SESSION_COOKIE.adminToken, "", { ...options, maxAge: 0 }); | |
| } |
| for (const name of Object.values(SESSION_COOKIE)) { | ||
| jar.delete(name); | ||
| } |
There was a problem hiding this comment.
[bug · high]
These cookies were set with Path=/, so deletion must match that path. Since this runs from /api/connection, name-only deletion can leave the root-scoped gateway credentials intact, causing “clear session” to report success while subsequent requests remain authenticated. Reissue each cookie with the original attributes and maxAge: 0.
Suggestion:
| for (const name of Object.values(SESSION_COOKIE)) { | |
| jar.delete(name); | |
| } | |
| const options = sessionCookieOptions(); | |
| for (const name of Object.values(SESSION_COOKIE)) { | |
| jar.set(name, "", { ...options, maxAge: 0 }); | |
| } |
2 participants
What
Add Gateway connection Settings: httpOnly session cookies for Gateway URL / API key / optional admin token, connection validate API routes, and the shared
gatewayFetchclient foundation.Why
Credential handling is the highest-scrutiny part of the Web UI and should be reviewable without the full resource-page surface area.
Related issue
Part of #6.
Scope and non-goals
In scope
web/src/lib/session.tsand connection API routesgatewayFetch, errors, paging, shared typesNon-goals
Design and behavior changes
maxAge(cleared on logout / browser session end)AENV_DEFAULT_GATEWAY_URLprefills the Gateway field when setCompatibility and operations
AENV_DEFAULT_GATEWAY_URLValidation
Commands and results:
Skipped checks and reasons:
Risks and reviewer notes
web/src/lib/session.ts,web/src/lib/api/connection*.ts,web/src/app/api/connection/**Stack
Merge order: #76 → #77 → #78 → #79
Checklist