fabi/Mangalord
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(admin-security): SSRF defence + CSRF fail-closed + analysis .no_proxy() (0.87.2)

Browse Source 
Four high/medium findings from the audit, plus their tests:

1. **SSRF on admin-editable URLs.** `CrawlerSettings::start_url` and
   `AnalysisSettings::endpoint` validated only with `Url::parse`. A
   hostile or CSRF-able admin could repoint at `169.254.169.254`
   (cloud metadata), `127.0.0.1:5432` (postgres), or any RFC1918 host
   — and the vision worker bearer-attaches an env-managed API key to
   every call. Extract `ensure_public_target` from `safety::is_safe_url`
   (allowlist-free public-host check: scheme + private-IP literal +
   localhost) and wire it into both fields. Docker DNS names
   (`mangalord-vision`) keep passing because they're hostnames, not
   IP literals. The analysis check is gated on `enabled=true` so the
   dev-default `localhost:8000` stays usable until the worker is
   actually turned on (toggling enabled=true later re-runs the gate).

2. **Admin CSRF fail-open default.** `ADMIN_ALLOWED_ORIGINS=` empty
   silently skipped the entire CSRF check, so an operator forgetting
   the env var shipped an unguarded admin surface. Cookie-auth POSTs
   now fail-closed when the allowlist is empty AND when neither
   `Origin` nor `Referer` accompanies the request. Two carve-outs:
   `Authorization: Bearer …` callers bypass (bots can't be CSRF'd),
   and requests with NO session cookie at all bypass to let the auth
   extractor return a clean 401 instead of a confusing 403.

3. **Analysis HTTP clients didn't `.no_proxy()`.** The crawler client
   already did. Ambient `HTTP_PROXY`/`HTTPS_PROXY` in container env
   would exfiltrate page-image bytes + the bearer key through an
   upstream proxy. Same for the readiness probe.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
MechaCat02
2026-06-22 21:04:50 +02:00
parent b14ed02670
commit dd25f073cd
10 changed files with 364 additions and 49 deletions
Download Patch File Download Diff File Expand all files Collapse all files

138
backend/src/settings.rs
View File

