objects: Escape names in Gophercloud#2821
Merged
EmilienM merged 1 commit intogophercloud:masterfrom Oct 31, 2023
Merged
Conversation
Member
Author
|
TODO:
|
6425888 to
700520e
Compare
700520e to
a1d5368
Compare
d4341f5 to
8f398f8
Compare
8f398f8 to
7c35633
Compare
7c35633 to
04e5b36
Compare
d08c293 to
bcebe30
Compare
bcebe30 to
89ae5cd
Compare
89ae5cd to
50a9cf7
Compare
b5c0fe2 to
0254522
Compare
0254522 to
7fa945d
Compare
7fa945d to
26ef2f7
Compare
26ef2f7 to
104cb14
Compare
104cb14 to
3e398db
Compare
3e398db to
4c35c1f
Compare
With this change, container and object names passed to the Gophercloud functions in `objectstorage/v1/containers` and `objectstorage/v1/objects` are now URL-escaped. If you were escaping them prior to passing them to Gophercloud, you should now remove escaping and pass the intended name to Gophercloud directly. **This is a breaking change.** API CHANGES: * Before this change, container and object names were sent as-is to Swift. With this change, **Gophercloud now escapes container and object names in all `objects` and `containers` functions**. If you were previously escaping names ( with, for example: `url.PathEscape` or `url.QueryEscape`), then you should REMOVE that and pass the intended names to Gophercloud directly. * The **`containers.ListOpts#Full` and `objects.ListOpts#Full` properties are REMOVED** from the Gophercloud API. The reason for that is: plaintext listing is unfixably wrong and won't handle special characters reliably (i.e. `\n`). * Empty container names, container names containing a slash (`/`), and empty object names are now rejected in Gophercloud before any call to Swift. * `containers.ErrInvalidContainerName` is now `v1.ErrInvalidContainerName` * New name validation errors: * `v1.ErrEmptyContainerName` * `v1.ErrEmptyObjectName` * In `objects.Copy`: the `destination` field (e.g. `objects.CopyOpts#Destination`) must be in the form `/container/object`: the function will reject a destination path if it doesn't start with a slash (`/`). CODE CHANGES: * `containers.List` and `objects.List` are now always setting `accept: application/json`. There is no plaintext listing any more, for the reason detailed above. * `containers` and `objects` acceptance tests are now run with nasty Unicode code points (including European characters, a space, "\n", backslash, slash where appropriate, emojis, emojis with modifiers, Chinese traditional characters), instead of only using nice ASCII characters. * Container and object names are now systematically validated before being sent to Swift. INCIDENTAL FIX: * it is now possible to successfuly call `objects.CreateTempURL` with a container name or object name containing the string `/v1/`.
4c35c1f to
9d0243f
Compare
pierreprinetti
commented
Oct 26, 2023
| // Copy the contents of one object to another. | ||
| copyOpts := objects.CopyOpts{ | ||
| Destination: cName + "/" + oNames[1], | ||
| Destination: "/" + cName + "/" + oNames[1], |
Member
Author
There was a problem hiding this comment.
As per the docs: the Copy Destination header must be in the form of /container/object.
|
|
||
| // List all created objects | ||
| listOpts := objects.ListOpts{ | ||
| Full: true, |
Member
Author
There was a problem hiding this comment.
List option Full: false (accept: text/plain) doesn't work and should not be used
Comment on lines
+33
to
+36
| for i := 0; i < len(oNames)-1; i++ { | ||
| oNames[i] = "test-object-" + tools.RandomFunnyString(8) | ||
| } | ||
| oNames[len(oNames)-1] = "test-object-with-/v1/-in-the-name" |
Member
Author
There was a problem hiding this comment.
There were two test cases. Now there are three: the last one tests that we correctly split the object path in CreateTempURL.
| }() | ||
|
|
||
| listOpts := objects.ListOpts{ | ||
| Full: true, |
Member
Author
There was a problem hiding this comment.
List option Full: false (accept: text/plain) doesn't work and should not be used. We always list "full" now (accept: application/json).
| } | ||
|
|
||
| listOpts = objects.ListOpts{ | ||
| Full: true, |
Member
Author
There was a problem hiding this comment.
List option Full: false (accept: text/plain) doesn't work and should not be used. We always list "full" now (accept: application/json).
|
|
||
| const forbiddenContainerRunes = "/" | ||
|
|
||
| func CheckContainerName(s string) error { |
Member
Author
There was a problem hiding this comment.
Moved to the parent directory v1
|
|
||
| // Copy is a function that copies one object to another. | ||
| func Copy(c *gophercloud.ServiceClient, containerName, objectName string, opts CopyOptsBuilder) (r CopyResult) { | ||
| url, err := copyURL(c, containerName, objectName) |
Member
Author
There was a problem hiding this comment.
moving this just a bit down below, just because url shadows the net/url package which we need for escaping.
| if err != nil { | ||
| return "", err | ||
| } | ||
| urlToBeSigned, err := tempURL(c, containerName, objectName) |
Member
Author
There was a problem hiding this comment.
We need two different URLs: one with non-escaped names (which will be signed), and one with escaped names (which will be used as the actual URL to send back to the caller). Documented here: https://docs.openstack.org/swift/latest/api/temporary_url_middleware.html#hmac-signature-for-temporary-urls
| } | ||
|
|
||
| secretKey := []byte(tempURLKey) | ||
| splitPath := strings.Split(url, opts.Split) |
Member
Author
There was a problem hiding this comment.
incidental fix: Split splits url each time opts.Split (default: /v1/) is found. But below, we only joined the first two parts back into a full URL. Which means that a path like https://swift/v1/AUTH_123/container/my/v1/object became https://swift/v1/AUTH_123/container/my (see #2827)
| // ExpectedListNames is the result expected from a call to `List` when just | ||
| // object names are requested. | ||
| var ExpectedListNames = []string{"hello", "goodbye"} | ||
| var ExpectedListNames = []string{"goodbye", "hello"} |
Member
Author
There was a problem hiding this comment.
I am not sure how this ever worked. Objects are always returned lexicographically sorted.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
With this change, container and object names passed to the Gophercloud functions in
objectstorage/v1/containersandobjectstorage/v1/objectsare now URL-escaped. If you were escaping them prior to passing them to Gophercloud, you should now remove escaping and pass the intended name to Gophercloud directly.This is a breaking change.
API CHANGES:
objectsandcontainersfunctions. If you were previously escaping names ( with, for example:url.PathEscapeorurl.QueryEscape), then you should REMOVE that and pass the intended names to Gophercloud directlycontainers.ListOpts#Fullandobjects.ListOpts#Fullproperties are REMOVED from the Gophercloud API. The reason for that is: plaintext listing is unfixably wrong and won't handle special characters reliably (i.e.\n)./), and empty object names are now rejected in Gophercloud before any call to Swift.containers.ErrInvalidContainerNameis nowv1.ErrInvalidContainerNamev1.ErrEmptyContainerNamev1.ErrEmptyObjectNameobjects.Copy: thedestinationfield (e.g.objects.CopyOpts#Destination) must be in the form/container/object: the function will reject a destination path if it doesn't start with a slash (/).CODE CHANGES:
containers.Listandobjects.Listare now always settingaccept: application/json. There is no plaintext listing any more, for the reason detailed above.containersandobjectsacceptance tests are now run with nasty Unicode code points (including European characters, a space, "\n", backslash, slash where appropriate, emojis, emojis with modifiers, Chinese traditional characters), instead of only using nice ASCII characters.INCIDENTAL FIX:
objects.CreateTempURLwith a container name or object name containing the string/v1/.Fixes #2816
Fixes #2547
Fixes #2827