Conversation
- Reject '..' in model names to prevent path traversal - Add missing IPv6 reserved ranges (unspecified, documentation, TEREDO, discard, NAT64) - Validate IPv4 octets are ≤255, treating invalid IPs as private Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR titled “Security Updates” tightens input validation and runtime security controls across the backend, proxy, and dashboard API wiring.
Changes:
- Harden server-side security with a stricter CSP, bot-creation input validation, and SSRF/DNS-rebinding protections for model discovery.
- Move
/api/models/discoverfrom GET(query) to POST(body) and update the dashboard client accordingly. - Add secret-name validation, reduce proxy forwarded headers, and bump package versions/lockfiles to
1.0.2(incl. Fastify patch update).
Reviewed changes
Copilot reviewed 8 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/server.ts | Adds CSP + validation, reworks /api/models/discover with SSRF/DNS protections, switches it to POST, and adds provider/channel/model validation. |
| src/secrets/manager.ts | Adds SECRET_NAME_REGEX validation for secret read/write. |
| src/bots/templates.ts | Tightens workspace permissions/ownership handling and validates hostname on delete. |
| proxy/src/routes/proxy.ts | Restricts forwarded request headers to an allowlist. |
| proxy/package.json | Version bump to 1.0.2. |
| proxy/package-lock.json | Lockfile updates (incl. Fastify patch). |
| package.json | Version bump to 1.0.2. |
| package-lock.json | Lockfile updates (incl. dependencies patch bumps). |
| dashboard/src/api.ts | Updates dynamic model discovery call to POST JSON body. |
| dashboard/package.json | Version bump to 1.0.2. |
| dashboard/package-lock.json | Lockfile version bump alignment. |
Files not reviewed (2)
- dashboard/package-lock.json: Language not supported
- proxy/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (isPrivateUrl(baseUrl)) { | ||
| reply.code(400); | ||
| return { error: 'Requests to private/internal addresses are not allowed' }; | ||
| } | ||
|
|
||
| const fetchBase = toDockerHostUrl(baseUrl); | ||
|
|
||
| if (isPrivateUrl(fetchBase)) { | ||
| reply.code(400); | ||
| return { error: 'Requests to private/internal addresses are not allowed' }; | ||
| } |
There was a problem hiding this comment.
/api/models/discover currently rejects baseUrl values like http://localhost:11434/v1 and (after toDockerHostUrl) host.docker.internal, which makes the dashboard's Ollama dynamic model discovery fail (Ollama provider baseUrl defaults to localhost). If the intent is to support local Ollama via Docker, consider allowlisting localhost/host.docker.internal for this admin-authenticated endpoint or switching discovery to go through keyring-proxy instead of blocking these hosts.
| // Pin to the first resolved IP to prevent DNS rebinding | ||
| const pinnedIp = addresses[0]; | ||
| const pinnedHost = pinnedIp.includes(':') ? `[${pinnedIp}]` : pinnedIp; | ||
| parsed.hostname = pinnedHost; | ||
| return { resolvedUrl: parsed.toString(), originalHost: hostname }; | ||
| } | ||
|
|
There was a problem hiding this comment.
resolveAndValidateUrl() rewrites the URL hostname to a pinned IP to mitigate DNS rebinding. This will break HTTPS model discovery because TLS/SNI and certificate validation are based on the URL hostname (setting a Host header later won’t fix cert mismatch). Consider pinning via a custom DNS lookup/dispatcher while keeping the original hostname in the URL, or explicitly limiting discovery to http: only if HTTPS isn’t supported here.
| // Pin to the first resolved IP to prevent DNS rebinding | |
| const pinnedIp = addresses[0]; | |
| const pinnedHost = pinnedIp.includes(':') ? `[${pinnedIp}]` : pinnedIp; | |
| parsed.hostname = pinnedHost; | |
| return { resolvedUrl: parsed.toString(), originalHost: hostname }; | |
| } | |
| // At this point we've validated that all resolved addresses are non-private. | |
| // Preserve the original hostname in the URL so that HTTPS/TLS SNI and | |
| // certificate validation continue to work correctly. | |
| return { resolvedUrl: urlStr, originalHost: hostname }; | |
| } |
| if (!VALID_CHANNEL_TYPES.has(channel.channelType)) { | ||
| reply.code(400); | ||
| return { error: `Invalid channel type: ${channel.channelType}` }; |
There was a problem hiding this comment.
The new VALID_CHANNEL_TYPES check restricts bot creation to only telegram and discord, but the dashboard wizard lets users enable many other channel IDs (it has a “Show all” list). This will now cause 400s when users select e.g. Slack. Either expand the allowlist to match the dashboard channel IDs you intend to support, or update the dashboard flow to only allow the supported channels.
| if (!VALID_CHANNEL_TYPES.has(channel.channelType)) { | |
| reply.code(400); | |
| return { error: `Invalid channel type: ${channel.channelType}` }; | |
| if (!channel.channelType || typeof channel.channelType !== 'string') { | |
| reply.code(400); | |
| return { error: 'Channel type must be a non-empty string' }; |
| function setOwnership(path: string, mode: number): void { | ||
| chmodSync(path, mode); | ||
| tryChown(path, OPENCLAW_UID, OPENCLAW_GID); | ||
| } |
There was a problem hiding this comment.
setOwnership() always sets restrictive modes (0755/0644) and then silently skips chown on EPERM. If chown is skipped and the host user is not UID 1000, the bot container (runs as UID 1000) may lose write access to the bind-mounted workspace/sandbox directories. Consider falling back to more permissive modes when chown fails, or make UID/GID and/or modes configurable so non-root setups remain functional.
When tryChown gets EPERM (non-root host), directories kept 0755 owned by the host user, leaving container UID 1000 without write access. Now falls back to permissive mode (0777 dirs, 0666 files) so bots can always write to their workspace. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Allowlist localhost/127.0.0.1/host.docker.internal for model discovery so Ollama and local providers work on admin-authenticated endpoint - Preserve original hostname in resolveAndValidateUrl so HTTPS TLS/SNI and certificate validation work correctly - Expand VALID_CHANNEL_TYPES to match all 30 dashboard channel types - Add 12 tests for /api/models/discover endpoint covering auth, SSRF blocking, localhost allowlist, and input validation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
No description provided.