feat(astrolabe): replace PDF.js with server-side PyMuPDF rendering

[cbcoutinho]

Summary

Replace client-side PDF.js with server-side PyMuPDF rendering for the Astrolabe PDF chunk viewer. This addresses CSP worker restrictions and ES private field access issues that affected Chromium browsers.

Changes

Backend (Python MCP Server)

• Add /api/v1/pdf-preview endpoint that renders PDF pages to PNG using PyMuPDF
• Download PDF via WebDAV using user's OAuth token
• Return base64-encoded PNG with page metadata

Backend (PHP Astrolabe App)

• Add pdfPreview controller action with OAuth token handling
• Add getPdfPreview service method to call MCP server
• Add route /api/pdf-preview
• Fix Psalm type errors with proper annotations and strict comparisons

Frontend (Vue)

• Refactor PDFViewer.vue to display server-rendered PNG images
• Use @nextcloud/axios for CSRF token handling
• Remove pdfjs-dist dependency entirely

Tests

• Add comprehensive unit tests for PDF preview endpoint
• Cover parameter validation, authentication, rendering, and edge cases

Why server-side rendering?

PDF.js has issues in Nextcloud's environment:

1. CSP restrictions prevent web workers from loading
2. ES private field access causes errors in some Chromium versions
3. External loading of pdfjs-dist introduced complexity

Server-side rendering with PyMuPDF:

• Works consistently across all browsers
• No CSP or worker issues
• Simpler frontend code
• Leverages existing OAuth/WebDAV infrastructure

Testing

• Verified in Firefox and Chromium
• All existing tests pass
• New unit tests for PDF preview endpoint

Supersedes #441 (original plotly.js update branch)

---

This PR was generated with the help of AI, and reviewed by a Human

[claude]

Code Review for PR #501: Server-side PDF Rendering with PyMuPDF

Summary

This PR successfully replaces client-side PDF.js rendering with server-side PyMuPDF rendering to address CSP worker restrictions and ES private field access issues in Chromium browsers. The implementation is well-structured and follows the project's architectural patterns.

✅ Strengths

Architecture & Design

• Clean separation of concerns: Backend endpoint (Python) → PHP controller → Vue frontend
• Follows ADR-018 pattern: Uses OAuth bearer tokens for authentication consistently
• Proper error handling: Comprehensive error cases with user-friendly messages
• Security-first approach: Uses user's OAuth token for WebDAV access, preventing unauthorized file access

Code Quality

• Excellent test coverage: Comprehensive unit tests covering parameter validation, authentication, rendering, and edge cases
• Type safety: PHP code uses strict types and proper annotations (Psalm-compliant)
• Good documentation: Clear docstrings and inline comments explaining the rationale
• Follows coding conventions: Matches CLAUDE.md standards for async/await, type hints, and error handling

Implementation Details

• Parameter validation: Proper bounds checking (page: 1-10000, scale: 0.5-5.0)
• Resource cleanup: doc.close() called even in error paths
• Efficient base64 encoding: Appropriate for image data transfer
• Reuses existing infrastructure: Leverages WebDAV client and token management

🔍 Issues & Concerns

1. Resource Management - Memory Safety ⚠️

Location: nextcloud_mcp_server/api/management.py:1914-1932

The PyMuPDF document object is opened and closed manually, which could leak resources if an exception occurs between open() and close().

Issue:

doc = pymupdf.open(stream=pdf_bytes, filetype="pdf")
total_pages = doc.page_count

# Validate page number
if page_num > total_pages:
    doc.close()  # Good: explicitly closed
    return JSONResponse(...)

page = doc[page_num - 1]
mat = pymupdf.Matrix(scale, scale)
pix = page.get_pixmap(matrix=mat, alpha=False)  # ⚠️ Could raise exception
png_bytes = pix.tobytes("png")  # ⚠️ Could raise exception
doc.close()

Recommendation: Use a context manager or try/finally block:

