Skip to content

fix(ai,llamacpp-server,stable-diffusion-server): strict local-only URL allow-list (sec)#533

Merged
sroussey merged 4 commits into
mainfrom
claude/sweet-edison-aSjg6
May 24, 2026
Merged

fix(ai,llamacpp-server,stable-diffusion-server): strict local-only URL allow-list (sec)#533
sroussey merged 4 commits into
mainfrom
claude/sweet-edison-aSjg6

Conversation

@sroussey
Copy link
Copy Markdown
Collaborator

Summary

Addresses a HIGH-severity review finding: the isLocalHostname heuristic in LlamaCppServer_Client.ts and StableDiffusionCpp_Client.ts could be bypassed.

The bypass

The previous check was a two-line string-prefix heuristic, duplicated across both provider clients:

if (host === "localhost" || host.endsWith(".localhost")) return true;
// ...
function isLocalIpv6(hostname: string): boolean {
  return (
    hostname === "::1" ||
    hostname.startsWith("fc") ||
    hostname.startsWith("fd") ||
    hostname.startsWith("fe80:")
  );
}

Concrete bypasses:

  • http://attacker.localhost/ was accepted because of the .endsWith(".localhost") rule, but attacker.localhost resolves via DNS to whatever the attacker controls (DNS-rebind / spoofed-A-record).
  • http://[feed::1]/ was accepted by the startsWith("fc") / startsWith("fd") / startsWith("fe80:") check vs IPv6 strings — feed::1 starts with fe but is not in fe80::/10. The check did no structural parsing at all.
  • Anything that DNS-resolved at runtime was at the mercy of the resolver, with no defence in depth.

The fix

New module packages/ai/src/provider-utils/localUrl.ts exporting:

  • parseIpv6(host) — proper structured IPv6 parser that returns 16 bytes (or null), handles :: expansion, IPv4-suffix forms (::ffff:127.0.0.1), and rejects zone IDs and out-of-range groups.
  • isLocalIpv4(host) — accepts only loopback 127.0.0.0/8, RFC 1918 (10/8, 172.16/12, 192.168/16), and link-local 169.254/16. Rejects 0.0.0.0, 255.255.255.255, leading zeros, and malformed input.
  • isLocalIpv6(host) — applies CIDR-style masks: ::1, fc00::/7 (ULA), fe80::/10 (link-local), and IPv4-mapped ::ffff:0:0/96 deferring to isLocalIpv4. No string-prefix matching.
  • isLocalHostname(host) — accepts only the literal localhost or an IPv4/IPv6 literal. A strict ^[0-9a-f:.]+$ character-class gate closes *.localhost, IDN, percent-encoded forms, underscores, and any other DNS-rebindable name.
  • normalizeLocalHttpUrl(rawUrl, label) — full pipeline: parses as URL, rejects non-http(s), rejects credentialed URLs, strips IPv6 brackets, calls isLocalHostname, and returns canonical form with query/hash stripped and trailing slashes removed. label parameterises the thrown error so callers don't need to wrap.

normalizeServerBaseUrl in both provider clients is now a thin wrapper that calls normalizeLocalHttpUrl(rawUrl, "LlamaCppServer") / normalizeLocalHttpUrl(rawUrl, "StableDiffusionCpp"). The previous local isLocalHostname / isLocalIpv4 / isLocalIpv6 / removeIpv6Brackets duplication is deleted.

Key test cases (packages/ai/src/provider-utils/localUrl.test.ts)

Acceptance: http://localhost:8080, http://127.0.0.1, http://10.0.0.1, http://192.168.1.1, http://172.16.0.1, http://172.31.255.255, http://169.254.1.1, http://[::1]/, http://[fc00::1]/, http://[FCAF:FE::1]/, http://[fd12:3456:789a::1]/, http://[fe80::1]/, http://[::ffff:127.0.0.1]/, https://localhost:8443/.

Rejection (regression tests for the bypasses): http://attacker.localhost/, http://localhost.attacker.com/, http://[feed::1]/, http://[fcZZ::1]/, http://[2001:db8::1]/, http://[fe80::1%eth0]/, http://172.32.0.1/, http://172.15.255.255/, http://8.8.8.8/, http://0.0.0.0/, http://255.255.255.255/, http://010.0.0.1/, http://1.2.3/, ftp://localhost/, ws://localhost/, file:///etc/passwd, http://user:pw@localhost/, not-a-url.

