fix(team): let one integration cover both callback events

The family rule compared variable names only, so a team admin who registered
an integration for the success event could not register the same integration,
with the same values, for the failure event.

Compare the values as well: repeating what the owning entry already stores
flattens to the same dict, so there is nothing to redirect. The stored side is
decrypted first, because the credentials are encrypted at rest and ciphertext
never equals the plaintext coming in.
This commit is contained in:
Yucheng Zhu 2026-09-01 14:22:32 -07:00
parent 75b717ba73
commit bf29ecf115
3 changed files with 32 additions and 11 deletions

View file

@ -77,7 +77,7 @@ def cross_entry_family_error(
callback_vars: Mapping[str, str] | None,
stored_vars_by_entry: Sequence[Mapping[str, str]],
) -> str | None:
"""Reject an entry that joins a credential family another entry already holds.
"""Reject an entry that changes a credential family another entry already holds.
Every stored entry's variables are flattened into one dict before a request
reads them, and the flattened dict is what the exporter authenticates and
@ -86,20 +86,31 @@ def cross_entry_family_error(
with the key from the first, and the request carries that key to the new
host.
Requiring one entry to own a family end to end removes the pairing. Only the
writers this endpoint newly admits are held to it, because a proxy admin
already holds every credential the proxy has. A team admin who does want to
move a family deletes the entry holding it first, which reveals nothing.
Requiring one entry to own a family end to end removes the pairing. Repeating
a value the holding entry already stores is allowed, because flattening then
produces the same dict either way -- that is how the same integration gets
registered for both the success and the failure event. A team admin who does
want to move a family deletes the entry holding it first, which reveals
nothing.
Only the writers this endpoint newly admits are held to this, because a proxy
admin already holds every credential the proxy has.
``stored_vars_by_entry`` has to arrive decrypted; the credential values are
encrypted at rest and ciphertext never equals the plaintext coming in.
"""
if not callback_vars:
return None
held: Final = {family for entry in stored_vars_by_entry for family in map(_family_of, entry) if family is not None}
stored: Final = { # mutable-ok: local view of the stored entries, never stored
var: value for entry in stored_vars_by_entry for var, value in entry.items() if _family_of(var) is not None
}
held: Final = {_family_of(var) for var in stored} # mutable-ok: local index, never stored
return next(
(
f"{family} is already configured by another callback entry on this team. "
f"Remove that entry before setting {var} here."
for var, family in ((v, _family_of(v)) for v in callback_vars)
if family is not None and family in held
for var, value, family in ((v, callback_vars[v], _family_of(v)) for v in callback_vars)
if family in held and stored.get(var) != value
),
None,
)

View file

@ -347,11 +347,16 @@ async def add_team_callbacks(
# flattened into one dict before a request reads them, so an entry
# naming only a destination would pair with a key written on another
# entry and carry it to that destination -- a key a team admin can read
# back nowhere. Proxy admins are exempt: they already hold every
# credential the proxy has.
# back nowhere. Repeating a value the owning entry already stores is
# fine, which is how one integration covers both events. Proxy admins
# are exempt: they already hold every credential the proxy has.
if user_api_key_dict.user_role != LitellmUserRoles.PROXY_ADMIN:
# Decrypted, because the check compares the incoming values against
# the stored ones and the credentials are encrypted at rest.
decrypted_logging: Final = decrypt_callback_vars(team_metadata).get("logging")
stored_entries: Final = decrypted_logging if isinstance(decrypted_logging, list) else ()
stored_entry_vars: Final = [ # mutable-ok: read-only input to the check, never stored
entry.get("callback_vars") or {} for entry in team_callback_settings
entry.get("callback_vars") or {} for entry in stored_entries
]
family_error: Final = cross_entry_family_error(data.callback_vars, stored_entry_vars)
if family_error is not None:

View file

@ -1538,6 +1538,11 @@ async def test_proxy_admin_still_told_the_team_is_unknown():
({"langsmith_api_key": "k"}, [{"dd_api_key": "k"}], False),
# variables that configure no backend carry nothing to redirect
({"turn_off_message_logging": "true"}, [{"langfuse_secret_key": "sk"}], False),
# the same integration registered for a second event: identical values
# flatten to the identical dict, so there is nothing to redirect
({"langfuse_host": "https://us.cloud.langfuse.com", "langfuse_public_key": "pk", "langfuse_secret_key": "sk"}, [{"langfuse_host": "https://us.cloud.langfuse.com", "langfuse_public_key": "pk", "langfuse_secret_key": "sk"}], False),
# the same shape with one value moved is the redirect again
({"langfuse_host": "http://attacker.invalid", "langfuse_public_key": "pk", "langfuse_secret_key": "sk"}, [{"langfuse_host": "https://us.cloud.langfuse.com", "langfuse_public_key": "pk", "langfuse_secret_key": "sk"}], True),
],
)
def test_one_entry_owns_a_credential_family(new_vars, stored, rejected):