Conversation
…in HTTP Local URLs (localhost, host.docker.internal) are explicitly allowlisted but resolveAndValidateUrl() used dns.resolve4() which can't find /etc/hosts entries like host.docker.internal, breaking Ollama model discovery. Gate DNS validation behind !isLocal to match existing pattern. Also disable HSTS and upgrade-insecure-requests from helmet defaults since BotMaker runs on plain HTTP — these caused browsers to silently fail loading assets over LAN. Allow Google Fonts in CSP. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add Rust toolchain via rustup (1.93.0 stable), yosys/ghdl for HDL/FPGA work, and a broad set of developer libraries and utilities: autotools, meson, protobuf, libxml2, libcurl, libncurses, libpcre2, wget, tree, openssh-client, rsync, zstd, and more. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adjusts BotMaker’s security headers and local model-discovery behavior to better support LAN/plain-HTTP deployments and local Ollama discovery from within Docker, while expanding the botenv build image tooling.
Changes:
- Updated Helmet/CSP configuration (allow Google Fonts; disable HSTS and
upgrade-insecure-requestsbehavior). - Updated
/api/models/discoverto skip DNS resolution for explicitly allowlisted “local” URLs (e.g.,host.docker.internal). - Added unit tests for local discovery URL handling; expanded botenv Docker image dependencies and added Rust via rustup.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/server.ts | Adjusts Helmet/CSP settings and changes local discovery URL handling to bypass DNS for allowlisted local hosts. |
| src/discover.test.ts | Adds tests validating model discovery for localhost and host.docker.internal using a mocked fetch. |
| Dockerfile.botenv | Installs additional build/tooling packages and installs Rust toolchain via rustup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Register security headers | ||
| await server.register(fastifyHelmet, { | ||
| // BotMaker runs on plain HTTP behind a LAN; HSTS and upgrade-insecure-requests | ||
| // cause browsers to silently fail when there is no TLS terminator. | ||
| hsts: false, |
There was a problem hiding this comment.
hsts: false disables Strict-Transport-Security unconditionally. If BotMaker is ever served over HTTPS (e.g., behind a reverse proxy that forwards headers), this removes an important security control. Consider making HSTS configurable (default enabled) and only disabling it when you know the external scheme is HTTP-only, rather than hard-coding it off for all deployments.
| // Register security headers | |
| await server.register(fastifyHelmet, { | |
| // BotMaker runs on plain HTTP behind a LAN; HSTS and upgrade-insecure-requests | |
| // cause browsers to silently fail when there is no TLS terminator. | |
| hsts: false, | |
| // Determine whether to enable HSTS. Default is enabled; set BOTMAKER_HSTS=false | |
| // in environments that are strictly HTTP-only (e.g., plain HTTP behind a LAN). | |
| const enableHsts = process.env.BOTMAKER_HSTS !== 'false'; | |
| // Register security headers | |
| await server.register(fastifyHelmet, { | |
| // HSTS can cause issues when there is no TLS terminator; allow opting out | |
| // via BOTMAKER_HSTS=false instead of disabling it unconditionally. | |
| hsts: enableHsts, |
| if (isLocal) { | ||
| // Local URLs are explicitly allowlisted — skip DNS resolution. | ||
| // host.docker.internal is in /etc/hosts only; dns.resolve4() can't find it. | ||
| resolvedUrl = url; | ||
| originalHost = new URL(url).hostname; | ||
| } else { | ||
| ({ resolvedUrl, originalHost } = await resolveAndValidateUrl(url)); | ||
| } |
There was a problem hiding this comment.
In the isLocal branch, the handler bypasses isPrivateUrl(...) protocol validation and proceeds to build/fetch the URL. That means non-http(s) local URLs (e.g. ftp://localhost/...) would be treated as allowlisted and return 200 with empty models instead of a 400. Add an explicit protocol check (http/https only) before entering the local fast-path, or update isLocalDiscoveryUrl to also require an http(s) scheme.
| vi.stubGlobal('fetch', mockFetch); | ||
|
|
||
| const server = await createTestServer(); | ||
| const token = await getAuthToken(server); | ||
| const response = await server.inject({ | ||
| method: 'POST', | ||
| url: '/api/models/discover', | ||
| payload: { baseUrl: 'http://localhost:11434/v1' }, | ||
| headers: { Authorization: `Bearer ${token}` }, | ||
| }); | ||
| expect(response.statusCode).toBe(200); | ||
| const body = JSON.parse(response.body) as { models: string[] }; | ||
| expect(body.models).toContain('qwen3-coder:30b'); | ||
| expect(body.models).toContain('nomic-embed-text'); | ||
| expect(mockFetch).toHaveBeenCalledOnce(); | ||
| // localhost is rewritten to host.docker.internal by toDockerHostUrl | ||
| expect(mockFetch.mock.calls[0][0]).toBe('http://host.docker.internal:11434/v1/models'); | ||
|
|
||
| vi.unstubAllGlobals(); | ||
| await server.close(); |
There was a problem hiding this comment.
These tests stub globalThis.fetch and manually call vi.unstubAllGlobals() at the end of each test. If an assertion throws before cleanup, the stub can leak into subsequent tests and cause order-dependent failures. Prefer restoring the stub in an afterEach (or a try/finally) so globals are always reset.
| # Install Rust toolchain via rustup (APT packages are severely outdated) | ||
| ENV RUSTUP_HOME=/usr/local/rustup \ | ||
| CARGO_HOME=/usr/local/cargo \ | ||
| PATH=/usr/local/cargo/bin:$PATH | ||
|
|
||
| RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs \ | ||
| | sh -s -- -y --profile minimal --default-toolchain stable \ | ||
| && rustc --version \ |
There was a problem hiding this comment.
Installing Rust via rustup with --default-toolchain stable makes the image build non-reproducible (the toolchain can change over time), which can lead to unexpected build breakages. Consider pinning a specific Rust toolchain version via an ARG/ENV (and optionally verifying the installer) to keep builds deterministic.
No description provided.