doc = pymupdf.open(stream=pdf_bytes, filetype="pdf")
try:
    total_pages = doc.page_count
    if page_num > total_pages:
        return JSONResponse(...)
    
    page = doc[page_num - 1]
    mat = pymupdf.Matrix(scale, scale)
    pix = page.get_pixmap(matrix=mat, alpha=False)
    png_bytes = pix.tobytes("png")
finally:
    doc.close()

2. Memory Consumption - Potential DoS ⚠️

Location: nextcloud_mcp_server/api/management.py:1909

The endpoint downloads the entire PDF into memory before rendering. Large PDFs (hundreds of MBs) could exhaust server memory, especially under concurrent requests.

Issue:

pdf_bytes, _ = await nc_client.webdav.read_file(file_path)
# No size limit check before loading into memory

Recommendations:

1. Add a file size check before downloading (e.g., max 50MB)
2. Consider streaming if WebDAV client supports it
3. Add rate limiting per user to prevent abuse
4. Document memory requirements in deployment docs

3. Error Sanitization - Information Leakage Risk ℹ️

Location: nextcloud_mcp_server/api/management.py:1951-1963

The generic error handler uses _sanitize_error_for_client(), but FileNotFoundError is handled separately. This is correct, but consider if PyMuPDF-specific errors should also have explicit handling.

Current behavior:

• FileNotFoundError: Returns "PDF file not found" (good)
• PyMuPDF parsing error: Returns generic "An internal error occurred" (safe but not helpful)

Suggestion: Add specific handling for common PyMuPDF exceptions:

except pymupdf.FileDataError:
    return JSONResponse(
        {"success": False, "error": "Invalid or corrupted PDF file"},
        status_code=400,
    )

4. PHP Type Safety ✅ (Minor)

Location: third_party/astrolabe/lib/Controller/ApiController.php:813

The pdfPreview method parameters have proper types, but the method could benefit from more detailed PHPDoc for the return type structure.

Current:

/**
 * @return JSONResponse
 */
public function pdfPreview(string $file_path, int $page = 1, float $scale = 2.0): JSONResponse

Suggested enhancement:

/**
 * @return JSONResponse<array{success: bool, image?: string, page_number?: int, total_pages?: int, error?: string}>
 */

5. Vue Component - Scale Prop ℹ️

Location: third_party/astrolabe/src/components/PDFViewer.vue:47-50

The scale prop defaults to 2.0 (144 DPI), which may be too high for mobile devices or slow connections. Consider making it responsive.

Suggestion: Add prop validation or adjust based on viewport:

scale: {
    type: Number,
    default: 2.0,
    validator: (value) => value >= 0.5 && value <= 5.0
}

🔒 Security Assessment

✅ Positive Security Aspects

1. OAuth token validation: Properly validates bearer tokens before processing
2. User-scoped access: Uses user's own token for WebDAV, preventing cross-user access
3. Input validation: Page and scale parameters properly bounded
4. Error sanitization: Generic errors don't leak internal details

⚠️ Security Considerations

1. File path validation: The file_path parameter is not validated for path traversal attacks. However, this is mitigated by:

   • WebDAV client likely handles path normalization
   • User's OAuth token restricts access to their own files
   • Still recommended to validate path doesn't contain ../ sequences

2. Rate limiting: No explicit rate limiting on this endpoint. Consider adding to prevent:

   • Resource exhaustion attacks (large PDFs)
   • Expensive rendering operations

📊 Performance Considerations

Current Implementation

• Memory usage: O(file_size + rendered_image_size) per request
• CPU usage: PyMuPDF rendering is CPU-intensive, especially at high scales
• Network: Base64 encoding adds ~33% overhead to image size

Recommendations

1. Caching: Consider caching rendered pages (file_path + page + scale as key)
2. Pagination: Default scale could be adaptive based on viewport
3. Compression: Consider using WebP instead of PNG for smaller payloads
4. Load shedding: Add circuit breaker if PDF rendering becomes a bottleneck

✅ Test Coverage

