fix: error check to ensure path id doesn't conflict body id (#1067)

Co-authored-by: nic-chen <johz@163.com>

api/internal/core/store/store.go

@@ -327,7 +327,6 @@ func (s *GenericStore) StringToObjPtr(str, key string) (interface{}, error) {
 	objPtr := reflect.New(s.opt.ObjType)
 	ret := objPtr.Interface()
 	err := json.Unmarshal([]byte(str), ret)
-	fmt.Println("ret:", ret, "s.opt.ObjType", s.opt.ObjType)
 	if err != nil {
 		log.Errorf("json marshal failed: %s", err)
 		return nil, fmt.Errorf("json unmarshal failed: %s", err)

api/internal/handler/handler.go

@@ -35,13 +35,16 @@
 package handler
 
 import (
-	"github.com/shiningrush/droplet"
-	"github.com/shiningrush/droplet/middleware"
+	"fmt"
 	"net/http"
 	"strings"
 
 	"github.com/gin-gonic/gin"
+	"github.com/shiningrush/droplet"
 	"github.com/shiningrush/droplet/data"
+	"github.com/shiningrush/droplet/middleware"
+
+	"github.com/apisix/manager-api/internal/utils"
 )
 
 type RegisterFactory func() (RouteRegister, error)
@@ -87,3 +90,17 @@ func (mw *ErrorTransformMiddleware) Handle(ctx droplet.Context) error {
 	}
 	return nil
 }
+
+func IDCompare(idOnPath string, idOnBody interface{}) error {
+	idOnBodyStr, ok := idOnBody.(string)
+	if !ok {
+		idOnBodyStr = utils.InterfaceToString(idOnBody)
+	}
+
+	// check if id on path is == to id on body ONLY if both ids are valid
+	if idOnPath != "" && idOnBodyStr != "" && idOnBodyStr != idOnPath {
+		return fmt.Errorf("ID on path (%s) doesn't match ID on body (%s)", idOnPath, idOnBodyStr)
+	}
+
+	return nil
+}

api/internal/handler/handler_test.go

@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package handler
+
+import (
+	"errors"
+	"net/http"
+	"testing"
+
+	"github.com/go-playground/assert/v2"
+	"github.com/shiningrush/droplet/data"
+)
+
+func TestSpecCodeResponse(t *testing.T) {
+	err := errors.New("schema validate failed: remote_addr: Must validate at least one schema (anyOf)")
+	resp := SpecCodeResponse(err)
+	assert.Equal(t, &data.SpecCodeResponse{StatusCode: http.StatusBadRequest}, resp)
+
+	err = errors.New("data not found")
+	resp = SpecCodeResponse(err)
+	assert.Equal(t, &data.SpecCodeResponse{StatusCode: http.StatusNotFound}, resp)
+
+	err = errors.New("system error")
+	resp = SpecCodeResponse(err)
+	assert.Equal(t, &data.SpecCodeResponse{StatusCode: http.StatusInternalServerError}, resp)
+}
+
+func TestIDCompare(t *testing.T) {
+	// init
+	cases := []struct {
+		idOnPath, desc string
+		idOnBody       interface{}
+		wantError      error
+	}{
+		{
+			desc:     "ID on body is int, and it could be considered the same as ID on path",
+			idOnPath: "1",
+			idOnBody: 1,
+		},
+		{
+			desc:      "ID on body is int, and it is different from ID on path",
+			idOnPath:  "1",
+			idOnBody:  2,
+			wantError: errors.New("ID on path (1) doesn't match ID on body (2)"),
+		},
+		{
+			desc:     "ID on body is same as ID on path",
+			idOnPath: "1",
+			idOnBody: "1",
+		},
+		{
+			desc:      "ID on body is different from ID on path",
+			idOnPath:  "a",
+			idOnBody:  "b",
+			wantError: errors.New("ID on path (a) doesn't match ID on body (b)"),
+		},
+		{
+			desc:     "No ID on body",
+			idOnPath: "1",
+		},
+		{
+			desc:     "No ID on path",
+			idOnBody: 1,
+		},
+	}
+	for _, c := range cases {
+		t.Run(c.desc, func(t *testing.T) {
+			err := IDCompare(c.idOnPath, c.idOnBody)
+			assert.Equal(t, c.wantError, err)
+		})
+	}
+}

api/internal/handler/route/route.go

@@ -347,13 +347,15 @@ type UpdateInput struct {
 
 func (h *Handler) Update(c droplet.Context) (interface{}, error) {
 	input := c.Input().(*UpdateInput)
-	if input.ID != "" {
-		input.Route.ID = input.ID
+
+	// check if ID in body is equal ID in path
+	if err := handler.IDCompare(input.ID, input.Route.ID); err != nil {
+		return &data.SpecCodeResponse{StatusCode: http.StatusBadRequest}, err
 	}
 
-	if input.Route.Host != "" && len(input.Route.Hosts) > 0 {
-		return &data.SpecCodeResponse{StatusCode: http.StatusBadRequest},
-			fmt.Errorf("only one of host or hosts is allowed")
+	// if has id in path, use it
+	if input.ID != "" {
+		input.Route.ID = input.ID
 	}
 
 	//check depend

api/internal/handler/route/route_test.go

@@ -757,6 +757,86 @@ func TestRoute(t *testing.T) {
 	//sleep
 	time.Sleep(time.Duration(100) * time.Millisecond)
 
+	// check ID discrepancy on Update
+
+	// Fail: test the string body id value != string route id value
+	errRoute := &UpdateInput{}
+	errRoute.ID = "2"
+	err = json.Unmarshal([]byte(reqBody), errRoute)
+	assert.Nil(t, err)
+	ctx.SetInput(errRoute)
+	ret, err = handler.Update(ctx)
+	assert.NotNil(t, err)
+	assert.EqualError(t, err, "ID on path (2) doesn't match ID on body (1)")
+	assert.Equal(t, http.StatusBadRequest, ret.(*data.SpecCodeResponse).StatusCode)
+
+	// Fail: tests the float body id value != string route id value
+	reqBodyErr := `{
+		"id": 1,
+		"uri": "/index.html",
+		"upstream": {
+			"type": "roundrobin",
+			"nodes": [{
+				"host": "www.a.com",
+				"port": 80,
+				"weight": 1
+			}]
+		}
+	}`
+	errRoute = &UpdateInput{}
+	errRoute.ID = "2"
+	err = json.Unmarshal([]byte(reqBodyErr), errRoute)
+	assert.Nil(t, err)
+	ctx.SetInput(errRoute)
+	ret, err = handler.Update(ctx)
+	assert.NotNil(t, err)
+	assert.EqualError(t, err, "ID on path (2) doesn't match ID on body (1)")
+	assert.Equal(t, http.StatusBadRequest, ret.(*data.SpecCodeResponse).StatusCode)
+
+	// Success: tests the float body id value is == string route id value
+	reqBodyErr = `{
+		"id": 10,
+		"uri": "/index.html",
+		"upstream": {
+			"type": "roundrobin",
+			"nodes": [{
+				"host": "www.a.com",
+				"port": 80,
+				"weight": 1
+			}]
+		}
+	}`
+	errRoute = &UpdateInput{}
+	errRoute.ID = "10"
+	err = json.Unmarshal([]byte(reqBodyErr), errRoute)
+	assert.Nil(t, err)
+	ctx.SetInput(errRoute)
+	ret, err = handler.Update(ctx)
+	assert.Nil(t, err)
+
+	// Success: tests the Body ID can be nil
+	reqBodyErr = `{
+		"uri": "/index.html",
+		"upstream": {
+			"type": "roundrobin",
+			"nodes": [{
+				"host": "www.a.com",
+				"port": 80,
+				"weight": 1
+			}]
+		}
+	}`
+	errRoute = &UpdateInput{}
+	errRoute.ID = "r1"
+	err = json.Unmarshal([]byte(reqBodyErr), errRoute)
+	assert.Nil(t, err)
+	ctx.SetInput(errRoute)
+	ret, err = handler.Update(ctx)
+	assert.Nil(t, err)
+
+	//sleep
+	time.Sleep(time.Duration(100) * time.Millisecond)
+
 	//list
 	listInput := &ListInput{}
 	reqBody = `{"page_size": 1, "page": 1}`
@@ -1066,7 +1146,6 @@ func TestRoute(t *testing.T) {
 	ctx.SetInput(inputDel)
 	_, err = handler.BatchDelete(ctx)
 	assert.Nil(t, err)
-
 }
 
 func Test_Route_With_Script(t *testing.T) {
@@ -1211,4 +1290,13 @@ func Test_Route_With_Script(t *testing.T) {
 	assert.Nil(t, err)
 	assert.Equal(t, stored.ID, route.ID)
 	assert.Nil(t, stored.Script)
+
+	//delete test data
+	inputDel := &BatchDelete{}
+	reqBody = `{"ids": "1"}`
+	err = json.Unmarshal([]byte(reqBody), inputDel)
+	assert.Nil(t, err)
+	ctx.SetInput(inputDel)
+	_, err = handler.BatchDelete(ctx)
+	assert.Nil(t, err)
 }

api/internal/handler/service/service.go

@@ -179,6 +179,12 @@ type UpdateInput struct {
 
 func (h *Handler) Update(c droplet.Context) (interface{}, error) {
 	input := c.Input().(*UpdateInput)
+
+	// check if ID in body is equal ID in path
+	if err := handler.IDCompare(input.ID, input.Service.ID); err != nil {
+		return &data.SpecCodeResponse{StatusCode: http.StatusBadRequest}, err
+	}
+
 	if input.ID != "" {
 		input.Service.ID = input.ID
 	}

api/internal/handler/ssl/ssl.go

@@ -203,6 +203,12 @@ type UpdateInput struct {
 
 func (h *Handler) Update(c droplet.Context) (interface{}, error) {
 	input := c.Input().(*UpdateInput)
+
+	// check if ID in body is equal ID in path
+	if err := handler.IDCompare(input.ID, input.SSL.ID); err != nil {
+		return &data.SpecCodeResponse{StatusCode: http.StatusBadRequest}, err
+	}
+
 	ssl, err := ParseCert(input.Cert, input.Key)
 	if err != nil {
 		return &data.SpecCodeResponse{StatusCode: http.StatusBadRequest}, err

api/internal/handler/upstream/upstream.go

@@ -17,12 +17,14 @@
 package upstream
 
 import (
+	"net/http"
 	"reflect"
 	"strings"
 
 	"github.com/api7/go-jsonpatch"
 	"github.com/gin-gonic/gin"
 	"github.com/shiningrush/droplet"
+	"github.com/shiningrush/droplet/data"
 	"github.com/shiningrush/droplet/wrapper"
 	wgin "github.com/shiningrush/droplet/wrapper/gin"
 
@@ -161,6 +163,12 @@ type UpdateInput struct {
 
 func (h *Handler) Update(c droplet.Context) (interface{}, error) {
 	input := c.Input().(*UpdateInput)
+
+	// check if ID in body is equal ID in path
+	if err := handler.IDCompare(input.ID, input.Upstream.ID); err != nil {
+		return &data.SpecCodeResponse{StatusCode: http.StatusBadRequest}, err
+	}
+
 	if input.ID != "" {
 		input.Upstream.ID = input.ID
 	}

api/internal/handler/upstream/upstream_test.go

@@ -115,7 +115,7 @@ func TestUpstream(t *testing.T) {
 	upstream2 := &UpdateInput{}
 	upstream2.ID = "1"
 	reqBody = `{
-		"id": "aaa",
+		"id": "1",
 		"name": "upstream3",
 		"description": "upstream upstream",
 		"type": "roundrobin",

api/test/e2e/id_compatible_test.go

@@ -17,7 +17,6 @@
 package e2e
 
 import (
-	"fmt"
 	"io/ioutil"
 	"net/http"
 	"testing"
@@ -458,7 +457,6 @@ func TestID_Not_In_Body(t *testing.T) {
 	assert.Nil(t, err)
 	defer resp.Body.Close()
 	respBody, _ := ioutil.ReadAll(resp.Body)
-	fmt.Println("string(respBody)", string(respBody))
 	list := gjson.Get(string(respBody), "data.rows").Value().([]interface{})
 	for _, item := range list {
 		route := item.(map[string]interface{})