From 58b8b8362e327ddbdb64158c3710c3a6e617c9e1 Mon Sep 17 00:00:00 2001 From: Praveen Mittal Date: Wed, 22 Jul 2026 00:44:39 +0200 Subject: [PATCH 1/2] fix: reject empty/whitespace/non-string credentials and empty media reference fields Fixes #15280 Two input-validation gaps found during an SDK audit: - Langfuse.__init__ checked `public_key is None` / `secret_key is None` to decide whether to disable the client, but an empty-string or whitespace-only env var (e.g. LANGFUSE_PUBLIC_KEY="") bypassed the check entirely, since "" is not None. The client then proceeded to construct a fully-wired tracer instead of cleanly disabling itself, failing only later with an opaque auth error on the first real request. Fixed by checking `isinstance(x, str)` and `x.strip()` instead of `x is None`, which also closes a non-string-input crash (e.g. public_key=123) along the way. - LangfuseMedia.parse_reference_string validated that required keys ("type", "id", "source") were present in the parsed data, but not that their values were non-empty, and crashed with a raw "not enough values to unpack" error on a malformed pipe-separated segment instead of its own documented ValueError. An empty media_id from this method flows directly into a real api.media.get() call in resolve_media_references. Fixed by skipping segments without "=" during parsing and checking value truthiness, not just key presence, in the final validation. Also includes: - New unit tests for LangfuseClient (langfuse/_utils/request.py) against a real local HTTP server via pytest-httpserver (previously a declared but unused dev dependency), covering malformed JSON responses, error status codes, and a slow/hung dependency actually enforcing the configured timeout end-to-end. - CONTRIBUTING.md: documented how to start a local Langfuse server for e2e tests (previously undocumented, despite being required); README.md: added a pointer to CONTRIBUTING.md. --- CONTRIBUTING.md | 33 +++++++- README.md | 4 + langfuse/_client/client.py | 4 +- langfuse/media.py | 9 +- tests/unit/test_initialization.py | 86 +++++++++++++++++++ tests/unit/test_media.py | 44 ++++++++++ tests/unit/test_request_client.py | 135 ++++++++++++++++++++++++++++++ 7 files changed, 310 insertions(+), 5 deletions(-) create mode 100644 tests/unit/test_request_client.py diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 45f1ed55d..791906411 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -36,13 +36,44 @@ Unit tests do not require a running Langfuse server: uv run --frozen pytest -n auto --dist worksteal tests/unit ``` -E2E tests require a running Langfuse server and environment variables based on `.env.template`: +E2E tests require a running Langfuse server and environment variables based on `.env.template`. To start one locally with Docker (the same approach CI uses in `.github/workflows/ci.yml`): + +```bash +mkdir -p ./langfuse-server && cd ./langfuse-server +curl -fsSL https://raw.githubusercontent.com/langfuse/langfuse/main/docker-compose.yml -o docker-compose.yml + +# These auto-provision a project on first boot with keys matching .env.template, +# so no manual setup via the UI is needed. +LANGFUSE_INIT_ORG_ID=0c6c96f4-0ca0-4f16-92a8-6dd7d7c6a501 \ +LANGFUSE_INIT_ORG_NAME="SDK Test Org" \ +LANGFUSE_INIT_PROJECT_ID=7a88fb47-b4e2-43b8-a06c-a5ce950dc53a \ +LANGFUSE_INIT_PROJECT_NAME="SDK Test Project" \ +LANGFUSE_INIT_PROJECT_PUBLIC_KEY=pk-lf-1234567890 \ +LANGFUSE_INIT_PROJECT_SECRET_KEY=sk-lf-1234567890 \ +LANGFUSE_INIT_USER_EMAIL=sdk-tests@langfuse.local \ +LANGFUSE_INIT_USER_NAME="SDK Tests" \ +LANGFUSE_INIT_USER_PASSWORD=langfuse-ci-password \ +NEXT_PUBLIC_LANGFUSE_RUN_NEXT_INIT=true \ +docker compose up -d + +# Wait until this succeeds before running e2e tests; the stack takes a +# moment to become healthy on first boot. +curl --fail --retry 20 --retry-delay 5 --retry-connrefused http://localhost:3000/api/public/health +``` + +Then, back in the repo root (`cd ..` from `./langfuse-server`), with `.env` copied from `.env.template`: ```bash uv run --frozen pytest -n 4 --dist worksteal tests/e2e -m "not serial_e2e" uv run --frozen pytest tests/e2e -m "serial_e2e" ``` +To stop the local server when done: + +```bash +(cd ./langfuse-server && docker compose down -v) +``` + Live-provider tests make real provider calls and require provider API keys: ```bash diff --git a/README.md b/README.md index e21f83618..464debbb2 100644 --- a/README.md +++ b/README.md @@ -52,3 +52,7 @@ langfuse.flush() - Full documentation: https://langfuse.com/docs - Machine-readable docs index (for AI agents): https://langfuse.com/llms.txt - API reference of this package: https://python.reference.langfuse.com + +## Contributing + +See [CONTRIBUTING.md](CONTRIBUTING.md) for local setup, quality checks, and the test suite layout. diff --git a/langfuse/_client/client.py b/langfuse/_client/client.py index d3f699399..268da791e 100644 --- a/langfuse/_client/client.py +++ b/langfuse/_client/client.py @@ -378,7 +378,7 @@ def __init__( langfuse_logger.setLevel(logging.DEBUG) public_key = public_key or os.environ.get(LANGFUSE_PUBLIC_KEY) - if public_key is None: + if not isinstance(public_key, str) or not public_key.strip(): langfuse_logger.warning( "Authentication error: Langfuse client initialized without public_key. Client will be disabled. " "Provide a public_key parameter or set LANGFUSE_PUBLIC_KEY environment variable. " @@ -387,7 +387,7 @@ def __init__( return secret_key = secret_key or os.environ.get(LANGFUSE_SECRET_KEY) - if secret_key is None: + if not isinstance(secret_key, str) or not secret_key.strip(): langfuse_logger.warning( "Authentication error: Langfuse client initialized without secret_key. Client will be disabled. " "Provide a secret_key parameter or set LANGFUSE_SECRET_KEY environment variable. " diff --git a/langfuse/media.py b/langfuse/media.py index 80e5bcd6a..acbdc2417 100644 --- a/langfuse/media.py +++ b/langfuse/media.py @@ -253,11 +253,16 @@ def parse_reference_string(reference_string: str) -> ParsedMediaReference: parsed_data = {} for pair in pairs: + if "=" not in pair: + continue key, value = pair.split("=", 1) parsed_data[key] = value - # Verify all required fields are present - if not all(key in parsed_data for key in ["type", "id", "source"]): + # Verify all required fields are present and non-empty. An empty value + # (e.g. "id=") would otherwise pass this check and later reach a real + # API call (LangfuseMedia.resolve_media_references -> api.media.get) + # with an empty media_id. + if not all(parsed_data.get(key) for key in ["type", "id", "source"]): raise ValueError("Missing required fields in reference string") return ParsedMediaReference( diff --git a/tests/unit/test_initialization.py b/tests/unit/test_initialization.py index 6664d318f..89b64a584 100644 --- a/tests/unit/test_initialization.py +++ b/tests/unit/test_initialization.py @@ -7,6 +7,7 @@ import os import pytest +from opentelemetry.trace import NoOpTracer from langfuse import Langfuse from langfuse._client.resource_manager import LangfuseResourceManager @@ -297,3 +298,88 @@ def test_https_and_http_urls(self, cleanup_env_vars): secret_key="test_sk", ) assert client2._base_url == "http://insecure.com" + + +class TestCredentialValidation: + """An empty-string key (e.g. an unset secret rendered as "" by a + templated Docker/k8s env file) must disable the client the same way a + missing key does, rather than silently proceeding to construct a client + that then fails confusingly on every real request. + """ + + @pytest.fixture(autouse=True) + def cleanup(self): + original = { + "LANGFUSE_PUBLIC_KEY": os.environ.get("LANGFUSE_PUBLIC_KEY"), + "LANGFUSE_SECRET_KEY": os.environ.get("LANGFUSE_SECRET_KEY"), + } + + yield + + with LangfuseResourceManager._lock: + LangfuseResourceManager._instances.clear() + + for key, value in original.items(): + if value is None: + os.environ.pop(key, None) + else: + os.environ[key] = value + + def test_empty_string_public_key_env_var_disables_client(self, monkeypatch): + monkeypatch.setenv("LANGFUSE_PUBLIC_KEY", "") + monkeypatch.setenv("LANGFUSE_SECRET_KEY", "test_sk") + + client = Langfuse() + + assert isinstance(client._otel_tracer, NoOpTracer) + assert client._resources is None + + def test_empty_string_secret_key_env_var_disables_client(self, monkeypatch): + monkeypatch.setenv("LANGFUSE_PUBLIC_KEY", "test_pk") + monkeypatch.setenv("LANGFUSE_SECRET_KEY", "") + + client = Langfuse() + + assert isinstance(client._otel_tracer, NoOpTracer) + assert client._resources is None + + def test_empty_string_public_key_argument_disables_client(self, monkeypatch): + monkeypatch.delenv("LANGFUSE_PUBLIC_KEY", raising=False) + + client = Langfuse(public_key="", secret_key="test_sk") + + assert isinstance(client._otel_tracer, NoOpTracer) + assert client._resources is None + + def test_whitespace_only_public_key_argument_disables_client(self, monkeypatch): + # A whitespace-only value is falsy-string-adjacent but not caught by + # a plain truthiness check (" " is truthy); it must still disable + # the client rather than proceed with a blank credential. + monkeypatch.delenv("LANGFUSE_PUBLIC_KEY", raising=False) + + client = Langfuse(public_key=" ", secret_key="test_sk") + + assert isinstance(client._otel_tracer, NoOpTracer) + assert client._resources is None + + def test_whitespace_only_secret_key_argument_disables_client(self, monkeypatch): + monkeypatch.delenv("LANGFUSE_SECRET_KEY", raising=False) + + client = Langfuse(public_key="test_pk", secret_key=" ") + + assert isinstance(client._otel_tracer, NoOpTracer) + assert client._resources is None + + def test_non_string_public_key_argument_disables_client_without_crashing( + self, monkeypatch + ): + # A caller passing a non-string value (violating the declared + # Optional[str] type at runtime, since Python doesn't enforce it) + # must still disable the client cleanly rather than crash inside + # the validation guard itself (e.g. calling .strip() on an int). + monkeypatch.delenv("LANGFUSE_PUBLIC_KEY", raising=False) + + client = Langfuse(public_key=123, secret_key="test_sk") # type: ignore[arg-type] + + assert isinstance(client._otel_tracer, NoOpTracer) + assert client._resources is None diff --git a/tests/unit/test_media.py b/tests/unit/test_media.py index 343939722..454a21804 100644 --- a/tests/unit/test_media.py +++ b/tests/unit/test_media.py @@ -91,6 +91,50 @@ def test_parse_invalid_reference_string(): ) # Missing fields +def test_parse_reference_string_with_empty_field_values_raises_missing_fields_error(): + # All three required keys present, but with empty values, used to pass + # the "key in parsed_data" check and return a reference with an empty + # media_id -- which would then reach a real api.media.get("") network + # call in LangfuseMedia.resolve_media_references. + with pytest.raises(ValueError, match="Missing required fields"): + LangfuseMedia.parse_reference_string("@@@langfuseMedia:type=|id=|source=@@@") + + +def test_parse_reference_string_with_malformed_pair_raises_missing_fields_error(): + # A pipe-separated segment with no "=" (here, a typo dropping the "=" + # from "id=") used to crash with a raw "not enough values to unpack" + # ValueError from the key/value split, instead of the intended + # "Missing required fields" validation error. + with pytest.raises(ValueError, match="Missing required fields"): + LangfuseMedia.parse_reference_string( + "@@@langfuseMedia:type=image/jpeg|idtest-id|source=bytes@@@" + ) + + +def test_parse_reference_string_ignores_malformed_pair_when_fields_still_present(): + # A malformed segment alongside all three required fields elsewhere in + # the string should be ignored, not crash the whole parse. + result = LangfuseMedia.parse_reference_string( + "@@@langfuseMedia:type=image/jpeg|badpair|id=test-id|source=bytes@@@" + ) + + assert result["media_id"] == "test-id" + assert result["content_type"] == "image/jpeg" + assert result["source"] == "bytes" + + +def test_parse_reference_string_ignores_trailing_empty_pair(): + # A trailing "|" produces an empty segment with no "=", which should be + # ignored rather than crash, as long as the required fields are present. + result = LangfuseMedia.parse_reference_string( + "@@@langfuseMedia:type=image/jpeg|id=test-id|source=bytes|@@@" + ) + + assert result["media_id"] == "test-id" + assert result["content_type"] == "image/jpeg" + assert result["source"] == "bytes" + + @pytest.mark.parametrize( ("url_expiry", "expected"), [ diff --git a/tests/unit/test_request_client.py b/tests/unit/test_request_client.py new file mode 100644 index 000000000..ba0bd23a1 --- /dev/null +++ b/tests/unit/test_request_client.py @@ -0,0 +1,135 @@ +"""Unit tests for LangfuseClient (langfuse/_utils/request.py) against a real, +local HTTP server. + +This exercises the low-level HTTP layer's behavior under real failure modes +that mocked-response tests do not catch: a malformed/non-JSON response body, +an error status code with a body Langfuse doesn't recognize, and a slow/hung +dependency that must be bounded by the client's configured timeout rather +than hanging indefinitely. +""" + +import time + +import httpx +import pytest +from werkzeug.wrappers import Response as WerkzeugResponse + +from langfuse._utils.request import APIError, APIErrors, LangfuseClient + + +@pytest.fixture +def client(httpserver): + return LangfuseClient( + public_key="test-public-key", + secret_key="test-secret-key", + base_url=httpserver.url_for("/"), + version="test-version", + timeout=5, + session=httpx.Client(), + ) + + +def test_batch_post_success(client, httpserver): + httpserver.expect_request("/api/public/ingestion").respond_with_json( + {"successes": [], "errors": []} + ) + + response = client.batch_post(batch=[]) + + assert response.status_code == 200 + + +def test_batch_post_malformed_json_response_raises_api_error(client, httpserver): + # A 200 response whose body isn't valid JSON must not crash with a raw + # json.JSONDecodeError; it should surface as the SDK's own APIError. + httpserver.expect_request("/api/public/ingestion").respond_with_data( + "not valid json", status=200, content_type="application/json" + ) + + with pytest.raises(APIError, match="Invalid JSON response received"): + client._process_response( + httpx.get(httpserver.url_for("/api/public/ingestion")), + success_message="ok", + return_json=True, + ) + + +def test_batch_post_207_with_errors_raises_api_errors(client, httpserver): + httpserver.expect_request("/api/public/ingestion").respond_with_json( + { + "errors": [ + {"status": 400, "message": "Bad request", "error": "Invalid event"} + ] + }, + status=207, + ) + + with pytest.raises(APIErrors): + client.batch_post(batch=[{"bad": "event"}]) + + +def test_batch_post_207_malformed_json_raises_api_error(client, httpserver): + httpserver.expect_request("/api/public/ingestion").respond_with_data( + "not valid json", status=207, content_type="application/json" + ) + + with pytest.raises(APIError, match="Invalid JSON response received"): + client.batch_post(batch=[{"some": "event"}]) + + +def test_batch_post_unauthorized_raises_api_error_with_status(client, httpserver): + httpserver.expect_request("/api/public/ingestion").respond_with_json( + {"message": "Unauthorized"}, status=401 + ) + + with pytest.raises(APIError) as exc_info: + client.batch_post(batch=[{"some": "event"}]) + + assert exc_info.value.status == 401 + + +def test_batch_post_generic_error_with_non_json_body_raises_api_error( + client, httpserver +): + # A 500 with a plain-text (non-JSON) body must not crash trying to parse + # it as JSON; it should fall back to the raw response text. + httpserver.expect_request("/api/public/ingestion").respond_with_data( + "Internal Server Error", status=500, content_type="text/plain" + ) + + with pytest.raises(APIError) as exc_info: + client.batch_post(batch=[{"some": "event"}]) + + assert exc_info.value.status == 500 + assert "Internal Server Error" in str(exc_info.value) + + +def test_batch_post_times_out_on_slow_server_instead_of_hanging(httpserver): + # A dependency that never responds must not hang the caller forever: the + # configured timeout has to actually be enforced end-to-end, not just + # accepted as a constructor argument. + def slow_handler(_request): + time.sleep(2) + return WerkzeugResponse("late", status=200) + + httpserver.expect_request("/api/public/ingestion").respond_with_handler( + slow_handler + ) + + slow_client = LangfuseClient( + public_key="test-public-key", + secret_key="test-secret-key", + base_url=httpserver.url_for("/"), + version="test-version", + timeout=0.2, + session=httpx.Client(), + ) + + start = time.monotonic() + with pytest.raises(httpx.TimeoutException): + slow_client.batch_post(batch=[{"some": "event"}]) + elapsed = time.monotonic() - start + + # Bounded well below the handler's 2s sleep: the client's own timeout + # fired rather than eventually receiving the late response. + assert elapsed < 1.0 From 008c111776763e18ba45331697724bb3713c893d Mon Sep 17 00:00:00 2001 From: Praveen Mittal Date: Wed, 22 Jul 2026 00:51:02 +0200 Subject: [PATCH 2/2] fix: respect explicit falsy credentials and reject whitespace-only media field values Addresses review feedback on this PR: - An explicitly passed empty/falsy credential argument (e.g. public_key="") was silently overridden by a real value from the environment, since `public_key or os.environ.get(...)` falls back on any falsy value, not just an omitted argument. Only fall back to the env var when the argument is exactly None (i.e. not provided), so an explicit falsy value is respected and correctly disables the client instead of being silently replaced. - LangfuseMedia.parse_reference_string checked that required field values were truthy, but a whitespace-only value (e.g. "id= ") is a non-empty string and passed that check, still reaching a real api.media.get() call with a blank id. Now checks that each value is non-blank after stripping. --- langfuse/_client/client.py | 16 ++++++++++++-- langfuse/media.py | 13 ++++++----- tests/unit/test_initialization.py | 36 +++++++++++++++++++++++++++++++ tests/unit/test_media.py | 11 ++++++++++ 4 files changed, 69 insertions(+), 7 deletions(-) diff --git a/langfuse/_client/client.py b/langfuse/_client/client.py index 268da791e..f3eacb4dd 100644 --- a/langfuse/_client/client.py +++ b/langfuse/_client/client.py @@ -377,7 +377,15 @@ def __init__( ) langfuse_logger.setLevel(logging.DEBUG) - public_key = public_key or os.environ.get(LANGFUSE_PUBLIC_KEY) + # Only fall back to the env var when the argument wasn't provided at + # all (None). An explicitly passed falsy value (e.g. public_key="") + # must be respected and rejected by the check below, not silently + # replaced by a real key that happens to be set in the environment. + public_key = ( + public_key + if public_key is not None + else os.environ.get(LANGFUSE_PUBLIC_KEY) + ) if not isinstance(public_key, str) or not public_key.strip(): langfuse_logger.warning( "Authentication error: Langfuse client initialized without public_key. Client will be disabled. " @@ -386,7 +394,11 @@ def __init__( self._otel_tracer = otel_trace_api.NoOpTracer() return - secret_key = secret_key or os.environ.get(LANGFUSE_SECRET_KEY) + secret_key = ( + secret_key + if secret_key is not None + else os.environ.get(LANGFUSE_SECRET_KEY) + ) if not isinstance(secret_key, str) or not secret_key.strip(): langfuse_logger.warning( "Authentication error: Langfuse client initialized without secret_key. Client will be disabled. " diff --git a/langfuse/media.py b/langfuse/media.py index acbdc2417..e19560054 100644 --- a/langfuse/media.py +++ b/langfuse/media.py @@ -258,11 +258,14 @@ def parse_reference_string(reference_string: str) -> ParsedMediaReference: key, value = pair.split("=", 1) parsed_data[key] = value - # Verify all required fields are present and non-empty. An empty value - # (e.g. "id=") would otherwise pass this check and later reach a real - # API call (LangfuseMedia.resolve_media_references -> api.media.get) - # with an empty media_id. - if not all(parsed_data.get(key) for key in ["type", "id", "source"]): + # Verify all required fields are present and non-blank. An empty or + # whitespace-only value (e.g. "id=" or "id= ") would otherwise pass + # this check and later reach a real API call + # (LangfuseMedia.resolve_media_references -> api.media.get) with a + # blank media_id. + if not all( + parsed_data.get(key, "").strip() for key in ["type", "id", "source"] + ): raise ValueError("Missing required fields in reference string") return ParsedMediaReference( diff --git a/tests/unit/test_initialization.py b/tests/unit/test_initialization.py index 89b64a584..9c06bdf22 100644 --- a/tests/unit/test_initialization.py +++ b/tests/unit/test_initialization.py @@ -383,3 +383,39 @@ def test_non_string_public_key_argument_disables_client_without_crashing( assert isinstance(client._otel_tracer, NoOpTracer) assert client._resources is None + + def test_explicit_empty_public_key_argument_is_not_overridden_by_env_var( + self, monkeypatch + ): + # An explicitly passed empty public_key must be respected (and + # disable the client) rather than silently falling back to a real + # key that happens to be set in the environment -- e.g. a shared + # dev machine or CI runner with LANGFUSE_PUBLIC_KEY already exported. + monkeypatch.setenv("LANGFUSE_PUBLIC_KEY", "pk-real-env-key") + + client = Langfuse(public_key="", secret_key="test_sk") + + assert isinstance(client._otel_tracer, NoOpTracer) + assert client._resources is None + + def test_explicit_empty_secret_key_argument_is_not_overridden_by_env_var( + self, monkeypatch + ): + monkeypatch.setenv("LANGFUSE_SECRET_KEY", "sk-real-env-key") + + client = Langfuse(public_key="test_pk", secret_key="") + + assert isinstance(client._otel_tracer, NoOpTracer) + assert client._resources is None + + def test_omitted_public_key_still_falls_back_to_env_var(self, monkeypatch): + # Sanity check for the precedence fix: when the argument is truly + # omitted (not passed at all, so it defaults to None), the env var + # fallback must still work exactly as before. + monkeypatch.setenv("LANGFUSE_PUBLIC_KEY", "pk-from-env") + monkeypatch.setenv("LANGFUSE_SECRET_KEY", "sk-from-env") + + client = Langfuse() + + assert not isinstance(client._otel_tracer, NoOpTracer) + assert client._resources is not None diff --git a/tests/unit/test_media.py b/tests/unit/test_media.py index 454a21804..551710686 100644 --- a/tests/unit/test_media.py +++ b/tests/unit/test_media.py @@ -100,6 +100,17 @@ def test_parse_reference_string_with_empty_field_values_raises_missing_fields_er LangfuseMedia.parse_reference_string("@@@langfuseMedia:type=|id=|source=@@@") +def test_parse_reference_string_with_whitespace_only_field_value_raises_missing_fields_error(): + # A whitespace-only value (e.g. "id= ") is non-empty as a raw string, + # so it would pass a plain truthiness check on the parsed value, and the + # resulting whitespace-only media_id would still reach a real + # api.media.get() call in LangfuseMedia.resolve_media_references. + with pytest.raises(ValueError, match="Missing required fields"): + LangfuseMedia.parse_reference_string( + "@@@langfuseMedia:type=image/jpeg|id= |source=bytes@@@" + ) + + def test_parse_reference_string_with_malformed_pair_raises_missing_fields_error(): # A pipe-separated segment with no "=" (here, a typo dropping the "=" # from "id=") used to crash with a raw "not enough values to unpack"