Skip to content

fix: langfuse trace leak key on model params#22188

Merged
yuneng-jiang merged 3 commits intolitellm_internal_dev_03_16_2026from
litellm_langfuse_key_leakage
Mar 17, 2026
Merged

fix: langfuse trace leak key on model params#22188
yuneng-jiang merged 3 commits intolitellm_internal_dev_03_16_2026from
litellm_langfuse_key_leakage

Conversation

@Harshit28j
Copy link
Copy Markdown
Contributor

@Harshit28j Harshit28j commented Feb 26, 2026

Leaked traces removed
image
image

image

Removes logging and callback_settings keys from these 3 dicts:

  • user_api_key_metadata
  • user_api_key_auth_metadata
  • user_api_key_team_metadata
    Removes user_api_key_auth entirely this contained the full auth object with nested metadata.logging that had the Langfuse credentials

@vercel
Copy link
Copy Markdown

vercel bot commented Feb 26, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
litellm Error Error Mar 10, 2026 11:57am

Request Review

@Harshit28j
Copy link
Copy Markdown
Contributor Author

@greptile please review this PR

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Feb 26, 2026

Greptile Summary

This PR fixes a security vulnerability where Langfuse traces were leaking sensitive credentials (Langfuse API keys, authorization headers, etc.) via two vectors: (1) optional_params being logged as modelParameters without filtering, and (2) sensitive keys (logging, callback_settings, user_api_key_auth) surviving into trace metadata.

Key changes:

  • langfuse.py: Both the v1 and v2 logging paths now pass sanitized_model_params (built via ModelParamHelper.get_standard_logging_model_parameters) instead of raw optional_params to Langfuse's generation calls. The old explicit secret_fields pop is removed as the whitelist approach already excludes it.
  • litellm_logging.py: scrub_sensitive_keys_in_metadata is expanded to strip logging and callback_settings from user_api_key_metadata, user_api_key_auth_metadata, and user_api_key_team_metadata, and to completely remove the user_api_key_auth key from metadata (previously it was only skipped during merge but still stored).
  • Mutation concern: The new scrubbing logic mutates the nested metadata dicts in-place via .pop(), whereas the old code created replacement dicts. If the nested dicts are shared references (e.g. originating from a live Pydantic model in the proxy), other code paths may observe the keys as removed.
  • No tests added: The expanded scrubbing logic (the three-dict loop and user_api_key_auth removal) is not covered by any new unit tests, making it difficult to validate correctness or catch regressions.

Confidence Score: 3/5

  • The security fix is directionally correct, but the in-place mutation of shared nested dicts is a subtle regression risk that should be addressed before merging.
  • The whitelist approach for model parameters is sound and well-implemented. However, the mutation of nested metadata dicts in-place (instead of the prior copy-and-replace pattern) introduces a potential aliasing bug where other code holding references to the same nested dicts will silently see keys disappear. Additionally, no unit tests were added for the new scrubbing behavior, meaning regressions would go undetected.
  • litellm/litellm_core_utils/litellm_logging.py — specifically the scrub_sensitive_keys_in_metadata in-place mutation pattern and the missing test coverage.

Important Files Changed

Filename Overview
litellm/litellm_core_utils/litellm_logging.py Expands scrub_sensitive_keys_in_metadata to remove logging and callback_settings from three metadata dicts and drop user_api_key_auth entirely. The new approach mutates nested dicts in-place (unlike the old copy-and-replace pattern), which could introduce aliasing side-effects; no new tests cover the expanded scrubbing behavior.
litellm/integrations/langfuse/langfuse.py Replaces direct use of optional_params as modelParameters / model_parameters with a sanitized whitelist via ModelParamHelper.get_standard_logging_model_parameters. The v1 path (Langfuse <2) and v2 path both now use the whitelist; the old explicit secret_fields pop is removed since the whitelist approach excludes it automatically.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[LiteLLM Request] --> B[scrub_sensitive_keys_in_metadata]
    B --> C{langfuse_masking_function\nin metadata?}
    C -- Yes --> D[Extract to _langfuse_masking_function]
    C -- No --> E
    D --> E[For each: user_api_key_metadata\nuser_api_key_auth_metadata\nuser_api_key_team_metadata]
    E --> F[Pop 'logging' key]
    E --> G[Pop 'callback_settings' key]
    E --> H[Pop 'user_api_key_auth' from metadata]
    F & G & H --> I[litellm_params stored on Logging object]
    I --> J{Langfuse v1 or v2?}
    J -- v1 --> K[get_standard_logging_model_parameters\nfrom optional_params whitelist]
    J -- v2 --> L{standard_logging_object\navailable?}
    L -- Yes --> M[Use standard_logging_object.model_parameters\nalready sanitized]
    L -- No --> N[get_standard_logging_model_parameters\nfrom optional_params whitelist]
    K --> O[trace.generation modelParameters=sanitized]
    M --> P[generation model_parameters=sanitized]
    N --> P
