add test(tag-routing): prevent header regex bypass for strict plain t… - #26805
Conversation
…ags. Add tests to validate the condition improve the conditional readability by naming the plain-tag check explicitly.
Greptile SummaryThis PR refactors the Confidence Score: 4/5Safe to merge — no behavioral change, just a cosmetic refactor with an added test. Only P2 findings: the PR description overstates a refactor as a bug fix, and the new test is missing the complementary match_any=True assertion. No P0 or P1 issues. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/router_strategy/tag_based_routing.py | Refactors bool(deployment_tags) into a named deployment_has_plain_tags variable for readability; no behavioral change since the two expressions are semantically identical for Optional[List[str]]. |
| tests/test_litellm/router_strategy/test_router_tag_routing.py | Adds a well-scoped unit test verifying regex fallback is blocked under match_any=False; uses no network calls and imports locally — but lacks the complementary match_any=True assertion to fully pin both directions of the condition. |
Reviews (1): Last reviewed commit: "add test(tag-routing): prevent header re..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def test_strict_tag_routing_without_request_tags_blocks_header_regex_fallback(): | ||
| """ | ||
| When tag_filtering_match_any=False, deployments with plain tags must require | ||
| those request tags before header regex can match. A spoofed User-Agent must | ||
| not route to a tagged deployment when the request has no tags. | ||
| """ | ||
| from litellm.router_strategy.tag_based_routing import _match_deployment | ||
|
|
||
| deployment = { | ||
| "model_name": "restricted-model", | ||
| "litellm_params": { | ||
| "model": "gpt-4o", | ||
| "tags": ["internal"], | ||
| "tag_regex": ["^User-Agent: internal-tool"], | ||
| }, | ||
| } | ||
|
|
||
| assert ( | ||
| _match_deployment( | ||
| deployment=deployment, | ||
| request_tags=None, | ||
| header_strings=["User-Agent: internal-tool"], | ||
| match_any=False, | ||
| ) | ||
| is None | ||
| ) |
There was a problem hiding this comment.
match_any=True
The new test only validates that regex is blocked under match_any=False. The symmetric case — that regex fallback still works under match_any=True when request tags are absent — is not covered. Without it, a future accidental tightening of the condition (e.g. removing the not match_any guard) would silently break the permissive path.
Consider adding:
assert (
_match_deployment(
deployment=deployment,
request_tags=None,
header_strings=["User-Agent: internal-tool"],
match_any=True, # permissive mode — regex fallback should be allowed
)
is not None
)| deployment_has_plain_tags = deployment_tags is not None and len(deployment_tags) > 0 | ||
| strict_tag_check_failed = not match_any and deployment_has_plain_tags |
There was a problem hiding this comment.
bool(deployment_tags) evaluates to False for both None and [], and to True for any non-empty list — exactly the same semantics as deployment_tags is not None and len(deployment_tags) > 0 for an Optional[List[str]] value. This is purely a readability refactor, not a functional fix. The PR description frames it as preventing a bypass, but the old code already guarded correctly. The named variable is a nice clarity improvement, but it may be worth aligning the commit message and PR description to accurately reflect this as a refactor + test addition rather than a bug fix, so future readers of the git history have correct context.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…/litellm into litellm_auth_bypass_tag_based_routing
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…_based_routing add test(tag-routing): prevent header regex bypass for strict plain t…
2 participants
Update strict tag check logic so deployments with plain tags cannot fall back to header regex matching when
tag_filtering_match_any=false. Also improve the conditional readability by naming the plain-tag check explicitly.Add tests to validate the condition
improve the conditional readability by naming the plain-tag check explicitly.
Relevant issues
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
tests/test_litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewDelays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
CI (LiteLLM team)
Branch creation CI run
Link:
CI run for the last commit
Link:
Merge / cherry-pick CI run
Links:
Screenshots / Proof of Fix
Type
🆕 New Feature
🐛 Bug Fix
🧹 Refactoring
📖 Documentation
🚄 Infrastructure
✅ Test
Changes