Canonicalisation: http://localhost/v1/?x=1#y -> http://localhost/v1. Hostname is lowercased; path case is preserved.

Error labels: parametric tests verify the provider label (LlamaCppServer / StableDiffusionCpp) appears in every thrown error path.

Breaking change

*.localhost (e.g. foo.localhost) is no longer accepted as a local base URL. This is intentional — it is the bypass vector that motivated this fix. Operators relying on *.localhost should switch to localhost or a loopback IP literal.

Notes / deviations from the plan

  • Test framework is Vitest, confirmed against packages/ai/src/task/index.test.ts. Tests use describe/it/expect from vitest.
  • The barrel for @workglow/ai/provider-utils is packages/ai/src/provider-utils.ts (single file with re-exports, not provider-utils/index.ts); added one new export * from "./provider-utils/localUrl" line to it.
  • For IPv6 acceptance cases where the WHATWG URL parser canonicalises hostnames in runtime-specific ways (case folding, converting ::ffff:127.0.0.1 to ::ffff:7f00:1, zero-run compression), tests assert acceptance only rather than exact canonical output. Exact-output assertions cover the stable cases ([::1], [fc00::1], [fe80::1], IPv4 literals, localhost).
  • The plan's http://[FCAFFE::1]/ example contains 6 hex digits in one group, which is not valid IPv6 (max 4 per group). Replaced with http://[FCAF:FE::1]/ which exercises the same uppercase-ULA acceptance path.

Test plan

  • bun run test packages/ai/src/provider-utils/localUrl.test.ts (or vitest run against packages/ai)
  • bun run build-types in packages/ai, providers/llamacpp-server, providers/stable-diffusion-server (ensure removed import / signature changes type-check)
  • bun run lint in the three affected packages
  • Smoke: call normalizeServerBaseUrl("http://attacker.localhost/") from each provider — must throw with the right label
  • Smoke: call normalizeServerBaseUrl("http://[feed::1]/") from each provider — must throw

Co-authored-by: Claude <noreply@anthropic.com>


Generated by Claude Code

…L allow-list (sec)

Replace the two-line string-prefix `isLocalHostname` heuristics in the
llama-server and stable-diffusion-server provider clients with a strict,
structured parser centralised in `@workglow/ai/provider-utils`.

The previous check accepted `attacker.localhost` via a `.endsWith(".localhost")`
suffix match (a DNS-rebind bypass) and accepted any IPv6 string merely starting
with `"fc"`, `"fd"`, or `"fe80:"` (no structural validation). The new helper
parses IPv6 into 16 bytes and applies CIDR-style masks for `::1`, `fc00::/7`,
and `fe80::/10` (plus IPv4-mapped equivalents), and shrinks the hostname
grammar to forms that cannot be DNS-rebound: literal `localhost`, RFC 1918 /
loopback / link-local IPv4 literals, and the IPv6 ranges above.

BREAKING: `*.localhost` (e.g. `foo.localhost`) is no longer accepted as a
local base URL.

Co-authored-by: Claude <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR centralizes strict local-only URL validation for local AI backend providers, replacing duplicated hostname/IP heuristics in the llama.cpp and stable-diffusion.cpp clients with a shared provider utility.

Changes:

  • Adds localUrl helpers for IPv4/IPv6 parsing, local allow-list checks, and canonical HTTP(S) URL normalization.
  • Updates both provider clients to delegate normalizeServerBaseUrl to the shared helper.
  • Adds regression tests and exports the new helpers via @workglow/ai/provider-utils.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
