-
Notifications
You must be signed in to change notification settings - Fork 29
Description
Problem / Motivation
After thoroughly analyzing the TIX CLI codebase, I've identified several areas where input validation and error handling could be significantly improved to enhance robustness, user experience, and security. Currently, the application has basic validation in some commands but lacks comprehensive input sanitization and consistent error handling patterns across the codebase.
Specific Issues Identified:
-
Missing Validation in File Operations (
cli.pylines 200-220):- The
addandeditcommands accept file paths via--attachflag without thorough validation - No check for file size limits (could lead to storage issues)
- Limited validation on file types (potential security concern)
- Path traversal vulnerability potential with
Path(file_path).expanduser().resolve()
- The
-
Inconsistent Error Messages:
- Different error formats across commands (some use console.print with [red], others use click.Abort())
- Missing user-friendly error messages for edge cases
- No error codes or standardized error structure
-
Priority/Tag Input Validation:
- Tag names aren't validated (could contain special characters causing JSON serialization issues)
- No limit on tag length or number of tags per task
- Priority values are validated by Click but error messages could be more helpful
-
URL Validation for Links (
--linkoption):- Links are added without URL validation
- No protocol verification (http/https)
- Could accept malformed URLs that fail when opened via
tix open
-
Task ID Boundary Validation:
- No validation for negative task IDs in some commands
- Missing checks for task ID overflow scenarios
Proposed Solution
Implement a comprehensive input validation and error handling system with the following components:
1. Create a Validation Module (tix/utils/validators.py)
"""Input validation utilities for TIX CLI"""
import re
from pathlib import Path
from typing import Optional, Tuple
from urllib.parse import urlparse
class ValidationError(Exception):
"""Custom exception for validation errors"""
def __init__(self, message: str, field: str = None):
self.message = message
self.field = field
super().__init__(self.message)
class Validators:
MAX_TAG_LENGTH = 50
MAX_TAGS_PER_TASK = 20
MAX_FILE_SIZE_MB = 50
ALLOWED_FILE_EXTENSIONS = {'.txt', '.pdf', '.md', '.docx', '.xlsx', '.png', '.jpg', '.jpeg'}
@staticmethod
def validate_task_text(text: str) -> Tuple[bool, Optional[str]]:
"""Validate task text input"""
if not text or not text.strip():
return False, "Task text cannot be empty"
if len(text) > 500:
return False, "Task text too long (max 500 characters)"
return True, None
@staticmethod
def validate_tag(tag: str) -> Tuple[bool, Optional[str]]:
"""Validate tag name"""
if not tag or not tag.strip():
return False, "Tag cannot be empty"
if len(tag) > Validators.MAX_TAG_LENGTH:
return False, f"Tag too long (max {Validators.MAX_TAG_LENGTH} characters)"
if not re.match(r'^[a-zA-Z0-9_-]+$', tag):
return False, "Tag can only contain letters, numbers, hyphens, and underscores"
return True, None
@staticmethod
def validate_file_attachment(file_path: str) -> Tuple[bool, Optional[str]]:
"""Validate file attachment"""
try:
path = Path(file_path).expanduser().resolve()
# Check if file exists
if not path.exists():
return False, f"File not found: {file_path}"
# Check if it's a file (not directory)
if not path.is_file():
return False, f"Path is not a file: {file_path}"
# Check file size
size_mb = path.stat().st_size / (1024 * 1024)
if size_mb > Validators.MAX_FILE_SIZE_MB:
return False, f"File too large: {size_mb:.1f}MB (max {Validators.MAX_FILE_SIZE_MB}MB)"
# Check file extension
if path.suffix.lower() not in Validators.ALLOWED_FILE_EXTENSIONS:
return False, f"File type not allowed: {path.suffix}. Allowed: {', '.join(Validators.ALLOWED_FILE_EXTENSIONS)}"
return True, None
except Exception as e:
return False, f"Error validating file: {str(e)}"
@staticmethod
def validate_url(url: str) -> Tuple[bool, Optional[str]]:
"""Validate URL for links"""
try:
result = urlparse(url)
if not all([result.scheme, result.netloc]):
return False, "Invalid URL format (must include protocol and domain)"
if result.scheme not in ['http', 'https']:
return False, "URL must use http or https protocol"
return True, None
except Exception:
return False, "Malformed URL"
@staticmethod
def validate_task_id(task_id: int, max_id: int = None) -> Tuple[bool, Optional[str]]:
"""Validate task ID"""
if task_id < 1:
return False, "Task ID must be positive"
if max_id and task_id > max_id:
return False, f"Task ID {task_id} exceeds maximum ({max_id})"
return True, None2. Create Error Handling Utilities (tix/utils/errors.py)
"""Centralized error handling for TIX CLI"""
from rich.console import Console
from enum import Enum
console = Console()
class ErrorCode(Enum):
"""Error codes for different types of errors"""
VALIDATION_ERROR = "VAL001"
FILE_NOT_FOUND = "FILE001"
STORAGE_ERROR = "STOR001"
PERMISSION_ERROR = "PERM001"
INVALID_INPUT = "INP001"
class ErrorHandler:
"""Centralized error handling"""
@staticmethod
def handle_validation_error(field: str, message: str, code: ErrorCode = ErrorCode.VALIDATION_ERROR):
"""Handle validation errors with consistent formatting"""
console.print(f"[red]✗ Validation Error[/red] [{code.value}]: {message}")
if field:
console.print(f"[dim] Field: {field}[/dim]")
@staticmethod
def handle_error(message: str, code: ErrorCode, details: str = None):
"""Handle general errors"""
console.print(f"[red]✗ Error[/red] [{code.value}]: {message}")
if details:
console.print(f"[dim] {details}[/dim]")3. Integration into Existing Commands
Update the add command in cli.py:
@cli.command()
@click.argument('task')
@click.option('--priority', '-p', default='medium',
type=click.Choice(['low', 'medium', 'high']),
help='Set task priority')
@click.option('--tag', '-t', multiple=True, help='Add tags to task')
@click.option('--attach', '-f', multiple=True, help='Attach file(s)')
@click.option('--link', '-l', multiple=True, help='Attach URL(s)')
def add(task, priority, tag, attach, link):
"""Add a new task with comprehensive validation"""
from tix.config import CONFIG
from tix.utils.validators import Validators
from tix.utils.errors import ErrorHandler, ErrorCode
# Validate task text
is_valid, error_msg = Validators.validate_task_text(task)
if not is_valid:
ErrorHandler.handle_validation_error('task', error_msg)
sys.exit(1)
# Validate and sanitize tags
validated_tags = []
default_tags = CONFIG.get('defaults', {}).get('tags', [])
all_tags = list(default_tags) + list(tag)
for t in all_tags:
is_valid, error_msg = Validators.validate_tag(t)
if not is_valid:
ErrorHandler.handle_validation_error('tag', f"{error_msg}: '{t}'")
console.print("[yellow]⚠ Skipping invalid tag[/yellow]")
continue
validated_tags.append(t)
# Check tag limit
if len(validated_tags) > Validators.MAX_TAGS_PER_TASK:
ErrorHandler.handle_validation_error(
'tags',
f"Too many tags (max {Validators.MAX_TAGS_PER_TASK})"
)
sys.exit(1)
validated_tags = list(dict.fromkeys(validated_tags)) # Remove duplicates
# Create task
new_task = storage.add_task(task.strip(), priority, validated_tags)
# Validate and attach files
if attach:
attachment_dir = Path.home() / ".tix" / "attachments" / str(new_task.id)
attachment_dir.mkdir(parents=True, exist_ok=True)
for file_path in attach:
is_valid, error_msg = Validators.validate_file_attachment(file_path)
if not is_valid:
ErrorHandler.handle_validation_error('attachment', error_msg)
continue
try:
src = Path(file_path).expanduser().resolve()
dest = attachment_dir / src.name
dest.write_bytes(src.read_bytes())
new_task.attachments.append(str(dest))
console.print(f"[green]✔[/green] Attached: {src.name}")
except Exception as e:
ErrorHandler.handle_error(
f"Failed to attach {file_path}",
ErrorCode.FILE_NOT_FOUND,
str(e)
)
# Validate and add links
if link:
if not hasattr(new_task, "links"):
new_task.links = []
for url in link:
is_valid, error_msg = Validators.validate_url(url)
if not is_valid:
ErrorHandler.handle_validation_error('link', f"{error_msg}: {url}")
continue
new_task.links.append(url)
console.print(f"[green]✔[/green] Added link: {url}")
storage.update_task(new_task, record_history=False)
color = {'high': 'red', 'medium': 'yellow', 'low': 'green'}[priority]
console.print(f"\n[green]✔[/green] Task #{new_task.id} created: [{color}]{task}[/{color}]")
if validated_tags:
console.print(f"[dim] Tags: {', '.join(validated_tags)}[/dim]")Alternatives Considered
- Using Pydantic for Validation: Would add another dependency but provide more robust validation
- Click's Built-in Validation: Limited in scope and less flexible for custom rules
- Decorator-based Validation: Could work but less transparent for users
Acceptance Criteria
- Create
tix/utils/validators.pywith comprehensive validation functions - Create
tix/utils/errors.pywith centralized error handling - Update
addcommand with full validation - Update
editcommand with validation - Update
searchcommand to validate query length - Add validation for
movecommand (check source/destination IDs) - All validation errors should have consistent formatting
- Write comprehensive unit tests for all validators (minimum 90% coverage)
- Write integration tests for error scenarios
- Update documentation with:
- List of error codes and their meanings
- Input constraints for each command
- Examples of validation errors
- No breaking changes to existing API
- All existing tests must pass
- Code follows project style guidelines (flake8, black)
Implementation Guidelines
For Contributors:
- Branch naming:
feature/input-validation-enhancement - Commit convention: Use conventional commits (feat:, test:, docs:)
- Testing: Add tests for each validator before implementation
- Documentation: Update README with new error codes section
- PR checklist:
- All tests passing
- Code coverage maintained above 85%
- Documentation updated
- Examples provided
Estimated Complexity: Medium-High (10-15 hours)
Good First Issue for: Contributors comfortable with Python, input validation, and error handling patterns
Labels: enhancement, good first issue, hacktoberfest, testing, security
Additional Context
This enhancement will:
- Improve user experience with clear, actionable error messages
- Prevent potential security issues (path traversal, file size attacks)
- Make the codebase more maintainable with centralized validation
- Provide better debugging with error codes
- Ensure data integrity in storage layer
Related Issues: #75 (testing improvements), #20 (performance - validation can prevent bad data)
Priority: High (security + UX improvement)