Skip to content

Commit d418356

Browse files
JonPurvisSammyjo20
JonPurvis
and
Sammyjo20
authored
Foundational improvements to v4 (#539)
* fixes-090326-v4 * Add error handling for invalid base directory paths Throw exceptions when realpath cannot be determined. * Refactor test descriptions for clarity * Updated workflows to run against v4 branch * improve lookup * improve checking * 🪄 Code Style Fixes * Removed serialize/unserialize from AccessTokenAuthenticator * 🪄 Code Style Fixes * Added back SoloRequest * Improved regex * allow solo * amend phpstan * Added test * work on windows * 🪄 Code Style Fixes * Fixed broken test for windows * wip * 🪄 Code Style Fixes * Delete tests/Fixtures/Saloon/file.json --------- Co-authored-by: Sam Carré <29132017+Sammyjo20@users.noreply.github.com> Co-authored-by: JonPurvis <7534029+JonPurvis@users.noreply.github.com>
1 parent 8039c76 commit d418356

14 files changed

Lines changed: 244 additions & 44 deletions

‎.github/workflows/php-cs-fixer.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ on:
66
- 'v1'
77
- 'v2'
88
- 'v3'
9+
- 'v4'
910
pull_request:
1011
branches:
1112
- '*'

‎.github/workflows/phpstan.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ on:
66
- 'v1'
77
- 'v2'
88
- 'v3'
9+
- 'v4'
910
pull_request:
1011
branches:
1112
- '*'

‎.github/workflows/tests.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ on:
66
- 'v1'
77
- 'v2'
88
- 'v3'
9+
- 'v4'
910
pull_request:
1011
branches:
1112
- '*'

‎src/Helpers/Storage.php‎

Lines changed: 76 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
namespace Saloon\Helpers;
66

7+
use InvalidArgumentException;
78
use Saloon\Exceptions\DirectoryNotFoundException;
89
use Saloon\Exceptions\UnableToCreateFileException;
910
use Saloon\Exceptions\UnableToCreateDirectoryException;
@@ -51,12 +52,81 @@ protected function buildPath(string $path): string
5152
return rtrim($this->baseDirectory, $trimRules) . DIRECTORY_SEPARATOR . ltrim($path, $trimRules);
5253
}
5354

55+
/**
56+
* Normalize a path by resolving . and .. segments (no filesystem access).
57+
*/
58+
protected function normalizePath(string $path): string
59+
{
60+
$leadingSlash = $path !== '' && $path[0] === DIRECTORY_SEPARATOR;
61+
$leadingDrive = mb_strlen($path) >= 2 && $path[1] === ':';
62+
63+
$segments = [];
64+
foreach (preg_split('#[/\\\\]+#', $path, -1, PREG_SPLIT_NO_EMPTY) ?: [] as $segment) {
65+
if ($segment === '.') {
66+
continue;
67+
}
68+
if ($segment === '..') {
69+
array_pop($segments);
70+
continue;
71+
}
72+
$segments[] = $segment;
73+
}
74+
75+
$result = implode(DIRECTORY_SEPARATOR, $segments);
76+
if ($leadingSlash && $result !== '') {
77+
$result = DIRECTORY_SEPARATOR . $result;
78+
}
79+
if ($leadingDrive && $result !== '' && ! preg_match('#^[a-zA-Z]:#', $result)) {
80+
$result = $path[0] . ':' . $result;
81+
}
82+
83+
return $result;
84+
}
85+
86+
/**
87+
* Ensure the resolved path is under the base directory to prevent path traversal.
88+
*
89+
* @throws InvalidArgumentException
90+
*/
91+
protected function ensurePathUnderBase(string $fullPath): void
92+
{
93+
$baseReal = realpath($this->baseDirectory);
94+
95+
if ($baseReal === false) {
96+
throw new InvalidArgumentException('Unable to determine the realpath of the base directory.');
97+
}
98+
99+
if (str_contains($fullPath, '~')) {
100+
throw new InvalidArgumentException('Path must remain inside the storage base directory.');
101+
}
102+
103+
$baseTrimmed = rtrim($this->baseDirectory, DIRECTORY_SEPARATOR . ' ');
104+
$baseNorm = $this->normalizePath($baseTrimmed);
105+
$fullNorm = $this->normalizePath($fullPath);
106+
$baseWithSep = $baseNorm . DIRECTORY_SEPARATOR;
107+
108+
if ($baseTrimmed !== '' && $fullNorm !== $baseNorm && ! str_starts_with($fullNorm, $baseWithSep)) {
109+
throw new InvalidArgumentException('Path must remain inside the storage base directory.');
110+
}
111+
112+
$pathSuffix = $baseTrimmed === '' ? $fullPath : ($fullNorm === $baseNorm ? '' : mb_substr($fullNorm, mb_strlen($baseWithSep)));
113+
$normalizedAbsolute = $this->normalizePath($baseReal . DIRECTORY_SEPARATOR . $pathSuffix);
114+
115+
$baseWithSeparator = $baseReal . DIRECTORY_SEPARATOR;
116+
if ($normalizedAbsolute !== $baseReal && ! str_starts_with($normalizedAbsolute, $baseWithSeparator)) {
117+
throw new InvalidArgumentException('Path must remain inside the storage base directory.');
118+
}
119+
}
120+
54121
/**
55122
* Check if the file exists
56123
*/
57124
public function exists(string $path): bool
58125
{
59-
return file_exists($this->buildPath($path));
126+
$fullPath = $this->buildPath($path);
127+
$this->ensurePathUnderBase($fullPath);
128+
129+
return file_exists($fullPath);
60130
}
61131

