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

don't allow leading slashes in s3 remote state key #15738

Merged
merged 1 commit into from
Aug 8, 2017

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Aug 4, 2017

S3 accepts objects with a leading slash and strips them off. This works
fine except in our workspace hierarchy, which then can no longer find
suffixes matching the full key name.

Fixes #15645

Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

LGTM. Silly nit-pick inline, which you can feel free to ignore if you disagree. 😀

// s3 will strip leading slashes from an object, so while this will
// technically be accepted by s3, it will break our workspace hierarchy.
if strings.HasPrefix(v.(string), "/") {
return nil, []error{fmt.Errorf("key cannot contain a leading '/'")}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is totally a nit, but "contain a leading slash" reads funny to me since I guess I don't think of a prefix as being "contained".

Suggest instead: "key must not start with '/'"

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, much better!

S3 accepts objects with a leading slash and strips them off. This works
fine except in our workspace hierarchy, which then can no longer find
suffixes matching the full key name.
@jbardin jbardin merged commit 5769ee7 into master Aug 8, 2017
@jbardin jbardin deleted the jbardin/s3-key-validation branch August 8, 2017 13:48
@ghost
Copy link

ghost commented Apr 7, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

S3 backend doesn't like a key starting in / (slash)
2 participants