fix: reject empty/whitespace/non-string credentials and empty media reference fields#1772
Open
mittalpk wants to merge 2 commits into
Open
fix: reject empty/whitespace/non-string credentials and empty media reference fields#1772mittalpk wants to merge 2 commits into
mittalpk wants to merge 2 commits into
Conversation
…eference 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.
mittalpk
force-pushed
the
fix/credential-validation-and-media-parsing
branch
from
July 21, 2026 22:46
35b9e72 to
58b8b83
Compare
| 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. " |
Contributor
There was a problem hiding this comment.
Falsey Arguments Use Environment Credentials
When an explicit credential is empty or another falsey value, argument or env replaces 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 for secret_key.
Suggested change
| "Provide a public_key parameter or set LANGFUSE_PUBLIC_KEY environment variable. " | |
| if public_key is None: | |
| public_key = os.environ.get(LANGFUSE_PUBLIC_KEY) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: langfuse/_client/client.py
Line: 384
Comment:
**Falsey Arguments Use Environment Credentials**
When an explicit credential is empty or another falsey value, `argument or env` replaces 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 for `secret_key`.
```suggestion
if public_key is None:
public_key = os.environ.get(LANGFUSE_PUBLIC_KEY)
```
How can I resolve this? If you propose a fix, please make it concise.| # (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.
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.
Suggested change
| if not all(parsed_data.get(key) for key in ["type", "id", "source"]): | |
| if not all( | |
| isinstance(parsed_data.get(key), str) and parsed_data[key].strip() | |
| for key in ["type", "id", "source"] | |
| ): |
Prompt To Fix With AI
This 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.…dia 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #15280
What does this PR do?
Two input-validation gaps found during an SDK audit, plus new test coverage for a previously-untested part of the HTTP layer:
1. Empty/whitespace/non-string credentials bypassed the auth-disable guard
Langfuse.__init__checkedpublic_key is None/secret_key is Noneto decide whether to disable the client. An empty-string or whitespace-only env var (e.g.LANGFUSE_PUBLIC_KEY=""— a real scenario with templated Docker/k8s env files) bypassed this check entirely, since""is notNone. The client then proceeded to construct a fully-wired tracer instead of disabling itself, failing only later with an opaque auth error on the first real request.Fixed by checking
isinstance(x, str)andx.strip()instead ofx is None— this also closes a non-string-input crash (e.g.public_key=123previously proceeded silently; a naive.strip()-only fix would have crashed withAttributeErroron this case, so theisinstancecheck handles all three: missing, blank, and wrong-type).2.
LangfuseMedia.parse_reference_stringaccepted malformed/empty input=(a typo, or a trailing|) crashed with a rawValueError: not enough values to unpackinstead of the method's own documentedValueError. Fixed by skipping segments without=during parsing.type,id,source) being present was checked, but not that their values were non-empty —type=|id=|source=passed validation and produced an emptymedia_id, which flows directly into a realapi.media.get()call inresolve_media_references. Fixed by checking value truthiness, not just key presence.Also included
LangfuseClient(langfuse/_utils/request.py) against a real local HTTP server viapytest-httpserver— previously a declared but entirely unused dev dependency. Covers malformed JSON response bodies (200/207/generic error codes) and a slow/hung dependency actually enforcing the configured timeout end-to-end (verified bounded well under the dependency's simulated delay, not just that a timeout argument is accepted).CONTRIBUTING.md: documented how to start a local Langfuse server for e2e tests (mirroring the exact commandsci.ymlalready runs), since this was previously undocumented despite being required. Includes teardown instructions.README.md: added a pointer toCONTRIBUTING.md.Type of change
Verification
All clean. Full unit suite: 676 passed, 3 skipped, 0 regressions (baseline 657 before this work). Every new test confirmed to fail against the pre-fix code before the corresponding fix was applied (verified via
git stashon each fix in isolation), anduv build --no-sourcesconfirmed the package still builds cleanly with these changes.Checklist
code_review.md..env.templateif needed.Greptile Summary
This PR tightens credential and media-reference validation and expands test and contributor guidance. The main changes are:
Confidence Score: 4/5
Credential precedence and whitespace-only media values need fixes before merging.
langfuse/_client/client.py and langfuse/media.py
Security Review
Explicit falsey credentials can be replaced by populated environment credentials before validation, enabling the client with credentials the caller did not select.
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix: reject empty/whitespace/non-string ..." | Re-trigger Greptile
Context used:
Learned From
langfuse/langfuse-python#1387