Merge pull request #11335 from littlestar642/url-encoded-args

URL-encode/decode resource names for HTTP API

.changelog/11335.txt

@@ -0,0 +1,3 @@
+```release-note:enhancement
+api: URL-encode/decode resource names for v1/agent endpoints in API
+```

agent/agent_endpoint.go

@@ -379,7 +379,10 @@ func (s *HTTPHandlers) AgentServices(resp http.ResponseWriter, req *http.Request
 // blocking watch using hash-based blocking.
 func (s *HTTPHandlers) AgentService(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
 	// Get the proxy ID. Note that this is the ID of a proxy's service instance.
-	id := strings.TrimPrefix(req.URL.Path, "/v1/agent/service/")
+	id, err := getPathSuffixUnescaped(req.URL.Path, "/v1/agent/service/")
+	if err != nil {
+		return nil, err
+	}
 
 	// Maybe block
 	var queryOpts structs.QueryOptions
@@ -398,7 +401,7 @@ func (s *HTTPHandlers) AgentService(resp http.ResponseWriter, req *http.Request)
 	}
 
 	// need to resolve to default the meta
-	_, err := s.agent.delegate.ResolveTokenAndDefaultMeta(token, &entMeta, nil)
+	_, err = s.agent.delegate.ResolveTokenAndDefaultMeta(token, &entMeta, nil)
 	if err != nil {
 		return nil, err
 	}
@@ -637,7 +640,10 @@ func (s *HTTPHandlers) AgentJoin(resp http.ResponseWriter, req *http.Request) (i
 	}
 
 	// Get the address
-	addr := strings.TrimPrefix(req.URL.Path, "/v1/agent/join/")
+	addr, err := getPathSuffixUnescaped(req.URL.Path, "/v1/agent/join/")
+	if err != nil {
+		return nil, err
+	}
 
 	if wan {
 		if s.agent.config.ConnectMeshGatewayWANFederationEnabled {
@@ -697,7 +703,10 @@ func (s *HTTPHandlers) AgentForceLeave(resp http.ResponseWriter, req *http.Reque
 	// Check if the WAN is being queried
 	_, wan := req.URL.Query()["wan"]
 
-	addr := strings.TrimPrefix(req.URL.Path, "/v1/agent/force-leave/")
+	addr, err := getPathSuffixUnescaped(req.URL.Path, "/v1/agent/force-leave/")
+	if err != nil {
+		return nil, err
+	}
 	if wan {
 		return nil, s.agent.ForceLeaveWAN(addr, prune, entMeta)
 	} else {
@@ -783,7 +792,11 @@ func (s *HTTPHandlers) AgentRegisterCheck(resp http.ResponseWriter, req *http.Re
 }
 
 func (s *HTTPHandlers) AgentDeregisterCheck(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
-	checkID := structs.NewCheckID(types.CheckID(strings.TrimPrefix(req.URL.Path, "/v1/agent/check/deregister/")), nil)
+	ID, err := getPathSuffixUnescaped(req.URL.Path, "/v1/agent/check/deregister/")
+	if err != nil {
+		return nil, err
+	}
+	checkID := structs.NewCheckID(types.CheckID(ID), nil)
 
 	// Get the provided token, if any, and vet against any ACL policies.
 	var token string
@@ -816,13 +829,21 @@ func (s *HTTPHandlers) AgentDeregisterCheck(resp http.ResponseWriter, req *http.
 }
 
 func (s *HTTPHandlers) AgentCheckPass(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
-	checkID := types.CheckID(strings.TrimPrefix(req.URL.Path, "/v1/agent/check/pass/"))
+	ID, err := getPathSuffixUnescaped(req.URL.Path, "/v1/agent/check/pass/")
+	if err != nil {
+		return nil, err
+	}
+	checkID := types.CheckID(ID)
 	note := req.URL.Query().Get("note")
 	return s.agentCheckUpdate(resp, req, checkID, api.HealthPassing, note)
 }
 
 func (s *HTTPHandlers) AgentCheckWarn(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
-	checkID := types.CheckID(strings.TrimPrefix(req.URL.Path, "/v1/agent/check/warn/"))
+	ID, err := getPathSuffixUnescaped(req.URL.Path, "/v1/agent/check/warn/")
+	if err != nil {
+		return nil, err
+	}
+	checkID := types.CheckID(ID)
 	note := req.URL.Query().Get("note")
 
 	return s.agentCheckUpdate(resp, req, checkID, api.HealthWarning, note)
@@ -830,7 +851,11 @@ func (s *HTTPHandlers) AgentCheckWarn(resp http.ResponseWriter, req *http.Reques
 }
 
 func (s *HTTPHandlers) AgentCheckFail(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
-	checkID := types.CheckID(strings.TrimPrefix(req.URL.Path, "/v1/agent/check/fail/"))
+	ID, err := getPathSuffixUnescaped(req.URL.Path, "/v1/agent/check/fail/")
+	if err != nil {
+		return nil, err
+	}
+	checkID := types.CheckID(ID)
 	note := req.URL.Query().Get("note")
 
 	return s.agentCheckUpdate(resp, req, checkID, api.HealthCritical, note)
@@ -869,7 +894,12 @@ func (s *HTTPHandlers) AgentCheckUpdate(resp http.ResponseWriter, req *http.Requ
 		return nil, nil
 	}
 
-	checkID := types.CheckID(strings.TrimPrefix(req.URL.Path, "/v1/agent/check/update/"))
+	ID, err := getPathSuffixUnescaped(req.URL.Path, "/v1/agent/check/update/")
+	if err != nil {
+		return nil, err
+	}
+
+	checkID := types.CheckID(ID)
 
 	return s.agentCheckUpdate(resp, req, checkID, update.Status, update.Output)
 }
@@ -951,7 +981,10 @@ func returnTextPlain(req *http.Request) bool {
 // AgentHealthServiceByID return the local Service Health given its ID
 func (s *HTTPHandlers) AgentHealthServiceByID(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
 	// Pull out the service id (service id since there may be several instance of the same service on this host)
-	serviceID := strings.TrimPrefix(req.URL.Path, "/v1/agent/health/service/id/")
+	serviceID, err := getPathSuffixUnescaped(req.URL.Path, "/v1/agent/health/service/id/")
+	if err != nil {
+		return nil, err
+	}
 	if serviceID == "" {
 		return nil, &BadRequestError{Reason: "Missing serviceID"}
 	}
@@ -1009,7 +1042,11 @@ func (s *HTTPHandlers) AgentHealthServiceByID(resp http.ResponseWriter, req *htt
 // AgentHealthServiceByName return the worse status of all the services with given name on an agent
 func (s *HTTPHandlers) AgentHealthServiceByName(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
 	// Pull out the service name
-	serviceName := strings.TrimPrefix(req.URL.Path, "/v1/agent/health/service/name/")
+	serviceName, err := getPathSuffixUnescaped(req.URL.Path, "/v1/agent/health/service/name/")
+	if err != nil {
+		return nil, err
+	}
+
 	if serviceName == "" {
 		return nil, &BadRequestError{Reason: "Missing service Name"}
 	}
@@ -1237,7 +1274,12 @@ func (s *HTTPHandlers) AgentRegisterService(resp http.ResponseWriter, req *http.
 }
 
 func (s *HTTPHandlers) AgentDeregisterService(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
-	sid := structs.NewServiceID(strings.TrimPrefix(req.URL.Path, "/v1/agent/service/deregister/"), nil)
+	serviceID, err := getPathSuffixUnescaped(req.URL.Path, "/v1/agent/service/deregister/")
+	if err != nil {
+		return nil, err
+	}
+
+	sid := structs.NewServiceID(serviceID, nil)
 
 	// Get the provided token, if any, and vet against any ACL policies.
 	var token string
@@ -1272,7 +1314,12 @@ func (s *HTTPHandlers) AgentDeregisterService(resp http.ResponseWriter, req *htt
 
 func (s *HTTPHandlers) AgentServiceMaintenance(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
 	// Ensure we have a service ID
-	sid := structs.NewServiceID(strings.TrimPrefix(req.URL.Path, "/v1/agent/service/maintenance/"), nil)
+	serviceID, err := getPathSuffixUnescaped(req.URL.Path, "/v1/agent/service/maintenance/")
+	if err != nil {
+		return nil, err
+	}
+
+	sid := structs.NewServiceID(serviceID, nil)
 
 	if sid.ID == "" {
 		resp.WriteHeader(http.StatusBadRequest)
@@ -1485,7 +1532,10 @@ func (s *HTTPHandlers) AgentToken(resp http.ResponseWriter, req *http.Request) (
 	}
 
 	// Figure out the target token.
-	target := strings.TrimPrefix(req.URL.Path, "/v1/agent/token/")
+	target, err := getPathSuffixUnescaped(req.URL.Path, "/v1/agent/token/")
+	if err != nil {
+		return nil, err
+	}
 
 	err = s.agent.tokens.WithPersistenceLock(func() error {
 		triggerAntiEntropySync := false
@@ -1558,7 +1608,10 @@ func (s *HTTPHandlers) AgentConnectCARoots(resp http.ResponseWriter, req *http.R
 func (s *HTTPHandlers) AgentConnectCALeafCert(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
 	// Get the service name. Note that this is the name of the service,
 	// not the ID of the service instance.
-	serviceName := strings.TrimPrefix(req.URL.Path, "/v1/agent/connect/ca/leaf/")
+	serviceName, err := getPathSuffixUnescaped(req.URL.Path, "/v1/agent/connect/ca/leaf/")
+	if err != nil {
+		return nil, err
+	}
 
 	args := cachetype.ConnectCALeafRequest{
 		Service: serviceName, // Need name not ID

agent/http.go

@@ -1126,3 +1126,15 @@ func (s *HTTPHandlers) parseFilter(req *http.Request, filter *string) {
 		*filter = other
 	}
 }
+
+func getPathSuffixUnescaped(path string, prefixToTrim string) (string, error) {
+	// The suffix may be URL-encoded, so attempt to decode
+	suffixRaw := strings.TrimPrefix(path, prefixToTrim)
+	suffixUnescaped, err := url.PathUnescape(suffixRaw)
+
+	if err != nil {
+		return suffixRaw, fmt.Errorf("failure in unescaping path param %q: %v", suffixRaw, err)
+	}
+
+	return suffixUnescaped, nil
+}

agent/http_test.go

@@ -1680,3 +1680,42 @@ func TestRPC_HTTPSMaxConnsPerClient(t *testing.T) {
 		})
 	}
 }
+
+func TestGetPathSuffixUnescaped(t *testing.T) {
+	t.Parallel()
+
+	cases := []struct {
+		name         string
+		pathInput    string
+		pathPrefix   string
+		suffixResult string
+		errString    string
+	}{
+		// No decoding required (resource name must be unaffected by the decode)
+		{"Normal Valid", "/foo/bar/resource-1", "/foo/bar/", "resource-1", ""},
+		// This function is not responsible for enforcing a valid URL, just for decoding escaped values.
+		// If there's an invalid URL segment in the path, it will be returned as is.
+		{"Unencoded Invalid", "/foo/bar/resource 1", "/foo/bar/", "resource 1", ""},
+		// Decode the encoded value properly
+		{"Encoded Valid", "/foo/bar/re%2Fsource%201", "/foo/bar/", "re/source 1", ""},
+		// Fail to decode an invalidly encoded input
+		{"Encoded Invalid", "/foo/bar/re%Fsource%201", "/foo/bar/", "re%Fsource%201", "failure in unescaping path param"},
+	}
+
+	for _, tc := range cases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+
+			suffixResult, err := getPathSuffixUnescaped(tc.pathInput, tc.pathPrefix)
+
+			require.Equal(t, suffixResult, tc.suffixResult)
+
+			if tc.errString == "" {
+				require.NoError(t, err)
+			} else {
+				require.Error(t, err)
+				require.Contains(t, err.Error(), tc.errString)
+			}
+		})
+	}
+}

api/agent.go

@@ -707,7 +707,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/"+serviceID)
+	r := a.c.newRequest("GET", "/v1/agent/service/"+url.PathEscape(serviceID))
 	r.setQueryOptions(q)
 	rtt, resp, err := a.c.doRequest(r)
 	if err != nil {
@@ -812,7 +812,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/"+serviceID)
+	r := a.c.newRequest("PUT", "/v1/agent/service/deregister/"+url.PathEscape(serviceID))
 	_, resp, err := a.c.doRequest(r)
 	if err != nil {
 		return err
@@ -827,7 +827,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/"+serviceID)
+	r := a.c.newRequest("PUT", "/v1/agent/service/deregister/"+url.PathEscape(serviceID))
 	r.setQueryOptions(q)
 	_, resp, err := a.c.doRequest(r)
 	if err != nil {
@@ -884,7 +884,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", status, checkID)
+	endpoint := fmt.Sprintf("/v1/agent/check/%s/%s", url.PathEscape(status), url.PathEscape(checkID))
 	r := a.c.newRequest("PUT", endpoint)
 	r.params.Set("note", note)
 	_, resp, err := a.c.doRequest(r)
@@ -932,7 +932,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", checkID)
+	endpoint := fmt.Sprintf("/v1/agent/check/update/%s", url.PathEscape(checkID))
 	r := a.c.newRequest("PUT", endpoint)
 	r.setQueryOptions(q)
 	r.obj = &checkUpdate{
@@ -976,7 +976,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/"+checkID)
+	r := a.c.newRequest("PUT", "/v1/agent/check/deregister/"+url.PathEscape(checkID))
 	r.setQueryOptions(q)
 	_, resp, err := a.c.doRequest(r)
 	if err != nil {
@@ -992,7 +992,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/"+addr)
+	r := a.c.newRequest("PUT", "/v1/agent/join/"+url.PathEscape(addr))
 	if wan {
 		r.params.Set("wan", "1")
 	}
@@ -1044,7 +1044,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/"+node)
+	r := a.c.newRequest("PUT", "/v1/agent/force-leave/"+url.PathEscape(node))
 	if opts.Prune {
 		r.params.Set("prune", "1")
 	}
@@ -1108,7 +1108,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/"+serviceID)
+	r := a.c.newRequest("GET", "/v1/agent/connect/ca/leaf/"+url.PathEscape(serviceID))
 	r.setQueryOptions(q)
 	rtt, resp, err := a.c.doRequest(r)
 	if err != nil {
@@ -1136,7 +1136,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/"+serviceID)
+	r := a.c.newRequest("PUT", "/v1/agent/service/maintenance/"+url.PathEscape(serviceID))
 	r.setQueryOptions(q)
 	r.params.Set("enable", "true")
 	r.params.Set("reason", reason)
@@ -1158,7 +1158,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/"+serviceID)
+	r := a.c.newRequest("PUT", "/v1/agent/service/maintenance/"+url.PathEscape(serviceID))
 	r.setQueryOptions(q)
 	r.params.Set("enable", "false")
 	_, resp, err := a.c.doRequest(r)
@@ -1355,7 +1355,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", target))
+	r := a.c.newRequest("PUT", fmt.Sprintf("/v1/agent/token/%s", url.PathEscape(target)))
 	r.setWriteOptions(q)
 	r.obj = &AgentToken{Token: token}