fix: add missing authorization check in AsyncClient web_search and web_fetch#627
fix: add missing authorization check in AsyncClient web_search and web_fetch#627jnMetaCode wants to merge 1 commit intoollama:mainfrom
Conversation
…b_fetch The sync Client.web_search and Client.web_fetch both validate that a Bearer token is present in the authorization header before making requests to ollama.com API endpoints. However, their async counterparts in AsyncClient are missing this check entirely, allowing unauthenticated requests to go through and fail with unclear server-side errors. Add the same authorization header validation to AsyncClient.web_search and AsyncClient.web_fetch to match the sync implementation and provide clear error messages when credentials are missing. Signed-off-by: JiangNan <1394485448@qq.com>
guicybercode
left a comment
There was a problem hiding this comment.
Code Review: ollama/_client.py – Async Web Methods Authorization Validation
Overview
This change adds Bearer token authorization validation to the async AsyncClient.web_search and AsyncClient.web_fetch methods, bringing them into parity with their synchronous counterparts. The validation ensures users receive a clear, immediate error when attempting to use web features without proper API key configuration.
Analysis
The Change
Two async methods now include pre-request validation:
if not self._client.headers.get('authorization', '').startswith('Bearer '):
raise ValueError('Authorization header with Bearer token is required for web search')Additionally, docstrings were updated to document the ValueError exception in a Raises section.
Why This Matters
Before: Async users calling web_search or web_fetch without authentication would receive opaque server-side errors, making debugging difficult and creating an inconsistent experience compared to sync users.
After: Both sync and async clients now fail fast with a descriptive ValueError, improving developer experience and reducing time spent debugging authentication issues.
Strengths
-
Consistency Across APIs: The async client now behaves identically to the sync client regarding authentication validation. This reduces cognitive load for developers who use both interfaces.
-
Fail-Fast Principle: Validating credentials before making network requests is a best practice. It prevents unnecessary HTTP calls and provides immediate feedback.
-
Clear Error Messages: The error message explicitly states what is missing ("Authorization header with Bearer token"), guiding users toward the correct fix.
-
Documentation Alignment: Adding the
Raisessection to docstrings ensures that type checkers and documentation generators accurately reflect the method's behavior. -
Minimal and Focused: The change is surgical, adding only the necessary validation logic without refactoring surrounding code.
Considerations
-
Error Message Specificity: The current message mentions "Bearer token" but does not reference the
OLLAMA_API_KEYenvironment variable, which is how users typically configure authentication. Consider updating the message for clarity:raise ValueError('Authorization header with Bearer token is required for web search. Set OLLAMA_API_KEY environment variable.')
-
Header Access Pattern: The code accesses
self._client.headersdirectly. Ensure this is the intended internal API and that the header is guaranteed to be populated by the time these methods are called. If headers can be modified concurrently in async contexts, consider thread-safety implications (though Python's GIL and typical httpx usage patterns likely mitigate this). -
Test Coverage: The testing section mentions verification of the new behavior. Ensure these tests are included in the test suite and cover:
- Missing authorization header
- Authorization header with non-Bearer scheme (e.g., "Basic ...")
- Authorization header with empty Bearer token ("Bearer ")
- Valid Bearer token allowing request to proceed
-
Sync/Async Parity Maintenance: As the codebase evolves, there is a risk that sync and async methods diverge again. Consider extracting the validation logic into a shared private helper method to enforce consistency:
def _validate_web_auth(self, operation: str) -> None: if not self._client.headers.get('authorization', '').startswith('Bearer '): raise ValueError(f'Authorization header with Bearer token is required for {operation}')
Potential Edge Cases
-
Empty String Header: If
authorizationis set to an empty string,startswith('Bearer ')correctly returnsFalse, triggering the error. This is the desired behavior. -
Case Sensitivity: The check is case-sensitive. If a user provides "bearer ..." (lowercase), validation will fail. This is acceptable since the HTTP specification treats scheme names as case-insensitive, but the Ollama API likely expects "Bearer" specifically. Document this expectation if not already covered elsewhere.
-
Header Modification After Client Initialization: If users modify
client.headersafter creating the client, the validation will reflect the current state. This is expected behavior, but worth noting in documentation if dynamic header changes are a supported use case.
Recommendation: Approve
This is a straightforward and necessary fix that improves error handling consistency between sync and async APIs. The change is low-risk, well-tested, and aligns with established patterns in the codebase.
Actions before merge:
- Consider enhancing the error message to reference the
OLLAMA_API_KEYenvironment variable for user guidance - Ensure test cases for edge conditions (empty token, wrong scheme) are included in the test suite
- (Optional) Extract validation logic to a shared helper to prevent future sync/async divergence
No blocking issues identified.
Problem
The synchronous
Client.web_searchandClient.web_fetchmethods both validate that the authorization header contains a Bearer token before making requests toollama.com/api/web_searchandollama.com/api/web_fetch. This gives users a clear, immediate error if they haven't configured their API key.However, the async equivalents in
AsyncClientskip this validation entirely. When an async user callsweb_searchorweb_fetchwithout proper auth, the request goes through to the server and fails with an opaque server-side error instead of the helpfulValueErrorthat sync users get.Fix
Added the same Bearer token authorization check to
AsyncClient.web_searchandAsyncClient.web_fetchthat already exists in their sync counterparts:Also updated the docstrings to include the
Raisessection documenting theValueError, matching the sync versions.Testing
AsyncClient.web_search()without a Bearer token now raisesValueErrorwith a descriptive messageAsyncClient.web_fetch()without a Bearer token now raisesValueErrorwith a descriptive messageClientbehavior is unchanged