Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
2b7c0ac
fix: validate file path in Blueprints::import() to prevent SSRF/LFI
javokhir-sec Jul 3, 2026
7e8bf7f
fix(blueprints): harden import() path validation against SSRF/LFI
javokhir-sec Jul 20, 2026
42bd4e5
fix(blueprints): address Copilot review — TOCTOU, log level, test ass…
javokhir-sec Jul 20, 2026
112279f
fix(test): correct XML canvas key to match getDatabaseType()
javokhir-sec Jul 21, 2026
20823a9
fix(test): use correct box name 'problem' from lean canvas template
javokhir-sec Jul 21, 2026
c594b4f
style: fix pint formatting
javokhir-sec Jul 21, 2026
ed54874
style: fix pint formatting
javokhir-sec Jul 21, 2026
4b0d1d7
fix(test): apply Copilot review — portability, unique names, cleanup
javokhir-sec Jul 21, 2026
d7d6ca9
Merge branch 'fix/ssrf-lfi-final' into fix/pint-csrf-final
javokhir-sec Jul 21, 2026
25645b9
chore: remove accidentally added files
javokhir-sec Jul 21, 2026
08b82db
fix: use base_path() for userfiles + fix tempnam orphan file
javokhir-sec Jul 21, 2026
8d8f629
fix(test): replace tempnam with rename to avoid orphaned temp files
javokhir-sec Jul 21, 2026
afe9f27
Merge branch 'fix/ssrf-lfi-final' into fix/pint-csrf-final
javokhir-sec Jul 21, 2026
c7b5370
fix(test): proper LFI test using .xml outside allowed dirs
javokhir-sec Jul 21, 2026
5fceadd
Merge branch 'fix/ssrf-lfi-final' into fix/pint-csrf-final
javokhir-sec Jul 21, 2026
e19ae8c
fix: remove composer.phar binary and add to .gitignore
javokhir-sec Jul 23, 2026
b280394
style: apply pint fixes — imports, concat_space, class_definition
javokhir-sec Jul 23, 2026
15a1652
test: add file:// wrapper assertion to SSRF URL test
javokhir-sec Jul 23, 2026
d849517
fix: sync all SSRF/LFI hardening fixes from PR #3656
javokhir-sec Jul 23, 2026
832192e
docs: clarify sys_get_temp_dir() trust model, restore .gitkeep
javokhir-sec Jul 23, 2026
c3686cb
fix: import and stub UserRepository in import acceptance test
javokhir-sec Jul 23, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -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
104 changes: 103 additions & 1 deletion app/Domain/Blueprints/Services/Blueprints.php
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,80 @@ 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 in sys_get_temp_dir() are accepted by design: this is how PHP
* delivers uploaded files to the application (upload_tmp_dir / sys_temp_dir).
* On Unix systems the temp directory is typically world-writable with the
* sticky bit; the allow-list check is the gate, and we accept the residual
* risk that another local user could place a malicious .xml there — that
* attacker already has local code execution as the web server user, so
* crafting a temp file does not represent an additional 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.
*
Expand Down Expand Up @@ -412,7 +486,35 @@ public function import(string $filename, string $canvasSlug, int $projectId, int
$dom = new DOMDocument('1.0', 'UTF-8');
$users = app()->make(UserRepository::class);

Comment on lines 486 to 488
$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);
Comment on lines +489 to +493
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;
}
Expand Down
Empty file.
227 changes: 223 additions & 4 deletions tests/Unit/app/Domain/Blueprints/Services/BlueprintsServiceTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,17 @@

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;
use Leantime\Domain\Blueprints\Models\CanvasTemplate;
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;

/**
Expand All @@ -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
Expand All @@ -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,
);
}

Expand Down Expand Up @@ -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, '<canvas key="leancanvas"><title>LFI Test</title></canvas>');

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, '<canvas key="leancanvas"><title>Test</title></canvas>');

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, '<?php echo "pwned";');

$txtBase = tempnam(sys_get_temp_dir(), 'leantime.');
$txtFile = $txtBase.'.txt';
rename($txtBase, $txtFile);
file_put_contents($txtFile, 'not xml');

try {
$this->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'
<?xml version="1.0" encoding="UTF-8"?>
<canvas key="leancanvas">
<title>Security Test Canvas</title>
<content>
<element key="problem">
<item>
<author firstname="A" lastname="B"/>
<description>Test item</description>
<status>status_draft</status>
<relates>relates_none</relates>
<data>none</data>
<conclusion>none</conclusion>
<assumptions>none</assumptions>
<priority>low</priority>
<progress>0</progress>
<duedate></duedate>
<tags></tags>
</item>
</element>
</content>
</canvas>
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
Expand Down Expand Up @@ -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 = [];
Expand All @@ -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;

Expand Down
Loading