Loading

Last reviewed commit: 9f15a99

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Feb 26, 2026

Additional Comments (2)

litellm/integrations/langfuse/langfuse.py
system_fingerprint will be silently dropped

On line 831, system_fingerprint is added to optional_params. However, system_fingerprint is a response field from OpenAI, not a request parameter — it is not present in CompletionCreateParamsNonStreaming or CompletionCreateParamsStreaming annotations. This means ModelParamHelper.get_standard_logging_model_parameters() will filter it out.

Similarly, standard_logging_object["model_parameters"] is also built via the same ModelParamHelper whitelist (see litellm_logging.py:5379), so it won't contain system_fingerprint either.

Previously, system_fingerprint was always logged to Langfuse in model_parameters. After this change, it will be silently dropped in both the standard_logging_object and fallback code paths. This is a behavioral regression.

Consider either:

  1. Adding system_fingerprint to the sanitized params after filtering, or
  2. Including it in the ModelParamHelper provider-specific params set

litellm/integrations/langfuse/langfuse.py
Type coercion only applied to unsanitized params

This loop coerces non-primitive values to strings (required by Langfuse v1's CreateGeneration). However, in the v2 path, when standard_logging_object is available (lines 840-843), sanitized_model_params comes directly from standard_logging_object["model_parameters"] — which never goes through this coercion. If any model parameter values are non-primitive types (e.g., dicts or lists), Langfuse v2 may handle them differently, but it's worth verifying that this doesn't cause serialization issues in edge cases.

@Harshit28j Harshit28j marked this pull request as ready for review March 10, 2026 11:51
Comment on lines +5503 to +5519
## remove sensitive logging/callback keys from metadata dicts
## these contain credentials (langfuse_secret_key, langfuse_public_key, etc.)
_sensitive_keys = {"logging", "callback_settings"}

for metadata_field in (
"user_api_key_metadata",
"user_api_key_auth_metadata",
"user_api_key_team_metadata",
):
for k, v in metadata["user_api_key_metadata"].items():
if k == "logging": # prevent logging user logging keys
cleaned_user_api_key_metadata[k] = (
"scrubbed_by_litellm_for_sensitive_keys"
)
else:
cleaned_user_api_key_metadata[k] = v
if metadata_field in metadata and isinstance(metadata[metadata_field], dict):
for sensitive_key in _sensitive_keys:
metadata[metadata_field].pop(sensitive_key, None)

metadata["user_api_key_metadata"] = cleaned_user_api_key_metadata
litellm_params["metadata"] = metadata
## remove user_api_key_auth entirely - contains full auth object with nested credentials
metadata.pop("user_api_key_auth", None)

litellm_params["metadata"] = metadata
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.

In-place mutation of shared nested dicts

The new code mutates metadata[metadata_field] in-place using .pop(), while the old code created a fresh cleaned_user_api_key_metadata dict and replaced the reference in metadata. This is a subtle but important difference.

If the nested dict (e.g. metadata["user_api_key_metadata"]) is the same object referenced by the original kwargs["litellm_params"]["metadata"]["user_api_key_metadata"] (since scrub_sensitive_keys_in_metadata receives litellm_params without a deep copy), then mutating it in place will modify the shared object. Any other code that still holds a reference to that nested dict will observe the keys removed.

Consider replacing the .pop() with a dict comprehension (matching the previous approach of creating a new dict) to avoid unexpected aliasing side-effects:

    for metadata_field in (
        "user_api_key_metadata",
        "user_api_key_auth_metadata",
        "user_api_key_team_metadata",
    ):
        if metadata_field in metadata and isinstance(metadata[metadata_field], dict):
            metadata[metadata_field] = {
                k: v
                for k, v in metadata[metadata_field].items()
                if k not in _sensitive_keys
            }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@greptile @greptileai thats the expected desired behaviour we want

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.

Got it. The in-place mutation ensures sensitive keys are removed from all references to these metadata objects, not just this code path. That makes sense for preventing credential leaks.

Comment on lines +5503 to 5521
## remove sensitive logging/callback keys from metadata dicts
## these contain credentials (langfuse_secret_key, langfuse_public_key, etc.)
_sensitive_keys = {"logging", "callback_settings"}

for metadata_field in (
"user_api_key_metadata",
"user_api_key_auth_metadata",
"user_api_key_team_metadata",
):
for k, v in metadata["user_api_key_metadata"].items():
if k == "logging": # prevent logging user logging keys
cleaned_user_api_key_metadata[k] = (
"scrubbed_by_litellm_for_sensitive_keys"
)
else:
cleaned_user_api_key_metadata[k] = v
if metadata_field in metadata and isinstance(metadata[metadata_field], dict):
for sensitive_key in _sensitive_keys:
metadata[metadata_field].pop(sensitive_key, None)

metadata["user_api_key_metadata"] = cleaned_user_api_key_metadata
litellm_params["metadata"] = metadata
## remove user_api_key_auth entirely - contains full auth object with nested credentials
metadata.pop("user_api_key_auth", None)

litellm_params["metadata"] = metadata

return litellm_params
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.

No unit tests for new scrubbing behavior

The PR does not add tests to verify the newly expanded scrubbing logic:

  • That logging and callback_settings are removed from user_api_key_auth_metadata and user_api_key_team_metadata (the old code only scrubbed user_api_key_metadata)
  • That user_api_key_auth is entirely removed from metadata
  • That the keys are completely removed (not replaced with a placeholder like the old "scrubbed_by_litellm_for_sensitive_keys" behavior)

The existing tests in test_langfuse_unit_tests.py only cover the langfuse_masking_function extraction path, leaving the core credential-scrubbing path untested. Adding a test similar to the following would help ensure the fix is solid:

def test_scrub_sensitive_keys_removes_credentials():
    from litellm.litellm_core_utils.litellm_logging import scrub_sensitive_keys_in_metadata

    litellm_params = {
        "metadata": {
            "user_api_key_metadata": {"logging": {"langfuse_secret_key": "sk-..."}, "safe_key": "ok"},
            "user_api_key_auth_metadata": {"callback_settings": {"secret": "..."}, "other": "val"},
            "user_api_key_team_metadata": {"logging": {"api_key": "..."}, "name": "team-a"},
            "user_api_key_auth": {"api_key": "sk-real-key", "metadata": {"logging": {...}}},
        }
    }
    result = scrub_sensitive_keys_in_metadata(litellm_params)
    meta = result["metadata"]
    assert "logging" not in meta["user_api_key_metadata"]
    assert meta["user_api_key_metadata"]["safe_key"] == "ok"
    assert "callback_settings" not in meta["user_api_key_auth_metadata"]
    assert "logging" not in meta["user_api_key_team_metadata"]
    assert "user_api_key_auth" not in meta

Rule Used: What: Ensure that any PR claiming to fix an issue ... (source)

@Harshit28j Harshit28j mentioned this pull request Mar 14, 2026
@yuneng-jiang yuneng-jiang changed the base branch from main to litellm_internal_dev_03_14_2026 March 14, 2026 19:32
@yuneng-jiang yuneng-jiang changed the base branch from litellm_internal_dev_03_14_2026 to litellm_internal_dev_03_16_2026 March 16, 2026 16:14
@yuneng-jiang yuneng-jiang merged commit ad62071 into litellm_internal_dev_03_16_2026 Mar 17, 2026
79 of 98 checks passed
@ishaan-berri ishaan-berri deleted the litellm_langfuse_key_leakage branch March 26, 2026 22:29
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.

2 participants