Skip to content

Commit

Permalink
Remove incorrect usage of url.PathEscape (#12184)
Browse files Browse the repository at this point in the history 
When r.toHTTP is called, http.Request is built with the path
already escaped. This removes all calls to url.PathEscape that
would have led to double-escaped URLs.
  • Loading branch information
Chris S. Kim authored Jan 25, 2022
1 parent d2f3665 commit f5d816d
Show file tree
Hide file tree
Showing 6 changed files with 41 additions and 46 deletions.
34 changes: 17 additions & 17 deletions api/acl.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -552,7 +552,7 @@ func (a *ACL) Update(acl *ACLEntry, q *WriteOptions) (*WriteMeta, error) {
//
// Deprecated: Use TokenDelete instead.
func (a *ACL) Destroy(id string, q *WriteOptions) (*WriteMeta, error) {
r := a.c.newRequest("PUT", "/v1/acl/destroy/"+url.PathEscape(id))
r := a.c.newRequest("PUT", "/v1/acl/destroy/"+id)
r.setWriteOptions(q)
rtt, resp, err := a.c.doRequest(r)
if err != nil {
Expand All @@ -571,7 +571,7 @@ func (a *ACL) Destroy(id string, q *WriteOptions) (*WriteMeta, error) {
//
// Deprecated: Use TokenClone instead.
func (a *ACL) Clone(id string, q *WriteOptions) (string, *WriteMeta, error) {
r := a.c.newRequest("PUT", "/v1/acl/clone/"+url.PathEscape(id))
r := a.c.newRequest("PUT", "/v1/acl/clone/"+id)
r.setWriteOptions(q)
rtt, resp, err := a.c.doRequest(r)
if err != nil {
Expand All @@ -594,7 +594,7 @@ func (a *ACL) Clone(id string, q *WriteOptions) (string, *WriteMeta, error) {
//
// Deprecated: Use TokenRead instead.
func (a *ACL) Info(id string, q *QueryOptions) (*ACLEntry, *QueryMeta, error) {
r := a.c.newRequest("GET", "/v1/acl/info/"+url.PathEscape(id))
r := a.c.newRequest("GET", "/v1/acl/info/"+id)
r.setQueryOptions(q)
rtt, resp, err := a.c.doRequest(r)
if err != nil {
Expand Down Expand Up @@ -696,7 +696,7 @@ func (a *ACL) TokenUpdate(token *ACLToken, q *WriteOptions) (*ACLToken, *WriteMe
if token.AccessorID == "" {
return nil, nil, fmt.Errorf("Must specify an AccessorID for Token Updating")
}
r := a.c.newRequest("PUT", "/v1/acl/token/"+url.PathEscape(token.AccessorID))
r := a.c.newRequest("PUT", "/v1/acl/token/"+token.AccessorID)
r.setWriteOptions(q)
r.obj = token
rtt, resp, err := a.c.doRequest(r)
Expand Down Expand Up @@ -725,7 +725,7 @@ func (a *ACL) TokenClone(tokenID string, description string, q *WriteOptions) (*
return nil, nil, fmt.Errorf("Must specify a tokenID for Token Cloning")
}

r := a.c.newRequest("PUT", "/v1/acl/token/"+url.PathEscape(tokenID)+"/clone")
r := a.c.newRequest("PUT", "/v1/acl/token/"+tokenID+"/clone")
r.setWriteOptions(q)
r.obj = struct{ Description string }{description}
rtt, resp, err := a.c.doRequest(r)
Expand All @@ -748,7 +748,7 @@ func (a *ACL) TokenClone(tokenID string, description string, q *WriteOptions) (*
// TokenDelete removes a single ACL token. The tokenID parameter must be a valid
// Accessor ID of an existing token.
func (a *ACL) TokenDelete(tokenID string, q *WriteOptions) (*WriteMeta, error) {
r := a.c.newRequest("DELETE", "/v1/acl/token/"+url.PathEscape(tokenID))
r := a.c.newRequest("DELETE", "/v1/acl/token/"+tokenID)
r.setWriteOptions(q)
rtt, resp, err := a.c.doRequest(r)
if err != nil {
Expand All @@ -766,7 +766,7 @@ func (a *ACL) TokenDelete(tokenID string, q *WriteOptions) (*WriteMeta, error) {
// TokenRead retrieves the full token details. The tokenID parameter must be a valid
// Accessor ID of an existing token.
func (a *ACL) TokenRead(tokenID string, q *QueryOptions) (*ACLToken, *QueryMeta, error) {
r := a.c.newRequest("GET", "/v1/acl/token/"+url.PathEscape(tokenID))
r := a.c.newRequest("GET", "/v1/acl/token/"+tokenID)
r.setQueryOptions(q)
rtt, resp, err := a.c.doRequest(r)
if err != nil {
Expand Down Expand Up @@ -871,7 +871,7 @@ func (a *ACL) PolicyUpdate(policy *ACLPolicy, q *WriteOptions) (*ACLPolicy, *Wri
return nil, nil, fmt.Errorf("Must specify an ID in Policy Update")
}

r := a.c.newRequest("PUT", "/v1/acl/policy/"+url.PathEscape(policy.ID))
r := a.c.newRequest("PUT", "/v1/acl/policy/"+policy.ID)
r.setWriteOptions(q)
r.obj = policy
rtt, resp, err := a.c.doRequest(r)
Expand All @@ -893,7 +893,7 @@ func (a *ACL) PolicyUpdate(policy *ACLPolicy, q *WriteOptions) (*ACLPolicy, *Wri

// PolicyDelete deletes a policy given its ID.
func (a *ACL) PolicyDelete(policyID string, q *WriteOptions) (*WriteMeta, error) {
r := a.c.newRequest("DELETE", "/v1/acl/policy/"+url.PathEscape(policyID))
r := a.c.newRequest("DELETE", "/v1/acl/policy/"+policyID)
r.setWriteOptions(q)
rtt, resp, err := a.c.doRequest(r)
if err != nil {
Expand All @@ -910,7 +910,7 @@ func (a *ACL) PolicyDelete(policyID string, q *WriteOptions) (*WriteMeta, error)

// PolicyRead retrieves the policy details including the rule set.
func (a *ACL) PolicyRead(policyID string, q *QueryOptions) (*ACLPolicy, *QueryMeta, error) {
r := a.c.newRequest("GET", "/v1/acl/policy/"+url.PathEscape(policyID))
r := a.c.newRequest("GET", "/v1/acl/policy/"+policyID)
r.setQueryOptions(q)
rtt, resp, err := a.c.doRequest(r)
if err != nil {
Expand Down Expand Up @@ -1021,7 +1021,7 @@ func (a *ACL) RulesTranslate(rules io.Reader) (string, error) {
// Deprecated: Support for the legacy syntax translation will be removed
// when legacy ACL support is removed.
func (a *ACL) RulesTranslateToken(tokenID string) (string, error) {
r := a.c.newRequest("GET", "/v1/acl/rules/translate/"+url.PathEscape(tokenID))
r := a.c.newRequest("GET", "/v1/acl/rules/translate/"+tokenID)
rtt, resp, err := a.c.doRequest(r)
if err != nil {
return "", err
Expand Down Expand Up @@ -1076,7 +1076,7 @@ func (a *ACL) RoleUpdate(role *ACLRole, q *WriteOptions) (*ACLRole, *WriteMeta,
return nil, nil, fmt.Errorf("Must specify an ID in Role Update")
}

r := a.c.newRequest("PUT", "/v1/acl/role/"+url.PathEscape(role.ID))
r := a.c.newRequest("PUT", "/v1/acl/role/"+role.ID)
r.setWriteOptions(q)
r.obj = role
rtt, resp, err := a.c.doRequest(r)
Expand All @@ -1098,7 +1098,7 @@ func (a *ACL) RoleUpdate(role *ACLRole, q *WriteOptions) (*ACLRole, *WriteMeta,

// RoleDelete deletes a role given its ID.
func (a *ACL) RoleDelete(roleID string, q *WriteOptions) (*WriteMeta, error) {
r := a.c.newRequest("DELETE", "/v1/acl/role/"+url.PathEscape(roleID))
r := a.c.newRequest("DELETE", "/v1/acl/role/"+roleID)
r.setWriteOptions(q)
rtt, resp, err := a.c.doRequest(r)
if err != nil {
Expand All @@ -1115,7 +1115,7 @@ func (a *ACL) RoleDelete(roleID string, q *WriteOptions) (*WriteMeta, error) {

// RoleRead retrieves the role details (by ID). Returns nil if not found.
func (a *ACL) RoleRead(roleID string, q *QueryOptions) (*ACLRole, *QueryMeta, error) {
r := a.c.newRequest("GET", "/v1/acl/role/"+url.PathEscape(roleID))
r := a.c.newRequest("GET", "/v1/acl/role/"+roleID)
r.setQueryOptions(q)
rtt, resp, err := a.c.doRequest(r)
if err != nil {
Expand Down Expand Up @@ -1365,7 +1365,7 @@ func (a *ACL) BindingRuleUpdate(rule *ACLBindingRule, q *WriteOptions) (*ACLBind
return nil, nil, fmt.Errorf("Must specify an ID in Binding Rule Update")
}

r := a.c.newRequest("PUT", "/v1/acl/binding-rule/"+url.PathEscape(rule.ID))
r := a.c.newRequest("PUT", "/v1/acl/binding-rule/"+rule.ID)
r.setWriteOptions(q)
r.obj = rule
rtt, resp, err := a.c.doRequest(r)
Expand All @@ -1387,7 +1387,7 @@ func (a *ACL) BindingRuleUpdate(rule *ACLBindingRule, q *WriteOptions) (*ACLBind

// BindingRuleDelete deletes a binding rule given its ID.
func (a *ACL) BindingRuleDelete(bindingRuleID string, q *WriteOptions) (*WriteMeta, error) {
r := a.c.newRequest("DELETE", "/v1/acl/binding-rule/"+url.PathEscape(bindingRuleID))
r := a.c.newRequest("DELETE", "/v1/acl/binding-rule/"+bindingRuleID)
r.setWriteOptions(q)
rtt, resp, err := a.c.doRequest(r)
if err != nil {
Expand All @@ -1404,7 +1404,7 @@ func (a *ACL) BindingRuleDelete(bindingRuleID string, q *WriteOptions) (*WriteMe

// BindingRuleRead retrieves the binding rule details. Returns nil if not found.
func (a *ACL) BindingRuleRead(bindingRuleID string, q *QueryOptions) (*ACLBindingRule, *QueryMeta, error) {
r := a.c.newRequest("GET", "/v1/acl/binding-rule/"+url.PathEscape(bindingRuleID))
r := a.c.newRequest("GET", "/v1/acl/binding-rule/"+bindingRuleID)
r.setQueryOptions(q)
rtt, resp, err := a.c.doRequest(r)
if err != nil {
Expand Down
29 changes: 14 additions & 15 deletions api/agent.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"fmt"
"io"
"net/http"
"net/url"
)

// ServiceKind is the kind of service being registered.
Expand Down Expand Up @@ -628,7 +627,7 @@ func (a *Agent) AgentHealthServiceByID(serviceID string) (string, *AgentServiceC
}

func (a *Agent) AgentHealthServiceByIDOpts(serviceID string, q *QueryOptions) (string, *AgentServiceChecksInfo, error) {
path := fmt.Sprintf("/v1/agent/health/service/id/%v", url.PathEscape(serviceID))
path := fmt.Sprintf("/v1/agent/health/service/id/%v", serviceID)
r := a.c.newRequest("GET", path)
r.setQueryOptions(q)
r.params.Add("format", "json")
Expand Down Expand Up @@ -669,7 +668,7 @@ func (a *Agent) AgentHealthServiceByName(service string) (string, []AgentService
}

func (a *Agent) AgentHealthServiceByNameOpts(service string, q *QueryOptions) (string, []AgentServiceChecksInfo, error) {
path := fmt.Sprintf("/v1/agent/health/service/name/%v", url.PathEscape(service))
path := fmt.Sprintf("/v1/agent/health/service/name/%v", service)
r := a.c.newRequest("GET", path)
r.setQueryOptions(q)
r.params.Add("format", "json")
Expand Down Expand Up @@ -707,7 +706,7 @@ func (a *Agent) AgentHealthServiceByNameOpts(service string, q *QueryOptions) (s
// agent-local state. That means there is no persistent raft index so we block
// based on object hash instead.
func (a *Agent) Service(serviceID string, q *QueryOptions) (*AgentService, *QueryMeta, error) {
r := a.c.newRequest("GET", "/v1/agent/service/"+url.PathEscape(serviceID))
r := a.c.newRequest("GET", "/v1/agent/service/"+serviceID)
r.setQueryOptions(q)
rtt, resp, err := a.c.doRequest(r)
if err != nil {
Expand Down Expand Up @@ -812,7 +811,7 @@ func (a *Agent) serviceRegister(service *AgentServiceRegistration, opts ServiceR
// ServiceDeregister is used to deregister a service with
// the local agent
func (a *Agent) ServiceDeregister(serviceID string) error {
r := a.c.newRequest("PUT", "/v1/agent/service/deregister/"+url.PathEscape(serviceID))
r := a.c.newRequest("PUT", "/v1/agent/service/deregister/"+serviceID)
_, resp, err := a.c.doRequest(r)
if err != nil {
return err
Expand All @@ -827,7 +826,7 @@ func (a *Agent) ServiceDeregister(serviceID string) error {
// ServiceDeregisterOpts is used to deregister a service with
// the local agent with QueryOptions.
func (a *Agent) ServiceDeregisterOpts(serviceID string, q *QueryOptions) error {
r := a.c.newRequest("PUT", "/v1/agent/service/deregister/"+url.PathEscape(serviceID))
r := a.c.newRequest("PUT", "/v1/agent/service/deregister/"+serviceID)
r.setQueryOptions(q)
_, resp, err := a.c.doRequest(r)
if err != nil {
Expand Down Expand Up @@ -884,7 +883,7 @@ func (a *Agent) updateTTL(checkID, note, status string) error {
default:
return fmt.Errorf("Invalid status: %s", status)
}
endpoint := fmt.Sprintf("/v1/agent/check/%s/%s", url.PathEscape(status), url.PathEscape(checkID))
endpoint := fmt.Sprintf("/v1/agent/check/%s/%s", status, checkID)
r := a.c.newRequest("PUT", endpoint)
r.params.Set("note", note)
_, resp, err := a.c.doRequest(r)
Expand Down Expand Up @@ -932,7 +931,7 @@ func (a *Agent) UpdateTTLOpts(checkID, output, status string, q *QueryOptions) e
return fmt.Errorf("Invalid status: %s", status)
}

endpoint := fmt.Sprintf("/v1/agent/check/update/%s", url.PathEscape(checkID))
endpoint := fmt.Sprintf("/v1/agent/check/update/%s", checkID)
r := a.c.newRequest("PUT", endpoint)
r.setQueryOptions(q)
r.obj = &checkUpdate{
Expand Down Expand Up @@ -976,7 +975,7 @@ func (a *Agent) CheckDeregister(checkID string) error {
// CheckDeregisterOpts is used to deregister a check with
// the local agent using query options
func (a *Agent) CheckDeregisterOpts(checkID string, q *QueryOptions) error {
r := a.c.newRequest("PUT", "/v1/agent/check/deregister/"+url.PathEscape(checkID))
r := a.c.newRequest("PUT", "/v1/agent/check/deregister/"+checkID)
r.setQueryOptions(q)
_, resp, err := a.c.doRequest(r)
if err != nil {
Expand All @@ -992,7 +991,7 @@ func (a *Agent) CheckDeregisterOpts(checkID string, q *QueryOptions) error {
// Join is used to instruct the agent to attempt a join to
// another cluster member
func (a *Agent) Join(addr string, wan bool) error {
r := a.c.newRequest("PUT", "/v1/agent/join/"+url.PathEscape(addr))
r := a.c.newRequest("PUT", "/v1/agent/join/"+addr)
if wan {
r.params.Set("wan", "1")
}
Expand Down Expand Up @@ -1044,7 +1043,7 @@ func (a *Agent) ForceLeavePrune(node string) error {
// ForceLeaveOpts is used to have the agent eject a failed node or remove it
// completely from the list of members.
func (a *Agent) ForceLeaveOpts(node string, opts ForceLeaveOpts) error {
r := a.c.newRequest("PUT", "/v1/agent/force-leave/"+url.PathEscape(node))
r := a.c.newRequest("PUT", "/v1/agent/force-leave/"+node)
if opts.Prune {
r.params.Set("prune", "1")
}
Expand Down Expand Up @@ -1108,7 +1107,7 @@ func (a *Agent) ConnectCARoots(q *QueryOptions) (*CARootList, *QueryMeta, error)

// ConnectCALeaf gets the leaf certificate for the given service ID.
func (a *Agent) ConnectCALeaf(serviceID string, q *QueryOptions) (*LeafCert, *QueryMeta, error) {
r := a.c.newRequest("GET", "/v1/agent/connect/ca/leaf/"+url.PathEscape(serviceID))
r := a.c.newRequest("GET", "/v1/agent/connect/ca/leaf/"+serviceID)
r.setQueryOptions(q)
rtt, resp, err := a.c.doRequest(r)
if err != nil {
Expand Down Expand Up @@ -1136,7 +1135,7 @@ func (a *Agent) EnableServiceMaintenance(serviceID, reason string) error {
}

func (a *Agent) EnableServiceMaintenanceOpts(serviceID, reason string, q *QueryOptions) error {
r := a.c.newRequest("PUT", "/v1/agent/service/maintenance/"+url.PathEscape(serviceID))
r := a.c.newRequest("PUT", "/v1/agent/service/maintenance/"+serviceID)
r.setQueryOptions(q)
r.params.Set("enable", "true")
r.params.Set("reason", reason)
Expand All @@ -1158,7 +1157,7 @@ func (a *Agent) DisableServiceMaintenance(serviceID string) error {
}

func (a *Agent) DisableServiceMaintenanceOpts(serviceID string, q *QueryOptions) error {
r := a.c.newRequest("PUT", "/v1/agent/service/maintenance/"+url.PathEscape(serviceID))
r := a.c.newRequest("PUT", "/v1/agent/service/maintenance/"+serviceID)
r.setQueryOptions(q)
r.params.Set("enable", "false")
_, resp, err := a.c.doRequest(r)
Expand Down Expand Up @@ -1355,7 +1354,7 @@ func (a *Agent) updateTokenFallback(token string, q *WriteOptions, targets ...st
}

func (a *Agent) updateTokenOnce(target, token string, q *WriteOptions) (*WriteMeta, int, error) {
r := a.c.newRequest("PUT", fmt.Sprintf("/v1/agent/token/%s", url.PathEscape(target)))
r := a.c.newRequest("PUT", fmt.Sprintf("/v1/agent/token/%s", target))
r.setWriteOptions(q)
r.obj = &AgentToken{Token: token}

Expand Down
11 changes: 5 additions & 6 deletions api/catalog.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package api

import (
"net"
"net/url"
"strconv"
)

Expand Down Expand Up @@ -255,9 +254,9 @@ func (c *Catalog) ConnectMultipleTags(service string, tags []string, q *QueryOpt
}

func (c *Catalog) service(service string, tags []string, q *QueryOptions, connect bool) ([]*CatalogService, *QueryMeta, error) {
path := "/v1/catalog/service/" + url.PathEscape(service)
path := "/v1/catalog/service/" + service
if connect {
path = "/v1/catalog/connect/" + url.PathEscape(service)
path = "/v1/catalog/connect/" + service
}
r := c.c.newRequest("GET", path)
r.setQueryOptions(q)
Expand Down Expand Up @@ -288,7 +287,7 @@ func (c *Catalog) service(service string, tags []string, q *QueryOptions, connec

// Node is used to query for service information about a single node
func (c *Catalog) Node(node string, q *QueryOptions) (*CatalogNode, *QueryMeta, error) {
r := c.c.newRequest("GET", "/v1/catalog/node/"+url.PathEscape(node))
r := c.c.newRequest("GET", "/v1/catalog/node/"+node)
r.setQueryOptions(q)
rtt, resp, err := c.c.doRequest(r)
if err != nil {
Expand All @@ -315,7 +314,7 @@ func (c *Catalog) Node(node string, q *QueryOptions) (*CatalogNode, *QueryMeta,
// a map of service ids to services. This different structure allows for using the wildcard specifier
// '*' for the Namespace in the QueryOptions.
func (c *Catalog) NodeServiceList(node string, q *QueryOptions) (*CatalogNodeServiceList, *QueryMeta, error) {
r := c.c.newRequest("GET", "/v1/catalog/node-services/"+url.PathEscape(node))
r := c.c.newRequest("GET", "/v1/catalog/node-services/"+node)
r.setQueryOptions(q)
rtt, resp, err := c.c.doRequest(r)
if err != nil {
Expand All @@ -339,7 +338,7 @@ func (c *Catalog) NodeServiceList(node string, q *QueryOptions) (*CatalogNodeSer

// GatewayServices is used to query the services associated with an ingress gateway or terminating gateway.
func (c *Catalog) GatewayServices(gateway string, q *QueryOptions) ([]*GatewayService, *QueryMeta, error) {
r := c.c.newRequest("GET", "/v1/catalog/gateway-services/"+url.PathEscape(gateway))
r := c.c.newRequest("GET", "/v1/catalog/gateway-services/"+gateway)
r.setQueryOptions(q)
rtt, resp, err := c.c.doRequest(r)
if err != nil {
Expand Down
7 changes: 3 additions & 4 deletions api/config_entry.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"encoding/json"
"fmt"
"io"
"net/url"
"strconv"
"strings"
"time"
Expand Down Expand Up @@ -379,7 +378,7 @@ func (conf *ConfigEntries) Get(kind string, name string, q *QueryOptions) (Confi
return nil, nil, err
}

r := conf.c.newRequest("GET", fmt.Sprintf("/v1/config/%s/%s", url.PathEscape(kind), url.PathEscape(name)))
r := conf.c.newRequest("GET", fmt.Sprintf("/v1/config/%s/%s", kind, name))
r.setQueryOptions(q)
rtt, resp, err := conf.c.doRequest(r)
if err != nil {
Expand All @@ -406,7 +405,7 @@ func (conf *ConfigEntries) List(kind string, q *QueryOptions) ([]ConfigEntry, *Q
return nil, nil, fmt.Errorf("The kind parameter must not be empty")
}

r := conf.c.newRequest("GET", fmt.Sprintf("/v1/config/%s", url.PathEscape(kind)))
r := conf.c.newRequest("GET", fmt.Sprintf("/v1/config/%s", kind))
r.setQueryOptions(q)
rtt, resp, err := conf.c.doRequest(r)
if err != nil {
Expand Down Expand Up @@ -486,7 +485,7 @@ func (conf *ConfigEntries) delete(kind, name string, params map[string]string, w
return false, nil, fmt.Errorf("Both kind and name parameters must not be empty")
}

r := conf.c.newRequest("DELETE", fmt.Sprintf("/v1/config/%s/%s", url.PathEscape(kind), url.PathEscape(name)))
r := conf.c.newRequest("DELETE", fmt.Sprintf("/v1/config/%s/%s", kind, name))
r.setWriteOptions(w)
for param, value := range params {
r.params.Set(param, value)
Expand Down
3 changes: 1 addition & 2 deletions api/coordinate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package api

import (
"github.com/hashicorp/serf/coordinate"
"net/url"
)

// CoordinateEntry represents a node and its associated network coordinate.
Expand Down Expand Up @@ -97,7 +96,7 @@ func (c *Coordinate) Update(coord *CoordinateEntry, q *WriteOptions) (*WriteMeta

// Node is used to return the coordinates of a single node in the LAN pool.
func (c *Coordinate) Node(node string, q *QueryOptions) ([]*CoordinateEntry, *QueryMeta, error) {
r := c.c.newRequest("GET", "/v1/coordinate/node/"+url.PathEscape(node))
r := c.c.newRequest("GET", "/v1/coordinate/node/"+node)
r.setQueryOptions(q)
rtt, resp, err := c.c.doRequest(r)
if err != nil {
Expand Down
Loading

0 comments on commit f5d816d

Please sign in to comment.