diff --git a/.gitignore b/.gitignore index 9fc08255f6..10209e2be9 100644 --- a/.gitignore +++ b/.gitignore @@ -137,3 +137,4 @@ tests/Httprequests/http-client.private.env.json .claude/docs/personal-companion-requirements.md .claude/docs/git-workflow.md cleanup-architecture.md +composer.phar diff --git a/app/Domain/Auth/Controllers/Login.php b/app/Domain/Auth/Controllers/Login.php index 6f0057db0a..2aae9ed99b 100644 --- a/app/Domain/Auth/Controllers/Login.php +++ b/app/Domain/Auth/Controllers/Login.php @@ -64,16 +64,8 @@ public function get(array $params): Response public function post(array $params): Response { if (isset($_POST['username']) === true && isset($_POST['password']) === true) { - if (isset($_POST['redirectUrl'])) { - $redirectUrl = urldecode(filter_var($_POST['redirectUrl'], FILTER_SANITIZE_URL)); - } else { - // Browser flow always submits the hidden redirectUrl field. Non- - // browser callers (curl/API/test rigs) may omit it; previously - // this branch yielded '' and Frontcontroller::redirect('') threw - // "Cannot redirect to an empty URL". Match the GET-path default - // from Auth::resolveSafeRedirect(). - $redirectUrl = BASE_URL.'/dashboard/home'; - } + + $redirectUrl = $this->authService->resolveSafeRedirect($_POST['redirectUrl'] ?? null); $username = trim($_POST['username']); $password = $_POST['password']; diff --git a/app/Domain/Auth/Services/Auth.php b/app/Domain/Auth/Services/Auth.php index 092c2b01ae..7581208f9e 100644 --- a/app/Domain/Auth/Services/Auth.php +++ b/app/Domain/Auth/Services/Auth.php @@ -486,13 +486,39 @@ public function resolveSafeRedirect(?string $redirect): string $redirectUrl = BASE_URL.'/dashboard/home'; if ($redirect !== null && trim($redirect) !== '' && trim($redirect) !== '/') { - $url = urldecode($redirect); + // Normalize backslash-based protocol tricks (e.g. \/\/attacker.com) + // to forward slashes before any checks. + $url = str_replace('\\', '/', rawurldecode($redirect)); + + // Strip the application base URL when present so that same-origin + // absolute URLs (e.g. https://my-leantime.com/dashboard/home) are + // treated the same as their relative counterparts. + if (str_starts_with($url, BASE_URL)) { + $url = substr($url, strlen(BASE_URL)); + } + + // Guard: protocol-relative URL (//attacker.com) — explicitly reject. + // FILTER_VALIDATE_URL treats these as valid without a scheme, but + // browsers resolve them to the current scheme, making them an open + // redirect vector. + if (str_starts_with($url, '//')) { + return $redirectUrl; + } + + // Guard: external absolute URL — reject. + // filter_var returns the URL (truthy) for well-formed absolute URLs + // with a scheme; relative paths return false. + if (filter_var($url, FILTER_VALIDATE_URL) !== false) { + return $redirectUrl; + } + + // At this point $url is a relative path. Guard against an empty + // path that could result from stripping a BASE_URL-only input. + $url = ltrim($url, '/'); - // Check for open redirects, don't allow redirects to external sites. - if ( - filter_var($url, FILTER_VALIDATE_URL) === false && - ! in_array($url, ['/auth/logout']) - ) { + // Block redirect to logout — allowing a POST-login redirect to + // /auth/logout would create a forced-logout loop. + if ($url !== '' && $url !== 'auth/logout') { $redirectUrl = BASE_URL.'/'.$url; } } diff --git a/app/Domain/Blueprints/Services/Blueprints.php b/app/Domain/Blueprints/Services/Blueprints.php index 34feb76c28..0385cf7ce8 100644 --- a/app/Domain/Blueprints/Services/Blueprints.php +++ b/app/Domain/Blueprints/Services/Blueprints.php @@ -378,6 +378,79 @@ public function deleteBoard(int $canvasId, string $canvasType): void $this->blueprintsRepo->deleteCanvas($canvasId); } + /** + * Validate that a resolved import file path is safe to read. + * + * Rejects files outside a fixed allow-list of local directories and + * requires a known extension. The caller must resolve the path via + * {@see realpath()} first — realpath canonicalizes the path (resolves + * symlinks, relative segments, and `..` traversal) so the allow-list + * check operates on the true absolute path rather than the + * user-supplied string. + * + * The allow-list covers two directories: + * - the PHP upload temp directory (UI file-upload flow), and + * - the shipped fixture directory under the Blueprints domain. + * + * base_path('userfiles') is intentionally EXCLUDED: the global userfiles + * storage is managed by the Files domain with per-file authorization. + * Allowing import() to read arbitrary .xml files from userfiles would + * bypass that authorization — a caller with CREATE on any project could + * ingest files they should not have access to. + * + * .xml files landing in sys_get_temp_dir() are accepted by design: this is + * the normal UI upload flow (PHP stores uploaded files in the temp dir) and + * the allow-list is the gate. Any local process that can drop an .xml into + * the temp dir is already trusted — the temp dir is writable by the web + * server user by definition, so writing there does not represent an + * additional privilege escalation. + * + * @param string $resolvedPath Already-resolved absolute path (from realpath) + * @return bool True when the path is within an allowed directory + * and has an allowed extension + */ + private function isImportPathAllowed(string $resolvedPath): bool + { + $allowedDirs = [ + sys_get_temp_dir(), + APP_ROOT.'/app/Domain/Blueprints/imports', + ]; + + // Validate file extension — only XML is permitted because import() + // parses via DOMDocument::loadXML(). Shipped fixture files under the + // imports/ directory are .xml as well. + $ext = strtolower(pathinfo($resolvedPath, PATHINFO_EXTENSION)); + if ($ext !== 'xml') { + Log::warning('Blueprints import: disallowed file extension', [ + 'resolvedPath' => $resolvedPath, + 'extension' => $ext, + ]); + + return false; + } + + // Anchor each allowed directory with a trailing separator so + // str_starts_with doesn't match sibling-prefix paths (e.g. + // /tmp-evil/x must NOT match against allowed /tmp). + foreach ($allowedDirs as $allowedDir) { + $resolvedAllowed = realpath($allowedDir); + + if ($resolvedAllowed === false) { + continue; + } + + if (str_starts_with($resolvedPath, $resolvedAllowed.DIRECTORY_SEPARATOR)) { + return true; + } + } + + Log::warning('Blueprints import: path traversal or SSRF attempt blocked', [ + 'resolvedPath' => $resolvedPath, + ]); + + return false; + } + /** * Import a canvas board from an XML file. * @@ -412,7 +485,35 @@ public function import(string $filename, string $canvasSlug, int $projectId, int $dom = new DOMDocument('1.0', 'UTF-8'); $users = app()->make(UserRepository::class); - $canvasData = file_get_contents($filename); + // Validate the file path and extension to prevent SSRF and Local File + // Inclusion. Reject URL wrappers (http://, ftp://, etc.), restrict + // reads to allowed local directories, and require a known import + // extension. + $resolvedPath = realpath($filename); + if ($resolvedPath === false) { + Log::warning('Blueprints import: file not found or path does not exist', [ + 'filename' => $filename, + ]); + + return false; + } + + if (! $this->isImportPathAllowed($resolvedPath)) { + return false; + } + + // Guard against non-regular files (FIFO, device, socket) in + // world-writable /tmp — a named pipe named *.xml would hang the + // request if read without this check. + if (! is_file($resolvedPath) || ! is_readable($resolvedPath)) { + Log::warning('Blueprints import: path is not a readable regular file', [ + 'resolvedPath' => $resolvedPath, + ]); + + return false; + } + + $canvasData = file_get_contents($resolvedPath); if ($canvasData === false) { return false; } diff --git a/tests/Unit/app/Domain/Auth/Services/AuthServiceTest.php b/tests/Unit/app/Domain/Auth/Services/AuthServiceTest.php index 35bd16bb17..ace41047d7 100644 --- a/tests/Unit/app/Domain/Auth/Services/AuthServiceTest.php +++ b/tests/Unit/app/Domain/Auth/Services/AuthServiceTest.php @@ -70,6 +70,80 @@ public function test_resolve_safe_redirect_blocks_external_url(): void ); } + public function test_resolve_safe_redirect_allows_same_origin_absolute_url(): void + { + $service = $this->makeService(); + + // Same-origin absolute URL — must be treated the same as a relative + // path by stripping the BASE_URL prefix. This is the exact scenario + // the maintainer flagged: the login form often submits a full + // absolute URL in the redirectUrl hidden field. + $this->assertSame( + BASE_URL.'/dashboard/home', + $service->resolveSafeRedirect(BASE_URL.'/dashboard/home') + ); + } + + public function test_resolve_safe_redirect_allows_same_origin_absolute_url_with_deep_path(): void + { + $service = $this->makeService(); + + $this->assertSame( + BASE_URL.'/tickets/showAll', + $service->resolveSafeRedirect(BASE_URL.'/tickets/showAll') + ); + } + + public function test_resolve_safe_redirect_allows_url_encoded_same_origin_absolute_url(): void + { + $service = $this->makeService(); + + // URL-encoded same-origin absolute URL — rawurldecode is called first, + // then BASE_URL is stripped. + $this->assertSame( + BASE_URL.'/tickets/showAll', + $service->resolveSafeRedirect(urlencode(BASE_URL.'/tickets/showAll')) + ); + } + + public function test_resolve_safe_redirect_rejects_external_url_disguised_with_base_url_prefix(): void + { + $service = $this->makeService(); + + // An external URL whose path happens to start with the same characters + // as BASE_URL — str_starts_with won't match because the scheme+host + // differ. This gets rejected as external. + $this->assertSame( + BASE_URL.'/dashboard/home', + $service->resolveSafeRedirect('https://evil.example.com/'.BASE_URL.'/dashboard/home') + ); + } + + public function test_resolve_safe_redirect_rejects_protocol_relative_url(): void + { + $service = $this->makeService(); + + // Protocol-relative URL (//attacker.com) — FILTER_VALIDATE_URL + // treats these as valid URLs, so they are correctly rejected + // and the default dashboard redirect is returned. + $this->assertSame( + BASE_URL.'/dashboard/home', + $service->resolveSafeRedirect('//attacker.com') + ); + } + + public function test_resolve_safe_redirect_rejects_backslash_protocol_trick(): void + { + $service = $this->makeService(); + + // Backslash variant (\/\/attacker.com) — some parsers treat + // this as a protocol-relative URL. Verify it is rejected. + $this->assertSame( + BASE_URL.'/dashboard/home', + $service->resolveSafeRedirect('\/\/attacker.com') + ); + } + public function test_check_password_strength_rejects_weak_and_accepts_strong(): void { $service = $this->makeService(); diff --git a/tests/Unit/app/Domain/Blueprints/Services/BlueprintsServiceTest.php b/tests/Unit/app/Domain/Blueprints/Services/BlueprintsServiceTest.php index b24b30d040..941552b114 100644 --- a/tests/Unit/app/Domain/Blueprints/Services/BlueprintsServiceTest.php +++ b/tests/Unit/app/Domain/Blueprints/Services/BlueprintsServiceTest.php @@ -2,6 +2,7 @@ namespace Unit\app\Domain\Blueprints\Services; +use Codeception\Test\Feature\Stub; use Leantime\Core\Auth\Permissions\PermissionService; use Leantime\Core\Exceptions\AuthorizationException; use Leantime\Core\Language as LanguageCore; @@ -9,6 +10,9 @@ use Leantime\Domain\Blueprints\Repositories\Blueprints as BlueprintsRepository; use Leantime\Domain\Blueprints\Services\Blueprints as BlueprintsService; use Leantime\Domain\Blueprints\Services\TemplateRegistry; +use Leantime\Domain\ContentTemplates\Models\ContentTemplate; +use Leantime\Domain\ContentTemplates\Services\ContentTemplateRegistry; +use Leantime\Domain\Users\Repositories\Users as UserRepository; use Unit\TestCase; /** @@ -17,7 +21,7 @@ */ class BlueprintsServiceTest extends TestCase { - use \Codeception\Test\Feature\Stub; + use Stub; /** * Build the service with a language stub that prefixes keys with "T:" so we @@ -31,7 +35,7 @@ private function service(?BlueprintsRepository $repo = null, ?TemplateRegistry $ $repo ?? $this->make(BlueprintsRepository::class), $registry ?? new TemplateRegistry, $language, - new \Leantime\Domain\ContentTemplates\Services\ContentTemplateRegistry, + new ContentTemplateRegistry, ); } @@ -508,6 +512,221 @@ public function test_import_authorizes_create_on_target_project_before_doing_any $service->import('/tmp/does-not-matter.xml', 'swot', 55, 1); } + // --------------------------------------------------------------------- + // import() path-validation regression tests (SSRF / LFI / CWE-918). + // --------------------------------------------------------------------- + + public function test_import_rejects_ssrf_url_wrappers(): void + { + // URL wrappers such as http://, ftp:// resolve to false via realpath(), + // but even if/when a stream wrapper could produce a realpath, the + // allow-list check catches it. This test also guards the more + // subtle case of file:///etc/passwd which some PHP builds resolve. + $service = $this->securedService( + $this->make(BlueprintsRepository::class), + $this->allowingPermissions() + ); + + // SSRF: HTTP URL — realpath() returns false, caught as "file not found". + $this->assertFalse( + $service->import('http://169.254.169.254/latest/meta-data/', 'lean', 55, 1), + 'HTTP URL must be rejected' + ); + + // SSRF: FTP URL. + $this->assertFalse( + $service->import('ftp://evil.com/blueprint.xml', 'lean', 55, 1), + 'FTP URL must be rejected' + ); + + // LFI: file:// wrapper. Some PHP builds resolve file:///etc/passwd + // via realpath() and would read it without the allow-list guard. + $this->assertFalse( + $service->import('file:///etc/passwd', 'lean', 55, 1), + 'file:// URL must be rejected' + ); + } + + public function test_import_rejects_lfi_absolute_path_to_system_file(): void + { + // Create an .xml file in a directory that is NOT in the allowed list. + // base_path('storage') is reliably outside sys_temp_dir, userfiles, and + // Blueprints/imports — unlike /var/tmp which can equal sys_get_temp_dir() + // on some systems. + $service = $this->securedService( + $this->make(BlueprintsRepository::class), + $this->allowingPermissions() + ); + + $outOfBounds = base_path('storage/leantime_lfi_test_'.uniqid('', true).'.xml'); + file_put_contents($outOfBounds, 'LFI Test'); + + try { + $this->assertFalse( + $service->import($outOfBounds, 'lean', 55, 1), + 'Absolute path to an .xml file outside allowed directories must be rejected' + ); + } finally { + if (file_exists($outOfBounds)) { + unlink($outOfBounds); + } + } + } + + public function test_import_rejects_dot_dot_path_traversal(): void + { + // Construct a path that starts inside the temp directory but + // traverses out and back into /etc/passwd. + $service = $this->securedService( + $this->make(BlueprintsRepository::class), + $this->allowingPermissions() + ); + + $traversal = sys_get_temp_dir().'/../../../etc/passwd'; + + $this->assertFalse( + $service->import($traversal, 'lean', 55, 1), + 'Path traversal (../) must be rejected' + ); + } + + public function test_import_rejects_sibling_prefix_bypass(): void + { + // str_starts_with without separator anchoring would allow + // /tmp-evil/blueprint.xml to match against allowed /tmp. + // The fix appends DIRECTORY_SEPARATOR to each allowed dir. + $service = $this->securedService( + $this->make(BlueprintsRepository::class), + $this->allowingPermissions() + ); + + // Create a directory whose name is a prefix of the real temp dir. + // Use a unique suffix to avoid collisions with leftover dirs from + // crashed runs or concurrent test processes. + $siblingDir = sys_get_temp_dir().'-evil-'.uniqid('', true); + if (! is_dir($siblingDir)) { + mkdir($siblingDir, 0700, true); + } + $siblingFile = $siblingDir.'/blueprint.xml'; + file_put_contents($siblingFile, 'Test'); + + try { + $this->assertFalse( + $service->import($siblingFile, 'lean', 55, 1), + 'Sibling-prefix path (e.g. /tmp-evil/…) must NOT match allowed /tmp' + ); + } finally { + unlink($siblingFile); + rmdir($siblingDir); + } + } + + public function test_import_rejects_disallowed_file_extensions(): void + { + // Only .xml is permitted. Other extensions must be + // rejected even when the file sits in an allowed directory. + $service = $this->securedService( + $this->make(BlueprintsRepository::class), + $this->allowingPermissions() + ); + + // Use tempnam() + rename to get unique filenames — fixed names + // in the shared temp dir can collide with crashed-run leftovers + // or concurrent test processes. + $phpBase = tempnam(sys_get_temp_dir(), 'leantime.'); + $phpFile = $phpBase.'.php'; + rename($phpBase, $phpFile); + file_put_contents($phpFile, 'assertFalse( + $service->import($phpFile, 'lean', 55, 1), + '.php extension must be rejected in an allowed directory' + ); + $this->assertFalse( + $service->import($txtFile, 'lean', 55, 1), + '.txt extension must be rejected in an allowed directory' + ); + } finally { + if (file_exists($phpFile)) { + unlink($phpFile); + } + if (file_exists($txtFile)) { + unlink($txtFile); + } + } + } + + public function test_import_accepts_xml_file_in_allowed_temp_dir(): void + { + // A .xml file placed in sys_get_temp_dir() (the normal upload flow) + // must pass path validation and successfully import via the repo. + // The repository is stubbed so the import completes and returns a + // known canvas id, proving that path validation did NOT block it. + $expectedId = 42; + $repo = $this->make(BlueprintsRepository::class, [ + 'existCanvas' => fn () => false, + 'addCanvas' => fn () => $expectedId, + 'addCanvasItem' => fn () => 1, + ]); + $service = $this->securedService($repo, $this->allowingPermissions()); + + // import() resolves UserRepository via app()->make(). Unit tests + // disable the database, so bind a stub that never touches it. + $usersStub = $this->make(UserRepository::class, [ + 'getUserIdByName' => fn () => 1, + ]); + app()->instance(UserRepository::class, $usersStub); + + $xml = <<<'XML' + + + Security Test Canvas + + + + + Test item + status_draft + relates_none + none + none + none + low + 0 + + + + + + +XML; + + $tmpBase = tempnam(sys_get_temp_dir(), 'leantime.'); + $tempFile = $tmpBase.'.xml'; + rename($tmpBase, $tempFile); + file_put_contents($tempFile, $xml); + + try { + $result = $service->import($tempFile, 'lean', 55, 1); + // Path validation passed and repo returned the expected canvas id. + $this->assertSame( + $expectedId, + $result, + 'XML file in allowed dir must pass path validation and be imported' + ); + } finally { + if (file_exists($tempFile)) { + unlink($tempFile); + } + } + } + public function test_create_board_applies_start_content_against_the_slug_not_the_db_type(): void { // Regression test for Phase 4: createBoard() is called with the DATABASE @@ -546,7 +765,7 @@ public function getByDatabaseType(string $dbType): ?CanvasTemplate } }; - $contentTemplates = new class extends \Leantime\Domain\ContentTemplates\Services\ContentTemplateRegistry + $contentTemplates = new class extends ContentTemplateRegistry { /** @var string[] */ public array $seenSlugs = []; @@ -557,7 +776,7 @@ public function getByDatabaseType(string $dbType): ?CanvasTemplate // by reference, and a typed-property reference is brittle. public function __construct() {} - public function get(string $appliesTo, string $key): ?\Leantime\Domain\ContentTemplates\Models\ContentTemplate + public function get(string $appliesTo, string $key): ?ContentTemplate { $this->seenSlugs[] = $appliesTo;