fix(remote): resolve request URLs with net/url so an empty path keeps no trailing separator

remote/client.go

@@ -154,34 +154,81 @@ func (c *Client) executeWithRetry(ctx context.Context, method, path string, body
 	return lastResp, lastErr
 }
 
-func (c *Client) executeOnce(ctx context.Context, method, path string, body []byte, extraHeaders map[string]string) (*http.Response, error) {
-	finalURL := path
-
-	// Handle URL construction and verification
+// resolveURL turns a caller-supplied path into the absolute URL to request.
+//
+// An absolute path is used verbatim once its scheme and host are checked
+// against the configured service. A relative one is resolved against the base
+// URL with net/url rather than string concatenation, so separators, escaping
+// and dot-segments follow the same rules the rest of the world uses.
+func (c *Client) resolveURL(path string) (string, error) {
 	if strings.HasPrefix(path, "http://") || strings.HasPrefix(path, "https://") {
-		// Absolute URL validation
-		if c.baseURL != "" {
-			base, err := url.Parse(c.baseURL)
-			if err != nil {
-				return nil, fmt.Errorf("remote: invalid base URL: %w", err)
-			}
+		if c.baseURL == "" {
+			return path, nil
+		}
 
-			provided, err := url.Parse(path)
-			if err != nil {
-				return nil, fmt.Errorf("remote: invalid absolute URL: %w", err)
-			}
+		base, err := url.Parse(c.baseURL)
+		if err != nil {
+			return "", fmt.Errorf("remote: invalid base URL: %w", err)
+		}
+		provided, err := url.Parse(path)
+		if err != nil {
+			return "", fmt.Errorf("remote: invalid absolute URL: %w", err)
+		}
 
-			// Ensure Scheme and Host match to prevent SSRF or credential leakage
-			if base.Scheme != provided.Scheme || base.Host != provided.Host {
-				return nil, fmt.Errorf("remote: absolute URL host %q does not match configured service host %q", provided.Host, base.Host)
-			}
+		// Ensure Scheme and Host match to prevent SSRF or credential leakage
+		if base.Scheme != provided.Scheme || base.Host != provided.Host {
+			return "", fmt.Errorf("remote: absolute URL host %q does not match configured service host %q", provided.Host, base.Host)
 		}
-	} else {
-		// Relative path handling
-		// Ensure baseURL ends with / and path doesn't start with / to avoid double slashes or missing slashes
-		base := strings.TrimSuffix(c.baseURL, "/")
-		p := strings.TrimPrefix(path, "/")
-		finalURL = base + "/" + p
+		return path, nil
+	}
+
+	base, err := url.Parse(c.baseURL)
+	if err != nil {
+		return "", fmt.Errorf("remote: invalid base URL: %w", err)
+	}
+	// Parsing splits the path from any query or fragment the caller appended to
+	// it (JSONRequest appends the encoded Query this way), so each part can be
+	// carried on the URL it belongs to instead of being spliced into a string.
+	ref, err := url.Parse(path)
+	if err != nil {
+		return "", fmt.Errorf("remote: invalid request path %q: %w", path, err)
+	}
+
+	// EscapedPath, not Path: joining the decoded form would turn an escaped
+	// separator ("x%2Fy") into a real one, silently addressing a different
+	// resource than the caller asked for.
+	p := ref.EscapedPath()
+
+	// JoinPath preserves a trailing separator, so a root-only path would
+	// produce "<base>/". That names a different resource than "<base>" and
+	// some servers reject it, and a caller reaching the service URL itself has
+	// no other way to say so — treat it as empty.
+	if p == "/" {
+		p = ""
+	}
+
+	u := base.JoinPath(p)
+
+	// A query on the base URL is unusual but not illegal, so merge rather than
+	// let either side silently win.
+	switch {
+	case ref.RawQuery == "":
+	case u.RawQuery == "":
+		u.RawQuery = ref.RawQuery
+	default:
+		u.RawQuery += "&" + ref.RawQuery
+	}
+	if ref.Fragment != "" {
+		u.Fragment = ref.Fragment
+	}
+
+	return u.String(), nil
+}
+
+func (c *Client) executeOnce(ctx context.Context, method, path string, body []byte, extraHeaders map[string]string) (*http.Response, error) {
+	finalURL, err := c.resolveURL(path)
+	if err != nil {
+		return nil, err
 	}
 
 	var bodyReader io.Reader

remote/client_test.go

@@ -161,6 +161,100 @@ func TestClient_BaseURL_Logic(t *testing.T) {
 		assert.Error(t, err2)
 		assert.Contains(t, err2.Error(), "does not match configured service host")
 	})
+
+	// An empty path addresses the service URL itself. Appending a separator
+	// would request a different resource, and some servers answer the trailing
+	// form with 405.
+	t.Run("relative path joins without a trailing separator", func(t *testing.T) {
+		for _, tc := range []struct {
+			name      string
+			basePath  string
+			path      string
+			query     url.Values
+			wantPath  string
+			wantQuery string
+		}{
+			{name: "empty path posts to the service URL itself", basePath: "/api/reports", path: "", wantPath: "/api/reports"},
+			{name: "root path is treated as empty", basePath: "/api/reports", path: "/", wantPath: "/api/reports"},
+			{name: "non-empty path is appended", basePath: "/api", path: "reports", wantPath: "/api/reports"},
+			{name: "leading separator is not doubled", basePath: "/api", path: "/reports", wantPath: "/api/reports"},
+
+			// Query parameters are appended to the path before it reaches the
+			// join, so the empty-path rule has to look past them: a path of
+			// "?a=1" still addresses the service URL itself.
+			{
+				name:      "empty path with query keeps the service URL itself",
+				basePath:  "/api/reports",
+				path:      "",
+				query:     url.Values{"id": {"42"}},
+				wantPath:  "/api/reports",
+				wantQuery: "id=42",
+			},
+			{
+				name:      "root path with query is treated as empty",
+				basePath:  "/api/reports",
+				path:      "/",
+				query:     url.Values{"id": {"42"}},
+				wantPath:  "/api/reports",
+				wantQuery: "id=42",
+			},
+			{
+				name:      "non-empty path with query is unaffected",
+				basePath:  "/api",
+				path:      "/reports",
+				query:     url.Values{"id": {"42"}},
+				wantPath:  "/api/reports",
+				wantQuery: "id=42",
+			},
+			{
+				name:      "query already in the path is preserved",
+				basePath:  "/api/reports",
+				path:      "?first=1",
+				query:     url.Values{"second": {"2"}},
+				wantPath:  "/api/reports",
+				wantQuery: "first=1&second=2",
+			},
+
+			// Resolution goes through net/url, so escaping and dot-segments
+			// follow the usual rules rather than whatever concatenation left.
+			{
+				name:     "an escaped separator is not turned into a real one",
+				basePath: "/api",
+				path:     "x%2Fy",
+				wantPath: "/api/x%2Fy",
+			},
+			{
+				name:     "a space in a segment is escaped",
+				basePath: "/api",
+				path:     "sp ace",
+				wantPath: "/api/sp%20ace",
+			},
+			{
+				name:     "dot segments are resolved before sending",
+				basePath: "/api/reports",
+				path:     "../other",
+				wantPath: "/api/other",
+			},
+		} {
+			t.Run(tc.name, func(t *testing.T) {
+				var gotPath, gotQuery string
+				server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+					// EscapedPath, not Path: the difference between a literal
+					// separator and an escaped one is the point of two cases here.
+					gotPath = r.URL.EscapedPath()
+					gotQuery = r.URL.RawQuery
+					w.WriteHeader(http.StatusOK)
+				}))
+				defer server.Close()
+
+				client := NewClient(server.URL + tc.basePath)
+				err := client.JSONRequest(context.Background(), Request{Method: "GET", Path: tc.path, Query: tc.query}, nil)
+				assert.NoError(t, err)
+				assert.Equal(t, tc.wantPath, gotPath)
+				assert.Equal(t, tc.wantQuery, gotQuery)
+			})
+		}
+	})
 }
 
 func TestClient_NoContentResponse(t *testing.T) {