62132
/**
@@ -72,7 +142,10 @@ public function missing(string $path): bool
72142
*/
73143
public function get(string $path): bool|string
74144
{
75-
return file_get_contents($this->buildPath($path));
145+
$fullPath = $this->buildPath($path);
146+
$this->ensurePathUnderBase($fullPath);
147+
148+
return file_get_contents($fullPath);
76149
}
77150

78151
/**
@@ -85,6 +158,7 @@ public function get(string $path): bool|string
85158
public function put(string $path, string $contents): static
86159
{
87160
$fullPath = $this->buildPath($path);
161+
$this->ensurePathUnderBase($fullPath);
88162

89163
$directoryWithoutFilename = implode(DIRECTORY_SEPARATOR, explode(DIRECTORY_SEPARATOR, $fullPath, -1));
90164

‎src/Helpers/URLHelper.php‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44

55
namespace Saloon\Helpers;
66

7+
use InvalidArgumentException;
8+
79
/**
810
* @internal
911
*/
@@ -19,10 +21,23 @@ public static function matches(string $pattern, string $value): bool
1921

2022
/**
2123
* Join a base url and an endpoint together.
24+
*
25+
* When the connector has a base URL, the endpoint must be a relative path (e.g. "/users" or "users").
26+
* Absolute URLs in the endpoint are not allowed in that case (SSRF / credential leakage).
27+
* When the base URL is empty (e.g. Solo Request), the endpoint may be an absolute URL.
28+
*
29+
* @throws InvalidArgumentException When the endpoint is an absolute URL and the base URL is not empty
2230
*/
2331
public static function join(string $baseUrl, string $endpoint): string
2432
{
25-
if (static::isValidUrl($endpoint)) {
33+
$baseTrimmed = trim($baseUrl, '/ ');
34+
if ($baseTrimmed !== '' && static::isValidUrl($endpoint)) {
35+
throw new InvalidArgumentException(
36+
'Absolute URLs are not allowed in the endpoint. The endpoint must be a relative path to prevent SSRF and credential leakage. To request a different host, use a connector with that host as the base URL.'
37+
);
38+
}
39+
40+
if ($baseTrimmed === '' && static::isValidUrl($endpoint)) {
2641
return $endpoint;
2742
}
2843

‎src/Http/Auth/AccessTokenAuthenticator.php‎

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -88,20 +88,4 @@ public function isNotRefreshable(): bool
8888
{
8989
return ! $this->isRefreshable();
9090
}
91-
92-
/**
93-
* Serialize the access token.
94-
*/
95-
public function serialize(): string
96-
{
97-
return serialize($this);
98-
}
99-
100-
/**
101-
* Unserialize the access token.
102-
*/
103-
public static function unserialize(string $string): static
104-
{
105-
return unserialize($string, ['allowed_classes' => true]);
106-
}
10791
}

‎src/Http/Faking/Fixture.php‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
namespace Saloon\Http\Faking;
66

77
use Saloon\MockConfig;
8+
use function preg_match;
89
use Saloon\Helpers\Storage;
910
use Saloon\Helpers\ArrayHelpers;
1011
use Saloon\Data\RecordedResponse;
@@ -155,6 +156,9 @@ public function store(RecordedResponse $recordedResponse): static
155156
/**
156157
* Get the fixture path
157158
*
159+
* The fixture name must not contain path components (/, \, ..) to prevent path traversal.
160+
* Only alphanumeric characters, hyphens, and underscores are allowed.
161+
*
158162
* @throws \Saloon\Exceptions\FixtureException
159163
*/
160164
public function getFixturePath(): string
@@ -169,6 +173,11 @@ public function getFixturePath(): string
169173
throw new FixtureException('The fixture must have a name');
170174
}
171175

