Refactor clip processor server layout #522
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| 'recent_requests': [dict(row) for row in recent_requests], | ||
| }) | ||
| except Exception as e: | ||
| return jsonify({'error': str(e), 'trace': traceback.format_exc()}), 500 |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To fix the issue, we will modify the exception handling in the debug_queue function to ensure that stack traces are not exposed to the client. Instead, we will log the stack trace on the server and return a generic error message to the client. This approach ensures that sensitive information is not leaked while still allowing developers to debug issues using server logs.
Steps to implement the fix:
- Replace the current
returnstatement in theexceptblock with a generic error message for the client. - Log the stack trace on the server using a logging library (e.g., Python's built-in
loggingmodule). - Ensure that the logging setup is configured to capture and store error logs appropriately.
| @@ -92,3 +92,4 @@ | ||
| except Exception as e: | ||
| return jsonify({'error': str(e), 'trace': traceback.format_exc()}), 500 | ||
| app.logger.error("An error occurred in debug_queue: %s", traceback.format_exc()) | ||
| return jsonify({'error': 'An internal error has occurred. Please try again later.'}), 500 | ||
|
|
| image_dir_abs_path = os.path.abspath(IMAGE_DIR) | ||
| if not image_abs_path.startswith(image_dir_abs_path + os.sep): | ||
| return jsonify({'error': 'Access denied'}), 403 | ||
| if not image_path.exists() or not image_path.is_file(): |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To fix the issue, we will:
- Normalize the
filenameand the constructedimage_pathusingos.path.normpathto eliminate any..segments or other irregularities. - Ensure that the normalized
image_pathis strictly contained within theIMAGE_DIRdirectory by comparing the absolute paths. - Remove redundant or less effective checks (e.g.,
if '/' in filename or '\\' in filename or '..' in filename) to simplify the code and avoid false assumptions about path traversal prevention.
The changes will be made in the serve_image function, specifically in the validation logic for filename and image_path.
| @@ -139,17 +139,26 @@ | ||
| try: | ||
| if '/' in filename or '\\' in filename or '..' in filename: | ||
| return jsonify({'error': 'Invalid filename'}), 400 | ||
| filename = os.path.basename(filename) | ||
| if '.' not in filename or filename.rsplit('.', 1)[1].lower() not in ALLOWED_EXTENSIONS: | ||
| return jsonify({'error': 'Invalid file type'}), 400 | ||
| image_path = IMAGE_DIR / filename | ||
| image_abs_path = os.path.abspath(image_path) | ||
| image_dir_abs_path = os.path.abspath(IMAGE_DIR) | ||
| if not image_abs_path.startswith(image_dir_abs_path + os.sep): | ||
| return jsonify({'error': 'Access denied'}), 403 | ||
| if not image_path.exists() or not image_path.is_file(): | ||
| return jsonify({'error': 'Image not found'}), 404 | ||
| try: | ||
| if not image_path.resolve().is_relative_to(IMAGE_DIR.resolve()): | ||
| # Normalize the filename and construct the full image path | ||
| normalized_filename = os.path.normpath(filename) | ||
| image_path = (IMAGE_DIR / normalized_filename).resolve() | ||
|
|
||
| # Ensure the file is within the allowed directory | ||
| if not image_path.is_relative_to(IMAGE_DIR.resolve()): | ||
| return jsonify({'error': 'Access denied'}), 403 | ||
|
|
||
| # Validate the file extension | ||
| if '.' not in normalized_filename or normalized_filename.rsplit('.', 1)[1].lower() not in ALLOWED_EXTENSIONS: | ||
| return jsonify({'error': 'Invalid file type'}), 400 | ||
|
|
||
| # Check if the file exists and is a regular file | ||
| if not image_path.exists() or not image_path.is_file(): | ||
| return jsonify({'error': 'Image not found'}), 404 | ||
|
|
||
| # Serve the file | ||
| response = send_file(image_path, mimetype=f'image/{image_path.suffix[1:]}') | ||
| response.headers['Content-Security-Policy'] = "default-src 'self'" | ||
| response.headers['X-Content-Type-Options'] = 'nosniff' | ||
| response.headers['Cache-Control'] = 'no-store, no-cache, must-revalidate, max-age=0' | ||
| response.headers['Pragma'] = 'no-cache' | ||
| return response | ||
| except (ValueError, RuntimeError): |
| image_dir_abs_path = os.path.abspath(IMAGE_DIR) | ||
| if not image_abs_path.startswith(image_dir_abs_path + os.sep): | ||
| return jsonify({'error': 'Access denied'}), 403 | ||
| if not image_path.exists() or not image_path.is_file(): |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To fix the issue, we will:
- Normalize the
filenameand construct the full path usingos.path.normpathto eliminate any..segments or redundant separators. - Validate that the normalized path is within the
IMAGE_DIRdirectory by comparing it against the absolute path ofIMAGE_DIR. - Remove redundant checks (e.g.,
is_relative_to) to simplify the logic and avoid masking potential issues.
This approach ensures that the constructed path is safe and contained within the intended directory.
| @@ -144,14 +144,8 @@ | ||
| return jsonify({'error': 'Invalid file type'}), 400 | ||
| image_path = IMAGE_DIR / filename | ||
| image_abs_path = os.path.abspath(image_path) | ||
| image_path = os.path.normpath(IMAGE_DIR / filename) | ||
| image_dir_abs_path = os.path.abspath(IMAGE_DIR) | ||
| if not image_abs_path.startswith(image_dir_abs_path + os.sep): | ||
| if not image_path.startswith(image_dir_abs_path + os.sep): | ||
| return jsonify({'error': 'Access denied'}), 403 | ||
| if not image_path.exists() or not image_path.is_file(): | ||
| if not os.path.isfile(image_path): | ||
| return jsonify({'error': 'Image not found'}), 404 | ||
| try: | ||
| if not image_path.resolve().is_relative_to(IMAGE_DIR.resolve()): | ||
| return jsonify({'error': 'Access denied'}), 403 | ||
| except (ValueError, RuntimeError): | ||
| return jsonify({'error': 'Access denied'}), 403 | ||
| response = send_file(image_path, mimetype=f'image/{image_path.suffix[1:]}') |
| if not image_path.exists() or not image_path.is_file(): | ||
| return jsonify({'error': 'Image not found'}), 404 | ||
| try: | ||
| if not image_path.resolve().is_relative_to(IMAGE_DIR.resolve()): |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To fix the issue, we will:
- Replace the use of
resolve()on line 153 with a more straightforward and reliable approach usingos.path.normpath. - Normalize the
image_pathusingos.path.normpathand validate that it starts with the absolute path ofIMAGE_DIR. - Remove the redundant
resolve()check, as it is no longer necessary.
This fix ensures that the constructed path is safe and contained within the intended directory, addressing the flagged issue.
| @@ -152,5 +152,6 @@ | ||
| try: | ||
| if not image_path.resolve().is_relative_to(IMAGE_DIR.resolve()): | ||
| normalized_path = os.path.normpath(image_path) | ||
| if not normalized_path.startswith(os.path.abspath(IMAGE_DIR) + os.sep): | ||
| return jsonify({'error': 'Access denied'}), 403 | ||
| except (ValueError, RuntimeError): | ||
| except Exception: | ||
| return jsonify({'error': 'Access denied'}), 403 |
| return jsonify({'error': 'Access denied'}), 403 | ||
| except (ValueError, RuntimeError): | ||
| return jsonify({'error': 'Access denied'}), 403 | ||
| response = send_file(image_path, mimetype=f'image/{image_path.suffix[1:]}') |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To fix the issue, we will:
- Normalize the
filenameand construct the full path usingos.path.joinandos.path.normpathto eliminate any..segments or other irregularities. - Verify that the normalized path starts with the absolute path of
IMAGE_DIRto ensure it is contained within the allowed directory. - Remove redundant checks (e.g.,
is_relative_to) and streamline the validation logic for clarity and reliability. - Ensure compatibility with Python versions below 3.9 by avoiding the use of
is_relative_to.
The changes will be made in the serve_image function in packages/clip-processor-py/src/server/routes.py.
| @@ -139,19 +139,17 @@ | ||
| try: | ||
| if '/' in filename or '\\' in filename or '..' in filename: | ||
| return jsonify({'error': 'Invalid filename'}), 400 | ||
| filename = os.path.basename(filename) | ||
| if '.' not in filename or filename.rsplit('.', 1)[1].lower() not in ALLOWED_EXTENSIONS: | ||
| return jsonify({'error': 'Invalid file type'}), 400 | ||
| image_path = IMAGE_DIR / filename | ||
| image_abs_path = os.path.abspath(image_path) | ||
| # Normalize the filename and construct the full path | ||
| image_path = os.path.normpath(os.path.join(IMAGE_DIR, filename)) | ||
| image_dir_abs_path = os.path.abspath(IMAGE_DIR) | ||
| if not image_abs_path.startswith(image_dir_abs_path + os.sep): | ||
|
|
||
| # Validate that the path is within the allowed directory | ||
| if not image_path.startswith(image_dir_abs_path + os.sep): | ||
| return jsonify({'error': 'Access denied'}), 403 | ||
| if not image_path.exists() or not image_path.is_file(): | ||
|
|
||
| # Validate the file extension | ||
| if '.' not in filename or filename.rsplit('.', 1)[1].lower() not in ALLOWED_EXTENSIONS: | ||
| return jsonify({'error': 'Invalid file type'}), 400 | ||
|
|
||
| # Check if the file exists and is a file | ||
| if not os.path.isfile(image_path): | ||
| return jsonify({'error': 'Image not found'}), 404 | ||
| try: | ||
| if not image_path.resolve().is_relative_to(IMAGE_DIR.resolve()): | ||
| return jsonify({'error': 'Access denied'}), 403 | ||
| except (ValueError, RuntimeError): | ||
| return jsonify({'error': 'Access denied'}), 403 | ||
| response = send_file(image_path, mimetype=f'image/{image_path.suffix[1:]}') |
| if 'twitch.tv' not in parsed_url.netloc and 'clips.twitch.tv' not in parsed_url.netloc: | ||
| pass | ||
| except Exception as e: | ||
| return jsonify({'error': f'URL parsing error: {str(e)}'}), 400 |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To fix the issue, we will replace the exposed exception details (str(e)) in the response with a generic error message. The detailed exception information will be logged on the server using Python's logging module. This ensures that developers can still debug the issue without exposing sensitive information to the user.
Steps to implement the fix:
- Import the
loggingmodule if it is not already imported. - Replace the response on line 206 with a generic error message.
- Log the exception details (
str(e)and the stack trace) on the server for debugging purposes.
| @@ -205,3 +205,4 @@ | ||
| except Exception as e: | ||
| return jsonify({'error': f'URL parsing error: {str(e)}'}), 400 | ||
| app.logger.error(f"URL parsing error: {str(e)}", exc_info=True) | ||
| return jsonify({'error': 'An error occurred while parsing the URL'}), 400 | ||
|
|
| ) | ||
| return jsonify(result) | ||
| except Exception as e: | ||
| return jsonify({'error': 'Error processing clip', 'message': str(e), 'trace': traceback.format_exc() if debug else None}), 500 |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To fix the issue, we will remove the stack trace (traceback.format_exc()) from the user-facing response entirely. Instead, we will log the stack trace on the server side for debugging purposes. This ensures that sensitive information is not exposed to external users while still allowing developers to diagnose issues using server logs. The trace field will be removed from the JSON response, and the exception details will be logged using a logging mechanism.
| @@ -219,3 +219,4 @@ | ||
| except Exception as e: | ||
| return jsonify({'error': 'Error processing clip', 'message': str(e), 'trace': traceback.format_exc() if debug else None}), 500 | ||
| app.logger.error("Error processing clip: %s", traceback.format_exc()) | ||
| return jsonify({'error': 'Error processing clip', 'message': 'An internal error occurred'}), 500 | ||
|
|
| ) | ||
| return jsonify(result) | ||
| except Exception as e: | ||
| return jsonify({'error': 'Error processing stream', 'message': str(e), 'trace': traceback.format_exc() if debug else None}), 500 |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To fix the issue, we will ensure that stack trace information is never exposed to external users, regardless of the debug parameter. Instead, we will log the stack trace on the server for debugging purposes and return a generic error message to the user. This approach aligns with best practices for error handling and prevents sensitive information from being leaked.
| @@ -250,3 +250,4 @@ | ||
| except Exception as e: | ||
| return jsonify({'error': 'Error processing stream', 'message': str(e), 'trace': traceback.format_exc() if debug else None}), 500 | ||
| app.logger.error("Error processing stream: %s", traceback.format_exc()) | ||
| return jsonify({'error': 'Error processing stream', 'message': 'An internal error has occurred.'}), 500 | ||
|
|
| ) | ||
| return jsonify(result) | ||
| except Exception as e: | ||
| return jsonify({'error': 'Error processing match', 'message': str(e), 'trace': traceback.format_exc() if debug else None}), 500 |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To fix the issue, we will ensure that stack trace information is not exposed to external users under any circumstances. Instead, we will log the stack trace on the server for debugging purposes and return a generic error message to the user. This approach adheres to the principle of minimizing information exposure while still allowing developers to diagnose issues using server-side logs.
The changes will involve:
- Replacing the
traceback.format_exc()in the response with a generic error message. - Logging the stack trace using a logging mechanism (e.g., Python's
loggingmodule) for internal use.
| @@ -250,3 +250,4 @@ | ||
| except Exception as e: | ||
| return jsonify({'error': 'Error processing stream', 'message': str(e), 'trace': traceback.format_exc() if debug else None}), 500 | ||
| app.logger.error("Error processing stream: %s", traceback.format_exc()) | ||
| return jsonify({'error': 'Error processing stream', 'message': 'An internal error occurred. Please try again later.'}), 500 | ||
|
|
||
| @@ -339,2 +340,3 @@ | ||
| except Exception as e: | ||
| return jsonify({'error': 'Error processing match', 'message': str(e), 'trace': traceback.format_exc() if debug else None}), 500 | ||
| app.logger.error("Error processing match: %s", traceback.format_exc()) | ||
| return jsonify({'error': 'Error processing match', 'message': 'An internal error occurred. Please try again later.'}), 500 |
1 participant
Summary
serverpackage with Flask app factory and routesapi_server.pycreate_appwhen launching the APITesting
git status --short