-
Notifications
You must be signed in to change notification settings - Fork 2
sec(web): Host allowlist, body caps, fail-closed sanitization; CI reproducibility #64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
3282751
9dc7b63
59361cc
758ee4b
2a29185
d535b5a
f8d3651
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ import { readFileSync } from "node:fs"; | |
| import { randomInt, timingSafeEqual } from "node:crypto"; | ||
| import { join, dirname } from "node:path"; | ||
| import { fileURLToPath } from "node:url"; | ||
| import { log } from "./logger.js"; | ||
| import { log, warn } from "./logger.js"; | ||
| import { serve, type PlatformServer } from "./platform-server.js"; | ||
| import { CONFIG, isConfigured } from "../config.js"; | ||
| import type { UserProfileData } from "./user-profile/types.js"; | ||
|
|
@@ -80,6 +80,37 @@ function isLoopbackHost(host: string): boolean { | |
| return LOCAL_HOSTS.has(host.trim().toLowerCase()); | ||
| } | ||
|
|
||
| function hostnameFromHostHeader(raw: string): string { | ||
| const host = raw.trim().toLowerCase(); | ||
| if (host.startsWith("[")) { | ||
| const end = host.indexOf("]"); | ||
| if (end !== -1) return host.slice(1, end); | ||
| } | ||
| const colon = host.lastIndexOf(":"); | ||
| if (colon > 0 && /^\d+$/.test(host.slice(colon + 1))) { | ||
| return host.slice(0, colon); | ||
| } | ||
| return host; | ||
| } | ||
|
Comment on lines
+83
to
+94
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Partially fixed in #66 ( |
||
|
|
||
| function isHostAllowed(headers: Headers, config: WebServerConfig): boolean { | ||
| const raw = headers.get("host"); | ||
| if (!raw) return false; | ||
| const hostname = hostnameFromHostHeader(raw); | ||
| const allowed = new Set(["127.0.0.1", "localhost", "::1"]); | ||
| const configured = hostnameFromHostHeader(config.host); | ||
| if (configured) allowed.add(configured); | ||
|
Comment on lines
+101
to
+102
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
A valid remote deployment that binds with Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verified — this was the breaking one. Fixed in #66 ( |
||
| if (allowed.has(hostname)) return true; | ||
| const rawLower = raw.trim().toLowerCase(); | ||
| if (config.port !== 80 && config.port !== 443) { | ||
| for (const h of allowed) { | ||
| if (rawLower === `${h}:${config.port}`) return true; | ||
| if (h.includes(":") && rawLower === `[${h}]:${config.port}`) return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| export class WebServer { | ||
| private server: PlatformServer | null = null; | ||
| private readonly config: WebServerConfig; | ||
|
|
@@ -260,6 +291,13 @@ export class WebServer { | |
| } | ||
|
|
||
| private async handleRequest(req: Request): Promise<Response> { | ||
| if (!isHostAllowed(req.headers, this.config)) { | ||
| warn("Rejected request with disallowed Host header", { | ||
| host: req.headers.get("host"), | ||
| }); | ||
| return this.jsonResponse({ success: false, error: "Forbidden" }, 403); | ||
| } | ||
|
|
||
| let url: URL; | ||
| try { | ||
| url = new URL(req.url); | ||
|
|
@@ -327,14 +365,18 @@ export class WebServer { | |
| if (!apiKey) return false; | ||
| const headerKey = req.headers.get("x-opencode-mem-key") ?? ""; | ||
| const bearerKey = (req.headers.get("authorization") ?? "").replace(/^Bearer\s+/, ""); | ||
| const keyBuf = Buffer.from(apiKey); | ||
| // Compare both possible header sources in constant time | ||
| const hBuf = Buffer.from(headerKey.padEnd(apiKey.length, "\0").slice(0, apiKey.length)); | ||
| const bBuf = Buffer.from(bearerKey.padEnd(apiKey.length, "\0").slice(0, apiKey.length)); | ||
| return ( | ||
| (headerKey.length === apiKey.length && timingSafeEqual(keyBuf, hBuf)) || | ||
| (bearerKey.length === apiKey.length && timingSafeEqual(keyBuf, bBuf)) | ||
| ); | ||
| // Compare on fixed-size padded buffers: constant 32-byte inputs mean the | ||
| // comparison time never depends on secret length, and the secret is never | ||
| // hashed. Truncation only matters beyond 32 bytes (~256-bit keys). | ||
| const pad32 = (secret: string) => { | ||
| const buf = Buffer.alloc(32); | ||
| Buffer.from(secret, "utf8").copy(buf, 0, 0, 32); | ||
| return buf; | ||
| }; | ||
| const expected = pad32(apiKey); | ||
| const headerOk = timingSafeEqual(pad32(headerKey), expected); | ||
| const bearerOk = timingSafeEqual(pad32(bearerKey), expected); | ||
| return headerOk || bearerOk; | ||
| } | ||
|
|
||
| private async _dispatchApiRoute( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| import { mkdtempSync, rmSync } from "node:fs"; | ||
| import { tmpdir } from "node:os"; | ||
| import { join } from "node:path"; | ||
| import { afterEach, beforeEach, describe, expect, it } from "vitest"; | ||
| import { CONFIG } from "../src/config.js"; | ||
| import { TranscriptManager } from "../src/services/sqlite/transcript-manager.js"; | ||
|
|
||
| describe("transcript FTS query sanitization", () => { | ||
| let tmp: string; | ||
| let originalPath: string; | ||
| let originalEnabled: boolean; | ||
| let mgr: TranscriptManager; | ||
|
|
||
| beforeEach(() => { | ||
| tmp = mkdtempSync(join(tmpdir(), "opencode-mem0-fts-")); | ||
| originalPath = CONFIG.storagePath; | ||
| originalEnabled = CONFIG.transcriptStorage.enabled; | ||
| CONFIG.storagePath = tmp; | ||
| CONFIG.transcriptStorage.enabled = true; | ||
| mgr = new TranscriptManager(); | ||
| mgr.saveTranscript("sess-1", "/p", [{ role: "user", content: "unclosed quote about react" }]); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| mgr.close(); | ||
| CONFIG.storagePath = originalPath; | ||
| CONFIG.transcriptStorage.enabled = originalEnabled; | ||
| rmSync(tmp, { recursive: true, force: true }); | ||
| }); | ||
|
|
||
| it("does not throw on FTS operator syntax and still matches tokens", () => { | ||
| expect(() => mgr.searchTranscripts(`"unclosed quote`)).not.toThrow(); | ||
| const quoted = mgr.searchTranscripts(`"unclosed quote`); | ||
| expect(quoted.transcripts.length).toBeGreaterThan(0); | ||
|
|
||
| expect(() => mgr.searchTranscripts(`" ( ay "*`)).not.toThrow(); | ||
| const junk = mgr.searchTranscripts(`" ( ay "*`); | ||
| expect(junk.transcripts).toEqual([]); | ||
| expect(junk.total).toBe(0); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| import { readFileSync } from "node:fs"; | ||
| import { describe, expect, it } from "vitest"; | ||
|
|
||
| const html = readFileSync(new URL("../src/web/index.html", import.meta.url), "utf8"); | ||
| const appJs = readFileSync(new URL("../src/web/app.js", import.meta.url), "utf8"); | ||
|
|
||
| describe("web dashboard sanitizer loading", () => { | ||
| it("defers app.js after DOMPurify", () => { | ||
| expect(html).toContain('<script defer src="/vendor/dompurify.min.js"></script>'); | ||
| expect(html).toContain('<script defer src="/app.js"></script>'); | ||
| expect(html.indexOf("/vendor/dompurify.min.js")).toBeLessThan(html.indexOf('src="/app.js"')); | ||
| }); | ||
|
|
||
| it("sanitizeHtml fails closed when DOMPurify is missing", () => { | ||
| expect(appJs).toContain("window.DOMPurify ? DOMPurify.sanitize(html) : esc(html)"); | ||
| expect(appJs).not.toMatch(/DOMPurify\.sanitize\(html\) : html/); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Punctuated transcript searches return empty
Queries like
react.js,don't, andemail@example.comsurvivesafeFtsQuerywith invalid FTS5 syntax. The caught database error returns no matches, even when transcripts contain those terms.Prompt for agents
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #66 (
146e4a0): tokens are phrase-quoted ("don't") in both the memory (searchFTS5) and transcript paths, so punctuation stays matchable and reserved words become valid empty phrase queries.tests/transcript-fts-sanitize.test.tscoversdon't/react.js/email@x/AND.