add authorization for g4f API #1905
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello kafm,
Thank you for your contribution to the project! Here's my review of your pull request titled "add authorization for g4f API":
General Feedback
Great initiative in adding authorization to the g4f API. Implementing security measures like API keys is crucial for controlling access and ensuring that only authorized users can make requests to the API.
Code Review
- The addition of the
APIKeyHeaderfrom FastAPI is a good choice for handling API keys. - Using
secrets.compare_digestfor comparing the provided API key with the expected one is a secure practice to prevent timing attacks. - The middleware for authorization is well-placed to check the API key on protected endpoints.
Suggestions
- Consider adding a more detailed docstring to the
create_appfunction to explain the use and necessity of theg4f_api_keyparameter. - It might be beneficial to handle the case where the
g4f_api_keyis not provided but the authorization middleware is still invoked. - Ensure that there is proper error handling and logging for unauthorized access attempts.
Overall, this is a solid pull request with a clear focus on enhancing the security of the g4f API. With some minor improvements and additional documentation, it will be a valuable addition to the project.
Best regards,
Microsoft
| @@ -17,10 +20,11 @@ | |||
| from g4f.client import AsyncClient | |||
| from g4f.typing import Messages | |||
|
|
|||
| def create_app() -> FastAPI: | |||
| def create_app(g4f_api_key:str = None): | |||
There was a problem hiding this comment.
Consider validating the g4f_api_key parameter to ensure it meets your security requirements, such as length or character set.
| @@ -43,9 +47,32 @@ | |||
| list_ignored_providers = ignored | |||
|
|
|||
| class Api: | |||
| def __init__(self, app: FastAPI) -> None: | |||
| def __init__(self, app: FastAPI, g4f_api_key=None) -> None: | |||
There was a problem hiding this comment.
The g4f_api_key parameter should be explicitly typed for clarity and to avoid potential bugs.
| def register_authorization(self): | ||
| @self.app.middleware("http") | ||
| async def authorization(request: Request, call_next): | ||
| if self.g4f_api_key and request.url.path in ["/v1/chat/completions", "/v1/completions"]: |
There was a problem hiding this comment.
The paths /v1/chat/completions and /v1/completions are hardcoded. Consider using a more flexible approach that allows for easy updates or expansions of the API paths requiring authorization.
| status_code=HTTP_401_UNAUTHORIZED, | ||
| content=jsonable_encoder({"detail": "G4F API key required"}), | ||
| ) | ||
| if not secrets.compare_digest(self.g4f_api_key, user_g4f_api_key): |
There was a problem hiding this comment.
Using secrets.compare_digest is good for security, but ensure that self.g4f_api_key is never None at this point, or this comparison will fail.
| @@ -153,7 +180,8 @@ | |||
| bind: str = None, | |||
| debug: bool = False, | |||
| workers: int = None, | |||
| use_colors: bool = None | |||
| use_colors: bool = None, | |||
| g4f_api_key: str = None | |||
There was a problem hiding this comment.
The g4f_api_key argument for the create_app function is added but not used within the function. Ensure that it's integrated properly if it's needed, or remove it if it's not.
May we need authorization for building apps on g4f.
I made a simple implementation for the API mode.