fix(security): reject raw paths that change the authenticated API origin (#102) (#113)

`request()`, `request_with_headers()` and the async `request()` built their URL
with a bare `urljoin(base_url + "/", path)`. urljoin implements RFC 3986
reference resolution, so a caller-supplied path beginning `//` is a network-path
reference and replaces the authority outright. The Authorization header is set
on the httpx client, not per request, so the customer's API key went with it.

Reproduced on a mock transport with a fixture key, before this change:

    '//fixture.invalid/v1/prices'      -> https://fixture.invalid/v1/prices
                                         Authorization: Token <key>
    '//user@fixture.invalid/v1/prices' -> https://user@fixture.invalid/v1/prices
                                         Authorization: Basic dXNlcjo=

The second form is worse than a leak: httpx reads the userinfo and swaps our
Token credential for a Basic one.

New `oilpriceapi/_url.py` resolves the URL once, centrally, and refuses
anything whose origin (scheme, host, port) is not exactly the configured base
origin. It also refuses backslashes (parsers downstream normalize "\" to "/")
and control characters (CR/LF request splitting) before resolution.

An explicit `base_url` keeps working — the guard pins to whatever the caller
configured, not to a hard-coded hostname — so proxies, staging hosts and test
servers are unaffected. Valid paths, encoded segments and query parameters are
unchanged.

Same defect class is filed against node #80, go #32, php #14.


Claude-Session: https://claude.ai/code/session_015ao5paex73xXvuM424Libo

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: karlwaldman

oilpriceapi/_url.py

@@ -0,0 +1,107 @@
+"""Pin every request to the configured API origin (#102).
+
+The clients used to build a request URL with a bare
+``urljoin(base_url + "/", path)``. ``urljoin`` implements RFC 3986 reference
+resolution, so a caller-supplied raw path that happens to be a *network-path
+reference* — anything starting ``//`` — replaces the authority outright:
+
+    urljoin("https://api.oilpriceapi.com/", "//elsewhere.example/v1/prices")
+    -> "https://elsewhere.example/v1/prices"
+
+The client's ``Authorization`` header is set on the httpx client, not per
+request, so the customer's API key travels to whatever host the caller's path
+resolved to. ``//user@elsewhere.example/...`` is worse still: httpx reads the
+userinfo and replaces the Token header with a Basic credential.
+
+This module resolves the URL once, centrally, and refuses anything whose origin
+(scheme, host, port) is not exactly the configured base origin. An explicit
+``base_url`` — a proxy, a staging host, a test server — keeps working, because
+the guard pins to whatever the caller configured, not to a hard-coded hostname.
+"""
+
+from __future__ import annotations
+
+from typing import Any, Tuple
+from urllib.parse import urljoin, urlsplit
+
+from .exceptions import ValidationError
+
+__all__ = ["resolve_api_url"]
+
+# Backslash: several URL parsers (WHATWG browsers, some proxies and gateways)
+# normalize "\" to "/", which turns "/\evil.example/x" into a network-path
+# reference after this SDK has already decided it was a plain path.
+# Control characters (including CR/LF and NUL) can split a request line or
+# smuggle a header. Neither can appear in a legitimate API path.
+_FORBIDDEN_CHARS = frozenset("\\") | frozenset(chr(c) for c in range(0x21)) | {chr(0x7F)}
+
+_DEFAULT_PORTS = {"http": 80, "https": 443}
+
+
+def _origin(url: str) -> Tuple[str, str, int]:
+    parts = urlsplit(url)
+    scheme = (parts.scheme or "").lower()
+    host = (parts.hostname or "").lower()
+    port = parts.port or _DEFAULT_PORTS.get(scheme, 0)
+    return scheme, host, port
+
+
+def _reject(path: Any, reason: str) -> "ValidationError":
+    # `path` is caller-supplied and carries no credential; the API key lives in
+    # a header, never in the path, so echoing it back is safe and is the only
+    # way the caller can see which value was refused.
+    return ValidationError(
+        message=(
+            f"Refusing to send this request: the path {reason}. "
+            "API paths must be relative to the configured base URL "
+            "(for example '/v1/prices/latest'). To talk to a different host, "
+            "construct a client with that base_url instead — a raw path may "
+            "not change the origin, because the API key would be sent to it."
+        ),
+        field="path",
+        value=path,
+    )
+
+
+def resolve_api_url(base_url: str, path: Any) -> str:
+    """Resolve ``path`` against ``base_url``, refusing any origin change.
+
+    Args:
+        base_url: The client's configured base URL (no trailing slash).
+        path: Caller-supplied API path.
+
+    Returns:
+        The absolute URL to request, guaranteed to share ``base_url``'s origin.
+
+    Raises:
+        ValidationError: If ``path`` is not a string, contains a character that
+            a URL parser could use to change the authority, or resolves to any
+            origin other than ``base_url``'s.
+    """
+    if not isinstance(path, str):
+        raise _reject(path, f"must be a string, got {type(path).__name__}")
+
+    if not path:
+        raise _reject(path, "is empty")
+
+    bad = _FORBIDDEN_CHARS.intersection(path)
+    if bad:
+        raise _reject(
+            path,
+            "contains a character that is not allowed in an API path "
+            f"({''.join(sorted(repr(c) for c in bad))})",
+        )
+
+    if "//" in path.split("?", 1)[0].split("#", 1)[0]:
+        # Covers the plain scheme-relative "//host/..." form and any absolute
+        # URL ("https://host/..."), before it can be resolved.
+        raise _reject(path, "may not contain '//'")
+
+    normalized = path if path.startswith("/") else "/" + path
+
+    url = urljoin(base_url + "/", normalized)
+
+    if _origin(url) != _origin(base_url):
+        raise _reject(path, "resolves to a different host than the configured base URL")
+
+    return url

oilpriceapi/async_client.py

@@ -11,13 +11,13 @@
 import os
 from datetime import datetime
 from typing import Any, AsyncGenerator, Dict, List, Optional, Union, cast
-from urllib.parse import urljoin
 
 import httpx
 
 logger = logging.getLogger(__name__)
 
 from ._subscriptions_common import unwrap_data
+from ._url import resolve_api_url
 from .async_resources import (
     AsyncAlertsResource,
     AsyncAnalyticsResource,
@@ -202,10 +202,10 @@ async def request(
         await self._ensure_client()
         assert self._client is not None  # set by _ensure_client
 
-        # Ensure path starts with / for proper urljoin behavior
-        if not path.startswith("/"):
-            path = "/" + path
-        url = urljoin(self.base_url + "/", path)
+        # Pin the request to the configured API origin. A raw path may not
+        # move the destination host, because the API key rides on this client
+        # and would go with it (#102).
+        url = resolve_api_url(self.base_url, path)
 
         # Retry logic
         import time as _time

oilpriceapi/client.py

@@ -8,7 +8,6 @@
 import os
 import time
 from typing import TYPE_CHECKING, Any, Dict, List, Optional, Tuple
-from urllib.parse import urljoin
 
 import httpx
 
@@ -18,6 +17,7 @@
 logger = logging.getLogger(__name__)
 
 from ._subscriptions_common import unwrap_data
+from ._url import resolve_api_url
 from .exceptions import (
     ConfigurationError,
     OilPriceAPIError,
@@ -231,10 +231,10 @@ def request(
             ServerError: On 5xx status
             TimeoutError: On request timeout
         """
-        # Ensure path starts with / for proper urljoin behavior
-        if not path.startswith("/"):
-            path = "/" + path
-        url = urljoin(self.base_url + "/", path)
+        # Pin the request to the configured API origin. A raw path may not
+        # move the destination host, because the API key rides on this client
+        # and would go with it (#102).
+        url = resolve_api_url(self.base_url, path)
 
         # Use provided timeout or default
         effective_timeout = timeout if timeout is not None else self.timeout
@@ -364,10 +364,10 @@ def request_with_headers(
         Returns:
             Tuple of (parsed JSON dict, httpx.Headers)
         """
-        # Ensure path starts with / for proper urljoin behavior
-        if not path.startswith("/"):
-            path = "/" + path
-        url = urljoin(self.base_url + "/", path)
+        # Pin the request to the configured API origin. A raw path may not
+        # move the destination host, because the API key rides on this client
+        # and would go with it (#102).
+        url = resolve_api_url(self.base_url, path)
 
         effective_timeout = timeout if timeout is not None else self.timeout
 

tests/unit/test_request_origin_guard.py

@@ -0,0 +1,197 @@
+"""Raw paths must never move the request off the authenticated API origin (#102).
+
+Every assertion here is made on what reaches the transport, not on an internal
+flag: an off-origin path must produce ZERO outbound requests, so the customer's
+API key is never attached to a host that is not OilPriceAPI.
+"""
+
+import asyncio
+
+import httpx
+import pytest
+
+from oilpriceapi import AsyncOilPriceAPI, OilPriceAPI
+from oilpriceapi.exceptions import OilPriceAPIError
+
+# Not a credential: a fixture string used only against a mock transport.
+FIXTURE_KEY = "-".join(["fixture", "not", "a", "real", "key"])
+
+FOREIGN_HOST = "fixture.invalid"
+
+# Raw paths a caller could build from untrusted input. Each one either moves the
+# origin outright or is normalized into an origin change by some parser in the
+# chain (scheme-relative, userinfo authority, backslash, absolute URL).
+OFF_ORIGIN_PATHS = [
+    "//fixture.invalid/v1/prices/latest",
+    "///fixture.invalid/v1/prices/latest",
+    "//user@fixture.invalid/v1/prices/latest",
+    "//fixture.invalid:8443/v1/prices/latest",
+    "/\\fixture.invalid/v1/prices/latest",
+    "\\\\fixture.invalid/v1/prices/latest",
+    "https://fixture.invalid/v1/prices/latest",
+    "http://fixture.invalid/v1/prices/latest",
+    "/v1/prices\nHost: fixture.invalid",
+]
+
+ON_ORIGIN_PATHS = [
+    "/v1/prices/latest",
+    "v1/prices/latest",
+    "/v1/prices/past_week",
+    "/v1/commodities/BRENT%20CRUDE/summary",
+    "/v1/subscriptions/9f3c1a2b-0000-4000-8000-000000000000",
+]
+
+
+class _Recorder:
+    """Mock transport that records every request that actually goes out."""
+
+    def __init__(self):
+        self.requests = []
+
+    def __call__(self, request):
+        self.requests.append(request)
+        return httpx.Response(200, json={"status": "success", "data": {}})
+
+    @property
+    def hosts(self):
+        return [r.url.host for r in self.requests]
+
+    @property
+    def authenticated_foreign_requests(self):
+        return [
+            r
+            for r in self.requests
+            if r.url.host != "api.oilpriceapi.com"
+            and (r.headers.get("authorization") or r.headers.get("Authorization"))
+        ]
+
+
+def _sync_client(recorder, **kwargs):
+    client = OilPriceAPI(api_key=FIXTURE_KEY, **kwargs)
+    client._client = httpx.Client(
+        base_url=client.base_url,
+        headers=client.headers,
+        transport=httpx.MockTransport(recorder),
+    )
+    return client
+
+
+def _async_client(recorder, **kwargs):
+    client = AsyncOilPriceAPI(api_key=FIXTURE_KEY, **kwargs)
+    client._client = httpx.AsyncClient(
+        base_url=client.base_url,
+        headers=client.headers,
+        transport=httpx.MockTransport(recorder),
+    )
+    return client
+
+
+@pytest.mark.parametrize("path", OFF_ORIGIN_PATHS)
+def test_sync_request_sends_nothing_for_off_origin_path(path):
+    recorder = _Recorder()
+    client = _sync_client(recorder)
+
+    with pytest.raises(OilPriceAPIError):
+        client.request("GET", path)
+
+    assert recorder.requests == [], (
+        f"path {path!r} reached the wire at {recorder.hosts}"
+    )
+
+
+@pytest.mark.parametrize("path", OFF_ORIGIN_PATHS)
+def test_sync_request_with_headers_sends_nothing_for_off_origin_path(path):
+    recorder = _Recorder()
+    client = _sync_client(recorder)
+
+    with pytest.raises(OilPriceAPIError):
+        client.request_with_headers("GET", path)
+
+    assert recorder.requests == []
+
+
+@pytest.mark.parametrize("path", OFF_ORIGIN_PATHS)
+def test_async_request_sends_nothing_for_off_origin_path(path):
+    recorder = _Recorder()
+
+    async def scenario():
+        client = _async_client(recorder)
+        with pytest.raises(OilPriceAPIError):
+            await client.request("GET", path)
+        await client._client.aclose()
+
+    asyncio.run(scenario())
+    assert recorder.requests == []
+
+
+def test_api_key_never_reaches_a_foreign_host():
+    """The property that actually matters: the key stays on our origin."""
+    recorder = _Recorder()
+    client = _sync_client(recorder)
+
+    for path in OFF_ORIGIN_PATHS:
+        try:
+            client.request("GET", path)
+        except OilPriceAPIError:
+            pass
+
+    assert recorder.authenticated_foreign_requests == []
+    assert FOREIGN_HOST not in recorder.hosts
+
+
+@pytest.mark.parametrize("path", ON_ORIGIN_PATHS)
+def test_valid_paths_still_reach_the_api_origin(path):
+    recorder = _Recorder()
+    client = _sync_client(recorder)
+
+    client.request("GET", path, params={"by_code": "BRENT_CRUDE_USD"})
+
+    assert len(recorder.requests) == 1
+    sent = recorder.requests[0]
+    assert sent.url.host == "api.oilpriceapi.com"
+    assert sent.url.scheme == "https"
+    assert sent.url.params.get("by_code") == "BRENT_CRUDE_USD"
+    assert sent.headers["Authorization"].endswith(FIXTURE_KEY)
+
+
+def test_explicit_custom_base_url_is_preserved():
+    """Proxies and test servers stay supported — the guard pins to the CONFIGURED origin."""
+    recorder = _Recorder()
+    client = _sync_client(recorder, base_url="https://proxy.internal.example:8443/api")
+
+    client.request("GET", "/v1/prices/latest")
+
+    assert len(recorder.requests) == 1
+    sent = recorder.requests[0]
+    assert sent.url.host == "proxy.internal.example"
+    assert sent.url.port == 8443
+
+
+def test_off_origin_path_is_still_rejected_under_a_custom_base_url():
+    recorder = _Recorder()
+    client = _sync_client(recorder, base_url="https://proxy.internal.example:8443/api")
+
+    with pytest.raises(OilPriceAPIError):
+        client.request("GET", "//fixture.invalid/v1/prices/latest")
+
+    assert recorder.requests == []
+
+
+def test_non_string_path_is_rejected_before_any_request():
+    recorder = _Recorder()
+    client = _sync_client(recorder)
+
+    with pytest.raises(OilPriceAPIError):
+        client.request("GET", None)
+
+    assert recorder.requests == []
+
+
+def test_rejection_message_does_not_leak_the_api_key():
+    recorder = _Recorder()
+    client = _sync_client(recorder)
+
+    with pytest.raises(OilPriceAPIError) as excinfo:
+        client.request("GET", "//fixture.invalid/v1/prices/latest")
+
+    assert FIXTURE_KEY not in str(excinfo.value)