Bladeren bron

fix(spoolman): allow LAN Spoolman in SSRF guard

  The SSRF guard added in this PR rejected all RFC-1918 private and loopback
  addresses, which breaks Bambuddy's primary deployment topology — Spoolman
  running on the same LAN as Bambuddy (192.168.x.x, 10.x.x.x, 127.0.0.1).
  Users hit "Spoolman URL must not point to a private, loopback, link-local,
  multicast, or unspecified address" on legitimate setups.

  Rescope the guard to block what's actually dangerous in this context:
  cloud metadata endpoints (AWS/Alibaba IMDS), multicast, unspecified,
  non-http(s) schemes, and numeric-encoded IP bypasses. Loopback and
  RFC-1918 ranges are now explicitly permitted.

  Tests:
  - test_ssrf_blocked_schemes_and_addresses updated with refined block list
  - test_ssrf_allows_lan_spoolman_topologies (new) asserts loopback +
    RFC-1918 are accepted so this regression cannot recur silently
  - TestSpoolmanInventorySSRFSpoolBuddyPath parametrize lists trimmed
maziggy 1 maand geleden
bovenliggende
commit
4416fd4577

+ 40 - 30
backend/app/api/routes/_spoolman_helpers.py

@@ -15,26 +15,42 @@ from urllib.parse import urlparse
 logger = logging.getLogger(__name__)
 
 
+_CLOUD_METADATA_IPS = frozenset(
+    {
+        # AWS / GCP / Azure / Oracle / DigitalOcean IMDS
+        ipaddress.ip_address("169.254.169.254"),
+        # Alibaba Cloud metadata
+        ipaddress.ip_address("100.100.100.200"),
+        # AWS IMDS IPv6
+        ipaddress.ip_address("fd00:ec2::254"),
+    }
+)
+
+
 def assert_safe_spoolman_url(url: str) -> None:
     """Raise ValueError if *url* should be blocked as an SSRF risk.
 
+    Bambuddy is typically deployed on a home LAN alongside Spoolman, so
+    loopback (127.0.0.1) and RFC-1918 private ranges (192.168.x.x, 10.x.x.x,
+    172.16-31.x) must be permitted — they are THE normal Spoolman topology.
+    This guard therefore targets the genuinely dangerous cases only.
+
     Checks performed:
-    - Scheme must be http or https.
+    - Scheme must be http or https (no file://, gopher://, dict://, etc.).
     - Numeric-encoded IP addresses in decimal (e.g. ``2130706433``) or hex
-      (e.g. ``0x7f000001``) are rejected — Python's ``ipaddress`` module raises
-      ``ValueError`` for these forms so they would otherwise bypass the IP-range
-      guard, but libc resolves them as valid IPv4 addresses.
-    - Bare numeric IP hosts in loopback (127.x, ::1), link-local (169.254.x,
-      fe80::), private (RFC-1918), multicast (224.x, ff::/8), or unspecified
-      (0.0.0.0, ::) ranges are rejected.
-    - IPv4-mapped IPv6 addresses (::ffff:x.x.x.x) are unwrapped to their IPv4
-      equivalent and subject to the same checks.
-
-    Hostname-based addresses ("localhost", "internal.corp") require DNS resolution
-    and are outside the scope of this guard — they are mitigated by network-level
-    controls in the deployment environment.  "localhost" is intentionally *not*
-    blocked here because running Spoolman on the same host is a common and
-    supported topology.
+      (e.g. ``0x7f000001``) are rejected. Python's ``ipaddress`` module raises
+      ``ValueError`` for these forms so they would otherwise bypass the
+      explicit-IP block below, but libc (and browsers) resolve them as valid
+      IPv4 addresses.
+    - Cloud provider metadata endpoints (169.254.169.254, 100.100.100.200,
+      fd00:ec2::254) are blocked — the classic SSRF credential-exfil target.
+    - Multicast (224.0.0.0/4, ff00::/8) and unspecified (0.0.0.0, ::) addresses
+      are blocked — pointless as a destination and suggests misuse.
+    - IPv4-mapped IPv6 addresses (::ffff:x.x.x.x) are unwrapped so they cannot
+      bypass the checks above.
+
+    Hostname-based addresses ("localhost", "spoolman.lan", "internal.corp")
+    are out of scope — DNS resolution is deliberately not performed here.
     """
     parsed = urlparse(url)
     if parsed.scheme.lower() not in ("http", "https"):