packages/ai/src/provider-utils/localUrl.ts Adds shared local-only URL parsing, validation, and canonicalization helpers.
packages/ai/src/provider-utils/localUrl.test.ts Adds tests for local URL acceptance, rejection, canonicalization, and error labels.
packages/ai/src/provider-utils.ts Re-exports the new local URL utilities.
providers/llamacpp-server/src/ai/common/LlamaCppServer_Client.ts Replaces duplicated URL validation with the shared helper.
providers/stable-diffusion-server/src/ai/common/StableDiffusionCpp_Client.ts Replaces duplicated URL validation with the shared helper.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +215 to +217
const bareHostname = stripIpv6Brackets(url.hostname);
if (!isLocalHostname(bareHostname)) {
throw new Error(`${label}: refusing non-local base URL ${rawUrl}`);
const rightGroups = right === "" ? [] : right.split(":");
const totalSlots = ipv4Tail ? 6 : 8;
const fill = totalSlots - leftGroups.length - rightGroups.length;
if (fill < 0) return null;
// the canonical output below.
const bareHostname = stripIpv6Brackets(url.hostname);
if (!isLocalHostname(bareHostname)) {
throw new Error(`${label}: refusing non-local base URL ${rawUrl}`);
Comment on lines +7 to +14
import { describe, expect, it } from "vitest";
import {
isLocalHostname,
isLocalIpv4,
isLocalIpv6,
normalizeLocalHttpUrl,
parseIpv6,
} from "./localUrl";
The pre-existing tests in packages/test/src/test/ai-provider-api/{LlamaCppServer,StableDiffusionCpp}_Client.test.ts assert the error matches /local HTTP/. The new error wording "refusing non-local base URL …" dropped that phrase. Restore it while keeping the URL in the message for debuggability.

Co-authored-by: Claude <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 24, 2026

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 62.03% 24471 / 39448
🔵 Statements 61.89% 25324 / 40914
🔵 Functions 62.9% 4629 / 7359
🔵 Branches 50.67% 11987 / 23656
File CoverageNo changed files found.
Generated in workflow #2428 for commit da651f1 by the Vitest Coverage Report Action

sroussey and others added 2 commits May 24, 2026 09:30
…ctness

Two Copilot review findings on PR #533:

1. `url.hostname` from `new URL()` is post-canonicalization. WHATWG
   rewrites `0x7f.0.0.1`, `2130706433`, and lenient-mode `010.0.0.1`
   to canonical dotted-quads, which silently bypass the strict-literal
   `isLocalHostname` grammar. Extract the host from the raw URL string
   via `extractRawHost(rawUrl)` and validate THAT literal instead.

2. The IPv6 parser accepted `fc00:0:0:0:0:0:0::1` (8 explicit groups
   plus a `::`) because the `fill` check was `< 0` rather than `<= 0`.
   `::` MUST compress one or more zero groups (RFC 4291 §2.2). Tighten
   to reject `fill <= 0`.

Also addresses the third finding (test discovery): the unit tests for
this module lived at `packages/ai/src/provider-utils/localUrl.test.ts`,
which is outside the `packages/test/src/test/*` glob the
`scripts/test.ts` harness scans, so they did not actually run in CI.
Move them under `packages/test/src/test/ai-provider-api/` (the
follow-up commit deletes the old colocated file) and add regression
cases for both new fixes plus the `local HTTP` wording pin.

Co-authored-by: Claude <noreply@anthropic.com>
Follow-up to the previous commit: this file lived at
packages/ai/src/provider-utils/localUrl.test.ts but never ran in CI
(the scripts/test.ts harness only scans packages/test/src/test/*).
The same suite — with the new WHATWG-bypass and IPv6 strictness
regressions added — now lives at
packages/test/src/test/ai-provider-api/localUrl.test.ts.

Co-authored-by: Claude <noreply@anthropic.com>
@sroussey sroussey merged commit 89cbbdb into main May 24, 2026
13 checks passed
sroussey added a commit that referenced this pull request May 26, 2026
…s against local-only allow-list (sec)

PR #533 added a local-only base-URL allow-list (isLocalHostname /
normalizeLocalHttpUrl / extractRawHost in @workglow/ai provider-utils).
It validates ONLY the initial base URL. Provider clients then call bare
fetch(...) with the default redirect:"follow", so a validated local
server that returns `302 Location: http://169.254.169.254/...` is
followed transparently to the new host — a residual SSRF bypass (e.g.
cloud-metadata exfiltration) that base-URL-only validation cannot stop.

Add localOnlyFetch (packages/ai/src/provider-utils/localOnlyFetch.ts):
fetch() with redirect:"manual" that re-validates EVERY 3xx Location
(resolved against the current URL) through the same isLocalHostname
allow-list, rejects non-HTTP(S) and credentialed redirect targets, and
caps the chain at 5 hops. Re-export it from the provider-utils barrel.

Route every provider fetch() that talks to an on-host backend through
localOnlyFetch with a provider label (LlamaCppServer / StableDiffusionCpp):
llamacpp-server ModelInfo (x2), ModelSearch, TextGeneration, ToolCalling,
TextEmbedding, TextSummary, TextRewriter; stable-diffusion-server
ModelInfo, ModelSearch, ImageGenerate, ImageEdit. Streaming response
handling is unchanged — the helper returns the final Response.

Adds packages/test/src/test/ai-provider-api/localOnlyFetch.test.ts.
sroussey added a commit that referenced this pull request May 27, 2026
…#536)

* fix(ai,llamacpp-server,stable-diffusion-server): re-validate redirects against local-only allow-list (sec)

PR #533 added a local-only base-URL allow-list (isLocalHostname /
normalizeLocalHttpUrl / extractRawHost in @workglow/ai provider-utils).
It validates ONLY the initial base URL. Provider clients then call bare
fetch(...) with the default redirect:"follow", so a validated local
server that returns `302 Location: http://169.254.169.254/...` is
followed transparently to the new host — a residual SSRF bypass (e.g.
cloud-metadata exfiltration) that base-URL-only validation cannot stop.

Add localOnlyFetch (packages/ai/src/provider-utils/localOnlyFetch.ts):
fetch() with redirect:"manual" that re-validates EVERY 3xx Location
(resolved against the current URL) through the same isLocalHostname
allow-list, rejects non-HTTP(S) and credentialed redirect targets, and
caps the chain at 5 hops. Re-export it from the provider-utils barrel.

Route every provider fetch() that talks to an on-host backend through
localOnlyFetch with a provider label (LlamaCppServer / StableDiffusionCpp):
llamacpp-server ModelInfo (x2), ModelSearch, TextGeneration, ToolCalling,
TextEmbedding, TextSummary, TextRewriter; stable-diffusion-server
ModelInfo, ModelSearch, ImageGenerate, ImageEdit. Streaming response
handling is unchanged — the helper returns the final Response.

Adds packages/test/src/test/ai-provider-api/localOnlyFetch.test.ts.

* fix(ai): make localOnlyFetch tolerant of non-spec Response mocks

Only treat a response as a 3xx redirect when its status is a real number
in [300,400) and it exposes a headers.get() method. Minimal test doubles
(e.g. { ok: true, json }) have undefined status/headers; previously
`undefined < 300 || undefined >= 400` was false, so they were misclassified
as redirects and `res.headers.get(...)` threw a TypeError, breaking
LocalBackendsProviderContracts unit tests. Real redirect Responses always
carry a numeric status and headers, so the SSRF re-validation on every hop
is unchanged.

* test: fix localOnlyFetch redirect case using link-local metadata IP

The "refuses a redirect to a non-local host" case targeted
169.254.169.254, but that address lives in the 169.254.0.0/16
link-local block, which isLocalHostname intentionally treats as local
(link-local backends are in scope). localOnlyFetch therefore followed
the redirect instead of rejecting it, ran the stub queue dry, and threw
"no response queued" rather than the expected "non-local host" message,
failing the assertion. Point the case at a genuinely external address
(RFC 5737 TEST-NET 203.0.113.10) so it exercises the non-local
rejection path without altering the SSRF allow-list.

* fix(ai): enforce loopback-only policy in localOnlyFetch + validate initial URL
sroussey added a commit that referenced this pull request May 31, 2026
…al-URL check (sec)

`assertLoopbackTarget` validated `new URL(input).hostname` for the initial URL,
which WHATWG canonicalises so `0x7f.0.0.1`, `2130706433`, and lenient `010.0.0.1`
all silently rewrite to `127.0.0.1`. Sibling `normalizeLocalHttpUrl` already closed
the same bypass in PR #533 by validating `extractRawHost(rawUrl)`; apply the same
fix here so any caller wiring `localOnlyFetch` against a user-supplied URL cannot
regress. Redirect targets stay on canonical hostname (no raw form available, and
the canonical check is still a tightening).

Regression tests in localOnlyFetch.test.ts cover all three IPv4 spellings and
assert zero fetches are made.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants