From 7cc8fa2db4420adc0f5d79e2e9c38e21e510e0e4 Mon Sep 17 00:00:00 2001 From: Sean O'Brien Date: Wed, 22 Nov 2023 13:31:59 -0500 Subject: [PATCH 1/9] bugfix: dot segment handling --- .changes/nextrelease/s3-dot-segment.json | 7 ++++ src/Api/Serializer/RestSerializer.php | 18 ++++++-- tests/S3/S3ClientTest.php | 53 ++++++++++++++++++++++++ 3 files changed, 74 insertions(+), 4 deletions(-) create mode 100644 .changes/nextrelease/s3-dot-segment.json diff --git a/.changes/nextrelease/s3-dot-segment.json b/.changes/nextrelease/s3-dot-segment.json new file mode 100644 index 0000000000..f5c139ea9f --- /dev/null +++ b/.changes/nextrelease/s3-dot-segment.json @@ -0,0 +1,7 @@ +[ + { + "type": "bugfix", + "category": "s3", + "description": "Disables transformation of request URI paths with dot segments" + } +] \ No newline at end of file diff --git a/src/Api/Serializer/RestSerializer.php b/src/Api/Serializer/RestSerializer.php index dbd925ad15..73a82f7f1d 100644 --- a/src/Api/Serializer/RestSerializer.php +++ b/src/Api/Serializer/RestSerializer.php @@ -248,14 +248,24 @@ function (array $matches) use ($varDefinitions) { $relative = substr($relative, 1); } - //Append path to endpoint when leading '//...' present - // as uri cannot be properly resolved - if ($this->api->isModifiedModel() - && strpos($relative, '//') === 0 + //Append path to endpoint when leading '//...' or + // '../' present as uri cannot be properly resolved + if (($this->api->isModifiedModel() + && strpos($relative, '//') === 0) ) { return new Uri($this->endpoint . $relative); } + if ($this->api->getServiceName() === 's3' + && strpos($relative, '../') !== false + ){ + return new Uri( + $this->endpoint + . (strpos($relative, '/') !== 0 ? '/' : '') + . $relative + ); + } + // Expand path place holders using Amazon's slightly different URI // template syntax. return UriResolver::resolve($this->endpoint, new Uri($relative)); diff --git a/tests/S3/S3ClientTest.php b/tests/S3/S3ClientTest.php index 9e6eabf8b5..4bd1f35748 100644 --- a/tests/S3/S3ClientTest.php +++ b/tests/S3/S3ClientTest.php @@ -2321,4 +2321,57 @@ public function testDoesNotComputeMD5($options, $operation) ); $s3->execute($command); } + + /** + * @dataProvider dotSegmentProvider + */ + public function testHandlesDotSegmentsInKey($key, $expectedUri) + { + $s3 = $this->getTestClient('s3'); + $this->addMockResults($s3, [[]]); + $command = $s3->getCommand('getObject', ['Bucket' => 'foo', 'Key' => $key]); + $command->getHandlerList()->appendSign( + Middleware::tap(function ($cmd, $req) use ($expectedUri) { + $this->assertSame($expectedUri, (string) $req->getUri()); + }) + ); + $s3->execute($command); + } + + public function dotSegmentProvider() + { + return [ + ['../foo' , 'https://foo.s3.amazonaws.com/../foo'], + ['bar/../../foo', 'https://foo.s3.amazonaws.com/bar/../../foo'], + ['/../foo', 'https://foo.s3.amazonaws.com//../foo'], + ['foo/bar/../baz', 'https://foo.s3.amazonaws.com/foo/bar/../baz'] + ]; + } + + /** + * @dataProvider dotSegmentPathStyleProvider + */ + public function testHandlesDotSegmentsInKeyWithPathStyle($key, $expectedUri) + { + $s3 = $this->getTestClient('s3', ['use_path_style_endpoint' => true]); + $this->addMockResults($s3, [[]]); + $command = $s3->getCommand('getObject', ['Bucket' => 'foo', 'Key' => $key]); + $command->getHandlerList()->appendSign( + Middleware::tap(function ($cmd, $req) use ($expectedUri) { + $this->assertSame($expectedUri, (string) $req->getUri()); + }) + ); + $s3->execute($command); + } + + public function dotSegmentPathStyleProvider() + { + return [ + ['../foo' , 'https://s3.amazonaws.com/foo/foo/../foo'], + ['bar/../../foo', 'https://s3.amazonaws.com/foo/foo/bar/../../foo'], + ['/../foo', 'https://s3.amazonaws.com/foo/foo//../foo'], + ['foo/bar/../baz', 'https://s3.amazonaws.com/foo/foo/foo/bar/../baz'], + ]; + } + } From 450690ced6dfdd4640dd5fd2c0276801eca5e97b Mon Sep 17 00:00:00 2001 From: Sean O'Brien Date: Wed, 22 Nov 2023 13:39:23 -0500 Subject: [PATCH 2/9] update docs --- .changes/nextrelease/s3-dot-segment.json | 2 +- src/Api/Serializer/RestSerializer.php | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/.changes/nextrelease/s3-dot-segment.json b/.changes/nextrelease/s3-dot-segment.json index f5c139ea9f..afe6799b55 100644 --- a/.changes/nextrelease/s3-dot-segment.json +++ b/.changes/nextrelease/s3-dot-segment.json @@ -4,4 +4,4 @@ "category": "s3", "description": "Disables transformation of request URI paths with dot segments" } -] \ No newline at end of file +] diff --git a/src/Api/Serializer/RestSerializer.php b/src/Api/Serializer/RestSerializer.php index 73a82f7f1d..6713655767 100644 --- a/src/Api/Serializer/RestSerializer.php +++ b/src/Api/Serializer/RestSerializer.php @@ -248,14 +248,16 @@ function (array $matches) use ($varDefinitions) { $relative = substr($relative, 1); } - //Append path to endpoint when leading '//...' or - // '../' present as uri cannot be properly resolved + //Append path to endpoint when leading '//...' + // present as uri cannot be properly resolved if (($this->api->isModifiedModel() && strpos($relative, '//') === 0) ) { return new Uri($this->endpoint . $relative); } + //Append path to endpoint when a parent directory '../' + // reference is detected as uri cannot be properly resolved if ($this->api->getServiceName() === 's3' && strpos($relative, '../') !== false ){ From d70553122624e7005990ddeb2763aa05706692dd Mon Sep 17 00:00:00 2001 From: Sean O'Brien Date: Wed, 22 Nov 2023 13:51:39 -0500 Subject: [PATCH 3/9] updates --- src/Api/Serializer/RestSerializer.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Api/Serializer/RestSerializer.php b/src/Api/Serializer/RestSerializer.php index 6713655767..8e74a4e930 100644 --- a/src/Api/Serializer/RestSerializer.php +++ b/src/Api/Serializer/RestSerializer.php @@ -250,15 +250,15 @@ function (array $matches) use ($varDefinitions) { //Append path to endpoint when leading '//...' // present as uri cannot be properly resolved - if (($this->api->isModifiedModel() - && strpos($relative, '//') === 0) + if ($this->api->isModifiedModel() + && strpos($relative, '//') === 0 ) { return new Uri($this->endpoint . $relative); } //Append path to endpoint when a parent directory '../' // reference is detected as uri cannot be properly resolved - if ($this->api->getServiceName() === 's3' + if ($this->api->isModifiedModel() && strpos($relative, '../') !== false ){ return new Uri( From 8633cd7ccbbd08aab0d356ba280ac9ab6a795a6a Mon Sep 17 00:00:00 2001 From: Sean O'Brien Date: Wed, 22 Nov 2023 13:53:56 -0500 Subject: [PATCH 4/9] update --- src/Api/Serializer/RestSerializer.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/Api/Serializer/RestSerializer.php b/src/Api/Serializer/RestSerializer.php index 8e74a4e930..66845ef91a 100644 --- a/src/Api/Serializer/RestSerializer.php +++ b/src/Api/Serializer/RestSerializer.php @@ -261,10 +261,12 @@ function (array $matches) use ($varDefinitions) { if ($this->api->isModifiedModel() && strpos($relative, '../') !== false ){ + if ($relative && $relative[0] !== '/') { + $relative = '/' . $relative; + } + return new Uri( - $this->endpoint - . (strpos($relative, '/') !== 0 ? '/' : '') - . $relative + $this->endpoint . $relative ); } From 529d3b18b74367267274fbbab0220060f687ab34 Mon Sep 17 00:00:00 2001 From: Sean O'Brien Date: Wed, 22 Nov 2023 14:04:09 -0500 Subject: [PATCH 5/9] update --- src/Api/Serializer/RestSerializer.php | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/Api/Serializer/RestSerializer.php b/src/Api/Serializer/RestSerializer.php index 66845ef91a..bc81d73d53 100644 --- a/src/Api/Serializer/RestSerializer.php +++ b/src/Api/Serializer/RestSerializer.php @@ -242,6 +242,12 @@ function (array $matches) use ($varDefinitions) { $path = rtrim($path, '/'); } $relative = $path . $relative; + + if ($this->api->isModifiedModel() + && strpos($relative, '../') !== false + ){ + return new Uri($this->endpoint . $relative); + } } // If endpoint has path, remove leading '/' to preserve URI resolution. if ($path && $relative[0] === '/') { @@ -258,17 +264,15 @@ function (array $matches) use ($varDefinitions) { //Append path to endpoint when a parent directory '../' // reference is detected as uri cannot be properly resolved - if ($this->api->isModifiedModel() - && strpos($relative, '../') !== false - ){ - if ($relative && $relative[0] !== '/') { - $relative = '/' . $relative; - } - - return new Uri( - $this->endpoint . $relative - ); - } +// if ($this->api->isModifiedModel() +// && strpos($relative, '../') !== false +// ){ +// if ($relative && $relative[0] !== '/') { +// $relative = '/' . $relative; +// } +// +// return new Uri($this->endpoint . $relative); +// } // Expand path place holders using Amazon's slightly different URI // template syntax. From 449b0504fc350f393e0337f8679173dc9a82d8f5 Mon Sep 17 00:00:00 2001 From: Sean O'Brien Date: Wed, 22 Nov 2023 14:06:34 -0500 Subject: [PATCH 6/9] remove comment --- src/Api/Serializer/RestSerializer.php | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/Api/Serializer/RestSerializer.php b/src/Api/Serializer/RestSerializer.php index bc81d73d53..9103c690d7 100644 --- a/src/Api/Serializer/RestSerializer.php +++ b/src/Api/Serializer/RestSerializer.php @@ -262,18 +262,6 @@ function (array $matches) use ($varDefinitions) { return new Uri($this->endpoint . $relative); } - //Append path to endpoint when a parent directory '../' - // reference is detected as uri cannot be properly resolved -// if ($this->api->isModifiedModel() -// && strpos($relative, '../') !== false -// ){ -// if ($relative && $relative[0] !== '/') { -// $relative = '/' . $relative; -// } -// -// return new Uri($this->endpoint . $relative); -// } - // Expand path place holders using Amazon's slightly different URI // template syntax. return UriResolver::resolve($this->endpoint, new Uri($relative)); From 3624d50dacb5147180aade3dfddd2987d05019f5 Mon Sep 17 00:00:00 2001 From: Sean O'Brien Date: Wed, 22 Nov 2023 14:08:16 -0500 Subject: [PATCH 7/9] remove modifiedmodel check --- src/Api/Serializer/RestSerializer.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/Api/Serializer/RestSerializer.php b/src/Api/Serializer/RestSerializer.php index 9103c690d7..1183a10986 100644 --- a/src/Api/Serializer/RestSerializer.php +++ b/src/Api/Serializer/RestSerializer.php @@ -243,9 +243,7 @@ function (array $matches) use ($varDefinitions) { } $relative = $path . $relative; - if ($this->api->isModifiedModel() - && strpos($relative, '../') !== false - ){ + if (strpos($relative, '../') !== false) { return new Uri($this->endpoint . $relative); } } From ab331b9955c5b8a368adf122ed364196ab63ad15 Mon Sep 17 00:00:00 2001 From: Sean O'Brien Date: Wed, 22 Nov 2023 14:22:58 -0500 Subject: [PATCH 8/9] update forward slash logic --- src/Api/Serializer/RestSerializer.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/Api/Serializer/RestSerializer.php b/src/Api/Serializer/RestSerializer.php index 1183a10986..b0ec0bbe1a 100644 --- a/src/Api/Serializer/RestSerializer.php +++ b/src/Api/Serializer/RestSerializer.php @@ -244,6 +244,9 @@ function (array $matches) use ($varDefinitions) { $relative = $path . $relative; if (strpos($relative, '../') !== false) { + if ($relative && $relative[0] !== '/') { + $relative = '/' . $relative; + } return new Uri($this->endpoint . $relative); } } From f121e9e925b876afa92889108a29140c23a220df Mon Sep 17 00:00:00 2001 From: Sean O'Brien Date: Wed, 22 Nov 2023 14:25:48 -0500 Subject: [PATCH 9/9] remove check --- src/Api/Serializer/RestSerializer.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Api/Serializer/RestSerializer.php b/src/Api/Serializer/RestSerializer.php index b0ec0bbe1a..db431e5e72 100644 --- a/src/Api/Serializer/RestSerializer.php +++ b/src/Api/Serializer/RestSerializer.php @@ -244,7 +244,7 @@ function (array $matches) use ($varDefinitions) { $relative = $path . $relative; if (strpos($relative, '../') !== false) { - if ($relative && $relative[0] !== '/') { + if ($relative[0] !== '/') { $relative = '/' . $relative; } return new Uri($this->endpoint . $relative);