From 69d5cd183f4c42e10fd59ed6b6586af47c21720a Mon Sep 17 00:00:00 2001 From: Bavisetti Narayan <72156168+NarayanBavisetti@users.noreply.github.com> Date: Wed, 30 Jul 2025 14:19:49 +0530 Subject: [PATCH 1/2] chore: added validation for description (#7507) * 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 --- apps/api/plane/api/serializers/issue.py | 21 ++ apps/api/plane/api/serializers/project.py | 28 ++ apps/api/plane/app/serializers/__init__.py | 1 + apps/api/plane/app/serializers/draft.py | 22 ++ apps/api/plane/app/serializers/issue.py | 21 ++ apps/api/plane/app/serializers/page.py | 74 ++++ apps/api/plane/app/serializers/project.py | 31 ++ apps/api/plane/app/serializers/workspace.py | 24 ++ apps/api/plane/app/views/page/base.py | 28 +- apps/api/plane/space/serializer/issue.py | 22 ++ apps/api/plane/utils/content_validator.py | 357 ++++++++++++++++++++ 11 files changed, 613 insertions(+), 16 deletions(-) create mode 100644 apps/api/plane/utils/content_validator.py diff --git a/apps/api/plane/api/serializers/issue.py b/apps/api/plane/api/serializers/issue.py index f906d4085f..c57f9d3597 100644 --- a/apps/api/plane/api/serializers/issue.py +++ b/apps/api/plane/api/serializers/issue.py @@ -21,6 +21,11 @@ from plane.db.models import ( State, User, ) +from plane.utils.content_validator import ( + validate_html_content, + validate_json_content, + validate_binary_data, +) from .base import BaseSerializer from .cycle import CycleLiteSerializer, CycleSerializer @@ -75,6 +80,22 @@ class IssueSerializer(BaseSerializer): except Exception: raise serializers.ValidationError("Invalid HTML passed") + # Validate description content for security + if data.get("description"): + is_valid, error_msg = validate_json_content(data["description"]) + if not is_valid: + raise serializers.ValidationError({"description": error_msg}) + + if data.get("description_html"): + is_valid, error_msg = validate_html_content(data["description_html"]) + if not is_valid: + raise serializers.ValidationError({"description_html": error_msg}) + + if data.get("description_binary"): + is_valid, error_msg = validate_binary_data(data["description_binary"]) + if not is_valid: + raise serializers.ValidationError({"description_binary": error_msg}) + # Validate assignees are from project if data.get("assignees", []): data["assignees"] = ProjectMember.objects.filter( diff --git a/apps/api/plane/api/serializers/project.py b/apps/api/plane/api/serializers/project.py index c76652e1e7..10ae7f4de3 100644 --- a/apps/api/plane/api/serializers/project.py +++ b/apps/api/plane/api/serializers/project.py @@ -3,6 +3,11 @@ from rest_framework import serializers # Module imports from plane.db.models import Project, ProjectIdentifier, WorkspaceMember +from plane.utils.content_validator import ( + validate_html_content, + validate_json_content, + validate_binary_data, +) from .base import BaseSerializer @@ -57,6 +62,29 @@ class ProjectSerializer(BaseSerializer): "Default assignee should be a user in the workspace" ) + # Validate description content for security + if "description" in data and data["description"]: + # For Project, description might be text field, not JSON + if isinstance(data["description"], dict): + is_valid, error_msg = validate_json_content(data["description"]) + if not is_valid: + raise serializers.ValidationError({"description": error_msg}) + + if "description_text" in data and data["description_text"]: + is_valid, error_msg = validate_json_content(data["description_text"]) + if not is_valid: + raise serializers.ValidationError({"description_text": error_msg}) + + if "description_html" in data and data["description_html"]: + if isinstance(data["description_html"], dict): + is_valid, error_msg = validate_json_content(data["description_html"]) + else: + is_valid, error_msg = validate_html_content( + str(data["description_html"]) + ) + if not is_valid: + raise serializers.ValidationError({"description_html": error_msg}) + return data def create(self, validated_data): diff --git a/apps/api/plane/app/serializers/__init__.py b/apps/api/plane/app/serializers/__init__.py index f0d98886e3..0116b20613 100644 --- a/apps/api/plane/app/serializers/__init__.py +++ b/apps/api/plane/app/serializers/__init__.py @@ -96,6 +96,7 @@ from .page import ( SubPageSerializer, PageDetailSerializer, PageVersionSerializer, + PageBinaryUpdateSerializer, PageVersionDetailSerializer, ) diff --git a/apps/api/plane/app/serializers/draft.py b/apps/api/plane/app/serializers/draft.py index f308352633..6479d44c83 100644 --- a/apps/api/plane/app/serializers/draft.py +++ b/apps/api/plane/app/serializers/draft.py @@ -17,6 +17,11 @@ from plane.db.models import ( DraftIssueCycle, DraftIssueModule, ) +from plane.utils.content_validator import ( + validate_html_content, + validate_json_content, + validate_binary_data, +) class DraftIssueCreateSerializer(BaseSerializer): @@ -64,6 +69,23 @@ class DraftIssueCreateSerializer(BaseSerializer): and data.get("start_date", None) > data.get("target_date", None) ): raise serializers.ValidationError("Start date cannot exceed target date") + + # Validate description content for security + if "description" in data and data["description"]: + is_valid, error_msg = validate_json_content(data["description"]) + if not is_valid: + raise serializers.ValidationError({"description": error_msg}) + + if "description_html" in data and data["description_html"]: + is_valid, error_msg = validate_html_content(data["description_html"]) + if not is_valid: + raise serializers.ValidationError({"description_html": error_msg}) + + if "description_binary" in data and data["description_binary"]: + is_valid, error_msg = validate_binary_data(data["description_binary"]) + if not is_valid: + raise serializers.ValidationError({"description_binary": error_msg}) + return data def create(self, validated_data): diff --git a/apps/api/plane/app/serializers/issue.py b/apps/api/plane/app/serializers/issue.py index 965d78aa2b..98ceaaad6c 100644 --- a/apps/api/plane/app/serializers/issue.py +++ b/apps/api/plane/app/serializers/issue.py @@ -38,6 +38,11 @@ from plane.db.models import ( IssueDescriptionVersion, ProjectMember, ) +from plane.utils.content_validator import ( + validate_html_content, + validate_json_content, + validate_binary_data, +) class IssueFlatSerializer(BaseSerializer): @@ -127,6 +132,22 @@ class IssueCreateSerializer(BaseSerializer): member_id__in=attrs["assignee_ids"], ).values_list("member_id", flat=True) + # Validate description content for security + if "description" in attrs and attrs["description"]: + is_valid, error_msg = validate_json_content(attrs["description"]) + if not is_valid: + raise serializers.ValidationError({"description": error_msg}) + + if "description_html" in attrs and attrs["description_html"]: + is_valid, error_msg = validate_html_content(attrs["description_html"]) + if not is_valid: + raise serializers.ValidationError({"description_html": error_msg}) + + if "description_binary" in attrs and attrs["description_binary"]: + is_valid, error_msg = validate_binary_data(attrs["description_binary"]) + if not is_valid: + raise serializers.ValidationError({"description_binary": error_msg}) + return attrs def create(self, validated_data): diff --git a/apps/api/plane/app/serializers/page.py b/apps/api/plane/app/serializers/page.py index 1fd2f4d3c8..78762e4b4e 100644 --- a/apps/api/plane/app/serializers/page.py +++ b/apps/api/plane/app/serializers/page.py @@ -1,8 +1,14 @@ # Third party imports from rest_framework import serializers +import base64 # Module imports from .base import BaseSerializer +from plane.utils.content_validator import ( + validate_binary_data, + validate_html_content, + validate_json_content, +) from plane.db.models import ( Page, PageLog, @@ -186,3 +192,71 @@ class PageVersionDetailSerializer(BaseSerializer): "updated_by", ] read_only_fields = ["workspace", "page"] + + +class PageBinaryUpdateSerializer(serializers.Serializer): + """Serializer for updating page binary description with validation""" + + description_binary = serializers.CharField(required=False, allow_blank=True) + description_html = serializers.CharField(required=False, allow_blank=True) + description = serializers.JSONField(required=False, allow_null=True) + + def validate_description_binary(self, value): + """Validate the base64-encoded binary data""" + if not value: + return value + + try: + # Decode the base64 data + binary_data = base64.b64decode(value) + + # Validate the binary data + is_valid, error_message = validate_binary_data(binary_data) + if not is_valid: + raise serializers.ValidationError( + f"Invalid binary data: {error_message}" + ) + + return binary_data + except Exception as e: + if isinstance(e, serializers.ValidationError): + raise + raise serializers.ValidationError("Failed to decode base64 data") + + def validate_description_html(self, value): + """Validate the HTML content""" + if not value: + return value + + # Use the validation function from utils + is_valid, error_message = validate_html_content(value) + if not is_valid: + raise serializers.ValidationError(error_message) + + return value + + def validate_description(self, value): + """Validate the JSON description""" + if not value: + return value + + # Use the validation function from utils + is_valid, error_message = validate_json_content(value) + if not is_valid: + raise serializers.ValidationError(error_message) + + return value + + def update(self, instance, validated_data): + """Update the page instance with validated data""" + if "description_binary" in validated_data: + instance.description_binary = validated_data.get("description_binary") + + if "description_html" in validated_data: + instance.description_html = validated_data.get("description_html") + + if "description" in validated_data: + instance.description = validated_data.get("description") + + instance.save() + return instance diff --git a/apps/api/plane/app/serializers/project.py b/apps/api/plane/app/serializers/project.py index 813cb068f4..dfa541d9f1 100644 --- a/apps/api/plane/app/serializers/project.py +++ b/apps/api/plane/app/serializers/project.py @@ -13,6 +13,11 @@ from plane.db.models import ( DeployBoard, ProjectPublicMember, ) +from plane.utils.content_validator import ( + validate_html_content, + validate_json_content, + validate_binary_data, +) class ProjectSerializer(BaseSerializer): @@ -58,6 +63,32 @@ class ProjectSerializer(BaseSerializer): return identifier + def validate(self, data): + # Validate description content for security + if "description" in data and data["description"]: + # For Project, description might be text field, not JSON + if isinstance(data["description"], dict): + is_valid, error_msg = validate_json_content(data["description"]) + if not is_valid: + raise serializers.ValidationError({"description": error_msg}) + + if "description_text" in data and data["description_text"]: + is_valid, error_msg = validate_json_content(data["description_text"]) + if not is_valid: + raise serializers.ValidationError({"description_text": error_msg}) + + if "description_html" in data and data["description_html"]: + if isinstance(data["description_html"], dict): + is_valid, error_msg = validate_json_content(data["description_html"]) + else: + is_valid, error_msg = validate_html_content( + str(data["description_html"]) + ) + if not is_valid: + raise serializers.ValidationError({"description_html": error_msg}) + + return data + def create(self, validated_data): workspace_id = self.context["workspace_id"] diff --git a/apps/api/plane/app/serializers/workspace.py b/apps/api/plane/app/serializers/workspace.py index 60866cade2..ec4c4bf63e 100644 --- a/apps/api/plane/app/serializers/workspace.py +++ b/apps/api/plane/app/serializers/workspace.py @@ -24,6 +24,11 @@ from plane.db.models import ( ) from plane.utils.constants import RESTRICTED_WORKSPACE_SLUGS from plane.utils.url import contains_url +from plane.utils.content_validator import ( + validate_html_content, + validate_json_content, + validate_binary_data, +) # Django imports from django.core.validators import URLValidator @@ -312,6 +317,25 @@ class StickySerializer(BaseSerializer): read_only_fields = ["workspace", "owner"] extra_kwargs = {"name": {"required": False}} + def validate(self, data): + # Validate description content for security + if "description" in data and data["description"]: + is_valid, error_msg = validate_json_content(data["description"]) + if not is_valid: + raise serializers.ValidationError({"description": error_msg}) + + if "description_html" in data and data["description_html"]: + is_valid, error_msg = validate_html_content(data["description_html"]) + if not is_valid: + raise serializers.ValidationError({"description_html": error_msg}) + + if "description_binary" in data and data["description_binary"]: + is_valid, error_msg = validate_binary_data(data["description_binary"]) + if not is_valid: + raise serializers.ValidationError({"description_binary": error_msg}) + + return data + class WorkspaceUserPreferenceSerializer(BaseSerializer): class Meta: diff --git a/apps/api/plane/app/views/page/base.py b/apps/api/plane/app/views/page/base.py index cb9b0e0923..96de81abfb 100644 --- a/apps/api/plane/app/views/page/base.py +++ b/apps/api/plane/app/views/page/base.py @@ -25,6 +25,7 @@ from plane.app.serializers import ( PageSerializer, SubPageSerializer, PageDetailSerializer, + PageBinaryUpdateSerializer, ) from plane.db.models import ( Page, @@ -538,32 +539,27 @@ class PagesDescriptionViewSet(BaseViewSet): {"description_html": page.description_html}, cls=DjangoJSONEncoder ) - # Get the base64 data from the request - base64_data = request.data.get("description_binary") - - # If base64 data is provided - if base64_data: - # Decode the base64 data to bytes - new_binary_data = base64.b64decode(base64_data) - # capture the page transaction + # Use serializer for validation and update + serializer = PageBinaryUpdateSerializer(page, data=request.data, partial=True) + if serializer.is_valid(): + # Capture the page transaction if request.data.get("description_html"): page_transaction.delay( new_value=request.data, old_value=existing_instance, page_id=pk ) - # Store the updated binary data - page.description_binary = new_binary_data - page.description_html = request.data.get("description_html") - page.description = request.data.get("description") - page.save() - # Return a success response + + # Update the page using serializer + updated_page = serializer.save() + + # Run background tasks page_version.delay( - page_id=page.id, + page_id=updated_page.id, existing_instance=existing_instance, user_id=request.user.id, ) return Response({"message": "Updated successfully"}) else: - return Response({"error": "No binary data provided"}) + return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST) class PageDuplicateEndpoint(BaseAPIView): diff --git a/apps/api/plane/space/serializer/issue.py b/apps/api/plane/space/serializer/issue.py index e1445b4e63..3549e76262 100644 --- a/apps/api/plane/space/serializer/issue.py +++ b/apps/api/plane/space/serializer/issue.py @@ -28,6 +28,11 @@ from plane.db.models import ( IssueVote, IssueRelation, ) +from plane.utils.content_validator import ( + validate_html_content, + validate_json_content, + validate_binary_data, +) class IssueStateFlatSerializer(BaseSerializer): @@ -283,6 +288,23 @@ class IssueCreateSerializer(BaseSerializer): and data.get("start_date", None) > data.get("target_date", None) ): raise serializers.ValidationError("Start date cannot exceed target date") + + # Validate description content for security + if "description" in data and data["description"]: + is_valid, error_msg = validate_json_content(data["description"]) + if not is_valid: + raise serializers.ValidationError({"description": error_msg}) + + if "description_html" in data and data["description_html"]: + is_valid, error_msg = validate_html_content(data["description_html"]) + if not is_valid: + raise serializers.ValidationError({"description_html": error_msg}) + + if "description_binary" in data and data["description_binary"]: + is_valid, error_msg = validate_binary_data(data["description_binary"]) + if not is_valid: + raise serializers.ValidationError({"description_binary": error_msg}) + return data def create(self, validated_data): diff --git a/apps/api/plane/utils/content_validator.py b/apps/api/plane/utils/content_validator.py new file mode 100644 index 0000000000..7b9932a35b --- /dev/null +++ b/apps/api/plane/utils/content_validator.py @@ -0,0 +1,357 @@ +# Python imports +import base64 +import json +import re + + +# Maximum allowed size for binary data (10MB) +MAX_SIZE = 10 * 1024 * 1024 + +# Maximum recursion depth to prevent stack overflow +MAX_RECURSION_DEPTH = 20 + +# Dangerous text patterns that could indicate XSS or script injection +DANGEROUS_TEXT_PATTERNS = [ + r"]*>.*?", + r"javascript\s*:", + r"data\s*:\s*text/html", + r"eval\s*\(", + r"document\s*\.", + r"window\s*\.", + r"location\s*\.", +] + +# Dangerous attribute patterns for HTML attributes +DANGEROUS_ATTR_PATTERNS = [ + r"javascript\s*:", + r"data\s*:\s*text/html", + r"eval\s*\(", + r"alert\s*\(", + r"document\s*\.", + r"window\s*\.", +] + +# Suspicious patterns for binary data content +SUSPICIOUS_BINARY_PATTERNS = [ + "]*>", + r"", + # JavaScript URLs in various attributes + r'(?:href|src|action)\s*=\s*["\']?\s*javascript:', + # Data URLs with text/html (potential XSS) + r'(?:href|src|action)\s*=\s*["\']?\s*data:text/html', + # Dangerous event handlers with JavaScript-like content + r'on(?:load|error|click|focus|blur|change|submit|reset|select|resize|scroll|unload|beforeunload|hashchange|popstate|storage|message|offline|online)\s*=\s*["\']?[^"\']*(?:javascript|alert|eval|document\.|window\.|location\.|history\.)[^"\']*["\']?', + # Object and embed tags that could load external content + r"<(?:object|embed)[^>]*(?:data|src)\s*=", + # Base tag that could change relative URL resolution + r"]*href\s*=", + # Dangerous iframe sources + r']*src\s*=\s*["\']?(?:javascript:|data:text/html)', + # Meta refresh redirects + r']*http-equiv\s*=\s*["\']?refresh["\']?', + # Link tags - simplified patterns + r']*rel\s*=\s*["\']?stylesheet["\']?', + r']*href\s*=\s*["\']?https?://', + r']*href\s*=\s*["\']?//', + r']*href\s*=\s*["\']?(?:data:|javascript:)', + # Style tags with external imports + r"]*>.*?@import.*?(?:https?://|//)", + # Link tags with dangerous rel types + r']*rel\s*=\s*["\']?(?:import|preload|prefetch|dns-prefetch|preconnect)["\']?', + # Forms with action attributes + r"]*action\s*=", +] + +# Dangerous JavaScript patterns for event handlers +DANGEROUS_JS_PATTERNS = [ + r"alert\s*\(", + r"eval\s*\(", + r"document\s*\.", + r"window\s*\.", + r"location\s*\.", + r"fetch\s*\(", + r"XMLHttpRequest", + r"innerHTML\s*=", + r"outerHTML\s*=", + r"document\.write", + r"script\s*>", +] + +# HTML self-closing tags that don't need closing tags +SELF_CLOSING_TAGS = { + "img", + "br", + "hr", + "input", + "meta", + "link", + "area", + "base", + "col", + "embed", + "source", + "track", + "wbr", +} + + +def validate_binary_data(data): + """ + Validate that binary data appears to be valid document format and doesn't contain malicious content. + + Args: + data (bytes or str): The binary data to validate, or base64-encoded string + + Returns: + tuple: (is_valid: bool, error_message: str or None) + """ + if not data: + return True, None # Empty is OK + + # Handle base64-encoded strings by decoding them first + if isinstance(data, str): + try: + binary_data = base64.b64decode(data) + except Exception: + return False, "Invalid base64 encoding" + else: + binary_data = data + + # Size check - 10MB limit + if len(binary_data) > MAX_SIZE: + return False, "Binary data exceeds maximum size limit (10MB)" + + # Basic format validation + if len(binary_data) < 4: + return False, "Binary data too short to be valid document format" + + # Check for suspicious text patterns (HTML/JS) + try: + decoded_text = binary_data.decode("utf-8", errors="ignore")[:200] + if any( + pattern in decoded_text.lower() for pattern in SUSPICIOUS_BINARY_PATTERNS + ): + return False, "Binary data contains suspicious content patterns" + except Exception: + pass # Binary data might not be decodable as text, which is fine + + return True, None + + +def validate_html_content(html_content): + """ + Validate that HTML content is safe and doesn't contain malicious patterns. + + Args: + html_content (str): The HTML content to validate + + Returns: + tuple: (is_valid: bool, error_message: str or None) + """ + if not html_content: + return True, None # Empty is OK + + # Size check - 10MB limit (consistent with binary validation) + if len(html_content.encode("utf-8")) > MAX_SIZE: + return False, "HTML content exceeds maximum size limit (10MB)" + + # Check for specific malicious patterns (simplified and more reliable) + for pattern in MALICIOUS_HTML_PATTERNS: + if re.search(pattern, html_content, re.IGNORECASE | re.DOTALL): + return ( + False, + f"HTML content contains potentially malicious patterns: {pattern}", + ) + + # Additional check for inline event handlers that contain suspicious content + # This is more permissive - only blocks if the event handler contains actual dangerous code + event_handler_pattern = r'on\w+\s*=\s*["\']([^"\']*)["\']' + event_matches = re.findall(event_handler_pattern, html_content, re.IGNORECASE) + + for handler_content in event_matches: + for js_pattern in DANGEROUS_JS_PATTERNS: + if re.search(js_pattern, handler_content, re.IGNORECASE): + return ( + False, + f"HTML content contains dangerous JavaScript in event handler: {handler_content[:100]}", + ) + + # Basic HTML structure validation - check for common malformed tags + try: + # Count opening and closing tags for basic structure validation + opening_tags = re.findall(r"<(\w+)[^>]*>", html_content) + closing_tags = re.findall(r"", html_content) + + # Filter out self-closing tags from opening tags + opening_tags_filtered = [ + tag for tag in opening_tags if tag.lower() not in SELF_CLOSING_TAGS + ] + + # Basic check - if we have significantly more opening than closing tags, it might be malformed + if len(opening_tags_filtered) > len(closing_tags) + 10: # Allow some tolerance + return False, "HTML content appears to be malformed (unmatched tags)" + + except Exception: + # If HTML parsing fails, we'll allow it + pass + + return True, None + + +def validate_json_content(json_content): + """ + Validate that JSON content is safe and doesn't contain malicious patterns. + + Args: + json_content (dict): The JSON content to validate + + Returns: + tuple: (is_valid: bool, error_message: str or None) + """ + if not json_content: + return True, None # Empty is OK + + try: + # Size check - 10MB limit (consistent with other validations) + json_str = json.dumps(json_content) + if len(json_str.encode("utf-8")) > MAX_SIZE: + return False, "JSON content exceeds maximum size limit (10MB)" + + # Basic structure validation for page description JSON + if isinstance(json_content, dict): + # Check for expected page description structure + # This is based on ProseMirror/Tiptap JSON structure + if "type" in json_content and json_content.get("type") == "doc": + # Valid document structure + if "content" in json_content and isinstance( + json_content["content"], list + ): + # Recursively check content for suspicious patterns + is_valid, error_msg = _validate_json_content_array( + json_content["content"] + ) + if not is_valid: + return False, error_msg + elif "type" not in json_content and "content" not in json_content: + # Allow other JSON structures but validate for suspicious content + is_valid, error_msg = _validate_json_content_recursive(json_content) + if not is_valid: + return False, error_msg + else: + return False, "JSON description must be a valid object" + + except (TypeError, ValueError) as e: + return False, "Invalid JSON structure" + except Exception as e: + return False, "Failed to validate JSON content" + + return True, None + + +def _validate_json_content_array(content, depth=0): + """ + Validate JSON content array for suspicious patterns. + + Args: + content (list): Array of content nodes to validate + depth (int): Current recursion depth (default: 0) + + Returns: + tuple: (is_valid: bool, error_message: str or None) + """ + # Check recursion depth to prevent stack overflow + if depth > MAX_RECURSION_DEPTH: + return False, f"Maximum recursion depth ({MAX_RECURSION_DEPTH}) exceeded" + + if not isinstance(content, list): + return True, None + + for node in content: + if isinstance(node, dict): + # Check text content for suspicious patterns (more targeted) + if node.get("type") == "text" and "text" in node: + text_content = node["text"] + for pattern in DANGEROUS_TEXT_PATTERNS: + if re.search(pattern, text_content, re.IGNORECASE): + return ( + False, + "JSON content contains suspicious script patterns in text", + ) + + # Check attributes for suspicious content (more targeted) + if "attrs" in node and isinstance(node["attrs"], dict): + for attr_name, attr_value in node["attrs"].items(): + if isinstance(attr_value, str): + # Only check specific attributes that could be dangerous + if attr_name.lower() in [ + "href", + "src", + "action", + "onclick", + "onload", + "onerror", + ]: + for pattern in DANGEROUS_ATTR_PATTERNS: + if re.search(pattern, attr_value, re.IGNORECASE): + return ( + False, + f"JSON content contains dangerous pattern in {attr_name} attribute", + ) + + # Recursively check nested content + if "content" in node and isinstance(node["content"], list): + is_valid, error_msg = _validate_json_content_array( + node["content"], depth + 1 + ) + if not is_valid: + return False, error_msg + + return True, None + + +def _validate_json_content_recursive(obj, depth=0): + """ + Recursively validate JSON object for suspicious content. + + Args: + obj: JSON object (dict, list, or primitive) to validate + depth (int): Current recursion depth (default: 0) + + Returns: + tuple: (is_valid: bool, error_message: str or None) + """ + # Check recursion depth to prevent stack overflow + if depth > MAX_RECURSION_DEPTH: + return False, f"Maximum recursion depth ({MAX_RECURSION_DEPTH}) exceeded" + if isinstance(obj, dict): + for key, value in obj.items(): + if isinstance(value, str): + # Check for dangerous patterns using module constants + for pattern in DANGEROUS_TEXT_PATTERNS: + if re.search(pattern, value, re.IGNORECASE): + return ( + False, + "JSON content contains suspicious script patterns", + ) + elif isinstance(value, (dict, list)): + is_valid, error_msg = _validate_json_content_recursive(value, depth + 1) + if not is_valid: + return False, error_msg + elif isinstance(obj, list): + for item in obj: + is_valid, error_msg = _validate_json_content_recursive(item, depth + 1) + if not is_valid: + return False, error_msg + + return True, None From d4705e16497e6dd4b1ab5da72ec5dd6898f60bc4 Mon Sep 17 00:00:00 2001 From: Bavisetti Narayan <72156168+NarayanBavisetti@users.noreply.github.com> Date: Fri, 25 Jul 2025 16:50:35 +0530 Subject: [PATCH 2/2] [WEB-4544] chore: added field validations in serializer (#7460) * chore: added field validations in serializer * chore: added enum for roles --- apps/api/plane/api/serializers/issue.py | 17 ++++- apps/api/plane/app/serializers/draft.py | 90 ++++++++++++++++++++----- apps/api/plane/app/serializers/issue.py | 71 +++++++++++++++---- 3 files changed, 148 insertions(+), 30 deletions(-) diff --git a/apps/api/plane/api/serializers/issue.py b/apps/api/plane/api/serializers/issue.py index c57f9d3597..20f967e3be 100644 --- a/apps/api/plane/api/serializers/issue.py +++ b/apps/api/plane/api/serializers/issue.py @@ -20,6 +20,7 @@ from plane.db.models import ( ProjectMember, State, User, + EstimatePoint, ) from plane.utils.content_validator import ( validate_html_content, @@ -126,13 +127,27 @@ class IssueSerializer(BaseSerializer): if ( data.get("parent") and not Issue.objects.filter( - workspace_id=self.context.get("workspace_id"), pk=data.get("parent").id + workspace_id=self.context.get("workspace_id"), + project_id=self.context.get("project_id"), + pk=data.get("parent").id, ).exists() ): raise serializers.ValidationError( "Parent is not valid issue_id please pass a valid issue_id" ) + if ( + data.get("estimate_point") + and not EstimatePoint.objects.filter( + workspace_id=self.context.get("workspace_id"), + project_id=self.context.get("project_id"), + pk=data.get("estimate_point").id, + ).exists() + ): + raise serializers.ValidationError( + "Estimate point is not valid please pass a valid estimate_point_id" + ) + return data def create(self, validated_data): diff --git a/apps/api/plane/app/serializers/draft.py b/apps/api/plane/app/serializers/draft.py index 6479d44c83..57600bff93 100644 --- a/apps/api/plane/app/serializers/draft.py +++ b/apps/api/plane/app/serializers/draft.py @@ -16,12 +16,15 @@ from plane.db.models import ( DraftIssueLabel, DraftIssueCycle, DraftIssueModule, + ProjectMember, + EstimatePoint, ) from plane.utils.content_validator import ( validate_html_content, validate_json_content, validate_binary_data, ) +from plane.app.permissions import ROLE class DraftIssueCreateSerializer(BaseSerializer): @@ -62,31 +65,84 @@ class DraftIssueCreateSerializer(BaseSerializer): data["label_ids"] = label_ids if label_ids else [] return data - def validate(self, data): + def validate(self, attrs): if ( - data.get("start_date", None) is not None - and data.get("target_date", None) is not None - and data.get("start_date", None) > data.get("target_date", None) + attrs.get("start_date", None) is not None + and attrs.get("target_date", None) is not None + and attrs.get("start_date", None) > attrs.get("target_date", None) ): raise serializers.ValidationError("Start date cannot exceed target date") # Validate description content for security - if "description" in data and data["description"]: - is_valid, error_msg = validate_json_content(data["description"]) + if "description" in attrs and attrs["description"]: + is_valid, error_msg = validate_json_content(attrs["description"]) if not is_valid: raise serializers.ValidationError({"description": error_msg}) - if "description_html" in data and data["description_html"]: - is_valid, error_msg = validate_html_content(data["description_html"]) + if "description_html" in attrs and attrs["description_html"]: + is_valid, error_msg = validate_html_content(attrs["description_html"]) if not is_valid: raise serializers.ValidationError({"description_html": error_msg}) - if "description_binary" in data and data["description_binary"]: - is_valid, error_msg = validate_binary_data(data["description_binary"]) + if "description_binary" in attrs and attrs["description_binary"]: + is_valid, error_msg = validate_binary_data(attrs["description_binary"]) if not is_valid: raise serializers.ValidationError({"description_binary": error_msg}) - return data + # Validate assignees are from project + if attrs.get("assignee_ids", []): + attrs["assignee_ids"] = ProjectMember.objects.filter( + project_id=self.context["project_id"], + role__gte=ROLE.MEMBER.value, + is_active=True, + member_id__in=attrs["assignee_ids"], + ).values_list("member_id", flat=True) + + # Validate labels are from project + if attrs.get("label_ids"): + label_ids = [label.id for label in attrs["label_ids"]] + attrs["label_ids"] = list( + Label.objects.filter( + project_id=self.context.get("project_id"), id__in=label_ids + ).values_list("id", flat=True) + ) + + # # Check state is from the project only else raise validation error + if ( + attrs.get("state") + and not State.objects.filter( + project_id=self.context.get("project_id"), + pk=attrs.get("state").id, + ).exists() + ): + raise serializers.ValidationError( + "State is not valid please pass a valid state_id" + ) + + # # Check parent issue is from workspace as it can be cross workspace + if ( + attrs.get("parent") + and not Issue.objects.filter( + project_id=self.context.get("project_id"), + pk=attrs.get("parent").id, + ).exists() + ): + raise serializers.ValidationError( + "Parent is not valid issue_id please pass a valid issue_id" + ) + + if ( + attrs.get("estimate_point") + and not EstimatePoint.objects.filter( + project_id=self.context.get("project_id"), + pk=attrs.get("estimate_point").id, + ).exists() + ): + raise serializers.ValidationError( + "Estimate point is not valid please pass a valid estimate_point_id" + ) + + return attrs def create(self, validated_data): assignees = validated_data.pop("assignee_ids", None) @@ -111,14 +167,14 @@ class DraftIssueCreateSerializer(BaseSerializer): DraftIssueAssignee.objects.bulk_create( [ DraftIssueAssignee( - assignee=user, + assignee_id=assignee_id, draft_issue=issue, workspace_id=workspace_id, project_id=project_id, created_by_id=created_by_id, updated_by_id=updated_by_id, ) - for user in assignees + for assignee_id in assignees ], batch_size=10, ) @@ -127,14 +183,14 @@ class DraftIssueCreateSerializer(BaseSerializer): DraftIssueLabel.objects.bulk_create( [ DraftIssueLabel( - label=label, + label_id=label_id, draft_issue=issue, project_id=project_id, workspace_id=workspace_id, created_by_id=created_by_id, updated_by_id=updated_by_id, ) - for label in labels + for label_id in labels ], batch_size=10, ) @@ -185,14 +241,14 @@ class DraftIssueCreateSerializer(BaseSerializer): DraftIssueAssignee.objects.bulk_create( [ DraftIssueAssignee( - assignee=user, + assignee_id=assignee_id, draft_issue=instance, workspace_id=workspace_id, project_id=project_id, created_by_id=created_by_id, updated_by_id=updated_by_id, ) - for user in assignees + for assignee_id in assignees ], batch_size=10, ) diff --git a/apps/api/plane/app/serializers/issue.py b/apps/api/plane/app/serializers/issue.py index 98ceaaad6c..8973264313 100644 --- a/apps/api/plane/app/serializers/issue.py +++ b/apps/api/plane/app/serializers/issue.py @@ -37,6 +37,7 @@ from plane.db.models import ( IssueVersion, IssueDescriptionVersion, ProjectMember, + EstimatePoint, ) from plane.utils.content_validator import ( validate_html_content, @@ -124,14 +125,6 @@ class IssueCreateSerializer(BaseSerializer): ): raise serializers.ValidationError("Start date cannot exceed target date") - if attrs.get("assignee_ids", []): - attrs["assignee_ids"] = ProjectMember.objects.filter( - project_id=self.context["project_id"], - role__gte=15, - is_active=True, - member_id__in=attrs["assignee_ids"], - ).values_list("member_id", flat=True) - # Validate description content for security if "description" in attrs and attrs["description"]: is_valid, error_msg = validate_json_content(attrs["description"]) @@ -148,6 +141,60 @@ class IssueCreateSerializer(BaseSerializer): if not is_valid: raise serializers.ValidationError({"description_binary": error_msg}) + # Validate assignees are from project + if attrs.get("assignee_ids", []): + attrs["assignee_ids"] = ProjectMember.objects.filter( + project_id=self.context["project_id"], + role__gte=15, + is_active=True, + member_id__in=attrs["assignee_ids"], + ).values_list("member_id", flat=True) + + # Validate labels are from project + if attrs.get("label_ids"): + label_ids = [label.id for label in attrs["label_ids"]] + attrs["label_ids"] = list( + Label.objects.filter( + project_id=self.context.get("project_id"), + id__in=label_ids, + ).values_list("id", flat=True) + ) + + # Check state is from the project only else raise validation error + if ( + attrs.get("state") + and not State.objects.filter( + project_id=self.context.get("project_id"), + pk=attrs.get("state").id, + ).exists() + ): + raise serializers.ValidationError( + "State is not valid please pass a valid state_id" + ) + + # Check parent issue is from workspace as it can be cross workspace + if ( + attrs.get("parent") + and not Issue.objects.filter( + project_id=self.context.get("project_id"), + pk=attrs.get("parent").id, + ).exists() + ): + raise serializers.ValidationError( + "Parent is not valid issue_id please pass a valid issue_id" + ) + + if ( + attrs.get("estimate_point") + and not EstimatePoint.objects.filter( + project_id=self.context.get("project_id"), + pk=attrs.get("estimate_point").id, + ).exists() + ): + raise serializers.ValidationError( + "Estimate point is not valid please pass a valid estimate_point_id" + ) + return attrs def create(self, validated_data): @@ -211,14 +258,14 @@ class IssueCreateSerializer(BaseSerializer): IssueLabel.objects.bulk_create( [ IssueLabel( - label=label, + label_id=label_id, issue=issue, project_id=project_id, workspace_id=workspace_id, created_by_id=created_by_id, updated_by_id=updated_by_id, ) - for label in labels + for label_id in labels ], batch_size=10, ) @@ -264,14 +311,14 @@ class IssueCreateSerializer(BaseSerializer): IssueLabel.objects.bulk_create( [ IssueLabel( - label=label, + label_id=label_id, issue=instance, project_id=project_id, workspace_id=workspace_id, created_by_id=created_by_id, updated_by_id=updated_by_id, ) - for label in labels + for label_id in labels ], batch_size=10, ignore_conflicts=True,