Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Join our Discord community for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| def get_asset(asset: str, width: int = None, height: int = None): | ||
| if not width and not height: | ||
| try: | ||
| return fastapi.responses.FileResponse(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/{asset}") |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To fix the issue, we need to validate the constructed file path to ensure it remains within the intended assets directory. This can be achieved by:
- Normalizing the constructed path using
os.path.normpathorpathlib.Path.resolve(). - Verifying that the normalized path starts with the intended base directory.
Additionally, we can use a whitelist of allowed filenames if the set of valid assets is known and limited.
The changes will be applied to the get_asset function in src/api.py:
- Normalize the constructed path.
- Check that the normalized path starts with the
assetsdirectory. - Raise an exception or return a 404 response if the validation fails.
| @@ -88,13 +88,19 @@ | ||
| def get_asset(asset: str, width: int = None, height: int = None): | ||
| if not width and not height: | ||
| try: | ||
| return fastapi.responses.FileResponse(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/{asset}") | ||
| except: | ||
| return fastapi.responses.JSONResponse(status_code=404, content={"message": "This asset does not exist."}) | ||
| base_path = pathlib.Path(__file__).parent.parent.resolve() / "assets" | ||
| try: | ||
| # Normalize the path and ensure it is within the base_path | ||
| asset_path = (base_path / asset).resolve() | ||
| if not str(asset_path).startswith(str(base_path)): | ||
| raise ValueError("Invalid asset path") | ||
| if not width and not height: | ||
| return fastapi.responses.FileResponse(asset_path) | ||
| except Exception: | ||
| return fastapi.responses.JSONResponse(status_code=404, content={"message": "This asset does not exist."}) | ||
| else: | ||
| if asset == "logo_no_bg": | ||
| image = Image.open(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/Astroid Logo no bg.png") | ||
| image = Image.open(asset_path) | ||
| new_image = image.resize((width, height)) | ||
| new_image.save(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/resized/Astroid Logo no bg.png") | ||
| return fastapi.responses.FileResponse(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/resized/Astroid Logo no bg{width}x{height}.png") | ||
| resized_path = base_path / "resized" / f"Astroid Logo no bg{width}x{height}.png" | ||
| new_image.save(resized_path) | ||
| return fastapi.responses.FileResponse(resized_path) | ||
| elif asset == "logo": |
| image = Image.open(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/Astroid Logo no bg.png") | ||
| new_image = image.resize((width, height)) | ||
| new_image.save(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/resized/Astroid Logo no bg.png") | ||
| return fastapi.responses.FileResponse(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/resized/Astroid Logo no bg{width}x{height}.png") |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
| image = Image.open(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/Astroid Logo.png") | ||
| new_image = image.resize((width, height)) | ||
| new_image.save(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/resized/Astroid Logo.png") | ||
| return fastapi.responses.FileResponse(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/resized/Astroid Logo{width}x{height}.png") |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To fix the issue, we need to validate and sanitize the width parameter before using it in file path construction. The best approach is to ensure that the width parameter is a positive integer and falls within an acceptable range. Additionally, we should normalize the constructed file path and verify that it resides within a safe root directory.
Steps to implement the fix:
- Validate the
widthparameter to ensure it is a positive integer within a reasonable range (e.g., 1 to 5000). - Normalize the constructed file path using
os.path.normpathorpathlib.Path.resolve()to eliminate any..segments. - Check that the normalized path starts with the expected root directory to prevent directory traversal attacks.
| @@ -95,11 +95,19 @@ | ||
| if asset == "logo_no_bg": | ||
| root_path = pathlib.Path(__file__).parent.parent.resolve() / "assets/resized" | ||
| if not (isinstance(width, int) and 1 <= width <= 5000): | ||
| return fastapi.responses.JSONResponse(status_code=400, content={"message": "Invalid width parameter."}) | ||
| image = Image.open(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/Astroid Logo no bg.png") | ||
| new_image = image.resize((width, height)) | ||
| new_image.save(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/resized/Astroid Logo no bg.png") | ||
| return fastapi.responses.FileResponse(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/resized/Astroid Logo no bg{width}x{height}.png") | ||
| safe_path = root_path / f"Astroid Logo no bg{width}x{height}.png" | ||
| new_image.save(safe_path) | ||
| return fastapi.responses.FileResponse(safe_path.resolve()) | ||
| elif asset == "logo": | ||
| root_path = pathlib.Path(__file__).parent.parent.resolve() / "assets/resized" | ||
| if not (isinstance(width, int) and 1 <= width <= 5000): | ||
| return fastapi.responses.JSONResponse(status_code=400, content={"message": "Invalid width parameter."}) | ||
| image = Image.open(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/Astroid Logo.png") | ||
| new_image = image.resize((width, height)) | ||
| new_image.save(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/resized/Astroid Logo.png") | ||
| return fastapi.responses.FileResponse(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/resized/Astroid Logo{width}x{height}.png") | ||
| safe_path = root_path / f"Astroid Logo{width}x{height}.png" | ||
| new_image.save(safe_path) | ||
| return fastapi.responses.FileResponse(safe_path.resolve()) | ||
| elif asset == "banner": |
| image = Image.open(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/resized/Astroid-banner.png") | ||
| new_image = image.resize((width, height)) | ||
| new_image.save(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/resized/Astroid-banner.png") | ||
| return fastapi.responses.FileResponse(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/resized/Astroid-banner{width}x{height}.png") |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To fix the issue, we need to validate the user-provided width and height parameters to ensure they are within acceptable bounds and are of the correct type. Additionally, we should sanitize the constructed file paths to prevent any unintended behavior.
The best approach is to:
- Validate
widthandheightto ensure they are positive integers within a reasonable range (e.g., between 1 and 5000). - Use
os.path.normpathto normalize the constructed file paths and ensure they remain within the intended directory. - Raise an exception or return an error response if the validation fails.
| @@ -88,5 +88,17 @@ | ||
| def get_asset(asset: str, width: int = None, height: int = None): | ||
| # Validate width and height | ||
| if width is not None and (width <= 0 or width > 5000): | ||
| return fastapi.responses.JSONResponse(status_code=400, content={"message": "Invalid width parameter."}) | ||
| if height is not None and (height <= 0 or height > 5000): | ||
| return fastapi.responses.JSONResponse(status_code=400, content={"message": "Invalid height parameter."}) | ||
|
|
||
| base_path = pathlib.Path(__file__).parent.parent.resolve() / "assets" | ||
| resized_path = base_path / "resized" | ||
|
|
||
| if not width and not height: | ||
| try: | ||
| return fastapi.responses.FileResponse(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/{asset}") | ||
| fullpath = os.path.normpath(base_path / asset) | ||
| if not str(fullpath).startswith(str(base_path)): | ||
| raise Exception("Path traversal detected.") | ||
| return fastapi.responses.FileResponse(fullpath) | ||
| except: | ||
| @@ -94,19 +106,22 @@ | ||
| else: | ||
| if asset == "logo_no_bg": | ||
| image = Image.open(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/Astroid Logo no bg.png") | ||
| new_image = image.resize((width, height)) | ||
| new_image.save(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/resized/Astroid Logo no bg.png") | ||
| return fastapi.responses.FileResponse(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/resized/Astroid Logo no bg{width}x{height}.png") | ||
| elif asset == "logo": | ||
| image = Image.open(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/Astroid Logo.png") | ||
| new_image = image.resize((width, height)) | ||
| new_image.save(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/resized/Astroid Logo.png") | ||
| return fastapi.responses.FileResponse(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/resized/Astroid Logo{width}x{height}.png") | ||
| elif asset == "banner": | ||
| image = Image.open(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/resized/Astroid-banner.png") | ||
| new_image = image.resize((width, height)) | ||
| new_image.save(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/resized/Astroid-banner.png") | ||
| return fastapi.responses.FileResponse(f"{pathlib.Path(__file__).parent.parent.resolve()}/assets/resized/Astroid-banner{width}x{height}.png") | ||
| else: | ||
| return fastapi.responses.JSONResponse(status_code=404, content={"message": "This asset does not exist."}) | ||
| try: | ||
| if asset == "logo_no_bg": | ||
| image = Image.open(base_path / "Astroid Logo no bg.png") | ||
| new_image = image.resize((width, height)) | ||
| new_image.save(resized_path / f"Astroid Logo no bg{width}x{height}.png") | ||
| return fastapi.responses.FileResponse(resized_path / f"Astroid Logo no bg{width}x{height}.png") | ||
| elif asset == "logo": | ||
| image = Image.open(base_path / "Astroid Logo.png") | ||
| new_image = image.resize((width, height)) | ||
| new_image.save(resized_path / f"Astroid Logo{width}x{height}.png") | ||
| return fastapi.responses.FileResponse(resized_path / f"Astroid Logo{width}x{height}.png") | ||
| elif asset == "banner": | ||
| image = Image.open(base_path / "Astroid-banner.png") | ||
| new_image = image.resize((width, height)) | ||
| new_image.save(resized_path / f"Astroid-banner{width}x{height}.png") | ||
| return fastapi.responses.FileResponse(resized_path / f"Astroid-banner{width}x{height}.png") | ||
| else: | ||
| return fastapi.responses.JSONResponse(status_code=404, content={"message": "This asset does not exist."}) | ||
| except: | ||
| return fastapi.responses.JSONResponse(status_code=500, content={"message": "Error processing the asset."}) | ||
|
|
| asset = await astroidapi.surrealdb_handler.AttachmentProcessor.get_attachment(assetId) | ||
| try: | ||
| if asset: | ||
| return fastapi.responses.FileResponse(f"{pathlib.Path(__file__).parent.resolve()}/astroidapi/TMP_attachments/{assetId}.{asset['type']}") |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To fix the issue, we need to ensure that the constructed file path is validated to prevent directory traversal attacks. This can be achieved by normalizing the path and verifying that it remains within the intended base directory. Specifically:
- Use
os.path.normpathto normalize the constructed path, removing any..sequences. - Check that the normalized path starts with the intended base directory (
astroidapi/TMP_attachments). - If the path is invalid (i.e., it does not start with the base directory), return an appropriate error response.
The changes will be made in the get_cdn_asset function in src/api.py.
| @@ -164,3 +164,8 @@ | ||
| if asset: | ||
| return fastapi.responses.FileResponse(f"{pathlib.Path(__file__).parent.resolve()}/astroidapi/TMP_attachments/{assetId}.{asset['type']}") | ||
| base_path = pathlib.Path(__file__).parent.resolve() / "astroidapi/TMP_attachments" | ||
| file_path = base_path / f"{assetId}.{asset['type']}" | ||
| normalized_path = file_path.resolve() | ||
| if not str(normalized_path).startswith(str(base_path)): | ||
| return fastapi.responses.JSONResponse(status_code=400, content={"message": "Invalid asset path."}) | ||
| return fastapi.responses.FileResponse(normalized_path) | ||
| else: |
| return fastapi.responses.JSONResponse(status_code=200, content={"message": f"An error occurred: {e}", | ||
| "details": "unexpectederror"}) |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To fix the issue, the code should avoid including the raw exception details (e) in the response sent to the user. Instead, a generic error message should be returned, while the detailed exception information (including stack trace) should be logged securely on the server for debugging purposes. This ensures that developers can access the necessary information to diagnose issues without exposing sensitive details to external users.
The changes will involve:
- Replacing the response message that includes
f"An error occurred: {e}"with a generic error message like"An internal error has occurred.". - Logging the exception details using
logging.exception()to capture the stack trace for server-side debugging.
| @@ -485,3 +485,4 @@ | ||
| except astroidapi.errors.HealtCheckError.EndpointCheckError as e: | ||
| return fastapi.responses.JSONResponse(status_code=200, content={"message": f"An error occurred: {e}", | ||
| logging.exception("An error occurred during endpoint health check.") | ||
| return fastapi.responses.JSONResponse(status_code=200, content={"message": "An internal error has occurred.", | ||
| "details": "unexpectederror"}) | ||
| @@ -491,4 +492,4 @@ | ||
| except astroidapi.errors.SurrealDBHandler.GetEndpointError as e: | ||
| traceback.print_exc() | ||
| return fastapi.responses.JSONResponse(status_code=404, content={"message": f"An error occurred: {e}", | ||
| logging.exception("An error occurred while retrieving endpoint information.") | ||
| return fastapi.responses.JSONResponse(status_code=404, content={"message": "An internal error has occurred.", | ||
| "details": "getendpointerror"}) | ||
| @@ -507,4 +508,4 @@ | ||
| except Exception as e: | ||
| logging.exception(traceback.print_exc()) | ||
| return fastapi.responses.JSONResponse(status_code=500, content={"message": f"An error occurred: {e}"}) | ||
| logging.exception("An error occurred during endpoint repair.") | ||
| return fastapi.responses.JSONResponse(status_code=500, content={"message": "An internal error has occurred."}) | ||
| else: |
| return fastapi.responses.JSONResponse(status_code=404, content={"message": f"An error occurred: {e}", | ||
| "details": "getendpointerror"}) |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To fix the issue, we need to ensure that sensitive exception details are not exposed to external users. Instead of including the exception message (e) in the response, we should log the detailed error information on the server and return a generic error message to the user. This approach aligns with the "GOOD" example in the background section.
Specifically:
- Replace the direct inclusion of
ein the response message with a generic error message. - Log the detailed exception information using
logging.exception()to ensure developers can still debug the issue. - Remove the use of
traceback.print_exc()in the response logic to avoid unnecessary exposure of stack trace information.
| @@ -491,4 +491,4 @@ | ||
| except astroidapi.errors.SurrealDBHandler.GetEndpointError as e: | ||
| traceback.print_exc() | ||
| return fastapi.responses.JSONResponse(status_code=404, content={"message": f"An error occurred: {e}", | ||
| logging.exception("An error occurred while handling a GetEndpointError.") | ||
| return fastapi.responses.JSONResponse(status_code=404, content={"message": "An error occurred while processing your request.", | ||
| "details": "getendpointerror"}) |
| return fastapi.responses.JSONResponse(status_code=200, content={"message": "Repaired.", "summary": summary}) | ||
| except Exception as e: | ||
| logging.exception(traceback.print_exc()) | ||
| return fastapi.responses.JSONResponse(status_code=500, content={"message": f"An error occurred: {e}"}) |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To fix the issue, we should avoid exposing the exception details (e) to the user. Instead, we can log the full stack trace on the server for debugging purposes and return a generic error message to the user. This ensures that sensitive information is not leaked while still allowing developers to diagnose the issue using server logs.
The changes involve:
- Replacing the response message on line 509 with a generic error message.
- Ensuring that the exception details are logged on the server using
logging.exception().
| @@ -507,4 +507,4 @@ | ||
| except Exception as e: | ||
| logging.exception(traceback.print_exc()) | ||
| return fastapi.responses.JSONResponse(status_code=500, content={"message": f"An error occurred: {e}"}) | ||
| logging.exception("An error occurred while repairing the endpoint.", exc_info=True) | ||
| return fastapi.responses.JSONResponse(status_code=500, content={"message": "An internal error has occurred. Please contact support if the issue persists."}) | ||
| else: |
| except KeyError: | ||
| if token == Bot.config.MASTER_TOKEN: | ||
| try: | ||
| os.remove(f"{pathlib.Path(__file__).parent.resolve()}/endpoints/{endpoint}.json") |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To fix the issue, we need to ensure that the constructed file path is safe and does not allow access to unintended files. This can be achieved by:
- Normalizing the constructed file path using
os.path.normpathorpathlib.Path.resolve()to eliminate any..segments. - Verifying that the normalized path is within the intended directory (e.g., the
endpointsdirectory). - Rejecting any paths that fall outside the intended directory.
The fix will involve:
- Normalizing the constructed file path.
- Checking that the normalized path starts with the resolved
endpointsdirectory path. - Raising an exception or returning an error response if the path is invalid.
| @@ -596,3 +596,7 @@ | ||
| try: | ||
| os.remove(f"{pathlib.Path(__file__).parent.resolve()}/endpoints/{endpoint}.json") | ||
| base_path = pathlib.Path(__file__).parent.resolve() / "endpoints" | ||
| file_path = (base_path / f"{endpoint}.json").resolve() | ||
| if not str(file_path).startswith(str(base_path)): | ||
| return fastapi.responses.JSONResponse(status_code=400, content={"message": "Invalid endpoint path."}) | ||
| os.remove(file_path) | ||
| return fastapi.responses.JSONResponse(status_code=200, content={"message": "Deleted."}) |
| await astroidapi.surrealdb_handler.sync_server_relations() | ||
| return fastapi.responses.JSONResponse(status_code=200, content={"message": "Success."}) | ||
| except Exception as e: | ||
| return fastapi.responses.JSONResponse(status_code=500, content={"message": f"An error occurred: {e}"}) |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To fix the issue, the exception details should not be included in the response sent to the user. Instead, a generic error message should be returned, and the exception details should be logged securely for debugging purposes. This ensures that sensitive information is not exposed to external users while still allowing developers to diagnose issues.
Steps to implement the fix:
- Replace the response message on line 757 with a generic error message, such as
"An internal error has occurred.". - Log the exception details using the
loggingmodule to ensure developers can access the stack trace for debugging. - Ensure that the logging configuration is already set up to write logs to a file or another secure location.
| @@ -756,3 +756,4 @@ | ||
| except Exception as e: | ||
| return fastapi.responses.JSONResponse(status_code=500, content={"message": f"An error occurred: {e}"}) | ||
| logging.error(f"An error occurred: {e}", exc_info=True) | ||
| return fastapi.responses.JSONResponse(status_code=500, content={"message": "An internal error has occurred."}) | ||
|
|
|
I'm confident in this change, but I'm not a maintainer of this project. Do you see any reason not to merge it? If this change was not helpful, or you have suggestions for improvements, please let me know! |
|
Just a friendly ping to remind you about this change. If there are concerns about it, we'd love to hear about them! |
|
This change may not be a priority right now, so I'll close it. If there was something I could have done better, please let me know! You can also customize me to make sure I'm working with you in the way you want. |
Many developers will be surprised to learn that
requestslibrary calls do not include timeouts by default. This means that an attempted request could hang indefinitely if no connection is established or if no data is received from the server.The requests documentation suggests that most calls should explicitly include a
timeoutparameter. This codemod adds a default timeout value in order to set an upper bound on connection times and ensure that requests connect or fail in a timely manner. This value also ensures the connection will timeout if the server does not respond with data within a reasonable amount of time.While timeout values will be application dependent, we believe that this codemod adds a reasonable default that serves as an appropriate ceiling for most situations.
Our changes look like the following:
More reading
🧚🤖 Powered by Pixeebot
Feedback | Community | Docs | Codemod ID: pixee:python/add-requests-timeouts