VAULT-23433: Dealing with Incomplete link Object in Custom Messages (#25009)

* fix VAULT-23433 by dealing with incomplete link object in custom message definitions * addressing review feedback * further refactoring - moving duplicated map len check into validateLinkMap
2025-10-29 17:52:32 +00:00 · 2024-01-23 17:12:45 -05:00
parent a1295a54e8
commit 2d7f09cc11
2 changed files with 204 additions and 38 deletions
--- a/vault/logical_system_custom_messages.go
+++ b/vault/logical_system_custom_messages.go
@@ -428,25 +428,9 @@ func (b *SystemBackend) handleCreateCustomMessages(ctx context.Context, req *log
 		return logical.ErrorResponse(err.Error()), nil
 	}

-	if len(linkMap) > 1 {
-		return logical.ErrorResponse("invalid number of elements in link parameter value; only a single element can be provided"), nil
-	}
-
-	var link *uicustommessages.MessageLink
-	if linkMap != nil {
-		link = &uicustommessages.MessageLink{}
-
-		for k, v := range linkMap {
-			href, ok := v.(string)
-			if !ok {
-				return logical.ErrorResponse(fmt.Sprintf("invalid url for %q key in link parameter value", k)), nil
-			}
-
-			link.Title = k
-			link.Href = href
-
-			break
-		}
+	link, resp := validateLinkMap(linkMap)
+	if resp != nil {
+		return resp, nil
 	}

 	options, err := parameterValidateMap("options", d)
@@ -490,6 +474,50 @@ func (b *SystemBackend) handleCreateCustomMessages(ctx context.Context, req *log
 	}, nil
 }

+// validateLinkMap takes care of detecting either incomplete or invalid link
+// parameter value.
+// An invalid link parameter value is one where there's either
+// an empty string for the title key or the href value or the href value not
+// being a string. A linkMap that is invalid, results in only a logical.Response
+// containing an error response being returned.
+// An incomplete link parameter value is one where the linkMap is either nil or
+// empty, where the title key is an empty string and the href value is an empty
+// string. A linkMap that is incomplete, results in neither a MessageLink nor a
+// Response being returned.
+// If the linkMap is neither invalid nor incomplete, a MessageLink is returned.
+func validateLinkMap(linkMap map[string]any) (*uicustommessages.MessageLink, *logical.Response) {
+	if len(linkMap) > 1 {
+		return nil, logical.ErrorResponse("invalid number of elements in link parameter value; only a single element can be provided")
+	}
+
+	for k, v := range linkMap {
+		href, ok := v.(string)
+
+		switch {
+		case !ok:
+			// href value is not a string, so return an error
+			return nil, logical.ErrorResponse(fmt.Sprintf("invalid url for %q key in link parameter value", k))
+		case len(k) == 0 && len(href) > 0:
+			// no title key, but there's an href value, so return an error
+			return nil, logical.ErrorResponse("missing title key in link parameter value")
+		case len(k) > 0 && len(href) == 0:
+			// no href value, but there's a title key, so return an error
+			return nil, logical.ErrorResponse(fmt.Sprintf("missing url for %q key in link parameter value", k))
+		case len(k) != 0 && len(href) != 0:
+			// when title key and href value are not empty, return a MessageLink
+			// pointer
+			return &uicustommessages.MessageLink{
+				Title: k,
+				Href:  href,
+			}, nil
+		}
+
+	}
+
+	// no title key and no href value, treat it as if no link was specified
+	return nil, nil
+}
+
 // handleReadCustomMessage is the operation callback for the READ operation of
 // the custom messages endpoint.
 func (b *SystemBackend) handleReadCustomMessage(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
@@ -572,25 +600,9 @@ func (b *SystemBackend) handleUpdateCustomMessage(ctx context.Context, req *logi
 		return logical.ErrorResponse(err.Error()), nil
 	}

-	if len(linkMap) > 1 {
-		return logical.ErrorResponse("invalid number of elements in link parameter value; only a single element can be provided"), nil
-	}
-
-	var link *uicustommessages.MessageLink
-	if linkMap != nil {
-		link = &uicustommessages.MessageLink{}
-
-		for k, v := range linkMap {
-			href, ok := v.(string)
-			if !ok {
-				return logical.ErrorResponse("invalid url for %q key link parameter value", k), nil
-			}
-
-			link.Title = k
-			link.Href = href
-
-			break
-		}
+	link, resp := validateLinkMap(linkMap)
+	if resp != nil {
+		return resp, nil
 	}

 	options, err := parameterValidateMap("options", d)
--- a/vault/logical_system_custom_messages_test.go
+++ b/vault/logical_system_custom_messages_test.go
@@ -437,6 +437,160 @@ func TestHandleCreateCustomMessage(t *testing.T) {
 	assert.Contains(t, resp.Data, "error")
 }

+// TestHandleCreateAndUpdateCustomMessageLinkProperty verifies some particular
+// cases in the handleCreateCustomMessage method surrounding the link property
+// of the custom message. Because the link property is modeled as a JSON object,
+// the possibility of an empty or partially constructed object is provided, so
+// this test function verifies that each of these cases is appropriately
+// handled.
+func TestHandleCreateAndUpdateCustomMessageLinkProperty(t *testing.T) {
+	testBackend := &SystemBackend{
+		Core: &Core{
+			customMessageManager: uicustommessages.NewManager(&logical.InmemStorage{}),
+		},
+	}
+
+	fieldData := &framework.FieldData{
+		Schema: map[string]*framework.FieldSchema{
+			"title": {
+				Type:     framework.TypeString,
+				Required: true,
+			},
+			"message": {
+				Type:     framework.TypeString,
+				Required: true,
+			},
+			"authenticated": {
+				Type:    framework.TypeBool,
+				Default: true,
+			},
+			"type": {
+				Type:    framework.TypeString,
+				Default: "banner",
+			},
+			"start_time": {
+				Type:     framework.TypeTime,
+				Required: true,
+			},
+			"end_time": {
+				Type: framework.TypeTime,
+			},
+			"link": {
+				Type: framework.TypeMap,
+			},
+			"options": {
+				Type: framework.TypeMap,
+			},
+		},
+		Raw: map[string]any{
+			"title":      "The title",
+			"message":    "VGhlIG1lc3NhZ2UK",
+			"start_time": "2024-01-01T00:00:00.000Z",
+		},
+	}
+
+	for _, testcase := range []struct {
+		name                  string
+		linkField             any
+		responseErrorExpected bool
+	}{
+		// First group of testcases, setup an incomplete link parameter,
+		// which result in a CustomMessage with a Link field set to nil
+		{
+			name:      "nil-link",
+			linkField: nil,
+		},
+		{
+			name:      "empty-link-map",
+			linkField: map[string]any{},
+		},
+		{
+			name: "empty-key-and-value-link-map",
+			linkField: map[string]any{
+				"": "",
+			},
+		},
+		{
+			name: "empty-value-link-map",
+			linkField: map[string]any{
+				"Click me": "",
+			},
+		},
+		{
+			name: "empty-key-nil-value-link-map",
+			linkField: map[string]any{
+				"": nil,
+			},
+		},
+		// Second group of testcases, setup an invalid link parameter,
+		// which result in an error being returned and the message not being
+		// created or updated.
+		{
+			name: "nil-value-link-map",
+			linkField: map[string]any{
+				"Click me": nil,
+			},
+			responseErrorExpected: true,
+		},
+		{
+			name: "invalid-value-link-map",
+			linkField: map[string]any{
+				"Click me": []int{},
+			},
+			responseErrorExpected: true,
+		},
+		{
+			name: "empty-key-link-map",
+			linkField: map[string]any{
+				"": "https://www.example.org",
+			},
+			responseErrorExpected: true,
+		},
+	} {
+		t.Run(testcase.name, func(t *testing.T) {
+			delete(fieldData.Raw, "link")
+
+			// Create a custom message with no link to be used to test the
+			// handleUpdateCustomMessage case.
+			resp, err := testBackend.handleCreateCustomMessages(namespace.ContextWithNamespace(context.Background(), namespace.RootNamespace), &logical.Request{}, fieldData)
+			require.NoError(t, err)
+			require.Contains(t, resp.Data, "id")
+			updatableMessageId := resp.Data["id"]
+
+			// Update the fieldData to include the testcase
+			fieldData.Raw["link"] = testcase.linkField
+			fieldData.Schema["id"] = &framework.FieldSchema{
+				Type:     framework.TypeString,
+				Required: true,
+			}
+			fieldData.Raw["id"] = updatableMessageId
+
+			// Test the Update operation
+			resp, err = testBackend.handleUpdateCustomMessage(namespace.ContextWithNamespace(context.Background(), namespace.RootNamespace), &logical.Request{}, fieldData)
+			assert.NoError(t, err, "UpdateCustomMessage")
+
+			if testcase.responseErrorExpected {
+				assert.Contains(t, resp.Data, "error", "UpdateCustomMessage")
+			} else {
+				assert.Nil(t, resp.Data["link"], "UpdateCustomMessage")
+			}
+
+			// Reset for testing the Create operation
+			delete(fieldData.Schema, "id")
+			delete(fieldData.Raw, "id")
+
+			resp, err = testBackend.handleCreateCustomMessages(namespace.ContextWithNamespace(context.Background(), namespace.RootNamespace), &logical.Request{}, fieldData)
+			assert.NoError(t, err, "CreateCustomMessage")
+
+			if testcase.responseErrorExpected {
+				assert.Contains(t, resp.Data, "error", "CreateCustomMessage")
+			} else {
+				assert.Nil(t, resp.Data["link"], "CreateCustomMessage")
+			}
+		})
+	}
+}
+
 // TestHandleReadCustomMessage verifies the proper functioning of the
 // (*SystemBackend).handleReadCustomMessage method. The tests focus on missing
 // or invalid request parameters as well as reading existing and non-