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

api/project: Fixes issue handling project names with URI encodable characters #6081

Merged
merged 1 commit into from Sep 5, 2019

Conversation

@tomponline
Copy link
Member

commented Aug 14, 2019

It was possible to do:

lxc project create "test project"

But one could not then do:

lxc project delete "test project"

As the project name was encoded on the URL as "test+project", but was not decoded when being passed to the database query.

Also see https://discuss.linuxcontainers.org/t/cant-rename-image-alias/5638

Signed-off-by: Thomas Parrott thomas.parrott@canonical.com

@tomponline

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

@stgraber is this acceptable or should this be done a more generic way?

@tomponline tomponline force-pushed the tomponline:tp-api-projects-name branch from ce3c658 to 9353f56 Aug 14, 2019

@stgraber

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

@tomponline same appear true for at least lxc profile create too.

Since this realistically couldn't have really worked for anyone, I'd say, look at everywhere we allow creating things with spaces today and then add checks to prevent it.

We should already have a similar check for slashes in most spots (only exception I can think of would be image aliases).

@stgraber stgraber added the Incomplete label Aug 14, 2019

@lxc-jenkins

This comment has been minimized.

Copy link

commented Aug 14, 2019

Testsuite passed

@tomponline

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

@stgraber what would be an acceptable character list for name objects, perhaps a regex like [A-Za-z0-9]+?

@stgraber

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

@tomponline we're probably fine with everything but slashes and spaces. We probably want to be somewhat generous in what we allow just to avoid breaking people on upgrade.

@tomponline

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

@stgraber ok thanks, and I would combine that with the URI decoding in that case as it leaves open the possibility of users using characters that get URI encoded but are still allowed (like brackets, percent, ampersand etc).

@stgraber

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Yeah, that's fine. Clearly I fixed our client to properly URL encode the requests, so would make sense to URL decode on the receiving end. Not sure if there's some way to do that somewhat centrally though.

client: Uses PathEscape rather than QueryEscape for URL part parts
Leaves QueryEscape for query string parts though.

PathEscape correctly encodes "+" as "%20", which the LXD server correctly decoces back to "+".

QueryEscape encodes " " as "+", but the LXD server doesn't decode path parts using QueryUnescape so it comes through as "+".

Fixes #6081

Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>

@tomponline tomponline force-pushed the tomponline:tp-api-projects-name branch from 9353f56 to 935cb4b Sep 5, 2019

@tomponline tomponline requested a review from stgraber Sep 5, 2019

@lxc-jenkins

This comment has been minimized.

Copy link

commented Sep 5, 2019

Testsuite passed

@stgraber stgraber merged commit 0100f47 into lxc:master Sep 5, 2019

5 checks passed

Branch target Branch target is correct
Details
DCO All commits signed-off
Details
Testsuite Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tomponline tomponline deleted the tomponline:tp-api-projects-name branch Sep 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.