Excellent test coverage with proper mocking:

• ✅ Parameter validation (missing, invalid, out of range)
• ✅ Authentication (missing token, invalid token)
• ✅ Successful rendering with PNG verification
• ✅ Error cases (file not found, page out of range)
• ✅ Edge cases (URL-encoded paths, boundary values)

Suggestion: Add integration test that:

1. Uploads a real PDF via WebDAV
2. Requests preview via the endpoint
3. Verifies rendered image dimensions match expected values

📝 Documentation

✅ Good

• Clear PR description explaining the rationale
• Inline comments in complex sections
• Proper docstrings with parameter descriptions

Suggestions

1. Update user-facing docs to mention PDF preview capability
2. Document memory requirements for deployment
3. Add troubleshooting section for common PyMuPDF errors

🎯 Recommendations

Must Fix (Before Merge)

1. Add try/finally for doc.close() to prevent resource leaks

Should Fix (High Priority)

1. Add file size limit check (e.g., 50MB max)
2. Validate file_path doesn't contain ../ sequences
3. Add specific exception handling for PyMuPDF errors

Nice to Have

1. Add caching for frequently accessed PDF pages
2. Make scale responsive to viewport size
3. Add integration test with real PDF file
4. Consider adding rate limiting per user

🏁 Conclusion

This is a well-implemented solution that effectively addresses the PDF.js issues. The code follows project conventions, has excellent test coverage, and uses appropriate security measures. The main concern is resource management (memory leaks and DoS potential), which should be addressed before merging.

Recommendation: ✅ Approve with minor changes (add try/finally block and file size limit)

Great work on this PR! The architectural approach is sound and the implementation quality is high. 🎉

[claude]

Code Review - PR #501: Replace PDF.js with Server-Side PyMuPDF Rendering

This is a well-executed architectural change that solves real browser compatibility issues. The implementation is solid, but I have some suggestions for improvement.

---

✅ Strengths

1. Excellent Problem-Solution Alignment

• Clear motivation: CSP worker restrictions and ES private field issues in Chromium
• Server-side rendering eliminates client-side complexity
• Leverages existing OAuth/WebDAV infrastructure effectively

2. Comprehensive Testing

• 716 lines of unit tests for the PDF preview endpoint (test_management_pdf_preview_endpoint.py)
• Tests cover: parameter validation, authentication, rendering, edge cases, and security
• Good use of mocked PDF bytes for fast, reliable testing
• Security tests include path traversal, file size limits, and corrupted PDFs

3. Good Code Organization

• Proper separation: Python MCP server endpoint → PHP API controller → Vue component
• Clean refactoring of management.py into domain-focused modules (commit 2e77746)
• Follows repository conventions from CLAUDE.md

4. Security Considerations

• OAuth token authentication required
• Path traversal validation (.. check at line 709)
• File size limit (50 MB at line 746)
• Proper error handling for corrupted/invalid PDFs

---

🔧 Issues & Recommendations

1. Security: Path Traversal Validation is Insufficient ⚠️

Issue: The path traversal check at visualization.py:709 only checks for ".." in the file path:

if ".." in file_path:
    return JSONResponse({"success": False, "error": "Invalid file path"}, status_code=400)

Problem: This can be bypassed with URL encoding (e.g., %2e%2e), double encoding, or Unicode variations. While WebDAV may normalize paths, defense-in-depth is important.

Recommendation: Use Python's pathlib to resolve and validate paths:

from pathlib import Path

# Normalize and check for traversal
try:
    # Remove leading slash for Path() to work correctly
    normalized_path = Path(file_path.lstrip('/'))
    # Check if '..' appears in any path components after normalization
    if '..' in normalized_path.parts:
        return JSONResponse({"success": False, "error": "Invalid file path"}, status_code=400)
except (ValueError, OSError):
    return JSONResponse({"success": False, "error": "Invalid file path"}, status_code=400)

Reference: Your own test at line 575 includes URL-encoded path traversal (/folder/..%2F..%2Fetc/passwd), which suggests you considered this attack vector.

