Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions docs/web-server.md
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,29 @@ stdout, and refuses any request that does not present it as
`Authorization: Bearer <token>`. Public paths: `/health/ping`,
`/health/auth-info`, `/openapi.json`, `/docs`, `/redoc`.

### Session cookie in single-process UI mode

With `kbagent serve --ui`, the browser never sees the bearer token:
`GET /` (and `GET /index.html`) answers the SPA shell with a
`Set-Cookie: kbagent_session=<token>; HttpOnly; SameSite=Strict; Path=/`
session cookie, and the auth middleware accepts that cookie whenever no
`Authorization` header is present. Scripted callers keep using the header.

Two layers keep that cookie from going stale across server restarts
*(since vNEXT)* — previously a restart (new token) could leave a tab that
reloaded from the browser cache silently 401-ing on every API call, with
each list rendering as empty:

- The shell is served with `Cache-Control: no-cache`, so a reload always
revalidates against the server — and the bootstrap route always answers
a full `200` with a fresh `Set-Cookie`.
- The SPA's API client treats a `401` as "cookie may be stale": it
re-fetches `/` once with `cache: "reload"` (bypassing every cache
layer), retries the request, and only if the retry still answers `401`
shows a visible **Session expired** banner (for `SESSION_EXPIRED` /
`SESSION_NOT_FOUND` the banner carries the server message, which names
the on-host `kbagent auth login` remedy).

### What's-new popup *(since vNEXT)*

The web UI shows a curated per-version highlights modal on load, once per
Expand Down
42 changes: 30 additions & 12 deletions src/keboola_agent_cli/server/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -747,19 +747,26 @@ def _install_ui(app: FastAPI, *, ui_dist: str, token: str) -> None:
(auth doesn't care about path, but PUBLIC_PATHS exact-matches do).
2) **Cookie-setting** ``GET /`` and ``GET /index.html``: read the built
``index.html``, return it with a ``Set-Cookie: kbagent_session=<token>;
HttpOnly; SameSite=Strict; Path=/`` header. Public (no auth) so the
SPA can bootstrap. The browser then attaches the cookie to every
same-origin REST + SSE request automatically. The token is HttpOnly
(no JS access -- XSS-resistant), SameSite=Strict (no cross-origin
sends -- CSRF-resistant), and lives only for the browser session.
HttpOnly; SameSite=Strict; Path=/`` header and ``Cache-Control:
no-cache`` (revalidate-always -- a cached shell served without a
request would keep a stale cookie alive across server restarts).
Public (no auth) so the SPA can bootstrap. The browser then attaches
the cookie to every same-origin REST + SSE request automatically. The
token is HttpOnly (no JS access -- XSS-resistant), SameSite=Strict
(no cross-origin sends -- CSRF-resistant), and lives only for the
browser session.

This replaces the older "inject ``window.__KBAGENT_TOKEN`` into a
``<script>`` tag" approach. The injected token landed in the JS heap
(XSS-readable) and the EventSource fallback (``?_kbagent_token=...``
query param) put it into uvicorn's access log -- both attack surfaces
are gone with the cookie-only design.
3) **StaticFiles mount at ``/``** with ``html=True`` so missing paths fall
through to ``index.html`` for SPA client-side routing.
3) **StaticFiles mount at ``/``** serving the built assets. The SPA is
hash-routed (deep links are ``/#/jobs/...``), so every shell load goes
through the ``GET /`` route above; the mount only ever serves real
files. (``html=True`` is kept for directory-index behavior -- note
Starlette's not-found fallback serves ``404.html``, which a Vite build
does not emit, so unknown non-API paths answer 404, not the shell.)

The mount is appended *after* all API routers, so any registered route
(``/projects``, ``/configs``, ``/agents``, ...) wins over a hypothetical
Expand Down Expand Up @@ -811,6 +818,15 @@ async def dispatch(self, request, call_next): # type: ignore[override]
async def _serve_ui_index() -> HTMLResponse:
html = (dist / "index.html").read_text(encoding="utf-8")
response = HTMLResponse(html)
# The shell is the cookie-delivery vehicle, so a browser must never
# satisfy a reload from its cache without asking the server: after a
# `kbagent serve` restart (new bearer token) a heuristically-cached
# index.html boots the SPA with the stale cookie and every /api/*
# call answers 401 with nothing visibly wrong. `no-cache` means
# "store, but revalidate every time" -- and since this route
# implements no conditional-request handling, revalidation is always
# a full 200 that re-sets the cookie below.
response.headers["Cache-Control"] = "no-cache"
# Browser session cookie: HttpOnly + SameSite=Strict + Path=/. No
# ``Secure`` flag because kbagent serve defaults to plain http on
# 127.0.0.1; setting Secure would prevent the cookie from ever being
Expand All @@ -828,9 +844,9 @@ async def _serve_ui_index() -> HTMLResponse:
)
return response

