Automated Template Builds, Proxmox User ACLs, Improved LDAP Integration#189
Automated Template Builds, Proxmox User ACLs, Improved LDAP Integration#189runleveldev wants to merge 17 commits intomainfrom
Conversation
009f4cb to
edee54e
Compare
| const req = https.get(url, { headers }, (res) => { | ||
| // Handle redirects | ||
| if (res.statusCode === 301 || res.statusCode === 302 || res.statusCode === 307 || res.statusCode === 308) { | ||
| const location = res.headers.location; | ||
| if (!location) { | ||
| return reject(new Error(`Redirect without Location header (status ${res.statusCode})`)); | ||
| } | ||
| // Follow redirect (without auth headers for CDN) | ||
| return httpGet(location, {}, redirectCount + 1) | ||
| .then(resolve) | ||
| .catch(reject); | ||
| } | ||
|
|
||
| let data = ''; | ||
| res.on('data', chunk => data += chunk); | ||
| res.on('end', () => { | ||
| if (timedOut) return; | ||
| resolve({ statusCode: res.statusCode, headers: res.headers, body: data }); | ||
| }); | ||
| }); |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
In general, to fix SSRF you must prevent untrusted input from freely controlling the destination of outbound HTTP(S) requests. For this code, that means: (1) validating and constraining the registry hostname derived from the image parameter, and (2) optionally tightening redirect following so we don’t escape those constraints via Location headers. The intent here is to fetch metadata from Docker/OCI registries; we can keep that functionality by enforcing that only safe registry hosts are allowed (e.g., public registries or a configured set) and by rejecting image references that specify arbitrary hosts.
The single best fix with minimal behavioral change is to introduce a small validator in docker-registry.js that checks a registry hostname against an allow‑list / safety rules and to apply it before using registry/registryHost to build URLs. Specifically:
- Add a helper
isRegistryHostAllowed(host)that:- Ensures the host is a syntactically valid hostname or IPv4/IPv6 literal.
- Rejects hostnames that resolve to private/bogon IP ranges (RFC1918, loopback, link‑local, etc.) to prevent access to internal networks.
- Optionally, you can tighten further by allowing only a configured set like
docker.io,registry-1.docker.io,ghcr.io, etc. (I’ll implement a conservative allow‑list plus internal-IP blocking).
- Use Node’s built-in
dns.promises.lookup/netmodule to check resolved IPs, since we are allowed to import standard libraries. - In
getImageConfig(registry, repo, tag), before constructingregistryHostand URLs, callisRegistryHostAllowed(registryHost)and throw an error (or return an HTTP 400 via the caller) if it fails. This is the main path used byGET /metadata. - Optionally, add a similar check to any other function that constructs registry URLs with potentially user-controlled hostnames, but within the given snippets only
getImageConfigis directly in the flow.
Because getImageConfig is already async, we can comfortably make the validator async and await it. We’ll add the required imports (dns and net) at the top of docker-registry.js and keep everything else intact. This change preserves existing behavior for normal public registries, but prevents users from forcing the backend to call arbitrary/internal hosts.
|
Talked with Robert a bit and suggested adding some core packages to the Dockerfile, whether it be anything from tmux, jq, git, etc. Items that are commonly used to save time. |
No description provided.