Skip to content

fix: reject empty/whitespace/non-string credentials and empty media reference fields - #1772

Open
mittalpk wants to merge 2 commits into
langfuse:mainfrom
mittalpk:fix/credential-validation-and-media-parsing
Open

fix: reject empty/whitespace/non-string credentials and empty media reference fields#1772
mittalpk wants to merge 2 commits into
langfuse:mainfrom
mittalpk:fix/credential-validation-and-media-parsing

Conversation

@mittalpk

@mittalpkmittalpk commented Jul 21, 2026

Copy link
Copy Markdown

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__ checked public_key is None / secret_key is None to 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 not None. 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) and x.strip() instead of x is None — this also closes a non-string-input crash (e.g. public_key=123 previously proceeded silently; a naive .strip()-only fix would have crashed with AttributeError on this case, so the isinstance check handles all three: missing, blank, and wrong-type).

2. LangfuseMedia.parse_reference_string accepted malformed/empty input

  • A pipe-separated segment without = (a typo, or a trailing |) crashed with a raw ValueError: not enough values to unpack instead of the method's own documented ValueError. Fixed by skipping segments without = during parsing.
  • Required keys (type, id, source) being present was checked, but not that their values were non-empty — type=|id=|source= passed validation and produced an empty media_id, which flows directly into a real api.media.get() call in resolve_media_references. Fixed by checking value truthiness, not just key presence.

Also included

  • New unit tests for LangfuseClient (langfuse/_utils/request.py) against a real local HTTP server via pytest-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 commands ci.yml already runs), since this was previously undocumented despite being required. Includes teardown instructions.
  • README.md: added a pointer to CONTRIBUTING.md.

Type of change

  • Bug fix
  • Documentation update

Verification

uv run --frozen ruff check .
uv run --frozen ruff format --check .
uv run --frozen mypy langfuse --no-error-summary
uv run --frozen pytest -n auto --dist worksteal tests/unit

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 stash on each fix in isolation), and uv build --no-sources confirmed the package still builds cleanly with these changes.

Checklist

  • I self-reviewed the diff using code_review.md.
  • I added or updated tests for behavior changes.
  • I updated docs, examples, or .env.template if needed.
  • I did not hand-edit generated files; if generated files changed, I used the upstream regeneration path.
  • I did not commit secrets or credentials.

Greptile Summary

This PR tightens credential and media-reference validation and expands test and contributor guidance. The main changes are:

  • Reject blank and non-string client credentials.
  • Validate required media-reference values and tolerate malformed segments.
  • Export a dedicated authentication-check exception.
  • Add HTTP failure and timeout tests.
  • Document local end-to-end test setup.

Confidence Score: 4/5

Credential precedence and whitespace-only media values need fixes before merging.

  • Explicit falsey credentials can silently fall back to environment credentials.
  • Whitespace-only media IDs still reach the media API.
  • The new exception export and surrounding error handling remain consistent.

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
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---### Issue 1 of 2
langfuse/_client/client.py:384
**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)```### Issue 2 of 2
langfuse/media.py:265
**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"] ):```

Reviews (1): Last reviewed commit: "fix: reject empty/whitespace/non-string ..." | Re-trigger Greptile

Greptile also left 2 inline comments on this PR.

Context used:

  • Rule used - Move imports to the top of the module instead of p... (source)

Learned From
langfuse/langfuse-python#1387

@claudeclaudeBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

…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
mittalpkforce-pushed the fix/credential-validation-and-media-parsing branch from 35b9e72 to 58b8b83CompareJuly 21, 2026 22:46
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. "

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1securityFalsey 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. "
ifpublic_keyisNone:
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.

@mittalpkmittalpkJul 26, 2026

Copy link
Copy Markdown
Author

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_key now only fall back to the env var when the argument is exactly None, so an explicit falsy value is respected and correctly disables the client instead of being silently replaced. Matches your suggested diff.

Comment threadlangfuse/media.py Outdated
# (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"]):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1Whitespace 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
ifnotall(parsed_data.get(key) forkeyin ["type", "id", "source"]):
ifnotall(
isinstance(parsed_data.get(key), str) andparsed_data[key].strip()
forkeyin ["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.

Copy link
Copy Markdown
Author

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) — the required-field check now strips before validating (parsed_data.get(key, "").strip()), so a whitespace-only value like id= is correctly rejected. Matches your suggested diff.

…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.
@mittalpk

Copy link
Copy Markdown
Author

Bumping this one too — happy to rebase or adjust scope if that helps it move.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@mittalpk