# SPA fallback + assets. ``html=True`` makes StaticFiles serve index.html
# for unknown paths (so /workspaces, /jobs, etc. client-side routes work
# on direct navigation). The auth middleware will still gate API calls;
# Assets + directory-index. The SPA is hash-routed, so client-side
# routes never reach this mount as paths -- it serves the built files
# only. The auth middleware will still gate API calls;
Comment thread
padak marked this conversation as resolved.
# static files are served before it sees them only because StaticFiles
# is the LAST mount and middleware runs on the unified scope -- so we
# widen PUBLIC_PATHS via prefix logic in the auth middleware itself.
Expand All @@ -846,8 +862,10 @@ def _allow_static_through_auth(app: FastAPI) -> None:

The auth middleware exempts ``PUBLIC_PATHS`` (docs, openapi, health). In UI
mode the SPA also needs ``GET /``, ``GET /index.html``, ``GET /assets/*``,
favicons, and the SPA's client-side routes (which resolve to index.html via
the StaticFiles ``html=True`` fallback) to load without a token.
favicons, and any path matching no registered endpoint to load without a
token. (The SPA is hash-routed, so its client-side routes never reach the
server as paths; an unknown path 404s from the static mount -- the property
that matters here is that it is *not* auth-walled into a 401.)

Route-aware (GHSA-ffpq-prmh-3gx2): a real endpoint must authenticate; only
genuine client-side SPA routes fall through to the public index.html shell.
Expand Down
33 changes: 33 additions & 0 deletions tests/test_serve_ui.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,39 @@ def test_assets_served_unauthenticated(self, tmp_path: Path, ui_dist: Path) -> N
assert "console.log" in resp.text


class TestUiShellCaching:
"""The SPA shell must never be trusted to a browser cache without revalidation.

Live-observed during the PR #665 UI audit: after a ``kbagent serve --ui``
restart (new bearer token), a browser reload served the *cached*
``index.html`` without contacting the server. No request means no fresh
``Set-Cookie``, so the stale session cookie 401'd every ``/api/*`` call
and the SPA silently rendered empty lists. ``Cache-Control: no-cache``
forces the browser to revalidate the shell on every load -- and because
the bootstrap route always answers a full 200 (it implements no
conditional-request handling), every revalidation re-sets the cookie.
"""

def test_shell_responses_always_revalidate(self, tmp_path: Path, ui_dist: Path) -> None:
client = _make_client(tmp_path, ui_dist=ui_dist)
for path in ("/", "/index.html"):
resp = client.get(path)
assert resp.status_code == 200, path
assert resp.headers.get("cache-control") == "no-cache", (
f"GET {path} must answer 'Cache-Control: no-cache' so a browser "
"revalidates the shell (and picks up a fresh session cookie) "
"after a server restart"
)

def test_assets_keep_default_caching(self, tmp_path: Path, ui_dist: Path) -> None:
# Build assets are content-hashed by Vite (a new build means new
# URLs), so the no-cache stamp is scoped to the shell only.
client = _make_client(tmp_path, ui_dist=ui_dist)
resp = client.get("/assets/main.js")
assert resp.status_code == 200
assert resp.headers.get("cache-control") != "no-cache"


class TestApiAlias:
def test_api_prefix_routes_to_bare_endpoint(self, tmp_path: Path, ui_dist: Path) -> None:
client = _make_client(tmp_path, ui_dist=ui_dist, token="t")
Expand Down
151 changes: 151 additions & 0 deletions web/frontend/src/api/client.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
/**
* Tests for the 401 self-heal path in the API client.
*
* In single-process UI mode (`kbagent serve --ui`) auth rides on the
* HttpOnly `kbagent_session` cookie set by `GET /`. After a server restart
* the cookie is stale, every API call answers 401, and -- before this fix --
* the SPA silently rendered empty lists. The client now re-fetches the shell
* once (`cache: "reload"` so no browser cache can swallow the request, which
* is exactly how the bug happened in the first place), retries the request,
* and only then surfaces a visible "session expired" signal.
*
* Runs in the default vitest node environment: `window` is stubbed with a
* real EventTarget so `dispatchEvent`/`addEventListener` behave like the
* browser's, and `fetch` is a vi.fn() -- no jsdom dependency needed.
*/
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
import { api, ApiError, SESSION_EXPIRED_EVENT } from "./client";

