feat(28022): Implement Moodle TUS upload with SSO authentication#67
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements TUS (Tus Upload Protocol) support for Moodle integration with SSO authentication. It enables chunked file uploads from Moodle instances while validating users through the existing SSO service.
Changes:
- Added SSO parameter extraction and session storage in the upload controller to support TUS authentication flow
- Introduced a new MoodleServerController implementing TUS protocol endpoints with SSO validation, file extension checks, and upload dispatch functionality
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| Controller/UploadController.php | Extracts SSO credentials from request and stores them in session for TUS upload authentication |
| Controller/MoodleServerController.php | Implements TUS server endpoints with SSO authentication, file validation, and upload management for Moodle integration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| $this->validateFileExtension($request); | ||
|
|
||
| $cleanSeries = preg_replace('/[^a-zA-Z0-9_\-]/', '', (string) $series); |
There was a problem hiding this comment.
The series sanitization logic is duplicated on line 146. Consider extracting this into a private method to improve maintainability and ensure consistency.
| $cleanSeries = preg_replace('/[^a-zA-Z0-9_\-]/', '', (string) $series); | |
| $sanitizeSeries = static function (string $series): string { | |
| $cleanSeries = preg_replace('/[^a-zA-Z0-9_\-]/', '', $series); | |
| return $cleanSeries ?? ''; | |
| }; | |
| $cleanSeries = $sanitizeSeries((string) $series); |
|
|
||
| $metadata = $request->headers->get('Upload-Metadata'); | ||
| if ($metadata && preg_match('/filename (?P<name>[^\s,]+)/', $metadata, $matches)) { | ||
| $filename = base64_decode($matches['name']); |
There was a problem hiding this comment.
Base64-decoded filename is used directly in file path construction without sanitization. Malicious filenames could potentially contain directory traversal sequences (e.g., '../'). Sanitize the decoded filename before using it in file operations.
| $filename = base64_decode($matches['name']); | |
| $decodedName = base64_decode($matches['name'], true); | |
| if (false === $decodedName) { | |
| return new Response('Invalid filename encoding', 400); | |
| } | |
| // Sanitize filename to prevent directory traversal and unsafe characters | |
| $filename = basename($decodedName); | |
| $filename = str_replace(['/', '\\'], '_', $filename); | |
| $filename = preg_replace('/[^a-zA-Z0-9._\- ]/', '_', $filename); | |
| $filename = trim((string) $filename); | |
| if ('' === $filename) { | |
| return new Response('Invalid filename', 400); | |
| } |
| $declaredMimeType = ''; | ||
|
|
||
| if (preg_match('/filename (?P<name>[^\s,]+)/', $metadata, $matches)) { | ||
| $filename = base64_decode($matches['name']); |
There was a problem hiding this comment.
Base64-decoded filename should be sanitized before use in logging and error messages to prevent log injection attacks or path traversal issues. Apply path sanitization similar to the series sanitization.
| $filename = base64_decode($matches['name']); | ||
| } | ||
| if (preg_match('/filetype (?P<type>[^\s,]+)/', $metadata, $matches)) { | ||
| $declaredMimeType = base64_decode($matches['type']); |
There was a problem hiding this comment.
The decoded MIME type is not validated before being passed to isAllowed(). Ensure the MIME type string is properly validated to prevent injection of malicious content into the validation logic.
| $this->uploadDispatcherService->dispatchUploadFromInbox( | ||
| $user, | ||
| $request->get('fileName'), |
There was a problem hiding this comment.
The fileName parameter is retrieved directly from the request without validation and passed to dispatchUploadFromInbox. This could allow directory traversal or arbitrary file access. Validate and sanitize the fileName parameter.
| $this->uploadDispatcherService->dispatchUploadFromInbox( | |
| $user, | |
| $request->get('fileName'), | |
| $fileName = $request->get('fileName'); | |
| if (!is_string($fileName) || $fileName === '') { | |
| return new JsonResponse(['success' => false, 'error' => 'Invalid file name'], 400); | |
| } | |
| // Prevent directory traversal and enforce simple file names | |
| if (strpos($fileName, '..') !== false || strpbrk($fileName, "/\\") !== false) { | |
| return new JsonResponse(['success' => false, 'error' => 'Invalid file name'], 400); | |
| } | |
| $fileName = basename($fileName); | |
| $this->uploadDispatcherService->dispatchUploadFromInbox( | |
| $user, | |
| $fileName, |
No description provided.