From f4369c51e4897fa9bd6a525db7d18b5d20a1e5f1 Mon Sep 17 00:00:00 2001
From: viettranx <viettranx@gmail.com>
Date: Wed, 1 Apr 2026 11:59:21 +0700
Subject: [PATCH] fix(providers): make provider_type immutable + add argString
 tests
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Prevent SSRF bypass via provider_type change on update — ACP skips URL
validation, so changing type post-creation could circumvent the check.
Add table-driven unit tests for argString() covering all JSON type
coercion paths (float64, int, NaN, json.Number, nil).
---
 internal/http/providers.go     | 17 +++++------------
 internal/tools/message_test.go | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/internal/http/providers.go b/internal/http/providers.go
index a529c9ec..f4996fd4 100644
--- a/internal/http/providers.go
+++ b/internal/http/providers.go
@@ -376,15 +376,11 @@ func (h *ProvidersHandler) handleUpdateProvider(w http.ResponseWriter, r *http.R
 		}
 	}
 
-	// Validate provider_type if being updated.
-	// IMPORTANT: Do NOT replace this with delete(updates, "provider_type").
-	// We must return 400 so the caller knows the value is invalid,
-	// silently deleting it would hide the error from the end user.
-	if pt, ok := updates["provider_type"]; ok {
-		if s, _ := pt.(string); !store.ValidProviderTypes[s] {
-			writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgInvalidRequest, "unsupported provider_type")})
-			return
-		}
+	// provider_type is immutable after creation — changing it would bypass
+	// security checks (e.g. ACP skips URL validation). Reject if client tries.
+	if _, ok := updates["provider_type"]; ok {
+		writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgInvalidRequest, "provider_type cannot be changed after creation")})
+		return
 	}
 
 	// Strip masked API key — don't overwrite real value with "***"
@@ -407,9 +403,6 @@ func (h *ProvidersHandler) handleUpdateProvider(w http.ResponseWriter, r *http.R
 	if name, ok := updates["name"].(string); ok && name != "" {
 		candidate.Name = name
 	}
-	if providerType, ok := updates["provider_type"].(string); ok && providerType != "" {
-		candidate.ProviderType = providerType
-	}
 	if apiKey, ok := updates["api_key"].(string); ok {
 		candidate.APIKey = apiKey
 	}
diff --git a/internal/tools/message_test.go b/internal/tools/message_test.go
index 57b5e198..d0032880 100644
--- a/internal/tools/message_test.go
+++ b/internal/tools/message_test.go
@@ -2,6 +2,8 @@ package tools
 
 import (
 	"context"
+	"encoding/json"
+	"math"
 	"os"
 	"path/filepath"
 	"testing"
@@ -497,6 +499,37 @@ func TestMessageToolNumericTargetUsesSendPath(t *testing.T) {
 	}
 }
 
+func TestArgString(t *testing.T) {
+	tests := []struct {
+		name string
+		args map[string]any
+		key  string
+		want string
+	}{
+		{"string value", map[string]any{"k": "hello"}, "k", "hello"},
+		{"string with spaces", map[string]any{"k": "  hi  "}, "k", "hi"},
+		{"empty string", map[string]any{"k": ""}, "k", ""},
+		{"missing key", map[string]any{}, "k", ""},
+		{"nil value", map[string]any{"k": nil}, "k", ""},
+		{"float64 integer", map[string]any{"k": float64(-1001847298537)}, "k", "-1001847298537"},
+		{"float64 positive", map[string]any{"k": float64(42)}, "k", "42"},
+		{"float64 zero", map[string]any{"k": float64(0)}, "k", "0"},
+		{"float64 NaN", map[string]any{"k": math.NaN()}, "k", ""},
+		{"int", map[string]any{"k": 123}, "k", "123"},
+		{"int64", map[string]any{"k": int64(-999)}, "k", "-999"},
+		{"json.Number", map[string]any{"k": json.Number("7654321")}, "k", "7654321"},
+		{"bool fallback", map[string]any{"k": true}, "k", "true"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := argString(tt.args, tt.key)
+			if got != tt.want {
+				t.Errorf("argString(%v, %q) = %q, want %q", tt.args, tt.key, got, tt.want)
+			}
+		})
+	}
+}
+
 func TestDeliveredMedia(t *testing.T) {
 	dm := NewDeliveredMedia()