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

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

@claude claude Bot 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
mittalpk force-pushed the fix/credential-validation-and-media-parsing branch from 35b9e72 to 58b8b83 Compare July 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.

P1 security 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.

Comment thread langfuse/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.

P1 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.
Sign up for free to 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