From b034988dd4787252bfdf7d1e8b4e0cdbf7f5b64a Mon Sep 17 00:00:00 2001 From: udo Date: Fri, 10 Jul 2026 11:45:40 +0000 Subject: [PATCH] fix: harden auth rendering and parsing --- README.md | 14 +- client/main.ts | 6 +- client/markdown-security-patch.ts | 33 +++++ docs/security-hardening.md | 66 +++++++++ package.json | 3 +- server/index.ts | 219 ++++++++++++++++++++++++++---- server/parsing-budget.ts | 57 ++++++++ shared/markdown-security.ts | 141 +++++++++++++++++++ tests/markdown-security.test.ts | 41 ++++++ tests/parsing-budget.test.ts | 63 +++++++++ 10 files changed, 609 insertions(+), 34 deletions(-) create mode 100644 client/markdown-security-patch.ts create mode 100644 docs/security-hardening.md create mode 100644 server/parsing-budget.ts create mode 100644 shared/markdown-security.ts create mode 100644 tests/markdown-security.test.ts create mode 100644 tests/parsing-budget.test.ts diff --git a/README.md b/README.md index 9353869..0afca6b 100644 --- a/README.md +++ b/README.md @@ -33,9 +33,19 @@ npm run build |---|---|---| | `PORT` | `3001` | Server listen port | | `HOST` | `127.0.0.1` | Server listen address. Use `0.0.0.0` only on a trusted network and with `PI_WEBUI_TOKEN` set. | -| `PI_WEBUI_TOKEN` | *(unset)* | Optional shared secret for `/api/ws` and the skills editor API. If set, open `/?token=` once; the client stores it in local storage for reconnects, removes `token` from the visible URL, and sends later WebSocket/authenticated API requests without keeping the token in the visible URL. Query-string tokens may still appear in browser/proxy logs for the initial page load and are readable by page JavaScript before removal, so use only on trusted local networks. If unset, WebSocket access relies on Origin checks and local/trusted hosting. | -| `PI_WEBUI_ALLOWED_ORIGINS` | same host as request | Optional comma-separated extra WebSocket `Origin` allowlist. | +| `PI_WEBUI_TOKEN` | *(unset)* | **Required by default** shared secret for `/api/ws` and the skills editor API. If set, open `/?token=` once on first load; the client stores it in `localStorage` and removes the query parameter from the URL. | +| `PI_WEBUI_ALLOW_ANONYMOUS` | `false` | Local development escape hatch only: tokenless mode requires this flag and a loopback `HOST`. Non-loopback/public binding always requires `PI_WEBUI_TOKEN`. | +| `PI_WEBUI_ALLOWED_ORIGINS` | same host as request | Additional comma-separated WebSocket `Origin` allowlist values. | | `PI_CODING_AGENT_DIR` | `~/.pi/agent` | Pi agent config directory for auth, models, settings, sessions, and shared skills. | +| `PI_WEBUI_SKILL_MAX_FILES` | `220` | Max SKILL parse candidates per skill directory scan. | +| `PI_WEBUI_SKILL_PARSE_BYTE_BUDGET` | `3000000` | Max total bytes read while scanning skills for a list call. | +| `PI_WEBUI_SKILL_FILE_MAX_BYTES` | `240000` | Max bytes per individual skill file. | +| `PI_WEBUI_SKILL_MAX_DEPTH` | `8` | Max recursive directory depth while scanning skill directories. | +| `PI_WEBUI_SKILL_DIRECTORY_ENTRY_LIMIT` | `1000` | Max entries inspected in any one visited skill directory. | +| `PI_WEBUI_SESSION_DIRECTORY_SCAN_LIMIT` | `800` | Max directory entries considered for a session listing scan before sorting and pagination. | +| `PI_WEBUI_SESSION_MAX_FILES` | `240` | Max session files parsed per listing request. | +| `PI_WEBUI_SESSION_PARSE_BYTE_BUDGET` | `6000000` | Max total bytes read for session list parsing per request. | +| `PI_WEBUI_SESSION_FILE_MAX_BYTES` | `220000` | Max leading bytes read from each session for bounded listing/search; large sessions remain listable but searches do not inspect content beyond this prefix. | | `PI_WEBUI_USER_SKILLS_DIR` | `~/.pi/skills` | Private skills edited from `/skills`. With the default working directory this is loaded by Pi as a project-local skills directory. | | `PI_WEBUI_SHARED_SKILLS_DIR` | `$PI_CODING_AGENT_DIR/skills` | Shared read-only skills listed in `/skills` and available to all agents using this agent dir. | | `PI_WEBUI_RESTRICT_MODELS_TO_SCOPED` | *(unset)* | If true/yes/on/1, only models from `PI_WEBUI_MODEL`, `PI_MODEL`, `settings.defaultProvider/defaultModel`, or `settings.enabledModels` are listed/selectable. | diff --git a/client/main.ts b/client/main.ts index d873f21..9e1a81e 100644 --- a/client/main.ts +++ b/client/main.ts @@ -1,5 +1,7 @@ import "@mariozechner/mini-lit/dist/ThemeToggle.js"; +import "@mariozechner/mini-lit/dist/MarkdownBlock.js"; import "./app.css"; +import { installMarkdownSecurityPatch } from "./markdown-security-patch.js"; import { html, render, nothing } from "lit"; import { icon } from "@mariozechner/mini-lit"; @@ -19,6 +21,7 @@ import { void MessageList; void MessageEditor; void StreamingMessageContainer; +installMarkdownSecurityPatch(); installAssistantMetadataRenderer(); import type { ClientMessage, @@ -27,6 +30,7 @@ import type { SerializedAgentState, SessionListItem, } from "../shared/protocol.js"; +import { sanitizeMarkdownLinks } from "../shared/markdown-security.js"; function toolResultText(result: ToolResultMessage | undefined): string { return result?.content @@ -234,7 +238,7 @@ function installAssistantMetadataRenderer() { for (const chunk of this.message.content) { if (chunk.type === "text" && chunk.text.trim() !== "") { - orderedParts.push(html``); + orderedParts.push(html``); } else if (chunk.type === "thinking" && chunk.thinking.trim() !== "") { orderedParts.push(html``); } else if (chunk.type === "toolCall" && !this.hideToolCalls) { diff --git a/client/markdown-security-patch.ts b/client/markdown-security-patch.ts new file mode 100644 index 0000000..27fb305 --- /dev/null +++ b/client/markdown-security-patch.ts @@ -0,0 +1,33 @@ +import { isSafeMarkdownLink } from "../shared/markdown-security.js"; + +function sanitizeMarkdownElement(root: ParentNode) { + for (const anchor of root.querySelectorAll("a[href]")) { + const href = anchor.getAttribute("href") || ""; + if (isSafeMarkdownLink(href)) continue; + anchor.removeAttribute("href"); + anchor.setAttribute("aria-disabled", "true"); + anchor.setAttribute("title", "Unsafe link blocked"); + } + for (const image of root.querySelectorAll("img[src]")) { + const src = image.getAttribute("src") || ""; + if (isSafeMarkdownLink(src) && !src.trim().toLowerCase().startsWith("mailto:") && !src.trim().toLowerCase().startsWith("tel:")) continue; + image.removeAttribute("src"); + image.setAttribute("title", "Unsafe image source blocked"); + } +} + +/** + * mini-lit renders Marked output through unsafeHTML. Sanitize the resulting DOM + * after every markdown-block update so inline links, reference links, autolinks, + * images, thinking blocks, and artifact previews all use one URL policy. + */ +export function installMarkdownSecurityPatch() { + const MarkdownBlock = customElements.get("markdown-block") as any; + if (!MarkdownBlock || MarkdownBlock.prototype.__piWebuiLinkSecurityPatch) return; + MarkdownBlock.prototype.__piWebuiLinkSecurityPatch = true; + const originalUpdated = MarkdownBlock.prototype.updated; + MarkdownBlock.prototype.updated = function (...args: any[]) { + if (typeof originalUpdated === "function") originalUpdated.apply(this, args); + sanitizeMarkdownElement(this); + }; +} diff --git a/docs/security-hardening.md b/docs/security-hardening.md new file mode 100644 index 0000000..15fd58a --- /dev/null +++ b/docs/security-hardening.md @@ -0,0 +1,66 @@ +# pi-webui Security Hardening Notes + +## Scope + +This document summarizes hardening changes applied in `server/index.ts` and `client/main.ts` for safer markdown rendering, stronger session security defaults, and bounded parsing of untrusted local files. + +## Implemented protections + +### 1) Markdown link allowlist in assistant output + +- Added `shared/markdown-security.ts` with: + - `ALLOWED_MARKDOWN_LINK_SCHEMES` (explicit allowlist) + - `sanitizeMarkdownLinks()` used by assistant message rendering in `client/main.ts` +- Allowed URL schemes are currently: + - `http`, `https`, `mailto`, `tel` + - Relative links (`./`, `../`, `/`) and fragment links (`#`) are preserved. +- Unsafe inline destinations are rewritten before rendering, and a global `markdown-block` post-render patch removes unsafe `href`/`src` attributes from reference links, autolinks, thinking blocks, and artifact previews as a sink-level defense. + +### 2) Security response headers + +`server/index.ts` now applies middleware and websocket headers for: + +- `Content-Security-Policy` +- `X-Content-Type-Options: nosniff` +- `Referrer-Policy: strict-origin-when-cross-origin` +- `X-Frame-Options: DENY` +- `Strict-Transport-Security` + +This applies to HTTP responses and websocket handshake responses. + +### 3) Token policy + +- Anonymous tokenless mode is no longer implicit. +- Server startup requires `PI_WEBUI_TOKEN` for every non-loopback binding. +- Tokenless local development requires both `PI_WEBUI_ALLOW_ANONYMOUS=true` and loopback `HOST` (`127.0.0.1`, `::1`, or `localhost`). +- `PI_WEBUI_TOKEN` is still not accepted from query parameters. +- WebSocket auth accepts: + - `Sec-WebSocket-Protocol` (`pi-webui-token.`) + - `x-pi-webui-token` / `Authorization: Bearer ...` +- REST auth accepts: + - `x-pi-webui-token` / `Authorization: Bearer ...` +- Query token support was removed from server-side token parsing while client bootstrap remains via `/?token=` -> localStorage + immediate URL cleanup. + +### 4) Bounded parsing for skills and sessions + +Added budgeted parsing limits with explicit defaults: + +- Skills: + - `PI_WEBUI_SKILL_MAX_FILES` + - `PI_WEBUI_SKILL_PARSE_BYTE_BUDGET` + - `PI_WEBUI_SKILL_FILE_MAX_BYTES` + - `PI_WEBUI_SKILL_MAX_DEPTH` + - `PI_WEBUI_SKILL_DIRECTORY_ENTRY_LIMIT` +- Sessions: + - `PI_WEBUI_SESSION_DIRECTORY_SCAN_LIMIT` + - `PI_WEBUI_SESSION_MAX_FILES` + - `PI_WEBUI_SESSION_PARSE_BYTE_BUDGET` + - `PI_WEBUI_SESSION_FILE_MAX_BYTES` + +Both listing flows stop parsing new files when request limits are reached and logically set `hasMore` where relevant. Session parsing reads only a bounded prefix from large files, so large sessions remain visible while deep full-text matches beyond the prefix are intentionally unavailable. + +## Remaining risks + +1. CSP is intentionally compatible with current app assumptions and still allows inline styles (`style-src 'unsafe-inline'`) because markdown + KaTeX rendering relies on inline style attributes. +2. Bounded session full-text search inspects only each file prefix; operators can raise limits deliberately when deeper historical search is required. +3. Very large skill/session trees may hide older results after configured file/count budgets are reached. diff --git a/package.json b/package.json index 774dced..13c2ac6 100644 --- a/package.json +++ b/package.json @@ -10,7 +10,8 @@ "build": "npm run build:client", "build:client": "vite build", "start": "tsx server/index.ts", - "preview": "npm run build && npm run start" + "preview": "npm run build && npm run start", + "test": "node --test --import tsx tests/*.test.ts" }, "dependencies": { "@mariozechner/mini-lit": "^0.2.1", diff --git a/server/index.ts b/server/index.ts index 5e28835..7aaaecd 100644 --- a/server/index.ts +++ b/server/index.ts @@ -17,6 +17,14 @@ import { type AgentSession, type AgentSessionEvent, } from "@mariozechner/pi-coding-agent"; +import { + canReadFile, + createParsingState, + markStopped, + parseDepthIsAllowed, + recordRead, + type ParsingLimits, +} from "./parsing-budget.js"; import { Type, type Model, type Api } from "@mariozechner/pi-ai"; import type { ClientMessage, @@ -29,7 +37,12 @@ import type { const __dirname = path.dirname(fileURLToPath(import.meta.url)); function parseIntEnv(name: string, fallback: number): number { const value = Number.parseInt(process.env[name] || "", 10); - return Number.isFinite(value) ? value : fallback; + return Number.isFinite(value) ? Math.max(0, value) : fallback; +} + +function parseLimitedIntEnv(name: string, fallback: number): number { + const value = parseIntEnv(name, fallback); + return value > 0 ? value : fallback; } const PORT = parseIntEnv("PORT", 3001); @@ -40,7 +53,9 @@ let litellmKey = process.env.LITELLM_KEY || ""; const app = express(); const server = createServer(app); -const webUiToken = process.env.PI_WEBUI_TOKEN || ""; +const webUiToken = (process.env.PI_WEBUI_TOKEN || "").trim(); +const allowAnonymous = ["1", "true", "yes", "on"].includes((process.env.PI_WEBUI_ALLOW_ANONYMOUS || "").trim().toLowerCase()); +const anonymousLoopbackOnly = HOST === "127.0.0.1" || HOST === "::1" || HOST === "localhost"; const allowedOrigins = new Set( (process.env.PI_WEBUI_ALLOWED_ORIGINS || "") .split(",") @@ -48,6 +63,29 @@ const allowedOrigins = new Set( .filter(Boolean), ); +const SECURITY_HEADERS = { + csp: "default-src 'self'; base-uri 'self'; form-action 'self'; img-src 'self' data:; font-src 'self'; style-src 'self' 'unsafe-inline'; script-src 'self'; connect-src 'self' ws: wss:; object-src 'none'; frame-ancestors 'none';", + hsts: "max-age=31536000; includeSubDomains; preload", + referrerPolicy: "strict-origin-when-cross-origin", + frameOptions: "DENY", + contentTypeOptions: "nosniff", +}; + +const SKILL_PARSE_LIMITS: ParsingLimits = { + maxFiles: parseLimitedIntEnv("PI_WEBUI_SKILL_MAX_FILES", 220), + maxBytesTotal: parseLimitedIntEnv("PI_WEBUI_SKILL_PARSE_BYTE_BUDGET", 3_000_000), + maxBytesPerFile: parseLimitedIntEnv("PI_WEBUI_SKILL_FILE_MAX_BYTES", 240_000), +}; +const SKILL_MAX_DEPTH = parseLimitedIntEnv("PI_WEBUI_SKILL_MAX_DEPTH", 8); +const SKILL_DIRECTORY_ENTRY_LIMIT = parseLimitedIntEnv("PI_WEBUI_SKILL_DIRECTORY_ENTRY_LIMIT", 1000); + +const SESSION_PARSE_LIMITS: ParsingLimits = { + maxFiles: parseLimitedIntEnv("PI_WEBUI_SESSION_MAX_FILES", 240), + maxBytesTotal: parseLimitedIntEnv("PI_WEBUI_SESSION_PARSE_BYTE_BUDGET", 6_000_000), + maxBytesPerFile: parseLimitedIntEnv("PI_WEBUI_SESSION_FILE_MAX_BYTES", 220_000), +}; +const SESSION_DIRECTORY_SCAN_LIMIT = parseLimitedIntEnv("PI_WEBUI_SESSION_DIRECTORY_SCAN_LIMIT", 800); + function isAllowedWsOrigin(origin: string | undefined, hostHeader: string | undefined): boolean { if (!origin) return true; if (allowedOrigins.has(origin)) return true; @@ -60,6 +98,7 @@ function isAllowedWsOrigin(origin: string | undefined, hostHeader: string | unde } function isValidToken(candidate: string | undefined): boolean { + if (!webUiToken && !allowAnonymous) return false; if (!webUiToken) return true; if (!candidate) return false; const expected = Buffer.from(webUiToken); @@ -67,6 +106,21 @@ function isValidToken(candidate: string | undefined): boolean { return expected.length === actual.length && timingSafeEqual(expected, actual); } +function tokenFromRequestHeader(req: express.Request): string | undefined { + const header = req.header("x-pi-webui-token") || req.header("authorization") || ""; + if (header.toLowerCase().startsWith("bearer ")) return header.slice(7).trim(); + return header.trim(); +} + +function tokenFromWebSocketHeaders(headers: Record): string | undefined { + const header = headers["x-pi-webui-token"] || headers["X-PI-WEBUI-TOKEN"]; + if (!header) return undefined; + if (Array.isArray(header)) return header[0]?.trim(); + const value = typeof header === "string" ? header.trim() : ""; + if (value.toLowerCase().startsWith("bearer ")) return value.slice(7).trim(); + return value; +} + function tokenFromWebSocketProtocol(header: string | string[] | undefined): string | undefined { const raw = Array.isArray(header) ? header.join(",") : header; if (!raw) return undefined; @@ -90,16 +144,32 @@ const wss = new WebSocketServer({ maxPayload: 1024 * 1024, verifyClient: (info, done) => { const originOk = isAllowedWsOrigin(info.origin, info.req.headers.host); - const urlToken = new URL(info.req.url || "/", "http://localhost").searchParams.get("token") || undefined; const protocolToken = tokenFromWebSocketProtocol(info.req.headers["sec-websocket-protocol"]); - const tokenOk = isValidToken(protocolToken || urlToken); + const headerToken = tokenFromWebSocketHeaders(info.req.headers as Record); + const tokenOk = isValidToken(protocolToken || headerToken); done(originOk && tokenOk, originOk ? 401 : 403, originOk ? "Unauthorized" : "Forbidden"); }, }); +app.use((_req, res, next) => { + res.setHeader("Content-Security-Policy", SECURITY_HEADERS.csp); + res.setHeader("X-Content-Type-Options", SECURITY_HEADERS.contentTypeOptions); + res.setHeader("Referrer-Policy", SECURITY_HEADERS.referrerPolicy); + res.setHeader("X-Frame-Options", SECURITY_HEADERS.frameOptions); + res.setHeader("Strict-Transport-Security", SECURITY_HEADERS.hsts); + next(); +}); app.use(express.urlencoded({ extended: false, limit: "64kb" })); app.use(express.json({ limit: "256kb" })); +wss.on("headers", (headers) => { + headers.push(`Content-Security-Policy: ${SECURITY_HEADERS.csp}`); + headers.push(`X-Content-Type-Options: ${SECURITY_HEADERS.contentTypeOptions}`); + headers.push(`Referrer-Policy: ${SECURITY_HEADERS.referrerPolicy}`); + headers.push(`X-Frame-Options: ${SECURITY_HEADERS.frameOptions}`); + headers.push(`Strict-Transport-Security: ${SECURITY_HEADERS.hsts}`); +}); + const HOME_DIR = os.homedir(); const AGENT_DIR = process.env.PI_CODING_AGENT_DIR || path.join(HOME_DIR, ".pi", "agent"); const AGENT_SETTINGS_PATH = path.join(AGENT_DIR, "settings.json"); @@ -198,37 +268,76 @@ function parseSkillFrontmatter(content: string): { fields: Record { +async function readSkillFile(filePath: string, source: SkillSource, editable: boolean, state: ReturnType): Promise { + const stats = await fs.promises.stat(filePath).catch(() => undefined); + const sizeBytes = stats?.size ?? 0; + const decision = canReadFile(sizeBytes, SKILL_PARSE_LIMITS, state); + if (!decision.canRead) { + if (decision.stop) markStopped(state, decision.reason!); + return undefined; + } + const content = await fs.promises.readFile(filePath, "utf8").catch(() => ""); if (!content) return undefined; const { fields, diagnostics } = parseSkillFrontmatter(content); - if (!fields.description) return undefined; + if (!fields.description) { + recordRead(sizeBytes, state); + return undefined; + } const folderName = path.basename(path.dirname(filePath)); const name = fields.name || folderName; const id = source === "user" && isValidSkillName(name) ? name : createHash("sha256").update(path.resolve(filePath)).digest("hex").slice(0, 16); + + recordRead(sizeBytes, state); return { id, name, description: fields.description || "", path: filePath, source, editable, diagnostics }; } +async function readDirectoryEntriesLimited(dir: string, maxEntries: number): Promise { + const out: fs.Dirent[] = []; + const handle = await fs.promises.opendir(dir).catch(() => undefined); + if (!handle) return out; + try { + for await (const entry of handle) { + out.push(entry); + if (maxEntries > 0 && out.length >= maxEntries) break; + } + } finally { + await handle.close().catch(() => undefined); + } + return out; +} + async function collectSkillsFromRoot(root: string, source: SkillSource, editable: boolean): Promise { const out: SkillListItem[] = []; - const visit = async (dir: string, allowRootFiles: boolean) => { - const entries = await fs.promises.readdir(dir, { withFileTypes: true }).catch(() => []); + const state = createParsingState(); + + const visit = async (dir: string, depth: number, allowRootFiles: boolean): Promise => { + if (state.stopReason) return; + if (!parseDepthIsAllowed(depth, SKILL_MAX_DEPTH)) { + return; + } + + const entries = await readDirectoryEntriesLimited(dir, SKILL_DIRECTORY_ENTRY_LIMIT); if (entries.some((entry) => entry.isFile() && entry.name === "SKILL.md")) { - const item = await readSkillFile(path.join(dir, "SKILL.md"), source, editable); + const item = await readSkillFile(path.join(dir, "SKILL.md"), source, editable, state); if (item) out.push(item); return; } + for (const entry of entries) { + if (state.stopReason) break; if (entry.name.startsWith(".") || entry.name === "node_modules") continue; const fullPath = path.join(dir, entry.name); - if (entry.isDirectory()) await visit(fullPath, false); - else if (allowRootFiles && entry.isFile() && entry.name.endsWith(".md")) { - const item = await readSkillFile(fullPath, source, editable); + if (entry.isDirectory()) { + await visit(fullPath, depth + 1, false); + } else if (allowRootFiles && entry.isFile() && entry.name.endsWith(".md")) { + const item = await readSkillFile(fullPath, source, editable, state); if (item) out.push(item); } } }; - await visit(root, true); + + await visit(root, 1, true); return out.sort((a, b) => a.name.localeCompare(b.name)); } @@ -261,14 +370,15 @@ async function getSkillContent(source: string, id: string): Promise<{ item: Skil const skills = await listSkills(); const item = skills.find((skill) => skill.source === source && skill.id === id); if (!item) return undefined; + + const stat = await fs.promises.stat(item.path).catch(() => undefined); + if (!stat || stat.size > SKILL_PARSE_LIMITS.maxBytesPerFile) return undefined; const content = await fs.promises.readFile(item.path, "utf8"); return { item, content }; } function tokenFromRequest(req: express.Request): string | undefined { - const header = req.header("x-pi-webui-token") || req.header("authorization") || ""; - if (header.toLowerCase().startsWith("bearer ")) return header.slice(7).trim(); - return header.trim() || (typeof req.query.token === "string" ? req.query.token : undefined); + return tokenFromRequestHeader(req); } function requireEditAuth(req: express.Request, res: express.Response, next: express.NextFunction) { @@ -513,14 +623,17 @@ async function sessionFiles(): Promise<{ filePath: string; mtime: Date }[]> { if (sessionListCache && sessionListCache.expires > now) return sessionListCache.files; const dir = getSessionDir(HOME_DIR); - const entries = await fs.promises.readdir(dir, { withFileTypes: true }).catch(() => []); - const files = (await Promise.all(entries + const entries = await readDirectoryEntriesLimited(dir, SESSION_DIRECTORY_SCAN_LIMIT); + const candidates = entries .filter((entry) => entry.isFile() && entry.name.endsWith(".jsonl")) - .map(async (entry) => { - const filePath = path.join(dir, entry.name); - const stats = await fs.promises.stat(filePath).catch(() => undefined); - return stats ? { filePath, mtime: stats.mtime } : undefined; - }))) + // Sort the bounded candidate set before stat calls and pagination. + .sort((a, b) => b.name.localeCompare(a.name)) + .slice(0, SESSION_DIRECTORY_SCAN_LIMIT); + const files = (await Promise.all(candidates.map(async (entry) => { + const filePath = path.join(dir, entry.name); + const stats = await fs.promises.stat(filePath).catch(() => undefined); + return stats ? { filePath, mtime: stats.mtime } : undefined; + }))) .filter((file): file is { filePath: string; mtime: Date } => Boolean(file)) .sort((a, b) => b.mtime.getTime() - a.mtime.getTime()); @@ -528,20 +641,59 @@ async function sessionFiles(): Promise<{ filePath: string; mtime: Date }[]> { return files; } +async function readSessionHead(filePath: string, state: ReturnType): Promise { + if (SESSION_PARSE_LIMITS.maxFiles > 0 && state.files >= SESSION_PARSE_LIMITS.maxFiles) { + markStopped(state, "file-count-limit"); + return undefined; + } + const remaining = SESSION_PARSE_LIMITS.maxBytesTotal > 0 + ? SESSION_PARSE_LIMITS.maxBytesTotal - state.bytes + : SESSION_PARSE_LIMITS.maxBytesPerFile; + const cap = Math.min(SESSION_PARSE_LIMITS.maxBytesPerFile, remaining); + if (cap <= 0) { + markStopped(state, "request-byte-limit"); + return undefined; + } + const handle = await fs.promises.open(filePath, "r").catch(() => undefined); + if (!handle) return undefined; + try { + const stats = await handle.stat(); + if (!stats.isFile()) return undefined; + const bytesToRead = Math.min(stats.size, cap); + const buffer = Buffer.alloc(bytesToRead); + const { bytesRead } = await handle.read(buffer, 0, bytesToRead, 0); + recordRead(bytesRead, state); + return buffer.subarray(0, bytesRead).toString("utf8"); + } finally { + await handle.close().catch(() => undefined); + } +} + async function listRecentSessions(options: { offset?: number; limit?: number; query?: string } = {}): Promise<{ items: SessionListItem[]; total: number; hasMore: boolean; offset: number; limit: number; query: string }> { const offset = Math.max(0, Math.floor(options.offset || 0)); const limit = Math.min(Math.max(1, Math.floor(options.limit || SESSION_LIST_LIMIT)), SESSION_LIST_MAX_LIMIT); const query = (options.query || "").trim().toLowerCase(); const files = await sessionFiles(); const matched: SessionListItem[] = []; + const state = createParsingState(); let seenMatches = 0; let hasMore = false; for (const { filePath, mtime } of files) { - const content = await fs.promises.readFile(filePath, "utf8").catch(() => ""); - if (!content) continue; + if (state.stopReason) { + hasMore = true; + break; + } + + const content = await readSessionHead(filePath, state); + if (!content) { + if (state.stopReason) hasMore = true; + continue; + } + const item = sessionItemFromContent(filePath, content, mtime); if (!item) continue; + const haystack = query ? `${item.name || ""}\n${item.firstMessage}\n${content}`.toLowerCase() : ""; if (query && !haystack.includes(query)) continue; if (seenMatches++ < offset) continue; @@ -1177,7 +1329,7 @@ app.post("/api/skills", requireEditAuth, async (req, res) => { const escapedDescription = description.replaceAll('"', "'"); const content = `---\nname: ${name}\ndescription: "${escapedDescription}"\n---\n\n# ${name}\n\n## When to use\n\n${description}\n\n## Instructions\n\n- Describe the workflow this skill should guide.\n- Add any setup, commands, references, or safety constraints the agent should follow.\n`; await writeSkillAtomic(skillFile, content); - const item = await readSkillFile(skillFile, "user", true); + const item = await readSkillFile(skillFile, "user", true, createParsingState()); res.json({ ok: true, skill: item, content, note: "Start a new session or reload the current one for Pi to advertise new/changed skills." }); }); @@ -1192,7 +1344,7 @@ app.put("/api/skills/user/:id", requireEditAuth, async (req, res) => { const skillFile = userSkillPath(id); if (!skillFile || !fs.existsSync(skillFile)) return res.status(404).json({ ok: false, error: "not_found" }); await writeSkillAtomic(skillFile, content); - const item = await readSkillFile(skillFile, "user", true); + const item = await readSkillFile(skillFile, "user", true, createParsingState()); res.json({ ok: true, skill: item, note: "Start a new session or reload the current one for Pi to advertise new/changed skills." }); }); @@ -1214,6 +1366,10 @@ app.get("/{*path}", (_req, res) => { async function main() { console.log("Initializing Pi agent session..."); + if (!webUiToken && (!allowAnonymous || !anonymousLoopbackOnly)) { + throw new Error("PI_WEBUI_TOKEN is required. Anonymous mode is allowed only when PI_WEBUI_ALLOW_ANONYMOUS=true and HOST is loopback."); + } + authStorage = AuthStorage.create(path.join(AGENT_DIR, "auth.json")); modelRegistry = ModelRegistry.create(authStorage, path.join(AGENT_DIR, "models.json")); @@ -1277,10 +1433,13 @@ async function main() { server.listen(PORT, HOST, () => { console.log(`Pi WebUI server listening on http://${HOST}:${PORT}`); + if (webUiToken) { - console.log("PI_WEBUI_TOKEN is configured; open the UI with ?token= once."); - } else { - console.warn("PI_WEBUI_TOKEN is not configured; relying on WebSocket Origin checks only."); + console.log("PI_WEBUI_TOKEN is required and enabled. Open the UI with ?token= once; the client stores it in localStorage and then removes it from the URL."); + } + + if (!webUiToken) { + console.log("PI_WEBUI_ALLOW_ANONYMOUS=true is set on loopback; tokenless local development access is enabled."); } }); } diff --git a/server/parsing-budget.ts b/server/parsing-budget.ts new file mode 100644 index 0000000..e654b8d --- /dev/null +++ b/server/parsing-budget.ts @@ -0,0 +1,57 @@ +export type ParsingStopReason = "file-count-limit" | "request-byte-limit" | "file-byte-limit"; + +export interface ParsingLimits { + maxFiles: number; + maxBytesTotal: number; + maxBytesPerFile: number; +} + +export interface ParsingState { + files: number; + bytes: number; + stopReason: ParsingStopReason | undefined; +} + +export interface ParsingDecision { + canRead: boolean; + stop: boolean; + reason?: ParsingStopReason; +} + +export function createParsingState(): ParsingState { + return { + files: 0, + bytes: 0, + stopReason: undefined, + }; +} + +export function canReadFile(sizeBytes: number, limits: ParsingLimits, state: ParsingState): ParsingDecision { + if (limits.maxBytesPerFile > 0 && sizeBytes > limits.maxBytesPerFile) { + return { canRead: false, stop: false, reason: "file-byte-limit" }; + } + + if (limits.maxFiles > 0 && state.files >= limits.maxFiles) { + return { canRead: false, stop: true, reason: "file-count-limit" }; + } + + if (limits.maxBytesTotal > 0 && state.bytes + sizeBytes > limits.maxBytesTotal) { + return { canRead: false, stop: true, reason: "request-byte-limit" }; + } + + return { canRead: true, stop: false }; +} + +export function recordRead(sizeBytes: number, state: ParsingState) { + state.files += 1; + state.bytes += Math.max(0, sizeBytes); +} + +export function markStopped(state: ParsingState, reason: ParsingStopReason) { + state.stopReason = reason; +} + +export function parseDepthIsAllowed(currentDepth: number, maxDepth: number): boolean { + if (maxDepth <= 0) return true; + return currentDepth <= maxDepth; +} diff --git a/shared/markdown-security.ts b/shared/markdown-security.ts new file mode 100644 index 0000000..6a36f10 --- /dev/null +++ b/shared/markdown-security.ts @@ -0,0 +1,141 @@ +export const ALLOWED_MARKDOWN_LINK_SCHEMES = Object.freeze(["http", "https", "mailto", "tel"] as const); +export type AllowedMarkdownLinkScheme = (typeof ALLOWED_MARKDOWN_LINK_SCHEMES)[number]; + +function decodeEntityCodePoint(raw: string, value: string, radix: number): string { + const codePoint = Number.parseInt(value, radix); + return Number.isFinite(codePoint) && codePoint >= 0 && codePoint <= 0x10ffff + ? String.fromCodePoint(codePoint) + : raw; +} + +function decodeUrlCharacterEntities(value: string): string { + return value + .replace(/&#x([0-9a-f]+);?/gi, (raw, hex) => decodeEntityCodePoint(raw, hex, 16)) + .replace(/&#([0-9]+);?/g, (raw, decimal) => decodeEntityCodePoint(raw, decimal, 10)) + .replace(/:/gi, ":") + .replace(/&tab;/gi, "\t") + .replace(/&newline;/gi, "\n"); +} + +function isAllowedUrlScheme(rawDestination: string): boolean { + const trimmed = rawDestination.trim(); + if (!trimmed) return false; + + // HTML attributes decode character references, and URL parsers ignore ASCII + // whitespace/control characters in schemes. Normalize before allowlisting. + const schemeProbe = decodeUrlCharacterEntities(trimmed).replace(/[\u0000-\u0020\u007f]/g, ""); + const schemeMatch = /^([a-zA-Z][a-zA-Z0-9+.-]*):/.exec(schemeProbe); + if (!schemeMatch) return true; // relative/hash/protocol-relative URL + const scheme = schemeMatch[1].toLowerCase() as AllowedMarkdownLinkScheme; + return ALLOWED_MARKDOWN_LINK_SCHEMES.includes(scheme); +} + +function splitMarkdownDestination(raw: string): { destination: string; title: string } { + const trimmed = raw.trim(); + if (!trimmed) return { destination: "", title: "" }; + + if (trimmed.startsWith("<")) { + const close = trimmed.indexOf(">"); + if (close < 0) return { destination: trimmed, title: "" }; + const destination = trimmed.slice(1, close).trim(); + const title = trimmed.slice(close + 1).trim(); + return { destination, title }; + } + + const firstWhitespace = [...trimmed].findIndex((char) => char === " " || char === "\t" || char === "\n" || char === "\r"); + if (firstWhitespace < 0) { + return { destination: trimmed, title: "" }; + } + + return { destination: trimmed.slice(0, firstWhitespace).trim(), title: trimmed.slice(firstWhitespace).trim() }; +} + +function sanitizeMarkdownLinkDestination(raw: string): string { + const { destination } = splitMarkdownDestination(raw); + // Check both the full raw destination (to catch whitespace-obfuscated schemes) + // and the parsed destination (to preserve optional markdown link titles). + if (!isAllowedUrlScheme(raw) || (destination && !isAllowedUrlScheme(destination))) { + return "#"; + } + return raw.trim(); +} + +function isMarkdownAutolinkCandidate(markdown: string): boolean { + const trimmed = markdown.trim(); + return trimmed.startsWith("<") && trimmed.endsWith(">") && trimmed.includes(":"); +} + +/** + * Return markdown text where markdown link destinations use an explicit allowlist + * of URL schemes. This protects against `javascript:`, `data:`, etc. + */ +export function sanitizeMarkdownLinks(markdown: string): string { + if (!markdown.includes("[") && !markdown.includes("<")) return markdown; + + let sanitized = ""; + let index = 0; + + while (index < markdown.length) { + const char = markdown[index]; + + const autolinkEnd = markdown.indexOf(">", index + 1); + if (char === "<" && autolinkEnd >= 0) { + const candidate = markdown.slice(index + 1, autolinkEnd); + const token = markdown.slice(index, autolinkEnd + 1); + if (isMarkdownAutolinkCandidate(token)) { + sanitized += isAllowedUrlScheme(candidate) ? token : `\`${candidate}\``; + index = autolinkEnd + 1; + continue; + } + } + + const linkTextStart = char === "[" || (char === "!" && markdown[index + 1] === "["); + if (!linkTextStart) { + sanitized += char; + index += 1; + continue; + } + + const open = markdown[index] === "!" ? index + 1 : index; + const labelEnd = markdown.indexOf("](", open); + if (labelEnd < 0) { + sanitized += char; + index += 1; + continue; + } + + const destinationStart = labelEnd + 2; + let destinationEnd = destinationStart; + let depth = 0; + while (destinationEnd < markdown.length) { + const next = markdown[destinationEnd]; + if (next === "\\") { + destinationEnd += 2; + continue; + } + if (next === "(") { + depth += 1; + } else if (next === ")") { + if (depth === 0) break; + depth -= 1; + } + destinationEnd += 1; + } + + if (destinationEnd >= markdown.length) { + sanitized += markdown.slice(index); + break; + } + + const prefix = markdown.slice(index, destinationStart); + const destination = markdown.slice(destinationStart, destinationEnd); + sanitized += `${prefix}${sanitizeMarkdownLinkDestination(destination)})`; + index = destinationEnd + 1; + } + + return sanitized; +} + +export function isSafeMarkdownLink(rawDestination: string): boolean { + return isAllowedUrlScheme(rawDestination); +} diff --git a/tests/markdown-security.test.ts b/tests/markdown-security.test.ts new file mode 100644 index 0000000..cf1d6aa --- /dev/null +++ b/tests/markdown-security.test.ts @@ -0,0 +1,41 @@ +import assert from "node:assert/strict"; +import { test } from "node:test"; +import { sanitizeMarkdownLinks, isSafeMarkdownLink, ALLOWED_MARKDOWN_LINK_SCHEMES } from "../shared/markdown-security.js"; + +test("allows safe markdown link schemes", () => { + const markdown = "[Website](https://example.com) and [Mail](mailto:person@example.com)"; + assert.equal(sanitizeMarkdownLinks(markdown), markdown); +}); + +test("blocks javascript and data links", () => { + assert.equal(sanitizeMarkdownLinks("[x](javascript:alert(1))"), "[x](#)"); + assert.equal(sanitizeMarkdownLinks("[x](data:text/plain,hello)"), "[x](#)"); + assert.equal(sanitizeMarkdownLinks("[x](javascript:alert(1))"), "[x](#)"); + assert.equal(sanitizeMarkdownLinks("[x](java\nscript:alert(1))"), "[x](#)"); +}); + +test("allows relative links", () => { + assert.equal(sanitizeMarkdownLinks("[x](docs/readme.md)"), "[x](docs/readme.md)"); + assert.equal(sanitizeMarkdownLinks("[x](../assets/logo.png)"), "[x](../assets/logo.png)"); +}); + +test("keeps markdown autolink for safe destination and blocks unsafe", () => { + const withSafe = sanitizeMarkdownLinks(""); + assert.equal(withSafe, ""); + + const withUnsafe = sanitizeMarkdownLinks(""); + assert.equal(withUnsafe, "`javascript:alert(1)`"); +}); + +test("checks reference-link destinations with the same sink policy", () => { + // Source rewriting intentionally leaves reference definitions intact; the + // markdown-block DOM patch calls this policy on the rendered href. + assert.equal(sanitizeMarkdownLinks("[x][bad]\n\n[bad]: javascript:alert(1)"), "[x][bad]\n\n[bad]: javascript:alert(1)"); + assert.equal(isSafeMarkdownLink("javascript:alert(1)"), false); + assert.equal(isSafeMarkdownLink("javascript:alert(1)"), false); +}); + +test("exposes allowlist", () => { + assert.ok(ALLOWED_MARKDOWN_LINK_SCHEMES.includes("https")); + assert.equal((ALLOWED_MARKDOWN_LINK_SCHEMES as readonly string[]).includes("javascript"), false); +}); diff --git a/tests/parsing-budget.test.ts b/tests/parsing-budget.test.ts new file mode 100644 index 0000000..0583d5e --- /dev/null +++ b/tests/parsing-budget.test.ts @@ -0,0 +1,63 @@ +import assert from "node:assert/strict"; +import { test } from "node:test"; +import { + canReadFile, + createParsingState, + markStopped, + parseDepthIsAllowed, + recordRead, + type ParsingLimits, +} from "../server/parsing-budget.js"; + +test("allows reads within limits", () => { + const state = createParsingState(); + const limits: ParsingLimits = { maxFiles: 2, maxBytesTotal: 100, maxBytesPerFile: 60 }; + + assert.equal(canReadFile(40, limits, state).canRead, true); + recordRead(40, state); + assert.equal(canReadFile(40, limits, state).canRead, true); + recordRead(40, state); + assert.equal(canReadFile(1, limits, state).canRead, false); + assert.equal(state.files, 2); + assert.equal(state.bytes, 80); +}); + +test("stops when request byte budget is exceeded", () => { + const state = createParsingState(); + const limits: ParsingLimits = { maxFiles: 10, maxBytesTotal: 100, maxBytesPerFile: 60 }; + + assert.deepEqual(canReadFile(50, limits, state), { + canRead: true, + stop: false, + }); + recordRead(50, state); + const decision = canReadFile(60, limits, state); + assert.equal(decision.canRead, false); + assert.equal(decision.stop, true); + assert.equal(decision.reason, "request-byte-limit"); + assert.equal(state.files, 1); +}); + +test("skips oversized files but keeps scanning", () => { + const state = createParsingState(); + const limits: ParsingLimits = { maxFiles: 2, maxBytesTotal: 100, maxBytesPerFile: 40 }; + assert.deepEqual(canReadFile(99, limits, state), { + canRead: false, + stop: false, + reason: "file-byte-limit", + }); + assert.equal(canReadFile(20, limits, state).canRead, true); +}); + +test("tracks depth limits", () => { + const depthLimit = 3; + assert.equal(parseDepthIsAllowed(1, depthLimit), true); + assert.equal(parseDepthIsAllowed(3, depthLimit), true); + assert.equal(parseDepthIsAllowed(4, depthLimit), false); +}); + +test("marks stopped state", () => { + const state = createParsingState(); + markStopped(state, "file-count-limit"); + assert.equal(state.stopReason, "file-count-limit"); +});