-
-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
_ #92
Conversation
Merge pull request #90 from TeamKillerX/dev
Reviewer's Guide by SourceryThis pull request introduces several changes to improve security, add new features, and update documentation. The main changes include adding Nginx configuration for sensitive file protection, implementing API key support for various endpoints, updating the chat_hacked function, and adding new API endpoint documentation. File-Level Changes
Tips
|
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @xtsea - I've reviewed your changes - here's some feedback:
Overall Comments:
- The changes to include API keys in URLs (e.g.,
url = f"{base_api_dev}/ryuzaki/blackbox?api_key={api_key}"
) pose a security risk. Consider using headers or a more secure method for API authentication instead of query parameters.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -37,9 +37,9 @@ def _make_request(method: str, url: str, params: dict = None, json_data: dict = | |||
return None | |||
|
|||
@staticmethod | |||
def ban(user_id: int = None, reason: str = None, is_working_dev=False) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 suggestion (security): API key in URL and default value change for is_working_dev
Similar to the changes in chatgpt.py, consider moving the api_key to the request headers instead of the URL for better security. Also, note that the default value of is_working_dev has changed from False to True, which may affect the default behavior of this method.
RyuzakiLib/hackertools/downloader.py
Outdated
@@ -10,6 +11,16 @@ def _ok(self, use_name=None, link=None): | |||
return {"link": link} | |||
return {} | |||
|
|||
async def with_download(self, open_files=None, response_url=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (performance): Use async HTTP client in async function
The synchronous requests.get() call in an async function can block the event loop. Consider using an asynchronous HTTP client like httpx.AsyncClient for better performance in asynchronous contexts.
import httpx
async def with_download(self, open_files=None, response_url=None):
async with httpx.AsyncClient() as client:
response = await client.get(response_url)
content = response.content
RyuzakiLib/hackertools/downloader.py
Outdated
finally: | ||
os.remove(open_files) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): Potential issue with file removal in finally block
The os.remove(open_files) in the finally block could attempt to delete a file that wasn't created if an exception occurred earlier. Consider moving this to the try block or adding a check to ensure the file exists before attempting to remove it.
docs/nginx-fastapi.md
Outdated
@@ -163,3 +163,71 @@ Now, you can access your FastAPI app via your domain at: | |||
|
|||
### Conclusion | |||
With these steps, you'll have FastAPI running on your VPS with your domain, and optionally secured with HTTPS via Let's Encrypt. You can scale this setup by using Docker, Gunicorn with Uvicorn workers, and more advanced deployment techniques if necessary. | |||
|
|||
### Solution: Increase Nginx Header Buffer Sizes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (documentation): Consider adding a brief explanation for increasing buffer sizes.
This would help users understand the purpose of these changes and when they might be necessary.
### Solution: Increase Nginx Header Buffer Sizes | |
### Solution: Increase Nginx Header Buffer Sizes | |
When dealing with large headers or complex requests, you may encounter "414 Request-URI Too Large" errors. To resolve this: | |
### Solution: Increase Nginx Header Buffer Sizes for Large Requests |
Merge pull request #92 from TeamKillerX/dev
Summary by Sourcery
Enhance the security and functionality of the application by adding API key support to various functions, increasing Nginx header buffer sizes, and updating documentation with new guides on securing sensitive files and using API endpoints.
New Features:
Enhancements:
Documentation: