-
Notifications
You must be signed in to change notification settings - Fork 316
fix: reject empty/whitespace/non-string credentials and empty media reference fields #1772
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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"]): | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
A required value containing only whitespace is truthy, so a reference such as
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: langfuse/media.py
Line: 265
Comment:
**Whitespace Media Fields Remain Valid**
A required value containing only whitespace is truthy, so a reference such as `id= ` still passes validation. `resolve_media_references` then sends the whitespace ID to `api.media.get`, and the media reference remains unresolved after that request fails.
```suggestion
if not all(
isinstance(parsed_data.get(key), str) and parsed_data[key].strip()
for key in ["type", "id", "source"]
):
```
How can I resolve this? If you propose a fix, please make it concise.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Already fixed in 008c111 (this PR's current HEAD) — the required-field check now strips before validating ( |
||||||||||||
| raise ValueError("Missing required fields in reference string") | ||||||||||||
|
|
||||||||||||
| return ParsedMediaReference( | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When an explicit credential is empty or another falsey value,
argument or envreplaces it before validation. If the matching environment variable is populated, the client becomes enabled with those unintended credentials instead of rejecting the explicit input; the same behavior occurs forsecret_key.Prompt To Fix With AI
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already fixed in 008c111 (this PR's current HEAD) —
public_key/secret_keynow only fall back to the env var when the argument is exactlyNone, so an explicit falsy value is respected and correctly disables the client instead of being silently replaced. Matches your suggested diff.