Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bugfix: dot segment handling #2835

Merged
merged 9 commits into from
Nov 22, 2023
Merged

Conversation

stobrien89
Copy link
Member

Description of changes:
Bypasses Guzzle's UriResolver for request paths that contain ../ parent path references. Previously, the UriResolver would strip dot segments, leading to a situation where objects other than the intended target could be created or accessed.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

&& strpos($relative, '//') === 0
//Append path to endpoint when leading '//...' or
// '../' present as uri cannot be properly resolved
if (($this->api->isModifiedModel()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these parentheses look like they were added by mistake

){
return new Uri(
$this->endpoint
. (strpos($relative, '/') !== 0 ? '/' : '')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is just making relative start with a slash if it doesn't? I might refactor this to say

if ($relative && $relative[0] != '/') {
     $relative = '/' . $relative;
}
 return new Uri($this->endpoint . $relative);

if you think that looks better.

Copy link
Member

@SamRemis SamRemis Nov 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, could we just move this up to the if statement above and short-circuit this around line 244?

@stobrien89 stobrien89 merged commit aebc9f8 into aws:master Nov 22, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants