Code review comment for lp:~rvb/gwacl/delete-deployment

Revision history for this message
Gavin Panella (allenap) wrote :

Looks good, but [1] is something we need to address before this
library is ready for production.

[1]

+    URI := "services/hostedservices/" + serviceName + "/deployments/" + deploymentName

serviceName and deploymentName ought to be passed through
url.QueryEscape... but then that will cause double escaping if set as
Path on a url.URL instance. Perhaps what needs to be done is to
replace / with %2F, *then* set as the Path. Something like:

    uri := url.Parse(base_url)
    uri.Path = (
        "services/hostedservices/" +
        strings.Replace(serviceName, "/", "%2F") +
        "/deployments/" +
        strings.Replace(deploymentName, "/", "%2F"))
    return api.session.delete(uri.String())

(Fwiw, I gleaned this from reading url.go alongside §3.3 of RFC 3986.)

This is not a problem unique to this branch, so I don't really expect
you to fix it, but we really ought to before stamping this with a 1.0
version.

Fwiw, a poor-man's fix would be to ensure that we (a) don't allow
forward-slash in user-supplied parts of URLs, and (b) ensure that URLs
are put together using url.URL so that proper escaping takes place.

review: Approve

« Back to merge proposal