@@ -153,8 +153,13 @@ impl CrawlerSettings {
errs.push("chapter_workers", "must be at least 1");
}
if let Some(url) = self.start_url.as_deref().map(str::trim).filter(|s| !s.is_empty()) {
if reqwest::Url::parse(url).is_err() {
errs.push("start_url", "must be a valid absolute URL");
// SSRF defence: Url::parse alone admits http://169.254.169.254
// / http://127.0.0.1:5432 etc., and the browser would happily
// navigate there with the admin's session. ensure_public_target
// refuses literal-IP private ranges + localhost while still
// accepting public hosts.
if let Err(e) = crate::crawler::safety::ensure_public_target(url) {
errs.push("start_url", url_safety_message(&e));
}
}
if self.job_timeout_secs < 1 {
@@ -231,6 +236,22 @@ impl CrawlerSettings {
}
}
/// Render a [`crate::crawler::safety::UrlSafetyError`] as an operator-friendly
/// validation message. The variant detail is preserved so a deliberate
/// `10.0.0.1` value gets "private/loopback IP not allowed" rather than the
/// generic "invalid URL".
fn url_safety_message(e: &crate::crawler::safety::UrlSafetyError) -> &'static str {
use crate::crawler::safety::UrlSafetyError as E;
match e {
E::Unparseable => "must be a valid absolute URL",
E::BadScheme(_) => "must use http:// or https://",
E::NoHost => "must include a host",
E::Loopback => "must not point at localhost",
E::PrivateIp(_) => "must not point at a private/loopback/link-local IP (SSRF risk)",
E::HostNotAllowed(_) => "host is not on the allowlist",
}
}
/// Rebuild a [`DownloadAllowlist`] from the DTO, always seeding the start-url
/// and CDN hosts (mirrors `config::build_download_allowlist`).
fn build_allowlist(
@@ -334,8 +355,29 @@ impl AnalysisSettings {
if self.workers < 1 {
errs.push("workers", "must be at least 1");
}
if self.endpoint.trim().is_empty() || reqwest::Url::parse(self.endpoint.trim()).is_err() {
let trimmed_endpoint = self.endpoint.trim();
if trimmed_endpoint.is_empty() {
errs.push("endpoint", "must be a valid absolute URL");
} else if let Err(e) = reqwest::Url::parse(trimmed_endpoint) {
// Always reject obviously-malformed URLs (matches prior behaviour).
let _ = e;
errs.push("endpoint", "must be a valid absolute URL");
} else if self.enabled {
// SSRF + API-key exfiltration defence — only enforced when the
// worker is actually live. Worker attaches an env-managed bearer
// token to every call; without this check, an admin (or one
// CSRF-able request) could repoint the endpoint at
// http://169.254.169.254 (cloud metadata) or http://postgres:5432
// (an internal service that would receive the bearer header).
// Docker DNS names (`mangalord-vision`) still pass because
// they're hostnames, not IP literals.
//
// A disabled config is allowed to carry the dev-default
// `http://localhost:8000/...` placeholder; the worker won't dial
// it, and toggling `enabled=true` later re-runs this validator.
if let Err(e) = crate::crawler::safety::ensure_public_target(trimmed_endpoint) {
errs.push("endpoint", url_safety_message(&e));
}
}
if self.enabled && self.model.trim().is_empty() {
errs.push("model", "required when analysis is enabled");
@@ -525,6 +567,37 @@ mod tests {
assert!(!out.download_allowlist.is_allow_any());
}
#[test]
fn crawler_start_url_rejects_ip_literal_attacks() {
let base = CrawlerConfig::default();
for url in [
"http://169.254.169.254/",
"http://127.0.0.1/catalog",
"http://localhost:5432/",
"http://10.0.0.1/",
] {
let dto = CrawlerSettings {
start_url: Some(url.to_string()),
..CrawlerSettings::from_config(&base)
};
let errs = dto.to_config(&base).unwrap_err();
let fields: Vec<_> = errs.errors.iter().map(|e| e.field.as_str()).collect();
assert!(fields.contains(&"start_url"), "must reject {url}");
}
}
#[test]
fn crawler_start_url_accepts_public_targets() {
let base = CrawlerConfig::default();
for url in ["https://catalog.example.com/", "http://catalog.example.com/"] {
let dto = CrawlerSettings {
start_url: Some(url.to_string()),
..CrawlerSettings::from_config(&base)
};
assert!(dto.to_config(&base).is_ok(), "must accept {url}");
}
}
#[test]
fn crawler_allow_any_host_bypasses_list() {
let base = CrawlerConfig::default();
@@ -617,6 +690,65 @@ mod tests {
}
}
#[test]
fn analysis_endpoint_rejects_ip_literal_attacks_when_enabled() {
// The vision worker bearer-attaches an env secret to every call;
// a hostile/CSRF-able admin must NOT be able to point endpoint at
// cloud metadata, loopback services, or RFC1918 hosts — when the
// worker is enabled (toggling enabled=true later re-runs this gate).
let base = AnalysisConfig::default();
for url in [
"http://169.254.169.254/v1/chat/completions",
"http://127.0.0.1:5432/",
"http://localhost:11434/v1/chat/completions",
"http://10.0.0.5/v1/chat/completions",
] {
let dto = AnalysisSettings {
enabled: true,
model: "test-model".to_string(),
endpoint: url.to_string(),
..AnalysisSettings::from_config(&base)
};
let errs = dto.to_config(&base).unwrap_err();
let fields: Vec<_> = errs.errors.iter().map(|e| e.field.as_str()).collect();
assert!(fields.contains(&"endpoint"), "must reject {url}");
}
}
#[test]
fn analysis_endpoint_accepts_docker_dns_and_public_hosts_when_enabled() {
// The documented default — docker DNS name resolving to a private IP
// at runtime — must still validate, because the bearer recipient
// identity is the operator-chosen hostname, not the underlying IP.
let base = AnalysisConfig::default();
for url in [
"http://mangalord-vision:8000/v1/chat/completions",
"https://api.openai.com/v1/chat/completions",
] {
let dto = AnalysisSettings {
enabled: true,
model: "test-model".to_string(),
endpoint: url.to_string(),
..AnalysisSettings::from_config(&base)
};
assert!(dto.to_config(&base).is_ok(), "must accept {url}");
}
}
#[test]
fn analysis_endpoint_skips_safety_check_when_disabled() {
// Disabled-but-saved bad URL is harmless (worker won't dial it);
// operators get to keep the dev-localhost placeholder until they
// toggle the worker on, when the validator re-runs.
let base = AnalysisConfig::default();
let dto = AnalysisSettings {
enabled: false,
endpoint: "http://localhost:8000/v1/chat/completions".to_string(),
..AnalysisSettings::from_config(&base)
};
assert!(dto.to_config(&base).is_ok());
}
#[test]
fn analysis_requires_model_only_when_enabled() {
let base = AnalysisConfig::default();