---

2. Performance: Missing Rate Limiting

Issue: No rate limiting on the PDF preview endpoint. Rendering PDFs is CPU-intensive (PyMuPDF operations), and an attacker could exhaust server resources by requesting many large PDFs.

Impact:

• Each request downloads entire PDF via WebDAV
• PyMuPDF rendering is CPU-bound
• High-scale factor (up to 5.0) increases rendering time
• No concurrent request limits

Recommendation: Add rate limiting using existing decorators or implement a simple token bucket:

# In visualization.py or a shared module
from functools import wraps
from collections import defaultdict
import time

_rate_limits = defaultdict(list)  # user_id -> [timestamp, timestamp, ...]

def rate_limit(max_requests: int, window_seconds: int):
    """Rate limit decorator: max_requests per window_seconds per user."""
    def decorator(func):
        @wraps(func)
        async def wrapper(request: Request, *args, **kwargs):
            user_id = "unknown"
            try:
                user_id, _ = await validate_token_and_get_user(request)
            except Exception:
                pass  # Will be caught by auth check in endpoint
            
            now = time.time()
            user_requests = _rate_limits[user_id]
            
            # Remove old requests outside the window
            user_requests[:] = [t for t in user_requests if now - t < window_seconds]
            
            if len(user_requests) >= max_requests:
                return JSONResponse(
                    {"success": False, "error": "Rate limit exceeded. Please try again later."},
                    status_code=429
                )
            
            user_requests.append(now)
            return await func(request, *args, **kwargs)
        return wrapper
    return decorator

# Apply to endpoint
@rate_limit(max_requests=30, window_seconds=60)  # 30 requests per minute
async def get_pdf_preview(request: Request) -> JSONResponse:
    ...

Alternative: Use existing @retry_on_429 pattern from base_client.py or add FastAPI middleware for rate limiting.

---

3. Error Handling: Overly Generic in PHP Controller

Issue: The ApiController::pdfPreview() method returns generic 500 errors for all failures:

if (isset($result['error'])) {
    return new JSONResponse(['success' => false, 'error' => $result['error']], Http::STATUS_INTERNAL_SERVER_ERROR);
}

Problem: Different errors should return different HTTP status codes:

• 404 for file not found
• 413 for file too large
• 400 for invalid parameters
• 401 for auth failures

Recommendation: Parse the Python response and map errors to appropriate status codes:

if (isset($result['error'])) {
    $error = $result['error'];
    $status = Http::STATUS_INTERNAL_SERVER_ERROR;
    
    // Map error messages to appropriate HTTP status codes
    if (stripos($error, 'not found') !== false) {
        $status = Http::STATUS_NOT_FOUND;
    } elseif (stripos($error, 'size limit') !== false) {
        $status = Http::STATUS_REQUEST_ENTITY_TOO_LARGE;
    } elseif (stripos($error, 'invalid') !== false || stripos($error, 'corrupted') !== false) {
        $status = Http::STATUS_BAD_REQUEST;
    } elseif (stripos($error, 'unauthorized') !== false) {
        $status = Http::STATUS_UNAUTHORIZED;
    }
    
    return new JSONResponse(['success' => false, 'error' => $error], $status);
}

This helps clients (and the Vue component at line 101-104) provide better user feedback.

---

4. Code Quality: Type Annotations in Refresh Callback

Good Fix: Commit c09ebe9 added proper return type annotations to refresh callbacks:

/** @return array{access_token: string, refresh_token: string, expires_in: int}|null */
$refreshCallback = function (string $refreshToken): ?array {
    // ...
};

Issue: The type annotation is incomplete - it doesn't specify the function signature.

Recommendation: Use full closure type annotation:

/** @var callable(string): array{access_token: string, refresh_token: string, expires_in: int}|null */
$refreshCallback = function (string $refreshToken): ?array {
    // ...
};

This satisfies Psalm static analysis more completely.

---

5. Performance: Consider Caching Rendered Pages

