fix(server): forward model auth header and preserve precedence#1883
fix(server): forward model auth header and preserve precedence#1883Pouyanpi wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThe PR updates authentication header handling in ChangesAuth Header Forwarding and Precedence
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nemoguardrails/server/schemas/utils.py`:
- Around line 152-154: The code uses forwarded.removeprefix("Bearer ") which is
case-sensitive and will not strip "bearer " or other case variants; update the
logic around raw_key to perform a case-insensitive prefix check on forwarded
(e.g., check forwarded.lower().startswith("bearer ")) and if true slice off the
original-length prefix from forwarded to produce raw_key (so token casing is
preserved), otherwise just strip forwarded; then continue using raw_key when
setting headers[auth_header_name] with the existing use_bearer handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c3a77fba-a65b-4934-acf2-ba5cc871ec77
📒 Files selected for processing (3)
nemoguardrails/server/schemas/utils.pytests/server/test_api.pytests/server/test_schema_utils.py
| raw_key = forwarded.removeprefix("Bearer ").strip() | ||
| if raw_key: | ||
| headers[auth_header_name] = f"Bearer {raw_key}" if use_bearer else raw_key |
There was a problem hiding this comment.
Handle case-insensitive bearer prefix normalization.
At Line 152, removeprefix("Bearer ") is case-sensitive. A valid header like authorization: bearer user-token won’t be stripped and can become an invalid provider key (e.g., x-api-key: bearer user-token).
Suggested fix
- raw_key = forwarded.removeprefix("Bearer ").strip()
+ forwarded_value = forwarded.strip()
+ if forwarded_value[:7].lower() == "bearer ":
+ raw_key = forwarded_value[7:].strip()
+ else:
+ raw_key = forwarded_value
if raw_key:
headers[auth_header_name] = f"Bearer {raw_key}" if use_bearer else raw_key📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| raw_key = forwarded.removeprefix("Bearer ").strip() | |
| if raw_key: | |
| headers[auth_header_name] = f"Bearer {raw_key}" if use_bearer else raw_key | |
| forwarded_value = forwarded.strip() | |
| if forwarded_value[:7].lower() == "bearer ": | |
| raw_key = forwarded_value[7:].strip() | |
| else: | |
| raw_key = forwarded_value | |
| if raw_key: | |
| headers[auth_header_name] = f"Bearer {raw_key}" if use_bearer else raw_key |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nemoguardrails/server/schemas/utils.py` around lines 152 - 154, The code uses
forwarded.removeprefix("Bearer ") which is case-sensitive and will not strip
"bearer " or other case variants; update the logic around raw_key to perform a
case-insensitive prefix check on forwarded (e.g., check
forwarded.lower().startswith("bearer ")) and if true slice off the
original-length prefix from forwarded to produce raw_key (so token casing is
preserved), otherwise just strip forwarded; then continue using raw_key when
setting headers[auth_header_name] with the existing use_bearer handling.
Greptile SummaryThis PR fixes auth header precedence in
|
| Filename | Overview |
|---|---|
| nemoguardrails/server/schemas/utils.py | Adds case-insensitive Authorization header lookup and restructures auth precedence so env-configured API keys always win over forwarded request headers; logic is sound for all configured providers. |
| tests/server/test_api.py | Adds OPENAI_API_KEY empty string to the existing forwarding test to prevent env-leak interference, and adds a new integration test verifying env-key precedence; coverage is appropriate. |
| tests/server/test_schema_utils.py | Adds three new unit tests for env-key precedence, forwarded-auth for OpenAI, and non-OpenAI provider header translation; each test isolates the relevant env var, though the Anthropic test does not clear MAIN_MODEL_BASE_URL. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[fetch_models called] --> B{env API key set?}
B -- Yes --> C[Use env key with provider auth header]
B -- No --> D{Forwarded Authorization header present?}
D -- No --> E[Send request without auth headers]
D -- Yes --> F{header name is Authorization AND use_bearer?}
F -- Yes --> G[Forward header value as-is]
F -- No --> H[Strip Bearer prefix from forwarded value]
H --> I{Stripped value non-empty?}
I -- Yes --> J[Set provider auth header with or without Bearer prefix]
I -- No --> E
C --> K[Apply extra_headers and send GET to provider URL]
G --> K
J --> K
E --> K
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
nemoguardrails/server/schemas/utils.py:148-154
The shortcut branch for the OpenAI-compatible case only triggers when both `auth_header_name == "Authorization"` **and** `use_bearer` are true, so the forwarded value is passed through verbatim. If a client sends a non-Bearer scheme (e.g. `Authorization: ApiKey xyz`), that value lands in `headers["Authorization"]` unmodified and the upstream provider will likely reject the request with a 401. Stripping and re-wrapping with `Bearer` (as the `else` branch already does) would make the forwarding consistent with the env-key path.
```suggestion
elif forwarded:
raw_key = forwarded.removeprefix("Bearer ").strip()
if raw_key:
headers[auth_header_name] = f"Bearer {raw_key}" if use_bearer else raw_key
```
### Issue 2 of 2
tests/server/test_schema_utils.py:481
The Anthropic provider constructs its URL using `MAIN_MODEL_BASE_URL` when that variable is set. If a test environment (or a previous test) leaves `MAIN_MODEL_BASE_URL` set, this test would hit a different URL than `https://api.anthropic.com/v1/models`. Since `httpx.AsyncClient` is fully mocked, the URL doesn't affect the assertion, but explicitly clearing the variable documents the intent and guards against subtle breakage if the mock scope ever narrows.
```suggestion
with patch.dict(os.environ, {"ANTHROPIC_API_KEY": "", "MAIN_MODEL_BASE_URL": ""}):
```
Reviews (1): Last reviewed commit: "Preserve model auth precedence" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| elif forwarded: | ||
| if auth_header_name == "Authorization" and use_bearer: | ||
| headers[auth_header_name] = forwarded | ||
| else: | ||
| raw_key = forwarded.removeprefix("Bearer ").strip() | ||
| if raw_key: | ||
| headers[auth_header_name] = f"Bearer {raw_key}" if use_bearer else raw_key |
There was a problem hiding this comment.
auth_header_name == "Authorization" and use_bearer are true, so the forwarded value is passed through verbatim. If a client sends a non-Bearer scheme (e.g. Authorization: ApiKey xyz), that value lands in headers["Authorization"] unmodified and the upstream provider will likely reject the request with a 401. Stripping and re-wrapping with Bearer (as the else branch already does) would make the forwarding consistent with the env-key path.
| elif forwarded: | |
| if auth_header_name == "Authorization" and use_bearer: | |
| headers[auth_header_name] = forwarded | |
| else: | |
| raw_key = forwarded.removeprefix("Bearer ").strip() | |
| if raw_key: | |
| headers[auth_header_name] = f"Bearer {raw_key}" if use_bearer else raw_key | |
| elif forwarded: | |
| raw_key = forwarded.removeprefix("Bearer ").strip() | |
| if raw_key: | |
| headers[auth_header_name] = f"Bearer {raw_key}" if use_bearer else raw_key |
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemoguardrails/server/schemas/utils.py
Line: 148-154
Comment:
The shortcut branch for the OpenAI-compatible case only triggers when both `auth_header_name == "Authorization"` **and** `use_bearer` are true, so the forwarded value is passed through verbatim. If a client sends a non-Bearer scheme (e.g. `Authorization: ApiKey xyz`), that value lands in `headers["Authorization"]` unmodified and the upstream provider will likely reject the request with a 401. Stripping and re-wrapping with `Bearer` (as the `else` branch already does) would make the forwarding consistent with the env-key path.
```suggestion
elif forwarded:
raw_key = forwarded.removeprefix("Bearer ").strip()
if raw_key:
headers[auth_header_name] = f"Bearer {raw_key}" if use_bearer else raw_key
```
How can I resolve this? If you propose a fix, please make it concise.| async def test_fetch_non_openai_provider_uses_forwarded_auth_without_env_key(): | ||
| """Forwarded auth falls back to provider-specific auth headers when env key is unset.""" | ||
| mock = _mock_httpx({"data": []}) | ||
| with patch.dict(os.environ, {"ANTHROPIC_API_KEY": ""}): |
There was a problem hiding this comment.
MAIN_MODEL_BASE_URL when that variable is set. If a test environment (or a previous test) leaves MAIN_MODEL_BASE_URL set, this test would hit a different URL than https://api.anthropic.com/v1/models. Since httpx.AsyncClient is fully mocked, the URL doesn't affect the assertion, but explicitly clearing the variable documents the intent and guards against subtle breakage if the mock scope ever narrows.
| with patch.dict(os.environ, {"ANTHROPIC_API_KEY": ""}): | |
| with patch.dict(os.environ, {"ANTHROPIC_API_KEY": "", "MAIN_MODEL_BASE_URL": ""}): |
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/server/test_schema_utils.py
Line: 481
Comment:
The Anthropic provider constructs its URL using `MAIN_MODEL_BASE_URL` when that variable is set. If a test environment (or a previous test) leaves `MAIN_MODEL_BASE_URL` set, this test would hit a different URL than `https://api.anthropic.com/v1/models`. Since `httpx.AsyncClient` is fully mocked, the URL doesn't affect the assertion, but explicitly clearing the variable documents the intent and guards against subtle breakage if the mock scope ever narrows.
```suggestion
with patch.dict(os.environ, {"ANTHROPIC_API_KEY": "", "MAIN_MODEL_BASE_URL": ""}):
```
How can I resolve this? If you propose a fix, please make it concise.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
Description [Generated by GH Copilot]
This pull request improves how API keys and authorization headers are handled and prioritized when fetching models, ensuring that environment-configured API keys take precedence over incoming Authorization headers. It also adds comprehensive tests to verify this behavior for both OpenAI-compatible and other providers.
Authentication and header precedence improvements:
fetch_modelsinnemoguardrails/server/schemas/utils.pyto prioritize environment variable API keys over incoming Authorization headers, ensuring that configured keys are always used when present.Testing enhancements:
tests/server/test_api.pyandtests/server/test_schema_utils.pyto verify that:Related Issue(s)
Checklist
Summary by CodeRabbit
Bug Fixes
Tests