@@ -43,9 +59,8 @@ def assert_safe_spoolman_url(url: str) -> None:
     hostname = (parsed.hostname or "").lower()
 
     # Reject decimal- and hex-encoded IPs (e.g. http://2130706433/ or
-    # http://0x7f000001/).  Python's ipaddress.ip_address() raises ValueError for
-    # these forms so they slip past the except-clause below, but the C library
-    # (and browsers) parse them as valid IPv4 addresses.
+    # http://0x7f000001/). These slip past ipaddress.ip_address() but libc
+    # (and browsers) parse them as IPv4 — an obvious bypass if not caught.
     if re.match(r"^(0x[0-9a-f]+|[0-9]+)$", hostname, re.I):
         raise ValueError("Spoolman URL must not use numeric-encoded IP addresses; use standard dotted-decimal notation")
 
@@ -55,22 +70,17 @@ def assert_safe_spoolman_url(url: str) -> None:
         # Not a bare IP — hostname-based addresses are out of scope.
         return
 
-    # Unwrap IPv4-mapped IPv6 (::ffff:169.254.x.x etc.) so their IPv4
-    # properties are evaluated correctly.
+    # Unwrap IPv4-mapped IPv6 (::ffff:169.254.169.254 etc.) so attackers can't
+    # encode a blocked IPv4 into an IPv6 literal to bypass the check.
     effective: ipaddress.IPv4Address | ipaddress.IPv6Address = addr
     if isinstance(addr, ipaddress.IPv6Address) and addr.ipv4_mapped is not None:
         effective = addr.ipv4_mapped
 
-    if (
-        effective.is_loopback
-        or effective.is_link_local
-        or effective.is_private
-        or effective.is_multicast
-        or effective.is_unspecified
-    ):
-        raise ValueError(
-            "Spoolman URL must not point to a private, loopback, link-local, multicast, or unspecified address"
-        )
+    if effective in _CLOUD_METADATA_IPS:
+        raise ValueError("Spoolman URL must not point to a cloud metadata endpoint")
+
+    if effective.is_multicast or effective.is_unspecified:
+        raise ValueError("Spoolman URL must not point to a multicast or unspecified address")
 
 
 _COLOR_HEX_RE = re.compile(r"^[0-9A-Fa-f]{6}$")

+ 40 - 9
backend/tests/integration/test_spoolman_inventory_api.py

@@ -758,14 +758,16 @@ class TestSpoolmanInventoryInputValidation:
             "file:///etc/passwd",
             "gopher://127.0.0.1:70/",
             "dict://internal.corp/",
-            "http://169.254.169.254/latest/meta-data/",  # AWS IMDS (link-local)
-            "http://[::1]:7912/",  # IPv6 loopback
-            "http://0.0.0.0/",  # unspecified
             "javascript:alert(1)",
+            "http://169.254.169.254/latest/meta-data/",  # AWS IMDS
+            "http://100.100.100.200/",  # Alibaba Cloud metadata
+            "http://[fd00:ec2::254]/",  # AWS IMDS IPv6
+            "http://0.0.0.0/",  # unspecified
             "http://224.0.0.1/",  # IPv4 multicast
             "http://[ff02::1]/",  # IPv6 multicast
-            "http://127.1.2.3/",  # 127.x.x.x loopback range
             "http://[::ffff:169.254.169.254]/",  # IPv4-mapped IPv6 IMDS bypass