Observation: Each request re-downloads and re-renders the PDF, even for the same page.

Recommendation: Add a simple LRU cache for rendered pages:

from functools import lru_cache
import hashlib

# Cache key: (user_id, file_path, page, scale)
@lru_cache(maxsize=100)  # Cache last 100 rendered pages
def _render_pdf_page_cached(cache_key: tuple) -> tuple[bytes, int]:
    user_id, file_path, page_num, scale, pdf_hash = cache_key
    # Actual rendering logic here
    ...

# In get_pdf_preview():
pdf_hash = hashlib.sha256(pdf_bytes).hexdigest()[:16]  # Short hash for cache key
cache_key = (user_id, file_path, page_num, scale, pdf_hash)
png_bytes, total_pages = _render_pdf_page_cached(cache_key)

Benefit: Significantly reduces CPU load when users navigate back/forth between pages.

Trade-off: Memory usage (but 100 pages × ~500KB ≈ 50MB is reasonable).

---

6. Logging: Too Verbose for Production

Issue: Every PDF preview request logs at INFO level (line 682, 687, 781):

logger.info(f"PDF preview request: file_path={file_path_param}, page={page_param}")
logger.info(f"PDF preview authenticated for user: {user_id}")
logger.info(f"Rendered PDF preview: {file_path} page {page_num}/{total_pages}, {len(png_bytes):,} bytes")

Problem: In production with many users, this creates excessive log noise.

Recommendation:

• Change lines 682, 687 to logger.debug()
• Keep line 781 as logger.info() but only log on error or every Nth request

---

7. Testing: Missing Integration Tests

Observation: Excellent unit tests, but no integration tests verifying:

• End-to-end flow: Vue component → PHP controller → Python API → WebDAV download → PyMuPDF render
• Actual PDF rendering with different PDF types (text PDFs, scanned PDFs, PDFs with images)
• Page navigation in the UI

Recommendation: Add integration test in tests/integration/test_astrolabe_pdf_viewer.py:

@pytest.mark.integration
async def test_pdf_viewer_end_to_end(nc_mcp_oauth_client, nc_client):
    """Test complete PDF viewing flow through Astrolabe UI."""
    # 1. Upload test PDF via WebDAV
    # 2. Index it via vector sync
    # 3. Search for content
    # 4. Render PDF page via API
    # 5. Verify base64 image is valid PNG
    ...

---

📋 Minor Issues

8. Vue Component: Missing Scale Control

The PDFViewer.vue component accepts a scale prop but provides no UI to adjust it. Consider adding zoom controls (+/-) for better UX.

9. Documentation: Update CLAUDE.md

The PR adds significant new functionality but doesn't update CLAUDE.md. Add a section under "Project Structure" or "Architecture Quick Reference" documenting the PDF preview endpoint.

---

🎯 Summary

Category	Rating	Comments
Architecture	⭐⭐⭐⭐⭐	Excellent separation of concerns, solves real problems
Security	⭐⭐⭐	Path traversal check is weak, no rate limiting
Testing	⭐⭐⭐⭐	Comprehensive unit tests, missing integration tests
Code Quality	⭐⭐⭐⭐	Clean code, good type annotations, follows conventions
Performance	⭐⭐⭐	No caching, no rate limiting, excessive logging

---

✅ Approval Recommendation

Approve with minor changes. The core implementation is solid and solves a real problem. The security and performance issues should be addressed before merge:

Critical (must fix):

1. Strengthen path traversal validation (Issue Test on PRs to master #1)
2. Add rate limiting (Issue Clean up ci #2)

Recommended (should fix):
3. Improve error handling in PHP controller (Issue #3)
4. Reduce logging verbosity (Issue #6)

Nice to have:
5. Add page rendering cache (Issue #5)
6. Add integration tests (Issue #7)
7. Update documentation (Issue #9)

---

Great work overall! The shift to server-side rendering is the right architectural choice, and the implementation demonstrates good engineering practices. 🚀