chore: added validation for description #7507
Conversation
WalkthroughThis change introduces comprehensive content validation for description fields across several serializers, ensuring that binary, HTML, and JSON content are checked for malicious patterns and size limits. A new utility module provides the validation logic. Related serializers and views are updated to use these validators, and a new serializer handles page binary updates securely. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant View
participant Serializer
participant ContentValidator
Client->>View: Sends request with description fields
View->>Serializer: Initializes with request data
Serializer->>ContentValidator: Calls validate_* for each field
ContentValidator-->>Serializer: Returns (is_valid, error_msg)
alt Any validation fails
Serializer-->>View: Raises ValidationError
View-->>Client: Returns error response
else All validations pass
Serializer-->>View: Returns validated data
View-->>Client: Proceeds with update/creation
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🔇 Additional comments (2)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
apps/api/plane/utils/binary_validator.py (1)
22-25: Consider making minimum length validation more meaningful.The 4-byte minimum length check seems arbitrary. Consider either:
- Removing this check if empty data is already handled above
- Using a more meaningful minimum based on actual document format requirements
- Adding a comment explaining why 4 bytes is the minimum
# Basic format validation -if len(binary_data) < 4: - return False, "Binary data too short to be valid document format" +# Most document formats have meaningful headers, so require minimum viable content +if len(binary_data) < 4: + return False, "Binary data too short to be valid document format"apps/api/plane/app/serializers/project.py (1)
16-20: Consider removing unused import.The
validate_binary_datafunction is imported but not used in this serializer. Consider removing it to keep imports clean.from plane.utils.content_validator import ( validate_html_content, validate_json_content, - validate_binary_data, )apps/api/plane/api/serializers/project.py (1)
6-10: Consider removing unused import.The
validate_binary_datafunction is imported but not used in this serializer. Consider removing it for cleaner code.from plane.utils.content_validator import ( validate_html_content, validate_json_content, - validate_binary_data, )apps/api/plane/app/serializers/page.py (1)
204-225: Improve exception handling for base64 decoding.The current exception handling might hide specific base64 decoding errors. Consider catching the specific exception type for better error messages.
- except Exception as e: - if isinstance(e, serializers.ValidationError): - raise - raise serializers.ValidationError("Failed to decode base64 data") + except serializers.ValidationError: + raise + except Exception as e: + raise serializers.ValidationError(f"Failed to decode base64 data: {str(e)}")apps/api/plane/utils/content_validator.py (3)
147-182: Improve exception handling in HTML structure validation.The bare
except Exception:clause should be more specific about what exceptions are expected.- except Exception: - # If HTML parsing fails, we'll allow it - pass + except (re.error, ValueError, TypeError): + # If regex parsing fails, we'll allow it + pass
186-234: Optimize size checking and improve exception handling.
- Using
json.dumps()just for size checking is inefficient for large objects- Generic
Exceptioncatch is too broadFor better performance with large JSON objects, consider using a recursive size estimation function instead of serializing the entire object. Also, make exception handling more specific:
- except (TypeError, ValueError) as e: - return False, "Invalid JSON structure" - except Exception as e: - return False, "Failed to validate JSON content" + except (TypeError, ValueError) as e: + return False, f"Invalid JSON structure: {str(e)}" + except (RecursionError, MemoryError) as e: + return False, f"Failed to validate JSON content: {str(e)}"
1-4: Consider adding type hints for better code documentation.Adding type hints would improve code readability and enable better IDE support.
# Python imports import json import re +from typing import Tuple, Optional, Dict, List, AnyThen update function signatures:
def validate_binary_data(binary_data: bytes) -> Tuple[bool, Optional[str]]: def validate_html_content(html_content: str) -> Tuple[bool, Optional[str]]: def validate_json_content(json_content: Dict[str, Any]) -> Tuple[bool, Optional[str]]:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/api/plane/api/serializers/issue.py(2 hunks)apps/api/plane/api/serializers/project.py(2 hunks)apps/api/plane/app/serializers/__init__.py(1 hunks)apps/api/plane/app/serializers/draft.py(2 hunks)apps/api/plane/app/serializers/issue.py(2 hunks)apps/api/plane/app/serializers/page.py(2 hunks)apps/api/plane/app/serializers/project.py(2 hunks)apps/api/plane/app/serializers/workspace.py(2 hunks)apps/api/plane/app/views/page/base.py(2 hunks)apps/api/plane/space/serializer/issue.py(2 hunks)apps/api/plane/utils/binary_validator.py(1 hunks)apps/api/plane/utils/content_validator.py(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: NarayanBavisetti
PR: makeplane/plane#7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
apps/api/plane/app/serializers/project.py (1)
Learnt from: NarayanBavisetti
PR: #7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
apps/api/plane/space/serializer/issue.py (1)
Learnt from: NarayanBavisetti
PR: #7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
apps/api/plane/app/serializers/workspace.py (1)
Learnt from: NarayanBavisetti
PR: #7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
apps/api/plane/api/serializers/issue.py (1)
Learnt from: NarayanBavisetti
PR: #7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
apps/api/plane/app/serializers/issue.py (1)
Learnt from: NarayanBavisetti
PR: #7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
apps/api/plane/api/serializers/project.py (1)
Learnt from: NarayanBavisetti
PR: #7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
apps/api/plane/app/serializers/draft.py (1)
Learnt from: NarayanBavisetti
PR: #7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
🔇 Additional comments (21)
apps/api/plane/utils/binary_validator.py (2)
14-16: LGTM - Empty data handling is appropriate.Allowing empty binary data as valid is correct for optional fields and follows good validation practices.
17-21: LGTM - Size limit is reasonable.The 10MB limit for binary document data is appropriate and helps prevent resource exhaustion attacks.
apps/api/plane/app/serializers/__init__.py (1)
99-99: LGTM - Standard import addition.The addition of
PageBinaryUpdateSerializerto the module exports follows the established pattern and enables its use throughout the application.apps/api/plane/app/serializers/workspace.py (2)
27-31: LGTM - Clean import organization.The content validation imports are properly organized and follow the existing import structure in the file.
320-337: LGTM - Comprehensive validation implementation.The validation logic is well-implemented with:
- Consistent pattern across all three description formats
- Proper error handling with field-specific error messages
- Conditional validation that only runs when fields are present and non-empty
- Appropriate use of ValidationError with structured error messages
This follows the same secure pattern implemented across other serializers in this PR.
apps/api/plane/space/serializer/issue.py (2)
31-35: LGTM - Proper import organization.The content validation imports are correctly placed and follow the existing import structure.
292-307: LGTM - Well-integrated validation logic.The description content validation is properly integrated into the existing validate method:
- Maintains existing date validation logic
- Follows consistent validation pattern across all three description formats
- Proper error handling with field-specific ValidationError messages
- Clean separation between different validation concerns
This implementation aligns well with the security objectives of the PR.
apps/api/plane/app/serializers/draft.py (2)
20-24: LGTM - Clean import addition.The content validation imports are properly organized and follow the established import patterns in the file.
73-88: LGTM - Consistent validation implementation.The description content validation follows the same secure pattern established across other serializers in this PR:
- Proper integration with existing date validation logic
- Consistent three-field validation pattern
- Appropriate error handling with field-specific ValidationError messages
- Clean conditional validation that only runs when fields are present and non-empty
This enhances the security of draft issue creation while maintaining code consistency.
apps/api/plane/app/serializers/project.py (1)
66-90: LGTM! Well-implemented content validation.The validation logic properly handles different data types for description fields:
- Uses type checking to determine appropriate validation method
- Consistent error handling with field-specific messages
- Good comments explaining the logic for mixed data types
This enhances security by validating user-provided description content before persistence.
apps/api/plane/app/serializers/issue.py (2)
41-45: LGTM! Clean imports.All imported validation functions are properly used in the validation logic.
135-150: LGTM! Proper content validation implementation.The validation logic is well-implemented with:
- Appropriate validation functions for each description field type
- Consistent error handling with field-specific messages
- Good placement in the validation flow after business logic
This strengthens security by validating description content before issue creation.
apps/api/plane/api/serializers/issue.py (2)
24-28: LGTM! Complete and necessary imports.All imported validation functions are properly utilized in the validation method.
83-97: LGTM! Consistent validation implementation.The content validation is properly implemented with:
- Appropriate use of
.get()method for field checking- Correct validation functions for each content type
- Consistent error handling pattern
- Good placement after HTML parsing in the validation flow
This adds necessary security validation for description content.
apps/api/plane/api/serializers/project.py (1)
65-87: LGTM! Consistent validation implementation.The validation logic properly mirrors the app serializer implementation with:
- Appropriate type checking for mixed data types
- Consistent error handling with field-specific messages
- Good placement in the validation flow
This maintains consistency across API layers while adding necessary security validation.
apps/api/plane/app/views/page/base.py (2)
28-28: LGTM! Necessary import addition.The
PageBinaryUpdateSerializerimport is properly added for the refactored partial_update method.
542-562: Excellent refactoring! Significant improvement in security and maintainability.The refactored approach provides several benefits:
Security improvements:
- Replaces manual base64 decoding with structured validation
- Leverages the content validation framework for security checks
Code quality improvements:
- Centralizes validation logic in the serializer
- Provides detailed error messages through serializer validation
- Follows consistent patterns with other endpoints
Maintainability:
- Eliminates manual data manipulation
- Makes the code more testable and debuggable
The background task integration is properly maintained while improving the overall robustness of the endpoint.
apps/api/plane/app/serializers/page.py (4)
3-3: LGTM!The imports are correctly organized with standard library imports before local imports, and they align with the usage in the new
PageBinaryUpdateSerializer.Also applies to: 7-11
197-202: LGTM!The serializer correctly extends
serializers.Serializerfor update-only operations, and the field definitions appropriately handle optional data with proper types.
226-236: LGTM!The HTML validation method correctly delegates to the utility function and properly handles error messages.
238-248: LGTM!The JSON validation method follows the same clean pattern as the HTML validator and properly handles errors.
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces comprehensive validation for description fields across various entities (issues, projects, drafts, pages, and workspace stickies) to prevent malicious content injection. The validation covers three formats: binary data, HTML content, and JSON structure to enhance security.
Key Changes:
- Added a new
content_validator.pyutility module with validation functions for binary, HTML, and JSON content - Integrated description validation across multiple serializers for issues, projects, drafts, pages, and workspace components
- Introduced a new
PageBinaryUpdateSerializerfor secure page description updates with proper validation
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
apps/api/plane/utils/content_validator.py |
New utility module providing security validation functions for binary, HTML, and JSON content |
apps/api/plane/space/serializer/issue.py |
Added description validation for space issue serializer |
apps/api/plane/app/views/page/base.py |
Updated page update endpoint to use new validation serializer |
apps/api/plane/app/serializers/workspace.py |
Added description validation for workspace sticky serializer |
apps/api/plane/app/serializers/project.py |
Added description validation for project serializer |
apps/api/plane/app/serializers/page.py |
Added new PageBinaryUpdateSerializer with comprehensive validation |
apps/api/plane/app/serializers/issue.py |
Added description validation for issue serializer |
apps/api/plane/app/serializers/draft.py |
Added description validation for draft issue serializer |
apps/api/plane/app/serializers/__init__.py |
Exported new PageBinaryUpdateSerializer |
apps/api/plane/api/serializers/project.py |
Added description validation for API project serializer |
apps/api/plane/api/serializers/issue.py |
Added description validation for API issue serializer |
- Improve content validation by consolidating patterns and enhancing recursion checks - Updated `PageBinaryUpdateSerializer` to simplify assignment of validated data. - Enhanced `content_validator.py` with consolidated dangerous patterns and added recursion depth checks to prevent stack overflow during validation. - Improved readability and maintainability of validation functions by using constants for patterns.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
* added PageBinaryUpdateSerializer for binary data validation and update * chore: added validation for description * chore: removed the duplicated file * Fixed coderabbit comments - Improve content validation by consolidating patterns and enhancing recursion checks - Updated `PageBinaryUpdateSerializer` to simplify assignment of validated data. - Enhanced `content_validator.py` with consolidated dangerous patterns and added recursion depth checks to prevent stack overflow during validation. - Improved readability and maintainability of validation functions by using constants for patterns. --------- Co-authored-by: Dheeraj Kumar Ketireddy <dheeru0198@gmail.com>
Description
This pull request adds several validations for the description field in JSON, HTML, and binary formats.
Test Scenarios
Summary by CodeRabbit
New Features
Bug Fixes
Refactor