function jsonResponse(status: number, body: unknown): Response {
return new Response(JSON.stringify(body), {
status,
headers: { "content-type": "application/json" },
});
}

function unauthorized(message = "Invalid Bearer token."): Response {
return jsonResponse(401, {
status: "error",
error: { code: "UNAUTHORIZED", message },
});
}

function shellResponse(): Response {
return new Response("<!doctype html>", {
status: 200,
headers: { "content-type": "text/html" },
});
}

/** URL of a fetch call, whether invoked with a string or a Request. */
function calledUrl(input: unknown): string {
return typeof input === "string" ? input : (input as Request).url;
}

beforeEach(() => {
const fakeWindow = new EventTarget() as unknown as Window & typeof globalThis;
(fakeWindow as unknown as { location: { origin: string } }).location = {
origin: "http://127.0.0.1:8001",
};
vi.stubGlobal("window", fakeWindow);
});

afterEach(() => {
vi.unstubAllGlobals();
});

describe("request 401 retry", () => {
it("re-bootstraps the session cookie and retries once after a 401", async () => {
const fetchMock = vi
.fn<typeof fetch>()
.mockResolvedValueOnce(unauthorized()) // GET /api/projects -> stale cookie
.mockResolvedValueOnce(shellResponse()) // GET / -> fresh Set-Cookie
.mockResolvedValueOnce(jsonResponse(200, { projects: [] })); // retry
vi.stubGlobal("fetch", fetchMock);

await expect(api.get("/projects")).resolves.toEqual({ projects: [] });

expect(fetchMock).toHaveBeenCalledTimes(3);
const [shellUrl, shellInit] = fetchMock.mock.calls[1];
expect(calledUrl(shellUrl)).toBe("/");
// cache: "reload" is the load-bearing part -- a plain fetch("/") could be
// answered from the very browser cache that made the cookie go stale.
expect(shellInit).toMatchObject({ cache: "reload", credentials: "include" });
});

it("dispatches a session-expired event when the retry still 401s", async () => {
const fetchMock = vi
.fn<typeof fetch>()
.mockResolvedValueOnce(unauthorized())
.mockResolvedValueOnce(shellResponse())
.mockResolvedValueOnce(unauthorized("Invalid Bearer token."));
vi.stubGlobal("fetch", fetchMock);
const events: CustomEvent[] = [];
window.addEventListener(SESSION_EXPIRED_EVENT, (evt) => {
events.push(evt as CustomEvent);
});

await expect(api.get("/projects")).rejects.toMatchObject({ status: 401 });

// Exactly one retry -- no loop of shell re-fetches on a genuinely
// broken session.
expect(fetchMock).toHaveBeenCalledTimes(3);
expect(events).toHaveLength(1);
expect(events[0].detail).toMatchObject({
code: "UNAUTHORIZED",
message: "Invalid Bearer token.",
});
});

it("does not retry or dispatch on non-401 errors", async () => {
const fetchMock = vi
.fn<typeof fetch>()
.mockResolvedValueOnce(
jsonResponse(502, {
status: "error",
error: { code: "API_ERROR", message: "upstream down" },
}),
);
vi.stubGlobal("fetch", fetchMock);
const events: Event[] = [];
window.addEventListener(SESSION_EXPIRED_EVENT, (evt) => events.push(evt));

await expect(api.get("/projects")).rejects.toBeInstanceOf(ApiError);

expect(fetchMock).toHaveBeenCalledTimes(1);
expect(events).toHaveLength(0);
});

it("shares a single shell re-fetch across concurrent 401s", async () => {
let releaseShell: (() => void) | undefined;
const shellGate = new Promise<void>((resolve) => {
releaseShell = resolve;
});
const seen401 = new Set<string>();
let shellFetches = 0;
const fetchMock = vi.fn<typeof fetch>((input) => {
const url = calledUrl(input);
if (url === "/") {
shellFetches += 1;
return shellGate.then(shellResponse);
}
if (!seen401.has(url)) {
seen401.add(url);
return Promise.resolve(unauthorized());
}
return Promise.resolve(jsonResponse(200, { ok: url }));
});
vi.stubGlobal("fetch", fetchMock);

const inFlight = Promise.all([api.get("/projects"), api.get("/jobs")]);
// Let both requests hit their 401 and pile onto the shell fetch.
await new Promise((resolve) => setTimeout(resolve, 0));
releaseShell?.();

await expect(inFlight).resolves.toEqual([
{ ok: "/api/projects" },
{ ok: "/api/jobs" },
]);
expect(shellFetches).toBe(1);
});
});
54 changes: 54 additions & 0 deletions web/frontend/src/api/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,45 @@ export class ApiError extends Error {

const API_BASE = "/api";

/**
* Fired on `window` when an API call still answers 401 after the session
* cookie has been re-bootstrapped. At that point the client cannot
* self-heal; the shell shows a visible "session expired" banner instead of
* letting every list silently render empty.
*/
export const SESSION_EXPIRED_EVENT = "kbagent:session-expired";

export interface SessionExpiredDetail {
code: string;
message: string;
}

let shellRefresh: Promise<void> | null = null;

/**
* Re-fetch the SPA shell to pick up a fresh `kbagent_session` cookie.
*
* `GET /` is the cookie-setting bootstrap route in single-process UI mode.
* `cache: "reload"` is load-bearing: a plain `fetch("/")` could be answered
* from the browser cache without any request reaching the server -- which is
* exactly how the cookie went stale in the first place (a reload after a
* `kbagent serve` restart served the cached shell, so no fresh Set-Cookie
* ever happened).
*
* Single-flight: a burst of parallel queries that all 401 shares one
* bootstrap fetch. Failures are swallowed -- the caller's retry surfaces
* the real error.
*/
function refreshSessionCookie(): Promise<void> {
shellRefresh ??= fetch("/", { cache: "reload", credentials: "include" })
.then(() => undefined)
.catch(() => undefined)
.finally(() => {
shellRefresh = null;
});
return shellRefresh;
}

interface RequestOptions {
manageToken?: string;
signal?: AbortSignal;
Expand All @@ -61,6 +100,7 @@ async function request<T>(
method: string,
path: string,
opts: RequestOptions = {},
isRetry = false,
): Promise<T> {
const headers: Record<string, string> = {};
if (opts.body !== undefined) {
Expand All @@ -79,6 +119,13 @@ async function request<T>(
// is a no-op (browser sends an empty cookie jar for the BFF origin).
credentials: "include",
});
if (res.status === 401 && !isRetry) {
// Stale session cookie (server restarted with a new bearer token):
// re-bootstrap the cookie and retry exactly once. Safe for mutations
// too -- a 401 means the request was rejected on auth, not applied.
await refreshSessionCookie();
return request<T>(method, path, opts, true);
}
if (!res.ok) {
let payload: KbagentError | null = null;
try {
Expand All @@ -88,6 +135,13 @@ async function request<T>(
}
const message = payload?.error?.message ?? res.statusText;
const code = payload?.error?.code ?? "HTTP_ERROR";
if (res.status === 401) {
window.dispatchEvent(
new CustomEvent<SessionExpiredDetail>(SESSION_EXPIRED_EVENT, {
detail: { code, message },
}),
);
}
throw new ApiError(code, message, res.status);
}
// 204 No Content
Expand Down
Loading