From 7052ef8343a8f5987921ce5f5b5293b9c833a3ab Mon Sep 17 00:00:00 2001 From: Aravinda-HWK Date: Mon, 10 Aug 2026 22:54:56 +0530 Subject: [PATCH 1/2] fix(remote): do not append a trailing separator for an empty request path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Joining the base URL and the request path unconditionally inserted a "/", so an empty path produced "/" instead of "". A trailing slash addresses a different resource than the bare path, and some servers reject the trailing form outright — the IPPC ePhyto Hub answers it with 405. An empty (or "/") path means the caller is addressing the service URL itself, so send the base URL verbatim. Non-empty paths join as before, still collapsing a duplicated separator. --- remote/client.go | 9 ++++++++- remote/client_test.go | 31 +++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/remote/client.go b/remote/client.go index acf91b7..eb72f8d 100644 --- a/remote/client.go +++ b/remote/client.go @@ -181,7 +181,14 @@ func (c *Client) executeOnce(ctx context.Context, method, path string, body []by // 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 + // An empty path addresses the service URL itself, so post to it + // verbatim: appending a separator would request a different resource + // ("/svc/" is not "/svc") and some servers reject the trailing form. + if p == "" { + finalURL = base + } else { + finalURL = base + "/" + p + } } var bodyReader io.Reader diff --git a/remote/client_test.go b/remote/client_test.go index fa3b0b6..7cb107f 100644 --- a/remote/client_test.go +++ b/remote/client_test.go @@ -161,6 +161,37 @@ 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 (the IPPC ePhyto + // Hub among them) 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 + wantPath string + }{ + {"empty path posts to the service URL itself", "/hub/DeliveryService", "", "/hub/DeliveryService"}, + {"root path is treated as empty", "/hub/DeliveryService", "/", "/hub/DeliveryService"}, + {"non-empty path is appended", "/hub", "DeliveryService", "/hub/DeliveryService"}, + {"leading separator is not doubled", "/hub", "/DeliveryService", "/hub/DeliveryService"}, + } { + t.Run(tc.name, func(t *testing.T) { + var got string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + got = r.URL.Path + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + client := NewClient(server.URL + tc.basePath) + err := client.JSONRequest(context.Background(), Request{Method: "GET", Path: tc.path}, nil) + assert.NoError(t, err) + assert.Equal(t, tc.wantPath, got) + }) + } + }) } func TestClient_NoContentResponse(t *testing.T) { From 9430b00fcd1ee82a66144a6c2de583b8fa2ae265 Mon Sep 17 00:00:00 2001 From: Aravinda-HWK Date: Sat, 15 Aug 2026 10:46:54 +0530 Subject: [PATCH 2/2] refactor(remote): resolve request URLs with net/url instead of string joins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous fix concatenated the base URL and path by hand, which left two defects. A request carrying query parameters reached the join with the query already appended to the path (JSONRequest does that before calling), so an empty path arrived as "?id=42" — not empty, so the trailing separator came back and the 405 the fix targeted returned for every call with a query. Path segments were also passed through unescaped. Resolve with net/url instead: parse the base and the request path, join the escaped path with URL.JoinPath, and carry query and fragment on the URL rather than splicing them into a string. Query merges when both the base URL and the request supply one. EscapedPath is joined rather than Path, since joining the decoded form would turn an escaped separator ("x%2Fy") into a real one and address a different resource than the caller asked for. One rule stays explicit: a root-only path resolves to the base URL itself. JoinPath preserves a trailing separator by design, so "/" would otherwise produce "/" — a different resource that some servers reject, and a caller addressing the service URL itself has no other way to say so. --- remote/client.go | 102 +++++++++++++++++++++++++++++------------- remote/client_test.go | 91 +++++++++++++++++++++++++++++++------ 2 files changed, 148 insertions(+), 45 deletions(-) diff --git a/remote/client.go b/remote/client.go index eb72f8d..9eed42c 100644 --- a/remote/client.go +++ b/remote/client.go @@ -154,41 +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) - } - - provided, err := url.Parse(path) - if err != nil { - return nil, fmt.Errorf("remote: invalid absolute URL: %w", err) - } + if c.baseURL == "" { + return path, nil + } - // 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) - } + base, err := url.Parse(c.baseURL) + if err != nil { + return "", fmt.Errorf("remote: invalid base URL: %w", err) } - } 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, "/") - // An empty path addresses the service URL itself, so post to it - // verbatim: appending a separator would request a different resource - // ("/svc/" is not "/svc") and some servers reject the trailing form. - if p == "" { - finalURL = base - } else { - finalURL = base + "/" + p + 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 "", fmt.Errorf("remote: absolute URL host %q does not match configured service host %q", provided.Host, base.Host) } + 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 "/". That names a different resource than "" 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 diff --git a/remote/client_test.go b/remote/client_test.go index 7cb107f..e61448a 100644 --- a/remote/client_test.go +++ b/remote/client_test.go @@ -163,32 +163,95 @@ func TestClient_BaseURL_Logic(t *testing.T) { }) // An empty path addresses the service URL itself. Appending a separator - // would request a different resource, and some servers (the IPPC ePhyto - // Hub among them) answer the trailing form with 405. + // 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 - wantPath string + name string + basePath string + path string + query url.Values + wantPath string + wantQuery string }{ - {"empty path posts to the service URL itself", "/hub/DeliveryService", "", "/hub/DeliveryService"}, - {"root path is treated as empty", "/hub/DeliveryService", "/", "/hub/DeliveryService"}, - {"non-empty path is appended", "/hub", "DeliveryService", "/hub/DeliveryService"}, - {"leading separator is not doubled", "/hub", "/DeliveryService", "/hub/DeliveryService"}, + {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 got string + var gotPath, gotQuery string server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - got = r.URL.Path + // 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}, nil) + err := client.JSONRequest(context.Background(), Request{Method: "GET", Path: tc.path, Query: tc.query}, nil) assert.NoError(t, err) - assert.Equal(t, tc.wantPath, got) + assert.Equal(t, tc.wantPath, gotPath) + assert.Equal(t, tc.wantQuery, gotQuery) }) } })