+            "http://2130706433/",  # decimal-encoded 127.0.0.1
+            "http://0x7f000001/",  # hex-encoded 127.0.0.1
         ],
     )
     async def test_ssrf_blocked_schemes_and_addresses(
@@ -775,7 +777,10 @@ class TestSpoolmanInventoryInputValidation:
         mock_spoolman_client,
         evil_url: str,
     ):
-        """SSRF: any Spoolman URL that is not http(s) must be rejected with 400."""
+        """SSRF: dangerous schemes, cloud metadata IPs, multicast, unspecified,
+        and numeric-encoded IPs must be rejected with 400. Loopback and
+        RFC-1918 private ranges are allowed — they are legitimate Spoolman
+        topologies for self-hosted Bambuddy deployments."""
         from backend.app.models.settings import Settings
 
         db_session.add(Settings(key="spoolman_enabled", value="true"))
@@ -787,6 +792,36 @@ class TestSpoolmanInventoryInputValidation:
             f"Expected 400 for SSRF URL {evil_url!r} but got {response.status_code}: {response.json()}"
         )
 
+    @pytest.mark.asyncio
+    @pytest.mark.integration
+    @pytest.mark.parametrize(
+        "lan_url",
+        [
+            "http://127.0.0.1:7912/",  # loopback
+            "http://[::1]:7912/",  # IPv6 loopback
+            "http://192.168.1.50:7912/",  # RFC-1918 /16
+            "http://10.0.0.5:7912/",  # RFC-1918 /8
+            "http://172.20.0.3:7912/",  # RFC-1918 /12
+        ],
+    )
+    async def test_ssrf_allows_lan_spoolman_topologies(
+        self,
+        async_client: AsyncClient,
+        db_session,
+        mock_spoolman_client,
+        lan_url: str,
+    ):
+        """Regression: Bambuddy's normal deployment is LAN-local Spoolman.
+        Loopback and RFC-1918 private addresses must NOT be rejected as SSRF."""
+        from backend.app.models.settings import Settings
+
+        db_session.add(Settings(key="spoolman_enabled", value="true"))
+        db_session.add(Settings(key="spoolman_url", value=lan_url))
+        await db_session.commit()
+
+        response = await async_client.get("/api/v1/spoolman/inventory/spools")
+        assert response.status_code != 400, f"LAN URL {lan_url!r} was incorrectly blocked as SSRF: {response.json()}"
+
     @pytest.mark.asyncio
     @pytest.mark.integration
     async def test_create_rejects_storage_location_too_long(
@@ -1282,9 +1317,7 @@ class TestSpoolmanInventorySSRFSpoolBuddyPath:
         [
             "file:///etc/passwd",
             "http://169.254.169.254/latest/meta-data/",  # AWS IMDS
-            "http://[::1]:7912/",  # IPv6 loopback
             "http://0.0.0.0/",  # unspecified
-            "http://10.0.0.1/",  # RFC-1918 private
             "http://[::ffff:169.254.169.254]/",  # IPv4-mapped IMDS bypass
         ],
     )
@@ -1323,7 +1356,6 @@ class TestSpoolmanInventorySSRFSpoolBuddyPath:
         "evil_url",
         [
             "http://169.254.169.254/latest/meta-data/",  # AWS IMDS
-            "http://10.0.0.1/",  # RFC-1918 private
             "http://[::ffff:169.254.169.254]/",  # IPv4-mapped IMDS bypass
         ],
     )
