sec(web): Host allowlist, body caps, fail-closed sanitization; CI reproducibility - #64
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const safeFtsQuery = query | ||
| .replace(/[*^:\-+?()"]/g, " ") | ||
| .replace(/\s+/g, " ") | ||
| .trim() | ||
| .slice(0, 500); |
There was a problem hiding this comment.
🟡 Punctuated transcript searches return empty
Queries like react.js, don't, and email@example.com survive safeFtsQuery with invalid FTS5 syntax. The caught database error returns no matches, even when transcripts contain those terms.
Prompt for agents
TranscriptManager.searchTranscripts in src/services/sqlite/transcript-manager.ts passes partially sanitized bare text to FTS5 MATCH. FTS5 rejects many punctuation characters not covered by the current regex, including periods, apostrophes, slashes, at-signs, brackets, exclamation marks, and ampersands. Reserved standalone tokens such as AND, OR, and NOT also remain operators or produce syntax errors. Convert user input into a valid literal-token FTS query, or add a reliable fallback that still searches the original terms. Add regression cases for common punctuated terms and reserved words.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
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.ts covers don't / react.js / email@x / AND.
There was a problem hiding this comment.
🟡 Changes recommended
The new Host-header parsing misclassifies unbracketed IPv6 literals (e.g., 2001:db8::1) as host:port and can incorrectly reject configured IPv6 hosts.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR tightens the web dashboard’s security posture and improves CI reproducibility by adding Host-header allowlisting, consistent request body limits across Bun/Node, and safer client-side sanitization behavior.
Changes:
- Enforce a fail-closed Host allowlist for all web server routes and make API-key comparison constant-time (via hashing).
- Apply a 256 KiB request body cap to Bun.serve to match the Node adapter, and make the dashboard’s sanitization/load ordering safer (defer app.js; escape when DOMPurify is unavailable).
- Add regression tests for Host allowlisting, sanitizer behavior, and transcript FTS query sanitization; pin Bun version in CI/SonarCloud with frozen lockfile installs.
File summaries
| File | Description |
|---|---|
| tests/web-server-routes.test.ts | Adds Host allowlist and API-key edge case coverage for web server routes. |
| tests/web-dashboard-sanitize.test.ts | Verifies script defer ordering and fail-closed sanitizeHtml behavior. |
| tests/transcript-fts-sanitize.test.ts | Adds regression coverage for sanitizing FTS5 operator syntax. |
| src/web/index.html | Defers app.js to ensure vendor scripts load first. |
| src/web/app.js | Makes sanitizeHtml escape content if DOMPurify is missing. |
| src/services/web-server.ts | Implements Host-header allowlisting and updates API-key comparison. |
| src/services/sqlite/transcript-manager.ts | Sanitizes FTS query input and fixes the joined SELECT column list. |
| src/services/platform-server.ts | Unifies request body cap by applying maxRequestBodySize to Bun.serve. |
| docs/CONFIGURATION.md | Documents Host allowlist behavior and proxy guidance. |
| docs/CHANGELOG.md | Records security and CI workflow changes under Unreleased. |
| .github/workflows/sonarcloud.yml | Pins Bun version and uses frozen lockfile installs for reproducibility. |
| .github/workflows/ci.yml | Pins Bun version (and uses frozen lockfile installs) for reproducibility. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| 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; | ||
| } |
There was a problem hiding this comment.
Partially fixed in #66 (146e4a0): unbracketed multi-colon Host strings now parse as whole hostnames. The "configured IPv6 hosts rejected" half of the claim is covered by the IPv6 test there.
7d2a974 to
ce0fc7b
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d2a974f69
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const configured = hostnameFromHostHeader(config.host); | ||
| if (configured) allowed.add(configured); |
There was a problem hiding this comment.
A valid remote deployment that binds with webServerHost: "0.0.0.0" now rejects every client request: browsers connecting through a LAN IP or DNS name send that address in Host, but this set permits only 0.0.0.0 and loopback names. The same regression occurs with a normal reverse proxy that preserves its public Host while the backend remains bound to 127.0.0.1; changing webServerHost to the public name also changes the value passed to serve as the listening hostname. Keep bind-address configuration separate from the accepted external host names (or explicitly support wildcard bind addresses), otherwise the dashboard and API are inaccessible in these deployments.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified — this was the breaking one. Fixed in #66 (146e4a0): new webServerAllowedHosts: string[] config keeps bind semantics (webServerHost) separate from accepted Host names, and docs/CONFIGURATION.md now documents binding remotely / behind a proxy correctly (the old guidance did change the listen interface, as you noted).
db135e8 to
c273e66
Compare
c273e66 to
3cd7f87
Compare
3cd7f87 to
d106f76
Compare
d106f76 to
3a3d275
Compare
3a3d275 to
f8d3651
Compare
|
Codex P1 + Copilot + Devin findings on #64: - webServerHost doubled as bind address AND Host-allowlist entry, so '0.0.0.0' binds rejected every remote client (403 on the whole dashboard) and the documented proxy workaround silently changed the listen interface. webServerAllowedHosts (default []) now lists extra accepted Host names; bind semantics of webServerHost unchanged. - unbracketed IPv6 Host literals (2001:db8::1) were misparsed as host:port ('host 2001:db8:, port 1'); multi-colon strings are now treated as the whole hostname. - safeFtsQuery stripped a fixed punctuation list — apostrophes, periods, @ and more still reached FTS5 as invalid MATCH syntax and returned silently empty results. Token quoting ("tok") neutralizes operators and keeps punctuation matchable; shared helper serves both the memory (searchFTS5) and transcript paths.
Summary
Full codebase audit batch 3/3 (web security + prod hygiene). Gates green:
format:check+typecheck+ 828 tests passed (baseline 821) +build.sec(web)/api/migration/run. Allowed:127.0.0.1/localhost/[::1]+ the configuredwebServerHostsec(web)maxRequestBodySize: 262_144— the Node adapter capped bodies at 256 KiB, the Bun path didn't (potential OOM, in-process with the host)sec(web)app.jsis nowdefer(loaded after DOMPurify) andsanitizeHtmlfails closed to escaped text instead of raw HTML when DOMPurify is missingsec(sqlite)sec(web)chore(ci)bun-version: "1.4.0"pinned inci.yml+sonarcloud.yml(waslatest), sonarbun install --frozen-lockfileVerification
bun run format:check && bun run typecheck && bun run test && bun run build— all green (828 passed / 1 skipped, 2 new test files)Notes
docs/CONFIGURATION.md(setwebServerHostexplicitly when proxying).