176+
if (str_contains($name, "\0") || str_contains($name, '..') || str_contains($name, '~')
177+
|| ! preg_match('/^[a-zA-Z0-9\/_\-\\\\]+$/', $name)) {
178+
throw new FixtureException('The fixture name must not contain directory traversal components or invalid characters. Only alphanumeric characters, hyphens, slashes, and underscores are allowed.');
179+
}
180+
172181
return sprintf('%s.%s', $name, $this::$fixtureExtension);
173182
}
174183

‎tests/Feature/FixturePathTraversalTest.php‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
use Saloon\Helpers\Storage;
6+
use Saloon\Http\Faking\Fixture;
7+
use Saloon\Data\RecordedResponse;
8+
use Saloon\Exceptions\FixtureException;
9+
10+
beforeEach(function () {
11+
$this->fixtureBaseDir = sys_get_temp_dir() . '/saloon_fixture_traversal_' . uniqid('', true);
12+
mkdir($this->fixtureBaseDir, 0777, true);
13+
});
14+
15+
afterEach(function () {
16+
if (isset($this->fixtureBaseDir) && is_dir($this->fixtureBaseDir)) {
17+
array_map('unlink', glob($this->fixtureBaseDir . '/*') ?: []);
18+
rmdir($this->fixtureBaseDir);
19+
}
20+
$escapeWritePath = sys_get_temp_dir() . '/traversal_write_test.json';
21+
if (file_exists($escapeWritePath)) {
22+
unlink($escapeWritePath);
23+
}
24+
$escapeReadPath = sys_get_temp_dir() . '/traversal_read_target.json';
25+
if (file_exists($escapeReadPath)) {
26+
unlink($escapeReadPath);
27+
}
28+
});
29+
30+
test('fixture name with path traversal throws when getting mock response and does not read outside base', function () {
31+
$storage = new Storage($this->fixtureBaseDir, true);
32+
33+
$externalPath = sys_get_temp_dir() . '/traversal_read_target.json';
34+
$secretContent = 'read_from_outside';
35+
file_put_contents($externalPath, json_encode([
36+
'statusCode' => 200,
37+
'headers' => [],
38+
'data' => '{"secret":"' . $secretContent . '"}',
39+
'context' => [],
40+
]));
41+
42+
$traversalName = '..' . DIRECTORY_SEPARATOR . 'traversal_read_target';
43+
$fixture = new Fixture($traversalName, $storage);
44+
45+
expect(fn () => $fixture->getMockResponse())
46+
->toThrow(FixtureException::class, 'The fixture name must not contain directory traversal components or invalid characters. Only alphanumeric characters, hyphens, slashes, and underscores are allowed.');
47+
48+
expect(file_get_contents($externalPath))->toContain($secretContent);
49+
});
50+
51+
test('fixture name with path traversal throws when storing and does not write outside base', function () {
52+
$storage = new Storage($this->fixtureBaseDir, true);
53+
54+
$traversalName = '..' . DIRECTORY_SEPARATOR . 'traversal_write_test';
55+
$fixture = new Fixture($traversalName, $storage);
56+
57+
$recordedResponse = new RecordedResponse(200, [], '{"pwned":true}');
58+
59+
expect(fn () => $fixture->store($recordedResponse))
60+
->toThrow(FixtureException::class, 'The fixture name must not contain directory traversal components or invalid characters. Only alphanumeric characters, hyphens, slashes, and underscores are allowed.');
61+
62+
$escapePath = sys_get_temp_dir() . DIRECTORY_SEPARATOR . 'traversal_write_test.json';
63+
expect(file_exists($escapePath))->toBeFalse();
64+
});

‎tests/Feature/MockRequestTest.php‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -739,3 +739,13 @@ function (PendingRequest $pendingRequest): MockResponse {
739739

740740
$mockClient->assertSent(UserRequest::class);
741741
});
742+
743+
test('fixtures can be created in sub directories', function () {
744+
$mockClient = new MockClient([
745+
MockResponse::fixture('my-integration' . DIRECTORY_SEPARATOR .'user'), // Test Exact Route
746+
]);
747+
748+
connector()->send(new UserRequest, $mockClient);
749+
750+
$mockClient->assertSent(UserRequest::class);
751+
});

‎tests/Fixtures/Requests/AbsoluteEndpointRequest.php‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace Saloon\Tests\Fixtures\Requests;
6+
7+
use Saloon\Enums\Method;
8+
use Saloon\Http\Request;
9+
10+
/**
11+
* Request whose endpoint is an absolute URL - used to replicate SSRF
12+
* (absolute URL override of baseUrl) in security tests.
13+
*/
14+
class AbsoluteEndpointRequest extends Request
15+
{
16+
protected Method $method = Method::GET;
17+
18+
public function __construct(
19+
private readonly string $endpoint = 'https://attacker.example.com/steal'
20+
) {
21+
}
22+
23+
public function resolveEndpoint(): string
24+
{
25+
return $this->endpoint;
26+
}
27+
}

0 commit comments

Comments
 (0)