@@ -1382,7 +1414,6 @@ class TestSpoolmanInventorySSRFSpoolBuddyPath:
         "evil_url",
         [
             "http://169.254.169.254/latest/meta-data/",  # AWS IMDS
-            "http://10.0.0.1/",  # RFC-1918 private
         ],
     )
     async def test_scale_update_weight_with_ssrf_url_degrades_gracefully(

+ 50 - 10
backend/tests/unit/services/test_spoolman_service.py

@@ -452,22 +452,62 @@ class TestSpoolmanClient:
 
 
 class TestInitSpoolmanClientSSRFGuard:
-    """init_spoolman_client must reject unsafe URLs before creating a client."""
+    """init_spoolman_client must reject genuinely unsafe URLs before creating a client.
 
-    @pytest.mark.asyncio
-    async def test_private_ip_raises_value_error(self):
-        with pytest.raises(ValueError, match="private|loopback|link-local|multicast|unspecified"):
-            await init_spoolman_client("http://10.0.0.1/")
+    Scope: cloud metadata endpoints, multicast, unspecified, non-http(s) schemes,
+    and numeric-encoded IP bypasses. Loopback and RFC-1918 private ranges are
+    explicitly allowed — Bambuddy's primary deployment is LAN-local Spoolman.
+    """
 
     @pytest.mark.asyncio
-    async def test_link_local_raises_value_error(self):
-        with pytest.raises(ValueError):
+    async def test_cloud_metadata_raises_value_error(self):
+        with pytest.raises(ValueError, match="cloud metadata"):
             await init_spoolman_client("http://169.254.169.254/latest/meta-data/")
 
     @pytest.mark.asyncio
-    async def test_loopback_ip_raises_value_error(self):
-        with pytest.raises(ValueError):
-            await init_spoolman_client("http://127.0.0.1:7912/")
+    async def test_multicast_raises_value_error(self):
+        with pytest.raises(ValueError, match="multicast|unspecified"):
+            await init_spoolman_client("http://224.0.0.1/")
+
+    @pytest.mark.asyncio
+    async def test_unspecified_raises_value_error(self):
+        with pytest.raises(ValueError, match="multicast|unspecified"):
+            await init_spoolman_client("http://0.0.0.0/")
+
+    @pytest.mark.asyncio
+    async def test_numeric_encoded_ip_raises_value_error(self):
+        # decimal-encoded 127.0.0.1 — libc resolves these but ipaddress doesn't
+        with pytest.raises(ValueError, match="numeric-encoded"):
+            await init_spoolman_client("http://2130706433/")
+
+    @pytest.mark.asyncio
+    async def test_non_http_scheme_raises_value_error(self):
+        with pytest.raises(ValueError, match="http or https"):
+            await init_spoolman_client("file:///etc/passwd")
+
+    @pytest.mark.asyncio
+    async def test_private_ip_is_allowed(self):
+        """Regression: RFC-1918 private addresses are the normal LAN topology."""
+        mock_instance = AsyncMock()
+        with (
+            patch("backend.app.services.spoolman._spoolman_client", None),
+            patch("backend.app.services.spoolman.SpoolmanClient", return_value=mock_instance) as mock_cls,
+        ):
+            client = await init_spoolman_client("http://192.168.1.50:7912/")
+        mock_cls.assert_called_once_with("http://192.168.1.50:7912/")
+        assert client is mock_instance
+
+    @pytest.mark.asyncio
+    async def test_loopback_ip_is_allowed(self):
+        """Regression: same-host Spoolman via loopback is a supported topology."""
+        mock_instance = AsyncMock()
+        with (
+            patch("backend.app.services.spoolman._spoolman_client", None),
+            patch("backend.app.services.spoolman.SpoolmanClient", return_value=mock_instance) as mock_cls,
+        ):
+            client = await init_spoolman_client("http://127.0.0.1:7912/")
+        mock_cls.assert_called_once_with("http://127.0.0.1:7912/")
+        assert client is mock_instance
 
     @pytest.mark.asyncio
     async def test_localhost_hostname_is_allowed(self):

File diff suppressed because it is too large
+ 0 - 0
static/assets/index-DwiMX8U1.js


+ 1 - 1
static/index.html

@@ -26,7 +26,7 @@
 
     <!-- Splash screens for iOS -->
     <link rel="apple-touch-startup-image" href="/img/android-chrome-512x512.png" />
-    <script type="module" crossorigin src="/assets/index-C8M6qM08.js"></script>
+    <script type="module" crossorigin src="/assets/index-DwiMX8U1.js"></script>
     <link rel="stylesheet" crossorigin href="/assets/index-CgXLDG6B.css">
   </head>
   <body>

Some files were not shown because too many files changed in this diff