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

multipart uploads: Hack to support gsutil cp #1182

Merged
merged 2 commits into from
May 27, 2023
Merged

Conversation

ekimekim
Copy link
Contributor

gsutil sends an invalid multipart boundary param, which golang's mime.ParseMediaType correctly rejects.
However, the real GCS evidently does not reject this, so in order to make gsutil work we need to support it.

In particular, gsutil sends a boundary param that is quoted using single-quotes when it should be using double-quotes.
In cases where the param is definitely invalid (so we're guarenteed not to break any valid values), we replace all single-quotes with double-quotes to produce the intended meaning.

Upstream bug: GoogleCloudPlatform/gsutil#1466

Fixes #217
The above issue found this bug and worked around it, but found another problem and never submitted the fix.
It seems that in the intervening time, the other problem was fixed independently, and I can successfully gsutil cp using this code.

`gsutil` sends an invalid multipart boundary param, which golang's `mime.ParseMediaType` correctly rejects.
However, the real GCS evidently does not reject this, so in order to make `gsutil` work we need to support it.

In particular, `gsutil` sends a boundary param that is quoted using single-quotes when it should be using double-quotes.
In cases where the param is definitely invalid (so we're guarenteed not to break any valid values), we replace all single-quotes with double-quotes to produce the intended meaning.

Upstream bug: GoogleCloudPlatform/gsutil#1466
@ekimekim
Copy link
Contributor Author

hm, now I think about it more, it might be safer to only do the ' -> " transform if we first try and fail to parse the mime type. right now, a boundary value like boundary="boundary='=" would do the wrong thing.

Copy link
Owner

@fsouza fsouza left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

fakestorage/upload.go Outdated Show resolved Hide resolved
@fsouza
Copy link
Owner

fsouza commented May 27, 2023

hm, now I think about it more, it might be safer to only do the ' -> " transform if we first try and fail to parse the mime type. right now, a boundary value like boundary="boundary='=" would do the wrong thing.

Yeah, I wonder if we need something more specific, like a regexp replace that is specific to boundary='(.+)' or something like that.

@fsouza fsouza merged commit 6e130ca into fsouza:main May 27, 2023
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.

gsutil cp bad request
2 participants